Repository navigation
refactor(scripts): use match and functional pipelines in Nushell scripts - #1485
Conversation
Replace the if/else status chain and defensive describe checks with match arms, guard patterns, and record destructuring for comment author and body extraction. Co-authored-by: ryoppippi <[email protected]>
Split portability, manifest, and version probes into small functions that dispatch via match list patterns and pipeline combinators instead of nested if/else blocks. Co-authored-by: ryoppippi <[email protected]>
Resolve the package dir and binary name through match, dispatch platform finalization with match, and turn the darwin dylib rewrite loop into a where/each pipeline that reports the first failed rewrite. Co-authored-by: ryoppippi <[email protected]>
Model binary readiness checks as functions returning an issue string or null, dispatched with match, instead of try/catch around imperative checks. Use path type so directories report 'is not a file' instead of an incidental ls error. Co-authored-by: ryoppippi <[email protected]>
📝 WalkthroughWalkthroughFour Nushell scripts are refactored into helper-based and ChangesPR comment upsert
Native package workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
CI failure diagnosisThe Fix appliedRan
Push blockedCommit The pullfrog push tool authenticates as Task list (6/6 completed)
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
ccusage-guide | 07a6cc2 | Commit Preview URL Branch Preview URL |
Jul 24 2026, 11:20 AM |
Match the formatting treefmt expects for ensure/stage helpers, and rewrite the upsert comment filter with intermediate lets so nufmt does not strip parentheses into invalid syntax. Co-authored-by: ryoppippi <[email protected]>
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — consistent refactor of four Nushell scripts from imperative if/else chains and for loops to functional match expressions, record/list patterns, and pipeline combinators (where/each/all). No user-facing behavior changes intended.
- Dispatch comment flows with
match—upsert-pr-comment.nuusesmatchwith guards for update status classification, record patterns forcomment_login/comment_bodyextraction, andmatchfor themaindispatch. - Extract helpers and use list-pattern
match—ensure-native-binary.nusplits portability, manifest, and version checks into small named functions;is_portable_binarydispatches viamatch [$platform, $binary]list pattern instead of nested if/else. - Pipeline-based darwin dylib rewrite —
stage-native-package.nuresolves package dir and finalization withmatch; the libiconv rewrite loop is now awhere/eachpipeline that collects results and reports the first failure. - Return issues as values, drop
try/catch—verify-native-package.nureturns issue strings (ornull) and dispatches withmatch; usespath typeinstead ofls, andget --optional 0instead offirst.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — one new commit (238334f) applies nufmt formatting to satisfy treefmt-check. Pure layout changes, no semantics affected.
upsert-pr-comment.nu—wherefilter split into intermediateletbindings (let login = comment_login $comment; let body = comment_body $comment) to avoid anufmtbug that strips parentheses into invalid Nushell syntax.ensure-native-binary.nu—manifest_lists_binaryandreported_versionexpressions wrapped across lines.stage-native-package.nu—let targetarray anderror makein the failure arm wrapped across lines.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
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="apps/ccusage/scripts/ensure-native-binary.nu">
<violation number="1" location="apps/ccusage/scripts/ensure-native-binary.nu:44">
P3: The `binary_name` function is defined identically in both `ensure-native-binary.nu` and `stage-native-package.nu`. Since these are standalone scripts, this won't cause runtime errors, but it creates a maintenance risk: any future change to the binary naming convention (e.g., a different extension, a renamed binary) requires updating both files in lockstep. Consider extracting `binary_name` to a shared Nushell module or source-able file that both scripts can import.</violation>
</file>
<file name="apps/ccusage/scripts/stage-native-package.nu">
<violation number="1" location="apps/ccusage/scripts/stage-native-package.nu:57">
P3: The new pipeline in `rewrite_darwin_system_libraries` runs all `install_name_tool` commands for matching libraries *before* checking for failures, then reports the first failed result. The original code used a `for` loop that failed fast on the first error. This means if multiple libiconv references exist and the first rewrite fails, subsequent (potentially also-failing) `install_name_tool` calls are still needlessly executed. While most binaries will only have a single libiconv reference (making this difference purely theoretical in practice), the PR states "no behavioral changes" and this is a minor behavioral divergence. Consider restoring fail-fast semantics by checking each rewrite result inline within the `each` block.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| $other => $other | ||
| } | ||
| } | ||
| def binary_name [platform: string] { |
There was a problem hiding this comment.
P3: The binary_name function is defined identically in both ensure-native-binary.nu and stage-native-package.nu. Since these are standalone scripts, this won't cause runtime errors, but it creates a maintenance risk: any future change to the binary naming convention (e.g., a different extension, a renamed binary) requires updating both files in lockstep. Consider extracting binary_name to a shared Nushell module or source-able file that both scripts can import.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/ccusage/scripts/ensure-native-binary.nu, line 44:
<comment>The `binary_name` function is defined identically in both `ensure-native-binary.nu` and `stage-native-package.nu`. Since these are standalone scripts, this won't cause runtime errors, but it creates a maintenance risk: any future change to the binary naming convention (e.g., a different extension, a renamed binary) requires updating both files in lockstep. Consider extracting `binary_name` to a shared Nushell module or source-able file that both scripts can import.</comment>
<file context>
@@ -42,85 +41,72 @@ def node_arch [] {
$other => $other
}
}
+def binary_name [platform: string] {
+ match $platform {
+ 'win32' => 'ccusage.exe'
</file context>
| | skip 1 | ||
| | each {|line| $line | str trim | split row --regex '\s+' | first } | ||
| | where {|library| $library =~ '^/nix/store/[^/]+-libiconv-[^/]+/lib/libiconv\.2\.dylib$' } | ||
| | each {|library| |
There was a problem hiding this comment.
P3: The new pipeline in rewrite_darwin_system_libraries runs all install_name_tool commands for matching libraries before checking for failures, then reports the first failed result. The original code used a for loop that failed fast on the first error. This means if multiple libiconv references exist and the first rewrite fails, subsequent (potentially also-failing) install_name_tool calls are still needlessly executed. While most binaries will only have a single libiconv reference (making this difference purely theoretical in practice), the PR states "no behavioral changes" and this is a minor behavioral divergence. Consider restoring fail-fast semantics by checking each rewrite result inline within the each block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/ccusage/scripts/stage-native-package.nu, line 54:
<comment>The new pipeline in `rewrite_darwin_system_libraries` runs all `install_name_tool` commands for matching libraries *before* checking for failures, then reports the first failed result. The original code used a `for` loop that failed fast on the first error. This means if multiple libiconv references exist and the first rewrite fails, subsequent (potentially also-failing) `install_name_tool` calls are still needlessly executed. While most binaries will only have a single libiconv reference (making this difference purely theoretical in practice), the PR states "no behavioral changes" and this is a minor behavioral divergence. Consider restoring fail-fast semantics by checking each rewrite result inline within the `each` block.</comment>
<file context>
@@ -10,43 +10,55 @@ const package_dirs = {
+ | skip 1
+ | each {|line| $line | str trim | split row --regex '\s+' | first }
+ | where {|library| $library =~ '^/nix/store/[^/]+-libiconv-[^/]+/lib/libiconv\.2\.dylib$' }
+ | each {|library|
+ {library: $library, rewrite: (run-external install_name_tool '-change' $library /usr/lib/libiconv.2.dylib $binary_path | complete)}
}
</file context>
Wrap the four-arg update_or_recreate_comment signature and long auth-skip print match arms the way nufmt expects. Co-authored-by: ryoppippi <[email protected]>
ccusage
@ccusage/ccusage-darwin-arm64
@ccusage/ccusage-darwin-x64
@ccusage/ccusage-linux-arm64
@ccusage/ccusage-linux-x64
@ccusage/ccusage-win32-x64
commit: |
ccusage performance comparisonPR SHA: This compares the Rust PR release binary against the configured base package on the same CI runner. Package runtime diagnosticsCompares the PR package wrapper, the installed native optional dependency binary, and the workspace release binary on the same large fixture. This identifies whether slow package results come from JavaScript wrapper overhead, the published native binary build, or the Rust core itself. Fixtures: Claude
Committed fixture performanceCommitted small fixtures for stable PR-to-PR feedback and explicit Claude/Codex command coverage. Fixtures: Claude
Large real-world-shaped fixture performanceGenerated fixtures shaped from aggregate local log statistics: thousands of JSONL files, many small sessions, and a long tail of larger sessions. No real prompts, paths, or outputs are stored in the fixtures. Fixtures: Claude
Artifact size
Lower medians and smaller artifacts are better. CI runner noise still applies; use same-run ratios as directional PR feedback, not release guarantees. |
ccusage performance comparisonPR SHA: This compares the PR package against the configured base package on the same CI runner. Package runtime diagnosticsCompares the PR package wrapper, the installed native optional dependency binary, and the workspace release binary on the same large fixture. This identifies whether slow package results come from JavaScript wrapper overhead, the published native binary build, or the Rust core itself. Fixtures: Claude
Committed fixture performanceCommitted small fixtures for stable PR-to-PR feedback and explicit Claude/Codex command coverage. Fixtures: Claude
Large real-world-shaped fixture performanceGenerated fixtures shaped from aggregate local log statistics: thousands of JSONL files, many small sessions, and a long tail of larger sessions. No real prompts, paths, or outputs are stored in the fixtures. Fixtures: Claude
Artifact size
Lower medians and smaller artifacts are better. CI runner noise still applies; use same-run ratios as directional PR feedback, not release guarantees. |
Summary
Rewrites the repo Nushell scripts in a more functional style:
matchexpressions (including guards, record patterns, and list patterns) replaceif/elsechains and defensivedescribechecks, and pipelines (where/each/all) replace loops and nested conditionals. No behavior changes intended..github/scripts/upsert-pr-comment.nu: the update-status dispatch (ok/missing/auth/error) is now amatch; comment author/body extraction uses record patterns and guard patterns instead ofdescribe =~ '^record'checks; status classification intry_update_commentusesmatchwith guards.apps/ccusage/scripts/ensure-native-binary.nu: nestedif/elseinis_portable_binary,native_package_includes_binary, andhas_expected_versionsplit into small functions dispatched viamatch(including a list pattern on[$target_platform, $binary]); the package-root scan is awhere/eachpipeline.apps/ccusage/scripts/stage-native-package.nu: package-dir resolution, binary naming, and per-platform finalization dispatch viamatch; the darwin dylib rewriteforloop is now a pipeline that collects failed rewrites as values and raises once.apps/ccusage/scripts/verify-native-package.nu: readiness checks return an issue string ornulland are dispatched withmatchinstead oftry/catcharound imperative checks. Usespath type, so a directory at the binary path now correctly reports "is not a file" instead of an incidentallserror.generate-e2e-fixture.nuwas already pipeline-based and is unchanged.CI follow-up
security & lint preflightinitially failed ontreefmt-check/ nufmt layout. Follow-up commits apply the expected wrapping and rewrite the upsertwherefilter with intermediatelets so nufmt does not strip parentheses into invalid syntax. Verified locally with the flake-pinnednufmt --dry-run(all five.nuscripts already formatted). CI is green.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Summary by cubic
Refactors Nushell scripts to use
matchand pipeline combinators for a clearer, more functional style. Appliesnufmtlayout to satisfytreefmt-check; no behavior changes.find_existing_comment,comment_login, andcomment_body; dispatch upsert viamatch; centralizeupdate_or_recreate_comment; classify update status viamatch; make filter, long signature, and print armsnufmt-friendly.binary_name,manifest_lists_binary,linux_binary_is_static,darwin_binary_links_only_system_dylibs, andreported_version; usematch(including list patterns) and pipelines for portability, manifest, and version checks.match; finalize per-platform viafinalize_target; rewrite darwin dylibs with a pipeline that reports the first failed rewrite.match; usepath typeso directories report “is not a file”; skip executability checks for.exe.Written for commit 07a6cc2. Summary will update on new commits.
Summary by CodeRabbit
--versionparsing.