From 213057cfc82f2986be229101fe98a0323ebe1605 Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Sat, 1 Aug 2026 14:36:56 -0400 Subject: [PATCH 1/2] fix(plan): keep review cards in document flow --- src/components/PlanViewerDialog.tsx | 212 +++++++++++++++--------- src/components/plan-review-flow.test.ts | 31 ++++ src/lib/plan-selection.ts | 31 ++++ src/styles.css | 7 + 4 files changed, 206 insertions(+), 75 deletions(-) create mode 100644 src/components/plan-review-flow.test.ts diff --git a/src/components/PlanViewerDialog.tsx b/src/components/PlanViewerDialog.tsx index ba842fbd..dd3a7301 100644 --- a/src/components/PlanViewerDialog.tsx +++ b/src/components/PlanViewerDialog.tsx @@ -1,4 +1,5 @@ -import { Show, For, createSignal, createEffect } from 'solid-js'; +import { Show, For, createSignal, createEffect, onCleanup } from 'solid-js'; +import { Portal } from 'solid-js/web'; import { Dialog } from './Dialog'; import { createDialogScroll } from '../lib/dialog-scroll'; import { ReviewProvider, useReview } from './ReviewProvider'; @@ -8,7 +9,7 @@ import { InlineInput } from './InlineInput'; import { AskCodeCard } from './AskCodeCard'; import { CloseIcon } from './icons'; import { createHighlightedMarkdown } from '../lib/marked-shiki'; -import { getPlanSelection } from '../lib/plan-selection'; +import { getPlanSelection, getPlanSelectionFlowAnchor } from '../lib/plan-selection'; import { openFileInEditor } from '../lib/shell'; import { theme } from '../lib/theme'; import { sf } from '../lib/fontScale'; @@ -86,6 +87,26 @@ interface HighlightRect { height: number; } +const PLAN_REVIEW_FLOW_SLOT_SELECTOR = '[data-plan-review-flow-slot]'; + +function insertPlanReviewFlowSlot(anchor: HTMLElement): HTMLDivElement { + const slot = document.createElement('div'); + slot.className = 'plan-review-flow-slot'; + slot.setAttribute('data-plan-review-flow-slot', ''); + + if (anchor.tagName === 'LI') { + anchor.append(slot); + return slot; + } + + let insertionPoint: Element = anchor; + while (insertionPoint.nextElementSibling?.matches(PLAN_REVIEW_FLOW_SLOT_SELECTOR)) { + insertionPoint = insertionPoint.nextElementSibling; + } + insertionPoint.after(slot); + return slot; +} + function PlanViewerContent(props: PlanViewerContentProps) { const review = useReview(); const planHtml = createHighlightedMarkdown(() => props.planContent); @@ -93,8 +114,8 @@ function PlanViewerContent(props: PlanViewerContentProps) { let contentRef: HTMLDivElement | undefined; let scrollRef: HTMLDivElement | undefined; - const [selectionY, setSelectionY] = createSignal(0); - const [cardOffsets, setCardOffsets] = createSignal>({}); + const [pendingFlowSlot, setPendingFlowSlot] = createSignal(); + const [flowSlots, setFlowSlots] = createSignal>({}); const [highlightRects, setHighlightRects] = createSignal([]); createDialogScroll( @@ -126,10 +147,32 @@ function PlanViewerContent(props: PlanViewerContentProps) { createEffect(() => { const target = review.scrollTarget(); if (!target) return; - const y = cardOffsets()[target.id]; - if (y !== undefined && scrollRef) { - scrollRef.scrollTo({ top: Math.max(0, y - 100), behavior: 'smooth' }); + const slot = flowSlots()[target.id]; + if (slot && scrollRef) { + const scrollRect = scrollRef.getBoundingClientRect(); + const slotRect = slot.getBoundingClientRect(); + const top = scrollRef.scrollTop + slotRect.top - scrollRect.top; + scrollRef.scrollTo({ top: Math.max(0, top - 100), behavior: 'smooth' }); + } + }); + + // Remove flow slots when their annotation or question is removed elsewhere (for example, + // from the review sidebar). + createEffect(() => { + const activeIds = new Set([ + ...review.annotations().map((annotation) => annotation.id), + ...review.activeQuestions().map((question) => question.id), + ]); + const currentSlots = flowSlots(); + const staleIds = Object.keys(currentSlots).filter((id) => !activeIds.has(id)); + if (staleIds.length === 0) return; + + for (const id of staleIds) { + currentSlots[id].remove(); } + setFlowSlots( + Object.fromEntries(Object.entries(currentSlots).filter(([id]) => activeIds.has(id))), + ); }); // Clear highlight overlays when pending selection is dismissed @@ -137,14 +180,12 @@ function PlanViewerContent(props: PlanViewerContentProps) { if (!review.pendingSelection()) setHighlightRects([]); }); - /** Capture selection rects and Y offset relative to contentRef. */ - function captureSelectionGeometry(): { y: number; rects: HighlightRect[] } { + /** Capture selection rects relative to contentRef. */ + function captureSelectionGeometry(): HighlightRect[] { const domSel = window.getSelection(); - if (!domSel || domSel.rangeCount === 0 || !contentRef) return { y: 0, rects: [] }; + if (!domSel || domSel.rangeCount === 0 || !contentRef) return []; const range = domSel.getRangeAt(0); const containerRect = contentRef.getBoundingClientRect(); - const rangeRect = range.getBoundingClientRect(); - const y = rangeRect.bottom - containerRect.top; const clientRects = range.getClientRects(); const rects: HighlightRect[] = []; for (let i = 0; i < clientRects.length; i++) { @@ -156,17 +197,23 @@ function PlanViewerContent(props: PlanViewerContentProps) { height: r.height, }); } - return { y, rects }; + return rects; } - function handleMouseUp() { + function handleMouseUp(event: MouseEvent) { if (!contentRef) return; + const eventTarget = event.target; + if (eventTarget instanceof Element && eventTarget.closest(PLAN_REVIEW_FLOW_SLOT_SELECTOR)) { + return; + } + const sel = getPlanSelection(contentRef, props.planFileName); - if (!sel) return; + const flowAnchor = getPlanSelectionFlowAnchor(contentRef); + if (!sel || !flowAnchor) return; - const { y, rects } = captureSelectionGeometry(); - setSelectionY(y); - setHighlightRects(rects); + setHighlightRects(captureSelectionGeometry()); + pendingFlowSlot()?.remove(); + setPendingFlowSlot(insertPlanReviewFlowSlot(flowAnchor)); // Clear native selection — overlay rects provide the visual highlight from here window.getSelection()?.removeAllRanges(); @@ -182,13 +229,45 @@ function PlanViewerContent(props: PlanViewerContentProps) { }); } - function handleSubmitWithPosition(text: string, mode: Parameters[1]) { - const y = selectionY(); + function handleSubmitInFlow(text: string, mode: Parameters[1]) { + const slot = pendingFlowSlot(); const id = review.handleSubmit(text, mode); - if (id) setCardOffsets((prev) => ({ ...prev, [id]: y })); + if (!id) return; + if (slot) setFlowSlots((prev) => ({ ...prev, [id]: slot })); + setPendingFlowSlot(undefined); setHighlightRects([]); } + function dismissPendingSelection() { + pendingFlowSlot()?.remove(); + setPendingFlowSlot(undefined); + review.clearPendingSelection(); + } + + function dismissAnnotation(id: string) { + review.dismissAnnotation(id); + removeFlowSlot(id); + } + + function dismissQuestion(id: string) { + review.dismissQuestion(id); + removeFlowSlot(id); + } + + function removeFlowSlot(id: string) { + const slot = flowSlots()[id]; + slot?.remove(); + setFlowSlots((prev) => { + if (!(id in prev)) return prev; + return Object.fromEntries(Object.entries(prev).filter(([slotId]) => slotId !== id)); + }); + } + + onCleanup(() => { + pendingFlowSlot()?.remove(); + Object.values(flowSlots()).forEach((slot) => slot.remove()); + }); + return ( <> {/* Header */} @@ -299,69 +378,52 @@ function PlanViewerContent(props: PlanViewerContentProps) { )} - {/* Inline input for pending selection — positioned near the selection */} - -
- -
+ {/* Inline input for pending selection — mounted after the selected block */} + + {(slot) => ( + + + + )} - {/* Annotation cards — positioned where the selection was made */} + {/* Annotation cards — mounted in document flow after the selected block */} {(annotation) => ( -
- review.dismissAnnotation(annotation.id)} - overlay - /> -
+ + {(slot) => ( + +
+ dismissAnnotation(annotation.id)} + /> +
+ + )} + )} - {/* Active questions — positioned where the selection was made */} + {/* Active questions — mounted in document flow after the selected block */} {(q) => ( -
- review.dismissQuestion(q.id)} - /> -
+ + {(slot) => ( + + dismissQuestion(q.id)} + /> + + )} + )} diff --git a/src/components/plan-review-flow.test.ts b/src/components/plan-review-flow.test.ts new file mode 100644 index 00000000..e3887025 --- /dev/null +++ b/src/components/plan-review-flow.test.ts @@ -0,0 +1,31 @@ +import { readFileSync } from 'fs'; +import { resolve } from 'path'; +import { describe, expect, it } from 'vitest'; + +const viewer = readFileSync(resolve(__dirname, 'PlanViewerDialog.tsx'), 'utf8'); +const selection = readFileSync(resolve(__dirname, '../lib/plan-selection.ts'), 'utf8'); +const css = readFileSync(resolve(__dirname, '../styles.css'), 'utf8'); + +describe('plan review flow slots', () => { + it('mounts inputs, comments, and questions in document flow', () => { + expect(viewer).toContain("slot.className = 'plan-review-flow-slot'"); + expect(viewer).toContain(''); + expect(viewer).toContain(''); + expect(viewer).toContain(''); + expect(viewer).not.toContain('cardOffsets'); + expect(viewer).not.toContain('selectionY'); + + const rule = css.match(/\.plan-review-flow-slot\s*\{([^}]*)\}/); + expect(rule).not.toBeNull(); + expect(rule?.[1]).toMatch(/display:\s*flow-root\s*;/); + expect(rule?.[1]).not.toMatch(/position:\s*absolute\s*;/); + }); + + it('anchors cards to valid rendered blocks and ignores card selections', () => { + expect(selection).toContain('export function getPlanSelectionFlowAnchor'); + expect(selection).toContain('range.endContainer.nodeType'); + expect(selection).toContain("block.tagName === 'TR'"); + expect(selection).toContain("element.closest('[data-plan-review-flow-slot]')"); + expect(viewer).toContain('eventTarget.closest(PLAN_REVIEW_FLOW_SLOT_SELECTOR)'); + }); +}); diff --git a/src/lib/plan-selection.ts b/src/lib/plan-selection.ts index 99e84ba7..12932a09 100644 --- a/src/lib/plan-selection.ts +++ b/src/lib/plan-selection.ts @@ -41,6 +41,37 @@ export function getPlanSelection(containerEl: HTMLElement, source: string): Plan }; } +/** Find the block that should own an inline review card for the current selection. */ +export function getPlanSelectionFlowAnchor(containerEl: HTMLElement): HTMLElement | null { + const selection = window.getSelection(); + if (!selection || selection.isCollapsed || selection.rangeCount === 0) return null; + + const range = selection.getRangeAt(0); + if (!containerEl.contains(range.commonAncestorContainer)) return null; + + let element: Element | null = + range.endContainer.nodeType === Node.ELEMENT_NODE + ? (range.endContainer as Element) + : range.endContainer.parentElement; + if (!element || element.closest('[data-plan-review-flow-slot]')) return null; + + const block = element.closest(BLOCK_SELECTOR); + if (block && block !== containerEl && containerEl.contains(block)) { + // A div cannot be a child of a table row, so place table comments after the table. + if (block.tagName === 'TR') { + const table = block.closest('table'); + if (table instanceof HTMLElement && containerEl.contains(table)) return table; + } + if (block instanceof HTMLElement) return block; + } + + // Fallback for rendered blocks such as Mermaid diagrams that are not in BLOCK_SELECTOR. + while (element.parentElement && element.parentElement !== containerEl) { + element = element.parentElement; + } + return element instanceof HTMLElement && element.parentElement === containerEl ? element : null; +} + /** Walk backwards from the selection start to find the nearest heading. */ function findNearestHeading(container: HTMLElement, startNode: Node): string { let node: Node | null = startNode; diff --git a/src/styles.css b/src/styles.css index 9e011aca..c35fbb38 100644 --- a/src/styles.css +++ b/src/styles.css @@ -2099,6 +2099,13 @@ body.dragging-task * { margin: 0.6em 0; } +/* Inline plan-review controls occupy real layout space after the selected block. */ +.plan-review-flow-slot { + display: flow-root; + width: 100%; + min-width: 0; +} + /* Lists — better spacing */ .plan-markdown-dialog ul, .plan-markdown-dialog ol { From 1409bbc80516dbb82a857f6416811affb6ebc811 Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Sat, 1 Aug 2026 21:29:07 -0400 Subject: [PATCH 2/2] fix(plan): reset review flow when plan changes --- src/components/PlanViewerDialog.tsx | 43 ++++++++++++++++--------- src/components/plan-review-flow.test.ts | 8 +++++ 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/components/PlanViewerDialog.tsx b/src/components/PlanViewerDialog.tsx index dd3a7301..6b7f16e0 100644 --- a/src/components/PlanViewerDialog.tsx +++ b/src/components/PlanViewerDialog.tsx @@ -1,4 +1,4 @@ -import { Show, For, createSignal, createEffect, onCleanup } from 'solid-js'; +import { Show, For, createSignal, createEffect, createMemo, onCleanup } from 'solid-js'; import { Portal } from 'solid-js/web'; import { Dialog } from './Dialog'; import { createDialogScroll } from '../lib/dialog-scroll'; @@ -39,6 +39,17 @@ function compilePlanReview(annotations: ReviewAnnotation[]): string { } export function PlanViewerDialog(props: PlanViewerDialogProps) { + const reviewSession = createMemo(() => { + if (!props.open) return undefined; + return { + planContent: props.planContent, + planFileName: props.planFileName, + taskId: props.taskId, + agentId: props.agentId, + worktreePath: props.worktreePath, + }; + }); + return ( - - - - + + {(session) => ( + + + + )} ); diff --git a/src/components/plan-review-flow.test.ts b/src/components/plan-review-flow.test.ts index e3887025..da00e4fa 100644 --- a/src/components/plan-review-flow.test.ts +++ b/src/components/plan-review-flow.test.ts @@ -28,4 +28,12 @@ describe('plan review flow slots', () => { expect(selection).toContain("element.closest('[data-plan-review-flow-slot]')"); expect(viewer).toContain('eventTarget.closest(PLAN_REVIEW_FLOW_SLOT_SELECTOR)'); }); + + it('starts a fresh review session when the plan identity changes', () => { + expect(viewer).toContain('const reviewSession = createMemo'); + expect(viewer).toContain(''); + expect(viewer).toContain('planContent={session.planContent}'); + expect(viewer).toContain('worktreePath={session.worktreePath}'); + expect(viewer).not.toContain(''); + }); });