Repository navigation
feat: PLAY-surface micro-interactions and harmonic character cues - #41
Conversation
Add hover, press, and selection motion to the harmonic compass, chord pads, and controller HUD so gestures read as musical cause-and-effect. Harmonic character now uses icon and ring weight in addition to colour. Co-authored-by: Harry Speight <sp80808@users.noreply.github.com>
The hover chrome must take its label as a built view so the shared button style compiles on macOS 14. Co-authored-by: Harry Speight <sp80808@users.noreply.github.com>
Reviewer's GuideImplements a reusable motion system and harmonic tension character model, then wires them through PLAY workspace views, harmonic wheel, controller HUD, and instrument selector to add hover/press/selection micro-interactions, content transitions, and non-color accessibility cues, with Reduce Motion-aware animation behavior and tests for the new tension character thresholds and semantics. Sequence diagram for left-stick chord selection and harmonic character cuessequenceDiagram
actor User
participant ControllerManager
participant AppState
participant HarmonicWheelView
participant EnhancedChordDisplayView
User->>ControllerManager: move_left_stick
ControllerManager->>AppState: update controllerState.leftStickMagnitude
ControllerManager->>AppState: update selectedChordIndex
AppState->>HarmonicWheelView: diatonicChords, selectedChordIndex
HarmonicWheelView->>HarmonicWheelView: selectedSector(center, innerRadius, outerRadius)
HarmonicWheelView->>AppState: chordSegments onTapGesture
AppState->>AppState: set currentChord
AppState->>EnhancedChordDisplayView: currentChord, activeNotes
EnhancedChordDisplayView->>EnhancedChordDisplayView: accessibilityChordLabel
EnhancedChordDisplayView->>HarmonicTensionCharacter: init(tension: chord.tension(...))
HarmonicTensionCharacter-->>EnhancedChordDisplayView: label
Note over EnhancedChordDisplayView: Accessibility label now includes chord name, roman numeral, and harmonic tension character label
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Several views now stack multiple
.animationmodifiers on the same container (e.g.ContextualHintSlot,EnhancedChordDisplayView,HarmonicWheelView.selectedSector), which can be hard to reason about and may produce unexpected interactions; consider consolidating these into a single animation per view or driving them via explicitwithAnimationwhere needed. - The tension-to-character mapping in
HarmonicTensionCharacteris now used in multiple UI surfaces (badge, chord nodes, accessibility labels); if you expect these thresholds or categories to evolve, consider centralizing the boundary values as named constants or documenting their musical intent so future tweaks stay consistent across all call sites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several views now stack multiple `.animation` modifiers on the same container (e.g. `ContextualHintSlot`, `EnhancedChordDisplayView`, `HarmonicWheelView.selectedSector`), which can be hard to reason about and may produce unexpected interactions; consider consolidating these into a single animation per view or driving them via explicit `withAnimation` where needed.
- The tension-to-character mapping in `HarmonicTensionCharacter` is now used in multiple UI surfaces (badge, chord nodes, accessibility labels); if you expect these thresholds or categories to evolve, consider centralizing the boundary values as named constants or documenting their musical intent so future tweaks stay consistent across all call sites.
## Individual Comments
### Comment 1
<location path="Sources/XPadUI/XMotion.swift" line_range="174-175" />
<code_context>
+ content
+ .opacity(pulseOpacity)
+ .shadow(color: isActive ? color.opacity(0.45) : .clear, radius: isActive ? 5 : 0)
+ .onAppear { phase = true }
+ .animation(
+ isActive && !reduceMotion
+ ? .easeInOut(duration: 1.15).repeatForever(autoreverses: true)
</code_context>
<issue_to_address>
**issue (bug_risk):** Live pulse never actually animates when `isActive` becomes true after appear.
`phase` is only set to `true` in `onAppear` and never updated, but the animation is keyed off `phase`. If `isActive` changes to `true` after the view appears, there’s no state change to start the animation, so opacity stays fixed. Consider driving the animation from `isActive` (e.g. update `phase` when `isActive` becomes true or key the animation directly off `isActive`) so the pulse begins when the active state toggles, not just on initial appearance.
</issue_to_address>
### Comment 2
<location path="Sources/XPadUI/PlayWorkspaceView.swift" line_range="216-222" />
<code_context>
Spacer()
}
.frame(height: 36)
+ .animation(reduceMotion ? nil : XTheme.transitionShort, value: appState.contextualHint)
+ .animation(reduceMotion ? nil : XTheme.transitionShort, value: appState.activeNotes.isEmpty)
}
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Stacking multiple `.animation` modifiers on the same container can be hard to reason about.
With two `.animation` modifiers on the same `HStack`, SwiftUI will typically apply only the last one, so changes to `contextualHint` or `activeNotes` may not animate as you expect. Consider using a single animation keyed off both values (e.g., a tuple) or attaching the animation to the specific subview that should animate (such as the banner) to keep behavior predictable.
```suggestion
Spacer()
}
.frame(height: 36)
.animation(
reduceMotion ? nil : XTheme.transitionShort,
value: (appState.contextualHint, appState.activeNotes.isEmpty)
)
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| .onAppear { phase = true } | ||
| .animation( |
There was a problem hiding this comment.
issue (bug_risk): Live pulse never actually animates when isActive becomes true after appear.
phase is only set to true in onAppear and never updated, but the animation is keyed off phase. If isActive changes to true after the view appears, there’s no state change to start the animation, so opacity stays fixed. Consider driving the animation from isActive (e.g. update phase when isActive becomes true or key the animation directly off isActive) so the pulse begins when the active state toggles, not just on initial appearance.
| Spacer() | ||
| } | ||
| .frame(height: 36) | ||
| .animation(reduceMotion ? nil : XTheme.transitionShort, value: appState.contextualHint) | ||
| .animation(reduceMotion ? nil : XTheme.transitionShort, value: appState.activeNotes.isEmpty) | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Stacking multiple .animation modifiers on the same container can be hard to reason about.
With two .animation modifiers on the same HStack, SwiftUI will typically apply only the last one, so changes to contextualHint or activeNotes may not animate as you expect. Consider using a single animation keyed off both values (e.g., a tuple) or attaching the animation to the specific subview that should animate (such as the banner) to keep behavior predictable.
| Spacer() | |
| } | |
| .frame(height: 36) | |
| .animation(reduceMotion ? nil : XTheme.transitionShort, value: appState.contextualHint) | |
| .animation(reduceMotion ? nil : XTheme.transitionShort, value: appState.activeNotes.isEmpty) | |
| } | |
| } | |
| Spacer() | |
| } | |
| .frame(height: 36) | |
| .animation( | |
| reduceMotion ? nil : XTheme.transitionShort, | |
| value: (appState.contextualHint, appState.activeNotes.isEmpty) | |
| ) | |
| } | |
| } |
Settings sheets, popovers, and MAP pickers now wrap, scroll, or fall back to a menu when space is tight so tabs, segmented controls, and nested wizard sheets stay inside their parent. Co-authored-by: Harry Speight <sp80808@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: sp80808 <211996837+sp80808@users.noreply.github.com>
Code review — PLAY-surface micro-interactions and harmonic character cuesReviewed the full diff (
1. The LIVE pulse never starts for anything that becomes active later (medium)
@State private var phase = false
...
.onAppear { phase = true }
.animation(
isActive && !reduceMotion ? .easeInOut(duration: 1.15).repeatForever(autoreverses: true) : nil,
value: phase
)The repeating animation is attached to a single Suggested shape: .onChange(of: isActive) { active in phase = active ? true : false }or drive the pulse from 2.
|
Keep main's Play/Practice shell, theme tokens, and wheel layout while restoring sector fill, risk rings, stick comet, harmonic character cues, and settings containment from the PLAY micro-interactions work. Co-authored-by: Harry Speight <sp80808@users.noreply.github.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a8db106d-cebe-4190-8fd0-f39b17e73b0e) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Left a non-blocking comment: Cursor Bugbot was present but skipped (usage limit), so this head is not approved. Human review is needed; no reviewers were assigned because the only eligible human is the PR author.
Sent by Cursor Approval Agent: Pull Request Router and Approver
Summary
Adds PLAY-surface micro-interactions, then a containment pass so settings menus no longer clip or overflow.
Merge conflicts with
mainare resolved: this branch keeps main's Play/Practice shell, theme tokens, compact viewport, splitter layout, and per-pad ripple, and restores sector fill, risk rings, stick comet, harmonic-character cues, hover/press motion, and settings containment from the PLAY work.PLAY control surface
HarmonicTensionCharacterlabel + SF Symbol + ring weight)Settings containment
XWidthSafePicker)Light/Regular/Heavy/Bass)Test plan
swift test(macOS CI) includingHarmonicTensionCharacterandStringGauge.compactNametestsSummary by Sourcery
Add responsive PLAY-surface interactions and harmonic character cues while keeping controller and workspace settings contained across compact layouts.
New Features:
Bug Fixes:
Enhancements:
Tests: