chore(web): move App.tsx's module-level block into files - #2149
Conversation
Phase 0 of #2126. Lines 239-668 of clients/web/src/App.tsx were already at module scope — helpers, formatters, constants, and the toast subcomponents, none of which close over component state — so the whole block moves verbatim into files. App.tsx drops from 5,303 to 4,892 lines with no behavior change. Placement follows the lib-vs-utils rule in AGENTS.md: pure computation to utils/, anything touching the environment to lib/. EMPTY_SETTINGS and the two loose toast ids deliberately do NOT land in a new top-level src/constants.ts, which the issue suggested: the web coverage include is a whitelist naming components/hooks/theme/lib/utils/server, so a module there would have fallen out of the >=90 gate silently. They go under utils/ instead. ReAuthBannerBar is in the block but not in the issue's table — it was added after the issue was written (#2108). It moves to components/groups/ReAuthBanner/, beside the banner it wraps. Every new module is at 100% on all four coverage dimensions. Two v8 ignores, each justified in place: getAuthToken's typeof-window guard (a happy-dom inherent path) and replayProtocolRequest's provably-dead default case. Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
Pull request overview
Phase 0 of decomposing App.tsx, extracting module-level helpers and UI fragments into gated, purpose-specific modules without intended behavior changes.
Changes:
- Moves environment adapters to
lib/and pure helpers toutils/. - Extracts toast and re-auth UI components.
- Adds unit tests, Storybook coverage, and architecture documentation.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
AGENTS.md |
Documents new lib and utils examples. |
clients/web/README.md |
Updates web architecture guidance. |
clients/web/src/App.tsx |
Replaces module-level definitions with imports. |
clients/web/src/lib/authToken.ts |
Extracts token and redirect handling. |
clients/web/src/lib/authToken.test.ts |
Tests token source priority and storage failures. |
clients/web/src/lib/protocolReplay.ts |
Extracts log conversion and protocol replay. |
clients/web/src/lib/protocolReplay.test.ts |
Tests replay methods and log filtering. |
clients/web/src/utils/errorFormat.ts |
Extracts error-formatting helpers. |
clients/web/src/utils/errorFormat.test.ts |
Tests error formatting branches. |
clients/web/src/utils/serverSettingsDefaults.ts |
Extracts stable settings defaults. |
clients/web/src/utils/serverSettingsDefaults.test.ts |
Verifies defaults and identity. |
clients/web/src/utils/stepUp.ts |
Extracts step-up adapters and type. |
clients/web/src/utils/stepUp.test.ts |
Tests step-up classification. |
clients/web/src/utils/toasts/toastIds.ts |
Extracts shared toast identifiers. |
clients/web/src/utils/toasts/toastIds.test.ts |
Tests stable toast identifiers. |
clients/web/src/utils/toasts/progressToasts.ts |
Extracts progress-toast formatting. |
clients/web/src/utils/toasts/progressToasts.test.ts |
Tests progress-toast behavior. |
clients/web/src/utils/toasts/taskToasts.ts |
Extracts task-toast helpers. |
clients/web/src/utils/toasts/taskToasts.test.ts |
Tests task status and formatting. |
clients/web/src/components/groups/ReAuthBanner/ReAuthBannerBar.tsx |
Extracts the re-auth banner container. |
clients/web/src/components/groups/ReAuthBanner/ReAuthBannerBar.test.tsx |
Tests banner positioning styles. |
clients/web/src/components/elements/Toasts/ToastPrimitives.tsx |
Adds shared toast primitives. |
clients/web/src/components/elements/Toasts/FetchBodyDroppedToastMessage.tsx |
Extracts body-drop warning content. |
clients/web/src/components/elements/Toasts/OutputValidationToastMessage.tsx |
Extracts validation warning content. |
clients/web/src/components/elements/Toasts/UrlElicitationErrorToastMessage.tsx |
Extracts elicitation error content. |
clients/web/src/components/elements/Toasts/Toasts.test.tsx |
Tests extracted toast components. |
clients/web/src/components/elements/Toasts/Toasts.stories.tsx |
Adds toast component stories. |
Suppressed comments (7)
clients/web/src/lib/protocolReplay.test.ts:30
- The double cast bypasses the
MessageEntrycontract even though this is intended to be a valid request fixture. Type it directly and supply the required tracking id andDatetimestamp.
} as unknown as MessageEntry;
clients/web/src/lib/protocolReplay.test.ts:35
- The double cast bypasses the
MessageEntrycontract even though this is intended to be a valid response fixture. Type it directly and supply the required tracking id andDatetimestamp.
} as unknown as MessageEntry;
clients/web/src/lib/authToken.test.ts:59
- The double cast is unnecessary and violates the repository's type-safety rule. Delete the injected property through the reflection API instead.
delete (window as unknown as Record<string, unknown>)[
INSPECTOR_API_TOKEN_GLOBAL
];
clients/web/src/lib/authToken.test.ts:64
- Avoid casting
windowthroughunknownjust to install the test global;Reflect.sethandles a dynamic property key without erasing the object's type.
(window as unknown as Record<string, unknown>)[INSPECTOR_API_TOKEN_GLOBAL] =
"from-global";
clients/web/src/lib/authToken.test.ts:74
- Avoid the unjustified double cast when setting the empty injected token; use the reflection API for the dynamic global property.
(window as unknown as Record<string, unknown>)[INSPECTOR_API_TOKEN_GLOBAL] =
"";
clients/web/src/lib/authToken.test.ts:80
- Avoid the unjustified double cast when setting the non-string injected token; use the reflection API for the dynamic global property.
(window as unknown as Record<string, unknown>)[INSPECTOR_API_TOKEN_GLOBAL] =
123;
clients/web/src/lib/authToken.test.ts:100
- Avoid casting
windowthroughunknownhere as well; use the reflection API to set the injected token while preserving type safety.
(window as unknown as Record<string, unknown>)[INSPECTOR_API_TOKEN_GLOBAL] =
"from-global";
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Move REPLAYABLE_PROTOCOL_METHODS / isReplayableProtocolMethod out of components/groups/protocolUtils.ts into utils/replayableProtocolMethods.ts. It is a pure predicate over a string set, so by the project's own lib-vs-utils rule it never belonged under components/ — and keeping it there was what made lib/protocolReplay.ts import 'up' into the component graph. Both consumers now import from utils/; the predicate keeps its own test. - Give replayProtocolRequest an exported ReplayClient = Pick<InspectorClient, ...> naming the nine methods it calls, so a caller (and the test) can satisfy the real contract instead of casting a stub into a whole client. - Rebuild the protocolReplay fixtures as plain typed MessageEntry values. The double cast had been hiding a real defect: MessageEntry.timestamp is a Date and the fixture passed a number, so the assertion confirmed a shape messagesToLogEntries cannot produce. - Replace the window casts in authToken.test.ts with Reflect.set / Reflect.deleteProperty. No 'as unknown as' remains in either test. - Drop the stale 'floats top-right' sentence on ReAuthBannerBar, which moved verbatim and arrived contradicting the paragraph below it. Declined: re-homing ReAuthBannerBar's transform/zIndex as a theme variant. 'styles' is the Styles API rather than an inline style, Mantine exposes neither property as a prop, and a styling change is out of scope for a phase whose safety argument is that nothing was edited. Signed-off-by: cliffhall <cliff@futurescale.com>
|
Round 1 of Copilot review addressed in 4b119c9. Mirroring here since the inline replies go hidden once the threads are outdated. Accepted — 6 of 7
Declined — 1 of 7
|
Closes #2127
Phase 0 of #2126 — the module-level block at the top of
clients/web/src/App.tsxmoves into files. App.tsx: 5,303 → 4,892 lines (−411). No behavior change: every symbol moved verbatim, and the only edits to the bodies are theexportkeyword and the import paths.What moved where
Placement follows the
libvsutilsrule in AGENTS.md — pure computation toutils/, anything touching the environment tolib/.redirectUrlProvider,getAuthTokenlib/authToken.tsmessagesToLogEntries,replayProtocolRequestlib/protocolReplay.tserrorMessage,errorCodeOf,formatErrorDetailsutils/errorFormat.tsEMPTY_SETTINGSutils/serverSettingsDefaults.tsisEmaStepUp,isStepUpConfirmation,StepUpSourceutils/stepUp.tsbodyDroppedToastId,CLIENT_CONFIG_LOAD_ERROR_NOTIFICATION_IDutils/toasts/toastIds.tsPROGRESS_TOAST_AUTOCLOSE_MS,progressToastId,formatProgressToastMessageutils/toasts/progressToasts.tsTASK_CANCELLED_TOAST_AUTOCLOSE_MS,TERMINAL_TASK_STATUSES,isTerminalTaskStatus,taskToastId,taskToastColor,TaskToastInput,formatTaskToastMessageutils/toasts/taskToasts.tsToastCauseList,ToastLinkButtoncomponents/elements/Toasts/ToastPrimitives.tsxFetchBodyDroppedToastMessagecomponents/elements/Toasts/FetchBodyDroppedToastMessage.tsxOutputValidationToastMessagecomponents/elements/Toasts/OutputValidationToastMessage.tsxUrlElicitationErrorToastMessagecomponents/elements/Toasts/UrlElicitationErrorToastMessage.tsxReAuthBannerBarcomponents/groups/ReAuthBanner/ReAuthBannerBar.tsxThree judgment calls worth reviewing rather than deciding silently
1.
EMPTY_SETTINGSand the toast ids did NOT go to a new top-levelsrc/constants.ts, which is what the issue's table suggested. The web coverageincludeinclients/web/vite.config.tsis a whitelist namingcomponents/hooks/theme/lib/utils/server, so a module atsrc/constants.tswould have fallen out of the ≥90 gate entirely and silently — the exact hazard AGENTS.md calls out. They live underutils/instead, where they are gated like everything else.2. The issue asked whether the error helpers already had a near-duplicate. Checked:
clients/web/src/{utils,lib,components}export nothing equivalent, so there was no home to extend. Theerr instanceof Error ? err.message : String(err)idiom does appear inline ~10 times incore/, but that is a different package with noweb-side importer, and hoisting it there is a wider change than this phase.3.
ReAuthBannerBaris in the block but not in the issue's table — it was added to App.tsx after the issue was written (#2108). It moved too, tocomponents/groups/ReAuthBanner/, beside the banner it wraps.Two
v8 ignoreannotations, both justified in placegetAuthToken'stypeof window === "undefined"guard — a happy-dom-inherent path, one of the categories AGENTS.md names. The function was never gated before (it lived in the ungatedApp.tsx), so this is the first time the branch has had to answer for itself.replayProtocolRequest'sdefault:case — provably dead:isReplayableProtocolMethodadmits exactly the nine methods the switch enumerates. Kept anyway so that adding a method toREPLAYABLE_PROTOCOL_METHODSwithout a case here reports a reason rather than resolving as a dispatched replay.Coverage
Every new module is at 100% on all four dimensions (lines/statements/functions/branches), comfortably over the ≥90 gate:
The four toast components render through
renderWithMantine(never a hand-rolled provider).getAuthTokengets a case per source in the priority order plus both storage-unavailable branches — happy-dom returns a freshStoragewrapper perwindow.sessionStorageaccess, so a spy on the instance never fires; the test replaces the property instead, and says so in a comment.Toasts/was the onlycomponents/elements/directory without a*.stories.tsx, so it has one now, matching the convention the web README states.Docs
AGENTS.mdandclients/web/README.mdboth carry an anchors list for thelibvsutilssplit; the new modules are added to both.Screenshots
None — this is a pure move with no rendered change. The four toast bodies and the re-auth banner render from the same JSX they did before, through the same call sites.
Verification
npm run cigreen.