Skip to content

docs: fix the temp id in the canonical optimistic() reducer snippets #1203

Description

@vivek7405

Problem

optimistic()'s reducer runs on EVERY .value read, not once per .add(). packages/core/src/optimistic.js L8-20:

get value() {
  let current = this.options.source();
  // ...
  for (const update of this.updates) {
    current = this.options.update(current, update.payload);   // re-runs each read
  }
  return current;
}

add(payload, promise) (L22-24) stores only { id, payload }. The optimistic ROW is rebuilt from the payload every time .value is read.

So a reducer that MINTS a value is minting it per render. The canonical snippet in AGENTS.md L384 and .agents/skills/webjs/references/optimistic-ui.md L37 does exactly that:

update: (state, title: string) => [
  ...state,
  { id: crypto.randomUUID(), title, completed: false, createdAt: new Date(), pending: true },
],

Every read of .value hands the pending row a NEW id (and a new createdAt). Nothing breaks visibly in the snippet as written, because its render() uses an unkeyed .map(). It breaks the moment an author does the thing the rest of the docs recommend for a list: key it with repeat(todos, t => t.id, ...) from @webjsdev/core/directives. Every render then sees a brand-new key for the pending row, so repeat tears the <li> down and rebuilds it on each update, losing focus, any in-progress CSS transition, and DOM state. The same applies to any consumer that treats id as stable (a @click handler capturing it, an aria-activedescendant, a test selector).

This matters more than a normal doc nit because these two files are the primary teaching surface: .agents/skills/webjs/references/optimistic-ui.md is copied verbatim into EVERY scaffolded app, and AGENTS.md is the cross-agent source. An agent building a todo list will copy this reducer.

A second, related shape exists in two more copies: packages/core/src/optimistic.js L78 (the JSDoc example) and website/app/docs/client-router/page.ts L71 both use a hardcoded { id: 'tmp', ... }. That is stable per read, so it does not have the repeat bug, but it collides when two adds are in flight (both pending rows get id: 'tmp'), which is precisely the case optimistic() advertises support for ("multiple .add() calls stack independently").

The scaffold's real component already does it correctly and is the model to copy: packages/cli/templates/gallery/modules/todo/components/todo-app.ts L49-53 computes const tempId = crypto.randomUUID(); in the handler and passes it as part of the payload (this.store.add({ kind: 'add', tempId, title }, promise)), so the reducer is pure and the id is stable for the life of the pending row.

Found during the review of #1200, which touched these snippets but deliberately did not restructure them (that PR was about unknown / any, and the change here alters the payload shape across three surfaces plus a core docstring).

Design / approach

Make the reducer PURE in all four canonical copies, and mint the temp id in the handler, matching what the gallery component already ships.

The shape to converge on, taken from todo-app.ts:

private optimisticTodos = optimistic(this, {
  source: () => this.todos,
  // Pure: derives the row from the payload, mints nothing. The reducer re-runs
  // on every `.value` read, so a `crypto.randomUUID()` here would hand the
  // pending row a fresh id per render and break any keyed list.
  update: (state, add: { tempId: string; title: string }) => [
    ...state,
    { id: add.tempId, title: add.title, completed: false, createdAt: new Date(), pending: true },
  ],
});

async handleSubmit(e: SubmitEvent) {
  // ...
  const tempId = crypto.randomUUID();
  const promise = createTodo({ title });
  this.optimisticTodos.add({ tempId, title }, promise);
  // ...
}

Note createdAt: new Date() has the same impurity in a weaker form (a new Date per read). It is not a correctness bug the way id is, since nothing keys on it, but the same fix covers it: move it into the payload, or leave it and say why in a comment. Decide once and apply consistently.

The payload stops being a bare title string, which is why this is not a one-line fix: the surrounding prose in optimistic-ui.md explains .add(payload, promise?) using the bare-string payload, and the update reducer's second parameter is typed title: string in both markdown copies. Those sentences need to move with the code.

Alternative considered and rejected: leave the snippets alone and add a warning that the reducer must be pure. That keeps a broken example on the page, and #1200 established the principle that an agent copies the example far more reliably than it follows the prose.

For the two { id: 'tmp' } copies, the same fix applies. If a shorter example is wanted in packages/core/src/optimistic.js's docstring, id: tempId with the payload destructured is barely longer than id: 'tmp'.

Implementation notes (for the implementing agent)

Where to edit (all four copies, they must agree):

  • AGENTS.md L376-392, the "Mutations: default to optimistic UI" section's declarative example. The reducer is L382-385, the handler L387-397.
  • .agents/skills/webjs/references/optimistic-ui.md L20-66, the "Declarative API (preferred)" example. Reducer L29-38, handler L41-57. This file is the canonical long-form copy and carries the explanatory prose that must move with the payload change: the .add(payload, promise) paragraph at L68 and the "Multiple .add() calls stack independently" bullet at L70.
  • packages/core/src/optimistic.js L72-93, the JSDoc example on the optimistic() export (id: 'tmp'). Its "Behaviour" list at L95-99 already states that .value folds every update through update, which is the fact the example contradicts.
  • website/app/docs/client-router/page.ts L62-85, the declarative snippet under the "Optimistic mutations" heading (id: 'tmp').

Reference implementation (do not change, copy from): packages/cli/templates/gallery/modules/todo/components/todo-app.ts L30-53. Its reducer is already a pure discriminated-union dispatch (op.kind === 'add' | 'toggle' | 'delete') with tempId in the payload.

Landmines / gotchas:

  • AGENTS.md and .agents/skills/webjs/references/optimistic-ui.md must stay byte-consistent in substance. They drifted before: docs: teach app agents to derive types, never unknown or any #1200's round-three review found the reconcile line in AGENTS.md had [...this.todos.filter(t => t.pending), result.data, ...this.todos.filter(t => !t.pending)] while the skill copy had [...this.todos, result.data], and the AGENTS.md one was wrong (the optimistic row is never written to this.todos, so the pending filter is always empty and the expression PREPENDED a row the reducer appended). Diff the two copies after editing.
  • The skill is copied, not duplicated. packages/cli/lib/create.js L641-653 copies .agents/skills/webjs/ from the repo root into every generated app (or from a prepack bundle produced by scripts/sync-scaffold-skill.mjs). Edit the repo-root canonical ONCE; never hand-edit a bundled copy under packages/cli/templates/.
  • Bun prose rewriting. That same copy runs bunifyProse over the skill markdown for --runtime bun. Keep any runnable command in the canonical npm form.
  • Invariant 9 (no backtick inside an html template body) applies to website/app/docs/client-router/page.ts, whose content lives inside an html template. Its <pre> blocks escape < as &lt; and backtick-escape the nested html\`` templates; match that exactly or the page 500s. #1200 hit this: a backtick inside a code comment in website/app/docs/backend-only/page.tsbroke the parse with aTS1005: ';' expected`.
  • Invariant 11 (prose punctuation) applies to every added markdown and doc-page line: no em-dashes, no space-surrounded hyphen or semicolon as a pause, WebJs capitalized in prose but lowercase webjs as a code token. Enforced by .claude/hooks/block-prose-punctuation.sh on NEW content.
  • Pre-existing defect in the same snippet, fix it while you are there: website/app/docs/client-router/page.ts L81-83 renders <ul>${...map(...)} with no closing &lt;/ul&gt;. The <pre> block's markup is unbalanced as printed.

Invariants to respect: AGENTS.md invariants 9 and 11 (above). The conventions-vs-checks split: this is example correctness in prose, so do NOT add a webjs check rule for it.

Tests + docs surfaces:

  • Doc surfaces per webjs-doc-sync: AGENTS.md, .agents/skills/webjs/references/optimistic-ui.md, the docs site (website/app/docs/client-router), and packages/core/src/optimistic.js's docstring. The scaffold gallery is the reference, not a target.
  • Tests: the markdown copies have no test coverage, so the gates are that the website app still passes webjs check and that /docs/client-router renders (boot it through createRequestHandler in prod mode and assert status < 400 plus non-empty body, the harness pattern in the start-work skill). packages/core/test/** covers optimistic() itself and is unaffected by a docstring edit; run node --test packages/core/test/*optimistic* to confirm.
  • Consider adding the missing unit test rather than only fixing prose: there is currently nothing asserting that .value is stable across repeated reads for the same queued update. A test that reads .value twice and asserts the pending row's id is identical would have caught this class of bug, and would fail against a minting reducer. Put it in packages/core/test/ alongside the existing optimistic tests.

Acceptance criteria

  • No canonical optimistic() example mints a value inside its update reducer; the temp id is computed in the handler and passed in the payload
  • All four copies (AGENTS.md, .agents/skills/webjs/references/optimistic-ui.md, packages/core/src/optimistic.js JSDoc, website/app/docs/client-router/page.ts) show the same shape, and the two { id: 'tmp' } copies no longer collide across concurrent adds
  • The prose that explains .add(payload, promise?) and the reducer's parameter type moves with the payload change, so no sentence still describes the bare-string payload
  • AGENTS.md and .agents/skills/webjs/references/optimistic-ui.md agree in substance (diff them)
  • website/app/docs/client-router/page.ts renders (200, non-empty) and the website passes webjs check; its <pre> markup is balanced
  • A packages/core/test/ unit test asserts .value is stable across repeated reads for one queued update
  • A counterfactual proves that test fires: restore a minting reducer in the test fixture and watch it go red

Metadata

Metadata

Assignees

Labels

documentationImprovements or additions to documentation

Type

No type

Projects

Status
Todo

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions