Repository navigation
Conversation
EngineConfigPage seeds its form once at load and PUTs the whole engine sub-object from that copy. The server replaces the sub-object row, so a save writes back every field the page loaded, including ones it does not show. The new tests reproduce four results of that: - Aggressive: thresholds, tool strategies, and the summarizer switch that the settings tab saved after the page loaded go back to the page's copy. - Ultra, default install: every save returns 400. The form sends modelPath "" and the route requires a non-empty string. - Ultra: a save drops ultra.enabled, because the page strips it from the body and the server replaces the row. - Ultra: emptying the model path returns 400; it should clear the path. Two more cases pin the fixed behavior: a save whose fresh read fails writes nothing, and a second save does not resend the first one's field. The UI test runs the page against a fake route that applies the real update schema. The node test runs the same flows through the real settings module and imports the save helper the fix adds, so it fails on this commit with a missing-module error.
The Ultra page's Preview button sends the whole form as config.ultra, including the empty model path the form shows by default. The preview route checks that config against the same strict schema as a save, so the preview returns 400 and the page shows "Preview failed." on every install that has no model path set. The UI test clicks Preview on a default install against a fake route that applies the preview schema. The node test checks a preview config built from the seeded form with that schema; it needs the helper the fix adds.
EngineConfigPage read the settings once at load and PUT the whole engine sub-object from that copy on every save. The server replaces the row, so a save wrote back every field the page loaded: - On the Aggressive page, thresholds, tool strategies, and the summarizer switch saved from the settings tab after the page loaded went back to the page's copy. - On the Ultra page, the form's empty model path failed the route's non-empty check, so every save on a default install returned 400. Once a path was set, a save dropped ultra.enabled, which the page strips from the body. A save now reads the stored sub-object again and applies only the fields edited since the page loaded or last saved. enabled is never taken from the form, and an emptied text field removes its key, so clearing the model path clears it. If that read fails, the save fails and writes nothing. Lite keeps its own body, since its server write already merges with the stored row. Preview had the same empty model path problem: the preview route checks config against the same schema, so an Ultra preview returned 400 on a default install. Preview now drops emptied text fields. The seeding and body logic moves to engineConfigSave.ts so node:test can cover it next to the UI test.
Build the form defaults with Object.fromEntries, and use configState directly in handleSave in place of a local alias.
After a save, the engine page kept showing the values it loaded, so a field another page changed could not be set back to its old value. A save whose response was lost left the baseline unchanged, so rolling that field back was never sent. The lite branch wrote back its load-time switch. A failed settings read at load left the form showing defaults as if they were saved. The UI tests reproduce each case through the page. The node tests drive the save helpers against the real settings store, so release PRs gate on them.
After a successful save the page re-seeds the form and its baseline from the PUT response, keeping any edit typed while the request was out. After a failed save the baseline drops the fields that save carried, since the server may have applied it, so the next save sends them again. Lite builds its body the same way as the other engines: the row stored at save time plus the fields edited since the last save. The cap is checked against its range before anything is sent, and a cleared cap sends null. When the settings read fails at load, the page shows the load error and keeps Save off, because the form would otherwise show defaults as saved values.
…ne-config-page-partial-save
A rejected PUT must keep the edit for the next save, a PUT the server applied before answering 500 must be sent again, a settings read that fails after a reload must not disable the reloaded page, and a keystroke landing as a save finishes must not revert the other fields. The form tests now expect per-field change events, and the node suite round-trips every engine sub-object through the update schema.
The form used to hand the page a whole rebuilt object spread from its props, so a keystroke landing while a save finished could revert fields to load-time values and the next save wrote them back. It now reports single field changes and the page applies them to its latest state. A failed read of the stored settings now gets its own message instead of the engine-information one, is set inside the load's cancelled guard so a superseded load cannot disable Save, and the save-time re-read tolerates a rejected GET like the load-time one already did.
6 tasks done
Owner
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
2 participants
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.
Problem
EngineConfigPageholds the detail configuration of the Lite, Aggressive, Ultra, Headroom, Session Dedup and CCR compression engines. On save it PUTs{ [subKey]: detail }wheredetailwas the form state seeded once at load. The server (updateCompressionSettingsinsrc/lib/db/compression.ts) replaces each sub-object row whole, and onlylitemerges, so every save wrote back the whole load-time copy. Three user-visible defects came out of that:modelPath: ""while the schema requiresz.string().trim().min(1).optional().ultra.enabled, because the form never shows that field.Fix
Client-side only;
updateCompressionSettingsis untouched (the server-side merge belongs tofix/compression-nested-partial-writes):engineConfigSave.ts::buildEngineDetailUpdate).enabledis never written from the form; the stored value passes through.EngineConfigForm.tsx).Testing (TDD, Hard Rule #18)
Tests first, seen failing, then the fix:
tests/unit/ui/engine-config-page-partial-save.test.tsx(vitest, jsdom): 16 tests covering the revert, the default-install 400,enabled, the cleared path, the failed re-read, retries after a rejected save and after a save the server applied but answered 500, and the keystroke race. The pass-2 tests ran red against2bcf1d8e9a(6 failures across the two UI files) and green afterea900baf14.tests/unit/compression/engine-config-save.test.ts(node:test against the realgetCompressionSettings/updateCompressionSettingson a temp DATA_DIR): 14 tests, including a round-trip of every engine sub-object through the strict update schema.Full gates on
ea900baf14:npm run test:vitest493/493,npm run typecheck:coreclean,npm run lintclean, i18n UI keys coverage PASS across 66 locales.CI note: the vitest
.tsxspecs run in thetest-vitestjob on the way to main; the node suite is the one that gates PRs intorelease/**.Browser QA
Six probes on an isolated dev server (fresh DATA_DIR, port 20198, gstack headless Chromium): aggressive cross-page preservation, ultra default-install save, model-path clear, llmlingua no-Save, lite toggle persistence, and the settings tab render. All pass. Evidence lives in the session's
.gstack/qa-reports/run-20261005T220000Z/(not committed).Review
gstack-review ran the specialist passes (security, performance, design and simplification clean; testing and maintainability findings addressed in the last two commits) plus adversarial and red-team passes. Findings fixed here: the settings-load error state (message, cancelled guard), per-field form changes (keystroke race), save retries (rejected, and applied-but-500), comment accuracy. Advisory and out of scope: the client-side read-then-write window (the server-side merge is the other branch's work), the dead per-engine "Preserve system prompt" checkbox, emptied number fields saving as 0, previews ignoring the sent config, and the pre-existing legacy-row read priority. Follow-up tasks are recorded for each.
Notes for reviewers
en.jsonstring; other locales fall back to English.changelog.d/fixes/fragment follows once this PR has a number.