feat(alpha.5): SPA mode + client router + desktop proof#121
feat(alpha.5): SPA mode + client router + desktop proof#121SisyphusZheng wants to merge 48 commits into
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: |
80af290
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3393e844.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
|
Now I have a complete picture of all the code. Here is my review: Review: feat(alpha.5) — SPA mode + client router + desktop proofThe 15+ prior bot review rounds have been thorough. All 14+ previously-flagged critical/high issues (hardcoded Below are the remaining issues I found independently: BUG1.
// line 109-110: marked "ready" BEFORE any async work
if (searchIndexReady) return;
searchIndexReady = true;
// line 141: fire-and-forget
ensureSearchIndex();
return json(search(q, CACHE_DIR)); // reads on-disk index before indexing completes
MEDIUM2.
export async function indexBook(...): Promise<void> {
const data = new Uint8Array(await Deno.readFile(pdfPath));
const parsed = await pdfParse(data);
index[bookId] = parsed.text;
// ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks event loop
}For large PDFs, 3.
Fully implemented public function with SHA-based cache logic, but 4. SPA shell references
<script type="module" src="${base}client-entry.js"></script>The adapter writes LOW5. Route manifest JSDoc path mismatch
But 6. ~100 LOC duplicated test mock infrastructure
7.
try {
const books = JSON.parse(raw);
for (const book of books) {
try {
await indexBook(path, book.id, CACHE_DIR);
} catch { /* skip */ } // per-book errors silently swallowed
}
} catch { /* skip */ } // top-level JSON parse errors silently swallowedIf What's good
Verdict: Merge-ready. The 1 bug is in example/dogfood code, not the framework. The router and SPA bootstrap are production-quality for alpha.5. |
Review: feat(alpha.5) — SPA mode + client router + desktop proofThe 15+ prior bot review rounds and follow-up commits resolved all critical/high issues. The router core, SPA glue, and route manifest generator are production-quality. Below are the remaining issues in the current code ( BUG1. searchIndexReady = true; // line 110 — set BEFORE any async indexing
ensureSearchIndex(); // line 141 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // reads on-disk index immediately
2. Sync I/O blocks event loop in export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks server
}
MEDIUM3. Fully implemented with sha-based cache + GitHub sync logic but 4. The adapter writes LOW5. ~100 LOC duplicated test mocks — Both define near-identical 6. JSDoc path mismatch — Shows What's good
Verdict: merge-ready. The 2 bugs are in example/dogfood code, not the framework core. The router and SPA bootstrap are production-quality for alpha.5. |
Root cause: @openelement/core JSX returns VNodes (virtual DOM), but renderComponent checked isRenderableNode (needs nodeType). All route output was silently discarded — pure white page. Fix: import renderToDom from @openelement/core/csr and convert VNode→DOM before appending to root element.
| let renderId = 0; | ||
|
|
||
| /** Duck-type check since `Node` may not exist in test environments (e.g. Deno). */ | ||
| function isRenderableNode(value: unknown): value is Node { |
Review: feat(alpha.5) — SPA mode + client router + desktop proofReviewed all 26 changed files independently. The 15+ prior bot review rounds resolved 14+ critical/high/bug issues — all previously-flagged items (hardcoded BUG1. searchIndexReady = true; // line 110 — set BEFORE async indexing
ensureSearchIndex(); // line 141 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // runs before indexing completes
MEDIUM2. The adapter writes 3. Sync I/O blocks event loop inside async export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks server
}
LOW4. Complete public function with sha-based cache + GitHub sync logic, but 5. ~100 LOC duplicated test mocks — Both define near-identical 6. Dead example route — What's good
Verdict: merge-ready. The 1 bug is in example/dogfood code, not the framework core. The router and SPA bootstrap are production-quality for alpha.5. |
| let renderId = 0; | ||
|
|
||
| /** Duck-type check since `Node` may not exist in test environments (e.g. Deno). */ | ||
| function isRenderableNode(value: unknown): value is Node { |
…ird-party CE Each route = OpenElement subclass with render(): - BookshelfPage, ReadingPage, NotesPage, SearchPage, SettingsPage, WcInteropPage - customElements.define() in each module - @openelement/ui: open-button, open-card, open-input throughout - Third-party CE: sl-button, sl-card, mwc-checkbox, mwc-button
|
Now I have a thorough understanding of all the key changed files. Here's my independent review: Review: feat(alpha.5) — SPA mode + client router + desktop proofThe prior bot review rounds and follow-up commits resolved 14+ issues. The core router ( BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Defined but never called anywhere in the file (confirmed by grep). The render path at lines 92-96 now uses 2. // line 109-110: marked "ready" BEFORE any async indexing
if (searchIndexReady) return;
searchIndexReady = true;
// ... lines 113-122: await indexBook() — actual indexing ...
// line 141: fire-and-forget, NOT awaited
ensureSearchIndex();
return json(search(q, CACHE_DIR)); // reads on-disk index immediately
MEDIUM3. export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks event loop
}For large PDFs, 4. <script type="module" src="${base}client-entry.js"></script>No build step produces this file (the adapter bypasses Vite's HTML pipeline). The desktop reader works around it via runtime injection at LOW5. ~100 LOC duplicated test mocks — Both define near-identical 6. try {
await indexBook(path, book.id, CACHE_DIR);
} catch { /* skip */ } // per-book errors silently lost
// ...
} catch { /* skip */ } // top-level JSON parse errors silently lostIf 7. Dead example route — Acknowledged placeholder (comments at lines 3-5 and What's good
Verdict: merge-ready after removing the dead |
Review: feat(alpha.5) — SPA mode + client router + desktop proofThe prior bot rounds resolved 14+ critical/high issues. The router core, SPA bootstrap, and route manifest generator are production-quality. Below are remaining issues in the current code ( BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Never called anywhere. The render path uses 2. searchIndexReady = true; // line 110 — set BEFORE async indexing
// ... async indexBook() ...
ensureSearchIndex(); // line 141 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // reads on-disk index immediately
MEDIUM3. export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks event loop
}Use 4. 5. <script type="module" src="${base}client-entry.js"></script>No build step produces this file. The desktop reader works around it via runtime injection ( LOW6. 7. 8. Route manifest JSDoc path mismatch — 9. ~100 LOC duplicated test mocks — What's good
Verdict: merge-ready. The 2 bugs are small fixes (remove dead function + fix race in example code). The router and SPA bootstrap are production-quality for alpha.5. |
spa.ts renderComponent: detect route.tagName → createElement + set loader data
reader.tsx: restore defineApp({ mode: 'spa' }) (was bypassed by hand-rolled bootstrap)
All 6 routes = OpenElement subclasses with proper shadow DOM
@openelement/ui: open-button/open-card/open-input throughout
Third-party CE: Shoelace + MWC via CDN
Independent Review: feat(alpha.5) — SPA mode + client router + desktop proofI read all 6 core changed files independently. The 15+ prior bot reviews and follow-up commits resolved all previously-flagged critical/high issues — hardcoded BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Defined but never called anywhere in the file (or codebase). The render path uses 2. searchIndexReady = true; // line 110 — set BEFORE any async indexing
// ... await indexBook(...) ... // lines 115-120 — actual indexing
// line 141: fire-and-forget, NOT awaited
ensureSearchIndex();
return json(search(q, CACHE_DIR)); // reads on-disk index before indexing completes
MEDIUM3. export async function indexBook(...): Promise<void> {
const data = new Uint8Array(await Deno.readFile(pdfPath));
const parsed = await pdfParse(data);
index[bookId] = parsed.text;
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks event loop
}For large PDFs, 4. Complete public function with sha-based cache + GitHub sync logic, but 5. <script type="module" src="${base}client-entry.js"></script>The adapter writes LOW6. ~100 LOC duplicated test mock infrastructure — Both define near-identical 7. try { await indexBook(...) } catch { /* skip */ } // per-book errors lost
} catch { /* skip */ } // JSON parse errors lostIf 8. Called on every SPA page load. Cache the result since the filename won't change between requests. 9. Dead example route — Acknowledged placeholder (comments at lines 3-5). Acceptable for alpha.5 proof. What's good
Verdict: merge-ready. The 2 bugs are in dead code (remove 4 lines) and example/dogfood code (race condition in reader), not the framework core. The router and SPA bootstrap are production-quality for alpha.5. |
|
I've independently reviewed all key changed files. Here is my consolidated review: Review: feat(alpha.5) — SPA mode + client router + desktop proofThe 15+ prior bot review rounds and follow-up commits resolved all previously-flagged critical/high issues (hardcoded BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Defined but has zero call sites anywhere in the codebase (confirmed by grep). The render path at lines 88-112 uses 2. if (searchIndexReady) return;
searchIndexReady = true; // line 110 — set BEFORE async indexing
// ... await indexBook() ... // lines 113-122 — actual indexing happens asyncThen called fire-and-forget at line 141: 3. Orphaned section header — // ─── SSR Error Context ──────────────────────────────────────────────Last line of the file with no implementation following. Either a placeholder that was never written or a leftover from removed code. Remove or add the implementation. MEDIUM4. export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks event loop
}Use 5. try { await indexBook(...) } catch { /* skip */ } // per-book
} catch { /* skip */ } // top-level JSON parseIf 6. <script type="module" src="${base}client-entry.js"></script>The adapter writes LOW7. Route manifest JSDoc path mismatch — 8. What's good
Verdict: merge-ready. The 3 bugs are small fixes (delete dead function, reorder 2 lines + await in search endpoint, delete orphaned comment). The router and SPA bootstrap code are production-quality for alpha.5. |
Review: feat(alpha.5) — SPA mode + client router + desktop proofReviewed all key changed files independently. The 16+ prior bot review rounds and follow-up commits resolved 14+ critical/high issues — every previously-flagged item (hardcoded BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Zero call sites anywhere in the codebase (grep confirms). The render path uses 2. searchIndexReady = true; // line 122 — set BEFORE async indexing
// ... await indexBook() ... // lines 129-133 — actual indexing
ensureSearchIndex(); // line 153 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // line 155 — reads on-disk index immediately
3. Orphaned section header — // ─── SSR Error Context ──────────────────────────────────────────────Last line of the file with no implementation following. Either a placeholder never written or a leftover from removed code. Remove or add the implementation. MEDIUM4. export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks event loop
}
5. <script type="module" src="${base}client-entry.js"></script>The adapter writes LOW6. try { await indexBook(...) } catch { /* skip */ }
} catch { /* skip */ }Malformed 7. Called on every SPA page load. Cache the result — the filename won't change between requests. 8. ~100 LOC duplicated test mocks — Both define near-identical What's good
Verdict: merge-ready after removing the dead |
Review: feat(alpha.5) — SPA mode + client router + desktop proofThe 15+ prior bot review rounds and follow-up commits resolved 14+ previously-flagged critical/high/bug issues. The router core, SPA glue, and route manifest generator are production-quality for alpha.5. Below are remaining unfixed findings. BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Zero call sites across the entire codebase (grep confirms). The render path at lines 88-112 uses 2. if (searchIndexReady) return;
searchIndexReady = true; // line 122 — set BEFORE any async indexing work
// ... await indexBook() ... // line 131 — actual indexing
ensureSearchIndex(); // line 153 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // line 155 — reads on-disk index immediately
MEDIUM3. export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks event loop
}For large PDFs, 4. <script type="module" src="${base}client-entry.js"></script>The adapter writes 5. Complete public function with sha-based cache + GitHub sync logic, but LOW6. Orphaned section header — // ─── SSR Error Context ──────────────────────────────────────────────Last line with no implementation following. Either missing code or dead comment. Remove or add implementation. 7.
8. } catch { /* skip */ }Malformed 9. ~100 LOC duplicated test mocks — Both define near-identical 10. Route manifest JSDoc path mismatch — JSDoc shows What's good
Verdict: merge-ready after removing dead |
|
I've read all the key changed files. Here's my independent review: Review: feat(alpha.5) — SPA mode + client router + desktop proofThe 20+ prior bot review rounds and follow-up commits resolved 14+ previously-flagged issues. The router core, SPA glue, and route manifest generator are production-quality. Below are remaining issues in the current code. BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Zero call sites anywhere in the codebase. The render path uses 2. Orphaned section header — // ─── SSR Error Context ──────────────────────────────────────────────Last line of the file with no implementation following. Either a placeholder never written or leftover from removed code. Remove or add the implementation. 3. searchIndexReady = true; // line 122 — set BEFORE any async indexing
// ... await indexBook() ... // lines 127-133 — actual indexing happens async
ensureSearchIndex(); // line 153 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // line 155 — reads on-disk index immediately
4. Sync I/O blocks event loop in async export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync — blocks server event loop
}For large PDFs this blocks the server. Use a non-blocking write: replace the body of MEDIUM5. <script type="module" src="${base}client-entry.js"></script>The adapter writes 6. Complete public function with sha-based cache + GitHub sync, but 7. try { await indexBook(...) } catch { /* skip */ }
} catch { /* skip */ }If 8. Called on every SPA page load. Cache the result since the filename won't change between requests. LOW9. ~100 LOC duplicated test mocks — Both define near-identical 10. Route manifest JSDoc path mismatch — JSDoc shows What's good
Verdict: Fix the 4 bugs (delete dead function + orphaned comment, fix the search race + sync I/O). The router and SPA bootstrap are production-quality for alpha.5. |
|
Based on my independent review of all core changed files against the 20+ prior bot review rounds, here is my consolidated review: Review: feat(alpha.5) — SPA mode + client router + desktop proofThe follow-up commits across 20+ review rounds resolved all 14+ previously-flagged critical/high issues: hardcoded BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Zero call sites across the entire codebase. The render paths at lines 96-103 (OpenElement 2. Orphaned section header — // ─── SSR Error Context ──────────────────────────────────────────────Last line of the file with no implementation following. Either a placeholder never written or leftover from removed code. 3. searchIndexReady = true; // line 122 — set BEFORE async indexing
// ... await indexBook() ... // lines 129-131 — actual async indexing
ensureSearchIndex(); // line 153 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // line 155 — reads empty on-disk index
4. Sync I/O blocks event loop inside async export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync (line 98)
}
MEDIUM5. <script type="module" src="${base}client-entry.js"></script>The adapter writes 6. Complete public function with sha-based cache + GitHub sync logic, but LOW7. try { await indexBook(...) } catch { /* skip */ }
} catch { /* skip */ }Malformed 8.
9. ~100 LOC duplicated test mocks — Both define near-identical 10. Route manifest JSDoc path mismatch — JSDoc shows What's good
Verdict: merge-ready after removing dead |
Review: feat(alpha.5) — SPA mode + client router + desktop proofThe 20+ prior bot reviews and follow-up commits resolved 14+ critical/high issues. The router core and SPA bootstrap are production-quality. Below are the remaining issues in the current code. BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Zero call sites in the entire codebase (grep confirms). The render path uses 2. if (searchIndexReady) return;
searchIndexReady = true; // line 122 — set BEFORE any async indexing
// ... await indexBook() ... // lines 127-133 — actual async indexing
ensureSearchIndex(); // line 153 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // line 155 — reads on-disk index immediately
3. Orphaned section header — // ─── SSR Error Context ──────────────────────────────────────────────Last line of the file with no implementation following. Either missing code or dead comment. Remove or add implementation. MEDIUM4. Sync I/O blocks event loop inside async export async function indexBook(...): Promise<void> {
// ... async pdfParse ...
saveSearchIndex(dir, index); // → Deno.writeTextFileSync (line 98) — blocks server
}
5. Complete public function with SHA-based cache + GitHub sync logic, never imported or called. Either wire in 6. <script type="module" src="${base}client-entry.js"></script>The adapter writes LOW7. try { await indexBook(...) } catch { /* skip */ }
} catch { /* skip */ }Malformed 8.
9. ~80 LOC duplicated test mocks — Both define near-identical 10. Route manifest JSDoc path mismatch — JSDoc shows What's good
Verdict: merge-ready after removing dead |
Independent ReviewReviewed all core changed files. The 20+ prior bot rounds resolved 14+ previously-flagged issues. Below are the remaining findings in the current code ( BUG1. Zero call sites across the entire codebase (grep confirms). The render path uses 2. searchIndexReady = true; // line 126 — set BEFORE async indexing
// ... await indexBook() ... // line 135 — actual indexing (async)
ensureSearchIndex(); // line 157 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // line 159 — reads on-disk index immediately
3. Orphaned section header — // ─── SSR Error Context ──────────────────────────────────────────────Last line of the file with no implementation. Either missing code or dead leftover. 4. Sync I/O blocks event loop inside async saveSearchIndex(dir, index); // → Deno.writeTextFileSync (line 98)
MEDIUM5. <script type="module" src="${base}client-entry.js"></script>The adapter writes 6. Complete public function with SHA-based cache + GitHub sync, never called from 7. Malformed 8. Uncached sync I/O runs on every SPA page load. Cache the result. LOW9. Route manifest JSDoc path mismatch — JSDoc shows 10. ~100 LOC duplicated test mocks — Both define near-identical What's good
Verdict: Fix the 4 bugs (delete dead function + orphaned header, fix search race + sync I/O). All are targeted local fixes. The router and SPA bootstrap are production-quality for alpha.5. |
Consolidated ReviewThe 20+ prior bot rounds resolved ~14 critical/high issues (hardcoded BUG1. function isRenderableNode(value: unknown): value is Node {
return value !== null && typeof value === 'object' && 'nodeType' in value;
}Zero call sites. The render paths at lines 96-103 (OpenElement 2. searchIndexReady = true; // line 126 — set BEFORE await indexBook()
// ... await indexBook() ... // line 135 — actual async indexing
ensureSearchIndex(); // line 157 — fire-and-forget, NOT awaited
return json(search(q, CACHE_DIR)); // line 159 — reads on-disk index immediately
3. Sync I/O blocks event loop inside async saveSearchIndex(dir, index); // → Deno.writeTextFileSync (line 98)
4. Orphaned section header — // ─── SSR Error Context ──────────────────────────────────────────────Last line of the file with no implementation. Either missing code or dead leftover. MEDIUM5. <script type="module" src="${base}client-entry.js"></script>The adapter writes 6. LOW7. 8. 9. Route manifest JSDoc path mismatch — 10. ~100 LOC duplicated test mocks — What's good
Verdict: Fix the 4 bugs. The |
2 participants
Changes
Implements #114 alpha.5 scope: SPA mode, client-side router, desktop proof, and Desktop Reader dogfood app.
Part 1: SPA Core (#115-#119)
93/93 SPA tests pass (50 new + 43 existing).
Part 2: Desktop Reader (#122-#127)
Reader architecture:
Verification
deno fmt✅deno lint✅deno check✅deno test packages/router/__tests__/ packages/app/__tests__/✅ 93/93deno task desktop-reader:smoke✅ 27/27Bot Review
Awaiting opencode-review.