[helgehoi]

Ticket

VI-5068 — [tamstool] Upgrade TAMS Store Browser to TAMS 8.2

Follow-up to #26. That PR removed a query parameter an 8.2 store rejects; this one fixes the reason nobody noticed for four months, plus the four siblings of the same problem found in a sweep.

Findings

A sweep of every query the app builds and every catch it holds turned up five places where a failure is reported as a success — and one encoding hole.

1. Both flow delete modals announce success before the delete happens

FlowDeleteModal.jsx, FlowDeleteTimeRangeModal.jsx:

const promises = selectedItems.map((item) => del({ flowId: item.id }));
addAlertItems(… "Flow X is being deleted" …);   // success alert, before the await
await Promise.all(promises);                     // no catch
setIsDeleting(false); setModalVisible(false);    // unreachable on failure

del is SWR's trigger with the default throwOnError, so a failed DELETE rejects. By then the user has been told it worked; the rejection then skips the rest of the handler, leaving the button spinning and the modal open with nothing to explain it.

2. The webhook delete modal swallows its error under a false comment

} catch {
  // Alert emitted by useApi
} finally {
  handleDismiss();
}

useApi emits no alerts — it holds no reference to the alerts store and simply throws. A failed webhook delete was therefore completely silent, and finally closed the modal as though it had succeeded.

3. A failed lookup answered with a plausible wrong id

Embed/index.jsx, resolveVideoSourceId:

} catch { return source.id; }

source.id is a legitimate answer for a source with no collection, so the caller cannot tell a resolved source from a failed one. It then reads another flow's fps, duration and status and renders them as this source's own. A confident wrong answer, on any flaky network.

4. Two catches that logged nothing

catch { /* skip */ } in both embed loops. This is how &_t=… — rejected with a 400 on every 5s poll since April — stayed invisible until it was read off a console by hand.

5. An unencoded, user-typed query value

api.del(`${path}/${arg.flowId}/segments?timerange=${arg.timerange}`)

A timerange is typed into a text field and carries [, : and ). Correct is %5B0%3A0_10%3A0). Every other query in the app goes through buildQuery/URLSearchParams; this one did not — on the destructive endpoint, against a store strict enough to 400 on an unknown parameter.

The fixes

1–2. All three delete modals settle first and report per item, via one useOutcomeAlerts helper in the alerts store rather than three copies of the alert-building. It returns whether everything succeeded; the timerange modal uses that to keep the typed timerange for a retry.
3. Returns null, and the caller leaves the source unresolved for the next pass.
4. Both catches console.warn with the source or flow id and the error.
5. Goes through buildQuery.

What the sweep did not find

Every query parameter the app sends is declared in the 8.2 spec. I extracted them from bbc/tams@8.2 and diffed:

Endpoint	App sends	Spec
/sources	limit, collected_by_ids, sort_by, reverse_order	✅
/flows	limit, source_id, collected_by_ids, sort_by, reverse_order, status	✅
/flows/{id}	include_timerange	✅
/flows/{id}/segments	accept_get_urls, reverse_order, include_object_timerange, timerange, limit, storage_backend_tag{,_exists}.{name}	✅
DELETE /flows/{id}/segments	timerange	✅

Both sort_by enums match the spec's two different lists, and status is only ever sent to /flows, which is the only endpoint that declares it. _t was the sole offender, and it escaped #20's spec review because it was hand-written into a template literal while everything reviewed went through buildQuery.

Testing

Against a stub TAMS 8.2 store driven through the real app in headless Chrome:

• No regression: a multi-source recording still resolves its video child and reaches Open and LIVE 5s after the ingest starts, without a reload
• The new failure path, with the collection request forced to 500:

  console warnings seen: [embed] could not resolve the video source for 1111… Error: 500 Internal Server Error
  before: open:false    t+30.0s: open:false
  

  It names the failure and shows nothing, where the old code would have resolved to the parent and shown another flow's data as this source's.
• npm run build passes (Node 22, same as CI)
• npm run lint still cannot run in this repo (ESLint 9 needs a flat config, deliberately left out of VI-5068 Support TAMS 8.2 in the TAMS Store Browser #20) — the build is the gate

The delete-modal paths are argued from code rather than exercised: reproducing them needs a store that rejects a DELETE, which the stub does not model. The reasoning is in the finding above, and the shape now matches the tag and webhook-register modals, which already reported errors correctly.

One deliberate trade-off: against a store that fails every source, the embed will now warn once per source per 5s poll. Noisy — but that noise is the point, and it is what four months of silence cost.

🤖 Generated with Claude Code