feat(prism): implement tweak gradient model extension (#653)#656
Closed
plastikfan wants to merge 1 commit into
Closed
feat(prism): implement tweak gradient model extension (#653)#656plastikfan wants to merge 1 commit into
plastikfan wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR adds four new gradient visualiser implementations (Waveform, Sweep, Bloom, Bands) to the ChangesGradient visualisers and registry
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant Registry
participant Visualiser
participant Renderer
Bootstrap->>Registry: RegisterVisualisers()
Registry->>Visualiser: Register(Waveform/Sweep/Bloom/Bands)
Bootstrap->>Registry: Visualisers()
Registry-->>Bootstrap: copied slice
Bootstrap->>Renderer: Render(steps, curve, easing, animFrame)
Renderer-->>Bootstrap: styled ANSI string
Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/prism/workshop/visualiser.go (1)
32-64: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding the registry against concurrent access.
visualiserRegistryis a package-level slice mutated byRegister/Resetand read byVisualisers()with no synchronization. The doc comments imply single-threaded bootstrap usage (RegisterVisualisers"called only from the tweak bootstrap path"), so this is likely safe today, but if the workshop ever registers/reloads visualisers while an animation loop concurrently callsVisualisers(), this becomes a data race.🔒 Optional: guard registry with a mutex
+import "sync" + +var registryMu sync.Mutex var visualiserRegistry []GradientVisualiser func Register(v GradientVisualiser) { + registryMu.Lock() + defer registryMu.Unlock() visualiserRegistry = append(visualiserRegistry, v) } func Visualisers() []GradientVisualiser { + registryMu.Lock() + defer registryMu.Unlock() out := make([]GradientVisualiser, len(visualiserRegistry)) copy(out, visualiserRegistry) return out } func Reset() { + registryMu.Lock() + defer registryMu.Unlock() visualiserRegistry = visualiserRegistry[:0] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prism/workshop/visualiser.go` around lines 32 - 64, The package-level visualiser registry is accessed from Register, Reset, and Visualisers with no synchronization, so guard those paths against concurrent access. Add a mutex around visualiserRegistry in the workshop visualiser registry code, and ensure RegisterVisualisers, Register, Reset, and Visualisers all use the same lock so reads and writes remain safe if registration ever overlaps with iteration.src/prism/workshop/waveform.go (1)
68-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate style-construction logic between the two render loops.
The
stepForPosition/color-lookup/lipgloss.NewStylesequence (lines 68-73 and 81-86) is repeated. Consider extracting a small helper, e.g.colourStyle(steps []contract.Color, idx int) lipgloss.Style, to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prism/workshop/waveform.go` around lines 68 - 87, The waveform rendering in the two loops repeats the same step lookup and lipgloss style construction, so extract that shared logic into a small helper in waveform.go (for example, a function used by the row rendering code in the waveform renderer) and call it from both loops; keep the helper responsible for selecting the color from steps via stepForPosition and returning the lipgloss.Style, then use it for both the heightChars row and the flat sweep bar row.src/prism/workshop/bloom.go (1)
56-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate ring-rendering logic between upper and lower halves.
The stepIdx computation, color style construction, and character write (Lines 60-78 and 88-106) are identical between the two loops. Extracting a small helper (e.g.,
writeRing(b, ring, n, animFrame, easeFactor, steps)) would remove the duplication and centralize the negative-modulo fix above in one place.♻️ Proposed refactor
+func writeRing( + b *strings.Builder, ring, n, animFrame int, + easeFactor float64, rings int, steps []contract.Color, +) { + stepIdx := ringAt(ring, n, animFrame, easeFactor) + if stepIdx < 0 || stepIdx >= n { + stepIdx = 0 + } + c := steps[stepIdx] + ch := charForRing(ring, rings) + style := lipgloss.NewStyle().Foreground( + lipgloss.Color( + fmt.Sprintf("#%02x%02x%02x", c.R, c.G, c.B), + ), + ) + b.WriteString(style.Render(ch)) + b.WriteString(" ") +}Then both loops call
writeRing(&b, ring, n, animFrame, easeFactor, rings, steps).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prism/workshop/bloom.go` around lines 56 - 111, The upper and lower diamond loops in bloom rendering duplicate the same ring-to-style/write logic, so extract that repeated work into a shared helper to keep it centralized. Move the stepIdx lookup, bounds correction, color/style construction, and character rendering currently repeated in the main render function into a helper such as a writeRing-style function, and have both the upper and lower loops call it using the existing ring, n, animFrame, easeFactor, rings, and steps inputs.src/prism/workshop/sweep.go (1)
42-52: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: cache style per unique color to avoid redundant allocation.
lipgloss.NewStyle()/fmt.Sprintfrun once per column even though onlylen(steps)distinct colors exist; whenWidth > n, colors repeat via the modulo wrap. Caching styles by step index would reduce allocations on wide renders.♻️ Proposed optimization
var b strings.Builder n := len(steps) + styleCache := make([]lipgloss.Style, n) + cached := make([]bool, n) for col := 0; col < v.Width; col++ { stepIdx := (col + animFrame) % n if stepIdx < 0 { stepIdx += n } - c := steps[stepIdx] - style := lipgloss.NewStyle().Foreground( - lipgloss.Color(fmt.Sprintf("#%02x%02x%02x", c.R, c.G, c.B)), - ) + if !cached[stepIdx] { + c := steps[stepIdx] + styleCache[stepIdx] = lipgloss.NewStyle().Foreground( + lipgloss.Color(fmt.Sprintf("#%02x%02x%02x", c.R, c.G, c.B)), + ) + cached[stepIdx] = true + } + style := styleCache[stepIdx] b.WriteString(style.Render("█")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prism/workshop/sweep.go` around lines 42 - 52, The loop in the sweep renderer recalculates the same lipgloss style and hex color string for repeated step colors, causing redundant allocations when width exceeds the number of steps. Update the rendering logic in the sweep code to cache styles by unique step index or color in the function that builds the row, and reuse the cached style for repeated `steps[stepIdx]` values instead of calling `lipgloss.NewStyle()` and `fmt.Sprintf` on every column.src/prism/workshop/sweep_test.go (1)
21-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add a negative
animFramecase.
sweep.goexplicitly guards against negativestepIdxafter the modulo (implying negativeanimFrameis an anticipated input), but no test entry exercises that branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prism/workshop/sweep_test.go` around lines 21 - 48, Add a negative animFrame test case to the gradient rendering table in the Sweep tests, since Render in the sweep logic already handles negative step indexes after modulo and that branch is currently untested. Extend the DescribeTable in the sweep_test.go suite with an Entry that calls v.Render through the existing stepsEntry path using a negative AnimFrame value, and keep the same assertions for non-empty ANSI output and block characters. This should target the Render method and the makeSteps helper so the new case exercises the negative-frame branch without changing the existing positive cases.src/prism/workshop/bands.go (1)
60-65: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInconsistent style-reuse between the two render rows; consider caching per-color styles.
Row 1 builds
mutedStyleonce outside the loop (Lines 56-58) and reuses it, but Row 2 constructs a brand-newlipgloss.NewStyle()plus afmt.Sprintfhex string on every column (Lines 75-81). SincestepForPositionfrequently maps many adjacent columns to the same step index (e.g.Width=40with only 2-8 steps), this recomputes an identical style/string many times perRendercall, which runs on every animation tick.Caching styles by step index would align with the pattern already used for the muted row and avoid redundant allocation on a hot path.
♻️ Proposed fix to cache styles per step index
// Row 2: gradient bar + styleCache := make(map[int]lipgloss.Style, n) for col := 0; col < v.Width; col++ { t := float64(col) / float64(v.Width) stepIdx := stepForPosition(t, n, curve) - c := steps[stepIdx] - style := lipgloss.NewStyle().Foreground( - lipgloss.Color( - fmt.Sprintf( - "#%02x%02x%02x", c.R, c.G, c.B, - ), - ), - ) + style, ok := styleCache[stepIdx] + if !ok { + c := steps[stepIdx] + style = lipgloss.NewStyle().Foreground( + lipgloss.Color( + fmt.Sprintf("#%02x%02x%02x", c.R, c.G, c.B), + ), + ) + styleCache[stepIdx] = style + } b.WriteString(style.Render("█")) }Also applies to: 70-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prism/workshop/bands.go` around lines 60 - 65, Row 2 in the bands renderer is rebuilding the same per-step color style on every column, unlike the reused mutedStyle pattern used earlier in Render. Update the row loop in bands.go so the style for each step index is cached and reused across columns, using the existing stepForPosition and color selection logic to key the cache. Keep the behavior the same, but avoid calling lipgloss.NewStyle and formatting the hex color repeatedly on this hot path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/prism/workshop/bands.go`:
- Around line 27-29: The registered BandsVisualiser is being created with Width
left unset, which causes Render to return empty output when it checks v.Width <=
0. Update RegisterVisualisers() to initialize BandsVisualiser with a valid
Width, or set a default Width inside BandsVisualiser/its render path so the
registry-created instance can render correctly. Use the BandsVisualiser type,
its Render method, and RegisterVisualisers() in visualiser.go to locate the fix.
In `@src/prism/workshop/bloom.go`:
- Around line 131-136: The ring color index calculation in ringAt can become
negative when easeFactor is negative, which breaks wrapping and causes callers
to clamp many rings to 0. Update ringAt to use a true modulo/wraparound
implementation that always returns a value in [0, n), and keep the existing
callers in bloom rendering relying on ringAt unchanged so the gradient cycles
correctly for all easing modes.
In `@src/prism/workshop/sweep.go`:
- Around line 17-19: The registered SweepVisualiser is created with Width left
at its zero value, so its Render behavior will never produce output. Update
RegisterVisualisers() to initialize the SweepVisualiser with a non-zero Width
before it is added, or ensure the Width field is set immediately after
construction and before exposure to the workshop UI; use the SweepVisualiser
type and its Render method as the key places to fix.
In `@src/prism/workshop/waveform.go`:
- Around line 105-119: The EaseInOut branch in animationSpeed is effectively
dead code because it always returns the same value as default, so update
animationSpeed in waveform.go to accept animFrame (or another time-varying
input) and use it in the EaseInOut calculation. Thread that new parameter
through the call site(s) that compute waveform animation speed so the branch
actually oscillates instead of returning a constant. Keep the behavior for
EasingKindEaseIn and EasingKindEaseOut unchanged.
---
Nitpick comments:
In `@src/prism/workshop/bands.go`:
- Around line 60-65: Row 2 in the bands renderer is rebuilding the same per-step
color style on every column, unlike the reused mutedStyle pattern used earlier
in Render. Update the row loop in bands.go so the style for each step index is
cached and reused across columns, using the existing stepForPosition and color
selection logic to key the cache. Keep the behavior the same, but avoid calling
lipgloss.NewStyle and formatting the hex color repeatedly on this hot path.
In `@src/prism/workshop/bloom.go`:
- Around line 56-111: The upper and lower diamond loops in bloom rendering
duplicate the same ring-to-style/write logic, so extract that repeated work into
a shared helper to keep it centralized. Move the stepIdx lookup, bounds
correction, color/style construction, and character rendering currently repeated
in the main render function into a helper such as a writeRing-style function,
and have both the upper and lower loops call it using the existing ring, n,
animFrame, easeFactor, rings, and steps inputs.
In `@src/prism/workshop/sweep_test.go`:
- Around line 21-48: Add a negative animFrame test case to the gradient
rendering table in the Sweep tests, since Render in the sweep logic already
handles negative step indexes after modulo and that branch is currently
untested. Extend the DescribeTable in the sweep_test.go suite with an Entry that
calls v.Render through the existing stepsEntry path using a negative AnimFrame
value, and keep the same assertions for non-empty ANSI output and block
characters. This should target the Render method and the makeSteps helper so the
new case exercises the negative-frame branch without changing the existing
positive cases.
In `@src/prism/workshop/sweep.go`:
- Around line 42-52: The loop in the sweep renderer recalculates the same
lipgloss style and hex color string for repeated step colors, causing redundant
allocations when width exceeds the number of steps. Update the rendering logic
in the sweep code to cache styles by unique step index or color in the function
that builds the row, and reuse the cached style for repeated `steps[stepIdx]`
values instead of calling `lipgloss.NewStyle()` and `fmt.Sprintf` on every
column.
In `@src/prism/workshop/visualiser.go`:
- Around line 32-64: The package-level visualiser registry is accessed from
Register, Reset, and Visualisers with no synchronization, so guard those paths
against concurrent access. Add a mutex around visualiserRegistry in the workshop
visualiser registry code, and ensure RegisterVisualisers, Register, Reset, and
Visualisers all use the same lock so reads and writes remain safe if
registration ever overlaps with iteration.
In `@src/prism/workshop/waveform.go`:
- Around line 68-87: The waveform rendering in the two loops repeats the same
step lookup and lipgloss style construction, so extract that shared logic into a
small helper in waveform.go (for example, a function used by the row rendering
code in the waveform renderer) and call it from both loops; keep the helper
responsible for selecting the color from steps via stepForPosition and returning
the lipgloss.Style, then use it for both the heightChars row and the flat sweep
bar row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5243e06c-a449-40af-b8f9-9c2c2ad5ed34
📒 Files selected for processing (12)
src/prism/workshop/bands.gosrc/prism/workshop/bands_test.gosrc/prism/workshop/bloom.gosrc/prism/workshop/bloom_test.gosrc/prism/workshop/helpers_test.gosrc/prism/workshop/sweep.gosrc/prism/workshop/sweep_test.gosrc/prism/workshop/visualiser.gosrc/prism/workshop/visualiser_test.gosrc/prism/workshop/waveform.gosrc/prism/workshop/waveform_test.gosrc/prism/workshop/workshop-suite_test.go
Comment on lines
+27
to
+29
| type BandsVisualiser struct { | ||
| Width int | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how/if Width gets set on visualisers obtained via the registry.
fd . -e go --full-path 'src/prism/workshop' -x cat -n {} \; 2>/dev/null | grep -n "func Register\|func Visualisers\|\.Width"
ast-grep run --pattern 'func Register($_) $_' --lang go src/prism/workshop
ast-grep run --pattern 'func Visualisers($$$) $_' --lang go src/prism/workshop
rg -n -B2 -A15 'func RegisterVisualisers' src/prism/workshop/visualiser.go
rg -nP '\.Width\s*=' src/prism/workshopRepository: snivilised/jaywalk
Length of output: 1790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any direct mutations of Width on BandsVisualiser or in the workshop package.
rg -n --hidden --glob '!**/vendor/**' --glob '!**/.git/**' \
'BandsVisualiser|\.Width\s*=|Width:\s*' .
# Inspect the workshop registry and render call sites for any width-setting path.
rg -n --hidden --glob '!**/vendor/**' --glob '!**/.git/**' \
'Visualisers\(\)|Render\(' src/prism/workshop
# Show the relevant workshop files around registry/render definitions.
sed -n '1,140p' src/prism/workshop/visualiser.go
sed -n '1,120p' src/prism/workshop/bands.goRepository: snivilised/jaywalk
Length of output: 15709
BandsVisualiser is registered with Width left at zero, so the registry path renders empty output. RegisterVisualisers() adds &BandsVisualiser{} and nothing in GradientVisualiser or visualiser.go sets Width, so the registered instance always hits v.Width <= 0 and returns "".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/prism/workshop/bands.go` around lines 27 - 29, The registered
BandsVisualiser is being created with Width left unset, which causes Render to
return empty output when it checks v.Width <= 0. Update RegisterVisualisers() to
initialize BandsVisualiser with a valid Width, or set a default Width inside
BandsVisualiser/its render path so the registry-created instance can render
correctly. Use the BandsVisualiser type, its Render method, and
RegisterVisualisers() in visualiser.go to locate the fix.
Comment on lines
+131
to
+136
| func ringAt( | ||
| ring, n, animFrame int, easeFactor float64, | ||
| ) int { | ||
| offset := int(float64(animFrame) * easeFactor) | ||
| return (ring + offset) % n | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Negative modulo can silently zero out ring colors.
easeFactor can go negative for EaseInOut (0.3 + 0.7*sin(...) ranges down to -0.4), making offset negative. Go's % preserves the sign of the dividend, so (ring + offset) % n can be negative; callers then clamp to 0 (Lines 64-66, 92-94), collapsing many rings to the same color instead of properly wrapping around the gradient. Use a true modulo instead.
🔧 Proposed fix
func ringAt(
ring, n, animFrame int, easeFactor float64,
) int {
offset := int(float64(animFrame) * easeFactor)
- return (ring + offset) % n
+ return ((ring+offset)%n + n) % n
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func ringAt( | |
| ring, n, animFrame int, easeFactor float64, | |
| ) int { | |
| offset := int(float64(animFrame) * easeFactor) | |
| return (ring + offset) % n | |
| } | |
| func ringAt( | |
| ring, n, animFrame int, easeFactor float64, | |
| ) int { | |
| offset := int(float64(animFrame) * easeFactor) | |
| return ((ring+offset)%n + n) % n | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/prism/workshop/bloom.go` around lines 131 - 136, The ring color index
calculation in ringAt can become negative when easeFactor is negative, which
breaks wrapping and causes callers to clamp many rings to 0. Update ringAt to
use a true modulo/wraparound implementation that always returns a value in [0,
n), and keep the existing callers in bloom rendering relying on ringAt unchanged
so the gradient cycles correctly for all easing modes.
Comment on lines
+17
to
+19
| type SweepVisualiser struct { | ||
| Width int | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any code that sets Width on registered visualisers after RegisterVisualisers()
rg -n -A3 -B3 '\.Width\s*=' --type=go src/prism/workshop
rg -n -A5 -B5 'RegisterVisualisers\(\)' --type=go src/prism/workshopRepository: snivilised/jaywalk
Length of output: 1623
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== sweep.go outline ==\n'
ast-grep outline src/prism/workshop/sweep.go --view expanded || true
printf '\n== visualiser.go outline ==\n'
ast-grep outline src/prism/workshop/visualiser.go --view expanded || true
printf '\n== sweep.go (numbered) ==\n'
cat -n src/prism/workshop/sweep.go
printf '\n== search for Width references in prism/workshop ==\n'
rg -n -A2 -B2 'Width|SweepVisualiser|GradientVisualiser|RegisterVisualisers\(' src/prism/workshop
printf '\n== search for interface/factory usage across repo ==\n'
rg -n -A2 -B2 'GradientVisualiser|RegisterVisualisers\(|SweepVisualiser|Width\s*=' srcRepository: snivilised/jaywalk
Length of output: 24126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== consumers of Visualisers() ==\n'
rg -n -A4 -B4 'Visualisers\(\)' src
printf '\n== consumers of RegisterVisualisers() ==\n'
rg -n -A4 -B4 'RegisterVisualisers\(\)' src
printf '\n== type assertions / direct use of SweepVisualiser ==\n'
rg -n -A4 -B4 'SweepVisualiser|GradientVisualiser' src | sed -n '1,260p'
printf '\n== any field mutation on visualiser registry results ==\n'
rg -n -A3 -B3 '\[\].*GradientVisualiser|visualiserRegistry|Register\(' src/prism/workshopRepository: snivilised/jaywalk
Length of output: 15415
Initialize SweepVisualiser.Width before registration
RegisterVisualisers() adds &SweepVisualiser{} with Width == 0, so Render() will always return "" for the registered instance. Pass the width in at construction time, or set it before the visualiser is exposed to the workshop UI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/prism/workshop/sweep.go` around lines 17 - 19, The registered
SweepVisualiser is created with Width left at its zero value, so its Render
behavior will never produce output. Update RegisterVisualisers() to initialize
the SweepVisualiser with a non-zero Width before it is added, or ensure the
Width field is set immediately after construction and before exposure to the
workshop UI; use the SweepVisualiser type and its Render method as the key
places to fix.
Comment on lines
+105
to
+119
| // animationSpeed returns a multiplier based on the easing | ||
| // preset. EaseIn accelerates, EaseOut decelerates, | ||
| // EaseInOut oscillates. | ||
| func animationSpeed(easing enums.EasingKind) float64 { | ||
| switch easing { //nolint:exhaustive // ok | ||
| case enums.EasingKindEaseIn: | ||
| return 1.5 | ||
| case enums.EasingKindEaseOut: | ||
| return 0.5 | ||
| case enums.EasingKindEaseInOut: | ||
| return 1.0 + math.Sin(float64(0)) | ||
| default: | ||
| return 1.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
EaseInOut never oscillates — dead-code constant.
math.Sin(float64(0)) always evaluates to 0, so this branch always returns 1.0, identical to the default case. The doc comment states "EaseInOut oscillates," but there's no dependency on animFrame or any time-varying input to actually produce oscillation.
🐛 Proposed fix
func animationSpeed(easing enums.EasingKind) float64 {
switch easing { //nolint:exhaustive // ok
case enums.EasingKindEaseIn:
return 1.5
case enums.EasingKindEaseOut:
return 0.5
- case enums.EasingKindEaseInOut:
- return 1.0 + math.Sin(float64(0))
+ case enums.EasingKindEaseInOut:
+ return 1.0 + math.Sin(float64(animFrame)*0.1)
default:
return 1.0
}
}Note: this requires threading animFrame into animationSpeed's signature and its call site.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/prism/workshop/waveform.go` around lines 105 - 119, The EaseInOut branch
in animationSpeed is effectively dead code because it always returns the same
value as default, so update animationSpeed in waveform.go to accept animFrame
(or another time-varying input) and use it in the EaseInOut calculation. Thread
that new parameter through the call site(s) that compute waveform animation
speed so the branch actually oscillates instead of returning a constant. Keep
the behavior for EasingKindEaseIn and EasingKindEaseOut unchanged.
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Tests