refactor: use DELETE ... LIMIT

[OGKevin]

Change-Id: 947ed5374ff74d8674d9355357a45c43
Change-Id-Short: qvslmuwsvkks

[OGKevin]

• refactor: use DELETE ... LIMIT #585 👈 (View in Graphite)
• feat: build SQLite from source with DELETE … LIMIT support #584
• refactor: cp_r is generic #583
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[OGKevin]

@copilot can you reproduce this failure in your environment and open a new PR with your proposed fix to resolve the CI failure. I can't spot the diff in devenv.nix on MacOS and therefore not sure what the issue is.

error: failed to run custom build command for `libsqlite3-sys v0.30.1`

Caused by:
  process didn't exit successfully: `/home/runner/work/cadmus/cadmus/target/debug/build/libsqlite3-sys-b988ea1e2e988e87/build-script-build` (exit status: 101)
  --- stdout
  cargo:rerun-if-env-changed=LIBSQLITE3_SYS_USE_PKG_CONFIG
  cargo:rerun-if-env-changed=SQLITE3_INCLUDE_DIR
  cargo:rerun-if-env-changed=SQLITE3_LIB_DIR
  cargo:rerun-if-env-changed=SQLITE3_STATIC
  cargo:link-target=sqlite3
  cargo:rerun-if-env-changed=SQLITE3_NO_PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG
  cargo:rerun-if-env-changed=SQLITE3_STATIC
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_SYSROOT_DIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR
  cargo:rerun-if-env-changed=SYSROOT
  cargo:rerun-if-env-changed=SQLITE3_STATIC
  cargo:rustc-link-search=native=/usr/lib/x86_64-linux-gnu
  cargo:rustc-link-lib=sqlite3
  cargo:rustc-link-lib=m
  cargo:rustc-link-lib=z
  cargo:rerun-if-env-changed=PKG_CONFIG_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG
  cargo:rerun-if-env-changed=SQLITE3_STATIC
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_SYSROOT_DIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR
  cargo:rerun-if-env-changed=LIBSQLITE3_SYS_BUNDLING

  --- stderr

  thread 'main' (5223) panicked at /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/libsqlite3-sys-0.30.1/build.rs:620:33:
  could not run bindgen on header target/cadmus-build-deps/x86_64-unknown-linux-gnu/sqlite/include/sqlite3.h
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: Process completed with exit code 101.

[Copilot]

@OGKevin I've opened a new pull request, #586, to work on those changes. Once the pull request is ready, I'll request review from you.

[OGKevin]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

This PR moves SQLite discovery to target-aware pkg-config paths, embeds a sqlite3.pc template and generates target-local pkg-config metadata during the SQLite build, extends SQLite compile-time defines and builds object code with -fPIC, updates CI and devenv to expose PKG_CONFIG_PATH_ and LIBCLANG_PATH, adds a GitHub Action step to export LIBCLANG_PATH, updates the Kobo build action to run setup and enable cross pkg-config, and changes dictionary entry deletion to batched DELETE ... LIMIT loops.

Sequence Diagram(s)

sequenceDiagram
  participant ensure_sqlite
  participant configure_amalgamation
  participant compile_amalgamation
  participant write_pkgconfig
  participant cargo_build

  ensure_sqlite->>configure_amalgamation: prepare sqlite amalgamation (VERSION read)
  configure_amalgamation->>compile_amalgamation: compile sqlite3.c with defines (-O2 -fPIC)
  compile_amalgamation->>write_pkgconfig: supply {lib, inc, version}
  write_pkgconfig->>cargo_build: write lib/pkgconfig/sqlite3.pc
  cargo_build->>write_pkgconfig: locate sqlite via PKG_CONFIG_PATH_<target>

Loading

Possibly related PRs

• OGKevin/cadmus#584: Related changes to the SQLite-from-source build and pkg-config generation.
• OGKevin/cadmus#571: Prior adjustments to CROSS_ENV handling for Kobo cross-compilation that this PR builds on.

Suggested labels

dependencies

