Import selected dpcode stability fixes#2
Merged
Conversation
📝 WalkthroughWalkthroughAppends a mouse-reporting reset sequence to terminal snapshots and replay resets; adds diff serialization and armed select-all/copy behavior with clipboard override; refactors Uint8 stream collection to accumulate binary chunks and enforce maxBytes; centralizes provider discovery pending logic in a new helper. ChangesTerminal Mouse Reporting Reset
Diff Panel Armed Select-All/Copy Behavior
Uint8 Stream Text Collection Refactor
Provider Discovery Pending State Centralization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
github-actions
Bot
added
size:L
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/lib/providerDiscoveryReactQuery.test.ts (1)
5-25: ⚡ Quick winAdd a direct test for the
isLoadingbranch.Current tests cover both
isFetchingpaths, but not the explicitisLoadingpath.✅ Suggested test addition
describe("isInitialModelDiscoveryPending", () => { + it("treats loading state as initial discovery", () => { + expect( + isInitialModelDiscoveryPending({ + isLoading: true, + isFetching: false, + isPlaceholderData: false, + }), + ).toBe(true); + }); + it("treats placeholder refetches as initial discovery", () => { expect( isInitialModelDiscoveryPending({ isLoading: false, isFetching: true,🤖 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 `@apps/web/src/lib/providerDiscoveryReactQuery.test.ts` around lines 5 - 25, Add a test that directly asserts the isLoading branch of isInitialModelDiscoveryPending: create a new it block in providerDiscoveryReactQuery.test that calls isInitialModelDiscoveryPending with isLoading: true (and isFetching/isPlaceholderData set to values that won't interfere, e.g., false) and expect the result toBe(true); reference the isInitialModelDiscoveryPending function name so the test specifically verifies the explicit isLoading path rather than relying on isFetching placeholder behavior.apps/web/src/lib/diffRendering.test.ts (1)
57-80: ⚡ Quick winStrengthen serialization test with a round-trip parse assertion.
Current
toContainchecks can pass even if diff structure is malformed. Add an assertion that reparsing the serialized output yields expected file/hunk stats.Proposed test hardening
describe("serializeRenderablePatchText", () => { it("serializes parsed file diffs back into complete unified patch text", () => { @@ expect(serialized).toContain("-const oldValue = 1;"); expect(serialized).toContain("+const newValue = 1;"); + expect(summarizePatchStats(serialized ?? "")).toEqual({ additions: 2, deletions: 1 }); }); });🤖 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 `@apps/web/src/lib/diffRendering.test.ts` around lines 57 - 80, Add a round-trip assertion: after serializing with serializeRenderablePatchText(getRenderablePatch(patch)), call getRenderablePatch(serialized) and assert the reparsed result's file and hunk metadata match expected values (e.g., number of files, hunks per file, and hunk header ranges) to ensure the serialized patch is a well-formed unified diff; use the existing helpers serializeRenderablePatchText and getRenderablePatch to perform the reparse and compare file/hunk stats rather than only string contains.
🤖 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 `@apps/web/src/components/DiffPanel.tsx`:
- Around line 580-586: The handleCopy handler currently returns early when
event.clipboardData is missing causing armed full-diff copies to fail; update
handleCopy to fall back to a reliable clipboard write by using
navigator.clipboard.writeText(diffCopyText) if available, and if not, create a
temporary textarea, set its value to diffCopyText, programmatically select it
and call document.execCommand('copy'), then clean up; keep the existing checks
for diffSelectAllArmedRef.current and diffCopyText, ensure
diffSelectAllArmedRef.current is set to false before attempting the async
navigator.clipboard.writeText to avoid races, and preserve
event.preventDefault() when using the in-event clipboardData branch.
---
Nitpick comments:
In `@apps/web/src/lib/diffRendering.test.ts`:
- Around line 57-80: Add a round-trip assertion: after serializing with
serializeRenderablePatchText(getRenderablePatch(patch)), call
getRenderablePatch(serialized) and assert the reparsed result's file and hunk
metadata match expected values (e.g., number of files, hunks per file, and hunk
header ranges) to ensure the serialized patch is a well-formed unified diff; use
the existing helpers serializeRenderablePatchText and getRenderablePatch to
perform the reparse and compare file/hunk stats rather than only string
contains.
In `@apps/web/src/lib/providerDiscoveryReactQuery.test.ts`:
- Around line 5-25: Add a test that directly asserts the isLoading branch of
isInitialModelDiscoveryPending: create a new it block in
providerDiscoveryReactQuery.test that calls isInitialModelDiscoveryPending with
isLoading: true (and isFetching/isPlaceholderData set to values that won't
interfere, e.g., false) and expect the result toBe(true); reference the
isInitialModelDiscoveryPending function name so the test specifically verifies
the explicit isLoading path rather than relying on isFetching placeholder
behavior.
🪄 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: 9a3d43d6-e7e1-450a-9d84-d359dd33853d
📒 Files selected for processing (14)
apps/server/src/stream/collectUint8StreamText.test.tsapps/server/src/stream/collectUint8StreamText.tsapps/server/src/terminal/Layers/Manager.test.tsapps/server/src/terminal/Layers/Manager.tsapps/web/src/components/ChatView.tsxapps/web/src/components/DiffPanel.logic.test.tsapps/web/src/components/DiffPanel.logic.tsapps/web/src/components/DiffPanel.tsxapps/web/src/components/terminal/terminalRuntime.tsapps/web/src/lib/diffRendering.test.tsapps/web/src/lib/diffRendering.tsapps/web/src/lib/providerDiscoveryReactQuery.test.tsapps/web/src/lib/providerDiscoveryReactQuery.tspackages/shared/src/terminalThreads.ts
Comment on lines
+580
to
+586
| const handleCopy = (event: ClipboardEvent) => { | ||
| if (!diffSelectAllArmedRef.current) return; | ||
| diffSelectAllArmedRef.current = false; | ||
| if (!diffCopyText || !event.clipboardData) return; | ||
| event.preventDefault(); | ||
| event.clipboardData.setData("text/plain", diffCopyText); | ||
| }; |
There was a problem hiding this comment.
Add a clipboard fallback when event.clipboardData is unavailable.
In the armed path, copy override currently no-ops if clipboardData is missing, which can drop the intended full-diff copy behavior in some environments.
Proposed fallback for reliability
const handleCopy = (event: ClipboardEvent) => {
if (!diffSelectAllArmedRef.current) return;
diffSelectAllArmedRef.current = false;
- if (!diffCopyText || !event.clipboardData) return;
- event.preventDefault();
- event.clipboardData.setData("text/plain", diffCopyText);
+ if (!diffCopyText) return;
+ if (event.clipboardData) {
+ event.preventDefault();
+ event.clipboardData.setData("text/plain", diffCopyText);
+ return;
+ }
+ void navigator.clipboard?.writeText(diffCopyText).catch(() => undefined);
};🤖 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 `@apps/web/src/components/DiffPanel.tsx` around lines 580 - 586, The handleCopy
handler currently returns early when event.clipboardData is missing causing
armed full-diff copies to fail; update handleCopy to fall back to a reliable
clipboard write by using navigator.clipboard.writeText(diffCopyText) if
available, and if not, create a temporary textarea, set its value to
diffCopyText, programmatically select it and call document.execCommand('copy'),
then clean up; keep the existing checks for diffSelectAllArmedRef.current and
diffCopyText, ensure diffSelectAllArmedRef.current is set to false before
attempting the async navigator.clipboard.writeText to avoid races, and preserve
event.preventDefault() when using the in-event clipboardData branch.
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)
apps/web/src/components/DiffPanel.tsx (1)
1080-1114:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse one effective collapsed state for oversized files.
Line 1113 forces collapse when
!canRender, but Lines 1137 and 1101 still use/toggleisCollapsedalone, so the chevron can imply expandability even when expansion is impossible.Suggested fix
- const isCollapsed = collapsedFiles.has(fileKey); + const isUserCollapsed = collapsedFiles.has(fileKey); + const isEffectivelyCollapsed = isUserCollapsed || !canRender; return ( <div @@ - if (!clickedHeader) return; + if (!clickedHeader || !canRender) return; event.stopPropagation(); toggleFileCollapsed(fileKey); }} > @@ - collapsed: isCollapsed || !canRender, + collapsed: isEffectivelyCollapsed, @@ - transform: isCollapsed ? "rotate(-90deg)" : "rotate(0deg)", + transform: isEffectivelyCollapsed + ? "rotate(-90deg)" + : "rotate(0deg)",Also applies to: 1133-1138, 1144-1148
🤖 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 `@apps/web/src/components/DiffPanel.tsx` around lines 1080 - 1114, The collapse UI uses isCollapsed but ignores the canRender flag in some places, so calculate a single effectiveCollapsed value (e.g., const effectiveCollapsed = isCollapsed || !canRender) inside the renderableFiles map and use that everywhere: pass effectiveCollapsed to the FileDiff collapsed prop, use effectiveCollapsed when rendering the chevron/expand indicator, and guard toggleFileCollapsed so it only runs when canRender is true (do not toggle when !canRender). Update references to isCollapsed in the click handler, chevron rendering, and any other places (e.g., where you check or display expandability) to use effectiveCollapsed to keep behavior consistent.
🤖 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 `@apps/web/src/components/DiffPanel.tsx`:
- Around line 1080-1114: The collapse UI uses isCollapsed but ignores the
canRender flag in some places, so calculate a single effectiveCollapsed value
(e.g., const effectiveCollapsed = isCollapsed || !canRender) inside the
renderableFiles map and use that everywhere: pass effectiveCollapsed to the
FileDiff collapsed prop, use effectiveCollapsed when rendering the
chevron/expand indicator, and guard toggleFileCollapsed so it only runs when
canRender is true (do not toggle when !canRender). Update references to
isCollapsed in the click handler, chevron rendering, and any other places (e.g.,
where you check or display expandability) to use effectiveCollapsed to keep
behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7131f3b-587c-403c-9b83-8b7636c372f3
📒 Files selected for processing (3)
apps/web/src/components/DiffPanel.tsxapps/web/src/lib/diffRendering.test.tsapps/web/src/lib/diffRendering.ts
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
Verification
bun run --cwd apps/web test src/lib/providerDiscoveryReactQuery.test.ts src/components/DiffPanel.logic.test.ts src/lib/diffRendering.test.tsbun run --cwd apps/server test src/stream/collectUint8StreamText.test.ts src/terminal/Layers/Manager.test.tsSummary by CodeRabbit
New Features
Bug Fixes
Tests