Sign up
View stargazersSign up

refactor(client): drop the VCS wake and dispatcher element scope options nobody uses#25152

merged2
main
chore/trim-effect-scaffolding
opened2d agobyJJulius Marminge
4 files changed
View on GitHubTwo-way sync enabledPR last imported 2d ago

Trimmed: the cloud account service options are no longer removed here. The mobile cloud account service in #25150 uses prepare, onSignOutTransition, source/wasActive, isAccountTransition, getState and the Failed state, and landing both PRs as first written would have broken main's typecheck. This PR now only removes the VCS wake support and the dispatcher element scopes, which have no callers. Any text below about the cloud account service no longer applies.

An external review of the effect-elimination extractions flagged API surface added for consumers that never arrived. Each option below had no caller on main (checked with git grep on github/main), so this removes it along with the tests that only exercised it.

  • Cloud account service (#24974, packages/client-runtime/src/relay/cloudAccountService.ts). The only consumer is web's managedAuth.tsx, which passes deactivate, cleanUp, activate and detach. Removed: the prepare handler, the Failed state and its retry on the next emission, onSignOutTransition, isAccountTransition, cleanUp's source/wasActive inputs, and getState (only tests read it). Internal state is now just the observed account id plus a generation counter.
  • VCS status revalidation (#24975, packages/client-runtime/src/state/vcsStatusRevalidation.ts). The only consumer, mobile's use-selected-thread-git-actions.ts, passes connected and onRefreshed. Removed: subscribeWake, wakeDebounceMs, and the wake atom with its timer. connected is now required, since the only caller always passes it.
  • Shortcut dispatcher (#25016, apps/web/src/shortcuts/dispatcher.ts). Removed: commandScopeRef and the element scope list. App code only uses registerCommands and registerMenuAction. I kept the keybindings option even though only tests pass it, because the review didn't ask about it.

Behavior change: if cleanUp rejects while switching accounts, the service used to mark activation Failed and retry it on Clerk's next emission. Now the rejection is reported and the new account isn't activated until the account changes again. Web's cleanUp settles all of its work and reports failures itself, so it never rejects and this path can't be reached today. Nothing else changes.

Checks: vp run check:changed passes (lint, knip, typecheck, tests next to the changes).

Model: Claude Opus 5.5, Claude Code.

🤖 Generated with Claude Code

Checks

23 successful · 15 skipped
All checks passed
3d9b3cb
MmacroscopeappBot2d ago
Synced from GitHub

Approvability

Verdict: Approved at 3d9b3cb

Macroscope's review found this PR approvable — This is a narrowly scoped cleanup removing unused dispatcher element scopes and VCS wake/debounce plumbing, with no production callers for the removed paths. Existing shortcut and VCS reconnect behavior remains intact, and the associated tests are updated to cover the retained APIs.

Notes:

  • All code in this push has already been reviewed. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot2d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between 1a13127c15b24132a896eb72fda060d36f705c08 and c0bc248380a4d53ee2cf6a8fa7ed0ef96eefc27c.

External image Files selected for processing: 6
  • apps/web/src/shortcuts/dispatcher.browser.test.ts
  • apps/web/src/shortcuts/dispatcher.ts
  • packages/client-runtime/src/relay/cloudAccountService.test.ts
  • packages/client-runtime/src/relay/cloudAccountService.ts
  • packages/client-runtime/src/state/vcsStatusRevalidation.test.ts
  • packages/client-runtime/src/state/vcsStatusRevalidation.ts

Limit details: You’ve used all 10 included reviews currently available.

Walkthrough

This pull request changes shortcut dispatching, cloud account service behavior, and VCS status revalidation. It removes element-scoped command registration and wake-triggered refreshes, and updates cloud account transitions to use observed-account tracking and queued cleanup.

Changes: Shortcut dispatch

Layer / File(s)Summary
Global command dispatch
apps/web/src/shortcuts/dispatcher.ts, apps/web/src/shortcuts/dispatcher.browser.test.ts		dispatchKeyDown now runs global command scopes only, and commandScopeRef was removed. Browser tests register commands globally and retain coverage of precedence, fallthrough, prevented-default handling, raw keys, and observers. The element-scope-versus-global precedence test was removed.

Cloud account service

Layer / File(s)Summary
Account observation and queued transitions
packages/client-runtime/src/relay/cloudAccountService.ts, packages/client-runtime/src/relay/cloudAccountService.test.ts		The service tracks the observed account and queues cleanup and activation. It deactivates credentials on sign-out and account switches, and skips queued activation when a later account change supersedes it. The service API removes state access and transition and preparation callbacks. Tests cover cleanup ordering and update transition expectations.

VCS status revalidation

Layer / File(s)Summary
Mount and reconnect refresh
packages/client-runtime/src/state/vcsStatusRevalidation.ts, packages/client-runtime/src/state/vcsStatusRevalidation.test.ts		The atom family now requires a connection flag and refreshes on mount and when the connection becomes true. Wake subscriptions, debounce configuration, and the wake-signal test were removed.

Priority: External image Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant AccountChanges
3 participant CloudAccountService
4 participant Credentials
5 participant Cleanup
6 AccountChanges->>CloudAccountService: emit account change
7 CloudAccountService->>Credentials: deactivate credentials
8 CloudAccountService->>Cleanup: queue account cleanup

Suggested labels: size:M

Suggested reviewers: macroscopeapp

Merge Risk: External image Minimal · up to c0bc2

The changed client workflows have no identified merge-blocking issue and are ready for normal checks before merging.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Title checkPassedThe title clearly identifies the two main changes in the actual diff: removal of VCS wake support and unused dispatcher element-scope options.
Description checkPassedThe description explains the rationale, affected components, behavior change, scope, and focused verification. It includes issue references and test results. However, it retains a detailed cloud accou…
Full details: Description check

Explanation

The description explains the rationale, affected components, behavior change, scope, and focused verification. It includes issue references and test results. However, it retains a detailed cloud account service section that does not match the actual changes and may confuse reviewers.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Added 2 commits

  • refactor(client): drop unused options left for future callers34243cd
  • refactor(client): keep the cloud account service options mobile needs3d9b3cb