Fix frontend HMR so the page doesn't break upon saving TS files and restructure frontend architecture#3871
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural refactor to improve the frontend's modularity, stability, and hot module replacement (HMR) experience. It separates concerns by introducing Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
6 issues found across 54 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/window/Workspace.svelte">
<violation number="1" location="frontend/src/components/window/Workspace.svelte:112">
P2: `abortResize` also calls `removeListeners()` but doesn't clear `activeResizeCleanup`. After a resize abort (via Escape or right-click), the `onDestroy` handler will still hold a stale reference. Add `activeResizeCleanup = undefined` in `abortResize` to keep the cleanup tracking consistent.</violation>
</file>
<file name="frontend/src/stores/fullscreen.ts">
<violation number="1" location="frontend/src/stores/fullscreen.ts:39">
P2: `update()` is used here only to read the current state — the callback returns `state` unmodified. Because Svelte's `safe_not_equal` treats all objects as changed, this triggers a spurious subscriber notification on every toggle. Use `get(store)` to read the value without notifying subscribers, which also eliminates the explicit `Promise` constructor antipattern.
Note: `get` needs to be added to the `svelte/store` import.</violation>
</file>
<file name="frontend/src/managers/fonts.ts">
<violation number="1" location="frontend/src/managers/fonts.ts:17">
P2: Missing `response.ok` check. If the API returns a non-2xx status, `response.json()` may succeed with an error body that lacks `items`, producing a confusing `TypeError` instead of a clear HTTP error.</violation>
<violation number="2" location="frontend/src/managers/fonts.ts:44">
P1: Missing `response.ok` check before consuming the response body. If the font URL returns an HTTP error (e.g., 404), the error page body will be silently passed to `onFontLoad` as font data, corrupting the backend state.</violation>
</file>
<file name="frontend/src/stores/dialog.ts">
<violation number="1" location="frontend/src/stores/dialog.ts:80">
P2: `fetch` can reject on network errors, skipping the `requestLicensesThirdPartyDialogWithLicenseText` call entirely. Wrap the fetch in a try/catch so the pre-defined fallback `licenseText` is actually used.</violation>
</file>
<file name="frontend/src/stores/document.ts">
<violation number="1" location="frontend/src/stores/document.ts:73">
P2: Missing `await tick()` before the store update — every other `subscribeLayoutUpdate` handler in this file awaits `tick()` first to let Svelte flush pending DOM updates before patching the layout. This one appears to be an accidental omission.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Code Review
The pull request refactors the frontend architecture by introducing a new "managers" and "stores" pattern, replacing the previous "I/O managers" and "state providers." This change involves renaming modules, updating imports and type definitions across many Svelte components, and implementing explicit destroy() methods for resource cleanup, which are integrated with Svelte's onDestroy and Hot Module Replacement (HMR) for improved lifecycle management. A high-severity path traversal vulnerability was identified in frontend/src/editor.ts, where unsanitized URL hash data could lead to arbitrary file access. Additionally, a medium-severity performance concern was raised in frontend/src/components/widgets/WidgetSpan.svelte due to the removal of a debouncer, potentially causing excessive messages during drag interactions.
5bb6104 to
52d2b38
Compare
e58c1de to
df8001f
Compare
9dc0c5f to
7be6803
Compare
05c5187 to
fcc53f5
Compare
There was a problem hiding this comment.
2 issues found across 54 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/managers/hyperlink.ts">
<violation number="1" location="frontend/src/managers/hyperlink.ts:11">
P2: Opening a new tab with `"_blank"` should include `noopener`/`noreferrer` to prevent reverse-tabnabbing via `window.opener`.</violation>
</file>
<file name="frontend/src/managers/persistence.ts">
<violation number="1" location="frontend/src/managers/persistence.ts:190">
P2: `findIndex` can return `-1` when `currentDocumentId` is not in `orderedSavedDocuments` (e.g., stale IDB state). In that case the code opens all documents via the second loop but then calls `selectDocument` with an ID that was never opened. Guard against `currentIndex === -1` by falling through to the `else` branch.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
No description provided.