fix: tolerate offline failures in cargo-globals and hyprpanel notifications#951
fix: tolerate offline failures in cargo-globals and hyprpanel notifications#951shunkakinoki wants to merge 2 commits intomainfrom
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Warning Rate limit exceeded
⌛ 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)
✨ 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 |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances system robustness and user experience by addressing two distinct issues. It improves the reliability of Home Manager's Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRTolerate offline failures in What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves robustness by making rustup commands in the cargo-globals module non-fatal to tolerate offline scenarios and patching hyprpanel to correctly handle notifications with empty actions. A security review found no specific vulnerabilities or critical issues. My review suggests logging a warning on failure instead of silently ignoring errors with || true for the cargo-globals change, and addressing code duplication in the hyprpanel patching logic for improved maintainability.
| $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable --no-self-update || true | ||
| $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable || true |
There was a problem hiding this comment.
Using || true completely silences any errors from the rustup commands. While this prevents the activation from failing when offline, it also hides other potential errors (e.g., permissions issues, disk full) that might be important to know about.
Consider logging a warning message on failure instead. This would still allow the script to proceed but would make debugging easier if a non-network-related error occurs.
$DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable --no-self-update || echo "warning: rustup toolchain install failed, continuing..." >&2
$DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable || echo "warning: rustup default stable failed, continuing..." >&2
| substituteInPlace src/components/notifications/Notification/index.tsx \ | ||
| --replace-fail \ | ||
| 'notification.get_actions().length' \ | ||
| 'notification.get_actions().filter((a) => a.id !== "").length' | ||
| substituteInPlace src/components/notifications/Actions/index.tsx \ | ||
| --replace-fail \ | ||
| 'notification.get_actions().map' \ | ||
| 'notification.get_actions().filter((a) => a.id !== "").map' |
There was a problem hiding this comment.
This patch introduces duplication of the filtering logic notification.get_actions().filter((a) => a.id !== "") across two files. This harms maintainability, as any future changes to this logic would need to be applied in both places.
Consider if there's a way to apply this filter only once. For example, if Notification/index.tsx is the parent component, it could perform the filtering and pass the result down to Actions/index.tsx. This might require a more complex patch than substituteInPlace can easily handle (e.g., using a .patch file), but would result in cleaner code.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="home-manager/modules/cargo-globals/default.nix">
<violation number="1" location="home-manager/modules/cargo-globals/default.nix:20">
P2: `|| true` silences all errors from `rustup`, not just network-related ones. This means legitimate failures (permissions issues, disk full, corrupted toolchain, etc.) will also be silently swallowed, making them very difficult to debug. Consider logging a warning on failure instead, so the activation still succeeds but the error is visible:
```nix
$DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable --no-self-update || echo "warning: rustup toolchain install failed, continuing..." >&2
$DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable || echo "warning: rustup default stable failed, continuing..." >&2
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable --no-self-update || true | ||
| $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable || true |
There was a problem hiding this comment.
P2: || true silences all errors from rustup, not just network-related ones. This means legitimate failures (permissions issues, disk full, corrupted toolchain, etc.) will also be silently swallowed, making them very difficult to debug. Consider logging a warning on failure instead, so the activation still succeeds but the error is visible:
$DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable --no-self-update || echo "warning: rustup toolchain install failed, continuing..." >&2
$DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable || echo "warning: rustup default stable failed, continuing..." >&2Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/modules/cargo-globals/default.nix, line 20:
<comment>`|| true` silences all errors from `rustup`, not just network-related ones. This means legitimate failures (permissions issues, disk full, corrupted toolchain, etc.) will also be silently swallowed, making them very difficult to debug. Consider logging a warning on failure instead, so the activation still succeeds but the error is visible:
```nix
$DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable --no-self-update || echo "warning: rustup toolchain install failed, continuing..." >&2
$DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable || echo "warning: rustup default stable failed, continuing..." >&2
```</comment>
<file context>
@@ -17,8 +17,8 @@ in
export CARGO_HOME="$HOME/.cargo"
- $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable
- $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable
+ $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable --no-self-update || true
+ $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable || true
export PKG_CONFIG_PATH="${pkgs.openssl.dev}/lib/pkgconfig${libiconvPkgConfigPath}''${PKG_CONFIG_PATH:+:$PKG_CONFIG_PATH}"
</file context>
| $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable --no-self-update || true | |
| $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable || true | |
| $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup toolchain install stable --no-self-update || echo "warning: rustup toolchain install failed, continuing..." >&2 | |
| $DRY_RUN_CMD ${pkgs.rustup}/bin/rustup default stable || echo "warning: rustup default stable failed, continuing..." >&2 |
1 participant
Summary
rustup toolchain install stablefails with DNS errors when network is unavailable at Home Manager activation time, causing the entire service to fail. Add--no-self-updateand|| trueto make it non-fatal.id: ""), which bypasses theget_actions().lengthguard and renders a blank pale bar. ApostPatchoverlay filters out empty-ID actions before the guard and the render map.Test plan
Summary by cubic
Prevent offline failures in cargo-globals and hide empty action bars in hyprpanel notifications. Home Manager activation no longer breaks without network, and notifications from apps like Claude Code render correctly.
Written for commit 5c7bb97. Summary will update on new commits.