Repository navigation
feat(runtime): switch preview screens inside one document - #458
Merged
Sun-sunshine06 merged 8 commits intoOct 4, 2026
Merged
Conversation
Keep multi-page prototypes in the existing sandbox by showing one data-oc-screen target at a time.
Headless Chrome on macOS leaves Control+A and Meta+A unselected, so field replacement now selects the control before typing. Non-Linux test workers match the existing Windows limit, which keeps Chrome cleanup and the large text-layout case inside their timeouts.
github-actions
Bot
added
docs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Findings
- [Minor] Inactive screens are hidden only via the
hiddenattribute, which any authordisplayrule overrides.packages/runtime/src/overlay.ts:600(showScreen) togglesscreen.setAttribute('hidden', '')/removeAttribute('hidden'), and the convention is documented inpackages/core/src/prompts/sections/output-rules.md:10. The UA rule[hidden]{display:none}loses to author styles, so a screen container that setsdisplay:flex/grid(or an inlinestyle="display:flex") — a very common shape for page-level containers — stays visible after a switch. The diff shows no injected[data-oc-screen][hidden]{display:none}rule, so selecting#idcan silently leave every screen visible at once.
Suggested fix: hide with an explicit inline style (save/restorescreen.style.display) or inject[data-oc-screen][hidden]{display:none !important}into the sandbox shell; if the constraint is intentional, say so in the prompt line too, and add a test screen that setsdisplay:flexso it can't regress.
Questions
- Does the sandbox HTML shell already inject a higher-priority
[hidden]{display:none}rule? It is not visible in this diff; if it does, the finding above is moot.
Summary
- Review mode: initial
- The change is small, opt-in, and additive: a
data-oc-screenconvention plus#idlink handling in the preview overlay, a one-line output-prompt update, and new overlay unit tests. No new dependencies, no persistence/schema change, MIT-compatible.Refs #225is used correctly (does not claim to close the broad request). - Main risk: the
hidden-based hiding mechanism depends on artifact CSS not overridingdisplay, which is easy to violate and fails silently (no error, wrong layout). Everything else is minor. - Non-blocking scope note:
apps/desktop/vitest.config.tsnow runs macOS withmaxWorkers: 2(previously only Windows). It is unrelated to the feature but explained in the PR body as a pre-push stability fix; fine to keep, just note it may slow macOS CI.
Testing
- New unit coverage in
packages/runtime/src/overlay.test.tsforshowScreen/ensureScreensuses a hand-built fake DOM (screenDouble) that does not modelhiddenor CSS, so the real visibility behavior and the 200ms reattach path are not exercised in a browser. - Suggested: a browser/Playwright test that renders a two-screen artifact (with one screen styled
display:flex) and asserts only the targeted screen is visible after clicking<a href="#id">. Not run (automation).
Open-CoDesign Bot
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Screen state currently leaks onto the document root and fails to reconcile dynamic screens or keyboard focus.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Adds opt-in same-document screen switching for interactive previews, covering part of issue #225.
Changes:
- Implements
data-oc-screenswitching and initialization. - Documents and tests the screen convention.
- Improves cross-platform browser tests and macOS test stability.
| File | Description |
|---|---|
packages/runtime/src/overlay.ts |
Adds preview screen switching. |
packages/runtime/src/overlay.test.ts |
Tests switching and initialization. |
packages/core/src/prompts/sections/output-rules.md |
Documents the screen convention. |
apps/desktop/vitest.config.ts |
Limits macOS and Windows workers. |
apps/desktop/src/renderer/src/components/TweakPanel.browser.test.ts |
Uses native input selection. |
apps/desktop/src/renderer/src/components/SourceEditPanel.browser.test.ts |
Uses native textarea selection. |
apps/desktop/src/renderer/src/components/chat/PromptInput.browser.test.ts |
Uses native textarea selection. |
.changeset/preview-screen-links.md |
Records runtime and prompt changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+603
to
+604
| root.setAttribute('data-oc-screen', id); | ||
| var screens = document.querySelectorAll('[data-oc-screen]'); |
Comment on lines
+609
to
+610
| if (name === id) screen.removeAttribute('hidden'); | ||
| else screen.setAttribute('hidden', ''); |
Comment on lines
+619
to
+624
| for (var i = 0; i < screens.length; i++) { | ||
| var name = screens[i].id || screens[i].getAttribute('data-oc-screen'); | ||
| if (current && name === current) return; | ||
| } | ||
| var first = screens[0]; | ||
| showScreen(first.id || first.getAttribute('data-oc-screen')); |
Contributor
There was a problem hiding this comment.
Findings
No new blocking or material findings in the follow-up diff.
- [Resolved] Previous [Minor] finding (inactive screens hidden only via the
hiddenattribute, overridable by authordisplayrules) is addressed.ensureScreensnow injects:root[data-oc-screen] [data-oc-screen][hidden]{display:none!important;}into the preview document (packages/runtime/src/overlay.ts:619), and the new browser testapps/desktop/src/renderer/src/preview/screens.browser.test.ts:26renders a two-screen artifact where one screen isdisplay:gridand the otherdisplay:flex, then asserts only the targeted screen is visible across#home/#pricingswitching. This exactly matches the suggested fix from the prior review.
Summary
- Review mode: follow-up after new commits
- The follow-up commits resolve the single prior [Minor] finding: the
hidden-attribute override is now backed by an injected!importantrule scoped to:root[data-oc-screen], plus a real-browser regression test covering grid/flex screens, input-state preservation, later-added screens, and removal of the active screen (packages/runtime/src/overlay.ts:616-640). - The
ensureScreensrewrite also fixes a latent gap in the previous version: whencurrentmatched, the old code returned without re-applyingshowScreen, so a React re-render that dropped the overlay'shiddenattributes could leave every screen visible. The new code callsshowScreen(current)each pass, which self-heals on the existing reattach interval. Refs #225remains correctly used (partial slice, does not claim to close the broader request); changeset covers both touched packages; no new dependencies, no persistence/schema change, no release-path impact.
Residual observations (not findings):
ensureScreensre-enforces a single-visible-screen state on every reattach tick (packages/runtime/src/overlay.ts:648). Any artifact that intentionally shows more than one[data-oc-screen]element at once, or toggles their visibility through a mechanism other than#idlinks, would be overridden every ~200ms. Acceptable under the documented opt-in convention, but worth remembering if a future slice adds a comparison/multi-pane mode.- The injected
<style>is appended to<html>rather than<head>, and is created once (guarded byif (!screenStyle)); if it is ever removed from the DOM it will not be re-inserted. Low impact in current use. apps/desktop/vitest.config.ts:5-6now caps macOS workers at 2 as well as Windows; unrelated to the feature but explained in the PR body as a pre-push stability fix.
Testing
- New real-browser coverage in
apps/desktop/src/renderer/src/preview/screens.browser.test.tsexercises the productionbuildInteractivePreviewDocument+INTERACTIVE_PREVIEW_SANDBOXpath, not a mocked overlay. - Unit coverage in
packages/runtime/src/overlay.test.tsforshowScreen/ensureScreensstill uses a hand-built fake DOM (screenDouble); the visibility behavior it cannot model is now covered by the browser test. - Not run (automation): I did not execute the suite; the browser test is
skipIf(!chrome), so environments without system Chrome will skip it rather than fail.
Open-CoDesign Bot
Contributor
There was a problem hiding this comment.
Findings
No new blocking or material findings in the follow-up diff.
- [Resolved] The prior [Minor] finding (inactive screens being hidden only via the
hiddenattribute, so authordisplayrules could override it) stays fixed. After the root-marker rename the injected rule is still present and correctly scoped::root[data-oc-active-screen] [data-oc-screen][hidden]{display:none!important;}(packages/runtime/src/overlay.ts:639).
Summary
- Review mode: follow-up after new commits
- The follow-up renames the overlay's root marker from
data-oc-screentodata-oc-active-screen(packages/runtime/src/overlay.ts:603) and updates the injected CSS scope to match. This removes the naming collision where<html>carried the same attribute as the screen elements it is meant to scope; the injected rule now keys off a marker that only the overlay writes. - It adds focus restoration in
showScreen: when the currently focused element sits inside a screen being hidden, focus moves to the destination screen (temporarilytabindex="-1", removed onblur) viafocus({ preventScroll: true })(packages/runtime/src/overlay.ts:621-634). This satisfies the output rule that "screen transitions must not strand focus on removed content" (packages/core/src/prompts/sections/output-rules.md). - Author-provided
tabindexon a screen is preserved (the code only synthesises-1when none exists and only attaches the one-shotblurcleanup in that case). Refs #225is still the correct link (partial slice, no closure claim), the changeset covers both touched packages, and there are no new dependencies, persistence/schema changes, or release-path impact.
Residual observations (not findings):
ensureScreenscontinues to re-assert a single visible screen on every reattach tick (packages/runtime/src/overlay.ts:648). Intentional under the documented opt-in convention, but any future multi-pane/comparison mode that shows more than one[data-oc-screen]element would be overridden ~every 200ms.- The injected
<style>is appended to<html>and created once behindif (!screenStyle)(packages/runtime/src/overlay.ts:636-640); if it is ever removed from the DOM it will not be re-inserted. Low impact in current use.
Testing
- The new real-browser test exercises the production
buildInteractivePreviewDocument+INTERACTIVE_PREVIEW_SANDBOXpath and now covers the follow-up behavior: focus moving to the newly active screen, transienttabindex="-1"being removed onblur, an author-settabindexbeing preserved, grid/flex screens, input-state preservation, later-added screens, and removal of the active screen (apps/desktop/src/renderer/src/preview/screens.browser.test.ts). - Unit coverage in
packages/runtime/src/overlay.test.tswas updated for the renamed root attribute. The focus branch itself is only exercised by the browser test (the hand-builtscreenDoublefake has nocontains/activeElement), which is acceptable since the browser test asserts the real behavior. - Not run (automation): I did not execute the suite; the browser tests are
skipIf(!chrome), so environments without system Chrome skip rather than fail.
Open-CoDesign Bot
5 of 11 tasks
Sun-sunshine06
pushed a commit
that referenced
this pull request
Oct 4, 2026
## Summary The sidebar now shows recorded token and USD totals for the current design: the design lifetime, local today, and the Monday-start local week. This is the weekly slice of the README Later item "Cost transparency". There is no separate issue. Totals come from usage already stored on settled run-journal responses. This does not add a price table, a pre-generation estimate, a budget cap, or a new database. Cancelled runs that include a provider response are counted. Failed runs without a response are not. Invalid usage numbers are treated as zero. The sidebar refetches those journal totals when a run settles. A cancellation that records a provider response updates the totals even though `lastUsage` does not change. The same design also refetches at the next local midnight, so Today and the Monday-start week move while it stays open, and again when the window is focused or shown. A response that arrives after cleanup, or after a newer read, is ignored. The pure totals, formatting, and local-midnight helper live in `@open-codesign/shared`. Desktop main and the renderer both import that package. The renderer no longer imports `src/main`, and the web TypeScript project no longer includes a main-process file. An earlier commit on this branch changed the macOS browser test harness. That change is reverted in `e2822c9` and is not part of this PR. The same harness fix remains local on this machine and on unmerged #458. ## Type of change - [x] New feature - [ ] Bug fix - [ ] Refactor (no behavior change) - [ ] Documentation - [ ] Build / CI / tooling - [ ] Breaking change ## Linked issue No linked issue. README Later item: Cost transparency, weekly budget slice only. ## Checklist - [x] I checked the linked issue / relevant context before starting - [x] `pnpm lint && pnpm typecheck && pnpm test` passes locally - [x] Added/updated tests for the change - [x] Added a changeset (`pnpm changeset`) if user-visible - [ ] Updated docs if behavior changed ## Dependency additions (if any) None. ## Screenshots / recordings (UI changes) Not attached. The new text sits under the existing per-run token line in the sidebar. ## Checks `be0c0b7` passed local pre-push: typecheck, Biome (674 files), and the full test suite. Desktop: 175 files, 2583 tests passed. Shared: 20 files, 313 tests passed. The net diff against main remains the usage change only.
Contributor
There was a problem hiding this comment.
Findings
No new blocking, major, or minor findings.
Summary
- Review mode: follow-up after new commits
- The PR's own diff is unchanged from the review at
38d8b7cf:packages/runtime/src/overlay.ts(same-documentdata-oc-screenswitching, scopeddisplay:none!importantrule, focus transfer),packages/runtime/src/overlay.test.ts,packages/core/src/prompts/sections/output-rules.md:10, the newapps/desktop/src/renderer/src/preview/screens.browser.test.ts, and.changeset/preview-screen-links.md. - The commits between
38d8b7cfand8b228b7cin the compare are main-line sync (image-generation base64 response setting,ImageGenerationTab, browser-test Select All helpers). Those files are not part of this PR's diff, so they are out of scope for this review. - Previously raised items remain resolved: the root marker is
data-oc-active-screenand is never written onto screen elements, the injected rule is scoped to:root[data-oc-active-screen], and focus moves to the destination screen via a transienttabindex="-1"removed onblur(packages/runtime/src/overlay.ts:600-655). Refs #225is still the correct link (partial slice, no closure claim); the changeset covers both touched packages; no new dependencies, persistence/schema changes, or release-path impact.
Residual observations (not findings):
ensureScreens()runs from every ~200msreattach()tick and re-asserts exactly one visible screen (packages/runtime/src/overlay.ts:669). Intentional under the documented opt-in convention, but a future multi-pane/comparison view that shows two[data-oc-screen]elements at once would be overridden.- The injected
<style>is created once behindif (!screenStyle)and appended to<html>(packages/runtime/src/overlay.ts:637-642); if it is ever removed from the DOM it is not re-inserted. Low impact in current use.
Testing
- The PR ships both a unit harness update (
packages/runtime/src/overlay.test.ts) and a real system-Chrome browser test (apps/desktop/src/renderer/src/preview/screens.browser.test.ts) exercising the productionbuildInteractivePreviewDocument+INTERACTIVE_PREVIEW_SANDBOXpath. - Not run (automation): I did not execute the suite; the browser tests are
skipIf(!chrome), so environments without system Chrome skip rather than fail.
Open-CoDesign Bot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add an opt-in screen-switching convention to the existing preview runtime: matching
idanddata-oc-screenattributes identify screens, and<a href="#id">selects one screen without navigating the sandbox document. Shared navigation stays outside the screen containers. Inactive screens usehiddenwith a scoped important display rule, preserving their authored grid/flex layout when shown again. Active state uses a separate root attribute so authored screen selectors only match actual containers. Switching from a focused in-screen link transfers focus to the destination; shared navigation keeps focus. Ordinary hash anchors retain their existing scroll behavior.The core output prompt describes this convention. This is a small slice of interactive prototyping, with no new router, dependencies, persistence schema, or desktop layout changes. It does not implement UI-to-component extraction or a separate page-flow editor.
The branch has been synchronized with main. Conflict resolution preserves the existing main-branch browser keyboard helpers and Vitest configuration; this PR no longer changes those test harness files.
Type of change
Linked issue
Refs #225. This PR implements same-document screen switching and does not close the broader request.
Validation
Updated revision
8b228b7passed the complete GitHub CI run: lint, typecheck, all test tasks, and the Electron build smoke. All 177 desktop test files passed, including the real-browser screen-switching regression. CodeQL and the latest automated review also passed.Current revision: workspace typechecking and Biome lint pass locally.
Runtime: 200 tests pass. The new real-system-Chrome screen-switching regression also passes in isolation, covering grid/flex visibility, state preservation, dynamic screens, and keyboard focus.
The Windows full-suite run passed 2,592 desktop tests but failed 6 tests plus one teardown hook, including Chromium operation/cleanup timeouts and a timing-sensitive color-input assertion. The color-input test passed when rerun alone; related existing TweakPanel timeouts also reproduced without this PR.
The local pre-push hook blocked the first push. A single-worker retry stalled in Chromium cleanup and was stopped. The conflict-resolution commit was subsequently pushed with the local hook disabled so the complete updated CI can run. Local full-suite validation is not claimed as passing.
git diff --checkpasses. No new dependencies or persistence changes.Checklist
pnpm changeset) if user-visibleScreenshots / recordings
Preview screenshots have been captured locally but are not attached yet. The file picker did not enable upload; no attachment URL has been produced. The interaction is opt-in through screen attributes, so existing single-screen artifacts keep their current behavior.