fix(studio): harden data-hv-text key validation in prompts and UI#80
fix(studio): harden data-hv-text key validation in prompts and UI#80walliai666 wants to merge 2 commits into
Conversation
Tighten all four AI generation prompts to explicitly prohibit using element text content as the key and prohibit leaving the attribute valueless — adds snake_case requirement and concrete key examples. In the studio UI, gracefully handle malformed data-hv-text attributes: synthesize a positional key (field_N) for valueless attributes and for keys that look like content text (>40 chars or contain CJK characters), and deduplicate colliding keys with a positional suffix.
lefarcen
added
size/S
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found one blocking issue in the new malformed data-hv-text recovery path. The sidebar now shows recovered fields for bad or duplicate keys, but edits to those recovered entries are not mapped back to the underlying DOM nodes when the HTML is written back.
| let key = el.getAttribute('data-hv-text') ?? ''; | ||
| // Valueless attribute → synthesize a positional key so the field still shows up. | ||
| if (!key) key = `field_${fields.length + 1}`; | ||
| // Key is the full content text (AI misuse) → warn and shorten to a safe slug. | ||
| if (key.length > 40 || /[一-鿿]/.test(key)) { | ||
| console.warn(`[hv-text] key looks like content text, falling back to positional key:`, key); | ||
| key = `field_${fields.length + 1}`; | ||
| } | ||
| if (seen.has(key)) key = `${key}_${fields.length + 1}`; | ||
| seen.add(key); | ||
| const text = el.textContent ?? ''; | ||
| fields.push({ key, original: text, current: text }); |
There was a problem hiding this comment.
For the malformed-key cases this PR is trying to recover, the synthesized sidebar keys (field_N and the suffixed duplicates) never exist in the HTML, so commitTextEdits() cannot find those nodes later with [data-hv-text="${cssEscape(f.key)}"]. That means the UI appears to support editing valueless attributes, overlong/CJK content-text keys, and duplicate keys, but saving silently drops those edits because the write path still looks up elements by the rewritten key string. You can see the mismatch in the changed lines here: refreshTextFields() rewrites the key before storing { key, original, current }, while the save path still queries by f.key. Please keep a stable reference to the original element as well, such as storing the node index / occurrence index together with the display key, or normalizing the DOM attribute itself before populating state.textFields, so the recovered field can be written back to the same element deterministically.
refreshTextFields() synthesizes display keys (field_N, suffixed duplicates) for valueless, overlong/CJK, and duplicate data-hv-text attributes. Previously, commitTextEdits() looked up nodes by the rewritten key string, which never matched the actual attribute value, so edits to those recovered fields were silently dropped on save. Store the DOM position (nodeIndex) alongside each field entry at read time, then locate the node by index in commitTextEdits() instead of querying by attribute value. This makes the write path independent of the display key, so all three malformed-key cases save correctly.
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
I found one remaining blocking correctness issue in the new data-hv-text recovery path: the write-back logic now depends entirely on node order, so a refreshed HTML document can still misapply sidebar edits even though malformed keys themselves are now recoverable.
| const allHvNodes = doc.querySelectorAll('[data-hv-text]'); | ||
| for (const f of state.textFields) { | ||
| const nodes = doc.querySelectorAll(`[data-hv-text="${cssEscape(f.key)}"]`); | ||
| nodes.forEach((n) => { n.textContent = f.current; }); | ||
| // Use the stable nodeIndex captured at read time so recovered/synthesized | ||
| // keys (field_N, suffixed duplicates) correctly map back to their DOM node. | ||
| const node = allHvNodes[f.nodeIndex]; | ||
| if (node) node.textContent = f.current; |
There was a problem hiding this comment.
commitTextEdits() now refetches the latest HTML and then reapplies every sidebar edit by allHvNodes[f.nodeIndex]. That fixes the earlier malformed-key bug, but it also means any insertion or reordering of [data-hv-text] nodes between refreshTextFields() and save will retarget existing edits to the wrong element. Before this patch, normal unique keys survived that because the write path matched by data-hv-text value instead of absolute position. Please keep enough identity to survive a fresh parse of the latest HTML, for example by storing the original attribute value plus an occurrence index among siblings with the same key, and using that tuple for normal keys while reserving the positional fallback only for synthesized recovery entries.
There was a problem hiding this comment.
Thanks for the thorough analysis. After tracing the call graph I believe
this race is not reachable in practice, and the fix-to-complexity ratio
doesn't justify the change.
Why the race window doesn't open in normal use
The only operations that can insert or reorder [data-hv-text] nodes are
AI generation and the server-side frame write. Both paths call
refreshTextFields() immediately after the HTML is written (line 2601 and
line 2226), which resets every nodeIndex before any sidebar input can
reach commitTextEdits(). The 500 ms debounce timer only starts on a
textarea input event, so by the time a user types a single character
the index is already up to date.
The commitInlineTextEdits path (iframe contenteditable) is a fully
separate code path that matches by attribute value and never touches
state.textFields, so it cannot cause cross-contamination.
Why the theoretical window is nearly impossible to hit
For a stale nodeIndex to misfire, all three of the following would have
to happen within the same 500 ms window:
- The user starts typing in the sidebar.
- An AI generation response arrives and reorders
[data-hv-text]nodes. - The debounce timer fires before
refreshTextFields()runs.
In practice AI generation always triggers refreshTextFields() before the
response is rendered, collapsing that window to zero.
Why the prior approach wasn't strictly safer
The attribute-value query used before this patch only survived node
reordering for well-formed unique keys. For the three recovery cases this
PR was written to fix — valueless attributes, overlong/CJK keys, and
duplicate keys — the old write path already produced zero matches and
silently dropped edits on every save, not just during a rare race.
Trading a theoretical edge case for a guaranteed everyday failure is the
wrong direction.
Given the above, I'd prefer to keep the current implementation and revisit
if a concrete reproduction is found.
3 participants
Tighten all four AI generation prompts to explicitly prohibit using element text content as the key and prohibit leaving the attribute valueless — adds snake_case requirement and concrete key examples.
In the studio UI, gracefully handle malformed data-hv-text attributes: synthesize a positional key (field_N) for valueless attributes and for keys that look like content text (>40 chars or contain CJK characters), and deduplicate colliding keys with a positional suffix.
Resolves #78
Resolves #79