Add more reactivity test for alpine, preact, ember, lit, svelte - #6462
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds grouped-cell rendering support across adapters, Ember owner lifecycle cleanup, Lit renderable typing, Svelte option synchronization, Preact exotic-component handling, and broad framework-specific reactivity, rendering, lifecycle, and SSR tests. ChangesAdapter rendering and reactivity
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
riccardoperra
force-pushed
the
test/adapters-reactivity-test
branch
from
July 29, 2026 22:10
7952940 to
619d1af
Compare
|
View your CI Pipeline Execution ↗ for commit 619d1af
☁️ Nx Cloud last updated this comment at |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We changed BoundFlexRenderComponent from Component<Record<string, never>> to Component<Record<string, any>> to fix the TypeScript type errors in the Svelte example apps. Record<string, never> made every prop value an impossible never type, causing TypeScript to reject the Header_Core<...> values passed via <header.FlexRender header={header} />. Our fix correctly represents a pre-bound component that can accept any props.
Tip
✅ We verified this fix by re-running tanstack-svelte-table-example-composable-tables:test:types.
diff --git a/packages/svelte-table/src/createTableHook.svelte.ts b/packages/svelte-table/src/createTableHook.svelte.ts
index afe06809..c25799d2 100644
--- a/packages/svelte-table/src/createTableHook.svelte.ts
+++ b/packages/svelte-table/src/createTableHook.svelte.ts
@@ -38,7 +38,7 @@ import type {
} from '@tanstack/table-core'
export type ComponentType<T extends Record<string, any>> = Component<T>
-export type BoundFlexRenderComponent = Component<Record<string, never>>
+export type BoundFlexRenderComponent = Component<Record<string, any>>
function createLiveView<TCurrent extends object, TExtensions extends object>(
getCurrent: () => TCurrent,
Or Apply changes locally with:
npx nx-cloud apply-locally eiWd-HgmI
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
riccardoperra
marked this pull request as ready for review
July 29, 2026 22:22
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/ember-table/tests/integration/external-state.test.gts (1)
763-766: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture the table outside the tracked getter.
Assigning
capturedTableas a side effect of a tracked getter couples the capture to render timing and makes the latercapturedTable!assertions implicitly dependent on the template having rendered. A field-level capture is clearer and equally valid.♻️ Suggested refactor
class TableComponent extends Component { table = useTable(this, () => ({ data: makeData(...UNSORTED.map((firstName) => ({ firstName }))), columns, features, atoms: { pagination: paginationAtom }, })) + constructor(...args: ConstructorParameters<typeof Component>) { + super(...args) + capturedTable = this.table + } + get pageSize() { - capturedTable = this.table return this.table.store.state.pagination.pageSize }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ember-table/tests/integration/external-state.test.gts` around lines 763 - 766, Move the capturedTable assignment out of the tracked pageSize getter and initialize it at field level when the table instance is created. Keep the getter limited to returning this.table.store.state.pagination.pageSize, while preserving the existing capturedTable assertions.packages/lit-table/src/createTableHook.ts (1)
778-779: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute header/footer through
FlexRendertoo, for symmetry withAppCell.
cellFlexRendernow delegates toFlexRender({ cell })(Line 747), but header/footer still callflexRender(...)directly. Behavior is identical today (FlexRender's header/footer branches do exactly this perpackages/lit-table/src/flexRender.ts:131-144), so this is purely about keeping one rendering entry point asFlexRendergrows special cases.♻️ Suggested refactor
- const headerFlexRender = () => - flexRender(header.column.columnDef.header, header.getContext()) + const headerFlexRender = () => FlexRender({ header })- const footerFlexRender = () => - flexRender(header.column.columnDef.footer, header.getContext()) + const footerFlexRender = () => FlexRender({ footer: header })Also applies to: 808-809
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lit-table/src/createTableHook.ts` around lines 778 - 779, Update the header and footer render helpers in createTableHook to delegate through FlexRender using the appropriate header/footer props, matching cellFlexRender’s FlexRender({ cell }) pattern. Replace the direct flexRender calls while preserving the existing header and footer contexts and output behavior.packages/lit-table/src/subscribe-directive.ts (1)
155-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate render logic with
createFakeHost().requestUpdate.The value-render snippet here is identical to the one in
requestUpdate(Lines 171-173). Extract a small private helper to keep both call sites in sync going forward.♻️ Suggested extraction
+ private renderCurrentValue(): void { + if (this.resolvedTemplate && this.controller) { + this.setValue(this.resolvedTemplate(this.controller.value)) + } + } + /** Restores the controller subscription when the directive is re-attached to the DOM. */ reconnected() { this.controller?.hostUpdate() - if (this.resolvedTemplate && this.controller) { - this.setValue(this.resolvedTemplate(this.controller.value)) - } + this.renderCurrentValue() }requestUpdate: () => { - if (this.resolvedTemplate && this.controller) { - this.setValue(this.resolvedTemplate(this.controller.value)) - } + this.renderCurrentValue() },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lit-table/src/subscribe-directive.ts` around lines 155 - 159, Extract the duplicated resolved-template rendering logic from reconnected() and requestUpdate into a small private helper, preserving the existing guards and calls to setValue with controller.value. Replace both call sites with the helper so future changes remain synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/ember-table/tests/integration/external-state.test.gts`:
- Around line 763-766: Move the capturedTable assignment out of the tracked
pageSize getter and initialize it at field level when the table instance is
created. Keep the getter limited to returning
this.table.store.state.pagination.pageSize, while preserving the existing
capturedTable assertions.
In `@packages/lit-table/src/createTableHook.ts`:
- Around line 778-779: Update the header and footer render helpers in
createTableHook to delegate through FlexRender using the appropriate
header/footer props, matching cellFlexRender’s FlexRender({ cell }) pattern.
Replace the direct flexRender calls while preserving the existing header and
footer contexts and output behavior.
In `@packages/lit-table/src/subscribe-directive.ts`:
- Around line 155-159: Extract the duplicated resolved-template rendering logic
from reconnected() and requestUpdate into a small private helper, preserving the
existing guards and calls to setValue with controller.value. Replace both call
sites with the helper so future changes remain synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df42cc5a-2428-4832-a9b1-e3df0293c972
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
knip.jsonpackages/alpine-table/package.jsonpackages/alpine-table/src/flexRender.tspackages/alpine-table/tests/unit/adapterCoverage.test.tspackages/alpine-table/tsconfig.jsonpackages/ember-table/src/FlexRender.gtspackages/ember-table/src/create-table-hook.tspackages/ember-table/src/use-table.tspackages/ember-table/tests/integration/create-table-hook.test.gtspackages/ember-table/tests/integration/external-state.test.gtspackages/ember-table/tests/integration/flex-render.test.gtspackages/ember-table/tests/integration/reactivity.test.gtspackages/lit-table/package.jsonpackages/lit-table/src/TableController.tspackages/lit-table/src/createTableHook.tspackages/lit-table/src/flexRender.tspackages/lit-table/src/subscribe-directive.tspackages/lit-table/tests/unit/adapterLifecycle.test.tspackages/lit-table/tests/unit/defaultReactivity.test.tspackages/lit-table/tests/unit/flexRender.test.tspackages/lit-table/tests/unit/rendering.test.tspackages/lit-table/tests/unit/selectorGate.test.tspackages/lit-table/tsconfig.jsonpackages/preact-table/package.jsonpackages/preact-table/src/FlexRender.tsxpackages/preact-table/tests/unit/adapterReactivity.test.tsxpackages/preact-table/tests/unit/createTableHook.test.tsxpackages/preact-table/tests/unit/rendering.test.tsxpackages/preact-table/tests/unit/ssr.test.tsxpackages/preact-table/tests/unit/useTable.test.tsxpackages/react-table/tests/adapterReactivity.test.tsxpackages/svelte-table/package.jsonpackages/svelte-table/src/FlexRender.sveltepackages/svelte-table/src/createTable.svelte.tspackages/svelte-table/tests/adapter-lifecycle.test.tspackages/svelte-table/tests/fixtures/CallbackHarness.sveltepackages/svelte-table/tests/fixtures/ContextFailure.sveltepackages/svelte-table/tests/fixtures/FlexRenderHarness.sveltepackages/svelte-table/tests/fixtures/HookCellBadge.sveltepackages/svelte-table/tests/fixtures/HookHarness.sveltepackages/svelte-table/tests/fixtures/HookHeaderBadge.sveltepackages/svelte-table/tests/fixtures/HookTableBadge.sveltepackages/svelte-table/tests/fixtures/PaginationHarness.sveltepackages/svelte-table/tests/fixtures/ReactivityHarness.sveltepackages/svelte-table/tests/fixtures/RenderBadge.sveltepackages/svelte-table/tests/fixtures/SelectorHarness.sveltepackages/svelte-table/tests/fixtures/SsrHarness.sveltepackages/svelte-table/tests/fixtures/hook-fixture.tspackages/svelte-table/tests/rendering.test.tspackages/svelte-table/tests/ssr.test.tspackages/svelte-table/tests/test-setup.tspackages/svelte-table/tsconfig.jsonpackages/svelte-table/vite.config.ts
KevinVandy
approved these changes
Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added more test for alpine, preact, ember, lit, svelte. Also this fixes the missing
cleanupin ember adapter integrationEmber fixes
Improved Ember adapter lifecycle handling by allowing
useTableandcreateAppTableto receive an Ember owner. Table reactivity is now automatically cleaned up when that owner is destroyed, preventing external atoms and destroyed tables from continuing to update each other.The existing owner-less API remains supported for standalone usage. Tests cover cleanup in both update directions, external atom precedence, state synchronization, and rendering behavior.
Summary by CodeRabbit
New Features
Bug Fixes