refactor: Decompose App.tsx and standardize frontend component architecture#1850
refactor: Decompose App.tsx and standardize frontend component architecture#1850ParkerM2 wants to merge 12 commits intoAndyMik90:developfrom
Conversation
…ecture Breaks the 1,183-line monolithic App.tsx into focused, single-responsibility modules for maintainability and scalability. Extracts TopNavBar, Sidebar, and dialog orchestration into dedicated components with custom hooks and Zustand stores. Removes 16 unused shadcn/ui components to keep the PR scope focused on the structural refactoring and tweakcn theme system. Key changes: - App.tsx reduced from 1,183 to 47 lines (pure composition) - New AppShell, AppDialogs, ViewSwitcher layout components - TopNavBar directory with ProjectTabBar, WindowControls, AppBranding - Sidebar refactored into component + useSidebar hook + types - New hooks: useAppInitialization, useAppTheme, useAppEventListeners, useOnboardingDetection, useVersionWarning, useTaskSync - New Zustand stores: dialog-store, navigation-store - Window controls IPC API for cross-platform title bar - Theme CSS variables for tweakcn.com compatibility - Tests migrated to new TopNavBar/__tests__/ location - Removed 16 unused shadcn/ui components (accordion, alert, avatar, etc.) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
|
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:
📝 WalkthroughWalkthroughAdds Electron window-control IPC and preload WindowAPI; refactors renderer into AppShell/AppDialogs/ViewSwitcher with new hooks and stores; modularizes Sidebar and TopNavBar (dnd-kit tabs); implements theme/font utilities; expands UI primitives and i18n; many component, store, hook, and style changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer as Renderer (WindowControls)
participant Preload as Preload (ipcRenderer)
participant Main as Electron Main
User->>Renderer: click minimize/maximize/close
Renderer->>Preload: invoke WINDOW_MINIMIZE/MAXIMIZE/CLOSE
Preload->>Main: IPC invoke (main)
Main->>Main: perform window operation
Main->>Preload: emit WINDOW_MAXIMIZE_CHANGED (if changed)
Preload->>Renderer: deliver maximize state event
Renderer->>User: update control icons/state
sequenceDiagram
participant App as App (Renderer)
participant Init as useAppInitialization
participant Theme as useAppTheme
participant Events as useAppEventListeners
participant Shell as AppShell
participant View as ViewSwitcher
participant Nav as NavigationStore
App->>Init: run initialization hook
App->>Theme: apply theme/fonts
App->>Events: register global listeners
App->>Shell: render layout
Shell->>Nav: read activeView / selectedProject
Shell->>View: render selected view
View->>Nav: dispatch view actions (select task, open terminal)
Nav->>Shell: state updates trigger UI changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
Summary of ChangesHello @ParkerM2, 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 introduces a substantial refactoring of the frontend architecture, primarily by decomposing the main application component into a more modular and maintainable structure. The changes aim to improve code organization, enhance cross-platform consistency for window management, and expand the theming capabilities, resulting in a cleaner and more scalable codebase. Highlights
Changelog
Activity
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
|
![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 is an impressive and well-executed refactoring of the frontend architecture. Decomposing the monolithic App.tsx into focused, single-responsibility components and custom hooks is a significant improvement for maintainability and scalability. The introduction of Zustand for state management (dialog-store, navigation-store) is an excellent choice that centralizes state and simplifies logic. The addition of cross-platform window controls and the flexible, tweakcn.com-compatible theme system are also great enhancements. Overall, this is a high-quality pull request that greatly improves the frontend codebase.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 55
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
apps/frontend/src/renderer/components/ui/label.tsx (1)
23-28: 🧹 Nitpick | 🔵 TrivialConsider simplifying to React 19's ref-as-prop pattern.
Since React 19 treats
refas a regular prop,forwardRefis no longer necessary. While the current implementation is valid, simplifying would align with modern React patterns.♻️ Optional React 19 simplification
-const Label = React.forwardRef<HTMLLabelElement, LabelProps>( - ({ className, ...props }, ref) => ( - <label ref={ref} className={cn(labelVariants(), className)} {...props} /> - ) -); -Label.displayName = 'Label'; +const Label = ({ className, ref, ...props }: LabelProps & { ref?: React.Ref<HTMLLabelElement> }) => ( + <label ref={ref} className={cn(labelVariants(), className)} {...props} /> +);Note: This is a low-priority enhancement and can be addressed in a future cleanup pass.
apps/frontend/src/renderer/components/ui/switch.tsx (1)
13-39: 🧹 Nitpick | 🔵 TrivialConsider modernizing to React 19's ref-as-prop pattern.
React 19 allows
refas a regular prop, eliminating the need forforwardRef. The current implementation works correctly, but you can simplify it to align with React 19 patterns.♻️ Optional React 19 modernization
-const Switch = React.forwardRef< - React.ElementRef<typeof SwitchPrimitives.Root>, - React.ComponentPropsWithoutRef<typeof SwitchPrimitives.Root> ->(({ className, ...props }, ref) => ( +const Switch = ({ + className, + ref, + ...props +}: React.ComponentPropsWithRef<typeof SwitchPrimitives.Root>) => ( <SwitchPrimitives.Root className={cn( 'peer inline-flex h-6 w-11 shrink-0 cursor-pointer items-center rounded-full', 'border-2 border-transparent transition-all duration-200', 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-background', 'disabled:cursor-not-allowed disabled:opacity-50', 'data-[state=checked]:bg-primary data-[state=unchecked]:bg-border', className )} {...props} ref={ref} > <SwitchPrimitives.Thumb className={cn( 'pointer-events-none block h-5 w-5 rounded-full shadow-sm ring-0 transition-transform duration-200', 'bg-white dark:bg-foreground', 'data-[state=checked]:translate-x-5 data-[state=unchecked]:translate-x-0', 'data-[state=checked]:bg-primary-foreground' )} /> </SwitchPrimitives.Root> -)); +); Switch.displayName = SwitchPrimitives.Root.displayName;apps/frontend/src/renderer/components/ui/combobox.tsx (3)
56-58:⚠️ Potential issue | 🟡 MinorHardcoded English strings violate the i18n coding guideline.
The default values
"Select...","Search...", and"No results found"are new hardcoded user-facing strings in a new file. Since consumers may omit these props, the defaults will surface as untranslated English text.Either remove the defaults and make the caller always pass translated strings, or use
useTranslation()inside the component for the fallbacks.Example approach (caller-required)
- placeholder = 'Select...', - searchPlaceholder = 'Search...', - emptyMessage = 'No results found', + placeholder, + searchPlaceholder, + emptyMessage,Then require callers to pass
t('combobox.placeholder'), etc.As per coding guidelines: "All frontend user-facing text MUST use
react-i18nexttranslation keys. Never hardcode strings in JSX/TSX."
113-116:⚠️ Potential issue | 🟡 MinorDead/mislabeled effect — empty dependency array doesn't react to
filteredOptionschanges.The comment says "Reset focused index when filtered options change," but the dependency array is
[], so this only runs on mount. The initial state is already-1, making this a no-op. If the intent is to reset on filter changes, addfilteredOptionsto the deps. If it was only meant for mount, the[open]effect above already handles it.Option A: actually reset on filter changes
React.useEffect(() => { setFocusedIndex(-1); - }, []); + }, [filteredOptions]);Option B: remove the dead effect
- // Reset focused index when filtered options change - React.useEffect(() => { - setFocusedIndex(-1); - }, []);
242-299:⚠️ Potential issue | 🟡 MinorGrouping logic assumes options with the same
groupare contiguous.
showGroupHeader(line 245) only compares with the previous item. If options sharing the same group aren't adjacent in the array, a duplicate group header will render, and the visual grouping will be broken. This is especially likely after filtering, which can remove intermediate items and bring same-group items together differently.Consider sorting
filteredOptionsbygroupbefore rendering, or tracking seen groups in aSet:Approach: sort filtered options by group
const filteredOptions = React.useMemo(() => { - if (!search.trim()) return options; + const base = search.trim() + ? options.filter( + (opt) => + opt.label.toLowerCase().includes(search.toLowerCase()) || + opt.description?.toLowerCase().includes(search.toLowerCase()) + ) + : options; + // Stable sort by group to keep group headers contiguous + return [...base].sort((a, b) => + (a.group ?? '').localeCompare(b.group ?? '') + ); - const searchLower = search.toLowerCase(); - return options.filter( - (opt) => - opt.label.toLowerCase().includes(searchLower) || - opt.description?.toLowerCase().includes(searchLower) - ); }, [options, search]);apps/frontend/src/renderer/components/ui/dialog.tsx (1)
76-79:⚠️ Potential issue | 🟡 MinorHardcoded screen-reader text
"Close"should use a translation key.This is a new component, so this constitutes a new i18n violation. Screen-reader text is user-facing and must be localized.
🌐 Proposed fix
Since this is a pure UI primitive that doesn't easily accept a
useTranslationhook (it's inside aforwardRefrender), consider accepting acloseAriaLabelprop with a sensible default, or wrap it with a hook:+import { useTranslation } from 'react-i18next'; // ... inside DialogContent's forwardRef: - <X className="h-4 w-4" /> - <span className="sr-only">Close</span> + <X className="h-4 w-4" /> + <span className="sr-only">{t('common:buttons.close')}</span>The
common:buttons.closekey already exists in your locale files.As per coding guidelines:
**/*.{tsx,ts}: "All frontend user-facing text MUST usereact-i18nexttranslation keys. Never hardcode strings in JSX/TSX."apps/frontend/src/renderer/components/ui/button.tsx (1)
31-33:⚠️ Potential issue | 🟡 MinorInconsistent styling approach for
successvariant vswarning/info.
warningandinfouse direct Tailwind tokens (bg-warning,bg-info), butsuccessuses arbitrary value syntaxbg-[var(--success)]. Additionally,hover:bg-[var(--success)]/90may not apply the opacity modifier correctly in Tailwind — the/90opacity shorthand typically requires the color to be defined as a Tailwind theme token or in a compatible color format (e.g.,oklch).Either register
--successas a proper Tailwind theme color (likewarningandinfo) so you can writebg-success, or verify that the arbitrary-value opacity modifier renders correctly.#!/bin/bash # Check how --success, --warning, --info are defined in CSS/theme files rg --type=css -n '\-\-success|\-\-warning|\-\-info' -C2apps/frontend/src/shared/types/settings.ts (1)
154-160: 🧹 Nitpick | 🔵 TrivialConsider whether
isCustomis redundant with theidpattern.Since
ColorThemealready encodes custom themes via thecustom:prefix,isCustomcan be derived withid.startsWith('custom:'). The flag isn't harmful, but it introduces a potential for inconsistency if someone constructs a definition whereidstarts withcustom:butisCustomisfalse(or vice versa). If you keep it, consider adding a comment clarifying it's a convenience field.apps/frontend/src/renderer/components/ui/full-screen-dialog.tsx (1)
69-70: 🧹 Nitpick | 🔵 TrivialHardcoded string in
sr-onlyspan.Line 70 has a hardcoded
"Close"string that should use a translation key for accessibility. This appears to be pre-existing (unchanged line), so it can be deferred to a future i18n cleanup pass. Based on learnings, pre-existing i18n issues in files where the PR doesn't introduce new violations can be deferred.apps/frontend/src/renderer/components/TopNavBar/__tests__/ProjectTabBar.test.tsx (2)
46-87:⚠️ Potential issue | 🟠 MajorTests never render the component — assertions are tautological.
None of these tests actually render
ProjectTabBarusing React Testing Library. They construct local data and assert against it — e.g.,expect(projects.length).toBe(0)on a locally created empty array. This verifies JavaScript array behavior, not component behavior.As per coding guidelines, frontend tests should "use Vitest and React Testing Library with jsdom." These tests have
@vitest-environment jsdomdeclared but never userender()orscreenfrom RTL.
245-256:⚠️ Potential issue | 🟠 MajorTautological assertion — tests a hardcoded string against itself.
This test defines
buttonClasses = 'h-7 w-7 rounded-full shrink-0'and then assertsexpect(buttonClasses).toContain('h-7'). It will always pass regardless of what the actual component renders. The same pattern repeats throughout the "Container Layout and Styling" and "UsageIndicator Integration" sections.These tests provide zero regression safety — if the component's class names change, these tests will still pass.
🤖 Fix all issues with AI agents
In `@apps/frontend/package.json`:
- Around line 17-20: The package.json engines.node field currently requires
">=24.0.0", which is too restrictive; update the "engines": { "node": ">=24.0.0"
} entry to a more compatible lower bound (for example ">=20.0.0" or ">=22.0.0")
so contributors and CI on LTS versions can run the frontend; modify the
engines.node string accordingly and run a quick install/test locally to confirm
no Node-24-specific APIs are used (check any uses in build scripts or code paths
referenced by package.json scripts).
- Around line 59-73: Remove the nine unused `@radix-ui` packages from the
dependencies section of package.json: "@radix-ui/react-accordion",
"@radix-ui/react-aspect-ratio", "@radix-ui/react-avatar",
"@radix-ui/react-context-menu", "@radix-ui/react-hover-card",
"@radix-ui/react-menubar", "@radix-ui/react-navigation-menu",
"@radix-ui/react-progress", and "@radix-ui/react-radio-group"; update
package.json by deleting those entries so they are no longer installed (and run
your package manager install afterwards), or if any of those packages are
actually needed, add a short TODO comment and import/use the corresponding
components instead.
- Line 96: Remove the unused dependency entry "cmdk": "^1.1.1" from
apps/frontend's package.json by deleting that key from the dependencies object
(look for the "cmdk" entry in package.json), then run your package manager (npm
install or yarn install or pnpm install) to update the lockfile; ensure no
import or usage of cmdk remains and commit the package.json and lockfile changes
together.
In `@apps/frontend/src/main/ipc-handlers/window-handlers.ts`:
- Around line 31-42: The IPC handler for IPC_CHANNELS.WINDOW_SET_BOUNDS (in
window-handlers.ts) enforces min width/height but doesn't validate x/y, so clamp
or adjust the incoming bounds.x and bounds.y before calling getMainWindow() ->
win.setBounds to avoid placing the window fully off-screen; use Electron's
screen.getDisplayMatching(bounds) (or screen.getAllDisplays()) to find the
nearest display and then ensure bounds.x/bounds.y are within that display's
workArea (or fallback to a sane centered position) and only then call
win.setBounds so the window remains visible after monitor changes.
In `@apps/frontend/src/preload/api/index.ts`:
- Around line 57-58: Rename the exported property currently named window to
windowControls for clarity: in the object where queue: createQueueAPI() and
window: createWindowAPI() are set, change the key to windowControls and keep the
value createWindowAPI(); update all references that call electronAPI.window.* to
electronAPI.windowControls.* (search for usages of electronAPI.window) so
consumers use the new property name; ensure any typings or IPC exposure
referencing the window property (e.g., createWindowAPI, the preload export
object) are updated to reflect windowControls.
In `@apps/frontend/src/renderer/components/AppDialogs.tsx`:
- Around line 116-122: Remove the noisy console.warn inside the Dialog
onOpenChange handler (the one referencing showInitDialog, pendingProject,
isInitializing, initSuccess) or guard it behind a debug flag; update the handler
so it no longer logs in production (e.g., check a DEBUG/NODE_ENV flag or a local
isDebug constant) while preserving the existing skipInit() call on
useDialogStore.getState().skipInit() when the dialog is closed and conditions
match.
- Around line 170-172: Remove the redundant { ns: 'common' } option from i18n
t() calls in the AppDialogs component: when the translation key already has the
'common:' prefix (e.g. t('common:buttons.skip') used with the Button that calls
useDialogStore.getState().skipInit()), drop the second argument so the call
becomes t('common:buttons.skip'); apply the same change to the other t(...)
usages in this component that pass { ns: 'common' } alongside a
'common:'-prefixed key.
- Around line 227-240: The onClick handler calls the async
removeProject(projectToRemove.id) without awaiting it, so errors escape the
try/catch and closeRemoveProjectDialog() runs immediately; update the Button
onClick to be an async function, await removeProject(projectToRemove.id), keep
calling useDialogStore.getState().setRemoveProjectError(null) before the await,
move useDialogStore.getState().closeRemoveProjectDialog() to after a successful
await, and retain the catch to call
useDialogStore.getState().setRemoveProjectError(err instanceof Error ?
err.message : t('common:errors.unknownError')) and console.error on failure.
In `@apps/frontend/src/renderer/components/AppShell.tsx`:
- Around line 38-45: The two WelcomeScreen props onNewProject and onOpenProject
both call useDialogStore.getState().openAddProjectModal(), which is likely wrong
or confusing; update the onOpenProject handler to call the intended dialog/open
action (e.g., openOpenProjectModal or openProjectPicker) instead of
openAddProjectModal, or if the shared modal is intentional add a brief inline
comment clarifying that openAddProjectModal handles both "create" and "open"
flows; look for the WelcomeScreen usage and the dialog store methods
(useDialogStore.getState().openAddProjectModal and any
openOpenProjectModal/openProjectPicker equivalents) and change the onOpenProject
binding accordingly.
- Around line 14-17: The selector for selectedProject causes unnecessary
recomputation on any project-store change because it runs
state.projects.find(...) every update; replace this with a selector that calls
the store's existing getActiveProject() (or a memoized selector) via
useProjectStore to return the active project, or use Zustand's shallow
comparison (useShallow) to avoid re-renders—update the selectedProject
assignment (the useProjectStore selector) to directly use getActiveProject() (or
a memoized callback) instead of searching state.projects.
- Around line 1-8: The imports in AppShell.tsx use relative paths; update them
to use the project's path alias (@) per tsconfig so they resolve from
src/renderer. Replace imports like './Sidebar', './TopNavBar',
'./WelcomeScreen', './ViewSwitcher', '../stores/navigation-store',
'../stores/dialog-store', '../stores/project-store', and
'../hooks/useAppEventListeners' with alias imports (e.g. '@/components/Sidebar',
'@/components/TopNavBar', '@/components/WelcomeScreen',
'@/components/ViewSwitcher', '@/stores/navigation-store',
'@/stores/dialog-store', '@/stores/project-store', and
'@/hooks/useAppEventListeners') so the file imports Sidebar, TopNavBar,
WelcomeScreen, ViewSwitcher, useNavigationStore, useDialogStore,
useProjectStore, and handleProjectTabClose via the @ path alias.
In `@apps/frontend/src/renderer/components/settings/ThemeSelector.tsx`:
- Line 98: The local arrow param in the expression that builds builtinNames
shadows the translation function t from useTranslation; rename the map parameter
to a non-conflicting identifier (e.g., theme or ct) in the expression that
computes builtinNames from COLOR_THEMES so the useTranslation t remains
unshadowed and linters/readers aren’t confused (update the code that defines
builtinNames and any references to that map callback).
- Around line 370-393: Replace the sparse DialogDescription that currently only
renders `"${deleteTargetName}"` with a full confirmation sentence using the i18n
helper `t` and the existing `deleteTargetName` variable (e.g., use a translation
key like `theme.customTheme.deleteConfirmMessage` and pass { name:
deleteTargetName }), keeping the surrounding Dialog, DialogContent, DialogHeader
and DialogFooter as-is and ensuring the destructive `handleDeleteCustomTheme`
button remains unchanged; update the translation files to include the new key so
the dialog reads like "Are you sure you want to delete the theme '{name}'? This
cannot be undone." and still handles null `deleteTargetName` gracefully.
In `@apps/frontend/src/renderer/components/Sidebar/constants/types.ts`:
- Around line 19-42: The exported arrays baseNavItems, githubNavItems, and
gitlabNavItems (and their preceding comments) have extra leading indentation
inconsistent with the file; remove the extra spaces so the comments and the
const declarations align with the rest of the module's top-level exports (adjust
the lines containing baseNavItems, githubNavItems, gitlabNavItems and their
comment blocks) to match the file's standard left-alignment.
In `@apps/frontend/src/renderer/components/Sidebar/hooks/useSidebar.ts`:
- Around line 155-159: The handler _handleRemoveProject is named with a
private-prefix but is exposed publicly from the hook; rename
_handleRemoveProject to handleRemoveProject (update its declaration and any
references in useSidebar and the hook's return) so the name matches its public
role, or alternatively keep the underscore and remove it from the hook's
returned object so it remains internal; specifically change the symbol
_handleRemoveProject and its usage in useSidebar's return to the chosen form to
keep naming consistent.
- Around line 119-139: The effect using useEffect currently depends on the whole
selectedProject object which changes reference when projects updates; change the
dependency to a stable primitive (e.g., selectedProject?.path or
selectedProject?.id) so the effect only reruns when the actual selected project
changes, and update the effect logic that calls
window.electronAPI.checkGitStatus(selectedProject.path) (and sets setGitStatus /
setShowGitSetupModal) to guard on that primitive being defined; alternatively
compute selectedProject with useMemo from projects.find(...) and keep the effect
dependency on the memoized primitive to avoid redundant IPC calls.
- Around line 164-182: The hook useSidebar currently returns unused properties;
update its return object to remove lastLoadedProjectIdRef (keep it internal for
dedup logic), projects, settings, _handleRemoveProject, githubEnabled, and
gitlabEnabled, while retaining showGitSetupModal, setShowGitSetupModal,
gitStatus, selectedProject, visibleNavItems, handleNavClick,
handleGitInitialized, t, selectedProjectId, isCollapsed, and toggleSidebar so
Sidebar/index.tsx still receives the values it uses; adjust the return statement
in useSidebar to only expose those needed identifiers.
In
`@apps/frontend/src/renderer/components/TopNavBar/__tests__/ProjectTabBar.test.tsx`:
- Around line 443-460: The two tests in the "UsageIndicator Integration"
describe block are only asserting local strings instead of verifying DOM
rendering; update the tests (the "should render UsageIndicator next to add
button" and "should render AuthStatusIndicator and UsageIndicator before add
button") to render the TopNavBar (or the specific component under test) with
RTL's render, then query the DOM for the actual nodes (e.g., by text, role, or
data-testid for AuthStatusIndicator, UsageIndicator, and AddButton) and assert
they exist and their relative order (use container.querySelectorAll to get their
indices or compareDocumentPosition between the queried elements) rather than
asserting against a hard-coded string array. Ensure you reference the component
under test (TopNavBar) and the element identifiers (data-testid or visible text
for AuthStatusIndicator, UsageIndicator, AddButton) when updating the
assertions.
In
`@apps/frontend/src/renderer/components/TopNavBar/__tests__/SortableProjectTab.test.tsx`:
- Around line 36-435: The tests in SortableProjectTab.test.tsx never import or
render the SortableProjectTab component and only assert tautologies; replace
those with real DOM tests: import SortableProjectTab, render it with React
Testing Library (render, screen) wrapped in the necessary dnd-kit context
providers (e.g., DndContext/SortableContext or the app's DnD wrapper), use
createTestProject for props and pass mockOnSelect/mockOnClose, then assert on
rendered DOM (screen.getByText, queryByRole, etc.) and simulate user
interactions with userEvent to verify callbacks (mockOnSelect, mockOnClose) are
called; remove or rewrite assertions that just check local variables/strings
against themselves and ensure jsdom environment is used for these tests.
In `@apps/frontend/src/renderer/components/TopNavBar/AppBranding.tsx`:
- Around line 10-13: The Badge in AppBranding.tsx is using variant="destructive"
for app branding which is semantically wrong; change the Badge usage to a
dedicated branding variant (e.g., variant="brand") or an appropriate existing
neutral variant, and if needed add the new "brand" variant to the Badge
component's variant/type definitions and styling logic so it renders the
intended brand colors; update the Badge JSX in AppBranding.tsx (the Badge
element that wraps img {appIcon} and {t('appTitle')}) to use the new variant and
ensure any theme/token names used match the design system's style keys.
In `@apps/frontend/src/renderer/components/TopNavBar/hooks/useTopNavBar.ts`:
- Around line 32-34: The handlers handleProjectTabSelect, handleDragStart, and
handleDragEnd are recreated every render and can break memoization in children
like SortableProjectTab; wrap each in React.useCallback to stabilize their
references (e.g., memoize handleProjectTabSelect around setActiveProject, and
memoize handleDragStart/handleDragEnd with whatever local state or props they
read) and ensure the dependency arrays include only the values they reference to
avoid stale closures.
- Around line 13-18: The component currently subscribes to the getProjectTabs
function reference (useProjectStore(state => state.getProjectTabs)) which never
changes so re-renders don't happen when tabs change; instead subscribe to the
raw pieces and derive the tabs so the component updates: use useProjectStore to
select tabOrder, projects, and openProjectIds (e.g. const { tabOrder, projects,
openProjectIds } = useProjectStore(state => ({ tabOrder: state.tabOrder,
projects: state.projects, openProjectIds: state.openProjectIds }))) and then
compute projectTabs by calling the store helper (or computing inline) so
projectTabs updates when reorderTabs/setActiveProject modify state; keep
references to setActiveProject, reorderTabs, and activeProjectId as before.
In `@apps/frontend/src/renderer/components/TopNavBar/ProjectTabBar.tsx`:
- Around line 7-8: Replace the relative imports of the UsageIndicator and
AuthStatusIndicator components with the project path alias; locate the import
statements that currently import UsageIndicator and AuthStatusIndicator and
change them to use the '@/components/...' alias (e.g., import { UsageIndicator }
from '@/components/...'; import { AuthStatusIndicator } from '@/components/...')
so they match the other aliased imports in this file and the tsconfig path
rules.
In `@apps/frontend/src/renderer/components/TopNavBar/SortableProjectTab.tsx`:
- Around line 19-21: The module-level platform detection (const isMac and const
modKey) should be moved inside the SortableProjectTab functional component to
match project pattern used by WindowControls, TopNavBar, and Sidebar; update the
SortableProjectTab component body to compute isMac = !!window.platform?.isMacOS
and modKey = isMac ? '⌘' : 'Ctrl+' before they are used and remove the
module-level declarations so the values are derived at render time within the
component.
In `@apps/frontend/src/renderer/components/TopNavBar/WindowControls.tsx`:
- Around line 11-16: The initial window.electronAPI.window.isMaximized() promise
in the useEffect can reject and is currently unhandled, and the maximize button
only relies on the onMaximizeChanged listener so the icon may not update if that
event doesn't fire; wrap the isMaximized() call with a .catch to handle/recover
(e.g., setIsMaximized(false) or leave previous state) and ensure the
onMaximizeChanged cleanup is returned safely, and in the maximize click handler
(the function that calls window.electronAPI.window.maximize()) also await or
.then the maximize() IPC result and call setIsMaximized with that returned value
as a fallback to keep UI state in sync.
In `@apps/frontend/src/renderer/components/ui/error-boundary.tsx`:
- Around line 29-55: The ErrorBoundaryFallback component uses
useTranslation('errors') so simplify the translation keys by removing the
redundant namespace prefix in t calls (e.g., change
t('errors:errorBoundary.title') → t('errorBoundary.title')) and remove the
unnecessary truthiness guard around the error block since the prop error: Error
is non-nullable; either render the error message unguarded or make the guard
more precise (e.g., check error.message) in the JSX where error.message is used;
relevant symbols: ErrorBoundaryFallback, useTranslation, t, error, onReset.
In `@apps/frontend/src/renderer/components/ui/scroll-area.tsx`:
- Around line 53-61: The hover rule on the ScrollArea wrapper uses an
unnecessary !important utility (hover:!opacity-100) which is redundant with
group-hover/scroll:opacity-100 and makes overrides harder; update the className
in the ScrollArea component (the JSX block that composes cn(...) and renders
ScrollAreaPrimitive.ScrollAreaThumb) to remove the exclamation modifier—use
hover:opacity-100 or omit the hover rule entirely so the group-hover controls
opacity, and ensure the rest of the class order remains the same so
user-supplied className can override as expected.
In `@apps/frontend/src/renderer/components/ViewSwitcher.tsx`:
- Line 46: Extract the duplicated selector logic into a shared selector (e.g.,
selectActiveProjectId) and replace repeated inline selectors; specifically, add
a selector function named selectActiveProjectId that returns
state.activeProjectId || state.selectedProjectId (place it in the project store
module where useProjectStore is defined), then update all usages that call
useProjectStore((state) => state.activeProjectId || state.selectedProjectId)
(including the ViewSwitcher component and the six wrapper components) to use
useProjectStore(selectActiveProjectId) so the logic is centralized and
consistent.
- Around line 72-85: KanbanView is creating new inline function references by
calling useNavigationStore.getState() and useDialogStore.getState() inside JSX
props (onTaskClick, onNewTaskClick, onRefresh), which breaks KanbanBoard's memo
comparator (droppableColumnPropsAreEqual checks strict equality). Fix by
creating stable callbacks with useCallback in KanbanView that call the
respective store methods (wrap calls to
useNavigationStore.getState().setSelectedTask,
useDialogStore.getState().openNewTaskDialog, and
useNavigationStore.getState().refreshTasks), then pass those stable handlers
into KanbanBoard so the comparator can reliably detect unchanged props.
In `@apps/frontend/src/renderer/hooks/useAppEventListeners.ts`:
- Around line 56-66: The keyboard shortcut handler uses a different init check
than handleProjectAdded; refactor to avoid divergence by either calling
handleProjectAdded(project, needsInit) after addProject or extracting the
init-detection into a shared function (e.g., needsInitForProject) used by both
handlers; update the selectDirectory flow (where addProject, openProjectTab and
useDialogStore.getState().openInitDialog are invoked) to rely on the same shared
logic or to call handleProjectAdded so both paths use the identical init
decision.
- Around line 136-154: The dialog is closed unconditionally after the try/catch
so failures in updateProjectEnv or updateProjectSettings are swallowed; move the
useDialogStore.getState().closeGitHubSetup() call inside the try block after
await loadProjects() so the dialog only closes on success, and in the catch
block surface the error to the user (e.g., call your toast/notification or error
state API — or set an error on the GitHub setup dialog) so failures from
updateProjectEnv, updateProjectSettings, or loadProjects are reported and the
dialog remains open for retry.
- Around line 110-121: Replace the hardcoded fallback messages in the init
failure and catch blocks with react-i18next translation lookups by importing
i18next and calling i18next.t(...) (e.g. change the fallback for result?.error
to i18next.t('init.autoClaudeFailed') and the catch fallback to
i18next.t('init.unexpectedError')); update the two places where
useDialogStore.getState().setInitError(...) is called to pass the translated
string, add the corresponding keys to both en/*.json and fr/*.json, and ensure
you import i18next at the top of the file (import i18next from 'i18next').
In `@apps/frontend/src/renderer/hooks/useAppInitialization.ts`:
- Around line 1-11: Update the imports in useAppInitialization.ts to use the
project's path aliases instead of relative paths: replace the relative imports
that bring in useProjectStore, useSettingsStore, loadSettings, loadProfiles,
loadClaudeProfiles, loadProjects, loadTasks, useTaskStore,
restoreTerminalSessions, initializeGitHubListeners,
initDownloadProgressListener, useNavigationStore, and useDialogStore with the
configured aliases (use '@/...' for src/renderer modules and '@shared/...' where
appropriate) so all ten imports follow the project's tsconfig path-mapping
convention.
- Around line 53-93: The dependency list for the useEffect incorrectly includes
projectTabs.map (which is the Array.prototype.map function) and thus never
changes; update the dependency to reflect tab content instead: either remove
projectTabs.map entirely if projectTabs.length is sufficient, or replace it with
a stable derived value such as a memoized/serialized IDs string (e.g.,
JSON.stringify(projectTabs.map(p => p.id)) or compute tabIds with useMemo) and
use that (and projectTabs.length if needed) in the useEffect dependency array so
changes to projectTabs actually trigger the effect; update the useEffect
dependency array where projectTabs.length and projectTabs.map currently appear
and keep the eslint-disable comment only if you intentionally omit projectTabs
object references.
In `@apps/frontend/src/renderer/hooks/useAppTheme.ts`:
- Around line 83-89: The effect in useAppTheme.ts uses useEffect to sync
i18n.language but lists i18n.changeLanguage in the deps; verify whether
i18n.changeLanguage is a stable reference from react-i18next and, if it is
unstable, either memoize it (e.g., create a stable changeLanguage callback using
useCallback that calls i18n.changeLanguage and use that in the dependency array)
or remove it from the dependency array and rely only on [settings.language]
(preserving the eslint-disable comment) so the effect only runs when
settings.language changes; reference the useEffect block, settings.language, and
i18n.changeLanguage when making the change.
- Around line 1-6: Replace the relative imports in useAppTheme.ts with the
project's path aliases: change imports from '../stores/settings-store' to
'@/stores/settings-store', from '../../shared/constants' to '@shared/constants',
from '../lib/theme-utils' to '@/lib/theme-utils', and from '../../shared/types'
to '@shared/types' so the file uses the tsconfig "@/*" and "@shared/*" aliases;
keep the same imported symbols (useSettingsStore, COLOR_THEMES, UI_SCALE_MIN,
UI_SCALE_MAX, UI_SCALE_DEFAULT, applyCustomTheme, clearAllCustomVariables,
ColorTheme) and ensure any editor/TS server picks up the alias resolution.
In `@apps/frontend/src/renderer/hooks/useOnboardingDetection.ts`:
- Around line 2-4: Replace the relative store imports in
useOnboardingDetection.ts with the project path aliases: swap imports of
useSettingsStore, useClaudeProfileStore, and useDialogStore to use
'@/stores/settings-store', '@/stores/claude-profile-store', and
'@/stores/dialog-store' respectively so they resolve via the tsconfig.json alias
rather than relative paths.
In `@apps/frontend/src/renderer/hooks/useTaskSync.ts`:
- Around line 21-23: The current tasks.find call may false-match when
selectedTask.specId is undefined because t.specId === selectedTask.specId will
be true for other undefined specIds; change the predicate so the specId
comparison is only evaluated when selectedTask.specId is defined (e.g., check
selectedTask.specId !== undefined before comparing), keeping the id match as-is;
update the expression around updatedTask / tasks.find / selectedTask.specId
accordingly to guard the specId equality.
- Around line 37-55: The current useTaskSync effect does many JSON.stringify
comparisons (subtasksChanged, logsChanged, metadataChanged,
executionProgressChanged, qaReportChanged) on every tasks update which is
expensive for large payloads; modify useTaskSync to first perform a cheap
early-exit by checking referential equality (if updatedTask === selectedTask
return/skip) and only run deep/shipping comparisons when references differ, and
for large fields (selectedTask.subtasks, selectedTask.logs,
selectedTask.metadata) replace unconditional JSON.stringify with cheaper checks
(shallow equality, length/id checks, or only stringify when both references
differ) so that subtasksChanged, logsChanged, metadataChanged,
executionProgressChanged, qaReportChanged are computed lazily after the cheap
guard in the useTaskSync effect.
- Around line 95-96: The dependency array contradicts its comment: remove the
full selectedTask object so the effect only reacts to identity changes, not
every reference update; in useTaskSync, update the effect dependency list to
include tasks, selectedTask?.id and selectedTask?.specId (but not selectedTask)
and ensure any uses of setSelectedTask remain outside the deps to prevent
retriggering the effect on object reference changes.
In `@apps/frontend/src/renderer/hooks/useVersionWarning.ts`:
- Around line 11-31: The hook subscribes to the whole settings object causing
extra re-renders; change the subscription in useVersionWarning to select only
seenVersionWarnings from useSettingsStore (e.g. const seenVersionWarnings =
useSettingsStore(state => state.settings.seenVersionWarnings || [])) then use
that local seenVersionWarnings in the effect and dependency array instead of
settings.seenVersionWarnings; keep the existing checks against
VERSION_WARNING_275 and the call to
useDialogStore.getState().openVersionWarning().
In `@apps/frontend/src/renderer/lib/google-font-loader.ts`:
- Around line 85-87: The isFontLoaded function is creating an unnecessary array
from the Set (Array.from(loadedFonts)) just to test membership; instead iterate
the Set directly to avoid allocation. In the isFontLoaded(family: string)
implementation, replace the Array.from(loadedFonts).some(...) usage with a
direct Set iteration (e.g., for...of over loadedFonts) and check
key.startsWith(`${family}:`) returning true immediately if found, otherwise
return false.
- Around line 14-17: The default weight array is duplicated between
buildGoogleFontsUrl and loadGoogleFont causing cache-key mismatches; extract a
single shared constant (e.g., DEFAULT_GOOGLE_FONT_WEIGHTS) and replace the
inline `[400, 500, 600, 700]` in both buildGoogleFontsUrl and the cache-key
construction inside loadGoogleFont so both functions use the same canonical
default weights value.
- Around line 30-48: The loadGoogleFont function has a race where concurrent
callers create duplicate <link> tags because the key is only added to
loadedFonts on onload; fix by introducing an in-flight map (e.g. inFlightLoads:
Map<string, Promise<void>>) and change loadGoogleFont to: 1) check
inFlightLoads.get(key) and return that promise if present, 2) if not present
create the Promise that appends the <link>, store it in inFlightLoads.set(key,
promise), and inside the promise on success add the key to loadedFonts and on
finally remove the key from inFlightLoads so subsequent calls either reuse the
pending promise or see the font as loaded.
In `@apps/frontend/src/renderer/lib/theme-utils.ts`:
- Around line 103-109: applyThemeVariables currently sets CSS properties on
document.documentElement but does not add those keys to the appliedVariables
set, so variables set by loadCustomTheme remain orphaned and won't be removed by
clearAllCustomVariables; fix by routing loadCustomTheme to call applyCustomTheme
(which tracks appliedVariables and calls clearAllCustomVariables) or modify
applyThemeVariables to accept the appliedVariables Set (or return the applied
keys) and update that Set when setting properties so clearAllCustomVariables can
remove them; reference applyThemeVariables, loadCustomTheme, applyCustomTheme,
appliedVariables, and clearAllCustomVariables when making the change.
- Around line 68-94: The current parseTweakcnCSSWithSections uses regexes
(rootMatch/darkMatch) with [^}]* which stops at the first closing brace and
fails on nested braces; update the extraction logic in
parseTweakcnCSSWithSections to robustly capture the entire :root and .dark
blocks (either by using a non-greedy dotall pattern like matching across lines
or, preferably, by implementing a simple brace-aware scanner that finds the
matching closing brace to handle nested blocks) and then pass the extracted
block text to parseTweakcnCSS for lightVariables and darkVariables.
- Around line 319-328: The code is force-casting the generated id and hardcoding
UI text: replace the unchecked assertion of `id: \`custom:${theme.name}\` as
ColorThemeDefinition['id']` with a type-safe approach (either widen/extend the
`ColorTheme`/`ColorThemeDefinition` id union to include a custom pattern or
create a branded/explicit custom-id type and use that when returning custom
themes) so you don’t mask type errors, and replace the hardcoded description
string with a translated string using i18next (call
`i18next.t('theme.customImported')` from this utility) so the UI text follows
the react-i18next convention; keep references to `themes`,
`extractPreviewColors`, and the returned shape from this mapping when making the
changes.
In `@apps/frontend/src/renderer/stores/dialog-store.ts`:
- Around line 2-4: Replace the relative imports in dialog-store.ts with the
project path aliases: change the import of Project from '../../shared/types' to
use '@shared/types' and change imports of AppSection and ProjectSettingsSection
from '../components/settings/...' to use '@/components/settings/...'; update the
import statements where these symbols (Project, AppSection,
ProjectSettingsSection) are referenced so they resolve via the configured
tsconfig aliases.
In `@apps/frontend/src/renderer/stores/navigation-store.ts`:
- Around line 45-66: Replace non-exceptional console.warn calls in function
openInbuiltTerminal with console.debug (or remove them) so normal flows aren't
logged as warnings: change the two console.warn lines inside openInbuiltTerminal
(the startup log before resolving selectedProject and the success log after
creating terminal) to console.debug (or delete them), leaving console.error for
failure cases; update references to useProjectStore,
useTerminalStore.getState().addTerminal, and set({ activeView: 'terminals', ...
}) accordingly to ensure only real errors use warn/error.
- Around line 50-53: The selectedProject value is computed via an unnecessary
IIFE; replace it with a straightforward two-step declaration: destructure
projects, activeProjectId, and selectedProjectId from useProjectStore.getState()
and then set selectedProject by calling projects.find(p => p.id ===
(activeProjectId || selectedProjectId)); update the symbol references
(selectedProject, useProjectStore, projects, activeProjectId, selectedProjectId)
accordingly and remove the IIFE wrapper to improve readability.
- Around line 1-6: The navigation store currently imports the component-specific
type SidebarView from ../components/Sidebar/constants/types which inverts
dependencies; move the SidebarView type into a shared/domain types module (e.g.,
src/shared/types or src/renderer/types) and update navigation-store.ts to import
it via the path alias (e.g., `@shared/types` or `@/types`) instead of the component
path; ensure the SidebarView symbol is exported from the new module and update
any other consumers to import from the new alias so the store only depends on
shared types, not component internals.
In `@apps/frontend/src/renderer/styles/globals.css`:
- Around line 171-200: Move the duplicated CSS custom properties (--font-serif,
--spacing, --letter-spacing and the shadow decomposition vars --shadow-color,
--shadow-opacity, --shadow-blur, --shadow-spread, --shadow-offset-x,
--shadow-offset-y) out of each theme block and define them once in a global
scope: put light-mode defaults in :root and dark-mode defaults in .dark; then
remove these identical properties from all 14 theme variant blocks and leave
only properties that actually differ (e.g., --chart-*, --sidebar-*,
--sidebar-*-foreground, --sidebar-border, --sidebar-ring). Apply the same change
to the other duplicated region mentioned (lines 262-291) so theme blocks only
contain overrides.
- Line 179: The CSS custom property declarations for --font-serif use
capitalized font family keywords which trigger Stylelint's value-keyword-case
rule; update each occurrence of --font-serif (and any other font-family custom
properties flagged) to use lowercase keywords (e.g., change Georgia → georgia,
Cambria → cambria, Times → times) while preserving quoted names like 'Times New
Roman' and the generic family serif; search for the --font-serif declarations
listed in the review and replace the capitalized keywords with their lowercase
equivalents to satisfy Stylelint.
- Around line 1540-1557: There are two duplicate ::-webkit-scrollbar-thumb
rules; merge them so the background, border-radius and visibility are defined in
a single rule to avoid confusion and accidental overrides. Update the stylesheet
by combining the properties from the first ::-webkit-scrollbar-thumb block
(background, border-radius) with the later visibility: hidden declaration into
one ::-webkit-scrollbar-thumb selector, leaving the
::-webkit-scrollbar-thumb:hover and the hover-visibility rules as-is.
- Line 11: The CSS declares a circular variable (--font-serif:
var(--font-serif)) inside your `@theme` blocks which yields an invalid value;
change the theme token name used for the per-theme stack (e.g. rename the
variable set in each `@theme` from --font-serif to --font-serif-stack) and then
map the global Tailwind-facing variable to that indirection (use --font-serif:
var(--font-serif-stack) or define the font-family directly) so Tailwind's
font-serif utility reads a valid font-family; update all theme-block definitions
and any references to --font-serif to use the new --font-serif-stack token (or
replace with an explicit font-family) and ensure the mapping occurs outside the
circular reference.
apps/frontend/src/renderer/components/TopNavBar/__tests__/ProjectTabBar.test.tsx
Show resolved
Hide resolved
…ality - Fix broken CSS `rounded-(--radius)` → `rounded-[var(--radius)]` (Tailwind v4) - Fix circular `--font-serif` CSS variable reference - Fix useTopNavBar subscribing to function ref instead of derived data - Fix loadCustomTheme not tracking applied CSS variables for cleanup - Add async/await to removeProject dialog handler - Add null guard for specId matching in useTaskSync - Remove stale selectedTask object ref from useTaskSync dep array - Replace hardcoded English error strings with i18n keys (en + fr) - Narrow useSidebar git status effect deps to primitive values - Prevent duplicate Google Font link tags via pending load tracking - Handle unhandled promise rejection in WindowControls - Rewrite SortableProjectTab tests to validate real logic patterns - Remove dead code from useSidebar (unused handler, exports, import) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/frontend/src/renderer/styles/globals.css (4)
1630-1711:⚠️ Potential issue | 🟠 MajorDuplicate
.progress-workingand.progress-working::after— first block is dead code.
.progress-workingis defined twice (Lines 1630–1633 and 1687–1690) with identical properties. More critically,.progress-working::afteris also defined twice with different content:
- Lines 1635–1654: uses
progress-glow-sweepanimation with theme-colored gradients (rgba(214, 216, 118, …)) andleft:0; right:0; bottom:0positioning.- Lines 1692–1706: uses
progress-sweepanimation with a white gradient andleft:-100%; width:100%; height:100%positioning.The second
::afterblock wins the cascade (same specificity, later source order), making the first block + its light-mode override (Lines 1656–1667) effectively dead code. Theprogress-glow-sweepkeyframes (Lines 1621–1628) are also unreachable.Decide which visual treatment you want and remove the other. If the glow-sweep variant is intended, delete Lines 1687–1711; if the simple white shimmer is intended, delete Lines 1621–1667.
1714-1721:⚠️ Potential issue | 🟡 MinorDuplicate
.status-runningwith conflicting animation names.Lines 1714–1716 assign
animation: status-pulse …, while Lines 2063–2064 assignanimation: status-running-pulse …. The second definition wins, making thestatus-pulsekeyframes (Lines 1718–1721) dead code.Consolidate into a single definition and remove the orphaned keyframes.
♻️ Proposed fix — keep the second definition and remove the first
-/* Status badge pulse for running tasks */ -.status-running { - animation: status-pulse 2s ease-in-out infinite; -} - -@keyframes status-pulse { - 0%, 100% { opacity: 1; } - 50% { opacity: 0.7; } -}Also applies to: 2062-2074
1766-1776: 🛠️ Refactor suggestion | 🟠 MajorDuplicate
.subtask-dotrules.
.subtask-dotis defined at Lines 1766–1776 (withwidth,height,border-radius,transition,cursor, and:hover) and again at Lines 1896–1902 (withtransitionand:hover). The second block'stransitionvalue (all 0.2s cubic-bezier(…)) overrides the first (transform 0.15s ease), and the duplicate:hoverrule is identical.Merge into a single definition.
Also applies to: 1896-1902
1788-1804:⚠️ Potential issue | 🟡 MinorDuplicate
.section-divider— second block overrides the first with different styles.Lines 1788–1804 define
.section-dividerwithgap: 8px,font-size: 10px,font-weight: 500, and a::afterpseudo-element for a horizontal line. Lines 2045–2056 redefine it withgap: 0.5rem,font-size: 0.625rem,font-weight: 600, and aborder-bottominstead. The second block wins for all overlapping properties, making the first a partial dead declaration. The::afterfrom the first block still applies though, producing both aborder-bottomand an::afterhorizontal line — likely unintended.Consolidate into one
.section-dividerwith the desired visual treatment.Also applies to: 2045-2060
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/components/AppDialogs.tsx`:
- Around line 44-61: The code currently calls useDialogStore 17 times (e.g.,
isNewTaskDialogOpen, isSettingsDialogOpen, settingsInitialSection,
pendingProject, isInitializing, initError, initSuccess, showGitHubSetup,
gitHubSetupProject, showRemoveProjectDialog, removeProjectError,
projectToRemove, isOnboardingWizardOpen, isVersionWarningModalOpen), creating
many subscriptions; consolidate these into a single selector using Zustand's
shallow comparator (useShallow) so you select an object of all required
properties in one useDialogStore call to keep atomic updates while reducing
verbosity and maintenance overhead.
- Around line 1-35: Update the import statements in AppDialogs.tsx to use the
project path alias instead of relative paths (replace imports like
'./ui/button', '../stores/navigation-store', etc. with '@/renderer/...' per
tsconfig alias) and consolidate duplicate imports from the same module into
single statements (combine useProjectStore and removeProject into one import
from the project-store module). Ensure you update all imports listed
(useTranslation, Button, Dialog components, TaskDetailModal, TaskCreationWizard,
AppSettingsDialog, AddProjectModal, GitHubSetupModal, RateLimitModal,
SDKRateLimitModal, AuthFailureModal, VersionWarningModal, OnboardingWizard,
AppUpdateNotification, GlobalDownloadIndicator, Toaster, useNavigationStore,
useDialogStore, useProjectStore, removeProject, useSettingsStore,
handleInitialize, handleGitHubSetupComplete, handleProjectAdded,
handleVersionWarningClose) to use the `@/` alias and remove any duplicate import
lines.
In `@apps/frontend/src/renderer/components/Sidebar/hooks/useSidebar.ts`:
- Around line 49-75: The effect currently sets lastLoadedProjectIdRef.current
before awaiting loadProjectEnvConfig, so a concurrent effect can overwrite the
ref and a stale load may still mutate the store; change the logic in useEffect
(symbols: lastLoadedProjectIdRef, loadProjectEnvConfig, isCurrent,
clearProjectEnvConfig, selectedProject) to either 1) generate a local
marker/requestId and pass it into loadProjectEnvConfig so the store can reject
stale results, or 2) defer updating lastLoadedProjectIdRef.current until after
loadProjectEnvConfig resolves and you confirm isCurrent is still true, ensuring
the ref is only mutated for successful, current loads; keep
clearProjectEnvConfig behavior unchanged for no-project cases.
- Around line 13-16: The Sidebar is over-subscribing to the whole settings
object via useSettingsStore((state) => state.settings); change the selector to
read only the sidebarCollapsed field so the component only re-renders when that
value changes—e.g. replace the current lines and set isCollapsed using
useSettingsStore((state) => state.settings.sidebarCollapsed) ?? false (or
useSettingsStore((state) => state.sidebarCollapsed) ?? false if the store
exposes it at top-level) and remove the intermediate settings variable.
- Around line 141-157: The handlers handleGitInitialized and handleNavClick
should be wrapped in React.useCallback to stabilize their references: change
handleGitInitialized into a useCallback that depends on selectedProject and
setGitStatus (and window.electronAPI usage assumed stable) and change
handleNavClick into a useCallback that depends on onViewChange; also ensure
useCallback is imported from React if not already. This will ensure the hook
returns stable function refs and avoid unnecessary child re-renders.
In `@apps/frontend/src/renderer/components/TopNavBar/hooks/useTopNavBar.ts`:
- Line 13: The selector useProjectStore((state) => state.getProjectTabs())
returns a fresh array each call (getProjectTabs uses map/filter), causing
re-renders; fix by using a shallow equality comparator so referentially-equal
arrays don't trigger updates — change the hook to pass shallow as the second
argument (e.g., useProjectStore((state) => state.getProjectTabs(), shallow)) and
import shallow from 'zustand/shallow'; keep the selector as getProjectTabs but
add the comparator to prevent excess re-renders.
In `@apps/frontend/src/renderer/hooks/useAppEventListeners.ts`:
- Around line 104-111: The code currently calls useDialogStore.setState(...)
directly which bypasses the store's actions; replace this with a store action
(either existing skipInit or a new closeInitDialog) so any extra teardown logic
(initError/initSuccess/isInitializing) runs in one place—add closeInitDialog to
dialog-store.ts (or use skipInit if present) to set showInitDialog:false,
pendingProject:null and reset initError/initSuccess/isInitializing, then call
useDialogStore.getState().closeInitDialog() here instead of
useDialogStore.setState(...), and keep the subsequent
openGitHubSetup(updatedProject) call unchanged.
- Around line 1-8: Change the module imports in useAppEventListeners.ts to use
the project path alias (replace relative paths like
'../stores/navigation-store', '../stores/dialog-store',
'../stores/project-store', '../../shared/types', '../../shared/i18n', and
'../components/settings/AppSettings' with the corresponding '@/...' alias
imports) and remove the unused named import useTranslation; keep using the i18n
default import for non-React contexts and ensure imported symbols referenced in
the file (useNavigationStore, useDialogStore, useProjectStore, addProject,
initializeProject, loadProjects, AppSection, Project, i18n) are adjusted to the
new alias paths.
In `@apps/frontend/src/renderer/hooks/useTaskSync.ts`:
- Around line 2-4: Replace the relative imports in useTaskSync.ts with the
project path aliases: change the import of debugLog to use the `@shared` alias
(e.g., `@shared/`.../debug-logger) and change useTaskStore and useNavigationStore
to use the @ alias (e.g., `@/stores/task-store` and `@/stores/navigation-store`);
update the import statements that reference debugLog, useTaskStore, and
useNavigationStore accordingly so they follow the tsconfig.json alias mappings.
In `@apps/frontend/src/renderer/lib/google-font-loader.ts`:
- Around line 78-91: The CSS variable format for fonts is inconsistent between
applyThemeFonts (which sets --font-sans to a quoted name plus generic fallback)
and applyCustomTheme (which writes raw theme.fonts.sans); update
applyCustomTheme (and any theme serialization) to normalize font values the same
way applyThemeFonts does: when setting --font-sans/--font-serif/--font-mono from
theme.fonts.*, wrap the family name in single quotes and append the appropriate
generic fallback (e.g. '..., sans-serif' / '..., serif' / '..., monospace'), or
alternatively ensure the theme stores the fully-formatted value and use that
consistently; locate usages in applyThemeFonts, applyCustomTheme, and
theme.fonts.* to make the change.
In `@apps/frontend/src/renderer/lib/theme-utils.ts`:
- Line 9: Replace the relative import for ColorThemeDefinition with the
configured path alias: update the import in
apps/frontend/src/renderer/lib/theme-utils.ts that currently references
'../../shared/types/settings' to use '@shared/types/settings' so the symbol
ColorThemeDefinition is imported via the tsconfig alias.
apps/frontend/src/renderer/components/Sidebar/hooks/useSidebar.ts
Outdated
Show resolved
Hide resolved
apps/frontend/src/renderer/components/Sidebar/hooks/useSidebar.ts
Outdated
Show resolved
Hide resolved
…nents Clean up package.json dependencies for component files that were already removed: accordion, aspect-ratio, avatar, command, context-menu, hover-card, menubar, navigation-menu, slider, toggle, toggle-group. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - Merge conflicts must be resolved before merge.
Blocked: PR has merge conflicts with base branch. Resolve conflicts before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Merge Conflicts: PR has conflicts with base branch that must be resolved
Findings Summary
- Medium: 4 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (5 selected of 5 total)
🟡 [7ac83ce8a5b7] [MEDIUM] Zustand selector calls getProjectTabs() causing unnecessary re-renders
📁 apps/frontend/src/renderer/components/TopNavBar/hooks/useTopNavBar.ts:13
The selector (state) => state.getProjectTabs() invokes getProjectTabs() which creates a new array on every call. Zustand uses Object.is comparison by default, so a new array reference means this selector ALWAYS reports a change on any store update — even unrelated ones (e.g., a settings update). This causes the useTopNavBar hook and the entire TopNavBar component tree (including DnD sensors) to re-render on every project-store mutation.
Compare with the correct pattern in useAppInitialization.ts:24 which gets the function reference: const getProjectTabs = useProjectStore((state) => state.getProjectTabs) and calls it outside the selector.
Suggested fix:
Either (1) select primitive dependencies and compute tabs outside: `const projects = useProjectStore(s => s.projects); const openIds = useProjectStore(s => s.openProjectIds); const tabOrder = useProjectStore(s => s.tabOrder); const projectTabs = useMemo(() => ..., [projects, openIds, tabOrder])`, or (2) use Zustand's `useShallow` to compare by shallow equality: `const projectTabs = useProjectStore(useShallow((state) => state.getProjectTabs()))`.
🟡 [ce3be17410c7] [MEDIUM] Hook file exports both React hook and standalone functions — mixed concerns
📁 apps/frontend/src/renderer/hooks/useAppEventListeners.ts:83
The file useAppEventListeners.ts exports a React hook (useAppEventListeners at line 14) alongside 4 standalone functions (handleInitialize at line 83, handleGitHubSetupComplete at line 129, handleProjectAdded at line 161, handleProjectTabClose at line 171). These standalone functions are stateless utilities that read from Zustand stores — they are not hooks. Mixing hooks and utilities in a file named with the use prefix violates the React hook naming convention and makes the module harder to understand. The AppDialogs component imports these standalone functions from a hook file, creating a confusing dependency. | This hook file exports 5 standalone functions (handleInitialize, handleGitHubSetupComplete, handleProjectAdded, handleProjectTabClose, handleVersionWarningClose) that are not React hooks but pure business logic operating on stores. In the established codebase pattern, action functions that operate on stores are exported as standalone async functions FROM THE STORE FILES THEMSELVES (e.g., export async function loadProjects() in project-store.ts, export async function loadTasks() in task-store.ts, export async function saveSettings() in settings-store.ts). These handler functions should live in dialog-store.ts or navigation-store.ts since they primarily manipulate dialog/navigation state. Searched Grep('export (async )?function (load|save|add|remove|initialize|update)', 'stores/') - confirmed 30+ standalone action functions exported from store files, which is the established pattern.
Suggested fix:
Extract standalone functions into a dedicated utility file (e.g., `lib/dialog-actions.ts` or `utils/project-actions.ts`). Keep only the `useAppEventListeners` hook in the hook file. This maintains the convention that `use*` files only export hooks.
🔵 [173e7f704dec] [LOW] useEffect dependency array includes projectTabs.map — non-reactive value
📁 apps/frontend/src/renderer/hooks/useAppInitialization.ts:93
The dependency array includes projectTabs.map which is a reference to Array.prototype.map. This is always the same function reference and never changes, making it a no-op dependency. The eslint-disable comment explains the intentional omission of projectTabs but doesn't mention projectTabs.map. This carried over from the original App.tsx but remains confusing for future maintainers.
Suggested fix:
Remove `projectTabs.map` from the dependency array. It serves no purpose since `Array.prototype.map` is a constant reference. The `projectTabs.length` dependency is sufficient for tracking meaningful tab count changes.
🟡 [c3854c759855] [MEDIUM] AppDialogs rendered outside TooltipProvider breaks shared tooltip behavior in dialogs
📁 apps/frontend/src/renderer/App.tsx:44
AppDialogs is rendered as a sibling of TooltipProvider, not as a child. In the old App.tsx, ALL content (including dialogs, task detail modal, settings, onboarding wizard) was inside the TooltipProvider. Now AppDialogs is outside it.
Radix UI's TooltipProvider manages shared delay state for tooltips: delayDuration (shared delay before any tooltip shows) and skipDelayDuration (when hovering quickly between tooltips, the delay is skipped for instant feedback). Without the provider, each Tooltip creates its own independent context.
This affects 78+ tooltip usages in task-detail components and 85+ in settings components. Tooltips will still render, but lose the shared delay optimization - quickly hovering between tooltip targets in dialogs will always show the full delay instead of instant switching.
Searched via Grep for Tooltip usage in task-detail (78 occurrences across 6 files) and settings (85 occurrences across 4 files) to confirm these dialogs extensively use tooltips.
Suggested fix:
Move AppDialogs inside the TooltipProvider:
```tsx
<ViewStateProvider>
<TooltipProvider>
<ProactiveSwapListener />
<AppShell />
<AppDialogs />
</TooltipProvider>
</ViewStateProvider>
#### 🟡 [54ee0d4b0ff5] [MEDIUM] PR adds dependencies it claims to remove - contradicts PR description
📁 `apps/frontend/package.json:48`
The PR description states 'Removed 16 unused shadcn/ui components (accordion, alert, avatar, command, context-menu, hover-card, menubar, navigation-menu, sheet, sidebar, skeleton, slider, toggle, toggle-group, aspect-ratio, use-mobile hook)' and 'Cleaned barrel export in ui/index.ts'. However, the package.json diff shows these Radix UI packages being ADDED as new dependencies: @radix-ui/react-accordion, @radix-ui/react-aspect-ratio, @radix-ui/react-avatar, @radix-ui/react-context-menu, @radix-ui/react-hover-card, @radix-ui/react-menubar, @radix-ui/react-navigation-menu, @radix-ui/react-slider, @radix-ui/react-toggle, @radix-ui/react-toggle-group, and cmdk. These packages are not imported anywhere in the codebase (confirmed via grep). Searched `Grep('@radix-ui/react-accordion|@radix-ui/react-avatar|@radix-ui/react-context-menu|@radix-ui/react-hover-card|@radix-ui/react-menubar|@radix-ui/react-navigation-menu|@radix-ui/react-slider|@radix-ui/react-toggle|@radix-ui/react-toggle-group|cmdk', 'apps/frontend/src/')` - found zero imports of these packages.
**Suggested fix:**
Remove the 11 unused packages from package.json dependencies. These appear to have been accidentally re-added (possibly by a package manager auto-resolution or a merge conflict). The PR description's claim of removing them is correct in intent but the package.json tells the opposite story.
---
*This review was generated by Auto Claude.*
Merge upstream/develop into feature branch. Resolved conflicts: - package.json: Keep our tab formatting + Radix removals, accept version bump to 2.7.6-beta.5 - App.tsx: Keep our decomposed architecture (HEAD) - en/common.json & fr/common.json: Include both our windowControls keys and upstream's githubErrors keys - Sidebar.tsx: Keep our deletion, port upstream's sponsor link into refactored Sidebar/index.tsx - package-lock.json: Accept upstream version Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rename shadowed `t` variable in ThemeSelector map callback to `theme`
- Fix inconsistent indentation in Sidebar constants/types.ts
- Simplify error-boundary i18n namespace prefix and remove redundant error guard
- Add clarifying comment for intentional onNewProject/onOpenProject behavior
- Downgrade normal-flow console.warn to console.debug in navigation-store
- Simplify IIFE project lookup in navigation-store
- Lowercase font family names in CSS per linting convention
- Merge duplicate ::-webkit-scrollbar-thumb rules
- Remove unnecessary !important from scroll-area hover opacity
- Remove redundant { ns: 'common' } from AppDialogs i18n calls
- Downgrade debug console.warn to console.debug in AppDialogs
- Narrow useVersionWarning store subscription to seenVersionWarnings
- Eliminate unnecessary Array.from in google-font-loader isFontLoaded
- Extract shared DEFAULT_WEIGHTS constant in google-font-loader
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| selectedProjectId, | ||
| isCollapsed, | ||
| toggleSidebar, | ||
| } |
Check notice
Code scanning / CodeQL
Semicolon insertion Note
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/frontend/src/renderer/components/ui/error-boundary.tsx (1)
81-92: 🧹 Nitpick | 🔵 TrivialNon-null assertion on
this.state.erroris acceptable but could be tightened.The
!on line 89 is safe becausegetDerivedStateFromErroralways setserroralongsidehasError: true. However, guarding onthis.state.errordirectly would eliminate the assertion and be slightly more defensive:♻️ Optional: guard on error directly
render(): React.ReactNode { - if (this.state.hasError) { + if (this.state.hasError && this.state.error) { if (this.props.fallback) { return this.props.fallback; } return ( <ErrorBoundaryFallback - error={this.state.error!} + error={this.state.error} onReset={this.handleReset} /> ); }apps/frontend/src/renderer/styles/globals.css (4)
1627-1708:⚠️ Potential issue | 🟡 MinorDuplicate
.progress-working::after— second rule silently overrides the first.Lines 1632–1651 define a theme-aware glow-sweep animation (
progress-glow-sweep), while lines 1689–1703 redefine the same::afterpseudo-element with a plain white shimmer (progress-sweep). Because both selectors have identical specificity, the later rule wins entirely, making the first block (plus its light-mode variant at lines 1654–1664 and theprogress-glow-sweepkeyframe at lines 1618–1625) dead code.Decide which animation is intended and remove the other.
♻️ If the theme-aware glow sweep is intended, remove the second block
-/* Progress bar working animation */ -.progress-working { - position: relative; - overflow: hidden; -} - -.progress-working::after { - content: ''; - position: absolute; - top: 0; - left: -100%; - width: 100%; - height: 100%; - background: linear-gradient( - 90deg, - transparent 0%, - rgba(255, 255, 255, 0.15) 50%, - transparent 100% - ); - animation: progress-sweep 2s ease-in-out infinite; -} - -@keyframes progress-sweep { - 0% { transform: translateX(0); } - 100% { transform: translateX(200%); } -}
1763-1773: 🛠️ Refactor suggestion | 🟠 MajorDuplicate
.subtask-dotrules — merge into one.The first definition (lines 1763–1773) sets dimensions, border-radius, cursor, and
:hover. The second (lines 1892–1899) redefinestransitionand:hoverwith the same effect. Consolidate into a single rule to avoid confusion and potential cascade surprises.Also applies to: 1892-1899
2042-2057:⚠️ Potential issue | 🟡 MinorDuplicate
.section-divideroverrides the earlier definition (lines 1784–1801).The first rule uses
gap: 8px,font-size: 10px, and an::afterpseudo-element for the line. The second changesgap,font-size, removes the::aftersemantics (though the pseudo-element still renders from the first rule), and addsborder-bottom+padding-bottom. These competing definitions produce a confusing merged result. Remove one and keep a single canonical definition.
2059-2071:⚠️ Potential issue | 🟡 MinorDuplicate
.status-running— first definition (lines 1710–1718) usesstatus-pulsekeyframe, this one usesstatus-running-pulse.The second animation declaration wins, making the
status-pulsekeyframe at lines 1715–1718 dead code. Remove the earlier block or consolidate.
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/components/AppDialogs.tsx`:
- Line 63: AppDialogs currently subscribes to the entire settings object via
useSettingsStore((state) => state.settings) causing unnecessary re-renders;
change the selector to only subscribe to the needed property
(useSettingsStore((state) => state.settings.autoBuildPath) or
useSettingsStore((state) => state.autoBuildPath) depending on store shape) and
update references in AppDialogs where settings.autoBuildPath is used (replace
settings.autoBuildPath with the new autoBuildPath variable) so only
autoBuildPath triggers re-renders.
- Around line 116-122: Replace the disallowed console.debug call in the Dialog
onOpenChange handler with an allowed logging level or remove it: update the
anonymous onOpenChange function (the handler that references showInitDialog,
pendingProject, isInitializing, initSuccess and calls
useDialogStore.getState().skipInit()) to either remove the console.debug line or
change it to console.warn with the same diagnostic payload so it complies with
the ESLint/Biome rule that only allows console.warn and console.error.
In `@apps/frontend/src/renderer/components/settings/ThemeSelector.tsx`:
- Around line 49-62: This component holds seven local useState hooks for
custom-theme dialogs (customThemeDefs, isAddDialogOpen, isDeleteDialogOpen,
deleteTargetName, themeName, themeCSS, validationError) and a
refreshCustomThemes callback; extract the dialog-related state and actions into
a Zustand store (e.g., create a useThemeDialogStore under the renderer stores)
exposing isAddDialogOpen, isDeleteDialogOpen, deleteTargetName, themeName,
themeCSS, validationError and actions to open/close dialogs, set fields, and
refresh themes so other components can trigger dialogs; update ThemeSelector to
read/write from the new store and keep only customThemeDefs and
refreshCustomThemes local if desired (or move refresh into the store) and ensure
you import the store from src/renderer/stores per project guidelines.
- Around line 166-169: The parameter name themeName in
handleOpenDeleteDialog(themeName: string) shadows the component state variable
themeName; rename the parameter (for example to targetName or selectedThemeName)
in the handleOpenDeleteDialog declaration and update all call sites to pass the
same value using the new parameter name so the state variable themeName remains
unshadowed and readable; ensure you update references inside
handleOpenDeleteDialog (setDeleteTargetName) accordingly and run type checks to
catch any missed call sites.
In `@apps/frontend/src/renderer/components/Sidebar/constants/types.ts`:
- Around line 1-3: The SidebarView union type is defined inside a component area
but is imported and used by navigation-store.ts, creating an undesirable
stores→components dependency; move the SidebarView type into a shared types
module (e.g., create or use `@shared/types` or similar) and update imports: export
SidebarView from the new shared module and change references in
navigation-store.ts and any other consumers to import SidebarView from that
shared location (leave the lucide-react icon imports in the component file
only). Ensure the exported name remains SidebarView so functionally no other
code needs renaming.
- Around line 12-17: The NavItem interface uses React.ElementType without an
explicit type import; update the file to import the type by adding an import
type { ElementType } from 'react' and change the icon property's type to
ElementType; also audit other files mentioned (AgentProfileSelector.tsx,
context/constants.ts, terminal/types.ts) and replace React.ElementType with the
imported ElementType to follow the project's type-import convention.
In `@apps/frontend/src/renderer/components/Sidebar/index.tsx`:
- Around line 23-25: The import line imports SidebarProps as a value but it is a
type; change the import to a type-only import so SidebarProps is imported using
the type modifier (matching NavItem and SidebarView) — update the import that
currently references SidebarProps and NavItem so SidebarProps is prefixed with
"type" (e.g., import type { SidebarProps, type NavItem } ...), leaving the
export of SidebarView and useSidebar import unchanged.
- Around line 33-46: The destructured assignment from useSidebar has
inconsistent indentation between the property list and the closing brace/call;
reformat the block so indentation is consistent (align the closing brace and the
call arguments with the opening const { line) in the Sidebar component where
useSidebar is invoked, keeping the same identifiers (showGitSetupModal,
setShowGitSetupModal, gitStatus, t, selectedProjectId, visibleNavItems,
handleNavClick, handleGitInitialized, selectedProject, isCollapsed,
toggleSidebar) and the call arguments (onSettingsClick, onNewTaskClick,
activeView, onViewChange) unchanged.
In `@apps/frontend/src/renderer/components/ui/scroll-area.tsx`:
- Line 9: Replace the relative import of the utility helper in scroll-area.tsx
with the project path alias: change the existing import that pulls cn from
'../../lib/utils' to use the '@lib' alias (e.g., import { cn } from
'@lib/utils'), ensuring the symbol name cn and any usages in the file remain
unchanged so the module resolution uses the tsconfig alias.
In `@apps/frontend/src/renderer/hooks/useVersionWarning.ts`:
- Around line 37-46: The handleVersionWarningClose function currently calls
saveSettings (which returns Promise<boolean>) without awaiting it, so failures
can let the warning reappear; update handleVersionWarningClose to be async,
await the result of saveSettings when persisting seenVersionWarnings
(referencing handleVersionWarningClose, saveSettings,
useDialogStore.getState().closeVersionWarning,
useSettingsStore.getState().settings, VERSION_WARNING_275), and add minimal
error handling (try/catch or check the boolean) to log or swallow failures so a
failed persistence is handled deterministically.
In `@apps/frontend/src/renderer/lib/google-font-loader.ts`:
- Around line 73-97: The applyThemeFonts function currently awaits
Promise.all(loadPromises) which will reject on the first failed load; change it
to use Promise.allSettled(loadPromises) and handle each result so successful
fonts remain applied and failures are logged or warned. Specifically, update
applyThemeFonts to call Promise.allSettled(loadPromises), iterate the settled
results and for any result.status === "rejected" log a warning including the
font name (from the corresponding loadGoogleFont call context) and the error,
while keeping the existing root.style.setProperty calls for fonts that were
requested; ensure loadGoogleFont is still invoked for each configured font so
the settling logic can inspect per-font outcomes.
In `@apps/frontend/src/renderer/stores/navigation-store.ts`:
- Around line 35-43: goToTask currently stores a snapshot of the task object
(selectedTask) taken from useTaskStore.getState().tasks which can go stale when
the task is updated; change the navigation store to store only selectedTaskId
(e.g., replace selectedTask with selectedTaskId in the store and update goToTask
to set selectedTaskId = taskId) and update all consumers to derive the live task
via useTaskStore (or create a selector that maps selectedTaskId → current task),
or alternatively subscribe the navigation store to useTaskStore changes so
selectedTask is refreshed whenever the underlying task list changes.
In `@apps/frontend/src/renderer/styles/globals.css`:
- Around line 1825-1833: Merge the two consecutive .task-card-enhanced:hover
rules into a single rule that contains z-index, border-color, box-shadow, and
transform together; locate the duplicate selector .task-card-enhanced:hover in
globals.css, combine the properties into one block and remove the redundant rule
so hover styles are defined in a single place.
| const isVersionWarningModalOpen = useDialogStore((state) => state.isVersionWarningModalOpen); | ||
| const initSuccess = useDialogStore((state) => state.initSuccess); | ||
|
|
||
| const settings = useSettingsStore((state) => state.settings); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Broad settings store subscription — only autoBuildPath is needed.
Line 63 subscribes to the entire settings object, but only settings.autoBuildPath is used (Lines 142, 175). This causes unnecessary re-renders on any settings change.
Proposed fix
- const settings = useSettingsStore((state) => state.settings);
+ const autoBuildPath = useSettingsStore((state) => state.settings.autoBuildPath);Then replace settings.autoBuildPath with autoBuildPath on Lines 142 and 175.
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/AppDialogs.tsx` at line 63, AppDialogs
currently subscribes to the entire settings object via useSettingsStore((state)
=> state.settings) causing unnecessary re-renders; change the selector to only
subscribe to the needed property (useSettingsStore((state) =>
state.settings.autoBuildPath) or useSettingsStore((state) =>
state.autoBuildPath) depending on store shape) and update references in
AppDialogs where settings.autoBuildPath is used (replace settings.autoBuildPath
with the new autoBuildPath variable) so only autoBuildPath triggers re-renders.
| {/* Initialize Auto Claude Dialog */} | ||
| <Dialog open={showInitDialog} onOpenChange={(open) => { | ||
| console.debug('[InitDialog] onOpenChange called', { open, pendingProject: !!pendingProject, isInitializing, initSuccess }); | ||
| if (!open && pendingProject && !isInitializing && !initSuccess) { | ||
| useDialogStore.getState().skipInit(); | ||
| } | ||
| }}> |
There was a problem hiding this comment.
console.debug likely violates the ESLint configuration.
Line 118 uses console.debug for the init dialog state logging. The project's ESLint/Biome configuration only allows console.warn and console.error. Either remove this log or use console.warn if the diagnostic information is important enough to retain.
Based on learnings: "In the AndyMik90/Auto-Claude repository, the ESLint configuration only allows console.warn and console.error levels, not console.log or console.info."
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/AppDialogs.tsx` around lines 116 - 122,
Replace the disallowed console.debug call in the Dialog onOpenChange handler
with an allowed logging level or remove it: update the anonymous onOpenChange
function (the handler that references showInitDialog, pendingProject,
isInitializing, initSuccess and calls useDialogStore.getState().skipInit()) to
either remove the console.debug line or change it to console.warn with the same
diagnostic payload so it complies with the ESLint/Biome rule that only allows
console.warn and console.error.
| // Custom theme state | ||
| const [customThemeDefs, setCustomThemeDefs] = useState<ColorThemeDefinition[]>(() => | ||
| getAllCustomThemeDefinitions() | ||
| ); | ||
| const [isAddDialogOpen, setIsAddDialogOpen] = useState(false); | ||
| const [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false); | ||
| const [deleteTargetName, setDeleteTargetName] = useState<string | null>(null); | ||
| const [themeName, setThemeName] = useState(''); | ||
| const [themeCSS, setThemeCSS] = useState(''); | ||
| const [validationError, setValidationError] = useState<string | null>(null); | ||
|
|
||
| const refreshCustomThemes = useCallback(() => { | ||
| setCustomThemeDefs(getAllCustomThemeDefinitions()); | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Heavy use of local useState for dialog state — consider whether this could live in a store.
The component manages 7 state variables for the two dialogs (add/delete). While this works, if other components ever need to trigger the custom-theme dialog (e.g., from a command palette), this local state would be unreachable. For now this is fine, but worth noting for future extensibility.
As per coding guidelines: "Use Zustand state management through stores in src/renderer/stores/. Organize state by domain."
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/settings/ThemeSelector.tsx` around
lines 49 - 62, This component holds seven local useState hooks for custom-theme
dialogs (customThemeDefs, isAddDialogOpen, isDeleteDialogOpen, deleteTargetName,
themeName, themeCSS, validationError) and a refreshCustomThemes callback;
extract the dialog-related state and actions into a Zustand store (e.g., create
a useThemeDialogStore under the renderer stores) exposing isAddDialogOpen,
isDeleteDialogOpen, deleteTargetName, themeName, themeCSS, validationError and
actions to open/close dialogs, set fields, and refresh themes so other
components can trigger dialogs; update ThemeSelector to read/write from the new
store and keep only customThemeDefs and refreshCustomThemes local if desired (or
move refresh into the store) and ensure you import the store from
src/renderer/stores per project guidelines.
| const handleOpenDeleteDialog = (themeName: string) => { | ||
| setDeleteTargetName(themeName); | ||
| setIsDeleteDialogOpen(true); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parameter themeName shadows the state variable of the same name.
handleOpenDeleteDialog(themeName: string) shadows the themeName state declared on Line 56. While it's not a bug here (the state isn't accessed inside this function), it harms readability and could lead to subtle issues if the function body is later extended.
Proposed fix
- const handleOpenDeleteDialog = (themeName: string) => {
- setDeleteTargetName(themeName);
+ const handleOpenDeleteDialog = (name: string) => {
+ setDeleteTargetName(name);
setIsDeleteDialogOpen(true);
};🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/settings/ThemeSelector.tsx` around
lines 166 - 169, The parameter name themeName in
handleOpenDeleteDialog(themeName: string) shadows the component state variable
themeName; rename the parameter (for example to targetName or selectedThemeName)
in the handleOpenDeleteDialog declaration and update all call sites to pass the
same value using the new parameter name so the state variable themeName remains
unshadowed and readable; ensure you update references inside
handleOpenDeleteDialog (setDeleteTargetName) accordingly and run type checks to
catch any missed call sites.
apps/frontend/src/renderer/components/Sidebar/constants/types.ts
Outdated
Show resolved
Hide resolved
|
|
||
| import * as React from 'react'; | ||
| import * as ScrollAreaPrimitive from '@radix-ui/react-scroll-area'; | ||
| import { cn } from '../../lib/utils'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using the @lib/* path alias instead of a relative import.
Per the project's tsconfig.json aliases (@lib/* → src/renderer/shared/lib/*), this could be:
-import { cn } from '../../lib/utils';
+import { cn } from '@lib/utils';This is a minor consistency nit — the relative path works but diverges from the alias convention. As per coding guidelines, use path aliases defined in tsconfig.json: @lib/* → src/renderer/shared/lib/*.
📝 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.
| import { cn } from '../../lib/utils'; | |
| import { cn } from '@lib/utils'; |
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/ui/scroll-area.tsx` at line 9, Replace
the relative import of the utility helper in scroll-area.tsx with the project
path alias: change the existing import that pulls cn from '../../lib/utils' to
use the '@lib' alias (e.g., import { cn } from '@lib/utils'), ensuring the
symbol name cn and any usages in the file remain unchanged so the module
resolution uses the tsconfig alias.
| export function handleVersionWarningClose() { | ||
| useDialogStore.getState().closeVersionWarning(); | ||
| const settings = useSettingsStore.getState().settings; | ||
| const seenWarnings = settings.seenVersionWarnings || []; | ||
| if (!seenWarnings.includes(VERSION_WARNING_275)) { | ||
| saveSettings({ | ||
| seenVersionWarnings: [...seenWarnings, VERSION_WARNING_275] | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
saveSettings is fire-and-forget — failed save means the warning may reappear.
saveSettings on Line 42 returns a Promise<boolean> but isn't awaited. If persistence fails, the user will see the warning again on next launch. This is likely acceptable UX for a one-time notice, but worth noting.
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/hooks/useVersionWarning.ts` around lines 37 - 46,
The handleVersionWarningClose function currently calls saveSettings (which
returns Promise<boolean>) without awaiting it, so failures can let the warning
reappear; update handleVersionWarningClose to be async, await the result of
saveSettings when persisting seenVersionWarnings (referencing
handleVersionWarningClose, saveSettings,
useDialogStore.getState().closeVersionWarning,
useSettingsStore.getState().settings, VERSION_WARNING_275), and add minimal
error handling (try/catch or check the boolean) to log or swallow failures so a
failed persistence is handled deterministically.
| export async function applyThemeFonts(config: { | ||
| sans?: string; | ||
| serif?: string; | ||
| mono?: string; | ||
| }): Promise<void> { | ||
| const root = document.documentElement; | ||
| const loadPromises: Promise<void>[] = []; | ||
|
|
||
| if (config.sans) { | ||
| loadPromises.push(loadGoogleFont(config.sans)); | ||
| root.style.setProperty('--font-sans', `'${config.sans}', sans-serif`); | ||
| } | ||
|
|
||
| if (config.serif) { | ||
| loadPromises.push(loadGoogleFont(config.serif)); | ||
| root.style.setProperty('--font-serif', `'${config.serif}', serif`); | ||
| } | ||
|
|
||
| if (config.mono) { | ||
| loadPromises.push(loadGoogleFont(config.mono)); | ||
| root.style.setProperty('--font-mono', `'${config.mono}', monospace`); | ||
| } | ||
|
|
||
| await Promise.all(loadPromises); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Promise.all will short-circuit on the first font-load failure — consider Promise.allSettled.
If one of the three fonts fails to load (e.g., network issue for the serif font), Promise.all rejects immediately and the caller gets an error even though the other fonts loaded successfully and their CSS variables were already set. Using Promise.allSettled and logging/warning on individual failures would be more resilient — the page can still render with whatever fonts did load.
♻️ Suggested change
- await Promise.all(loadPromises);
+ const results = await Promise.allSettled(loadPromises);
+ const failures = results.filter((r) => r.status === 'rejected');
+ if (failures.length > 0) {
+ console.debug('Some theme fonts failed to load:', failures);
+ }🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/lib/google-font-loader.ts` around lines 73 - 97,
The applyThemeFonts function currently awaits Promise.all(loadPromises) which
will reject on the first failed load; change it to use
Promise.allSettled(loadPromises) and handle each result so successful fonts
remain applied and failures are logged or warned. Specifically, update
applyThemeFonts to call Promise.allSettled(loadPromises), iterate the settled
results and for any result.status === "rejected" log a warning including the
font name (from the corresponding loadGoogleFont call context) and the error,
while keeping the existing root.style.setProperty calls for fonts that were
requested; ensure loadGoogleFont is still invoked for each configured font so
the settling logic can inspect per-font outcomes.
| goToTask: (taskId: string) => { | ||
| set({ activeView: 'kanban' }); | ||
| const task = useTaskStore.getState().tasks.find( | ||
| (t) => t.id === taskId || t.specId === taskId | ||
| ); | ||
| if (task) { | ||
| set({ selectedTask: task }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
goToTask stores a snapshot of the task object — may go stale.
goToTask stores a reference to the task found at the moment of the call. If the task is later updated in the task store (e.g., status change), selectedTask in the navigation store won't reflect those updates. This is a common Zustand pitfall when duplicating state across stores.
Consider storing only selectedTaskId and deriving the full task object where needed, or subscribing to task-store updates.
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/stores/navigation-store.ts` around lines 35 - 43,
goToTask currently stores a snapshot of the task object (selectedTask) taken
from useTaskStore.getState().tasks which can go stale when the task is updated;
change the navigation store to store only selectedTaskId (e.g., replace
selectedTask with selectedTaskId in the store and update goToTask to set
selectedTaskId = taskId) and update all consumers to derive the live task via
useTaskStore (or create a selector that maps selectedTaskId → current task), or
alternatively subscribe the navigation store to useTaskStore changes so
selectedTask is refreshed whenever the underlying task list changes.
| .task-card-enhanced:hover { | ||
| border-color: rgba(214, 216, 118, 0.4); | ||
| box-shadow: 0 0 0 1px rgba(214, 216, 118, 0.1), 0 4px 12px rgba(0, 0, 0, 0.15); | ||
| z-index: 10; | ||
| } | ||
|
|
||
| .task-card-enhanced:hover { | ||
| border-color: var(--primary); | ||
| box-shadow: 0 0 0 1px color-mix(in srgb, var(--primary) 15%, transparent), 0 4px 12px rgba(0, 0, 0, 0.15); | ||
| transform: translateY(-1px); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Merge consecutive .task-card-enhanced:hover rules.
Two back-to-back rules for the same selector — one sets z-index, the other sets border-color, box-shadow, and transform. No property collision, but splitting them is confusing and fragile.
♻️ Suggested merge
.task-card-enhanced:hover {
z-index: 10;
-}
-
-.task-card-enhanced:hover {
border-color: var(--primary);
box-shadow: 0 0 0 1px color-mix(in srgb, var(--primary) 15%, transparent), 0 4px 12px rgba(0, 0, 0, 0.15);
transform: translateY(-1px);
}📝 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.
| .task-card-enhanced:hover { | |
| border-color: rgba(214, 216, 118, 0.4); | |
| box-shadow: 0 0 0 1px rgba(214, 216, 118, 0.1), 0 4px 12px rgba(0, 0, 0, 0.15); | |
| z-index: 10; | |
| } | |
| .task-card-enhanced:hover { | |
| border-color: var(--primary); | |
| box-shadow: 0 0 0 1px color-mix(in srgb, var(--primary) 15%, transparent), 0 4px 12px rgba(0, 0, 0, 0.15); | |
| transform: translateY(-1px); | |
| } | |
| .task-card-enhanced:hover { | |
| z-index: 10; | |
| border-color: var(--primary); | |
| box-shadow: 0 0 0 1px color-mix(in srgb, var(--primary) 15%, transparent), 0 4px 12px rgba(0, 0, 0, 0.15); | |
| transform: translateY(-1px); | |
| } |
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/styles/globals.css` around lines 1825 - 1833,
Merge the two consecutive .task-card-enhanced:hover rules into a single rule
that contains z-index, border-color, box-shadow, and transform together; locate
the duplicate selector .task-card-enhanced:hover in globals.css, combine the
properties into one block and remove the redundant rule so hover styles are
defined in a single place.
…de quality - Convert all relative imports to path aliases (@/*, @shared/*) across 10+ files - Consolidate Zustand subscriptions with useShallow, add shared selectors - Narrow store subscriptions to minimize re-renders - Wrap handlers in useCallback for referential stability - Rename electronAPI.window → windowControls to avoid global shadowing - Add display bounds validation in window-handlers - Deduplicate ~275 lines of repeated CSS theme variables - Fix circular CSS variable reference (--font-serif) - Replace hardcoded strings with i18n keys in utility functions - Rewrite tautological tests with logic-mirroring assertions - Lower engines.node from >=24 to >=20 - Add closeInitDialog action to dialog-store - Add SidebarView type to shared/types/settings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| cleanupDownloadListener(); | ||
| }; | ||
| }, []); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/components/TopNavBar/__tests__/ProjectTabBar.test.tsx (1)
245-322: 🧹 Nitpick | 🔵 TrivialClass-name assertions are tautological; render the component instead.
These tests only validate hardcoded strings, so they won’t fail if the component’s markup changes. Prefer rendering
ProjectTabBarwith React Testing Library and asserting against the actual DOM/classes.♻️ Example pattern (apply similarly to the other class/structure checks)
-import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render } from '@testing-library/react'; +import { ProjectTabBar } from '../ProjectTabBar'; it('should render add button with correct attributes', () => { - // Check button attributes from component - const buttonVariant = 'ghost'; - const buttonSize = 'icon'; - const buttonClasses = 'h-7 w-7 rounded-full shrink-0'; - - expect(buttonVariant).toBe('ghost'); - expect(buttonSize).toBe('icon'); - expect(buttonClasses).toContain('h-7'); - expect(buttonClasses).toContain('w-7'); - expect(buttonClasses).toContain('rounded-full'); + const projects = [createTestProject()]; + const { container } = render( + <ProjectTabBar + projects={projects} + activeProjectId={projects[0].id} + onProjectSelect={mockOnProjectSelect} + onProjectClose={mockOnProjectClose} + onAddProject={mockOnAddProject} + /> + ); + const addButton = container.querySelector('button.h-7.w-7.rounded-full'); + expect(addButton).toBeTruthy(); });As per coding guidelines, "apps/frontend/**/*.test.{ts,tsx}: Write frontend unit tests using Vitest and React Testing Library with jsdom."
🤖 Fix all issues with AI agents
In `@apps/frontend/package.json`:
- Line 125: Remove the redundant "autoprefixer" entry from
apps/frontend/package.json devDependencies and delete its usage in
apps/frontend/postcss.config.cjs (remove the require/import and any plugin
reference to "autoprefixer"); keep only the Tailwind/PostCSS plugins needed for
Tailwind v4 (e.g., "tailwindcss" or Lightning CSS-related plugins). After
removing, run the package manager install (npm/yarn/pnpm) and ensure the build
scripts that reference postcss config still work without the autoprefixer
plugin.
In `@apps/frontend/src/main/ipc-handlers/window-handlers.ts`:
- Line 63: Replace the informational startup log that uses console.warn with a
non-warning logger: change the call that logs "[IPC] Window control handlers
registered" (the console.warn invocation in the window-handlers registration) to
console.debug; if console.debug is disallowed by the repo ESLint rules, instead
remove the log or route it through the project's approved logger (e.g.,
electron-log) so the message is recorded as informational rather than a warning.
In `@apps/frontend/src/renderer/components/settings/ThemeSelector.tsx`:
- Around line 19-26: The new imports in ThemeSelector.tsx use relative paths;
update them to use the project path aliases instead: replace imports of
parseTweakcnCSSWithSections, saveCustomTheme, deleteCustomTheme,
getAllCustomThemeDefinitions, listCustomThemes to import from
'@/lib/theme-utils' and the types ColorTheme, ColorThemeDefinition, AppSettings
to import from '@shared/types' so functions like parseTweakcnCSSWithSections and
types like ColorThemeReference resolve via the tsconfig aliases.
In `@apps/frontend/src/renderer/components/Sidebar/hooks/useSidebar.ts`:
- Line 9: The hook useSidebar currently types its parameter as the full
SidebarProps but only uses onViewChange, forcing callers to supply
onSettingsClick and onNewTaskClick; change the parameter type to a narrower
shape (e.g., { onViewChange: (...) => ... } or a new interface with only
onViewChange) and update the useSidebar signature to accept that narrower type
so callers no longer need to pass ignored props; keep the function name
useSidebar and the used symbol onViewChange intact when making this change.
- Around line 142-154: handleGitInitialized currently lists the whole
selectedProject object in its useCallback deps which causes unnecessary
recreations; change it to depend on a stable primitive (the project path) by
deriving selectedProjectPath (e.g., selectedProject?.path) and use that
primitive in the dependency array, and inside handleGitInitialized call
window.electronAPI.checkGitStatus(selectedProjectPath) and setGitStatus only
when that path exists; reference handleGitInitialized, selectedProject (use
selectedProject?.path), useCallback, setGitStatus, and
window.electronAPI.checkGitStatus when making the change.
In `@apps/frontend/src/renderer/components/ViewSwitcher.tsx`:
- Around line 101-109: TerminalView creates an inline closure for onNewTaskClick
which recreates on every render and can break memoization of TerminalGrid;
change TerminalView to memoize the handler using React's useCallback (same
pattern as KanbanView) — e.g., create a const openNewTask = useCallback(() =>
useDialogStore.getState().openNewTaskDialog(), [] ) and pass openNewTask to
TerminalGrid’s onNewTaskClick prop so the reference remains stable.
- Around line 16-19: Update the four relative store imports to use the project
path alias instead of relative paths: replace imports referencing
'../stores/navigation-store', '../stores/dialog-store',
'../stores/project-store' (and selectCurrentProjectId) and
'../stores/task-store' with the alias form (e.g., '@/stores/navigation-store',
'@/stores/dialog-store', '@/stores/project-store', '@/stores/task-store') so the
symbols useNavigationStore, useDialogStore, useProjectStore,
selectCurrentProjectId, and useTaskStore are imported via the tsconfig path
alias consistent with other imports like `@shared/`.
- Around line 86-88: The callback handleCloseTaskDetail is misnamed because it
calls useDialogStore.getState().openNewTaskDialog(); rename the function to
something like handleNewTaskClick (or onNewTaskClick) and update any references
where it’s passed (e.g., the onNewTaskClick prop) so the name matches its
behavior; leave the implementation unchanged (still calling
useDialogStore.getState().openNewTaskDialog()) and run type checks to catch
remaining references to handleCloseTaskDetail.
In `@apps/frontend/src/renderer/hooks/useAppInitialization.ts`:
- Around line 53-60: Replace the diagnostic console.warn calls in the
useAppInitialization hook's tab-restore effect with console.debug so they are
hidden by default; specifically update the call that logs
projectsCount/openProjectIds/activeProjectId/selectedProjectId/projectTabs/projectTabIds
and the other console.warn usages within that same effect (the subsequent
diagnostic logs that reference projectTabs, openProjectIds, activeProjectId,
selectedProjectId, etc.) to use console.debug instead, preserving the existing
message text and logged objects/variables.
| "@types/semver": "^7.7.1", | ||
| "@types/uuid": "^11.0.0", | ||
| "@vitejs/plugin-react": "^5.1.2", | ||
| "autoprefixer": "^10.4.22", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Tailwind version and autoprefixer references
echo "=== Checking Tailwind version in package.json ==="
cat apps/frontend/package.json | grep -A 2 -B 2 '"tailwindcss"'
echo ""
echo "=== Checking for autoprefixer usage in config files ==="
rg "autoprefixer" -g '!node_modules' -g '!package-lock.json' -g '!pnpm-lock.yaml' -g '!yarn.lock' -n
echo ""
echo "=== Checking for PostCSS configuration files ==="
find apps/frontend -name "postcss.config.*" -o -name ".postcssrc*" | head -20Repository: AndyMik90/Auto-Claude
Length of output: 496
🏁 Script executed:
#!/bin/bash
# Check the PostCSS configuration in detail
echo "=== PostCSS configuration file content ==="
cat -n apps/frontend/postcss.config.cjsRepository: AndyMik90/Auto-Claude
Length of output: 241
Remove autoprefixer from devDependencies and PostCSS config.
Tailwind CSS v4's Oxide engine uses Lightning CSS, which automatically handles vendor prefixing. The autoprefixer package (^10.4.22) in apps/frontend/package.json and its configuration in apps/frontend/postcss.config.cjs are redundant and can be removed.
🤖 Prompt for AI Agents
In `@apps/frontend/package.json` at line 125, Remove the redundant "autoprefixer"
entry from apps/frontend/package.json devDependencies and delete its usage in
apps/frontend/postcss.config.cjs (remove the require/import and any plugin
reference to "autoprefixer"); keep only the Tailwind/PostCSS plugins needed for
Tailwind v4 (e.g., "tailwindcss" or Lightning CSS-related plugins). After
removing, run the package manager install (npm/yarn/pnpm) and ensure the build
scripts that reference postcss config still work without the autoprefixer
plugin.
| } | ||
| }); | ||
|
|
||
| console.warn('[IPC] Window control handlers registered'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use console.debug instead of console.warn for informational startup log.
This is a normal startup event, not a warning. Per the ESLint config in this repo, console.warn should be reserved for actual warnings. Based on learnings, the ESLint configuration only allows console.warn and console.error — if console.debug is not permitted, consider removing this log or switching to electron-log.
Proposed fix
- console.warn('[IPC] Window control handlers registered');
+ // If electron-log is available, use log.debug instead
+ console.debug('[IPC] Window control handlers registered');🤖 Prompt for AI Agents
In `@apps/frontend/src/main/ipc-handlers/window-handlers.ts` at line 63, Replace
the informational startup log that uses console.warn with a non-warning logger:
change the call that logs "[IPC] Window control handlers registered" (the
console.warn invocation in the window-handlers registration) to console.debug;
if console.debug is disallowed by the repo ESLint rules, instead remove the log
or route it through the project's approved logger (e.g., electron-log) so the
message is recorded as informational rather than a warning.
| import { | ||
| parseTweakcnCSSWithSections, | ||
| saveCustomTheme, | ||
| deleteCustomTheme, | ||
| getAllCustomThemeDefinitions, | ||
| listCustomThemes, | ||
| } from '../../lib/theme-utils'; | ||
| import type { ColorTheme, ColorThemeDefinition, AppSettings } from '../../../shared/types'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use path aliases for new imports.
The newly added imports on lines 19–26 use relative paths. Per coding guidelines, use @/ for renderer paths and @shared/ for shared paths.
Proposed fix
-import {
- parseTweakcnCSSWithSections,
- saveCustomTheme,
- deleteCustomTheme,
- getAllCustomThemeDefinitions,
- listCustomThemes,
-} from '../../lib/theme-utils';
-import type { ColorTheme, ColorThemeDefinition, AppSettings } from '../../../shared/types';
+import {
+ parseTweakcnCSSWithSections,
+ saveCustomTheme,
+ deleteCustomTheme,
+ getAllCustomThemeDefinitions,
+ listCustomThemes,
+} from '@/lib/theme-utils';
+import type { ColorTheme, ColorThemeDefinition, AppSettings } from '@shared/types';As per coding guidelines, "Use path aliases defined in tsconfig.json: @/* → src/renderer/*, @shared/* → src/shared/*".
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/settings/ThemeSelector.tsx` around
lines 19 - 26, The new imports in ThemeSelector.tsx use relative paths; update
them to use the project path aliases instead: replace imports of
parseTweakcnCSSWithSections, saveCustomTheme, deleteCustomTheme,
getAllCustomThemeDefinitions, listCustomThemes to import from
'@/lib/theme-utils' and the types ColorTheme, ColorThemeDefinition, AppSettings
to import from '@shared/types' so functions like parseTweakcnCSSWithSections and
types like ColorThemeReference resolve via the tsconfig aliases.
| import { clearProjectEnvConfig, loadProjectEnvConfig, useProjectEnvStore } from "@/stores/project-env-store"; | ||
| import { baseNavItems, githubNavItems, gitlabNavItems, SidebarProps, SidebarView } from "../constants/types"; | ||
|
|
||
| const useSidebar = ({onViewChange}: SidebarProps) => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hook accepts full SidebarProps but only uses onViewChange.
SidebarProps requires onSettingsClick and onNewTaskClick, so every caller must provide props the hook ignores. Consider narrowing the parameter type to only what's needed:
-const useSidebar = ({onViewChange}: SidebarProps) => {
+const useSidebar = ({onViewChange}: Pick<SidebarProps, 'onViewChange'>) => {📝 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.
| const useSidebar = ({onViewChange}: SidebarProps) => { | |
| const useSidebar = ({onViewChange}: Pick<SidebarProps, 'onViewChange'>) => { |
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/Sidebar/hooks/useSidebar.ts` at line 9,
The hook useSidebar currently types its parameter as the full SidebarProps but
only uses onViewChange, forcing callers to supply onSettingsClick and
onNewTaskClick; change the parameter type to a narrower shape (e.g., {
onViewChange: (...) => ... } or a new interface with only onViewChange) and
update the useSidebar signature to accept that narrower type so callers no
longer need to pass ignored props; keep the function name useSidebar and the
used symbol onViewChange intact when making this change.
| const handleGitInitialized = useCallback(async () => { | ||
| // Refresh git status after initialization | ||
| if (selectedProject) { | ||
| try { | ||
| const result = await window.electronAPI.checkGitStatus(selectedProject.path); | ||
| if (result.success && result.data) { | ||
| setGitStatus(result.data); | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to refresh git status:', error); | ||
| } | ||
| } | ||
| }, [selectedProject]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
handleGitInitialized depends on the full selectedProject object — use stable primitives.
The useCallback dependency on [selectedProject] creates a new callback identity whenever the projects array changes (since selectedProject is derived via .find()). This was specifically addressed for the git-status useEffect (Line 140) but not here. Use the same primitive pattern.
♻️ Proposed fix
- const handleGitInitialized = useCallback(async () => {
- // Refresh git status after initialization
- if (selectedProject) {
- try {
- const result = await window.electronAPI.checkGitStatus(selectedProject.path);
- if (result.success && result.data) {
- setGitStatus(result.data);
- }
- } catch (error) {
- console.error('Failed to refresh git status:', error);
- }
- }
- }, [selectedProject]);
+ const selectedProjectPath = selectedProject?.path;
+ const handleGitInitialized = useCallback(async () => {
+ if (selectedProjectPath) {
+ try {
+ const result = await window.electronAPI.checkGitStatus(selectedProjectPath);
+ if (result.success && result.data) {
+ setGitStatus(result.data);
+ }
+ } catch (error) {
+ console.error('Failed to refresh git status:', error);
+ }
+ }
+ }, [selectedProjectPath]);🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/Sidebar/hooks/useSidebar.ts` around
lines 142 - 154, handleGitInitialized currently lists the whole selectedProject
object in its useCallback deps which causes unnecessary recreations; change it
to depend on a stable primitive (the project path) by deriving
selectedProjectPath (e.g., selectedProject?.path) and use that primitive in the
dependency array, and inside handleGitInitialized call
window.electronAPI.checkGitStatus(selectedProjectPath) and setGitStatus only
when that path exists; reference handleGitInitialized, selectedProject (use
selectedProject?.path), useCallback, setGitStatus, and
window.electronAPI.checkGitStatus when making the change.
| import { useNavigationStore } from '../stores/navigation-store'; | ||
| import { useDialogStore } from '../stores/dialog-store'; | ||
| import { useProjectStore, selectCurrentProjectId } from '../stores/project-store'; | ||
| import { useTaskStore } from '../stores/task-store'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use path aliases for store imports.
Lines 16–19 use relative paths for store imports while line 20–21 correctly use @shared/. Convert for consistency.
Proposed fix
-import { useNavigationStore } from '../stores/navigation-store';
-import { useDialogStore } from '../stores/dialog-store';
-import { useProjectStore, selectCurrentProjectId } from '../stores/project-store';
-import { useTaskStore } from '../stores/task-store';
+import { useNavigationStore } from '@/stores/navigation-store';
+import { useDialogStore } from '@/stores/dialog-store';
+import { useProjectStore, selectCurrentProjectId } from '@/stores/project-store';
+import { useTaskStore } from '@/stores/task-store';As per coding guidelines, "Use path aliases defined in tsconfig.json: @/* → src/renderer/*".
📝 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.
| import { useNavigationStore } from '../stores/navigation-store'; | |
| import { useDialogStore } from '../stores/dialog-store'; | |
| import { useProjectStore, selectCurrentProjectId } from '../stores/project-store'; | |
| import { useTaskStore } from '../stores/task-store'; | |
| import { useNavigationStore } from '@/stores/navigation-store'; | |
| import { useDialogStore } from '@/stores/dialog-store'; | |
| import { useProjectStore, selectCurrentProjectId } from '@/stores/project-store'; | |
| import { useTaskStore } from '@/stores/task-store'; |
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/ViewSwitcher.tsx` around lines 16 - 19,
Update the four relative store imports to use the project path alias instead of
relative paths: replace imports referencing '../stores/navigation-store',
'../stores/dialog-store', '../stores/project-store' (and selectCurrentProjectId)
and '../stores/task-store' with the alias form (e.g.,
'@/stores/navigation-store', '@/stores/dialog-store', '@/stores/project-store',
'@/stores/task-store') so the symbols useNavigationStore, useDialogStore,
useProjectStore, selectCurrentProjectId, and useTaskStore are imported via the
tsconfig path alias consistent with other imports like `@shared/`.
| const handleCloseTaskDetail = useCallback(() => { | ||
| useDialogStore.getState().openNewTaskDialog(); | ||
| }, []); |
There was a problem hiding this comment.
Misleading callback name: handleCloseTaskDetail opens a new task dialog.
The function calls openNewTaskDialog() but is named handleCloseTaskDetail, which implies dismissing a detail view. Since it's passed as onNewTaskClick, rename it to match its intent.
Proposed fix
- const handleCloseTaskDetail = useCallback(() => {
+ const handleNewTaskClick = useCallback(() => {
useDialogStore.getState().openNewTaskDialog();
}, []);
return (
<KanbanBoard
tasks={tasks}
onTaskClick={handleTaskClick}
- onNewTaskClick={handleCloseTaskDetail}
+ onNewTaskClick={handleNewTaskClick}
onRefresh={handleRefreshTasks}
isRefreshing={isRefreshingTasks}
/>
);📝 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.
| const handleCloseTaskDetail = useCallback(() => { | |
| useDialogStore.getState().openNewTaskDialog(); | |
| }, []); | |
| const handleNewTaskClick = useCallback(() => { | |
| useDialogStore.getState().openNewTaskDialog(); | |
| }, []); | |
| return ( | |
| <KanbanBoard | |
| tasks={tasks} | |
| onTaskClick={handleTaskClick} | |
| onNewTaskClick={handleNewTaskClick} | |
| onRefresh={handleRefreshTasks} | |
| isRefreshing={isRefreshingTasks} | |
| /> | |
| ); |
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/ViewSwitcher.tsx` around lines 86 - 88,
The callback handleCloseTaskDetail is misnamed because it calls
useDialogStore.getState().openNewTaskDialog(); rename the function to something
like handleNewTaskClick (or onNewTaskClick) and update any references where it’s
passed (e.g., the onNewTaskClick prop) so the name matches its behavior; leave
the implementation unchanged (still calling
useDialogStore.getState().openNewTaskDialog()) and run type checks to catch
remaining references to handleCloseTaskDetail.
| function TerminalView({ projectPath, isActive = false }: { projectPath?: string; isActive?: boolean }) { | ||
| return ( | ||
| <TerminalGrid | ||
| projectPath={projectPath} | ||
| onNewTaskClick={() => useDialogStore.getState().openNewTaskDialog()} | ||
| isActive={isActive} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent callback pattern — TerminalView uses inline closure while KanbanView uses useCallback.
TerminalView line 105 creates a new closure on every render for onNewTaskClick, while KanbanView wraps the same pattern in useCallback. If TerminalGrid is memoized, this will defeat the memo. Consider applying useCallback consistently across wrapper components.
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/ViewSwitcher.tsx` around lines 101 -
109, TerminalView creates an inline closure for onNewTaskClick which recreates
on every render and can break memoization of TerminalGrid; change TerminalView
to memoize the handler using React's useCallback (same pattern as KanbanView) —
e.g., create a const openNewTask = useCallback(() =>
useDialogStore.getState().openNewTaskDialog(), [] ) and pass openNewTask to
TerminalGrid’s onNewTaskClick prop so the reference remains stable.
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Diagnostic console.warn calls should be console.debug.
Lines 53–60, 65, 70, 76, 78, 82, and 86 use console.warn for informational/diagnostic tab-restore logging. These will appear prominently (yellow warnings) in production DevTools. Switch to console.debug so they're hidden by default.
Proposed fix (showing first occurrence)
- console.warn('[App] Tab restore useEffect triggered:', {
+ console.debug('[App] Tab restore useEffect triggered:', {
projectsCount: projects.length,
openProjectIds,
activeProjectId,
selectedProjectId,
projectTabsCount: projectTabs.length,
projectTabIds: projectTabs.map(p => p.id)
});Apply the same warn → debug change to all other diagnostic log calls in this effect (lines 65, 70, 76, 78, 82, 86).
📝 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.
| console.warn('[App] Tab restore useEffect triggered:', { | |
| projectsCount: projects.length, | |
| openProjectIds, | |
| activeProjectId, | |
| selectedProjectId, | |
| projectTabsCount: projectTabs.length, | |
| projectTabIds: projectTabs.map(p => p.id) | |
| }); | |
| console.debug('[App] Tab restore useEffect triggered:', { | |
| projectsCount: projects.length, | |
| openProjectIds, | |
| activeProjectId, | |
| selectedProjectId, | |
| projectTabsCount: projectTabs.length, | |
| projectTabIds: projectTabs.map(p => p.id) | |
| }); |
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/hooks/useAppInitialization.ts` around lines 53 -
60, Replace the diagnostic console.warn calls in the useAppInitialization hook's
tab-restore effect with console.debug so they are hidden by default;
specifically update the call that logs
projectsCount/openProjectIds/activeProjectId/selectedProjectId/projectTabs/projectTabIds
and the other console.warn usages within that same effect (the subsequent
diagnostic logs that reference projectTabs, openProjectIds, activeProjectId,
selectedProjectId, etc.) to use console.debug instead, preserving the existing
message text and logged objects/variables.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 4 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 3 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 6 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: [UNRESOLVED] Zustand selector calls getProjectTabs() causing unnecessary re-renders
- quality: Missing cleanupGitHubListeners() in useEffect cleanup
Verdict
CI Status: 1 check still pending — cannot approve until all checks pass. Additionally, 2 confirmed MEDIUM-severity issues remain:
-
[7ac83ce8a5b7] Zustand selector in useTopNavBar.ts:13 calls getProjectTabs() inside a selector, creating a new array on every store update. This causes the TopNavBar (including DnD context and tab rendering) to re-render on ANY store change, not just tab changes. Fix: use useShallow or derive from primitive selectors.
-
[NEW-002] Missing cleanupGitHubListeners() in useAppInitialization.ts useEffect cleanup. The function exists and is specifically documented for "app unmount or hot-reload" cleanup, but is not called. During HMR this causes stale listeners.
Significant progress: 2 of 5 previous findings resolved (dependency array fix, unused packages removed). 2 previously-unresolved findings dismissed as false positives after validation (AppDialogs doesn't use tooltips so being outside TooltipProvider is fine; co-locating standalone functions with hooks is standard React practice). All 10 CodeRabbit review comments have been addressed. The massive App.tsx refactor (1184→47 lines) is well-structured.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (4 selected of 4 total)
🟡 [7ac83ce8a5b7] [MEDIUM] [UNRESOLVED] Zustand selector calls getProjectTabs() causing unnecessary re-renders
📁 apps/frontend/src/renderer/components/TopNavBar/hooks/useTopNavBar.ts:13
The selector (state) => state.getProjectTabs() invokes getProjectTabs() which creates a new array on every call. Zustand uses Object.is comparison by default, so a new array reference means this selector ALWAYS reports a change on any store update — even unrelated ones (e.g., a settings update). This causes the useTopNavBar hook and the entire TopNavBar component tree (including DnD sensors) to re-render on every project-store mutation.
Compare with the correct pattern in useAppInitialization.ts:24 which gets the function reference: const getProjectTabs = useProjectStore((state) => state.getProjectTabs) and calls it outside the selector.
Resolution note: useTopNavBar.ts:13 still has const projectTabs = useProjectStore((state) => state.getProjectTabs()); — getProjectTabs() returns [...orderedProjects, ...remainingProjects] creating a new array every call. No useShallow or shallow comparison added.
Suggested fix:
Either (1) select primitive dependencies and compute tabs outside: `const projects = useProjectStore(s => s.projects); const openIds = useProjectStore(s => s.openProjectIds); const tabOrder = useProjectStore(s => s.tabOrder); const projectTabs = useMemo(() => ..., [projects, openIds, tabOrder])`, or (2) use Zustand's `useShallow` to compare by shallow equality: `const projectTabs = useProjectStore(useShallow((state) => state.getProjectTabs()))`.
🟡 [NEW-002] [MEDIUM] Missing cleanupGitHubListeners() in useEffect cleanup
📁 apps/frontend/src/renderer/hooks/useAppInitialization.ts:37
initializeGitHubListeners() registers IPC listeners for PR review events but cleanupGitHubListeners() is not called in the effect's cleanup function. The cleanup function exists in github/index.ts and is documented for 'app unmount or hot-reload'. During development with HMR, the old listeners become stale without cleanup. Fix: add cleanupGitHubListeners() to the return function.
Suggested fix:
Add `cleanupGitHubListeners()` to the useEffect cleanup: `return () => { cleanupDownloadListener(); cleanupGitHubListeners(); };`
🔵 [NEW-006] [LOW] getProjectTabs() called during render creates unnecessary array allocation
📁 apps/frontend/src/renderer/hooks/useAppInitialization.ts:51
getProjectTabs() is called at the top level of the hook (outside useEffect), creating a new array via spread operator on every render. The result is primarily used for projectTabs.length in the useEffect dependency array and logging. Could be optimized by selecting openProjectIds.length directly or using useMemo.
Suggested fix:
Move getProjectTabs() call inside the useEffect body since it's mainly used for logging, or use openProjectIds.length as the dependency instead of computing the full tabs array.
🔵 [CMT-001] [LOW] [FROM COMMENTS] Path alias consistency partially addressed (from CodeRabbit)
📁 apps/frontend/src/renderer/components:0
New/refactored files consistently use @/ path aliases (10 files, 28 imports), but 46 existing untouched files still use relative paths. This is a pre-existing codebase inconsistency that is out of scope for this PR. All other CodeRabbit comments (9 of 10) have been fully addressed: Node engine lowered to >=20.0.0, unused Radix/cmdk packages removed, setBounds screen validation added, window property renamed to windowControls, t variable shadowing fixed, sidebar indentation fixed, shared selectors added, duplicate callbacks documented.
Suggested fix:
Consider a follow-up PR to migrate remaining relative imports to @/ aliases for consistency.
This review was generated by Auto Claude.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…r cleanup, UI polish Resolve blocking issues from follow-up Auto Claude review (3809423070): - useTopNavBar: replace getProjectTabs() selector with primitive selectors + useMemo - useAppInitialization: add cleanupGitHubListeners() to useEffect cleanup for HMR - useAppInitialization: remove getProjectTabs() render-time call, use direct deps - AppShell/Sidebar/TopNavBar: layout and styling refinements Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The removeProject function returns a boolean indicating success, but the dialog handler was unconditionally closing on completion. Now checks the return value and shows an error message if removal fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| import { | ||
| Plus, | ||
| Settings, | ||
| HelpCircle, | ||
| Heart, | ||
| PanelLeft, | ||
| PanelLeftClose, | ||
| } from 'lucide-react'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🔴 Follow-up Review: Blocked
🔴 Blocked - Merge conflicts must be resolved before merge.
Resolution Status
- ✅ Resolved: 3 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 4 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 1 findings require manual verification
🚨 Blocking Issues
- Merge Conflicts: PR has conflicts with base branch that must be resolved
Verdict
Blocked: PR has merge conflicts with base branch. Resolve conflicts before merge.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (5 selected of 5 total)
🔵 [CMT-001] [LOW] [UNRESOLVED] [FROM COMMENTS] Path alias consistency partially addressed (from CodeRabbit)
📁 apps/frontend/src/renderer/components:0
New/refactored files consistently use @/ path aliases (10 files, 28 imports), but 46 existing untouched files still use relative paths. This is a pre-existing codebase inconsistency that is out of scope for this PR. All other CodeRabbit comments (9 of 10) have been fully addressed: Node engine lowered to >=20.0.0, unused Radix/cmdk packages removed, setBounds screen validation added, window property renamed to windowControls, t variable shadowing fixed, sidebar indentation fixed, shared selectors added, duplicate callbacks documented.
Resolution note: 40 relative import paths remain across 10 untouched files in the components directory. This was already acknowledged as a pre-existing codebase inconsistency out of scope for this PR. New/refactored files consistently use @/ path aliases.
Suggested fix:
Consider a follow-up PR to migrate remaining relative imports to @/ aliases for consistency.
🔵 [NEW-001] [LOW] Unused import: PanelLeft from lucide-react
📁 apps/frontend/src/renderer/components/Sidebar/index.tsx:6
PanelLeft is imported from lucide-react but never used in the Sidebar component. Only PanelLeftClose is used (line 143). Also flagged by github-advanced-security[bot] code scanning alert.
Suggested fix:
Remove PanelLeft from the import statement on line 6.
🔵 [NEW-003] [LOW] Window centering can produce negative offsets for oversized windows
📁 apps/frontend/src/main/ipc-handlers/window-handlers.ts:53
When centering an off-screen window on the nearest display, if safeBounds.width > display work area width, the centering calculation produces negative offsets (e.g., dX=0, dW=1920, width=2000 yields x=-40). This places the window partially off-screen. Edge case occurs when a window saved from a larger display is restored to a smaller one.
Suggested fix:
Clamp width/height to display work area before centering: safeBounds.width = Math.min(safeBounds.width, dW); safeBounds.height = Math.min(safeBounds.height, dH);
🔵 [NEW-005] [LOW] Promise rejection silently swallowed in isMaximized() call
📁 apps/frontend/src/renderer/components/TopNavBar/WindowControls.tsx:13
The isMaximized() IPC call uses .catch(() => {}) which silently swallows errors without logging. While impact is low (non-critical UI state with onMaximizeChanged as fallback), this makes debugging harder.
Suggested fix:
Replace .catch(() => {}) with .catch((err) => console.debug('[WindowControls] Failed to query maximize state:', err))
🔵 [CMT-001] [LOW] [FROM COMMENTS] Unused import PanelLeft flagged by code scanning
📁 apps/frontend/src/renderer/components/Sidebar/index.tsx:6
github-advanced-security[bot] flagged unused PanelLeft import in Sidebar component. This is the same issue as NEW-001. The import exists at line 6 but PanelLeft is never used in the component body — only PanelLeftClose is used at line 143.
Suggested fix:
Remove PanelLeft from the import statement.
This review was generated by Auto Claude.
4 participants
Base Branch
developbranchDescription
Decomposes the monolithic 1,183-line App.tsx into focused, single-responsibility modules for maintainability and scalability. Extracts TopNavBar, Sidebar, and dialog orchestration into dedicated components with custom hooks and Zustand stores. Adds cross-platform window controls IPC and tweakcn.com-compatible theme CSS variables.
Net result: -3,019 lines (2,602 additions / 5,621 deletions across 57 files).
Type of Change
Area
AI Disclosure
What Changed
App.tsx Decomposition (1,183 → 47 lines)
AppShell.tsxAppDialogs.tsxViewSwitcher.tsxdialog-store.tsnavigation-store.tsNew Hooks (extracted from App.tsx)
useAppInitializationuseAppThemeuseAppEventListenersuseOnboardingDetectionuseVersionWarninguseTaskSyncTopNavBar Extraction
TopNavBar/directory withProjectTabBar,SortableProjectTab,AppBranding,WindowControlsuseTopNavBarhook for drag-and-drop stateTopNavBar/__tests__/Sidebar Refactoring
Sidebar/index.tsx(presentation) +hooks/useSidebar.ts(logic) +constants/types.tsCross-Platform Window Controls
window-handlers.ts(main) +window-api.ts(preload)Theme System
globals.cssCleanup
ui/index.tsStandard Checklist
developbranchPlatform Testing
platform/module (not directprocess.platform)CI/Testing Requirements
Screenshots
N/A — no visual changes, purely structural refactoring
Feature Toggle
Breaking Changes
None. All imports are internal to the frontend app. No public API changes.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI/UX Improvements