Skip to content

Introduce network request handler#4194

Merged
timon-schelling merged 14 commits into
masterfrom
network-request-handler
Jun 4, 2026
Merged

Introduce network request handler#4194
timon-schelling merged 14 commits into
masterfrom
network-request-handler

Conversation

@timon-schelling
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request migrates the font catalog loading and resource resolution logic from the Svelte frontend to the Rust backend, introducing a new Network message handler that uses reqwest to fetch resources asynchronously. Feedback has been provided to address three key issues: first, the fetch method should check for HTTP success status codes using response.error_for_status() to prevent caching corrupted error pages; second, the font variant parsing logic needs to strip the "italic" suffix before parsing the weight to avoid silently skipping non-400 italic variants; and third, the dynamically loaded font catalog should be dispatched back to the system via FontsMessage::CatalogLoaded so it is saved in the application state rather than being discarded and re-fetched on subsequent requests.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread editor/src/messages/network/utility_types.rs
Comment thread editor/src/messages/portfolio/fonts/utility_types.rs
Comment thread editor/src/messages/portfolio/document/resource/resource_message_handler.rs Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 issues found across 23 files

Confidence score: 2/5

  • High merge risk: there are multiple concrete, high-confidence runtime issues (sev 7-8) affecting request execution and resource loading behavior, so this is not yet a safe merge despite being fixable.
  • Most severe functional issue is in editor/src/messages/network/network_message.rs: cloning NetworkMessage::Request drops the request closure, which can turn valid requests into empty-request error handling and silently break network operations.
  • Resource resolution has likely regressions in editor/src/messages/portfolio/document_migration.rs and editor/src/messages/portfolio/document/resource/resource_message_handler.rs (incorrect embedded source registration, and an unconditional break that prevents fallback sources after fetch failure), plus editor/src/messages/network/utility_types.rs can hang flows due to missing client timeouts.
  • Pay close attention to editor/src/messages/network/network_message.rs, editor/src/messages/portfolio/document_migration.rs, editor/src/messages/portfolio/document/resource/resource_message_handler.rs, and editor/src/messages/network/utility_types.rs - these are the most likely to cause user-visible request/resource failures or stalls.
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/portfolio_message_handler.rs">

<violation number="1" location="editor/src/messages/portfolio/portfolio_message_handler.rs:426">
P2: Unconditional `ResolveAll` can trigger repeated font-catalog network loads when catalog is empty. This regresses startup/resource-resolve performance and can cause avoidable duplicate API calls.</violation>
</file>

<file name="editor/src/messages/portfolio/fonts/fonts_message_handler.rs">

<violation number="1" location="editor/src/messages/portfolio/fonts/fonts_message_handler.rs:26">
P2: LoadCatalog requests are not deduplicated, so startup/tool events can trigger parallel catalog fetches. This adds avoidable network load and nondeterministic last-response-wins catalog updates.</violation>
</file>

<file name="editor/src/messages/portfolio/document/resource/resource_message.rs">

<violation number="1" location="editor/src/messages/portfolio/document/resource/resource_message.rs:2">
P1: Custom agent: **PR title enforcement**

PR title is not in imperative mood and lacks a leading action verb required by the PR title convention.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

@@ -1,5 +1,5 @@
use crate::messages::prelude::*;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Custom agent: PR title enforcement

PR title is not in imperative mood and lacks a leading action verb required by the PR title convention.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/resource/resource_message.rs, line 2:

<comment>PR title is not in imperative mood and lacks a leading action verb required by the PR title convention.</comment>

<file context>
@@ -1,5 +1,5 @@
 use crate::messages::prelude::*;
-use graph_craft::application_io::resource::ResourceId;
+use graph_craft::application_io::resource::{DataSource, ResourceHash, ResourceId};
 use graphene_std::text::Font;
 use std::sync::Arc;
</file context>

Comment thread editor/src/messages/portfolio/document/resource/resource_message_handler.rs Outdated
Comment thread editor/src/messages/network/utility_types.rs Outdated
Comment thread editor/src/messages/network/network_message.rs Outdated
Comment thread editor/src/messages/portfolio/document_migration.rs
let FontsMessageContext { resource_storage } = context;

