Fix For component missing re-renders on in-place array mutations#41
Fix For component missing re-renders on in-place array mutations#41scottmessinger wants to merge 2 commits intomainfrom
Conversation
For was not wrapped in tracked(), so it iterated arrays (.length, .map()) without an active signal subscriber. In-place mutations like push/splice bumped ownKeys/length signals but nobody was listening — no re-render. Existing dev tests masked this because the shared module-level store's $NODE persisted across test cycles, causing subsequent tests to take a code path that includes trackArrayVersion. A fresh createStore always failed. Wrap For in tracked() so it subscribes to array structural signals. Add regression test using a fresh store to prevent future masking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes missing For re-renders when the backing array is mutated in-place (e.g., push, splice) by ensuring For renders under an active reactive subscriber, and adds a regression test to prevent the issue from being masked by cross-test store/node reuse.
Changes:
- Wrap
Forintracked()so it subscribes during iteration and re-renders on in-place array mutations. - Update
For’s doc comment to reflect the new reactive behavior. - Add a regression test that reproduces the “fresh store + push” scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react/src/use-store.ts | Wrapes For with tracked() to ensure array iteration subscribes and in-place mutations trigger updates. |
| packages/js-krauset/src/main.test.tsx | Adds a regression test covering push-driven updates on a brand-new createStore instance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const For: <T>(props: ForProps<T>) => React.JSX.Element | null = tracked( | ||
| <T>(props: ForProps<T>): React.JSX.Element | null => { | ||
| const { each, children, fallback } = props; | ||
|
|
||
| if (!each || each.length === 0) { | ||
| return fallback ? React.createElement(React.Fragment, null, fallback) : null; | ||
| } | ||
| if (!each || each.length === 0) { | ||
| return fallback ? React.createElement(React.Fragment, null, fallback) : null; | ||
| } | ||
|
|
||
| return React.createElement( | ||
| React.Fragment, | ||
| null, | ||
| each.map((item, index) => { | ||
| const child = children(item, index); | ||
| return React.createElement( | ||
| React.Fragment, | ||
| null, | ||
| each.map((item, index) => { | ||
| const child = children(item, index); | ||
|
|
||
| // Assign stable key from item.id if available | ||
| if (React.isValidElement(child)) { | ||
| const key = item && typeof item === "object" && "id" in item ? (item as any).id : index; | ||
| // Assign stable key from item.id if available | ||
| if (React.isValidElement(child)) { | ||
| const key = item && typeof item === "object" && "id" in item ? (item as any).id : index; | ||
|
|
||
| return React.cloneElement(child, { key } as any); | ||
| } | ||
| return React.cloneElement(child, { key } as any); | ||
| } | ||
|
|
||
| return child; | ||
| }), | ||
| ); | ||
| } | ||
| return child; | ||
| }), | ||
| ); | ||
| }, | ||
| ) as any; |
There was a problem hiding this comment.
For is exported with an as any cast, which disables type-checking of the tracked(...) wrapper against the declared generic call signature. This makes it easy for future refactors to accidentally break the component contract without TypeScript catching it. Consider avoiding any by preserving the inner component type (e.g., define a ForImpl and cast via unknown to typeof ForImpl), or updating tracked()'s typing to better support generic components so this file doesn’t need an escape hatch.
| * per-component signal scoping. | ||
| * item.id (or index as fallback). Wrapped in tracked() so that in-place | ||
| * array mutations (push, splice, etc.) trigger re-renders by subscribing | ||
| * to the array's length and ownKeys signals. |
There was a problem hiding this comment.
The updated docstring says For is wrapped in tracked() to subscribe to array signals, but it no longer mentions the important usage constraint that reactive reads performed inside the children render callback will now be tracked to For’s own effect (potentially causing whole-list re-renders). Consider adding a brief note recommending that row/item components be wrapped in tracked() (or otherwise avoid reactive reads in the callback) to keep updates scoped per row.
| * to the array's length and ownKeys signals. | |
| * to the array's length and ownKeys signals. Note that reactive reads | |
| * performed inside the `children` render callback are tracked to `For`’s | |
| * own effect, which can cause whole-list re-renders; to keep updates | |
| * scoped per row, wrap row/item components in `tracked()` (or avoid | |
| * reactive reads directly in the callback). |
| describe("fresh store push regression", () => { | ||
| it("push on a fresh store triggers re-render without prior assignment", async () => { | ||
| const { createStore } = await import("@supergrain/core"); | ||
| const { tracked, For } = await import("@supergrain/react"); | ||
|
|
||
| const [freshStore] = createStore<{ data: { id: number; label: string }[] }>({ data: [] }); | ||
|
|
||
| const TestApp = tracked(() => { | ||
| return ( | ||
| <For each={freshStore.data}> | ||
| {(item: { id: number; label: string }) => ( | ||
| <tr key={item.id}> | ||
| <td>{item.label}</td> | ||
| </tr> | ||
| )} | ||
| </For> | ||
| ); | ||
| }); | ||
|
|
||
| const result = render( | ||
| <table> | ||
| <tbody> | ||
| <TestApp /> | ||
| </tbody> | ||
| </table>, | ||
| ); | ||
| const tbody = result.container.querySelector("tbody")!; | ||
|
|
||
| await act(async () => { | ||
| freshStore.data.push({ id: 1, label: "one" }, { id: 2, label: "two" }); | ||
| }); | ||
|
|
||
| expect(tbody.querySelectorAll("tr").length).toBe(2); | ||
|
|
||
| cleanup(); | ||
| }); |
There was a problem hiding this comment.
This test calls cleanup() manually at the end, but if an assertion fails earlier the DOM won’t be cleaned up and could leak into later tests. Prefer adding an afterEach(() => cleanup()) (or afterEach(cleanup)) within this describe block and removing the manual call so cleanup runs even on failures.
The existing For tests all used pre-populated stores or did an assignment before push, so $NODE signal metadata was always present when push ran. This masked the bug where For didn't subscribe to array structural changes. Add three tests that cover the gap: - push on empty array with fresh store (exact production benchmark scenario) - splice on array with fresh store and no prior assignment - push after initial empty render without any store.data assignment Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Forwas not wrapped intracked(), so it iterated arrays (.length,.map()) without an active signal subscriberpush/splicebumpedownKeys/lengthsignals but nobody was listening — no re-render$NODEpersisted across test cyclescreateStoreto prevent future maskingTest plan
fresh store push regression) verifies push works on a brand-new store without prior assignment🤖 Generated with Claude Code