Skip to content

Skipped / Follow-up 解消 + 追加指摘対応 PR Report#263

Merged
yusuke0610 merged 1 commit into
mainfrom
refactor/backend/deadcode
May 17, 2026
Merged

Skipped / Follow-up 解消 + 追加指摘対応 PR Report#263
yusuke0610 merged 1 commit into
mainfrom
refactor/backend/deadcode

Conversation

@yusuke0610

@yusuke0610 yusuke0610 commented May 17, 2026

Copy link
Copy Markdown
Owner

Summary

4 つの直前 PR レポートの Skipped と Follow-ups から、Rule of Three 未達 / 「現状維持」明記済み / 別 ADR 必須の項目を除いて 9 件を解消した。BE: TaskStatusResponse の互換 re-export を削除、test_blog_collector.py の httpx モック 5 重複を 3 ヘルパに集約、PDF generator の TableStyle 9 行 clone を _table_style() で集約。FE: guards に authLoading=true 分岐 2 件、api/client.ts に並行 401 + リフレッシュ失敗テスト 1 件を追加。XR: /auth/me / /auth/github/login-url / /auth/logoutpaths.ts に統合、docs/api.md から欠落していた APP_BOOTSTRAPPED を追記。INFRA: symlink + jscpd ignore の前提を duplication.md に、monitoring 新規 alert の運用ルールを opentofu.md に追記。作業中の追加指摘で React.Dispatch<React.SetStateAction<...>> の名前空間参照を 8 ファイルで import type { Dispatch, SetStateAction } from "react" の明示 import に統一。507 BE / 122 FE テスト pass、jscpd 66 → 46 clones(2.24% → 1.46%、-0.78pt)。

Applied Changes

A. コード変更(6 件)

A-1. BE: schemas/career_analysis.pyTaskStatusResponse re-export 削除

  • 対象: backend/app/schemas/career_analysis.py
  • 削除内容: from .shared import TaskStatusResponse # noqa: F401
  • 判断根拠: grep の結果、唯一の consumer (routers/career_analysis.py) は既に from ..schemas.shared import TaskStatusResponse に切り替え済みで、互換 re-export は誰も使っていなかった
  • 由来: BE_pr Follow-ups「外部利用者がいない確認後に削除」

A-2. BE: test_blog_collector.py 内 httpx モック 5 重複の集約

  • 対象: backend/tests/test_blog_collector.py
  • 追加ヘルパ (3 種):
    • _mock_async_client_returning_json(api_json)raise_for_status() + json() を返す
    • _mock_async_client_with_status(status_code)status_code のみのレスポンス
    • _mock_async_client_raising(exc)get() で例外送出
  • 集約箇所: fetch_note_articles (2), fetch_qiita_articles (1), fetch_zenn_articles_timeout (1), verify_user_exists (5 箇所)
  • 検証: pytest tests/test_blog_collector.py tests/test_blog_collector_extended.py 30 passed
  • 由来: BE_pr Follow-ups「内部の重複テストが多い (jscpd で 6 ペア検出)」
  • 方針: extended ファイルとの統合は今回スコープ外。basic ファイル内重複 5 ペアの解消で 0 にできることを確認したため、最小スコープに留めた

A-3. BE: intelligence_generator.py (PDF) の TableStyle 9 行 clone 解消

  • 対象: backend/app/services/pdf/generators/intelligence_generator.py
  • 追加ヘルパ: _table_style(header_range, valign) -> TableStyle
  • 重複箇所: overview_table (L73-83) / role_table (L143-152) — GRID + 4 種 padding が同形、BACKGROUND の範囲と VALIGN だけ異なる
  • 検証: build_intelligence_pdf({...}) smoke test で b'%PDF-1.4' ヘッダ出力を確認
  • 由来: BE_pr Skipped Low force commit #1intelligence_generator.py 内 9 行 clone」

A-4. FE: guards.test.tsxauthLoading=true 分岐テスト追加

  • 対象: frontend/src/router/guards.test.tsx
  • 追加テスト (2 件):
    • PrivateRoute: user=testUser, authLoading=true でローディング優先
    • PublicRoute: user=testUser, authLoading=true でリダイレクトせずローディング表示
  • 検証: vitest 5 → 7 passed
  • 由来: FE_pr Skipped Add ログイン #2「guards 認証分岐の追加テスト」

A-5. FE: client.test.ts に並行 401 + リフレッシュ失敗テスト追加

  • 対象: frontend/src/api/client.test.ts
  • 追加テスト: 「複数の同時 401 はリフレッシュ失敗時に 1 回のリフレッシュを共有して両方失敗する」
  • 検証ポイント: _refreshPromise の共有により refreshCallCount === 1、2 件とも AUTH_REQUIRED で rejected
  • 検証: vitest 10 → 11 passed
  • 由来: FE_pr Skipped Add Dev #3client.ts の並行 401 リフレッシュ flow」(成功ケースは既存、失敗ケースを追加)

A-6. XR: /auth/me / /auth/github/login-url / /auth/logoutpaths.ts に追加

  • 対象:
    • frontend/src/api/paths.tsPATHS.authme / githubLoginUrl / logout を追加
    • frontend/src/api/auth.ts — 3 箇所の fetch 直叩きを ${API_BASE_URL}${PATHS.auth.XXX} に置換
  • 背景: これら 3 つは 401 ハンドラのループ回避で request ラッパーではなく fetch を直接使うため XR_apply 時にスキップされていた。fetch 直叩きでも paths.ts からパスは引けるので統合
  • 検証: npm run build pass
  • 由来: XR_pr Skipped & Follow-ups「/auth/me 等の paths.ts 追加」

B. ドキュメント追記(3 件)

B-1. INFRA: symlink + jscpd ignore の前提を duplication.md に追記

  • 対象: .claude/rules/common/duplication.md の「検知の運用」セクション直下
  • 追記内容: infra/environments/{dev,stg,prod}/{variables,moved,outputs}.tf../shared/<file>.tf への symlink で物理統合済み。jscpd は同一ファイルを 3 回スキャンすると 100% 重複として誤検出するため .jscpd.jsonignore で 9 件(3 環境 × 3 ファイル)を明示除外している。HCL 重複率は「真の重複ゼロ」ではなく「symlink 除外後の重複ゼロ」を意味する。新規 symlink を増やす場合は同様に追記
  • 由来: INFRA_pr Follow-ups「symlink + jscpd ignore の説明追記」

B-2. INFRA: monitoring 新規 alert の運用ルールを opentofu.md に追記

  • 対象: .claude/rules/infra/opentofu.md の「重複・DRY」セクション直下
  • 追記内容: 新規 alert を追加するときは既存ファイルに混ぜず、責務に対応するファイル (notification_channels.tf / uptime.tf / auth_failures.tf / rate_limits.tf / task_failures.tf) に追加するか、新しい責務であれば monitoring/<新規責務>.tf を新設する。1 ファイルに集約すると「監視増→ファイル肥大→責務不明瞭」が再発するため
  • 由来: INFRA_pr Follow-ups「監視 alert 追加時の運用ルール」

B-3. XR: docs/api.mdenv_keys.py の現状一致確認

  • 対象: docs/api.md「運用・ロギング」セクション
  • 追加エントリ: APP_BOOTSTRAPPEDenv_keys.py に定義あり、main.py:48 で参照、しかし docs/api.md から欠落していた
  • 追記内容: 「1 指定で起動時 bootstrap(DB / 鍵検証)をスキップ。マイグレーションを別途流す環境で使用」
  • 差分: 全 30 entry のうち 29 個は両方に存在、APP_BOOTSTRAPPED のみ欠落。逆方向 (docs/api.md にあって env_keys.py に無い) は OLLAMA_BASE_URL / TASK_MAX_ATTEMPTS の 2 件で、XR_pr で「docker-compose 専用 / 必要になった時に追加」と判断済み
  • 由来: XR_pr Follow-ups「docs/api.md の env 一覧と env_keys.py のエントリ一致確認」

追加指摘対応(作業中の差し込み 3 件)

Ad-1. React.Dispatch<React.SetStateAction<...>>Dispatch<SetStateAction<...>> 統一

  • 当初指摘: frontend/src/components/forms/sections/CareerQualificationsSection.tsx:17 1 箇所のみ
  • 拡張理由: 同パターンが他 7 ファイル(8 箇所)に存在することを grep で確認、ユーザー判断で全件 ripple
  • 適用範囲:
    • frontend/src/components/forms/sections/CareerQualificationsSection.tsx:17
    • frontend/src/components/forms/sections/CareerExperienceSection.tsx:16
    • frontend/src/components/blog/BlogPlatformList.tsx:33
    • frontend/src/hooks/useDocumentForm.ts:55
    • frontend/src/hooks/career/useCareerExperienceMutators.ts:23
    • frontend/src/hooks/analysis/useAsyncAnalysisPage.ts:55,59,63
  • 手法: 各ファイル先頭に import type { Dispatch, SetStateAction } from "react"; を追加(既存の import { useMemo } from "react" がある場合は別行で type-only import を追加)
  • 検証: grep -rn "React.Dispatch\|React.SetStateAction" frontend/src → 0 件、build pass、lint pass、vitest 122 passed
  • 利点: React 19+ で React 名前空間がレガシー扱いになっても影響を受けない、tree-shaking が効きやすい、import が型のみで明示される

