Migrate legacy gradients via an isolated measurement pre-pass#4268
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 validatedocument_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 activedocument_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_gradientmisses wiredFillnode 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
There was a problem hiding this comment.
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
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.
99cb8b8 to
40199d0
Compare
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.