Skip to content

Stop rebuilding the 500-button level list, and make speech logging lazy - #90

Merged
min0625 merged 1 commit into
mainfrom
perf/level-list-render
Jul 28, 2026
Merged

min0625 merged 1 commit into
mainfrom
perf/level-list-render

Conversation

@min0625

@min0625 min0625 commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Three separate costs, all on paths a player hits repeatedly.

Level picker no longer rebuilds 500 buttons per open

renderLevelList() wiped and rebuilt the whole list every time the picker
opened, giving each button its own click closure just to remember its own id.

  • One delegated listener on #level-list reading data-id replaces the 500
    closures.
  • The render is memoized on currentLevel/levelCount — the only two inputs
    that decide what the list looks like. levelCount is in the key because it
    is 0 until boot()'s index.json lands; keying on currentLevel alone
    would mark that empty list as drawn and never repaint it.
  • Buttons are built into a DocumentFragment and swapped in with
    replaceChildren(), and the icon <span> is createElement'd instead of
    asking the HTML parser to parse the same markup 500 times.

.level-btn gets content-visibility: auto

Unhiding #screen-levels used to lay out and paint all 500 buttons (each with
its own .icon mask); now only the dozen actually on screen. 7.3ms → 2.8ms on
the same machine. The memoization stops the DOM rebuild — it cannot stop the
layout pass that comes with display:none → visible, which is what this covers.

contain-intrinsic-size deliberately leaves the inline axis as none: a
skipped element gets size containment, so an intrinsic width becomes its
min-content contribution, and the columns are 1fr (= minmax(auto, 1fr)), so
it would set a floor under every column. The measured 78px would floor the
grid at 5*80 + 4*12 + 2*16 = 470px and overflow horizontally on every phone
narrower than .screen's 480px cap.

The block axis is the one that matters anyway — scroll length, and where
showLevels()'s scrollIntoView({block:'center'}) lands. Its 18px only holds
if the line box is pinned, so line-height is 1.125 instead of font-dependent
normal (ui-rounded / Segoe UI Rounded / system-ui each give a different
value); 1px per row over 100 rows is 100px of scroll error. min-height: 48px
is a backstop — content-visibility and contain-intrinsic-size are
independent declarations, and a browser honouring only the first would size
skipped buttons to 0 and collapse the list. It doubles as the 48px touch-target
floor.

Speech debug logging is lazy, and only wired under ?debug

The two diagnostics in speak() read getVoices() / speaking / pending /
paused — synchronous trips into the TTS daemon on iOS — and speak() sits on
the correct-word hot path. Passing built strings meant paying for them on every
word just to hand them to a noop; the speak() sync measurement in main.js
was partly measuring cost the logging itself created.

log now takes a thunk, and setSpeechDebug(dbg) is guarded by DEBUG. Both
halves are needed: the thunk stops argument evaluation, and the guard stops a
non-null sink from invoking the thunk at all (dbg() returning early is too
late by then).

Testing

mise run check (Biome + 48 unit tests) passes. Layout and scroll behaviour of
the level picker are manual-check items (design doc §12): open the picker at a
late level on a narrow viewport and confirm no horizontal overflow and that the
current level lands centred.

🤖 Generated with Claude Code

Three separate costs, all paid on paths a player hits repeatedly.

- renderLevelList() rebuilt all 500 buttons on every open of the level
  picker, each with its own click closure just to remember its own id.
  Now: one delegated listener on #level-list reading data-id, and the
  render is memoized on `currentLevel/levelCount` — the only two inputs
  that change what the list looks like. levelCount is in the key because
  it is 0 until boot()'s index.json lands; keying on currentLevel alone
  would mark that empty list as drawn and never repaint it. Buttons are
  built into a fragment and swapped in with replaceChildren(), and the
  icon span is createElement'd instead of asking the HTML parser to parse
  the same markup 500 times.

