Repository navigation
style(wework): polish workbench chrome - #1223
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUI/spacing/typography tightened across composer controls, message rendering, sidebars, workspace panels, and dialogs; ProjectSection now always renders with task truncation and relative-time labels; several portals/menus moved to body with measured positioning; tests and locales updated to match these changes. ChangesUI styling, layout behavior, i18n, and tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wework/src/components/layout/workspace-panels/WorkspacePanelActions.tsx (1)
36-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t shrink these panel toggles below the mobile touch target.
Both workspace toggle buttons are now
32x32, which makes them miss the repo’s minimum mobile hit area. Keep them at leasth-11 min-w-[44px]so the panel controls stay usable on touch devices. As per coding guidelines, "All interactive elements on mobile must be at least 44px × 44px - useh-11 min-w-[44px]for buttons".🤖 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 `@wework/src/components/layout/workspace-panels/WorkspacePanelActions.tsx` around lines 36 - 56, The toggle buttons for bottom and right panels (look for the elements with data-testid "toggle-bottom-workspace-panel-button" and "toggle-right-workspace-panel-button", and handlers onToggleBottomPanel/onToggleRightPanel using icons PanelBottom and PanelRight) are too small; update their className to enforce the repo mobile hit area by replacing the current size utilities (e.g. "h-8 w-8") with "h-11 min-w-[44px]" while preserving existing centering, rounding, text and hover classes and the conditional bg/text utilities so the buttons remain visually identical but meet the 44×44px touch target requirement.frontend/src/features/projects/components/ProjectSection.tsx (1)
174-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe new 32px controls regress the mobile touch target across the whole section.
The create button, project expander, overflow trigger, new-conversation button, and show-more button were all tightened to
h-8/w-8. That puts several primary interactions below the repo’s 44px minimum touch size. Please keep these controls at leasth-11tall before merging. As per coding guidelines, "All interactive elements on mobile must be at least 44px × 44px - useh-11 min-w-[44px]for buttons".Also applies to: 325-332, 354-380, 453-458
🤖 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 `@frontend/src/features/projects/components/ProjectSection.tsx` around lines 174 - 183, Several interactive controls in ProjectSection (the create Button with data-testid based on isUnifiedSection/isWorkspaceSection, the project expander, overflow trigger, new-conversation button, and show-more button) were reduced to h-8/w-8 and violate the 44×44 mobile touch target rule; update these Button elements to use at least h-11 and min-w-[44px] (or the shared utility h-11 min-w-[44px]) instead of h-8/w-8 so they meet the "All interactive elements on mobile must be at least 44px × 44px" guideline, ensuring you replace className usages like "h-8 min-w-8 w-8" with "h-11 min-w-[44px]" (and remove fixed small widths) across the referenced Button instances.wework/src/components/common/TextInputDialog.tsx (1)
47-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the dialog controls at the mobile minimum and use the standard primary action.
These changes shrink the close button, input, cancel button, and confirm button to 32px-36px tall. That drops below the repo’s 44px mobile target, and the confirm action in this dialog still bypasses the required
variant="primary"styling. Please keep these controls at leasth-11tall and render the confirm action with the shared primary button treatment. As per coding guidelines, "All interactive elements on mobile must be at least 44px × 44px - useh-11 min-w-[44px]for buttons" and "Primary action buttons in dialogs MUST usevariant=\"primary\"to ensure visual consistency and proper styling".Also applies to: 60-65, 67-88
🤖 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 `@wework/src/components/common/TextInputDialog.tsx` around lines 47 - 52, The dialog controls are undersized and the confirm button doesn't use the shared primary styling; update the close button, cancel button, confirm button, and the input to meet mobile minimums by using className="h-11 min-w-[44px]" (or h-11 for inputs) and ensure the confirm action is rendered with the shared Button component using variant="primary" (e.g., where confirmLabel/handleConfirm/onConfirm is used). Locate the close button (onClose, cancelLabel, inputTestId) and the dialog action buttons in TextInputDialog.tsx and replace their current height classes (e.g., h-8, h-9, h-10) with h-11 min-w-[44px]; change the confirm button JSX to pass variant="primary" to the shared Button component so it uses the repo primary styling.
🧹 Nitpick comments (1)
frontend/src/__tests__/features/projects/components/project-sidebar-behavior.test.tsx (1)
178-212: ⚡ Quick winAdd one regression test for the new task cap and time label branches.
These updated assertions only cover the one-task path, so the newly added
DEFAULT_VISIBLE_PROJECT_TASKS,project-show-more-tasks-button, andrelativeTime.*rendering paths can still regress unnoticed. A fixture with 6+ tasks and stableupdated_atvalues would close that gap.🤖 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 `@frontend/src/__tests__/features/projects/components/project-sidebar-behavior.test.tsx` around lines 178 - 212, Add a regression test that renders ProjectSection with a project fixture containing at least 6 tasks (set deterministic updated_at timestamps) and a mock DEFAULT_VISIBLE_PROJECT_TASKS threshold to verify the "project-show-more-tasks-button" appears; assert only DEFAULT_VISIBLE_PROJECT_TASKS task rows are initially rendered, click the show-more button and assert the remaining tasks become visible, and verify relative time labels (selectors matching "relativeTime.*") render the expected stable strings for the provided updated_at values.
🤖 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 `@frontend/src/features/projects/components/ProjectSection.tsx`:
- Around line 166-171: The button with data-testid "project-section-toggle" in
ProjectSection.tsx is currently a focusable inert control with
aria-expanded={true}; either make it non-interactive markup if the section is
always open (replace the <button> with a heading or <span> and remove
aria-expanded), or restore proper toggle behavior by introducing component state
(e.g., isExpanded), set aria-expanded={isExpanded}, and wire an
onClick/onKeyDown handler to toggle that state (keep data-testid and accessible
label) so keyboard users can interact with the control.
---
Outside diff comments:
In `@frontend/src/features/projects/components/ProjectSection.tsx`:
- Around line 174-183: Several interactive controls in ProjectSection (the
create Button with data-testid based on isUnifiedSection/isWorkspaceSection, the
project expander, overflow trigger, new-conversation button, and show-more
button) were reduced to h-8/w-8 and violate the 44×44 mobile touch target rule;
update these Button elements to use at least h-11 and min-w-[44px] (or the
shared utility h-11 min-w-[44px]) instead of h-8/w-8 so they meet the "All
interactive elements on mobile must be at least 44px × 44px" guideline, ensuring
you replace className usages like "h-8 min-w-8 w-8" with "h-11 min-w-[44px]"
(and remove fixed small widths) across the referenced Button instances.
In `@wework/src/components/common/TextInputDialog.tsx`:
- Around line 47-52: The dialog controls are undersized and the confirm button
doesn't use the shared primary styling; update the close button, cancel button,
confirm button, and the input to meet mobile minimums by using className="h-11
min-w-[44px]" (or h-11 for inputs) and ensure the confirm action is rendered
with the shared Button component using variant="primary" (e.g., where
confirmLabel/handleConfirm/onConfirm is used). Locate the close button (onClose,
cancelLabel, inputTestId) and the dialog action buttons in TextInputDialog.tsx
and replace their current height classes (e.g., h-8, h-9, h-10) with h-11
min-w-[44px]; change the confirm button JSX to pass variant="primary" to the
shared Button component so it uses the repo primary styling.
In `@wework/src/components/layout/workspace-panels/WorkspacePanelActions.tsx`:
- Around line 36-56: The toggle buttons for bottom and right panels (look for
the elements with data-testid "toggle-bottom-workspace-panel-button" and
"toggle-right-workspace-panel-button", and handlers
onToggleBottomPanel/onToggleRightPanel using icons PanelBottom and PanelRight)
are too small; update their className to enforce the repo mobile hit area by
replacing the current size utilities (e.g. "h-8 w-8") with "h-11 min-w-[44px]"
while preserving existing centering, rounding, text and hover classes and the
conditional bg/text utilities so the buttons remain visually identical but meet
the 44×44px touch target requirement.
---
Nitpick comments:
In
`@frontend/src/__tests__/features/projects/components/project-sidebar-behavior.test.tsx`:
- Around line 178-212: Add a regression test that renders ProjectSection with a
project fixture containing at least 6 tasks (set deterministic updated_at
timestamps) and a mock DEFAULT_VISIBLE_PROJECT_TASKS threshold to verify the
"project-show-more-tasks-button" appears; assert only
DEFAULT_VISIBLE_PROJECT_TASKS task rows are initially rendered, click the
show-more button and assert the remaining tasks become visible, and verify
relative time labels (selectors matching "relativeTime.*") render the expected
stable strings for the provided updated_at values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ff03f73-fc41-4c50-bff3-563be913b0e6
📒 Files selected for processing (22)
frontend/src/__tests__/features/projects/components/project-sidebar-behavior.test.tsxfrontend/src/__tests__/features/tasks/components/sidebar/TaskSidebar.test.tsxfrontend/src/features/projects/components/ProjectSection.tsxfrontend/src/features/tasks/components/sidebar/TaskSidebar.tsxfrontend/src/i18n/locales/en/projects.jsonfrontend/src/i18n/locales/zh-CN/projects.jsonwework/src/components/chat/MessageList.tsxwework/src/components/chat/composer/AddContextMenu.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/chat/composer/ModelSelector.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/chat/composer/ProjectWorkBar.tsxwework/src/components/chat/composer/SkillSelector.tsxwework/src/components/common/ActionMenu.tsxwework/src/components/common/TextInputDialog.tsxwework/src/components/layout/DesktopSettingsMenu.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/EnvironmentInfoPopover.tsxwework/src/components/layout/workspace-panels/WorkspacePanelActions.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.tsxwework/src/components/projects/ProjectCreateDialog.tsx
✅ Files skipped from review due to trivial changes (11)
- wework/src/components/common/ActionMenu.tsx
- wework/src/components/chat/composer/ProjectWorkBar.tsx
- wework/src/components/layout/DesktopSettingsMenu.tsx
- wework/src/components/chat/composer/SkillSelector.tsx
- wework/src/components/layout/DesktopSidebar.tsx
- wework/src/components/chat/MessageList.tsx
- wework/src/components/chat/composer/ModelSelector.tsx
- wework/src/components/layout/EnvironmentInfoPopover.tsx
- wework/src/components/projects/ProjectCreateDialog.tsx
- wework/src/components/chat/composer/AddContextMenu.tsx
- wework/src/components/layout/workspace-panels/WorkspacePanelCards.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- wework/src/components/layout/DesktopWorkbenchMain.tsx
- wework/src/components/chat/composer/ComposerToolbar.tsx
Comment on lines
166
to
171
| <button | ||
| type="button" | ||
| data-testid="project-section-toggle" | ||
| onClick={() => setSectionCollapsed(!sectionCollapsed)} | ||
| aria-expanded={!sectionCollapsed} | ||
| className="flex h-full min-w-0 flex-1 items-center justify-between" | ||
| aria-expanded={true} | ||
| className="flex h-full min-w-0 flex-1 items-center" | ||
| > |
Contributor
There was a problem hiding this comment.
Replace the inert section-header button with non-interactive markup, or wire the toggle back up.
project-section-toggle is still a focusable <button>, but it no longer performs any action and aria-expanded is hardcoded to true. That leaves keyboard users on a dead control. If this section is intentionally always open, render a heading/span here instead of a button.
🤖 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 `@frontend/src/features/projects/components/ProjectSection.tsx` around lines
166 - 171, The button with data-testid "project-section-toggle" in
ProjectSection.tsx is currently a focusable inert control with
aria-expanded={true}; either make it non-interactive markup if the section is
always open (replace the <button> with a heading or <span> and remove
aria-expanded), or restore proper toggle behavior by introducing component state
(e.g., isExpanded), set aria-expanded={isExpanded}, and wire an
onClick/onKeyDown handler to toggle that state (keep data-testid and accessible
label) so keyboard users can interact with the control.
…031339 # Conflicts: # wework/src/components/chat/composer/ModelSelector.tsx # wework/src/components/chat/composer/ProjectWorkBar.tsx # wework/src/components/layout/DesktopSidebar.tsx # wework/src/components/projects/ProjectCreateDialog.tsx
Contributor
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 (1)
wework/src/components/projects/ProjectCreateDialog.tsx (1)
260-447:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd modal semantics and move focus into the portaled dialog.
Now that this mounts under
document.body, it behaves like a real modal overlay, but the dialog still has norole="dialog"/aria-modaland doesn't move focus inside on open. Keyboard and screen-reader users can stay behind the backdrop or tab back into the page.♿ Suggested starting point
- <div - data-testid="project-create-dialog" - className="w-full max-w-[560px] rounded-lg border border-[`#d8d8d8`] bg-white p-6 shadow-2xl" - > + <div + role="dialog" + aria-modal="true" + aria-labelledby="project-create-dialog-title" + data-testid="project-create-dialog" + className="w-full max-w-[560px] rounded-lg border border-[`#d8d8d8`] bg-white p-6 shadow-2xl" + > ... - <h2 className="text-base font-semibold text-[`#202124`]">{title}</h2> + <h2 id="project-create-dialog-title" className="text-base font-semibold text-[`#202124`]"> + {title} + </h2>🤖 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 `@wework/src/components/projects/ProjectCreateDialog.tsx` around lines 260 - 447, The dialog lacks modal semantics and focus management; add role="dialog" and aria-modal="true" to the element with data-testid="project-create-dialog" and give the title element a stable id (e.g. project-create-title) then set aria-labelledby on the dialog to that id; implement focus management by creating a ref for the dialog element (used in the createPortal JSX), add tabIndex={-1} to make it focusable, and in a useEffect when the dialog mounts (or when open) move focus into the dialog ref and save/restore the previously focused element on unmount/close (onClose) so keyboard/screen-reader users are trapped into the modal until closed.
🧹 Nitpick comments (1)
wework/src/components/layout/DesktopWorkbenchLayout.test.tsx (1)
471-525: 🏗️ Heavy liftSplit the new overlay/flyout cases into a dedicated spec file.
This test file is already over 1,300 lines, so adding more portal/layout coverage here makes the fixture harder to maintain. Please move these cases into a focused spec such as
DesktopWorkbenchLayout.overlays.test.tsx. As per coding guidelines, "File size limit: If a file exceeds 1000 lines, split it into multiple sub-modules".🤖 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 `@wework/src/components/layout/DesktopWorkbenchLayout.test.tsx` around lines 471 - 525, Move the two overlay/flyout tests ("renders project create menu as a body-level flyout to the right of the trigger" and "renders project create dialog as a page-level overlay") out of the large DesktopWorkbenchLayout test file into a new focused spec (e.g., DesktopWorkbenchLayout.overlays.test.tsx); copy the tests as-is but also copy/import any helpers they depend on (DesktopWorkbenchLayout component, baseProps, render, screen, userEvent, waitFor, createRect) and keep the getBoundingClientRect spy and its mockRestore in the first test, then delete the two tests from the original file so the large file is reduced while the portal/overlay assertions (test IDs: 'projects-create-button', 'projects-create-button-menu', 'project-create-dialog', 'project-existing-folder-button') remain covered in the new spec.
🤖 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 `@wework/src/components/common/ActionMenu.tsx`:
- Around line 160-165: The menu item's height is set to h-8 (32px) which
violates the 44px touch target; in the ActionMenu component update the className
for the menu row (the array building that currently includes 'flex h-8 w-full
items-center gap-2 px-3 text-left text-[13px] leading-[18px]') to use 'h-11' and
add 'min-w-[44px]' (i.e., replace 'h-8' with 'h-11 min-w-[44px]') while leaving
the danger conditional classes unchanged so all shared menu items meet mobile
tap-target guidelines.
- Around line 147-159: The pointer-selection guard pointerSelectionRef.current
can remain true if the item click unmounts the button before its click handler
runs; update the logic that closes the menu (e.g., inside handleItemSelect or
the callback that performs menu close/unmount) to set
pointerSelectionRef.current = false when an item action closes the menu so
future keyboard activations are not dropped; ensure the reset happens after
performing the item action but before or as part of the menu-unmount/close flow
so both onPointerDown and onClick paths are safe.
In `@wework/src/components/layout/DesktopWorkbenchLayout.test.tsx`:
- Around line 472-485: The getBoundingClientRect stub is too broad because
element.querySelector('[data-testid="projects-create-button"]') matches
ancestors; narrow it to only return the trigger rect for the actual anchor by
checking the element itself (e.g. use
element.matches('[data-testid="projects-create-button"]') or
element.dataset.testid === 'projects-create-button') inside the
vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation
callback so only the true trigger element (not its ancestors) returns the
createRect({ left:104, top:246, width:28, height:28 }); keep the existing
separate case for 'projects-create-button-menu' unchanged.
---
Outside diff comments:
In `@wework/src/components/projects/ProjectCreateDialog.tsx`:
- Around line 260-447: The dialog lacks modal semantics and focus management;
add role="dialog" and aria-modal="true" to the element with
data-testid="project-create-dialog" and give the title element a stable id (e.g.
project-create-title) then set aria-labelledby on the dialog to that id;
implement focus management by creating a ref for the dialog element (used in the
createPortal JSX), add tabIndex={-1} to make it focusable, and in a useEffect
when the dialog mounts (or when open) move focus into the dialog ref and
save/restore the previously focused element on unmount/close (onClose) so
keyboard/screen-reader users are trapped into the modal until closed.
---
Nitpick comments:
In `@wework/src/components/layout/DesktopWorkbenchLayout.test.tsx`:
- Around line 471-525: Move the two overlay/flyout tests ("renders project
create menu as a body-level flyout to the right of the trigger" and "renders
project create dialog as a page-level overlay") out of the large
DesktopWorkbenchLayout test file into a new focused spec (e.g.,
DesktopWorkbenchLayout.overlays.test.tsx); copy the tests as-is but also
copy/import any helpers they depend on (DesktopWorkbenchLayout component,
baseProps, render, screen, userEvent, waitFor, createRect) and keep the
getBoundingClientRect spy and its mockRestore in the first test, then delete the
two tests from the original file so the large file is reduced while the
portal/overlay assertions (test IDs: 'projects-create-button',
'projects-create-button-menu', 'project-create-dialog',
'project-existing-folder-button') remain covered in the new spec.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a23a7b25-72c0-446a-83eb-24bb35d73ab2
📒 Files selected for processing (3)
wework/src/components/common/ActionMenu.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/projects/ProjectCreateDialog.tsx
Comment on lines
+147
to
+159
| onPointerDown={event => { | ||
| event.preventDefault() | ||
| event.stopPropagation() | ||
| pointerSelectionRef.current = true | ||
| void handleItemSelect(item) | ||
| }} | ||
| onClick={() => { | ||
| if (pointerSelectionRef.current) { | ||
| pointerSelectionRef.current = false | ||
| return | ||
| } | ||
| void handleItemSelect(item) | ||
| }} |
Contributor
There was a problem hiding this comment.
Reset the pointer-selection guard when an item closes the menu.
Closing the menu on pointerdown can unmount the button before its click handler runs. When that happens, pointerSelectionRef.current stays true, and the next keyboard activation gets dropped once.
🔧 Proposed fix
const handleItemSelect = async (item: ActionMenuItem) => {
+ pointerSelectionRef.current = false
setOpen(false)
setMenuPosition(null)
await item.onSelect()
}🤖 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 `@wework/src/components/common/ActionMenu.tsx` around lines 147 - 159, The
pointer-selection guard pointerSelectionRef.current can remain true if the item
click unmounts the button before its click handler runs; update the logic that
closes the menu (e.g., inside handleItemSelect or the callback that performs
menu close/unmount) to set pointerSelectionRef.current = false when an item
action closes the menu so future keyboard activations are not dropped; ensure
the reset happens after performing the item action but before or as part of the
menu-unmount/close flow so both onPointerDown and onClick paths are safe.
Comment on lines
+160
to
+165
| className={[ | ||
| 'flex h-8 w-full items-center gap-2 px-3 text-left text-[13px] leading-[18px]', | ||
| item.danger | ||
| ? 'text-[#ff7b7b] hover:bg-white/10' | ||
| : 'text-white hover:bg-white/10', | ||
| ].join(' ')} |
Contributor
There was a problem hiding this comment.
Don't shrink shared menu items below the 44px touch target.
h-8 makes each action only 32px tall. Since this is a shared component, the new sizing regresses the mobile tap-target requirement and makes the menu harder to use on touch devices. As per coding guidelines, "All interactive elements on mobile must be at least 44px × 44px - use h-11 min-w-[44px] for buttons".
📏 Proposed fix
- 'flex h-8 w-full items-center gap-2 px-3 text-left text-[13px] leading-[18px]',
+ 'flex h-11 min-h-[44px] w-full items-center gap-2 px-3 text-left text-[13px] leading-[18px]',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| className={[ | |
| 'flex h-8 w-full items-center gap-2 px-3 text-left text-[13px] leading-[18px]', | |
| item.danger | |
| ? 'text-[#ff7b7b] hover:bg-white/10' | |
| : 'text-white hover:bg-white/10', | |
| ].join(' ')} | |
| className={[ | |
| 'flex h-11 min-h-[44px] w-full items-center gap-2 px-3 text-left text-[13px] leading-[18px]', | |
| item.danger | |
| ? 'text-[`#ff7b7b`] hover:bg-white/10' | |
| : 'text-white hover:bg-white/10', | |
| ].join(' ')} |
🤖 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 `@wework/src/components/common/ActionMenu.tsx` around lines 160 - 165, The menu
item's height is set to h-8 (32px) which violates the 44px touch target; in the
ActionMenu component update the className for the menu row (the array building
that currently includes 'flex h-8 w-full items-center gap-2 px-3 text-left
text-[13px] leading-[18px]') to use 'h-11' and add 'min-w-[44px]' (i.e., replace
'h-8' with 'h-11 min-w-[44px]') while leaving the danger conditional classes
unchanged so all shared menu items meet mobile tap-target guidelines.
Comment on lines
+472
to
+485
| const getBoundingClientRectSpy = vi | ||
| .spyOn(HTMLElement.prototype, 'getBoundingClientRect') | ||
| .mockImplementation(function () { | ||
| const element = this as HTMLElement | ||
|
|
||
| if (element.querySelector('[data-testid="projects-create-button"]')) { | ||
| return createRect({ left: 104, top: 246, width: 28, height: 28 }) | ||
| } | ||
|
|
||
| if (element.dataset.testid === 'projects-create-button-menu') { | ||
| return createRect({ left: 0, top: 0, width: 176, height: 76 }) | ||
| } | ||
|
|
||
| return createRect({ left: 0, top: 0, width: 0, height: 0 }) |
Contributor
There was a problem hiding this comment.
Narrow the getBoundingClientRect stub to the actual trigger anchor.
element.querySelector('[data-testid="projects-create-button"]') matches every ancestor that contains the button, not just the menu anchor. That can make unrelated layout measurements return the trigger rect and hide real positioning regressions.
🤖 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 `@wework/src/components/layout/DesktopWorkbenchLayout.test.tsx` around lines
472 - 485, The getBoundingClientRect stub is too broad because
element.querySelector('[data-testid="projects-create-button"]') matches
ancestors; narrow it to only return the trigger rect for the actual anchor by
checking the element itself (e.g. use
element.matches('[data-testid="projects-create-button"]') or
element.dataset.testid === 'projects-create-button') inside the
vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation
callback so only the true trigger element (not its ancestors) returns the
createRect({ left:104, top:246, width:28, height:28 }); keep the existing
separate case for 'projects-create-button-menu' unchanged.
…031339 # Conflicts: # frontend/src/features/tasks/components/sidebar/TaskSidebar.tsx
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/tasks/components/sidebar/TaskSidebar.tsx (1)
778-778:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix height calculation to match margin.
The margin was changed from
my-3(24px total) tomy-2(16px total), but the height calculation still subtracts 24px. This creates a mismatch that affects the sidebar's vertical sizing.📏 Proposed fix
- <div - className="hidden lg:flex lg:flex-col w-full h-full bg-base rounded-3xl shadow-sidebar my-2" - style={{ height: 'calc(100% - 24px)' }} + <div + className="hidden lg:flex lg:flex-col w-full h-full bg-base rounded-3xl shadow-sidebar my-2" + style={{ height: 'calc(100% - 16px)' }}🤖 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 `@frontend/src/features/tasks/components/sidebar/TaskSidebar.tsx` at line 778, The sidebar's vertical spacing was changed in TaskSidebar (className string in TaskSidebar.tsx) from my-3 to my-2 but the height calculation still subtracts 24px; update the height calculation used by TaskSidebar (wherever the inline style/CSS calc for height is defined in this component) to subtract 16px (total) to match my-2 so the sidebar height and margins align.
🤖 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.
Outside diff comments:
In `@frontend/src/features/tasks/components/sidebar/TaskSidebar.tsx`:
- Line 778: The sidebar's vertical spacing was changed in TaskSidebar (className
string in TaskSidebar.tsx) from my-3 to my-2 but the height calculation still
subtracts 24px; update the height calculation used by TaskSidebar (wherever the
inline style/CSS calc for height is defined in this component) to subtract 16px
(total) to match my-2 so the sidebar height and margins align.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebcfdbd3-3126-4d82-b3ef-f034bc13d263
📒 Files selected for processing (2)
frontend/src/__tests__/features/tasks/components/sidebar/TaskSidebar.test.tsxfrontend/src/features/tasks/components/sidebar/TaskSidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/tests/features/tasks/components/sidebar/TaskSidebar.test.tsx
qdaxb
approved these changes
Jun 2, 2026
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
2 participants
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.
Summary by CodeRabbit
New Features
Style
Tests