Context window control and best reply badge#30
Conversation
…ply badge - Migration 145: adds context_window_size column to chats table - ChatAPI: adds contextWindowSize to Chat type, useUpdateContextWindowSize mutation - ChatState: adds applyContextWindow() to slice message sets before LLM calls - MessageAPI: wires applyContextWindow at 3 call sites (restartLegacy, streamTools, addToCompare), resolving window size from chat or app_metadata fallback - ContextWindowPill: new pill component in ChatInput showing "N/T turns" when active, popover with toggle + number input - MultiChat: adds filled star badge next to model name for historical selected messages, and star action button in hover bar for historical unselected messages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Adds per-chat context window limiting (with a global app_metadata fallback) and introduces a “best reply” affordance for historical tools-block replies to improve QoL when working with longer/multi-model chats.
Changes:
- Added
context_window_sizetochats(migration + Chat API typing/querying + mutation to update it). - Implemented context-window slicing via
applyContextWindow(...)and wired it into 3 LLM conversation build paths. - Added UI for context window control (
ContextWindowPill) and a star badge/action for “best reply” on older tools rows.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/components/MultiChat.tsx | Adds star badge + star action button for historical tools messages. |
| src/ui/components/ContextWindowPill.tsx | New pill/popover UI to enable/disable and set per-chat context window size. |
| src/ui/components/ChatInput.tsx | Surfaces ContextWindowPill in the input bar for non-reply chats. |
| src/core/chorus/ChatState.ts | Adds applyContextWindow utility for slicing message sets by last N turns. |
| src/core/chorus/api/MessageAPI.ts | Resolves window size (chat setting / app_metadata fallback) and applies it at LLM call sites. |
| src/core/chorus/api/ChatAPI.ts | Adds contextWindowSize to Chat and a useUpdateContextWindowSize mutation. |
| src-tauri/src/migrations.rs | Migration 145 adds context_window_size column to chats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <button | ||
| className={`hover:text-foreground ${message.selected ? "text-amber-400" : ""}`} | ||
| onClick={() => { | ||
| selectMessage.mutate({ | ||
| chatId: message.chatId, | ||
| messageSetId: | ||
| message.messageSetId, | ||
| messageId: | ||
| message.id, | ||
| }); | ||
| }} |
There was a problem hiding this comment.
selectMessage.mutate is called without the required blockType argument. useSelectMessage expects { chatId, messageSetId, messageId, blockType }, and omitting it will either fail type-checking or result in a DB update with an undefined block type (no rows updated), so clicking the star won’t reliably mark the message as selected. Pass blockType: "tools" here (and consider no-op’ing the click when message.selected is already true).
| export function applyContextWindow( | ||
| messageSets: MessageSetDetail[], | ||
| windowSize: number | undefined, | ||
| ): MessageSetDetail[] { | ||
| if (windowSize === undefined) return messageSets; | ||
| if (windowSize === 0) return []; | ||
| return messageSets.slice(-(windowSize * 2)); | ||
| } |
There was a problem hiding this comment.
applyContextWindow should defensively handle invalid window sizes (e.g., negative numbers or NaN). With the current implementation, a negative value will slice from the front (unexpectedly dropping recent context), and NaN will effectively disable slicing in a non-obvious way. Suggest clamping/validating with something like: if windowSize is not a finite integer > 0, treat it as undefined (full history) or as 0 (empty), depending on intended semantics.
| const chat = await queryClient.ensureQueryData( | ||
| chatQueries.detail(chatId), | ||
| ); | ||
| const appMetadata = await queryClient.ensureQueryData({ | ||
| queryKey: appMetadataKeys.appMetadata(), | ||
| queryFn: () => fetchAppMetadata(), | ||
| }); | ||
| const windowSize = | ||
| chat?.contextWindowSize ?? | ||
| (appMetadata["default_context_window_size"] | ||
| ? parseInt(appMetadata["default_context_window_size"]) | ||
| : undefined); | ||
| const conversation = [ | ||
| ...llmConversation(previousMessageSets), | ||
| ...llmConversation(applyContextWindow(previousMessageSets, windowSize)), |
There was a problem hiding this comment.
The default_context_window_size fallback is parsed with parseInt(...) but never validated. If the metadata value is non-numeric, negative, or 0, windowSize becomes NaN/invalid and is still passed into applyContextWindow, producing surprising behavior. Consider centralizing window-size resolution into a small helper (used at all three call sites) that validates Number.isFinite, clamps to an integer, and ignores non-positive values.
| {isActive && ( | ||
| <div className="space-y-1.5"> | ||
| <label className="text-xs text-muted-foreground"> | ||
| Last N turns to include | ||
| </label> | ||
| <input | ||
| type="number" | ||
| min={1} | ||
| value={windowSize} | ||
| onChange={(e) => handleSizeChange(e.target.value)} | ||
| className="w-full rounded-md border border-input bg-background px-2 py-1 text-sm focus:outline-none focus:ring-1 focus:ring-ring" | ||
| /> |
There was a problem hiding this comment.
The context window size <input> is controlled by windowSize coming from the chat query, and onChange immediately triggers a mutation. This makes the field hard to edit (e.g., backspace produces '' → ignored → value snaps back; typing multi-digit values will fire multiple DB writes and can lag). Keep a local input state (string) while editing and commit via debounce/onBlur/Enter, then update the persisted value.
- Fix readChat converting contextWindowSize=0 to undefined (critical: Current Message mode was silently dropped on reload since ?? treats 0 as falsy; now uses strict null check) - Pre-compute dimming cutoff and message set slicing with useMemo in MultiChat to avoid per-render recalculations - Add stable EMPTY_STRING_SET constant for minimizedModels fallback - Add ARIA role=radiogroup, role=radio, aria-checked to ContextWindowPill toggle buttons for screen reader accessibility - Add focus-visible ring styles to toggle buttons - Add htmlFor/id association for number input label - Disable number input when in Full History mode - Add aria-hidden to dimmed MessageSetView containers - Reduce dimming transition from 300ms to 200ms - Add 15 new test cases for contextWindowState (edge cases: contextWindowSize 0, NaN, Infinity, empty string, number inputs, undefined stored turns)
Implements two QoL features:
Feature 1: Context Window Control
Lets users limit how many previous turns are sent to the LLM per chat, with a global
app_metadatafallback.context_window_size INTEGER DEFAULT NULLtochatsChatAPI: exposescontextWindowSizeonChattype; newuseUpdateContextWindowSizemutationChatState: newapplyContextWindow(messageSets, windowSize)utility that slices to the lastN×2message setsMessageAPI: wiresapplyContextWindowat all three LLM call sites (useRestartMessageLegacy,useStreamToolsMessage,useAddMessageToCompareBlock), resolving window size from the chat's own setting ordefault_context_window_sizeinapp_metadataContextWindowPill: pill button in the chat input bar; shows "N/T turns" when active; popover has a toggle switch + number inputFeature 2: Best Reply Badge
For historical tools-block message sets (not the last row):
message.selected === trueselectMessage; tooltip reads "Mark as best" / "Best reply"Test Plan
by-claude