feat: Nix flake with devShell, package build, and Home Manager module#224
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces Nix flake-based reproducible builds and development for ObsidianIRC on Linux, standardizes Node.js 22 across environments, expands build and contributor documentation, adds CI flake checks, and fixes server host normalization in the Tauri app. ChangesNix Flake Development & Packaging Infrastructure
Application Maintenance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/components/ui/AddServerModal.tsxOops! Something went wrong! :( ESLint: 9.39.1 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Dockerfile (1)
1-1: ⚖️ Poor tradeoffOptional: Consider running Nginx as non-root user for improved security.
The container runs as root by default. For improved security posture, consider adding a non-root user and configuring Nginx to run under that user. This follows the principle of least privilege.
🔒 Example hardening (optional)
In the runtime stage, before the CMD:
FROM nginx:alpine COPY docker/nginx.conf /etc/nginx/conf.d/default.conf COPY --from=builder /app/dist /usr/share/nginx/html +RUN addgroup -g 1000 appuser && \ + adduser -D -u 1000 -G appuser appuser && \ + chown -R appuser:appuser /usr/share/nginx/html /var/cache/nginx /var/run && \ + sed -i 's/user nginx;/user appuser;/g' /etc/nginx/nginx.conf +USER appuser EXPOSE 80Note: This requires updating
nginx.confto remove references to paths that require root, and may need adjustment based on your nginx configuration.Also applies to: 45-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 1, Add a non-root user and run Nginx as that user in the runtime stage: create a dedicated user/group (e.g., nginx/nginx), ensure files and directories Nginx needs are chowned to that user (logs, cache, runtime directories and any served content), update nginx.conf to use non-privileged ports or remove references to root-only paths, and set USER to the new unprivileged account before CMD; adjust any places in the Dockerfile that copy files or set permissions (references: Dockerfile runtime stage, nginx.conf, and the final CMD that launches Nginx) so the container no longer runs as root..github/workflows/publish.yaml (1)
131-131: ⚡ Quick winConsider using
npm cifor reproducible builds.Four jobs in this workflow still use
npm installinstead ofnpm ci. Thepublish-ios(line 517) andpublish-web(line 618) jobs already usenpm ci, which provides reproducible installs by strictly following package-lock.json. For consistency and to match the pattern in other workflows (cloudflare_pages, github_pages, workflow.yaml), consider changing these tonpm ci.♻️ Suggested changes
- - name: install frontend dependencies - run: npm install + - name: install frontend dependencies + run: npm ciApply this pattern to lines 131, 209, 287, and 394.
Based on learnings: "Use Node ≥22 as specified in
package.jsonenginesfield and runnpm cifrom repo root for reproducible installs."Also applies to: 209-209, 287-287, 394-394
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yaml at line 131, Several workflow steps currently run "npm install" which yields non-reproducible installs; replace each "run: npm install" step with "run: npm ci" in the publish workflow (the steps currently at the positions corresponding to the earlier "npm install" occurrences) to ensure installs strictly follow package-lock.json, and confirm those steps execute from the repository root (or set working-directory: .) and that the workflow uses setup-node with node-version: 22 (or >=22) to match package.json engines; update all four occurrences to match the existing publish-ios/publish-web pattern.INSTALL.md (1)
17-17: 💤 Low valueConsider breaking the NixOS bullet into sub-bullets for readability.
This bullet point contains multiple distinct pieces of information (build command, Home Manager module, platform limitations, and cross-references) all in one very long sentence. While technically complete, it's quite dense.
Consider restructuring as:
- **NixOS / Nix-with-flakes (`x86_64-linux`, `aarch64-linux`)**: - Direct build: `nix build .#obsidianirc` - Home Manager module: `programs.obsidianirc` — see [nix/hm-module.nix](nix/hm-module.nix) - Build instructions: [BUILD.md — Nix (flake)](BUILD.md#nix-flake) - Note: This flake does not ship packages or devShells for macOS/Windows under Nix (use releases above or host toolchains — [BUILD.md](BUILD.md))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@INSTALL.md` at line 17, Split the long NixOS/Nix-with-flakes bullet into multiple sub-bullets under the existing "**NixOS / Nix-with-flakes (`x86_64-linux`, `aarch64-linux`)**" heading: create a sub-bullet for the direct build command (`nix build .#obsidianirc`), one for the Home Manager module reference (`programs.obsidianirc` and note `nix/hm-module.nix`), one pointing to the build instructions (`BUILD.md — Nix (flake)`), and a final note about platform limitations (that the flake does not ship macOS/Windows packages or devShells and to use releases/host toolchains); keep the same content but reformat to improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 310-311: The ordered list under "Translating new strings" restarts
at 1./2., breaking continuity; update the two lines currently showing "1. Run
`npm run i18n:compile`..." and "2. Commit `src/locales/`" to continue the
previous sequence (change to "3." and "4.") or alternatively convert them into a
clearly labeled substep section (e.g., add a subheading and keep 1./2. there) so
the workflow order is unambiguous; ensure the text of the commands/commit step
remains unchanged.
In `@BUILD.md`:
- Around line 61-62: The BUILD.md comment about VITE_BACKEND_URL being
"currently unused elsewhere in-tree" is inconsistent with its use/documentation
in the Docker build and vite.config.ts; update the text to accurately reflect
reality: if VITE_BACKEND_URL is forwarded via vite.config.ts and supported by
the Docker build (as referenced in the Docker section), remove the phrase
"currently unused elsewhere in-tree" and state that it is a supported build arg
forwarded to vite via vite.config.ts; otherwise, explain why it remains
documented (e.g., reserved for future use or only used in specific environments)
so readers aren’t misled. Use the symbol VITE_BACKEND_URL and mention
vite.config.ts in the updated sentence so the source of forwarding is clear.
In `@nix/package-args.nix`:
- Around line 13-27: The filter currently only checks baseName against a static
list using builtins.elem, which misses `.env` and `.env.*` filenames; update the
predicate inside filter (the lambda taking path and _type that defines baseName)
to also reject any baseName matching the regex "^\\.env(\\..*)?$" (using
builtins.match) so that both `.env` and `.env.<anything>` files are excluded in
addition to the existing builtins.elem checks.
---
Nitpick comments:
In @.github/workflows/publish.yaml:
- Line 131: Several workflow steps currently run "npm install" which yields
non-reproducible installs; replace each "run: npm install" step with "run: npm
ci" in the publish workflow (the steps currently at the positions corresponding
to the earlier "npm install" occurrences) to ensure installs strictly follow
package-lock.json, and confirm those steps execute from the repository root (or
set working-directory: .) and that the workflow uses setup-node with
node-version: 22 (or >=22) to match package.json engines; update all four
occurrences to match the existing publish-ios/publish-web pattern.
In `@Dockerfile`:
- Line 1: Add a non-root user and run Nginx as that user in the runtime stage:
create a dedicated user/group (e.g., nginx/nginx), ensure files and directories
Nginx needs are chowned to that user (logs, cache, runtime directories and any
served content), update nginx.conf to use non-privileged ports or remove
references to root-only paths, and set USER to the new unprivileged account
before CMD; adjust any places in the Dockerfile that copy files or set
permissions (references: Dockerfile runtime stage, nginx.conf, and the final CMD
that launches Nginx) so the container no longer runs as root.
In `@INSTALL.md`:
- Line 17: Split the long NixOS/Nix-with-flakes bullet into multiple sub-bullets
under the existing "**NixOS / Nix-with-flakes (`x86_64-linux`,
`aarch64-linux`)**" heading: create a sub-bullet for the direct build command
(`nix build .#obsidianirc`), one for the Home Manager module reference
(`programs.obsidianirc` and note `nix/hm-module.nix`), one pointing to the build
instructions (`BUILD.md — Nix (flake)`), and a final note about platform
limitations (that the flake does not ship macOS/Windows packages or devShells
and to use releases/host toolchains); keep the same content but reformat to
improve readability.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc81fe98-e8bf-45b1-9745-8daf01a19272
⛔ Files ignored due to path filters (2)
flake.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.envrc.github/workflows/cloudflare_pages.yaml.github/workflows/github_pages.yaml.github/workflows/publish.yaml.github/workflows/workflow.yaml.gitignoreAGENTS.mdARCHITECTURE.mdBUILD.mdCONTRIBUTING.mdDockerfileINSTALL.mdbiome.jsonflake.nixnix/hm-module.nixnix/obsidianirc.nixnix/overlay.nixnix/package-args.nixpackage.jsonrust-toolchain.tomlsrc-tauri/Cargo.tomlsrc-tauri/tauri.conf.jsontests/setup.ts
matheusfillipe
left a comment
matheusfillipe
left a comment
There was a problem hiding this comment.
Is it a limitation of nix to need to use node 22?
We can support node>=22 but I would prefer docker and CI/CD to stay in the version they were previously and no pinning (for now)
Not a Nix limitation; Nix has every Node version available. I pinned to 22 because it's the current LTS and what CI was using and 23 (what the Dockerfile had) is already EOL/unsupported. But I'm happy to revert the Docker/CI changes and keep them as they were. The Nix flake uses its own Node internally so it doesn't need the rest of the repo to pin. |
…r module - devShell: Node 22, Tauri Linux libs, rustup (x86_64/aarch64-linux) - packages.obsidianirc: cargo-tauri.hook + fetchNpmDeps full build - overlays.default: injects pkgs.obsidianirc on Linux - homeManagerModules.obsidianirc: programs.obsidianirc with viteBuildEnv, autostart - checks: HM module smoke-evaluation with stub package - .envrc: direnv use flake integration - rust-toolchain.toml: pin stable + rustfmt/clippy - .gitignore: add .direnv/, /result, !.envrc
Runs nix flake check + nix build on ubuntu-latest to validate the flake.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ui/AddServerModal.tsx`:
- Around line 117-119: The normalization for serverHost in the cleanHost
computation fails to strip plain ws:// (only strips wss://) and redundantly
includes "irc" in the protocol group; update the regex used in the first
.replace() (the one applied to serverHost inside cleanHost) to match both ws and
wss (use wss?) and remove the trailing irc alternative so it correctly strips
ws://, wss://, http(s)://, irc(s):// and ircs://; keep the subsequent .replace()
that removes ports unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dcd86fe-a810-4e99-b53e-5d149d30a659
⛔ Files ignored due to path filters (2)
flake.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.envrc.github/workflows/workflow.yaml.gitignoreAGENTS.mdARCHITECTURE.mdBUILD.mdCONTRIBUTING.mdINSTALL.mdflake.nixnix/hm-module.nixnix/obsidianirc.nixnix/overlay.nixnix/package-args.nixpackage.jsonrust-toolchain.tomlsrc/components/ui/AddServerModal.tsx
✅ Files skipped from review due to trivial changes (7)
- .envrc
- package.json
- rust-toolchain.toml
- ARCHITECTURE.md
- CONTRIBUTING.md
- BUILD.md
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .gitignore
- nix/overlay.nix
- INSTALL.md
- nix/obsidianirc.nix
- flake.nix
- nix/package-args.nix
- nix/hm-module.nix
Prevents port duplication (e.g. wss://host:6697:443) when the server host string already contains a port from the discover grid prefill.
What is the actual origin of this duplication @unle4rn? Maybe the discover grid shouldn't be benerating invalid uri's to beging with... This is actually a reocurring issue I've caused somewhen. |
- AGENTS.md: reduce Nix section to 3 lines + link to BUILD.md - BUILD.md: trim Home Manager/direnv details to bullet points - INSTALL.md: replace sub-bullets with one-liner linking to BUILD.md
The discover grid itself is fine. It passes a full URI like |
2 participants
Summary
Adds a Nix flake for reproducible development and native Linux packaging (x86_64, aarch64). Also fixes a Tauri connection bug discovered during testing.
What's included
Nix flake
nix develop— Node 22, Tauri Linux libs (WebKitGTK 4.1, GTK 3, OpenSSL, etc.), rustupnix build .#obsidianirc— Full Tauri desktop build viacargo-tauri.hook+fetchNpmDepsoverlays.default— Injectspkgs.obsidianircinto nixpkgs on Linuxprograms.obsidianirc) —viteBuildEnvfor baking VITE_* vars, XDG autostart option, assertions for proper overlay/xdg setupnix flake check— Smoke-evaluates the HM module without building Tauri.envrcwithuse flakerust-toolchain.toml— Pins stable + rustfmt/clippyCI
nix-linuxjob:nix flake check && nix buildon ubuntu-latestOther
package.json:engines.node >= 22Bug fix
wss://host:6697:443) when the discover grid prefills a host that already contains a port.Path to official nixpkgs
nix run github:ObsidianIRC/ObsidianIRCor add to their flake inputsnix/obsidianirc.nix. Once merged, Hydra builds it and every NixOS user gets it fromcache.nixos.orgfor freeTesting
npm run test— 59 files, 785 passed ✅npm run build✅nix flake check✅nix build .#obsidianirc— binary launches and connects ✅Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores