feat(icon): implement asynchronous icon loading to improve startup pe… - #1509
Conversation
📝 WalkthroughWalkthroughThese changes refactor icon loading from synchronous registration at app startup to asynchronous on-demand loading via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/renderer/src/main.ts (1)
71-77: Same redundancy assettings/main.ts.
preloadIcons()here andensureIconsLoaded()insrc/renderer/src/App.vue'sonMountedboth schedule the same work post-mount. Dedup viastate.loadPromisemakes this safe, but you can keep just one call site for clarity. If you intend this to start loading as early as possible (beforeApp.vuemounts its hook), consider callingpreloadIcons()directly (withoutsetTimeout) right afterapp.mount('#app')— thesetTimeout(…, 0)only defers by a macrotask and provides no measurable benefit over the direct call since dynamicimport()is already async.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/main.ts` around lines 71 - 77, Both preloadIcons() in main.ts and ensureIconsLoaded() in App.vue schedule the same post-mount icon loading (deduped by state.loadPromise), so remove the redundant setTimeout call: either call preloadIcons() directly right after app.mount('#app') to start loading ASAP or delete this call and rely on ensureIconsLoaded() in App.vue; update references to preloadIcons, ensureIconsLoaded, and state.loadPromise accordingly so only one callsite initiates the load and no setTimeout(..., 0) is used.src/renderer/settings/main.ts (1)
75-81: Post-mountsetTimeout(preloadIcons, 0)is redundant withensureIconsLoaded()inApp.vue'sonMounted.Both fire after mount and are deduped via the shared
loadPromise, so this is harmless — just noting it so you can drop one if you prefer a single entry point. Keeping the one inApp.vueensures loading is tied to the component lifecycle; thesetTimeouthere adds nothing beyond yielding a macrotask.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/main.ts` around lines 75 - 81, Redundant post-mount scheduling: remove the setTimeout wrapper that calls preloadIcons() in main.ts because App.vue already triggers icon loading via ensureIconsLoaded() in its onMounted; locate the setTimeout(() => { preloadIcons().catch(...) }, 0) block in src/renderer/settings/main.ts (the preloadIcons function and ensureIconsLoaded in App.vue are the shared load entry points) and delete that setTimeout block so icon loading is only invoked from the component lifecycle (or alternatively keep the main.ts call and remove the ensureIconsLoaded() call in App.vue—pick one consistent single entry point).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/src/App.vue`:
- Around line 389-400: The App currently calls ensureIconsLoaded() without
awaiting it in onMounted, causing AppBar/WindowSideBar icons to briefly render
empty; update the startup flow so ensureIconsLoaded() completes before
icon-heavy chrome is rendered — either await ensureIconsLoaded() in onMounted or
introduce an iconsReady reactive flag (set when ensureIconsLoaded resolves) and
gate AppBar/WindowSideBar render with that flag (leave RouterView gating via
isStartupRouteReady as-is); refer to ensureIconsLoaded, onMounted, AppBar,
WindowSideBar, RouterView, isStartupRouteReady, and the
addCollection/preloadIcons logic in iconLoader.ts when making the change.
In `@src/renderer/src/lib/iconLoader.ts`:
- Around line 58-64: The catch block in ensureIconsLoaded currently sets
state.isLoaded = true which prevents future retries; change the error handling
so that on failure you keep state.isLoaded = false, clear state.loadPromise (so
subsequent callers can attempt a retry), still log the error and do not rethrow,
and ensure state.isLoading is set to false in the finally block; refer to
ensureIconsLoaded, state.isLoaded, state.isLoading, and state.loadPromise when
making this change.
---
Nitpick comments:
In `@src/renderer/settings/main.ts`:
- Around line 75-81: Redundant post-mount scheduling: remove the setTimeout
wrapper that calls preloadIcons() in main.ts because App.vue already triggers
icon loading via ensureIconsLoaded() in its onMounted; locate the setTimeout(()
=> { preloadIcons().catch(...) }, 0) block in src/renderer/settings/main.ts (the
preloadIcons function and ensureIconsLoaded in App.vue are the shared load entry
points) and delete that setTimeout block so icon loading is only invoked from
the component lifecycle (or alternatively keep the main.ts call and remove the
ensureIconsLoaded() call in App.vue—pick one consistent single entry point).
In `@src/renderer/src/main.ts`:
- Around line 71-77: Both preloadIcons() in main.ts and ensureIconsLoaded() in
App.vue schedule the same post-mount icon loading (deduped by
state.loadPromise), so remove the redundant setTimeout call: either call
preloadIcons() directly right after app.mount('#app') to start loading ASAP or
delete this call and rely on ensureIconsLoaded() in App.vue; update references
to preloadIcons, ensureIconsLoaded, and state.loadPromise accordingly so only
one callsite initiates the load and no setTimeout(..., 0) is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e38d520f-0618-460d-8f0a-5b4da1120f8b
📒 Files selected for processing (5)
src/renderer/settings/App.vuesrc/renderer/settings/main.tssrc/renderer/src/App.vuesrc/renderer/src/lib/iconLoader.tssrc/renderer/src/main.ts
| onMounted(() => { | ||
| window.addEventListener('keydown', handleEscKey) | ||
|
|
||
| // initialize store data | ||
| // Ensure icons are loaded (load asynchronously, can happen in parallel with store init) | ||
| void ensureIconsLoaded() | ||
|
|
||
| // Start all critical data loads in parallel, don't wait for them | ||
| // This way session data starts loading much earlier instead of waiting for initAppStores() to complete | ||
| void initAppStores() | ||
| void sessionStore.fetchSessions() | ||
| setupMcpDeeplink() | ||
| setupAppIpcRuntime() |
There was a problem hiding this comment.
Icon flicker risk on first paint.
RouterView is gated by isStartupRouteReady, but AppBar / WindowSideBar render immediately and likely contain @iconify/vue Icon usages. Because ensureIconsLoaded() is not awaited and the JSON collections are dynamically imported, there will be a short window where those icons render blank until the chunks land and addCollection() runs. Not a correctness bug, but worth validating visually on a cold load — if noticeable, gate icon-heavy chrome on the resolved promise or inline a minimal critical icon set.
Otherwise the parallelization (initAppStores + fetchSessions + ensureIconsLoaded not awaited) looks fine; the existing dedup inside iconLoader.ts handles the duplicate call from main.ts's setTimeout(preloadIcons, 0) safely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/App.vue` around lines 389 - 400, The App currently calls
ensureIconsLoaded() without awaiting it in onMounted, causing
AppBar/WindowSideBar icons to briefly render empty; update the startup flow so
ensureIconsLoaded() completes before icon-heavy chrome is rendered — either
await ensureIconsLoaded() in onMounted or introduce an iconsReady reactive flag
(set when ensureIconsLoaded resolves) and gate AppBar/WindowSideBar render with
that flag (leave RouterView gating via isStartupRouteReady as-is); refer to
ensureIconsLoaded, onMounted, AppBar, WindowSideBar, RouterView,
isStartupRouteReady, and the addCollection/preloadIcons logic in iconLoader.ts
when making the change.
| } catch (error) { | ||
| console.error('[Startup][Renderer] Failed to load icons:', error) | ||
| // 继续执行，不要因为 icon 加载失败而中断应用 | ||
| state.isLoaded = true | ||
| } finally { | ||
| state.isLoading = false | ||
| } |
There was a problem hiding this comment.
Error path marks isLoaded = true, preventing any future retry.
On failure, both isLoaded is set to true and isLoading is cleared. Subsequent calls to ensureIconsLoaded() will short-circuit on line 28–30 and never retry, so a transient failure (e.g., chunk load error on a flaky network) permanently leaves the app icon-less for that session. Consider leaving isLoaded = false on error and clearing state.loadPromise so the next caller can retry, while still not throwing out of ensureIconsLoaded to avoid blocking the app.
🛡️ Suggested change
} catch (error) {
console.error('[Startup][Renderer] Failed to load icons:', error)
- // 继续执行，不要因为 icon 加载失败而中断应用
- state.isLoaded = true
+ // Allow a future caller to retry instead of permanently marking as loaded.
+ state.loadPromise = null
} finally {
state.isLoading = false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error('[Startup][Renderer] Failed to load icons:', error) | |
| // 继续执行，不要因为 icon 加载失败而中断应用 | |
| state.isLoaded = true | |
| } finally { | |
| state.isLoading = false | |
| } | |
| } catch (error) { | |
| console.error('[Startup][Renderer] Failed to load icons:', error) | |
| // Allow a future caller to retry instead of permanently marking as loaded. | |
| state.loadPromise = null | |
| } finally { | |
| state.isLoading = false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/lib/iconLoader.ts` around lines 58 - 64, The catch block in
ensureIconsLoaded currently sets state.isLoaded = true which prevents future
retries; change the error handling so that on failure you keep state.isLoaded =
false, clear state.loadPromise (so subsequent callers can attempt a retry),
still log the error and do not rethrow, and ensure state.isLoading is set to
false in the finally block; refer to ensureIconsLoaded, state.isLoaded,
state.isLoading, and state.loadPromise when making this change.
|
I found three actionable issues in this PR:
|
…rformance
Summary by CodeRabbit
Release Notes
Performance
Reliability