Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions docs/refactoring/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
# リファクタリング計画 全体像

最終更新: 2026-06-11

このディレクトリは、shift2 に蓄積した技術的負債を段階的に解消するためのリファクタリング計画です。各フェーズは独立したドキュメントになっており、上から順に着手することを前提に依存関係を設計しています。

## 背景: 現状の負債サマリ(2026-06-11 調査)

| 領域 | 規模 | 主な問題 |
|------|------|----------|
| `index.html` | 3,005 行 | 約 9 割(約 2,700 行)が埋め込み CSS。25 以上のクラスが重複定義。インライン `onclick` が 20 箇所 |
| `js/modules/` | 16 ファイル・約 5,800 行 | ES modules ではなく `<script>` タグ読み込み順依存。`timeToMinutes` 等の重複関数。`api.js` を迂回した直接 `fetch` が 7 箇所。旧 UI のデッドコード |
| `server/src` | 約 4,000 行 | ルートの try/catch 定型が 37 箇所・`res.status(500)` が 49 箇所。5 モデルにほぼ同一の CRUD が約 48 メソッド。`CalendarService.ts` が 863 行・16 責務 |
| フロントテスト | 17 ファイル・約 5,100 行 | **実コードを import せず、テスト内に実装のコピーを定義する運用**(CLAUDE.md 公認)。実装との乖離リスクが構造的に存在 |
| バックテスト | 8 ファイル・約 3,200 行 | 全依存をモック化しモック値を検証するだけのテスト、実装を呼ばないインラインコピーのテストが混在 |
| E2E | 2 ファイル | 全テストが `test.skip` で無効。CI 未接続。`baseURL` のポートも実態と不一致 |
| CI / カバレッジ | — | フロントのカバレッジ計測対象が空。閾値 statements 3%。backend-tests ステップは `\|\| echo` で失敗を握りつぶす |
| ドキュメント | — | CLAUDE.md の「アーキテクチャ」節が旧構成(ポート 8081、Client ID ハードコード等)のまま |
| リポジトリ衛生 | — | `.serena/` が追跡されている、`help.html` に独立した重複 CSS(約 265 行) |

## フェーズ構成と依存関係

```
Phase 0: リポジトリ衛生とドキュメント整合 (挙動変更なし・即効)
Phase 1: テスト基盤の再建 (以降のフェーズの安全網)
Phase 2: バックエンド構造 ┐
Phase 3: フロントエンド構造 ├ 1 完了後は並行可能
Phase 4: CSS / HTML ┘(ただし同時着手は 2 つまで推奨)
Phase 5: E2E と CI 品質ゲート (全フェーズの成果を固定化)
```

| フェーズ | ドキュメント | 目安規模 |
|----------|--------------|----------|
| 0 | [phase-0-hygiene.md](phase-0-hygiene.md) | PR 2〜3 本・小 |
| 1 | [phase-1-test-foundation.md](phase-1-test-foundation.md) | PR 5〜8 本・大 |
| 2 | [phase-2-backend.md](phase-2-backend.md) | PR 6〜9 本・大 |
| 3 | [phase-3-frontend.md](phase-3-frontend.md) | PR 5〜7 本・大 |
| 4 | [phase-4-css-html.md](phase-4-css-html.md) | PR 3〜4 本・中 |
| 5 | [phase-5-e2e-ci.md](phase-5-e2e-ci.md) | PR 3〜5 本・中 |

## 全フェーズ共通の原則

1. **挙動を変えない。** リファクタリング PR に機能追加・バグ修正を混ぜない。バグを見つけたら別 PR(TDD: 再現テスト → 修正)で先に直す。
2. **1 PR = 1 ステップ。** 各フェーズ文書の「作業項目」1 つを 1 PR にする。レビュー可能な差分量(目安 ±400 行以内、機械的な一括置換を除く)を守る。
3. **テストが先。** 対象コードにテストがなければ、現在の挙動を固定する特性テスト(characterization test。現状の出力をそのまま期待値にするテスト)を先に書いてから動かす。
4. **CLAUDE.md のブランチ運用に従う。** main 直 push 禁止、feature ブランチ → PR → マージ。
5. **各 PR で全テスト green + ローカルでの手動スモーク**(ログイン → シフト申請 → 一覧表示 → キャンセル)を確認する。
6. **計測してから消す。** デッドコード削除は「全ファイル grep で参照ゼロ」を PR 説明に明記する。

## 進捗の付け方

各フェーズ文書内のチェックリストを PR マージ時に更新する(チェック更新はそのリファクタ PR に含めてよい)。フェーズ完了時にこの README の下表を更新する。

| フェーズ | 状態 | 完了日 |
|----------|------|--------|
| 0 | 未着手 | — |
| 1 | 未着手 | — |
| 2 | 未着手 | — |
| 3 | 未着手 | — |
| 4 | 未着手 | — |
| 5 | 未着手 | — |

## 効果測定(before / after)

フェーズ完了ごとに以下を記録する。

```bash
# ファイル規模
wc -l index.html js/modules/*.js | tail -1
find server/src -name "*.ts" ! -path "*__tests__*" | xargs wc -l | tail -1

# テスト
npm test 2>&1 | tail -4

# カバレッジ
npm run test:coverage 2>&1 | tail -10
```

ベースライン(2026-06-11): フロント約 8,800 行(index.html 含む)/ バックエンド本体約 4,000 行 / テスト 366 件 green。
59 changes: 59 additions & 0 deletions docs/refactoring/phase-0-hygiene.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# Phase 0: リポジトリ衛生とドキュメント整合

**ゴール**: 挙動を一切変えずに、誤った情報源・無意味なファイル・設定の矛盾を排除する。以降のフェーズで人(と AI エージェント)が古い情報に騙されない状態を作る。

**前提**: なし(いつでも着手可能)

## 背景

- `CLAUDE.md` の「アーキテクチャ」「Google OAuth 設定」節は初期のスタティック SPA 時代の記述のまま(CLAUDE.md:175-181 の「認可済みオリジン: localhost:8081」「Client ID は index.html と app.js にハードコード」等)。現在は Express がポート 3000 で配信し、Client ID は `/api/config` から実行時取得(js/modules/config.js)であり、事実と異なる。
- `playwright.config.ts:11` の `baseURL: 'http://localhost:8080'` が開発サーバー(3000)と不一致。
- `.serena/` (外部 AI ツールの設定)が git 追跡されている。
- `app.js`・`js/modules/shiftRequest.js` 等に旧 UI のデッドコードが残存。

## 作業項目

### 0-1. CLAUDE.md / README.md の実態同期(PR 1 本)

- [ ] CLAUDE.md の「アーキテクチャ」節(ファイル構成・認証フロー・Google OAuth 設定)を現構成に書き換える
- ファイル構成: `index.html` + `js/modules/*.js`(16 ファイル)+ `server/src`(Express + TS)
- Client ID: `.env` → `/api/config` → `config.js` の実行時取得フローを記載
- ポートは 3000 に統一(8081 への言及を全削除)
- [ ] 「実装上のポイント」節の `decodeJwtResponse` / `#loginSection` 等の記述が現コードと一致するか検証し、ずれていれば修正
- [ ] README.md のセットアップ手順を一読し、現在の `npm run dev` フローと一致させる
- [ ] CLAUDE.md の「フロントエンドテストの慣習」節に「Phase 1 で実コード import 方式へ移行予定」と注記(Phase 1 完了時に節ごと書き換え)

**受け入れ条件**: ドキュメント内の全ポート番号・全ファイルパスが実在のものと一致する。

### 0-2. 設定ファイルの矛盾解消・不要ファイル削除(PR 1 本)

- [ ] `playwright.config.ts` の `baseURL` をサーバー実ポートに合わせる(`webServer` にも `port` を明示)
- [ ] `.serena/` を git 追跡から外す(`git rm -r --cached .serena` + `.gitignore` 追加)。チームで Serena を使っていないなら削除
- [ ] `codecov.yml` / `jest.config.ts` のカバレッジ閾値に「現状値は暫定。Phase 5 で引き上げ」のコメントを付ける(数値変更は Phase 5)
- [ ] `.github/workflows/test.yml` の backend-tests ステップの `|| echo "Backend tests not yet configured"` を削除(バックエンドテストは既に 154 件存在しており、失敗の握りつぶしは危険)

