core: named rows and one home per repeated rule - #135
Conversation
Pure refactor, no behavior change. The wide tuple returns in db.rs (two of them wearing #[allow(clippy::type_complexity)]) become named row structs — ResourceListing, ResourceDetail, ResourceBacklinkRow, ResourceEdgeRow, PendingChunk, InboundEdge — and the inbound-edge queries stop fetching a src_id no consumer ever read. Rules and dances written more than once now live once: the meta reads behind the schema stamp and the recorded embedder (meta_value), the resolve-ref-then-locate opening of every note-addressed façade op (resolve_ref_to_path), the two copy-pasted halves of graph::neighbors (collect_neighbors), the fuse-then-resolve tail shared by the two search entry points (resolve_hits), the mtime stat read (ingest::unix_mtime), and the dot-hidden vault-membership rule that lived as three walk predicates plus validator prose (pathspec::is_hidden). move_note's by_file map drops the src_id it stored but never read, matching move_resource's shape. b2-embed gets the same treatment: files_present is THE installed check the loader, the provision fast path, and the settings picker all share, and embed_err is the one candle-to-core error map. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WE6yzYZhgzwrwYpzaYzMs2
Pure code motion plus adapter dedup, byte-identical output (the 48-test output contract passes unchanged). Each subcommand's body moves verbatim out of the 620-line dispatch match into its own cmd_* fn; the reindex arm's two println! walls split into print_reindex_plan / print_reindex_report. The 19 hand-rolled --json sites share one print_json (serde joins the manifest for the trait bound only); mv/rm stop serializing reports they throw away in human mode; the presentation rules get names (display_name, arrow, decorate, is_dir_arg, print_rewrite_tally, print_dangled); the 10-arm debug-detail match — every arm already equal to err.to_string() through #[error(transparent)] — collapses to that one call; open_vault drops the (Vault, bool) tuple nine of ten callers discarded (search asks use_fake_embedder() directly, same value by construction); vault_or_cwd returns &Path instead of cloning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WE6yzYZhgzwrwYpzaYzMs2
Pure refactor, IPC shapes and user-facing strings unchanged. The 23 call sites discarding open_vault's semantic flag become open_read / open_semantic, so a command's model posture reads in the call itself; the two byte-identical error-detail matches collapse to err.to_string() (Core/Embed are #[error(transparent)]); user_message trades its wildcard arm for the explicit Core|Embed pattern, so a new host variant fails to compile instead of silently degrading to "Something went wrong"; the stats ledger's serialize-and-write tail becomes write_ledger (parent creation stays record_to's alone); poisoned-lock recovery and its rationale live once in lock_recover; EmbedStat maps via From instead of a hand-copied closure; and ReindexGuard moves next to the AppState slot it guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WE6yzYZhgzwrwYpzaYzMs2
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR refactors CLI and desktop command handling, introduces structured core database records, centralizes path, metadata, timestamp, embedding, locking, and output helpers, and updates consumers across core operations without changing reported behavior. ChangesCore database and operation refactoring
CLI command refactoring
Desktop and embedding refactoring
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
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 `@crates/b2-core/src/ingest.rs`:
- Around line 1204-1205: Move the pathspec::is_hidden check in
collect_vault_files ahead of the note-classification match so hidden paths are
excluded before the None arm can add them to notes, while preserving recursive
directory and resource handling for visible paths. Add a regression test
covering a dot-prefixed Markdown file and verify it is absent from the collected
vault paths.
In `@crates/b2-core/src/search.rs`:
- Around line 140-151: Update resolve_hits so limit == 0 returns immediately,
then iterate through all fused candidates without applying take(limit) before
resolution; only stop once hits.len() reaches limit, allowing later valid notes
to replace unresolved chunks while preserving ranked order.
🪄 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: d9a48398-9740-4be1-b50c-16ac1fc60b1f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
crates/b2-cli/Cargo.tomlcrates/b2-cli/src/main.rscrates/b2-core/src/db.rscrates/b2-core/src/dirs.rscrates/b2-core/src/graph.rscrates/b2-core/src/ingest.rscrates/b2-core/src/mv.rscrates/b2-core/src/pathspec.rscrates/b2-core/src/rm.rscrates/b2-core/src/search.rscrates/b2-core/src/vault.rscrates/b2-core/tests/write.rscrates/b2-desktop/src/commands.rscrates/b2-desktop/src/error.rscrates/b2-desktop/src/main.rscrates/b2-desktop/src/stats.rscrates/b2-desktop/src/watch.rscrates/b2-embed/src/config.rscrates/b2-embed/src/model.rscrates/b2-embed/src/provision.rs
Doc-only. The predicate's comment said a dot-prefixed name is never vault material, but the ingest walk's note route deliberately keeps its historical behavior (a dot-prefixed .md still indexes — collect_vault_files says so). One definition of hidden, applied per route — say exactly that, so a reader (or a reviewer) can't take the predicate as a claim the walk doesn't make. Surfaced by PR #135 review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WE6yzYZhgzwrwYpzaYzMs2
Pure refactor, no behavior change. The wide tuple returns in db.rs
(two of them wearing #[allow(clippy::type_complexity)]) become named
row structs — ResourceListing, ResourceDetail, ResourceBacklinkRow,
ResourceEdgeRow, PendingChunk, InboundEdge — and the inbound-edge
queries stop fetching a src_id no consumer ever read. Rules and dances
written more than once now live once: the meta reads behind the
schema stamp and the recorded embedder (meta_value), the
resolve-ref-then-locate opening of every note-addressed façade op
(resolve_ref_to_path), the two copy-pasted halves of graph::neighbors
(collect_neighbors), the fuse-then-resolve tail shared by the two
search entry points (resolve_hits), the mtime stat read
(ingest::unix_mtime), and the dot-hidden vault-membership rule that
lived as three walk predicates plus validator prose
(pathspec::is_hidden). move_note's by_file map drops the src_id it
stored but never read, matching move_resource's shape. b2-embed gets
the same treatment: files_present is THE installed check the loader,
the provision fast path, and the settings picker all share, and
embed_err is the one candle-to-core error map.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01WE6yzYZhgzwrwYpzaYzMs2
Summary by CodeRabbit