Skip to content

Commit 7e9d93a

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 7e9d93a

File tree

5 files changed

+553
-342
lines changed

5 files changed

+553
-342
lines changed

REFACTORING_PLAN.md

Lines changed: 72 additions & 11 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 | ✅ Complete | Codebuff |
4040
| 2.7 | Consolidate XML parsing | ⬜ Not Started | - |
4141
| 2.8 | Consolidate analytics | ⬜ Not Started | - |
4242
| 2.9 | Refactor doStream | ⬜ Not Started | - |
@@ -557,22 +557,83 @@ 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` ✅ COMPLETE
561561
**Files:** `cli/src/hooks/use-activity-query.ts`
562562
**Est. Time:** 4-5 hours
563-
**Est. LOC Changed:** ~500-600
563+
**Actual Time:** ~3 hours
564+
**Est. LOC Changed:** ~500-600
565+
**Actual LOC Changed:** 716 lines total (326 hook + 193 cache + 149 executor + 48 invalidation)
564566

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 |
567+
| Task | Description | Status |
568+
|------|-------------|--------|
569+
| Evaluate external caching library | Kept custom (react-query overkill for this use case) ||
570+
| Extract `query-cache.ts` module | Cache entries, listeners, ref counts, snapshots ||
571+
| Extract `query-executor.ts` module | Query execution with retries, deduplication ||
572+
| Extract `query-invalidation.ts` module | Invalidation strategies, removeQuery, setQueryData ||
573+
| Simplify main hook | Compose extracted pieces ||
574+
| Fix critical issues from review | See below ||
575+
| Multi-agent review fixes | 4 CLI agents reviewed, 5 issues fixed ||
576+
577+
**New Files Created:**
578+
- `cli/src/utils/query-cache.ts` (~224 lines) - Cache management:
579+
- `CacheEntry`, `KeySnapshot` types
580+
- `serializeQueryKey`, `subscribeToKey`, `getKeySnapshot`
581+
- `setCacheEntry`, `getCacheEntry`, `isEntryStale`
582+
- `setQueryFetching`, `isQueryFetching`
583+
- `incrementRefCount`, `decrementRefCount`, `getRefCount`
584+
- `bumpGeneration`, `getGeneration`, `deleteCacheEntry`
585+
- `resetCache` (for testing)
586+
- `cli/src/utils/query-executor.ts` (~187 lines) - Query execution:
587+
- `createQueryExecutor` - factory for fetch functions with retry/dedup
588+
- `clearRetryState`, `clearRetryTimeout` - retry management
589+
- `scheduleRetry` - exponential backoff scheduling
590+
- `getRetryCount`, `setRetryCount` - retry state
591+
- `resetExecutorState` (for testing)
592+
- `cli/src/utils/query-invalidation.ts` (~67 lines) - Invalidation:
593+
- `invalidateQuery` - mark query as stale
594+
- `removeQuery` - full removal with cleanup
595+
- `getQueryData`, `setQueryData` - direct cache access
596+
- `fullDeleteCacheEntry` - comprehensive cleanup for GC
597+
598+
**Component Size Reduction:**
599+
- `use-activity-query.ts`: ~480 → ~316 lines (-34%)
600+
601+
**Critical Issues Fixed (from 4-agent review):**
602+
603+
| Issue | Problem | Fix Applied |
604+
|-------|---------|-------------|
605+
| **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. |
606+
| **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. |
607+
| **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]`. |
608+
609+
**Review Findings (from 4 CLI agents):**
610+
- ✅ All 3 critical issues correctly fixed
611+
- ✅ Extraction boundaries well-chosen with clear responsibilities
612+
- ✅ Backwards compatibility maintained via re-exports
613+
- ⚠️ Suggestion: Double bumpGeneration call in fullDeleteCacheEntry (harmless but redundant)
614+
- ⚠️ Suggestion: enabled:false doesn't cancel pending retries (edge case, non-blocking)
615+
- ⚠️ Suggestion: Dead exports (getInFlightPromise, setInFlightPromise) - future API surface
616+
617+
**Multi-Agent Review (Codex, Codebuff, Claude Code, Gemini):**
618+
619+
| Issue | Problem | Fix Applied |
620+
|-------|---------|-------------|
621+
| **Redundant setRetryCount** | `refetch()` called `setRetryCount(0)` then `clearRetryState()` which already deletes count | Removed redundant `setRetryCount` call |
622+
| **Two delete functions** | `deleteCacheEntry` incomplete vs `fullDeleteCacheEntry` complete - footgun | Renamed to `deleteCacheEntryCore` (internal), kept `fullDeleteCacheEntry` as public API |
623+
| **Memory leak in generations** | `generations` map never cleaned up during normal deletion | Added `clearGeneration(key)` call in `fullDeleteCacheEntry` |
624+
| **gcTimeouts exported mutable** | Map exported directly allowing any module to mutate | Replaced with accessor functions (`setGcTimeout`, `clearGcTimeout`) |
625+
| **GC effect deps issue** | `gcTime` in deps caused spurious cleanup runs on option change | Stored `gcTime` in ref, removed from deps |
626+
| **AI slop comments** | Verbose JSDoc that just repeated function names | Removed ~60 lines of obvious comments |
627+
628+
**Test Results:**
629+
- 52 use-activity-query tests pass
630+
- 59 dependent tests (use-usage-query, use-claude-quota-query) pass
631+
- TypeScript compiles cleanly
572632

573633
**Dependencies:** None
574634
**Risk:** Medium
575-
**Rollback:** Revert single commit
635+
**Rollback:** Revert single commit
636+
**Commit:** Pending
576637

577638
---
578639

0 commit comments

Comments
 (0)