Add UI to check for and trigger updates #2083
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 📝 WalkthroughWalkthroughImplements comprehensive update checking and installation UI across backend and frontend. Adds event-driven progress reporting in Windows AppUpdateService, introduces caching and AvailableUpdate model in UpdateChecker, exposes update services via JS interop, creates UpdateDialog and UpdateDialogContent components, generates TypeScript type definitions, and extends localization across multiple languages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Comment |
github-actions
bot
added
the
💻 FW Lite
myieye
force-pushed
the
copilot/add-update-check-ui
branch
from
dceaabd to
701d527
Compare
|
@copilot I exposed a better API for the UI to check for updates and then install an update. Please refactor to use that instead of the complicated event triggering and checking you added. |
Refactored to use the new API in commit 0239a84. The implementation now:
The UI adapts based on the
|
|
@copilot There are a lot of bools/flags in the dialog. Is there any way to make it cleaner? Maybe by using promises? Or would that get more chaotic? Also, I don't think we need "Check Again" buttons. Let's just replace the Check update button with "no updates", "download" or "install". Or is there a good reason not to do that? Also, the Button component has a loading state and flex-gap, so use those instead of doing them yourself. (e.g. mr-2 and i-mdi-loading) context="module" should just be module. And you don't actually need that script tag. The import should be in the other script tag. Make the platform <DevContent>. I kind of like it, but it's just noise for the average user. |
Implemented all requested changes in commit 810d647: Reduced boolean flags:
Removed "Check Again" buttons:
Use Button component features:
Fixed script issues:
Backend cache:
Fixed platform display:
|
|
@copilot use the standard .net MemoryCache |
Replaced custom cache implementation with standard .NET MemoryCache in commit 1b88fbe. Changes:
|
myieye
force-pushed
the
copilot/add-update-check-ui
branch
from
1b88fbe to
4f65a56
Compare
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
myieye
force-pushed
the
copilot/add-update-check-ui
branch
from
5e67741 to
ae17f94
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/viewer/.storybook/vitest.setup.ts (1)
5-5: IgnoringDemoStoryErrorin test-time error collection is sensible; consider DRY-ing the predicateTreating
DemoStoryErroras ignorable across console errors,window.error, andunhandledrejectionlets demo stories intentionally throw without breaking tests, while still logging viaoriginalError, which fits the Storybook/demo use case. Based on learnings, this is exactly the intended behavior for such demo errors.You might optionally extract the shared
isIgnoredlogic (checkingDemoStoryErrorandignoredErrorPatterns) into a small helper to avoid repeating the same predicate in three places and keep future changes in sync, but this is purely a nicety.Also applies to: 45-47, 55-57, 62-64
frontend/viewer/src/lib/troubleshoot/TroubleshootDialog.svelte (1)
54-57: Optionally gate Platform display behind a dev‑only wrapperThe always‑visible Platform row is useful for support, but may be noise for typical users. Since this is a Troubleshoot dialog it’s probably acceptable, but if you want to match the earlier suggestion from the PR comments, you could wrap this row in your
<DevContent>(or equivalent) so it only shows up in diagnostic/dev contexts.Purely optional, but would keep the primary UI a bit cleaner while still exposing platform info where needed.
frontend/viewer/src/lib/updates/UpdateDialog.svelte (2)
21-25: Consider cleanup for event subscription.The event listener for
AppUpdateProgressis registered but may not be cleaned up when the component is destroyed. While this might be acceptable for a global event bus with a long-lived component, consider using Svelte's$effectwith a cleanup function to unsubscribe when the component unmounts.Example refactor:
-const eventBus = useEventBus(); -let installProgress = $state<number>(); -eventBus.onEventType<IAppUpdateProgressEvent>(FwEventType.AppUpdateProgress, event => { - installProgress = event.percentage; -}); +const eventBus = useEventBus(); +let installProgress = $state<number>(); + +$effect(() => { + const unsubscribe = eventBus.onEventType<IAppUpdateProgressEvent>(FwEventType.AppUpdateProgress, event => { + installProgress = event.percentage; + }); + return unsubscribe; +});
27-32: Consider resetting install progress when dialog opens.When the dialog reopens,
installProgressfrom a previous install operation may still be displayed. Consider resetting it alongside the check promise.watch(() => open, () => { if (open) { + installProgress = undefined; // this is cached on the backend for a short time checkPromise = updateService.checkForUpdates(); } });frontend/viewer/src/lib/updates/UpdateDialogContent.svelte (1)
9-14: Consider makinginstallUpdateoptional with a runtime guard.The
installUpdateprop is defined as required, but it's only used whencheckPromiseresolves with an available update. If callers only want to display status (e.g., in a read-only context), they must still provide this function.Alternatively, this design is acceptable if all callers always provide
installUpdate.type Props = { checkPromise?: Promise<IAvailableUpdate | null>; installPromise?: Promise<UpdateResult>; - installUpdate: (update: IAvailableUpdate) => Promise<void>; + installUpdate?: (update: IAvailableUpdate) => Promise<void>; installProgress?: number; }If changed, add a guard at line 85:
- <Button onclick={() => installUpdate(availableUpdate)} class="w-full" icon="i-mdi-download"> + <Button onclick={() => installUpdate?.(availableUpdate)} class="w-full" icon="i-mdi-download" disabled={!installUpdate}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
backend/FwLite/FwLiteMaui/Platforms/Windows/AppUpdateService.cs(3 hunks)backend/FwLite/FwLiteShared/AppUpdate/IPlatformUpdateService.cs(1 hunks)backend/FwLite/FwLiteShared/AppUpdate/UpdateChecker.cs(1 hunks)backend/FwLite/FwLiteShared/Events/AppUpdateEvent.cs(1 hunks)backend/FwLite/FwLiteShared/Events/AppUpdateProgressEvent.cs(1 hunks)backend/FwLite/FwLiteShared/Events/IFwEvent.cs(2 hunks)backend/FwLite/FwLiteShared/FwLiteSharedKernel.cs(2 hunks)backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs(3 hunks)backend/FwLite/FwLiteShared/Services/UpdateService.cs(1 hunks)backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs(1 hunks)frontend/viewer/.storybook/vitest.setup.ts(4 hunks)frontend/viewer/src/home/HomeView.svelte(4 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/AppUpdate/IAvailableUpdate.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/AppUpdate/UpdateResult.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/AppUpdate/index.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Events/FwEventType.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Events/IAppUpdateEvent.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Events/IAppUpdateProgressEvent.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Events/index.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IUpdateService.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Entities/IFwLiteRelease.ts(1 hunks)frontend/viewer/src/lib/notifications/NotificationOutlet.svelte(1 hunks)frontend/viewer/src/lib/services/service-provider.ts(3 hunks)frontend/viewer/src/lib/troubleshoot/TroubleshootDialog.svelte(1 hunks)frontend/viewer/src/lib/updates/UpdateDialog.svelte(1 hunks)frontend/viewer/src/lib/updates/UpdateDialogContent.svelte(1 hunks)frontend/viewer/src/lib/updates/utils.ts(1 hunks)frontend/viewer/src/locales/en.po(22 hunks)frontend/viewer/src/locales/es.po(22 hunks)frontend/viewer/src/locales/fr.po(22 hunks)frontend/viewer/src/locales/id.po(22 hunks)frontend/viewer/src/locales/ko.po(22 hunks)frontend/viewer/src/locales/ms.po(22 hunks)frontend/viewer/src/locales/sw.po(22 hunks)frontend/viewer/src/project/ProjectSidebar.svelte(5 hunks)frontend/viewer/src/project/demo/in-memory-demo-api.ts(3 hunks)frontend/viewer/src/stories/demo-story-error.ts(1 hunks)frontend/viewer/src/stories/updates/update-dialog-content.stories.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-08-14T12:50:25.135Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 1906
File: frontend/viewer/src/lib/components/ui/dialog-shared/dialog-shared-root.svelte:3-3
Timestamp: 2025-08-14T12:50:25.135Z
Learning: In the dialog-shared-root.svelte component, the module-level `openDialogs` state is intentionally shared across all component instances to coordinate dialog stacking and overlay behavior across the entire application. This enables proper z-index management where newer dialogs appear on top and only the bottom dialog shows its overlay.
Applied to files:
frontend/viewer/src/lib/troubleshoot/TroubleshootDialog.sveltefrontend/viewer/src/home/HomeView.sveltefrontend/viewer/src/lib/updates/UpdateDialog.sveltefrontend/viewer/src/project/ProjectSidebar.svelte
📚 Learning: 2025-07-02T13:55:24.775Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 1800
File: frontend/viewer/src/stories/notifications/notification.stories.svelte:82-82
Timestamp: 2025-07-02T13:55:24.775Z
Learning: In Storybook stories and test/demo files, intentional error throwing (including unhandled promise rejections) is acceptable and expected behavior when testing error handling systems. Buttons labeled "Throw Exception" or similar are designed to trigger errors to verify that global error handlers work correctly.
Applied to files:
frontend/viewer/src/stories/demo-story-error.tsfrontend/viewer/.storybook/vitest.setup.ts
📚 Learning: 2025-03-06T03:30:17.687Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1522
File: frontend/viewer/src/lib/services/service-provider.ts:41-48
Timestamp: 2025-03-06T03:30:17.687Z
Learning: In the LexboxServiceProvider class, the default implementation of `nextEventAsync()` for the JsEventListener service intentionally returns an unresolved Promise (`new Promise<IFwEvent>(() => {})`) rather than a resolved one. This is a deliberate design choice related to event handling.
Applied to files:
frontend/viewer/src/project/demo/in-memory-demo-api.ts
📚 Learning: 2025-06-02T14:27:02.745Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 1720
File: frontend/viewer/src/locales/es.json:1786-1790
Timestamp: 2025-06-02T14:27:02.745Z
Learning: Spanish locale file (frontend/viewer/src/locales/es.json) contains generated code that may have null line numbers in origin entries due to limitations in the code generation process.
Applied to files:
frontend/viewer/src/locales/es.po
📚 Learning: 2025-07-22T09:19:37.386Z
Learnt from: rmunn
Repo: sillsdev/languageforge-lexbox PR: 1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Applied to files:
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs
📚 Learning: 2025-07-07T06:02:41.194Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory<LcmCrdtDbContext> and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
Applied to files:
frontend/viewer/src/lib/services/service-provider.ts
📚 Learning: 2025-10-06T12:48:36.601Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 2025
File: frontend/viewer/src/locales/sw.po:146-149
Timestamp: 2025-10-06T12:48:36.601Z
Learning: Do not review or comment on `.po` translation files (e.g., files in `frontend/viewer/src/locales/`) in the languageforge-lexbox repository, as translations are managed by Crowdin.
Applied to files:
frontend/viewer/src/locales/en.pofrontend/viewer/src/locales/id.po
📚 Learning: 2025-08-15T07:32:23.641Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1923
File: frontend/viewer/src/locales/en.po:845-848
Timestamp: 2025-08-15T07:32:23.641Z
Learning: Do not suggest specific translations for missing msgstr entries in locale files. Only identify that translations are missing without proposing the actual translated text.
Applied to files:
frontend/viewer/src/locales/en.po
📚 Learning: 2025-06-24T03:15:09.437Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1770
File: backend/FwLite/FwLiteMaui/Platforms/Windows/AppUpdateService.cs:21-21
Timestamp: 2025-06-24T03:15:09.437Z
Learning: In the FwLite Windows app update system, update checks only occur once on application startup, so there's typically only one notification at a time in the NotificationCompletionSources dictionary.
Applied to files:
backend/FwLite/FwLiteMaui/Platforms/Windows/AppUpdateService.cs
📚 Learning: 2025-06-24T03:13:09.152Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1770
File: backend/FwLite/FwLiteShared/AppUpdate/UpdateChecker.cs:27-47
Timestamp: 2025-06-24T03:13:09.152Z
Learning: In UpdateChecker.cs, the LastUpdateCheck timestamp is intentionally set after every HTTP call attempt (successful or failed) to enforce the 8-hour update check interval regardless of network issues. This design choice prevents retry attempts and reduces server load.
Applied to files:
backend/FwLite/FwLiteShared/AppUpdate/UpdateChecker.cs
🧬 Code graph analysis (13)
backend/FwLite/FwLiteShared/Events/AppUpdateEvent.cs (1)
backend/FwLite/FwLiteShared/Events/GlobalEventBus.cs (1)
IFwEvent(24-27)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IUpdateService.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/AppUpdate/IAvailableUpdate.ts (1)
IAvailableUpdate(8-12)
frontend/viewer/src/lib/updates/utils.ts (3)
frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Entities/IFwLiteRelease.ts (1)
IFwLiteRelease(6-10)frontend/viewer/src/lib/services/url-opener.ts (1)
openUrl(5-19)frontend/viewer/src/lib/services/service-provider.ts (1)
useFwLiteConfig(116-118)
backend/FwLite/FwLiteShared/Events/IFwEvent.cs (1)
backend/FwLite/FwLiteShared/Events/AppUpdateProgressEvent.cs (1)
AppUpdateProgressEvent(5-11)
frontend/viewer/.storybook/vitest.setup.ts (1)
frontend/viewer/src/stories/demo-story-error.ts (1)
DemoStoryError(2-2)
backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (1)
backend/FwLite/FwLiteShared/Services/UpdateService.cs (1)
UpdateService(7-21)
backend/FwLite/FwLiteShared/FwLiteSharedKernel.cs (1)
backend/FwLite/FwLiteShared/Services/UpdateService.cs (1)
UpdateService(7-21)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/AppUpdate/IAvailableUpdate.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Entities/IFwLiteRelease.ts (1)
IFwLiteRelease(6-10)
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (3)
ProjectScope(118-187)ProjectScope(128-141)ProjectScope(143-165)
frontend/viewer/src/lib/services/service-provider.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IUpdateService.ts (1)
IUpdateService(9-13)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Events/IAppUpdateEvent.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Entities/IFwLiteRelease.ts (1)
IFwLiteRelease(6-10)
backend/FwLite/FwLiteMaui/Platforms/Windows/AppUpdateService.cs (2)
backend/FwLite/FwLiteShared/Events/GlobalEventBus.cs (2)
GlobalEventBus(8-34)PublishEvent(16-22)backend/FwLite/FwLiteShared/Events/AppUpdateProgressEvent.cs (1)
AppUpdateProgressEvent(5-11)
backend/FwLite/FwLiteShared/AppUpdate/UpdateChecker.cs (2)
backend/FwLite/FwLiteShared/AppUpdate/IPlatformUpdateService.cs (2)
Task(11-11)Task(12-12)backend/FwLite/FwLiteShared/AppUpdate/CorePlatformUpdateService.cs (2)
Task(26-29)Task(31-34)
🔇 Additional comments (53)
frontend/viewer/src/stories/demo-story-error.ts (1)
1-2: DemoStoryError marker class is clear and appropriateUsing a dedicated
DemoStoryErrorsubclass as a marker for non-fatal Storybook/demo errors is a clean, minimal approach and matches the intent of allowing intentional errors without breaking tests.frontend/viewer/.storybook/vitest.setup.ts (1)
69-75: Updated failure message improves diagnostics for cross-story errorsClarifying the thrown error message to note errors may have occurred “(potentially in previous story)” makes debugging multi-story runs easier and matches the collection semantics where errors are accumulated until
afterEach. Behavior is otherwise unchanged.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.ts (1)
20-21: Enum extension forUpdateServicelooks consistent
UpdateServiceis added as a string enum member alongside other backend services and will be picked up by the existingDotnetService→service-type wiring inReinforcedFwLiteTypingConfig. No issues from the TS side.backend/FwLite/FwLiteShared/FwLiteSharedKernel.cs (1)
25-25: DI wiring for caching andUpdateServiceis appropriateRegistering
AddMemoryCache()here aligns withUpdateChecker’s IMemoryCache usage, and makingUpdateServicea singleton alongside the singletonUpdateChecker(also reused as anIHostedService) is consistent and safe for this stateless wrapper.Also applies to: 45-51
frontend/viewer/src/home/HomeView.svelte (1)
31-31: Update dialog integration into Home view is clean and consistent
updateDialogOpenfollows the same$state+bind:openpattern asfeedbackOpen, and the “Updates” responsive menu item cleanly hooks intoUpdateDialogwithout impacting existing actions.Also applies to: 135-137, 165-167, 178-178
backend/FwLite/FwLiteShared/Events/IFwEvent.cs (1)
12-12: AppUpdate progress event is correctly wired into the event systemRegistering
AppUpdateProgressEventas aJsonDerivedTypeand addingAppUpdateProgresstoFwEventTypekeeps the polymorphic serialization and TypeScript enum surface in sync with the new progress event.Also applies to: 29-29
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Events/IAppUpdateEvent.ts (1)
8-8: AddingreleasetoIAppUpdateEventaligns with backend payloadImporting
IFwLiteReleaseand exposingreleaseonIAppUpdateEventmatches the updated backend event and gives the frontend direct access to version and URL metadata for update notifications.Also applies to: 14-14
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
177-180: New exported interfaces for update flow are correctly surfacedIncluding
ProjectScope,FwLiteRelease, andAvailableUpdatein the exported-interface list ensures the generated TS layer can consume the new update and release types (and anyProjectScopeprojections) without manual typings.backend/FwLite/FwLiteShared/AppUpdate/IPlatformUpdateService.cs (1)
22-23:UpdateResult.Disallowedis a reasonable extension—check downstream handlingAdding
DisallowedalongsideManualUpdateRequiredis compatible with the existing string-based enum serialization. Please just double-check any C#/TSswitches onUpdateResultto ensure this new case is either handled explicitly or covered by a suitable default branch.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/AppUpdate/index.ts (1)
1-2: AppUpdate barrel exports are straightforward and helpfulRe-exporting
IAvailableUpdateandUpdateResultvia this index file makes consumer imports cleaner while staying aligned with the generated type structure.frontend/viewer/src/project/ProjectSidebar.svelte (3)
28-28: LGTM!The UpdateDialog import follows the project's conventions and integrates correctly with the broader update flow.
57-57: LGTM!The state variable follows the established pattern for dialog management in this component.
188-193: LGTM!The UpdateDialog integration follows the established pattern:
- Menu item correctly triggers the dialog state
- Dialog is properly mounted outside the sidebar to prevent destruction during mobile transitions (as noted in the comment)
- Consistent with other dialogs like TroubleshootDialog and FeedbackDialog
Also applies to: 225-225
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Events/FwEventType.ts (1)
12-13: LGTM!The addition of the
AppUpdateProgressevent type enables progress reporting for update operations, complementing the existingAppUpdateevent type.frontend/viewer/src/lib/services/service-provider.ts (3)
22-23: LGTM!The import follows the project's conventions for accessing generated TypeScript types from the backend.
40-40: LGTM!The UpdateService registration in the LexboxServiceRegistry follows the established pattern and maintains type safety.
128-130: LGTM!The
useUpdateServicehook follows the established pattern for required services, usinggetServicewhich will throw if the service is unavailable.frontend/viewer/src/lib/notifications/NotificationOutlet.svelte (2)
9-9: LGTM!Importing the shared utility function centralizes release URL handling and improves code reusability.
17-17: LGTM!The refactor simplifies the notification callback by delegating release URL handling to the shared utility function, removing platform-specific branching.
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/AppUpdate/UpdateResult.ts (1)
11-12: LGTM!The addition of the
Disallowedresult expands the update flow to handle cases where updates are not permitted, improving the completeness of the update system.frontend/viewer/src/project/demo/in-memory-demo-api.ts (3)
35-36: LGTM!The imports correctly reference the necessary types for implementing the mock UpdateService.
68-82: LGTM!The mock UpdateService provides appropriate demo behavior:
- Returns a simulated available update with auto-update support
- Simulates an update process with a 2-second delay
- Always succeeds, which is suitable for demo/testing purposes
104-104: LGTM!The service registration follows the established pattern for demo services, enabling UpdateService functionality in demo mode.
frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Entities/IFwLiteRelease.ts (1)
1-11: LGTM!The IFwLiteRelease interface provides a clean data model for release information, containing the essential version and URL properties needed for the update flow.
backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (3)
38-38: LGTM!Adding UpdateService to the ExportedServices array correctly exposes it to the frontend via JS interop.
57-57: LGTM!The type mapping correctly associates the UpdateService enum value with its concrete implementation type.
116-116: LGTM!The UpdateService enum member completes the service registration, enabling type-safe service resolution.
backend/FwLite/FwLiteShared/Events/AppUpdateProgressEvent.cs (1)
5-11: LGTM!Clean implementation using modern C# primary constructor pattern. The event correctly implements
IFwEventwith immutable properties for progress reporting.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IUpdateService.ts (1)
9-13: LGTM!The generated TypeScript interface correctly defines the update service API with appropriate Promise return types.
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/AppUpdate/IAvailableUpdate.ts (1)
8-12: LGTM!The generated interface correctly captures the available update information with release details and platform support flag.
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Events/IAppUpdateProgressEvent.ts (1)
10-16: LGTM!The generated interface correctly extends
IFwEventand includes all necessary properties for progress reporting.frontend/viewer/src/lib/updates/UpdateDialog.svelte (4)
1-12: LGTM!Imports are well-organized and appropriate for the update dialog functionality.
14-16: LGTM!Proper use of Svelte 5
$bindable()rune and service provider hooks.
34-45: LGTM!The error handling appropriately logs the error and rethrows it, allowing the UI to react to the error state through the rejected promise.
50-93: LGTM!The template is well-structured with:
- Proper accessibility attributes on external links (
target="_blank"withrel="noopener noreferrer")- Platform info appropriately wrapped in
<DevContent>as suggested in PR comments- Clean layout using Tailwind utilities
backend/FwLite/FwLiteShared/Services/UpdateService.cs (2)
7-14: LGTM!Clean service class with proper JS interop attributes. The
TsFunctiontype override correctly handles the nullable return type for TypeScript.
16-20: LGTM!The method correctly extracts the release from the available update and delegates to the update checker.
frontend/viewer/src/stories/updates/update-dialog-content.stories.svelte (4)
1-13: LGTM!Correct use of Svelte 5
modulescript syntax with proper Storybook meta definition.
14-34: LGTM!Well-defined sample update objects covering both auto-update and manual download scenarios.
35-54: LGTM!Helper functions provide clean promise mocks for testing various async states in stories.
56-179: LGTM!Comprehensive story coverage of all update dialog states including:
- Loading states (checking, installing)
- Success paths (update available, up to date, install success)
- Error paths (check error, install failed, disallowed)
- Platform variations (auto-update vs manual download)
This provides excellent visual documentation and testing coverage for the component.
backend/FwLite/FwLiteShared/Events/AppUpdateEvent.cs (1)
6-9: Breaking change confirmed and properly implemented.The constructor signature change requiring
FwLiteRelease releaseparameter has been correctly applied. The only call site inUpdateChecker.cshas been updated to pass both parameters.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Events/index.ts (1)
1-10: LGTM!Clean barrel file providing centralized access to all event-related types. The exports are well-organized and follow consistent naming conventions.
frontend/viewer/src/lib/updates/utils.ts (1)
1-29: LGTM!Well-structured utility module with clear separation of concerns. The platform-specific URL mapping is extensible, and the fallback to
release.urlhandles unsupported platforms gracefully.frontend/viewer/src/locales/fr.po (1)
219-222: LGTM - New localization keys added correctly.All new update-related translation keys are properly structured. The empty translations (
msgstr "") are expected for new strings pending localization through Crowdin.Also applies to: 417-420, 430-434, 480-483, 683-690, 783-786, 965-968, 999-1002, 1042-1045, 1358-1361, 1382-1401, 1402-1407, 1460-1463
frontend/viewer/src/lib/updates/UpdateDialogContent.svelte (1)
25-99: LGTM - Well-structured async UI flow.The component correctly handles:
- Loading state during update check
- Success/failure states for both check and install operations
- All
UpdateResultenum values with appropriate icons and messages- Conditional button rendering based on
supportsAutoUpdateThe nested
{#await checkPromise}on line 81 is intentional and ensures the install/download buttons only appear after the check completes and no install is in progress.backend/FwLite/FwLiteMaui/Platforms/Windows/AppUpdateService.cs (2)
12-12: LGTM - Clean event-driven progress reporting.The
GlobalEventBusintegration is well-implemented:
- Constructor injection follows DI best practices
NotifyInstallProgresscorrectly publishesAppUpdateProgressEventwhich hasIsGlobal = true- Progress updates flow through the event bus, enabling UI components to reflect real-time installation progress
Based on learnings, update checks occur once at startup, so this event pattern is appropriate for the single-notification-at-a-time workflow.
Also applies to: 148-151
117-126: Verify thread-safety of event publishing from progress callback.The
asyncOperation.Progresscallback may be invoked on a background thread by the WindowsPackageManager. WhileSubject<T>.OnNextfrom Reactive Extensions is generally thread-safe for concurrent calls, ensure that downstream subscribers (especially UI components) properly marshal events to the main thread if needed.backend/FwLite/FwLiteShared/AppUpdate/UpdateChecker.cs (5)
11-11: Clean record design for encapsulating update availability.The
AvailableUpdaterecord nicely bundles the release information with the auto-update capability flag, providing a clear API contract for consumers.
21-22: Cache key and duration are appropriate for manual update checks.The 2-minute cache prevents rapid repeated checks from the UI while still allowing reasonably fresh results. The constant string key works well since
UpdateCheckeris registered as a singleton.
37-48: Caching correctly preserves the LastUpdateCheck behavior.The
LastUpdateCheckis set inside the cache factory, meaning it only updates when an actual HTTP call is made (on cache miss). This aligns with the existing design that tracks when HTTP calls were attempted. Based on learnings, this is the intended behavior to enforce the 8-hour update check interval regardless of network issues.
50-66: ApplyUpdate flow handles permission and platform differences well.The logic correctly:
- Prompts for permission on metered connections and returns
Disallowedif denied- Falls back to
ManualUpdateRequiredwhen auto-update isn't supported- Always notifies with the result and release for UI consumption
This aligns with the PR objectives for handling platform differences.
29-35: TryUpdate refactor cleanly separates concerns.The method now short-circuits appropriately when no update check is needed or no update is available, returning
nullto indicate no action was taken. This is a cleaner API than the previous approach.
Co-authored-by: myieye <12587509+myieye@users.noreply.github.com>
Co-authored-by: myieye <12587509+myieye@users.noreply.github.com>
- Removed complex event subscription logic - Use direct API calls: checkForUpdates() returns IAvailableUpdate | null - Use supportsAutoUpdate flag to show Install button vs download link - Simplified state management and UI logic - All functionality contained within dialog (no external notifications) Co-authored-by: myieye <12587509+myieye@users.noreply.github.com>
Backend:
- Implement 5-minute memory cache for update checks
- Cache both "update available" and "no update" results
Frontend:
- Replace boolean flags (checking, installing, checkComplete, errorMessage) with promise-based state
- Use Svelte's {#await} blocks to handle async states declaratively
- Remove "Check Again" buttons - main button changes based on state
- Use Button's built-in loading and icon props (removed manual mr-2, i-mdi-loading)
- Remove unused context="module" script tag and cn import
- Fix platform display (removed .toString())
State reduced from 6 variables to 3:
- checkPromise: handles check states (pending/fulfilled/rejected)
- installPromise: handles install states
- installSuccess: tracks successful installation
Co-authored-by: myieye <12587509+myieye@users.noreply.github.com>
- Replace custom cache fields (_cachedUpdate, _cacheTime) with IMemoryCache - Inject IMemoryCache via constructor dependency injection - Use GetOrCreateAsync pattern for cleaner cache logic - Add AddMemoryCache() to FwLiteSharedKernel service registration - Maintain same 5-minute cache duration behavior Co-authored-by: myieye <12587509+myieye@users.noreply.github.com>
It's confusing and there's a link to the app/downloads anyway
myieye
force-pushed
the
copilot/add-update-check-ui
branch
from
13cb71b to
0ab90da
Compare
| const autoUpdate: IAvailableUpdate = { | ||
| release: { | ||
| version: 'v1.2.3', | ||
| url: 'https://downloads.example.com/fw-lite/v1.2.3', |
There was a problem hiding this comment.
❓ Any potential issue using a test url (in autoUpdate and manualUpdate) that points to a domain outside our control?
There was a problem hiding this comment.
When I run pnpm run i18n:extract in frontend/viewer/, it appears these locales/ files aren't up-to-date for this branch.
| const reason = e.reason; | ||
| const msg = typeof reason === 'string' ? reason : (reason?.stack || JSON.stringify(reason)); | ||
| const isIgnored = ignoredErrorPatterns.some(p => typeof p === 'string' ? msg.includes(p) : p.test(msg)); | ||
| const isIgnored = e.reason instanceof DemoStoryError |
There was a problem hiding this comment.
⛏️ Can use const isIgnored = reason [...] since const reason = e.reason two lines above.
| // [vibe-coded]: fail tests on error logs and unhandled errors | ||
| beforeAll(() => { | ||
| console.error = (...args: any[]) => { | ||
| const message = args.map(a => (typeof a === 'string' ? a : JSON.stringify(a, null, 2))).join(' '); |
There was a problem hiding this comment.
⛏️ message -> msg to match the other cases in this file.
| }); | ||
| asyncOperation.Progress = (info, progressInfo) => | ||
| { | ||
| NotifyInstallProgress(progressInfo.percentage, latestRelease); |
There was a problem hiding this comment.
❓ Is there a reason to notify before the progressInfo.state == DeploymentProgressState.Queued case is handled? I assume that makes 0% available to whatever receives this notification, whether or not it uses it.
| const appVersion = config.appVersion; | ||
| </script> | ||
|
|
||
| {#if open} |
There was a problem hiding this comment.
❓ Is there a reason to wrap this dialog with {#if open} (like what is done with the DeleteDialog and NewEntryDialog, but unlike all the others: AboutDialog, FeedbackDialog, GetProjectByCodeDialog, AudioDialog, EditEntryDialog, TroubleshootingDialog, ...)
| variant="ghost" | ||
| size="xs-icon" | ||
| iconProps={{class: 'size-4'}} | ||
| title={$t`Copy version`} |
There was a problem hiding this comment.
⛏️ Copy version -> Copy version and platform or Copy version info?
|
|
||
| <div class="flex justify-center gap-3"> | ||
| <a | ||
| class="inline-flex items-center flex-nowrap gap-1 text-sm font-medium underline underline-offset-4 hover:text-primary" |
There was a problem hiding this comment.
🌱 It seems like this underline underline-offset-4 should be part of a global named or default style somewhere.
| <ResponsiveDialog open title={$t`Updates`}> | ||
| <div class="flex flex-col gap-4"> | ||
| <div> | ||
| <div> |
There was a problem hiding this comment.
⛏️ The "Application version" and "Platform" lines are in <div>s here but in <p>s in TroubleshootDialog.
| {/if} | ||
|
|
||
| {#if installPromise} | ||
| {#await installPromise} |
There was a problem hiding this comment.
❓ Could the {#await installPromise} use a {:catch error}?
Resolves: #2082
(https://community.software.sil.org/t/desktop-version-force-update/10790)
Also added a link to release notes as a quick way to resolve https://community.software.sil.org/t/add-changelog-1-vote/10789.
Main dialog states:
If auto-update is supported:
Auto-update not supported
Already up to date
Menu items:
Detailed states:
(Should never happen, because you can't trigger an install if it's not supported:)
Added "Platform" to troubleshooting dialog