Repository navigation
fix: make a deployed Ultimate app render, and stop answering a browser with JSON - #52
Conversation
…r with JSON Four defects, none visible from a test suite, all found by opening the deployed demo app in a real browser. **The CSP blocked the framework's own stylesheet.** `style-src 'self'` against a 64,090-character inline `<style>`: `sheet` null, **zero rules parsed**, every Ultimate app unstyled in production since the header shipped. It survived because `defineHttpConfig` sets `csp.reportOnly: dev` — `x dev` reports the violation to the console and paints the page anyway, so only production enforced it. The bodies are known at boot, so `style-src` now carries a sha256 per emitted `<style>`, computed from the real text in `startWeb` — one place, covering every caller of `startRoles`, so a future entry point cannot forget it. `buildCsp` loses its `nonce` parameter: nothing ever passed one, and a second inline-coverage path that never works is what axiom 1 forbids. `style-src-attr 'unsafe-inline'` is a deliberate, attribute-scoped relaxation — 14 components pass *computed* custom properties (`--avatar-size: 42px`), which no hash can express; `<style>` elements still fall back to the hashed `style-src`. **`packages/ui/src/global.scss` — "one import for an app document shell" — was imported by nothing, anywhere.** No `:root` block reached any document, so every `var(--color-*)` and `var(--space-*)` resolved to nothing. `stylesFor` also dropped `surface === 'shared'` entirely, and ordered by insertion rather than globals-first. The global layer now arrives through the app's own module graph (`shared/global.scss` + a one-line `shared/global.ts`, both scaffolded by `x new`) because render is tier 4 and ui is tier 5. `X_STYLES_GLOBAL_MISSING` fails the `budgets` step when a rendered document carries CSS and defines no `:root` custom properties. **The `<head>` had no charset, viewport or `color-scheme`.** Without a viewport a phone lays the page out at ~980px and scales it down, so every deployed app rendered zoomed-out whatever its CSS said. `documentBaseline()` merges first, so a route can still override any of the three. **A browser that failed `auth: 'required'` got problem+json rendered as raw text.** It now gets `303 → /signin?next=…`; an agent asking for JSON still gets the document. Off until an app names its page — guessing `/signin` for an app that says `/login` is a 404, which is worse than the JSON. `nextAfterSignIn` is the one reader of `?next=` and refuses anything that is not a same-origin path, because the page that hands out a session is the last place an open redirect belongs. `createSession`/`createAccount` redirect a browser too; the native form POST used to answer one with a JSON blob. Measured on the demo app: 0 → **585 CSS rules parsed**, tokens resolving, 0 CSP console errors. The landing page was still the generator's placeholder pointing at a route that 401s — its old test passed the whole time, because the placeholder had a title too. `bun run verify`: all 17 green. Reference-app ratchet: 10 of 17, 7 pinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds app-owned global stylesheet handling, document-style verification, browser authentication redirects, and hashed inline-style CSP support. It also updates the social-media example landing page, scaffolding, documentation, and related tests. ChangesGlobal stylesheet pipeline
Authentication redirect flow
Inline-style CSP coverage
Social-media example landing page
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 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 `@dummy/social-media-clone/apps/web/shared/global.scss`:
- Line 5: Remove the empty // separator at
dummy/social-media-clone/apps/web/shared/global.scss lines 5-5,
dummy/social-media-clone/apps/web/shared/tokens.scss lines 5-5, and
examples/dummy/apps/web/shared/global.scss lines 5-5; no other stylesheet
changes are needed.
In `@dummy/social-media-clone/apps/web/site/page.module.scss`:
- Line 56: Update the proof-card grid’s grid-template-columns declaration to cap
the minimum track at the available container width while preserving the existing
18rem preferred minimum on wider viewports, so cards can shrink without
overflowing the padded .page container.
In `@dummy/social-media-clone/apps/web/site/page.tsx`:
- Around line 2-3: Move the existing deployment-description comment in the
landing page module before all imports and rewrite it as a 1–4 line header
stating the page’s single responsibility and why it remains static. Keep the
explanation focused on the seeded framework stress-test deployment rather than
implementation details.
In `@dummy/social-media-clone/apps/web/site/signin/page.tsx`:
- Line 41: Replace the hardcoded /dashboard fallback in nextAfterSignIn within
dummy/social-media-clone/apps/web/site/signin/page.tsx:41-41 with
config.auth.afterSignInPath or a route prop carrying that configured value.
Apply the same change to the fallback in
dummy/social-media-clone/apps/web/site/signup/page.tsx:32-32 so both flows use
the centralized afterSignInPath configuration.
In `@dummy/social-media-clone/packages/i18n/catalogs/en.json`:
- Around line 210-233: Shorten the site.home.description value in
dummy/social-media-clone/packages/i18n/catalogs/en.json to 50–160 characters. In
dummy/social-media-clone/apps/web/site/page.test.ts lines 23-29, add an
assertion that page.config.meta’s description is within the same inclusive
range.
In `@packages/cli/src/app-auth.ts`:
- Around line 5-6: Add file-header comments in packages/cli/src/app-auth.ts near
the node:fs and node:path imports explaining why these Node APIs are
unavoidable. Add the required file header in packages/cli/src/app-auth.test.ts
near its Node temporary-directory imports, documenting why those APIs are
necessary; no other changes are required.
- Line 31: Update the redirect-target validation around the visible path check
to reject network-path and backslash-based cross-origin targets such as
//evil.test and /\evil.test. Parse candidate redirect values against the request
origin and accept only same-origin paths, preserving valid local absolute paths;
add regression coverage for both malicious cases.
In `@packages/cli/src/cmd-dev.test.ts`:
- Line 67: Replace the raw colour values in the stylesheet fixtures with the
established semantic colour-token pattern: update
packages/cli/src/cmd-dev.test.ts lines 67-67, changing `#123456`, and
packages/cli/src/dev-roles.test.ts lines 296-296, changing red, using the same
framework token in both fixtures.
In `@packages/cli/src/dev-roles.ts`:
- Around line 150-156: Extract the WebBinding definition and startWeb()
implementation from dev-roles.ts into a focused web-role module, including the
related web-server and CSP configuration. Keep startRoles() in dev-roles.ts as
the role lifecycle coordinator, updating imports and call sites to use the
extracted symbols while preserving existing behavior.
In `@packages/cli/src/document-styles.ts`:
- Around line 1-5: Shorten the header in packages/cli/src/document-styles.ts
(lines 1-5) to no more than four lines while stating the file’s single
responsibility. Add a one-to-four-line header before imports in
packages/cli/src/document-styles.test.ts (lines 1-2) explaining that the tests
protect the document-style gate.
In `@packages/cli/src/templates/scaffold-app.ts`:
- Around line 405-406: Update appFiles() to emit an adjacent
apps/web/shared/global.test.ts alongside global.ts, using the generated
global-style entry to verify that importing it registers root custom properties
on a rendered surface. Ensure the generated test passes and fails if that
side-effect registration is removed.
In `@packages/http/src/auth-redirect.test.ts`:
- Line 1: Add a 1–4 line header comment before the imports in the auth redirect
test module, explaining that it protects authentication redirect behavior and
why that behavior must remain reliable. Keep the comment focused on the module’s
single responsibility and do not describe implementation details.
In `@packages/http/src/auth-redirect.ts`:
- Around line 52-57: Update nextAfterSignIn to catch URIError from
decodeURIComponent(raw) and return fallback for malformed encoded return paths,
while preserving the existing validation for valid values. Add a regression test
covering "%" and asserting that fallback is returned.
In `@packages/http/src/overlay-style.ts`:
- Around line 8-16: Replace the raw hexadecimal palette declarations in
packages/http/src/overlay-style.ts lines 8-16 with approved semantic colour
token references while keeping the overlay standalone. In
packages/http/src/security-headers.test.ts lines 61 and 91, replace each
color:red fixture with non-colour CSS declarations. In
packages/cli/src/style-csp.test.ts lines 19-20, 31, and 45, replace each
named-colour fixture declaration with non-colour CSS fixtures.
In `@packages/render/CLAUDE.md`:
- Line 30: Correct the malformed baseline date wording in the `<head>` baseline
documentation, replacing “Absent until As of 2026-08” with a clear phrase such
as “Introduced in August 2026” or “Absent before August 2026,” while preserving
the surrounding behavior description.
In `@packages/render/src/head.test.ts`:
- Around line 218-227: Strengthen the tests around documentBaseline() and
headFromMeta(): assert exact charset, viewport, and color-scheme attributes
rather than only keys, and provide one route/SEO tag through renderMeta so the
test compares its position against the baseline tags. Preserve
documentBaseline() merging first while confirming route tags remain
overrideable.
In `@packages/render/src/module-loader.test.ts`:
- Line 16: Replace the raw color values in the test CSS constants and fixtures
around GLOBAL_CSS and the cases at lines 106, 116, and 126 with non-color
declarations plus a neutral custom-property sentinel. Preserve the existing
stylesheet ordering and deduplication assertions while removing all literal
color values from these tests.
In `@packages/ui/README.md`:
- Around line 161-165: Update the CSP example around brandStyleCspSource(brand)
to include an explicit style-src-attr directive alongside style-src, allowing
inline style attributes only as intended while preserving the existing
stylesheet source restriction.
🪄 Autofix
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: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b760f680-f74a-4594-b2e6-ea76e98733ce
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!**/bun.lock
📒 Files selected for processing (74)
docs/architecture/16-build-pipeline.mddummy/social-media-clone/app.config.tsdummy/social-media-clone/apps/web/api/auth.tsdummy/social-media-clone/apps/web/shared/global.scssdummy/social-media-clone/apps/web/shared/global.test.tsdummy/social-media-clone/apps/web/shared/global.tsdummy/social-media-clone/apps/web/shared/tokens.scssdummy/social-media-clone/apps/web/site/page.module.scssdummy/social-media-clone/apps/web/site/page.test.tsdummy/social-media-clone/apps/web/site/page.tsxdummy/social-media-clone/apps/web/site/signin/page.tsxdummy/social-media-clone/apps/web/site/signup/page.tsxdummy/social-media-clone/package.jsondummy/social-media-clone/packages/i18n/catalogs/en.jsondummy/social-media-clone/types/scss.d.tsexamples/dummy/CLAUDE.mdexamples/dummy/README.mdexamples/dummy/apps/web/shared/global.scssexamples/dummy/apps/web/shared/global.test.tsexamples/dummy/apps/web/shared/global.tsexamples/dummy/apps/web/shared/theme.scssexamples/dummy/apps/web/tsconfig.jsonexamples/dummy/tsconfig.jsonexamples/dummy/types/scss.d.tsframework.manifest.jsonpackages/admin/src/dev/index.tspackages/admin/src/dev/server.test.tspackages/admin/src/dev/server.tspackages/cli/CLAUDE.mdpackages/cli/src/app-auth.test.tspackages/cli/src/app-auth.tspackages/cli/src/cmd-dev.test.tspackages/cli/src/cmd-dev.tspackages/cli/src/cmd-verify.tspackages/cli/src/dev-roles.test.tspackages/cli/src/dev-roles.tspackages/cli/src/document-styles.test.tspackages/cli/src/document-styles.tspackages/cli/src/serve.tspackages/cli/src/style-csp.test.tspackages/cli/src/style-csp.tspackages/cli/src/templates/scaffold-app.tspackages/cli/src/templates/scaffold-repo.tspackages/core/src/config.tspackages/core/src/index.tspackages/http/CLAUDE.mdpackages/http/src/auth-redirect.test.tspackages/http/src/auth-redirect.tspackages/http/src/config.tspackages/http/src/index.tspackages/http/src/overlay-style.tspackages/http/src/overlay.test.tspackages/http/src/overlay.tspackages/http/src/pipeline.test.tspackages/http/src/pipeline.tspackages/http/src/security-headers.test.tspackages/http/src/security-headers.tspackages/render/CLAUDE.mdpackages/render/src/css-modules.test.tspackages/render/src/css-modules.tspackages/render/src/errors.tspackages/render/src/head-seo.test.tspackages/render/src/head.test.tspackages/render/src/head.tspackages/render/src/index.tspackages/render/src/module-loader.test.tspackages/render/src/module-loader.tspackages/ui/README.mdpackages/ui/src/global.scsspackages/ui/src/index.tspackages/ui/src/theme/brand.test.tspackages/ui/src/theme/brand.tswiki/CLI-Reference.mdwiki/Error-Codes.md
💤 Files with no reviewable changes (1)
- examples/dummy/apps/web/shared/theme.scss
| // @ultimat3/ui's custom properties (`:root{--color-*;--space-*;…}`) and then its reset. Every rule | ||
| // a component module emits reads those properties through `var(--…)`, so without this file the | ||
| // browser drops every one of those declarations and the app renders unstyled. | ||
| // |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove empty // comments.
Stylelint reports scss/comment-no-empty at every listed site. Remove the empty separator so stylesheet lint can pass.
dummy/social-media-clone/apps/web/shared/global.scss#L5-L5: Remove the empty//separator.dummy/social-media-clone/apps/web/shared/tokens.scss#L5-L5: Remove the empty//separator.examples/dummy/apps/web/shared/global.scss#L5-L5: Remove the empty//separator.
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 5-5: Unexpected empty comment (scss/comment-no-empty)
(scss/comment-no-empty)
📍 Affects 3 files
dummy/social-media-clone/apps/web/shared/global.scss#L5-L5(this comment)dummy/social-media-clone/apps/web/shared/tokens.scss#L5-L5examples/dummy/apps/web/shared/global.scss#L5-L5
🤖 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 `@dummy/social-media-clone/apps/web/shared/global.scss` at line 5, Remove the
empty // separator at dummy/social-media-clone/apps/web/shared/global.scss lines
5-5, dummy/social-media-clone/apps/web/shared/tokens.scss lines 5-5, and
examples/dummy/apps/web/shared/global.scss lines 5-5; no other stylesheet
changes are needed.
Source: Linters/SAST tools
|
|
||
| .list { | ||
| display: grid; | ||
| grid-template-columns: repeat(auto-fit, minmax(18rem, 1fr)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Allow proof cards to shrink on narrow viewports.
minmax(18rem, 1fr) can overflow after .page applies horizontal padding. Cap the minimum track at the container width.
Proposed fix
- grid-template-columns: repeat(auto-fit, minmax(18rem, 1fr));
+ grid-template-columns: repeat(auto-fit, minmax(min(18rem, 100%), 1fr));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| grid-template-columns: repeat(auto-fit, minmax(18rem, 1fr)); | |
| grid-template-columns: repeat(auto-fit, minmax(min(18rem, 100%), 1fr)); |
🤖 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 `@dummy/social-media-clone/apps/web/site/page.module.scss` at line 56, Update
the proof-card grid’s grid-template-columns declaration to cap the minimum track
at the available container width while preserving the existing 18rem preferred
minimum on wider viewports, so cards can shrink without overflowing the padded
.page container.
| // It says what this deployment IS — a framework stress test with seeded data — because an | ||
| // unlabelled demo reads as a real network, and the first thing anyone does is try to sign up. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Make this a module header.
Move the header before imports. State the landing page's single responsibility and why it stays static. These lines describe deployment content instead.
As per coding guidelines: “Add a 1–4 line header comment to each file stating its single responsibility.”
🤖 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 `@dummy/social-media-clone/apps/web/site/page.tsx` around lines 2 - 3, Move the
existing deployment-description comment in the landing page module before all
imports and rewrite it as a 1–4 line header stating the page’s single
responsibility and why it remains static. Keep the explanation focused on the
seeded framework stress-test deployment rather than implementation details.
Source: Coding guidelines
| // Where the pipeline said this visitor was going before it bounced them here. `nextAfterSignIn` | ||
| // refuses anything that is not a same-origin path — `?next=` is off the URL bar, and an | ||
| // unchecked value makes the one page that hands out a session an open redirect. | ||
| const next = nextAfterSignIn(props.query[NEXT_PARAM], '/dashboard'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use config.auth.afterSignInPath as the fallback.
Changing afterSignInPath now leaves these native forms posting /dashboard. The action and page flows can then disagree.
dummy/social-media-clone/apps/web/site/signin/page.tsx#L41-L41: replace the literal fallback withconfig.auth.afterSignInPath, or pass that configured value as a route prop.dummy/social-media-clone/apps/web/site/signup/page.tsx#L32-L32: replace the literal fallback withconfig.auth.afterSignInPath, or pass that configured value as a route prop.
As per path instructions, “Define once, project everywhere — a fact stated in two places will drift; generate it.”
📍 Affects 2 files
dummy/social-media-clone/apps/web/site/signin/page.tsx#L41-L41(this comment)dummy/social-media-clone/apps/web/site/signup/page.tsx#L32-L32
🤖 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 `@dummy/social-media-clone/apps/web/site/signin/page.tsx` at line 41, Replace
the hardcoded /dashboard fallback in nextAfterSignIn within
dummy/social-media-clone/apps/web/site/signin/page.tsx:41-41 with
config.auth.afterSignInPath or a route prop carrying that configured value.
Apply the same change to the fallback in
dummy/social-media-clone/apps/web/site/signup/page.tsx:32-32 so both flows use
the centralized afterSignInPath configuration.
Source: Path instructions
| "title": "A social network, built to break a framework", | ||
| "description": "Every Ultimate primitive, once, in one real app: relational visibility decided by a policy, a live feed, offline reads, uploads and an admin that shares the same authz object as the API.", | ||
| "cta": "Read the public feed", | ||
| "signin": "Sign in", | ||
| "seeded": "Seeded demo data. Every account, post and photo here is fabricated.", | ||
| "proofs": { | ||
| "title": "What this deployment is evidence for", | ||
| "policy": { | ||
| "title": "One authz object, four surfaces", | ||
| "body": "Who may see a post is friendship and blocks, not a tenant column — and the HTTP route, the live query, the MCP tool and the admin button all ask the same policy." | ||
| }, | ||
| "realtime": { | ||
| "title": "Live queries, not polling", | ||
| "body": "A read declared live: true becomes a subscription, with the row gate re-decided per subscriber." | ||
| }, | ||
| "offline": { | ||
| "title": "Offline is a route declaration", | ||
| "body": "Each route says precache, runtime or network-only, and the service worker is generated from the route table." | ||
| }, | ||
| "admin": { | ||
| "title": "Admin is view-only by permission", | ||
| "body": "It holds admin:read and never admin:write. One decision renders the button and answers the call." | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the site metadata description within 50–160 characters.
site.home.description is 186 characters, but page.config.meta emits it as the route description.
dummy/social-media-clone/packages/i18n/catalogs/en.json#L210-L233: Shortensite.home.descriptionto 160 characters or fewer.dummy/social-media-clone/apps/web/site/page.test.ts#L23-L29: Assert thatmeta.descriptionis 50–160 characters.
Based on learnings: site/ routes must provide a 50–160 character meta.description.
📍 Affects 2 files
dummy/social-media-clone/packages/i18n/catalogs/en.json#L210-L233(this comment)dummy/social-media-clone/apps/web/site/page.test.ts#L23-L29
🤖 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 `@dummy/social-media-clone/packages/i18n/catalogs/en.json` around lines 210 -
233, Shorten the site.home.description value in
dummy/social-media-clone/packages/i18n/catalogs/en.json to 50–160 characters. In
dummy/social-media-clone/apps/web/site/page.test.ts lines 23-29, add an
assertion that page.config.meta’s description is within the same inclusive
range.
Source: Learnings
| --x-bg: #fbfbfd; --x-surface: #ffffff; --x-border: #e3e3ea; | ||
| --x-text: #1b1b1f; --x-muted: #6b6b76; --x-danger: #b3261e; --x-accent: #2f4fd8; | ||
| --x-code-bg: #f3f3f7; | ||
| } | ||
| @media (prefers-color-scheme: dark) { | ||
| :root { | ||
| --x-bg: #111115; --x-surface: #1a1a20; --x-border: #2e2e38; | ||
| --x-text: #ececf1; --x-muted: #9b9baa; --x-danger: #ff8a80; --x-accent: #9db2ff; | ||
| --x-code-bg: #22222b; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace raw colours with semantic tokens.
Raw hexadecimal and named colours violate the repository colour contract. Keep the overlay standalone, but provide its palette through approved semantic tokens. Use non-colour CSS declarations in hash fixtures.
packages/http/src/overlay-style.ts#L8-L16: Replace hexadecimal palette values with semantic token references.packages/http/src/security-headers.test.ts#L61-L61: Replacecolor:redwith a non-colour CSS fixture.packages/http/src/security-headers.test.ts#L91-L91: Replace bothcolor:redfixtures with non-colour CSS fixtures.packages/cli/src/style-csp.test.ts#L19-L20: Replace named-colour fixture declarations.packages/cli/src/style-csp.test.ts#L31-L31: Replace the named-colour fixture declaration.packages/cli/src/style-csp.test.ts#L45-L45: Replace the named-colour fixture declaration.
As per coding guidelines, “Do not use raw colours; use the framework's colour tokens instead.” As per path instructions, “A raw hex, rgb(), or named colour is blocking.”
📍 Affects 3 files
packages/http/src/overlay-style.ts#L8-L16(this comment)packages/http/src/security-headers.test.ts#L61-L61packages/http/src/security-headers.test.ts#L91-L91packages/cli/src/style-csp.test.ts#L19-L20packages/cli/src/style-csp.test.ts#L31-L31packages/cli/src/style-csp.test.ts#L45-L45
🤖 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 `@packages/http/src/overlay-style.ts` around lines 8 - 16, Replace the raw
hexadecimal palette declarations in packages/http/src/overlay-style.ts lines
8-16 with approved semantic colour token references while keeping the overlay
standalone. In packages/http/src/security-headers.test.ts lines 61 and 91,
replace each color:red fixture with non-colour CSS declarations. In
packages/cli/src/style-csp.test.ts lines 19-20, 31, and 45, replace each
named-colour fixture declaration with non-colour CSS fixtures.
Sources: Coding guidelines, Path instructions
| | Responses | return `RenderResult`. `@ultimat3/http` builds the `Response`. | | ||
| | Solid | no `solid-js` import anywhere in this package. Inject primitives. The JSX factory in `jsx.ts` builds inert nodes — it is not a Solid renderer and must never become one. | | ||
| | The loaders | `module-loader.ts` installs them at `index.ts` module scope, once. A plugin only affects modules loaded after it, so a second install point is a page that renders in one entry point and not another. | | ||
| | `<head>` baseline | `documentBaseline()` in `head.ts` — charset, viewport, `color-scheme` — merged FIRST so a route can still override any of them. Absent until `As of 2026-08`, and the missing `viewport` is why every deployed app rendered zoomed-out on a phone whatever its CSS said. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the malformed baseline date.
Line 30 says Absent until \As of 2026-08`. This is not a valid date phrase. Use Introduced in August 2026orAbsent before August 2026`.
🤖 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 `@packages/render/CLAUDE.md` at line 30, Correct the malformed baseline date
wording in the `<head>` baseline documentation, replacing “Absent until As of
2026-08” with a clear phrase such as “Introduced in August 2026” or “Absent
before August 2026,” while preserving the surrounding behavior description.
| test('every document carries charset, viewport and color-scheme', () => { | ||
| const keys = documentBaseline().map((tag) => tag.key); | ||
| expect(keys).toEqual(['meta:charset', 'meta:viewport', 'meta:color-scheme']); | ||
| }); | ||
|
|
||
| test('they arrive through headFromMeta, ahead of the seo tags', () => { | ||
| const html = renderHead(headFromMeta({ title: 'T' } as never, { renderMeta: () => [] })); | ||
| expect(html).toContain('<meta charset="utf-8">'); | ||
| expect(html).toContain('<meta name="viewport" content="width=device-width, initial-scale=1">'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the baseline test fail on wrong order or metadata.
Line 223 passes an empty renderMeta, so the test cannot verify that baseline tags precede SEO tags. Lines 218-220 also check only keys. Incorrect attribute values would pass.
Add one route tag, compare tag positions, and assert the exact charset, viewport, and color-scheme attributes.
As per path instructions, packages/render/CLAUDE.md requires documentBaseline() to merge FIRST while route tags remain overrideable.
Proposed test hardening
- const keys = documentBaseline().map((tag) => tag.key);
- expect(keys).toEqual(['meta:charset', 'meta:viewport', 'meta:color-scheme']);
+ expect(documentBaseline()).toEqual([
+ { kind: 'meta', key: 'meta:charset', attrs: { charset: 'utf-8' } },
+ {
+ kind: 'meta',
+ key: 'meta:viewport',
+ attrs: { name: 'viewport', content: 'width=device-width, initial-scale=1' },
+ },
+ {
+ kind: 'meta',
+ key: 'meta:color-scheme',
+ attrs: { name: 'color-scheme', content: 'light dark' },
+ },
+ ]);
- const html = renderHead(headFromMeta({ title: 'T' } as never, { renderMeta: () => [] }));
+ const html = renderHead(
+ headFromMeta({ title: 'T' } as never, {
+ renderMeta: () => [{ kind: 'title', key: 'title', content: 'T' }],
+ }),
+ );
+ expect(html.indexOf('<meta charset="utf-8">')).toBeLessThan(html.indexOf('<title>T</title>'));🤖 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 `@packages/render/src/head.test.ts` around lines 218 - 227, Strengthen the
tests around documentBaseline() and headFromMeta(): assert exact charset,
viewport, and color-scheme attributes rather than only keys, and provide one
route/SEO tag through renderMeta so the test compares its position against the
baseline tags. Preserve documentBaseline() merging first while confirming route
tags remain overrideable.
Source: Path instructions
| const PACKAGE = '/srv/demo/packages/ui/src/card.module.scss'; | ||
| /** The app's global layer: tokens + reset, the one file both surfaces' documents must carry. */ | ||
| const GLOBAL = '/srv/demo/apps/web/shared/global.scss'; | ||
| const GLOBAL_CSS = ':root{--color-fg:38 34 31}*{margin:0}'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Do not use raw colours in test CSS.
Line 16 and Lines 106, 116, and 126 use raw colour values. These values are not needed to test stylesheet ordering or deduplication. Use non-colour declarations and a neutral custom-property sentinel instead.
As per coding guidelines, “Do not use raw colours; use the framework's colour tokens instead.”
Proposed fix
-const GLOBAL_CSS = ':root{--color-fg:38 34 31}*{margin:0}';
+const GLOBAL_CSS = ':root{--global-test:1}*{margin:0}';
...
- loadStylesheet(SITE, '.hero{color:red}');
+ loadStylesheet(SITE, '.hero{display:block}');
...
- loadStylesheet(APP, '.panel{color:blue}');
+ loadStylesheet(APP, '.panel{display:grid}');
...
- loadStylesheet(PACKAGE, '.card{color:green}');
+ loadStylesheet(PACKAGE, '.card{contain:layout}');Also applies to: 106-106, 116-116, 126-126
🤖 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 `@packages/render/src/module-loader.test.ts` at line 16, Replace the raw color
values in the test CSS constants and fixtures around GLOBAL_CSS and the cases at
lines 106, 116, and 126 with non-color declarations plus a neutral
custom-property sentinel. Preserve the existing stylesheet ordering and
deduplication assertions while removing all literal color values from these
tests.
Source: Coding guidelines
|
|
||
| // …and the one source that admits it under the framework's locked CSP. The baseline is | ||
| // `style-src 'self' 'sha256-…'` with no 'unsafe-inline', so a tag whose hash the header does | ||
| // not carry is a stylesheet the browser parses zero rules out of. | ||
| `Content-Security-Policy: style-src 'self' ${brandStyleCspSource(brand)}`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep style-src-attr in the CSP example.
The header on Line 165 omits style-src-attr. If an application copies this complete header, browsers fall back to the restrictive style-src directive and block intended inline custom-property styles. Add the explicit attribute directive.
Proposed documentation fix
-Content-Security-Policy: style-src 'self' ${brandStyleCspSource(brand)};
+Content-Security-Policy: style-src 'self' ${brandStyleCspSource(brand)}; style-src-attr 'unsafe-inline';As per path instructions, “allow style attributes only where intended.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // …and the one source that admits it under the framework's locked CSP. The baseline is | |
| // `style-src 'self' 'sha256-…'` with no 'unsafe-inline', so a tag whose hash the header does | |
| // not carry is a stylesheet the browser parses zero rules out of. | |
| `Content-Security-Policy: style-src 'self' ${brandStyleCspSource(brand)}`; | |
| // …and the one source that admits it under the framework's locked CSP. The baseline is | |
| // `style-src 'self' 'sha256-…'` with no 'unsafe-inline', so a tag whose hash the header does | |
| // not carry is a stylesheet the browser parses zero rules out of. | |
| `Content-Security-Policy: style-src 'self' ${brandStyleCspSource(brand)}; style-src-attr 'unsafe-inline'`; |
🤖 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 `@packages/ui/README.md` around lines 161 - 165, Update the CSP example around
brandStyleCspSource(brand) to include an explicit style-src-attr directive
alongside style-src, allowing inline style attributes only as intended while
preserving the existing stylesheet source restriction.
Source: Path instructions
Four defects, none visible from a test suite, all found by opening the deployed demo app in a real browser.
1. The CSP blocked the framework's own stylesheet
style-src 'self'against a 64,090-character inline<style>.document.querySelector('style').sheetwas null — zero rules parsed out of 64KB. Every Ultimate app has rendered unstyled in production since the header shipped.It survived because
defineHttpConfigsetscsp.reportOnly: dev:x devreports the violation to the console and paints the page anyway, so only production ever enforced it.The style bodies are known at boot, so
style-srcnow carries a sha256 per emitted<style>, computed from the real text — never a checked-in constant. It lives instartWeb, so every caller ofstartRolesis covered and a future entry point cannot forget the line.A/B on one document, real production boot (
dev: false, enforced):style.sheetnullsheet.cssRules.lengthcolorrgb(0,0,0)rgb(179,38,30)buildCsploses itsnonceparameter — nothing in the framework ever passed one, and a second inline-coverage path that never works is what axiom 1 forbids. This is a signature change to a published export; it had no effect at any call site.style-src-attr 'unsafe-inline'is a deliberate, attribute-scoped relaxation: 14 components pass computed custom properties (--avatar-size: 42px), which no hash and no'unsafe-hashes'can express — the browser says so itself.<style>elements still fall back to the hashedstyle-src, so adefineTheme()value carrying</style>is still a refusal. Moving those properties into generated classes is the real fix and a much larger change.2.
packages/ui/src/global.scsswas imported by nothing, anywhereIts own header says "One import for an app document shell: every custom property, then the reset." Nothing imported it — not the framework, not the scaffold, not the reference app. 0
:rootblocks reached any document, so everyvar(--color-*)/var(--space-*)resolved to""and each rule reading one was dropped.Two more bugs in the same function:
stylesFordroppedsurface === 'shared'entirely (a stylesheet in the one directory both graphs import was filtered out of both documents), and ordered by insertion rather than globals-first (the reset could lose a specificity tie to whichever page loaded first).The global layer arrives through the app's own module graph —
shared/global.scss+ a one-lineshared/global.ts, both scaffolded byx new— becauserenderis tier 4 anduiis tier 5, so no framework package may pull it in. No new sideways edge, no widened tier table.The guard:
X_STYLES_GLOBAL_MISSINGfailsx verify'sbudgetsstep when a rendered document carries CSS and defines no:rootcustom properties. Cutting the one edge reproduces the production bug at 64,070 characters — against the live deploy's 64,090.3. The
<head>had no charset, viewport orcolor-schemeWithout a viewport a phone lays the page out at ~980px and scales it down, so every deployed app rendered zoomed-out regardless of its CSS.
documentBaseline()merges first, so a route can still override any of the three.4. A browser that failed
auth: 'required'got problem+json as raw textNow
303 → /signin?next=%2Fdashboard; an agent asking for JSON still gets the problem document. Off until an app names its page — guessing/signinfor an app that spells it/loginis a 404, which is worse than the JSON it replaced.nextAfterSignInis the one reader of?next=and refuses anything that is not a same-origin path (https://evil.test,//evil.test,/\evil.test,javascript:) — the page that hands out a session is the last place an open redirect belongs.createSession/createAccountredirect a browser too; the native form POST used to answer one with a JSON blob.Measured, on the demo app
0 → 585 CSS rules parsed, tokens resolving (
--color-bg: 253 246 240,--space-4: 1rem),<main>computingdisplay: gridwith real padding and colour, 0 CSP console errors.The landing page was still the generator's placeholder — "Everything you need, one command from shippable" over a single link to a route that 401s a signed-out visitor. Its old test passed the whole time, because the placeholder had a title too; the new one asserts what the page emits.
Gates
bun run verify— all 17 steps greenbun run scripts/reference-app-gate.ts— 10 of 17 pass, 7 pinned, ratchet holds🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Documentation