fix(renderer): contain a page render throw and get it into the log

[Pixnop]

Summary

Point 2 of #370. A render exception in one page no longer takes the window down with it, and it now reaches error.log.

Beta.7 blanked Manage Mods for anyone with Vanilla Variants installed. A null in a ModDB tag list threw on the first render, React unmounted the whole tree, and nothing was written anywhere. Three player reports and a live probe went into finding a one-line bug. With this in place a log would have named it.

Where the boundary sits

In AnimatedRoutes (App.tsx), around <Routes> and nothing above it. That puts it inside <main>, so the shell keeps running while one page is broken: main menu, session button, activity center, notifications overlay, the ModDB prompt, the global mod update checker. I did not wrap the app in one outer boundary, because a page bug would then leave a player with a message and nothing to click.

resetKey is the current pathname, and a change to it clears the boundary, so walking back into the crashed page gets a real attempt rather than a cached failure. That is not a recovery path on its own, though, which is what the review caught: Home is inside this boundary too, so with Home throwing the pathname never changes and a fallback that only navigates leaves the message on screen for the rest of the session. The fallback's actions now clear the boundary themselves.

The shell does not get its own last-resort boundary. A throw in MainMenu is already an app with no navigation left, so a second fallback would only redraw a dead end. What it does need is the log line, and React rethrows an uncaught render error, which the global error listener below picks up.

What a player sees

Instead of a black window, in the page area: "This page couldn't be displayed", that the rest of the launcher still works, and that the details went to the log files with Info & Help > Debug info naming the folder. Two actions, built from ListWrapper / ButtonsWrapper / FormButton like the rest of the app:

• Go to the main menu clears the boundary and navigates to /. On any page but Home that is the whole recovery. On Home it re-renders the page that threw, which is the right thing to try and is sometimes enough, since plenty of render throws come from a state a re-render no longer reaches.
• Open Info & Help clears the boundary and navigates to /info-and-help. This is the path that does not depend on Home rendering, and it lands on the page the text above already points at for the logs.

The block carries role="alert" with a label, and focus moves to the first action on mount, since the page the player's focus was on has just gone.

New keys live under components.pageError in en-US.json. The other 13 locales fall back until they catch up.

What a maintainer finds in error.log

