fix(ExportHtml): preserve nested ol start across sibling ul#7791
Merged
Conversation
When an ordered-list level was the only consumer of olItemCounts, closing any list at that depth (including an unordered list that happens to share the level) reset olItemCounts[level] to 0. A later, unrelated ordered list at the same depth then took the "counter exists but is 0" branch in the ol-opening logic and emitted `<ol class="...">` without the start attribute that line.start would have supplied. Round-trip: importing <ul>...<ul>...</ul></ul><ol><li>x<ol><li>y</li></ol></li></ol> exported the inner ol as `<ol class="number">` instead of `<ol start="2" class="number">`, because the closing of the inner bullet ul wrote olItemCounts[2]=0 before the outer ol even opened. Gate the reset on line.listTypeName === 'number' so closing an unordered list never touches the ol bookkeeping. Closing an actual ordered list still resets, as #7470 intended. Fixes #7786 Fixes #7787 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by QodoPrevent ul closure from poisoning ol counter at same depth
WalkthroughsDescription• Fix nested ordered list start attribute loss when sibling unordered list closes • Gate ordered-list counter reset on list type to prevent unordered list closures from poisoning ol bookkeeping • Preserve line.start for unrelated ordered lists at same depth after unordered list closure Diagramflowchart LR
A["Closing UL at depth N"] --> B["Check list type"]
B --> C["Type is 'number'?"]
C -->|Yes| D["Reset olItemCounts[N]"]
C -->|No| E["Skip reset"]
D --> F["Later OL at depth N uses line.start"]
E --> F
File Changes1. src/node/utils/ExportHtml.ts
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Round-tripping
<ul>...<ul>...</ul></ul><ol><li>x<ol><li>y</li></ol></li></ol>throughsetHTML→getHTMLwas emitting the inner<ol>as<ol class=\"number\">instead of the expected<ol start=\"2\" class=\"number\">.Root cause: the depth-decrease loop in
getHTMLFromAtextresetsolItemCounts[level] = 0to signal "this ol level was explicitly closed, don't fall back toline.startnext time it reopens" (#7470). The gate only checked depth, not list type, so closing a bullet<ul>at depth 2 was also writingolItemCounts[2] = 0. When a later, unrelated<ol>opened at depth 2, the ol-opening logic took the "counter exists but is 0" branch and droppedline.start.Fix: only run the counter reset when the line being closed is itself a
numberlist. UL closures no longer touch ol bookkeeping.Trade-off
The 0-sentinel design from #7470 still applies for genuine ol-close cases — if you close an ol and reopen one later at the same depth,
line.startis still ignored (per that PR's intent). This change narrows the gate; it doesn't reverse the prior decision.Test plan
Backend sweep on a fresh
developclone (Node 24.14.0):appendTextAuthorx1,importexportGetPostx2,pad.tsx3 ("Get Authors of the Pad", "Pad with complex nested lists of different types", "copyPadWithoutHistory > creates a new pad with the same content as the source pad").pad.tsnested-list cases now pass. The other 4 failures are unrelated (Failing backend test: listAuthorsOfPad returns 1 author after setHTML when 0 expected #7785, Failing backend test: non-numeric :rev export returns 500 without 'rev is not a number' body #7788, Failing backend test: appendText without authorId still attributes one author #7790) and tracked separately.tests/backend/specs/api/ortests/backend/specs/admin/.Note: these failures are currently invisible in CI because of the broken backend test glob — see #7789, which has to land for CI to actually run these tests.
Fixes #7786
Fixes #7787
🤖 Generated with Claude Code