-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: コードベース負債の修正とルール整備 #325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
040eff3
全体リファクタリング計画書を作成(Phase 0〜6)
claude c7e62c4
docs: backend 層の境界ルールを追加し CLAUDE.md に再発防止項目を追記
claude 98f4187
docs: frontend コンポーネント設計ルールを追加
claude ac2a25d
docs: CONTRIBUTING.md のブランチ戦略と ADR 一覧を最新化
claude 1e61c5d
docs: PR テンプレートと Stop hook(settings.json)を追加
claude dfd1b14
refactor: backend 層境界違反 3 件を修正(パターン A/B/C)
claude de2132a
refactor(frontend): CareerResumeFormのモーダルstateをuseCareerFormModalsに抽出
claude d00dfb1
fix(frontend): useCareerFormModals の save/deleteDoc 型を Promise<unknow…
claude 6fac823
fix: CodeRabbitレビューコメントの修正
claude 9a53ea9
fix(settings): Stop hookのdupe-checkをnix未インストール環境でスキップするよう修正
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| --- | ||
| paths: | ||
| - backend/** | ||
| --- | ||
|
|
||
| # Backend 層の境界ルール | ||
|
|
||
| `backend/architecture.md` が「何があるか」を示すのに対し、このファイルは「各層で何を書いてはいけないか」の禁止事項を補完する。 | ||
| 新しい負債を発見したら本ファイルの Bad/Good 例と `CLAUDE.md`「失敗から学んだ知見」の両方を更新すること。 | ||
|
|
||
| ## 層ごとの責務と禁止事項 | ||
|
|
||
| | 層 | 書いてよいこと | 書いてはいけないこと | | ||
| |---|---|---| | ||
| | `routers/` | エンドポイント定義・`Depends` による依存性解決・`raise_app_error` / `HTTPException` への変換・rate limit デコレータ | 外部 API 呼び出し・`db.query(...)` 直書き・ビジネスロジック・collector / fetcher の直接 import | | ||
| | `services/` | ビジネスロジック・外部 API 呼び出し・ドメイン例外の定義と raise・トランザクション管理 | `HTTPException`(HTTP 知識を service に持ち込まない)・プレゼンテーション整形 | | ||
| | `repositories/` | ORM クエリ・CRUD・`db.commit()` / `db.rollback()` | ビジネスロジック・外部 API 呼び出し・HTTP 知識 | | ||
| | `models/` | テーブル定義・リレーション・制約・DB レベルの型変換(`format_year_month` 等) | ソート・フォーマット等の表示ロジック・`sort_utils` のような presentation 層ユーティリティの import | | ||
|
|
||
| ## 禁止パターンと修正例 | ||
|
|
||
| ### パターン A — router への外部 API 例外処理の漏れ | ||
|
|
||
| ```python | ||
| # Bad: router が collector を直接 import し、外部 API 例外を自前で処理している | ||
| # routers/blog/accounts.py | ||
| from ...services.blog.collector import ( | ||
| BlogPlatformRequestError, | ||
| UnsupportedBlogPlatformError, | ||
| normalize_username, | ||
| verify_user_exists, | ||
| ) | ||
|
|
||
| @router.post("/accounts") | ||
| async def add_account(body: BlogAccountCreate, ...): | ||
| try: | ||
| normalized = normalize_username(body.platform, body.username) | ||
| except UnsupportedBlogPlatformError as exc: | ||
| raise HTTPException(status_code=400, ...) from exc | ||
| try: | ||
| user_exists = await verify_user_exists(body.platform, normalized) | ||
| except BlogPlatformRequestError as exc: | ||
| raise HTTPException(status_code=502, ...) from exc | ||
| ``` | ||
|
|
||
| ```python | ||
| # Good: service 層が外部 API 例外を吸収。router は raise_app_error への変換のみ | ||
| # services/blog/account_service.py | ||
| async def add_account(self, platform: str, username: str) -> BlogAccount: | ||
| normalized = normalize_username(platform, username) # ドメイン例外を raise | ||
| user_exists = await verify_user_exists(platform, normalized) # ドメイン例外を raise | ||
| if not user_exists: | ||
| raise BlogAccountNotFoundError(...) | ||
| return self._account_repo.upsert(platform, normalized) | ||
|
|
||
| # routers/blog/accounts.py | ||
| @router.post("/accounts") | ||
| async def add_account(body: BlogAccountCreate, ...): | ||
| service = BlogAccountService(db, user.id) | ||
| try: | ||
| return await service.add_account(body.platform, body.username) | ||
| except BlogPlatformRequestError as exc: | ||
| raise_app_error(ErrorCode.EXTERNAL_API_ERROR, ...) from exc | ||
| except BlogAccountNotFoundError as exc: | ||
| raise_app_error(ErrorCode.NOT_FOUND, ...) from exc | ||
| ``` | ||
|
|
||
| ### パターン B — router 内への DB クエリ直書き | ||
|
|
||
| ```python | ||
| # Bad: router のモジュールレベル関数が db.query を直接実行している | ||
| # routers/github_link.py | ||
| def _get_or_create_cache(db: Session, user_id: str) -> GitHubLinkCache: | ||
| cache = db.query(GitHubLinkCache).filter_by(user_id=user_id).first() | ||
| if not cache: | ||
| cache = GitHubLinkCache(user_id=user_id) | ||
| db.add(cache) | ||
| db.flush() | ||
| return cache | ||
| ``` | ||
|
|
||
| ```python | ||
| # Good: repository 層に移設。router からは service 経由で呼ぶ | ||
| # repositories/github_link.py | ||
| class GitHubLinkCacheRepository: | ||
| def get_or_create(self, user_id: str) -> GitHubLinkCache: | ||
| cache = self.db.query(GitHubLinkCache).filter_by(user_id=user_id).first() | ||
| if not cache: | ||
| cache = GitHubLinkCache(user_id=user_id) | ||
| self.db.add(cache) | ||
| self.db.flush() | ||
| # IntegrityError 後の再 SELECT が None を返す場合は RuntimeError を上げる | ||
| # (CLAUDE.md「失敗から学んだ知見」参照) | ||
| return cache | ||
| ``` | ||
|
|
||
| ### パターン C — ORM model への表示ロジック混入 | ||
|
|
||
| ```python | ||
| # Bad: model が sort_utils を import し @property でソート済みリストを返す | ||
| # models/resume.py | ||
| from ..services.shared.sort_utils import sort_by_period_desc | ||
|
|
||
| class Resume(Base): | ||
| @property | ||
| def experiences(self) -> list["ResumeExperience"]: | ||
| return sort_by_period_desc(list(self.experience_rows)) | ||
| ``` | ||
|
|
||
| ```python | ||
| # Good 案1: relationship に order_by を指定(DB レベルで解決できる場合) | ||
| # models/resume.py | ||
| experience_rows: Mapped[list["ResumeExperience"]] = relationship( | ||
| back_populates="resume", | ||
| cascade="all, delete-orphan", | ||
| order_by="ResumeExperience.start_date.desc()", | ||
| ) | ||
|
|
||
| # Good 案2: service 層でソート(動的な条件が必要な場合) | ||
| # services/shared/resume_format.py(既存)で sort_by_period_desc を呼ぶ | ||
| ``` | ||
|
|
||
| ## 例外変換の責務ルール | ||
|
|
||
| - 外部 API 固有例外(`BlogPlatformRequestError` 等)は発生するモジュール(collector / fetcher 等)内で定義し、service が raise する | ||
| - router では `raise_app_error(...)` か `HTTPException` への変換のみ行う | ||
| - service 層は `HTTPException` を import しない。HTTP ステータスコードを service に持ち込まない | ||
| - ドメイン例外クラスの定義場所: 例外を raise するモジュールと同じファイルに置く |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| --- | ||
| paths: | ||
| - frontend/** | ||
| --- | ||
|
|
||
| # Frontend コンポーネント設計ルール | ||
|
|
||
| `frontend/architecture.md` が「何があるか」を示すのに対し、このファイルは「コンポーネント設計の判断基準」を補完する。 | ||
| 行数はあくまで目安(強制閾値ではない)。超過した場合に責務が複数混在していないかを確認するトリガーとして使う。 | ||
|
|
||
| ## 行数の目安 | ||
|
|
||
| | 対象 | 目安 | 判断 | | ||
| |---|---|---| | ||
| | コンポーネント(`.tsx`) | 300 行超 | 分割を検討する | | ||
| | コンポーネント(`.tsx`) | 500 行超 | 責務が複数混在している可能性が高い。必ず分割する | | ||
| | カスタムフック(`.ts`) | 150 行超 | 分割を検討する | | ||
|
|
||
| - ページコンポーネント(`pages/`)は薄いラッパーを保つ。ロジックはカスタムフックへ移動する | ||
| - 「行数が少ないから問題ない」ではなく「責務が1つに絞られているか」を本質的な判断基準とする | ||
|
|
||
| ## props drilling の定義と Context 導入の判断基準 | ||
|
|
||
| **props drilling の定義**: 中間コンポーネントが実際には使わない props を、下位コンポーネントへ「素通し」で渡す構造。 | ||
|
|
||
| ```tsx | ||
| // Bad: ParentForm → ChildSection → GrandchildEditor で同じハンドラ群を素通し | ||
| // ChildSection は onUpdateField / onAddItem / focusLocator を自分では使わず下に渡すだけ | ||
| <ChildSection | ||
| onUpdateField={onUpdateField} | ||
| onAddItem={onAddItem} | ||
| focusLocator={focusLocator} | ||
| /> | ||
| // ChildSection の中身 | ||
| <GrandchildEditor | ||
| onUpdateField={onUpdateField} | ||
| onAddItem={onAddItem} | ||
| focusLocator={focusLocator} | ||
| /> | ||
| ``` | ||
|
|
||
| **Context 導入の判断基準**: | ||
| - 同じ props を **3 層以上素通し**する場合は Context または専用フックによる解消を検討する | ||
| - 2 層までの素通しは許容(過剰な Context 導入を避ける) | ||
| - 「素通し」か「実際に使っている」かを区別する。中間コンポーネントが props を使っていれば drilling ではない | ||
|
|
||
| **Context を導入すべき条件**: | ||
| - drilling が 3 層以上 AND 複数の並列コンポーネントが同じ状態・ハンドラを参照する場合 | ||
|
|
||
| ## モーダル管理パターン | ||
|
|
||
| 親コンポーネントに `useState` でモーダル開閉状態が 3 個以上になったら専用フックに切り出す。 | ||
|
|
||
| ```tsx | ||
| // Bad: 親コンポーネントに複数のモーダル状態が並ぶ | ||
| const [showDeleteConfirm, setShowDeleteConfirm] = useState(false); | ||
| const [showSaveConfirm, setShowSaveConfirm] = useState(false); | ||
| const [editingField, setEditingField] = useState<"career_summary" | "self_pr" | null>(null); | ||
| // さらに PdfPreviewModal の状態も... | ||
| ``` | ||
|
|
||
| ```tsx | ||
| // Good: 専用フックに切り出す(useProjectModalState パターンを参照) | ||
| // hooks/career/useProjectModalState.ts の設計に倣う | ||
| const { isOpen, openModal, closeModal, modalProject } = useProjectModalState(formState); | ||
|
|
||
| // 複数のモーダルを1フックにまとめる(関連度が高い場合) | ||
| const { deleteConfirm, saveConfirm, markdownField, openDeleteConfirm, ... } = useCareerFormModals(); | ||
| ``` | ||
|
|
||
| ## 既存の良いパターンへの参照 | ||
|
|
||
| 新規実装時は以下を規範として参照する。 | ||
|
|
||
| | パターン | ファイル | 用途 | | ||
| |---|---|---| | ||
| | フォーム CRUD 共通化 | `hooks/useDocumentForm.ts` | loading/saving/error/cache 状態を一元管理 | | ||
| | モーダル状態の切り出し | `hooks/career/useProjectModalState.ts` | モーダル開閉・対象オブジェクト管理の規範例 | | ||
| | 更新ハンドラ群の切り出し | `hooks/career/useCareerExperienceMutators.ts` | 複数の mutation ハンドラをフックに集約 | | ||
| | 非同期タスク進捗 | `hooks/useTaskPolling.ts` | ポーリングロジックをフックに切り出した例 | | ||
| | 汎用 UI コンポーネント | `components/ui/` | 新規共通 UI の置き場(toast/, Skeleton 等が既存) | |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "hooks": { | ||
| "Stop": [ | ||
| { | ||
| "matcher": "", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "command -v nix >/dev/null 2>&1 && make dupe-check || true" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| ## 変更概要 | ||
|
|
||
| <!-- 何を変えたか・なぜ変えたかを 1〜3 行で --> | ||
|
|
||
| ## セルフレビューチェックリスト | ||
|
|
||
| ### 必須確認 | ||
|
|
||
| - [ ] `make ci` が pass している(lint + test + build-frontend) | ||
| - [ ] コメント・ドキュメント・エラーメッセージは日本語で記述した | ||
|
|
||
| ### 条件付き確認(該当する場合のみ N/A と記入) | ||
|
|
||
| - [ ] `app/schemas/` または `app/routers/` を変更した場合: `make codegen-types` を実行し `frontend/src/api/generated.ts` の差分をコミットした | ||
| - [ ] 新しいページ・認証・ナビゲーション・レイアウトを変更した場合: E2E を実行した(`nix develop --command bash -c "cd frontend && npm run test:e2e"`) | ||
| - [ ] 新規環境変数を追加した場合: `env_keys.py` / `docs/api.md` / `infra/modules/cloud_run/main.tf` / `docker-compose.yml` の 4 箇所を同期した | ||
| - [ ] `frontend/src/` で日本語メッセージを `frontend/src/constants/messages.ts` の定数経由で参照した(リテラル直書きなし) | ||
|
|
||
| ### 破壊的変更 | ||
|
|
||
| - [ ] 破壊的変更なし(API 契約・DB スキーマ・既存の公開インターフェースに変更なし) | ||
| - [ ] 破壊的変更あり → 概要: <!-- ここに記入 --> | ||
|
|
||
| ### ADR(設計判断を伴う変更の場合のみ) | ||
|
|
||
| - [ ] 新しいライブラリ採用・アーキテクチャ変更を伴う場合、ADR を作成した(または既存 ADR が対応している) | ||
|
|
||
| --- | ||
|
|
||
| PR タイトル形式: `<type>: <内容>`(日本語) | ||
| type: `feat` / `fix` / `docs` / `refactor` / `test` / `chore` / `infra` | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the PR title format text.
Line 19 omits the required
<type>placeholder, even though the template later lists the correcttype: <内容>convention. As written, this will steer contributors toward malformed titles.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents