test: decouple the feature-switch gates from the template's demo screens
The template's demonstration screens exist to explain the template. A product replaces them with its domain, so deleting them is the expected end state — but two gates were coupled to them, and the merge accommodated that coupling instead of fixing it. product-features.test.ts derives its own scope now: a registry must gate on the manifest when it imports a module belonging to a manifest-declared feature. The previous fix put installed-feature-runtimes.tsx in the exempt list, which silenced the guard for that file permanently. Verified by removing the manifest reference from installed-feature-adapters.ts and watching the guard fail; a counter asserts the sweep still watches at least one file. product-feature-switch.test.tsx exercises the kill switch end to end again. The mechanism is the ownership lookup plus isFeatureActive, which has nothing to do with which screens ship, so the ownership map is the fixture: one real registered route attributed to a real installed feature, with the product's own router, components and codecs. That restoration exposed a real gap. The navigation-withdrawal half is not implemented here: it lives in the template's PrimaryNavigation and this product does not render the template's AppShell at all — the public header is a hand-written list of paths. A disabled feature's route is refused by the router but its link would still be advertised. Harmless only while no feature-owned route is navigable, which is now asserted so the gap cannot ship silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
cd1ef5cda2
commit
93ce86eef4
@@ -211,14 +211,67 @@ parents.
|
||||
To compare against a pre-merge state, use a separate checkout rather than
|
||||
stashing an in-progress merge.
|
||||
|
||||
## Correction after review
|
||||
|
||||
Two of the resolutions above were wrong, and were fixed in a follow-up commit.
|
||||
|
||||
The template's demonstration screens — `platform-overview-page`, the UI and
|
||||
state galleries, the auth example and the reference feature's screens — exist to
|
||||
explain the template. A product replaces them with its domain, and deleting them
|
||||
is the expected end state, not a regression. Two gates were nevertheless coupled
|
||||
to them, and the first resolution accommodated that coupling instead of fixing
|
||||
it.
|
||||
|
||||
### `tests/unit/product-features.test.ts` — a hard-coded exemption became a rule
|
||||
|
||||
The guard requires every installed registry to compose from the manifest.
|
||||
`installed-feature-runtimes.tsx` was added to its exempt list once the reference
|
||||
feature's presentation layer was gone. That silenced the guard for that file
|
||||
permanently.
|
||||
|
||||
It now derives its own scope: a registry must gate on the manifest **when it
|
||||
imports a module belonging to a manifest-declared feature**. A product whose
|
||||
registries compose only its own domain drops out of the rule honestly, and the
|
||||
guard fires again the moment a declared feature is imported without gating —
|
||||
verified by removing the manifest reference from
|
||||
`installed-feature-adapters.ts` and watching the guard fail. A counter asserts
|
||||
the sweep is still watching at least one file, so an empty scope cannot pass
|
||||
silently.
|
||||
|
||||
### `tests/component/product-feature-switch.test.tsx` — coverage restored
|
||||
|
||||
The end-to-end kill-switch assertions were replaced with composition checks
|
||||
because the screens they rendered were gone. The mechanism under test is the
|
||||
ownership lookup plus `isFeatureActive`, which has nothing to do with which
|
||||
screens ship, so **the ownership map is now the fixture**: one real registered
|
||||
route is attributed to a real installed feature, and the router, shell,
|
||||
components and codecs are all the product's own. The deep-link half is asserted
|
||||
end to end again.
|
||||
|
||||
### What that restoration exposed
|
||||
|
||||
The navigation-withdrawal half **is not implemented in this product**. It lives
|
||||
in the template's `PrimaryNavigation`, and this product does not render the
|
||||
template's `AppShell` at all — the public site header is a hand-written list of
|
||||
paths in `src/features/tech-log/presentation/public/components/site-header.tsx`,
|
||||
and the studio has its own shell.
|
||||
|
||||
So a disabled feature's route is refused by the router but its link would still
|
||||
be advertised. That is harmless only while no feature-owned route is navigable,
|
||||
which is true today and is now asserted. If that assertion fails, the header has
|
||||
to consult `ROUTE_FEATURE_OWNER` — or navigation has to move back onto
|
||||
`NAVIGATION_ROUTES` — before the route ships.
|
||||
|
||||
## Follow-ups this merge deliberately did not decide
|
||||
|
||||
1. **TechLog is outside the product manifest.** It is composed directly rather
|
||||
than as a `SelectableProductFeature`, so the runtime kill switch does not
|
||||
govern it. Promoting it is a product decision.
|
||||
2. **The runtime kill switch currently governs no route.** The only installed
|
||||
feature's screens were removed, so the switch still gates operations and
|
||||
schemas but has no user-visible effect. It starts asserting again as soon as
|
||||
a feature-owned route is registered.
|
||||
3. **The reference feature declares routes it cannot serve.** Either its screens
|
||||
return, or its route declarations should be withdrawn from its contract.
|
||||
govern it. That is defensible — a product's own domain is not an optional
|
||||
feature — but it means the switch governs nothing user-visible today.
|
||||
2. **The reference feature declares routes it cannot serve.** Its screens were
|
||||
deleted with the rest of the demonstration UI, but its contract still
|
||||
declares `REFERENCE_RESOURCE_*` routes. Either the declarations go, or the
|
||||
feature does. `TechLog` does not import it (`git grep reference-feature --
|
||||
src/features/tech-log` is empty), so removing it is a live option and
|
||||
FE-GATE-020 exists to prove it can be removed.
|
||||
3. **The public site header is not feature-aware.** See above.
|
||||
|
||||
@@ -1,47 +1,149 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
// @vitest-environment jsdom
|
||||
|
||||
import {
|
||||
ROUTE_FEATURE_OWNER,
|
||||
ROUTE_REGISTRY,
|
||||
} from "../../src/features/installed-feature-contracts.ts";
|
||||
import { ROUTE_RUNTIME } from "../../src/features/installed-feature-runtimes.tsx";
|
||||
import { INSTALLED_PRODUCT_FEATURE_IDS } from "../../src/features/installed-product-manifest.ts";
|
||||
import { render, screen } from "@testing-library/react";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
|
||||
import { createAnonymousSessionAdapter } from "../../src/adapters/auth/external-session-adapter.ts";
|
||||
import { createTechLogFeatureInstalledInput } from "../../src/features/tech-log/adapters/create-tech-log-feature-input.ts";
|
||||
import { createTestApplication } from "../helpers/create-test-application.ts";
|
||||
import { createProductFeaturesStub } from "../helpers/runtime-capabilities-stub.ts";
|
||||
|
||||
/**
|
||||
* §3.5. The runtime kill switch, as it applies to *this* product.
|
||||
* §3.5. The runtime kill switch, exercised through the running app.
|
||||
*
|
||||
* The template asserted the switch end to end by rendering the reference
|
||||
* feature's screens and taking them out of service. This product deleted those
|
||||
* screens during the UI migration, so that render is no longer possible: the
|
||||
* feature contributes API operations, schemas and mappers, but no route.
|
||||
* Withdrawing a feature from navigation is not the same as taking it out of
|
||||
* service: a typed deep link would still mount it. Both halves are asserted on
|
||||
* the same render, so the switch cannot be half-wired.
|
||||
*
|
||||
* Deleting the test with the screens would have removed the only thing watching
|
||||
* this seam, so what is asserted here is the invariant that survives the
|
||||
* migration and would have caught the trap the merge itself nearly introduced —
|
||||
* a route registered from a feature contract whose component no longer exists.
|
||||
* The moment a feature-owned route is registered again, the second test starts
|
||||
* asserting the switch end to end without being rewritten.
|
||||
* The template asserted this by rendering its own demonstration screens. Those
|
||||
* screens exist to explain the template, and a product replaces them with its
|
||||
* domain — so a test that needs them makes deleting them look like a
|
||||
* regression. It is not: the mechanism under test is the ownership lookup plus
|
||||
* `isFeatureActive`, and neither has anything to do with which screens ship.
|
||||
*
|
||||
* So the ownership map is the fixture. `ROUTE_FEATURE_OWNER` is empty in this
|
||||
* product because no installed feature contributes a route; one real registered
|
||||
* route is attributed to a real installed feature here, and everything else —
|
||||
* router, shell, components, codecs — is the product's own.
|
||||
*/
|
||||
|
||||
const OWNED_ROUTE_ID = "TECH_LOG_EXPLORE";
|
||||
const OWNER_FEATURE_ID = "reference-feature";
|
||||
const OWNED_ROUTE_PATH = "/explore";
|
||||
const OWNED_ROUTE_NAV_LABEL = "탐색";
|
||||
|
||||
vi.mock("../../src/features/installed-feature-contracts.ts", async (original) => {
|
||||
const actual = await original<
|
||||
typeof import("../../src/features/installed-feature-contracts.ts")
|
||||
>();
|
||||
return {
|
||||
...actual,
|
||||
ROUTE_FEATURE_OWNER: Object.freeze({
|
||||
[OWNED_ROUTE_ID]: OWNER_FEATURE_ID,
|
||||
}),
|
||||
};
|
||||
});
|
||||
|
||||
const { AppRouter } = await import("../../src/presentation/routes/app-router.tsx");
|
||||
const { ApplicationProvider } = await import(
|
||||
"../../src/presentation/providers/application-provider.tsx"
|
||||
);
|
||||
|
||||
function renderAt(path: string, disabled: boolean) {
|
||||
window.history.pushState({}, "", path);
|
||||
return render(
|
||||
<ApplicationProvider
|
||||
application={createTestApplication({
|
||||
session: createAnonymousSessionAdapter(),
|
||||
featureInputs: { "tech-log": createTechLogFeatureInstalledInput().input },
|
||||
...(disabled
|
||||
? {
|
||||
productFeatures: createProductFeaturesStub({
|
||||
[OWNER_FEATURE_ID]: "DISABLED_BY_CONFIG",
|
||||
}),
|
||||
}
|
||||
: {}),
|
||||
})}
|
||||
>
|
||||
<AppRouter />
|
||||
</ApplicationProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
function outOfServiceSurface() {
|
||||
return screen.queryByText(
|
||||
(_, element) =>
|
||||
element?.getAttribute("data-disabled-feature") === OWNER_FEATURE_ID,
|
||||
);
|
||||
}
|
||||
|
||||
describe("runtime product feature switch", () => {
|
||||
it("registers no route that cannot be mounted", () => {
|
||||
it("takes the owned route out of service for a direct deep link", async () => {
|
||||
renderAt(OWNED_ROUTE_PATH, true);
|
||||
const surface = await screen.findByText(
|
||||
(_, element) =>
|
||||
element?.getAttribute("data-disabled-feature") === OWNER_FEATURE_ID,
|
||||
{},
|
||||
{ timeout: 5000 },
|
||||
);
|
||||
expect(surface).toBeTruthy();
|
||||
});
|
||||
|
||||
it("serves the same deep link while the feature is active", async () => {
|
||||
renderAt(OWNED_ROUTE_PATH, false);
|
||||
expect(outOfServiceSurface()).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Composition invariants that hold whichever screens a product ships. The first
|
||||
* one is what caught a bad merge resolution: composing the route registry from
|
||||
* every installed feature's `contract.routes` registered the reference
|
||||
* feature's paths after this product had deleted their components.
|
||||
*/
|
||||
describe("route composition", () => {
|
||||
it("registers no route that cannot be mounted", async () => {
|
||||
const { ROUTE_REGISTRY } = await vi.importActual<
|
||||
typeof import("../../src/features/installed-feature-contracts.ts")
|
||||
>("../../src/features/installed-feature-contracts.ts");
|
||||
const { ROUTE_RUNTIME } = await import(
|
||||
"../../src/features/installed-feature-runtimes.tsx"
|
||||
);
|
||||
const unmountable = Object.keys(ROUTE_REGISTRY).filter(
|
||||
(routeId) => !(routeId in ROUTE_RUNTIME),
|
||||
);
|
||||
expect(unmountable).toEqual([]);
|
||||
});
|
||||
|
||||
it("keeps every feature-owned route inside the installed selection", () => {
|
||||
// A route owned by a feature the manifest did not install would be
|
||||
// reachable with nothing behind it.
|
||||
const orphaned = Object.entries(ROUTE_FEATURE_OWNER)
|
||||
.filter(([routeId]) => routeId in ROUTE_REGISTRY)
|
||||
.filter(([, featureId]) => !INSTALLED_PRODUCT_FEATURE_IDS.includes(featureId));
|
||||
expect(orphaned).toEqual([]);
|
||||
/**
|
||||
* The other half of the switch — withdrawing a disabled feature's entry from
|
||||
* navigation — lives in the template's `PrimaryNavigation`, which this
|
||||
* product does not render: TechLog's public header is a hand-written list of
|
||||
* paths, and the studio shell has its own. So the withdrawal half is not
|
||||
* implemented here.
|
||||
*
|
||||
* That is harmless only while no feature-owned route is navigable, which is
|
||||
* what this asserts. If it fails, the header has to consult
|
||||
* `ROUTE_FEATURE_OWNER` (or navigation has to move back onto
|
||||
* `NAVIGATION_ROUTES`) before that route ships — otherwise a disabled
|
||||
* feature would keep advertising a link to a page that refuses to load.
|
||||
*/
|
||||
it("has no navigable feature-owned route while the shell is not feature-aware", async () => {
|
||||
const actual = await vi.importActual<
|
||||
typeof import("../../src/features/installed-feature-contracts.ts")
|
||||
>("../../src/features/installed-feature-contracts.ts");
|
||||
const navigableOwned = actual.NAVIGATION_ROUTES.filter(
|
||||
(definition) => actual.ROUTE_FEATURE_OWNER[definition.routeId] !== undefined,
|
||||
).map((definition) => definition.routeId);
|
||||
expect(navigableOwned).toEqual([]);
|
||||
});
|
||||
|
||||
it("still declares a product manifest to narrow", () => {
|
||||
// The switch is only meaningful while something is selectable; if this ever
|
||||
// empties, the mechanism above is dead code rather than a guarantee.
|
||||
expect(INSTALLED_PRODUCT_FEATURE_IDS.length).toBeGreaterThan(0);
|
||||
it("attributes ownership only to routes the product registered", async () => {
|
||||
const actual = await vi.importActual<
|
||||
typeof import("../../src/features/installed-feature-contracts.ts")
|
||||
>("../../src/features/installed-feature-contracts.ts");
|
||||
for (const routeId of Object.keys(actual.ROUTE_FEATURE_OWNER)) {
|
||||
expect(Object.keys(actual.ROUTE_REGISTRY)).toContain(routeId);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -186,23 +186,37 @@ describe("every installed registry consults the manifest", () => {
|
||||
"installed-product-manifest.ts",
|
||||
// Capabilities have their own §3.5 selection file and override vocabulary.
|
||||
"installed-runtime-capabilities.ts",
|
||||
// Template merge. This product removed the reference feature's screens,
|
||||
// so no installed feature contributes a route component and this file has
|
||||
// nothing to gate. `tests/component/product-feature-switch.test.tsx`
|
||||
// holds the mountability invariant meanwhile; restore this entry to the
|
||||
// guarded set the day a feature contributes a route runtime again.
|
||||
"installed-feature-runtimes.tsx",
|
||||
// Message keys stay total on purpose; see the file for why.
|
||||
"installed-feature-messages.ts",
|
||||
]);
|
||||
// A registry has to gate on the manifest when it composes something a
|
||||
// manifest-declared feature owns. Deriving that from the file's own imports
|
||||
// is the difference between a rule and a list: a product that replaces the
|
||||
// template's demonstration feature with its own domain drops out of the
|
||||
// rule honestly, and the day it imports a declared feature again the guard
|
||||
// fires without anyone having to remember to re-add it.
|
||||
//
|
||||
// An earlier fix named `installed-feature-runtimes.tsx` in the exempt set
|
||||
// once this product deleted the reference feature's screens. That silenced
|
||||
// the guard for that file permanently — a worse outcome than the problem it
|
||||
// solved.
|
||||
let guarded = 0;
|
||||
for (const registry of registries) {
|
||||
if (exempt.has(registry)) continue;
|
||||
const source = await readFile(nodePath.join(root, registry), "utf8");
|
||||
const composesDeclaredFeature = COMPILED_PRODUCT_FEATURE_IDS.some(
|
||||
(featureId) => source.includes(`./${featureId}/`),
|
||||
);
|
||||
if (!composesDeclaredFeature) continue;
|
||||
guarded += 1;
|
||||
expect(
|
||||
/INSTALLED_PRODUCT_FEATURE(S|_IDS)/u.test(source),
|
||||
`${registry} must compose from the product manifest`,
|
||||
`${registry} composes a manifest-declared feature and must gate on the manifest`,
|
||||
).toBe(true);
|
||||
}
|
||||
// The rule has to still be watching something; an empty sweep would pass
|
||||
// no matter what happened to these files.
|
||||
expect(guarded).toBeGreaterThan(0);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user