Repository navigation
feat: Persist AI settings to TOML in the CLI - #653
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds editable AI settings: backend parses and exposes providers, custom models, and model parameters; exposes PUT /api/ai/settings to persist changes to the TOML config; frontend store wires autosave (debounced, deduped) to save AI settings when the runtime config is writable. ChangesAI Settings Persistence: Config Load, Edit, and Autosave
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend (saveAiSettingsToServer)
participant Server as SqlroomsHttpServer
participant Writer as _write_ai_settings_to_toml
participant FS as Config file
UI->>Server: PUT /api/ai/settings {settings, defaultProvider?, defaultModel?} (X-SQLRooms-Token)
Server->>Writer: validate payload and call writer with config_path
Writer->>FS: merge and write updated config.toml (preserve api_key_env)
Writer-->>Server: return config_path
Server-->>UI: 200 OK {config_path}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/ai-core/__tests__/AiSlice.modelSelection.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/ai-core/src/AiSlice.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
python/sqlrooms-cli/tests/test_launcher.py (1)
440-485: ⚡ Quick winExpand persistence assertions for
/api/ai/settings.The payload includes
defaultModelandmodelParameters, but this test does not assert those persisted/runtime outcomes. Adding them will catch partial-write regressions.Suggested assertion additions
assert doc["ai"]["default_provider"] == "anthropic" + assert doc["ai"]["default_model"] == "claude-4-sonnet" assert doc["ai"]["providers"][0]["api_key"] == "anthropic-key" + assert doc["ai"]["model_parameters"]["max_steps"] == 8 + assert doc["ai"]["model_parameters"]["additional_instruction"] == "" assert server.llm_provider == "anthropic" + assert server.llm_model == "claude-4-sonnet"🤖 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 `@python/sqlrooms-cli/tests/test_launcher.py` around lines 440 - 485, Expand assertions in test_api_put_ai_settings_writes_config to verify that the sent defaultModel and modelParameters were persisted and applied at runtime: after loading the toml (doc) assert doc["ai"]["default_model"] == "claude-4-sonnet" and that doc["ai"]["model_parameters"]["max_steps"] == 8 and doc["ai"]["model_parameters"]["additional_instruction"] == "" (and/or equivalent keys), and assert the server runtime fields were updated (e.g. server.llm_model == "claude-4-sonnet" and server.model_parameters["max_steps"] == 8) so the test catches partial-write regressions; locate assertions near the existing tomllib load and server.llm_provider checks in test_api_put_ai_settings_writes_config within SqlroomsHttpServer test setup.
🤖 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 `@apps/sqlrooms-cli-ui/src/store.ts`:
- Around line 715-726: The autosave updates lastSnapshot before the async PUT
completes, which prevents retries on failure; change the logic in the save timer
handler (the setTimeout block that calls saveAiSettingsToServer and references
getAiSettingsTomlPayload) so that lastSnapshot is only assigned after
saveAiSettingsToServer resolves successfully (i.e., in the .then or after
await), and leave lastSnapshot unchanged on .catch so the pending snapshot can
be retried; ensure you still clear saveTimer appropriately and keep the same
payload parsing behavior.
In `@python/sqlrooms-cli/README.md`:
- Around line 49-50: Update the README sentence "AI settings changed in the CLI
UI are saved back to this file automatically when config loading is enabled" to
also state that saving occurs only if the config file is writable; specifically
mention both conditions (config loading enabled and file writable) so users know
the CLI will not persist changes when the config file lacks write permissions.
In `@python/sqlrooms-cli/sqlrooms/web/launcher.py`:
- Around line 603-610: The current except block around
_write_ai_settings_to_toml maps client validation errors to 500; update error
handling to catch validation/client errors (e.g., ValueError) separately and
return JSONResponse({"error": str(exc)}, status_code=400) for those cases, while
preserving the general except Exception as exc branch to log via
logger.error("Failed to write AI settings to %s: %s", self.config_path, exc) and
return 500 for unexpected server errors; locate the try/except that calls
_write_ai_settings_to_toml in launcher.py and implement the specific ValueError
(or a small tuple of expected validation exceptions) except before the generic
one.
- Around line 158-160: The code currently accepts floats for max_steps and
silently truncates them via int(max_steps); change the validation around
max_steps so only integer values are accepted (e.g., check isinstance(max_steps,
int)), and if a non-integer numeric (or non-int) value is provided raise a clear
exception (TypeError or ValueError) with a message indicating maxSteps must be
an integer; update the branch that currently does params_table.add("max_steps",
int(max_steps)) to only run for ints and leave additionalInstruction handling
(model_parameters.get("additionalInstruction")) unchanged.
---
Nitpick comments:
In `@python/sqlrooms-cli/tests/test_launcher.py`:
- Around line 440-485: Expand assertions in
test_api_put_ai_settings_writes_config to verify that the sent defaultModel and
modelParameters were persisted and applied at runtime: after loading the toml
(doc) assert doc["ai"]["default_model"] == "claude-4-sonnet" and that
doc["ai"]["model_parameters"]["max_steps"] == 8 and
doc["ai"]["model_parameters"]["additional_instruction"] == "" (and/or equivalent
keys), and assert the server runtime fields were updated (e.g. server.llm_model
== "claude-4-sonnet" and server.model_parameters["max_steps"] == 8) so the test
catches partial-write regressions; locate assertions near the existing tomllib
load and server.llm_provider checks in test_api_put_ai_settings_writes_config
within SqlroomsHttpServer test setup.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbc355c4-30c7-414b-a894-250c8d3b3478
📒 Files selected for processing (8)
apps/sqlrooms-cli-ui/src/runtimeConfig.tsapps/sqlrooms-cli-ui/src/serverApi.tsapps/sqlrooms-cli-ui/src/store.tspython/sqlrooms-cli/README.mdpython/sqlrooms-cli/sqlrooms/cli.pypython/sqlrooms-cli/sqlrooms/web/launcher.pypython/sqlrooms-cli/tests/test_cli.pypython/sqlrooms-cli/tests/test_launcher.py
| models: Array<{modelName: string}>; | ||
| } | ||
| >; | ||
| customModels?: Array<{ |
Collaborator
There was a problem hiding this comment.
I'd use [] instead of Array
dmitriy-kostianetskiy
approved these changes
May 22, 2026
dmitriy-kostianetskiy
approved these changes
May 25, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/sqlrooms-cli-ui/src/store.ts (1)
629-645:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIgnore stale autosave responses.
Older PUTs can finish after newer ones. In that case, Line 636 can rewind
lastSnapshot, and Lines 640-644 can show a failure toast even though a later snapshot already saved successfully. Gate the.then/.catchside effects behind a monotonically increasing request id so only the latest in-flight save updates local autosave state.💡 Minimal fix
function startAiSettingsTomlAutosave() { if (!runtimeConfig.configWritable) return; let saveTimer: ReturnType<typeof setTimeout> | null = null; + let latestSaveRequestId = 0; let lastSnapshot = JSON.stringify( getAiSettingsTomlPayload(roomStore.getState()), ); @@ } saveTimer = setTimeout(() => { saveTimer = null; + const requestId = ++latestSaveRequestId; void saveAiSettingsToServer( runtimeConfig, JSON.parse(snapshot) as ReturnType<typeof getAiSettingsTomlPayload>, ) .then(() => { + if (requestId !== latestSaveRequestId) return; lastSnapshot = snapshot; }) .catch((error) => { + if (requestId !== latestSaveRequestId) return; console.warn('Failed to save AI settings to SQLRooms config', error); toast.error('Failed to save AI settings', { id: AI_SETTINGS_SAVE_FAILED_TOAST_ID,🤖 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 `@apps/sqlrooms-cli-ui/src/store.ts` around lines 629 - 645, The autosave can apply out-of-order results: capture a monotonically increasing save request id when starting the save and only run the .then/.catch side effects if the finished request id matches the latest id; implement a counter (e.g., saveRequestCounter) incremented before calling saveAiSettingsToServer and store the current id in a local variable, then in the promise handlers check id === latestSaveRequestId before mutating lastSnapshot or showing toast.error (references: saveAiSettingsToServer, lastSnapshot, saveTimer, AI_SETTINGS_SAVE_FAILED_TOAST_ID).
🤖 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.
Outside diff comments:
In `@apps/sqlrooms-cli-ui/src/store.ts`:
- Around line 629-645: The autosave can apply out-of-order results: capture a
monotonically increasing save request id when starting the save and only run the
.then/.catch side effects if the finished request id matches the latest id;
implement a counter (e.g., saveRequestCounter) incremented before calling
saveAiSettingsToServer and store the current id in a local variable, then in the
promise handlers check id === latestSaveRequestId before mutating lastSnapshot
or showing toast.error (references: saveAiSettingsToServer, lastSnapshot,
saveTimer, AI_SETTINGS_SAVE_FAILED_TOAST_ID).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8b45aa3-6257-4719-8a96-9e62d16d1665
📒 Files selected for processing (1)
apps/sqlrooms-cli-ui/src/store.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/ai-core/src/AiSlice.ts`:
- Around line 398-403: When you repair an invalid baseConfig.currentSessionId
(in the block that checks sessionIdSet), also update baseConfig.openSessionTabs
so the repaired currentSessionId is present; either prepend/append the
reassigned id to openSessionTabs or reset openSessionTabs to
[baseConfig.currentSessionId] if it would otherwise be empty/invalid. Locate the
check that uses sessionIdSet and baseConfig.sessions and ensure after setting
baseConfig.currentSessionId you modify baseConfig.openSessionTabs to include
that id (reference symbols: baseConfig.currentSessionId, baseConfig.sessions,
sessionIdSet, baseConfig.openSessionTabs).
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fab2be8-7530-4d58-855f-77a2d4d2a44d
📒 Files selected for processing (15)
apps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsxapps/sqlrooms-cli-ui/src/components/AssistantPanel.tsxapps/sqlrooms-cli-ui/src/components/AssistantSettingsDialog.tsxapps/sqlrooms-cli-ui/src/store.tspackages/ai-core/README.mdpackages/ai-core/__tests__/AiSlice.modelSelection.test.tspackages/ai-core/src/AiSlice.tspackages/ai-core/src/components/InlineApiKeyInput.tsxpackages/ai-core/src/components/ModelSelector.tsxpackages/ai-core/src/components/QueryControls.tsxpackages/ai-core/src/components/SessionControls.tsxpackages/ai-core/src/components/session/DeleteSessionButton.tsxpackages/ai-settings/src/components/AiModelParameters.tsxpackages/ai-settings/src/components/AiModelsSettings.tsxpackages/ai-settings/src/components/AiProvidersSettings.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/ai-core/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/sqlrooms-cli-ui/src/store.ts
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
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 by CodeRabbit
New Features
Bug Fixes
Documentation