feat(sdk): show a queued marker on data-lavish-question scopes - #219
feat(sdk): show a queued marker on data-lavish-question scopes#219stone16 wants to merge 5 commits into
Conversation
💡 Codex Reviewlavish-axi/src/artifact-sdk.js Line 1688 in 9060866 When a queued question is only partly visible inside a scroll container—for example, only a few pixels remain above the panel's bottom edge— ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Speaking as Kun's firstmate: This is a hold. The PR is CONFLICTING with |
What Changed
data-lavish-questionwrapper with an internal_lavishQuestionKey, and the chrome mirrors those keys back into the sandboxed iframe over a newlavish:setQueuedQuestionKeysmessage — on every render and again on each iframeload, since the chrome owns queue state and a live reload resets the SDK. The field joins_lavishQueueKeyinstripInternalPromptFields, so it never reaches/api/:key/prompts. Queueing with an explicit emptyqueueKeyopts out of both replacement and the marker.role="status"overlay (✓ Queued) that is repositioned rather than rebuilt, so the queued state is announced once instead of on every pass. Repositioning is rAF-coalesced and driven by capture-phasescroll(nested scrollers don't bubble),resize, aResizeObserver, and aMutationObserverthat catches attribute-driven reflows; observers start only while a key is queued. When the scope is clipped fully out of view the marker fades instead of unmounting.intersectClipRectstosrc/artifact-sdk.js(serialized into the injected SDK bysrc/server.js) to clamp theposition: fixedmarker against every scroll/overflow ancestor's padding box plus the viewport, so a question inside a scrollable panel no longer rings the content after it; non-finite clip edges are ignored rather than collapsing the box. README documents the marker and its opt-out, AGENTS.md records the_lavishQuestionKeyprotocol, and tests cover the SDK geometry, chrome queue sync/clear/restore, and SDK-serialization guards.Risk Assessment
✅ Low: The follow-up commit cleanly resolves both round-1 findings exactly as instructed (stable per-scope role=status markers with style-only repositioning, plus capture-phase scroll, attribute/characterData mutation, and ResizeObserver coverage), introduces no observer feedback loops or leaks thanks to shadow-boundary isolation and queue-gated lifecycle, and leaves the queue protocol, chrome sync, and tests intact.
Testing
No baseline commands had been run for this round, so I started from the targeted automated tests:
test/artifact-sdk.test.jsandtest/chrome-client-queue.test.js(104 pass, covering the five newintersectClipRectscases) plus the two name-filteredqueued-questionguards intest/server.test.jsthat pin the clamp into the serialized SDK. Because this is a purely visual overlay change, I then ran it end to end in a real browser: I served a purpose-built artifact throughlavish-axiand clicked through the actual Lavish chrome, with adata-lavish-questionform clipped by anoverflow:autopanel and a deliberately marked block immediately below it. The marker now stops at the panel's padding edge (bottom 492) instead of the question's real bottom (550), so it no longer paints 21px of green ring over the block starting at 529; clamping is two-sided, applies equally tooverflow:hidden, and respects the padding box rather than the border box, while a question with no clipping ancestor still rings its full box. Fading fully out of view kept the same marker node and a singlerole="status"live region, and the queue → remove → send lifecycle cleared markers correctly with the internal question key stripped from the agent payload. No console errors, no findings. I deliberately did not run the opt-inLAVISH_AXI_BROWSER_E2E=1suites — they exercise the layout audit and whiteboard rather than this code path, and my own real-browser run covers it directly. Teardown left the worktree clean./var/folders/14/wd6c42m15v1c7q5ph2jb9bh40000gn/T/no-mistakes-evidence/01KZ0FQTZ9FPQC7TVKJS95Z71E/32-marker-clamped-to-panel-edge.png)/var/folders/14/wd6c42m15v1c7q5ph2jb9bh40000gn/T/no-mistakes-evidence/01KZ0FQTZ9FPQC7TVKJS95Z71E/33-both-questions-queued.png)/var/folders/14/wd6c42m15v1c7q5ph2jb9bh40000gn/T/no-mistakes-evidence/01KZ0FQTZ9FPQC7TVKJS95Z71E/38-overflow-hidden-ancestor-also-clamps.png)Evidence: Measured marker geometry vs. the pre-fix viewport-only clamp, marker-identity and lifecycle table
panel padding box 264/492 · question's own box 380–550 · pre-fix viewport-only clamp would end at 550 · actual marker 380–492 · #after-panel occupies 529–581 (previously overlapped by 21px, now untouched). Panel scrolled: marker top pinned to 264, not the question's real top 260/206. Question fully below the fold: node kept as m1 with opacity "0" (no re-announcement). Unclipped question: 72–242, exactly its own border box./var/folders/14/wd6c42m15v1c7q5ph2jb9bh40000gn/T/no-mistakes-evidence/01KZ0FQTZ9FPQC7TVKJS95Z71E/30-before-queueing.png) - Evidence: Queueing an answer inside the scroll panel shows the ✓ Queued marker and the pill in the Conversation panel (local file:/var/folders/14/wd6c42m15v1c7q5ph2jb9bh40000gn/T/no-mistakes-evidence/01KZ0FQTZ9FPQC7TVKJS95Z71E/31-queued-inside-scroll-panel.png) - Evidence: Removing a queued prompt clears only that question's marker (local file:/var/folders/14/wd6c42m15v1c7q5ph2jb9bh40000gn/T/no-mistakes-evidence/01KZ0FQTZ9FPQC7TVKJS95Z71E/34-removing-prompt-clears-its-marker.png) - Evidence: Send to Agent clears all queued-question markers (local file:/var/folders/14/wd6c42m15v1c7q5ph2jb9bh40000gn/T/no-mistakes-evidence/01KZ0FQTZ9FPQC7TVKJS95Z71E/36-send-to-agent-clears-markers.png)Evidence: Agent poll payload — internal _lavishQuestionKey stripped from the delivered prompt
prompts[1]{uid,prompt,selector,tag,text}: "2","Rollout plan: Canary then full",form#q-plan,form,Which rollout plan? (not inside any scroller) Canary then full Big bang Queue answerEvidence: Demo artifact used for the end-to-end run (question clipped by a scrollable panel + geometry probe)
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
src/artifact-sdk.js:1865- Queued-question markers are position:fixed overlays repositioned only on window scroll (non-capture listener), window resize, and childList mutations. Three common cases move the question wrapper without firing any of these: (1) scrolling an inner overflow container (scroll events from nested scrollables never reach a non-capture window listener), (2) attribute-driven reflows such as class toggles for accordions/tabs (the MutationObserver observes childList+subtree only, not attributes), and (3) late image/font loads. In each case the persistent marker visually detaches from its wrapper and overlays unrelated content until the next window scroll/resize/mutation. Fix mechanically: add a capture-phase scroll listener ({ capture: true, passive: true }) dedicated to scheduleQueuedQuestionRender, and/or observe marked scopes with ResizeObserver (already declared as a global in this file).src/artifact-sdk.js:1687- renderQueuedQuestionState removes and re-creates marker elements carrying role="status" (implicit aria-live polite) on every render pass, and passes are scheduled per animation frame during scrolling plus on every DOM mutation. Screen readers that announce newly inserted status regions (the common toast pattern) will re-announce "Answer queued" repeatedly while the user scrolls or the artifact mutates. Reuse persistent marker nodes and update only their position styles, or keep role="status"/aria-label on one stable node per question so the queued state is announced once when it changes.src/artifact-sdk.js:702- queuePrompt deliberately skips _lavishQuestionKey when the caller passes an explicit empty queueKey (queuePrompt(text, { queueKey: "" })) inside a data-lavish-question wrapper, so such answers queue with no visible Queued marker. This diverges from the new README sentence stating question wrappers "show a visible Queued state while their answer is waiting to be sent" (unconditionally). The Object.hasOwn carve-out looks intentional (empty queueKey is the opt-out of replacement semantics), but the marker suppression is a separate product decision worth confirming: either document the opt-out edge in README or drop the explicitQueueKey condition so any queued answer from a question wrapper shows the marker.🔧 Fix: keep queued-question markers stable and attached through reflows
1 info still open:
src/artifact-sdk.js:1752- While any question answer is queued, the dedicated MutationObserver watches the entire document with attributes+characterData+childList+subtree, so every style/class/text mutation anywhere coalesces into a rAF pass running document.querySelectorAll("[data-lavish-question]") plus getBoundingClientRect reads. This is a deliberate, commented tradeoff and is fully gated (observers start only when keys are queued, stop when empty; artifacts without questions pay nothing), with no feedback loop because all render-pass writes stay inside the shadow root that subtree observation cannot pierce. Acceptable as shipped; only worth revisiting if an animation-heavy artifact ever shows jank while an answer is queued (e.g. by caching the scope list between structural mutations).🔧 **Test** - 1 issue found → auto-fixed ✅
src/artifact-sdk.js:1699- The queued-question marker is clamped only to the viewport, not to ancestor clipping/scroll containers, so a question inside a scrollable panel paints its green ring over unrelated content outside that panel.positionQueuedQuestionMarkercomputes the box fromscope.getBoundingClientRect()and clamps withMath.min(rect.right, window.innerWidth)/Math.min(rect.bottom, window.innerHeight)only. Reproduced: adata-lavish-questionform inside anoverflow:autopanel is clipped at the panel edge, but its ring extends ~200px below the panel and frames the paragraph after it (evidence10-marker-overflows-nested-scroll-container.png). Unlike the transient.lavish-text-highlight, which shares this fixed-overlay limitation but is cleared when the annotation card closes, this marker persists for the whole review, so the bleed is visible the entire time. Whether ancestor clipping is in scope for this change is the author's call.node --test test/chrome-client-queue.test.js(72 tests, includes the 3 new queued-question sync/clear/restore tests)node --test --test-name-pattern "artifact SDK (renders queued|keeps queued-question)" test/server.test.jsManual E2E: started a real server vianode bin/lavish-axi.js <artifact> --no-openwith an isolatedLAVISH_AXI_STATE_DIRand ephemeral port, opened the session URL in Chrome via chrome-devtoolsManual: clicked a radio + "Queue answer" in the artifact iframe -> verified the green✓ Queuedring and label appear on that question only (screenshot 02)Manual: re-queued a different option -> verified exactly one marker and one replaced pill remain (screenshot 03)Manual: edited the artifact file to trigger live reload -> verified markers restored from the persisted queue (screenshot 04)Manual: scrolled the nested#scrollercontainer -> measured question top 559->429 and marker top 559->429, marker node id unchanged (screenshot 05)Manual: expanded<details>(attribute-only reflow, no childList/resize/scroll event) -> measured plan 155->245 and audit 429->518 with markers matching and node ids unchanged (screenshot 06)Manual: clicked "Remove queued prompt" on one pill -> verified only that question's marker cleared, the other node survived unchanged (screenshot 07)Manual: resized viewport 960->520 so a queued question left the viewport -> verifiedopacity: "0"with the node retained, and restored on resize backManual:take_snapshotof the accessibility tree -> verified twostatus "Answer queued"regions withatomic/live="polite"Manual:node bin/lavish-axi.js poll <artifact> --timeout-ms 45000while clicking "Send to Agent" -> verified the agent receives{uid,prompt,selector,tag,text}with_lavishQuestionKeystripped, and that markers cleared on send (screenshot 09)🔧 Fix: clamp queued-question markers to clipping ancestors
✅ Re-checked - no issues remain.
node --test test/artifact-sdk.test.js test/chrome-client-queue.test.js— 104 tests pass, including the five newintersectClipRectsgeometry tests (scrollable-ancestor clamp, no-clipping-ancestor passthrough, nested innermost tightening, empty intersection, non-finite clip edges)node --test --test-name-pattern "queued-question" test/server.test.js— both SDK-serialization guards pass, including the one that fails if the viewport-onlyMath.min(rect.right, window.innerWidth)clamp comes backManual end-to-end: served a purpose-built artifact (clip-demo.html, adata-lavish-questionform clipped by anoverflow:autopanel, with a marked "must NOT be ringed" block right after it) vianode bin/lavish-axi.js <file> --no-openon an ephemeral port + isolatedLAVISH_AXI_STATE_DIR, then drove it in Chrome through chrome-devtools with real mouse clicks on the radio and Queue answer buttonMeasured livegetBoundingClientRect()geometry inside the running artifact across panel scrollTop 0 / 120 / end / back, comparing the actual marker box against the pre-fixMath.min(rect.bottom, window.innerHeight)value computed from the same rectsVerified the marker node is repositioned and never rebuilt: probe-stamped id stayedm1through fading toopacity:"0"(viewport shrunk to 274px so the question fell fully below the fold) and back, and Chrome's a11y tree showed exactly onestatus "Answer queued" atomic live="polite"region per queued questionVerifiedoverflow: hiddenclamps identically by editing the demo's panel CSS and letting live reload apply it (artifact_revision=2), then re-queueing the answerVerified the queued-marker lifecycle end to end: queue → remove pill in the Conversation panel → Send to Agent, checking marker count after eachnode bin/lavish-axi.js poll <file> --timeout-ms 8000— confirmed the delivered prompt carries only{uid,prompt,selector,tag,text}, i.e. the internal_lavishQuestionKeyis stripped before reaching the agentmcp__chrome-devtools__list_console_messages(errors + warnings, preserved across navigations) — nonegit status --porcelainafter teardown — clean; temp state dir and detached server removed🔧 **Document** - 1 issue found → auto-fixed ✅
README.md:164- README states that wrappers marked withdata-lavish-questionshow a Queued state, butsrc/artifact-sdk.js:731-733only sets_lavishQuestionKeywhen the attribute has a non-empty value AND the caller either omitsqueueKeyor passes a non-empty one. SoqueuePrompt(..., { queueKey: "" })(ordata-lavish-question="") silently produces no marker. This reads as a deliberate opt-out tied to the dedupe key, so I did not widen the README sentence — confirm whether that opt-out is intended user-facing contract worth one clause, or an implementation detail README should keep omitting.🔧 Fix: document queued-question marker opt-out in README
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.