Skip to content

Commit ca1d12d

Browse files
refactor: address comments.
1 parent 7a7357e commit ca1d12d

5 files changed

Lines changed: 131 additions & 14 deletions

File tree

docs/next-steps.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,3 +45,19 @@ Focused follow-up work for `@knighted/develop`.
4545
- Ensure assistant/editor integration remains compatible with this model (edits should target one of the fixed tabs) without expanding to dynamic tab metadata yet.
4646
- Suggested implementation prompt:
4747
- "Implement Phase 2 UX/UI tab support in @knighted/develop with a fixed first-pass tab model: Component, Styles, and App only (no arbitrary tab names yet). Add a clear tab UI for switching editor panes, preserve existing editor behavior/content wiring, and keep render/lint/typecheck/diagnostics flows working with the selected tab context where relevant. Keep AI/BYOT feature-flag behavior unchanged, maintain CDN-first runtime constraints, and do not add dependencies. Add targeted Playwright coverage for tab switching, default/active tab behavior, and interactions with existing render/style-mode flows. Validate with npm run lint and targeted Playwright tests."
48+
49+
6. **Document implicit App strict-flow behavior (auto render)**
50+
- Add a short behavior matrix in docs that explains when implicit App wrapping is allowed versus when users must define `App` explicitly.
51+
- Include concrete Component editor examples for each case so reviewer/user expectations are clear.
52+
- Suggested example cases to document:
53+
- Allowed implicit wrap (standalone top-level JSX, no imports/declarations), for example:
54+
- `(<button type="button">Standalone</button>) as any`
55+
- Requires explicit `App` (top-level JSX with declarations/imports), for example:
56+
- `const label = 'Hello'`
57+
- `const Button = () => <button>{label}</button>`
58+
- `(<Button />) as any`
59+
- Recommended explicit pattern, for example:
60+
- `const Button = () => <button>Hello</button>`
61+
- `const App = () => <Button />`
62+
- Suggested implementation prompt:
63+
- "Document the current implicit App behavior in @knighted/develop for auto-render mode using a compact behavior matrix and concrete component-editor snippets. Clearly distinguish supported implicit wrapping from cases that intentionally require an explicit App (such as top-level JSX mixed with imports/declarations). Keep docs concise, aligned with current runtime behavior, and include at least one positive and one explicit-error example."

playwright/rendering-modes.spec.ts

Lines changed: 73 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,28 @@ test('auto render implicit App includes multiple component declarations', async
288288
).toContainText(['bar', 'foo'])
289289
})
290290

291+
test('auto render does not treat lowercase helpers as implicit components', async ({
292+
page,
293+
}) => {
294+
await waitForInitialRender(page)
295+
296+
await ensurePanelToolsVisible(page, 'component')
297+
await page.getByLabel('ShadowRoot').uncheck()
298+
299+
await setComponentEditorSource(
300+
page,
301+
[
302+
'const helper = () => <button type="button">helper</button>',
303+
'function render() { return <div /> }',
304+
].join('\n'),
305+
)
306+
307+
await expect(page.getByRole('status', { name: 'App status' })).toHaveText('Error')
308+
await expect(page.locator('#preview-host pre')).toContainText(
309+
'Expected a function or const named App.',
310+
)
311+
})
312+
291313
test('auto render wraps standalone JSX with trailing semicolon and comment', async ({
292314
page,
293315
}) => {
@@ -307,6 +329,29 @@ test('auto render wraps standalone JSX with trailing semicolon and comment', asy
307329
).toContainText('implicit app from jsx expression')
308330
})
309331

332+
test('auto render requires explicit App for declarations plus top-level JSX expression', async ({
333+
page,
334+
}) => {
335+
await waitForInitialRender(page)
336+
337+
await ensurePanelToolsVisible(page, 'component')
338+
await page.getByLabel('ShadowRoot').uncheck()
339+
340+
await setComponentEditorSource(
341+
page,
342+
[
343+
"const label = 'kept declarations'",
344+
'const Button = () => <button type="button">{label}</button>',
345+
'(<Button />) as any',
346+
].join('\n'),
347+
)
348+
349+
await expect(page.getByRole('status', { name: 'App status' })).toHaveText('Error')
350+
await expect(page.locator('#preview-host pre')).toContainText(
351+
'Top-level JSX with declarations or imports requires an explicit App component.',
352+
)
353+
})
354+
310355
test('renders export default arrow component when auto render is disabled', async ({
311356
page,
312357
}) => {
@@ -351,10 +396,36 @@ test('renders export default class component in react mode', async ({ page }) =>
351396

352397
await page.getByRole('button', { name: 'Render' }).click()
353398

399+
await expect(page.getByRole('status', { name: 'App status' })).toHaveText('Rendered')
400+
const previewButtons = page
401+
.getByRole('region', { name: 'Preview output' })
402+
.getByRole('button')
403+
await expect(previewButtons.first()).toContainText('default export class')
404+
})
405+
406+
test('supports export default App without redeclaration', async ({ page }) => {
407+
await waitForInitialRender(page)
408+
409+
await ensurePanelToolsVisible(page, 'component')
410+
await page.getByLabel('ShadowRoot').uncheck()
411+
await page.getByLabel('Auto render').uncheck()
412+
413+
await setComponentEditorSource(
414+
page,
415+
[
416+
'function App() {',
417+
' return <button type="button">export default App</button>',
418+
'}',
419+
'export default App',
420+
].join('\n'),
421+
)
422+
423+
await page.getByRole('button', { name: 'Render' }).click()
424+
354425
await expect(page.getByRole('status', { name: 'App status' })).toHaveText('Rendered')
355426
await expect(
356-
page.getByRole('region', { name: 'Preview output' }).getByRole('button'),
357-
).toContainText('default export class')
427+
page.getByRole('region', { name: 'Preview output' }).getByRole('button').first(),
428+
).toContainText('export default App')
358429
})
359430

360431
test('persists layout and theme across reload', async ({ page }) => {

src/modules/github-pr-drawer.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ const stripTopLevelAppWrapper = async ({ source, getTopLevelDeclarations }) => {
334334
return source
335335
}
336336

337-
return mergeWhitespaceAroundRemoval(removeRanges({ source, ranges })).trim()
337+
return mergeWhitespaceAroundRemoval(removeRanges({ source, ranges }))
338338
} catch {
339339
return source
340340
}

src/modules/jsx-top-level-declarations.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
const createEmptyMetadata = () => ({
22
declarations: [],
3+
importCount: 0,
34
hasTopLevelJsxExpression: false,
45
topLevelJsxExpressionRange: null,
56
})
@@ -28,6 +29,7 @@ export const collectTopLevelTransformMetadata = ({ source, transformJsxSource })
2829

2930
return {
3031
declarations: Array.isArray(result?.declarations) ? result.declarations : [],
32+
importCount: Array.isArray(result?.imports) ? result.imports.length : 0,
3133
hasTopLevelJsxExpression: result?.hasTopLevelJsxExpression === true,
3234
topLevelJsxExpressionRange: isSourceRange(result?.topLevelJsxExpressionRange)
3335
? result.topLevelJsxExpressionRange

src/modules/render-runtime.js

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -404,8 +404,12 @@ export const createRenderRuntimeController = ({
404404
const hasAppDeclaration = declarations =>
405405
hasFunctionLikeDeclarationNamed({ declarations, name: 'App' })
406406

407+
const isComponentLikeName = name => typeof name === 'string' && /^[A-Z]/.test(name)
408+
407409
const getComponentNames = declarations =>
408-
getFunctionLikeDeclarationNames({ declarations, excludeNames: ['App'] })
410+
getFunctionLikeDeclarationNames({ declarations, excludeNames: ['App'] }).filter(
411+
isComponentLikeName,
412+
)
409413

410414
const isSourceRange = range =>
411415
Array.isArray(range) &&
@@ -451,27 +455,41 @@ export const createRenderRuntimeController = ({
451455
return source
452456
}
453457

454-
const { declarations, hasTopLevelJsxExpression, topLevelJsxExpressionRange } =
455-
getTopLevelTransformMetadata({ source, transformJsxSource })
458+
const {
459+
declarations,
460+
importCount,
461+
hasTopLevelJsxExpression,
462+
topLevelJsxExpressionRange,
463+
} = getTopLevelTransformMetadata({ source, transformJsxSource })
456464
if (hasAppDeclaration(declarations)) {
457465
return source
458466
}
459467

460-
const componentNames = getComponentNames(declarations)
461-
if (componentNames.length > 0) {
462-
const children = componentNames.map(name => ` <${name} />`).join('\n')
463-
return `${source}\n\nconst App = () => (\n <>\n${children}\n </>\n)`
464-
}
465-
466468
if (hasTopLevelJsxExpression) {
467469
const expressionSource = sourceFromRange({
468470
source,
469471
range: topLevelJsxExpressionRange,
470472
})
471473

472-
if (expressionSource) {
473-
return `const App = () => (${expressionSource})`
474+
if (!expressionSource) {
475+
throw new Error(
476+
'Unable to infer top-level JSX entry for implicit App. Define App explicitly.',
477+
)
474478
}
479+
480+
if (declarations.length > 0 || importCount > 0) {
481+
throw new Error(
482+
'Top-level JSX with declarations or imports requires an explicit App component.',
483+
)
484+
}
485+
486+
return `const App = () => (${expressionSource})`
487+
}
488+
489+
const componentNames = getComponentNames(declarations)
490+
if (componentNames.length > 0) {
491+
const children = componentNames.map(name => ` <${name} />`).join('\n')
492+
return `${source}\n\nconst App = () => (\n <>\n${children}\n </>\n)`
475493
}
476494

477495
return source
@@ -685,6 +703,16 @@ export const createRenderRuntimeController = ({
685703
? withImplicitAppWrapper(source, transformJsxSource)
686704
: source
687705
const userCode = executableSource
706+
.replace(
707+
/^\s*export\s+default\s+([A-Za-z_$][\w$]*)\s*;?\s*$/gm,
708+
(match, identifier) => {
709+
if (identifier === 'App') {
710+
return ''
711+
}
712+
713+
return `const App = ${identifier}`
714+
},
715+
)
688716
.replace(/^\s*export\s+default\s+/gm, 'const App = ')
689717
.replace(/^\s*export\s+(?=function|const|let|var|class)/gm, '')
690718
.replace(/^\s*export\s*\{[^}]*\}\s*;?\s*$/gm, '')

0 commit comments

Comments
 (0)