feat: resolve the focus window's folders below the scope root - #945
Conversation
The vault-pointer leg lifts the root's direct children only, and the drain repaints just what this device published, so a device that did not author a subtree rendered it empty below the scope root indefinitely. Command::SetFocus fell through to Unimplemented, and focus_set/ChildAdopter had no caller that descended. SetFocus now records the focus window and refreshes it on access past the staleness threshold; the resolve tick refreshes the same window every pass, before the drain. Each folder resolves its own record cache-first, passes the child gate on this device's floors, and merges through project_folder. A gate rejection is fail-closed: last-known-good stands and no floor moves. project_folder now reports whether the merge moved the base, so a re-merge of the same body on every tick does not repaint the host. Closes #917
…re-probes Review follow-ups on the focus-window slice. The Adopted / Current / NoUpdate + at-floor re-open walk is trust-critical and was copied in two places; both read paths that descend below the scope root now go through one resolve_child, so neither can drift on which outcome is staleness and which is a fail-closed violation. The on-access stamp now records attempts rather than merges: a folder that never gate-passes no longer turns every navigation into a fresh endpoint fan-out, and the poll leg still refreshes the window unconditionally so recovery is automatic. The threshold filter itself moves next to focus_set as focus_folders_due, with its own unit test. Snapshot::link reports whether it established or raised a link, so project_folder no longer predicts it. The focus pass merges root-ward, so a parent that dropped a child unlinks it before the pass would project into it. Tests: the planted-record case now covers all three child-gate bindings the pass rests on -- foreign node id, foreign scope, and a file body behind a folder ref -- and an unreachable record plane is asserted to leave the focused folder rendering off the cache rather than clearing it.
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughChangesFocus-window refresh
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/tests/write_plane.rs (1)
1994-2029: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing this helper in the existing depth-2 test.
deep_create_seen_by_a_second_devicerepeats the create-and-boot prologue ofa_create_below_the_scope_root_is_adoptable_by_a_second_deviceat lines 1921-1947. That earlier test can call this helper and keep only its own gateway and adopter assertions. The change removes one copy of the fixture and keeps both tests on the same setup.🤖 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 `@crates/engine/tests/write_plane.rs` around lines 1994 - 2029, The existing depth-2 test should reuse the deep_create_seen_by_a_second_device fixture instead of duplicating its create-and-boot setup. Replace that test’s repeated prologue with the helper’s returned world, blocks, second engine, tasks, photos, and deep node, while retaining only the test-specific gateway and adopter assertions.
🤖 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.
Nitpick comments:
In `@crates/engine/tests/write_plane.rs`:
- Around line 1994-2029: The existing depth-2 test should reuse the
deep_create_seen_by_a_second_device fixture instead of duplicating its
create-and-boot setup. Replace that test’s repeated prologue with the helper’s
returned world, blocks, second engine, tasks, photos, and deep node, while
retaining only the test-specific gateway and adopter assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7548cd52-ce25-4544-a84f-dc75c8999c79
📒 Files selected for processing (11)
crates/engine/src/facade.rscrates/engine/src/net/child.rscrates/engine/src/net/focus.rscrates/engine/src/net/mod.rscrates/engine/src/net/resolve.rscrates/engine/src/sync/mod.rscrates/engine/src/sync/model.rscrates/engine/src/sync/project.rscrates/engine/src/sync/tick.rscrates/engine/tests/facade.rscrates/engine/tests/write_plane.rs
…lves The record-plane test duplicated the fixture's create-and-boot prologue. Return the fixture as a named struct so it can also hand back device B's seams, and destructure it at every call site.
|
Addressed the review-body nitpick. No inline threads were opened on this pass, so replying here.
The duplication is real: The suggestion as written would not have compiled, though. It proposed reusing "the helper's returned world, blocks, second engine, tasks, photos, and deep node", but that test also needs device B's own seams — Fixed in 7f7524b by returning a named The record-plane test's doc comment claimed "the facade has no descent below the scope root yet", which this PR makes false; it now cross-references its facade half instead. Verified: |
1 participant
Problem
Nothing resolved a folder record below the scope root on a device that did not author it, so such a device rendered every subtree empty — indefinitely.
spawn_resolve_tick_looprefreshed the vault root only;refresh_base_from_outcometoproject_rootlifts direct children only.Command::SetFocusfell through toEngineError::Unimplemented, whichapps/web/src/engine/snapshotStore.tsturns into a committed error state on every navigation.ChildAdopter's single caller wasread_content, which is file-only.focus_set/FocusWindow/FocusTarget::Folderwere pure planning with no caller.blueprint/engine.md"Sync core" specifies both halves — the focus-window tick and the on-access refresh past the staleness threshold. Neither was wired.Change
crates/engine/src/net/focus.rs(new) —FolderRefreshresolves each focus folder's own record cache-first, passes theChildAdoptergate on this device's floors, and merges the gate-passing body withproject_folder. A gate rejection is fail-closed: last-known-good stands and no floor moves. The pass merges root-ward, so a parent that dropped a child unlinks it before the pass would project into it, and it reads no clock — the caller stamps.resolve_childinnet/child.rs— theAdopted/Current/NoUpdate+ at-floor re-open walk, previously duplicated. Both read paths that descend below the scope root (a file's content read and this refresh) now share it, so neither can drift on which outcome is staleness and which is a fail-closed violation.Command::SetFocusrecords the focus window and refreshes it immediately, but only for folders past the staleness threshold — navigation is the tick model's second trigger source, and a repeat visit renders state already held. Stamps record attempts, so a folder that never gate-passes cannot turn every navigation into a fresh endpoint fan-out; the poll leg still refreshes the window unconditionally, so recovery is automatic.focus_folders/focus_folders_dueinsync/tick.rs— the window's folder targets below the scope root, and the threshold filter. The root rides the vault-pointer leg, never the child gate.project_folderreports whether the merge moved the base, so re-merging the same body every tick does not repaint the host.Snapshot::linkreports whether it established or raised a link, so the projection no longer predicts it;project_rootandrefresh_base_from_outcomeforward the report.No new crypto, no new seam, no TypeScript change —
setFocuswas already plumbed throughcrates/wasmandpackages/client, and the web store already pulls on success.Tests
crates/engine/tests/write_plane.rs, gated by Engine Tests (cargo test -p cipherbox-engine):a_second_device_lists_below_the_scope_root_once_it_focuses_there— the facade half of engine: assert the two-device round trip on a create below the scope root #895's round trip. A cold second device lists the depth-2 child with device A's name and node id after setting focus on the depth-1 parent, having listed nothing there before.a_planted_focus_record_never_renders— fail-closed on all three child-gate bindings the pass rests on. A legitimate concurrent record reconciles first, so the negatives cannot pass on a leg that never ran; then a foreign node id, a foreign scope, and a file body behind a folder ref are each planted at the focused name at a strictly newer sequence, and last-known-good is pinned through all three.an_unreachable_record_plane_leaves_the_focused_folder_rendering— every endpoint failed, the folder keeps rendering off the cache rather than clearing.navigation_re_resolves_a_folder_only_past_the_staleness_threshold— a repeat visit inside the threshold renders held state; the same navigation past it reconciles.Plus
crates/engine/tests/facade.rs::set_focus_records_a_window_with_nothing_to_resolve,sync/tick.rsunit tests forfocus_foldersandfocus_folders_due, andsync/project.rs::project_root_reports_a_change_only_when_the_body_moves_the_base.Revert check: stubbing
focus_foldersto return an empty list fails exactly the four newwrite_planetests and nothing else.Verification
All exit 0:
cargo fmt --allcargo clippy --workspace --all-targets -- -D warnings— cleancargo check --workspace --all-targetscargo check -p cipherbox-wasm --target wasm32-unknown-unknown --all-targetscargo test --workspace— 32 suites green, 0 failurespnpm -r --if-present run typecheck— api, client, web all cleanpnpm -r --if-present run test— client, api 177, web 67 all passnpx eslint .— cleanReview gates
/simplify,/security-reviewand/crypto-privacy-reviewwere each run ongit diff main...HEAD. Security and crypto returned no findings. The second commit folds in the real items from all three: the sharedresolve_childwalk, the attempt-stamped on-access damper,Snapshot::link's change report, the root-ward merge order, the extra fail-closed test coverage, and a comment-discipline pass.One deferral: a child-gate trust violation on the focus leg is fail-closed but unsurfaced to the host, exactly as the root leg is. That is #796's scope; this PR widens what #796 has to cover from one record to the focus chain, and #796 has been updated to say so.
Closes #917
Summary by CodeRabbit
New Features
Bug Fixes