feat(smplr): add sampler instruments#212
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Deploying patchies with
|
| Latest commit: |
12bf228
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fa8e179c.patchies.pages.dev |
| Branch Preview URL: | https://add-smplr-objects.patchies.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
ui/src/objects/smplr/SmplrInstrumentAudioNode.ts (1)
165-175: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid floating promise in
applyProgramChange.
void this.applySettings(...)creates an unawaited promise; ifapplyLiveSettingsorreloadthrows, it becomes an unhandled rejection. SinceapplyProgramChangeis only called from the synchronousapplyCommand, you can safely mark itasyncandawaitthe call, or catch and route errors toonStatusChange.- private applyProgramChange(program: number): void { + private async applyProgramChange(program: number): Promise<void> { const patch = this.descriptor.handleProgramChange?.(program, { ...this.settings, instrumentNames: this.instrument?.instrumentNames }); if (!patch) return; this.onSettingsPatch?.(patch); - void this.applySettings({ ...this.settings, ...patch }); + await this.applySettings({ ...this.settings, ...patch }); }Then update the call site in
applyCommand:case 'program': - this.applyProgramChange(command.program); + await this.applyProgramChange(command.program); break;And mark
applyCommandasasync(or handle the promise insend).- private applyCommand(command: ReturnType<typeof normalizeSmplrMessage>): void { + private async applyCommand(command: ReturnType<typeof normalizeSmplrMessage>): Promise<void> {Or, minimally, keep it synchronous but surface the error:
- void this.applySettings({ ...this.settings, ...patch }); + this.applySettings({ ...this.settings, ...patch }).catch((err) => { + this.onStatusChange?.({ state: 'error', message: String(err) }); + });🤖 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 `@ui/src/objects/smplr/SmplrInstrumentAudioNode.ts` around lines 165 - 175, The `applyProgramChange` flow in `SmplrInstrumentAudioNode` is creating a floating promise by calling `applySettings` with `void`, which can surface as an unhandled rejection. Make `applyProgramChange` async and await the `applySettings` call, then update the synchronous `applyCommand` path (and any direct caller like `send`) to await or otherwise handle the returned promise; if you keep it sync, catch errors from `applySettings` and route them through `onStatusChange` instead of dropping them.ui/src/objects/smplr/descriptors.test.ts (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test only asserts a declaration shape, not behavior.
Checking
typeof descriptor.loadInstrument === 'function'inspects an implementation detail rather than an observable outcome. Either drop it or convert it into a behavioral assertion (e.g., that invoking a descriptor's loader stays lazy / doesn't eagerly import heavy modules). As per coding guidelines: "Do NOT create tests that only inspect source text, declarations, prompts, imports, or implementation details."🤖 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 `@ui/src/objects/smplr/descriptors.test.ts` around lines 42 - 46, The current test in smplrDescriptors only checks the declaration shape of loadInstrument instead of observable behavior. Update the descriptors test to either remove this assertion or replace it with a behavioral test against smplrDescriptors that verifies the loader stays lazy (for example, invoking a descriptor’s loader does not eagerly import heavy modules). Focus the change in the descriptors test suite and the loadInstrument-related descriptor entries.Source: Coding guidelines
🤖 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 `@docs/design-docs/specs/165-smplr-sampled-instruments.md`:
- Around line 277-292: The spec for the Soundfont2 parser currently shows a
direct constructor call, but the implementation uses a closure-based adapter via
createSoundfont passed into module.Soundfont2(). Update the spec to describe the
local adapter pattern used by the Soundfont2 integration, making clear that
parser construction is wrapped in an indirection layer. Reference the soundfont2
parser section and the createSoundfont adapter so the documented contract
matches the actual API-evolution-resistant implementation.
- Around line 81-103: The SmplrInstrumentDescriptor spec is out of sync with the
real descriptor contract, so update the interface to match the implementation
used by SmplrInstrumentDescriptor and loadInstrument: include the missing
destination and onLoadProgress parameters, the getDisplayName(settings) shape,
and defaultVelocity, and remove updateLiveSetting if it is not part of the
canonical type. If this doc is meant to stay simplified, add an explicit note
that it is non-canonical and point readers to the shared descriptor definition
in smplr/descriptors for the authoritative interface.
In `@ui/src/lib/components/settings/ObjectSettings.svelte`:
- Around line 405-413: The trigger markup in ObjectSettings.svelte nests a
<button> inside Popover.Trigger, which already renders a button and creates
invalid interactive content. Update the Popover.Trigger usage around
getSelectedOptionLabel(field) and ChevronsUpDown to use the child snippet
pattern or apply the button styling directly to Popover.Trigger so there is only
one interactive element.
In `@ui/src/lib/nodes/defaultNodeData.ts`:
- Around line 395-397: The node defaults mapping in defaultNodeData should not
return the shared descriptor object from smplrDescriptors[type].defaultSettings.
Update the logic around the settings/settingsSchema creation so each node gets a
fresh deep copy of defaultSettings before it is assigned, using the existing
descriptor lookup in the mapping callback. Preserve settingsSchema as-is, but
ensure the returned settings value is cloned per node to avoid shared nested
state like soundfont2~ instrumentNames leaking across instances.
In `@ui/src/objects/smplr/descriptors.ts`:
- Around line 240-256: The SF2 loader in loadInstrument is dropping progress
updates because it omits onLoadProgress and passes a no-op into commonOptions,
so the status UI never advances during SoundFont2 loading. Update the
soundfont2~ descriptor to accept onLoadProgress from the loader args and forward
that real callback through commonOptions (and any Soundfont2 progress hook
exposed by smplr) instead of using an empty function, keeping behavior
consistent with the other descriptor loaders.
---
Nitpick comments:
In `@ui/src/objects/smplr/descriptors.test.ts`:
- Around line 42-46: The current test in smplrDescriptors only checks the
declaration shape of loadInstrument instead of observable behavior. Update the
descriptors test to either remove this assertion or replace it with a behavioral
test against smplrDescriptors that verifies the loader stays lazy (for example,
invoking a descriptor’s loader does not eagerly import heavy modules). Focus the
change in the descriptors test suite and the loadInstrument-related descriptor
entries.
In `@ui/src/objects/smplr/SmplrInstrumentAudioNode.ts`:
- Around line 165-175: The `applyProgramChange` flow in
`SmplrInstrumentAudioNode` is creating a floating promise by calling
`applySettings` with `void`, which can surface as an unhandled rejection. Make
`applyProgramChange` async and await the `applySettings` call, then update the
synchronous `applyCommand` path (and any direct caller like `send`) to await or
otherwise handle the returned promise; if you keep it sync, catch errors from
`applySettings` and route them through `onStatusChange` instead of dropping
them.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64c51042-ec03-4000-bc2c-ba9d139fbe3c
⛔ Files ignored due to path filters (2)
ui/bun.lockis excluded by!**/*.lockui/src/lib/generated/object-schemas.generated.tsis excluded by!**/generated/**
📒 Files selected for processing (45)
docs/design-docs/specs/165-smplr-sampled-instruments.mddocs/superpowers/plans/2026-06-27-smplr-sampled-instruments.mdui/package.jsonui/src/lib/ai/object-descriptions-types.tsui/src/lib/ai/object-prompts/index.tsui/src/lib/audio/v2/nodes/index.tsui/src/lib/components/settings/ObjectSettings.svelteui/src/lib/data/license-data.tsui/src/lib/extensions/object-packs.tsui/src/lib/nodes/defaultNodeData.tsui/src/lib/nodes/node-types.tsui/src/lib/objects/schemas/index.tsui/src/lib/settings/index.tsui/src/lib/settings/options.test.tsui/src/lib/settings/options.tsui/src/lib/settings/types.tsui/src/objects/smplr/DrumMachineNode.svelteui/src/objects/smplr/ElectricPianoNode.svelteui/src/objects/smplr/MalletNode.svelteui/src/objects/smplr/MellotronNode.svelteui/src/objects/smplr/PianoNode.svelteui/src/objects/smplr/SmolkenNode.svelteui/src/objects/smplr/SmplrInstrumentAudioNode.test.tsui/src/objects/smplr/SmplrInstrumentAudioNode.tsui/src/objects/smplr/SmplrNodeLayout.svelteui/src/objects/smplr/Soundfont2Node.svelteui/src/objects/smplr/SoundfontNode.svelteui/src/objects/smplr/VersilianNode.svelteui/src/objects/smplr/audio-nodes.tsui/src/objects/smplr/descriptors.test.tsui/src/objects/smplr/descriptors.tsui/src/objects/smplr/messages.test.tsui/src/objects/smplr/messages.tsui/src/objects/smplr/programs.tsui/src/objects/smplr/prompt.tsui/src/objects/smplr/schema.tsui/static/content/objects/drum-machine~.mdui/static/content/objects/epiano~.mdui/static/content/objects/mallet~.mdui/static/content/objects/mellotron~.mdui/static/content/objects/piano~.mdui/static/content/objects/smolken~.mdui/static/content/objects/soundfont2~.mdui/static/content/objects/soundfont~.mdui/static/content/objects/versilian~.md
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
heypoom
force-pushed
the
add-smplr-objects
branch
from
0b20154 to
e22da58
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-06-27-smplr-sampled-instruments.md (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix heading level for Task 1.
### Task 1should be## Task 1(or add a## Tasksh2 before it) to comply with standard Markdown heading increment rules. This keeps the document structure consistent with markdownlint expectations.-### Task 1: Dependencies And Spec Baseline +## Task 1: Dependencies And Spec Baseline(Apply the same change to all Task headings if they remain h3.)
🤖 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 `@docs/superpowers/plans/2026-06-27-smplr-sampled-instruments.md` at line 13, The Task headings in this plan are using an h3 level where the document structure expects an h2-level section or a preceding Tasks section. Update the heading for Task 1, and any other Task headings in this document, to use consistent Markdown hierarchy by either promoting them to ## or inserting a ## Tasks parent heading before the task list.
🤖 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 `@ui/src/objects/smplr/messages.ts`:
- Around line 139-140: The controlChange mapping in messages.ts only clamps
msg.value, so invalid CC numbers can still pass through unchanged. Update the
controlChange branch to normalize msg.control to a valid MIDI byte as well,
alongside msg.value, before returning the cc object. Use the existing
isFiniteNumber, clampMidi, and the controlChange handling logic in this module
to keep the fix localized and consistent.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-06-27-smplr-sampled-instruments.md`:
- Line 13: The Task headings in this plan are using an h3 level where the
document structure expects an h2-level section or a preceding Tasks section.
Update the heading for Task 1, and any other Task headings in this document, to
use consistent Markdown hierarchy by either promoting them to ## or inserting a
## Tasks parent heading before the task list.
🪄 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: 65d80ab4-ac24-48b9-b3de-6d2902d9c734
⛔ Files ignored due to path filters (2)
ui/bun.lockis excluded by!**/*.lockui/src/lib/generated/object-schemas.generated.tsis excluded by!**/generated/**
📒 Files selected for processing (45)
docs/design-docs/specs/165-smplr-sampled-instruments.mddocs/superpowers/plans/2026-06-27-smplr-sampled-instruments.mdui/package.jsonui/src/lib/ai/object-descriptions-types.tsui/src/lib/ai/object-prompts/index.tsui/src/lib/audio/v2/nodes/index.tsui/src/lib/components/settings/ObjectSettings.svelteui/src/lib/data/license-data.tsui/src/lib/extensions/object-packs.tsui/src/lib/nodes/defaultNodeData.tsui/src/lib/nodes/node-types.tsui/src/lib/objects/schemas/index.tsui/src/lib/settings/index.tsui/src/lib/settings/options.test.tsui/src/lib/settings/options.tsui/src/lib/settings/types.tsui/src/objects/smplr/DrumMachineNode.svelteui/src/objects/smplr/ElectricPianoNode.svelteui/src/objects/smplr/MalletNode.svelteui/src/objects/smplr/MellotronNode.svelteui/src/objects/smplr/PianoNode.svelteui/src/objects/smplr/SmolkenNode.svelteui/src/objects/smplr/SmplrInstrumentAudioNode.test.tsui/src/objects/smplr/SmplrInstrumentAudioNode.tsui/src/objects/smplr/SmplrNodeLayout.svelteui/src/objects/smplr/Soundfont2Node.svelteui/src/objects/smplr/SoundfontNode.svelteui/src/objects/smplr/VersilianNode.svelteui/src/objects/smplr/audio-nodes.tsui/src/objects/smplr/descriptors.test.tsui/src/objects/smplr/descriptors.tsui/src/objects/smplr/messages.test.tsui/src/objects/smplr/messages.tsui/src/objects/smplr/programs.tsui/src/objects/smplr/prompt.tsui/src/objects/smplr/schema.tsui/static/content/objects/drum-machine~.mdui/static/content/objects/epiano~.mdui/static/content/objects/mallet~.mdui/static/content/objects/mellotron~.mdui/static/content/objects/piano~.mdui/static/content/objects/smolken~.mdui/static/content/objects/soundfont2~.mdui/static/content/objects/soundfont~.mdui/static/content/objects/versilian~.md
✅ Files skipped from review due to trivial changes (14)
- ui/src/objects/smplr/VersilianNode.svelte
- ui/src/objects/smplr/Soundfont2Node.svelte
- ui/static/content/objects/mellotron~.md
- ui/src/objects/smplr/SmolkenNode.svelte
- ui/static/content/objects/epiano~.md
- ui/static/content/objects/drum-machine~.md
- ui/static/content/objects/versilian~.md
- ui/src/objects/smplr/ElectricPianoNode.svelte
- ui/static/content/objects/mallet~.md
- ui/static/content/objects/piano~.md
- ui/static/content/objects/soundfont2~.md
- ui/static/content/objects/smolken~.md
- ui/package.json
- ui/static/content/objects/soundfont~.md
🚧 Files skipped from review as they are similar to previous changes (28)
- ui/src/lib/settings/options.test.ts
- ui/src/objects/smplr/prompt.ts
- ui/src/objects/smplr/messages.test.ts
- ui/src/lib/ai/object-prompts/index.ts
- ui/src/objects/smplr/PianoNode.svelte
- ui/src/lib/nodes/defaultNodeData.ts
- ui/src/objects/smplr/programs.ts
- ui/src/objects/smplr/descriptors.test.ts
- ui/src/objects/smplr/audio-nodes.ts
- ui/src/objects/smplr/SoundfontNode.svelte
- ui/src/objects/smplr/DrumMachineNode.svelte
- ui/src/objects/smplr/MalletNode.svelte
- ui/src/lib/extensions/object-packs.ts
- ui/src/lib/settings/index.ts
- ui/src/objects/smplr/MellotronNode.svelte
- ui/src/lib/audio/v2/nodes/index.ts
- ui/src/lib/ai/object-descriptions-types.ts
- ui/src/lib/data/license-data.ts
- ui/src/lib/nodes/node-types.ts
- ui/src/lib/objects/schemas/index.ts
- ui/src/objects/smplr/SmplrInstrumentAudioNode.test.ts
- ui/src/lib/settings/options.ts
- ui/src/objects/smplr/descriptors.ts
- ui/src/lib/settings/types.ts
- ui/src/objects/smplr/schema.ts
- ui/src/objects/smplr/SmplrNodeLayout.svelte
- ui/src/lib/components/settings/ObjectSettings.svelte
- ui/src/objects/smplr/SmplrInstrumentAudioNode.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/lib/components/settings/ObjectSettings.svelte (1)
163-170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the combobox filter on every close path.
comboboxQuery[field.key]is only reset afterselectComboboxOption(). If the popover closes via outside click, Escape, or blur, reopening it keeps the stale filter and can hide most options immediately.Also applies to: 414-454
🤖 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 `@ui/src/lib/components/settings/ObjectSettings.svelte` around lines 163 - 170, The combobox filter state in ObjectSettings.svelte is only cleared in selectComboboxOption(), so closing the popover through other paths leaves comboboxQuery[field.key] stale. Update the combobox close handling in ObjectSettings.svelte so every close path (outside click, Escape, blur, and option selection) also resets comboboxQuery for the affected field, using the existing comboboxOpen/comboboxQuery state and the combobox-related handlers in the same component.
🧹 Nitpick comments (1)
ui/src/objects/smplr/descriptors.test.ts (1)
1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid locking descriptor order in this test.
These assertions are order-sensitive and mostly pin declaration shape, so a harmless reorder of
SMPLR_OBJECT_TYPESorsmplrDescriptorswill fail the suite without changing behavior. Prefer asserting membership/coverage and resolving each supported type throughgetSmplrDescriptor(...)instead of checking exact key order.As per coding guidelines,
**/*.test.{ts,tsx}: "Do NOT add 'guardrail' tests that lock wording or code shape unless that wording is itself a user-visible product contract."🤖 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 `@ui/src/objects/smplr/descriptors.test.ts` around lines 1 - 22, The smplr descriptors test is over-constraining declaration order by asserting exact array and object key ordering in the smplrDescriptors/SMPLR_OBJECT_TYPES checks. Update the test to verify the supported descriptor set by membership/coverage instead of exact order, and use getSmplrDescriptor(...) to confirm each object type resolves correctly rather than comparing Object.keys(...) to SMPLR_OBJECT_TYPES.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@ui/src/lib/components/settings/ObjectSettings.svelte`:
- Around line 163-170: The combobox filter state in ObjectSettings.svelte is
only cleared in selectComboboxOption(), so closing the popover through other
paths leaves comboboxQuery[field.key] stale. Update the combobox close handling
in ObjectSettings.svelte so every close path (outside click, Escape, blur, and
option selection) also resets comboboxQuery for the affected field, using the
existing comboboxOpen/comboboxQuery state and the combobox-related handlers in
the same component.
---
Nitpick comments:
In `@ui/src/objects/smplr/descriptors.test.ts`:
- Around line 1-22: The smplr descriptors test is over-constraining declaration
order by asserting exact array and object key ordering in the
smplrDescriptors/SMPLR_OBJECT_TYPES checks. Update the test to verify the
supported descriptor set by membership/coverage instead of exact order, and use
getSmplrDescriptor(...) to confirm each object type resolves correctly rather
than comparing Object.keys(...) to SMPLR_OBJECT_TYPES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b8b0175-7de3-43bf-a05c-61b59dc33879
📒 Files selected for processing (10)
docs/design-docs/specs/165-smplr-sampled-instruments.mdui/src/lib/components/settings/ObjectSettings.svelteui/src/lib/extensions/object-packs.tsui/src/lib/settings/types.tsui/src/lib/settings/visibility.test.tsui/src/lib/settings/visibility.tsui/src/objects/smplr/descriptors.test.tsui/src/objects/smplr/descriptors.tsui/src/objects/smplr/prompt.tsui/static/content/objects/soundfont~.md
✅ Files skipped from review due to trivial changes (4)
- ui/src/lib/settings/visibility.test.ts
- ui/src/objects/smplr/prompt.ts
- ui/static/content/objects/soundfont~.md
- docs/design-docs/specs/165-smplr-sampled-instruments.md
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/objects/smplr/descriptors.ts
heypoom
force-pushed
the
add-smplr-objects
branch
from
9021dcb to
644dc0b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
docs/superpowers/plans/2026-06-28-gm-smplr-multichannel.md (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix tech stack reference: "SvelteKit 5" → "Svelte 5".
SvelteKit versioning is independent of Svelte. The current major Svelte version is 5, but SvelteKit is at v2. This should read "Svelte 5" or "SvelteKit + Svelte 5".
🤖 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 `@docs/superpowers/plans/2026-06-28-gm-smplr-multichannel.md` at line 9, Update the tech stack line to use the correct framework version reference: replace “SvelteKit 5” with “Svelte 5” or “SvelteKit + Svelte 5”. Adjust the “Tech Stack” entry in the markdown so it reflects that SvelteKit and Svelte versioning are separate, and keep the rest of the stack list unchanged.ui/src/objects/smplr/GmAudioNode.ts (1)
180-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
ts-patternforSmplrCommanddispatch.This is discriminated-union branching on
command.type, so the currentswitchloses the exhaustiveness guarantee the repo standard expects here. As per coding guidelines,**/*.{ts,tsx}: "Usets-patternfor exhaustive branching on discriminated unions, enums, or mode/state values..."🤖 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 `@ui/src/objects/smplr/GmAudioNode.ts` around lines 180 - 241, The SmplrCommand dispatch in applyCommand is using a switch on command.type, which should be rewritten to the repo’s ts-pattern style for exhaustive discriminated-union handling. Update GmAudioNode.applyCommand to use ts-pattern matching over SmplrCommand so every command variant is covered explicitly and future variants are checked exhaustively, while preserving the current start, stop, stopAll, cc, program, volume, ignored, detune, and reverse behaviors.Source: Coding guidelines
ui/src/lib/objects/builtin-shorthands.test.ts (1)
47-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the registry path instead of the backing array.
This only proves the array entry exists and that its inline
transform()returns the expected literal. Please assertObjectShorthandRegistry.getInstance().tryTransform('gm~')instead. As per coding guidelines,**/*.test.{ts,tsx}: Test observable behavior through public APIs, rendered UI, store state, emitted events, tool results, or user-visible outcomes.🤖 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 `@ui/src/lib/objects/builtin-shorthands.test.ts` around lines 47 - 58, The test is asserting the shorthand entry directly from the backing array instead of exercising the public registry API. Update the gm~ test in builtin-shorthands.test.ts to use ObjectShorthandRegistry.getInstance().tryTransform('gm~') and assert on that returned result, so the test covers observable behavior through the registry path rather than the inline transform function.Source: Coding guidelines
🤖 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 `@docs/superpowers/plans/2026-06-28-gm-smplr-multichannel.md`:
- Around line 107-111: The focused test command for Task 5 Step 1 includes
unrelated settings tests, which should be removed from this verification step.
Update the test command in the plan so it only runs the SMPLR-focused paths
under src/objects/smplr, and leave out src/lib/settings/options.test.ts and
src/lib/settings/visibility.test.ts.
- Line 13: The task headings are using too deep a level after the top-level
heading, so update `Task 1: Channel Routing Helpers` and the other similar task
headings to the next appropriate markdown level. Adjust the heading markup in
the plan document so the section titles follow the document hierarchy correctly
and satisfy the markdown lint rule.
In `@ui/src/objects/midi.file/midi-file-player.ts`:
- Around line 312-340: The preload metadata helpers are skipping channels that
never emit a programChange, so channels using the GM default program 0 are not
warmed ahead of playback. Update getProgramStateAt and getUniqueProgramStates in
midi-file-player.ts to seed each used channel with program 0 when no prior
programChange exists, so the arrays consumed by
GmAudioNode.applyLoadedProgramMetadata() still include those channels. Keep the
existing sorting/shape of MidiFileProgramState output unchanged.
In `@ui/src/objects/smplr/gm-settings.ts`:
- Around line 48-56: The `drumInstrument` setting in `GM_SETTINGS` is shown too
broadly because `visibleWhen` only checks `source === 'soundfont'`, while
`createSoundfontInstrument()` only uses it when the kit is not `Custom`. Update
the visibility condition for the `drumInstrument` field to also exclude custom
kits so the UI matches the runtime behavior and only shows it when it can affect
Soundfont playback.
In `@ui/src/objects/smplr/GmAudioNode.ts`:
- Around line 373-381: `loadProgramInstrument()` is emitting the `ready` status
before the new channel has been added to `this.channels`, so `activeChannels` is
one behind. Update the status emission in `GmAudioNode.loadProgramInstrument()`
/ `ensureChannelInstrument()` flow so the channel is registered before calling
`onStatusChange`, or compute `activeChannels` from the pending channel count,
and keep the `updateMonitorChannel`/`onStatusChange` state in sync.
- Around line 676-678: Clamp loaded program values to the MIDI range 0..127 in
normalizeProgram so state stays aligned with program lookup behavior. Update the
GmAudioNode helper normalizeProgram to bound values above 127 as well as
negatives before writing into preloadRequests and channelState, matching the
normalization used by the program resolution helpers in programs.ts.
In `@ui/src/objects/smplr/GmNode.svelte`:
- Around line 68-71: The revert flow in revertSettings() is not handling the
audioService.send() result the same way as the normal settings write, so a
failed revert can leave persisted defaults out of sync with the runtime. Update
revertSettings() to await or otherwise handle the audioService.send(node.id,
'settings', GM_DEFAULT_SETTINGS) promise with the same success/error path used
for the regular settings save, and only commit the reverted state when the
runtime update succeeds.
- Around line 84-112: The async mount flow in onMount is not guarded against
teardown, so if the component unmounts while audioService.createNode() or the
initial audioService.send() is still pending, the continuation can still attach
runtimeNode callbacks and persist settings after onDestroy() has already cleaned
up. Add a mounted/aborted guard around the GmNode.svelte onMount sequence using
the existing node/messageContext/runtimeNode flow, and bail out before
installing onStatusChange/onMonitorChange or calling send() if onDestroy() has
run. Ensure onDestroy() marks the instance as disposed and that any
late-resolving createNode() path removes or ignores the orphaned node.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-06-28-gm-smplr-multichannel.md`:
- Line 9: Update the tech stack line to use the correct framework version
reference: replace “SvelteKit 5” with “Svelte 5” or “SvelteKit + Svelte 5”.
Adjust the “Tech Stack” entry in the markdown so it reflects that SvelteKit and
Svelte versioning are separate, and keep the rest of the stack list unchanged.
In `@ui/src/lib/objects/builtin-shorthands.test.ts`:
- Around line 47-58: The test is asserting the shorthand entry directly from the
backing array instead of exercising the public registry API. Update the gm~ test
in builtin-shorthands.test.ts to use
ObjectShorthandRegistry.getInstance().tryTransform('gm~') and assert on that
returned result, so the test covers observable behavior through the registry
path rather than the inline transform function.
In `@ui/src/objects/smplr/GmAudioNode.ts`:
- Around line 180-241: The SmplrCommand dispatch in applyCommand is using a
switch on command.type, which should be rewritten to the repo’s ts-pattern style
for exhaustive discriminated-union handling. Update GmAudioNode.applyCommand to
use ts-pattern matching over SmplrCommand so every command variant is covered
explicitly and future variants are checked exhaustively, while preserving the
current start, stop, stopAll, cc, program, volume, ignored, detune, and reverse
behaviors.
🪄 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: 8f8ea5fe-7a67-4eef-9466-9432f80f340a
⛔ Files ignored due to path filters (1)
ui/src/lib/generated/object-schemas.generated.tsis excluded by!**/generated/**
📒 Files selected for processing (38)
docs/design-docs/specs/165-smplr-sampled-instruments.mddocs/superpowers/plans/2026-06-28-gm-smplr-multichannel.mdui/src/lib/ai/object-descriptions-types.tsui/src/lib/ai/object-prompts/index.tsui/src/lib/components/settings/ObjectSettings.svelteui/src/lib/extensions/object-packs.tsui/src/lib/migration/migrate-patch.tsui/src/lib/migration/migrations/015-gm-to-ui-node.test.tsui/src/lib/migration/migrations/015-gm-to-ui-node.tsui/src/lib/nodes/defaultNodeData.tsui/src/lib/nodes/node-types.tsui/src/lib/objects/builtin-shorthands.test.tsui/src/lib/objects/builtin-shorthands.tsui/src/lib/objects/schemas/index.tsui/src/lib/services/NodeOperationsService.test.tsui/src/lib/settings/types.tsui/src/lib/settings/visibility.test.tsui/src/lib/settings/visibility.tsui/src/objects/midi.file/midi-file-player.test.tsui/src/objects/midi.file/midi-file-player.tsui/src/objects/midi.file/schema.tsui/src/objects/smplr/GmAudioNode.test.tsui/src/objects/smplr/GmAudioNode.tsui/src/objects/smplr/GmNode.svelteui/src/objects/smplr/SmplrNodeLayout.svelteui/src/objects/smplr/audio-nodes.tsui/src/objects/smplr/descriptors.test.tsui/src/objects/smplr/descriptors.tsui/src/objects/smplr/gm-channel-state.test.tsui/src/objects/smplr/gm-channel-state.tsui/src/objects/smplr/gm-settings.tsui/src/objects/smplr/programs.tsui/src/objects/smplr/prompt.tsui/src/objects/smplr/schema.tsui/static/content/objects/gm~.mdui/static/content/objects/midi.file.mdui/static/content/objects/soundfont2~.mdui/static/content/objects/soundfont~.md
✅ Files skipped from review due to trivial changes (5)
- ui/src/objects/smplr/gm-channel-state.test.ts
- ui/src/lib/migration/migrations/015-gm-to-ui-node.test.ts
- ui/static/content/objects/soundfont2~.md
- ui/static/content/objects/midi.file.md
- ui/src/lib/ai/object-descriptions-types.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- ui/src/lib/extensions/object-packs.ts
- ui/src/objects/smplr/audio-nodes.ts
- ui/src/lib/settings/visibility.test.ts
- ui/src/lib/nodes/defaultNodeData.ts
- ui/src/lib/ai/object-prompts/index.ts
- ui/src/lib/objects/schemas/index.ts
- ui/src/objects/smplr/prompt.ts
- ui/src/objects/smplr/schema.ts
- ui/src/lib/nodes/node-types.ts
- ui/src/objects/smplr/descriptors.test.ts
- ui/src/lib/components/settings/ObjectSettings.svelte
- docs/design-docs/specs/165-smplr-sampled-instruments.md
- ui/src/objects/smplr/descriptors.ts
- ui/src/objects/smplr/SmplrNodeLayout.svelte
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/objects/pads/PadsAudioNode.ts (1)
111-155: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep
onTriggeraligned with the scheduled start time.
triggerOn()still callsonTriggerbeforesource.start(). Whentimeis in the future, the pad flash now leads the actual audio, which breaks the "when a pad is triggered" contract on Line 57. Either fire the callback at the resolved start time or pass that time through so the component can schedule the flash correctly.🤖 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 `@ui/src/objects/pads/PadsAudioNode.ts` around lines 111 - 155, The `triggerOn` method in `PadsAudioNode` fires `onTrigger` immediately, but it should be aligned with the resolved `startTime` used for `source.start()` so scheduled notes don’t flash early. Update the callback flow so `onTrigger` is invoked at the same time as playback begins, or pass the computed start time through to the caller so the UI can schedule the flash correctly. Keep the fix localized to `triggerOn` and preserve the existing voice/gain handling.
🧹 Nitpick comments (1)
ui/src/objects/smplr/GmAudioNode.test.ts (1)
336-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid asserting Smplr constructor options directly.
expect(soundfontCalls[0]).toMatchObject(...)couples this test to the internalmodule.Soundfont(...)call shape instead of observable node behavior. The monitor snapshot assertion already covers the public contract; if you want stronger coverage, drive it throughsend()/status or monitor updates instead. As per coding guidelines,**/*.test.{ts,tsx}: "Test observable behavior through public APIs... Do NOT create tests that only inspect ... implementation details."Also applies to: 356-356
🤖 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 `@ui/src/objects/smplr/GmAudioNode.test.ts` around lines 336 - 340, The test in GmAudioNode.test is asserting the internal Soundfont constructor options via soundfontCalls, which is an implementation detail. Remove the direct expectation on module.Soundfont arguments and keep the snapshot/public-API assertions; if you need extra coverage, exercise the node through send() or monitor/status updates and verify observable behavior from GmAudioNode instead.Source: Coding guidelines
🤖 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 `@ui/src/objects/smplr/GmAudioNode.ts`:
- Around line 251-253: The ensureProgramInstrument cache-hit/preload-join path
in GmAudioNode only updates channels and status, so promoted instruments can
leave the monitor idle and miss live settings updates. In the
ensureProgramInstrument flow, after registering the channel, also refresh the
monitor state and reapply live settings for the promoted instrument so it
reflects the current monitor state and any later volume changes. Use the
existing GmAudioNode methods and fields around this branch, especially
this.channels, onStatusChange, applyLiveSettings, and the monitor update logic.
- Around line 213-220: Clamp the `programChange` value before it is written to
state or shown in the monitor in the `with({ type: 'program' })` branch of
`GmAudioNode`; right now `addPreloadRequest()` normalizes the value but
`setChannelProgram()` and `updateMonitorChannel()` still receive the raw
`programCommand.program`, causing `channelState` and monitor data to diverge
from the actual loaded instrument. Use the same normalized/clamped program value
consistently for `programChannels.add(channel)`, `setChannelProgram(...)`, and
`updateMonitorChannel(...)`, relying on the existing program-handling flow in
`GmAudioNode` and `addPreloadRequest()`.
---
Outside diff comments:
In `@ui/src/objects/pads/PadsAudioNode.ts`:
- Around line 111-155: The `triggerOn` method in `PadsAudioNode` fires
`onTrigger` immediately, but it should be aligned with the resolved `startTime`
used for `source.start()` so scheduled notes don’t flash early. Update the
callback flow so `onTrigger` is invoked at the same time as playback begins, or
pass the computed start time through to the caller so the UI can schedule the
flash correctly. Keep the fix localized to `triggerOn` and preserve the existing
voice/gain handling.
---
Nitpick comments:
In `@ui/src/objects/smplr/GmAudioNode.test.ts`:
- Around line 336-340: The test in GmAudioNode.test is asserting the internal
Soundfont constructor options via soundfontCalls, which is an implementation
detail. Remove the direct expectation on module.Soundfont arguments and keep the
snapshot/public-API assertions; if you need extra coverage, exercise the node
through send() or monitor/status updates and verify observable behavior from
GmAudioNode instead.
🪄 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: 7e265057-f21c-4cd7-8a33-8a4f9479b465
📒 Files selected for processing (22)
docs/design-docs/specs/97-pads-drum-sampler.mddocs/superpowers/plans/2026-06-27-smplr-sampled-instruments.mddocs/superpowers/plans/2026-06-28-gm-smplr-multichannel.mdui/src/lib/components/nodes/ScopeNode.svelteui/src/lib/objects/builtin-shorthands.test.tsui/src/lib/settings/types.tsui/src/lib/settings/visibility.test.tsui/src/lib/settings/visibility.tsui/src/objects/midi.file/midi-file-player.test.tsui/src/objects/midi.file/midi-file-player.tsui/src/objects/pads/PadsAudioNode.test.tsui/src/objects/pads/PadsAudioNode.tsui/src/objects/pads/prompt.tsui/src/objects/smplr/GmAudioNode.test.tsui/src/objects/smplr/GmAudioNode.tsui/src/objects/smplr/GmNode.svelteui/src/objects/smplr/SmplrNodeLayout.svelteui/src/objects/smplr/descriptors.test.tsui/src/objects/smplr/gm-settings.tsui/src/objects/smplr/messages.test.tsui/src/objects/smplr/messages.tsui/static/content/objects/pads~.md
✅ Files skipped from review due to trivial changes (4)
- ui/src/lib/components/nodes/ScopeNode.svelte
- docs/superpowers/plans/2026-06-27-smplr-sampled-instruments.md
- docs/superpowers/plans/2026-06-28-gm-smplr-multichannel.md
- ui/static/content/objects/pads~.md
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/src/lib/settings/visibility.ts
- ui/src/lib/objects/builtin-shorthands.test.ts
- ui/src/objects/smplr/messages.test.ts
- ui/src/objects/smplr/gm-settings.ts
- ui/src/objects/midi.file/midi-file-player.ts
- ui/src/objects/midi.file/midi-file-player.test.ts
- ui/src/objects/smplr/descriptors.test.ts
- ui/src/objects/smplr/GmNode.svelte
- ui/src/objects/smplr/SmplrNodeLayout.svelte
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/src/objects/pads/PadsAudioNode.test.ts (1)
79-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid asserting the internal
GainNodestate here.Line 79 couples the test to
PadsAudioNode's current wiring instead of a public contract. If the bang→velocity mapping needs coverage, extract that mapping into a small helper and test its input/output, while keeping this spec focused on observable effects like scheduled playback. As per coding guidelines,**/*.test.{ts,tsx}should "Test observable behavior through public APIs" and should "Do NOT create tests that only inspect ... implementation details."🤖 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 `@ui/src/objects/pads/PadsAudioNode.test.ts` at line 79, The test is asserting the internal GainNode state instead of a public behavior, so it is too coupled to PadsAudioNode’s implementation. Move the bang-to-velocity mapping into a small helper from PadsAudioNode and add direct input/output coverage there, then update PadsAudioNode.test.ts to verify observable effects only, such as scheduled playback or other public outcomes exposed by PadsAudioNode.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@ui/src/objects/pads/PadsAudioNode.test.ts`:
- Line 79: The test is asserting the internal GainNode state instead of a public
behavior, so it is too coupled to PadsAudioNode’s implementation. Move the
bang-to-velocity mapping into a small helper from PadsAudioNode and add direct
input/output coverage there, then update PadsAudioNode.test.ts to verify
observable effects only, such as scheduled playback or other public outcomes
exposed by PadsAudioNode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ade72b67-6cdb-4442-a612-875dcb6c31c4
⛔ Files ignored due to path filters (1)
ui/src/lib/generated/object-schemas.generated.tsis excluded by!**/generated/**
📒 Files selected for processing (6)
docs/design-docs/specs/97-pads-drum-sampler.mdui/src/objects/pads/PadsAudioNode.test.tsui/src/objects/pads/PadsAudioNode.tsui/src/objects/pads/prompt.tsui/src/objects/pads/schema.tsui/static/content/objects/pads~.md
✅ Files skipped from review due to trivial changes (2)
- ui/static/content/objects/pads~.md
- docs/design-docs/specs/97-pads-drum-sampler.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/src/objects/pads/prompt.ts
- ui/src/objects/pads/PadsAudioNode.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/src/objects/pads/PadsAudioNode.ts (1)
212-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate timer-scheduling pattern with
SamplerNode.ts.
scheduleTriggerFlash/clearTriggerTimerhere reimplement the same "delay a visual callback viasetTimeout, track per-key timers plus a bulk-clearSet" logic thatSamplerNode.tsimplements independently (schedulePlaybackStart/clearPlaybackStartTimer/playbackStartTimerSet). Consider extracting a small sharedTimerRegistry/ScheduledNotifierhelper (e.g.schedule(key, delayMs, cb),clear(key),clearAll()) used by both nodes to avoid maintaining two copies of the same cleanup logic.🤖 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 `@ui/src/objects/pads/PadsAudioNode.ts` around lines 212 - 241, The timer scheduling in PadsAudioNode’s scheduleTriggerFlash and clearTriggerTimer duplicates the same per-item setTimeout bookkeeping already present in SamplerNode’s schedulePlaybackStart, clearPlaybackStartTimer, and playbackStartTimerSet. Extract the shared delay-and-cleanup behavior into a small reusable helper such as TimerRegistry or ScheduledNotifier with methods like schedule, clear, and clearAll, then update both PadsAudioNode and SamplerNode to use it. Keep the existing Voice-specific and playback-specific callbacks, but remove the duplicated Set/timer cleanup logic from both classes.
🤖 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 `@ui/src/lib/components/nodes/SamplerNode.svelte`:
- Around line 350-369: The playback progress bar logic in SamplerNode.svelte is
treating all playback as a single session, but SamplerNoteVoiceManager can have
multiple overlapping voices. Update startPlaybackProgressBar and
stopPlaybackProgressBar to track active playback sources by ID or count instead
of a single isPlaying flag, and only initialize/reset the interval when
transitioning from zero active sources to one. Ensure
onPlaybackStart/onPlaybackStop only hide or stop the bar when no sources remain
active, so overlapping notes do not cancel each other’s progress.
---
Nitpick comments:
In `@ui/src/objects/pads/PadsAudioNode.ts`:
- Around line 212-241: The timer scheduling in PadsAudioNode’s
scheduleTriggerFlash and clearTriggerTimer duplicates the same per-item
setTimeout bookkeeping already present in SamplerNode’s schedulePlaybackStart,
clearPlaybackStartTimer, and playbackStartTimerSet. Extract the shared
delay-and-cleanup behavior into a small reusable helper such as TimerRegistry or
ScheduledNotifier with methods like schedule, clear, and clearAll, then update
both PadsAudioNode and SamplerNode to use it. Keep the existing Voice-specific
and playback-specific callbacks, but remove the duplicated Set/timer cleanup
logic from both classes.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0938936-7852-4ee2-bf51-e4b62effeaa6
📒 Files selected for processing (9)
docs/design-docs/specs/163-sampler-scheduled-playback.mddocs/design-docs/specs/97-pads-drum-sampler.mdui/src/lib/audio/v2/nodes/SamplerNode.test.tsui/src/lib/audio/v2/nodes/SamplerNode.tsui/src/lib/components/nodes/SamplerNode.svelteui/src/objects/pads/PadsAudioNode.test.tsui/src/objects/pads/PadsAudioNode.tsui/src/objects/smplr/GmAudioNode.test.tsui/src/objects/smplr/GmAudioNode.ts
✅ Files skipped from review due to trivial changes (2)
- docs/design-docs/specs/163-sampler-scheduled-playback.md
- docs/design-docs/specs/97-pads-drum-sampler.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/src/objects/smplr/GmAudioNode.test.ts
- ui/src/objects/smplr/GmAudioNode.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/lib/components/nodes/SamplerNode.svelte (1)
355-407: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAuto-stop still follows only the first active voice. The global
stopPlayback()path stops all sources, butactivePlaybackRate/effectiveEndare set only on the 0→1 transition, so overlapping voices with different rates or offsets can still trigger a whole-sampler stop based on the first voice’s estimate. Track end timing per source, or disable this auto-stop path once multiple voices are active.🤖 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 `@ui/src/lib/components/nodes/SamplerNode.svelte` around lines 355 - 407, The auto-stop logic in `startPlaybackProgressBar`/`stopPlaybackProgressBar` is still using a single shared `activePlaybackRate` and `effectiveEnd` from the first voice, so overlapping voices with different offsets or rates can incorrectly trigger `stopPlayback()`. Update the playback tracking in `SamplerNode.svelte` to compute end timing per source event (for example by tracking each active voice separately) or change the interval logic to skip auto-stopping once more than one voice is active. Keep the `stopPlayback()` and `startPlaybackProgressBar()` paths aligned so the sampler only stops when the actual active source(s) have finished.
🤖 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.
Outside diff comments:
In `@ui/src/lib/components/nodes/SamplerNode.svelte`:
- Around line 355-407: The auto-stop logic in
`startPlaybackProgressBar`/`stopPlaybackProgressBar` is still using a single
shared `activePlaybackRate` and `effectiveEnd` from the first voice, so
overlapping voices with different offsets or rates can incorrectly trigger
`stopPlayback()`. Update the playback tracking in `SamplerNode.svelte` to
compute end timing per source event (for example by tracking each active voice
separately) or change the interval logic to skip auto-stopping once more than
one voice is active. Keep the `stopPlayback()` and `startPlaybackProgressBar()`
paths aligned so the sampler only stops when the actual active source(s) have
finished.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cd9924c-035c-4116-99cb-81f51a9494d5
📒 Files selected for processing (1)
ui/src/lib/components/nodes/SamplerNode.svelte
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/src/lib/audio/sampler-playback-progress.test.ts (1)
10-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the loop-enabled wraparound branch.
All three tests exercise
loopEnabled: false. TheloopEnabled: truebranch inadvanceSamplerPlaybackProgress(resettingvoiceProgresstoloopStart) is untested, including the case where the step overshootsvoice.endbefore wrapping.🤖 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 `@ui/src/lib/audio/sampler-playback-progress.test.ts` around lines 10 - 75, Add a test in sampler-playback-progress.test.ts that exercises the loop-enabled path in advanceSamplerPlaybackProgress, specifically the branch that wraps voiceProgress back to loopStart when loopEnabled is true. Use the existing addVoice helper and the advanceSamplerPlaybackProgress function to cover an overshoot past voice.end in a single step, then assert the voice remains active and its progress is reset to loopStart instead of stopping playback.
🤖 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.
Nitpick comments:
In `@ui/src/lib/audio/sampler-playback-progress.test.ts`:
- Around line 10-75: Add a test in sampler-playback-progress.test.ts that
exercises the loop-enabled path in advanceSamplerPlaybackProgress, specifically
the branch that wraps voiceProgress back to loopStart when loopEnabled is true.
Use the existing addVoice helper and the advanceSamplerPlaybackProgress function
to cover an overshoot past voice.end in a single step, then assert the voice
remains active and its progress is reset to loopStart instead of stopping
playback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd19ecea-b694-44a3-b759-8a4e43fef492
📒 Files selected for processing (3)
ui/src/lib/audio/sampler-playback-progress.test.tsui/src/lib/audio/sampler-playback-progress.tsui/src/lib/components/nodes/SamplerNode.svelte
heypoom
force-pushed
the
add-smplr-objects
branch
from
ff7a7a1 to
12bf228
Compare
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.
Add sampler instruments from the smplr library
Summary by CodeRabbit
piano~,epiano~,drums~,mallet~,mellotron~,versilian~,smolken~, plussoundfont~,soundfont2~, and multi-channelgm~.pads~with optional absolute-time scheduling for triggers.gm~, sampled-instrument objects,soundfont~,soundfont2~, andmidi.fileloaded metadata.