[AksharP5]

What

Editing an inline style now preserves !important and browser fallback declarations. Undo and failed-batch rollback restore the edited properties' declaration sequences while retaining unrelated changes made afterward.

Why

Reducing an authored style to one value per property loses information. Changing opacity on color:red !important;color:blue could change the text color. Discarding display:block from display:-moz-box !important;display:block also changes Chromium's layout.

Undo must also keep shorthand and longhand positions when remote edits insert, remove, duplicate or move other declarations. Chromium 156 confirms these matching before/after cases; each result also survives reopening from overrides:

Authored style and remote change after replacing/removing the shorthand	Before correction	32841d191
opacity:1;background:red;background:green;background-color:blue; remove opacity, then undo	Green at 1aa2a406c	Blue
background-color:blue;background:red;background:green; prepend opacity:0.5, then undo	Blue at c4be136ba	Green
background-color:blue;opacity:1;background:red;background:green; prepend opacity:0.5, then undo	Blue at 8686fb30b	Green
background:red;opacity:1;background-color:blue;background-color:purple;background:green; remove background, insert opacity:0.5 after blue, then undo	Purple at f75eecc8d	Green
background-color:blue;opacity:1;background:red;background:green; move/update opacity before the longhand, then undo	Blue at 9d98dad4a	Green
margin-left:5px;opacity:1;margin:10px;margin:20px; move/update opacity before the longhand, then undo	5px at 9d98dad4a	20px
background-color:blue;background:red;background:green;opacity:1; move/update trailing opacity to the front, then undo	Blue at e28859236	Green
margin-left:5px;margin:10px;margin:20px;opacity:1; move/update trailing opacity to the front, then undo	5px at e28859236	20px
background-color:blue;background:red;background:green;opacity:1; move opacity to the front without changing its value	Blue at dd9bd4269	Green
margin-left:5px;margin:10px;margin:20px;opacity:1; move opacity to the front without changing its value	5px at dd9bd4269	20px

Additional cases cover unrelated declaration reinsertion, excess duplicates, and a new longhand before the edited property.

Related work

Closes #6267. The original priority-scanning and fallback-preservation review threads are resolved.

How

Keep ordered declarations for serialization and select values by priority for reads. A forward comment scanner handles priority markers without regex backtracking. Explicit edits replace or remove all declarations of that property.

Selective undo counts remaining occurrences and uses surviving untouched properties to guide duplicate matching. Scalar inverses can append edited properties, so those properties do not block matching untouched declarations. Edited properties still provide placement boundaries for added declarations. Adjacent anchors from the same original interval between restored declarations are grouped, allowing untouched declarations to reorder within that interval. Anchors cannot be combined across a restored declaration. Anchors outside a following unchanged declaration's original interval are excluded even if their values stayed the same, so a moved declaration cannot strand a surviving longhand after the restored shorthand. The traversal remains linear and preserves both the selected original declarations and current untouched declarations in order. Dispatch and replay share override-recording logic.

Events containing selective style inverses use formatVersion: 2; ordinary events remain version 1. Custom history integrations must check each event's version, preserve restoreStyleProperties, and replay through the SDK's applyPatches(). Generic RFC 6902 appliers do not implement selective restoration. The package docs, integration guide and type reference document this contract.

Suggested squash/release note:

BREAKING CHANGE: Patch events may use formatVersion: 2. Integrations must accept version 2, check the version on every event, preserve restoreStyleProperties, and replay these operations through the SDK's applyPatches(). Saved overrides need no migration.

Compatibility review follow-up

At 959d58388e55e486ab00606ed6280d5d7bcc63f0, added two persistence controls in session.dispatch.test.ts without changing production code:

• A legacy record containing only the two version-1 patch arrays replays with its original whole-style replacement semantics and reopens from unchanged override JSON.
• A host stores events in the sequence 1 → 2 → 1, including the full selective inverse. After JSON storage and reopening, host-owned undo restores both display fallbacks and keeps a later remote opacity change. Redo and reopening also preserve the result. Replay emits ordinary version-1 resolved patches.