SQLite takes the scenic pkg-config route,
No hardcoded paths in the boot,
With cross-compilation in sight,
And clang libs shining bright,
The build dance finds a cleaner suit! 🎵

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'refactor: use DELETE ... LIMIT' follows the Conventional Commits format with a 'refactor' type and describes a specific implementation change, directly addressing the main alteration in crates/core/src/task/dictionary_index.rs.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

crates/core/src/task/dictionary_index.rs (1)

747-790: 💤 Low value

Consider adding a test that exercises the batching loop.

The existing tests use 5 entries or fewer, which never triggers the loop iteration (since BATCH_SIZE is 5000). A test with entries exceeding BATCH_SIZE would validate the accumulation logic actually works across multiple iterations.

Not blocking—the current tests cover the core deletion correctness, and forcing 5000+ inserts in a unit test is a bit heavy. Just something to consider if you want fuller coverage.

🤖 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/core/src/task/dictionary_index.rs` around lines 747 - 790, Add a test
that triggers the batching loop by inserting more than BATCH_SIZE entries and
calling delete_entries_for_dict to ensure multiple iterations run: in a new test
(similar to test_delete_entries_for_dict_removes_all_entries) obtain the pool,
create a dict via insert_meta, then insert BATCH_SIZE + 1 entries (use the
BATCH_SIZE constant from the module) with insert_entry in a loop, call
delete_entries_for_dict(pool, dict_id, &shutdown).await.expect(...), and assert
the deleted count equals BATCH_SIZE + 1 and count_entries(pool, dict_id).await
== 0; reuse RUNTIME.block_on and ShutdownSignal::never() like the other tests to
execute async code.

🤖 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 @.github/actions/apt/action.yml:
- Around line 23-27: The current snippet sets libdir from llvm-config or from
dirname(find ...) and then unconditionally exports LIBCLANG_PATH, but when find
yields nothing dirname returns "." and we export an invalid LIBCLANG_PATH;
update the logic around the libdir variable: after attempting llvm-config and
the find/dirname fallback, validate that libdir is non-empty and not "." (and
not equal to "/" if you prefer) before appending "LIBCLANG_PATH=$libdir" to
GITHUB_ENV, and if validation fails, either emit a clear error and exit (so
bindgen failures aren’t masked) or skip exporting to avoid masking the discovery
failure; reference the libdir variable and the echo "LIBCLANG_PATH=..." export
in your change.

In `@crates/build-deps/src/build/sqlite.rs`:
- Around line 148-172: The call to Assets::get(...).expect(...) in
write_pkgconfig should return an error via the function Result instead of
panicking; replace the expect usage with a fallible call that uses the ?
operator and add context (e.g., via .context("...") from anyhow/Context) so the
failure to find the embedded "sqlite3.pc.template" is propagated; update the
variable binding for template accordingly and keep the rest of write_pkgconfig
(including subsequent .context calls) unchanged.

In `@devenv.nix`:
- Around line 400-406: Remove or conditionalize the hard-set SQLITE3_LIB_DIR and
SQLITE3_INCLUDE_DIR so they don’t override target-aware pkg-config routing;
specifically, stop exporting SQLITE3_LIB_DIR and SQLITE3_INCLUDE_DIR
unconditionally in the section that also sets PKG_CONFIG_PATH_${...} (and the
explicit PKG_CONFIG_PATH_arm_unknown_linux_gnueabihf) so libsqlite3-sys can use
PKG_CONFIG_PATH_<target> to pick the cross-target sqlite. Modify the devenv.nix
logic around the SQLITE3_* variables to either omit them for cross builds or
ensure they are computed per-target (matching the same
${pkgs.stdenv.hostPlatform.rust.rustcTargetSpec} used for
PKG_CONFIG_PATH_<target>), and leave the PKG_CONFIG_PATH_<target> entries as the
authoritative routing mechanism.

---

Nitpick comments:
In `@crates/core/src/task/dictionary_index.rs`:
- Around line 747-790: Add a test that triggers the batching loop by inserting
more than BATCH_SIZE entries and calling delete_entries_for_dict to ensure
multiple iterations run: in a new test (similar to
test_delete_entries_for_dict_removes_all_entries) obtain the pool, create a dict
via insert_meta, then insert BATCH_SIZE + 1 entries (use the BATCH_SIZE constant
from the module) with insert_entry in a loop, call delete_entries_for_dict(pool,
dict_id, &shutdown).await.expect(...), and assert the deleted count equals
BATCH_SIZE + 1 and count_entries(pool, dict_id).await == 0; reuse
RUNTIME.block_on and ShutdownSignal::never() like the other tests to execute
async code.

🪄 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: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: dbdb0bc9-a95d-41dd-9baa-678a4d29f1bc

📥 Commits

Reviewing files that changed from the base of the PR and between 714bd49 and 663f6df.

⛔ Files ignored due to path filters (4)

• .sqlx/query-8cbf73c5365d86fbc42959f30555ac5a3169404e401cc439fd4f31e349a92626.json is excluded by !.sqlx/**
• .sqlx/query-9df58f98329eddad68ccb464dcec346042e7573735b717dbfec78d53551ee8c1.json is excluded by !.sqlx/**
• .sqlx/query-df3706df659c5cf32a664296c12fa3c02df879b4b9db4b09afac9f0c08c22243.json is excluded by !.sqlx/**
• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (13)

• .github/actions/apt/action.yml
• .github/actions/build-kobo/action.yml
• .github/workflows/cargo.yml
• Cargo.toml
• build-scripts/sqlite/README-cadmus.md
• crates/build-deps/Cargo.toml
• crates/build-deps/assets/sqlite3.pc.template
• crates/build-deps/src/build/sqlite.rs
• crates/build-deps/src/versions.rs
• crates/core/Cargo.toml
• crates/core/src/task/dictionary_index.rs
• devenv.nix
• xtask/src/tasks/setup.rs

📜 Review details

🧰 Additional context used

📓 Path-based instructions (7)

Cargo.toml

📄 CodeRabbit inference engine (AGENTS.md)

Define dependency versions in root Cargo.toml under [workspace.dependencies]

Files:

• Cargo.toml

**/Cargo.toml

📄 CodeRabbit inference engine (AGENTS.md)

**/Cargo.toml: Use caret requirements for most dependencies in Cargo.toml
Alphabetize dependencies within logical groups in Cargo.toml
Use default-features = false when fine-grained control is needed in Cargo.toml
Keep related crate families at compatible versions (e.g. all opentelemetry-* crates) in Cargo.toml

Files:

• Cargo.toml
• crates/build-deps/Cargo.toml
• crates/core/Cargo.toml

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Prefer ? operator over unwrap() / expect() in library and app code
Use thiserror for custom error types and anyhow for ad-hoc errors in Rust
Use iterators over index-based loops in Rust
Use &str over String in function parameters when ownership is not needed
Prefer borrowing over cloning in Rust
Inline expressions directly into struct fields — avoid intermediate bindings solely to pass them into a struct literal
Avoid unsafe unless required and documented in Rust
Avoid premature collect() — keep iterators lazy in Rust
Ensure Rust code compiles without warnings
Prefer newtype wrappers over raw primitives for domain concepts (e.g., Fingerprint(String) instead of bare String)
Wrap String, i64, and similar primitives when the value represents a specific domain concept (fingerprints, file kinds, identifiers, etc.)
Implement Display, FromStr, and other standard traits on newtype wrappers to keep them ergonomic
Match on enum variants instead of string comparisons — the compiler catches missing arms
Comment why, not what. Avoid inline comments — extract code into well-named, documented functions instead
Avoid dead-code comments (commented-out code) in Rust
Avoid changelog comments (Modified by X on date) in Rust
Avoid divider comments (//=====) in Rust
Annotations (TODO, FIXME, HACK, NOTE) are acceptable with context in Rust
Use structured fields with the tracing crate — never use string formatting for log data
Use only structured fields for logging — data goes in fields, not format strings
Do not use module prefixes in logs — instrumentation scope provides context
Do not mix structured fields with format arguments in logging
Use debug log level for development and troubleshooting detail
Use info log level for important runtime events
Use warn log level for recoverable issues (retries, fallbacks)
Use error log level for failures requiring attention
Return domain types directly from sqlx queries instead of parsin...

Files:

• xtask/src/tasks/setup.rs
• crates/core/src/task/dictionary_index.rs
• crates/build-deps/src/build/sqlite.rs
• crates/build-deps/src/versions.rs

.github/workflows/cargo.yml

📄 CodeRabbit inference engine (AGENTS.md)

.github/workflows/cargo.yml: The clippy and test jobs in .github/workflows/cargo.yml use a matrix to check every feature flag individually and in combination
For workspace-wide feature flags in CI, use --workspace --all-targets --features <name> in .github/workflows/cargo.yml
For crate-specific feature flags in CI, use -p <crate> --all-targets --features <name> in .github/workflows/cargo.yml
All feature flag combinations should run on ubuntu-latest in .github/workflows/cargo.yml
Only default and test features should produce build artifacts in .github/workflows/cargo.yml

Files:

• .github/workflows/cargo.yml

crates/core/**/*.rs

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

crates/core/**/*.rs: Rustdoc examples should follow this preference order: fully compilable > no_run > ignore. Use no_run when the example compiles but cannot execute (file I/O, network, database). Use ignore only for private/pub(crate) items unreachable from the test harness with a comment explaining why. Use # to hide boilerplate setup lines. Verify with cargo test --doc.
Use typed SQL macros (sqlx::query!, sqlx::query_as!, sqlx::query_scalar!) in Rust code. Never use untyped sqlx::query() / sqlx::query_as() / sqlx::query_scalar(). Use .flatten() on query_scalar! results for nullable columns (Option<Option<T>> → Option<T>).
Untyped SQL queries are allowed only for dynamic SQL (e.g. runtime ORDER BY column) if and only if the function has unit tests covering every dynamic path, with a comment explaining why the typed macro cannot be used.
All user-visible strings must use the fl! macro (use crate::fl;). Never hardcode string literals for labels, buttons, placeholders, or notifications. User-visible strings include: label() implementations on SettingKind traits, input field label strings in Event::OpenNamedInput, menu entry text (EntryKind::Command, EntryKind::RadioButton, etc.), and button labels, notification text.

Files:

• crates/core/src/task/dictionary_index.rs

crates/core/**/*.{sql,rs}

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

All SQL queries must list explicit column names. Do not use SELECT *.

Files:

• crates/core/src/task/dictionary_index.rs

crates/core/**/*.{ftl,rs}

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

Every user-visible string needs a Fluent message ID in crates/core/i18n/en-GB/cadmus_core.ftl. Use fl!("message-id") at the call site. For parameterised strings, use Fluent variables ({ $var }) in the .ftl file and pass values via fl!("id", var = value).

Files:

• crates/core/src/task/dictionary_index.rs

🧠 Learnings (1)

📚 Learning: 2026-05-23T14:57:43.820Z

Learnt from: OGKevin
Repo: OGKevin/cadmus PR: 478
File: crates/core/src/view/settings_editor/refresh_rate_by_kind_editor.rs:91-91
Timestamp: 2026-05-23T14:57:43.820Z
Learning: When reviewing Rust code that calls `ToggleableKeyboard::new(parent_rect: Rectangle, number_mode: bool)` (from `crates/core/src/view/toggleable_keyboard.rs`), treat the second argument as `number_mode` (numeric/number-layout mode), not as an initial visibility flag. In the constructor implementation, `visible` is initialized to `false` and the keyboard starts hidden; only the numeric layout changes based on `number_mode` (e.g., for numeric inputs like `refresh rate u8`). Therefore, do not flag `ToggleableKeyboard::new(rect, true)` as “making the keyboard start visible.”

Applied to files:

• crates/core/src/task/dictionary_index.rs

🔇 Additional comments (18)

xtask/src/tasks/setup.rs (1)

107-107: LGTM!

crates/core/src/task/dictionary_index.rs (3)

521-522: LGTM!

Also applies to: 604-605

---

632-644: LGTM!

---

623-630: DELETE … LIMIT flag: SQLITE_ENABLE_UPDATE_DELETE_LIMIT is enabled—crates/build-deps/src/build/sqlite.rs passes -DSQLITE_ENABLE_UPDATE_DELETE_LIMIT, so the DELETE ... LIMIT at lines 623-630 shouldn’t fail with a syntax error.

Cargo.toml (1)

21-21: LGTM!

crates/build-deps/Cargo.toml (1)

14-14: LGTM!

crates/build-deps/assets/sqlite3.pc.template (1)

1-9: LGTM!

crates/build-deps/src/build/sqlite.rs (6)

41-43: LGTM!

---

54-55: LGTM!

---

97-99: LGTM!

---

106-107: LGTM!

---

137-138: LGTM!

---

226-229: LGTM!

build-scripts/sqlite/README-cadmus.md (1)

42-51: LGTM!

.github/workflows/cargo.yml (1)

7-9: LGTM!

.github/actions/build-kobo/action.yml (1)

65-67: LGTM!

Also applies to: 73-75

crates/build-deps/src/versions.rs (1)

71-83: LGTM!

crates/core/Cargo.toml (1)

72-75: LGTM!

[OGKevin]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (2)

crates/build-deps/src/build/sqlite.rs (1)

234-236: ⚡ Quick win

Consider using .extend() for cleaner collection building.

The for-loop at lines 234-236 works but could be more idiomatic. Instead of iterating and pushing each define, you can use .extend() to append all items at once.

♻️ Suggested refactor

-    for define in SQLITE_DEFINES {
-        compile_args.push(define);
-    }
+    compile_args.extend(SQLITE_DEFINES);

🤖 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/build-deps/src/build/sqlite.rs` around lines 234 - 236, Replace the
explicit loop that pushes each item into compile_args with a single call to
extend to append all elements from SQLITE_DEFINES at once; locate the loop that
iterates over SQLITE_DEFINES and pushes into compile_args and change it so
compile_args.extend(SQLITE_DEFINES) (or compile_args.extend_from_slice if types
require) to make the collection building more idiomatic and concise.

