Skip to content

Commit c8b7f3d

Browse files
os-zhuangclaude
andauthored
test(app-shell): ratchet the component-props-to-any cast that hid #3025's dead prop, and pin the schema.direction boundary (#3036)
Two follow-ups to #3025, both about stopping that bug from recurring. **The ratchet.** #3025 removed two `const PanelGroup = ResizablePanelGroup as React.FC<any>` casts. Those casts were not incidental to the bug — they caused it. Erasing a component's props to `any` meant that when react-resizable-panels v4 renamed `direction` to `orientation`, the stale prop type-checked against nothing and survived a whole major version as a dead prop React forwarded to the DOM. Deleting the casts is what turned it back into a hard TS2322. What makes this worth a ratchet rather than review vigilance is how both casts were justified: one blamed vite-plugin-dts for "not resolving the direction prop type correctly", the other a prop that "does not always narrow cleanly in our TS config". Neither was true — v4's `GroupProps` simply has no `direction` key. The cast reads as a workaround for a toolchain quirk while actually disabling the check that catches a breaking upstream rename, so it is the kind of thing that gets re-added in good faith. Repo-wide over `packages/<pkg>/src` and `apps/<app>/src`, following the existing `*.ratchet.test.ts` shape (#2269, ADR-0054) so it runs in the gating `pnpm test` job. Covers `FC` / `FunctionComponent` / `ComponentType` parameterised with `any`, qualified or not. Current count is zero, which is why now is the moment to set the gate. Three tests, because a ratchet has two silent failure modes of its own: a scan path that finds nothing, and a regex that stops matching. Both are pinned — the second against the exact casts #3025 removed, plus the legitimate neighbours it must not fire on (`as Record<string, any>`, `as ComponentProps<typeof Button>`, `const C: React.FC<Props>`). **The boundary comment.** `renderers/complex/resizable.tsx` maps `schema.direction` onto the library's `orientation`. That looks exactly like a site #3025 missed. It is not: `direction` is ObjectUI's own public authoring vocabulary and appears in stored view metadata, so renaming it to match the library would break every saved `resizable` schema. Said so in place, since the next person to grep for `direction` will land there. Verified the ratchet actually bites: reintroducing the exact cast into `custom/navigation-overlay.tsx` fails it with the file and line, and the four sibling ratchets plus the 15 resizable tests stay green. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent e25e300 commit c8b7f3d

2 files changed

Lines changed: 140 additions & 1 deletion

