Skip to content

コメントのついているPRでも「アサインされました」と通知される #116

Description

@limit7412

コメントのついている PR でも、reason=assign の通知が初回向け文言「アサインされました」のままになる。#104 で導入したフォローアップ文言(「担当している PR/Issue に動きがありました」)に切り替わらないケースがある。


方針

症状

#104 の対応後も、コメントが付いている PR への通知(reason=assign)で「アサインされました」が表示されることがある。フォローアップ文言への切り替え条件 Subject#commented?src/github/models.cr)が false を返しているとみられる。

原因(仮説)

commented? は「latest_comment_url が空でなく、かつ subject.url と異なる」ことをコメントありの根拠にしている。しかし実際の /notifications ペイロードでは、スレッドにコメントがあっても latest_comment_urlsubject.url と同値(または空)で返るケースがあると考えられる。候補は 2 つ:

  1. latest_comment_url が「スレッドの現在の最新コメント」ではなく「通知を発生させた最新イベント」を反映していて、push・レビュー・アサイン変更などの非コメントイベントが起点の通知では subject.url に戻る
  2. PR のレビューコメント(Files changed 上のコメント)やレビュー本文が latest_comment_url に反映されず、会話タブの issue comment だけが反映される(レビューでのみ議論されている PR は常に false になる)

どちらのケースでも commented? が false になり、初回文言に落ちる。#104 は「latest_comment_url はスレッドの最新コメントを指すため、コメント済みスレッドへの push でも真になる」という前提だったが、この前提が実ペイロードと合っていない可能性が高い。

検証手順

Lambda は取得した通知ペイロードを全件ログ出力している(Github::NotificationRepository#find_notifications_unreadnotifications body (page N))。CloudWatch Logs(ap-northeast-1 / /aws/lambda/github-notifications-slack-*)で reason=assign かつコメントが付いている PR の通知を探し、その latest_comment_urlsubject.url の実際の値を確認して、仮説 1 / 2 のどちらか(または両方)を確定させる。

未実施: AWS の認証が切れており本セッションからログを参照できなかった。ただし後述の対応は仮説 1 / 2 のどちらでも同じ経路で解消するため、特定を待たずに実装を進めた。原因の確定は事後でよく、確定した内容によっては制約(下記)の見直しに使う。

対応方針

latest_comment_url 単独の判定をやめ、既に毎回取得している subject 本文のコメント数を使う。追加の API コールは不要。

Github::Usecase#build_message は本文表示のために subject.comment_url を毎回 GET している。commented? が false のとき、この URL は PR / Issue オブジェクト自体を指す(comment_url のフォールバック仕様)。そして PR レスポンスには comments(会話タブのコメント数)と review_comments(レビューコメント数)、Issue レスポンスには comments が含まれる。この取得済みレスポンスから件数を拾えば「スレッドにコメントが存在するか」を正確に判定できる。

判定ロジック:

followup 適用 ⇔ subject.commented? || (取得済み本文の comments + review_comments が 1 以上)
  • commented? が true(latest_comment_url が実コメントを指す)→ 従来どおり followup。このとき取得済み本文はコメントオブジェクトで件数フィールドは無い(nil)が、判定は既に true なので影響なし
  • commented? が false → 取得済みの PR / Issue オブジェクトの件数で判定

実装箇所

  • src/github/models.cr
    • Commentcomments : Int32? / review_comments : Int32?#commented? を追加(コメントオブジェクトには存在しないフィールドなので nilable)
    • Notification#reason_message / #pretext が取得済み Comment を任意引数で受け取り、新設の #followup?FOLLOWUP_MESSAGES への切り替えを判定
    • Subject#commented? の doc に、偽でもコメントが無いとは限らない(片道の判定である)ことを追記
  • src/github/usecase.cr: build_message で取得済み commentpretext 生成に渡す
  • spec/github/models_spec.cr / spec/github/usecase_spec.cr
    • PR: comments > 0 / review_comments > 0 / 両方 0 / フィールド無し(nil)の各パターン
    • Issue: comments > 0 の切り替え
    • Comment#commented? 単体と、usecase 経由の pretext

制約・スコープ外

  • コメント 0 件でレビュー(approve のみ等)だけが付いた PR は comments / review_comments とも 0 のため従来どおり「アサインされました」になる。レビューの有無は PR レスポンスに含まれず追加 API が必要なため、今回は許容する
  • 本文取得に失敗した通知は判定材料が無いため従来文言のまま
  • review_requested も同じ FOLLOWUP_MESSAGES テーブルと判定を使うため、同種の取りこぼしが同時に改善される

対応 PR

#117

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions