Skip to content

perf(core): Remove Zod from startup path and optimize output generation#1306

Merged
yamadashy merged 2 commits intomainfrom
perf/optimize-startup-output-paths
Mar 28, 2026
Merged

perf(core): Remove Zod from startup path and optimize output generation#1306
yamadashy merged 2 commits intomainfrom
perf/optimize-startup-output-paths

Conversation

@yamadashy
Copy link
Copy Markdown
Owner

@yamadashy yamadashy commented Mar 26, 2026

Summary

Cherry-picked (partial) from #1295. Removes unnecessary Zod import from the startup path and fixes several output generation inefficiencies.

Changes

  • Remove Zod import from error handling (errorHandle.ts): Replace direct z.ZodError instanceof check with duck typing (error.name === 'ZodError' + issues array check). Zod was loaded on every CLI invocation but only used in error paths. Startup time: -22%.
  • Fix O(n²) array spread (outputGenerate.ts): Replace reduce with [...acc, ...curr] (copies on every iteration) with flatMap (O(n)). Also remove redundant Set wrapping.
  • Parallelize write + clipboard (produceOutput.ts): Run disk write and clipboard copy concurrently with Promise.all instead of sequentially.
  • Remove unnecessary sort (outputSort.ts): File change counts were sorted solely for trace-level logging output.

Excluded from this cherry-pick

  • tokenCounterFactory.ts: Promise-caching race fix (depends on gpt-tokenizer migration)
  • filePathSort.ts / fileTreeGenerate.ts: localeCompare → toLowerCase optimization (could change sort order for non-ASCII file paths)

Benchmark (from automated perf suite)

  • Startup time (--version): 79ms → 62ms (-22%)

Checklist

  • Run npm run test
  • Run npm run lint

Open with Devin

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 26, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 216f7bdc-4b9e-4096-aac0-cf05cde69ac0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR modifies five core modules across metrics, output generation, and error handling. Changes include making worker cleanup async, optimizing file path and directory aggregation logic, parallelizing output writing operations, and refactoring Zod error detection from instanceof to duck typing to reduce external dependency coupling.

Changes

Cohort / File(s) Summary
Worker Cleanup
src/core/metrics/workers/calculateMetricsWorker.ts
Updated onWorkerTermination to await freeTokenCounters() instead of calling synchronously, pausing worker teardown until token counter resources are fully freed.
Output Generation & Sorting
src/core/output/outputGenerate.ts, src/core/output/outputSort.ts
Simplified file path computation in full-tree mode from deduped set to direct concat, streamlined empty-directory aggregation from reduce to flatMap, removed unused FileSearchResult import, and eliminated sorted array creation and associated trace logging.
Output Packaging
src/core/packager/produceOutput.ts
Changed output writing and clipboard-copy operations to execute concurrently via Promise.all instead of sequentially, allowing both operations to complete in parallel.
Error Handling
src/shared/errorHandle.ts
Removed static zod import and refactored rethrowValidationErrorIfZodError to detect Zod errors via duck typing (name === 'ZodError' and issues array check) instead of instanceof z.ZodError.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title directly summarizes the main changes: removing Zod from startup path and optimizing output generation, which matches the core objectives of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description is comprehensive, well-structured, and covers all required elements: clear summary of changes, detailed breakdown of each modification with rationale, benchmark results, and completed checklist items.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/optimize-startup-output-paths

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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 significantly enhances application performance by optimizing the startup path and streamlining output generation processes. It achieves a notable 22% reduction in startup time through strategic deferral of Zod loading, improved array manipulation efficiency, parallelization of I/O operations, and removal of redundant sorting logic.

Highlights

  • Zod Import Removal: Removed the unnecessary Zod import from the startup path by replacing direct z.ZodError instanceof checks with duck typing, leading to a 22% reduction in startup time.
  • Output Generation Optimization: Fixed an O(n²) array spread inefficiency in outputGenerate.ts by replacing reduce with flatMap and removing redundant Set wrapping for merging file paths.
  • Parallelized I/O Operations: Improved performance by running disk write and clipboard copy operations concurrently using Promise.all instead of sequentially in produceOutput.ts.
  • Removed Unnecessary Sorting: Eliminated a redundant sorting operation of file change counts in outputSort.ts that was only used for trace-level logging.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

gemini-code-assist[bot]

This comment was marked as resolved.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 26, 2026

⚡ Performance Benchmark