File tree

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* objectui#3025 ratchet (AGENTS.md Commandment #6 — "No `any`"). Same shape as
6+
* the ADR-0054 and #2269 ratchets: runs in the gating `pnpm test` job and fails
7+
* if the anti-pattern reappears.
8+
*
9+
* The bug this guards. Two call sites carried
10+
* `const PanelGroup = ResizablePanelGroup as React.FC<any>`. The cast erased
11+
* the component's props to `any`, so when react-resizable-panels v4 renamed
12+
* `PanelGroup`'s `direction` prop to `orientation`, the stale `direction` kept
13+
* type-checking against nothing — for a whole major version. It silently became
14+
* a dead prop that React forwarded to the DOM as a stray attribute. Removing
15+
* the casts is what turned it back into a hard `TS2322`.
16+
*
17+
* Both comments blamed the toolchain (vite-plugin-dts "not resolving the prop
18+
* type correctly"; a prop that "does not always narrow cleanly in our TS
19+
* config"). Neither was true. That is the real hazard here: a cast like this
20+
* reads as a workaround for a tooling quirk while actually disabling the check
21+
* that would have caught a breaking upstream rename.
22+
*
23+
* If this fails: do not widen a component's props to `any` to make an error go
24+
* away. Read the dependency's current prop types — the prop was probably
25+
* renamed or removed. If a genuinely-invalid prop must be passed, type the
26+
* escape hatch narrowly (`as ComponentProps<typeof X> & { extra?: T }`), never
27+
* with a blanket `any`.
28+
*
29+
* SCOPE — repo-wide over `packages/<pkg>/src` and `apps/<app>/src`, production
30+
* sources only. Test files are excluded, both to match the existing ratchets
31+
* and because this file necessarily contains the pattern in its own regex.
32+
*/
33+
34+
import { describe, it, expect } from 'vitest';
35+
import { readdirSync, readFileSync, statSync } from 'node:fs';
36+
import path from 'node:path';
37+
import { fileURLToPath } from 'node:url';
38+
39+
const here = path.dirname(fileURLToPath(import.meta.url));
40+
// packages/app-shell/src -> repo root
41+
const repoRoot = path.resolve(here, '../../..');
42+
43+
/**
44+
* Banned: casting a value to a component type parameterised with `any`.
45+
*
46+
* Covers the three spellings that erase props identically — `FC`,
47+
* `FunctionComponent`, `ComponentType` — with or without the `React.`
48+
* qualifier, and tolerant of whitespace inside the type arguments.
49+
*/
50+
const COMPONENT_ANY_CAST =
51+
/\bas\s+(?:React\.)?(?:FC|FunctionComponent|ComponentType)\s*<\s*any\s*>/;
52+
53+
function collectSourceFiles(): string[] {
54+
const out: string[] = [];
55+
const walk = (dir: string) => {
56+
for (const entry of readdirSync(dir, { withFileTypes: true })) {
57+
const name = entry.name;
58+
if (name === 'node_modules' || name === 'dist' || name.startsWith('.wt-') || name === '.next') {
59+
continue;
60+
}
61+
const full = path.join(dir, name);
62+
if (entry.isDirectory()) {
63+
walk(full);
64+
} else if (/\.tsx?$/.test(name) && !/\.(test|spec)\.tsx?$/.test(name)) {
65+
out.push(full);
66+
}
67+
}
68+
};
69+
for (const group of ['packages', 'apps']) {
70+
const groupDir = path.join(repoRoot, group);
71+
let pkgs: string[];
72+
try {
73+
pkgs = readdirSync(groupDir);
74+
} catch {
75+
continue;
76+
}
77+
for (const pkg of pkgs) {
78+
const srcDir = path.join(groupDir, pkg, 'src');
79+
try {
80+
if (statSync(srcDir).isDirectory()) walk(srcDir);
81+
} catch {
82+
/* package has no src/ */
83+
}
84+
}
85+
}
86+
return out;
87+
}
88+
89+
describe('objectui#3025 — no component-props-to-any cast ratchet', () => {
90+
it('finds the repo sources (guards against a broken scan path)', () => {
91+
// The repo has thousands of source files; if this collapses, the scan
92+
// globs have gone stale and the ratchet would silently pass on nothing.
93+
expect(collectSourceFiles().length).toBeGreaterThan(500);
94+
});
95+
96+
it('detects the pattern it is meant to ban (guards against a dead regex)', () => {
97+
// A ratchet whose regex silently stops matching is worse than no ratchet,
98+
// so pin it against the exact casts #3025 removed, plus spelling variants.
99+
for (const sample of [
100+
'const PanelGroup = ResizablePanelGroup as React.FC<any>;',
101+
'const X = Y as FC<any>',
102+
'const X = Y as React.ComponentType< any >',
103+
'const X = Y as FunctionComponent<any>',
104+
]) {
105+
expect(COMPONENT_ANY_CAST.test(sample), sample).toBe(true);
106+
}
107+
// ...and must not fire on legitimate neighbours.
108+
for (const ok of [
109+
'const x = y as Record<string, any>',
110+
'const x = y as ComponentProps<typeof Button>',
111+
'const C: React.FC<Props> = (p) => null',
112+
]) {
113+
expect(COMPONENT_ANY_CAST.test(ok), ok).toBe(false);
114+
}
115+
});
116+
117+
it('has zero component-props-to-any casts in production sources', () => {
118+
const offenders: string[] = [];
119+
for (const file of collectSourceFiles()) {
120+
const src = readFileSync(file, 'utf8');
121+
for (const line of src.split('\n')) {
122+
if (COMPONENT_ANY_CAST.test(line)) {
123+
offenders.push(`${path.relative(repoRoot, file)} :: ${line.trim()}`);
124+
}
125+
}
126+
}
127+
// If this fails: you widened a component's props to `any`. That disables
128+
// the check that catches renamed/removed props across a dependency major —
129+
// exactly how #3025's dead `direction` prop survived. Read the current
130+
// prop types instead, or type the escape hatch narrowly. Do not allowlist.
131+
expect(offenders).toEqual([]);
132+
});
133+
});

packages/components/src/renderers/complex/resizable.tsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,13 @@ ComponentRegistry.register('resizable',
2020
({ schema, className, ...props }: { schema: ResizableSchema; className?: string; [key: string]: any }) => {
2121
const panels = Array.isArray(schema.panels) ? schema.panels : [];
2222
return (
23-
<ResizablePanelGroup
23+
<ResizablePanelGroup
24+
/* `schema.direction` → `orientation` is a deliberate boundary
25+
translation, not a leftover from the react-resizable-panels v3→v4
26+
rename (#3025). `direction` is ObjectUI's own public authoring
27+
vocabulary and appears in stored view metadata, so renaming it to
28+
match the library's prop would break every saved `resizable`
29+
schema. Keep the two names distinct and translate here. */
2430
orientation={(schema.direction || 'horizontal') as "horizontal" | "vertical"}
2531
className={className}
2632
{...props}

0 commit comments

Comments
 (0)