Repository navigation
fix: resolve Windows runtime failures and desktop context menus - #725
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe pull request adds desktop context menus and persisted spellcheck settings. It also changes update-lock owner storage, rooted directory access, session deletion moves, workspace lookup, and embedded Lucide catalog parsing. ChangesDesktop context menus and spellcheck
Update-lock owner metadata
Rooted directory access and removal
Embedded icon catalog parsing
Workspace lookup for sessions
Session deletion and database moves
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
actor User
participant Electron as Electron webContents
participant ProductWindow
participant ContextMenu as productContextMenu
participant WindowState as window state store
User->>Electron: Right-click editable text or selection
Electron->>ProductWindow: Emit context-menu event and parameters
ProductWindow->>ContextMenu: Build menu from context and spellcheck session
ContextMenu-->>ProductWindow: Return menu template and settings callback
ProductWindow->>Electron: Display native menu
User->>Electron: Select a menu action
Electron->>ProductWindow: Invoke menu action callback
ProductWindow->>WindowState: Save changed spellcheck settings
Merge Risk: ⚪ Minimal · up to This change fixes several Windows runtime failures and adds desktop context menus and spellcheck settings. The review found no concrete merge-blocking risk, so it is reasonable to merge after the normal checks. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 30 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title accurately identifies the main change areas: Windows runtime fixes and desktop context menus. It is concise and specific enough for the changeset. Linked Issues check The reviewed head addresses the coding requirements of all seven active linked issues. [ #717] keeps the exclusive lock onupdate.lockand moves Windows owner metadata toupdate.lock.owner, with st…Out of Scope Changes check The changes remain within the linked issue scope. The added tests cover the affected Windows file, lock, session, icon, and desktop behaviors. Resolver interface updates support the [ #719] query chang…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 30 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
-
- Commit to this branch
- Create a new PR
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pinned, err := f.lease.pinOwnedFile(ctx, owner, path) | ||
| if err != nil { | ||
| return result, errors.Join(moveErr, fmt.Errorf("store: rebind database after directory move: %w", err)) |
There was a problem hiding this comment.
On Windows, canceling a deletion request after the directory rename can prevent rollback. MoveParentDirectory closes the held database file, then uses the request's ctx to reopen it and check its owner. If that check fails, f.pinned.file stays nil. The rollback in stageBoundSessionDelete calls the same move method, but validateActive rejects the closed file before restoring the directory.
The catalog row remains, while its history stays under the staged name until restart recovery. Keep the held database file usable after a committed move, and use a separate cleanup context for reopening and rollback.
This branch was successfully deployed
1 participant
Summary
This PR fixes the seven reports shown in the issue batch: Windows daemon startup, rooted directory creation, session removal, session-list latency, CRLF source-build icons, desktop context menus, and desktop spelling controls.
Closes #717
Closes #718
Closes #719
Closes #720
Closes #721
Closes #698
Closes #699
Root causes and changes
Windows daemon startup (#717)
The runtime mutation lock uses a mandatory Windows byte-range lock. Reading, truncating, or writing its owner metadata through a second file handle conflicts with the lock that the process already owns. Windows now stores the process identity in
update.lock.owner, following the existing daemon lock's.pidsidecar convention. The exclusive lock remains onupdate.lock; this does not bypass mutual exclusion. Unix keeps its existing in-file metadata. Release clears the corresponding owner payload, and acquisition continues to distinguish stale process identities.Windows rooted directory creation (#718)
The no-follow directory walk requested generic write access to every component, including the drive root and
Users. A standard user can write under their home without having write access to these ancestors. Existing ancestors now open with read access; only the final mutation target requests write access. Missing components are still created by the handle-relative native operation, and every component retains reparse-point rejection. The regression uses a real ACL that denies mutation on an ancestor while its existing child remains writable.Session inventory latency (#719)
Persisted session metadata projected the workspace root by calling full runtime
Resolve, which validates resource snapshots and scans skills/agents. That work was repeated for historical sessions even though the caller only needed the registration's root directory. Session queries now use the existing inexpensiveGetregistration operation. The session package's consumed resolver interface explicitly requires that operation, and all consumers/fakes are updated together. Session creation and resume still perform complete runtime resolution. No TTL cache, stale-root fallback, timeout increase, or resource-validation suppression is introduced.Windows session deletion (#720)
Deletion already quiesces stored readers and holds a database-family lease. However, the deletion capability then pins
events.dbwith an open file handle. Windows refuses a directory rename while a descendant file remains open, including this protective handle. The family owner now closes that descendant handle for the Windows rename while retaining its lease and the bound directory capability. It then reopens the database, verifies the exact file identity and owner, and advances the retained path. This covers staging, commit, rollback, and restart recovery. Final tombstone cleanup closes the database before the bound directory removal transaction. Native recovery testing also exposed a separate durability bug: move capabilities held read/delete access, while removing their contents callsFlushFileBuffers, which requires write access. Bound removal now acquires a mutation handle for the private candidate and verifies the same directory identity before deleting contents; recursive deletion opens nested directories for mutation as well. Rename-only handles retain their narrower rights, and durability errors remain fatal. Unix keeps the existing pinned-handle rename behavior.CRLF source-build icons (#721)
The embedded Lucide catalog was split on LF without removing CR, so a CRLF checkout produced different catalog keys. Catalog parsing now normalizes line terminators, and
.gitattributespins the embedded generated catalog to LF. Coverage compares the entire LF and CRLF catalogs and checkspalette, without modifying generated catalog contents.Desktop context menus (#698)
The product BrowserWindow now handles Electron's native context-menu event. Editable fields receive capability-aware Undo, Redo, Cut, Copy, Paste, and Select All roles. Text selections receive Copy, and links receive Copy link. The renderer continues to own its custom context menus.
Desktop spellchecking (#699)
Misspelled words expose dictionary suggestions and Add to dictionary. Editable fields include a Spelling submenu with an enable/disable checkbox. Windows and Linux offer independently selectable dictionaries, supporting simultaneous English and Portuguese. At least one dictionary stays selected; disabling is an explicit separate setting. macOS displays the OS-language ownership explanation and retains native language selection.
Optional spelling preferences are stored with existing window geometry/zoom in
desktop-window.jsonand restored at startup. Existing window-state files remain accepted. Languages are restored before the enabled flag: Electron 44.5.1SetSpellCheckerLanguagesenables the checker on Windows/Linux when the language list is nonempty, so applying the enabled flag first incorrectly reactivated a saved disabled preference. The packaged Linux relaunch assertion caught this and remains unchanged. This is a desktop-shell preference, so no daemon config, HTTP route, IPC bridge, or web settings surface is added.Compatibility and impact
docs/_memory/change-impact.md; operator desktop documentation and the affected QA scenarios are updated.Verification
Current head:
8b9efc5eaefa1835b18601e67d18f8cfcdf4e63a. CI run 38114169207 has passed all Go race shards, Go lint/build, native Windows, Darwin, SDK, preflight, desktop verification, runtime E2E, packaged desktop E2E, all four web E2E shards, and the frontend lane. Both aggregate verification gates passed; the complete workflow concluded successfully. React Doctor and Vercel checks also passed for this head.TestUpdateLockTestManagerQueriesUseWorkspaceRegistrationon Windows and the Linux session suitepalette.Native results: Windows job, packaged Linux desktop job, and daemon-containing race shard. The affected QA scenario files record scoped receipts; they do not claim new full manual provider/composer walks.
Before the operator moved all remaining gates to CI, focused local race suites and workspace create/resume integration passed. A packaged macOS context-menu walk verified the real clipboard, disabled spelling across relaunch, and custom-menu coexistence. Local full-gate attempts are not represented as a complete pass: one storage migration hit its deadline under concurrent load and passed unchanged in isolation; the next full attempt was stopped at the operator's request. No subsequent local gates or test suites were run.
Test preconditions and diagnostic retention
The Windows ACL fixture gives the writable child an explicit protected ACL before restricting its ancestor, because Windows propagates changes to inherited child ACLs. It verifies both the denied ancestor and writable child before checking the nested write. The interrupted-deletion fixture releases both session and attachment capabilities before constructing its replacement manager, matching process death. Existing deletion, ownership, content, and tombstone assertions remain intact.
The Windows workflow previously ran filtered terminal/private-file/Gateway suites rather than the complete Go inventory. It now includes the owning startup, ACL, metadata-query, icon, bound-removal, and deletion/rollback/recovery cases, exercising Windows semantics directly.
A previous Linux daemon run reported a race, but its summarized log omitted the stacks and failed jobs did not retain their raw JSON. The existing artifact upload now runs on failure too. The current daemon execution was fresh (222.142 seconds, no cached result), with 2,135 test/subtest pass events and no race diagnostic in its retained JSON. This does not establish a causal fix for the earlier race, whose source remains unknown. No race suppression, assertion weakening, timeout increase, or automatic retry was added.
Summary by CodeRabbit