Skip to content

Make ResourceStorage trait &self-based and remove 'static requirement#4188

Merged
timon-schelling merged 2 commits into
masterfrom
resource-trait-refactor
Jun 1, 2026
Merged

Make ResourceStorage trait &self-based and remove 'static requirement#4188
timon-schelling merged 2 commits into
masterfrom
resource-trait-refactor

Conversation

@TrueDoctor
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 refactors the resource storage system by changing ResourceStorage methods to take shared references (&self) instead of mutable references (&mut self), and updating ResourceFuture to use a lifetime parameter instead of being 'static. Consequently, Box<dyn ResourceStorage> is replaced with Arc<dyn ResourceStorage> across the codebase, simplifying resource handling and removing the need for RwLock wrappers. The feedback suggests a minor cleanup in desktop/src/app.rs to use the imported Arc::new directly instead of its fully qualified path.

Comment thread desktop/src/app.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.

1 issue found across 12 files

Confidence score: 2/5

  • There is a high-confidence, high-severity concern in node-graph/graph-craft/src/application_io.rs: using .expect() can still panic when resources is None, which creates a clear runtime regression risk.
  • Because this is a concrete crash path (severity 8/10, confidence 9/10) in updated logic, the merge risk is elevated until error handling is made graceful instead of panicking.
  • Pay close attention to node-graph/graph-craft/src/application_io.rs - ensure the resources == None path returns a handled error rather than triggering a panic.

Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.

Re-trigger cubic

Comment thread node-graph/graph-craft/src/application_io.rs
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.

No issues found across 12 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@timon-schelling timon-schelling added this pull request to the merge queue Jun 1, 2026
Merged via the queue into master with commit 7b9c480 Jun 1, 2026
11 checks passed
@timon-schelling timon-schelling deleted the resource-trait-refactor branch June 1, 2026 10:37
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