Keep plan review cards in document flow - #249
Conversation
johannesjo
left a comment
johannesjo
left a comment
There was a problem hiding this comment.
I found two behavior regressions in the new flow-slot interaction, both reproduced in Chromium against the DOM structure introduced here:
- A multi-block selection that crosses an existing flow slot includes the card review/answer text in
Selection.toString(), so subsequent review or Ask prompts can quote UI content as plan content. - Highlight geometry is a one-time snapshot even though preceding slots now reflow, resize, and disappear, so the persisted highlight can drift away from the selected text.
The added tests only assert source strings and CSS, so they do not exercise either DOM behavior.
There is also a current integration blocker: main advanced to 1aabea4 and the PR now conflicts in PlanViewerDialog.tsx around the scroll-target guard (if (!target?.id) return). Please update the branch and preserve that guard while resolving the conflict.
1409bbc to
65fb8d7
Compare
|
Addressed all requested changes in
Validation: full test suite 1,755 passed/23 skipped plus 11 client tests; type checks, static checks, and frontend build pass. |
|
Double-checked current head I found one remaining Important issue and one Minor issue: Important — preserve rendered selection text
This affects both Review and Ask because both consume Minor — blockquote styling leaks into review UIFor a selection inside a blockquote, the slot is inserted after the selected Verdict: Needs changes. I did not find another Critical or Important lifecycle, cleanup, scrolling, security, or performance issue in the five-file diff. |
johannesjo
left a comment
johannesjo
left a comment
There was a problem hiding this comment.
The flow-slot text exclusion, reflow tracking, session cleanup, and blockquote reset now look addressed, and CI is green. One Important correctness issue remains.
Important — preserve native rendered selection semantics
getPlanSelectionVisibleText at src/lib/plan-selection.ts:74-97 still changes common Markdown selections. It walks only text nodes, drops whitespace-only nodes unless they are inside PRE, and inserts a separator only when the nearest recognized block changes.
I reproduced this in Chromium against current head eeb8fc2, using structures emitted by the project's Marked renderer:
**Hello** *world*renders as adjacent inline elements separated by a whitespace text node. Native selection isHello world; the reconstruction returnsHelloworld.- A Markdown hard break renders as
<br>. Native selection isFirst\nSecond; the reconstruction returnsFirstSecond. A soft source newline has the inverse problem: Chromium renders it as a space, while the reconstruction retains the raw newline from the text node. - A two-row Markdown table has native selection text
A\tB\nC\tD; the reconstruction returnsAB\nCDbecause every cell in a row maps to the sameTRblock.
Both Review and Ask persist this reconstructed sel.selectedText, so their prompts can misquote ordinary plan content. Please preserve browser-rendered whitespace, <br> breaks, and table-cell boundaries while continuing to exclude flow-slot UI and hidden Mermaid/SVG text. Add exact-equality regression cases for inline whitespace, soft/hard breaks, and table cells; the current happy-dom tests do not exercise these native selection semantics.
|
Addressed the remaining rendered-selection semantics issue in |
johannesjo
left a comment
johannesjo
left a comment
There was a problem hiding this comment.
Re-reviewed at cf08838 in real Chromium, driving the selection APIs against HTML produced by this project's own marked instance rather than hand-written markup.
What's fixed. Rounds 1–2 hold up under that setup. A selection spanning an existing card gives Selection.toString() containing the card text while getPlanSelection returns only Before plan text\nAfter plan text. Retained ranges plus ResizeObserver re-measure correctly, including observer teardown. The hidden-SVG handling is in fact stricter than native — Chromium includes non-rendered <defs><text> in Selection.toString(), and this correctly drops it. The blockquote reset is in.
Why I'm not listing more broken cases
This is the third round on one defect class. The cases aren't the problem; the harness is. Here is the whole thing in one controlled experiment — two inputs differing by a single newline character:
| markup | native | getPlanSelection |
|---|---|---|
<tr><td>A</td><td>B</td></tr> |
A\tB |
A\tB ✅ |
<tr>\n<td>A</td>\n<td>B</td>\n</tr> |
A\tB |
A B ❌ |
The second row is what marked actually emits. The first is the fixture at plan-selection.client.test.tsx:146. So the table case — the exact one I named last round — is still broken in the app, and its regression test passes because it is written against markup the renderer never produces.
For reference, the mechanism: the inter-cell text node's closest(BLOCK_SELECTOR) resolves to the <tr>, so it survives the guard at plan-selection.ts:141, collapses to ' ', and sets lastCell = null at :147 — which disables the cell !== lastCell branch at :114.
Across 14 renderer-produced structures, 8 match native (tables, multi-paragraph, blockquote→prose, nested lists, and code-fence→prose diverge). I'm deliberately not itemizing them, because six more assertions would just relocate the boundary — the next structure nobody thought to enumerate fails the same way. Three changes instead:
1. Change the approach: stop reconstructing, delegate to the browser
getPlanSelectionVisibleText is a hand-rolled reimplementation of the browser's selection serializer, and it has needed a new special case per structure for three rounds. The rules it encodes (collapse whitespace, separate on block change, tab between cells) are approximations of layout, so they will keep missing structures that only differ by rendering.
Concretely: clone the range, strip [data-plan-review-flow-slot] from the fragment, mount it offscreen, read innerText — which is the browser's rendered-text algorithm:
const fragment = range.cloneContents();
fragment.querySelectorAll(PLAN_REVIEW_FLOW_SLOT_SELECTOR).forEach((n) => n.remove());
const host = document.createElement('div');
host.className = containerEl.className; // inherit white-space/font context
host.style.cssText = `position:absolute;left:-99999px;top:0;width:${containerEl.clientWidth}px;`;
containerEl.parentElement.append(host);
host.append(fragment);
const text = host.innerText;
host.remove();I measured this sketch on the same 14 cases: 11/14 vs the current 8/14, and it gets tables, nested lists, blockquotes and multi-paragraph exactly right with no per-structure rules. It also drops the hidden <defs>/<style> text for free, since those aren't rendered — so it subsumes the Mermaid handling too.
Two caveats, since I'd rather you hit them here than in review: innerText only reflects layout if the clone is genuinely rendered (offscreen, not display:none), and reading it forces a reflow. My three remaining deltas are surplus blank lines, not lost content — I expect they close with the host inheriting the container's computed style more faithfully, but I haven't tuned it, so treat 11/14 as the floor for the approach rather than its ceiling.
Worth saying: plain Selection.toString() is not a sufficient shortcut. It's correct for everything except the flow-slot UI text, but that's the one case this feature exists to handle.
2. Fix the harness
vitest.client.config.ts sets environment: 'happy-dom', so none of the .client.test.tsx tests can observe native selection semantics — they compare the implementation against strings written by the same person who wrote the implementation, in an emulator. That's why three rounds of "added exact-equality regressions" haven't caught this.
Two things have to change together:
- Fixtures must come from the renderer, not from hand-written HTML. Any test whose markup is typed by hand can be shaped, accidentally, around the implementation — which is exactly what happened with the table.
- The oracle must be a real browser. happy-dom cannot supply the expected value even with generated fixtures, because the thing being asserted is Chromium's behavior. This needs vitest browser mode or a Playwright-driven spec.
3. Acceptance criterion
So that passing isn't a matter of picking good examples:
For a corpus of Markdown documents rendered through the project's own renderer, for every selection tested,
getPlanSelection(container, src).selectedTextequalswindow.getSelection().toString().trim()in Chromium — with any deviation listed explicitly in the test as a named exception carrying a stated reason.
The corpus should cover at least: paragraphs, headings, nested and loose lists, tables (header + body), fenced code, blockquotes, inline emphasis/code/links, hard and soft breaks, rendered Mermaid — plus the two cases where deviation from native is intended: selections spanning a flow slot, and hidden SVG text.
This is checkable and can't be satisfied by fixture choice: expected values come from the browser, inputs come from the renderer, and anything that can't reach parity has to be visible and argued rather than quietly absent. I'd rather see it land with three named, justified exceptions than with a green suite that proves nothing.
Non-blocking, verified separately
plan-selection.ts:158-159—isHiddenSelectionElement(agetComputedStylewalk) runs before theintersectsNodeprune, so cost tracks document size rather than selection size. Selecting the same 9 characters: 0.06 ms at 20 blocks, 0.77 ms at 200, 7.3 ms at 800 — per mouseup. Swapping the two conditions fixes it.PlanViewerDialog.tsx:108— for anLIanchor,anchor.append(slot)puts the card after the entire nested sublist. Selecting the parent item rendersParent item | Child one | Child two | [CARD] | Sibling.PlanViewerDialog.tsx:49-58—reviewSessionreturns a fresh object each recompute andShow keyedremounts on identity, so a plan-watcher push (App.tsx:536) while the dialog is open discards in-progress comments and half-typed input. That matches the stated intent, but it's a real behavior change from the previousShow when={props.open}and worth an explicit call.
Verdict: needs changes — on the approach and the harness, not on another round of cases.
johannesjo
left a comment
johannesjo
left a comment
There was a problem hiding this comment.
Re-reviewed at cf08838 in real Chromium, driving the selection APIs against HTML produced by this project's own marked instance rather than hand-written markup.
What's fixed. Rounds 1–2 hold up under that setup. A selection spanning an existing card gives Selection.toString() containing the card text while getPlanSelection returns only Before plan text\nAfter plan text. Retained ranges plus ResizeObserver re-measure correctly, including observer teardown. The hidden-SVG handling is in fact stricter than native — Chromium includes non-rendered <defs><text> in Selection.toString(), and this correctly drops it. The blockquote reset is in.
Why I'm not listing more broken cases
This is the third round on one defect class. The cases aren't the problem; the harness is. Here is the whole thing in one controlled experiment — two inputs differing by a single newline character:
| markup | native | getPlanSelection |
|---|---|---|
<tr><td>A</td><td>B</td></tr> |
A\tB |
A\tB ✅ |
<tr>\n<td>A</td>\n<td>B</td>\n</tr> |
A\tB |
A B ❌ |
The second row is what marked actually emits. The first is the fixture at plan-selection.client.test.tsx:146. So the table case — the exact one I named last round — is still broken in the app, and its regression test passes because it is written against markup the renderer never produces.
For reference, the mechanism: the inter-cell text node's closest(BLOCK_SELECTOR) resolves to the <tr>, so it survives the guard at plan-selection.ts:141, collapses to ' ', and sets lastCell = null at :147 — which disables the cell !== lastCell branch at :114.
Across 14 renderer-produced structures, 8 match native (tables, multi-paragraph, blockquote→prose, nested lists, and code-fence→prose diverge). I'm deliberately not itemizing them, because six more assertions would just relocate the boundary — the next structure nobody thought to enumerate fails the same way. Three changes instead:
1. Change the approach: stop reconstructing, delegate to the browser
getPlanSelectionVisibleText is a hand-rolled reimplementation of the browser's selection serializer, and it has needed a new special case per structure for three rounds. The rules it encodes (collapse whitespace, separate on block change, tab between cells) are approximations of layout, so they will keep missing structures that only differ by rendering.
Concretely: clone the range, strip [data-plan-review-flow-slot] from the fragment, mount it offscreen, read innerText — which is the browser's rendered-text algorithm:
const fragment = range.cloneContents();
fragment.querySelectorAll(PLAN_REVIEW_FLOW_SLOT_SELECTOR).forEach((n) => n.remove());
const host = document.createElement('div');
host.className = containerEl.className; // inherit white-space/font context
host.style.cssText = `position:absolute;left:-99999px;top:0;width:${containerEl.clientWidth}px;`;
containerEl.parentElement.append(host);
host.append(fragment);
const text = host.innerText;
host.remove();I measured this sketch on the same 14 cases: 11/14 vs the current 8/14, and it gets tables, nested lists, blockquotes and multi-paragraph exactly right with no per-structure rules. It also drops the hidden <defs>/<style> text for free, since those aren't rendered — so it subsumes the Mermaid handling too.
Two caveats, since I'd rather you hit them here than in review: innerText only reflects layout if the clone is genuinely rendered (offscreen, not display:none), and reading it forces a reflow. My three remaining deltas are surplus blank lines, not lost content — I expect they close with the host inheriting the container's computed style more faithfully, but I haven't tuned it, so treat 11/14 as the floor for the approach rather than its ceiling.
Worth saying: plain Selection.toString() is not a sufficient shortcut. It's correct for everything except the flow-slot UI text, but that's the one case this feature exists to handle.
2. Fix the harness
vitest.client.config.ts sets environment: 'happy-dom', so none of the .client.test.tsx tests can observe native selection semantics — they compare the implementation against strings written by the same person who wrote the implementation, in an emulator. That's why three rounds of "added exact-equality regressions" haven't caught this.
Two things have to change together:
- Fixtures must come from the renderer, not from hand-written HTML. Any test whose markup is typed by hand can be shaped, accidentally, around the implementation — which is exactly what happened with the table.
- The oracle must be a real browser. happy-dom cannot supply the expected value even with generated fixtures, because the thing being asserted is Chromium's behavior. This needs vitest browser mode or a Playwright-driven spec.
3. Acceptance criterion
So that passing isn't a matter of picking good examples:
For a corpus of Markdown documents rendered through the project's own renderer, for every selection tested,
getPlanSelection(container, src).selectedTextequalswindow.getSelection().toString().trim()in Chromium — with any deviation listed explicitly in the test as a named exception carrying a stated reason.
The corpus should cover at least: paragraphs, headings, nested and loose lists, tables (header + body), fenced code, blockquotes, inline emphasis/code/links, hard and soft breaks, rendered Mermaid — plus the two cases where deviation from native is intended: selections spanning a flow slot, and hidden SVG text.
This is checkable and can't be satisfied by fixture choice: expected values come from the browser, inputs come from the renderer, and anything that can't reach parity has to be visible and argued rather than quietly absent. I'd rather see it land with three named, justified exceptions than with a green suite that proves nothing.
Non-blocking, verified separately
plan-selection.ts:158-159—isHiddenSelectionElement(agetComputedStylewalk) runs before theintersectsNodeprune, so cost tracks document size rather than selection size. Selecting the same 9 characters: 0.06 ms at 20 blocks, 0.77 ms at 200, 7.3 ms at 800 — per mouseup. Swapping the two conditions fixes it.PlanViewerDialog.tsx:108— for anLIanchor,anchor.append(slot)puts the card after the entire nested sublist. Selecting the parent item rendersParent item | Child one | Child two | [CARD] | Sibling.PlanViewerDialog.tsx:49-58—reviewSessionreturns a fresh object each recompute andShow keyedremounts on identity, so a plan-watcher push (App.tsx:536) while the dialog is open discards in-progress comments and half-typed input. That matches the stated intent, but it's a real behavior change from the previousShow when={props.open}and worth an explicit call.
Verdict: needs changes — on the approach and the harness, not on another round of cases.
|
Addressed the rendered-selection approach and harness issue in
Validation passed locally: targeted client tests ( |
|
Addressed the remaining browser-harness and native-selection acceptance criteria in
Validation passed: focused Chromium suite (13/13), full tests (1,755 unit passed / 23 skipped; 22 client passed), |
|
Follow-up |
|
Thanks for sticking with this through four rounds — the core restructure is right, and I want to keep it. But I don't think this should merge in its current shape: the PR has grown well past what the fix needs, and one of the newer changes is a behavior regression. What's good. The bug is real: cards absolutely positioned at Four things I'd block on. 1. Keying the session on plan content destroys in-progress reviews
Every piece of review state is a component-local That matters here because plan content is live-watched, not static. On 2.
|
|
Round 5's four asks are genuinely addressed at
The flow-slot restructure itself still looks right, and dropping Critical — selections inside a fenced code block lose every newline and all indentation
When a selection sits entirely inside one code block, the common ancestor is
The paragraph→code→paragraph case still works, because there Fix that I measured: rebuild the ancestor chain before mounting — walk from The remaining spanning delta is the surplus-blank-line class I flagged as acceptable in round 4 — not lost content. Important1. Triple-clicking a paragraph anchors the card to the next block. I drove real Chromium sets 2. Unaddressed from round 4. Through the project's own marked config:
if (anchor.tagName === 'LI') {
const nested = anchor.querySelector(':scope > ul, :scope > ol');
nested ? nested.before(slot) : anchor.append(slot);
return slot;
}Correcting myself on a sub-point: I expected the skipped sibling-slot walk to invert repeat comments on one item, and it does not — 3. Zero tests — and the two that matter don't need Playwright.
Negative control: the same file against 4. Plan-watcher push destroys the in-progress inline input. The effect clears The dangling 5. After a plan update the inline cards never come back, and the sidebar jump goes silent.
6. My round-5 ask, taken literally, but
Minor
Where that leaves it. The restructure is right and I want it. Blocking: the code-block regression, the triple-click anchor, the The anchor bug is one I should have caught in round 4 when I was already driving Chromium — sorry for surfacing it this late. Round 5's offer stands: if you'd rather not do another pass, say the word and I'll pick it up from here. Verification notes: Chromium 151 via CDP for the selection/anchor/ |
2 participants
Summary
Addresses the plan-review inline comment overlap item in #214.
Validation
npm test -- src/components/plan-review-flow.test.ts(3 passed)npx vitest run src(65 files passed, 792 tests passed)npm run checknpm run check:staticnpm run build:frontend