fix(workspace-shell): neutralize source header contract#2912
Conversation
📝 WalkthroughWalkthroughWorkspaceShell now emits a neutral header class. Ecosystem explorer integrations target the updated shell navigation and header structure, Kitchen Sink styling follows the new selectors, and translation review uses dynamic viewport sizing. ChangesWorkspace shell migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/kitchen-sink-react/src/app.cssESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
thought (non-blocking): Accessibility audit (advisory)The sharded axe audit is report-only while the baseline and runtime budget mature.
Shard 1 reportShard 2 reportShard 3 report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tooling/translation-review/src/App.tsx (1)
893-893: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: Remove redundant physical height class.
You've correctly implemented the logical height calculation
min-block-size: calc(100dvh - 40px);instyles.csson the.review-appclass. Adding the Tailwind physical utilitymin-h-[calc(100dvh-40px)]here applies a physicalmin-heightproperty concurrently, which is redundant and undermines the goal of the CSS logical-property audit.♻️ Proposed fix
- className="review-app min-h-[calc(100dvh-40px)]" + className="review-app"🤖 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 `@tooling/translation-review/src/App.tsx` at line 893, Remove the redundant Tailwind min-h utility from the review-app element’s className in App.tsx, leaving the existing review-app class so styles.css remains the sole source of the logical minimum block-size.
🤖 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 `@_shared/design/ecosystem-bar/ecosystem-bar.stack.js`:
- Around line 261-264: Eliminate the duplicated ecosystem-bar selector logic by
making _shared/design/ecosystem-bar/ecosystem-bar.stack.js the single source of
truth, configured through the build system or imported directly. Remove the
corresponding hand-copied selectors from
apps/docs/templates/harborline/public/main.js and
apps/kitchen-sink/wwwroot/js/ecosystem-bar.stack.js, ensuring both consumers
obtain the shared implementation.
---
Nitpick comments:
In `@tooling/translation-review/src/App.tsx`:
- Line 893: Remove the redundant Tailwind min-h utility from the review-app
element’s className in App.tsx, leaving the existing review-app class so
styles.css remains the sole source of the logical minimum block-size.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 1b4c9630-2101-4975-aa08-53b066422aed
⛔ Files ignored due to path filters (1)
_shared/design/ecosystem-bar/dist/ecosystem-bar.docfx.jsis excluded by!**/dist/**,!**/dist/**
📒 Files selected for processing (8)
_shared/design/ecosystem-bar/ecosystem-bar.stack.jsapps/docs/templates/harborline/public/main.jsapps/kitchen-sink-react/src/app.cssapps/kitchen-sink/wwwroot/js/ecosystem-bar.stack.jspackages/workspace-shell/src/WorkspaceShell.tsxpackages/workspace-shell/tests/WorkspaceShell.test.tsxtooling/translation-review/src/App.tsxtooling/translation-review/src/styles.css
| '.workspace-shell-source-header [data-shell-command="toggle-navigation"],' + | ||
| '.top-navbar [data-shell-command="toggle-navigation"],' + | ||
| '.ks-chrome button[aria-label="Collapse component navigation"],' + | ||
| '.ks-chrome button[aria-label="Expand component navigation"],' + | ||
| '.workspace-shell-source-header button[aria-label="Collapse component navigation"],' + | ||
| '.workspace-shell-source-header button[aria-label="Expand component navigation"],' + |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
issue (blocking): Hand-parallel duplication of single-source logic.
As per path instructions, we must avoid hand-parallel duplicate copies of single-source things (A4). The exact same ecosystem-bar selector updates have been manually copy-pasted across the shared stack file and both consumer applications.
_shared/design/ecosystem-bar/ecosystem-bar.stack.js#L261-L264: Prevent manual duplication here by configuring the build system to copy this single source of truth, or by importing it directly.apps/docs/templates/harborline/public/main.js#L861-L864: Remove this hand-parallel copy.apps/kitchen-sink/wwwroot/js/ecosystem-bar.stack.js#L261-L264: Remove this hand-parallel copy.
📍 Affects 3 files
_shared/design/ecosystem-bar/ecosystem-bar.stack.js#L261-L264(this comment)apps/docs/templates/harborline/public/main.js#L861-L864apps/kitchen-sink/wwwroot/js/ecosystem-bar.stack.js#L261-L264
🤖 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 `@_shared/design/ecosystem-bar/ecosystem-bar.stack.js` around lines 261 - 264,
Eliminate the duplicated ecosystem-bar selector logic by making
_shared/design/ecosystem-bar/ecosystem-bar.stack.js the single source of truth,
configured through the build system or imported directly. Remove the
corresponding hand-copied selectors from
apps/docs/templates/harborline/public/main.js and
apps/kitchen-sink/wwwroot/js/ecosystem-bar.stack.js, ensuring both consumers
obtain the shared implementation.
Source: Path instructions
|
Immutable-head receipt — reviewed Focused verification passed on the exact head:
Mac browser matrix attempted with installed Google Chrome for 1440x900 and 390x844, light/dark axe, viewport-height, screenshots, and keyboard focus order. This managed host aborts Chrome immediately after launch under its sandbox, before a page/context can be created ( |
ctwoodwa
left a comment
ctwoodwa
left a comment
There was a problem hiding this comment.
CHANGES — exact head b80707cef3d891ecdfbca1c07c2a2bb66bfe7c69
issue (blocking): tooling/translation-review/src/App.tsx:893 adds min-h-[calc(100dvh-40px)] on the same frame where styles.css:42 already sets min-block-size: calc(100dvh - 40px). This creates duplicate physical/logical sizing authorities and fails card 2755's explicit no-logical-property-drift condition. The required CSS logical-properties gate runs CSS-only, so its green result does not cover this JSX utility. Remove the physical Tailwind utility and leave className="review-app" so the logical declaration is the single source.
All other adjudication evidence passed at this head: WorkspaceShell 8/8; Translation Review 18/18 including axe; WorkspaceShell typecheck; Translation Review and Gallery production builds; ecosystem-bar generated no-diff; token, logical-CSS, and breakpoint gates; and a sandboxed Chromium sweep completing 512/512 Gallery light/dark axe scans with zero route or scan errors. Exact-head CI is fully green. The neutral source-header selectors and the 40px + calc(100dvh - 40px) runtime viewport math are correct.
Leave this PR open and unarmed pending the logical-property correction.
|
Correction pushed at exact head
Please run fresh Layer-0 and UX review at this head. The pre-existing unresolved CodeRabbit |
1 participant
Summary
dvhframe height to Gallery and Translate.Verification
pnpm --filter @shipyard/workspace-shell test -- WorkspaceShell.test.tsxpnpm --filter @shipyard/translation-review test(includes axe coverage)pnpm --filter @shipyard/translation-review buildpnpm check:token-discipline:ui-react:cipnpm audit:css-logical:cipnpm check:breakpoint-discipline:ui-reactNotes
tsc6binary.Closes #2755
Summary by CodeRabbit
Bug Fixes
Refactor