feat(smplr): add sampler instruments#212
Open
heypoom wants to merge 17 commits into
Open
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 25 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughAdds sampled-instrument support across settings, message/program normalization, descriptors, runtime nodes, GM multi-channel behavior, registry wiring, and object docs. ChangesSmplr Sampled Instruments
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 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: |
6abed9e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://92aa8ae8.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
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
New Features
piano~,epiano~,drums~,mallet~,mellotron~,versilian~,smolken~, plussoundfont~,soundfont2~, and multi-channelgm~.bang,programChange,controlChange, gain/detune/reverse) and descriptor-driven playback with improved sampled-instrument settings UI, including searchable comboboxes and conditional field visibility.Bug Fixes
noteOnvelocity0acting asnoteOff).Documentation
gm~, all sampled instruments, and SoundFont objects, plus updated MIDI-file messaging notes.Tests