mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
Zachary Lowden
c56e46ab90
* feat(moderator): a /feedback triage surface for the Feedback table The onsite feedback prompts have been writing to a table nothing reads. Three of the four statuses its CHECK constraint allows were unreachable by any code path that existed. This is the reader. - `/feedback`: one page, status + area filters in the URL, keyset paging, and an inline `?open=<id>` detail panel. - Context is rendered as a CLAIM, not evidence. `context.path` is a bare pathname, so the panel rebuilds the URL from `path` + `filters` (omitting the store defaults) — `path` alone links to a different page than the one the report is about. - The Faro session id becomes a Grafana Explore link, and only while the rows still exist: past the 72 h Loki retention there is no link and an explicit expiry note, because "no logs found", "this session produced no telemetry" and "the link is broken" are three facts with one observable. - Two permissions, `feedback.status.set` and `feedback.bug.promote`, composed with the page grant rather than welded to it. - Promoting mints a `Bug` with `publishedAt` NULL and `content` NULL, so a reporter's words cannot reach the public board and no unsanitised text reaches an HTML-rendered field. Both writes are scoped on the state the operator was looking at — the triage UPDATE on the status they saw, the link on `bugId IS NULL` — and zero affected rows is a 409, never a success. The `20260911120000_feedback_triage` migration is merged but NOT APPLIED to any environment; a human applies it per environment as every migration here is. This commit is the `schema.full.prisma` model edit plus the regenerated files. * fix(moderator): resolve the /feedback review findings Three review agents over the segment, then two delta audits over the fixes. The defects, in the order they cost an operator something: - Attachment ids were rendered through `getEdgeUrl`, which returns its argument VERBATIM when it starts with `http`/`blob` — and the producer bounds them by length only. A reporter could point a moderator's browser at any origin and collect a read receipt naming who opened their report. `splitContext` now requires a Cloudflare-key shape; a rejected value is shown as text rather than dropped. - A duplicate id in that same client-supplied array THROWS `each_key_duplicate` in production, making a report permanently unopenable. Deduplicated. - The triage note was blanked on every save and the next click destroyed it. `update()` resets the form before `invalidateAll`, and Svelte does not rewrite a bound value that has not changed — so the box sat empty over a column that still held text. `FormState` grew a `reset` option; the default is unchanged for its other 37 call sites. - Two refusals had no surface at all: the panel is unmounted before the message is assigned whenever the row leaves the view, and a no-JS submit has no panel to begin with. A page-level alert now covers exactly the cases the panel cannot, and both panel alerts sit outside the branch that the reload flips. - "Attach to an existing issue" 404'd on an issue created seconds earlier — the existence check read the replica. It reads the primary, as does the check that decides whether a refusal says "already linked" or "that report is gone". - An empty attach form refused with "Give the issue a title" over a form showing no title field. The mode is posted explicitly instead of inferred. - The canonicalising redirect fired on the action POST too, so a no-JS client would re-run the action and be told a save that worked had conflicted. Also: `//host` rejected in the reconstructed URL, the keyset cursor parsed once in the query schema rather than twice across two layers, `MAX_INT4`/`isInt4Id` and `clearPaging` taken from their canonical homes, `issuesUrl` given one definition, the sibling row type exported rather than hand-copied, `$bindable` filter controls moved to function bindings, and the page split into a filter bar, a detail panel, a context panel and a promote panel. Tests: 33 added over the round, including a `load` suite that pins the GET-only redirect and the cursor bound, and a PGlite case for the promote path's rollback. * fix(moderator): blind-audit findings on the /feedback queue Three comments asserted a mechanism SvelteKit does not have. They said a refusal re-runs `load` before `FormState` assigns its error, unmounting the panel. In `@sveltejs/kit@2.66.0` both the reset and `invalidateAll()` sit inside `if (result.type === 'success')` (`runtime/app/forms.js:99-107`), so no reload happens on any refusal. The behaviour was right and the reason was invented — and it is the sentence a maintainer would use to delete the panel-level alert as redundant, which is exactly the defect an earlier round introduced. Corrected to what is true: the page-level alert is the NO-JS surface, the panel alert is the JS one, and the two cover disjoint paths. Where hoisting those alerts out of their branch chains no longer has a reason, they now say so rather than carrying a replacement rationale. - The keyset cursor was untested at any page size but one, so the mutant returning `items[0].id` instead of the last row's SURVIVED a green run — at `FEEDBACK_PAGE_SIZE` 50 that repeats 49 rows and strands 49. The fixture now seeds three rows and pages at two, where the first and last of a page differ, and the mutant dies on `expected 3 to be 2`. - The page 500'd for the one role that could already reach it. `allows()` short-circuits for `moderator:admin`, so the nav entry and its badge are live before any `/admin` tick, and the badge counts on `status` alone — a real number against an unmigrated database, then 42703 out of `load` on every click. It now catches that one code and says which migration to apply. The migration header claimed the opposite and is corrected; it is the sentence someone would pick a deploy order from. - `handledById` NULL rendered two different wrong ways: the detail view printed "Handled by #null" for a deleted account, and the list called a live account with no username "deleted account". Two independent nullables, one helper. - A row reopened to `new` kept its `bugId`, which put it in the unhandled queue showing the linked-issue panel instead of the promote form — and `linkInTransaction` refuses any row whose `bugId` is set, with no unlink control anywhere. The link is now cleared alongside the handler columns, for the same reason: `new` means untriaged. Also: `reset: false` on the promote form, whose hidden `id`/`mode` inputs a reset would blank; sibling links drop the keyset cursor so they cannot land on "not in this view"; the copy button returns to idle. Two things are recorded rather than fixed. The list read stays on the replica — it fails closed and matches every other queue here — with the cost written down at the call site. And the PGlite tier binds `dbRead` and `dbWrite` to one client, so no test pins which of them a call site uses; that is now stated in the suite instead of being implied by a green run. * fix(moderator): stop clearing bugId, and narrow the 42703 degrade Reverts the `bugId` clearing added last round, and corrects a claim that had been repeated three times without anyone tracing it. THE REVERT. Clearing the issue link when a row returns to `new` traded one bad state for a worse one. `handledById`/`handledAt` record WHO ACTED, which a reopen retracts; the link records that this report is ABOUT that issue, which a reopen does not make false. Clearing it destroyed that with no way back — the number is stored nowhere else, `ModActivity` has no column to put it in, and `getSiblingFeedback` silently dropped the row from every sibling's list — and left a `Bug` with nothing pointing at it, the state `promoteFeedbackToBug` runs a transaction rollback rather than create. Two comments in one file disagreed about whether that matters; the existing rule wins. What that leaves open is recorded rather than papered over: a linked row can never be re-linked, because the promote path requires `bugId IS NULL`. That is a missing unlink control, it is missing at every status, and reopening is not a sensible back door to it. Not added here. `docs/moderator-app/` §5 now states the rule and the gap, so the doc and the code agree. THE FALSE CLAIM. A test comment said the surviving cursor mutant "makes 49 unreachable". It does not. Walked against the harness: seven rows at `limit: 3` page 7-6-5 / 6-5-4 / 5-4-3 / 4-3-2 / 3-2-1 — every id still reached, each turn repeating all but one row. The cost is a queue that drains 50× slower, not one with holes in it. The fixture's justification is unchanged, which is the point. Also: `isMissingTriageColumns` now requires the message to name one of the four columns that migration adds, not merely the `42703` code. The page answers with one specific instruction, and `42703` is raised by any absent column — so the code alone would give that instruction to an unrelated typo while the `catch` suppressed the error that would have named it. Matched on the column name rather than on "does not exist", which does not survive `lc_messages`. And the degrade is now exercised end to end: a new pre-migration PGlite fixture runs the real query against the real pre-migration table and asserts on whatever the driver actually raises, rather than on a hand-built `{ code }`. Measured: `code` is top level and the message is `column f.handledById does not exist`. * docs(moderator): record the frozen-link gap at the guard that enforces it Round 3 of the audit ladder: the note explaining why a linked row can never be re-linked sat inside triageFeedback, 150 lines from linkInTransaction's `bugId IS NULL` clause -- which is what a contributor adding an unlink control would actually open. Cross-referenced, and the ON DELETE SET NULL cascade named so it is not mistaken for a control. Also replaces a count in a test comment with "every other", after the count drifted when this round added a case. Fix the form, not the number. Comment-only: no executable line changes. * docs(moderator): qualify the error-shaped claim, and drop a wrong lesson Round 4: "Every OTHER case constructs { code, message }" was false -- the last case in that block deliberately feeds non-error values. Qualified, and the qualifier is now called out as load-bearing so it is not trimmed later. Also drops the parenthetical claiming the earlier count had drifted. It had not: traced the block across the three fix commits -- 3 cases at9416bfd5, 5 atb58b8887, where the comment said "both other" while there were already four others. The count was wrong when written, not later. Removed rather than replaced with a third theory about it. Comment-only: no executable line changes.