refactor: extracted 3 more hooks from App.tsx#412
Merged
Conversation
Move session restoration, migration, corruption recovery, background git info fetching, and session/group loading into a dedicated hook. App.tsx reduced from 9,556 to 9,191 lines (−365).
Move input keyboard handling (tab completion, @ mentions, slash commands, enter-to-send, command history, escape) into a dedicated hook. App.tsx reduced from 9,191 to 9,018 lines (−173).
Move 14 startup effects into a dedicated hook: splash screen coordination, GitHub CLI check, Windows warning, gist URLs, beta updates sync, update check, leaderboard sync, SpecKit/OpenSpec loading, SSH configs, stats DB check, notification settings sync, and playground debug. The hook reads directly from Zustand stores, eliminating the handleSyncAutoRunStats dependency by inlining the store update. App.tsx reduced by ~253 lines (9,018 → 8,765). 43 new tests added (20,118 total, all passing).
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughExtracts startup and input logic from App.tsx into three new public hooks (useAppInitialization, useSessionRestoration, useInputKeyDown) and adds extensive unit tests validating initialization, session restoration, and input key handling across many success and failure paths. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant InitHook as useAppInitialization
participant SessionHook as useSessionRestoration
participant InputHook as useInputKeyDown
participant Settings as SettingsStore
participant Maestro as window.maestro
App->>SessionHook: mount -> call useSessionRestoration()
App->>InitHook: mount -> call useAppInitialization()
App->>InputHook: mount -> call useInputKeyDown()
InitHook->>Settings: read settings / check loaded state
SessionHook->>Settings: load sessions/groups -> restoreSession(...)
InitHook->>Maestro: request gh cli status, ssh configs, commands, updates
Maestro-->>InitHook: respond with status, configs, commands, update info
InitHook->>SessionHook: wait for initialLoadComplete -> coordinate splash hide
InputHook->>App: expose handleInputKeyDown -> App attaches to input component
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 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)
Comment |
Greptile SummaryContinues App.tsx decomposition by extracting three custom hooks (Phases 2E-2G), successfully removing ~830 lines while maintaining full functionality. Key Changes
Design Excellence
Impact
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App.tsx ~9.5k lines] -->|Phase 2E| B[useSessionRestoration]
A -->|Phase 2F| C[useInputKeyDown]
A -->|Phase 2G| D[useAppInitialization]
B -->|405 lines| E[Session Loading & Migration]
B -->|49 tests| F[Corruption Recovery]
B -->|Background| G[Git Info Fetching]
C -->|319 lines| H[Tab Completion Navigation]
C -->|47 tests| I[Mention Autocomplete]
C -->|Mode-aware| J[Slash Commands & Enter-to-Send]
D -->|291 lines| K[Splash Screen Coordination]
D -->|43 tests| L[Platform Checks & CLI Detection]
D -->|Startup| M[Commands & Stats Loading]
E --> N[App.tsx ~8.7k lines]
F --> N
G --> N
H --> N
I --> N
J --> N
K --> N
L --> N
M --> N
N -->|Result| O[830 lines removed]
N -->|Result| P[139 new unit tests]
N -->|Result| Q[Zero-parameter hooks with Zustand]
style A fill:#ff6b6b
style N fill:#51cf66
style O fill:#51cf66
style P fill:#51cf66
style Q fill:#51cf66
Last reviewed commit: 9cca90c |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/renderer/hooks/ui/useAppInitialization.ts (2)
226-241: Silent error swallowing may hide operational issues.The
.catch(console.error)pattern on lines 240 and 258 logs errors but doesn't propagate them to Sentry for tracking. Per coding guidelines, unexpected errors should bubble up to Sentry. Consider whether SSH config or stats DB failures warrant error tracking.If these are expected/recoverable failures that shouldn't alert:
✏️ Optional: Add explicit comment documenting why errors are swallowed
// --- SSH remote configs loading --- useEffect(() => { window.maestro?.sshRemote ?.getConfigs() .then((result) => { if (result.success && result.configs) { setSshRemoteConfigs( result.configs.map((c: { id: string; name: string }) => ({ id: c.id, name: c.name, })) ); } }) - .catch(console.error); + // Expected failure if SSH is not configured - non-critical for app startup + .catch((error) => console.debug('[SSH] Config loading failed (non-critical):', error)); }, []);As per coding guidelines: "Do NOT silently swallow exceptions with try-catch-console.error blocks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/ui/useAppInitialization.ts` around lines 226 - 241, The current useEffect calling window.maestro?.sshRemote?.getConfigs swallows errors via .catch(console.error); change this to report unexpected errors to Sentry (or your app error-reporting pipeline) instead of only console.error, e.g., catch the error, call the Sentry report function and then either rethrow or explicitly document why it can be ignored; update the promise chain around useEffect -> window.maestro?.sshRemote?.getConfigs() and the setSshRemoteConfigs handling to ensure errors are sent to Sentry (or rethrown) and add an inline comment if you intentionally want to ignore recoverable failures.
118-128: Silent catch hides file gist URL loading failures.The empty
.catch(() => {})on line 127 provides no visibility into failures. At minimum, add debug logging to aid troubleshooting.✏️ Suggested fix
useEffect(() => { window.maestro.settings .get('fileGistUrls') .then((savedUrls) => { if (savedUrls && typeof savedUrls === 'object') { useTabStore.getState().setFileGistUrls(savedUrls as Record<string, GistInfo>); } }) - .catch(() => {}); + .catch((error) => { + console.debug('[App] Failed to load file gist URLs (non-critical):', error); + }); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/ui/useAppInitialization.ts` around lines 118 - 128, The empty catch on the window.maestro.settings.get('fileGistUrls') promise hides failures; update the catch in the useEffect inside useAppInitialization (where useTabStore.getState().setFileGistUrls is called) to log the caught error (using the app logger or console.debug/console.error) with context like "failed to load fileGistUrls" so failures are visible for debugging while preserving existing behavior.src/renderer/hooks/session/useSessionRestoration.ts (2)
230-236: Dead code path:aiSuccessis alwaystrue.The
aiSpawnResultandaiSuccessare hardcoded constants, making theelsebranch at lines 303-313 unreachable. If this is intentional (deferred spawn), consider removing the dead branch or adding a comment explaining future intent.- const aiSpawnResult = { pid: 0, success: true }; - const aiSuccess = true; - - if (aiSuccess) { + // Don't eagerly spawn AI processes on session restore: + // - Batch mode agents spawn per message in useInputProcessing + // - Terminal uses runCommand (fresh shells per command) + // aiPid stays at 0 until user sends their first message + const aiSpawnResult = { pid: 0, success: true }; + { // ... existing code ... - } else { - console.error(`Failed to restore session ${session.id}`); - return { - ...session, - aiPid: -1, - terminalPid: 0, - state: 'error' as SessionState, - isLive: false, - liveUrl: undefined, - }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useSessionRestoration.ts` around lines 230 - 236, The variables aiSpawnResult and aiSuccess in useSessionRestoration.ts are hardcoded (aiSpawnResult = { pid: 0, success: true } and aiSuccess = true) which makes the downstream else branch unreachable; either remove the dead else branch or derive aiSuccess from aiSpawnResult (e.g., set aiSuccess = aiSpawnResult.success) and ensure any spawn logic updates aiSpawnResult when you later implement deferred spawning. Update the code around the aiSpawnResult/aiSuccess usage (referencing aiSpawnResult and aiSuccess in useSessionRestoration) accordingly and add a short comment explaining the chosen approach (deferred spawn vs. immediate spawn result).
46-51: Consider documenting the stable reference pattern.The
useMemo(() => useSessionStore.getState(), [])pattern extracts stable action references once. This is correct but non-obvious. A brief comment would help future maintainers understand why these aren't inuseCallbackdependency arrays.✏️ Suggested documentation
// --- Store actions (stable, non-reactive) --- + // These actions are extracted once and never change, so they don't need + // to be listed in useCallback dependencies throughout this hook. const { setSessions, setGroups, setActiveSessionId, setSessionsLoaded } = useMemo( () => useSessionStore.getState(), [] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useSessionRestoration.ts` around lines 46 - 51, Add a short explanatory comment above the useMemo calls in useSessionRestoration.ts clarifying that useMemo(() => useSessionStore.getState(), []) and useMemo(() => useGroupChatStore.getState(), []) are used to extract stable, non-reactive action references (setSessions, setGroups, setActiveSessionId, setSessionsLoaded, setGroupChats) once so they can be used safely outside React dependency arrays; mention that these are intentionally not wrapped in useCallback or included in effect dependencies because they are stable references returned by the store and will not change.src/__tests__/renderer/hooks/useInputKeyDown.test.ts (1)
161-172: Add missing assertion for Ctrl+F test consistency.The Ctrl+F test only verifies
preventDefaultwas called, but doesn't verifyoutputSearchOpenstate like the Cmd+F test does. For consistency and complete coverage, add the same assertion.✏️ Suggested addition
it('opens output search on Ctrl+F', () => { const deps = createMockDeps(); const { result } = renderHook(() => useInputKeyDown(deps)); const e = createKeyEvent('f', { ctrlKey: true }); act(() => { result.current.handleInputKeyDown(e); }); expect(e.preventDefault).toHaveBeenCalled(); + expect(useUIStore.getState().outputSearchOpen).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useInputKeyDown.test.ts` around lines 161 - 172, The Ctrl+F test "opens output search on Ctrl+F" is missing the assertion that the output search state was toggled; update the test in useInputKeyDown.test.ts to, after calling result.current.handleInputKeyDown(e) and asserting e.preventDefault, also assert that the mock dependency state reflects the open search (e.g., verify createMockDeps()'s state.outputSearchOpen or the appropriate setter was called) — mirror the same assertion used in the Cmd+F test so the test verifies both preventDefault and that useInputKeyDown actually opened the output search.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/renderer/hooks/useInputKeyDown.test.ts`:
- Around line 161-172: The Ctrl+F test "opens output search on Ctrl+F" is
missing the assertion that the output search state was toggled; update the test
in useInputKeyDown.test.ts to, after calling
result.current.handleInputKeyDown(e) and asserting e.preventDefault, also assert
that the mock dependency state reflects the open search (e.g., verify
createMockDeps()'s state.outputSearchOpen or the appropriate setter was called)
— mirror the same assertion used in the Cmd+F test so the test verifies both
preventDefault and that useInputKeyDown actually opened the output search.
In `@src/renderer/hooks/session/useSessionRestoration.ts`:
- Around line 230-236: The variables aiSpawnResult and aiSuccess in
useSessionRestoration.ts are hardcoded (aiSpawnResult = { pid: 0, success: true
} and aiSuccess = true) which makes the downstream else branch unreachable;
either remove the dead else branch or derive aiSuccess from aiSpawnResult (e.g.,
set aiSuccess = aiSpawnResult.success) and ensure any spawn logic updates
aiSpawnResult when you later implement deferred spawning. Update the code around
the aiSpawnResult/aiSuccess usage (referencing aiSpawnResult and aiSuccess in
useSessionRestoration) accordingly and add a short comment explaining the chosen
approach (deferred spawn vs. immediate spawn result).
- Around line 46-51: Add a short explanatory comment above the useMemo calls in
useSessionRestoration.ts clarifying that useMemo(() =>
useSessionStore.getState(), []) and useMemo(() => useGroupChatStore.getState(),
[]) are used to extract stable, non-reactive action references (setSessions,
setGroups, setActiveSessionId, setSessionsLoaded, setGroupChats) once so they
can be used safely outside React dependency arrays; mention that these are
intentionally not wrapped in useCallback or included in effect dependencies
because they are stable references returned by the store and will not change.
In `@src/renderer/hooks/ui/useAppInitialization.ts`:
- Around line 226-241: The current useEffect calling
window.maestro?.sshRemote?.getConfigs swallows errors via .catch(console.error);
change this to report unexpected errors to Sentry (or your app error-reporting
pipeline) instead of only console.error, e.g., catch the error, call the Sentry
report function and then either rethrow or explicitly document why it can be
ignored; update the promise chain around useEffect ->
window.maestro?.sshRemote?.getConfigs() and the setSshRemoteConfigs handling to
ensure errors are sent to Sentry (or rethrown) and add an inline comment if you
intentionally want to ignore recoverable failures.
- Around line 118-128: The empty catch on the
window.maestro.settings.get('fileGistUrls') promise hides failures; update the
catch in the useEffect inside useAppInitialization (where
useTabStore.getState().setFileGistUrls is called) to log the caught error (using
the app logger or console.debug/console.error) with context like "failed to load
fileGistUrls" so failures are visible for debugging while preserving existing
behavior.
- Add missing outputSearchOpen assertion to Ctrl+F test (useInputKeyDown) - Remove dead aiSpawnResult/aiSuccess variables and unreachable else branch (useSessionRestoration) - Expand comment explaining useMemo store action extraction pattern (useSessionRestoration) - Upgrade SSH config error handling from bare console.error to console.warn with context (useAppInitialization) - Replace silent catch on fileGistUrls load with console.debug for debuggability (useAppInitialization)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/renderer/hooks/useInputKeyDown.test.ts`:
- Around line 111-137: The beforeEach reset misses the UI flag outputSearchOpen,
causing cross-test leakage; update the beforeEach to set outputSearchOpen: false
on the mocked InputContext (add outputSearchOpen: false to the
Object.assign(mockInputContext, {...}) block) and/or explicitly reset the UI
store by calling useUIStore.setState({ outputSearchOpen: false }) so tests that
toggle Cmd+F don’t leak state into subsequent tests.
In `@src/renderer/hooks/session/useSessionRestoration.ts`:
- Around line 83-118: The catch blocks in fetchGitInfoInBackground (and the
similar blocks around the other catches) currently only console.warn errors and
swallow them; update these catches to distinguish expected/recoverable errors by
checking a specific error code or type (e.g., error.code or instanceof a known
error) and handle those by setting sshConnectionFailed via setSessions as now,
but for unexpected failures call captureException (or captureMessage) from the
sentry utility and then rethrow (or at minimum capture and rethrow) to avoid
swallowing; reference the gitService methods (isRepo, getBranches, getTags), the
state updater setSessions, and import captureException/captureMessage from your
sentry helper and invoke it inside the catch before rethrowing or marking the
session failure.
In `@src/renderer/hooks/ui/useAppInitialization.ts`:
- Around line 83-93: Several useEffect catch blocks (e.g., the GitHub CLI
availability check that calls window.maestro.git.checkGhCli()) currently swallow
errors; replace those console-only catches with Sentry reporting and targeted
handling: import captureException/captureMessage from src/utils/sentry and in
each catch call captureException(error, { extra: { context: 'checkGhCli' } }) or
captureMessage with a descriptive string, include relevant local state/context
(function name like checkGhCli, checkGhAuth, or the affected setter such as
setGhCliAvailable) and known error codes should be handled explicitly (map known
codes to user-friendly behavior) while rethrowing or allowing unexpected errors
to bubble after reporting.
Comment on lines
+83
to
+118
| try { | ||
| const isGitRepo = await gitService.isRepo(cwd, sshRemoteId); | ||
|
|
||
| let gitBranches: string[] | undefined; | ||
| let gitTags: string[] | undefined; | ||
| let gitRefsCacheTime: number | undefined; | ||
| if (isGitRepo) { | ||
| [gitBranches, gitTags] = await Promise.all([ | ||
| gitService.getBranches(cwd, sshRemoteId), | ||
| gitService.getTags(cwd, sshRemoteId), | ||
| ]); | ||
| gitRefsCacheTime = Date.now(); | ||
| } | ||
|
|
||
| setSessions((prev) => | ||
| prev.map((s) => | ||
| s.id === sessionId | ||
| ? { | ||
| ...s, | ||
| isGitRepo, | ||
| gitBranches, | ||
| gitTags, | ||
| gitRefsCacheTime, | ||
| sshConnectionFailed: false, | ||
| } | ||
| : s | ||
| ) | ||
| ); | ||
| } catch (error) { | ||
| console.warn( | ||
| `[fetchGitInfoInBackground] Failed to fetch git info for session ${sessionId}:`, | ||
| error | ||
| ); | ||
| setSessions((prev) => | ||
| prev.map((s) => (s.id === sessionId ? { ...s, sshConnectionFailed: true } : s)) | ||
| ); |
There was a problem hiding this comment.
Route caught errors to Sentry (or rethrow) instead of console-only logging.
At Line 83-118, 304-314, and 368-377, errors are caught and only logged to console while continuing. This conflicts with the repo rule against swallowing exceptions. Please handle only expected/recoverable errors with explicit codes, and use captureException/captureMessage from src/utils/sentry for unexpected failures (or rethrow after capture).
Also applies to: 304-314, 368-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/session/useSessionRestoration.ts` around lines 83 - 118,
The catch blocks in fetchGitInfoInBackground (and the similar blocks around the
other catches) currently only console.warn errors and swallow them; update these
catches to distinguish expected/recoverable errors by checking a specific error
code or type (e.g., error.code or instanceof a known error) and handle those by
setting sshConnectionFailed via setSessions as now, but for unexpected failures
call captureException (or captureMessage) from the sentry utility and then
rethrow (or at minimum capture and rethrow) to avoid swallowing; reference the
gitService methods (isRepo, getBranches, getTags), the state updater
setSessions, and import captureException/captureMessage from your sentry helper
and invoke it inside the catch before rethrowing or marking the session failure.
Comment on lines
+83
to
+93
| // --- GitHub CLI availability check --- | ||
| useEffect(() => { | ||
| window.maestro.git | ||
| .checkGhCli() | ||
| .then((status) => { | ||
| setGhCliAvailable(status.installed && status.authenticated); | ||
| }) | ||
| .catch(() => { | ||
| setGhCliAvailable(false); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Replace console-only catch blocks with Sentry reporting or targeted handling.
At Line 83-93, 105-115, 118-129, 147-158, 172-198, 204-224, 231-246, and 249-265, errors are caught and only logged. This violates the repo rule against silently swallowing exceptions; use captureException/captureMessage from src/utils/sentry with context or let unexpected errors bubble after handling known error codes.
Also applies to: 105-115, 118-129, 147-158, 172-198, 204-224, 231-246, 249-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/ui/useAppInitialization.ts` around lines 83 - 93, Several
useEffect catch blocks (e.g., the GitHub CLI availability check that calls
window.maestro.git.checkGhCli()) currently swallow errors; replace those
console-only catches with Sentry reporting and targeted handling: import
captureException/captureMessage from src/utils/sentry and in each catch call
captureException(error, { extra: { context: 'checkGhCli' } }) or captureMessage
with a descriptive string, include relevant local state/context (function name
like checkGhCli, checkGhAuth, or the affected setter such as setGhCliAvailable)
and known error codes should be handled explicitly (map known codes to
user-friendly behavior) while rethrowing or allowing unexpected errors to bubble
after reporting.
…Each Prevents Cmd+F / Ctrl+F tests from leaking outputSearchOpen state into subsequent test cases.
This was referenced Feb 19, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Continues the App.tsx decomposition (Phases 2E–2G), extracting three more custom hooks to reduce the monolith by ~830 lines (9,556 → 8,767). Each hook is fully self-contained, reads from Zustand stores directly, and has comprehensive unit tests.
useSessionRestoration(405 lines, 49 tests)Extracts session loading, migration, and corruption recovery into
hooks/session/useSessionRestoration.ts:projectRoot,autoRunFolderPath,fileTreeAutoRefreshIntervalaiTabs→ creates default tab with warning;toolType: 'terminal'→ migrates toclaude-codeidleon restart (processes don't survive app restart)initialLoadCompleteproxy ref bridging.currentAPI to store booleanuseInputKeyDown(319 lines, 47 tests)Extracts the main input keyboard event handler into
hooks/input/useInputKeyDown.ts:getState()useAppInitialization(291 lines, 43 tests)Extracts all one-time startup effects into
hooks/ui/useAppInitialization.ts:window.playground())Key design decisions
getState()— no callback dependencies passed in from App.tsx. This eliminated hook ordering constraints and stale closure risks.getState()inside callbacks/effects: Leaderboard sync, standing ovation checks, and enter-to-send all read settings at call time rather than closing over reactive values, preventing stale closures.initialLoadComplete: Bridges the legacyref.current = trueAPI touseSessionStore.setInitialLoadComplete()so both patterns stay in sync without a migration.Stats
Test plan
projectRoot/autoRunFolderPathget defaults on restart@opens dropdown → filter/select inserts mention/opens menu → mode-specific filtering → Tab/Enter selectsghis installed/authenticated, disabled otherwiseSummary by CodeRabbit
Refactor
Tests