0xMiden / 0xMiden/web-sdk

useNotesIfChanged/useNoteStream don't reflect a note's status change (pending → consumed) when the note set is otherwise unchanged

Aperta
#304 2 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
TypeScript
Stelle
1
Fork
21
Merge medio
12h 14m
PR unite (30g)
41

Descrizione

### Packages versions

@miden-sdk/react 0.15.9 (repo: 0xMiden/web-sdk, package: packages/react-sdk), reproduced on current main.

### Bug description

## Bug: `useNotes()` / `useNoteStream()` never reflect a note's status change (pending → committed → consumed) when the note set otherwise stays the same

### Package
`@miden-sdk/react` (repo: `0xMiden/web-sdk`, `packages/react-sdk`)

### Summary
`MidenStore.setNotesIfChanged` and `setConsumableNotesIfChanged` are meant to be a
"smart refetch" optimization: only push a new `notes` / `consumableNotes` array into
the Zustand store (and thus trigger a re-render) when something actually changed,
to avoid needless re-renders after every sync.

The problem is that "changed" is currently defined purely as **the set of note IDs
changed** (`packages/react-sdk/src/store/MidenStore.ts`):

```ts
const prevIds = new Set();
for (const n of state.notes) { ... prevIds.add(id) ... }
const newIds = new Set();
for (const n of notes) { ... newIds.add(id) ... }
if (prevIds.size === newIds.size && [...prevIds].every((id) => newIds.has(id))) {
return {}; // <-- bails out, keeps the OLD note objects in the store
}
```

A Miden note keeps the **same ID for its entire lifecycle** — only its status
(`isConsumed()`, `isProcessing()`, `isInclusionPending()`, ...) changes as it moves
from pending → committed → consumed. Because the comparison only looks at ID
membership, a note that transitions status *in place* (without any other note being
added or removed in that same fetch) is treated as "unchanged," and the **stale**
`InputNoteRecord` object from the previous fetch is kept in the store forever.

This directly breaks the intended usage shown in the hook's own JSDoc example:

```tsx
{notes.map(n => (


Note: {n.id().toString()} - {n.isConsumed() ? 'Consumed' : 'Pending'}

))}
```

Once a note is consumed, this will keep rendering `"Pending"` indefinitely (until
some unrelated note is added/removed and forces a full refresh), even though
`useNotes()`/`useNoteStream()` refetch from the client after every sync
(`lastSyncTime` change) specifically so status changes propagate.

The same issue affects `setConsumableNotesIfChanged`, used by `useNotes()` for
`consumableNotes`.

### Repro (unit test against current `main`)
```ts
it("should update notes when a note's status changes even though its ID is unchanged", () => {
const pending = createMockInputNoteRecord("0xnote1", false);
useMidenStore.getState().setNotes([pending] as any);

const consumed = createMockInputNoteRecord("0xnote1", true);
useMidenStore.getState().setNotesIfChanged([consumed] as any);

const stored = useMidenStore.getState().notes[0] as any;
expect(stored.isConsumed()).toBe(true); // fails on current main: stays `false`
});
```
This fails on current `main` — `setNotesIfChanged` returns `{}` because the ID set
(`{"0xnote1"}`) didn't change, so the store keeps the old (pending) object.

Note that this is effectively *asserted as the intended behavior* by the existing
test `"should skip update when note IDs are the same"` in
`MidenStore.test.ts`, which only varies note identity, not status — so the gap
wasn't caught.

### Suggested fix
Compare a fingerprint of `id + relevant status flags` instead of ID alone, so a
status flip is detected as a real change while two fetches that return the *same*
notes in the *same* status still short-circuit (preserving the original
re-render-avoidance goal). I have a patch + passing regression test ready
(all 846 existing unit tests in `react-sdk` still pass, plus typecheck is clean).

### Environment
- Found via manual code review, `web-sdk` @ `main` (`@miden-sdk/react` `0.15.9`).
- Reproduced with `vitest` in `packages/react-sdk`.

Happy to open a PR for this once assigned, per CONTRIBUTING.md.

### How can this be reproduced?

1. Clone 0xMiden/web-sdk, go to packages/react-sdk.
2. Add this test to src/__tests__/store/MidenStore.test.ts:
ts
it("should update notes when a note's status changes even though its ID is unchanged", () => {
const pending = createMockInputNoteRecord("0xnote1", false);
useMidenStore.getState().setNotes([pending] as any);

const consumed = createMockInputNoteRecord("0xnote1", true);
useMidenStore.getState().setNotesIfChanged([consumed] as any);

const stored = useMidenStore.getState().notes[0] as any;
expect(stored.isConsumed()).toBe(true);
});
3. Run npx vitest run src/__tests__/store/MidenStore.test.ts.
4. In a real app: call useNotes() (default status: "all"), wait for a note to become consumed on-chain, trigger a sync (so refetch() runs again with the same overall note ID set). notes in the store still shows the note as not consumed.

### Relevant log output

```shell
FAIL src/__tests__/store/MidenStore.test.ts > setNotesIfChanged - status change regression > should update notes when a note's status changes even though its ID is unchanged
AssertionError: expected false to be true // Object.is equality

- Expected
+ Received

- true
+ false

❯ src/__tests__/store/MidenStore.test.ts:516:33
514|
515| const stored = useMidenStore.getState().notes[0] as any;
516| expect(stored.isConsumed()).toBe(true);
| ^
517| });
518| });

Test Files 1 failed (1)
Tests 1 failed | 38 passed (39)
After applying the fix (fingerprint-based comparison), the same test and the full suite pass: Test Files 62 passed (62) / Tests 846 passed (846)
```

Guida per i contributori

Apri la guida per i contributori

Direzione di ricerca

The bug is in packages/react-sdk/src/store/MidenStore.ts, specifically the setNotesIfChanged and setConsumableNotesIfChanged functions. Start by examining the current ID-based comparison logic. The fix involves changing the comparison to include note status flags. Run the existing test suite with vitest, and add the provided regression test to MidenStore.test.ts to verify the fix. Done looks like the test passes and the note status updates correctly in the store.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
react, typescript
Ambito
frontend
Tipo di issue
Bug
Difficoltà
3/5
Tempo stimato
1-2 giorni
Stato di attività
Attiva
Chiarezza
Specificata chiaramente
Idoneità per principianti
65/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.