Skip to content

improvement(tables): harden pagination for short pages and surface name-validation errors#5351

Open
TheodoreSpeaks wants to merge 2 commits into
stagingfrom
debug/table-row
Open

improvement(tables): harden pagination for short pages and surface name-validation errors#5351
TheodoreSpeaks wants to merge 2 commits into
stagingfrom
debug/table-row

Conversation

@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator

Summary

  • grid pagination now terminates on an empty page or a covered totalCount instead of rows.length < pageSize, so a short server page can never be misread as end-of-table (prep for byte-bounded pages / larger rows). Termination logic lives in a new pure module hooks/queries/utils/table-rows-pagination.ts shared by getNextPageParam and the use-table drain loops
  • offset fallback resumes at the actual loaded-row count (not pages × pageSize); drain loops get a fail-fast no-progress guard
  • validateRowSize measures UTF-8 bytes (Buffer.byteLength) instead of UTF-16 code units — a "400KB" CJK row was actually ~1.2MB on disk/wire
  • dev-preview TABLE_MAX_PAGE_BYTES env (off by default): server cuts row pages at a byte budget, always keeping ≥1 row. Used to exercise the client termination end-to-end; the production SQL-side cut is a follow-up
  • renaming/creating a table with an invalid name now shows the validation message as a toast instead of silently reverting the field

Type of Change

  • Improvement

Testing

New pure-fn test suites (pagination termination, byte trim) + rewritten use-table tests incl. first coverage of ensureRowsLoadedUpTo and the short-page-continues regression case. tsc, lint:check, check:api-validation:strict, check:react-query, check:client-boundary all pass. Manually exercised locally against a seeded 60×400KB-row table with the byte cut enabled.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Jul 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 2, 2026 2:46am

Request Review

@cursor

cursor Bot commented Jul 2, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes core table scroll/drain and row-size enforcement paths used by bulk ops and limits; behavior is well covered by new tests but incorrect termination could miss rows or over-fetch.

Overview
Hardens table row infinite pagination so a page shorter than the requested size is never treated as end-of-table. Termination now uses a shared helper (hasMoreTableRows / getNextTableRowsPageParam) keyed off empty pages and page-0 totalCount, with the next offset based on loaded row count (not pages × pageSize). Grid drain loops (ensureAllRowsLoaded, ensureRowsLoadedUpTo) use the same rules and fail fast when fetchNextPage adds no page (e.g. cancelQueries races).

Adds an optional dev TABLE_MAX_PAGE_BYTES path: the row list service can trim a fetched page to a serialized-data byte budget (always keeping ≥1 row), aligned with the new client termination behavior.

validateRowSize now measures UTF-8 bytes of serialized row data instead of UTF-16 length. Create/rename table mutations show the first validation issue in a toast instead of silently failing on name rules.

Tests cover the pagination module, byte trim, rewritten use-table drain/ensureRowsLoadedUpTo cases, and updated getNextPageParam expectations.

Reviewed by Cursor Bugbot for commit 431e177. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR hardens table row pagination by replacing the rows.length < pageSize termination heuristic with a count-based approach (hasMoreTableRows), extracting the logic into a shared pure module and adding fail-fast no-progress guards to the drain loops. It also fixes validateRowSize to measure UTF-8 bytes via Buffer.byteLength instead of UTF-16 code units, adds a dev-preview TABLE_MAX_PAGE_BYTES byte-budget cut for exercising client termination, and surfaces table name-validation errors as toasts instead of silently reverting the field.

  • Pagination termination: new hasMoreTableRows / getNextTableRowsPageParam utilities in hooks/queries/utils/table-rows-pagination.ts are shared by getNextPageParam and both useTable drain loops; offset fallback now uses actual loaded-row count rather than pages × pageSize, closing a gap on short server pages.
  • Byte measurement fix: validateRowSize and trimRowsToByteBudget both use Buffer.byteLength(JSON.stringify(...)) for UTF-8–correct size checks; a CJK row that was previously ~1/3 its real size in the old string.length count is now measured correctly.
  • Error surfacing: useCreateTable and useRenameTable onError handlers now call extractValidationIssues(error)[0]?.message ?? error.message so the first validation issue is shown as a toast rather than silently dropped.

Confidence Score: 5/5

Safe to merge — all production paths are additive or corrective fixes with no regressions introduced.

The pagination termination rewrite is well-reasoned: the new count-based rule is strictly more correct than the old page-fullness heuristic, the shared utilities are pure functions with comprehensive unit tests, and the drain loops gain explicit no-progress guards. The UTF-8 byte fix closes a real undercount for multi-byte content, and the validation toast change strictly improves user feedback. The dev-preview byte-cut is disabled by default and isolated behind a null-check.

No files require special attention. The acknowledged stale-low totalCount early-exit in hasMoreTableRows and the per-row envelope exclusion in trimRowsToByteBudget are both explicitly documented and accepted as pre-production trade-offs.

Important Files Changed

Filename Overview
apps/sim/hooks/queries/utils/table-rows-pagination.ts New pure module providing countLoadedTableRows, hasMoreTableRows, and getNextTableRowsPageParam; termination logic is well-documented and covered by a dedicated test suite.
apps/sim/app/workspace/[workspaceId]/tables/[tableId]/hooks/use-table.ts Both drain loops updated to use hasMoreTableRows; fail-fast no-progress guard added; hasMore return value now also accounts for hasMoreTableRows(pages).
apps/sim/hooks/queries/tables.ts getNextPageParam delegates to getNextTableRowsPageParam; validation error toasts now surface the first issue message via extractValidationIssues for both create and rename mutations.
apps/sim/lib/table/rows/paging.ts New trimRowsToByteBudget helper; measures only row.data bytes with documented headroom requirement, always keeps ≥1 row for forward progress.
apps/sim/lib/table/validation.ts validateRowSize corrected from UTF-16 string.length to Buffer.byteLength — fixes undercount for CJK/emoji content.
apps/sim/lib/table/rows/service.ts queryRows now applies trimRowsToByteBudget when TABLE_MAX_PAGE_BYTES is set; off by default; no change to production path.
apps/sim/lib/table/constants.ts Added getMaxPageBytes() reading the new TABLE_MAX_PAGE_BYTES env var; returns null when unset or zero.
apps/sim/lib/core/config/env.ts Added optional TABLE_MAX_PAGE_BYTES numeric env var; correctly optional and integer-validated.

Reviews (2): Last reviewed commit: "fix(tables): align getNextPageParam test..." | Re-trigger Greptile

Comment thread apps/sim/lib/table/rows/paging.ts
Comment thread apps/sim/hooks/queries/utils/table-rows-pagination.ts
@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator Author

Addressed review notes:

  • trimRowsToByteBudget TSDoc now states the budget counts data only (envelope excluded — callers leave headroom; the production SQL cut must account for the same overhead)
  • hasMoreTableRows TSDoc now documents both staleness directions precisely: stale-high self-corrects via the empty-page rule; stale-low stops the drain early, accepted because the view is already stale and run-stream/interval invalidations refetch

CI failure was hooks/queries/tables.test.ts still asserting the old rows.length < pageSize termination (and passing an empty allPages, which the new implementation reads) — rewritten to the count-based semantics incl. the short-page-continues regression case.

@greptile review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant