Persist stacks to bounded JSONL - #568
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR implements persistent JSONL-based stack storage, exposes RPCs and IPCs for stacks and availability, integrates StackStore into SessionStore and run sinks, updates renderer contracts, and conditionally shows the Stacks UI based on availability. ChangesStack Persistence and Availability
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
3be82ec to
8a113ce
Compare
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 `@src/stack_store.rs`:
- Around line 149-153: The stacks() method needs an explicit guard for limit ==
0 to avoid the current indirect behavior that clears the buffer and sets
trimmed_or_missing incorrectly; add an early return at the top of stacks() that
when limit == 0 returns Ok(StackQueryResult { records: Vec::new(),
trimmed_or_missing: false }) (or the repository's chosen sentinel for "no
trimming") so the boundary is deterministic and later logic never runs for this
case; update any tests or callers if they expect a different trimmed_or_missing
convention.
- Around line 262-263: The call to fs::rename(temp_path, &self.path) in
compaction can behave non-atomically or fail on Windows/other filesystems;
update the compaction code that uses fs::rename(temp_path, &self.path) to first
ensure temp_path and self.path are on the same filesystem (or document that
requirement), and implement a robust fallback: on rename error, perform an
atomic-safe copy (copy file contents from temp_path to self.path, fsync the
destination directory if possible), verify write success, then remove temp_path;
also add a clear error log including the rename error to help debugging and
retry/propagate the error as before.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 239cfee4-d279-4dcd-a185-ca58279bb222
📒 Files selected for processing (14)
desktop/electron/main.cjsdesktop/electron/preload.cjsdesktop/electron/renderer/src/App.jsxdesktop/electron/renderer/src/components/StacksPage.jsxdesktop/electron/renderer/window.d.tsresearch/stack-persistence.mdsrc/bin/nav-local-backend.rssrc/lib.rssrc/session.rssrc/stack_store.rssrc/stacks.rstests/local_backend.rstests/session.rstests/stack_store.rs
Summary
Validation
rtk cargo fmt --all --checkrtk cargo testrtk cargo clippy --all-targets -- -D warningsrtk bun lintrtk bun run renderer:buildrtk bun testpassed outside sandbox after sandbox backend-startup permission errorsSummary by CodeRabbit
New Features
Documentation
Tests