The integration guide now stores formatVersion, derives the history entry type from PatchEvent, checks each event and stored record, and explains the upgrade order. Records known to come from version-1-only SDKs remain replayable; an older host schema may label those known records as version 1. New or unknown unversioned records must not be assumed to be version 1. Saved override objects need no format migration.

Maintainer decision still required: accept the public version-2 contract and the breaking release/migration note above. Version-1-only consumers must upgrade before adopting this SDK. Rejecting an event in a listener does not roll back the mutation already committed. Generic RFC 6902 appliers do not supply selective replay semantics.

Test plan

fails without the fix: packages/sdk/src/session.test.ts, the background and margin cases in restores $property fallbacks after untouched declarations are reordered with opacity held at 1. Both new parameter rows fail at dd9bd4269, placing the restored shorthand before its longhand, and pass at 32841d191.

• All 754 SDK tests pass, including replacement/removal, duplicate and inserted declarations, deletion/reinsertion, history, rollback, and replay coverage.
• Built-SDK Chromium 156 verification passes 80 observations spanning original states, undo, redo/undo, JSON version-2 inverse replay and reopening from overrides. The 16 wrong current-head lifecycle results from the review are corrected; value-changing controls still pass.
• SDK rebuild, normal pre-commit typechecks, oxlint, oxfmt, Fallow, comment checks and commitlint pass.
• Persisted version-1 and mixed version-1/version-2 host history tests pass. The integration guide documents consumer migration and retains the version in stored records. The GitHub-signed commit's complete tree matches the tested index.

The branch includes main 3b2058a2b and upstream checkout recovery #6577. The full workspace build previously passed on this base; the SDK was rebuilt for this correction. No fresh Windows or full-workspace test run was performed locally. Traversal remains linear; no new throughput benchmark is claimed.

GPT-6 Astra high audited dd9bd4269 and found that an unrelated declaration moved without changing value could still cross the restoration interval. This is corrected at 32841d191f32de56b6f486d5fce8a8606e03bd30. The correction was verified with the matching failed baseline, full SDK suite and browser lifecycle controls; the reviewer has not re-reviewed this new commit. Jrusso subsequently reviewed 32841d1, passed 259 focused tests and requested an explicit compatibility decision and persistence coverage. The coverage and guide follow-up are published at 959d58388e55e486ab00606ed6280d5d7bcc63f0; production files are unchanged from 32841d1. All 12 required checks pass on 959d583, with no failed or pending checks. The maintainer compatibility decision remains pending.

[mintlify]

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit 959d583.

Approve preview

[miguel-heygen]

Reviewed at c8a48aba.

The approach is right. parseStyleAttr is the one place the SDK turns an inline style into the map that setStyle writes back, so choosing by importance there fixes edits, persistence, reopening and undo together. Skipping top-level comments in the ; splitter is a good side fix too. I ran the SDK suite at this head (701 pass), and with the importance check removed, the seven new tests fail, so they catch the bug. I also reproduced the issue with the published 0.8.194.

One change is needed before merge: the CodeQL warning on packages/sdk/src/engine/model.ts:299 is real. Both \/\*[\s\S]*?\*\/ alternatives sit inside a repeated group, so a run like /**//**//**/ can be split into comments in exponentially many ways when the match fails. I tried a style such as color: red !important; color: blue !/**//**/…/**/ a b through getElementStyles: each extra *//* roughly doubles the time, so an attribute under 200 characters blocks the thread for minutes. Since this runs on every style read of authored HTML, one odd attribute can hang an editing session.

