Repository navigation
Add deterministic sidebar ownership and scale gates - #8279
azooz2003-bit wants to merge 1 commit into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
📝 WalkthroughWalkthroughThe PR updates the sidebar workspace-row debug probe to accept workspace identifiers, expands SwiftUI sidebar layout and ownership scale tests, bounds the geometry feedback canary, and adds comprehensive ChangesSidebar contract and scale tests
CoalesceLatest regression tests
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 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 SummaryThis PR adds deterministic sidebar ownership and scale gates: a one-line production change threads
Confidence Score: 4/5Safe to merge: the only production source change is a one-character DEBUG probe signature update that is compiled out of Release builds entirely, leaving runtime behaviour unchanged. The production diff is minimal and correct. The test additions are well-structured causal checks. The one gap worth noting is that cmuxTests/SidebarPointerInteractionScaleTests.swift — the Important Files Changed
Reviews (1): Last reviewed commit: "test: guard sidebar ownership at scale" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// An AppKit renderer can reuse these exact row values and operation bounds. | ||
| @Suite(.serialized) | ||
| final class SidebarImmutableRowScaleTests { | ||
| private static let realizedRowCeiling = 150 |
There was a problem hiding this comment.
realizedRowCeiling = 150 does not protect the 100-workspace scale point
SidebarImmutableRowScaleTests iterates over [1, 10, 100, 1_000] and checks initialRowBodies < 150 at every step, but the threshold was sized to catch O(n) at 1 000 workspaces. At 100 workspaces a full-list realization (the regression being guarded) produces 100 bodies, which still satisfies 100 < 150, so the test silently passes even if LazyVStack virtualisation is defeated. Only the 1 000-workspace iteration would fail. A per-scale effective ceiling (e.g. min(workspaceCount, realizedRowCeiling)) would close the gap.
| """ | ||
| ) | ||
| } | ||
|
|
||
| let newlyRealizedWorkspaceIds = harness.counter.realizedWorkspaceIds | ||
| .subtracting(initiallyRealizedWorkspaceIds) | ||
| #expect( |
There was a problem hiding this comment.
print() emits structured baseline data to stdout
The SIDEBAR_SCALE_BASELINE JSON lines are written with print(). Test code is exempt from the unified-logging rule, but if this output is intended for CI tooling or trend tracking, it would be more reliably captured via os_log / Logger at a debug level, or written to a named file from the test, rather than mixed into the test runner's stdout stream where it competes with framework output.
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/SidebarLazyLayoutScaleTests.swift (1)
457-485: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the realization thresholds discriminate the behavior under test.
Both gates can pass without proving viewport-bounded or feedback-driven realization.
cmuxTests/SidebarLazyLayoutScaleTests.swift#L457-L485: compare the feedback fixture with an identical no-feedback baseline.cmuxTests/SidebarPointerInteractionScaleTests.swift#L450-L507: bound unique realized workspace IDs below the total row count for overflowing fixtures.🤖 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 `@cmuxTests/SidebarLazyLayoutScaleTests.swift` around lines 457 - 485, Strengthen the realization assertions at cmuxTests/SidebarLazyLayoutScaleTests.swift:457-485 by rendering an identical no-feedback baseline alongside BoundedGeometryFeedbackRowFixture and comparing their realization counts, so the test proves feedback-driven behavior rather than merely exceeding a fixed threshold. At cmuxTests/SidebarPointerInteractionScaleTests.swift:450-507, track unique realized workspace IDs for the overflowing fixtures and assert that their count remains below the total row count, preserving the existing interaction coverage while proving viewport-bounded realization.
🤖 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.
Outside diff comments:
In `@cmuxTests/SidebarLazyLayoutScaleTests.swift`:
- Around line 457-485: Strengthen the realization assertions at
cmuxTests/SidebarLazyLayoutScaleTests.swift:457-485 by rendering an identical
no-feedback baseline alongside BoundedGeometryFeedbackRowFixture and comparing
their realization counts, so the test proves feedback-driven behavior rather
than merely exceeding a fixed threshold. At
cmuxTests/SidebarPointerInteractionScaleTests.swift:450-507, track unique
realized workspace IDs for the overflowing fixtures and assert that their count
remains below the total row count, preserving the existing interaction coverage
while proving viewport-bounded realization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83fe2b0c-baba-46e4-9f4f-8e1d46e217aa
📒 Files selected for processing (5)
Sources/ContentView.swiftSources/Debug/SidebarLazyContractProbe.swiftcmuxTests/SidebarLazyLayoutScaleTests.swiftcmuxTests/SidebarPointerInteractionScaleTests.swiftcmuxTests/WorkspaceSidebarObservationTests.swift
lawrencecchen
added
the
stale-revisit
3 participants
Summary
AsyncPublisher.valuesconsumes valuesThis changes no release behavior. The only source change is DEBUG probe plumbing that records which workspace rows SwiftUI realizes.
Verification
sblifeapp exercised with 55 workspaces, top-to-bottom scrolling, and selection at both ends; no reentrant-layout, fatal, assertion, crash, or exception log entriesHistorical red replay
The two ownership tests were replayed against pre-#8211 sidebar commit
856a18629d8fb5d245aacccdfee37ed4e6d1f514. No publisher fixes were applied.VerticalTabsSidebar.body8 times, expected 0The historical sidebar implementation remained unchanged. Compatibility adjustments only mapped the probe to the old
tab.idname and exposed an existing test helper.