Repository navigation
feat: flatten workspace chrome and consolidate settings - #29
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR consolidates identity, agent connection, and theme controls into ChangesWorkspace UX consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor WorkspaceUser
participant WorkspaceSettings
participant SessionWorkspace
participant ConnectionModal
WorkspaceUser->>WorkspaceSettings: Open settings
WorkspaceUser->>WorkspaceSettings: Save name or select theme
WorkspaceSettings->>SessionWorkspace: Submit workspace changes
WorkspaceUser->>WorkspaceSettings: Connect agent
WorkspaceSettings->>SessionWorkspace: Start connection flow
SessionWorkspace->>ConnectionModal: Display status and agent count
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
apps/web/src/components/workspace-settings.tsx (2)
206-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider labelling the dialog with its heading.
The dialog sets
aria-label="Workspace settings"while it also renders<h2>Settings</h2>. Screen reader users hear two different names for the same surface. Give the heading an id and usearia-labelledbyto keep one name.🤖 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/web/src/components/workspace-settings.tsx` around lines 206 - 219, Update the dialog in the isOpen render block to remove the conflicting aria-label, assign a unique id to the Settings h2, and reference that id through aria-labelledby on the element with role="dialog".
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider importing the shared
ConnectionStatetype.
session-workspace.tsxalready defines aConnectionStateunion and passes it to this component. A local duplicate can drift if a new state is added. Import the shared type instead.🤖 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/web/src/components/workspace-settings.tsx` at line 8, Replace the local ConnectionState union in workspace-settings.tsx with an import of the shared ConnectionState type defined by session-workspace.tsx, and update the component references to use that imported type.e2e-workspace-ux-validate.ts (2)
154-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAwait the provider connection and reuse the app's endpoint derivation.
connectAgentPresencebuilds the WebSocket URL by rewriting the MCP URL protocol and forcingpathname = '/ws'. This duplicates the app's own endpoint logic. If the server path changes, the helper connects to a dead endpoint and the only symptom is the 15-secondwaitForFunctiontimeout at line 536.Wait for the provider
statusevent to reportconnectedbefore returning, so a bad endpoint fails with a clear message.🤖 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 `@e2e-workspace-ux-validate.ts` around lines 154 - 173, Update connectAgentPresence to derive the WebSocket endpoint using the app’s existing endpoint-resolution logic instead of manually rewriting the MCP URL and forcing /ws. Make the helper await the WebsocketProvider status event until it reports connected before returning, and surface a clear failure if the connection cannot be established.
692-692: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse locator-based waits for the selection checks.
Line 692 and 716 pass captured
ElementHandles after.hover()/.click()that can replace the React Flow elements. Use the existing locators withexpect(...).toHaveClass(...), or poll a selector string insidewaitForFunction, to avoid stale-handle failures.🤖 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 `@e2e-workspace-ux-validate.ts` at line 692, Update the selection checks around the visible node interactions to use the existing locator rather than an ElementHandle captured from visibleNode.elementHandle(). Replace the waitForFunction calls after hover/click with locator-based expect(...).toHaveClass(...) assertions or selector polling, preserving the is-selected requirement while avoiding stale React Flow element handles.apps/web/src/components/session-workspace.tsx (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing one focusable-element selector.
workspace-settings.tsxdefinesFOCUSABLE_SELECTORwith radio and hidden-input handling. This file defines a second, less strict selector for the same purpose. Extract one constant into a shared module so both focus traps stay consistent.🤖 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/web/src/components/session-workspace.tsx` at line 65, Extract the focusable-element selector into a shared module, using the stricter selector from workspace-settings.tsx that includes radio and hidden-input handling. Update MODAL_FOCUSABLE_SELECTOR in session-workspace.tsx and FOCUSABLE_SELECTOR in workspace-settings.tsx to import and reuse that shared constant so both focus traps remain consistent.e2e/support/workspace.ts (1)
112-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse this exported helper instead of duplicating it.
e2e-workspace-ux-validate.tsdefines its ownopenWorkspaceSettingsat lines 129-137 with the same body. That file already importsselectWorkspaceThemefrom this module, andselectWorkspaceThemecalls this exported helper. Two implementations of the same interaction can drift apart.Import this helper in
e2e-workspace-ux-validate.tsand delete the local copy. Consider exportingcloseWorkspaceSettingshere as well.🤖 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 `@e2e/support/workspace.ts` around lines 112 - 120, Reuse the exported openWorkspaceSettings helper from workspace.ts in e2e-workspace-ux-validate.ts: add it to the existing import, remove that file’s local duplicate implementation, and preserve all current call sites. Export closeWorkspaceSettings from workspace.ts as well if the validation flow currently needs the same shared interaction.apps/web/src/app/globals.css (1)
1044-1046: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the React Flow v12 CSS variable variants and drop the unused Controls variable.
React Flow v12 uses the
-defaultvariants (--xy-controls-box-shadow-default,--xy-node-boxshadow-hover-default,--xy-node-boxshadow-selected-default), sovar(--xy-*)does resolve to a value.diagram-canvas.tsxdoes not render React Flow<Controls>, so remove the Controls override to avoid dead CSS.🤖 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/web/src/app/globals.css` around lines 1044 - 1046, Update the React Flow CSS variable declarations by removing the unused --xy-controls-box-shadow override and renaming the hover and selected node variables to their v12 -default variants: --xy-node-boxshadow-hover-default and --xy-node-boxshadow-selected-default.
🤖 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/web/src/app/globals.css`:
- Around line 487-497: Update the max-width 720px media query so
.workspace-settings-close, .workspace-settings-actions button, and
.workspace-settings-connect receive a 44px minimum height on phone viewports,
matching the existing .workspace-settings-trigger touch-target sizing while
preserving their other styles.
In `@apps/web/src/components/session-workspace.tsx`:
- Around line 959-971: In apps/web/src/components/session-workspace.tsx:959-971,
add display-name state, update it in saveDisplayName and in the collaboration
effect that assigns currentIdentityRef.current, and pass that state to
WorkspaceSettings instead of reading the ref during render. In
apps/web/src/components/workspace-settings.tsx:130-137, separate the
isOpen-driven draft initialization and autofocus effect from the
keydown/pointerdown listener effect, keeping the listeners dependent on [close,
isOpen] so display-name changes do not reset drafts or steal focus.
In `@e2e-workspace-ux-validate.ts`:
- Around line 147-152: Update selectThemePreference so the system preference
resolves the expected theme from the page’s emulated color scheme, while
explicit dark and light preferences retain their direct mappings. Use the
resolved value when waiting for the html[data-theme] locator, ensuring system
mode works under dark emulation.
- Around line 546-549: Fix the cleanup wait in the finally block by ensuring it
observes a mounted agent-status element: reopen the settings dialog before
waitForFunction, or switch the predicate to the persistent presence-bar status
element. Preserve the requirement that the wait resolves only after “MCP agent
working” is no longer present.
---
Nitpick comments:
In `@apps/web/src/app/globals.css`:
- Around line 1044-1046: Update the React Flow CSS variable declarations by
removing the unused --xy-controls-box-shadow override and renaming the hover and
selected node variables to their v12 -default variants:
--xy-node-boxshadow-hover-default and --xy-node-boxshadow-selected-default.
In `@apps/web/src/components/session-workspace.tsx`:
- Line 65: Extract the focusable-element selector into a shared module, using
the stricter selector from workspace-settings.tsx that includes radio and
hidden-input handling. Update MODAL_FOCUSABLE_SELECTOR in session-workspace.tsx
and FOCUSABLE_SELECTOR in workspace-settings.tsx to import and reuse that shared
constant so both focus traps remain consistent.
In `@apps/web/src/components/workspace-settings.tsx`:
- Around line 206-219: Update the dialog in the isOpen render block to remove
the conflicting aria-label, assign a unique id to the Settings h2, and reference
that id through aria-labelledby on the element with role="dialog".
- Line 8: Replace the local ConnectionState union in workspace-settings.tsx with
an import of the shared ConnectionState type defined by session-workspace.tsx,
and update the component references to use that imported type.
In `@e2e-workspace-ux-validate.ts`:
- Around line 154-173: Update connectAgentPresence to derive the WebSocket
endpoint using the app’s existing endpoint-resolution logic instead of manually
rewriting the MCP URL and forcing /ws. Make the helper await the
WebsocketProvider status event until it reports connected before returning, and
surface a clear failure if the connection cannot be established.
- Line 692: Update the selection checks around the visible node interactions to
use the existing locator rather than an ElementHandle captured from
visibleNode.elementHandle(). Replace the waitForFunction calls after hover/click
with locator-based expect(...).toHaveClass(...) assertions or selector polling,
preserving the is-selected requirement while avoiding stale React Flow element
handles.
In `@e2e/support/workspace.ts`:
- Around line 112-120: Reuse the exported openWorkspaceSettings helper from
workspace.ts in e2e-workspace-ux-validate.ts: add it to the existing import,
remove that file’s local duplicate implementation, and preserve all current call
sites. Export closeWorkspaceSettings from workspace.ts as well if the validation
flow currently needs the same shared interaction.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc57721e-fab5-4521-ac86-ed049eed83dd
📒 Files selected for processing (9)
apps/web/src/app/globals.cssapps/web/src/components/diagram-canvas.tsxapps/web/src/components/session-workspace.tsxapps/web/src/components/theme-control.tsxapps/web/src/components/workspace-settings.test.tsapps/web/src/components/workspace-settings.tsxapps/web/src/lib/session.test.tse2e-workspace-ux-validate.tse2e/support/workspace.ts
💤 Files with no reviewable changes (1)
- apps/web/src/components/theme-control.tsx
This branch was successfully deployed
1 participant
Closes #28
What changed
Architecture
This slice reuses the existing ThemeProvider, Awareness identity, and agent-connection flow. Settings is presentation/composition only: it does not add Yjs state, protocol fields, or a second source of truth. Diagram renaming remains tab-scoped, and source/activity flyout ownership remains unchanged.
Verification
git diff --checkKnown follow-up
At the late populated mobile state, the nominal 320 px screenshot is still 339 px wide because the footer/presence region can overflow. Settings itself remains contained and usable. The exact reproduction and acceptance gate are tracked in #18 rather than being misreported as fixed here.
Summary by CodeRabbit
New Features
Style
Tests