Repository navigation
Portal: invalidate the split-divider hit-test cache on nested subview insertion - #8580
Conversation
|
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:
📝 WalkthroughWalkthroughSplit-divider collection now records hierarchy nodes with split-view state. Swizzled ChangesSplit-divider cache invalidation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WindowPortal
participant PortalSplitDividerRegion
participant PortalSplitDividerCacheInvalidator
participant PortalViewHierarchyMutationTracker
WindowPortal->>PortalSplitDividerRegion: Collect divider regions and hierarchy nodes
WindowPortal->>PortalSplitDividerCacheInvalidator: Register root and hierarchy nodes
PortalSplitDividerCacheInvalidator->>PortalViewHierarchyMutationTracker: Check registration generation
PortalViewHierarchyMutationTracker-->>WindowPortal: Return current or stale status
WindowPortal->>PortalSplitDividerRegion: Recollect regions when stale
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches🧪 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 |
5b09076 to
79721ad
Compare
Greptile SummaryThis PR fixes a stale split-divider hit-test cache that persisted after a split view was inserted into a nested container. The root cause was that the cache validity was checked only against the root view's direct subview list, which doesn't change on nested insertions;
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to cache-validity logic, all accesses remain on the main actor, and the hot-path allocation concern raised in a prior round has already been resolved. The fix correctly expands cache-validity checks from the root view's direct subview list to the subview-identity lists of every structure-observed view, catching nested insertions that KVO misses. The pointer-move comparison path uses a count check plus a lazy zip with no intermediate array, keeping allocations to the unavoidable AppKit view.subviews reads. Both portals receive the same treatment, and a new test pins the regression. No correctness, isolation, or hot-path issues remain. Files Needing Attention: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["splitDividerRegions() called\n(pointer-move event)"] --> B{rootView available?}
B -- No --> C["Clear cache\nReturn []"]
B -- Yes --> D{cachedSplitDividerRegions\n& cachedSplitDividerStructure\nexist?}
D -- No --> G
D -- Yes --> E["structureSnapshotsMatch(structure)\nFor each observed view:\n count check → zip ObjectIdentifier check\n(no intermediate array)"]
E -- Match --> F["allLive(regions)?"]
F -- Yes --> HIT["Return cached regions ✓"]
F -- No --> G
E -- Mismatch\n(nested insertion detected) --> G
G["collect(in: rootView)\nTraverse view tree\nGather split divider regions\n+ structureObservedViews"] --> H["Cache regions\nSnapshot all observed views\n(root, direct children,\nsplit ancestors,\narrangedSubviews)"]
H --> I["Update KVO observers\n(eager fast path)"]
I --> RET["Return fresh regions"]
Reviews (3): Last reviewed commit: "portal: compare structure snapshots with..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… insertion The split-divider region cache trusted KVO of NSView.subviews to catch structural changes, but addSubview does not reliably emit that KVO across macOS versions, so a split view inserted into a nested container left the cache stale and hit-testing wrong. Replace the root-only subview-id check with a structure fingerprint over all observed views (root, its subviews, split ancestors), recomputed on the lookup path; the KVO observers stay as an eager fast path but correctness no longer depends on them. Same fix in the Browser portal, which duplicated the cache.
23a3d19 to
dfb0bad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/BrowserWindowPortal.swift`:
- Around line 1036-1040: Update the cached split-divider validation to include
the current root view identity in the authoritative structure record. In
Sources/BrowserWindowPortal.swift lines 1036-1040 and
Sources/TerminalWindowPortal.swift lines 374-378, pass rootView to
PortalSplitDividerRegion.structureSnapshotsMatch and require the recorded root
to match before returning cached regions; otherwise fail closed and recollect.
In `@Sources/PortalSplitDividerRegion.swift`:
- Around line 253-255: Update structureSnapshots(of:) so the structural
fingerprint includes every traversed NSView container in the hierarchy, not only
the views returned by collect; recursively snapshot each view’s subviews (or
implement an equivalent full-tree fingerprint), preserving subviewIds for each
captured container so nested split insertion invalidates cached portal
structures.
🪄 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 Plus
Run ID: bc2d9285-a521-43d5-9f23-51b22f82cf1c
📒 Files selected for processing (3)
Sources/BrowserWindowPortal.swiftSources/PortalSplitDividerRegion.swiftSources/TerminalWindowPortal.swift
…ment The divider cache's structure snapshots only track the content root, its direct children, and views that were split-related when the cache warmed up. Two gaps: a split inserted under a container two levels deep changes no observed subview list, and a replaced-but-still-alive content root passes validation because no snapshot records which root it was built from. Both leave hit-testing on stale empty regions. Failing tests first; the fix lands in the next commit.
…digest The structure snapshots only covered the content root, its direct children, and views that were split-related when the cache warmed, so a split inserted under a deeper container changed no recorded subview list and the stale cache kept winning. They also never recorded which root they were built from, so a replaced-but-still-alive content root passed validation against the detached tree. Replace the snapshots with a digest keyed to the root's identity that a full-tree walk rebuilds on the lookup path: each split's identity, ancestor chain, arranged subviews, orientation, and effective visibility. An insertion under any container now misses the cache, while subview churn that cannot affect dividers still reuses it, and the subviews KVO stays bounded to the same views as before. Both portals share the digest through PortalSplitDividerRegion.
|
@ejc3 The finished branch is pushed to |
|
Follow-up: the canonical policy gate required the hierarchy hub to own its own source file. The canonical branch is now finalized at |
|
Latest exact-SHA fork sync: the canonical branch is now at The branch includes Canonical branch autoreview, the merge-conflict gate against @ejc3 please fast-forward |
…che-invalidation # Conflicts: # tests/test_omp_extension_install.py
|
Current head @greptile-apps review |
|
Current head @greptile-apps review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Current head be1e22b supersedes the prior review SHA. Cursor’s wrapped-detached-reentry finding is addressed at the mutation ownership boundary with exact behavior coverage; the test-only and fixed focused runs are linked on its thread. @coderabbitai review @greptile-apps review |
|
✅ Action performedReview finished.
|
|
Current head 0edb762 supersedes the prior review SHA. In addition to the wrapped-reentry fix, the first registration after an inactive cache interval now revokes proofs retained while mutation tracking was intentionally dormant, with a separate behavior regression/fix commit pair. @coderabbitai review @greptile-apps review |
|
✅ Action performedReview finished.
|
|
CodeRabbit policy closeout for current head 0edb762: the Cmux No Ambient Global State item is consciously not changed because it misclassifies the AppKit bridge. PortalViewHierarchyMutationTracker is a real per-window owner: window, generation, weak cache registrations, and sort depth are instance state, and the owning NSWindow retains exactly that instance through an Objective-C association. The static methods are the minimal dispatch boundary called by process-wide swizzled AppKit methods to resolve the affected window-owned tracker; they hold no mutable process-wide state. Constructor-injecting a tracker into arbitrary NSView mutations is impossible without adding a second ambient registry, which would weaken the current ownership boundary. The immutable association keys are identity tokens, not runtime state. The existing Docstring Coverage warning also remains consciously rejected for the previously documented reason: these are private/internal AppKit implementation methods, while the non-obvious ownership and performance contracts are documented at type/mutation boundaries and in behavior tests. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 779aa42. Configure here.
* Hide current iOS Agent GUI * Revert portal hierarchy mutation tracker launch crash (#10018) * Revert "Fix portal mutation tracker launch crash (#10008)" This reverts commit a161d16. * Revert "Portal: invalidate the split-divider hit-test cache on nested subview insertion (#8580)" This reverts commit bcfb2d7. --------- Co-authored-by: Austin Wang <austinwang115@gmail.com>
2 participants
What you'd hit
After an
NSSplitViewis inserted, removed, replaced, reordered, or changed inside a nested container, terminal and browser portal hit testing can keep using stale divider regions until an unrelated geometry change refreshes the cache.Root cause
The original cache validity key covered only the content root's direct children. Nested mutations do not change that list, and
NSView.subviewsis not documented as KVO-compliant. A later full-tree digest restored correctness but moved an O(view-count) traversal onto every pointer lookup, which is the latency-sensitive path this cache exists to protect.A generation index also needs a provenance boundary: a subtree indexed as containing no split views can be detached, mutate while no window tracker can observe it, and then be reattached with a stale proof.
Fix
Both portal hosts now retain the weak identity of the root that produced their cached regions and validate that cache through one window-owned
PortalViewHierarchyMutationTracker:PortalSplitDividerCacheInvalidatorinstalls one process-wide hook for every supported publicNSViewhierarchy mutation entrypoint: bothaddSubviewforms, thesubviewssetter, both removal methods, replacement, andsortSubviews.sortSubviewsrecords identity order only for split-bearing indexed parents and invalidates only when the order changes. Browser interaction-layer sorting now skips already ordered subviews.The hook methods are resolved as a complete set before any implementation exchange, so installation cannot leave a partially hooked mutation surface.
Tests and performance proof
PortalHitTestingPerformanceTestscovers nested/deep insertion, removal, replacement, content-root changes, detached root and nested-subtree mutation, hidden-to-visible transitions, moves, no-op and real sorting, and pointer-cache reuse.342b48d2c18f1e44a1625c5bdf257f47bf7a3ba0: 14 tests executed; the new detached nested-subtree regression was the sole failure.053695f2d7c88f8abdd6e90d9de5d3e59988f95c: all 14 tests passed.cab9967802bf8e002cf5fbf54b4b61f5b67a313e: all 14 tests passed after mergingorigin/mainat6089fa04d3effd27e43c5c6104a4eada62fe859f.Project normalization, test wiring (654 files), workspace/package policy, app-host isolation/retry tests, script syntax, and
git diff --checkpass locally. No warning-budget or file-length-budget file is changed.Required PR CI is waiting for the contributor fork to fast-forward from stale PR head
7346268746cff72113c85d9bc5b09f7d8cdc357bto canonical headcab9967802bf8e002cf5fbf54b4b61f5b67a313e. CurrentmainCI independently reproduces the unrelated app-host/session failures seen in the earlier exact-branch full run; their owning repair PRs remain upstream.Review
Canonical branch autoreview at
cab9967802bf8e002cf5fbf54b4b61f5b67a313ereports no accepted/actionable findings (patch is correct, 0.9 confidence). Its merge-conflict gate against currentorigin/mainand the cmux policy gate are clean. All three actual inline review threads are resolved.Localization audit
No user-facing strings were added or changed.
Note
High Risk
Process-wide method swizzling on NSView/NSSplitView affects every hierarchy mutation in the app; incorrect hook or generation logic could cause stale divider hit-testing or extra cache churn on the pointer path.
Overview
Fixes stale split-divider hit-test regions when
NSSplitViewlayout changes in nested containers, after detach/reattach, or via arranged-subview APIs—without walking the full view tree on every pointer move.Terminal and browser portal hosts now tie cache validity to the weak cached root plus
PortalSplitDividerCacheInvalidator.isHierarchyCurrent, replacing checks on direct child identity lists.New window-owned tracking (
PortalViewHierarchyMutationTracker, registration tokens, per-view node state) records split-presence at cache build and bumps a generation when divider-relevant structure changes. Process-wide AppKit hooks onNSView/NSSplitViewmutation entry points feed that tracker; subview KVO for structure is removed. Detached or cross-window mutations and unknown nonempty subtrees fail closed; split-free same-window moves can stay on a fast path.PortalSplitDividerRegion.collectnow returns hierarchy nodes for indexing instead of structure-only observation lists. The browser slot view skipssortSubviewswhen interaction-layer priorities are already ordered.Tests cover cache reuse, deep insertion, arranged-subview changes, content-root swaps, detached proofs, and bounded work across many caches.
Reviewed by Cursor Bugbot for commit dfe3fd7. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Performance
Bug Fixes
Tests