-
-
Notifications
You must be signed in to change notification settings - Fork 79
nh search: fix links and add github link #482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Warning Rate limit exceeded@neunenak has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughUpdates add terminal hyperlink handling and flake-aware nixpkgs path resolution so search output prints file:// and GitHub links for package positions, with fallbacks when hyperlinks or flake resolution are unavailable. CHANGELOG documents the user-visible changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NH as "nh search"
participant Hyper as "Hyperlink Support (lazy)"
participant Resolver as "Nixpkgs Path Resolver"
participant Formatter as "Output Formatter"
User->>NH: run search
NH->>Hyper: ensure HYPERLINKS_SUPPORTED (lazy init)
Hyper-->>NH: supported? (cached)
NH->>Resolver: resolve nixpkgs path (channel -> flake_ref)
Resolver->>Resolver: try `nix eval -f <nixpkgs> path`
alt path found
Resolver-->>NH: return path
else not found
Resolver->>Resolver: try `nix eval --raw {flake_ref}#path`
Resolver-->>NH: return path or empty
end
NH->>Formatter: format homepage + position
Formatter->>Formatter: build `file://{nixpkgs_path}/{position}` and GitHub URL
alt hyperlinks supported
Formatter-->>User: render clickable hyperlinks (homepage + GitHub links)
else not supported
Formatter-->>User: render plain text URLs and file:// paths
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Performance Comparison
|
e0fe1c0 to
3e11c0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/search.rs (1)
298-322: GitHub link could be shown even without local nixpkgs path.The GitHub link generation doesn't depend on
nixpkgs_path, but it's nested inside the!nixpkgs_path.is_empty()check. Users without a resolvable local nixpkgs path won't see the GitHub link, even though it would still be valid and useful.Consider restructuring to always show the GitHub link when the position is available:
if let Some(package_position) = &elem.package_position { match package_position.split(':').next() { Some(position) => { - // Position from search.nixos.org is already a relative path - // like "pkgs/by-name/..." if !nixpkgs_path.is_empty() { print!(" Defined at: "); print_hyperlink( position, &format!("file://{nixpkgs_path}/{position}"), ); - - print!(" Github link: "); - let url = format!("{GITHUB_NIXPKGS_URL}/{position}"); - print_hyperlink(&url, &url); } + + print!(" Github link: "); + let url = format!("{GITHUB_NIXPKGS_URL}/{position}"); + print_hyperlink(&url, &url); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)src/search.rs(5 hunks)
🔇 Additional comments (5)
src/search.rs (5)
49-66: LGTM!Good use of
OnceLockto cache the hyperlink support check. The OSC 8 escape sequence implementation for terminal hyperlinks is correct.
93-100: LGTM!The flake reference mapping logic handles the common cases appropriately, with a sensible fallback to the registry default.
252-268: LGTM!The graceful error handling with Option chaining ensures the program doesn't crash if nixpkgs path resolution fails, defaulting to an empty string which is properly checked before use.
289-292: LGTM!Clean refactoring to use the new
print_hyperlinkhelper for homepage URLs.
102-119: Verify whether nixpkgs exposes apathattribute and whether the flake defines apathoutput.The commands
nix eval -f '<nixpkgs>' pathandnix eval --raw '{flake_ref}#path'reference non-standard attributes/outputs that do not exist in typical nixpkgs or flake setups. Based on nix documentation:
- Standard nixpkgs does not expose a top-level
pathattribute (though.outPathis available on derivations).- Standard flakes do not expose a top-level
pathoutput (templates have nestedtemplates.*.path, but notoutputs.path).These commands will likely fail on most systems unless the target has been heavily customized. More reliable alternatives exist (e.g.,
builtins.findFile builtins.nixPath "nixpkgs"or evaluating.outPath).Confirm whether this custom
pathattribute/output is intentional to the project, or if the commands should be replaced with standard nix expressions.
eae5174 to
1d5d161
Compare
when you run a nh search <packagename> command, the output previously looked like: ``` $ nh search fluffychat Querying search.nixos.org, with channel nixos-unstable... warning: Nix search path entry '/home/greg/.nix-defexpr/channels' does not exist, ignoring error: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I) Took 120ms Most relevant results at the end fluffychat-web (2.0.0) Chat with your friends (matrix client) Homepage: https://fluffychat.im/ Defined at: pkgs/by-name/fl/fluffychat/package.nix fluffychat (2.0.0) Chat with your friends (matrix client) Homepage: https://fluffychat.im/ Defined at: pkgs/by-name/fl/fluffychat/package.nix ``` And the "Defined at" links were broken on nix flake systems - they pointed at a nonexistant file. This commit fixes that broken link and also adds an additional link to the file on the nixpkgs github repository: ``` $ cargo run -- search fluffychat Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.10s Running `target/debug/nh search fluffychat` Querying search.nixos.org, with channel nixos-unstable... Took 160ms Most relevant results at the end fluffychat-web (2.2.0) Chat with your friends (matrix client) Homepage: https://fluffychat.im/ Defined at: pkgs/by-name/fl/fluffychat/package.nix Github link: https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/by-name/fl/fluffychat/package.nix fluffychat (2.2.0) Chat with your friends (matrix client) Homepage: https://fluffychat.im/ Defined at: pkgs/by-name/fl/fluffychat/package.nix Github link: https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/by-name/fl/fluffychat/package.nix ```
1d5d161 to
35d3723
Compare
Sanity Checking
nix fmtto format my Nix codecargo fmtto format my Rust codecargo clippyand fixed any new linter warnings.logic
description.
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwinAdd a 👍 reaction to pull requests you find important.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.