Latest commit:92816d7 fix(core): Address review feedback on worker cleanup and progress messages
Status:✅ Benchmark complete!
Ubuntu:2.29s (±0.03s) → 2.25s (±0.08s) · -0.03s (-1.4%)
macOS:1.25s (±0.17s) → 1.22s (±0.16s) · -0.03s (-2.7%)
Windows:2.83s (±0.08s) → 2.85s (±0.55s) · +0.02s (+0.8%)
Details
  • Packing the repomix repository with node bin/repomix.cjs
  • Warmup: 2 runs (discarded)
  • Measurement: 10 runs / 20 on macOS (median ± IQR)
  • Workflow run
History

767def8 fix(core): Address review feedback on worker cleanup and progress messages

Ubuntu:2.24s (±0.02s) → 2.18s (±0.02s) · -0.06s (-2.6%)
macOS:1.27s (±0.11s) → 1.25s (±0.09s) · -0.02s (-1.7%)
Windows:2.78s (±0.02s) → 2.70s (±0.04s) · -0.08s (-2.9%)

1b7c5b6 fix(core): Address review feedback on worker cleanup and progress messages

Ubuntu:2.47s (±0.01s) → 2.42s (±0.04s) · -0.05s (-1.9%)
macOS:1.24s (±0.04s) → 1.22s (±0.05s) · -0.02s (-1.7%)
Windows:3.01s (±0.09s) → 3.00s (±0.12s) · -0.01s (-0.2%)

945dbab chore: Revert unrelated package-lock.json changes

Ubuntu:2.38s (±0.01s) → 2.31s (±0.02s) · -0.06s (-2.6%)
macOS:1.23s (±0.03s) → 1.25s (±0.06s) · +0.02s (+1.5%)
Windows:2.87s (±0.10s) → 3.29s (±0.04s) · +0.41s (+14.3%)

b5cbb73 fix(core): Address review feedback on worker cleanup and progress messages

Ubuntu:2.34s (±0.00s) → 2.27s (±0.03s) · -0.07s (-3.0%)
macOS:1.26s (±0.06s) → 1.23s (±0.07s) · -0.03s (-2.3%)
Windows:2.94s (±0.08s) → 2.84s (±0.06s) · -0.11s (-3.6%)

9a1add9 perf(core): Remove Zod from startup path and optimize output generation

Ubuntu:2.49s (±0.03s) → 2.42s (±0.03s) · -0.08s (-3.0%)
macOS:1.44s (±0.12s) → 1.30s (±0.13s) · -0.13s (-9.3%)
Windows:2.97s (±0.48s) → 2.85s (±0.03s) · -0.12s (-4.0%)

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages bot commented Mar 26, 2026

Deploying repomix with  Cloudflare Pages  Cloudflare Pages

Latest commit: 92816d7
Status: ✅  Deploy successful!
Preview URL: https://ace3f28b.repomix.pages.dev
Branch Preview URL: https://perf-optimize-startup-output.repomix.pages.dev

View logs

coderabbitai[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.10%. Comparing base (a49d81a) to head (92816d7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/core/output/outputGenerate.ts 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1306      +/-   ##
==========================================
+ Coverage   87.09%   87.10%   +0.01%     
==========================================
  Files         115      115              
  Lines        4354     4358       +4     
  Branches     1010     1014       +4     
==========================================
+ Hits         3792     3796       +4     
  Misses        562      562              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yamadashy yamadashy marked this pull request as draft March 26, 2026 13:48
@yamadashy yamadashy marked this pull request as ready for review March 26, 2026 13:53
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 26, 2026

Code Review — Claude

Overall, this is a clean and well-scoped performance PR. The optimizations are correct, the cherry-pick exclusions are well-reasoned, and the second commit shows good responsiveness to review feedback. A few observations below.

Noteworthy Findings

1. Duck-typed ZodError: add a regression guard test (Recommended)

The duck-typing approach in errorHandle.ts is sound and the startup savings justify it. However, rethrowValidationErrorIfZodError currently has no direct unit tests — it's only exercised indirectly through configLoad.test.ts. Since the function now relies on error.name === 'ZodError' (a string convention rather than a type-system guarantee), a dedicated test that throws a real ZodError through this function would catch any future Zod version that changes the .name property. This is especially relevant since the project uses zod ^4.x which is a recent major rewrite.

Details — suggested test sketch
import { z } from 'zod';
import { rethrowValidationErrorIfZodError, RepomixConfigValidationError } from './errorHandle.js';

test('rethrowValidationErrorIfZodError converts ZodError to RepomixConfigValidationError', () => {
  const schema = z.object({ name: z.string() });
  try {
    schema.parse({ name: 123 });
  } catch (error) {
    expect(() => rethrowValidationErrorIfZodError(error, 'Config error')).toThrow(RepomixConfigValidationError);
  }
});

test('rethrowValidationErrorIfZodError ignores non-ZodError', () => {
  expect(() => rethrowValidationErrorIfZodError(new Error('not zod'), 'msg')).not.toThrow();
});

2. allFilePaths may contain intra-array duplicates (Not needed for this PR, but worth noting)

The comment on line 350 correctly states additionalFiles is disjoint from allFilePaths. However, the old new Set(...) also deduplicated within allFilePaths itself. If two rootDirs overlap, allFilePaths could contain duplicates that now pass through to filePathsForTree. This is a pre-existing issue (not introduced by this PR) since the dedup was accidental — but worth tracking as a follow-up.

3. Trace logging format change in outputSort.ts (Not needed)

The trace log now receives a raw Record<string, number> object instead of a sorted [key, value][] array. This changes the trace output format slightly, but has zero behavioral impact. Fine as-is.

Bot Feedback Assessment

The second commit already addressed the key feedback from Gemini (unnecessary await), CodeRabbit (same), and Devin (progress message overwrite from parallelization). CodeRabbit approved the latest commit. The remaining open CodeRabbit suggestions (defensive validation of Zod issue fields, propagating additionalFiles into filePathsByRoot) are overcautious for the scope of this PR — Zod's issues array shape is well-defined and stable, and filePathsByRoot is only used in the tree rendering path which already handles this correctly.

Verdict

Looks good to merge. The only actionable suggestion is adding a unit test for the duck-typed ZodError detection to prevent silent regressions on future Zod updates.


🤖 Generated with Claude Code

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 26, 2026

Code Review — PR #1306

Clean, well-scoped PR. The source code changes are correct and the optimizations are real. A few observations:

Noteworthy

1. Startup time claim needs clarification

The -22% benchmark is quoted from the parent PR (#1295), but some changes from that PR were excluded from this cherry-pick. More importantly, zod is still eagerly imported on every CLI invocation via cliRun.ts → defaultAction.ts → configSchema.ts (which has import { z } from 'zod'). The duck-typing change in errorHandle.ts is still valuable for correctness (works across worker boundaries), but the startup time benefit from this specific change may be negligible since Zod is loaded regardless through configSchema.ts. Consider clarifying the benchmark scope in the PR description.

2. No tests for rethrowValidationErrorIfZodError

The duck-typing logic is the most significant behavioral change in this PR, but there are no existing tests for this function (no tests/shared/errorHandle.test.ts). Consider adding tests for:

  • Real ZodError → rethrown as RepomixConfigValidationError
  • Object with name: 'ZodError' but missing issues → should not match
  • Non-ZodError → passes through without throwing
Minor observations

3. Package-lock.json still contains picomatch bumps

The third commit message says "Revert unrelated package-lock.json changes" but the diff still contains picomatch version bumps (2.3.1→2.3.2, 4.0.x→4.0.4) across all four lock files. These appear to be upgrades, not reverts. Harmless, but the commit message is inaccurate.

4. Commit message missing scope

chore: Revert unrelated package-lock.json changes → should be chore(deps): ... per project conventions.

5. Trace log readability regression (minor)

outputSort.ts previously sorted file change counts before trace logging, making it easy to spot the most-changed files. The raw object is less readable for debugging, but this is a reasonable tradeoff.

What looks good

  • flatMap replacing O(n²) reduce+spread — correct and measurably better for large repos
  • Removing redundant Set dedup where disjointness is guaranteed — good comment explaining why
  • Duck typing approach for ZodError — safe failure mode (falls through to generic error handling if check doesn't match)
  • No security concerns, no integration risks
  • Clean code, well-documented assumptions

🤖 Generated with Claude Code

@yamadashy yamadashy force-pushed the perf/optimize-startup-output-paths branch from 945dbab to 1b7c5b6 Compare March 26, 2026 15:46
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 26, 2026

PR Review

Overall this is a clean, well-motivated PR. The O(n squared) to O(n) fix and the Zod import removal are solid wins. A few items worth addressing:
1. Missing tests for rethrowValidationErrorIfZodError - The duck-typing change in errorHandle.ts is the most semantically significant change in this PR, yet rethrowValidationErrorIfZodError has zero test coverage. No tests/shared/errorHandle.test.ts exists. Given the function is exported and called from production paths (configLoad.ts), it warrants tests covering: (a) A real ZodError instance is caught and re-thrown as RepomixConfigValidationError, (b) A plain Error passes through without re-throw, (c) Partial matches (name is ZodError but no issues array) are correctly rejected.
2. Unchecked element-level cast on issues array - The Array.isArray guard confirms issues is an array, but individual elements are not validated before accessing .path.join() and .message. If an element lacks a path array, .join() would throw a TypeError inside the error handler, the worst place for a secondary crash. Consider adding per-element guards.
3. PR description mentions produceOutput.ts changes not in the diff - The description lists Parallelize write and clipboard as an included change, but produceOutput.ts is not in the actual diff (appears to have been reverted). The description should be updated.
4. Minor notes - Zod ZodIssue.path is (string or number)[], not string[]. Works at runtime via coercion but the cast misrepresents the contract. Also, the -22% startup improvement was measured with --version (exits before config loading). On normal invocations, configSchema.ts still imports Zod eagerly. The improvement is real but scoped to fast-exit paths.
The array optimizations in outputGenerate.ts and the trace-log cleanup in outputSort.ts look correct and clean.
Generated with Claude Code

@yamadashy yamadashy force-pushed the perf/optimize-startup-output-paths branch from 1b7c5b6 to 767def8 Compare March 27, 2026 16:18
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 27, 2026

PR Review — perf(core): Remove Zod from startup path and optimize output generation

Nice set of targeted perf wins. The changes are clean and the O(n²) → O(n) flatMap fix and Zod import removal are solid improvements. A few findings worth considering:

Findings

1. Unsafe type assertion on ZodError issues elementssrc/shared/errorHandle.ts:128-129

Details

The code casts issues to Array<{ path: string[]; message: string }> without validating individual element structure. If an error matches the duck type but has malformed issues (e.g., missing path or message), err.path.join('.') will throw an uncaught TypeError.

Consider adding a defensive guard:

const zodErrorText = issues
  .filter((err): err is { path: string[]; message: string } =>
    Array.isArray(err?.path) && typeof err?.message === 'string'
  )
  .map((err) => `[${err.path.join('.')}] ${err.message}`)
  .join('\n  ');

Risk: Low in practice (Zod issues are well-structured), but defensive coding here prevents obscure crashes in edge cases.

2. Set dedup removal may miss intra-array duplicatessrc/core/output/outputGenerate.ts:351

Details

The old code Array.from(new Set([...allFilePaths, ...additionalFiles])) deduplicated both across and within allFilePaths. The new .concat() only guarantees no cross-array overlap (via the filter). If allFilePaths itself contains duplicates (possible when multiple rootDirs yield overlapping paths), they'll now pass through to the tree.

The comment correctly explains why inter-set dedup isn't needed, but doesn't address intra-set uniqueness. Worth verifying that allFilePaths is guaranteed unique upstream, or keeping the Set as a safety net.

3. No unit tests for the duck-typed rethrowValidationErrorIfZodErrorsrc/shared/errorHandle.ts

Details

This function has no dedicated unit tests. The duck typing change is the highest-risk modification in this PR. While configLoad.test.ts exercises it indirectly with a real ZodError, the specific scenario this enables (recognizing a plain-object ZodError that isn't an instanceof z.ZodError) is untested.

A simple unit test with a synthetic object would guard against regressions:

it('should detect duck-typed ZodError', () => {
  const fakeZodError = Object.assign(new Error('validation'), {
    name: 'ZodError',
    issues: [{ path: ['foo', 'bar'], message: 'Required' }],
  });
  expect(() => rethrowValidationErrorIfZodError(fakeZodError, 'Config error'))
    .toThrow(RepomixConfigValidationError);
});

Minor Notes

  • Trace log readability (outputSort.ts:54): The logged object is now unsorted — slightly harder to scan when debugging git sort behavior. Not a blocker.
  • Conventions: All changes follow project conventions. PR title follows Conventional Commits correctly.

Verdict

Looks good to merge. The optimizations are well-motivated and correctly implemented. The findings above are suggestions for hardening, not blockers.


🤖 Generated with Claude Code

@yamadashy yamadashy force-pushed the perf/optimize-startup-output-paths branch from 6fa05f5 to 767def8 Compare March 27, 2026 16:30
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 27, 2026

Code Review — PR #1306

Clean, well-scoped performance PR. The Zod import removal for startup time and the O(n²) → O(n) fix are solid wins. A few items worth considering:

Noteworthy Findings

1. allFilePaths duplicates may survive into tree (outputGenerate.ts:350)

The comment correctly notes additionalFiles is disjoint from allFilePaths. However, the old new Set([...allFilePaths, ...additionalFiles]) also incidentally deduped allFilePaths itself. If rootDirs overlap (e.g., one root is a subdirectory of another), allFilePaths from packager.ts (filePathsByDir.flatMap(...)) could contain duplicates that now survive into filePathsForTree. Consider adding a brief comment noting the assumption that allFilePaths is duplicate-free, or verifying this invariant upstream.

2. Duck-typed ZodError: unguarded issue-element shape (errorHandle.ts:127)

The Array.isArray check confirms issues is an array, but the cast to { path: string[]; message: string } is unchecked at runtime. If a future Zod version changed the issue shape, err.path.join('.') would throw an opaque TypeError rather than a useful validation error. Minor risk, but a defensive .filter() or element-shape check would make this more robust.

Details — style consistency

The existing isError and isRepomixError helpers in the same file use const obj = error as Record<string, unknown> as a named intermediate before narrowing. The new code uses inline anonymous-object casts (error as { issues: unknown[] }), which is stylistically inconsistent within the file.

3. Missing test coverage for changed code paths

  • rethrowValidationErrorIfZodError has no direct unit tests — only exercised indirectly via configLoad.test.ts. The duck-typing guard itself (the core of this change) is not tested in isolation.
  • The includeEmptyDirectories: true + multiple roots branch in buildOutputGeneratorContext (where the flatMap optimization was applied) has zero test coverage.

4. Removed sort degrades trace-level debugging (outputSort.ts:53)

The sorted log output was useful for quickly identifying highest-churn files during debugging. The unsorted Record output in arbitrary insertion order is harder to scan. The sort cost is negligible relative to the git I/O preceding it — consider keeping it for diagnostic value.

CI Workflow Notes

Benchmark workflow observations
  • macOS runner cost: 20-run macOS benchmark on every main push adds meaningful CI billing over time (~10x Linux runner cost). Consider reducing macOS runs or restricting to periodic schedules.
  • Auto-push cascade: perf-benchmark-history.yml uses contents: write + auto-push: true, which commits to the repo on every main push. This could trigger downstream push-triggered workflows. Verify branch protection rules prevent cascade effects.
  • Actions are properly pinned to commit SHAs — good practice.

Summary

Finding Severity
allFilePaths duplicates survive with overlapping roots Low
ZodError issue-element shape unguarded at runtime Low
No unit tests for duck-typing logic or empty-dirs multi-root path Low
Removed sort degrades trace debugging UX Minor

Overall this is a well-motivated PR with clear perf wins. The items above are mostly edge cases and test gaps — nothing blocking.

🤖 Generated with Claude Code

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 27, 2026

PR Review

Clean, well-scoped cherry-pick. The Zod removal and O(n2) fix are solid wins. A few items to consider:
1. PR description lists a reverted change - The body still advertises Parallelize write + clipboard (produceOutput.ts), but this was added in commit 1 and reverted in commit 2. The final diff does not touch produceOutput.ts. Updating the description before merge would prevent confusion.
2. No unit tests for rethrowValidationErrorIfZodError duck typing - This function has zero direct tests anywhere in the test suite. The behavioral change from instanceof z.ZodError to duck typing is the riskiest part of this PR. If the check is ever subtly wrong, config validation errors silently propagate as unhandled errors instead of user-friendly messages. Suggested test cases: (a) A real z.ZodError is rethrown as RepomixConfigValidationError, (b) A plain Error with name ZodError but no issues property passes through untouched, (c) An unrelated Error passes through untouched.
3. Duck typing does not validate individual issue structure - errorHandle.ts:128 - The code validates that issues is an Array, but casts each element to a specific shape without checking. If an issue object has an unexpected shape, err.path.join() would throw a TypeError at runtime, hiding the original validation error. Consider filtering issues to only those matching the expected shape before mapping.
4. Minor: trace log readability regression (outputSort.ts) - The old code logged a sorted list of [file, count] pairs ranked by change frequency. The new code logs the raw Record object with non-deterministic key order. Low severity, but worth noting for anyone debugging sort behavior with --verbose.

Overall this is a clean, low-risk performance improvement. Items 1-2 are the most actionable before merge.
Generated with Claude Code (https://claude.ai/code)

yamadashy and others added 2 commits March 28, 2026 14:01
Partial cherry-pick from commit 75bec9e (#1295).

Changes included:
- Replace Zod instanceof check with duck typing in errorHandle.ts to
  avoid eagerly importing Zod on every CLI invocation (-22% startup time)
- Replace O(n²) reduce+spread with flatMap in outputGenerate.ts
- Remove redundant Set wrapping where inputs are already disjoint
- Parallelize disk write and clipboard copy in produceOutput.ts
- Remove unnecessary sort of file change counts in outputSort.ts
- Add missing await to freeTokenCounters in calculateMetricsWorker.ts

Excluded from cherry-pick:
- tokenCounterFactory.ts (depends on gpt-tokenizer migration)
- filePathSort.ts / fileTreeGenerate.ts (localeCompare changes risk
  altering sort order for non-ASCII file paths)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sages

- Remove unnecessary `await` on synchronous `freeTokenCounters()` in
  worker termination handler (flagged by Gemini and CodeRabbit)
- Revert write+clipboard parallelization to sequential execution to
  prevent progress message "Writing output file..." from being
  immediately overwritten by clipboard callback (flagged by Devin)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@yamadashy yamadashy force-pushed the perf/optimize-startup-output-paths branch from 767def8 to 92816d7 Compare March 28, 2026 05:01
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 28, 2026

Code Review Summary

Clean, well-scoped performance PR. The changes are correct and the optimizations are sound. One actionable item below.

Noteworthy Findings

1. Missing test coverage for rethrowValidationErrorIfZodError duck typing ⚠️

The most significant change in this PR — replacing instanceof z.ZodError with duck typing — has zero dedicated test coverage. The function is exercised indirectly via configLoad.test.ts, but no test verifies the duck-typing logic itself. Since this creates an implicit contract with Zod's internal error structure (error.name === 'ZodError', .issues array), a future Zod upgrade (the project uses ^4.3.6) could silently break user-facing validation messages with no test failure.

Recommendation: Add a test in tests/shared/errorHandle.test.ts that constructs an actual ZodError (importing z only in the test file — no runtime cost) and verifies it throws RepomixConfigValidationError. This serves as a regression canary. Also consider testing that non-ZodError errors pass through unchanged.

2. Minor type inaccuracy in Zod issue casting

The type assertion as { issues: Array<{ path: string[]; message: string }> } uses string[] for path, but Zod's actual type is (string | number)[]. This works at runtime because Array.join() coerces numbers to strings, but the cast is technically incorrect. Not a bug today, but could surprise a future reader who indexes into path expecting strings.

3. Theoretical dedup change (negligible risk)

The old new Set([...allFilePaths, ...additionalFiles]) would also deduplicate any duplicates within allFilePaths itself. The new concat only guarantees cross-array disjointness (which is correct — established 3 lines above). In practice, allFilePaths is already unique from upstream, so this is not a real concern. The inline comment explaining the disjointness invariant is appreciated.

4. Other changes — all clean
  • flatMap refactor: Strictly better than the old reduce+spread pattern. The old code also wastefully merged filePaths only to discard them. Good cleanup.
  • Sort removal in outputSort.ts: The sort was only used for logger.trace() output. Removing it has zero functional impact.
  • Conventions: Commit messages follow Conventional Commits. JSDoc comments properly explain the duck-typing rationale. Dependency injection patterns are correctly maintained.
  • Security: No concerns. The duck-typed error objects originate from controlled catch blocks around zod.parse() calls, so spoofing is not a realistic vector.

Overall: solid PR. The flatMap and Set-removal optimizations are clear wins. The Zod lazy-loading is a smart startup optimization. Adding test coverage for the duck typing would close the only meaningful gap.

🤖 Generated with Claude Code

@yamadashy yamadashy merged commit aeed190 into main Mar 28, 2026
61 checks passed
@yamadashy yamadashy deleted the perf/optimize-startup-output-paths branch March 28, 2026 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant