feat: add a show-diff suggest flag#144
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Changes--show-diff flag for suggest command
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/suggest.ts (1)
220-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInner
truncationshadows the outer one declared at Line 180.The block-scoped
let truncationhere shadows the outertruncationfromtruncateDiff(...). They represent the same concept computed twice (once for--show-diff/--dry-rundisplay, once from the LLM result), which is confusing and a common lint flag. Consider renaming the inner variable (e.g.,genTruncation) for clarity.🤖 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 `@src/commands/suggest.ts` around lines 220 - 222, The inner truncation declaration inside the while loop in suggest.ts shadows the outer truncation from truncateDiff(...), which makes the flow confusing and trips linting. Rename the loop-local variable in the suggestion generation path (for example, to genTruncation) and update the corresponding uses in the loop so it is clear it refers to the LLM-produced truncation info, not the earlier display truncation.
🤖 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.
Outside diff comments:
In `@src/commands/suggest.ts`:
- Around line 220-222: The inner truncation declaration inside the while loop in
suggest.ts shadows the outer truncation from truncateDiff(...), which makes the
flow confusing and trips linting. Rename the loop-local variable in the
suggestion generation path (for example, to genTruncation) and update the
corresponding uses in the loop so it is clear it refers to the LLM-produced
truncation info, not the earlier display truncation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9429c7b0-5c6b-4c23-b2e9-bf45aa3c9823
📒 Files selected for processing (3)
src/commands/suggest.tssrc/index.tstests/e2e/suggest-smoke.test.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for CLI entry point implementation in
src/index.ts
Files:
src/index.ts
src/commands/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Organize command implementations in
src/commands/directory
Files:
src/commands/suggest.ts
🔇 Additional comments (6)
src/commands/suggest.ts (3)
101-101: LGTM!
180-201: LGTM!
203-211: LGTM!src/index.ts (1)
75-75: LGTM!Also applies to: 87-87
tests/e2e/suggest-smoke.test.mjs (2)
448-516: LGTM!
518-579: LGTM!
|
Patch the findings above😊 |
|
Patched the CodeRabbit finding in 58689eb by renaming the suggestion-generation truncation metadata so it no longer shadows the earlier display truncation value.\n\nValidated locally with:\n- npm test\n- npx prettier --check src/commands/suggest.ts\n- git diff --check |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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/commands/suggest.ts`:
- Around line 203-211: The `generateSuggestionsStream` truncation behavior is
already aligned with the CLI preview, so the missing coverage is an e2e test for
`suggest.ts` when `options.showDiff` and `--stream` are both enabled. Add a test
in the existing CLI/e2e suite that exercises `suggest` with both flags, asserts
the “Diff being analyzed” output uses the truncated diff preview, and verifies
the streamed response still matches the same truncated payload produced by
`truncateDiff`/`config.maxDiffSize`.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41295f00-0bed-4cc1-8037-423ff25064cb
📒 Files selected for processing (3)
src/commands/suggest.tssrc/index.tstests/e2e/suggest-smoke.test.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for CLI entry point implementation in
src/index.ts
Files:
src/index.ts
src/commands/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Organize command implementations in
src/commands/directory
Files:
src/commands/suggest.ts
🔇 Additional comments (5)
src/commands/suggest.ts (2)
180-180: LGTM!
222-222: LGTM!Also applies to: 241-241, 281-281, 293-298
src/index.ts (1)
75-75: LGTM!Also applies to: 87-87
tests/e2e/suggest-smoke.test.mjs (2)
448-516: LGTM!
518-579: LGTM!
|
Patched the latest CodeRabbit finding in 823d289 by adding e2e coverage for Validated locally with:
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 166 complexity · 6 duplication
Metric Results Complexity 166 Duplication 6
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The current implementation is not up to standards due to high cyclomatic complexity and code duplication in src/commands/suggest.ts, which has been identified as a high-risk file. While the PR successfully introduces the --show-diff flag and its associated E2E tests, it introduces a logic inconsistency: the user is shown a truncated diff preview, but the actual request sent to the LLM still contains the raw, untruncated diff. Additionally, the new logic for truncation is executed unconditionally, impacting performance for large diffs even when the preview or dry-run modes are disabled. These structural and logical issues should be addressed to ensure maintainability and correctness.
About this PR
- There is a systemic risk of divergence between what the user sees in the
--show-diffpreview and what is actually sent to the LLM provider. The truncation logic is currently duplicated or ignored during the final API calls, which can lead to unexpected token usage or provider-side errors that the user cannot see in the preview.
2 comments outside of the diff
src/commands/suggest.ts
line 89🔴 HIGH RISK
ThesuggestCommandfunction is becoming too complex (cyclomatic complexity: 18) and exceeds maintainability thresholds. This file is currently considered high-risk due to high complexity combined with low coverage. Refactor the function to extract Git change detection and LLM suggestion loops into private helper functions.
line 130🟡 MEDIUM RISK
Suggestion: The configuration loading and API key validation logic is duplicated fromsrc/commands/batch.ts. Extract this logic into a shared utility function insrc/utils/config.tsto improve maintainability.
Test suggestions
- Verify --show-diff prints the diff content to stdout for staged changes.
- Verify truncation notice is displayed when the diff exceeds maxDiffSize.
- Verify --show-diff works correctly with unstaged changes in auto-mode.
- Verify the diff is printed before streaming suggestions begin.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| @@ -267,7 +278,7 @@ export async function suggestCommand( | |||
| try { | |||
| const result = await generateSuggestions(config, diffResult.diff, profile, apiKey); | |||
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Pass truncatedDiff to the generateSuggestions call to maintain consistency with the preview shown to the user via the --show-diff flag.
| @@ -227,7 +238,7 @@ export async function suggestCommand( | |||
| try { | |||
| for await (const event of generateSuggestionsStream(config, diffResult.diff, profile, apiKey, streamProvider)) { | |||
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Pass the pre-truncated truncatedDiff to generateSuggestionsStream instead of the raw diffResult.diff. This ensures the LLM receives exactly what was shown to the user in the preview.
| } | ||
|
|
||
| const profile = await buildProfile(config.historySize); | ||
| const { diff: truncatedDiff, info: truncation } = truncateDiff(diffResult.diff, config.maxDiffSize); |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The truncateDiff call is now performed unconditionally. Move this call so it only executes when options.dryRun or options.showDiff is true to avoid unnecessary memory allocation and CPU usage for large diffs in standard runs.
|
Addressed the current Codacy/CodeRabbit show-diff input feedback in e5a3f20. The command now only precomputes the preview truncation for dry-run/show-diff, and Validation:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/commands/suggest.ts (1)
215-215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't feed the already-truncated preview back into generation.
Line 215 reuses
preview!.diff, but bothgenerateSuggestions()andgenerateSuggestionsStream()truncate theirdiffargument again. BecausetruncateDiff()can return a string slightly larger thanmaxDiffSizeonce it appends the marker, the second pass can rewrite the payload, so--show-diffno longer guarantees that the printed preview is the exact prompt sent to the LLM. It also makesgeneratedTruncationdisappear in verbose/warning output whenever the pre-truncated preview happens to fit on the second pass.Suggested fix
- const analysisDiff = options.showDiff ? preview!.diff : diffResult.diff; + const analysisDiff = diffResult.diff;🤖 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 `@src/commands/suggest.ts` at line 215, The `suggest.ts` flow is reusing `preview!.diff` for `analysisDiff`, which means the already-truncated preview is being truncated again inside `generateSuggestions()` and `generateSuggestionsStream()`. Update the `options.showDiff` path so the exact preview used for display is also the exact payload passed to generation, or skip the second truncation when the diff is already a preview; keep the relevant logic around `analysisDiff`, `truncateDiff`, and the `generateSuggestions*` callers consistent so `--show-diff` matches the prompt and truncation warnings remain accurate.
🤖 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.
Duplicate comments:
In `@src/commands/suggest.ts`:
- Line 215: The `suggest.ts` flow is reusing `preview!.diff` for `analysisDiff`,
which means the already-truncated preview is being truncated again inside
`generateSuggestions()` and `generateSuggestionsStream()`. Update the
`options.showDiff` path so the exact preview used for display is also the exact
payload passed to generation, or skip the second truncation when the diff is
already a preview; keep the relevant logic around `analysisDiff`,
`truncateDiff`, and the `generateSuggestions*` callers consistent so
`--show-diff` matches the prompt and truncation warnings remain accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3784788b-490b-4337-a197-dab34e373ca6
📒 Files selected for processing (2)
src/commands/suggest.tstests/e2e/suggest-smoke.test.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use
.jsextensions for all local imports in ESM TypeScript source files, and useimport typefor type-only imports.
Files:
src/commands/suggest.ts
src/{commands/suggest.ts,providers/sse.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Support streaming suggestions via SSE parsing, enabled by the
--streamflag on thesuggestcommand.
Files:
src/commands/suggest.ts
tests/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.mjs: Use Node.js built-innode:test(not Jest or Mocha) for test files undertests/.
Some tests may skip on Windows when platform-specific mock logic is involved.
Files:
tests/e2e/suggest-smoke.test.mjs
tests/e2e/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
End-to-end tests under
tests/e2e/should create real temporary Git repositories and local HTTP servers.
Files:
tests/e2e/suggest-smoke.test.mjs
|
Addressed the current CodeRabbit duplicate-truncation finding in 31a244b.
Validation:
|
4edd292 to
31a244b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/e2e/suggest-smoke.test.mjs`:
- Around line 627-638: The unstaged suggest smoke test only checks for the
presence of +updated, so it can miss divergences between the displayed diff and
the prompt content. Update the test around the suggest CLI flow in
suggest-smoke.test.mjs to capture and compare the full diff text from the
preview and the prompt, using the same pattern as the staged/streaming
assertions with extractPromptDiff(...) and extractShownDiff(stdout). Keep the
existing checks, but add a strict equality assertion to verify the shown diff
exactly matches the prompt diff for the unstaged path.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d45af5be-52e6-4200-ac14-ef2d842af0ba
📒 Files selected for processing (4)
src/commands/suggest.tssrc/index.tssrc/llm/client.tstests/e2e/suggest-smoke.test.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use
.jsextensions for all local imports in ESM TypeScript source files, and useimport typefor type-only imports.
Files:
src/index.tssrc/llm/client.tssrc/commands/suggest.ts
src/{history/store.ts,llm/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Analyze recent commits in
buildProfile()and inject the learned style profile into the system prompt as style guidance.
Files:
src/llm/client.ts
src/{commands/suggest.ts,providers/sse.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Support streaming suggestions via SSE parsing, enabled by the
--streamflag on thesuggestcommand.
Files:
src/commands/suggest.ts
tests/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.mjs: Use Node.js built-innode:test(not Jest or Mocha) for test files undertests/.
Some tests may skip on Windows when platform-specific mock logic is involved.
Files:
tests/e2e/suggest-smoke.test.mjs
tests/e2e/**/*.mjs
📄 CodeRabbit inference engine (AGENTS.md)
End-to-end tests under
tests/e2e/should create real temporary Git repositories and local HTTP servers.
Files:
tests/e2e/suggest-smoke.test.mjs
🔇 Additional comments (3)
src/commands/suggest.ts (1)
101-101: LGTM!Also applies to: 180-183, 204-216, 226-226, 243-252, 290-292, 304-308, 410-410, 481-481
src/llm/client.ts (1)
1-6: LGTM!Also applies to: 8-21, 23-34, 41-52, 73-74, 90-90, 102-104, 118-124, 143-143, 153-168, 172-177
src/index.ts (1)
75-75: LGTM!Also applies to: 87-87
| const result = await runCli(['suggest', '--show-diff', '--yes'], { cwd: repo, env }); | ||
| const stdout = stripAnsi(result.stdout); | ||
|
|
||
| assert.equal(result.code, 0); | ||
| assert.equal(result.stderr, ''); | ||
| assert.match(stdout, /Diff being analyzed:/); | ||
| assert.match(stdout, /diff --git a\/README\.md b\/README\.md/); | ||
| assert.match(stdout, /\+updated/); | ||
| assert.doesNotMatch(stdout, /Use unstaged changes for suggestions/); | ||
| assert.match(stdout, /Selected:\s+feat: inspect unstaged diff/); | ||
| assert.match(requests.at(-1).messages[1].content, /\+updated/); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert full preview/prompt equivalence for the unstaged path.
This case only proves both outputs contain +updated. It would still pass if the shown diff and the actual prompt diverged elsewhere, which is the main contract this flag is trying to protect. Please mirror the staged/streaming checks here with a full extractPromptDiff(...) === extractShownDiff(stdout) assertion.
🤖 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/e2e/suggest-smoke.test.mjs` around lines 627 - 638, The unstaged
suggest smoke test only checks for the presence of +updated, so it can miss
divergences between the displayed diff and the prompt content. Update the test
around the suggest CLI flow in suggest-smoke.test.mjs to capture and compare the
full diff text from the preview and the prompt, using the same pattern as the
staged/streaming assertions with extractPromptDiff(...) and
extractShownDiff(stdout). Keep the existing checks, but add a strict equality
assertion to verify the shown diff exactly matches the prompt diff for the
unstaged path.
Summary
--show-diff/-dtocommit-echo suggestso users can see the exact diff content being sent to the LLMCloses #87
Testing
npm testSummary by CodeRabbit
-d, --show-diffto thesuggestcommand to display the exact diff being analyzed, including truncation markers/notices when applicable.--show-diffis enabled (including streaming).--show-diffacross staged/unstaged and streamed cases withmaxDiffSizetruncation, validating stdout/stderr ordering and the diff sent for analysis.