mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
0dbe0a6bfea75bda90f5ef017b4111eb6183941b
25696 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Zachary Lowden
|
0dbe0a6bfe |
F5b: bound the model-slot App Block iframe to the viewer's viewport (#4589)
* fix(app-blocks): bound the model-slot iframe to the viewer's viewport (F5b)
`IframeHost.applyHeight` had three layers of height defense: the
isFinite/positive value guard, `manifest.iframe.maxHeight`, and
`HARD_HEIGHT_CEILING` (8000px).
The gap is layer 2 being optional. `public/schemas/app-block/v1.json` types
`iframe.maxHeight` as `["integer","null"]` ("null for unbounded") and `iframe`
declares no required fields at all, so a manifest that simply omits it is
bounded only at 8000px. A block self-reporting 3000px therefore got a 3000px
iframe inside a ~640px phone viewport, and the inline slot swallowed the page.
Adds layer 4:
next = Math.min(next, Math.max(min, viewportHeight));
The iframe may never exceed the visible viewport height, but the manifest's
`minHeight` still wins if the viewport is somehow shorter than it. The block
scrolls internally instead, which is the intended outcome.
Two properties the shape alone does not give you:
- the viewport is read at CALL time, never captured at mount, so a rotate or
a browser-chrome resize cannot leave a stale bound in place;
- the block's own STATED height is stashed in a ref and the clamp is
re-applied on `resize`, so the bound tracks the viewport in both
directions. Host-side only — nothing is posted back to the block, which
is never asked to re-measure (RESIZE_IFRAME is one-way).
A viewport that cannot be measured (SSR, or an `innerHeight` that is not a
positive finite number) means DO NOT CLAMP, never clamp-to-zero: a failed
measurement degrades to the pre-existing three-layer behaviour.
Surface: the inline model-page slot only. `IframeHost` is the only
RESIZE_IFRAME consumer in `src/`; the full-page host `PageBlockHost`
(`/apps/run/<slug>`) has no RESIZE_IFRAME handling and is already
viewport-bound by its own `calc(100dvh - HEADER_HEIGHT_PX)`. It is untouched.
Tests: `IframeHostViewportHeightClamp.browser.test.tsx`, 6 cases, all at an
explicitly-set phone viewport. The harness default is 414x896, at which every
height the neighbouring IframeHost suites assert (640/700/800) is already under
the bound and the clamp never fires — so each test calls `page.viewport(...)`
first and then re-reads `window.innerHeight` to prove the value took.
red at origin/main (
|
||
|
Zachary Lowden
|
f7d558ce8b |
fix(models): bump Model."updatedAt" when restoreModelById un-deletes a model (#4608)
* fix(models): bump Model."updatedAt" when restoreModelById un-deletes a model
restoreModelById un-deletes with raw SQL, so Prisma's @updatedAt does not
fire. A restored model therefore lands in Draft carrying the timestamp it
had before it was deleted -- by construction older than its own deletion.
remove-old-drafts reaps
status IN ('Draft','Deleted') AND m."updatedAt" < now() - INTERVAL '30 days'
and cascade-deletes the model with its versions, files and training data,
irreversibly. So a model restored today is eligible for that cascade
tonight, with only downloadCount < 10 and the job's activity fences between
it and destruction.
Adds "updatedAt" = now() to the statement. Restoring a model is a write to
the row, so the bump is what the column is supposed to mean as well as the
thing that stops the reaper eating it.
Same defect class and same one-token fix as #4595, which closed the two
sweep sites and named this one as an unfixed sibling. This is the third and
last raw-SQL-into-Draft site in the tree.
Coverage:
- restore-model-updated-at.service.test.ts (new) drives the real
restoreModelById through the canonical dbMock and reads the statement it
actually issues: the bump is present, assigned exactly now() and
unconditional; the SET clause is pinned as an ordered LEDGER so an added
column write is caught too; the deletion-column clears, the status CASE
and the status = 'Deleted' predicate are pinned so the fix has to be
additive; and a scope guard holds the change to Model."updatedAt",
leaving ModelVersion alone.
- no-unbumped-draft-status-write.test.ts gains this file, and the exclusion
note explaining why it was left out is removed. Its extractor had to be
widened first: it anchored on `UPDATE "Model" ` WITH a trailing space,
which cannot see a statement that breaks the line after the table name,
and it took only the FIRST match per file -- model.service.ts holds three
raw UPDATE "Model" statements and only one of them drafts. It now reads
every such statement in a file and binds the rule to the ones writing
Draft.
* docs(models): correct the restore-clock claim — it is the deletion instant
The comments said the restored row carries a timestamp "older than its own
deletion". That overstated it. deleteModelById writes through the Prisma
client (tx.model.update with a nested modelVersions.updateMany), so
@updatedAt DOES fire at deletion time and the frozen value is the deletion
instant itself.
Measured rather than assumed: an enumeration of every raw UPDATE "Model"
in src/ finds the restore statement is the ONLY one that touches a Model's
'Deleted' status. Every other delete path is a client write.
The defect and its severity are unchanged, and the precise statement is
sharper than the wrong one: a model that sat deleted for longer than 30
days is past remove-old-drafts' threshold the instant it is restored. The
job's activity fences do not save it either -- they read ModelVersion
timestamps, which the same Prisma delete froze at the same instant, so both
the age test and the fence expire together.
Comments only. No behaviour or assertion changes; the same 14 tests pass.
* docs(tests): correct the statement count and state why the ledger extractor was narrower than its rule
Two corrections to the guard's own doc comment.
COUNT. It said model.service.ts holds THREE raw UPDATE "Model" statements.
It holds SEVEN. The three came from grepping the SPACE-ANCHORED spelling --
the very anchor the rewrite exists to remove -- and then reading that count
as the true one. Re-derived by enumerating every occurrence and slicing each
to the end of its tagged template: lines 1929, 2244, 3008, 3111, 3706, 4356,
4992, of which only 1929 drafts.
MECHANISM, which the wrong count was hiding. The drafting statement is the
FIRST of the seven, so a first-match rule on its own would have found it. It
is the space anchor that skips it, and only then does first-match land on
captureMinorFlagSnapshot's meta write. The two defects COMPOSE; fixing
either half alone yields a guard that is right for the wrong reason.
And why no review round could have found it: both files the extractor was
originally written against hold exactly ONE raw UPDATE "Model" each, and
both spell it with a trailing space because of the table alias. Both
unstated assumptions were true of the entire corpus the guard could see, so
a doc comment claiming a general rule over an implementation covering two
accidentally-conforming files is indistinguishable from a correct guard by
reading it, running it, or auditing it -- only by pointing it at a file it
was not written for.
Comments only. Same 14 tests pass.
* fix(tests): make the ledger extractor nesting-aware and pin an exact drafting-statement count
Two problems, neither in shipped behaviour, both in what the guard could see.
TERMINATOR. The extractor read each statement to "the next backtick". On a
template whose interpolation is itself a tagged template -- the shape of the
description write in model.service.ts -- that backtick is the NESTED one, so
the extraction TRUNCATES. Measured: that statement extracted 156 chars and
stopped mid-ternary; it now extracts 207. Truncation REMOVES text, so it can
drop the 'Draft' literal and silently take a statement out of scope. The
comment claiming this "can only pull MORE text into scope, so it fails
closed" described the opposite of the real failure mode and is gone.
templateEnd now tracks ${...} by brace depth, recursing through nested
templates and skipping quoted strings in the JS. An unterminated template is
an ERROR rather than a skip.
CONTROL. "At least one drafting statement per file" is only sufficient while
every ledger file holds exactly one -- a property of this corpus, not of the
guard. With two, one can drop out of the extractor's view and the file still
reports a non-empty list. The ledger now pins the EXACT count per file, which
fails both when a statement disappears and when one arrives unwatched.
Proof, since this is the case the single-statement coincidence hides. A
second drafting statement planted in model.service.ts:
- plain spelling, unbumped, count=2 -> KILLED, "drafting statement #2 does
not bump"
- plain spelling, bumped, count left at 1 -> KILLED by the count control
- NESTED-TEMPLATE spelling, unbumped, count=2 -> KILLED, "#2 does not bump"
That third case is the one that matters: under the old terminator it
extracted as 'UPDATE "Model" SET ${Prisma.sql', carried no 'Draft', dropped
out of the drafting set entirely, and the old "at least one" control passed
-- silently green over an unbumped drafting statement.
Full sweep re-run against THIS head, not carried forward: 11 mutants across
both batteries, all killed, 2 controls green. typecheck 0 errors.
* docs(models): rewrite the bump's justification from the reaper's predicate
The stated exposure was unreachable, and this is the THIRD wrong framing of
it: "a clock older than its own deletion" (wrong), then "carries the deletion
instant, reapable the instant it is restored" (wrong -- it was ALREADY
reapable). Each was written from a story about what restoring does. This one
is written from what the predicate reads.
remove-old-drafts selects status IN ('Draft','Deleted'), so 'Deleted' is
already in the reaped set and the clock runs while the model sits deleted: a
low-download model is destroyed the night after deletion + 30 days, still
Deleted, never restored. Restoring changes exactly one term -- status goes
Deleted -> Draft (still in the set) or Unpublished/Scheduled (out of it).
Nothing else moves: the ModelVersion statement is raw SQL and does not bump
mv."updatedAt" either, and downloadCount / availability / the ModelMetric
join are untouched. The post-restore candidate set is a strict SUBSET of the
pre-restore one, so without the bump restoring can NEVER make a model
reapable that was not already. The "restore it and it dies that night"
scenario cannot occur: surviving months of nightly reaping requires a sparing
condition, and every one of those still holds after the restore.
The two real justifications, both previously absent:
1. A PARTLY-SPENT CLOCK. Deleted day 0, restored day 29 -> reaped the night
of day 30/31, one day after restore. The bump turns the remainder into a
full REAP_AGE_DAYS.
2. THE old-draft WARNING, arguably the stronger reason. It warns on Draft
ONLY -- a Deleted model is deliberately never warned -- and its band
evaluates ONCE at U + OLD_DRAFT_NOTICE_DAYS, never re-evaluated for that
U. A model restored with its old U is Draft, so warnable for the first
time, but its band is already past: cascade-deleted UNWARNED. The bump
re-arms it.
Also corrected in passing: "the same delete froze the ModelVersion timestamps
at the same instant" was imprecise. deleteModelById's nested
modelVersions.updateMany is scoped to status IN (Published, Scheduled), so a
Draft-only model has NO version row bumped at delete. Those timestamps are
OLDER, which makes the fences less protective, not more -- conservative, so
the conclusion held, but say it accurately.
Every claim above re-derived from source in this tree: the predicate in
remove-old-drafts.ts, the constants in draft-reaping.ts (REAP_AGE_DAYS 30,
OLD_DRAFT_LEAD_DAYS 7, OLD_DRAFT_NOTICE_DAYS 23), the Draft-only status term
and once-evaluated band in model.notifications.ts, and the updateMany scope
in deleteModelById.
Why it matters that this is right: a maintainer who tests the old scenario
finds no such model, concludes the justification is fiction, and deletes a
line that is load-bearing for both cases above.
Comments only. Same 67 tests pass.
* docs(tests): name the phantom-statement cause, split the limits list by failure direction, narrow the unwarned claim
Three comment/message findings from the round-2 delta audit. Zero executable
change: the only non-comment lines in this diff are two assertion-message
strings, both purely descriptive.
1. THE COUNT CONTROL'S MESSAGE POINTED AT THE WRONG FIX. modelUpdateStatements
anchors on the raw text UPDATE "Model", so a comment or string that merely
QUOTES the statement counts as one. It fails closed, which is right, but the
message said "more means a new drafting site arrived -- bump the count", and
a maintainer following that literally would install a phantom in the ledger
permanently. Not hypothetical: the comment block on restoreModelById already
contains 'Draft' and "updatedAt" = now(), so it is one mention away from
tripping this, and prose about this guard is exactly what would do it. The
message now names both causes and says to REWORD THE PROSE rather than raise
the count. Watched it render: planting the audit's exact phantom comment
turns the guard red with the new text.
2. THE LIMITS LIST MIXED FAIL-CLOSED WITH FAIL-OPEN. Regex literals and //
comments inside an interpolation fail CLOSED -- the scan runs long or errors.
An UPDATE "Model" nested inside another one's interpolation fails OPEN: since
templateEnd now correctly reads to the outer close, the inner statement is
absorbed rather than enumerated. Measured: outer bumped, inner drafting and
unbumped gives drafting=1, bumped=1, exact count 1, all green. Deliberately
not engineered around -- nobody writes that shape, and a parser change costs
more risk than it buys -- but it is now listed, with each limit labelled by
the direction it fails. A limits list that mixes the two is worse than none.
3. "CASCADE-DELETED UNWARNED" WAS WIDER THAN THE MECHANISM. A model deleted day
0 and restored day 10 is Draft at day 23 with U = day 0, so the band DOES
match and it IS warned. The unwarned outcome needs the restore to land after
U + OLD_DRAFT_NOTICE_DAYS. Both branches are now spelled out. The
justification is unchanged; the sentence was over-wide, and this specific
comment tells the next editor not to justify the line from a story, so it has
to meet its own bar.
Also: earlier commit messages on this branch quote bare test counts ("Same 67
tests pass", "Same 14 tests pass") with no file set, which cannot be checked as
written. Not rewriting pushed history under a passed audit, so the correction
is here: this change is green at 124 tests / 8 files over exactly
services/__tests__/{no-unbumped-draft-status-write,
restore-model-updated-at.service, no-lint-rules-script-drift,
no-direct-shared-module-mock, no-io-in-transaction}.test.ts
jobs/__tests__/{reset-to-draft-without-requirements,remove-old-drafts}.test.ts
notifications/__tests__/old-draft-reaper-parity.test.ts
No new mutation sweep: nothing executable moved.
|
||
|
Zachary Lowden
|
56e534b101 |
fix(ui): Tooltip must wrap Menu.Target, or the menu never opens (#4610)
* fix(ui): Tooltip must wrap Menu.Target, or the menu never opens Menu.Target clones its child to attach the toggle handler and its ref; Tooltip also clones ITS child and overrides the ref. So nesting the Tooltip INSIDE Menu.Target hands the menu a ref to nothing and the trigger silently stops opening the dropdown -- nothing throws, nothing warns, the tooltip still works and the button still highlights. Six sites had the broken order, written independently: Comics/ComicExportButton.tsx Generation/Input/DrawingEditor/DrawingToolbar.tsx ImageGeneration/GeneratedImageActions.tsx generation_v2/preset/PresetHeaderButton.tsx pages/comics/[id]/[[...slug]].tsx (x2, both moderator menus) Each is a nesting inversion only -- labels, props and handlers are unchanged. The reference implementation and the measured rationale are in Apps/AppListingActionsMenu.tsx, which already had the correct order. Adds no-menu-target-tooltip-nesting as a convention guard so the seventh site cannot ship the same way. It runs in the `unit` project, which is the tier that can block a merge; browser-mode tests here run only in the report-only preview pipeline. The guard carries its own positive controls (it must still recognise the CORRECT shape in the real tree, so a scanner wired to nothing cannot pass as zero violations) and states its limits in a scope note -- it is a source scan, so it cannot see the same defect reached through a variable or a wrapper component. Registered in test:lint-rules and both guard lists, per no-lint-rules-script-drift. * fix(test): report guard line numbers against the real file Blanking comments to spaces instead of deleting them keeps every byte offset intact. Deleting shifted every line after a block comment, so the guard named ComicExportButton.tsx:206 for a defect on line 210 -- a file:line that reads as authoritative and sends the reader to the wrong place. Found by running the guard against the pre-fix tree and checking the six reported lines against the files. * style: drop unrelated prettier churn from the three touched files Running prettier --write over the whole file reformatted lines that have nothing to do with the nesting fix (import wrapping, ternary collapsing, a function signature). Prettier is report-only for modified files here, so the churn bought nothing and made the diff harder to review. These three files now carry the nesting inversion and nothing else. |
||
|
briant
|
b7fd0d0685 | chore(moderator): release moderator-v0.0.55 moderator-v0.0.55 | ||
|
Zachary Lowden
|
3d4f44a89a |
docs(AppBlocks): stop calling the node unit tier a merge gate — it is not one (#4607)
Comment-only. No behaviour change, no test logic touched.
Measured ground truth:
* `.github/workflows/lint.yml:405` sets
`continue-on-error: ${{ github.event_name == 'pull_request' }}` on the `unit`
job. That workflow's `on:` also includes `push: branches: [main]` and
`workflow_dispatch`. Line 377 of that same file already says so:
"`conclusion` is USELESS here: `continue-on-error` makes it `success` while
the suite underneath is broken."
* `gh api repos/civitai/civitai/branches/main/protection` reports
`has("required_status_checks")` == false, and the `required_status_checks`
endpoint 404s with "Required status checks not enabled". The only ruleset on
`main` is a `deletion` rule.
So NO status check is required on `main`, by either mechanism. On a pull request
the node `unit` tier is REPORT-ONLY. It renders an honest verdict only on a push
to `main` or a `workflow_dispatch`. A red run is a signal a human must read — it
is never a merge blocker.
Across `src/components/AppBlocks/` ~13 comments asserted the opposite: "the
GATING tier", "the copy that can block a merge", "only the gating tier can stop a
merge", plus one test title reading "in the BLOCKING tier". Two files in the same
directory (`pageBlockHostMaxWidth.test.ts`, `ledgerSelectorSurvivesProdStrip.test.ts`)
already carried the corrected wording; this converges the rest onto it.
The distinction the old wording was reaching for is real and is preserved: the
node `unit` project EXECUTES the assertion, whereas the Vitest browser-mode
`component` project is the always-report-only `preview / component-tests` status.
What is false is calling either one a merge gate. The guards themselves are
unchanged and still catch exactly what their headers claim they catch.
Verification:
* node `unit` scoped to src/components/AppBlocks/ — 7 failed | 747 passed (754)
both at origin/main and at this commit, identical failing set (the
pre-existing hiddenBlocks.test.ts localStorage TypeError).
* node scripts/typecheck.mjs — 0 type errors.
* prettier + eslint clean on all 15 changed paths.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
briant
|
1b17cf32e2 |
feat(moderator): evidence reaction abuse in User Lookup
The reactions panel showed one count per creator and told the moderator "concentration on one is the signal". A laugh-react harassment report was provable but not with this panel: the reported target held 64 of 21,840 reactions across 3,296 creators, nowhere near the top ten, and the account was about as unconcentrated as one gets. Volume is not the signal, the reaction MIX is. Each row now breaks out Heart/Like/Laugh/Cry/Dislike with a first-to-last span, and the list carries a second half: creators whose reactions are majority Laugh/Cry/Dislike over MIN_FLAGGED, ranked by negative share. On the account above that surfaces 22 creators the panel could not previously distinguish -- several a stronger pattern than the one reported -- so one report yields the list of everyone it happened to. flaggedTotal reports how many match overall, since rendering the cap as the total would read as the whole victim list. The ranking partitions inside the qualifying set and orders on share rather than absolute count. Ranked over every group by absolute negatives, five browse-heavy creators carrying a few hundred incidental Laughs take every slot and the 38-of-40 target never appears -- the original failure, on exactly the accounts that get reported. A per-minute burst count was built and dropped: it does not discriminate (152 of 3,307 targets had a 20+/minute burst, which is what thumbing through a gallery looks like) and cost 22s against 4.4s. first/last answer the timing question for free. Measured against the previous count-only query, ~150ms at 22K reactions and 4.4s on the heaviest account sampled -- the FILTERs and min/max ride the scan the count already pays for. Addresses panel: the bare "25x" is labelled, the already-queried first-seen is rendered, and the intro no longer describes a registered/subscribed-from filter that belongs to the linked-accounts column, nor claims completeness over a list capped at 100 with ban events excluded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
9dbcf3dd26 |
docs(creator-studio): donation goals as a crowdfunded free month; group monetization docs
A creator thread raised early access being switched to paid access after donations were taken. The refund the thread assumed protects donors does not exist: the 30-day early-access refund covers buyers, on unpublish only, and the gate write path has no refund at all. A met goal does not stick either — endPaidAccessNow only sets endsAt, and the next editor save writes a fresh gate, so a fully-funded goal can be re-gated the same afternoon. Earlier drafts answered this with escrow, a permanent guarantee, or a transition matrix. All three are dropped for four rules: 1. a donation goal requires download access 2. goal met -> free to everyone for 30 days, no purchases during it 3. after that the creator prices it however they like 4. anything without a goal is freely priced; its date is an estimate Rule 1 closes the generation-only escape without a rule about it, and guarantees a met goal hands over something permanent. Nothing is terminal and nothing is refunded, which supersedes both the earlier "both terminal" decision and Q18/Q19 in paid-access-decay.md — flagged there, along with the conflict this creates with its permanent PaidAccessGuarantee (D1). Also recorded: Justin's four PaidAccess thoughts with an assessment each, the price-tags direction that competes with template targeting (review deferred until the DonationGoal keep-or-drop call), and the donation income concentration that argues against a broad poll on it — top 10 earners take 78%, median earner 5,015 Buzz a quarter. Per-creator figures omitted; this repo is public. The eight paid-access, pricing and donation docs move to creator-studio/monetization/. Relative links rewritten both directions and all 759 links under docs/ checked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Briant Diehl
|
74306cc04c |
Merge pull request #4603 from kaydaxter/krea2-custom-checkpoints
krea2 custom checkpoint support |
||
|
briant
|
9d3bc3f420 |
fix(krea2): edit workflow ran community checkpoints on turbo's control set
Removing modelLocked at the ecosystem level unlocked img2img:edit too, where the variant is chosen per-version: any id that wasn't `raw` fell to editTurbo, whose 15-step / cfg-2 ceilings sit below what an undistilled finetune needs. The six finetunes #4602 names are all full finetunes, so on edit they were not merely mis-defaulted but undriveable, with no way for the user to fix it. Fall back to editRaw instead — symmetric with the txt2img fallback this PR already established, behaviour-identical for both official bases, and it adds no new silently-substituting (ecosystem, workflow) pair to the #3520 population. Also: - ask "is this official?" in one place (isOfficialKrea2Version) rather than denylisting raw/turbo in the handler, so a fifth build is one edit - cover the txt2img path: diffusionModel absent on the official builds and present on community ones, plus the FAL size tiers and checkpoint+LoRA - add krea2-graph.test.ts pinning the unlock itself. With modelLocked on, the checkpoint clamp in common.ts rewrote every non-official id before the handler ever ran, and no test observed it — the feature could be reverted green - drop comments this change made false: the header claimed edit picks its variant regardless of version, and training.ts called the checkpoint locked Coverage is still the gate: all 201 community Krea 2 checkpoint models are covered:false today, so generation coverage needs flipping before the #4602 finetunes can actually run or enter auctions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
34510fb587 |
fix(jobs): bump Model."updatedAt" when a sweep flips a model to Draft (#4595)
* fix(jobs): bump Model."updatedAt" when a sweep flips a model to Draft
reset-to-draft-without-requirements ends with a raw $executeRaw UPDATE on
"Model". Prisma's @updatedAt does not fire on raw SQL, so a model this nightly
sweep flips to Draft keeps whatever "updatedAt" it carried while Published --
measured as far back as 2023-10-17 on live rows.
remove-old-drafts reaps status IN ('Draft','Deleted') AND m."updatedAt" <
now() - INTERVAL '30 days' and cascade-deletes the model with its versions,
files and training data, irreversibly. A swept model therefore arrives in Draft
with its entire 30-day grace period already spent. Both jobs share the same
cron minute (43 2 * * *).
Today 1,308 of the 1,309 already-swept models past the clock are held back from
deletion by nothing but the reaper's downloadCount >= 10 term -- an accidental,
undocumented and self-gameable protection. The #4579 activity fence does not
help: a sweep is a system write, not creator activity.
Same bypass, same fix, in the admin backfill that later flips those rows from
Unpublished to Draft.
Deliberately NOT applied to any "ModelVersion" statement: that column means "a
creator edited this version" and the reaper's activity fence reads it as a
creator-activity signal. Bumping it on a system write would corrupt the fence.
Adds src/server/jobs/__tests__/reset-to-draft-without-requirements.test.ts,
including a guard pinning the ModelVersion exclusion.
* fix(notifications): scope old-draft warning to what the reaper will destroy
Audit round on #4595. The Model."updatedAt" bump makes ~1,308 swept models
eligible for the 'old-draft' notification for the first time -- their clocks
were previously too old to fall inside its BETWEEN window.
That notification promises "will be deleted in 1 week" and is toggleable:false,
but its predicate carried only the status and age terms. The reaper also
requires availability != 'Private', ModelMetric."downloadCount" < 10, and two
activity fences, and ~99% of that cohort is held back by the downloadCount term
alone. Warning them would have been false, unmutable, and all in one minute.
Mirrors the reaper's remaining terms into the notification, using EXISTS over
"ModelMetric" to reproduce its INNER JOIN + DISTINCT semantics (a model with no
metric row is not reapable, so it must not be warned). This is an
APPROXIMATION, not a prediction: the notification evaluates at day 23 and the
reaper acts at day 30, so downloadCount can cross 10 and activity can appear in
between. Errors now land on the safe side -- warn, then spare.
The rule is now open-coded in two files, so old-draft-reaper-parity.test.ts is
the seam guard: it asserts the shared terms appear on BOTH sides and that the
fence and 30-day-interval counts match, failing when the set grows or shrinks.
A shared SQL constant was rejected -- the reaper spells the download term as a
JOIN and the notification as an EXISTS, and their windows differ on purpose.
Also in this round:
- Correct the ModelVersion."updatedAt" comment and guard message. The claim
that bumping it on a system write is forbidden was wrong: unpublishModelById
does exactly that, deliberately, because the column is on the public v1
payload. The guard stays -- it pins THIS change's scope -- but no longer
asserts a rule the codebase contradicts, and the false "permanently spares"
reasoning is removed (both clocks expire at the same instant).
- Pin the UPDATE "Model" SET list to exactly its three assignments. Injecting
"publishedAt" = now() survived every previous guard; toContain cannot see an
ADDED column.
- Add no-unbumped-draft-status-write, a convention guard covering the admin
backfill statement, wired into test:lint-rules and both docs that mirror it.
* fix(notifications): resolve old-draft fences to the reaper's window, not its text
Round-2 audit on #4595. The previous commit copied the reaper's three activity
fences into the old-draft notification VERBATIM, including their now() anchor.
The two queries run 7 days apart, so identical now()-relative text is a
DIFFERENT absolute window on each side:
notification fires at U+23d; 'now() - 30 days' resolves to U-7d
reaper acts at U+30d; 'now() - 30 days' resolves to U
The notification's condition was therefore strictly stricter by the lead time.
The canonical abandoned draft -- model, version and file all created at U, which
remove-old-drafts documents as the norm -- has activity newer than U-7d, so its
NOT EXISTS was false and it was EXCLUDED from the warning, then reaped a week
later with no notice. Before this PR that model was warned. The band is ~1
minute wide, so a miss is permanent.
Fixes the interval to resolve to the reaper's window, and derives it rather
than hardcoding it. Adds src/server/common/draft-reaping.ts holding
REAP_AGE_DAYS, ACTIVITY_WINDOW_DAYS, OLD_DRAFT_LEAD_DAYS and the derived
OLD_DRAFT_NOTICE_DAYS = REAP_AGE_DAYS - OLD_DRAFT_LEAD_DAYS. The BETWEEN band,
all three fences and the message copy now come from that one expression.
The module is dependency-free on purpose: model.notifications sits in a graph
that no-server-infra-in-app-graph forbids from reaching src/server/db, so it
cannot import the job. remove-old-drafts re-exports the two it already
exported, so existing importers are unchanged.
The parity guard asserted TEXTUAL identity of the intervals, which encoded the
defect and additionally went RED on the correct fix -- a guard blocking its own
repair (measured: 5 failures). Rewritten to compare RESOLVED windows: it now
simulates the canonical abandoned draft against the numbers both queries
actually carry and asserts the model is warned before it is reaped. Fence-set
and shared-term parity are kept, so R1/R2 coverage is unchanged.
Also adds a control that re-imports the module against a doubled lead and
watches every derived value move -- without it a hardcoded '23' or '1 week'
passes, because those equal the constants' current values. Measured: three
such mutants survived the whole file before it existed.
Also in this round:
- Retract the same wrong ModelVersion."updatedAt" rationale in
backfill-swept-trained-models.ts that was corrected elsewhere last round;
the two sibling files were giving opposite reasons for the same omission.
- Restate the "errors land on the safe side" comment, which was false while the
7-day skew created a reap-without-warn direction. It is true after this fix.
- Widen the ModelVersion scope guard to match unquoted `updatedAt` too.
* fix(notifications): drop the old-draft activity fences; they cannot be sound
Round-3 audit on #4595. The previous commit derived the notification's fence
interval as REAP_AGE_DAYS - OLD_DRAFT_LEAD_DAYS, which is correct only if the
reaper is a one-shot evaluation at U + 30d. It is not: remove-old-drafts is a
nightly cron that retries forever, so it really fires at
max(Model."updatedAt", latest version/file activity) + REAP_AGE_DAYS
while this notification evaluates ONCE, in a ~1-minute band at U + 23d.
So the fences still excluded every model whose version or file landed after the
model row was last written -- which remove-old-drafts documents as the norm,
"the finished resource lands hours or weeks later". Worked example: model row
Jan 1, version and file Jan 3. Notification Jan 24, cutoff Jan 1, activity Jan 3
is newer -> excluded, and never re-evaluated. Reaper first fires Feb 3, cutoff
Jan 4, activity Jan 3 is older -> fences clear -> cascade-deleted, unwarned. On
origin/main that model was warned.
No choice of interval fixes a predicate evaluated at a single instant against a
condition that keeps moving, so the two NOT EXISTS activity clauses are REMOVED
rather than re-tuned. status, the BETWEEN band, availability and the
downloadCount EXISTS stay: every one of the ~1,308 false alarms this predicate
was tightened to prevent came from downloadCount >= 10, so the value terms alone
do the job the fences never contributed to.
The value terms are safe for the reason the fences were not -- they do not
depend on predicting WHEN the reaper fires. downloadCount is monotonically
non-decreasing in practice, so a reading of < 10 at day 23 still holds at day
30; availability is NOT NULL with a Public default, and changing it writes the
Model row, bumping updatedAt and re-arming the band. That reasoning is now in
the comment.
Errors are therefore structurally one-directional: warned-then-spared, or warned
earlier than the message implies. A model the reaper destroys is never silently
deleted.
Reshapes the guard around that property instead of parity, which was the wrong
invariant and passed over both prior defects:
- a structural rule that the query carries NO time-relative activity clause, in
three forms (no now()-relative comparison, no NOT EXISTS, no reference to
"ModelVersion"/"ModelFile" at all);
- a property test parameterised over activity offsets U+0, +0.5, +2, +20, +40 --
the single fixture at U+0 is the one offset where round 3's predicate was
accidentally correct, which is how it passed -- modelling the reap instant as
max(U, activity) + REAP_AGE_DAYS rather than U + REAP_AGE_DAYS;
- value-term parity and the mechanical constants control, both kept.
Measured: the round-2 fence shape fails 8 of 17, the round-3 shape 7 of 17.
* test(notifications): close the old-draft guard with an allowlist; name its exclusions
Round-4 audit on #4595. The payload (no activity fence in the old-draft query)
is unchanged and was confirmed correct. This closes three ways the GUARDS and
the comments overstated what they establish.
1. The structural guards were a DENY-LIST of spellings -- a regex for
`<alias>."col" [<>] now() - INTERVAL`, the literal `NOT EXISTS (`, and the
literals "ModelVersion"/"ModelFile" -- and a deny-list of spellings is
unbounded by construction. Measured: this genuine activity fence passed all
17 tests, because `<=` puts `=` where the regex wants a space and it reads a
real maintained column on a table the guards whitelisted:
AND (mm."lastVersionAt" IS NULL
OR mm."lastVersionAt" <= now() - INTERVAL '23 days')
Two cruder rewrites (`NOT (EXISTS (`, `0 = (SELECT count(*)`) also survived.
Replaced with CLOSED allowlists: the query's table set must be exactly
{Model, ModelMetric} and its qualified column refs exactly the eight it uses.
Any new term now fails until someone adds it deliberately. The two spelling
guards are kept as secondary, for their specific failure messages, and are no
longer relied on as the closure.
2. The parameterised offset test was TAUTOLOGICAL. At HEAD the query has no
time-relative term, so its loop body never executed and the only live
assertion was `U + OLD_DRAFT_NOTICE_DAYS < max(U, activity) + REAP_AGE_DAYS`,
true by construction for any query since NOTICE = REAP - LEAD and every
offset is >= 0. It could not fail, and it missed the walk-past above
entirely. Deleted rather than dressed up -- the allowlist carries that
property. Its worked example survives as documentation on the guard.
(Its reap model was also wrong: the true instant is
max(U + REAP_AGE_DAYS, activity + ACTIVITY_WINDOW_DAYS), which coincides with
what it computed only because both constants are 30 -- the exact conflation
draft-reaping.ts exists to forbid. Corrected where that model is stated.)
3. Two comments asserted absolutes that are false in the dangerous direction.
The invariant "no term here may exclude a model the reaper will destroy" now
names its two exclusions:
- status: the reaper destroys ('Draft','Deleted'); this warns on Draft only,
so a Deleted model is reaped unwarned. Deliberate -- a user who deleted a
model has already expressed the intent -- and now pinned by a test so it is
visible rather than disclosed only in a PR comment. Not widened; that is a
product decision.
- downloadCount is an ASSUMPTION, not a property, and "in practice" is
dropped. It has no incrementing writer -- every write is a full recompute --
so it is decreasing-capable by construction. Failure path recorded: a Draft
rolled up to 12 is excluded at day 23; deleting a version carrying 8
downloads calls updateModelLastVersionAt, which early-returns with no
Published version, so Model."updatedAt" is not bumped and the band never
re-arms; a later recompute yields 4 and the reaper destroys it unwarned.
Architectural -- the band gives one evaluation -- so recorded, not patched.
The availability argument is strengthened rather than assumed: its only raw-SQL
writer, entityAvailabilityUpdate, sets "updatedAt" = NOW() in the same
statement, so a change re-arms the band.
* fix(lint): derive the lead-text pluralisation in a function, not inline
CI red on #4595: the `ESLint (added files)` step (BLOCKING, exit 123) rejected
three `: number` annotations in the added file draft-reaping.ts with
@typescript-eslint/no-inferrable-types.
Those annotations were added in an earlier round to silence TS2367: written
inline against the constants, TypeScript narrows each `const` to its literal
type (`7`), so `OLD_DRAFT_LEAD_DAYS === 1` inside the pluralisation is provably
false. So the two tools were in direct tension -- annotate and ESLint rejects
it, drop the annotation and tsc rejects the comparison, and no test can see
either because esbuild does not typecheck.
Fixed at the cause rather than by deleting the annotations: the pluralisation
moves into `formatLeadText(days: number)`. A parameter is typed `number` rather
than a literal, so the comparisons inside are legitimate and the constants need
no annotation. Both tiers now pass. The reason is recorded on the function so it
is not inlined back.
Payload unchanged, verified rather than assumed: the old-draft SQL and message
string are byte-identical either side of this commit, measured with one
instrument at both points -- raw sha256:16 7a06452b4a37a76a / 987 chars,
comment-stripped 97b450e57f262556 / 568 chars, and
"...will be deleted in 1 week."
Why local lint missed it: this worktree's node_modules is symlinked from the
primary clone, where eslint resolves @typescript-eslint 6.21.0, whose
`recommended` DROPPED no-inferrable-types. `eslint --print-config` shows the rule
absent from the effective config, and a deliberately inferrable file lints clean
-- the local instrument could not go red on this rule at all. CI installs from
the lockfile and gets 5.62.0, where it is an error. Verified here by running the
rule explicitly.
|
||
|
Zachary Lowden
|
e92cf5fe4a |
test(geometry): a browser tier that loads the real cascade at a phone viewport (#4601)
* test(geometry): a browser tier that loads the real cascade at a phone viewport
Adds a fourth Vitest project, `geometry`, and demonstrates it catching a defect
whose own source comment records that nothing rendered can see it.
WHAT THE GAP IS, AND WHAT IT IS NOT. The `component` project is NOT jsdom — it is
real headless Chromium via @vitest/browser-playwright, `page.viewport()` moves
`window.innerWidth`, and `getBoundingClientRect()` returns real boxes. What it is
missing is the STYLESHEET and the VIEWPORT. `test/component-setup.tsx` injects
only the `:root` custom properties parsed out of globals.css, so the document
holds 24 CSS rules: Mantine classes are styleless, Tailwind utilities are inert,
and any `getComputedStyle` assertion whose expected value is the CSS initial
value passes against a broken component. And nothing sets a viewport, so files
inherit the runner's silent 414x896.
THE SAME FIXTURE, THE SAME CORRECT SOURCE, IN BOTH TIERS (PageBlockHost in its
production shell chain):
`component` `geometry`
viewport 414 x 896 390 x 844 (default vs set)
CSS rules in the document 24 3,677
box-sizing on a bare div content-box border-box
`className="flex"` block flex
chrome bar height 200 31
host frame height 350 844
APP COLUMN HEIGHT 150 813
That last row is the argument. 150 is ALSO what the app column measures once the
recorded `flex: 1` defect is planted, so a threshold written in the `component`
tier would have to expect the number the DEFECT produces.
THE HARNESS. `test/geometry-setup.tsx` loads the production cascade in production
order — the `@layer tailwind-preflight, theme, mantine, modules;` statement first
(as _document.tsx emits it), then globals.css, then every `@mantine/*` layer
stylesheet _app.tsx imports. It defaults to a 390x844 phone and THROWS unless the
window reports back the size it asked for; tests assert `observed` against their
own literal on top of that. It exports measurement helpers: `box`,
`childrenUnionBox` (the union of child rects — `scrollHeight` is clamped to the
padding box and cannot see a parent taller than its content), `flexAxis`,
`flexLonghands` (longhands, because `getComputedStyle(el).flex` serialises
`1 1 220px` and `1 1 0%` identically), and `cascadeEvidence`.
WHY A PROJECT AND NOT A CHANGE TO THE SHARED SETUP. Loading the cascade in
`component-setup.tsx` moves existing numbers — measured, the same chrome bar is
200px there and 31px with the cascade, a 169px move on one element, under 212
files / 2,362 tests of which 14 read getBoundingClientRect and 20 read
getComputedStyle. The per-file import pattern (15 files do it today, 3 also take
globals.css) stays available and is not deprecated; what it cannot give is a
guarantee — those files each picked their own subset, none declares the @layer
order, none sets a viewport, and the "did my stylesheet load" guard is
re-hand-rolled per file. The `geometry` glob (`src/**/*.geometry.test.tsx`) is
disjoint from every other project's, so nothing that runs today changes project
and no file is collected twice.
DEMONSTRATED RED. `PageBlockHostFillHeight.geometry.test.tsx` asserts that the
app column reaches the bottom of the host frame at 390x844 and at 390x640.
Dropping `flex: 1` from `app-page-content` (the mutation PageBlockHost.tsx's own
comment records as invisible to every rendered tier) fails it:
the app column ends at y=181 inside a frame that ends at y=844 — 663px of the
phone is blank below a running App Block. The column measured 150px of the
frame's 844px, with flex longhands {"grow":"0","shrink":"1","basis":"auto"}.
Restoring the property returns it to 10/10. A second mutant — dropping `flex: 1`
from the frame's `fit === 'fill'` branch — collapses the column to 269px and is
caught by the literal floor rather than by the frame/content comparison, which
both mutations keep satisfied.
BOTH MUTANTS ARE ALSO CAUGHT IN THE NODE TIER TODAY, by verbatim source pins in
pageBlockHostMaxWidth.test.ts and pageRunScrollContract.test.ts. Stated plainly
so this is not read as claiming otherwise. The difference is what each guard can
SEE: a source pin is a claim about the text of one file, blind to a collapse
arriving from the cascade, from an ancestor or from a viewport, and it has to be
rewritten every time the block is legitimately reformatted.
WHAT RUNS THIS TODAY: NOTHING. lint.yml selects `--project 'unit*'`,
`'@civitai/*'` and `'app:*'`; no pattern matches `component` or `geometry`,
because the Actions runners install no Chromium. `component` has the preview
pipeline's report-only status; `geometry` has no CI home at all. Wiring one is a
pipeline change and deliberately not in this PR — but a harness nothing runs
rots, so it is said out loud in the setup file rather than left to be discovered.
Verification: typecheck 0 errors · geometry 2 files / 10 tests passed · component
212 files / 2,362 tests passed (unchanged) · scripts unit tier 24 files / 521
tests passed · eslint clean on both new test files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* ci(geometry): run the geometry tier, and refuse a green that collected nothing
Adds a `Geometry tests` job to lint.yml. Without it this harness runs in no gate
at all — the workflow selects projects by name (`unit*`, `@civitai/*`, `app:*`)
and none of those patterns matches `geometry`.
ALIGNED WITH THE `unit` JOB'S COMMENT, NOT AN OVERRIDE OF IT. That comment gives
exactly one reason for keeping browser tests out — "they need Chromium, which
this job does not install. That is the whole reason." — which is a statement
about what that job provides, not a ban on providing it. The same comment then
retracts the only other objection on the record ("Don't cite a cold-cache flake
as a reason to keep this job Chromium-free") and names vitest.config.mts's
dedupe + optimizeDeps pre-bundling as the canonical fix. A job that DOES install
Chromium satisfies the stated condition.
GEOMETRY ONLY. `component` stays ungated and that is now visible rather than
fixed. Measured on a 16-core box: `geometry` is 2 files / 10 tests in 9.07s;
`component` is 212 files / 2,362 tests in 112.92s wall, of which 334s is test
time spread across workers — so a 2-core runner (browser pool `min(12, cpus-1)`
= 1 instance) does not divide it. That is an order of magnitude more expensive,
with 212 files of pass/fail history this workflow has never seen. It belongs in
its own PR.
REPORT-ONLY, mirroring `unit` and for its stated reason: blocking a brand-new
tier from day one would red unrelated PRs and the job would be switched off
within a week. `main` has no required_status_checks, so nothing here blocks a
merge either way; `continue-on-error` only decides whether a red renders as red
or as red-but-ignored. The FLIP TO BLOCKING note says concretely what would make
that safe. The `unit` job's selectors and its `continue-on-error` are untouched.
CHROMIUM comes from `pnpm exec playwright install --with-deps chromium` — the
workspace-local playwright, so the revision follows this repo's own pin rather
than a second version written into the workflow. Desynchronising those two is
the documented failure mode (CLAUDE.md records 59 preview specs dying on a
revision mismatch with zero specs run). A local NixOS bundle mismatch is a
property of that host and is handled by PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH; the
pin is not moved to accommodate it.
A GREEN VITEST RUN IS A CLAIM, NOT EVIDENCE — the hazard the `packages` job's
ledger exists for, one project over. `--project` matching nothing exits 0, and
this tier's glob is deliberately narrow, which is exactly the kind of pattern
that can quietly stop matching. The new step asserts floors of 2 files and 10
tests from the JSON report, with `if: always()` so it also fires when the tests
fail or the runner aborts without writing a report.
Two things measured rather than assumed while writing it: the file count comes
from `testResults.length`, NOT `numTotalTestSuites` — against a real report this
run is 2 files while that field reads 4, because it counts `describe` blocks.
And the gate script was extracted back out of the parsed YAML and executed on
all three arms before commit: real report -> exit 0 (2 files, 10 tests); a
report with `testResults: []` -> exit 1; a missing report -> exit 1 with its own
message.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(ci): the geometry job must be report-only on PRs ONLY, not on pushes to main
Caught by this repo's own gate on the previous commit's run:
`scripts/__tests__/main-branch-ci-coverage.test.ts` > "has no unconditionally
report-only job on the push path" went red (Unit tests shard 2, 1 failed / 5,624
passed).
The job was written with a literal `continue-on-error: true`. That is report-only
on EVERY event, including a push to `main`, where it produces a run reporting
`success` while the step underneath failed — the stale-TRUE shape that guard
exists to forbid, and the exact hazard the `unit` job's own comment spells out:
"A green that has to be disbelieved is worse than no run at all."
Now `${{ github.event_name == 'pull_request' }}`, byte-identical to `unit`'s.
Report-only on PRs (a new tier should not red unrelated work while it settles),
honest verdict on `main` (where the merge has already happened and there is no
unrelated work to protect). The guard accepts a conditional precisely because a
conditional can differ between a PR and a push; a literal cannot.
The comment now records this so the next reader does not "simplify" it back.
Red at
|
||
|
briant
|
d49ea2f7ec |
docs(generation): paid model loading feature doc + implementation checklist
Captures the 2026-08-18 lab call (pay to load any model into the generation cluster, resident 48 hours, priced by size, retiring auctions) and reconciles it against the code and the orchestrator SDK. Where the call's model and the contract disagree: - ResourceInfo.availability has FOUR states, not three. queuePosition lives on `unavailable`, so "queued" and "not loaded" are one status split by a null check; `unsupported` must never be offered a paid load. - GET /v2/resources?view=queue is already in @civitai/client, though the call recorded it as Koen's one missing piece. Needs confirming as deployed. - GenerationCoverage is a VIEW, not a flag. LoRA/TI/VAE/LoCon/DoRA are already covered once licensed and scanned — they are merely not resident, so a LoRA-first v1 needs no view change. Checkpoints additionally require CoveredCheckpoint membership. - CoveredCheckpoint is owned and pruned weekly by handle-auctions, which deletes every row outside the winner set — so paid loading and auctions are mechanically incompatible for checkpoints, not just conceptually. - The site does not price or charge this; it submits with the user's token and the orchestrator bills the bearer. Price the CTA from a whatIf submit, and assertWorkflowOwner is required (no-unguarded-billable-submit). Unowned gaps recorded rather than left to be discovered: the RentCivit licence gate (charging to load a model the licence forbids on-site generation for), a refund path for loads that never finish, no display of when the 48 hours expire, and concurrent purchase of the same resource. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
95a33b85f5 |
feat(nav): user-customizable sub nav (#4591)
* feat(nav): let users reorder, hide and collapse their sub nav Placement of a sub-nav item now comes from the user's saved config rather than the viewport. `homeOptions` and the container queries that moved `grouped` items in and out of the More menu by width are replaced by a registry with a `defaultPlacement`, a pure resolver, and a gear that opens a customization modal. On the homepage, where a gear already existed, it becomes a dropdown choosing between the page and the nav. The registry and resolver are React-free so the node test project — the gating tier — can import them. `homeOptions` lived in a `.tsx` importing Mantine and tRPC, which is why it had no tests. A newly-shipped nav item must reach every user without a backfill, so an item absent from a saved config is anchored beside the registry neighbours that user actually placed, scoped to the target zone, rather than appended. Gates run last, so an item pinned before an entitlement was lost is dropped whatever the config says. `postsNavItem`/`eventsNavItem` keep `toggleable: true` — it is what suppresses them at the base layer and what keeps a user's stored value in the overlay, so removing it would have turned both on for everyone. Only the account switches retire; the resolver seeds from the raw stored settings value, per item, so a user who had one on and later saves a config does not lose it. Config is bounded in `setUserSettingsInput` (known keys, per-zone max, no key in two zones): it rides `User.settings`, which is Redis-cached per user and serialised into every logged-in SSR render. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xww4JCPRUXdbtQpdEFKpTK * fix(nav): one merge behind the sub nav and its settings modal Review found four defects, three of them at the seam between the modal and the nav rather than inside either. The modal carried its own merge that skipped the retired-flag seed, so a user with Posts in their bar saw it as Hidden and lost it on their next save — with the account switch already gone. It also appended items the resolver anchors, so the first nav item shipped after this would have listed last in the modal and moved to the end of the bar on save. Both halves were individually correct; only their disagreement was wrong. `resolveNavZones` is now the one implementation, and `seedRows` is a flatten of it. `posts` and `events` carry no gate. Gating them on the flags they replace filtered them out of the modal for the default-off majority, which is everyone — and `NavTidyNotice` points that exact audience at the modal saying they can put Posts back. Placement is the config's job now; the flags only seed. `useCurrentUserSettings` returns `{}` while loading, which is non-null and so convinces `useSeededState` it has already seeded. On the degraded SSR bootstrap path the modal would have seeded from that and written registry defaults over a saved layout. `useCurrentUserSettingsState` reports resolution, and Save waits for it. Mantine's Tooltip `disabled` gates only the Transition, not the portal, so every bar item appended a div to document.body and paid useFloating on each render of a component that re-renders on every navigation — for a tooltip that cannot open, for 100% of users on day one. Also: two comments asserted the opposite of what the code does, and the characterization test claimed a pre-refactor baseline that was never committed. Controls, each reverted and re-run: dropping the seed from the modal merge prints `expected 'hidden' to be 'bar'`; re-gating posts prints `expected [Function visible] to be undefined`; removing `navigation` from the write whitelist fails 5 assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xww4JCPRUXdbtQpdEFKpTK * feat(nav): two-group organizer, locked home, and a notice for the feature Justin's review of the first build. Four changes, one of which moves the stored shape. Group membership and visibility become separate things. `bar` and `more` are the two groups plus their order; `hidden` is a visibility set that overlaps them, so a switched-off item keeps its slot and comes back where the user left it rather than at the end. The modal is two drag groups with a per-row switch, matching `ProfileSectionsSettingsInput`, in place of the three-way segmented control. `home` is locked: not draggable, not switchable, always first in the primary group. It is the way back from anywhere, so a user who hid it would have no route home from a page whose own nav they had just broken. The resolver enforces it, not only the UI — a hand-written config naming `home` in `more` or in `hidden` is ignored. The tooltip on an icon-only tab now uses the same `capitalize` class the pill's label uses, rather than a title-cased copy of the string. A helper would be a second source of casing free to drift from the CSS, which is how the tooltip came to read "models" beside a pill reading "Models". `NavTidyNotice` is replaced by `NavCustomizeNotice`. Its audience widened with it: the old one nudged only users missing Posts or Events, while what this announces is new to everyone signed in, so the feature-flag gate is gone. New dismissal id for the same reason — reusing `nav-tidy-notice` would have hidden it from everyone who dismissed the notice it replaces. Modal header and footer are fixed; only the list between them scrolls. Controls, each reverted and re-run: unlocking `home` prints `expected [] to deeply equal [ 'home' ]`; making `hidden` drop an item from its group prints `expected [ 'c', 'a', 'b' ] to deeply equal [ 'c', 'b', 'a' ]`. That second control survived its first fixture, which used the registry's own order — the dropped item re-anchored into the same slot, so both implementations agreed. The fixture now reverses the saved order, which is the only arrangement where the two differ. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xww4JCPRUXdbtQpdEFKpTK * fix(nav): pin locked items, delete the retired flags, stop writing no-op configs Five review lanes over the head commit. One blocking defect, and Justin's call to drop the flags outright rather than migrate them. `home` could be pushed out of the first slot. `locked` stopped it being dragged, but it was still placed by the anchoring pass, which computes a slot from where the nearest registry sibling sits in the USER's order — and `home` is registry index 0, so it always fell to the "nearest following" branch. Dragging Models to the end of the bar rendered Home second from last, unrecoverable without Reset since Home has no drag handle. Locked entries are now placed before the config is read, at the head of their group in registry order. `postsNavItem` and `eventsNavItem` are deleted, not retired-in-place. No backfill: anyone who had one on re-adds it from the modal, which is the whole point of the feature. That removes the seed, the two flag definitions, and the account-settings filter with them. A no-op Save wrote ~300 bytes of config that exactly matched the defaults, on a column that is Redis-cached per user and serialised into every logged-in SSR render. Saving an untouched layout now deletes the key instead, which also leaves the user tracking nav items that ship later. Aborting a cross-group drag committed the move — `onDragOver` mutates group membership as the pointer crosses and there was no cancel handler. Registered `KeyboardSensor`, because `SortableItem` spreads dnd-kit's attributes and was announcing the rows as keyboard-reorderable when they were not. `vault` never highlighted as active: its url's first segment is `user`, so it needs an explicit match. Two test fixes. One asserted a guard it could not see — `resolveNavItems` filters unresolvable keys again on the way out, so deleting the guard it named left it green; it now asserts on the layout, which is what the modal reads. And nothing anywhere asserted `showLabels`, so hardcoding it was green while "turn labels off" silently never persisted. Controls, each reverted and re-run: unpinning locked items prints `expected [ 'b', 'home', 'a' ] to deeply equal [ 'home', 'b', 'a' ]`; dropping the unknown-key guard fails 2; hardcoding showLabels prints `expected true to be false`. The drag-cancel handlers were written but never wired to the DndContext — eslint's unused-var warning is the only thing that caught it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xww4JCPRUXdbtQpdEFKpTK --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
2c066f84fb |
fix(postgres-query): --notifications could not connect at all (#4600)
`node .claude/skills/postgres-query/query.mjs --notifications ...` failed
every time with:
Query error: self-signed certificate in certificate chain
The script sets `ssl: { rejectUnauthorized: false }` on the client, but
`NOTIFICATION_DB_REPLICA_URL` carries `sslmode=require`, and pg honours
the connection string over the client option - so the bastion's
self-signed certificate was verified and rejected. The target was
unusable for every seat on the box, not just intermittently.
Every other target already works because their URLs say
`sslmode=no-verify`, which agrees with the policy the script sets
explicitly. This drops only `sslmode=require`, leaving `no-verify`
alone: it already means what we want, and rewriting it would be a change
with no effect for a later reader to reason about.
Verified after the change, in a tree with node_modules:
--notifications SELECT COUNT(*) FROM "Notification"
WHERE type = 'new-image-comment' -> 776221 rows, 16.7s
--prod SELECT 1 -> ok, 97ms (unchanged)
The count cross-checks against a standalone script that stripped the
same param independently, so the fix is reaching the same database.
Found while verifying a notification-details question for the fix in
PR #4599; the workaround there was a local throwaway script, which would
have died with that session and left the next seat hitting the same wall.
Claude-Session: https://claude.ai/code/session_01BtYZrp2py6qZLag9LcgGBs
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Justin Maier
|
05b2d86532 |
fix(images): a null postId in a notification link 500d the image page (#4599)
* fix(images): a null postId in a notification link 500d the image page
`image-reaction-milestone` interpolated `details.postId` into its URL
unconditionally. An image in no post - an article cover, mostly - has
postId null, which template-interpolates to the literal string `null`.
`/images/[imageId]` parsed that with `numericString`, `Number("null")`
came back NaN, and the bare `.parse` threw mid-render. A throw in render
is a 500, so the link Civitai had just mailed the user was dead.
Measured on prod 2026-09-03, before the fix:
/images/140935761 -> 200
/images/140935761?postId=null -> 500
/images/140935761?postId=abc -> 500
/images/140935761?postId= -> 200 (Number("") is 0, so it coerces)
So it is any non-numeric postId, not the string `null` specifically.
25,135 images on prod have a null postId and are an article cover.
Both halves land, and they are not redundant:
1. The emitter omits the param when postId is null.
2. `/images/[id]` reads its query params with a new
`parseImageQueryParams` helper - safeParse with an empty fallback,
which is what `useZodRouteParams` has always done with this same
schema over this same router query. That page was the outlier.
Half 2 is the one that matters most: links already delivered sit in a
user's notification history forever and cannot be rewritten, so the read
side has to survive junk on its own. Half 1 alone would leave every
existing link broken.
Each test fails on its own half being reverted, verified by reverting
each half in turn:
half 1 reverted -> expected '/images/140935761?postId=null' to be
'/images/140935761'
half 2 reverted -> ZodError, "'null' cannot be converted to a number"
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BtYZrp2py6qZLag9LcgGBs
* fix(images): close the rest of the junk-postId path, and pin the call site
Five-lane review of the first commit. Three lanes converged on the same
gap and one contradicted a claim I had made in three places.
The tests did not cover the thing that broke prod. Reverting
`[imageId].tsx` to a bare `.parse` while leaving `parseImageQueryParams`
exported and unused left all 6 tests green, typecheck green, lint green,
and the page 500ing again. Verified by running that mutation. The
behaviour lives at the call site, so it is now pinned at the call site by
a convention guard, `no-throwing-image-query-parse`, which bans a bare
`.parse` of `imagesQueryParamSchema` and matches the `.omit(...)` chained
form too. Confirmed it goes red on that same mutation, naming file and
line.
Two more live sites for the same junk, both reachable from the fixed URL:
- `ImageDetailModal` bare-parsed the same schema off the same
`useBrowserRouter` query. `removeEmpty` drops null VALUES, not the
string 'null', so the junk survived. Reachable in one click:
RemixGalleryCard -> triggerRoutedDialog -> image-detail.dialog.ts
spreads the current query forward, junk included.
- `ImageDetailProvider` read `postId` off the raw query with a CAST, not
a parse, so 'null' beat the parsed filter and went out to
`image.getInfinite` against an input typed `z.number()` - a guaranteed
failed request on every load of the links this page was fixed to serve.
Both now go through the helper, which takes an optional schema for the
`.omit(...)` variant.
Corrected, because it was wrong: the first commit said delivered links
"cannot be rewritten". They can. The URL is not stored -
`getNotificationMessage` recomputes it at render from the stored details
JSON, so the emitter fix IS retroactive for in-app notifications. Half 2
still earns its place - a link copied out of the app, and `?postId=abc`
from any source, are both beyond the emitter's reach - but the reason was
overstated and the comments now say so.
Tests, each driven red on purpose before being trusted:
- whole-object discard, not per-key: a strip-the-bad-key implementation
returned `{period, sort}` where `{}` is asserted.
- producer -> consumer seam: dropping `postId` from the controller's
details gave '/images/141298569' where '?postId=7' is asserted. Nothing
else tied the key the handler reads to the key the controller writes.
- dropped an `as` cast that was a no-op today and a silent suppression
the moment `prepareMessage`'s parameter is narrowed.
Full suite 25,022 passed, 1 failed - `appListingMenuSurface.test.ts`, a
Windows path-separator assertion, untouched here and reproduced
identically at origin/main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BtYZrp2py6qZLag9LcgGBs
* docs(test): say what the parse guard cannot see
A review lane's closing note, and it is the useful half. The third call
site in this incident was `ImageDetailProvider` reading `postId` off the
router query with a CAST, not a parse. A cast has no runtime behaviour
at all: it appears in no grep for `.parse(`, it never throws, and this
guard's matcher would never have flagged it. It was found by a reviewer
following the data flow, not by any pattern search.
Recorded in the guard itself because a guard sitting next to a defect
gets read as covering it, and the next cast of a router-query value
needs a different guard rather than a wider regex here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BtYZrp2py6qZLag9LcgGBs
* fix(test): the guard was one formatting pass from reporting clean
Review of the guard I added last commit. It caught the one mutation I
had tested and missed 8 of 9 other shapes of the same defect - measured
against its own regex, not reasoned about.
The one that matters: the matcher ran line by line while its pattern
spans from the schema name to `.parse(`, so any call prettier had
wrapped was invisible. That is not hypothetical. The modal's own fixed
call already spans several lines, because the throwing form of that
expression is ~100 chars and prettier breaks it. Add one key to that
`.omit({...})` and the guard goes blind on the exact file whose chained
shape it was widened to catch - reporting clean over a live bug.
Fixed:
- Scan the whole source and derive the line from the match offset,
instead of `split('\n').forEach`. Also catches `parseAsync` (throws on
rejection, same defect) and `?.`.
- Reach, not count. `MIN_SCANNED_FILES = 2000` could not fail in the way
that mattered: a walk that had lost `components/` and `pages/` - all
three call sites from the incident - still returned 3,077 files and
cleared it. Replaced with an assertion that the walk actually reaches
the four files this exists to watch.
- `stripComments` now preserves newlines. Deleting block comments
outright shifted every line after them: on `image.utils.ts` a
violation at line 170 was reported as line 125, and that is the file
the failure message sends people to.
- The old "does not report the fixed sites" control was vacuous. Neither
file it named contains a comment the matcher would flag, so it passed
with `stripComments` deleted outright. The fixture now carries a
commented violation, which is the only thing that makes stripping
load-bearing.
- `prettier-ignore` on the wrapped fixture entries. Without it
`prettier --write` collapses them and deletes the multi-line coverage
this commit exists to add - measured, it did exactly that on the first
version of this file.
Every one driven red on purpose:
page -> bare `.parse` RED
modal -> throwing chain, wrapped RED (old guard: green)
walk stops descending into components/ RED ("no longer reaches ...")
stripComments neutered RED
Left undone deliberately: an aliased import, a destructured `parse`, and
`const S = schema; S.parse(q)` all still pass, as does the cast class.
Those need an AST, not a wider regex. Recorded in the docblock instead,
because the shapes above are what a person writes by accident and
indirection is not - and a guard nobody can read gets deleted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BtYZrp2py6qZLag9LcgGBs
* fix(test): a semicolon un-guarded the chain, and reach alone missed half the tree
Second review pass on the guard rewrite. Three findings, all of them a way
this reports clean while the defect is live, or one edit from it. None was
reachable from the four controls in the previous commit - those exercise
mutations I already knew about, which is exactly the evidence shape the
review exists to distrust.
1. The chain argument was bounded by `[^;]`, which cannot cross a statement
terminator. So any chained call whose callback has a block body was
invisible:
imagesQueryParamSchema
.refine((v) => { const ok = !!v; return ok; }, 'x')
.parse(query)
That is not exotic here - `zod-helpers.ts` writes `ctx.addIssue(...)`
inside a callback body, so it is the house style. Anyone adding
validation to a chained call silently un-guarded it. Now matched as
balanced parens. The two alternatives are disjoint, so there is nothing
to backtrack over: 200 chained links resolve in 0 ms, and a file where
the schema is never parsed still yields no match.
2. Reach alone had the mirror of the count's blind spot. A count could not
see a targeted loss; reach cannot see any directory holding none of the
named files disappear. Measured: a walk that had lost 2,945 files - half
the tree - passed every assertion. Both instruments are back, and the
floor sits just under the real number (5,000 against 5,842) rather than
at the 34% that made the old one useless.
`components/Image/DetailV2/ImageDetail2.tsx` is named explicitly because
no count can catch it: that directory is 12 files, so losing all of it
leaves 5,830 and clears any usable floor. It holds the component
`[imageId].tsx` actually renders.
3. The `/g` regex was a module constant. `matchAll` is safe - it clones and
never writes back, measured - but one `.test()` call anywhere in the file
leaves `lastIndex` mid-source and every following file starts scanning
from that offset, skipping violations by scan order and failing GREEN.
The PREVIOUS version of this guard used `.test(line)`, so a revert or a
copy-paste out of git history is what reintroduces it. Now a factory; a
fresh regex cannot be poisoned.
Seven controls, each driven red on purpose. Three and five would have passed
before this commit:
page -> bare .parse RED
modal -> throwing chain, wrapped RED
modal -> chain whose callback has a statement RED <- new
regex bounded by [^;] again RED <- new
walk loses DetailV2 only (5,830 files) RED <- new
walk loses server+shared+hooks+... (2,909) RED <- new
stripComments neutered RED
Checked and left alone: `prettier-ignore` is load-bearing and correctly
placed, and no other formatter or autofix in this repo would rewrite those
lines - eslint extends `eslint-config-prettier` only, and
`no-unnecessary-condition`, the one fixer that would have eaten the
optional-chain case, is not enabled. Two stripper false negatives need a
string literal faking a comment delimiter; the plausible instance (a glob
like 'src/**/*.ts') is caught, and the rest needs a tokenizer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BtYZrp2py6qZLag9LcgGBs
* revert(test): drop the regex guard - the invariant is a tree property
Executing a stop rule agreed before the answer was known: if a fourth
review found another green-while-broken hole in this guard, remove it
rather than write a fifth draft. It did, so this removes it.
The deciding case is the one the guard's own docblock cited as its
motivating example. `zod-helpers.ts` writes:
.transform((val, ctx) => {
const parsed = Number(val);
if (isNaN(parsed)) ctx.addIssue(`...`);
return parsed;
})
`if (` opens depth 2 and `isNaN(` opens depth 3, and the matcher's
balanced-paren arm admits depth 2. Verified: that shape is MISSED, and a
depth ladder puts the boundary exactly between `Number(v)` and
`Number(String(v))`. The fixture passed only because `!!v` needs no
parens - the guard was green while blind to the idiom it named.
That is the third costume of one defect. Each draft moved a boundary -
`[^;]`, then depth-1 nesting, then depth-2 - and each new boundary is a
line the next ordinary line of code crosses. `f(g(h(x)))` inside a
callback is not exotic. The invariant is "a call expression whose callee
resolves to this schema", which is a property of the syntax TREE; every
regex approximating it is a guess about how deeply the next author will
nest, and it fails GREEN.
Three drafts, three holes, none found by me reasoning about it - each
found by a reviewer running shapes through it. The base rate did not
decay: pass two found two, pass three found three. That is evidence the
surface is hard, not that it was converging.
Also removed for the same reason: MUST_REACH was a hand-maintained
mirror of an import graph. 34 files import `image.utils` across ~20
directories; the list named five, and the rest fit inside the floor's
slack. Adding two more would not have changed the shape.
WHAT THIS COSTS, stated plainly rather than buried: nothing now pins the
page's call site. Reverting `[imageId].tsx` to a bare `.parse` leaves
every test green and restores the production 500. That gap is documented
in `reaction.null-postid-url.test.ts` where the next reader will be
standing, not only here.
The replacement is an AST check - resolve the callee of each
`.parse`/`.parseAsync` back to its import - which also catches the
aliasing and destructuring all three regex drafts documented as
permanently blind. Separate PR, its own review, so it does not ride in
on this one. `no-server-infra-in-app-graph.test.ts` already walks the
tree with `typescript` that way.
The fix itself is untouched and unaffected: the emitter, the three call
sites, and their tests have not changed since
|
||
|
kaydaxter
|
c0ddb36434 | krea2 custom checkpoint support | ||
|
Luis E. Rojas Cabrera
|
9a15b59c29 |
Merge pull request #4596 from civitai/fix/floor-purchasesmultiplier-at-1-to-prevent
fix(buzz): floor purchasesMultiplier at 1 to prevent zero buzz credit |
||
|
ZacxDev
|
a6d272d271 | 5.1.70 v5.1.70 | ||
|
Luis Rojas
|
e8922d726d |
fix(buzz): floor purchasesMultiplier at 1 to prevent zero buzz credit
Non-finite, negative, or zero purchasesMultiplier causes getBuzzBulkMultiplier to return zero buzz despite real payment. The paid-purchase paths (Stripe, Paddle, NowPayments) then write an idempotency marker, making the zero-credit unrepairable by retry. Floor purchasesMultiplier at 1 in getBuzzBulkMultiplier (the unconditional call site) to ensure customers always receive at least their purchased buzz. Add tests verifying NaN, negative, zero, and sub-1 multipliers are clamped to 1, yielding totalCustomBuzz = buzzAmount (no bonus). Keep the floor here, not in getMultipliersForUser, to avoid double-flooring downstream consumers (award computation, Redis Lua cap) that rely on raw multiplier values. |
||
|
Zachary Lowden
|
ce0e369de0 |
fix(app-blocks): full-bleed opt-out ledger was inert in production (data-testid is stripped) (#4590)
* fix(app-blocks): full-bleed opt-out ledger was inert in production
The ledger in globals.css was keyed on
[data-testid='app-page-frame'][data-block-id='...'], but next.config.mjs sets
compiler.reactRemoveProperties: { properties: ['^data-testid$'] } under
NODE_ENV === 'production', so every data-testid is compiled out of the live
DOM. The compound selector therefore matched nothing on civitai.com and
playable-collections rendered letterboxed at the 1600px cap - the exact
outcome the rule's own comment says it exists to prevent.
Measured on civitai.com/apps/run/playable-collections (image
20260902233645-bbbe837): the rule was present in the deployed CSS verbatim;
0 elements matched the compound selector; 1 matched
[data-block-id='playable-collections']; 0 data-testid attributes across 615
elements while 209 elements carried other data-* attributes; the computed
--app-page-max-width on the capped box was 1600px.
Fix: PageBlockHost stamps a presence marker, data-app-page-frame, beside
data-block-id on the same element - the same idiom AdhesiveAd and AppFooter
already use for globals.css hooks - and the ledger is re-keyed onto
[data-app-page-frame][data-block-id='...']. The frame half is kept rather
than dropped so the ledger still says "this is the page host", not "any host
carrying a block id".
New guard: __tests__/ledgerSelectorSurvivesProdStrip.test.ts, in the gating
node unit project. It parses the strip list out of next.config.mjs and the
ledger selectors out of globals.css and fails if a ledger selector depends on
an attribute production removes - reading one side and comparing it to the
other rather than restating a literal. It also checks the ledger's HOW-TO-ADD
template (the next entry is copied from it) and that every attribute the
ledger depends on is really stamped by PageBlockHost. Fails closed on an
unparseable config, an empty strip list, or zero parsed ledger rules, and
carries its own negative control.
Red at origin/main (2 failed / 3 passed of 5), green at HEAD (14/14 with the
existing node ledger guard). No rendered test can see this defect: every tier
runs with NODE_ENV != production, where the testid is present.
* test(app-blocks): pin the ledger selector as a RELATIONSHIP, and the strip's own gate
Three audit findings on this PR, all of the same family: a guard whose message
claims more than its implementation checks.
F2 — the stamping guard was a whole-file substring search, so RELOCATION survived
the entire gating tier. ledgerSelectorSurvivesProdStrip.test.ts asked
`!host.includes(attr + '=')` over the text of PageBlockHost.tsx, under a message
saying the attribute was "not stamped on its root". Measured: moving
data-app-page-frame off the host root onto the app-page-content wrapper
re-creates the shipped production defect exactly — the compound selector
[data-app-page-frame][data-block-id='...'] then matches zero elements, because
its two halves are on different boxes — and the gating node tier stayed
byte-identically green (7 failed | 741 passed both ways; the 7 are the
pre-existing hiddenBlocks failures). Only the report-only browser tier caught it.
Fixed on both sides, per the sibling guard's own doctrine that the opt-out is a
relationship between two attributes on ONE element:
- ledgerSelectorSurvivesProdStrip now parses PageBlockHost.tsx and asks whether
SOME ONE JSX element stamps every attribute a ledger selector chains. A spread
attribute is deliberately not counted, so an element whose attributes this
cannot read fails closed rather than being credited.
- pageBlockHostMaxWidth's "stamps data-block-id on the host root" test now reads
the parsed frame element's own attribute list and asserts BOTH halves are on
it, plus that data-block-id is fed blockId. It replaces a text region slice.
- a second negative control covers the new relationship helper with the exact
defect shape: both attributes present, on two different elements.
F3 — the "gated on a production build" assertion was satisfied by an unrelated
line. It sliced next.config.mjs at the first reactRemoveProperties and looked for
NODE_ENV === 'production' anywhere in the prefix; line 9 is
`const isProd = process.env.NODE_ENV === 'production'`, 174 lines earlier and
unrelated, which satisfied it alone. Measured: rewriting compiler: so the strip
applies unconditionally left that guard 5 passed, green.
Now next.config.mjs is PARSED (ts.createSourceFile, ScriptKind.JS — still not
imported; importing it pulls in the Next build pipeline). The gate assertion reads
the compiler: property's OWN conditional and makes three separate claims: the
condition is the production test, the strip is in the branch that condition
selects, and it is NOT also in the other branch. Parsing makes the strip-list
parse comment-proof for free, which was the other half of the finding: a
commented-out reactRemoveProperties would be the first hit for any text search.
The strip-list parse also now asserts exactly one declaration rather than
grading the first of several.
F1 — the publisher-facing HOW-TO still taught the production-inert selector.
docs/features/app-blocks.md showed the data-testid spelling, which is the copy app
authors actually read, and nothing guarded that file at all. It now shows
[data-app-page-frame][data-block-id='...'] and carries the same
never-key-on-data-testid warning as the ledger. The doc's claim that the browser
test made the instructions un-rottable was false after this PR (that test now
injects the new shape) and overstated in any case, since the browser project
reports as the non-blocking preview / component-tests status: the paragraph now
says what is actually checked, by which tier, and what is not checked at all.
To make that a true claim rather than a softer one, the doc's CSS block is now
read by ledgerSelectorSurvivesProdStrip in the gating unit project, held to the
same rule as the ledger's own HOW-TO-ADD template.
Mutation matrix, gating node unit project scoped to src/components/AppBlocks/
(baseline 7 failed | 741 passed at the PR head, 7 failed | 743 passed after):
relocation (marker moved to app-page-content) base: SURVIVED 741 -> now 2 kills
unconditional strip (ternary removed) base: SURVIVED 741 -> now 1 kill
marker deleted entirely (positive control) base: KILLED -> still killed
doc reverted to the data-testid spelling base: SURVIVED -> now 1 kill
strip moved to the non-production branch -> 1 kill
gate condition hoisted to the isProd constant -> 1 kill
Every kill is on this change's own assertion message, not a neighbour's; each
mutant was applied in a cp -a copy with its .git file removed and reverted to a
cmp-clean tree afterwards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NjiVTnUgwbC1jVLCZBRtzp
* test(app-blocks): close four gaps the ledger guard's own round left open
Round-2 audit findings on this PR. All four are the same family: a claim
wider than what is actually asserted.
1. THE DOC MADE A FALSE CLAIM ABOUT GATING, AND THE FIX FOR "READS AS
COVERAGE WHILE PROVIDING NONE" RE-INTRODUCED IT ONE ROUND LATER.
`docs/features/app-blocks.md` said the guard "runs in the node `unit`
project, which is a blocking CI check - so the specific way this recipe
went wrong before ... cannot come back silently." Both halves are false.
Measured against the GitHub API: `branches/main/protection` reports
`has("required_status_checks") == false`, and the endpoint itself 404s
with "Required status checks not enabled" - NO status check blocks a
merge to `main` in this repo. And `.github/workflows/lint.yml:405` sets
`continue-on-error: ${{ github.event_name == 'pull_request' }}` on the
`unit` job, under its own header "REPORT-ONLY ON PULL REQUESTS, REAL
VERDICT ON `main`". The paragraph also contrasted this against the
browser tier it correctly called non-blocking, handing the reader a
distinction that does not exist and calibrating trust the wrong way.
Rewritten to what actually holds: the guard is real and does catch the
`data-testid` spelling, it annotates on a PR and renders an honest
verdict on a push to `main`, and it is not a door - a red guard is a
signal a reviewer must read.
PRE-EXISTING CORRECTION, outside this PR's range: the same false "which
can [block a merge]" sentence in
`src/components/AppBlocks/__tests__/pageBlockHostMaxWidth.test.ts:26`
(present on `origin/main`), plus two same-file repetitions of the
"gating tier" framing, one of which this PR itself added.
2. `stripPatterns()` FAILED OPEN ON A MIXED ARRAY. It filtered the parsed
`reactRemoveProperties.properties` array to string literals and graded
whatever survived, silently dropping a spread/identifier/interpolated
entry - so the guard compared the ledger against a SHORTER strip list
than production applies. Measured on the pre-fix code: with
`properties: ['^data-testid$', ...EXTRA_STRIPS]` where
`EXTRA_STRIPS = ['^data-app-page-frame$']`, this file reported 7 passed
/ 7 while production was stripping the very attribute the ledger had
just been re-keyed onto - i.e. it re-certified the shipped defect as
fixed. Now asserts every element was readable, matching the fail-closed
style of the two checks above it.
3. THE DOCUMENTED-RECIPE GUARDS CHECKED ONLY THE STRIP MECHANISM. A
documented selector matches zero elements two ways, and this file's own
header names both; only the first was checked on the doc and on the
ledger's HOW-TO template. Measured on the pre-fix code: changing the
recipe to `[data-app-page-fram][data-block-id='your-app-slug']` (one
dropped `e`) left it 7 passed / 7 - nothing strips a misspelling, and
nothing stamps it either, so an author copying it gets a rule matching
zero elements and a letterboxed app: the same user-visible outcome as
the shipped bug, through the other door. The realistic trigger is a
RENAME, where `globals.css` and `PageBlockHost.tsx` are both pinned and
the documented copies are pinned by nothing. `stampedTogether` is now
applied to both documented surfaces as well as the shipped rules.
4. THE THIRD GATE ASSERTION WAS UNREACHABLE AND ITS MESSAGE COULD NEVER
PRINT. Both defect shapes it named are consumed earlier: the strip in
BOTH branches dies in `stripPatterns()` (5 failed / 2 passed,
"declares `reactRemoveProperties` 2 times, not once"), and the strip in
the non-production branch only dies on the `whenTrue` assertion
(1 failed / 6 passed) - both re-measured here. With exactly one
`reactRemoveProperties` enforced, the only inputs left were text
coincidences in `getText()`, i.e. false positives of the
spelled-not-structural kind this round set out to remove. Deleted, and
its share of the "THREE ASSERTIONS" comment deleted with it;
`compilerGate()` no longer returns the field nobody reads.
Nit, same pass: `stampedAttributeSets`'s comment claimed a
spread-bearing element "cannot satisfy a selector". It can, via its
explicitly-written attributes; only spread-CARRIED attributes are
uncredited. The fail-closed direction was right, the sentence was not.
VERIFICATION. Node `unit` scoped to `src/components/AppBlocks/`:
7 failed | 743 passed (750) both before and after - the 7 are pre-existing
`hiddenBlocks.test.ts` failures ("Cannot read properties of undefined
(reading 'clear')"), unrelated. Every mutation above was run in a `cp -a`
copy with its `.git` removed, reverted and `cmp`-verified byte-identical
afterwards, and each was measured on the PRE-fix code as well: mixed strip
array 7/7 green before / 5 failed | 2 passed after; doc typo 7/7 green
before / 1 failed | 6 passed after, on the new stamped-together message;
`globals.css` template typo 7/7 green before / 1 failed | 6 passed after.
Positive control (doc selector reverted to `data-testid`) still dies on the
strip message, 1 failed | 6 passed. `node scripts/typecheck.mjs`: 0 type
errors in 83s at the 8192 MB cap - the first run of the same command
reported 10 `error TS` lines, so the instrument was watched red before its
green was believed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NjiVTnUgwbC1jVLCZBRtzp
* test(app-blocks): stop the ultrawide guards claiming a gate that does not exist
Two honesty defects in messages a developer reads at failure time.
1. Residual "gating"/"blocking" language in pageBlockHostMaxWidth.test.ts,
in a file whose own header already says the opposite. Measured ground
truth: .github/workflows/lint.yml carries
`continue-on-error: ${{ github.event_name == 'pull_request' }}` on the
node `unit` job, and `main` has no required status checks at all (the
branch-protection endpoint 404s; the only ruleset is `deletion`). So the
node tier is report-only on a PR and renders an honest verdict on a push
to `main` or a `workflow_dispatch` — and neither tier blocks a merge, by
either mechanism.
The two that mattered are assertion messages, not prose: a developer
renaming APP_PAGE_MAX_WIDTH_PX was told this guard was "the only BLOCKING
check on the ultrawide cap", and one renaming the frame marker was told
"nothing rendered in the gating tier can see this". Both now say what is
true and still say why deleting the guard costs something: this file is
the only place that reads that declaration at all, because the browser
tier deliberately imports nothing from the host.
Swept the whole file rather than the reported lines: the header title
("the GATING half" -> "the SOURCE half", pairing with the browser file's
"MEASURED"), "THE GATING TIER" -> "THE NODE TIER", "the whole gating
suite" -> "the whole node suite", and "the one regression this file
exists to block" -> "to catch".
2. ledgerSelectorSurvivesProdStrip.test.ts's template assertion inspected
the WHOLE un-stripped globals.css -- shipped rules AND the HOW-TO
template -- under messages naming only the template. Measured under the
relocation mutant: it printed two selectors and blamed the template for
both, one of them the shipped playable-collections rule, pointing a
developer at a correct template to fix a defect that is not in it.
Scoped it to commentsOnly(), the exact complement of the stripComments()
the shipped-rule guards read, so the two scopes partition the file
instead of overlapping. Shipped-rule coverage is unchanged (the strip
mechanism and the stamped-together mechanism each keep their own test).
The vacuity guard gets stronger for free: a deleted template can no
longer be masked by a shipped rule.
Added a derived control on the scoping itself -- shipped selectors must
not appear in the comment-only scope -- with a non-vacuity assertion in
front of it, so degrading commentsOnly() toward the identity function
fails loudly instead of silently restoring the misattribution.
Verified in a worktree at
|
||
|
ZacxDev
|
71a2f0b146 | 5.1.69 v5.1.69 | ||
|
Zachary Lowden
|
44307be231 |
feat(app-blocks): allow the init-fragment fast path for app-requests (#4594)
Adds the first entry to BLOCK_INIT_FRAGMENT_ALLOWLIST, which has shipped empty since it was introduced. The App Requests block now receives its host theme in the iframe URL fragment, so it can paint in the right theme before first paint instead of flashing the wrong one and repainting when the init message lands. Why this block clears the bar the gate documents: - It ships a reader for the fragment. Its index.html resolves the host theme in an inline pre-paint script and records the result on a boot-theme attribute that React reads back on its first commit. - It reads location.hash nowhere at runtime. The only textual mentions in its source are doc comments warning against reading it, because the SDK's transport strips the fragment during init. Perturbing a block's own hash routing is the hazard that put playable-collections on the denylist; this block is clear of it. Keying: the block declares blockId "app-requests" and a page with no slots, and on the page-run surface slug === blockId, so the single string is correct for both lookups. A comment next to the entry says so, and warns that a slot-mounted block would need its blockId specifically -- the model slot has no slug, so a slug-only entry there is silently inert. Tests: the previous suite drove only the injectable form with a synthetic allowlist, which by construction cannot observe the shipped constant, and its one production-binding assertion pinned the allowlist as empty. That test is replaced by an exact whole-set pin (fails on an unintended addition as well as a removal) plus assertions that the fast path is on for this block on page-run and, crucially, still OFF on dev-tunnel and review-preview -- so allowlisting a published app did not leak the fragment onto a moderator's preview of that same app's next unreviewed submission. That surface assertion was vacuous while the allowlist was empty and is reachable for the first time here. Two header comments that asserted the allowlist was empty are corrected rather than left to rot, including the "nothing to revoke in a hurry" note, which no longer holds now that the set has an entry. Claude-Session: https://claude.ai/code/session_014FaWjDf5cNU6jEnDEHo8cU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Luis E. Rojas Cabrera
|
16e2567b21 |
Merge pull request #4546 from civitai/fix/correct-transaction-direction-labels
fix(transactions): correct transaction direction labels |
||
|
Luis E. Rojas Cabrera
|
a82b9a1030 |
Merge pull request #4593 from civitai/fix/suppress-blocked-users-messages-in-group
fix(chat): suppress blocked users' messages in group chats |
||
|
Luis E. Rojas Cabrera
|
b7af8e28b5 |
Merge pull request #2827 from civitai/feat/custom-anima-training
feat(training): allow custom Anima checkpoints as a trainer base |
||
|
Luis E. Rojas Cabrera
|
263f13617c |
Merge pull request #4560 from civitai/fix/restore-search-indexed-images-on-model
fix(model): restore search-indexed images on model republish |
||
|
Justin Maier
|
885e3a621f | chore(event-engine): release event-engine-v1.9.15 event-engine-v1.9.15 | ||
|
Luis Rojas
|
9565f481ec |
fix(chat): suppress blocked users' messages in group chats
When a user is blocked, their messages are now hidden in group chat conversations, matching the behavior for models, images, and other content elsewhere on the site. Previously, blocked users' messages were only suppressed outside of chat. Messages are filtered from display, and quoted/referenced messages from blocked users are replaced with a "Blocked message" placeholder. Blocking already worked correctly in 1:1 chats; this extends the fix to group chats. Adds filterBlockedChatMessages utility and test coverage for the filtering behavior. |
||
|
briant
|
bbbe837d13 | 5.1.68 v5.1.68 | ||
|
briant
|
1dcfb3b21e |
docs(creator-studio): paid-access unification design, all questions answered
Reconciles early access and permanent paid access into one gate with a scheduled discount ladder and an irrevocable free-access guarantee. The gate keeps its composite PK and gains one column; grants, price steps and the guarantee become their own tables, and the promotion tables are re-keyed onto the gate's polymorphic axis so comics come along later. Three things the design turns on, each verified against production: - steps anchor on initialPublishedAt, never publishedAt, which the early-access expiry job overwrites with NOW() - PriceStepMode is its own enum: SaleDiscountType.Fixed means Buzz taken off, the inverse of what a step needs - freeAt is a ceiling on every read, so a permanent gate cannot defeat a guarantee All 17 product questions are answered. Four non-blocking follow-ups remain, on copy and cadence rather than shape. Pricing templates stay blocked, now on this shipping rather than on a decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
briant
|
f82da64d55 |
feat(generation): add Muse Image (Meta) with txt2img and img2img:edit
Bumps @civitai/client to 0.2.0-beta.98, which is where the MuseImage input types landed. Muse Image is API-only through fal, model-locked, no LoRA support - the same shape as Reve, so the graph and handler follow those. The input surface is prompt, aspectRatio and quantity, plus images[] on edit; there is no seed, cfg, steps or negative prompt to expose. Aspect ratios fix the long edge at 2048 to match fal's documented 16:9 output of 2048x1152. 'auto' is reserved for edit, where the output ratio comes from the reference images. Needs manual SQL per environment before it works: version 3291238 must be renamed to 'Image' on baseModel 'Muse Image', and added to EcosystemCheckpoints so branch 1 of GenerationCoverage covers it while the version is still Draft. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
a15034f536 |
fix(stickers): stop the draft editor chrome rotating onto the sticker (#4578)
* fix(stickers): stop the draft editor chrome rotating onto the sticker
The toolbar, note field, payout caption and Buzz button were children of the
element carrying the sticker's rotation. Two corner pills rode that rotation
outright, landing on the artwork past a quarter turn; the buy cluster was
counter-rotated upright but still ANCHORED to the sticker's rotated bottom
edge, so the anchor swung over the artwork and the upright cluster painted on
top of the thing being edited.
Rotation now lives on an inner element and all the chrome is its sibling, so
nothing can turn it and nothing can swing where it lands. The two corner pills
merge into the single bar under the sticker that narrow drafts already used,
which also retires the panel-band geometry that existed only to keep the buy
button off them.
The standoff from the sticker is derived from its measured size and angle
rather than from a constant or from the local edge - a constant is wrong at
every size and the local edge is wrong at every angle. The caption carries a
creator username with no useful length bound, so the cluster's own height is
kept out of the clearance entirely: it is anchored by the edge nearest the
sticker and grows away from it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V79q7EDZKCs2AWEHKSaeFj
* fix(stickers): stop the flip decision oscillating into a render loop
Two defects found by driving the real page, both introduced by the previous
commit and both invisible to the component harness, which loads no stylesheet
and so gives the draft no layout at all.
A DOMRect's properties are getters on the prototype rather than own enumerable
ones, so `{ ...rect }` copies nothing. `shiftDown` built the alternative
candidate box that way and it came out with `left` and `right` undefined;
`overlaps` then evaluates `undefined > tray.left` as false, so the tray was
invisible to whichever of the two boxes was derived. Unflipped, `below` was the
real rect, saw the tray and flipped. Flipped, `below` was derived, could not see
the tray and unflipped. Measured on the image detail page: 39 alternating
decisions and "Maximum update depth exceeded", surfacing inside Mantine's
SegmentedControl, whose inline ref callback re-runs on every render and turns a
repeatedly re-rendering parent into runaway updates. The type cannot catch this:
DOMRect satisfies Box structurally and the spread of one type-checks while being
empty at runtime. Every field is now named.
Separately, EdgeImage has no intrinsic size until it loads, so a draft dragged
in from the tray measures zero tall for its first frames. Deciding a flip from
that put the two candidate positions close enough together that each argued for
the other. `measure` now keeps the current side until the sticker has a size,
the same rule `placementControlPosition` states next door.
The standoffs move in the same state update as the side. Writing the margin to
the node while the class came from state split them across a render, leaving the
element carrying the standoff for the side it had just left - 127px of real
movement against a model that said 155.
Tests, each shown failing on a revert of what it pins:
- a DOMRect-shaped fixture whose edges are prototype getters, so it spreads to
nothing the way the browser's does; an object literal passes with or without
the bug. Reverting gives "expected undefined to be 322" and, on the
same-decision-from-both-sides case, "expected false to be true".
- the standoff reaching the DOM, which nothing covered: swapping the two margins
at the call site gives "expected 14 to be greater than 62.34", deleting them
gives "expected '' not to be ''".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V79q7EDZKCs2AWEHKSaeFj
* test(stickers): record what the draft chrome tests do not cover
Acting on a test review of
|
||
|
Zachary Lowden
|
2ea3ef1630 |
fix(app-chrome): site-consistent links, full-bleed chrome, real icons in Recently run (#4577)
Three pieces of feedback on the App Blocks page chrome. LINK STYLE. The "Marketplace" crumb was a hand-styled Text carrying c="blue.6" and td="underline". It is now a Mantine Anchor, the idiom every other link here uses. The hard-coded shade was not arbitrary — an audit had bumped it from blue.4 for contrast — so the swap had to land on the same colour, and it does: --mantine-color-anchor resolves to blue-6 on light (pixel-identical) and blue-4 on dark, where the old fixed shade stayed dark. Measured against this bar's own background, dark goes 3.82:1 -> 5.49:1, i.e. failing WCAG AA to passing. The resting underline is kept, via Anchor's own underline="always" rather than a hand-rolled decoration. Dropping it to take the library default was tried and reverted: the crumb's neighbours are dimmed, so hue became the sole resting cue at 1.07:1, and WCAG 1.4.1 (F73) permits colour alone only above 3:1. Five other call sites in this repo reach for the same prop for the same reason. Note for anyone reading the old comments: blue.6 does NOT clear AA on the light chrome surface — 3.37:1 against 4.5:1 for 12px text. That shortfall pre-dates this change and is untouched by it; what changed is that three places no longer assert otherwise. WIDTH. The ultrawide cap sat on the host root, so AppBlockChrome was capped along with the app and a full-page app read as a boxed widget dropped into the page. The cap now lives on a new content wrapper holding the app and its failure card, and the chrome spans the page like every other site-level bar. This reverses a deliberate earlier decision, and its cost is real and now paid: on a very wide display the chrome is wider than the app it labels — the same relationship the site header has to every page's content column. The cap's value, its var() read, its fallback and the full-bleed opt-out ledger are all unchanged. --app-page-max-width is still declared once in globals.css and still overridden per-app ON THE FRAME, from which it inherits to the content wrapper, so ledger rules keep working with no selector change. RECENTLY RUN. The chrome already rendered an app icon when the entry had one. The gap was upstream: the run page — the one writer that means "the viewer actually RAN this app" — never recorded an icon, because nothing on its SSR path had read app_listings for media. So apps merely opened from the store showed their real icon and the apps a viewer actually runs showed a placeholder. It is the listing's icon, not the manifest's: this chrome is the spoof-proof surface and already launders the app name through sanitizeAppChromeName, so a publisher-supplied image with no review step has no business beside it. The read joins the existing Promise.all (slug-keyed, so no serial hop on the app-launch path) and fails open to null, because createServerSideProps has no try/catch above it and a rejection would be a 500 on the page that runs the app. TESTING. Every guard was watched fail on pre-change code. Three adversarial audit rounds ran; the ladder stopped when two consecutive rounds' fixes changed zero payload lines. Round 1 found a WCAG regression and that the layout change had no gating-tier coverage at all — two mutants, including one that rendered the app as a sliver, were green across 24,879 node tests. Rounds 2 and 3 found that the guards added to close that were themselves blind to the mutations they were written for: first an offset comparison that any sibling satisfied, then an AST helper that returned '' for an unreadable style against a .not.toContain assertion. Both now fail loudly, verified against five mutants each dying for its own reason. No round found a production defect after round 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YVStb9vhnKD2oybCWdJGUB |
||
|
Justin Maier
|
f390cd4b30 |
fix(feature-flags): coerce the flag in every query gate, and guard the shape (#4587)
* fix(feature-flags): coerce the flag in every query gate, and guard the shape
`enabled: features.X` does not disable a query. Two facts combine, and
neither is visible at the call site: `FeatureAccess` is SPARSE at runtime
while its type says dense, so an absent flag reads `undefined`; and React
Query resolves `enabled` as `!== false`, so `undefined` is ENABLED.
The flag-off query therefore fires. The component still hides, because
`if (!features.X) return null` is fine on `undefined` - only the request
survives. A kill switch that stops no traffic is worse than no kill
switch, because the next incident's flag flip looks like it did
something.
Note what this is NOT: a type error. `FeatureAccess` is declared
`Record<FeatureFlagKey, boolean>`, deliberately, so consumers can write
truthy checks without coercion - which is also why a lint rule keyed on
types would find nothing here and why the guard is a text scan.
21 call sites, not the 18 a `enabled: features.` grep finds. The guard
found three the grep could not, all of the same shape: the `!!` is
present, just not on the flag.
enabled: !!currentUser && features.buzz useBuzz.ts:79
enabled: !!accountId && features.buzz useBuzz.ts:102
enabled: isActualOwner && features.articleRatingDispute
articles/[id]/[[...slug]].tsx:218
Those are `undefined` whenever the flag is absent, exactly like the bare
form, and they read as already-guarded.
The guard is a scan rather than a lint rule, and its positive control is
the point: the broken and correct spellings are exercised against text
the test owns, because a pattern that silently stops matching reports a
clean tree forever. It understands `!!(a || b)` (which coerces every read
inside the group) and skips comment lines - both were false positives it
raised against real files before it did.
ClickUp: 868kw8959
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0196Bp9Xe4LdQ5YSt2MAusWT
* fix(feature-flags): catch the indirection form, and floor the scan per extension
Review found the guard reporting clean over ten more instances of the
same bug, and a scan floor that could not catch its own likeliest
regression.
The indirection form is the bug one step out: the flag is read into a
variable and the gate is passed on as `{ enabled }`, so a scan anchored
on the `enabled:` key never sees a flag. One of them was
`YellowBuzzMigrationNotice.tsx:23` - the line that actually gates the
query, one line above a site the first revision fixed. Coercing
`useCreatorAnnouncementsFeature()` at its source closes four call sites
at once.
The scan floor asserted `scanned > 3000` against a tree of ~3950 `.ts`
plus ~1860 `.tsx`. Dropping `.tsx` from the glob - the likeliest
regression, and the half where every React Query gate lives - still
cleared it. Now floored per extension.
Widening the pattern immediately found a tenth site and two FALSE
positives of its own: `!!ctx.features.X` is coerced, but a bare `(?<!!!)`
lookbehind reads only the two characters before `features.`, which are
`x.`. The lookbehind now spans an identifier chain.
`TrainingSelectFile` stays UNCOERCED behind an explicit
`no-untruthy-query-gate-exempt:` marker. Coercing it removes live
orchestrator run state from non-mod trainers - `getRunState` is gated on
`imageTraining`, not on this flag - and whether the stored-metadata
fallback is good enough for an in-flight run is a product call, pending.
The marker exists so the site stays visible rather than the pattern being
narrowed to dodge it.
Severity, corrected: these gates are inert today rather than leaking.
Flipt is authoritative over the `availability` role check, and every flag
behind them is Flipt-enabled with a public default of true, so reading
`['mod']` in the registry tells you what happens when Flipt is
unreachable, not what is happening. They would fail silently the first
time someone flips one.
ClickUp: 868kw8959
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0196Bp9Xe4LdQ5YSt2MAusWT
* test(feature-flags): ratchet the exemption, and read the comment block not a window
Review found the opt-out re-opening the door the guard exists to hold
shut. Pasting the marker above any uncoerced gate turned the guard green
and failed nothing - an exemption nobody can see is the same as narrowing
the pattern, one line at a time. `scan` now collects exempted sites and
asserts the exact list, so adding one is a red diff. Mutant: silencing a
real gate prints `expected [ ...(2) ] to deeply equal [ Array(1) ]`.
The marker also exempted a WINDOW rather than a site - six preceding
lines, code in between and all, so one marker silently covered every gate
below it. It now walks upward through contiguous comment lines only,
stopping at the first line of code. That also removes an accidental
fragility: the one live exemption's reason runs five lines against a
window of six, one sentence from ceasing to apply.
Also documents the default-parameter sink - `useQueryFollowedAnnouncements
(enabled = true)`, where an absent flag selects the default and the gate
reads as ON rather than merely failing to disable. Its one instance is
closed by coercing at the source, but the shape carries no `enabled:` key
and the guard cannot see it.
ClickUp: 868kw8959
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0196Bp9Xe4LdQ5YSt2MAusWT
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
d49e281429 |
feat(apps): store card CTA fills the row, and Edit moves into a shared overflow menu (#4583)
* feat(apps): store card CTA fills the row, and Edit moves into a shared overflow menu
Two changes to the App Store listing card's action row, plus the consolidation
they forced.
1. THE PRIMARY CTA FILLS THE ROW instead of stepping up the Mantine size scale.
The next size up is 42px tall and the row's 46px height is load-bearing (it
sits in an h-full grid row, so a taller control grows every card in that row
across the store), so the only free axis is horizontal.
The obvious implementation is wrong: a bare flex-grow on the CTA eats all the
free space at EVERY width, starving the recommend rollup that the existing
container query exists to protect. So the rollup's 70px floor — until now only
the arithmetic behind that query — becomes an enforced min-width, and the CTA
takes the remainder. Measured at a 462px container: rollup 95.7 (its natural
width), CTA 310.3.
2. EDIT MOVES INTO A ⋮ OVERFLOW MENU. The card carried two Edit controls — a text
Button and an icon-only ActionIcon — swapped by an @[360px] container query.
Both are gone and so is that breakpoint: a ⋮ trigger is a fixed 36px at every
width, so the query had nothing left to decide. It is DELETED rather than kept
as a constant nothing needs.
3. THE MENU IS SHARED, NOT COPIED. AppListingDetailBody already shipped this
menu; a second copy is how the card and the detail page drifted over the CTA
glyph mapping, which had to be extracted to appListingActionGlyph.ts after the
fact. The whole thing — item set, order, labels, four eligibility predicates
and four modals whose mount site is load-bearing — moves to
AppListingActionsMenu, and the detail body now renders it.
GEOMETRY, RE-DERIVED FROM MEASUREMENT (not re-recorded)
threshold = actions(184) + row gap(10) + rollup floor(70) = 264
actions 184 is measured: a 36px ⋮ + the row's 10px gap="xs" + the widest CTA at
its natural 137.9px. It happens to equal the pre-change value because the
control the menu replaced was an icon Edit at the same size={36} — named in the
code, because "the number did not move" is also what an unmeasured number looks
like.
Measured (menu card, widest CTA, no reviews):
card | container | actions nat/rendered | rollup | row h | rollup
280 | 248 | 184 / 248 (grown) | 0 | 46 | HIDDEN
296 | 264 | 184 / 184 | 70.1 | 46 | at FLOOR
314 | 282 | 184 / 184 | 88.1 | 46 | clamped
494 | 462 | 184 / 356.3 (grown) | 95.7 | 46 | natural
The 264 row is the model and the measurement agreeing to 0.1px. Row height is
46 in every cell.
The three constants and the arithmetic now live in appListingCardView.ts and are
gated in the BLOCKING node project: the sum, the rounding direction, and a
source read asserting the component's @[264px] class spells the same number (a
Tailwind arbitrary variant cannot read a JS constant, so the duplication is
unavoidable — the drift is what gets gated).
GATING, AND A CONSEQUENCE WORTH READING
The menu renders when it would hold at least one item — the detail page's own
predicate, now written once in useAppListingMenuGates. A signed-OUT viewer gets
no menu, so their card is byte-unchanged (pinned: actions 138, rollup >= 130).
But useCanReportListing is !!useCurrentUser(), so an ORDINARY SIGNED-IN SHOPPER
does get a menu, and their action cluster goes 137.9 -> 184 like an owner's.
That is a real change on the most common path. It is measured and pinned in a
test rather than left to a screenshot; narrowing it (dropping review/report from
the card's copy) is a one-line change to useAppListingMenuGates.
A moderator viewing someone else's card also gets a menu, and does change
geometry. Accepted, and stated in a comment.
AppListingCard gains a `preview` prop, passed at OffsiteReviewQueue's two card
call sites. Without it the moderator reviewing an UNAPPROVED shadow listing —
who is by definition a moderator — would be offered live takedown actions
against a listing whose status and whose id are both unguaranteed.
TWO THINGS MEASURED RATHER THAN REASONED
- Menu.Target > Tooltip > ActionIcon SILENTLY BREAKS THE MENU. Both clone their
child and Tooltip overrides the ref, so the trigger stops opening the
dropdown. A 2x2 probe (tooltip x stopPropagation) failed both tooltip-inside
arms and passed all four with Tooltip wrapping Menu.Target. Several other
files in this repo use the broken order; out of scope here, not fixed.
- The modals mount LAZILY, on first open. The grid renders ~24 cards, and four
modals each would be ~96 modal subtrees and ~24 matchMedia listeners for a
viewer who will open at most one. Every one of them is reachable only from
inside this menu, so nothing is lost.
LEDGERS RE-POINTED (each had to be edited, which is the ledger working)
appModeratorMessageForm.callSites, appListingReportCallSites and
appListingDetailModalPlacement all read AppListingDetailBody.tsx as the mount
site. The mount moved with the menu, so they now read
AppListingActionsMenu.tsx. An extraction that had silently dropped the wiring
would have shown up in them as a SHRINK.
Six browser suites mock FeatureFlagsProvider wholesale naming only
useFeatureFlags; the card's graph now reaches useOptionalFeatureFlags, which
took the whole component run down with an unattributable mocking error. Fixed by
naming BOTH hooks, NOT by an importOriginal spread — that was tried and moves
the same failure one module over, exactly as
AppBlocks/__tests__/featureFlagsMockCompleteness.test.ts documents.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NBUj8SH7D2H8NTgHW1hDHm
* fix(apps): the store card's overflow menu is owner + moderator only
The previous commit put Review and Report into the card's copy of the shared
overflow menu. useCanReportListing is !!useCurrentUser(), so the menu's "would
it hold at least one item" predicate resolved TRUE for every signed-in viewer:
an ordinary shopper got a menu, and their action cluster went 137.9 -> 184 like
an owner's. Not intended. This narrows the CARD to owner + moderator and leaves
the DETAIL page exactly as it is.
THE GATE SHAPE, AND WHY IT IS A SURFACE NAME RATHER THAN A BOOLEAN
New pure module appListingMenuSurface.ts owns the one difference between the
two surfaces; AppListingActionsMenu takes a required `surface: 'card' |
'detail'`, and the card's copy AND-s the surface term onto canReview/canReport.
A prop like viewerActions={false} would put the POLICY at each call site, so a
third surface would spell its own answer and the two could disagree — the
predicate-duplicated-across-call-sites shape this shared module was extracted
to end. Naming the surface leaves the call site saying only WHERE it is; the
module says what that means, in one place, and a node test reads both call
sites to check they spell it.
Required, no default: a default silently picks a policy for a call site whose
author never considered it, and the wrong direction ('detail') is the one that
hands out the viewer actions. tsc now asks at the moment a third surface
appears.
Both hooks stay called UNCONDITIONALLY and the surface term is applied after.
`offersViewerActions && useCanReportListing()` short-circuits past a hook;
confirmed against eslint, which reports react-hooks/rules-of-hooks on that
form. The term is AND-ed, never OR-ed, so a surface can only ever DROP an item
its eligibility predicate already admitted.
THE TESTING, WHICH IS THE POINT OF THIS COMMIT
The defect shipped because the toBe(138) action-row guard ran SIGNED-OUT only.
A signed-out viewer had no menu before the change and none after, so that
assertion was structurally incapable of seeing the 137.9 -> 184 shift that had
just landed on every signed-in shopper. It stayed green through exactly the
regression it reads as covering.
The repair is not a second copy of the numbers — that reproduces the failure
one viewer over. Both geometry guards are now ONE assertion body run over both
viewers (signed-out, signed-in non-owner non-moderator), so the claim is "these
two measure identically" rather than two independent claims that happen to
share literals. Plus a DOM-level card test for the signed-in shopper, and an
owner arm as its positive control.
WATCHED FAIL at
|
||
|
Justin Maier
|
279a8bd404 |
feat(announcements): attribute creator announcements in the notifications panel (#4585)
* feat(announcements): attribute creator announcements in the notifications panel A creator-authored announcement rendered with no author at all — title, body, cover and nothing else — so in the notifications panel, where creator and Civitai cards are interleaved in one list, it was visually indistinguishable from an official Civitai announcement. The author now sits in a bar across the top of the card frame: avatar, linked username, posted-at on the left; the options menu and a dismiss button on the right. Creator announcements are dismissible for the first time, and the delete action moved from a loose icon into the options menu. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * test(announcements): cover the author identity the select cannot reach Both new cases fail if `withAuthorIdentity` is dropped from the feed path; the suite was previously blind to it because every feed fixture omitted `user`, so the helper only ever took its empty branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * fix(announcements): act on the five review lanes - one `DeleteCreatorAnnouncementButton` with `as`, following HideUserButton, so the two chromes cannot differ on permission, confirm or in-flight state - the byline fails CLOSED: an author-less row keeps a top bar rather than rendering in the exact shape of an official Civitai card - the cover divider is gated on the same container query that hides the cover - the options menu no longer disappears with the author, so a moderator keeps delete on the row most likely to need it - the profile query no longer fetches author identity it never renders Tests: the "no creator byline" case rendered only one source and passed with the whole feature deleted; the tRPC mock's spreads were inert over a Proxy and its comment said otherwise; the dismissal store was never actually reset between tests - measured by running the dismissal test first, which failed four others. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * test(announcements): pin the fail-closed byline An author-less row must keep its top bar. Nothing covered that: the guard added in the previous commit could be reverted with every test still green, and the failure direction is the one this feature exists to prevent - a creator announcement rendering in the exact shape of an official Civitai one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * fix(announcements): bust the followed feed when an announcement is deleted Deleting from the notifications panel reported success and left the card on screen: only getCreatorAnnouncements was invalidated, while the panel reads getFollowedAnnouncements, and trpc sets staleTime Infinity with refetchOnWindowFocus false. A second click on the same card then reported failure. The mute mutation beside it already busts both feeds. Pre-existing - the panel had a delete control before this branch - but the options menu is now its permanent home there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * test(announcements): cover the prune effect, and stop overclaiming in its comment The comment on the guard test said deleting the effect would fail it. It would not: with the effect gone nothing prunes, the dismissal stays, and the assertion still holds. Separate arm added for the effect itself, and the reset in beforeEach no longer resets by pruning against an empty live set - the behaviour the panel exists to guard against, which would make the reset a silent no-op the day that guard moves into the store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * test(announcements): pin that both feeds are busted on delete The panel's browser test stubs useDeleteCreatorAnnouncement out entirely, so the invalidation set is unobservable there by construction. This asserts both invalidate calls by name - asserting only the profile feed passes against the bug it covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * test(announcements): pin who is offered the delete control The gate was rewritten this round when the two chromes collapsed behind `as`, and mutating canDelete to `!!currentUser` reddened nothing in the repo. Four cases, positives paired with negatives so "absent" means the gate refused rather than that nothing rendered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * test(announcements): make the delete-gate negatives observable The first version read `elements()` straight after render, which returns 0 because the commit is asynchronous rather than because the gate refused - measured: widening canDelete to `!!currentUser` left all four green. Awaiting a sibling marker proves the tree committed before the absence is read; the same mutant now fails, naming the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * test(announcements): tighten the byline tests Pin that the author-less fallback label is absent when an author exists - the other branch of the same ternary, so rendering both would have passed. Plus two nits from review: type the fixture overrides so a typo'd key cannot silently render the ordinary fixture, and use Object.hasOwn in the tRPC stub's Proxy so prototype members are not returned instead of throwing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 * test(announcements): spread the real trpc module in the mutation test The hand-written factory tripped no-wholesale-module-mock, correctly: the day '~/utils/trpc' gains an export the factory omits, the file fails to load and collects zero tests while reading as green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vAEASSgSXDKe2gZ9f7FU9 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
88fddd91d7 |
fix(notifications): stop the reaction milestone counting suppressed accounts (#4586)
`createReactionNotification` judged the milestone on a raw `dbRead.imageReaction.count()`, which counts every row. Every number a user is shown filters `metricExcludedUsers`, the reaction-farm suppression list — every ClickHouse aggregate does, and since #4584 so does the Redis metric cache. So the milestone fired on a number no displayed count agreed with, at thresholds of 5/10/20/50/100 where the divergence is largest. That mismatch is what a user reported, and it is why 868kuyu5t was blocked on 868m0a19b rather than "fixed" by making the notification read the aggregate. The count now subtracts the same list, fetched through the Redis cache. Reading the ClickHouse aggregate instead was considered and rejected. It fails two ways this does not: `getImageMetricsObject` soft-fails to `{}`, so an unavailable read becomes a SILENTLY SKIPPED notification, and the aggregate carries forward-only staleness (`entityMetricTotal_v3` recomputes only for entities in `entityMetricDirty_v3`), so a milestone could fire on a total the aggregate itself would not support once refreshed. `getMetricExcludedUserIds` therefore never propagates a failure. `fetchThroughCache` rejects when the origin fails with nothing cached, and the caller runs as `.catch(handleLogError)`, so propagating would reintroduce exactly the silent skip. It also validates that the cached value is an array: `fetchThroughCache` returns any present `data` unvalidated, and a cached `null` would reach the caller's `.length` OUTSIDE the catch, throwing into the same silent skip. Both paths return an empty list, which means an unfiltered count — the behaviour before this change. Degrade to the old bug, never to silence. Pinned by tests that force each failure. Known and deliberately not addressed here: - Filtering can LOWER the count, so `match` can move down. Where a burst crossed several thresholds at once only the top key was written, so a lower milestone can arrive after a higher one. Guarding it means one `notificationExists` HTTP call per higher threshold, re-run on every reaction to exactly the images this suppression targets — a standing cross-service N+1 for a cosmetic ordering quirk. It needs a batched existence check first; the spoke has none. - An exclusion later reverted does not restore a withheld milestone: evaluation only runs when a reaction is created, so if no further reaction lands, the owner never receives it. Pre-change they would have. - No reaction-type filter. The milestone counts `Dislike` while the display does not, but `availableReactions` is a flat map that INCLUDES `Dislike`, and the four-reaction set exists only as a hand-written literal in five places, so closing it here means a sixth copy. Inert today (0 Dislikes in the last 200,000 reaction rows). - Nothing is repaired. Milestones already delivered stand. The list is fetched with an explicit TTL of `CacheTTL.sm`. That is also `fetchThroughCache`'s default; it is passed anyway so a change to that default cannot silently widen this past the "within ~5 min" the admin endpoint promises. `SELECT userId FROM metricExcludedUsers FINAL WHERE active = 1` now exists in three places in `src/`: this service, `metric-reaction-repair.service.ts` and `contest-score.queries.ts`. The other two are deliberately NOT converted — they must throw rather than silently see an empty list, since one writes compensation rows at scale and the other feeds contest disqualification. The failure modes differ on purpose; consolidating them would be the defect. Refs ClickUp 868m0a19b. Unblocks 868kuyu5t. Claude-Session: https://claude.ai/code/session_015Pcj1JeRGSkvVo7f3Ek6q2 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
ad8135eba7 |
fix(jobs): fence remove-old-drafts on version and file activity (#4579)
* fix(jobs): fence remove-old-drafts on version and file activity
remove-old-drafts deletes any Draft/Deleted model whose Model."updatedAt"
is older than 30 days and whose download count is under 10, and that DELETE
cascades to every ModelVersion and ModelFile under it. It is irreversible.
Model."updatedAt" is a Prisma @updatedAt column, so it moves only when a
client writes the Model ROW. None of the things that actually happen to a
draft write that row:
- upsertModelVersion writes ModelVersion, never the Model
- training completion writes ModelVersion only
- file upload writes neither
- updateModelLastVersionAt returns early unless a Published version
exists, so it never fires for a draft
- the trainer UI skips the model write when the form is not dirty
(TrainingBasicInfo), which makes the miss the default rather than the
exception
So on a draft the clock freezes at "creator last edited the model's
metadata" while the finished resource lands hours or weeks later as a
ModelFile. Model 2831418 is the exhibit: createdAt == updatedAt, version
3194997 updated much later with trainingStatus Approved, a 272 MB model
file created hours before the reap, and a 113 MB training-data file with
dataPurged=false. It matched the predicate exactly.
Measured on a night's candidate set: 1,103 models across 691 users, of
which 139 had version-or-file activity inside the 30-day window and 138 of
those still held a training dataset.
Two fences, because the delete cannot be undone:
1. The replica SELECT gains a Private exclusion (matching how
reset-to-draft-without-requirements spells it) and two NOT EXISTS
clauses covering ModelVersion."createdAt"/"updatedAt" and
ModelFile."createdAt". Both resolve by index; measured cost on the real
candidate set was 345 ms before and 327 ms after.
2. filterModelsWithRecentActivity re-checks the same rule on the primary,
on rows read inside the batch loop, immediately before the DELETE. The
SELECT runs against the read replica minutes earlier, so it cannot see a
version or file written during replica lag or between the SELECT and the
DELETE. It needs no extra query: the per-batch ModelVersion lookup that
already ran to collect version ids now also returns the timestamps. It
fails closed on an unreadable required timestamp, and on a row it cannot
attribute to a model it refuses the whole batch rather than silently
protecting nothing.
Fencing on the newest ModelVersion."createdAt" alone does not work, and
neither does bumping Model."updatedAt" on version create: the exhibit's
version was created in the same minute as the model. Only a fence that
reads ModelFile."createdAt" or ModelVersion."updatedAt" sees it.
The job also now logs which models it destroyed. It previously logged
counts only, so a "my model vanished" report was unreconstructable. One
Axiom event per batch carries that batch's model ids and their owners,
bounded by BATCH_SIZE at 10 ids per event; skipped models and failed
batches are named the same way. Only the versions of models actually
deleted are handed to the storage-resolver deregister, so sparing a model
no longer drops its objects out of the quarantine allowlist.
* fix(jobs): pin the loss-report ids, and decouple the reap age from the fence window
Audit round 1 on this PR found three things. None changes what the job deletes;
all three are about guards that read as coverage while providing none.
1+2. The two Axiom events that make a "my model vanished" report answerable were
unpinned. Mutating `modelIds: deletable` -> `batch` (the destroyed-ids event)
and `modelIds: skipped` -> `batch` (the spared-models warning) both SURVIVED a
fully green 31/31 suite. Every existing test of those events uses a batch in
which deletable, skipped and batch are the same list, so each was satisfied by
logging any of the three. The one mixed-batch test asserted deleteBatches()
and the deregister call but never read either event.
Both assertions now live in that mixed-batch test, the only place the three
lists differ. Verified: each mutant now dies, and dies to its OWN assertion
message, not collaterally to the other.
3. The seam guard was steering a future edit into the deletion threshold.
`expect(intervals).toEqual(Array(4).fill(INTERVAL '${ACTIVITY_WINDOW_DAYS}
days'))` pinned all four literals to one constant -- including
m."updatedAt" < now() - INTERVAL '30 days', which is the abandonment
threshold, not part of the fence. Narrowing the fence to 7 days would have
turned that guard red, and the obvious way to make it green again is to
rewrite the age clause too -- which does not narrow the fence, it widens what
the reaper DESTROYS, from untouched-for-30-days to untouched-for-7-days, with
the suite green. Widening was safe; only narrowing was dangerous, and nothing
marked the asymmetry.
Split out REAP_AGE_DAYS and assert the three fence clauses and the age clause
separately, against separate constants, plus a count guard so a fifth interval
cannot escape both. Verified decoupled: a mutant on the age literal now kills
only the age guard, and a mutant on a fence literal only the fence guard.
Mutation results, re-run after the fix (33 tests, both files):
delete `batch` instead of `deletable` DIES (positive control)
revert the deregister scoping to all versions DIES (positive control)
destroyed-ids event -> `batch` DIES (was SURVIVED)
spared-models event -> `batch` DIES (was SURVIVED)
fence interval 30 -> 90 days DIES, fence guard only
age interval 30 -> 7 days DIES, age guard only
add a fifth INTERVAL clause DIES, count guard
eslint and prettier clean on the changed files (prettier negative-controlled).
NOT addressed, deliberately, and left for the reviewer: a model that is both
`Deleted` and `Private` is now unreapable by any job in src/server/jobs -- the
`availability != 'Private'` exclusion combined with remove-old-drafts being the
only reaper of `Deleted` models. The PR body frames the exclusion as covering
private DRAFTS; the Deleted half is unstated and carries a data-retention edge,
since the user explicitly deleted those. Narrowing it to
`NOT (availability = 'Private' AND status = 'Draft')` would restore reaping for
that set, but that re-enables a deletion path and is not a change to make
unilaterally in a PR whose purpose is to stop over-deletion.
* fix(jobs): match SQL guards against executable SQL, and correct two comments
Audit round 2 findings. No behavioural change to the job.
1. 🟡 The interval guards matched raw SQL, so a `--` comment counted as a clause.
Round 2 measured it: adding an explanatory comment that merely MENTIONS an
interval -- e.g. "-- fence: spare anything whose version moved within
INTERVAL '30 days'" -- turned the count guard red with "expected 4 but got 5"
under a title claiming a clause had escaped. No fifth clause existed. This is
not hypothetical: the previous commit added a SQL comment two lines above the
age clause and avoided the trip only by not writing the literal.
The same un-stripped read was wrong in the other direction too: a comment
quoting a clause's exact text satisfied a `toContain` guard with the real
clause deleted. That was backstopped by the whole-predicate pin, so it was
latent rather than live.
Extracted `readExecutableSql()` -- the stripping already existed inside
`readPredicate()` -- and pointed every SQL-text guard at it, so there is one
rule in one place rather than two conventions in one describe block.
2. 🟢 `REAP_AGE_DAYS`'s doc claimed "It is read only by the m.\"updatedAt\"
clause in the SELECT below". A SQL literal cannot read a TypeScript constant.
The constant has ZERO runtime readers -- its only references are in the test
file. Rewritten to say what it actually is (a documentation anchor the test
pins the literal against) and to name the asymmetry with
ACTIVITY_WINDOW_DAYS, which IS read at runtime. The test-file header made the
same wrong claim about "the constants the TypeScript fence uses" and is
corrected alongside it.
3. 🟢 The previous commit's counterfactual said narrowing the fence would go
"with the suite green". The whole-predicate pin would also have gone red in
that world, so the hazard was one step longer than stated. The hazard itself
is unchanged -- that red names no clause, and the repair it invites is the
dangerous one -- but the comment now says so.
Verified by mutation (33 tests, both files, green baseline first):
a doc comment MENTIONING an interval 20/20 PASS (was red before: the finding)
a real fifth interval clause DIES -- count guard + predicate pin
comment quoting a clause, real one deleted DIES -- fence guard + predicate pin
delete `batch` instead of `deletable` DIES -- its own assertion
destroyed-ids event -> `batch` DIES -- its own assertion
Repo-invariant lint-rule suites: 5 files / 200 tests pass. eslint rc 0 with zero
output; prettier clean on both files.
CORRECTION to the previous commit's mutation table, which overstated two rows:
"age interval 30 -> 7 days -- DIES, age guard only" and "fence interval 30 -> 90
days -- DIES, fence guard only" each in fact fail TWO tests, their own guard plus
`pins the whole predicate`. The decoupling claim those rows support is unaffected
and holds in both directions -- an age mutant never trips the fence guard and
vice versa -- but "only" was wrong and a mutation table in a commit body is
exactly what a later reader cites instead of re-deriving.
* test(jobs): stop readExecutableSql claiming to be a SQL parser
Round-3 audit finding, comment-only. The helper's doc said its output is "the
SQL the database actually executes". It is not: `/--[^\n]*/g` strips to
end-of-line unconditionally, including a `--` inside a quoted string literal,
which PostgreSQL does not treat as a comment. Measured -- a JOIN line carrying
`m.name NOT LIKE '%--%'` followed by a real fifth INTERVAL clause is fully green
at 33/33, while the pre-round-2 raw-SQL version of the count guard catches it.
So repointing the guards at stripped SQL traded the false-RED it was fixing for
a narrow false-GREEN, and the doc asserted a guarantee the code does not carry.
The trade is still net-positive and the trigger is contrived -- it needs a
`--`-bearing string literal, on one line, before a new clause, OUTSIDE the
`WHERE ... ORDER BY` slice, since inside it the whole-predicate pin fails loudly
-- and nothing in this SELECT is near that shape. Documenting the limit rather
than parsing SQL in a test helper, with the remedy named for whoever hits it.
No executable change. 33/33 on both files; full `test:lint-rules` 28 files / 448
tests rc 0; eslint rc 0 zero output; prettier clean.
Also corrects this PR's own reporting: an earlier round quoted the lint-rule
suites as "5 files / 200 tests", which was the subset that round named, not the
repo's `test:lint-rules` script. The real figure is 28 files / 448 tests.
|
||
|
Justin Maier
|
63987c09d1 |
fix(rewards): floor the multiplier where it is spent, not only where it is recorded (#4582)
* fix(rewards): floor the multiplier where it is spent, not only where it is recorded #4572 floored the ClickHouse audit row. The award computation stayed unfloored: `rewardsMultiplier` comes off operator-authored `Product.metadata` as a raw float, and `Infinity` reaches the Redis Lua cap script as the string "Infinity", whose `tonumber` is nil — the arithmetic on nil throws out of `redis.eval` and into the user mutation that triggered the reward. A negative is worse than it looks: the event is recorded `capped` and pays nothing, but the Lua has already written the dedup entry as `a:-200`, which the reader pattern `^a:(%d+)$` cannot match, so every later read of that entry falls back to the current award for the rest of the UTC day. `clampRewardMultiplier` floors at 0 and falls back by sign for a non-finite value. It deliberately carries NO ceiling and is deliberately not `clampBuzzEventMultiplier`: that one holds the buzzEvents column 9.99, which is right for an audit row and wrong for a value `sendAward` pays from, where gold at 4x a MAX_GLOBAL_BONUS of 5 is a legitimate 20. Applied where the value is SPENT rather than where it is read, so the event keeps the raw multiplier and `toClickhouseBuzzEvent` can still record `multiplierRaw`. Clamping at the read destroyed that audit trail and reddened seven tests in base.reward.forid.test.ts. Dormant in prod today: all 17 products carrying the key are plain numeric, none negative. ClickUp: 868m06pn5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0196Bp9Xe4LdQ5YSt2MAusWT * fix(rewards): coerce before the finite check, and drop the purchases floor Two money bugs found by review of the first commit. The clamp tested its argument with `Number.isFinite`, but on the batch path `event.multiplier` is read back out of a ClickHouse `Decimal(3, 2)` where it is number-typed and string-valued. `Number.isFinite("4.00")` is false, so a legitimate 4x took the non-finite fallback and paid 1x. That is the same underpay `f450100aba` fixed in `toClickhouseBuzzEvent`, reached through a different reader - this was the third reader of that value and the only one at a site that pays. Coerce inside the helper so every call site is covered and the `multiplier: number` signature stops implying something the runtime does not honour. `purchasesMultiplier` should never have been floored here. It feeds `getBuzzBulkMultiplier`, which is called unconditionally, so a 0 makes `mainBuzzAdded` `-buzzAmount` and `totalCustomBuzz` 0 - a completed Stripe/Paddle/NowPayments purchase credits nothing, and `completeStripeBuzzPurchase` then writes the `transactionId` the early return uses as its idempotency marker, so a retry cannot repair it. A 0 is meaningful on the rewards side (`rewardsIneligible`) and meaningless on the purchases side. That path needs its own floor, decided on its own terms; both floors are reverted here and tracked separately. Also: the fold's two `Math.max` merge arms had no test - every case gave its user one row, so reverting only those arms left the suite green while `Math.max(1.5, NaN)` is NaN. And a comment claimed the bonus-event test caught a floor-to-1 implementation; it does not, and now records the reason that holds. ClickUp: 868m06pn5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0196Bp9Xe4LdQ5YSt2MAusWT * test(rewards): pin the two decisions that were written down but not asserted Review round 3, all comment and test changes - no behaviour change. The purchases NON-floor is the reverted regression from round 2, and it was stated in three comment blocks and pinned by no assertion. Every `purchasesMultiplier` fixture in the suite was 1, 1.05, 0.5 or null, and the clamp is the identity on all of them, so re-adding the floor at both sites passed all 54 tests. It now fails with `expected +0 to be -3`. `globalRewardsBonus`'s finite check and [1, 5] clamp were deletable green while a new comment cited that guard as the reason the other half needed no test. The fixture's multiplier of 20 resolves to 2, already inside the range, so the clamp was inert on the only input any test supplied. At 200 it resolves to 20 and the clamp has to do something: `expected 20 to be 5`. Also, the round-2 change made a round-1 comment false. It claimed the event keeps the raw multiplier so `toClickhouseBuzzEvent` can record an operator typo as `multiplierRaw` - but `getMultipliersForUser` now clamps at the read, so `event.multiplier` is always clamped by the time the event is built. The clamps at the spending sites are still worth keeping, for a narrower reason that is now what the comment says: they cover a value that did not come through `getMultipliersForUser`, such as a `pending` row written before this shipped and read back by `process`. Plus: a stale "FOUR places" count left by the purchases revert (it is two); the quoted-multiplier test asserting its own precondition rather than commenting it, the way its sibling does; a unit-level assertion for the `Number()` coercion, which was reachable only through the three-mock integration test; and a negative case dropped from the merge-arm test because `Math.max(1.5, -1)` is 1.5 floored or not, so it could never carry that mutant. ClickUp: 868m06pn5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0196Bp9Xe4LdQ5YSt2MAusWT * test(rewards): pin the purchases decision at BOTH producers, and the real reason for the clamps Review round 4. Comments and tests only. The purchases non-floor was pinned at `getMultipliersForUser` and nowhere else, and that test mocks `userMultipliersCache.fetch` - so the fold never runs in it. The symmetry edit, two lines below the comment forbidding it, stayed green: every purchases fixture in the fold's own file is 1.05, 1, 0.5 or null, and the clamp is the identity on all of them. Asserted now at the producer where the edit is actually tempting. The rationale for why the clamps sit at the spending sites has now been written three times and was wrong twice - a `multiplierRaw` audit trail (made false by this PR's own read-side clamp) and a pending row read back by `process` (impossible: `isProcessable = !isOnDemand`, so `process` never calls `processOnDemand`). The reason that holds: `getMultipliersForUser` floors the BASE and then multiplies by the bonus without re-clamping the product, so it can return a non-finite value built from two finite floored factors - `clamp(1e308) * 5` is `Infinity`, and no read-side clamp closes that because the overflow happens after it. Asserted rather than narrated this time. Two more arms of the bonus guard were deletable green, and a comment added last round cited that guard as the reason its half needed no test. `Math.max(raw, 1)` missing halves every reward on the site during a bonus event; the `Number.isFinite` arm missing makes every amount NaN, which `sendAward`'s own `> 0` filter drops - paying stops sitewide. Also: a stale mutant message left by the 20 -> 200 fixture move, a closing condition on the purchases test so the eventual correct floor updates it rather than being blocked by it, and an unreconstructible count dropped from a comment. ClickUp: 868m06pn5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0196Bp9Xe4LdQ5YSt2MAusWT * fix(rewards): clamp the third reader, and pin the three gaps the mutants missed Review round 5. `getUserRewardDetails` is the third reader of this multiplier and had no clamp: display rather than money, but `getMultipliersForUser` can return a non-finite product, and an advertised award of `Infinity` is still a bug. The first version of this clamp passed 204 tests with the clamp deleted - a clamp with no test, which is what the rest of this change argues against - so it now has one that prints `expected Infinity to be 100` on revert. The purchases non-floor was pinned on the fold's first-row arm only. Flooring just the merge arm passed all 19 tests in that file, which is the same shape the file's own comment records catching once already on the rewards side. `Math.max(1.05, NaN)` is NaN, so that arm is what a paying member with one bad row actually gets. The bonus-event banner gate had no coverage anywhere in the repo: `grep -rln rewardsBonusEvent src --include=*.test.ts` returned nothing. Widening it to `>= 1` advertises a running bonus event to every user for an event that multiplies by 1, and the new bonus test already sets up that exact state. And the rationale for WHERE the clamps sit was wrong a third time: "no read-side clamp closes that" is false, because the overflow happens inside `getMultipliersForUser` and so before `apply` reads it. The two questions are now separated - the overflow is why to clamp at all, and `multiplierRaw` audit fidelity is why not at the read - which is what the two comments were contradicting each other about. Also: `toBe(Infinity)` rather than `Number.isFinite(...)).toBe(false)`, which also passed if the key stopped being returned at all; and a closing condition on the non-finite pin so clamping the product later updates it rather than being blocked by it. ClickUp: 868m06pn5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0196Bp9Xe4LdQ5YSt2MAusWT * fix(rewards): clamp claimBuzz, the fourth reader - it pays Review round 6. `claimBuzz` reads `rewardsMultiplier` from `getMultipliersForUser` and multiplies a BuzzClaim amount by it straight into `createBuzzTransaction` with no clamp, so it inherits the same non-finite product every other site now clamps against - and unlike `getUserRewardDetails` it moves real money. The ticket named `claim/buzz/[id].tsx` as a payout site fed by this function; I had read that as context rather than as a site. Two comments claimed a complete census of readers ("the THIRD reader", "has THREE readers") and were wrong by exactly this one. Both are now scoped to the file they describe and name the fourth, because a reader who trusts a census does not go looking. Two test gaps closed, both one value: The display site's negative control used a multiplier of 4, and the suite itself proves the two clamp helpers agree on 4 - so swapping in `clampBuzzEventMultiplier` there, the one edit this module's loudest comment forbids, passed. At 20 it advertises 9.99x against the 20x `sendAward` pays, and reddens. The bonus-event banner gate had a control for widening it and none for narrowing it, so hiding the banner from every user for every event was invisible. Asserting the field rather than not-null closes both directions. ClickUp: 868m06pn5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0196Bp9Xe4LdQ5YSt2MAusWT --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
b4bad5d28f |
fix(event-engine): make the metric cache and live signals respect metricExcludedUsers (#4584)
The Redis hash `metrics:<entityType>:<id>` has two writers with different definitions of the same number. `MetricService` repopulates it on a cache miss from `entityMetricDailyAgg_v2`, which filters the reaction-farm suppression list `metricExcludedUsers`. `RedisCache.increment` / `incrementOnce` HINCRBY it per Kafka event and did not. So a farm reaction landed immediately and was stripped only when the 12h TTL lapsed: image 141298569 read 24 from the cache and 4 from ClickHouse, and which one a user saw depended on cache age. Measured on 150 images touched by an excluded user in the last 10 days (88 had a cached hash): Redis exceeded the aggregate on 43, with 11 at a ratio of 1.5 or more — redis 13 / view 2 among them. `metricSignals.sendDelta` is gated for the same reason. It broadcasts to a topic keyed by the same string as the cache key, so every viewer of the entity receives the delta; filtering the cache alone would push a +1 that no store holds and the count would revert on the next fetch. Excluded users' events still reach ClickHouse. The raw event table is what the aggregate filters FROM and what metric-reaction-repair reconciles against, so suppression belongs to the read side and not to the record. The list is mirrored into memory and refreshed every 5 minutes, matching what /api/admin/reaction-abuse already documents. It fails open: a ClickHouse error keeps the last good set, and a failure at boot leaves it empty, which is the pre-fix behaviour. Failing closed would drop every increment and freeze every displayed count. Two properties this does NOT have, stated so nobody assumes otherwise: - It repairs nothing. Counts already inflated stay inflated until their key is evicted and repopulated from the aggregate; `exclude` performs no cache purge. - Exclusion is forward-only in ClickHouse too — `entityMetricTotal_v3` recomputes only for entities in `entityMetricDirty_v3` — so an image whose farm reactors were excluded after its last reaction stays inflated there as well. Refs ClickUp 868m0a19b. Claude-Session: https://claude.ai/code/session_015Pcj1JeRGSkvVo7f3Ek6q2 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
ef7698fdf5 |
fix(licensing): judge the stored per-image fee stamp on saves that omit it (#4580)
* fix(licensing): judge the stored per-image fee stamp on saves that omit it `licensingSourceVersionId` is nullish in the version upsert schema while `baseModel` and `modelId` are required, and the lineage guard was gated on the submitted value. A payload that omits the stamp therefore skipped the guard entirely, and Prisma left the column alone — a stored stamp surviving the very change that invalidated it. `TrainingSubmit` sends that shape: it updates the first version with a newly chosen `baseModel` and no stamp field, so a creator who stamped a version and then re-ran training on a different base kept a third party's per-image fee. Seed the stored value into the guard when the caller sent `undefined`; an explicit `null` still clears. Measured on prod first: 0 rows are in that state today (2,124 stamped versions, 0 with a base model disagreeing with their root) and no base-model coercion has ever been recorded, so this is a reachable hole rather than a live one. Also adds `Model.type` to `watchedEntityFields` — changing it clears every stamped version beneath the model, and unwatched, the sweep that looks for why a fee moved sees the clears and nothing that caused them. The audit reads the type from the transaction rather than the replica, since a lagging read reports it unchanged and emits no row on exactly the save that needed one. Migration clears the 23 pre-`LicensingRoot` rows that 20260831200000_clear_orphaned_licensing_source deliberately left alone. Same predicate, bound inverted. NEEDS APPLYING BY HAND, and the repaired ids then POSTed to /api/v1/model-versions/bust-cache as a moderator. CU 868kzk4qn, 868kwf2fd, 868kzk4rr Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VJUGFrqYuNWTWCyTARTf4h * fix(licensing): pin the rest of the ladder the seed reopens, and stop the log claiming the client sent it Review found three things the first commit got wrong or left implicit. The seed does not only reopen the guard's base-model rung — it reopens the whole rejection ladder to saves that omit the stamp, `not-a-root` and `model-not-found` included. Both are now pinned by their own tests; only the model-type rung was before, so the other two were an undisclosed behaviour change. `model-version-licensing-source-rejected` carried `requestedSourceVersionId` straight off `input`, which after seeding may be a value no client ever sent. The log's stated job is to show whether clients are still submitting bad stamps, so a `seeded` discriminator keeps that readable rather than counting the server's own repairs as client traffic. The audit's transaction-read fix was pinned only by a source-text assertion on the variable's name, which `typeBeforeUpdate = beforeUpdate?.type` satisfies while putting both the audit and the repair back on the replica. Adds the behavioural test the existing harness was already set up for — the fixture's writer and replica types deliberately disagree, so a replica-sourced before-side emits no row at all. Also corrects both `selects every watched ... column` docstrings: a watched field missing from the before side is not skipped, it is written with a blank `oldValue` — the guard at entity-change-helpers.ts:107 covers the after side only. And bounds the span that guard slices, which went vacuous rather than red if the ternary it anchors on ever stopped ending in `: null;`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VJUGFrqYuNWTWCyTARTf4h * fix(licensing): pin the cleanup to the 23 ids, not to a date bound that cannot pin anything The first version reused the sibling migration's `createdAt` bound with the comparison inverted, and claimed that bound held the statement to the rows that were measured. It does not. `createdAt` is immutable, so a date bound caps the candidate pool; membership is decided by the `NOT EXISTS`, which reads `LicensingRoot` and `Model."type"` at whatever moment a human runs the file. Nothing in this workspace writes `LicensingRoot` — every reference across src, apps and packages is a read — so its rows are inserted and corrected out of band, during work like this ticket. One root deleted, or one root's `modelType` corrected, and pre-cutoff stamps that pass today stop passing with no version and no model having moved. The bound is blind to that because it constrains the wrong column. So the id list is the scope and the subquery is the rule. The subquery stays rather than trusting the ids alone: a row the app repaired in the meantime — this ticket's own guard now coerces a stored source on saves that omit it — fails `IS NOT NULL` and is skipped. The statement can therefore only ever clear fewer than 23, never more, whenever it runs. Verified as a SELECT against prod: the id-pinned predicate matches 23, and injecting a legitimately-stamped version id into the list still returns 23 rather than 24, which is the control that the subquery is doing real work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VJUGFrqYuNWTWCyTARTf4h * docs(licensing): say why the seed's `?? null` is dead, so it is not lifted out of its guard The `?? null` is unreachable — the condition it sits under has already established the stored column is non-null — and exists only because a `const` boolean does not narrow `storedVersion` for TypeScript. That makes the line look safe to lift out from under the condition, and a later edit relaxing it to "seed whenever the client said nothing" would then write an explicit null and clear a stamp. Without the `??` that edit would not compile; with it, nothing stops it but the condition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VJUGFrqYuNWTWCyTARTf4h * fix(licensing): correct two overstatements, and drop the source-text test its replacement dominates The header claimed nothing in this workspace writes `LicensingRoot`. A committed migration does — three `INSERT`s in 20260715130000_add_licensing_root_table — and that path is under `packages`, so the claim's own scoping did not save it. The argument it supports is unaffected and is stronger stated correctly: no APPLICATION code writes the table, and the only writes in the repo are that one-shot seed, so ongoing curation happens out of band, invisibly to `git grep`. As written, the next person to audit the choice greps the table, hits three INSERTs, and has grounds to discount the whole header. "Can only ever clear FEWER than 23" excluded the expected outcome. `mv.id IN (23 ids)` on a primary key caps the statement at 23, so the invariant is at most 23 — and 23 is what you get if no owner saves in the meantime. An applier who got 23 had been told by the file that something was wrong. Deletes `audits the type off the transaction read, not the replica one`. That is this file's own documented rule — when the behavioural test exists, the source-text one goes rather than joining it — and the domination was measured first, not assumed: the behavioural test fails on the plain revert the deleted one caught, and also on a spread-order swap it passed, since the literal survives while the spread overwrites the value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VJUGFrqYuNWTWCyTARTf4h * test(licensing): pin the audit test's replica side locally so a fixture edit cannot disarm it The divergence between the writer's type and the replica's IS the discriminator, and only half of it was visible in the test: the writer side was set locally, the replica side inherited from the shared `storedModel`. Normalising that fixture to LORA would have left the assertion passing and the mutation passing, and nothing else in the file would have noticed — `storedModel.type` feeds only this before-side, the repair gate reads the writer, and `versionAuditRows` filters Model rows out. Measured as a pair rather than argued. With the fixture normalised AND the audit's spread order swapped so `...beforeUpdate` overwrites the type: before this commit 11 passed (11) — mutant survives, test vacuous and green after this commit 1 failed (11) — mutant caught Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VJUGFrqYuNWTWCyTARTf4h --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
b4f3227f5f |
fix(generation): render MiniMax Music 3 output in the queue
The handler emits `$type: 'miniMaxMusic3'`, but two `$type` switches only knew
about `aceStepAudio` and default to image-shaped handling, so a completed song
produced an empty queue card with no error anywhere:
- `normalizeStepOutput` fell through to `default: return []`, leaving the step
with no outputs at all.
- `StepData.mediaType` returned 'image', which seeds `GeneratedOutputWrapper`'s
`loaded` false (audio never fires `onLoaded`) and lets the post/remix menus
treat a song as a postable image.
`MiniMaxMusic3Output` is `{ blob: AudioBlob }` with no cover-image variant, so
there is no video-container branch to mirror from ACE.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Briant Diehl
|
77760cefac |
Merge pull request #4551 from civitai/feat/orchestrator-step-queue-position
Read step-level queuePosition; release training gate by workflow id |
||
|
briant
|
4a8393e46f |
fix(training): scope the moderation gate to its own model version
Review pass over the step-level queuePosition migration.
- Bind the gate release to the `modelVersion:{id}` tag the submit path writes,
rather than to the workflow id carried in ModelFile.metadata. Both callers
refuse before the POST when the two disagree.
- Stop honouring a posted `modelVersionId` on `updateFile`. The lookup there
authorizes the file being edited, not a destination version; the upsert call
sites already send the file's existing version, so nothing changes for them.
- Percent-encode the workflow id in the manager URL.
- Surface a half-landed deny. `moderateTrainingData` now returns whether the
resource-training-v2 callback landed, and a gate that was released but not
recorded reaches the moderator as its own warning rather than plain success —
the run IS stopped, so this is not a failed deny and must not invite a retry.
A workflow reporting no status no longer skips the callback silently.
- Carry `step.queuePosition` through the signal update path, so the queue card's
position and ETA refresh instead of freezing until a reload.
- Pin @civitai/client to the published 0.2.0-beta.97 (beta.96 was never
published) and regenerate the lockfile.
Tests cover the gate request shape, the callback, the ownership refusal, the
signal merge and update scoping; each was checked against a mutation of the code
it guards rather than only against green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Manuel Emilio Urena
|
e6fe40b0f2 |
fix(civitai-link): drop the Stable Diffusion wording and fold the success modal into the wizard (#4576)
Two leftovers from yesterday's Link copy/UI pass. The worker's room-presence handler still emitted "Stable Diffusion service connected" / "disconnected" — the last Stable-Diffusion-specific strings in the Link surface, and the only ones a connected user actually sees. They now carry the instance's own name, so a user with more than one app can tell which one dropped: "Workstation connected". Falls back to "App" when the instance has no name yet, since the name arrives from the Link server on join and a rename propagates through the same `instance` state the toast reads. Reaching `connected` also popped CivitaiLinkSuccessModal on top of the wizard, which was a second modal asking for the name the wizard's own last step had just asked for. Step 3 now swaps its pairing block for the connected state in place: the check, the confirmation copy, and the AppRow preview, with the name field and footer carried over. A name typed before the app pairs survives into the success state instead of being re-prompted. The modal file is deleted; the `connected` effect advances to step 3 rather than opening a dialog, preserving the old behaviour of showing success from whichever step the user was on. Two things fixed while verifying it in the browser against a local link-service: `text-success-5` on the check icon is a dead class. tailwind.config.js defines no `success` color — it exists only in the Mantine theme — so the icon fell back to currentColor and rendered grey inside an already-dim ring, which is why the success state read as barely-there. It came over from the deleted modal, so it had never worked. Now the Mantine token (`--mantine-color-success-5`, #1EBD8E) with a heavier stroke, and the ring at 20% alpha to match designs/civitai-link.pen. IconBoxMultiple (two overlapping squares) did not match the canvas, which draws lucide `boxes` in all three places it appears — the path card, AppRow, and the popover's pitch chip. Swapped to IconPackages in all three. Verified end to end on a local link-service by pairing a real socket peer into the room: both toasts, the inline success state, and the rename propagating into the toast. Claude-Session: https://claude.ai/code/session_014hbyPW1XDB6HV3U8wXLXYf Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
baf5bc1959 | chore(moderator): release moderator-v0.0.54 moderator-v0.0.54 | ||
|
Zachary Lowden
|
d596565541 |
feat(apps): let an app paint its own boot state (manifest bootSkeleton) (#4563)
* feat(apps): let an app paint its own boot state (manifest bootSkeleton)
A host-drawn loading state can never hydrate without a visible change: it
renders in the HOST's theme with generic geometry, then cross-fades to the
app's own layout. Only a skeleton shipped by the app can be theme- and
geometry-identical to the thing that replaces it.
The blocker was that the host makes such a skeleton invisible. THREE things
hid it, and standing down any two of them still leaves the app's boot state
unseen:
1. the branded veil — opaque, `inset: 0`, until BLOCK_READY;
2. the iframe itself — `opacity: 0` until BLOCK_READY;
3. the reveal `translateY(8px)` settle — which is itself a layout shift, at
the exact moment an app-painted skeleton exists to avoid one.
`manifest.bootSkeleton: true` stands down all three for that app. Everything
else is unchanged, and NOT declaring it stays the safe default: no veil plus
an empty `#root` is a blank white iframe for 300-1200ms, which is worse than
what the veil was doing. The host's own skeleton remains the fallback for
every app that does not opt in.
`pointerEvents` is deliberately NOT opted out — a skeleton is not
interactive, and the block must stay inert until it holds a token.
Read from the APPROVED manifest snapshot and coerced with a strict
`=== true`, so publisher JSON carrying "false"/0/{} cannot switch a host
behaviour on. A false declaration is cosmetic and scoped to that app's own
page; the platform build will refuse a build that declares it with an empty
`#root` (talos-infra, separately).
This is inert until an app declares it — no app does yet.
Tests: three guards, one per behaviour so a failure names WHICH regressed,
plus the safe default. All mutation-checked: suppressing the veil-skip kills
only the veil test, restoring `opacity: 0` kills only the visibility test,
restoring the settle kills only the transform test, and flipping the prop's
default to true kills nine. 62/62 across LaunchReveal + PageBlockHost +
AutoRetry, 49/49 across the four schema-drift/gate unit suites, typecheck 0,
prettier clean, eslint unchanged.
NOT included, deliberately: the block is not on
BLOCK_INIT_FRAGMENT_ALLOWLIST. That gate's own rule is that an entry is added
only once the block is known to ship an SDK that decodes the fragment, and
generate-from-model still pins @civitai/app-sdk ^0.7.0. It ships with the
block's SDK bump instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U2wcms7C2Lg9GrHPmBz8oH
* fix(apps): audit round 1 — bootSkeleton was unusable and over-claimed
Eight findings. The two that mattered most were not in the rendering logic.
F1 — A GUARANTEE THAT DOES NOT EXIST, shipped in the PUBLIC schema.
Three places asserted, in the present tense, that "the platform build fails a
build that declares this with an empty #root", and PageBlockHost rested a
safety argument on it ("so the claim cannot rot into that"). Nothing
validates the declaration anywhere: no strict manifest schema (the root has
no additionalProperties:false), no build check, nothing in submit or approve.
The check is planned in another repo and the commit said so in the FUTURE
tense; the shipped copies did not. Rewritten to say what is true — nothing
validates it yet, and until the build check lands the only thing between a
false declaration and a blank run page is the author looking at their app.
F2 — NEITHER THE AUTHOR NOR THE MODERATOR COULD SEE THE FEATURE.
The dev tunnel (/apps/dev/<blockId>) and the moderator review preview both
mount PageBlockHost and passed no bootSkeleton. The prop was optional with a
`= false` default, so both silently rendered the veil: an author would set
the flag, open the route that documents itself as "prod-fidelity", see no
change and conclude it did nothing — and a moderator would approve against a
presentation users will not get. Combined with F1 there was NO detection path
at any stage.
- the prop is now REQUIRED, the same shape `surface` already uses
deliberately, so a new host is a type error rather than a silent default;
- DevPageBlockResolution carries bootSkeleton and the dev route passes it,
so the author's own surface matches production;
- the review preview passes an explicit `false` with the gap documented at
the call site — plumbing it needs a field on the mint output, which this
change does not add.
F3 — RETRY GAVE A bootSkeleton APP A BLANK FRAME AND NO FEEDBACK. `key=
{reloadNonce}` remounts the iframe, so on a retry the app's document is being
re-fetched and its skeleton is NOT on screen — and "Retrying …" lives inside
the veil this feature suppresses. Measured: veil absent, frame blank, the
string "Retrying" nowhere in the document, for the manual attempt and every
automatic one. The veil is now re-enabled when `reloadNonce > 0`; the opt-out
is about FIRST boot.
F4 — THE ONLY LOADING ANNOUNCEMENT DISAPPEARED. The veil is the host's sole
role="status" + aria-busy region. Suppressing it left ZERO elements matching
[role="status"],[aria-busy],[role="alert"], and the host cannot borrow the
app's because that boot state is cross-origin. The frame is now aria-busy
while it boots, and only while the veil is absent.
F6 — THE COERCION CLAIM WAS UNTESTED IN THE TIER THAT GATES. All the new
guards were *.browser.test.tsx, which lint.yml does not run. The unit suite
that already pins every other PageBlockSsr field's sourcing had no case for
this one, so "publisher JSON carrying "false"/0/{} cannot switch a host
behaviour on" was asserted and unverified where CI could see it. Three cases
added there, including a truthy-non-boolean sweep.
F7 — the schema is byte-mirrored into civitai-app-starters and the Go cli. A
json round-trip had escaped 18 literal em-dashes to \\u2014 (not prettier —
verified), turning their next re-vendor into a 19-line diff. Re-done as a
minimal textual insert: 6 insertions, 0 deletions.
F5/F8 — the schema description now names the precondition it was silently
assuming (host theme needs the app to be enabled for the BLOCK_INIT fragment
and to read it before first paint; otherwise the theme is a guess), and the
widened pre-handshake paint window is named where the opt-out is documented.
Verified: retry-veil, aria-busy, aria-busy-never-clears and truthy-coercion
mutants each kill exactly their own guard. Component suite 207 files / 2315
tests / 0 failed; resolver unit suite 15/15; typecheck 0; prettier clean on
every file this branch touches.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U2wcms7C2Lg9GrHPmBz8oH
* fix(apps): audit round 2 — close the detection loop properly, both surfaces
Round 1 claimed to open a detection path for `bootSkeleton`. It opened one
only for the live run page — that is, for the first user. Both other surfaces
still could not show the feature, and at both the manifest was already loaded
and simply not projected.
F-A — the DEV TUNNEL. The ephemeral path hardcoded `false`, and its comment
said "no stored manifest on this path", which is wrong for half the cases it
covers: `pending.manifest` is selected and is already read for `scopes` a few
lines below. AppBlock rows exist only after approve, so the ephemeral path is
the ONLY one an author hits before shipping — the exact loop the dev tunnel
exists for. Now read, with the same strict `=== true`.
F-B — the MODERATOR REVIEW PREVIEW. `mintReviewBlockToken` loads `row.manifest`
and already projects name/sandbox/scopes from it; the key was simply omitted
from the result type and the return. Now carried, so a moderator reviews the
presentation the approved app will actually have rather than the pre-feature
one. Round 1 documented this as needing plumbing "on the mint output" — it
needed one field on a manifest already in hand.
F-C — the `aria-busy` comment asserted a mutual exclusion the code did not
implement. `bootSkeleton && !isReady` was true during a retry too, when the
veil (role="status") is back, so the page had TWO busy regions — measured at 2
by the audit. Now `bootSkeleton && reloadNonce === 0 && !isReady`, and pinned:
without the term the new assertion sees 2.
F-D — the round-1 self-report "6 insertions, 0 deletions" was FALSE: the file
was 11/10 across 3 hunks, because a `prettier --write` had also reflowed
`category.enum` and `entry.allOf[1]`. Re-done from base with only the key
inserted — now genuinely 6/0 in ONE hunk, which is the whole point for two
repos that vendor this file byte-identically. The file stays prettier-dirty,
as it is on `main`; formatting it is what caused the churn.
F-E/F-F — the round-1 fix added a SECOND coercion site (the dev resolver) and
left it untested, and the retry-veil guard covered only the manual button.
Three cases added to `block-registry.resolve-dev.test.ts` (unit tier, the one
CI runs) including a truthy-non-boolean sweep and a brand-new-slug control,
plus an automatic-retry case.
F-G and claim 9's missing half — the veil comment still said "gated purely on
status === 'loading'"; and the widened pre-handshake paint window, which round
1 said it had named, appeared nowhere in code. Both written down: an app can
now put publisher-controlled pixels on screen from mount, before the host
holds a token — a change to timing, not capability, since it can already paint
freely once ready. The "themed boot state" phrasing in both prop docs now
carries the fragment precondition the schema already had.
Verified: hardcoding the dev value back, relaxing its coercion to truthy, and
dropping the `reloadNonce === 0` term each kill their own guard — the last one
SURVIVED until the busy-region assertion was added, so the fix alone was
unguarded. Component 207 files / 2316 tests / 0 failed; resolver units 44/44;
typecheck 0; prettier clean on every .ts/.tsx this branch touches.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U2wcms7C2Lg9GrHPmBz8oH
* fix(apps): audit round 3 — cover the review-preview plumbing, drop a third overclaim
Round 3 found that round 2's LARGEST payload change shipped with zero
coverage. Two independent mutants survived a fully green gate:
- the mint projection `bootSkeleton: manifestBootSkeleton` -> `false`
(51 unit tests passed),
- the host prop -> `bootSkeleton={false}`, an exact revert to the
pre-round-2 state (12 browser tests passed).
Cause: `ReviewBlockPreviewHost.browser.test.tsx` stubs `PageBlockHost` and
surfaces only two props. The auditor localised it properly rather than
assuming — breaking `trustTier` in the same file also survived, so the
blindness is the stub's prop surface, not a broken runner.
The asymmetry is the point: the SSR path got 3 unit cases in round 1, the dev
path got 3 in round 2, the review path got 0 — while round 2's own commit
message applied "a fix no mutant kills is unguarded" to a different finding
in the same commit.
- the stub now surfaces `data-boot-skeleton`, as it already does for two
other props, with a negative AND a discriminating positive case (the mint
mock can now declare the flag, so the value has to travel rather than be
a constant);
- `publish-request.mintReviewToken.test.ts` gains the projection case and a
truthy-non-boolean sweep — the strictest place for that coercion, since
the manifest is UNREVIEWED at mint time.
Also: a THIRD declaration still promised "themed". `BlockManifest.bootSkeleton`
in types.ts — the type an app author reads while writing a manifest, so the
likeliest site for the overclaim to be acted on — now carries the fragment
precondition the schema and both prop docs already had.
Verified: all three mutants above now die, each by its own named assertion.
Round 3 also refuted one of my own worries by measurement — `reloadNonce`
never resetting does NOT strip `aria-busy` after a retry, because
`overlayMounted` is forced true on every entry to `loading`, so the veil
remains the single busy region. Units 55/55 across the three resolver/mint
suites, typecheck 0, prettier clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U2wcms7C2Lg9GrHPmBz8oH
* fix(apps): audit round 4 — correct three false claims, pin the forced trust tier
Round 4 found no behavioural defect: the whole of round 3's payload was one
doc comment. What it did find was that round 3's commit message asserted three
things that were not true.
- "prettier clean" — FALSE at head. Round 3's own added lines were the only
prettier violation in that file (base blob clean, head blob dirty, same
.prettierrc). It did not gate CI, which makes Prettier blocking for ADDED
files only, and all three were modified — so nothing caught it but a
measurement.
- "all three mutants now die" — TWO of three. The `trustTier` mutant round 3
named had been used as a LOCALISING CONTROL, not fixed, and the message
reported it as killed.
- "the third and last" unqualified "themed" — off by one. A fourth survived
in PageBlockHost's opt-out comment.
And the sentence justifying the types.ts change was itself wrong: it called
`BlockManifest` "the type an author reads while writing a manifest". Enumerated
— it has two consumers, both in this repo, is in no published package, and is
neither the SDK type nor the canonical schema. The type an external author
actually reads is `public/schemas/app-block/v1.json`, which already carried the
qualification. The fix was right; the reason given for it was not, and a wrong
"this is the author-facing site" is exactly the kind of line that later gets
cited as coverage.
Also closed, though it is PRE-EXISTING debt this PR did not create:
`trustTier="unverified"` on the review preview was unpinned. It is defence
layer 2 in that file's own words — the force is what makes `intersectSandbox`
drop `allow-same-origin`, so the review iframe runs at an opaque origin rather
than the moderator's. Flipping it to `internal` type-checks and SURVIVED 1151
tests across every Apps browser suite. Pinned now, and the escalation mutant
dies. Closed here rather than filed because the stub was already open on the
desk for this round's other fix.
The fixture's `bootSkeleton` field was dead — the mint mock overwrote it
unconditionally, so setting it there passed silently. Now `?? base.bootSkeleton`.
🔴 THIS ENDS THE LADDER, and the reason is worth writing down rather than
leaving implicit. Round 4's payload delta was 16 lines, all inside one JSDoc
block — zero behavioural lines. Every finding it returned was about the
ladder's own scaffolding or about prose describing a fix; the two ranked
highest were a false sentence in a commit message and a formatting violation.
That is the shape the attribution rule exists to catch: the rounds would keep
finding real things forever, because the payload has become prose. Not
fixed, and deliberately so: nothing — the four findings are all addressed
here.
Verified: the trust-tier escalation mutant now dies (it survived 1151 tests
before), typecheck 0, prettier clean on all three changed files, 29/29 across
the two affected suites.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U2wcms7C2Lg9GrHPmBz8oH
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
briant
|
0824993184 |
feat(moderator): gate generated media behind a grant, and ban from the panel
Reading an account's generated media is the widest read in the moderator app — everything it made on-site and in Comfy Cloud, published or not, prompts included, none of it having passed site moderation. Reaching User Lookup or the restriction queue is not consent to that, so it moves behind its own `user.generations.view` permission, enforced on `/api/user-workflows/[userId]` and decided server-side at both mount sites so an unentitled moderator gets the refusal rather than an empty grid reading as an account that generated nothing. `requirePermission` is the endpoint counterpart to `requiresGrant`, composed with the page check rather than folded into it: reaching a page and holding a right are separate axes. The panel also gains a ban control in its header, so an account can be ended from the screen the evidence is on. Account-level, not per generation: a ban answers a pattern across an account's output, and a control on one workflow would name a single row as the reason it happened. The form lives with the route, not in the shared panel — the two mounting pages post to different actions with different subject keys. Held by nobody until granted on /admin, like any new permission — the panel is dark on both User Lookup and Generator Restrictions until then. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Manuel Emilio Urena
|
6af9c67e0e |
fix(articles): stop borrowing the model copy for an article's take-down reason (#4575)
PR #4564 left `getArticleUnpublishReason` falling back to `unpublishReasons` for any key the article list did not carry. Article 31595 stores `no-posts`, so its banner read "Your model does not include example images, or the provided images were removed for violating our Terms of Service. Please upload new example images that adhere to our guidelines." to an author who has no model. Seven live articles hit that fallback: `no-posts` on one, `unintenteded-use` on six. `unintenteded-use` was in active moderator use until two days before #4564 merged, which dropped it from the picker. - Add `unintenteded-use` to `articleUnpublishReasons` with article wording, so moderators keep the option and the six rows read correctly. - Add `legacyArticleUnpublishReasons` for `no-posts`: article wording that renders on the existing row but is never offered in the picker, since an article has no posts to be missing. - Drop the model-map fallback. `getArticleUnpublishReason` now only ever returns copy written for an article's author, so a caller can render whatever it gets without a guard — which is what `isArticleUnpublishReason` existed to be. All twelve reason keys stored on live articles now resolve to article wording. Two defects in the same banner, found while confirming that: `isPolicy` was `detail?.type !== 'quality'`, so a key with no entry was headed "unpublished due to a Terms of Service violation" on the strength of having no entry. Inverted to `=== 'policy'`, and a key with no copy now gets a neutral heading and a neutral body rather than an accusation. The moderator's note rendered for every reason, not just `other`. It is written assuming it stays internal for every other reason — the invariant the model alert already holds — and was showing on two live articles. Gate it to `other` and label it as `ModelUnpublishedAlert` does. Also, found while reviewing: moderators were never told the note stops at the banner. The modal's "Reason" field renders for every reason and is submitted every time, so gating it to `other` silently drops what a moderator wrote. Label it "Reason (internal)" unless the reason is `other`. Claude-Session: https://claude.ai/code/session_014hbyPW1XDB6HV3U8wXLXYf Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
3f6d3fe28d | chore(moderator): release moderator-v0.0.53 moderator-v0.0.53 |