Skip to content

fix: prefixIds handles SMIL begin/end offsets and space-optional semicolons - #2288

Open
si-kui-a wants to merge 1 commit into
svg:mainfrom
si-kui-a:fix/prefixids-smil-offset
Open

fix: prefixIds handles SMIL begin/end offsets and space-optional semicolons#2288
si-kui-a wants to merge 1 commit into
svg:mainfrom
si-kui-a:fix/prefixids-smil-offset

Conversation

@si-kui-a

@si-kui-a si-kui-a commented Sep 3, 2026

Copy link
Copy Markdown

Fixes #2207.

Problem

Two related bugs in the "prefix begin/end attribute value" block of plugins/prefixIds.js:

  1. The suffix check val.endsWith('.end') || val.endsWith('.start') only matched a bare syncbase-value like b.end, not one with a clock-value offset like b.end-0.5s or a.begin+0.1s — both valid SMIL timing syntax. Any offset suffix made the whole check fail, silently leaving the id reference unprefixed and breaking the animation chain it pointed to. (Side note: this repo's only existing fixture for this code path, prefixIds.10.svg.txt, only exercises the .end half of the check — .start isn't valid SMIL syncbase syntax at all per MDN, which documents only id.begin/id.end, so that branch was untested, un-triggerable dead code from the start.)

  2. Separately — and this one only surfaced by actually running the issue's own reproduction end to end, not by reading the diff for fix 1 — the sibling split, attr.split(/\s*;\s+/) for the semicolon-separated list of timing values, requires at least one whitespace character after the semicolon to split at all. Valid SMIL timing lists don't require that whitespace (0;b.end-0.5s is valid, and is literally the reporter's own reproduction case). Without a space, the whole multi-value string never gets split, so it would still fail to prefix correctly even with fix 1 alone.

Fix

  1. Replaced the suffix check with a regex, /^([^.]+)\.(begin|end)([+-].+)?$/, that captures the id, the begin/end keyword, and an optional offset separately — the offset is preserved verbatim, only the id gets prefixed.
  2. Changed the split delimiter's trailing \s+ to \s*, making whitespace optional on both sides of the semicolon.

Testing

Added test/plugins/prefixIds.14.svg.txt, reproducing the exact scenario from #2207 (including the unspaced semicolon).

Ran the actual suite locally (not just hand-traced): pnpm vitest run — 521 passed, 3 skipped (includes multipass idempotence checking built into the fixture-test harness). Confirmed the new fixture genuinely fails against the pre-fix code by temporarily stashing the fix and re-running (it fails as expected, with the exact broken output described in the issue), then restored the fix and reconfirmed passing. pnpm lint (eslint + prettier) and pnpm typecheck (tsc) both clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HG5wiw3Z8QdjNWb5bFDLiA

…colons

Two related bugs in the same "prefix begin/end attribute value" block:

1. The suffix check `val.endsWith('.end') || val.endsWith('.start')` only
   matched a bare syncbase-value like "b.end", not one with a clock-value
   offset like "b.end-0.5s" or "a.begin+0.1s" -- both valid SMIL timing
   syntax. Any offset suffix made the whole check fail, silently leaving the
   id reference unprefixed and breaking the animation chain it pointed to.
   Also, this repo's only existing test/plugin fixture for this code path
   (prefixIds.10.svg.txt) only exercises the `.end` half of the check;
   `.start` is not valid SMIL syncbase syntax at all (MDN documents only
   `id.begin`/`id.end`) and was untested and un-triggerable dead code.
   Replaced the suffix check with a regex that captures the id, the
   begin/end keyword, and an optional offset separately, so the offset can
   be preserved verbatim while just the id gets prefixed.

2. Separately, `val.split('.')`'s sibling split -- `attr.split(/\s*;\s+/)`
   for the semicolon-separated list of timing values -- requires at least
   one whitespace character *after* the semicolon to split at all. Valid
   SMIL timing lists don't require that whitespace (e.g. "0;b.end-0.5s" is
   valid and is literally the reporter's own reproduction case in svg#2207).
   Without a space, the whole multi-value string was never split, so it hit
   the single-value regex as one unsplit blob and would not have matched
   correctly even after fix 1. Changed the trailing `\s+` to `\s*` so
   whitespace is optional on both sides of the semicolon, matching how
   values are actually written when SVGO didn't add the whitespace itself.

Found bug 2 by actually running the issue's own exact reproduction end to
end and checking the real output, not by reading the diff -- reading fix 1
alone looked complete, but running it against unspaced input surfaced the
still-broken case immediately.

Added test/plugins/prefixIds.14.svg.txt reproducing the exact scenario from
svg#2207 (including the unspaced semicolon). Verified for real: `pnpm vitest
run` (521 passed, 3 skipped, includes multipass idempotence via the fixture
harness), confirmed the new fixture genuinely fails against the pre-fix code
by stashing the fix and re-running, then restored and reconfirmed passing.
`pnpm lint` and `pnpm typecheck` both clean.

Fixes svg#2207.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HG5wiw3Z8QdjNWb5bFDLiA
@SethFalco

Copy link
Copy Markdown
Member

Hey @si-kui-a!

I'll let off these 2 PRs that you've opened, but going forward, could you please refrain from using an LLM to contribute to this project autonomously or communicate to on your behalf.

For better or for worse, none of the currently active maintainers afaik will raise a problem with someone using an LLM, including myself. However, to use an LLM to generate code that I'm not convinced you've even read, and then to generate communications is just disrespectful.

Especially for smaller PRs like this, reviewing your own code and doing a brief write up is the least you could do. Collaboration is a core pillar of OSS, so we welcome your contribution, but we expect to be collaborating with a human, perhaps even a human with a robot assistant, but not with the robot itself.

@si-kui-a

si-kui-a commented Sep 3, 2026

Copy link
Copy Markdown
Author

Hi @SethFalco !
Thank you very much for your feedback, and I sincerely appreciate your understanding and for merging those two PRs.

I want to be open about this:
I relied on an LLM to help phrase my messages because I lack confidence in my English and wanted to avoid misunderstandings.

However, you are absolutely right—over-relying on it while neglecting code review defeats the purpose of collaboration and goes against the spirit of open source.

I am deeply sorry if I made you feel disrespected; that was certainly never my intention.

Moving forward, I will ensure that I personally and rigorously review every line of code.

I will also do my best to express myself in my own words during communication, using such tools only for basic translation assistance.

Thank you again for your patience and for pointing this out to me.

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.

prefixIds does not correctly handle SMIL begin/end with offsets (e.g. ".end-0.5s")

2 participants