fix(gradio): reapply audio volume persistence with sane default#697
Merged
ChuxiJ merged 5 commits intoace-step:mainfrom Feb 26, 2026
Merged
fix(gradio): reapply audio volume persistence with sane default#697ChuxiJ merged 5 commits intoace-step:mainfrom
ChuxiJ merged 5 commits intoace-step:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a helper to produce Gradio audio update payloads (rewinding playback) and rewires result handlers to use it; adds persistent, synchronized audio-player volume via a new JS asset and head injection; and includes unit tests for audio rewind and volume preference behavior. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Browser/DOM
participant JS as Audio Preferences JS
participant Storage as localStorage
participant Audio as Audio Elements
participant Server as Python/Gradio
User->>Browser: interact with slider or audio control
Browser->>JS: volumechange / input event
JS->>JS: isTrustedUserEvent() check
alt trusted
JS->>Storage: save preferred volume
JS->>Audio: apply preferred volume to all players
JS->>Browser: sync sliders
else untrusted
JS->>JS: ignore change
end
Browser->>JS: page load or DOM mutation
JS->>Storage: read preferred volume
JS->>Audio: apply stored volume to discovered players
User->>Browser: trigger generation
Browser->>Server: generate request
Server->>Server: prepare results, call build_audio_slot_update(...)
Server->>Browser: update audio slots (value + playback_position=0)
Browser->>Audio: players receive new src and rewind
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/studio.html (1)
442-571:⚠️ Potential issue | 🟠 MajorFirst-run volume defaults to 0 (mute) due to
Number(null).
localStorage.getItem(...)returnsnullwhen missing, andNumber(null)becomes0, so first-run volume is forced to mute. Add an explicit null/undefined/"" guard and seed a sane default before scanning players.✅ Suggested fix
- function clampVolume(value) { - const parsed = Number(value); - if (!Number.isFinite(parsed)) return null; - if (parsed < 0) return 0; - if (parsed > 1) return 1; - return parsed; - } + const DEFAULT_AUDIO_VOLUME = 0.5; + function clampVolume(value) { + if (value === null || value === undefined || value === "") return null; + const parsed = Number(value); + if (!Number.isFinite(parsed)) return null; + if (parsed < 0) return 0; + if (parsed > 1) return 1; + return parsed; + } ... - preferredAudioVolume = loadPreferredAudioVolume(); - scanAndRegisterAudioPlayers(); + preferredAudioVolume = loadPreferredAudioVolume(); + if (preferredAudioVolume === null) { + savePreferredAudioVolume(DEFAULT_AUDIO_VOLUME); + if (preferredAudioVolume === null) { + preferredAudioVolume = DEFAULT_AUDIO_VOLUME; + } + } + scanAndRegisterAudioPlayers();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/studio.html` around lines 442 - 571, The loadPreferredAudioVolume function currently passes null into Number (via localStorage.getItem) which becomes 0 and mutes first-run users; change loadPreferredAudioVolume to first read the raw value, return a sane default (e.g., 0.5) if raw is null/undefined/empty string, otherwise parse and clamp; ensure preferredAudioVolume is set to that returned default before calling scanAndRegisterAudioPlayers so registerAudioElement/syncAllAudioVolumes use the seeded volume (update loadPreferredAudioVolume and the preferredAudioVolume initialization call sites referenced by loadPreferredAudioVolume, preferredAudioVolume, and scanAndRegisterAudioPlayers).acestep/ui/gradio/events/results/generation_info_test.py (1)
59-65:⚠️ Potential issue | 🟠 MajorMake the fallback test deterministic when Gradio is installed.
clear_audio_outputs_for_new_generation()will import Gradio if available, so this test can fail in environments where Gradio is installed. Force the ImportError path to keep the test deterministic.🛠️ Suggested fix
def test_returns_nine_nones(self): """Should return a tuple of 9 None values.""" - result = clear_audio_outputs_for_new_generation() + real_import = __import__ + + def guarded_import(name, *args, **kwargs): + if name == "gradio": + raise ImportError("gradio not available") + return real_import(name, *args, **kwargs) + + with patch("builtins.__import__", side_effect=guarded_import): + result = clear_audio_outputs_for_new_generation() self.assertIsInstance(result, tuple)As per coding guidelines, "Keep tests deterministic, fast, and scoped to changed behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/results/generation_info_test.py` around lines 59 - 65, The test test_returns_nine_nones must force the ImportError branch so it is deterministic when Gradio is installed; wrap the call to clear_audio_outputs_for_new_generation() in a unittest.mock.patch of builtins.__import__ that raises ImportError only when the imported module name is "gradio" (and delegates to the real __import__ otherwise), then call clear_audio_outputs_for_new_generation() and run the same assertions; update the test function test_returns_nine_nones to use this patch so the code path that handles missing Gradio is exercised reliably.
🧹 Nitpick comments (1)
ui/studio_html_test.py (1)
15-25: Add a regression assertion for missing-storage default volume.
Given the first-run guard in the volume persistence logic, add a small test here that asserts the HTML contains the default-volume seeding branch (e.g., null/undefined storage checks + DEFAULT volume assignment). This will protect against first-run mute regressions.As per coding guidelines: Add or update tests for every behavior change and bug fix; include at least one success-path test, one regression/edge-case test for the bug being fixed, and one non-target behavior check when relevant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/studio_html_test.py` around lines 15 - 25, Add a new regression test in ui/studio_html_test.py (e.g., test_volume_defaults_on_missing_storage) that asserts the rendered HTML contains the first-run/default-volume seeding branch: look for references such as localStorage.getItem('volume') being null/undefined checks and assignment/usage of DEFAULT volume (e.g., DEFAULT_VOLUME or a default numeric value) and the code path that applies that default via applyPreferredVolumeToAudio/audioEl (similar to existing test_volumechange_listener_guards_non_trusted_events which checks audioEl.addEventListener('volumechange'... and isTrustedUserEvent). Ensure the new test asserts the presence of the null/undefined storage guard plus the default assignment string so first-run mute regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acestep/ui/gradio/events/results/audio_playback_updates_test.py`:
- Around line 33-55: The tests use a hardcoded /tmp path which triggers Ruff
S108; update the test cases that call build_audio_slot_update (specifically
test_build_audio_slot_update_sets_value_and_rewinds,
test_build_audio_slot_update_clear_path_rewinds, and
test_build_audio_slot_update_without_optional_flags_preserves_defaults) to use a
neutral sample path (e.g., "sample.flac" or a relative fixture path) instead of
"/tmp/sample.flac" so no OS temp directory is hardcoded while keeping assertions
against build_audio_slot_update unchanged.
In `@acestep/ui/gradio/events/results/generation_info_test.py`:
- Around line 19-21: The i18n stub creates i18n_mod.t as a lambda that takes
**kwargs which Ruff flags as unused; update the stub so the parameter is named
with a leading underscore (e.g., **_kwargs) or replace the lambda with a small
function def t(key, **_kwargs): return key so the unused-kwargs lint warning is
silenced; update the assignment i18n_mod.t = ... accordingly.
In `@acestep/ui/gradio/events/results/generation_info.py`:
- Around line 25-40: The broad except in
clear_audio_outputs_for_new_generation() should be narrowed so real import
errors aren't masked; change the except Exception: to except
(ModuleNotFoundError, ImportError): when importing gradio so only missing-import
cases fall back to returning (None,) * 9 and other import failures still surface
(keep the rest of the function intact, including the local import and the
gr.update tuple construction).
In `@acestep/ui/gradio/interfaces/audio_player_preferences.py`:
- Around line 1-392: The module is too large because
get_audio_player_preferences_head embeds a ~300-line JS string; extract that JS
payload into a separate static asset (e.g., audio_player_preferences.js) and
replace the large string in get_audio_player_preferences_head with a small HTML
head snippet that loads the external script (e.g., a <script src="..."> tag or
dynamic loader). Ensure the JS file preserves all symbols/behavior (STORAGE_KEY,
applyPreferredVolume, scanPlayers, start, etc.), update any packaging/template
code to include the new asset at runtime, and adjust tests/imports to reference
the new file instead of the giant string literal.
---
Outside diff comments:
In `@acestep/ui/gradio/events/results/generation_info_test.py`:
- Around line 59-65: The test test_returns_nine_nones must force the ImportError
branch so it is deterministic when Gradio is installed; wrap the call to
clear_audio_outputs_for_new_generation() in a unittest.mock.patch of
builtins.__import__ that raises ImportError only when the imported module name
is "gradio" (and delegates to the real __import__ otherwise), then call
clear_audio_outputs_for_new_generation() and run the same assertions; update the
test function test_returns_nine_nones to use this patch so the code path that
handles missing Gradio is exercised reliably.
In `@ui/studio.html`:
- Around line 442-571: The loadPreferredAudioVolume function currently passes
null into Number (via localStorage.getItem) which becomes 0 and mutes first-run
users; change loadPreferredAudioVolume to first read the raw value, return a
sane default (e.g., 0.5) if raw is null/undefined/empty string, otherwise parse
and clamp; ensure preferredAudioVolume is set to that returned default before
calling scanAndRegisterAudioPlayers so registerAudioElement/syncAllAudioVolumes
use the seeded volume (update loadPreferredAudioVolume and the
preferredAudioVolume initialization call sites referenced by
loadPreferredAudioVolume, preferredAudioVolume, and
scanAndRegisterAudioPlayers).
---
Nitpick comments:
In `@ui/studio_html_test.py`:
- Around line 15-25: Add a new regression test in ui/studio_html_test.py (e.g.,
test_volume_defaults_on_missing_storage) that asserts the rendered HTML contains
the first-run/default-volume seeding branch: look for references such as
localStorage.getItem('volume') being null/undefined checks and assignment/usage
of DEFAULT volume (e.g., DEFAULT_VOLUME or a default numeric value) and the code
path that applies that default via applyPreferredVolumeToAudio/audioEl (similar
to existing test_volumechange_listener_guards_non_trusted_events which checks
audioEl.addEventListener('volumechange'... and isTrustedUserEvent). Ensure the
new test asserts the presence of the null/undefined storage guard plus the
default assignment string so first-run mute regressions are caught.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
acestep/ui/gradio/events/results/audio_playback_updates.pyacestep/ui/gradio/events/results/audio_playback_updates_test.pyacestep/ui/gradio/events/results/batch_navigation.pyacestep/ui/gradio/events/results/generation_info.pyacestep/ui/gradio/events/results/generation_info_test.pyacestep/ui/gradio/events/results/generation_progress.pyacestep/ui/gradio/interfaces/__init__.pyacestep/ui/gradio/interfaces/audio_player_preferences.pyacestep/ui/gradio/interfaces/audio_player_preferences_test.pyacestep/ui/gradio/interfaces/generation_tab_generate_controls.pyui/studio.htmlui/studio_html_test.py
acestep/ui/gradio/events/results/audio_playback_updates_test.py
Outdated
Show resolved
Hide resolved
1larity
force-pushed
the
fix/gradio-volume-persistence
branch
from
1981f03 to
dad9c4d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acestep/ui/gradio/events/results/audio_playback_updates_test.py`:
- Around line 8-27: The test helper should add type hints, a concise docstring
describing inputs/outputs, and an explicit module-load guard to avoid
AttributeError when running in different contexts: annotate _load_module() to
return ModuleType, update its docstring to state it loads the target module by
file path and raises ImportError on failure, add a guard that checks spec and
spec.loader before calling spec.loader.exec_module (raising ImportError with a
clear message if missing), type-hint _MODULE as ModuleType and
build_audio_slot_update with the appropriate Callable signature, and add return
type hints for _FakeGr.update (e.g., Dict[str, Any]); reference symbols:
_load_module, _MODULE, build_audio_slot_update, and class _FakeGr.
In `@acestep/ui/gradio/events/results/generation_info_test.py`:
- Around line 13-31: The helper _load_module currently mutates sys.modules
globally; wrap the sys.modules stubs in a context so they don't leak (use
unittest.mock.patch.dict to temporarily set keys like "acestep", "acestep.ui",
"acestep.ui.gradio", and "acestep.ui.gradio.i18n") and ensure you restore
behavior after the function exits; also add a return type hint (-> ModuleType)
to _load_module and keep the same logic of creating i18n_mod and loading
generation_info.py via importlib.util.spec_from_file_location and
spec.loader.exec_module(module).
In `@acestep/ui/gradio/events/results/generation_info.py`:
- Around line 25-40: Add an explicit return type annotation to
clear_audio_outputs_for_new_generation; declare it to return a tuple of optional
values (e.g. "-> Tuple[Optional[Any], ...]") and import typing names (Tuple,
Optional, Any) at top of module. Keep the implementation as-is (the branch that
returns (None,) * 9 and the branch that builds tuple(gr.update(...)) must
conform to that annotated return type.
In `@acestep/ui/gradio/interfaces/audio_player_preferences.js`:
- Around line 70-89: The Biome lint warns about implicit returns in the arrow
callbacks passed to NodeList.forEach inside forEachAudioPlayer and
forEachVolumeSlider; change the inline arrow callbacks "(player) =>
callback(player)" and "(slider) => callback(slider)" to block-bodied arrows that
call the callback without returning a value (e.g. "(player) => {
callback(player); }") so the callbacks explicitly return void; update both
occurrences in the functions forEachAudioPlayer and forEachVolumeSlider.
In `@acestep/ui/gradio/interfaces/audio_player_preferences.py`:
- Around line 9-26: The package is missing non-Python asset packaging so
_load_preferences_script() will fail to find audio_player_preferences.js at
runtime; update packaging so the .js is included by adding include-package-data
= true in [project] of pyproject.toml and adding a MANIFEST.in that includes the
asset (e.g., include acestep/ui/gradio/interfaces/*.js), or alternatively add
package-data rules that reference the file via _ASSET_FILENAME so
get_audio_player_preferences_head can read it after installation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
acestep/ui/gradio/events/results/audio_playback_updates.pyacestep/ui/gradio/events/results/audio_playback_updates_test.pyacestep/ui/gradio/events/results/batch_navigation.pyacestep/ui/gradio/events/results/generation_info.pyacestep/ui/gradio/events/results/generation_info_test.pyacestep/ui/gradio/events/results/generation_progress.pyacestep/ui/gradio/interfaces/__init__.pyacestep/ui/gradio/interfaces/audio_player_preferences.jsacestep/ui/gradio/interfaces/audio_player_preferences.pyacestep/ui/gradio/interfaces/audio_player_preferences_test.pyacestep/ui/gradio/interfaces/generation_tab_generate_controls.pyui/studio.htmlui/studio_html_test.py
🚧 Files skipped from review as they are similar to previous changes (4)
- acestep/ui/gradio/events/results/generation_progress.py
- acestep/ui/gradio/events/results/audio_playback_updates.py
- ui/studio_html_test.py
- acestep/ui/gradio/interfaces/audio_player_preferences_test.py
1larity
force-pushed
the
fix/gradio-volume-persistence
branch
from
dad9c4d to
a2767eb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
acestep/ui/gradio/events/results/generation_info.py (1)
25-25:⚠️ Potential issue | 🟡 MinorReturn type annotation still missing on
clear_audio_outputs_for_new_generation.This was previously confirmed as addressed but is absent from the current code.
🛠️ Proposed fix
-def clear_audio_outputs_for_new_generation(): +def clear_audio_outputs_for_new_generation() -> tuple[Any, ...]:Also ensure
Anyis imported fromtyping(already present in the file).As per coding guidelines: "Add type hints for new/modified functions when practical in Python".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/results/generation_info.py` at line 25, Add a return type annotation to the function clear_audio_outputs_for_new_generation by updating its signature to include -> None (i.e., def clear_audio_outputs_for_new_generation() -> None:) to satisfy the project typing guidelines; confirm Any is already imported from typing (no additional imports required) and run type checks to ensure no other signatures were missed.acestep/ui/gradio/events/results/audio_playback_updates_test.py (1)
8-27:⚠️ Potential issue | 🟡 MinorReturn type hint on
_load_moduleand_FakeGr.updatestill missing; no spec-load guard.Both were confirmed as addressed in a prior review round but are absent from the current code. The
type: ignore[union-attr]on line 13 also suppresses what should be a handled error path.🛠️ Proposed fix
+from types import ModuleType +from typing import Any + -def _load_module(): +def _load_module() -> ModuleType: """Load the target module directly by file path for isolated testing.""" module_path = Path(__file__).with_name("audio_playback_updates.py") spec = importlib.util.spec_from_file_location("audio_playback_updates", module_path) + if spec is None or spec.loader is None: + raise RuntimeError(f"Failed to create module spec for {module_path}") module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) # type: ignore[union-attr] + spec.loader.exec_module(module) return module class _FakeGr: """Minimal Gradio-like stub exposing ``update``.""" `@staticmethod` - def update(**kwargs): + def update(**kwargs: Any) -> dict[str, Any]: """Return kwargs for direct assertion in tests.""" return kwargsAs per coding guidelines: "Add type hints for new/modified functions when practical in Python" and "Handle errors explicitly in Python; avoid bare
except".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/results/audio_playback_updates_test.py` around lines 8 - 27, Add explicit return type hints and an explicit loader check: annotate _load_module() to return types.ModuleType, and annotate _FakeGr.update(...) to return dict[str, typing.Any]; inside _load_module() check that spec and spec.loader are not None before calling spec.loader.exec_module and raise a clear exception if they're missing (remove the type: ignore[union-attr] and handle the error path explicitly). Use references to _load_module, _MODULE, build_audio_slot_update and _FakeGr.update to locate where to add the annotations and the spec-loader guard.acestep/ui/gradio/events/results/generation_info_test.py (1)
13-31:⚠️ Potential issue | 🟠 Major
sys.modulesstubs still leak globally —patch.dictwrapper not applied.The function directly mutates
sys.modules(lines 17–25) and callsspec.loader.exec_moduleoutside any context manager, so stub entries persist after_load_module()returns and bleed into subsequent test modules. The previously proposedpatch.dictfix was confirmed as addressed in an earlier commit but does not appear in the current code.Additionally
_load_moduleis missing its-> types.ModuleTypereturn type hint, and there is no guard forspec is None / spec.loader is None.🛠️ Proposed fix
-def _load_module(): +def _load_module() -> types.ModuleType: """Load target module directly by file path for isolated testing.""" + stub_modules: dict[str, types.ModuleType] = { + "acestep": types.ModuleType("acestep"), + "acestep.ui": types.ModuleType("acestep.ui"), + "acestep.ui.gradio": types.ModuleType("acestep.ui.gradio"), + "acestep.ui.gradio.i18n": types.ModuleType("acestep.ui.gradio.i18n"), + } + stub_modules["acestep.ui.gradio.i18n"].t = lambda key, **_kwargs: key + stub_modules["acestep"].ui = stub_modules["acestep.ui"] + stub_modules["acestep.ui"].gradio = stub_modules["acestep.ui.gradio"] + stub_modules["acestep.ui.gradio"].i18n = stub_modules["acestep.ui.gradio.i18n"] - acestep_pkg = sys.modules.setdefault("acestep", types.ModuleType("acestep")) - ui_pkg = sys.modules.setdefault("acestep.ui", types.ModuleType("acestep.ui")) - gradio_pkg = sys.modules.setdefault("acestep.ui.gradio", types.ModuleType("acestep.ui.gradio")) - i18n_mod = types.ModuleType("acestep.ui.gradio.i18n") - i18n_mod.t = lambda key, **_kwargs: key - sys.modules["acestep.ui.gradio.i18n"] = i18n_mod - acestep_pkg.ui = ui_pkg - ui_pkg.gradio = gradio_pkg - gradio_pkg.i18n = i18n_mod module_path = Path(__file__).with_name("generation_info.py") spec = importlib.util.spec_from_file_location("generation_info", module_path) + if spec is None or spec.loader is None: + raise RuntimeError(f"Failed to create module spec for {module_path}") module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) # type: ignore[union-attr] + with patch.dict(sys.modules, stub_modules): + spec.loader.exec_module(module) return moduleAs per coding guidelines: "Avoid hidden state and unintended side effects in Python code" and "Add type hints for new/modified functions when practical in Python".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/results/generation_info_test.py` around lines 13 - 31, The helper _load_module currently mutates sys.modules globally and executes spec.loader.exec_module without guards or cleanup; change it to use unittest.mock.patch.dict as a context manager to temporarily inject the stub modules into sys.modules (so entries don't leak), add a return type hint "-> types.ModuleType" on _load_module, and validate the importlib.util.spec_from_file_location result and spec.loader (raise a clear RuntimeError if spec or spec.loader is None) before calling spec.loader.exec_module(module); reference symbols: _load_module, sys.modules, patch.dict, importlib.util.spec_from_file_location, spec.loader.exec_module, and types.ModuleType.acestep/ui/gradio/interfaces/audio_player_preferences.js (1)
76-77:⚠️ Potential issue | 🟡 MinorReintroduced Biome
useIterableCallbackReturnlint violations.The
forEachcallbacks still use implicit returns. This was previously flagged and appears reintroduced here.🛠️ Proposed fix
- root.querySelectorAll("audio").forEach((player) => callback(player)); + root.querySelectorAll("audio").forEach((player) => { + callback(player); + }); root.querySelectorAll("input.volume-slider[type='range'], input#volume[type='range']").forEach( - (slider) => callback(slider) + (slider) => { + callback(slider); + } );#!/bin/bash # Verify implicit-return callbacks in forEach are gone. # Expected: no matches after applying the fix. rg -nP --type=js '\.forEach\(\(\w+\)\s*=>\s*callback\(\w+\)\)' acestep/ui/gradio/interfaces/audio_player_preferences.jsAlso applies to: 86-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/interfaces/audio_player_preferences.js` around lines 76 - 77, Replace implicit-return arrow callbacks passed to forEach with block-bodied arrow functions; e.g., change the call in root.querySelectorAll("audio").forEach((player) => callback(player)) to use a block body like root.querySelectorAll("audio").forEach((player) => { callback(player); }); Do the same for the other occurrences referenced (around the similar forEach at lines ~86-88) so no forEach callbacks use implicit returns.
🧹 Nitpick comments (2)
acestep/ui/gradio/events/results/generation_info.py (1)
40-40: Use iterable unpacking instead of tuple concatenation (Ruff RUF005).🛠️ Proposed fix
- return tuple(gr.update(playback_position=0) for _ in range(8)) + (None,) + return (*[gr.update(playback_position=0) for _ in range(8)], None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/results/generation_info.py` at line 40, Replace the tuple concatenation with iterable unpacking to satisfy Ruff RUF005: in the return statement that currently builds a tuple from (gr.update(playback_position=0) for _ in range(8)) + (None,), change it to use starred unpacking so the generated items and the trailing None are combined via unpacking (i.e., use a single return that unpacks the generator expression and appends None) — update the statement in generation_info.py where gr.update(playback_position=0) is produced.acestep/ui/gradio/interfaces/audio_player_preferences.js (1)
12-13: Consider pruning disconnected roots fromknownRoots.
knownRootsonly grows and keeps strong references to detached roots, which can accumulate across remount-heavy sessions and increase scan cost over time. Pruning stale roots before iteration would make scans more stable long-term.Also applies to: 205-215, 228-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/interfaces/audio_player_preferences.js` around lines 12 - 13, knownRoots currently accumulates detached roots; before any iteration over knownRoots (e.g., the loops around the areas you flagged) prune it by filtering out stale roots (check root.isConnected === false or !document.contains(root) / !root.ownerDocument?.contains(root)) and for each removed root also delete its entry from readyForPersistence (readyForPersistence.delete(root)); update any in-place iterations to use the pruned array (or mutate knownRoots in-place) so scans only touch connected roots and memory/scan cost won't grow unbounded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acestep/ui/gradio/interfaces/audio_player_preferences.py`:
- Around line 9-16: The JS asset audio_player_preferences.js is not guaranteed
to be included in built wheels so _load_preferences_script() can raise
FileNotFoundError in production; fix by either adding a hatch force-include for
the asset under [tool.hatch.build.targets.wheel.force-include] (include the path
acert: ac e s t e p/ui/gradio/interfaces/audio_player_preferences.js) in
pyproject.toml, or modify _load_preferences_script() to catch FileNotFoundError
and raise a clear, actionable error message that explains the missing asset and
instructs to add the hatch force-include entry (reference the function name
_load_preferences_script and the asset filename audio_player_preferences.js when
implementing the error handling).
---
Duplicate comments:
In `@acestep/ui/gradio/events/results/audio_playback_updates_test.py`:
- Around line 8-27: Add explicit return type hints and an explicit loader check:
annotate _load_module() to return types.ModuleType, and annotate
_FakeGr.update(...) to return dict[str, typing.Any]; inside _load_module() check
that spec and spec.loader are not None before calling spec.loader.exec_module
and raise a clear exception if they're missing (remove the type:
ignore[union-attr] and handle the error path explicitly). Use references to
_load_module, _MODULE, build_audio_slot_update and _FakeGr.update to locate
where to add the annotations and the spec-loader guard.
In `@acestep/ui/gradio/events/results/generation_info_test.py`:
- Around line 13-31: The helper _load_module currently mutates sys.modules
globally and executes spec.loader.exec_module without guards or cleanup; change
it to use unittest.mock.patch.dict as a context manager to temporarily inject
the stub modules into sys.modules (so entries don't leak), add a return type
hint "-> types.ModuleType" on _load_module, and validate the
importlib.util.spec_from_file_location result and spec.loader (raise a clear
RuntimeError if spec or spec.loader is None) before calling
spec.loader.exec_module(module); reference symbols: _load_module, sys.modules,
patch.dict, importlib.util.spec_from_file_location, spec.loader.exec_module, and
types.ModuleType.
In `@acestep/ui/gradio/events/results/generation_info.py`:
- Line 25: Add a return type annotation to the function
clear_audio_outputs_for_new_generation by updating its signature to include ->
None (i.e., def clear_audio_outputs_for_new_generation() -> None:) to satisfy
the project typing guidelines; confirm Any is already imported from typing (no
additional imports required) and run type checks to ensure no other signatures
were missed.
In `@acestep/ui/gradio/interfaces/audio_player_preferences.js`:
- Around line 76-77: Replace implicit-return arrow callbacks passed to forEach
with block-bodied arrow functions; e.g., change the call in
root.querySelectorAll("audio").forEach((player) => callback(player)) to use a
block body like root.querySelectorAll("audio").forEach((player) => {
callback(player); }); Do the same for the other occurrences referenced (around
the similar forEach at lines ~86-88) so no forEach callbacks use implicit
returns.
---
Nitpick comments:
In `@acestep/ui/gradio/events/results/generation_info.py`:
- Line 40: Replace the tuple concatenation with iterable unpacking to satisfy
Ruff RUF005: in the return statement that currently builds a tuple from
(gr.update(playback_position=0) for _ in range(8)) + (None,), change it to use
starred unpacking so the generated items and the trailing None are combined via
unpacking (i.e., use a single return that unpacks the generator expression and
appends None) — update the statement in generation_info.py where
gr.update(playback_position=0) is produced.
In `@acestep/ui/gradio/interfaces/audio_player_preferences.js`:
- Around line 12-13: knownRoots currently accumulates detached roots; before any
iteration over knownRoots (e.g., the loops around the areas you flagged) prune
it by filtering out stale roots (check root.isConnected === false or
!document.contains(root) / !root.ownerDocument?.contains(root)) and for each
removed root also delete its entry from readyForPersistence
(readyForPersistence.delete(root)); update any in-place iterations to use the
pruned array (or mutate knownRoots in-place) so scans only touch connected roots
and memory/scan cost won't grow unbounded.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
acestep/ui/gradio/events/results/audio_playback_updates.pyacestep/ui/gradio/events/results/audio_playback_updates_test.pyacestep/ui/gradio/events/results/batch_navigation.pyacestep/ui/gradio/events/results/generation_info.pyacestep/ui/gradio/events/results/generation_info_test.pyacestep/ui/gradio/events/results/generation_progress.pyacestep/ui/gradio/interfaces/__init__.pyacestep/ui/gradio/interfaces/audio_player_preferences.jsacestep/ui/gradio/interfaces/audio_player_preferences.pyacestep/ui/gradio/interfaces/audio_player_preferences_test.pyacestep/ui/gradio/interfaces/generation_tab_generate_controls.pyui/studio.htmlui/studio_html_test.py
🚧 Files skipped from review as they are similar to previous changes (4)
- acestep/ui/gradio/events/results/generation_progress.py
- acestep/ui/gradio/interfaces/generation_tab_generate_controls.py
- acestep/ui/gradio/interfaces/init.py
- acestep/ui/gradio/interfaces/audio_player_preferences_test.py
1larity
force-pushed
the
fix/gradio-volume-persistence
branch
from
a2767eb to
4249748
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
acestep/ui/gradio/events/results/generation_info.py (1)
25-40:⚠️ Potential issue | 🟡 MinorRe-add an explicit return type on
clear_audio_outputs_for_new_generation.This modified function lost its return annotation; please restore it to keep typing/tooling consistent and avoid regressions from the prior fix.
🔧 Proposed fix
-def clear_audio_outputs_for_new_generation(): +def clear_audio_outputs_for_new_generation() -> tuple[Any, ...]:#!/bin/bash python - <<'PY' import ast from pathlib import Path p = Path("acestep/ui/gradio/events/results/generation_info.py") tree = ast.parse(p.read_text(encoding="utf-8")) fn = next( n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == "clear_audio_outputs_for_new_generation" ) print("return_annotation_present:", fn.returns is not None) PYAs per coding guidelines: "Add type hints for new/modified functions when practical in Python".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/results/generation_info.py` around lines 25 - 40, The function clear_audio_outputs_for_new_generation lost its return type annotation; restore a precise typing return (e.g. add "-> Tuple[Optional[object], ...]" or "-> Tuple[Any, ...]" and import the necessary names from typing) so tooling knows the function returns a 9-tuple of updates/None; update the top of the file to import Tuple, Optional, Any as needed and add the annotation to the clear_audio_outputs_for_new_generation definition.acestep/ui/gradio/events/results/generation_info_test.py (1)
13-31:⚠️ Potential issue | 🟡 MinorScope
sys.modulesstubs to avoid cross-test state leakage.
_load_module()mutatessys.modulesglobally; this can bleed into other tests. Wrap stub injection inpatch.dictand keep it local to module execution.🔧 Proposed fix
-def _load_module(): +def _load_module() -> types.ModuleType: """Load target module directly by file path for isolated testing.""" - # Stub i18n package path so importing generation_info.py does not require - # importing the full Gradio UI package in headless test environments. - acestep_pkg = sys.modules.setdefault("acestep", types.ModuleType("acestep")) - ui_pkg = sys.modules.setdefault("acestep.ui", types.ModuleType("acestep.ui")) - gradio_pkg = sys.modules.setdefault("acestep.ui.gradio", types.ModuleType("acestep.ui.gradio")) - i18n_mod = types.ModuleType("acestep.ui.gradio.i18n") - i18n_mod.t = lambda key, **_kwargs: key - sys.modules["acestep.ui.gradio.i18n"] = i18n_mod - acestep_pkg.ui = ui_pkg - ui_pkg.gradio = gradio_pkg - gradio_pkg.i18n = i18n_mod + stub_modules = { + "acestep": types.ModuleType("acestep"), + "acestep.ui": types.ModuleType("acestep.ui"), + "acestep.ui.gradio": types.ModuleType("acestep.ui.gradio"), + "acestep.ui.gradio.i18n": types.ModuleType("acestep.ui.gradio.i18n"), + } + stub_modules["acestep.ui.gradio.i18n"].t = lambda key, **_kwargs: key + stub_modules["acestep"].ui = stub_modules["acestep.ui"] + stub_modules["acestep.ui"].gradio = stub_modules["acestep.ui.gradio"] + stub_modules["acestep.ui.gradio"].i18n = stub_modules["acestep.ui.gradio.i18n"] module_path = Path(__file__).with_name("generation_info.py") spec = importlib.util.spec_from_file_location("generation_info", module_path) module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) # type: ignore[union-attr] + with patch.dict(sys.modules, stub_modules): + spec.loader.exec_module(module) # type: ignore[union-attr] return module#!/bin/bash python - <<'PY' from pathlib import Path import re src = Path("acestep/ui/gradio/events/results/generation_info_test.py").read_text(encoding="utf-8") m = re.search(r"def _load_module\(\):(.+?)return module", src, flags=re.S) body = m.group(1) if m else "" print("uses_sys_modules_setdefault:", "sys.modules.setdefault" in body) print("uses_patch_dict_scope:", "with patch.dict(sys.modules" in body or 'with patch.dict("sys.modules"' in body) PYAs per coding guidelines: "Avoid hidden state and unintended side effects in Python code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/events/results/generation_info_test.py` around lines 13 - 31, The test helper _load_module currently mutates sys.modules via sys.modules.setdefault and assigns attributes on the stub packages (acestep, acestep.ui, acestep.ui.gradio, acestep.ui.gradio.i18n), which can leak state across tests; change _load_module to create the same stub entries but inside a with patch.dict(sys.modules, {...}, clear=False): context so the temporary modules (keys like "acestep", "acestep.ui", "acestep.ui.gradio", "acestep.ui.gradio.i18n") are injected only while you call spec.loader.exec_module(module) and are automatically reverted afterwards—keep the existing i18n_mod.t lambda and the assignments to module attributes (acestep_pkg.ui, ui_pkg.gradio, gradio_pkg.i18n) but move them into the patch.dict block and remove direct sys.modules.setdefault calls outside the context to avoid cross-test leakage in _load_module.acestep/ui/gradio/interfaces/audio_player_preferences.js (1)
76-77:⚠️ Potential issue | 🟡 MinorReapply block-bodied
forEachcallbacks to satisfy Biome.These callbacks implicitly return a value and currently trigger
lint/suspicious/useIterableCallbackReturn.🛠️ Proposed fix
- root.querySelectorAll("audio").forEach((player) => callback(player)); + root.querySelectorAll("audio").forEach((player) => { + callback(player); + }); ... - root.querySelectorAll("input.volume-slider[type='range'], input#volume[type='range']").forEach( - (slider) => callback(slider) - ); + root.querySelectorAll("input.volume-slider[type='range'], input#volume[type='range']").forEach( + (slider) => { + callback(slider); + } + );#!/bin/bash # Expect: no matches after applying the fix. rg -nP 'forEach\(\((player|slider)\)\s*=>\s*callback\(' acestep/ui/gradio/interfaces/audio_player_preferences.jsAlso applies to: 86-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/interfaces/audio_player_preferences.js` around lines 76 - 77, The Biome lint flags the concise arrow callbacks used with forEach because they implicitly return a value; update the callbacks to use a block body so they return undefined explicitly. Replace the single-expression arrow callbacks like root.querySelectorAll("audio").forEach((player) => callback(player)); with block-bodied equivalents that call callback inside a brace-delimited function (e.g., (player) => { callback(player); }) and do the same for the other affected forEach usages (the similar slider/element loops around lines 86-88) ensuring you update the callbacks that invoke callback(...) to use { callback(...); } form.
🧹 Nitpick comments (1)
acestep/ui/gradio/interfaces/audio_player_preferences.js (1)
12-13: Prune detached roots to avoid long-session root retention.
knownRootsholds strong references indefinitely; after repeated remounts, stale roots can accumulate and increase scan/sync cost.♻️ Proposed refactor
const knownRoots = []; +const isRootConnected = (root) => { + if (!root) { + return false; + } + if (root === document) { + return true; + } + if (root.host && typeof root.host.isConnected === "boolean") { + return root.host.isConnected; + } + return root.isConnected === true; +}; + +const pruneKnownRoots = () => { + for (let i = knownRoots.length - 1; i >= 0; i -= 1) { + const root = knownRoots[i]; + if (!isRootConnected(root)) { + knownRoots.splice(i, 1); + } + } +}; ... const stopAndRewindAllPlayers = () => { discoverRoots(); + pruneKnownRoots(); forEachAudioPlayer((player) => { if (!player) { return; } ... const scanPlayers = () => { discoverRoots(); + pruneKnownRoots(); forEachAudioPlayer(registerPlayer); forEachVolumeSlider(registerVolumeSlider); syncAllVolumeControlsToPreferred(); };Also applies to: 200-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/ui/gradio/interfaces/audio_player_preferences.js` around lines 12 - 13, knownRoots currently holds strong references and should be changed to avoid retaining detached roots; replace the array-based knownRoots with a WeakSet (e.g., knownRoots = new WeakSet()) and update all places that add/remove/iterate roots to use weak membership semantics (use knownRoots.add(root) / knownRoots.delete(root)) and avoid relying on iteration over the WeakSet—when you need to enumerate live roots, derive the list by filtering the existing iterable you already have (or filter any array by checking root.isConnected) or use readyForPersistence (WeakMap) keys to determine liveness; also apply the same replacement/pruning logic for the other occurrence around the 200–206 region so stale roots are not retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acestep/ui/gradio/interfaces/audio_player_preferences_test.py`:
- Around line 8-14: Update _load_module to have an explicit return type
(typing.ModuleType) and guard the spec/loader resolution instead of using the
type: ignore; specifically, import types.ModuleType (or typing.ModuleType),
change the signature to def _load_module() -> ModuleType, check that spec
returned by importlib.util.spec_from_file_location is not None and that
spec.loader is not None, and if either is None raise a clear RuntimeError
(including module_path in the message); then call
spec.loader.exec_module(module) without a type: ignore and return the loaded
module.
---
Duplicate comments:
In `@acestep/ui/gradio/events/results/generation_info_test.py`:
- Around line 13-31: The test helper _load_module currently mutates sys.modules
via sys.modules.setdefault and assigns attributes on the stub packages (acestep,
acestep.ui, acestep.ui.gradio, acestep.ui.gradio.i18n), which can leak state
across tests; change _load_module to create the same stub entries but inside a
with patch.dict(sys.modules, {...}, clear=False): context so the temporary
modules (keys like "acestep", "acestep.ui", "acestep.ui.gradio",
"acestep.ui.gradio.i18n") are injected only while you call
spec.loader.exec_module(module) and are automatically reverted afterwards—keep
the existing i18n_mod.t lambda and the assignments to module attributes
(acestep_pkg.ui, ui_pkg.gradio, gradio_pkg.i18n) but move them into the
patch.dict block and remove direct sys.modules.setdefault calls outside the
context to avoid cross-test leakage in _load_module.
In `@acestep/ui/gradio/events/results/generation_info.py`:
- Around line 25-40: The function clear_audio_outputs_for_new_generation lost
its return type annotation; restore a precise typing return (e.g. add "->
Tuple[Optional[object], ...]" or "-> Tuple[Any, ...]" and import the necessary
names from typing) so tooling knows the function returns a 9-tuple of
updates/None; update the top of the file to import Tuple, Optional, Any as
needed and add the annotation to the clear_audio_outputs_for_new_generation
definition.
In `@acestep/ui/gradio/interfaces/audio_player_preferences.js`:
- Around line 76-77: The Biome lint flags the concise arrow callbacks used with
forEach because they implicitly return a value; update the callbacks to use a
block body so they return undefined explicitly. Replace the single-expression
arrow callbacks like root.querySelectorAll("audio").forEach((player) =>
callback(player)); with block-bodied equivalents that call callback inside a
brace-delimited function (e.g., (player) => { callback(player); }) and do the
same for the other affected forEach usages (the similar slider/element loops
around lines 86-88) ensuring you update the callbacks that invoke callback(...)
to use { callback(...); } form.
---
Nitpick comments:
In `@acestep/ui/gradio/interfaces/audio_player_preferences.js`:
- Around line 12-13: knownRoots currently holds strong references and should be
changed to avoid retaining detached roots; replace the array-based knownRoots
with a WeakSet (e.g., knownRoots = new WeakSet()) and update all places that
add/remove/iterate roots to use weak membership semantics (use
knownRoots.add(root) / knownRoots.delete(root)) and avoid relying on iteration
over the WeakSet—when you need to enumerate live roots, derive the list by
filtering the existing iterable you already have (or filter any array by
checking root.isConnected) or use readyForPersistence (WeakMap) keys to
determine liveness; also apply the same replacement/pruning logic for the other
occurrence around the 200–206 region so stale roots are not retained.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
acestep/ui/gradio/events/results/audio_playback_updates.pyacestep/ui/gradio/events/results/audio_playback_updates_test.pyacestep/ui/gradio/events/results/batch_navigation.pyacestep/ui/gradio/events/results/generation_info.pyacestep/ui/gradio/events/results/generation_info_test.pyacestep/ui/gradio/events/results/generation_progress.pyacestep/ui/gradio/interfaces/__init__.pyacestep/ui/gradio/interfaces/audio_player_preferences.jsacestep/ui/gradio/interfaces/audio_player_preferences.pyacestep/ui/gradio/interfaces/audio_player_preferences_test.pyacestep/ui/gradio/interfaces/generation_tab_generate_controls.pypyproject.tomlui/studio.htmlui/studio_html_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- acestep/ui/gradio/events/results/audio_playback_updates_test.py
- acestep/ui/gradio/interfaces/audio_player_preferences.py
- acestep/ui/gradio/interfaces/init.py
ChuxiJ
approved these changes
Feb 26, 2026
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
2 participants
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.
Follow-up to merged PR #680.
This reapplies and hardens Gradio audio volume behavior with first-run safety defaults.
What this fixes
0.5) when persisted storage is missing/invalid, and saves it on startup.CodeRabbit findings: pre-existing vs introduced
Pre-existing before the volume-fix line of work
generation_info.pyclear_audio_outputs_for_new_generation.Introduced by the volume-fix/reapply line (or its follow-up refactor)
ui/studio.htmlNumber(null) -> 0) in the earlier volume-fix commit line (34f0f1c); corrected here with default seeding.audio_playback_updates_test.pygeneration_info_test.pysys.modulesleakage risk from local module-loading stubs (added in follow-up test refactor).audio_player_preferences.jsforEachcallbacks (file introduced/extracted in this PR sequence).pyproject.tomlPre-existing changes in this PR branch
(Already part of this PR before latest CodeRabbit follow-up)
playback_position=0updates.gr.skip()for in-flight clear path).elem_idwiring for deterministic stop/rewind hook.audio_player_preferences.jswith a thin Python loader.CodeRabbit follow-up fixes (latest)
audio_playback_updates_test.pyspec/spec.loader) with clearImportError._FakeGr.update.generation_info_test.py_load_module()now usespatch.dict(sys.modules, ...)so stubs do not leak globally.ModuleTypereturn annotation.generation_info.pyclear_audio_outputs_for_new_generation.audio_player_preferences.jsforEachcallbacks to block-bodied arrows to avoid implicit-return lint warnings.pyproject.toml)acestep/ui/gradio/interfaces/audio_player_preferences.jsin wheel and sdist targets.Packaging risk check (.toml change)
This is the only project-wide config change in the follow-up and was re-validated:
dist/ace_step-1.5.0-py3-none-any.whldist/ace_step-1.5.0.tar.gzacestep/ui/gradio/interfaces/audio_player_preferences.jsValidation checks passed
Executed in UV-managed venv context:
Ran 19 tests ... OKuv run python -m unittest discover -s acestep/ui/gradio/events/results -p *_test.pyRan 36 tests ... OKRan 16 tests ... OKScope
Summary by CodeRabbit