Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the legacy dev options modal into the SolidJS modal system, moving dev controls into the component-based modal stack and updating the dev overlay entry points accordingly.
Changes:
- Replace legacy
dev-optionsmodal implementation with a SolidDevOptionsModalcomponent. - Add a dev-only overlay button group to open Dev Options / server configure.
- Remove legacy dynamic-import loader + old dev button CSS.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/ts/utils/async-modules.ts | Removes dynamic-import helper previously used to load the dev options modal. |
| frontend/src/ts/stores/modals.ts | Extends ModalId union with DevOptions for the new Solid modal. |
| frontend/src/ts/modals/dev-options.ts | Deletes legacy modal implementation (non-Solid). |
| frontend/src/ts/index.ts | Removes dev-only bootstrapping that dynamically injected legacy dev buttons. |
| frontend/src/ts/components/modals/Modals.tsx | Registers the new DevOptionsModal in the Solid modal tree. |
| frontend/src/ts/components/modals/DevOptionsModal.tsx | New Solid modal containing dev actions (quick login, debug toggles, etc.). |
| frontend/src/ts/components/layout/overlays/Overlays.tsx | Adds dev-only overlay buttons to open Dev Options and backend configure. |
| frontend/src/styles/core.scss | Removes styling for the old injected #devButtons container. |
| @@ -10,6 +11,7 @@ export function Modals(): JSXElement { | |||
| <VersionHistoryModal /> | |||
| <ContactModal /> | |||
| <SupportModal /> | |||
| <DevOptionsModal /> | |||
| </> | |||
There was a problem hiding this comment.
DevOptionsModal is now always imported + rendered in Modals, which ships dev-only actions (quick login, test toggles) in production bundles and makes them theoretically triggerable (e.g. via any future call to showModal("DevOptions")). Gate rendering behind isDevEnvironment() and preferably lazy-load the modal component (dynamic import) to keep dev-only code out of prod bundles.
| ).then(() => { | ||
| hideLoaderBar(); | ||
| }); |
There was a problem hiding this comment.
Quick Login ignores the signIn return value (it can resolve with { success: false, message }) and never surfaces failures to the user. Handle the resolved result (notify on failure) and ensure hideLoaderBar() runs via finally so the loader can’t get stuck if an unexpected exception occurs.
| ).then(() => { | |
| hideLoaderBar(); | |
| }); | |
| ) | |
| .then((result: unknown) => { | |
| // Handle signIn returning { success: boolean, message?: string } | |
| if ( | |
| result && | |
| typeof result === "object" && | |
| "success" in result && | |
| !(result as { success: boolean }).success | |
| ) { | |
| const message = | |
| (result as { message?: string }).message ?? | |
| "Quick login failed."; | |
| Notifications.add(message, -1); | |
| } | |
| }) | |
| .catch((error) => { | |
| // Surface unexpected failures | |
| Notifications.add("Quick login failed.", -1, { | |
| details: error, | |
| }); | |
| }) | |
| .finally(() => { | |
| hideLoaderBar(); | |
| }); |
| <Show when={isDevEnvironment()}> | ||
| <div class="fixed top-30 left-0 z-10000 flex w-max flex-col gap-2 text-xs"> | ||
| <Button | ||
| href={`${envConfig.backendUrl}/configure/`} | ||
| ariaLabel={{ | ||
| text: "Configure server", | ||
| position: "right", | ||
| }} | ||
| fa={{ | ||
| icon: "fa-server", | ||
| }} | ||
| class="rounded-tl-none rounded-bl-none p-2" | ||
| /> | ||
| <Button | ||
| ariaLabel={{ | ||
| text: "Dev options", | ||
| position: "right", | ||
| }} | ||
| onClick={() => showModal("DevOptions")} | ||
| fa={{ | ||
| icon: "fa-flask", | ||
| }} | ||
| class="rounded-tl-none rounded-bl-none p-2" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
After moving dev options to Solid modals, the legacy popup DOM + styles for #devOptionsModal still exist (e.g. src/html/popups.html + styles/popups.scss) but there’s no longer any TS code referencing it. Remove the unused legacy modal markup/styles to avoid dead UI + maintenance overhead.
|
@copilot resolve all review comments |
|
Continuous integration check(s) failed. Please review the failing check's logs and make the necessary changes. |
|
@copilot fix lint |
| Notifications.add("Quick login failed.", -1, { | ||
| details: error, | ||
| }); |
There was a problem hiding this comment.
Notifications.add expects details?: object | string, but the catch param is typed unknown and passed through as-is (details: error). This won’t type-check and can also produce non-serializable / unhelpful details at runtime. Narrow/convert error before passing (eg error instanceof Error ? { message: error.message, stack: error.stack } : String(error)), or use the existing error-message helper.
| Notifications.add("Quick login failed.", -1, { | |
| details: error, | |
| }); | |
| const details = | |
| error instanceof Error | |
| ? { message: error.message, stack: error.stack } | |
| : String(error); | |
| Notifications.add("Quick login failed.", -1, { details }); |
| import { JSXElement, Show, lazy } from "solid-js"; | ||
|
|
||
| import { isDevEnvironment } from "../../utils/misc"; | ||
| import { ContactModal } from "./ContactModal"; | ||
| import { SupportModal } from "./SupportModal"; | ||
| import { VersionHistoryModal } from "./VersionHistoryModal"; | ||
|
|
||
| const DevOptionsModal = lazy(() => | ||
| import("./DevOptionsModal").then((m) => ({ default: m.DevOptionsModal })), | ||
| ); | ||
|
|
||
| export function Modals(): JSXElement { | ||
| return ( | ||
| <> | ||
| <VersionHistoryModal /> | ||
| <ContactModal /> | ||
| <SupportModal /> | ||
| <Show when={isDevEnvironment()}> | ||
| <DevOptionsModal /> | ||
| </Show> |
There was a problem hiding this comment.
lazy() suspends while the module loads, but this repo doesn’t appear to use <Suspense> anywhere (including where <Modals /> is mounted). Without a Suspense boundary, Solid’s lazy components can throw/promises can bubble to the root. Either render DevOptionsModal via a normal import (it’s already dev-gated), or wrap it in a <Suspense fallback={null}> (and add the import).
No description provided.