xtask/src/tasks/setup.rs (1)

109-121: 💤 Low value

Windows target detection might be worth adding.

The fallback logic at line 119 defaults to x86_64-unknown-linux-gnu when no platform matches, which means Windows hosts aren't explicitly handled. For an internal tool this might be fine, but if Windows is a supported dev environment you might want to add:

} else if cfg!(target_arch = "x86_64") && cfg!(target_os = "windows") {
    "x86_64-pc-windows-msvc".to_string()

🤖 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 `@xtask/src/tasks/setup.rs` around lines 109 - 121, The TARGET fallback logic
using std::env::var("TARGET").unwrap_or_else currently omits Windows hosts;
update the unwrap_or_else closure (the branch logic around the
cfg!(target_arch/target_os) checks) to add Windows entries — e.g., add branches
for x86_64 + target_os = "windows" returning
"x86_64-pc-windows-msvc".to_string() (and optionally the aarch64 Windows triple
if you need ARM64 Windows support) so Windows dev environments are detected
instead of falling back to the Linux triple.

🤖 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/build-deps/src/build/sqlite.rs`:
- Around line 234-236: Replace the explicit loop that pushes each item into
compile_args with a single call to extend to append all elements from
SQLITE_DEFINES at once; locate the loop that iterates over SQLITE_DEFINES and
pushes into compile_args and change it so compile_args.extend(SQLITE_DEFINES)
(or compile_args.extend_from_slice if types require) to make the collection
building more idiomatic and concise.

In `@xtask/src/tasks/setup.rs`:
- Around line 109-121: The TARGET fallback logic using
std::env::var("TARGET").unwrap_or_else currently omits Windows hosts; update the
unwrap_or_else closure (the branch logic around the cfg!(target_arch/target_os)
checks) to add Windows entries — e.g., add branches for x86_64 + target_os =
"windows" returning "x86_64-pc-windows-msvc".to_string() (and optionally the
aarch64 Windows triple if you need ARM64 Windows support) so Windows dev
environments are detected instead of falling back to the Linux triple.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 9d93eea3-05b2-416f-a9fa-65fdccab7964

📥 Commits

Reviewing files that changed from the base of the PR and between 663f6df and 7ef523e.

⛔ Files ignored due to path filters (4)

• .sqlx/query-8cbf73c5365d86fbc42959f30555ac5a3169404e401cc439fd4f31e349a92626.json is excluded by !.sqlx/**
• .sqlx/query-9df58f98329eddad68ccb464dcec346042e7573735b717dbfec78d53551ee8c1.json is excluded by !.sqlx/**
• .sqlx/query-df3706df659c5cf32a664296c12fa3c02df879b4b9db4b09afac9f0c08c22243.json is excluded by !.sqlx/**
• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (13)

• .github/actions/apt/action.yml
• .github/actions/build-kobo/action.yml
• .github/workflows/cargo.yml
• Cargo.toml
• build-scripts/sqlite/README-cadmus.md
• crates/build-deps/Cargo.toml
• crates/build-deps/assets/sqlite3.pc.template
• crates/build-deps/src/build/sqlite.rs
• crates/build-deps/src/versions.rs
• crates/core/Cargo.toml
• crates/core/src/task/dictionary_index.rs
• devenv.nix
• xtask/src/tasks/setup.rs

✅ Files skipped from review due to trivial changes (1)

• build-scripts/sqlite/README-cadmus.md

🚧 Files skipped from review as they are similar to previous changes (9)

• crates/core/Cargo.toml
• .github/actions/apt/action.yml
• crates/build-deps/assets/sqlite3.pc.template
• crates/build-deps/Cargo.toml
• .github/workflows/cargo.yml
• Cargo.toml
• crates/core/src/task/dictionary_index.rs
• .github/actions/build-kobo/action.yml
• crates/build-deps/src/versions.rs

📜 Review details

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Prefer ? operator over unwrap() / expect() in library and app code
Use thiserror for custom error types and anyhow for ad-hoc errors in Rust
Use iterators over index-based loops in Rust
Use &str over String in function parameters when ownership is not needed
Prefer borrowing over cloning in Rust
Inline expressions directly into struct fields — avoid intermediate bindings solely to pass them into a struct literal
Avoid unsafe unless required and documented in Rust
Avoid premature collect() — keep iterators lazy in Rust
Ensure Rust code compiles without warnings
Prefer newtype wrappers over raw primitives for domain concepts (e.g., Fingerprint(String) instead of bare String)
Wrap String, i64, and similar primitives when the value represents a specific domain concept (fingerprints, file kinds, identifiers, etc.)
Implement Display, FromStr, and other standard traits on newtype wrappers to keep them ergonomic
Match on enum variants instead of string comparisons — the compiler catches missing arms
Comment why, not what. Avoid inline comments — extract code into well-named, documented functions instead
Avoid dead-code comments (commented-out code) in Rust
Avoid changelog comments (Modified by X on date) in Rust
Avoid divider comments (//=====) in Rust
Annotations (TODO, FIXME, HACK, NOTE) are acceptable with context in Rust
Use structured fields with the tracing crate — never use string formatting for log data
Use only structured fields for logging — data goes in fields, not format strings
Do not use module prefixes in logs — instrumentation scope provides context
Do not mix structured fields with format arguments in logging
Use debug log level for development and troubleshooting detail
Use info log level for important runtime events
Use warn log level for recoverable issues (retries, fallbacks)
Use error log level for failures requiring attention
Return domain types directly from sqlx queries instead of parsin...

Files:

• xtask/src/tasks/setup.rs
• crates/build-deps/src/build/sqlite.rs

🪛 GitHub Actions: PR `#585` / 0_Analyze (rust).txt

crates/build-deps/src/build/sqlite.rs

[warning] 105-105: CodeQL WARN: macro expansion failed for '$crate::format_args_nl' (multiple similar macro-expansion warnings reported in this file).

🔇 Additional comments (7)

crates/build-deps/src/build/sqlite.rs (2)

148-173: LGTM!

---

1-89: LGTM!

Also applies to: 90-146, 175-206

devenv.nix (1)

393-410: LGTM!

xtask/src/tasks/setup.rs (4)

1-28: LGTM!

---

30-83: LGTM!

---

107-108: Nice defensive programming.

The #[must_use] attribute ensures the return value won't be accidentally discarded. Good call.

---

89-104: LGTM!