feat: configurable chat history scrollback#427
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an app-wide “chat history scrollback” setting (number of conversation items retained per thread, with null meaning unlimited), exposes it in Settings UI, and wires it into thread item retention/trimming behavior across the app (including a deferred-trim behavior while settings hydrate).
Changes:
- Make
prepareThreadItems()accept an optionalmaxItemsPerThread(including unlimited vianull) and update reducers to pass the configured limit. - Add
chatHistoryScrollbackItemsto app settings (frontend + Tauri) with normalization/defaulting, plus Settings UI controls (unlimited toggle, presets, custom input). - Add/extend unit + integration tests covering retention, trimming on config reduction, and the deferred-trim hydration flow.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/threadItems.ts | Adds configurable maxItemsPerThread behavior (null = unlimited) to thread item preparation/retention. |
| src/utils/threadItems.test.ts | Adds tests for custom and unlimited scrollback behavior. |
| src/types.ts | Extends AppSettings with chatHistoryScrollbackItems: number | null. |
| src/features/threads/hooks/useThreadsReducer.ts | Adds maxItemsPerThread to thread state and a new reducer slice for config changes. |
| src/features/threads/hooks/useThreadsReducer.test.ts | Tests trimming behavior when the max is reduced. |
| src/features/threads/hooks/useThreads.ts | Plumbs the setting into the reducer initialization and updates via setMaxItemsPerThread. |
| src/features/threads/hooks/useThreads.integration.test.tsx | Adds an integration test validating deferred trimming until the setting is applied. |
| src/features/threads/hooks/threadReducer/threadItemsSlice.ts | Passes state.maxItemsPerThread into prepareThreadItems() for all relevant thread item updates. |
| src/features/threads/hooks/threadReducer/threadConfigSlice.ts | New slice to normalize/apply setMaxItemsPerThread and trim existing thread items. |
| src/features/settings/hooks/useAppSettings.ts | Adds defaults + normalization for chatHistoryScrollbackItems. |
| src/features/settings/components/sections/SettingsDisplaySection.tsx | Adds Settings UI for chat scrollback (unlimited toggle, presets, custom input + guidance). |
| src/features/settings/components/sections/SettingsDisplaySection.test.tsx | Adds UI tests for the new chat scrollback controls. |
| src/features/settings/components/SettingsView.test.tsx | Updates settings fixtures to include the new setting. |
| src/App.tsx | Passes scrollback setting into useThreads, using null while settings are loading to avoid premature trimming. |
| src-tauri/src/types.rs | Adds the setting to the Tauri AppSettings struct with a default of Some(200) and a test assertion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }); | ||
|
|
||
| it("applies scrollback limit before loading thread items", async () => { |
There was a problem hiding this comment.
This test name reads like the scrollback limit is applied before loading items, but the assertions actually verify the opposite (load unlimited when scrollbackItems is null, then trim after rerender to 200). Renaming the test to reflect the deferred-trimming behavior would make the intent clearer.
| it("applies scrollback limit before loading thread items", async () => { | |
| it("trims active items to scrollback limit after loading thread items", async () => { |
There was a problem hiding this comment.
Renamed the test to better match the deferred-trim behavior (defers trimming until scrollback settings hydrate). Fixed in 433fe25.
| function normalizeMaxItemsPerThread(value: number | null): number | null { | ||
| if (value === null) { | ||
| return null; | ||
| } | ||
| if (typeof value !== "number" || !Number.isFinite(value) || value <= 0) { | ||
| return 200; | ||
| } | ||
| return Math.floor(value); |
There was a problem hiding this comment.
normalizeMaxItemsPerThread hard-codes 200 as the fallback default. This same default is also defined in other places (e.g., threadItems + settings normalization), which makes it easy for defaults to drift over time. Consider importing a single shared constant (or exporting the threadItems default) and using it here.
There was a problem hiding this comment.
Good call — replaced the hard-coded 200 with shared CHAT_SCROLLBACK_DEFAULT from src/utils/chatScrollback.ts. Fixed in 433fe25.
| nextItemsByThread[threadId] = prepareThreadItems(items, { | ||
| maxItemsPerThread: normalized, | ||
| }); |
There was a problem hiding this comment.
setMaxItemsPerThread rebuilds every thread’s item array by calling prepareThreadItems for all entries in itemsByThread. Since reducer state already stores prepared items, this does extra normalization/truncation work and can be expensive on startup or with many threads. Consider trimming via a lightweight slice of the existing arrays (and only re-preparing when you detect unprepared/raw items).
| nextItemsByThread[threadId] = prepareThreadItems(items, { | |
| maxItemsPerThread: normalized, | |
| }); | |
| const limit = normalized ?? items.length; | |
| const startIndex = Math.max(items.length - limit, 0); | |
| nextItemsByThread[threadId] = items.slice(startIndex); |
There was a problem hiding this comment.
Updated setMaxItemsPerThread to only slice(-limit) when the thread exceeds the new limit (and preserve references otherwise) instead of re-running prepareThreadItems for every thread. Fixed in 433fe25.
| const CHAT_SCROLLBACK_DEFAULT = 200; | ||
| const CHAT_SCROLLBACK_MIN = 50; | ||
| const CHAT_SCROLLBACK_MAX = 5000; | ||
|
|
||
| function normalizeChatHistoryScrollbackItems(value: unknown): number | null { | ||
| if (value === null) { | ||
| return null; | ||
| } | ||
| const parsed = | ||
| typeof value === "number" | ||
| ? value | ||
| : typeof value === "string" | ||
| ? Number(value) | ||
| : Number.NaN; | ||
| if (!Number.isFinite(parsed)) { | ||
| return CHAT_SCROLLBACK_DEFAULT; | ||
| } | ||
| const clamped = Math.min( | ||
| CHAT_SCROLLBACK_MAX, | ||
| Math.max(CHAT_SCROLLBACK_MIN, parsed), | ||
| ); | ||
| return Math.round(clamped); | ||
| } |
There was a problem hiding this comment.
The scrollback constants (default/min/max) are duplicated here and again in the Settings UI, and there’s also a separate default in the Tauri settings struct. This duplication increases the risk of inconsistent behavior (e.g., UI allows a value the settings normalizer clamps differently). Consider moving these constants + the normalization/clamp helper to a shared module and reusing it across the settings hook + settings UI (and ideally aligning the backend default too).
There was a problem hiding this comment.
Centralized scrollback defaults/presets/clamping in src/utils/chatScrollback.ts and reused it from both useAppSettings and SettingsDisplaySection to avoid drift. Fixed in 433fe25 + ffd637f.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d99da41ba7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value={scrollbackDraft} | ||
| disabled={scrollbackUnlimited} | ||
| onChange={(event) => setScrollbackDraft(event.target.value)} | ||
| onBlur={commitScrollback} |
There was a problem hiding this comment.
When the max-items input loses focus, onBlur always calls commitScrollback, so clicking another control (for example the “Unlimited chat history” toggle or Reset) first persists the draft value before that click handler runs. In the common case where the draft is lower than the current limit, this transient update immediately trims thread items via setMaxItemsPerThread/prepareThreadItems (slice), and the subsequent higher/unlimited update cannot restore the dropped local history. This causes unintended data loss from a single click path and should avoid committing on blur when focus is moving to another scrollback control.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — guarded the max-items onBlur so it does not commit when focus is moving to another scrollback control (toggle/preset/reset). Added data-scrollback-control markers + regression tests. Fixed in ffd637f.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Co-authored-by: Thomas Ricouard <ricouard77@gmail.com>
2 participants
Fixes #422
This is a reduced-scope follow-up to #423 (closed): it keeps the configurable scrollback setting + retention logic, but drops the conversation view virtualization to avoid the scroll-jump issue and reduce complexity.
What
null= unlimited).Notes
How to validate
Tests
npm run lintnpm run typechecknpm run testcd src-tauri && cargo test