feat(code-review): mark files as viewed in diff review - #2762
Conversation
|
React Doctor found 11 issues in 2 files · 11 warnings. 11 warnings
Reviewed by React Doctor for commit |
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/ui/src/features/code-review/reviewShellParts.tsx:206-241
**Nested `<button>` inside `<button>` — invalid HTML**
`FileHeaderRow` renders a `<button>` (line 206) and `ViewedCheckbox` renders another `<button>` inside it (line 251). Nesting interactive elements inside a `<button>` is prohibited by the HTML spec. While React's synthetic event system makes `e.stopPropagation()` work in practice, the resulting DOM is invalid and screen readers / keyboard-navigation tools may behave unpredictably (e.g. some AT software flattens or skips the inner button). A straightforward fix is to change `FileHeaderRow`'s root element to a `<div>` with `role="button"` and `tabIndex={0}`, or to restructure the layout so the viewed checkbox sits adjacent to rather than inside the collapse button.
### Issue 2 of 2
packages/ui/src/features/code-review/reviewViewedStore.ts:28-33
**`clearTask` is defined but never called — store grows unbounded**
The persisted store accumulates a `taskId → fileKey → true` entry for every task the user ever reviews and never prunes them. Over time this silently inflates the `localStorage` entry for `"review-viewed-storage"`. Consider calling `clearTask` when a task is deleted or archived, or add a storage migration / TTL that removes entries older than a configurable window.
Reviews (1): Last reviewed commit: "feat(code-review): mark files as viewed ..." | Re-trigger Greptile |
|
Reviews (2): Last reviewed commit: "feat(code-review): auto-collapse already..." | Re-trigger Greptile |
jonathanlab
left a comment
There was a problem hiding this comment.
Thank you! Some feedback here:
-
The use of signature caching, content hashing, and ref usage here feels excessive here. Are we sure there's not a cleaner way to do this? Can't we just calculate the fingerprint once on file load/change instead of recalculating it on every render/draw?
-
Let's go with "Viewed" instead of "Read", I think this is convention across most reviewing platforms and what people are used to.
-
I think the button is also quite hard to read, could we either make it clearer that this is a button, similar to GitHub's UI, or at least make the font size bigger
8eb34a1 to
d7cafe2
Compare
f03da04 to
4af0d6a
Compare
Add a togglable "Viewed" checkbox, right-aligned in each file header in the review/diff viewer. Marking a file viewed also collapses it (mirrors GitHub); unmarking re-expands it. State is local + persisted, keyed by taskId -> file key, so it works across all diff sources (local, branch, PR, cloud) and survives restarts. Note: pre-commit hook bypassed; monorepo typecheck fails on a pre-existing error in canvas/WebsiteLayout.tsx (Button "loading" prop) from main, unrelated to this change. Generated-By: PostHog Code Task-Id: ea2a1d14-f772-40f5-bd7d-3799f77e31b4
- FileHeaderRow: wrap only the toggle target in a button; open-file and viewed controls are now siblings, not nested interactive elements - reviewViewedStore: bound the persisted store via LRU eviction (MAX_TASKS) and drop tasks with no viewed files; remove unused clearTask Generated-By: PostHog Code Task-Id: ea2a1d14-f772-40f5-bd7d-3799f77e31b4
Store a content signature when a file is marked read and compare it against the current diff to surface a "Changed" state. Rename the user-facing control to "read". Bound persisted state with archival pruning plus an LRU backstop, memoize signatures by file identity, and skip pruning the task whose review is open. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b
… merges Archived tasks won't be re-reviewed, so drop their persisted read state as part of the archive orchestration (covers single and bulk). Likewise clear it once the reviewed task's PR is merged. Adds a clearTask action to the review-viewed store. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b
Display "<read>/<total> read" next to the file count in the review toolbar, counting only files marked read at their current signature. Lower the persisted-size backstop from 4000 to 500 entries. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b
Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b
…e local read signature - ReviewShell passed a bare Task to useTaskPrStatus, so cloudPrUrl/ taskRunEnvironment were undefined and the PR-merge read-state clear never fired for cloud tasks. Resolve cloudPrUrl via useCloudPrUrl and pass the run environment, matching the other useTaskPrStatus call sites. - Local read signatures hashed parsed hunks, which change when the hide-whitespace toggle re-fetches a different diff, falsely flipping read files to "Changed". Base the signature on the git blob object ids from the patch index line instead: content-identifying and unaffected by the toggle (falls back to hunk geometry when absent). Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b
…Tasks action clearTask(id) and pruneArchived([id]) did the same single-key delete, and "pruneArchived" read wrong for the merge path. Collapse both into a single clearTasks(ids) action used by archive, merge, and the archived-task backstop. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b
…isFileRead, cap 250 - Add unit tests for reviewViewedStore (mark/unmark, clearTasks, entry-cap eviction + active-task retention) and the signature helpers (patch hash, blob-id preference / whitespace stability, fallbacks). - Keep currentSignatures' reference stable across collapse toggles so toggling one file no longer re-renders every ViewedCheckbox via context. - Extract isFileRead() so the toolbar count and the checkbox share one predicate. - Raise the persisted-entry backstop from 150 to 250. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b
Collapse state is ephemeral while read state persists, so previously-read files came back expanded on each open. On the first open per task (once signatures load), collapse files whose stored signature still matches — mirroring GitHub. Changed-since-read files stay expanded, and the one-shot guard means manually re-expanding a read file afterwards sticks. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b
- Fingerprints are now computed once per file load/refetch via useMemo on the files arrays in each page, instead of per item rebuild. Drops the WeakMap signature cache and the prev-map ref stabilization in ReviewShell; signatures flow down as a plain prop. - Rename user-facing "Read" to "Viewed" (button, tooltips, toolbar count) to match reviewing-platform convention; internal names follow. - Make the viewed toggle look like a button: bordered, rounded, larger text. Generated-By: PostHog Code Task-Id: 4ccb2163-f475-4055-b186-3216daed734f
Generated-By: PostHog Code Task-Id: 4ccb2163-f475-4055-b186-3216daed734f
…icts - Remove LRU eviction from setViewed: clearTasks on archive/merge is the real cleanup path; simple object mutation is enough - Remove the two LRU-specific tests - Resolve rebase conflicts (clearTerminalStates from main + clearViewedState from this branch both land in archiveOrchestration) Generated-By: PostHog Code Task-Id: 4ccb2163-f475-4055-b186-3216daed734f
4af0d6a to
33c1984
Compare
Generated-By: PostHog Code Task-Id: 4ccb2163-f475-4055-b186-3216daed734f
|
@jonathanlab made a dumber version of caching without refs to tweak it |
…-file-as-read-in-pr-review Resolve conflicts in code-review diff header: keep the PR's viewedKey alongside main's generic headerTrailing/trailing slot (used by pr-review). Generated-By: PostHog Code Task-Id: 809f0c08-81d8-4a96-a0ee-9567cc2bab91
…-file-as-read-in-pr-review Generated-By: PostHog Code Task-Id: a6c472e2-e9b9-45d3-8fd4-089b3a9dd85d
There was a problem hiding this comment.
There is an active CHANGES_REQUESTED review from @jonathanlab with substantive concerns about implementation complexity (signature caching and content hashing approach) and UX (terminology "Read" vs "Viewed", button legibility). These remain unaddressed and block auto-approval.
There was a problem hiding this comment.
Gates denied due to an active CHANGES_REQUESTED review from @jonathanlab with substantive unaddressed concerns about implementation complexity and UX terminology.
Generated-By: PostHog Code Task-Id: 99f136fb-8ba3-4b77-83c6-2a29793b9205
adboio
left a comment
There was a problem hiding this comment.
some small feedback:
- in the full page diff panel, the file tree should show some indication that the file is viewed
- (opinion) clicking a file in the file tree that is marked as viewed should still scroll to it, but not unfurl it
otherwise looks good, as far as i can tell @jonathanlab 's feedback was well-addressed, good stuff 🫡
Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8
Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8
Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8
Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8
Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8
Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8
Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8
Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8
|
/trunk merge |
|
😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details. |


Problem
When reviewing a diff there's no way to track which files you've already looked at, no signal when a file changes after you've reviewed it, and nothing to get reviewed files out of the way.
Changes
CleanShot.2026-06-18.at.15.53.53.mp4
read / total) next to the file countHow did you test this?
pnpm --filter @posthog/ui typecheckandbiome lintpass on the changed files (the one remaining typecheck error is a pre-existing, unrelated issue incanvas/WebsiteLayout.tsx)Automatic notifications
Created with PostHog Code