feat(pr-review): server inbox, honest recents, and stale-submit recovery (w5a) - #5362
feat(pr-review): server inbox, honest recents, and stale-submit recovery (w5a)#5362iscekic wants to merge 13 commits into
Conversation
Add , a server-fixed GraphQL search over review-requested open PRs, so the mobile inbox reads only PRs the caller's token can see. drops non-PR and identity-less nodes instead of throwing, and the schema guard now validates the inbox query.
Start the pulls, repos, and GraphQL legs of getPullRequest, and the two paginate legs of listChecks, before awaiting. Await with Promise.allSettled and rethrow the first rejection in leg order, so a second failure never becomes an unhandled rejection. Authorization stays inside the token retry.
Route context expansion through the react-query cache so re-expanding a gap is a hit, and hold the parsed viewed-files map in a process cache that a generation guard keeps from being overwritten by a late first read. Sign-out still drops the private content.
Replace the entry screen's ScrollView with a single FlashList whose header and footer stay mounted in every inbox state, so direct URL open and Recents survive an inbox failure. Recents now write only after the first authorized payload, and failed entries expose Retry and Remove.
Partition queued comments by head SHA and send only the fresh ones, so a stale item never blocks the valid ones. Stale items stay queued with an Outdated badge and edit/delete, and removeComments carries the hydration guard so an in-flight loadDraft cannot re-add just-posted comments.
Replace the ScrollView with a FlashList and page on scroll, so a large PR does not mount every file row. A search still fetches the full listed set, and a failed next page shows a retry CTA.
Replace the members ScrollView with a FlashList and drive it from a pure item builder, with loading/error/empty evaluated in that order so an org error never renders No members yet. Extract the discussion tab's state selection into a pure, tested selector.
Use the repo's deferred-promise pattern (undefined as unknown as) and mockReturnValueOnce so the two late-read tests pass oxlint.
The mapper receives only fresh items; stale items are never sent.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous Review Summaries (2 snapshots, latest commit c48dcae)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c48dcae)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit dddb570)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (47 files)
Reviewed by grok-4.6 · Input: 100.5K · Output: 12.2K · Cached: 337.7K Review guidance: REVIEW.md from base branch |
Remove the identity error-kind mapping in selectPrInboxView and inline the one-line shouldLoadAllFiles predicate at its single call site.
iscekic
changed the title
iscekic
changed the title
pandemicsyn
left a comment
pandemicsyn
left a comment
There was a problem hiding this comment.
lgtm, couple bot notes inline and some merge conflicts but preapproved
| // items persist even when the invalidated load never resolves. The | ||
| // persistence effect writes the remainder (an empty remainder clears the | ||
| // stored entry through its empty-state branch), so no direct clearDraft. | ||
| hydrationGenerationRef.current += 1; |
There was a problem hiding this comment.
bot: A successful Approve before draft hydration finishes can erase an existing persisted pending-comment queue.
Before hydration, APPROVE submits with the empty in-memory queue, then calls removeComments([]). That still invalidates the pending load and marks the provider hydrated, so the empty-state persistence effect clears the stored draft and the delayed load is discarded.
Suggested fix: Gate submission until hydration settles, or make an empty removeComments([]) a true no-op. The latter must skip both the hydration-generation increment and setHydrated(true); skipping only the generation bump still enables the empty-state clearDraft. Add a mounted regression test with deferred loadDraft, persisted items, and an immediate APPROVE; resolving the load must retain the queue and must not clear its stored draft.
| !fetchAll.isRunning && | ||
| !fetchAll.error | ||
| ) { | ||
| void runRef.current(); |
There was a problem hiding this comment.
bot: An active search can repeatedly request a failed next page without showing a retry.
fetchNextPage() can resolve to an error-state result while retaining prior pages and hasNextPage: true. useFetchToCompletion.run() only stops for missing data or no next page, so it requests that cursor again. Because this does not enter its catch, fetchAll.error remains unset; the normal later-page retry is hidden while search is active.
Suggested fix: Inspect the resolved observer result for an error state and route it through the existing error path (for example, throw result.error before evaluating hasNextPage) so the loop stops and the load-all retry renders. Add coverage using a resolved error result with prior pages and hasNextPage: true: it should make one request, stop, and show the load-all retry.
|
|
||
| export async function clearViewedFiles(): Promise<void> { | ||
| await deleteAccountMetadata(PR_REVIEW_VIEWED_KEY); | ||
| cachedMap = null; |
There was a problem hiding this comment.
bot: A populated viewed-file cache remains readable while sign-out deletion is in flight.
clearViewedFiles awaits deleteAccountMetadata before invalidating cachedMap, so a concurrent getViewedFiles can return the prior account's paths. The existing generation guard only protects reads already in flight when invalidation occurs.
Suggested fix: Add a clearing-in-progress fence: invalidate the cache/generation before deletion, and ensure reads during the awaited deletion return or await an empty authoritative state without publishing a fresh SecureStore read of the not-yet-deleted value. Add a test with a populated cache and delayed deletion; a concurrent read during the delay must never return the prior paths.
2 participants
Summary
Reviewer Notes
The PR review entry screen now shows a server-backed inbox of pull requests requesting the viewer's review, paginated through a new
listInboxquery. The search string is fixed server-side, so a caller can only ever see pull requests their own GitHub token can already see. Direct URL open is unchanged, and pagination is capped at 20 pages to stay inside GraphQL search rate limits.Files
apps/web/src/lib/github-pr-review/mappers.ts— addsbuildInboxResultand theGraphQlInboxNodetype; drops non-PR search nodes.apps/web/src/lib/github-pr-review/dtos.ts— adds the inbox item and result schemas plusINBOX_PAGE_SIZE.apps/mobile/src/lib/pr-review/use-pr-inbox.ts— infinite-query hook; 20-page cap and 30-second stale time.apps/mobile/src/components/pr-review/pr-review-inbox-list.tsx— FlashList body composing the paste header, inbox rows, and recents footer.apps/mobile/src/components/pr-review/pr-review-inbox-view.ts— pure state selector for the seven inbox states.apps/mobile/src/components/pr-review/pr-review-entry-screen.tsx— mounts the inbox list; renders the recents Retry and Remove buttons.apps/mobile/src/components/ui/icons.ts— adds the Inbox icon.The overview and check queries now start their sub-requests in parallel and settle them with
Promise.allSettled, rethrowing the first rejection in leg order. This removes the sequential round-trips that previously serialized the overview's REST and GraphQL legs and the checks' two paginations.Files
apps/web/src/routers/github-pr-review-router.ts— addslistInbox,INBOX_QUERY, andINBOX_SEARCH_QUERY; parallelizesgetPullRequestandlistChecks.The submit sheet now partitions queued comments into fresh and stale by comparing each comment's recorded commit SHA against the current head. Only fresh comments are sent in the batched submit; stale comments stay queued, are flagged outdated, and the sheet reports a partial result instead of dismissing. This replaces the previous behavior of submitting everything against the latest head and surfacing per-item 422s.
Files
apps/mobile/src/lib/pr-review/partition-pending-items.ts— splits the queue into fresh and stale; an empty head makes every item fresh.apps/mobile/src/components/pr-review/pr-review-submit.tsx— submits fresh items only, removes them viaremoveComments, and shows the partial result.apps/mobile/src/components/pr-review/pr-review-submit-view.ts— CTA label and partial-result copy selectors.apps/mobile/src/components/pr-review/pr-review-pending-comment-row.tsx— stale flag, Outdated badge, and updated hint copy.apps/mobile/src/lib/pr-review/build-submit-review-input.ts— documents the fresh-only submission contract.apps/mobile/src/lib/pr-review/pending-review-provider.tsx— addsremoveCommentswith the hydration-generation guard.Recent PRs now carry a last-load outcome and are written only after the first authorized payload loads; a failed open marks an existing entry failed rather than creating one. The entry screen shows a Retry button for failed entries and a Remove button for every entry.
Files
apps/mobile/src/lib/pr-review/recent-prs.ts— addslastResult,removeRecentPr, andmarkRecentPrFailed.apps/mobile/src/components/pr-review/pr-review-screen.tsx— backfills withlastResult: 'ok'; marks an existing entry failed on error.apps/mobile/src/components/pr-review/recent-pr-row-state.ts— row-state selector for title, identity, and failed marker.The file navigator now renders file rows in a virtualized list and loads pages on scroll instead of fetching the full file set on mount. An active search still drives fetch-to-completion so filtering covers the full listed set. The memoized rows read the latest select and toggle handlers through refs, so the rows stay identity-stable across a search re-render yet never go stale when a head-SHA change swaps the toggle handler.
Files
apps/mobile/src/components/pr-review/diff/pr-diff-file-navigator.tsx— FlashList with onEndReached and memoized rows; per-path callbacks read the latest handlers through refs.apps/mobile/src/lib/pr-review/diff/navigator-file-filter.ts— extracted path filter and load-all decision.The organization Members screen now renders a single virtualized list built from a pure item builder, with loading, error, and empty states selected by a dedicated error-view selector. Invited members now render alongside active members in the same list.
Files
apps/mobile/src/components/organization/members-screen.tsx— FlashList with section, member, and invite items.apps/mobile/src/components/organization/members-list-items.ts— item builder with exact composition rules.apps/mobile/src/components/organization/org-list-error-view.ts— error-view selector; no reconnect state.Diff context expansion now fetches through the React Query cache with a 5-minute stale time and 10-minute GC time instead of a raw tRPC client call, so repeated context loads reuse cached lines. Viewed-file state adds a process-lifetime cache with a generation guard; when a read observes a generation change, it returns the authoritative current map rather than the stale parse, so a toggle or clear never races a re-read.
Files
apps/mobile/src/lib/pr-review/diff/use-pr-diff-context-loader.ts—queryClient.fetchQuerywith stale and GC times.apps/mobile/src/lib/pr-review/diff/context-window.ts— extracted window arithmetic.apps/mobile/src/lib/pr-review/viewed-files.ts— cached map plus a generation guard; the guard now protects the caller's return value, not only the cache.The Discussion tab's state selection is extracted into a pure
selectDiscussionTabViewfunction so the seven outcomes can be unit-tested. The tab renders exactly what it rendered before; only the decision is moved.Files
apps/mobile/src/components/pr-review/pr-review-discussion-tab-view.ts— pure state selector for the seven tab states.apps/mobile/src/components/pr-review/pr-review-discussion-tab.tsx— uses the selector instead of inline branches.Satisfied on main, not reassigned and not reimplemented:
P1-F-45,P2-GH-52d,P1-F-46c,P2-E-30a,P2-E-33b.Two assumptions were corrected during the build:
submitReviewonexpectedHeadSha, and it does not returnstale_headin the review path;stale_headexists only in the merge reconcile path. Stale handling is solved entirely on the client.Tests: 20 test files added or updated.
Generated: none.
Verification
Visual Changes
PR Review entry screen with the server-backed inbox
Submit sheet showing an outdated comment kept in the queue
Human steps: none known.
Notes: none.