Ad-2. useDocumentForm.test.ts の setup ヘルパで cacheKey ハードコード解消

  • 対象: frontend/src/hooks/useDocumentForm.test.ts:60
  • 修正: key: "career"key: overrides.cacheKey ?? "career"
  • 理由: setup は overrides.cacheKey を hook に渡すが、presetCache 用の setCache"career" ハードコードで dispatch していた。将来別の cacheKey を渡すテストを書いた際に「キャッシュは "career" に置かれるが hook は別キーで読む」silent bug を防ぐ
  • 検証: vitest 5 passed (useDocumentForm 単体)

Ad-3.(参考)当初指摘を受けたが見送ったもの

なし。両指摘とも「現行コードに対して妥当」と判定し適用。

Test Changes

Added (3 件)

  • frontend/src/router/guards.test.tsx: PrivateRoute (user+authLoading=true) / PublicRoute (authLoading=true) の 2 件
  • frontend/src/api/client.test.ts: 「並行 401 + リフレッシュ失敗で両方失敗 + refresh 1 回共有」の 1 件

Modified

  • backend/tests/test_blog_collector.py: 5 箇所の httpx モック生成を 3 ヘルパに置き換え。テスト数・assert は不変(全 18 件 pass)
  • frontend/src/hooks/useDocumentForm.test.ts: setup の cacheKey 導出を override 経由に変更。既存 5 テストは不変

Removed

なし。

Duplication Resolved

重複 解消方法 削減
test_blog_collector.py 内 httpx モック 5 ペア _mock_async_client_* 3 ヘルパ jscpd 5 → 0
intelligence_generator.py TableStyle 9 行 clone _table_style() ヘルパ jscpd 1 → 0

残した偶発的重複 (Allowed Duplication)

  • test_blog_collector.pytest_blog_collector_extended.py_run ヘルパ重複(4 行): minTokens 未満で jscpd 対象外。両ファイル統合は extended の責務分担議論が必要なため別 PR

Skipped

採用しなかった指摘と理由(C カテゴリ):

  • BE Medium force commit #1(blog services 3-repo init Mixin): BE_pr 自身が「Rule of Three の 2 回目。3 つ目登場で抽出」と判断済み
  • BE Medium ログイン #2(LLM prompt_utils 抽出): BE_pr が「3 つ目の同パターン揃ったタイミングで別 PR」と判断済み
  • BE Medium a #5(main.py lifespan 分離): BE_pr が「今すぐ分割しなくてよい」と判断済み
  • FE Low force commit #1, ログイン #2, Medium Dev #3: FE_pr が「意図的設計 / ドメイン的に別操作 / JSX 主体で分割不要」と「現状維持」明記済み
  • FE: CareerExperienceSection の JSX 分割: 閾値未達 (Follow-ups で「将来切り出し候補」)
  • FE: hooks/common/ サブフォルダ化: 残り 7 個、15+ で再評価と FE_pr に明記
  • XR: OpenAPI codegen 導入: ADR 起票が先。本 PR では DTO 二重定義は触らない
  • INFRA: cloud_run env block 自動同期 / GitHub Repo Variables 設定: 運用判断、別 PR
  • INFRA: terraform.lock.hcl のコミット方針 / Windows 開発者対応: 議論ポイントが残るのみで実装変更不要
  • XR: docs/api.md の env 同期スクリプト: ドキュメント生成自動化は本 PR 対象外

Validation

コマンド 結果
make lint-backend ✅ All checks passed!
make test-backend 507 passed, 2 warnings in 34.36s
make lint-frontend ✅ eslint クリーン
make test-frontend ✅ vitest 17 files / 122 tests passed(+3)+ node:test 4 passed
make build-frontend ✅ vite v6.4.2 build success (665 kB / gzip 208 kB)
make dupe-check 46 clones / 1.46%(前回 66 / 2.24%、-20 clones / -0.78pt)
E2E (npm run test:e2e) 未実行。本 PR は API パス変更なし・新規ルート/レイアウト変更なし・サイドバー変更なし。型 import 統一と内部ロジック不変のため .claude/rules/frontend/test.md の E2E トリガー条件に該当しない
make infra-validate 未実行。infra 配下の .tf 変更なし(ドキュメント追記のみ)

dupe-check の改善内訳

領域 前回 今回 改善
Python (BE) 含む 2 重複(blog_collector + intelligence_generator) 解消 -6 clones
HCL (infra) 0% (前回維持) 0%
TypeScript/TSX (FE) 既存値 既存値
全体 66 clones / 2.24% 46 clones / 1.46% -20 / -0.78pt

