Address Greptile review on onboarding feature showcase#461
Merged
Conversation
- Hide the decorative showcase from VoiceOver via .accessibilityHidden(true) - Reset typedCount inside the ghost card's fade-out so it doesn't snap blank - Identify emoji candidate rows by stable alias instead of array index
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #459 addressing the three Greptile findings on
OnboardingFeatureShowcase:.accessibilityHidden(true)to keep mid-animation text fragments out of VoiceOver.GhostTextDemoCardnow resetstypedCount = 0inside its closing fade-out animation, matchingEmojiPickerDemoCardso the typed text fades instead of snapping blank between loops.ForEachnow keys on the stablealiasstring instead of the array index (\.offset).Validation
Linked issues
Refs #459.
Risk / rollout notes
Onboarding-only, decorative view. No behavior change to suggestions, settings, or any service.
Greptile Summary
This PR is a follow-up cleanup to #459, applying three targeted fixes to
OnboardingFeatureShowcase.swiftbased on a prior review. All changes are isolated to a single decorative, onboarding-only file with no impact on the real suggestion pipeline, settings, or services..accessibilityHidden(true)is now applied to the top-levelVStackinOnboardingFeatureShowcase, preventing mid-animation text fragments from being read by VoiceOver.typedCount = 0is moved inside the closingwithAnimationblock inGhostTextDemoCard, matchingEmojiPickerDemoCardso the typed text fades out smoothly instead of snapping blank between loops.DemoEmojiPopupcandidate list now usesid: \.element.alias(a stable string) instead ofid: \.offset(an array index), avoiding potential SwiftUI identity churn.Confidence Score: 5/5
Safe to merge — all three changes are correct, confined to a single decorative onboarding view, and have no effect on any real app functionality.
The three changes are straightforward and well-matched to the problems they solve. Moving
typedCount = 0into the animation block is consistent with howEmojiPickerDemoCardalready handles its own reset. TheForEachkey change from\.offsetto\.element.aliasis valid given the hardcoded, distinct alias values. The.accessibilityHidden(true)placement on the top-levelVStackcorrectly covers both child cards. No logic errors, no regressions, no edge cases left unaddressed.No files require special attention — the single changed file is a self-contained decorative view.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Task as .task loop participant State as @State (typedCount / showGhost / accepted) participant View as SwiftUI View Note over Task,View: GhostTextDemoCard animation loop Task->>State: "typedCount = 0, showGhost = false, accepted = false (direct)" loop type each character Task->>State: "typedCount += 1" View-->>View: re-render typed text end Task->>State: "withAnimation { showGhost = true }" View-->>View: ghost text fades in Task->>State: "withAnimation { accepted = true }" View-->>View: ghost turns primary color Task->>State: "withAnimation { showGhost=false, accepted=false, typedCount=0 } NEW" View-->>View: typed text + ghost fade out smoothly Note right of State: Previously typedCount=0 was bare assignment at loop topReviews (1): Last reviewed commit: "Address Greptile review on onboarding sh..." | Re-trigger Greptile