Skip to content

feat(desktop): TAM-6806: Relegate red toast error messages - #11020

Merged
NavarroEmilioLuis merged 34 commits into
mainfrom
feature/tam-6806-relegate-red-toast-error-messages
Sep 16, 2026
Merged

NavarroEmilioLuis merged 34 commits into
mainfrom
feature/tam-6806-relegate-red-toast-error-messages

Conversation

@NavarroEmilioLuis

@NavarroEmilioLuis NavarroEmilioLuis commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Changes

Red toast error messages are noisy but also sometimes those errors are not actionable by users and they need to contact support. This works creates a table where these are relegated so that a user can send an email directly to support with all the info attached.

Auto-Deploy

  • Deploy
Options
  • Artillery load test
  • Seed from closest snapshot
  • Generate fake data
  • More data (20Gi)
  • No facility servers (central-only)
  • No sync (facility tasks scaled to zero)
  • Skip mobile build
  • Always build mobile
  • Stay up for 8 hours
  • Stay up for 24 hours
  • Stay up (no TTL)
  • Build images only (don't deploy)
  • Build all images (amd64 + Windows; default is arm64 only)
  • Pause this deploy

Tests

  • Run E2E tests
  • Run DAST scan

Review Hero

  • Run Review Hero
  • Auto-fix review suggestions Wait for Review Hero to finish, resolve any comments you disagree with or want to fix manually, then check this to auto-fix the rest.
  • Auto-fix CI failures Check this to auto-fix lint errors, test failures, and other CI issues.
  • Auto-merge upstream Check this to merge the base branch into this PR, with AI conflict resolution if needed.
  • Save suppressions Check this to capture 👎 reactions on Review Hero comments as suppression rules in .github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.

Comment thread packages/web/app/api/relegateSystemError.js Outdated
Comment thread packages/web/app/views/patients/PatientListingView.jsx Outdated
Comment thread packages/web/app/store/systemErrors.js
Comment thread packages/web/app/components/Sidebar/Sidebar.jsx Outdated
Comment thread packages/web/app/api/relegateSystemError.js Outdated
Comment thread packages/central-server/app/systemErrorReport.js
Comment thread packages/web/app/components/Table/useClientSideTableData.js
Comment thread packages/central-server/app/systemErrorReport.js
Comment thread packages/web/app/api/relegateSystemError.js Outdated
@review-hero

review-hero Bot commented Sep 8, 2026

Copy link
Copy Markdown

🦸 Review Hero Summary
18 agents reviewed this PR | 2 critical | 7 suggestions | 1 nitpick | Filtering: consensus 3 voters, 10 below threshold, 2 suppressed

Below consensus threshold (10 unique issues not confirmed by majority)
Location Agent Severity Comment
packages/central-server/app/systemErrorReport.js:46 Security suggestion userId is read from the request body and embedded in the email as "Reporting user id", but the request is already authenticated — req.user.id is available. As written a user can attribute a rep...
packages/facility-server/__tests__/apiv1/SystemErrorReport.test.js:39 Integration tests nitpick The route resolves recipients via settings[facilityId].get('systemErrorReport'), but the suite only ever sees the schema default ['support@bes.au'], so the settings lookup itself (and the spec ...
packages/facility-server/app/routes/apiv1/index.js:320 Design & Architecture nitpick systemErrorReport is mounted on referenceDataRoutes (and out of the otherwise-alphabetical order, between reports and scheduledVaccine). These sub-routers are purely organisational grouping...
packages/facility-server/app/routes/apiv1/systemErrorReport.js:7 Integration tests suggestion POST /api/systemErrorReport (facility) relies on spread order — { ...body, userId: user.id, recipients } — to stop a client overriding the reporting user id or the recipient list, but no test c...
packages/facility-server/app/routes/apiv1/systemErrorReport.js:14 Integration tests suggestion settings[facilityId].get(...) is unguarded, but req.facilityId is only set when a facility has been selected — auth.js explicitly notes "when we login to a multi-facility server, we don't initi...
packages/facility-server/app/routes/apiv1/systemErrorReport.js:15 Design & Architecture suggestion Recipient resolution is on the wrong side of the wire: the facility reads systemErrorReport.recipients from settings and posts them to central, and central's route accepts whatever recipients t...
packages/web/app/api/TamanuApi.jsx:105 Design & Architecture suggestion isAdminRoute() puts route-structure knowledge (/^\\/admin(\\/|$)/, plus a five-line comment explaining that /facility-admin must not match) inside the API client, which otherwise knows nothing ...
packages/web/app/components/Table/Table.jsx:543 Design & Architecture suggestion The Table/Paginator footer rework (new leftContent prop, renderExportButton extraction, export button moved inside the paginator cell, FooterContent re-laid out to space-between) change...
packages/web/app/components/Table/useClientSideTableData.js:20 Bugs & Correctness suggestion page is never clamped when data shrinks, and the only reset happens on sort change. With more than one page of errors (>25) sitting on page 2, sending the log removes all rows and leaves `page ...
packages/web/app/store/systemErrors.js:40 Design & Architecture nitpick The reducer carries ~35 lines of prose comment for ~30 lines of code, much of it explaining dev-vs-prod redux-persist behaviour and justifying the LOGIN_SUCCESS clear at length. The behavioural f...

Nitpicks

File Line Agent Comment
packages/central-server/__tests__/systemErrorReport.test.js 91 Integration tests expect(response).not.toHaveSucceeded() in the unauthenticated and email-failure cases passes on any status ≥ 400, including a 500 from an unrelated crash, so neither case actually pins the contract. Use the project matchers instead: toHaveRequestError() (or toHaveStatus(401)) for the unauth...
Local fix prompt (copy to your coding agent)
Fix these issues identified on the pull request. One commit per issue fixed.

-------

`packages/web/app/api/relegateSystemError.js:27`: `crypto.randomUUID()` is only defined in a secure context, and Tamanu is regularly accessed over plain HTTP by IP address — this exact hazard is already called out in the codebase (`packages/web/app/features/Invoice/InvoiceForm/InvoiceForm.jsx:106-110`: "Saves users from getting crash with 'crypto.randomUUID is not a function'"), and `aiFormBuilder/chatState.js:8` guards it with `globalThis.crypto?.randomUUID?.() ?? …`. Here it is unguarded and runs from inside `TamanuApi.fetch`'s catch block, so on such a deployment every relegated server error throws a `TypeError` that replaces the original API error seen by the caller, and no system error is ever recorded — i.e. the whole feature fails closed on the deployments most likely to hit server errors. Use `uuid`'s v4 (as InvoiceForm does) or the optional-chaining fallback pattern.

-------

`packages/web/app/views/patients/PatientListingView.jsx:196`: `DebugAddSystemErrorButton` ("Temp: manually adds errors to debug") is rendered unconditionally in the patient listing top bar, so every clinical user in production gets an untranslated "Add system error (debug)" button that injects fake error rows into the System errors view and can then be emailed to support as a real report. This looks like leftover scaffolding — remove it before merge (or gate it behind `IS_DEVELOPMENT` from `utils/env`).

-------

`packages/web/app/store/systemErrors.js:66`: `ADD_SYSTEM_ERROR` appends without any cap or de-duplication, and the 24h purge only runs when the System errors view is opened. A polling/refetching query against a broken endpoint (react-query retries, task/sync polling) will push a new row every few seconds for the whole session, growing the slice unboundedly and producing a support email containing thousands of near-identical lines. Consider capping the list (keep the most recent N) and/or collapsing repeats of the same path+message with a count.

-------

`packages/web/app/components/Sidebar/Sidebar.jsx:351`: The generic `Sidebar` now hardcodes knowledge of one specific menu item: `item.children.find(child => child.key === 'systemErrors')` appears three times (retracted branch, expanded branch, and the `child.key === 'systemErrors'` overlaid-dot check), plus a bespoke branch that renders the item a second time outside its own section. A shared navigation component shouldn't know about a single feature by string key — the next item that wants a badge will copy-paste this. Drive it from the menu item definition instead (e.g. `child.badgeSelector`/`showWhenSectionCollapsed` declared in `FACILITY_MENU_ITEMS`), so `Sidebar` only knows "some items can carry a badge and stay visible when collapsed".

-------

`packages/web/app/api/relegateSystemError.js:27`: The error is flattened into one pre-formatted English blob (`Something went wrong on the server. Path: X. Message: Y`) before being stored, which throws away the structure the rest of the feature needs: the table can't ever split path from detail into its own columns or sort/filter on path, the string can't be translated (every other user-facing string in this PR is `TranslatedText`), and the tests end up regex-matching prose. Store the fields (`path`, `detail`, `timestamp`) and format at render time in `SystemErrors`/`SendErrorLogModal` instead.

-------

`packages/central-server/app/systemErrorReport.js:52`: `recipients` is taken straight from the request body and used as the email `to:`, with no server-side constraint and no test covering that trust boundary — `systemErrorReport.test.js` only ever posts recipients it chose itself, so it asserts the endpoint faithfully mails wherever the caller says. Any authenticated central user can therefore use this as a mail relay with attacker-controlled body text. Either derive/allowlist recipients on the central side (the facility already resolves them from settings, so the client value adds nothing) and add a test that a body-supplied recipient is ignored, or add an explicit test documenting the allowlist.

-------

`packages/web/app/components/Table/useClientSideTableData.js:22`: This `useMemo` never hits its cache: `customSort` from `useTableSorting` is a plain function re-created on every render (see `useTableSorting.js:8` — no `useCallback`), so the dep array changes identity every render and the full array is copied, sorted and sliced on each one. It's also why the missing `orderBy`/`order` deps don't show up as a bug. Depend on `[data, page, rowsPerPage, orderBy, order]` instead (or wrap `customSort` in `useCallback([orderBy, order])`) so the sort actually only runs when the inputs change — otherwise any future caller with a non-trivial dataset re-sorts on every parent re-render.

-------

`packages/central-server/app/systemErrorReport.js:24`: The schema bounds the `errors` array from below (`.min(1)`) but not from above, and puts no length limit on `message` or `additionalInformation`. Since the web slice that feeds this is itself unbounded, a normal (not malicious) submission can carry thousands of entries, which are then concatenated into a single in-memory email body and handed to the SMTP service — a large allocation per request and likely an SMTP rejection rather than a useful report. Add `.max(...)` to the array and to the free-text fields so the payload and resulting email are bounded.

-------

`packages/web/app/api/relegateSystemError.js:1`: This introduces module-level mutable state (`let handler = null`) plus a global setter as the bridge from the API layer into Redux — a hidden singleton that any importer can silently rebind, and which forces the `if (handler) ... else console.error` dead-end branch for a case that can only happen if startup ordering breaks. The file's own comment notes the existing solution: `setAuthFailureHandler` hangs the callback off the `API` instance. Follow that pattern (`API.setSystemErrorHandler(...)` / `this.systemErrorHandler`) so the dependency is owned by the object that uses it, is per-instance for tests, and the unregistered-handler branch becomes unnecessary.

-------

`packages/central-server/__tests__/systemErrorReport.test.js:91`: `expect(response).not.toHaveSucceeded()` in the unauthenticated and email-failure cases passes on any status ≥ 400, including a 500 from an unrelated crash, so neither case actually pins the contract. Use the project matchers instead: `toHaveRequestError()` (or `toHaveStatus(401)`) for the unauthenticated request, and an explicit `toHaveStatus(500)` for the send failure.

@github-actions

github-actions Bot commented Sep 13, 2026

Copy link
Copy Markdown

Android builds 📱

@NavarroEmilioLuis
NavarroEmilioLuis added this pull request to the merge queue Sep 16, 2026
Merged via the queue into main with commit d1ca5bd Sep 16, 2026
115 of 117 checks passed
@NavarroEmilioLuis
NavarroEmilioLuis deleted the feature/tam-6806-relegate-red-toast-error-messages branch September 16, 2026 23:38
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.

1 participant