- .level-btn gets content-visibility: auto, so unhiding #screen-levels
  only lays out and paints the dozen buttons actually on screen instead
  of all 500 (7.3ms -> 2.8ms on the same machine). The memoization above
  stops the DOM rebuild; it cannot stop the layout pass that comes with
  display:none -> visible, which is what this covers.

  contain-intrinsic-size deliberately leaves the inline axis as `none`:
  a skipped element gets size containment, so an intrinsic width would
  become its min-content contribution, and the columns are 1fr — i.e.
  minmax(auto, 1fr) — so it would set a floor under every column. The
  measured 78px would floor the grid at 5*80 + 4*12 + 2*16 = 470px and
  overflow horizontally on every phone narrower than .screen's 480px cap.
  The block axis is the one that matters anyway (scroll length and where
  scrollIntoView lands). Its 18px is only correct if the line box is
  pinned, so line-height is 1.125 rather than font-dependent `normal`;
  1px per row over 100 rows is 100px of scroll error. min-height: 48px
  is a backstop — content-visibility and contain-intrinsic-size are
  independent declarations, and a browser that honours only the first
  would size skipped buttons to 0 and collapse the list.

- dictionary-card's debug log now takes a thunk, and main.js only wires
  the sink when ?debug is set. The two diagnostics read getVoices() /
  speaking / pending / paused, which are synchronous trips into the TTS
  daemon on iOS, and speak() sits on the correct-word hot path — passing
  built strings meant paying for them on every word just to hand them to
  a noop. Both halves are needed: the thunk stops argument evaluation,
  and the DEBUG guard stops a non-null sink from calling the thunk at all
  (dbg() returning early is too late by then).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Optimize level picker rendering and lazy-load speech diagnostics

✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Memoizes level-list rendering and delegates button clicks to avoid repeated DOM reconstruction.
• Defers offscreen level-button layout and painting while preserving responsive grid sizing.
• Evaluates speech diagnostics only when debug logging is enabled.
Diagram

graph TD
  A["Repeated action"] -->|"opens picker"| B["Level picker"] --> C{"Render key changed"}
  C -->|"yes"| D["Fragment swap"] --> E["Visible layout"]
  C -->|"no"| E
  A -->|"correct word"| F["Speech request"] --> H["TTS engine"]
  F --> G{"Debug enabled"}
  G -->|"yes"| H
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Virtualize the level grid
  • ➕ Creates DOM nodes only for visible levels
  • ➕ Scales beyond the current 500-level list
  • ➖ Requires custom scroll-position and focus management
  • ➖ Complicates accessibility and current-level navigation
  • ➖ Adds substantial complexity for a bounded list
2. Incrementally update existing buttons
  • ➕ Avoids rebuilding the complete list when the current level changes
  • ➕ Preserves button identity and focus
  • ➖ Requires careful synchronization of completed, current, and locked states
  • ➖ Adds mutable update logic for limited additional benefit
3. Guard each speech diagnostic directly
  • ➕ Avoids allocating logging closures when debugging is disabled
  • ➕ Makes lazy evaluation explicit at each expensive diagnostic
  • ➖ Duplicates sink checks across every logging site
  • ➖ Makes future diagnostics easier to accidentally evaluate eagerly

Recommendation: Keep the PR’s memoization, event delegation, fragment replacement, and native content-visibility approach: it captures most of the benefit without introducing virtualization complexity. Lazy logging through a centralized thunk API is also maintainable; direct sink guards would save minor closure allocation but are unlikely to justify duplicated conditional logic.

Files changed (3) +69 / -18

Enhancement (1) +21 / -0
style.cssSkip rendering offscreen level buttons +21/-0

Skip rendering offscreen level buttons

• Adds 'content-visibility' and block-axis intrinsic sizing so hidden level buttons avoid unnecessary layout and paint work. Pins line height and minimum touch-target height to preserve scrolling accuracy and browser fallback behavior.

src/style.css

Other (2) +48 / -18
dictionary-card.jsMake speech diagnostics lazily evaluated +17/-10

Make speech diagnostics lazily evaluated

• Replaces the no-op logger with a nullable debug sink accepting lazily evaluated message thunks. Expensive speech-synthesis state and voice queries now run only when a debug sink is attached.

src/dictionary-card.js

main.jsMemoize and batch level-list rendering +31/-8

Memoize and batch level-list rendering

• Attaches speech logging only under '?debug'. Memoizes the level list by current level and level count, delegates clicks through the list container, and builds buttons in a DocumentFragment before one atomic replacement.

src/main.js

@min0625
min0625 merged commit 428a210 into main Jul 28, 2026
1 check passed
@min0625
min0625 deleted the perf/level-list-render branch July 28, 2026 15:30
@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 80 rules
✅ Skills: frontend-design

Grey Divider


Remediation recommended

1. style.css adds non-English comments 📘 Rule violation ⚙ Maintainability
Description
Newly added source-code comments in src/style.css, src/dictionary-card.js, and src/main.js are
written in Traditional Chinese, including documentation for .level-btn, lazy speech logging,
delegated level selection, memoization, and fragment rendering. Because CSS block comments and
JavaScript comments are source-code comments, they violate the English-only requirement.
Code

src/style.css[R512-528]

+  /* 500 顆按鈕裡同時看得到的只有十幾顆,其餘的版面與繪製(每顆各一層 .icon 的 mask)純屬白算。
+     省的是「每次開關卡選擇」都要付的那筆:screen 從 display:none 翻回來時 500 顆全部重新算,
+     main.js 的記憶化擋得掉重建 DOM,擋不掉這一次——實測同一台機器 7.3ms → 2.8ms。
+     寬度**一定要 none**,不能填實測的 80−2(border)=78:被略過的元素會進 size containment,
+     那個寬度就變成它的 min-content 貢獻,而欄是 1fr=minmax(auto, 1fr),等於幫每一欄設下
+     80px 下限——5×80 + 4×12(gap) + 2×16(padding) = 470px,比 .screen 的 480px 上限窄的
+     每一支手機(390/375/360…)都撐不下,關卡格會橫向溢出、右欄被切掉。寬度交給 grid 決定。
+     高度才是這裡真正要給的:捲動長度與 scrollIntoView 的落點都靠它。18 是**內容框**
+     =48(整顆) − 28(padding) − 2(border),而 48 只有在行高被釘住時才成立——normal 行高隨字型
+     而異(ui-rounded/Segoe UI Rounded/system-ui 各一個值),所以 line-height 寫死 1.125
+     (16px × 1.125 = 18px)。差 1px × 100 列就是捲軸與「捲到目前關卡」差 100px。
+     用比例不用 px:玩家把系統字級調大時整顆要跟著長高,min-height 只保底不封頂。
+     前面的 auto =渲染過一次就記住真實尺寸,估值只在首次掠過時用到。改 padding/border/
+     line-height 時三個數字要一起維護。
+     min-height 是保命索,不是排版:content-visibility 與 contain-intrinsic-size 是兩條獨立
+     宣告,不會一起被忽略——只認得前者的瀏覽器(Chrome 111 以前不吃 contain-intrinsic-size
+     的 auto 前綴)會把被略過的按鈕算成 0 高,整份清單塌掉。順便也是 48px 觸控目標下限。 */
Evidence
The cited regions show newly added Traditional Chinese comments: the CSS block comment at lines
512–528, the multi-line dictionary-card.js comment at lines 15–18, and the main.js comments
describing lazy speech logging, delegated level selection, memoization, and fragment rendering.
These additions conflict with PR Compliance ID 2366288, which requires source-code comments in
modified files to be written in English.

Rule 2366288: Source code comments must be written in English
src/style.css[512-528]
src/dictionary-card.js[15-18]
src/main.js[156-157]
src/main.js[609-621]
src/main.js[627-628]

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

## Issue description
Translate the newly added Traditional Chinese source-code comments in `style.css`, `dictionary-card.js`, and `main.js` into English without changing their technical meaning, including the `.level-btn` sizing and browser-compatibility rationale and the performance rationale in `main.js`.

## Issue Context
PR Compliance ID 2366288 requires all added or modified line and block comments in source files, including stylesheets, to be written in English.

## Fix Focus Areas
- src/style.css[512-528]
- src/dictionary-card.js[15-18]
- src/main.js[156-157]
- src/main.js[609-621]
- src/main.js[627-628]

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


2. Locked levels remain clickable 🐞 Bug ≡ Correctness
Description
The delegated handler starts any matching .level-btn without checking disabled; in affected
browsers, clicking the icon inside a disabled button bubbles to the list and loads that locked
level. Since startLevel() accepts every in-range ID, this bypasses linear progression.
Code

src/main.js[614]

+  if (b) startLevel(Number(b.dataset.id));
Evidence
The new delegated listener calls startLevel() for every closest level button, while locked buttons
are disabled but contain a clickable descendant span. startLevel() only rejects IDs beyond the
total count and therefore loads a future in-range level; this violates the documented linear-unlock
rule.

src/main.js[612-615]
src/main.js[639-643]
src/main.js[321-349]
docs/word-puzzle-game-design.md[191-194]

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 delegated level-list click handler can activate a disabled locked button when a click originating from its child icon bubbles to `#level-list`. Add an enabled-state guard, and ensure the level-loading boundary cannot bypass progression.

## Issue Context
Locked buttons contain child spans, and descendant-click behavior for disabled controls varies by browser. `startLevel()` currently accepts any ID up to `levelCount`, including levels beyond `save.currentLevel`.

## Fix Focus Areas
- src/main.js[612-615]
- src/main.js[321-349]
- src/main.js[639-643]

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


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread src/style.css
Comment on lines +512 to +528
/* 500 顆按鈕裡同時看得到的只有十幾顆,其餘的版面與繪製(每顆各一層 .icon 的 mask)純屬白算。
省的是「每次開關卡選擇」都要付的那筆:screen 從 display:none 翻回來時 500 顆全部重新算,
main.js 的記憶化擋得掉重建 DOM,擋不掉這一次——實測同一台機器 7.3ms → 2.8ms。
寬度**一定要 none**,不能填實測的 80−2(border)=78:被略過的元素會進 size containment,
那個寬度就變成它的 min-content 貢獻,而欄是 1fr=minmax(auto, 1fr),等於幫每一欄設下
80px 下限——5×80 + 4×12(gap) + 2×16(padding) = 470px,比 .screen 的 480px 上限窄的
每一支手機(390/375/360…)都撐不下,關卡格會橫向溢出、右欄被切掉。寬度交給 grid 決定。
高度才是這裡真正要給的:捲動長度與 scrollIntoView 的落點都靠它。18 是**內容框**
=48(整顆) − 28(padding) − 2(border),而 48 只有在行高被釘住時才成立——normal 行高隨字型
而異(ui-rounded/Segoe UI Rounded/system-ui 各一個值),所以 line-height 寫死 1.125
(16px × 1.125 = 18px)。差 1px × 100 列就是捲軸與「捲到目前關卡」差 100px。
用比例不用 px:玩家把系統字級調大時整顆要跟著長高,min-height 只保底不封頂。
前面的 auto =渲染過一次就記住真實尺寸,估值只在首次掠過時用到。改 padding/border/
line-height 時三個數字要一起維護。
min-height 是保命索,不是排版:content-visibility 與 contain-intrinsic-size 是兩條獨立
宣告,不會一起被忽略——只認得前者的瀏覽器(Chrome 111 以前不吃 contain-intrinsic-size
的 auto 前綴)會把被略過的按鈕算成 0 高,整份清單塌掉。順便也是 48px 觸控目標下限。 */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. style.css adds non-english comments 📘 Rule violation ⚙ Maintainability

Newly added source-code comments in src/style.css, src/dictionary-card.js, and src/main.js are
written in Traditional Chinese, including documentation for .level-btn, lazy speech logging,
delegated level selection, memoization, and fragment rendering. Because CSS block comments and
JavaScript comments are source-code comments, they violate the English-only requirement.
Agent Prompt
## Issue description
Translate the newly added Traditional Chinese source-code comments in `style.css`, `dictionary-card.js`, and `main.js` into English without changing their technical meaning, including the `.level-btn` sizing and browser-compatibility rationale and the performance rationale in `main.js`.

## Issue Context
PR Compliance ID 2366288 requires all added or modified line and block comments in source files, including stylesheets, to be written in English.

## Fix Focus Areas
- src/style.css[512-528]
- src/dictionary-card.js[15-18]
- src/main.js[156-157]
- src/main.js[609-621]
- src/main.js[627-628]

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

Comment thread src/main.js
// 鎖住的關卡是 disabled,本來就不發 click,不必在這裡再判一次。
$('level-list').addEventListener('click', (e) => {
const b = e.target.closest('.level-btn');
if (b) startLevel(Number(b.dataset.id));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Locked levels remain clickable 🐞 Bug ≡ Correctness

The delegated handler starts any matching .level-btn without checking disabled; in affected
browsers, clicking the icon inside a disabled button bubbles to the list and loads that locked
level. Since startLevel() accepts every in-range ID, this bypasses linear progression.
Agent Prompt
## Issue description
The delegated level-list click handler can activate a disabled locked button when a click originating from its child icon bubbles to `#level-list`. Add an enabled-state guard, and ensure the level-loading boundary cannot bypass progression.

## Issue Context
Locked buttons contain child spans, and descendant-click behavior for disabled controls varies by browser. `startLevel()` currently accepts any ID up to `levelCount`, including levels beyond `save.currentLevel`.

## Fix Focus Areas
- src/main.js[612-615]
- src/main.js[321-349]
- src/main.js[639-643]

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

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.

1 participant