refactor: Introduce useBoardBackgroundSettings hook for managing boar… #192
Conversation
…d background settings with persistence - Refactored BoardBackgroundModal to utilize the new useBoardBackgroundSettings hook, improving code organization and reusability. - Updated methods for setting board background, card opacity, column opacity, and other settings to include server persistence. - Enhanced error handling and user feedback with toast notifications for successful and failed operations. - Added keyboard shortcut support for selecting folders in FileBrowserDialog, improving user experience. - Improved KanbanCard component layout and added dropdown menu for editing and viewing model information.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new hook that persists board background settings to the server, updates the background modal to use async persisted setters, adds a keyboard shortcut to the file browser dialog, and restructures Kanban card header/badge layout and spacing. Changes
Sequence DiagramsequenceDiagram
participant Modal as BoardBackgroundModal
participant Hook as useBoardBackgroundSettings
participant Store as Local Store
participant Client as HTTP Client
participant Server as Server
Modal->>Hook: call async setter (e.g., setCardOpacity / setBoardBackground)
Hook->>Store: update local state (immediate)
Hook->>Hook: assemble current settings (include imageVersionTimestamp when applicable)
Hook->>Client: POST persist settings
Client->>Server: request persist
Server-->>Client: response (200 / error)
alt success
Client-->>Hook: success
Hook-->>Modal: resolve, return
else failure
Client-->>Hook: error
Hook->>Modal: show toast error, log
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @webdevcody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors how board background settings are managed, centralizing their logic into a new custom hook that also handles automatic server persistence. Additionally, it enhances user experience by introducing a keyboard shortcut for folder selection in the file browser and improving the layout and accessibility of actions and model information within Kanban cards. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Pull Request Review: Board Background Settings Hook RefactorOverviewThis PR introduces a new ✅ Strengths1. Good Separation of ConcernsThe new hook ( 2. Consistent Async PatternsAll setting update functions are now properly async, which correctly reflects their side effects (server persistence). 3. Improved Error HandlingThe hook includes try-catch blocks with toast notifications for user feedback on save failures. 4. Enhanced UX Improvements
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a well-structured useBoardBackgroundSettings hook to centralize and persist board background settings, which is a great improvement for code organization and reusability. The refactoring of BoardBackgroundModal to use this hook is clean. I also appreciate the UX enhancements, such as the keyboard shortcut in the FileBrowserDialog and the layout consolidation in the KanbanCard component. My review includes a couple of suggestions for the new hook to further improve its maintainability by reducing code duplication. Overall, this is a solid refactoring.
| const setCardOpacity = useCallback( | ||
| async (projectPath: string, opacity: number) => { | ||
| const current = getCurrentSettings(projectPath); | ||
| store.setCardOpacity(projectPath, opacity); | ||
| await persistSettings(projectPath, { ...current, cardOpacity: opacity }); | ||
| }, | ||
| [store, persistSettings, getCurrentSettings] | ||
| ); | ||
|
|
||
| const setColumnOpacity = useCallback( | ||
| async (projectPath: string, opacity: number) => { | ||
| const current = getCurrentSettings(projectPath); | ||
| store.setColumnOpacity(projectPath, opacity); | ||
| await persistSettings(projectPath, { ...current, columnOpacity: opacity }); | ||
| }, | ||
| [store, persistSettings, getCurrentSettings] | ||
| ); | ||
|
|
||
| const setColumnBorderEnabled = useCallback( | ||
| async (projectPath: string, enabled: boolean) => { | ||
| const current = getCurrentSettings(projectPath); | ||
| store.setColumnBorderEnabled(projectPath, enabled); | ||
| await persistSettings(projectPath, { | ||
| ...current, | ||
| columnBorderEnabled: enabled, | ||
| }); | ||
| }, | ||
| [store, persistSettings, getCurrentSettings] | ||
| ); | ||
|
|
||
| const setCardGlassmorphism = useCallback( | ||
| async (projectPath: string, enabled: boolean) => { | ||
| const current = getCurrentSettings(projectPath); | ||
| store.setCardGlassmorphism(projectPath, enabled); | ||
| await persistSettings(projectPath, { | ||
| ...current, | ||
| cardGlassmorphism: enabled, | ||
| }); | ||
| }, | ||
| [store, persistSettings, getCurrentSettings] | ||
| ); | ||
|
|
||
| const setCardBorderEnabled = useCallback( | ||
| async (projectPath: string, enabled: boolean) => { | ||
| const current = getCurrentSettings(projectPath); | ||
| store.setCardBorderEnabled(projectPath, enabled); | ||
| await persistSettings(projectPath, { | ||
| ...current, | ||
| cardBorderEnabled: enabled, | ||
| }); | ||
| }, | ||
| [store, persistSettings, getCurrentSettings] | ||
| ); | ||
|
|
||
| const setCardBorderOpacity = useCallback( | ||
| async (projectPath: string, opacity: number) => { | ||
| const current = getCurrentSettings(projectPath); | ||
| store.setCardBorderOpacity(projectPath, opacity); | ||
| await persistSettings(projectPath, { | ||
| ...current, | ||
| cardBorderOpacity: opacity, | ||
| }); | ||
| }, | ||
| [store, persistSettings, getCurrentSettings] | ||
| ); | ||
|
|
||
| const setHideScrollbar = useCallback( | ||
| async (projectPath: string, hide: boolean) => { | ||
| const current = getCurrentSettings(projectPath); | ||
| store.setHideScrollbar(projectPath, hide); | ||
| await persistSettings(projectPath, { ...current, hideScrollbar: hide }); | ||
| }, | ||
| [store, persistSettings, getCurrentSettings] | ||
| ); |
There was a problem hiding this comment.
These setter functions are repetitive and can be simplified. The current implementation gets settings, updates the store, and then manually merges the new value for persistence. A cleaner pattern is to update the store first, then get the updated settings, and pass them directly to persistSettings. This removes redundancy and improves maintainability.
For example, setCardOpacity can be refactored as:
const setCardOpacity = useCallback(
async (projectPath: string, opacity: number) => {
store.setCardOpacity(projectPath, opacity);
const current = getCurrentSettings(projectPath);
await persistSettings(projectPath, current);
},
[store, persistSettings, getCurrentSettings]
);This pattern should be applied to all the setters in this block (setCardOpacity, setColumnOpacity, setColumnBorderEnabled, setCardGlassmorphism, setCardBorderEnabled, setCardBorderOpacity, setHideScrollbar) to improve maintainability and reduce code duplication.
| const clearBoardBackground = useCallback( | ||
| async (projectPath: string) => { | ||
| store.clearBoardBackground(projectPath); | ||
| // Clear the boardBackground settings | ||
| await persistSettings(projectPath, { | ||
| imagePath: null, | ||
| imageVersion: undefined, | ||
| cardOpacity: 100, | ||
| columnOpacity: 100, | ||
| columnBorderEnabled: true, | ||
| cardGlassmorphism: true, | ||
| cardBorderEnabled: true, | ||
| cardBorderOpacity: 100, | ||
| hideScrollbar: false, | ||
| }); | ||
| }, | ||
| [store, persistSettings] | ||
| ); |
There was a problem hiding this comment.
To improve maintainability, it's better to use the defaultBackgroundSettings constant from the app store instead of hardcoding default values. This ensures consistency if the defaults ever change. You'll need to add defaultBackgroundSettings to your import from @/store/app-store.
const clearBoardBackground = useCallback(
async (projectPath: string) => {
store.clearBoardBackground(projectPath);
// Clear the boardBackground settings by resetting to defaults
await persistSettings(projectPath, {
...defaultBackgroundSettings,
imageVersion: undefined,
});
},
[store, persistSettings]
);![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/components/kanban-card.tsx (1)
626-638: Add accessible label to dropdown trigger.Similar to the running task dropdown, this menu trigger also lacks an accessible label for screen reader users.
🔎 Proposed fix
<Button variant="ghost" size="sm" className="h-6 w-6 p-0 hover:bg-muted/80 rounded-md" onClick={(e) => e.stopPropagation()} onPointerDown={(e) => e.stopPropagation()} data-testid={`menu-${feature.id}`} + aria-label="Open task menu" > <MoreVertical className="w-3.5 h-3.5 text-muted-foreground" /> </Button>
🧹 Nitpick comments (3)
apps/ui/src/components/dialogs/file-browser-dialog.tsx (2)
119-121: Missing dependency inhandleSelectRecentcallback.
browseDirectoryis called inside the callback but not listed in the dependency array. This could cause stale closure issues ifbrowseDirectorywere to change (though currently it's defined as a plain async function, not memoized).Suggested fix
- const handleSelectRecent = useCallback((path: string) => { - browseDirectory(path); - }, []); + const handleSelectRecent = useCallback((path: string) => { + browseDirectory(path); + }, [browseDirectory]);Note: This would require wrapping
browseDirectoryinuseCallbackfirst to avoid unnecessary re-renders.
423-425: Consider usinguserAgentDatafor future-proofing.
navigator.platformis deprecated. While it still works, consider usingnavigator.userAgentData?.platformwith a fallback for better future compatibility. This is a minor concern and not blocking.apps/ui/src/hooks/use-board-background-settings.ts (1)
14-34: Optimistic updates without rollback on failure.The current pattern updates local state first, then persists to server. If persistence fails, local and server state will diverge until page refresh. Consider either:
- Returning success/failure from
persistSettingsso callers can rollback- Re-fetching settings on failure to sync state
This is acceptable for a settings UI where divergence is low-impact, but worth noting.
Optional: Return success status for potential rollback
const persistSettings = useCallback( - async (projectPath: string, settingsToUpdate: Record<string, unknown>) => { + async (projectPath: string, settingsToUpdate: Record<string, unknown>): Promise<boolean> => { try { const result = await httpClient.settings.updateProject( projectPath, { boardBackground: settingsToUpdate, } ); if (!result.success) { console.error("Failed to persist settings:", result.error); toast.error("Failed to save settings"); + return false; } + return true; } catch (error) { console.error("Failed to persist settings:", error); toast.error("Failed to save settings"); + return false; } }, [httpClient] );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/ui/src/components/dialogs/board-background-modal.tsx(6 hunks)apps/ui/src/components/dialogs/file-browser-dialog.tsx(2 hunks)apps/ui/src/components/views/board-view/components/kanban-card.tsx(3 hunks)apps/ui/src/hooks/use-board-background-settings.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/ui/src/hooks/use-board-background-settings.ts (1)
apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(1023-1028)
apps/ui/src/components/dialogs/board-background-modal.tsx (2)
apps/ui/src/store/app-store.ts (1)
useAppStore(853-2358)apps/ui/src/hooks/use-board-background-settings.ts (1)
useBoardBackgroundSettings(9-182)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (8)
apps/ui/src/components/views/board-view/components/kanban-card.tsx (1)
611-625: LGTM!The delete button positioning and event handling are well-implemented. The button is correctly placed in the top-right area with appropriate styling and accessibility attributes.
apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
211-235: Well-implemented keyboard shortcut with proper cleanup.The keyboard shortcut implementation is clean:
- Properly guarded by
open,currentPath, andloadingstates- Event listener cleanup on unmount/dependency change
handleSelectcorrectly memoized with its dependenciesapps/ui/src/hooks/use-board-background-settings.ts (2)
9-11: LGTM on hook initialization.The hook correctly uses the Zustand store and the singleton HTTP client pattern.
170-181: Clean API surface.The hook exposes a well-organized set of functions matching the store actions with added persistence. Good separation of concerns.
apps/ui/src/components/dialogs/board-background-modal.tsx (4)
39-50: Clean separation of store reads and hook writes.Good refactoring pattern: reading from the store directly (
currentProject,boardBackgroundByProject) while delegating writes to the persistence hook. This maintains reactive reads while adding server persistence.
231-246: Note:asyncon slider handlers has no effect on caller.The slider's
onValueChangedoesn't await the handler, so marking these asasyncprovides no benefit at the call site. Theawaitinside does ensure sequential execution within the handler, but errors won't propagate. This is fine, just noting for clarity.
141-144: Proper sequencing of server save and settings persistence.The flow correctly:
- Saves image to server via
httpClient.saveBoardBackground- Updates store and persists settings via
setBoardBackground- Shows success toast
This ensures the image exists before the settings reference it.
206-229: LGTM on clear background flow.The implementation correctly deletes the background from the server first, then clears local settings and persists the reset state.
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-6 w-6 p-0 hover:bg-muted/80 rounded-md" | ||
| onClick={(e) => e.stopPropagation()} | ||
| onPointerDown={(e) => e.stopPropagation()} | ||
| data-testid={`menu-running-${feature.id}`} | ||
| > | ||
| <MoreVertical className="w-3.5 h-3.5 text-muted-foreground" /> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent align="end" className="w-36"> | ||
| <DropdownMenuItem | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onEdit(); | ||
| }} | ||
| data-testid={`edit-running-${feature.id}`} | ||
| className="text-xs" | ||
| > | ||
| <Edit className="w-3 h-3 mr-2" /> | ||
| Edit | ||
| </DropdownMenuItem> | ||
| {/* Model info in dropdown */} | ||
| <div className="px-2 py-1.5 text-[10px] text-muted-foreground border-t mt-1 pt-1.5"> | ||
| <div className="flex items-center gap-1"> | ||
| <Cpu className="w-3 h-3" /> | ||
| <span>{formatModelName(feature.model ?? DEFAULT_MODEL)}</span> | ||
| </div> | ||
| </div> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> |
There was a problem hiding this comment.
Add accessible label to dropdown trigger.
The dropdown menu trigger button uses only a visual icon (MoreVertical) without an accessible label, which makes it difficult for screen reader users to understand the button's purpose.
🔎 Proposed fix
<Button
variant="ghost"
size="sm"
className="h-6 w-6 p-0 hover:bg-muted/80 rounded-md"
onClick={(e) => e.stopPropagation()}
onPointerDown={(e) => e.stopPropagation()}
data-testid={`menu-running-${feature.id}`}
+ aria-label="Open task menu"
>
<MoreVertical className="w-3.5 h-3.5 text-muted-foreground" />
</Button>📝 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.
| <DropdownMenu> | |
| <DropdownMenuTrigger asChild> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className="h-6 w-6 p-0 hover:bg-muted/80 rounded-md" | |
| onClick={(e) => e.stopPropagation()} | |
| onPointerDown={(e) => e.stopPropagation()} | |
| data-testid={`menu-running-${feature.id}`} | |
| > | |
| <MoreVertical className="w-3.5 h-3.5 text-muted-foreground" /> | |
| </Button> | |
| </DropdownMenuTrigger> | |
| <DropdownMenuContent align="end" className="w-36"> | |
| <DropdownMenuItem | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onEdit(); | |
| }} | |
| data-testid={`edit-running-${feature.id}`} | |
| className="text-xs" | |
| > | |
| <Edit className="w-3 h-3 mr-2" /> | |
| Edit | |
| </DropdownMenuItem> | |
| {/* Model info in dropdown */} | |
| <div className="px-2 py-1.5 text-[10px] text-muted-foreground border-t mt-1 pt-1.5"> | |
| <div className="flex items-center gap-1"> | |
| <Cpu className="w-3 h-3" /> | |
| <span>{formatModelName(feature.model ?? DEFAULT_MODEL)}</span> | |
| </div> | |
| </div> | |
| </DropdownMenuContent> | |
| </DropdownMenu> | |
| <DropdownMenu> | |
| <DropdownMenuTrigger asChild> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className="h-6 w-6 p-0 hover:bg-muted/80 rounded-md" | |
| onClick={(e) => e.stopPropagation()} | |
| onPointerDown={(e) => e.stopPropagation()} | |
| data-testid={`menu-running-${feature.id}`} | |
| aria-label="Open task menu" | |
| > | |
| <MoreVertical className="w-3.5 h-3.5 text-muted-foreground" /> | |
| </Button> | |
| </DropdownMenuTrigger> | |
| <DropdownMenuContent align="end" className="w-36"> | |
| <DropdownMenuItem | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onEdit(); | |
| }} | |
| data-testid={`edit-running-${feature.id}`} | |
| className="text-xs" | |
| > | |
| <Edit className="w-3 h-3 mr-2" /> | |
| Edit | |
| </DropdownMenuItem> | |
| {/* Model info in dropdown */} | |
| <div className="px-2 py-1.5 text-[10px] text-muted-foreground border-t mt-1 pt-1.5"> | |
| <div className="flex items-center gap-1"> | |
| <Cpu className="w-3 h-3" /> | |
| <span>{formatModelName(feature.model ?? DEFAULT_MODEL)}</span> | |
| </div> | |
| </div> | |
| </DropdownMenuContent> | |
| </DropdownMenu> |
🤖 Prompt for AI Agents
In apps/ui/src/components/views/board-view/components/kanban-card.tsx around
lines 496 to 529, the dropdown trigger button uses only an icon and lacks an
accessible label; add an accessible label (e.g., aria-label="More actions" or
aria-label={`More actions for ${feature.title || feature.id}`}) to the Button so
screen readers can announce its purpose, or include a visually-hidden text
element inside the trigger; ensure the label does not alter styling or event
handlers and keep the existing data-testid intact.
| <div className="px-2 py-1.5 text-[10px] text-muted-foreground border-t mt-1 pt-1.5"> | ||
| <div className="flex items-center gap-1"> | ||
| <Cpu className="w-3 h-3" /> | ||
| <span>{formatModelName(feature.model ?? DEFAULT_MODEL)}</span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated model info block into a reusable component.
The model info display block is duplicated in two locations (lines 522-527 for running tasks and lines 665-670 for in-progress tasks) with identical structure and logic. This violates the DRY principle and increases maintenance burden.
🔎 Proposed refactor
Create a reusable component before the KanbanCard component:
+/**
+ * Displays model information in dropdown menus
+ */
+function ModelInfoBlock({ model }: { model?: string }) {
+ return (
+ <div className="px-2 py-1.5 text-[10px] text-muted-foreground border-t mt-1 pt-1.5">
+ <div className="flex items-center gap-1">
+ <Cpu className="w-3 h-3" />
+ <span>{formatModelName(model ?? DEFAULT_MODEL)}</span>
+ </div>
+ </div>
+ );
+}
+
export const KanbanCard = memo(function KanbanCard({Then replace both occurrences:
Edit
</DropdownMenuItem>
- {/* Model info in dropdown */}
- <div className="px-2 py-1.5 text-[10px] text-muted-foreground border-t mt-1 pt-1.5">
- <div className="flex items-center gap-1">
- <Cpu className="w-3 h-3" />
- <span>{formatModelName(feature.model ?? DEFAULT_MODEL)}</span>
- </div>
- </div>
+ <ModelInfoBlock model={feature.model} />
</DropdownMenuContent>Also applies to: 665-670
🤖 Prompt for AI Agents
In apps/ui/src/components/views/board-view/components/kanban-card.tsx around
lines 522-527 (and also update 665-670), extract the duplicated model info block
into a small reusable component (e.g., ModelInfo) placed above the KanbanCard
component in the same file: accept a model prop (string | undefined), render the
existing div structure with the Cpu icon and span using formatModelName(model ??
DEFAULT_MODEL), and preserve classes/markup exactly; then replace both original
blocks with <ModelInfo model={feature.model} /> (or the correct prop name) and
add appropriate TypeScript props typing and any needed imports (formatModelName,
DEFAULT_MODEL, Cpu) so the file compiles.
| // Get current background settings for a project | ||
| const getCurrentSettings = useCallback( | ||
| (projectPath: string) => { | ||
| const current = store.boardBackgroundByProject[projectPath]; | ||
| return current || { | ||
| imagePath: null, | ||
| cardOpacity: 100, | ||
| columnOpacity: 100, | ||
| columnBorderEnabled: true, | ||
| cardGlassmorphism: true, | ||
| cardBorderEnabled: true, | ||
| cardBorderOpacity: 100, | ||
| hideScrollbar: false, | ||
| }; | ||
| }, | ||
| [store.boardBackgroundByProject] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicated default settings across multiple locations.
The default background settings are defined in at least three places:
defaultBackgroundSettingsinapp-store.tsgetCurrentSettingsfallback (lines 40-49)clearBoardBackgroundreset values (lines 156-164)
Consider importing and reusing defaultBackgroundSettings from the store to maintain consistency.
Suggested fix
+import { useAppStore, defaultBackgroundSettings } from "@/store/app-store";
// In getCurrentSettings:
const getCurrentSettings = useCallback(
(projectPath: string) => {
const current = store.boardBackgroundByProject[projectPath];
- return current || {
- imagePath: null,
- cardOpacity: 100,
- columnOpacity: 100,
- columnBorderEnabled: true,
- cardGlassmorphism: true,
- cardBorderEnabled: true,
- cardBorderOpacity: 100,
- hideScrollbar: false,
- };
+ return current || defaultBackgroundSettings;
},
[store.boardBackgroundByProject]
);
// In clearBoardBackground:
const clearBoardBackground = useCallback(
async (projectPath: string) => {
store.clearBoardBackground(projectPath);
- await persistSettings(projectPath, {
- imagePath: null,
- imageVersion: undefined,
- cardOpacity: 100,
- columnOpacity: 100,
- columnBorderEnabled: true,
- cardGlassmorphism: true,
- cardBorderEnabled: true,
- cardBorderOpacity: 100,
- hideScrollbar: false,
- });
+ await persistSettings(projectPath, {
+ ...defaultBackgroundSettings,
+ imageVersion: undefined,
+ });
},
[store, persistSettings]
);Also applies to: 151-168
🤖 Prompt for AI Agents
In apps/ui/src/hooks/use-board-background-settings.ts around lines 36 to 52, the
hook currently inlines default background settings which duplicate the canonical
defaultBackgroundSettings defined in app-store.ts; update this file to import
defaultBackgroundSettings from the store and use that constant as the fallback
in getCurrentSettings (and also replace the inline reset values used in
clearBoardBackground at lines ~151-168) so all places reuse the single source of
truth.
| const setCardOpacity = useCallback( | ||
| async (projectPath: string, opacity: number) => { | ||
| const current = getCurrentSettings(projectPath); | ||
| store.setCardOpacity(projectPath, opacity); | ||
| await persistSettings(projectPath, { ...current, cardOpacity: opacity }); | ||
| }, | ||
| [store, persistSettings, getCurrentSettings] | ||
| ); | ||
|
|
||
| const setColumnOpacity = useCallback( | ||
| async (projectPath: string, opacity: number) => { | ||
| const current = getCurrentSettings(projectPath); | ||
| store.setColumnOpacity(projectPath, opacity); | ||
| await persistSettings(projectPath, { ...current, columnOpacity: opacity }); | ||
| }, | ||
| [store, persistSettings, getCurrentSettings] | ||
| ); |
There was a problem hiding this comment.
Performance: Slider changes trigger immediate server calls.
Sliders fire onValueChange continuously during drag, which will trigger many API calls. Consider debouncing persistence for slider-based settings while keeping local store updates immediate for responsive UI.
Suggested approach: Debounce persistence
import { useMemo } from "react";
import debounce from "lodash/debounce"; // or implement your own
// Inside the hook:
const debouncedPersist = useMemo(
() => debounce(
(projectPath: string, settings: Record<string, unknown>) => {
persistSettings(projectPath, settings);
},
300
),
[persistSettings]
);
const setCardOpacity = useCallback(
async (projectPath: string, opacity: number) => {
const current = getCurrentSettings(projectPath);
store.setCardOpacity(projectPath, opacity);
// Use debounced version for sliders
debouncedPersist(projectPath, { ...current, cardOpacity: opacity });
},
[store, debouncedPersist, getCurrentSettings]
);Alternatively, handle debouncing at the UI layer in the modal component.
🤖 Prompt for AI Agents
In apps/ui/src/hooks/use-board-background-settings.ts around lines 76 to 92,
slider setters immediately call persistSettings causing many API calls; change
them to update the local store synchronously for UI responsiveness but debounce
the persistence calls: create a debounced wrapper for persistSettings using
useMemo (e.g., lodash.debounce or a custom debounce) with a ~300ms delay,
replace await persistSettings(...) in setCardOpacity and setColumnOpacity with
calls to the debouncedPersist(projectPath, settings), update hook dependencies
to include the debounced function, and add a cleanup (useEffect) to cancel the
debounced call on unmount to avoid leaks or stray requests.
- Adjusted spacing and alignment in the KanbanCard component for better visual consistency. - Refactored badge rendering logic to use a more compact layout, enhancing readability. - Cleaned up code formatting for improved maintainability and clarity. - Updated Card component styles to ensure consistent padding and margins.
Pull Request Review: useBoardBackgroundSettings HookSummaryThis PR introduces a new custom hook ✅ Strengths
🐛 Potential Issues1. Race Conditions in Settings Persistence (Medium Priority)Location: The hook updates local state synchronously but persists to the server asynchronously without proper race condition handling: const setCardOpacity = useCallback(
async (projectPath: string, opacity: number) => {
const current = getCurrentSettings(projectPath);
store.setCardOpacity(projectPath, opacity); // Synchronous
await persistSettings(projectPath, { ...current, cardOpacity: opacity }); // Async
},
[store, persistSettings, getCurrentSettings]
);Issue: If a user rapidly changes multiple settings (e.g., dragging a slider), each call gets the "current" settings, updates the store, then persists. The Recommendation:
2. Silent Failure on Persistence Errors (Medium Priority)Location: if (!result.success) {
console.error("Failed to persist settings:", result.error);
toast.error("Failed to save settings");
}Issue: When persistence fails, the local store has already been updated. This creates a state mismatch where the UI shows settings that aren't actually saved on the server. On page reload, the user's changes will be lost. Recommendation:
3. Missing Error Boundaries (Low Priority)Location: All async event handlers in The component handlers are now async but React doesn't catch async errors in event handlers: const handleCardOpacityChange = useCallback(
async (value: number[]) => {
if (!currentProject) return;
await setCardOpacity(currentProject.path, value[0]); // Error not caught
},
[currentProject, setCardOpacity]
);Recommendation: Add try-catch blocks around the async calls or use an error boundary. 4. Stale Closure in getCurrentSettings (Low Priority)Location: const getCurrentSettings = useCallback(
(projectPath: string) => {
const current = store.boardBackgroundByProject[projectPath];
// ...
},
[store.boardBackgroundByProject]
);Issue: The dependency Recommendation: Consider using 🎨 Code Quality Observations1. Good: Keyboard Shortcut ImplementationThe FileBrowserDialog keyboard shortcut is well implemented with proper cleanup: useEffect(() => {
if (!open) return;
const handleKeyDown = (e: KeyboardEvent) => { ... };
window.addEventListener("keydown", handleKeyDown);
return () => window.removeEventListener("keydown", handleKeyDown);
}, [open, currentPath, loading, handleSelect]);2. Minor: Inconsistent Gap SpacingLocation: The gap was changed from 3. Minor: Magic NumbersLocation: Multiple locations The code uses magic numbers like const OPACITY_MAX = 100;
const OPACITY_MIN = 0;⚡ Performance Considerations
🔒 Security ConcernsNo significant security issues identified. The code:
🧪 Test CoverageConcern: No tests were added for the new hook or updated components. Recommendations:
📝 Suggestions for Improvement
type BoardBackgroundSettings = {
imagePath: string | null;
imageVersion?: number;
cardOpacity: number;
columnOpacity: number;
// ... etc
};
🎯 Overall AssessmentThis is a well-structured refactoring that improves code organization and adds useful features. The main concerns are around error handling and race conditions in the persistence layer. I recommend addressing the medium-priority issues before merging. Recommendation: Approve with suggested changes. Review generated with assistance from Claude Code |
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 (1)
apps/ui/src/components/views/board-view/components/kanban-card.tsx (1)
654-663: Add accessible label to dropdown trigger.This dropdown trigger also lacks an accessible label. Screen reader users won't know the button's purpose.
🔎 Proposed fix
<DropdownMenuTrigger asChild> <Button variant="ghost" size="sm" className="h-6 w-6 p-0 hover:bg-muted/80 rounded-md" onClick={(e) => e.stopPropagation()} onPointerDown={(e) => e.stopPropagation()} data-testid={`menu-${feature.id}`} + aria-label="Open task menu" > <MoreVertical className="w-3.5 h-3.5 text-muted-foreground" /> </Button> </DropdownMenuTrigger>
♻️ Duplicate comments (2)
apps/ui/src/components/views/board-view/components/kanban-card.tsx (2)
542-549: Extract duplicated model info block into a reusable component.The model info display block is duplicated in two locations (lines 542-549 for running tasks and lines 691-698 for in-progress tasks) with identical structure and logic. This violates the DRY principle and increases maintenance burden.
🔎 Proposed refactor
Create a reusable component before the KanbanCard component:
+/** + * Displays model information in dropdown menus + */ +function ModelInfoBlock({ model }: { model?: string }) { + return ( + <div className="px-2 py-1.5 text-[10px] text-muted-foreground border-t mt-1 pt-1.5"> + <div className="flex items-center gap-1"> + <Cpu className="w-3 h-3" /> + <span>{formatModelName(model ?? DEFAULT_MODEL)}</span> + </div> + </div> + ); +} + export const KanbanCard = memo(function KanbanCard({Then replace both occurrences:
Edit </DropdownMenuItem> - {/* Model info in dropdown */} - <div className="px-2 py-1.5 text-[10px] text-muted-foreground border-t mt-1 pt-1.5"> - <div className="flex items-center gap-1"> - <Cpu className="w-3 h-3" /> - <span> - {formatModelName(feature.model ?? DEFAULT_MODEL)} - </span> - </div> - </div> + <ModelInfoBlock model={feature.model} /> </DropdownMenuContent>Also applies to: 691-698
518-528: Add accessible label to dropdown trigger.The dropdown menu trigger button uses only a visual icon (MoreVertical) without an accessible label, which makes it difficult for screen reader users to understand the button's purpose.
🔎 Proposed fix
<Button variant="ghost" size="sm" className="h-6 w-6 p-0 hover:bg-muted/80 rounded-md" onClick={(e) => e.stopPropagation()} onPointerDown={(e) => e.stopPropagation()} data-testid={`menu-running-${feature.id}`} + aria-label="Open task menu" > <MoreVertical className="w-3.5 h-3.5 text-muted-foreground" /> </Button>
🧹 Nitpick comments (1)
apps/ui/src/components/views/board-view/components/kanban-card.tsx (1)
773-797: Consider more robust PR URL parsing.The current implementation uses
.split("/").pop()which could return an empty string if the URL has a trailing slash (e.g.,https://github.com/user/repo/pull/123/), resulting in "Pull Request #" being displayed without a number.🔎 Proposed improvement
{/* PR URL Display */} {typeof feature.prUrl === "string" && /^https?:\/\//i.test(feature.prUrl) && (() => { - const prNumber = feature.prUrl.split("/").pop(); + const prNumber = feature.prUrl + .split("/") + .filter(Boolean) + .pop(); return ( <div className="mb-2">This filters out empty segments before extracting the last part, handling trailing slashes gracefully.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/components/ui/card.tsx(1 hunks)apps/ui/src/components/views/board-view/components/kanban-card.tsx(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/ui/src/components/views/board-view/components/kanban-card.tsx (2)
apps/ui/src/store/app-store.ts (1)
useAppStore(853-2358)apps/ui/src/components/ui/card.tsx (2)
CardHeader(95-95)CardContent(100-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (3)
apps/ui/src/components/views/board-view/components/kanban-card.tsx (2)
342-428: Nice refactor of badge rendering.The consolidation of badges into a compact row with conditional rendering improves the visual layout and maintains proper accessibility with tooltips. The badge display logic correctly prioritizes error states and handles blocking dependencies appropriately.
437-503: Header and actions restructuring looks good.The reorganization of header badges, controls, and actions improves code organization. The use of optional chaining for
feature.planSpec?.statusis good defensive coding practice. The layout changes maintain all existing functionality while improving visual structure.Also applies to: 505-703, 982-1046
apps/ui/src/components/ui/card.tsx (1)
14-14: Card component gap-1 is intentional for tight internal spacing.The Card component was newly created with
gap-1(4px) as its initial internal spacing. This compact vertical gap between direct children is designed to work with the sub-components (CardHeader, CardContent, CardFooter), which each define their own padding (px-6) and manage their own internal spacing. The component is used across 10+ files in the application as a container with no reported layout issues.
| {/* Category row */} | ||
| <div className="px-3 pt-4"> | ||
| <span className="text-[11px] text-muted-foreground/70 font-medium"> | ||
| {feature.category} | ||
| </span> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
Add null check for category display.
The category is rendered without checking if feature.category exists. If the property is undefined, this will display the text "undefined" on the card.
🔎 Proposed fix
{/* Category row */}
- <div className="px-3 pt-4">
- <span className="text-[11px] text-muted-foreground/70 font-medium">
- {feature.category}
- </span>
- </div>
+ {feature.category && (
+ <div className="px-3 pt-4">
+ <span className="text-[11px] text-muted-foreground/70 font-medium">
+ {feature.category}
+ </span>
+ </div>
+ )}📝 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.
| {/* Category row */} | |
| <div className="px-3 pt-4"> | |
| <span className="text-[11px] text-muted-foreground/70 font-medium"> | |
| {feature.category} | |
| </span> | |
| </div> | |
| {/* Category row */} | |
| {feature.category && ( | |
| <div className="px-3 pt-4"> | |
| <span className="text-[11px] text-muted-foreground/70 font-medium"> | |
| {feature.category} | |
| </span> | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
In apps/ui/src/components/views/board-view/components/kanban-card.tsx around
lines 430 to 436, the JSX renders feature.category directly which can display
"undefined" when the property is missing; update the render to check for a
truthy category and fall back to an empty string or a placeholder (e.g., '' or
'Uncategorized') before outputting so nothing or a sensible label is shown when
feature.category is undefined or null.
…d background settings with persistence
Summary by CodeRabbit
New Features
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.