**受け入れ条件**: CI が green のまま。`git ls-files | grep .serena` が空。

### 0-3. 明白なデッドコードの削除(PR 1 本)

削除前に**全ファイル grep で参照ゼロを確認し、その証跡を PR 説明に記載**する。

- [ ] `js/modules/shiftRequest.js` の旧 UI 関数群: `openShiftRequestModal`(:177)、`updateTimeSlotCapacity`(:205)、`generateTimeSlots`(:253)、`shiftRemarks` 参照(:311)— HTML 側に対応要素がないことを確認の上削除
- [ ] `index.html` の `display:none` で恒久的に隠されているタブ(:2751, :2753 付近)が本当に不要か確認し、不要なら HTML・関連 JS ごと削除
- [ ] `server/src/services/CalendarService.ts` の後方互換ラッパー: `addShiftToCalendar`(単なる委譲)、`deleteShiftFromCalendar`(コメントに「後方互換性のために残されています」)— 呼び出し元ゼロを確認して削除
- [ ] 上記削除で参照が消えるテスト(コピー実装をテストしているもの)も同時に削除

**受け入れ条件**: 全テスト green。手動スモーク(ログイン → 申請 → 一覧 → キャンセル)が通る。

## リスクと対策

| リスク | 対策 |
|--------|------|
| 「デッドコード」が実は動的参照されている(`onclick="..."` 文字列内など) | grep は関数名の文字列一致で行い、`index.html`・`help.html` の属性値も対象に含める |
| `.serena` 削除が他メンバーの環境を壊す | 追跡から外すだけにし、ローカルファイルは残す(`--cached`) |

## 完了条件(Definition of Done)

- CLAUDE.md / README.md / 各種設定のポート・パス・手順がすべて実態と一致
- 旧 UI デッドコードと後方互換ラッパーが削除され、全テスト green
- CI でバックエンドテストの失敗が正しく fail として扱われる
79 changes: 79 additions & 0 deletions docs/refactoring/phase-1-test-foundation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# Phase 1: テスト基盤の再建

**ゴール**: 「テストが実コードを検証している」状態を作り、Phase 2〜4 の構造変更を安全に行える安全網を確立する。

**前提**: Phase 0 完了(デッドコードが消えており、消えたコードのテストを書かずに済む)

## 背景

現在のテストは量(フロント約 5,100 行 + バック約 3,200 行、計 366 件)の割に、リファクタリングの安全網としてほとんど機能しない。

1. **フロントテストは実コードを import していない。** CLAUDE.md 公認の運用として、テストファイル内に「期待挙動を表現するインライン関数」を定義してそれをテストしている(例: `test/special-shift-slots.test.js:18-42` に `buildSpecialShiftSlots` のコピー、`test/special-shift-name.test.js:239-296` に `displayMyShifts` 相当の再実装)。実装側を壊してもテストは緑のまま。
2. **バックテストにも実装を呼ばないテストがある。** `server/src/__tests__/CalendarService.test.ts:56-149` は CalendarService を呼ばず、テスト内のローカル変数やコピー関数を検証している(実質トートロジー)。
3. **ルートテストは全依存モック**(モデル・CalendarService・db)で、HTTP 層の分岐は検証できているが、SQL を含むモデル層の実挙動は未検証。

## 方針

- フロント `js/modules/*.js` を **挙動を変えずに Jest から require 可能**にする。ブラウザ側は従来どおり `<script>` タグで動くよう、各ファイル末尾に CommonJS ガード付きエクスポートを追加する方式を採る(完全な ES modules 化は Phase 3 末で実施。ここでは最小変更に留める):

```js
// ファイル末尾に追加(ブラウザでは無視される)
if (typeof module !== 'undefined' && module.exports) {
module.exports = { buildSpecialShiftSlots, timeToMinutes, minutesToTime };
}
```

- バックエンドは **better-sqlite3 のインメモリ DB(`:memory:`)を使った統合テスト**を導入し、モデル層と「ルート + 実 DB」の縦の検証を可能にする。

## 作業項目

### 1-1. テスト棚卸しと分類(PR 1 本、ドキュメントのみ)

- [ ] 全テストファイルを「A: 実コードを検証」「B: コピー実装を検証(書き換え対象)」「C: トートロジー(削除対象)」に分類し、`docs/refactoring/test-inventory.md` として記録
- [ ] B のテストごとに、対応する実関数(ファイル・関数名)を対応表にする

### 1-2. js/modules にエクスポートガードを追加(PR 1 本)

- [ ] 純粋ロジックを持つモジュールから着手: `utils.js`、`shiftRequest.js`(`buildSpecialShiftSlots`/`timeToMinutes`/`minutesToTime`)、`specialShifts.js`、`state.js`
- [ ] ブラウザ挙動が不変であることを手動スモークで確認(`module` 未定義ガードにより no-op)
- [ ] `jest.config.ts` の frontend プロジェクトで `collectCoverageFrom` に `js/modules/**` を設定(閾値はまだ上げない)

**受け入れ条件**: 既存テスト全 green、ブラウザ動作不変、`require('../js/modules/utils.js')` がテストから成功する。

### 1-3. フロントテストの実コード接続(PR 3〜5 本に分割)

分類 B のテストを 1 ファイル〜数ファイルずつ書き換える。

- [ ] テスト内のコピー実装を削除し、実モジュールの import に置換
- [ ] **置換時に実装とコピーの差分を必ず確認する。** 乖離が見つかった場合はどちらが正かを判断し、バグなら別 PR で TDD 修正(このフェーズの PR に混ぜない)
- [ ] DOM 依存の強い関数(`display*` 系)は、jsdom + `test-utils.js` のフィクスチャで実関数を呼ぶ形に書き換える。書き換えコストが高すぎるものは「E2E でカバーする」と判断して削除し、Phase 5 の E2E 対象リストに追記
- [ ] 分類 C(トートロジー)のテストを削除: `CalendarService.test.ts:56-149` のコピー検証 describe、`test/shift-delete-functions.test.js` の自明 expect 等

**受け入れ条件**: `test/` 配下に実装コピー関数が残っていない(`grep -rn "function buildSpecialShiftSlots\|function mergeConsecutiveTimeSlots" test/` が空)。

### 1-4. バックエンド統合テストの導入(PR 2 本)

- [ ] `server/src/database/db.ts` を「スキーマ定義」と「接続生成」に分離し、テストから `:memory:` DB を注入できるようにする(既存 import 互換の default export は維持)
- [ ] モデル統合テスト: 5 モデルの CRUD を実 SQL で検証(`server/src/__tests__/integration/models.test.ts`)
- [ ] ルート統合テスト: supertest + 実 DB(CalendarService のみモック)で、申請 → 取得 → 削除の代表フローを検証

**受け入れ条件**: モデル層の SQL がテストで実行される(カバレッジで models/ が計測される)。

### 1-5. CLAUDE.md の運用ルール更新(PR 1 本)

- [ ] 「フロントエンドテストの慣習」節を削除し、「テストは実コードを import する。エクスポートガード方式」の新ルールに書き換える
- [ ] 新規テストでコピー実装方式を禁止する旨を明記

## リスクと対策

| リスク | 対策 |
|--------|------|
| コピー実装と実装の乖離が「テスト書き換え」で大量に発覚する | 乖離は発見ごとに記録し、修正はバグ修正 PR として分離。書き換え PR は「実装の現挙動」を正としてテストを合わせる |
| エクスポートガード追加漏れ・タイポでブラウザが壊れる | 追加は機械的な末尾追記のみとし、PR ごとに手動スモーク必須 |
| `db.ts` 分離が本番 DB パスに影響 | default export の生成ロジック(パス解決)は変更せず、関数抽出のみ行う |

## 完了条件(Definition of Done)

- テスト内の実装コピーがゼロ、トートロジーテストがゼロ
- フロントの主要ロジック関数・バックエンドのモデル層がカバレッジ計測に乗っている
- 全テスト green、CI green
Loading
Loading