feat(ui): prompt to save view preferences on quit#468
Conversation
Greptile SummaryThis PR adds a quit-time prompt that detects in-session view-preference changes (theme, layout, line numbers, wrapping, hunk headers, agent notes, copy decorations), shows a diff of what changed, and optionally persists them to the resolved config file via a hand-rolled TOML upsert that preserves tables and comments.
Confidence Score: 4/5Safe to merge — the TOML upsert is well-tested and the quit flow is gated correctly; the main rough edge is the confirmation notice that fires and is discarded before the terminal can render it. The core save path (TOML manipulation, config-path resolution, keyboard routing) is solid and covered by unit and interaction tests. Two cosmetic issues stand out: a misplaced JSDoc comment that orphans resolveConfiguredCliInput's docs, and the session notice in saveViewPreferencesAndQuit being emitted synchronously before onQuit() tears down the component, so the confirmation is almost certainly never rendered. Neither blocks correctness — preferences are written to disk before quit — but the UX confirmation is dead code in its current form. src/ui/App.tsx (saveViewPreferencesAndQuit notice timing) and src/core/config.ts (orphaned JSDoc) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant App
participant useAppKeyboardShortcuts
participant saveGlobalViewPreferences
participant ConfigFile
User->>App: press q (quit)
App->>App: requestQuit()
alt hasUnsavedViewPreferences
App->>App: setShowHelp(false)
App->>App: setSaveConfigPromptOpen(true)
App-->>User: show "Save view preferences?" modal
alt User presses Enter / s
useAppKeyboardShortcuts->>App: saveViewPreferencesAndQuit()
App->>saveGlobalViewPreferences: preferences + configPath
saveGlobalViewPreferences->>ConfigFile: upsertTopLevelTomlValue (per key)
ConfigFile-->>saveGlobalViewPreferences: written
saveGlobalViewPreferences-->>App: configPath
App->>App: showSessionNotice(...)
App->>App: onQuit()
else User presses d / n
useAppKeyboardShortcuts->>App: discardViewPreferencesAndQuit()
App->>App: setSaveConfigPromptOpen(false)
App->>App: onQuit()
else User presses Esc
useAppKeyboardShortcuts->>App: closeSaveConfigPrompt()
App->>App: setSaveConfigPromptOpen(false)
end
else no unsaved changes
App->>App: onQuit()
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant App
participant useAppKeyboardShortcuts
participant saveGlobalViewPreferences
participant ConfigFile
User->>App: press q (quit)
App->>App: requestQuit()
alt hasUnsavedViewPreferences
App->>App: setShowHelp(false)
App->>App: setSaveConfigPromptOpen(true)
App-->>User: show "Save view preferences?" modal
alt User presses Enter / s
useAppKeyboardShortcuts->>App: saveViewPreferencesAndQuit()
App->>saveGlobalViewPreferences: preferences + configPath
saveGlobalViewPreferences->>ConfigFile: upsertTopLevelTomlValue (per key)
ConfigFile-->>saveGlobalViewPreferences: written
saveGlobalViewPreferences-->>App: configPath
App->>App: showSessionNotice(...)
App->>App: onQuit()
else User presses d / n
useAppKeyboardShortcuts->>App: discardViewPreferencesAndQuit()
App->>App: setSaveConfigPromptOpen(false)
App->>App: onQuit()
else User presses Esc
useAppKeyboardShortcuts->>App: closeSaveConfigPrompt()
App->>App: setSaveConfigPromptOpen(false)
end
else no unsaved changes
App->>App: onQuit()
end
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/core/config.ts:360-361
**Stray JSDoc steals documentation from `resolveConfiguredCliInput`**
The original `/** Resolve CLI input against global and repo-local config files. */` comment was the JSDoc for `resolveConfiguredCliInput`, but `saveGlobalViewPreferences` was inserted immediately after it. Now two JSDoc blocks precede `saveGlobalViewPreferences`, the first of which is incorrect, and `resolveConfiguredCliInput` (line 392) has no JSDoc at all. Most tooling uses the last preceding block, so generated docs will show the right text for `saveGlobalViewPreferences`, but `resolveConfiguredCliInput` silently loses its documentation. Move the `/** Resolve CLI input... */` comment down to sit directly above `resolveConfiguredCliInput`.
### Issue 2 of 3
src/ui/App.tsx:709-722
**Session notice is never visible — app quits before the notice renders**
`showSessionNotice(...)` schedules a React state update, but `onQuit()` is called synchronously in the same callback before any re-render. If `onQuit()` tears down the terminal (or unmounts the component tree) before the next paint, the "Saved view preferences to …" message is never displayed. The save itself succeeds, but the user gets no confirmation feedback. Consider calling `onQuit()` inside a `setTimeout` or a `useEffect` that triggers after the notice has rendered, similar to how other deferred-then-quit flows are handled in the codebase.
### Issue 3 of 3
src/ui/hooks/useAppKeyboardShortcuts.ts:274-282
**Undocumented `[s]` shortcut to save — the modal UI only shows `[Enter]`**
The keyboard handler accepts both `Enter` and `s` to trigger `saveViewPreferencesAndQuit`, but the modal in `App.tsx` renders only `[Enter] Save and quit` as the hint. A user who presses `s` will save-and-quit without any visual cue that `s` is a valid binding, while a user who looks for an `s` shortcut (consistent with other modals in the app) will not find the hint. Either drop the `s` binding or add it to the modal label.
Reviews (1): Last reviewed commit: "feat(ui): prompt to save view preference..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** Resolve CLI input against global and repo-local config files. */ | ||
| /** Persist accepted in-app view preferences to the user-level Hunk config file. */ |
There was a problem hiding this comment.
resolveConfiguredCliInput
The original /** Resolve CLI input against global and repo-local config files. */ comment was the JSDoc for resolveConfiguredCliInput, but saveGlobalViewPreferences was inserted immediately after it. Now two JSDoc blocks precede saveGlobalViewPreferences, the first of which is incorrect, and resolveConfiguredCliInput (line 392) has no JSDoc at all. Most tooling uses the last preceding block, so generated docs will show the right text for saveGlobalViewPreferences, but resolveConfiguredCliInput silently loses its documentation. Move the /** Resolve CLI input... */ comment down to sit directly above resolveConfiguredCliInput.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/config.ts
Line: 360-361
Comment:
**Stray JSDoc steals documentation from `resolveConfiguredCliInput`**
The original `/** Resolve CLI input against global and repo-local config files. */` comment was the JSDoc for `resolveConfiguredCliInput`, but `saveGlobalViewPreferences` was inserted immediately after it. Now two JSDoc blocks precede `saveGlobalViewPreferences`, the first of which is incorrect, and `resolveConfiguredCliInput` (line 392) has no JSDoc at all. Most tooling uses the last preceding block, so generated docs will show the right text for `saveGlobalViewPreferences`, but `resolveConfiguredCliInput` silently loses its documentation. Move the `/** Resolve CLI input... */` comment down to sit directly above `resolveConfiguredCliInput`.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Moved the stale JSDoc back above resolveConfiguredCliInput and left the persistence helper with its own accurate comment.
This comment was generated by Pi using OpenAI GPT-5
| const saveViewPreferencesAndQuit = useCallback(() => { | ||
| try { | ||
| const configPath = saveGlobalViewPreferences(currentViewPreferences, { | ||
| configPath: bootstrap.viewPreferencesConfigPath, | ||
| }); | ||
| initialViewPreferencesRef.current = currentViewPreferences; | ||
| showSessionNotice(`Saved view preferences to ${configPath}`); | ||
| onQuit(); | ||
| } catch (error) { | ||
| showSessionNotice( | ||
| error instanceof Error ? error.message : "Failed to save view preferences.", | ||
| ); | ||
| } | ||
| }, [bootstrap.viewPreferencesConfigPath, currentViewPreferences, onQuit, showSessionNotice]); |
There was a problem hiding this comment.
showSessionNotice(...) schedules a React state update, but onQuit() is called synchronously in the same callback before any re-render. If onQuit() tears down the terminal (or unmounts the component tree) before the next paint, the "Saved view preferences to …" message is never displayed. The save itself succeeds, but the user gets no confirmation feedback. Consider calling onQuit() inside a setTimeout or a useEffect that triggers after the notice has rendered, similar to how other deferred-then-quit flows are handled in the codebase.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/App.tsx
Line: 709-722
Comment:
**Session notice is never visible — app quits before the notice renders**
`showSessionNotice(...)` schedules a React state update, but `onQuit()` is called synchronously in the same callback before any re-render. If `onQuit()` tears down the terminal (or unmounts the component tree) before the next paint, the "Saved view preferences to …" message is never displayed. The save itself succeeds, but the user gets no confirmation feedback. Consider calling `onQuit()` inside a `setTimeout` or a `useEffect` that triggers after the notice has rendered, similar to how other deferred-then-quit flows are handled in the codebase.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Updated the save path to defer onQuit() briefly after showing the saved-preferences notice, and adjusted the regression test to cover the delayed quit.
This comment was generated by Pi using OpenAI GPT-5
| if (!saveConfigPromptOpenRef.current) { | ||
| return false; | ||
| } | ||
|
|
||
| consumeKey(key); | ||
| if (key.name === "return" || key.name === "enter" || key.name === "s" || key.sequence === "s") { | ||
| saveViewPreferencesAndQuit(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
[s] shortcut to save — the modal UI only shows [Enter]
The keyboard handler accepts both Enter and s to trigger saveViewPreferencesAndQuit, but the modal in App.tsx renders only [Enter] Save and quit as the hint. A user who presses s will save-and-quit without any visual cue that s is a valid binding, while a user who looks for an s shortcut (consistent with other modals in the app) will not find the hint. Either drop the s binding or add it to the modal label.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/hooks/useAppKeyboardShortcuts.ts
Line: 274-282
Comment:
**Undocumented `[s]` shortcut to save — the modal UI only shows `[Enter]`**
The keyboard handler accepts both `Enter` and `s` to trigger `saveViewPreferencesAndQuit`, but the modal in `App.tsx` renders only `[Enter] Save and quit` as the hint. A user who presses `s` will save-and-quit without any visual cue that `s` is a valid binding, while a user who looks for an `s` shortcut (consistent with other modals in the app) will not find the hint. Either drop the `s` binding or add it to the modal label.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Added the s shortcut to the modal hint so the displayed binding matches the keyboard handler.
This comment was generated by Pi using OpenAI GPT-5
benvinegar
force-pushed
the
feat/save-view-preferences-on-quit
branch
from
f480c95 to
f097d53
Compare
1 participant
Summary
Verification
bun run typecheckbun test src/core/config.test.ts src/ui/AppHost.interactions.test.tsx --test-name-pattern 'config persistence|merges global|defaults unspecified|quit prompt'\n\nThis PR description was generated by Pi using OpenAI GPT-5