Conversation
|
@BugBot run |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the plan panel by introducing an interactive annotation system. Users can now provide targeted feedback on specific parts of an AI-generated plan, which can then be used to refine the plan. This feature aims to improve the collaborative and iterative process of plan development. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (0 items)No low-priority items. 🚫 Dropped False Positives (8 items)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
the
needs-human:review-issue
|
@BugBot run |
|
Skipping Bugbot: Unable to authenticate your request. Please make sure Bugbot is properly installed and configured for this repository. |
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (3 items)
🚫 Dropped False Positives (6 items)
Generated by Dyadbot multi-agent code review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| const prompt = currentAnnotations | ||
| .map( | ||
| (a, i) => `**Comment ${i + 1}:**\n> ${a.selectedText}\n\n${a.comment}`, |
There was a problem hiding this comment.
🟡 MEDIUM | data-integrity
Stale annotations sent in prompt when plan content changes mid-session
handleSendComments reads annotations directly from the atom and includes all of them in the prompt, regardless of whether the plan text has changed since the annotation was created. The highlighting logic in applyPlanAnnotationHighlights already validates annotations against current text (actualText === annotation.selectedText) and skips stale ones — but this function has no equivalent staleness check.
If the plan content is revised (e.g. by a prior stream completing and updating currentPlan) while annotations still exist for that chatId, the prompt will reference old selectedText excerpts that no longer appear in the plan, sending misleading context to the LLM.
💡 Suggestion: Filter currentAnnotations through the same text-match validation that applyPlanAnnotationHighlights uses before building the prompt — or add a check like segments + readPlanTextFromSegments to confirm each annotation still matches.
| highlightNode.splitText(charsToHighlight); | ||
|
|
||
| const mark = document.createElement("mark"); | ||
| const isFirstFragment = index === 0; |
There was a problem hiding this comment.
🟢 LOW | edge-case
isFirstFragment can be wrong if an earlier segment is skipped
The reverse loop sets isFirstFragment = index === 0, which is correct when all segments produce a mark. However, if overlappingSegments[0] is skipped (due to charsToHighlight <= 0 or !textNode.parentNode), no mark in the annotation receives role="button" or tabindex="0". The annotation would be visible but unreachable by keyboard.
This is an unlikely edge case but worth noting. A fix would be to track whether any mark has been assigned the interactive attributes yet, rather than relying on index === 0.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard.
The major bugs identified in earlier rounds (race condition/data loss in Issues Summary
🟢 Low Priority Notes (5 items)
🚫 Dropped False Positives (4 items)
Generated by Dyadbot multi-agent code review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee35c658f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| refreshVersions(); | ||
| invalidateTokenCount(); | ||
| onSettled?.(); | ||
| onSettled?.({ success: true }); |
There was a problem hiding this comment.
I checked src/ipc/handlers/chat_stream_handlers.ts:1721-1738: cancelling a chat emits chat:response:end with wasCancelled: true, not an error. This callback still reports success: true, so PlanPanel.handleSendComments clears planAnnotationsAtom after a user-cancelled “Send Comments” run even though no revised plan was produced. In that cancel path the user loses all of their review comments and has nothing to retry from.
Useful? React with 👍 / 👎.
| const handleClick = (e: MouseEvent) => { | ||
| const target = e.target instanceof HTMLElement ? e.target : null; | ||
| const mark = target?.closest(ANNOTATION_MARK_SELECTOR) as HTMLElement; | ||
| if (!mark) return; | ||
|
|
||
| openPopoverForMark(mark); |
There was a problem hiding this comment.
For plans that contain Markdown links or GFM autolinks, the highlight mark is inserted inside the <a> element, and VanillaMarkdownParser still wires link clicks to ipc.system.openExternalUrl(...) in src/components/chat/DyadMarkdownParser.tsx. Because this handler opens the comment popover without cancelling the event, clicking a commented link both opens the comment UI and follows the link, which makes linked text effectively impossible to review safely.
Useful? React with 👍 / 👎.
| const chatAnnotations = useMemo( | ||
| () => (chatId ? (annotations.get(chatId) ?? []) : []), | ||
| [chatId, annotations], |
There was a problem hiding this comment.
I checked src/hooks/usePlanEvents.ts:57-67: every plan:update replaces the plan content for the same chat, but nothing prunes planAnnotationsAtom. Once the text shifts, applyPlanAnnotationHighlights silently stops rendering mismatched annotations, yet this component still uses the raw list for the badge/sidebar and overlap checks. After any manual plan revision or regeneration in the same chat, users can end up with invisible “ghost” comments that are still sent back to the model and can block creating a new annotation at those offsets.
Useful? React with 👍 / 👎.
🤖 Claude Code Review SummaryPR Confidence: 4/5All review comments have been addressed with code changes; confidence is high but the Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — product principles were clear enough for all decisions in this review. 🤖 Generated by Claude Code |
wwwillchen
left a comment
wwwillchen
left a comment
There was a problem hiding this comment.
lgtm, see comment below to improve e2e test case and then OK to merge
There was a problem hiding this comment.
lets update this e2e test case so it actually tests sending the annotation comments and makes sure the request being sent is correct.
|
@BugBot run |
| > | ||
| <button | ||
| onClick={handleCommentClick} | ||
| aria-label="Add comment" |
There was a problem hiding this comment.
🟡 MEDIUM | interaction-design
Floating comment button touch target is too small (~26px)
The button has p-1.5 padding around a 14px icon, producing roughly a 26×26px click target. This is well below the recommended 44×44px minimum (WCAG 2.5.8 / Apple HIG). Users on trackpads or touch-enabled devices will find it difficult to hit.
Similarly, the edit/delete buttons in CommentCard.tsx use p-1 with 12px icons (~20×20px targets).
💡 Suggestion: Increase to at least p-2 with a 16px icon on the floating button, and p-1.5 on the CommentCard action buttons. Alternatively, add an invisible hit-area expansion via a pseudo-element or negative margin.
| } | ||
| clearPlanAnnotationHighlights(container); | ||
| }; | ||
| }, [chatAnnotations, currentPlan]); |
There was a problem hiding this comment.
🟡 MEDIUM | complexity
~60 lines of DOM orchestration logic inline in useEffect
This highlight effect (lines 67–129) defines refreshHighlights, scheduleHighlightRefresh, creates a MutationObserver, and manages frameId / isApplyingHighlights state — all inline within the component. Extracting this into a custom hook (e.g., usePlanAnnotationHighlights(containerRef, annotations, currentPlan)) would improve PlanPanel readability and make the highlight logic independently testable.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. This PR adds a solid plan annotation system with good accessibility (ARIA roles, keyboard navigation, focus management) and proper async handling via the new Issues Summary
🟢 Low Priority Notes (5 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 259 passed, 3 failed, 10 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/annotator.spec.ts \
e2e-tests/github.spec.ts \
e2e-tests/template-create-nextjs.spec.ts
|
- Fix race condition in handleSendComments: await streamMessage, clear annotations only on success - Add viewport clamping for floating comment button/form positioning - Dismiss floating UI on scroll even when form is open - Add scroll dismiss handler to CommentPopover - Add aria-labels to icon-only buttons (comment, edit, delete, view comments) - Add keyboard shortcut hint to comment form - Add autoFocus and keyboard shortcuts to edit textarea in CommentCard - Sync editedText state with annotation.comment prop via useEffect - Stabilize chatAnnotations reference with useMemo to prevent unnecessary MutationObserver cycles - Use native button element for CommentsFloatingButton popover trigger - Show "Sending..." label on send button during submission Co-Authored-By: Claude Opus 4.5 <[email protected]>
- preserve plan annotation drafts during scroll and add cancel affordances - improve plan comment accessibility, focus handling, and popover positioning - reduce unnecessary highlight refreshes and extend plan annotation coverage Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix focus effect re-stealing focus during editing by depending on annotationId only - Apply clamped popover position directly to DOM to avoid redundant re-renders - Reposition popover on scroll instead of dismissing it - Use onSettled callback for clearing annotations after stream completes - Clamp x-coordinate to prevent off-screen positioning - Pass chatAnnotations as prop to avoid duplicated atom derivation - Add type="button" to icon action buttons - Disable Save button when comment is whitespace-only - Extract annotation Map mutation helpers to reduce duplication - Add JSDoc comment to getBoundaryTextOffset algorithm Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Preserve annotations on stream failure by only clearing on success via onSettled result
- Add key={chatId} to SelectionCommentButton to reset state on chat switch
- Cancel requestAnimationFrame on effect cleanup to prevent resource leaks
- Extract ANNOTATION_ID_ATTRIBUTE and ANNOTATION_MARK_SELECTOR constants
- Clamp comment form Y position to prevent overflow below viewport
- Add selectionchange listener for keyboard and touch selection support
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
azizmejri1
force-pushed
the
plan-annotator
branch
from
765b600 to
44d8c91
Compare
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Most significant issues from prior reviews (race condition on annotation clearing, missing accessibility attributes, popover focus management) have already been addressed in the latest revision. Issues Summary
🟢 Low Priority Notes (10 items)
🚫 Dropped False Positives (3 items)
Generated by Dyadbot multi-agent code review |
|
|
||
| const prompt = currentAnnotations | ||
| .map( | ||
| (a, i) => `**Comment ${i + 1}:**\n> ${a.selectedText}\n\n${a.comment}`, |
There was a problem hiding this comment.
🟡 MEDIUM | data-integrity
Multi-line selectedText breaks markdown blockquote in prompt
When a.selectedText contains newlines (e.g., user selected text spanning multiple paragraphs), the blockquote formatting > ${a.selectedText} only prefixes the first line with >. Subsequent lines appear outside the blockquote, corrupting the prompt structure and making it ambiguous whether those lines are quoted text or comment text.
💡 Suggestion: Replace newlines in selectedText with \n> to continue the blockquote:
`> ${a.selectedText.replace(/\n/g, '\n> ')}`| }; | ||
|
|
||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") dismiss({ restoreFocus: true }); |
There was a problem hiding this comment.
🟡 MEDIUM | interaction
Escape key closes popover AND cancels edit simultaneously
When the CommentCard's edit textarea is active inside this popover, pressing Escape triggers both the textarea's onKeyDown handler (which calls handleCancel to exit edit mode) and this document-level keydown listener (which calls dismiss() to close the entire popover). The user expects Escape to cancel the edit first, then a second Escape to close the popover, but instead both happen at once.
💡 Suggestion: In CommentCard's onKeyDown handler for Escape, add e.stopPropagation() to prevent the event from reaching the document-level listener.
🎭 Playwright Test Results❌ Some tests failed
Summary: 259 passed, 3 failed, 6 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/context_manage.spec.ts \
e2e-tests/local_agent_generate_image.spec.ts \
e2e-tests/select_component.spec.ts
|
🎭 Playwright Test Results❌ Some tests failed
Summary: 793 passed, 8 failed, 9 flaky, 254 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/queued_message.spec.ts
|
2 participants
close #2989