Improve large Content database performance - #2522
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest head addresses all four previously reported correctness defects: Builder attachment now reloads the imported document set for binding/hydration, and created/duplicated mutation projections use document identity instead of position-derived offsets. I verified those fixes and resolved the four stale review threads before this review.
The remaining implementation is directionally sound: server-side constrained reads remain page-bounded, retained-row loading is preserved, bulk SQL work is chunked, and Builder review preparation remains progressively scoped. One new access-control issue remains in personal sidebar-order validation: the replacement query checks database membership but not the same per-document visibility constraints used by the database response path.
Key finding
- 🟡 MEDIUM —
update-content-database-personal-viewcan accept IDs for hidden or inaccessible documents. Apply the existing visibility/access predicates before persisting sidebar order.
Risk assessment: Standard (shared Content actions and user-visible data scoping).
🧪 Browser testing: Will run after this review (PR touches UI code)
| const rows = await getDb() | ||
| .select({ id: schema.contentDatabaseItems.id }) | ||
| .from(schema.contentDatabaseItems) | ||
| .where( | ||
| and( | ||
| eq(schema.contentDatabaseItems.databaseId, databaseId), | ||
| inArray(schema.contentDatabaseItems.id, itemIds), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🟡 Scope sidebar-order validation to visible items
This membership-only query accepts any item ID belonging to the database, whereas the previous getContentDatabaseResponse path filtered items through document visibility/access rules. A viewer can therefore persist an inaccessible document ID in personal sidebar order; validate IDs through the same access-scoped projection before saving them.
Additional Info
Found by 1 of 3 incremental review agents; confirmed against the prior visibility-filtered response path.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest commits correctly address the previously reported mutation projection and Builder attachment bugs: created/duplicated rows are now fetched by document identity, sparse-position regression coverage was added, and initial Builder setup is scoped to imported document IDs. The prior sidebar-order visibility comment remains applicable and was intentionally not reposted because it is already open.
One new correctness regression remains in the table view path. Grouped table views no longer request a complete client dataset unless they use a secondary-federation constraint, so a grouped database with more than one page only groups the first 100 rows and omits later groups/counts. This is a product-semantic regression from the former grouped expansion behavior and should be fixed before merge.
Risk assessment: Standard (shared Content UI and action/data behavior).
🧪 Browser testing: Will run after this review (PR touches UI code)
| return ( | ||
| (args.viewType === "table" && args.tableQueryMode === "client-required") || |
There was a problem hiding this comment.
🟡 Preserve complete data for grouped table views
Grouped table views now remain on the initial page unless tableQueryMode is client-required. For databases with more than one page, toolbarGroups and grouped rendering therefore see only the first 100 rows and omit later groups/counts; retain the complete-dataset path for grouped views or add server-side grouping semantics.
Additional Info
Found by 1 of 2 incremental review agents; confirmed against the prior grouped-table expansion behavior.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest commit fixes nested object GET parameters by JSON-encoding them on the client and exercising the corresponding server-side restoration path. The new round-trip tests cover the intended tableQuery case, and the two previously reported Content issues remain open and were not reposted.
One new compatibility regression is present in the generic action query serializer: the new typeof value === "object" branch also captures non-plain objects such as Date and URLSearchParams. Dates now become JSON-quoted strings rather than their established string form, and URLSearchParams instances become {}, changing or losing values for existing GET action callers. Restrict JSON encoding to plain records while preserving prior handling for other object types.
Risk assessment: Standard (shared core action transport used across the application).
🧪 Browser testing: Will run after this review (PR touches UI code)
| if (typeof value === "object") { | ||
| // defineAction restores JSON strings when the schema expects an object. | ||
| // Preserve nested GET params instead of collapsing them to "[object Object]". | ||
| qs.append(key, JSON.stringify(value)); |
There was a problem hiding this comment.
🟡 Preserve non-plain GET parameter values
This branch JSON-serializes every object, including Date and URLSearchParams. A Date now arrives JSON-quoted instead of in its prior parseable string form, while URLSearchParams serializes to {} and loses its entries; restrict this path to plain records and retain the existing string conversion for other object types.
Additional Info
Found by 1 of 3 incremental review agents; confirmed from the changed serializer behavior.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest commit optimizes Builder field refreshes by treating projected fields absent from a successful non-raw response as explicit empty values, allowing already-materialized source snapshots to avoid redundant provider reads. The added database coverage exercises both the no-reread empty-field path and the stale-snapshot refresh path. The three previously reported comments remain unresolved and were not reposted.
Two new medium-severity correctness risks remain in the absence-preservation boundary:
- Nested projected paths are accepted by normalization, but the source adapter stores top-level
data.*values rather than independently materializing nesteddata.foo.barkeys. The new loop can therefore overwrite real nested provider values with authoritative nulls. - MCP search fallback can retain an entry after per-entry hydration fails; absence-preservation then treats missing projected keys as known empty and suppresses the required retry/backfill path.
Risk assessment: Standard (shared Builder provider read and Content source-field behavior).
🧪 Browser testing: Will run after this review (PR touches UI code)
| for (const fieldPath of projectedFieldPaths) { | ||
| if (!Object.prototype.hasOwnProperty.call(sourceValues, fieldPath)) { | ||
| sourceValues[fieldPath] = null; |
There was a problem hiding this comment.
🟡 Do not mark nested Builder projections as absent
The normalizer accepts paths such as data.author.name, but the source adapter materializes the containing data.author value rather than a data.author.name key. This loop can therefore add data.author.name: null even when the provider returned a real nested value, causing the source-field backfill to skip its refresh and bind an empty property.
Additional Info
Found by 1 of 3 incremental review agents; confirmed against normalizeBuilderCmsApiEntry and builderSourceValuesFromRecord.
| fetchImpl, | ||
| privateKey, | ||
| }); | ||
| return preserveProjectedBuilderFieldAbsence( |
There was a problem hiding this comment.
🟡 Do not treat failed MCP hydration as an empty field
The MCP search fallback retains the original entry when per-entry hydration fails. Passing that partial result through absence preservation turns missing projected fields into authoritative nulls, so source-field creation can skip its Builder reread and persist empty values instead of retrying or surfacing the unreadable state.
Additional Info
Found by 1 of 3 incremental review agents; confirmed against the per-entry hydration fallback.
2 participants
Problem
Content databases with hundreds or thousands of rows did complete-corpus source and client work before showing a useful table. In the recorded 584-row blog workflow, ordinary actions could leave the interface blank for seconds. Large reads also amplified mutation refreshes and Builder review preparation.
Approach
Keep Content actions as the shared UI and agent boundary, but make ordinary work proportional to the requested page or review scope. Table constraints run in portable SQL when their semantics are supported; visible rows remain in place during refresh; mutations return exact bounded deltas; Builder source binding and review work stay progressive.
What changed
Safety and operations
There is no schema migration or provider write in this change. Access filtering, typed failures, source provenance, optimistic rollback, and authoritative document bodies remain on their existing action paths. The change is not feature-flagged: the shaped risk strategy requires system-ready acceptance before merge. Temporary timing probes and task fixtures were removed; the corrected resource ledger records action-level marker-absence proof.
Verification
Current PR head:
03c1a6f40. The final commit records the Land no-go packet only; implementation revisiond07d402f6is the code revision covered by the evidence below.Required acceptance is still blocked, not deferred: the exact Builder-provider attach/hydration/review replay lacks a local vault credential, and independent human QA could not start because its host did not expose the in-app browser. The persisted system-ready no-go packet prohibits merge.
Review focus