PR activity notifications in lite - #15645
Conversation
|
This pull request changes Lite's UI, so it is labelled Attach them however you like — drag images straight into a comment, or Swap the label for cc @PavelLaptev — visual change. |
| PRIMARY KEY(`path`, `hunk_header`) | ||
| ); | ||
|
|
||
| -- table review_seen |
There was a problem hiding this comment.
@mtsgrd i wonder if this class of state "seen x" is better off living in local storage - more flexibility, no worries around migrations etc
There was a problem hiding this comment.
Pull request overview
Adds pull request activity tracking and notifications to the Lite app by introducing a per-review “seen” watermark stored in but-db, exposing it via the but-api SDK, and using it to drive unread dots and toast notifications.
Changes:
- Introduces
review_seenstorage + reconcile/mark/list logic in Rust (but-db,but-forge) and exposes it viabut-api. - Extends the generated SDK (graph + linear) with
listReviewSeen/markReviewSeenplus cache tags and parameter metadata. - Implements Lite UI unread indicators + toast detector/classifier with settings support, and adds harness + unit tests for the activity loop.
Reviewed changes
Copilot reviewed 26 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/but-sdk/src/generated/linear/index.js | Exports new review-seen APIs from the linear SDK bundle. |
| packages/but-sdk/src/generated/linear/index.d.ts | Adds typings for listReviewSeen, markReviewSeen, and ReviewSeen. |
| packages/but-sdk/src/generated/linear/cacheTags.js | Adds cache tags for providing/invalidating ReviewSeen. |
| packages/but-sdk/src/generated/linear/cacheTags.d.ts | Typings for new ReviewSeen cache tag wiring. |
| packages/but-sdk/src/generated/linear/apiParamNames.js | Adds param name metadata for new endpoints. |
| packages/but-sdk/src/generated/linear/apiParamNames.d.ts | Typings for param name metadata additions. |
| packages/but-sdk/src/generated/graph/index.js | Exports new review-seen APIs from the graph SDK bundle. |
| packages/but-sdk/src/generated/graph/index.d.ts | Adds typings for listReviewSeen, markReviewSeen, and ReviewSeen. |
| packages/but-sdk/src/generated/graph/cacheTags.js | Adds cache tags for providing/invalidating ReviewSeen. |
| packages/but-sdk/src/generated/graph/cacheTags.d.ts | Typings for new ReviewSeen cache tag wiring. |
| packages/but-sdk/src/generated/graph/apiParamNames.js | Adds param name metadata for new endpoints. |
| packages/but-sdk/src/generated/graph/apiParamNames.d.ts | Typings for param name metadata additions. |
| crates/but-forge/src/seen.rs | Implements list/mark logic and DTO for per-review seen watermarks. |
| crates/but-forge/src/lib.rs | Wires the new seen module into the public forge API surface. |
| crates/but-forge/src/db.rs | Reconciles review_seen table with the review cache during sync and tests the behavior. |
| crates/but-db/tests/db/migration.rs | Updates migration snapshot to include review_seen table + migration id. |
| crates/but-db/src/table/review_seen.rs | Adds the review_seen table schema + CRUD/reconcile helpers. |
| crates/but-db/src/table/mod.rs | Registers review_seen table module. |
| crates/but-db/src/lib.rs | Exposes ReviewSeen and registers its migrations. |
| crates/but-api/src/tags.rs | Adds ReviewSeen cache tag. |
| crates/but-api/src/legacy/forge.rs | Exposes list_review_seen / mark_review_seen over the API. |
| apps/lite/ui/src/settings.ts | Adds prNotifications default setting. |
| apps/lite/ui/src/routes/project/$id/workspace/WorkspaceLists/BranchRow.tsx | Shows unread-dot on PR chip for branches with unread PR activity. |
| apps/lite/ui/src/routes/project/$id/workspace/Sidebar.tsx | Adds a workspace badge counting applied-branch PRs with unread activity. |
| apps/lite/ui/src/routes/project/$id/workspace/Settings/General.tsx | Adds “Pull request activity” (loud/quiet/off) settings control. |
| apps/lite/ui/src/routes/project/$id/workspace/Row.module.css | Adds unread-dot styling. |
| apps/lite/ui/src/routes/project/$id/workspace/Page.tsx | Mounts the toast detector hook for activity notifications. |
| apps/lite/ui/src/routes/project/$id/workspace/Details.tsx | Adds unread dot to PR tab toggle + marks review seen on PR tab view. |
| apps/lite/ui/src/review-seen.ts | New utilities/hook for unread derivation and advancing the watermark. |
| apps/lite/ui/src/review-notifications.ts | New toast detector logic wiring live queries into the classifier. |
| apps/lite/ui/src/review-activity.ts | Pure classifier for deciding loud vs quiet activity. |
| apps/lite/ui/src/review-activity.test.ts | Vitest coverage for classification + observation behavior. |
| apps/lite/ui/src/api/queries.ts | Adds listReviewSeenQueryOptions react-query wrapper. |
| apps/lite/ui/src/api/mutations.ts | Adds useMarkReviewSeen mutation hook. |
| apps/lite/harness/tests/panel.test.tsx | End-to-end harness tests for toasts + unread dot behavior. |
| apps/lite/harness/tests/fixtures.ts | Adds forge fixtures used by harness review-notification tests. |
| apps/lite/harness/browser/Panel.tsx | Enables the same toast detector in the harness panel. |
| apps/lite/electron/src/settings.ts | Extends settings schema to include prNotifications. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 38 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
apps/lite/ui/src/routes/project/$id/workspace/Details.tsx:3179
useReviewUnreadis enabled even whenopenReviewis null, which still subscribes to the seen-marks query for branches with no open PR. Gate the hook withopenReview !== nullto avoid unnecessary observers/work in the common "no PR yet" case.
!!forgeInfo?.capabilities.prService && notificationsLevel !== "off",
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 38 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
apps/lite/ui/src/routes/project/$id/workspace/WorkspaceLists/BranchRow.tsx:174
useReviewUnreadis enabled for every branch row as long as PR support is available, even whenopenPullRequestis null. That still creates alistReviewSeenobserver per row (with review number 0), which is avoidable overhead in large branch lists.
!!forgeInfo?.capabilities.prService && notificationsLevel !== "off",
apps/lite/ui/src/routes/project/$id/workspace/Details.tsx:3036
useReviewUnreadis enabled even when there is no open review yet (reviewis null/undefined), which still registers alistReviewSeensubscription and selector work. Disabling the hook until a review exists avoids unnecessary observers.
forgeInfo?.capabilities.prService === true && notificationsLevel !== "off",
apps/lite/ui/src/routes/project/$id/workspace/Details.tsx:3179
- In
AppliedBranchDetails,useReviewUnreadstays enabled even whenopenReviewis null/undefined, creating alistReviewSeenobserver (review number 0) until the listing resolves. Gate the hook onopenReviewexisting to avoid per-branch overhead when no PR is present.
!!forgeInfo?.capabilities.prService && notificationsLevel !== "off",
|
Os it okay that we see notifications only for applied branches? @krlvi |
Comments left on a diff line were fetched by nothing, so a reviewer who only commented inline showed up as an empty "Reviewed" card, and their `@you` never raised a mention. - `listReviewThreads` reads them through GraphQL's `reviewThreads`. REST reports neither the thread a comment belongs to nor whether it was resolved; on #15645 half of Copilot's threads were already resolved, and REST would have shown every one as open. - The pull request tab nests threads under the review they were posted with, quotes the code each hangs on, and says why a thread is not on the diff. - The branch diff renders open threads on the line they were left on and pins them on the minimap. A thread whose line no longer says what the forge quoted is dropped: an amend the forge has not seen moves the code underneath, and it keeps calling the thread current. - Replies post from either surface through `addPullRequestReviewThreadReply`, keyed on the thread's own id. - Mentions inside diff comments count now, so an `@you` on a line toasts like one left in the conversation. - Right-clicking quoted code copies it or opens the file at that line. A selection anywhere in the app now offers copy, which the window's menu previously gave only inside editable fields.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
apps/lite/ui/src/routes/project/$id/workspace/WorkspaceLists/BranchRow.tsx:555
- The unread indicator is currently only conveyed visually (dot) and via a
titletooltip, which isn’t reliably announced by screen readers. Add an accessible label so assistive tech users can tell when the PR chip has new activity.
<span
className={classes(rowStyles.fadedText, rowStyles.metaItem)}
title={reviewUnread ? "New activity on this pull request" : undefined}
>
apps/lite/ui/src/routes/project/$id/workspace/Details.tsx:2834
- The unread-activity dot on the Pull Request tab is purely visual (
aria-hidden), so screen readers won’t learn that there’s new activity. Consider adding anaria-labelthat includes the unread state whenprUnreadis true.
<Toggle render={<ToggleStyles />} value={"pr" satisfies BranchTab} disabled={prDisabled}>
{prDisabled ? "No pull request" : "Pull Request"}
{!prDisabled && prUnread && <span aria-hidden className={rowStyles.unreadDot} />}
</Toggle>
| /** A write from another window arrives as a storage event. */ | ||
| const onStorage = (): void => { | ||
| cached = null; | ||
| notify(); | ||
| }; |
| const { data: appliedBranches } = useQuery({ | ||
| ...headInfoQueryOptions(projectId), | ||
| enabled: prService, | ||
| select: (headInfo) => | ||
| new Set( | ||
| headInfo.stacks.flatMap((stack) => | ||
| stack.segments.flatMap((segment) => segment.refName?.displayName ?? []), | ||
| ), | ||
| ), | ||
| }); |
| const readMarks = (projectId: string): SeenMarks => { | ||
| const key = storageKey(projectId); | ||
| if (cached?.key === key) return cached.marks; | ||
| const marks = parseMarks(localStorage.getItem(key)); | ||
| cached = { key, marks }; | ||
| return marks; | ||
| }; |
| const writeMarks = (projectId: string, marks: SeenMarks): void => { | ||
| localStorage.setItem(storageKey(projectId), JSON.stringify(marks)); | ||
| cached = { key: storageKey(projectId), marks }; | ||
| notify(); | ||
| }; |
| const prUnread = useReviewUnread( | ||
| projectId, | ||
| { number: review?.number ?? 0, modifiedAt: review?.modifiedAt ?? null }, | ||
| review !== null && forgeInfo?.capabilities.prService === true && notificationsLevel !== "off", | ||
| ); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
apps/lite/ui/src/review-seen.ts:100
onStoragereacts to everystorageevent, even when another window updates unrelated localStorage keys. That needlessly invalidates the cache and wakes all unread subscribers. Filtering to this feature’s key prefix avoids spurious work when other state is written in another window.
/** A write from another window arrives as a storage event. */
const onStorage = (): void => {
cached = null;
notify();
};
| <span aria-hidden>·</span> <RelativeTime timestamp={timestamp} /> | ||
| </> | ||
| )} | ||
| <FreshBadge timestamp={timestamp} /> |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/lite/ui/src/review-seen.ts:90
storageevents fire for any localStorage key (and for unrelated writes from other windows). As written, every storage event invalidates the cache and wakes all subscribers, which can trigger unnecessary snapshot recomputation across many BranchRow instances. Filter the event to only react to the PR activity watermark keys, and consider removing the listener when the last subscriber unsubscribes.
/** A write from another window arrives as a storage event. */
const onStorage = (): void => {
cached = null;
notify();
};
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
apps/lite/ui/src/routes/project/$id/workspace/PullRequestComments.tsx:340
FreshBadgefor timeline events is rendered without anauthor, so it can’t suppress “New” badges for the current user’s own timeline events (unlike comments/submissions whereauthoris passed). If the forge reportsevent.actorfor these events, threading it through would keep “your own actions are silent” consistent.
<span aria-hidden>·</span> <RelativeTime timestamp={timestamp} />
</>
)}
<FreshBadge timestamp={timestamp} />
</div>
| // One toast per review, always: a combined notice could name only one | ||
| // destination, and the reader would have to guess which. | ||
| for (const { change, applied, title, description } of loud) { | ||
| const toastId = toastManager.add({ | ||
| title, | ||
| description, | ||
| actionProps: { | ||
| children: "View", | ||
| onClick: () => { | ||
| // A review outside the workspace may have no local branch | ||
| // to select, so its View opens the forge page instead. | ||
| if (applied) jumpTo(change.review); | ||
| else void window.lite.openInWebBrowser(change.review.htmlUrl); | ||
| toastManager.close(toastId); | ||
| }, | ||
| }, | ||
| // Stays until dismissed: activity worth interrupting for is | ||
| // worth finding when the user looks back at the window. | ||
| timeout: 0, | ||
| }); | ||
| } |
| <NewSinceSummary | ||
| entries={items.map((item) => ({ | ||
| kind: item.kind === "event" ? item.event.kind : item.kind, | ||
| atMs: item.at, | ||
| author: | ||
| item.kind === "comment" | ||
| ? (item.comment.author?.login ?? null) | ||
| : item.kind === "submission" | ||
| ? (item.submission.author?.login ?? null) | ||
| : null, | ||
| }))} | ||
| /> |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
apps/lite/ui/src/review-seen.ts:90
onStoragecurrently invalidates the cache and notifies subscribers for anystorageevent (even unrelated keys), and the globalstoragelistener is never removed when the last subscriber unsubscribes. This can cause unnecessary re-renders and work in multi-window scenarios.
/** A write from another window arrives as a storage event. */
const onStorage = (): void => {
cached = null;
notify();
};
apps/lite/ui/src/routes/project/$id/workspace/Sidebar.tsx:74
- This
flatMapcallback sometimes returns astringand sometimes anArray, relying onflatMaponly flattening arrays. It works, but it’s easy to misread and TypeScript inference can get weird; returning an array consistently is clearer and less error-prone.
select: (headInfo) =>
new Set(
headInfo.stacks.flatMap((stack) =>
stack.segments.flatMap((segment) => segment.refName?.displayName ?? []),
),
| for (const [number, seen] of Object.entries(stored)) { | ||
| if ( | ||
| typeof seen === "string" && | ||
| Number.isInteger(Number(number)) && | ||
| !Number.isNaN(Date.parse(seen)) | ||
| ) | ||
| marks[Number(number)] = seen; | ||
| } |
| typeof entry[1] === "string" && | ||
| !Number.isNaN(Date.parse(entry[1])), | ||
| ); | ||
| if (kept.length > 0) entries[Number(number)] = kept.slice(0, unseenCap); |
| // What the dwell may record as skipped: the conversation's own unread- | ||
| // eligible items. Events register from the surface that renders them. | ||
| const own = (login: string | null | undefined) => | ||
| currentLogin != null && login != null && login === currentLogin; | ||
| const freshItems = [ | ||
| ...(comments ?? []) | ||
| .filter( | ||
| (comment) => comment.id > 0 && comment.createdAt !== null && !own(comment.author?.login), | ||
| ) | ||
| .map((comment) => ({ key: `c:${comment.id}`, atMs: Date.parse(comment.createdAt ?? "") })), | ||
| ...(submissions ?? []) | ||
| .filter((submission) => submission.submittedAt !== null && !own(submission.author?.login)) | ||
| .map((submission) => ({ | ||
| key: `s:${submission.id}`, | ||
| atMs: Date.parse(submission.submittedAt ?? ""), | ||
| })), | ||
| ]; |
| const subscribeMarks = (listener: () => void): (() => void) => { | ||
| // On first use, so merely importing this module listens to nothing. | ||
| if (!watchingStorage) { | ||
| watchingStorage = true; | ||
| window.addEventListener("storage", onStorage); | ||
| } | ||
| // A first subscriber is a fresh surface: re-read whatever storage holds. | ||
| if (listeners.size === 0) { | ||
| cached = null; | ||
| cachedUnseen = null; | ||
| } | ||
| listeners.add(listener); | ||
| return () => listeners.delete(listener); | ||
| }; |
| // What the dwell may record as skipped: the conversation's own unread- | ||
| // eligible items. Events register from the surface that renders them. | ||
| const own = (login: string | null | undefined) => | ||
| currentLogin != null && login != null && login === currentLogin; | ||
| const freshItems = [ | ||
| ...(comments ?? []) | ||
| .filter( | ||
| (comment) => comment.id > 0 && comment.createdAt !== null && !own(comment.author?.login), | ||
| ) | ||
| .map((comment) => ({ key: `c:${comment.id}`, atMs: Date.parse(comment.createdAt ?? "") })), | ||
| ...(submissions ?? []) | ||
| .filter((submission) => submission.submittedAt !== null && !own(submission.author?.login)) | ||
| .map((submission) => ({ | ||
| key: `s:${submission.id}`, | ||
| atMs: Date.parse(submission.submittedAt ?? ""), | ||
| })), | ||
| ]; |
| const { sinceMs, selfLogin, projectId, reviewNumber } = useContext(SeenOnArrivalContext); | ||
| const ref = useRef<HTMLSpanElement | null>(null); | ||
| // Decided at mount and held: store writes must not pull the marker out | ||
| // from under the reader mid-visit. The next visit re-decides. | ||
| const [show] = useState(() => { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
apps/lite/ui/src/review-seen.ts:209
subscribeMarksadds a globalstorageevent listener but never removes it when the last subscriber unsubscribes. This keeps the handler (and cached state) alive for the life of the window even if PR notifications are turned off/unmounted.
if (listeners.size === 0) {
cached = null;
cachedUnseen = null;
}
listeners.add(listener);
return () => listeners.delete(listener);
};
apps/lite/ui/src/routes/project/$id/workspace/Sidebar.tsx:74
flatMaphere sometimes returns a string and sometimes an array (segment.refName?.displayName ?? []). That works by accident but is easy to misread and can confuse TS inference; returning a consistent array is clearer and avoids subtle flattening behavior.
new Set(
headInfo.stacks.flatMap((stack) =>
stack.segments.flatMap((segment) => segment.refName?.displayName ?? []),
),
| const subscribeInbox = (listener: () => void): (() => void) => { | ||
| // On first use, so merely importing this module listens to nothing. | ||
| if (!watchingStorage) { | ||
| watchingStorage = true; | ||
| window.addEventListener("storage", onStorage); | ||
| } | ||
| // A first subscriber is a fresh surface: re-read whatever storage holds. | ||
| if (listeners.size === 0) cached = null; | ||
| listeners.add(listener); | ||
| return () => listeners.delete(listener); | ||
| }; |
Draws the eye to pull requests with new activity, remembers what has been seen, and files what happened into a notification bell. - A bell in the window's top-right corner carries a red dot while unread notifications wait; its panel lists them richly, newest first — a comment with author and snippet reads differently from "pushed 3 commits" or an approval. Clicking an entry marks it read and jumps to the PR tab, or the forge page when the branch is not local. Opening the panel alone marks nothing read. - What files where turns on one question: is a human waiting on you? Someone else's comment, a verdict, a review request naming you, and `@you` mentions — loud on any open PR, applied or not. Commits stay quieter facts the bell still carries; a request naming someone else, a dismissed verdict, and your own actions are silence. A kind's items on one review coalesce into one entry per poll. - Seen state is a per-review watermark in local storage, deliberately disposable and per-machine: reviews are stamped seen the first time they are listed, so a wiped store reads as "everything is read" rather than a wall of stale dots, and only activity after a review first appeared can be unread. Marks never move backwards; viewing a PR tab advances one after a short dwell, re-armed on window focus so an unfocused view does not count. Quiet surfaces stay: an unread dot on the branch row's PR chip and the Details PR tab, and a count on the Workspace sidebar tab. - Each new entry in the conversation wears a small "New" marker, and the markers are per-item: an entry counts as looked at only once it has sat in the focused viewport for a beat. Items the dwell advances past unseen are recorded as skips — the complement is stored on purpose, since a list of read items would grow with everything ever read once one stubborn item pinned the mark — so their markers and the dot survive until each is actually seen. Skips are capped and pruned; dropping one merely reads as seen. Your own replies never mark. - Detection rides the existing 60s review-listing poll: a `modifiedAt` bump past the watermark triggers one targeted comments, submissions and timeline fetch for that PR alone, and the baseline seeds from the stored watermarks so activity that landed while the app was closed still announces itself once. - One dial in Settings → General: `Pull request activity: loud | quiet | off`. Loud shows the bell; quiet keeps only the dots; off hides it all. - Testable without a forge: the classifier and the stores are pure modules driven by fabricated events in vitest, and the harness panel mounts the real detector, so the whole loop — poll, classify, file, dot — runs against fake forge handlers.






Draws the eye to pull requests with new activity, remembers what has been seen, and files what happened into a notification bell.
The bell sits in the window's top-right corner with a red dot while unread notifications wait. Its panel lists entries richly, newest first — a comment with author and snippet reads differently from "pushed 3 commits" or an approval. Clicking an entry marks it read and jumps to the PR tab (or the forge page when the branch isn't local); opening the panel alone marks nothing read.
One axis decides what files: is a human waiting on you? Someone else's comment, a verdict, a review request naming you, and
@youmentions — loud on any open PR, applied or not. Commits are quieter facts the bell still carries; requests naming someone else, dismissed verdicts and your own actions are silence. A kind's items on one review coalesce into one entry per poll.Seen state is per-review watermarks in local storage — deliberately disposable and per-machine:
Newmarker, and the markers are per-item: an entry counts as looked at only once it has sat in the focused viewport for a beat. Items the dwell advances past unseen are recorded as skips — the complement is stored on purpose, since a list of read items would grow with everything ever read once one stubborn item pinned the mark — so their markers and the dot survive until each is actually seen. Skips are capped; dropping one merely reads as seen.review_seentable in but-db; krlvi's review pointed out that disposable per-machine state doesn't need a migration or a round trip, so the table, its endpoints and its cache tag are gone.Detection rides the existing 60s review-listing poll: a
modifiedAtbump past the watermark triggers one targeted comments/submissions/timeline fetch for that PR only, and the detector baselines from the stored watermarks, so activity that landed while the app was closed still announces itself once. One dial in Settings → General:Pull request activity: loud | quiet | off(loud shows the bell, quiet keeps only the dots, off hides it all).Testable without a forge: the classifier and the stores are pure modules driven by fabricated events in vitest, and the harness panel mounts the real detector so
panel.test.tsxruns the whole loop — poll, classify, file, dot — against fake forge handlers. Also verified live against real GitHub data.Landing the click on the exact comment rides the stacked diff-comments PR, where inline comments become addressable. OS notifications and a dock badge for the unfocused window are deliberately left for a later follow-up.