Skip to content

Make translate:sync-hash idempotent - #1360

Merged
comfyui-wiki merged 1 commit into
mainfrom
fix/sync-hash-i18n-idempotent
Aug 8, 2026
Merged

Make translate:sync-hash idempotent#1360
comfyui-wiki merged 1 commit into
mainfrom
fix/sync-hash-i18n-idempotent

Conversation

@christian-byrne

@christian-byrne christian-byrne commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #1358.

The bug

syncChunkedHashes reassembles each translated file as:

const { frontmatter, body } = parseFrontmatterAndBody(targetContent);
const bodyText = body.endsWith("\n") ? body : `${body}\n`;
const raw = `${frontmatter}\n${bodyText}`;

parseFrontmatterAndBody already appends the separating newline to frontmatter and consumes the original from body:

const match = content.match(/^(---\n[\s\S]*?\n---)\n?([\s\S]*)$/);
return { frontmatter: `${match[1]}\n`, body: match[2] };

So frontmatter + body reconstructs the file exactly, and the extra \n adds a blank line after the closing --- on every pass.

The blank line lives in the body, so it changes no hash. That is why the tool kept reporting drift on files whose recorded hashes were already correct: output === targetContent in syncOneFile could never hold, so the unchanged branch was unreachable for every chunked file.

Verified

Repeated full runs on a clean main, before:

run reported cumulative diff
1 71 updated 253 insertions
2 17 updated 270 insertions
3 17 updated 287 insertions
4 17 updated 304 insertions
5 17 updated 321 insertions

Unbounded — 17 files never converged.

After:

run reported cumulative diff
1 60 updated 203 insertions
2 0 updated 203 insertions
3 0 updated 203 insertions

--dry-run afterwards prints Nothing to sync. Hashes already match English source.

The 71 → 60 drop is the false positives disappearing; the remaining 60 are genuine English-source drift, which this PR does not write. The reproduction from the issue now reports the opposite of what it used to:

$ bun .github/scripts/i18n/sync-hash-i18n.ts --dry-run tutorials/image/qwen/qwen-image-layered.mdx
  [ja] already in sync: ja/tutorials/image/qwen/qwen-image-layered.mdx
  [zh] already in sync: zh/tutorials/image/qwen/qwen-image-layered.mdx
  [ko] already in sync: ko/tutorials/image/qwen/qwen-image-layered.mdx
Done: 0 updated, 3 already in sync, 0 missing target(s)

A second instance of the same defect, found by the test

Writing the regression test surfaced a second non-idempotent path. When the frontmatter contains nothing but translation metadata, stripping it leaves cleaned empty and ${open}${cleaned}\n${metaBlock} emits a blank first line inside the frontmatter:

run 0: "---\ntranslationSourceHash: aaaaaaaa\n…"
run 1: "---\n\ntranslationSourceHash: aaaaaaaa\n…"   ← gained a line
run 2: "---\n\ntranslationSourceHash: aaaaaaaa\n…"   ← stable from here

Bounded rather than unbounded, but still not idempotent, and it is live on zh/custom-nodes/workflow_templates.mdx. Fixed once as a shared frontmatterMetaPrefix helper and applied at all three call sites — chunked-translate.ts, sync-hash-i18n.ts, and translate-i18n.ts, which carried the identical code.

Regression test

sync-hash-i18n.test.ts, 11 tests. The load-bearing ones:

  • running the sync twice is the same as running it once — the invariant the issue asks for.
  • stays stable over repeated passes — five passes, byte-identical.
  • does not insert a blank line after the frontmatter — fails on the old code at pass 1.
  • leaves the body byte-for-byte untouched.
  • preserves blank lines a previous buggy run already committed — the fix must not add more, and must not reflow prose a human may have edited.
  • handles frontmatter that holds nothing but translation metadata — the second bug above.
  • computeSyncedContent reaches the unchanged branch — the branch that was dead.

To make the module importable without executing a repo-wide sync, main() is now behind import.meta.main. Running bun .github/scripts/i18n/sync-hash-i18n.ts is unchanged.

Nothing ran the tests

.github/scripts/i18n/ already shipped chunked-translate.test.ts and repair-fences.test.ts, and no workflow invoked them — which is the reason this was invisible to CI, and a regression test added without fixing that would have been decoration. i18n-scripts-test.yml runs bun test ./.github/scripts/i18n/ on changes under that directory. All 31 tests (20 pre-existing + 11 new) pass.

The blank lines already in the history — deliberately a separate PR

526 translated .mdx files carry blank lines this tool added. Attributed against d56a77bd^, the commit before the tool existed:

files
translated files with ≥2 blank lines after frontmatter today 542
grew since the tool landed 526
unchanged since before the tool existed (legitimate) 4
created after the tool landed 12

Growth pattern: 498 went 1 → 2, 18 went 1 → 3, 6 went 1 → 4, 3 went 2 → 3, 1 went 0 → 2. Worst case is 9 blank lines (zh/tutorials/video/minimax/minimax-h3.mdx). 6a4aa997 (#1228) is one of the commits that added them.

That cleanup is a mechanical whitespace diff across 526 files. Folding it in here would bury a four-line logic fix under an unreviewable diff, and it is only durable once this PR lands — before that, the next translate:sync-hash run puts the lines straight back. It is opened as a stacked follow-up rather than deferred: #1361

syncChunkedHashes reassembled each file as `${frontmatter}\n${bodyText}`, but
parseFrontmatterAndBody had already appended the separating newline to
`frontmatter` and consumed the original. Every pass therefore gained one blank
line after the closing `---`.

The blank line sits in the body, so it changes no hash — which is why the tool
kept reporting drift on files whose recorded hashes were already correct. It
also meant `output === targetContent` in syncOneFile could never hold, making
the `unchanged` branch unreachable for chunked files and `--dry-run` useless as
a drift signal: on a clean main it reported 71 of 84 files as needing a sync,
and 17 of those never converged no matter how many times it ran.

Before (clean main, repeated full runs):

    run 1: 71 updated   253 insertions
    run 2: 17 updated   270 insertions
    run 3: 17 updated   287 insertions
    run 4: 17 updated   304 insertions

After:

    run 1: 60 updated   203 insertions   (genuine hash drift, not written here)
    run 2:  0 updated   203 insertions
    run 3:  0 updated   203 insertions
    dry-run: "Nothing to sync. Hashes already match English source."

The 71 -> 60 drop is the false positives disappearing; the reproduction from the
issue (tutorials/image/qwen/qwen-image-layered.mdx) now reports "already in
sync" for all three languages instead of "would sync hash".

The regression test found a second instance of the same defect: when the
frontmatter contains nothing but translation metadata, stripping it leaves an
empty body and `${open}${cleaned}\n${metaBlock}` emitted a blank first line
inside the frontmatter that no later run could clear. That is bounded rather
than unbounded, but still non-idempotent, and it is live on
zh/custom-nodes/workflow_templates.mdx. Fixed once in a shared
frontmatterMetaPrefix helper and applied at all three call sites — the third,
in translate-i18n.ts, had the identical bug.

Also:

- main() is now guarded by `import.meta.main` so the module can be imported by
  tests without running a full sync over the repo. Running the script directly
  is unaffected.
- .github/scripts/i18n already shipped two bun:test suites and nothing ran them,
  which is why this was invisible to CI. Added i18n-scripts-test.yml.

Fixes #1358
@mintlify

mintlify Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
comfy 🟢 Ready View Preview Aug 7, 2026, 12:51 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 38 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f4f36632-f3a0-4465-b752-5cec4265f7ca

📥 Commits

Reviewing files that changed from the base of the PR and between 2943e37 and a245c2b.

📒 Files selected for processing (5)
  • .github/scripts/i18n/chunked-translate.ts
  • .github/scripts/i18n/sync-hash-i18n.test.ts
  • .github/scripts/i18n/sync-hash-i18n.ts
  • .github/scripts/i18n/translate-i18n.ts
  • .github/workflows/i18n-scripts-test.yml

Comment @coderabbitai help to get the list of available commands.

@christian-byrne

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@comfyui-wiki
comfyui-wiki merged commit 5886ef8 into main Aug 8, 2026
9 checks passed
@github-actions
github-actions Bot deleted the fix/sync-hash-i18n-idempotent branch August 8, 2026 12:36
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.

translate:sync-hash never converges: reports 71/84 files as drifted and appends a blank line every run

2 participants