コメントのついている 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_url が subject.url と同値(または空)で返るケースがあると考えられる。候補は 2 つ:
latest_comment_url が「スレッドの現在の最新コメント」ではなく「通知を発生させた最新イベント」を反映していて、push・レビュー・アサイン変更などの非コメントイベントが起点の通知では subject.url に戻る
- PR のレビューコメント(Files changed 上のコメント)やレビュー本文が
latest_comment_url に反映されず、会話タブの issue comment だけが反映される(レビューでのみ議論されている PR は常に false になる)
どちらのケースでも commented? が false になり、初回文言に落ちる。#104 は「latest_comment_url はスレッドの最新コメントを指すため、コメント済みスレッドへの push でも真になる」という前提だったが、この前提が実ペイロードと合っていない可能性が高い。
検証手順
Lambda は取得した通知ペイロードを全件ログ出力している(Github::NotificationRepository#find_notifications_unread の notifications body (page N))。CloudWatch Logs(ap-northeast-1 / /aws/lambda/github-notifications-slack-*)で reason=assign かつコメントが付いている PR の通知を探し、その latest_comment_url と subject.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
Comment に comments : Int32? / review_comments : Int32? と #commented? を追加(コメントオブジェクトには存在しないフィールドなので nilable)
Notification#reason_message / #pretext が取得済み Comment を任意引数で受け取り、新設の #followup? で FOLLOWUP_MESSAGES への切り替えを判定
Subject#commented? の doc に、偽でもコメントが無いとは限らない(片道の判定である)ことを追記
src/github/usecase.cr: build_message で取得済み comment を pretext 生成に渡す
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
コメントのついている 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_urlがsubject.urlと同値(または空)で返るケースがあると考えられる。候補は 2 つ:latest_comment_urlが「スレッドの現在の最新コメント」ではなく「通知を発生させた最新イベント」を反映していて、push・レビュー・アサイン変更などの非コメントイベントが起点の通知ではsubject.urlに戻るlatest_comment_urlに反映されず、会話タブの issue comment だけが反映される(レビューでのみ議論されている PR は常に false になる)どちらのケースでも
commented?が false になり、初回文言に落ちる。#104 は「latest_comment_url はスレッドの最新コメントを指すため、コメント済みスレッドへの push でも真になる」という前提だったが、この前提が実ペイロードと合っていない可能性が高い。検証手順
Lambda は取得した通知ペイロードを全件ログ出力している(
Github::NotificationRepository#find_notifications_unreadのnotifications body (page N))。CloudWatch Logs(ap-northeast-1 //aws/lambda/github-notifications-slack-*)で reason=assign かつコメントが付いている PR の通知を探し、そのlatest_comment_urlとsubject.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が含まれる。この取得済みレスポンスから件数を拾えば「スレッドにコメントが存在するか」を正確に判定できる。判定ロジック:
commented?が true(latest_comment_url が実コメントを指す)→ 従来どおり followup。このとき取得済み本文はコメントオブジェクトで件数フィールドは無い(nil)が、判定は既に true なので影響なしcommented?が false → 取得済みの PR / Issue オブジェクトの件数で判定実装箇所
src/github/models.crCommentにcomments : Int32?/review_comments : Int32?と#commented?を追加(コメントオブジェクトには存在しないフィールドなので nilable)Notification#reason_message/#pretextが取得済みCommentを任意引数で受け取り、新設の#followup?でFOLLOWUP_MESSAGESへの切り替えを判定Subject#commented?の doc に、偽でもコメントが無いとは限らない(片道の判定である)ことを追記src/github/usecase.cr:build_messageで取得済みcommentをpretext生成に渡すspec/github/models_spec.cr/spec/github/usecase_spec.crcomments > 0/review_comments > 0/ 両方 0 / フィールド無し(nil)の各パターンcomments > 0の切り替えComment#commented?単体と、usecase 経由の pretext制約・スコープ外
comments/review_commentsとも 0 のため従来どおり「アサインされました」になる。レビューの有無は PR レスポンスに含まれず追加 API が必要なため、今回は許容するreview_requestedも同じFOLLOWUP_MESSAGESテーブルと判定を使うため、同種の取りこぼしが同時に改善される対応 PR
#117