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 significantly refactors the frontend architecture by introducing explicit creation and destruction functions for managers and stores, enhancing modularity and HMR compatibility. It also adds a mechanism for the frontend to request a full resend of UI layouts from the backend, addressing potential state loss during HMR. Minor improvements include converting a Rust enum to a macro for better maintainability and adding a new ESLint rule for stricter TypeScript. Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces significant architectural changes to the frontend, primarily focusing on improving resource management and Hot Module Replacement (HMR) compatibility. The refactoring shifts from returning manager/store instances with destroy methods to using module-scoped variables and explicit create and destroy functions. This centralizes resource cleanup in Editor.svelte's onDestroy lifecycle hook, which is a cleaner approach for managing subscriptions and event listeners during HMR re-mounts. The addition of ResendAllLayouts in the Rust backend and its invocation in the frontend ensures UI state consistency after HMR. The new define_layout_target! macro in Rust is a good improvement for enum definition maintainability. The ESLint rule addition for non-null assertions enhances type safety. Overall, these changes are well-considered and improve the robustness and maintainability of the application.
There was a problem hiding this comment.
4 issues found across 21 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/persistence.ts">
<violation number="1" location="frontend/src/managers/persistence.ts:44">
P2: Create and reuse one module-level `idb.createStore(...)` instead of recreating it in each function/handler.</violation>
</file>
<file name="frontend/src/managers/fonts.ts">
<violation number="1" location="frontend/src/managers/fonts.ts:9">
P2: The module-scoped AbortController is reused after being aborted, so fetches fail after manager teardown/recreation (including HMR).</violation>
</file>
<file name="frontend/src/stores/app-window.ts">
<violation number="1" location="frontend/src/stores/app-window.ts:32">
P2: Overwriting a single global `editorRef` makes store cleanup instance-unsafe; re-creating the store before teardown can leak old frontend message subscriptions.</violation>
</file>
<file name="frontend/src/managers/input.ts">
<violation number="1" location="frontend/src/managers/input.ts:555">
P1: Unconditional `app?.focus()` steals focus on every input event. In the original code, focus was only restored when `!canvasFocused && newInCanvasArea`. This line forces focus to the app container on every pointer move, key press, etc., breaking text inputs, menus, and all non-canvas interactions. Remove this line — the conditional `app?.focus()` inside the `if` block below handles the intended behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
No description provided.