Conversation
|
Deploying openshockapp with
|
| Latest commit: |
8cfd1cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://07fef459.openshockapp.pages.dev |
| Branch Preview URL: | https://feature-replace-dialog-manag.openshockapp.pages.dev |
![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: c3288ebf36
ℹ️ 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".
| </script> | ||
|
|
||
| {#if $OwnHubsStore == null} | ||
| {#if ownHubs.size === 0} |
There was a problem hiding this comment.
The new loading guard uses ownHubs.size === 0, so accounts that legitimately have zero hubs (or any failed fetch that leaves the map empty) are stuck on a perpetual "Loading..." state and never see the empty-state UI. This check conflates "still loading" with "loaded but empty"; use an explicit loading flag instead of map size.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR replaces the existing confirm-dialog/store approach with a new promise-based dialog manager, and migrates several app state modules (SignalR + hubs state) to Svelte 5 runes ($state, SvelteMap) with corresponding updates across routes and components.
Changes:
- Introduces a new dialog manager (
dialog.open/confirm/alert) and migrates hub/share flows to use it. - Refactors hubs state into a runes-based
HubsStore.svelte.tsand updates all consumers. - Refactors SignalR state into runes-based module state with getter APIs and updates all callers.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/Footer.svelte | Switches SignalR UI indicator to getConnectionState(). |
| src/routes/+layout.svelte | Replaces old confirm dialog manager with new DialogManager. |
| src/routes/(authenticated)/shockers/shared/+page.svelte | Migrates online hub lookup to onlineHubs SvelteMap. |
| src/routes/(authenticated)/shockers/own/+page.svelte | Migrates hubs source to ownHubs SvelteMap and adjusts loading logic. |
| src/routes/(authenticated)/shares/user/outgoing/edit-share.svelte | Migrates delete confirmation to dialog.confirm(). |
| src/routes/(authenticated)/shares/user/invites/outgoing-invite-item.svelte | Migrates cancel-invite confirmation to dialog.confirm(). |
| src/routes/(authenticated)/shares/user/invites/incoming-invite-item.svelte | Migrates deny-invite confirmation to dialog.confirm(). |
| src/routes/(authenticated)/shares/user/incoming/manage-share.svelte | Migrates incoming-share removal confirmation to dialog.confirm(). |
| src/routes/(authenticated)/shares/user/incoming/incoming-share-item.svelte | Migrates remove-share confirmation to dialog.confirm(). |
| src/routes/(authenticated)/shares/user/dialog-share-code-create.svelte | Uses ownHubs SvelteMap for available shockers list. |
| src/routes/(authenticated)/shares/user/+layout.svelte | Imports refreshOwnHubs from new runes store module. |
| src/routes/(authenticated)/shares/public/[shareId=guid]/edit/dialog-add-shocker.svelte | Uses ownHubs SvelteMap for shocker selection. |
| src/routes/(authenticated)/shares/public/[shareId=guid]/edit/+page.svelte | Imports refreshOwnHubs from new runes store module. |
| src/routes/(authenticated)/hubs/dialog-hub-regenerate-token.svelte | Removes dedicated dialog component (moved into actions/snippets). |
| src/routes/(authenticated)/hubs/dialog-hub-pair.svelte | Removes dedicated dialog component (moved into actions/snippets). |
| src/routes/(authenticated)/hubs/dialog-hub-edit.svelte | Removes dedicated dialog component (moved into actions/snippets). |
| src/routes/(authenticated)/hubs/dialog-hub-delete.svelte | Removes dedicated dialog component (moved into actions/snippets). |
| src/routes/(authenticated)/hubs/dialog-hub-create.svelte | Removes dedicated dialog component (replaced by dialog-manager snippet). |
| src/routes/(authenticated)/hubs/data-table-actions.svelte | Inlines hub dialogs as dialog-manager snippets; updates SignalR connection usage. |
| src/routes/(authenticated)/hubs/columns.ts | Simplifies to type exports; changes firmware version typing to string. |
| src/routes/(authenticated)/hubs/[hubId=guid]/update/+page.svelte | Migrates to new SignalR getters + runes-based hubs/online state. |
| src/routes/(authenticated)/hubs/+page.svelte | Replaces DataTable with manual Table + dialog-manager create-hub flow. |
| src/routes/(anonymous)/logout/+page.ts | Updates SignalR teardown import path. |
| src/routes/(anonymous)/login/+page.svelte | Updates SignalR init import path. |
| src/lib/stores/HubsStore.ts | Removes legacy Svelte store implementation. |
| src/lib/stores/HubsStore.svelte.ts | Adds runes-based hubs store using SvelteMap + HubOnlineState class. |
| src/lib/stores/ConfirmDialogStore.ts | Removes legacy confirm dialog store. |
| src/lib/signalr/index.svelte.ts | Refactors SignalR state/connection to $state + getters; removes store exports. |
| src/lib/signalr/handlers/OtaRollback.ts | Migrates online hub updates to onlineHubs SvelteMap. |
| src/lib/signalr/handlers/OtaInstallSucceeded.ts | Migrates online hub updates to onlineHubs SvelteMap. |
| src/lib/signalr/handlers/OtaInstallStarted.ts | Migrates online hub updates to onlineHubs SvelteMap. |
| src/lib/signalr/handlers/OtaInstallProgress.ts | Migrates online hub updates to onlineHubs SvelteMap. |
| src/lib/signalr/handlers/OtaInstallFailed.ts | Migrates online hub updates to onlineHubs SvelteMap. |
| src/lib/signalr/handlers/DeviceUpdate.ts | Updates import for refreshOwnHubs from new runes store module. |
| src/lib/signalr/handlers/DeviceStatus.ts | Migrates status updates to HubOnlineState instances in onlineHubs. |
| src/lib/components/dialog-manager/types.ts | Adds dialog-manager type system for alert/confirm/custom dialogs. |
| src/lib/components/dialog-manager/dialog-store.svelte.ts | Adds promise-based dialog store and dialog creation helpers. |
| src/lib/components/dialog-manager/dialog-manager.svelte | Adds global dialog renderer for the dialog queue. |
| src/lib/components/dialog-manager/dialog-custom-content.svelte | Adds snippet-based custom dialog content renderer. |
| src/lib/components/dialog-manager/dialog-confirm-content.svelte | Adds built-in confirm dialog UI content. |
| src/lib/components/dialog-manager/dialog-alert-content.svelte | Adds built-in alert dialog UI content. |
| src/lib/components/confirm-dialog/dialog-manager.svelte | Removes legacy confirm dialog manager component. |
| src/lib/components/confirm-dialog/dialog-confirm.svelte | Removes legacy confirm dialog component. |
| src/lib/components/ControlModules/SimpleControlModule.svelte | Switches SignalR usage to getConnection(). |
| src/lib/components/ControlModules/SharedShockerControlModule.svelte | Switches SignalR usage to getConnection(). |
| src/lib/components/ControlModules/RichControlModule.svelte | Switches SignalR usage to getConnection(). |
| src/lib/components/ControlModules/MapControlModule.svelte | Switches SignalR usage to getConnection(). |
| src/lib/components/ControlModules/ClassicControlModule.svelte | Switches SignalR usage to getConnection(). |
| src/hooks.client.ts | Updates SignalR init import path. |
Comments suppressed due to low confidence (1)
src/lib/signalr/index.svelte.ts:85
- If
connection.start()throws,connectionremains non-null, so subsequentinitializeSignalR()calls will early-return and never retry connecting. Consider settingconnection = null(and possibly disposing handlers) in the catch block so a later call can attempt reconnection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {#if ownHubs.size === 0} | ||
| <p>Loading...</p> | ||
| {:else} |
There was a problem hiding this comment.
ownHubs.size === 0 is used as a loading check, but an empty map can also mean the user simply has no hubs/shockers. This will leave the page stuck on "Loading..." for users with 0 hubs. Introduce an explicit loading flag (e.g., set true before refreshOwnHubs() and false in a finally) and render an empty-state when loading is false and the list is empty.
| @@ -0,0 +1,35 @@ | |||
| <script lang="ts"> | |||
| import * as Dialog from '$lib/components/ui/dialog'; | |||
| import { getOldestDialog } from './dialog-store.svelte.ts'; | |||
There was a problem hiding this comment.
Importing ./dialog-store.svelte.ts is inconsistent with the rest of the codebase, which imports .svelte.ts modules via the .svelte specifier (e.g. $lib/stores/HubsStore.svelte, $lib/components/dialog-manager/dialog-store.svelte). Consider switching this to ./dialog-store.svelte for consistency and to avoid resolver/tooling edge cases.
| import { getOldestDialog } from './dialog-store.svelte.ts'; | |
| import { getOldestDialog } from './dialog-store.svelte'; |
| setTimeout(() => removeDialog(id), 150); | ||
| resolve(result); | ||
| }; | ||
|
|
||
| dialogs.set(id, contextFactory(wrappedResolve) as DialogContext); |
There was a problem hiding this comment.
createDialog() removes dialogs after a hard-coded 150ms delay, but the dialog UI uses duration-200 / open/close state-driven animations. Also, close/resolve from dialog content never toggles the Dialog.Root open state to false, so the “closed” animation and any close lifecycle (e.g. focus return) may not run. Consider: (1) driving open false first, then removing after the animation duration (or on animation end), and (2) aligning the timeout with the actual dialog transition duration.
| setTimeout(() => removeDialog(id), 150); | |
| resolve(result); | |
| }; | |
| dialogs.set(id, contextFactory(wrappedResolve) as DialogContext); | |
| // Drive the dialog's open state to false so close animations and lifecycle run, | |
| // then remove it after the transition duration. | |
| const existingContext = dialogs.get(id) as DialogContext & { open?: boolean } | undefined; | |
| if (existingContext) { | |
| existingContext.open = false; | |
| dialogs.set(id, existingContext as DialogContext); | |
| } | |
| // Match the timeout to the dialog's close transition duration (e.g. duration-200). | |
| setTimeout(() => removeDialog(id), 200); | |
| resolve(result); | |
| }; | |
| const context = contextFactory(wrappedResolve) as DialogContext & { open?: boolean }; | |
| // Ensure dialogs start in the open state so the UI can animate them in. | |
| context.open = true; | |
| dialogs.set(id, context as DialogContext); |
No description provided.