Conversation
github-actions
bot
added
the
waiting for review
There was a problem hiding this comment.
Pull request overview
Refactors the quote-related modals (search/rate/report/submit/approve) from legacy DOM-driven modal implementations to SolidJS modal components backed by centralized modal/state stores.
Changes:
- Added SolidJS modal components for quote workflows and registered them in the global modals mount.
- Introduced new Solid state modules for quote rating/reporting and rewired callers to use them.
- Removed legacy quote modal TS modules + corresponding HTML dialog markup and SCSS styling; adjusted shared UI components (Button, SlimSelect) to support the new modal UX.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/ts/test/test-logic.ts | Switches quote-rate reset to new state module. |
| frontend/src/ts/test/result.ts | Uses new getQuoteStats from quote-rate state. |
| frontend/src/ts/states/quote-report.ts | New state + entrypoint to open QuoteReport modal. |
| frontend/src/ts/states/quote-rate.ts | New state + API helpers for quote rating/stats and opening QuoteRate modal. |
| frontend/src/ts/states/modals.ts | Extends ModalId union to include quote modal IDs. |
| frontend/src/ts/modals/quote-submit.ts | Removes legacy quote submit modal implementation. |
| frontend/src/ts/modals/quote-search.ts | Removes legacy quote search modal implementation. |
| frontend/src/ts/modals/quote-report.ts | Removes legacy quote report modal implementation. |
| frontend/src/ts/modals/quote-rate.ts | Removes legacy quote rate modal implementation. |
| frontend/src/ts/modals/quote-filter.ts | Removes legacy quote filter modal implementation (replaced by SimpleModal usage). |
| frontend/src/ts/modals/quote-approve.ts | Removes legacy quote approve modal implementation. |
| frontend/src/ts/modals/mobile-test-config.ts | Opens new QuoteSearch modal from mobile test config. |
| frontend/src/ts/event-handlers/test.ts | Rewires quote search/rate/report button handlers to new modal/state APIs. |
| frontend/src/ts/components/ui/SlimSelect.tsx | Ensures SlimSelect dropdown renders within a local container (contentLocation). |
| frontend/src/ts/components/modals/QuoteSubmitModal.tsx | New Solid QuoteSubmit modal. |
| frontend/src/ts/components/modals/QuoteSearchModal.tsx | New Solid QuoteSearch modal (includes submit/approve modals). |
| frontend/src/ts/components/modals/QuoteReportModal.tsx | New Solid QuoteReport modal. |
| frontend/src/ts/components/modals/QuoteRateModal.tsx | New Solid QuoteRate modal. |
| frontend/src/ts/components/modals/QuoteApproveModal.tsx | New Solid QuoteApprove modal. |
| frontend/src/ts/components/modals/Modals.tsx | Mounts the new quote modal components. |
| frontend/src/ts/components/common/Button.tsx | Updates event handler typings to receive the MouseEvent (enables stopPropagation usage). |
| frontend/src/ts/commandline/lists.ts | Rewires quote search command to open new QuoteSearch modal. |
| frontend/src/styles/popups.scss | Removes legacy quote modal styles. |
| frontend/src/html/popups.html | Removes legacy quote modal dialog markup. |
| return; | ||
| } | ||
| void QuoteReportModal.show(TestWords.currentQuote?.id); | ||
| showQuoteReportModal(TestWords.currentQuote?.id); |
There was a problem hiding this comment.
TestWords.currentQuote is already null-checked above, but optional chaining makes the arg type number | undefined, which fails under strictNullChecks and can break compilation. Pass TestWords.currentQuote.id (or change showQuoteReportModal to accept/guard undefined).
| showQuoteReportModal(TestWords.currentQuote?.id); | |
| showQuoteReportModal(TestWords.currentQuote.id); |
| @@ -126,9 +126,7 @@ async function setup(modalEl: ElementWithUtils): Promise<void> { | |||
| TestLogic.restart(); | |||
| } | |||
| } else if (lenAttr === "-2") { | |||
There was a problem hiding this comment.
This opens the new Solid modal via states/modals, but mobile-test-config is still a legacy utils/animated-modal dialog, so it won't be hidden/"chained" and you can end up with multiple dialogs open/stacked. Hide/close the legacy modal before showing QuoteSearch (or provide a bridge that restores the previous modal on close).
| } else if (lenAttr === "-2") { | |
| } else if (lenAttr === "-2") { | |
| void modal.hide(); |
| }; | ||
|
|
||
| const applyQuote = (quoteId: number): void => { | ||
| if (isNaN(quoteId) || quoteId < 0) { |
There was a problem hiding this comment.
Validation allows quoteId === 0 but the error text says "at least 1". This should reject 0 as well (and ideally align with the legacy behavior everywhere quote IDs are accepted).
| if (isNaN(quoteId) || quoteId < 0) { | |
| if (isNaN(quoteId) || quoteId < 1) { |
| createEffect( | ||
| on(lengthFilter, (lengths) => { | ||
| if (lengths.includes("4") && !hasCustomFilter()) { | ||
| showSimpleModal({ | ||
| title: "Enter minimum and maximum number of words", | ||
| inputs: [ | ||
| { type: "number", placeholder: "1" }, | ||
| { type: "number", placeholder: "100" }, | ||
| ], | ||
| buttonText: "save", | ||
| execFn: async (min: string, max: string) => { | ||
| const minNum = parseInt(min, 10); | ||
| const maxNum = parseInt(max, 10); | ||
| if (isNaN(minNum) || isNaN(maxNum)) { | ||
| return { status: "notice", message: "Invalid min/max values" }; | ||
| } | ||
| setCustomFilterMin(minNum); | ||
| setCustomFilterMax(maxNum); | ||
| setHasCustomFilter(true); | ||
| return { status: "success", message: "Saved custom filter" }; | ||
| }, | ||
| }); | ||
| } | ||
| }), |
There was a problem hiding this comment.
When the user selects the "custom" length filter, a SimpleModal is shown, but there's no handling for dismiss/backdrop/escape. If the modal is closed without saving, lengthFilter can keep "4" selected while hasCustomFilter stays false, so the UI shows custom selected but it has no effect. Consider clearing "custom" from lengthFilter on cancel/dismiss, or track a pending custom config and only add "4" after successful save.
| const handleBeforeShow = (): void => { | ||
| const quote = currentQuote(); | ||
| if (!quote) return; | ||
| setRating(0); | ||
| setHoverRating(0); | ||
| const snapshot = DB.getSnapshot(); | ||
| const alreadyRated = snapshot?.quoteRatings?.[quote.language]?.[quote.id]; | ||
| if (isSafeNumber(alreadyRated)) { | ||
| setRating(alreadyRated); | ||
| } | ||
| void getQuoteStats(quote); | ||
| }; |
There was a problem hiding this comment.
getQuoteStats(quote) is fired-and-forgotten. If the user submits quickly (esp. when updating an existing rating), quoteStats() may still be null/{}, causing incorrect ratings/totalRating/average calculations and displaying "0" ratings. Consider awaiting the fetch in beforeShow, or disabling submit until stats are loaded (or ensure the submit path fetches stats if missing).
No description provided.