Repository navigation
Fix sidebar status pills for newly created workspaces - #5662
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Equatable sidebar snapshot types, moves sidebar publishers to snapshot-driven CombineLatest pipelines with removeDuplicates(), wires new source and test files into the Xcode project, and updates tests to reset counters and assert late-subscriber and heartbeat-only behaviors. ChangesSidebar observation and tests
sequenceDiagram
participant Workspace
participant SidebarPublisher as "makeSidebarObservationPublisher"
participant ImmediatePublisher as "makeSidebarImmediateObservationPublisher"
participant TestSubscriber as "Unit tests / UI"
Workspace->>SidebarPublisher: combineLatest workspace fields, status, git, remote, listeningPorts
Workspace->>ImmediatePublisher: combineLatest title/pinning/color + latest conversation fields
SidebarPublisher->>TestSubscriber: emit Void when SidebarObservationState changes
ImmediatePublisher->>TestSubscriber: emit Void when SidebarImmediateObservationState changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (19 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 |
Greptile SummaryReplaces the
Confidence Score: 5/5Safe to merge — the fix is structurally correct and replaces a timing-repair side-channel with canonical current-value observation. The switch from MergeMany + dropFirst to CombineLatest + removeDuplicates directly addresses the root cause: CombineLatest emits the combined current state immediately on subscription, so a sidebar row that subscribes after workspace status has already been mutated still receives the correct initial snapshot. All Equatable conformances required by the new state structs were verified in place. Existing tests are correctly updated to reset their counters after subscription, and three new deterministic tests cover the late-subscriber, immediate-field, and heartbeat-ignore cases. No bad state is left representable and no timing repair is needed. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant SidebarRow
participant LazyPublisher as sidebarObservationPublisher (lazy var)
participant CombineLatest as CombineLatest chain
participant Published as @Published properties
Note over SidebarRow,Published: Late-subscriber scenario (post-fix)
Published->>Published: statusEntries mutated before subscribe
SidebarRow->>LazyPublisher: ".sink { refreshWorkspaceSnapshot() }"
LazyPublisher->>CombineLatest: create subscription
CombineLatest->>Published: "subscribe to all @Published fields"
Published-->>CombineLatest: emit current values immediately (CombineLatest semantics)
CombineLatest->>CombineLatest: build SidebarObservationState(current values)
CombineLatest->>CombineLatest: removeDuplicates() — state is new, passes through
CombineLatest-->>SidebarRow: emit () — initial sync event
SidebarRow->>SidebarRow: refreshWorkspaceSnapshot()
Note over SidebarRow,Published: Subsequent mutation
Published->>Published: statusEntries mutated again
Published-->>CombineLatest: emit new value
CombineLatest->>CombineLatest: build updated SidebarObservationState
CombineLatest->>CombineLatest: removeDuplicates() — state changed, passes
CombineLatest-->>SidebarRow: emit ()
SidebarRow->>SidebarRow: refreshWorkspaceSnapshot()
Note over SidebarRow,Published: Heartbeat-only change (no invalidation)
Published->>Published: remoteHeartbeatCount / remoteLastHeartbeatAt mutated
Note over CombineLatest: These fields are NOT in SidebarObservationState
CombineLatest--xSidebarRow: no emission
Reviews (5): Last reviewed commit: "Mark sidebar observation tests main acto..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return Publishers.MergeMany(publishers) | ||
| // Rows cache an immutable sidebar snapshot. Emit once per subscription | ||
| // so rows mounted after metadata already changed still rebuild from | ||
| // the workspace's current state instead of waiting for another change. | ||
| .prepend(()) | ||
| .eraseToAnyPublisher() |
There was a problem hiding this comment.
.prepend(()) ensures every subscriber gets one event but does not structurally prevent a stale snapshot: the row still relies on receiving an event at all in order to call refreshWorkspaceSnapshot(), so any future code path that skips subscription or lets the cancellable go out of scope early would silently render stale state. The structural fix would be to have the snapshot cache eagerly populate from workspace state when the lazy publisher is first accessed (or when the subscription begins), making the initial value the canonical representation rather than a side-channel emit. As written, the invariant "snapshot is current before display" is still enforced only by the existence of this prepend.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Agreed. I replaced the .prepend(()) timing repair with current-state observation publishers for both sidebarObservationPublisher and sidebarImmediateObservationPublisher.
The publishers now compose the relevant @Published current values into Equatable sidebar-visible state, removeDuplicates() that state, and only then map to the existing Void invalidation signal. That makes the first subscriber emission canonical state from the workspace itself instead of a synthetic side-channel event, while preserving the row snapshot boundary and the existing refreshWorkspaceSnapshot() mutation path.
I also kept the late status-subscriber regression and added coverage for a late immediate-field subscriber, with the older mutation tests reset after subscription so they still prove subsequent mutations invalidate the sidebar.
This branch was successfully deployed
1 participant
Summary
Workspace.sidebarObservationPublisheremit one initial sync event per subscriber before normal debounced updates.Root cause
New workspace rows could cache an empty
workspaceSnapshotStoragebefore theirtab.sidebarObservationPublishersubscription became active. Because the workspace publisher dropped the first@Publishedvalue, a publisher materialized after the first status mutation treated the already-mutated status dictionary as its initial value and dropped it, leaving the row stuck on the empty cached snapshot. This rules out ForEach identity and status filtering: the model data was present and the snapshot builder renders it when refreshed.Testing
Closes #5659
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Ensure sidebar status pills render for newly created workspaces by emitting the current workspace state to new subscribers. Addresses #5659 and prevents late-subscribing rows from caching empty snapshots.
sidebarImmediateObservationPublisherandsidebarObservationPublisherto emit current@Publishedvalues withremoveDuplicates, so each subscriber gets an initial event; madeWorkspaceRemoteConnectionStateEquatableand moved helpers toWorkspaceSidebarObservation.swift.cmux/cmux_DEV, and marked tests@MainActorfor stable execution.Written for commit 3c6fe02. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests