ReviewOS
Reload

also looking at this

stacks/stx

feat(dx): dev-mode warning for unbalanced template tags (#1769)

#1770
Merged glennmichael123 wants to merge feat/1769-tag-balance-dev-warning into main
4 files +277 -0

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/stx/src/includes.tsmodified+15-0
@@ -69,6 +69,7 @@
6969import type { StxOptions } from './types'
7070import path from 'node:path'
7171import { processConditionals } from './conditionals'
72import { isProduction, isTest } from './env'
7273import { processExpressions, usesSignalsInScript } from './expressions'
7374import { LRUCache } from './performance-utils'
7475import { createSafeFunction, isExpressionSafe, safeEvaluate, safeEvaluateObject } from './safe-evaluator'
@@ -880,6 +881,20 @@ catch (error: unknown) {
880881 partialContent = processStreamDirectives(partialContent, context)
881882 }
882883
884 // #1769: dev-mode tag-balance heads-up on the RAW partial, before SFC
885 // extraction / directive processing auto-closes anything. A dropped
886 // </div> silently re-parents following siblings and blanks the page with
887 // no error at any layer; a coarse per-tag open/close count catches it.
888 // On by default in development; opt-in via `debug` elsewhere; off in prod
889 // + the test runner. Pre-gated so the dynamic import is dev-only.
890 if (partialContent && (options.debug || (!isProduction() && !isTest()))) {
891 try {
892 const { warnUnbalancedTags } = await import('./template-tag-balance')
893 warnUnbalancedTags(partialContent, includeFilePath)
894 }
895 catch {}
896 }
897
883898 // SFC Support: Extract <template>, <script>, and <style> sections
884899 let workingContent = partialContent
885900
packages/stx/src/process.tsmodified+11-0
@@ -60,6 +60,8 @@ import { processInlineAssets } from './inline-assets'
6060import { addCloakToConditionalDirectives, addCloakToUnresolvedExpressions, processJsonDirective, processMemoDirective, processOnceDirective, processRefAttributes } from './misc-directives'
6161import { validateClientScript } from './script-validation'
6262import { safeEvaluate } from './safe-evaluator'
63import { isProduction, isTest } from './env'
64import { warnUnbalancedTags } from './template-tag-balance'
6365import { deriveLayoutGroup } from 'stx-router/layout-metadata'
6466
6567// Re-export public API from extracted modules (preserves backwards compatibility)
@@ -299,6 +301,15 @@ export async function processDirectives(
299301 context.__onceStore = new Set<string>()
300302 }
301303
304 // #1769: dev-mode tag-balance heads-up for top-level views/layouts, on the
305 // RAW template before any processing auto-closes tags. @include'd partials
306 // are checked separately in the include pipeline. On by default in
307 // development; opt-in via `debug` elsewhere; off in prod + the test runner.
308 // Deduped + best-effort.
309 if ((options.debug || (!isProduction() && !isTest())) && filePath && filePath.endsWith('.stx')) {
310 warnUnbalancedTags(template, filePath)
311 }
312
302313 try {
303314 return await performanceMonitor.timeAsync('template-processing', async () => {
304315 let result = await processDirectivesInternal(template, context, filePath, options, dependencies)
packages/stx/src/template-tag-balance.tsadded+146-0
@@ -0,0 +1,146 @@
1/**
2 * Dev-mode template tag-balance check (stacksjs/stx#1769).
3 *
4 * A single dropped closing tag (e.g. a missing `</div>`) is silently
5 * auto-closed by the HTML parser at EOF, re-parenting every following sibling
6 * INSIDE the unclosed element. Those siblings then inherit the element's
7 * `x-show`/`display:none` state and render blank — with zero diagnostics at any
8 * layer (the server 200s, hydration succeeds, no console output). The failure
9 * is purely structural and only visible by eye.
10 *
11 * This does a coarse, void- and optional-close-aware open/close COUNT per tag
12 * name and reports any mismatch. It intentionally does not attempt a full HTML
13 * parse: the goal is a cheap, high-signal dev heads-up, not a validator.
14 */
15
16// Void elements never take a closing tag — excluded from counting entirely.
17const VOID_TAGS: ReadonlySet<string> = new Set([
18 'area', 'base', 'br', 'col', 'embed', 'hr', 'img', 'input',
19 'link', 'meta', 'param', 'source', 'track', 'wbr',
20])
21
22// Elements whose end tag may be legally omitted (HTML spec). A count mismatch
23// on these is usually idiomatic (`<li>`/`<td>`/`<p>` without a close), so we
24// never warn on them — this is what keeps the check false-positive-free.
25const OPTIONAL_CLOSE_TAGS: ReadonlySet<string> = new Set([
26 'html', 'head', 'body', 'p', 'li', 'dt', 'dd', 'rt', 'rp',
27 'option', 'optgroup', 'thead', 'tbody', 'tfoot', 'tr', 'td', 'th',
28 'colgroup', 'caption',
29])
30
31export interface TagBalanceIssue {
32 /** Tag name as written (case preserved, so `<Icon>`/`</Icon>` match). */
33 tag: string
34 opened: number
35 closed: number
36 /** 1-based line of the first opening tag of this name (best-effort). */
37 firstOpenLine: number
38}
39
40/** Replace every non-newline char with a space so line/column numbers of the
41 * surviving text stay accurate after a region is neutralised. */
42function blankKeepingLines(s: string): string {
43 return s.replace(/[^\n]/g, ' ')
44}
45
46/**
47 * Return the set of tag names whose opening/closing counts don't balance.
48 * Pure and side-effect free.
49 */
50export function findUnbalancedTags(template: string): TagBalanceIssue[] {
51 // Neutralise regions where a `<`/`>` is not markup: scripts, styles, HTML
52 // comments, stx interpolations, and quoted attribute values (so a literal
53 // `>` inside an attribute value can't prematurely "close" a tag).
54 const stripped = template
55 .replace(/<script\b[^>]*>[\s\S]*?<\/script>/gi, blankKeepingLines)
56 .replace(/<style\b[^>]*>[\s\S]*?<\/style>/gi, blankKeepingLines)
57 .replace(/<!--[\s\S]*?-->/g, blankKeepingLines)
58 .replace(/\{\{\{[\s\S]*?\}\}\}/g, blankKeepingLines)
59 .replace(/\{\{[\s\S]*?\}\}/g, blankKeepingLines)
60 .replace(/\{!![\s\S]*?!!\}/g, blankKeepingLines)
61 .replace(/"[^"]*"/g, blankKeepingLines)
62 .replace(/'[^']*'/g, blankKeepingLines)
63
64 interface OpenInfo { count: number, firstLine: number }
65 const opens = new Map<string, OpenInfo>()
66 const closes = new Map<string, number>()
67
68 const tagRe = /<(\/?)([A-Za-z][\w-]*)\b[^>]*?(\/?)>/g
69 let m: RegExpExecArray | null
70 // eslint-disable-next-line no-cond-assign
71 while ((m = tagRe.exec(stripped)) !== null) {
72 const isClose = m[1] === '/'
73 const name = m[2]
74 const selfClose = m[3] === '/'
75 if (VOID_TAGS.has(name.toLowerCase()))
76 continue
77 if (selfClose)
78 continue
79 if (isClose) {
80 closes.set(name, (closes.get(name) ?? 0) + 1)
81 }
82 else {
83 const existing = opens.get(name)
84 if (existing) {
85 existing.count++
86 }
87 else {
88 const line = stripped.slice(0, m.index).split('\n').length
89 opens.set(name, { count: 1, firstLine: line })
90 }
91 }
92 }
93
94 const issues: TagBalanceIssue[] = []
95 for (const name of new Set([...opens.keys(), ...closes.keys()])) {
96 if (OPTIONAL_CLOSE_TAGS.has(name.toLowerCase()))
97 continue
98 const opened = opens.get(name)?.count ?? 0
99 const closed = closes.get(name) ?? 0
100 if (opened !== closed)
101 issues.push({ tag: name, opened, closed, firstOpenLine: opens.get(name)?.firstLine ?? 0 })
102 }
103 return issues
104}
105
106// Dedupe key = filePath + template length, so a given broken file warns once
107// (not once per request or per @include site); an edit that changes the file's
108// length re-arms the warning. Dev-only + bounded to actually-broken files.
109const warnedKeys = new Set<string>()
110
111/**
112 * Dev-mode warning: log any tag-balance issues for a template. Best-effort — a
113 * diagnostic must never throw and break the build.
114 */
115export function warnUnbalancedTags(template: string, filePath: string): void {
116 let issues: TagBalanceIssue[]
117 try {
118 issues = findUnbalancedTags(template)
119 }
120 catch {
121 return
122 }
123 if (issues.length === 0)
124 return
125
126 const dedupeKey = `${filePath}:${template.length}`
127 if (warnedKeys.has(dedupeKey))
128 return
129 warnedKeys.add(dedupeKey)
130
131 const label = filePath ? (filePath.split(/[/\\]/).pop() || filePath) : 'template'
132 for (const { tag, opened, closed, firstOpenLine } of issues) {
133 if (opened > closed) {
134 const where = firstOpenLine ? ` (first opened at line ${firstOpenLine})` : ''
135 console.warn(
136 `[stx] ${label}: <${tag}> ${opened} opened, ${closed} closed — an unclosed <${tag}>${where} `
137 + `may swallow the content that follows it.`,
138 )
139 }
140 else {
141 console.warn(
142 `[stx] ${label}: <${tag}> ${opened} opened, ${closed} closed — ${closed - opened} stray </${tag}>.`,
143 )
144 }
145 }
146}
packages/stx/test/template-tag-balance.test.tsadded+105-0
@@ -0,0 +1,105 @@
1/**
2 * Tests for the dev-mode template tag-balance check (stacksjs/stx#1769).
3 *
4 * A dropped closing tag (e.g. a missing </div>) is auto-closed at EOF and
5 * silently re-parents the following siblings inside the unclosed element,
6 * blanking the page with no error at any layer. These verify the coarse
7 * open/close count catches that while staying false-positive-free on void
8 * elements, optional-close elements, self-closing components, and tags that
9 * appear inside scripts/styles/comments/interpolations/attributes.
10 */
11import { describe, expect, it, spyOn } from 'bun:test'
12import { processDirectives } from '../src/process'
13import { findUnbalancedTags, warnUnbalancedTags } from '../src/template-tag-balance'
14
15describe('findUnbalancedTags (#1769)', () => {
16 it('flags a dropped closing tag', () => {
17 const issues = findUnbalancedTags(`<div><div x-show="a"><p>A</p><div x-show="b"><p>B</p></div></div>`)
18 expect(issues).toHaveLength(1)
19 expect(issues[0].tag).toBe('div')
20 expect(issues[0].opened).toBe(3)
21 expect(issues[0].closed).toBe(2)
22 expect(issues[0].firstOpenLine).toBe(1)
23 })
24
25 it('is silent for balanced markup', () => {
26 expect(findUnbalancedTags(`<div><section><p>hi</p></section></div>`)).toEqual([])
27 })
28
29 it('ignores void elements', () => {
30 expect(findUnbalancedTags(`<div><br><img src="x"><input></div>`)).toEqual([])
31 })
32
33 it('ignores self-closing components', () => {
34 expect(findUnbalancedTags(`<div><Icon name="x" /><Foo-Bar /></div>`)).toEqual([])
35 })
36
37 it('ignores optional-close elements (li / td / p)', () => {
38 expect(findUnbalancedTags(`<ul><li>a<li>b</ul><table><tr><td>x<td>y</table>`)).toEqual([])
39 })
40
41 it('ignores tags inside scripts, styles, and comments', () => {
42 expect(findUnbalancedTags(`<div><script>if(a<b){}</script><style>.x>.y{}</style><!-- <div> --></div>`)).toEqual([])
43 })
44
45 it('ignores > inside attributes and interpolations', () => {
46 expect(findUnbalancedTags(`<div title="a > b">0<span>x</span></div>`)).toEqual([])
47 })
48
49 it('flags an unbalanced component tag (case preserved)', () => {
50 const issues = findUnbalancedTags(`<div><StxLink to="/a"><span>x</span></div>`)
51 expect(issues.some(i => i.tag === 'StxLink' && i.opened === 1 && i.closed === 0)).toBe(true)
52 })
53
54 it('flags a stray closing tag', () => {
55 const issues = findUnbalancedTags(`<div><span>x</span></div></div>`)
56 expect(issues[0].tag).toBe('div')
57 expect(issues[0].closed).toBeGreaterThan(issues[0].opened)
58 })
59
60 it('keeps accurate line numbers past neutralised regions', () => {
61 const tpl = `<div>\n<script>\n<div>\n</script>\n<section>\n<div>\n</div>\n<span>x</span>`
62 const issues = findUnbalancedTags(tpl)
63 // the <div> inside <script> is ignored; the outer <div> on line 1 is unclosed
64 expect(issues.find(i => i.tag === 'div')?.firstOpenLine).toBe(1)
65 })
66})
67
68describe('warnUnbalancedTags (#1769)', () => {
69 it('warns with a helpful, file-labelled message', () => {
70 const warn = spyOn(console, 'warn').mockImplementation(() => {})
71 warnUnbalancedTags(`<div><div x-show="a"><p>A</p></div>`, '/x/SettingsTab.stx')
72 const msg = warn.mock.calls.map(c => c.join(' ')).join('\n')
73 expect(msg).toContain('SettingsTab.stx')
74 expect(msg).toContain('<div>')
75 expect(msg).toMatch(/opened/)
76 warn.mockRestore()
77 })
78
79 it('does not throw and stays silent on balanced input', () => {
80 const warn = spyOn(console, 'warn').mockImplementation(() => {})
81 expect(() => warnUnbalancedTags(`<div><p>ok</p></div>`, '/x/Ok.stx')).not.toThrow()
82 expect(warn.mock.calls.some(c => c.join(' ').includes('opened'))).toBe(false)
83 warn.mockRestore()
84 })
85})
86
87describe('processDirectives tag-balance integration (#1769)', () => {
88 it('warns in debug mode when a top-level template drops a </div>', async () => {
89 const warn = spyOn(console, 'warn').mockImplementation(() => {})
90 const tpl = `<div><div x-show="a"><p>A</p><div x-show="b"><p>B</p></div></div>`
91 await processDirectives(tpl, { props: {} }, '/proj/TabPanelWidget.stx', { debug: true } as any, new Set())
92 const balMsg = warn.mock.calls.map(c => c.join(' ')).filter(l => l.includes('opened')).join('\n')
93 expect(balMsg).toContain('TabPanelWidget.stx')
94 warn.mockRestore()
95 })
96
97 it('does not emit a balance warning when debug is off', async () => {
98 const warn = spyOn(console, 'warn').mockImplementation(() => {})
99 const tpl = `<section><div x-show="a"><p>A</p><div><p>B</p></div></section>`
100 await processDirectives(tpl, { props: {} }, '/proj/NoDebugWidget.stx', { debug: false } as any, new Set())
101 const balMsg = warn.mock.calls.map(c => c.join(' ')).filter(l => l.includes('opened')).join('\n')
102 expect(balMsg).toBe('')
103 warn.mockRestore()
104 })
105})