A comment pattern that cannot overlap fixes it: \/\*[^*]*\*+(?:[^/*][^*]*\*+)*\/, used in both places. With that swap, mutate.test.ts and session.test.ts still pass, and an input with tens of thousands of repeats returns right away with the correct result. That should also clear the CodeQL check. A test row with a long /**//**/ run would keep it that way.

Test (runtime) was cancelled at its 10-minute limit rather than failing a test, so it only needs a re-run.

Verdict: REQUEST_CHANGES
Reasoning: The fix and tests are right, but the new priority regex backtracks exponentially on authored styles; a one-pattern swap fixes it.

[AksharP5]

Latest follow-up: 959d58388e55e486ab00606ed6280d5d7bcc63f0 adds the persisted-history coverage and migration guide requested in Jrusso's review of 32841d1. SDK production files and version-2 semantics are unchanged from that head.

All 754 SDK tests pass, including legacy version-1 whole-style records and JSON-stored mixed 1 → 2 → 1 host history through undo, redo and override reopening. The SDK build, normal hooks, lint/format, Fallow, commitlint and staged/published comment gates pass. The GitHub-verified commit's complete tree matches the tested snapshot.

The guide now retains formatVersion in stored records, checks every event/replay record and states the consumer upgrade order. Maintainers still need to accept the breaking format-2 contract; Jrusso left COMMENT for that decision, and the earlier change request remains active. All 12 required checks pass on 959d583, with no failed or pending checks. The branch is conflict-free; the maintainer compatibility decision remains pending. No merge performed.

Prior runtime validation remains at 32841d1: 80 muted Chromium observations passed for the unchanged production files, covering undo, redo/undo, JSON inverse replay and override reopening. This follow-up adds no new browser, native Windows or full-workspace validation. Mintlify is unavailable locally.

[jrusso1020]

The forward scanner fixes Miguel's ReDoS finding, thanks. One thing left: editing an unrelated property now drops a valid fallback declaration, which changes how Chromium renders the element. Details inline. An edit, serialize, reopen regression test would pin it.

[miguel-heygen]

Reviewed at bbf98d6a.

Thanks for the quick turnaround. Both earlier requests are addressed.

The comment handling no longer uses a repeated regex. The forward scans in splitStyleAtTopLevel and skipStyleTrivia stay linear: an 80k-character !/* + *//* run returns in about 25 ms with the right result, and the other long inputs I tried (unterminated comments, runs of !, long escape runs, 20,000 declarations) all stay in the low milliseconds. CodeQL is clean at this head.

setElementStyles now rewrites only the properties being edited and keeps every other declaration in order, so display:-moz-box !important;display:block keeps its fallback through an unrelated edit, save and reopen. The whole-style inverse in handleSetStyle also makes undo, batch rollback and reopening from the overrides restore the exact original string when the edited property itself had several declarations. I checked that with the display, color !important, gradient fallback and shorthand/longhand cases.

I ran the SDK suite here (731 pass) and broke each part of the fix in turn: with the importance check removed, the old setElementStyles restored, or the whole-style inverse removed, the matching new tests fail. Strings, url() and var() that contain !important are correctly not treated as a priority.

One optional follow-up that already happens on main: an explicit edit puts the new value where the property's first declaration was. So with background:red;background-color:blue;background:green, setting background leaves background-color: blue after it and the element renders blue. Writing it where the last declaration was would match the cascade.

Verdict: APPROVE
Reasoning: The regex blocker and the fallback-loss issue are both fixed and covered by tests that fail without the fix, undo is now exact, and CI is green at this head.

[miguel-heygen]

Re-reviewed at 1aa2a406.

Good catch on the whole-style inverse. At the previous head, undoing a fallback edit also rolled back unrelated changes made after it, and this commit fixes that properly. I checked it beyond the new tests:

• local display edit, remote opacity, local display: null, two undos: both display declarations are back, opacity stays, and reopening from the overrides gives the same HTML
• background:red;background-color:blue;background:green: undo after a later unrelated edit restores the sequence in order
• hyphenated keys ("background-color") and a throwing batch() with a fallback edit followed by an unrelated edit both restore exactly
• a version-2 inverse that goes through JSON and is replayed with applyPatches() keeps later remote edits, and the re-emitted event is a plain version-1 style replace

Full SDK suite passes here (734). Each new piece is covered: if applyPatches ignores restoreStyleProperties, writes overrides from the raw patches, drops newly added properties, or always emits version 1, a new test fails.

On the !: I agree it is a breaking change. The old docs said a formatVersion bump is breaking, and hosts that followed the old sample (if (formatVersion !== 1) throw) will now throw from inside setStyle when the edited property has repeated declarations. The type also widens from 1 to 1 | 2. Saved overrides are unaffected, and the reference, guide and package docs are updated. Two optional suggestions:

1. The release notes take this entry from the PR title, which reads like a plain fix. It would help to add a line to the squash message saying what integrators must change: accept version 2, check the version on every event instead of once at startup, keep restoreStyleProperties, and replay through applyPatches().
2. For maintainers to decide: RFC 6902 says appliers ignore members they don't recognise, so the field could also ride on version 1. Old hosts would then fall back to restoring the whole style instead of throwing. Either way works; it depends on which failure mode you prefer for older integrations.

Verdict: APPROVE
Reasoning: The new commit fixes a real undo regression with tests that fail without it, the breaking marker matches the SDK's documented version contract and is documented, saved data needs no migration, and CI is green at this head.

[AksharP5]

Follow-up to Jrusso's current-head compatibility review: published the persistence coverage and integration guide update at 959d58388e55e486ab00606ed6280d5d7bcc63f0.

session.dispatch.test.ts now covers a legacy history record containing only the two version-1 patch arrays, retaining whole-style replacement semantics and reopening from existing override JSON. A second test stores the emitted version sequence 1 → 2 → 1 as JSON, closes/reopens the composition and performs host-owned undo/redo. Both original display fallbacks return, a later remote opacity edit survives, selective fields remain intact, replay emits resolved version-1 patches, and saved overrides reopen without conversion.

The guide stores formatVersion in every new record, derives the host entry type from PatchEvent, checks each event and replay record, and explains how to label only known version-1 legacy records. All 754 SDK tests, the SDK build and normal commit/comment gates pass. This revision changes tests and documentation only.

The maintainer decision is still open: accept version 2 as a breaking public contract with the proposed release note, and require consumers/replay SDKs to upgrade before new producers emit it. A version-1-only listener is not compatible with every new event, and rejecting an event does not roll back its already-committed SDK mutation. Saved override objects keep their existing format; old records cannot recover declarations already lost before they were stored.

[jrusso1020]

Reviewed at 32841d191f32de56b6f486d5fce8a8606e03bd30.

The focused current-source mutate/session/dispatch suites pass 259 tests, and the earlier untouched-style-loss fix is present. I am leaving this at COMMENT because emitting formatVersion 2 for selective restore inverses changes the public persistence contract. Before a stamp, I want a maintainer decision on that migration plus coverage showing how existing persisted patches and host consumers handle both versions. I did not rerun the full authored SDK suite or a browser edit/undo round trip. This is a compatibility decision, not a newly reproduced data-loss claim.

Need: yes · Size: ok · Reuse: existing shared implementation · Simpler: avoid parallel paths · Structure: fine
Risk: high - public persisted inverse format migration.

Verdict: COMMENT
Reasoning: The next action above remains before a new approval.

Reasoning: The focused current-source mutate/session/dispatch suites pass 259 tests, and the earlier untouched-style-loss fix is present. I am leaving this at COMMENT because emitting formatVersion 2 for selective restore inverses changes the public persistence contract. Before a stamp, I want a maintainer decision on that migration plus coverage showing how existing persisted patches and host consumers handle both versions. I did not rerun the full authored SDK suite or a browser edit/undo round trip. This is a compatibility decision, not a newly reproduced data-loss claim.