UI/UX品質の欠陥13件を直す（G-P2 全葉） - #53
Merged
Merged
Conversation
顔モザイクUI・字幕AI修正の実装と並行するタスクとして、UI/UX品質 （G-P2、監査P2-1〜P2-8）を実装した。マスター承認どおり、縦型切り抜き （別PRで進行中）とは依存が無いため並行して進めた。 - P2-1: 状態JSONの書き込みを atomic-json.mjs（一時ファイル→fsync→ rename）へ集約。書き込み中に強制終了しても既存の state.json が 壊れないことを実子プロセスのSIGKILLで検証。 - P2-2: av-verify.mjs がストリーム欠落を「長さ0秒」ではなく検証失敗 として扱うようにした。 - P2-3: ASS字幕のcentisecond→時分秒変換で、59.996秒等の境界で 0:00:59.100のような不正な時刻が出ていたバグを直した。 - P2-4-A/B/C: ジョブのTTL自動削除・保存容量クォータ・同時実行数上限 を job-lifecycle.mjs / pipeline-runner.mjs へ実装。 - P2-5/6/7/8: webapp-mockup のスマホ幅崩れ・文字サイズ・chipの aria-pressed同期・モーダルのフォーカス管理（初期フォーカス・ Tabトラップ・Escape・復帰）を直した。 全13葉で凍結前の探り3本（何もしていない偽物が落ちる／正しい実装の 値を測る／意図的に壊したものが落ちる）を実行し、記録した。 P2-5/7/8の検証には実ブラウザ（Playwright）が要る。video-shorts の 「npm依存ゼロ」はbuild-dist.mjsが配布しない保留経路(server/・ webapp-mockup/)には及ばないため、devDependencyとして正式導入し、 CIにChromiumインストールのステップを足した（既存のwebapp-mockup/ measure.mjsが未宣言のままplaywrightを使っていた状態も解消）。 pnpm -r test 全緑（video-shorts単体で新規46件を含む全PASS）。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNo27NkwpWAcFS1BkQ75B8
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe PR adds atomic JSON persistence, media stream validation, job TTL, quota, and concurrency controls. It also adds Playwright and static regression checks, responsive mobile layout behavior, larger text sizes, synchronized ARIA states, and shared modal focus management. ChangesReliability and Webapp Validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
findRuleBody() が selectorName を正規表現へ埋め込む際、. と # しか
エスケープしておらず、\ 等の他の正規表現特殊文字が漏れていた。
現状の呼び出しはリポジトリ内のリテラル(".two-col")のみで実害はないが、
CodeQL 高深刻度指摘への対応として、正規表現の特殊文字を全てエスケープ
する escapeRegExp() へ置き換えた。
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNo27NkwpWAcFS1BkQ75B8
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@video-shorts/AGENTS.md`:
- Around line 18-22: Update the test-command description in AGENTS.md to remove
the claim that pnpm --filter video-shorts test is equivalent to only two checks,
and describe it as the full project test suite, including all configured checks
such as the Playwright browser tests.
In `@video-shorts/package.json`:
- Around line 5-8: Update the matching P2-5, P2-7, and P2-8 entries in
docs/roadmap.html to reflect the newly added acceptance checks, including their
atomic-tree leaves, criteria, and verify fields. Do not defer this roadmap
update to a follow-up commit; keep the documented progress aligned with the test
coverage added in the test script.
In `@video-shorts/server/index.mjs`:
- Around line 206-213: Update the upload handling around computeUsedBytes and
hasQuotaAvailable to track process-wide reserved upload bytes, including
Content-Length when declared, and require usedBytes + reservedBytes +
declaredLength to stay within STORAGE_QUOTA_BYTES before accepting the upload.
For unknown-length streams, check the remaining quota before each write and
reject before exceeding it. Release the reservation on every success, rejection,
and failure path.
- Around line 69-79: Update sweepExpiredJobsNow to exclude job IDs that are
queued or currently running through startJob, so active work/output directories
are never deleted based solely on mtime. Track the lifecycle state using the
existing job queue or running-job state, pass those IDs into sweepExpiredJobs,
and preserve cleanup for expired jobs that are not active.
In `@video-shorts/server/job-lifecycle.mjs`:
- Around line 103-105: Update the expired-directory cleanup in the job lifecycle
flow to wrap each fs.rmSync call in per-directory error handling, log the path
when deletion fails, and only append to removed after a successful deletion.
Preserve processing of other directories despite an individual EACCES, EPERM, or
other removal error.
In `@video-shorts/src/atomic-json.mjs`:
- Around line 21-31: writeJsonAtomically の fs.writeSync 呼び出しを更新し、書き込んだバイト数を検証して
JSON バッファ全体が書き込まれるまで残りをループ処理してください。全バイトの書き込み完了後にのみ fsyncSync と renameSync
を実行し、短書きで未完成の一時ファイルが公開されないようにします。
- Line 31: fs.renameSync(tmp, filePath) の直後に、filePath の親ディレクトリを openSync して
fsyncSync し、ディレクトリ記録を永続化してください。ディレクトリ同期用ファイルディスクリプタは必ずクローズし、Windows
など同期不要または失敗し得る環境ではその挙動を明示的に許容・処理してください。
In `@video-shorts/src/av-verify.mjs`:
- Around line 17-28: Update runFfprobe to enforce a finite execution timeout for
the spawned ffprobe process, terminate it when the timeout expires, and reject
with an error that includes the command arguments. Update docs/roadmap.html to
document the timeout handling, and extend tests/av-verify-stream-check.mjs to
cover the timeout-driven FAIL path.
In `@video-shorts/tests/job-concurrency-check.mjs`:
- Around line 80-84: Extend the assertions in the concurrency test to verify the
FIFO contract by asserting that tr.startedOrder exactly matches [0, 1, 2, 3, 4].
Keep the existing concurrency-limit and exactly-once execution checks unchanged.
In `@video-shorts/tests/job-ttl-check.mjs`:
- Around line 47-53: The fallback resolver tests depend on ambient environment
variables, so make each suite deterministic. In
video-shorts/tests/job-ttl-check.mjs lines 47-53,
video-shorts/tests/job-quota-check.mjs lines 42-48, and
video-shorts/tests/job-concurrency-check.mjs lines 55-63, replace undefined
fallback inputs with explicit invalid values or isolate and clear the
corresponding environment variable before testing defaults; preserve the
assertions for configured valid values and fallback behavior.
In `@video-shorts/tests/webapp-aria-pressed-check.mjs`:
- Around line 46-50: Add the mosaic, trim, and cut chip groups to the groups
array in the aria-pressed test, using each group’s corresponding selector and
values from webapp-mockup/index.html. Preserve the existing size and subtitle
coverage so every changed toggle group is validated for synchronization.
In `@video-shorts/tests/webapp-font-size-static-check.mjs`:
- Around line 95-99: Update the font-size parsing logic around body.match and
minGuaranteedPx to find and process every font-size declaration in source order,
preserving the final declaration as the effective value. Do not skip an
explicitly declared value when minGuaranteedPx returns null; instead, propagate
a test failure for that rule, while retaining the existing out collection for
successfully parsed declarations.
In `@video-shorts/tests/webapp-mobile-layout-check.mjs`:
- Around line 47-49: Make browser runtime errors fail all checks by collecting
pageerror events and asserting the collection is empty. Update
video-shorts/tests/webapp-mobile-layout-check.mjs lines 47-49, replacing the
disconnected window.__loadErrors check; apply the same collection and pre-exit
assertion in video-shorts/tests/webapp-aria-pressed-check.mjs lines 42-44 and
video-shorts/tests/webapp-modal-focus-check.mjs lines 75-77.
In `@video-shorts/tests/webapp-mobile-layout-static-check.mjs`:
- Around line 42-56: Update extractMaxWidthMediaBody to parse all max-width
media queries, select the rule that applies at 375px, and verify that the
expected 640px breakpoint is present rather than returning the first match.
Preserve the existing balanced-brace extraction for the selected media query and
make the static check fail when the 640px rule is absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 37dfaf6c-f19e-45ed-9f5c-f6bc4ede97f7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.github/workflows/ci.yml.gitignoredocs/roadmap.htmlvideo-shorts/AGENTS.mdvideo-shorts/package.jsonvideo-shorts/pipeline.mjsvideo-shorts/server/index.mjsvideo-shorts/server/job-lifecycle.mjsvideo-shorts/server/pipeline-runner.mjsvideo-shorts/src/atomic-json.mjsvideo-shorts/src/av-verify.mjsvideo-shorts/src/srt-builder.mjsvideo-shorts/tests/av-verify-stream-check.mjsvideo-shorts/tests/helpers/launch-chromium.mjsvideo-shorts/tests/job-concurrency-check.mjsvideo-shorts/tests/job-quota-check.mjsvideo-shorts/tests/job-ttl-check.mjsvideo-shorts/tests/srt-builder-timestamp-check.mjsvideo-shorts/tests/state-atomic-write-check.mjsvideo-shorts/tests/webapp-aria-pressed-check.mjsvideo-shorts/tests/webapp-font-size-static-check.mjsvideo-shorts/tests/webapp-mobile-layout-check.mjsvideo-shorts/tests/webapp-mobile-layout-static-check.mjsvideo-shorts/tests/webapp-modal-focus-check.mjsvideo-shorts/webapp-mockup/app.jsvideo-shorts/webapp-mockup/index.htmlvideo-shorts/webapp-mockup/styles-editing.cssvideo-shorts/webapp-mockup/styles-overlay.cssvideo-shorts/webapp-mockup/styles.css
Comment on lines
+5
to
+8
| "test": "node tests/smoke.mjs && python3 tests/transcribe-corrections-check.py && python3 tests/face-mosaic-check.py && python3 tests/mosaic-ui-check.py && node tests/restart-reconnect-check.mjs && node tests/cli-job-isolation-check.mjs && node tests/dist-slim-check.mjs && node tests/render-escape-check.mjs && node tests/term-dictionary-check.mjs && node tests/caption-store-check.mjs && node tests/caption-api-check.mjs && python3 tests/term-apply-check.py && node tests/trim-plan-check.mjs && node tests/trim-duration-check.mjs && python3 tests/trim-sync-check.py && python3 tests/trim-vfr-sync-check.py && node tests/ai-caption-fix-check.mjs && node tests/state-atomic-write-check.mjs && node tests/av-verify-stream-check.mjs && node tests/srt-builder-timestamp-check.mjs && node tests/job-ttl-check.mjs && node tests/job-quota-check.mjs && node tests/job-concurrency-check.mjs && node tests/webapp-font-size-static-check.mjs && node tests/webapp-mobile-layout-static-check.mjs && node tests/webapp-mobile-layout-check.mjs && node tests/webapp-aria-pressed-check.mjs && node tests/webapp-modal-focus-check.mjs" | ||
| }, | ||
| "devDependencies": { | ||
| "playwright": "^1.48.0" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Update docs/roadmap.html in this PR.
The PR objectives defer the roadmap update to a follow-up commit. Do not defer it. This PR adds acceptance checks for P2-5, P2-7, and P2-8, so update the matching atomic-tree leaves, criteria, and verify entries before merge.
As per coding guidelines, “PR では docs/roadmap.html を必ず更新する.” Based on learnings, “進捗管理は docs/roadmap.html の原子ツリーを正とする.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@video-shorts/package.json` around lines 5 - 8, Update the matching P2-5,
P2-7, and P2-8 entries in docs/roadmap.html to reflect the newly added
acceptance checks, including their atomic-tree leaves, criteria, and verify
fields. Do not defer this roadmap update to a follow-up commit; keep the
documented progress aligned with the test coverage added in the test script.
Sources: Coding guidelines, Learnings
PR #53 のマージ前レビューで見つかった、実害のある欠陥を直した。 - Playwrightの3検査(mobile-layout/aria-pressed/modal-focus)がpageerrorを ログに出すだけでassertしておらず、ブラウザ側の実行時エラーがあっても CIが緑になっていた。イベントを集めて空であることをassertするようにした。 - TTL掃除(P2-4-A)がディレクトリのmtimeだけを見ており、実行中/待機中の ジョブを誤って削除しうる状態だった。pipeline-runnerの実行中ジョブID 一覧を掃除の除外リストとして渡すようにした。 - atomic-json.mjs がfs.writeSyncの戻り値(部分書き込み)を無視していた。 書き切るまでループするようにした。renameSync後に親ディレクトリを fsyncしていなかった点も直した(電源喪失時のrename巻き戻り対策)。 - アップロードのクォータ判定に競合状態があった(同時リクエストが全部 チェックを通過してから書き込みが始まる、Content-Lengthを計算に 入れていない)。プロセス内で予約済みバイト数を追跡する形にした。 - av-verify.mjsのffprobe呼び出しにタイムアウトが無く、壊れた入力で ハングしうた。30秒のタイムアウトを追加した。 - job-lifecycle.mjsの削除失敗(EACCES等)を握りつぶしていた。 - 環境変数への依存でTTL/クォータ/同時実行数のフォールバックテストが 不安定だった。テストごとに環境変数を退避・削除するようにした。 - aria-pressed検査がサイズ/字幕chipしか見ていなかった(mosaic/trim/cut を追加)。font-sizeの静的検査が同一ルール内の複数宣言で最初の値しか 見ていなかった(cascade通りの有効値を採用するようにした)。 - job-concurrency検査にFIFO順の検証が無かった。mobile-layout静的検査が 最初に見つかったmedia queryをそのまま使っており、より小さい ブレークポイントの規則を誤って「効いている」とみなしうた。 すべて実装を戻すと新しい検査が落ちることを確認済み。pnpm -r test 全緑。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNo27NkwpWAcFS1BkQ75B8
PR #53のCI緑（typecheck/lint/test/build、CodeQL、roadmap-required）を 確認し、P2-1〜P2-8の13葉すべてをdoneにしてevidence(CI run URL)を記録した。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNo27NkwpWAcFS1BkQ75B8
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
3 participants
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.
内容
マスター承認どおり、縦型切り抜き（G-EDIT-REFRAME、別PRで進行中）と並行してUI/UX品質（G-P2、監査P2-1〜P2-8の13葉）を実装した。
atomic-json.mjs（一時ファイル→fsync→rename）。実子プロセスのSIGKILLで書き込み中の破損が起きないことを検証。av-verify.mjsがストリーム欠落を「長さ0秒」ではなく検証失敗として扱う。0:00:59.100)が出ていたバグを修正。job-lifecycle.mjs/pipeline-runner.mjs。各葉とも AGENTS.md の凍結前探り3本（①何もしていない偽物が落ちる ②正しい実装の値を実測 ③意図的に壊したものが落ちる）を実行済み。
確認
pnpm -r test緑（video-shorts / scripts とも全PASS）node scripts/verify-roadmap-evidence.mjs緑pnpm install --frozen-lockfile緑（playwright追加後もlockfile整合）docs/roadmap.htmlの更新（各葉のstatus/evidence）は、このPRのCI run URLが確定次第、追いコミットで反映する。Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation