Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRoutes tray-initiated update checks to the settings/about renderer, replaces Changes
Sequence Diagram(s)sequenceDiagram
participant Tray as Tray/Menu
participant Main as Main Process
participant WinMgr as Window Presenter
participant Settings as Settings Renderer
Tray->>Main: TRAY_EVENTS.CHECK_FOR_UPDATES
Main->>WinMgr: createSettingsWindow()
WinMgr->>Settings: Window created / shown
Main->>WinMgr: sendToWindow(settingsId, SETTINGS_EVENTS.NAVIGATE, {route:'settings-about'})
Main->>WinMgr: sendToWindow(settingsId, SETTINGS_EVENTS.CHECK_FOR_UPDATES)
Note over Settings: Renderer handles external check\nand invokes upgrade store
sequenceDiagram
participant User as User
participant Renderer as Settings Renderer
participant Upgrade as Upgrade Store
participant Main as Upgrade Presenter (Main)
User->>Renderer: Click "Manual Download (official)"
Renderer->>Upgrade: handleUpdate('official')
Upgrade->>Main: goDownloadUpgrade('official')
Main->>Main: open official URL / start download
alt Download completes
Main->>Upgrade: STATUS_CHANGED(downloaded)
Upgrade->>Renderer: updateState = ready_to_install
else Download error
Main->>Upgrade: STATUS_CHANGED(error)
Upgrade->>Renderer: showManualDownloadOptions = true
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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: d04e34f228
ℹ️ 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".
Comment on lines
+271
to
+272
| if (upgrade.updateState === 'available' || upgrade.isReadyToInstall) { | ||
| await upgrade.handleUpdate('auto') |
There was a problem hiding this comment.
In the new tray flow (eventListenerSetupHook now sends SETTINGS_EVENTS.CHECK_FOR_UPDATES), this handler is reused for external “check for updates” requests. When an update has already been downloaded in the background, upgrade.isReadyToInstall is true here, so the tray menu ends up calling upgrade.handleUpdate('auto'), which immediately goes to restartToUpdate(). That turns a non-destructive “Check for Updates” action into an unexpected quit/install path.
Useful? React with 👍 / 👎.
Comment on lines
+304
to
+308
| onMounted(async () => { | ||
| deviceInfo.value = await devicePresenter.getDeviceInfo() | ||
| appVersion.value = await devicePresenter.getAppVersion() | ||
| updateChannel.value = await configPresenter.getUpdateChannel() | ||
| console.log(deviceInfo.value) | ||
| await syncUpdateStatus() | ||
| window.electron?.ipcRenderer?.on(SETTINGS_EVENTS.CHECK_FOR_UPDATES, handleExternalCheckUpdate) |
There was a problem hiding this comment.
The main-process tray handler sends SETTINGS_EVENTS.CHECK_FOR_UPDATES immediately and retries once after 250 ms, but this component does not register its listener until after three awaited IPC calls. On a cold-opened settings window or a slower machine, both events can be emitted before line 308 runs, so clicking the tray’s “Check for Updates” item only opens Settings → About and never actually starts the check.
Useful? React with 👍 / 👎.
Comment on lines
195
to
199
| isSilent.value = silent | ||
| if (isChecking.value) return | ||
| isChecking.value = true | ||
| if (isChecking.value) return rawStatus.value | ||
|
|
||
| try { | ||
| await upgradeP.checkUpdate() |
There was a problem hiding this comment.
This no longer marks the store as checking until the async UPDATE_EVENTS.STATUS_CHANGED round-trip comes back from the main process. During that gap isChecking stays false, so the About button remains enabled and repeated clicks can issue multiple upgradeP.checkUpdate() IPCs. The main presenter’s checkUpdate() only guards on _lock, and _lock is not set while a check is in flight, so these requests are not coalesced.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/types/i18n.d.ts (1)
1797-1801:⚠️ Potential issue | 🟠 MajorUpdate i18n type declarations to include all new update keys.
officialDownloadwas added, but the newly introduced keys used by the UI (installUpdate,versionAvailable,autoUpdateFailed,topbarButton) are still missing fromDefineLocaleMessage.💡 Suggested type update
githubDownload: string officialDownload: string checkUpdate: string downloading: string installNow: string + installUpdate: string + versionAvailable: string + autoUpdateFailed: string + topbarButton: string autoUpdate: string restarting: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/i18n.d.ts` around lines 1797 - 1801, The i18n type declarations are missing several new translation keys used by the UI; update the locale message type (e.g., the DefineLocaleMessage interface/type) to include the new keys installUpdate, versionAvailable, autoUpdateFailed, and topbarButton alongside existing keys like officialDownload/autoUpdate/installNow/downloading/checkUpdate so TypeScript knows these messages exist; modify the DefineLocaleMessage (and any related LocaleMessages or locale type aliases) to add these string properties.
🧹 Nitpick comments (4)
test/renderer/components/AboutUsSettings.test.ts (1)
31-48: Consider adding tests for different upgrade states.The mock uses static values (
updateState: 'error',showManualDownloadOptions: true). While this tests the fallback download scenario well, consider adding tests for other states likeidle,downloading, andready_to_installto verify the conditional rendering logic.Example: Parameterized state tests
it.each([ { updateState: 'idle', showManualDownloadOptions: false, expectedButtons: ['意见反馈', '免责声明'] }, { updateState: 'downloading', showManualDownloadOptions: false, expectedButtons: ['意见反馈', '免责声明'] }, { updateState: 'ready_to_install', showManualDownloadOptions: false, expectedButtons: ['意见反馈', '免责声明', '安装更新'] }, ])('renders correct buttons for state: $updateState', async ({ updateState, showManualDownloadOptions, expectedButtons }) => { // Update mock values per test case upgradeStoreMock.updateState = updateState upgradeStoreMock.showManualDownloadOptions = showManualDownloadOptions // ... mount and assert })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/AboutUsSettings.test.ts` around lines 31 - 48, Add parameterized tests that exercise different upgrade states by reusing and mutating the existing upgradeStoreMock object in AboutUsSettings.test.ts: update upgradeStoreMock.updateState and upgradeStoreMock.showManualDownloadOptions for cases like 'idle', 'downloading', and 'ready_to_install', then mount the component and assert the rendered buttons/UI for each case (e.g., presence/absence of '安装更新' and fallback manual download options). Use it.each or a loop to iterate cases and call the existing refresh/check/handle mocks as needed so each scenario verifies the conditional rendering logic driven by updateState and showManualDownloadOptions.src/renderer/src/components/AppBar.vue (1)
105-107: Consider inlining the handler.The
handleInstallUpdatefunction is a simple wrapper that only callsupgrade.handleUpdate('auto'). You could inline this in the template for brevity, though keeping it as-is is also fine for clarity.Optional: Inline the handler
-const handleInstallUpdate = async () => { - await upgrade.handleUpdate('auto') -}And in template:
- `@click`="handleInstallUpdate" + `@click`="upgrade.handleUpdate('auto')"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/AppBar.vue` around lines 105 - 107, The small wrapper handleInstallUpdate can be removed and its body inlined where it's used in the template: replace usages of the handleInstallUpdate handler with a direct call to upgrade.handleUpdate('auto') (e.g., `@click`="upgrade.handleUpdate('auto')"), then delete the handleInstallUpdate definition; ensure the template has access to the upgrade object (import/prop/returned from setup) so the call preserves the same context.test/renderer/stores/upgradeStore.test.ts (1)
55-167: Good test coverage for the refactored store.The tests properly validate:
- Manual check state transitions
- Presenter status hydration (downloaded, downloading)
- Null snapshot handling
- Error state with manual download fallback
Consider adding tests for:
- The concurrent check guard (
isChecking.valuecheck incheckUpdate)handleUpdate('auto')triggeringstartDownloadUpdatehandleUpdatewhenisReadyToInstallis true (should callrestartToUpdate)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/stores/upgradeStore.test.ts` around lines 55 - 167, Add tests covering the three missing behaviors: ensure the concurrent check guard prevents duplicate checks by asserting that calling useUpgradeStore().checkUpdate twice while isChecking is true only invokes upgradePresenterMock.getUpdateStatus once (reference isChecking and checkUpdate), add a test that calling useUpgradeStore().handleUpdate('auto') triggers upgradePresenterMock.startDownloadUpdate (reference handleUpdate and startDownloadUpdate), and add a test that when store.isReadyToInstall is true calling handleUpdate (no arg or appropriate install action) invokes upgradePresenterMock.restartToUpdate (reference isReadyToInstall, handleUpdate, and restartToUpdate); mock the presenter responses and assert the mocks were called as expected.src/main/presenter/lifecyclePresenter/hooks/ready/eventListenerSetupHook.ts (1)
93-98: Consider adding a comment explaining the retry rationale.The 250ms delayed retry is a defensive pattern, but its purpose isn't immediately clear. A brief comment would help future maintainers understand why this is needed.
Note: The
sendToWindowcalls silently fail if the window is closed between the initial call and the retry. This is acceptable behavior for a best-effort update check trigger, but be aware that failures won't be logged.Suggested comment
navigateToAbout() triggerUpdateCheck() + // Retry after a short delay to handle cases where the settings window + // isn't fully initialized to receive IPC messages immediately after creation setTimeout(() => { if (presenter.windowPresenter.getSettingsWindowId() === settingsWindowId) { navigateToAbout() triggerUpdateCheck() } }, 250)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/lifecyclePresenter/hooks/ready/eventListenerSetupHook.ts` around lines 93 - 98, Add a brief inline comment above the setTimeout retry block explaining the defensive retry rationale: that we wait 250ms then re-check presenter.windowPresenter.getSettingsWindowId() against settingsWindowId to handle a race where the settings window may not yet be registered, and if it matches we call navigateToAbout() and triggerUpdateCheck() as a best-effort (silent failures from sendToWindow are acceptable). Mention that this is intentional to avoid noisy errors when the window closes between the initial attempt and the retry.
🤖 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/i18n/da-DK/update.json`:
- Around line 13-15: Replace the English strings for the i18n keys
versionAvailable, autoUpdateFailed, and topbarButton in the da-DK update.json
with Danish translations: keep the {version} placeholder in versionAvailable and
change its value to a Danish phrase (e.g., "{version} er tilgængelig"),
translate autoUpdateFailed to Danish (e.g., "Automatisk opdatering mislykkedes.
Brug venligst de manuelle downloadlinks nedenfor.") and change topbarButton to
"Opdater"; update the string values for the keys versionAvailable,
autoUpdateFailed, and topbarButton accordingly in the JSON.
In `@src/renderer/src/i18n/fa-IR/update.json`:
- Around line 12-15: Replace the English text for the i18n keys installUpdate,
versionAvailable, autoUpdateFailed, and topbarButton with proper Farsi
translations in the update.json language bundle; update the values for
"installUpdate" (currently "Install Update"), "versionAvailable" (currently
"{version} is available"), "autoUpdateFailed" (currently "Automatic update
failed. Please use the manual download links below."), and "topbarButton"
(currently "Update") with their Farsi equivalents while preserving placeholders
like {version} and JSON string formatting.
In `@src/renderer/src/i18n/fr-FR/update.json`:
- Around line 14-16: The French locale contains English strings for the keys
"versionAvailable", "autoUpdateFailed", and "topbarButton"; update
src/renderer/src/i18n/fr-FR/update.json by replacing those values with proper
French translations (for example: "versionAvailable" -> "{version} est
disponible", "autoUpdateFailed" -> "La mise à jour automatique a échoué.
Veuillez utiliser les liens de téléchargement manuels ci‑dessous.",
"topbarButton" -> "Mettre à jour") so the UI is fully localized for French
users.
In `@src/renderer/src/i18n/he-IL/update.json`:
- Around line 12-15: The Hebrew locale file has untranslated English strings for
the keys "installUpdate", "versionAvailable", "autoUpdateFailed", and
"topbarButton" in update.json; replace those English values with proper Hebrew
translations for each key (preserving the placeholder {version} in
"versionAvailable"), ensuring correct UTF-8 encoding and grammatical phrasing
for UI context so the he-IL locale no longer mixes Hebrew and English.
In `@src/renderer/src/i18n/ja-JP/update.json`:
- Around line 14-16: Replace the English strings for the Japanese locale by
updating the keys versionAvailable, autoUpdateFailed, and topbarButton in the
ja-JP update.json: set versionAvailable to a Japanese phrase like "{version}
が利用可能です", autoUpdateFailed to "自動更新に失敗しました。以下の手動ダウンロードリンクを使用してください。", and
topbarButton to "更新" so the locale file contains proper Japanese translations.
In `@src/renderer/src/i18n/ko-KR/update.json`:
- Around line 14-16: The three new localization keys "versionAvailable",
"autoUpdateFailed", and "topbarButton" in
src/renderer/src/i18n/ko-KR/update.json are still in English; translate their
values to Korean to keep the update flow consistent (e.g., translate "{version}
is available" to a natural Korean string preserving the {version} placeholder,
translate "Automatic update failed. Please use the manual download links below."
and "Update" into appropriate Korean phrases) and update the JSON values
accordingly.
In `@src/renderer/src/i18n/pt-BR/update.json`:
- Around line 13-15: The three i18n keys versionAvailable, autoUpdateFailed, and
topbarButton are still in English; update their values to Portuguese (e.g.,
versionAvailable -> "{version} está disponível", autoUpdateFailed ->
"Atualização automática falhou. Por favor, use os links de download manuais
abaixo.", topbarButton -> "Atualizar") so the pt-BR locale is fully translated
and avoids mixed-language UI.
In `@src/renderer/src/i18n/ru-RU/update.json`:
- Around line 14-16: Replace the English strings in the Russian locale JSON for
the keys versionAvailable, autoUpdateFailed, and topbarButton with Russian
translations, preserving the {version} placeholder exactly for versionAvailable;
update the values of "versionAvailable", "autoUpdateFailed", and "topbarButton"
in src/renderer/src/i18n/ru-RU/update.json to appropriate Russian text.
In `@src/renderer/src/stores/upgrade.ts`:
- Around line 177-180: The code assigns a hardcoded Chinese string directly to
updateError (pattern: updateError.value = error || '更新出错'), which should be
replaced with a language-neutral fallback or an i18n key; update the two spots
that use this pattern to either set a neutral default like 'Update error' or set
an i18n key (e.g., 'error.update') and, if available, call the translation
helper before assigning (or let the consuming component translate the key),
ensuring updateError and consumers such as AboutUsSettings.vue receive a
translatable/value-neutral message instead of raw Chinese text.
---
Outside diff comments:
In `@src/types/i18n.d.ts`:
- Around line 1797-1801: The i18n type declarations are missing several new
translation keys used by the UI; update the locale message type (e.g., the
DefineLocaleMessage interface/type) to include the new keys installUpdate,
versionAvailable, autoUpdateFailed, and topbarButton alongside existing keys
like officialDownload/autoUpdate/installNow/downloading/checkUpdate so
TypeScript knows these messages exist; modify the DefineLocaleMessage (and any
related LocaleMessages or locale type aliases) to add these string properties.
---
Nitpick comments:
In `@src/main/presenter/lifecyclePresenter/hooks/ready/eventListenerSetupHook.ts`:
- Around line 93-98: Add a brief inline comment above the setTimeout retry block
explaining the defensive retry rationale: that we wait 250ms then re-check
presenter.windowPresenter.getSettingsWindowId() against settingsWindowId to
handle a race where the settings window may not yet be registered, and if it
matches we call navigateToAbout() and triggerUpdateCheck() as a best-effort
(silent failures from sendToWindow are acceptable). Mention that this is
intentional to avoid noisy errors when the window closes between the initial
attempt and the retry.
In `@src/renderer/src/components/AppBar.vue`:
- Around line 105-107: The small wrapper handleInstallUpdate can be removed and
its body inlined where it's used in the template: replace usages of the
handleInstallUpdate handler with a direct call to upgrade.handleUpdate('auto')
(e.g., `@click`="upgrade.handleUpdate('auto')"), then delete the
handleInstallUpdate definition; ensure the template has access to the upgrade
object (import/prop/returned from setup) so the call preserves the same context.
In `@test/renderer/components/AboutUsSettings.test.ts`:
- Around line 31-48: Add parameterized tests that exercise different upgrade
states by reusing and mutating the existing upgradeStoreMock object in
AboutUsSettings.test.ts: update upgradeStoreMock.updateState and
upgradeStoreMock.showManualDownloadOptions for cases like 'idle', 'downloading',
and 'ready_to_install', then mount the component and assert the rendered
buttons/UI for each case (e.g., presence/absence of '安装更新' and fallback manual
download options). Use it.each or a loop to iterate cases and call the existing
refresh/check/handle mocks as needed so each scenario verifies the conditional
rendering logic driven by updateState and showManualDownloadOptions.
In `@test/renderer/stores/upgradeStore.test.ts`:
- Around line 55-167: Add tests covering the three missing behaviors: ensure the
concurrent check guard prevents duplicate checks by asserting that calling
useUpgradeStore().checkUpdate twice while isChecking is true only invokes
upgradePresenterMock.getUpdateStatus once (reference isChecking and
checkUpdate), add a test that calling useUpgradeStore().handleUpdate('auto')
triggers upgradePresenterMock.startDownloadUpdate (reference handleUpdate and
startDownloadUpdate), and add a test that when store.isReadyToInstall is true
calling handleUpdate (no arg or appropriate install action) invokes
upgradePresenterMock.restartToUpdate (reference isReadyToInstall, handleUpdate,
and restartToUpdate); mock the presenter responses and assert the mocks were
called as expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05cfa564-1fd0-46c8-918c-40221aa77d60
📒 Files selected for processing (25)
src/main/events.tssrc/main/presenter/lifecyclePresenter/hooks/ready/eventListenerSetupHook.tssrc/main/presenter/upgradePresenter/index.tssrc/renderer/settings/components/AboutUsSettings.vuesrc/renderer/src/App.vuesrc/renderer/src/components/AppBar.vuesrc/renderer/src/components/ui/UpdateDialog.vuesrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/update.jsonsrc/renderer/src/i18n/en-US/update.jsonsrc/renderer/src/i18n/fa-IR/update.jsonsrc/renderer/src/i18n/fr-FR/update.jsonsrc/renderer/src/i18n/he-IL/update.jsonsrc/renderer/src/i18n/ja-JP/update.jsonsrc/renderer/src/i18n/ko-KR/update.jsonsrc/renderer/src/i18n/pt-BR/update.jsonsrc/renderer/src/i18n/ru-RU/update.jsonsrc/renderer/src/i18n/zh-CN/update.jsonsrc/renderer/src/i18n/zh-HK/update.jsonsrc/renderer/src/i18n/zh-TW/update.jsonsrc/renderer/src/stores/upgrade.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/types/i18n.d.tstest/renderer/components/AboutUsSettings.test.tstest/renderer/stores/upgradeStore.test.ts
💤 Files with no reviewable changes (2)
- src/renderer/src/App.vue
- src/renderer/src/components/ui/UpdateDialog.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/stores/upgrade.ts (1)
211-217:⚠️ Potential issue | 🟠 MajorManual download failures are currently silent.
startUpdate()returnsfalseon failure, but the'github' | 'official'path ignores that result. IfgoDownloadUpgrade()fails, the UI never enters an error state and the user gets no feedback.💡 Minimal fix
- await startUpdate(type) + const success = await startUpdate(type) + if (!success) { + applyStatus('error', updateInfo.value, DEFAULT_UPDATE_ERROR) + }Also applies to: 240-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/stores/upgrade.ts` around lines 211 - 217, startUpdate currently swallows errors from upgradeP.goDownloadUpgrade (returning false) so callers on the 'github'|'official' path never see failures; change startUpdate to not swallow the failure—after logging the error, rethrow it (or return a rejected promise) instead of returning false so callers awaiting startUpdate will receive the failure and can enter the UI error state; update the function startUpdate to log the error and then throw the caught error from upgradeP.goDownloadUpgrade so calling code can detect and handle the failure.
♻️ Duplicate comments (1)
src/renderer/src/stores/upgrade.ts (1)
39-39:⚠️ Potential issue | 🟡 MinorLocalize the fallback error message.
DEFAULT_UPDATE_ERRORstill flows intoupdateError, and that value is surfaced directly in settings/toast UI. Hardcoding English here bypasses localization for every fallback error path. Store an i18n key here, or translate at the display boundary instead of keeping raw text in the store.As per coding guidelines,
src/renderer/src/**/*.{vue,ts,tsx}must "Use vue-i18n keys fromsrc/renderer/src/i18nfor all user-facing strings; never hardcode UI text".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/stores/upgrade.ts` at line 39, DEFAULT_UPDATE_ERROR currently contains hardcoded English and is propagated into updateError which is shown in settings/toast UI; replace this raw string with an i18n key (or return the key here and translate at the display boundary) so the UI uses vue-i18n instead of hardcoded text. Update the constant DEFAULT_UPDATE_ERROR in the upgrade store to a sensible i18n key name and ensure any consumers of updateError (settings/toast components) call $t(...) when rendering, or alternatively perform the translation inside the store before exposing the value; reference DEFAULT_UPDATE_ERROR and updateError to locate the change.
🤖 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/stores/upgrade.ts`:
- Around line 99-108: The progress handler applyProgress currently writes
updateProgress.value without checking the stale-update guard
(externalMutationToken), so asynchronous syncFromPresenterStatus responses can
overwrite newer PROGRESS events; modify applyProgress to respect the same guard
used in applyStatus by comparing/using externalMutationToken (or delegating
progress updates through the existing guarded path), ensuring applyProgress only
applies progress when the token matches the latest presenter sync and preventing
older syncFromPresenterStatus responses from clobbering newer PROGRESS events;
update any other progress-handling functions (the other ranges mentioned) to use
the same guard logic.
- Around line 110-126: syncFromPresenterStatus() can throw if
upgradeP.getUpdateStatus() rejects, so update the function to catch errors and
avoid propagating them: wrap the await upgradeP.getUpdateStatus() (or the whole
function body) in a try/catch, log or surface the error as appropriate, and
return rawStatus.value on error; also update any fire-and-forget callers that do
"void syncFromPresenterStatus()" to either await the call or attach .catch() so
unhandled promise rejections are prevented. Ensure you reference the existing
symbols (syncFromPresenterStatus, upgradeP.getUpdateStatus, latestSyncRequestId,
externalMutationToken, rawStatus, applyStatus) when making the changes.
---
Outside diff comments:
In `@src/renderer/src/stores/upgrade.ts`:
- Around line 211-217: startUpdate currently swallows errors from
upgradeP.goDownloadUpgrade (returning false) so callers on the
'github'|'official' path never see failures; change startUpdate to not swallow
the failure—after logging the error, rethrow it (or return a rejected promise)
instead of returning false so callers awaiting startUpdate will receive the
failure and can enter the UI error state; update the function startUpdate to log
the error and then throw the caught error from upgradeP.goDownloadUpgrade so
calling code can detect and handle the failure.
---
Duplicate comments:
In `@src/renderer/src/stores/upgrade.ts`:
- Line 39: DEFAULT_UPDATE_ERROR currently contains hardcoded English and is
propagated into updateError which is shown in settings/toast UI; replace this
raw string with an i18n key (or return the key here and translate at the display
boundary) so the UI uses vue-i18n instead of hardcoded text. Update the constant
DEFAULT_UPDATE_ERROR in the upgrade store to a sensible i18n key name and ensure
any consumers of updateError (settings/toast components) call $t(...) when
rendering, or alternatively perform the translation inside the store before
exposing the value; reference DEFAULT_UPDATE_ERROR and updateError to locate the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48702755-fa6f-4613-a864-5fa8f3d6e112
📒 Files selected for processing (12)
src/renderer/settings/components/AboutUsSettings.vuesrc/renderer/src/i18n/da-DK/update.jsonsrc/renderer/src/i18n/fa-IR/update.jsonsrc/renderer/src/i18n/fr-FR/update.jsonsrc/renderer/src/i18n/he-IL/update.jsonsrc/renderer/src/i18n/ja-JP/update.jsonsrc/renderer/src/i18n/ko-KR/update.jsonsrc/renderer/src/i18n/pt-BR/update.jsonsrc/renderer/src/i18n/ru-RU/update.jsonsrc/renderer/src/stores/upgrade.tstest/renderer/components/AboutUsSettings.test.tstest/renderer/stores/upgradeStore.test.ts
✅ Files skipped from review due to trivial changes (4)
- src/renderer/src/i18n/ko-KR/update.json
- src/renderer/src/i18n/ja-JP/update.json
- src/renderer/settings/components/AboutUsSettings.vue
- test/renderer/stores/upgradeStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/renderer/src/i18n/pt-BR/update.json
- src/renderer/src/i18n/he-IL/update.json
- src/renderer/src/i18n/fa-IR/update.json
- test/renderer/components/AboutUsSettings.test.ts
- src/renderer/src/i18n/da-DK/update.json
- src/renderer/src/i18n/ru-RU/update.json
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
UI/UX Changes
Localization
Tests