Repository navigation
Smooth profile avatar editor transitions - #8640
klopez4212 wants to merge 10 commits into
Conversation
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab13e98357
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| currentAvatarUrl: openingDraft is ProfileUrlAvatarDraft | ||
| ? openingDraft.url | ||
| : null, |
There was a problem hiding this comment.
Keep the suggested emoji out of the image baseline
When a fresh editor has no initialDraft, openingDraft is an uncommitted suggestion, but passing it as currentAvatarUrl makes it the fallback in every mode. After the user switches to Image, onModeChanged clears the draft and disables Done, yet the preview still displays that discarded emoji rather than the empty dotted placeholder. Use the actual initial avatar as the persistent baseline, while using the suggestion only to initialize the Emoji mode.
Useful? React with 👍 / 👎.
| // restart it or allow another navigation action mid-transition. | ||
| controller.view.isUserInteractionEnabled = false | ||
| controller.navigationItem.leftBarButtonItem?.isEnabled = false | ||
| controller.setEditorVisible(false) |
There was a problem hiding this comment.
Restore the placeholder during the cancel animation
When the user opens a fresh avatar editor and presses Back, revealEditor has hidden the UIKit avatar, and this return path does not unhide it until restoreProfilePage runs after Flutter's 200 ms hide completes. The Flutter preview also remains fully opaque, so the suggested emoji collapses and then abruptly swaps to the dotted placeholder at teardown instead of performing the intended crossfade. The cancellation path needs to reveal/fade the native placeholder while the Flutter preview exits.
Useful? React with 👍 / 👎.
jedwards27
left a comment
There was a problem hiding this comment.
🤖 :bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 24a370606044c0c7be787cc2fd98ec0bfe1d606b..c2d3d080299731525bf52abf907e34e7126bbcb4
Risk: medium — user-visible mobile avatar setup/editor transitions plus iOS native/Flutter layering and lifecycle.
I found two blocking transition/state defects at the reviewed head.
-
Fresh setup → Image mode shows an unsaved suggested emoji as if it were the real avatar.
- Evidence: when
initialDraft == null,ProfileSetupAvatarEditorcreates a default emojiopeningDraft(mobile/lib/features/profile/profile_setup_avatar_editor.dart:57-65). The same suggested URL is then passed ascurrentAvatarUrlin both embedded and fallback editors (profile_setup_avatar_editor.dart:197-199,profile_setup_avatar_editor.dart:277-279). When the user changes mode, the setup editor clears the draft (profile_setup_avatar_editor.dart:206-209,profile_setup_avatar_editor.dart:285-288), butProfileAvatarEditorfalls back fromdrafttocurrentAvatarUrlfor Image preview (mobile/lib/features/profile/profile_avatar_editor.dart:279-305). - Failure: a new user opens avatar setup, sees the suggested Emoji avatar, then switches to Image. Done is disabled because there is no image draft, but the preview still shows the discarded suggested emoji instead of the empty dotted avatar. The UI is now lying about the selectable/savable state — small circus, real clown.
- Author action: split the suggested emoji seed from the persisted/current-avatar baseline. Keep the suggested emoji as the initial Emoji draft/control state, but use the actual persisted baseline (
initialDraft, ornullfor fresh setup) for Image-mode fallback/current avatar. If the editor needs the suggested URL to initialize Emoji controls, pass that through a mode-scoped seed or derive it fromdraft; do not reuse it as the globalcurrentAvatarUrl. Add a regression for fresh setup → Image showingProfileSetupEmptyAvatarwhile Done remains disabled. - Verification owner: author fixes/adds the regression; reviewer/CI re-check targeted mobile tests at the new exact head.
- Evidence: when
-
The transparent native editor can become visually opaque again after an appearance update.
- Evidence: this PR makes the embedded avatar
FlutterViewControllernon-opaque and clear (mobile/ios/Runner/NativeProfileSetup.swift:189-190) and the embedded Dart scaffold transparent, but the existing appearance handler still doesavatarController?.view.backgroundColor = controller.view.backgroundColor(NativeProfileSetup.swift:38-44). Dart can send those appearance updates while the iOS profile setup session is active (mobile/lib/features/profile/profile_setup_page.dart:219-231). - Failure: if appearance/theme changes while the editor is open, the editor view is repainted with the native page background. That breaks the PR’s return/crossfade contract because the native chrome/placeholder can no longer show through during exit.
- Author action: preserve the avatar Flutter controller as non-opaque/clear across appearance updates; route theme changes through the Flutter theme channel without assigning the controller view an opaque native background. Add Swift coverage that opens the editor, invokes
appearance, and assertsisViewOpaque == falseandbackgroundColor == .clearstill hold. - Verification owner: author adds Swift coverage; reviewer/CI re-check Mobile Swift Native plus the targeted behavior at the new exact head.
- Evidence: this PR makes the embedded avatar
Validation/evidence reviewed: local source trace at clean head c2d3d080299731525bf52abf907e34e7126bbcb4; authenticated reviewer jedwards27, PR author klopez4212; live PR head still c2d3d080299731525bf52abf907e34e7126bbcb4. PR CI is green at that head for mobile checks/tests/Android, Mobile Swift Native/Release/Results, DCO, and security. Focused review validation also reported git diff --check, just mobile-check, targeted profile_setup_avatar_editor_test.dart, and the full just mobile-test passing. Green CI does not exercise the two misleading-state/appearance-update paths above.
Manual/native evidence: no physical-device/native recording was used for this verdict; that is residual confidence risk, not the reason for requesting changes.
Residual risk: after the fix, re-review the exact new head and specifically verify fresh setup → Image placeholder/Done-disabled state and iOS appearance updates preserving the transparent editor handoff.
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
jedwards27
left a comment
There was a problem hiding this comment.
🤖 :bot: Jude’s code review agent reviewed the current head and approves this revision.
Exact head reviewed: bffb83f5e8c5135027de08ed0d0f5fa5af44c305
Base: 24a370606044c0c7be787cc2fd98ec0bfe1d606b
I found no unresolved author-actionable defect in this re-review.
What I re-checked:
- Fresh profile setup still seeds an unsaved suggested emoji for the Emoji tab, but the editor now passes only the real saved/initial avatar URL—or
null—ascurrentAvatarUrl. Switching to Image mode clears the active draft and shows the empty setup placeholder with Done disabled instead of treating the suggested emoji as the Image baseline. - The embedded iOS avatar editor remains non-opaque/clear. The native
appearancepath updates native chrome and forwards theme changes to Dart without repainting the childFlutterViewControllerbackground, so the native return handoff stays transparent. - The surrounding profile/avatar editor seams remain coherent: save/cancel behavior, async image-pick cancellation, native/Flutter lifecycle cleanup, reduced-motion handling, and the new transition/placeholder regression coverage.
Validation evidence for this exact head:
- Local/GitHub head freshness confirmed at
bffb83f5e8c5135027de08ed0d0f5fa5af44c305; PR author is distinct from the authenticated reviewer. git diff --check 24a370606044c0c7be787cc2fd98ec0bfe1d606b...HEADpassed.- Focused profile Flutter coverage passed, including
profile_setup_avatar_editor_test.dartand related profile avatar/theme/edit/retry tests. just mobile-checkpassed.- Targeted native RunnerTest for the profile setup recovery/transparent embedded editor path passed locally.
- Full
just mobile-testevidence passed for the current head. - GitHub status rollup has no non-success/non-skipped checks; Mobile Checks, Mobile Tests, Mobile Android, Mobile Swift Native/Release/Results, Semgrep, zizmor, and DCO are green.
Residual confidence gap: I did not require a physical-device/native video for approval. The relevant native source seam, local targeted RunnerTest, and exact-head CI cover the previous transparency defect sufficiently for review purposes.
Author action: none.
Smooths profile avatar editing with concurrent 200 ms entry/return transitions, fixed-size circle movement between tabs, and a crossfade to the dotted placeholder on cancel. New empty avatars open on Emoji, ready to save.
Review fixes: keep the suggested emoji separate from the Image fallback, and preserve native editor transparency across appearance changes. Regression journeys cover empty Image previews, retained Emoji selection, and light/dark updates.
Validation: focused Flutter journeys pass; restoring the old baseline fails both fresh-editor regressions. At
bffb83f5e: full Flutter suite and push checks passed; all 107 native tests passed on the iOS simulator. Prior iPhone visual approval covers the transition work; these review fixes still need a device check.Follow-up to #8628.