fix: the four dead controls, the empty wiki, and failures reported where they happen - #29
Merged
Merged
Conversation
… happen Group 1 of plans/desktop-ui.md — the part of the desktop UI plan that ships on its own, because until it lands there is no way to create a page in the packaged application. - `Ask.tsx` asks the shell's questions through a native `<dialog>` opened with `showModal`, so the focus trap, Escape and the return of focus are the platform's rather than hand-rolled. Every `globalThis.prompt` / `globalThis.confirm` call site is converted: new page, rename, delete, the record occasion, and the source retitle. Electron implements none of the three, which is why all four controls did nothing at all. - The record occasion keeps 4.16's fallback: not answering the box records under the timestamp rather than refusing, and the button says so instead of saying "Cancel" and recording anyway. - A lint rule bans `prompt`, `confirm` and `alert` under `src/renderer`, in both their bare and `globalThis.`/`window.`/`self.` forms. The test runs the repository's real config, because a rule that does not fire during `pnpm lint` is worth nothing. - `scaffold()` seeds `wiki/index.md` and `wiki/changelog.md`. The skills tell the agent to link a new page from the index and the checks read both, and neither file existed until something happened to write one. `log.md` stays absent: it is a log, and an empty one is noise. - An empty wiki explains itself — this window does not write pages, the agent does — and shows the path to open in a harness. That is the central fact about the product and it was said nowhere. - Failures are reported where they happened rather than in one line above every pane: `notices.ts` keeps one notice per place, so a failed drop no longer erases a failed page load, and a rename's "repointed the links on …" stops arriving through the error channel. Verified: `pnpm test:coverage` (1119 tests, the 76% floor per package), `pnpm lint`, `pnpm typecheck`, `npx @protonspy/scc validate` — all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBt5LRVJd5Z7nagJGRwvSC
- **Enter submitted the wrong button.** Both dialog buttons were submit buttons with cancel first in tree order, and implicit submission goes through the form's default button — the first submit button — not the focused one. Typing a slug and pressing Enter closed the box with cancel's empty `returnValue` and threw the answer away, in every prompt this plan adds. The buttons now come from `buttonsFor`, which is where the rule is written down and what the new tests assert: exactly one button submits, and it is the one that answers. - **`seedWiki` wrote without the project's own confinement.** A dangling symlink planted at `wiki/index.md` is exactly the case `existsSync` answers "no" to, and the write then followed it out of the project. It now goes through `assertWithin` like every other writer in the package, and writes with `wx` — `O_CREAT | O_EXCL`, which the kernel refuses on a symlink of any kind, so the check and the write cannot disagree. - A question left pending when the component that asked it unmounts is settled rather than left hanging. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBt5LRVJd5Z7nagJGRwvSC
… O_EXCL The second security round was right: `O_CREAT | O_EXCL` refusing a symlink is a POSIX rule with no Win32 equivalent, and Windows is what this ships on. The guard is now `lstat`, which answers about the name rather than about what the name points at — anything there at all, symlink or directory or file, and the seed is not written. That holds identically on every platform. `wx` stays for the sliver between the two calls. Both regression tests also scaffolded into a directory holding only `wiki/`, which `isEmptyOrProject` refuses — they would have failed in CI, where the symlink one actually runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBt5LRVJd5Z7nagJGRwvSC
📝 WalkthroughWalkthroughThe desktop renderer now uses native dialogs and location-scoped notices. Scaffolding creates initial wiki index and changelog files safely. ESLint prevents renderer use of browser blocking dialogs. ChangesDesktop renderer UI
Wiki scaffolding
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant useDialogs
participant Ask
participant HTMLDialogElement
App->>useDialogs: ask(question)
useDialogs->>Ask: render question
Ask->>HTMLDialogElement: showModal()
HTMLDialogElement-->>Ask: submit or cancel returnValue
Ask-->>useDialogs: resolve answer
useDialogs-->>App: return answer
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
apps/desktop/src/renderer/Sources.tsx (1)
41-65: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRender
{dialog}regardless of theerror/rows/empty branch.
{dialog}is rendered only in the final return branch, at Line 62. If a retitle dialog is pending (anask()promise is awaiting an answer inSourceItem) and a subsequentload()triggered byreloadKeyfails or returns an empty list,Sourcesre-renders through theerror(Line 41),!rows(Line 42), orrows.length === 0(Line 43-47) branch instead. None of those branches include{dialog}.
Sourcesdoes not unmount in that case, so the unmount-settle cleanup inuseDialogs()(Ask.tsx, Line 51) never runs. The open<Ask>element disappears from the DOM without itsonClosefiring, and the pendingask()promise never resolves. Theretitle()call that awaits it inSourceItemhangs indefinitely.Render
{dialog}unconditionally so a pending question always stays visible and answerable, independent of the surrounding content state.🔧 Proposed fix
- if (error) return <p className="error">{error}</p>; - if (!rows) return <p className="empty">Reading the sources…</p>; - if (rows.length === 0) { - return ( - <p className="empty">No sources yet. Drop a file on this window, or record something.</p> - ); - } - - return ( - <> - <ul className="list"> - {rows.map((row) => ( - <SourceItem - key={row.id} - row={row} - ask={ask} - onOpenPage={onOpenPage} - onChanged={() => void load()} - /> - ))} - </ul> - {dialog} - </> - ); + return ( + <> + {error ? ( + <p className="error">{error}</p> + ) : !rows ? ( + <p className="empty">Reading the sources…</p> + ) : rows.length === 0 ? ( + <p className="empty">No sources yet. Drop a file on this window, or record something.</p> + ) : ( + <ul className="list"> + {rows.map((row) => ( + <SourceItem + key={row.id} + row={row} + ask={ask} + onOpenPage={onOpenPage} + onChanged={() => void load()} + /> + ))} + </ul> + )} + {dialog} + </> + );🤖 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/Sources.tsx` around lines 41 - 65, Update the Sources component’s early error, loading, and empty-state returns so they also render the dialog from useDialogs. Ensure dialog remains mounted in every render branch, including the final list branch, so pending ask() promises remain answerable.
🤖 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/renderer/dialogs.ts`:
- Around line 12-14: Update the doc comment in the dialogs module to reference
Ask.tsx instead of the stale Dialogs.tsx name, leaving the rest of the comment
unchanged.
---
Outside diff comments:
In `@apps/desktop/src/renderer/Sources.tsx`:
- Around line 41-65: Update the Sources component’s early error, loading, and
empty-state returns so they also render the dialog from useDialogs. Ensure
dialog remains mounted in every render branch, including the final list branch,
so pending ask() promises remain answerable.
🪄 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: 51bea4a8-0584-424c-af0b-e9ad7acbe53e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/desktop/package.jsonapps/desktop/src/renderer/App.tsxapps/desktop/src/renderer/Ask.tsxapps/desktop/src/renderer/Sources.tsxapps/desktop/src/renderer/dialogs.tsapps/desktop/src/renderer/notices.tsapps/desktop/src/renderer/tokens.cssapps/desktop/tests/dialogs.spec.tsapps/desktop/tests/no-dead-dialogs.spec.tsapps/desktop/tests/notices.spec.tseslint.config.jspackages/access/src/scaffold.tspackages/access/src/store/index.tspackages/access/tests/scaffold.spec.tsplans/desktop-ui.md
Comment on lines
+12
to
+14
| * The components in `Dialogs.tsx` arrange these and decide nothing. What is | ||
| * here is the part a test can reach: what each question says, which button | ||
| * says what, and what closing the box means. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix stale file reference in doc comment.
Line 12 refers to Dialogs.tsx. The component that consumes these questions is Ask.tsx. Update the comment to name the correct file, so a reader who looks for Dialogs.tsx does not conclude it is missing.
📝 Proposed fix
- * The components in `Dialogs.tsx` arrange these and decide nothing. What is
+ * The components in `Ask.tsx` arrange these and decide nothing. What is📝 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.
Suggested change
| * The components in `Dialogs.tsx` arrange these and decide nothing. What is | |
| * here is the part a test can reach: what each question says, which button | |
| * says what, and what closing the box means. | |
| * The components in `Ask.tsx` arrange these and decide nothing. What is | |
| * here is the part a test can reach: what each question says, which button | |
| * says what, and what closing the box means. |
🤖 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/dialogs.ts` around lines 12 - 14, Update the doc
comment in the dialogs module to reference Ask.tsx instead of the stale
Dialogs.tsx name, leaving the rest of the comment unchanged.
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Group 1 of
plans/desktop-ui.md— the group that plan says ships on its own, because until it lands there is no way to create a page in the packaged application. Tasks 1.1 to 1.5, all ticked in the plan.What changed
1.1 — the dialogs.
Ask.tsxasks the shell's questions through a native<dialog>opened withshowModal(), so the focus trap, Escape, and returning focus to whatever held it before are the platform's rather than hand-rolled. EveryglobalThis.prompt/globalThis.confirmcall site is converted: new page, rename, delete, the recording occasion, and the source retitle. Electron implements none of the three, which is why all four controls did nothing at all — and produced no error either, since the throw was outside thetry.The record occasion keeps 4.16's fallback: not answering the box records under the timestamp rather than refusing, and the button says so — Record without a name — instead of saying "Cancel" and recording anyway.
1.2 — the lint rule.
prompt,confirmandalertare banned undersrc/renderer, in their bare form and throughglobalThis./window./self.. The test runs the repository's real config rather than a copy of the rule, because every way of getting this wrong — a glob missing.tsx, the property form going unnoticed, the block sitting afterprettier— looks like a working config from the inside.1.3 — the wiki's own pages.
scaffold()seedswiki/index.mdandwiki/changelog.md. The skills tell the agent to link a new page from the index and the checks read both, and neither file existed until something happened to write one. Neither seed contains a wikilink, becausecheckRecordsreads every[[…]]in the changelog as a page that should exist — a test asserts a brand-new project has nothing at all forow checkto report.log.mdstays absent: it is a log, and an empty one is noise.1.4 — an empty wiki explains itself. This wiki has no pages yet reads as a defect and invites the conclusion that the application is broken. It now says the thing that is true and is said nowhere else in the product — this window does not write pages, your agent does — and shows the project path to open in a harness.
1.5 — failures where they happened.
notices.tskeeps one notice per place instead of one string for the whole window. A failed drop no longer erases a failed page load; a failure inside a pane is reported inside it; a source retitle that used to reject into nothing at all now says why on its own row; and a rename's "repointed the links on …" stops arriving through the error channel in the same red box a failed rename used.How it was verified
pnpm test:coverage(the whole workspace, 76% floor per package, exit 0) ·pnpm lint·pnpm run typecheck·npx @protonspy/scc validate— no findings.code-reviewandsecurity-reviewwere run on the diff, then re-run after the fixes. Three findings, all closed:buttonsFor, where the rule is written down and where the tests can pin it.seedWikiwrote withexistsSync+writeFileSync, which follows a dangling symlink planted at a seed path straight out of the project. Fixed withassertWithinplus anlstatguard:O_CREAT | O_EXCLrefusing a symlink is a POSIX rule with no Win32 equivalent, and Windows is what this ships on, so the guard cannot rest on it.One thing worth a second opinion
Escape on the record occasion dialog starts a recording. That is the plan's own wording for 1.1 — "cancelling names the recording by timestamp rather than refusing to record" — and 4.16's rule that capture is worth more than a naming rule. The cancel button says so, but Escape does not get to say anything. The persistent recording indicator makes an unintended one visible immediately, which is why I implemented it as written; if you would rather Escape meant "don't record", it is one label and one branch.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HBt5LRVJd5Z7nagJGRwvSC
Summary by CodeRabbit
New Features
Improvements
Tests