[OPIK-8050] [FE] fix: paginate prompt version history and fix version labeling - #8140
Conversation
… labeling Version history on the Prompt tab only ever loaded the first 25 versions (single page, client-computed vN labels), so older versions were unreachable and a deep link to one silently fell back to the latest version while still showing a stale label. Switches to a paginated usePromptVersionsByIdInfinite hook, wires the lazy-load already built into VersionHistoryTimeline, and replaces index-derived labels with the backend's persistent version_number (falling back to commit when a pre-migration row has none). Also fixes OPIK-8189: the Compare sheet recomputed labels from whatever was locally loaded instead of the true total, mislabeling versions past the first page. Guards a stale/crafted activeVersionId from rendering a different prompt's content, aligns the new hook's cache key with existing mutation invalidations so the sidebar refreshes after writes, and extends the Diff dropdown to auto-paginate so "Compare against" isn't capped at the first loaded page either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
⏱️ pre-commit per-hook timing
⏭️ 35 skipped (no matching files changed)
|
|
This change looks worth a test. Two things here and both land on the same untested screen. The version-history sidebar is now an infinite query (25/page, sentinel auto-load, and the Diff and "Deploy to environment" menus force-load the remaining pages when opened), and every version label switches from the positional Would target What it would check
Deploying a test environment for this PR and exploring it — results will follow in a comment. areas: Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 03 Sep 22:02 UTC. |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
Explored this PR on its own test environment. Worked all 5 ranked items in Per-item results
3 — Deploy menu never force-loads its remaining pages (suspicious, high confidence)Reproduced 3×, cleanly, on one page load with the network instrumented:
Expected: opening the Deploy menu pages until every version is loaded, so the owner label is right on first open. Observed: it only becomes right if something else happened to load page 2 first. The label logic itself is correct — This is fixed on the PR branch. Commit Action for the reader: redeploy 3 flows look worth a permanent test:
Writing the spec now; a draft PR will follow. Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-explored after a push on 03 Sep 23:01 UTC. |
…ct orchestration hook - Gate the deep-link, diff-menu, and scroll-sentinel pagination triggers on isFetching/isError (not just isFetchingNextPage) to stop a permanently failing page fetch from hammering the backend in an infinite retry loop. - Reject version_type=mask when resolving the active version by id, both for rendering and for the playground-load fetch, since the by-id lookup isn't scoped the way the paginated list is. - Fix the mobile version dropdown to highlight the resolved active version instead of the raw query param. - Share one fetcher between usePromptVersionsById and usePromptVersionsByIdInfinite instead of duplicating the request logic. - Reuse an already-loaded version from the paginated list instead of issuing a redundant by-id fetch when switching between loaded versions. - Extract version-history orchestration (pagination, selection, active version resolution) out of PromptTab into usePromptVersionHistory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…edupe pagination triggers
- DeployToEnvironmentMenu now auto-paginates while open (mirroring the Diff
menu) so "Currently vN" resolves for owners on unloaded pages instead of
silently omitting them.
- Replace remaining positional v{total-idx} labels with the backend's
version_number/commit in usePromptVersionLabel, usePromptVersionsWithLabels,
and PromptLibraryMenu, so labels stay correct across Playground, trace
details, prompt select boxes, Optimizations, and Agent Runner even after
older versions are deleted.
- Merge the deep-link, Diff-menu, and Deploy-menu pagination triggers into a
single effect so at most one fetchNextPage() fires per render — two
separate effects both firing in the same render could cancel each other
via TanStack Query's default cancelRefetch behavior.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ination Inject the already-resolved activeVersion into the versions array passed to ComparePromptVersionDialog when it isn't yet in the paginated list. Without this, opening Diff while a deep-linked version's page was still loading made the dialog silently fall back to the newest loaded version instead of the one actually requested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Proposed a permanent test for this change. Test proposal — opik#8140 (
|
| # | Spec (all in tests/prompts/prompt-version-history-pagination.spec.ts) |
Capability | Result |
|---|---|---|---|
| 1 | Version history pages through all 30 versions once, in order (v30…v1 compared as a whole list), and the 30s background refetch leaves it unchanged |
@cap:prompts.version-history |
Passed — 56.7s |
| 2 | Diff menu offers 29 items past the page boundary; compare sheet titled Compare v2 → v30 with no third version label anywhere in it |
@cap:prompts.compare-versions |
Passed — 9.7s |
| 3 | "Deploy to" menu names Currently v3 for development when v3 is on the second page |
@cap:prompts.version-history |
Failed on this environment — kept, see below |
Also green: tag_lint.py → 60 specs checked, 1 exempt, 0 problem(s).
Why #3 is kept despite failing
pr-8140.dev.comet.com does not serve the PR head. The served bundle
(App-BXMEnoru.js) renders the Deploy dropdown as jsxs(DropdownMenu,{children:[…]})
with no onOpenChange — commit a7d83e15, the first of the PR's four commits.
The fix (60f0e337) is not deployed there.
Reproduced with the network instrumented: on a fresh load the Deploy menu fires
zero page-2 requests and shows no Currently note; open the Diff menu (which
does force-load) and reopen Deploy, and it reads development · Currently v3
with exactly one Currently note. So the assertion is correct and only the
force-load is missing — which is what the PR's own commit adds.
This is the "failed, and the app is at fault" case: it fails against a build
predating the fix. It is unverified against head 9562adf — no environment
serves that. CI on the target branch settles it; if it still fails there, drop it.
Dropped
0 of 3 candidates dropped. All three in candidates.json were strong and
verified_on_staging: true, and all three became specs.
Deliberately not written (from the exploration's own findings):
- The positional-vs-
version_numberlabelling divergence itself — unfalsifiable on OSS, which has no delete-prompt-version endpoint, soversion_number == positionalways. - The deep-link chase burning every remaining page on an unresolvable
activeVersionId— real but wasteful-not-wrong, and unchanged by this PR. PromptLibraryMenu's un-paginatedpage:1, size:25version list — pre-existing, out of scope.
Taxonomy
prompts.compare-versions → covered: true (t2-cuj). prompts.version-history
stays covered: true at t1-smoke (the smoke spec covers the shallow case), with
a note recording what the new spec adds. Spec added to the area's specs: list.
Gap worth filing: there is no prompts.deploy-to-environment key. Spec #3
belongs there; it is filed under version-history because inventing a key
silently breaks the coverage map. Deploying a version from that menu remains
untested either way.
Environment note
The backend at pr-8140.dev.comet.com 502'd for roughly 20 minutes during this
run (frontend kept serving). Not a product bug, but any suite pointed at that
host today will look flaky. The suite also needs OPIK_API_KEY set to any
non-empty value there: the pinned Opik TS SDK demands a key for every
*.comet.com hostname, even on this no-auth OSS install.
Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review.
|
🌙 Nightly cleanup: The test environment for this PR ( |
aadereiko
left a comment
aadereiko
left a comment
There was a problem hiding this comment.
version_numberis NULL for all pre-existing versions - i don't if you have considered this- Background request load multiplies by the number of loaded pages.
useInfiniteQuerystores the loaded pages as{ pages: [...], pageParams: [...] }.
A refetch re-fetches every loaded page, sequentially. Now combine that with what the PR carries over and adds:
- refetchInterval: 30000 (kept from the old code) — fires every 30 seconds.
- refetchOnWindowFocus defaults to true — fires on every tab focus.
- Every version create / delete / deploy invalidates this key.
- Opening the Diff or Deploy menu auto-pages through every version
useInfiniteQuery refetches every already-loaded page sequentially on any trigger, and the Diff/Deploy menus can load many pages for large prompts — so the prior refetchInterval/refetchOnWindowFocus multiplied request volume by however many pages a session had loaded. Drop those from the infinite query and instead poll the cheap prompt object (version_count) on the same interval, invalidating the versions list only when that count actually changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the review!
|
The prior fix's intent comment claimed refetchOnWindowFocus was off, but never actually set it — the default true still refetched every loaded page on window focus, defeating the point of bounding refetch cost to actual version_count changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…alog resets - usePromptVersionsByIdInfinite: getNextPageParam now treats an empty page as terminal instead of trusting `total`, which could loop forever if the two go out of sync (e.g. a concurrent delete between the count and the page query). Also gives the infinite-query cache key an explicit `view: "infinite"` marker instead of relying on the implicit (and breakable) "this hook never has page/size" invariant to stay distinct from usePromptVersionsById's key. - usePromptVersionHistory: memoize isChasingDeepLink so a stale/nonexistent activeVersionId doesn't re-scan the whole (growing) version list on every unrelated render while it chases pages. - ComparePromptVersionDialog: the selection-reset effect now fires only on the open transition (reading the rest via a ref) instead of on every versions/versionOptions change while already open, so a background refetch (pagination continuing, a version_count invalidation) no longer silently resets the comparison the user is looking at. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ination ComparePromptVersionDialog previously stored the selected base/diff PromptVersion objects as state, snapshotted on open. Combined with the open-only reconciliation effect (added to stop background refetches from resetting the user's selection), this meant a background refetch could leave the dialog rendering stale content, or a removed version rendering content that no longer exists. Store only the selected version id in state and re-resolve the actual version live from `versions` on every render instead: a background refetch now keeps rendered content current, and a version that disappears from the list resolves to undefined (renders nothing) rather than stale data — while still not resetting the selection on harmless background changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
anyMediaChanged alone gated the media section, unlike the Prompt/Metadata sections which already require both baseVersion and diffVersion. Since collectMedia falls back to empty media arrays when a version is undefined (e.g. deleted out from under an open dialog), any media on the surviving version read as "changed" and rendered — a one-sided comparison against a version that no longer exists. Add the same baseVersion && diffVersion guard already used by the other two sections. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Details
Version history on the Prompt tab only ever loaded the first 25 versions (a single
useQuery(page=1,size=25)), so older versions were unreachable, and a deep link to one silently fell back to the latest version while still showing a stalevNlabel. This switches to a paginatedusePromptVersionsByIdInfinitehook and wires the lazy-load already built intoVersionHistoryTimeline, and replaces client-computed index labels (v${total-idx}) with the backend's persistentversion_number(falling back tocommitfor pre-migration rows that never got backfilled). This also fixes OPIK-8189, where the Compare sheet recomputed labels from whatever was locally loaded instead of the true total, mislabeling versions past the first page.Along the way, a few related issues surfaced and are fixed in the same diff:
activeVersionIdquery param (now fetched independently of the current prompt's own list) could render a different prompt's content — guarded so it always falls back toprompt.latest_versioninstead.DeployToEnvironmentMenureceived the raw, unguardedactiveVersionIdinstead of the ownership-checked version — could deploy-to-environment against the wrong prompt's version.isFetchingflag instead of justisFetchingNextPage.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
Verified against a local dev instance (backend :8080, frontend :5174) using Playwright, plus
npx tsc --noEmitandnpx eslinton all touched files (all clean).Scenarios validated:
version_number(orcommitfallback) consistently across the sidebar, main panel, Compare dialog, and Deploy-to "Currently vN" tooltip.activeVersionIdin the URL shows the skeleton then renders correctly with no flash; resizing across thexlbreakpoint keeps mobile/desktop selection in sync.Not run: a read-only/no-permission role (local dev is single-admin with no RBAC configured) and the Playground-load flow's own confirm-dialog/navigation timing (that code is untouched by this diff; the touched code feeding into it was verified via code review instead of fighting flaky dropdown-open timing in automation).
Documentation
N/A — no public API or documented behavior changed beyond the bug fixes described above.