fix(studio): retire legacy Style and Grade sections from flat inspector#2126
Conversation
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed as part of the 15-PR edit-panel redesign stack. Full stack review posted on #2120. No blockers on this PR. — Miga
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Final current-head pass: cabe45e. Reviewed the stack boundaries and current diff against the flat-inspector contracts; required checks have no failures/pending checks and no unresolved review threads remain. No new drift found; residual notes are non-blocking.
vanceingalls
force-pushed
the
studio-flat-07-retire-legacy-style-grade
branch
from
cabe45e to
40487ac
Compare
vanceingalls
force-pushed
the
studio-flat-06-pinning-multifield
branch
from
f3389dc to
45a06a5
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Superseding earlier approval: the latest max-review pass found confirmed correctness issues in this flat-inspector stack (identity/selector-index state, hide-all write races, timing inference, slider pointer/keyboard/reset semantics, and duplicate React keys). Hold merge and require fixes plus re-review on the current stack head.
vanceingalls
force-pushed
the
studio-flat-06-pinning-multifield
branch
from
45a06a5 to
ef3e937
Compare
vanceingalls
force-pushed
the
studio-flat-07-retire-legacy-style-grade
branch
from
40487ac to
cbbe2ae
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head cbbe2ae792bff48585200c321ee8e3ffdf63d658. The retirement itself is mechanically small, but it makes the flat Style/Grade replacements authoritative while their correctness gaps remain: custom masks cannot be represented safely, opacity zero is mis-summarized, slider keyboard semantics are absent, and grade state/metadata retry behavior is unsafe.
A retirement PR must prove behavior parity with the surface it removes. Keep the legacy path available until those direct lower-layer blockers are fixed and covered by parity tests.
Verdict: REQUEST CHANGES
Reasoning: This layer activates incomplete replacement surfaces and removes the safe fallback before parity blockers are resolved.
— Deepwork
vanceingalls
force-pushed
the
studio-flat-07-retire-legacy-style-grade
branch
from
cbbe2ae to
f997ebb
Compare
vanceingalls
force-pushed
the
studio-flat-06-pinning-multifield
branch
from
ef3e937 to
b4e30fe
Compare
|
The specific lower-layer parity gaps this review held the retirement on are now fixed:
Both PRs are stacked above this one, so none of it shows at this PR's own head. With those parity gaps closed, the flat Style/Grade replacement is behavior-complete relative to the legacy path being retired here. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at f997ebbd8655c65646fc6b337d4b49ec30c667b1.
The single incremental commit is byte-equivalent to the previously reviewed head (patch-id bd941432…). It still retires the legacy Style/Grade surfaces while the replacement stack below it retains value-loss, identity, persistence, cache, and accessibility blockers. No fix/test delta was added in this restack.
Verdict: REQUEST CHANGES
Reasoning: Removing the fallback remains unsafe until the blocked replacement contracts are corrected and regression-tested.
— Deepwork
|
Confirming: this PR's own head is unchanged, correctly — the fixes for this review's findings were not squashed backward into it. They land in PR #2225 and PR #2416, which sit directly on top of this PR in the same Graphite stack ( This is a stacked-PR review gate question, not a missing-fix question: the stack is merged bottom-up in one sitting via Graphite ( Evaluating each intermediate head against "is this correct if it were the final merged state" doesn't match how the stack actually ships. Could you review mergeability at the stack tip (#2416, which contains every fix) rather than blocking each intermediate PR on findings that are already resolved a few commits further up the same stack? Happy to walk through the specific commit that fixes any finding you want re-verified at the tip. |
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
|
This was flagged as blocked on #2124's negative-cache bug — that bug is already fixed further up the same stack (see the reply on #2124: Nothing left to fix in this PR specifically. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Stack-scoped re-review at f997ebb.
The incremental layer is unchanged, and its prior requested changes are now resolved in the accepted mandatory tip fix, #2416 at 832d5be. I verified the cumulative tip closes the flat-inspector identity, value-parity, lifecycle, persistence, accessibility, and constrained-layout findings with focused regression coverage. This approval assumes the Graphite stack lands bottom-up through #2416; this layer should not be landed while dropping the approved tip fix.
Verdict: APPROVE
Reasoning: The accepted tip-fix strategy closes the blocker inherited or introduced at this layer, and the verified cumulative stack is coherent through #2416.
— Deepwork
The flat inspector rendered its new Style/Grade groups AND the legacy ColorGradingSection/StyleSections components a second time below them, visibly doubling every control. Remove the now-redundant legacy render call sites (and their now-unused imports) from PropertyPanelFlat.tsx; those components stay intact for the legacy (flag-off) PropertyPanel.
vanceingalls
force-pushed
the
studio-flat-07-retire-legacy-style-grade
branch
from
f997ebb to
22359ea
Compare
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
vanceingalls
force-pushed
the
studio-flat-06-pinning-multifield
branch
from
b4e30fe to
0524f1c
Compare
vanceingalls
dismissed
miguel-heygen’s stale review
The base branch was changed.
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
3 participants
What
Seventh PR in the flat inspector stack (see #2120 for the foundation and full stack list). Removes the legacy
ColorGradingSection/StyleSectionsrender call sites fromPropertyPanelFlat.tsx— with the flag on, these were rendering a second time below their flat equivalents (Color grading, Radius, Stroke, Effects, Clip, Transparency, Fill all doubled).Stack: #2120 → #2121 → #2122 → #2123 → #2124 → #2125 → #2126 (this).
Why
This was flagged as an Important (non-blocking) finding in the final whole-branch review, then confirmed live during manual testing of #2125 — every Style/Grade control was visibly duplicated. Flat coverage was already confirmed complete for both groups, so the legacy twins were pure redundant duplication, not a fallback for missing functionality.
How
<ColorGradingSection>and<StyleSections hideFlex>blocks (and their now-dead imports) fromPropertyPanelFlat.tsxonly.propertyPanelColorGradingSection.tsx,propertyPanelStyleSections.tsx) are untouched — the non-flat legacyPropertyPanel.tsxstill uses them when the feature flag is off.useColorGradingControllernow runs exactly once per render instead of twice (the second invocation was inside the now-removed legacyColorGradingSectioninstance) — resolving another Important finding from the whole-branch review.Test plan
data-panel-sectionattribute) is absent with the flag on, scoped carefully to avoid a false failure against the flat Style group's own "Fill" row label (which is supposed to still render).oxlint/oxfmt/fallowclean.