fix: prefer bundled dashboard over stale data dist#8172
Conversation
dosubot
Bot
added
size:M
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The logic for selecting between
data/distand the bundled dashboard dist is now duplicated betweencheck_dashboard_filesinmain.pyandAstrBotDashboard.__init__; consider centralizing this selection in a single helper to keep behavior consistent over time. - In
AstrBotDashboard.__init__, whenshould_use_bundled_dashboard_distcauses a fallback to the bundled dist, you only log in theelif bundled_dist.exists()branch; consider adding a log message that explicitly notes when a staledata/distis being ignored to make version-related behavior easier to trace.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for selecting between `data/dist` and the bundled dashboard dist is now duplicated between `check_dashboard_files` in `main.py` and `AstrBotDashboard.__init__`; consider centralizing this selection in a single helper to keep behavior consistent over time.
- In `AstrBotDashboard.__init__`, when `should_use_bundled_dashboard_dist` causes a fallback to the bundled dist, you only log in the `elif bundled_dist.exists()` branch; consider adding a log message that explicitly notes when a stale `data/dist` is being ignored to make version-related behavior easier to trace.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/io.py" line_range="260-267" />
<code_context>
+ return Path(get_astrbot_path()) / "astrbot" / "dashboard" / "dist"
+
+
+def should_use_bundled_dashboard_dist(
+ user_dist: str | Path, current_version: str
+) -> bool:
+ user_version = _read_dashboard_dist_version(user_dist)
+ bundled_dist = get_bundled_dashboard_dist_path()
+ if user_version is None or not bundled_dist.exists():
+ return False
+ return VersionComparator.compare_version(current_version, user_version) > 0
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle malformed or prefixed version strings more defensively in VersionComparator usage.
`user_version` may include a leading `v` (e.g., `v1.2.3`) or otherwise differ from the clean semantic version that `VersionComparator` expects. This can lead to incorrect comparisons or parsing errors, potentially breaking dashboard startup.
Normalize both versions before comparison (e.g., strip a leading `v`) and wrap the compare call in a `try/except` for `ValueError`/`TypeError`, defaulting to `False` (keep the user dist) on failure to make this more robust to malformed version files.
</issue_to_address>
### Comment 2
<location path="tests/test_main.py" line_range="205-214" />
<code_context>
+ mock_download.assert_not_called()
+
+
@pytest.mark.asyncio
async def test_check_dashboard_files_with_webui_dir_arg(monkeypatch):
"""Tests that providing a valid webui_dir skips all checks."""
</code_context>
<issue_to_address>
**issue (testing):** Use an AsyncMock (or async stub) when patching `get_dashboard_version`, since it is awaited in `check_dashboard_files`.
Since `get_dashboard_version()` is awaited in `check_dashboard_files`, the patched object here must be awaitable. A regular `Mock` with `return_value="v0.0.1"` will raise when awaited. Please use an `AsyncMock` (e.g. `mock.AsyncMock(return_value="v0.0.1")`) or an `async def` stub so the test accurately reflects the async behavior and remains stable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @pytest.mark.asyncio | ||
| async def test_check_dashboard_files_uses_bundled_dist_when_data_dist_is_stale( | ||
| tmp_path, | ||
| ): | ||
| """Tests that a stale data/dist does not override bundled dashboard assets.""" | ||
| data_dir = tmp_path / "data" | ||
| data_dist = data_dir / "dist" | ||
| bundled_dist = tmp_path / "bundled-dist" | ||
| data_dist.mkdir(parents=True) | ||
| bundled_dist.mkdir() |
There was a problem hiding this comment.
issue (testing): Use an AsyncMock (or async stub) when patching get_dashboard_version, since it is awaited in check_dashboard_files.
Since get_dashboard_version() is awaited in check_dashboard_files, the patched object here must be awaitable. A regular Mock with return_value="v0.0.1" will raise when awaited. Please use an AsyncMock (e.g. mock.AsyncMock(return_value="v0.0.1")) or an async def stub so the test accurately reflects the async behavior and remains stable.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a version-based fallback mechanism for the dashboard WebUI, ensuring that bundled assets are used if the user-provided assets are outdated. This is achieved through new version comparison utilities and updates to the server and main startup logic. Review feedback suggests improving the robustness of the version check when metadata is missing and streamlining the version retrieval logic to avoid redundant I/O.
| user_version = _read_dashboard_dist_version(user_dist) | ||
| bundled_dist = get_bundled_dashboard_dist_path() | ||
| if user_version is None or not bundled_dist.exists(): | ||
| return False | ||
| return VersionComparator.compare_version(current_version, user_version) > 0 |
There was a problem hiding this comment.
The current implementation returns False if the user dashboard version cannot be read (user_version is None). This results in the application preferring a potentially broken or unversioned dashboard in data/dist over a valid bundled one, which contradicts the goal of avoiding stale or broken UI after an upgrade.
It is safer to prefer the bundled dashboard if the user-installed one is missing its version file, provided the bundled one exists. Additionally, checking for the existence of the bundled distribution before reading the user version avoids unnecessary file I/O when no fallback is available.
| user_version = _read_dashboard_dist_version(user_dist) | |
| bundled_dist = get_bundled_dashboard_dist_path() | |
| if user_version is None or not bundled_dist.exists(): | |
| return False | |
| return VersionComparator.compare_version(current_version, user_version) > 0 | |
| bundled_dist = get_bundled_dashboard_dist_path() | |
| if not bundled_dist.exists(): | |
| return False | |
| user_version = _read_dashboard_dist_version(user_dist) | |
| if user_version is None: | |
| return True | |
| return VersionComparator.compare_version(current_version, user_version) > 0 |
| if should_use_bundled_dashboard_dist(dist_dir, VERSION): | ||
| bundled_version = _read_dashboard_dist_version( | ||
| get_bundled_dashboard_dist_path() | ||
| ) | ||
| if bundled_version is not None: | ||
| return bundled_version | ||
| return _read_dashboard_dist_version(dist_dir) | ||
|
|
||
| bundled = get_bundled_dashboard_dist_path() | ||
| if bundled.exists(): | ||
| return _read_dashboard_dist_version(bundled) | ||
| return None |
There was a problem hiding this comment.
The logic in get_dashboard_version can be simplified and made more efficient. The current implementation makes redundant calls to _read_dashboard_dist_version and get_bundled_dashboard_dist_path. Since should_use_bundled_dashboard_dist already performs the necessary checks, the return paths can be streamlined to avoid re-reading the same files.
| if should_use_bundled_dashboard_dist(dist_dir, VERSION): | |
| bundled_version = _read_dashboard_dist_version( | |
| get_bundled_dashboard_dist_path() | |
| ) | |
| if bundled_version is not None: | |
| return bundled_version | |
| return _read_dashboard_dist_version(dist_dir) | |
| bundled = get_bundled_dashboard_dist_path() | |
| if bundled.exists(): | |
| return _read_dashboard_dist_version(bundled) | |
| return None | |
| if should_use_bundled_dashboard_dist(dist_dir, VERSION): | |
| return _read_dashboard_dist_version(get_bundled_dashboard_dist_path()) | |
| return _read_dashboard_dist_version(dist_dir) | |
| bundled = get_bundled_dashboard_dist_path() | |
| if bundled.exists(): | |
| return _read_dashboard_dist_version(bundled) | |
| return None |
|
Addressed the review feedback in 8f3389c: dashboard dist version comparison now normalizes a leading v, rejects malformed version files conservatively, and the check_dashboard_files tests now patch the awaited get_dashboard_version with AsyncMock. Validation passed: focused tests for the stale/malformed dashboard dist cases, py_compile on astrbot/core/utils/io.py and tests/test_main.py, and ruff check on the touched files. |
2 participants
Fixes #8170.
Summary
data/distdashboard assets by comparing their version with the current core version.data/distis older, instead of serving stale login UI after an upgrade.--webui-dirunchanged.To verify
uv run pytest tests\test_main.py tests\test_dashboard.py::test_dashboard_uses_bundled_dist_when_data_dist_is_stale -q --basetemp .tmp\pytest -p no:cacheprovideruv run ruff check main.py astrbot\core\utils\io.py astrbot\dashboard\server.py tests\test_main.py tests\test_dashboard.pyuv run ruff format --check main.py astrbot\core\utils\io.py astrbot\dashboard\server.py tests\test_main.py tests\test_dashboard.pyuv run python -m py_compile main.py astrbot\core\utils\io.py astrbot\dashboard\server.py tests\test_main.py tests\test_dashboard.pygit diff --checkSummary by Sourcery
Ensure the dashboard uses bundled assets when user data dist is stale relative to the core version.
Bug Fixes:
Enhancements:
Tests: