fix: reset list numbering when switching from unordered to ordered list

[JohnMcLear]

Summary

One-line logic fix in ace2_inner.ts's applyNumberList function: reset the position counter when the list type changes at the same nesting level (e.g., bullet -> number).

Root Cause

renumberList walks backward to find the first line of a contiguous list block, then walks forward through applyNumberList to assign start values. The function counted all list items at the same level in a single position counter — regardless of whether they were bullet or number items.

When an ordered list followed an unordered list:

bullet a    ← position=1 (bullet1)
bullet b    ← position=2 (bullet1)  
number 1    ← position=3 (number1) — should be 1!
number 2    ← position=4 (number1) — should be 2!

The first OL item got start=3 instead of start=1. Since domline.ts only applies the list-start-number1 CSS class when start === 1, the CSS counter-reset never fired and all OL items displayed as "1".

Test plan

• Frontend: new Playwright test issue #5160 ordered list increments correctly after unordered list passes
• Frontend: 2 pre-existing ordered list test failures confirmed on develop (not caused by this change)
• Backend: messages.ts tests pass (10/10)
• Manual: create UL with 2 items, then OL directly after → numbers should be 1, 2, 3

Fixes #5160

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Fix ordered list numbering after unordered list

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Reset list numbering counter when switching from unordered to ordered lists
• Fixes ordered list items showing "1" instead of incrementing after bullet lists
• Adds regression test for issue #5160 with multiple list type transitions

Diagram

flowchart LR
  A["Unordered list items<br/>position counter increments"] -->|"List type changes<br/>bullet to number"| B["Reset position to 1"]
  B --> C["Ordered list items<br/>numbered correctly 1,2,3"]

Loading

File Changes

1. src/static/js/ace2_inner.ts 🐞 Bug fix +9/-0

Reset position counter on list type change

• Added prevType variable to track previous list type in applyNumberList function
• Extract current list type from regex match result as curType
• Reset position counter to 1 when list type changes at same nesting level
• Added comment referencing issue #5160 explaining the fix

src/static/js/ace2_inner.ts

---

2. src/tests/frontend-new/specs/ordered_list.spec.ts 🧪 Tests +38/-0

Add regression test for ordered list after unordered

• Added regression test for issue #5160 covering unordered to ordered list transition
• Test creates 2 bullet items followed by 3 numbered items
• Verifies each ordered list item has correct start attribute (1, 2, 3)
• Uses proper selectors and timeout handling for async rendering

src/tests/frontend-new/specs/ordered_list.spec.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Forced toolbar clicks 🐞 Bug ☼ Reliability

Description

The new Playwright regression test uses click({force: true}) on toolbar buttons, which bypasses
Playwright actionability checks and can let the test pass even if the button is not actually usable.
This reduces the test’s ability to catch real UI regressions around list toggling.

Code

src/tests/frontend-new/specs/ordered_list.spec.ts[R63-72]

+    const $insertUnorderedButton = page.locator('.buttonicon-insertunorderedlist');
+    await $insertUnorderedButton.click({force: true});
+    await writeToPad(page, 'bullet a');
+    await page.keyboard.press('Enter');
+    await writeToPad(page, 'bullet b');
+    await page.keyboard.press('Enter');
+
+    // Now switch to ordered list for the next items
+    const $insertOrderedButton = page.locator('.buttonicon-insertorderedlist');
+    await $insertOrderedButton.click({force: true});

Evidence

The test explicitly forces the unordered/ordered list toolbar clicks, bypassing normal click
constraints and thus weakening the regression signal.

src/tests/frontend-new/specs/ordered_list.spec.ts[63-73]
Best Practice: Playwright click() actionability checks

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test uses `click({force: true})` on toolbar buttons, which can mask real regressions because Playwright will click even when the element is not actionably clickable.

### Issue Context
This is a UI regression test; it should fail if the toolbar controls are not actually usable.

### Fix Focus Areas
- src/tests/frontend-new/specs/ordered_list.spec.ts[63-73]

### Suggested fix
- Replace `click({force: true})` with normal `click()`.
- Optionally add `await expect(locator).toBeVisible()` (and/or enabled) before clicking to make failures clearer.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Fixed sleep in test 🐞 Bug ➹ Performance

Description

The regression test adds a hard-coded 500ms wait before assertions, which slows the suite and is not
tied to an actual condition. The subsequent Playwright expectations already poll with timeouts, so
this should be replaced with condition-based waiting (or removed if redundant).

Code

src/tests/frontend-new/specs/ordered_list.spec.ts[R79-85]

+    // Wait for renumbering
+    await page.waitForTimeout(500);
+
+    // The first ordered list item (line 3) should have start=1
+    const thirdLine = padBody.locator('div').nth(2);
+    await expect(thirdLine.locator('ol')).toHaveAttribute('start', '1', {timeout: 5000});
+

Evidence

The test sleeps for 500ms and then immediately uses toHaveAttribute(..., {timeout: 5000}), which
already waits/polls for the expected DOM state; the sleep adds runtime without asserting a
condition.

src/tests/frontend-new/specs/ordered_list.spec.ts[79-92]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test uses `page.waitForTimeout(500)` as a fixed delay, which slows CI and is less robust than waiting on an explicit condition.

### Issue Context
The following `expect(...).toHaveAttribute(..., {timeout: 5000})` calls already provide polling/waiting.

### Fix Focus Areas
- src/tests/frontend-new/specs/ordered_list.spec.ts[79-85]

### Suggested fix
- Remove the `waitForTimeout(500)` line.
- If additional synchronization is truly needed, wait for an explicit condition (for example, `await expect(thirdLine.locator('ol')).toHaveAttribute('start', '1')` as the first wait) rather than a fixed sleep.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more