Avoid blocking folder navigation during directory size calculation - #118
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughDirectory-size traversal now runs in a cancellable background worker. The event loop receives generation-checked results while remaining available for navigation. New unit, integration, protocol, and documentation updates cover cancellation and stale-result handling. ChangesAsynchronous directory-size processing
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Backend loop
participant dirsizereq
participant Worker
participant Filesystem
Backend loop->>dirsizereq: start_next(State)
dirsizereq->>Worker: start(row, path)
Worker->>Filesystem: walk_cancellable(path)
Filesystem-->>Worker: DirSize result
Worker-->>Backend loop: Event::DirSize(Done)
Backend loop->>dirsizereq: report_done(Done)
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Directory-size calculation moves off the event loop while preserving cancellation and result freshness behavior. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.59% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 10 files. (2 skipped: 2 unsupported.)
✨ 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 |
|
YOU COOKED, will merge on next! |
|
Adding a data point in case it helps prioritise this one. On 0.2.1-3 (the Omarchy package) the inline walk is what makes flea feel slow to start for me. My home has two folders full of JS monorepo worktrees, about 8.2M entries between them, mostly
Sampling To confirm, I made This PR would fix it properly and keep the sizes. It has shown as conflicting with main since 0.3.0 landed. I can test a rebased build against this tree and report numbers if that is useful. |
|
Follow-up with numbers: I built this branch as it stands (21dace5, on top of v0.2.0) and ran it next to the installed 0.2.1-3 on the same tree as above (~8.2M entries under two folders in Through the GUI: launch at
Startup itself is the same on both. The difference is what happens right after: on 0.2.1 the first action waits about 1.6 s behind the walk, on this branch it is about 57 ms. Driving the backend directly over the protocol says the same thing. Sizes still arrive. Staying in Caveats: warm cache only, and I tested the branch as written, not a rebase onto current main. The conflicts are in |
|
Shipped in |
Opening a small directory can wait behind a recursive size calculation for a directory in the previous view.
dirsizecurrently walks inline in the backend event loop, so the 150 ms loading indicator can appear even when listing the destination itself takes less than a millisecond.This moves size calculation to one persistent worker. The event loop continues handling navigation, and sizes arrive through the existing event channel. Viewport-only requests, the completed-result cache, the 2 s walk deadline, and the wire format are preserved.
dirsizecancelinvalidate unfinished work with a generation counter. Late replies cannot populate a different listing or a reused row index.dirsizecancel.Cancellation is cooperative: a blocked syscall can still delay subsequent size results until it returns. It no longer blocks the navigation event loop. Other synchronous operations, including scanning the destination itself, are outside this change.
Validation
cargo build --lockedandcargo build --release --lockedpassed.cargo test --lockedandcargo test --release --locked: 606 passed, 0 failed in each profile on this isolated branch.BIN=./target/debug/flea bash tests/protocol.shpassed.git diff --checkpassed; the four existing compiler warnings are unchanged.This is targeted validation of the Rust/backend change, not a claim that the entire upstream GUI/test runner is green.
Four new unit tests cover cancellation before/during traversal, a deterministically paused worker whose stale result is rejected after row-index reuse, and a real 1,900-level directory tree. The worker reserves a 16 MiB stack for the existing recursive walk.
The protocol suite also uses a test-only
LD_PRELOAD/opendirbarrier to pin a running size walk and verify cancel/re-request, list, sort, and quit. All four scenarios pass against debug and release; unchanged upstream fails the responsiveness check as expected. This helper requiresccand Python 3. The production binary has no test hooks or new dependencies.Local latency check
Five requests per release build on the same local Btrfs filesystem:
b992e764These are backend request-to-first-rows times, not end-to-end GUI frame times. For each request pair, list the parent of a large local source tree, request that tree's
dirsize, wait 10 ms, then send alistfor a small directory. Measure until the correspondingrowsreply. Both arms use release builds and the same paths; no cache dropping or filesystem modifications are involved.Summary by CodeRabbit
New Features
Bug Fixes
Documentation