Remove surface and window from ApplicationIo#3941
Remove surface and window from ApplicationIo#3941timon-schelling wants to merge 7 commits intomasterfrom
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 refactors the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring to remove window and surface management from ApplicationIo, preparing for asset database integration. The changes are extensive and consistent across the codebase, successfully decoupling the application I/O from platform-specific windowing. The introduction of CanvasSurfaceCache is a good encapsulation of wasm-specific rendering logic.
I've found one critical issue in the new canvas caching logic that could lead to a panic, and I've provided a fix. I also have a minor suggestion to improve code clarity in the same area. Otherwise, the changes look solid and well-executed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 15 files
Confidence score: 3/5
- Manual
Send/Syncassertion forCanvasSurfaceineditor/src/node_graph_executor/runtime.rsweakens compiler thread-safety guarantees around DOM/GPU handles, which could lead to subtle concurrency bugs. CanvasFramehashing innode-graph/graph-craft/src/document/value.rsignoresresolution, so different frame sizes may collide and degrade memoization or cache correctness.- Score reflects medium-severity, user-impacting risks rather than clear merge-blockers, so it’s somewhat safe but worth addressing.
- Pay close attention to
editor/src/node_graph_executor/runtime.rsandnode-graph/graph-craft/src/document/value.rs- thread-safety invariants and hashing/memoization correctness.
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/node_graph_executor/runtime.rs">
<violation number="1" location="editor/src/node_graph_executor/runtime.rs:704">
P1: Avoid manually asserting `Send`/`Sync` for `CanvasSurface` without a stronger invariant; this removes compiler-enforced thread-safety guarantees for DOM/GPU handles.
(Based on your team's feedback about use derive and safe code before unsafe.) [FEEDBACK_USED]</violation>
<violation number="2" location="editor/src/node_graph_executor/runtime.rs:722">
P2: Handle `undefined`/`null` from `Reflect::get` when initializing `window.imageCanvases`; checking only `is_err()` misses the common “missing property” case.</violation>
</file>
<file name="node-graph/graph-craft/src/document/value.rs">
<violation number="1" location="node-graph/graph-craft/src/document/value.rs:507">
P2: `CanvasFrame` hashing ignores `resolution`, making distinct frame sizes hash identically and weakening memoization/cache keys.
(Based on your team's feedback about avoiding misleading or inconsistent hashing implementations.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Performance Benchmark Results
|
|
The perf benchmarks look very suspicious |
|
I think this is nothing related to this PR but the result of a failure during the last run. |
timon-schelling
force-pushed
the
remove-window-from-app-io
branch
from
ad00530 to
09cfa10
Compare
timon-schelling
force-pushed
the
remove-window-from-app-io
branch
2 times, most recently
from
bbaa8e1 to
9c46db7
Compare
is now equivalent to PlatformApplicationIo::new
timon-schelling
force-pushed
the
remove-window-from-app-io
branch
from
1a78100 to
503a4db
Compare
There was a problem hiding this comment.
7 issues found across 36 files
Confidence score: 2/5
- There is a clear runtime regression risk in
node-graph/graph-craft/src/application_io/native.rs: two high-confidenceunwrap()calls can panic when optional GPU/application I/O state isNone, which is user-facing and likely to break execution paths. node-graph/graph-craft/src/application_io/native.rsalso skips HTTP status validation, so 4xx/5xx responses can be misread as valid resource bytes, increasing the chance of downstream failures and confusing behavior.- Additional correctness issues (hash discriminant omission in
node-graph/graph-craft/src/document/value.rsand adapter probe handling innode-graph/graph-craft/src/application_io/wasm.rs) add uncertainty even though they are less severe than the panic paths. - Pay close attention to
node-graph/graph-craft/src/application_io/native.rs,node-graph/graph-craft/src/document/value.rs,node-graph/graph-craft/src/application_io/wasm.rs- panic handling, response validation, and executor initialization logic need hardening before merge.
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/graph-craft/src/application_io/native.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/native.rs:94">
P2: HTTP status codes are not validated; 4xx/5xx responses are treated as successful resource bytes. Call `error_for_status()` before reading the body.</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/native.rs:117">
P1: This `unwrap()` can panic because `EditorApi.application_io` is nullable (`Option`) and defaults to `None`.
(Based on your team's feedback about preferring `if let`/safe handling over unnecessary `unwrap` and making invalid states unrepresentable.) [FEEDBACK_USED]</violation>
<violation number="3" location="node-graph/graph-craft/src/application_io/native.rs:123">
P1: This `unwrap()` can panic when WGPU is unavailable because `gpu_executor` is intentionally optional.
(Based on your team's feedback about preferring `if let`/safe handling over unnecessary `unwrap` and making invalid states unrepresentable.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/document/value.rs">
<violation number="1" location="node-graph/graph-craft/src/document/value.rs:498">
P2: Include the enum discriminant in `RenderOutputType::hash` before hashing fields to avoid avoidable cross-variant collisions.
(Based on your team's feedback about avoiding misleading or inconsistent hashing implementations.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/wasm.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/wasm.rs:26">
P2: The `requestAdapter` probe returns a nested `Option`, so a failed `call0` is treated as success and still triggers `WgpuExecutor::new()`.</violation>
</file>
<file name="node-graph/libraries/canvas-utils/src/lib.rs">
<violation number="1" location="node-graph/libraries/canvas-utils/src/lib.rs:1">
P3: Use `//!` for module-level docs instead of `///!` so the file-level documentation is attached and rendered correctly.</violation>
</file>
<file name="node-graph/libraries/application-io/src/lib.rs">
<violation number="1" location="node-graph/libraries/application-io/src/lib.rs:137">
P3: The `[`PlatformEditorApi`]` intra-doc link won't resolve because that type alias is defined in the downstream `graph-craft` crate, not in this crate. Either use a plain-text reference without link syntax, or provide a full path if one exists.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| #[cfg(feature = "wgpu")] | ||
| impl<'a> From<&'a NativeApplicationIo> for &'a WgpuExecutor { | ||
| fn from(app_io: &'a NativeApplicationIo) -> Self { | ||
| app_io.gpu_executor.as_ref().unwrap() |
There was a problem hiding this comment.
P1: This unwrap() can panic when WGPU is unavailable because gpu_executor is intentionally optional.
(Based on your team's feedback about preferring if let/safe handling over unnecessary unwrap and making invalid states unrepresentable.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/graph-craft/src/application_io/native.rs, line 123:
<comment>This `unwrap()` can panic when WGPU is unavailable because `gpu_executor` is intentionally optional.
(Based on your team's feedback about preferring `if let`/safe handling over unnecessary `unwrap` and making invalid states unrepresentable.) </comment>
<file context>
@@ -0,0 +1,125 @@
+#[cfg(feature = "wgpu")]
+impl<'a> From<&'a NativeApplicationIo> for &'a WgpuExecutor {
+ fn from(app_io: &'a NativeApplicationIo) -> Self {
+ app_io.gpu_executor.as_ref().unwrap()
+ }
+}
</file context>
2 participants
Preparation for asset database integration.