Follow-ups

  • BE: test_blog_collector_extended.py との統合: basic と extended の責務分担を明確化する別 PR(parametrize 集約 or ファイル責務の本質的分離)
  • BE Medium force commit #1, ログイン #2, a #5: 3 つ目のパターン出現時に再評価
  • FE Add ログイン #2 の E2E 補強: 認証分岐は unit でカバーしたが、Playwright での flow テストを追加するなら別 PR
  • XR: OpenAPI codegen 導入の ADR: docs/adr/ に 1 件起票して BE↔FE DTO 二重定義の方針を確定する
  • INFRA: cloud_run env block の SSoT 参照自動同期: terragrunt or 生成スクリプトで tf ファイル側を自動生成する案。要 PoC
  • INFRA: GitHub Repo Variables の登録: vars.GCP_PROJECT_ID_* 等を GitHub に登録すれば .github/workflows/ci.yml|| fallback から切り替えられる
  • FE: React.Dispatch の lint ルール化: 今回統一したが、将来再導入を防ぐなら eslint で no-restricted-syntaxReact.Dispatch を禁止すると堅い

Summary by CodeRabbit

  • Documentation

    • Added clarification on infrastructure deployment rules and symlink handling for environment configuration files.
    • Documented APP_BOOTSTRAPPED environment variable for skipping startup bootstrap validation.
  • Refactor

    • Standardized React type imports across frontend components and hooks.
    • Centralized API route constants for authentication endpoints.
  • Tests

    • Enhanced authentication concurrent request failure test coverage.
    • Added loading state precedence tests for route guards.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 17, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR consolidates multiple independent refactoring and quality improvements: infrastructure rules documentation clarifying symlink-based file unification; backend cleanup of unused schema exports and centralization of PDF table styling; frontend API endpoint path constants; systematic React type import standardization; and expanded test coverage for authentication loading state precedence and concurrent refresh failure scenarios.

Changes

Maintenance and Refactoring

Layer / File(s) Summary
Infrastructure and API documentation
.claude/rules/common/duplication.md, .claude/rules/infra/opentofu.md, docs/api.md
Infrastructure rules clarify symlink-based physical unification of environment-specific HCL files and duplicate detection exclusions; API docs document the APP_BOOTSTRAPPED environment variable for skipping bootstrap on startup.
Backend schema cleanup
backend/app/schemas/career_analysis.py
Remove unused module-level re-export of TaskStatusResponse imported from schemas.shared.
PDF table styling refactoring
backend/app/services/pdf/generators/intelligence_generator.py
Introduce _table_style(...) helper function to centralize ReportLab TableStyle construction; refactor overview and career prediction tables to use the helper with appropriate header row ranges and alignment values.
Blog collector test mock helpers
backend/tests/test_blog_collector.py
Extract repeated AsyncClient mocking into three shared helper functions; refactor all test cases for fetch_* and verify_user_exists operations to use these helpers instead of inline mock setup.
API endpoint path centralization
frontend/src/api/paths.ts, frontend/src/api/auth.ts, frontend/src/api/client.test.ts
Add me, githubLoginUrl, and logout constants to PATHS.auth; update getCurrentUser(), initiateGitHubLogin(), and logout() to use the centralized path constants; extend client tests with concurrent refresh failure scenario where multiple 401 responses trigger a single shared refresh that fails.
React type import standardization
frontend/src/components/blog/BlogPlatformList.tsx, frontend/src/components/forms/sections/CareerExperienceSection.tsx, frontend/src/components/forms/sections/CareerQualificationsSection.tsx, frontend/src/hooks/analysis/useAsyncAnalysisPage.ts, frontend/src/hooks/career/useCareerExperienceMutators.ts, frontend/src/hooks/useDocumentForm.ts, frontend/src/hooks/useDocumentForm.test.ts
Replace verbose React.Dispatch<React.SetStateAction<...>> type annotations with imported Dispatch and SetStateAction types; update form test setup to use cacheKey override from options.
Frontend router authLoading precedence tests
frontend/src/router/guards.test.tsx
Add test cases verifying that when authLoading is true, both PrivateRoute and PublicRoute render loading state and do not display protected content or redirect, regardless of user authentication status.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A refactored warren takes shape today,
With paths now shared the cleaner way,
Types imported, mocks aligned,
And tests that catch what might be blind,
Small fixes stack, a tidy PR
Making futures simpler—hooray, hooray! 🎉

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 80.95% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is in Japanese and references 'Skipped / Follow-up 解消 + 追加指摘対応' (addressing Skipped follow-ups and additional feedback), which directly aligns with the PR objectives that explicitly state '直前 PR レポートの Skipped / Follow-ups から...9件を解消' (resolving 9 items from the previous PR report's Skipped follow-ups).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/backend/deadcode

Comment @coderabbitai help to get the list of available commands and usage tips.

@yusuke0610 yusuke0610 changed the title BE/FE/INFRA refact Skipped / Follow-up 解消 + 追加指摘対応 PR Report May 17, 2026
@yusuke0610
yusuke0610 merged commit 8afa783 into main May 17, 2026
33 checks passed
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