Repository navigation
Keep iOS Mac avatar color slots stable during Mac switches - #7858
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR keeps iOS Mac avatar color slots stable while switching active Macs. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "Drop stale budget entry for MobileShellC..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📝 WalkthroughWalkthrough
ChangesStable Mac color assignments
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MobileShellComposite
participant MobileWorkspaceAggregation
participant WorkspacePreviews
MobileShellComposite->>MobileWorkspaceAggregation: update stable slots from existing assignments and Mac IDs
MobileWorkspaceAggregation-->>MobileShellComposite: return updated slot map
MobileShellComposite->>MobileWorkspaceAggregation: derive workspaces with stable machineColorIndex
MobileWorkspaceAggregation->>WorkspacePreviews: stamp previews with owner Mac slots
WorkspacePreviews-->>MobileShellComposite: return derived workspace previews
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceAggregationTests.swift (1)
48-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency: use the
derivedWorkspaceshelper for consistency.Line 56 calls
MobileWorkspaceAggregation().derivedWorkspaces(...)directly while all other tests in this suite were migrated to use thederivedWorkspaces(statesByMac:foregroundMacDeviceID:)helper. Consider using the helper here too for consistency.♻️ Optional refactor for consistency
- let derived = MobileWorkspaceAggregation().derivedWorkspaces( - statesByMac: states, - foregroundMacDeviceID: "mac-a", - machineColorIndex: idx - ) + let derived = derivedWorkspaces(statesByMac: states, foregroundMacDeviceID: "mac-a")Note: the helper computes its own color index from scratch, so if you need to assert against the explicit
idxvalues, keep the direct call. The current assertions only checkidx["mac-a"]andidx["mac-b"]against the derived results, so using the helper would still work if you computeidxvia themachineColorIndexhelper first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceAggregationTests.swift` around lines 48 - 66, Use the suite’s derivedWorkspaces(statesByMac:foregroundMacDeviceID:) helper instead of calling MobileWorkspaceAggregation().derivedWorkspaces(...) directly in distinctMacsGetDistinctColorIndicesAndSameMacShares. Preserve the explicit machine color index assertions by computing idx with the existing machineColorIndex helper and ensuring the helper-derived results remain consistent with those values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@Packages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceAggregationTests.swift`:
- Around line 48-66: Use the suite’s
derivedWorkspaces(statesByMac:foregroundMacDeviceID:) helper instead of calling
MobileWorkspaceAggregation().derivedWorkspaces(...) directly in
distinctMacsGetDistinctColorIndicesAndSameMacShares. Preserve the explicit
machine color index assertions by computing idx with the existing
machineColorIndex helper and ensuring the helper-derived results remain
consistent with those values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53d68e8f-fd12-490a-b1b3-9ff61c7f12e1
📒 Files selected for processing (3)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceAggregation.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceAggregationTests.swift
MobileWorkspaceAggregation.machineColorIndex now takes a state parameter that grew MobileShellComposite.swift by 15 lines, past the CI budget's hard cap (files already over the 900-line default cap must have zero net growth vs. origin/main, independent of the checked-in budget value). Move updateStableMacColorSlots() into the existing MobileShellComposite+MacSwitchState.swift extension file (same pattern already used there for isMacSwitchInFlight) and combine two short declarations with semicolons, matching existing precedent elsewhere in the file (see effectiveViewportSizesBySurfaceID). Net line count vs. origin/main is now zero; no behavior change.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a246305. Configure here.
added 2 commits
July 12, 2026 19:07
`stableMacColorSlots` is additive-only, so sign-out and team switches leak the previous account/team's Mac→color assignments instead of resetting them. These tests currently fail without the fix.
updateStableMacColorSlots() only ever adds entries (that's the fix for the switching-color-flash bug), so nothing previously cleared it. On sign-out the next account inherited the old account's Mac→color assignments; on a team switch the old team's background Macs lingered in the slot map forever, permanently shifting new Macs' color numbers upward. Both resets have to run after every didSet-triggering assignment in their function: signOut()'s earlier `foregroundMacDeviceID = nil` and currentTeamDidChange()'s `workspacesByMac = ...filter` each trigger updateStableMacColorSlots() against the still-stale workspacesByMac, so a reset placed before them gets silently repopulated.
| didSet { recomputeDerivedWorkspaceState() } | ||
| } | ||
| private let workspaceAggregation = MobileWorkspaceAggregation() | ||
| let workspaceAggregation = MobileWorkspaceAggregation(); var stableMacColorSlots: [String: Int] = [:] // see MobileShellComposite+MacSwitchState.swift |
Contributor
There was a problem hiding this comment.
stableMacColorSlots still starts empty and is only filled when workspacesByMac fires its didSet. The initializer assigns the initial workspace seed directly, so that observer does not run during construction. If a shell is created with real Mac workspace state, the first read of machineColorIndex can still return an empty map until a later workspace mutation happens, so the first Computers render can show missing or fallback avatar colors before the stable slots appear. Seed this map from the initial workspacesByMac before the shell is exposed.
added 4 commits
July 12, 2026 19:20
MobileShellComposite.swift grew by 12 lines for the sign-out/team-switch reset, and MobileShellCompositePreviewTests.swift crossed the 500-line tracking threshold with its new regression tests. Both are real, reviewed growth, not incidental bloat, so refresh the budget rather than split either file.
…color-flash # Conflicts: # .github/swift-file-length-budget.tsv
MobileShellComposite.swift is already well past its 900-line hard cap on main, which bans any growth at all regardless of the checked-in budget number. Move the reset bodies into MobileShellComposite+MacSwitchState.swift (already under threshold, and already used for exactly this purpose) and collapse the two call sites onto existing lines, bringing the file back to main's exact line count. MobileShellCompositePreviewTests.swift crossing 500 lines for the first time is also an unconditional failure, not something a budget entry can accept. Split the two new color-slot regression tests into a sibling MobileShellCompositeColorSlotResetTests.swift, matching the codebase's established split-on-threshold-crossing convention, and revert the shared signOutReturnsToPreviewHostState test to its pre-fix form.
The file dropped back to 488 lines (under the 500-line tracking threshold) after the color-slot regression tests moved to their own sibling file, so it no longer needs a budget row.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MobileWorkspaceAggregation.machineColorIndexranked each Mac's color slot by alphabetical position among whatever keys currently existed in the liveworkspacesByMacdictionary. During a switch,dropStalePreviousForegroundsynchronously drops the old foreground Mac's key before the new Mac's siblings are re-added asynchronously, so the dictionary briefly holds exactly one key — collapsing that Mac's rank to slot 0 ([Color.blue, Color.cyan]), regardless of its real assigned color.MobileShellComposite.stableMacColorSlots), instead of being recomputed from the live key set on every recompute.MobileWorkspaceAggregation.machineColorIndexis now a pure(existingAssignments, adding) -> updatedAssignmentsfunction immune to live key-set churn.Test plan
swift testinPackages/iOS/CmuxMobileShellModel— 95/95 passing, including a new regression test that reproduces the switch-churn pattern (mutating the live key set while asserting existing Macs' indices never change).d924896adds the failing regression test only,c35d9dfeadds the fix.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
In-memory UI color mapping only; bounded resets on sign-out/team switch with dedicated tests. No auth, networking, or data persistence changes.
Overview
Fixes a one-frame avatar color flash when switching the active Mac on iOS. Per-Mac color slots are no longer derived from alphabetical rank among whatever keys are currently in
workspacesByMacon each recompute (which briefly collapses to a single key mid-switch).MobileShellCompositenow keepsstableMacColorSlotsfor the shell lifetime:updateStableMacColorSlots()runs before workspace derivation,machineColorIndexexposes that map, andderivedWorkspacestakes the precomputed index instead of recomputing from the live key set. Sign-out clears all slots; team switch prunes to the foreground Mac only so old-team Macs do not leak into numbering while the foreground color stays stable.MobileWorkspaceAggregation.machineColorIndexis now additive—(existingAssignments, adding)preserves prior slots and assigns new Macs deterministically. Regression tests cover switch churn, sign-out, and team-switch boundaries.Reviewed by Cursor Bugbot for commit 5a49608. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes the one-frame avatar color flash on iOS when switching Macs by assigning stable per-Mac color slots for the shell lifetime, and resets those slots on sign-out/team switches to prevent color leakage.
stableMacColorSlotsinMobileShellCompositeand update it before deriving UI state.MobileWorkspaceAggregation.machineColorIndex(existingAssignments:adding:); skips empty/anonymous IDs.machineColorIndexintoderivedWorkspacesto keep previews consistent during switch churn.stableMacColorSlotson sign-out and on team switch; keep only the foreground Mac’s slot across a team switch to avoid a color flash.MobileShellComposite+MacSwitchState.swiftand split color-slot reset tests intoMobileShellCompositeColorSlotResetTests.swiftto stay within CI’s file-length budget; no behavior change.Written for commit 5a49608. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests