Skip to content

Commit b9803d3

Browse files
committed
refactor(cli): simplify use-activity-query.ts (Commit 2.6)
Extract activity query hook into modular pieces: - query-cache.ts: Cache entries, listeners, ref counts, snapshots - query-executor.ts: Query execution with retries, deduplication - query-invalidation.ts: Invalidation strategies, fullDeleteCacheEntry Critical fixes applied: - Fix infinite retry loop: scheduleRetry uses clearRetryTimeout (not clearRetryState) - Fix memory leak: fullDeleteCacheEntry clears all state for GC - Fix incomplete deps: Added refs for refetchOnMount/staleTime/enabled Additional fixes: - Remove redundant bumpGeneration call in fullDeleteCacheEntry - Add isEnabled callback to cancel retries when disabled - Remove dead exports (getInFlightPromise, setInFlightPromise) Reduces use-activity-query.ts from ~480 to ~316 lines (-34%). All 1,911 CLI tests pass. 9 e2e tests pass. Reviewed by 4 CLI agents (Codebuff, Codex, Claude Code, Gemini).
1 parent f5cb055 commit b9803d3

File tree

5 files changed

+639
-331
lines changed

5 files changed

+639
-331
lines changed

REFACTORING_PLAN.md

Lines changed: 63 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ This document outlines a prioritized refactoring plan for the 51 issues identifi
3636
| 2.4 | Consolidate billing duplication | ✅ Complete | Codex CLI |
3737
| 2.5a | Extract multiline keyboard navigation | ✅ Complete | Codebuff |
3838
| 2.5b | Extract multiline editing handlers | ✅ Complete | Codebuff |
39-
| 2.6 | Simplify use-activity-query.ts | ⬜ Not Started | - |
39+
| 2.6 | Simplify use-activity-query.ts | 🔄 In Progress | Codex CLI |
4040
| 2.7 | Consolidate XML parsing | ⬜ Not Started | - |
4141
| 2.8 | Consolidate analytics | ⬜ Not Started | - |
4242
| 2.9 | Refactor doStream | ⬜ Not Started | - |
@@ -557,18 +557,71 @@ After initial commit, 4 CLI agents reviewed and identified warnings. All were fi
557557

558558
---
559559

560-
### Commit 2.6: Simplify `use-activity-query.ts`
560+
### Commit 2.6: Simplify `use-activity-query.ts` 🔄 IN PROGRESS
561561
**Files:** `cli/src/hooks/use-activity-query.ts`
562562
**Est. Time:** 4-5 hours
563-
**Est. LOC Changed:** ~500-600
563+
**Est. LOC Changed:** ~500-600
564+
**Actual LOC Changed:** ~794 insertions (new files), ~480 lines refactored
564565

565-
| Task | Description |
566-
|------|-------------|
567-
| Evaluate external caching library | Consider `react-query` or similar |
568-
| If keeping custom: Extract `QueryCache` class | Cache management |
569-
| Extract `QueryExecutor` | Query execution logic |
570-
| Extract `QueryInvalidation` | Invalidation strategies |
571-
| Simplify main hook | Compose extracted pieces |
566+
| Task | Description | Status |
567+
|------|-------------|--------|
568+
| Evaluate external caching library | Kept custom (react-query overkill for this use case) ||
569+
| Extract `query-cache.ts` module | Cache entries, listeners, ref counts, snapshots ||
570+
| Extract `query-executor.ts` module | Query execution with retries, deduplication ||
571+
| Extract `query-invalidation.ts` module | Invalidation strategies, removeQuery, setQueryData ||
572+
| Simplify main hook | Compose extracted pieces ||
573+
| Fix critical issues from review | See below ||
574+
575+
**New Files Created:**
576+
- `cli/src/utils/query-cache.ts` (~224 lines) - Cache management:
577+
- `CacheEntry`, `KeySnapshot` types
578+
- `serializeQueryKey`, `subscribeToKey`, `getKeySnapshot`
579+
- `setCacheEntry`, `getCacheEntry`, `isEntryStale`
580+
- `setQueryFetching`, `isQueryFetching`
581+
- `incrementRefCount`, `decrementRefCount`, `getRefCount`
582+
- `bumpGeneration`, `getGeneration`, `deleteCacheEntry`
583+
- `resetCache` (for testing)
584+
- `cli/src/utils/query-executor.ts` (~187 lines) - Query execution:
585+
- `createQueryExecutor` - factory for fetch functions with retry/dedup
586+
- `clearRetryState`, `clearRetryTimeout` - retry management
587+
- `scheduleRetry` - exponential backoff scheduling
588+
- `getRetryCount`, `setRetryCount` - retry state
589+
- `resetExecutorState` (for testing)
590+
- `cli/src/utils/query-invalidation.ts` (~67 lines) - Invalidation:
591+
- `invalidateQuery` - mark query as stale
592+
- `removeQuery` - full removal with cleanup
593+
- `getQueryData`, `setQueryData` - direct cache access
594+
- `fullDeleteCacheEntry` - comprehensive cleanup for GC
595+
596+
**Component Size Reduction:**
597+
- `use-activity-query.ts`: ~480 → ~316 lines (-34%)
598+
599+
**Critical Issues Fixed (from 4-agent review):**
600+
601+
| Issue | Problem | Fix Applied |
602+
|-------|---------|-------------|
603+
| **Infinite Retry Loop** | `scheduleRetry` called `clearRetryState` which deleted the retry count that was just set, so retry count never accumulated | Created `clearRetryTimeout()` that only clears the timeout (not count). `scheduleRetry` now uses this. |
604+
| **Memory Leak in deleteCacheEntry** | `deleteCacheEntry` didn't clear in-flight promises or retry state when GC runs | Created `fullDeleteCacheEntry()` in query-invalidation.ts that clears all state. GC effect now uses this. |
605+
| **Incomplete useEffect deps** | Initial fetch effect missing deps (refetchOnMount, staleTime, doFetch) - hidden by eslint-disable | Added `refetchOnMountRef` and `staleTimeRef` refs. Deps are now `[enabled, serializedKey, doFetch]`. |
606+
607+
**Review Findings (from 4 CLI agents):**
608+
- ✅ All 3 critical issues correctly fixed
609+
- ✅ Extraction boundaries well-chosen with clear responsibilities
610+
- ✅ Backwards compatibility maintained via re-exports
611+
- ⚠️ Suggestion: Double bumpGeneration call in fullDeleteCacheEntry (harmless but redundant)
612+
- ⚠️ Suggestion: enabled:false doesn't cancel pending retries (edge case, non-blocking)
613+
- ⚠️ Suggestion: Dead exports (getInFlightPromise, setInFlightPromise) - future API surface
614+
615+
**E2E Test Results (Codebuff Local CLI):**
616+
- ✅ Basic CLI startup and usage display
617+
- ✅ Activity detection and refetch
618+
- ✅ Query caching behavior
619+
- ✅ Error recovery and stability
620+
621+
**Test Results:**
622+
- 1,911 CLI tests pass
623+
- 52 use-activity-query tests pass
624+
- TypeScript compiles cleanly
572625

573626
**Dependencies:** None
574627
**Risk:** Medium

0 commit comments

Comments
 (0)