spike: isolate the Play runtime inside a shadow root - #35
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughPlay now mounts the application inside an open shadow root, injects runtime styles there, retargets authored stylesheet selectors, moves Play-specific CSS into runtime styles, and propagates selected frame state to the app host. ChangesPlay shadow-root runtime
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PlayBoot
participant retargetApplicationStyles
participant applicationStyle
participant runtimeRoot
participant appHost
PlayBoot->>retargetApplicationStyles: transform fetched application CSS
retargetApplicationStyles-->>PlayBoot: return retargeted CSS
PlayBoot->>applicationStyle: assign transformed CSS
PlayBoot->>runtimeRoot: mount application stylesheet
appHost->>appHost: update data-frame on frame selection
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d89238d to
644a538
Compare
The app's page and surface hosts, the primitive base styles, and the authored application stylesheet now live inside a shadow root on #uh-app (#30): shell chrome and machine-owned trees cannot collide, and surface layering has a defined stacking root owned by the runtime. Shell chrome styles stay document-level; the application stylesheet is appended after the runtime bases inside the shadow so it still wins the cascade. All 283 web tests stay green, and the instagram comments sheet renders contained inside the prototype frame against a live spock start.
Authored stylesheets legitimately use :root, html, body, and #uh-app; inside the shadow those match nothing and the app's design tokens silently die (found on the desktop frame). Selector heads are rewritten to :host at injection, with class-name lookalikes (.post-body) proven untouched.
…adow Three follow-up holes found on the desktop frame and closed: comments before a selector shielded it from retargeting (the token :root block silently died), frame-keyed rules (#uh-frame[data-frame]) could not see across the boundary (the frame state is now mirrored onto the host and those heads rewrite to :host(#uh-app[...])), and the rewrites preserve id-level specificity so mobile-first base rules do not win ties they used to lose. Desktop now renders identically to pre-spike main; covered by three retargeting unit tests.
644a538 to
3aed9bb
Compare
| * the app's design tokens and base rules silently die (#30 spike). | ||
| */ | ||
| export function retargetApplicationStyles(styleText: string): string { | ||
| // 주석은 셀렉터 앵커를 가리므로 런타임 사본에서는 제거한다. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/play/tests/chrome.test.ts (1)
204-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the app-host frame contract.
appHostis not returned andframeButtonsis empty, so removing the production assignment would still pass. ExposeappHostand assertdataset.framefor initial selection and a frame change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/play/tests/chrome.test.ts` at line 204, Update the test setup and assertions around appHost and frameButtons to return appHost, then assert that appHost.dataset.frame reflects the initially selected frame and updates after a frame change. Ensure the test would fail if the production app-host frame assignment were removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/play/shell.ts`:
- Around line 230-250: Update retargetApplicationStyles in web/src/play/shell.ts
to collapse selectors combining document ancestry (:root, html, or body) or
`#uh-frame` with `#uh-app` into a single :host(`#uh-app`) selector, preserving any
attributes and remaining descendants. Extend regression coverage in
web/src/play/tests/shell.test.ts for these compound patterns, including body
`#uh-app` and :root `#uh-app`, and verify they do not retain `#uh-app` as a shadow
descendant.
---
Nitpick comments:
In `@web/src/play/tests/chrome.test.ts`:
- Line 204: Update the test setup and assertions around appHost and frameButtons
to return appHost, then assert that appHost.dataset.frame reflects the initially
selected frame and updates after a frame change. Ensure the test would fail if
the production app-host frame assignment were removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe9851f7-9faa-4be1-855a-4875da654c2f
📒 Files selected for processing (8)
web/src/play/chrome.tsweb/src/play/main.tsweb/src/play/mount.tsweb/src/play/runtime.cssweb/src/play/shell.cssweb/src/play/shell.tsweb/src/play/tests/chrome.test.tsweb/src/play/tests/shell.test.ts
💤 Files with no reviewable changes (1)
- web/src/play/shell.css
2 participants
Spike for #30's shell/runtime separation (the shadow-root option).
The app's page and surface hosts, the primitive base styles, and the authored application stylesheet now live inside a shadow root on
#uh-app. Shell chrome stays document-level; the application stylesheet is appended after the runtime bases inside the shadow, so it still wins the cascade the shell comments promise.What this buys: the machine-owned trees and the shell cannot collide in either direction, and surface layering gets a defined stacking root owned by the runtime — the containment half of #30. The placement/backdrop/focus contract itself stays open as the spec half.
Verification: all 283 web tests green; against a live
spock start, the instagram feed renders identically and the comments sheet opens contained inside the prototype frame (screenshots below). Four regressions found and fixed during the spike, all of the same family — authored styles that legitimately assume the document: (1) the stylesheet initially stayed document-level and never reached the shadowed tree; (2):root/body/#uh-appselector heads matched nothing inside the shadow, so the design-token block silently died (found on the desktop frame) — they now retarget to:host; (3) frame-keyed rules (#uh-frame[data-frame=…]) cannot see across the boundary — the frame state is mirrored onto the host and those heads rewrite to:host(#uh-app[…]), preserving id-level specificity so mobile-first base rules do not win ties they used to lose; (4) comments before a selector shielded it from retargeting. Desktop now renders identically to pre-spikemain; the retargeting is covered by unit tests.Feed unchanged (no regression):
Comments sheet contained inside the prototype frame:
