---

## Code Review: `reports/phase3/scripts/build_top200_v2_h5.py`

This diff is a major refactoring of the H5 page builder — from a standalone inline HTML generator (~925 lines) to a template-based approach (~312 lines) that reuses `skill-market-h5.html` as a shell. The table-based layout is replaced with a card grid, the scoring scale changes from 20 to 5, and the UI switches from select-based filters to chip-based filters.

---

### `build_top200_v2_h5.py:100` — `CLEANUP` — Dead return value in `extract_h5_shell()`

**Summary:** The function returns a tuple `(head_and_body, "")` where the second element is always an empty string.

**Suggestion:** Return just `head_and_body` as a plain string, and update the caller (`shell = extract_h5_shell()` instead of `shell, _ = extract_h5_shell()`).

---

### `build_top200_v2_h5.py:105–158` — `CLEANUP` — Fragile template text replacement

**Summary:** The function performs 10+ sequential `.replace()` calls against exact HTML markup in `skill-market-h5.html`. If the template is ever reformatted or tweaked (whitespace changes, attribute reordering), these replacements silently no-op — the old strings remain in the output.

**Failure scenario:** Someone edits `skill-market-h5.html` (e.g., adds a newline inside a `<div>` or changes `class="sub"` to `class="subtitle"`). The build script produces output with stale V1 text labels instead of V2 labels, with no warning.

**Suggestion:** Consider one of:
- Extract translatable strings/labels into a JSON config that both the template and this script reference.
- Use regex or a small HTML parser (e.g., `html.parser`) to find elements by class or ID and replace text content only.
- At minimum, add a post-build assertion that checks the output for expected V2 strings.

---

### `build_top200_v2_h5.py:247` — `CLEANUP` — `esc()` + inline `onclick` bypasses HTML escaping in JS context

**Summary:** The `esc()` function escapes `'` to `&#39;`, but when interpolated into an inline `onclick` attribute:

```javascript
onclick="openDrawer('${esc(r.skill_id)}')"
```

the HTML parser decodes `&#39;` back to `'` **before** the JavaScript engine runs. If a `skill_id` contained a single quote (e.g., `it's_a_test`), the decoded JS would be:

```javascript
openDrawer('it's_a_test')
```

which is a syntax error — the string terminates at the second `'`.

**Failure scenario:** A skill_id containing `'` would break the card's click handler. Extremely unlikely given skill ID naming conventions (slugs like `namespace__skill-name`), but incorrect in principle.

**Suggestion:** Prefer event delegation — attach one click handler on the grid container and read `data-id` from the card:
```javascript
document.getElementById('grid').addEventListener('click', e => {
  const card = e.target.closest('[data-id]');
  if (card) openDrawer(card.dataset.id);
});
```

Same issue applies to line 268 (`onclick="copyText('${esc(r.copied_path || '')}', this)"`).

---

### `build_top200_v2_h5.py:176–182` — `CLEANUP` — `securityLabel()` fallback ordering is fragile

**Summary:** The function first checks exact keys in the `m` map, then falls back to `String(s).includes('passed')` before `String(s).includes('caution')`. If a new security value containing **both** "passed" and "caution" were introduced (e.g., `passed_with_caution_but_flagged`), it would match `includes('passed')` first and return `'通过'` instead of `'谨慎通过'`.

**Failure scenario:** A hypothetical new security status `passed_with_caution_but_flagged` would display as "通过" rather than "谨慎通过". In practice, all known values are explicitly mapped, so this only matters if new values appear at runtime without updating the code.

**Suggestion:** Check `caution` before `passed` in the fallback chain, since "caution" is a more specific qualifier:
```javascript
if (String(s).includes('caution')) return '谨慎通过';
if (String(s).includes('passed')) return '通过';
```

---

### `build_top200_v2_h5.py:233–237` — `CLEANUP` — Sort options unreachable via UI

**Summary:** The JS sort handler supports `rank`, `rank-desc`, `score`, `score-asc`, and `name` modes. But the template's `<select>` (after `extract_h5_shell()` replacements) only has options for `score` and `score-asc`. The `name` sort option and `rank-desc` are never exposed as user-choosable options. The default `state.sort = 'rank'` (line 172) works for initial load, but users can't switch to name or reverse-rank sorting.

**Suggestion:** Either add the missing `<option>` elements in `extract_h5_shell()`, or remove the unreachable sort cases from the `switch` statement.

---

### `build_top200_v2_h5.py:25–50` — `CLEANUP` — `load_category_zh_map()` silently degrades on missing files

**Summary:** If both CSV files (`platform-high-frequency-skills.csv` and `platform-high-frequency-groups.csv`) are absent, the function silently falls back to just the 3-entry `SUBGROUP_CATEGORY_ZH` dict and whatever the optional markdown filter provides. The output will have very few Chinese category names, but no warning is raised.

**Suggestion:** Log a warning when neither CSV is found, so the developer knows the Chinese mapping is incomplete.

---

### `build_top200_v2_h5.py:68–97` — `CLEANUP` — `transform_records()` copies fields verbatim without validation

**Summary:** The function copies fields from raw JSON to output dicts without any type casting or validation (except `protected` → `bool`). Fields like `weighted_score` and `selection_score` could be strings or numbers from the input, and the JS code on the consumer side has to handle both (e.g., `Number(r.weighted_score) || 0`). Consider normalizing numeric fields here in Python to reduce JS complexity.

**Suggestion:** Add type coercion for numeric fields in Python:
```python
"weighted_score": _to_float(row.get("weighted_score")),
"selection_score": _to_float(row.get("selection_score")),
"rank": _to_int(row.get("rank")),
```

---

## Summary

- **0** blocker findings
- **0** correctness bugs
- **6** cleanup/simplification opportunities
- **Decision:** Ready to merge — no blockers or bugs found. The cleanup items (especially the fragile template replacement and the `onclick` escaping) are worth tracking but not blocking.