はじめに
「あ、このバグ、前にも本番障害になったやつだ...」
こんな経験はありませんか？
一度修正したはずのバグが、別の場所で再発する。レビューで気づければいいけど、人間は忘れる。ESLintなどの静的解析でも検出できない。
Hubbleのフロントエンドでは、過去にレビューで見逃されやすかったバグをDevinのナレッジに登録し、機械的に再発を検出する仕組みを導入しています。
前提：Devinによる事前レビュー
Cognitionの公式ブログ記事を参考に、GitHub ActionsとDevinを組み合わせて人がレビューする前に、Devinによる自動レビューの仕組みを実装することができます。
この仕組みは @KOHETs が実装してくれました。
その結果、Hubbleのフロントエンドでは、PRが作成・更新・再オープンされたときに自動的にDevinによるコードレビューが実施されます。
なぜナレッジに登録するのか
ESLintなどでは検出できないバグがある
以下のようなバグは、ツールでの検出が難しいです：
- 特定のファイル種別でのみ発生するもの
- コード単体では正しいが、非同期のタイミングで問題になるもの
- 文法的には正しいが、業務ロジック上NGとなるもの
人間は忘れる
「XXXファイル内でYYYを使う場合、必ずZZZフラグをチェックする」
このルール、チーム全員が常に覚えていられるでしょうか？
新しいメンバーが入ってきたら？半年後に同じパターンを書くことになったら？
人間は忘れる。だから機械に覚えておいてもらう。
実例：GuardファイルでのSelector使用パターン
Angularアプリケーションでは、ルートガード（Guard）を使用して、ユーザーが特定のルートにアクセスできるかどうかを制御します。
APIで取得したデータを元にアクセス権限を判定するケースがありますが、毎回APIを呼ぶのはコストが高いため、HubbleではデータをNgRxストアに保持し、Guard側でストアの状態を参照して判定するパターンを採用しています。
ここで問題となるのは、非同期で読み込まれるデータを扱う際のタイミング問題です。データが読み込まれる前に判定処理が実行されると、意図しない動作になる可能性があります。
ナレッジへの登録
このパターンをナレッジに登録することで、Devinが自動的にこの問題を検出できるようになります。
登録内容の例：
重要: 以下のレビューパターンは、Guardファイル（XXX.guard.ts）のみを対象としています。それ以外のファイルでは指摘しないでください。
## 1. XXX使用時の必須検証
### 概要
以下のコードは、`selectHoges`を使用していますが、`loaded`状態をチェックしていないため、データが読み込まれる前に空配列を処理してしまう可能性があります。
### 不具合パターン（必ず指摘すること）
```typescript
// ❌ NGパターン - loadedフラグをチェックせずにhogesを使用
this.store.pipe(
select(hogeSelectors.selectHoges),
map(({ hoges }) => {
return hoges.some((hoge) => hoge.code === target);
}),
);
```
### 正しいパターン
```typescript
// ✅ 正しいパターン - 必ずfilter(({ loaded }) => loaded)でチェック
this.store.pipe(
select(hogeSelectors.selectHogesStatus),
filter(({ loaded }) => loaded), // ← この行が必須！
map(({ hoges }) => {
return hoges.some((hoge) => hoge.code === target);
}),
);
```
### 理由
- `selectHoges`は配列のみを返すため、データが読み込まれたかどうかを判断できない
- `selectHogesStatus`は`{ loaded, hoges }`の形式で返すため、loadedフラグを確認してから安全にhogesを使用できる
- loadedフラグをチェックしないと、データ読み込み前に空配列を処理してしまい、意図しない動作になる可能性がある
### 重要度
**高優先度** - hogesを使用する箇所では、必ずloadedフラグの確認が必要です。`selectHoges`を直接使用している場合は、高優先度の不具合として必ず指摘してください。
※補足
ナレッジの概要に記載している下記の説明について：
以下のコードは、selectHogesを使用していますが、loaded状態をチェックしていないため、データが読み込まれる前に空配列を処理してしまう可能性があります。
「Selector側でloaded判定を入れればよいのでは？」と思われるかもしれませんが、Selectorはあくまで“状態を同期的に加工するだけ”の仕組みで、RxJSのfilterのように「値が揃うまで待つ」という時間的な制御は行えません。そのため、Guard側でfilter(({ loaded }) => loaded)を使って明示的に待つ必要があります。
ナレッジの解説
-
NG / OK のコード例を明示すると、Devin が不具合として認識しやすい
-
理由も添えることで、指摘意図を正確に伝えられる
あとは、ナレッジのDevin uses this when...に下記を設定することでGuardファイルの時のみチェックしてくれます。
※Devinは使うだけお金がかかるので、必要な時だけレビューしてもらうため
When reviewing TypeScript guard files (*.guard.ts) in pull requests
実際のレビュー指摘
ナレッジ登録後、わざと問題のあるコードでPRを作成したところ...
ちゃんとDevinが指摘してくれました 🎉
まとめ
レビューで見逃すとクリティカルになりそうなものはDevinに事前レビューしてもらうのが良いと思います。
理想は仕組みやツールで防げる状態で、どうしても防げないものだけをナレッジに登録する。
そのため、ナレッジは少ないほど良いと考えています。