■ 1. 調査の動機
- レビューが薄い理由の不明:
- 同じPRを見ても自分はLGTMで終わり、隣の人は10件の的確な指摘を出す
- 本人に聞いても「気になった箇所を見てるだけ」という答えしか返らない
- 実物を数える方針:
- GitHub APIでベテランのインラインコメントを全件取得し、主題ごとに分類する
- 対象は3リポジトリ・44本のPRに付いた187件
■ 2. 収集方法と分類軸
- 一括取得エンドポイントの利用:
- リポジトリ全体のレビューコメントを取れるエンドポイントを使い、PRを1本ずつ回すより速く集める
gh api "repos/{owner}/{repo}/pulls/comments?per_page=100" --paginateに jq で対象ユーザーを絞る- 除外処理:
- 取得した212件から、スレッド内の返信21件と本文が完全一致する重複4件を除く
- 残る187件を分析対象とする
- 3つの分類軸:
- 型は何を指摘したか（条件の誤り、デッドコード、非対称など）
- 動作は見つけるために何を開いたか（既存の別ファイル、呼び先、公式ドキュメントなど）
- 領域はそもそも何に注意を向けているか
- 領域を主題に選ぶ理由:
- レビューで手が止まる原因は「探し方を知らない」ではなく「そこに注意が向いていない」ことが大半
- よって領域の分析のほうが実務に効く
■ 3. 11領域の分布
- 領域の全体像:
- 187件は11の大領域、さらに37の中領域に分かれる
- 上位から下位までの件数:
- 1位: 隣のコードと揃っているか
- 31件、問いは「同じことをしている場所と食い違ってないか」
- 2位: 契約と意図は保存されているか
- 30件、問いは「半年後の人が同じ判断に辿り着けるか」
- 3位: 値そのものは正しいか
- 25件、問いは「この数字・この判定は合っているか」
- 4位: 失敗に気づけるか
- 17件、問いは「壊れたとき人間は知れるか」
- 5位: 使う人から見てどうか
- 16件、問いは「運用担当とユーザーの手元で何が起きるか」
- 6位: 外部との境界
- 15件、問いは「自分が書いていないものは本当にそう動くか」
- 7位/8位: データが壊れないかと設計の見通し
- ともに13件
- 9位: 速度とコスト
- 12件
- 10位: 変更はどこまで効くか
- 11件
- 11位: 検証できるか
- 4件
- バグ指摘の比率:
- 上位2領域だけで61件、全体の33%を占める
- バグ相当にあたる「値そのもの」と「データが壊れないか」は合計38件で20%にとどまる
■ 4. レイヤーによる領域の入れ替わり
- 同一人物でも対象で偏りが変わる:
- バックエンドは「値の正しさ」が20%で突出し、金額・時刻・DB制約に集中する
- モバイルは「データが壊れないか」「変更の波及」「外部SDK」に寄る
- Webフロントは「一貫性」と「意図の保存」だけで半分を占め、「外部との境界」と「検証できるか」はゼロ
- 実務上の含意:
- 全領域を毎回見る必要はなく、差分のレイヤーで見る領域を絞れる
■ 5. 1位: 隣のコードと揃っているか
- 発見経路の偏り:
- 31件のうち27件、87%が「リポジトリ内の別の場所を開いた」ことから生まれている
- 合計と明細のロジック不一致:
- 合計は
Math.abs(quantity)と異常値フォールバック0で集計するのに、各行の表示はMath.absなし・フォールバック1- 返品などで数量がマイナスになると各行を足しても合計にならない表示になる
- 合計と明細は必ずペアなので両方開く、というだけの動作で見つかる
- 別名が本番から未参照:
- 共通処理を別モジュールへ切り出す際、元クラスに委譲用の別名を5つ残していた
- 移動先が内部でモジュールグローバルを直接参照していたため、その別名は本番コードからの参照が0
- テストで @patch しても差し替わらず実物が呼ばれ、SQL定数を書き換えても実行SQLは変わらない
- 挙動不変のリファクタなのに、テストの縫い目だけが静かに偽物になっていた
■ 6. 2位: 契約と意図は保存されているか
- 領域の性格:
- バグでも設計ミスでもなく、「なぜそう書いたか」が失われることを防ぐ指摘
- マジックナンバーの根拠:
- CSSの
calc(50% + 3.5rem)を、親の幅6rem + gap 1rem = 7rem の半分ずらす計算だとレビュー側で自力で解く- そのうえで親の幅やgapを変えたときに追従修正が必要になるため、一言コメントを求める
- 合っていることを確認してから、依存が見えないことだけを指摘する
- git履歴による裏取り:
- カメラ設定を新クラスへ移行したPRで、旧実装のコミットまで遡り設定値の指定が引き継がれていないことを立証する
- 別のPRではPR説明に書かれた因果そのものを、当時のコミットを特定して反証する
- PR説明文は主張であって事実ではない、という扱い方をとる
■ 7. 3位: 値そのものは正しいか
- 領域の性格:
- 数字が1つズレると残高や集計値が静かに間違い、しかもテストは通る
- テストも同じ思い込みで書かれるため検知できない
- 単価と合計の取り違え:
- 一覧の金額表示が
price * quantityからprice単体に変わり、数量分の乗算が消えていた- API側では price は税込の単価であり、合計は掛け算して出す仕様である
- 実機で同じ取引が一覧550円・詳細230円と食い違うところまで確認して指摘している
- タイムゾーンによる期限切れの9時間ずれ:
expires_atはタイムゾーンを持たない型でUTCのつもりの日時を入れ、NOW()はタイムゾーン付きの値を返す- 型が違うためPostgreSQLは変換して比較し、タイムゾーンなしの値はセッションの TimeZone のローカル時刻として解釈される
- TimeZone が Asia/Tokyo だとUTCのつもりの値がJSTとして読まれ、実際より9時間早く期限切れと判定される
- 金銭やポイントの有効期限で起きると、まだ生きているはずのものが消える
- 指摘の書き方:
- 「今は正しく動く」ことをまず認め、なぜ今は動くのかまで書く
- 動く理由はDBコンテナの TimeZone が未指定でたまたまUTCだから、というだけである
- 効いているのはアプリ側のTZではなくDBセッション側の設定なので、DBの起動オプション1つで壊れる
- そのうえで
NOW() AT TIME ZONE 'utc'のような設定に依存しない書き方を提案する■ 8. 4位: 失敗に気づけるか
- 領域の性格:
- 機能としては正しく動くのに、壊れたことが誰にも伝わらない指摘が17件ある
- catchが発火しない設定:
- aspida のクライアントが
throwHttpErrors: false（既定値）で初期化され、await した post が400/500でも例外を投げない- try/catch で囲んでいるのに catch 節に入らず、APIが拒否しても成功バナーが出ていた
- 設定ファイル1行が「try/catch があるから安全」という直感をひっくり返す
- エラー文言の握り潰し:
- 「同じ対象へ既に登録済みの可能性があります」という親切な例外メッセージを新設した
- しかし拾うデコレータの捕捉リストに入っておらず、上位の except Exception に落ちて汎用文言に置き換わる
- 丁寧な文言を見つけたら、それが画面に到達する経路を追うという定型の動きをとる
■ 9. 5位: 使う人から見てどうか
- 領域の性格:
- コードとしては正しいが、画面の前にいる人が困る指摘
- リダイレクトで絞り込みが落ちる:
- 管理画面で特定ユーザーに絞ってから操作すると、戻り先URLにユーザーIDが渡されず絞り込みが外れる
- ページ番号だけ保持されるため、直前まで見ていた行とはまったく別の行が並ぶ
- ここまで具体化すると、単なるUXの話が誤操作のリスクに変わる
- オーバーレイの覆い漏れ:
- 処理中のローディングオーバーレイがコンテナの内側にあり、フッターのボタンを覆っていない
- 二重実行自体は別のフラグで防げているが、処理中に対象データを削除できる導線が残る
■ 10. 6位: 外部との境界
- 領域の問い:
- SDK・OS API・CLI・フレームワークなど自分が書いていないものが、本当にそう動くのか
- setterをadderと誤認した呼び出し:
- ML KitのバーコードスキャナはBuilderでフォーマットを指定する
- 公式ドキュメントに "Only the last call will be respected if calling this method multiple times" とある
- forEach で1つずつ渡すと最後の1件しか残らない
- さらに呼び出し元の定数定義まで追い、どのカードが読めなくなるかまで特定している
- Content-Type依存のボディ読み取り:
- Flaskの
request.get_json(silent=True)はmimetypeがJSONを示さなければ、ボディの中身に関わらず None を返す- 相手が正しいJSONを送ってもヘッダを付けていなければ弾かれ、外部連携の疎通当日にハマる
- silent なしの挙動はFlask 2.1で400、2.3で415に変わり、silent=True 側は昔から None のまま変わっていない
- lockファイルで実バージョンを確認してから調べている点が、この指摘の効き所である
■ 11. 7位: データが壊れないか
- 領域の性格:
- 正常系では露見せず、障害時・同時実行時・画面遷移時にだけ顔を出す
- 中間テーブルからの孤児化:
- ユーザーを作った直後は引けるが、後から所属テーブルの行が消えると取得関数のどの分岐でも引けなくなる
- レコードは残っているのに誰からも参照できない状態になる
- DB制約が前提を保証していない:
- GROUP BY user_id して MIN(company_id) を採る実装に対する指摘である
- DB制約は1ユーザー×1会社を強制しておらず、片方は会社単位ユニーク、もう片方は施設単位ユニークである
- 複数所属の行が作れた瞬間に画面では片方しか見えなくなる
- アプリが暗黙に前提とするカーディナリティを、スキーマ定義まで開いて確かめている
■ 12. 7位: 設計の見通し
- 領域の位置づけ:
- いわゆる「コードレビューらしいコードレビュー」だが、同率7位で全体の7%しかない
- nullを返しうる値へのキャスト:
- null を返す可能性がある関数の戻り値に
as stringを3回使っている- 外側で存在チェック済みなので、変数に一度入れればキャストは不要になるという3行の書き換え提案をする
- 型が効いていないことの伝え方:
- TanStack Query（v5系で確認）でクエリの meta に独自キーを渡していた
- Register インターフェースに queryMeta を宣言していないため meta の型が
Record<string, unknown>のままである- どんなキー名でも通るので、キー名をタイポしてもコンパイルで拾えない
- 「型が緩い」ではなく「タイポが通る」と言うと、直す理由がはっきりする
■ 13. 9位: 速度とコスト
- 数字で語る指摘:
- この領域の指摘はほぼ全件に実測値が入っており、「重そう」ではなく数字で言う
- インデックスと呼ばれる頻度:
- 新しく追加された検索クエリに対し、モデル定義のインデックス宣言を開いて対応するインデックスがないと確認する
- 同じクエリは別の場所にもあるが、あちらは初回1回きり、こちらは再入路である
- セッション有効期限が365日である以上、毎回のアクセスがここを通る
- インデックスの有無より、呼ばれる頻度の差を言語化しているのが効き所である
- タイムアウトの足し算:
- アプリ側のタイムアウトを40秒に設定した差分に対し、本番ロードバランサーの実測値を出す
- 実測値は直近7日で平均0.4秒台、p99が3秒台、最大18秒である
- ここで40秒使うと合計1分近くになり、ALBの idle_timeout 60秒に迫ると指摘する
- 自分より外側のタイムアウトを調べて足し算するという発想をとる
■ 14. 10位: 変更はどこまで効くか
- 領域の性格:
- バグではなく「意図した範囲を超えている」という指摘であり、PRの説明文が正しくても成立する
- propのデフォルト値:
- 共通フォーム部品に新しい表示オプションが追加され、そのデフォルトが有効になっていた
- この prop を渡していないログインや設定などの既存画面でも挙動が変わるため、opt-in を提案する
- 引数オブジェクトの破壊的変更:
- マージ関数が引数で受け取ったdictのスコアを直接書き換えるため、呼び出し側が持つ配列の中身も一緒に変わる
- 今は同じ関数の中で使い終わるので影響はないと前置きしたうえで指摘する
- あとからログや保存に回したときに混ざる、という将来の話をしている
■ 15. 11位: 検証できるか
- 領域の位置づけ:
- 件数は最少の4件だが、「テストがあるから安心」を崩すタイプなので独立させた
- モックし忘れによる実送信:
- 通知を送る経路を通るテスト3本が、通知クライアントをモックしていない
- テスト用の設定キー一覧にもその環境変数が入っておらず、変数が設定された環境でテストを流すと実際に通知が飛ぶ
- 同じファイルの他のテストはきちんとモックしており、その非対称から見つけている
- マージ前に試せない構造:
- CIワークフローのジョブに if 条件が付き、対象ブランチ以外では手動実行してもジョブごとスキップされる
- この修正が効くかをマージ前に確認できない構造である
- 指摘だけで終わらせず、一時ブランチで空打ちする具体的なコマンドまで添えている
■ 16. 数えて分かったこと
- 3分の1は劣化を止める側:
- 1位の31件と2位の30件で61件、33%を占める
- この2領域に共通するのは、指摘した時点では何も壊れていないことである
- 壊れるのは新しい選択肢を1つ足したとき、共通スタイルの値を変えたとき、片方だけ直したときであり、要するに未来の改修である
- この人のレビューはPRを通すための検査ではなく、コードベースの劣化速度を落とす作業として設計されている
- 自分はレビューをバグ探しだと思っていたため、壊れていないコードには何も言えなかった
- 気づけるかが独立した品質項目:
- 4位の17件は機能としては全部正しく動き、機能テストでは1件も落ちない
- 該当するのはsilent failure、届かないログ、外部から撃てる通知、潰されるエラー文言である
- 「動くか」だけを見ているとこの17件はゼロになる
- 「壊れたときに人間が知れるか」を品質項目として持っているかが、深掘りレビューの分かれ目である
- 69%はもう1枚開いた結果:
- 何を開いたかの軸で数えると、187件のうち129件、69%が差分の外を見た結果である
- 差分だけを上から下に読んで出せるのは58件、31%しかない
- 実力差の正体は着眼点のセンスではなく、もう1枚ファイルを開いたかどうかという作業量である
- 1位の領域は87%が別の場所を開く動作から生まれ、その大半はgrepと対になるファイルを並べるだけで専門知識をほぼ必要としない
■ 17. 自分の穴と改善計画
- 自己分析の結果:
- 同じPRに自分が出したレビューを同じ11領域に振ると、3位の値と5位の使う人には入っていた
- 1位・2位・10位には1件も入っておらず、見ていた領域が3つしかなかった
- 習慣化する6つの中領域:
- 根拠の保存は数値リテラル・固定値に「この根拠はどこに書いてあるか」と聞く動作で20件
- 対の非対称は一覧/詳細、合計/明細、iOS/Androidなど対になる名前を探して並べる動作で10件
- 二重定義は新しく定義した定数の値をgrepする動作で9件
- デッドコードは参照をgrepで数え、ガードがfalseになる条件を上流で確認する動作で8件
- silent failureはHTTPクライアントの設定を開き、catch の到達条件を全部言う動作で8件
- 変更の波及範囲は変更した部品の呼び出し元をgrepで数える動作で7件
- 着手順の根拠:
- この6つで62件、全体の33%を占める
- 最多の「根拠の保存」は20件中14件が差分を読むだけで出せる
- ファイルを1枚も開かずに始められるものがいちばん件数が多く、ここから始めるのが確実に速い
■ 18. 結論
- 差は文章力ではない:
- レビューがうまい人を真似ようとすると、言い回しや指摘の丁寧さに目が行きがちである
- 実際に187件を数えると、差がついていたのは注意の向け先が11箇所あるか3箇所しかないかであった
- 数える方法の推奨:
- すごいと思えるレビュアーが手元にいるなら、GitHub APIで全部引っ張って数えるとよい
- 半日もかからず、自分がどの領域に一度も足を踏み入れていないかが身も蓋もなく出てくる