Repository navigation
Conversation
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
c8a48ab to
9746e88
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
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.
|
The review concerns are addressed in
All 708 SDK tests and the independent correctness/scaling review pass. No further source correction was needed in this audit. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
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.
| result[toCamel(rawProp)] = value; | ||
| const prop = toCamel(rawProp); | ||
| const isImportant = hasImportant(value); | ||
| if (important.has(prop) && !isImportant) continue; |
There was a problem hiding this comment.
With <span style="display:-moz-box !important;display:block">hello</span>, Chrome uses display:block. After setStyle(id, {opacity:"0.5"}) this serializes only display:-moz-box !important, so the span becomes inline. The current SDK keeps block. Please keep fallback declarations when serializing.
Changing only opacity could change red text to blue when an inline style had
color:red !important;color:blue. The SDK kept the last declaration, then rewrote that lossy style map. Saving, reopening and undoing the opacity edit kept the wrong color.Choose repeated declarations by importance, then source order, while preserving the selected spelling. Scan comments and CSS whitespace forward, and recognize each declaration's priority once. Close an unfinished trailing comment before appending another property so the new edit takes effect. Explicit color replacement/removal and undo retain their existing behavior.
Same HTML and public SDK workflow, computed color in Chrome:
Priority comments are consumed by
skipStyleTrivia, which advances past each closing*/; there is no repeated group of comment-pattern alternatives. Regression cases cover 4,096 adjacent comments for both CodeQL inputs. Priority recognition also avoids rescanning a long winning value for every losing duplicate. The valid 100 KB duplicate-declaration fixture took 4.10 seconds forsetStyle()during independent review; the corrected complete edit/save/reopen workflow takes about 15–26 ms locally under Bun and Node. Timing is measured separately from the regression tests.Validation: all 708 SDK tests, fresh full workspace build, SDK/parser typechecks, browser edit/history/persistence checks, lint/format/comments and normal signed-commit hooks pass. Controls cover comments, EOF comments, CSS escapes, whitespace, strings and explicit replacement. The actual integrity-checked published SDK 0.8.194 reproduces the original color loss under Node with compatible existing dependencies.
CI's packer produces parser and SDK archives of 191,703 and 88,933 bytes; both pass the unchanged production budgets. No dependency, workflow, suppression or size-budget change is included. Host CodeQL, a fresh
Test (runtime), aggregateTestand all 12 required checks pass on9746e8862.Independent review of
9746e8862found no blockers. It independently passed all 708 SDK tests, public API/history/browser controls, scaling checks and archive verification without changing source or build outputs.SDK examples retain 19 previously verified baseline type errors. Mintlify validation was unavailable because
mintis absent. No consumer installation, Studio shell session or full video export was run.Closes #6267