feat(models): add context window controls - #1203
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds provider-level and per-model context-window settings. The management API validates and persists these values. The Models page provides localized controls for non-native providers, with validation, save feedback, catalog refresh, and integration coverage. ChangesProvider context-window configuration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Reviewer as Models page
participant API as Provider routes
participant Config as Provider configuration
participant Catalog as Catalog refresh
Reviewer->>API: PATCH provider and model context windows
API->>Config: Validate and persist settings
API-->>Reviewer: Save response
Reviewer->>Catalog: Refresh provider catalog
Catalog-->>Reviewer: Updated provider and model metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/pages/Models.tsx`:
- Around line 339-345: Update selectContextModel to preserve unsaved context
overrides in a per-model contextModelDrafts map instead of overwriting the
current draft when switching models; load each model’s existing draft or
persisted value when selected, and reset contextModelDrafts in
openContextSettings when opening settings for a new provider.
- Around line 356-398: Update saveContextSettings so the context modal closes
and publishFeedback reports success immediately after readJsonOrThrow confirms
the PATCH succeeded. Then call load(true) without gating or reversing the
successful save flow on its return value; remove the refreshed-result check and
contextError assignment for refresh failure, allowing the existing catalog error
handling to surface it separately.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac238eff-f247-4502-8a44-7cb79cbe2d3d
⛔ Files ignored due to path filters (1)
docs-site/public/pr-screenshots/1073-context-window-controls.jpgis excluded by!**/*.jpg
📒 Files selected for processing (15)
docs-site/src/content/docs/reference/configuration/providers.mdgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/models-groups.tsgui/src/pages/Models.tsxgui/src/styles-models-workspace.cssgui/tests/models-empty-provider.test.tsxgui/tests/models-provider-head.test.tssrc/server/management/provider-routes.tssrc/types.tstests/management-provider-validation.test.ts
| const selectContextModel = (modelId: string) => { | ||
| const group = groups.find(candidate => candidate.provider === contextModalProvider); | ||
| setContextModelId(modelId); | ||
| setContextModelDraft(group?.modelContextWindows?.[modelId] | ||
| ? String(group.modelContextWindows[modelId]) | ||
| : ""); | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Switching the model selector silently discards the unsaved override draft.
selectContextModel at Line 339 always overwrites contextModelDraft from group.modelContextWindows?.[modelId] the moment the user picks a different model in the Select at Line 1336-1342. If the user types a new override value for the currently selected model and then reselects a different model to check its value, the typed-but-unsaved value is lost with no warning, since nothing persists it before the overwrite.
Preserve per-model drafts across selector changes so browsing other models does not erase in-progress edits.
[recommended_refactor_or_suggestion]
♻️ Proposed fix: keep a per-model draft map
const [contextModelDraft, setContextModelDraft] = useState("");
+ const [contextModelDrafts, setContextModelDrafts] = useState<Record<string, string>>({}); const selectContextModel = (modelId: string) => {
const group = groups.find(candidate => candidate.provider === contextModalProvider);
+ setContextModelDrafts(prev => ({ ...prev, [contextModelId]: contextModelDraft }));
setContextModelId(modelId);
- setContextModelDraft(group?.modelContextWindows?.[modelId]
- ? String(group.modelContextWindows[modelId])
- : "");
+ setContextModelDraft(
+ contextModelDrafts[modelId]
+ ?? (group?.modelContextWindows?.[modelId] ? String(group.modelContextWindows[modelId]) : ""),
+ );
};contextModelDrafts should also be reset in openContextSettings when the dialog opens for a new provider.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/pages/Models.tsx` around lines 339 - 345, Update selectContextModel
to preserve unsaved context overrides in a per-model contextModelDrafts map
instead of overwriting the current draft when switching models; load each
model’s existing draft or persisted value when selected, and reset
contextModelDrafts in openContextSettings when opening settings for a new
provider.
| const saveContextSettings = async () => { | ||
| if (!contextModalProvider) return; | ||
| const providerWindow = parseContextWindowDraft(contextDefaultDraft); | ||
| const modelWindow = parseContextWindowDraft(contextModelDraft); | ||
| if (providerWindow === undefined || modelWindow === undefined) { | ||
| setContextError(t("models.contextInvalid")); | ||
| return; | ||
| } | ||
| const group = groups.find(candidate => candidate.provider === contextModalProvider); | ||
| if (!group) { | ||
| setContextError(t("models.contextSaveFailed")); | ||
| return; | ||
| } | ||
|
|
||
| setContextSaving(true); | ||
| setContextError(""); | ||
| try { | ||
| const body: Record<string, unknown> = { contextWindow: providerWindow }; | ||
| if (contextModelId) { | ||
| body.modelContextWindows = { [contextModelId]: modelWindow }; | ||
| } | ||
| const response = await fetch( | ||
| `${apiBase}/api/providers?name=${encodeURIComponent(contextModalProvider)}`, | ||
| { | ||
| method: "PATCH", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(body), | ||
| }, | ||
| ); | ||
| await readJsonOrThrow(response, t("models.contextSaveFailed")); | ||
| const refreshed = await load(true); | ||
| if (!refreshed) { | ||
| setContextError(t("models.loadFail")); | ||
| return; | ||
| } | ||
| setContextModalProvider(null); | ||
| publishFeedback(true, t("models.contextSaved")); | ||
| } catch (error) { | ||
| setContextError(error instanceof Error ? error.message : t("models.contextSaveFailed")); | ||
| } finally { | ||
| setContextSaving(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A refresh failure after a successful save reports the save as failed, unlike every other modal mutation in this file.
At Line 385, readJsonOrThrow(response, ...) does not throw, so the PATCH succeeded and the context-window values are persisted. At Line 386, load(true) is only a post-save catalog refresh. If it fails (transient network error, slow proxy), Lines 387-390 set contextError to t("models.loadFail") ("Failed to load models — is the proxy running?") and return before Line 391-392 ever run — so the modal stays open, showing an error that implies the save itself failed, when it did not.
Compare with addCustomModel (Lines 690-697) and updateCustomModel (Lines 714-721): both close the modal and call publishFeedback(true, ...) immediately once the write succeeds, then await load(true) afterward without gating success on its result. saveContextSettings is the only modal-driven mutation in this file that reverses that order and conditions the success path on the refresh outcome.
A user hitting a refresh hiccup right after saving will see a misleading "proxy not running" style message and may retry the save unnecessarily. Report success as soon as the PATCH succeeds, and let the refresh failure surface separately (the page already has catalogState.showError for a stale-catalog banner at Line 1554).
🐛 Proposed fix: match the addCustomModel/updateCustomModel pattern
await readJsonOrThrow(response, t("models.contextSaveFailed"));
- const refreshed = await load(true);
- if (!refreshed) {
- setContextError(t("models.loadFail"));
- return;
- }
setContextModalProvider(null);
publishFeedback(true, t("models.contextSaved"));
+ await load(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const saveContextSettings = async () => { | |
| if (!contextModalProvider) return; | |
| const providerWindow = parseContextWindowDraft(contextDefaultDraft); | |
| const modelWindow = parseContextWindowDraft(contextModelDraft); | |
| if (providerWindow === undefined || modelWindow === undefined) { | |
| setContextError(t("models.contextInvalid")); | |
| return; | |
| } | |
| const group = groups.find(candidate => candidate.provider === contextModalProvider); | |
| if (!group) { | |
| setContextError(t("models.contextSaveFailed")); | |
| return; | |
| } | |
| setContextSaving(true); | |
| setContextError(""); | |
| try { | |
| const body: Record<string, unknown> = { contextWindow: providerWindow }; | |
| if (contextModelId) { | |
| body.modelContextWindows = { [contextModelId]: modelWindow }; | |
| } | |
| const response = await fetch( | |
| `${apiBase}/api/providers?name=${encodeURIComponent(contextModalProvider)}`, | |
| { | |
| method: "PATCH", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify(body), | |
| }, | |
| ); | |
| await readJsonOrThrow(response, t("models.contextSaveFailed")); | |
| const refreshed = await load(true); | |
| if (!refreshed) { | |
| setContextError(t("models.loadFail")); | |
| return; | |
| } | |
| setContextModalProvider(null); | |
| publishFeedback(true, t("models.contextSaved")); | |
| } catch (error) { | |
| setContextError(error instanceof Error ? error.message : t("models.contextSaveFailed")); | |
| } finally { | |
| setContextSaving(false); | |
| } | |
| }; | |
| const saveContextSettings = async () => { | |
| if (!contextModalProvider) return; | |
| const providerWindow = parseContextWindowDraft(contextDefaultDraft); | |
| const modelWindow = parseContextWindowDraft(contextModelDraft); | |
| if (providerWindow === undefined || modelWindow === undefined) { | |
| setContextError(t("models.contextInvalid")); | |
| return; | |
| } | |
| const group = groups.find(candidate => candidate.provider === contextModalProvider); | |
| if (!group) { | |
| setContextError(t("models.contextSaveFailed")); | |
| return; | |
| } | |
| setContextSaving(true); | |
| setContextError(""); | |
| try { | |
| const body: Record<string, unknown> = { contextWindow: providerWindow }; | |
| if (contextModelId) { | |
| body.modelContextWindows = { [contextModelId]: modelWindow }; | |
| } | |
| const response = await fetch( | |
| `${apiBase}/api/providers?name=${encodeURIComponent(contextModalProvider)}`, | |
| { | |
| method: "PATCH", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify(body), | |
| }, | |
| ); | |
| await readJsonOrThrow(response, t("models.contextSaveFailed")); | |
| setContextModalProvider(null); | |
| publishFeedback(true, t("models.contextSaved")); | |
| await load(true); | |
| } catch (error) { | |
| setContextError(error instanceof Error ? error.message : t("models.contextSaveFailed")); | |
| } finally { | |
| setContextSaving(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/pages/Models.tsx` around lines 356 - 398, Update saveContextSettings
so the context modal closes and publishFeedback reports success immediately
after readJsonOrThrow confirms the PATCH succeeded. Then call load(true) without
gating or reversing the successful save flow on its return value; remove the
refreshed-result check and contextError assignment for refresh failure, allowing
the existing catalog error handling to surface it separately.
94d0e2e to
d648818
Compare
|
Thank you — this is adopted, and the layering you chose held up under review. Your three commits are the first three of #1223, cherry-picked with authorship intact. The structural decision that mattered most was leaving catalog derivation alone: Four corrections on top, all found by independent audit rather than preference: Only the selected model was saved. Sending everything that differs would have been wrong the other way. The 10s poll can refresh a field mid-modal; comparing drafts against live state then marks an untouched field dirty and reverts a change the user never made. It now requires both conditions — the user touched it, and the value differs from an opening snapshot held as canonical numbers, so retyping 64000 as "64,000" is not an edit. The provider default needed the same treatment; it was previously sent on every Apply.
An override for a model that left live discovery was unreachable — in the drafts map, absent from the picker. One note on your test for the post-save refresh failure: it was already correct. I initially wrote it up as a defect and was wrong — Closing in favor of #1223. The credit is yours. |
…reverting concurrent changes Adopts #1203 by @estelledc — the first three commits are theirs, cherry-picked with authorship intact. The approach was right: expose the existing `providers.<id>.contextWindow` / `modelContextWindows` contract at the management and UI layers without touching catalog derivation, which already materializes those values when upstream metadata is absent. Four corrections, all found by independent audit. **Only the selected model was saved.** The drafts map held edits for every model but the PATCH was keyed on `contextModelId`, so a value typed into model A and then abandoned by switching to B vanished — no error, no warning. The PR's own test pinned that as correct. It now sends every model the user typed into. **But "every model that differs" would have been wrong the other way.** The 10s poll can refresh a field while the modal is open; diffing drafts against live state would then call an untouched field dirty and revert someone else's change. Two conditions are required: the user touched it, AND the value differs from what the modal opened with. Both apply to the provider default too, which was previously sent unconditionally and would stamp a stale number over a concurrent update. The snapshot holds canonical numbers, not the raw text. Retyping 64000 as "64,000" is not an edit, and treating it as one would resurrect the same stale-write. When nothing survives the comparison, no PATCH is sent at all and the feedback says so rather than claiming an update. **`Number.isInteger(1e100)` is true.** Both the management validator and the form accepted it; it would persist and serialize into the catalog as an enormous number that can make Codex reject the file. Both now require a safe integer. The default is only validated when touched, so a value inherited from a hand-edited config cannot block an unrelated per-model save. **An override for a model that left live discovery was unreachable.** It sat in the drafts map, absent from the picker, impossible to inspect or clear. Tests: the exact #1073 reproduction is split in two, because a single case setting `modelContextWindows` keeps passing with the provider-wide fallback deleted. Ablations were driven red in their real defect shape rather than as artificially strong mutants — notably, comparing against live `groups` while keeping the touched guard is only visible when a field is edited, reverted, and changed server-side, which the suite now covers. Translated provider docs (ko/ja/ru/zh-cn) described both fields as caps only, which reads as the opposite of the fix for non-English users. Co-authored-by: zhouxun <zhouxun.13@bytedance.com> Closes #1073
…reverting concurrent changes Adopts #1203 by @estelledc — the first three commits are theirs, cherry-picked with authorship intact. The approach was right: expose the existing `providers.<id>.contextWindow` / `modelContextWindows` contract at the management and UI layers without touching catalog derivation, which already materializes those values when upstream metadata is absent. Four corrections, all found by independent audit. **Only the selected model was saved.** The drafts map held edits for every model but the PATCH was keyed on `contextModelId`, so a value typed into model A and then abandoned by switching to B vanished — no error, no warning. The PR's own test pinned that as correct. It now sends every model the user typed into. **But "every model that differs" would have been wrong the other way.** The 10s poll can refresh a field while the modal is open; diffing drafts against live state would then call an untouched field dirty and revert someone else's change. Two conditions are required: the user touched it, AND the value differs from what the modal opened with. Both apply to the provider default too, which was previously sent unconditionally and would stamp a stale number over a concurrent update. The snapshot holds canonical numbers, not the raw text. Retyping 64000 as "64,000" is not an edit, and treating it as one would resurrect the same stale-write. When nothing survives the comparison, no PATCH is sent at all and the feedback says so rather than claiming an update. **`Number.isInteger(1e100)` is true.** Both the management validator and the form accepted it; it would persist and serialize into the catalog as an enormous number that can make Codex reject the file. Both now require a safe integer. The default is only validated when touched, so a value inherited from a hand-edited config cannot block an unrelated per-model save. **An override for a model that left live discovery was unreachable.** It sat in the drafts map, absent from the picker, impossible to inspect or clear. Tests: the exact #1073 reproduction is split in two, because a single case setting `modelContextWindows` keeps passing with the provider-wide fallback deleted. Ablations were driven red in their real defect shape rather than as artificially strong mutants — notably, comparing against live `groups` while keeping the touched guard is only visible when a field is edited, reverted, and changed server-side, which the suite now covers. Translated provider docs (ko/ja/ru/zh-cn) described both fields as caps only, which reads as the opposite of the fix for non-English users. Co-authored-by: zhouxun <zhouxun.13@bytedance.com> Closes #1073
…reverting concurrent changes Adopts #1203 by @estelledc — the first three commits are theirs, cherry-picked with authorship intact. The approach was right: expose the existing `providers.<id>.contextWindow` / `modelContextWindows` contract at the management and UI layers without touching catalog derivation, which already materializes those values when upstream metadata is absent. Four corrections, all found by independent audit. **Only the selected model was saved.** The drafts map held edits for every model but the PATCH was keyed on `contextModelId`, so a value typed into model A and then abandoned by switching to B vanished — no error, no warning. The PR's own test pinned that as correct. It now sends every model the user typed into. **But "every model that differs" would have been wrong the other way.** The 10s poll can refresh a field while the modal is open; diffing drafts against live state would then call an untouched field dirty and revert someone else's change. Two conditions are required: the user touched it, AND the value differs from what the modal opened with. Both apply to the provider default too, which was previously sent unconditionally and would stamp a stale number over a concurrent update. The snapshot holds canonical numbers, not the raw text. Retyping 64000 as "64,000" is not an edit, and treating it as one would resurrect the same stale-write. When nothing survives the comparison, no PATCH is sent at all and the feedback says so rather than claiming an update. **`Number.isInteger(1e100)` is true.** Both the management validator and the form accepted it; it would persist and serialize into the catalog as an enormous number that can make Codex reject the file. Both now require a safe integer. The default is only validated when touched, so a value inherited from a hand-edited config cannot block an unrelated per-model save. **An override for a model that left live discovery was unreachable.** It sat in the drafts map, absent from the picker, impossible to inspect or clear. Tests: the exact #1073 reproduction is split in two, because a single case setting `modelContextWindows` keeps passing with the provider-wide fallback deleted. Ablations were driven red in their real defect shape rather than as artificially strong mutants — notably, comparing against live `groups` while keeping the touched guard is only visible when a field is edited, reverted, and changed server-side, which the suite now covers. Translated provider docs (ko/ja/ru/zh-cn) described both fields as caps only, which reads as the opposite of the fix for non-English users. Co-authored-by: zhouxun <zhouxun.13@bytedance.com> Closes #1073
2 participants
Summary
contextWindowandmodelContextWindowsthrough the existing provider management route, including positive-integer validation and per-model merge/delete behavior.Closes #1073
Verification
bun run test- 9640 passed, 8 skipped, 0 failed across 598 files.bun test tests/management-provider-validation.test.ts- 47 passed, 0 failed.bun run typecheck- passed.cd gui && bun test- 647 passed, 0 failed across 116 files.cd gui && bun run lint- passed.cd gui && bun run build- passed.cd docs-site && bun run build- passed, 221 pages built.0stayed in the dialog with a validation error; a valid save returnedPATCH 200; reopening showed256000and64000; no application errors appeared in the console.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes