Feature/refactor - #115
Merged
Merged
Feature/refactor#115
Conversation
Covers workspace-pge and workspace-ojh. Toolchain (workspace-pge): - Declare @ant-design/icons, react and react-dom as explicit dependencies; they were imported but never declared. - Add typecheck / test / test:watch scripts, engines "^22.12.0 || >=24.0.0" and .nvmrc 24. - src/setupTests.js now imports '@testing-library/jest-dom/vitest' and is wired in via setupFiles; previously the setup file was never loaded. - Add a harness test so a silently-broken test harness cannot ship again. Spec bootstrap (workspace-ojh): - src/swagger_spec.json is git-ignored yet imported by App.tsx, so a fresh clone could not build. `npm run spec` now materializes it. - scripts/downloadYML.js fetches scripts/releases.json itself, treats the GitHub token as optional (the asset is public), retries with backoff, validates the YAML, and writes only on full success so a failed run can never clobber a good spec. - scripts/checkSpec.js runs as prebuild/predev, so a missing spec fails with a message naming `npm run spec` instead of an unresolved-import error. - CI: both workflows use `npm run spec`, .nvmrc and npm caching. test.yml now actually runs typecheck and `npm test` - it previously only built, which was a workspace-pge criterion that had not been implemented. Note: regenerating the spec picked up a real upstream change vs the stale local copy - /cgmy/riskmetric and /cgmyse/riskmetric differ in where `additionalProperties: false` attaches (semantically minor).
…m App Covers workspace-938 and workspace-7da. Logo and module renames (workspace-938): - Remove the unused `import React` that failed noUnusedLocals under strict TS. - Move Logo to src/components/Logo.tsx with a typed LogoProps interface (height/width/className, defaulting to 1em/1em/logo-primary). - Rename styles.tsx -> styles.ts and copyToClipboard.tsx -> .ts; neither contains JSX, so the .tsx extension was wrong. Firebase initialization (workspace-7da): - src/firebase.ts now owns config composition (src/config.json plus the build-time VITE_FirebaseAPIKey) and a lazy, memoised singleton. - App.tsx no longer calls initializeApp()/getAuth() at module scope, so importing it performs no side effects and components can be rendered in tests without live Firebase wiring, or with a stubbed module. - Also drops the stale `process.env.REACT_APP_FirebaseAPIKey` CRA comment. Verified: tsc clean, build passes, tests green. The lazy-init invariant was proven in a browser by asserting getApps() is empty after importing App.tsx and equals one only after the first getFirebaseAuth() call.
…pboard API
copyToClipboard was a vendored gist built on the deprecated
document.execCommand('copy'). It injected a hidden <textarea>, clobbered the
user's existing text selection, leaned on four non-null assertions against
document.getSelection(), and could not report failure - so the UI always
claimed the token had been copied.
It also carried outright dead code: el.contentEditable and el.readOnly were
saved from a freshly created element (so always the defaults) and restored
after that element had already been removed from the DOM.
navigator.clipboard.writeText needs none of that: no DOM injection, no
selection to trample, and it reports failures asynchronously.
A legacy execCommand fallback is deliberately NOT kept. Every browser in the
project's browserslist supports the async API ("not dead" excludes IE11 and
legacy Safari) and both deploy targets are secure contexts - GitHub Pages
serves HTTPS and vite dev runs on localhost - so the fallback would only add
deprecated surface for browsers this project does not support.
The caller now awaits the promise and shows a distinct failure message,
distinguishing an unsupported context from a refused write.
…E_TAG Environment configuration was implicit and partly dead: the key arrived via import.meta.env with a stale Create React App-era comment describing a process.env variable that no longer applied, and CI appended VITE_TAG to .env on every deploy while nothing in src ever read it, so the release tag never reached the page. src/env.ts is now the only place env is read, typed via an ImportMetaEnv augmentation, and it validates the required key before render. A misconfigured deployment shows a readable Configuration error panel instead of a fully rendered page whose auth calls fail for reasons nothing on screen explains. Two details worth recording: The validation logic is written as pure functions taking the env as an argument, because Vite replaces import.meta.env.* with literals at transform time. They therefore cannot be stubbed at runtime, and an earlier test using vi.stubEnv passed only by coincidentally matching a local .env - it would have gone red on CI, which runs without one. Purity keeps every branch testable. firebaseOptions became a function rather than a module-scope const: calling the validator from a const would have moved the failure back to import time, which is exactly what the Firebase extraction removed. VITE_TAG is surfaced in the footer rather than dropped, which keeps the existing outputTag deploy machinery meaningful. Note it is the option_price_faas release tag, not this site's package version.
…real project The README was the original CRA template. It documented npm start and npm run eject - neither exists here - described npm test as interactive watch mode, pointed at a build/ output directory this project does not use, and said nothing about the actual stack, the required environment variables, the OpenAPI spec bootstrap, or the GitHub Pages deploy path. Rewritten against verified ground truth: every documented command was checked against package.json scripts, and every internal link against the filesystem. Three errors were caught in the draft by that check rather than left in place: the Vitest config is vitetest.config.ts, not vitest.config.ts; scripts/releases.json is listed as generated and git-ignored rather than as a committed file, since presenting it otherwise repeats the exact mistake this README exists to fix; and the migration note was reworded so it does not reintroduce the literal legacy command strings it is describing as gone. Troubleshooting entries are drawn from failures actually observed during this work, not generic boilerplate.
…mponents
App.tsx mixed the auth observer, the token fetch, the signed-in/out branching
and all presentation in one 120-line component. It is now a thin composition
root: state lives in src/hooks/useAuth.ts, presentation in
src/components/{AppHeader,TokenNotice,ApiDocs,AppFooter,AuthLoading}.tsx.
App.tsx no longer imports firebase/auth at all.
Two defects the decomposition fixed on the way:
A failed getIdToken ended in a catch that only wrote to the developer console,
so the UI rendered a signed-in shell holding an empty token and Copy Token
cheerfully copied nothing. The hook now records the failure and the root
renders it.
There was no loading state. Firebase delivers the persisted session
asynchronously, so until that first callback landed the code had to guess, and
it guessed signed-out - flashing the login buttons at returning users. The
hook exposes loading/authenticated/signed-out and AuthLoading covers the gap.
useAuth takes an optional injected Auth so a test can supply a fake instead of
live Firebase wiring, which was the stated motivation for extracting Firebase in
the first place. The lazy, memoised singleton is only touched at render time, so
importing the hook stays side-effect free.
Verified with a throwaway suite (5 tests, all passing) covering the loading ->
authenticated transition, signed-out resolution, the token-failure path,
observer unregistration on unmount, and that importing App.tsx still creates no
Firebase app. That file is deliberately not committed here - the permanent test
suite is workspace-czo's deliverable - but the scenarios above are the ones to
adopt.
The header used floats for both of its children: the logo floated leading and the menu floated trailing. Nothing contained the floats, so the vertical alignment was faked with paddingTop = (64 - 50) / 2 recomputed in JS, and the trailing float was free to collide with the brand instead of being pushed away from it. Both floats are gone. The header is a flex container with align-items: center and the menu gets margin-inline-start: auto, which absorbs the remaining space and makes overlap structurally impossible. The paddingTop constant and its arithmetic are deleted rather than ported. Spacing is now a named scale of CSS custom properties in index.css (--space-xs..--space-4xl, --header-height, --logo-size). index.css holds the values so there is exactly one place to retune them; every consumer references a token. The horizontal gutter is a clamp(), so header, content and footer share one rhythm that is 16px on a phone and 48px on a desktop with no breakpoint bookkeeping. The old fixed 50px gutter ate 100px of a 320px screen. Also deletes .App (referenced nowhere) and the now-unused src/styles.ts, and moves the config-error panel off inline magic pixels onto a .config-error class. That class lives in index.css, not App.css, because the failed-config branch never mounts App and so never loads App.css. Verified across 320/375/414/768/1024/1440/1920px in a real browser: no document overflow, logo contained in the header band, and logo right edge never crossing the menu left edge. The suite was validated with a negative control - widening the logo to 400px fails exactly the 320/375/414 cases and still passes 768+, confirming the assertions measure real overflow rather than passing vacuously. Those viewport tests are held back for the permanent suite (workspace-czo); tsc and the existing 19 tests pass, build is clean.
…in failures Adding an OAuth provider previously meant copying a whole <Row><Col> block and its pair of empty spacer columns, then wiring another button by hand. Each provider also had its AuthProvider constructed at module scope, and every signInWithPopup call was fire-and-forget with no catch. Providers now live in a config array (src/authProviders.ts). Adding one is a single entry: key, label, icon, and a create() factory. The login panel maps over it, so no component, markup or layout change is needed, and new buttons inherit width, centring, pending state and error handling for free. Sign-in failures are now visible. describeSignInError maps the Firebase codes users actually hit - popup closed, popup blocked, network failure, account already exists, too many requests - onto a sentence they can act on, with unknown codes still surfacing the underlying message. useAuth owns signIn and tracks pendingProvider so a second click cannot stack a request Firebase would cancel anyway. Providers are built in create() at sign-in time rather than at import. Beyond keeping the import side-effect free, this gives every attempt a fresh provider, so a cancelled popup cannot leave stale scopes behind for the next. Centring uses flexbox with a max-width token instead of empty grid columns, so it works at 320px rather than only at md and up. Also drops the react-social-login-buttons dependency: it baked 29 hardcoded brand colours into its own JS, which no ConfigProvider theming could reach, and conflicts with the token-based theming direction. antd Buttons with @ant-design/icons brand glyphs cover it. Found while verifying, and fixed: - describeSignInError used `cause instanceof Error ? cause.message : String(cause)`, so a thrown object literal collapsed to "[object Object]" and the user saw nothing useful. Now reads `message` structurally. - This project has no global box-sizing reset and antd only sets border-box on its own components, so any of our own elements using width:100% with horizontal padding would overflow the viewport. Added a reset scoped to our classes; a global * reset would reach into Swagger UI, whose layout we neither own nor want to perturb. Verified with 19 throwaway tests (38 total, all passing) covering: one button per config entry for any count; the shipped config rendering all three providers; the whole entry reaching signIn; rendering constructing no provider; shipped factories returning a fresh instance per call; error mapping for closed/blocked/network/unknown/non-Error; other buttons disabled while one is in flight; and centring at 320/375/768/1920 with equal side gaps, width capped at the token, tappable targets and no document overflow. An end-to-end test drives the real App with a mocked popup: the click reaches signInWithPopup with a real GoogleAuthProvider, and the mapped message appears in the DOM. Held back for the permanent suite (workspace-czo), preserved at /tmp/sju-verified-login.test.tsx and /tmp/sju-verified-signin-e2e.test.tsx.
The 'Adding a sign-in provider' example told readers to call
new OAuthAuthProvider("microsoft.com"). No such export exists - the real
generic class is OAuthProvider. Confirmed against the @firebase/auth
typings (OAuthProvider: 33 hits, OAuthAuthProvider: 0) and reproduced:
TS2724: '"firebase/auth"' has no exported member named 'OAuthAuthProvider'.
Did you mean 'OAuthProvider'?
Self-inflicted in the previous commit and caught only by checking my own
claim instead of assuming it. Also verified the corrected snippet actually
typechecks against the real ProviderConfig type, and moved LoginPanel into
the components/ block of the repository-layout tree instead of listing it
as a stray top-level path.
Addresses the four acceptance criteria: 1. `npm test` runs a non-zero number of tests and passes locally: 66 tests, 9 files. 2. The test workflow fails the build on test failure. Verified empirically rather than assumed: a deliberately failing test exits 1; a run matching zero test files also exits 1, so the suite cannot quietly empty itself and still report green; no continue-on-error anywhere in the workflows. 3. useAuth, copyToClipboard, the login component and the header/menu all have tests. The first three were held out of the tree pending this ticket and are now permanent; the header/menu gap is filled by AppHeader.test.tsx, which tests behaviour (the Log Out item tracks session state and fires exactly once) rather than only the geometry the viewport suite already measures. 4. The stale jest-dom import is gone; setupTests.js uses the /vitest subpath. Also makes coverage reporting real. The config declared a `coverage` block but @vitest/coverage-v8 was never installed, so `--coverage` died with MISSING DEPENDENCY - a trap waiting for anyone who tried it. Installed the matching provider and added `npm run test:coverage`. Now 80% lines overall, with the four AC-critical modules at useAuth 87%, copyToClipboard 100%, LoginPanel 100%, AppHeader 100%. Failure screenshots and .vitest-attachments are gitignored; browser mode writes them into src/ and they would otherwise land in the repo. Switched the viewport tests off the deprecated @vitest/browser/context import to vitest/browser. This suite is meant to be durable, so leaving an import that "will stop working in the next major version" in every responsive test was the kind of latent time bomb this ticket exists to remove. Verified the replacement provides page.viewport before converting all three files. Three findings worth recording, all from testing instead of assuming: - I was convinced TokenNotice was broken. antd Alert's heading prop is `message`, and the component passes `title`, so I expected a headingless alert. antd 6.3.1 reports the opposite: "message is deprecated, use title". The component was already correct and my intended fix would have broken it. Only the deprecation warning surfaced this; grepping for the old prop name would have found nothing and left the code untouched by accident rather than by knowledge. - Two copy-feedback tests were passing vacuously. antd renders `message` into its own portal that survives unmount(), so tests 2 and 3 were reading test 1's leftover toast. A clean-state guard made them fail immediately. The fix is message.destroy() in beforeEach. Wiping document.body.innerHTML is wrong in a subtler way: antd caches that container, so later toasts render into a detached node that never appears, which looks like the feature broke rather than like the teardown was wrong. - The coverage table in the terminal render is truncated and omitted files that were in fact exercised (AppHeader, LoginPanel showed nothing). Reading coverage-final.json directly gave the true per-file numbers. Trusting the console render would have led me to report coverage of modules I could not see. These are documented in README under "Gotchas when writing tests here", along with the render/await, effect-flush and viewport-API traps.
There was no ConfigProvider anywhere in the app; the header said theme="dark"
in the component and the logo's fill was a hex literal in the stylesheet.
Re-skinning meant hunting colours through JSX and CSS.
src/theme.ts is now the only file in src/ that contains a colour literal. It
feeds both consumers from one palette per mode: antd via
<ConfigProvider theme={themeFor(mode)}>, and our own stylesheet via CSS
custom properties applied to the document element. App.css lost its hex,
AppHeader lost its hardcoded menu scheme (now a prop sourced from the theme),
and the config-error panel is themed too because the vars are applied before
the try/catch rather than inside the happy path.
The brand colour is deliberately not the button colour. BRAND.accent is
3.90:1 on white - clears WCAG's 3:1 for the logo as non-text content, fails
the 4.5:1 needed for text. BRAND.action is the same hue darkened until white
label text clears AA in all three states (rest 5.48, hover 4.97, active
7.70), because WCAG applies to hover and active, not just rest. Component
tokens are pinned explicitly rather than left to antd's derivation, so an
antd upgrade cannot silently move a colour out of compliance.
var() has no fallback on purpose: a fallback keeps the old colour alive when a
token is missing, which is exactly how a hardcoded value smuggles itself back.
Three layers of enforcement, because the invariant needs to outlive review:
- theme.test.ts asserts every contrast pair in light and dark, and pins the
accent/button distinction so it cannot be "simplified" back into reuse
- theme.appearance.test.tsx proves the token reaches the SVG fill, and that
overriding --brand-accent restyles it without touching a component
- npm run lint:colors fails the build on any hex/rgb outside theme.ts, wired
into predev, prebuild and the test workflow
Verified with negative controls rather than by eye: substituting BRAND.accent
for BRAND.action fails the AA tests; grey header text on the header band
fails at 2.01:1; an unthemed button is confirmed to be antd's stock
blue rgb(22,119,255), so the themed-button assertion distinguishes real
theming from nothing. The previously-unenforced claim that theming applied at
all now has a test: a dropped ConfigProvider would fail instead of shipping
blue.
typecheck clean, 94 tests pass (12 files), build passes, lint:colors clean.
deploy.yml ran spec -> build -> upload -> publish. A deploy - including a
manually re-run one - could publish code that failed its own tests, with the
failure reported on a separate workflow that nothing was waiting on.
deploy.yml is now three jobs: test -> build -> deploy. The test job generates the
spec the module graph needs, installs Chromium and runs the gate; build declares
`needs: test`, so a failing suite stops the artifact being uploaded at all
rather than leaving it to a red tick someone has to notice.
The check set is defined once, as `npm run verify` (colour guard + typecheck +
tests), and both workflows call it. Restating the steps per workflow is exactly
how the local gate and the deployed gate would drift apart. verify deliberately
excludes build: the deploy build step stamps VITE_TAG and VITE_FirebaseAPIKey
into .env first, so a build inside verify would produce an artifact without them.
Verified before wiring anything up:
- the suite passes with .env absent. This had to be checked rather than assumed,
because .env is git-ignored and CI has none - gating deploys on a job that
only passes on my machine would have been worse than the gap it fixes.
- injected failing test -> npm run verify exits 1, so publish would stop
- stray colour literal in App.css -> exits 1 via the guard
- a run matching zero test files exits 1, so an empty suite cannot pass
- parsed both workflows with js-yaml and asserted: build.needs == test,
deploy.needs == build, the test job runs spec + playwright + the gate, and
no workflow contains continue-on-error, || true, allow_failure or if: always()
No actionlint/act is available in this environment, so the structural assertions
are machine-checked against the parsed YAML rather than by a workflow linter; the
job graph and the exit-code behaviour are verified, the GitHub-side runtime is not.
README updated: the Commands table gains verify/test:coverage/lint:colors and the
stale claim that "typecheck is the static gate" is gone, since it stopped being
true when the colour guard landed.
index.tsx and App.tsx each resolved the theme mode independently, using two
different defensive idioms for the same storage read (`globalThis.localStorage?.
getItem("theme")` vs `typeof localStorage === "undefined" ? ... : ...`). Both
worked, so nothing signalled the duplication - but the two results feed different
sinks: the CSS custom properties and the antd header/menu theme. Editing one and
not the other yields a theme that half-updates, which reads as a styling bug and
diagnoses as a wiring bug.
`readThemeMode()` in src/theme.ts is now the single reader. index.tsx resolves
once and passes the mode down; App.tsx takes it as a required prop and contains no
storage or system-preference reads of its own.
The safety nets moved to the call site rather than living inside the default
helpers. My first cut wrapped only the defaults, which meant an injected throwing
storage sailed straight through - caught by the Safari-private-mode test I wrote,
since that mode throws on storage *access* rather than returning null and an
uncaught throw here stops the app rendering at all.
scripts/checkThemeSource.js makes the invariant structural: any localStorage,
sessionStorage or matchMedia reference outside src/theme.ts fails the build.
Verified by planting a stray read in App.tsx and one in AppFooter - both flagged
with file and line. Guards are now aggregated behind `npm run lint` so test.yml and
deploy.yml cannot drift from it.
Test coverage note: injecting the environment covered every branch except the one
the app actually runs, because injection bypasses the default closures entirely.
Added src/theme.browsermode.test.ts to exercise the real localStorage and
matchMedia path. Also dropped a test I had written claiming to verify the stored
choice beating a dark OS preference - headless Chrome reports light, so it could
never fail at the thing it claimed to test; the pure resolveThemeMode tests cover
that override with the input stated explicitly.
Gate: lint + typecheck + 103 tests (13 files), and build, all pass.
Coverage thresholds set from the actual measured values, rounded down to a whole percent: 83 statements, 81 branches, 85 functions, 83 lines. The measured numbers and the resulting headroom are recorded in the config so the gap between floor and reality stays visible to whoever raises it next. Deliberately at the floor rather than an idealised target. An aspirational number fails on every run until someone under deadline pressure sets it to whatever turns CI green, which lands the floor lower than where we already are -- the opposite of the point. Thresholds only enforce when coverage actually runs, and `npm run verify` did not run it, so both CI workflows were calling a gate that could not bite. verify now uses test:coverage; the check set stays defined once in package.json, as with the deploy workflow. The include list is explicit because the denominator was not stable. The bare "src" made v8 try to parse the JSON data files as modules; they fail and drop out, so the same tree measured 207 statements in one run and 191 in another. A ratchet is only worth having if it ratchets a fixed set of files. Measured values are unchanged by this. Verified: the gate fails on a raised threshold (83.76% vs 90%), fails on the committed config with coverage genuinely reduced (71.72% vs 83%), and fails when the measurement itself breaks -- a run where the browser never launches reports 0% and exits non-zero instead of passing quietly. Full verify passes: 103 tests, both lint guards, thresholds met, deterministic across three repeat runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.