SCRUM-6436 preserve the sequence viewer for standalone variants - #1665
Conversation
|
@claude Please review this focused follow-up to merged PR #1664 together with alliance-genome/agr_ui#1875 at |
|
Claude finished @christabone's task in 6m 56s —— View job Review: SCRUM-6436 —
|
Review — SCRUM-6436 standalone variant viewer signalScoped to the three changed Java files (one service method, one OpenAPI description, test updates). I traced the new query against the ES mappings, The change looks correct. Specifics I checked:
Low-severity note (non-blocking)
Test noteThe old assertion I did not execute the test suite (no populated Maven repository in this environment), so the 10-passing-tests claim in the description is unverified here. |
|
@claude Addressed the test note in |
|
Claude finished @christabone's task in 2m 54s —— View job Confirming head
|
| Suite | Result |
|---|---|
AlleleESServiceTest |
7/7 pass |
ESServiceSourceProjectionTest |
2/2 pass |
GeneControllerTest |
1/1 pass |
BUILD SUCCESS on both invocations — the 10-test claim in the PR description is verified.
On the deferred count optimization
Accepted, and I think the reasoning is right. My findings #1 (per-page repetition) and #2 (exact count for a boolean) both scaled with page count, and your measurement — mapped-ID cardinality fits in one page for the representative genes, six-gene stage query at ~222 ms — collapses both to a single probe per gene view. At that shape, track_total_hits on a filter-only bool is not worth pulling the shared SearchDAO into a three-file correction. If a gene ever crosses 1000 mapped IDs, the page-1 gate is still the cheap fix and stays a one-liner in AlleleESService.
Two items from my earlier review that this commit doesn't touch
Neither blocks; flagging so the deferral is deliberate rather than incidental.
-
The probe's boundedness still has no assertion (my AGR-1002 add automatedGeneSynopsis to GeneDocument #5).
CapturingAlleleESServicerecordscapturedPaginationonly whenresponseIndex == 0(lines 198-200), so a future edit droppingsetSourceIncludes(List.of("category"))or raising the limit atAlleleESService.java:252-253would pull full_sourceon a 99k-row gene with all 7 tests still green. Capturing both paginations and assertinglimit == 1/sourceIncludes == ["category"]on the second is the assertion that actually guards the SCRUM-6436 bound allele viewer payloads #1664 fix — a smaller change than the one you just made. -
A probe failure still 500s a payload that already succeeded (my Added OpenAPI spec to wildfly AGR-886 #4).
SearchDAO.performQueryswallowsIOExceptionand returnsnull, soAlleleESService.java:255NPEs and discards the identifier list computed one line earlier. A try/catch around line 240 that logs and omits the supplemental field keeps a transient failure on an optional hint from taking down the required result.
The still-open product question from my first review — a gene with standalone variants and zero mapped alleles mounts the viewer with allelesVisible: [] — is unchanged by this commit and is a conversation with Jim, not a code change.
· Branch: scrum-6436-standalone-variant-viewer-ids
|
@claude Addressed the remaining boundedness test gap in |
|
Claude finished @christabone's task in 3m 28s —— View job Confirming head
|
| Suite | Result |
|---|---|
ESServiceSourceProjectionTest |
2/2 pass |
AlleleESServiceTest |
7/7 pass |
GeneControllerTest |
1/1 pass |
| Total | 10/10, BUILD SUCCESS |
On not swallowing probe failures — agreed, and for a sharper reason than "don't hide errors"
Your argument is correct and I'll withdraw my #4 suggestion. The decisive point is that omission is indistinguishable from a genuine false on the wire. hasViewerContent reads the flag only when results.length === 0 — precisely the RGD:2917 shape — and an absent field is falsey there. So a 200-with-omitted-field is not a degraded answer, it's a wrong answer wearing a success code, and staleTime: Infinity makes it wrong for the rest of the session with no event that would refetch it.
Laying out all three options against that:
| Disposition | Transient probe failure produces | Recoverable? |
|---|---|---|
| Omit field (my #4) | viewer hidden on a gene that should show it | no — cached for the session |
Fail open (true) |
empty viewer mounted on a gene with no content | no — cached for the session |
| Fail fast (current) | error state | yes — errors bypass staleTime and refetch on remount/retry |
Both non-fail-fast options cache a wrong answer; only fail-fast produces a state react-query will ever re-ask about. Given that the flag is load-bearing exactly when the required result is empty, "partial success" isn't a meaningful state for this endpoint — it degrades the same field it exists to deliver. Fail-fast is right.
Two things that make it hold up in practice, both of which I checked:
- The failure is observable, not silent:
GeneController.getAlleleViewerIdscatches, logs"Error while retrieving allele viewer identifiers"with the stack, and throwsRestErrorExceptionwith a structured message (GeneController.java:155-160). A probe failure shows up in logs rather than as a mystery 500. - The failure shape is uniform with the identifier query —
SearchDAO.performQueryreturningnullonIOExceptionalready NPEs atAlleleESService.java:226for the primary query, so the probe doesn't introduce a new failure semantics, just a second instance of the existing one. Nothing to reconcile.
Purely optional polish, not worth a commit on its own: the fail-fast path is reached via NPE-on-null rather than an explicit check, so the thrown type is incidental to the design. If SearchDAO's null-on-IOException contract is ever tightened, an explicit if (searchResponse == null) throw new IllegalStateException(...) at both call sites would state the intent. Today it behaves identically.
Remaining open items (all previously deferred, none blocking)
- Per-page repetition (my AGR-949 Increased boost on category:gene to 1.1 #1) — deferred on your measurement that mapped-ID cardinality fits one page for representative genes, which collapses it to one probe per gene view. Still a one-liner page-1 gate in
AlleleESServiceif a gene ever crosses 1000 IDs. - Exact count for a boolean (my add associationType column #2) — deferred to avoid pulling shared
SearchDAOinto a three-file correction. Agreed; ~222 ms on the six-gene stage query is not a problem worth that blast radius. - Delta B (my Added fail on all errors #3) — the probe's
existsQuery("variantList.curatedVariantGenomicLocations.hgvs")is narrower than the table query, which accepts standalone rows with anyvariantList. Worth the stage check on whether all 916 of RGD:2917's rows carry hgvs, since that's the one place "exactly preserves" doesn't literally hold. Not a code change. - UI:
getVisibleViewerAlleleIdsoverride branch (agr_ui#1875) — returnsselectionOverride.alleleIdswithout theALLELE_WITH_*_VARIANTcategory filter the deleted code applied. Depends on howuseAlleleSelectionpopulates it. - Product question — a gene with standalone variants and zero mapped alleles mounts the viewer with
allelesVisible: []. Faithful to the old UI; whether faithful is right is a conversation with Jim, not a code change.
From my side the Java change is ready. Nothing in 3a62e2218 regressed, and the OOM fix from #1664 is now protected by a test that fails when the projection is removed.
· Branch: scrum-6436-standalone-variant-viewer-ids
|
@oblodgett The focused post-#1664 follow-up is fully updated at Review-driven hardening completed:
Paired UI PR alliance-genome/agr_ui#1875 is green and Claude-marked LGTM at |
Summary
supplementalData.hasStandaloneVariantsusing the same gene and table filterscategoryprojection instead of returning their high-cardinality HGVS setWhy
Jim identified that Rat
RGD:2917has 916 standalone variant rows but zero mapped allele IDs. The old UI mounted the viewer because those rows have non-emptyvariantListvalues; the new UI incorrectly interpreted the empty ID result as no viewer content.The stage alias also contains genes with roughly 99,000 standalone/high-throughput variant rows. Returning every HGVS value would create unnecessary paging for records that are not in the embedded allele-associated VCF. A bounded presence signal restores viewer mounting without compromising the OOM fix.
Stage evidence
RGD:2917: 0 mapped allele IDs, 916 standalone mapped variantsRGD:2332: 6 mapped allele IDs, 1,355 standalone mapped variantsVerification
GeneControllerTest,AlleleESServiceTest,ESServiceSourceProjectionTestcategoryfield82edbfa3