fix: exclude error-paused sessions from Auto Run time tracking#731
fix: exclude error-paused sessions from Auto Run time tracking#731pedramamini wants to merge 3 commits intomainfrom
Conversation
- Standardized multi-phase auto-run docs into one flat, dated subdirectory 📁 - Explicitly banned nested project/feature folder structures for phase outputs 🚫 - Improved guidance for clean lexicographic sorting with zero-padded phases 🔢 - Made it easier to add entire effort folders to auto-run at once ➕ - Clarified organization rules so related phase documents stay tightly grouped 🧭
…cking (#242) Error-paused Auto Run sessions (errorPaused: true) were still counted in activeBatchSessionIds, causing achievement time to accumulate even while the session was paused waiting for error resolution. This could result in inflated total time and premature badge unlocks. Filter out error-paused sessions in useAutoRunAchievements before using them for time delta calculation, while preserving their inclusion in activeBatchSessionIds for UI purposes (sidebar indicators, peak usage stats).
📝 WalkthroughWalkthroughThis pull request fixes incorrect time accumulation during auto-run sessions that encounter errors. It modifies the achievement tracking hook to exclude Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR correctly fixes Auto Run achievement time accumulation during error states by filtering
Confidence Score: 4/5Safe to merge — production logic is correct; the only issue is a test that doesn't fully verify the resume-from-error path Score of 4 reflects that the core hook change is well-implemented and the batchStore correctly issues new object references in production, ensuring useMemo recomputes. One point deducted because the 'resumes counting' test uses in-place mutation and does not actually validate the intended behavior, leaving a gap in test coverage for the resume path. src/tests/renderer/hooks/useAutoRunAchievements.test.ts — the 'resumes counting' test needs Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[activeBatchSessionIds
batchRunStates] --> B[useMemo
filter errorPaused]
B --> C{activeNonPausedSessionIds
.length === 0?}
C -- Yes --> D[reset lastUpdateTime = 0
no interval]
C -- No --> E[setInterval 60s]
E --> F[deltaMs = elapsedMs
× non-paused count]
F --> G[updateAutoRunProgress
deltaMs]
G --> H{newBadgeLevel
!== null?}
H -- Yes --> I[setStandingOvationData
badge]
H -- No --> J[wait next tick]
A2[activeBatchSessionIds
sessions] --> K[Peak Stats Effect
updateUsageStats]
style D fill:#ffd,stroke:#cc0
style I fill:#dfd,stroke:#080
Reviews (1): Last reviewed commit: "style: format docs/releases.md with pret..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('resumes counting when error is resolved', () => { | ||
| // Start with error-paused session | ||
| mockBatchRunStates['session-1'] = { isRunning: true, errorPaused: true }; | ||
|
|
||
| const { rerender } = renderHook( | ||
| ({ ids }) => useAutoRunAchievements({ activeBatchSessionIds: ids }), | ||
| { initialProps: { ids: ['session-1'] } } | ||
| ); | ||
|
|
||
| act(() => { | ||
| vi.advanceTimersByTime(60000); | ||
| }); | ||
| expect(mockUpdateAutoRunProgress).not.toHaveBeenCalled(); | ||
|
|
||
| // Resolve error | ||
| mockBatchRunStates['session-1'] = { isRunning: true, errorPaused: false }; | ||
| rerender({ ids: ['session-1'] }); | ||
|
|
||
| act(() => { | ||
| vi.advanceTimersByTime(60000); | ||
| }); | ||
| expect(mockUpdateAutoRunProgress).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
useMemo
The test mutates mockBatchRunStates in-place at line 828:
mockBatchRunStates['session-1'] = { isRunning: true, errorPaused: false };
rerender({ ids: ['session-1'] });mockBatchRunStates is declared as const (line 60), so the useBatchStore mock always delivers the same object reference to its selector. When a property is reassigned, the container reference stays identical.
React's useMemo compares deps with Object.is. Since batchRunStates (= mockBatchRunStates) is the same reference before and after the mutation, useMemo returns the cached [], the effect's dep (activeNonPausedSessionIds.length === 0) never changes, no interval is ever created, and mockUpdateAutoRunProgress is never called — making the toHaveBeenCalledTimes(1) assertion at line 834 a false positive (or vacuous pass).
The fix is to switch mockBatchRunStates to let so the entire reference can be replaced, which useMemo will detect:
// Line 60: change const → let
let mockBatchRunStates: Record<string, any> = {};
// In beforeEach — reassign instead of key-deleting:
mockBatchRunStates = {};
// In the test — replace the full reference:
mockBatchRunStates = { 'session-1': { isRunning: true, errorPaused: false } };
rerender({ ids: ['session-1'] });Note: the beforeEach reset at lines 138–139 (Object.keys(mockBatchRunStates).forEach(key => delete ...)) must also become mockBatchRunStates = {} for consistency once let is used.
| return () => { | ||
| clearInterval(intervalId); | ||
| }; | ||
| }, [activeBatchSessionIds.length]); | ||
| }, [activeNonPausedSessionIds.length]); |
There was a problem hiding this comment.
.length-only dep needs a comment to prevent future regressions
The effect body accesses activeNonPausedSessionIds at lines 59 and 78, but the dependency array only lists activeNonPausedSessionIds.length. This is deliberate — using the full array reference would restart the interval (and reset lastUpdateTime) whenever session composition changes even if the count stays the same, introducing timer drift. However:
- It would trigger
react-hooks/exhaustive-depswithout explanation. - A future developer could "fix" it by adding the full array, reintroducing the very bug this PR avoids.
Add a comment documenting the intent:
| return () => { | |
| clearInterval(intervalId); | |
| }; | |
| }, [activeBatchSessionIds.length]); | |
| }, [activeNonPausedSessionIds.length]); | |
| return () => { | |
| clearInterval(intervalId); | |
| }; | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| // Intentionally depend on .length only: the array reference changes on every | |
| // composition change (even same-count), which would restart the interval and | |
| // reset lastUpdateTime unnecessarily. Only the count affects the deltaMs multiplier. | |
| }, [activeNonPausedSessionIds.length]); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/releases.md (1)
174-174: Optional: Correct "Github" capitalization.Line 174 uses "Github" but the official spelling is "GitHub" (capital H). The static analysis tool correctly flagged this.
📝 Proposed fix
-🌳 Github Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`. +🌳 GitHub Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 174, Update the release-note sentence that currently reads "🌳 Github Worktree support was added..." to use the correct capitalization "GitHub"; locate the exact string in the releases entry (the sentence starting with the tree emoji) and replace "Github" with "GitHub" so the line becomes "🌳 GitHub Worktree support was added..." ensuring consistency with official branding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/releases.md`:
- Line 174: Update the release-note sentence that currently reads "🌳 Github
Worktree support was added..." to use the correct capitalization "GitHub";
locate the exact string in the releases entry (the sentence starting with the
tree emoji) and replace "Github" with "GitHub" so the line becomes "🌳 GitHub
Worktree support was added..." ensuring consistency with official branding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0f23a68-eeb5-419d-8c17-dbf1f653eb8a
📒 Files selected for processing (4)
docs/releases.mdsrc/__tests__/renderer/hooks/useAutoRunAchievements.test.tssrc/prompts/maestro-system-prompt.mdsrc/renderer/hooks/batch/useAutoRunAchievements.ts
1 participant
Summary
errorPaused: true) were still counted inactiveBatchSessionIds, causing time to accumulate even while sessions were paused waiting for error resolutionuseAutoRunAchievementsbefore computing time deltas, while preserving their inclusion inactiveBatchSessionIdsfor UI purposes (sidebar indicators, peak usage stats)Closes #242
Test plan
useAutoRunAchievementstests pass (38 existing + 3 new)npm run lint)Summary by CodeRabbit
Release Notes
Improvements
Documentation