Skip to content

Commit 78dd97b

Browse files
refactor: address comments, add clear styles button.
1 parent 763c48b commit 78dd97b

7 files changed

Lines changed: 120 additions & 18 deletions

File tree

AGENTS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,8 @@ Repository structure:
4848

4949
## CDN and runtime expectations
5050

51-
- Keep dependency loading compatible with existing provider/fallback model in src/cdn.js.
52-
- Treat src/cdn.js as the source of truth for CDN-managed runtime libraries; add/update
51+
- Keep dependency loading compatible with existing provider/fallback model in src/modules/cdn.js.
52+
- Treat src/modules/cdn.js as the source of truth for CDN-managed runtime libraries; add/update
5353
CDN candidates there instead of hardcoding module URLs in feature modules.
5454
- Prefer extending existing CDN import key patterns instead of ad hoc dynamic imports.
5555
- Maintain graceful fallback behavior when CDN modules fail to load.

docs/build-and-deploy.md

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
This project uses two runtime modes:
44

5-
- Local development mode: dynamic CDN resolution from `src/cdn.js` with esm.sh as default.
5+
- Local development mode: dynamic CDN resolution from `src/modules/cdn.js` with esm.sh as default.
66
- Production mode: CDN-first build artifacts in `dist`, with `build:esm` as the current preferred deploy build.
77

88
## Local Development
@@ -45,8 +45,8 @@ npm run build:importmap-mode
4545
| Mode | Resolver | Import map step | JSPM index needed | Typical use |
4646
| --- | --- | --- | --- | --- |
4747
| `importMap` | Import map in `dist/index.html` | Yes | Yes | Default production mode |
48-
| `esm` | `src/cdn.js` (`esm.sh` primary) | No | No | Stable fallback mode |
49-
| `jspmGa` | `src/cdn.js` (`ga.jspm.io` primary) | No | No | Direct ga.jspm.io testing |
48+
| `esm` | `src/modules/cdn.js` (`esm.sh` primary) | No | No | Stable fallback mode |
49+
| `jspmGa` | `src/modules/cdn.js` (`ga.jspm.io` primary) | No | No | Direct ga.jspm.io testing |
5050
<!-- prettier-ignore-end -->
5151

5252
Mode notes:
@@ -72,7 +72,7 @@ This runs two steps:
7272
- `sass=1.93.2`
7373
- `less=4.4.2`
7474
- Traces generated `dist/prod-imports.js`
75-
- Import specifiers come from `importMap` entries in `src/cdn.js` (`cdnImportSpecs`)
75+
- Import specifiers come from `importMap` entries in `src/modules/cdn.js` (`cdnImportSpecs`)
7676

7777
Preview the built site locally:
7878

@@ -99,20 +99,20 @@ Related docs:
9999

100100
- `docs/code-mirror.md` for CodeMirror CDN integration rules, fallback behavior, and validation checklist.
101101

102-
- `src/cdn.js` is the source of truth for CDN-managed runtime libraries (including fallback candidates). Add/update CDN specs there instead of hardcoding module URLs inside feature modules.
102+
- `src/modules/cdn.js` is the source of truth for CDN-managed runtime libraries (including fallback candidates). Add/update CDN specs there instead of hardcoding module URLs inside feature modules.
103103

104104
- In production, the current preferred deploy mode is ESM resolution (`window.__KNIGHTED_PRIMARY_CDN__ = "esm"`).
105105
- In `importMap` mode, runtime resolution is import-map first; if a specifier is missing from the generated map, runtime falls back through the CDN
106-
provider chain configured in `src/cdn.js`.
107-
- In `esm` and `jspmGa` modes, runtime resolution is handled entirely by the CDN provider chain configured in `src/cdn.js` without an import map.
106+
provider chain configured in `src/modules/cdn.js`.
107+
- In `esm` and `jspmGa` modes, runtime resolution is handled entirely by the CDN provider chain configured in `src/modules/cdn.js` without an import map.
108108

109109
### Sass Loading Gotchas
110110

111111
- Symptom: switching to Sass mode shows `Unable to load Sass compiler for browser usage: Dynamic require of "url" is not supported`.
112112
- Cause: some `esm.sh` Sass outputs currently include runtime paths that are not browser-safe for this app.
113-
- Current mitigation: `src/cdn.js` keeps `esm.sh` first, then falls back to `unpkg` for Sass via `sass@1.93.2/sass.default.js?module`.
113+
- Current mitigation: `src/modules/cdn.js` keeps `esm.sh` first, then falls back to `unpkg` for Sass via `sass@1.93.2/sass.default.js?module`.
114114
- Important context: this can appear even if the Sass URL has not changed in this repo, because CDN-transformed module output can change upstream.
115115
- If this regresses again:
116-
- Verify Sass import candidates in `src/cdn.js`.
116+
- Verify Sass import candidates in `src/modules/cdn.js`.
117117
- Reproduce directly in browser devtools with `await import('<candidate-url>')`.
118118
- Keep at least one known browser-safe fallback provider in the Sass candidate list.

