feat(gui-app): wire the PR diff tile into bundle-find - #1200
Merged
Merged
Conversation
The virtualized PR diff tile registered no tile-find adapter, so the in-app find bar reported search as unavailable and native find-in-page only saw the rows Virtuoso had mounted (#1195). Give it the Git bundle tile's find session: - usePrBundleDiffFind mirrors useGitBundleDiffFind - per-file metadata units for every file up front (searchable without a mount), coverage states (binary / truncated / collapsed / large / unloaded), a content identity keyed on the comparison OIDs + whitespace mode + file list, and reveal navigation that scrolls Virtuoso to the target row (mounting it, which in split mode issues its fetch) and expands a collapsed one. - Sections stamp data-bundle-diff-file-id and notify on expand; the split fetch body registers failed / binary coverage; PrPatchContent - the one place a patch reaches the screen in either mode - registers the loaded patch under an OID-addressed cache key, so both the split view and the old-host monolith fallback agree on what is searchable and a patch stays searchable after its row virtualizes away. - A large file's content stays out of the index until its "Load diff" is pressed - reveal does not press it - matching the bundle tile's guard. - isPrLocalDiffLargeFile is hoisted to lib/pr so the renderer and the find session cannot disagree about which files are guarded. Closes #1195 Signed-off-by: Hardik Shingala <hardik@traycer.ai>
The find session retains a loaded patch after its row unmounts (collapse, Virtuoso eviction) so it stays searchable - but the row's "Load diff" and "Load Full" approvals were row-local useState, so a reveal that remounted the row landed on the large-file placeholder (no DOM to paint the match in) or on the bounded re-fetch of a fully-loaded truncated file, whose cached truncated answer then registered over the retained full patch and made the tail's matches vanish (cold review, should-fix). Move both approvals into PrLocalDiffFilesView - the same lifetime as the find session's retention - keyed by sectionStateKey, so they still expire with the comparison they were granted for and a summary refresh that resolves new OIDs cannot carry byteBudget: null onto a new comparison. Signed-off-by: Hardik Shingala <hardik@traycer.ai>
Pure: isPrLocalDiffLargeFile (null counts are large), prBundleDiffFindFileId, prBundleLoadedPatchCacheKey (differs on every byte-changing input, JSON-safe against path/field collisions). Integrated (<PrDiffTile> under TileFindScope, real tile-find + canvas stores, mocked Virtuoso with a renderRows toggle): every file's metadata is indexed without a mount; a loaded split patch stays searchable after its row unmounts (mutation-probed against PrPatchContent's registration); the coverage message names unloaded / collapsed / large / binary files; a reveal into a collapsed file scrolls Virtuoso to its index and expands it; a rejected per-file fetch registers failed coverage; the monolith fallback searches a mounted patch and counts a null patch as truncated; and a large file's "Load diff" approval survives collapse + re-expand. Test-authoring notes baked into the file: never wrap search() in waitFor when the host client is Promise-mocked (the store publish re-wakes waitFor's MutationObserver before the pending fetch's microtask runs - it spins at 100% CPU), await a concrete DOM signal and search once; and close the find session before toggling collapse in a test, since reveal keeps the active match's file expanded on every renderer identity change. Signed-off-by: Hardik Shingala <hardik@traycer.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Summary by CodeRabbit
WalkthroughThe PR adds bundle-find support for virtualized PR diffs. It introduces stable file and patch identities, shared load approvals, coverage registration, virtualized navigation, and stale patch cleanup for split and monolith modes. ChangesPR diff find integration
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR adds find support for virtualized PR diff tiles and includes passing coverage for the key search, loading, expansion, and fallback behaviors. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Find
participant PrBundleDiffFind
participant PrDiffBody
participant Virtuoso
participant PatchQuery
Find->>PrBundleDiffFind: search registered file metadata
PrBundleDiffFind->>Virtuoso: scroll to matching virtualized row
PrBundleDiffFind->>PrDiffBody: expand collapsed file when required
PatchQuery->>PrDiffBody: return patch state
PrDiffBody->>PrBundleDiffFind: register or unregister patch coverage
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42e11bc995
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The bundle-find session retains a loaded patch past its section's unmount on purpose, and a retained patch outranks any coverage state in the coverage counts. So when a LATER request for the same content fails - most directly "Load Full" after a truncated patch was shown, which is a new query key with no data of its own - the section rendered GitErrorBlock while find kept matching text that was no longer in the DOM, reported the file as truncated instead of failed, and left navigation pending (Codex review on #1200). Add unregisterLoadedPatch to the shared registration context (idempotent, same identity discipline as registerLoadedPatch) and call it on the failure transition from both the PR tile's split fetch body and the Git bundle's BundleInlineDiff, which had the identical latent class. Pinned by a mutation-probed integrated test: truncated patch searchable, Load Full fails, the token no longer matches and coverage says failed, not truncated. Signed-off-by: Hardik Shingala <hardik@traycer.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88185c0914
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…is pending Past "Load Full" a mounted section will never render the truncated bytes again - the approval only moves forward - so they are dead for find from the moment the new query key is pending, not only once it fails: leaving them indexed had find matching text that was not in the DOM, reporting the file as truncated, and parking navigation on a skeleton for the length of the request (Codex review on #1200). Unregister on the pending transition in the PR tile's split fetch body and in the Git bundle's BundleInlineDiff (same class), keeping the invariant: registered ⇔ renderable by this section, or retained after its unmount. Pinned by a deferred-fetch test: 1 match (truncated shown) → 0 (skeleton) → 2 (full patch). Signed-off-by: Hardik Shingala <hardik@traycer.ai>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1195.
What
The virtualized PR diff tile now registers with the app's bundle-find machinery, mirroring the Git bundle tile (
useGitBundleDiffFind). Before this the tile registered no tile-find adapter at all — before or after the summary/per-file split (#1183) — so the in-app find bar reported "Search is not available for this tile yet." on PR tiles, and after the split native find-in-page only saw the rows Virtuoso had mounted.How
usePrBundleDiffFind(components/epic-canvas/pr/pr-bundle-diff-find.ts) — per-file metadata units for every file up front (basename, dir, path, previousPath, status, ±counts, "binary" — searchable without a mount), input-level coverage states (binary→ monolithpatch: null=truncated→collapsed→large→unloaded), a content identity keyed on the comparison OIDs + whitespace mode + patch mode + file list, and reveal navigation viauseBundleDiffFindNavigation(scrolls Virtuoso to the row — mounting it, which in split mode issues its fetch — and expands a collapsed one viaupdatePrDiffTileViewInTab).PrLocalDiffFilesViewowns it; the sharedPrLocalDiffFileSectionstampsdata-bundle-diff-file-idand notifies on expand; the split fetch body registersfailed(query error /unavailable) andbinary(response.isBinary);PrPatchContent— the one place a patch reaches the screen in either mode — registers the loaded patch under an OID-addressed cache key, before its hunk check (a header-only change is loaded, not "unloaded"). So what "searchable" means is identical for a new host and the old-host monolith fallback by construction, and a patch stays searchable after its row virtualizes away.largerows (the placeholder exists to keep an unbounded patch off the main thread until asked for). The coverage message says "N large files were not fully searched".PrSectionLoadApprovalscontext, keyed bysectionStateKey) rather than as rowuseState— the find session retains a loaded patch after its row unmounts, so a reveal that remounts the row must render the same bytes, not the placeholder or a bounded re-fetch. Keying bysectionStateKeykeeps the existing guarantee that an approval expires with the comparison it was granted for.isPrLocalDiffLargeFilehoisted tolib/pr/so the renderer and the find session cannot disagree about which files are guarded.Not in scope, called out for honesty: no loading/error
sourceOverride(the tile's skeleton and unavailable body leave no adapter registered — the pre-range states are brief and honestly "unavailable"); the Git bundle tile's own row-localfullDiffIdentityhas the same remount-after-retention characteristic for its truncated rows — same class, separate change if wanted.Review
Cold-reviewed (2 rounds, converged): round 1 raised one should-fix — the load-approval lifetime mismatch above — fixed in
aa13f99a; round 2 verified the repair on both failure scenarios, approval expiry, context-churn/effect hygiene, and the throw-on-missing-provider choice, with no new defects.Tests
52/52across the four PR suites; 19 new:pr-bundle-diff-find.test.ts): large-file predicate (null counts are large), file id, cache key differs on every byte-changing input and is JSON-safe against path/field collisions.pr-diff-tile-find.test.tsx,<PrDiffTile>underTileFindScopewith the real tile-find + canvas stores, mocked Virtuoso with arenderRowstoggle): metadata indexed without a mount; loaded split patch survives row unmount (mutation-probed againstPrPatchContent's registration); coverage message names unloaded/collapsed/large/binary; reveal into a collapsed file scrolls to its index and expands it; rejected per-file fetch ⇒failed; monolith fallback searches a mounted patch and countspatch: nullastruncated; a large file's "Load diff" approval survives collapse + re-expand.Two test-authoring notes are documented in the suite for the next person: don't wrap
search()inwaitForwhen the host client is Promise-mocked (the store publish re-wakeswaitFor's MutationObserver before the pending fetch's microtask ever runs — it spins at 100% CPU); and close the find session before toggling collapse, since reveal keeps the active match's file expanded on every renderer identity change (shared bundle-find behavior).