Skip to content

Fix sidebar tab live presentation invalidating equatable rows - #4690

Closed
austinywang wants to merge 13 commits into
mainfrom
issue-4618-tabitemview-equatable
Closed

austinywang wants to merge 13 commits into
mainfrom
issue-4618-tabitemview-equatable

Conversation

@austinywang

@austinywang austinywang commented May 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Move sidebar workspace unread badge, notification subtitle/remote detail, and shortcut/close trailing accessory into focused live child views.
  • Remove live notification and shortcut-hint values from the heavy TabItemView equality inputs so .equatable() protects the static row body.
  • Keep context-menu shortcut-hint freezing by snapshotting the current live presentation only when the menu appears.

Fixes #4618

Testing

  • Not run locally per task instructions: no local tests, no xcodebuild, and no ./scripts/reload.sh.
  • No failing regression test commit: the bug is the SwiftUI .equatable() body-invalidation boundary and there is no clean runtime harness in this repo to assert body evaluation without source-shape tests or UI launches.

HQ build command

CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-4618-tabitemview-equatable --launch


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.


Note

Medium Risk
Touches high-traffic sidebar rendering and notification/shortcut presentation; behavior should match prior UI but equatable boundaries can hide stale state if miswired.

Overview
Fixes sidebar performance (#4618) by keeping TabItemView’s .equatable() boundary on mostly static row data while live UI still updates in small child views.

Unread badges, notification/conversation subtitles, SSH remote lines, close button, and workspace shortcut hints are split into SidebarWorkspaceUnreadBadge, SidebarWorkspaceLiveDetails, SidebarWorkspaceTrailingAccessory, and SidebarWorkspaceShortcutHintOverlay, each observing TerminalNotificationStore or modifierKeyMonitor locally. The parent no longer precomputes unreadCount, latestNotificationText, or showsModifierShortcutHints for equality; it passes frozenShortcutHintsTabId / frozenShortcutHintsValue per row instead.

Context-menu shortcut-hint freeze now snapshots the live modifier state on menu appear (not a pre-resolved bool). sidebarLatestNotificationText centralizes subtitle text with trimmed body, then title fallback.

Reviewed by Cursor Bugbot for commit a8ef325. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Fixes #4618 by isolating live sidebar tab UI into small observing views and freezing only shortcut-hint state during context menus so .equatable() rows avoid full re-renders. Typing is smoother; unread badges, notification subtitles (MainActor-trimmed with body→title fallback), SSH status, and close/shortcut accessories update live.

  • Bug Fixes

    • Keep .equatable() rows stable by moving unread badge, latest-notification/conversation subtitle, remote status, and trailing close/shortcut UI into observing child views; shortcut hint now draws via an overlay that doesn’t affect layout.
    • Freeze only modifier-key shortcut hints while a context menu is open by snapshotting modifierKeyMonitor.isModifierPressed on appear and scoping frozen tab/value to the active row.
  • Refactors

    • Added SidebarWorkspaceUnreadBadge, SidebarWorkspaceLiveDetails, SidebarWorkspaceTrailingAccessory, and SidebarWorkspaceShortcutHintOverlay; pass modifierKeyMonitor into TabItemView; removed live values from equality and included frozenShortcutHintsTabId/frozenShortcutHintsValue.
    • Centralized notification text in sidebarLatestNotificationText (MainActor-trimmed with body→title fallback) and removed the stale sidebar notification flag; now resolves visibility via settings.showsNotificationMessage.

Written for commit a8ef325. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Fixed modifier-hint badge from updating while workspace context menus are open, preventing visual inconsistencies.
  • Refactor

    • Refactored sidebar workspace row display architecture to improve component organization and latest notification text handling.

Review Change Stack

@vercel

vercel Bot commented May 24, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Jun 6, 2026 1:29pm
cmux-staging Building Building Preview, Comment Jun 6, 2026 1:29pm

@coderabbitai

coderabbitai Bot commented May 24, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This refactor moves unread-count, latest-notification-text, and shortcut-hint UI from the parent TabItemView into child views that observe notificationStore and modifierKeyMonitor directly. It introduces a freeze mechanism to capture shortcut-hint visibility when the context menu appears. TabItemView's Equatable is updated to exclude live-presentation fields, making its parent equality guard effective. Resolves performance issue #4618.

Changes

Sidebar row component refactoring

Layer / File(s) Summary
Helpers: latest-notification text and shortcut label
Sources/ContentView.swift
Adds sidebarLatestNotificationText(...) to centralize body/title selection and whitespace trimming, and adjusts workspaceShortcutLabel to return nil for missing digits and concatenate modifier+digit.
Row subcomponents: unread badge, live details, trailing accessory
Sources/ContentView.swift
Introduces SidebarWorkspaceUnreadBadge to render unread count from notificationStore, SidebarWorkspaceLiveDetails to render latest-notification text or subtitle with optional SSH remote row, and SidebarWorkspaceTrailingAccessory to choose between shortcut-pill vs close-button using frozen-hint state.
TabItemView contract: property and Equatable updates
Sources/ContentView.swift
Updates TabItemView properties to carry frozen-shortcut-hint state and modifierKeyMonitor instead of live presentation fields. Adjusts Equatable to include frozen state and modifierKeyMonitor while excluding live unread/notification/modifier-hint presentation, making the parent's equality guard effective.
Freeze capture mechanism: context-menu lifecycle
Sources/ContentView.swift
Updates onContextMenuAppear to capture the current tab ID and modifier-key pressed state, storing frozen values that prevent shortcut-hint changes while the context menu is open.
Parent wiring and view composition
Sources/ContentView.swift
Updates the TabItemView call site to pass notificationStore, modifierKeyMonitor, and frozen-shortcut-hint state. Restructures row layout by replacing inline unread/notification/shortcut rendering with calls to new subcomponents. Removes outdated documentation.

Possibly related PRs

  • manaflow-ai/cmux#4736: Both PRs update TabItemView.== at the same code hotspot; main PR changes equality to exclude live presentation fields (fixing #4618), while retrieved PR also updates equality for drag/drop snapshot fields.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A sidebar row once re-rendered whole,
but now child views take their proper role—
frozen hints stay put when menus appear,
unread badges live where they're held dear!
SwiftUI's Equatable guard rings true at last. ✨


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (2 errors, 1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Cmux Swift Concurrency ❌ Error PR introduces 3 new child views using @ObservedObject for notificationStore, a Combine pattern flagged by swift-concurrency-modernization rules. Refactor to use @Observable TerminalNotificationStore (pattern already present via SidebarDragState) instead of @ObservedObject.
Cmux Swiftui State Layout ❌ Error PR introduces @ObservedObject notificationStore in LazyVStack row subtrees, violating swiftui-state-layout.md by placing store references in row subtrees instead of using snapshots. Replace @ObservedObject with immutable snapshots and action closures, or use @Observable instead of ObservableObject pattern.
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Cmux Swift Actor Isolation ❓ Inconclusive No result was produced after verification. Marking as INCONCLUSIVE. Re-run the check or adjust instructions to produce a final result.
✅ Passed checks (13 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically describes the main refactoring: fixing sidebar tab equatable rows being invalidated by live presentation values.
Linked Issues check ✅ Passed The PR fully addresses issue #4618 by extracting live UI (unread badge, notification details, shortcut hints) into child views that observe stores directly, removing live values from TabItemView equality, and keeping shortcut-hint freezing via per-row snapshots.
Out of Scope Changes check ✅ Passed All changes are scoped to ContentView.swift sidebar refactoring with no modifications to auth, data, network, or unrelated functionality.
Cmux Swift Blocking Runtime ✅ Passed All new code uses reactive SwiftUI patterns without blocking primitives. Task.sleep and NSLock in file are existing code unrelated to this PR's sidebar presentation refactoring.
Cmux No Hacky Sleeps ✅ Passed PR changes only Swift code (Sources/ContentView.swift); the check applies only to TypeScript, JavaScript, shell, and build scripts, which are explicitly excluded from this PR.
Cmux Swift @Concurrent ✅ Passed All new/modified code follows Swift concurrent annotation rules: @MainActor synchronous helper, proper @ObservedObject usage in views, no @concurrent violations.
Cmux Swift File And Package Boundaries ✅ Passed Adds 97 net lines to oversized ContentView.swift via focused UI refactoring; three private SwiftUI views; no mixed responsibilities, domain logic, or testable feature logic in app target.
Cmux Swift Logging ✅ Passed The PR contains no new logging statements violating .github/review-bot-rules/swift-logging.md. All added code adds UI refactoring with no print, debugPrint, dump, NSLog, or unguarded Logger calls.
Cmux User-Facing Error Privacy ✅ Passed The PR only refactors private UI components without adding or modifying user-facing error messages, alerts, or sensitive information that would violate the privacy policy.
Cmux Full Internationalization ✅ Passed PR is a private refactoring that extracts live-updating sidebar UI into child views without introducing new user-facing strings or changing localization requirements.
Cmux Architecture Rethink ✅ Passed PR extracts live reads into child views, removes them from TabItemView's Equatable; no timing repairs, duplicate ownership, or lifecycle splits; invariant clearly documented.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR only refactors sidebar View components within ContentView.swift; no new NSWindow, NSPanel, NSWindowController, SwiftUI Window, or WindowGroup code introduced.
Description check ✅ Passed PR description includes summary, testing details, and acknowledgment of limitations, but is missing demo video URL and incomplete checklist items.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-4618-tabitemview-equatable

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.

❤️ Share

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

@greptile-apps

greptile-apps Bot commented May 24, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR isolates live-updating sidebar elements into small child views (SidebarWorkspaceUnreadBadge, SidebarWorkspaceLiveDetails, SidebarWorkspaceTrailingAccessory, SidebarWorkspaceShortcutHintOverlay) so that TerminalNotificationStore publishes and modifier-key transitions no longer invalidate the heavy TabItemView body protected by .equatable(). The context-menu shortcut-hint freeze now snapshots modifierKeyMonitor.isModifierPressed live at menu-appear time instead of from a pre-resolved bool passed into the row.

  • Unread badge, notification subtitle, SSH status, close button, and shortcut-hint overlay are moved into dedicated child views that directly observe notificationStore (@ObservedObject) or modifierKeyMonitor (@Observable read in body), keeping TabItemView.body from re-rendering on every notification event or modifier-key press.
  • frozenShortcutHintsTabId / frozenShortcutHintsValue replace showsModifierShortcutHints in the equality check; per-row scoping ensures only the context-menu target row re-evaluates when a menu opens, while all other rows stay protected.
  • Notification text extraction is centralized in sidebarLatestNotificationText with corrected trim-before-empty-check logic (whitespace-only body now falls back to title), a deliberate behavior improvement noted in the PR description.

Confidence Score: 5/5

Safe to merge; the refactor correctly moves live-updating sidebar elements into small child views without introducing stale state or broken invariants.

The core equatable boundary works correctly: notification and modifier-key changes now reach only the lightweight child views, not the heavy TabItemView body. The frozen shortcut-hint scoping is precise — only the context-menu target row re-evaluates when the freeze state changes. The notification text logic change (trim-before-empty check, title fallback) is an intentional improvement stated in the PR description. No observable-isolation mistakes, no blocking primitives, and no stale-state paths were introduced.

No files require special attention.

Important Files Changed

Filename Overview
Sources/ContentView.swift Adds four private child views for live sidebar data, removes unreadCount/latestNotificationText/showsModifierShortcutHints from TabItemView equality, replaces them with frozenShortcutHintsTabId/Value; notification text extraction centralized; all changes are architecturally correct.

Reviews (10): Last reviewed commit: "fix: remove stale sidebar notification f..." | Re-trigger Greptile

coderabbitai[bot]
coderabbitai Bot previously requested changes May 24, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@Sources/ContentView.swift`:
- Around line 9585-9587: Trim the notification body before deciding whether to
use the title: compute a trimmedBody by calling trimmingCharacters(in:
.whitespacesAndNewlines) on notification.body, then set text to
trimmedBody.isEmpty ? notification.title : trimmedBody (and continue to trim the
final text as currently done). Update the logic around variables text/trimmed so
empty-but-whitespace bodies fall back to notification.title rather than
suppressing it.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0cd53467-88ad-4f05-a770-41075f36a529

📥 Commits

Reviewing files that changed from the base of the PR and between 8627dc9 and e7288d6.

📒 Files selected for processing (1)
  • Sources/ContentView.swift

Comment thread Sources/ContentView.swift Outdated
Comment thread Sources/ContentView.swift
Comment thread Sources/ContentView.swift
Comment thread Sources/ContentView.swift
@austinywang
austinywang dismissed coderabbitai[bot]’s stale review May 24, 2026 16:24

Resolved by 88dc4c4; CodeRabbit confirmed the inline trim comment as addressed and the latest CodeRabbit status is success.

…w-equatable

# Conflicts:
#	Sources/ContentView.swift

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b9050c6. Configure here.

Comment thread Sources/ContentView.swift
Comment thread Sources/ContentView.swift
coderabbitai[bot]
coderabbitai Bot previously requested changes May 24, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift`:
- Around line 212-251: Add two unit tests exercising the inverted freeze
scenarios for
SidebarWorkspaceShortcutHintFreezePolicy.showsModifierShortcutHints: 1) create a
matching tabId SidebarTabItemPresentationSnapshot with
showsModifierShortcutHints = false and call showsModifierShortcutHints(tabId:
sameId, liveModifierPressed: true, frozenPresentation: frozen) and assert the
result is false; 2) call showsModifierShortcutHints(tabId: UUID(),
liveModifierPressed: false, frozenPresentation: nil) and assert the result is
false. Ensure test names reflect the behavior (e.g.,
testFrozenPresentationHidesShortcutHintWhenLiveWouldShow and
testNoFrozenPresentationHidesShortcutHintWhenLiveDoesNotShow).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 152e3db1-4d90-4d25-b9fa-8315c7620c8d

📥 Commits

Reviewing files that changed from the base of the PR and between e7288d6 and b9050c6.

📒 Files selected for processing (2)
  • Sources/ContentView.swift
  • cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift

Comment thread cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift Outdated
@austinywang
austinywang dismissed coderabbitai[bot]’s stale review May 24, 2026 20:09

Resolved by a639798; the requested shortcut hint freeze false-case tests were added and CodeRabbit marked the thread addressed, with latest CodeRabbit status success.

…w-equatable

# Conflicts:
#	Sources/ContentView.swift
#	cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift
@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026

This branch was successfully deployed

1 active deployment
Preview – cmux — a8ef3253 Deployed Jun 6, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: TabItemView Equatable short-circuit is bypassed by livePresentation derived in body

3 participants