refactor: migrate to canonical shared hooks (Phase 09)#834
refactor: migrate to canonical shared hooks (Phase 09)#834pedramamini merged 1 commit intoRunMaestro:rcfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaced manual Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Thanks for the contribution, @jSydorowicz21! This is a clean, well-scoped refactor. The 4 No merge conflicts, net -19 lines, LGTM. Approving. |
Greptile SummaryThis PR migrates 4 raw Confidence Score: 5/5Safe to merge — all 4 migrations are behavior-preserving and the exclusion rationale is sound. All changed call sites correctly target window events, the canonical hook's handlerRef pattern eliminates stale-closure risk, and every intentionally-skipped site (visibilitychange on document, conditional listeners, useFocusAfterRender re-render semantics) is correctly explained. No P0 or P1 findings. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Component Mounts] --> B[useEventListener called]
B --> C[handlerRef.current = handler]
C --> D[useEffect runs\neventType dep]
D --> E[window.addEventListener\neventType, listener]
E --> F{Event Fires?}
F -->|yes| G[listener invokes\nhandlerRef.current]
G --> H[Handler executes\nwith latest closure]
F -->|re-render| I[handlerRef.current updated\nto latest handler]
I --> F
A --> J[Component Unmounts]
J --> K[useEffect cleanup runs]
K --> L[window.removeEventListener\neventType, listener]
subgraph Migration Sites
M[tour:action — useTourActions.ts]
N[tour:action — SessionList.tsx]
O[beforeunload — useHandsOnTimeTracker.ts]
P[keydown — PlaybookDetailView]
end
subgraph Intentionally NOT Migrated
Q[visibilitychange — document target]
R[keydown w/ condition — SessionList Escape]
S[mousedown — conditional dropdown]
end
Reviews (1): Last reviewed commit: "refactor: migrate to canonical shared ho..." | Re-trigger Greptile |
jSydorowicz21
added
refactor
Sub-phase 09A: useEventListener migrations
- useTourActions: replace useEffect + window.addEventListener('tour:action')
- SessionList: replace useEffect + window.addEventListener('tour:action')
- useHandsOnTimeTracker: replace useEffect + window.addEventListener('beforeunload')
- MarketplaceModal (PlaybookDetailView): replace useEffect + window.addEventListener('keydown')
Sub-phase 09B: no migrations
- useActiveSession has semantic drift (falls back to first session vs inline
`?? null`), unsafe for existing `sessions.find()` call sites
- Hand-rolled debounce sites (useAutoRunUndo, useSessionDebounce, useTabHoverOverlay,
GitStatusWidget) are per-session keyed, dep-cleanup-scoped, or shared-timer
patterns that useDebouncedCallback does not express
- Most setTimeout + focus patterns are imperative (inside click/key handlers),
which useFocusAfterRender (useLayoutEffect + re-runs every render) does not fit
Skipped in 09A
- keydown listeners using capture: true (DirectorNotesModal, SettingsSearch,
NewInstanceModal, EditAgentModal, PhaseReviewScreen, UsageDashboardModal)
- hook does not support listener options
- listeners on document or with passive: true (modal visibilitychange handler,
activityBus, scroll tracker) - hook is window-only
- listeners conditionally registered on isOpen - hook is unconditional
- useRemoteHandlers: async handler + 380-line body + closure capture, too risky
- useMobileLandscape: pairs addEventListener with initial imperative call
jSydorowicz21
force-pushed
the
dedup/phase-09-shared-hooks
branch
from
0548426 to
077a44b
Compare
jSydorowicz21
added
refactor
2 participants
Summary
Migrates a conservative subset of hook patterns to canonical shared hooks. This phase is intentionally narrow - most call sites had semantic subtleties that would have caused regressions if blindly replaced.
Net: -19 lines across 4 files
09A - useEventListener migrations (4 sites)
Replaced raw
addEventListener/removeEventListenerpairs with the canonicaluseEventListenerhook fromsrc/renderer/hooks/utils/useEventListener.ts:useTourActions.ts-tour:actioneventSessionList.tsx-tour:actioneventuseHandsOnTimeTracker.ts-beforeunloadeventMarketplaceModal.tsx-keydownevent in PlaybookDetailView09A - useFocusAfterRender migrations (0 sites)
Intentionally zero - the canonical hook uses
useLayoutEffectwith NO dep array and re-runs every render while itsconditionis true. Replacing the commonuseEffect(() => { setTimeout(() => ref.current?.focus(), 50); }, [])pattern would cause the focus to be re-stolen on every subsequent render, breaking the app.09B - (0 sites migrated)
useActiveSession()semantic gap:selectActiveSessionfalls back tostate.sessions[0]when there's no active session; inlinesessions.findreturnsundefined. Blindly migrating would surface stale-session data instead of the "no active session" branch.Debounce/throttle misfits: Every hand-rolled debounce/throttle has a reason the canonical hook doesn't fit.
Supersedes #823 (accidentally closed during rebase).
Test plan
npm run lintpasses (all 3 tsconfigs)npx prettier --check .passesRisk
Very low. 4 behavior-preserving migrations, no component changes.
Summary by CodeRabbit