feat(alpha.5): SPA mode + client router + desktop proof#121
Conversation
#115 Client Router Core - createRouter() in @openelement/router/client-router - history/hash/auto modes, named/optional params, guards, query parsing - 15 tests #116 Route Lazy Loading - route-manifest.ts in @openelement/adapter-vite - scan routes/ → generate lazy import() manifest - 16 tests #117 SPA Bootstrap - defineApp({ mode: 'spa' }) in @openelement/app/spa - mount/dispose lifecycle, router integration - 12 tests #118 Client Data Layer - loader/action run client-side in SPA mode - useLoaderData()/useActionData() work without server - 7 tests #119 Deno Desktop Proof - examples/deno-desktop-spa/ — SPA + Deno.serve() + deno desktop - fmt/lint/check pass
Deploying openelement with
|
| Latest commit: |
fd1becf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c2ce2133.lessjs.pages.dev |
| Branch Preview URL: | https://dev.lessjs.pages.dev |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Here's my review: CRITICAL1. SPA hardcodes
|
| Priority | Count | Key issues |
|---|---|---|
| Critical | 2 | Hardcoded history mode, duplicate popstate listener |
| Medium | 3 | Dead type, broken example, incomplete build task |
| Low | 3 | Anti-pattern re-exports, wasteful loop, version inconsistency |
The router core (client-router.ts) and the data-context integration are solid. The main problems are in the SPA glue layer (spa.ts) — the mode: 'history' hardcode and the duplicate popstate listener should be fixed before merge. The rest are cleanup-level.
|
Here's my review: CRITICAL — none foundThe previously-flagged critical issues (hardcoded BUG1.
|
| Priority | Count | Key issues |
|---|---|---|
| Bug | 2 | Windows sep replace bug, + not decoded in query strings |
| Medium | 2 | Dead routes file in example, missing peer dependency |
| Low | 3 | Swallowed render errors, misleading JSDoc, fragile alias order |
The router core (client-router.ts) and data-context integration are solid. The onChange callback pattern correctly avoids the duplicate-listener bug that was in earlier versions. The test coverage is thorough (84/84 passing) with well-designed mock helpers. The formatError cause-chain enhancement in errors.ts is a nice defensive improvement.
The two bugs (replaceAll and + decoding) should be fixed before merge. The rest are cleanup-level.
|
Review complete. The two prior bot reviews already caught and fixed 12+ issues. The remaining concerns are the data stack race condition (acknowledged but unfixed), the |
|
Here's my review: Review: feat(alpha.5) — SPA mode + client router + desktop proofI reviewed all 24 changed files against the 3 prior bot review threads. The follow-up commits ( BUGNone found. The MEDIUM1. 2. try {
await Deno.mkdir(outDir, { recursive: true });
} catch (e) {
if (!(e instanceof Deno.errors.AlreadyExists)) throw e;
}LOW3. 4. 5. Dead Summary
The router core ( Verdict: merge-ready. The 2 medium items can be addressed in a follow-up without blocking this PR. |
Review: SPA mode + client routerThe three prior bot reviews already caught and fixed 12+ issues (hardcoded BUG1. When route A's guard returns // Line 226-228
if (typeof result === 'string') {
return navigate(result); // ← unbounded recursion, no depth guard
}Fix: Add a 2. The ponytail comment at lines 86-88 acknowledges this: if a second navigation fires before the first // Line 93: pops data pushed by the NEXT render, not this render
__internal_popData();
// ...
const loaderData = await runLoader(); // ← async gap: another renderRoute may have startedFix: Add a render-id counter — skip stale renders when the counter has changed. HIGH / MEDIUM3. When // Line 243
return navigate(result); // ← pushes history, should replaceSemantics: LOW4. Fragile route count via regex const matches = content.match(/=> import\(/g);
return matches ? matches.length : 0;Counting routes by matching Summary
The router core ( |
ReviewThe follow-up commits addressed all 12+ issues from prior reviews — hardcoded BUG1. Guard redirect infinite recursion — if (typeof result === 'string') {
return navigate(result); // ← no depth guard
}If route A's guard returns 2. Per Deno docs, MEDIUM3. async function replace(path: string): Promise<void> {
// ... guard check ...
if (typeof result === 'string') {
return navigate(result); // ← pushState, not replaceState
}
history.replaceState(...);
}
4. Stale-render race — The ponytail comment at lines 86-88 acknowledges: if LOW5. Fragile route count in generated manifest — const matches = content.match(/=> import\(/g);
return matches ? matches.length : 0;Counting Summary
The router core ( |
Review: feat(alpha.5) — SPA mode + client router + desktop proofI reviewed the 24 changed files against all 4 prior bot review threads. The follow-up commits addressed 12+ previously-flagged issues: hardcoded BUG1. Guard redirect infinite recursion — no depth counter if (typeof result === 'string') {
return navigate(result); // ← unbounded recursion, no depth guard
}If guard A returns 2. Stale-render race (acknowledged, unfixed) __internal_popData(); // line 93: may pop the wrong frame
const loaderData = await runLoader(); // line 96: async gap — another renderRoute may have started
__internal_pushLoaderData(loaderData); // line 97: pushes a second frame, data leak between routesIf a second navigation fires before the first HIGH3. if (typeof result === 'string') {
return navigate(result); // ← calls pushState, should be replaceState
}
MEDIUM4. Fragile route count via regex const matches = content.match(/=> import\(/g);
return matches ? matches.length : 0;The return entries.length;5. Deno Desktop example: dead LOW6. Summary
What's good: The router core ( |
Review: feat(alpha.5) — SPA mode + client router + desktop proofI reviewed all 26 changed files against the 7 prior bot review threads. The follow-up commits resolved all previously-flagged issues: hardcoded BUG1. void Promise.resolve(options.onChange?.()).catch((err) => {
console.error('[router] onChange failed:', err);
});
} catch (err) {
console.error('[router] onChange failed:', err); // ← never reached
}
MEDIUM2. Desktop example renders an empty DOM — no visible SPA behavior const app = defineApp({ mode: 'spa' });
app.mount('#root');No routes are passed, so 3. "build": "deno check src/index.ts"
LOW4. Stale comment: "ponytail: detect file:// protocol" now uses The comment says "ponytail: detect file:// protocol" — this is no longer a ponytail; 5. Router build task uses positional args — fragile if files are reordered "build": "deno check src/data-context.ts src/client-router.ts"Positional arguments are order-sensitive for readability but not correctness. Consider What's good
Summary
All previously-flagged critical/high issues are resolved. This is merge-ready — the one bug is a dead-code cleanup that doesn't affect behavior. The router and SPA bootstrap code are production-quality. |
Bot review triage
93/93 tests pass. Awaiting final review. |
|
Now I have a thorough understanding of the code. Let me provide my final review. Review: feat(alpha.5) — SPA mode + client router + desktop proofI reviewed all 26 changed files against the 7 prior bot review threads. The follow-up commits ( CRITICAL — none foundAll previously-flagged critical/high issues are resolved in the current code ( BUG — none foundThe LOW1. async function handleFormSubmit(event: Event): Promise<void> {
// ...
const loaderData = await runLoader(); // line 141 — async gap
__internal_pushLoaderData(loaderData); // line 144
__internal_pushActionData(actionData); // line 145
renderComponent(); // line 147 — no renderId check
}If a navigation fires mid-flight during the action's 2. 3. 4. What's good
Summary
Verdict: merge-ready. All 14+ previously-flagged issues across 7 review rounds are resolved. The router and SPA bootstrap code is production-quality for alpha.5. The 4 low items are cleanup-level that can be addressed in follow-up PRs. |
S1: Shell scaffold — deno.json, main.ts (Deno.serve HTTP + /api/*), CSS theming S2: Fixtures — 3 public domain books + placeholder PDFs S3: Route registry — 5 routes (bookshelf, reading, notes, search, settings) S4: Persistence — repo.ts (GitHub pull + sha cache), search.ts (pdf-parse), storage.ts (localStorage CRUD) S5: Reader UX — keyboard shortcuts, theme controls, note creation, Markdown export S6: Smoke tests — 27/27 (6 export + 6 routes + 15 storage) Architecture: - GitHub repo as content backend (READER_REPO env var, default open-element/reader-fixtures) - Sha-based incremental sync to ~/.open-reader/ - Server-side pdf-parse text extraction → /api/search endpoint - Browser-native PDF rendering via <embed> - No Preact/React — pure DOM DocumentFragment routes
2 participants
Changes
Implements the v0.41.0-alpha.5 train for SPA mode, client-side routing, Desktop Reader dogfood, SSG/build-plan hardening, Open Props UI token cleanup, third-party Web Component compatibility, CI stabilization, and review follow-up fixes.
Latest head:
fd1becf5Alpha.5 Issue Coverage
createRouter()in@openelement/routeradapter-vitedefineApp({ mode: 'spa' })@openelement/ui@0.41.0-alpha.5is publishedssg.build(plan) -> artifactsentry pointbuild()interfacebuild(plan) -> artifactsin@openelement/ssgbuild()interfaceopen-props-tokens.tsrefactored to Open Props + semantic aliases@openelement/ssgopen-*components use unified tokensopenElement()/openPipeline()pure wiring refactorIssue Closure
Closes #114, #115, #116, #117, #118, #119, #122, #123, #124, #125, #126, #127, #128, #129, #130, #131, #132, #133, #134, #135, #136, #137, #138, #139, #140, #141, #142, #143, #144, #155, #156, #157, #158, #159, #160, #161, #162, #163, #164, #165, #166, #167, #168, #169, #170, #171, #172, #173, #174, #175, #176, #177, #178, #179.
Leaves #120 open as the post-release fresh-example restore after the 0.41.0-alpha.5 UI package is published. #150 is tracked as alpha.6 follow-up work, not an alpha.5 blocker.
Review Follow-Ups Addressed
indexBook()call while preserving the shared Promise gate.repo.tsprototype module; GitHub sync is owned byhost-store.ts./app,/router, and/adapter-vitepackage build tasks todeno check src/so exported side modules are covered./client-entry.js; fallback shells now avoid a 404 and real apps keep their Vite-emittedindex.html.client-entry.jsdoes not come back accidentally.errors.tssection header.Deno.readFile.Deno.writeTextFilewith serialized write queues.tools/visual-smoke.tsanddeno task test:visual-smokefor docs + Reader screenshot smoke coverage.wwwAPI reference against the alpha.5 package graph and current package exports.<open/>the canonical docs logo primitive and marked legacy SVG logo assets as compatibility-only.Verification
deno task typecheck✅deno task desktop-reader:smoke✅deno task test:visual-smoke✅deno task test:e2e✅ (one homepage performance retry was flaky; isolated rerun passed in 1.8s)deno task test✅ (1172 passed)deno task autoflow:ci✅deno task fmt:check✅deno task lint✅deno check tools/visual-smoke.ts examples/deno-desktop-reader/main.ts examples/deno-desktop-reader/app/search.ts✅deno task deno-api:check✅deno task third-party-wc:smoke✅deno run -A --unsafe-proto tools/visual-smoke.ts✅ screenshots written totest-results/visual-smoke/deno test --allow-read --allow-write --allow-env --allow-net --allow-run packages/adapter-vite/__tests__/build.test.ts examples/deno-desktop-reader/app/__tests__/✅CI=1 deno task autoflow:ci✅Remote Gate Status