✨ Enhanced Diff Management with decorators #692
Conversation
WalkthroughReplaces the legacy diff-view and virtual-storage flows with a streaming "vertical diff" subsystem. Adds diff models and converters (vscode/src/diff/types.ts, diff/myers.ts), static-diff streaming adapter (staticDiffAdapter.ts), file/IDE helpers (ideUtils.ts, pathUtils.ts), decorations and handler/manager for vertical diffs (diff/vertical/*), a CodeLens provider, new VS Code commands for show/accept/reject/clear diffs and block-level actions, auto-accept-on-save config and status bar, and webview actions to drive the decorator-based workflow. Removes diffView, virtualStorage, many diff-related commands, and multiple legacy ModifiedFile UI components and exports. Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
ibolton336
force-pushed
the
decorators-3
branch
2 times, most recently
from
8166a64 to
d743288
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (35)
webview-ui/src/index.css (1)
155-156: New PF global warning token added — looks good; consider using it consistentlyThe token definition is fine. To maximize value, consider replacing hard-coded warning/orange usages in the UI (e.g., “pending” states) with this token for consistency and easy theming.
vscode/src/utilities/ideUtils.ts (2)
8-17: Prefer TextDecoder over Buffer for decoding; better portability and BOM handlingUsing TextDecoder avoids Node-specific Buffer coupling and handles UTF-8 cleanly in modern VS Code hosts.
Apply this diff:
- const content = await vscode.workspace.fs.readFile(uri); - return Buffer.from(content).toString("utf8"); + const content = await vscode.workspace.fs.readFile(uri); + const decoder = new TextDecoder("utf-8"); + return decoder.decode(content);
30-39: Open file UX optionsConsider exposing optional arguments to control viewColumn and preview behavior to avoid hijacking the current editor tab unintentionally.
Possible extension:
-export async function openFile(filepath: string): Promise<void> { +export async function openFile( + filepath: string, + options?: { viewColumn?: vscode.ViewColumn; preview?: boolean } +): Promise<void> { try { const uri = vscode.Uri.file(filepath); const document = await vscode.workspace.openTextDocument(uri); - await vscode.window.showTextDocument(document); + await vscode.window.showTextDocument(document, { + viewColumn: options?.viewColumn, + preview: options?.preview ?? false, + });webview-ui/src/components/ResolutionsPage/ModifiedFile/modifiedFileMessage.css (5)
130-153: Primary “view with decorations” button — consistent with PF tokensGood use of PF primary tokens and explicit min-width for emphasis. Consider adding focus-visible styles to improve keyboard accessibility parity.
Example:
.view-with-decorations-button:focus { box-shadow: 0 0 0 2px var(--pf-global--primary-color--100); } +.view-with-decorations-button:focus-visible { + outline: 2px solid var(--pf-global--primary-color--200); + outline-offset: 2px; +}
212-217: Duplicate “force icon visibility” rules — consolidate to reduce override noiseTwo separate blocks force opacity/visibility (and then later add display). Merge them into one to avoid stacking specificity and future maintenance issues.
Apply this diff to keep a single consolidated block:
-.modified-file-actions .pf-v6-c-button svg, -.modified-file-actions .pf-v6-c-button .pf-v6-c-button__icon { - opacity: 1 !important; - visibility: visible !important; -} - -/* Ensure all SVG icons are visible in PatternFly v6 buttons */ -.modified-file-actions .pf-v6-c-button svg { - opacity: 1 !important; - visibility: visible !important; - display: inline-block !important; -} +/* Ensure all SVG icons are visible in PatternFly v6 buttons */ +.modified-file-actions .pf-v6-c-button svg, +.modified-file-actions .pf-v6-c-button .pf-v6-c-button__icon { + opacity: 1 !important; + visibility: visible !important; + display: inline-block !important; +}Also applies to: 285-289
219-238: Icon color overrides for button variants — consider using currentColorInstead of hard-coding to editor foreground, you could set svg { fill: currentColor } and drive color via the button’s color, reducing special-casing across variants. PF v6 typically respects currentColor for icons.
Example:
-.modified-file-actions .pf-v6-c-button.pf-m-primary svg { - color: var(--vscode-editor-foreground) !important; - fill: var(--vscode-editor-foreground) !important; -} +.modified-file-actions .pf-v6-c-button.pf-m-primary svg { + fill: currentColor !important; +}Then set the button’s color as appropriate for each variant and theme, letting icons inherit automatically.
291-325: Diff status banner — align warning/pending with new PF warning tokenBanner styling looks good. If you plan to represent “pending” states here, consider leveraging the newly added --pf-global--warning-color--100 for borders and accents.
130-153: Adopt the new PF warning token for “pending” states across this componentTo reduce hex scatter and improve theming, replace hard-coded #ffa500 with var(--pf-global--warning-color--100) (defined in index.css). This affects several pending-state rules not in the changed hunk; proposed patch below:
Additional patch (outside the selected lines):
-.hunk-item.hunk-pending { - border: 2px solid #ffa500; - border-left: 2px solid #ffa500 !important; - background-color: rgba(255, 165, 0, 0.05); - box-shadow: 0 2px 8px rgba(255, 165, 0, 0.2); +.hunk-item.hunk-pending { + border: 2px solid var(--pf-global--warning-color--100); + border-left: 2px solid var(--pf-global--warning-color--100) !important; + background-color: color-mix(in srgb, var(--pf-global--warning-color--100) 12%, transparent); + box-shadow: 0 2px 8px color-mix(in srgb, var(--pf-global--warning-color--100) 20%, transparent); } -.hunk-item.hunk-pending::before { +.hunk-item.hunk-pending::before { content: "?"; position: absolute; top: -8px; right: -8px; width: 20px; height: 20px; - background-color: #ffa500; + background-color: var(--pf-global--warning-color--100); color: white; border-radius: 50%; display: flex; align-items: center; justify-content: center; font-size: 12px; font-weight: bold; z-index: 10; animation: hunkPending 0.5s ease-in-out; } -.hunk-item.hunk-pending .hunk-item-header { - border-bottom-color: #ffa500; -} +.hunk-item.hunk-pending .hunk-item-header { + border-bottom-color: var(--pf-global--warning-color--100); +} -.hunk-status-badge.hunk-status-pending { - background-color: #ffa500; +.hunk-status-badge.hunk-status-pending { + background-color: var(--pf-global--warning-color--100); color: white; - border: 1px solid #ffa500; + border: 1px solid var(--pf-global--warning-color--100); }Note: color-mix is broadly supported in modern Chromium, but if you need to avoid it for compatibility, keep rgba() variants derived from the token.
vscode/src/diff/types.ts (1)
5-38: Type surface is clear and cohesiveThe DiffType/Line/Char and IDE interfaces look solid and straightforward. As a future enhancement, consider a discriminated union for ApplyState to avoid many optional fields and enforce state-specific shape at compile time.
Example (no immediate change required):
export type ApplyState = | { status: "streaming"; numDiffs?: number; filepath?: string; streamId: string; toolCallId?: string } | { status: "done"; numDiffs?: number; fileContent?: string; filepath?: string } | { status: "closed"; filepath?: string; streamId?: string; toolCallId?: string };vscode/src/diff/myers.ts (2)
31-41: Whitespace-only changes are being collapsed — verify intent or make configurableYou’re merging consecutive old/new pairs when their lines are equal after trimming. This hides whitespace-only edits from the diff and prevents them from being accepted/applied downstream. If that’s not strictly intended, gate it behind an option so callers can opt in.
Proposed change (preserves current behavior by default):
-export function myersDiff(oldContent: string, newContent: string): DiffLine[] { +export function myersDiff( + oldContent: string, + newContent: string, + options: { ignoreWhitespaceOnly?: boolean } = { ignoreWhitespaceOnly: true }, +): DiffLine[] { @@ - // Combine consecutive old/new pairs that are identical after trimming - for (let i = 0; i < ourFormat.length - 1; i++) { - if ( - ourFormat[i]?.type === "old" && - ourFormat[i + 1]?.type === "new" && - ourFormat[i].line.trim() === ourFormat[i + 1].line.trim() - ) { - ourFormat[i] = { type: "same", line: ourFormat[i].line }; - ourFormat.splice(i + 1, 1); - } - } + if (options.ignoreWhitespaceOnly) { + // Combine consecutive old/new pairs that are identical after trimming + for (let i = 0; i < ourFormat.length - 1; i++) { + if ( + ourFormat[i]?.type === "old" && + ourFormat[i + 1]?.type === "new" && + ourFormat[i].line.trim() === ourFormat[i + 1].line.trim() + ) { + // Prefer the "new" line when collapsing to preserve latest whitespace + ourFormat[i] = { type: "same", line: ourFormat[i + 1].line }; + ourFormat.splice(i + 1, 1); + i--; // Re-check current index after splice to catch chains + } + } + }Note: When collapsing, consider preferring the “new” line content so accepted diffs preserve the new whitespace.
55-206: Char indices use UTF-16 code units — verify consumer expectationsdiffChars and string.length operate in UTF-16 code units. If any consumer maps indices to editor columns or grapheme clusters (e.g., emojis, combined glyphs), this can misalign. If you need grapheme awareness, iterate with Intl.Segmenter or a grapheme splitter. Otherwise, document that indices are UTF-16-based.
webview-ui/src/components/ResolutionsPage/ModifiedFile/modifiedFileActions.css (4)
57-60: Duplicate selector for .pf-v6-c-tooltip__content; consolidate to a single blockYou define .pf-v6-c-tooltip__content twice. Combine them to reduce CSS duplication and potential drift.
Apply this consolidation:
.pf-v6-c-tooltip__content { - background-color: var(--vscode-editor-background) !important; - color: var(--vscode-editor-foreground) !important; -} - -/* Fix tooltip text alignment */ -.pf-v6-c-tooltip__content { - text-align: left !important; + background-color: var(--vscode-editor-background) !important; + color: var(--vscode-editor-foreground) !important; + text-align: left !important; }Also applies to: 81-83
21-24: Potential specificity/override confusion for .continue-button margin-leftTwo rules set margin-left on .continue-button, with different values. The more specific .diff-status-banner .continue-button will win inside the banner, but the global .continue-button may surprise in other contexts. Consider scoping the border and spacing consistently, or drop margin-left from the global rule if not needed elsewhere.
Also applies to: 45-48
31-40: Minimum widths are helpful; consider responsive adjustmentsFixed min-widths ensure stable layouts, but can squeeze in narrow panels. If users resize the webview, consider responsive breakpoints or allowing buttons to wrap.
31-40: Prefer VS Code theme vars over raw rgba where possibleHard-coded rgba(255, 255, 255, …) might not contrast well in some themes. Where feasible, use VS Code theme variables for backgrounds and borders to maintain accessibility across themes.
Also applies to: 50-78
vscode/src/diff/vertical/decorations.ts (6)
31-40: Theme hard-coding: prefer theme colors over literal RGBAindexDecorationType and belowIndexDecorationType use hard-coded rgba whites. Consider theme keys (e.g., editor.lineHighlightBackground, editor.selectionBackground) to ensure readability across themes.
58-74: Range merging logic is good; minor iteration guardThe contiguous range merge in addLines is solid. Consider early-returning after setDecorations to keep the method side-effect surface minimal, but functionally this is fine.
142-150: Avoid calling setDecorations in a loop; batch apply for performanceapplyDecorations sets decorations per range/type in a loop, which can be expensive. If you refactor to a single decoration type with DecorationOptions[], you can call setDecorations once per apply cycle.
80-92: Verbose console logging — gate behind a debug flag/loggerThe repeated console.log calls will spam the extension host logs during streaming. Consider using your existing logger with a debug level and/or a feature flag.
Also applies to: 116-136, 160-167
169-184: deleteRangesStartingAt sequential scan — OK, but consider binary search if ranges growCurrent linear scan is fine for small/medium lists. If removed ranges can be large and sorted, a binary search would reduce time complexity. Optional at this stage.
3-20: Use a single shared DecorationType with per-range renderOptions; drop unsupported CSS hacks
- Per-line calls to
vscode.window.createTextEditorDecorationTypeare expensive and increase GC pressure. Define one sharedremovedLineDecorationTypein vscode/src/diff/vertical/decorations.ts:const removedLineDecorationType = vscode.window.createTextEditorDecorationType({ isWholeLine: true, backgroundColor: { id: "diffEditor.removedLineBackground" }, outlineWidth: "1px", outlineStyle: "solid", outlineColor: { id: "diffEditor.removedTextBorder" }, rangeBehavior: vscode.DecorationRangeBehavior.ClosedClosed, });- Collect each removed line into a
DecorationOptionsarray and set per-range ghost text via:const options: DecorationOptions[] = removedLines.map(line => ({ range: /* your range */, renderOptions: { after: { contentText: line, color: "#808080" } } })); editor.setDecorations(removedLineDecorationType, options);- VS Code’s
textDecorationonly maps to the CSStext-decorationproperty; arbitrary rules likedisplay: noneorwhite-space: preare unsupported hacks and may break. To preserve whitespace or hide text reliably, use:
- Non-breaking spaces (
\u00A0) for spacing- Supported props:
margin,opacity,fontStyle, etc.- The InlineCompletion / injected text API for ghosted text
webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts (1)
46-106: Type guards are permissive — consider tightening if inputs varyisModifiedFileMessageValue only checks for a string path, which is fine for leniency. If payloads start varying more, consider validating messageToken/content/diff presence to avoid constructing partial states. Optional.
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (3)
68-71: Avoid treating an intentionally empty selection as "no selection"selectedContent || content will ignore an empty string selection. Use nullish coalescing.
- const contentToApply = selectedContent || content; + const contentToApply = selectedContent ?? content;
258-268: Clean up props passed to ModifiedFileActions
- You pass both onView and onViewWithDecorations; the former triggers an obsolete message path. Prefer removing onView to avoid confusion.
- isFileApplied being wired to isViewingDiff is semantically odd; if the intent is “show Continue once viewing started,” consider renaming the prop in ModifiedFileActions to reflect that. Otherwise, pass a true "applied" state.
<ModifiedFileActions actionTaken={actionTaken} mode={mode} normalizedData={normalizedData} onApply={() => applyFile()} onReject={rejectFile} - onView={viewFileInVSCode} onViewWithDecorations={viewFileWithDecorations} onQuickResponse={handleQuickResponse} - isFileApplied={isViewingDiff} + isFileApplied={actionTaken === "applied"} onContinue={handleContinue} />
173-176: Address lint warnings: remove stray blank linesStatic analysis flagged extra blank lines.
- - - -And:
- -Also applies to: 271-274
vscode/src/diff/verticalDiffCodeLens.ts (2)
24-37: Compute totals once and reuse in top-level CodeLensesMinor readability/perf nit: avoid repeating reduce.
- // Add accept/reject all buttons at the top of the file - const topRange = new vscode.Range(0, 0, 0, 0); + // Add accept/reject all buttons at the top of the file + const topRange = new vscode.Range(0, 0, 0, 0); + const totalGreen = blocks.reduce((sum, b) => sum + b.numGreen, 0); + const totalRed = blocks.reduce((sum, b) => sum + b.numRed, 0); codeLenses.push( new vscode.CodeLens(topRange, { - title: `✓ Accept All Changes (${blocks.reduce((sum, b) => sum + b.numGreen, 0)}+, ${blocks.reduce((sum, b) => sum + b.numRed, 0)}-)`, + title: `✓ Accept All Changes (${totalGreen}+, ${totalRed}-)`, command: "konveyor.acceptDiff", arguments: [document.uri.fsPath], }), new vscode.CodeLens(topRange, { - title: "✗ Reject All Changes", + title: `✗ Reject All Changes`, command: "konveyor.rejectDiff", arguments: [document.uri.fsPath], }), );
24-37: Unify identifier type (URI vs fsPath) across commands for consistencyTop-level actions pass fsPath; per-block actions pass fileUri. The commands handle both, but normalizing to one form improves consistency and reduces conversions.
Also applies to: 43-52
vscode/src/extension.ts (1)
687-690: Stale comment mentions "merge editor"The disposal comment still references "merge editor". Update to reflect the vertical diff system to avoid confusion.
- // Removed decorator disposal since we're using merge editor now + // Vertical diff system handles its own cleanup; no decorator disposal needed herevscode/src/commands.ts (3)
788-820: content parameter is unused in showDiffWithDecorationsYou accept content from the webview but don't use it. Either remove it from the command signature and upstream message, or annotate its planned use to avoid confusion.
- "konveyor.showDiffWithDecorations": async ( - filePath: string, - diff: string, - content: string, - messageToken: string, - ) => { + "konveyor.showDiffWithDecorations": async ( + filePath: string, + diff: string, + _content: string, // unused + messageToken: string, + ) => {Optionally follow up by removing content from the message payload if not required.
990-1009: Scope CodeLens provider to file scheme to reduce overheadRegistering with "*" applies to everything (including untitled, virtual docs). Restrict to scheme: "file" to avoid unnecessary provider calls.
- state.extensionContext.subscriptions.push( - vscode.languages.registerCodeLensProvider("*", verticalCodeLensProvider), - ); + state.extensionContext.subscriptions.push( + vscode.languages.registerCodeLensProvider({ scheme: "file" }, verticalCodeLensProvider), + );
130-133: Minor nit: update message label for restart solution server errorsThis path logs correctly; consider aligning user-facing messages with logger phrasing for consistency in future edits. No changes required.
vscode/src/diff/staticDiffAdapter.ts (2)
20-25: Parsing multi-file unified diffs without filtering to filePath may merge unrelated hunks.parsePatch can return multiple “files” (hunks) from a unified diff. This adapter ignores filenames and processes all hunks, which will corrupt the mapping if unifiedDiff contains more than one file. Consider filtering by old/new filename that corresponds to filePath, or require/validate single-file diffs.
Would you like me to refactor parseUnifiedDiffToDiffLines to accept filePath and filter hunks accordingly?
166-177: Delegate accept/reject-all to the manager’s public API (ensures consistent behavior).Calling clearForfileUri directly here bypasses the higher-level logic in VerticalDiffManager (and clearForfileUri isn’t async-aware). Prefer using the manager’s acceptAll/rejectAll which operate per file and keep state in sync.
Apply these diffs:
async acceptAll(filePath: string): Promise<void> { const fileUri = vscode.Uri.file(filePath).toString(); - this.verticalDiffManager.clearForfileUri(fileUri, true); + await this.verticalDiffManager.acceptAll(fileUri); }async rejectAll(filePath: string): Promise<void> { const fileUri = vscode.Uri.file(filePath).toString(); - this.verticalDiffManager.clearForfileUri(fileUri, false); + await this.verticalDiffManager.rejectAll(fileUri); }webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx (2)
51-53: Remove unsupported Icon prop; use styling instead.PatternFly’s Icon doesn’t support a status prop. This prop is likely ignored and may trigger TS complaints depending on types. Style the icon via className or inline styles instead.
Apply this diff:
- <Icon status="warning"> + <Icon className="pf-v5-u-warning-color-100"> <ExclamationTriangleIcon color="#b98412" /> </Icon>Or drop the wrapper and style the ExclamationTriangleIcon directly with a className.
178-230: Prop type drift: remove unused onApply/onReject from QuickResponseButtons.The QuickResponseButtons props include onApply/onReject but they are not used. Trim to avoid confusion.
Apply this diff:
const QuickResponseButtons: React.FC<{ quickResponses: Array<{ id: string; content: string }>; isNew: boolean; mode: "agent" | "non-agent"; actionTaken: "applied" | "rejected" | null; onView: () => void; onQuickResponse: (responseId: string) => void; - onApply: () => void; - onReject: () => void; -}> = ({ quickResponses, isNew, mode, actionTaken, onView, onQuickResponse }) => ( +}> = ({ quickResponses, isNew, mode, actionTaken, onView, onQuickResponse }) => (And remove the now-nonexistent props from the callsite (lines 274-276).
- onApply={onApply} - onReject={onReject}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
vscode/src/commands.ts(5 hunks)vscode/src/diff/myers.ts(1 hunks)vscode/src/diff/staticDiffAdapter.ts(1 hunks)vscode/src/diff/types.ts(1 hunks)vscode/src/diff/vertical/decorations.ts(1 hunks)vscode/src/diff/vertical/handler.ts(1 hunks)vscode/src/diff/vertical/manager.ts(1 hunks)vscode/src/diff/verticalDiffCodeLens.ts(1 hunks)vscode/src/extension.ts(6 hunks)vscode/src/extensionState.ts(3 hunks)vscode/src/utilities/configuration.ts(1 hunks)vscode/src/utilities/ideUtils.ts(1 hunks)vscode/src/webviewMessageHandler.ts(2 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx(6 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx(5 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/index.ts(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/modifiedFileActions.css(1 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/modifiedFileMessage.css(3 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts(1 hunks)webview-ui/src/index.css(1 hunks)
💤 Files with no reviewable changes (1)
- webview-ui/src/components/ResolutionsPage/ModifiedFile/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-01T13:53:20.774Z
Learnt from: djzager
PR: konveyor/editor-extensions#645
File: vscode/src/webviewMessageHandler.ts:575-577
Timestamp: 2025-08-01T13:53:20.774Z
Learning: The TOGGLE_AGENT_MODE action handler in vscode/src/webviewMessageHandler.ts correctly calls toggleAgentMode() without parameters, as the toggleAgentMode function in vscode/src/utilities/configuration.ts has been updated to internally handle reading and toggling the agent mode configuration value.
Applied to files:
vscode/src/webviewMessageHandler.ts
🧬 Code Graph Analysis (12)
vscode/src/utilities/ideUtils.ts (1)
webview-ui/src/utils/vscode.ts (1)
vscode(18-18)
vscode/src/diff/myers.ts (1)
vscode/src/diff/types.ts (2)
DiffLine(7-10)DiffChar(12-21)
vscode/src/diff/staticDiffAdapter.ts (3)
vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(12-298)vscode/src/diff/types.ts (1)
DiffLine(7-10)vscode/src/diff/vertical/handler.ts (1)
fileUri(84-86)
vscode/src/diff/vertical/decorations.ts (1)
vscode/src/diff/vertical/handler.ts (1)
range(74-78)
vscode/src/diff/verticalDiffCodeLens.ts (2)
vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(12-298)vscode/src/diff/vertical/handler.ts (2)
fileUri(84-86)range(74-78)
vscode/src/extensionState.ts (2)
vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(12-298)vscode/src/diff/staticDiffAdapter.ts (1)
StaticDiffAdapter(10-186)
vscode/src/commands.ts (4)
vscode/src/client/tracer.ts (1)
logger(60-65)webview-ui/src/utils/vscode.ts (1)
vscode(18-18)vscode/src/diff/vertical/handler.ts (1)
fileUri(84-86)vscode/src/diff/verticalDiffCodeLens.ts (1)
VerticalDiffCodeLensProvider(4-58)
vscode/src/extension.ts (6)
vscode/src/extensionState.ts (1)
ExtensionState(21-57)vscode/src/utilities/configuration.ts (1)
getConfigAutoAcceptOnSave(70-71)vscode/src/diff/vertical/handler.ts (1)
fileUri(84-86)vscode/src/utilities/ideUtils.ts (1)
SimpleIDE(52-57)vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(12-298)vscode/src/diff/staticDiffAdapter.ts (1)
StaticDiffAdapter(10-186)
vscode/src/diff/vertical/manager.ts (2)
vscode/src/diff/vertical/handler.ts (3)
fileUri(84-86)VerticalDiffHandler(27-582)VerticalDiffHandlerOptions(15-25)vscode/src/diff/types.ts (2)
DiffLine(7-10)IDE(33-38)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (4)
vscode/webpack.config.js (1)
mode(8-8)webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts (1)
isLocalChange(110-110)shared/src/types/types.ts (1)
LocalChange(99-108)vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts (1)
handleQuickResponse(8-140)
vscode/src/webviewMessageHandler.ts (2)
vscode/src/client/tracer.ts (1)
logger(60-65)webview-ui/src/utils/vscode.ts (1)
vscode(18-18)
vscode/src/diff/vertical/handler.ts (4)
vscode/src/diff/types.ts (2)
ApplyState(23-30)DiffLine(7-10)vscode/src/diff/vertical/decorations.ts (4)
RemovedLineDecorationManager(107-185)AddedLineDecorationManager(47-102)indexDecorationType(31-35)belowIndexDecorationType(36-40)vscode/src/diff/vertical/manager.ts (1)
VerticalDiffCodeLens(6-10)vscode/src/diff/myers.ts (1)
myersDiff(25-53)
🪛 GitHub Check: Lint
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
[warning] 173-173:
Delete ⏎⏎⏎⏎
[warning] 271-271:
Delete ⏎⏎
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
🔇 Additional comments (28)
vscode/src/utilities/ideUtils.ts (2)
52-57: SimpleIDE wiring looks goodThe thin wrapper class cleanly exposes the utilities with no surprising state or this-binding issues.
41-47: No downstream consumers ofgetCurrentFile()—OS path return is safeA repository-wide search found only:
vscode/src/utilities/ideUtils.ts(implementation)vscode/src/diff/types.ts(interface declaration)No call sites normalize or parse URIs (e.g. via
Uri.file(),Uri.parse(), or literalfile://), and there are no other usages ofgetCurrentFile(). Returning the platform’s native file path will have no impact on downstream code.webview-ui/src/components/ResolutionsPage/ModifiedFile/modifiedFileMessage.css (3)
87-87: Button width reductions are sensibleShrinking main Accept/Reject min-width from 140px to 120px improves layout density without harming readability.
Also applies to: 111-111
175-195: Dark theme: global PF v6 icon recolor — verify intended scopeThese selectors target all PF v6 buttons across the entire webview in dark mode, not just ModifiedFile. That’s a broad override and may unintentionally recolor icons in unrelated components.
If the intent is to scope to the ModifiedFile component, consider tightening selectors:
-.vscode-dark .pf-v6-c-button svg, -.vscode-dark .pf-v6-c-button .pf-v6-c-button__icon svg { +.vscode-dark .modified-file-actions .pf-v6-c-button svg, +.vscode-dark .modified-file-actions .pf-v6-c-button .pf-v6-c-button__icon svg { color: var(--vscode-editor-foreground) !important; fill: var(--vscode-editor-foreground) !important; } -.vscode-dark .pf-v6-c-button:hover svg, -.vscode-dark .pf-v6-c-button:hover .pf-v6-c-button__icon svg { +.vscode-dark .modified-file-actions .pf-v6-c-button:hover svg, +.vscode-dark .modified-file-actions .pf-v6-c-button:hover .pf-v6-c-button__icon svg { color: var(--vscode-editor-foreground) !important; fill: var(--vscode-editor-foreground) !important; }
240-260: Accept/Reject icon colors — good specificity and theme alignmentClear intent and specificity ensure these rules win over the broader icon overrides; using VS Code gutter colors keeps parity with the editor.
vscode/src/diff/myers.ts (2)
5-15: Good utility: clean conversion from jsdiff Change to DiffLineThe trailing-EOF handling and type derivation are sound. Splitting by newline and trimming a terminal empty element aligns with your ignoreNewlineAtEof decision.
43-50: Dropping trailing empty ‘old’ lines — confirm there’s no downstream relianceThis normalization removes trailing empty deletions. If a consumer expects to visualize or apply removal of trailing blank lines, this will hide them. Please confirm with the vertical diff handler/apply flow.
webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts (1)
61-66: Robust guard for quickResponses — niceThe array and length checks prevent passing empty or malformed quick responses to the UI. Good defensive coding.
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (3)
130-155: LGTM: new SHOW_DIFF_WITH_DECORATORS flow with re-entry guardThe isViewingDiff guard prevents duplicate posts, and the payload matches the host handler. Looks good.
157-171: LGTM: handleContinue aligns with new flowThis mirrors apply behavior and properly resets UI state.
103-128: Replace legacy SHOW_MAXIMIZED_DIFF with SHOW_DIFF_WITH_DECORATORSWe confirmed via
rgthat the only occurrence of"SHOW_MAXIMIZED_DIFF"is in:
- webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (lines ~117–121)
Apply this diff:
- // Agent mode: Use SHOW_MAXIMIZED_DIFF message - interface ShowMaximizedDiffPayload { - path: string; - content: string; - diff: string; - messageToken: string; - } - const payload: ShowMaximizedDiffPayload = { - path: filePath, - content: content, - diff: fileDiff, - messageToken: messageToken, - }; - window.vscode.postMessage({ - type: "SHOW_MAXIMIZED_DIFF", - payload, - }); + // Agent mode: Use SHOW_DIFF_WITH_DECORATORS message + interface ShowDiffWithDecoratorsPayload { + path: string; + content: string; + diff: string; + messageToken: string; + } + const payload: ShowDiffWithDecoratorsPayload = { + path: filePath, + content, + diff: fileDiff, + messageToken, + }; + window.vscode.postMessage({ + type: "SHOW_DIFF_WITH_DECORATORS", + payload, + });• Verify that the host application supports the new
SHOW_DIFF_WITH_DECORATORSevent and remove any leftover legacy handling ofSHOW_MAXIMIZED_DIFF.vscode/src/diff/verticalDiffCodeLens.ts (1)
15-21: LGTM: empty state handlingReturning [] when no blocks exist is correct and avoids unnecessary lenses.
vscode/src/webviewMessageHandler.ts (2)
234-250: LGTM: New SHOW_DIFF_WITH_DECORATORS action and error handlingThe command invocation, logging, and user-facing error message are all appropriate. The payload matches the webview sender.
490-493: LGTM: Normalization aligns with diff processingReplacing CRLF and trimming a single trailing newline is a sensible normalization for content comparison.
vscode/src/extension.ts (4)
218-220: LGTM: Vertical diff initialization early in startupInitializing the vertical diff system up-front is appropriate given downstream dependencies.
327-345: Auto-accept-on-save behavior: confirm intended default and UXAuto-accept-on-save is gated behind getConfigAutoAcceptOnSave() (default appears to be true). Confirm this default is intentional; auto-accepting all blocks on any save could surprise users.
Would you like to flip the default to false and prompt users to enable it, or surface a one-time notification when auto-accept first triggers?
436-451: LGTM: Diff status bar setupGood use of command binding and event listeners to maintain status.
453-473: LGTM: Status bar updates computed from active file blocksAccurately derives totals and visibility per active editor.
vscode/src/commands.ts (4)
822-837: LGTM: Accept all diffs commandGuards against uninitialized adapter and handles active-editor fallback.
838-852: LGTM: Reject all diffs commandSame as accept; good validation and messaging.
854-880: LGTM: Per-block accept/reject commandsURI-to-fsPath conversion and adapter invocation are correct. Error handling is appropriate.
882-899: LGTM: Clear diff decorations commandCleans a single file or all active diffs; safe-guards around manager presence look good.
vscode/src/diff/staticDiffAdapter.ts (1)
140-155: Good: clean integration point and diagnostics around streaming.Using messageToken as a streamId and logging the active editor and counts will help trace decorated-diff sessions.
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx (2)
84-176: Solid action model and disabling logic.
- “Review Changes” primary CTA for agent mode with isViewingDiff/actionTaken guards is clear.
- Accept/Reject All hidden during diff viewing reduces confusion.
- Proper ARIA labels.
232-296: Nice UX: contextual banner and “Continue” flow in agent mode.The banner succinctly instructs users on how to proceed in-editor and exposes a Continue action for completing the flow.
vscode/src/diff/vertical/manager.ts (1)
183-251: Overall: good orchestration of streaming lifecycle and UI context.
- Properly sets konveyor.diffVisible and konveyor.streamingDiff.
- Re-enables user change listener after streaming; disables it on errors.
- Captures logDiffs for diagnostics.
vscode/src/diff/vertical/handler.ts (2)
295-385: Thoughtful reapplication with Myers diff and decoration rebuild.The approach to reject pending blocks, replace the target range deterministically, and then reconstruct CodeLens blocks is sound. Handling trailing newline semantics avoids off-by-one issues.
208-254: Block accept/reject shifting looks correct; keep skipStatusUpdate in acceptAll/rejectAll callers.Offset calculation uses -(accept ? numRed : numGreen), which matches line count change. Remember to pass skipStatusUpdate when bulk-applying to avoid noisy intermediate status.
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
webview-ui/src/components/ResolutionsPage/resolutionsPage.css (4)
65-70: Prevent truncation/overflow: avoid forcing no-wrap and use gap for spacing
flex-wrap: nowrap !important;on the title risks overflow on narrow viewports (especially with long titles). Usinggapon the container preserves layout without relying on a margin on the indicator and allows the title to wrap when needed.Apply this minimal change (paired with the previous comment’s selector scoping if you accept it):
.pf-v6-c-page__main-section .pf-v6-c-title { display: flex !important; align-items: center !important; - flex-wrap: nowrap !important; + gap: var(--pf-v6-global--spacer--sm); min-width: 0 !important; }Optional (if you need single-line with truncation): add ellipsis handling to the title text node/wrapper.
/* Example if title text is wrapped in .title-text */ .resolutions-page .pf-v6-c-title .title-text { min-width: 0; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; flex: 1 1 auto; }
72-76: Prefer container gap over ad-hoc margins; remove fixed 10px spacingThe margin on
.loading-indicatorbecomes unnecessary once the container usesgap. It also hard-codes LTR spacing. Removing it prevents RTL issues and keeps spacing consistent with PF tokens.Apply:
.pf-v6-c-page__main-section .pf-v6-c-title .loading-indicator { display: inline-flex !important; - margin-left: 10px !important; flex-shrink: 0 !important; }If you keep margin instead of container
gap, at least use logical properties and PF spacers:- margin-left: 10px !important; + margin-inline-start: var(--pf-v6-global--spacer--sm) !important;
79-91: Reduce heavy-handed overrides in the responsive block; also scope to pageForcing
visibility: visibleandopacity: 1globally on PF titles can override legitimate hidden states and utility classes. If the goal is to counter a specific PF utility that hides the header on small screens, scope to the page and only assertdisplay.Apply:
@media (max-width: 768px) { - .pf-v6-c-page__main-section .pf-v6-c-title { - display: flex !important; - visibility: visible !important; - opacity: 1 !important; - } + .resolutions-page .pf-v6-c-title { + display: flex !important; + } - .pf-v6-c-page__main-section .pf-v6-c-title .loading-indicator { - display: inline-flex !important; - visibility: visible !important; - opacity: 1 !important; - } + .resolutions-page .pf-v6-c-title .loading-indicator { + display: inline-flex !important; + } }If elements are hidden via PF responsive utility classes, consider removing those utilities from the markup on this page rather than overriding them in CSS.
65-76: Alignment nit: baseline often reads better for text + spinner
align-items: centeris fine, but aligning tobaselinecan visually integrate the spinner with the text on various font sizes.Consider:
-.resolutions-page .pf-v6-c-title { - display: flex !important; - align-items: center !important; +.resolutions-page .pf-v6-c-title { + display: flex !important; + align-items: baseline !important; gap: var(--pf-v6-global--spacer--sm); min-width: 0 !important; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
vscode/src/diff/vertical/manager.ts(1 hunks)webview-ui/src/components/ResolutionsPage/resolutionsPage.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vscode/src/diff/vertical/manager.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
ibolton336
force-pushed
the
decorators-3
branch
from
835efa4 to
a68623d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
vscode/src/diff/vertical/manager.ts (6)
27-36: Make createVerticalDiffHandler async and await the prior handler’s clear to avoid racesThe previous handler’s async clear is not awaited. This can interleave document edits/decoration teardown with new handler setup.
Apply:
- createVerticalDiffHandler( + async createVerticalDiffHandler( fileUri: string, startLine: number, endLine: number, options: VerticalDiffHandlerOptions, - ): VerticalDiffHandler | undefined { - if (this.fileUriToHandler.has(fileUri)) { - this.fileUriToHandler.get(fileUri)?.clear(false); - this.fileUriToHandler.delete(fileUri); - } + ): Promise<VerticalDiffHandler | undefined> { + const existing = this.fileUriToHandler.get(fileUri); + if (existing) { + await existing.clear(false); + this.fileUriToHandler.delete(fileUri); + }
205-211: Remove redundant pre-clear; avoid double-clear and race with new handler setupYou clear here and again inside createVerticalDiffHandler. Keep one clear and await it in a single place (the factory).
- // Clear any existing handler - const existingHandler = this.getHandlerForFile(fileUri); - if (existingHandler) { - console.log("[Manager] Clearing existing handler"); - await existingHandler.clear(false); - }
156-178: Make clearForFileUri async and await handler.clear before cleanup; fix casingWithout awaiting, you can delete state and toggle UI context before the handler finishes tearing down decorations/edits.
- clearForfileUri(fileUri: string | undefined, accept: boolean = false) { + async clearForFileUri(fileUri: string | undefined, accept = false): Promise<void> { if (!fileUri) { return; } const handler = this.fileUriToHandler.get(fileUri); if (handler) { - handler.clear(accept); + await handler.clear(accept); this.fileUriToHandler.delete(fileUri); } this.disableDocumentChangeListener(); this.fileUriToCodeLens.delete(fileUri); this.refreshCodeLens(); void vscode.commands.executeCommand("setContext", "konveyor.diffVisible", false); // Notify status change if (this.onDiffStatusChange) { this.onDiffStatusChange(fileUri); } }Run to locate remaining references to the old name and update call sites to
await this.clearForFileUri(...):#!/bin/bash rg -n -C2 --type=ts 'clearForfileUri|createVerticalDiffHandler\s*\('
253-274: Accept-all should consume fresh state until empty; then await clearIterating a snapshot while state mutates can miss or misaddress blocks. Loop until empty and await the async clear.
- const handler = this.fileUriToHandler.get(fileUri); - if (handler) { - // Accept all blocks - const blocks = this.fileUriToCodeLens.get(fileUri); - if (blocks) { - for (const block of blocks) { - await handler.acceptRejectBlock(true, block.start, block.numGreen, block.numRed); - } - } - this.clearForfileUri(fileUri, true); - } + const handler = this.fileUriToHandler.get(fileUri); + if (!handler) return; + // Accept blocks until none remain (state mutates per operation) + let blocks = this.fileUriToCodeLens.get(fileUri); + while (blocks && blocks.length > 0) { + const { start, numGreen, numRed } = blocks[0]; + await handler.acceptRejectBlock(true, start, numGreen, numRed); + blocks = this.fileUriToCodeLens.get(fileUri); + } + await this.clearForFileUri(fileUri, true);
276-297: Mirror the accept-all fix for reject-all; loop until empty and await clearAvoid stale indices and ensure proper teardown.
- const handler = this.fileUriToHandler.get(fileUri); - if (handler) { - // Reject all blocks - const blocks = this.fileUriToCodeLens.get(fileUri); - if (blocks) { - for (const block of blocks) { - await handler.acceptRejectBlock(false, block.start, block.numGreen, block.numRed); - } - } - this.clearForfileUri(fileUri, false); - } + const handler = this.fileUriToHandler.get(fileUri); + if (!handler) return; + // Reject blocks until none remain (state mutates per operation) + let blocks = this.fileUriToCodeLens.get(fileUri); + while (blocks && blocks.length > 0) { + const { start, numGreen, numRed } = blocks[0]; + await handler.acceptRejectBlock(false, start, numGreen, numRed); + blocks = this.fileUriToCodeLens.get(fileUri); + } + await this.clearForFileUri(fileUri, false);
215-229: Await the now-async createVerticalDiffHandlerUpdate the call site to ensure prior clear completes before constructing the new handler.
- const diffHandler = this.createVerticalDiffHandler(fileUri, startLine, endLine, { + const diffHandler = await this.createVerticalDiffHandler(fileUri, startLine, endLine, {
🧹 Nitpick comments (3)
vscode/src/diff/vertical/manager.ts (3)
63-80: Signature nit: this listener creator doesn’t return a DisposableThe method doesn’t return the created disposable. Make the signature void to match behavior.
- private enableDocumentChangeListener(): vscode.Disposable | undefined { + private enableDocumentChangeListener(): void { if (this.userChangeListener) { //Only create one listener per file return; }
37-39: Simplify equality check; string compare is sufficient hereURI.equal is unnecessary for toString() equality.
- if (editor && URI.equal(editor.document.uri.toString(), fileUri)) { + if (editor && editor.document.uri.toString() === fileUri) {
212-214: Drop the arbitrary 150ms sleep; rely on awaited clear for UI stabilityOnce you await handler.clear, the extra delay adds latency without guarantees. Prefer event-driven gating or no delay.
- // Small delay to ensure UI updates - await new Promise((resolve) => setTimeout(resolve, 150)); + // No artificial delay needed once clear is awaited
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
vscode/src/diff/vertical/manager.ts(1 hunks)webview-ui/src/components/ResolutionsPage/resolutionsPage.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webview-ui/src/components/ResolutionsPage/resolutionsPage.css
🧰 Additional context used
🧬 Code Graph Analysis (1)
vscode/src/diff/vertical/manager.ts (2)
vscode/src/diff/vertical/handler.ts (3)
fileUri(84-86)VerticalDiffHandler(27-582)VerticalDiffHandlerOptions(15-25)vscode/src/diff/types.ts (2)
DiffLine(7-10)IDE(33-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
ibolton336
force-pushed
the
decorators-3
branch
2 times, most recently
from
d6e90fe to
4c0b212
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
vscode/src/extension.ts (1)
72-76: Status bar item should be disposedThe StatusBarItem is created but never added to the extension subscriptions, which means it won't be automatically disposed when the extension deactivates.
this.diffStatusBarItem = vscode.window.createStatusBarItem( vscode.StatusBarAlignment.Right, 100, ); + this.context.subscriptions.push(this.diffStatusBarItem);vscode/src/diff/vertical/manager.ts (2)
145-146: Await clearForFileUri to ensure proper cleanupThe
clearForFileUricall should be awaited to ensure the handler cleanup completes before proceeding.if (blocks.length === 1) { - this.clearForFileUri(fileUri, true); + await this.clearForFileUri(fileUri, true);
31-57: Clear handler asynchronously to prevent race conditionsThe handler clear operation at line 38 is not awaited, which could lead to race conditions where the old handler's cleanup operations interleave with the new handler creation.
- createVerticalDiffHandler( + async createVerticalDiffHandler( fileUri: string, startLine: number, endLine: number, options: VerticalDiffHandlerOptions, - ): VerticalDiffHandler | undefined { + ): Promise<VerticalDiffHandler | undefined> { if (this.fileUriToHandler.has(fileUri)) { - this.fileUriToHandler.get(fileUri)?.clear(false); + const existingHandler = this.fileUriToHandler.get(fileUri); + if (existingHandler) { + await existingHandler.clear(false); + } this.fileUriToHandler.delete(fileUri); }Then update the call site at line 221:
- const diffHandler = this.createVerticalDiffHandler(fileUri, startLine, endLine, { + const diffHandler = await this.createVerticalDiffHandler(fileUri, startLine, endLine, {
🧹 Nitpick comments (7)
vscode/src/commands.ts (4)
550-555: Fix the indentation consistency in the filter objectThe "Executable Files" and "All Files" properties have inconsistent indentation compared to the rest of the object structure.
filters: isWindows ? { - "Executable Files": ["exe"], - "All Files": ["*"], - } + "Executable Files": ["exe"], + "All Files": ["*"], + } : { - "All Files": ["*"], - }, + "All Files": ["*"], + },
788-819: Ensure proper error handling throughout the diff application flowThe error handling provides user feedback, but there are a few considerations:
- The error message could be more specific - consider distinguishing between initialization errors and diff application errors
- The function swallows errors after displaying them, which could mask issues in the calling code
Consider propagating the error after user notification to allow calling code to handle it appropriately:
} catch (error) { logger.error("Error in vertical diff:", error); - vscode.window.showErrorMessage(`Failed to show diff: ${error}`); + const errorMessage = error instanceof Error ? error.message : String(error); + if (!state.staticDiffAdapter) { + vscode.window.showErrorMessage(`Diff system not initialized: ${errorMessage}`); + } else { + vscode.window.showErrorMessage(`Failed to apply diff: ${errorMessage}`); + } + throw error; // Re-throw to allow proper error propagation }
822-836: Consider adding try-catch error handling for consistencyWhile the undefined checks are good, consider wrapping the acceptAll call in try-catch for consistency with other command handlers.
"konveyor.acceptDiff": async (filePath?: string) => { if (!filePath) { const editor = vscode.window.activeTextEditor; if (!editor) { return; } filePath = editor.document.fileName; } if (!state.staticDiffAdapter) { vscode.window.showErrorMessage("Vertical diff system not initialized"); return; } - await state.staticDiffAdapter.acceptAll(filePath); - vscode.window.showInformationMessage("Changes accepted"); + try { + await state.staticDiffAdapter.acceptAll(filePath); + vscode.window.showInformationMessage("Changes accepted"); + } catch (error) { + logger.error("Error accepting diff changes:", error); + vscode.window.showErrorMessage(`Failed to accept changes: ${error}`); + } },
838-852: Apply consistent error handling patternSimilar to the acceptDiff command, add error handling for consistency.
"konveyor.rejectDiff": async (filePath?: string) => { if (!filePath) { const editor = vscode.window.activeTextEditor; if (!editor) { return; } filePath = editor.document.fileName; } if (!state.staticDiffAdapter) { vscode.window.showErrorMessage("Vertical diff system not initialized"); return; } - await state.staticDiffAdapter.rejectAll(filePath); - vscode.window.showInformationMessage("Changes rejected"); + try { + await state.staticDiffAdapter.rejectAll(filePath); + vscode.window.showInformationMessage("Changes rejected"); + } catch (error) { + logger.error("Error rejecting diff changes:", error); + vscode.window.showErrorMessage(`Failed to reject changes: ${error}`); + } },vscode/src/extension.ts (1)
447-462: Consider adding the status bar item to subscriptions in setupDiffStatusBarWhile the status bar item will be disposed if added to subscriptions in the constructor, it would be cleaner to handle its lifecycle in the same method where it's configured.
private setupDiffStatusBar(): void { this.diffStatusBarItem.name = "Konveyor Diff Status"; this.diffStatusBarItem.tooltip = "Click to accept/reject all diff changes"; this.diffStatusBarItem.command = "konveyor.showDiffActions"; this.diffStatusBarItem.hide(); // Update status bar when active editor changes this.listeners.push( vscode.window.onDidChangeActiveTextEditor((editor) => { this.updateDiffStatusBar(editor); }), ); // Initial update this.updateDiffStatusBar(vscode.window.activeTextEditor); }vscode/src/diff/vertical/manager.ts (2)
69-72: Return type mismatch in enableDocumentChangeListenerThe method signature indicates it should return
vscode.Disposable | undefined, but when a listener already exists, it returnsvoid(implicit undefined from the empty return statement).private enableDocumentChangeListener(): vscode.Disposable | undefined { if (this.userChangeListener) { //Only create one listener per file - return; + return undefined; }
209-214: Remove redundant handler clearingThe existing handler is already cleared here, but
createVerticalDiffHandlerwill also clear it (once made async per earlier comment), resulting in redundant operations.- // Clear any existing handler - const existingHandler = this.getHandlerForFile(fileUri); - if (existingHandler) { - console.log("[Manager] Clearing existing handler"); - await existingHandler.clear(false); - }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
vscode/src/commands.ts(5 hunks)vscode/src/diff/staticDiffAdapter.ts(1 hunks)vscode/src/diff/vertical/manager.ts(1 hunks)vscode/src/extension.ts(7 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/resolutionsPage.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- webview-ui/src/components/ResolutionsPage/resolutionsPage.css
- vscode/src/diff/staticDiffAdapter.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T12:10:46.399Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#581
File: vscode/src/extension.ts:243-251
Timestamp: 2025-07-23T12:10:46.399Z
Learning: In vscode/src/extension.ts, clearing all configErrors in the onDidSaveTextDocument listener for settings YAML is intentional behavior. The setupModelProvider and updateConfigErrors functions are designed to work together to rebuild the entire configuration error state.
Applied to files:
vscode/src/extension.ts
🧬 Code Graph Analysis (3)
vscode/src/commands.ts (3)
vscode/src/client/tracer.ts (1)
logger(60-65)vscode/src/diff/vertical/handler.ts (1)
fileUri(84-86)vscode/src/diff/verticalDiffCodeLens.ts (1)
VerticalDiffCodeLensProvider(4-58)
vscode/src/extension.ts (5)
vscode/src/extensionState.ts (1)
ExtensionState(21-57)vscode/src/utilities/configuration.ts (1)
getConfigAutoAcceptOnSave(70-71)vscode/src/utilities/ideUtils.ts (1)
SimpleIDE(52-57)vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(13-315)vscode/src/diff/staticDiffAdapter.ts (1)
StaticDiffAdapter(10-167)
vscode/src/diff/vertical/manager.ts (3)
vscode/src/diff/vertical/handler.ts (3)
fileUri(84-86)VerticalDiffHandler(27-582)VerticalDiffHandlerOptions(15-25)vscode/src/diff/types.ts (2)
DiffLine(7-10)IDE(33-38)agentic/src/cache.ts (1)
InMemoryCacheWithRevisions(129-186)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
🔇 Additional comments (7)
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (1)
403-409: Verification complete – CSS hooked correctly, no code/test dependencies found
- CSS selectors in
webview-ui/src/components/ResolutionsPage/resolutionsPage.cssreference.resolutions-page, confirming the style hook is applied.- No references to
.resolutions-pagewere found in TS/JS source or in unit/e2e tests—no further updates required.✅ Approved for merge.
vscode/src/commands.ts (1)
990-1010: LGTM! Well-structured CodeLens provider registrationThe CodeLens provider registration is properly implemented with:
- Appropriate null checks for the vertical diff manager
- Registration to extension subscriptions for cleanup
- Proper refresh callback connection
- Clear logging for debugging
vscode/src/extension.ts (2)
330-355: Robust implementation of auto-accept on save featureThe implementation correctly:
- Checks the configuration setting before processing
- Verifies that a handler exists for the file
- Provides user feedback via information messages
- Handles errors with proper logging and user notification
- Executes BEFORE the save operation to ensure the document is saved in its final state
430-445: Well-designed vertical diff initializationThe initialization properly:
- Creates the SimpleIDE abstraction for file operations
- Initializes the VerticalDiffManager with required dependencies
- Sets up the diff status change callback for UI updates
- Creates the StaticDiffAdapter for handling unified diffs
- Includes appropriate logging
vscode/src/diff/vertical/manager.ts (3)
160-182: LGTM! Proper async implementation of clearForFileUriThe method correctly awaits the handler's clear operation before proceeding with cleanup, preventing race conditions between document edits and state cleanup.
283-291: LGTM! Proper handling of concurrent mutationsGood use of
slice()to create a shallow copy of the blocks array before iteration. This prevents issues whenacceptRejectBlockmutates the original array during processing.
305-313: LGTM! Consistent handling pattern with acceptAllThe
rejectAllmethod correctly mirrors theacceptAllimplementation with proper array copying to avoid mutation issues.
ibolton336
force-pushed
the
decorators-3
branch
2 times, most recently
from
e86d633 to
55aa122
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (1)
32-40: Fix initial actionTaken mapping: map "discarded" to "rejected" (not "rejected").LocalChange.state is "pending" | "applied" | "discarded". Initialize actionTaken by mapping "discarded" → "rejected" and keep "applied" as-is. Optionally also tolerate a legacy "rejected" string if present.
- const [actionTaken, setActionTaken] = useState<"applied" | "rejected" | null>(() => { - if (status === "applied") { - return "applied"; - } - if (status === "rejected") { - return "rejected"; - } - return null; // for "pending" or undefined - }); + const [actionTaken, setActionTaken] = useState<"applied" | "rejected" | null>(() => { + if (status === "applied") return "applied"; + if (status === "discarded" || status === "rejected") return "rejected"; + return null; // for "pending" or undefined + });vscode/src/diff/vertical/handler.ts (1)
483-494: Fix potential out-of-bounds error when deleting ranges at end of document.The method constructs an end position at
range.end.line + 1, which can exceed document bounds when the range ends at the last line.Apply this diff to fix the bounds issue:
private async deleteRangeLines(ranges: vscode.Range[]) { await this.editor.edit( (editBuilder) => { for (const range of ranges) { + const docLineCount = this.editor.document.lineCount; + const safeEndLine = Math.min(range.end.line + 1, docLineCount); editBuilder.delete( - new vscode.Range(range.start, new vscode.Position(range.end.line + 1, 0)), + new vscode.Range(range.start, new vscode.Position(safeEndLine, 0)), ); } }, { undoStopAfter: false, undoStopBefore: false }, ); }
🧹 Nitpick comments (3)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (3)
138-164: Harden decorated-view handler: guard non-agent mode and align payload types.
- Add an early return for non-agent mode to avoid posting a message the host may not handle, and delegate to onView when available.
- The comment says “Prevent multiple applications”; it should say “Prevent multiple view requests”.
- If messageToken and content can be undefined (common for LocalChange), mark them optional in the payload type or assert their presence before posting.
- const viewFileWithDecorations = (filePath: string, fileDiff: string) => { - // Prevent multiple applications + const viewFileWithDecorations = (filePath: string, fileDiff: string) => { + // In non-agent mode, fall back to standard view behavior + if (mode !== "agent") { + if (onView && isLocalChange(data)) onView(data); + return; + } + + // Prevent multiple concurrent view requests if (isViewingDiff) { return; } setIsViewingDiff(true); // Use the reliable vertical diff system - interface ShowDiffWithDecoratorsPayload { - path: string; - content: string; - diff: string; - messageToken: string; - } + interface ShowDiffWithDecoratorsPayload { + path: string; + diff: string; + content?: string; + messageToken?: string; + } const payload: ShowDiffWithDecoratorsPayload = { path: filePath, content: content, diff: fileDiff, messageToken: messageToken, }; window.vscode.postMessage({ type: "SHOW_DIFF_WITH_DECORATORS", payload, }); };If the host strictly requires messageToken/content, alternatively add runtime guards to ensure they exist before posting and log/return otherwise.
165-179: DRY: reuse applyFile in handleContinue.handleContinue duplicates applyFile logic. Delegate to applyFile() to keep behavior consistent and reduce maintenance.
- const handleContinue = () => { - // Mark as applied to continue the conversation flow - if (mode === "agent") { - postFileResponse("apply", messageToken, path, content); - // Trigger scroll after action in agent mode - onUserAction?.(); - } else { - if (onApply && isLocalChange(data)) { - const modifiedChange: LocalChange = { ...data, content }; - onApply(modifiedChange); - } - } - setActionTaken("applied"); - setIsViewingDiff(false); - }; + const handleContinue = () => applyFile();
272-276: Verify prop semantics: isFileApplied receives isViewingDiff.Passing isViewingDiff into isFileApplied is potentially misleading. If the child uses this to gate a “Continue” action while a decorated view is active, consider renaming the prop to isViewingDiff (or isDiffActive) for clarity, or pass a correctly named boolean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
vscode/package.json(2 hunks)vscode/src/commands.ts(5 hunks)vscode/src/diff/staticDiffAdapter.ts(1 hunks)vscode/src/diff/vertical/handler.ts(1 hunks)vscode/src/diff/vertical/manager.ts(1 hunks)vscode/src/extension.ts(9 hunks)vscode/src/extensionState.ts(3 hunks)vscode/src/utilities/configuration.ts(1 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx(5 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- vscode/src/utilities/configuration.ts
- webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts
- vscode/src/commands.ts
- vscode/src/diff/staticDiffAdapter.ts
- vscode/src/extensionState.ts
- vscode/src/diff/vertical/manager.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T12:10:46.399Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#581
File: vscode/src/extension.ts:243-251
Timestamp: 2025-07-23T12:10:46.399Z
Learning: In vscode/src/extension.ts, clearing all configErrors in the onDidSaveTextDocument listener for settings YAML is intentional behavior. The setupModelProvider and updateConfigErrors functions are designed to work together to rebuild the entire configuration error state.
Applied to files:
vscode/src/extension.ts
🧬 Code Graph Analysis (3)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (2)
webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts (1)
isLocalChange(110-110)shared/src/types/types.ts (1)
LocalChange(99-108)
vscode/src/diff/vertical/handler.ts (4)
vscode/src/diff/types.ts (2)
ApplyState(23-30)DiffLine(7-10)vscode/src/diff/vertical/decorations.ts (4)
RemovedLineDecorationManager(107-185)AddedLineDecorationManager(47-102)belowIndexDecorationType(36-40)indexDecorationType(31-35)vscode/src/diff/vertical/manager.ts (2)
VerticalDiffCodeLens(7-11)clearForFileUri(160-182)vscode/src/diff/myers.ts (1)
myersDiff(25-53)
vscode/src/extension.ts (5)
vscode/src/extensionState.ts (1)
ExtensionState(21-57)vscode/src/utilities/configuration.ts (1)
getConfigAutoAcceptOnSave(70-71)vscode/src/utilities/ideUtils.ts (1)
SimpleIDE(52-57)vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(13-328)vscode/src/diff/staticDiffAdapter.ts (1)
StaticDiffAdapter(10-162)
🔇 Additional comments (16)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (4)
41-41: LGTM: separate diff-view state is appropriate.This state cleanly gates the decorated view workflow and prevents re-entrancy.
74-74: LGTM: reset diff view after apply.Closing the decorated view on apply avoids stale UI.
96-96: LGTM: reset diff view after reject.Consistent with apply; prevents lingering decorated view.
189-189: LGTM: reset diff view on quick responses.Keeps UI coherent when using quick actions.
vscode/src/diff/vertical/handler.ts (6)
87-120: LGTM! Well-implemented clear method with proper cleanup and notifications.The method correctly handles both accept and reject cases, properly clears decorations, updates status, and handles disposal. The user feedback through status updates and diff status change notifications is comprehensive.
126-160: Robust queue processing implementation with proper locking mechanism.The queue-based approach with lock protection prevents race conditions when processing diff lines. The method correctly handles cases where the editor is not active, queuing items for later processing.
162-205: LGTM! Comprehensive run method with proper error handling.The async generator consumption pattern is well-implemented. The method includes proper cancellation checks, handles the deletion buffer, applies Myers diff for final reconciliation, and includes appropriate error recovery with rollback.
294-384: Well-structured Myers diff reapplication with correct line preservation logic.The method properly resets the initial diff state, reconstructs old/new content, handles trailing newlines correctly to prevent line count discrepancies, and rebuilds code lens blocks. The 100ms delay comment explains the necessity for UI synchronization.
386-440: Insertion buffer mechanism correctly handles deferred deletions.The method properly manages the deletion buffer for visual diff blocks, correctly shifts decorations, and updates code lens state. The logic for handling empty buffers and inserting placeholder lines is sound.
506-527: Clean and straightforward diff line processing logic.The switch statement correctly handles each diff type with appropriate buffer management and line index updates.
vscode/package.json (1)
543-549: LGTM! Well-configured auto-accept-on-save setting.The new configuration option is properly defined with a sensible default (true), clear description, and appropriate ordering.
vscode/src/extension.ts (5)
72-75: LGTM! Status bar item properly registered for disposal.The status bar item is correctly created and added to context.subscriptions (line 280), ensuring proper cleanup on extension deactivation.
315-358: Well-implemented auto-accept-on-save feature with proper error handling.The implementation correctly:
- Checks if auto-accept is enabled before processing
- Verifies if the file has active diffs before attempting to accept
- Provides user feedback for both success and failure cases
- Handles errors gracefully with appropriate logging
432-447: LGTM! Clean initialization of vertical diff system.The initialization properly creates the IDE wrapper, sets up the managers with appropriate dependencies, configures the status change callback, and provides clear logging.
449-485: Well-structured diff status bar setup with comprehensive updates.The status bar implementation correctly:
- Sets up meaningful tooltip and command associations
- Updates on active editor changes
- Shows diff counts (green/red) when diffs are present
- Hides when no diffs exist
701-719: Robust cleanup in deactivation with proper error handling.The deactivation properly disposes of the vertical diff manager, clears the static diff adapter reference, and includes comprehensive error handling with try-catch-finally to ensure cleanup completes even if errors occur.
ibolton336
force-pushed
the
decorators-3
branch
from
11d16cc to
c01a83c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
vscode/src/diff/vertical/handler.ts (1)
449-461: Fix empty-file append: avoid document.lineAt(-1) when lineCount is 0insertTextAboveLine() dereferences lineAt(lineCount - 1) even when the document is empty, which will throw. Handle lineCount === 0 by inserting at Position(0, 0) and omitting the leading newline.
private async insertTextAboveLine(index: number, text: string) { await this.editor.edit( (editBuilder) => { const lineCount = this.editor.document.lineCount; - if (index >= lineCount) { - // Append to end of file - editBuilder.insert( - new vscode.Position(lineCount, this.editor.document.lineAt(lineCount - 1).text.length), - `\n${text}`, - ); - } else { - editBuilder.insert(new vscode.Position(index, 0), `${text}\n`); - } + if (index >= lineCount) { + // Append to end of file + if (lineCount === 0) { + editBuilder.insert(new vscode.Position(0, 0), text); + } else { + const lastLineLen = this.editor.document.lineAt(lineCount - 1).text.length; + editBuilder.insert(new vscode.Position(lineCount, lastLineLen), `\n${text}`); + } + } else { + editBuilder.insert(new vscode.Position(index, 0), `${text}\n`); + } }, { undoStopAfter: false, undoStopBefore: false }, ); }
🧹 Nitpick comments (6)
vscode/src/diff/vertical/handler.ts (6)
473-481: Clamp delete end position in deleteLinesAt to mirror deleteRangeLines safetydeleteLinesAt() computes a Range to line index + numLines without clamping. If callers ever pass a value that reaches beyond the buffer (e.g., at EOF), this may throw. Align with deleteRangeLines() by clamping to document.lineCount.
private async deleteLinesAt(index: number, numLines = 1) { - const startLine = new vscode.Position(index, 0); + const startLine = new vscode.Position(index, 0); + const safeEndLine = Math.min(index + numLines, this.editor.document.lineCount); await this.editor.edit( (editBuilder) => { - editBuilder.delete(new vscode.Range(startLine, startLine.translate(numLines))); + editBuilder.delete(new vscode.Range(startLine, new vscode.Position(safeEndLine, 0))); }, { undoStopAfter: false, undoStopBefore: false }, ); }
330-334: Avoid double trailing newline when Myers output ends with an 'old' sentinelreplaceContent appends a trailing newline when the original content ends with one. Because old lines are materialized as empty strings to reserve vertical space, body can already end with a newline (e.g., when the last diff item is old). Guard against double-EOL to prevent an extra blank line at the block boundary.
Please verify with a case where the last hunk ends with deletions-only.
- const replaceContent = - myersDiffs.map((diff) => (diff.type === "old" ? "" : diff.line)).join("\n") + - (originalContentEndsWithNewline ? "\n" : ""); + const body = myersDiffs.map((diff) => (diff.type === "old" ? "" : diff.line)).join("\n"); + // If body already ends with a newline (e.g., last item was 'old'), don't add another. + const replaceContent = body.endsWith("\n") + ? body + : body + (originalContentEndsWithNewline ? "\n" : "");
386-441: Off-by-one in placeholder insertion for deletion-only end-of-streaminsertDeletionBuffer inserts "\n".repeat(this.deletionBuffer.length - 1). This intentionally holds back one line to keep the cursor aligned during streaming. However, if the stream ends with only deletions (no following "same" or "new"), the final flush will still insert only N-1 placeholders for N deletions, temporarily undercounting placeholders until Myers reapply. If that's intentional, add a short comment; otherwise, consider inserting all N at end-of-stream.
Consider adding a clarifying comment:
- // Insert the block of deleted lines as empty new lines + // Insert the block of deleted lines as empty new lines. + // We intentionally insert (N - 1) to keep the current line aligned while streaming. + // Myers reapply will normalize the final shape at the end of the stream.
505-526: Queue-time edits are robust; minor logging noise could be gatedQueueing and handling of diff lines is solid. The console logs here are verbose and will flood the devtools console during large diffs. Consider gating logs behind a debug flag or VS Code setting.
495-504: Index progress decorations are effectively disabledupdateIndexLineDecorations() immediately returns. Since it's called from multiple places as a visual progress cue, either wire it to a real decoration (as per decorations.ts) or remove the calls to avoid confusion.
324-343: Replace range might exclude trailing EOL; consider replacing through end-of-line to simplify EOL handlingCurrently, replace() targets this.range which ends at (endLine, MAX_SAFE_INTEGER). This excludes the newline after endLine. You’re compensating with originalContentEndsWithNewline checks. An alternative is to replace through the start of endLine+1 (clamped), then omit special-casing EOLs. This can reduce edge cases around double EOLs.
Example:
const doc = this.editor.document; const safeEndLine = Math.min(this.endLine + 1, doc.lineCount); const replaceRange = new vscode.Range(this.startLine, 0, safeEndLine, 0); // build `replaceContent` without appending a trailing newline
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
vscode/src/diff/vertical/handler.ts(1 hunks)vscode/src/diff/vertical/manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vscode/src/diff/vertical/manager.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
vscode/src/diff/vertical/handler.ts (4)
vscode/src/diff/types.ts (2)
ApplyState(23-30)DiffLine(7-10)vscode/src/diff/vertical/decorations.ts (4)
RemovedLineDecorationManager(107-185)AddedLineDecorationManager(47-102)belowIndexDecorationType(36-40)indexDecorationType(31-35)vscode/src/diff/vertical/manager.ts (2)
VerticalDiffCodeLens(7-11)clearForFileUri(160-182)vscode/src/diff/myers.ts (1)
myersDiff(25-53)
🔇 Additional comments (2)
vscode/src/diff/vertical/handler.ts (2)
528-541: CodeLens shift skips the active block by design; ensure removal of the active block always happensshiftCodeLensObjects() filters out x.start === startLine (the block being accepted/rejected). Make sure acceptRejectBlock() always removes the corresponding block before any other shifting operations to avoid orphaned entries if start values drift in edge cases. Looks correct as written; leaving a note to double-check manager->provider expectations.
122-160: Queue drain on editor activation looks goodThe lock-and-poke pattern using queueDiffLine(undefined) upon editor activation is a good approach to resume processing without race conditions. No changes needed.
ibolton336
force-pushed
the
decorators-3
branch
from
efb0014 to
754000f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
vscode/src/utilities/pathUtils.ts (2)
1-1: Optional: Prefer node:url import for built-in URL utilitiesUsing "node:url" makes it explicit that this is a Node built-in and helps bundlers avoid pulling browser polyfills.
-import { fileURLToPath } from "url"; +import { fileURLToPath } from "node:url";
3-8: Make file URL detection more robust and browser-safe; improve Windows “/C:/” handling
- Accept any “file:” scheme variant (e.g., file:/, file://, mixed-case) via URL parsing instead of a fragile string prefix check.
- Gracefully handle malformed URLs (avoid throwing; fall back to plain path).
- Guard against process being undefined in non-Node/browser contexts.
- Handle “/C:” with or without a trailing slash and both separators ([\/]).
-export function fileUriToPath(path: string): string { - const cleanPath = path.startsWith("file://") ? fileURLToPath(path) : path; - return process.platform === "win32" && cleanPath.match(/^\/[A-Za-z]:\//) - ? cleanPath.substring(1) - : cleanPath; -} +export function fileUriToPath(input: string): string { + // Try to interpret as a file URL first; fall back to plain path on failure. + let cleanPath = input; + if (typeof input === "string" && input.length >= 5 && input.slice(0, 5).toLowerCase() === "file:") { + try { + const url = new URL(input); + if (url.protocol === "file:") { + cleanPath = fileURLToPath(url); + } + } catch { + // Not a valid URL; treat as a plain path. + } + } + const isWin = typeof process !== "undefined" && process.platform === "win32"; + if (isWin && /^\/[A-Za-z]:(?:[\\/]|$)/.test(cleanPath)) { + return cleanPath.substring(1); + } + return cleanPath; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
vscode/src/commands.ts(5 hunks)vscode/src/diff/vertical/manager.ts(1 hunks)vscode/src/diff/verticalDiffCodeLens.ts(1 hunks)vscode/src/utilities/pathUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- vscode/src/commands.ts
- vscode/src/diff/verticalDiffCodeLens.ts
- vscode/src/diff/vertical/manager.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
vscode/src/extension.ts (2)
315-358: Auto-accept on save must use event.waitUntil; settings.yaml should be handled onDidSave (reads stale file otherwise)
- The onWillSave listener currently awaits asynchronous work directly, which does not delay the save. VS Code only guarantees pre-save edits if you call event.waitUntil with a Thenable.
- The comment claims acceptance happens “BEFORE saving,” but without waitUntil this is not guaranteed.
- Running setupModelProvider onWillSave reads settings.yaml from disk before the editor flushes the updated content; this regresses the previously intentional onDidSave behavior and can validate stale config.
Apply this refactor to make the save path correct and deterministic:
- vscode.workspace.onWillSaveTextDocument(async (event) => { - const doc = event.document; - - // Handle settings.yaml configuration changes - if (doc.uri.fsPath === paths().settingsYaml.fsPath) { - const configError = await this.setupModelProvider(paths().settingsYaml); - this.state.mutateData((draft) => { - draft.configErrors = []; - if (configError) { - draft.configErrors.push(configError); - } - updateConfigErrors(draft, paths().settingsYaml.fsPath); - }); - } - - // Auto-accept all diff decorations BEFORE saving (if enabled) - // This ensures the document is saved in its final state - if (getConfigAutoAcceptOnSave() && this.state.verticalDiffManager) { - const fileUri = doc.uri.toString(); - const handler = this.state.verticalDiffManager.getHandlerForFile(fileUri); - if (handler && handler.hasDiffForCurrentFile()) { - try { - // Accept all diffs BEFORE the save operation - // This ensures the document is saved in its final state - await this.state.staticDiffAdapter?.acceptAll(doc.uri.fsPath); - this.state.logger.info( - `Auto-accepted all diff decorations for ${doc.fileName} before save`, - ); - // Show user feedback that diffs were auto-accepted - vscode.window.showInformationMessage( - `Auto-accepted all diff changes for ${doc.fileName} - saving final state`, - ); - } catch (error) { - this.state.logger.error( - `Failed to auto-accept diff decorations before save for ${doc.fileName}:`, - error, - ); - vscode.window.showErrorMessage( - `Failed to auto-accept diff changes for ${doc.fileName}: ${error}`, - ); - } - } - } - }), + vscode.workspace.onWillSaveTextDocument((event) => { + event.waitUntil( + (async () => { + const doc = event.document; + + // Auto-accept all diff decorations BEFORE saving (if enabled) + if (getConfigAutoAcceptOnSave() && this.state.verticalDiffManager) { + const fileUri = doc.uri.toString(); + const handler = this.state.verticalDiffManager.getHandlerForFile(fileUri); + if (handler && handler.hasDiffForCurrentFile()) { + try { + // Ensure acceptance completes before save + await this.state.verticalDiffManager.acceptAll(fileUri); + this.state.logger.info( + `Auto-accepted all diff changes for ${doc.fileName} before save`, + ); + vscode.window.showInformationMessage( + `Auto-accepted all diff changes for ${doc.fileName} - saving final state`, + ); + } catch (error) { + this.state.logger.error( + `Failed to auto-accept diff changes before save for ${doc.fileName}:`, + error, + ); + vscode.window.showErrorMessage( + `Failed to auto-accept diff changes for ${doc.fileName}: ${error}`, + ); + } + } + } + })(), + ); + }),Then, add a separate onDidSave handler for settings.yaml so the new contents are on disk before parsing:
// Place near the other listeners (e.g., after onWillSave) this.listeners.push( vscode.workspace.onDidSaveTextDocument(async (doc) => { if (doc.uri.fsPath === paths().settingsYaml.fsPath) { const configError = await this.setupModelProvider(paths().settingsYaml); this.state.mutateData((draft) => { draft.configErrors = []; if (configError) { draft.configErrors.push(configError); } updateConfigErrors(draft, paths().settingsYaml.fsPath); }); } }), );Optional but recommended: keep adapter behavior consistent by making StaticDiffAdapter.acceptAll/rejectAll call the manager’s acceptAll/rejectAll (see snippet in the next comment).
315-358: Replace StaticDiffAdapter.acceptAll with VerticalDiffManager.acceptAll to ensure diffs are actually appliedStaticDiffAdapter.acceptAll currently delegates to
clearForFileUri(..., true), which only disposes decorations without applying any diff blocks. By contrast,VerticalDiffManager.acceptAllfirst iterates and applies each block before clearing. To guarantee “accept all before save” actually applies every change, please make these updates:• In vscode/src/extension.ts, inside the onWillSaveTextDocument handler:
Replaceawait this.state.staticDiffAdapter?.acceptAll(doc.uri.fsPath);with
await this.state.verticalDiffManager.acceptAll(fileUri);• In vscode/src/diff/staticDiffAdapter.ts, update the adapter to delegate correctly:
export class StaticDiffAdapter { // … - async acceptAll(filePath: string): Promise<void> { - const fileUri = vscode.Uri.file(filePath).toString(); - await this.verticalDiffManager.clearForFileUri(fileUri, true); - } + async acceptAll(filePath: string): Promise<void> { + const fileUri = vscode.Uri.file(filePath).toString(); + await this.verticalDiffManager.acceptAll(fileUri); + } - async rejectAll(filePath: string): Promise<void> { - const fileUri = vscode.Uri.file(filePath).toString(); - await this.verticalDiffManager.clearForFileUri(fileUri, false); - } + async rejectAll(filePath: string): Promise<void> { + const fileUri = vscode.Uri.file(filePath).toString(); + await this.verticalDiffManager.rejectAll(fileUri); + } }These changes restore the intended behavior—applying all diff blocks, not silently dropping them.
🧹 Nitpick comments (3)
vscode/src/extension.ts (3)
72-76: Ensure StatusBarItem is always disposed even if initialize failsYou create the StatusBarItem in the constructor but only push it to context.subscriptions later (Line 280). If initialize throws before Line 280, extension.dispose won’t dispose the StatusBarItem, leaking a resource (even if hidden).
- Push the item into subscriptions immediately after creation.
- Remove the later push to avoid duplication.
@@ this.diffStatusBarItem = vscode.window.createStatusBarItem( vscode.StatusBarAlignment.Right, 100, ); + this.context.subscriptions.push(this.diffStatusBarItem); @@ - this.context.subscriptions.push(this.diffStatusBarItem);Also applies to: 280-281
60-60: Publicly exposing state increases couplingExposing the entire ExtensionState as public can make future refactors harder. Consider exposing a narrower surface (e.g., dedicated methods to update diff status bar, access modelProvider, etc.) and keep state private.
449-454: Nit: optional accessibility label for status bar itemConsider setting accessibilityInformation for better screen reader support, e.g., “Konveyor Diff Status: X additions, Y deletions. Click for actions.”
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
vscode/src/diff/vertical/manager.ts(1 hunks)vscode/src/extension.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vscode/src/diff/vertical/manager.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T12:10:46.399Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#581
File: vscode/src/extension.ts:243-251
Timestamp: 2025-07-23T12:10:46.399Z
Learning: In vscode/src/extension.ts, clearing all configErrors in the onDidSaveTextDocument listener for settings YAML is intentional behavior. The setupModelProvider and updateConfigErrors functions are designed to work together to rebuild the entire configuration error state.
Applied to files:
vscode/src/extension.ts
🧬 Code Graph Analysis (1)
vscode/src/extension.ts (6)
vscode/src/extensionState.ts (1)
ExtensionState(21-57)vscode/src/paths.ts (2)
paths(100-105)ExtensionPaths(8-29)vscode/src/utilities/configuration.ts (1)
getConfigAutoAcceptOnSave(70-71)vscode/src/utilities/ideUtils.ts (1)
SimpleIDE(52-57)vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(14-329)vscode/src/diff/staticDiffAdapter.ts (1)
StaticDiffAdapter(10-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
- GitHub Check: Build (linux)
🔇 Additional comments (4)
vscode/src/extension.ts (4)
432-447: Vertical diff system initialization looks solidInstantiates SimpleIDE, wires VerticalDiffManager and StaticDiffAdapter, and propagates onDiffStatusChange to the status bar updater. This is cohesive and keeps concerns localized.
424-426: Good UX: defers status bar wiring until after initializationDeferring setupDiffStatusBar until after initialization helps avoid UI flicker or stale counts on startup.
466-485: Status bar update logic is correct and efficient
- Hides when no diffs present.
- Sums added/removed across blocks without expensive operations.
- Uses the fileUri-based handler lookup to scope to the active editor.
700-720: Deactivation cleans up vertical diff managers and extension reliably
- Disposes VerticalDiffManager explicitly.
- Clears adapter reference.
- Calls extension.dispose and guards with try/finally.
Looks good.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
vscode/src/diff/vertical/handler.ts (2)
487-497: Good fix: end-of-document clamp prevents out-of-bounds deletes.Clamping the end position to document.lineCount removes the original edge-case when deleting the last decorated line. This aligns with VS Code’s edit API expectations.
126-139: Queue processing preserves order on errors.Good use of a simple lock plus unshift() to re-queue the current item on edit failure (e.g., editor switch), maintaining correct processing order.
Also applies to: 144-158
🧹 Nitpick comments (5)
vscode/src/diff/vertical/handler.ts (5)
2-2: Avoid pulling in uri-js; compare URIs with strings (or vscode.Uri) directly.Using uri-js just for equality adds an unnecessary dependency in the extension host. String equality on toString() is sufficient here.
-import * as URI from "uri-js";- if (URI.equal(editor.document.uri.toString(), this.fileUri)) { + if (editor.document.uri.toString() === this.fileUri) { this.editor = editor; this.removedLineDecorations.applyToNewEditor(editor); this.addedLineDecorations.applyToNewEditor(editor); this.updateIndexLineDecorations(); this.refreshCodeLens(); // Handle any lines received while editor was closed this.queueDiffLine(undefined); }If uri-js is used elsewhere in the PR, we can keep it. Otherwise, consider removing it from dependencies.
Also applies to: 58-68
453-466: Empty-file insertion path yields different trailing-newline semantics.When lineCount === 0, you insert text without appending a newline, while other paths append a newline around the inserted text. This can lead to subtle off-by-one line count differences when inserting deletion buffers vs. single lines into an empty file.
Consider normalizing insertion behavior. One option is to compute the exact insertion string once:
- For index >= lineCount: insertion = "\n" + text
- Else: insertion = text + "\n"
- For empty file: insertion = text + (text.endsWith("\n") ? "" : "\n")
I can draft a precise diff if you want to standardize all branches.
499-507: Index progress decorations disabled – consider removing imports if permanent.updateIndexLineDecorations() is a no-op, but you still import indexDecorationType/belowIndexDecorationType and clear them. If progress will remain disabled, consider removing the unused progress decorations and related imports to reduce noise.
32-32: Remove unused state newLinesAdded.newLinesAdded is incremented but never read. Dead state can confuse future maintainers.
- private newLinesAdded = 0;- this.newLinesAdded++;Also applies to: 471-475
300-304: Reverse mutates the shared blocks array; safe here but brittle.blocks.reverse() mutates the array retrieved from the map. It works because you immediately iterate and then clear decorations, but it does mutate shared state while acceptRejectBlock also updates the map. Safer to iterate a copy in reverse order.
- for (const block of blocks.reverse()) { + for (const block of [...blocks].reverse()) { await this.acceptRejectBlock(false, block.start, block.numGreen, block.numRed, true); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
vscode/src/diff/vertical/handler.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
vscode/src/diff/vertical/handler.ts (4)
vscode/src/diff/types.ts (2)
ApplyState(23-30)DiffLine(7-10)vscode/src/diff/vertical/decorations.ts (4)
RemovedLineDecorationManager(107-185)AddedLineDecorationManager(47-102)belowIndexDecorationType(36-40)indexDecorationType(31-35)vscode/src/diff/vertical/manager.ts (2)
VerticalDiffCodeLens(8-12)clearForFileUri(161-183)vscode/src/diff/myers.ts (1)
myersDiff(25-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
🔇 Additional comments (2)
vscode/src/diff/vertical/handler.ts (2)
312-335: Confirm trailing newline strategy in Myers reapply.Mapping old → "" and appending a trailing newline only when the original range had one maintains vertical alignment (blank placeholders for deletions). This is intentional for vertical diff. Just confirm that the original range (this.range) corresponds to the whole file when intended; otherwise, the trailing newline condition could mismatch the actual file’s final newline.
If this.range is sometimes a subsection, you may want to derive the trailing newline from the target subsection semantics rather than the entire document’s EOL policy.
532-545: Potential duplicate-start removal in shiftCodeLensObjects.Filtering by x.start !== startLine assumes block starts are unique. If blocks can share a start line (edge cases after multiple shifts), this may remove more than intended.
Confirm uniqueness of block starts. If not guaranteed, consider using object identity or an ID to remove only the targeted block.
ibolton336
force-pushed
the
decorators-3
branch
from
8c4fe95 to
bd6796e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
vscode/src/extension.ts (1)
456-471: Register the missing konveyor.showDiffActions commandThe status bar item references
konveyor.showDiffActionscommand which doesn't appear to be registered anywhere.Add the command registration:
private setupDiffStatusBar(): void { this.diffStatusBarItem.name = "Konveyor Diff Status"; this.diffStatusBarItem.tooltip = "Click to accept/reject all diff changes"; - this.diffStatusBarItem.command = "konveyor.showDiffActions"; + this.diffStatusBarItem.command = { + command: "konveyor.showDiffActions", + title: "Show Diff Actions" + }; this.diffStatusBarItem.hide(); + // Register the command + this.context.subscriptions.push( + vscode.commands.registerCommand("konveyor.showDiffActions", async () => { + const editor = vscode.window.activeTextEditor; + if (!editor || !this.state.verticalDiffManager) { + return; + } + + const fileUri = editor.document.uri.toString(); + const handler = this.state.verticalDiffManager.getHandlerForFile(fileUri); + if (!handler || !handler.hasDiffForCurrentFile()) { + return; + } + + const action = await vscode.window.showQuickPick( + ["Accept All Changes", "Reject All Changes", "Cancel"], + { placeHolder: "Choose an action for diff changes" } + ); + + if (action === "Accept All Changes") { + await this.state.staticDiffAdapter?.acceptAll(editor.document.uri.fsPath); + } else if (action === "Reject All Changes") { + await this.state.staticDiffAdapter?.rejectAll(editor.document.uri.fsPath); + } + }) + ); + // Update status bar when active editor changesvscode/src/diff/vertical/manager.ts (1)
96-117: Update handler decorations instead of just loggingThe
handleDocumentChangemethod calculates line deltas but doesn't actually update the handler's decorations, which will cause misaligned diff decorations when users edit the document.Call the handler's
updateLineDeltamethod to properly update decorations:private handleDocumentChange( event: vscode.TextDocumentChangeEvent, - _handler: VerticalDiffHandler, + handler: VerticalDiffHandler, ) { // Loop through each change in the event event.contentChanges.forEach((change) => { // Calculate the number of lines added or removed const linesAdded = change.text.split("\n").length - 1; const linesDeleted = change.range.end.line - change.range.start.line; // Calculate the net change in lines const lineDelta = linesAdded - linesDeleted; // Get the line number where the change occurred const lineNumber = change.range.start.line; - // Update decorations based on the change - // Note: updateDecorations method would need to be implemented in handler - // For now, we'll just log the change - this.logger.debug(`Document change at line ${lineNumber}, delta: ${lineDelta}`); + // Update decorations based on the change + const fileUri = event.document.uri.toString(); + handler.updateLineDelta(fileUri, lineNumber, lineDelta); + this.logger.debug(`Updated decorations for document change at line ${lineNumber}, delta: ${lineDelta}`); }); }vscode/src/diff/vertical/handler.ts (1)
90-101: Process ranges bottom-up to prevent index drift during rejectionWhen rejecting all changes, the sequential processing of ranges can cause index misalignment as earlier deletions shift subsequent line positions.
Process ranges in reverse order to maintain correct line positions:
const removedRanges = this.removedLineDecorations.ranges; if (accept) { // Accept all: delete all the red ranges and clear green decorations await this.deleteRangeLines(removedRanges.map((r) => r.range)); } else { // Reject all: Re-insert red lines, delete green ones - for (const r of removedRanges) { + // Process bottom-up to prevent index drift + const sortedRanges = [...removedRanges].sort((a, b) => b.range.start.line - a.range.start.line); + for (const r of sortedRanges) { await this.deleteRangeLines([r.range]); await this.insertTextAboveLine(r.range.start.line, r.line); } await this.deleteRangeLines(this.addedLineDecorations.ranges); }
🧹 Nitpick comments (2)
vscode/src/diff/staticDiffAdapter.ts (1)
20-85: Handle edge cases in diff parsingThe parsing logic doesn't handle some edge cases that could lead to incorrect diff application:
- Empty hunks or missing hunks could cause misalignment
- Multi-file diffs might not be handled correctly
Add validation and better error handling:
private parseUnifiedDiffToDiffLines(unifiedDiff: string, originalContent: string): DiffLine[] { const diffLines: DiffLine[] = []; const originalLines = originalContent.split("\n"); // Parse the unified diff const parsedDiff = parsePatch(unifiedDiff); if (!parsedDiff || parsedDiff.length === 0) { throw new Error("Failed to parse diff"); } + // Validate single file diff + if (parsedDiff.length > 1) { + this.logger.warn(`Multiple file diffs detected (${parsedDiff.length}), using first file only`); + } + + const fileDiff = parsedDiff[0]; + if (!fileDiff.hunks || fileDiff.hunks.length === 0) { + this.logger.debug("No hunks in diff, returning unchanged content"); + return originalLines.map(line => ({ type: "same", line })); + } + // Track which original lines have been processed let originalLineIndex = 0; - for (const fileDiff of parsedDiff) { - if (!fileDiff.hunks) { - continue; - } - - for (const hunk of fileDiff.hunks) { + for (const hunk of fileDiff.hunks) { // Add unchanged lines before this hunk const hunkStartLine = hunk.oldStart - 1; // Convert to 0-based + + // Validate hunk boundaries + if (hunkStartLine < originalLineIndex) { + this.logger.warn(`Overlapping hunks detected at line ${hunkStartLine}`); + continue; + }vscode/src/diff/vertical/handler.ts (1)
478-485: Add bounds checking for line deletionWhile unlikely in normal operation,
deleteLinesAtcould attempt to delete beyond document bounds if called with largenumLinesvalues.Add defensive bounds checking:
private async deleteLinesAt(index: number, numLines = 1) { const startLine = new vscode.Position(index, 0); + const docLineCount = this.editor.document.lineCount; + const safeNumLines = Math.min(numLines, Math.max(0, docLineCount - index)); + await this.editor.edit( (editBuilder) => { - editBuilder.delete(new vscode.Range(startLine, startLine.translate(numLines))); + editBuilder.delete(new vscode.Range(startLine, startLine.translate(safeNumLines))); }, { undoStopAfter: false, undoStopBefore: false }, ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
vscode/src/commands.ts(5 hunks)vscode/src/diff/staticDiffAdapter.ts(1 hunks)vscode/src/diff/types.ts(1 hunks)vscode/src/diff/vertical/decorations.ts(1 hunks)vscode/src/diff/vertical/handler.ts(1 hunks)vscode/src/diff/vertical/manager.ts(1 hunks)vscode/src/extension.ts(9 hunks)vscode/src/utilities/ideUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- vscode/src/utilities/ideUtils.ts
- vscode/src/diff/vertical/decorations.ts
- vscode/src/commands.ts
- vscode/src/diff/types.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T12:10:46.399Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#581
File: vscode/src/extension.ts:243-251
Timestamp: 2025-07-23T12:10:46.399Z
Learning: In vscode/src/extension.ts, clearing all configErrors in the onDidSaveTextDocument listener for settings YAML is intentional behavior. The setupModelProvider and updateConfigErrors functions are designed to work together to rebuild the entire configuration error state.
Applied to files:
vscode/src/extension.ts
🧬 Code Graph Analysis (4)
vscode/src/diff/vertical/handler.ts (4)
vscode/src/diff/types.ts (2)
ApplyState(23-30)DiffLine(7-10)vscode/src/diff/vertical/decorations.ts (4)
RemovedLineDecorationManager(127-227)AddedLineDecorationManager(47-122)belowIndexDecorationType(36-40)indexDecorationType(31-35)vscode/src/diff/vertical/manager.ts (2)
VerticalDiffCodeLens(9-13)clearForFileUri(163-185)vscode/src/diff/myers.ts (1)
myersDiff(25-53)
vscode/src/diff/staticDiffAdapter.ts (4)
vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(15-331)vscode/src/client/tracer.ts (1)
logger(60-65)vscode/src/diff/types.ts (1)
DiffLine(7-10)vscode/src/diff/vertical/handler.ts (1)
fileUri(83-85)
vscode/src/extension.ts (6)
vscode/src/extensionState.ts (1)
ExtensionState(21-57)vscode/src/utilities/configuration.ts (1)
getConfigAutoAcceptOnSave(70-71)vscode/src/diff/types.ts (1)
FileEditor(33-38)vscode/src/utilities/ideUtils.ts (1)
FileEditor(52-57)vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(15-331)vscode/src/diff/staticDiffAdapter.ts (1)
StaticDiffAdapter(11-166)
vscode/src/diff/vertical/manager.ts (3)
vscode/src/diff/types.ts (2)
DiffLine(7-10)FileEditor(33-38)agentic/src/cache.ts (1)
InMemoryCacheWithRevisions(129-186)vscode/src/utilities/pathUtils.ts (1)
fileUriToPath(3-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
- GitHub Check: Build (linux)
🔇 Additional comments (4)
vscode/src/diff/staticDiffAdapter.ts (1)
100-141: No changes needed—diff placement doesn’t depend on editor.selectionI inspected
VerticalDiffManager.streamDiffLinesinvscode/src/diff/vertical/manager.tsand found that it always applies edits using explicit ranges derived from the parsedDiffLinedata. It never reads or relies oneditor.selection. Therefore, setting a full-document selection before streaming diffs isn’t necessary.vscode/src/diff/vertical/manager.ts (1)
216-228: Cache key format consistency verifiedI searched for all
kaiFsCache.set/.getcalls and any direct URI-based cache keys—only the.set(filePath, …)invscode/src/diff/vertical/manager.tswas found. There are no other cache operations or URI keys, so the current absolute-path key usage is consistent.vscode/src/diff/vertical/handler.ts (2)
454-456: LGTM! Empty file handling fixedGood fix for the empty file edge case by explicitly checking and inserting at Position(0, 0).
491-492: LGTM! Out-of-bounds protection addedGood defensive programming - clamping
endLinetodocument.lineCountprevents potential out-of-bounds errors.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (1)
32-40: Fix type mismatch when initializing actionTaken (discarded → rejected)status can be "pending" | "applied" | "discarded". The initializer checks for "rejected", which is never present, so "discarded" won’t be reflected. Map discarded to rejected.
Apply this diff:
- const [actionTaken, setActionTaken] = useState<"applied" | "rejected" | null>(() => { - if (status === "applied") { - return "applied"; - } - if (status === "rejected") { - return "rejected"; - } - return null; // for "pending" or undefined - }); + const [actionTaken, setActionTaken] = useState<"applied" | "rejected" | null>(() => { + if (status === "applied") return "applied"; + if (status === "discarded") return "rejected"; + return null; // for "pending" or undefined + });vscode/src/extension.ts (1)
316-359: Move auto-accept logic to onDidSave to avoid save racesAsync operations in onWillSaveTextDocument can race with the save. Accepting diff decorations there may yield partial writes and re-entrancy hazards.
Apply this diff to remove the auto-accept from onWillSave (keep the settings.yaml handling):
- // Auto-accept all diff decorations BEFORE saving (if enabled) - // This ensures the document is saved in its final state - if (getConfigAutoAcceptOnSave() && this.state.verticalDiffManager) { - const fileUri = doc.uri.toString(); - const handler = this.state.verticalDiffManager.getHandlerForFile(fileUri); - if (handler && handler.hasDiffForCurrentFile()) { - try { - // Accept all diffs BEFORE the save operation - // This ensures the document is saved in its final state - await this.state.staticDiffAdapter?.acceptAll(doc.uri.fsPath); - this.state.logger.info( - `Auto-accepted all diff decorations for ${doc.fileName} before save`, - ); - // Show user feedback that diffs were auto-accepted - vscode.window.showInformationMessage( - `Auto-accepted all diff changes for ${doc.fileName} - saving final state`, - ); - } catch (error) { - this.state.logger.error( - `Failed to auto-accept diff decorations before save for ${doc.fileName}:`, - error, - ); - vscode.window.showErrorMessage( - `Failed to auto-accept diff changes for ${doc.fileName}: ${error}`, - ); - } - } - }And add a separate onDidSaveTextDocument handler with a per-file guard to avoid loops:
// Place near other listeners const savingAfterAccept = new Set<string>(); this.listeners.push( vscode.workspace.onDidSaveTextDocument(async (doc) => { if (!getConfigAutoAcceptOnSave() || !this.state.verticalDiffManager) return; const fileUri = doc.uri.toString(); const filePath = doc.uri.fsPath; // Avoid infinite save loops if (savingAfterAccept.has(fileUri)) { savingAfterAccept.delete(fileUri); return; } const handler = this.state.verticalDiffManager.getHandlerForFile(fileUri); if (handler && handler.hasDiffForCurrentFile()) { try { await this.state.staticDiffAdapter?.acceptAll(filePath); this.state.logger.info(`Auto-accepted all diff decorations for ${doc.fileName} after save`); savingAfterAccept.add(fileUri); await doc.save(); vscode.window.showInformationMessage( `Auto-accepted all diff changes for ${doc.fileName}`, ); } catch (error) { this.state.logger.error( `Failed to auto-accept diff decorations after save for ${doc.fileName}:`, error, ); vscode.window.showErrorMessage( `Failed to auto-accept diff changes for ${doc.fileName}: ${error}`, ); } } }), );
🧹 Nitpick comments (6)
vscode/package.json (2)
475-481: New setting looks good; consider scope semanticskonveyor.diff.autoAcceptOnSave default and description are sensible. Consider whether "resource" scope would better support per-workspace overrides; "window" applies globally to the window.
610-610: Add missing contributed commands in package.jsonThe following programmatically‐registered commands aren’t declared under
contributes.commandsinagentic/package.json. Declaring them will surface them in the Command Palette (and satisfy Marketplace/lint rules). You can optionally hide them from the palette with a"when": "false"or"when": "never"clause.• File: agentic/package.json
Insert under"contributes": { "commands": [ … ] }:{ "command": "konveyor.showDiffWithDecorations", "title": "Show Diff with Decorations", "category": "Konveyor" }, { "command": "konveyor.acceptDiff", "title": "Accept All Diff Changes", "category": "Konveyor" }, { "command": "konveyor.rejectDiff", "title": "Reject All Diff Changes", "category": "Konveyor" }, { "command": "konveyor.acceptVerticalDiffBlock", "title": "Accept Diff Block", "category": "Konveyor" }, { "command": "konveyor.rejectVerticalDiffBlock", "title": "Reject Diff Block", "category": "Konveyor" }, { "command": "konveyor.clearDiffDecorations", "title": "Clear Diff Decorations", "category": "Konveyor" }, { "command": "konveyor.showDiffActions", "title": "Show Diff Actions", "category": "Konveyor" }vscode/src/utilities/ModifiedFiles/handleModifiedFile.ts (2)
137-141: Deletion detection may misclassify whitespace-only editsUsing modifiedContent.trim() === "" (Line 140) treats files that end up with only whitespace as deletions. If that’s not desired, consider a stricter check: compare length, or only treat as deleted when modifiedContent === "".
166-185: Pending interaction callback ignores response payloadThe resolver added to pendingInteractions (Line 169) discards the response argument, which may be needed to determine apply/reject or partial-apply semantics in some flows. If higher layers rely on this value, this will silently drop it.
If the intent is only to unblock the queue, add a comment clarifying that response is handled elsewhere (e.g., in webviewMessageHandler). Otherwise, forward response to the appropriate handler here.
Also prefer using state.logger over console.error for consistency:
- } catch (error) { - console.error(`Error in ModifiedFile resolver for messageId: ${msg.id}:`, error); + } catch (error) { + state.logger.error(`Error in ModifiedFile resolver for messageId: ${msg.id}:`, error);webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (1)
265-269: Prop naming mismatch: isFileApplied receives isViewingDiffYou pass isViewingDiff to a prop named isFileApplied. This is misleading and can cause UI logic errors. Either rename the prop in ModifiedFileActions to isViewingDiff, or pass a true "file applied" boolean.
vscode/src/extension.ts (1)
72-75: Dispose StatusBarItem reliably even if initialize failsThe StatusBarItem is created in the constructor but only added to subscriptions later (Line 281). If initialize throws before that, the item won’t be disposed.
Apply this diff to push it into subscriptions right after creation:
this.diffStatusBarItem = vscode.window.createStatusBarItem( vscode.StatusBarAlignment.Right, 100, ); + this.context.subscriptions.push(this.diffStatusBarItem);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
vscode/package.json(3 hunks)vscode/src/commands.ts(8 hunks)vscode/src/extension.ts(11 hunks)vscode/src/utilities/ModifiedFiles/handleModifiedFile.ts(1 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx(6 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T02:13:55.897Z
Learnt from: djzager
PR: konveyor/editor-extensions#678
File: vscode/src/commands.ts:135-139
Timestamp: 2025-08-12T02:13:55.897Z
Learning: In vscode/src/commands.ts, the solutionState is conditionally reset to "none" after successful getSolution completion, but only when !state.isWaitingForUserInteraction. This prevents permanent blocking of future getSolution calls while still maintaining proper concurrency control during active workflows.
Applied to files:
vscode/src/commands.ts
📚 Learning: 2025-07-23T12:10:46.399Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#581
File: vscode/src/extension.ts:243-251
Timestamp: 2025-07-23T12:10:46.399Z
Learning: In vscode/src/extension.ts, clearing all configErrors in the onDidSaveTextDocument listener for settings YAML is intentional behavior. The setupModelProvider and updateConfigErrors functions are designed to work together to rebuild the entire configuration error state.
Applied to files:
vscode/src/extension.ts
🧬 Code Graph Analysis (4)
vscode/src/utilities/ModifiedFiles/handleModifiedFile.ts (1)
vscode/src/utilities/ModifiedFiles/queueManager.ts (1)
handleUserInteractionComplete(170-187)
vscode/src/commands.ts (2)
vscode/src/diff/vertical/handler.ts (1)
fileUri(83-85)vscode/src/diff/verticalDiffCodeLens.ts (1)
VerticalDiffCodeLensProvider(4-62)
vscode/src/extension.ts (6)
vscode/src/extensionState.ts (1)
ExtensionState(21-57)vscode/src/utilities/configuration.ts (1)
getConfigAutoAcceptOnSave(70-71)vscode/src/diff/types.ts (1)
FileEditor(33-38)vscode/src/utilities/ideUtils.ts (1)
FileEditor(52-57)vscode/src/diff/vertical/manager.ts (1)
VerticalDiffManager(15-331)vscode/src/diff/staticDiffAdapter.ts (1)
StaticDiffAdapter(11-166)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (2)
webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts (1)
isLocalChange(110-110)shared/src/types/types.ts (1)
LocalChange(99-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
🔇 Additional comments (1)
vscode/src/commands.ts (1)
791-805: Handle new-file case when opening original content failsshowDiffWithDecorations opens the current file to get originalContent. For new files, openTextDocument will fail; fall back to empty original content. Also, content param is currently unused—fine to leave but be aware.
[ suggest_essential_refactor ]
Apply this diff:- // Get original content - const uri = Uri.file(filePath); - const doc = await workspace.openTextDocument(uri); - const originalContent = doc.getText(); + // Get original content; fall back to empty for new files + let originalContent = ""; + try { + const doc = await workspace.openTextDocument(Uri.file(filePath)); + originalContent = doc.getText(); + } catch { + originalContent = ""; + }
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
shared/src/types/types.ts (1)
183-183: PreferunknownoveranyfordiffResolvedStatesWe’ve verified that all producers (extension.ts, commands.ts) and consumers (ExtensionStateContext.tsx) default to
{}and will remain compatible when typed asRecord<string, unknown>. Please apply this minimal change:• shared/src/types/types.ts (line 183)
- diffResolvedStates: Record<string, any>; + diffResolvedStates: Record<string, unknown>;Optional long-term refactor for stronger typing:
• Create
shared/src/types/diff.tsexport type DiffResolvedStates = Record<string, unknown>; // refine shape when stable• Update
shared/src/types/types.ts- diffResolvedStates: Record<string, unknown>; + diffResolvedStates: DiffResolvedStates;This will catch invalid uses at compile time without affecting runtime behavior.
webview-ui/src/context/ExtensionStateContext.tsx (1)
48-49: Use nullish coalescing (??) instead of||when defaulting objects.
||can inadvertently treat other falsey values as "absent". While unlikely here,??is the safer, idiomatic choice for defaulting possibly undefined/null inputs.Apply this diff:
- diffResolvedStates: windowData.diffResolvedStates || {}, + diffResolvedStates: windowData.diffResolvedStates ?? {},
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
shared/src/types/types.ts(1 hunks)webview-ui/src/context/ExtensionStateContext.tsx(3 hunks)
🔇 Additional comments (2)
webview-ui/src/context/ExtensionStateContext.tsx (2)
27-28: Good: defaultdiffResolvedStatesinitialized.Initializing to an empty object ensures downstream code never deals with
undefined.
84-85: Guard against unintended resets on partial postMessage payloads.If an incoming message omits
diffResolvedStates, this line forces it back to{}, potentially discarding previously known state. If the extension always posts the full state, this is fine; otherwise, prefer preserving the last known value and only overriding when provided.Apply this minimal improvement:
- diffResolvedStates: event.data.diffResolvedStates || {}, + diffResolvedStates: event.data.diffResolvedStates ?? {},If you expect partial updates, consider preserving prior state:
useEffect(() => { const handleMessage = (event: MessageEvent<Partial<ExtensionData>>) => { setState((prev) => { const next: ExtensionData = { ...defaultState, ...prev, ...event.data, localChanges: Array.isArray(event.data.localChanges) ? event.data.localChanges : prev.localChanges, ruleSets: Array.isArray(event.data.ruleSets) ? event.data.ruleSets : prev.ruleSets, enhancedIncidents: Array.isArray(event.data.enhancedIncidents) ? event.data.enhancedIncidents : prev.enhancedIncidents, chatMessages: Array.isArray(event.data.chatMessages) ? event.data.chatMessages : prev.chatMessages, configErrors: Array.isArray(event.data.configErrors) ? event.data.configErrors : prev.configErrors, profiles: Array.isArray(event.data.profiles) ? event.data.profiles : prev.profiles, diffResolvedStates: event.data.diffResolvedStates ?? prev.diffResolvedStates, }; return next; }); }; // ... }, []);To verify whether the extension always posts complete
ExtensionData, search for the posting sites:
ibolton336
force-pushed
the
decorators-3
branch
from
c72e97d to
b86ee88
Compare
- Add decorator support for code lens accept/reject functionality - Implement visual layer for code lens interactions - Add diff manager for handling code changes - Stream diff functionality with proper error handling - Accept/Reject all operations with clean save functionality - Style improvements and cleanup Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
ibolton336
force-pushed
the
decorators-3
branch
from
058517b to
8357e77
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/pages/vscode.page.ts (2)
161-165: Reduce selector brittleness; wait explicitly before dblclickThe exact heading text “Konveyor: Konveyor Issues” can drift. Match via regex and add an explicit visibility wait for stability before double-click.
- await this.window - .getByRole('heading', { name: 'Konveyor: Konveyor Issues' }) - .first() - .dblclick(); + const issuesHeading = this.window + .getByRole('heading', { name: /Konveyor Issues/ }) + .first(); + await expect(issuesHeading).toBeVisible({ timeout: 15000 }); + await issuesHeading.dblclick();
166-170: Target role=button and add explicit wait for the “Open Analysis View” actionScoping by label is fine, but targeting role=button and asserting visibility first improves reliability and debuggability. A small click delay helps on slower CI (consistent with other waits in this suite).
- await this.window - .getByLabel('Konveyor Issues Section') - .getByLabel('Open Konveyor Analysis View') - .first() - .click(); + const openAnalysisBtn = this.window + .getByLabel('Konveyor Issues Section') + .getByRole('button', { name: 'Open Konveyor Analysis View' }) + .first(); + await expect(openAnalysisBtn).toBeVisible({ timeout: 15000 }); + await openAnalysisBtn.click({ delay: 500 });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/e2e/pages/vscode.page.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T16:16:56.005Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
Applied to files:
tests/e2e/pages/vscode.page.ts
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/pages/vscode.page.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (2)
tests/e2e/pages/vscode.page.ts (2)
158-158: Comment-only change — looks goodNo functional impact.
150-157: Harden palette path: wait for Analysis View to open and verify command definitionBefore returning immediately after the quick-command, add a short wait to ensure the Analysis View tab actually appears—otherwise flakiness can creep in when the command is accepted but the view loads slowly. Also, double-check that your package.json defines the command title and category exactly as used below.
• File: tests/e2e/pages/vscode.page.ts (around lines 150–157)
• Verify in your extension’s package.json under contributes.commands that you have:
- category:
"Konveyor"- title:
"Open Konveyor Analysis View"
so that the palette entry is exactlyKonveyor: Open Konveyor Analysis View.Suggested diff:
try { await this.executeQuickCommand('Konveyor: Open Konveyor Analysis View'); + // Ensure the Analysis View actually appeared before returning + await this.window + .locator(`div.tab.active[aria-label="${KAIViews.analysisView}"]`) + .waitFor({ timeout: 15000 }); return; } catch (error) { // Fallback to activity bar approach for when extension is visible }Please confirm the command definition in package.json and that the ARIA label locator (
KAIViews.analysisView) matches the UI.
ibolton336
force-pushed
the
decorators-3
branch
from
8357e77 to
2ff15ec
Compare
There was a problem hiding this comment.
LGTM
The things that I was playing with that I believe absolutely must work worked. I don't see anything that stands out as wrong in the code but admittedly I struggled to apply a critical/focused eye to all 44 pages of changes. I'm confident that accepting this now moves us forward and gives us time to address more in follow-ups.
ibolton336
force-pushed
the
decorators-3
branch
from
5868b53 to
1693775
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vscode/src/utilities/ModifiedFiles/handleFileResponse.ts (1)
17-27: Use path.dirname instead of string slicing for cross‑platform directory creationString slicing with "/" will fail on Windows paths. Use Node’s path API to derive the directory and guard against ".".
Apply this diff:
+import * as path from "path"; @@ - // Ensure the directory structure exists - const directoryPath = filePath.substring(0, filePath.lastIndexOf("/")); - if (directoryPath) { - const directoryUri = vscode.Uri.file(directoryPath); + // Ensure the directory structure exists (cross-platform) + const directoryPath = path.dirname(filePath); + if (directoryPath && directoryPath !== "." && directoryPath !== filePath) { + const directoryUri = vscode.Uri.file(directoryPath); try { await vscode.workspace.fs.createDirectory(directoryUri); } catch (dirError) { state.logger .child({ component: "handleFileResponse.createNewFile" }) .error(`Failed to create directory at ${directoryPath}:`, dirError); } }
♻️ Duplicate comments (3)
vscode/src/utilities/ModifiedFiles/handleModifiedFile.ts (2)
66-71: Guard against undefined modifiedContent in isDeleted computationCalling trim() on undefined will throw. Align deletion detection with a safe check used across the codebase.
Apply this diff:
- const isNew = fileState.originalContent === undefined; - const isDeleted = !isNew && fileState.modifiedContent.trim() === ""; + const isNew = fileState.originalContent === undefined; + const isDeleted = + !isNew && + (fileState.modifiedContent === undefined || + fileState.modifiedContent.trim() === "");
137-141: Same unsafe trim() in handler; unify with createFileDiff logicRepeat of the undefined trim() issue; fix here too to avoid runtime TypeErrors during message handling.
Apply this diff:
- const isNew = fileState.originalContent === undefined; - const isDeleted = !isNew && fileState.modifiedContent.trim() === ""; + const isNew = fileState.originalContent === undefined; + const isDeleted = + !isNew && + (fileState.modifiedContent === undefined || + fileState.modifiedContent.trim() === "");webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (1)
28-33: Guard access to activeDecorators and drop debug logsAvoid indexing with an undefined messageToken and remove noisy console logs.
Apply this diff:
- const hasActiveDecorators = !!(state.activeDecorators && state.activeDecorators[messageToken]); - console.log( - `[ModifiedFileMessage] messageToken: ${messageToken}, hasActiveDecorators: ${hasActiveDecorators}, activeDecorators:`, - state.activeDecorators, - ); + const hasActiveDecorators = Boolean( + messageToken && state.activeDecorators?.[messageToken], + );
🧹 Nitpick comments (12)
vscode/src/utilities/ModifiedFiles/handleFileResponse.ts (1)
169-182: Avoid reusing the name messageIndex inside mutateData for clarityThe inner messageIndex shadows the outer one; prefer a distinct name to reduce cognitive load.
Apply this diff:
- state.mutateData((draft) => { - const messageIndex = draft.chatMessages.findIndex( + state.mutateData((draft) => { + const idx = draft.chatMessages.findIndex( (msg) => msg.messageToken === messageToken, ); - if ( - messageIndex >= 0 && - draft.chatMessages[messageIndex].kind === ChatMessageType.ModifiedFile + if ( + idx >= 0 && + draft.chatMessages[idx].kind === ChatMessageType.ModifiedFile ) { - const modifiedFileMessage = draft.chatMessages[messageIndex].value as any; + const modifiedFileMessage = draft.chatMessages[idx].value as any; modifiedFileMessage.status = "applied"; } });vscode/src/utilities/ModifiedFiles/handleModifiedFile.ts (3)
48-53: Don’t clear all pending promises on error; it can break other files’ processingNuking the shared array risks hiding work that other handlers are awaiting. Prefer targeted cleanup or leave the array intact.
Apply this diff to stop clearing globally:
- // Clear any promises that might be stuck in the array - // Since we can't easily identify which promises are related to this specific file, - // we'll clear all promises that haven't been resolved yet - // This is a conservative approach to prevent stuck promises - modifiedFilesPromises.length = 0; + // Avoid clearing shared promises array here; let callers manage their own awaits.And, after successful await, reset the list to prevent unbounded growth (see next comment).
130-133: Reset modifiedFilesPromises after successful await to avoid growthPromises accumulate across messages; after they’ve all resolved, clear the array to release references.
Apply this diff:
- await Promise.all(modifiedFilesPromises); + await Promise.all(modifiedFilesPromises); + // All processed; clear the list to prevent growth across messages + modifiedFilesPromises.length = 0;
142-157: Initialize a pending status on the chat message to aid UI logicExplicitly set status: "pending" so the webview can display consistent state before user action.
Apply this diff:
draft.chatMessages.push({ kind: ChatMessageType.ModifiedFile, messageToken: msg.id, timestamp: new Date().toISOString(), value: { path: filePath, content: fileState.modifiedContent, originalContent: fileState.originalContent, // Use from ModifiedFileState isNew: isNew, isDeleted: isDeleted, diff: diff, messageToken: msg.id, // Add message token to value for reference + status: "pending", }, });webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (3)
35-47: Resolve lint warning and normalize status mapping ("discarded" → "rejected")Use status as a fallback only when global state is absent, and map "discarded" to "rejected".
Apply this diff:
- // Get status from global state as the single source of truth + // Get status from global state as the single source of truth const currentMessage = state.chatMessages.find((msg) => msg.messageToken === messageToken); const globalStatus = currentMessage?.kind === ChatMessageType.ModifiedFile ? (currentMessage.value as any)?.status : null; - const actionTaken = - globalStatus === "applied" ? "applied" : globalStatus === "rejected" ? "rejected" : null; - - console.log( - `[ModifiedFileMessage] Global state check - messageToken: ${messageToken}, globalStatus: ${globalStatus}, actionTaken: ${actionTaken}`, - ); + const mappedNormalizedStatus = + status === "applied" ? "applied" : status === "discarded" ? "rejected" : null; + const actionTaken = + globalStatus === "applied" + ? "applied" + : globalStatus === "rejected" + ? "rejected" + : mappedNormalizedStatus;
125-148: Also guard SHOW_DIFF_WITH_DECORATORS on missing messageTokenMake the decorator view robust in edge cases; otherwise the host may ignore the message.
Apply this diff:
- setIsViewingDiff(true); + setIsViewingDiff(true); @@ - const payload: ShowDiffWithDecoratorsPayload = { + const payload: ShowDiffWithDecoratorsPayload = { path: filePath, content: content, diff: fileDiff, - messageToken: messageToken, + messageToken: messageToken ?? "", }; - window.vscode.postMessage({ - type: "SHOW_DIFF_WITH_DECORATORS", - payload, - }); + if (messageToken) { + window.vscode.postMessage({ + type: "SHOW_DIFF_WITH_DECORATORS", + payload, + }); + } else { + console.warn("[ModifiedFileMessage] Missing messageToken for SHOW_DIFF_WITH_DECORATORS."); + }
220-230: Pass through an isProcessingAction flag (optional)If you later surface host round‑trip latency, consider passing an isProcessingAction state into ModifiedFileActions to disable buttons while awaiting host responses.
Would you like me to wire a lightweight isProcessingAction state that toggles around postMessage calls?
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx (1)
13-24: Remove unused onView prop to reduce API surface (and update caller)onView is required in props but ignored in the component (renamed _onView). Drop it to avoid confusion and update the caller to stop passing it.
Apply this diff here:
interface ModifiedFileActionsProps { actionTaken: "applied" | "rejected" | null; normalizedData: NormalizedFileData; onApply: () => void; onReject: () => void; - onView: (path: string, diff: string) => void; onViewWithDecorations?: (path: string, diff: string) => void; isViewingDiff?: boolean; onContinue?: () => void; hasActiveDecorators?: boolean; isProcessingAction?: boolean; } @@ -const ModifiedFileActions: React.FC<ModifiedFileActionsProps> = ({ +const ModifiedFileActions: React.FC<ModifiedFileActionsProps> = ({ actionTaken, normalizedData, onApply, onReject, - onView: _onView, onViewWithDecorations, isViewingDiff, onContinue, hasActiveDecorators, isProcessingAction, }) => {And in the caller (ModifiedFileMessage.tsx), remove the prop:
- <ModifiedFileActions + <ModifiedFileActions actionTaken={actionTaken} normalizedData={normalizedData} onApply={() => applyFile()} onReject={rejectFile} - onView={viewFileInVSCode} onViewWithDecorations={viewFileWithDecorations} isViewingDiff={isViewingDiff} onContinue={handleContinue} hasActiveDecorators={hasActiveDecorators} />Also applies to: 175-187
vscode/src/webviewMessageHandler.ts (4)
315-318: Normalization trims only one trailing newline; consider trimming all trailing newlines and centralize helperIf you want to avoid false mismatches when there are multiple trailing newlines, strip all of them. Also, this helper is duplicated in CONTINUE_WITH_FILE_STATE; pull it into a shared util to keep parity with myers.ts and prevent drift.
- const normalize = (text: string) => { - // Use the same normalization approach as myers.ts - return text.replace(/\r\n/g, "\n").replace(/\n$/, ""); // Remove trailing newline for comparison - }; + const normalize = (text: string) => + text.replace(/\r\n/g, "\n").replace(/\n+$/, ""); // Remove trailing newlines for comparisonIf myers.ts deliberately removes only a single newline, import and reuse that exact function instead to keep behavior identical.
420-423: Deduplicate normalization helperSame normalization logic as in CHECK_FILE_STATE; extract to a shared utility (e.g., utilities/text/normalize.ts) and use it in both places.
- const normalize = (text: string) => text.replace(/\r\n/g, "\n").replace(/\n$/, ""); + // reuse a shared normalize(text) helper here
430-431: Remove stray console.log noise; use logger.debug or drop itConsole logs in VS Code extensions are noisy and inconsistent with winston.
- console.log("Continue decision: ", { responseId, hasChanges }); + logger.debug("Continue decision", { responseId, hasChanges });
189-205: Refine SHOW_DIFF_WITH_DECORATORS handler signature and add a fail-fast guardTo prevent accidental payload drift and eliminate
any, update the handler invscode/src/webviewMessageHandler.tsso it:
• Accepts a strongly-typedpayloadobject.
• Early-exits if required fields (pathordiff) are missing.
• Delegates to the existing command registration invscode/src/commands.ts(“konveyor.showDiffWithDecorations” takes(filePath, diff, content, messageToken)in that order).Suggested diff:
--- a/vscode/src/webviewMessageHandler.ts +++ b/vscode/src/webviewMessageHandler.ts @@ -184,16 +184,28 @@ export const webviewMessageHandler: MessageHandler = { SHOW_DIFF_WITH_DECORATORS: async ({ path, diff, content, messageToken }, state, logger) => { try { logger.info("SHOW_DIFF_WITH_DECORATORS called", { path, messageToken }); - // Execute the command to show diff with decorations using streaming approach - await vscode.commands.executeCommand( - "konveyor.showDiffWithDecorations", - path, - diff, - content, - messageToken, - ); + // Sanity-check required fields before dispatch + if (!path || !diff) { + logger.error("SHOW_DIFF_WITH_DECORATORS missing required fields", { path, diff, payload: { content, messageToken } }); + return; + } + + // Forward to the registered extension command + await vscode.commands.executeCommand( + "konveyor.showDiffWithDecorations", + path, // maps to filePath in commands.ts + diff, + content, + messageToken, + ); } catch (error) { logger.error("Error handling SHOW_DIFF_WITH_DECORATORS:", error); vscode.window.showErrorMessage(`Failed to show diff with decorations: ${error}`); } },And update the handler signature to declare the payload’s shape:
type ShowDiffWithDecoratorsPayload = { path: string; diff: string; content: string; messageToken: string; }; … SHOW_DIFF_WITH_DECORATORS: async ( payload: ShowDiffWithDecoratorsPayload, state, logger ) => { const { path, diff, content, messageToken } = payload; … }This ensures strong typing at the message boundary and fails fast when essential data is missing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
tests/e2e/pages/vscode.page.ts(1 hunks)vscode/src/utilities/ModifiedFiles/handleFileResponse.ts(1 hunks)vscode/src/utilities/ModifiedFiles/handleModifiedFile.ts(1 hunks)vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts(0 hunks)vscode/src/webviewMessageHandler.ts(5 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx(2 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx(6 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/pages/vscode.page.ts
- webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-01T13:53:20.774Z
Learnt from: djzager
PR: konveyor/editor-extensions#645
File: vscode/src/webviewMessageHandler.ts:575-577
Timestamp: 2025-08-01T13:53:20.774Z
Learning: The TOGGLE_AGENT_MODE action handler in vscode/src/webviewMessageHandler.ts correctly calls toggleAgentMode() without parameters, as the toggleAgentMode function in vscode/src/utilities/configuration.ts has been updated to internally handle reading and toggling the agent mode configuration value.
Applied to files:
vscode/src/webviewMessageHandler.tswebview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
🪛 GitHub Check: Lint
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
[warning] 24-24:
'status' is assigned a value but never used
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (windows)
🔇 Additional comments (7)
vscode/src/utilities/ModifiedFiles/handleFileResponse.ts (2)
112-168: Apply flow reads clean and robust; analyzer trigger is well-scopedGood separation of create/update/delete, agent-mode analysis trigger is wrapped to not fail the apply path, and solution-server notification covers both delete and update cases.
184-197: Confirm whether the solution server requires a discard notification on rejectCurrently, reject only updates local state. If the solution server expects a discard signal (symmetry with changeApplied), wire it here to keep external state consistent.
Option if needed:
} else { // For reject, also update the global state state.mutateData((draft) => { ... }); + // Notify solution server that the proposed change was rejected + try { + await vscode.commands.executeCommand("konveyor.changeDiscarded", path); + } catch (error) { + logger.error("Error notifying solution server on reject:", error); + } }webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx (2)
45-94: “Continue” disabled while decorators are active — confirm host clears the flagThe banner disables Continue when hasActiveDecorators is true. Ensure host reliably clears activeDecorators after accept/reject/save, otherwise users could get stuck.
If needed, emit a “decoratorsClosed” or similar signal from host to flip hasActiveDecorators to false once the decorated editor is torn down.
120-136: Good UX: single “Review Changes” button funnels users into the decorated flowThe primary action and spinner handling read clean and match the new vertical diff experience.
vscode/src/webviewMessageHandler.ts (3)
43-44: LGTM: toggleAgentMode import and usage are correctThis aligns with the prior change where toggleAgentMode() reads/toggles config internally; no parameters needed.
257-284: diff.reversePatch usage is valid in diff v5.xVerified that the
diffv5.x API publicly exportsreversePatch(patch)and that it accepts either a singleStructuredPatchor an array of them, returning the corresponding reversed patch (StructuredPatch | StructuredPatch[]) (npmjs.com, jsdocs.io). The output ofreversePatchcan be passed directly toapplyPatch(orapplyPatches), as those methods accept either a unified‐diff string or structured/parsed-patch object(s) (npmjs.com).No changes required.
175-188: Confirm whether REJECT_FILE should handle a backend “reject” or can be removed
- I didn’t find any client-side emitters posting a
type: "REJECT_FILE"message in the webview code, so this handler is never invoked and only shows a toast.- If you intend a dedicated “reject changes” action:
- Update the handler signature to accept
messageToken(andcontent)- Call
await handleFileResponse(messageToken, "reject", path, content, state)after showing the toast- Change the error log label from
"NEW_REJECT_FILE"to"REJECT_FILE"for consistency- If this flow isn’t needed, consider removing the
REJECT_FILEentry (or renaming it toREJECT_FILE_LOCALLY) to avoid dead code and update the surrounding comments accordingly
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
Outdated
Show resolved
Hide resolved
ibolton336
force-pushed
the
decorators-3
branch
from
1693775 to
d0b6c41
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (3)
28-30: Guard against undefined/empty messageToken when checking activeDecoratorsIndexing with an empty string can create a misleading “truthy” lookup under the "" key.
- const hasActiveDecorators = !!(state.activeDecorators && state.activeDecorators[messageToken]); + const hasActiveDecorators = Boolean( + messageToken && state.activeDecorators?.[messageToken], + );
105-109: Don’t post FILE_RESPONSE without a valid messageTokenPosting with undefined/empty token can break the host handler. Keep UI behavior consistent but avoid sending invalid messages.
- // Use FILE_RESPONSE flow for standalone apply button - postFileResponse("apply", messageToken, path, contentToApply); - // Trigger scroll after action - onUserAction?.(); + // Use FILE_RESPONSE flow for standalone apply button (only with a valid token) + if (messageToken) { + postFileResponse("apply", messageToken, path, contentToApply); + // Trigger scroll after action + onUserAction?.(); + } else { + console.warn("[ModifiedFileMessage] Missing messageToken on apply; ignoring action."); + }
115-119: Guard reject flow on messageToken as wellSame rationale as apply.
- // Use FILE_RESPONSE flow for standalone reject button - postFileResponse("reject", messageToken, path); - // Trigger scroll after action - onUserAction?.(); + // Use FILE_RESPONSE flow for standalone reject button (only with a valid token) + if (messageToken) { + postFileResponse("reject", messageToken, path); + // Trigger scroll after action + onUserAction?.(); + } else { + console.warn("[ModifiedFileMessage] Missing messageToken on reject; ignoring action."); + }vscode/src/webviewMessageHandler.ts (2)
258-260: Bug: Uint8Array.toString() doesn’t decode UTF‑8; decode bytes properly.Using .toString() on Uint8Array returns a non-text representation and will break comparisons and downstream logic.
Apply this diff:
- const currentContent = await vscode.workspace.fs.readFile(uri); - const currentText = currentContent.toString(); + const currentBytes = await vscode.workspace.fs.readFile(uri); + const currentText = new TextDecoder("utf-8").decode(currentBytes);
261-271: Guard missing originalContent; otherwise you’ll incorrectly “apply” against an empty baseline.If modifiedFileState is missing or originalContent is undefined, defaulting to "" will mark any non-empty file as “changed”, causing an unintended “apply”.
Apply this diff:
- const modifiedFileState = state.modifiedFiles.get(path); - const originalContent = modifiedFileState?.originalContent || ""; + const modifiedFileState = state.modifiedFiles.get(path); + if (!modifiedFileState?.originalContent) { + logger.warn("CONTINUE_WITH_FILE_STATE: missing originalContent; defaulting to reject", { + path, + messageToken, + }); + await handleFileResponse(messageToken, "reject", path, content, state); + return; + } + const originalContent = modifiedFileState.originalContent;Nit: For the newline normalization, consider handling multiple trailing newlines:
- const normalize = (text: string) => text.replace(/\r\n/g, "\n").replace(/\n$/, ""); + const normalize = (text: string) => text.replace(/\r\n/g, "\n").replace(/\n+$/, "");
🧹 Nitpick comments (14)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx (4)
26-46: Prefer PatternFly tokens over hard-coded colors in StatusDisplayHard-coding "green"/"red" makes themes inconsistent (dark mode, downstream themes). Use PF CSS vars for semantic colors.
- <CheckCircleIcon color="green" /> Changes applied + <CheckCircleIcon color="var(--pf-v5-global--success-color--100)" /> Changes applied ... - <TimesCircleIcon color="red" /> Changes rejected + <TimesCircleIcon color="var(--pf-v5-global--danger-color--100)" /> Changes rejected
88-96: Re-evaluate disabling “Continue” while decorators are activeSetting isDisabled={hasActiveDecorators} can strand users: they cannot proceed while a diff is open even if they’ve already saved/accepted. If “active decorators” simply means the diff is visible, this block may be overzealous.
Consider enabling Continue and letting the backend return the authoritative state via CONTINUE_WITH_FILE_STATE. If you must gate, perhaps only disable when there are pending, unapplied hunks (a stronger signal than “decorators visible”).
- isDisabled={hasActiveDecorators} + isDisabled={false}If we keep the guard, please document why decorators imply “cannot continue.”
205-209: Guard and memoize the onViewWithDecorations callbackMinor: wrap the callback in useCallback and guard against missing diff/path to avoid needless re-renders and accidental undefined payloads.
- onViewWithDecorations={ - onViewWithDecorations - ? () => onViewWithDecorations(normalizedData.path, normalizedData.diff) - : undefined - } + onViewWithDecorations={ + onViewWithDecorations && normalizedData.path && normalizedData.diff + ? () => onViewWithDecorations(normalizedData.path, normalizedData.diff) + : undefined + }
63-86: Use PatternFly Icon status prop for semantic colorsPatternFly v5’s React
<Icon>component supports astatusprop ('custom' | 'info' | 'success' | 'warning' | 'danger') which applies the appropriate semantic color via PF CSS modifiers (e.g.pf-m-info,pf-m-warning) automatically—no manual hex colors needed. (react-staging.patternfly.org, pf5.patternfly.org)Please update the two icon usages in
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx(around lines 63–86) as follows:- <Icon status="warning"> - <ExclamationTriangleIcon color="#b98412" /> - </Icon> + <Icon status="warning"> + <ExclamationTriangleIcon /> + </Icon> … - <Icon> - <InfoCircleIcon color="#4394e5" /> - </Icon> + <Icon status="info"> + <InfoCircleIcon /> + </Icon>This will ensure your icons inherit PatternFly’s semantic color tokens without hard-coding hex values.
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (4)
33-37: Avoid any-cast; narrow ModifiedFile message value safelyRemove the any and leverage the shared ModifiedFileMessageValue type.
- const globalStatus = - currentMessage?.kind === ChatMessageType.ModifiedFile - ? (currentMessage.value as any)?.status - : null; + let globalStatus: "applied" | "rejected" | null = null; + if (currentMessage?.kind === ChatMessageType.ModifiedFile) { + const v = currentMessage.value as ModifiedFileMessageValue; + globalStatus = (v.status as "applied" | "rejected" | undefined) ?? null; + }
146-161: Validate messageToken before CONTINUE_WITH_FILE_STATEMirror the safety check used in other host calls. Optionally set local “processing” here if the action can originate outside the banner.
- const handleContinue = () => { - // Send CONTINUE_WITH_FILE_STATE message to check current file state - window.vscode.postMessage({ - type: "CONTINUE_WITH_FILE_STATE", - payload: { - messageToken, - path, - content, - }, - }); + const handleContinue = () => { + if (!messageToken) { + console.warn("[ModifiedFileMessage] Missing messageToken on continue; ignoring."); + return; + } + // Send CONTINUE_WITH_FILE_STATE message to check current file state + window.vscode.postMessage({ + type: "CONTINUE_WITH_FILE_STATE", + payload: { + messageToken, + path, + content, + }, + });
186-189: Satisfy linter by breaking long ternary across linesThis addresses the warnings surfaced by the Lint check.
- <span className={`status-badge status-${effectiveActionTaken}`}> - {effectiveActionTaken === "applied" ? "✓ Applied" - : effectiveActionTaken === "rejected" ? "✗ Rejected" - : "⏳ Processing..."} - </span> + <span className={`status-badge status-${effectiveActionTaken}`}> + {effectiveActionTaken === "applied" + ? "✓ Applied" + : effectiveActionTaken === "rejected" + ? "✗ Rejected" + : "⏳ Processing..."} + </span>
62-68: Optionally derive isViewingDiff from activeDecorators to avoid stale bannerIf the editor closes the diff without changing status, the banner may remain. Syncing to decorator presence avoids confusion.
useEffect(() => { if (effectiveActionTaken !== null && effectiveActionTaken !== "processing") { setIsViewingDiff(false); } }, [effectiveActionTaken]); + + // Optional: hide banner if decorators are no longer active + useEffect(() => { + if (!hasActiveDecorators && isViewingDiff) { + setIsViewingDiff(false); + } + }, [hasActiveDecorators, isViewingDiff]);webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (2)
26-74: Type the state parameter instead of any in useResolutionDataUsing any obscures contract changes and hurts refactors. Prefer a typed shape (e.g., ExtensionState) from context/types.
Example (outline):
const useResolutionData = (state: ExtensionState) => { /* ... */ };If ExtensionState isn’t exported, consider a local interface that captures only the consumed fields.
138-199: Prune unused dependency from renderChatMessagesisFetchingSolution isn’t referenced inside the callback; removing it avoids unnecessary memo invalidations.
- }, [chatMessages, isFetchingSolution, isAnalyzing, triggerScrollOnUserAction]); + }, [chatMessages, isAnalyzing, triggerScrollOnUserAction]);vscode/src/webviewMessageHandler.ts (4)
175-188: REJECT_FILE doesn’t update state/solution server; fix logging label and route through the unified handler when possible.Right now this only shows a toast and logs. It neither updates chat message status nor notifies the solution server (e.g., konveyor.changeDiscarded). Also, the error label says "NEW_REJECT_FILE", which looks stale.
Consider routing through handleFileResponse when a messageToken is available; otherwise, fall back to notifying the solution server only. Also correct the log label.
Apply this diff:
- REJECT_FILE: async ({ path }, _state, logger) => { + REJECT_FILE: async ( + { path, messageToken }: { path: string; messageToken?: string }, + state, + logger, + ) => { try { - // For rejecting changes, we don't need to do anything since we're not - // directly modifying the real file until the user applies changes - vscode.window.showInformationMessage( - `Changes rejected for ${vscode.workspace.asRelativePath(vscode.Uri.file(path))}`, - ); + if (messageToken) { + await handleFileResponse(messageToken, "reject", path, undefined, state); + } else { + // Fallback for callers that don't use chat messages + await vscode.commands.executeCommand("konveyor.changeDiscarded", path); + } + vscode.window.showInformationMessage(`Changes rejected for ${vscode.workspace.asRelativePath(vscode.Uri.file(path))}`); } catch (error) { - logger.error("Error handling NEW_REJECT_FILE:", error); + logger.error("Error handling REJECT_FILE:", error); vscode.window.showErrorMessage(`Failed to reject changes: ${error}`); } },
272-276: Remove console.log and use logger consistently.Avoid mixing console.log with the structured logger; keep logs in the output channel.
Apply this diff:
logger.debug( `Continue decision: ${responseId.toUpperCase()} - ${hasChanges ? "file has changes" : "file unchanged"}`, ); - console.log("Continue decision: ", { responseId, hasChanges });
284-296: Redundant status mutation; handleFileResponse already updates chat message status.handleFileResponse mutates the message state (applied/rejected) and resolves pending interactions. The extra mutateData here can cause duplicate updates and unnecessary re-renders.
Apply this diff to remove both blocks:
- // Update the chat message status - state.mutateData((draft) => { - const messageIndex = draft.chatMessages.findIndex( - (msg) => msg.messageToken === messageToken, - ); - if ( - messageIndex >= 0 && - draft.chatMessages[messageIndex].kind === ChatMessageType.ModifiedFile - ) { - const modifiedFileMessage = draft.chatMessages[messageIndex].value as any; - modifiedFileMessage.status = hasChanges ? "applied" : "rejected"; - } - }); @@ - state.mutateData((draft) => { - const messageIndex = draft.chatMessages.findIndex( - (msg) => msg.messageToken === messageToken, - ); - if ( - messageIndex >= 0 && - draft.chatMessages[messageIndex].kind === ChatMessageType.ModifiedFile - ) { - const modifiedFileMessage = draft.chatMessages[messageIndex].value as any; - modifiedFileMessage.status = "rejected"; - } - });Also applies to: 302-313
255-271: Optional: prefer unsaved editor content over filesystem when comparing.If the user hasn’t saved yet, fs.readFile won’t reflect the in-editor changes. Prefer an open editor’s text when available.
Apply this diff (builds on the decoding fix):
- // Get the current file content - const currentBytes = await vscode.workspace.fs.readFile(uri); - const currentText = new TextDecoder("utf-8").decode(currentBytes); + // Get the current file content: prefer unsaved editor text, fallback to filesystem + const openDoc = vscode.workspace.textDocuments.find((d) => d.uri.fsPath === path); + const currentText = openDoc + ? openDoc.getText() + : new TextDecoder("utf-8").decode(await vscode.workspace.fs.readFile(uri));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
vscode/src/utilities/ModifiedFiles/handleFileResponse.ts(1 hunks)vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts(0 hunks)vscode/src/webviewMessageHandler.ts(4 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/DiffLegend.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/DiffLinesRenderer.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/EnhancedDiffRenderer.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/HunkSelectionInterface.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx(5 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModal.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModalHeader.css(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModalHeader.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/SingleHunkDisplay.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/enhancedSyntaxHighlighting.css(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/index.ts(1 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(4 hunks)
💤 Files with no reviewable changes (10)
- webview-ui/src/components/ResolutionsPage/ModifiedFile/enhancedSyntaxHighlighting.css
- webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModalHeader.css
- webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModalHeader.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/HunkSelectionInterface.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/SingleHunkDisplay.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/DiffLegend.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/DiffLinesRenderer.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/EnhancedDiffRenderer.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModal.tsx
- vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- vscode/src/utilities/ModifiedFiles/handleFileResponse.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T02:13:55.897Z
Learnt from: djzager
PR: konveyor/editor-extensions#678
File: vscode/src/commands.ts:135-139
Timestamp: 2025-08-12T02:13:55.897Z
Learning: In vscode/src/commands.ts, the solutionState is conditionally reset to "none" after successful getSolution completion, but only when !state.isWaitingForUserInteraction. This prevents permanent blocking of future getSolution calls while still maintaining proper concurrency control during active workflows.
Applied to files:
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
📚 Learning: 2025-08-01T13:53:20.774Z
Learnt from: djzager
PR: konveyor/editor-extensions#645
File: vscode/src/webviewMessageHandler.ts:575-577
Timestamp: 2025-08-01T13:53:20.774Z
Learning: The TOGGLE_AGENT_MODE action handler in vscode/src/webviewMessageHandler.ts correctly calls toggleAgentMode() without parameters, as the toggleAgentMode function in vscode/src/utilities/configuration.ts has been updated to internally handle reading and toggling the agent mode configuration value.
Applied to files:
vscode/src/webviewMessageHandler.tswebview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
🧬 Code graph analysis (4)
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (7)
webview-ui/src/hooks/useScrollManagement.ts (1)
useScrollManagement(5-259)shared/src/types/types.ts (1)
ModifiedFileMessageValue(294-304)webview-ui/src/hooks/actions.ts (1)
openFile(52-58)webview-ui/src/components/ResolutionsPage/MessageWrapper.tsx (1)
MessageWrapper(9-11)webview-ui/src/components/ResolutionsPage/ToolMessage.tsx (1)
ToolMessage(116-181)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (1)
ModifiedFileMessage(17-236)webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (1)
ReceivedMessage(22-83)
vscode/src/webviewMessageHandler.ts (2)
vscode/src/client/tracer.ts (1)
logger(60-65)vscode/src/utilities/ModifiedFiles/handleFileResponse.ts (1)
handleFileResponse(92-225)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx (1)
webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts (1)
NormalizedFileData(33-44)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (3)
shared/src/types/types.ts (2)
ModifiedFileMessageValue(294-304)LocalChange(99-108)webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts (1)
useModifiedFileData(46-107)webview-ui/src/context/ExtensionStateContext.tsx (1)
useExtensionStateContext(102-108)
🪛 GitHub Check: Lint
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
[warning] 188-188:
Insert ···
[warning] 187-187:
Replace ···················:·effectiveActionTaken·===·"rejected" with ····················:·effectiveActionTaken·===·"rejected"⏎·····················
[warning] 186-186:
Replace ·?·"✓·Applied"· with ⏎····················?·"✓·Applied"
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
🔇 Additional comments (6)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (1)
163-171: Confirm message type alignment: OPEN_FILE_IN_EDITOR vs OPEN_FILEElsewhere we dispatch openFile(…) which emits OPEN_FILE. Ensure the extension’s webview handler implements OPEN_FILE_IN_EDITOR to prevent a no-op.
Would you like me to scan vscode/src/webviewMessageHandler.ts for the handler and open a quick follow-up PR if the message type needs alignment?
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (1)
126-128: Good consolidation of data selection in one hookThe simplified “hasNothingToView/hasContent” logic reads clean and matches the new chat-centric flow.
webview-ui/src/components/ResolutionsPage/ModifiedFile/index.ts (1)
6-11: Re-exports look correct and non-breakingDefault-export alias for ModifiedFileActions and forwarding detectLanguage/isLanguageSupported from shared are appropriate. Make sure the shared package indeed re-exports both helpers.
If you want, I can add a tiny type-only import check to CI to ensure these symbols resolve.
vscode/src/webviewMessageHandler.ts (3)
43-43: Good: toggleAgentMode imported and used without params (matches current config API).This aligns with the updated configuration helper which internally reads and toggles the value. Noted in prior learnings; no action needed.
189-205: Manual verification neededOur automated search did not find any occurrences of
.toString()being used for byte‐array→string decoding nor any other direct assignments tostatusonModifiedFilemessages outside ofhandleFileResponse. Please manually confirm that:
- No parts of the codebase still decode text via
Uint8Array.toString(),Buffer.toString(), or similar.- All updates to
ModifiedFilemessage statuses flow through the single canonical handler (handleFileResponse).
189-205: SHOW_DIFF_WITH_DECORATORS signature verifiedI’ve confirmed that the
konveyor.showDiffWithDecorationscommand handler invscode/src/commands.tsis declared with the signature(filePath: string, diff: string, content: string, messageToken: string)and that the invocation in
vscode/src/webviewMessageHandler.tspasses the arguments in exactly that same order:await vscode.commands.executeCommand( "konveyor.showDiffWithDecorations", path, diff, content, messageToken, );Since the parameter order and types align perfectly, no wiring changes are needed here.
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
Show resolved
Hide resolved
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
Show resolved
Hide resolved
ibolton336
force-pushed
the
decorators-3
branch
from
d0b6c41 to
8352944
Compare
…ns loaded When too many extensions are installed, the Konveyor extension may be hidden in the Additional Views overflow menu, making it inaccessible via activity bar selectors. This fix prioritizes using the command palette to open the analysis view, which works reliably regardless of extension visibility. Changes: - Modified openAnalysisView() to try command palette first - Falls back to existing activity bar approach if command palette fails - Fixed logic flow to only run fallback when needed Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
ibolton336
force-pushed
the
decorators-3
branch
from
8352944 to
489790f
Compare
Signed-off-by: Ian Bolton <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (12)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (6)
25-30: GuardhasActiveDecoratorsagainst empty/undefinedmessageTokenIndexing with an empty string can create confusing lookups; also keep console noise down elsewhere (see below).
Apply this diff:
- const hasActiveDecorators = !!(state.activeDecorators && state.activeDecorators[messageToken]); + const hasActiveDecorators = Boolean( + messageToken && state.activeDecorators?.[messageToken], + );
62-65: Remove verbose console logging in production pathsThis log will spam the console on each render and includes potentially large fields.
Apply this diff:
- console.log( - `[ModifiedFileMessage] Status check - messageToken: ${messageToken}, path: ${path}, data.status: ${status}, globalStatus: ${globalStatus}, actionTaken: ${actionTaken}, effectiveActionTaken: ${effectiveActionTaken}, foundMessage: ${!!currentMessage}`, - ); + // Consider adding debug logs behind a dev flag if needed.
113-124: Preserve empty-string selections and don’t emit FILE_RESPONSE without a token
- Using
||treats "" as falsy and falls back to full content.- Sending a FILE_RESPONSE with an empty/undefined
messageTokenbreaks host correlation.Apply this diff:
const applyFileChanges = (selectedContent?: string) => { setIsViewingDiff(false); - setActionTaken("applied"); - - // Use provided selected content or fall back to full content - const contentToApply = selectedContent || content; - - // Use FILE_RESPONSE flow for standalone apply button - postFileResponse("apply", messageToken, path, contentToApply); - // Trigger scroll after action - onUserAction?.(); + // Optional: set to "processing" until backend confirms status. + // setActionTaken("processing"); + setActionTaken("applied"); + + // Preserve intentionally empty selections + const contentToApply = selectedContent !== undefined ? selectedContent : content; + + if (messageToken) { + postFileResponse("apply", messageToken, path, contentToApply); + onUserAction?.(); + } else { + console.warn("[ModifiedFileMessage] Missing messageToken on apply; ignoring action."); + } };
126-134: Guard reject flow onmessageTokenAvoid emitting an uncorrelated reject; optionally use “processing” until confirmation.
Apply this diff:
const rejectFileChanges = () => { setIsViewingDiff(false); - setActionTaken("rejected"); + // setActionTaken("processing"); + setActionTaken("rejected"); - // Use FILE_RESPONSE flow for standalone reject button - postFileResponse("reject", messageToken, path); - // Trigger scroll after action - onUserAction?.(); + if (messageToken) { + postFileResponse("reject", messageToken, path); + onUserAction?.(); + } else { + console.warn("[ModifiedFileMessage] Missing messageToken on reject; ignoring action."); + } };
136-160: ValidatemessageTokenbefore SHOW_DIFF_WITH_DECORATORS and set view state after postingPrevent uncorrelated diff requests and avoid getting stuck in “viewing” state if validation fails.
Apply this diff:
const viewFileWithDecorations = (filePath: string, fileDiff: string) => { - if (isViewingDiff) { - return; - } - - setIsViewingDiff(true); + if (isViewingDiff) return; + if (!messageToken) { + console.warn("[ModifiedFileMessage] Missing messageToken on view; aborting SHOW_DIFF_WITH_DECORATORS."); + return; + } interface ShowDiffWithDecoratorsPayload { path: string; content: string; diff: string; messageToken: string; } const payload: ShowDiffWithDecoratorsPayload = { path: filePath, content: content, diff: fileDiff, messageToken: messageToken, }; window.vscode.postMessage({ type: "SHOW_DIFF_WITH_DECORATORS", payload, }); + setIsViewingDiff(true); };
161-176: GuardCONTINUE_WITH_FILE_STATEonmessageTokenAvoid sending a message the host can’t correlate.
Apply this diff:
const handleContinue = () => { - // Send CONTINUE_WITH_FILE_STATE message to check current file state - window.vscode.postMessage({ - type: "CONTINUE_WITH_FILE_STATE", - payload: { - messageToken, - path, - content, - }, - }); - // Trigger scroll after action - onUserAction?.(); + if (messageToken) { + window.vscode.postMessage({ + type: "CONTINUE_WITH_FILE_STATE", + payload: { messageToken, path, content }, + }); + onUserAction?.(); + } else { + console.warn("[ModifiedFileMessage] Missing messageToken on continue; ignoring action."); + } // Keep viewing diff state until backend responds with final status // This prevents the UI from reverting to action buttons prematurely };vscode/src/webviewMessageHandler.ts (6)
266-267: Normalization: trim multiple trailing newlinesTrimming a single newline may produce false positives when files end with multiple line breaks.
- const normalize = (text: string) => text.replace(/\r\n/g, "\n").replace(/\n$/, ""); + const normalize = (text: string) => text.replace(/\r\n/g, "\n").replace(/\n+$/, "");
275-276: Use logger instead of console.logStick to the centralized logger for consistency and channel control.
- console.log("Continue decision: ", { responseId, hasChanges }); + // (removed) use logger.debug/info above
284-296: Remove redundant chat status mutation; handleFileResponse already sets statushandleFileResponse updates ModifiedFile message status for both apply and reject (see vscode/src/utilities/ModifiedFiles/handleFileResponse.ts). Double mutation risks divergence.
- // Update the chat message status - state.mutateData((draft) => { - const messageIndex = draft.chatMessages.findIndex( - (msg) => msg.messageToken === messageToken, - ); - if ( - messageIndex >= 0 && - draft.chatMessages[messageIndex].kind === ChatMessageType.ModifiedFile - ) { - const modifiedFileMessage = draft.chatMessages[messageIndex].value as any; - modifiedFileMessage.status = hasChanges ? "applied" : "rejected"; - } - });
302-313: Remove redundant status mutation in error pathSame rationale as above; handleFileResponse("reject", …) already updates status.
- state.mutateData((draft) => { - const messageIndex = draft.chatMessages.findIndex( - (msg) => msg.messageToken === messageToken, - ); - if ( - messageIndex >= 0 && - draft.chatMessages[messageIndex].kind === ChatMessageType.ModifiedFile - ) { - const modifiedFileMessage = draft.chatMessages[messageIndex].value as any; - modifiedFileMessage.status = "rejected"; - } - });
255-260: Bug: Decoding file bytes via Uint8Array.toString() is incorrect; prefer unsaved editor text then UTF-8 decodeUint8Array.toString() doesn’t decode text; it returns a non-text representation, leading to wrong comparisons and content. Also, prefer unsaved editor content over disk to respect in-memory edits.
Apply this focused patch:
- // Get the current file content - const currentContent = await vscode.workspace.fs.readFile(uri); - const currentText = currentContent.toString(); + // Get the current file content: prefer unsaved editor text, fallback to filesystem + const openDoc = vscode.workspace.textDocuments.find((d) => d.uri.fsPath === path); + const currentText = openDoc + ? openDoc.getText() + : new TextDecoder("utf-8").decode(await vscode.workspace.fs.readFile(uri));Optionally scan the repo for similar patterns:
#!/bin/bash # All fs.readFile call sites rg -nP --type=ts 'workspace\.fs\.readFile\(' -C2 # Places already using safe decoding rg -nP --type=ts 'TextDecoder|Buffer\.from\(.+\)\.toString\(..utf-8|utf8..\)'
261-264: Guard missing originalContent to avoid false "apply" decisionsIf originalContent is absent, defaulting it to "" makes any non-empty file look changed. Fail fast and reject to avoid unintended applies.
- // Get the original content to compare against - const modifiedFileState = state.modifiedFiles.get(path); - const originalContent = modifiedFileState?.originalContent || ""; + // Get the original content to compare against + const modifiedFileState = state.modifiedFiles.get(path); + if (!modifiedFileState?.originalContent) { + logger.warn("CONTINUE_WITH_FILE_STATE: missing originalContent; defaulting to reject", { + path, + messageToken, + }); + await handleFileResponse(messageToken, "reject", path, content, state); + return; + } + const originalContent = modifiedFileState.originalContent;
🧹 Nitpick comments (5)
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (2)
26-35: Type safety: avoidanyfor state inuseResolutionDataUsing
anyhere discards compile-time guarantees. Prefer the concrete extension state type (e.g., ExtensionState) so downstream selectors remain safe and refactors are less error-prone.Would you like me to wire the proper type from ExtensionStateContext or extensionState.ts?
138-199: Trim unused dependency and guard against unknown kinds
isFetchingSolutionisn’t used insiderenderChatMessages; remove it from deps to avoid unnecessary re-creations.- Optional: add a default branch to log/ignore unknown
ChatMessageTypefor future-proofing.Apply this diff to the dependency list:
- }, [chatMessages, isFetchingSolution, isAnalyzing, triggerScrollOnUserAction]); + }, [chatMessages, isAnalyzing, triggerScrollOnUserAction]);webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (2)
31-42: Make message lookup robust: rely onmessageToken+ kind onlyMatching on
contentis brittle (content may change or be large).messageTokenshould uniquely identify the message.Apply this diff:
- const currentMessage = state.chatMessages.find( - (msg) => - msg.messageToken === messageToken && - msg.kind === ChatMessageType.ModifiedFile && - (msg.value as any)?.path === path && - (msg.value as any)?.content === content, - ); + const currentMessage = state.chatMessages.find( + (msg) => msg.messageToken === messageToken && msg.kind === ChatMessageType.ModifiedFile, + );
67-76: Stabilize effect deps to avoid unnecessary updatesDepending on
currentMessage(an object) andstate.chatMessages(an array) may retrigger frequently. Key offmessageTokenandglobalStatus(and optionallypath) to reduce churn.Apply this diff:
- }, [globalStatus, currentMessage, messageToken, path, state.chatMessages]); + }, [globalStatus, messageToken, path]);vscode/src/webviewMessageHandler.ts (1)
177-188: Fix log label and confirm whether rejection needs server/state notification
- Typo: log label says NEW_REJECT_FILE; action is REJECT_FILE.
- Confirm the design: If this action is used by the UI, do we also need to notify the solution server and update chat state via handleFileResponse("reject", …)? If this action is intentionally ephemeral, no change needed.
Apply this tiny fix:
- logger.error("Error handling NEW_REJECT_FILE:", error); + logger.error("Error handling REJECT_FILE:", error);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
tests/e2e/pages/vscode.page.ts(1 hunks)vscode/src/utilities/ModifiedFiles/handleFileResponse.ts(1 hunks)vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts(0 hunks)vscode/src/webviewMessageHandler.ts(4 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/DiffLegend.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/DiffLinesRenderer.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/EnhancedDiffRenderer.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/HunkSelectionInterface.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx(5 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModal.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModalHeader.css(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModalHeader.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/SingleHunkDisplay.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/enhancedSyntaxHighlighting.css(0 hunks)webview-ui/src/components/ResolutionsPage/ModifiedFile/index.ts(1 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(4 hunks)
💤 Files with no reviewable changes (10)
- webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModal.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/EnhancedDiffRenderer.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/DiffLinesRenderer.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModalHeader.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileModalHeader.css
- webview-ui/src/components/ResolutionsPage/ModifiedFile/SingleHunkDisplay.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/enhancedSyntaxHighlighting.css
- webview-ui/src/components/ResolutionsPage/ModifiedFile/DiffLegend.tsx
- vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts
- webview-ui/src/components/ResolutionsPage/ModifiedFile/HunkSelectionInterface.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/pages/vscode.page.ts
- vscode/src/utilities/ModifiedFiles/handleFileResponse.ts
- webview-ui/src/components/ResolutionsPage/ModifiedFile/index.ts
- webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileActions.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-01T13:53:20.774Z
Learnt from: djzager
PR: konveyor/editor-extensions#645
File: vscode/src/webviewMessageHandler.ts:575-577
Timestamp: 2025-08-01T13:53:20.774Z
Learning: The TOGGLE_AGENT_MODE action handler in vscode/src/webviewMessageHandler.ts correctly calls toggleAgentMode() without parameters, as the toggleAgentMode function in vscode/src/utilities/configuration.ts has been updated to internally handle reading and toggling the agent mode configuration value.
Applied to files:
vscode/src/webviewMessageHandler.tswebview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx
📚 Learning: 2025-08-12T02:13:55.897Z
Learnt from: djzager
PR: konveyor/editor-extensions#678
File: vscode/src/commands.ts:135-139
Timestamp: 2025-08-12T02:13:55.897Z
Learning: In vscode/src/commands.ts, the solutionState is conditionally reset to "none" after successful getSolution completion, but only when !state.isWaitingForUserInteraction. This prevents permanent blocking of future getSolution calls while still maintaining proper concurrency control during active workflows.
Applied to files:
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
🧬 Code graph analysis (3)
vscode/src/webviewMessageHandler.ts (2)
vscode/src/client/tracer.ts (1)
logger(60-65)vscode/src/utilities/ModifiedFiles/handleFileResponse.ts (1)
handleFileResponse(92-225)
webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (3)
shared/src/types/types.ts (2)
ModifiedFileMessageValue(294-304)LocalChange(99-108)webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts (1)
useModifiedFileData(46-107)webview-ui/src/context/ExtensionStateContext.tsx (1)
useExtensionStateContext(102-108)
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (7)
webview-ui/src/hooks/useScrollManagement.ts (1)
useScrollManagement(5-259)shared/src/types/types.ts (3)
Incident(5-10)ToolMessageValue(292-292)ModifiedFileMessageValue(294-304)webview-ui/src/hooks/actions.ts (1)
openFile(52-58)webview-ui/src/components/ResolutionsPage/MessageWrapper.tsx (1)
MessageWrapper(9-11)webview-ui/src/components/ResolutionsPage/ToolMessage.tsx (1)
ToolMessage(116-181)webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (1)
ModifiedFileMessage(17-253)webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (1)
ReceivedMessage(22-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test (linux)
- GitHub Check: Test (windows)
- GitHub Check: Test (macos)
🔇 Additional comments (10)
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (5)
43-51: Readable, correct empty/content checksThe simplified
hasNothingToViewandhasContentconditions are clear and correct for the new chat-first flow.
126-128: Nice consolidation to a single data surfacePulling just the fields needed by the page improves readability and reduces prop churn.
202-202: Consistent page class namingThe
resolutions-pageclass aligns with existing CSS naming; good for consistency.
231-235: Good UX for empty stateShowing a concise “No resolutions available.” message when nothing is present is a helpful baseline.
175-196: Disable ‘run-analysis’ during analysis — goodThe quick-response disabling based on
isAnalyzingprevents duplicate runs. Solid guard.webview-ui/src/components/ResolutionsPage/ModifiedFile/ModifiedFileMessage.tsx (2)
188-206: Minimized state shows “Processing…” but local handlers set final states immediatelyYou render a processing variant, but local apply/reject set “applied”/“rejected” right away. If the host rejects or errors, UI may briefly be inconsistent. Consider setting “processing” until you receive the corresponding status from global state.
I can wire this end-to-end if you confirm the host will always emit a final status event for apply/reject.
237-247: Props passed to ModifiedFileActions look correctHandlers and state flags cover the new decorator-based flow well.
vscode/src/webviewMessageHandler.ts (3)
43-43: TOGGLE_AGENT_MODE import and usage LGTMImporting toggleAgentMode and invoking it with no arguments (see Lines 248-250) matches the current configuration API and our retrieved learning from Aug 1, 2025. No changes needed.
189-205: SHOW_DIFF_WITH_DECORATORS delegation looks goodCleanly delegates to konveyor.showDiffWithDecorations with a minimal wrapper and proper error surfacing. Matches the new vertical-diff architecture.
175-176: No lingering references to removed diff actions
I ran targeted searches for the old action constants (APPLY_FILE,DISCARD_FILE,SHOW_MAXIMIZED_DIFF,VIEW_FILE) and legacy command IDs (konveyor.applyDiscardDiff,konveyor.discardDiffView, etc.) and found no code or webview callers outside of the comment at lines 175–176 inwebviewMessageHandler.ts. The only matches inpackage.jsonare the newkonveyor.diffView.*commands introduced for the unified decorator flow, which is expected.Everything related to the old diff actions has been cleanly removed—no further changes are required here.
There was a problem hiding this comment.
I tested this with latest commits and this is all working great. I can confirm all major issues we spotted so far have been resolved. There is one small issue in setupModelProvider thats moved to onWillSaveTextDocument which is causing the provider to not reload correctly when config changes on disk. Since I am out next week, dont want to block on my approval. But itd be great if you could keep the setup provider part in onDidSave as it was before so we dont need to have a follow up issue for it.
Signed-off-by: Ian Bolton <[email protected]>
Resolves:
konveyor/kai#844
#689
#588
Summary by CodeRabbit
New Features
UI
UX
Removals