Refactor the TypeScript data flow for full type safety and auto-generation of Rust types#3865
Refactor the TypeScript data flow for full type safety and auto-generation of Rust types#3865
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 focuses on improving the type safety of the Graphite editor by refactoring the TypeScript data flow and automating the generation of Rust types. This change ensures consistency between the frontend and backend, reduces manual effort, and enhances the overall maintainability of the project. Additionally, the removal of deprecated code and the update of dependencies contribute to a more streamlined and efficient codebase. Highlights
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.
Code Review
This pull request is an impressive and substantial refactoring that successfully achieves its goal of full type safety in the TypeScript data flow by replacing specta with tsify for auto-generating Rust types. The removal of the manual messages.ts file is a significant improvement for maintainability. The changes are extensive, touching many parts of the codebase on both the Rust and TypeScript sides, and have been executed with high quality.
The refactoring of Rust enums like LayoutGroup and the introduction of helper functions have improved the API's ergonomics. On the frontend, the transition to the new auto-generated types, the removal of any and as assertions, and the refactoring of components like WidgetSpan.svelte to use a registry pattern are all excellent changes that enhance robustness and scalability. I have reviewed the changes thoroughly and found no issues of medium or higher severity. This is a very well-executed and beneficial improvement to the codebase.
Note: Security Review did not run due to the size of the PR.
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
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="editor/src/messages/portfolio/document/document_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_message_handler.rs:2301">
P1: The "Transform Origin" checkbox reads `self.overlays_visibility_settings.pivot` instead of `self.overlays_visibility_settings.origin`. This ties the Origin overlay toggle to the Pivot state, so toggling Origin in the UI reflects and controls the wrong setting.</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.
2 issues found across 9 files (changes from recent commits).
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/README.md">
<violation number="1" location="frontend/src/README.md:5">
P3: Typo: duplicated article "a a TypeScript section" → "a TypeScript section".</violation>
</file>
<file name="frontend/wasm/README.md">
<violation number="1" location="frontend/wasm/README.md:11">
P3: Typo: "communcation" → "communication". The underlying filename has the same typo, so ideally rename the file too and update this reference.</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.
1 issue found across 6 files (changes from recent commits).
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/README.md">
<violation number="1" location="frontend/README.md:19">
P3: Grammar error: "it [ESLint] checks" has a dangling pronoun. Remove "it " so the sentence reads "When you use `npm run check`, [ESLint](https://eslint.org/) checks the code…".</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.
4 issues found across 12 files (changes from recent commits).
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="node-graph/libraries/no-std-types/Cargo.toml">
<violation number="1" location="node-graph/libraries/no-std-types/Cargo.toml:27">
P2: Removing the `dep:` prefix from optional dependencies exposes implicit standalone `tsify` and `wasm-bindgen` features, allowing them to be enabled independently of the `wasm` feature. This is inconsistent with the `std` feature in the same file which uses `dep:` (e.g., `"dep:serde"`), and it could lead to partial configurations where one dependency is enabled without the other.</violation>
</file>
<file name="node-graph/libraries/raster-types/Cargo.toml">
<violation number="1" location="node-graph/libraries/raster-types/Cargo.toml:12">
P1: Dropping `"core-types/wasm"` from the `wasm` feature means `core-types`'s `wasm` feature is never activated. No other crate in the workspace propagates it. This silently disables all `#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]` annotations in `core-types` (at least 6 types across `text.rs`, `types.rs`, `uuid.rs`, and `lib.rs`), so those types will not generate TypeScript bindings in wasm builds.</violation>
</file>
<file name="node-graph/libraries/vector-types/Cargo.toml">
<violation number="1" location="node-graph/libraries/vector-types/Cargo.toml:11">
P1: Removing `"core-types/wasm"` from the feature list means `core-types`'s `wasm` feature is no longer enabled by anyone in the workspace. The `#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]` annotations on types like `EditorId`, `Font`, and others in `core-types` will be inactive, so their TypeScript bindings won't be auto-generated.</violation>
</file>
<file name="node-graph/libraries/core-types/Cargo.toml">
<violation number="1" location="node-graph/libraries/core-types/Cargo.toml:14">
P2: Removing `no-std-types/wasm` from the feature list orphans the `wasm` feature of `no-std-types` — no crate in the workspace enables it anymore. The `#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]` annotations on blend mode and color types in `no-std-types` will no longer activate, so those types won't get TypeScript bindings generated via Tsify in WASM builds. If those types still need TS bindings, this feature propagation should be kept (or moved elsewhere). If they no longer need Tsify, the dead `cfg_attr` annotations should be cleaned up.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
f976746 to
21671ce
Compare
…ro to have optional icons; improve Svelte 5 configs
This reverts commit 6b71111.
21671ce to
e0c7c9d
Compare
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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="website/content/volunteer/guide/codebase-overview/debugging-tips.md">
<violation number="1" location="website/content/volunteer/guide/codebase-overview/debugging-tips.md:40">
P2: The advice to use debug builds for profiling is backwards. Debug builds have fundamentally different performance characteristics, so profiling them can surface bottlenecks that don't exist in release and miss ones that do. The standard practice is to profile release builds with debug symbols enabled (e.g. `[profile.release] debug = true`) so you get both accurate timings and readable stack traces.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ation of Rust types (#3865) * Migrate Specta to Tsify to auto-generate messages.ts, working except colors and widgets * Adopt the generated FillColor/Color/GradientStops * Fix widget typing * Separate WidgetGroup enum variants into wrapper structs * Small rename * Simplify widgets further * Clean up message type references * Switch type imports to the auto-generated file * Remove lowercase serde rename * Fix FillChoice deserialization * Fix small regression from #3837 * Improve type safety * Make WidgetSpan type-safe * More cleanup and type safety * More type safety * More type safety * Get the rest to type-check without errors; improve widget builder macro to have optional icons; improve Svelte 5 configs * Cargo fmt * Fix imports * Update outdated readme info * Fix lint command rename references * Fix typos * One more typos fix * Remove unnecessary dep: prefix from the edited Cargo.toml files * Remove excess parts from Cargo.toml * Fix compiling on desktop * Revert "Remove excess parts from Cargo.toml" This reverts commit 6b71111. * Update dev docs with simpler, more accurate instructions
…ation of Rust types (#3865) * Migrate Specta to Tsify to auto-generate messages.ts, working except colors and widgets * Adopt the generated FillColor/Color/GradientStops * Fix widget typing * Separate WidgetGroup enum variants into wrapper structs * Small rename * Simplify widgets further * Clean up message type references * Switch type imports to the auto-generated file * Remove lowercase serde rename * Fix FillChoice deserialization * Fix small regression from #3837 * Improve type safety * Make WidgetSpan type-safe * More cleanup and type safety * More type safety * More type safety * Get the rest to type-check without errors; improve widget builder macro to have optional icons; improve Svelte 5 configs * Cargo fmt * Fix imports * Update outdated readme info * Fix lint command rename references * Fix typos * One more typos fix * Remove unnecessary dep: prefix from the edited Cargo.toml files * Remove excess parts from Cargo.toml * Fix compiling on desktop * Revert "Remove excess parts from Cargo.toml" This reverts commit 6b71111. * Update dev docs with simpler, more accurate instructions
…ation of Rust types (#3865) * Migrate Specta to Tsify to auto-generate messages.ts, working except colors and widgets * Adopt the generated FillColor/Color/GradientStops * Fix widget typing * Separate WidgetGroup enum variants into wrapper structs * Small rename * Simplify widgets further * Clean up message type references * Switch type imports to the auto-generated file * Remove lowercase serde rename * Fix FillChoice deserialization * Fix small regression from #3837 * Improve type safety * Make WidgetSpan type-safe * More cleanup and type safety * More type safety * More type safety * Get the rest to type-check without errors; improve widget builder macro to have optional icons; improve Svelte 5 configs * Cargo fmt * Fix imports * Update outdated readme info * Fix lint command rename references * Fix typos * One more typos fix * Remove unnecessary dep: prefix from the edited Cargo.toml files * Remove excess parts from Cargo.toml * Fix compiling on desktop * Revert "Remove excess parts from Cargo.toml" This reverts commit 6b71111. * Update dev docs with simpler, more accurate instructions
Closes #1148
Fully removes messages.ts. Its contents are now fully auto-generated! Type safety is now also universal, with
anyandasbeing fully removed (except in one place unavoidable due to Svelte components, hopefully able to be improved when we upgrade to Svelte 5 runes mode), and the absence of both being enforced with ESLint rules. Replacesspectawithtsifysince the latter integrates seamlessly with serde-wasm-bindgen that already is used to generate our types on hot reload.