feat: Bump MSRV to 1.94 and utilize new language features#197
Merged
Conversation
github-actions
Bot
added
the
documentation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBumps Rust MSRV to 1.94, adds .gitignore and Clippy settings, and replaces several eager String-producing helpers with formatter-returning Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Welcome to Dependi, @DanikVitek! 🎉 Thank you for your first contribution to this project! We really appreciate you taking the time to help improve Dependi. A maintainer will review your PR soon. In the meantime, please make sure:
If you have any questions, feel free to ask here. Happy coding! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
dependi-lsp/src/registries/rubygems.rs (1)
153-160: Consider using fully inline format args inensure!for consistency.The
anyhow::ensure!macro mixes inline capture{package_name}with a positional{}for the status. While this works correctly, it may conflict with theallow-mixed-uninlined-format-args = falseClippy configuration if the lint applies to this macro.♻️ Suggested refactor to use fully inline args
+ let status = gem_response.status(); anyhow::ensure!( gem_response.status().is_success(), - "Failed to fetch gem info for {package_name}: {}", - gem_response.status(), + "Failed to fetch gem info for {package_name}: {status}", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/registries/rubygems.rs` around lines 153 - 160, The ensure! message mixes an inline `{package_name}` with a positional `{}` for the status; to avoid mixed uninlined format args, capture the status into a local (e.g., let status = gem_response.status();) and change the ensure! call to use fully inline placeholders like "Failed to fetch gem info for {package_name}: {status}" so the macro uses named placeholders consistently (refer to gem_url, gem_response and the ensure! invocation).dependi-lsp/src/registries/cargo_sparse.rs (1)
106-111: Optional consistency refactor: useanyhow::ensure!for status checks here too.Line 106 still uses manual branching while other registry clients in this PR now use
ensure!.♻️ Suggested consistency patch
- if !response.status().is_success() { - anyhow::bail!( - "Failed to fetch crate info for {package_name} from sparse registry: {}", - response.status() - ); - } + anyhow::ensure!( + response.status().is_success(), + "Failed to fetch crate info for {package_name} from sparse registry: {}", + response.status() + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/registries/cargo_sparse.rs` around lines 106 - 111, The status check currently uses a manual if/anyhow::bail branch for response and package_name; replace it with anyhow::ensure!(response.status().is_success(), "Failed to fetch crate info for {package_name} from sparse registry: {}", response.status()) to match the other registry clients, and add use anyhow::ensure if not already imported so the ensure! macro is available.dependi-lsp/src/auth/mod.rs (1)
230-231: Nit: "Safety" section is unconventional here.The
# Safetydoc section is typically reserved for documenting invariants that callers ofunsafefunctions must uphold. Since this function is safe Rust, consider renaming to# Notesor# Unicode Handlingto clarify this describes implementation behavior rather thanunsafecontracts.📝 Suggested doc update
/// Returns an [`fmt::Display`] implemetation which redacts the given token for safe logging. /// /// Shows only the first few characters to help identify which token is in use /// without exposing the full secret. /// -/// # Safety +/// # Unicode Handling /// This function is UTF-8 safe and operates on characters, not bytes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/auth/mod.rs` around lines 230 - 231, The doc comment for the function that says "This function is UTF-8 safe and operates on characters, not bytes." uses an inappropriate "# Safety" header; change that header to something like "# Notes" or "# Unicode Handling" to avoid implying unsafe caller invariants (update the doc comment above the function whose doc contains that exact sentence).dependi-lsp/src/reports.rs (1)
114-117: Consider simplifying the string comparison.The double-deref comparison
**package != *current_packageworks correctly (both sides coerce tostr), but it's a bit dense. A clearer alternative would bepackage != ¤t_packageor usingas_str():if package.as_str() != current_package || version.as_str() != current_version {That said, the current code is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/reports.rs` around lines 114 - 117, Replace the dense double-deref comparisons in the conditional with clearer comparisons: change the condition in the block containing **package != *current_package || **version != *current_version to compare package and version against the current values directly (e.g. package != ¤t_package || version != ¤t_version or package.as_str() != current_package || version.as_str() != current_version), keeping the rest of the body (updating current_package/current_version and writeln!) unchanged; this simplifies the expression while preserving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dependi-lsp/src/backend.rs`:
- Around line 1282-1287: The header is split across lines because writeln!(f,
"\n### ⚠ {vulns_count} Security ",)? adds a trailing newline before
f.write_str(...) — fix by writing the whole header in one write call instead of
two: replace the separate writeln!(...) and f.write_str(...) sequence (around
writeln!(f, "\n### ⚠ {vulns_count} Security ",)? and the following
f.write_str(...) call) with a single write!/writeln! that formats the pluralized
"Vulnerability"/"Vulnerabilities" inline so the output becomes "### ⚠
{vulns_count} Security Vulnerabilities" on one line.
In `@dependi-lsp/src/registries/go_proxy.rs`:
- Around line 176-179: The code currently builds repository URLs using the full
module_path which can include submodules or a vN suffix; update the logic in
dependi-lsp/src/registries/go_proxy.rs where repository is computed from
module_path so that for hosts like "github.com" and "gitlab.com" you normalize
to the VCS root by splitting module_path on '/' and using only the first
up-to-three segments (i.e., domain/org/repo), falling back to the whole path if
it has fewer than three segments; then set repository to
Some(format!("https://{root}")) instead of using the entire module_path so links
point to the actual repo root (handle cases like "github.com/org/repo/submodule"
and "github.com/org/repo/v2").
---
Nitpick comments:
In `@dependi-lsp/src/auth/mod.rs`:
- Around line 230-231: The doc comment for the function that says "This function
is UTF-8 safe and operates on characters, not bytes." uses an inappropriate "#
Safety" header; change that header to something like "# Notes" or "# Unicode
Handling" to avoid implying unsafe caller invariants (update the doc comment
above the function whose doc contains that exact sentence).
In `@dependi-lsp/src/registries/cargo_sparse.rs`:
- Around line 106-111: The status check currently uses a manual if/anyhow::bail
branch for response and package_name; replace it with
anyhow::ensure!(response.status().is_success(), "Failed to fetch crate info for
{package_name} from sparse registry: {}", response.status()) to match the other
registry clients, and add use anyhow::ensure if not already imported so the
ensure! macro is available.
In `@dependi-lsp/src/registries/rubygems.rs`:
- Around line 153-160: The ensure! message mixes an inline `{package_name}` with
a positional `{}` for the status; to avoid mixed uninlined format args, capture
the status into a local (e.g., let status = gem_response.status();) and change
the ensure! call to use fully inline placeholders like "Failed to fetch gem info
for {package_name}: {status}" so the macro uses named placeholders consistently
(refer to gem_url, gem_response and the ensure! invocation).
In `@dependi-lsp/src/reports.rs`:
- Around line 114-117: Replace the dense double-deref comparisons in the
conditional with clearer comparisons: change the condition in the block
containing **package != *current_package || **version != *current_version to
compare package and version against the current values directly (e.g. package !=
¤t_package || version != ¤t_version or package.as_str() !=
current_package || version.as_str() != current_version), keeping the rest of the
body (updating current_package/current_version and writeln!) unchanged; this
simplifies the expression while preserving behavior.
🪄 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
Run ID: 0dd56577-72ac-42b6-97f8-f2ad20c77601
📒 Files selected for processing (41)
.gitignoreCONTRIBUTING.mddependi-lsp/.clippy.tomldependi-lsp/Cargo.tomldependi-lsp/benches/benchmarks.rsdependi-lsp/src/auth/mod.rsdependi-lsp/src/backend.rsdependi-lsp/src/bin/test_dates.rsdependi-lsp/src/cache/mod.rsdependi-lsp/src/cache/sqlite.rsdependi-lsp/src/file_types.rsdependi-lsp/src/main.rsdependi-lsp/src/parsers/cargo.rsdependi-lsp/src/parsers/csharp.rsdependi-lsp/src/parsers/npm.rsdependi-lsp/src/parsers/npm_lock.rsdependi-lsp/src/parsers/php.rsdependi-lsp/src/parsers/python.rsdependi-lsp/src/parsers/ruby.rsdependi-lsp/src/providers/code_actions.rsdependi-lsp/src/providers/completion.rsdependi-lsp/src/providers/diagnostics.rsdependi-lsp/src/providers/document_links.rsdependi-lsp/src/providers/inlay_hints.rsdependi-lsp/src/registries/cargo_sparse.rsdependi-lsp/src/registries/crates_io.rsdependi-lsp/src/registries/go_proxy.rsdependi-lsp/src/registries/npm.rsdependi-lsp/src/registries/nuget.rsdependi-lsp/src/registries/packagist.rsdependi-lsp/src/registries/pub_dev.rsdependi-lsp/src/registries/pypi.rsdependi-lsp/src/registries/rubygems.rsdependi-lsp/src/reports.rsdependi-lsp/src/utils.rsdependi-lsp/src/vulnerabilities/cache.rsdependi-lsp/src/vulnerabilities/osv.rsdependi-lsp/tests/fuzz_regression.rsdependi-zed/.clippy.tomldependi-zed/Cargo.tomldependi-zed/src/lib.rs
Comment on lines
176
to
179
| let repository = | ||
| if module_path.starts_with("github.com/") || module_path.starts_with("gitlab.com/") { | ||
| Some(format!("https://{}", module_path)) | ||
| Some(format!("https://{module_path}")) | ||
| } else { |
Contributor
There was a problem hiding this comment.
Normalize Go repository URLs to the VCS root.
module_path is not always the repository root. For modules like github.com/org/repo/submodule or github.com/org/repo/v2, https://{module_path} points at the wrong GitHub/GitLab URL, so VersionInfo.repository can become a broken link.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/registries/go_proxy.rs` around lines 176 - 179, The code
currently builds repository URLs using the full module_path which can include
submodules or a vN suffix; update the logic in
dependi-lsp/src/registries/go_proxy.rs where repository is computed from
module_path so that for hosts like "github.com" and "gitlab.com" you normalize
to the VCS root by splitting module_path on '/' and using only the first
up-to-three segments (i.e., domain/org/repo), falling back to the whole path if
it has fewer than three segments; then set repository to
Some(format!("https://{root}")) instead of using the entire module_path so links
point to the actual repo root (handle cases like "github.com/org/repo/submodule"
and "github.com/org/repo/v2").
Contributor
Author
There was a problem hiding this comment.
@mpiton this should probably be addressed in another PR, as not related to this one
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
DanikVitek
force-pushed
the
feat/bump-msrv
branch
from
eca1a41 to
c1b6182
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dependi-lsp/src/providers/completion.rs (1)
121-143: Consider optimizing empty documentation check.The current pattern allocates a
Stringwith.to_string()before checking if it's empty. For cases where the documentation is often empty (no release date and not the latest version), this creates unnecessary allocations.A potential optimization:
let has_content = is_latest || release_date.is_some(); let documentation = if has_content { Some(Documentation::MarkupContent(MarkupContent { kind: MarkupKind::Markdown, value: fmt::from_fn(|f| { /* ... */ }).to_string(), })) } else { None };This is a minor optimization and may not be worth the added complexity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/providers/completion.rs` around lines 121 - 143, The current block builds a String via fmt::from_fn(...).to_string() unconditionally and then checks .is_empty(), causing unnecessary allocation when there is no content; change the logic around the documentation variable to first test whether content will exist (e.g. let has_content = is_latest || release_date.is_some()), and only call fmt::from_fn and allocate the String when has_content is true; keep the same formatting logic (using fmt::from_fn, is_latest, release_date, fmt_release_age) and return Some(Documentation::MarkupContent(MarkupContent { kind: MarkupKind::Markdown, value: <generated string> })) or None accordingly.dependi-lsp/src/reports.rs (1)
114-118: Consider simplifying the string comparison.The comparison
**package != *current_packageworks but is harder to read. Sincepackage: &Stringandcurrent_package: String, a clearer approach would be:- if **package != *current_package || **version != *current_version { + if *package != current_package || *version != current_version {This compares
StringtoStringdirectly, which is more idiomatic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/reports.rs` around lines 114 - 118, The double-deref comparisons are hard to read; instead compare the borrowed String to the owned String directly (e.g., use package != ¤t_package and version != ¤t_version or package.as_str() != current_package.as_str()) in the conditional that updates current_package/current_version and writes the header; keep the existing assignments current_package = package.clone() and current_version = version.clone() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dependi-lsp/src/providers/completion.rs`:
- Around line 121-143: The current block builds a String via
fmt::from_fn(...).to_string() unconditionally and then checks .is_empty(),
causing unnecessary allocation when there is no content; change the logic around
the documentation variable to first test whether content will exist (e.g. let
has_content = is_latest || release_date.is_some()), and only call fmt::from_fn
and allocate the String when has_content is true; keep the same formatting logic
(using fmt::from_fn, is_latest, release_date, fmt_release_age) and return
Some(Documentation::MarkupContent(MarkupContent { kind: MarkupKind::Markdown,
value: <generated string> })) or None accordingly.
In `@dependi-lsp/src/reports.rs`:
- Around line 114-118: The double-deref comparisons are hard to read; instead
compare the borrowed String to the owned String directly (e.g., use package !=
¤t_package and version != ¤t_version or package.as_str() !=
current_package.as_str()) in the conditional that updates
current_package/current_version and writes the header; keep the existing
assignments current_package = package.clone() and current_version =
version.clone() unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 569ae64c-4c7c-4784-98d1-c91cb58971be
📒 Files selected for processing (39)
.gitignoredependi-lsp/.clippy.tomldependi-lsp/Cargo.tomldependi-lsp/benches/benchmarks.rsdependi-lsp/src/auth/mod.rsdependi-lsp/src/backend.rsdependi-lsp/src/bin/test_dates.rsdependi-lsp/src/cache/mod.rsdependi-lsp/src/cache/sqlite.rsdependi-lsp/src/file_types.rsdependi-lsp/src/main.rsdependi-lsp/src/parsers/cargo.rsdependi-lsp/src/parsers/csharp.rsdependi-lsp/src/parsers/npm.rsdependi-lsp/src/parsers/php.rsdependi-lsp/src/parsers/python.rsdependi-lsp/src/parsers/ruby.rsdependi-lsp/src/providers/code_actions.rsdependi-lsp/src/providers/completion.rsdependi-lsp/src/providers/diagnostics.rsdependi-lsp/src/providers/document_links.rsdependi-lsp/src/providers/inlay_hints.rsdependi-lsp/src/registries/cargo_sparse.rsdependi-lsp/src/registries/crates_io.rsdependi-lsp/src/registries/go_proxy.rsdependi-lsp/src/registries/npm.rsdependi-lsp/src/registries/nuget.rsdependi-lsp/src/registries/packagist.rsdependi-lsp/src/registries/pub_dev.rsdependi-lsp/src/registries/pypi.rsdependi-lsp/src/registries/rubygems.rsdependi-lsp/src/reports.rsdependi-lsp/src/utils.rsdependi-lsp/src/vulnerabilities/cache.rsdependi-lsp/src/vulnerabilities/osv.rsdependi-lsp/tests/fuzz_regression.rsdependi-zed/.clippy.tomldependi-zed/Cargo.tomldependi-zed/src/lib.rs
✅ Files skipped from review due to trivial changes (22)
- .gitignore
- dependi-lsp/.clippy.toml
- dependi-lsp/src/cache/mod.rs
- dependi-zed/.clippy.toml
- dependi-lsp/Cargo.toml
- dependi-lsp/src/parsers/csharp.rs
- dependi-lsp/src/parsers/php.rs
- dependi-lsp/src/parsers/cargo.rs
- dependi-zed/Cargo.toml
- dependi-lsp/src/vulnerabilities/cache.rs
- dependi-lsp/src/vulnerabilities/osv.rs
- dependi-lsp/src/bin/test_dates.rs
- dependi-lsp/src/registries/nuget.rs
- dependi-lsp/src/parsers/npm.rs
- dependi-lsp/src/parsers/ruby.rs
- dependi-lsp/tests/fuzz_regression.rs
- dependi-lsp/src/parsers/python.rs
- dependi-zed/src/lib.rs
- dependi-lsp/benches/benchmarks.rs
- dependi-lsp/src/cache/sqlite.rs
- dependi-lsp/src/providers/code_actions.rs
- dependi-lsp/src/registries/packagist.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- dependi-lsp/src/registries/pypi.rs
- dependi-lsp/src/registries/pub_dev.rs
- dependi-lsp/src/providers/document_links.rs
- dependi-lsp/src/registries/npm.rs
- dependi-lsp/src/utils.rs
- dependi-lsp/src/file_types.rs
- dependi-lsp/src/auth/mod.rs
- dependi-lsp/src/registries/cargo_sparse.rs
- dependi-lsp/src/main.rs
DanikVitek
force-pushed
the
feat/bump-msrv
branch
from
c1b6182 to
b31cf39
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dependi-lsp/src/providers/diagnostics.rs (1)
386-386: Optional: de-duplicate repeated cache-key closure in tests.The repeated
|name| format!("test:{name}")is consistent, but extracting a tiny local helper in the test module would reduce repetition and future edit noise.Also applies to: 405-405, 414-414, 453-453, 490-490, 526-526, 575-575, 608-608, 640-640, 695-695, 736-736, 769-769, 815-815, 846-846, 877-877, 912-912, 945-945, 978-978
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/providers/diagnostics.rs` at line 386, Multiple tests repeat the same cache-key closure `|name| format!("test:{name}")` when calling `create_diagnostics`; extract a small helper in the test module (e.g., `fn test_cache_key(name: &str) -> String`) and replace each inline closure (references at calls to `create_diagnostics`) with that helper to reduce duplication and future edit noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dependi-lsp/src/providers/diagnostics.rs`:
- Line 386: Multiple tests repeat the same cache-key closure `|name|
format!("test:{name}")` when calling `create_diagnostics`; extract a small
helper in the test module (e.g., `fn test_cache_key(name: &str) -> String`) and
replace each inline closure (references at calls to `create_diagnostics`) with
that helper to reduce duplication and future edit noise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f90e26fb-00cf-4b1e-aa7c-d364e3279e0b
📒 Files selected for processing (39)
.gitignoredependi-lsp/.clippy.tomldependi-lsp/Cargo.tomldependi-lsp/benches/benchmarks.rsdependi-lsp/src/auth/mod.rsdependi-lsp/src/backend.rsdependi-lsp/src/bin/test_dates.rsdependi-lsp/src/cache/mod.rsdependi-lsp/src/cache/sqlite.rsdependi-lsp/src/file_types.rsdependi-lsp/src/main.rsdependi-lsp/src/parsers/cargo.rsdependi-lsp/src/parsers/csharp.rsdependi-lsp/src/parsers/npm.rsdependi-lsp/src/parsers/php.rsdependi-lsp/src/parsers/python.rsdependi-lsp/src/parsers/ruby.rsdependi-lsp/src/providers/code_actions.rsdependi-lsp/src/providers/completion.rsdependi-lsp/src/providers/diagnostics.rsdependi-lsp/src/providers/document_links.rsdependi-lsp/src/providers/inlay_hints.rsdependi-lsp/src/registries/cargo_sparse.rsdependi-lsp/src/registries/crates_io.rsdependi-lsp/src/registries/go_proxy.rsdependi-lsp/src/registries/npm.rsdependi-lsp/src/registries/nuget.rsdependi-lsp/src/registries/packagist.rsdependi-lsp/src/registries/pub_dev.rsdependi-lsp/src/registries/pypi.rsdependi-lsp/src/registries/rubygems.rsdependi-lsp/src/reports.rsdependi-lsp/src/utils.rsdependi-lsp/src/vulnerabilities/cache.rsdependi-lsp/src/vulnerabilities/osv.rsdependi-lsp/tests/fuzz_regression.rsdependi-zed/.clippy.tomldependi-zed/Cargo.tomldependi-zed/src/lib.rs
✅ Files skipped from review due to trivial changes (17)
- .gitignore
- dependi-lsp/Cargo.toml
- dependi-lsp/.clippy.toml
- dependi-zed/.clippy.toml
- dependi-lsp/src/parsers/csharp.rs
- dependi-lsp/src/cache/mod.rs
- dependi-lsp/src/parsers/cargo.rs
- dependi-lsp/src/bin/test_dates.rs
- dependi-lsp/src/parsers/npm.rs
- dependi-lsp/src/parsers/php.rs
- dependi-lsp/src/parsers/ruby.rs
- dependi-lsp/src/registries/packagist.rs
- dependi-lsp/src/vulnerabilities/cache.rs
- dependi-lsp/src/parsers/python.rs
- dependi-lsp/src/main.rs
- dependi-zed/src/lib.rs
- dependi-lsp/src/providers/inlay_hints.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- dependi-lsp/src/registries/nuget.rs
- dependi-lsp/src/vulnerabilities/osv.rs
- dependi-lsp/tests/fuzz_regression.rs
- dependi-lsp/src/registries/crates_io.rs
- dependi-lsp/src/registries/rubygems.rs
- dependi-lsp/src/registries/go_proxy.rs
- dependi-lsp/src/providers/code_actions.rs
- dependi-lsp/src/utils.rs
- dependi-lsp/benches/benchmarks.rs
- dependi-lsp/src/registries/npm.rs
- dependi-lsp/src/providers/document_links.rs
DanikVitek
force-pushed
the
feat/bump-msrv
branch
from
b31cf39 to
a749dec
Compare
mpiton
requested changes
Mar 23, 2026
Owner
There was a problem hiding this comment.
Thanks for this thorough modernization work @DanikVitek! The fmt::from_fn pattern, let-chains, and inlined format args are all welcome improvements. I ran a full review (Rust-specific + general code quality) and the codebase compiles cleanly with cargo +1.94.0 check/clippy/test. All 450 unit tests and 7 integration tests pass.
There are a few items to address before we can merge — mostly around API contracts and project conventions. Nothing major.
Must fix before merge
1. Missing CHANGELOG.md entry
### Changed
- Bump MSRV from 1.85 to 1.94; adopt stable let-chains, `fmt::from_fn` for
zero-allocation display formatting, and inlined format args across the codebase
### Fixed
- Use `env::var_os` instead of `env::var` for `CARGO_HOME` to avoid failures on non-UTF-8 paths(The env::var_os change is a genuine correctness fix, not just refactoring — worth calling out separately!)
2. Deprecated functions — consider removing instead
All production call sites for the deprecated functions (redact_token, format_release_age, truncate_string, registry_package_url, generate_markdown_report) have been migrated to their fmt_* equivalents in this same PR. The #[cfg_attr(not(test), deprecated)] pattern works but is non-standard and makes the deprecation invisible in #[cfg(test)] contexts.
Since no production code calls them anymore, I'd suggest one of:
- Preferred: Remove the old functions entirely. Update any tests that still call them to use the new
fmt_*versions. This is the cleanest option. - Alternative: If you want to keep them for backward compatibility, use plain
#[deprecated]and add#[allow(deprecated)]at the specific test call sites.
3. fmt_registry_package_url — document the exhaustiveness guarantee
The old registry_package_url returned Option<String> (always Some in practice, but the type signaled "might not apply"). The new fmt_registry_package_url is infallible. This is fine since all FileType variants are covered by the exhaustive match, but the callers in document_links.rs and the hover handler in backend.rs lost their Option-based guard.
Could you add a brief doc comment noting that this function is guaranteed to produce a valid URL for all FileType variants? That way, when someone adds a new ecosystem in the future, they'll know to update this function. Example:
/// Returns a registry URL for the given package name.
///
/// This is exhaustive over all `FileType` variants — if a new variant is added,
/// update the match arm here.
Suggestions (non-blocking)
Doc typos
auth/mod.rs:"implemetation"→"implementation"in thefmt_redact_tokendoc commentutils.rs:"implemantation"→"implementation"in thefmt_truncate_stringdoc comment#[must_use]messages:"which do not have any effects"→"which does not have any effects"
Remove + fmt::Debug from tooltip return types
fmt_vulnerability_tooltip, fmt_deprecation_tooltip, and fmt_yanked_tooltip return impl fmt::Display + fmt::Debug. The Debug impl from fmt::from_fn just prints the closure address, which isn't useful. The other fmt_* functions in this PR return only impl fmt::Display. For consistency, I'd drop the + fmt::Debug bound.
#[rustfmt::skip] in normalize_version
The #[rustfmt::skip] on the strip_prefix chain alignment is understandable but we generally avoid overriding rustfmt in production code. Would you be open to removing it and letting rustfmt handle the layout?
What I liked
- The
env::var_oschange forCARGO_HOMEis a real improvement — avoidsUtf8Erroron non-UTF-8 paths (possible on Linux/macOS with locale issues). - Consistent use of
anyhow::ensure!across registries is much cleaner than manualif !status { bail!() }. - The
plural_suffixhelper insidefmt_release_ageusing nestedfmt::from_fnis clever and avoids the repeated ternary pattern. - Let-chains in the lockfile resolution blocks (
backend.rs) dramatically reduce nesting — much more readable.
Thanks again for taking this on! Happy to help if you have questions about any of the above.
Contributor
Author
|
@mpiton Thanks for the review. Only, I'd like to point out that the |
Owner
|
@DanikVitek Good catch on Let's go with adding And agreed on the Go proxy URL normalization — that's a separate issue, not related to this PR. For the other three items (CHANGELOG entry, deprecated functions cleanup, |
Contributor
Author
@mpiton This message seems redundant as the compiler will ensure the |
Contributor
Author
|
Also, should I maybe re-implement some of the formatting functions to return a non-opaque type that has a custom |
Owner
|
@DanikVitek Fair point on the exhaustive On the non-opaque types with custom So the only remaining items before merge are:
Let me know when you push the updates and I'll re-review! |
DanikVitek
force-pushed
the
feat/bump-msrv
branch
from
a749dec to
b880c2a
Compare
DanikVitek
force-pushed
the
feat/bump-msrv
branch
from
b880c2a to
3f41f05
Compare
Contributor
Author
|
Ready for review |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dependi-lsp/src/utils.rs (1)
5-32:⚠️ Potential issue | 🟡 Minor
max_charsis still violated for widths below 3.The docstring says the ellipsis counts toward the maximum, but this implementation always emits
"..."once it decides to truncate. That meansfmt_truncate_string(_, 0..=2)returns a string longer thanmax_chars, which can still break fixed-width UI assumptions. Either clamp the ellipsis to the requested width or document the minimum-width behavior explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/utils.rs` around lines 5 - 32, The truncation currently always appends "..." which violates the max_chars contract for max_chars < 3; update fmt_truncate_string to clamp the ellipsis to the requested width and adjust the kept characters accordingly: compute an ellipsis string of length min(3, max_chars) ("" for 0, "." for 1, ".." for 2, "..." for >=3), set keep_chars = max_chars.saturating_sub(ellipsis.len()) instead of subtracting 3 unconditionally, then emit up to keep_chars chars from s followed by that ellipsis (writing nothing if max_chars == 0); update the logic around keep_chars and the write of "..." in fmt_truncate_string to use these values.dependi-lsp/src/providers/diagnostics.rs (1)
217-235:⚠️ Potential issue | 🟠 MajorDon't hardcode crates.io in yanked diagnostics.
CargoSparseRegistrycan now report yanked versions for alternative registries too, but this helper still says the package was yanked from crates.io and links users to crates.io unconditionally. That gives the wrong remediation path for private or third-party Cargo registries. Thread the actual registry/package URL into this diagnostic instead of assuming crates.io.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/providers/diagnostics.rs` around lines 217 - 235, The diagnostic currently hardcodes crates.io when building the message and related_info URL; instead, derive the registry base URL from the package/registry metadata (e.g., use a registry_url or source field available on version_info or dep) and fall back to "https://crates.io" only if none is present; update the message text (built in fmt::from_fn) to mention the actual registry host rather than "crates.io", and replace crates_io_url with a computed registry_package_url (formatting the registry base + dep.name or the registry's package path) and parse that into Url for DiagnosticRelatedInformation; keep the existing Range::default() fallback behavior.
🧹 Nitpick comments (3)
dependi-lsp/tests/fuzz_regression.rs (1)
12-18: Use cacheddep_lineconsistently for indexing.After introducing
dep_line, Line 18 still indexes withdep.line. Prefer a single source of truth for readability and future maintenance.Suggested tiny cleanup
- let line = lines[dep.line as usize]; + let line = lines[dep_line as usize];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/tests/fuzz_regression.rs` around lines 12 - 18, Replace the direct use of dep.line when indexing into lines with the cached dep_line variable for consistency and clarity: after computing let dep_line = dep.line and asserting bounds using lines_len and parser_name, use dep_line as the usize index (i.e., lines[dep_line as usize]) instead of lines[dep.line as usize] so all accesses/readability rely on the single cached symbol dep_line.dependi-lsp/src/reports.rs (1)
88-101: You can keep the grouping state borrowed.This loop still clones
packageandversionon each group transition. AnOption<(&str, &str)>keeps the formatter allocation-free here as well.♻️ Suggested simplification
- let mut current_package = String::new(); - let mut current_version = String::new(); + let mut current: Option<(&str, &str)> = None; for VulnerabilityReportEntry { package, version, id, severity, description, url, } in vulnerabilities { - if *package != current_package || *version != current_version { - current_package = package.clone(); - current_version = version.clone(); + let key = (package.as_str(), version.as_str()); + if current != Some(key) { + current = Some(key); writeln!(f, "### {package}@{version}\n")?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/reports.rs` around lines 88 - 101, The loop currently clones package and version into String variables (current_package/current_version) on each group transition; instead change the grouping state to an Option<(&str,&str)> (e.g., current_group: Option<(&str,&str)>) and when you detect a new group assign current_group = Some((package.as_str(), version.as_str())) rather than cloning; update comparisons to compare against the borrowed tuple (e.g., if current_group != Some((package.as_str(), version.as_str())) or pattern-match the Option) and use the borrowed values where needed so VulnerabilityReportEntry fields are kept borrowed and no allocation occurs during grouping transitions.dependi-lsp/src/providers/inlay_hints.rs (1)
47-47: Letpadding_leftown the spacing.
padding_left: Some(true)already asks the client to insert the separator before the hint. Keeping a literal leading space in the label can render a double gap in clients that honor the flag. Please sanity-check this in Zed, since manual testing is still pending.🔧 Minimal fix
- label: InlayHintLabel::String(format!(" {label}")), + label: InlayHintLabel::String(label),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/providers/inlay_hints.rs` at line 47, The label currently includes a leading space via InlayHintLabel::String(format!(" {label}")) which duplicates spacing when padding_left: Some(true) is set; remove the manual leading space so the label uses format!("{}", label) (or just label.to_string()) and rely on the existing padding_left flag to insert the separator, and then verify the hint renders correctly in clients like Zed that honor padding_left.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dependi-lsp/src/backend.rs`:
- Around line 1365-1366: The hover shows the manifest spec because the code
unconditionally uses dep.version for display and release-date lookup; change the
dep_version binding to prefer dep.resolved_version when present (fall back to
dep.version otherwise) so both the printed "Current:" value and the release-date
lookup use the resolved version. Update the variables around
dep_name/dep_version (the let dep_name = &*dep.name; let dep_version =
&*dep.version; lines) so dep_version is set from
dep.resolved_version.as_ref().map(|s| &**s).unwrap_or(&*dep.version) (or
equivalent) and ensure the same logic is applied in the other occurrence
referenced (around lines 1385-1390).
In `@dependi-lsp/src/providers/completion.rs`:
- Around line 34-49: The code currently checks days < 0 which misses future
times shorter than a day; instead check the raw signed duration (e.g., if
duration < chrono::Duration::zero() or duration.num_seconds() < 0) before
deriving days/hours/minutes so we never format negative units; locate the block
using the duration variable and calls to
duration.num_days()/num_hours()/num_minutes() and update the guard to reject any
negative duration, then add a regression test that constructs now + 1h (or
+3600s) and asserts the formatted output is "just now" (or the expected
future-handling string).
In `@dependi-lsp/src/providers/inlay_hints.rs`:
- Around line 64-68: The tooltip/build string is using the declared spec
(dep.version / dep_version) while status is computed from
dep.effective_version(), causing mismatched displays; replace uses of
dep.version/dep_version in the update tooltip and the other occurrences
referenced (around where dep_name, dep_version, status, and effective_version
are computed) to use dep.effective_version() (store it in a local
effective_version variable if not already) so the displayed "from -> to" text
compares the actual effective version (e.g., 3.9.1) to the new version rather
than the declared range/lock spec; update all similar interpolations (the other
locations you noted) to use that effective_version variable instead of
dep.version.
In `@dependi-lsp/src/registries/packagist.rs`:
- Around line 151-155: The current check in the Packagist registry path (where
package_name is used to build the p2 URL) only uses contains('/') and allows
malformed names; update the validation to require exactly one '/' and two
non-empty segments before constructing url (e.g., split package_name on '/' and
ensure the resulting iterator yields exactly two non-empty parts), and bail with
an error if this check fails so only valid vendor/package names reach the p2 URL
construction.
---
Outside diff comments:
In `@dependi-lsp/src/providers/diagnostics.rs`:
- Around line 217-235: The diagnostic currently hardcodes crates.io when
building the message and related_info URL; instead, derive the registry base URL
from the package/registry metadata (e.g., use a registry_url or source field
available on version_info or dep) and fall back to "https://crates.io" only if
none is present; update the message text (built in fmt::from_fn) to mention the
actual registry host rather than "crates.io", and replace crates_io_url with a
computed registry_package_url (formatting the registry base + dep.name or the
registry's package path) and parse that into Url for
DiagnosticRelatedInformation; keep the existing Range::default() fallback
behavior.
In `@dependi-lsp/src/utils.rs`:
- Around line 5-32: The truncation currently always appends "..." which violates
the max_chars contract for max_chars < 3; update fmt_truncate_string to clamp
the ellipsis to the requested width and adjust the kept characters accordingly:
compute an ellipsis string of length min(3, max_chars) ("" for 0, "." for 1,
".." for 2, "..." for >=3), set keep_chars =
max_chars.saturating_sub(ellipsis.len()) instead of subtracting 3
unconditionally, then emit up to keep_chars chars from s followed by that
ellipsis (writing nothing if max_chars == 0); update the logic around keep_chars
and the write of "..." in fmt_truncate_string to use these values.
---
Nitpick comments:
In `@dependi-lsp/src/providers/inlay_hints.rs`:
- Line 47: The label currently includes a leading space via
InlayHintLabel::String(format!(" {label}")) which duplicates spacing when
padding_left: Some(true) is set; remove the manual leading space so the label
uses format!("{}", label) (or just label.to_string()) and rely on the existing
padding_left flag to insert the separator, and then verify the hint renders
correctly in clients like Zed that honor padding_left.
In `@dependi-lsp/src/reports.rs`:
- Around line 88-101: The loop currently clones package and version into String
variables (current_package/current_version) on each group transition; instead
change the grouping state to an Option<(&str,&str)> (e.g., current_group:
Option<(&str,&str)>) and when you detect a new group assign current_group =
Some((package.as_str(), version.as_str())) rather than cloning; update
comparisons to compare against the borrowed tuple (e.g., if current_group !=
Some((package.as_str(), version.as_str())) or pattern-match the Option) and use
the borrowed values where needed so VulnerabilityReportEntry fields are kept
borrowed and no allocation occurs during grouping transitions.
In `@dependi-lsp/tests/fuzz_regression.rs`:
- Around line 12-18: Replace the direct use of dep.line when indexing into lines
with the cached dep_line variable for consistency and clarity: after computing
let dep_line = dep.line and asserting bounds using lines_len and parser_name,
use dep_line as the usize index (i.e., lines[dep_line as usize]) instead of
lines[dep.line as usize] so all accesses/readability rely on the single cached
symbol dep_line.
🪄 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
Run ID: d18cbb8d-ca8a-498e-881d-bc43a2091f6e
📒 Files selected for processing (43)
.gitignoreCHANGELOG.mdCONTRIBUTING.mddependi-lsp/.clippy.tomldependi-lsp/Cargo.tomldependi-lsp/benches/benchmarks.rsdependi-lsp/src/auth/mod.rsdependi-lsp/src/backend.rsdependi-lsp/src/bin/test_dates.rsdependi-lsp/src/cache/mod.rsdependi-lsp/src/cache/sqlite.rsdependi-lsp/src/file_types.rsdependi-lsp/src/main.rsdependi-lsp/src/parsers/cargo.rsdependi-lsp/src/parsers/csharp.rsdependi-lsp/src/parsers/npm.rsdependi-lsp/src/parsers/npm_lock.rsdependi-lsp/src/parsers/php.rsdependi-lsp/src/parsers/pubspec_lock.rsdependi-lsp/src/parsers/python.rsdependi-lsp/src/parsers/ruby.rsdependi-lsp/src/providers/code_actions.rsdependi-lsp/src/providers/completion.rsdependi-lsp/src/providers/diagnostics.rsdependi-lsp/src/providers/document_links.rsdependi-lsp/src/providers/inlay_hints.rsdependi-lsp/src/registries/cargo_sparse.rsdependi-lsp/src/registries/crates_io.rsdependi-lsp/src/registries/go_proxy.rsdependi-lsp/src/registries/npm.rsdependi-lsp/src/registries/nuget.rsdependi-lsp/src/registries/packagist.rsdependi-lsp/src/registries/pub_dev.rsdependi-lsp/src/registries/pypi.rsdependi-lsp/src/registries/rubygems.rsdependi-lsp/src/reports.rsdependi-lsp/src/utils.rsdependi-lsp/src/vulnerabilities/cache.rsdependi-lsp/src/vulnerabilities/osv.rsdependi-lsp/tests/fuzz_regression.rsdependi-zed/.clippy.tomldependi-zed/Cargo.tomldependi-zed/src/lib.rs
✅ Files skipped from review due to trivial changes (23)
- dependi-lsp/.clippy.toml
- dependi-lsp/src/cache/mod.rs
- CONTRIBUTING.md
- dependi-zed/.clippy.toml
- dependi-lsp/Cargo.toml
- CHANGELOG.md
- dependi-lsp/src/parsers/npm.rs
- dependi-lsp/src/bin/test_dates.rs
- .gitignore
- dependi-zed/Cargo.toml
- dependi-lsp/src/vulnerabilities/cache.rs
- dependi-lsp/src/parsers/php.rs
- dependi-lsp/src/parsers/ruby.rs
- dependi-lsp/src/vulnerabilities/osv.rs
- dependi-lsp/src/parsers/cargo.rs
- dependi-lsp/src/registries/pub_dev.rs
- dependi-lsp/src/registries/npm.rs
- dependi-lsp/src/main.rs
- dependi-lsp/src/cache/sqlite.rs
- dependi-lsp/src/parsers/npm_lock.rs
- dependi-lsp/benches/benchmarks.rs
- dependi-zed/src/lib.rs
- dependi-lsp/src/parsers/python.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- dependi-lsp/src/providers/document_links.rs
- dependi-lsp/src/registries/rubygems.rs
- dependi-lsp/src/registries/crates_io.rs
- dependi-lsp/src/providers/code_actions.rs
- dependi-lsp/src/registries/go_proxy.rs
- dependi-lsp/src/auth/mod.rs
Comment on lines
+1365
to
+1366
| let dep_name = &*dep.name; | ||
| let dep_version = &*dep.version; |
Contributor
There was a problem hiding this comment.
Hover still shows the manifest spec instead of the resolved version.
This formatter prints dep.version as Current: and uses it for the release-date lookup, so any dependency with resolved_version can get a hover that disagrees with the inlay hint/diagnostic status and shows the wrong age.
🐛 Suggested fix
- let dep_name = &*dep.name;
- let dep_version = &*dep.version;
+ let dep_name = dep.name.as_str();
+ let dep_version = dep.effective_version();
...
let current_date_str = info
- .get_release_date(&dep.version)
+ .get_release_date(dep_version)
.map(|dt| format!(" ({})", fmt_release_age(dt)))
.unwrap_or_default();
writeln!(f, "**Current:** {dep_version}{current_date_str}")?;Also applies to: 1385-1390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/backend.rs` around lines 1365 - 1366, The hover shows the
manifest spec because the code unconditionally uses dep.version for display and
release-date lookup; change the dep_version binding to prefer
dep.resolved_version when present (fall back to dep.version otherwise) so both
the printed "Current:" value and the release-date lookup use the resolved
version. Update the variables around dep_name/dep_version (the let dep_name =
&*dep.name; let dep_version = &*dep.version; lines) so dep_version is set from
dep.resolved_version.as_ref().map(|s| &**s).unwrap_or(&*dep.version) (or
equivalent) and ensure the same logic is applied in the other occurrence
referenced (around lines 1385-1390).
Comment on lines
+34
to
49
| let days = duration.num_days(); | ||
| if days < 0 { | ||
| return f.write_str("just now"); | ||
| } | ||
|
|
||
| if days == 0 { | ||
| let hours = duration.num_hours(); | ||
| if hours == 0 { | ||
| let mins = duration.num_minutes(); | ||
| if mins < 1 { | ||
| return f.write_str("just now"); | ||
| } | ||
| return write!(f, "{mins} min{s} ago", s = plural_suffix(mins)); | ||
| } | ||
| return format!("{} min{} ago", mins, if mins == 1 { "" } else { "s" }); | ||
| return write!(f, "{hours} hour{s} ago", s = plural_suffix(hours)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For chrono::Duration/TimeDelta, what do num_days()andnum_hours()return for a negative duration such as-5 hours?
💡 Result:
For a negative chrono::Duration or TimeDelta such as -5 hours (equivalent to TimeDelta::hours(-5)), the methods num_days and num_hours return negative values: -5 for num_hours and 0 for num_days.
Citations:
- 1: https://docs.rs/chrono/latest/chrono/struct.TimeDelta.html
- 2: https://docs.rs/chrono/latest/src/chrono/time_delta.rs.html
Future release dates can still render negative ages.
The future-date guard only checks days < 0. For a timestamp like now + 5h, chrono::Duration::num_days() returns 0 while num_hours() returns -5, causing the branch to format -5 hours ago. Guard on the raw signed duration before deriving units.
Suggested fix
- let days = duration.num_days();
- if days < 0 {
+ if duration.num_seconds() <= 0 {
return f.write_str("just now");
}
+ let days = duration.num_days();Add a regression test with a +1h future timestamp.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let days = duration.num_days(); | |
| if days < 0 { | |
| return f.write_str("just now"); | |
| } | |
| if days == 0 { | |
| let hours = duration.num_hours(); | |
| if hours == 0 { | |
| let mins = duration.num_minutes(); | |
| if mins < 1 { | |
| return f.write_str("just now"); | |
| } | |
| return write!(f, "{mins} min{s} ago", s = plural_suffix(mins)); | |
| } | |
| return format!("{} min{} ago", mins, if mins == 1 { "" } else { "s" }); | |
| return write!(f, "{hours} hour{s} ago", s = plural_suffix(hours)); | |
| } | |
| if duration.num_seconds() <= 0 { | |
| return f.write_str("just now"); | |
| } | |
| let days = duration.num_days(); | |
| if days == 0 { | |
| let hours = duration.num_hours(); | |
| if hours == 0 { | |
| let mins = duration.num_minutes(); | |
| if mins < 1 { | |
| return f.write_str("just now"); | |
| } | |
| return write!(f, "{mins} min{s} ago", s = plural_suffix(mins)); | |
| } | |
| return write!(f, "{hours} hour{s} ago", s = plural_suffix(hours)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/providers/completion.rs` around lines 34 - 49, The code
currently checks days < 0 which misses future times shorter than a day; instead
check the raw signed duration (e.g., if duration < chrono::Duration::zero() or
duration.num_seconds() < 0) before deriving days/hours/minutes so we never
format negative units; locate the block using the duration variable and calls to
duration.num_days()/num_hours()/num_minutes() and update the guard to reject any
negative duration, then add a regression test that constructs now + 1h (or
+3600s) and asserts the formatted output is "just now" (or the expected
future-handling string).
Comment on lines
+64
to
+68
| let dep_name = &*dep.name; | ||
| let dep_version = &*dep.version; | ||
|
|
||
| // Handle local dependencies first (highest priority - no registry lookup needed) | ||
| if is_local_dependency(&dep.version) { | ||
| if is_local_dependency(dep_version) { |
Contributor
There was a problem hiding this comment.
Use the effective version in update tooltips.
status is computed from dep.effective_version(), but the tooltip text now interpolates the declared spec from dep.version. With lockfile-backed deps or range syntax, this produces misleading strings like ^3.9 -> 3.10.0 even though the compared current version was 3.9.1.
🐛 Suggested fix
- let dep_name = &*dep.name;
- let dep_version = &*dep.version;
+ let dep_name = dep.name.as_str();
+ let declared_version = dep.version.as_str();
+ let effective_version = dep.effective_version();
- if is_local_dependency(dep_version) {
+ if is_local_dependency(declared_version) {
let tooltip = format!(
"**Local Dependency**\n\n\
\"{dep_name}\" is a local/path dependency.\n\n\
Version info is not available for local packages.\n\n\
This is expected for dependencies using:\n\
• path = \"./...\"\n\
• git = \"https://...\"\n\
• git = \"git@...\"\n\
• github:owner/repo",
);
return ("→ Local".to_string(), Some(tooltip));
}
- tracing::debug!("Not a local dependency: {dep_name} with version '{dep_version}'",);
+ tracing::debug!("Not a local dependency: {dep_name} with version '{declared_version}'");
...
- "{yanked_tooltip}\n\n---\n**Update available:** {dep_version} -> {latest}",
+ "{yanked_tooltip}\n\n---\n**Update available:** {effective_version} -> {latest}",
...
- "{dep_tooltip}\n\n---\n**Update available:** {dep_version} -> {latest}",
+ "{dep_tooltip}\n\n---\n**Update available:** {effective_version} -> {latest}",
...
- "{vuln_tooltip}\n\n---\n**Update available:** {dep_version} -> {latest}",
+ "{vuln_tooltip}\n\n---\n**Update available:** {effective_version} -> {latest}",
...
- let tooltip = format!("Update available: {dep_version} -> {latest}");
+ let tooltip = format!("Update available: {effective_version} -> {latest}");Also applies to: 82-82, 95-96, 115-116, 142-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/providers/inlay_hints.rs` around lines 64 - 68, The
tooltip/build string is using the declared spec (dep.version / dep_version)
while status is computed from dep.effective_version(), causing mismatched
displays; replace uses of dep.version/dep_version in the update tooltip and the
other occurrences referenced (around where dep_name, dep_version, status, and
effective_version are computed) to use dep.effective_version() (store it in a
local effective_version variable if not already) so the displayed "from -> to"
text compares the actual effective version (e.g., 3.9.1) to the new version
rather than the declared range/lock spec; update all similar interpolations (the
other locations you noted) to use that effective_version variable instead of
dep.version.
Comment on lines
151
to
+155
| if !package_name.contains('/') { | ||
| anyhow::bail!( | ||
| "Invalid package name: {} (expected vendor/package)", | ||
| package_name | ||
| ); | ||
| anyhow::bail!("Invalid package name: {package_name} (expected vendor/package)"); | ||
| } | ||
|
|
||
| let url = format!("{}/p2/{}.json", self.base_url, package_name); | ||
| let url = format!("{}/p2/{package_name}.json", self.base_url); |
Contributor
There was a problem hiding this comment.
Reject malformed Packagist names before building the p2 URL.
contains('/') still lets vendor/, /package, and vendor/package/extra through, so this path interpolation turns bad input into misleading HTTP lookups instead of a local validation error. Validate exactly one slash and non-empty segments here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/registries/packagist.rs` around lines 151 - 155, The current
check in the Packagist registry path (where package_name is used to build the p2
URL) only uses contains('/') and allows malformed names; update the validation
to require exactly one '/' and two non-empty segments before constructing url
(e.g., split package_name on '/' and ensure the resulting iterator yields
exactly two non-empty parts), and bail with an error if this check fails so only
valid vendor/package names reach the p2 URL construction.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
dependi-lsp/src/providers/completion.rs (1)
34-49:⚠️ Potential issue | 🟠 MajorFuture timestamps under 24h still render as negative ages.
num_days()returns0for durations like-5h, so thedays < 0guard misses that case and the next branch can format-5 hours ago. Guard on the raw signed duration before deriving days/hours/minutes, and add a+1hregression test.💡 Guard on the signed duration first
- let days = duration.num_days(); - if days < 0 { + if duration.num_seconds() <= 0 { return f.write_str("just now"); } + let days = duration.num_days();For chrono::TimeDelta/Duration, what do `num_days()` and `num_hours()` return for a negative duration such as `-5 hours`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/providers/completion.rs` around lines 34 - 49, The age formatting logic uses duration.num_days()/num_hours() which can produce negative sub-day values (e.g. -5h) and lead to "-5 hours ago"; before computing days/hours/minutes (the block around let days = duration.num_days()), check the signed duration directly (e.g. if duration.num_seconds() < 0 or duration < Duration::zero()) and treat small future offsets as "just now" (or return early) so negative sub-day durations don't fall through; adjust subsequent computations to use the absolute duration when formatting if needed, and add a regression test that constructs a timestamp +1h to verify it renders "just now" (update the test referencing the formatter used in the module).dependi-lsp/src/backend.rs (1)
1365-1366:⚠️ Potential issue | 🟠 MajorUse the resolved version in hover details.
The hover still prints
dep.versionand uses it for the release-date lookup. Whenresolved_versionis present, that can disagree with the inlay hint/diagnostic state and show the wrong age.💡 Proposed fix
- let dep_name = &*dep.name; - let dep_version = &*dep.version; + let dep_name = dep.name.as_str(); + let dep_version = dep.effective_version();- .get_release_date(&dep.version) + .get_release_date(dep_version)Also applies to: 1385-1389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/backend.rs` around lines 1365 - 1366, The hover code is using dep.version instead of the resolved version, causing mismatches; change the logic that sets dep_version (and any subsequent release-date lookup) to prefer dep.resolved_version when Some(...) and fall back to dep.version otherwise (i.e., set dep_version = dep.resolved_version.as_deref().unwrap_or(&*dep.version) or equivalent), and apply the same change in the other hover/release-date block that currently reads dep.version (the blocks referencing dep_name/dep_version and the release-date lookup).dependi-lsp/src/providers/inlay_hints.rs (1)
64-65:⚠️ Potential issue | 🟡 MinorKeep update tooltips aligned with the compared version.
These strings still interpolate
dep.versioneven though the comparison was done againstdep.effective_version(). With a resolved lockfile version, the tooltip can disagree with the actual status.Also applies to: 95-96, 115-116, 142-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/providers/inlay_hints.rs` around lines 64 - 65, The tooltip strings use dep.version even though comparisons use dep.effective_version(), causing mismatches; update all places that bind or interpolate dep.version (e.g., where dep_name and dep_version are set and any subsequent tooltip text construction) to use dep.effective_version() instead (keep dep_name = &*dep.name, but set dep_version to &*dep.effective_version() and replace other dep.version uses with dep.effective_version()) so the displayed version matches the compared/resolved version.
🧹 Nitpick comments (2)
dependi-lsp/src/reports.rs (1)
88-102: You can keep the grouping state borrowed here.
current_package = package.clone()/current_version = version.clone()reintroduceStringallocations on every new package/version heading. Tracking the previous(package.as_str(), version.as_str())pair keeps this loop allocation-free and matches the intent of the formatter refactor better.💡 Borrow the current group key instead of cloning it
- let mut current_package = String::new(); - let mut current_version = String::new(); + let mut current_package_version: Option<(&str, &str)> = None; for VulnerabilityReportEntry { package, version, @@ - if *package != current_package || *version != current_version { - current_package = package.clone(); - current_version = version.clone(); + let package_version = (package.as_str(), version.as_str()); + if current_package_version != Some(package_version) { + current_package_version = Some(package_version); writeln!(f, "### {package}@{version}\n")?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/reports.rs` around lines 88 - 102, The loop re-allocates Strings via current_package = package.clone() / current_version = version.clone(); instead, make the grouping state borrowed by changing current_package/current_version to &str (or a (&str,&str) tuple) initialized to "" and update them with package.as_str() / version.as_str() when the group changes; keep the same comparison logic in the loop that checks (*package != current_package || *version != current_version) but compare the &strs directly and write the heading using the owned package/version from the loop—this removes the per-iteration String allocations while preserving VulnerabilityReportEntry, package, version, and the grouping behavior.dependi-lsp/src/providers/inlay_hints.rs (1)
1070-1070: Skip the extra test logging.This
println!only adds noise tocargo testoutput and does not strengthen the assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/providers/inlay_hints.rs` at line 1070, Remove the noisy test-only println! by deleting the line println!(" -> is_local: {is_local}"); in the inlay hints test/code path; if you want optional debug output instead, replace it with a conditional debug log (e.g., tracing::debug!(is_local = ?is_local) or wrap in cfg!(debug_assertions)), but the simplest fix is to remove the println! so cargo test output is clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dependi-lsp/src/file_types.rs`:
- Around line 84-97: The public API removed registry_package_url; add a
deprecated compatibility wrapper method on FileType named
registry_package_url(&self, name: &str) -> String that calls the new
fmt_registry_package_url(name) and returns its to_string(), annotate it with
#[deprecated(note = "use fmt_registry_package_url which returns a Display
formatter")] and preserve a doc comment pointing users to
fmt_registry_package_url; update references in the impl where needed so existing
downstream callers keep working while emitting deprecation warnings.
In `@dependi-lsp/src/providers/inlay_hints.rs`:
- Around line 85-86: The yanked-version check uses dep.version (dep_version)
instead of the effective installed spec, so locked/manifest-backed deps can hide
a yanked resolved version; update the check in the block that calls version_info
and info.is_version_yanked(...) to pass dep.effective_version() (or a local
effective_version variable you compute earlier) instead of dep.version
(dep_version), and mirror the same change for the other occurrence mentioned
(around the 272-281 area) so both status and yanked checks use the same
effective_version value.
In `@dependi-lsp/src/utils.rs`:
- Around line 21-32: The truncation path in fmt_truncate_string can exceed
max_chars because it always appends "..." even when max_chars < 3; change the
logic to clamp the ellipsis to at most max_chars and compute keep_chars after
deciding ellipsis length. Specifically, inside fmt_truncate_string compute
ellipsis_len = min(3, max_chars), set keep_chars =
max_chars.saturating_sub(ellipsis_len), write only keep_chars characters from s,
then write an ellipsis composed of ellipsis_len dots (or nothing when
ellipsis_len is 0) so the total output never exceeds max_chars.
---
Duplicate comments:
In `@dependi-lsp/src/backend.rs`:
- Around line 1365-1366: The hover code is using dep.version instead of the
resolved version, causing mismatches; change the logic that sets dep_version
(and any subsequent release-date lookup) to prefer dep.resolved_version when
Some(...) and fall back to dep.version otherwise (i.e., set dep_version =
dep.resolved_version.as_deref().unwrap_or(&*dep.version) or equivalent), and
apply the same change in the other hover/release-date block that currently reads
dep.version (the blocks referencing dep_name/dep_version and the release-date
lookup).
In `@dependi-lsp/src/providers/completion.rs`:
- Around line 34-49: The age formatting logic uses
duration.num_days()/num_hours() which can produce negative sub-day values (e.g.
-5h) and lead to "-5 hours ago"; before computing days/hours/minutes (the block
around let days = duration.num_days()), check the signed duration directly (e.g.
if duration.num_seconds() < 0 or duration < Duration::zero()) and treat small
future offsets as "just now" (or return early) so negative sub-day durations
don't fall through; adjust subsequent computations to use the absolute duration
when formatting if needed, and add a regression test that constructs a timestamp
+1h to verify it renders "just now" (update the test referencing the formatter
used in the module).
In `@dependi-lsp/src/providers/inlay_hints.rs`:
- Around line 64-65: The tooltip strings use dep.version even though comparisons
use dep.effective_version(), causing mismatches; update all places that bind or
interpolate dep.version (e.g., where dep_name and dep_version are set and any
subsequent tooltip text construction) to use dep.effective_version() instead
(keep dep_name = &*dep.name, but set dep_version to &*dep.effective_version()
and replace other dep.version uses with dep.effective_version()) so the
displayed version matches the compared/resolved version.
---
Nitpick comments:
In `@dependi-lsp/src/providers/inlay_hints.rs`:
- Line 1070: Remove the noisy test-only println! by deleting the line println!("
-> is_local: {is_local}"); in the inlay hints test/code path; if you want
optional debug output instead, replace it with a conditional debug log (e.g.,
tracing::debug!(is_local = ?is_local) or wrap in cfg!(debug_assertions)), but
the simplest fix is to remove the println! so cargo test output is clean.
In `@dependi-lsp/src/reports.rs`:
- Around line 88-102: The loop re-allocates Strings via current_package =
package.clone() / current_version = version.clone(); instead, make the grouping
state borrowed by changing current_package/current_version to &str (or a
(&str,&str) tuple) initialized to "" and update them with package.as_str() /
version.as_str() when the group changes; keep the same comparison logic in the
loop that checks (*package != current_package || *version != current_version)
but compare the &strs directly and write the heading using the owned
package/version from the loop—this removes the per-iteration String allocations
while preserving VulnerabilityReportEntry, package, version, and the grouping
behavior.
🪄 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
Run ID: d2091097-b7f4-4bca-9676-8c84e43ebc34
📒 Files selected for processing (40)
.gitignoreCHANGELOG.mddependi-lsp/.clippy.tomldependi-lsp/Cargo.tomldependi-lsp/benches/benchmarks.rsdependi-lsp/src/auth/mod.rsdependi-lsp/src/backend.rsdependi-lsp/src/bin/test_dates.rsdependi-lsp/src/cache/mod.rsdependi-lsp/src/cache/sqlite.rsdependi-lsp/src/file_types.rsdependi-lsp/src/main.rsdependi-lsp/src/parsers/cargo.rsdependi-lsp/src/parsers/csharp.rsdependi-lsp/src/parsers/npm.rsdependi-lsp/src/parsers/php.rsdependi-lsp/src/parsers/python.rsdependi-lsp/src/parsers/ruby.rsdependi-lsp/src/providers/code_actions.rsdependi-lsp/src/providers/completion.rsdependi-lsp/src/providers/diagnostics.rsdependi-lsp/src/providers/document_links.rsdependi-lsp/src/providers/inlay_hints.rsdependi-lsp/src/registries/cargo_sparse.rsdependi-lsp/src/registries/crates_io.rsdependi-lsp/src/registries/go_proxy.rsdependi-lsp/src/registries/npm.rsdependi-lsp/src/registries/nuget.rsdependi-lsp/src/registries/packagist.rsdependi-lsp/src/registries/pub_dev.rsdependi-lsp/src/registries/pypi.rsdependi-lsp/src/registries/rubygems.rsdependi-lsp/src/reports.rsdependi-lsp/src/utils.rsdependi-lsp/src/vulnerabilities/cache.rsdependi-lsp/src/vulnerabilities/osv.rsdependi-lsp/tests/fuzz_regression.rsdependi-zed/.clippy.tomldependi-zed/Cargo.tomldependi-zed/src/lib.rs
✅ Files skipped from review due to trivial changes (18)
- .gitignore
- dependi-zed/.clippy.toml
- dependi-lsp/src/cache/mod.rs
- dependi-lsp/src/bin/test_dates.rs
- dependi-lsp/.clippy.toml
- dependi-lsp/src/parsers/csharp.rs
- dependi-zed/Cargo.toml
- dependi-lsp/src/vulnerabilities/cache.rs
- dependi-lsp/src/parsers/npm.rs
- dependi-lsp/src/parsers/php.rs
- dependi-lsp/src/parsers/cargo.rs
- dependi-lsp/src/registries/pub_dev.rs
- dependi-lsp/src/parsers/python.rs
- dependi-lsp/benches/benchmarks.rs
- dependi-lsp/src/main.rs
- dependi-lsp/src/cache/sqlite.rs
- dependi-lsp/src/providers/code_actions.rs
- dependi-lsp/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (10)
- dependi-lsp/src/vulnerabilities/osv.rs
- dependi-lsp/src/parsers/ruby.rs
- dependi-lsp/src/registries/packagist.rs
- CHANGELOG.md
- dependi-lsp/src/registries/go_proxy.rs
- dependi-lsp/src/registries/cargo_sparse.rs
- dependi-lsp/src/registries/nuget.rs
- dependi-lsp/src/auth/mod.rs
- dependi-lsp/src/providers/document_links.rs
- dependi-lsp/src/registries/npm.rs
Comment on lines
+84
to
+97
| /// Returns an <code>[fmt::Display] + [fmt::Debug]</code> implementation | ||
| /// which formats the given package `name` as a registry URL. | ||
| #[must_use = "returns a type implementing Display and Debug, which does not have any effects unless they are used"] | ||
| pub fn fmt_registry_package_url(self, name: &str) -> impl fmt::Display + fmt::Debug { | ||
| fmt::from_fn(move |f| match self { | ||
| FileType::Cargo => write!(f, "https://crates.io/crates/{name}"), | ||
| FileType::Npm => write!(f, "https://www.npmjs.com/package/{name}"), | ||
| FileType::Python => write!(f, "https://pypi.org/project/{name}"), | ||
| FileType::Go => write!(f, "https://pkg.go.dev/{name}"), | ||
| FileType::Php => write!(f, "https://packagist.org/packages/{name}"), | ||
| FileType::Dart => write!(f, "https://pub.dev/packages/{name}"), | ||
| FileType::Ruby => write!(f, "https://rubygems.org/gems/{name}"), | ||
| FileType::Csharp => write!(f, "https://www.nuget.org/packages/{name}"), | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `file_types` is exported from the crate root.
rg -n -C2 'pub mod file_types|pub use .*FileType' dependi-lsp/srcRepository: mpiton/zed-dependi
Length of output: 280
Add a deprecated compatibility wrapper for registry_package_url.
Since file_types is publicly exported from the crate root (pub mod file_types; in lib.rs), removing the registry_package_url method breaks downstream users of the public API. Provide a deprecated shim that wraps the new formatter-based approach to maintain backward compatibility:
+ #[deprecated = "use fmt_registry_package_url(name) and format it directly, or call .to_string()"]
+ pub fn registry_package_url(&self, name: &str) -> Option<String> {
+ Some((*self).fmt_registry_package_url(name).to_string())
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/file_types.rs` around lines 84 - 97, The public API removed
registry_package_url; add a deprecated compatibility wrapper method on FileType
named registry_package_url(&self, name: &str) -> String that calls the new
fmt_registry_package_url(name) and returns its to_string(), annotate it with
#[deprecated(note = "use fmt_registry_package_url which returns a Display
formatter")] and preserve a doc comment pointing users to
fmt_registry_package_url; update references in the impl where needed so existing
downstream callers keep working while emitting deprecation warnings.
Comment on lines
85
to
+86
| if let Some(info) = version_info | ||
| && info.is_version_yanked(&dep.version) | ||
| && info.is_version_yanked(dep_version) |
Contributor
There was a problem hiding this comment.
Check yanked versions against the effective version.
status already compares dep.effective_version(), but the yanked path still matches and describes dep.version. For lockfile-backed deps, that can hide a yanked resolved version entirely and show the manifest spec instead of the installed version.
Also applies to: 272-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/providers/inlay_hints.rs` around lines 85 - 86, The
yanked-version check uses dep.version (dep_version) instead of the effective
installed spec, so locked/manifest-backed deps can hide a yanked resolved
version; update the check in the block that calls version_info and
info.is_version_yanked(...) to pass dep.effective_version() (or a local
effective_version variable you compute earlier) instead of dep.version
(dep_version), and mirror the same change for the other occurrence mentioned
(around the 272-281 area) so both status and yanked checks use the same
effective_version value.
Comment on lines
+21
to
+32
| pub fn fmt_truncate_string(s: &str, max_chars: usize) -> impl fmt::Display + fmt::Debug { | ||
| fmt::from_fn(move |f| { | ||
| let char_count = s.chars().count(); | ||
| if char_count <= max_chars { | ||
| return f.write_str(s); | ||
| } | ||
|
|
||
| let keep_chars = max_chars.saturating_sub(3); | ||
| let truncated: String = s.chars().take(keep_chars).collect(); | ||
| format!("{}...", truncated) | ||
| let keep_chars = max_chars.saturating_sub(3); | ||
| s.chars() | ||
| .take(keep_chars) | ||
| .try_for_each(|c| f.write_char(c))?; | ||
| write!(f, "...") |
Contributor
There was a problem hiding this comment.
The truncation path can exceed max_chars.
When max_chars is 0, 1, or 2, this still emits "...", so callers asking for a hard cap can get a longer string than requested. That contradicts the documented contract and can overflow fixed-width UI fields.
💡 Clamp the ellipsis to the requested width
if char_count <= max_chars {
return f.write_str(s);
}
+ if max_chars < 3 {
+ return match max_chars {
+ 0 => Ok(()),
+ 1 => f.write_char('.'),
+ _ => f.write_str(".."),
+ };
+ }
+
let keep_chars = max_chars.saturating_sub(3);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn fmt_truncate_string(s: &str, max_chars: usize) -> impl fmt::Display + fmt::Debug { | |
| fmt::from_fn(move |f| { | |
| let char_count = s.chars().count(); | |
| if char_count <= max_chars { | |
| return f.write_str(s); | |
| } | |
| let keep_chars = max_chars.saturating_sub(3); | |
| let truncated: String = s.chars().take(keep_chars).collect(); | |
| format!("{}...", truncated) | |
| let keep_chars = max_chars.saturating_sub(3); | |
| s.chars() | |
| .take(keep_chars) | |
| .try_for_each(|c| f.write_char(c))?; | |
| write!(f, "...") | |
| pub fn fmt_truncate_string(s: &str, max_chars: usize) -> impl fmt::Display + fmt::Debug { | |
| fmt::from_fn(move |f| { | |
| let char_count = s.chars().count(); | |
| if char_count <= max_chars { | |
| return f.write_str(s); | |
| } | |
| if max_chars < 3 { | |
| return match max_chars { | |
| 0 => Ok(()), | |
| 1 => f.write_char('.'), | |
| _ => f.write_str(".."), | |
| }; | |
| } | |
| let keep_chars = max_chars.saturating_sub(3); | |
| s.chars() | |
| .take(keep_chars) | |
| .try_for_each(|c| f.write_char(c))?; | |
| write!(f, "...") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/utils.rs` around lines 21 - 32, The truncation path in
fmt_truncate_string can exceed max_chars because it always appends "..." even
when max_chars < 3; change the logic to clamp the ellipsis to at most max_chars
and compute keep_chars after deciding ellipsis length. Specifically, inside
fmt_truncate_string compute ellipsis_len = min(3, max_chars), set keep_chars =
max_chars.saturating_sub(ellipsis_len), write only keep_chars characters from s,
then write an ellipsis composed of ellipsis_len dots (or nothing when
ellipsis_len is 0) so the total output never exceeds max_chars.
mpiton
approved these changes
Mar 23, 2026
Owner
mpiton
left a comment
mpiton
left a comment
There was a problem hiding this comment.
All previous review items have been addressed — approving.
What was fixed since last review
- ✅ CHANGELOG.md entry added (Fixed + Changed)
- ✅ Old deprecated functions fully removed (clean, no
#[cfg_attr(not(test), deprecated)]) - ✅
+ fmt::Debugadded consistently to allfmt_*return types - ✅
#[rustfmt::skip]removed - ✅ Doc typos fixed
Verification
- All 450 unit tests and 7 integration tests pass with
cargo +1.94.0 clippy -D warningscleancargo fmt --checkclean
CodeRabbit "Major" items
The 3 items flagged by CodeRabbit as 🟠 Major are all pre-existing bugs, not regressions from this PR (CodeRabbit itself marks them "Outside diff range"). I've created #201 to track them separately.
Minor non-blocking notes for future consideration
- CHANGELOG: "Removed
String-returning formatting functions as deprecated" fits better under### Removedthan### Changedper Keep a Changelog — can be fixed when cutting the next release fmt_cache_keyispubbut only consumed internally —pub(crate)would be more precise if not intended as public API
Great work @DanikVitek, thanks for the thorough modernization! 🎉
|
Congratulations on your first merged PR, @DanikVitek! 🎊 Your contribution is now part of Dependi. Thank you for helping make dependency management better for Zed users! You're now officially a Dependi contributor. We hope to see you again! 💜 |
mpiton
added a commit
that referenced
this pull request
Mar 23, 2026
- Replace hardcoded "crates.io" in yanked diagnostics with registry-specific names and URLs via `FileType` parameter threading (#201) - Fix future timestamps rendering as negative age by guarding on `num_seconds() <= 0` instead of `num_days() < 0` (#201) - Use `dep.effective_version()` in hover handler and inlay hint tooltips for consistency with resolved lockfile versions (#201) - Fix `fmt_truncate_string` emitting "..." for `max_chars < 3` (#201) Closes #201
9 tasks
mpiton
added a commit
that referenced
this pull request
Mar 23, 2026
- Replace hardcoded "crates.io" in yanked diagnostics with registry-specific names and URLs via `FileType` parameter threading (#201) - Fix future timestamps rendering as negative age by guarding on `num_seconds() <= 0` instead of `num_days() < 0` (#201) - Use `dep.effective_version()` in hover handler and inlay hint tooltips for consistency with resolved lockfile versions (#201) - Fix `fmt_truncate_string` emitting "..." for `max_chars < 3` (#201) Closes #201
This was referenced Apr 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bump the MSRV to 1.94 and utilize the new language features such as
let-chains inifandwhile, andcore::fmt::from_fnto reduce allocations.Type of Change
Related Issue
Closes #193
Changes Made
Cargo.tomls'rust-versionCONTRIBUTING.mdto mention the new MSRVifs using the now-stablelet-chainsStringandVecallocations by utilizing thecore::fmt::{Write, from_fn}APIformat_args!("{a} {b:?}")overformat_args!("{} {:?}", a, b)where applicable[lints.clippy]inCargo.tomlTesting
cargo testpassescargo clippy -- -D warningspassescargo fmt --checkpassesChecklist
Screenshots (if applicable)
Summary by CodeRabbit
Requirements
Bug Fixes
Enhancements
Documentation
Chores