Fix theme inconsistencies in terminal and git diff view #8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ew, and ClaudeTerminal components
sahithvibudhi
approved these changes
Aug 2, 2025
phuongnd08
added a commit
that referenced
this pull request
Nov 14, 2025
phuongnd08
added a commit
that referenced
this pull request
Nov 15, 2025
…cheduler promises (#89) * fix: Resolve worker teardown timeout by properly handling recursive scheduler promises Fixed a critical issue where Playwright E2E tests would complete successfully but the worker would timeout during teardown after 60 seconds. This was caused by floating promises in the scheduler's recursive chain. Problem: - The scheduleNext() function in ClaudeTerminal.tsx was calling itself recursively on line 211 without returning the promise - Line 219 was calling scheduleNext() without any promise handling - This created untracked promise chains that continued running even after tests completed, preventing the Playwright worker from cleaning up properly - All 45 tests would pass, but the worker teardown would timeout at exactly 60s Solution: - Added explicit Promise<void> return type to scheduleNext function - Changed line 213 to 'return scheduleNext()' to properly chain the promise - Used 'void' operator on line 221 to explicitly mark the initial call as fire-and-forget while maintaining the promise chain internally - This allows the promise chain to be properly tracked and cleaned up when the scheduler is stopped The fix ensures that when stopScheduler() is called (either explicitly or during test cleanup), the entire promise chain can resolve properly, allowing the Playwright worker to teardown without timing out. Evidence from CI logs: - Last test completed: 09:00:35 - Worker timeout: 09:01:35 (exactly 60s later) - Error: "Worker teardown timeout of 60000ms exceeded" This fixes the broken main build at commit 7b1e53b. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: Trigger CI build #2 for verification * chore: Trigger CI build #3 for verification * chore: Trigger CI build #4 for consistency verification * chore: Trigger CI build #5 for consistency verification * chore: Trigger CI build #6 for consistency verification * chore: Trigger CI build #7 for consistency verification * chore: Trigger CI build #8 for consistency verification * fix: Add proper scheduler cleanup on component unmount to prevent worker teardown timeout This fix addresses the intermittent worker teardown timeout issue that occurred in 50% of CI builds (4 out of 8 builds failed). Problem: - Previous fix (commit 8cdbad5) only partially resolved the issue - The scheduler's recursive promise chain continued running after tests completed - No cleanup mechanism existed to terminate pending scheduler promises when the Playwright worker began teardown - This caused a race condition: if worker teardown started while scheduler promises were still pending, it would timeout after 60 seconds Root Cause: - Line 244-245 had a comment saying "No cleanup needed on unmount" - The `void scheduleNext()` call on line 221 created a floating promise chain - When tests complete and workers tear down, pending scheduler promises prevent clean shutdown Solution: - Added useEffect cleanup hook that runs on component unmount - Clears any pending timeouts from scheduler - Sets `isRunning: false` to break the recursive promise chain - This ensures all pending scheduler promises can resolve cleanly Testing: - Previous results: 50% failure rate (4/8 builds failed) - All failures showed: "Worker teardown timeout of 60000ms exceeded" - This fix ensures scheduler stops completely before worker teardown begins Documentation: - Added WORKER_TEARDOWN_ANALYSIS.md with detailed failure analysis - Documents the 50% failure pattern and root cause 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: Trigger CI build #10 for final verification * chore: Trigger CI build #11 for final verification * chore: Trigger CI build #12 for final verification * chore: Trigger CI build #13 for final verification * fix: Remove cleanup hook that broke scheduler persistence The cleanup useEffect hook added in bc7cd75 was overly aggressive and broke the scheduler persistence feature. When switching between projects, the ClaudeTerminal component unmounts, triggering the cleanup hook which stops the scheduler prematurely. The scheduler is designed to persist across project switches using a cache (schedulerStateCache). The cleanup hook interfered with this design. This commit removes the cleanup hook and relies on the original fix from commit 8cdbad5 (return scheduleNext()) to properly manage promise chains. Related test: apps/desktop/e2e/project-switch-scheduler-persist.spec.ts:181 Documentation: CLEANUP_HOOK_FAILURE_ANALYSIS.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: Trigger build 1/5 * chore: Trigger build 2/5 * chore: Trigger build 3/5 * chore: Trigger build 4/5 * chore: Trigger build 5/5 * fix: Use AbortController to cleanup scheduler on unmount without breaking persistence This fix resolves the conflict between two requirements: 1. Scheduler must persist when switching between projects (cache-based) 2. Pending promises must be cancelled during Playwright worker teardown Previous attempts: - Commit 8cdbad5: 50% failure rate (no cleanup) - Commit bc7cd75: 100% failure with scheduler persistence broken - Commit ddbf2b5: 40% failure rate (worker teardown timeout returned) New approach using AbortController: - Add AbortController ref that persists across renders - Check signal.aborted in scheduleNext before continuing - Cleanup useEffect with EMPTY dependency array - Only runs on true component unmount, not on every render - Allows scheduler to persist during project switches - Cleanly cancels pending promises during worker teardown Related documentation: ROUND3_FAILURE_ANALYSIS.md Related test: apps/desktop/e2e/project-switch-scheduler-persist.spec.ts:181 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: Trigger build 1/5 (AbortController fix) * chore: Trigger build 2/5 (AbortController fix) * chore: Trigger build 3/5 (AbortController fix) * chore: Trigger build 4/5 (AbortController fix) * chore: Trigger build 5/5 (AbortController fix) * fix: Reset AbortController on mount and use window unload for cleanup The previous AbortController implementation had a critical flaw: the signal remained aborted after component unmount, breaking scheduler persistence when the component remounted during project switches. Root cause: React refs persist across unmount/remount cycles. When switching back to a project, the same component instance remounts with the SAME aborted AbortController, causing the scheduler to immediately exit. New approach: 1. Reset AbortController on each mount (fresh signal for each mount cycle) 2. Use window 'beforeunload' event for cleanup instead of useEffect cleanup 3. beforeunload only fires on actual window close/Playwright teardown 4. Does NOT fire during normal component unmount (project switches) This fixes both issues: - ✅ Scheduler persists during project switches (fresh AbortController on mount) - ✅ Pending promises cancelled during worker teardown (beforeunload event) Related documentation: ROUND4_ABORTCONTROLLER_FAILURE.md Related test: apps/desktop/e2e/project-switch-scheduler-persist.spec.ts:181 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: Trigger build 1/5 (AbortController reset fix) * chore: Trigger build 2/5 (AbortController reset fix) * chore: Trigger build 3/5 (AbortController reset fix) * chore: Trigger build 4/5 (AbortController reset fix) * chore: Trigger build 5/5 (AbortController reset fix) * docs: Add final success summary for worker teardown fix * chore: Remove documentation and monitoring files from PR --------- Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix theme inconsistencies in terminal and git diff view
#7