feat(design-system): add professional finish review loop - #153
Conversation
Why: JSC-77 needs one canonical professional-finish review loop for hierarchy, state quality, focus consistency, and exemplar reference coverage. What: add the professional-finish rubric and gold-standard reference set, scope shared focus selectors to opt-in design-system targets, wire the exemplar runner to precheck the contract, and stabilize generated-source/type-check resolution for widget manifest rebuilds. Impact/Risk: shared CSS no longer applies a bare global :focus-visible ring or global mouse-focus outline suppression. Widget manifest hashes are refreshed from the stabilized build path. Runtime/token builds force project emits so stale build info cannot hide missing declaration files. Validation: pnpm validate:tokens -> pass Validation: pnpm design-system-guidance:check:ci -> pass (existing warnings only) Validation: pnpm generated-source:check -> pass Validation: pnpm typecheck -> pass Validation: pnpm test:policy -> pass Validation: pnpm test:exemplar-evaluation:list -> pass (nonfatal sharp/libvips duplicate-class warning during web visual listing) Validation: git diff --check -> pass Co-authored-by: Codex <noreply@openai.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a professional-finish precheck and review loop that gates exemplar evaluation, scopes focus styling to opt-in Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Runner (script)
participant FS as Filesystem (docs, enhanced.css)
participant Precheck as Precheck/Checker
participant Evaluator as Exemplar Evaluator
participant CI as Test Harness
Runner->>FS: read PROFESSIONAL_FINISH_REVIEW.md
Runner->>FS: read packages/tokens/src/enhanced.css
Runner->>Precheck: invoke checks (function or command)
Precheck->>FS: validate rubric headers, reference set, focus selectors
alt precheck passes
Runner->>Evaluator: run `pnpm test:exemplar-evaluation`
Evaluator->>CI: produce screenshots/results
CI->>Runner: evaluation outcome
else precheck fails
Precheck->>Runner: return failure (abort)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tokens/src/enhanced/focus.ts (1)
96-116:⚠️ Potential issue | 🟠 MajorScope mouse-focus outline suppression to opt-in targets only.
The exported CSS still applies
:focus:not(:focus-visible)globally, which conflicts with the new scoped focus contract and can remove native outlines from non-opted-in elements.Proposed fix
- :focus:not(:focus-visible) { + .ds-focusable:focus:not(:focus-visible), + [data-ds-focusable]:focus:not(:focus-visible) { outline: none; }As per coding guidelines
**: Use repository evidence first... Treat contradictions between code, docs, and workflow policy as review-worthy even when code changes look reasonable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tokens/src/enhanced/focus.ts` around lines 96 - 116, The global rule ":focus:not(:focus-visible)" in the focusVisibleCSS string removes native outlines for all elements; limit this suppression to opt-in elements by replacing that global selector with the same scoped selectors used above (e.g., ".ds-focusable:focus:not(:focus-visible)" and "[data-ds-focusable]:focus:not(:focus-visible)") so only elements using the ds-focusable contract lose outlines when focused by mouse; update the focusVisibleCSS constant accordingly.docs/design-system/PROFESSIONAL_FINISH_REVIEW.md (1)
86-87: 🧹 Nitpick | 🔵 TrivialAdd trailing newline.
The file ends at line 86 without a trailing newline. Most style guides and tools expect files to end with a newline character.
📄 Proposed fix
- Update this reference set when a protected exemplar is renamed, removed, or superseded. +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-system/PROFESSIONAL_FINISH_REVIEW.md` around lines 86 - 87, The file ends without a trailing newline; open docs/design-system/PROFESSIONAL_FINISH_REVIEW.md and add a single newline character at the end of the file so the final line "Update this reference set when a protected exemplar is renamed, removed, or superseded." is terminated by a newline (ensure the file now ends with '\n').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design-system/A11Y_CONTRACTS.md`:
- Around line 30-35: Add a generated Table of Contents near the top of the
A11Y_CONTRACTS.md file (just below the main title/header) that lists the
top-level sections (e.g., "Shared Focus Indicator", any other H2/H3 headers)
with Markdown anchor links; update the TOC whenever headers change and follow
the project's Markdown TOC style for other docs matching **/*.md so users can
navigate large docs easily.
In `@packages/json-render/tsconfig.json`:
- Around line 5-7: The tsconfig path mapping for the module identifier
"@design-studio/ui" points to a non-existent declaration file
"packages/ui/dist/index.d.ts"; update the "paths" entry in the JSON "paths"
section of tsconfig.json so that "@design-studio/ui" maps to the package source
(packages/ui/src/index.ts) to match other dev package mappings and restore
successful type-checking.
In `@scripts/policy/run-exemplar-evaluation.mjs`:
- Around line 63-67: The assertIncludes function throws a generic Error when
needle is not found; enhance the message by including the source identifier plus
a short snippet or truncated preview of content to aid debugging. Update
function assertIncludes(content, needle, source) to construct an error string
that contains source, the missing needle, and either a content.slice(0, N)
preview or the index position (content.indexOf(needle)) so the thrown Error
carries more context for locating the problem during tests.
---
Outside diff comments:
In `@docs/design-system/PROFESSIONAL_FINISH_REVIEW.md`:
- Around line 86-87: The file ends without a trailing newline; open
docs/design-system/PROFESSIONAL_FINISH_REVIEW.md and add a single newline
character at the end of the file so the final line "Update this reference set
when a protected exemplar is renamed, removed, or superseded." is terminated by
a newline (ensure the file now ends with '\n').
In `@packages/tokens/src/enhanced/focus.ts`:
- Around line 96-116: The global rule ":focus:not(:focus-visible)" in the
focusVisibleCSS string removes native outlines for all elements; limit this
suppression to opt-in elements by replacing that global selector with the same
scoped selectors used above (e.g., ".ds-focusable:focus:not(:focus-visible)" and
"[data-ds-focusable]:focus:not(:focus-visible)") so only elements using the
ds-focusable contract lose outlines when focused by mouse; update the
focusVisibleCSS constant accordingly.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66b8efe3-8df6-42df-a152-2b2bfec7fa21
⛔ Files ignored due to path filters (1)
packages/widgets/src/sdk/generated/widget-manifest.jsis excluded by!**/generated/**
📒 Files selected for processing (14)
FORJAMIE.mddocs/design-system/A11Y_CONTRACTS.mddocs/design-system/PROFESSIONAL_FINISH_REVIEW.mddocs/design-system/PROFESSIONAL_UI_CONTRACT.mdpackages/cloudflare-template/src/worker/widget-manifest.generated.tspackages/json-render/tsconfig.jsonpackages/runtime/package.jsonpackages/tokens/package.jsonpackages/tokens/src/enhanced.csspackages/tokens/src/enhanced/focus.tspackages/ui/tsconfig.jsonpackages/widgets/vite.config.tsplatforms/web/apps/web/vite.config.tsscripts/policy/run-exemplar-evaluation.mjs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
🧰 Additional context used
📓 Path-based instructions (20)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome (
biome.json) for JS/TS formatting and keep files formatted viapnpm format
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{js,ts,tsx,jsx,py,rs,mjs,cjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{js,ts,tsx,jsx,py,rs,mjs,cjs}: Fabricated data/entropy (Math.random() without injected seed), hard-coded mock responses, TODO/FIXME/HACK comments, placeholder stubs, disabled features, or fake metrics MUST NOT exist in production code paths
Functions SHOULD be <= 40 LOC; split if readability suffers
No ambient randomness/time in core logic; inject seeds/clocks/IDs
Never swallow errors; add context and route to logging layer
Constants: UPPER_SNAKE_CASE
No hard-coded secrets; use env injection/secret manager; validate/sanitize all external inputs
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{js,ts,tsx,jsx,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Apps/binaries/services MUST include service identity in outputs, error messages, and logs (service:"<service_name>")
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{js,ts,tsx,jsx,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Long-running work MUST accept cancellation (AbortSignal in JS/TS; cancellation tokens/channels in Rust)
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{js,ts,tsx,jsx,mjs,json}
📄 CodeRabbit inference engine (CODESTYLE.md)
Packages MUST target the repo baseline Node version (pinned in .mise.toml and compat.json)
Files:
packages/widgets/vite.config.tspackages/json-render/tsconfig.jsonpackages/tokens/src/enhanced/focus.tspackages/tokens/package.jsonpackages/ui/tsconfig.jsonpackages/runtime/package.jsonscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{ts,tsx,mts,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
JSON imports MUST use import attributes (import data from "./foo.json" with { type: "json" })
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{ts,tsx}: Explicit types at all public API boundaries (functions, modules, React props)
Use concrete types or unknown + narrowing instead of any (forbidden pattern: : any, as any, Promise, Record<string, any>)
Unsafe type assertions (value as unknown as T) forbidden; use type guards or schema validation instead
//@ts-ignoreand //@ts-nocheckare forbidden; use //@ts-expect-error-- reason + ticket instead
Unsafe 'as SomeType' without runtime guard is forbidden; use guard function or schema validator
JSON.parse() and Response.json() return any; boundary mitigation MUST use schema validation (Zod/Valibot) or typed parser helper
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{js,ts,tsx,jsx,mjs,cjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{js,ts,tsx,jsx,mjs,cjs}: ESLint-disable comments MUST include reason and ticket; expiry date optional
JavaScript/TypeScript variables/functions: camelCase
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{ts,tsx,js,mjs}: Exported async APIs that perform I/O or long work MUST accept AbortSignal
JSON inputs at boundaries in JS/TS MUST be schema-validated
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{rs,ts,tsx}
📄 CodeRabbit inference engine (CODESTYLE.md)
Tauri UI-facing state MUST be deterministic and testable
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{ts,tsx,jsx,js,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Types/components: PascalCase
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{ts,tsx,js,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
CLI/TUI: --plain / --no-color modes required
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{ts,js,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{ts,js,py,rs,mjs}: OpenTelemetry SHOULD be used where services/CLIs exist; logs SHOULD be structured and include service at app boundaries
Streaming: default token delta streaming for CLIs, optional aggregated mode, JSON event streaming optional if supported
MCP adapters/helpers MUST not hard-code user-specific paths; health checks MUST be scriptable; egress/network policies MUST be explicit
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.{js,ts,tsx,jsx,md,mdx,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Waivers across ESLint, Vale, Semgrep, Clippy, CI checks MUST include: rule ID, reason, ticket/issue reference, expiry (date) OR ADR reference; expired waivers MUST fail CI
Files:
packages/widgets/vite.config.tspackages/tokens/src/enhanced/focus.tsdocs/design-system/A11Y_CONTRACTS.mddocs/design-system/PROFESSIONAL_FINISH_REVIEW.mdFORJAMIE.mddocs/design-system/PROFESSIONAL_UI_CONTRACT.mdscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**
⚙️ CodeRabbit configuration file
**: Use repository evidence first: package scripts, tracked docs, harness contracts, and validation wrappers over assumptions.
Treat contradictions between code, docs, and workflow policy as review-worthy even when code changes look reasonable.
Prioritize behavioral risk, missing validation, and governance drift over style-only feedback.
Files:
packages/widgets/vite.config.tspackages/json-render/tsconfig.jsonpackages/tokens/src/enhanced/focus.tsdocs/design-system/A11Y_CONTRACTS.mdpackages/tokens/package.jsonpackages/ui/tsconfig.jsonpackages/runtime/package.jsondocs/design-system/PROFESSIONAL_FINISH_REVIEW.mdFORJAMIE.mddocs/design-system/PROFESSIONAL_UI_CONTRACT.mdpackages/tokens/src/enhanced.cssscripts/policy/run-exemplar-evaluation.mjsplatforms/web/apps/web/vite.config.tspackages/cloudflare-template/src/worker/widget-manifest.generated.ts
**/*.json
📄 CodeRabbit inference engine (AGENTS.md)
Always verify JSON syntax with
jqor similar tool before declaring JSON fixes completeJSON MUST be valid UTF-8; prefer machine-generated JSON for large files; minimize hand-edited large JSON
Files:
packages/json-render/tsconfig.jsonpackages/tokens/package.jsonpackages/ui/tsconfig.jsonpackages/runtime/package.json
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Add a Table of Contents for documentation files
Files:
docs/design-system/A11Y_CONTRACTS.mddocs/design-system/PROFESSIONAL_FINISH_REVIEW.mdFORJAMIE.mddocs/design-system/PROFESSIONAL_UI_CONTRACT.md
**/*.{md,mdx,adoc,rst}
📄 CodeRabbit inference engine (CODESTYLE.md)
All docs and long-form prose MUST be linted with Vale (scope: **/.md, **/.mdx, **/.adoc, **/.rst)
Files:
docs/design-system/A11Y_CONTRACTS.mddocs/design-system/PROFESSIONAL_FINISH_REVIEW.mdFORJAMIE.mddocs/design-system/PROFESSIONAL_UI_CONTRACT.md
FORJAMIE.md
📄 CodeRabbit inference engine (AGENTS.md)
Maintain a detailed
FORJAMIE.mdat the repo root explaining the whole project in plain language for future reference
Files:
FORJAMIE.md
**/*.{tsx,jsx,html,css}
📄 CodeRabbit inference engine (CODESTYLE.md)
Tailwind: avoid "magic numbers" when theme tokens exist
Files:
packages/tokens/src/enhanced.css
🔍 Remote MCP Linear
JSC-77 Issue Context and Requirements
Core Objective
The PR addresses JSC-77: "Add professional-finish review loops with focus consistency, visual QA, and gold-standard references", which is part of the larger JSC-72 audit backlog focused on improving agent-generated UI quality based on the March 2026 audit findings.
Original Requirements (JSC-77 "Done when" criteria)
- Focus behavior has one clear canonical pattern — The PR delivers this by scoping all focus styling to opt-in selectors (
.ds-focusable/[data-ds-focusable]), eliminating contradictory guidance. - Reviewers and agents have a shared rubric for professional finish — The PR introduces
PROFESSIONAL_FINISH_REVIEW.mdwith a multi-dimension rubric covering hierarchy, spacing rhythm, focus quality, state quality, and motion restraint. - Visual QA covers more than pixel drift — The PR adds
run-exemplar-evaluation.mjsprecheck logic that validates rubric adherence and reference alignment, treating screenshot parity as insufficient.
Supporting Audit Context (JSC-72)
The parent issue indicates the March 2026 audit identified that "the biggest gap is not missing tokens" but rather that "design-system integrity, semantic-slot discipline, accessibility guidance, composition rules, and review loops are not yet encoded strongly enough." This PR directly addresses the review-loop and composition-rules gaps.
Risk Profile
- Primary risks: Focus styling scope changes (global to scoped) and Vite/TypeScript workspace alias resolution for local development consistency.
- Rollback mechanism: Reverting the PR reverts generated widget manifest hashes and focus CSS to prior global behavior.
Issue Status
- Current state: Todo (unstarted in Linear, but PR
#153is open/draft) - Labels: Agent, Docs, Improvement
- Assigned: jscraik
- Linked PR: GitHub PR
#153attached as supporting artifact
🔇 Additional comments (13)
packages/tokens/package.json (1)
34-34: Build-script update looks correct.Switching to
tsc -b ... --forceis consistent with project-reference builds and helps prevent stale incremental state from hiding missing declaration output.packages/widgets/vite.config.ts (1)
62-65: Alias additions are aligned with actual widget imports.These entries correctly close the local workspace-resolution gap for
@design-studio/json-renderand@design-studio/ui.platforms/web/apps/web/vite.config.ts (1)
12-64: Workspace alias coverage is solid.The expanded alias map matches real import patterns in the web app and should improve local source resolution consistency.
packages/runtime/package.json (1)
20-20: Runtime build-script change is appropriate.Using
tsc -b ... --forcehere is consistent with monorepo reference builds and deterministic emit behavior.packages/cloudflare-template/src/worker/widget-manifest.generated.ts (1)
9-137: Generated manifest hash refresh looks consistent.The update preserves widget identity/path metadata while rotating generated build hashes as expected.
packages/tokens/src/enhanced.css (1)
97-117: Well-designed scoped focus pattern.The focus styling is now properly scoped to opt-in selectors, preventing double-ring conflicts with component-owned focus styles. The implementation correctly:
- Uses both class (
.ds-focusable) and data attribute ([data-ds-focusable]) patterns for flexibility- Scopes the inset variant for inputs consistently
- Restricts mouse-focus suppression to the same scoped elements, preserving native outlines elsewhere
This is a breaking change for any components that previously relied on the global
:focus-visiblestyling. Existing focusable elements will now need to add.ds-focusableordata-ds-focusableto receive the design-system focus ring.FORJAMIE.md (1)
193-195: Clear documentation of the professional-finish review loop.The changelog entry accurately documents the JSC-77 changes including the contract precheck, scoped focus selectors, and workspace resolution updates. This maintains the detailed project map required by the coding guidelines.
docs/design-system/PROFESSIONAL_FINISH_REVIEW.md (1)
49-66: Gold-standard reference set alignment verified.The 11 references in this document match the
GOLD_STANDARD_REFERENCESconstant inscripts/policy/run-exemplar-evaluation.mjs. The script validates that each reference appears in this document, creating a bidirectional contract between documentation and enforcement.Minor observation: The last reference row combines "template browser page" and "template widget page" in a single cell, while the script lists them as separate entries. This works because the script uses
includes()to check presence, but consider splitting them into separate rows for clarity and to match the script's array structure.scripts/policy/run-exemplar-evaluation.mjs (2)
135-155: Clean runner refactor to support both function and command checks.The check runner correctly handles both synchronous
runfunctions andspawnSynccommands with proper error handling. Theresult = { status: 0 }initialization and conditional update pattern is clear.
89-93: Regex correctly detects bare:focus-visiblerules.The regex
/(^|[\s,{]):focus-visible\s*\{/effectively prevents bare global:focus-visiblerules and is working as intended. Testing against the actualenhanced.cssconfirms all:focus-visibleselectors are properly scoped (.ds-focusable:focus-visible,[data-ds-focusable]:focus-visible, etc.) and the regex correctly identifies zero bare violations. The pattern's use of\scovers whitespace edge cases including newlines, and the implementation is sound.docs/design-system/PROFESSIONAL_UI_CONTRACT.md (2)
131-147: Consistent contract updates for focus scoping and professional finish review.The Focus Contract expansion (lines 131-133) properly documents the mouse-only focus suppression scoping requirement and points to the canonical implementation. The new Professional Finish Review Loop section (lines 135-147) correctly delegates to the detailed rubric document while establishing the key constraint that "screenshot diff parity" is insufficient.
These additions align with the code changes in
packages/tokens/src/enhanced.cssand the validation inscripts/policy/run-exemplar-evaluation.mjs.
191-192: Enforcement inputs properly updated.The contract correctly documents that professional finish is enforced by coupling the browser gate with the precheck (line 191-192), and
pnpm test:exemplar-evaluationis added to the enforcement inputs list (line 205). This creates a clear audit trail from contract to executable validation.Also applies to: 205-205
packages/ui/tsconfig.json (1)
7-10: [rewritten comment]
[classification tag]
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffa2b53a22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "@design-studio/runtime": path.resolve(__dirname, "../../../../packages/runtime/src"), | ||
| "@design-studio/tokens": path.resolve(__dirname, "../../../../packages/tokens/src"), |
There was a problem hiding this comment.
These aliases switch @design-studio/runtime and @design-studio/tokens to source-folder resolution, but manualChunks in the same file still keys off id.includes("@design-studio/runtime") and id.includes("@design-studio/tokens"). In Vite/Rollup builds, id is the resolved absolute path (for example /packages/runtime/src/...), so those chunk rules stop matching and runtime/tokens code falls into default chunks, which regresses chunk isolation and cache behavior for production web builds.
Useful? React with 👍 / 👎.
Fixed 4 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/tsconfig.json`:
- Around line 8-9: Remove the hard-coded "paths" entries in
packages/ui/tsconfig.json that point to ../../packages/runtime/dist/index.d.ts
and ../../packages/tokens/dist/index.d.ts; instead rely on the root TypeScript
project references so TypeScript resolves `@design-studio/runtime` and
`@design-studio/tokens` from their source projects. Specifically, delete the
"paths" block in packages/ui/tsconfig.json (the entries referencing runtime and
tokens), confirm the root tsconfig.json includes project references for runtime
and tokens, and ensure packages/ui package.json lists `@design-studio/runtime` and
`@design-studio/tokens` as workspace dependencies so the composite project
build/resolution works without prebuilt dist .d.ts artifacts.
In `@scripts/policy/run-exemplar-evaluation.mjs`:
- Around line 4-6: The new log/output and error messages related to
PROFESSIONAL_FINISH_DOC and ENHANCED_CSS must include the service identity tag;
update any console.log/console.error or thrown Error text introduced near those
constants and the later lines around the block referenced (around the output at
142-148) to append or embed service:"run-exemplar-evaluation" (or the canonical
service name) into the message strings so every emitted message includes
service:"<service_name>" for compliance; search for usages of
PROFESSIONAL_FINISH_DOC, ENHANCED_CSS, and the output/error statements in that
module and modify their message formatting to include the service identity.
- Around line 93-101: The current precheck in run-exemplar-evaluation.mjs guards
against a bare global :focus-visible rule but misses rules that remove native
outlines such as a global ":focus:not(:focus-visible) { outline: none; }";
update the validation around enhancedCss (and the ENHANCED_CSS constant) to also
detect and reject patterns that target :focus:not(:focus-visible) or global
:focus selectors combined with "outline: none" (e.g., via a regex that looks for
/:focus\s*:\s*not\s*\(\s*:focus-visible\s*\)\s*\{[^}]*outline\s*:\s*none/ or
global :focus rules setting outline:none) and throw an Error with the same
style/message as the existing check; ensure assertIncludes usage remains to
validate the required .ds-focusable and [data-ds-focusable] selectors.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 899527e2-fd65-4995-9ed8-b14f7cf2d231
📒 Files selected for processing (4)
docs/design-system/A11Y_CONTRACTS.mdpackages/json-render/tsconfig.jsonpackages/ui/tsconfig.jsonscripts/policy/run-exemplar-evaluation.mjs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (15)
**/*.json
📄 CodeRabbit inference engine (AGENTS.md)
Always verify JSON syntax with
jqor similar tool before declaring JSON fixes completeJSON MUST be valid UTF-8; prefer machine-generated JSON for large files; minimize hand-edited large JSON
Files:
packages/ui/tsconfig.jsonpackages/json-render/tsconfig.json
**/*.{js,ts,tsx,jsx,mjs,json}
📄 CodeRabbit inference engine (CODESTYLE.md)
Packages MUST target the repo baseline Node version (pinned in .mise.toml and compat.json)
Files:
packages/ui/tsconfig.jsonpackages/json-render/tsconfig.jsonscripts/policy/run-exemplar-evaluation.mjs
**
⚙️ CodeRabbit configuration file
**: Use repository evidence first: package scripts, tracked docs, harness contracts, and validation wrappers over assumptions.
Treat contradictions between code, docs, and workflow policy as review-worthy even when code changes look reasonable.
Prioritize behavioral risk, missing validation, and governance drift over style-only feedback.
Files:
packages/ui/tsconfig.jsondocs/design-system/A11Y_CONTRACTS.mdpackages/json-render/tsconfig.jsonscripts/policy/run-exemplar-evaluation.mjs
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Add a Table of Contents for documentation files
Files:
docs/design-system/A11Y_CONTRACTS.md
**/*.{md,mdx,adoc,rst}
📄 CodeRabbit inference engine (CODESTYLE.md)
All docs and long-form prose MUST be linted with Vale (scope: **/.md, **/.mdx, **/.adoc, **/.rst)
Files:
docs/design-system/A11Y_CONTRACTS.md
**/*.{js,ts,tsx,jsx,md,mdx,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Waivers across ESLint, Vale, Semgrep, Clippy, CI checks MUST include: rule ID, reason, ticket/issue reference, expiry (date) OR ADR reference; expired waivers MUST fail CI
Files:
docs/design-system/A11Y_CONTRACTS.mdscripts/policy/run-exemplar-evaluation.mjs
**/*.{js,ts,tsx,jsx,py,rs,mjs,cjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{js,ts,tsx,jsx,py,rs,mjs,cjs}: Fabricated data/entropy (Math.random() without injected seed), hard-coded mock responses, TODO/FIXME/HACK comments, placeholder stubs, disabled features, or fake metrics MUST NOT exist in production code paths
Functions SHOULD be <= 40 LOC; split if readability suffers
No ambient randomness/time in core logic; inject seeds/clocks/IDs
Never swallow errors; add context and route to logging layer
Constants: UPPER_SNAKE_CASE
No hard-coded secrets; use env injection/secret manager; validate/sanitize all external inputs
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{js,ts,tsx,jsx,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Apps/binaries/services MUST include service identity in outputs, error messages, and logs (service:"<service_name>")
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{js,ts,tsx,jsx,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Long-running work MUST accept cancellation (AbortSignal in JS/TS; cancellation tokens/channels in Rust)
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,tsx,mts,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
JSON imports MUST use import attributes (import data from "./foo.json" with { type: "json" })
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{js,ts,tsx,jsx,mjs,cjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{js,ts,tsx,jsx,mjs,cjs}: ESLint-disable comments MUST include reason and ticket; expiry date optional
JavaScript/TypeScript variables/functions: camelCase
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{ts,tsx,js,mjs}: Exported async APIs that perform I/O or long work MUST accept AbortSignal
JSON inputs at boundaries in JS/TS MUST be schema-validated
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,tsx,jsx,js,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Types/components: PascalCase
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,tsx,js,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
CLI/TUI: --plain / --no-color modes required
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,js,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{ts,js,py,rs,mjs}: OpenTelemetry SHOULD be used where services/CLIs exist; logs SHOULD be structured and include service at app boundaries
Streaming: default token delta streaming for CLIs, optional aggregated mode, JSON event streaming optional if supported
MCP adapters/helpers MUST not hard-code user-specific paths; health checks MUST be scriptable; egress/network policies MUST be explicit
Files:
scripts/policy/run-exemplar-evaluation.mjs
🔍 Remote MCP Linear
Relevant Linear issue context (JSC-77)
- Title: Add professional-finish review loops with focus consistency, visual QA, and gold-standard references.
- Goal: add review mechanisms to catch the last ~20% of quality issues in agent-generated UI (focus, hierarchy, spacing, state, motion).
- Deliverables: reconcile canonical focus-ring strategy; add a visual/design review rubric; add a gold‑standard reference set of exemplar screens/stories.
- Done-when criteria: (1) single canonical focus behavior, (2) shared rubric for professional finish, (3) visual QA beyond pixel drift.
- Current status: In Progress / started (startedAt: 2026-04-25T19:09:29.572Z). Branch name: jscraik/jsc-77-add-professional-finish-review-loops-with-focus-consistency.
- Priority/labels/assignee: Medium priority; labels: Agent, Docs, Improvement; assignee: jscraik.
- Linked PR: feat(design-system): add professional finish review loop — #153 (attached to the Linear issue).
🔇 Additional comments (4)
docs/design-system/A11Y_CONTRACTS.md (2)
5-56: TOC addition looks good and improves navigability.This is clear and complete for the current section set.
As per coding guidelines
**/*.md: Add a Table of Contents for documentation files.
82-88: Shared Focus Indicator contract is clear and testable.The requirements are specific enough to be enforced by policy checks and component-level review.
scripts/policy/run-exemplar-evaluation.mjs (1)
63-71: Better assertion diagnostics here.Including a preview in failure output materially improves debugging when contract markers are missing.
packages/json-render/tsconfig.json (1)
5-7: Good fix:@design-studio/uinow resolves to the workspace source entrypoint.This aligns with the available
packages/ui/src/index.tsexport surface and avoids the prior missing-distdeclaration failure mode.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c42c6c4441
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| __dirname, | ||
| "../../../../packages/ui/src/components/ui/base", | ||
| ), | ||
| "@design-studio/ui/chat": path.resolve(__dirname, "../../../../packages/ui/src/app/chat"), |
There was a problem hiding this comment.
This alias points @design-studio/ui/chat at packages/ui/src/app/chat, but the manualChunks logic in the same Vite config still only recognizes chat modules under /src/components/ui/chat/ (or /dist/chat.js), so chat code imported from the new alias path will no longer land in the design-studio-chat chunk. In production web builds that means the intended chat chunk split is silently lost, increasing chunk churn and weakening cache locality for chat-only changes.
Useful? React with 👍 / 👎.
| assertIncludes(reviewDoc, reference, PROFESSIONAL_FINISH_DOC); | ||
| } | ||
|
|
||
| if (/(^|[\s,{]):focus-visible\s*\{/.test(enhancedCss)) { |
There was a problem hiding this comment.
The contract check regex only flags bare selectors written as :focus-visible { ... }, so a prohibited global selector can slip through as part of a list like :focus-visible, .ds-focusable:focus-visible { ... } without failing the precheck. Because this script is meant to enforce the focus-scope contract in CI, that blind spot allows a real policy regression to pass undetected.
Useful? React with 👍 / 👎.
Why: CodeRabbit review found the professional-finish precheck needed service-tagged output and stronger native outline suppression detection, and the UI package should not hard-code dist declaration paths for runtime/token packages. What: add service identity to exemplar evaluation output/errors, reject bare global :focus outline suppression in the professional-finish contract, and replace UI dist path overrides with references to runtime/token build configs. Impact/Risk: local UI type-check now relies on build-config project references instead of package-local dist aliases. The exemplar precheck will fail closed if a global focus outline reset returns. Validation: jq . packages/ui/tsconfig.json >/dev/null -> pass Validation: pnpm -C packages/ui type-check -> pass Validation: pnpm test:exemplar-evaluation:list -> pass Validation: pnpm test:policy -> pass Validation: pnpm typecheck -> pass Validation: git diff --check -> pass Co-authored-by: Codex <noreply@openai.com>
Why: the remote branch advanced with CodeRabbit autofix changes while the local review follow-up commit was ready to push. What: merge the A11Y contract documentation addition and combine the missing-marker preview with service-tagged exemplar-evaluation errors. Keep json-render on its existing dist declaration mapping because the remote source-path mapping fails package type-checking. Impact/Risk: preserves the professional-finish review fixes while retaining the working json-render type-check contract. Validation: jq . packages/ui/tsconfig.json >/dev/null -> pass Validation: jq . packages/json-render/tsconfig.json >/dev/null -> pass Validation: pnpm typecheck -> pass Validation: pnpm test:exemplar-evaluation:list -> pass Validation: pnpm test:policy -> pass Validation: git diff --check -> pass Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/json-render/tsconfig.json (1)
5-7:⚠️ Potential issue | 🟠 MajorAvoid dist-only TS alias for
@design-studio/uiin json-render.This mapping reintroduces build-artifact coupling and can drift from workspace source resolution (for example, widget Vite aliases
@design-studio/uitopackages/ui/src).♻️ Proposed fix
"compilerOptions": { "outDir": "./dist", "paths": { - "@design-studio/ui": ["packages/ui/dist/index.d.ts"] + "@design-studio/ui": ["packages/ui/src/index.ts"] } },Run this to verify artifact coupling and alias mismatch:
#!/bin/bash set -euo pipefail echo "== Validate JSON syntax ==" jq . packages/json-render/tsconfig.json >/dev/null echo "== Current json-render alias target ==" jq -r '.compilerOptions.paths["@design-studio/ui"][]' packages/json-render/tsconfig.json echo "== Does mapped dist file exist now? ==" TARGET="$(jq -r '.compilerOptions.paths["@design-studio/ui"][0]' packages/json-render/tsconfig.json)" if [ -f "$TARGET" ]; then echo "FOUND: $TARGET" else echo "MISSING: $TARGET" fi echo "== Compare with workspace runtime alias usage ==" rg -n --type=ts -C2 '"@design-studio/ui"' packages/widgets/vite.config.tsAs per coding guidelines
**: Use repository evidence first: package scripts, tracked docs, harness contracts, and validation wrappers over assumptions. Treat contradictions between code, docs, and workflow policy as review-worthy even when code changes look reasonable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/json-render/tsconfig.json` around lines 5 - 7, The tsconfig paths in packages/json-render currently map the module alias "@design-studio/ui" to the built artifact (packages/ui/dist/index.d.ts), causing build-artifact coupling and mismatches with runtime/vite aliases; update the mapping for "@design-studio/ui" in packages/json-render's compilerOptions.paths to point to the workspace source (e.g., packages/ui/src entry) or to a workspace-aware alias (or remove the override so TypeScript resolves the package via project references), ensuring the change is made to the "@design-studio/ui" paths entry so json-render resolves the same source as widgets/vite.config.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/policy/run-exemplar-evaluation.mjs`:
- Around line 159-161: The caught-error logging currently emits Error instances
raw (via error.message) and can omit the required service identity tag; update
the console.error call in run-exemplar-evaluation.mjs to always prefix the
message with SERVICE_LOG_TAG and then the error text (e.g., use SERVICE_LOG_TAG
+ the error.message for Error instances or String(error) otherwise), ensuring
every emitted error includes the service:"<service_name>" identity.
---
Duplicate comments:
In `@packages/json-render/tsconfig.json`:
- Around line 5-7: The tsconfig paths in packages/json-render currently map the
module alias "@design-studio/ui" to the built artifact
(packages/ui/dist/index.d.ts), causing build-artifact coupling and mismatches
with runtime/vite aliases; update the mapping for "@design-studio/ui" in
packages/json-render's compilerOptions.paths to point to the workspace source
(e.g., packages/ui/src entry) or to a workspace-aware alias (or remove the
override so TypeScript resolves the package via project references), ensuring
the change is made to the "@design-studio/ui" paths entry so json-render
resolves the same source as widgets/vite.config.ts.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7bb521d-d02e-41ca-ad4e-688b1c3335b9
📒 Files selected for processing (4)
FORJAMIE.mdpackages/json-render/tsconfig.jsonpackages/ui/tsconfig.jsonscripts/policy/run-exemplar-evaluation.mjs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
🧰 Additional context used
📓 Path-based instructions (16)
**/*.json
📄 CodeRabbit inference engine (AGENTS.md)
Always verify JSON syntax with
jqor similar tool before declaring JSON fixes completeJSON MUST be valid UTF-8; prefer machine-generated JSON for large files; minimize hand-edited large JSON
Files:
packages/json-render/tsconfig.jsonpackages/ui/tsconfig.json
**/*.{js,ts,tsx,jsx,mjs,json}
📄 CodeRabbit inference engine (CODESTYLE.md)
Packages MUST target the repo baseline Node version (pinned in .mise.toml and compat.json)
Files:
packages/json-render/tsconfig.jsonpackages/ui/tsconfig.jsonscripts/policy/run-exemplar-evaluation.mjs
**
⚙️ CodeRabbit configuration file
**: Use repository evidence first: package scripts, tracked docs, harness contracts, and validation wrappers over assumptions.
Treat contradictions between code, docs, and workflow policy as review-worthy even when code changes look reasonable.
Prioritize behavioral risk, missing validation, and governance drift over style-only feedback.
Files:
packages/json-render/tsconfig.jsonpackages/ui/tsconfig.jsonscripts/policy/run-exemplar-evaluation.mjsFORJAMIE.md
**/*.{js,ts,tsx,jsx,py,rs,mjs,cjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{js,ts,tsx,jsx,py,rs,mjs,cjs}: Fabricated data/entropy (Math.random() without injected seed), hard-coded mock responses, TODO/FIXME/HACK comments, placeholder stubs, disabled features, or fake metrics MUST NOT exist in production code paths
Functions SHOULD be <= 40 LOC; split if readability suffers
No ambient randomness/time in core logic; inject seeds/clocks/IDs
Never swallow errors; add context and route to logging layer
Constants: UPPER_SNAKE_CASE
No hard-coded secrets; use env injection/secret manager; validate/sanitize all external inputs
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{js,ts,tsx,jsx,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Apps/binaries/services MUST include service identity in outputs, error messages, and logs (service:"<service_name>")
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{js,ts,tsx,jsx,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Long-running work MUST accept cancellation (AbortSignal in JS/TS; cancellation tokens/channels in Rust)
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,tsx,mts,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
JSON imports MUST use import attributes (import data from "./foo.json" with { type: "json" })
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{js,ts,tsx,jsx,mjs,cjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{js,ts,tsx,jsx,mjs,cjs}: ESLint-disable comments MUST include reason and ticket; expiry date optional
JavaScript/TypeScript variables/functions: camelCase
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{ts,tsx,js,mjs}: Exported async APIs that perform I/O or long work MUST accept AbortSignal
JSON inputs at boundaries in JS/TS MUST be schema-validated
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,tsx,jsx,js,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Types/components: PascalCase
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,tsx,js,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
CLI/TUI: --plain / --no-color modes required
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{ts,js,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
**/*.{ts,js,py,rs,mjs}: OpenTelemetry SHOULD be used where services/CLIs exist; logs SHOULD be structured and include service at app boundaries
Streaming: default token delta streaming for CLIs, optional aggregated mode, JSON event streaming optional if supported
MCP adapters/helpers MUST not hard-code user-specific paths; health checks MUST be scriptable; egress/network policies MUST be explicit
Files:
scripts/policy/run-exemplar-evaluation.mjs
**/*.{js,ts,tsx,jsx,md,mdx,py,rs,mjs}
📄 CodeRabbit inference engine (CODESTYLE.md)
Waivers across ESLint, Vale, Semgrep, Clippy, CI checks MUST include: rule ID, reason, ticket/issue reference, expiry (date) OR ADR reference; expired waivers MUST fail CI
Files:
scripts/policy/run-exemplar-evaluation.mjsFORJAMIE.md
FORJAMIE.md
📄 CodeRabbit inference engine (AGENTS.md)
Maintain a detailed
FORJAMIE.mdat the repo root explaining the whole project in plain language for future reference
Files:
FORJAMIE.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Add a Table of Contents for documentation files
Files:
FORJAMIE.md
**/*.{md,mdx,adoc,rst}
📄 CodeRabbit inference engine (CODESTYLE.md)
All docs and long-form prose MUST be linted with Vale (scope: **/.md, **/.mdx, **/.adoc, **/.rst)
Files:
FORJAMIE.md
🔍 Remote MCP Linear
Relevant context found
-
Linear issue JSC-77 (title, goal, deliverables, done-when, branch) — status: In Progress; branch: jscraik/jsc-77-add-professional-finish-review-loops-with-focus-consistency.
-
Issue comment by author summarizing the implemented work, validation steps, and notes:
- Confirms added files and key changes: PROFESSIONAL_FINISH_REVIEW.md, run-exemplar-evaluation.mjs gating, scoped focus CSS (.ds-focusable / [data-ds-focusable]), workspace path/alias stabilizations, and regenerated widget manifest.
- Lists validation commands that passed (typecheck, policy tests, exemplar-evaluation listing, builds, pre-push hook) and notes one non-fatal sharp/libvips duplicate-class warning during web visual listing.
- Notes sandboxed push initially failed due to macOS Chromium launch permissions; retry outside sandbox succeeded.
- PR remains draft pending independent CodeRabbit/Codex reviews.
🔇 Additional comments (3)
packages/ui/tsconfig.json (1)
8-11: Good shift to project references for runtime/tokens resolution.This removes fragile dist-path coupling in UI type resolution and better matches composite build linkage.
scripts/policy/run-exemplar-evaluation.mjs (1)
94-113: Professional-finish focus checks are well enforced here.The precheck now blocks bare global focus selectors and requires scoped design-system focus selectors before visuals run.
FORJAMIE.md (1)
195-196: Nice documentation update for JSC-77 enforcement details.These entries clearly capture the policy gate, focus-scope contract, and follow-up hardening in one place.
| console.error( | ||
| error instanceof Error ? error.message : `${SERVICE_LOG_TAG} ${String(error)}`, | ||
| ); |
There was a problem hiding this comment.
Prefix all caught error messages with service identity.
error.message is emitted raw for Error instances, so some failures (for example ENOENT) will miss the required service tag.
🔧 Proposed fix
} catch (error) {
result = { status: 1 };
+ const message = error instanceof Error ? error.message : String(error);
console.error(
- error instanceof Error ? error.message : `${SERVICE_LOG_TAG} ${String(error)}`,
+ message.includes(SERVICE_LOG_TAG) ? message : `${SERVICE_LOG_TAG} ${message}`,
);
}As per coding guidelines **/*.{js,ts,tsx,jsx,py,rs,mjs}: Apps/binaries/services MUST include service identity in outputs, error messages, and logs (service:"<service_name>").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/policy/run-exemplar-evaluation.mjs` around lines 159 - 161, The
caught-error logging currently emits Error instances raw (via error.message) and
can omit the required service identity tag; update the console.error call in
run-exemplar-evaluation.mjs to always prefix the message with SERVICE_LOG_TAG
and then the error text (e.g., use SERVICE_LOG_TAG + the error.message for Error
instances or String(error) otherwise), ensuring every emitted error includes the
service:"<service_name>" identity.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f46caa6dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .ds-focusable:focus-visible, | ||
| [data-ds-focusable]:focus-visible { |
There was a problem hiding this comment.
Scoping the shared ring to only .ds-focusable / [data-ds-focusable] removes keyboard-visible focus from existing controls that already set focus:outline-none but do not add a local focus-visible:ring-* (for example, packages/ui/src/app/chat/compose/ComposeInstructionsPanel/ComposeInstructionsPanel.tsx textarea). Before this change those controls still received the global :focus-visible ring; after it, they can become effectively focus-invisible for keyboard users, which is an accessibility regression in current UI surfaces.
Useful? React with 👍 / 👎.
| /(^|[\s,{]):focus\s*:\s*not\s*\(\s*:focus-visible\s*\)\s*\{[^}]*outline\s*:\s*none/i.test( | ||
| enhancedCss, | ||
| ) || | ||
| /(^|[\s,{]):focus\s*\{[^}]*outline\s*:\s*none/i.test(enhancedCss) |
There was a problem hiding this comment.
The new bare-:focus guard only matches when :focus (or :focus:not(:focus-visible)) is immediately followed by {, so prohibited global selectors can bypass the precheck when they appear in comma lists such as :focus:not(:focus-visible), .x { outline: none; }. That leaves a policy hole where native outlines can be globally suppressed without failing test:exemplar-evaluation.
Useful? React with 👍 / 👎.
1 participant
Pull request checklist
Summary
Checklist
main; this PR is from a dedicated branch.codex/*for agent-created branches).bash scripts/validate-codestyle.sh,pnpm check,test -f memory.json && jq -e '.meta.version == "1.0" and (.preamble.bootstrap | type == "boolean") and (.preamble.search | type == "boolean") and (.entries | type == "array")' memory.json >/dev/null.Testing
pnpm validate:tokens;pnpm design-system-guidance:check:ci;pnpm generated-source:check;pnpm typecheck;pnpm test:policy;pnpm test:exemplar-evaluation:list;git diff --check;bash scripts/validate-codestyle.sh;pnpm check;test -f memory.json && jq -e '.meta.version == "1.0" and (.preamble.bootstrap | type == "boolean") and (.preamble.search | type == "boolean") and (.entries | type == "array")' memory.json >/dev/null;pnpm -C platforms/web/apps/web build;git push -u origin jscraik/feature/jsc-77-professional-finish-review-loops(pre-push hook)pnpm design-system-guidance:check:ciretained existing advisory warnings only.pnpm test:exemplar-evaluation:listretained a nonfatal sharp/libvips duplicate-class warning during web visual listing. The first sandboxed push attempt was blocked by macOS Chromium launch permissions; rerunning the same push outside the sandbox passed the pre-push hook.bash scripts/validate-codestyle.sh-> passpnpm check-> passtest -f memory.json && jq -e '.meta.version == "1.0" and (.preamble.bootstrap | type == "boolean") and (.preamble.search | type == "boolean") and (.entries | type == "array")' memory.json >/dev/null-> passpnpm validate:tokens-> pass;pnpm design-system-guidance:check:ci-> pass (existing warnings only);pnpm generated-source:check-> pass;pnpm typecheck-> pass;pnpm test:policy-> pass;pnpm test:exemplar-evaluation:list-> pass (nonfatal sharp/libvips duplicate-class warning during web visual listing);git diff --check-> pass;pnpm -C platforms/web/apps/web build-> pass;git push -u origin jscraik/feature/jsc-77-professional-finish-review-loops-> pass (pre-push hook passed)Review artifacts
semgrep:changedreported no changedsrc/**implementation files.ffa2b53a; Linear issue JSC-77.Notes
This PR turns JSC-77 into a concrete review loop: a documented professional-finish rubric, an enforced exemplar precheck, scoped focus-ring behavior, and local workspace resolution repairs so the visual and generated-source gates can exercise the right surfaces. The branch uses the Codex desktop configured
jscraik/feature/*prefix, while the repository template still namescodex/*for agent-created branches; the checklist keeps that policy mismatch visible for review rather than marking it as satisfied.