Skip to content

fix(approvals): stop a non-object context rendering one cell per character - #63

Merged
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/approve-page-string-context
Jul 30, 2026
Merged

fix(approvals): stop a non-object context rendering one cell per character#63
ralyodio merged 1 commit into
moshcoder:mainfrom
clawedassistant26:fix/approve-page-string-context

Conversation

@clawedassistant26

Copy link
Copy Markdown
Contributor

The bug

GET /approve/:id builds the context grid with:

const ctx = a.context ? JSON.parse(a.context) : {};
const cells = Object.entries(ctx).map(...)

POST /api/approvals stores the context as JSON.stringify(req.body.context) verbatim, so what comes back out is whatever the caller sent. It is not necessarily a label/value map, and Object.entries() on a non-object is meaningless.

Reproduced against the real router on main (dfcb8c1), a throwaway libsql db, no fault injection:

context posted cells rendered
{"env":"prod","sha":"a1b2c3"} env=prod, sha=a1b2c3 (correct)
"deploy failed on prod" 21 cells: 0=d, 1=e, 2=p, 3=l, 4=o, 5=y, 6= , … one per character
42 0 cells — the value vanishes
true 0 cells — the value vanishes
["a","b"] 0=a, 1=b

A one-line message plus a plain-string context is the obvious thing to reach for from an API client, and apps/pwa/README.md documents POST /api/approvals as the ingest endpoint. The operator gets a page full of single-letter tiles instead of the note that was sent.

The fix

apps/pwa/src/routes/approvals.mjs only, two lines of logic:

const raw = a.context ? JSON.parse(a.context) : {};
const ctx = raw !== null && typeof raw === "object" ? raw : { context: raw };

Only a real object is a map; anything else becomes a single context cell.

Deliberately preserved:

  • Objects render exactly as before, key by key.
  • Arrays are objects, so their index-labelled cells are unchanged — no behaviour change that isn't motivated by the bug above.
  • No context still renders no grid (the ${cells ? ... : ""} guard already handles it).
  • Values still go through esc().

The raw !== null guard is also why the route can't be made to throw here: Object.entries(null) is a TypeError, and every route in this app is an async handler, which Express 4 does not route to the error handler.

Tests

New apps/pwa/test/approvals-context.test.mjs — 5 tests, following the existing approvals-notify.test.mjs pattern (real approvalsRouter, throwaway libsql file db, skip-guard so the root npm test stays green without npm install in apps/pwa).

Stash-verified with git stash push -- apps/pwa/src/:

  • without the fix: 3 pass / 2 fail (the string test and the number/boolean test)
  • with the fix: 5 / 5

Full root suite npm test: 202 tests / 202 pass / 0 fail (baseline on main was 197).

…acter

The approve page builds its context grid with Object.entries(JSON.parse(a.context)).
POST /api/approvals stores context as JSON.stringify(req.body.context) verbatim,
so it is whatever the caller sent, not necessarily a label/value map.

A string context indexes per character: context:"deploy failed on prod"
rendered 21 grid cells labelled 0 to 20, one letter each. A number or boolean
context yields no entries at all, so the value silently vanished from the page.

Only a real object is a map. Anything else now renders as a single "context"
cell. Objects and arrays are untouched.

Regression test: apps/pwa/test/approvals-context.test.mjs boots the real router
against a throwaway libsql database and asserts the grid for object, string,
number, boolean, array and absent contexts. Two of the five fail without the fix.
@ralyodio
ralyodio merged commit 5c4e753 into moshcoder:main Jul 30, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants