Skip to content

fix(desktop): cache browser discovery in settings#453

Merged
hiqiancheng merged 1 commit into
mainfrom
fix/browser-settings-discovery-cache
Jun 10, 2026
Merged

fix(desktop): cache browser discovery in settings#453
hiqiancheng merged 1 commit into
mainfrom
fix/browser-settings-discovery-cache

Conversation

@hiqiancheng

@hiqiancheng hiqiancheng commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

First external contributors may need to complete the CLA Assistant check before merge.
For code changes, this PR must link the related issue or RFC in the section below.

Summary

  • Cache installed-browser discovery and the default browser data path for the current app runtime so Settings tab switches do not repeatedly invoke native discovery.
  • Refresh installed browsers asynchronously when the browser executable dropdown opens, while preserving existing options if refresh fails.
  • Run Windows reg query and taskkill helper commands with CREATE_NO_WINDOW so browser discovery/cleanup does not flash command windows or surface reg.exe dialogs.

Related issue or RFC

Link the issue or RFC:

For code changes, link the tracking issue. Only documentation wording, link fixes, or comment-only cleanups may skip the issue-first flow.

AI assistance disclosure

If AI tools materially assisted this contribution, disclose that here or point to the relevant commit trailer.

  • Tool(s) used: Codex
  • Scope of assistance: implementation, local verification, PR description drafting
  • Human review or rewrite performed: changes were reviewed during local testing and before commit
  • Architecture or boundary impact: no new cross-boundary abstraction; adds a small browser settings runtime cache and one select open-state event

Testing evidence

List the commands you ran and the results you observed.

  • pnpm exec vitest run --configLoader runner tests/SettingsView/settingsBrowserSection.test.ts --passWithNoTests from apps/desktop: passed, 1 file / 15 tests.
  • cargo check --manifest-path src-tauri/Cargo.toml --lib from apps/desktop: passed.
  • pnpm test:pr from repo root: desktop checks passed through type check, test type check, lint check, format check, Rust check, 152 frontend test files / 963 tests, Rust tests, and frontend coverage. The final monorepo step failed at @touchai/site build because this local environment does not resolve the astro command ('astro' is not recognized as an internal or external command).
  • Manual startup: pnpm tauri dev launched successfully after freeing local disk space; verified target\\debug\\TouchAI.exe and Vite localhost:1420 were running.
  • pnpm test:e2e: not run locally; this local desktop E2E harness was not exercised after the targeted/manual verification above, so CI is the first full E2E proof.

Did you follow TDD (test-first) for feature and fix work? Strongly recommended. See docs/testing/testing.md.

pnpm exec vitest run --configLoader runner tests/SettingsView/settingsBrowserSection.test.ts --passWithNoTests
cargo check --manifest-path src-tauri/Cargo.toml --lib
pnpm test:pr

Risk notes

  • AgentService, runtime, MCP, or schema impact: limited browser runtime process-spawn behavior change on Windows; no AgentService, MCP, or schema impact.
  • database baseline or migration impact: none.
  • release or packaging impact: low; Windows helper process spawning now uses hidden/background creation flags for registry and taskkill helpers.

Screenshots or recordings

No screenshots; behavior is a settings performance/process-spawn fix.

Checklist

  • The PR title follows Conventional Commits and is valid for squash merge.
  • This PR is either ready for review or explicitly marked as a Draft PR.
  • I did not use [WIP] or similar title prefixes.
  • If AI materially assisted this PR, I disclosed the tools and scope and I personally reviewed every affected change.
  • I can explain the why, what, and how of this change without relying on an AI tool.
  • If this touches AgentService, runtime, MCP, or schema boundaries, there is an accepted RFC.
  • If this changes architecture or adds a new cross-boundary abstraction, there is an accepted RFC.
  • I ran pnpm test:pr for this code PR, or this is a docs-only change.
  • If I changed Rust behavior or tests, I reviewed pnpm test:coverage:rust or relied on CI coverage evidence.
  • If I changed desktop startup/window/search/popup/settings/E2E paths, I ran pnpm test:e2e locally or documented why CI is the first valid proof.
  • I added tests or explained why tests are not appropriate.
  • I updated docs when behavior changed.

@github-actions github-actions Bot added area:tauri Tauri shell or desktop runtime changes area:frontend Frontend UI or view-layer changes labels Jun 10, 2026
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This pull request implements intelligent runtime caching for browser discovery operations to eliminate redundant native calls, while adding Windows-specific process suppression and lifecycle-aware refresh triggers tied to user interaction in the settings dropdown.

Changes

Browser Discovery Runtime Caching

Layer / File(s) Summary
Windows process background suppression
apps/desktop/src-tauri/src/core/browser/process.rs
Registry query (reg) and process-tree kill (taskkill) commands are configured to suppress visible console windows on Windows via CREATE_NO_WINDOW flag and a centralized helper.
CustomSelect open state event propagation
apps/desktop/src/components/CustomSelect.vue
Component now explicitly types and propagates update:open events, allowing parents to detect dropdown state changes.
Runtime cache module for browser data
apps/desktop/src/views/SettingsView/components/Browser/runtimeCache.ts
New module provides in-memory caching with promise deduplication for installed browsers and default browser path, supporting force-refresh and test reset.
Browser settings cached loading and lifecycle management
apps/desktop/src/views/SettingsView/components/Browser/index.vue
Settings component integrates cached loaders with lifecycle guards (isUnmounted), discovery progress tracking, and dropdown-triggered forced refresh via handleBrowserExecutableDropdownOpen.
Test coverage for caching and refresh behavior
apps/desktop/tests/SettingsView/settingsBrowserSection.test.ts
Tests verify single native invocation across remounts, asynchronous refresh on dropdown open, and deterministic behavior via cache reset in setup.

Sequence Diagram

sequenceDiagram
  participant SettingsComponent
  participant RuntimeCache
  participant NativeBrowser
  participant UserDropdown
  SettingsComponent->>RuntimeCache: onMounted: loadCachedInstalledBrowsers()
  RuntimeCache->>NativeBrowser: first call: native discovery
  NativeBrowser-->>RuntimeCache: installed browsers list
  RuntimeCache-->>SettingsComponent: cached result
  SettingsComponent->>SettingsComponent: render dropdown
  UserDropdown->>SettingsComponent: `@update`:open event
  SettingsComponent->>RuntimeCache: handleBrowserExecutableDropdownOpen(force=true)
  RuntimeCache->>NativeBrowser: force bypass cache: native discovery
  NativeBrowser-->>RuntimeCache: refreshed list
  RuntimeCache-->>SettingsComponent: new result (or reuse in-flight)
  SettingsComponent->>SettingsComponent: update UI if not unmounted
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

area:tauri, area:frontend

Poem

🐰 Cache and dash, no more delays,
Browser lists bright through all our days,
Silent processes run so clean,
Fastest discovery ever seen,
Refresh on drop with clever cheer,
And tests confirm the magic here!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(desktop): cache browser discovery in settings' follows Conventional Commits format with proper prefix, scope, and clearly describes the main change of caching browser discovery.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed PR description comprehensively covers summary, related issue, AI disclosure, testing evidence, risk notes, and completed checklist items with appropriate caveats.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/browser-settings-discovery-cache

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@hiqiancheng hiqiancheng added this pull request to the merge queue Jun 10, 2026
Merged via the queue into main with commit 0106912 Jun 10, 2026
30 checks passed
@hiqiancheng hiqiancheng deleted the fix/browser-settings-discovery-cache branch June 10, 2026 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:frontend Frontend UI or view-layer changes area:tauri Tauri shell or desktop runtime changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant