Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds centralized CUSTOM_EVENTS and STORAGE_KEYS constants, a new withToolSync plugin that emits TOOL_CHANGE on pointer-type transitions, integrates the plugin into the canvas and tests, replaces hardcoded storage/event strings, refactors toolbar buttons to per-tool Buttons with SELECTED_BUTTON_CLASS, and updates styles and tests. Changes
Sequence DiagramsequenceDiagram
actor User
participant Toolbar as BoardToolbar
participant Canvas as BoardCanvas
participant Transforms as BoardTransforms
participant Plugin as with-tool-sync
participant Window as window
User->>Toolbar: pointer down on tool button
Toolbar->>Canvas: handleToolChange(toolId)
Canvas->>Transforms: updatePointerType(newType)
Transforms->>Plugin: (patched) intercept updatePointerType
alt pointer type changed
Plugin->>Plugin: update WeakMap(prevPointer)
Plugin->>Window: dispatchEvent(CUSTOM_EVENTS.TOOL_CHANGE)
end
Window->>Window: notify listeners (use-board-state, UI)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
| Metric | Coverage |
|---|---|
| Lines | 0% |
| Functions | 93.3% |
| Branches | 78.86% |
| Statements | 91.26% |
| Average | 65.9% |
How to view coverage report
- Download the coverage artifact from the link above
- Extract the ZIP file
- Open
index.htmlin your browser
Commit: 93963854d16ee1fd27d9d39763824351f7e08656
![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
🧹 Nitpick comments (3)
features/toolbar/components/BoardToolbar.tsx (3)
130-141: Consider addingaria-pressedfor accessibility consistency.Other tool buttons have
aria-pressedto indicate active state, but the shapes dropdown trigger is missing this attribute. While it's a dropdown trigger, it still represents an active tool state when a shape is selected.♿ Suggested accessibility enhancement
<Button variant="ghost" size="icon" className={`${BUTTON_CLASS} ${buttonSizeClass} flex items-center justify-center ${isShapeActive ? SELECTED_BUTTON_CLASS : ''}`} aria-label="Shapes" + aria-pressed={isShapeActive} data-testid="shapes-dropdown-trigger" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/toolbar/components/BoardToolbar.tsx` around lines 130 - 141, Add an aria-pressed attribute to the shapes dropdown trigger Button so screen readers can detect the active tool state: update the DropdownMenuTrigger/Button (symbols: DropdownMenuTrigger, Button, BUTTON_CLASS, buttonSizeClass) to include aria-pressed={isShapeActive} (ensuring it reflects the isShapeActive boolean and remains consistent with SELECTED_BUTTON_CLASS usage and data-testid="shapes-dropdown-trigger").
232-245: Addaria-labelandaria-pressedfor accessibility.The handdrawn toggle button is missing
aria-labelandaria-pressedattributes, unlike other toolbar buttons.♿ Suggested accessibility enhancement
<Button variant="ghost" size="icon" className={`${BUTTON_CLASS} ${buttonSizeClass} flex items-center justify-center ${handdrawn ? SELECTED_BUTTON_CLASS : ''}`} + aria-label={handdrawn ? 'Disable Handdrawn Mode' : 'Enable Handdrawn Mode'} + aria-pressed={handdrawn} onPointerDown={(e: React.PointerEvent) => { e.preventDefault(); e.stopPropagation(); toggleHanddrawn(); }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/toolbar/components/BoardToolbar.tsx` around lines 232 - 245, The handdrawn toggle Button (the Button element wrapping HANDRAWN_ICON) lacks accessibility attributes; add an appropriate aria-label (e.g., "Toggle hand-drawn mode") and aria-pressed bound to the handdrawn state (aria-pressed={handdrawn}) on that Button so screen readers recognize it as a toggle, keeping the existing onPointerDown handler and classes; follow the same attribute pattern used by other toolbar buttons in this component (see Button, toggleHanddrawn, handdrawn, HANDRAWN_ICON).
264-306: Addaria-labelto selection action buttons.The Duplicate and Delete buttons are missing
aria-labelattributes for screen reader accessibility.♿ Suggested accessibility enhancement
<Button variant="ghost" size="icon" className={`${BUTTON_CLASS} ${buttonSizeClass} flex items-center justify-center`} + aria-label="Duplicate" onPointerDown={(e: React.PointerEvent) => {<Button variant="ghost" size="icon" className={`${BUTTON_CLASS} ${buttonSizeClass} flex items-center justify-center hover:bg-destructive/10 hover:text-destructive`} + aria-label="Delete" onPointerDown={(e: React.PointerEvent) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/toolbar/components/BoardToolbar.tsx` around lines 264 - 306, The Duplicate and Delete icon-only action Buttons in BoardToolbar (the Button elements wrapping Copy and Trash2 icons) lack aria-labels; add aria-label="Duplicate" to the Button that calls duplicateElements(board) and aria-label="Delete" to the Button that calls deleteFragment(board) so screen readers can announce the action, keeping the existing onPointerDown handlers and Tooltip/TooltipTrigger structure intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/board/plugins/with-sticky-note.ts`:
- Around line 11-12: Remove the duplicate CUSTOM_EVENTS.TOOL_CHANGE emission
that occurs after sticky-note completion: locate the sticky-note completion
logic in with-sticky-note (the code that switches pointer via
BoardTransforms.updatePointerType(board, PlaitPointerType.selection) and then
dispatches CUSTOM_EVENTS.TOOL_CHANGE again) and delete that extra dispatch; rely
on BoardTransforms.updatePointerType (and the installed withToolSync) to emit
the TOOL_CHANGE event automatically. Ensure no other code in with-sticky-note
still manually dispatches CUSTOM_EVENTS.TOOL_CHANGE when calling
BoardTransforms.updatePointerType.
---
Nitpick comments:
In `@features/toolbar/components/BoardToolbar.tsx`:
- Around line 130-141: Add an aria-pressed attribute to the shapes dropdown
trigger Button so screen readers can detect the active tool state: update the
DropdownMenuTrigger/Button (symbols: DropdownMenuTrigger, Button, BUTTON_CLASS,
buttonSizeClass) to include aria-pressed={isShapeActive} (ensuring it reflects
the isShapeActive boolean and remains consistent with SELECTED_BUTTON_CLASS
usage and data-testid="shapes-dropdown-trigger").
- Around line 232-245: The handdrawn toggle Button (the Button element wrapping
HANDRAWN_ICON) lacks accessibility attributes; add an appropriate aria-label
(e.g., "Toggle hand-drawn mode") and aria-pressed bound to the handdrawn state
(aria-pressed={handdrawn}) on that Button so screen readers recognize it as a
toggle, keeping the existing onPointerDown handler and classes; follow the same
attribute pattern used by other toolbar buttons in this component (see Button,
toggleHanddrawn, handdrawn, HANDRAWN_ICON).
- Around line 264-306: The Duplicate and Delete icon-only action Buttons in
BoardToolbar (the Button elements wrapping Copy and Trash2 icons) lack
aria-labels; add aria-label="Duplicate" to the Button that calls
duplicateElements(board) and aria-label="Delete" to the Button that calls
deleteFragment(board) so screen readers can announce the action, keeping the
existing onPointerDown handlers and Tooltip/TooltipTrigger structure intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e6c3f2e-83d8-4604-bc6a-17c07f6153be
📒 Files selected for processing (14)
CLAUDE.mdfeatures/board/components/BoardCanvas.tsxfeatures/board/grid/grid-plugin.tsfeatures/board/hooks/use-board-state.tsxfeatures/board/plugins/with-sticky-note.tsfeatures/board/plugins/with-tool-sync.tsfeatures/toolbar/components/BoardToolbar.tsxshared/constants/events.tsshared/constants/index.tsshared/constants/storage.tsshared/constants/styles.tstests/components/board-toolbar-mobile.test.tsxtests/e2e/text-sticky.spec.tstests/unit/sticky-note.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/toolbar-selection-indicator.spec.ts`:
- Around line 75-79: The test currently exits without assertion when
imageButton.count() === 0; update the test to fail or be skipped explicitly
instead of silently passing: assert that imageButton.count() is greater than 0
(e.g., expect(await imageButton.count()).toBeGreaterThan(0)) before interacting,
or call test.skip/test.fixme under the same feature flag that hides the Image
tool; ensure you modify the block referencing imageButton and the surrounding
test so the absence of the Image button causes a deterministic skip or a failing
assertion rather than a silent pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 660951a3-7e22-4d5b-be62-c5066dc89154
📒 Files selected for processing (1)
tests/e2e/toolbar-selection-indicator.spec.ts
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 (2)
features/toolbar/components/BoardToolbar.tsx (2)
271-275:⚠️ Potential issue | 🟠 MajorDon’t execute duplicate/delete on press-down.
Running these mutations in
onPointerDownremoves the normal “press, drag away, release to cancel” behavior and makes accidental duplicate/delete much easier, especially on touch. Fire them ononClickinstead.Suggested fix
- onPointerDown={(e: React.PointerEvent) => { - e.preventDefault(); - e.stopPropagation(); - duplicateElements(board); - }} + onPointerDown={suppressBoardPointerDown} + onClick={() => duplicateElements(board)} ... - onPointerDown={(e: React.PointerEvent) => { - e.preventDefault(); - e.stopPropagation(); - deleteFragment(board); - }} + onPointerDown={suppressBoardPointerDown} + onClick={() => deleteFragment(board)}Also applies to: 294-298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/toolbar/components/BoardToolbar.tsx` around lines 271 - 275, The pointer handlers for the duplicate/delete actions in BoardToolbar.tsx are currently invoking duplicateElements(board) and deleteElements(board) inside onPointerDown, which prevents the normal drag-away-to-cancel UX; change those handlers to fire on onClick instead (replace onPointerDown with onClick for the duplicateElements and deleteElements invocations) and remove the preventDefault/stopPropagation calls from the pointer-down handlers so the actions only run on click/tap release; apply the same change to the other similar block (the duplicate/delete pair around the other handler region).
232-245:⚠️ Potential issue | 🟠 MajorAdd accessible name and pressed state to the handdrawn toggle.
This is an icon-only toggle, but it has no
aria-labeloraria-pressed. Screen readers currently can't tell what the control does or whether handdrawn mode is enabled.Suggested fix
<Button variant="ghost" size="icon" className={`${BUTTON_CLASS} ${buttonSizeClass} flex items-center justify-center ${handdrawn ? SELECTED_BUTTON_CLASS : ''}`} + aria-label={handdrawn ? 'Disable Handdrawn Mode' : 'Enable Handdrawn Mode'} + aria-pressed={handdrawn} onPointerDown={(e: React.PointerEvent) => { e.preventDefault(); e.stopPropagation(); toggleHanddrawn(); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/toolbar/components/BoardToolbar.tsx` around lines 232 - 245, The handdrawn icon-only Button lacks accessible labeling and state; update the Button (inside TooltipTrigger) to include an explicit accessible name and pressed state by adding an aria-label (e.g., aria-label="Toggle handdrawn mode" or a localized string) and aria-pressed={handdrawn} so screen readers know the control purpose and whether handdrawn mode is enabled; keep the existing onPointerDown/toggleHanddrawn handler and class toggles (BUTTON_CLASS, SELECTED_BUTTON_CLASS, iconSizeClass, HANDRAWN_ICON) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/toolbar/components/BoardToolbar.tsx`:
- Around line 61-65: The tool buttons currently change state inside the pointer
event handler (createToolHandler) which prevents keyboard activation; refactor
so that createToolHandler only prevents board pointer propagation (keep
e.preventDefault()/e.stopPropagation() on onPointerDown) but move the actual
call to handleToolChange(toolId) into an onClick handler attached to the same
button; update every tool button that uses createToolHandler (and the
occurrences flagged in the review) so pointer-down only suppresses board events
and onClick performs the tool state change.
---
Outside diff comments:
In `@features/toolbar/components/BoardToolbar.tsx`:
- Around line 271-275: The pointer handlers for the duplicate/delete actions in
BoardToolbar.tsx are currently invoking duplicateElements(board) and
deleteElements(board) inside onPointerDown, which prevents the normal
drag-away-to-cancel UX; change those handlers to fire on onClick instead
(replace onPointerDown with onClick for the duplicateElements and deleteElements
invocations) and remove the preventDefault/stopPropagation calls from the
pointer-down handlers so the actions only run on click/tap release; apply the
same change to the other similar block (the duplicate/delete pair around the
other handler region).
- Around line 232-245: The handdrawn icon-only Button lacks accessible labeling
and state; update the Button (inside TooltipTrigger) to include an explicit
accessible name and pressed state by adding an aria-label (e.g.,
aria-label="Toggle handdrawn mode" or a localized string) and
aria-pressed={handdrawn} so screen readers know the control purpose and whether
handdrawn mode is enabled; keep the existing onPointerDown/toggleHanddrawn
handler and class toggles (BUTTON_CLASS, SELECTED_BUTTON_CLASS, iconSizeClass,
HANDRAWN_ICON) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4516b281-0d15-49f8-b8e9-7f62c286009c
📒 Files selected for processing (2)
features/board/plugins/with-sticky-note.tsfeatures/toolbar/components/BoardToolbar.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- features/board/plugins/with-sticky-note.ts
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.
Summary by CodeRabbit
New Features
Style
Refactor
Tests