Implement content-addressed resource storage for raster images#4148
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a persistent resource storage system for Graphite, replacing the practice of embedding raw image data directly within the node graph. It implements a ResourceStorage trait with specialized backends: IndexedDbResourceStorage for web environments and MmapResourceStorage for desktop, utilizing Blake3 for content hashing. Key architectural changes include a preprocessor that automatically swaps resource hashes for dedicated resource nodes and updates to the document save/load flow to handle externalized assets. Feedback focuses on scalability and stability concerns, specifically the memory-intensive hydration of the entire IndexedDB store on startup, the use of expect() and panic! which could lead to application crashes, and the inefficient re-initialization of HTTP clients during resource loading.
| self.resize(); | ||
|
|
||
| self.desktop_wrapper.init(self.wgpu_context.clone()); | ||
| let resource_storage = MmapResourceStorage::new(dirs::app_resources_dir()).expect("Failed to initialize on-disk resource storage"); |
Contributor
There was a problem hiding this comment.
Using .expect() here will cause the application to crash immediately if the resources directory cannot be created or accessed (e.g., due to permission issues). While this occurs at startup, it would be better to handle this error gracefully and show a message to the user, especially since this directory is critical for the new resource system.
| async fn load_resource<'a: 'n>(_: impl Ctx, _primary: (), #[scope("editor-api")] _editor: &'a PlatformEditorApi, #[name("URL")] url: String) -> Arc<[u8]> { | ||
| let placeholder = || -> Arc<[u8]> { Arc::from(Vec::<u8>::new()) }; | ||
|
|
||
| let response = match reqwest::Client::new().get(&url).send().await { |
Contributor
There was a problem hiding this comment.
Creating a new reqwest::Client for every execution of the load_resource node is inefficient. reqwest clients are intended to be reused to benefit from internal connection pooling and to avoid the overhead of re-initializing the client state for every request.
Suggested change
| let response = match reqwest::Client::new().get(&url).send().await { | |
| let response = match _editor.application_io.as_ref().map(|io| io.http_client()).unwrap_or_else(reqwest::Client::new).get(&url).send().await { |
Comment on lines
+254
to
+256
| application_io.load_resource(&hash).unwrap_or_else(|| { | ||
| panic!("Resource {hash} not found"); | ||
| }) |
Contributor
There was a problem hiding this comment.
Using panic! inside a node is generally discouraged as it will crash the entire node graph executor and potentially the application. If a resource is missing (e.g., if it was manually deleted from the disk), it would be better to return an empty Resource or a placeholder image and log a warning instead of crashing.
application_io.load_resource(&hash).unwrap_or_else(|| {
log::error!("Resource {hash} not found");
Resource::new(Vec::new())
})
timon-schelling
force-pushed
the
content-hash-based-resources
branch
3 times, most recently
from
c902384 to
31b7b33
Compare
Keavon
force-pushed
the
master
branch
2 times, most recently
from
4b7a823 to
847b8e9
Compare
timon-schelling
force-pushed
the
master
branch
2 times, most recently
from
15fcaac to
d5f0140
Compare
timon-schelling
force-pushed
the
content-hash-based-resources
branch
from
058da30 to
8200b15
Compare
timon-schelling
force-pushed
the
content-hash-based-resources
branch
2 times, most recently
from
35eec4d to
3c27e65
Compare
timon-schelling
force-pushed
the
content-hash-based-resources
branch
from
1ff1dbd to
1b08a3a
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
19 issues found across 44 files
Confidence score: 2/5
- Merge risk is high because several high-confidence 8/10 issues are user-impacting: panic paths in
node-graph/nodes/gstd/src/platform_application_io.rsandnode-graph/graph-craft/src/application_io.rscan crash resource loading/render flows when resources or hashes are missing. - There is concrete document integrity risk across
node-graph/graph-craft/src/application_io/resource/indexed_db.rsandeditor/src/messages/resource/resource_message_handler.rs: reads/exports depend on in-memory cache and can omit resources that still exist only in IndexedDB, leading to broken saves. - Migration and persistence paths also look fragile in
editor/src/messages/portfolio/document_migration.rsandeditor/src/messages/portfolio/portfolio_message_handler.rs, where legacy shapes/state cleanup can be skipped and leave incompatible or stale persisted data. - Pay close attention to
node-graph/nodes/gstd/src/platform_application_io.rs,node-graph/graph-craft/src/application_io/resource/indexed_db.rs, andeditor/src/messages/resource/resource_message_handler.rs- crash and data-loss behaviors are the main merge blockers.
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/wasm.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/wasm.rs:1">
P1: Custom agent: **PR title enforcement**
PR title is not in imperative mood and lacks a leading action verb</violation>
</file>
<file name="editor/src/messages/portfolio/document/utility_types/embedded_resources.rs">
<violation number="1" location="editor/src/messages/portfolio/document/utility_types/embedded_resources.rs:47">
P2: Serializing the `HashMap` directly makes saved document output nondeterministic. Sort the resource entries before writing them so identical documents serialize identically.</violation>
</file>
<file name="node-graph/libraries/application-io/src/lib.rs">
<violation number="1" location="node-graph/libraries/application-io/src/lib.rs:155">
P2: Hash the shared `Io` allocation here, not the `Arc` handle's address. As written, `Arc::clone` of the same `application_io` produces different `EditorApi` hashes and can cause avoidable cache misses.
(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.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io.rs:68">
P1: Don't `expect` on `resources` here. `new()`/`default()` leave it unset, and some call sites (for example the CLI) construct `PlatformApplicationIo` without calling `inject_resources()`, so loading a content-hash resource can abort the whole process.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/dispatcher.rs">
<violation number="1" location="editor/src/dispatcher.rs:42">
P2: `with_executor()` now leaves the resource subsystem uninitialized, so non-test callers built through this constructor will drop `ResourceMessage`s and can panic when a resource handle is requested.
(Based on your team's feedback about making invalid states unrepresentable in data models.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource/mmap.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource/mmap.rs:56">
P2: Verify the mapped bytes still hash to the requested `ResourceHash` before returning them.</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/resource/mmap.rs:107">
P1: Don't report a successful resource hash after the filesystem write failed. Otherwise callers can store a hash that this backend can never load.</violation>
</file>
<file name="node-graph/nodes/gstd/src/platform_application_io.rs">
<violation number="1" location="node-graph/nodes/gstd/src/platform_application_io.rs:140">
P2: Use an actual transparent image placeholder here; empty bytes make failed loads disappear instead of preserving the transparent fallback promised by this node.</violation>
<violation number="2" location="node-graph/nodes/gstd/src/platform_application_io.rs:142">
P2: This now treats every resource as an HTTP URL, so local asset paths documented for this node will always fail.</violation>
<violation number="3" location="node-graph/nodes/gstd/src/platform_application_io.rs:265">
P1: Don't panic when the resource backend or hash is missing; this node can crash the whole render path on ordinary load failures.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource.rs:12">
P2: This `Mutex` is unnecessary for the in-memory store; it only adds contention and panic-on-poison paths to code that could use a plain `HashMap`.
(Based on your team's feedback about reusing existing abstractions instead of adding mutexes.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/application.rs">
<violation number="1" location="editor/src/application.rs:12">
P2: `Editor::new()` now returns a partially initialized editor and depends on a separate `replace_application_io()` call before graph execution is safe.
(Based on your team's feedback about placing integrity-enforcing initialization where the state is created.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/messages/portfolio/document_migration.rs">
<violation number="1" location="editor/src/messages/portfolio/document_migration.rs:1594">
P1: Handle the legacy 1-input image node shape here too; after the alias remap, older embedded-image documents skip this migration and keep an incompatible input layout.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/utility_types.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:304">
P1: This rewires the bitmap layer to a hash before the resource write can fail safely. If `ResourceMessage::Write` hits an I/O error, the node still keeps the hash-only reference and later resource loading panics when that hash is missing.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/messages/resource/resource_message_handler.rs">
<violation number="1" location="editor/src/messages/resource/resource_message_handler.rs:54">
P1: The non-test default creates an unusable `ResourceMessageHandler`: it can panic in `resources()` or silently export no resources during save.
(Based on your team's feedback about avoiding panics and making invalid states unrepresentable.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/messages/portfolio/portfolio_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/portfolio_message_handler.rs:452">
P1: This GC fallback loads and selects another document, then aborts the cleanup pass for legacy persisted state entries.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| fn load_resource(&self, hash: ResourceHash) -> graphene_application_io::ResourceFuture { | ||
| self.resources.as_ref().expect("Resource storage not initialized").load(hash) |
Contributor
There was a problem hiding this comment.
P1: Don't expect on resources here. new()/default() leave it unset, and some call sites (for example the CLI) construct PlatformApplicationIo without calling inject_resources(), so loading a content-hash resource can abort the whole process.
(Based on your team's feedback about avoiding panics in application code.)
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.rs, line 68:
<comment>Don't `expect` on `resources` here. `new()`/`default()` leave it unset, and some call sites (for example the CLI) construct `PlatformApplicationIo` without calling `inject_resources()`, so loading a content-hash resource can abort the whole process.
(Based on your team's feedback about avoiding panics in application code.) </comment>
<file context>
@@ -1,14 +1,93 @@
+ }
+
+ fn load_resource(&self, hash: ResourceHash) -> graphene_application_io::ResourceFuture {
+ self.resources.as_ref().expect("Resource storage not initialized").load(hash)
+ }
+}
</file context>
| })(); | ||
|
|
||
| if let Err(error) = write_result { | ||
| log::error!("Failed to write resource to {path:?}: {error}"); |
Contributor
There was a problem hiding this comment.
P1: Don't report a successful resource hash after the filesystem write failed. Otherwise callers can store a hash that this backend can never load.
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/resource/mmap.rs, line 107:
<comment>Don't report a successful resource hash after the filesystem write failed. Otherwise callers can store a hash that this backend can never load.</comment>
<file context>
@@ -0,0 +1,160 @@
+ })();
+
+ if let Err(error) = write_result {
+ log::error!("Failed to write resource to {path:?}: {error}");
+ let _ = fs::remove_file(&tmp);
+ }
</file context>
|
|
||
| #[node_macro::node(category(""))] | ||
| pub async fn resource<'a: 'n>(_: impl Ctx, _primary: (), #[scope("editor-api")] editor_api: &'a PlatformEditorApi, hash: ResourceHash) -> Resource { | ||
| let application_io = editor_api.application_io.as_ref().expect("ApplicationIo must be available when using resources"); |
Contributor
There was a problem hiding this comment.
P1: Don't panic when the resource backend or hash is missing; this node can crash the whole render path on ordinary load failures.
(Based on your team's feedback about avoiding panics in application code.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gstd/src/platform_application_io.rs, line 265:
<comment>Don't panic when the resource backend or hash is missing; this node can crash the whole render path on ordinary load failures.
(Based on your team's feedback about avoiding panics in application code.) </comment>
<file context>
@@ -254,3 +259,34 @@ where
+
+#[node_macro::node(category(""))]
+pub async fn resource<'a: 'n>(_: impl Ctx, _primary: (), #[scope("editor-api")] editor_api: &'a PlatformEditorApi, hash: ResourceHash) -> Resource {
+ let application_io = editor_api.application_io.as_ref().expect("ApplicationIo must be available when using resources");
+ application_io.load_resource(hash).await.unwrap_or_else(|| {
+ panic!("Resource {hash} not found");
</file context>
| async fn load_resource<'a: 'n>(_: impl Ctx, _primary: (), #[scope("editor-api")] _editor: &'a PlatformEditorApi, #[name("URL")] url: String) -> Arc<[u8]> { | ||
| let placeholder = || -> Arc<[u8]> { Arc::from(Vec::<u8>::new()) }; | ||
|
|
||
| let response = match reqwest::Client::new().get(&url).send().await { |
Contributor
There was a problem hiding this comment.
P2: This now treats every resource as an HTTP URL, so local asset paths documented for this node will always fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gstd/src/platform_application_io.rs, line 142:
<comment>This now treats every resource as an HTTP URL, so local asset paths documented for this node will always fail.</comment>
<file context>
@@ -137,26 +136,32 @@ fn image_to_bytes(_: impl Ctx, image: List<Raster<CPU>>) -> List<u8> {
+async fn load_resource<'a: 'n>(_: impl Ctx, _primary: (), #[scope("editor-api")] _editor: &'a PlatformEditorApi, #[name("URL")] url: String) -> Arc<[u8]> {
+ let placeholder = || -> Arc<[u8]> { Arc::from(Vec::<u8>::new()) };
+
+ let response = match reqwest::Client::new().get(&url).send().await {
+ Ok(response) => response,
+ Err(error) => {
</file context>
| let Ok(data) = data.await else { | ||
| return Arc::from(include_bytes!("../../../graph-craft/src/null.png").to_vec()); | ||
| async fn load_resource<'a: 'n>(_: impl Ctx, _primary: (), #[scope("editor-api")] _editor: &'a PlatformEditorApi, #[name("URL")] url: String) -> Arc<[u8]> { | ||
| let placeholder = || -> Arc<[u8]> { Arc::from(Vec::<u8>::new()) }; |
Contributor
There was a problem hiding this comment.
P2: Use an actual transparent image placeholder here; empty bytes make failed loads disappear instead of preserving the transparent fallback promised by this node.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gstd/src/platform_application_io.rs, line 140:
<comment>Use an actual transparent image placeholder here; empty bytes make failed loads disappear instead of preserving the transparent fallback promised by this node.</comment>
<file context>
@@ -137,26 +136,32 @@ fn image_to_bytes(_: impl Ctx, image: List<Raster<CPU>>) -> List<u8> {
- let Ok(data) = data.await else {
- return Arc::from(include_bytes!("../../../graph-craft/src/null.png").to_vec());
+async fn load_resource<'a: 'n>(_: impl Ctx, _primary: (), #[scope("editor-api")] _editor: &'a PlatformEditorApi, #[name("URL")] url: String) -> Arc<[u8]> {
+ let placeholder = || -> Arc<[u8]> { Arc::from(Vec::<u8>::new()) };
+
+ let response = match reqwest::Client::new().get(&url).send().await {
</file context>
Suggested change
| let placeholder = || -> Arc<[u8]> { Arc::from(Vec::<u8>::new()) }; | |
| let placeholder = || -> Arc<[u8]> { Arc::from(include_bytes!("../../../graph-craft/src/null.png").to_vec()) }; |
| @@ -0,0 +1,47 @@ | |||
| #[cfg(target_family = "wasm")] | |||
Contributor
There was a problem hiding this comment.
P2: This Mutex is unnecessary for the in-memory store; it only adds contention and panic-on-poison paths to code that could use a plain HashMap.
(Based on your team's feedback about reusing existing abstractions instead of adding mutexes.)
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/resource.rs, line 12:
<comment>This `Mutex` is unnecessary for the in-memory store; it only adds contention and panic-on-poison paths to code that could use a plain `HashMap`.
(Based on your team's feedback about reusing existing abstractions instead of adding mutexes.) </comment>
<file context>
@@ -0,0 +1,47 @@
+
+#[derive(Debug, Default)]
+pub struct HashMapResourceStorage {
+ resources: Mutex<HashMap<ResourceHash, Resource>>,
+}
+
</file context>
Contributor
There was a problem hiding this comment.
6 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/graph-craft/src/application_io/wasm.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/wasm.rs:1">
P1: Custom agent: **PR title enforcement**
PR title is not in imperative mood and lacks a leading action verb</violation>
</file>
<file name="editor/src/messages/portfolio/document/utility_types/embedded_resources.rs">
<violation number="1" location="editor/src/messages/portfolio/document/utility_types/embedded_resources.rs:47">
P2: Serializing the `HashMap` directly makes saved document output nondeterministic. Sort the resource entries before writing them so identical documents serialize identically.</violation>
</file>
<file name="node-graph/libraries/application-io/src/lib.rs">
<violation number="1" location="node-graph/libraries/application-io/src/lib.rs:155">
P2: Hash the shared `Io` allocation here, not the `Arc` handle's address. As written, `Arc::clone` of the same `application_io` produces different `EditorApi` hashes and can cause avoidable cache misses.
(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.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io.rs:68">
P1: Don't `expect` on `resources` here. `new()`/`default()` leave it unset, and some call sites (for example the CLI) construct `PlatformApplicationIo` without calling `inject_resources()`, so loading a content-hash resource can abort the whole process.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/dispatcher.rs">
<violation number="1" location="editor/src/dispatcher.rs:42">
P2: `with_executor()` now leaves the resource subsystem uninitialized, so non-test callers built through this constructor will drop `ResourceMessage`s and can panic when a resource handle is requested.
(Based on your team's feedback about making invalid states unrepresentable in data models.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource/mmap.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource/mmap.rs:56">
P2: Verify the mapped bytes still hash to the requested `ResourceHash` before returning them.</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/resource/mmap.rs:107">
P1: Don't report a successful resource hash after the filesystem write failed. Otherwise callers can store a hash that this backend can never load.</violation>
</file>
<file name="node-graph/nodes/gstd/src/platform_application_io.rs">
<violation number="1" location="node-graph/nodes/gstd/src/platform_application_io.rs:140">
P2: Use an actual transparent image placeholder here; empty bytes make failed loads disappear instead of preserving the transparent fallback promised by this node.</violation>
<violation number="2" location="node-graph/nodes/gstd/src/platform_application_io.rs:142">
P2: This now treats every resource as an HTTP URL, so local asset paths documented for this node will always fail.</violation>
<violation number="3" location="node-graph/nodes/gstd/src/platform_application_io.rs:265">
P1: Don't panic when the resource backend or hash is missing; this node can crash the whole render path on ordinary load failures.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource.rs:4">
P2: Add a non-OPFS fallback here. This OPFS-only wasm backend loses persistent resources on supported Safari 15.0-15.1.</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/resource.rs:12">
P2: This `Mutex` is unnecessary for the in-memory store; it only adds contention and panic-on-poison paths to code that could use a plain `HashMap`.
(Based on your team's feedback about reusing existing abstractions instead of adding mutexes.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/application.rs">
<violation number="1" location="editor/src/application.rs:12">
P2: `Editor::new()` now returns a partially initialized editor and depends on a separate `replace_application_io()` call before graph execution is safe.
(Based on your team's feedback about placing integrity-enforcing initialization where the state is created.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/messages/portfolio/document_migration.rs">
<violation number="1" location="editor/src/messages/portfolio/document_migration.rs:1594">
P1: Handle the legacy 1-input image node shape here too; after the alias remap, older embedded-image documents skip this migration and keep an incompatible input layout.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/utility_types.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:304">
P1: This rewires the bitmap layer to a hash before the resource write can fail safely. If `ResourceMessage::Write` hits an I/O error, the node still keeps the hash-only reference and later resource loading panics when that hash is missing.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/messages/resource/resource_message_handler.rs">
<violation number="1" location="editor/src/messages/resource/resource_message_handler.rs:54">
P1: The non-test default creates an unusable `ResourceMessageHandler`: it can panic in `resources()` or silently export no resources during save.
(Based on your team's feedback about avoiding panics and making invalid states unrepresentable.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/messages/portfolio/portfolio_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/portfolio_message_handler.rs:452">
P1: This GC fallback loads and selects another document, then aborts the cleanup pass for legacy persisted state entries.</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource/opfs.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource/opfs.rs:1">
P2: Custom agent: **PR title enforcement**
PR title "Content hash based resources" is not in imperative mood and lacks a leading action verb. Rewrite using an imperative leading verb, e.g., "Add content-hash-based resources" or "Use content hashes for resource naming".</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/resource/opfs.rs:36">
P1: Do not hide OPFS enumeration failure here. This makes `on_disk` empty, so previously stored resources read as missing for the whole session instead of triggering the existing fallback path.</violation>
</file>
<file name="editor/src/messages/frontend/frontend_message.rs">
<violation number="1" location="editor/src/messages/frontend/frontend_message.rs:104">
P0: This `Await` message breaks on desktop. The native bridge serializes it without the future, so it deserializes into an empty `FrontendMessageFuture` and panics on `future.await`. Keep this variant off the serialized bridge or handle it before RON.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,344 @@ | |||
| use graphene_application_io::{Resource, ResourceFuture, ResourceHash, ResourceStorage, Resources}; | |||
Contributor
There was a problem hiding this comment.
P2: Custom agent: PR title enforcement
PR title "Content hash based resources" is not in imperative mood and lacks a leading action verb. Rewrite using an imperative leading verb, e.g., "Add content-hash-based resources" or "Use content hashes for resource naming".
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/resource/opfs.rs, line 1:
<comment>PR title "Content hash based resources" is not in imperative mood and lacks a leading action verb. Rewrite using an imperative leading verb, e.g., "Add content-hash-based resources" or "Use content hashes for resource naming".</comment>
<file context>
@@ -0,0 +1,344 @@
+use graphene_application_io::{Resource, ResourceFuture, ResourceHash, ResourceStorage, Resources};
+use js_sys::Uint8Array;
+use std::collections::{HashMap, HashSet, VecDeque};
</file context>
| #[cfg(not(target_family = "wasm"))] | ||
| pub mod mmap; | ||
| #[cfg(target_family = "wasm")] | ||
| pub mod opfs; |
Contributor
There was a problem hiding this comment.
P2: Add a non-OPFS fallback here. This OPFS-only wasm backend loses persistent resources on supported Safari 15.0-15.1.
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/resource.rs, line 4:
<comment>Add a non-OPFS fallback here. This OPFS-only wasm backend loses persistent resources on supported Safari 15.0-15.1.</comment>
<file context>
@@ -1,7 +1,7 @@
#[cfg(not(target_family = "wasm"))]
pub mod mmap;
+#[cfg(target_family = "wasm")]
+pub mod opfs;
use graphene_application_io::{Resource, ResourceFuture, ResourceHash, ResourceStorage, Resources};
</file context>
efe8946 to
363d1b0
Compare
Contributor
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="frontend/wrapper/src/editor_wrapper.rs">
<violation number="1" location="frontend/wrapper/src/editor_wrapper.rs:86">
P2: Custom agent: **PR title enforcement**
PR title must be in imperative mood and start with an action verb</violation>
<violation number="2" location="frontend/wrapper/src/editor_wrapper.rs:149">
P2: Re-check `EDITOR_HAS_CRASHED` before forwarding awaited messages. This async path can still send `FrontendMessage`s after a crash.</violation>
</file>
<file name="editor/src/messages/portfolio/document/document_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_message_handler.rs:930">
P2: Avoid cloning the whole document here. It copies undo/redo history and other skipped state on every save.</violation>
</file>
<file name="editor/src/messages/frontend/utility_types.rs">
<violation number="1" location="editor/src/messages/frontend/utility_types.rs:89">
P1: Do not derive `Default` on this one-shot future wrapper. `default()` builds an empty future, and the native `FrontendMessage::Await` deserialize path turns that into a panic at `.await` during save.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -11,6 +11,7 @@ use crate::helpers::poll_node_graph_evaluation; | |||
| use crate::helpers::{auto_save_all_documents, calculate_hash, render_image_data_to_canvases, request_animation_frame, set_timeout, translate_key, wrapper}; | |||
Contributor
There was a problem hiding this comment.
P2: Custom agent: PR title enforcement
PR title must be in imperative mood and start with an action verb
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/wrapper/src/editor_wrapper.rs, line 86:
<comment>PR title must be in imperative mood and start with an action verb</comment>
<file context>
@@ -83,10 +83,10 @@ impl EditorWrapper {
};
- let storage: Box<dyn graph_craft::application_io::ResourceStorage> = match IndexedDbResourceStorage::load("graphite-resources").await {
+ let storage: Box<dyn graph_craft::application_io::ResourceStorage> = match OpfsResourceStorage::load("resources").await {
Ok(storage) => Box::new(storage),
Err(error) => {
</file context>
Contributor
There was a problem hiding this comment.
6 issues found across 13 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/graph-craft/src/application_io/wasm.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/wasm.rs:1">
P1: Custom agent: **PR title enforcement**
PR title is not in imperative mood and lacks a leading action verb</violation>
</file>
<file name="editor/src/messages/portfolio/document/utility_types/embedded_resources.rs">
<violation number="1" location="editor/src/messages/portfolio/document/utility_types/embedded_resources.rs:47">
P2: Serializing the `HashMap` directly makes saved document output nondeterministic. Sort the resource entries before writing them so identical documents serialize identically.</violation>
</file>
<file name="node-graph/libraries/application-io/src/lib.rs">
<violation number="1" location="node-graph/libraries/application-io/src/lib.rs:155">
P2: Hash the shared `Io` allocation here, not the `Arc` handle's address. As written, `Arc::clone` of the same `application_io` produces different `EditorApi` hashes and can cause avoidable cache misses.
(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.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io.rs:68">
P1: Don't `expect` on `resources` here. `new()`/`default()` leave it unset, and some call sites (for example the CLI) construct `PlatformApplicationIo` without calling `inject_resources()`, so loading a content-hash resource can abort the whole process.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/dispatcher.rs">
<violation number="1" location="editor/src/dispatcher.rs:42">
P2: `with_executor()` now leaves the resource subsystem uninitialized, so non-test callers built through this constructor will drop `ResourceMessage`s and can panic when a resource handle is requested.
(Based on your team's feedback about making invalid states unrepresentable in data models.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource/mmap.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource/mmap.rs:56">
P2: Verify the mapped bytes still hash to the requested `ResourceHash` before returning them.</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/resource/mmap.rs:107">
P1: Don't report a successful resource hash after the filesystem write failed. Otherwise callers can store a hash that this backend can never load.</violation>
</file>
<file name="node-graph/nodes/gstd/src/platform_application_io.rs">
<violation number="1" location="node-graph/nodes/gstd/src/platform_application_io.rs:140">
P2: Use an actual transparent image placeholder here; empty bytes make failed loads disappear instead of preserving the transparent fallback promised by this node.</violation>
<violation number="2" location="node-graph/nodes/gstd/src/platform_application_io.rs:142">
P2: This now treats every resource as an HTTP URL, so local asset paths documented for this node will always fail.</violation>
<violation number="3" location="node-graph/nodes/gstd/src/platform_application_io.rs:265">
P1: Don't panic when the resource backend or hash is missing; this node can crash the whole render path on ordinary load failures.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource.rs:4">
P2: Add a non-OPFS fallback here. This OPFS-only wasm backend loses persistent resources on supported Safari 15.0-15.1.</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/resource.rs:12">
P2: This `Mutex` is unnecessary for the in-memory store; it only adds contention and panic-on-poison paths to code that could use a plain `HashMap`.
(Based on your team's feedback about reusing existing abstractions instead of adding mutexes.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/application.rs">
<violation number="1" location="editor/src/application.rs:12">
P2: `Editor::new()` now returns a partially initialized editor and depends on a separate `replace_application_io()` call before graph execution is safe.
(Based on your team's feedback about placing integrity-enforcing initialization where the state is created.) [FEEDBACK_USED]</violation>
</file>
<file name="editor/src/messages/portfolio/document_migration.rs">
<violation number="1" location="editor/src/messages/portfolio/document_migration.rs:1594">
P1: Handle the legacy 1-input image node shape here too; after the alias remap, older embedded-image documents skip this migration and keep an incompatible input layout.</violation>
</file>
<file name="editor/src/messages/resource/resource_message_handler.rs">
<violation number="1" location="editor/src/messages/resource/resource_message_handler.rs:54">
P1: The non-test default creates an unusable `ResourceMessageHandler`: it can panic in `resources()` or silently export no resources during save.
(Based on your team's feedback about avoiding panics and making invalid states unrepresentable.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource/opfs.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource/opfs.rs:1">
P2: Custom agent: **PR title enforcement**
PR title "Content hash based resources" is not in imperative mood and lacks a leading action verb. Rewrite using an imperative leading verb, e.g., "Add content-hash-based resources" or "Use content hashes for resource naming".</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/resource/opfs.rs:30">
P2: Do not `unsafe impl Send/Sync` for this OPFS storage. `FileSystemDirectoryHandle` is JS state, so this makes a thread-safety promise the type cannot enforce. A future threaded or worker caller turns this into UB instead of a compile error.
(Based on your team's feedback about using derive and safe code before `unsafe`.) [FEEDBACK_USED]</violation>
<violation number="3" location="node-graph/graph-craft/src/application_io/resource/opfs.rs:36">
P1: Do not hide OPFS enumeration failure here. This makes `on_disk` empty, so previously stored resources read as missing for the whole session instead of triggering the existing fallback path.</violation>
<violation number="4" location="node-graph/graph-craft/src/application_io/resource/opfs.rs:95">
P1: Mark this hash on disk only after the OPFS write succeeds. Right now one failed write leaves `on_disk` set, so later writes of the same bytes are skipped and the resource can disappear after cache eviction or reload.</violation>
</file>
<file name="editor/src/messages/frontend/frontend_message.rs">
<violation number="1" location="editor/src/messages/frontend/frontend_message.rs:31">
P2: Do not deserialize `Await` with a default future. That builds an empty `FrontendMessageFuture` that panics on the first `.await`. Make this variant non-wire or keep it out of the serializable enum.</violation>
<violation number="2" location="editor/src/messages/frontend/frontend_message.rs:104">
P0: This `Await` message breaks on desktop. The native bridge serializes it without the future, so it deserializes into an empty `FrontendMessageFuture` and panics on `future.await`. Keep this variant off the serialized bridge or handle it before RON.</violation>
</file>
<file name="frontend/wrapper/src/editor_wrapper.rs">
<violation number="1" location="frontend/wrapper/src/editor_wrapper.rs:86">
P2: Custom agent: **PR title enforcement**
PR title must be in imperative mood and start with an action verb</violation>
<violation number="2" location="frontend/wrapper/src/editor_wrapper.rs:149">
P2: Re-check `EDITOR_HAS_CRASHED` before forwarding awaited messages. This async path can still send `FrontendMessage`s after a crash.</violation>
</file>
<file name="editor/src/messages/frontend/utility_types.rs">
<violation number="1" location="editor/src/messages/frontend/utility_types.rs:89">
P1: Do not derive `Default` on this one-shot future wrapper. `default()` builds an empty future, and the native `FrontendMessage::Await` deserialize path turns that into a panic at `.await` during save.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="desktop/wrapper/src/intercept_frontend_message.rs">
<violation number="1" location="desktop/wrapper/src/intercept_frontend_message.rs:11">
P2: Do not block on `FrontendMessage::Await` here. This makes save-time resource loading run on the desktop event-loop thread and can freeze the app while the document is being prepared.</violation>
</file>
<file name="editor/src/messages/portfolio/document/document_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_message_handler.rs:943">
P1: Do not drop failed resource loads here. `flatten()` turns a missing asset into a partial save, so reopening the file can lose linked resources.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| if !guard.on_disk.contains(&hash) { | ||
| let bytes = bytes.unwrap_or_else(|| Arc::<[u8]>::from(data)); | ||
| guard.on_disk.insert(hash); |
Contributor
There was a problem hiding this comment.
P1: Mark this hash on disk only after the OPFS write succeeds. Right now one failed write leaves on_disk set, so later writes of the same bytes are skipped and the resource can disappear after cache eviction or reload.
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/resource/opfs.rs, line 95:
<comment>Mark this hash on disk only after the OPFS write succeeds. Right now one failed write leaves `on_disk` set, so later writes of the same bytes are skipped and the resource can disappear after cache eviction or reload.</comment>
<file context>
@@ -0,0 +1,344 @@
+
+ if !guard.on_disk.contains(&hash) {
+ let bytes = bytes.unwrap_or_else(|| Arc::<[u8]>::from(data));
+ guard.on_disk.insert(hash);
+ guard.queue.push_back(Mutation::Write { hash, bytes });
+ kick_worker(&self.inner, &mut guard);
</file context>
| pub(super) fn intercept_frontend_message(dispatcher: &mut DesktopWrapperMessageDispatcher, message: FrontendMessage) -> Option<FrontendMessage> { | ||
| match message { | ||
| FrontendMessage::Await { future } => { | ||
| let message = futures::executor::block_on(async move { future.await }); |
Contributor
There was a problem hiding this comment.
P2: Do not block on FrontendMessage::Await here. This makes save-time resource loading run on the desktop event-loop thread and can freeze the app while the document is being prepared.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/wrapper/src/intercept_frontend_message.rs, line 11:
<comment>Do not block on `FrontendMessage::Await` here. This makes save-time resource loading run on the desktop event-loop thread and can freeze the app while the document is being prepared.</comment>
<file context>
@@ -7,6 +7,10 @@ use super::messages::{DesktopFrontendMessage, FileFilter, OpenFileDialogContext,
pub(super) fn intercept_frontend_message(dispatcher: &mut DesktopWrapperMessageDispatcher, message: FrontendMessage) -> Option<FrontendMessage> {
match message {
+ FrontendMessage::Await { future } => {
+ let message = futures::executor::block_on(async move { future.await });
+ return intercept_frontend_message(dispatcher, message);
+ }
</file context>
TrueDoctor
approved these changes
May 21, 2026
Contributor
There was a problem hiding this comment.
5 issues found across 25 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/application-io/src/lib.rs">
<violation number="1" location="node-graph/libraries/application-io/src/lib.rs:155">
P2: Hash the shared `Io` allocation here, not the `Arc` handle's address. As written, `Arc::clone` of the same `application_io` produces different `EditorApi` hashes and can cause avoidable cache misses.
(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.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io.rs:68">
P1: Don't `expect` on `resources` here. `new()`/`default()` leave it unset, and some call sites (for example the CLI) construct `PlatformApplicationIo` without calling `inject_resources()`, so loading a content-hash resource can abort the whole process.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource/mmap.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource/mmap.rs:107">
P1: Don't report a successful resource hash after the filesystem write failed. Otherwise callers can store a hash that this backend can never load.</violation>
</file>
<file name="node-graph/nodes/gstd/src/platform_application_io.rs">
<violation number="1" location="node-graph/nodes/gstd/src/platform_application_io.rs:140">
P2: Use an actual transparent image placeholder here; empty bytes make failed loads disappear instead of preserving the transparent fallback promised by this node.</violation>
<violation number="2" location="node-graph/nodes/gstd/src/platform_application_io.rs:142">
P2: This now treats every resource as an HTTP URL, so local asset paths documented for this node will always fail.</violation>
<violation number="3" location="node-graph/nodes/gstd/src/platform_application_io.rs:265">
P1: Don't panic when the resource backend or hash is missing; this node can crash the whole render path on ordinary load failures.
(Based on your team's feedback about avoiding panics in application code.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource.rs:4">
P2: Add a non-OPFS fallback here. This OPFS-only wasm backend loses persistent resources on supported Safari 15.0-15.1.</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/resource.rs:12">
P2: This `Mutex` is unnecessary for the in-memory store; it only adds contention and panic-on-poison paths to code that could use a plain `HashMap`.
(Based on your team's feedback about reusing existing abstractions instead of adding mutexes.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/graph-craft/src/application_io/resource/opfs.rs">
<violation number="1" location="node-graph/graph-craft/src/application_io/resource/opfs.rs:1">
P2: Custom agent: **PR title enforcement**
PR title "Content hash based resources" is not in imperative mood and lacks a leading action verb. Rewrite using an imperative leading verb, e.g., "Add content-hash-based resources" or "Use content hashes for resource naming".</violation>
<violation number="2" location="node-graph/graph-craft/src/application_io/resource/opfs.rs:95">
P1: Mark this hash on disk only after the OPFS write succeeds. Right now one failed write leaves `on_disk` set, so later writes of the same bytes are skipped and the resource can disappear after cache eviction or reload.</violation>
</file>
<file name="frontend/wrapper/src/editor_wrapper.rs">
<violation number="1" location="frontend/wrapper/src/editor_wrapper.rs:86">
P2: Custom agent: **PR title enforcement**
PR title must be in imperative mood and start with an action verb</violation>
</file>
<file name="desktop/wrapper/src/intercept_frontend_message.rs">
<violation number="1" location="desktop/wrapper/src/intercept_frontend_message.rs:11">
P2: Do not block on `FrontendMessage::Await` here. This makes save-time resource loading run on the desktop event-loop thread and can freeze the app while the document is being prepared.</violation>
</file>
<file name="node-graph/libraries/core-types/src/resource.rs">
<violation number="1" location="node-graph/libraries/core-types/src/resource.rs:1">
P2: Custom agent: **PR title enforcement**
PR title 'Content hash based resources' is a noun phrase and does not use imperative mood with a leading action verb.</violation>
<violation number="2" location="node-graph/libraries/core-types/src/resource.rs:52">
P2: Do not hash raw bytes here. This makes every memo pass rescan the whole resource instead of using the content hash.</violation>
</file>
<file name="node-graph/nodes/raster/src/std_nodes.rs">
<violation number="1" location="node-graph/nodes/raster/src/std_nodes.rs:293">
P2: This duplicates the existing image decode path. Pull the shared decode/conversion into one helper. Two copies of this gamma/premultiply logic will drift.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,58 @@ | |||
| use dyn_any::StaticType; | |||
Contributor
There was a problem hiding this comment.
P2: Custom agent: PR title enforcement
PR title 'Content hash based resources' is a noun phrase and does not use imperative mood with a leading action verb.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/resource.rs, line 1:
<comment>PR title 'Content hash based resources' is a noun phrase and does not use imperative mood with a leading action verb.</comment>
<file context>
@@ -0,0 +1,58 @@
+use dyn_any::StaticType;
+use graphene_hash::CacheHash;
+use std::hash::Hash;
</file context>
|
|
||
| #[node_macro::node(category(""))] | ||
| pub fn image(_: impl Ctx, _primary: (), image: Image<Color>) -> List<Raster<CPU>> { | ||
| pub fn image<'a: 'n>(_: impl Ctx, resource: Resource) -> List<Raster<CPU>> { |
Contributor
There was a problem hiding this comment.
P2: This duplicates the existing image decode path. Pull the shared decode/conversion into one helper. Two copies of this gamma/premultiply logic will drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/raster/src/std_nodes.rs, line 293:
<comment>This duplicates the existing image decode path. Pull the shared decode/conversion into one helper. Two copies of this gamma/premultiply logic will drift.</comment>
<file context>
@@ -288,6 +289,29 @@ pub fn empty_image(_: impl Ctx, transform: DAffine2, color: List<Color>) -> List
}
+#[node_macro::node(category(""))]
+pub fn image<'a: 'n>(_: impl Ctx, resource: Resource) -> List<Raster<CPU>> {
+ let image_data = resource.as_ref();
+
</file context>
|
|
||
| impl CacheHash for Resource { | ||
| fn cache_hash<H: core::hash::Hasher>(&self, state: &mut H) { | ||
| self.as_ref().hash(state); |
Contributor
There was a problem hiding this comment.
P2: Do not hash raw bytes here. This makes every memo pass rescan the whole resource instead of using the content hash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/resource.rs, line 52:
<comment>Do not hash raw bytes here. This makes every memo pass rescan the whole resource instead of using the content hash.</comment>
<file context>
@@ -0,0 +1,58 @@
+
+impl CacheHash for Resource {
+ fn cache_hash<H: core::hash::Hasher>(&self, state: &mut H) {
+ self.as_ref().hash(state);
+ }
+}
</file context>
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Member
Author
|
!build desktop (Run ID 26232036120) |
|
|
|
timon-schelling
changed the title
timon-schelling
force-pushed
the
content-hash-based-resources
branch
from
1cf3e9a to
8d3f973
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Real world usage size reduction example
