Skip to content

fix(compression): merge engines writes by id instead of replacing the row - #15613

Merged
diegosouzapw merged 9 commits into
diegosouzapw:release/v3.8.52from
woodsonl:fix/compression-engines-partial-write
Oct 6, 2026
Merged

diegosouzapw merged 9 commits into
diegosouzapw:release/v3.8.52from
woodsonl:fix/compression-engines-partial-write

Conversation

@woodsonl

@woodsonl woodsonl commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • PUT /api/settings/compression stored the engines map it received, whole. The two dashboard pages that toggle compression engines (/dashboard/context/omniglyph and /dashboard/context/settings) each kept their own copy of the map from page load and sent it back complete, so a toggle made on one page was overwritten by the next toggle on the other page, or by the next toggle after a failed settings GET, which silently rebuilt the map from defaults. The server now merges each written engine entry over the stored entry by engine id, field by field. While no engines row is stored yet, the merge base is the map the read path derives from the legacy per-engine settings.
  • The two pages now send only the engine they changed. The Omniglyph page's save path was rewritten around one save(body, rollback) helper with a generation counter, so the 2-second "Saved." timer of an earlier save can no longer clear a later save's error, and a failed save (non-ok response or rejected fetch) rolls the toggle back.
  • docs/openapi.yaml now documents the PUT's engines body property and the merge semantics; the requestBody never listed engines.
  • A non-text (BLOB) engines row in key_value is treated as absent by the write path, matching the read path.

Related Issues

Validation

  • Change type: UI + settings write path.
  • Red first: on e86a139 (the last test-only commit) the 4 new server cases fail 0/4, and the UI suites fail 6/8 (the 2 passes are pre-existing cases). On this tip the same suites run 10/10 and 14/14.
  • Green: node --test on engines-partial-write.test.ts + context-combos-default-route.test.ts: 10/10. npx vitest run on omniglyphContextPage.test.tsx, compressionPanelEnginesPartialWrite.test.tsx, compressionPanel.test.tsx: 14/14.
  • npx prettier --check on the changed files is clean.
  • Production-code changes include new automated tests in this PR.

Tests Added Or Updated

  • tests/unit/compression/engines-partial-write.test.ts (5 cases): stored engines survive a single-engine write; a toggle made on one page survives the other page's write; a stored level survives a write that only sets enabled; the first engines write merges over the legacy-derived map; a BLOB engines row is treated as absent.
  • tests/unit/ui/omniglyphContextPage.test.tsx (7 cases): enabling sends only { engines: { omniglyph: { enabled: true } } }; disabling sends only the off entry; a rejected fetch rolls the switch back and shows the error; a timer left by an earlier save does not clear a later save's error; the profile select PATCHes only the profile.
  • tests/unit/ui/compressionPanelEnginesPartialWrite.test.tsx (3 cases): single-engine PUTs from the panel, two engines in turn, and a level change that carries the current switch state.

Reviewer Notes

  • ⚠️ base-red inherited: 🔴 Release branch not green: release/v3.8.52 #15306
  • Review evidence: Codex adversarial pass returned clean with a recommendation to approve. A five-specialist review (testing, maintainability, api-contract, simplification, adversarial) plus a red-team pass, with two independent skeptics per finding, confirmed 4 informational findings (all addressed in 60d1fa2 and 2601f4a: the openapi documentation, the BLOB-guard test, the off-toggle test, the rejected-fetch test) and refuted 2. One style advisory was declined with its reason recorded on the branch.
  • Semantics note: PUT { engines: {} } used to store an explicit all-off map; it now changes nothing, because nothing is merged over. No caller in src/, open-sse/ or bin/ sends an empty or wholesale map; the MCP engine tool rebuilds the full map from a fresh read and is unaffected.

updateCompressionSettings stores whatever engines map it receives as
the whole row, and the read path turns every id missing from that row
off. These tests fail on the current base: a write carrying one engine
turns the other stored engines off, a panel toggle after an Omniglyph
toggle turns Omniglyph back off, a write that sets only enabled drops
the stored level, and the first engines write on an install with no
engines row drops the engines derived from legacy settings.
The Omniglyph page and the context settings panel PUT the whole engines
map from the copy each loaded, so a toggle on one page overwrites what
the other changed after it loaded. These tests expect each page to send
only the engine it changed. The Omniglyph tests also expect a failed
save to put the switch back, and the saved-status timer of an earlier
save to leave a later save's error on screen. All fail on the current
base.
… row

updateCompressionSettings stored the engines map it received as the whole
row, and the read path turns every id missing from that row off. The
Omniglyph page and the context settings panel each PUT the full map from
the copy they loaded, so a toggle on one page reverted engine changes the
other page made after it loaded, and a toggle after a failed GET stored a
map holding one engine and turned every other engine off.

The write path now merges each written entry over the stored entry, field
by field. While no engines row exists it merges over the map the read path
derives from legacy settings, the map the dashboard shows. Both pages send
only the engine they changed. The Omniglyph page also puts the switch back
when a save fails, and its saved-status timer clears the status only when
no later save has started.
Two default-route tests wrote only the engines they wanted on and relied
on the write turning every other engine off. Writes now merge over the
engines the install already has, so these tests write every engine id
with the rest set off.
The requestBody schema never listed `engines`, and nothing said the PUT
merges per engine id instead of replacing the stored map. Document the
entry shape (enabled required, level optional and kept when omitted) and
the legacy-derived fallback base used while no engines row is stored.
…cted saves

Review findings: the write-path guard that treats a non-text engines row
as absent had no case seeding one, the rewritten Omniglyph toggle was
only exercised turning on, and a rejected fetch (as opposed to a non-ok
response) was never saved. Each case pins existing behavior.
@woodsonl
woodsonl requested a review from diegosouzapw as a code owner October 6, 2026 00:54
@diegosouzapw
diegosouzapw merged commit e1e9230 into diegosouzapw:release/v3.8.52 Oct 6, 2026
42 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants