docs: update article. - #146
Conversation
There was a problem hiding this comment.
Pull request overview
Updates docs/article.md to be a publishable article with front matter metadata and expanded/modernized content describing the @knighted/develop browser-native workflow.
Changes:
- Added YAML front matter (title, published flag, description, tags).
- Expanded the “DOM mode” explanation with a concrete TSX example using a Web Worker.
- Refined and reorganized the feature list and the “Try It” steps (workspaces, share URLs, GitHub sync flow).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docs/article.md:53
- The example creates a Blob-backed object URL for the Worker but never revokes it. In a long-lived session (or if users adapt this pattern for repeated workers), this can leak memory; it’s easy to show the safe pattern by storing the URL and revoking it after termination.
const worker = new Worker(
URL.createObjectURL(new Blob([workerCode], { type: 'application/javascript' })),
)
worker.onmessage = e => {
counterEl.textContent = String(e.data)
if (e.data <= 0) worker.terminate()
}
.github/workflows/playwright.yml:8
- Because
pull_requestno longer has apathsfilter, this workflow will still schedule all 5 matrix jobs on every PR update (even docs-only changes). The step-levelif:avoids running tests, but you still pay for runner startup + checkout + paths-filter 5 times; if CI minutes are a concern, consider either restoringpull_request.paths(if branch protection doesn’t require this check on every PR), or splitting into a single required “gate” job plus a conditional matrix job driven by a one-time change-detection job output.
pull_request:
branches:
- main
types:
- opened
docs/article.md:40
- In the example worker timer, the computed setTimeout delay can go negative under scheduler drift or if the tab was throttled, which causes the loop to “catch up” immediately and spam messages. Clamping the delay keeps the example robust and prevents unexpected rapid countdown jumps.
setTimeout(tick, 1000 - ((performance.now() - startTime) - (ticks * 1000)));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/modules/app-core/defaults.js:21
defaultModuleJsxdefines the button element on a single long line, which is hard to read/maintain and diverges from the multi-line example used in the docs. Consider formatting the JSX into multiple string entries so future edits (attributes/children) are less error-prone.
'export const Counter = ({ label }: CounterProps) => {',
" const el = <button class='counter-button' type='button'>{label}: 0</button> as HTMLButtonElement",
' let count = 0',
src/app.js:292
- PR title is "docs: update article.", but this change also modifies the app’s default workspace/template behavior by adding a new default module tab and new default module content. Consider updating the PR title (or splitting) so release notes/semantic categorization matches the runtime change.
const defaultComponentTabPath = 'src/components/App.tsx'
const defaultModuleTabPath = 'src/components/Counter.tsx'
const defaultStylesTabPath = 'src/styles/app.css'
const defaultComponentTabName = 'App.tsx'
const defaultModuleTabName = 'Counter.tsx'
const defaultStylesTabName = 'app.css'
const editorKinds = ['component', 'styles']
const editorPanelsByKind = {
component: componentEditorPanel,
styles: stylesEditorPanel,
}
const editorHeaderLabelByKind = {
component: componentEditorHeaderLabel,
styles: stylesEditorHeaderLabel,
}
const editorHeaderDirtyStatusByKind = {
component: componentEditorDirtyStatus,
styles: stylesEditorDirtyStatus,
}
const defaultTabNameByKind = {
component: defaultComponentTabName,
styles: defaultStylesTabName,
}
jsxEditor.value = defaultJsx
cssEditor.value = defaultCss
let previewHost = document.getElementById('preview-host')
let jsxCodeEditor = null
let cssCodeEditor = null
let diagnosticsFlowController = null
let runtimeCore = null
let getJsxSource = () => jsxEditor.value
let getCssSource = () => cssEditor.value
let renderRuntime = null
let pendingClearAction = null
let suppressEditorChangeSideEffects = false
let appToastDismissTimer = null
const workspaceStorage = createWorkspaceStorageAdapter()
let activeWorkspaceRecordId = ''
let activeWorkspaceCreatedAt = null
let workspacesDrawerController = null
let isApplyingWorkspaceSnapshot = false
let hasCompletedInitialWorkspaceBootstrap = false
const workspaceTabsState = createWorkspaceTabsState({
tabs: [
{
id: 'entry',
name: defaultComponentTabName,
path: defaultComponentTabPath,
language: 'javascript-jsx',
role: 'entry',
isActive: true,
content: defaultJsx,
},
{
id: 'counter',
name: defaultModuleTabName,
path: defaultModuleTabPath,
language: 'javascript-jsx',
role: 'module',
isActive: false,
content: defaultModuleJsx,
},
docs/article.md:59
- The
Counter.tsxsnippet in the article toggles theis-evenclass but never updatesdata-active, even though the default CSS example in this repo uses[data-active='true']styling. Adding thedataset.activeupdate keeps the article’s code sample aligned with the styling behavior readers will see.
el.onclick = () => {
count += 1
el.textContent = `${label}: ${count}`
el.classList.toggle('is-even', count % 2 === 0)
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (4)
playwright/github-pr-drawer/open-pr-create.spec.ts:2100
- Same issue as above: this makes the test dependent on a specific ordering of Git tree entries, which is not required to validate that the correct default paths are submitted. Using order-independent assertions reduces brittleness if tab ordering or file update ordering changes.
const submittedPaths = (treeRequests[0]?.tree as Array<Record<string, unknown>>).map(
entry => entry.path,
)
expect(submittedPaths).toEqual([
'src/components/App.tsx',
'src/components/Counter.tsx',
'src/styles/app.css',
])
src/app.js:39
- The PR title indicates a docs-only change ("docs: update article"), but this diff also changes the default workspace template and UI bootstrap behavior (adds a new default module tab and its contents) plus updates Playwright coverage. Please rename the PR to reflect the runtime/test changes, or split docs vs functional changes into separate PRs to keep review scope clear.
import { defaultCss, defaultJsx, defaultModuleJsx } from './modules/app-core/defaults.js'
src/modules/app-core/defaults.js:21
- defaultModuleJsx inlines the JSX button element into a single very long line, which is inconsistent with the multi-line JSX formatting used in defaultReactJsx and makes the default template harder to read/edit in the editor. Consider formatting the element across multiple lines (and using double quotes in JSX attributes like the other templates) for consistency and readability.
'export const Counter = ({ label }: CounterProps) => {',
" const el = <button class='counter-button' type='button'>{label}: 0</button> as HTMLButtonElement",
' let count = 0',
playwright/github-pr-drawer/open-pr-create.spec.ts:201
- This assertion makes the test depend on the order of entries in the Git tree payload. Ordering isn't part of the behavior being validated here (the important part is that the default workspace paths are included), so this is unnecessarily brittle if tab ordering changes. Prefer asserting membership (arrayContaining) plus length, and avoid throwing if
treeis unexpectedly undefined.
This issue also appears on line 2093 of the same file.
const submittedPaths = (treeRequests[0]?.tree as Array<Record<string, unknown>>).map(
entry => entry.path,
)
expect(submittedPaths).toEqual([
'src/components/App.tsx',
'src/components/Counter.tsx',
'src/styles/app.css',
])
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/app.js:39
- PR title suggests a docs-only change, but this PR also changes runtime defaults (adds a new default workspace module tab) and updates Playwright expectations. Consider updating the PR title/description (or splitting) so reviewers/users understand the behavior change scope.
import { defaultCss, defaultJsx, defaultModuleJsx } from './modules/app-core/defaults.js'
No description provided.