BE-705: Move the entities table view onto the table endpoint - #9095
BE-705: Move the entities table view onto the table endpoint#9095TimDiekmann wants to merge 19 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
There was a problem hiding this comment.
Reviewed the full diff against the actual base branch t/be-705-graph-entities-table-endpoint (this PR is stacked on #9094; endpoint behavior reviewed there, this review covers the frontend migration), with a focus on correctness and performance per BE-705. Items under Important are the ones I'd treat as blocking; Suggestions are non-blocking.
Important
1. Bulk archive/un-archive never refreshes the table once the user has paginated
handleBulkActionCompleted calls tableQuery.refetch() (apps/hash-frontend/src/pages/shared/entities-visualizer.tsx:691), which is Apollo's refetch of the current variables. After the user loads a second page, tableCursor is set, and three independent mechanisms each prevent the refresh from doing anything:
- Client dedup discards the response. The refetched page's cursor token is already in
consumedCursors, so the processing effect hitsif (continues && current.consumedCursors.has(pageCursor)) { return current; }(apps/hash-frontend/src/pages/shared/entities-visualizer/use-entities-table-query.tsx:296) and drops the fresh data entirely. The guard exists to dedupe Apollo's cache-then-network double emission, but it can't distinguish that from a deliberate refresh. - The server snapshot is pinned anyway. The cursor token seals
transaction_time/decision_time(libs/@local/graph/postgres-store/src/store/postgres/knowledge/entity/table.rs:352-374), so even without the dedup the refetched page would reflect the pre-mutation snapshot and could not show the archive. - Earlier pages and the summary are never refetched. Only the last page's variables are re-run, and
includeSummary: !cursoris false for a cursored request (use-entities-table-query.tsx:232), sototalResultCountalso stays stale.
Net effect: archive rows on any table with more than one loaded page and nothing on screen changes — no error, no update — until the user changes a filter/sort/view or reloads. The mutation succeeds server-side, so this reads as a silent failure. The single-page case works only because pageCursor === null makes continues false, which will hide this in casual testing.
The hook's public refetch needs "restart the accumulated sequence" semantics — reset the cursor, clear the accumulation, refetch cursor-less first-page variables — rather than "re-run the last page's variables". Note the in-place tableQuery.refetch() calls at entities-visualizer.tsx:806 and entities-visualizer.tsx:865 have the same problem once paginated.
2. Failed first table page shows a "Try again" button that retries the wrong (skipped) query
On the default /entities table view (unpinned, no selection — the most common page), a failed first queryEntitiesTable page produces a permanently dead retry button:
tableQuery.erroris mirrored intotableSummaryError(entities-visualizer.tsx:361-363), which flows into the external-modeSummarySource.- In
useAvailableTypes, external mode reportstypeUniverseError = summarySource.errorsince no summary ever arrived (apps/hash-frontend/src/pages/shared/entities-visualizer/shared/use-available-types.ts:258-259), makingtypeUniverseBlocksResultstrue (entities-visualizer.tsx:701-704). - The retry onClick branches on
typeUniverseBlocksResultsbeforeusesTableEndpoint(entities-visualizer.tsx:803-804), so it callsrefetchTypeUniverse— which in external mode is still the raw Apollo refetch of thesummarizeEntitiesquery (use-available-types.ts:291). That query is skipped (skip: !shouldFetchAvailableTypes,use-available-types.ts:129) and its data is ignored entirely by the external branches; Apollo's refetch punches throughskip(the repo documents this and guards it withguardedRefetchinuse-entities-visualizer-data.tsx:176-185— a guarduseAvailableTypeslacks).
So clicking "Try again" fires a wasted summarizeEntities request, tableQuery is never refetched, its error never clears, and the error screen persists no matter how many clicks — recreating exactly the dead-retry state the PR description says it set out to eliminate. Either check usesTableEndpoint before the type-universe branch in the onClick, or have useAvailableTypes return a no-op/caller-supplied refetch in external mode so the invalid wiring is unrepresentable.
3. Selecting a type pill drops the noisy-system-type exclusion, inflating the pills and changing row results
When selectedTypeIds is non-null the request sends types: { type: "include", entityTypeIds } and noisySystemBaseUrls is silently dropped — it is only sent in the exclude arm (use-entities-table-query.tsx:207-212). On the endpoint, base-URL exclusions live in the scope filter only for the Exclude variant, and the summary's type maps are computed over the scope before the type filter (table.rs:854-876, scope_summary). Two user-visible consequences:
- Pills flip on selection. The moment any pill is selected, the first page re-issues (cursor resets on filter change) and its summary — now the pill source via
SummarySource— spans the web/archived scope with no noisy exclusion, so User/Machine/Organization/notification/page types (the exact types FE-1238 removed) appear in the pill list and counts, then vanish when the selection is cleared. - Rows diverge from the other views.
buildEntitiesFilterappliesignoreNoisySystemTypesFilterwhenever the type is not pinned, including with a selection active (shared/build-filter.ts:180-182— the doc comment there explains the multi-type-entity case), so a multi-type entity carrying a selected type plus a noisy type was excluded before but appears now, and the Grid/Graph views (still on the subgraph builder) disagree with the Table view under the same filter state.
The endpoint request should carry the noisy base-URL exclusions alongside a selection (the Include variant would need to accept exclusions, or the client keeps them in scope), or at minimum the summary fed to the pills should be filtered client-side.
4. Every appended page regenerates table data for ALL accumulated rows (quadratic pagination cost)
The accumulation effect builds rows = [...current.rows, ...response.rows] and then calls generateTableDataFromEndpointRows over the entire accumulation, not just the new page (use-entities-table-query.tsx:313-337), synchronously inside the setAccumulated updater. Per row the generator does real work — getClosedMultiEntityTypeFromMap, a loop over every property in the closed type schema, allOf ancestor walks, two format() date calls, generateEntityLabel fallback — plus re-running the getTextWidth canvas measurement for every property column each fold. With limit: 500, page k reprocesses k×500 rows: at 20 pages each further load-more stalls the main thread re-deriving 10k+ already-rendered rows whose output cannot change. The deleted use-entities-table-data.tsx was deliberately incremental (generate for the new page only; append rows, union columns/sets).
Since the PR already plans to extract the fold for unit testing, extracting it as an incremental merge fixes both at once. The only genuinely whole-set outputs — the shared-type-title intersection and the all-rows-miss-source/target column hiding — can be carried as running aggregates/counters as the old code did.
5. A cursor surviving a pinned-type prop change builds a summary-less accumulation with no error — filter chips spin forever
resetCursors is wired to every in-component mutation (filters, sort, view, conversions — entities-visualizer.tsx:218-259) but nothing clears tableCursor when the entityTypeId/entityTypeBaseUrl props change, and EntitiesVisualizer is rendered without a key at both call sites, so Next.js query-param-only navigation (e.g. /entities?entityTypeIdOrBaseUrl=X → /entities, or between two type pages' entities tabs) keeps the component — and the stale cursor — mounted. The hook then sends the foreign cursor with includeSummary: !cursor = false. In the processing effect, continues is false (new requestKey) but pageCursor !== null, so the missing-summary guard if (pageCursor === null && !response.summary) (use-entities-table-query.tsx:307) is bypassed and a fresh accumulation is stored with summary: null (use-entities-table-query.tsx:338) — plus the rows are the foreign cursor's continuation page, silently skipping the first ~500 rows.
No error is raised: totalResultCount stays null and in external mode useAvailableTypes reports loading forever (summarySource.summary === null && !summarySource.error, use-available-types.ts:284) — exactly the endless-loading state the summary guard was added to prevent, since every follow-up request also carries a cursor. The hook should refuse (or clear) a cursor that wasn't issued for the current requestKey — e.g. only include cursor in the variables when it was produced by an accumulation with a matching key.
Suggestions
hadCachedContentis computed but never consumed. It runs a synchronousapolloClient.readQueryof the full 500-row response on every variables change (use-entities-table-query.tsx:368-376), is declared on the result type (use-entities-table-query.tsx:54) and churns the returned memo's identity, yetentities-visualizer.tsxnever readstableQuery.hadCachedContentand no other consumer exists. It appears copied from the subgraph hook's shape. Since this PR defines the new hook's contract from scratch, dropping the field and the cache read is free.cache-and-networkguarantees a wasted 500-row server query for any replayed continuation page. On a cache hit for a cursor-bearing page, the cache pass appends the page and marks the cursor consumed; the network pass then arrives and is unconditionally discarded by the guard atuse-entities-table-query.tsx:296(the file's own doc comment onconsumedCursorsanticipates this double emission). Since a continuation page is pinned to the first page's snapshot by design, freshness adds nothing — usecache-firstfor cursor-bearing pages, or process the re-emission by replacing the page in place.- Row data is retained in multiple copies with no eviction.
AccumulatedPages.rows(use-entities-table-query.tsx:76) keeps every raw page solely so the full-table regeneration can re-run (the incremental fix in Important #4 removes this need),tableData.rowsholds derived wrappers, and each cursor page's complete scalar response is cached under distinctrequestvariables in Apollo with nokeyArgs/eviction policy — so per-cursor cache entries (including abandoned filter sequences') grow unboundedly for the session. A type policy that avoids caching cursor pages, or evicts consumed ones, would cap this. - The processing effect is keyed on
hideColumnsarray identity (use-entities-table-query.tsx:366): a caller passing a literal array would loop regeneration on every render for first pages (the PR knows —entities.page.tsxadds auseMemowith a comment saying exactly that), while on continuation pages a changedhideColumnssilently doesn't apply due to the consumed-cursor guard. Column hiding is cheap post-processing overcolumns(generate-table-data-from-endpoint-rows.ts:132-151); deriving visible columns in a separate memo outside the accumulation removes both the identity hazard and the asymmetry. - A failed same-variables refresh is mislabelled as a failed load-more.
dataIsStaleis only true whenaccumulated.requestKey !== requestKey(use-entities-table-query.tsx:405), so when the bulk-actiontableQuery.refetch()rejects, the banner atentities-visualizer.tsx:856says "Something went wrong loading more entities" even though the on-screen rows are stale pre-mutation results — the "showing the previous results" wording is exactly what applies but is unreachable here. An in-flight-refresh flag would let the banner label it correctly. hasPinnedTypes+resolvedPinnedEntityTypeIdsis a two-field invariant the type system doesn't enforce (use-entities-table-query.tsx:143): the request builder usesresolvedPinnedEntityTypeIds ?? ...without consultinghasPinnedTypes, sofalse+ non-null array silently pins, whiletrue+ empty array flipsunresolvablePins. A discriminated parameter (e.g.pinnedTypes: null | { resolvedIds: VersionedUrl[] | null }, mirroring theSummarySourcepattern this PR introduces) makes the invalid states unrepresentable. No live bug — the sole caller maintains the invariant today.- The builder-agreement test hand-picks 9 cases and leaves most of the closed operator space unpinned (
build-property-filter-clause.test.ts:86). Its own comment states the invariant absolutely ("the two must never disagree on null-ness" —isPropertyFilterActiveanswers for both paths via one builder), yetnotEquals,greaterThanOrEqual,lessThan,lessThanOrEqual,isEmpty,isTrue,isFalse, the entirebooleankind, and the whitespace-only number branch (build-property-filter-clause.ts:28) are never exercised. Because the gates are duplicated per-operator in both builders, a divergence in any untested branch passes this suite while producing exactly the failure the test exists to prevent. Both unions are small and closed — enumerating operators × kinds × a fixed value set fully pins the invariant at negligible cost. - The rewritten row-to-table transform has zero tests (
generate-table-data-from-endpoint-rows.ts:190), despite being a pure function (bar a one-linegetTextWidthmock) and despite this same PR adding a vitest suite for a sibling pure helper. None of the PR-new behavior is exercised: theendpointRow.label ?? generateEntityLabel(...)fallback,linkEndpointCellfalling back toendpoint.entityIdfor permission-hidden endpoints (the PR body's advertised one-sided-link behavior), theindex > 0shared-title guard, the page-vs-endpointarchivedsplit, the empty-rows edge of source/target column hiding, and the threethrow new Errorcontract checks feeding the new Sentry/error surfacing. A table-driven suite with a few syntheticEntityTableRowfixtures would pin all of these. - The already-pure merge helpers need not wait for the deferred fold extraction. The Known Issues section defers unit-testing the accumulation fold, which is reasonable for the React-coupled parts — but
mergeClosedMultiEntityTypeMaps(use-entities-table-query.tsx:91) andmergeDefinitions(use-entities-table-query.tsx:114) are pure module-level functions testable with just anexport. The recursive merge has real edge behavior worth pinning (source page'sschemasilently wins on collision at line 102); a regression corrupts the closed-type map for every multi-page table and nothing would catch it.
Strengths
- The core BE-705 goal lands: the table view truly skips the heavy subgraph path (
use-entities-visualizer-data.tsxskips both the summarize and subgraph queries whenview === "Table"), the summary is requested only on the first page (includeSummary: !cursor), and the type pills reuse it through the discriminatedSummarySource— eliminating a full extra round trip and sourcing pills from the same transaction as the page. - The page-accumulation state machine is carefully reasoned for its main hazards: consumed-cursor dedup neutralizes Apollo's cache-then-network double emission (safe precisely because the endpoint cursor pins the snapshot), a stale accumulation never offers its continuation (
cursorgated onisCurrent), a failed page leaves its cursor unconsumed so retry genuinely reprocesses it, and therequestKeyguard quarantines late responses from previous filter sets (use-entities-table-query.tsx:292-343). - Processing responses in an effect instead of Apollo's production-exception-swallowing
onCompleted, with explicit contract-break guards (missing definitions/closed types/first-page summary) surfaced to both the error UI and Sentry, closes a real invisible-failure class the oldupdateTableDatapath had. - The builder-agreement test plus the kind×operator gates retrofitted onto
buildPropertyFilterClausefix a genuine pre-existing bug (string parameters sent togreater/less) and pin the invariant thatisPropertyFilterActiveanswers for both query paths. - The shared-type-title logic fixes the old code's index-0 self-wipe (the prune loop ran against an empty set on the first entity, so the header was always "Entity"); the
index > 0guard makes the feature work for the first time. - One-sided link handling is more correct than before: per-side
noSource/noTargetcounting replaces the old both-or-nothinglinkDatabranch, and endpoint types are verifiably present in the response's closed-type maps, solinkEndpointCell's lookups are safe. - Per-row client work drops meaningfully: rows prefer the server-materialized
endpointRow.label, and link endpoint cells come from denormalized endpoint fields instead of subgraph traversal (getEntityRevision/generateLinkEntityLabelremoved). ArchivableEntityis a well-judged structural slice — the doc comment explains precisely why it is not aPickofEntity, and theisTypediscriminant remains sound for the new union inBulkActionsDropdown.- Hot identities are handled deliberately: the
closedMultiEntityTypesmemo dedupes by sorted type-id key instead of recomputing per row, andresetCursors/conversions/hideColumnsare memoized at the call sites to keep the query variables stable.
Generated by Claude Code
2222708 to
10edad5
Compare
10edad5 to
71e0bfa
Compare
02952e2 to
fe51fe8
Compare
PR SummaryMedium Risk Overview
Removes client-side subgraph → table assembly ( Reviewed by Cursor Bugbot for commit 4b0412e. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The
Generated by Claude Code |
|
e44b01e cleared the tsc errors, but That's the Verified fix: annotate branch 2's callback so both branches are plain rowTypeIdLists:
entities?.map((entity): VersionedUrl[] => entity.metadata.entityTypeIds) ?? [],
Generated by Claude Code |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4b0412e. Configure here.

🌟 What is the purpose of this PR?
Moves the entities table view off the subgraph query and onto the dedicated
queryEntitiesTableendpoint. The table renders from flat endpoint rows, and its data flow lives in a hook decoupled from the subgraph path — the Grid and Graph views stay on the old query, whose hook goes back to serving only them.🔗 Related links
🔍 What does this change?
useEntitiesTableQuery(new): fetches pages through the endpoint, accumulates rows across pages, deep-merges the closed-type maps and definitions, and processes responses in an effect — Apollo'sonCompletedswallows exceptions in production. ArequestKeyresets the accumulation when the filters change, consumed cursors dedupe Apollo's cache-then-network double emission, and a failed page keeps prior rows on screen with its cursor unconsumed so a retry reprocesses it.useAvailableTypesthrough a discriminatedSummarySource, so the pills come from the same transaction as the page instead of a second summarize call.ArchivableEntityslice, so table rows qualify without carrying full entity metadata. One-sided links (an endpoint hidden by permissions) render their visible side.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
useEntitiesTableQueryis not unit-tested yet — extracting it into a pure function with vitest coverage is a planned follow-up.🐾 Next steps
useEntitiesTableQueryand unit-test it🛡 What tests cover this?
build-property-filter-clause.test.ts: value coercion, incomplete and kind-mismatched filters staying inert, and the endpoint/subgraph builder agreement on null-ness❓ How to test this?
yarn dev, open/entities📹 Demo
(screenshots to follow — UI is visually unchanged, the data path is new)