Refactor/fix jscpd duplicates#50
Merged
Merged
Conversation
Extract shared CLI, hook, environment, color, trace, and fixture helpers used across the test suite. Refactor repeated test setup and assertions so the tests pass the normal jscpd duplicate scan without changing product behavior. Also add jscpd as a dev dependency.
Integrate jscpd via a new 'check-duplicates' script and wire it into both 'check' and 'check:ci' so duplicate code is caught in CI.
📝 WalkthroughWalkthroughThis PR extracts duplicated code patterns across hooks, command analysis, output formatting, and tests into centralized shared helpers. It establishes new infrastructure modules (hook utilities, child command normalization, worktree helpers, expanded test utilities) and refactors multiple subsystems to use these shared functions, reducing maintenance burden and improving consistency across the codebase. ChangesShared Utilities and Infrastructure
Hook System Refactoring
Command Analysis and Core Refactoring
Doctor Subsystem Tests
Test Infrastructure and Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
==========================================
+ Coverage 98.53% 98.54% +0.01%
==========================================
Files 52 53 +1
Lines 6975 6959 -16
==========================================
- Hits 6873 6858 -15
+ Misses 102 101 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR is a clean internal refactoring with no issues found. All extracted helpers preserve the original semantics, the one deliberate behavioral change in Confidence Score: 5/5Pure internal refactor with no public API or runtime behaviour changes. Every extracted helper was verified to preserve the original logic exactly. The one deliberate behavioural change (xargs now treats replacement tokens in env-var values as dynamic) is a security improvement covered by a new test. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "refactor(tests): simplify test utilities..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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)
tests/bin/doctor/system-info.test.ts (1)
99-101: ⚡ Quick winInline the single-use
fetchervariable.
fetcheris assigned fromprobes.fetcherand used exactly once on line 101. Per coding guidelines, values used only once should be inlined to reduce variable count.♻️ Proposed fix
- const probes = createCopilotDeferredFetcher(); - const fetcher = probes.fetcher; - const sysInfoPromise = getSystemInfo(fetcher); + const probes = createCopilotDeferredFetcher(); + const sysInfoPromise = getSystemInfo(probes.fetcher);As per coding guidelines, "Reduce total variable count by inlining when a value is only used once."
🤖 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 `@tests/bin/doctor/system-info.test.ts` around lines 99 - 101, The local variable fetcher is assigned from probes.fetcher and only used once in the call to getSystemInfo; inline it by passing probes.fetcher directly to getSystemInfo and remove the unnecessary fetcher binding. Update the snippet that calls createCopilotDeferredFetcher() so you keep const probes = createCopilotDeferredFetcher(); and change getSystemInfo(fetcher) to getSystemInfo(probes.fetcher), eliminating the single-use variable.tests/bin/help.test.ts (1)
135-138: ⚡ Quick winPrefer immutable result capture in these tests.
These two tests introduce mutable
let result+ reassignment. Returning bothoutputand callback result fromcaptureOutputkeeps this immutable and simpler.♻️ Suggested refactor
-function captureOutput(fn: () => void): string { +function captureOutput<T>(fn: () => T): { output: string; result: T } { const originalLog = console.log; let output = ''; console.log = (...args: unknown[]) => { output += `${args.map(String).join(' ')}\n`; }; try { - fn(); + const result = fn(); + return { output, result }; } finally { console.log = originalLog; } - return output; }- let result = false; - const output = captureOutput(() => { - result = showCommandHelp('doctor'); - }); + const { output, result } = captureOutput(() => showCommandHelp('doctor'));- let result = false; - const output = captureOutput(() => { - result = showCommandHelp('-cc'); - }); + const { output, result } = captureOutput(() => showCommandHelp('-cc'));#!/bin/bash # Verify mutable capture pattern occurrences in this test file. rg -nP -C2 'let\s+result\s*=\s*false;|result\s*=\s*showCommandHelp\(' tests/bin/help.test.tsAs per coding guidelines, "Prefer
constoverlet. Use ternaries or early returns instead of reassignment." and "Reduce total variable count by inlining when a value is only used once."Also applies to: 145-148
🤖 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 `@tests/bin/help.test.ts` around lines 135 - 138, Replace the mutable pattern (let result = false; result = showCommandHelp('doctor')) by updating captureOutput to return both the captured output and the callback return value and then use const to destructure them (e.g., const { output, result } = captureOutput(() => showCommandHelp('doctor')) or const [output, result] = captureOutput(() => showCommandHelp('doctor'))); apply the same change for the other occurrence using showCommandHelp and captureOutput so tests use immutable const bindings and no reassignment.tests/bin/cli-statusline.test.ts (1)
105-112: ⚡ Quick winUse functional iteration for mode-based test registration
Line 105 uses a
for...ofloop; this repo’s TS/JS guideline prefers functional array methods. Converting toforEachkeeps behavior identical and aligns style.Suggested change
- for (const mode of modes) { - test(`shows ${mode.name}`, async () => { - await expectStatusline( - { CLAUDE_SETTINGS_PATH: enabledSettingsPath, ...mode.env }, - mode.output, - ); - }); - } + modes.forEach((mode) => { + test(`shows ${mode.name}`, async () => { + await expectStatusline( + { CLAUDE_SETTINGS_PATH: enabledSettingsPath, ...mode.env }, + mode.output, + ); + }); + });As per coding guidelines, "Prefer functional array methods (flatMap, filter, map) over for loops; use type guards on filter to maintain type inference downstream".
🤖 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 `@tests/bin/cli-statusline.test.ts` around lines 105 - 112, The test registration uses a for...of loop over modes; replace it with a functional iteration such as modes.forEach(mode => { test(`shows ${mode.name}`, async () => { await expectStatusline({ CLAUDE_SETTINGS_PATH: enabledSettingsPath, ...mode.env }, mode.output); }); }); to comply with the repo guideline preferring functional array methods—ensure you keep the async test callback and the call to expectStatusline unchanged and reference the modes array, the test registration, and expectStatusline function when making the change.
🤖 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/core/analyze/xargs.ts`:
- Around line 65-74: The worktreeMode decision currently uses
hasDynamicReplacement which only checks childTokens, missing cases where the
replacementToken appears in childCommand.envAssignments (e.g., after
normalizeChildCommand); update the check before calling analyzeGit so that
hasDynamicReplacement also inspects childCommand.envAssignments (or compute a
new flag like hasDynamicReplacementInEnv) and pass worktreeMode: false whenever
replacementToken is found in either childTokens or childCommand.envAssignments,
referencing childCommand, childTokens, replacementToken, hasDynamicReplacement,
childCommand.envAssignments, and analyzeGit.
In `@tests/bin/explain/command.test.ts`:
- Around line 399-403: The test currently allows passing if only 'recurse' trace
steps exist, which can hide regressions in recursion-limit signaling; update the
assertions to directly check recursionLimitErrorStep rather than summing
'recurse' counts. Replace the combined assertion that uses
getTraceSteps(explainCommand(deepNested)) and a recurse-count sum with a direct
assertion like expect(recursionLimitErrorStep(deepNested)).toBeTruthy() (and the
analogous negative case where the limit should not trigger use toBeFalsy()) so
the tests assert the intended recursion-limit behavior explicitly; update both
the block using getTraceSteps/explainCommand/deepNested and the second
occurrence around lines 409-412 accordingly.
In `@tests/helpers.ts`:
- Around line 67-74: The withTempDir fixture currently cleans up in the finally
block immediately after returning fn(dir), which causes premature deletion when
fn is async; change withTempDir to be async (returning Promise<T>) and await the
callback result inside the try (e.g., const result = await fn(dir); return
result;) so cleanup in the finally runs after the awaited work completes; apply
the same change to the other fixture wrapper at the block around lines 205-212
(the analogous temp-file/dir helper) so both handle Promise-returning callbacks
correctly.
---
Nitpick comments:
In `@tests/bin/cli-statusline.test.ts`:
- Around line 105-112: The test registration uses a for...of loop over modes;
replace it with a functional iteration such as modes.forEach(mode => {
test(`shows ${mode.name}`, async () => { await expectStatusline({
CLAUDE_SETTINGS_PATH: enabledSettingsPath, ...mode.env }, mode.output); }); });
to comply with the repo guideline preferring functional array methods—ensure you
keep the async test callback and the call to expectStatusline unchanged and
reference the modes array, the test registration, and expectStatusline function
when making the change.
In `@tests/bin/doctor/system-info.test.ts`:
- Around line 99-101: The local variable fetcher is assigned from probes.fetcher
and only used once in the call to getSystemInfo; inline it by passing
probes.fetcher directly to getSystemInfo and remove the unnecessary fetcher
binding. Update the snippet that calls createCopilotDeferredFetcher() so you
keep const probes = createCopilotDeferredFetcher(); and change
getSystemInfo(fetcher) to getSystemInfo(probes.fetcher), eliminating the
single-use variable.
In `@tests/bin/help.test.ts`:
- Around line 135-138: Replace the mutable pattern (let result = false; result =
showCommandHelp('doctor')) by updating captureOutput to return both the captured
output and the callback return value and then use const to destructure them
(e.g., const { output, result } = captureOutput(() => showCommandHelp('doctor'))
or const [output, result] = captureOutput(() => showCommandHelp('doctor')));
apply the same change for the other occurrence using showCommandHelp and
captureOutput so tests use immutable const bindings and no reassignment.
🪄 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: dd151320-a61c-4b4f-880f-3ed618f22151
⛔ Files ignored due to path filters (6)
bun.lockis excluded by!**/*.lockdist/bin/cc-safety-net.jsis excluded by!**/dist/**dist/bin/hooks/common.d.tsis excluded by!**/dist/**dist/core/analyze/child-command.d.tsis excluded by!**/dist/**dist/core/worktree.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (34)
package.jsonsrc/bin/doctor/format.tssrc/bin/hooks/claude-code.tssrc/bin/hooks/common.tssrc/bin/hooks/copilot-cli.tssrc/bin/hooks/gemini-cli.tssrc/core/analyze/analyze-command.tssrc/core/analyze/child-command.tssrc/core/analyze/parallel.tssrc/core/analyze/xargs.tssrc/core/rules-git.tssrc/core/worktree.tstests/bin/cli-statusline.test.tstests/bin/colors.test.tstests/bin/doctor/activity.test.tstests/bin/doctor/config.test.tstests/bin/doctor/format.test.tstests/bin/doctor/system-info.test.tstests/bin/explain/cli.test.tstests/bin/explain/command.test.tstests/bin/explain/format.test.tstests/bin/explain/redact.test.tstests/bin/help.test.tstests/bin/hooks/claude-code-hook.test.tstests/bin/hooks/copilot-cli-hook.test.tstests/bin/hooks/gemini-cli-hook.test.tstests/bin/hooks/hook-helpers.tstests/core/analyze/analyze-coverage.test.tstests/core/audit.test.tstests/core/config.test.tstests/core/rules-custom-integration.test.tstests/core/rules-custom.test.tstests/helpers.tstests/scripts/generate-changelog.test.ts
The hasDynamicReplacement check now also looks at envAssignments for the replacement token, catching cases like where the token appears in an environment variable value rather than a direct argument.
Convert test helpers to properly await async callbacks before cleanup. This prevents race conditions where temp directories or worktrees are cleaned up before async operations complete. Adds dedicated tests validating cleanup waits for pending promises.
- Use forEach instead of for...of in statusline test loop - Inline fetcher variable in system-info test - Simplify captureOutput to return both output and result via object destructuring, eliminating mutable let bindings in callers
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 `@tests/bin/hooks/hook-helpers.ts`:
- Around line 85-93: The helper expectNoHookOutput currently only asserts stdout
and exitCode, allowing hooks to emit stderr unnoticed; update the function
(expectNoHookOutput) to also destructure and assert that stderr is an empty
string (e.g., expect(stderr).toBe('')) after calling run(input, env) so the
helper truly enforces "no output" across stdout and stderr while keeping the
existing exitCode assertion.
🪄 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: 21d735e4-7461-4686-81c9-9632c480964f
⛔ Files ignored due to path filters (2)
dist/bin/cc-safety-net.jsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (11)
src/core/analyze/xargs.tstests/bin/cli-statusline.test.tstests/bin/doctor/config.test.tstests/bin/doctor/system-info.test.tstests/bin/explain/command.test.tstests/bin/help.test.tstests/bin/hooks/hook-helpers.tstests/core/analyze/analyze-coverage.test.tstests/core/rules-git.test.tstests/helpers.test.tstests/helpers.ts
✅ Files skipped from review due to trivial changes (2)
- tests/helpers.test.ts
- tests/core/rules-git.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/core/analyze/xargs.ts
- tests/bin/doctor/system-info.test.ts
- tests/helpers.ts
- tests/core/analyze/analyze-coverage.test.ts
- tests/bin/doctor/config.test.ts
- tests/bin/cli-statusline.test.ts
- tests/bin/explain/command.test.ts
Comment on lines
+85
to
+93
| export async function expectNoHookOutput( | ||
| run: (input: object | string, env?: Record<string, string>) => Promise<HookResult>, | ||
| input: object | string, | ||
| env?: Record<string, string>, | ||
| ): Promise<void> { | ||
| const { stdout, exitCode } = await run(input, env); | ||
| expect(stdout).toBe(''); | ||
| expect(exitCode).toBe(0); | ||
| } |
There was a problem hiding this comment.
expectNoHookOutput should also assert empty stderr.
Right now a hook can emit stderr and still pass this helper, which conflicts with the helper name and can hide regressions in “no output” scenarios.
Suggested patch
export async function expectNoHookOutput(
run: (input: object | string, env?: Record<string, string>) => Promise<HookResult>,
input: object | string,
env?: Record<string, string>,
): Promise<void> {
- const { stdout, exitCode } = await run(input, env);
+ const { stdout, stderr, exitCode } = await run(input, env);
expect(stdout).toBe('');
+ expect(stderr).toBe('');
expect(exitCode).toBe(0);
}🤖 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 `@tests/bin/hooks/hook-helpers.ts` around lines 85 - 93, The helper
expectNoHookOutput currently only asserts stdout and exitCode, allowing hooks to
emit stderr unnoticed; update the function (expectNoHookOutput) to also
destructure and assert that stderr is an empty string (e.g.,
expect(stderr).toBe('')) after calling run(input, env) so the helper truly
enforces "no output" across stdout and stderr while keeping the existing
exitCode assertion.
Owner
Author
|
@greptileai review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/bin/hooks/common.ts(hook I/O + command analysis),src/core/analyze/child-command.ts(child command normalization), andtests/bin/hooks/hook-helpers.ts(hook test utilities)jscpdduplicate detection to thecheck/check:ciscripts to prevent future regressionsChanges
N/A
New shared modules
src/bin/hooks/common.ts— ConsolidatesreadHookInput,parseHookJson, andhandleBlockedHookCommandthat were previously duplicated across all three hook handlers (Claude Code, Copilot CLI, Gemini CLI)src/core/analyze/child-command.ts— ExtractsnormalizeChildCommand(wrapper stripping, busybox handling, env merging) andcollectCommandTemplatethat were duplicated betweenparallel.tsandxargs.tsSource refactors
src/bin/hooks/{claude-code,copilot-cli,gemini-cli}.ts— Replaced duplicated stdin reading, JSON parsing, env loading, config loading, and command analysis with calls tocommon.tshelperssrc/bin/doctor/format.ts— Extracted a sharedformatAsciiTablefunction, replacing 5 nearly-identical inline table rendering blocks (hooks, rules, config, environment, activity, update tables)src/core/analyze/parallel.ts— Replaced inline wrapper stripping + busybox handling withnormalizeChildCommandsrc/core/analyze/xargs.ts— Same refactor asparallel.tssrc/core/analyze/analyze-command.ts— ExtractedexportTrackedGitContextEnvNameto deduplicate two identical export-handling blockssrc/core/rules-git.ts— Replaced localfindDotGitPathwith sharedfindDotGitInAncestorsfromworktree.tssrc/core/worktree.ts— ExportedfindDotGitInAncestorsfor reuse byrules-git.tsTest refactors
tests/bin/hooks/hook-helpers.ts— New shared helper withrunHook,expectNoHookOutput, input builders (copilotBashInput,geminiShellInput,claudeCodeBashInput), and platform-specific hook runnerstests/helpers.ts— Added shared utilities:withTempDir,runSafetyNetCli,withStdoutColor,getTraceSteps,withLinkedWorktreeFixture,FakeGitFileFixture--statbelow)Tooling
jscpdas a devDependencycheck-duplicatesscript (bunx jscpd src tests --exitCode 1 --reporters ai)check-duplicatesinto bothcheckandcheck:ciscriptsScreenshots
N/A — internal refactor, no UI changes.
Testing
This now includes the new
check-duplicatesstep, so it validates both correctness and that no new duplicates are introduced.Related Issue
N/A
PR Checklist
bun run checkpasses (lint, types, dead code, rules, tests)package.jsonSummary by CodeRabbit
New Features
check-duplicatesnpm script to detect duplicate code patterns.Chores
Refactor
Tests