This plan was written on 2026-08-29, before #226 landed. Its Task 2 lists what to move out of routes/admin.ts into the shared image pipeline, and that list is now missing `stripUploadedImages` and the `reencodeInPlace` import it depends on, because neither existed when the list was written.
Executing it as written would have left the re-encode behind in admin.ts, and the public intake route added in Task 5 would then have had an upload path that skips EXIF stripping entirely. That is precisely what #226 exists to prevent — a stranger photographing an item at home publishing the coordinates it was taken at — and nothing in the suite would have failed to say so, because the intake tests are written against a route that does not exist yet.
The task now names the function, says why it matters, and adds a check with a definite answer: after the move, routes/admin.ts must no longer import imageProcessing. If it still does, something was left behind.
Ref #222, #226
Four tasks. A pure findOrFail helper with Vitest coverage, the nine unchecked lookups converted to use it, the expect timeout raised from Playwright's unset default of 5s to 10s, and the test #245 skipped brought back.
The helper deliberately imports nothing and lives apart from support/api.ts. api.ts imports @playwright/test, and vitest.config.ts runs with environment: 'node' over tests/unit only — putting the helper there would drag a browser harness into the unit suite to test six lines of pure logic.
Two things the survey changed. filters.spec.ts:216 is excluded: its .find() searches CSS class names on a string array rather than test data, so it has no missing-row failure mode. And three `expect(row).toBeTruthy()` assertions are deleted rather than kept, because findOrFail has already thrown by then — leaving them would tell the next reader the value might be falsy, which is the confusion the change exists to remove.
Worker-count reduction is explicitly not in this plan. It is a real lever and may still be needed, but applying it at the same time would make it impossible to tell which change fixed anything.
The plan states the criterion it cannot check: CI's load is not reproducible here, so two consecutive green local runs mean the refactor is sound, not that the flakiness is gone. Several consecutive green CI runs are the real bar, and #241 stays open until then.
Ref #241
Five distinct specs failed across two runs of identical code with no overlap between the sets, so which test fails is decided by the scheduler. The cost is already being paid: #245 skipped the only end-to-end check that adding an item reaches the database, purely to get main green.
The design separates two mechanisms that had been treated as one. Unchecked lookups into shared collections — `collection.find(...)` dereferenced immediately, found at eight or more sites — are a defect regardless of concurrency: when the row is missing the test dies with "Cannot read properties of undefined" naming test plumbing rather than failing an assertion that says what it wanted. Load-induced timing is the other, and is the larger share of what has actually been observed: three of four local failures and the CI one are assertions in a spec's own browser context that nothing else can touch.
Two claims from earlier in this investigation are retracted in the document rather than quietly dropped. The verification-resend limiter is not a shared axis — it is keyed per customer and every test registers its own — and the suite contains no snapshot-style assertions, so there is nothing to convert to web-first. Both were stated as fact on the issue, and both would have justified work that was not needed.
Per-worker databases are ruled out structurally: every worker talks to one backend on :3000, so isolation there means N backends, not N databases. Worker-count reduction is deliberately deferred rather than taken now, because applying it at the same time would mask whether fixing the defects worked.
The honest limit is recorded too. CI's load cannot be reproduced here on demand, so the timing changes rest on reasoning rather than a red-to-green demonstration, and the success criterion is several consecutive green runs rather than one.
Ref #241
Five tasks: prove sharp installs where it actually runs, the re-encode policy as a pure module, wiring it into the single middleware every upload path already passes through, the backfill over already-stored photos, and the deployment sequence.
Re-encoding rather than deleting tags. Deleting requires knowing every tag that could carry something sensitive, across formats and camera makers, indefinitely; rebuilding the file from decoded pixels leaves nothing that could have been missed. Same reasoning that makes uploadTypes.ts an allowlist.
Format is preserved rather than normalised to WebP. Converting would compress better but changes every stored extension, and therefore item_images.image_path, turning the backfill into a rename with a window where rows point at files that no longer exist. A privacy fix does not need that risk, and the backfill consequently touches no database rows at all.
The backfill is lossy and irreversible, so it reports by default and needs --apply, writes to a temporary file and renames so an interruption cannot leave a half-written image being served, and is idempotent by construction: a file already stripped and already within bounds is skipped rather than put through a second lossy pass. That property is a pure function with its own unit test, because being wrong about it degrades every image a little more on every run.
Two traps the plan handles that the issue only named. An animated WebP read without the animated flag decodes to a single frame and is silently written back as a still, so the flag is set for WebP and only WebP — it changes how resize reads height, which would be wrong for the other types. And sharp before 0.33 has no withExif, which the tests need to build their fixture; on an older version they fail as though the stripping were broken.
Ref #226
Two tasks added to the slice-1 plan, closing the half of the upload gap the middleware ordering does not.
The ordering fix stops a caller with a bad token writing anything. A caller with a working one can still send six eight-megabyte files per request against a limiter that allows twenty requests a window, and nothing checks whether the volume can take it. That volume is shared with the admin upload path, so intake filling it is a shop outage rather than an intake outage.
Task 8 refuses an upload when less than a gigabyte remains, on the admin item routes as well as intake, failing closed because a volume that cannot be measured is not one to assume is empty. Task 9 makes an absent cap mean the bounded default of twenty-five rather than unlimited: the router as written treated omission as "no limit", so the ordinary act of creating a link produced an unbounded one, and a cap that has to be remembered is not a control.
Two larger findings are filed rather than folded in. Re-encoding uploads to strip EXIF and cut stored bytes (#226) touches the shared pipeline and adds a native dependency; a global ceiling with an abuse alert (#227) needs its own state and an email. The EXIF one is worth stating plainly: nothing strips metadata today, so an uploaded phone photo publishes the coordinates it was taken at, at a public URL. That is already true of the admin path and is not introduced here, but this slice widens who can put such a file there.
Ref #220
The first of four slices from the intake design, and the only one that is worth planning in detail yet — the later slices' shape depends on what this one actually produces.
Seven tasks: the schema, extracting the validated image-upload pipeline out of routes/admin.ts so the public endpoint reuses it rather than growing a near-copy of it, token generation and hashing, the admin API for issuing and revoking links, the public submission endpoint, the submission page, and the admin screen.
Three things the plan settles that the design left open or got wrong. The image caps become the constants already in the codebase rather than the 10-photo and 10 MB figures the design invented, because two different caps on one pipeline is a defect waiting to happen. The feature flag is dropped from this slice: nothing is reachable until a link exists, and the flag earns its keep in slice 2 where a paid API call appears. And the link is resolved before multer runs, so a stranger holding a bad token cannot cause a byte to be written to the uploads volume — cleanup afterwards would leave an unauthenticated caller in control of disk churn, and leans on an unlink that a crash between write and delete would skip. That ordering is asserted by a test, so a later reordering fails loudly rather than silently.
Ref #220
`price_cents` stays NOT NULL and gains a default of 80.00. Where the model suggests a price the worker writes it onto the item; where it does not, the default stands.
This is cheaper to build than the nullable design it replaces — the column's type is unchanged, so the fifteen files that read `price_cents`, the cart and the checkout among them, keep working untouched, and the migration adds a default and nothing else.
It also gives up a guarantee. The storefront can now be reached by a price the admin never chose, so the protection moves out of the schema and into the review queue, where it is weaker. 80.00 is a plausible number rather than an obvious sentinel, so a default left unnoticed sells the item instead of announcing itself the way "$0.00" would. `price_source` is added to make that legible: the queue labels a price as coming from the model, the default, or the admin, and marks anything unconfirmed as such at the point of publishing. Publishing unconfirmed remains allowed — that is the decision taken — but it is stated rather than silent.
Publishing still happens only from the queue, and the notification email still carries no publish button.
Ref #220
A named, revocable link lets someone without an account send in photos of one item plus a note; a background worker drafts the listing; the admin is emailed and publishes it deliberately from a review queue.
Most of the lifecycle already exists and is reused rather than rebuilt: `pending` has been the unpublished state since #90 and is already excluded from every public query, the upload path already validates magic bytes against a three-type allowlist, mail already has editable templates and an allowlist guard, and node-cron is already the background-work pattern. What is new is a way in for someone with no admin account, the first LLM integration in this codebase, and somewhere to review a draft.
The design turns on one invariant: nothing reaches the storefront at a price a model guessed. The suggested price lives on the draft and never on the item, the email carries no publish button, and the publish path refuses an item with no price. That is also why `price_cents` becomes nullable rather than defaulting to zero — a sentinel that formats as "$0.00" is the same class of quiet failure as `DEMO_MODE` once being "demo unless the value is exactly false", and nullability makes the compiler enumerate all fifteen call sites instead.
Ref #220
Task 1 leaves 4 tests and Task 2 adds 7, so the running total is 11 rather than 12, and the same off-by-one carried into Tasks 3, 4 and the completion criteria. Caught by the Task 2 implementer, which flagged the mismatch rather than inventing a test to reach the stated number.
Nine tasks, each ending in something independently testable, in an order where every task leaves both screens working. The dimensions are built and unit-tested first, the shell after them, and the two screens are wired last — so the old components stay in place until the thing replacing them is proven.
The two behaviour changes the design accepted are each covered twice: a unit test on the chips that produce them, and an end-to-end assertion on what a person sees. The admin tally reading three for three statuses, and a non-default availability producing a removable chip.
Task 8 carries the deletions, deliberately last. Removing `activeFilterCount` and `hasActiveFilters` turns every remaining caller into a compile error, which is the cheapest way to find them.
Two defects found reviewing the plan against the code rather than against itself: the test fixture omitted `Category.item_count` and would not have compiled, and Task 9 added a page object method that nothing used — `chooseAvailability` and `filterChip` already exist.
The plan also records what must not be read as a regression. Two assertions in the storefront specs fail on every branch because the unpaginated grid cannot render 1,600+ development rows inside Playwright's default timeout, which is #186 and predates this work.
Refs #188
#169 made the filter drawer shared, which was the right first move and not the finish. Per-screen differences are booleans, the bar around the drawer was never shared at all, and the storefront's availability preset sits outside the system because the shared component cannot express "this belongs in the bar, not the drawer".
The design replaces the flags with composition: a screen contributes a list of filter dimensions, each declaring where it renders, how to render it, and what chips it contributes. A screen-specific control becomes an ordinary dimension, appearing in the chip row and counting toward the tally without the shared code knowing what it is.
Dimensions are plain data rather than components or context, for a concrete reason rather than a stylistic one. The drawer sets destroyOnHidden, so its sections are unmounted whenever it is closed — exactly when the chip row matters most. Anything that registers on mount would lose those chips the moment the drawer closed, which rules out the otherwise-idiomatic context-and-children approach.
The tally becomes the number of chips, so the count and the chip row cannot disagree — today they are computed by two routes and agree by coincidence, which the admin already has to correct by hand. Three visible behaviours change as a result, recorded in the spec rather than left to be discovered.
Adding vitest is in scope. The design's value rests on chips() being pure, and the frontend has no unit runner at all, so without one the core of it would ship covered only indirectly and expensively through Playwright.
The spec also records what is deliberately untouched: the filter state, the URL serialisation, the backend, and the two e2e assertions already failing on #186 — which must not be read as regressions from this work.
Refs #188
Records why /orders is a page rather than another modal route. The app has both precedents: /account, /login and /register are in MODAL_ROUTES and render over a backdrop, while /cart and /privacy are ordinary pages. Order history is closer to the cart, a list you read rather than a dialog you dismiss.
A modal at /account/orders was the smaller change and was rejected: it inherits the same 700px width and 70vh scroll cap, so it moves the table without giving it anything. Tabs inside the modal were rejected for the same reason, since they fix the scrolling and leave the cramping.
The doc also records the one part of this that is a fix rather than a move. A failed load currently shows a toast and leaves the table empty, and the toast goes away while the empty table does not, so a customer whose request failed sees exactly what a customer with no orders sees. Loading, failed and empty become three distinct states, with a retry on the failed one.
Per-order detail and server-side paging are written down as deliberately out of scope, so that leaving them out reads as a decision rather than an oversight.
Refs #121
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five findings from a whole-branch review, two of which mattered.
The catalogue boundary could not be recovered by the very controls it exists to keep alive. When the item grid threw, the header, filter chips and Clear filters stayed mounted — that was the point — but clicking Clear filters only changed the URL. A boundary does not reset on a client-side state change, so the fallback went on rendering over a catalogue that would by then have loaded perfectly well, and the only offered action reloaded the same failing URL. The shop read as permanently broken, which is the failure this whole change exists to prevent, reintroduced one level down. ErrorBoundary now takes an optional resetKey and clears itself when it changes; the catalogue boundary passes the filter key. The page and modal boundaries deliberately do not take one, because their escapes are hard navigations that remount the tree already — recorded on the prop so nobody completes the pattern by symmetry.
The client-error endpoint could fill the disk. It is unauthenticated, each accepted report wrote about 8.7 KB, and Docker's default json-file driver has no size cap — so the rate limiter bounded a render loop, as its comment claims, but not a few hundred cheap source addresses. Stack and component stack now truncate at 1000 rather than 4000, which is still around fifteen frames and cuts the worst case to under 3 KB, and the QA compose file caps and rotates the log. Production is a Portainer stack outside this repository and needs the same option applied there; noted in the design doc rather than left implied.
Three smaller things. A falsy thrown value defeated the boundary entirely: throw null is legal, and branching on the error object alone treated it as no error, re-rendered the children, threw again, and would have taken the root down — a blank page, the one outcome this is all here to avoid. The boundary now tracks hasError separately and synthesises a real Error for non-Error throws. The modal fallback announced its title twice to a screen reader, once as the dialog's name and once as the heading inside it, so the redundant dialog title is gone. And the design doc claimed the development-only detail shows the component stack when it only ever showed the message; corrected, with a note that the stack still reaches the server log, which is where it is useful.
Verified after all five: backend lint 0 errors, 144 integration tests, frontend lint 0 errors and 31 warnings, 87 end-to-end tests, all against a freshly created database.
Refs #62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Records the two things the design got wrong. antd's Result renders its title as a plain div, so the design's Result usage and its getByRole('heading') assertions contradicted each other and the tests could never have passed as written — resolved by giving the title real heading semantics rather than by loosening the assertion, because an error page with no heading leaves a screen-reader user navigating by headings nothing to find. And import.meta.env had no ambient declaration anywhere in the app, so the DEV gate did not type-check until vite-env.d.ts was added.
The Vite error overlay risk the design flagged did not materialise.
Refs #62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four tasks, each ending in an independently testable deliverable: the backend endpoint with its own rate limiter, the boundary and fallback components, the three mount points with end-to-end coverage, and the production-gate verification.
Refs #62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found while working the design into a plan: the obvious implementation of the fallback's escape actions is wrong, and the spec was recommending it.
A React error boundary does not reset when the route changes. The original spec justified placing the root boundary inside BrowserRouter on the grounds that the fallback needed router context to offer a way back — but a fallback offering a Link would change the URL and go on rendering the fallback, which reads as the app being permanently broken rather than recovering. Every escape action is therefore a hard navigation: reload, or setting window.location.href. The placement is unchanged, but it is now justified by what the boundary guards rather than by reasoning that does not hold.
Three consequences recorded while there. The three fallbacks get distinct titles rather than one shared string, so a customer learns which part failed and the tests get an unambiguous locator for which boundary caught. The modal throw trigger mounts as an unconditional sibling inside its boundary, so /?boom=modal exercises it with the storefront behind rather than depending on /account resolving a session first. And a fourth end-to-end test asserts the report actually reaches /api/client-errors by observing the request, rather than trusting the reporter was called.
Also recorded: new files use antd/es deep imports, this project's documented convention — not antd/lib, which #65 notes loads a second React context and breaks ConfigProvider.
Refs #62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Settles the four questions #62 left open, and records why each alternative lost.
Three mount points rather than one: root, the catalogue, and the modal block. The catalogue boundary is the one that earns its keep, because the likeliest throw in this app is a component rendering API data and the item grid renders the most of it per page — containing it there keeps the header, cart badge and filters alive instead of handing the customer one dead page. The modal boundary exists because the modal-route arrangement couples two independent trees: without a boundary between them a throw in Account blanks the storefront behind it, and a throw in the storefront takes the open modal with it.
Errors get reported to a new POST /api/client-errors that logs and returns 204, with no storage. A boundary that only shows a message leaves nobody knowing it happened, which is the exact failure shape this project has designed against three times already. A persisted store with an admin screen was rejected as a subsystem larger than the rest of the issue.
Rate limiting needs its own limiter rather than the existing one. rateLimit.ts already documents that passwordResetRequestLimiter is keyed on caller and email, and that reusing it where there is no email collapses every caller into one shared bucket — so this endpoint gets a separate limiter keyed on req.ip, which is the real client address because trust proxy is already set.
Recorded as rejected: an outermost boundary around the providers, which would sit outside ConfigProvider and need a second hand-styled fallback for a case that is remote — their render bodies are state and JSX with no data mapping. Flagged for revisiting if that stops being true.
Also recorded: the rate limiter is deliberately not asserted in the integration suite, because its store is process-wide and a test that exhausts the allowance leaks into every later test keyed on the same address.
Refs #62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Records what #61 still needs after #67 delivered two of its four asks, and the two constraints that shape the rest: backend route logic is covered only by the integration suite, which is manual-only because of a post-run hang, and the frontend has no unit tests at all so its coverage has to come from instrumenting the app and collecting from Playwright.
Also records the thing most likely to mislead later — end-to-end coverage marks a line covered when the browser merely ran it, so the frontend number will read considerably better than the testing behind it, and the 80% gate will be easier to clear on frontend changes than backend ones. Accepted deliberately, because the alternative leaves every frontend pull request failing a gate it cannot satisfy.
Refs #61
TypeScript's strict mode checks types and nothing else, so nothing enforced the React hook rules, the SonarJS rules, or unhandled-promise detection. Adds a flat config per workspace, a lint script in each, and a lint job in tests.yml.
The rule selection is the substance of this change and is measured rather than guessed. A full-strength config reports 435 violations across 50 files, but 325 of those are the no-unsafe-* family from recommendedTypeChecked, every one downstream of pool.query() returning any rows and untyped fetch responses. Typing those boundaries is the whole of #65, so enabling the rules here would ship a linter whose output is three-quarters another issue's backlog — the reliable way to teach everyone to ignore lint output. This enables recommended plus the two type-aware rules that catch defects rather than describe type debt, which leaves 110 findings.
Both configs downgrade every preset to a warning and then list the error rules explicitly at the bottom, so the CI gate is readable in one place instead of inferred from four presets' defaults. Errors are no-floating-promises, no-misused-promises, rules-of-hooks, exhaustive-deps and jsx-a11y/alt-text; everything else warns. No --max-warnings flag is needed because ESLint already exits non-zero on errors and zero on warnings. no-misused-promises runs with checksVoidReturn.attributes false, since onClick={async () => ...} is idiomatic React and safe when the handler catches its own errors — at the default it flags every antd button in the admin screens, 25 of its 28 hits, and a rule that is 89% noise gets switched off within a week.
The 37 errors this surfaced were mostly not the mechanical fix they looked like. The plan assumed the 30 floating promises were fire-and-forget loaders that already handled their own failures, which was true of the one sampled when writing the design and false for most of the rest: Admin, Categories, Customers, Tags, Settings, Account and CustomerAuthContext all had no rejection handling at all, so `void` on them would have hidden real failures rather than annotated deliberate ones. Each of those loaders now catches and surfaces the failure before the call site voids it. The CustomerAuthContext one was a live bug — a rejected fetchMe left loading true forever, rendering as a permanent spinner instead of a signed-out page.
Admin's load became a useCallback so its effect can name it honestly rather than suppress the dependency, Categories' drop handler was split so the function antd receives returns void as its type says, and Cart's effect now names refreshCartContext, which is a useCallback with an empty dependency list and so cannot re-run it. The only disable added is in asyncRoute, where returning a promise where Express expects void is the entire point of the wrapper and the promise cannot reject.
Two of the issue's premises did not survive measurement, both recorded in the spec: exhaustive-deps flags 2 cases rather than the 10 inferred from empty dependency arrays, and the backend was already clean on the defect rules because #59 wrapped every async route.
Verified: lint, build, 78 unit, 134 integration and 83 e2e all pass in both workspaces, and the CI gate was confirmed to fail by introducing a deliberate violation rather than by assuming the job is wired correctly.
Closes#60
Hard-wrapped at 100 columns, which assumes a viewer width neither Gitea's web UI nor the VS Code markdown preview has, so the wrap points landed mid-sentence for the person reading it. Paragraphs and list items are now one line each; tables, code fences and the header block keep their own breaks because those are structure rather than wrapped prose.
Refs #60
Records the measurement the design rests on — 435 violations from a full-strength config, of which 325 are the no-unsafe-* family — and the decision not to enable recommendedTypeChecked, since those 325 all trace to untyped pool.query rows and fetch responses, which is the whole of #65. Also records two of the issue's premises that measurement contradicts: exhaustive-deps flags 2 rather than the 10 the issue inferred from empty dependency arrays, and the backend is already clean on the defect rules because #59 wrapped every async route.
Refs #60
The wireframes behind the storefront filter layout decisions lived only in
.superpowers/brainstorm/, which is gitignored, so they were lost to anyone
reading the repo.
That directory stays ignored — it also holds a brainstorming-session token,
PID files, and absolute local paths, none of which belong in the repo. The
mockups themselves are design artifacts, so they are copied into the specs
directory and wrapped as standalone pages: the tool serves them as fragments
inside its own frame, so its style tokens and toggleSelect helper are inlined
to make them open in a browser with no server and no network.
Both rejected layouts and the rejected mobile variant are kept alongside the
chosen ones — the comparison is the part worth preserving.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Storefront gains a Filters drawer holding the category tree, colour-coded
tag pills, and a price range, with applied filters shown as removable
chips. Filter state lives in the URL query string, so a filtered view is
shareable and the back button works. Item cards now show their category
and tags.
Admin gains Categories and Tags tabs, and the item form gains a category
TreeSelect plus a tags Select that creates new tags on the fly.
The admin category tree tracks expansion in state rather than using
defaultExpandAll: that prop is evaluated once at mount, so a branch added
afterwards rendered collapsed and its children were unreachable. Creating
or moving a node now expands its parent. Caught by the new admin e2e spec.
The chip row is marked as a named group so its "Clear all" stays
distinguishable from the drawer's.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records the resolved requirements for issue #23: manual category tree
(no rule engine), single category per item with descendant matching,
central tag registry with hashed-then-overridable colours, AND semantics
for multi-tag filtering, and a drawer-plus-chips storefront filter UI.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bermudalamb
