feat: add nix paths to COMMON_BIN_PATHS #1386
Conversation
|
|
Summary of ChangesHello @phatblat, 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 the application's ability to locate common binaries by integrating standard Nix package manager paths into the environment variable detection logic. This change specifically targets macOS and Linux systems, ensuring that tools installed via Nix or NixOS are more readily discoverable by the frontend application. Highlights
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
|
📝 WalkthroughWalkthroughThis PR extends the COMMON_BIN_PATHS constant in the frontend's environment utilities with Nix-related binary paths for Darwin and Linux platforms, enabling the application to locate executables installed via the Nix package manager. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
![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 adds common Nix binary paths to the COMMON_BIN_PATHS constant for both macOS and Linux, which will help in locating executables installed via Nix. The changes are correct and improve support for Nix environments. I've added one suggestion to improve maintainability by avoiding code duplication.
| '~/.nix-profile/bin', // Nix/home-manager user profile | ||
| '/nix/var/nix/profiles/default/bin', // Determinate Nix default profile |
There was a problem hiding this comment.
These Nix paths are also added for Linux below. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider defining these common paths in a shared constant and spreading it into both the darwin and linux arrays. This would prevent them from getting out of sync in the future.
For example:
const NIX_COMMON_PATHS = [
'~/.nix-profile/bin', // Nix/home-manager user profile
'/nix/var/nix/profiles/default/bin', // Determinate Nix default profile
];
export const COMMON_BIN_PATHS: Record<string, string[]> = {
darwin: [
// ... other paths
...NIX_COMMON_PATHS,
],
linux: [
// ... other paths
...NIX_COMMON_PATHS,
'/run/current-system/sw/bin', // NixOS system profile
],
// ...
};
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/env-utils.ts (1)
125-137: Avoid hardcoded Nix paths here; route through platform abstraction/config instead.These newly added absolute paths are platform-specific and violate the repo guidance for this area. Please move Nix path resolution into the platform abstraction (or a dedicated config/provider) and surface them via a function (e.g.,
getCommonBinPaths()), rather than hardcoding in this module. As per coding guidelines, avoid platform-specific path literals in **/*.{ts,tsx,js,py}.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 1 structural/other issue(s) require attention.
Branch is out of date with base branch. Update branch first - if no conflicts arise, you can merge. If merge conflicts arise, resolve them and run follow-up review again. 1 non-blocking suggestion(s) to consider.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
Findings Summary
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (1 selected of 1 total)
🔵 [1053dc69eac9] [LOW] [Potential] Gemini DRY suggestion is a nitpick - existing pattern is intentional
📁 apps/frontend/src/main/env-utils.ts:125
Gemini Code Assist suggested extracting shared Nix paths to avoid duplication between darwin and linux. However, this contradicts the established codebase pattern: three other paths (usr/local/bin, ~/.local/bin, ~/.dotnet/tools) are already intentionally duplicated between platforms. Self-contained arrays improve readability - you can see all paths for a platform at a glance. The refactoring cost (added indirection, spread operators) outweighs the benefit of saving 2 lines.
Suggested fix:
No change needed. The current pattern of self-contained platform arrays is intentional and matches existing codebase conventions.
This review was generated by Auto Claude.
AndyMik90
force-pushed
the
develop
branch
2 times, most recently
from
67a743f to
e83e445
Compare
3 participants
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Adds common nix paths to
COMMON_BIN_PATHSfor darwin and linux.Related Issue
Closes #
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemChecklist
developbranchencoding="utf-8"for text filesPlatform Testing Checklist
CRITICAL: This project supports Windows, macOS, and Linux. Platform-specific bugs are a common source of breakage.
platform/module instead of directprocess.platformchecksfindExecutable()or platform abstractions)If you only have access to one OS: CI now tests on all platforms. Ensure all checks pass before submitting.
CI/Testing Requirements
Screenshots
Feature Toggle
use_feature_nameBreaking Changes
Breaking: Yes / No
Details:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.