Skip to content

[PM-40478] fix: Update Send tab visibility immediately after policy sync - #2917

Open
matt-livefront wants to merge 1 commit into
mainfrom
matt/PM-40478-send-disabled-tab
Open

[PM-40478] fix: Update Send tab visibility immediately after policy sync#2917
matt-livefront wants to merge 1 commit into
mainfrom
matt/PM-40478-send-disabled-tab

Conversation

@matt-livefront

@matt-livefront matt-livefront commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

PM-40478

📔 Objective

  • The Send tab's visibility (driven by the disableSend organization policy) previously took two syncs to update after the policy was toggled server-side. TabCoordinator recomputed visibility only when vaultRepository.organizationsPublisher() emitted, but SyncService.fetchSync() persists organizations before policies, so that emission always raced ahead of the policy data it was meant to reflect.
  • Adds SyncService.syncCompletePublisher(), a new multicast completion signal fired at the true end of a successful sync (alongside the existing onFetchSyncSucceeded delegate call), modeled on NotificationCenterService.didEnterBackgroundPublisher().
  • TabCoordinator now also subscribes to this publisher and rechecks the disableSend policy once a sync has fully settled, in addition to its existing organizations-driven subscription (which continues to drive the vault title unchanged).

📸 Screenshots

Before After
send-tab-before.mov
send-tab-after.mov

@matt-livefront
matt-livefront requested a review from a team as a code owner July 27, 2026 20:51
@matt-livefront matt-livefront added ai-review Request a Claude code review t:bug Change Type - Bug labels Jul 27, 2026
@github-actions github-actions Bot added the app:password-manager Bitwarden Password Manager app context label Jul 27, 2026
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new SyncService.syncCompletePublisher() signal, its emission point at the end of fetchSync(forceSync:isPeriodic:), and the TabCoordinator subscription that rechecks the disableSend policy once a sync has fully persisted. The root-cause analysis is correct: organizations are persisted before policies, so the organizations-driven emission could only ever observe pre-update policy data. Emission placement is correct — it fires only on the fully successful path, after all replace* calls, and is skipped on the early-return and error paths. Wiring through TabModule, MockSyncService, and the affected test setups is complete, and updateTabs(isSendEnabled:) remains idempotent so the additional subscription cannot thrash the tab bar.

Code Review Details
  • 🎨 : streamOrganizations() still performs a now-redundant disableSend check using pre-policy-persist data, which can race the new sync-complete update
    • BitwardenShared/UI/Platform/Tabs/TabCoordinator.swift:252

One additional low-severity note, mentioned here rather than inline: syncCompleteSubject is a CurrentValueSubject seeded with (), so every new subscriber immediately receives one synthetic completion event — this is harmless (and useful) for the current consumer and for test determinism, but the replay behavior isn't reflected in the syncCompletePublisher() protocol documentation.

Comment on lines +252 to +263
/// Streams sync-complete events, re-checking the Send policy and updating tab visibility once
/// a sync has fully persisted (organizations and policies both included).
private func streamSyncComplete() {
syncCompleteStreamTask = Task { [policyService, syncService] in
for await _ in syncService.syncCompletePublisher() {
let isSendDisabled = await policyService.policyAppliesToUser(.disableSend)
await MainActor.run { [weak self] in
self?.updateTabs(isSendEnabled: !isSendDisabled)
}
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 SUGGESTED: streamOrganizations() still runs its own disableSend check, which can race this one.

Details and rationale

streamOrganizations() (lines 241-244) computes isSendDisabled from the organizations emission, which — as the PR objective describes — fires before replacePolicies/replacePoliciesNew have run, so it reads the pre-update policy value and calls updateTabs(isSendEnabled:) with a stale result.

Both stream tasks independently await policyService.policyAppliesToUser(.disableSend) and then hop to the main actor, so the delivery order of the two updateTabs calls isn't guaranteed. If the organizations task's hop lands after the sync-complete task's, the stale value wins and the tab bar stays wrong until the next sync — the same symptom this PR fixes.

Since fetchSync is the only writer of policy data (replacePolicies and replacePoliciesNew have no other callers), the new sync-complete stream already covers every policy change. Dropping the policy check from streamOrganizations() would leave that stream driving only the vault title (as described in the PR objective) and remove the possibility of a stale write:

let canShowVaultFilter = await vaultRepository.canShowVaultFilter()
if organizations.isEmpty || !canShowVaultFilter {
    navigator.rootViewController?.title = Localizations.myVault
} else {
    navigator.rootViewController?.title = Localizations.vaults
}
// policy check + updateTabs removed; handled by streamSyncComplete()

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.09%. Comparing base (58ef05a) to head (fbaa2d9).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2917      +/-   ##
==========================================
- Coverage   81.10%   81.09%   -0.01%     
==========================================
  Files        1038     1038              
  Lines       67332    67356      +24     
==========================================
+ Hits        54608    54623      +15     
- Misses      12724    12733       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant