refactor: ロジック層テスト拡充 + onerror 漏れ修正 + qr-ticket シリアライザ整理 (#169)

[fumtas1k]

概要

#169 のうち項目 1〜3 を対応。項目 4 (qrcode-generator 副作用 import 解消) は #216 として独立起票済み。

改修内容

項目 1: ロジック層テスト拡充

• `src/utils/tests/encoding.test.ts`: `detectEncoding` (BOM 検出 / サイズ制限 throw)、`decodeToText` (UTF-8 / ASCII / 日本語)、`convertBytes` (BOM 付き変換 / 往復)、`textToUtf8Bytes` (ASCII / 絵文字 / 日本語) のテスト追加
• `src/hooks/tests/useClampedInput.test.tsx`: 新規作成（12 ケース、handleChange / handleBlur 周り）
• `src/hooks/tests/useCodec.test.ts`: 既存 6 → 11 テスト（空入力即時クリア / debounce キャンセル / throw 後 recover）
• `src/hooks/tests/useQrCamera.test.ts`: 既存 8 → 12 テスト（`getUserMedia` reject 経路の安全性）
• `qr-reader.test.ts` は既に `canvas.getContext` null パスを含む充実したテストが存在したため追加なし

項目 2: `download.ts` onerror 漏れ修正

`src/utils/download.ts` `downloadPngFromSvgElement` の `img.onerror` で `URL.revokeObjectURL` を呼ぶハンドラ追加。`svgContentToPngBlob` との整合確保。

項目 3: `qr-ticket.ts` シリアライザ整理 ⚠️ 破壊的変更を含む

`src/utils/qr-ticket.ts`:

• `serializeTicket` / `parseQrString` 対称ペアを公開エクスポート
• `sanitizeField` を「`|` 含有時 throw する validation」に方針変更（従来のスペース silent 置換から） ⚠️
• `PAYLOAD_FIELD_COUNT` を `PAYLOAD_FIELDS` 配列の length から導出
• `verifyTicket` を `parseQrString` ベースに書き換えて対称ペアを完結（commit `1760704`）
• `parseQrString` の JSDoc に「signature は base64url で `|` を含まない前提」を明記（commit `1760704`）

⚠️ Migration note (破壊的変更)

`sanitizeField` の挙動変更により、`|` を含む値が `serializeTicket` / `parseQrString` 経由で渡された場合、従来は半角スペースに silent 置換されていたところが throw に変わる。

• 直接の影響: なし（既存テストは `|` を含まない正常系のため）
• 上位 UI 層 (`QrTicket.tsx`) では現在汎用 try/catch に拾われ「QRコードの生成中にエラーが発生しました」と表示されるため、UX 観点の改善 (`|` 入力を専用メッセージで弾く事前 validation) は PR refactor: QrTicket を 3 ドメインフックに分割 (#167) #221 (refactor: EncodingConverter / QrTicket の責務分割と派生状態最適化 #167-B QrTicket 3 hook 分割) merge 後のフォローアップ PR で別途対応する
• 決定記録: `docs/decisions.md` [060] エントリとして追加（commit `73ae453`）

レビュー反映履歴

commit	内容
97748c3	encoding.test.ts 拡充
9e2bc8f	useClampedInput.test.tsx 新規
985700d	download.ts onerror 修正
4904b02	qr-ticket シリアライザ整理（serializeTicket / parseQrString / sanitizeField throw 化）
7243846	useCodec.test.ts 拡充
32c00e3	useQrCamera.test.ts 拡充
73ae453	レビュー反映: decisions.md [060] エントリ追加
1760704	レビュー反映: verifyTicket を parseQrString ベース化 + JSDoc 拡充

スコープ外

• 項目 4: qrcode-generator の副作用 import 解消 → refactor(qrcode): qrcode-generator の副作用付き import を createQrSvg ヘルパに閉じ込める #216 で別途対応
• E2E flake (E2E テストの個別 timeout 設定方針を整理する #193 関連) → test(e2e): a11y-live-region の goto を domcontentloaded に + helpers timeout 追加 (#193) #219 で別途対応中
• UX 改善 (`|` 入力の専用エラーメッセージ): refactor: QrTicket を 3 ドメインフックに分割 (#167) #221 merge 後のフォローアップ PR
• nit 3 件 (encoding test assertion 緩い / serializeTicket と buildPayload の重複統一 / download onerror パステスト): refactor: PR #218 後追い nit 整理 (encoding test assertion / serializer 統一 / download.ts onerror テスト) #222 で別 issue 起票

検証

• `npm run test`: 464 → 473 件、全件緑
• `astro check`: 0 errors / 0 warnings
• `npm run test:e2e`: 142 passed / 1 skipped、全件緑（クリーンラン 42.8s）
• `aria-*` / `role=` の削除なし

関連 issue

• refactor: ロジック層テスト拡充と細かいクリーンアップ（4 項目まとめ） #169 (項目 1〜3 完了。項目 4 は refactor(qrcode): qrcode-generator の副作用付き import を createQrSvg ヘルパに閉じ込める #216 で対応)
• refactor(qrcode): qrcode-generator の副作用付き import を createQrSvg ヘルパに閉じ込める #216 (qrcode-generator 副作用 import 解消)
• refactor: PR #218 後追い nit 整理 (encoding test assertion / serializer 統一 / download.ts onerror テスト) #222 (本 PR レビュー由来の nit 3 件まとめ)

[fumtas1k]

レビューありがとうございます。3 つの独立改修すべて方向性として妥当で、特にテスト拡充の質が高く感心しました。マージブロッカーは無いですが、項目 3 の破壊的変更まわりで UX/ドキュメント観点の確認が 2 件あります。

---

良かった点

• テスト拡充 (項目 1) が非常に丁寧:
  • useClampedInput.test.tsx は初期値 / handleChange / handleBlur / 文字列→数値変換を網羅した 12 ケースで、parseInt('3.9', 10) = 3 の小数切り捨てまで明示的にテストしているのは良い保険。
  • useCodec.test.ts の追加は PR #149 のリグレッション保護（空入力即時クリア / debounce キャンセル / throw 後 recover）にちゃんと焦点が当たっていて、retrospective として理想形。
  • useQrCamera.test.ts の NotAllowedError / NotFoundError / その他 / unmount 後 throw しない、の 4 ケースで getUserMedia reject 経路が安全に閉じることを保証しているのが良い。
  • encoding.test.ts の convertBytes 往復テスト・絵文字（4 バイト）・日本語といった境界ケースまで揃っている。
• download.ts の onerror 修正 (項目 2) は単純で必要な修正。svgContentToPngBlob との挙動整合が取れてメモリリークの懸念が消えた。
• PAYLOAD_FIELD_COUNT = PAYLOAD_FIELDS.length + 1 は + 1 は signature 分 のコメント込みでマジックナンバーが解消されており、保守性が上がっている。

---

質問・改善提案（要確認 2 件）

1. sanitizeField の throw 化が UI 入力経路でハンドリングされていない疑い（要確認・優先度: 中）

旧 sanitizeField は | を半角スペースに silent 置換していたところを、本 PR で「| を含む場合は throw」に変更しています。silent な値破壊から明示エラーへという方針自体は正しいですが、QrTicket.tsx の呼び出し側を確認したところ:

• handleGenerate 内の for (const row of tickets) ループで signTicket(payload, ...) → buildPayload → sanitizeField で throw すると、外側の汎用 try/catch に拾われて "QRコードの生成中にエラーが発生しました" という汎用メッセージが表示されます。
• UI 側に | 入力を事前 reject する validation はありません（isSafeTicketId は ticket ID にしか効きません。eventId / name / category は素通しです）。
• 結果: 旧挙動では「| がスペースに置換された QR が生成される」 → 新挙動では「汎用エラーで原因が分からない」になり、ユーザー視点ではむしろ UX が劣化する可能性があります。

提案: 以下のいずれか（マージ前にどちらかを選ぶか、フォローアップ issue を切るのを推奨）。

• (a) handleGenerate の事前 validation に「いずれかのフィールドに | が含まれていないか」のチェックを追加して、専用メッセージ（例: "フィールドに | を含めることはできません"）を出す。
• (b) try/catch 側で error instanceof Error && error.message.includes('|') を判定して、専用メッセージを setGenerateError に流す。
• (c) 当面はフォローアップ issue で扱う旨を PR description に明記する。

2. PR description / docs/decisions.md への破壊的変更の明示（優先度: 中）

項目 3 は qr-ticket.ts の public API 表面の変更（serializeTicket / parseQrString 新規 export + sanitizeField の挙動変更）です。とくに 「| をスペースに置換 → throw」は外向き挙動の破壊的変更にあたります。

• 現 PR description では「sanitizeField を validation に方針変更」と 1 行で済ませていますが、migration note として「旧来 | を入力していた場合は throw に変わる」を目立つ位置に書くと、リリースノート / 後追い時の参照性が上がります。
• CLAUDE.md 4 章のドキュメント更新ルールに照らすと、docs/decisions.md への記録対象に該当する可能性があります（API 表面の変更 + 既存挙動の破壊的変更）。確認をお願いします。

---

nit（任意・優先度: 低）

3. detectEncoding UTF-16 BE BOM テストの assertion が曖昧

expect(['UTF16LE', 'UTF16BE']).toContain(result.encoding);

encoding-japanese の内部マッピングに依存した「どちらでも通る」assertion になっており、ライブラリのアップデートで silent に挙動が変わる可能性があります。hasBom: true の確認のみに留めるか、ライブラリ挙動をピン止めするテストとして明示コメント付けると堅牢です。コメントで lib 挙動を説明している点は good ですが、テストとしての契約が緩いのが気になります。

4. parseQrString の lastIndexOf('|') の前提コメント

Ed25519 base64url 署名は | を含まない

実装上は ECDSA P-256 + base64url なので「base64url 文字集合（A-Za-z0-9_-）に | は含まれない」が正確な前提になります。parseQrString の JSDoc に 「signature には | が含まれない（base64url 仕様）前提」 と一行追記すると将来読む人に親切です。

5. serializeTicket と buildPayload の重複

serializeTicket は buildPayload の単純な delegate です。長期的にはどちらか一方に統一する方針があると良いです（例: buildPayload を private 化、serializeTicket を公開 API として残す等）。ただし破壊的変更なので 本 PR ではそのままで OK、フォローアップ issue 候補として記録するのを推奨。

6. parseQrString が現時点で未使用

verifyTicket は依然として自前で rawData.split('|') していて、新規 export した parseQrString を使用していません。今 PR では「対称ペアの整理」として理解できるので問題なしですが、verifyTicket を parseQrString ベースに書き換えるフォローアップがあると整理が完結します。

7. download.ts の onerror パスのテスト

jsdom で Image.onerror を再現するのは難しいので必須ではありませんが、URL.revokeObjectURL が onerror でも呼ばれることを spy で確認するテストがあると完璧でした。優先度は低いです。

8. 1 PR / 3 改修の構成

issue #169 の項目 1〜3 を 1 PR に統合した点は意図として理解できますが、「sanitizeField の throw 化」は単独でレビュー単位として独立性が高かったかなと感じました。次回以降、API 表面が変わる項目は分割した方がレビュー観点を集中させやすそうです（マージブロッカーではありません）。

---

まとめ

• コード品質・テスト品質ともに高く、項目 1〜2 は LGTM。
• 項目 3 の sanitizeField throw 化に関して UI 側の補助 validation か、エラーメッセージの専用化を検討してください（質問 1）。
• PR description / decisions.md に破壊的変更を明記してください（質問 2）。
• 以上はマージブロッカーではなく、「マージ前に方針合意 or フォローアップ issue 化」のいずれかで対応可能です。

Generated by Claude Code

---

Generated by Claude Code

[fumtas1k]

レビューありがとうございます。マージブロッカーではないとの評価でしたが、要対応 2 件と nit を以下のように振り分けて対応しました。

反映済み（本 PR 内追コミット）

• 質問 2（破壊的変更の明示）: `docs/decisions.md` に [060] エントリを追加し、PR description にも Migration note セクションを目立つ形で追加しました（commit `73ae453`）
• nit 4（`parseQrString` の JSDoc）: signature は ECDSA P-256 + base64url で `|` を含まない前提を JSDoc に明記しました（commit `1760704`）
• nit 6（`verifyTicket` を `parseQrString` ベースに書き換え）: 自前 `split('|')` を新ペア利用に置き換えて対称ペアを完結させました。既存テスト（パイプ数不一致 / timestamp 数値でない 等）が同じエラーメッセージを返すことを確認しました（commit `1760704`）

別 PR で対応

• 質問 1（UX 改善: `|` 入力の専用エラーメッセージ）: 現状 `QrTicket.tsx` を改修中の PR refactor: QrTicket を 3 ドメインフックに分割 (#167) #221 (refactor: EncodingConverter / QrTicket の責務分割と派生状態最適化 #167-B 3 hook 分割) と完全衝突するため、refactor: QrTicket を 3 ドメインフックに分割 (#167) #221 merge 後のフォローアップ PR で新 `useTicketGeneration` フック内に事前 validation を入れる設計にしました。UX 劣化が一時的に残る点を PR description の Migration note にも明記しています

別 issue 起票

• nit 3 / 5 / 7（encoding test assertion 緩い / serializeTicket と buildPayload 重複統一 / download onerror パステスト）: refactor: PR #218 後追い nit 整理 (encoding test assertion / serializer 統一 / download.ts onerror テスト) #222 にまとめて起票しました
• nit 8（PR 分割の感想）は次回以降 API 表面が変わる項目を分割する運用方針として記録します（メタなので issue 化なし）

検証は `npm run test` 473 件緑 / `astro check` 0 errors / `npm run test:e2e` 142 passed (クリーンラン)。再確認お願いします。

[fumtas1k]

再レビュー (head: 1760704)

前回レビューで指摘した重要 (a)(b) と nit 6 件への対応を確認しました。総じて丁寧な対応で、追加修正もペアの対称性が完結しており品質が向上しています。マージ可能 と判断します。残課題は軽微なフォローアップ提案のみです。

---

解消状況の評価

重要 (a) — sanitizeField throw 化の UX 劣化 → フォローアップ deferral

• PR description で「PR #221 (QrTicket 3 hook 分割) merge 後のフォローアップ PR で対応」と明示。QrTicket.tsx 衝突回避の判断は妥当です。
• ただし: フォローアップ専用 issue がまだ起票されていません (QrTicket sanitize / sanitizeField / パイプ UX 等で検索しヒットなし)。docs/shared-agent-rules.md 6.4 章「先送り時は必ず issue 化」に沿うため、PR #221 merge 待ちの追跡 issue を 1 件起票しておくことを推奨します (タイトル例: fix(QrTicket): | 含有時の専用エラーメッセージ追加 (PR #221 merge 後))。

重要 (b) — 破壊的変更の文書化 → docs/decisions.md [060]

• 背景 / 決断 / 影響・移行 / 却下した選択肢 / 関連 PR・issue の 5 セクション構造で、既存 [058] [059] と整合的。silent 置換維持・別文字置換・エスケープ方式を理由付きで却下している点も網羅的で、後から経緯を追える資産になりました。
• PR description の「⚠️ Migration note」セクションも目立つ位置に配置されており、読み手への配慮として完璧です。

nit 6 件 → #222 で起票

• #222 起票済みを確認。encoding test assertion / serializer 統一 / download.ts onerror テストの 3 件が内容含めて適切に整理されています。残りの 3 件 (parseQrString と verifyTicket の重複 / serializeTicket と buildPayload の重複等) は本 PR 内の追加修正 (commit 1760704) で実質解消済みのため、#222 の 3 件で track 範囲は十分です。

---

追加修正の精査

verifyTicket の parseQrString ベース化 (commit 1760704)

挙動同等性を確認しました:

観点	旧実装	新実装
フィールド数 6 の検証	parts.length !== PAYLOAD_FIELD_COUNT	parseQrString 内で payloadParts.length !== PAYLOAD_FIELDS.length (= 5) + signature 非空チェック
signature 空ガード	なし (旧コードは !s チェックで間接的に検出)	parseQrString で if (!signature) return null の早期 return + verifyTicket 内 !s チェック残置で二重防御
エラーメッセージ	同一 (QRデータの形式が不正です)	同一

→ 挙動は等価かつ若干堅牢化 しており、serializeTicket (内部 buildPayload) ↔ parseQrString の対称ペアが完結。設計改善として優秀です。

parseQrString JSDoc 拡充

ECDSA P-256 + base64url の文字集合 (A-Za-z0-9_-) が | を含まないため lastIndexOf('|') が一意境界を返す、という仕様前提が明記されました。将来 qr-ticket.ts を触る人が lastIndexOf の正当性を一目で理解できます。完璧。

---

軽微な提案 (任意・後追い可)

1. PAYLOAD_FIELD_COUNT が dead export 化: verifyTicket のリファクタにより、リポジトリ全体検索で実コード参照は 0 件になりました (qr-ticket.ts 内 JSDoc コメントの記述のみ)。export 維持コストは微小ですが、#222 の「serializer 統一」と合わせて削除候補になります。
2. UX 改善 follow-up issue の起票: 上記 (a) 解消状況の通り、PR #221 merge 後の作業として 1 件起票しておくと追跡漏れが防げます。

---

検証結果

• npm run test: 473 件全件緑
• astro check: 0 errors / 0 warnings
• npm run test:e2e: 142 passed / 1 skipped (42.8s)

すべてクリーンラン。前回指摘事項は本 PR 内対応分・別 issue track 分ともに解消済みで、マージ進めて問題ありません。お疲れさまでした。

Generated by Claude Code

---

Generated by Claude Code

[fumtas1k]

再レビューありがとうございます。残課題 2 件を別 issue で track する形で対応しました。

• 重要 (a) フォローアップ追跡: fix(QrTicket): | 含有時の専用エラーメッセージ追加 (PR #221 merge 後フォローアップ) #224 を起票しました（`fix(QrTicket): | 含有時の専用エラーメッセージ追加 (PR refactor: QrTicket を 3 ドメインフックに分割 (#167) #221 merge 後フォローアップ)`）。PR refactor: QrTicket を 3 ドメインフックに分割 (#167) #221 merge 後の develop ベースで新 `useTicketGeneration` 内に事前 validation を入れる方針です。
• `PAYLOAD_FIELD_COUNT` dead export 化: refactor: PR #218 後追い nit 整理 (encoding test assertion / serializer 統一 / download.ts onerror テスト) #222 にスコープを追加しました（項目 4 として「`serializer 統一`」と同 PR で扱う）。

検証結果 (473 件緑 / astro check 0 / e2e 142 passed) は維持されており、ご指示の通り merge を進めて問題ない状態かと思います。