fix: close the three plan gaps whose visible half was missing (3.7, 6.5, 8.12) - #17
Conversation
…e doorway Three tasks the plan ticked whose visible half was missing, and the small untruths around them. 6.5 — a page now says which sources it came from, on the page itself. The backend, its IPC channel and its tests all existed; no component called any of them, so the feature was done from every angle except the user's. It carries the title and somewhere to click rather than the bare id, and clicking opens the panel a provenance link opens (8.6) at the source's own start — `p1` for a document, `0:00` for a recording, which are the anchors `pdf.ts` and 4.13 write. A citation whose source is gone is shown as broken rather than dropped, for the same reason 8.5 marks an unresolvable wikilink: hiding it would leave the reader believing the page is sourced. 8.12 — the launcher asks for the content language instead of passing a hardcoded "en", so a project created through the application is no longer born in a language nobody chose. It is a form rather than a chain of `prompt()` calls: Electron does not implement `window.prompt`, and three named options are not something a text box can offer. The list lives in one module both screens import, typed as the `Language` union, so a language added to the setting and forgotten in the picker is a compile error. 3.7 — the inbox watcher is wired into the shell, which is what 8.2 was always meant to hold open. An arrival is reported where a drop is reported, because the doorway and the drop zone are two ways into one registration. It starts asynchronously — `watchInbox` waits for chokidar's initial scan — and a window closed before the handle arrives closes it anyway. A doorway that stops working is reported rather than logged: a quiet watcher is indistinguishable from an empty inbox, and what it drops is material an agent believes it handed over. Alongside them: - `PUSH_CHANNELS` names the main-to-renderer channels once, so a push channel never gets an `ipcMain.handle` nobody wrote. - The symlink and junction containment tests report a skip instead of returning green when the OS refuses to create the link. A junction needs no privilege on Windows, so failing to make one there is now a failure, not a skip — that is the whole reason the case exists beside the symlink one. - `fetch-ffmpeg.mjs` no longer claims to pin a hash it does not pin. The URL is a rolling one, so the digest comes from the environment by design; the comment now says that instead of the opposite. - `transcriptionProgress` is deleted. It had no callers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X24yWBFHyEkjZV5k8hDN4i
The inbox wiring changed what opening a project does, and the reviews caught it from two sides. **`raw/` arrives with a clone.** A repository can ship `raw/_inbox/x.pdf`, and the watcher as wired ingested it during its initial scan: a stranger's bytes parsed by pdfjs or mammoth in the privileged main process, and the file deleted out of the user's working tree, with nobody having clicked anything. The doorway exists for an agent handing something over *during a session* — that is what an event is — so the desktop now passes `ingestExisting: false`. What is already there is listed and left alone; what arrives while the window is open is still taken on sight. Left alone is not lost: `inboxWaiting` and `inboxDrain` say what is in the doorway and take it when asked. That report is pulled rather than pushed, which is the other half of the fix — `webContents.send` before the document has loaded is dropped with no queue and no error, so every outcome of the initial scan was announced into the void. Live arrivals stay pushed and are now buffered until `did-finish-load`. Also from the reviews: - `readManifest` checks the manifest's shape instead of casting it. A `title` that was not a string reached the screen as a React child and blanked the whole window — there is no error boundary — and this diff had just moved that render onto the primary reading view. The id comes from the directory now, never from the file, which is what `adr:0011` already said. - `PageSources` clears before it fetches. It survives navigation, so page A's provenance sat under page B's title until the walk returned — the one wrong answer available to a component that says where a page came from. - A source that is there but unreadable no longer reports as "there is no source named x". Different problems, different fixes. - `return started.close()` rather than `void`: the discarded promise escaped the chained `.catch` and became an unhandled rejection in the main process. - The inbox listener no longer bumps `reloadKey`. The ingest writes under `raw/`, which 8.10's watcher already reports and coalesces, where thirty arrivals here were thirty un-coalesced walks of the project. - A drop appends to the banner instead of replacing it, so an inbox report the user has not dismissed is not discarded; a repeated identical outcome is not appended twice. Tests: the fragment case now asserts through `locateCitation` rather than against the constant it was transcribing — the recording branch is the one that can silently break, and asserting `"0:00"` would have encoded a wrong string faithfully. The channel test pins `PUSH_CHANNELS` membership, which it had started trusting from the same place production trusts it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X24yWBFHyEkjZV5k8hDN4i
📝 WalkthroughWalkthroughThe PR adds desktop inbox watching and draining, structured source citation results, manifest validation, and a language-aware project creation form. It also updates IPC routing, renderer state handling, access exports, filesystem tests, and FFmpeg extraction guidance. ChangesInbox ingestion
Source citations and manifest validation
Language-aware project creation
Validation and build documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant InboxWatcher
participant ElectronMain
participant PreloadBridge
participant RendererApp
InboxWatcher->>ElectronMain: publish inbox outcome
ElectronMain->>PreloadBridge: send buffered inbox event
PreloadBridge->>RendererApp: deliver outcome
RendererApp->>PreloadBridge: request inbox drain
PreloadBridge->>ElectronMain: invoke inboxDrain
ElectronMain->>InboxWatcher: drain pending files
InboxWatcher-->>ElectronMain: return drop outcomes
ElectronMain-->>RendererApp: return drained outcomes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/desktop/src/renderer/languages.ts (1)
10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake language coverage exhaustive.
Import the canonical
LANGUAGESlist from@open-wiki/access. Define labels asRecord<Language, string>and generate the renderer options from that list.🤖 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/languages.ts` around lines 10 - 17, Update the renderer language options around the LANGUAGES constant to import and iterate over the canonical LANGUAGES list from `@open-wiki/access`. Replace the array’s inline language coverage with a Record<Language, string> label map, then generate each option using the canonical value and its mapped label so all supported languages are covered exhaustively.
🤖 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/index.ts`:
- Around line 76-81: Update the inboxDrain helper so the active watcher branch
still invokes inbox.drain() but returns an empty outcome array, since the
watcher’s onOutcome handler already delivers those results to the renderer;
preserve drainInbox(root, ...) and its returned outcomes when no watcher exists.
Add a regression test covering a fake InboxControl whose drain both emits and
resolves outcomes, verifying the resulting report contains no duplicates.
In `@apps/desktop/src/renderer/App.tsx`:
- Around line 596-604: Update the InboxWaiting load flow around the load
callback and its useEffect to use a live flag like PageSources: mark the request
active before calling bridge().inboxWaiting(), ignore setNames updates after
cleanup, and deactivate the flag in the effect cleanup so stale responses cannot
overwrite newer results. Preserve the existing no-bridge guard and empty-list
error handling.
In `@apps/desktop/src/renderer/Launcher.tsx`:
- Around line 135-183: Update the new-project form in the Launcher component to
use a form onSubmit handler that triggers create, make the Create button a
submit button, and set the Cancel button to type="button". Disable the name and
directory inputs plus every language radio input while busy, preserving the
existing create and cancel behavior.
In `@apps/desktop/src/renderer/Panels.tsx`:
- Around line 68-76: Update the resolved-source anchor in the source rendering
branch of Panels to be keyboard-accessible by replacing it with a type="button"
button styled like a link, while preserving its title, displayed source.title,
and onOpen(source.id, source.fragment) behavior.
In `@packages/access/tests/paths.spec.ts`:
- Around line 63-72: In packages/access/tests/paths.spec.ts at lines 63-72, add
a shared capability-error check that allows ctx.skip only for symlink errors
with codes EPERM, EACCES, or ENOSYS; rethrow all other errors so they fail the
test. Apply the same classification at lines 87-97 while preserving the existing
Windows-specific failure behavior.
---
Nitpick comments:
In `@apps/desktop/src/renderer/languages.ts`:
- Around line 10-17: Update the renderer language options around the LANGUAGES
constant to import and iterate over the canonical LANGUAGES list from
`@open-wiki/access`. Replace the array’s inline language coverage with a
Record<Language, string> label map, then generate each option using the
canonical value and its mapped label so all supported languages are covered
exhaustively.
🪄 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: 9df41e1b-9711-40a0-ad75-02635ded4863
📒 Files selected for processing (24)
apps/desktop/src/main/channels.tsapps/desktop/src/main/index.tsapps/desktop/src/main/ingest.tsapps/desktop/src/main/ipc.tsapps/desktop/src/main/preload.tsapps/desktop/src/main/sources.tsapps/desktop/src/main/transcribe-run.tsapps/desktop/src/renderer/App.tsxapps/desktop/src/renderer/Launcher.tsxapps/desktop/src/renderer/Panels.tsxapps/desktop/src/renderer/Settings.tsxapps/desktop/src/renderer/bridge.tsapps/desktop/src/renderer/languages.tsapps/desktop/src/renderer/tokens.cssapps/desktop/tests/sources.spec.tspackages/access/src/index.tspackages/access/src/read.tspackages/access/src/sources/inbox.tspackages/access/src/sources/manifest.tspackages/access/tests/paths.spec.tspackages/access/tests/sources-inbox.spec.tspackages/access/tests/sources-manifest.spec.tsplans/open-wiki.mdscripts/fetch-ffmpeg.mjs
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/transcribe-run.ts
| // The window's own watcher once its initial scan has finished, so an explicit | ||
| // drain and an event cannot both read the same file and both try to register | ||
| // the same id. Standalone until then: a drain must still work in the seconds | ||
| // between the window opening and the scan completing. | ||
| const inboxDrain = (root: string): Promise<InboxOutcome[]> => | ||
| inbox ? inbox.drain() : drainInbox(root, { stabilityMs: INBOX_STABILITY_MS }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Explicit inbox drain reports every outcome twice to the renderer.
inboxDrain returns inbox.drain()'s result. inbox.drain() (the InboxWatcher from packages/access/src/sources/inbox.ts) internally calls announce(outcome) for every outcome and returns the same outcomes. announce invokes the onOutcome handler registered at line 151, which pushes the outcome to the renderer over CHANNELS.inbox. The renderer already appends every CHANNELS.inbox push through its onInbox subscription (apps/desktop/src/renderer/App.tsx), then separately appends the array returned by the inboxDrain() IPC call through the onTaken prop (apps/desktop/src/renderer/App.tsx), which does not deduplicate.
The result: clicking "Add them" while the watcher is active shows each newly ingested or refused file twice in the "X of Y added" report. This only affects the explicit on-request drain; live add/change events are reported once, as expected.
Return an empty array from the watcher branch of this helper, since those outcomes are already delivered through onOutcome.
🐛 Proposed fix to stop the double delivery
const inboxDrain = (root: string): Promise<InboxOutcome[]> =>
- inbox ? inbox.drain() : drainInbox(root, { stabilityMs: INBOX_STABILITY_MS });
+ inbox
+ ? // `inbox.drain()` already reports every outcome through `onOutcome`
+ // (`CHANNELS.inbox`) below. Returning them here too would hand the
+ // renderer the same outcomes a second time.
+ inbox.drain().then(() => [])
+ : drainInbox(root, { stabilityMs: INBOX_STABILITY_MS });Consider adding a regression test in apps/desktop/tests/sources.spec.ts that wires a fake InboxControl whose drain() both resolves outcomes and simulates the onOutcome push, then asserts the resulting drop report has no duplicate entries — the current tests only exercise inboxDrain() without a live deps.inbox, so this path is untested.
Also applies to: 147-169
🤖 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 76 - 81, Update the inboxDrain
helper so the active watcher branch still invokes inbox.drain() but returns an
empty outcome array, since the watcher’s onOutcome handler already delivers
those results to the renderer; preserve drainInbox(root, ...) and its returned
outcomes when no watcher exists. Add a regression test covering a fake
InboxControl whose drain both emits and resolves outcomes, verifying the
resulting report contains no duplicates.
| const load = useCallback(() => { | ||
| if (!hasBridge()) return; | ||
| void bridge() | ||
| .inboxWaiting() | ||
| .then(setNames) | ||
| .catch(() => setNames([])); | ||
| }, []); | ||
|
|
||
| useEffect(load, [load, reloadKey]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
InboxWaiting.load() has no guard against out-of-order responses.
load() calls bridge().inboxWaiting() on every reloadKey change with no protection against a slower, earlier response overwriting a later one's result. reloadKey changes often in this component tree (drops, inbox drains, coalesced project-change events), so overlapping calls are plausible.
The sibling PageSources component in apps/desktop/src/renderer/Panels.tsx guards its own reloadKey-driven fetch with a live flag for the same reason. Apply the same pattern here.
♻️ Proposed fix to ignore stale responses
const load = useCallback(() => {
if (!hasBridge()) return;
- void bridge()
- .inboxWaiting()
- .then(setNames)
- .catch(() => setNames([]));
+ let live = true;
+ void bridge()
+ .inboxWaiting()
+ .then((found) => {
+ if (live) setNames(found);
+ })
+ .catch(() => {
+ if (live) setNames([]);
+ });
+ return () => {
+ live = false;
+ };
}, []);
- useEffect(load, [load, reloadKey]);
+ useEffect(() => load(), [load, reloadKey]);📝 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.
| const load = useCallback(() => { | |
| if (!hasBridge()) return; | |
| void bridge() | |
| .inboxWaiting() | |
| .then(setNames) | |
| .catch(() => setNames([])); | |
| }, []); | |
| useEffect(load, [load, reloadKey]); | |
| const load = useCallback(() => { | |
| if (!hasBridge()) return; | |
| let live = true; | |
| void bridge() | |
| .inboxWaiting() | |
| .then((found) => { | |
| if (live) setNames(found); | |
| }) | |
| .catch(() => { | |
| if (live) setNames([]); | |
| }); | |
| return () => { | |
| live = false; | |
| }; | |
| }, []); | |
| useEffect(() => load(), [load, reloadKey]); |
🤖 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/App.tsx` around lines 596 - 604, Update the
InboxWaiting load flow around the load callback and its useEffect to use a live
flag like PageSources: mark the request active before calling
bridge().inboxWaiting(), ignore setNames updates after cleanup, and deactivate
the flag in the effect cleanup so stale responses cannot overwrite newer
results. Preserve the existing no-bridge guard and empty-list error handling.
| return ( | ||
| <section className="launcher__new"> | ||
| <h3>New project</h3> | ||
| <label> | ||
| Name | ||
| <input | ||
| className="editor__source" | ||
| value={name} | ||
| placeholder="fenix" | ||
| autoFocus | ||
| onChange={(event) => setName(event.target.value)} | ||
| /> | ||
| </label> | ||
| <label> | ||
| Directory | ||
| <input | ||
| className="editor__source" | ||
| value={directory} | ||
| placeholder="C:\projects\fenix" | ||
| onChange={(event) => setDirectory(event.target.value)} | ||
| /> | ||
| </label> | ||
| <fieldset className="launcher__languages"> | ||
| <legend>Content language</legend> | ||
| <p className="empty"> | ||
| What transcription is told to expect, and what the generated CLAUDE.md tells the agent to | ||
| write pages in. The schema itself stays English, and this is changeable later. | ||
| </p> | ||
| <div className="editor__bar"> | ||
| {LANGUAGES.map((option) => ( | ||
| <label key={option.value}> | ||
| <input | ||
| type="radio" | ||
| name="language" | ||
| checked={language === option.value} | ||
| onChange={() => setLanguage(option.value)} | ||
| />{" "} | ||
| {option.label} | ||
| </label> | ||
| ))} | ||
| </div> | ||
| </fieldset> | ||
| <div className="editor__bar"> | ||
| <button onClick={() => void create()} disabled={busy}> | ||
| {busy ? "Creating…" : "Create"} | ||
| </button> | ||
| <button onClick={onCancel} disabled={busy}> | ||
| Cancel | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use form submission and disable all input controls while creating.
Because this wrapper is a section, Enter in a text input does not start creation. Use a <form onSubmit> with a submit button and a type="button" cancel button.
Add disabled={busy} to both text inputs and each language radio input. Otherwise, the displayed values can change while createProject uses the earlier values.
Proposed fix
- <section className="launcher__new">
+ <form
+ className="launcher__new"
+ onSubmit={(event) => {
+ event.preventDefault();
+ void create();
+ }}
+ >
...
<input
className="editor__source"
value={name}
+ disabled={busy}
...
<input
className="editor__source"
value={directory}
+ disabled={busy}
...
name="language"
checked={language === option.value}
+ disabled={busy}
onChange={() => setLanguage(option.value)}
...
- <button onClick={() => void create()} disabled={busy}>
+ <button type="submit" disabled={busy}>
...
- <button onClick={onCancel} disabled={busy}>
+ <button type="button" onClick={onCancel} disabled={busy}>
...
- </section>
+ </form>📝 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.
| return ( | |
| <section className="launcher__new"> | |
| <h3>New project</h3> | |
| <label> | |
| Name | |
| <input | |
| className="editor__source" | |
| value={name} | |
| placeholder="fenix" | |
| autoFocus | |
| onChange={(event) => setName(event.target.value)} | |
| /> | |
| </label> | |
| <label> | |
| Directory | |
| <input | |
| className="editor__source" | |
| value={directory} | |
| placeholder="C:\projects\fenix" | |
| onChange={(event) => setDirectory(event.target.value)} | |
| /> | |
| </label> | |
| <fieldset className="launcher__languages"> | |
| <legend>Content language</legend> | |
| <p className="empty"> | |
| What transcription is told to expect, and what the generated CLAUDE.md tells the agent to | |
| write pages in. The schema itself stays English, and this is changeable later. | |
| </p> | |
| <div className="editor__bar"> | |
| {LANGUAGES.map((option) => ( | |
| <label key={option.value}> | |
| <input | |
| type="radio" | |
| name="language" | |
| checked={language === option.value} | |
| onChange={() => setLanguage(option.value)} | |
| />{" "} | |
| {option.label} | |
| </label> | |
| ))} | |
| </div> | |
| </fieldset> | |
| <div className="editor__bar"> | |
| <button onClick={() => void create()} disabled={busy}> | |
| {busy ? "Creating…" : "Create"} | |
| </button> | |
| <button onClick={onCancel} disabled={busy}> | |
| Cancel | |
| </button> | |
| return ( | |
| <form | |
| className="launcher__new" | |
| onSubmit={(event) => { | |
| event.preventDefault(); | |
| void create(); | |
| }} | |
| > | |
| <h3>New project</h3> | |
| <label> | |
| Name | |
| <input | |
| className="editor__source" | |
| value={name} | |
| disabled={busy} | |
| placeholder="fenix" | |
| autoFocus | |
| onChange={(event) => setName(event.target.value)} | |
| /> | |
| </label> | |
| <label> | |
| Directory | |
| <input | |
| className="editor__source" | |
| value={directory} | |
| disabled={busy} | |
| placeholder="C:\projects\fenix" | |
| onChange={(event) => setDirectory(event.target.value)} | |
| /> | |
| </label> | |
| <fieldset className="launcher__languages"> | |
| <legend>Content language</legend> | |
| <p className="empty"> | |
| What transcription is told to expect, and what the generated CLAUDE.md tells the agent to | |
| write pages in. The schema itself stays English, and this is changeable later. | |
| </p> | |
| <div className="editor__bar"> | |
| {LANGUAGES.map((option) => ( | |
| <label key={option.value}> | |
| <input | |
| type="radio" | |
| name="language" | |
| checked={language === option.value} | |
| disabled={busy} | |
| onChange={() => setLanguage(option.value)} | |
| />{" "} | |
| {option.label} | |
| </label> | |
| ))} | |
| </div> | |
| </fieldset> | |
| <div className="editor__bar"> | |
| <button type="submit" disabled={busy}> | |
| {busy ? "Creating…" : "Create"} | |
| </button> | |
| <button type="button" onClick={onCancel} disabled={busy}> | |
| Cancel | |
| </button> | |
| </div> | |
| </form> | |
| ); |
🤖 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/Launcher.tsx` around lines 135 - 183, Update the
new-project form in the Launcher component to use a form onSubmit handler that
triggers create, make the Create button a submit button, and set the Cancel
button to type="button". Disable the name and directory inputs plus every
language radio input while busy, preserving the existing create and cancel
behavior.
| {source.kind === null ? ( | ||
| <span className="wikilink--broken" title={source.reason ?? source.id}> | ||
| {source.id} | ||
| </span> | ||
| ) : ( | ||
| <a title={source.id} onClick={() => onOpen(source.id, source.fragment)}> | ||
| {source.title} | ||
| </a> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- file outline ---'
ast-grep outline apps/desktop/src/renderer/Panels.tsx --match 'SourceAt' --view expanded || true
printf '%s\n' '--- relevant source ---'
sed -n '1,130p' apps/desktop/src/renderer/Panels.tsx
printf '%s\n' '--- anchor/button styles and usages ---'
rg -n -C 3 'link-button|wikilink--broken|SourceAt|onOpen=\{|<a[^>]*onClick' apps/desktop/src/renderer apps/desktop/src -g '*.{css,scss,tsx,ts}' || trueRepository: protonspy/open-wiki
Length of output: 15280
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- remaining component and styles ---'
sed -n '180,245p' apps/desktop/src/renderer/Panels.tsx
sed -n '250,310p' apps/desktop/src/renderer/tokens.css
printf '%s\n' '--- accessibility and link-button conventions ---'
rg -n -C 4 'tabIndex|onKeyDown|role="link"|role=.link.|button.*className|className=.*button|jsx-a11y|accessib' apps/desktop package.json -g '*.{tsx,ts,css,json}' || true
printf '%s\n' '--- deterministic JSX check ---'
python3 - <<'PY'
from pathlib import Path
text = Path("apps/desktop/src/renderer/Panels.tsx").read_text()
start = text.index('<a title={source.id}')
end = text.index('</a>', start) + len('</a>')
element = text[start:end]
print(element)
for attr in ("href", "tabIndex", "onKeyDown", "role"):
print(f"{attr}: {attr in element}")
PYRepository: protonspy/open-wiki
Length of output: 5541
Make the resolved source link keyboard-accessible.
The <a> has no href, tabIndex, role, or keyboard handler. Replace it with a <button type="button"> styled as a link, or add complete keyboard link behavior.
🤖 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/Panels.tsx` around lines 68 - 76, Update the
resolved-source anchor in the source rendering branch of Panels to be
keyboard-accessible by replacing it with a type="button" button styled like a
link, while preserving its title, displayed source.title, and onOpen(source.id,
source.fragment) behavior.
| try { | ||
| symlinkSync(outside, link); | ||
| } catch { | ||
| // Some Windows accounts lack the symlink privilege; that is a different | ||
| // failure from the containment logic and should not fail this test. | ||
| rmSync(root, { recursive: true, force: true }); | ||
| } catch (err) { | ||
| // Creating a symlink is privileged on most Windows accounts, and that is | ||
| // a different failure from the containment logic. **Reported as a skip, | ||
| // never as a pass**: a test that silently returns green is one nobody | ||
| // knows stopped running, and this is the check standing between a | ||
| // citation and a file anywhere on disk. | ||
| rmSync(outside, { force: true }); | ||
| ctx.skip(`symlink creation unavailable: ${err instanceof Error ? err.message : err}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not convert arbitrary filesystem errors into skipped tests.
Use one shared capability-error check. Skip only EPERM, EACCES, or ENOSYS. Keep all other errors as test failures.
packages/access/tests/paths.spec.ts#L63-L72: classify symlink errors before callingctx.skip.packages/access/tests/paths.spec.ts#L87-L97: preserve the Windows failure behavior and apply the same classification on other platforms.
📍 Affects 1 file
packages/access/tests/paths.spec.ts#L63-L72(this comment)packages/access/tests/paths.spec.ts#L87-L97
🤖 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 `@packages/access/tests/paths.spec.ts` around lines 63 - 72, In
packages/access/tests/paths.spec.ts at lines 63-72, add a shared
capability-error check that allows ctx.skip only for symlink errors with codes
EPERM, EACCES, or ENOSYS; rethrow all other errors so they fail the test. Apply
the same classification at lines 87-97 while preserving the existing
Windows-specific failure behavior.
The release would have failed at "Fetch and verify ffmpeg", with the hash set and everything else correct. The archive is `ffmpeg-<version>-essentials_build/bin/ffmpeg.exe`, so stripping one leading component left `bin/ffmpeg.exe` and the script's own check — `extraction produced no vendor/ffmpeg/ffmpeg.exe` — was right to fail. Nothing had ever run it against a real download: `vendor/ffmpeg/` is gitignored, no test covers a 110 MB fetch, and CI only reaches this step on a `v*` tag, which this repository has never had. Naming the member is also what makes the docstring true. It has always claimed to extract "only `ffmpeg.exe`", where it was unpacking the documentation and the presets as well. Verified by running it: `node scripts/fetch-ffmpeg.mjs` against the real URL with the real digest now reports `ok → vendor\ffmpeg\ffmpeg.exe`, and that binary answers `ffmpeg version 8.1.2-essentials_build`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X24yWBFHyEkjZV5k8hDN4i
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/fetch-ffmpeg.mjs (1)
84-89: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the missing-hash guidance match the external configuration contract.
FFMPEG_SHA256is supplied through the environment. The current message also tells maintainers to pin the hash in this script. Use environment-only wording.Proposed wording
- "FFMPEG_SHA256 is not set. Pin the expected hash of FFMPEG_URL (in this " + - "script or the FFMPEG_SHA256 env var) before fetching — a download " + - "that verifies nothing is the fetch-and-execute this product refuses.", + "FFMPEG_SHA256 is not set. Set the expected SHA-256 for FFMPEG_URL in " + + "the FFMPEG_SHA256 environment variable before fetching. The product " + + "refuses to bundle an unverified download.",🤖 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 `@scripts/fetch-ffmpeg.mjs` around lines 84 - 89, Update the missing-FFMPEG_SHA256 message in the fetch validation block to state that the expected hash must be set through the FFMPEG_SHA256 environment variable only; remove the suggestion to pin it in the script while preserving the existing failure behavior.
🤖 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 `@scripts/fetch-ffmpeg.mjs`:
- Around line 74-78: Update the extraction command in the spawnSync call to
invoke bsdtar explicitly instead of tar, while preserving the existing ZIP
arguments, destination, member pattern, and inherited stdio behavior.
---
Outside diff comments:
In `@scripts/fetch-ffmpeg.mjs`:
- Around line 84-89: Update the missing-FFMPEG_SHA256 message in the fetch
validation block to state that the expected hash must be set through the
FFMPEG_SHA256 environment variable only; remove the suggestion to pin it in the
script while preserving the existing failure behavior.
🪄 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: e22290b1-7eb6-46d4-915f-5365fb74a21f
📒 Files selected for processing (1)
scripts/fetch-ffmpeg.mjs
| const result = spawnSync( | ||
| "tar", | ||
| ["-xf", zip, "-C", dest, "--strip-components=2", "*/bin/ffmpeg.exe"], | ||
| { stdio: "inherit" }, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Find callers and runner/tool assumptions.
rg -n -C 6 'fetch-ffmpeg\.mjs|runs-on:|shell:|FFMPEG_URL|FFMPEG_SHA256' \
.github/workflows/release.yml scripts || true
# Identify the local tar implementation.
if command -v tar >/dev/null 2>&1; then
tar --version 2>&1 || true
fiRepository: protonspy/open-wiki
Length of output: 15697
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the extraction code and workflow context, then check repository
# documentation for platform and tool assumptions.
sed -n '65,82p' scripts/fetch-ffmpeg.mjs
sed -n '14,25p;90,112p' .github/workflows/release.yml
rg -n -i 'bsdtar|gnu tar|windows-latest|ffmpeg-release-essentials|strip-components|wildcard' \
README.md docs scripts .github 2>/dev/null || trueRepository: protonspy/open-wiki
Length of output: 3418
🌐 Web query:
GitHub Actions windows-latest runner tar implementation bsdtar GNU tar Windows image
💡 Result:
GitHub Actions Windows runners, including the windows-latest image, contain both BSD tar and GNU tar by default, but the GitHub-maintained actions/cache tooling uses specific logic to select between them [1][2]. BSD tar is natively available on Windows (typically at C:\Windows\System32\tar.exe) [3][1]. GNU tar is also available on these images, typically provided by Git for Windows (e.g., at C:\Program Files\Git\usr\bin\tar.exe) [1][4][2]. The GitHub actions/cache implementation has evolved in how it handles these tools: 1. Historical context: Early versions of the cache action explicitly used the system BSD tar because it was reliable and present on Windows [5]. However, this caused various issues, such as incompatibility with zstd compression and handling of certain file structures [6][1]. 2. Current implementation: The cache action logic is designed to prioritize the use of GNU tar for better compatibility and performance, particularly with zstd [6][7]. The implementation attempts to detect the presence and path of GNU tar; if detected, it uses that version with necessary flags (like --force-local) to ensure compatibility with Windows file paths [3][8]. 3. Fallback mechanism: If GNU tar is not found or is unavailable, the action falls back to BSD tar [6][7]. This ensures that the action remains functional across different runner environments, including self-hosted runners where GNU tar might not be installed in the standard locations found on GitHub-hosted runners [3][9]. In summary, while both tools exist on the Windows runner image, the GitHub cache action dynamically selects the tool based on availability and capability, with a preference for GNU tar [6][3]. Users writing their own workflows can explicitly call either binary by providing the full path (e.g., C:\Program Files\Git\usr\bin\tar.exe for GNU tar) if they have specific requirements [4].
Citations:
- 1: Select GNU tar for caching if available on hosted runners actions/toolkit#552
- 2: Add gnu-tar actions/runner-images#1534
- 3: https://gitea.psi.ch/actions/cache/src/commit/306f72536b44ddb5d2a1a1d8413a33eb8979df7b/src/tar.ts
- 4: Tar cannot overwrite read-only files on Windows actions/runner-images#936
- 5: Use BSD tar on windows actions/cache#126
- 6: Cache compression - cross OS support actions/cache#984
- 7: Cache package release for compression change in windows actions/toolkit#1281
- 8: https://gitea.psi.ch/actions/cache/src/commit/ccc66f769ef88f1608e1026d68e46f4e2c6ead35/src/tar.ts
- 9: cache trying to use git for windows' tar.exe on self-hosted runner, failing to find correct gzip actions/toolkit#1311
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '65,82p' scripts/fetch-ffmpeg.mjs
sed -n '14,25p;90,112p' .github/workflows/release.yml
rg -n -i 'bsdtar|gnu tar|windows-latest|ffmpeg-release-essentials|strip-components|wildcard' \
README.md docs scripts .github 2>/dev/null || trueRepository: protonspy/open-wiki
Length of output: 3418
🌐 Web query:
site:github.com/actions/runner-images windows-latest tar.exe PATH C:\\Windows\\System32 C:\\Program Files\\Git\\usr\\bin
💡 Result:
On GitHub Actions windows-latest runners, the availability and priority of tar.exe has been a long-standing point of conflict due to the presence of multiple implementations on the system [1][2][3]. The two primary locations are: 1. C:\Windows\System32\tar.exe: This is the native Windows implementation (bsdtar). It is generally preferred by system-level tools but has known limitations, such as difficulties handling certain .tar.xz archives or specific symlink configurations [1][4][5]. 2. C:\Program Files\Git\usr\bin\tar.exe: This is the GNU tar implementation bundled with Git for Windows [4][3]. It is often more compatible with Linux-style workflows and complex archives but can encounter path resolution issues when called from non-Git shells, or cause regressions if it inadvertently overrides the system default [1][4]. Key Technical Context: - Path Conflicts: Because both directories are typically included in the PATH environment variable, the version of tar executed depends on the order of the PATH entries at runtime [1][6]. GitHub Actions runner images have undergone several configuration changes over time to balance these conflicts, often leading to inconsistent behavior where PATH order may shift or differ across image versions [1][7][6]. - Workarounds: If your workflow requires a specific version of tar, it is recommended to explicitly call the binary by its full path [4]. For example, if you require the Git-provided GNU tar to avoid issues with the system-native version, you can invoke it directly: & "C:\Program Files\Git\usr\bin\tar.exe" -xzvf archive.tar.gz [4] - PATH Modification: If you must ensure a specific tar is used by default throughout a job, you can modify the PATH for subsequent steps by prepending the preferred directory to the GITHUB_PATH environment variable [5][8]: echo "C:\Program Files\Git\usr\bin" >> $env:GITHUB_PATH Due to the sensitive nature of the runner environment's default PATH configuration, modifying these system variables can sometimes lead to unexpected behavior in other tools [7][6][9]. Explicitly invoking the required executable by its absolute path remains the most reliable practice for GitHub Actions [4].
Citations:
- 1: The PATH has changed on Windows, and the Git shell tar is used instead of Windows tar actions/runner-images#480
- 2: Jobs stuck when trying to untar an archive actions/runner-images#282
- 3: Add gnu-tar actions/runner-images#1534
- 4: Tar cannot overwrite read-only files on Windows actions/runner-images#936
- 5: Permission issues on Windows restoring boost from cache actions/runner-images#4679
- 6: Windows
PATHno longer acts reliably / rearranges itself actions/runner-images#11328 - 7: Switch Unix tools from Git internal tools to MSYS2 tools on Windows images actions/runner-images#1525
- 8: [windows-2022] Please remove
C:\Strawberry\c\binfromPATHactions/runner-images#5459 - 9: Plethora of tools installed in path can lead to corruption and unexpected behavior on Windows actions/runner-images#11368
🌐 Web query:
bsdtar extract archive member wildcard command line pattern default --wildcards
💡 Result:
In bsdtar, wildcard pattern matching is enabled by default when extracting or listing archive members [1][2][3]. When you provide a filename or pattern on the command line, bsdtar interprets shell-style globbing patterns (as documented in tcsh(1)) to identify which items in the archive to process [2][3]. There is no dedicated --wildcards or --no-wildcards command-line option in bsdtar to toggle this behavior [4]. Because wildcards are always active, if you need to extract a file that contains special shell characters (such as an asterisk ''), you must escape those characters, for example by using backslashes ('*') or enclosing them in brackets ('[]') [4]. If you are reading filenames from a file (using the -T or --files-from option), bsdtar continues to treat those entries as patterns, which can cause unexpected behavior if the file list contains literal filenames that happen to include wildcard characters [4]. In such cases, you must manually escape any characters that have special meaning to the globbing engine before passing them to bsdtar [4].
Citations:
- 1: https://github.com/libarchive/libarchive/blob/master/tar/bsdtar.1
- 2: https://man.freebsd.org/cgi/man.cgi?query=bsdtar
- 3: https://man.freebsd.org/cgi/man.cgi?format=html&query=bsdtar&sektion=1
- 4: Please add --no-wildcards to bsdtar libarchive/libarchive#1733
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the exact tar invocation and all workflow steps that can invoke the
# script. Check whether the repository pins a tar executable or PATH.
rg -n -C 4 'spawnSync\(\s*"tar"|tar\.exe|PATH|fetch-ffmpeg\.mjs|runs-on:' \
scripts .githubRepository: protonspy/open-wiki
Length of output: 8178
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 'spawnSync\(\s*"tar"|tar\.exe|PATH|fetch-ffmpeg\.mjs|runs-on:' \
scripts .githubRepository: protonspy/open-wiki
Length of output: 8178
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Probe the local GNU tar with an in-memory-created ZIP archive in a temporary
# directory. This does not execute repository code or alter repository files.
python3 - <<'PY'
import pathlib
import subprocess
import tempfile
import zipfile
with tempfile.TemporaryDirectory() as td:
root = pathlib.Path(td)
archive = root / "ffmpeg.zip"
destination = root / "out"
destination.mkdir()
with zipfile.ZipFile(archive, "w") as zf:
zf.writestr("ffmpeg-7.0-essentials_build/bin/ffmpeg.exe", b"ok")
for args in (
["-xf", str(archive), "-C", str(destination),
"--strip-components=2", "*/bin/ffmpeg.exe"],
["-xf", str(archive), "-C", str(destination),
"--wildcards", "--strip-components=2", "*/bin/ffmpeg.exe"],
):
result = subprocess.run(
["tar", *args], text=True, capture_output=True
)
print("args:", args[2:])
print("status:", result.returncode)
print("stderr:", result.stderr.strip())
print("output:", sorted(str(p.relative_to(destination)) for p in destination.rglob("*")))
for child in destination.iterdir():
if child.is_dir():
for p in child.rglob("*"):
if p.is_file():
p.unlink()
child.rmdir()
else:
child.unlink()
PYRepository: protonspy/open-wiki
Length of output: 678
🌐 Web query:
GNU tar support ZIP archives tar -xf .zip official manual
💡 Result:
GNU tar does not natively support ZIP archives [1][2]. It is designed specifically to handle tar archives and various compression formats (such as gzip, bzip2, xz, etc.) used in conjunction with tar [1][2]. If you are using the tar command and successfully able to manipulate ZIP files, you are likely using bsdtar (part of the libarchive project), which is often aliased to tar on some operating systems like macOS and certain BSD-based distributions [3][4][5][6]. Unlike GNU tar, bsdtar supports a wide variety of archive formats, including ZIP, 7-zip, and JAR [3][4][5]. To verify which version of tar you have installed, you can run the following command in your terminal: tar --version If the output mentions "GNU tar," it will not support ZIP files [1][2]. If it mentions "bsdtar" or "libarchive," it will support ZIP files [4][5].
Citations:
- 1: https://man.archlinux.org/man/tar.1.en
- 2: https://manpages.debian.org/bullseye/tar/tar.1.en.html
- 3: https://www.daemon-systems.org/man/tar.1.html
- 4: https://github.com/libarchive/libarchive/blob/v3.7.7/tar/bsdtar.1
- 5: https://man.archlinux.org/man/bsdtar.1.en
- 6: https://unix.stackexchange.com/questions/146264/is-there-a-way-to-convert-a-zip-to-a-tar-without-extracting-it-to-the-filesystem
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import pathlib
import subprocess
import tempfile
import zipfile
with tempfile.TemporaryDirectory() as td:
root = pathlib.Path(td)
archive = root / "ffmpeg.zip"
destination = root / "out"
destination.mkdir()
with zipfile.ZipFile(archive, "w") as zf:
zf.writestr("ffmpeg-7.0-essentials_build/bin/ffmpeg.exe", b"ok")
for args in (
["-xf", str(archive), "-C", str(destination),
"--strip-components=2", "*/bin/ffmpeg.exe"],
["-xf", str(archive), "-C", str(destination),
"--wildcards", "--strip-components=2", "*/bin/ffmpeg.exe"],
):
result = subprocess.run(["tar", *args], text=True, capture_output=True)
print("args:", args[2:])
print("status:", result.returncode)
print("stderr:", result.stderr.strip())
print("output:", sorted(
str(p.relative_to(destination)) for p in destination.rglob("*")
))
for p in sorted(destination.rglob("*"), reverse=True):
if p.is_file():
p.unlink()
elif p.is_dir():
p.rmdir()
PYRepository: protonspy/open-wiki
Length of output: 678
Invoke bsdtar explicitly for ZIP extraction.
windows-latest includes both bsdtar and GNU tar. spawnSync("tar", ...) depends on PATH. GNU tar cannot read the ZIP archive, so extraction fails before the member pattern is evaluated. Invoke bsdtar explicitly or use a ZIP-specific extractor.
🤖 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 `@scripts/fetch-ffmpeg.mjs` around lines 74 - 78, Update the extraction command
in the spawnSync call to invoke bsdtar explicitly instead of tar, while
preserving the existing ZIP arguments, destination, member pattern, and
inherited stdio behavior.
1 participant
An audit of
plans/open-wiki.mdagainst the code found three tasks ticked[x]whose backend, IPC and tests all existed but whose user-facing half did not.
This closes them, plus the small untruths found alongside.
What changed
6.5 — a page says which sources it came from.
sourcesOfPagehad a handler,a channel and tests; no component called any of them, so the feature was done
from every angle except the user's. It now renders under the frontmatter,
carrying the source's title and somewhere to click rather than the bare id.
Clicking opens the panel a provenance link opens (8.6) at the source's own
start —
p1for a document,0:00for a recording, which are the anchorspdf.tsand 4.13 actually write. A citation whose source is gone is shown asbroken rather than dropped, for the same reason 8.5 marks an unresolvable
wikilink.
8.12 — the launcher asks for the content language. Changing it afterwards
worked and
ow init --languageasked, but the launcher passed a hardcoded"en", so a project created through the application was born in a languagenobody chose. It is a form rather than a chain of
prompt()calls: Electrondoes not implement
window.prompt, and three named options are not something atext box can offer. The list lives in one module both screens import, typed as
the
Languageunion.3.7 — the inbox watcher runs.
watchInboxwas built and tested with nocaller anywhere; 8.2's shell now holds it open for the life of a project window.
What the reviews changed
Wiring the watcher altered what opening a project does, and both reviewers
converged on it.
raw/arrives with agit clone, so a repository can shipraw/_inbox/x.pdf—and the watcher as first wired ingested it during its initial scan: a stranger's
bytes parsed by pdfjs or mammoth in the privileged main process, and the file
deleted out of the user's working tree, with nobody having clicked anything. The
desktop now passes
ingestExisting: false. What is already in the doorway islisted and left alone; what arrives while the window is open is still taken on
sight, which is what the doorway is actually for.
Left alone is not lost —
inboxWaiting/inboxDrainsay what is there and takeit when asked. That report is pulled rather than pushed, which is the other
half:
webContents.sendbefore the document has loaded is dropped with no queueand no error, so every outcome of the initial scan was being announced into the
void. Live arrivals stay pushed, buffered until
did-finish-load.Also closed:
readManifestnow checks the manifest's shape instead of castingit (a non-string
titlereached the screen as a React child and blanked thewindow, and this diff had just moved that render onto the primary view);
PageSourcesclears before fetching, so page A's provenance no longer sitsunder page B's title; an unreadable source is told apart from an absent one;
return started.close()instead ofvoid, which was escaping the chained.catch; the inbox listener no longer bumpsreloadKey, since 8.10's watcheralready reports and coalesces the same write.
Which plan
plans/open-wiki.md— tasks 3.7, 6.5, 8.12, with the notes updated to recordwhat was decided rather than what was hoped.
How it was verified
pnpm run typecheck,pnpm lint,pnpm format:check— clean.pnpm test— 1079 passing, 1 skipped (a symlink case that now reports theskip when the OS refuses to create the link, instead of silently returning
green as it did before).
pnpm test:coverageandscripts/ci/check-coverage.mjsper package — allabove the 76% floor (access 94.49%, audio 99.46%, cli 96.70%, mcp 100%,
desktop 92.93%).
npx @protonspy/scc validate— no findings.code-reviewandsecurity-reviewsubagents on the diff; everything aboveunder "what the reviews changed" is theirs.
Not verified: group 4's three manual audio checks remain outstanding — no
test in this repository has captured a frame or run ffmpeg, and nothing here
changes that. The new React components have no component tests either; this repo
has no such infrastructure, and coverage counts
src/**/*.tsonly.🤖 Generated with Claude Code
https://claude.ai/code/session_01X24yWBFHyEkjZV5k8hDN4i
Summary by CodeRabbit