Repository navigation
Extract custom-sidebar data-context projection from ContentView into CmuxSidebarLayout - #6167
azooz2003-bit wants to merge 1 commit into
Conversation
Pulls the interpreter data-context projection out of the VerticalTabsSidebar view in ContentView.swift into a new leaf package, CmuxSidebarLayout, as a pure value-typed builder with the live god-types inverted behind Sendable input snapshots. What moved: - CustomSidebarDataContextBuilder: owns all SwiftValue assembly, default values, and optional-field omission rules for the top-level context, per-workspace, and per-surface value objects (was the inline bodies of customSidebarDataContext / customSidebarWorkspaceValue / customSidebarSurfaceValues). - CustomSidebarContextSnapshot / CustomSidebarWorkspaceSnapshot (+ Progress / Remote) / CustomSidebarSurfaceSnapshot: Sendable, Equatable value-typed inputs. The seam: the app still reads live Workspace / TabManager / SidebarUnreadModel state, but only to project it into the snapshot value types; every SwiftValue field name, default, and omit-when-nil/empty rule now lives in the package, so no package type ever names a god type. The Calendar is constructor-injected (defaults to .current) for testability. Byte-identical: the produced [String: SwiftValue] tree is unchanged. Field names, the nil-or-empty-string omission rules (description/color/messages/ directory), the git/PR/progress/remote field sets, the clock formatting, and the surface pane-walk ordering are all preserved; surfaces without a panel id are still skipped at projection time. SwiftValue dict ordering never affected output (rendering sorts keys / looks up by key). Tests: 8 Testing-framework cases for the builder using fixed-calendar fakes and known snapshots (clock components, optional presence/omission, empty-string omission, progress-without-label, surface enrichment). est lines removed from ContentView.swift: 20 net (86 deleted, 66 thinner projection added); budget ratcheted 16674 -> 16654. NOTE: app build validated by CI. The new package builds and its tests pass locally (swift build / swift test in Packages/CmuxSidebarLayout). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA new local Swift package ChangesCmuxSidebarLayout Package and ContentView Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 SummaryThis PR extracts the custom-sidebar interpreter data-context projection from
Confidence Score: 4/5The extraction is mechanically faithful — all field names, omission rules, and pane-walk ordering are preserved. The only issues are API surface design choices that don't affect runtime behavior. All production logic is byte-identical to the original inline code and is backed by 8 targeted tests. The two findings are non-blocking design notes: workspaceValue/surfaceValue are wider than necessary given @testable import covers tests, and the surfaceCount/surfaces.count divergence invariant is real but undocumented in the public type. CustomSidebarWorkspaceSnapshot.swift — the surfaceCount docstring should explain that it intentionally counts panel-less tabs skipped by the surfaces array. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[VerticalTabsSidebar\nContentView.swift] -->|customSidebarDataContext| B[customSidebarContextSnapshot]
B -->|tabManager.tabs.enumerated| C[customSidebarWorkspaceSnapshot\nper workspace]
C -->|bonsplitController pane walk| D[customSidebarSurfaceSnapshots\nfiltered by panelId]
D --> E[CustomSidebarSurfaceSnapshot\nSendable / Equatable]
C --> F[CustomSidebarWorkspaceSnapshot\nSendable / Equatable\nnote: surfaceCount ≠ surfaces.count]
B --> G[CustomSidebarContextSnapshot\nSendable / Equatable]
F --> G
E --> F
G -->|CustomSidebarDataContextBuilder\n.dataContext| H["[String: SwiftValue] tree"]
H -->|workspaces, workspaceCount,\nselectedTitle, selectedId,\nunreadTotal, clock| I[Custom Sidebar Interpreter]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Total surface count across panes (`workspaces[i].tabCount`). | ||
| public let surfaceCount: Int |
There was a problem hiding this comment.
surfaceCount / surfaces.count divergence is undocumented
In ContentView.swift, surfaceCount is set to the full bonsplit pane walk (allPaneIds.reduce(0) { $0 + tabs(inPane:) }), while surfaces is built with a guard let panelId = workspace.panelIdFromSurfaceId(tab.id) else { continue } filter — so surfaceCount intentionally exceeds surfaces.count when any tab lacks a panel ID. The builder maps surfaceCount to tabCount and surfaces to tabs, meaning tabs.count and tabCount will diverge in the interpreter context. This is preserved byte-identical from the original, but the public API docstring gives no hint of the split invariant. A future snapshot constructor that sets surfaceCount: surfaces.count (the natural default) would silently change tabCount for any workspace that has panel-less tabs.
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
`@Packages/CmuxSidebarLayout/Tests/CmuxSidebarLayoutTests/CustomSidebarDataContextBuilderTests.swift`:
- Line 218: The customColor fixture value uses a 3-digit hex format (`#fff`) which
violates the workspace color contract that requires 6-digit hex values in
`#RRGGBB` format. Replace the "customColor: "`#fff`"" assignment with a 6-digit hex
color value (for example, "`#ffffff`") to ensure the test fixture enforces the
same color format constraint that production code expects.
🪄 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: b40cdae8-4c29-4760-851b-2fc64ae91ac8
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
Packages/CmuxSidebarLayout/Package.resolvedPackages/CmuxSidebarLayout/Package.swiftPackages/CmuxSidebarLayout/Sources/CmuxSidebarLayout/CustomSidebarContextSnapshot.swiftPackages/CmuxSidebarLayout/Sources/CmuxSidebarLayout/CustomSidebarDataContextBuilder.swiftPackages/CmuxSidebarLayout/Sources/CmuxSidebarLayout/CustomSidebarSurfaceSnapshot.swiftPackages/CmuxSidebarLayout/Sources/CmuxSidebarLayout/CustomSidebarWorkspaceSnapshot.swiftPackages/CmuxSidebarLayout/Tests/CmuxSidebarLayoutTests/CustomSidebarDataContextBuilderTests.swiftSources/ContentView.swiftcmux.xcodeproj/project.pbxproj
| surfaces: [], | ||
| surfaceCount: 0, | ||
| customDescription: "desc", | ||
| customColor: "#fff", |
There was a problem hiding this comment.
Use a 6-digit workspace color in this fixture.
"#fff" conflicts with the repo’s workspace color contract (#RRGGBB). Use a 6-digit value so tests reinforce the same invariant production code expects.
Suggested patch
- customColor: "`#fff`",
+ customColor: "`#FFFFFF`",Based on learnings, workspace/tab colors in this repo should use only 6-digit #RRGGBB values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| customColor: "#fff", | |
| customColor: "`#FFFFFF`", |
🤖 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/CmuxSidebarLayout/Tests/CmuxSidebarLayoutTests/CustomSidebarDataContextBuilderTests.swift`
at line 218, The customColor fixture value uses a 3-digit hex format (`#fff`)
which violates the workspace color contract that requires 6-digit hex values in
`#RRGGBB` format. Replace the "customColor: "`#fff`"" assignment with a 6-digit hex
color value (for example, "`#ffffff`") to ensure the test fixture enforces the
same color format constraint that production code expects.
Source: Learnings
|
Superseded by #6226 — consolidated into the existing CmuxSidebar package (no new micro-package) per over-engineering review. The extraction is preserved there. |
This branch was successfully deployed
1 participant
Extracts the custom-sidebar interpreter data-context projection out of the giant
VerticalTabsSidebarview inSources/ContentView.swiftinto a new leaf package,CmuxSidebarLayout, as a pure value-typed builder with the live god-types inverted behindSendableinput snapshots.This is the clean, latency-insensitive subset of the
sidebar-layout-compositiondomain. The rest of that domain (VerticalTabsSidebarbody, the.equatableTabItemViewrow, the frame-anchor preference plumbing, the markdown renderer and the shortcut/accessory policies) is either latency-critical-per-keystroke or already extracted intoCmuxFoundation/CmuxSidebar, so this PR takes the genuinely clean piece rather than forcing the hot views into a package.What moved
CustomSidebarDataContextBuilder(the major type): owns allSwiftValueassembly, default values, and optional-field omission rules for the top-level context, per-workspace, and per-surface value objects. This was the inline bodies ofcustomSidebarDataContext(now:)/customSidebarWorkspaceValue(_:index:selectedId:)/customSidebarSurfaceValues(_:focusedPanelId:).CustomSidebarContextSnapshot,CustomSidebarWorkspaceSnapshot(+ nestedProgress/Remote),CustomSidebarSurfaceSnapshot:Sendable,Equatablevalue-typed inputs.The seam (dependency inversion)
The app still reads live
Workspace/TabManager/SidebarUnreadModelstate, but only to project it into the snapshot value types; the package never names a god type.ContentViewkeeps three thin private projection methods that build the snapshots and callCustomSidebarDataContextBuilder().dataContext(for:). TheCalendaris constructor-injected (defaults to.current) for testability. The package depends only onCmuxSwiftRender(forSwiftValue).Byte-identical
The produced
[String: SwiftValue]tree is unchanged. Preserved exactly: every field name, the nil-or-empty-string omission rules (description/color/latestMessage/latestPrompt/ surfacedirectory), the git / PR / progress / remote field sets, theclock%02d:%02d:%02dformatting and components, and the surface pane-walk ordering (surfaces without a panel id are still skipped at projection time).SwiftValueis a dict-backed object so key ordering never affected output (displayStringsorts keys; the interpreter looks up by key). The PR value objects are passed through already-projected by the existingWorkspace.customSidebarPullRequestValues()extension, so display ordering is unchanged.Tests
8
Testing-framework cases for the builder using fixed-calendar fakes and known snapshots: always-present top-level keys, empty-selectionselectedId, clock-component derivation, always-present + optional workspace fields, empty-string omission, progress-without-label, and surface enrichment presence/omission.Stats
est lines removed from
ContentView.swift: 20 net (86 deleted, 66 thinner projection added). File-length budget ratcheted 16674 -> 16654.scripts/lint-ios-package-conventions.shprints OK with zero newlint:allow.swift buildandswift testpass inPackages/CmuxSidebarLayout.NOTE: the full app build is validated by CI (not run locally, per the 22-parallel-agent constraint).
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracted the custom sidebar data-context projection from
VerticalTabsSidebarinto a new leaf package,CmuxSidebarLayout. Behavior is unchanged, but the logic is now decoupled, value-typed, and testable.Refactors
CmuxSidebarLayoutwithCustomSidebarDataContextBuilderand snapshot types (CustomSidebarContextSnapshot,CustomSidebarWorkspaceSnapshotwithProgress/Remote,CustomSidebarSurfaceSnapshot).ContentViewnow builds snapshots and callsCustomSidebarDataContextBuilder().dataContext(for:).CmuxSwiftRender; no app “god types” referenced..current) for consistent clock derivation.[String: SwiftValue]is byte-identical: same fields, omission rules (empty/nil), clock format/components, PR/git/progress/remote sets, and surface walk/ordering.Tests
Written for commit 2e871c9. Summary will update on new commits.
Summary by CodeRabbit