Fix OFFSET calculation in multi-database line format output#27
Fix OFFSET calculation in multi-database line format output#27DanielCardonaRojas merged 3 commits intomainfrom
Conversation
When using multiple databases (via additional_databases config), the OFFSET field was hardcoded to 0 instead of being calculated from the bookmark's resolution. This caused the television integration preview to not scroll to the correct line. Changes: - Modified `write_annotated_line_format` to accept an optional line fetcher function - Updated `write_annotated_bookmarks` to pass the line fetcher through - Modified `handle_list` to create a line fetcher that looks up the correct database for each bookmark Fixes #26 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR extends line-number support for multi-database bookmark listing by refactoring Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 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 (3)
src/cli/output.rs (1)
422-430: ConsiderOption<&dyn Fn(...)>instead of genericFto simplify theNonecall site.The new signature is generic over
F: Fn(&str) -> Option<usize>. This works, but has two downsides:
- Each caller with a distinct closure type monomorphizes the whole function (and, transitively,
write_annotated_line_format).- Callers that want to opt out must write
None as Option<&fn(&str) -> Option<usize>>(seehandle_searchinsrc/cli/handlers.rsline 1744) becauseNonealone can't pinF.Switching to a trait object removes the turbofish and shrinks generated code with negligible runtime cost on this formatting path:
♻️ Proposed signature change
-pub fn write_annotated_bookmarks<F>( - mode: &OutputMode, - bookmarks: &[AnnotatedBookmark], - line_format: Option<&str>, - get_line_fn: Option<&F>, -) -> io::Result<()> -where - F: Fn(&str) -> Option<usize>, -{ +pub fn write_annotated_bookmarks( + mode: &OutputMode, + bookmarks: &[AnnotatedBookmark], + line_format: Option<&str>, + get_line_fn: Option<&dyn Fn(&str) -> Option<usize>>, +) -> io::Result<()> {And the same on
write_annotated_line_format. Callers then passSome(&get_line_fn)as before andNoneworks without a cast. The same treatment can apply towrite_bookmarks_line_format/write_bookmarks_implif you want consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/output.rs` around lines 422 - 430, Change the generic closure parameter on write_annotated_bookmarks to a trait object to avoid monomorphization and awkward None casts: replace the generic F and where F: Fn(&str) -> Option<usize> with a parameter type Option<&dyn Fn(&str) -> Option<usize>> (and remove the generic/where clause), and make the same change for write_annotated_line_format (and optionally write_bookmarks_line_format / write_bookmarks_impl for consistency); update all call sites (e.g., handle_search) to pass Some(&get_line_fn) or plain None without casting and adjust any internal uses to call the Fn via the dyn reference.src/cli/handlers.rs (2)
1740-1745:None as Option<&fn(&str) -> Option<usize>>is an awkward turbofish escape-hatch.This cast is only necessary because
write_annotated_bookmarksis generic overFand Rust can't inferFwhen you passNone. See the paired comment onsrc/cli/output.rs(line 422-430) suggesting adyn Fnsignature, which would reduce this call to simplyNone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/handlers.rs` around lines 1740 - 1745, The call site is using an awkward cast `None as Option<&fn(&str) -> Option<usize>>` to satisfy the generic parameter of output::write_annotated_bookmarks; change the write_annotated_bookmarks signature (and any related callers) to accept a non-generic function pointer type such as Option<&dyn Fn(&str) -> Option<usize>> (or Option<Box<dyn Fn(&str) -> Option<usize>>>) so callers can simply pass None without a cast; update the function signature in src/cli/output.rs (and adjust internal uses) and then replace the call here with output::write_annotated_bookmarks(mode, &annotated, None, None).
1531-1567: Multi-db line lookup is O(N²) and scans even when{LINE}/{OFFSET}aren't used.Two concerns on the new multi-db path:
get_line_fnlinearly scansallon every invocation.write_annotated_line_formatcalls it once per rendered bookmark, so formatting is O(N²) in the number of multi-db bookmarks. The single-db branch already guards onneeds_line(line 1506/1512) and builds an O(1) map — the multi-db branch does neither.db_mapand the closure are built unconditionally, doing wasted work when the user hasn't requested{LINE}/{OFFSET}(e.g., JSON/Table output, or a Custom template without those placeholders).Consider gating on
needs_lineand indexing byshort_iddirectly. Something like:♻️ Proposed refactor
- // Multi-database case with line number support - let mut all = Vec::new(); - // Keep track of which database each bookmark belongs to - let mut db_map: std::collections::HashMap<String, &Database> = - std::collections::HashMap::new(); - for (label, db) in &dbs { - db_map.insert(label.clone(), db); - let bookmarks = db.list_bookmarks(&filter)?; - for bm in bookmarks { - all.push((label.clone(), bm)); - } - } - let annotated: Vec<output::AnnotatedBookmark> = all - .iter() - .map(|(label, bm)| output::AnnotatedBookmark { source: label, bookmark: bm }) - .collect(); - - // Create a line fetcher that looks up the line from the correct database - let get_line_fn = |short_id: &str| -> Option<usize> { - // Find the bookmark with this short_id - for (label, bm) in &all { - if crate::cli::output::short_id(&bm.id) == short_id - && let Some(db) = db_map.get(label) - { - return get_bookmark_line(db, &bm.id, &bm.file_path); - } - } - None - }; - - output::write_annotated_bookmarks( - mode, - &annotated, - args.line_format.as_deref(), - Some(&get_line_fn), - )?; + // Multi-database case + let mut all: Vec<(String, Bookmark)> = Vec::new(); + let mut db_by_label: std::collections::HashMap<&str, &Database> = + std::collections::HashMap::new(); + for (label, db) in &dbs { + db_by_label.insert(label.as_str(), db); + for bm in db.list_bookmarks(&filter)? { + all.push((label.clone(), bm)); + } + } + let annotated: Vec<output::AnnotatedBookmark> = all + .iter() + .map(|(label, bm)| output::AnnotatedBookmark { source: label, bookmark: bm }) + .collect(); + + if needs_line { + // Pre-index by short_id for O(1) lookup during rendering. + let by_short: std::collections::HashMap<String, (&str, &Bookmark)> = all + .iter() + .map(|(label, bm)| (short_id(&bm.id).to_string(), (label.as_str(), bm))) + .collect(); + let get_line_fn = |sid: &str| -> Option<usize> { + let (label, bm) = by_short.get(sid)?; + let db = db_by_label.get(label)?; + get_bookmark_line(db, &bm.id, &bm.file_path) + }; + output::write_annotated_bookmarks( + mode, + &annotated, + args.line_format.as_deref(), + Some(&get_line_fn), + )?; + } else { + output::write_annotated_bookmarks( + mode, + &annotated, + args.line_format.as_deref(), + None as Option<&fn(&str) -> Option<usize>>, + )?; + }Note: the indexed map also makes short-id collisions deterministic (first inserted wins) rather than position-dependent, though collisions on 8-hex-char UUID prefixes are already unlikely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/handlers.rs` around lines 1531 - 1567, The multi-db branch currently builds db_map and a get_line_fn that scans the entire all Vec for every short_id lookup, causing O(N²) formatting and doing wasted work when lines aren't needed; change this to first check needs_line and only then build an O(1 index keyed by short_id (use crate::cli::output::short_id(&bm.id) as the key) that maps to either (label, Bookmark) or directly to (&Database, Bookmark.id, Bookmark.file_path), so write_annotated_bookmarks can call a constant-time lookup; ensure the indexed map insertion uses "first-in wins" semantics for collisions and avoid constructing db_map/get_line_fn when needs_line is false, leaving the single-db fast path unchanged (refer to all, db_map, get_line_fn, needs_line, output::AnnotatedBookmark, and write_annotated_bookmarks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/handlers.rs`:
- Around line 1740-1745: The call site is using an awkward cast `None as
Option<&fn(&str) -> Option<usize>>` to satisfy the generic parameter of
output::write_annotated_bookmarks; change the write_annotated_bookmarks
signature (and any related callers) to accept a non-generic function pointer
type such as Option<&dyn Fn(&str) -> Option<usize>> (or Option<Box<dyn Fn(&str)
-> Option<usize>>>) so callers can simply pass None without a cast; update the
function signature in src/cli/output.rs (and adjust internal uses) and then
replace the call here with output::write_annotated_bookmarks(mode, &annotated,
None, None).
- Around line 1531-1567: The multi-db branch currently builds db_map and a
get_line_fn that scans the entire all Vec for every short_id lookup, causing
O(N²) formatting and doing wasted work when lines aren't needed; change this to
first check needs_line and only then build an O(1 index keyed by short_id (use
crate::cli::output::short_id(&bm.id) as the key) that maps to either (label,
Bookmark) or directly to (&Database, Bookmark.id, Bookmark.file_path), so
write_annotated_bookmarks can call a constant-time lookup; ensure the indexed
map insertion uses "first-in wins" semantics for collisions and avoid
constructing db_map/get_line_fn when needs_line is false, leaving the single-db
fast path unchanged (refer to all, db_map, get_line_fn, needs_line,
output::AnnotatedBookmark, and write_annotated_bookmarks).
In `@src/cli/output.rs`:
- Around line 422-430: Change the generic closure parameter on
write_annotated_bookmarks to a trait object to avoid monomorphization and
awkward None casts: replace the generic F and where F: Fn(&str) -> Option<usize>
with a parameter type Option<&dyn Fn(&str) -> Option<usize>> (and remove the
generic/where clause), and make the same change for write_annotated_line_format
(and optionally write_bookmarks_line_format / write_bookmarks_impl for
consistency); update all call sites (e.g., handle_search) to pass
Some(&get_line_fn) or plain None without casting and adjust any internal uses to
call the Fn via the dyn reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a351b41-ad33-472b-a86d-3fcf213d95a4
📒 Files selected for processing (2)
src/cli/handlers.rssrc/cli/output.rs
1 participant
Problem
When using multiple databases (via
additional_databasesconfig), theOFFSETfield was hardcoded to 0 instead of being calculated from the bookmark's resolution. This caused the television integration preview to not scroll to the correct line.Root Cause
The
write_annotated_line_formatfunction had this code:This was a limitation from when multi-db support was first added - the line number lookup function wasn't being passed through the multi-db code path.
Solution
write_annotated_line_formatto accept an optional line fetcher functionwrite_annotated_bookmarksto pass the line fetcher throughhandle_listto create a line fetcher that looks up the correct database for each bookmarkTesting
{OFFSET}placeholderFixes scrolling issue in television integration when using
additional_databasesconfig.Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit