fix(preview): converge HTML files on a retained real-URL runtime - #7353
fix(preview): converge HTML files on a retained real-URL runtime#7353lefarcen wants to merge 141 commits into
Conversation
lefarcen
added
size/XL
lefarcen
added
the
needs-validation
|
🧪 This PR changes preview/runtime behavior in a way that should get a manual QA pass before merge, so I’ve added |
Visual regression reviewHead: 46 changed · 7 unchanged · 0 new without baseline · 0 failed Changed cases
26 additional changed case(s) omitted from this comment. Unchanged cases
Visual diff is advisory only and does not block merging. |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
The streaming approach is well scoped, but the new head scanner can leave valid large documents without their scoped preview base because it exits raw-text state on a tag-name prefix. That affects the containment and relative-resource behavior this fix is intended to preserve.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
@lefarcen The current blocker on this head is already well covered by @PerishCode's review: the raw-text scanner can false-close on prefixes like
|
|
Addressed the blocking raw-text delimiter review in a1b14b1. The scanner now accepts a raw-text closing tag only when the tag name is followed by HTML whitespace, |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
The streaming and iframe recovery paths are otherwise coherent, but the head scanner still disagrees with the HTML parser for self-closing syntax on raw-text elements. That can suppress the scoped preview base for affected large documents.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
@lefarcen Thanks for pushing the follow-up on |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
The streaming injection and navigation-recovery paths are otherwise coherent, and the two previously reported raw-text scanner issues are fixed on this head. One first-paint identity check still rejects valid same-document URL changes, which can leave the cover stuck in the exact slow-resource case this signal is meant to recover.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
@lefarcen Thanks for pushing the new head |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
The previous raw-text and visible-paint blockers are fixed on this head, but the streaming scanner still does not preserve the existing redirect-loop behavior for scripts outside the head. Large previews can therefore reload-loop where the buffered path remains guarded.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
@lefarcen Thanks for pushing the new head |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
The streaming parser and iframe recovery fixes are coherent on this head, including the prior raw-text, same-document paint, and body-script scanning follow-ups. One remaining blocker prevents the passive-guard promise from holding for large files when the relevant signal is outside FileViewer's routing prefix.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
@lefarcen Thanks for pushing the new head |
Round one landed the contract and a pure timing module with no call sites. This wires the phases that the preview runtime itself can observe, and writes the dashboard queries against them. PreviewSessionFrames reports bootstrap_handshake, capabilities_applied, version_promoted, last_good_retained and recovery_attempted; IframeKeepAlivePool reports cache_reclaimed through the single choke point every eviction already passes through. navigation_start and first_visible_paint stay unwired and belong to FileViewer. Every path out of a staged standby reports, not just promotion. A version_promoted row is emitted for the daemon refusing a version and for a standby that never settles, with the gate that was still open named from the session snapshot -- otherwise promotion success rate has no denominator and reads 100% forever. last_good_retained likewise reports retained=false when the previous browsing context is gone, so the retention rate has negative rows to divide by. Legacy-url documents promote on browser load, a weaker gate, and deliberately report nothing. Reporting goes through the consent-gated analytics channel, not reportSafetyEvent. That bypass exists so stability failures arrive from users who declined analytics; phase durations of a healthy preview are operational data about a normal session and do not qualify. Splitting per phase is not available either: every metric is a ratio, and a numerator and denominator drawn from different populations is not a rate. Framing (surface, open kind, attach trigger) is now fixed by whoever opens the attach instead of being re-supplied per phase, so two components cannot label one attach two different ways.
…tml-preview-bridge
The phase timing layer measures every preview phase against an anchor opened when the host settles on a navigation, and recordPhase fails closed without one. So until this lands, nothing the runtime components emit reaches the sink: handshake, capabilities, promotion, retention, recovery and eviction all stay silent. That failure mode is invisible by construction — the dashboard reads as a preview nobody opened, not as instrumentation that is half wired. The anchor opens on every attach including warm ones. Reusing a cold anchor for a warm re-attach would inherit the cold open's elapsed time and collapse the "restored within 100 ms" ratio, which reads as a product regression rather than a measurement bug. attach_trigger is derived by specificity, first match wins, and unknown is deliberately not reachable: it counts as an unsanctioned navigation by design, so labelling something unknown to avoid deciding surfaces as a red target-zero metric instead of being quietly absorbed. Warm means this exact document was already attached in this session, which is what the restore metric is about. The pool exposes no membership query, so this is tracked here rather than inferred from it. The spec pins the two labels the metrics turn on rather than merely asserting something was emitted. Its first case is a diagnostic rather than an independent control — removing the anchor reddens it too, since the anchor is the switch — but it separates "the sink was never installed" from "the sink is live and this phase never fired", which need different fixes.
The component preview understood import/export syntax with four regular
expressions and then downloaded @babel/standalone from unpkg to finish the job
inside the sandbox. Regexes do not understand JavaScript, and the failures were
not subtle. Observed against the previous implementation:
input const HINT = "写 export default function 就能预览";
output const HINT = "写 就能预览";
(typeof function !== 'undefined' ? function : null)
The string's contents were edited and the emitted expression was not valid
JavaScript, so a component whose copy happened to mention exporting killed its
own preview outright. `export * from './x'` survived verbatim into a classic
script, which is also a syntax error. A relative import was deleted in silence,
leaving the user with `Button is not defined` and nothing pointing at the cause.
Sucrase parses instead — its parser is a fork of Babel's — and it runs in the
host, so the document receives plain JavaScript. That removes the largest of the
three CDN scripts and lets the compiled source be inlined into a <script> tag
rather than eval'd from a string, which also restores real line numbers in
stack traces. Output is CommonJS; the harness supplies `require` and reads
module.exports. An unresolvable module is now named in the error instead of
disappearing.
Moving compilation into the host moved where a syntax error lands, so it is
caught here and rendered into the same error panel as before. Without that, a
typo in a component would throw inside the viewer's render path and take the
whole file viewer down; the spec covering it went red before this was added.
React and ReactDOM still come from unpkg. Serving them locally is the remaining
half of the offline defect and is left as an explicit todo in the spec rather
than implied by a passing suite.
The existing specs were rewritten rather than deleted: they pinned the old
mechanism, but the authoring shapes they protect — default function export,
named export, renamed hook imports, default re-export — are unchanged and are
now asserted against the compiled output.
Sucrase's CLI depends on commander@4, which took the hoist root and made
@deepseek-ai/dsh-cmdline resolve its types against the wrong major, breaking the
dsh-runtime build. Pinned through pnpm.overrides, alongside the thirteen
overrides already there; only the library entry is imported, never the CLI.
Seven of the eight preview phases already reported; first visible paint did not, so the one metric users actually feel — how long until something is on screen — had no source. The capability was already present: the bridge has carried visiblePaintCount() for white-screen detection all along. It only ever reported the failure, never the moment it succeeded. The primary detector relays the browser's own first-contentful-paint entry, a record it keeps whether or not anyone reads it, so observing costs nothing. A raf probe covers frames where no paint entry is recorded, and a timeout is the give-up verdict. In full: one synchronous getEntriesByType at arm, at most one PerformanceObserver disconnected on its first entry, at most two rAF callbacks, one cancellable timeout, and at most three of the existing bounded node walks. No interval, no resident timer, no loop, no DOM observer, no writes. The probe arms on a weaker predicate than the white-screen check and deliberately does not wait for readyState complete: a document that never finishes loading is exactly the one whose measurement is missing, and gating on complete would have made those rows vanish instead of arriving as timeouts. It times out ahead of the white-screen window so paint always speaks first, and a frame taken off screen releases rather than minting a false negative about something nobody was watching. The report travels on its own message type rather than as an observability event. reportPreviewIframeMessage ends in a catch-all that turns any parsed observability message into a runtime error, so routing it there would have published one fabricated error per healthy preview. This phase never gates promotion, retention, discard or reload; the payload carries observation_only so a dashboard author can see that inside PostHog without reading the code.
The remaining half of the offline defect stays a todo rather than a passing assertion, with the approach written where whoever picks it up will read it: generate the UMD builds from the installed react/react-dom at install and build time and gitignore the output, so the dependency stays the single source of truth and no vendored copy can drift. Relocating the fetch to another CDN was considered and rejected — it moves the external dependency rather than removing it. Whether that file loads inside the opaque-origin sandbox under the packaged od:// protocol is unverified and must be run on a real build before claiming it.
The bridge measures the paint and posts it; nothing recorded it. That completes the eight phases, and it is the one metric a user actually feels — how long until something is on screen. Leaving it unwired was not a neutral gap: an empty metric and a fast one look identical on a dashboard, so the absence would have read as "previews are instant". The report arrives on its own message type rather than as an observability event, because that channel ends in a catch-all which turns any parsed observability message into a runtime error — routing paint through it would publish one fabricated error for every healthy preview. Attribution requires a non-null contentWindow before comparing sources. A detached frame has none and a message with no source has none either, so an inequality check alone lets anything through at exactly the moment there is no document to attribute it to. Also records the working handoff for the pause point, including the measured compile costs so they are not re-litigated from intuition.
The preview document is its own browsing context with no allow-same-origin, so it cannot borrow the application's React and has to load a copy over the network. Loading it from unpkg meant this surface could not render in the packaged client offline or on machines that cannot reach a CDN — the same failure class the Preview Lab corpus run attributed 8 of its 12 white screens to. Compilation had already moved into the host, so the symptom was a readable "React preview runtime failed to load" rather than a white screen, but the surface was still unusable. The UMD builds are staged out of node_modules at install, dev and build time rather than committed. React is already a dependency of this app, so it stays the single source of truth: a version bump restages automatically, and there is no vendored duplicate for anyone to keep in sync. The output is gitignored, matching how the landing site already handles generated public assets. Relocating the fetch to another CDN was considered and rejected: it moves the external dependency instead of removing it, and offline still fails. Production builds, not development — 139 KB against 1.16 MB, and the harness renders thrown errors into its own panel either way. Two things the specs now hold that nothing else would have caught: that the document reaches no external host at all, and that the paths it references are actually staged. The second exists because those paths are strings on both sides with no build-time link between them, so a rename would only ever surface as a 404 the user meets as a failed runtime. The staged files are allowlisted in guard as generated vendor output.
…ransport Presenting a deck promoted the right document and then could not hide its chrome, because the presentation bridge never reached the document at all. The bridge was built as a URL-negotiated one (`odPreviewBridge=presentation`), which works on `/raw` and `/powered` — the paths the runtime convergence moved off. The settled document now loads from the scoped preview origin, and `buildPreviewSessionNavigation` only ever appends the passive guards there, deliberately: "Interactive Deck support is negotiated after navigation; it must never become part of the document URL." So the token was requested and never delivered, and measured live the document reported no `od:deck-presentation-ready` at all. Putting the switch in the URL is not the fix — it is the thing the convergence forbids. A URL change is a navigation, so toggling presentation that way would reload the very document presenting is supposed to keep alive: the audience would watch it blank and restart at slide one, losing scroll, animation and any embedded playback. So it is installed with the rest of the runtime instead, next to `observability`. That is safe by construction: the bridge only registers a message listener at parse time and does nothing until the host negotiates, and the host already posts `od:deck-presentation` on entering presentation. Both ends existed; only the middle was missing. This is not a regression — `deck-presentation.ts` does not exist at the merge base. The bridge is new on this branch and was never wired to the transport it was built for. Live, on the deck case, before -> after: bridgeReady false -> true acknowledged false -> true receiptAgreesWithDocument false -> true hiddenWhilePresenting false -> true restoredAfterExit true -> true document navigations 0 -> 0 (presenting still never navigates) Red spec carries its own control (`od:preview:hello` must be present, so a missing bridge is a missing module and not a broken probe) and goes red again with the module removed. It addresses the scoped origin through a raw Host header, because `fetch` silently drops one and the first version of the spec was passing against the wrong endpoint. Only the chrome-hiding contract is closed here. Click-to-advance now intercepts during presentation where it previously did not, but does not yet move the slide (`forwardAdvanced: false`) — not diagnosed, not claimed. daemon build exit 0, web typecheck exit 0, pnpm guard exit 0; contracts dist rebuilt for the new capability; 26 tests across the neighbouring daemon suites green.
lefarcen
added
size/XXL
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thank you for the substantial follow-up on this head. Installing the deck presentation bridge on the scoped origin, serving historical versions as real documents, and staging React from the app instead of a CDN are the right product shape.
I completed a fresh changed-range pass. The retained FileViewer runtime and presentation install look coherent. One merge-safe install issue is inline below: the new web postinstall breaks Docker image builds because the staging script is not in the install layer.
Two related follow-ups, same increment: non-deck plugin example previews now navigate the plugin preview route, so the existing containment CSP blocks CDN Tailwind and fonts that srcdoc used to allow; and Share to Export HTML for React components downloads the in-app vendor script URLs, which do not load outside this app. Relax that plugin preview CSP for iframe navigations, and inline or bundle the staged React files for export.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| "postinstall": "pnpm run stage:react-runtime", | ||
| "prebuild": "pnpm run stage:react-runtime", | ||
| "predev": "pnpm run stage:react-runtime", | ||
| "stage:react-runtime": "tsx ./scripts/copy-react-runtime.ts", |
There was a problem hiding this comment.
The new web postinstall always runs the React runtime copy script. That is correct for a full checkout, but the Docker build still copies only the web package manifest before the frozen lockfile install; the copy script is added later with the rest of apps.
Why it matters: the live image build on this head failed during web postinstall because the script file is not in the install layer, so Docker and Railway images cannot install. The web prebuild hook already restages before the Next build, so postinstall is not required for that later step once install succeeds.
Evidence: this package now declares postinstall plus prebuild and predev staging via the copy script. The Docker install layer special-cases the repo-root postinstall helper, but does not copy this new web script. The failed image job dies in web postinstall before the later apps copy.
Suggested change: drop postinstall and keep predev and prebuild, or copy the web staging script into the Docker install layer next to the web package manifest, or make staging a no-op when the script file is absent so a manifest-only install layer can succeed.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Confirmed still current on head e64849e. Live build-and-push on this head still dies in the install layer: apps/web postinstall runs tsx ./scripts/copy-react-runtime.ts and fails with ERR_MODULE_NOT_FOUND because deploy/Dockerfile copies only apps/web/package.json before pnpm install --frozen-lockfile. The two new commits did not touch this. The suggested fix is unchanged: drop postinstall and keep predev/prebuild, or copy the staging script into the Docker install layer, or make staging a no-op when the script file is absent.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
Opening a project URL that names a file could end up showing a different file — the one with the newest mtime — and then rewrite the URL to match, so it never self-corrected. The route-sync effect had no `routeFileName` guard. On the first commit the tab state has not hydrated, so `target` is null and it navigated with `fileName: null`, stripping the file out of the URL. That is not a cosmetic edit: the auto-open guard is `if (routeFileName) return` and it reads the *live* route, so stripping the file also disarms the only thing stopping `selectPrimaryProjectFile` — which ranks plain HTML files equally and breaks the tie on newest mtime — from opening a different file and latching it. The fix is the idiom already sitting two lines above for the conversation id (added by #1508 for the same deep-link reason): the routed value keeps authority until the local state is authoritative. Reachable with no cached tab state for the project — another machine, another browser, cleared site data, or a project this browser has never opened — and needs a slow load, which is why it shows up cold. Cached tab state is what protects the warm path. Evidence, by layer, because they prove different things: * Live, reproduced twice against a real runtime: requested `/projects/<p>/files/index.html`, browser settled on `/projects/<p>/conversations/<cid>/files/lru-6.html` (the last-created file) and stayed there 30s+. The server log shows only two page requests, so the navigation was client-side. * Unit, red before / green after, falsified by reverting the one line: the route sync clears the routed file from the URL. * NOT reproduced at unit level: the end-to-end wrong-file outcome. The remaining ordering could not be forced faithfully in jsdom, so that step rests on the live reproduction, not on this test. The existing suite could not have caught any of this: `ProjectView.tabs-navigation.test.tsx` mocks `navigate` and passes `routeFileName` as a static prop, so the URL -> `routeFileName` feedback the defect depends on cannot occur. The new spec wires that loop back up, and its control confirms auto-selection really does prefer the newest file. 369 tests across all 15 ProjectView suites green; web typecheck exit 0.
Clicking back during a deck presentation could bounce the slide forward again
and settle on the wrong one, roughly one time in three.
A presented deck has more than one voice. The authored document reports its
slide, and so does the injected deck runtime module — instrumenting every
writer showed both answering a single `go`, disagreeing while the move was
still in flight. The host adopted whichever arrived last, and because the
adopted value feeds `deckSlideIndex`, which the transport replays, a stale
answer became a fresh intent. That closes a loop. The measured write trace for
one backward click, with only the transport writing after the two legitimate
moves:
goToSlide:2, replay:2, goToSlide:1, replay:1,
replay:2, replay:1, replay:2, replay:1, ... (up to 59 writes, unbounded)
So a report is only authoritative once it agrees with what the host last asked
for; until then the host is mid-move and a disagreeing report is a stale voice,
not news. The intent expires after 1.5s so a deck that clamps the index, or has
no runtime to answer at all, can never freeze host state — this is a
tie-breaker, not a lock.
Measured on the live product, 8 runs each:
before 5 pass / 3 fail, 11-59 deck writes per run
after 7 pass / 0 fail, exactly 2 writes per run (1 run blocked)
The write count collapsing to 2 is the real evidence: the loop is gone, not
merely sampled at a luckier moment.
Three earlier theories were tested against the live product and discarded
rather than shipped: the speaker-notes popup (disproven twice — the
oscillation is identical with `window.open` neutralised), a stale closure over
`activeDeckSlideIndex`, and a stale `modeStateRef` in the transport (it is
assigned during render, so it is never stale).
The decision is a pure function with its own spec, including the expiry
boundary, because the wiring itself is not reachable from jsdom.
Pre-existing on this branch, unrelated and not touched here: two FileViewer
tests still assert the old layout where the presentation overlay owned its own
iframe ("allows downloads in the in-tab HTML presentation iframe", "closes deck
in-tab presentation when the present iframe forwards Escape"). Both fail
identically with these changes stashed.
web typecheck exit 0, pnpm guard exit 0, 361 of 363 FileViewer tests pass with
no new failures.
Two FileViewer specs still asserted the pre-convergence layout, where the presentation overlay owned its own iframe. Presenting now promotes the document already running, so `.present-overlay iframe` matches nothing and both had been failing on this branch independently of any recent change. Neither invariant is stale, only the element they looked at: * a presented document must still be able to download — checked on the preview frame's own sandbox, which carries `allow-downloads` (`NORMAL_PREVIEW_FRAME_SANDBOX`), and the overlay is now asserted to own no iframe at all * Escape forwarded from inside the sandboxed document must still tear the presentation down — the product handler already accounts for the new layout (it matches `ev.source` against the active preview frame); the spec was dispatching from the viewer's inert second frame, so it now selects `iframe[data-od-active="true"]` Checked the download case against the product before rewriting it rather than assuming: had the promoted frame lacked `allow-downloads`, this spec would have been reporting a real regression instead of a stale selector. 363 of 363 FileViewer tests pass; web typecheck exit 0; pnpm guard exit 0.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thank you for the deeplink file-wins fix, the deck-slide intent helper, and for pointing the presentation unit specs at the promoted frame. That is careful work on a hard runtime.
I completed a fresh changed-range pass on this head. Three merge-safe items are inline: the required presentation P0 still looks for an overlay iframe the unit tests now assert is gone, the new slide-intent gate is only armed from goToSlide, and exported React HTML still points at in-app /vendor/react-runtime URLs.
The existing Docker postinstall thread on apps/web/package.json is still open and confirmed by live build-and-push: install still runs stage:react-runtime before COPY apps, so copy-react-runtime.ts is missing and the image cannot install.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| // The invariant is unchanged — a presented document must still be able to | ||
| // download — but the frame it belongs to moved. Presenting promotes the | ||
| // document already running instead of minting a second one, so the overlay | ||
| // owns no iframe and the sandbox to check is the preview's own. | ||
| await waitFor(() => { | ||
| const frame = document.body.querySelector('.present-overlay iframe'); | ||
| expect(frame?.getAttribute('sandbox')).toBe('allow-scripts allow-downloads'); | ||
| expect(frame?.getAttribute('data-od-render-mode')).toBe('url-load'); | ||
| expect(container.querySelector('.html-viewer.is-tab-present')).toBeTruthy(); | ||
| }); | ||
| expect(container.querySelector('.html-viewer.is-tab-present')).toBeTruthy(); | ||
| expect(document.body.querySelector('.present-overlay iframe')).toBeNull(); | ||
| const presentedFrame = container.querySelector('.viewer.is-tab-present iframe'); | ||
| expect(presentedFrame).not.toBeNull(); | ||
| expect(presentedFrame?.getAttribute('sandbox')).toContain('allow-downloads'); |
There was a problem hiding this comment.
The unit tests now correctly assert that presenting promotes the running preview: this block expects .present-overlay iframe to be null and checks sandbox on .viewer.is-tab-present iframe instead.
The required P0 was not updated the same way. e2e/ui/app-manual-edit.test.ts:791 still does overlay.frameLocator('iframe[title="present"]') and waits for the "Slide One" heading. Live CI UI P0 (project-workspace-editor) failed that assertion on both the initial run and the retry (element(s) not found, 10s timeout).
Why it matters: this is a required P0. The product change is intentional — these unit tests document that the overlay owns no iframe — so the lane stays red even when in-tab presentation works.
Suggested change: retarget that e2e to the promoted active preview frame, matching this spec and the Escape test below. For example page.locator('.viewer.is-tab-present iframe[data-od-active="true"]') (or artifactPreviewFrame(page)). Keep asserting the overlay is visible and that Exit presentation still closes it.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Confirmed still current on head e64849e. e2e/ui/app-manual-edit.test.ts:791 still does overlay.frameLocator('iframe[title="present"]') and waits for the "Slide One" heading. The unit specs at this FileViewer block still expect .present-overlay iframe to be null. The two new commits did not retarget that P0. Please point the e2e at the promoted active preview frame (for example page.locator('.viewer.is-tab-present iframe[data-od-active="true"]') or artifactPreviewFrame(page)), keep asserting the overlay is visible, and keep Exit presentation closing it.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| const decision = decideDeckSlideReport( | ||
| pendingDeckSlideIntentRef.current, | ||
| data.active, | ||
| Date.now(), | ||
| ); | ||
| if (decision === 'ignore') return; | ||
| if (decision === 'adopt-and-settle') pendingDeckSlideIntentRef.current = null; |
There was a problem hiding this comment.
This new gate ignores a disagreeing od:slide-state only while pendingDeckSlideIntentRef is set. Two holes keep the measured 1↔2 replay loop reachable.
Pending is armed only in goToSlide (line 13971). Toolbar, keyboard, and the presentation-overlay prev/next controls still call postSlide('next'|'prev') without arming it. The queued-send slideNavRequest effect (lines 15653–15655) writes host slide state — and on the converged path lets PreviewRuntimeTransport replay od:slide go — without arming it either. decideDeckSlideReport(null, …) returns 'adopt', so a stale sibling report becomes host state and is echoed as a fresh go.
On 'adopt-and-settle' this handler also clears pending immediately. The bug this helper documents is two voices per go. If the confirming report arrives first, the late stale report sees pending === null and is adopted — same echo through deckSlideIndex → replayPreviewBridgeModes.
Suggested change: route host-initiated moves through goToSlide (or set pending before postSlide / setSlideState). After a matching confirm, keep ignoring mismatches until DECK_SLIDE_INTENT_TIMEOUT_MS without adopting them. Add a FileViewer test: goToSlide(1), then od:slide-state 1, then 2 within the window must not change host state or post go(2); slideNavRequest to 1 must arm the same gate.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Confirmed still current on head e64849e. pendingDeckSlideIntentRef is still armed only in goToSlide (now around 13978). postSlide('next'|'prev') still posts without arming it, and the slideNavRequest effect (15650–15662) still writes host slide state without arming it. decideDeckSlideReport(null, …) still returns 'adopt', so a stale sibling report can still become host state and echo as a fresh go. The two new commits did not change this path. Please route host-initiated moves through goToSlide (or set pending before postSlide / setSlideState), and after a matching confirm keep ignoring mismatches until DECK_SLIDE_INTENT_TIMEOUT_MS without adopting them.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| const REACT_URL = '/vendor/react-runtime/react.production.min.js'; | ||
| const REACT_DOM_URL = '/vendor/react-runtime/react-dom.production.min.js'; |
There was a problem hiding this comment.
These origin-root /vendor/react-runtime/… URLs are correct for the in-app srcdoc: the preview resolves them against the Open Design origin after stage:react-runtime. exportReactComponentAsHtml in apps/web/src/runtime/exports.ts (around line 830) downloads buildReactComponentSrcdoc() unchanged, so a file opened outside this app 404s on React — the same white-screen class the CDN removal was meant to kill. react-component.test.ts currently locks the /vendor/ URL in as the success contract.
Why it matters: Share → Export HTML is a user-facing download. The in-app preview can look fine while the exported file is blank.
Suggested change: keep host-root URLs for in-app preview; for export, inline the staged UMDs or emit sibling files with relative src. Split the test so the in-app contract can stay /vendor/ while exported HTML must not depend on the Open Design origin.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Confirmed still current on head e64849e. REACT_URL / REACT_DOM_URL are still origin-root /vendor/react-runtime/…, and exportReactComponentAsHtml in apps/web/src/runtime/exports.ts still downloads buildReactComponentSrcdoc() unchanged. The two new commits did not split the in-app vs export contracts. Please keep host-root URLs for in-app preview; for export, inline the staged UMDs or emit sibling files with relative src. Split the test so the in-app contract can stay /vendor/ while exported HTML must not depend on the Open Design origin.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
Saving from Manual Edit and leaving replaced the whole preview document, so the page lost everything it was holding — canvas pixels, timers, scroll, form state. Entering and leaving without saving was fine, which is what made this look like a scroll-restore problem rather than a document swap. There is already a mechanism to prevent exactly this. `syncRetainedManualEditDocument` sets a latch once the Manual Edit bridge has applied the exact persisted bytes to the live document, and `previewRuntimeCanAdoptPersistedManualEditDocument` reads it to keep the document identity stable through the watcher echo the save produces. The comments there say the latch is meant to be consumed after Edit closes. `exitManualEditModeAfterFlush` cleared it in the same synchronous block that closed edit mode, one step before the render that would have read it — so it was always null at the point of use and the mechanism had never once taken effect. The latch releases itself when the source genuinely differs, so clearing it on exit was both redundant and harmful. Measured on the live product with a temporary probe at the decision point: probe patchKind: set-style, matched: true <- "no reload needed" witness document reborn 2.7s later, canvas ink 0 <- replaced anyway Set correctly, then discarded before it could be used. User confirmed the fix against the same reproduction. The adopt rule is extracted to `manual-edit-document-latch.ts` with its own spec, including the two self-release cases, because the call site is an inline expression inside a 20k-line component and the invariant deserves to be readable on its own. Also disproven along the way, and worth not re-investigating: `scroll` is in `BASE_VIEWER_CAPABILITIES` and is never disabled by an edit-mode exit, so the `&&scrollEnabled` drop in the scroll runtime module is not reachable this way. Lost scroll was a symptom of the document swap, not a restore failure. Adjacent, verified, not fixed here: `od:preview-scroll-capture` has no receiver on the converged transport — only the legacy srcDoc bridge implements it — so the exact-capture round trip always times out at its 120ms budget and silently degrades to the last unsolicited scroll report. 369 tests across FileViewer plus the new spec pass; web typecheck exit 0.
… transport The host cannot read scroll out of an opaque-origin preview document, so before a mode change it asks the document for its exact position and waits on that answer, keyed by `requestId`, with a 120ms budget. Only the legacy srcDoc bridge ever implemented that request (`preview-runtime/src/srcdoc.ts`). The scroll runtime module — which is what the converged transport actually installs — handled `-restore`, `-by` and `-content-size-request` and simply ignored `-capture`, and its `postScroll()` had no way to carry a `requestId` even if it had. So on the transport this refactor moved everything onto, every capture burned the full 120ms and then silently degraded to whatever unsolicited scroll report happened to be most recent. Two costs, both silent: a dead 120ms await before every manual-edit commit that takes this path, and a scroll restore working from an approximate position instead of the exact one it asked for. `postScroll` now takes an optional id and echoes it only when answering a request — an unsolicited report must stay unsolicited, because the host routes the two through completely different branches. Red spec drives the module through the real bootstrap and asserts the reply carries the id; it goes red again with the handler removed. 91 daemon http tests pass, daemon typecheck 0, daemon build 0, pnpm guard 0. Found while tracing a different defect (state lost on leaving Manual Edit), which turned out to be a document swap rather than a restore failure — this one is real but was not the cause of that.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thank you for the Manual Edit latch fix and for answering exact-scroll capture on the converged transport. Those two commits look right for ordinary HTML: leaving Edit no longer drops the latch before the next render can freeze document identity, and the scroll module now echoes requestId so the host's 120ms capture can resolve.
I completed another full changed-range pass on this head. One new coupling issue is inline: the runtime adopt path keys the latch on raw source, but a later retire in the same render keys it on livePreviewSource, which undoes the keep-the-document fix for decks.
The four existing threads are still current here (Docker postinstall / image build, presentation P0 overlay iframe, slide-intent arming, and exported React HTML). I am replying on those threads rather than duplicating them.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.| const previewRuntimeCanAdoptPersistedManualEditDocument = | ||
| shouldAdoptPersistedManualEditDocument({ | ||
| manualEditMode, | ||
| manualEditSrcDocActive, | ||
| latch: manualEditPersistedDocumentRef.current, | ||
| reloadKey, | ||
| sourceFingerprint: source === null ? null : previewSourceFingerprint(source), | ||
| }); |
There was a problem hiding this comment.
This new adopt call keys the Manual Edit latch on raw source, but a later retire in the same render still keys it on a different byte domain, so the keep-the-document fix does not hold for decks.
shouldAdoptPersistedManualEditDocument here compares previewSourceFingerprint(source) with the latch armed from persisted file bytes in syncRetainedManualEditDocument. That is the identity freeze dade39a needs. Later in the same HtmlViewer render, unchanged code at 10656–10672 still does:
const canAdoptPersistedManualEditDocument =
__omp_shell("interactivePreviewModeActive")
&& persistedManualEditDocument?.reloadKey === reloadKey
&& livePreviewSource != null
&& persistedManualEditDocument.sourceFingerprint === previewSourceFingerprint(livePreviewSource);
if (
__omp_shell("interactivePreviewModeActive")
&& persistedManualEditDocument
&& livePreviewSource != null
&& !canAdoptPersistedManualEditDocument
) {
manualEditPersistedDocumentRef.current = null;
}livePreviewSource is inlinedSource ?? deckVisualSource, and for a deck deckVisualSource is normalizeDeckVisualSource(removeSpeakerNotesFromHtml(source)). normalizeDeckVisualSource strips whitespace before </body> and trimEnd()s, so pretty-printed decks have fp(livePreviewSource) !== fp(source) even with no speaker notes.
What happens after a saved deck edit closes:
- First render: this adopt is true (
sourcematches) →previewRuntimeRevisionIdentitystays frozen. - Same render: the retire sees a fingerprint miss and clears the latch in place (no setState).
- The watcher echo (new mtime / content-refresh, same bytes) re-renders with the latch already gone → identity updates →
/preview-urlremint → the browsing context is replaced.
That is the document-swap this commit measured and claimed to stop (canvas pixels, timers, scroll, form state). Ordinary personal non-deck HTML is unaffected because livePreviewSource === source there.
Inverse of the same coupling: on the converged URL-load path, setInlinedSource(null) returns immediately, so team HTML with relative assets keeps livePreviewSource === null and the retire never runs. A later revert to the saved bytes can then re-adopt and skip a remint — the case the retire comment says must not happen.
Suggested change: retire the runtime latch with the same fingerprint domain as this adopt (source === null ? null : previewSourceFingerprint(source)), via shouldAdoptPersistedManualEditDocument. Keep the srcDoc freeze on livePreviewSource if that transport still needs it, but do not share that boolean as the runtime latch self-release. A FileViewer regression that saves a pretty-printed deck from Manual Edit and asserts the same iframe node survives the watcher echo would lock this in.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
Entering or leaving presentation flashed solid black for ~350 ms, roughly one attempt in six. Every measurement showed `navigationCount: 0` and `frameLoads: 0`, so nothing reloaded — the preview frame is relocated to a new host, the browser recomposites, and for a frame the new box is empty. The flash was black rather than merely empty because `.viewer.is-tab-present .viewer-body` changed layout AND painted an opaque ground in the same rule, so the ground composited while the document had not yet repainted into its new box. The ground is not decoration — slides that do not fill the window need it for letterbox bars — so it moves rather than goes: `.present-backdrop` is its own fixed layer beneath the promoted preview, whose box never changes, mounted with the presentation and faded in once the document has painted. Three earlier attempts are recorded in the handoff because they are the instructive part. Delaying the ground by a frame fixed entry only. Dropping it before the layout in the same handler did nothing, since React batches both updates into one commit — so "first" and "second" were the same render. Two `requestAnimationFrame`s survived eight deck-only runs, then flashed on the next FULL run, where deck is the third case and the browser is busier. All three tried to TIME the ground against a reflow, which is a bet that loses when load changes; a constant box removes the possibility instead. Each wrong version was caught by running again, never by re-reading the code. Live, deck case: before, ~1 in 6 at ~350 ms. After, three full rounds — nine case-transitions under the load that reproduced it — all 0 ms, with presenting still hiding authored deck chrome and restoring it on exit. The underlying relocation flash is NOT fixed and is now documented as one item rather than five: without an opaque ground the empty frame shows the page background instead of black, still measurable on file-tab, project and LRU transitions. Fixing that means changing what the frame shows while it moves, which touches every preview path and is an architectural decision — five separate timing patches would repeat exactly the mistake above. 367 FileViewer tests plus the new spec pass; web typecheck 0; pnpm guard 0.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thank you for the presentation-backdrop follow-up. Moving the letterbox ground onto .present-backdrop so .viewer.is-tab-present .viewer-body can stay transparent is the right shape for the black flash you measured.
I completed another full changed-range pass on this head. The four-file presentation commit does not introduce a new issue. The five existing threads remain current on apps/web/package.json (postinstall), e2e/ui/app-manual-edit.test.ts (presentation P0 iframe), FileViewer.tsx (pendingDeckSlideIntentRef and the Manual Edit latch), and apps/web/src/runtime/react-component.ts (REACT_URL). Detailed confirmation is inline.
Because this reviewer has already reached the three-request-changes cap for this PR, I am leaving these as COMMENT for maintainer follow-up rather than opening another blocking review. Grateful for the careful iteration on a hard runtime.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| "typecheck": "tsc -b --noEmit", | ||
| "test": "vitest run -c vitest.config.ts --maxWorkers=2" | ||
| "dev": "next dev --turbopack", | ||
| "postinstall": "pnpm run stage:react-runtime", |
There was a problem hiding this comment.
The new web postinstall still always runs the React runtime copy script. That is correct for a full checkout, but the Docker build still copies only the web package manifest before the frozen lockfile install; the copy script is added later with the rest of apps.
Why it matters: live build-and-push on this head (9a7ee07, run 33908357612) still fails in the install layer. apps/web postinstall runs tsx ./scripts/copy-react-runtime.ts and dies with ERR_MODULE_NOT_FOUND because deploy/Dockerfile copies only apps/web/package.json before pnpm install --frozen-lockfile. Docker and Railway images cannot install. The presentation-backdrop commit did not touch this path. The web prebuild hook already restages before the Next build, so postinstall is not required for that later step once install succeeds.
Evidence: this postinstall line is still present, Dockerfile still copies the web manifest at line 29 then installs at lines 31–33, and only afterwards COPY apps ./apps. The failed job log shows Cannot find module '/app/apps/web/scripts/copy-react-runtime.ts'.
Suggested change: drop postinstall and keep predev/prebuild, or copy apps/web/scripts/copy-react-runtime.ts into the Docker install layer, or make staging a no-op when the script file is absent.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on 427ee93. This path-validation commit does not touch the web install layer.
apps/web/package.json still has unconditional postinstall: pnpm run stage:react-runtime, and deploy/Dockerfile still copies only apps/web/package.json before pnpm install --frozen-lockfile. Live build-and-push on this head still fails with ERR_MODULE_NOT_FOUND for /app/apps/web/scripts/copy-react-runtime.ts.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| }); | ||
| expect(container.querySelector('.html-viewer.is-tab-present')).toBeTruthy(); | ||
| expect(document.body.querySelector('.present-overlay iframe')).toBeNull(); |
There was a problem hiding this comment.
The unit tests now correctly assert that presenting promotes the running preview: this block expects .present-overlay iframe to be null and checks sandbox on .viewer.is-tab-present iframe instead.
The required P0 was not updated the same way. e2e/ui/app-manual-edit.test.ts:791 still does overlay.frameLocator('iframe[title="present"]') and waits for the "Slide One" heading. The new .present-backdrop portal has no iframe and does not retarget that P0.
Why it matters: this is a required P0. The product change is intentional — presenting no longer mints a second browsing context — so the e2e as written cannot observe the slide and will time out looking for an overlay iframe that this test already asserts is gone.
Evidence: this added assertion is expect(document.body.querySelector('.present-overlay iframe')).toBeNull(), while e2e/ui/app-manual-edit.test.ts lines 789–793 still locate .present-overlay then iframe[title="present"]. Those e2e lines are unchanged on this head.
Suggested change: point the e2e at the promoted active preview frame (for example page.locator('.viewer.is-tab-present iframe[data-od-active="true"]') or artifactPreviewFrame(page)), keep asserting the overlay is visible, and keep Exit presentation closing it.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on 427ee93. This commit does not update the presentation P0.
e2e/ui/app-manual-edit.test.ts:791 still does overlay.frameLocator('iframe[title="present"]'). Live UI P0 (project-workspace-editor) on this head still fails waiting for .present-overlay iframe[title="present"] → heading Slide One.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| Date.now(), | ||
| ); | ||
| if (decision === 'ignore') return; | ||
| if (decision === 'adopt-and-settle') pendingDeckSlideIntentRef.current = null; |
There was a problem hiding this comment.
This gate ignores a disagreeing od:slide-state only while pendingDeckSlideIntentRef is set. Two holes keep the measured 1↔2 replay loop reachable.
Pending is armed only in goToSlide (line 13982). Toolbar, keyboard, and the presentation-overlay prev/next controls still call postSlide('next'|'prev') without arming it. The queued-send slideNavRequest effect (lines 15686–15697) writes host slide state — and on the converged path lets PreviewRuntimeTransport replay od:slide go — without arming it either. decideDeckSlideReport(null, …) returns 'adopt', so a stale sibling report can still become host state and echo as a fresh go.
Why it matters: presenting a deck still has two reporters. A host-initiated prev/next that never sets pending will adopt the in-flight disagreeing report and re-enter the oscillation this helper was added to stop.
Evidence: pendingDeckSlideIntentRef.current = { index: target, atMs: Date.now() } exists only in goToSlide. postSlide at 13958 has no pending write. Keyboard 14251/14254, toolbar 16950/16967, and overlay 18169/18186 still call postSlide('next'|'prev'). The presentation-backdrop commit did not change this path.
Suggested change: route host-initiated moves through goToSlide (or set pending before postSlide / setSlideState), and after a matching confirmation clear the pending intent as this adopt-and-settle branch already does.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on 427ee93. This commit does not touch deck slide intent.
pendingDeckSlideIntentRef is still armed only in goToSlide (around 13982). Toolbar, keyboard, and the presentation overlay still call postSlide('next'|'prev') directly, so a disagreeing od:slide-state is still adopted.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| // not committed. Relative to the embedding document, so a preview served from | ||
| // any origin resolves them against that origin. | ||
| const REACT_URL = '/vendor/react-runtime/react.production.min.js'; | ||
| const REACT_DOM_URL = '/vendor/react-runtime/react-dom.production.min.js'; |
There was a problem hiding this comment.
These origin-root /vendor/react-runtime/… URLs are correct for the in-app srcdoc: the preview resolves them against the Open Design origin after stage:react-runtime. exportReactComponentAsHtml in apps/web/src/runtime/exports.ts (around line 830) still downloads buildReactComponentSrcdoc() unchanged, so a file opened outside this app 404s on React — the same white-screen class the CDN removal was meant to kill.
Why it matters: Share → Export HTML is a user-facing download. The in-app preview can look fine while the exported file is blank anywhere that is not this origin.
Evidence: REACT_URL / REACT_DOM_URL are still /vendor/react-runtime/react.production.min.js and …/react-dom.production.min.js. exportReactComponentAsHtml still does const doc = buildReactComponentSrcdoc(source, { title }) with no export-specific URLs. The presentation-backdrop commit did not split the in-app vs export contracts.
Suggested change: keep host-root URLs for in-app preview; for export, inline the staged UMDs or emit sibling files with relative src. Split the test so the in-app contract can stay /vendor/ while exported HTML must not depend on the Open Design origin.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on 427ee93. This commit does not touch the React runtime URLs.
REACT_URL / REACT_DOM_URL in apps/web/src/runtime/react-component.ts are still origin-root /vendor/react-runtime/…. exportReactComponentAsHtml still serializes buildReactComponentSrcdoc() unchanged, so a saved export still cannot resolve those scripts.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| manualEditSrcDocActive, | ||
| latch: manualEditPersistedDocumentRef.current, | ||
| reloadKey, | ||
| sourceFingerprint: source === null ? null : previewSourceFingerprint(source), |
There was a problem hiding this comment.
This adopt call keys the Manual Edit latch on raw source, but a later retire in the same render still keys it on a different byte domain, so the keep-the-document fix does not hold for decks.
shouldAdoptPersistedManualEditDocument here compares previewSourceFingerprint(source) with the latch armed from persisted file bytes in syncRetainedManualEditDocument. That is the identity freeze dade39a needs. Later in the same HtmlViewer render, unchanged code at 10660–10676 still does:
const canAdoptPersistedManualEditDocument =
__omp_shell("interactivePreviewModeActive")
&& persistedManualEditDocument?.reloadKey === reloadKey
&& livePreviewSource != null
&& persistedManualEditDocument.sourceFingerprint === previewSourceFingerprint(livePreviewSource);For decks, livePreviewSource is inlinedSource ?? deckVisualSource, not raw source. A matching persist can therefore be adopted here and then retired in the same render because the fingerprints disagree.
Why it matters: leaving Manual Edit on a deck can still drop the latch before the next render consumes it, which is the reload-the-live-document bug this latch was added to stop.
Evidence: adopt fingerprint is source at this line; retire fingerprint is livePreviewSource at 10664. livePreviewSource is assigned at 10212–10214 as inlinedSource ?? deckVisualSource. The presentation-backdrop commit did not change either domain.
Suggested change: key both adopt and retire on the same byte domain the document actually displays (livePreviewSource / previewSource), and do not retire in the same render that just adopted.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on 427ee93. This commit does not touch the Manual Edit latch.
shouldAdoptPersistedManualEditDocument here still receives previewSourceFingerprint(source) (raw source). The later retire path still keys a different byte domain, so the keep-the-document fix still does not hold for decks.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
|
Rechecked Once those are fixed and the failing checks are green again, this should be ready for another pass. |
…rite
Writing a file whose name starts with `./` failed. `./x` is how HTML writes a
sibling reference and names exactly the same file as `x`, so a document could
declare a resource the write API then refused.
Found by running real artifacts through the product: 12 of 400 design-system
artifacts failed on `./DESIGN.md`, and an earlier batch failed on
`./assets/template.html`. Reduced against the running daemon, where the shape
is unmistakable:
DESIGN.md -> 200
./DESIGN.md -> 500
assets/template.html -> 200
./assets/template.html -> 500
`validateProjectPath` rejected `.` and `..` with one pattern, but they are not
the same thing: `..` escapes the project and must stay rejected, while `.` is a
no-op segment. It is now dropped during normalisation rather than rejected.
The traversal guard is unchanged and verified live after the fix, including the
combination that would matter if `.` were handled carelessly:
../escape.txt still rejected
./../escape.txt still rejected <- `./` cannot smuggle a `..`
/abs.txt still rejected
./.file-versions/x.html still rejected <- reserved internal directory
Normalisation is verified by what lands on disk, not just by the status code:
after writing `./DESIGN.md` and `assets/./styles.css` the project lists
`DESIGN.md` and `assets/styles.css`.
Spec goes red again with the filter reverted; 28 tests across the neighbouring
path/security suites pass; daemon typecheck 0, daemon build 0, pnpm guard 0.
Separate and NOT changed here: these validation failures surface as 500 rather
than 400. A rejected path is bad input, not a server fault, and the status
should say so — recorded in the handoff rather than folded into this fix.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thank you for the careful ./ path follow-up. Dropping no-op . segments while keeping .., empty names, and reserved directories rejected is the right split, and the new project-path-dot-segments cases match the measured DESIGN.md / assets/template.html 500s.
I reviewed the two-file delta on this head (validateProjectPath plus the new daemon tests) and re-checked the open threads against the live tree and CI. This commit does not introduce a new issue.
The five existing threads remain current, so I am not duplicating them as new inline comments:
apps/web/package.jsonstill runsstage:react-runtimeon everypostinstall, anddeploy/Dockerfilestill copies only the web manifest beforepnpm install. Livebuild-and-pushon this head still dies withERR_MODULE_NOT_FOUNDforcopy-react-runtime.ts.e2e/ui/app-manual-edit.test.tsstill looks for.present-overlay iframe[title="present"].UI P0 (project-workspace-editor)failed on this head at that locator.pendingDeckSlideIntentRefis still armed only ingoToSlide; toolbar/keyboardpostSlide('next'|'prev')still skip it.REACT_URL/REACT_DOM_URLinapps/web/src/runtime/react-component.tsare still origin-root/vendor/react-runtime/….- Manual Edit latch adopt in
FileViewerstill fingerprints rawsource, so the keep-the-document fix still does not hold for decks.
Happy to re-review once those threads move. The path fix itself is in good shape.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
Saving a single HTML file re-navigated the retained previews of every other open document. Measured live through the L2 harness: saving `index.html` reloaded `lru-5.html` and `lru-6.html`, each coming back with an unchanged content hash and a brand-new preview session — proof they were reloaded because of bookkeeping, not because their content moved. A reload discards the document's JS heap, timers, canvas and scroll, which is exactly the loss the retained-frame runtime exists to prevent. Three writes reach the file watcher for one user-visible save, and only one of them is user content: - the daemon's own `.file-versions` snapshot and its manifest, which the daemon refuses user writes into and excludes from member mirrors; - the `<name>.artifact.json` sidecar, generated metadata the daemon hides from the file listing and the app hides from the file tree; - the document itself. The first is now silenced at the watcher, sourced from the same reserved- segment set the write API already enforces so the two cannot drift. The second is attributed to the document it describes. The third scopes to itself: an HTML document is authored to be opened, not referenced, so editing one does not entitle it to disturb the others. Appearing and disappearing files keep the project-wide refresh, because a reference that just started or stopped resolving leaves an open page showing real breakage that only a reload repairs. The retained-frame pool follows the same rule, so a scoped change no longer destroys the parked browsing contexts of documents that did not change. Residual risk, narrow and nameable: page A embeds page B, B is edited, and A keeps showing the previous B until anything else in the project changes or A is reloaded by hand.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thank you for pinning one HTML save to that document. ProjectView.handleProjectEvent now scopes an existing HTML change through changeScopesToItself, artifactManifestSubjectPath attributes *.artifact.json to its subject, and makeIgnored / WATCHER_ONLY_IGNORE_NAMES silence RESERVED_PROJECT_FILE_SEGMENTS. The new cases in ProjectView.html-change-scope.test.tsx and project-watchers.test.ts make that line hold.
These two commits do not touch the five still-open threads; the inline notes below are replies on those same locations.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| "typecheck": "tsc -b --noEmit", | ||
| "test": "vitest run -c vitest.config.ts --maxWorkers=2" | ||
| "dev": "next dev --turbopack", | ||
| "postinstall": "pnpm run stage:react-runtime", |
There was a problem hiding this comment.
Still current on f7e73fd. These two commits do not touch the web install layer.
apps/web/package.json still has unconditional postinstall: pnpm run stage:react-runtime, and deploy/Dockerfile still copies only apps/web/package.json before pnpm install --frozen-lockfile. Live build-and-push on this head still fails with ERR_MODULE_NOT_FOUND for apps/web/scripts/copy-react-runtime.ts during the install layer.
Please drop postinstall (keep prebuild / predev), or copy the staging script into the Docker install layer before pnpm install.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on b4eaa06. The new commit is docs-only and does not touch the web install layer.
apps/web/package.json still has unconditional postinstall: pnpm run stage:react-runtime (tsx ./scripts/copy-react-runtime.ts). deploy/Dockerfile still copies only apps/web/package.json at line 29, then runs pnpm install --frozen-lockfile at lines 31–33, and only copies apps afterward at line 35. Live build-and-push on this head (run 33944910082) still fails with ERR_MODULE_NOT_FOUND for apps/web/scripts/copy-react-runtime.ts.
Suggested fix: copy apps/web/scripts/copy-react-runtime.ts into the install layer before pnpm install, or make postinstall a no-op when that script is absent and keep prebuild / predev as the real staging hooks.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| }); | ||
| expect(container.querySelector('.html-viewer.is-tab-present')).toBeTruthy(); | ||
| expect(document.body.querySelector('.present-overlay iframe')).toBeNull(); |
There was a problem hiding this comment.
Still current on f7e73fd. These two commits do not retarget the presentation P0.
This unit spec still expects .present-overlay iframe to be null and checks sandbox on .viewer.is-tab-present iframe. e2e/ui/app-manual-edit.test.ts:791 still does overlay.frameLocator('iframe[title="present"]') and waits for the "Slide One" heading, so the required UI job is still looking for a frame the unit tests now assert is gone.
Please point that P0 at the promoted active preview frame (for example page.locator('.viewer.is-tab-present iframe')) so the e2e contract matches this block.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on b4eaa06. The docs-only commit does not retarget the presentation P0.
This unit spec still expects .present-overlay iframe to be null and checks sandbox on .viewer.is-tab-present iframe. e2e/ui/app-manual-edit.test.ts:791 still does overlay.frameLocator('iframe[title="present"]') and waits for the "Slide One" heading, so the required UI job is still looking for a frame the unit tests now assert is gone. Live UI P0 (project-workspace-editor) is red on this head.
Suggested fix: retarget that P0 to the promoted .viewer.is-tab-present iframe, matching the unit spec in this file.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| Date.now(), | ||
| ); | ||
| if (decision === 'ignore') return; | ||
| if (decision === 'adopt-and-settle') pendingDeckSlideIntentRef.current = null; |
There was a problem hiding this comment.
Still current on f7e73fd. These two commits do not touch deck slide intent.
pendingDeckSlideIntentRef is still armed only in goToSlide (around 13982). Toolbar, keyboard, and the presentation overlay still call postSlide('next'|'prev') without arming it, and the slideNavRequest effect (15686–15698) still writes host slide state without arming it. decideDeckSlideReport(null, …) still returns 'adopt', so a stale sibling report can still become host state and echo.
Please arm the same pending intent from every host-initiated move (postSlide('next'|'prev') and the queued slide-nav effect), not only goToSlide.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on b4eaa06. The docs-only commit does not touch deck slide intent.
pendingDeckSlideIntentRef is still armed only in goToSlide (line 13982). postSlide (lines 13958–13974) still posts next / prev without arming it, so toolbar, keyboard, and presentation controls stay unguarded. The slideNavRequest effect (lines 15686–15706) still writes host slide state without arming it, and decideDeckSlideReport(null, …) still receives a disagreeing od:slide-state as news.
Suggested fix: arm pendingDeckSlideIntentRef with the intended index inside postSlide for next / prev, and in the slideNavRequest effect before setSlideState.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| // not committed. Relative to the embedding document, so a preview served from | ||
| // any origin resolves them against that origin. | ||
| const REACT_URL = '/vendor/react-runtime/react.production.min.js'; | ||
| const REACT_DOM_URL = '/vendor/react-runtime/react-dom.production.min.js'; |
There was a problem hiding this comment.
Still current on f7e73fd. These two commits do not split the in-app vs export React contracts.
REACT_URL / REACT_DOM_URL here are still origin-root /vendor/react-runtime/…. exportReactComponentAsHtml in apps/web/src/runtime/exports.ts still serializes buildReactComponentSrcdoc() unchanged, so a file opened outside this app still 404s on React — the same white-screen class the CDN removal was meant to close.
Please keep host-root URLs for in-app preview; for export, inline the staged UMDs or rewrite those script tags to file-relative copies that travel with the downloaded HTML.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on b4eaa06. The docs-only commit does not split the in-app vs export React contracts.
REACT_URL / REACT_DOM_URL here are still origin-root /vendor/react-runtime/…. exportReactComponentAsHtml in apps/web/src/runtime/exports.ts (lines 829–832) still downloads buildReactComponentSrcdoc() unchanged, so a file opened outside this app 404s on React.
Suggested fix: keep origin-root URLs for in-app srcdoc, and have the export path inline or rewrite to file-relative bundled React so a downloaded HTML file does not 404.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
| manualEditSrcDocActive, | ||
| latch: manualEditPersistedDocumentRef.current, | ||
| reloadKey, | ||
| sourceFingerprint: source === null ? null : previewSourceFingerprint(source), |
There was a problem hiding this comment.
Still current on f7e73fd. These two commits do not touch the Manual Edit latch.
shouldAdoptPersistedManualEditDocument here still receives previewSourceFingerprint(source) (raw source). The later retire path still keys livePreviewSource (inlinedSource ?? deckVisualSource), so a deck whose visual bytes differ from persisted file bytes still drops the latch in the same render and replaces the document the bridge already applied.
Please fingerprint the same byte domain on adopt, arm, and retire — livePreviewSource (or the saved bytes the bridge actually applied), not raw source.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
There was a problem hiding this comment.
Still current on b4eaa06. The docs-only commit does not touch the Manual Edit latch.
shouldAdoptPersistedManualEditDocument here still receives previewSourceFingerprint(source) (raw source at line 9563). The later retire path (lines 10660–10676) still keys livePreviewSource (inlinedSource ?? deckVisualSource), so a deck whose visual bytes differ from persisted file bytes still drops the latch in the same render and replaces the document the save just proved exact.
Suggested fix: pass the same fingerprint domain to both adopt and retire — either previewSourceFingerprint(livePreviewSource) at this call, or one shared helper both sites call.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@lefarcen thank you for the handoff note on the dot-entry line and the agent re-probe cost. collectFiles in apps/daemon/src/projects.ts still skips every e.name.startsWith('.') entry at line 306, and detectAgents in apps/daemon/src/runtimes/detection.ts still probes every agent def per request at line 789; those notes correctly stay out of this branch.
I completed a fresh changed-range pass on this head. The only delta since the last review is that docs commit, so there is no new runtime finding. These five existing threads remain current, and I confirmed them there rather than opening duplicates:
apps/web/package.jsonpostinstall/stage:react-runtimestill runstsx ./scripts/copy-react-runtime.tswhiledeploy/Dockerfilecopies only the web manifest beforepnpm install --frozen-lockfile, so livebuild-and-pushstill dies withERR_MODULE_NOT_FOUND.e2e/ui/app-manual-edit.test.tsline 791 still doesoverlay.frameLocator('iframe[title="present"]')even thoughapps/web/tests/components/FileViewer.test.tsxnow expects.present-overlay iframeto be null.pendingDeckSlideIntentRefinapps/web/src/components/FileViewer.tsxis still armed only ingoToSlide;postSlide('next'|'prev')and theslideNavRequesteffect still do not arm it.REACT_URL/REACT_DOM_URLinapps/web/src/runtime/react-component.tsare still origin-root/vendor/react-runtime/…, andexportReactComponentAsHtmlinapps/web/src/runtime/exports.tsstill serializesbuildReactComponentSrcdoc()unchanged.shouldAdoptPersistedManualEditDocumentinFileViewer.tsxstill fingerprints rawsource, while the retire path still fingerprintslivePreviewSource(inlinedSource ?? deckVisualSource).
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
3 participants
Fixes #7278
Why
HTML preview had accumulated several mutually exclusive transports: a real project URL for passive viewing, srcdoc/Blob snapshots for injected bridges, and separate recovery/parking paths for Electron. A routine action such as switching File tabs, entering Edit, returning from Code, or receiving an agent-written revision could therefore navigate a different iframe. Users saw blank previews, loading covers, flashes, lost scroll/deck state, stale edit content, and broken relative CSS/JS—especially for large or multi-file prototypes such as #7278.
This PR converges the product on one project-scoped real-URL document runtime per HTML file. Opening the document, enabling capabilities, retaining state, and replacing a changed document are now separate responsibilities. The currently painted document stays on screen while a candidate revision loads, proves its exact identity/capabilities/visible paint, and is then promoted atomically.
What users will see
support.js, Babel/JSX, images, fonts, or other project files—open from their real project URL instead of falling back to a Blob/srcdoc navigation.Surface area
apps/web; no new entry point or controlodsubcommand or flag, newtools-dev/tools-packflag, or newOD_*env varskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies); workspace-packagepackage.jsonfiles are out of scopeScreenshots
No new UI entry point. This changes the behavior of the existing FileViewer. Packaged-Electron evidence was captured against the existing Preview/Code toolbar and file tabs; the acceptance checks below continuously sampled the actual visible iframe during Reload and capability transitions rather than relying on a static screenshot.
Bug fix verification
apps/daemon/tests/project-file-range.test.tsandapps/daemon/tests/http/html-stream-injection.test.tscover the former >2 MiB bridge omission, relative resources, chunk boundaries, raw-text elements, malformed heads, ranges, guards, and redirects.apps/web/tests/components/PreviewSessionFrames.test.tsxcovers last-good retention, same-version reload attempts, timeout/retry, exact promotion, state suspension, privilege changes, and old-frame eviction.apps/web/tests/components/FileViewer.test.tsxcovers terminal real-URL Edit/Comment/Draw/Deck transitions, File/Project switching, agent-written revisions, mint retry, explicit Reload, no redundant scope mint, and stale/aborted navigation rejection.apps/desktop/tests/main/preview-navigation-failure.test.tscovers active real-URL navigation failure forwarding.e2e/tests/artifact-render-parity.test.tsfixes the retained-runtime parity report contract and dynamic-canvas classification.Validation
pnpm guardpnpm typecheck(all workspaces, after merging latestmain)PreviewSessionFrames.test.tsx+FileViewer.test.tsx)tools-pack, and launched the installed Electron app in isolated namespacepreview-runtimestyle.css+support.js: explicit Reload created and promoted a fresh iframe while reusing the exact scoped URL; CSS and JS re-executed; old frame was removed from DOM and the keep-alive poolrequestAnimationFramesamples over 2.2 s observed zero blank frames; desktop log contained noERR_ABORTED/did-fail-load