Convert flake-iter to TypeScript #39
Conversation
|
Warning Rate limit exceeded@lucperkins has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 19 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 (1)
WalkthroughReplaces Nix-centric CI/runtime with a Node/TypeScript toolchain: adds package.json, TS build/configs, ESLint/Prettier, a new Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Runner
participant Cache as actions/cache
participant NPM as npm (node)
participant Build as tsup/build
participant Repo as Git repo
Note over GH: validate.yml — multi-step validate flow
GH->>GH: Get npm cache directory (step output)
GH->>Cache: Restore cache (key: OS + package-lock.json)
GH->>NPM: Install dependencies (`npm ci`)
GH->>NPM: Run lint (`npm run lint`)
GH->>Build: Run build (`npm run build`)
GH->>Repo: Check git status
alt staged changes present
GH->>GH: Fail "Ensure no staged changes"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
flake.nix (1)
24-26: Consider pinning to a specific Node.js version for reproducibility.Using
nodejs_latestmay introduce unexpected breaking changes when nixpkgs updates. For better reproducibility across developer environments, consider pinning to a specific major version likenodejs_22ornodejs_20.Apply this diff to pin to Node.js 22 (or adjust to your preferred version):
- packages = with pkgs; [ - nodejs_latest - action-validator - ]; + packages = with pkgs; [ + nodejs_22 + action-validator + ];package.json (2)
29-29: Consider pinning the GitHub dependency to a specific commit or tag.Using
github:DeterminateSystems/detsys-tswithout a commit SHA or version tag can lead to non-reproducible builds if the upstream repository changes.Pin to a specific commit for reproducibility:
- "detsys-ts": "github:DeterminateSystems/detsys-ts" + "detsys-ts": "github:DeterminateSystems/detsys-ts#<commit-sha>"
13-13: Consider cross-platform script compatibility.
rm -rf distis Unix-specific and will fail on Windows. If Windows development is expected, consider using a cross-platform solution.- "all": "rm -rf dist && npm run format && npm run lint && npm run build" + "all": "npx rimraf dist && npm run format && npm run lint && npm run build"Or add
rimrafas a dev dependency.tsconfig.json (1)
9-10: Remove redundantnoImplicitAnyoption.The
noImplicitAny: truesetting is redundant becausestrict: truealready enables it along with several other strict checks.Apply this diff to simplify the configuration:
"strict": true /* Enable all strict type-checking options. */, - "noImplicitAny": true /* Raise error on expressions and declarations with an implied 'any' type. */, "esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */,src/index.ts (1)
20-25: Clarify the environment variable comment.The comment on line 22 mentions specific environment variables (
$FLAKE_ITER_RUNNER_MAPand$FLAKE_ITER_NIX_SYSTEM), but the code spreads all environment variables fromprocess.env. While this is correct (those specific vars will be included), the comment could be clearer about passing the entire environment.Consider updating the comment to be more precise:
const exitCode = await actionsExec.exec(binaryPath, [], { env: { - ...process.env, // To get $FLAKE_ITER_RUNNER_MAP or $FLAKE_ITER_NIX_SYSTEM (depending on workflow step) + ...process.env, // Pass all environment variables, including $FLAKE_ITER_RUNNER_MAP and $FLAKE_ITER_NIX_SYSTEM }, ignoreReturnCode: true, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
dist/index.d.mtsis excluded by!**/dist/**dist/index.mjsis excluded by!**/dist/**dist/index.mjs.mapis excluded by!**/dist/**,!**/*.mapflake.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.github/workflows/validate.yml(1 hunks).github/workflows/workflow.yml(2 hunks).gitignore(1 hunks).prettierignore(1 hunks).prettierrc(0 hunks)eslint.config.js(1 hunks)flake.nix(2 hunks)package.json(1 hunks)prettier.config.cjs(1 hunks)src/index.ts(1 hunks)tsconfig.json(1 hunks)tsconfig.typedoc.json(1 hunks)tsdown.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .prettierrc
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
dist/index.mjs (3)
EVENT_EXECUTION_FAILURE(6-6)binaryPath(17-17)exitCode(18-21)
🔇 Additional comments (16)
flake.nix (4)
2-2: LGTM - Clean simplification of inputs.The consolidation to a single nixpkgs source from FlakeHub is appropriate for the TypeScript migration and simplifies dependency management.
5-7: LGTM - Correct pattern usage.The outputs signature and lib inheritance are properly structured. Using
{ self, ... }@inputsprovides flexibility whileinputs.nixpkgs.libcorrectly accesses the library functions.
9-13: Verify the omission of x86_64-darwin.The systems list excludes x86_64-darwin (Intel Macs), which may affect developers still using Intel-based macOS machines. Please confirm this omission is intentional.
16-16: LGTM - Clean forEachSystem implementation.The simplified implementation correctly instantiates pkgs for each system and aligns well with the single nixpkgs source approach.
.gitignore (1)
1-1: LGTM!Standard Node.js ignore pattern for dependencies.
.prettierignore (1)
1-3: LGTM!Appropriate ignore patterns for build artifacts, dependencies, and auto-generated lockfile.
tsconfig.typedoc.json (1)
1-8: LGTM!TypeDoc-specific configuration appropriately extends base config and excludes test files from documentation generation.
prettier.config.cjs (1)
1-12: LGTM!Well-structured Prettier configuration with import sorting plugin. Using
.cjsextension is correct for CommonJS in an ESM project..github/workflows/workflow.yml (1)
127-129: Same path resolution concern applies here.This step has the same
working-directoryand relative path pattern. Ensure the path todist/index.mjsresolves correctly wheninputs.directoryis not the repo root..github/workflows/validate.yml (2)
22-33: LGTM!Good practice to cache npm dependencies with a key based on
package-lock.jsonhash.
47-51: LGTM!Git status and diff checks ensure the committed
dist/matches the build output, preventing stale artifacts.eslint.config.js (1)
5-94: LGTM - Comprehensive TypeScript ESLint configuration.Well-structured flat config with strict TypeScript rules. The rule set promotes type safety and code quality.
src/index.ts (3)
7-15: LGTM!The class setup and constructor configuration are well-structured. The DetSysAction configuration properly specifies the action name, fetch style, diagnostics suffix, and Nix requirement.
27-32: LGTM!The error handling properly records the failure event for diagnostics and sets the action as failed with a clear message including the exit code.
35-43: LGTM!The no-op
post()method and entry point pattern are appropriate. The immediate invocation ofmain()at the module level is the standard pattern for GitHub Actions, and theexecute()method fromDetSysActionwill handle any internal error management.tsdown.config.ts (1)
9-11: This is the default tsdown behavior and is intentional.
dts.resolve: falseleaves external type dependencies as import statements in generated declaration files rather than bundling them. This is the recommended configuration for most libraries, keeping declaration files clean and maintaining proper dependency boundaries.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/validate.yml (1)
44-45: Consider removing this step if not needed for debugging.This step displays the git status but doesn't fail on changes. Since the actual validation occurs in the next step (line 48), this step appears to be for informational purposes only. Consider removing it unless you specifically want the output for debugging.
src/index.ts (1)
20-24: Type assertion on process.env may hide type issues.The type assertion
process.env as Record<string, string>on line 22 forces all environment variables to be strings, butprocess.envvalues can bestring | undefined. This could mask potential issues if required environment variables are missing.Consider filtering out undefined values or using a more precise type:
- const exitCode = await actionsExec.exec(binaryPath, [], { - // To get $FLAKE_ITER_RUNNER_MAP or $FLAKE_ITER_NIX_SYSTEM (depending on workflow step) - env: process.env as Record<string, string>, - ignoreReturnCode: true, - }); + const exitCode = await actionsExec.exec(binaryPath, [], { + // To get $FLAKE_ITER_RUNNER_MAP or $FLAKE_ITER_NIX_SYSTEM (depending on workflow step) + env: Object.fromEntries( + Object.entries(process.env).filter(([_, v]) => v !== undefined) + ) as Record<string, string>, + ignoreReturnCode: true, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.github/workflows/validate.yml(1 hunks).github/workflows/workflow.yml(2 hunks)package.json(1 hunks)src/index.ts(1 hunks)tsup.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/workflow.yml
- package.json
🔇 Additional comments (12)
.github/workflows/validate.yml (6)
19-20: LGTM!The workflow validation step is correctly structured and uses the appropriate nix develop command.
22-25: LGTM!The npm cache directory retrieval is correctly implemented with proper newline handling for GitHub outputs.
35-36: LGTM!Using
npm ciis the correct choice for CI environments, ensuring reproducible builds from the lockfile.
38-39: LGTM!The linting step is properly configured and runs through the nix development environment.
41-42: LGTM!The build step is correctly configured to compile the TypeScript project.
27-33: No issues with npm cache configuration.actions/cache@v5 is a valid and current version that runs on the Node.js 24 runtime. The cache setup properly uses the output from the previous step for the path, a hashed key based on package-lock.json, and fallback restore-keys—all appropriate patterns for npm caching.
src/index.ts (3)
34-35: Empty post() method is acceptable.The no-op
post()implementation is clearly documented and appropriate when no cleanup is needed.
17-24: Consider error handling for fetchExecutable().The
fetchExecutable()call is not wrapped in try-catch. If it fails, the action will crash ungracefully. While the parentDetSysActionclass may handle errors internally, verify that the base class properly catches and reports failures, or add explicit error handling if needed.
38-42: The immediate invocation pattern is correct and properly aligns with GitHub Actions entry points. TheDetSysAction.execute()method is designed to orchestrate the complete action lifecycle, invoking both themain()andpost()methods as needed. The explicitpost()method (even though empty) demonstrates proper lifecycle awareness. No changes needed.tsup.config.ts (3)
4-17: Configuration looks appropriate for GitHub Actions.The tsup configuration is well-suited for building a GitHub Action:
- CJS format is standard for GitHub Actions
- Node20 target aligns with modern Action runner environments
- Source maps aid debugging
- DTS generation with resolution helps with type checking
13-13: Bundling all dependencies withnoExternalis intentional for this CLI tool.This approach is standard for distributable command-line tools where a self-contained bundle is desired. The three dependencies present (@actions/core, @actions/exec, detsys-ts) are pure JavaScript/TypeScript libraries with no native module bindings, so runtime issues are not a concern. Bundle size is increased, but this tradeoff is typical for standalone tools.
1-1: No action required. TheresolveJsonModulecompiler option is already enabled intsconfig.json, allowing JSON imports to work correctly.Likely an incorrect or invalid review comment.
This is currently a draft because it's blocked on flake-iter not yet being available through i.d.s.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.