mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
feat(apps): retire the legacy /apps/[appBlockId] detail page — redirect to the store detail (#3493)
* feat(apps): retire the legacy /apps/[appBlockId] detail page (redirect to the store detail)
The per-app route `/apps/<appBlockId>` was a second, diverging detail surface for
the same app. It rendered `by <app name>` where the store correctly renders the
owner's username, and its raw bridge-less `<iframe>` "Live preview" painted a
permanent light-theme panel on a dark page. Its whole action set is already
covered by `/apps/store-preview/<slug>`.
`getServerSideProps` now redirects it there:
- approved listing found -> 302 to `/apps/store-preview/<slug>`
- no approved listing (pending / rejected / never-approved app) -> site-standard
404, NOT a bounce to `/apps`. Listings are minted at approval, so those apps
have no store target; sending their owner to a store that by construction does
not list them is a silent dead end.
The store-visibility flag gate stays FIRST, before the route param is read and
before any DB access, so the retirement is not an existence oracle for a viewer
the flag does not grant.
The branch is extracted as a pure, I/O-free `resolveLegacyAppRedirect` module so
both decided outcomes are asserted in the node-env unit project (13 tests),
including slug encoding / open-redirect containment.
Redirect only: the page body and the three inbound callsites are deliberately
untouched so a stale bookmark or external link resolves through the hop. Sibling
routes (`/edit`, `/edit-manifest`, `/listing`, `/revenue`) are unaffected.
* test(apps): cover the SSR half of the retirement + retarget the e2e spec
Follow-up to the adversarial review of this PR, which found that the
security-relevant half of the change was the untested half.
- Extract the whole SSR decision into `resolveLegacyAppRoute`, taking the
listing lookup as an argument. The gate-before-query ordering is the real
invariant here and was invisible to a test of the string-building function
alone; with the lookup injected, a test asserts it is never even CALLED for a
viewer without store visibility. Verified by mutation: moving the gate after
the lookup fails 2 tests.
- Pin the approved-only filter via `approvedListingSlugQuery`. Dropping
`status: 'approved'` was a one-word edit that inverted the decided behaviour
while every existing test still passed. Verified by mutation: 1 test fails.
- Cover route-param handling (missing / blank / non-string) and the
either-flag OR-fallback. 13 -> 21 tests.
- Retarget `tests/preview-apps-marketplace.spec.ts`: its last leg asserted a
heading on the retired page, which no longer renders — it would have tested
the store detail by accident and raced that page's client-side query. It now
asserts the retirement itself (lands under /apps/store-preview/), and names a
404 as a real data signal rather than flake.
- Correct the callsite count in the docs: there are FOUR inbound links, not
three (the editor's own "Back" was missed). Record the two accepted
consequences of leaving them alone, including that the store detail's
info-mode CTA for a model-slot app now hops back to itself (no live app is
in that state — every approved on-site listing declares a page).
- Narrow the existence-oracle claim in the module docstring: gate-first
protects viewers WITHOUT store visibility; for those with it, 302-vs-404 is
a new HTTP-level signal the old page did not emit.
* docs(apps): name the Install gap, drop assertions that cannot fail
Second independent adversarial pass over this PR. Its conditional blocker
resolves to a NIT against live data (0 approved on-site listings are page-less
— every one declares a page, so none takes the `info` branch), but it surfaced
a claim of mine that was simply wrong and several assertions with no failure
mode.
- Correct the "the store detail covers the whole action set" claim. It does
not: `AppListingDetailBody` has NO install/manage affordance, so a
model-slot app would have nowhere to install from. Vacuous today, but it is
the real gap to close before a model-slot app is approved — and the reason
the follow-up must RETARGET the info-mode CTA, not just delete the route.
- Note the sibling `legacyEditRedirect` in listingEditNav.ts, which does the
same job for the owner-edit routes, and why the stricter non-string handling
here is deliberate rather than an oversight.
- Note on /apps that the documented one-line rollback to the AppBlock grid is
now partial: AppBlockCard's links point at the retired route.
- Replace `destination.startsWith(STORE_PREVIEW_PATH_PREFIX)` — a tautology,
since the destination is built from that same constant — with the property
that can actually fail: the slug never introduces a second path segment.
That is the real open-redirect containment assertion.
- Drop a vestigial `detailName.length > 0` from the e2e leg and assert instead
that the viewer did not remain on the legacy route.
- Record all five verified break-it mutations in the test file header.
* style(apps): drop an unrelated reformat from the retired page
A prettier --write run collapsed a JSX block that this PR does not otherwise
touch. Restore main's formatting so the diff stays confined to the change.
* docs+test(apps): name the THIRD unreplicated gate, correct the orphan claim, kill four unfailable assertions
Audit follow-up on the legacy-route retirement. Production logic unchanged —
this is a disclosure correction plus four test defects.
Disclosure:
- The gate-parity note named TWO unreplicated destination gates; there are
THREE. `getListingDetail` also applies a store-scope KIND gate
(`scope === 'public-external' && row.kind !== 'offsite'`). Because this
route's param is an `AppBlock.id` and every listing carrying an
`appBlockId` is `kind='onsite'` (20/20 in live data), that gate covers
100% of this route's resolvable set under `public-external` — where the
deploy and maturity gates are 0-instance. Unreachable today only because
the page gate keys on the same Flipt flag as the scope's first axis; a
widen of both breaks the alignment. The remediation is restated as all
three and is explicitly NOT implemented here — it needs the resolved store
scope and red-capability threaded into an SSR resolver that takes neither.
- "Nothing is orphaned" was FALSE. The legacy 5-star AppBlockReview write
form's only other host is AppDetailsModal ← AppBlockCard ← MarketplaceBody
/ RecentlyOpenedApps, and MarketplaceBody has had no importer in app code
since /apps swapped to AppListingsMarketplaceBody. After this redirect the
surface has no reachable entry point. Harmless today (`app_block_reviews`
is 0 rows in production) but the follow-up must decide its fate explicitly.
- "Four callsites" is four FILES / five link sites — AppBlockCard links twice.
Also named: the hop drops the query string (harmless in-product; external
tracking params are lost).
Tests (21 -> 24), each verified by a killing mutation:
- Passing the UNTRIMMED appBlockId to the lookup while still guarding the
trimmed value left the suite fully green — every fixture id was
whitespace-free. A ' apb_x ' granted-viewer case now catches it.
- Two `expect(...includes('/')).toBe(false)` sub-assertions each followed a
literal-equality assertion over the same string in the same `it`, so they
could never be the assertion that failed. Removed; the containment property
now has its own `it` over separator-bearing inputs no literal constrains.
- The traversal claim was overstated: encodeURIComponent('..') === '..', so a
dot-only slug resolves to /apps/, not under the prefix. Claim narrowed and
the dot-only case pinned (unreachable given SLUG_REGEX).
- The retargeted e2e leg asserted `landedOn !== '/apps/<id>'` after asserting
it matched /^\/apps\/store-preview\/.+/ — unfailable. Replaced by a
one-path-segment assertion and a redirect-chain assertion that pins the
retirement to the HTTP layer.
This commit is contained in:
Zachary Lowden
@@ -5,7 +5,7 @@ import { trpcQuery } from './preview-trpc';
|
||||
/**
|
||||
* Preview-e2e (F-C): App Blocks MARKETPLACE discovery + per-app detail — the
|
||||
* anon-capable PUBLIC read path (`blocks.listAvailable` → `blocks.getAppDetail`)
|
||||
* plus the two host pages that render them (`/apps`, `/apps/<appBlockId>`).
|
||||
* plus `/apps`, and the retirement of the legacy `/apps/<appBlockId>` route.
|
||||
* Otherwise untested by the preview suite; a PR that broke the marketplace
|
||||
* listing/detail projection or the `features.appBlocks` page gate passes every
|
||||
* other preview spec today.
|
||||
@@ -43,10 +43,11 @@ import { trpcQuery } from './preview-trpc';
|
||||
* / non-approved id (the router then throws NOT_FOUND — our helper would
|
||||
* surface that as a thrown error). We assert the shape of a discovered,
|
||||
* known-approved id, so a non-null detail is expected.
|
||||
* - Detail page (`/apps/[appBlockId]/index.tsx`) renders the block name as
|
||||
* `<Title order={2}>{name}</Title>` where
|
||||
* `name = detail.manifest.name ?? detail.blockId ?? appBlockId` — so the
|
||||
* visible host-rendered name == `manifest.name || blockId`.
|
||||
* - Legacy detail page (`/apps/[appBlockId]/index.tsx`) is RETIRED: its
|
||||
* `getServerSideProps` now resolves the app's approved `AppListing` and
|
||||
* redirects to `/apps/store-preview/<slug>` (or `notFound` when the app has no
|
||||
* approved listing). It no longer renders, so this spec asserts the REDIRECT
|
||||
* rather than a heading on it.
|
||||
* - Marketplace page (`/apps/index.tsx`) renders the `AppsSubNav` tabs bar — its
|
||||
* first tab is `{ href: '/apps', label: 'Marketplace' }` (AppsSubNav.tsx:55) —
|
||||
* plus a search `TextInput` (placeholder "Search by name or block id"). It no
|
||||
@@ -161,16 +162,73 @@ test.describe('App Blocks marketplace discovery + detail render (mod)', () => {
|
||||
'detail.manifest.targets should be an array (slot badges)'
|
||||
).toBe(true);
|
||||
|
||||
// The DETAIL PAGE renders that block's name (host-rendered <Title> text). This
|
||||
// proves the page's getAppDetail query + SSR appBlocks gate work end-to-end,
|
||||
// not just the bare tRPC call. domcontentloaded ONLY.
|
||||
// The legacy per-app route `/apps/<appBlockId>` is RETIRED: it now redirects to
|
||||
// the unified store detail. This leg used to assert that route rendered the
|
||||
// block's name; that page no longer renders at all, so asserting a heading here
|
||||
// would either test the store detail by accident or race its client-side query.
|
||||
// Assert the RETIREMENT instead — the invariant this route now has.
|
||||
//
|
||||
// Deliberately asserts the destination PATH PREFIX, not `/apps/store-preview/
|
||||
// <blockId>`: the redirect resolves the store slug from the listing row, and
|
||||
// pinning blockId here would re-import the very assumption the redirect avoids.
|
||||
//
|
||||
// 🔴 A 404 here is a real signal, not flake: it means this approved app has no
|
||||
// approved `AppListing` row. Auto-create-on-approve is best-effort (it logs and
|
||||
// continues on failure) and apps approved before it shipped were backfilled by
|
||||
// hand, so the gap is recoverable but does not self-heal. Fix the data (run the
|
||||
// mod-only listing backfill), don't relax this assertion. domcontentloaded ONLY.
|
||||
const detailResp = await page.goto(`/apps/${encodeURIComponent(first.id)}`, {
|
||||
waitUntil: 'domcontentloaded',
|
||||
});
|
||||
expect(detailResp?.status(), `GET /apps/${first.id} status`).toBeLessThan(400);
|
||||
await expect(
|
||||
page.getByRole('heading', { name: detailName }),
|
||||
`the detail page should render the block name "${detailName}"`
|
||||
).toBeVisible();
|
||||
expect(
|
||||
detailResp?.status(),
|
||||
`GET /apps/${first.id} should follow the retirement redirect to a served page (a 404 means this approved app has no approved AppListing row)`
|
||||
).toBeLessThan(400);
|
||||
|
||||
const landedOn = new URL(page.url()).pathname;
|
||||
expect(
|
||||
landedOn,
|
||||
`the retired /apps/${first.id} should land on the unified store detail, not render itself`
|
||||
).toMatch(/^\/apps\/store-preview\/.+/);
|
||||
|
||||
// 🔴 The two assertions below replace an earlier `.not.toBe('/apps/<id>')`, which
|
||||
// COULD NOT FAIL: given the `toMatch` above has passed, `landedOn` already starts
|
||||
// `/apps/store-preview/`, and `/apps/<appBlockId>` never can — so the inequality
|
||||
// held by construction. It wore the name of a guard while guarding nothing. Both
|
||||
// replacements pin a property the `toMatch` genuinely leaves open.
|
||||
|
||||
// (a) The slug occupies EXACTLY ONE path segment. `/^\/apps\/store-preview\/.+/`
|
||||
// happily accepts `/apps/store-preview/a/b` — i.e. a destination that climbed
|
||||
// out of the detail route into some other page. This is the browser-level
|
||||
// twin of the containment property the unit test pins on the built string.
|
||||
const slugSegment = landedOn.slice('/apps/store-preview/'.length);
|
||||
expect(
|
||||
slugSegment.split('/').filter(Boolean),
|
||||
`the store slug must be one path segment, got "${slugSegment}"`
|
||||
).toHaveLength(1);
|
||||
|
||||
// (b) The retirement happened at the HTTP layer — a real server redirect out of
|
||||
// the legacy route — not a client-side bounce rendered by the page. Walk the
|
||||
// redirect chain of the response we landed on and require the legacy path in
|
||||
// it. This fails if anyone reimplements the retirement as a `router.replace`
|
||||
// in the component (which would reintroduce exactly the render-then-race this
|
||||
// leg was rewritten to avoid), and it fails if a future `/apps/*` middleware
|
||||
// or rewrite starts serving the store detail directly without the 302.
|
||||
const redirectChain: string[] = [];
|
||||
for (
|
||||
let req = detailResp?.request().redirectedFrom();
|
||||
req && redirectChain.length < 10;
|
||||
req = req.redirectedFrom()
|
||||
) {
|
||||
redirectChain.push(new URL(req.url()).pathname);
|
||||
}
|
||||
expect(
|
||||
redirectChain.map((p) => decodeURIComponent(p)),
|
||||
`GET /apps/${
|
||||
first.id
|
||||
} must reach the store detail via a SERVER redirect out of the legacy route (chain: ${
|
||||
redirectChain.join(' -> ') || '<none>'
|
||||
})`
|
||||
).toContain(`/apps/${first.id}`);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user