Add step sequencer with audio engine fixes - #1
Merged
Conversation
- New standalone step sequencer TUI (simplesampler-seq) with 16-step grid, cursor navigation, pattern switching, count-in, and metronome - Fix audio crackling: configurable blocksize (1024 for sequencer), _Voice class with __slots__ for faster callback, voice cap (64), optimized drain loop - Add [/] keys for prev/next pattern navigation with wrap-around, solving access to 10+ patterns beyond the 1-9 shortcuts - Master config (ss_config.toml) with metronome and sequencer settings - Pattern save/load, bar-boundary pattern switching, BPM control - Non-blocking UI callbacks to keep engine thread off the GIL
📝 WalkthroughWalkthroughAdds a Textual step-sequencer UI, a daemon-threaded SequencerEngine with metronome/count‑in/pattern switching, Pydantic config and sequence schemas with persistence, refactored audio playback (typed Voice, blocksize/voice limits), CLI/dev entry points, and sample preloading. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SequencerApp
participant SequencerEngine
participant AudioPlayer
participant FileSystem
User->>SequencerApp: launch(bank_path, pattern_path, config)
SequencerApp->>FileSystem: load_config(), load bank & pattern files
FileSystem-->>SequencerApp: SSConfig, samples, SequenceFile
SequencerApp->>SequencerApp: preload samples (cap)
SequencerApp->>SequencerEngine: init(audio, sequence, cache, callbacks)
User->>SequencerApp: action_toggle_play()
SequencerApp->>SequencerEngine: start()
activate SequencerEngine
SequencerEngine->>AudioPlayer: play metronome / play_data(sample)
loop Each Step
SequencerEngine->>SequencerApp: on_step(step)
SequencerApp->>SequencerApp: update playhead & UI
SequencerApp->>User: render grid update
end
User->>SequencerApp: action_toggle_play()
SequencerApp->>SequencerEngine: stop()
deactivate SequencerEngine
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simplesampler/audio/playback.py (1)
148-152:⚠️ Potential issue | 🟡 MinorMulti-channel WAV files (>2 channels) will produce a shape mismatch in the audio callback.
When
channels > 2and no resampling is needed,audio_floathas shape(n, channels)which will fail when mixed into the 2-channeloutdatabuffer. Consider either down-mixing to stereo or rejecting unsupported channel counts.Proposed fix
# Reshape channels if channels == 1: audio_float = np.column_stack((audio_float, audio_float)) - else: + elif channels == 2: audio_float = audio_float.reshape(-1, channels) + else: + # Down-mix to stereo: take first two channels + audio_float = audio_float.reshape(-1, channels)[:, :2]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/audio/playback.py` around lines 148 - 152, The audio callback currently assumes mono or stereo by reshaping audio_float based only on channels, which breaks for channels > 2; update the handling in the block that checks channels (references: audio_float, channels, outdata) to either (1) down-mix any multichannel input to stereo (e.g., average or weighted-mix channels into two columns) before writing to outdata, or (2) explicitly reject/raise a clear error for unsupported channel counts > 2; implement the down-mix or error path in the same conditional branch where audio_float is reshaped so outdata always receives a (n, 2) array.
🧹 Nitpick comments (5)
src/simplesampler/sequencer/app.py (3)
313-317: Silent exception swallowing when building cell cache.The bare
except Exception: passcould hide real layout bugs (e.g., missing cells due to a compose error). Consider logging at debug level.try: cell = self.query_one(f"#cell-{pad_id}-{s}", StepCell) self._cells[(pad_id, s)] = cell - except Exception: - pass + except Exception as exc: + print(f"Warning: cell #{pad_id}-{s} not found: {exc}", file=sys.stderr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/sequencer/app.py` around lines 313 - 317, The try/except around self.query_one(f"#cell-{pad_id}-{s}", StepCell) silently swallows all exceptions and hides layout bugs; replace the bare except with catching Exception as e and log the failure at debug (including pad_id, s, the selector string and exception info) before continuing, so update the block that populates self._cells[(pad_id, s)] to log the error via the module/class logger instead of pass while still allowing execution to proceed.
283-283: Unused loop variablerow_idx.Rename to
_per convention. Flagged by Ruff (B007).- for row_idx, pad_id in enumerate(self.pad_ids): + for _, pad_id in enumerate(self.pad_ids):Or simply
for pad_id in self.pad_ids:since the index is unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/sequencer/app.py` at line 283, The loop uses an unused index variable `row_idx` when iterating over `self.pad_ids`; update the loop in the method containing `for row_idx, pad_id in enumerate(self.pad_ids):` to either drop the index (`for pad_id in self.pad_ids:`) or rename the index to `_` (`for _, pad_id in enumerate(self.pad_ids):`) to satisfy Ruff (B007) and remove the unused variable.
520-535: Replace private Textual APIs with officialpost_message()for thread-safe, non-blocking UI updates.The code accesses
self._loopandself._context(), which are private/undocumented Textual internals. Textual's official, stable API for non-blocking thread-to-UI communication ispost_message(), which is thread-safe and designed for this fire-and-forget pattern. Alternatively,call_from_thread()is also stable but is blocking (waits for callback completion).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/sequencer/app.py` around lines 520 - 535, The _post_to_main method uses private Textual internals (self._loop, self._context()) and asyncio.run_coroutine_threadsafe; replace this with Textual's public thread-safe post_message API: create a simple Message subclass (e.g., CallbackMessage) that carries the callback and args, call self.post_message(CallbackMessage(...)) from the calling thread, and implement an on_callback_message (or on_message handler) in the app to invoke the callback(*args) on the UI thread; remove references to self._loop, self._context(), and run_coroutine_threadsafe and keep call_from_thread only if you need blocking behavior.src/simplesampler/schemas/ss_config.py (1)
20-20: Ambiguous EN DASH character in comment.Line 20 uses
–(EN DASH U+2013) instead of-(HYPHEN-MINUS U+002D) in# 0.0 – 1.0. Flagged by Ruff (RUF003).Fix
- volume: float = 0.7 # 0.0 – 1.0 + volume: float = 0.7 # 0.0 - 1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/schemas/ss_config.py` at line 20, Replace the EN DASH character in the inline comment for the volume default to a standard ASCII hyphen-minus: locate the volume variable declaration (volume: float = 0.7) in ss_config.py and change the comment text from "# 0.0 – 1.0" to "# 0.0 - 1.0" (ensure the hyphen is U+002D so Ruff RUF003 is resolved and file encoding remains unchanged).src/seq.py (1)
6-6: Roundaboutsys.pathmanipulation.
os.path.join(os.path.dirname(__file__), "simplesampler", "..")resolves back toos.path.dirname(__file__). Simplify:-sys.path.insert(0, os.path.join(os.path.dirname(__file__), "simplesampler", "..")) +sys.path.insert(0, os.path.dirname(__file__))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/seq.py` at line 6, The sys.path insertion in src/seq.py is redundant because os.path.join(os.path.dirname(__file__), "simplesampler", "..") resolves to os.path.dirname(__file__); replace that line to either remove the sys.path manipulation entirely if imports work without it, or explicitly insert the simplesampler directory with sys.path.insert(0, os.path.join(os.path.dirname(__file__), "simplesampler")) so the intended package is added instead of the project root; update the single line containing sys.path.insert(...) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/simplesampler/schemas/ss_config.py`:
- Around line 53-57: Malformed or invalid TOML currently raises
tomllib.TomlDecodeError or pydantic.ValidationError and crashes; wrap the
tomllib.load + SSConfig.model_validate calls inside a try/except in the loop
that iterates over paths, catch tomllib.TomlDecodeError and
pydantic.ValidationError, log a warning (including the path and error) and then
either continue to the next path or return a safe default SSConfig (e.g.
construct SSConfig with defaults) so the app doesn't crash; ensure you reference
SSConfig.model_validate and tomllib.load in the try/except and do not let raw
exceptions propagate.
In `@src/simplesampler/sequencer/app.py`:
- Around line 338-341: The code accessing
self.sequence.patterns[self._pending_pattern] can raise IndexError if
self._pending_pattern refers to a deleted/stale index; update the logic in the
method that builds the pending string (where pending = "" / pend_name = ...) to
first validate that self._pending_pattern is not None and is a valid index into
self.sequence.patterns (e.g., 0 <= self._pending_pattern <
len(self.sequence.patterns)); if the index is out of range, clear
self._pending_pattern (set to None) and leave pending as empty, otherwise safely
read .name; ensure this same validation is applied or invoked after pattern
deletions (see action_delete_pattern) to avoid stale references.
- Around line 663-676: action_load_patterns currently assigns
self.engine.sequence while the engine thread may be running, causing race
conditions; modify it to check engine state, stop the engine if it's running
(e.g., call engine.stop() or engine.pause()), then perform
SequenceFile.load(path), set self.sequence and self.engine.sequence, call
_sync_grid_from_sequence(), _update_cursor(), _refresh_status(), and finally
restart the engine only if it was running before (e.g., engine.start() or
engine.resume()); ensure any available engine locking or thread-safe API on the
engine is used around the swap to avoid races.
- Around line 264-268: In compose(), remove the unused local variable
assignments pat_name = self._current_pattern_name(), pat_count =
len(self.sequence.patterns), and sig = self.sequence.time_signature (they are
flagged as unused); keep total_steps if used elsewhere, or otherwise remove any
other unused locals—locate these in the compose method and delete the three
unused assignments referencing _current_pattern_name, sequence.patterns, and
sequence.time_signature.
- Around line 635-644: action_delete_pattern mutates self.sequence.patterns
while playback may queue pattern switches; update action_delete_pattern to
prevent race by either stopping the engine before mutating or explicitly
invalidating pending switches: call the engine stop/pause method (or acquire its
lock) before popping the pattern, and after removal set any queued pending
pattern references to None (clear the sequencer's queued `_pending_pattern` and
the engine instance's `_pending_pattern`) before calling
_sync_grid_from_sequence(), _update_cursor(), and _refresh_status(). Ensure you
reference and clear the attributes named `_pending_pattern` on both the
sequencer/queue and the engine and only resume playback after the mutation is
complete.
In `@src/simplesampler/sequencer/engine.py`:
- Around line 88-97: The start() method can spawn two threads because _playing
is only set inside _run(); add a short-lived guard flag (e.g., self._starting)
checked alongside self._playing at the top of start(), set self._starting = True
just before spawning the new thread and clear it inside _run() once the count-in
either completes or the thread exits; update any early-return logic to check
both self._playing and self._starting, and ensure the existing _stop_event
handling remains per-thread-safe (clear _stop_event only when actually starting
a fresh run) so two concurrent threads cannot be created during count-in.
- Line 150: The code caches total_steps into a local variable (total_steps =
self.sequence.total_steps) before the playback loop which becomes stale if
self.sequence is reassigned (e.g., action_load_patterns in app.py); change the
loop to read self.sequence.total_steps each iteration (or re-evaluate
total_steps immediately after any sequence swap or stop playback before
replacing) so modulo wrapping uses the current sequence length; search for the
local variable total_steps and the playback loop in the Engine class/method to
update the read site to self.sequence.total_steps each iteration or add logic to
refresh total_steps when self.sequence is replaced.
---
Outside diff comments:
In `@src/simplesampler/audio/playback.py`:
- Around line 148-152: The audio callback currently assumes mono or stereo by
reshaping audio_float based only on channels, which breaks for channels > 2;
update the handling in the block that checks channels (references: audio_float,
channels, outdata) to either (1) down-mix any multichannel input to stereo
(e.g., average or weighted-mix channels into two columns) before writing to
outdata, or (2) explicitly reject/raise a clear error for unsupported channel
counts > 2; implement the down-mix or error path in the same conditional branch
where audio_float is reshaped so outdata always receives a (n, 2) array.
---
Nitpick comments:
In `@src/seq.py`:
- Line 6: The sys.path insertion in src/seq.py is redundant because
os.path.join(os.path.dirname(__file__), "simplesampler", "..") resolves to
os.path.dirname(__file__); replace that line to either remove the sys.path
manipulation entirely if imports work without it, or explicitly insert the
simplesampler directory with sys.path.insert(0,
os.path.join(os.path.dirname(__file__), "simplesampler")) so the intended
package is added instead of the project root; update the single line containing
sys.path.insert(...) accordingly.
In `@src/simplesampler/schemas/ss_config.py`:
- Line 20: Replace the EN DASH character in the inline comment for the volume
default to a standard ASCII hyphen-minus: locate the volume variable declaration
(volume: float = 0.7) in ss_config.py and change the comment text from "# 0.0 –
1.0" to "# 0.0 - 1.0" (ensure the hyphen is U+002D so Ruff RUF003 is resolved
and file encoding remains unchanged).
In `@src/simplesampler/sequencer/app.py`:
- Around line 313-317: The try/except around
self.query_one(f"#cell-{pad_id}-{s}", StepCell) silently swallows all exceptions
and hides layout bugs; replace the bare except with catching Exception as e and
log the failure at debug (including pad_id, s, the selector string and exception
info) before continuing, so update the block that populates self._cells[(pad_id,
s)] to log the error via the module/class logger instead of pass while still
allowing execution to proceed.
- Line 283: The loop uses an unused index variable `row_idx` when iterating over
`self.pad_ids`; update the loop in the method containing `for row_idx, pad_id in
enumerate(self.pad_ids):` to either drop the index (`for pad_id in
self.pad_ids:`) or rename the index to `_` (`for _, pad_id in
enumerate(self.pad_ids):`) to satisfy Ruff (B007) and remove the unused
variable.
- Around line 520-535: The _post_to_main method uses private Textual internals
(self._loop, self._context()) and asyncio.run_coroutine_threadsafe; replace this
with Textual's public thread-safe post_message API: create a simple Message
subclass (e.g., CallbackMessage) that carries the callback and args, call
self.post_message(CallbackMessage(...)) from the calling thread, and implement
an on_callback_message (or on_message handler) in the app to invoke the
callback(*args) on the UI thread; remove references to self._loop,
self._context(), and run_coroutine_threadsafe and keep call_from_thread only if
you need blocking behavior.
Comment on lines
+663
to
+676
| def action_load_patterns(self): | ||
| path = self.pattern_path or self._default_pattern_path() | ||
| if not os.path.isfile(path): | ||
| self.notify(f"File not found: {path}", severity="warning") | ||
| return | ||
| try: | ||
| self.sequence = SequenceFile.load(path) | ||
| self.engine.sequence = self.sequence | ||
| self._sync_grid_from_sequence() | ||
| self._update_cursor() | ||
| self._refresh_status() | ||
| self.notify(f"Loaded: {path}", severity="information") | ||
| except Exception as e: | ||
| self.notify(f"Load failed: {e}", severity="error") |
There was a problem hiding this comment.
Loading patterns during playback replaces the sequence without stopping the engine.
self.engine.sequence = self.sequence (Line 670) swaps the sequence object that the engine thread is actively iterating over, which can cause stale total_steps, missed bar boundaries, or IndexErrors in the engine's _run loop. Consider stopping the engine before loading.
Proposed fix
def action_load_patterns(self):
path = self.pattern_path or self._default_pattern_path()
if not os.path.isfile(path):
self.notify(f"File not found: {path}", severity="warning")
return
try:
+ was_playing = self.engine.playing
+ if was_playing:
+ self.engine.stop()
+ self._playhead = -1
+ self._clear_playhead()
self.sequence = SequenceFile.load(path)
self.engine.sequence = self.sequence
self._sync_grid_from_sequence()
self._update_cursor()
self._refresh_status()
self.notify(f"Loaded: {path}", severity="information")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def action_load_patterns(self): | |
| path = self.pattern_path or self._default_pattern_path() | |
| if not os.path.isfile(path): | |
| self.notify(f"File not found: {path}", severity="warning") | |
| return | |
| try: | |
| self.sequence = SequenceFile.load(path) | |
| self.engine.sequence = self.sequence | |
| self._sync_grid_from_sequence() | |
| self._update_cursor() | |
| self._refresh_status() | |
| self.notify(f"Loaded: {path}", severity="information") | |
| except Exception as e: | |
| self.notify(f"Load failed: {e}", severity="error") | |
| def action_load_patterns(self): | |
| path = self.pattern_path or self._default_pattern_path() | |
| if not os.path.isfile(path): | |
| self.notify(f"File not found: {path}", severity="warning") | |
| return | |
| try: | |
| was_playing = self.engine.playing | |
| if was_playing: | |
| self.engine.stop() | |
| self._playhead = -1 | |
| self._clear_playhead() | |
| self.sequence = SequenceFile.load(path) | |
| self.engine.sequence = self.sequence | |
| self._sync_grid_from_sequence() | |
| self._update_cursor() | |
| self._refresh_status() | |
| self.notify(f"Loaded: {path}", severity="information") | |
| except Exception as e: | |
| self.notify(f"Load failed: {e}", severity="error") |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 675-675: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/simplesampler/sequencer/app.py` around lines 663 - 676,
action_load_patterns currently assigns self.engine.sequence while the engine
thread may be running, causing race conditions; modify it to check engine state,
stop the engine if it's running (e.g., call engine.stop() or engine.pause()),
then perform SequenceFile.load(path), set self.sequence and
self.engine.sequence, call _sync_grid_from_sequence(), _update_cursor(),
_refresh_status(), and finally restart the engine only if it was running before
(e.g., engine.start() or engine.resume()); ensure any available engine locking
or thread-safe API on the engine is used around the swap to avoid races.
| if self.on_playback_start: | ||
| self.on_playback_start() | ||
|
|
||
| total_steps = self.sequence.total_steps |
There was a problem hiding this comment.
total_steps cached before loop becomes stale if sequence is replaced mid-playback.
action_load_patterns in app.py (Line 670) reassigns self.engine.sequence during playback. The local total_steps here would still reflect the old value, causing modulo wrapping on the wrong bar length. Consider re-reading self.sequence.total_steps each iteration, or stopping playback before loading.
Proposed fix
- total_steps = self.sequence.total_steps
next_time = time.perf_counter()
while not self._stop_event.is_set():
+ total_steps = self.sequence.total_steps
# Recalculate interval in case BPM changed
interval = self._step_interval()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/simplesampler/sequencer/engine.py` at line 150, The code caches
total_steps into a local variable (total_steps = self.sequence.total_steps)
before the playback loop which becomes stale if self.sequence is reassigned
(e.g., action_load_patterns in app.py); change the loop to read
self.sequence.total_steps each iteration (or re-evaluate total_steps immediately
after any sequence swap or stop playback before replacing) so modulo wrapping
uses the current sequence length; search for the local variable total_steps and
the playback loop in the Engine class/method to update the read site to
self.sequence.total_steps each iteration or add logic to refresh total_steps
when self.sequence is replaced.
- Add _starting guard flag to engine.start() preventing double thread spawn during count-in; update playing property to reflect count-in state - Wrap _run() in try/finally via _run_inner() for guaranteed flag cleanup - Stop engine before sequence swap in action_load_patterns to prevent data race with daemon thread - Clear pending pattern refs before list mutation in action_delete_pattern - Bounds-check _pending_pattern in _status_text to handle stale index - Handle malformed TOML/invalid config in ss_config.py load_config() - Remove unused variables in compose()
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/simplesampler/sequencer/app.py (3)
280-280: Unused loop variablerow_idx.Rename to
_per convention.Proposed fix
- for row_idx, pad_id in enumerate(self.pad_ids): + for _, pad_id in enumerate(self.pad_ids):Or simply
for pad_id in self.pad_ids:since the index is unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/sequencer/app.py` at line 280, The loop uses an unused index variable `row_idx` in the statement `for row_idx, pad_id in enumerate(self.pad_ids):`; remove the unused variable by changing the loop to `for pad_id in self.pad_ids:` (or if you prefer keeping enumerate for clarity use `_` as in `for _, pad_id in enumerate(self.pad_ids):`) so the unused `row_idx` is eliminated; update any references to `row_idx` if present (none expected) and run tests to confirm no behavior change.
310-314: Silentexcept: passswallows cell-cache build failures.If a cell widget is missing from the DOM (e.g., due to a compose bug), this will silently produce an incomplete
_cellscache, leading to mysterious UI glitches where steps don't highlight. At minimum, log a debug message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/sequencer/app.py` around lines 310 - 314, The empty except is hiding failures when building the cell cache; update the try/except around the query_one call (referencing query_one, StepCell, and self._cells[(pad_id, s)]) to catch a specific exception (e.g., LookupError/ComponentNotFound or Exception as e) and emit a debug-level log that includes pad_id, s and the exception message instead of silently passing, so missing DOM cells are recorded for diagnosis while preserving existing behavior when appropriate.
520-535: Replace private Textual_loopand_context()with public APIs.
self._loopandself._context()are private implementation details (leading underscore) with no stability guarantees. Textual usescontextvarsinternally, and these attributes may change or be removed in any release.The supported alternative is
App.call_from_thread(), a public API for thread-safe UI updates. Since the current fire-and-forget pattern avoids blocking the audio engine thread (which is critical for timing), wrapself.call_later()insidecall_from_thread, or post a customMessageinstead — both are stable public APIs and preserve non-blocking behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/sequencer/app.py` around lines 520 - 535, The _post_to_main method uses private Textual attributes self._loop and self._context() which are unstable; replace this implementation to use the public App.call_from_thread API to schedule non-blocking UI updates: inside _post_to_main invoke self.call_from_thread(...) and within that callback either call self.call_later(...) with the provided callback and args or post a custom Message (self.post_message(...)) to trigger widget mutations safely; update references to _post_to_main (and remove reliance on self._loop/self._context) so the audio thread remains non-blocking while using stable Textual APIs.src/simplesampler/sequencer/engine.py (1)
101-111:stop()resets state without waiting for the thread to actually exit.
stop()sets_stop_eventand immediately clears_playing/_starting/_current_step. Meanwhile, the daemon thread in_run'sfinallyblock also resets the same flags. This creates a brief window wherestop()sets_current_step = 0, the thread hasn't exited yet, and_run's finally block will set it to 0 again (harmless) — but the_playingflag is also reset by both. Currently no real bug because the thread only setsFalse, but if logic evolves this pattern is fragile. A comment noting this intentional dual-reset would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/sequencer/engine.py` around lines 101 - 111, stop() currently flips the _stop_event and immediately clears _playing/_starting/_current_step while the daemon thread's _run finally block also resets the same flags, which is intentional to avoid join() but can confuse future maintainers; add a concise comment inside the stop() method referencing the _run method and explaining that the immediate reset of flags is deliberate (to avoid deadlocks with call_from_thread and because the thread will also clear them on exit), and note that both sides setting the same flags is safe and expected so future changes don't remove one side inadvertently.src/simplesampler/schemas/ss_config.py (1)
21-21: Ambiguous Unicode character in comment.Ruff flags the
–(EN DASH) in the comment. Replace with a standard hyphen-to avoid potential tooling warnings.Proposed fix
- volume: float = 0.7 # 0.0 – 1.0 + volume: float = 0.7 # 0.0 - 1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simplesampler/schemas/ss_config.py` at line 21, The comment on the volume field uses an EN DASH which triggers tooling warnings; update the comment in ss_config.py for the symbol volume (volume: float = 0.7) to use a standard ASCII hyphen so it reads "0.0 - 1.0" instead of "0.0 – 1.0".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/simplesampler/sequencer/app.py`:
- Around line 239-260: The current _preload_samples uses os.path.getsize
(on-disk size) to enforce MAX_PRELOAD_BYTES which miscounts decoded memory;
change the logic in _preload_samples to decode first via self.audio.load_wav
into a temporary variable (or load then inspect), measure its actual memory with
ndarray.nbytes, and only insert into self.sample_cache[pad.id] and increment
total_size if that decoded nbytes keeps total_size + decoded_nbytes <=
MAX_PRELOAD_BYTES; if it would exceed the cap, discard the decoded array and log
"Skipped" instead; keep exception handling around load_wav and ensure total_size
tracks decoded bytes not file size so memory accounting is accurate.
- Around line 703-710: The cleanup path calls AudioPlayer.cleanup twice via
action_quit -> self.audio.cleanup() then exit() -> on_unmount() ->
self.audio.cleanup(), and AudioPlayer.cleanup currently calls self.stream.stop()
/ self.stream.close() with no guards; make cleanup idempotent by checking the
stream state before touching it (e.g., in AudioPlayer.cleanup check if
self.stream is truthy and not getattr(self.stream, "closed", False) before
calling stop()/close(), then set self.stream = None or a _closed flag after
closing), and optionally remove the redundant self.audio.cleanup() from either
action_quit or on_unmount so cleanup is only invoked once.
---
Duplicate comments:
In `@src/simplesampler/sequencer/app.py`:
- Around line 635-649: The deletion is still racy because the engine may be
iterating over sequence.patterns while you pop() — stop the engine before
mutating the list and restart it afterward (mirror action_load_patterns
behavior). In action_delete_pattern: check whether the engine is currently
running (use the same flag/method action_load_patterns uses), store that state,
clear self._pending_pattern and self.engine._pending_pattern, call
self.engine.stop() (or the project's stop/wait helper), perform
sequence.patterns.pop(idx) and adjust self.sequence.active_pattern, call
_sync_grid_from_sequence/_update_cursor/_refresh_status, and finally restart the
engine only if it was running before. This prevents concurrent iteration in
engine._run_inner from seeing a mutated list.
In `@src/simplesampler/sequencer/engine.py`:
- Line 163: The code currently caches total_steps into the local variable
total_steps before the playback loop, which can become stale if steps_per_beat
or time_signature change at runtime; update the playback loop to re-read
self.sequence.total_steps each iteration (just like interval is re-read) and use
that value inside the loop instead of the pre-cached total_steps so changes to
the sequence are respected by SequencerEngine (look for the playback loop that
references total_steps and interval and replace the cached usage with
self.sequence.total_steps per-iteration).
---
Nitpick comments:
In `@src/simplesampler/schemas/ss_config.py`:
- Line 21: The comment on the volume field uses an EN DASH which triggers
tooling warnings; update the comment in ss_config.py for the symbol volume
(volume: float = 0.7) to use a standard ASCII hyphen so it reads "0.0 - 1.0"
instead of "0.0 – 1.0".
In `@src/simplesampler/sequencer/app.py`:
- Line 280: The loop uses an unused index variable `row_idx` in the statement
`for row_idx, pad_id in enumerate(self.pad_ids):`; remove the unused variable by
changing the loop to `for pad_id in self.pad_ids:` (or if you prefer keeping
enumerate for clarity use `_` as in `for _, pad_id in enumerate(self.pad_ids):`)
so the unused `row_idx` is eliminated; update any references to `row_idx` if
present (none expected) and run tests to confirm no behavior change.
- Around line 310-314: The empty except is hiding failures when building the
cell cache; update the try/except around the query_one call (referencing
query_one, StepCell, and self._cells[(pad_id, s)]) to catch a specific exception
(e.g., LookupError/ComponentNotFound or Exception as e) and emit a debug-level
log that includes pad_id, s and the exception message instead of silently
passing, so missing DOM cells are recorded for diagnosis while preserving
existing behavior when appropriate.
- Around line 520-535: The _post_to_main method uses private Textual attributes
self._loop and self._context() which are unstable; replace this implementation
to use the public App.call_from_thread API to schedule non-blocking UI updates:
inside _post_to_main invoke self.call_from_thread(...) and within that callback
either call self.call_later(...) with the provided callback and args or post a
custom Message (self.post_message(...)) to trigger widget mutations safely;
update references to _post_to_main (and remove reliance on
self._loop/self._context) so the audio thread remains non-blocking while using
stable Textual APIs.
In `@src/simplesampler/sequencer/engine.py`:
- Around line 101-111: stop() currently flips the _stop_event and immediately
clears _playing/_starting/_current_step while the daemon thread's _run finally
block also resets the same flags, which is intentional to avoid join() but can
confuse future maintainers; add a concise comment inside the stop() method
referencing the _run method and explaining that the immediate reset of flags is
deliberate (to avoid deadlocks with call_from_thread and because the thread will
also clear them on exit), and note that both sides setting the same flags is
safe and expected so future changes don't remove one side inadvertently.
Comment on lines
+239
to
+260
| def _preload_samples(self): | ||
| print("Preloading samples...", file=sys.stderr) | ||
| total_size = 0 | ||
| for pad in self.bank.pads: | ||
| if pad.sample_path and os.path.exists(pad.sample_path): | ||
| try: | ||
| size = os.path.getsize(pad.sample_path) | ||
| if total_size + size <= MAX_PRELOAD_BYTES: | ||
| self.sample_cache[pad.id] = self.audio.load_wav(pad.sample_path) | ||
| total_size += size | ||
| print( | ||
| f" Loaded: {pad.name} ({size / 1024:.1f} KB)", | ||
| file=sys.stderr, | ||
| ) | ||
| else: | ||
| print(f" Skipped: {pad.name} (cache full)", file=sys.stderr) | ||
| except Exception as e: | ||
| print(f" Error: {pad.sample_path}: {e}", file=sys.stderr) | ||
| print( | ||
| f"Preload complete: {total_size / 1024 / 1024:.2f} MB cached", | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
Cache size limit compares raw file size, not decoded array size.
os.path.getsize returns the on-disk WAV file size, but load_wav decodes it into a float32 stereo numpy array, which can be ~6× larger (16-bit mono WAV → float32 stereo). A 10 MB file-size cap could result in ~60 MB of actual heap usage, making MAX_PRELOAD_BYTES misleading.
Consider tracking self.sample_cache[pad.id].nbytes after loading instead:
Proposed fix
for pad in self.bank.pads:
if pad.sample_path and os.path.exists(pad.sample_path):
try:
- size = os.path.getsize(pad.sample_path)
- if total_size + size <= MAX_PRELOAD_BYTES:
- self.sample_cache[pad.id] = self.audio.load_wav(pad.sample_path)
- total_size += size
+ data = self.audio.load_wav(pad.sample_path)
+ arr_size = data.nbytes
+ if total_size + arr_size <= MAX_PRELOAD_BYTES:
+ self.sample_cache[pad.id] = data
+ total_size += arr_size
print(
- f" Loaded: {pad.name} ({size / 1024:.1f} KB)",
+ f" Loaded: {pad.name} ({arr_size / 1024:.1f} KB)",
file=sys.stderr,
)Note: this approach does decode every file once (even if it's ultimately skipped due to cap), but keeps the memory accounting accurate. If that's a concern, a rough estimate from file size could be used as a pre-check.
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 255-255: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/simplesampler/sequencer/app.py` around lines 239 - 260, The current
_preload_samples uses os.path.getsize (on-disk size) to enforce
MAX_PRELOAD_BYTES which miscounts decoded memory; change the logic in
_preload_samples to decode first via self.audio.load_wav into a temporary
variable (or load then inspect), measure its actual memory with ndarray.nbytes,
and only insert into self.sample_cache[pad.id] and increment total_size if that
decoded nbytes keeps total_size + decoded_nbytes <= MAX_PRELOAD_BYTES; if it
would exceed the cap, discard the decoded array and log "Skipped" instead; keep
exception handling around load_wav and ensure total_size tracks decoded bytes
not file size so memory accounting is accurate.
Comment on lines
+703
to
+710
| async def action_quit(self): | ||
| self.engine.stop() | ||
| self.audio.cleanup() | ||
| self.exit() | ||
|
|
||
| def on_unmount(self): | ||
| self.engine.stop() | ||
| self.audio.cleanup() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find AudioPlayer class and its cleanup method
fd -e py -e pyc . | xargs rg -l "class AudioPlayer" | head -5Repository: NotoriousArnav/SimpleSampler
Length of output: 107
🏁 Script executed:
# Search for AudioPlayer class definition and cleanup method
rg "class AudioPlayer" -A 50 -B 2Repository: NotoriousArnav/SimpleSampler
Length of output: 3746
🏁 Script executed:
# Find the exit() and on_unmount() methods in app.py
rg "def exit|def on_unmount" src/simplesampler/sequencer/app.py -A 5Repository: NotoriousArnav/SimpleSampler
Length of output: 167
🏁 Script executed:
# Check if there are any try/except blocks around audio.cleanup() calls
rg "audio\.cleanup" src/simplesampler/sequencer/app.py -B 2 -A 2Repository: NotoriousArnav/SimpleSampler
Length of output: 265
🌐 Web query:
sounddevice OutputStream stop close idempotent already closed stream behavior
💡 Result:
In python-sounddevice, OutputStream.stop() and OutputStream.close() are not implemented as idempotent “no-ops if already closed” methods.
Stream.close()unconditionally callsPa_CloseStream(self._ptr)and then setsself._ptr = NULL(even if PortAudio returns an error). Callingclose()again will therefore callPa_CloseStream(NULL)(i.e., on an invalid pointer). Whether that merely returns an error or misbehaves depends on the PortAudio build/host API;sounddeviceitself doesn’t guard against it. [1]Stream.stop()similarly callsPa_StopStream(self._ptr)with no “already closed” check, sostop()afterclose()will callPa_StopStream(NULL). By defaultignore_errors=True, sosounddevicewon’t raise even if PortAudio reports an error, but it still makes the underlying call. [1]- You can reliably test
stream.closed(Trueiff_ptr == NULL) before stopping/closing. [2]
Practical pattern:
if not stream.closed:
stream.stop() # waits for queued audio to play out (PortAudio semantics)
stream.close() # discards pending buffers if still active(stop() vs close() behavior comes from PortAudio: Pa_StopStream() waits for queued buffers; Pa_CloseStream() will abort if needed before closing.) [3][4]
Sources: sounddevice implementation of stop(), close(), and closed [1][2]; PortAudio stream start/stop/abort/close overview [3][4].
References: [1] (python-sounddevice.readthedocs.io) [2] (python-sounddevice.readthedocs.io) [3] (portaudio.com) [4] (portaudio.com)
Citations:
- 1: https://python-sounddevice.readthedocs.io/en/latest/_modules/sounddevice.html
- 2: https://python-sounddevice.readthedocs.io/en/latest/_modules/sounddevice.html
- 3: https://www.portaudio.com/docs/v19-doxydocs/api_overview.html?utm_source=openai
- 4: https://www.portaudio.com/docs/proposals/010-ClarifyStreamStateMachine.html?utm_source=openai
audio.cleanup() is called twice on quit, and AudioPlayer.cleanup() lacks idempotent guards.
action_quit() calls self.audio.cleanup() then self.exit(), which triggers on_unmount() and calls cleanup() again. While engine.stop() is idempotent, AudioPlayer.cleanup() directly calls self.stream.stop() and self.stream.close() with no guards. Calling these sounddevice methods on an already-closed stream will invoke PortAudio functions with invalid pointers; sounddevice's ignore_errors=True default masks the issue but the underlying calls still occur.
Guard with if not self.stream.closed: before stopping/closing, or refactor the exit flow to call cleanup() once.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/simplesampler/sequencer/app.py` around lines 703 - 710, The cleanup path
calls AudioPlayer.cleanup twice via action_quit -> self.audio.cleanup() then
exit() -> on_unmount() -> self.audio.cleanup(), and AudioPlayer.cleanup
currently calls self.stream.stop() / self.stream.close() with no guards; make
cleanup idempotent by checking the stream state before touching it (e.g., in
AudioPlayer.cleanup check if self.stream is truthy and not getattr(self.stream,
"closed", False) before calling stop()/close(), then set self.stream = None or a
_closed flag after closing), and optionally remove the redundant
self.audio.cleanup() from either action_quit or on_unmount so cleanup is only
invoked once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores