✨ Interactive diagnostic issue selection UX #647
Conversation
WalkthroughAdds diagnostic selection and messaging support across shared types, the VS Code extension, and the webview UI. Introduces a Diagnostic chat message type, DiagnosticIssue/DiagnosticSummary/DiagnosticMessageValue interfaces, and KONVEYOR_OUTPUT_CHANNEL_NAME. Adds summarizeTasksStructured to produce structured diagnostics and updates summarizeTasks to delegate to it. Propagates optional selectedIssues through webview actions, message handlers, and handleQuickResponse. Adds DiagnosticMessage and DiagnosticIssuesView React components (with CSS) to render and select issues, integrates them into ReceivedMessage/ResolutionsPage, and re-exports related components and types. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
.vscode/launch.json (1)
20-22: Optional: expand glob to cover CommonJS bundleIf you ever need to debug the CJS artifacts (
*.cjs) emitted bytsup, consider adding the pattern below to both arrays:- "${workspaceFolder}/agentic/dist/**/*.js" + "${workspaceFolder}/agentic/dist/**/*.{js,cjs}"webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx (1)
1-62: Well-implemented modal component!The modal implementation is clean and follows React best practices. The click-outside-to-close pattern with
stopPropagationis properly implemented.Consider adding ARIA attributes for better accessibility:
- <div className="diagnostic-modal-overlay" onClick={onClose}> - <div className="diagnostic-modal" onClick={(e) => e.stopPropagation()}> + <div className="diagnostic-modal-overlay" onClick={onClose} role="dialog" aria-modal="true"> + <div className="diagnostic-modal" onClick={(e) => e.stopPropagation()} role="document">webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (3)
107-120: Remove unused dependency from useCallbackThe
diagnosticSummaryis not used in the callback function and should be removed from the dependency array:- [selectedIssues, diagnosticSummary], + [selectedIssues],
253-257: Avoid hard-coding response ID checkThe hard-coded check for
response.id === "yes"is fragile. Consider a more maintainable approach:disabled={ isProcessing || selectedResponse !== null || - (response.id === "yes" && selectedIssues.size === 0) + (response.requiresSelection && selectedIssues.size === 0) }This would require adding a
requiresSelectionproperty to the QuickResponse type to explicitly indicate which responses need selected issues.
127-266: Consider accessibility enhancementsThe component structure is solid, but consider these accessibility improvements:
- Add ARIA labels to interactive elements:
-<button className="diagnostic-action-btn" onClick={handleSelectAll}> +<button className="diagnostic-action-btn" onClick={handleSelectAll} aria-label="Select all diagnostic issues">
- Add keyboard navigation support for the file list
- Consider using
role="group"for the issues list with appropriate ARIA labels
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.vscode/launch.json(1 hunks)agentic/package.json(1 hunks)agentic/src/nodes/diagnosticsIssueFix.ts(3 hunks)agentic/src/types.ts(1 hunks)shared/src/index.ts(1 hunks)shared/src/types/types.ts(2 hunks)vscode/src/taskManager/utils.ts(3 hunks)vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts(3 hunks)vscode/src/utilities/ModifiedFiles/processMessage.ts(3 hunks)vscode/src/webviewMessageHandler.ts(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx(4 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/diagnosticFileModal.css(1 hunks)webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css(1 hunks)webview-ui/src/hooks/actions.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (1)
webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (1)
ReceivedMessage(39-116)
webview-ui/src/hooks/actions.ts (1)
shared/src/types/actions.ts (2)
WebviewAction(56-59)WebviewActionType(28-55)
vscode/src/webviewMessageHandler.ts (1)
vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts (1)
handleQuickResponse(8-97)
webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx (1)
shared/src/types/types.ts (1)
DiagnosticIssue(316-322)
vscode/src/utilities/ModifiedFiles/processMessage.ts (2)
vscode/src/taskManager/types.ts (1)
TasksList(22-25)vscode/src/taskManager/utils.ts (1)
summarizeTasksStructured(21-72)
vscode/src/taskManager/utils.ts (2)
shared/src/types/types.ts (2)
DiagnosticIssue(316-322)DiagnosticSummary(324-328)vscode/src/taskManager/types.ts (1)
TasksList(22-25)
shared/src/types/types.ts (2)
vscode/src/taskManager/utils.ts (2)
DiagnosticIssue(4-9)DiagnosticSummary(11-15)shared/src/index.ts (3)
DiagnosticIssue(7-7)DiagnosticSummary(7-7)DiagnosticMessageValue(7-7)
🔇 Additional comments (21)
agentic/package.json (1)
25-26: Source-map generation flag is a solid additionIncluding
--sourcemapin both the build and watch scripts will let VS Code map breakpoints back to TypeScript, matching the updated launch config. Nothing else to flag..vscode/launch.json (1)
14-17: Addition ofagentic/distglobs aligns debugging paths – looks goodThe new glob ensures the extension host can locate compiled JS from the
agenticpackage when debugging.webview-ui/src/components/ResolutionsPage/diagnosticFileModal.css (1)
1-141: LGTM! Well-structured CSS for modal dialog.The CSS follows VSCode theming conventions consistently and provides a clean, accessible modal interface. The layout uses flexbox appropriately, includes proper hover states, and maintains visual hierarchy through appropriate spacing and typography.
shared/src/index.ts (1)
7-7: LGTM! Proper type export for diagnostic types.The export enables consistent usage of diagnostic types across the codebase, supporting the new diagnostic issue selection workflow.
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (1)
301-301: LGTM! Proper integration of diagnostic summary data.The extraction and passing of
diagnosticSummarycorrectly integrates with the new diagnostic issue selection workflow. The type casting toanyis acceptable given the dynamic nature of message values, though consider adding proper typing in future iterations.Also applies to: 307-307
agentic/src/types.ts (1)
58-58: LGTM! Appropriate extension of user interaction interface.The addition of
selectedIssuesas an optional string array properly supports the diagnostic issue selection workflow while maintaining backward compatibility.vscode/src/webviewMessageHandler.ts (1)
425-426: LGTM! Proper integration of selectedIssues parameter.The addition of
selectedIssuesto the QUICK_RESPONSE handler correctly extends the quick response flow to support diagnostic issue selection, maintaining consistency with thehandleQuickResponsefunction signature.webview-ui/src/hooks/actions.ts (2)
73-77: LGTM - Well-structured interface for quick response payload.The
QuickResponsePayloadinterface is properly typed with appropriate optionality forselectedIssues. The field names are clear and consistent with the broader messaging pattern.
86-91: LGTM - Action creator follows established patterns.The
quickResponsefunction correctly implements the action creator pattern used throughout the file and properly types the payload with the newQuickResponsePayloadinterface.vscode/src/utilities/ModifiedFiles/processMessage.ts (2)
51-61: LGTM - Enhanced function provides structured diagnostic data.The refactored
createTasksMessagefunction now returns both a user-facing message and structured diagnostic data, enabling richer UI interactions. TheObject.fromEntriesconversion ensures theMapis properly serialized for message passing.The updated user prompt clearly instructs users to review and select issues, which aligns with the new selection workflow.
119-127: LGTM - Correctly updated to use structured message data.The function properly destructures the enhanced return value from
createTasksMessageand includes both the message text and diagnostic summary in the chat message value, enabling the UI to render structured diagnostic information.webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css (1)
1-249: LGTM - Comprehensive and well-structured CSS for diagnostic UI.The stylesheet provides thorough styling for the diagnostic issues view with:
- Consistent use of VSCode CSS variables for theming
- Proper interactive states (hover, disabled, selected)
- Responsive design with appropriate max-heights and overflow handling
- Clean flexbox layouts for alignment
- Semantic class naming conventions
The CSS follows modern best practices and maintains consistency with VSCode's design system.
vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts (3)
12-12: LGTM - Clean parameter addition for selected issues.The optional
selectedIssuesparameter maintains backward compatibility while enabling the new diagnostic issue selection functionality.
29-46: LGTM - Proper handling of selected issues in response data.The function correctly includes
selectedIssuesin the response data for tasks interactions and maintains proper type safety with the updated union type annotation.
71-71: LGTM - Selected issues propagated to interaction resolver.The
selectedIssuesare properly passed through to the pending interaction resolver, completing the flow from UI to backend processing.agentic/src/nodes/diagnosticsIssueFix.ts (2)
141-169: LGTM - Efficient filtering logic with proper task matching.The implementation correctly filters diagnostic tasks based on user selections:
- Uses
Setfor efficient O(1) lookup performance- Properly constructs task identifiers using
${group.uri}-${task}format- Removes empty groups after filtering to maintain data integrity
- Includes helpful debug logging for troubleshooting
The filtering ensures only user-approved issues are processed downstream.
266-266: LGTM - Important prompt clarification added.The added instruction explicitly prevents the AI from fixing unselected issues, which is crucial for respecting user intentions and avoiding unintended modifications.
shared/src/types/types.ts (1)
137-137: LGTM!The new
Diagnosticenum value follows the established naming pattern for chat message types.vscode/src/taskManager/utils.ts (1)
74-80: LGTM!Good approach to maintain backward compatibility by wrapping the new structured function.
webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (2)
66-77: LGTM!The quick response handler correctly includes selected issue IDs in the message payload.
85-114: Well-structured component integration!The conditional rendering logic cleanly separates regular quick responses from diagnostic-specific UI. The component properly delegates issue selection handling to
DiagnosticIssuesViewwhen diagnostic data is present.
| const uniqueTasks = Array.from(new Set(taskList)); | ||
| const fileIssues: DiagnosticIssue[] = uniqueTasks.map((task) => ({ | ||
| id: `${uri}-${task}`, | ||
| message: task.length > 200 ? task.slice(0, 197) + "..." : task, | ||
| uri, | ||
| filename, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve ID generation for diagnostic issues
The current ID generation using ${uri}-${task} could be problematic:
- Task strings can be very long, creating unwieldy IDs
- Special characters in tasks might cause issues
- Not guaranteed unique if the same task appears multiple times
Consider using a more robust approach:
- const fileIssues: DiagnosticIssue[] = uniqueTasks.map((task) => ({
- id: `${uri}-${task}`,
+ const fileIssues: DiagnosticIssue[] = uniqueTasks.map((task, index) => ({
+ id: `${filename}-issue-${index}`,
message: task.length > 200 ? task.slice(0, 197) + "..." : task,
uri,
filename,
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const uniqueTasks = Array.from(new Set(taskList)); | |
| const fileIssues: DiagnosticIssue[] = uniqueTasks.map((task) => ({ | |
| id: `${uri}-${task}`, | |
| message: task.length > 200 ? task.slice(0, 197) + "..." : task, | |
| uri, | |
| filename, | |
| })); | |
| const uniqueTasks = Array.from(new Set(taskList)); | |
| const fileIssues: DiagnosticIssue[] = uniqueTasks.map((task, index) => ({ | |
| id: `${filename}-issue-${index}`, | |
| message: task.length > 200 ? task.slice(0, 197) + "..." : task, | |
| uri, | |
| filename, | |
| })); |
🤖 Prompt for AI Agents
In vscode/src/taskManager/utils.ts around lines 39 to 45, the ID generation for
diagnostic issues uses `${uri}-${task}`, which can produce overly long IDs,
include problematic special characters, and may not be unique if tasks repeat.
To fix this, generate IDs by combining the URI with a stable hash or index of
the task instead of the full task string. This ensures shorter, safe, and unique
IDs for each diagnostic issue.
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx
Outdated
Show resolved
Hide resolved
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx
Outdated
Show resolved
Hide resolved
ibolton336
force-pushed
the
browse-diagnostics
branch
2 times, most recently
from
b2443ce to
44fc192
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (1)
10-23: Import types from shared module instead of duplicating.The DiagnosticIssue and DiagnosticSummary interfaces should be imported from the shared module to maintain consistency and avoid duplication.
-// Import diagnostic types from the shared types file -interface DiagnosticIssue { - id: string; - message: string; - uri: string; - filename: string; - selected?: boolean; -} - -interface DiagnosticSummary { - summary: string; - issuesByFile: Record<string, DiagnosticIssue[]>; - totalIssues: number; -} +import { DiagnosticIssue, DiagnosticSummary } from "@editor-extensions/shared";
🧹 Nitpick comments (2)
webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx (2)
1-62: Fix linting issues for code consistency.The static analysis tool has identified several formatting and style issues that should be addressed:
Apply these fixes to resolve the linting warnings:
-import React from 'react'; -import { DiagnosticIssue } from '@editor-extensions/shared'; -import './diagnosticFileModal.css'; +import React from "react"; +import { DiagnosticIssue } from "@editor-extensions/shared"; +import "./diagnosticFileModal.css";- if (!isOpen) return null; + if (!isOpen) { + return null; + }Fix indentation and formatting in the JSX:
- <div className="modal-content"> - <div className="issues-list"> + <div className="modal-content"> + <div className="issues-list"> {issues.map((issue, index) => ( <div key={issue.id} className="issue-detail"> <div className="issue-number">#{index + 1}</div> <div className="issue-content"> <div className="issue-message">{issue.message}</div> - <div className="issue-uri"> - File: {issue.uri} - </div> + <div className="issue-uri">File: {issue.uri}</div> </div> </div> ))} </div> - {issues.length === 0 && ( - <div className="no-issues"> - No diagnostic issues found in this file. - </div> + <div className="no-issues">No diagnostic issues found in this file.</div> )} </div> - <div className="modal-footer">
20-58: Consider enhancing accessibility features.The modal could benefit from additional accessibility improvements:
+import React, { useEffect, useRef } from "react"; export const DiagnosticFileModal: React.FC<DiagnosticFileModalProps> = ({ isOpen, filename, issues, onClose, }) => { + const modalRef = useRef<HTMLDivElement>(null); + + useEffect(() => { + const handleEscape = (event: KeyboardEvent) => { + if (event.key === 'Escape') { + onClose(); + } + }; + + if (isOpen) { + document.addEventListener('keydown', handleEscape); + modalRef.current?.focus(); + } + + return () => { + document.removeEventListener('keydown', handleEscape); + }; + }, [isOpen, onClose]); if (!isOpen) { return null; } return ( - <div className="diagnostic-modal-overlay" onClick={onClose}> - <div className="diagnostic-modal" onClick={(e) => e.stopPropagation()}> + <div className="diagnostic-modal-overlay" onClick={onClose} role="dialog" aria-modal="true"> + <div + className="diagnostic-modal" + onClick={(e) => e.stopPropagation()} + ref={modalRef} + tabIndex={-1} + aria-labelledby="modal-title" + > <div className="modal-header"> - <h2>Diagnostic Issues in {filename}</h2> - <button className="modal-close-btn" onClick={onClose}> - × + <h2 id="modal-title">Diagnostic Issues in {filename}</h2> + <button className="modal-close-btn" onClick={onClose} aria-label="Close modal"> + <span aria-hidden="true">×</span> </button>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.vscode/launch.json(1 hunks)agentic/package.json(1 hunks)agentic/src/nodes/diagnosticsIssueFix.ts(3 hunks)agentic/src/types.ts(1 hunks)shared/src/index.ts(1 hunks)shared/src/types/types.ts(2 hunks)vscode/src/taskManager/utils.ts(3 hunks)vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts(3 hunks)vscode/src/utilities/ModifiedFiles/processMessage.ts(3 hunks)vscode/src/webviewMessageHandler.ts(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx(4 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/diagnosticFileModal.css(1 hunks)webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css(1 hunks)webview-ui/src/hooks/actions.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- agentic/package.json
- webview-ui/src/components/ResolutionsPage/diagnosticFileModal.css
- webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css
🚧 Files skipped from review as they are similar to previous changes (12)
- shared/src/index.ts
- agentic/src/types.ts
- webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
- .vscode/launch.json
- vscode/src/utilities/ModifiedFiles/processMessage.ts
- vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts
- webview-ui/src/hooks/actions.ts
- shared/src/types/types.ts
- agentic/src/nodes/diagnosticsIssueFix.ts
- webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx
- vscode/src/webviewMessageHandler.ts
- vscode/src/taskManager/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx (1)
shared/src/types/types.ts (1)
DiagnosticIssue(316-322)
🪛 GitHub Check: Lint
webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx
[warning] 51-51:
Delete ········
[warning] 46-46:
Replace ⏎··············No·diagnostic·issues·found·in·this·file.⏎············ with No·diagnostic·issues·found·in·this·file.
[warning] 44-44:
Delete ··········
[warning] 37-37:
Replace ⏎····················File:·{issue.uri}⏎·················· with File:·{issue.uri}
[warning] 29-29:
Delete ········
[warning] 18-18:
Expected { after 'if' condition
[warning] 3-3:
Replace './diagnosticFileModal.css' with "./diagnosticFileModal.css"
[warning] 2-2:
Replace '@editor-extensions/shared' with "@editor-extensions/shared"
[warning] 1-1:
Replace 'react' with "react"
⏰ 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 (windows)
- GitHub Check: Test (linux)
- GitHub Check: Test (macos)
🔇 Additional comments (5)
webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx (2)
5-10: LGTM! Well-defined props interface.The
DiagnosticFileModalPropsinterface is clearly defined with appropriate types for modal state management and data display.
18-60: LGTM! Solid modal implementation.The component follows React best practices with proper event handling, key usage in lists, and conditional rendering for empty states. The modal overlay behavior with click-to-close and stopPropagation is correctly implemented.
webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (3)
36-36: LGTM! Well-integrated diagnostic support.The new
diagnosticSummaryprop andselectedIssuesstate management are properly implemented with appropriate typing and handlers.Also applies to: 49-49, 79-81
70-77: LGTM! Smart integration of selected issues with quick responses.The quick response handler correctly includes selected issue IDs in the message payload, and the conditional rendering logic appropriately delegates quick response handling to the DiagnosticIssuesView when diagnostic data is present.
Also applies to: 92-102
106-113: LGTM! Clean integration with DiagnosticIssuesView.The conditional rendering and prop passing for the DiagnosticIssuesView component is well-implemented, providing all necessary data and callbacks for proper functionality.
ibolton336
force-pushed
the
browse-diagnostics
branch
2 times, most recently
from
16cd467 to
a8dc28b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (1)
10-23: Import types from shared module instead of duplicating.The local interface definitions should be removed and imported from the shared module as previously suggested. This maintains consistency and avoids code duplication.
-// Import diagnostic types from the shared types file -interface DiagnosticIssue { - id: string; - message: string; - uri: string; - filename: string; - selected?: boolean; -} - -interface DiagnosticSummary { - summary: string; - issuesByFile: Record<string, DiagnosticIssue[]>; - totalIssues: number; -} +import { DiagnosticIssue, DiagnosticSummary } from "@editor-extensions/shared";
🧹 Nitpick comments (3)
webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx (3)
1-3: Fix linting issues with quote consistency.The linting tool suggests using double quotes consistently throughout the import statements.
-import React from 'react'; -import { DiagnosticIssue } from '@editor-extensions/shared'; -import './diagnosticFileModal.css'; +import React from "react"; +import { DiagnosticIssue } from "@editor-extensions/shared"; +import "./diagnosticFileModal.css";
18-18: Add braces for consistency with linting rules.The linting tool expects braces around if statement bodies for consistency.
- if (!isOpen) return null; + if (!isOpen) { + return null; + }
37-39: Fix indentation and formatting issues.The linting tool indicates formatting inconsistencies with indentation and line breaks.
<div className="issue-uri"> - File: {issue.uri} + File: {issue.uri} </div>{issues.length === 0 && ( - <div className="no-issues"> - No diagnostic issues found in this file. - </div> + <div className="no-issues">No diagnostic issues found in this file.</div> )}Also applies to: 45-47
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.vscode/launch.json(1 hunks)agentic/package.json(1 hunks)agentic/src/nodes/diagnosticsIssueFix.ts(3 hunks)agentic/src/types.ts(1 hunks)shared/src/index.ts(1 hunks)shared/src/types/types.ts(2 hunks)vscode/src/taskManager/utils.ts(3 hunks)vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts(3 hunks)vscode/src/utilities/ModifiedFiles/processMessage.ts(3 hunks)vscode/src/webviewMessageHandler.ts(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx(4 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/diagnosticFileModal.css(1 hunks)webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css(1 hunks)webview-ui/src/hooks/actions.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css
- webview-ui/src/hooks/actions.ts
- webview-ui/src/components/ResolutionsPage/diagnosticFileModal.css
- .vscode/launch.json
🚧 Files skipped from review as they are similar to previous changes (11)
- agentic/package.json
- shared/src/index.ts
- webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
- agentic/src/types.ts
- vscode/src/webviewMessageHandler.ts
- vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts
- vscode/src/taskManager/utils.ts
- agentic/src/nodes/diagnosticsIssueFix.ts
- shared/src/types/types.ts
- vscode/src/utilities/ModifiedFiles/processMessage.ts
- webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx (1)
shared/src/types/types.ts (1)
DiagnosticIssue(316-322)
🪛 GitHub Check: Lint
webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx
[warning] 51-51:
Delete ········
[warning] 46-46:
Replace ⏎··············No·diagnostic·issues·found·in·this·file.⏎············ with No·diagnostic·issues·found·in·this·file.
[warning] 44-44:
Delete ··········
[warning] 37-37:
Replace ⏎····················File:·{issue.uri}⏎·················· with File:·{issue.uri}
[warning] 29-29:
Delete ········
[warning] 18-18:
Expected { after 'if' condition
[warning] 3-3:
Replace './diagnosticFileModal.css' with "./diagnosticFileModal.css"
[warning] 2-2:
Replace '@editor-extensions/shared' with "@editor-extensions/shared"
[warning] 1-1:
Replace 'react' with "react"
🔇 Additional comments (6)
webview-ui/src/components/ResolutionsPage/DiagnosticFileModal.tsx (1)
12-60: LGTM! Well-structured modal component.The modal component follows React best practices with:
- Proper TypeScript interface definition
- Correct event handling with stopPropagation to prevent unwanted closes
- Good accessibility with semantic HTML structure
- Appropriate empty state handling
- Clean separation of header, content, and footer sections
The component integrates well with the shared DiagnosticIssue type and provides a focused view for file-specific diagnostic issues.
webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (5)
48-49: LGTM! Proper state management for diagnostic features.The addition of
selectedIssuesstate alongside the existingselectedResponsestate is well-implemented and follows React patterns.
70-77: LGTM! Enhanced quick response handling with selected issues.The updated
handleQuickResponsefunction correctly includes the selected diagnostic issue IDs in the message payload, enabling the backend to process user selections appropriately.
79-81: LGTM! Clean callback handler for issue selection.The
handleIssueSelectionChangefunction provides a clean interface for the childDiagnosticIssuesViewcomponent to update the parent's selected issues state.
91-102: LGTM! Intelligent conditional rendering of quick responses.The logic properly prevents duplicate quick response buttons by conditionally disabling them in the main Message component when diagnostic summary is present, while still passing the responses to the DiagnosticIssuesView for appropriate rendering.
106-113: LGTM! Well-integrated diagnostic issues view.The conditional rendering of
DiagnosticIssuesViewwith proper prop passing maintains the component's existing functionality while seamlessly adding the new diagnostic selection capabilities.
ibolton336
force-pushed
the
browse-diagnostics
branch
from
a8dc28b to
e00a94c
Compare
ibolton336
changed the title
ibolton336
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (2)
4-23: Import types from shared module to avoid duplication.The comment indicates these types should be imported from the shared module, and the relevant code snippets confirm that
DiagnosticIssueandDiagnosticSummaryare already defined inshared/src/types/types.ts.
125-155: Refactor duplicated selection logic to improve maintainability.This code duplicates the selection update and parent notification logic, making it harder to maintain and potentially error-prone.
🧹 Nitpick comments (3)
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (2)
66-78: Consider enhancing to use actual issue line numbers.The current implementation defaults to line 1, but diagnostic issues likely contain specific line information that could provide better navigation experience.
If diagnostic issues contain line number information, consider enhancing this to use the actual line:
const handleFileClick = useCallback((filename: string, issues: DiagnosticIssue[]) => { const firstIssue = issues[0]; if (firstIssue) { window.vscode.postMessage({ type: "OPEN_FILE", payload: { file: firstIssue.uri, - line: 1, // Default to line 1, could be enhanced to find the specific line with the issue + line: firstIssue.line || 1, // Use actual line number if available }, }); } }, []);
196-209: Simplify selection summary calculation for better readability.The IIFE pattern makes this code harder to read and maintain. Consider extracting the calculation for better clarity.
{selectedIssues.size > 0 && ( <div className="selection-summary"> - {(() => { - const selectedFiles = Object.entries(diagnosticSummary.issuesByFile).filter(([filename, issues]) => - issues.some(issue => selectedIssues.has(issue.id)) - ); - return ( - <div> - {selectedIssues.size} issue{selectedIssues.size !== 1 ? 's' : ''} selected across {selectedFiles.length} file{selectedFiles.length !== 1 ? 's' : ''} - </div> - ); - })()} + {(() => { + const selectedFiles = Object.entries(diagnosticSummary.issuesByFile).filter(([_, issues]) => + issues.some(issue => selectedIssues.has(issue.id)) + ); + const issueText = selectedIssues.size === 1 ? 'issue' : 'issues'; + const fileText = selectedFiles.length === 1 ? 'file' : 'files'; + + return ( + <div> + {selectedIssues.size} {issueText} selected across {selectedFiles.length} {fileText} + </div> + ); + })()} </div> )}Or better yet, extract to a separate variable:
+ const selectedFiles = Object.entries(diagnosticSummary.issuesByFile).filter(([_, issues]) => + issues.some(issue => selectedIssues.has(issue.id)) + ); + const issueText = selectedIssues.size === 1 ? 'issue' : 'issues'; + const fileText = selectedFiles.length === 1 ? 'file' : 'files'; {selectedIssues.size > 0 && ( <div className="selection-summary"> - {(() => { - const selectedFiles = Object.entries(diagnosticSummary.issuesByFile).filter(([filename, issues]) => - issues.some(issue => selectedIssues.has(issue.id)) - ); - return ( - <div> - {selectedIssues.size} issue{selectedIssues.size !== 1 ? 's' : ''} selected across {selectedFiles.length} file{selectedFiles.length !== 1 ? 's' : ''} - </div> - ); - })()} + <div> + {selectedIssues.size} {issueText} selected across {selectedFiles.length} {fileText} + </div> </div> )}webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx (1)
102-123: Consider extracting complex disabled logic for better readability.The quick response logic is correct, especially the validation that prevents "yes" responses when no issues are selected. However, the complex boolean expression could be more readable.
Consider extracting the disabled logic for better readability:
+ const isResponseDisabled = (response: QuickResponseWithToken) => { + return response.isDisabled || + isProcessing || + selectedResponse !== null || + (response.id === "yes" && selectedIssues.length === 0); + }; quickResponses={ quickResponses.map((response) => ({ ...response, onClick: () => { handleQuickResponse(response.id, response.messageToken); }, - isDisabled: response.isDisabled || isProcessing || selectedResponse !== null || - (response.id === "yes" && selectedIssues.length === 0), + isDisabled: isResponseDisabled(response), content: response.content, })) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts(3 hunks)vscode/src/utilities/ModifiedFiles/processMessage.ts(3 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx(5 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- webview-ui/src/components/ResolutionsPage/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
- vscode/src/utilities/ModifiedFiles/processMessage.ts
- vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts
- webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (2)
vscode/src/taskManager/utils.ts (2)
DiagnosticIssue(4-9)DiagnosticSummary(11-15)shared/src/types/types.ts (2)
DiagnosticIssue(316-322)DiagnosticSummary(324-328)
⏰ 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 (9)
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (4)
25-31: LGTM! Well-structured component setup.The props interface is well-defined with appropriate optional parameters, and using
Set<string>for selectedIssues and expandedFiles provides efficient O(1) lookups for state management.
33-51: LGTM! Efficient issue selection handling.The toggle logic is correct and efficient, properly using
useCallbackwith all necessary dependencies. The parent notification mechanism works as expected.
53-64: LGTM! Simple and correct file toggle implementation.The file expansion toggle logic is straightforward and correctly implemented with proper
useCallbackusage.
80-96: LGTM! Bulk selection operations are well implemented.Both
handleSelectAllandhandleSelectNonefunctions correctly update the selection state and notify the parent component with appropriate callback usage.webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx (5)
40-57: LGTM! Well-structured component with good empty state handling.The component props are well-defined, and the early return logic that prevents rendering phantom blank messages is a good practice for UX.
59-66: LGTM! Clean timestamp formatting utility.The function properly handles both string and Date inputs and formats consistently.
68-79: LGTM! Proper quick response handling with issue selection.The function correctly includes selected issue IDs in the message payload, which aligns with the PR's objective of enabling task selection based on diagnostic issues.
81-83: LGTM! Simple and correct callback implementation.The issue selection change handler properly updates state for parent-child component communication.
85-101: LGTM! Proper integration of diagnostic issues view.The main message rendering correctly integrates the
DiagnosticIssuesViewcomponent with appropriate props and callback handling.
webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx
Outdated
Show resolved
Hide resolved
ibolton336
force-pushed
the
browse-diagnostics
branch
2 times, most recently
from
76fb6e7 to
6d0ee7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (2)
69-81: Consider enhancing file positioning for better UXThe current implementation opens files at line 1. Consider extracting line information from diagnostic issues to position the cursor at the actual issue location for improved user experience.
const handleFileClick = useCallback((filename: string, issues: DiagnosticIssue[]) => { // Open the file directly in VSCode const firstIssue = issues[0]; if (firstIssue) { + // Extract line number if available in the issue message or URI + const lineMatch = firstIssue.message.match(/line (\d+)/i) || firstIssue.uri.match(/:(\d+):/); + const line = lineMatch ? parseInt(lineMatch[1], 10) : 1; + window.vscode.postMessage({ type: "OPEN_FILE", payload: { file: firstIssue.uri, - line: 1, // Default to line 1, could be enhanced to find the specific line with the issue + line, }, }); } }, []);
204-214: Fix unused variable flagged by static analysisThe
filenamevariable is destructured but never used in the filter function for calculating selected files.const selectedFiles = Object.entries(diagnosticSummary.issuesByFile).filter( - ([filename, issues]) => issues.some((issue) => selectedIssues.has(issue.id)), + ([, issues]) => issues.some((issue) => selectedIssues.has(issue.id)), );webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx (1)
1-24: Clean imports but unused prop detectedThe imports and type definitions are well-structured. However, the
isLoadingprop is defined but never used in the component.interface DiagnosticMessageProps { content?: string; extraContent?: React.ReactNode; - isLoading?: boolean; timestamp?: string | Date; quickResponses?: QuickResponseWithToken[]; isMessageResponded?: boolean; diagnosticSummary: DiagnosticSummary; question?: string; // Optional question to provide context for Yes/No buttons onQuickResponse?: () => void; // Callback when a quick response is selected }If loading state is needed for future functionality, consider prefixing with underscore:
_isLoading?: boolean;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
vscode/src/taskManager/utils.ts(3 hunks)vscode/src/utilities/ModifiedFiles/processMessage.ts(3 hunks)webview-ui/src/components/AnalysisPage/AnalysisPage.tsx(0 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx(5 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(6 hunks)webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css(1 hunks)
💤 Files with no reviewable changes (1)
- webview-ui/src/components/AnalysisPage/AnalysisPage.tsx
✅ Files skipped from review due to trivial changes (1)
- webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css
🚧 Files skipped from review as they are similar to previous changes (4)
- vscode/src/utilities/ModifiedFiles/processMessage.ts
- webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
- webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx
- vscode/src/taskManager/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (2)
shared/src/index.ts (2)
DiagnosticSummary(7-7)DiagnosticIssue(7-7)shared/src/types/types.ts (2)
DiagnosticSummary(324-328)DiagnosticIssue(316-322)
🪛 GitHub Check: Lint
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx
[warning] 206-206:
'filename' is defined but never used. Allowed unused args must match /^_/u
webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx
[warning] 29-29:
'isLoading' is defined but never used. Allowed unused args must match /^_/u
⏰ 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 (8)
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (4)
1-9: LGTM! Clean imports and type definitionsThe imports are properly structured and the types are correctly imported from the shared module, addressing previous review feedback about avoiding local type duplication.
16-36: Excellent state management designThe use of
Set<string>for tracking selections and the centralizedupdateSelectedIssuesfunction provides clean, efficient state management while properly respecting theisMessageRespondedflag to prevent unwanted changes.
38-101: Well-structured callback handlers with proper memoizationThe callback functions are properly memoized with correct dependency arrays and consistently respect the
isMessageRespondedflag to prevent state changes when appropriate.
125-200: Excellent accessibility and UX implementationThe component includes proper ARIA attributes (
aria-expanded), meaningful button titles, and clear disabled states. The visual hierarchy with toggle icons and issue counts provides good user feedback.webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx (4)
26-53: Well-structured component with good early return logicThe component properly handles empty states with an early return to prevent phantom messages, and the timestamp formatting is well-implemented with proper localization.
55-76: Robust quick response handlingThe handler properly manages state updates, parent notifications, and VSCode messaging. The inclusion of selected issue IDs in the response payload enables the backend to process user selections effectively.
78-118: Excellent component integration and conditional renderingThe component properly integrates
DiagnosticIssuesViewand handles conditional rendering of questions and quick responses. The disabled logic for buttons is comprehensive, ensuring users must select issues before confirming "yes" responses.
84-87: Good security practices with HTML sanitizationThe use of
rehypeSanitizeplugin ensures that any HTML content is properly sanitized, preventing XSS vulnerabilities while allowing rich text display.Also applies to: 114-114
|
@ibolton336 this is looking great! |
ibolton336
force-pushed
the
browse-diagnostics
branch
3 times, most recently
from
96d4d2d to
a62dd2e
Compare
Signed-off-by: Ian Bolton <[email protected]>
ibolton336
force-pushed
the
browse-diagnostics
branch
from
c7da901 to
cb48aba
Compare
ibolton336
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx (2)
30-30: Remove unusedisLoadingprop.The
isLoadingprop is accepted but never used in the component. Remove it or implement loading state handling if needed.Based on static analysis
146-148: Guard against empty quickResponses array.Line 147 accesses
quickResponses[0]?.messageTokenwith a fallback to empty string, but this could cause issues if the array is empty and the callback is somehow invoked. Consider using a safer approach.Apply this diff to ensure messageToken is always available:
- onClick: () => { - handleQuickResponse("fix-all", quickResponses[0]?.messageToken || "", true); - }, + onClick: () => { + const messageToken = quickResponses[0]?.messageToken; + if (messageToken) { + handleQuickResponse("fix-all", messageToken, true); + } + },webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (1)
177-249: Refactor duplicated message rendering logic.The
ChatMessageType.Diagnostic(lines 177-211) andChatMessageType.String(lines 213-249) branches have nearly identical logic for rendering diagnostic messages. Extract the common rendering logic into a helper function.Apply this diff to reduce duplication:
+ const renderDiagnosticMessage = useCallback((msg: ChatMessage, isMessageResponded: boolean) => { + const message = msg.value?.message as string; + const diagnosticSummary = (msg.value as any)?.diagnosticSummary; + const selectedResponse = msg.selectedResponse; + + // Determine if we have Yes/No quick responses and create an appropriate question + const hasYesNoResponses = + Array.isArray(msg.quickResponses) && + msg.quickResponses.some((response) => response.id === "yes" || response.id === "no"); + + const question = hasYesNoResponses + ? "Would you like me to fix the selected issues?" + : undefined; + + return ( + <MessageWrapper key={msg.messageToken}> + <ReceivedMessage + timestamp={msg.timestamp} + content={message} + diagnosticSummary={diagnosticSummary} + question={question} + isMessageResponded={isMessageResponded} + onQuickResponse={() => handleQuickResponse(msg.messageToken)} + quickResponses={ + Array.isArray(msg.quickResponses) && msg.quickResponses.length > 0 + ? msg.quickResponses.map((response) => ({ + ...response, + messageToken: msg.messageToken, + isDisabled: response.id === "run-analysis" && isAnalyzing, + isSelected: selectedResponse === response.id, + })) + : undefined + } + /> + </MessageWrapper> + ); + }, [handleQuickResponse, isAnalyzing]); + const renderChatMessages = useCallback(() => { // ... existing code ... if (msg.kind === ChatMessageType.Diagnostic) { - const message = msg.value?.message as string; - const diagnosticSummary = (msg.value as any)?.diagnosticSummary; - // ... rest of duplicated code - return (/* ... */); + return renderDiagnosticMessage(msg, isMessageResponded); } if (msg.kind === ChatMessageType.String) { - const message = msg.value?.message as string; - const diagnosticSummary = (msg.value as any)?.diagnosticSummary; - // ... rest of duplicated code - return (/* ... */); + return renderDiagnosticMessage(msg, isMessageResponded); }webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (1)
116-116: Format long line for readability.The
isDisabledcondition on line 116 is too long. Break it into multiple lines for better readability.Based on static analysis
Apply this diff:
- isDisabled: response.isDisabled || isProcessing || isMessageResponded || selectedResponse !== null, + isDisabled: + response.isDisabled || isProcessing || isMessageResponded || selectedResponse !== null,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
agentic/src/types.ts(1 hunks)shared/src/index.ts(1 hunks)shared/src/types/types.ts(2 hunks)vscode/src/taskManager/utils.ts(3 hunks)vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts(3 hunks)vscode/src/utilities/ModifiedFiles/processMessage.ts(3 hunks)vscode/src/webviewMessageHandler.ts(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx(1 hunks)webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx(5 hunks)webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx(6 hunks)webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css(1 hunks)webview-ui/src/components/ResolutionsPage/index.ts(1 hunks)webview-ui/src/hooks/actions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- vscode/src/webviewMessageHandler.ts
- webview-ui/src/components/ResolutionsPage/diagnosticIssuesView.css
- agentic/src/types.ts
- shared/src/types/types.ts
- vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts
- shared/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (7)
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (1)
shared/src/types/types.ts (2)
DiagnosticSummary(298-302)DiagnosticIssue(290-296)
webview-ui/src/hooks/actions.ts (1)
shared/src/types/actions.ts (2)
WebviewAction(54-57)WebviewActionType(27-53)
vscode/src/utilities/ModifiedFiles/processMessage.ts (2)
vscode/src/taskManager/types.ts (1)
TasksList(22-25)vscode/src/taskManager/utils.ts (1)
summarizeTasksStructured(9-60)
webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (3)
shared/src/types/types.ts (1)
DiagnosticSummary(298-302)webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx (1)
DiagnosticMessage(27-182)webview-ui/src/hooks/actions.ts (1)
quickResponse(61-66)
vscode/src/taskManager/utils.ts (2)
vscode/src/taskManager/types.ts (1)
TasksList(22-25)shared/src/types/types.ts (2)
DiagnosticSummary(298-302)DiagnosticIssue(290-296)
webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx (4)
shared/src/types/types.ts (3)
QuickResponse(95-100)DiagnosticSummary(298-302)DiagnosticIssue(290-296)vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts (1)
handleQuickResponse(8-125)webview-ui/src/hooks/actions.ts (1)
quickResponse(61-66)webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (1)
DiagnosticIssuesView(11-219)
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (3)
vscode/src/utilities/ModifiedFiles/handleQuickResponse.ts (1)
handleQuickResponse(8-125)webview-ui/src/components/ResolutionsPage/MessageWrapper.tsx (1)
MessageWrapper(9-11)webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (1)
ReceivedMessage(30-129)
🪛 GitHub Check: Lint
webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx
[warning] 221-221:
Insert ⏎
[warning] 206-206:
'filename' is defined but never used. Allowed unused args must match /^_/u
webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx
[warning] 116-116:
Insert ⏎·········
webview-ui/src/components/ResolutionsPage/DiagnosticMessage.tsx
[warning] 30-30:
'isLoading' is defined but never used. Allowed unused args must match /^_/u
⏰ 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: Package (linux)
🔇 Additional comments (7)
webview-ui/src/hooks/actions.ts (1)
55-66: LGTM!The
QuickResponsePayloadinterface andquickResponseaction creator are well-defined. The optionalselectedIssuesfield properly supports the new diagnostic issue selection feature.webview-ui/src/components/ResolutionsPage/index.ts (1)
1-2: LGTM!The barrel exports properly expose the new
DiagnosticMessagecomponent alongside the existingReceivedMessage.webview-ui/src/components/ResolutionsPage/DiagnosticIssuesView.tsx (1)
1-221: LGTM with minor formatting adjustment needed.The component implementation is solid with proper state management, event handling, and accessibility support. The selection logic correctly handles per-file and global selection states.
Minor formatting issue: Add a newline at the end of the file (line 221) as indicated by the linter.
Based on static analysis
webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx (1)
117-135: LGTM!The addition of
respondedMessageTokensstate andhandleQuickResponsecallback properly tracks which messages have received quick responses, enabling the UI to reflect the responded state.webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx (2)
55-70: LGTM!The conditional delegation to
DiagnosticMessagewhendiagnosticSummaryis provided correctly separates concerns and enables specialized rendering for diagnostic content.
86-102: LGTM!The
handleQuickResponsefunction properly invokes theonQuickResponsecallback before posting the message, enabling the parent component to track responded messages.vscode/src/taskManager/utils.ts (1)
9-60: ID format consistency verified. BothsummarizeTasksStructuredandflattenCurrentTasksuset.getUri().fsPathandt.toString(), matching the${uri}-${task}format inhandleQuickResponse. No changes required.
| const createTasksMessage = (tasks: TasksList): { message: string; diagnosticSummary: any } => { | ||
| const diagnosticSummary = summarizeTasksStructured(tasks); | ||
| return { | ||
| message: `It appears that my fixes caused following issues. Please review the issues below and select which ones you'd like me to fix. `, | ||
| diagnosticSummary: { | ||
| summary: diagnosticSummary.summary, | ||
| issuesByFile: diagnosticSummary.issuesByFile, | ||
| totalIssues: diagnosticSummary.totalIssues, | ||
| }, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use proper type instead of any for diagnosticSummary.
The return type uses any for diagnosticSummary. Import and use the DiagnosticSummary type from @editor-extensions/shared for type safety.
Apply this diff:
+import { DiagnosticSummary } from "@editor-extensions/shared";
+
-const createTasksMessage = (tasks: TasksList): { message: string; diagnosticSummary: any } => {
+const createTasksMessage = (tasks: TasksList): { message: string; diagnosticSummary: DiagnosticSummary } => {
const diagnosticSummary = summarizeTasksStructured(tasks);
return {
message: `It appears that my fixes caused following issues. Please review the issues below and select which ones you'd like me to fix. `,
- diagnosticSummary: {
- summary: diagnosticSummary.summary,
- issuesByFile: diagnosticSummary.issuesByFile,
- totalIssues: diagnosticSummary.totalIssues,
- },
+ diagnosticSummary,
};
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const createTasksMessage = (tasks: TasksList): { message: string; diagnosticSummary: any } => { | |
| const diagnosticSummary = summarizeTasksStructured(tasks); | |
| return { | |
| message: `It appears that my fixes caused following issues. Please review the issues below and select which ones you'd like me to fix. `, | |
| diagnosticSummary: { | |
| summary: diagnosticSummary.summary, | |
| issuesByFile: diagnosticSummary.issuesByFile, | |
| totalIssues: diagnosticSummary.totalIssues, | |
| }, | |
| }; | |
| }; | |
| import { DiagnosticSummary } from "@editor-extensions/shared"; | |
| const createTasksMessage = (tasks: TasksList): { message: string; diagnosticSummary: DiagnosticSummary } => { | |
| const diagnosticSummary = summarizeTasksStructured(tasks); | |
| return { | |
| message: `It appears that my fixes caused following issues. Please review the issues below and select which ones you'd like me to fix. `, | |
| diagnosticSummary, | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In vscode/src/utilities/ModifiedFiles/processMessage.ts around lines 51 to 61,
the function return type currently uses `any` for `diagnosticSummary`; import
the `DiagnosticSummary` type from `@editor-extensions/shared` and replace `any`
with `DiagnosticSummary` in the function return type and the `diagnosticSummary`
variable annotation so the function signature becomes `{ message: string;
diagnosticSummary: DiagnosticSummary }`; add the appropriate type-only import at
the top of the file and ensure the object you return matches the
`DiagnosticSummary` shape.
Signed-off-by: Ian Bolton <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/tests/agent_flow_coolstore.test.ts (1)
86-86: Consider resettinglastYesButtonCountwhen appropriate.
lastYesButtonCountis incremented but never decremented. If the enabled Yes button count decreases (e.g., from 2→1) and later returns to the previous level (1→2), the conditionenabledYesButtonCount > lastYesButtonCounton line 116 will be false, causing the test to skip clicking the Yes button even though a new Yes button may have appeared.Consider tracking button state changes more robustly, for example by resetting
lastYesButtonCountwhen it exceedsenabledYesButtonCount:const enabledYesButtonCount = await enabledYesButton.count(); const disabledYesButtonCount = await disabledYesButton.count(); const enabledFixAllButtonCount = await enabledFixAllButton.count(); + + // Reset counter if button count decreased + if (enabledYesButtonCount < lastYesButtonCount) { + lastYesButtonCount = enabledYesButtonCount; + } if (enabledYesButtonCount > 0 && enabledYesButtonCount > lastYesButtonCount) {
🧹 Nitpick comments (2)
tests/e2e/tests/agent_flow_coolstore.test.ts (2)
116-128: Extract screenshot path construction to reduce duplication.The screenshot path construction logic is duplicated across multiple branches (lines 121-128, 133-140, 144-151). This makes the code harder to maintain and increases the risk of inconsistencies.
Extract the path construction into a helper function:
const getScreenshotPath = (suffix: string): string => pathlib.join( SCREENSHOTS_FOLDER, 'agentic_flow_coolstore', `${config.model.replace(/[.:]/g, '-')}`, `${1000 - maxIterations}-${suffix}.png` );Then use it in each branch:
await vscodeApp.waitDefault(); await enabledYesButton.last().click(); console.log('Yes button clicked'); await vscodeApp.getWindow().screenshot({ - path: pathlib.join( - SCREENSHOTS_FOLDER, - 'agentic_flow_coolstore', - `${config.model.replace(/[.:]/g, '-')}`, - `${1000 - maxIterations}-yesNo.png` - ), + path: getScreenshotPath('yesNo'), });Also applies to: 129-140
99-107: Consider explicit visibility checks before clicking buttons.The test clicks buttons using
.last().click()without explicitly verifying visibility or stability. WhilewaitDefault()provides some delay, it may not guarantee the button is in a stable, clickable state, especially if the UI is still rendering or animating.Consider adding explicit visibility checks before clicking:
if (enabledYesButtonCount > 0 && enabledYesButtonCount > lastYesButtonCount) { lastYesButtonCount = enabledYesButtonCount; await vscodeApp.waitDefault(); + await expect(enabledYesButton.last()).toBeVisible({ timeout: 5000 }); await enabledYesButton.last().click();Alternatively, if screen overlays are an issue (as noted in the retrieved learnings), consider using
dispatchEvent('click')instead of.click().Also applies to: 119-119, 131-131, 142-142
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/tests/agent_flow_coolstore.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/tests/agent_flow_coolstore.test.ts
🧬 Code graph analysis (1)
tests/e2e/tests/agent_flow_coolstore.test.ts (1)
tests/e2e/utilities/consts.ts (1)
SCREENSHOTS_FOLDER(2-2)
⏰ 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 (macos)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
3 participants
Resolves #635
Screen.Recording.2025-10-03.at.3.53.11.PM.mov
Summary by CodeRabbit
New Features
Style
Refactor
Tests