feat: Add error handling to auto-mode facade and implement followUp f…#776
Conversation
…eature. Fix Claude weekly usage indicator. Fix mobile card drag
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds Claude usage integration to auto-mode, introduces facade-level error handling and a FacadeError type, improves Claude CLI ANSI/TTY parsing and tests, finalizes in-progress tasks on feature completion, adds weekly pace indicators to usage UI and utilities, and refactors Kanban drag-handle wiring and task-status rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Facade as AutoModeServiceFacade
participant FeatureStore as FeatureLoader/Storage
participant ClaudeService as ClaudeUsageService
participant Events as EventEmitter
Client->>Facade: followUpFeature(featureId, params)
Facade->>FeatureStore: loadFeature(featureId)
FeatureStore-->>Facade: feature + planSpec
Facade->>FeatureStore: readPreviousContext(featureId)
FeatureStore-->>Facade: previousContext
Facade->>ClaudeService: (optional) enrich/record usage (facadeOptions.claudeUsageService)
ClaudeService-->>Facade: usageAck (optional)
Facade->>Facade: build continuationPrompt, store imagePaths
Facade->>Facade: executeFeature(featureId, continuationPrompt)
Facade->>Events: emit auto_mode_task_started / events...
Facade-->>Client: ack / response
Note over Facade,Events: on non-abort errors -> handleFacadeError -> emit auto_mode_error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the auto-mode functionality by introducing robust error handling within the facade layer and fully implementing the agent follow-up feature. It also refines the display of Claude usage statistics with a new weekly pace indicator and corrects task status management upon feature completion. Additionally, the user interface sees an improvement in mobile drag-and-drop interactions for Kanban cards. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/ui/src/components/usage-popover.tsx (1)
415-431:⚠️ Potential issue | 🟠 MajorAdd
sonnetWeeklyResetTimetoClaudeUsagetype and use it for Sonnet card pace calculation.The Sonnet card at line 422 shows
sonnetResetTextindicating a separate reset schedule from the general weekly counter (e.g., "Resets Dec 27" vs "Resets Dec 22"), but calculatespacePercentageusingclaudeUsage.weeklyResetTime. This causes the pace indicator to be inaccurate.The
ClaudeUsagetype inapps/ui/src/store/types/usage-types.tsandapps/server/src/routes/claude/types.tsis missing asonnetWeeklyResetTimefield. Add this field and pass it togetExpectedWeeklyPacePercentage()for the Sonnet card instead ofweeklyResetTime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/usage-popover.tsx` around lines 415 - 431, The Sonnet card uses claudeUsage.weeklyResetTime for pacePercentage but its reset text comes from a separate sonnet schedule; add a sonnetWeeklyResetTime field to the ClaudeUsage type (in apps/ui/src/store/types/usage-types.ts and apps/server/src/routes/claude/types.ts) and populate it from the server, then update the Sonnet UsageCard instantiation in usage-popover.tsx to call getExpectedWeeklyPacePercentage(claudeUsage.sonnetWeeklyResetTime) instead of claudeUsage.weeklyResetTime so the pace indicator matches the Sonnet reset schedule.apps/ui/src/components/ui/task-progress-panel.tsx (1)
134-144:⚠️ Potential issue | 🟡 MinorInconsistent
currentTaskIdclearing betweentask_completeandtask_status.Line 142 unconditionally sets
setCurrentTaskId(null)when a task completes, which will clobber the current ID even if a different task is alreadyin_progress(e.g., if task_started fires for the next task before task_complete arrives for the previous one). The newerauto_mode_task_statushandler at line 159 correctly uses a conditional update:setCurrentTaskId((current) => (current === taskEvent.taskId ? null : current));Consider applying the same conditional pattern here for consistency:
Suggested fix
- setCurrentTaskId(null); + setCurrentTaskId((current) => (current === taskEvent.taskId ? null : current));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/task-progress-panel.tsx` around lines 134 - 144, The handler for the 'auto_mode_task_complete' case unconditionally calls setCurrentTaskId(null) which can clobber a newly started task; change it to use the same conditional updater used in the 'auto_mode_task_status' handler: call setCurrentTaskId((current) => (current === taskEvent.taskId ? null : current)) so the currentTaskId is only cleared when it matches taskEvent.taskId (locate in the case 'auto_mode_task_complete' where taskEvent is defined).apps/server/src/services/auto-mode/facade.ts (1)
663-676: 🛠️ Refactor suggestion | 🟠 Major
followUpFeatureduplicates error-handling logic instead of usinghandleFacadeError.The other wrapped methods (
startAutoLoop,stopAutoLoop,executeFeature,stopFeature) all delegate tohandleFacadeError. This catch block manually replicates the sameclassifyError+emitAutoModeEventpattern. Use the new helper for consistency.♻️ Suggested diff
} catch (error) { - const errorInfo = classifyError(error); - if (!errorInfo.isAbort) { - this.eventBus.emitAutoModeEvent('auto_mode_error', { - featureId, - featureName: undefined, - branchName: null, - error: errorInfo.message, - errorType: errorInfo.type, - projectPath: this.projectPath, - }); - } + this.handleFacadeError(error, 'followUpFeature', featureId); throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/auto-mode/facade.ts` around lines 663 - 676, The catch in followUpFeature duplicates the classifyError + emitAutoModeEvent logic; replace that manual handling by delegating to the existing helper: call this.handleFacadeError(error, { featureId, featureName: undefined, branchName: null, projectPath: this.projectPath }) (or the exact payload shape used by other callers) instead of running classifyError and eventBus.emitAutoModeEvent directly, then rethrow or return as handleFacadeError does to match the other methods (startAutoLoop/stopAutoLoop/executeFeature/stopFeature).apps/server/src/services/auto-mode/compat.ts (1)
31-42:⚠️ Potential issue | 🟡 Minor
claudeUsageServiceis passed toFacadeOptionsbut never extracted or used in facade creation.
claudeUsageServiceis added toFacadeOptionsintypes.tsand passed viafacadeOptionsincompat.tsline 42, butAutoModeServiceFacade.create()(facade.ts lines 138–144) does not destructure it. The value is silently dropped and never passed to any service initialization. Either wire it into the appropriate services that need to check usage limits, or remove the parameter if it's not yet needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/auto-mode/compat.ts` around lines 31 - 42, The FacadeOptions field claudeUsageService is being set in compat.ts but never consumed when building the facade; update AutoModeServiceFacade.create (and any internal destructuring in facade.ts around lines where FacadeOptions is handled) to accept and destructure claudeUsageService and then pass it into the service(s) that enforce usage limits (or otherwise need it) during initialization (or, if no service currently needs it, remove claudeUsageService from FacadeOptions and compat.ts). Specifically, modify AutoModeServiceFacade.create to include claudeUsageService in its parameter destructuring and forward that value into the appropriate service constructors or sharedServices wiring so it is not dropped.
🧹 Nitpick comments (8)
apps/ui/src/components/views/board-view/mobile-usage-bar.tsx (1)
206-216:getExpectedWeeklyPacePercentageis called on every render.This is a pure computation based on
new Date()andweeklyResetTime, so it's cheap. However, if the parent re-renders frequently (e.g., due to the 2-minute stale check or other store changes), you could memoize it withuseMemokeyed onclaudeUsage.weeklyResetTime. Not a performance concern today, just a minor note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/mobile-usage-bar.tsx` around lines 206 - 216, Pure computation getExpectedWeeklyPacePercentage is being invoked on every render; memoize its result with React's useMemo to avoid recomputing when unrelated state changes. Wrap the call passed to the Weekly UsageBar's pacePercentage prop in useMemo, e.g. const pace = useMemo(() => getExpectedWeeklyPacePercentage(claudeUsage.weeklyResetTime), [claudeUsage.weeklyResetTime]); and pass pace to the UsageBar (label "Weekly") instead of calling getExpectedWeeklyPacePercentage inline.apps/ui/src/components/usage-popover.tsx (1)
238-257: Empty<div />spacer when no paceLabel — consider a simpler approach.The empty
<div />works because ofjustify-between, but it adds a meaningless DOM node. You could instead conditionally applyjustify-endwhen there's no paceLabel, or just useml-autoon the resetText element.♻️ Optional simplification
- <div className="mt-2 flex items-center justify-between"> - {paceLabel ? ( - <p - className={cn( - 'text-[10px] font-medium', - safePercentage > (pacePercentage ?? 0) ? 'text-orange-500' : 'text-green-500' - )} - > - {paceLabel} - </p> - ) : ( - <div /> - )} - {resetText && ( + <div className="mt-2 flex items-center justify-between"> + {paceLabel && ( + <p + className={cn( + 'text-[10px] font-medium', + safePercentage > (pacePercentage ?? 0) ? 'text-orange-500' : 'text-green-500' + )} + > + {paceLabel} + </p> + )} + {resetText && ( - <p className="text-xs text-muted-foreground flex items-center gap-1"> + <p className="text-xs text-muted-foreground flex items-center gap-1 ml-auto"> <Clock className="w-3 h-3" /> {resetText} </p> - )} - </div> + )} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/usage-popover.tsx` around lines 238 - 257, The empty <div /> used as a spacer inside the container with class "mt-2 flex items-center justify-between" is unnecessary; update the JSX so you don't render a meaningless node — either conditionally set the container's justify class to "justify-between" when paceLabel is present and "justify-end" when it's not, or remove the spacer and add "ml-auto" to the resetText <p> (the element referenced by resetText and the container wrapper around paceLabel) so layout stays the same without the extra DOM node.apps/ui/src/store/utils/usage-utils.ts (1)
53-59: "Ahead/behind pace" labels may read as inverted to users.When
actual > expected(using more budget than expected), the label says "behind pace." In common English, "behind pace" means slower than a target rate — but here the user is consuming faster. Consider whether "ahead of pace" / "behind pace" should be swapped, or use clearer phrasing like "X% over budget pace" / "X% under budget pace."This is a UX / copy concern rather than a code bug — the code comments do explain the intent, so feel free to keep it if the team has aligned on the wording.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/utils/usage-utils.ts` around lines 53 - 59, The labels are potentially inverted: when actualPercentage > expectedPercentage (diff > 0) the UI shows "behind pace" which reads as consuming faster; change the copy mapping in this block that computes diff (using actualPercentage and expectedPercentage) so that diff > 0 yields a clear "over/over budget/over pace" message (e.g., "X% over budget pace" or "X% over pace") and diff < 0 yields "under/under budget pace" (e.g., "X% under budget pace"), updating the return strings accordingly to avoid the ahead/behind ambiguity.apps/ui/src/components/ui/task-progress-panel.tsx (2)
40-40:currentTaskIdstate is written but never read.The state value is discarded (
[, setCurrentTaskId]), meaning the component re-renders on everysetCurrentTaskIdcall for no observable effect. If it's only bookkeeping, auseRefwould avoid unnecessary re-renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/task-progress-panel.tsx` at line 40, The currentTaskId state is created but its value is never read (const [, setCurrentTaskId] = useState...), causing unnecessary re-renders when setCurrentTaskId is called; replace this useState with a useRef (e.g., const currentTaskIdRef = useRef<string | null>(null)) and update all places that call setCurrentTaskId to instead set currentTaskIdRef.current so you retain the bookkeeping without triggering component re-renders; ensure any logic that actually needs to read the id uses currentTaskIdRef.current or reinstates state only where a read is required.
146-162:auto_mode_task_statusevent carries a fulltasksarray that is ignored.Per the type definition, this event includes
tasks: ParsedTask[]alongside the singletaskId/status. The handler only applies the single-task update. If the server sends a reconciled task list (e.g., after a reconnect or bulk status change), the UI could drift out of sync.Consider whether a full-state reconciliation from
taskEvent.taskswould be more robust here, or document why the incremental approach is preferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/task-progress-panel.tsx` around lines 146 - 162, The handler for the 'auto_mode_task_status' event currently only applies an incremental update via setTasks and ignores the event's tasks array; change it to reconcile full state from taskEvent.tasks (the ParsedTask[] carried on the event) instead of only updating one task: use setTasks(() => taskEvent.tasks.map(/* convert ParsedTask to TaskInfo shape */)) to replace the task list, and update setCurrentTaskId based on the reconciled list (e.g., set to the task with status 'in_progress' or null if none) so the UI cannot drift out of sync; keep the existing single-task update logic only if you intentionally want incremental updates and document that choice.apps/server/src/services/claude-usage-service.ts (1)
297-306: Duplicated lightweight ANSI-cleaning regex in two places.The same two-step regex (cursor-forward → spaces, then strip remaining CSI) appears in both the timeout handler (lines 299-301) and the
onDatacallback (lines 336-338). A small private helper would eliminate the duplication and make future regex fixes single-site.♻️ Example helper
+ /** Lightweight ANSI clean for quick checks (not full stripAnsiCodes). */ + private quickClean(text: string): string { + return text + .replace(/\x1B\[(\d+)C/g, (_m: string, n: string) => ' '.repeat(parseInt(n, 10))) + .replace(/\x1B\[[0-9;?]*[A-Za-z@]/g, ''); + }Also applies to: 336-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/claude-usage-service.ts` around lines 297 - 306, Introduce a small private helper (e.g., stripAnsiForCheck or cleanOutputForCheck) that encapsulates the two-step ANSI-cleaning logic currently duplicated: the cursor-forward replacement using /\x1B\[(\d+)C/g and the CSI strip with /\x1B\[[0-9;?]*[A-Za-z@]/g, then replace the duplicated inline logic in both the timeout handler (where cleanedForCheck is created) and the onData callback with calls to this helper so both locations use the single shared implementation.apps/server/src/services/feature-state-manager.ts (1)
108-150: Production code is correct, but the task finalization logic is duplicated.The
waiting_approvalandverifiedbranches share nearly identical task-finalization logic (markin_progress→completed, recount, clearcurrentTaskId). Consider extracting a small helper to reduce duplication.Note: the pipeline failures are caused by incorrect test expectations (see review on the test file), not by this code.
♻️ Example extraction
+ /** + * Finalize in_progress tasks to completed and update counters. + */ + private finalizeInProgressTasks(planSpec: PlanSpec): number { + let tasksFinalized = 0; + if (planSpec.tasks) { + for (const task of planSpec.tasks) { + if (task.status === 'in_progress') { + task.status = 'completed'; + tasksFinalized++; + } + } + planSpec.tasksCompleted = planSpec.tasks.filter((t) => t.status === 'completed').length; + planSpec.currentTaskId = undefined; + } + return tasksFinalized; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/feature-state-manager.ts` around lines 108 - 150, Extract the duplicated task-finalization logic into a small helper (e.g., finalizeTasksForCompletion or finalizeInProgressTasks) that accepts the feature or its planSpec, iterates planSpec.tasks to mark any task.status === 'in_progress' → 'completed', recalculates planSpec.tasksCompleted, and clears planSpec.currentTaskId; then call this helper from both the status === 'waiting_approval' and status === 'verified' branches so only justFinishedAt handling remains in each branch (keep existing behavior of not touching pending tasks).apps/server/src/services/auto-mode/types.ts (1)
116-131: Prefer a top-level import forErrorTypeinstead of an inlineimport()type.
@automaker/typesis already imported on line 11. AddingErrorTypeto that import keeps the type surface consistent and avoids the inlineimport(…)noise.♻️ Suggested diff
-import type { Feature, ModelProvider } from '@automaker/types'; +import type { Feature, ModelProvider, ErrorType } from '@automaker/types';/** Classified error type from the error handler */ - errorType: import('@automaker/types').ErrorType; + errorType: ErrorType;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/auto-mode/types.ts` around lines 116 - 131, The FacadeError interface uses an inline import() for ErrorType; change this to a top-level import by adding ErrorType to the existing import from '@automaker/types' (the file's imports at the top) and then replace "errorType: import('@automaker/types').ErrorType;" inside the FacadeError interface with "errorType: ErrorType;". This removes the inline import noise while keeping the type reference consistent for the FacadeError definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 646-657: The code currently sets feature.imagePaths using
p.split('/') which breaks on Windows and then calls
this.featureStateManager.updateFeatureStatus which re-reads the feature from
disk so the mutated in-memory imagePaths are never persisted and causes unwanted
side effects; replace p.split('/').pop() with path.basename(p) and persist the
new imagePaths explicitly instead of calling updateFeatureStatus: after setting
feature.imagePaths call the existing atomicWriteJson helper (or whichever
project-level atomic write utility is used) to write the feature JSON file for
this.projectPath/featureId so the imagePaths are saved without changing
timestamps or emitting status events, or alternatively pass imagePaths through
the executeFeature/options path (e.g., via the ExecutionService/executeFeature
options) if that flow is already supported.
In `@apps/server/tests/unit/services/feature-state-manager.test.ts`:
- Around line 154-189: The test incorrectly expects pending tasks to be marked
completed; update the test in
apps/server/tests/unit/services/feature-state-manager.test.ts so it matches
production logic: rename the test to remove "and pending tasks", assert that
only the in_progress task (task-2) is marked 'completed' while the pending task
(task-3) remains 'pending', ensure currentTaskId is cleared
(savedFeature.planSpec?.currentTaskId is undefined) and tasksCompleted reflects
only the completed tasks (e.g., 2 if two tasks are completed), referencing
manager.updateFeatureStatus and savedFeature.planSpec?.tasks to locate
assertions to change.
- Around line 191-228: The test incorrectly asserts that a 'pending' task is
marked 'completed' when updating to 'verified'—update the expectations in the
test for the updateFeatureStatus flow (the assertions reading savedFeature from
the atomicWriteJson mock) so only the 'in_progress' task(s) are expected to
become 'completed' (leave 'pending' tasks unchanged), set tasksCompleted to the
new count of completed tasks (e.g., 2) and keep assertions for clearing
currentTaskId and justFinishedAt as-is; locate assertions referencing
savedFeature.planSpec?.tasks, savedFeature.planSpec?.tasksCompleted,
savedFeature.planSpec?.currentTaskId and savedFeature.justFinishedAt and adjust
their expected values accordingly.
In `@apps/ui/src/components/ui/task-progress-panel.tsx`:
- Around line 65-74: The code is casting ParsedTask.status and taskEvent.status
unsafely which allows 'failed' to enter TaskInfo state and break rendering;
update the mapping that builds initialTasks (the map over planTasks producing
TaskInfo objects where status is currently assigned using (t.status as
TaskInfo['status']) || 'pending') and the handler that assigns taskEvent.status
(where taskEvent.status is cast directly) to explicitly normalize statuses to
the allowed TaskInfo['status'] values ('in_progress' | 'completed' | 'pending'):
implement a small helper like normalizeTaskStatus(status: ParsedTask['status'] |
string): TaskInfo['status'] and call it both in the initialTasks creation and
when consuming taskEvent.status (instead of direct casts or relying on falsy
fallbacks) so any unknown or disallowed values (including 'failed') map to
'pending' (or another defined default).
---
Outside diff comments:
In `@apps/server/src/services/auto-mode/compat.ts`:
- Around line 31-42: The FacadeOptions field claudeUsageService is being set in
compat.ts but never consumed when building the facade; update
AutoModeServiceFacade.create (and any internal destructuring in facade.ts around
lines where FacadeOptions is handled) to accept and destructure
claudeUsageService and then pass it into the service(s) that enforce usage
limits (or otherwise need it) during initialization (or, if no service currently
needs it, remove claudeUsageService from FacadeOptions and compat.ts).
Specifically, modify AutoModeServiceFacade.create to include claudeUsageService
in its parameter destructuring and forward that value into the appropriate
service constructors or sharedServices wiring so it is not dropped.
In `@apps/server/src/services/auto-mode/facade.ts`:
- Around line 663-676: The catch in followUpFeature duplicates the classifyError
+ emitAutoModeEvent logic; replace that manual handling by delegating to the
existing helper: call this.handleFacadeError(error, { featureId, featureName:
undefined, branchName: null, projectPath: this.projectPath }) (or the exact
payload shape used by other callers) instead of running classifyError and
eventBus.emitAutoModeEvent directly, then rethrow or return as handleFacadeError
does to match the other methods
(startAutoLoop/stopAutoLoop/executeFeature/stopFeature).
In `@apps/ui/src/components/ui/task-progress-panel.tsx`:
- Around line 134-144: The handler for the 'auto_mode_task_complete' case
unconditionally calls setCurrentTaskId(null) which can clobber a newly started
task; change it to use the same conditional updater used in the
'auto_mode_task_status' handler: call setCurrentTaskId((current) => (current ===
taskEvent.taskId ? null : current)) so the currentTaskId is only cleared when it
matches taskEvent.taskId (locate in the case 'auto_mode_task_complete' where
taskEvent is defined).
In `@apps/ui/src/components/usage-popover.tsx`:
- Around line 415-431: The Sonnet card uses claudeUsage.weeklyResetTime for
pacePercentage but its reset text comes from a separate sonnet schedule; add a
sonnetWeeklyResetTime field to the ClaudeUsage type (in
apps/ui/src/store/types/usage-types.ts and
apps/server/src/routes/claude/types.ts) and populate it from the server, then
update the Sonnet UsageCard instantiation in usage-popover.tsx to call
getExpectedWeeklyPacePercentage(claudeUsage.sonnetWeeklyResetTime) instead of
claudeUsage.weeklyResetTime so the pace indicator matches the Sonnet reset
schedule.
---
Nitpick comments:
In `@apps/server/src/services/auto-mode/types.ts`:
- Around line 116-131: The FacadeError interface uses an inline import() for
ErrorType; change this to a top-level import by adding ErrorType to the existing
import from '@automaker/types' (the file's imports at the top) and then replace
"errorType: import('@automaker/types').ErrorType;" inside the FacadeError
interface with "errorType: ErrorType;". This removes the inline import noise
while keeping the type reference consistent for the FacadeError definition.
In `@apps/server/src/services/claude-usage-service.ts`:
- Around line 297-306: Introduce a small private helper (e.g., stripAnsiForCheck
or cleanOutputForCheck) that encapsulates the two-step ANSI-cleaning logic
currently duplicated: the cursor-forward replacement using /\x1B\[(\d+)C/g and
the CSI strip with /\x1B\[[0-9;?]*[A-Za-z@]/g, then replace the duplicated
inline logic in both the timeout handler (where cleanedForCheck is created) and
the onData callback with calls to this helper so both locations use the single
shared implementation.
In `@apps/server/src/services/feature-state-manager.ts`:
- Around line 108-150: Extract the duplicated task-finalization logic into a
small helper (e.g., finalizeTasksForCompletion or finalizeInProgressTasks) that
accepts the feature or its planSpec, iterates planSpec.tasks to mark any
task.status === 'in_progress' → 'completed', recalculates
planSpec.tasksCompleted, and clears planSpec.currentTaskId; then call this
helper from both the status === 'waiting_approval' and status === 'verified'
branches so only justFinishedAt handling remains in each branch (keep existing
behavior of not touching pending tasks).
In `@apps/ui/src/components/ui/task-progress-panel.tsx`:
- Line 40: The currentTaskId state is created but its value is never read (const
[, setCurrentTaskId] = useState...), causing unnecessary re-renders when
setCurrentTaskId is called; replace this useState with a useRef (e.g., const
currentTaskIdRef = useRef<string | null>(null)) and update all places that call
setCurrentTaskId to instead set currentTaskIdRef.current so you retain the
bookkeeping without triggering component re-renders; ensure any logic that
actually needs to read the id uses currentTaskIdRef.current or reinstates state
only where a read is required.
- Around line 146-162: The handler for the 'auto_mode_task_status' event
currently only applies an incremental update via setTasks and ignores the
event's tasks array; change it to reconcile full state from taskEvent.tasks (the
ParsedTask[] carried on the event) instead of only updating one task: use
setTasks(() => taskEvent.tasks.map(/* convert ParsedTask to TaskInfo shape */))
to replace the task list, and update setCurrentTaskId based on the reconciled
list (e.g., set to the task with status 'in_progress' or null if none) so the UI
cannot drift out of sync; keep the existing single-task update logic only if you
intentionally want incremental updates and document that choice.
In `@apps/ui/src/components/usage-popover.tsx`:
- Around line 238-257: The empty <div /> used as a spacer inside the container
with class "mt-2 flex items-center justify-between" is unnecessary; update the
JSX so you don't render a meaningless node — either conditionally set the
container's justify class to "justify-between" when paceLabel is present and
"justify-end" when it's not, or remove the spacer and add "ml-auto" to the
resetText <p> (the element referenced by resetText and the container wrapper
around paceLabel) so layout stays the same without the extra DOM node.
In `@apps/ui/src/components/views/board-view/mobile-usage-bar.tsx`:
- Around line 206-216: Pure computation getExpectedWeeklyPacePercentage is being
invoked on every render; memoize its result with React's useMemo to avoid
recomputing when unrelated state changes. Wrap the call passed to the Weekly
UsageBar's pacePercentage prop in useMemo, e.g. const pace = useMemo(() =>
getExpectedWeeklyPacePercentage(claudeUsage.weeklyResetTime),
[claudeUsage.weeklyResetTime]); and pass pace to the UsageBar (label "Weekly")
instead of calling getExpectedWeeklyPacePercentage inline.
In `@apps/ui/src/store/utils/usage-utils.ts`:
- Around line 53-59: The labels are potentially inverted: when actualPercentage
> expectedPercentage (diff > 0) the UI shows "behind pace" which reads as
consuming faster; change the copy mapping in this block that computes diff
(using actualPercentage and expectedPercentage) so that diff > 0 yields a clear
"over/over budget/over pace" message (e.g., "X% over budget pace" or "X% over
pace") and diff < 0 yields "under/under budget pace" (e.g., "X% under budget
pace"), updating the return strings accordingly to avoid the ahead/behind
ambiguity.
| if (imagePaths && imagePaths.length > 0) { | ||
| feature.imagePaths = imagePaths.map((p) => ({ | ||
| path: p, | ||
| filename: p.split('/').pop() || p, | ||
| mimeType: 'image/*', | ||
| })); | ||
| await this.featureStateManager.updateFeatureStatus( | ||
| this.projectPath, | ||
| featureId, | ||
| feature.status || 'in_progress' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Two concerns with imagePaths persistence.
-
p.split('/').pop()will fail on Windows paths (backslash separators). Usepath.basename(p)instead —pathis already imported at line 14. -
Calling
updateFeatureStatuswith the existing status solely to persistimagePathsis a side-effect-heavy workaround: it will overwriteupdatedAt, clearjustFinishedAt, and emit afeature_status_changedevent. The image paths are set on the in-memoryfeatureobject, butupdateFeatureStatusre-reads from disk, so these imagePaths will be lost — the method loads its own copy of the feature from disk and never sees the mutation at line 647.
🐛 Fix: persist imagePaths via a dedicated write or updateFeaturePlanSpec
The current approach mutates the local feature object's imagePaths but then calls updateFeatureStatus which re-reads the feature from disk — so the in-memory imagePaths are never persisted. You need to either:
- Write the feature directly with
atomicWriteJsonafter updatingimagePaths, or - Pass
imagePathsthrough theoptionsparameter toexecuteFeatureso it can attach them at execution time.
- // Store image paths on the feature so executeFeature can pick them up
- if (imagePaths && imagePaths.length > 0) {
- feature.imagePaths = imagePaths.map((p) => ({
- path: p,
- filename: p.split('/').pop() || p,
- mimeType: 'image/*',
- }));
- await this.featureStateManager.updateFeatureStatus(
- this.projectPath,
- featureId,
- feature.status || 'in_progress'
- );
- }Depending on how executeFeature / ExecutionService consumes image paths, either pass them via options or persist with a dedicated helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/services/auto-mode/facade.ts` around lines 646 - 657, The
code currently sets feature.imagePaths using p.split('/') which breaks on
Windows and then calls this.featureStateManager.updateFeatureStatus which
re-reads the feature from disk so the mutated in-memory imagePaths are never
persisted and causes unwanted side effects; replace p.split('/').pop() with
path.basename(p) and persist the new imagePaths explicitly instead of calling
updateFeatureStatus: after setting feature.imagePaths call the existing
atomicWriteJson helper (or whichever project-level atomic write utility is used)
to write the feature JSON file for this.projectPath/featureId so the imagePaths
are saved without changing timestamps or emitting status events, or
alternatively pass imagePaths through the executeFeature/options path (e.g., via
the ExecutionService/executeFeature options) if that flow is already supported.
| const initialTasks: TaskInfo[] = (planTasks as ParsedTask[]).map((t: ParsedTask) => ({ | ||
| id: t.id, | ||
| description: t.description, | ||
| filePath: t.filePath, | ||
| phase: t.phase, | ||
| status: | ||
| t.id === currentId | ||
| ? ('in_progress' as const) | ||
| : (t.status as TaskInfo['status']) || ('pending' as const), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where ParsedTask and TaskInfo are defined
echo "=== Searching for ParsedTask definition ==="
rg -n "type ParsedTask|interface ParsedTask" --type=ts -A 15
echo ""
echo "=== Searching for TaskInfo definition ==="
rg -n "type TaskInfo|interface TaskInfo" --type=ts -A 15Repository: AutoMaker-Org/automaker
Length of output: 2348
🏁 Script executed:
#!/bin/bash
# Check line 152 and surrounding context
sed -n '145,162p' apps/ui/src/components/ui/task-progress-panel.tsxRepository: AutoMaker-Org/automaker
Length of output: 792
🏁 Script executed:
#!/bin/bash
# Check the auto_mode_task_status event type definition
echo "=== Searching for auto_mode_task_status in AutoModeEvent ==="
rg -n "auto_mode_task_status" --type=ts -B2 -A5 | head -40
echo ""
echo "=== Full AutoModeEvent definition ==="
rg -n "type AutoModeEvent|interface AutoModeEvent" --type=ts -A 50 | head -80Repository: AutoMaker-Org/automaker
Length of output: 8238
🏁 Script executed:
#!/bin/bash
# Check if there are other places where this same cast occurs
echo "=== Checking for similar unsafe casts ==="
rg -n "as TaskInfo\['status'\]" --type=ts
echo ""
echo "=== Check if 'failed' status is actually used in practice ==="
rg -n "'failed'" --type=ts | grep -i "status\|task"Repository: AutoMaker-Org/automaker
Length of output: 3694
🏁 Script executed:
#!/bin/bash
# Check how the status field is rendered in the component
echo "=== Checking status usage in task rendering ==="
rg -n "status" apps/ui/src/components/ui/task-progress-panel.tsx | head -20
echo ""
echo "=== Check badge/icon rendering based on status ==="
sed -n '175,250p' apps/ui/src/components/ui/task-progress-panel.tsxRepository: AutoMaker-Org/automaker
Length of output: 4182
🏁 Script executed:
#!/bin/bash
# Continue reading the task item rendering
sed -n '250,300p' apps/ui/src/components/ui/task-progress-panel.tsxRepository: AutoMaker-Org/automaker
Length of output: 2530
Fix unsafe status casts that allow 'failed' to corrupt component state.
ParsedTask['status'] includes 'failed' but TaskInfo['status'] does not. Both casts on lines 73 and 152 are unsafe:
- Line 73: The fallback
|| ('pending' as const)only catches falsy values; ift.status === 'failed', it passes through unchecked. - Line 152: No fallback at all—
taskEvent.statusis cast directly.
When 'failed' enters state, the rendering logic (lines 253–255) matches none of the three conditions (in_progress, completed, pending), so the task icon won't render and styling won't apply.
Normalize explicitly at both locations:
Suggested fix
# Line 73
- status:
- t.id === currentId
- ? ('in_progress' as const)
- : (t.status as TaskInfo['status']) || ('pending' as const),
+ status:
+ t.id === currentId
+ ? ('in_progress' as const)
+ : t.status === 'completed'
+ ? ('completed' as const)
+ : t.status === 'in_progress'
+ ? ('in_progress' as const)
+ : ('pending' as const),
# Line 152
- ? { ...t, status: taskEvent.status as TaskInfo['status'] }
+ ? { ...t, status: taskEvent.status === 'completed' ? ('completed' as const) : taskEvent.status === 'in_progress' ? ('in_progress' as const) : ('pending' as const) }Alternatively, extract a normalization function to avoid duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/ui/task-progress-panel.tsx` around lines 65 - 74, The
code is casting ParsedTask.status and taskEvent.status unsafely which allows
'failed' to enter TaskInfo state and break rendering; update the mapping that
builds initialTasks (the map over planTasks producing TaskInfo objects where
status is currently assigned using (t.status as TaskInfo['status']) ||
'pending') and the handler that assigns taskEvent.status (where taskEvent.status
is cast directly) to explicitly normalize statuses to the allowed
TaskInfo['status'] values ('in_progress' | 'completed' | 'pending'): implement a
small helper like normalizeTaskStatus(status: ParsedTask['status'] | string):
TaskInfo['status'] and call it both in the initialTasks creation and when
consuming taskEvent.status (instead of direct casts or relying on falsy
fallbacks) so any unknown or disallowed values (including 'failed') map to
'pending' (or another defined default).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements across the board. It adds robust error handling to the auto-mode facade, implements the followUpFeature, and fixes issues with Claude usage parsing and task status updates on the UI. The changes are well-structured and include corresponding tests. I've identified a couple of areas for improvement: one for consistency in error handling within the facade, and another related to duplicated logic in the feature state manager. Additionally, there's a discrepancy in a new test case that should be addressed to ensure it correctly reflects the implementation's behavior. Overall, this is a solid contribution.
There was a problem hiding this comment.
Similar to the waiting_approval test, this test for the verified status seems to have incorrect assertions. The implementation does not complete pending tasks, but the test expects all tasks, including pending ones, to be marked as completed. Please update the assertions to match the actual implementation logic.
| // All tasks should be completed | |
| expect(savedFeature.planSpec?.tasks?.[0].status).toBe('completed'); | |
| expect(savedFeature.planSpec?.tasks?.[1].status).toBe('completed'); | |
| expect(savedFeature.planSpec?.tasks?.[2].status).toBe('completed'); | |
| // currentTaskId should be cleared | |
| expect(savedFeature.planSpec?.currentTaskId).toBeUndefined(); | |
| // tasksCompleted should equal total tasks | |
| expect(savedFeature.planSpec?.tasksCompleted).toBe(3); | |
| // Only in_progress tasks should be completed | |
| expect(savedFeature.planSpec?.tasks?.[0].status).toBe('completed'); | |
| expect(savedFeature.planSpec?.tasks?.[1].status).toBe('completed'); | |
| expect(savedFeature.planSpec?.tasks?.[2].status).toBe('pending'); | |
| // currentTaskId should be cleared | |
| expect(savedFeature.planSpec?.currentTaskId).toBeUndefined(); | |
| // tasksCompleted should be 2, not 3 | |
| expect(savedFeature.planSpec?.tasksCompleted).toBe(2); |
| // Finalize task statuses when feature is done: | ||
| // - Mark any in_progress tasks as completed (agent finished but didn't explicitly complete them) | ||
| // - Do NOT mark pending tasks as completed (they were never started) | ||
| // - Clear currentTaskId since no task is actively running | ||
| // This prevents cards in "waiting for review" from appearing to still have running tasks | ||
| if (feature.planSpec?.tasks) { | ||
| let tasksFinalized = 0; | ||
| for (const task of feature.planSpec.tasks) { | ||
| if (task.status === 'in_progress') { | ||
| task.status = 'completed'; | ||
| tasksFinalized++; | ||
| } | ||
| } | ||
| if (tasksFinalized > 0) { | ||
| logger.info( | ||
| `[updateFeatureStatus] Finalized ${tasksFinalized} in_progress tasks for feature ${featureId} moving to waiting_approval` | ||
| ); | ||
| } | ||
| // Update tasksCompleted count to reflect actual completed tasks | ||
| feature.planSpec.tasksCompleted = feature.planSpec.tasks.filter( | ||
| (t) => t.status === 'completed' | ||
| ).length; | ||
| feature.planSpec.currentTaskId = undefined; | ||
| } | ||
| } else if (status === 'verified') { | ||
| // Also finalize in_progress tasks when moving directly to verified (skipTests=false) | ||
| // Do NOT mark pending tasks as completed - they were never started | ||
| if (feature.planSpec?.tasks) { | ||
| for (const task of feature.planSpec.tasks) { | ||
| if (task.status === 'in_progress') { | ||
| task.status = 'completed'; | ||
| } | ||
| } | ||
| feature.planSpec.tasksCompleted = feature.planSpec.tasks.filter( | ||
| (t) => t.status === 'completed' | ||
| ).length; | ||
| feature.planSpec.currentTaskId = undefined; | ||
| } |
There was a problem hiding this comment.
The logic for finalizing tasks when a feature's status changes to waiting_approval or verified is nearly identical. To reduce code duplication and improve maintainability, you could extract this logic into a private helper method.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/server/tests/unit/services/feature-state-manager.test.ts`:
- Line 154: Rename the test declaration whose description is 'should finalize
in_progress and pending tasks when moving to waiting_approval' to accurately
describe behavior (e.g., 'should finalize in_progress tasks when moving to
waiting_approval' or 'should finalize in_progress but keep pending tasks when
moving to waiting_approval') so the test name matches the assertions; update the
it(...) string in apps/server/tests/unit/services/feature-state-manager.test.ts
accordingly.
- Around line 191-228: The failing test assumes pending tasks are completed when
moving to 'verified', but production only auto-completes tasks in 'in_progress'
state; update the assertions in the test (the case that calls
manager.updateFeatureStatus and inspects the savedFeature from atomicWriteJson)
to expect that tasks with status 'pending' remain 'pending' (i.e., only assert
tasks[0] and tasks[1] are 'completed' and tasks[2] is still 'pending'), keep the
checks for currentTaskId cleared, tasksCompleted equal the number of completed
tasks, and justFinishedAt cleared as before.
1 participant
…eature. Fix Claude weekly usage indicator. Fix mobile card drag
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements
Tests