Skip to content

Inbox: a review posted from diffity marks the pull request handled, and handled pull requests stay listed #104

Description

@fiddur

Why

A pull request leaves the inbox at the first poll after its review is posted: GitHub's review-requested:@me search drops it, the daemon retires the row as hidden ("review no longer requested"), and the page lists nothing retired. The inbox never learns that the review happened. It cannot tell a posted review from a withdrawn request, and it cannot show that the author has pushed since.

Handled pull requests should stay listed, with the revision the review was posted against, so the page can show that the pull request has been updated since. They stay in that list until the author asks for a new review (the search lists the pull request again), or the pull request is merged or closed.

The signal

Every review diffity posts to GitHub records a handled mark in the inbox store. diffity writes to the forge in exactly one place — the review server's POST /api/github/create-review (packages/cli/src/server.ts, which calls createReview in packages/github/src/pr.ts) — so that handler is the hook. After createGitHubReview returns with reviewUrl !== null (GitHub accepted the review), record:

field value
pr_id owner/repo#number (the inbox's row id, prId())
head_sha details.headSha, the head the review was posted against
event COMMENT / APPROVE / REQUEST_CHANGES
review_url result.reviewUrl
at now, ISO
  • It runs in whichever diffity posted: a session the inbox opened, or one the reviewer started themselves on their own clone. Anything posted from diffity counts.
  • Store location: inboxStorePath() (honours DIFFITY_DATA_DIR like the rest of the inbox). The server opens the store, writes one row, closes it. If no inbox store exists yet, it is created — a later diffity inbox then already has the history.
  • A failed record must never fail the post: catch, log a warning on the server's stderr, still answer the browser with the result.
  • Only a review the reviewer submits counts. A future auto-comment by the preparation agent (Inbox: Auto-commenting #101) must not mark a pull request handled.
  • New module packages/cli/src/inbox/handled.ts: recordHandledReview({ owner, repo, number, headSha, event, reviewUrl, now, storePath? }), so the server's handler stays one call and the unit test can point it at a temp store.

Store

inbox_handled, an append-only log (a pull request can be handled at several heads):

CREATE TABLE IF NOT EXISTS inbox_handled (
  id INTEGER PRIMARY KEY AUTOINCREMENT,
  pr_id TEXT NOT NULL,
  head_sha TEXT NOT NULL,
  event TEXT NOT NULL,
  review_url TEXT,
  at TEXT NOT NULL
);
CREATE INDEX IF NOT EXISTS inbox_handled_pr_at ON inbox_handled (pr_id, at);
  • recordHandled(mark), latestHandled(prId): Handled | null ({ headSha, event, reviewUrl, at }), handledIds(): string[] (distinct pr_id).
  • Two processes now write the store (the daemon's tick and the posting server). It already runs PRAGMA journal_mode = WAL; add PRAGMA busy_timeout = 5000 so neither write fails the other with SQLITE_BUSY.
  • New status handled in INBOX_STATUSES. It is not retired (isRetired stays done/hidden): a handled row is listed and re-polled every tick, but the tick never prepares it.

Reconcile

ReconcileInput gains handled: Handled | null — the latest mark for the row, store.latestHandled(id). Every caller (runTick, both loops, and prepareBumped) passes it.

In decide(), the not requested branch (the search does not list the pull request), snapshot open, becomes:

  1. MERGEDdone "merged"; CLOSEDdone "closed" (unchanged).
  2. existing.status === 'dismissed' && existing.headSha === snapshot.headSha && handlednull. The reviewer dismissed the handled row; the dismissal covers this head, as it does elsewhere.
  3. handled:
    • existing.status is queued or preparingnull. A bump's preparation is in flight; it owns the row.
    • existing.status is prepared or stale and existing.preparedAt > handled.atnull. The reviewer bumped a handled pull request and got a fresh review they have not posted yet; it stays openable.
    • otherwise → handled, reason from handledReason(handled, snapshot).
  4. otherwise → hidden "review no longer requested" (unchanged).

handledReason: head unchanged → you approved / you requested changes / you commented; head moved → new commits since you approved / … since you requested changes / … since you commented. The page derives its updated flag from the shas, not from this text.

The requested branch is unchanged. A handled row the search lists again is the author's re-request: it falls through to queued with prepare: true, subject to the skipTitles and CI wrappers like any other queued row. Known edges, both harmless:

  • Search-index lag: for a tick or two after posting, the search may still list the pull request. The row stays prepared at its head (null transition) and the view already lists it as handled (below); the status flips at the first poll that no longer lists it.
  • An author push inside that lag window makes the requested prepared row stale and re-prepares it, as any requested stale row is today.

Tick

  • The loop over rows the search no longer returns keeps skipping retired rows; handled rows are not retired, so they are observed and reconciled every tick (with the mark). Worktree removal stays behind if (transition && pr.worktreePath), so a prepared → handled transition reclaims the worktree once and later handled → handled ticks do nothing.
  • Adoption: after that loop, every store.handledIds() id without an inbox_prs row is asked about once (viewPr on the ref parsed from owner/repo#n), observed with requested: false, and reconciled the same way — so a review posted from the reviewer's own clone on a pull request the inbox never saw shows up as handled (or done, never polled again). One gh pr view per never-seen pull request, once.
  • prepareBumped needs no change beyond passing handled: it already derives requested from the pull request being open, so a bump on a handled row goes queued → preparing → prepared; rule 3 above then leaves that fresh review alone until it is posted, at which point the new mark (at > preparedAt) turns the row handled again.
  • BUMPABLE gains handled: ↑ on a handled row prepares a fresh review of the current head, re-request or not.
  • Dismiss on a handled row works as on any other: it stays dismissed while the head is unchanged (rule 2) and comes back as handled with "new commits since…" when the author pushes.

View and page

  • InboxRow.handled: { at, event, reviewUrl, headSha, updated } | null, updated = handled.headSha !== pr.headSha.
  • InboxView.handled: InboxRow[]: rows whose status is handled, plus rows that are prepared/stale with a mark newer than their preparedAt (the reviewer has posted the prepared review; the daemon retires the row at its next poll). Updated rows first, then by at descending. Those rows leave ready.
  • New section Handled on the page, between Queue and Other. A handled row:
    • links to the pull request on GitHub (r.url, new tab) — the worktree is gone, there is nothing local to open;
    • shows size, CI dot, repo#n, title; meta "by author · you approved · 2 h ago · 3f2a1c4" (short handled sha, hover = full sha and the review URL);
    • badge handled normally, updated in the attention colour when updated is true, and then the meta reads "new commits since you approved";
    • actions: ↑ (title "Prepare a fresh review of the current head") and × dismiss.
  • diffity inbox status: a Handled section with handled / updated as the left label, repo#n title and the reason; the empty-inbox check counts handled rows as content; --json carries handled.
  • README / inbox docs: describe the Handled list, what puts a pull request there, what takes it out.

Tests

  • store: record / latest / ids; a second InboxStore on the same file sees a mark the first wrote (the server-side write is visible to the daemon).
  • reconcile: every branch above, including dismissed-and-handled, bumped-and-not-yet-posted, posted-after-bump, re-request.
  • tick: handled rows are re-polled and the reason follows the head; worktree reclaimed once; adoption of an unknown handled id; a bump on a handled row survives the next tick; merge retires it to done.
  • view: section membership including the prepared-with-newer-mark rule, ordering, updated.
  • page: parse check as in tests/inbox-page.test.ts.
  • server routes: create-review records a mark in the store under DIFFITY_DATA_DIR; a store that cannot be written does not fail the post.
  • inbox status output with a handled row.

Not in scope

  • Reading GitHub's reviews to detect a review posted outside diffity.
  • Keeping merged or closed handled pull requests listed; they leave the list as done, as everything merged does today. A time window can be added later if wanted.
  • Opening the old review from a handled row (its worktree is reclaimed; the bundle is kept as before).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions