Add workspace reset/backup recovery and robust import/export handling for Raycast - #133
Conversation
Serialize workspace mutations so overlapping commands cannot lose updates, and add confirmed reset-all with a durable backup restore path matching CmdPal. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesRaycast storage mutations now serialize through a write queue and support durable reset-all backup and restoration. Import parsing accepts CmdPal layout envelopes, suggestions preserve task types, transfer dialogs become asynchronous, and macOS launches support tabs or separate windows. The hub, security flows, CI, and documentation reflect these workflows. Storage and recovery
Import, suggestions, and forms
Transfer and macOS launch behavior
Supporting delivery
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="QuickShell.Raycast/src/lib/storage.ts" line_range="167-176" />
<code_context>
+ }
+
+ /** Restores the durable reset-all backup into the live store. */
+ async restoreFromBackup(): Promise<StorageTransferResult> {
+ return this.withWriteLock(async () => {
+ await this.flushRecentWrites();
+ const raw = await this.adapter.getItem(BACKUP_STORAGE_KEY);
+ if (!raw) {
+ return { success: true, message: "No workspace backup found." };
+ }
+
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(raw);
+ } catch {
+ throw new Error("Workspace backup is not valid JSON.");
+ }
+
+ const restored = migrateStoredData(parsed);
+ await this.save(restored, { preserveSecurity: false, allowSubmittedSecurity: true });
+ return {
</code_context>
<issue_to_address>
**issue:** On invalid backup JSON, consider clearing the backup key or degrading gracefully to avoid persistent hard failures.
If the backup value is corrupted, `restoreFromBackup` throws `"Workspace backup is not valid JSON."` but leaves the bad value under `BACKUP_STORAGE_KEY`. Every subsequent call will keep failing with no way to recover or clear the backup via this flow. Consider treating this as a one-time error: catch the parse failure, clear the backup key (or replace it with a known-good empty snapshot), and return a non-throwing `StorageTransferResult` indicating the backup was unusable and has been discarded.
</issue_to_address>
### Comment 2
<location path="QuickShell.Raycast/src/__tests__/storage.test.ts" line_range="365-374" />
<code_context>
expect(loaded[0].lastUsedUtc).toBe(usedAt.toISOString());
});
+
+ it("serializes overlapping mutations so the later write cannot clobber the earlier one", async () => {
+ const storage = new QuickShellStorage(createMemoryStorageAdapter());
+ const first = createWorkspace(createStableId(), "Alpha");
+ const second = createWorkspace(createStableId(), "Beta");
+
+ await Promise.all([storage.upsertWorkspace(first), storage.upsertWorkspace(second)]);
+
+ const loaded = await storage.getWorkspaces();
+ expect(loaded).toHaveLength(2);
+ expect(loaded.map((workspace) => workspace.name).sort()).toEqual(["Alpha", "Beta"]);
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a concurrency test where two overlapping mutations touch the *same* workspace to better validate the write lock behavior.
Right now the test only covers concurrent `upsertWorkspace` calls to different workspaces. To fully exercise `withWriteLock`, add a scenario where overlapping mutations target the same workspace (e.g., two `upsertWorkspace` calls updating different fields, or `upsertWorkspace` plus `setFavorite` / `markWorkspaceUsed`). That will verify the lock correctly prevents lost updates when multiple commands race on a single record, matching the real-world regression this is meant to prevent.
</issue_to_address>
### Comment 3
<location path="QuickShell.Raycast/src/__tests__/storage.test.ts" line_range="385-394" />
<code_context>
+ expect(await storage.hasBackup()).toBe(false);
+ });
+
+ it("resetAll clears workspaces, keeps undo, and writes a durable backup", async () => {
+ const adapter = createMemoryStorageAdapter();
+ const storage = new QuickShellStorage(adapter);
+ const id = createStableId();
+ await storage.upsertWorkspace(createWorkspace(id, "Alpha"));
+
+ const result = await storage.resetAll();
+ expect(result.success).toBe(true);
+ expect(await storage.getWorkspaces()).toHaveLength(0);
+ expect(await storage.hasBackup()).toBe(true);
+ expect(storage.canUndo()).toBe(true);
+
+ await storage.undo();
+ expect(await storage.getWorkspaces()).toHaveLength(1);
+ expect((await storage.getWorkspaces())[0].name).toBe("Alpha");
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions that `resetAll` preserves settings and that the backup snapshot contents look as expected.
Since `resetAll` also preserves `settings` and writes a durable backup under `BACKUP_STORAGE_KEY`, it would be helpful for this test to: (1) initialize a non-default `settings` value and assert it is unchanged after `resetAll`, and (2) read the adapter’s `BACKUP_STORAGE_KEY` entry and assert that it deserializes into a structure containing the original workspace (and its name), rather than only checking `hasBackup()`.
Suggested implementation:
```typescript
import { QuickShellStorage, BACKUP_STORAGE_KEY } from "../storage";
```
```typescript
it("resetAll clears workspaces, keeps undo, preserves settings, and writes a durable backup", async () => {
const adapter = createMemoryStorageAdapter();
const storage = new QuickShellStorage(adapter);
const id = createStableId();
const workspace = createWorkspace(id, "Alpha");
// Initialize non-default settings so we can assert they are preserved.
const initialSettings = {
// Use a clearly non-default value so changes are easy to detect.
preferredShellId: "non-default-shell",
} as any;
await storage.setSettings(initialSettings);
await storage.upsertWorkspace(workspace);
const result = await storage.resetAll();
expect(result.success).toBe(true);
// Workspaces are cleared.
expect(await storage.getWorkspaces()).toHaveLength(0);
// Settings are preserved.
const settingsAfterReset = await storage.getSettings();
expect(settingsAfterReset).toEqual(initialSettings);
// Backup exists and is durable.
expect(await storage.hasBackup()).toBe(true);
expect(storage.canUndo()).toBe(true);
// Backup snapshot contents look as expected.
const backupRaw = await adapter.getItem(BACKUP_STORAGE_KEY);
expect(backupRaw).not.toBeNull();
const backup = JSON.parse(backupRaw as string);
// The backup should contain the original workspace with its name.
expect(Array.isArray(backup.workspaces)).toBe(true);
expect(backup.workspaces).toHaveLength(1);
expect(backup.workspaces[0].id).toBe(workspace.id);
expect(backup.workspaces[0].name).toBe("Alpha");
// Undo restores the original workspace.
await storage.undo();
const restoredWorkspaces = await storage.getWorkspaces();
expect(restoredWorkspaces).toHaveLength(1);
expect(restoredWorkspaces[0].name).toBe("Alpha");
});
```
1. Ensure the import path for `BACKUP_STORAGE_KEY` (`../storage` in the edit above) matches the actual location of the constant in your codebase. If `BACKUP_STORAGE_KEY` lives elsewhere (e.g. `"../storage/QuickShellStorage"`), adjust the import accordingly.
2. The test assumes `QuickShellStorage` exposes `setSettings` and `getSettings`. If your API uses different names (e.g. `updateSettings`, `loadSettings`, or a property), update the calls and expectations to match the real methods.
3. The test expects `createMemoryStorageAdapter()` to expose an async `getItem(key: string)` method compatible with `BACKUP_STORAGE_KEY`. If the adapter API differs (e.g. `adapter.storage.get(key)` or synchronous access), adapt the backup reading code to the correct API.
4. The structure of the backup snapshot is assumed to be `{ workspaces: Workspace[]; ... }`. If your actual backup schema is different (e.g. nested under `snapshot.workspaces` or includes settings), adjust the JSON shape assertions accordingly while still verifying that the original workspace (and its name) is present in the backup.
5. The settings object in the test uses a `preferredShellId` field as a clearly non-default value. Replace this with a realistic non-default settings shape from your codebase (e.g. matching your `Settings` type), so TypeScript type-checking remains correct.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d42a762e72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (4) 🔗 Fix PR: #134 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 4 fixed
|
Replace shared writeDepth bypass with unlocked nested helpers so concurrent mutations always queue, discard corrupt backups gracefully, strengthen storage tests, and update persistence host docs. Co-authored-by: Anthony Thompson <github@trackdub.com>
Comment triage (merge-readiness pass)Fixed
Declined (with rationale)
CI re-running on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@QuickShell.Raycast/src/__tests__/storage.test.ts`:
- Around line 393-430: Strengthen the test around
QuickShellStorage.upsertWorkspace and the adapter’s setItem ordering so it
verifies serialization rather than shared cache state: track when the first
persisted write completes, assert the second write does not start until then,
and inspect the final persisted quickshell-data blob to confirm it contains both
workspaces. Avoid using getWorkspaces as the sole assertion, since saveUnlocked
updates the in-memory cache before persistence.
In `@QuickShell.Raycast/src/lib/storage.ts`:
- Around line 663-683: Update withWriteLock to track whether the lock is
currently held and immediately throw a clear error when a nested invocation
occurs, before waiting on the queue. Set the held state after acquiring the
previous operation, and reliably clear it in finally alongside release; keep
existing queue serialization and operation behavior unchanged.
- Around line 185-186: Update restoreFromBackup() so restored data does not
rehydrate workspaceSecurity from the backup blob: save the migrated data with
untrusted security semantics like import/reset, or explicitly clear its security
map before calling saveUnlocked().
- Around line 257-266: Update the save method to flush pending recent writes via
flushRecentWritesUnlocked() before invoking saveUnlocked(data, options) within
the withWriteLock callback. Preserve the existing save options and locking
behavior so pending markWorkspaceUsed updates are applied before the snapshot is
saved.
In `@QuickShell.Raycast/src/open-workspace.tsx`:
- Around line 611-617: The reset and restore flows report no-op or discarded
outcomes as successful actions. In QuickShell.Raycast/src/lib/storage.ts:35-39,
replace the literal-success StorageTransferResult with an outcome discriminator
and assign the appropriate value at every return in resetAll() and
restoreFromBackup(). In QuickShell.Raycast/src/open-workspace.tsx:611-617 and
:631-637, select the toast style and title from result.outcome so only actual
reset/restore operations use success messaging, while no-op or discarded results
use neutral or failure messaging.
- Around line 809-815: Update the conditional Restore Backup action in the
workspace actions list to use the same destructive action style as Reset All
Workspaces, while preserving its existing title, icon, and onAction handler.
- Around line 597-600: Update handleResetAll so it requires loaded workspace
data before showing or executing the destructive reset, rather than treating
undefined data as zero workspaces. Use the existing data state from usePromise
to guard the action, preserving the count-based wording and preventing
storage.resetAll() from running when the workspace list is not loaded.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bcc39543-29e0-4c57-81c7-1556128abfcb
📒 Files selected for processing (7)
QuickShell.Raycast/src/__tests__/storage.test.tsQuickShell.Raycast/src/lib/schema.tsQuickShell.Raycast/src/lib/storage.tsQuickShell.Raycast/src/open-workspace.tsxdocs/architecture/hosts.mddocs/architecture/parity-matrix.mddocs/architecture/persistence.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Analyze C# with CodeQL
- GitHub Check: .NET build and test
- GitHub Check: Performance harness (artifacts)
🧰 Additional context used
📓 Path-based instructions (1)
QuickShell.Raycast/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain Raycast behavior in parity with Core launch behavior and use the project's Vitest test conventions.
Files:
QuickShell.Raycast/src/lib/schema.tsQuickShell.Raycast/src/__tests__/storage.test.tsQuickShell.Raycast/src/open-workspace.tsxQuickShell.Raycast/src/lib/storage.ts
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
QuickShellStoragecurrently uses an in-memory cache over Raycast storage, debouncesmarkWorkspaceUsed()with a 500 ms timer, caps undo history at 25 snapshots, and performs structural mutations viaflushRecentWrites()→load()→ mutate →save()for upsert/delete/duplicate, pin/layout, branch-target, and trust changes.open-workspace.tsxcurrently rehydratesworkspaces,settings,layoutEntries, andbranchTargetstogether, loads security separately when trust is enabled, and the hub actions expose only Export/Import plus Undo/Redo in the Transfer/History sections.- Repo docs describe Raycast as a parallel LocalStorage-backed host that does not share the desktop
%LOCALAPPDATA%\QuickShell\JSON stores, and the parity matrix marks worktree branch targets as Raycast-local in the blob rather than the desktopworktree-branch-targets.json. - The checked-in
storage.test.tsbase file already covers persistence,markWorkspaceUsed, settings updates, undo after delete, duplicate trust handling, import/replace security, and schema corruption protection. - DeepWiki couldn’t provide repo-structure context for
tonythethompson/QuickShellbecause the repository isn’t indexed there.
🔇 Additional comments (12)
QuickShell.Raycast/src/lib/storage.ts (6)
70-116: LGTM!
142-163: LGTM!
308-350: LGTM!
363-446: LGTM!Also applies to: 460-488, 497-619
621-632: LGTM!
646-661: LGTM!QuickShell.Raycast/src/__tests__/storage.test.ts (2)
5-5: LGTM!Also applies to: 365-391
432-501: LGTM!QuickShell.Raycast/src/lib/schema.ts (1)
5-6: LGTM!QuickShell.Raycast/src/open-workspace.tsx (1)
81-81: LGTM!Also applies to: 126-132, 146-146
docs/architecture/hosts.md (1)
67-67: LGTM!docs/architecture/persistence.md (1)
144-144: LGTM!
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Add ALS nested-lock detection, flush before public save, outcome-aware toasts, load-state guard for reset confirm, stronger write-order test, and destructive styling for Restore Backup. Co-authored-by: Anthony Thompson <github@trackdub.com>
Follow-up triage (CodeRabbit on
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
QuickShell.Raycast/src/lib/storage.ts (2)
192-193: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve current settings during backup restore.
The backup contains the full cache, so Line 193 restores its old
settingstoo. Changing settings after reset and then restoring silently reverts them, despite this workflow being documented as preserving settings and the UI promising to replace only the workspace list.Proposed fix
const restored = migrateStoredData(parsed); + const current = await this.load(); + restored.settings = { ...current.settings }; await this.saveUnlocked(restored, { preserveSecurity: false, allowSubmittedSecurity: true });🤖 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 `@QuickShell.Raycast/src/lib/storage.ts` around lines 192 - 193, Update the backup restore flow around migrateStoredData and saveUnlocked so restored data replaces only the workspace list while preserving the current settings. Capture or reuse the existing settings before restoration, then merge them into the migrated backup data before calling saveUnlocked, without altering the documented reset behavior.
632-642: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMove
scheduleRecentWriteFlush()out ofwithWriteLock()inmarkWorkspaceUsed().AsyncLocalStorage.run()context carries into the timer callback, soflushRecentWrites()can hit the nested-lock guard and leaverecentWriteDirtyset without persistinglastUsedUtc. Schedule the debounce after the lock releases, and add a Vitest regression for the idle debounce 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 `@QuickShell.Raycast/src/lib/storage.ts` around lines 632 - 642, The markWorkspaceUsed method schedules the recent-write debounce while still inside withWriteLock, allowing the timer callback to inherit the lock context and skip persistence. Keep the workspace update and recentWriteDirty assignment inside the lock, move scheduleRecentWriteFlush() to run after withWriteLock completes, and add a Vitest regression covering persistence through the idle debounce path.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@QuickShell.Raycast/src/lib/storage.ts`:
- Around line 192-193: Update the backup restore flow around migrateStoredData
and saveUnlocked so restored data replaces only the workspace list while
preserving the current settings. Capture or reuse the existing settings before
restoration, then merge them into the migrated backup data before calling
saveUnlocked, without altering the documented reset behavior.
- Around line 632-642: The markWorkspaceUsed method schedules the recent-write
debounce while still inside withWriteLock, allowing the timer callback to
inherit the lock context and skip persistence. Keep the workspace update and
recentWriteDirty assignment inside the lock, move scheduleRecentWriteFlush() to
run after withWriteLock completes, and add a Vitest regression covering
persistence through the idle debounce path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 57358b53-d29c-4907-a99b-f8059e024a76
📒 Files selected for processing (3)
QuickShell.Raycast/src/__tests__/storage.test.tsQuickShell.Raycast/src/lib/storage.tsQuickShell.Raycast/src/open-workspace.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Analyze Raycast TypeScript with CodeQL
- GitHub Check: Analyze C# with CodeQL
- GitHub Check: Raycast lint, test, and build
- GitHub Check: Performance harness (artifacts)
- GitHub Check: .NET build and test
🧰 Additional context used
📓 Path-based instructions (1)
QuickShell.Raycast/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain Raycast behavior in parity with Core launch behavior and use the project's Vitest test conventions.
Files:
QuickShell.Raycast/src/open-workspace.tsxQuickShell.Raycast/src/lib/storage.tsQuickShell.Raycast/src/__tests__/storage.test.ts
🔍 Remote MCP GitHub Copilot
Additional review context
- Issue
#37requires a management action, confirmation, backup creation/preservation, and recovery after reset. - Issue
#47specifically targets lost updates from concurrent load-modify-save operations; serialization or an explicit retry/error path is required. - The repository’s baseline
QuickShellStorageperforms mutations throughflushRecentWrites()→load()→save(), confirming the lost-update risk addressed by this PR. - The baseline storage adapter persists arbitrary keys via
getItem/setItem, so a separate durable backup key is supported by the existing abstraction. - Related PR
#116changesmoveFavoriteto support top/bottom movement and changes boundary behavior to returnnull; this overlaps the same storage method and should be checked for merge/API compatibility.
🔇 Additional comments (3)
QuickShell.Raycast/src/lib/storage.ts (1)
1-1: LGTM!Also applies to: 36-121, 133-168, 265-630
QuickShell.Raycast/src/__tests__/storage.test.ts (1)
403-417: LGTM!Also applies to: 431-432, 443-443, 463-463, 494-494, 506-511
QuickShell.Raycast/src/open-workspace.tsx (1)
81-81: LGTM!Also applies to: 126-146, 597-658, 829-842
Expand the persistence tour and host parity docs so reset-all, restart-safe restore, backup key ownership, and mutation serialization are explicit as-built behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Accept Core layout envelopes (`entries`, including Workspace wrappers) so CmdPal exports import correctly, and make the Windows file picker async with a startup toast so the UI does not freeze during PowerShell cold start. Co-authored-by: Cursor <cursoragent@cursor.com>
…sing backup security to trusted
WinForms and osascript steal the foreground; activate Raycast again when the picker closes so the Quick Shell screen resurfaces for toasts and confirmations. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep CmdPal layout rows aligned when duplicates are skipped, hide transfer toasts on cancel, narrow focus-restore error handling, and preserve current settings when restoring a reset backup. Co-authored-by: Cursor <cursoragent@cursor.com>
Follow-up triage (open Qodo / CodeRabbit threads)Fixed (
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
QuickShell.Raycast/src/lib/storage.ts (1)
794-803: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFailed debounced flush is marked dirty but never rescheduled.
scheduleRecentWriteFlush's timer callback catches aflushRecentWrites()failure (e.g. a lock timeout while another writer is stuck) and setsrecentWriteDirty = true, but doesn't callscheduleRecentWriteFlush()again. Unless anothermarkWorkspaceUsed()happens to fire later, that recency update is silently stranded in memory and lost if the process exits.🛡️ Proposed fix
this.recentWriteTimer = setTimeout(() => { void this.flushRecentWrites().catch(() => { this.recentWriteDirty = true; + this.scheduleRecentWriteFlush(); }); }, RECENT_WRITE_DEBOUNCE_MS);🤖 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 `@QuickShell.Raycast/src/lib/storage.ts` around lines 794 - 803, Update scheduleRecentWriteFlush so the flushRecentWrites catch handler preserves recentWriteDirty and schedules another debounced flush by invoking scheduleRecentWriteFlush again. Ensure the retry occurs after the failed attempt while retaining the existing timer cleanup and debounce behavior.
♻️ Duplicate comments (1)
QuickShell.Raycast/src/open-workspace.tsx (1)
673-685: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
resetAll"noop" outcome still shows a Success-styled toast — same complaint as before, only half-fixed.The title now reflects
outcome === "noop", butstyleis still hard-coded toToast.Style.Success.handleRestoreBackupright below correctly derives style from outcome (result.outcome === "restored" ? Success : Failure); apply the same here so "nothing to reset" doesn't read as a completed destructive action.🛡️ Proposed fix
const isNoop = result.outcome === "noop"; await showToast({ - style: Toast.Style.Success, + style: isNoop ? Toast.Style.Failure : Toast.Style.Success, title: isNoop ? "Nothing to reset" : "Workspaces reset", message: result.message, });🤖 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 `@QuickShell.Raycast/src/open-workspace.tsx` around lines 673 - 685, Update the toast configuration in the resetAll handler to derive style from result.outcome, using Success only when workspaces were actually reset and Failure for the "noop" outcome. Keep the existing title, message, and error handling unchanged, following the outcome-based pattern in handleRestoreBackup.
🤖 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 `@QuickShell.Raycast/src/__tests__/storage.test.ts`:
- Around line 591-615: The existing test covers lock timeout but not the
re-entrancy guard. Add a regression test near the `withWriteLock` timeout test
that triggers a mutator invoking `this.save()` while an operation already holds
the write lock, then assert the promise rejects with the “Nested
QuickShellStorage write lock” error. Ensure the test uses the existing
storage/workspace setup and does not rely on hanging timers.
In `@QuickShell.Raycast/src/__tests__/workspace-transfer-files.test.ts`:
- Around line 34-56: The runtime fallback behavior of
pickWindowsTransferJsonPath is untested. Mock node:child_process using the
existing Vitest conventions and add a case where the initial pwsh invocation
rejects with a non-ENOENT error containing stdout, then verify the valid
selection is preserved while the powershell.exe fallback and error-swallowing
path are exercised.
In `@QuickShell.Raycast/src/lib/import-export.ts`:
- Around line 125-129: Update the workspace import logic around rawId,
workspaceId, and the workspaces/layoutEntries pushes to track IDs already
assigned and generate a fresh stable ID whenever a valid source ID repeats,
ensuring every layout workspaceId is unique and existing remapping preserves
each row’s workspace. Add a regression test covering duplicate source IDs and
verifying both imported workspaces receive distinct IDs with correctly
associated layout rows.
In `@QuickShell.Raycast/src/lib/workspace-transfer-files.ts`:
- Around line 100-111: Add a diagnostic console.error in the catch block
handling the PowerShell invocation, immediately before returning null for
non-ENOENT errors. Include the caught error and relevant context so real script
or environment failures are distinguishable from user cancellation, while
preserving the existing return behavior and ENOENT fallback.
- Around line 54-64: In the dialog command sequence returned by the workspace
transfer helper, emit the selected path before invoking
reactivateRaycastPowerShellSnippet(). Preserve the existing conditional
selection behavior, then run the focus-restoration step afterward so a valid
path is written even if reactivation fails.
In `@QuickShell.Raycast/src/open-workspace.tsx`:
- Around line 466-503: Update handleOpenCompanions to wrap storage and
post-launch operations in the same try/catch pattern used by handleOpenFolder,
handleOpenUrl, and handleTrust. On any thrown error from
storage.getStoredWorkspace or runPostLaunchActions, invoke showStorageFailure
and prevent the error from becoming an unhandled rejection, while preserving the
existing success and expected-failure toasts.
- Around line 473-477: Remove the unsupported companionSelection option from the
authorizePostLaunchEffects call, unless companion selection is intentionally
required; in that case, add it to the function’s options type and implementation
in authorizePostLaunchEffects within security.ts.
---
Outside diff comments:
In `@QuickShell.Raycast/src/lib/storage.ts`:
- Around line 794-803: Update scheduleRecentWriteFlush so the flushRecentWrites
catch handler preserves recentWriteDirty and schedules another debounced flush
by invoking scheduleRecentWriteFlush again. Ensure the retry occurs after the
failed attempt while retaining the existing timer cleanup and debounce behavior.
---
Duplicate comments:
In `@QuickShell.Raycast/src/open-workspace.tsx`:
- Around line 673-685: Update the toast configuration in the resetAll handler to
derive style from result.outcome, using Success only when workspaces were
actually reset and Failure for the "noop" outcome. Keep the existing title,
message, and error handling unchanged, following the outcome-based pattern in
handleRestoreBackup.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 97ad60a6-b767-4c62-bd45-0b8bc1b3f284
📒 Files selected for processing (11)
QuickShell.Raycast/src/__tests__/import-export.test.tsQuickShell.Raycast/src/__tests__/storage.test.tsQuickShell.Raycast/src/__tests__/workspace-transfer-files.test.tsQuickShell.Raycast/src/lib/import-export.tsQuickShell.Raycast/src/lib/migration.tsQuickShell.Raycast/src/lib/storage.tsQuickShell.Raycast/src/lib/workspace-transfer-files.tsQuickShell.Raycast/src/open-workspace.tsxdocs/architecture/hosts.mddocs/architecture/parity-matrix.mddocs/architecture/persistence.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Analyze C# with CodeQL
- GitHub Check: .NET build and test
- GitHub Check: Performance harness (artifacts)
🧰 Additional context used
📓 Path-based instructions (1)
QuickShell.Raycast/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain Raycast behavior in parity with Core launch behavior and use the project's Vitest test conventions.
Files:
QuickShell.Raycast/src/__tests__/workspace-transfer-files.test.tsQuickShell.Raycast/src/__tests__/import-export.test.tsQuickShell.Raycast/src/lib/migration.tsQuickShell.Raycast/src/lib/import-export.tsQuickShell.Raycast/src/lib/workspace-transfer-files.tsQuickShell.Raycast/src/open-workspace.tsxQuickShell.Raycast/src/__tests__/storage.test.tsQuickShell.Raycast/src/lib/storage.ts
🪛 ast-grep (0.45.0)
QuickShell.Raycast/src/lib/workspace-transfer-files.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔍 Remote MCP GitHub Copilot
Additional review context
-
Related PR
#112also modifiesQuickShellStorage.save(), layout synchronization, and import persistence. Its implementation carriesbranchTargetsandlayoutEntriesthrough saves and explicitly excludesbranchTargetsfrom portable exports. This overlaps the PR’s lock/refactor paths and may create merge-order/API conflicts. -
Related PR
#116changesmoveFavoritefrom"up" | "down"returningWorkspaceto supporting"top" | "bottom"and returningWorkspace | nullfor boundary no-ops. The PR under review refactors the same method under the write-lock, so compatibility with#116must be checked carefully. -
Repository code search found no additional occurrences of
pickWorkspaceTransferJsonPath,moveFavorite,resetAll, orrestoreFromBackupbeyond the available indexed results. -
DeepWiki could not provide repository context because
tonythethompson/QuickShellis not indexed; no architectural facts were obtained from that source.
🔇 Additional comments (22)
QuickShell.Raycast/src/lib/import-export.ts (2)
1-1: LGTM!
245-249: LGTM!QuickShell.Raycast/src/__tests__/import-export.test.ts (3)
114-155: LGTM!
157-196: LGTM!
198-229: LGTM!QuickShell.Raycast/src/lib/workspace-transfer-files.ts (6)
66-78: LGTM on the narrowed catch types themselves (matches the test's explicit rejection of a barecatch {}); see the adjacent comment on lines 54-64 for the ordering hazard this creates downstream.
1-11: LGTM!
9-9: 🎯 Functional Correctness | ⚡ Quick win
DialogKindis module-private but leaks into exported function signatures.
DialogKind(line 9) isn't exported, yet it's used as the parameter type ofpickWorkspaceTransferJsonPath,buildWindowsTransferPowerShell, andbuildMacTransferOsascript, all newly exported here. If this project ever emits declaration files ("declaration": true), TypeScript raises TS4025 ("exported function has or is using private name 'DialogKind'"). Trivial to future-proof by exporting the type.🔧 Suggested fix
-type DialogKind = "save" | "open"; +export type DialogKind = "save" | "open";Also applies to: 20-31, 139-139
117-136: LGTM!
139-168: LGTM!
170-183: LGTM!QuickShell.Raycast/src/__tests__/workspace-transfer-files.test.ts (1)
1-32: LGTM!QuickShell.Raycast/src/lib/storage.ts (3)
697-742: Nested-lock guard + timeout resolve prior critical/major concerns.
AsyncLocalStorage-based re-entrancy detection plus thePromise.racetimeout give a loud failure instead of a silent deadlock, and thegate/writeTailchaining is structured so a timed-out waiter's own release can't let a later caller jump ahead of a genuinely stuck predecessor (the chain still depends on the stuck writer's gate). Good fix for both previously flagged issues.
144-170: Resolves prior critical/major flags on restore trust and save() flushing.
restoreFromBackup()'s structural validation plusmigrateStoredData(parsed, { defaultToTrusted: false })correctly stops trust rehydration from an untrusted backup blob (matches the new "does not rehydrate trust" test).save()now flushes pending recent writes before persisting, closing the stale-snapshot gap from the earlier review round.Also applies to: 173-218, 283-296
76-79: Consistent write-lock/flush composition across all mutators.
hasBackup,undo/redo,importJson,grantTrust/revokeTrust, workspace CRUD, branch-target and layout/favorite mutations, andupdateSettingsall correctly compose viawithWriteLock+ the private*Unlockedhelpers rather than the publicsave/flushRecentWrites, so nothing here can trigger the new nested-lock guard.Also applies to: 81-142, 333-663, 675-690
QuickShell.Raycast/src/__tests__/storage.test.ts (1)
393-437: Solid coverage for serialization, backup/restore, and lock-timeout behavior.The rewritten
writeOrder/persisted-blob assertions at 393-437 now actually depend on the lock (addresses the prior comment). Backup no-op/reset/restore/trust and malformed-backup tests are thorough and matchstorage.tssemantics exactly, including the "no rehydrate trust without explicit backup security" case.Also applies to: 439-589, 591-615
QuickShell.Raycast/src/lib/migration.ts (1)
18-27: LGTM!Also applies to: 47-66
QuickShell.Raycast/src/open-workspace.tsx (2)
573-604: 🎯 Functional Correctness | ⚡ Quick winConfirm the loading toast isn't hidden after the follow-up toast is already shown.
loadingis created as an Animated toast and updated in place (loading.title = …) while waiting on the async file dialog — good, that matches Raycast's documented "update the same Toast" pattern. But on completion, a separateshowToast({...})call creates a second toast for the result, andloading.hide()only runs afterward infinally. Since Raycast shows one toast at a time, callinghide()on the superseded handle after the new one is already visible risks dismissing the just-shown result toast (or is at best a no-op) — worth confirming against current Raycast behavior, and simpler either way to just callloading.hide()right before showing the result toast (or updateloadingin place instead of creating a second toast).Also applies to: 605-652
654-671: Loading-state guard and destructive styling for Reset/Restore both look right.
handleResetAllnow bails out with a toast whendataisn't loaded yet instead of understating the delete count, andRestore Backup…now carriesAction.Style.DestructivealongsideReset All Workspaces…— both past comments addressed.Also applies to: 886-899
docs/architecture/hosts.md (1)
13-13: LGTM!Also applies to: 58-69
docs/architecture/parity-matrix.md (1)
24-25: LGTM!Also applies to: 45-45
docs/architecture/persistence.md (1)
144-158: LGTM!
Allow Open Directory on POSIX paths, hide elevation UX on Mac, enable Terminal/iTerm tab multi-launch, expand Mac discover roots, and gate macOS CI with lint and build. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Qodo found 2 finding(s) but none met the |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <github@trackdub.com>
I can’t update the PR title or description here, but I suggest: Title
Description
|
macOS CI ray lint failed on discover-git-repos-view.tsx Prettier style after the usePromise error toast change. Apply the project Prettier layout. Co-authored-by: Anthony Thompson <github@trackdub.com>
tonythethompson
changed the title
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Preserve macOS elevation metadata on save, reject future CmdPal envelope versions, keep separator-only merge layouts from appending phantom workspaces, fix Mac same-as-previous grouping, require existing dirs for companion WSL paths, and tighten Open Companion / reset toast handling. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Code review by qodo was updated up to the latest commit c9fa50d |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Animated style left a forever spinner when resetAll returned outcome noop. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Code review by qodo was updated up to the latest commit 1317fbf |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@AGENTS.md`:
- Line 7: Update the product overview in AGENTS.md to distinguish the
Windows-only .NET desktop hosts—QuickShell, QuickShell.Run, and
QuickShell.Suggest—from the separate QuickShell.Raycast TypeScript host, which
supports macOS; avoid describing the entire product as Windows-only.
In `@QuickShell.Raycast/src/lib/mac-launch.ts`:
- Around line 93-100: Update the launch planning flow around the entry loop and
separateWindows handling so every entry’s Mac terminal host is resolved
sequentially, carrying previousHostId through same-as-previous entries before
any window grouping occurs. Then group the already-resolved sequence normally or
emit one-entry groups when separateWindows is enabled, preserving existing
launch planning behavior. Add a regression test covering an iTerm entry followed
by same-as-previous with separateWindows enabled.
In `@QuickShell.Raycast/src/open-workspace.tsx`:
- Around line 681-686: Update the toast style selection in the reset flow around
isNoop so the no-op “Nothing to reset” path uses a non-animated Toast.Style,
while preserving the existing success style for completed resets and the current
title/message values.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a1527afe-3f02-4810-b920-3622bde398d9
📒 Files selected for processing (16)
AGENTS.mdQuickShell.Raycast/src/__tests__/import-export.test.tsQuickShell.Raycast/src/__tests__/security.test.tsQuickShell.Raycast/src/__tests__/single-row-launch.test.tsQuickShell.Raycast/src/__tests__/storage.test.tsQuickShell.Raycast/src/__tests__/workspace-transfer-files.test.tsQuickShell.Raycast/src/components/discover-git-repos-view.tsxQuickShell.Raycast/src/components/workspace-form.tsxQuickShell.Raycast/src/lib/import-export.tsQuickShell.Raycast/src/lib/mac-launch.tsQuickShell.Raycast/src/lib/migration.tsQuickShell.Raycast/src/lib/security.tsQuickShell.Raycast/src/lib/workspace-transfer-files.tsQuickShell.Raycast/src/open-workspace.tsxdocs/architecture/hosts.mddocs/architecture/persistence.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: CodeQL / Analyze C# with CodeQL: Add workspace reset/backup recovery and robust import/export handling for Raycast
Conclusion: failure
##[group]Run dotnet test QuickShell.Core.Tests/QuickShell.Core.Tests.csproj -c Release -p:Platform=x64 --no-build --verbosity minimal
�[36;1mdotnet test QuickShell.Core.Tests/QuickShell.Core.Tests.csproj -c Release -p:Platform=x64 --no-build --verbosity minimal�[0m
shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
env:
DOTNET_NOLOGO: true
DOTNET_CLI_TELEMETRY_OPTOUT: true
NUGET_XMLDOC_MODE: skip
DOTNET_ROOT: C:\Program Files\dotnet
CODEQL_ACTION_FEATURE_MULTI_LANGUAGE: false
CODEQL_ACTION_FEATURE_SANDWICH: false
CODEQL_ACTION_FEATURE_SARIF_COMBINE: true
CODEQL_ACTION_FEATURE_WILL_UPLOAD: true
CODEQL_ACTION_VERSION: 4.37.3
JOB_RUN_UUID: 75310bd3-66b5-478f-819a-784e63d6c580
CODEQL_ACTION_INIT_HAS_RUN: true
CODEQL_ACTION_ANALYSIS_KEY: .github/workflows/codeql.yml:analyze-csharp
CODEQL_WORKFLOW_STARTED_AT:
CODEQL_ACTION_CLI_VERSION_INFO: {"cmd":"C:\\hostedtoolcache\\windows\\CodeQL\\2.26.1\\x64\\codeql\\codeql.exe","version":{"productName":"CodeQL","vendor":"GitHub","version":"2.26.1","sha":"1ffdebae722922fb50a64b14a97789ed6161f4a7","branches":["codeql-cli-2.26.1"],"copyright":"Copyright (C) 2019-2026 GitHub, Inc.","unpackedLocation":"C:\\hostedtoolcache\\windows\\CodeQL\\2.26.1\\x64\\codeql","configFileLocation":"C:\\Users\\runneradmin\\.config\\codeql\\config","configFileFound":false,"overlayVersion":4,"features":{"analysisSummaryV2Default":true,"buildModeOption":true,"bundleSupportsIncludeDiagnostics":true,"bundleSupportsIncludeLogs":true,"bundleSupportsOverlay":true,"databaseInterpretResultsSupportsSarifRunProperty":true,"featuresInVersionResult":true,"indirectTracingSupportsStaticBinaries":false,"informsAboutUnsupportedPathFilters":true,"supportsPython312":true,"mrvaPackCreate":true,"threatModelOption":true,"traceCommandUseBuildMode":true,"v2ramSizing":true,"mrvaPackCreateMultipleQueries":true,"setsCodeqlRunnerEnvVar":true,"sarifMergeRunsFromEqualCategory":true,"forceOverwrite":true,"generateSummarySymb...
GitHub Actions: CodeQL / 0_Analyze C# with CodeQL.txt: Add workspace reset/backup recovery and robust import/export handling for Raycast
Conclusion: failure
##[group]Run dotnet test QuickShell.Core.Tests/QuickShell.Core.Tests.csproj -c Release -p:Platform=x64 --no-build --verbosity minimal
�[36;1mdotnet test QuickShell.Core.Tests/QuickShell.Core.Tests.csproj -c Release -p:Platform=x64 --no-build --verbosity minimal�[0m
shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
env:
DOTNET_NOLOGO: true
DOTNET_CLI_TELEMETRY_OPTOUT: true
NUGET_XMLDOC_MODE: skip
DOTNET_ROOT: C:\Program Files\dotnet
CODEQL_ACTION_FEATURE_MULTI_LANGUAGE: false
CODEQL_ACTION_FEATURE_SANDWICH: false
CODEQL_ACTION_FEATURE_SARIF_COMBINE: true
CODEQL_ACTION_FEATURE_WILL_UPLOAD: true
CODEQL_ACTION_VERSION: 4.37.3
JOB_RUN_UUID: 75310bd3-66b5-478f-819a-784e63d6c580
CODEQL_ACTION_INIT_HAS_RUN: true
CODEQL_ACTION_ANALYSIS_KEY: .github/workflows/codeql.yml:analyze-csharp
CODEQL_WORKFLOW_STARTED_AT:
CODEQL_ACTION_CLI_VERSION_INFO: {"cmd":"C:\\hostedtoolcache\\windows\\CodeQL\\2.26.1\\x64\\codeql\\codeql.exe","version":{"productName":"CodeQL","vendor":"GitHub","version":"2.26.1","sha":"1ffdebae722922fb50a64b14a97789ed6161f4a7","branches":["codeql-cli-2.26.1"],"copyright":"Copyright (C) 2019-2026 GitHub, Inc.","unpackedLocation":"C:\\hostedtoolcache\\windows\\CodeQL\\2.26.1\\x64\\codeql","configFileLocation":"C:\\Users\\runneradmin\\.config\\codeql\\config","configFileFound":false,"overlayVersion":4,"features":{"analysisSummaryV2Default":true,"buildModeOption":true,"bundleSupportsIncludeDiagnostics":true,"bundleSupportsIncludeLogs":true,"bundleSupportsOverlay":true,"databaseInterpretResultsSupportsSarifRunProperty":true,"featuresInVersionResult":true,"indirectTracingSupportsStaticBinaries":false,"informsAboutUnsupportedPathFilters":true,"supportsPython312":true,"mrvaPackCreate":true,"threatModelOption":true,"traceCommandUseBuildMode":true,"v2ramSizing":true,"mrvaPackCreateMultipleQueries":true,"setsCodeqlRunnerEnvVar":true,"sarifMergeRunsFromEqualCategory":true,"forceOverwrite":true,"generateSummarySymb...
🧰 Additional context used
📓 Path-based instructions (2)
QuickShell.Raycast/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Maintain Raycast behavior in TypeScript in parity with Core launch behavior, especially argument escaping, target resolution, and
wtlaunch planning.
Files:
QuickShell.Raycast/src/__tests__/single-row-launch.test.tsQuickShell.Raycast/src/__tests__/import-export.test.tsQuickShell.Raycast/src/lib/migration.tsQuickShell.Raycast/src/lib/mac-launch.tsQuickShell.Raycast/src/__tests__/security.test.tsQuickShell.Raycast/src/__tests__/storage.test.tsQuickShell.Raycast/src/lib/workspace-transfer-files.tsQuickShell.Raycast/src/__tests__/workspace-transfer-files.test.tsQuickShell.Raycast/src/lib/security.tsQuickShell.Raycast/src/lib/import-export.ts
QuickShell.Raycast/src/**/__tests__/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for Raycast tests and keep Windows launch behavior covered in parity with Core.
Files:
QuickShell.Raycast/src/__tests__/single-row-launch.test.tsQuickShell.Raycast/src/__tests__/import-export.test.tsQuickShell.Raycast/src/__tests__/security.test.tsQuickShell.Raycast/src/__tests__/storage.test.tsQuickShell.Raycast/src/__tests__/workspace-transfer-files.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: tonythethompson/QuickShell
Timestamp: 2026-07-29T06:13:56.652Z
Learning: When changing behavior, prefer the as-built architecture tours over proposal documents `docs/architecture/0001`-`0005`, and update the matching tour when changing an architectural spine.
Learnt from: CR
Repo: tonythethompson/QuickShell
Timestamp: 2026-07-29T06:13:56.652Z
Learning: After deploying the CmdPal extension, use `Reload Command Palette Extension`; deploying is required rather than merely building to register the MSIX.
🪛 ast-grep (0.45.0)
QuickShell.Raycast/src/lib/workspace-transfer-files.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
QuickShell.Raycast/src/__tests__/workspace-transfer-files.test.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
AGENTS.md
[uncategorized] ~49-~49: The official name of this software platform is spelled with a capital “H”.
Context: ... | | .github/workflows/ | ci.yml (build/test)...
(GITHUB)
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
- The PR is
#133and currently has successful Raycast Windows/macOS lint, test, build, and TypeScript CodeQL checks. The C# CodeQL check is failing, although the PR changes only Raycast/ documentation files. - Prior review findings led to concrete fixes for:
- write-queue serialization and nested-lock detection;
- durable malformed-backup disposal;
- settings-preserving restore;
- duplicate CmdPal IDs and skipped-layout rows;
- async transfer fallback/focus restoration;
- companion directory authorization.
- One unresolved current finding remains:
handleResetAllusesToast.Style.Animatedfor the terminal"noop"outcome. Because the toast is not retained or hidden, resetting an empty store may leave an indefinite spinner. - The default branch still exposes the old synchronous
pickWorkspaceTransferJsonPath()contract and caller; PR#133updates both to async, so caller consistency must be verified against the final PR revision. - DeepWiki could not retrieve the requested repository context because
tonythethompson/QuickShellis not indexed/found.
🔇 Additional comments (15)
QuickShell.Raycast/src/lib/import-export.ts (1)
4-4: LGTM!Also applies to: 83-85, 212-221
QuickShell.Raycast/src/__tests__/import-export.test.ts (1)
114-166: LGTM!Also applies to: 168-209, 211-250, 252-278, 280-311
QuickShell.Raycast/src/components/discover-git-repos-view.tsx (1)
30-44: LGTM!Also applies to: 127-141
QuickShell.Raycast/src/components/workspace-form.tsx (1)
154-200: LGTM!Also applies to: 334-334, 367-372, 397-397, 524-526, 585-585, 731-731, 782-800
QuickShell.Raycast/src/lib/workspace-transfer-files.ts (1)
112-117: LGTM!Also applies to: 205-206
QuickShell.Raycast/src/__tests__/workspace-transfer-files.test.ts (1)
17-17: LGTM!Also applies to: 77-104, 114-142, 144-216
QuickShell.Raycast/src/lib/mac-launch.ts (1)
13-16: LGTM!QuickShell.Raycast/src/lib/migration.ts (1)
170-175: LGTM!QuickShell.Raycast/src/__tests__/storage.test.ts (1)
624-626: LGTM!QuickShell.Raycast/src/lib/security.ts (1)
178-182: LGTM!Also applies to: 337-337, 495-512
QuickShell.Raycast/src/__tests__/security.test.ts (1)
253-256: LGTM!Also applies to: 290-293, 323-368, 381-381, 418-440
QuickShell.Raycast/src/__tests__/single-row-launch.test.ts (1)
1-23: LGTM!docs/architecture/hosts.md (1)
9-19: LGTM!Also applies to: 33-69, 75-95
docs/architecture/persistence.md (1)
142-158: LGTM!AGENTS.md (1)
39-49: LGTM!Also applies to: 124-124
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <github@trackdub.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <github@trackdub.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <github@trackdub.com>
|
Code review by qodo was updated up to the latest commit ac2d94c |
| "try", | ||
| ' set chosenFile to choose file name with prompt "Export Quick Shell workspaces" default name defaultName', | ||
| " set chosenPath to POSIX path of chosenFile", | ||
| "on error", | ||
| ' set chosenPath to ""', |
There was a problem hiding this comment.
🟡 Remediation Recommended
1. Dialog catches every applescript error 📘 Rule violation ☼ Reliability
The generated macOS save dialog uses an unqualified on error block, converting all AppleScript failures into an empty path that callers interpret as cancellation. Unexpected permission, scripting, or runtime failures are therefore silently suppressed.
Agent Prompt
## Issue description
The macOS transfer scripts catch every error and convert it into an empty selection. Handle only the expected user-cancellation error (`-128`) and allow unexpected errors to propagate.
## Issue Context
Both save and open dialog scripts need equivalent narrow error handling so operational failures are not reported as cancellation.
## Fix Focus Areas
- QuickShell.Raycast/src/lib/workspace-transfer-files.ts[168-172]
- QuickShell.Raycast/src/lib/workspace-transfer-files.ts[180-184]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const existingIndex = groupIndexByHost.get(hostId); | ||
| if (existingIndex !== undefined) { | ||
| groups[existingIndex].entries.push(entry); |
There was a problem hiding this comment.
🟡 Remediation Recommended
2. Fallback terminals split tabs 🐞 Bug ≡ Correctness
groupMacLaunchEntries groups by the requested host before unavailable iTerm launches fall back to Terminal.app. With tabs enabled, mixed Terminal/iTerm entries therefore open two Terminal windows instead of sharing one tabbed window when iTerm is not installed.
Agent Prompt
## Issue description
Mac launch entries are grouped using their requested host ID, but unavailable iTerm hosts later fall back to Terminal.app. Group by the effective installed application so all Terminal.app entries share one tabbed invocation.
## Issue Context
`resolveMacAppName` maps an unavailable iTerm host to Terminal, while `groupMacLaunchEntries` currently creates separate `terminal` and `iterm` groups before that fallback is known.
## Fix Focus Areas
- QuickShell.Raycast/src/lib/mac-launch.ts[52-59]
- QuickShell.Raycast/src/lib/mac-launch.ts[91-108]
- QuickShell.Raycast/src/lib/mac-launch.ts[199-202]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 74a787b |
3 participants
Summary
Summary by cubic
Serialize
QuickShellStoragewrites behind an in‑process queue with AsyncLocalStorage nested‑lock detection and a 30s timeout so overlapping mutations can’t clobber or hang. Add a confirmed Reset All that writes a durablequickshell-data.bak, preserves settings, supports Undo, and can Restore Backup after restart; plus macOS tabbed multi‑launch (when preferred), POSIX Open Directory, richer discover roots, CmdPal/Core layout import, and async import/export with focus restore and clearer toasts.New Features
quickshell-data.bak; keeps settings; supports Undo and Restore Backup.entries,Workspacewrappers, separators, flat PascalCase); auto‑build Raycast layout for an empty store.Bug Fixes
lock-timeout.osascripterrors and treat stdout cancels as cancel.Test plan
npm testinQuickShell.Raycast(206 tests)npm run lintinQuickShell.RaycastCloses #37
Closes #47
Made with Cursor