feat(desktop): the Electron shell, the design system, and browsing the wiki (8.1, 8.2, 8.5, 8.10) - #13
Conversation
…e wiki Plan 8.1, 8.2, 8.5 and 8.10. The entry point is wiring and nothing else. Which project this window is, what the renderer may ask for, how a page becomes HTML, where Back goes, what a folder change means — each is a module beside it that a test calls without starting a window, because CI has no display. That is the same seam the recorder put under `CaptureSource` and the pipeline put under `FfmpegRunner`, applied to the one part of this product that cannot be run in CI at all. Three things about the window are deliberate rather than default. `contextIsolation` is on, `nodeIntegration` off, `sandbox` on, and the page carries a CSP of `default-src 'none'`: this window renders markdown an agent wrote, and a renderer with Node in it is one prompt injection away from being the agent's hands. The project root is bound in the main process and never crosses the bridge, so the renderer names a slug and never a path. And a page is resolved by slug *through the index* rather than by joining a slug onto a path — `adr:0016` makes the index the only thing that knows where a page sits, which means the correct implementation is also the one that does no path arithmetic on untrusted input. A launch that names no project says so and quits. Falling back to the working directory would open somebody's home folder as a wiki; 8.4's launcher is what belongs there, and saying so beats guessing. The design system is tokens in one file. Dense and dark because of what the window is for — a wiki read beside the harness, glanced at while a meeting records — and neither of those wants generous whitespace. Focus is visible on everything, always. The recording indicator is persistent and not subtle: somebody who forgets it is running ends up with a recording of a conversation the other people in it believe ended. Browsing renders with `html: false`, turns a wikilink into an href the app intercepts rather than a navigation, and shows a *broken* wikilink as broken where the reader would have clicked it — a link to nowhere that looks like a link is worse than obviously missing text, and it is the same thing 7.1 reports. Following a link after going back discards the forward history, exactly as a browser does; anything else and Back stops meaning "where I came from". The watcher covers `wiki/` and `raw/` and not `.state/`, which is not content. `awaitWriteFinish` is not tuning: `fs.watch` reports a file the moment it appears, which on a copy is halfway through being written, and a page read halfway through has no frontmatter. CI skips Electron's ~100 MB runtime, because nothing in CI starts a window. The installer build of 10.1 is where it is actually needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
**Opening the window opened the microphone.** `useRecording` polls `recordStatus` on mount, that went through the lazy `session ??=`, and `recorder.exe` opens both WASAPI devices the moment it launches — before it reads a single request. So launching the application put the microphone into Windows' in-use state and held it for the window's lifetime, while the chrome said nothing was being recorded. That is precisely the failure the persistent indicator exists to prevent, inverted. `ensure` and `peek` are now different methods; only `record:start` reaches `ensure`, and a poll with no session answers idle. **`record:start` took a filesystem path from the renderer.** Every other handler binds the project root rather than accepting one, and this was the exception — a compromised renderer directing recorder output anywhere the user can write. It now takes the *occasion*; the id comes from 4.16 and the directory from the id, so a recording lands under `raw/` because there is nowhere else it can go. **The renderer could navigate away with the preload attached.** The click handler in the page is renderer-side JavaScript, which is a convenience, not a boundary — a drag-and-drop or any renderer bug navigates without one, and a preload re-runs on every navigation, so a window that reached a remote origin would hand that origin `window.ow`. The main process now refuses navigation. And `shell.openExternal` was handed any URL: on Windows that is `ShellExecute`, which invokes whichever protocol handler is registered, and `ms-msdt:`, `ms-officecmd:` and `search-ms:` against a WebDAV share are documented paths from a link in a document to code execution. The scheme is allowlisted, and the page handles `auxclick` too — a middle click is what asks Electron to open a window, and it never fired `onClick`. **Both markdown extensions are markdown-it rules now, not replacements over its output.** A `String.replace` on serialised HTML does not know what an attribute is: a citation inside a link title landed in `title="…"`, its own quote ended that attribute, and the rest became attribute names on somebody else's tag. It also rewrote the inside of code spans, so every page quoting `[[target]]` or `rec://…` — several in this repository — rendered its own examples as live links, and text markdown-it had already escaped was escaped twice. Links are routed by `data-ow-page` rather than an `href` scheme, because markdown-it renders `[x](page:evil)` quite happily and a scheme proves nothing about who minted it. The recorder client grew the failure paths it was missing: `child` and `child.stdin` have error listeners, so a spawn failure or an `EPIPE` after a crash no longer takes down the Electron main process; `stderr` is drained and bounded, so a panicked sidecar neither hangs on a full pipe nor loses its message; a send that throws removes its queue entry rather than leaving it to be resolved by the next response; `dispose` marks the session closed; a dead session is replaced rather than reused forever; and the unsolicited line `recorder.exe` writes before exiting on a device failure is kept and reported, because it is the whole explanation. 8.2 asks for record, pause and stop, and there was no way to do any of them — the IPC existed and nothing called it. There is a control in the chrome now. Three tests were replaced for asserting the implementation rather than the requirement: the markdown ones fed hand-written HTML fragments to a helper and never ran a page through the pipeline, and the "ignores an unsolicited line" test asserted only that nothing threw, in the one arrangement where the behaviour it named happened to hold. The rule 8.10 actually states — is this change the open page? — moved out of the untestable component into `watcher.ts`. Also: Electron 33 is outside its support window and its Chromium no longer gets security fixes, which is the wrong floor for an application whose threat model is untrusted content in a renderer; bumped to 38. The CSP names `base-uri` and `form-action`, which do not inherit from `default-src`. And `docs/stack.md` records why markdown-it is here, which this repo's own rule required and the diff had skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds a complete Electron desktop wiki application. It includes project resolution, Markdown page access, recorder sidecar integration, live file watching, secure IPC, renderer navigation, recording controls, styling, build configuration, and comprehensive tests. ChangesDesktop wiki application
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant MainProcess
participant ProjectAPI
participant Watcher
Renderer->>MainProcess: request project and wiki data
MainProcess->>ProjectAPI: read project metadata and page index
ProjectAPI-->>MainProcess: return wiki data
MainProcess-->>Renderer: return page content
Watcher-->>MainProcess: emit normalized project change
MainProcess-->>Renderer: forward change notification
Renderer->>ProjectAPI: refresh index and open page
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
The only flaky test in the repository. It is the one test in that file that loads pdfjs, and cold on a CI runner the dynamic import plus the first parse run past vitest's 5 s default — so it fails on how fast the machine is rather than on anything about the code, which is the worst kind of red because it teaches everyone to re-run instead of read. The new timeout is deliberately generous rather than tuned to what it takes today: a number chosen to *just* pass is the same flake again on a slower day. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
apps/desktop/src/main/watcher.ts (1)
99-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging swallowed watcher errors.
watcher.on("error", () => {})discards every error with no trace. The comment explains why the UI should not surface it, but a silent catch also removes any way for a developer or support engineer to tell that the live-reload feature has stopped working (e.g., on a network share raisingEPERMrepeatedly). Add a minimal diagnostic log (e.g.,console.erroror a debug channel) without changing the user-facing behavior.🔍 Proposed diagnostic logging
- watcher.on("error", () => {}); + watcher.on("error", (err) => { + console.error("[watcher] file watch error:", err); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/watcher.ts` around lines 99 - 102, Update the error handler passed to watcher.on in the watcher setup to emit a minimal diagnostic log containing the watcher error, while continuing to suppress the error from reaching the window and preserving the existing user-facing behavior.apps/desktop/src/renderer/markdown.ts (1)
135-168: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
provenanceRulecan nest an<a>inside another<a>.The rule rewrites every
texttoken inblock.childrenthat matches the provenance pattern, regardless of whether that token sits inside an already-openlink_open/wikilink_openpair (both render as<a>via the default token renderer, sinceblock.childrenis a flat token list, not a nested tree). If a citation string likesrc://doc#p1appears inside a real link's label or a wikilink label — plausible in a wiki whose whole purpose is provenance tracking — the result is a nested<a>inside another<a>: invalid markup, and ambiguous forelement.closest("a, span[title]")in the click handler.Track anchor depth while iterating
block.childrenand skip citation-splitting fortexttokens betweenlink_open/link_closeorwikilink_open/wikilink_close.♻️ Proposed depth-tracking guard
function provenanceRule(state: StateCore): void { for (const block of state.tokens) { if (block.type !== "inline" || !block.children) continue; const rebuilt: Token[] = []; + let anchorDepth = 0; for (const child of block.children) { + if (child.type === "link_open" || child.type === "wikilink_open") anchorDepth++; + if (child.type === "link_close" || child.type === "wikilink_close") anchorDepth--; - if (child.type !== "text") { + if (child.type !== "text" || anchorDepth > 0) { rebuilt.push(child); continue; } rebuilt.push(...splitProvenance(state, child)); } block.children = rebuilt; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/markdown.ts` around lines 135 - 168, Update provenanceRule to track anchor depth while iterating each block’s flat children, incrementing for link_open and wikilink_open and decrementing for their corresponding close tokens. Only call splitProvenance for text tokens when the tracked depth is zero; preserve all existing tokens unchanged while inside either link type.apps/desktop/src/renderer/tokens.css (1)
93-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFocus disappears in forced-colors mode.
The rule sets
outline: noneand draws the ring withbox-shadow. Windows high contrast mode (forced-colors: active) does not paintbox-shadow. The focus indicator is then absent, which is the exact failure the comment above this rule warns against.Restore a real outline under
forced-colors: active.♿ Proposed addition
:focus-visible { outline: none; box-shadow: var(--focus-ring); border-radius: var(--radius-sm); } + +/* + * Windows high contrast does not paint `box-shadow`, so the ring above is not + * there. An outline is, and it takes the system's own colour. + */ +@media (forced-colors: active) { + :focus-visible { + outline: 2px solid Highlight; + outline-offset: 2px; + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/tokens.css` around lines 93 - 97, Update the :focus-visible rule to restore a visible outline when forced-colors mode is active, overriding the outline removal with a system-compatible outline while preserving the existing box-shadow styling for normal color modes.apps/desktop/src/renderer/recording.ts (2)
58-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider separating the announced label from the ticking elapsed time.
describeRecordingreturnslabelandelapsedas one text pair. The consumer atapps/desktop/src/renderer/RecordingIndicator.tsxrenders both inside one node withrole="status"andaria-live="polite".elapsedchanges once per second becauseuseRecordingpolls atPOLL_MS. A screen reader therefore announces the timer every second, which hides other output.Consider marking the elapsed value
aria-hidden="true"in the component, and keeping only the state label inside the live region.describeRecordingalready returns the two values separately, so no change to this file is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/recording.ts` around lines 58 - 68, Update the RecordingIndicator component to keep only the recording state label in the role="status" aria-live="polite" live region and mark the separately rendered elapsed value aria-hidden="true". Leave describeRecording unchanged, preserving its existing label and elapsed values.
73-96: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid a re-render on every poll, and prevent overlapping ticks.
Two issues in this effect:
setRecording(readStatus(status))always passes a new object. React compares by reference, so the whole subtree that consumesuseRecordingre-renders once per second, including while nothing is recorded.apps/desktop/src/renderer/App.tsxcallsuseRecordingat the top level, so this is the whole application view.setIntervaldoes not wait for the previoustickto settle. If onerecordStatuscall is slow, ticks overlap. Each overlapping call appends an entry to thequeueinRecorderSession.call, which has no timeout.Return the previous state when nothing changed, and chain the next poll after the current one finishes.
♻️ Proposed change
export function useRecording(pollMs = POLL_MS): Recording { const [recording, setRecording] = useState<Recording>(IDLE); useEffect(() => { if (!hasBridge()) return; let live = true; + let timer: ReturnType<typeof setTimeout> | undefined; + // A new object every second re-renders the whole view for no change. + const settle = (next: Recording): void => { + setRecording((previous) => + previous.state === next.state && previous.recordedMs === next.recordedMs ? previous : next, + ); + }; const tick = async (): Promise<void> => { try { const status = await bridge().recordStatus(); - if (live) setRecording(readStatus(status)); + if (live) settle(readStatus(status)); } catch { // No sidecar running is the ordinary case, not an error worth a // banner: it means nothing is being recorded. - if (live) setRecording(IDLE); + if (live) settle(IDLE); + } finally { + // Chained, not an interval: a slow answer must not stack another poll. + if (live) timer = setTimeout(() => void tick(), pollMs); } }; void tick(); - const timer = setInterval(() => void tick(), pollMs); return () => { live = false; - clearInterval(timer); + if (timer !== undefined) clearTimeout(timer); }; }, [pollMs]); return recording; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/recording.ts` around lines 73 - 96, Update useRecording so its state setter returns the previous recording object when the newly read status is unchanged, avoiding re-renders during idle or unchanged polling. Replace the setInterval-based scheduling with a self-chaining tick that schedules the next poll only after the current recordStatus call settles, while preserving cleanup and the live guard.apps/desktop/tests/wiring.spec.ts (1)
120-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
.stateexclusion test able to fail.The test writes
.state/log.jsonfirst andwiki/a.mdsecond, then waits forseen.length > 0and asserts that every seen path starts withwiki/. If.statewere watched, its event could still arrive after the assertion ran, so the test would pass. The assertion resolves on the first event, whichever file produced it.Wait for the
wiki/a.mdevent specifically, then give the watcher a settling window and assert that no.statepath was reported.💚 Proposed change
writeFileSync(join(root, ".state", "log.json"), "[]", "utf8"); writeFileSync(join(root, "wiki", "a.md"), "x", "utf8"); - await waitFor(() => seen.length > 0); - expect(seen.every((p) => p.startsWith("wiki/"))).toBe(true); + // The wiki write is the marker: once it is reported, the watcher has + // processed both writes, so a `.state` event would already be here. + await waitFor(() => seen.includes("wiki/a.md")); + await new Promise((r) => setTimeout(r, 150)); + expect(seen.filter((p) => p.startsWith(".state"))).toEqual([]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/tests/wiring.spec.ts` around lines 120 - 135, Update the test “does not watch .state, which is not content” to wait specifically until the wiki/a.md change is observed rather than merely waiting for any event. After that event, allow a settling window for delayed watcher notifications, then assert that no reported path belongs to .state while preserving the existing cleanup.apps/desktop/src/main/recorder.ts (1)
178-198: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout or a queue bound to
call.
callnever settles if the sidecar reads a request and returns no line. The transport only rejects pending entries when the child closes. If the sidecar stalls while still alive, every promise stays pending.The renderer polls
record:statusevery second (apps/desktop/src/renderer/recording.ts,POLL_MS = 1000). Each poll appends one entry toqueue. A stalled sidecar therefore growsqueuewithout bound and leaves one unresolved promise per second.Consider a per-request deadline that rejects the head entry and removes it from
queue, or a maximum queue depth that refuses new calls.♻️ Sketch of a per-request deadline
export class RecorderSession { private readonly queue: Pending[] = []; private closed = false; + /** A request the sidecar never answers must not leave a button spinning. */ + private static readonly TIMEOUT_MS = 10_000; ... call( method: RecorderMethod, params: Record<string, unknown> = {}, ): Promise<Record<string, unknown>> { if (this.closed) return Promise.reject(new RecorderError(this.reasonItDied())); return new Promise((resolvePromise, rejectPromise) => { - const pending: Pending = { resolve: resolvePromise, reject: rejectPromise }; + const drop = (): void => { + const at = this.queue.indexOf(pending); + if (at >= 0) this.queue.splice(at, 1); + }; + const timer = setTimeout(() => { + drop(); + rejectPromise(new RecorderError(`the recorder did not answer "${method}"`)); + }, RecorderSession.TIMEOUT_MS); + const pending: Pending = { + resolve: (payload) => { + clearTimeout(timer); + resolvePromise(payload); + }, + reject: (error) => { + clearTimeout(timer); + rejectPromise(error); + }, + }; this.queue.push(pending); try { this.transport.send(JSON.stringify({ method, ...params })); } catch (e) { - const at = this.queue.indexOf(pending); - if (at >= 0) this.queue.splice(at, 1); + clearTimeout(timer); + drop(); rejectPromise(e instanceof Error ? e : new RecorderError(String(e))); } }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/recorder.ts` around lines 178 - 198, Update Recorder.call to prevent indefinitely pending requests when the sidecar remains alive without responding: add a per-request timeout that rejects the timed-out request with a RecorderError and removes that same Pending entry from queue, while preserving normal response handling and transport.send failure cleanup. Ensure the timer is cleared when the request settles to avoid affecting completed calls.apps/desktop/src/main/index.ts (1)
63-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove channel registration out of
createWindow, or key handlers by sender.
ipcMain.handleis process-global.createWindowregisters every channel, and theclosedlistener removes every channel. The application creates one window today, so this works. Two problems appear as soon as a second window exists:
ipcMain.handlethrows when a channel already has a handler. The secondcreateWindowcall fails.- Closing one window calls
ipcMain.removeHandlerfor all channels, which silently disables IPC for the window that is still open.Register the channels once at startup and resolve the per-window
apifromevent.sender, or useipcMain.handlewith a registry keyed bywebContents.id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/index.ts` around lines 63 - 78, Move the ipcMain.handle registration out of createWindow into one-time startup initialization, or maintain handlers keyed by event.sender.webContents.id so multiple windows cannot conflict. Preserve per-window API dispatch by resolving the correct api from the sender, and remove the closed listener’s loop that calls ipcMain.removeHandler for every channel; only clean up the closing window’s associated resources and registry entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/project.ts`:
- Around line 47-59: Update resolveProject’s --project branch to validate the
resolved target with looksLikeProject(dir) instead of isDirectory(dir), matching
the cwd fallback and returning null for existing directories without the
expected project structure.
In `@apps/desktop/src/renderer/index.html`:
- Around line 15-18: Remove the unsupported frame-ancestors directive from the
Content-Security-Policy meta tag in the renderer HTML. Preserve the remaining
CSP directives unchanged; do not add alternative framing protection unless an
existing supported header or protocol-based mechanism is already part of the
implementation.
In `@apps/desktop/src/renderer/navigation.ts`:
- Around line 109-127: Update onPageClick to call shell.openExternal for
openable external URLs before navigation is refused, using isOpenableExternally
to validate the href. Preserve internal-link navigation and ensure
refuseNavigation does not leave approved external left-click links unopened;
keep setWindowOpenHandler behavior unchanged.
In `@apps/desktop/src/renderer/tokens.css`:
- Around line 22-26: Update the --ink-3 token to a color that achieves at least
4.5:1 contrast against --surface-0, preserving the existing four-step contrast
claim and its use by .frontmatter dt.
---
Nitpick comments:
In `@apps/desktop/src/main/index.ts`:
- Around line 63-78: Move the ipcMain.handle registration out of createWindow
into one-time startup initialization, or maintain handlers keyed by
event.sender.webContents.id so multiple windows cannot conflict. Preserve
per-window API dispatch by resolving the correct api from the sender, and remove
the closed listener’s loop that calls ipcMain.removeHandler for every channel;
only clean up the closing window’s associated resources and registry entry.
In `@apps/desktop/src/main/recorder.ts`:
- Around line 178-198: Update Recorder.call to prevent indefinitely pending
requests when the sidecar remains alive without responding: add a per-request
timeout that rejects the timed-out request with a RecorderError and removes that
same Pending entry from queue, while preserving normal response handling and
transport.send failure cleanup. Ensure the timer is cleared when the request
settles to avoid affecting completed calls.
In `@apps/desktop/src/main/watcher.ts`:
- Around line 99-102: Update the error handler passed to watcher.on in the
watcher setup to emit a minimal diagnostic log containing the watcher error,
while continuing to suppress the error from reaching the window and preserving
the existing user-facing behavior.
In `@apps/desktop/src/renderer/markdown.ts`:
- Around line 135-168: Update provenanceRule to track anchor depth while
iterating each block’s flat children, incrementing for link_open and
wikilink_open and decrementing for their corresponding close tokens. Only call
splitProvenance for text tokens when the tracked depth is zero; preserve all
existing tokens unchanged while inside either link type.
In `@apps/desktop/src/renderer/recording.ts`:
- Around line 58-68: Update the RecordingIndicator component to keep only the
recording state label in the role="status" aria-live="polite" live region and
mark the separately rendered elapsed value aria-hidden="true". Leave
describeRecording unchanged, preserving its existing label and elapsed values.
- Around line 73-96: Update useRecording so its state setter returns the
previous recording object when the newly read status is unchanged, avoiding
re-renders during idle or unchanged polling. Replace the setInterval-based
scheduling with a self-chaining tick that schedules the next poll only after the
current recordStatus call settles, while preserving cleanup and the live guard.
In `@apps/desktop/src/renderer/tokens.css`:
- Around line 93-97: Update the :focus-visible rule to restore a visible outline
when forced-colors mode is active, overriding the outline removal with a
system-compatible outline while preserving the existing box-shadow styling for
normal color modes.
In `@apps/desktop/tests/wiring.spec.ts`:
- Around line 120-135: Update the test “does not watch .state, which is not
content” to wait specifically until the wiki/a.md change is observed rather than
merely waiting for any event. After that event, allow a settling window for
delayed watcher notifications, then assert that no reported path belongs to
.state while preserving the existing cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 92903f61-5094-4689-a6fa-d7fbec9df794
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.github/workflows/ci.ymlapps/desktop/package.jsonapps/desktop/src/main/api.tsapps/desktop/src/main/index.tsapps/desktop/src/main/ipc.tsapps/desktop/src/main/preload.tsapps/desktop/src/main/project.tsapps/desktop/src/main/recorder.tsapps/desktop/src/main/watcher.tsapps/desktop/src/renderer/App.tsxapps/desktop/src/renderer/RecordingIndicator.tsxapps/desktop/src/renderer/bridge.tsapps/desktop/src/renderer/index.htmlapps/desktop/src/renderer/main.tsxapps/desktop/src/renderer/markdown.tsapps/desktop/src/renderer/navigation.tsapps/desktop/src/renderer/recording.tsapps/desktop/src/renderer/tokens.cssapps/desktop/tests/recorder.spec.tsapps/desktop/tests/renderer.spec.tsapps/desktop/tests/shell.spec.tsapps/desktop/tests/wiring.spec.tsapps/desktop/tsconfig.jsonapps/desktop/vite.config.tsapps/desktop/vitest.config.tsdocs/stack.mdpackage.jsonplans/open-wiki.md
| export function resolveProject(args: LaunchArgs): string | null { | ||
| const flag = args.argv.indexOf("--project"); | ||
| if (flag >= 0) { | ||
| const named = args.argv[flag + 1]; | ||
| if (named && !named.startsWith("--")) { | ||
| const dir = isAbsolute(named) ? named : resolve(args.cwd, named); | ||
| return isDirectory(dir) ? dir : null; | ||
| } | ||
| return null; | ||
| } | ||
| const cwd = resolve(args.cwd); | ||
| return looksLikeProject(cwd) ? cwd : null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--project path skips the "looks like a project" check that the cwd fallback enforces.
Line 53 validates the --project target with isDirectory(dir) only. The cwd fallback at line 58 uses looksLikeProject(cwd) instead. This means --project /some/existing/dir succeeds even when dir lacks raw/, wiki/, and .state/, while the same directory reached through cwd would correctly return null.
The file's own documentation states the intent for this function: falling back to an unvalidated directory "would open the user's home folder as a wiki" (see the module doc comment, Lines 13-16). That same risk applies to --project if it is ever invoked with a stale, wrong, or manually-typed path outside the ow shim's control — the renderer would open against a directory with no wiki/raw/state structure instead of being told there is no project.
shell.spec.ts does not cover this case: the --project tests only exercise a directory that already has the three subfolders (Lines 48-56) or a path that does not exist at all (Lines 68-70).
Align the two branches by validating looksLikeProject in both.
🐛 Proposed fix
const flag = args.argv.indexOf("--project");
if (flag >= 0) {
const named = args.argv[flag + 1];
if (named && !named.startsWith("--")) {
const dir = isAbsolute(named) ? named : resolve(args.cwd, named);
- return isDirectory(dir) ? dir : null;
+ return looksLikeProject(dir) ? dir : null;
}
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function resolveProject(args: LaunchArgs): string | null { | |
| const flag = args.argv.indexOf("--project"); | |
| if (flag >= 0) { | |
| const named = args.argv[flag + 1]; | |
| if (named && !named.startsWith("--")) { | |
| const dir = isAbsolute(named) ? named : resolve(args.cwd, named); | |
| return isDirectory(dir) ? dir : null; | |
| } | |
| return null; | |
| } | |
| const cwd = resolve(args.cwd); | |
| return looksLikeProject(cwd) ? cwd : null; | |
| } | |
| export function resolveProject(args: LaunchArgs): string | null { | |
| const flag = args.argv.indexOf("--project"); | |
| if (flag >= 0) { | |
| const named = args.argv[flag + 1]; | |
| if (named && !named.startsWith("--")) { | |
| const dir = isAbsolute(named) ? named : resolve(args.cwd, named); | |
| return looksLikeProject(dir) ? dir : null; | |
| } | |
| return null; | |
| } | |
| const cwd = resolve(args.cwd); | |
| return looksLikeProject(cwd) ? cwd : null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/project.ts` around lines 47 - 59, Update
resolveProject’s --project branch to validate the resolved target with
looksLikeProject(dir) instead of isDirectory(dir), matching the cwd fallback and
returning null for existing directories without the expected project structure.
| <meta | ||
| http-equiv="Content-Security-Policy" | ||
| content="default-src 'none'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; media-src 'self'; font-src 'self'; base-uri 'none'; form-action 'none'; frame-ancestors 'none'" | ||
| /> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n apps/desktop/src/renderer/index.html | sed -n '1,45p'
printf '%s\n' '--- Electron header enforcement references ---'
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' \
"onHeadersReceived|defaultSession|Content-Security-Policy|frame-ancestors" \
apps/desktop package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -200 || trueRepository: protonspy/open-wiki
Length of output: 1647
🌐 Web query:
Content Security Policy frame-ancestors meta element ignored official specification
💡 Result:
The frame-ancestors directive is officially ignored when delivered via a HTML meta element [1][2][3]. According to the Content Security Policy specification, this directive—along with others like sandbox and report-uri—must be delivered via an HTTP response header to be enforced [1][2]. This behavior exists because the frame-ancestors directive determines whether a page can be embedded at the very beginning of the page load process [4]. By the time a browser parses an HTML meta element, the decision to embed the page may have already been made, making it too late for the meta-defined policy to securely prevent framing [4]. To implement frame-ancestors, you must configure your web server, application framework, or CDN to include the Content-Security-Policy HTTP header in the response [5][4][6]. If you attempt to include frame-ancestors in a meta tag, the browser will silently discard that specific directive while continuing to process any other valid directives present in the policy [4][2].
Citations:
- 1: https://www.w3.org/TR/2014/WD-CSP11-20140211/
- 2: https://https-hardening.com/web-security-headers-fundamentals/cross-origin-frame-controls-x-frame-options/
- 3: https://stackoverflow.com/questions/53094845/content-security-policy-frame-ancestors-directive-not-working-in-meta-elemen
- 4: https://inventivehq.com/blog/x-frame-options-vs-csp-frame-ancestors
- 5: https://stackoverflow.com/questions/37799258/content-security-policy-directive-frame-ancestors-self
- 6: https://github.com/dotnet/AspNetCore.Docs/blob/main/aspnetcore/blazor/security/content-security-policy.md
🌐 Web query:
Electron webRequest onHeadersReceived Content-Security-Policy file URL official documentation
💡 Result:
Electron's webRequest API, specifically the onHeadersReceived method, is intended for modifying HTTP response headers [1][2]. It is important to note that the file:// protocol is not an HTTP-based protocol and does not support or recognize HTTP headers [3]. Consequently, the webRequest.onHeadersReceived event is not triggered for file:// protocol requests, and you cannot use this method to inject a Content-Security-Policy (CSP) into files loaded via file:// [3]. For resources loaded using the file:// protocol, the official Electron documentation recommends using the HTML tag delivery mechanism instead [1]: If you must apply a CSP to local files and cannot use the tag (for example, if you need to dynamically inject or change policies for local content), developers often turn to protocol interception as an alternative [4]. By using protocol.interceptBufferProtocol or similar methods, you can intercept requests for file:// resources and modify the HTML content to include the desired CSP meta tag before it is rendered by the application [4].
Citations:
- 1: https://electronjs.org/docs/latest/tutorial/security
- 2: https://electronjs.org/docs/latest/api/web-request
- 3: doc: add CSP examples electron/electron#13167
- 4: https://blog.coding.kiwi/electron-csp-local/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- desktop files ---'
git ls-files apps/desktop | sed -n '1,240p'
printf '%s\n' '--- load/navigation code and desktop config ---'
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' \
"load(File|URL)|file://|http://|https://|BrowserWindow|webContents|protocol|vite|renderer" \
apps/desktop | head -240
printf '%s\n' '--- package metadata ---'
fd -i 'package.json' . -t f | xargs -r -n1 sh -c 'echo "--- $0"; cat "$0"'Repository: protonspy/open-wiki
Length of output: 12922
🌐 Web query:
site:electronjs.org/docs latest tutorial security Content-Security-Policy file protocol meta tag frame-ancestors
💡 Result:
You cannot use the frame-ancestors directive within a Content-Security-Policy (CSP) meta tag. According to web security standards, frame-ancestors is a policy directive that is ignored if it is provided via a meta tag; it must be delivered via an HTTP response header to function correctly. Additionally, the Content-Security-Policy meta tag is generally not supported for the file:// protocol in many browser and environment contexts. When working with protocols such as file://, security policies are often restricted or handled differently than over standard web protocols (http/https). For applications requiring strict CSP controls (such as Electron apps), it is standard practice to use the session.webRequest.onHeadersReceived API or equivalent mechanisms to inject the CSP as an HTTP header, rather than relying on a meta tag.
Citations:
- 1: https://electronjs.org/docs/latest/tutorial/debugging-main-process
- 2: https://electronjs.org/docs/latest/api/safe-storage
- 3: https://electronjs.org/docs/latest/experimental
- 4: https://electronjs.org/docs/latest/api/structures/jump-list-item
- 5: https://electronjs.org/docs/latest/development/api-history-migration-guide
Remove frame-ancestors from the meta CSP. Browsers ignore this directive in a <meta> element. The production renderer uses file://, so session.webRequest.onHeadersReceived cannot enforce it. Use a supported header or protocol-based approach only if framing protection is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/index.html` around lines 15 - 18, Remove the
unsupported frame-ancestors directive from the Content-Security-Policy meta tag
in the renderer HTML. Preserve the remaining CSP directives unchanged; do not
add alternative framing protection unless an existing supported header or
protocol-based mechanism is already part of the implementation.
| /** | ||
| * Whether the application may hand a URL to the system browser. | ||
| * | ||
| * `shell.openExternal` is `ShellExecute` on Windows, which invokes whichever | ||
| * protocol handler is registered — `ms-msdt:`, `ms-officecmd:`, `search-ms:` | ||
| * against a WebDAV share. Those are documented paths from "a link in a | ||
| * document" to code execution, and markdown-it's own link filter blocks only | ||
| * `javascript:`, `vbscript:`, `file:` and most `data:`. So the answer is an | ||
| * allowlist rather than a blocklist. | ||
| */ | ||
| const OPENABLE = new Set(["http:", "https:", "mailto:"]); | ||
|
|
||
| export function isOpenableExternally(href: string): boolean { | ||
| try { | ||
| return OPENABLE.has(new URL(href).protocol); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace external-link handling across the main process and renderer.
set -euo pipefail
echo "--- index.ts navigation/openExternal wiring ---"
rg -n -B3 -A15 'will-navigate|refuseNavigation|shell.openExternal|isOpenableExternally' apps/desktop/src/main/index.ts
echo "--- navigation.ts isOpenableExternally usages repo-wide ---"
rg -n 'isOpenableExternally' apps/desktop/src
echo "--- App.tsx onPageClick full body (beyond the supplied snippet) ---"
rg -n -A40 'const onPageClick' apps/desktop/src/renderer/App.tsxRepository: protonspy/open-wiki
Length of output: 5346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
index = Path("apps/desktop/src/main/index.ts").read_text()
app = Path("apps/desktop/src/renderer/App.tsx").read_text()
refuse = re.search(
r"const refuseNavigation\s*=\s*\([^)]*\)\s*=>\s*\{(?P<body>.*?)\n\s*\};",
index,
re.S,
)
click = re.search(
r"const onPageClick\s*=\s*useCallback\(\s*\(event:.*?\)\s*=>\s*\{(?P<body>.*?)\n\s*\},",
app,
re.S,
)
assert refuse and click
refuse_body = refuse.group("body")
click_body = click.group("body")
print("index.ts imports isOpenableExternally:",
'import { isOpenableExternally } from "../renderer/navigation.js";' in index)
print("setWindowOpenHandler calls shell.openExternal:",
"if (isOpenableExternally(url)) void shell.openExternal(url);" in index)
print("refuseNavigation calls shell.openExternal:",
"shell.openExternal" in refuse_body)
print("refuseNavigation prevents disallowed URL:",
"if (url !== allowed) event.preventDefault();" in refuse_body)
print("onPageClick prevents page targets:",
'if (target.kind === "page")' in click_body and "event.preventDefault();" in click_body)
print("onPageClick prevents source targets:",
'target.kind === "source"' in click_body)
print("onPageClick has external branch:",
re.search(r"target\.kind\s*===\s*['\"]external['\"]", click_body) is not None)
print("onPageClick calls shell.openExternal:",
"shell.openExternal" in click_body)
PYRepository: protonspy/open-wiki
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- apps/desktop/src/main/index.ts ---"
sed -n '80,110p' apps/desktop/src/main/index.ts
echo "--- apps/desktop/src/renderer/App.tsx ---"
sed -n '118,140p' apps/desktop/src/renderer/App.tsxRepository: protonspy/open-wiki
Length of output: 2931
Route left-click external links through shell.openExternal.
onPageClick allows external links to navigate, but refuseNavigation blocks that navigation without opening the system browser. setWindowOpenHandler handles only the new-window path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/navigation.ts` around lines 109 - 127, Update
onPageClick to call shell.openExternal for openable external URLs before
navigation is refused, using isOpenableExternally to validate the href. Preserve
internal-link navigation and ensure refuseNavigation does not leave approved
external left-click links unopened; keep setWindowOpenHandler behavior
unchanged.
| /* --- Ink. Four steps of contrast against surface-0, all above 4.5:1. */ | ||
| --ink-0: #e8ecf2; | ||
| --ink-1: #b6bec9; | ||
| --ink-2: #808a97; | ||
| --ink-3: #5b646f; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
--ink-3 does not meet the 4.5:1 the comment claims.
The comment states that all four ink steps are above 4.5:1 against --surface-0. --ink-3 (#5b646f) against --surface-0 (#101216) computes to about 3.17:1 by the WCAG relative-luminance formula. --ink-2 (#808a97) computes to about 5.36:1, so the other steps hold.
--ink-3 is applied to .frontmatter dt at line 258, which renders at --text-xs (12px). That is normal-size body text, so WCAG AA requires 4.5:1.
Either lighten --ink-3 to reach 4.5:1, or correct the comment and restrict --ink-3 to non-text use.
🎨 A value that reaches 4.5:1
--ink-2: `#808a97`;
- --ink-3: `#5b646f`;
+ /* `#5b646f` was 3.17:1 against --surface-0; this is ~4.6:1. */
+ --ink-3: `#737d8a`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/tokens.css` around lines 22 - 26, Update the
--ink-3 token to a color that achieves at least 4.5:1 contrast against
--surface-0, preserving the existing four-step contrast claim and its use by
.frontmatter dt.
1 participant
Closes tasks 8.1, 8.2, 8.5 and 8.10 of
plans/open-wiki.md. This is the first Electron code in the repo.What changed
The entry point is wiring and nothing else. Which project this window is, what the renderer may ask for, how a page becomes HTML, where Back goes, what a folder change means — each is a module beside it that a test calls without starting a window, because CI has no display. Same seam the recorder put under
CaptureSourceand the pipeline underFfmpegRunner.The project root is bound in the main process and never crosses the bridge. The renderer names a slug, never a path — and a page is resolved by slug through the index rather than by joining a slug onto a path, so
adr:0016makes the correct implementation also the one that does no path arithmetic on untrusted input.A launch that names no project says so and quits. Falling back to the working directory would open somebody's home folder as a wiki.
The three findings worth reading
Opening the window opened the microphone. The status poll went through the lazy
session ??=, andrecorder.exeopens both WASAPI devices the moment it launches — before it reads a request. Launching the app held the microphone for the window's lifetime while the chrome said nothing was being recorded: exactly the failure the persistent indicator exists to prevent, inverted.record:starttook a filesystem path from the renderer. Every other handler binds the project root; this was the exception. It now takes the occasion, and the directory comes from 4.16's id.The renderer could navigate away with the preload attached. A click handler in the page is a convenience, not a boundary. The main process refuses navigation now, and
shell.openExternal— which isShellExecuteon Windows, invoking whichever protocol handler is registered — allowlists the scheme.Plus: both markdown extensions became markdown-it rules rather than
String.replaceover rendered HTML, which was breaking out oftitle="…"attributes and rendering every code span that quotes the wiki syntax as a live link. Several pages in this repository do exactly that.How it was verified
pnpm test— 907 passing (100 new in@open-wiki/desktop)pnpm --filter @open-wiki/desktop test:coverage— 93.5% lines, floor is 76%pnpm run typecheck,pnpm lint,prettier --check— cleanscc validate— no findingscode-reviewandsecurity-reviewsubagents on the diff; every finding closed in the follow-up commitWhat is not covered
.tsxview components are outside the coverage instrument (the shared config includessrc/**/*.ts), andpreload.tsis excluded because it importselectronand decides nothing. The review checked specifically whether meaningful behaviour hid there; the one rule that had — "is this change the open page?" — moved intowatcher.tswhere it is tested.Electron is bumped to 38: 33 is outside its support window and its Chromium no longer receives security fixes, which is the wrong floor for an application whose threat model is untrusted content in a renderer.
🤖 Generated with Claude Code
https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
Summary by CodeRabbit
New Features
Style