match message {
FontsMessage::LoadCatalog => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: LoadCatalog requests are not deduplicated, so startup/tool events can trigger parallel catalog fetches. This adds avoidable network load and nondeterministic last-response-wins catalog updates.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/fonts/fonts_message_handler.rs, line 26:

<comment>LoadCatalog requests are not deduplicated, so startup/tool events can trigger parallel catalog fetches. This adds avoidable network load and nondeterministic last-response-wins catalog updates.</comment>

<file context>
@@ -22,8 +23,17 @@ impl MessageHandler<FontsMessage, FontsMessageContext<'_>> for FontsMessageHandl
 		let FontsMessageContext { resource_storage } = context;
 
 		match message {
+			FontsMessage::LoadCatalog => {
+				responses.add(NetworkMessage::request(|client| async move {
+					let Some(catalog) = FontCatalog::load_from_api(&client).await else {
</file context>

responses.add(PortfolioMessage::DocumentPassMessage {
document_id,
message: DocumentMessage::Resource(ResourceMessage::Resolve),
message: DocumentMessage::Resource(ResourceMessage::ResolveAll),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Unconditional ResolveAll can trigger repeated font-catalog network loads when catalog is empty. This regresses startup/resource-resolve performance and can cause avoidable duplicate API calls.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/portfolio_message_handler.rs, line 426:

<comment>Unconditional `ResolveAll` can trigger repeated font-catalog network loads when catalog is empty. This regresses startup/resource-resolve performance and can cause avoidable duplicate API calls.</comment>

<file context>
@@ -422,14 +421,9 @@ impl MessageHandler<PortfolioMessage, PortfolioMessageContext<'_>> for Portfolio
 				responses.add(PortfolioMessage::DocumentPassMessage {
 					document_id,
-					message: DocumentMessage::Resource(ResourceMessage::Resolve),
+					message: DocumentMessage::Resource(ResourceMessage::ResolveAll),
 				});
 			}
</file context>

Comment thread editor/src/messages/network/network_message.rs Outdated
Comment thread editor/src/messages/portfolio/document/resource/resource_message_handler.rs Outdated
Comment thread editor/src/messages/portfolio/document/resource/resource_message_handler.rs Outdated
Comment thread editor/src/messages/tool/tool_messages/text_tool.rs Outdated
@timon-schelling timon-schelling changed the title Network request handler Introduce network request handler Jun 3, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 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/portfolio_message_handler.rs">

<violation number="1" location="editor/src/messages/portfolio/portfolio_message_handler.rs:426">
P2: Unconditional `ResolveAll` can trigger repeated font-catalog network loads when catalog is empty. This regresses startup/resource-resolve performance and can cause avoidable duplicate API calls.</violation>
</file>

<file name="editor/src/messages/portfolio/fonts/fonts_message_handler.rs">

<violation number="1" location="editor/src/messages/portfolio/fonts/fonts_message_handler.rs:26">
P2: LoadCatalog requests are not deduplicated, so startup/tool events can trigger parallel catalog fetches. This adds avoidable network load and nondeterministic last-response-wins catalog updates.</violation>
</file>

<file name="editor/src/messages/portfolio/document/resource/resource_message.rs">

<violation number="1" location="editor/src/messages/portfolio/document/resource/resource_message.rs:2">
P1: Custom agent: **PR title enforcement**

PR title is not in imperative mood and lacks a leading action verb required by the PR title convention.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread editor/src/messages/network/utility_types.rs Outdated
@timon-schelling timon-schelling enabled auto-merge June 4, 2026 13:24
@timon-schelling timon-schelling added this pull request to the merge queue Jun 4, 2026
Merged via the queue into master with commit ee7daa5 Jun 4, 2026
11 checks passed
@timon-schelling timon-schelling deleted the network-request-handler branch June 4, 2026 13:28
Keavon pushed a commit that referenced this pull request Jun 5, 2026
* Impl NetworkMessageHandler

* Repalce Frontend fetch like messages with NetworkMessage

* Reimpl resource loading with NetworkMessage

* Fix wasm

* dedublicate resource requests

* Embedd font resources created by migration

* Fixup

* Fix tests

* Fix font catalog not loading

* Cleanup

* Fix layouts not updating when font catalog is loaded

* Review

* Review

* Cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants