Skip to content

Migrate legacy gradients via an isolated measurement pre-pass#4268

Merged
Keavon merged 6 commits into
masterfrom
gradient-migration-prepass
Jun 22, 2026
Merged

Migrate legacy gradients via an isolated measurement pre-pass#4268
Keavon merged 6 commits into
masterfrom
gradient-migration-prepass

Conversation

@Keavon

@Keavon Keavon commented Jun 22, 2026

Copy link
Copy Markdown
Member

Fixes the initial migration strategy in #4241 that involved a deferred migration pass on the first frame upon loading, in lieu of a per-node isolated migration now.

Convert legacy bounding-box-relative Fill::Gradient values to absolute space on document load by measuring each fill's evaluated geometry, rather than walking each layer's primary flow. The pre-pass scans the whole root network so fills on secondary branches (e.g. a Copy to Points instance) and fills in hidden, disabled, or orphaned branches are all found, redirecting the document export to each fill in turn to force its branch to evaluate and reading its geometry back from the inspect result.

With every legacy gradient converted before any render, the legacy bounding-box render path is removed: the ATTR_GRADIENT_LEGACY marker, the renderer's legacy brush branch, and fill_to_graphic_list's bbox bake all go away. Gradients nested inside subgraph node networks are out of scope and warned about.

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

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.

Code Review

This pull request refactors the legacy gradient migration process by introducing a one-time measurement pre-pass (GradientMigration) that runs before rendering, scanning and converting legacy bounding-box-relative gradients to absolute space. This simplifies the rendering pipeline by removing the deferred migration pass and legacy markers. The review feedback highlights critical improvements to prevent the migration from getting permanently stuck on early returns (such as channel errors or empty exports) and suggests associating the migration state with a specific DocumentId to avoid cross-document interference when switching active documents.

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/node_graph_executor.rs
Comment thread editor/src/node_graph_executor.rs
Comment thread editor/src/node_graph_executor.rs
Comment thread editor/src/node_graph_executor.rs Outdated
Comment thread editor/src/node_graph_executor.rs Outdated
Comment thread editor/src/node_graph_executor.rs

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

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.

4 issues found across 10 files

Confidence score: 2/5

  • In editor/src/node_graph_executor.rs, the early-return path can leave migration state (self.gradient_migration / document.pending_gradient_migration) uncleared when no export exists, which can permanently stall gradient migration for that document; users may stay stuck on legacy gradients indefinitely — ensure this path always clears or completes migration state before merging.
  • In editor/src/node_graph_executor.rs, the in-progress migration guard appears not to validate document_id, so switching documents during migration can block execution in the newly opened document; this risks cross-document lockout behavior — gate the early return by matching the active document_id (or reset stale migration state) before merge.
  • In editor/src/messages/portfolio/portfolio_message_handler.rs, migration enforcement is limited to viewport renders, so export/eyedropper can run before conversion and process legacy gradients; users could get inconsistent first-run export/preview results — apply the same migration gate to export/eyedropper paths and add a regression test for first-run export.
  • In editor/src/messages/portfolio/document_migration.rs, has_nested_legacy_gradient misses wired Fill node gradients in subgraphs, which can leave some legacy gradients un-migrated and make behavior inconsistent after rollout — expand detection to connected gradient inputs (not just inline values) before merging.
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_migration.rs">

<violation number="1" location="editor/src/messages/portfolio/document_migration.rs:1127">
P2: `has_nested_legacy_gradient` only detects inline `Fill::Gradient` values on node inputs inside subgraph networks, but misses `Fill` nodes whose gradient input is wired (connected from another node) rather than stored inline. A wired gradient inside a subgraph network wouldn't trigger the warning even though it also can't be migrated by the root-only pre-pass.</violation>
</file>

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

<violation number="1" location="editor/src/messages/portfolio/portfolio_message_handler.rs:1626">
P1: Gradient migration is enforced only for viewport renders, so export/eyedropper can bypass conversion. First-run export/preview may process legacy gradients before absolute-space migration finishes.</violation>
</file>

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

Re-trigger cubic

Comment thread editor/src/messages/portfolio/portfolio_message_handler.rs
Comment thread editor/src/node_graph_executor.rs
Comment thread editor/src/node_graph_executor.rs
Comment thread editor/src/messages/portfolio/document_migration.rs Outdated

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

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.

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="editor/src/messages/portfolio/document_migration.rs">

<violation number="1" location="editor/src/messages/portfolio/document_migration.rs:1127">
P2: `has_nested_legacy_gradient` only detects inline `Fill::Gradient` values on node inputs inside subgraph networks, but misses `Fill` nodes whose gradient input is wired (connected from another node) rather than stored inline. A wired gradient inside a subgraph network wouldn't trigger the warning even though it also can't be migrated by the root-only pre-pass.</violation>
</file>

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

<violation number="1" location="editor/src/messages/portfolio/portfolio_message_handler.rs:1626">
P1: Gradient migration is enforced only for viewport renders, so export/eyedropper can bypass conversion. First-run export/preview may process legacy gradients before absolute-space migration finishes.</violation>
</file>

<file name="editor/src/node_graph_executor.rs">

<violation number="1" location="editor/src/node_graph_executor.rs:473">
P2: Canceling migration by setting `gradient_migration = None` can leak stale measurement compilation updates into the active document. Keep canceled migration responses suppressed (or doc-scoped) until in-flight migration compilations are drained.</violation>
</file>

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

Re-trigger cubic

Comment thread editor/src/node_graph_executor.rs
Keavon added 2 commits June 22, 2026 14:59
Convert legacy bounding-box-relative Fill::Gradient values to absolute
space on document load by measuring each fill's evaluated geometry,
rather than walking each layer's primary flow. The pre-pass scans the
whole root network so fills on secondary branches (e.g. a Copy to Points
instance) and fills in hidden, disabled, or orphaned branches are all
found, redirecting the document export to each fill in turn to force its
branch to evaluate and reading its geometry back from the inspect result.

With every legacy gradient converted before any render, the legacy
bounding-box render path is removed: the ATTR_GRADIENT_LEGACY marker, the
renderer's legacy brush branch, and fill_to_graphic_list's bbox bake all
go away. Gradients nested inside subgraph node networks are out of scope
and warned about.
… switches

- Advance the queue on dispatch early returns (missing export or channel
  send error) so a failure can't leave the migration permanently stuck
  with the document unable to render.
- Tag the migration with its DocumentId: cancel-and-restart when a
  different document's migration is requested, and only apply a
  measurement to the active document's in-progress migration, so
  switching documents mid-migration can't stall or cross-apply.
@Keavon Keavon merged commit 1158d93 into master Jun 22, 2026
11 checks passed
@Keavon Keavon deleted the gradient-migration-prepass branch June 22, 2026 23:59
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.

1 participant