feat(glass-office): registry-driven tour, mobile shell law, and a11y polish - #119
Merged
Merged
Conversation
…polish
Final P6 cross-floor polish slice. No new navigation mechanism, no
fabricated backend facts; every change extends the resolved elevator
registry, existing controllers, and shared shell.
Guided tour
- Replace legacy tab-button GUIDED_TOUR_STEPS with buildGuidedTourSteps,
a pure composer over the resolved elevator registry: one owner-language
stop per floor (by stable room id, default-room fallback), then the
Help/Docs, Config, and command-palette shell stops. Hidden or
capability-disabled floors and rooms can never become stops.
- Anchor each floor section with data-tour-id="floor-<id>"; the overlay
spotlights the real section, walks by stable room id (fails closed on
unknown ids), shows honest recovery copy when a stop is off/hidden
(zero-rect aware), traps Tab, and restores focus to the launcher (or an
equivalent visible launcher when the invoker unmounted mid-tour).
Quick guides
- Rewrite the Office ("Your desk") and Window quick guides in plain,
glance-able owner language. Help/Docs, Config, Setup, and conventional
room navigation remain the direct path; the tour is never the only way.
390px mobile shell law
- Fit the elevator to the 52px rail (lamp + room marks, no overflow past
the viewport). The Office becomes a stacked scrollable feed with Needs
You first without mutating the stored arrangement (arrange mode shows
true order); the fixed desktop 6x4 canvas is unchanged. Reef and Office
Chatter summarize before disclosure at narrow widths via one shared
useNarrowViewport hook.
Accessibility
- Add useDialogFocus (focus-in, Tab trap, exact-invoker restore) and apply
it to Settings and the command palette, both now role="dialog"
aria-modal labeled. aria-current marks the active room; the posture pill
is an announced status; rail/topbar actions have accessible names; the
live-feed scroll respects prefers-reduced-motion.
Proof
- New e2e/p6-final-polish.spec.ts: elevator tour walk, launcher focus
restore, reduced-motion, hidden-target honesty, 390px unclipped chrome
and mark/badge non-overlap, stacked Office feed, Window summaries, 200%
zoom, editable-field keyboard guard, and long-copy incident band, all
with exact console/page/request failure accounting. Rewrite the core
tour spec to the elevator model. New unit suites for buildGuidedTourSteps,
GuidedTourOverlay, useDialogFocus, mobile Office ordering, and Window
summaries.
Local gates: typecheck, zero-warning lint, unit 840/840, build, core E2E
49/49, ExecAss generated contract --check, independent validator, and
git diff --check all pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR rebuilds guided tours from resolved elevator floors, adds accessible focus management for dialogs and tours, introduces responsive mobile shell behavior, updates help copy and styling, and adds mounted and browser-based validation for geometry, keyboard behavior, hostile content, and incident recovery. ChangesMission Control final polish
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant MissionControl
participant ElevatorRegistry
participant GuidedTourOverlay
Browser->>MissionControl: start guided tour
MissionControl->>ElevatorRegistry: read resolved floors
ElevatorRegistry-->>MissionControl: return floors and rooms
MissionControl->>GuidedTourOverlay: render generated step
GuidedTourOverlay->>MissionControl: select stable room target
MissionControl-->>Browser: update spotlight and progress
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (3)
apps/mission-control/e2e/p6-final-polish.spec.ts (2)
189-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
expectWithinViewportdoesn't check the bottom edge.Only left/top/right are asserted, so an element extending below the fold passes. If that's intentional (vertical scrolling is expected), a brief comment or a name like
expectHorizontallyWithinViewportwould prevent the helper from being read as full containment proof.🤖 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 `@apps/mission-control/e2e/p6-final-polish.spec.ts` around lines 189 - 195, Update expectWithinViewport to assert that box.y + box.height does not exceed viewport!.height + 0.5, completing the viewport containment check. If the helper is intentionally limited to horizontal containment because vertical scrolling is expected, rename it to expectHorizontallyWithinViewport instead.
481-483: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
pressSequentially()instead oftype()here.locator.type()is deprecated in Playwright, and this test needs per-character input semantics.🤖 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 `@apps/mission-control/e2e/p6-final-polish.spec.ts` around lines 481 - 483, Replace the deprecated type() call on askInput in the input interaction flow with pressSequentially(), preserving the "24b" value and per-character input behavior.apps/mission-control/src/App.tsx (1)
879-891: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard against redundant re-navigation on unrelated re-renders.
guidedTourStepsis recomputed viauseMemo(..., [elevatorFloors]); ifelevatorFloors(returned byuseResolvedElevator, not shown here) isn't itself memoized/stable across unrelated re-renders, this effect will re-fireselectAvailableRoom(step.roomId)on every such render even though the current step hasn't changed — potentially re-triggering room-selection side effects repeatedly while the tour sits on one step.A cheap guard using the already-computed
resolvedActiveRoomIdwould make this effect resilient regardless of upstream memoization:♻️ Proposed guard
useEffect(() => { if (!guidedTourOpen) { return; } const step = guidedTourSteps[guidedTourStep]; - if (step?.roomId) { + if (step?.roomId && step.roomId !== resolvedActiveRoomId) { selectAvailableRoom(step.roomId); } - }, [guidedTourOpen, guidedTourStep, guidedTourSteps, selectAvailableRoom]); + }, [guidedTourOpen, guidedTourStep, guidedTourSteps, resolvedActiveRoomId, selectAvailableRoom]);Please confirm whether
useResolvedElevatormemoizes its output; if it already does, this guard is still a cheap, harmless safety net.🤖 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 `@apps/mission-control/src/App.tsx` around lines 879 - 891, Update the guided-tour navigation effect around guidedTourOpen and guidedTourStep to call selectAvailableRoom only when step.roomId differs from the already-resolved active room, using resolvedActiveRoomId as the guard. Preserve the existing behavior for missing room IDs and unavailable rooms, and include the active-room value in the effect dependencies as needed.
🤖 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 `@apps/mission-control/e2e/p6-final-polish.spec.ts`:
- Around line 311-320: Strengthen the focus validation in the `page.evaluate`
result and its assertions so the restored active element must be a connected,
keyboard-focusable element rather than `document.body`. Preserve the
detached-element check while explicitly rejecting body focus and validating
focusability.
- Around line 377-403: Update the block measurement loop around blockBoxes to
convert each boundingBox() result from viewport-relative to document-absolute
coordinates by incorporating the page scroll offsets, while preserving the
existing visibility and positive-size assertions. Use these absolute boxes for
the needs-you ordering and stacked-layout checks so scrolling between
measurements cannot affect comparisons.
In `@apps/mission-control/src/app/AppShell.test.tsx`:
- Around line 594-612: Update the AppShell test teardown to retain the rendered
React root and call root.unmount() inside act during afterEach before clearing
document.body, ensuring component effect cleanup runs. Preserve the existing
global stubbing cleanup after unmounting.
---
Nitpick comments:
In `@apps/mission-control/e2e/p6-final-polish.spec.ts`:
- Around line 189-195: Update expectWithinViewport to assert that box.y +
box.height does not exceed viewport!.height + 0.5, completing the viewport
containment check. If the helper is intentionally limited to horizontal
containment because vertical scrolling is expected, rename it to
expectHorizontallyWithinViewport instead.
- Around line 481-483: Replace the deprecated type() call on askInput in the
input interaction flow with pressSequentially(), preserving the "24b" value and
per-character input behavior.
In `@apps/mission-control/src/App.tsx`:
- Around line 879-891: Update the guided-tour navigation effect around
guidedTourOpen and guidedTourStep to call selectAvailableRoom only when
step.roomId differs from the already-resolved active room, using
resolvedActiveRoomId as the guard. Preserve the existing behavior for missing
room IDs and unavailable rooms, and include the active-room value in the effect
dependencies as needed.
🪄 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
Run ID: 0cc8e49b-8e29-407d-bf47-f68c27453142
📒 Files selected for processing (20)
apps/mission-control/e2e/core.spec.tsapps/mission-control/e2e/p6-final-polish.spec.tsapps/mission-control/src/App.tsxapps/mission-control/src/app/AppShell.test.tsxapps/mission-control/src/app/AppShell.tsxapps/mission-control/src/app/GuidedTourOverlay.test.tsxapps/mission-control/src/app/GuidedTourOverlay.tsxapps/mission-control/src/app/TabHelpBanner.tsxapps/mission-control/src/app/useDialogFocus.test.tsxapps/mission-control/src/app/useDialogFocus.tsapps/mission-control/src/app/useNarrowViewport.tsapps/mission-control/src/features/execassOffice/ExecassOfficePanel.test.tsxapps/mission-control/src/features/execassOffice/ExecassOfficePanel.tsxapps/mission-control/src/features/glassWindow/GlassWindowExperiential.test.tsxapps/mission-control/src/features/glassWindow/GlassWindowPage.tsxapps/mission-control/src/glass/guidedTour.test.tsapps/mission-control/src/glass/guidedTour.tsapps/mission-control/src/styles.cssapps/mission-control/src/ui/CommandPalette.tsxdocs/plans/2026-07-23-glass-office-claude-resume.md
Owner
Author
|
Hostile QA correction pushed once at 8ebd183. All three actionable review findings and all three nits were reproduced/classified and corrected; independent QA also found and fixed the closed-disclosure Settings focus escape and the narrow/zoom toast occlusion. Final local packet: typecheck PASS, zero-warning lint PASS, unit 841/841, build PASS, core E2E 50/50, both ExecAss contract checks PASS, diff-check PASS, contracts untouched. Fresh desktop/390/200%-zoom/partial-error artifacts were inspected. No manual CodeRabbit re-review was requested; this head is now in the ordinary incremental review and CI loop. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/mission-control/e2e/p6-final-polish.spec.ts (2)
665-672: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the topbar measurement out of the badge loop.
expectVisibleNonZeroRect(page, ".mc-topbar")re-runs its full visibility/hit-test probe on every badge. Measure once before the loop so every comparison uses one layout snapshot, matching the discipline used at Lines 477-479.♻️ Single measurement
+ const zoomTopbarWithIncident = await expectVisibleNonZeroRect( + page, + ".mc-topbar", + ); for (const badge of await zoomBadges.all()) { if (!(await badge.isVisible())) continue; const box = await badge.boundingBox(); expect(box).not.toBeNull(); - expectContained(box!, await expectVisibleNonZeroRect(page, ".mc-topbar")); + expectContained(box!, zoomTopbarWithIncident); expectNoOverlap(zoomTitleWithIncident, box!); await expectWithinViewport(page, box!); }🤖 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 `@apps/mission-control/e2e/p6-final-polish.spec.ts` around lines 665 - 672, Measure the visible non-zero rectangle for “.mc-topbar” once before the loop over zoomBadges, then reuse that stored rectangle in each expectContained call. Keep the badge visibility, bounding-box, overlap, and viewport assertions unchanged.
846-852: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExact-count poll only proves arrival, not stability.
expect.poll(...).toEqual({console: 1, requests: 1, responses: 1})resolves the moment the counters first match and returns immediately. A duplicate 503 or console error arriving a tick later is never observed, which undercuts the "exact failure accounting" claim in the test title. Re-assert the counters after the subsequent recovery step so overshoot is caught.♻️ Re-assert after recovery
await expect(partialProvider).toContainText("streaming"); + expect({ + console: injectedConsoleErrors, + requests: injectedRequests, + responses: injectedResponses, + }).toEqual({ console: 1, requests: 1, responses: 1 }); expect(unexpectedErrors).toEqual([]);🤖 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 `@apps/mission-control/e2e/p6-final-polish.spec.ts` around lines 846 - 852, Update the exact failure-count assertions in the test around the injectedConsoleErrors, injectedRequests, and injectedResponses poll: retain the initial poll for arrival, then re-assert the same exact counts after the subsequent recovery step so any later duplicate 503 or console error causes the test to fail.
🤖 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.
Nitpick comments:
In `@apps/mission-control/e2e/p6-final-polish.spec.ts`:
- Around line 665-672: Measure the visible non-zero rectangle for “.mc-topbar”
once before the loop over zoomBadges, then reuse that stored rectangle in each
expectContained call. Keep the badge visibility, bounding-box, overlap, and
viewport assertions unchanged.
- Around line 846-852: Update the exact failure-count assertions in the test
around the injectedConsoleErrors, injectedRequests, and injectedResponses poll:
retain the initial poll for arrival, then re-assert the same exact counts after
the subsequent recovery step so any later duplicate 503 or console error causes
the test to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dced79a4-ac88-4014-b8ab-912fc5ed9391
📒 Files selected for processing (8)
apps/mission-control/e2e/p6-final-polish.spec.tsapps/mission-control/src/App.tsxapps/mission-control/src/app/AppShell.test.tsxapps/mission-control/src/app/AppShell.tsxapps/mission-control/src/app/GuidedTourOverlay.tsxapps/mission-control/src/app/useDialogFocus.test.tsxapps/mission-control/src/app/useDialogFocus.tsapps/mission-control/src/styles.css
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/mission-control/src/app/useDialogFocus.test.tsx
- apps/mission-control/src/app/AppShell.test.tsx
- apps/mission-control/src/App.tsx
- apps/mission-control/src/app/GuidedTourOverlay.tsx
- apps/mission-control/src/app/useDialogFocus.ts
- apps/mission-control/src/app/AppShell.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Final P6 whole-building polish for Glass Office. This PR completes the registry-driven guided tour, plain-language quick guides, 390px shell/Office/Window behavior, focus and accessibility sweep, reduced motion, and zoom/long-copy/resilience proof. It extends the resolved elevator registry, existing controllers, and shared shell; it adds no backend authority, alternate navigation system, or fabricated facts.
Product behavior
prefers-reduced-motion, ignores elevator shortcuts in editable fields, and gives Settings, command palette, and tour real focus trap/restoration behavior.Hostile QA corrections
Security and authority boundary
Validation on final correction head
npm run typecheck— PASSnpm run lint -- --max-warnings=0— PASSnpm run test:unit -- --run— 841/841 PASSnpm run build— PASSnpm run test:e2e:core— 50/50 PASS in 7.5 minutescargo run -q -p carsinos-protocol --bin generate_execass_contract -- --check— PASSpython scripts/validate_execass_contract.py— PASSgit diff --check— PASSnode_modules/remains untracked.Regenerated desktop, 390px, 200%-zoom, and deliberate partial-error artifacts under
runtime/qa/p6-polish/were visually inspected. Checkpoint track:GLASS_OFFICE_P6_CROSS_FLOOR WORK.Summary by CodeRabbit