feat(ui): migrate to React Query for data fetching #499
Conversation
- Install @tanstack/react-query and @tanstack/react-query-devtools - Add QueryClient with default stale times and retry config - Create query-keys.ts factory for consistent cache key management - Wrap app root with QueryClientProvider and DevTools Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add useFeatures, useFeature, useAgentOutput for feature data - Add useGitHubIssues, useGitHubPRs, useGitHubValidations, useGitHubIssueComments - Add useClaudeUsage, useCodexUsage with polling intervals - Add useRunningAgents, useRunningAgentsCount - Add useWorktrees, useWorktreeInfo, useWorktreeStatus, useWorktreeDiffs - Add useGlobalSettings, useProjectSettings, useCredentials - Add useAvailableModels, useCodexModels, useOpencodeModels - Add useSessions, useSessionHistory, useSessionQueue - Add useIdeationPrompts, useIdeas - Add CLI status queries (claude, cursor, codex, opencode, github) - Add useCursorPermissionsQuery, useWorkspaceDirectories - Add usePipelineConfig, useSpecFile, useSpecRegenerationStatus Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add feature mutations (create, update, delete with optimistic updates) - Add auto-mode mutations (start, stop, approve plan) - Add worktree mutations (create, delete, checkout, switch branch) - Add settings mutations (update global/project, validate API keys) - Add GitHub mutations (create PR, validate PR) - Add cursor permissions mutations (apply profile, copy config) - Add spec mutations (generate, update, save) - Add pipeline mutations (toggle, update config) - Add session mutations with cache invalidation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add useAutoModeQueryInvalidation for feature/agent events - Add useSpecRegenerationQueryInvalidation for spec updates - Add useGitHubValidationQueryInvalidation for PR validation events - Bridge WebSocket events to cache invalidation for real-time updates Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add reusable SkeletonPulse component to replace 4 duplicate definitions - Update CLI status components to use shared skeleton - Simplify CLI status components by using React Query hooks Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace manual fetching in use-board-features with useFeatures query - Migrate use-board-actions to use mutation hooks - Update kanban-card and agent-info-panel to use query hooks - Migrate agent-output-modal to useAgentOutput query - Migrate create-pr-dialog to useCreatePR mutation - Remove manual loading/error state management - Add proper cache invalidation on mutations Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Migrate use-worktrees to useWorktrees query hook - Migrate use-branches to useWorktreeBranches query hook - Migrate use-available-editors to useAvailableEditors query hook - Migrate use-worktree-actions to use mutation hooks - Update worktree-panel component to use query data - Remove manual state management for loading/errors Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Migrate use-github-issues to useGitHubIssues query - Migrate use-issue-comments to useGitHubIssueComments infinite query - Migrate use-issue-validation to useGitHubValidations with mutations - Migrate github-prs-view to useGitHubPRs query - Support pagination for comments with useInfiniteQuery - Remove manual loading state management Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Migrate use-cursor-permissions to query and mutation hooks - Migrate use-cursor-status to React Query - Migrate use-skills-settings to useUpdateGlobalSettings mutation - Migrate use-subagents-settings to mutation hooks - Migrate use-subagents to useDiscoveredAgents query - Migrate opencode-settings-tab to React Query hooks - Migrate worktrees-section to query hooks - Migrate codex/claude usage sections to query hooks - Remove manual useState for loading/error states Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Migrate claude-usage-popover to useClaudeUsage query with polling - Migrate codex-usage-popover to useCodexUsage query with polling - Migrate usage-popover to React Query hooks - Migrate running-agents-view to useRunningAgents query - Replace manual polling intervals with refetchInterval - Remove manual loading/error state management Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Migrate workspace-picker-modal to useWorkspaceDirectories query - Migrate session-manager to useSessions query - Migrate git-diff-panel to useGitDiffs query - Migrate prompt-list to useIdeationPrompts query - Migrate spec-view hooks to useSpecFile query and spec mutations - Migrate use-board-background-settings to useProjectSettings query - Migrate use-guided-prompts to useIdeationPrompts query - Migrate use-project-settings-loader to React Query - Complete React Query migration across all components Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces many imperative Electron/API calls and local state with centralized React Query hooks and mutations, adds a configured QueryClient + query-keys, introduces extensive UI virtualization and memoization, adds many query/mutation barrels and invalidation hooks, and adds dependency-resolver helpers and related tests. Changes
Sequence Diagram(s)(Skipped — changes span many distinct features and flows; no single focused multi-component flow was singled out for visualization.) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 @Shironex, 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 significant refactor of the UI's data fetching mechanism, replacing manual implementations with TanStack React Query. This change aims to improve the application's responsiveness, reduce code duplication, and provide a more maintainable architecture for data management. The PR also includes several infrastructure improvements and component migrations to fully leverage React Query's capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the UI's data fetching and state management by migrating numerous components from manual useState/useEffect patterns and Zustand store interactions to TanStack React Query. Key changes include upgrading React Query dependencies, introducing new query and mutation hooks (useClaudeUsage, useCodexUsage, useWorkspaceDirectories, useSessions, useWorktreeDiffs, useGitDiffs, usePipelineConfig, useFeature, useAgentOutput, useWorktreeBranches, useVerifyFeature, useResumeFeature, useFeatures, useAvailableEditors, useWorktreeInitScript, useGithubIssues, useGitHubIssueComments, useValidateIssue, useMarkValidationViewed, useGitHubPRs, useGenerateIdeationSuggestions, useRunningAgents, useStopFeature, useSetInitScript, useDeleteInitScript, useCreateSpec, useRegenerateSpec, useGenerateFeatures, useSaveSpec, useCursorPermissionsQuery, useApplyCursorProfile, useCopyCursorConfig, useCursorCliStatus, useUpdateGlobalSettings, useUpdateProjectSettings, useSubagentsSettings, useSkillsSettings, useDiscoveredAgents), and centralizing query invalidation logic based on WebSocket events. Many components now derive loading, error, and data states directly from React Query hooks, simplifying their internal logic. A new SkeletonPulse component was added for loading indicators. Review comments highlight the need to map generic React Query errors back to more specific, user-friendly messages in some usage popovers, to fully migrate components to React Query as the single source of truth for data (rather than dual-updating Zustand), and to consider adding generic toast notifications for query errors in the global error handler.
|
@coderabbitai review this |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx (1)
85-250: MissingonClosein useEffect dependency array.The
onClosecallback is invoked at line 234 but is not included in the dependency array at line 250. This can cause a stale closure bug where an outdatedonClosereference is called if the parent re-renders with a new callback.Proposed fix
return () => { unsubscribe(); }; - }, [open, featureId]); + }, [open, featureId, onClose]);apps/ui/src/components/views/github-issues-view/hooks/use-issue-validation.ts (1)
264-287: CallresolveModelString()before passing model to mutation.Per coding guidelines for TypeScript files, model aliases (haiku, sonnet, opus) must be resolved to full model names using
resolveModelString()from@automaker/model-resolver. Currently,modelToUseis passed directly to the mutation without resolution. Apply the same pattern used inmodel-config.ts:const modelToUse = resolveModelString(normalizedEntry.model);Import:
import { resolveModelString } from '@automaker/model-resolver';apps/ui/src/components/views/github-prs-view.tsx (1)
380-391:group-hoverwon't work — parent lacksgroupclass.The button uses
group-hover:opacity-100but the parent<div>inPRRow(line 315) doesn't have thegroupclass. The button will remain invisible on hover.Suggested fix
Add
groupclass to the parent div:<div className={cn( - 'flex items-start gap-3 p-3 cursor-pointer hover:bg-accent/50 transition-colors', + 'group flex items-start gap-3 p-3 cursor-pointer hover:bg-accent/50 transition-colors', isSelected && 'bg-accent' )} onClick={onClick} >
🤖 Fix all issues with AI agents
In `@apps/ui/src/components/session-manager.tsx`:
- Around line 155-160: The useEffect currently lists a boolean expression
([sessions.length > 0]) which is unstable and causes ESLint/misrun issues;
update it to use a stable dependency list and a ref to run only once on initial
load: add a useRef flag (e.g., firstLoadRef) and inside the useEffect that
depends on sessions (or sessions.length) check if firstLoadRef.current is false
and sessions.length > 0 then call checkRunningSessions(sessions) and set
firstLoadRef.current = true; ensure checkRunningSessions and sessions are
included in the dependency array (or memoize checkRunningSessions) so ESLint
warnings are resolved.
In `@apps/ui/src/components/usage-popover.tsx`:
- Around line 137-139: The wrapper functions fetchClaudeUsage and
fetchCodexUsage are defined with no parameters but are invoked with a boolean
(false) elsewhere; update either the call sites to remove the unused argument or
change the wrappers to accept a parameter and forward it to
refetchClaude/refetchCodex (e.g., add a param like force?: boolean and call
refetchClaude(force) / refetchCodex(force)) so the signatures match the usages;
adjust all occurrences (including the calls around lines ~302-311, 307, 414) for
consistency.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx`:
- Line 100: Update the import paths in kanban-card.tsx and agent-info-panel.tsx
to follow the `@automaker/`* package pattern: replace imports from '@/lib/utils'
with '@automaker/utils', replace '@/lib/agent-context-parser' with
'@automaker/agent-context-parser', and replace '@/store/app-store' with the
appropriate `@automaker` package (e.g., '@automaker/store' or
'@automaker/app-store' depending on the published package name used elsewhere in
the repo); adjust the import lines in those two files to use these `@automaker/`*
module specifiers, ensure any exported symbols (e.g., useWorktrees,
currentProject, agent-context parsing helpers) are correctly referenced after
the change, and run type-check/build to confirm the new module paths resolve.
In
`@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-branches.ts`:
- Around line 16-28: The gitRepoStatus defaults currently set isGitRepo: true
and hasCommits: true cause the UI to assume repo capability before
useWorktreeBranches completes; change the construction of gitRepoStatus (where
branchData is read) to default both fields to false (e.g., use
branchData?.isGitRepo ?? false and branchData?.hasCommits ?? false) so the UI
reflects an unknown/unavailable state while isLoadingBranches is true or the
query fails; ensure worktree-actions-dropdown behavior relies on these
conservative values rather than presuming true.
In `@apps/ui/src/components/views/spec-view/hooks/use-spec-loading.ts`:
- Around line 27-47: loadSpec currently calls queryClient.invalidateQueries then
immediately uses queryClient.getQueryData to check status, which can return
stale data; replace that pattern by awaiting the latest status via
queryClient.fetchQuery (or awaiting a refetch) for the queryKey from
queryKeys.specRegeneration.status(currentProject.path) to obtain up-to-date
statusData before checking isRunning, then proceed to invalidate/refetch
queryKeys.spec.file; keep the currentProject?.path guard and existing queryKey
symbols (loadSpec, queryClient.invalidateQueries,
queryKeys.specRegeneration.status, queryKeys.spec.file,
queryClient.getQueryData) but remove the stale getQueryData use in favor of
fetchQuery/awaited refetch.
In `@apps/ui/src/components/views/spec-view/hooks/use-spec-save.ts`:
- Around line 9-10: The mutation is being instantiated with an empty-string
fallback which can produce an invalid path; change the call site so useSaveSpec
receives a valid path or is not created when currentProject is null: either pass
currentProject?.path (allowing undefined) instead of '' or wrap the useSaveSpec
call behind a conditional so saveMutation is only created when currentProject
exists, and/or update useSaveSpec to accept an optional projectPath and guard
inside its saveSpec handler to prevent writes when projectPath is missing;
reference useSaveSpec and saveMutation and ensure any UI checks (e.g., the
existing Line 13 guard) align with this conditional instantiation.
In `@apps/ui/src/hooks/mutations/use-auto-mode-mutations.ts`:
- Around line 103-125: useStopFeature only invalidates runningAgents but not the
features cache, which can leave stale feature status in the UI; update
useStopFeature to also invalidate the features cache after success by calling
queryClient.invalidateQueries for the features key (e.g.,
queryClient.invalidateQueries({ queryKey: queryKeys.features.all() })); if
features are namespaced by projectPath, either add projectPath to the
hook/mutation input and invalidate the specific project features key, or use the
broader queryKey invalidation pattern to clear all features if projectPath isn't
available.
In `@apps/ui/src/hooks/mutations/use-github-mutations.ts`:
- Around line 40-44: The example and mutation code pass a model alias string
directly to the API; call resolveModelString from `@automaker/model-resolver` to
convert the alias to a canonical model identifier before sending the payload.
Import resolveModelString into use-github-mutations.ts and use it to transform
the model field (where model: 'sonnet' or the variable holding model is
assembled) inside the mutation handler or payload builder (e.g., in the function
that constructs the request body in useGithubMutations) so the resolved model
string is sent to the API instead of the alias.
In `@apps/ui/src/hooks/mutations/use-spec-mutations.ts`:
- Around line 155-179: The mutation constructs a file path with projectPath and
will write to "/.automaker/..." if projectPath is empty; update useSaveSpec so
mutationFn validates projectPath before calling api.writeFile (e.g., at the top
of mutationFn check if (!projectPath || projectPath.trim() === '') throw new
Error('Invalid projectPath')); then build the path and call api.writeFile, so
you never attempt to write to the root when projectPath is empty and the onError
handler will surface the error; reference useSaveSpec, its mutationFn,
projectPath and api.writeFile for where to add the guard.
In `@apps/ui/src/hooks/queries/use-settings.ts`:
- Around line 105-122: The query key for useDiscoveredAgents omits the sources
parameter so changes to sources won't invalidate cache; update the queryKey to
include sources (e.g., pass sources or a stable representation like sources ??
[] as an extra segment) when calling queryKeys.settings.agents inside
useDiscoveredAgents, or modify queryKeys.settings.agents to accept a sources
argument and use that in the returned key; keep the rest of the hook
(getElectronAPI(), api.settings.discoverAgents, enabled, staleTime) unchanged.
In `@apps/ui/src/hooks/queries/use-worktrees.ts`:
- Around line 163-206: The query key for useWorktreeBranches is missing the
includeRemote flag so cached results can be wrong; update the query key factory
used by queryKeys.worktrees.branches to accept includeRemote (e.g.,
branches(worktreePath, includeRemote)) and change the call in
useWorktreeBranches to pass includeRemote into queryKey, ensuring the key
uniquely reflects both worktreePath and includeRemote; keep the rest of the
query logic and enabled/staleTime behavior unchanged.
In `@apps/ui/src/hooks/use-query-invalidation.ts`:
- Around line 139-164: In useGitHubValidationQueryInvalidation, the optional
chaining on api.github?.onValidationEvent can leave unsubscribe undefined and
cause the returned cleanup to throw; fix by explicitly checking for api.github
before subscribing (e.g., if (!api.github) return undefined or set unsubscribe
to a noop function) so that unsubscribe is always a callable cleanup; update the
code around getElectronAPI()/api.github and the unsubscribe return so the
cleanup always returns a function (or undefined safely) to avoid calling
undefined.
♻️ Duplicate comments (3)
apps/ui/src/lib/query-client.ts (1)
57-70: Query errors are silently logged without user feedback.Unlike
handleMutationError, this handler only logs errors and doesn't show a toast for general query failures. While background refetch failures may not need toasts, initial query failures could leave users unaware of issues if individual queries don't implement their ownonError.Consider whether this is intentional UX behavior or if a generic toast should be added for consistency with mutation error handling.
apps/ui/src/components/views/board-view.tsx (1)
1571-1577: Dual source of truth remains a concern.The cache invalidation via React Query followed by Zustand update maintains two sources of truth for pipeline configuration. While the comment acknowledges this is for "backward compatibility," this creates potential for state drift between React Query cache and Zustand store.
Consider tracking a follow-up task to fully migrate consumers to React Query, eliminating the need for the Zustand update. Until then, ensure the invalidation completes before the Zustand update to maintain consistency.
Potential improvement for consistency
// Invalidate React Query cache to refetch updated config - queryClient.invalidateQueries({ + await queryClient.invalidateQueries({ queryKey: queryKeys.pipeline.config(currentProject.path), }); // Also update Zustand for backward compatibility setPipelineConfig(currentProject.path, config);Using
awaitensures the cache is invalidated before updating Zustand, though the real fix is to eliminate the dual state.apps/ui/src/components/claude-usage-popover.tsx (1)
185-191: Generic error message loses actionable context.The previous implementation likely had specific error handling (e.g.,
TRUST_PROMPT) that provided tailored user guidance. The current generic message suggestsclaude loginfor all errors, which may not be appropriate for network failures or other non-auth issues.Consider mapping error types to specific user-friendly messages, similar to how other usage popovers in the codebase handle this.
🧹 Nitpick comments (46)
apps/ui/src/hooks/use-project-settings-loader.ts (2)
29-32: Consider resetting the ref when settings change to avoid stale state.The
appliedProjectRefprevents re-application when the same project stays selected. However, if settings are updated elsewhere (e.g., another client or settings panel) and React Query refetches, the new values won't be applied because the ref still matches the current project path.If re-applying updated settings is desired, consider keying the ref on a combination of project path and a settings version/timestamp, or removing the guard and relying on setter idempotency.
55-72: Type safety could be improved in the settingsMap iteration.The cast on line 70 loses type information. While this works at runtime if the API types align, TypeScript won't catch mismatches between setting keys and their expected value types (e.g.,
cardOpacityexpects a number,columnBorderEnabledexpects a boolean).If you prefer the iteration pattern, consider a type-safe approach:
♻️ Suggested improvement
- // Settings map for cleaner iteration - const settingsMap = { - cardOpacity: setCardOpacity, - columnOpacity: setColumnOpacity, - columnBorderEnabled: setColumnBorderEnabled, - cardGlassmorphism: setCardGlassmorphism, - cardBorderEnabled: setCardBorderEnabled, - cardBorderOpacity: setCardBorderOpacity, - hideScrollbar: setHideScrollbar, - } as const; - - // Apply all settings that are defined - for (const [key, setter] of Object.entries(settingsMap)) { - const value = bg?.[key as keyof typeof bg]; - if (value !== undefined) { - (setter as (path: string, val: typeof value) => void)(projectPath, value); - } - } + // Apply all board background settings that are defined + if (bg?.cardOpacity !== undefined) setCardOpacity(projectPath, bg.cardOpacity); + if (bg?.columnOpacity !== undefined) setColumnOpacity(projectPath, bg.columnOpacity); + if (bg?.columnBorderEnabled !== undefined) setColumnBorderEnabled(projectPath, bg.columnBorderEnabled); + if (bg?.cardGlassmorphism !== undefined) setCardGlassmorphism(projectPath, bg.cardGlassmorphism); + if (bg?.cardBorderEnabled !== undefined) setCardBorderEnabled(projectPath, bg.cardBorderEnabled); + if (bg?.cardBorderOpacity !== undefined) setCardBorderOpacity(projectPath, bg.cardBorderOpacity); + if (bg?.hideScrollbar !== undefined) setHideScrollbar(projectPath, bg.hideScrollbar);The explicit approach trades brevity for full type safety—TypeScript will catch any type mismatches at compile time.
apps/ui/src/hooks/use-board-background-settings.ts (2)
16-24:awaitonmutate()has no effect; usemutateAsync()if awaiting is intended.
persistSettingscallsupdateProjectSettings.mutate(), which is fire-and-forget and returnsvoid. Theawaitin callers likesetBoardBackground(line 63) does nothing. This pattern repeats across all wrapper functions.If you need to wait for the mutation to complete (e.g., for error handling or sequencing), use
mutateAsync()instead. Otherwise, remove theasync/awaitkeywords to avoid confusion.Option 1: Use mutateAsync for true async behavior
const persistSettings = useCallback( (projectPath: string, settingsToUpdate: Record<string, unknown>) => { - updateProjectSettings.mutate({ + return updateProjectSettings.mutateAsync({ projectPath, settings: { boardBackground: settingsToUpdate }, }); }, [updateProjectSettings] );Option 2: Remove async/await if fire-and-forget is intentional
const setBoardBackground = useCallback( - async (projectPath: string, imagePath: string | null) => { + (projectPath: string, imagePath: string | null) => { const current = getCurrentSettings(projectPath); const toUpdate = { ...current, imagePath, imageVersion: imagePath ? Date.now() : undefined, }; store.setBoardBackground(projectPath, imagePath); - await persistSettings(projectPath, toUpdate); + persistSettings(projectPath, toUpdate); }, [store, persistSettings, getCurrentSettings] );Also applies to: 47-66
143-160: Consider extracting default settings to avoid duplication.The default values in
clearBoardBackground(lines 147-157) duplicate those ingetCurrentSettings(lines 31-40). Extracting to a shared constant would ensure consistency.Suggested refactor
+const DEFAULT_BACKGROUND_SETTINGS = { + imagePath: null, + imageVersion: undefined, + cardOpacity: 100, + columnOpacity: 100, + columnBorderEnabled: true, + cardGlassmorphism: true, + cardBorderEnabled: true, + cardBorderOpacity: 100, + hideScrollbar: false, +}; + export function useBoardBackgroundSettings() { // ... const getCurrentSettings = useCallback( (projectPath: string) => { const current = store.boardBackgroundByProject[projectPath]; - return ( - current || { - imagePath: null, - cardOpacity: 100, - // ... - } - ); + return current || DEFAULT_BACKGROUND_SETTINGS; }, [store.boardBackgroundByProject] ); // ... const clearBoardBackground = useCallback( async (projectPath: string) => { store.clearBoardBackground(projectPath); - await persistSettings(projectPath, { - imagePath: null, - // ... - }); + await persistSettings(projectPath, DEFAULT_BACKGROUND_SETTINGS); }, [store, persistSettings] );apps/ui/src/lib/query-client.ts (1)
131-138: Consider deduplicating error handling to avoid double-logging.The cache subscription calls
handleQueryErrorfor every query error, but individual queries using this client will also have their errors logged through the same handler if they don't provide their ownonError. This could lead to duplicate logs for some error scenarios.Additionally, queries that provide their own error handling may still trigger this global handler, potentially causing unexpected side effects like premature
handleServerOffline()calls.apps/ui/src/components/views/settings-view/hooks/use-cursor-status.ts (1)
47-49: TheuseCallbackwrapper aroundrefetchis unnecessary.
refetchfrom React Query is already a stable function reference. Wrapping it inuseCallbackadds indirection without benefit.♻️ Suggested simplification
- const loadData = useCallback(() => { - refetch(); - }, [refetch]); + const loadData = refetch;Alternatively, if you prefer keeping the same return semantics (void instead of Promise):
- const loadData = useCallback(() => { - refetch(); - }, [refetch]); + const loadData = useCallback(() => void refetch(), [refetch]);apps/ui/src/components/views/settings-view/providers/claude-settings-tab/hooks/use-subagents.ts (1)
63-68: Redundantrefetch()call afterinvalidateQueries().
invalidateQueries()already marks the query as stale and triggers a background refetch if there are active observers. Callingrefetch()immediately after creates a duplicate fetch.♻️ Suggested simplification
const refreshFilesystemAgents = useCallback(async () => { await queryClient.invalidateQueries({ queryKey: queryKeys.settings.agents(currentProject?.path ?? ''), }); - await refetch(); }, [queryClient, currentProject?.path, refetch]); + }, [queryClient, currentProject?.path]);apps/ui/src/hooks/mutations/use-ideation-mutations.ts (1)
10-10: Remove unusedtoastimport.The
toastimport is not used in this file. The comment on line 80 confirms that toast notifications are handled by the component.🧹 Suggested fix
import { getElectronAPI } from '@/lib/electron'; import { queryKeys } from '@/lib/query-keys'; -import { toast } from 'sonner'; import type { IdeaCategory, IdeaSuggestion } from '@automaker/types';apps/ui/src/hooks/queries/use-ideation.ts (1)
47-62: Consider cache key implications when query is disabled.When
projectPathis undefined, the queryKey uses an empty string (queryKeys.ideation.ideas('')). While the query is disabled, this creates a cache entry with an empty path. If multiple disabled queries exist, they share the same cache key, which could cause unexpected cache behavior when one gets enabled.A common alternative is to only compute the queryKey when enabled, or use a sentinel value like
'__disabled__'to make it explicit.💡 Alternative pattern
export function useIdeas(projectPath: string | undefined) { return useQuery({ - queryKey: queryKeys.ideation.ideas(projectPath ?? ''), + queryKey: queryKeys.ideation.ideas(projectPath!), queryFn: async () => { if (!projectPath) throw new Error('No project path'); // ... }, enabled: !!projectPath, staleTime: STALE_TIMES.FEATURES, }); }With
enabled: false, the queryFn won't run, so the!assertion is safe. However, the current approach also works correctly since the empty string key is effectively isolated by the disabled state.apps/ui/src/hooks/use-guided-prompts.ts (1)
50-53: ThehandleRefetchwrapper may be unnecessary.React Query's
refetch()already returns a Promise. The wrapper converts the return type but doesn't change behavior. If consumers don't rely on the void return type, you could exposerefetchdirectly.However, if the interface contract (
Promise<void>) is important for backward compatibility, this wrapper is appropriate.💡 Simplified alternative (if void return is not required)
- // Convert async refetch to match the expected interface - const handleRefetch = useCallback(async () => { - await refetch(); - }, [refetch]); - return { // ... - refetch: handleRefetch, + refetch, // ... };apps/ui/src/components/views/settings-view/worktrees/worktrees-section.tsx (1)
162-175: Consider migrating checkbox settings to React Query mutations for consistency.These checkbox handlers still use direct API calls with only
console.errorfor failures. While functional, this is inconsistent with the React Query migration pattern used for init scripts. Users won't know if settings fail to persist.This could be addressed in a follow-up to maintain consistency and provide better user feedback via toast notifications.
apps/ui/src/hooks/queries/use-models.ts (1)
60-94: Therefreshparameter doesn't affect cache key, leading to confusing behavior.Both
useCodexModelsanduseOpencodeModelsaccept arefreshparameter that's passed to the API but not included in the query key. This means:
useCodexModels(true)anduseCodexModels(false)share the same cache- The
refreshvalue only matters on the first call or after cache invalidationConsider one of these approaches:
Option 1: Include refresh in query key (if different cache entries are desired)
export function useCodexModels(refresh = false) { return useQuery({ - queryKey: queryKeys.models.codex(), + queryKey: [...queryKeys.models.codex(), { refresh }], queryFn: async (): Promise<CodexModel[]> => {Option 2: Remove parameter, use queryClient.invalidateQueries for refresh (recommended)
-export function useCodexModels(refresh = false) { +export function useCodexModels() { return useQuery({ queryKey: queryKeys.models.codex(), queryFn: async (): Promise<CodexModel[]> => { const api = getElectronAPI(); - const result = await api.codex.getModels(refresh); + const result = await api.codex.getModels(false);Then use
queryClient.invalidateQueries({ queryKey: queryKeys.models.codex() })when a refresh is needed.apps/ui/src/hooks/mutations/use-settings-mutations.ts (1)
69-114: Dual invocation pattern is clever but verifyprojectPathextraction inonSuccess.The flexible call pattern supporting both
useUpdateProjectSettings(projectPath)anduseUpdateProjectSettings().mutate({ projectPath, settings })is well-designed.However, on Line 102,
data.projectPathrelies on the spread from Line 99. If the API result already has aprojectPathproperty, it could shadow your added one. Consider being explicit:🔧 Suggested improvement
- return { ...result, projectPath: path }; + return { result, projectPath: path }; }, onSuccess: (data) => { - const path = data.projectPath || projectPath; + const path = data.projectPath;apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx (2)
65-74: Redundant refetch after invalidation.
invalidateQueriesalready triggers a refetch for active queries. The explicitawait refetchCliStatus()on Line 72 causes a duplicate fetch. Consider removing it:♻️ Suggested simplification
const handleRefreshOpencodeCli = useCallback(async () => { await Promise.all([ queryClient.invalidateQueries({ queryKey: queryKeys.cli.opencode() }), queryClient.invalidateQueries({ queryKey: queryKeys.models.opencodeProviders() }), queryClient.invalidateQueries({ queryKey: queryKeys.models.opencode() }), ]); - await refetchCliStatus(); toast.success('OpenCode CLI refreshed'); -}, [queryClient, refetchCliStatus]); +}, [queryClient]);
53-63: Type assertion onmethodmasks type safety without validating actual values.Line 58 asserts
cliStatusData.auth.method(which the API types as a genericstring) toOpencodeAuthStatus['method'], a union of five specific literals. The fallback|| 'none'only catches falsy values—if the API returns an unexpected string value, the assertion silently passes:method: (cliStatusData.auth.method as OpencodeAuthStatus['method']) || 'none',Consider validating the method against known values (
'api_key' | 'api_key_env' | 'oauth' | 'config_file') before using it, or ensure the upstream API type definition guarantees only valid values are returned.apps/ui/src/components/views/spec-view/hooks/use-spec-save.ts (1)
12-18:asynckeyword is misleading since mutation is fire-and-forget.The
saveSpecfunction is declaredasyncbutmutation.mutate()doesn't return a promise. This makesawait saveSpec()from callers return immediately without waiting for the save to complete.If callers need to await completion, use
mutateAsyncinstead:♻️ Option 1: Use mutateAsync for awaitable behavior
const saveSpec = async () => { if (!currentProject) return; - saveMutation.mutate(appSpec, { - onSuccess: () => setHasChanges(false), - }); + await saveMutation.mutateAsync(appSpec); + setHasChanges(false); };♻️ Option 2: Remove async if fire-and-forget is intended
-const saveSpec = async () => { +const saveSpec = () => { if (!currentProject) return; saveMutation.mutate(appSpec, { onSuccess: () => setHasChanges(false), }); };apps/ui/src/components/views/settings-view/providers/claude-settings-tab/hooks/use-skills-settings.ts (1)
20-48: Consider adding error handling for mutations.Both
updateEnabledandupdateSourcesonly handleonSuccess. If the mutation fails, users won't receive feedback. Consider addingonErrorcallbacks to show error toasts.Proposed improvement
updateSettingsMutation.mutate( { enableSkills: newEnabled }, { onSuccess: () => { useAppStore.setState({ enableSkills: newEnabled }); toast.success(newEnabled ? 'Skills enabled' : 'Skills disabled'); }, + onError: (error) => { + toast.error('Failed to update skills setting', { + description: error instanceof Error ? error.message : 'Unknown error', + }); + }, } );Apply the same pattern to
updateSources.apps/ui/src/hooks/queries/use-pipeline.ts (1)
24-38: Consider using a sentinel or conditional query key to avoid cache collisions.When
projectPathisundefined, the query key becomesqueryKeys.pipeline.config(''). If multiple unrelated components call this hook without a path, they would share the same cache entry (keyed by empty string), potentially leading to stale or incorrect data when a valid path is later provided.Since
enabled: !!projectPathprevents the query from executing whenprojectPathis falsy, thethrowon line 28 is effectively unreachable during normal operation—but the query key is still registered.Suggested improvement
export function usePipelineConfig(projectPath: string | undefined) { return useQuery({ - queryKey: queryKeys.pipeline.config(projectPath ?? ''), + queryKey: queryKeys.pipeline.config(projectPath!), queryFn: async (): Promise<PipelineConfig | null> => { - if (!projectPath) throw new Error('No project path'); const api = getHttpApiClient(); const result = await api.pipeline.getConfig(projectPath); if (!result.success) { throw new Error(result.error || 'Failed to fetch pipeline config'); } return result.config ?? null; }, enabled: !!projectPath, staleTime: STALE_TIMES.SETTINGS, }); }The non-null assertion (
!) is safe here becauseenabled: !!projectPathguaranteesprojectPathis truthy whenqueryFnexecutes. This also removes the redundant throw statement.apps/ui/src/hooks/queries/use-usage.ts (1)
59-77: Consider extracting shared query logic.Both
useClaudeUsageanduseCodexUsageare nearly identical. While the current approach is acceptable for clarity, you could reduce duplication with a factory function if more usage hooks are added in the future.♻️ Optional: Factory pattern for usage hooks
function createUsageHook<T>( queryKeyFn: () => readonly string[], apiFn: () => Promise<T | { error: string; message?: string }> ) { return function useUsage(enabled = true) { return useQuery({ queryKey: queryKeyFn(), queryFn: async (): Promise<T> => { const result = await apiFn(); if ('error' in result) { throw new Error(result.message || result.error); } return result; }, enabled, staleTime: STALE_TIMES.USAGE, refetchInterval: enabled ? USAGE_POLLING_INTERVAL : false, placeholderData: (previousData) => previousData, }); }; }apps/ui/src/hooks/queries/use-git.ts (1)
19-36: Verify empty-string query key behavior.The hook uses an empty string fallback for
queryKeywhenprojectPathis undefined (line 21). While the query is disabled in this case (enabled: !!projectPath && enabled), this pattern could potentially cause cache collisions if multiple disabled queries share the same['git', 'diffs', '']key.Consider returning early or using a more defensive pattern:
♻️ Alternative: Skip query when path is missing
export function useGitDiffs(projectPath: string | undefined, enabled = true) { return useQuery({ - queryKey: queryKeys.git.diffs(projectPath ?? ''), + queryKey: queryKeys.git.diffs(projectPath!), queryFn: async () => { - if (!projectPath) throw new Error('No project path'); const api = getElectronAPI(); const result = await api.git.getDiffs(projectPath); if (!result.success) { throw new Error(result.error || 'Failed to fetch diffs'); } return { files: result.files ?? [], diff: result.diff ?? '', }; }, enabled: !!projectPath && enabled, staleTime: STALE_TIMES.WORKTREES, }); }The non-null assertion is safe here because
queryFnonly runs whenenabledis true, which requiresprojectPathto be truthy.apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (2)
485-491: Consider removingasynckeyword fromhandleVerifyFeature.The function no longer awaits any promises since
mutation.mutate()is fire-and-forget. Theasynckeyword is now unnecessary.♻️ Minor cleanup
const handleVerifyFeature = useCallback( - async (feature: Feature) => { + (feature: Feature) => { if (!currentProject) return; verifyFeatureMutation.mutate(feature.id); }, [currentProject, verifyFeatureMutation] );
493-503: Consider removingasynckeyword fromhandleResumeFeature.Same as above—the function no longer awaits any promises.
♻️ Minor cleanup
const handleResumeFeature = useCallback( - async (feature: Feature) => { + (feature: Feature) => { logger.info('handleResumeFeature called for feature:', feature.id); if (!currentProject) { logger.error('No current project'); return; } resumeFeatureMutation.mutate({ featureId: feature.id, useWorktrees }); }, [currentProject, resumeFeatureMutation, useWorktrees] );apps/ui/src/components/views/spec-view/hooks/use-spec-generation.ts (1)
414-441: Consider addingonSuccesscallback for resilience.The mutation relies entirely on the WebSocket event subscription (lines 216-400) for success handling. If the WebSocket connection is interrupted or the event is missed, the UI state could become stale. Consider adding an
onSuccesscallback as a fallback to ensure the UI reflects the operation result.Suggested addition
createSpecMutation.mutate( { projectOverview: projectOverview.trim(), generateFeatures, analyzeProject: analyzeProjectOnCreate, featureCount: generateFeatures ? featureCountOnCreate : undefined, }, { + onSuccess: () => { + // Events handle the detailed state updates, but this ensures + // we don't get stuck if WebSocket events are missed + logger.debug('[useSpecGeneration] Spec creation request sent successfully'); + }, onError: (error) => { // ... existing error handling }, } );apps/ui/src/hooks/queries/use-workspace.ts (1)
12-15: Consider exportingWorkspaceDirectoryinterface.The interface is duplicated in
workspace-picker-modal.tsx(lines 13-16). Consider exporting it from this file to maintain a single source of truth.Suggested change
-interface WorkspaceDirectory { +export interface WorkspaceDirectory { name: string; path: string; }Then in
workspace-picker-modal.tsx:-interface WorkspaceDirectory { - name: string; - path: string; -} +import { useWorkspaceDirectories, type WorkspaceDirectory } from '@/hooks/queries';apps/ui/src/components/views/board-view/worktree-panel/hooks/use-branches.ts (1)
30-41: Good refetch logic, butrefetch()result is ignored.When calling
refetch()for the same path, the returned promise is discarded. If consumers expectfetchBranchesto be awaitable or want to handle refetch errors, this could be a limitation.Consider whether the interface should remain fire-and-forget or if error propagation is needed.
apps/ui/src/components/codex-usage-popover.tsx (1)
228-236: Consider disabling the refresh button during fetch.The button checks
!isFetchingbefore callingrefetch(), but visually it's not disabled. Users may click multiple times expecting action. Consider addingdisabled={isFetching}for better UX.Suggested improvement
<Button variant="ghost" size="icon" - className={cn('h-6 w-6', isFetching && 'opacity-80')} + className={cn('h-6 w-6', isFetching && 'opacity-80')} + disabled={isFetching} onClick={() => !isFetching && refetch()} >apps/ui/src/components/views/settings-view/hooks/use-cursor-permissions.ts (2)
1-3: Remove unuseduseEffectimport.
useEffectis imported but not used in this file.Suggested fix
-import { useState, useCallback, useEffect } from 'react'; +import { useState, useCallback } from 'react';
29-39: Consider cleanup for the timeout.The
setTimeoutfor resettingcopiedConfigworks, but if the component unmounts before the timeout fires, it could cause a React state update warning. This is a minor edge case.Optional: Add cleanup with useEffect
useEffect(() => { if (!copiedConfig) return; const timer = setTimeout(() => setCopiedConfig(false), 2000); return () => clearTimeout(timer); }, [copiedConfig]); const copyConfig = useCallback( (profileId: 'strict' | 'development') => { copyConfigMutation.mutate(profileId, { onSuccess: () => setCopiedConfig(true), }); }, [copyConfigMutation] );apps/ui/src/components/views/running-agents-view.tsx (1)
24-41: Stop button disables for all agents when any stop is pending.When
stopFeature.mutate()is called,stopFeature.isPendingbecomestrueand disables the Stop button for all running agents (line 172), not just the one being stopped. This may confuse users who want to stop multiple agents quickly.Consider tracking pending state per feature ID:
const [pendingStops, setPendingStops] = useState<Set<string>>(new Set()); const handleStopAgent = useCallback( (featureId: string) => { setPendingStops(prev => new Set(prev).add(featureId)); stopFeature.mutate(featureId, { onSettled: () => { setPendingStops(prev => { const next = new Set(prev); next.delete(featureId); return next; }); }, }); }, [stopFeature] );Then use
disabled={pendingStops.has(agent.featureId)}on the button.apps/ui/src/components/ui/git-diff-panel.tsx (1)
380-387: Consider using a type guard for cleaner error handling.The error handling logic could be simplified:
♻️ Optional refactor
- const error = queryError - ? queryError instanceof Error - ? queryError.message - : 'Failed to load diffs' - : null; + const error = queryError + ? (queryError instanceof Error ? queryError.message : 'Failed to load diffs') + : null;Or extract a utility function if this pattern is repeated elsewhere in the codebase.
apps/ui/src/components/usage-popover.tsx (1)
27-28: Unused constantREFRESH_INTERVAL_SECONDS.This constant is defined but never used in the component. Polling is now handled by React Query's configuration in the hooks. Consider removing it to avoid confusion.
♻️ Proposed fix
-// Fixed refresh interval (45 seconds) -const REFRESH_INTERVAL_SECONDS = 45;apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
100-113: Consider migrating polling to React Query's refetchInterval.The manual
setIntervalfor worktree polling works, but React Query can handle this natively viarefetchInterval. This would provide automatic cleanup and consistency with the init script hook's approach.♻️ Suggested approach
When migrating
useWorktreesto React Query, configure it with:useQuery({ queryKey: queryKeys.worktrees.list(projectPath), queryFn: fetchWorktreesFromAPI, refetchInterval: 5000, });This eliminates manual interval management and ensures proper cleanup.
apps/ui/src/hooks/queries/use-running-agents.ts (1)
55-61: Consider usingselectfor derived data.The current pattern works due to React Query's deduplication, but using the
selectoption provides a cleaner derivation with proper memoization:♻️ Suggested refactor using select
export function useRunningAgentsCount() { - const query = useRunningAgents(); - return { - ...query, - data: query.data?.count ?? 0, - }; + return useQuery({ + queryKey: queryKeys.runningAgents.all(), + queryFn: async (): Promise<RunningAgentsResult> => { + const api = getElectronAPI(); + const result = await api.runningAgents.getAll(); + if (!result.success) { + throw new Error(result.error || 'Failed to fetch running agents'); + } + return { + agents: result.runningAgents ?? [], + count: result.totalCount ?? 0, + }; + }, + staleTime: STALE_TIMES.RUNNING_AGENTS, + select: (data) => data.count, + }); }Alternatively, extract the queryFn to avoid duplication.
apps/ui/src/components/claude-usage-popover.tsx (1)
167-176: Adddisabledattribute for better accessibility.The button guards clicks with
!isFetchingbut doesn't setdisabled, which can confuse assistive technologies and users.♻️ Suggested fix
<Button variant="ghost" size="icon" className={cn('h-6 w-6', isFetching && 'opacity-80')} onClick={() => !isFetching && refetch()} + disabled={isFetching} > <RefreshCw className={cn('w-3.5 h-3.5', isFetching && 'animate-spin')} /> </Button>apps/ui/src/hooks/queries/use-spec.ts (1)
77-103: Good polling pattern withenabledtoggle.The hook correctly uses
refetchInterval: enabled ? 5000 : falseto enable/disable polling based on context. The graceful fallback for missingapi.specRegenerationis defensive.Minor: Consider extracting the hardcoded
5000to a constant (e.g.,STALE_TIMES.REGENERATION_STATUS) for consistency with other hooks.apps/ui/src/hooks/queries/use-settings.ts (1)
66-76: Inconsistent error handling compared to other hooks.
useSettingsStatusreturns the raw API result without checkingresult.success, while all other hooks in this file throw on failure. This inconsistency may confuse consumers expecting uniform behavior.♻️ Suggested fix for consistency
export function useSettingsStatus() { return useQuery({ queryKey: queryKeys.settings.status(), queryFn: async () => { const api = getElectronAPI(); const result = await api.settings.getStatus(); + if (!result.success) { + throw new Error(result.error || 'Failed to fetch settings status'); + } return result; }, staleTime: STALE_TIMES.SETTINGS, }); }apps/ui/src/components/views/board-view/hooks/use-board-features.ts (1)
27-32: UnusedrefetchfromuseFeatures.The
refetchfunction is destructured and renamed toloadFeatureson line 31, but it's never used. Instead,loadFeaturesis redefined in the return object (lines 164-168) to callinvalidateQueries. While both trigger a refetch, this is confusing.♻️ Simplify by removing unused destructuring
// Use React Query for features const { data: features = [], isLoading, - refetch: loadFeatures, } = useFeatures(currentProject?.path);Or use the
refetchdirectly:return { features, isLoading, persistedCategories, - loadFeatures: () => { - queryClient.invalidateQueries({ - queryKey: queryKeys.features.all(currentProject?.path ?? ''), - }); - }, + loadFeatures: refetch, loadCategories, saveCategory, };Also applies to: 164-168
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts (1)
19-28: Consider removing redundantasynckeyword.The handler functions are declared
asyncbut don't useawait. Sincemutation.mutate()is synchronous (fire-and-forget), theasynckeyword is unnecessary and could mislead readers into thinking these operations are awaited.Suggested change
const handleSwitchBranch = useCallback( - async (worktree: WorktreeInfo, branchName: string) => { + (worktree: WorktreeInfo, branchName: string) => { if (switchBranchMutation.isPending || branchName === worktree.branch) return; switchBranchMutation.mutate({ worktreePath: worktree.path, branchName, }); }, [switchBranchMutation] );Apply the same pattern to
handlePull,handlePush, andhandleOpenInEditor.apps/ui/src/hooks/mutations/use-github-mutations.ts (2)
47-48: Remove unusedqueryClientdeclaration.
queryClientis declared but never used inuseValidateIssue. The comment at lines 91-92 explains cache invalidation happens via WebSocket events, so this declaration is dead code.Suggested fix
export function useValidateIssue(projectPath: string) { - const queryClient = useQueryClient(); - return useMutation({
136-158: Consider refactoringuseGetValidationStatusto a query hook.This hook only fetches data without side effects, making it semantically a query rather than a mutation. Using
useMutationfor read-only operations loses React Query benefits like automatic caching, stale-while-revalidate, and background refetching.Suggested refactor to useQuery
export function useValidationStatus(projectPath: string, enabled = true) { return useQuery({ queryKey: queryKeys.github.validationStatus(projectPath), queryFn: async () => { const api = getElectronAPI(); if (!api.github?.getValidationStatus) { throw new Error('Validation status API not available'); } const result = await api.github.getValidationStatus(projectPath); if (!result.success) { throw new Error(result.error || 'Failed to get validation status'); } return result.runningIssues ?? []; }, enabled, staleTime: STALE_TIMES.RUNNING_AGENTS, // Running validations change frequently }); }apps/ui/src/hooks/mutations/use-worktree-mutations.ts (5)
84-107: Consider acceptingprojectPathfor consistent cache invalidation.The generic
['worktrees']invalidation is overly broad compared to other hooks that usequeryKeys.worktrees.all(projectPath). This will invalidate worktree caches for all projects, not just the affected one.Consider either:
- Accept
projectPathas a hook parameter (likeuseCreateWorktree)- Accept
projectPathin the mutation payload alongsideworktreePathThis would enable scoped invalidation:
queryKeys.worktrees.all(projectPath).♻️ Example refactor
-export function useCommitWorktree() { +export function useCommitWorktree(projectPath: string) { const queryClient = useQueryClient(); return useMutation({ mutationFn: async ({ worktreePath, message }: { worktreePath: string; message: string }) => { const api = getElectronAPI(); const result = await api.worktree.commit(worktreePath, message); if (!result.success) { throw new Error(result.error || 'Failed to commit changes'); } return result.result; }, - onSuccess: (_, { worktreePath }) => { - // Invalidate all worktree queries since we don't know the project path - queryClient.invalidateQueries({ queryKey: ['worktrees'] }); + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: queryKeys.worktrees.all(projectPath) }); toast.success('Changes committed'); },
114-165: Same broad invalidation pattern.Same suggestion applies here — consider accepting
projectPathas a hook parameter forusePushWorktreeandusePullWorktreeto enable scoped cache invalidation.
172-221: Good conditional handling, but could leverageprojectPathfor scoped invalidation.The toast logic handles different PR creation outcomes well. The
options?.projectPathis available in the payload — consider using it (when present) for more targeted invalidation:onSuccess: (result, { options }) => { - queryClient.invalidateQueries({ queryKey: ['worktrees'] }); + if (options?.projectPath) { + queryClient.invalidateQueries({ queryKey: queryKeys.worktrees.all(options.projectPath) }); + } else { + queryClient.invalidateQueries({ queryKey: ['worktrees'] }); + } queryClient.invalidateQueries({ queryKey: ['github', 'prs'] });
275-338: Same broad invalidation pattern as other hooks.Consider the same refactor for
useSwitchBranchanduseCheckoutBranchto acceptprojectPathfor scoped invalidation.
397-420: Consider usingprojectPathfor scoped invalidation.
projectPathis available in the mutation function. Consider using it for more targeted cache invalidation, or alternatively accept it as a hook parameter for consistency with other hooks.apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts (1)
31-36: Potential stale store data when worktrees list becomes empty.The condition
worktrees.length > 0prevents syncing an empty array to the store. While this avoids clearing during initial load (whendatais undefined), it also means if all worktrees are legitimately removed, the store retains stale data.Consider checking
dataexistence instead:Suggested improvement
// Sync worktrees to Zustand store when they change useEffect(() => { - if (worktrees.length > 0) { + if (data) { setWorktreesInStore(projectPath, worktrees); } - }, [worktrees, projectPath, setWorktreesInStore]); + }, [data, worktrees, projectPath, setWorktreesInStore]);
apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/worktree-panel/hooks/use-branches.ts
Show resolved
Hide resolved
Shironex
added
Testers-Requested
- Add cache invalidation to useBoardPersistence after create/update/delete - Add useAutoModeQueryInvalidation to board-view for WebSocket events - Add cache invalidation to github-issues-view after converting issue to task - Add cache invalidation to analysis-view after generating features - Fix UI not updating when features are added, updated, or completed Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update query keys to include all relevant parameters (branches, agents) - Fix use-branches to pass includeRemote parameter to query key - Fix use-settings to include sources in agents query key - Update running-agents-view to use correct query key structure - Update use-spec-loading to properly use spec query hooks - Add missing queryClient invalidation in auto-mode mutations - Add missing cache invalidation in spec mutations after creation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Merged latest changes from v0.13.0rc into feat/react-query while preserving React Query migration. Key merge decisions: - Kept React Query hooks for data fetching (useRunningAgents, useStopFeature, etc.) - Added backlog plan handling to running-agents-view stop functionality - Imported both SkeletonPulse and Spinner for CLI status components - Used Spinner for refresh buttons across all settings sections - Preserved isBacklogPlan check in agent-output-modal TaskProgressPanel - Added handleOpenInIntegratedTerminal to worktree actions while keeping React Query mutations
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/github-prs-view.tsx (1)
317-320:group-hoverwon’t trigger without agroupparent.The external-link button uses
group-hover:opacity-100, but the row container lacksgroup, so the hover state may never activate. Consider addinggroupto the row wrapper.✅ Minimal fix
- className={cn( - 'flex items-start gap-3 p-3 cursor-pointer hover:bg-accent/50 transition-colors', + className={cn( + 'group flex items-start gap-3 p-3 cursor-pointer hover:bg-accent/50 transition-colors', isSelected && 'bg-accent' )}Also applies to: 384-384
apps/ui/src/components/dialogs/workspace-picker-modal.tsx (1)
27-79: UseisErrorfrom React Query instead of relying onerror instanceof Error.The current error handling depends on
errorbeing anErrorinstance. While the hook implementation throwsErrorobjects, using the explicitisErrorboolean flag is more defensive and avoids false negatives where the UI falls through to the empty state instead of showing an error message. AddisErrorto the destructuring and use it to branch error UI, along with a safe fallback message for non-Errorfailures.Suggested fix
- const { data: directories = [], isLoading, error, refetch } = useWorkspaceDirectories(open); + const { data: directories = [], isLoading, isError, error, refetch } = useWorkspaceDirectories(open); - const errorMessage = error instanceof Error ? error.message : null; + const errorMessage = + isError ? (error instanceof Error ? error.message : 'Failed to load projects') : null; - {errorMessage && !isLoading && ( + {isError && !isLoading && ( <div className="flex flex-col items-center justify-center h-full gap-3 text-center px-4"> <div className="w-12 h-12 rounded-full bg-destructive/10 flex items-center justify-center"> <AlertCircle className="w-6 h-6 text-destructive" /> </div> <p className="text-sm text-destructive">{errorMessage}</p> <Button variant="secondary" size="sm" onClick={() => refetch()} className="mt-2"> Try Again </Button> </div> )} - {!isLoading && !errorMessage && directories.length === 0 && ( + {!isLoading && !isError && directories.length === 0 && ( <div className="flex flex-col items-center justify-center h-full gap-3 text-center px-4"> <div className="w-12 h-12 rounded-full bg-muted flex items-center justify-center"> <Folder className="w-6 h-6 text-muted-foreground" /> </div> <p className="text-sm text-muted-foreground"> No projects found in workspace directory </p> </div> )} - {!isLoading && !errorMessage && directories.length > 0 && ( + {!isLoading && !isError && directories.length > 0 && (
🤖 Fix all issues with AI agents
In `@apps/ui/src/components/claude-usage-popover.tsx`:
- Around line 20-32: The popover's authentication check (variable isCliVerified
in claude-usage-popover.tsx) is stricter than the other components—it's checking
claudeAuthStatus?.authenticated && claudeAuthStatus?.method ===
'cli_authenticated' before calling useClaudeUsage; to align behavior, remove the
method check and set isCliVerified to claudeAuthStatus?.authenticated (or, if
the stricter policy is desired, update claude-usage-section.tsx and
usage-popover.tsx to include the method check); update the isCliVerified
definition and any callers of useClaudeUsage accordingly so all three components
use the same authentication logic.
In `@apps/ui/src/components/views/ideation-view/components/prompt-list.tsx`:
- Line 63: The disabled state is inconsistent: the click-guard checks
generateMutation.isPending but the computed isDisabled (used for
styling/hover/cursor) does not; update the isDisabled calculation in the
prompt-list component to include generateMutation.isPending (alongside
loadingPromptId and generatingPromptIds.has(prompt.id)) so that when
generateMutation.isPending is true the card shows disabled styles and pointer
events are prevented for the prompt with prompt.id.
In
`@apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx`:
- Around line 65-74: The refresh flow in handleRefreshOpencodeCli needs error
handling: wrap the invalidateQueries + refetchCliStatus calls in a try/catch,
await the Promise.all and then await refetchCliStatus inside the try block, call
toast.success('OpenCode CLI refreshed') only on success, and in the catch block
call toast.error(...) with the caught error message (or formatted error) so
failures to invalidate or refetch show an error toast; reference the
handleRefreshOpencodeCli function, queryClient.invalidateQueries,
refetchCliStatus, toast.success and toast.error when making the change.
♻️ Duplicate comments (1)
apps/ui/src/components/views/board-view.tsx (1)
1409-1414: Dual source of truth is acceptable for gradual migration.The cache invalidation followed by Zustand update for backward compatibility is a pragmatic approach during migration. Consider tracking this as tech debt to eventually consolidate on React Query as the single source of truth once all consumers are migrated.
🧹 Nitpick comments (17)
apps/ui/src/components/ui/git-diff-panel.tsx (1)
355-390: Well-implemented React Query migration with correct hook patterns.The implementation correctly:
- Calls both hooks unconditionally (respecting Rules of Hooks)
- Uses parameter-based conditional enabling (
projectPath: undefineddisables the query)- Derives state from the appropriate hook based on
useWorktrees- Extracts
files,diffContent, anderrorfrom the selected hook's dataOne minor observation:
loadDiffs(the refetch function) is passed directly toonClickhandlers. While this works, the refetch function will receive the click event as an argument (which it ignores). This is a common pattern but could be made more explicit.💡 Optional: More explicit click handler (nitpick)
- const loadDiffs = useWorktrees ? refetchWorktree : refetchGit; + const loadDiffs = () => { + if (useWorktrees) { + refetchWorktree(); + } else { + refetchGit(); + } + };apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktree-actions.ts (1)
68-76: MissingisPendingguard.Unlike
handleSwitchBranch,handlePull, andhandlePush, this handler doesn't checkopenInEditorMutation.isPendingbefore callingmutate. Rapid clicks could trigger multiple editor open attempts.♻️ Add isPending guard for consistency
const handleOpenInEditor = useCallback( async (worktree: WorktreeInfo, editorCommand?: string) => { + if (openInEditorMutation.isPending) return; openInEditorMutation.mutate({ worktreePath: worktree.path, editorCommand, }); }, [openInEditorMutation] );apps/ui/src/components/views/running-agents-view.tsx (3)
26-26: Move logger creation outside the component or memoize it.Creating
loggerinside the component causes it to be recreated on every render. Since it's included in the dependency array ofhandleStopAgent(line 54), this triggers unnecessary callback recreation.♻️ Suggested fix
+const logger = createLogger('RunningAgentsView'); + export function RunningAgentsView() { const [selectedAgent, setSelectedAgent] = useState<RunningAgent | null>(null); const { setCurrentProject, projects } = useAppStore(); const navigate = useNavigate(); - - const logger = createLogger('RunningAgentsView');
77-83: Missingloggerin dependency array.
handleViewLogsusesloggerbut doesn't include it in the dependency array. This is a minor ESLint exhaustive-deps violation. If you moveloggeroutside the component as suggested above, this becomes a non-issue since module-level constants don't need to be in deps.
199-207: Consider disabling Stop button per-agent during mutation.The Stop button is disabled for all agents when any
stopFeaturemutation is pending. If a user wants to stop multiple agents quickly, they'd need to wait for each to complete. Consider tracking which agent is being stopped to only disable that specific button.♻️ Per-agent pending state tracking
+const [stoppingAgentId, setStoppingAgentId] = useState<string | null>(null); const handleStopAgent = useCallback( async (agent: RunningAgent) => { const api = getElectronAPI(); const isBacklogPlan = agent.featureId.startsWith('backlog-plan:'); if (isBacklogPlan && api.backlogPlan) { logger.debug('Stopping backlog plan agent', { featureId: agent.featureId }); + setStoppingAgentId(agent.featureId); await api.backlogPlan.stop(); + setStoppingAgentId(null); refetch(); return; } + setStoppingAgentId(agent.featureId); - stopFeature.mutate({ featureId: agent.featureId, projectPath: agent.projectPath }); + stopFeature.mutate( + { featureId: agent.featureId, projectPath: agent.projectPath }, + { onSettled: () => setStoppingAgentId(null) } + ); }, [stopFeature, refetch, logger] ); // In the button: -disabled={stopFeature.isPending} +disabled={stoppingAgentId === agent.featureId}apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx (1)
80-80: Consider extracting polling interval as a constant.The 3000ms polling interval is duplicated. Extract it to a named constant for clarity and easier adjustment.
♻️ Extract polling constant
+const AGENT_INFO_POLL_INTERVAL = 3000; + export function AgentInfoPanel({ // ... }) { // ... const { data: freshFeature } = useFeature(projectPath, feature.id, { enabled: shouldFetchData && !contextContent, - pollingInterval: shouldPoll ? 3000 : false, + pollingInterval: shouldPoll ? AGENT_INFO_POLL_INTERVAL : false, }); const { data: agentOutputContent } = useAgentOutput(projectPath, feature.id, { enabled: shouldFetchData && !contextContent, - pollingInterval: shouldPoll ? 3000 : false, + pollingInterval: shouldPoll ? AGENT_INFO_POLL_INTERVAL : false, });Also applies to: 86-86
apps/ui/src/components/views/analysis-view.tsx (1)
642-656: Consider error handling for partial failures in feature creation loop.The loop creates multiple features sequentially but doesn't track individual failures. If some features fail to create, the cache is still invalidated and
featureListGeneratedis set totrue, potentially showing a success message despite partial failures.💡 Optional: Track partial failures
+ const errors: string[] = []; for (const detectedFeature of detectedFeatures) { - await api.features.create(currentProject.path, { + const result = await api.features.create(currentProject.path, { id: generateUUID(), category: detectedFeature.category, description: detectedFeature.description, status: 'backlog', steps: [], } as any); + if (!result.success) { + errors.push(result.error || `Failed to create ${detectedFeature.category}`); + } } // Invalidate React Query cache to sync UI queryClient.invalidateQueries({ queryKey: queryKeys.features.all(currentProject.path), }); - setFeatureListGenerated(true); + if (errors.length > 0) { + setFeatureListError(`Created ${detectedFeatures.length - errors.length}/${detectedFeatures.length} features`); + } else { + setFeatureListGenerated(true); + }apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts (1)
50-55: Consider potential redundancy between local state update and cache invalidation.The code calls both
updateFeature(result.feature.id, result.feature)(local Zustand state) andqueryClient.invalidateQueries(). When the query refetches, it may overwrite the optimistically updated local state, causing a brief flicker or redundant re-render.This is acceptable during migration, but long-term you may want to either:
- Remove the local state update and rely solely on React Query's cache, or
- Use
queryClient.setQueryData()for optimistic updates instead of invalidationapps/ui/src/components/codex-usage-popover.tsx (1)
83-94: Error classification fallback may be inaccurate.The error mapping defaults to
AUTH_ERRORfor any message that doesn't match the "not available" or "bridge/API" patterns. This could incorrectly classify network errors, timeouts, or other non-auth-related failures as authentication errors, potentially showing misleading user guidance.💡 Consider using UNKNOWN as the default fallback
if (message.includes('bridge') || message.includes('API')) { return { code: ERROR_CODES.API_BRIDGE_UNAVAILABLE, message }; } - return { code: ERROR_CODES.AUTH_ERROR, message }; + if (message.includes('auth') || message.includes('login') || message.includes('credential')) { + return { code: ERROR_CODES.AUTH_ERROR, message }; + } + return { code: ERROR_CODES.UNKNOWN, message }; }, [queryError]);You'd also need to add a UI case for
UNKNOWNerrors with a generic message.apps/ui/src/components/views/settings-view/api-keys/claude-usage-section.tsx (1)
41-75: Consider aligning UsageCard with other implementations.This
UsageCardcomponent is simpler than the versions inusage-popover.tsxandclaude-usage-popover.tsx(missingisPrimaryandstaleprops). This may be intentional for the settings view, but consider extracting a shared component to reduce duplication across the three files.apps/ui/src/components/usage-popover.tsx (2)
28-29: Unused constant:REFRESH_INTERVAL_SECONDSappears to be dead code.This constant is defined but not used anywhere in the file. It was likely part of the previous manual polling implementation that has been replaced by React Query's built-in polling.
🧹 Proposed cleanup
-// Fixed refresh interval (45 seconds) -const REFRESH_INTERVAL_SECONDS = 45; - // Helper to format reset time for Codex
160-226: Consider extractingUsageCardto a shared component.This
UsageCardimplementation (withisPrimaryandstaleprops) is duplicated inclaude-usage-popover.tsx. Consider extracting it to a shared component to reduce code duplication.apps/ui/src/components/claude-usage-popover.tsx (2)
47-123: Duplicated components:ProgressBarandUsageCard.These components are nearly identical to the versions in
usage-popover.tsx. Consider extracting them to a shared location (e.g.,@/components/ui/usage-card.tsx) to reduce duplication and ensure consistent styling across the codebase.
182-194: Consider adding structured error handling for better user guidance.Unlike
usage-popover.tsxwhich parses errors into specific codes (TRUST_PROMPT,API_BRIDGE_UNAVAILABLE, etc.) and provides tailored messages, this component shows a generic message for all errors. Consider aligning the error handling for consistency and better UX.For example, if the error contains "Trust prompt" or "folder permission", guide the user to approve access in the terminal rather than suggesting
claude login.apps/ui/src/components/session-manager.tsx (1)
148-156: Avoid a potential double refetch ininvalidateSessions.
invalidateQueriesalready refetches active queries in React Query v5; callingrefetchSessions()immediately afterward can trigger a second request. Consider sticking to a single refresh path (either rely on invalidation + cache read, or just refetch once) to avoid redundant network calls.apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx (2)
128-128: Consider limiting loading UI to initial fetch.
UsingisFetchingalone will flag loading during background refreshes; if you only want initial-load indicators, gate on empty data orisLoading.♻️ Possible refinement
- const isLoadingDynamicModels = isFetchingProviders || isFetchingModels; + const isLoadingDynamicModels = + (isFetchingProviders && providersData.length === 0) || + (isFetchingModels && modelsData.length === 0);
135-153: Avoid repeated provider casts.
Consider hoisting theas OpenCodeProviderInfo[]cast to a singleprovidersconst (or type the hook result) to reduce duplication and improve readability.
| // Check if CLI is verified/authenticated | ||
| const isCliVerified = | ||
| claudeAuthStatus?.authenticated && claudeAuthStatus?.method === 'cli_authenticated'; | ||
|
|
||
| // Check if data is stale (older than 2 minutes) - recalculates when claudeUsageLastUpdated changes | ||
| const isStale = useMemo(() => { | ||
| return !claudeUsageLastUpdated || Date.now() - claudeUsageLastUpdated > 2 * 60 * 1000; | ||
| }, [claudeUsageLastUpdated]); | ||
|
|
||
| const fetchUsage = useCallback( | ||
| async (isAutoRefresh = false) => { | ||
| if (!isAutoRefresh) setLoading(true); | ||
| setError(null); | ||
| try { | ||
| const api = getElectronAPI(); | ||
| if (!api.claude) { | ||
| setError({ | ||
| code: ERROR_CODES.API_BRIDGE_UNAVAILABLE, | ||
| message: 'Claude API bridge not available', | ||
| }); | ||
| return; | ||
| } | ||
| const data = await api.claude.getUsage(); | ||
| if ('error' in data) { | ||
| // Detect trust prompt error | ||
| const isTrustPrompt = | ||
| data.error === 'Trust prompt pending' || | ||
| (data.message && data.message.includes('folder permission')); | ||
| setError({ | ||
| code: isTrustPrompt ? ERROR_CODES.TRUST_PROMPT : ERROR_CODES.AUTH_ERROR, | ||
| message: data.message || data.error, | ||
| }); | ||
| return; | ||
| } | ||
| setClaudeUsage(data); | ||
| } catch (err) { | ||
| setError({ | ||
| code: ERROR_CODES.UNKNOWN, | ||
| message: err instanceof Error ? err.message : 'Failed to fetch usage', | ||
| }); | ||
| } finally { | ||
| if (!isAutoRefresh) setLoading(false); | ||
| } | ||
| }, | ||
| [setClaudeUsage] | ||
| ); | ||
|
|
||
| // Auto-fetch on mount if data is stale (only if CLI is verified) | ||
| useEffect(() => { | ||
| if (isStale && isCliVerified) { | ||
| fetchUsage(true); | ||
| } | ||
| }, [isStale, isCliVerified, fetchUsage]); | ||
|
|
||
| useEffect(() => { | ||
| // Skip if CLI is not verified | ||
| if (!isCliVerified) return; | ||
|
|
||
| // Initial fetch when opened | ||
| if (open) { | ||
| if (!claudeUsage || isStale) { | ||
| fetchUsage(); | ||
| } | ||
| } | ||
| // Use React Query for usage data | ||
| const { | ||
| data: claudeUsage, | ||
| isLoading, | ||
| isFetching, | ||
| error, | ||
| dataUpdatedAt, | ||
| refetch, | ||
| } = useClaudeUsage(isCliVerified); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the authentication check patterns across Claude usage components
echo "=== claude-usage-section.tsx auth check ==="
rg -n "canFetchUsage|authenticated" apps/ui/src/components/views/settings-view/api-keys/claude-usage-section.tsx
echo ""
echo "=== claude-usage-popover.tsx auth check ==="
rg -n "isCliVerified|authenticated" apps/ui/src/components/claude-usage-popover.tsx
echo ""
echo "=== usage-popover.tsx auth check ==="
rg -n "isClaudeAuthenticated|authenticated" apps/ui/src/components/usage-popover.tsxRepository: AutoMaker-Org/automaker
Length of output: 1183
Inconsistent authentication check across Claude usage components.
claude-usage-popover.tsx checks both authenticated and method === 'cli_authenticated' (lines 21-22), while claude-usage-section.tsx (line 80) and usage-popover.tsx (line 70) only check claudeAuthStatus?.authenticated. Verify if this stricter check in the popover is intentional; if not, align the authentication logic across all three components.
🤖 Prompt for AI Agents
In `@apps/ui/src/components/claude-usage-popover.tsx` around lines 20 - 32, The
popover's authentication check (variable isCliVerified in
claude-usage-popover.tsx) is stricter than the other components—it's checking
claudeAuthStatus?.authenticated && claudeAuthStatus?.method ===
'cli_authenticated' before calling useClaudeUsage; to align behavior, remove the
method check and set isCliVerified to claudeAuthStatus?.authenticated (or, if
the stricter policy is desired, update claude-usage-section.tsx and
usage-popover.tsx to include the method check); update the isCliVerified
definition and any callers of useClaudeUsage accordingly so all three components
use the same authentication logic.
| } | ||
|
|
||
| if (loadingPromptId || generatingPromptIds.has(prompt.id)) return; | ||
| if (loadingPromptId || generateMutation.isPending || generatingPromptIds.has(prompt.id)) return; |
There was a problem hiding this comment.
Inconsistent disabled state: generateMutation.isPending not reflected in isDisabled.
The guard at line 63 correctly includes generateMutation.isPending, but the isDisabled variable at line 132 doesn't account for it. This causes cards to appear clickable (hover effects, pointer cursor) while the mutation is pending, even though clicks are blocked.
Proposed fix
- const isDisabled = loadingPromptId !== null || isGenerating;
+ const isDisabled = loadingPromptId !== null || isGenerating || generateMutation.isPending;Also applies to: 132-132
🤖 Prompt for AI Agents
In `@apps/ui/src/components/views/ideation-view/components/prompt-list.tsx` at
line 63, The disabled state is inconsistent: the click-guard checks
generateMutation.isPending but the computed isDisabled (used for
styling/hover/cursor) does not; update the isDisabled calculation in the
prompt-list component to include generateMutation.isPending (alongside
loadingPromptId and generatingPromptIds.has(prompt.id)) so that when
generateMutation.isPending is true the card shows disabled styles and pointer
events are prevented for the prompt with prompt.id.
| // Refresh all opencode-related queries | ||
| const handleRefreshOpencodeCli = useCallback(async () => { | ||
| setIsCheckingOpencodeCli(true); | ||
| setIsLoadingDynamicModels(true); | ||
| try { | ||
| const api = getElectronAPI(); | ||
| if (api?.setup?.getOpencodeStatus) { | ||
| const result = await api.setup.getOpencodeStatus(); | ||
| setCliStatus({ | ||
| success: result.success, | ||
| status: result.installed ? 'installed' : 'not_installed', | ||
| method: result.auth?.method, | ||
| version: result.version, | ||
| path: result.path, | ||
| recommendation: result.recommendation, | ||
| installCommands: result.installCommands, | ||
| }); | ||
| if (result.auth) { | ||
| setAuthStatus({ | ||
| authenticated: result.auth.authenticated, | ||
| method: (result.auth.method as OpencodeAuthStatus['method']) || 'none', | ||
| hasApiKey: result.auth.hasApiKey, | ||
| hasEnvApiKey: result.auth.hasEnvApiKey, | ||
| hasOAuthToken: result.auth.hasOAuthToken, | ||
| }); | ||
| } | ||
|
|
||
| if (result.installed) { | ||
| // Refresh providers | ||
| if (api?.setup?.getOpencodeProviders) { | ||
| const providersResult = await api.setup.getOpencodeProviders(); | ||
| if (providersResult.success && providersResult.providers) { | ||
| setCachedOpencodeProviders(providersResult.providers); | ||
| } | ||
| } | ||
|
|
||
| // Refresh dynamic models | ||
| if (api?.setup?.refreshOpencodeModels) { | ||
| const modelsResult = await api.setup.refreshOpencodeModels(); | ||
| if (modelsResult.success && modelsResult.models) { | ||
| setDynamicOpencodeModels(modelsResult.models); | ||
| } | ||
| } | ||
|
|
||
| toast.success('OpenCode CLI refreshed'); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| logger.error('Failed to refresh OpenCode CLI status:', error); | ||
| toast.error('Failed to refresh OpenCode CLI status'); | ||
| } finally { | ||
| setIsCheckingOpencodeCli(false); | ||
| setIsLoadingDynamicModels(false); | ||
| } | ||
| }, [setDynamicOpencodeModels, setCachedOpencodeProviders]); | ||
| await Promise.all([ | ||
| queryClient.invalidateQueries({ queryKey: queryKeys.cli.opencode() }), | ||
| queryClient.invalidateQueries({ queryKey: queryKeys.models.opencodeProviders() }), | ||
| queryClient.invalidateQueries({ queryKey: queryKeys.models.opencode() }), | ||
| ]); | ||
| await refetchCliStatus(); | ||
| toast.success('OpenCode CLI refreshed'); | ||
| }, [queryClient, refetchCliStatus]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx | head -100Repository: AutoMaker-Org/automaker
Length of output: 4434
🏁 Script executed:
# Search for invalidateQueries usage to understand the pattern
rg "invalidateQueries" apps/ui/src --type tsx --type ts -B 2 -A 2 | head -50Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
# Check if there's error handling in similar refresh/refetch operations
rg "refetch|invalidate" apps/ui/src/components --type tsx --type ts -B 3 -A 3 | head -80Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
# Search for invalidateQueries usage with proper syntax
rg "invalidateQueries" apps/ui/src -A 2 -B 2 | head -60Repository: AutoMaker-Org/automaker
Length of output: 4979
🏁 Script executed:
# Look for error handling in async operations in the same component directory
rg "try.*catch|catch.*error" apps/ui/src/components --type-list | head -30Repository: AutoMaker-Org/automaker
Length of output: 456
🏁 Script executed:
# Check React Query documentation pattern in the codebase
rg "Promise\.all.*invalidate" apps/ui/src -B 3 -A 5 | head -80Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Search for refetch error handling patterns and refetchCliStatus usage
rg "refetchCliStatus" apps/ui/src -B 2 -A 5Repository: AutoMaker-Org/automaker
Length of output: 2095
🏁 Script executed:
# Look for useOpencodeCliStatus hook definition
fd "hooks" -t d apps/ui/src | head -5Repository: AutoMaker-Org/automaker
Length of output: 322
🏁 Script executed:
# Check the query hooks file
find apps/ui/src -name "*queries*" -type fRepository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Find the queries hook file
find apps/ui/src/hooks -name "*queries*" -o -name "*opencode*" 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 92
🏁 Script executed:
# Look for hooks/queries directory
ls -la apps/ui/src/hooks/ 2>/dev/null | grep -E "queries|opencode"Repository: AutoMaker-Org/automaker
Length of output: 125
🏁 Script executed:
# List files in queries directory
ls -la apps/ui/src/hooks/queries/Repository: AutoMaker-Org/automaker
Length of output: 1215
🏁 Script executed:
# Search for useOpencodeCliStatus definition
rg "useOpencodeCliStatus" apps/ui/src/hooks/queries -A 10Repository: AutoMaker-Org/automaker
Length of output: 1593
Add error handling to the refresh flow.
The refetchCliStatus() call can reject if the underlying query fails. Currently, the success toast fires unconditionally even if invalidation or refetch fails. Add a try/catch block to handle errors, matching the error handling patterns already used elsewhere in this component.
🛠️ Suggested fix
- const handleRefreshOpencodeCli = useCallback(async () => {
- await Promise.all([
- queryClient.invalidateQueries({ queryKey: queryKeys.cli.opencode() }),
- queryClient.invalidateQueries({ queryKey: queryKeys.models.opencodeProviders() }),
- queryClient.invalidateQueries({ queryKey: queryKeys.models.opencode() }),
- ]);
- await refetchCliStatus();
- toast.success('OpenCode CLI refreshed');
- }, [queryClient, refetchCliStatus]);
+ const handleRefreshOpencodeCli = useCallback(async () => {
+ try {
+ await Promise.all([
+ queryClient.invalidateQueries({ queryKey: queryKeys.cli.opencode() }),
+ queryClient.invalidateQueries({ queryKey: queryKeys.models.opencodeProviders() }),
+ queryClient.invalidateQueries({ queryKey: queryKeys.models.opencode() }),
+ ]);
+ await refetchCliStatus();
+ toast.success('OpenCode CLI refreshed');
+ } catch {
+ toast.error('Failed to refresh OpenCode CLI');
+ }
+ }, [queryClient, refetchCliStatus]);📝 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.
| // Refresh all opencode-related queries | |
| const handleRefreshOpencodeCli = useCallback(async () => { | |
| setIsCheckingOpencodeCli(true); | |
| setIsLoadingDynamicModels(true); | |
| try { | |
| const api = getElectronAPI(); | |
| if (api?.setup?.getOpencodeStatus) { | |
| const result = await api.setup.getOpencodeStatus(); | |
| setCliStatus({ | |
| success: result.success, | |
| status: result.installed ? 'installed' : 'not_installed', | |
| method: result.auth?.method, | |
| version: result.version, | |
| path: result.path, | |
| recommendation: result.recommendation, | |
| installCommands: result.installCommands, | |
| }); | |
| if (result.auth) { | |
| setAuthStatus({ | |
| authenticated: result.auth.authenticated, | |
| method: (result.auth.method as OpencodeAuthStatus['method']) || 'none', | |
| hasApiKey: result.auth.hasApiKey, | |
| hasEnvApiKey: result.auth.hasEnvApiKey, | |
| hasOAuthToken: result.auth.hasOAuthToken, | |
| }); | |
| } | |
| if (result.installed) { | |
| // Refresh providers | |
| if (api?.setup?.getOpencodeProviders) { | |
| const providersResult = await api.setup.getOpencodeProviders(); | |
| if (providersResult.success && providersResult.providers) { | |
| setCachedOpencodeProviders(providersResult.providers); | |
| } | |
| } | |
| // Refresh dynamic models | |
| if (api?.setup?.refreshOpencodeModels) { | |
| const modelsResult = await api.setup.refreshOpencodeModels(); | |
| if (modelsResult.success && modelsResult.models) { | |
| setDynamicOpencodeModels(modelsResult.models); | |
| } | |
| } | |
| toast.success('OpenCode CLI refreshed'); | |
| } | |
| } | |
| } catch (error) { | |
| logger.error('Failed to refresh OpenCode CLI status:', error); | |
| toast.error('Failed to refresh OpenCode CLI status'); | |
| } finally { | |
| setIsCheckingOpencodeCli(false); | |
| setIsLoadingDynamicModels(false); | |
| } | |
| }, [setDynamicOpencodeModels, setCachedOpencodeProviders]); | |
| await Promise.all([ | |
| queryClient.invalidateQueries({ queryKey: queryKeys.cli.opencode() }), | |
| queryClient.invalidateQueries({ queryKey: queryKeys.models.opencodeProviders() }), | |
| queryClient.invalidateQueries({ queryKey: queryKeys.models.opencode() }), | |
| ]); | |
| await refetchCliStatus(); | |
| toast.success('OpenCode CLI refreshed'); | |
| }, [queryClient, refetchCliStatus]); | |
| // Refresh all opencode-related queries | |
| const handleRefreshOpencodeCli = useCallback(async () => { | |
| try { | |
| await Promise.all([ | |
| queryClient.invalidateQueries({ queryKey: queryKeys.cli.opencode() }), | |
| queryClient.invalidateQueries({ queryKey: queryKeys.models.opencodeProviders() }), | |
| queryClient.invalidateQueries({ queryKey: queryKeys.models.opencode() }), | |
| ]); | |
| await refetchCliStatus(); | |
| toast.success('OpenCode CLI refreshed'); | |
| } catch { | |
| toast.error('Failed to refresh OpenCode CLI'); | |
| } | |
| }, [queryClient, refetchCliStatus]); |
🤖 Prompt for AI Agents
In
`@apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx`
around lines 65 - 74, The refresh flow in handleRefreshOpencodeCli needs error
handling: wrap the invalidateQueries + refetchCliStatus calls in a try/catch,
await the Promise.all and then await refetchCliStatus inside the try block, call
toast.success('OpenCode CLI refreshed') only on success, and in the catch block
call toast.error(...) with the caught error message (or formatted error) so
failures to invalidate or refetch show an error toast; reference the
handleRefreshOpencodeCli function, queryClient.invalidateQueries,
refetchCliStatus, toast.success and toast.error when making the change.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/ui/src/components/views/chat-history.tsx`:
- Around line 272-275: The absolutely positioned div with className "absolute
left-0 right-0" that uses style={{ transform: `translateY(${offsetTop}px)` }}
should include an explicit top anchor to ensure consistent vertical positioning
across browsers; update that element (the div in the ChatHistory component
rendering the translated items) to add the "top-0" class to its className so it
starts at the top of its parent before the translateY is applied.
In `@apps/ui/src/components/views/graph-view/hooks/use-graph-nodes.ts`:
- Around line 86-88: The memoized nodes/edges in the useGraphNodes hook are
missing renderMode and enableEdgeAnimations in the useMemo dependency array, so
update the useMemo call inside useGraphNodes to include renderMode and
enableEdgeAnimations alongside the existing dependencies; this ensures changes
to renderMode (used in node/edge data) and enableEdgeAnimations (used for edge
animation) invalidate the memo and recompute nodes/edges correctly.
In
`@apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx`:
- Around line 55-63: The authStatus useMemo in opencode-settings-tab.tsx is not
mapping the optional error field from cliStatusData.auth into the
OpencodeAuthStatus object, which loses UI error details; update the return
object inside the authStatus useMemo (the function that builds
OpencodeAuthStatus) to include error: cliStatusData.auth.error (with the
appropriate cast to OpencodeAuthStatus['error'] or nullable type) so the
optional error is preserved and passed through to any components consuming
authStatus.
In `@apps/ui/src/styles/global.css`:
- Around line 1124-1127: The .perf-contain rule using content-visibility: auto
should include a contain-intrinsic-size to prevent layout shifts; update the
.perf-contain CSS (the rule that contains contain: layout paint and
content-visibility: auto) to add a contain-intrinsic-size with an appropriate
placeholder height (or use the "auto <length>" pattern, e.g., auto
<expected-height>) to reserve space until the element is painted, or if heights
vary significantly implement a dynamic approach using IntersectionObserver +
ResizeObserver to set contain-intrinsic-size at runtime.
♻️ Duplicate comments (5)
apps/ui/src/components/views/board-view.tsx (1)
1434-1439: Dual source of truth acknowledged but should be addressed.The cache invalidation is correctly implemented, but updating both React Query cache and Zustand store creates a dual source of truth. This was previously flagged in review comments.
Consider tracking this as technical debt to remove the Zustand state for pipeline config once all consumers migrate to React Query.
apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (1)
4-8: Import paths should use@automaker/*namespace.Per coding guidelines, imports should use
@automaker/*packages instead of relative@/paths. This applies to@/lib/utilsand@/store/app-store. Based on learnings,@automaker/utilsexports utilities likecn.apps/ui/src/components/views/board-view/components/kanban-column.tsx (1)
3-4: Import path should use@automaker/utils.Per coding guidelines, replace
'@/lib/utils'with'@automaker/utils'for thecnimport.apps/ui/src/components/views/board-view/kanban-board.tsx (1)
7-12: Import paths should use@automaker/*namespace.Per coding guidelines, replace
'@/store/app-store'and'@/lib/utils'with corresponding@automaker/*packages.apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx (1)
66-75: Add error handling around React Query invalidation/refetch.The refresh flow still unconditionally shows success; failures during invalidation/refetch should surface an error toast. This mirrors a concern already raised in earlier review comments for the same lines.
🛠️ Suggested fix
const handleRefreshOpencodeCli = useCallback(async () => { - await Promise.all([ - queryClient.invalidateQueries({ queryKey: queryKeys.cli.opencode() }), - queryClient.invalidateQueries({ queryKey: queryKeys.models.opencodeProviders() }), - queryClient.invalidateQueries({ queryKey: queryKeys.models.opencode() }), - ]); - await refetchCliStatus(); - toast.success('OpenCode CLI refreshed'); + try { + await Promise.all([ + queryClient.invalidateQueries({ queryKey: queryKeys.cli.opencode() }), + queryClient.invalidateQueries({ queryKey: queryKeys.models.opencodeProviders() }), + queryClient.invalidateQueries({ queryKey: queryKeys.models.opencode() }), + ]); + await refetchCliStatus(); + toast.success('OpenCode CLI refreshed'); + } catch { + toast.error('Failed to refresh OpenCode CLI'); + } }, [queryClient, refetchCliStatus]);
🧹 Nitpick comments (7)
apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx (1)
1-1: Consider removing@ts-nocheckdirective.This file-level TypeScript suppression disables all type checking, which can mask legitimate type errors introduced by future changes. If there are specific type issues, consider using targeted
@ts-expect-errorcomments with explanations instead.apps/ui/src/components/views/board-view/components/kanban-column.tsx (1)
20-24: New virtualization-support props are well-designed.The props provide a clean API for the parent
VirtualizedListto control scrolling and spacing. Consider adding JSDoc comments forcontentRef,onScroll, andcontentStyleto match the documentation style used forwidth.apps/ui/src/components/views/board-view/kanban-board.tsx (2)
190-204: Potential render cascade from measurement updates.Each call to
registerItemthat detects a size change triggerssetMeasureVersion, causing the entireVirtualizedListto re-render. With many items mounting simultaneously, this could cause multiple sequential renders.Consider batching measurement updates using a microtask or debounce:
♻️ Suggested batching approach
+ const pendingUpdatesRef = useRef(false); + const registerItem = useCallback( (id: string) => (node: HTMLDivElement | null) => { if (!node || !shouldVirtualize) return; const measuredHeight = node.getBoundingClientRect().height; const previousHeight = measurementsRef.current.get(id); if ( previousHeight === undefined || Math.abs(previousHeight - measuredHeight) > VIRTUALIZATION_MEASURE_EPSILON_PX ) { measurementsRef.current.set(id, measuredHeight); - setMeasureVersion((value) => value + 1); + if (!pendingUpdatesRef.current) { + pendingUpdatesRef.current = true; + queueMicrotask(() => { + pendingUpdatesRef.current = false; + setMeasureVersion((value) => value + 1); + }); + } } }, [shouldVirtualize] );
500-629: Significant code duplication between rendering paths.The virtualized (lines 536-584) and non-virtualized (lines 588-625) rendering paths duplicate most of the
KanbanCardprop wiring. Consider extracting a shared render function to reduce maintenance burden.♻️ Extract shared card renderer
const renderCard = (feature: Feature, index: number, isVirtualized: boolean) => { let shortcutKey: string | undefined; if (column.id === 'in_progress' && index < 10) { shortcutKey = index === 9 ? '0' : String(index + 1); } return ( <KanbanCard key={feature.id} feature={feature} onEdit={() => onEdit(feature)} onDelete={() => onDelete(feature.id)} // ... other props reduceEffects={reduceEffects} /> ); };Then use it in both paths with appropriate wrappers.
apps/ui/src/components/views/chat-history.tsx (1)
159-161: Add proper type annotation for thesessionparameter.Using
anybypasses type checking. Consider using the appropriate session type from your store or defining an interface.Suggested fix
- const handleSelectSession = (session: any) => { + const handleSelectSession = (session: ChatSession) => { setCurrentChatSession(session); };You'll need to import or define the
ChatSessiontype that matches your store's session structure.apps/ui/src/components/views/graph-view/components/dependency-edge.tsx (1)
66-135: Consider extracting the delete-button UI to avoid duplication.Compact and full render paths now duplicate the same button markup; a small shared helper keeps behavior/styling in sync.
apps/ui/src/components/views/graph-view/components/task-node.tsx (1)
143-147: Tailwind v4 important modifier should be postfix (bg-border!).
The new compact handle classes still use the old!prefix form.Based on learnings, Tailwind v4 uses postfix `!` for important utilities.♻️ Suggested update
- 'w-3 h-3 !bg-border border-2 border-background', + 'w-3 h-3 bg-border! border-2 border-background', 'transition-colors duration-200', - 'hover:!bg-brand-500', + 'hover:bg-brand-500!',- 'w-3 h-3 !bg-border border-2 border-background', + 'w-3 h-3 bg-border! border-2 border-background', 'transition-colors duration-200', - 'hover:!bg-brand-500', + 'hover:bg-brand-500!',Also applies to: 215-218
| <div | ||
| className="absolute left-0 right-0" | ||
| style={{ transform: `translateY(${offsetTop}px)` }} | ||
| > |
There was a problem hiding this comment.
Add top-0 to the absolutely positioned container for predictable positioning.
Without an explicit top value, the absolutely positioned element's vertical position may behave inconsistently across browsers. Since you're using transform: translateY to position items, the container should start at the top of its parent.
Suggested fix
<div
- className="absolute left-0 right-0"
+ className="absolute top-0 left-0 right-0"
style={{ transform: `translateY(${offsetTop}px)` }}
>📝 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.
| <div | |
| className="absolute left-0 right-0" | |
| style={{ transform: `translateY(${offsetTop}px)` }} | |
| > | |
| <div | |
| className="absolute top-0 left-0 right-0" | |
| style={{ transform: `translateY(${offsetTop}px)` }} | |
| > |
🤖 Prompt for AI Agents
In `@apps/ui/src/components/views/chat-history.tsx` around lines 272 - 275, The
absolutely positioned div with className "absolute left-0 right-0" that uses
style={{ transform: `translateY(${offsetTop}px)` }} should include an explicit
top anchor to ensure consistent vertical positioning across browsers; update
that element (the div in the ChatHistory component rendering the translated
items) to add the "top-0" class to its className so it starts at the top of its
parent before the translateY is applied.
| renderMode = GRAPH_RENDER_MODE_FULL, | ||
| enableEdgeAnimations = true, | ||
| }: UseGraphNodesProps) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/ui/src/components/views/graph-view/hooks && wc -l use-graph-nodes.tsRepository: AutoMaker-Org/automaker
Length of output: 89
🏁 Script executed:
cd apps/ui/src/components/views/graph-view/hooks && sed -n '70,110p' use-graph-nodes.tsRepository: AutoMaker-Org/automaker
Length of output: 1612
🏁 Script executed:
cd apps/ui/src/components/views/graph-view/hooks && sed -n '180,210p' use-graph-nodes.tsRepository: AutoMaker-Org/automaker
Length of output: 479
🏁 Script executed:
cd apps/ui/src/components/views/graph-view/hooks && sed -n '80,195p' use-graph-nodes.tsRepository: AutoMaker-Org/automaker
Length of output: 4580
Add renderMode and enableEdgeAnimations to the useMemo dependency array.
Both renderMode (used in node and edge data at lines ~119 and ~176) and enableEdgeAnimations (used in edge animation at line ~168) are consumed within the memoized callback. Without them in the dependency array, the memoized nodes/edges become stale if these props change independently of the other dependencies, causing the graph to render with outdated render mode or animation settings.
Proposed fix
- }, [features, runningAutoTasks, filterResult, actionCallbacks, backgroundSettings]);
+ }, [
+ features,
+ runningAutoTasks,
+ filterResult,
+ actionCallbacks,
+ backgroundSettings,
+ renderMode,
+ enableEdgeAnimations,
+ ]);🤖 Prompt for AI Agents
In `@apps/ui/src/components/views/graph-view/hooks/use-graph-nodes.ts` around
lines 86 - 88, The memoized nodes/edges in the useGraphNodes hook are missing
renderMode and enableEdgeAnimations in the useMemo dependency array, so update
the useMemo call inside useGraphNodes to include renderMode and
enableEdgeAnimations alongside the existing dependencies; this ensures changes
to renderMode (used in node/edge data) and enableEdgeAnimations (used for edge
animation) invalidate the memo and recompute nodes/edges correctly.
apps/ui/src/components/views/settings-view/providers/opencode-settings-tab.tsx
Show resolved
Hide resolved
|
@coderabbitai Review this pr now |
|
✅ Actions performedReview triggered.
|
DhanushSantosh
added
Enhancement
# Conflicts: # apps/ui/src/components/views/board-view.tsx # apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx # apps/ui/src/components/views/board-view/hooks/use-board-features.ts # apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx # apps/ui/src/hooks/use-project-settings-loader.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx (1)
284-286: Fix undefinedsetOutputin backlog-plan handler.
setOutputis no longer defined after the streaming migration, so this will throw. Append tostreamedContentinstead.🔧 Proposed fix
- if (newContent) { - setOutput((prev) => `${prev}${newContent}`); - } + if (newContent) { + setStreamedContent((prev) => prev + newContent); + }
🤖 Fix all issues with AI agents
In `@apps/ui/src/components/views/running-agents-view.tsx`:
- Around line 40-55: The backlog-plan branch in handleStopAgent calls await
api.backlogPlan.stop() without any error handling, so wrap that call in a
try/catch: call await api.backlogPlan.stop() inside a try, on success call
refetch(), and in the catch call logger.error('Failed to stop backlog plan', {
featureId: agent.featureId, error }) (and optionally surface a user notification
if available); keep the existing return after refetch to mirror the current flow
and leave the regular stopFeature.mutate path unchanged.
In `@apps/ui/src/hooks/use-project-settings-loader.ts`:
- Around line 30-47: The effect currently prevents re-applying settings when
only the settings object changes because it only compares
appliedProjectRef.current to currentProject.path; change the guard to also
consider React Query's dataUpdatedAt from useProjectSettings so subsequent
refetches re-apply: store both the project path and the settings' dataUpdatedAt
(or maintain a separate appliedUpdatedAt ref) and compare both
appliedProjectRef.current and settings.dataUpdatedAt before returning; when
applying settings update appliedProjectRef (or appliedUpdatedAt) with the new
dataUpdatedAt so future updates are detected.
♻️ Duplicate comments (1)
apps/ui/src/components/views/board-view.tsx (1)
1475-1480: Potential dual source of truth persists.
This still invalidates React Query and writes to Zustand. If both must exist, ensure they’re strictly synchronized or migrate remaining consumers.
🧹 Nitpick comments (4)
apps/ui/src/components/views/running-agents-view.tsx (2)
26-26: Move logger to module scope to avoid recreation on every render.Creating the logger inside the component causes a new instance each render. Since the context string
'RunningAgentsView'is static, move it outside the component. This also resolves the stale closure issue inhandleViewLogs(line 83) whereloggeris used but not in the dependency array.♻️ Suggested fix
+const logger = createLogger('RunningAgentsView'); + export function RunningAgentsView() { const [selectedAgent, setSelectedAgent] = useState<RunningAgent | null>(null); const { setCurrentProject, projects } = useAppStore(); const navigate = useNavigate(); - - const logger = createLogger('RunningAgentsView');Then update the dependency arrays:
[stopFeature, refetch, logger] + [stopFeature, refetch]
199-207: Consider per-agent pending state for better UX (optional).Currently
stopFeature.isPendingdisables all Stop buttons when any agent is being stopped. This is likely intentional to prevent race conditions. If per-agent feedback is desired, you could comparestopFeature.variables?.featureIdagainstagent.featureIdto only disable the specific button being acted upon.♻️ Optional enhancement
<Button variant="destructive" size="sm" onClick={() => handleStopAgent(agent)} - disabled={stopFeature.isPending} + disabled={stopFeature.isPending && stopFeature.variables?.featureId === agent.featureId} >apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx (1)
396-410: Add guard for consistency and clarity.
TaskProgressPanelsafely handles emptyprojectPathby returningnullwhen tasks cannot be loaded. However, add the guard for consistency with howGitDiffPanelis handled below (line 407) and to avoid unnecessary component lifecycle operations.Suggested guard
- {!isBacklogPlan && ( - <TaskProgressPanel - featureId={featureId} - projectPath={resolvedProjectPath} - className="shrink-0 mx-3 my-2" - /> - )} + {!isBacklogPlan && resolvedProjectPath && ( + <TaskProgressPanel + featureId={featureId} + projectPath={resolvedProjectPath} + className="shrink-0 mx-3 my-2" + /> + )}apps/ui/src/components/views/board-view/hooks/use-board-features.ts (1)
24-32: Disambiguate the twoloadFeaturesmeanings.
The localloadFeatures(refetch) and the exportedloadFeatures(invalidate) do different things, which is easy to misread. Consider renaming the refetch alias to avoid accidental misuse.♻️ Possible refactor
- const { - data: features = [], - isLoading, - refetch: loadFeatures, - } = useFeatures(currentProject?.path); + const { + data: features = [], + isLoading, + refetch: refetchFeatures, + } = useFeatures(currentProject?.path); ... - loadFeatures(); + refetchFeatures(); ... - loadFeatures(); + refetchFeatures();Also applies to: 108-116, 186-190
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts (1)
104-125: AddremoveFeaturecall to persist feature deletion to local store.
persistFeatureCreate(line 90) andpersistFeatureUpdate(line 52) both callupdateFeatureto sync the local store immediately after API success.persistFeatureDeleteshould similarly callremoveFeaturefor consistency, as the method exists inuseAppStoreand would keep the local store in sync with the API state. Without this, deleted features could remain in the store until the query cache refetch completes, risking UI inconsistency if the callback is invoked independently.
🧹 Nitpick comments (1)
apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts (1)
51-65: Redundant cache update pattern:setQueryDatafollowed byinvalidateQueriescauses unnecessary refetch.Lines 53-61 update the cache with the server response, then lines 63-65 immediately invalidate the same query, triggering a refetch. Since
result.featureis the authoritative data from the API,setQueryDataalone is sufficient—no refetch needed.♻️ Suggested fix: Remove the redundant invalidation
queryClient.setQueryData<Feature[]>( queryKeys.features.all(currentProject.path), (features) => { if (!features) return features; return features.map((feature) => feature.id === updatedFeature.id ? updatedFeature : feature ); } ); - // Invalidate React Query cache to sync UI - queryClient.invalidateQueries({ - queryKey: queryKeys.features.all(currentProject.path), - });
Summary
Changes Included (11 commits)
Foundation
Query Hooks
Mutation Hooks
Infrastructure
Component Migrations
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Performance
✏️ Tip: You can customize this high-level summary in your review settings.