Performance audit sweep: cloning, async I/O, indexing, parallel discovery#12
Merged
Conversation
Add /.bob directory and docs/performance-audit.md to .gitignore.
![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: 3340d522d6
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a performance-audit sweep across core services (shortcut repository, git repo discovery, startup warm-up/tracing) while adding targeted tests and documentation to lock in behavior and guide accessibility verification.
Changes:
- Introduces async shortcut warm-up + opt-in startup tracing, plus lazy initialization for CmdPal fallback UI.
- Optimizes core paths (shortcut lookups/search, git repo discovery parallelism, JSON parsing reuse) with new/updated unit tests.
- Adds documentation for performance findings and accessibility testing; adjusts minor UI/form spacing and glyph mapping.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| QuickShell/Services/TerminalCatalogChoices.cs | Adds minimal default-profile choices helper to reduce eager enumeration. |
| QuickShell/Services/SettingsCardJson.cs | Tweaks Adaptive Card spacing + replaces recent-count stepper with a toggle template. |
| QuickShell/Services/QuickShellRuntimeServices.cs | Kicks off background shortcut preload during initialization. |
| QuickShell/QuickShellSettingsManager.cs | Defers default-profile choices; updates “recents” setting strings and normalization flow. |
| QuickShell/QuickShellFallback.cs | Lazily constructs fallback page and avoids repeated shortcut enumeration. |
| QuickShell/QuickShellCommandsProvider.cs | Adds startup tracing spans + lazy fallback page construction and lifecycle handling. |
| QuickShell/Pages/TerminalDefaultsSettingsForm.cs | Spacing tweaks on adaptive card inputs/actions. |
| QuickShell/Pages/ShortcutTransferSettingsForm.cs | Spacing tweaks in transfer/import conflict UI. |
| QuickShell/Pages/HomeDisplaySettingsForm.cs | Switches recents configuration from stepper to toggle + saves on change action. |
| QuickShell.Run/Main.cs | Adds startup tracing spans + background shortcut preload kickoff. |
| QuickShell.Core/Services/WtProfilesService.cs | Extracts single JSON parse path and exposes ReadProfilesFromJson for tests. |
| QuickShell.Core/Services/StartupPerformanceTrace.cs | Adds opt-in startup timing via QUICKSHELL_STARTUP_TRACE. |
| QuickShell.Core/Services/ShortcutRepository.cs | Adds indexes, async preload/reload, lower history cap, and reduces search allocations. |
| QuickShell.Core/Services/ShortcutRecents.cs | Uses bounded display clamp for recents count. |
| QuickShell.Core/Services/ShortcutLayoutJson.cs | Adds async JSON parse helper for async repository load. |
| QuickShell.Core/Services/ShortcutGlyphs.cs | Swaps glyph mappings for Duplicate vs CopyPath. |
| QuickShell.Core/Services/QuickShellRecentSettings.cs | Changes recents setting to enable/disable semantics with fixed cap. |
| QuickShell.Core/Services/GitRepoDiscovery.cs | Adds bounded parallel discovery while preserving limits and stable ordering. |
| QuickShell.Core.Tests/WorkspaceUtilityTests.cs | Adds tests for bounded parallel discovery + updated recent-count semantics. |
| QuickShell.Core.Tests/TerminalProfileIntegrationTests.cs | Adds test for nested defaultProfile application in WT settings JSON. |
| QuickShell.Core.Tests/TerminalCatalogChoicesTests.cs | Adds test for minimal default profile choices. |
| QuickShell.Core.Tests/StartupPerformanceTraceTests.cs | Adds tests for env-var opt-in logic. |
| QuickShell.Core.Tests/ShortcutRepositoryPerformanceShapeTests.cs | Adds performance-shape/correctness tests for indexing, allocations, async load. |
| QuickShell.Core.Tests/ShortcutDisplayTests.cs | Updates glyph assertions and adds Duplicate glyph test. |
| docs/performance-audit.md | Adds detailed performance audit report and implementation status. |
| ACCESSIBILITY.md | Adds accessibility testing checklist and store-submission guidance. |
| .gitignore | Stops ignoring ACCESSIBILITY.md; adds /.bob. |
| .codex/config.toml | Sets Codex sandbox/approval defaults (security-sensitive). |
- TerminalCatalog: cache fields were read outside the lock after EnsureCached() returned, so a concurrent InvalidateCache() could null them mid-read and throw NRE. Bundled the cache into one atomically-swapped snapshot object instead. - ShortcutFormPage: ShortcutForm subscribed to the static, process- lifetime Drafts.Cleared event with no reliable unsubscribe path if the form was abandoned (Escape, navigate away) instead of explicitly saved/canceled, permanently rooting the instance. Subscribe via a weak-reference trampoline so an abandoned form can be collected. - GitRepoIndex: EnsureFresh held the lock for the full duration of the (multi-second, multi-thousand-directory) filesystem scan, serializing every other cache reader/invalidator behind it. Scan now runs off the lock via a coalesced single-flight task. - ShortcutRepository: Dispose() called Timer.Dispose() (no wait) then immediately disposed the semaphore/mutex the timer callback uses, leaving a shutdown race where an in-flight callback could hit ObjectDisposedException. Switched to Timer.Dispose(WaitHandle) to block until any in-flight callback finishes first. - SettingsFormHelpers.ScheduleRefresh: fire-and-forget Task.Run had no try/catch, so an exception (e.g. page torn down mid-flight) became an unobserved task exception. Wrapped in a best-effort catch. Found via a targeted concurrency/leak/bottleneck sweep across QuickShell.Core, QuickShell, and QuickShell.Run; QuickShell.Run itself had no new findings beyond the audit's already-documented per-keystroke stat-file bottleneck. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- ShortcutFormTemplateCache: build the template outside the lock so a slow build for one key doesn't block unrelated GetOrBuild/Invalidate callers. - SettingsFormHelpers: remove ScheduleDebouncedReload, dead code with no call sites whose shared process-static CancellationTokenSource would incorrectly cancel unrelated callers' pending reloads if ever wired up for more than one debounce source. Left PendingShortcutEditForm's per-render allocation as-is: it snapshots the pending draft's name at construction, so caching the instance across GetContent() calls could show a stale description if the pending draft changes while the settings page stays open. Not worth the correctness risk to save one small allocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- GitRepoDiscovery: stream child-directory enumeration instead of ToArray()-ing every entry up front, so ShouldStop()/MaxDirectoriesScanned can short-circuit a very wide directory instead of paying to materialize it all first. - .codex/config.toml: stop committing sandbox_mode=danger-full-access and approval_policy=never; these grant full filesystem/network access with no approval gate to anyone who clones the repo or runs Codex in CI. Set locally (untracked) if wanted. - QuickShellSettingsManager: clarify that the recent-workspaces setting is an on/off toggle (fixed cap, any non-zero value treated as "on"), not a literal count, since the raw text field previously implied otherwise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- GitRepoIndex.EnsureFresh: clear _refreshInFlight in a catch/finally when the discovery task faults. Previously a faulted task stayed cached forever, so every future call reused (and re-threw from) the same dead task instead of retrying. - SettingsFormHelpers.ScheduleRefresh: narrow the catch to ObjectDisposedException/COMException (the expected "page/COM host torn down" cases) instead of swallowing every exception, so real bugs still surface. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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.
Summary
Implements the performance audit findings in docs/performance-audit.md, then fixes correctness/concurrency issues found via review and a follow-up targeted sweep.
Performance sweep:
Dictionary-backed name/ID indexes toShortcutRepository, kept in sync across load/mutate/undo/redoGitRepoDiscoverywith a bounded worker pool, preserving result ordering and scan limitsSearch()/SearchForRootPalette()via span-based query matchingWtProfilesServiceJSON parsing into a singleReadProfilesFromJsonpathQUICKSHELL_STARTUP_TRACE=1Bugs found and fixed during review:
TerminalCatalog's cached fields were read outside the lock afterEnsureCached()returned, so a concurrentInvalidateCache()could null them mid-read and throwNullReferenceException. Bundled the cache into one atomically-swapped snapshot object.ShortcutFormsubscribed to the static, process-lifetimeDrafts.Clearedevent with no reliable unsubscribe path if the form was abandoned (Escape, navigating away) instead of explicitly saved/canceled, permanently rooting the instance. Subscribes via a weak-reference trampoline now.GitRepoIndex.EnsureFreshheld its lock for the full duration of the multi-second, multi-thousand-directory filesystem scan, serializing every other cache reader/invalidator behind it. The scan now runs off-lock via a coalesced single-flight task.GitRepoDiscovery's pending-work counter so the stop-check and increment can't interleave, closing a theoretical stranded-item race in the parallel worker pool. Also switched child-directory enumeration to a streaming iterator soShouldStop()/MaxDirectoriesScannedcan short-circuit a very wide directory instead of materializing it viaToArray()first.ShortcutRepository.Dispose()disposed the persist timer without waiting for an in-flight callback, then immediately disposed the semaphore/mutex the callback uses — a shutdown race that could throwObjectDisposedExceptionon a background thread. Switched toTimer.Dispose(WaitHandle)to block until any in-flight callback finishes first.SettingsFormHelpers.ScheduleRefresh's fire-and-forget task had no exception handling, so a failure became an unobserved task exception. Now catches only the expected teardown exceptions (ObjectDisposedException,COMException); anything else still surfaces.ShortcutFormTemplateCachenow builds its template outside the lock so one slow build doesn't block unrelated callers.ScheduleDebouncedReloadcode whose shared process-staticCancellationTokenSourcewould have incorrectly canceled unrelated callers' pending reloads if it were ever wired up for more than one debounce source.Security:
sandbox_mode = "danger-full-access"/approval_policy = "never"from the committed.codex/config.toml— these granted full filesystem/network access with no approval gate to anyone cloning the repo or running Codex in CI. Left a comment pointing at setting them locally (untracked) instead.Clarity:
NormalizeCounttreats any non-zero value as "on").Test plan
dotnet build QuickShell.slndotnet test QuickShell.Core.Tests\QuickShell.Core.Tests.csproj— 179 passed🤖 Generated with Claude Code