One error line per throw, tagged [front] [app] [adapters/errorLog.ts] [PageErrorBoundary]. Nothing in it is text read off the error. The second review is the reason it now reads this way: the previous version kept an identifier out of a recognised message and any non-whitespace location out of a stack frame, and both of those treat syntax as provenance. new TypeError("PASSWORD123 is not a function") is a message any code can build, and error.stack is a writable property, so at loadProfile (app://renderer/PASSWORD123:12:9) is a frame any code can write. Both findings were right.

• Name, from an allowlist of the built-in names React and the DOM actually raise (Error, TypeError, RangeError, SyntaxError, ReferenceError, AbortError, DOMException). Anything else reads Error.
• Message, reduced to one of nine fixed tokens and nothing else: null-property-read, undefined-property-read, null-property-write, undefined-property-write, not-a-function, not-iterable, stack-overflow, invalid-array-length, or unclassified-message. No identifier, no property name, no placeholder standing in for one. The token says what kind of failure it was; which property it was is not in the log on purpose, because the stack says which file and line to open and the name is right there in our own source.
• JS stack, one line per frame as at <file>:<line>:<col>. A frame is kept only when its location is under one of the two prefixes our own renderer actually runs from: app://renderer/assets/, which is where src/main/index.ts loads the packaged window from (app://renderer/index.html, served by the app protocol handler out of the bundle directory, so every chunk is assets/<name>-<hash>.js), and http://localhost:<port>/src/, the vite dev server that ELECTRON_RENDERER_URL points at. Those are the same two shapes isAllowedRendererUrl in src/ipc/validation.ts accepts on the main-process side. Everything else is dropped whole. Only the last path segment is emitted, and only when it is a plain file name ending in .js, .mjs, .ts or .tsx; directories, the scheme, the host, the query and the function name all go. Frames are capped at 60.
• Component stack, the component names React wrote, one per line, capped at 60. These are treated as provenance-safe where a message identifier is not, and the reason is where they come from rather than what they look like: React builds a component stack out of the function names of the components it rendered, so every name in it was written in our own source and shipped in our own bundle. A message identifier is the opposite, it is whatever value happened to be in scope when the error was built, which is exactly where a credential lives. The shape check (^[A-Z][A-Za-z0-9]{0,40}$) and the cap are a backstop for the abnormal path, since a component stack arrives as a plain string.
• A rejection that is not an Error contributes non-error-throw and its typeof, and nothing from the value.

For beta.7 that reads TypeError: null-property-read, with ManageMods-<hash>.js and its line in the stack and ManageMods in the component stack. Which line of which file, plus which of the nine failure kinds, is what turned three player reports into a one-line bug, and none of it needs the property name to get there.

What can no longer reach it

An error message is an arbitrary string. new Error(sessionKey) puts a credential in one, a rejected fetch quotes a signed URL in one, and reject(token) is a rejection reason. The main-process redactor only recognises marked fields, known query keys and absolute paths, so none of those would be touched: the first version of this file forwarded all of them and reopened the class #368 closed, and the second still forwarded two narrower slices of them. Nothing forwarded now is a string the renderer read off the error, so:

• Any message, whether or not it matched a shape. A recognised shape contributes its token, not its text.
• The identifier or property name inside a recognised message, in every interpolated position.
• The value of a rejection that is not an Error, of any type.
• The name of a custom error class.
• Every function name in a stack, including on a frame that is otherwise kept.
• Every stack location that is not under our two application-owned prefixes, and on one that is, everything except the file name, line and column.
• The location on a component-stack line, and any name that is not a short PascalCase word.

Two residuals, both tested and left deliberate. A hand-written stack naming a file that does not exist in the bundle, app://renderer/assets/PASSWORD123.js:1:1, still yields PASSWORD123.js: one path segment with no separators and a source-file extension, where real frames come from files the build produced. And an uppercase word on a component-stack line passes the shape check; closing that would take a build-time allowlist of every component we ship, for a value React does not put there.

Reads are per field and each inside its own guard, so an error with a getter or a toString that throws still produces a report with its other fields intact, and cannot throw out of the global error listener that called it. The report is then bounded to 8 KiB, the frame lists trimmed before the header: src/ipc/handlers/utilsHandlers.ts rejects anything past 16 KiB and replaces the whole event with a generic warning, so an unbounded stack used to lose the event entirely.

PageErrorBoundary lives under components/, which tests/security-boundaries.test.ts holds off the preload bridge, so the write goes through src/renderer/src/adapters/errorLog.ts rather than touching window.api itself.

What boundaries do not catch

Event handlers, timers, async code and unawaited promises never reach React. main.tsx installs window error and unhandledrejection listeners on the same log path before the first render. They log and return: no preventDefault, no rethrow, so the devtools console still shows what it always did.

Type

• Bug fix

Checklist

• Targets dev, not main.
• npm run typecheck passes.
• npm run lint:ci passes.
• npm run format:check passes.
• npm run test:coverage passes, coverage at or above the floor in vitest.config.ts.
• npm run build:unpack passes.

Testing

tests/rendererErrorLog.test.ts drives logRenderError directly and asserts on two things for every case: the payload window.api.utils.logMessage received, and the line that comes out the far end, run through the real assertString(..., 16_384) and redactSensitiveText the main process applies. tests/renderer-dom/pageErrorBoundary.test.tsx mounts the real App with ManageMods mocked to throw the beta.7 TypeError, and with HomePage wrapped in a switch a test can flip, which is how "the page stopped throwing" is expressed.

Every test is pinned by a mutant:

Mutant	Killed by
Replace <PageErrorBoundary> with a fragment in App.tsx	all the boundary tests, plus an uncaught error escaping the run
Drop the logRenderError call in componentDidCatch	"writes the error name, message and component stack to the log"
Log without info.componentStack	same test
Drop addEventListener("error", ...)	"logs an error event no boundary could have caught", "logs what is not an Error at all, without the value it carried"
Drop addEventListener("unhandledrejection", ...)	"logs an unhandled promise rejection", same
Drop the focus effect in the fallback	"focuses the way out and navigates home when it is taken"
Forward message verbatim instead of classifying it	8 tests, including the three secret regressions and "still logs when the message getter throws"
Forward String(error) for a non-Error rejection	4 tests, including "does not leave the renderer when it is a rejection reason that is not an Error" and "still logs when toString throws"
Keep a stack line that does not match the frame pattern	5 tests, including "does not leave the renderer when it is inside a stack frame"
Move the message read outside its guard	"still logs when the message getter throws, and keeps the stack"
Drop the 8 KiB bound	"survives a 100 KiB stack, under the limit, still naming the failure" and "survives both oversized at once and still reaches the log file"
Drop the explicit reset on the recovery action	"recovers once the page stops throwing, on the route the boundary is already on"

The four mutants for this round, on top of the table above:

Mutant	Killed by
Restore the identifier capture in the is not a function shape	"does not leave the renderer as the callee of a bad call", "tells a bad call apart"
Restore the function name on a frame that is kept	"drops the function name of a frame it keeps", plus 6 others
Loosen the location pattern back to any non-whitespace text	"drops a frame whose location is not a path our build produced", "drops a frame whose location is a bare host with no path of ours", plus 7 others
Drop the frame cap	"forwards at most a bounded number of frames", "forwards at most a bounded number of component names"

Each mutant was applied alone to the committed tree and restored after, with git status checked clean between runs. The table above it was verified the same way against the previous head.

Six earlier assertions changed with the payload, all of them assertions on text that is now a token: the recognised-message cases, the frame locations, and the beta.7 line in the boundary test. Every other test is untouched and green.

• Focused files: 33 passed (tests/rendererErrorLog.test.ts), 11 passed (tests/renderer-dom/pageErrorBoundary.test.tsx).
• npm run typecheck: passed for node, web and test configurations.
• npm run lint:ci: 0 errors, the same 15 existing React hook warnings.
• npm run format:check: passed.
• npm run test:coverage: 163 files, 2081 passed, 2 skipped; 93.39% statements, 90.13% branches, 92.81% functions, 94.96% lines.
• npm run build:unpack: passed with Electron 44.1.1 on Linux.

Not run: the packaged binary. Nothing here changes the main process.

Related issues

Part of #370

[Zaldaryon]

Changes requested

High: render-error logging can write arbitrary secrets

errorLog.ts:16-26 copies name, message, stack, component-stack text, and String(error) directly into the bridge payload. The main-process redactor only recognizes marked fields, known query keys, and selected absolute paths. A bare secret in an error message or non-Error rejection therefore reaches error.log unchanged. This reopens the credential logging class addressed by #368.

Normalize the error into a fixed safe payload before the bridge, and add regressions for an unmarked secret in reason, message, and stack, checking both the bridge payload and the final log.

Medium: the recovery button cannot recover from a Home render failure

The boundary clears its state only when resetKey changes at PageErrorBoundary.tsx:33-35, but the fallback always navigates to / at PageErrorBoundary.tsx:68. The Home route is inside the same boundary. If Home throws, resetKey stays /, so the only recovery action leaves the fallback unchanged.

Provide a valid recovery path when the current route is /, and add an integrated regression with Home throwing during render.

Medium: extraction happens outside the guard and the payload exceeds the IPC limit

The logger reads error properties and calls String(error) before its try block at errorLog.ts:16-23. A hostile getter or toString() can make the global handler throw. It also sends the complete stacks without applying the 16 KiB limit enforced by utilsHandlers.ts:21-28, so oversized events are discarded and replaced by a generic warning.

Extract defensively, bound the payload before sending, and add tests for hostile values and a stack larger than 16 KiB.

The head is also six commits behind current dev (1d5280a3). Update the branch and rerun the five required checks before requesting approval.

[Pixnop]

@Zaldaryon on the first finding: correct, and it is the worst of the three. errorLog.ts copied name, message, stack, the component stack and String(error) onto the bridge and leaned on the main-process redactor, which only knows marked fields, known query keys and absolute paths. new Error(sessionKey) matches none of those, and neither does reject(token). That is exactly what #368 closed elsewhere, and I reopened it here.

The payload is now built rather than copied. The name comes from an allowlist of the built-in names React and the DOM raise, everything else reads Error. The message is matched against the shapes V8 raises by itself (the Cannot read/set properties of X forms, X is not a function, X is not iterable, Maximum call stack size exceeded, Invalid array length) and emitted as that shape's own template, with the identifier or property inside it kept only when it matches a strict identifier pattern under 48 characters; anything else is unclassified-message. JS stack lines survive only if they match at <name> (<location>:<line>:<col>), so the header line and anything a throw wrote into the middle of a stack are dropped. Component-stack lines keep the name React wrote and lose the location. A non-Error rejection contributes non-error-throw and its typeof, never the value.

Beta.7 still reads TypeError: Cannot read properties of null (reading 'toLowerCase') with ManageMods in the component stack, which was the bar: a null dereference at a named component has to stay distinguishable from a stack overflow.

Tests are in tests/rendererErrorLog.test.ts, and every one of them asserts twice: on the payload window.api.utils.logMessage received, and on the line that comes out of the real assertString(..., 16_384) plus redactSensitiveText pipeline. An unmarked secret is planted in reason, in message, in a stack frame, in the error name, and interpolated into a message that almost matches a known shape. tests/renderer-dom/pageErrorBoundary.test.tsx adds the integrated version through installMockWindowApi, with a page throwing an error whose message is the secret. Mutants: forwarding message verbatim goes red on 8 tests, forwarding String(error) on 4, keeping a stack line that does not match the frame pattern on 5.

[Pixnop]

On the second finding: correct. I wrote the resetKey logic and the fallback's action in the same file and did not put them together. Home is inside the boundary, the action navigated to /, so with Home throwing the pathname never changed, the boundary stayed in its error state, and the one button on screen did nothing at all. The comment I left in the file even says React holds a boundary until something changes the key, which should have been the tell.

Two changes. The boundary exposes a reset, and both actions call it before navigating, so recovery no longer depends on the route changing. And there is a second action, "Open Info & Help", which goes to /info-and-help: a destination outside the failing route, so it works whatever Home does, and the page the fallback's own text already points at for the logs. "Go to the main menu" stays the first action and keeps the focus on mount.

Two regressions in tests/renderer-dom/pageErrorBoundary.test.tsx, both with the real App and HomePage wrapped in a switch. "recovers once the page stops throwing, on the route the boundary is already on" starts at #/ with Home throwing, takes the recovery action once and asserts the fallback is still there (nothing changed, so Home throws again, and what matters is that it was given a real attempt), then flips the switch, takes it again, and asserts the real home page renders. "still leads somewhere usable when the page keeps throwing" never flips the switch and asserts Info & Help renders. Dropping the explicit reset turns the first one red.

[Pixnop]

On the third finding: correct on both halves, and both are embarrassing because the file's own comment explains why the write was guarded and then does the reads above the guard.

Extraction. Every read and every stringification now happens inside a guard, one per field, so a getter or a toString that throws costs that field and not the report. instanceof is guarded too, since a proxy can throw from its getPrototypeOf trap. Nothing can throw out of logRenderError and be handed straight back to it by the global error listener that called it.

Size. assertString(message, "log message", 16_384) rejects rather than truncates, and the handler replaces a rejected line with a generic warning, so an oversized report lost the whole event. The report is now bounded to 8 KiB, measured on the serialised bytes: frames come off the two lists first, the longer one at a time, and the header is truncated only if that is somehow not enough. Half the limit rather than just under it, because redaction runs after and only ever makes a line longer. A coarse 60-frame ceiling sits in front of it so a recursion stack is not walked in full.

Tests in tests/rendererErrorLog.test.ts: a message getter that throws (the stack still lands), a stack getter that throws (the message still lands), a toString that throws, a proxy that throws on every trap, a 100 KiB stack, a 100 KiB component stack, and both oversized at once. Each asserts the payload was still sent, is under 16 KiB, passes assertString without throwing, and still names the failure and the component. Moving one read outside its guard turns the hostile-getter test red; dropping the byte bound turns the two oversize tests red, because 60 frames of bundled paths is about 24 KiB.

[Pixnop]

Rebased onto the current dev tip (5fa0462, which carries #375). No source change; the five required checks are re-running on the new head.

[Zaldaryon]

Changes requested

The current logger closes the original raw message, hostile getter, and size failures, but it still forwards attacker-controlled data through two allowlists.

High: identifier-shaped secrets remain in recognized error messages

At src/renderer/src/adapters/errorLog.ts:44-45, asIdentifier accepts any identifier or dotted path up to 48 characters. A renderer error with this message:

new TypeError("PASSWORD123 is not a function")

produces a payload containing PASSWORD123 is not a function. The same issue affects the property name in the null dereference and assignment shapes. Identifier syntax proves formatting, not provenance. The secret reaches the bridge and the final log.

Use fixed message categories or an explicit allowlist of application-owned names, and add negative tests with identifier-shaped secrets in each interpolated position.

High: a matching stack frame can carry arbitrary location data

At src/renderer/src/adapters/errorLog.ts:58, the location pattern accepts any non-whitespace, non-parenthesis text up to 400 characters. This matching frame is therefore retained:

at loadProfile (app://renderer/PASSWORD123:12:9)

The secret reaches the bridge and the final log. Restrict the accepted location format to trusted application paths or emit only a sanitized filename and line information. Add a regression with a credential-shaped value in a matching stack location.

The exact head is also behind the current dev. PR #375 merged into dev at 5fa04624a76e4433bf16fe86e1c73d51d657fe4d, while this PR still targets the previous base 1d5280a3db7efc6af7bb2adb32d7214530c1d446. Rebase the branch, rerun all five required checks, and request a fresh review after fixing the leaks.

Verification

• Reviewed head: 4d38867cd0791518f901468b6dd4f88da94da9dd.
• Focused boundary and logger tests passed 31 cases.
• The two leak reproductions above passed against the current head and showed the secret in the renderer payload.
• The previous required checks passed on the previous base.

[Pixnop]

Both findings were correct, and I should have caught them myself: this is the third time this week the same lesson has come up in this repo. Identifier syntax proves formatting, not provenance. A thing that looks like a property name, a function name, a file path, is only safe because of where it came from, never because of its shape, and an error is precisely the object that carries whatever value was in scope when it was built.

So I stopped filtering captured text and stopped capturing it. A recognised message maps to a fixed token and nothing else, no identifier and no ? placeholder. A stack frame survives only if its location is under a prefix our own build produces, and then only its file name, line and column, function name dropped. The one place a name still goes through is the React component stack, and I put the argument for it in the file rather than leaving it implicit: React derives those from the function names of components we wrote and ship, so they are application-owned by construction, where a message identifier is the opposite. The shape check and the cap are there as a backstop, not as the argument. Two residuals are named in the PR body with the reason I left each one.

Pushed on the rebase onto 5fa04624. Five checks rerun: typecheck, lint:ci (0 errors, the same 15 hook warnings), format:check, test:coverage (163 files, 2081 passed, 2 skipped, 93.39% statements), build:unpack. Four new mutants in the PR body, each applied alone to the committed tree and restored, git status clean between runs. Back to you.