playwright/app.spec.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -658,6 +658,34 @@ test('clear all diagnostics removes style compile diagnostics', async ({ page })
658658
)
659659
})
660660

661+
test('clear styles diagnostics removes style compile diagnostics', async ({ page }) => {
662+
await waitForInitialRender(page)
663+
664+
await ensurePanelToolsVisible(page, 'styles')
665+
666+
await page.locator('#style-mode').selectOption('sass')
667+
await setStylesEditorSource(page, '.card { color: $missing; }')
668+
669+
await expect(page.locator('#diagnostics-toggle')).toHaveClass(
670+
/diagnostics-toggle--error/,
671+
)
672+
673+
await page.locator('#diagnostics-toggle').click()
674+
await expect(page.locator('#diagnostics-styles')).toContainText(
675+
'Style compilation failed.',
676+
)
677+
678+
await page.locator('#diagnostics-clear-styles').click()
679+
await expect(page.locator('#diagnostics-component')).toContainText(
680+
'No diagnostics yet.',
681+
)
682+
await expect(page.locator('#diagnostics-styles')).toContainText('No diagnostics yet.')
683+
await expect(page.locator('#diagnostics-toggle')).toHaveText('Diagnostics')
684+
await expect(page.locator('#diagnostics-toggle')).toHaveClass(
685+
/diagnostics-toggle--neutral/,
686+
)
687+
})
688+
661689
test('typecheck success reports ok diagnostics state in button and drawer', async ({
662690
page,
663691
}) => {
@@ -722,6 +750,38 @@ test('component lint error reports diagnostics count and details', async ({ page
722750
)
723751
})
724752

753+
test('clear component diagnostics resets rendered lint-issue status pill', async ({
754+
page,
755+
}) => {
756+
await waitForInitialRender(page)
757+
758+
await setComponentEditorSource(
759+
page,
760+
[
761+
'const unusedValue = 1',
762+
'const App = () => <button type="button">lint me</button>',
763+
].join('\n'),
764+
)
765+
766+
await runComponentLint(page)
767+
768+
await expect(page.locator('#status')).toHaveText(/Rendered \(Lint issues: [1-9]\d*\)/)
769+
await expect(page.locator('#status')).toHaveClass(/status--error/)
770+
771+
await page.locator('#diagnostics-toggle').click()
772+
await page.locator('#diagnostics-clear-component').click()
773+
774+
await expect(page.locator('#diagnostics-component')).toContainText(
775+
'No diagnostics yet.',
776+
)
777+
await expect(page.locator('#diagnostics-toggle')).toHaveText('Diagnostics')
778+
await expect(page.locator('#diagnostics-toggle')).toHaveClass(
779+
/diagnostics-toggle--neutral/,
780+
)
781+
await expect(page.locator('#status')).toHaveText('Rendered')
782+
await expect(page.locator('#status')).toHaveClass(/status--neutral/)
783+
})
784+
725785
test('component lint ignores unused App View and render bindings', async ({ page }) => {
726786
await waitForInitialRender(page)
727787

src/app.js

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ const diagnosticsToggle = document.getElementById('diagnostics-toggle')
4040
const diagnosticsDrawer = document.getElementById('diagnostics-drawer')
4141
const diagnosticsClose = document.getElementById('diagnostics-close')
4242
const diagnosticsClearComponent = document.getElementById('diagnostics-clear-component')
43+
const diagnosticsClearStyles = document.getElementById('diagnostics-clear-styles')
4344
const diagnosticsClearAll = document.getElementById('diagnostics-clear-all')
4445
const diagnosticsComponent = document.getElementById('diagnostics-component')
4546
const diagnosticsStyles = document.getElementById('diagnostics-styles')
@@ -469,7 +470,8 @@ const syncLintPendingState = () => {
469470

470471
const runComponentLint = async () => {
471472
activeComponentLintAbortController?.abort()
472-
activeComponentLintAbortController = new AbortController()
473+
const controller = new AbortController()
474+
activeComponentLintAbortController = controller
473475
componentLintPending = false
474476
syncLintPendingState()
475477
incrementLintDiagnosticsRuns()
@@ -478,20 +480,24 @@ const runComponentLint = async () => {
478480

479481
try {
480482
const result = await lintDiagnostics.lintComponent({
481-
signal: activeComponentLintAbortController.signal,
483+
signal: controller.signal,
482484
})
483485
if (result) {
484486
lastComponentLintIssueCount = result.issueCount
485487
}
486488
} finally {
487489
decrementLintDiagnosticsRuns()
488-
setLintButtonLoading({ button: lintComponentButton, isLoading: false })
490+
if (activeComponentLintAbortController === controller) {
491+
activeComponentLintAbortController = null
492+
setLintButtonLoading({ button: lintComponentButton, isLoading: false })
493+
}
489494
}
490495
}
491496

492497
const runStylesLint = async () => {
493498
activeStylesLintAbortController?.abort()
494-
activeStylesLintAbortController = new AbortController()
499+
const controller = new AbortController()
500+
activeStylesLintAbortController = controller
495501
stylesLintPending = false
496502
syncLintPendingState()
497503
incrementLintDiagnosticsRuns()
@@ -500,14 +506,17 @@ const runStylesLint = async () => {
500506

501507
try {
502508
const result = await lintDiagnostics.lintStyles({
503-
signal: activeStylesLintAbortController.signal,
509+
signal: controller.signal,
504510
})
505511
if (result) {
506512
lastStylesLintIssueCount = result.issueCount
507513
}
508514
} finally {
509515
decrementLintDiagnosticsRuns()
510-
setLintButtonLoading({ button: lintStylesButton, isLoading: false })
516+
if (activeStylesLintAbortController === controller) {
517+
activeStylesLintAbortController = null
518+
setLintButtonLoading({ button: lintStylesButton, isLoading: false })
519+
}
511520
}
512521
}
513522

@@ -797,6 +806,12 @@ if (diagnosticsClearComponent) {
797806
}
798807
})
799808
}
809+
if (diagnosticsClearStyles) {
810+
diagnosticsClearStyles.addEventListener('click', () => {
811+
clearDiagnosticsScope('styles')
812+
clearStylesLintDiagnosticsState()
813+
})
814+
}
800815
if (diagnosticsClearAll) {
801816
diagnosticsClearAll.addEventListener('click', () => {
802817
clearAllDiagnostics()

src/index.html

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,9 @@ <h2>Diagnostics</h2>
379379
<button class="render-button" id="diagnostics-clear-component" type="button">
380380
Clear component
381381
</button>
382+
<button class="render-button" id="diagnostics-clear-styles" type="button">
383+
Clear styles
384+
</button>
382385
<button class="render-button" id="diagnostics-clear-all" type="button">
383386
Clear all
384387
</button>

src/modules/diagnostics-ui.js

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@ export const createDiagnosticsUiController = ({
5252
diagnosticsByScope.component.level === 'ok' ||
5353
diagnosticsByScope.styles.level === 'ok'
5454

55+
const hasDiagnosticsErrorResult = () =>
56+
diagnosticsByScope.component.level === 'error' ||
57+
diagnosticsByScope.styles.level === 'error'
58+
5559
const hasActiveDiagnosticsRuns = () =>
5660
activeTypeDiagnosticsRuns > 0 || activeLintDiagnosticsRuns > 0
5761

@@ -60,7 +64,7 @@ export const createDiagnosticsUiController = ({
6064
return 'pending'
6165
}
6266

63-
if (getDiagnosticsIssueCount() > 0) {
67+
if (getDiagnosticsIssueCount() > 0 || hasDiagnosticsErrorResult()) {
6468
return 'error'
6569
}
6670

@@ -73,6 +77,19 @@ export const createDiagnosticsUiController = ({
7377

7478
const updateUiIssueIndicators = () => {
7579
const diagnosticsLevel = getDiagnosticsIssueLevel()
80+
const hasIssues = getDiagnosticsIssueCount() > 0
81+
const isDiagnosticsPending = typeDiagnosticsPending || lintDiagnosticsPending
82+
83+
if (
84+
!hasIssues &&
85+
!isDiagnosticsPending &&
86+
statusLevel === 'error' &&
87+
(statusNode.textContent.startsWith('Rendered (Type errors:') ||
88+
statusNode.textContent.startsWith('Rendered (Lint issues:'))
89+
) {
90+
statusNode.textContent = 'Rendered'
91+
statusLevel = 'neutral'
92+
}
7693

7794
statusNode.classList.remove('status--neutral', 'status--pending', 'status--error')
7895
statusNode.classList.add(`status--${statusLevel}`)

src/styles/diagnostics.css

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,13 @@
167167
}
168168
}
169169

170+
@media (prefers-reduced-motion: reduce) {
171+
.diagnostics-toggle--pending::after {
172+
animation: none;
173+
border-right-color: color-mix(in srgb, var(--accent) 70%, var(--panel-text));
174+
}
175+
}
176+
170177
@media (max-width: 900px) {
171178
.diagnostics-drawer {
172179
top: auto;

0 commit comments

Comments
 (0)