Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
|
AI review status for this pull request.
|
|
/approve-spec d057c47 |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the commit-time caches fix the re-renders after a discarded render (both new tests fail on main and pass here), but two small issues remain.
- Under
StrictMode, each plugin validation warning now prints twice on mount and on every plugin change, because the second render pass misses the cache andmergePluginsvalidates again (useBaseTablePlugins.ts:196-201); validate in the layout effect only when the committed array changes. - The guard test's plugin adds a column keyed
value, whichtestColumnsalready uses, so React logs duplicate-key errors and the assertion relies on unsupported behavior (Table.perf.test.tsx:713); use a unique key such asextra.
A few non-blocking comment wording suggestions are inline. CI has not run on this head yet because the pull request is a draft.
[Automated review]
d057c47 to
00016b6
Compare
|
/approve-spec 00016b6 |
There was a problem hiding this comment.
Requesting changes: both caches now keep their committed arrays, and the earlier StrictMode double-warning and duplicate-key issues are fixed; one wording issue remains.
- Miss: the changeset says the next committed render "no longer re-renders every row", but a Table with the tree plugin over expandable rows still re-renders every row after a discarded render with other columns, because that plugin writes its own column cache during render; narrow the sentence to Table's own column and plugin caches (
.changeset/table-caches-at-commit.md:5).
CI's test jobs have not run on this head because the pull request is a draft.
[Automated review]
BaseTable reuses its last column array while the columns are shallowly equal, and useBaseTablePlugins reuses its last plugin array while every plugin is the same instance. Both caches were written while rendering. A render React discards, as a suspended transition does, left its own columns or plugins there. The next committed render then missed the cache, and every memoized row rendered again. Each cache now holds the last committed array. A render reads it, and a layout effect writes it at commit. The plugin hook also validates its plugins there, only when the committed array changes, so each warning prints once under StrictMode.
00016b6 to
b18ca6f
Compare
|
/approve-spec b18ca6f |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
Both Table caches now keep their committed arrays, so a render React discards no longer makes the next commit re-render every row, and the earlier StrictMode double warning, duplicate test column key and changeset wording are fixed. CI's test jobs skip drafts, so mark the pull request ready for review; its branch is in a fork, so a maintainer adds the ci:run label so CI runs.
[Automated review]
cixzhang
added
the
ci:run
|
/approve-spec 7998c38 |
There was a problem hiding this comment.
Requesting changes: both Table caches now keep their committed arrays, so a render React discards no longer makes the next commit re-render every row, but the new commit-time writes never run inside a hidden <Activity>.
- Miss: inside a hidden
<Activity>React runs no layout effect, so once the columns or a plugin change while hidden, every later hidden render that passes a new array of the same columns or a new record of the same plugin, and the reveal, re-render every row where main re-renders none (useBaseTablePlugins.ts:203-210,BaseTable.tsx:622-624); write both caches in an insertion effect, asBottomSheet.tsxalready does for its commit-time ref, and add the hidden case beside the two discarded-render tests.
Swapping the two useIsomorphicLayoutEffect calls for useInsertionEffect fixes the hidden case and keeps all 19 Table.perf tests passing. CI's test jobs skip drafts, so they have not run on this head.
The column identity cache and the merged plugin array lived in refs that render read and a layout effect wrote. Each now comes from one internal helper that returns one array per list of objects, compared by identity and in order. It keeps a copy, never a caller's array. No state or ref is written during render, and a render React discards cannot replace the committed array. State adjusted during render loops for generated columns and for plugins written inline, which are new on every pass. The helper's result depends only on the items, so it cannot loop, and it adds no render pass. Base plugins now compare by content, so an inline base array keeps its merged array. Plugin validation stays an Effect, once per merged array.
vjeux
changed the title
The Owns item names architecture:react-component-runtime in full for INV17, so the knowledge check can follow the reference.
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 3a98d1f |
github-actions
Bot
added
needs:spec-owner-review
|
/approve-spec 1fe0959 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Requesting changes: both Table caches now come from one identity-keyed helper, so a render React discards no longer makes the next commit re-render every row, and the change itself looks right. Two things need fixing first:
- The new Owns item in
docs/specs/components/Table/Table.spec.md(line 132) citesarchitecture:react-component-runtimeINV17, whichmaindoes not have while #10161 is open, so land #10161 first and update this branch, or drop the citation. - The description says that running
Table.perf.test.tsxagainst main's sources fails two red-first tests and passes the other 17, but the file now has 24 tests and three fail on main (the two discarded-render tests and the inline base-array test at line 771), so please restate it as three red-first and 21 passing.
Non-blocking: useBaseTablePlugins now returns one array shared by every caller with the same plugins while its return type stays mutable, so a caller that mutates it changes other callers' arrays. CI's test jobs skip drafts, so they have not run on this head.
Claims and files checked
Claims:
- Changeset: Table keeps one array per list of columns and per list of plugins, compared by identity: verified (
BaseTable.tsx:594,useBaseTablePlugins.ts:190). - Changeset: a discarded render can no longer replace the array the table last showed: verified (no ref is written during render;
internByIdentity.ts:29-41). - Changeset: the next commit reuses the arrays, so rows skip rendering when columns and plugins match: verified (
Table.perf.test.tsx:628,:691, traced against the base sources). - Changeset: the hook keeps its array for a new base array of the same plugins: verified (
Table.perf.test.tsx:771). - Changeset: a plugin with its own render-time column cache, such as the tree plugin, still re-renders every row: verified (
useTableTreeData.tsx:642). - Description: both render-time ref writes are replaced by one identity helper that returns one array per ordered list: verified.
- Description: one red-first test per cache fails on main: false; the plugin cache has two (
:691,:771). - Description: before the change both caches were written during render, so a discarded render left its own array: verified in the base sources.
- Description: no state or ref is written during render, and validation runs in an Effect once per merged array: verified (
useBaseTablePlugins.ts:194-206). - Description: output was never wrong, and the tree plugin still re-renders after a discarded render: verified.
- Description: base plugins compare by content, so an inline base array keeps its array: verified (
:771). - Description: the Table record names the helper under Owns, and the verification row adds the helper and its test: verified (
Table.spec.md:129-135,:468). - Description: the Owns item's cited invariant comes from an earlier pull request this one merges after: verified that it is absent on
main(blocking above). - Description: the new Effect names what it syncs, acquires nothing and can replay, and no rendered value comes from a ref: verified.
- Description: a spec owner decided which Table spots follow the rule and records it on this pull request: partly verified; an engineering owner approved the spec at this head, but the split of spots is not stated on the pull request.
- Description: column settings' active keys were fixed before this pull request: verified (state in the base
useTableColumnSettings.tsx). - Description: the ten tests in the Testing table exist as described: verified (
Table.perf.test.tsx:628-815,internByIdentity.test.ts). - Description: the helper stores a copy and the copy guards cover it: verified (
internByIdentity.ts:39,Table.perf.test.tsx:801,internByIdentity.test.ts). - Description: against main's sources two red-first tests fail and 17 pass: false; 24 tests, three red-first, 21 passing.
- Description: with one file restored, only that cache's tests fail: consistent with the test logic (one column test, two plugin tests).
- Description: the Table folder's test run, typecheck, lint, format and knowledge check pass: not confirmed here; CI skipped this draft.
- Description: the earlier state-based version looped, and render cost is within noise of main: not checked.
- Description: other open pull requests on these files merge or conflict as stated: not checked.
- Description:
mainwas merged in with no conflicts and the change moved to identity keys afterwards: verified from the commits. - Description: writing rubric results for the record change: verified (present tense, no pull request reference in the row).
Files:
.changeset/table-caches-at-commit.md: clear.docs/specs/components/Table/Table.spec.md: INV17 citation (blocking above); verification row clear.packages/core/src/Table/BaseTable.tsx: clear.packages/core/src/Table/Table.perf.test.tsx: clear; the description's count of its tests is stale.packages/core/src/Table/internByIdentity.test.ts: clear.packages/core/src/Table/internByIdentity.ts: clear.packages/core/src/Table/useBaseTablePlugins.ts: clear; non-blocking note on the shared result.
| - `Table/internByIdentity.ts`: keeps resolved columns and merged plugins stable | ||
| across renders by input identity, with no state or ref; each entry is written | ||
| once, on first sight of an identity path, and never changed | ||
| (`architecture:react-component-runtime` INV17's lazy one-time |
There was a problem hiding this comment.
Blocking: architecture:react-component-runtime on main has no INV17 (its invariants run INV16, then INV18); INV17 is added by #10161, which is still open. Merged first, this current record cites a claim that does not exist, and the knowledge check does not catch prose citations. Land #10161 and update this branch, or drop the parenthetical citation.
There was a problem hiding this comment.
This merges after #10161, which adds INV17, and main is merged in then. The description says so.
| * @returns Stable array of plugins suitable for BaseTable. Every caller with | ||
| * the same plugins gets this one array, so treat it as read-only. |
There was a problem hiding this comment.
Non-blocking: this hook is exported, and its result is now one module-cached array shared by every caller with the same plugins, while the return type stays mutable TablePlugin<T>[]. A caller that pushes to it changes other callers' arrays and every later render with those plugins; this sentence is the only guard.
There was a problem hiding this comment.
A ReadonlyArray return type would change the exported hook's type. BaseTable's plugins prop takes TablePlugin<T>[], so callers that pass the result on would stop compiling. So @returns asks callers to treat it as read-only, and a readonly type is left for an owner's API decision.
|
Thanks. 1: this merges after #10161, which adds INV17, and main gets merged in then. 2: the description's red-first counts are corrected: three tests fail on main's sources, and the other 21 pass. |
|
/approve-spec 51bbf3c |
Render-time ref writes fixed, each an identity cache:
resolvedColumnsRefincore/Table/BaseTable.tsxandprevRefincore/Table/useBaseTablePlugins.ts. Red-first: yes; one column test and two plugin tests fail on main.#10161 added
architecture:react-component-runtimeINV17 on main; the Table record's Owns item cites it.User impact
People with tables in an app that uses transitions. BaseTable reuses its last column array while the columns are shallowly equal.
useBaseTablePluginsreuses its last plugin array while every plugin is the same instance. Both caches were written while rendering.A render React discards, as a suspended transition does, left its own columns or plugins there. The next committed render then missed the cache, and every memoized row rendered again.
Problem and solution fit
Each cache now comes from one internal helper,
internByIdentity, which returns one array per list of objects, compared by identity and in order. No state or ref is written during render, so a render React discards cannot replace the committed array. Plugin validation stays an Effect, once per merged array.Output was never wrong; a miss only costs identity. The tree plugin keeps its own column cache during render, so after a discarded render its rows still re-render.
Base plugins compare by content, so an inline base array of the same plugins keeps its array (FR12). The Table record names the helper under Owns, since two modules use it.
Expected behavior and authority
architecture:react-component-runtimeINV3 and INV4: each new effect names what it syncs, acquires nothing and can replay; no rendered value comes from a ref mutation./approve-specon this pull request approves its record change. The three map to lines as follows:plugins/columnSettings/useTableColumnSettings.tsxBaseTable.tsx,resolvedColumnsRefuseBaseTablePlugins.ts,prevRefSmallest restoration
One internal helper for both caches, and one validation Effect. No API changes.
Evidence
Scope
Testing
At this PR's head, 51bbf3c, the Table folder runs 42 files and 790 tests. All runs below are at 1fe0959, before main's merge.
Each test commits a table, then renders it again in a transition while a sibling suspends for good, so React never commits that render. A cell renderer records every row it renders.
resolvedColumnsRefprevReftransformColumnsreturns a new column each call renders, and re-renders with its parent. Its key changes on every render, as its array did on main.prevRefresolvedColumnsRefcolumnsprop re-renders when its data array is new.prevRefprevRefinternByIdentityinternByIdentity.test.ts: a new array of the same objects gets the first array. A new object with the same contents, another order or another count gets another. An edit to the first array reaches no other caller.resolvedColumnsRefresolvedColumnsRefpnpm exec vitest run packages/core/src/Table --project ui: 42 files, 789 tests pass.Table.perf.test.tsxrun whole against main's sources: the three red-first tests fail, and the other 21 pass. The StrictMode guard fails at d057c47, which validated during render.BaseTable.tsxrestored, only the column test fails. With onlyuseBaseTablePlugins.tsrestored, only the two plugin tests fail.bank/table-caches-state-loops(53527af945). Generated columns and inline plugins are new on every pass, so its adjustment never settled. All three tests marked against it fail there with "Too many re-renders". The helper's result depends only on the items, so it cannot loop.@astryxdesign/coretypecheck: 0 errors. ESLint and Prettier on the changed files: clean.node scripts/check-knowledge.mjs: aligned.#8861merges cleanly with this change.#4690and feat(Table): Promise-based infinite loading with useTableInfiniteScroll #10570, both open, already conflict with main, in regions this change does not touch.Writing rubric results
component:Table's ownership boundary gains one present-tense item under Owns: whatinternByIdentity.tsdoes, who imports it, and why no existing helper serves. The verification map row for FR12 and PR1 adds the helper and its test as evidence.