Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's guide (collapsed on small PRs)Reviewer's Guide此 PR 更新了主题背景图的处理方式：从使用本地静态文件改为通过外部随机背景服务获取，同时调整了测试以验证新的行为，并继续确保不会使用任何外部字体 CDN。 从外部服务加载背景图的时序图sequenceDiagram
actor User
participant Browser
participant CSS as advanced_material_alas_css
participant ExternalBG as api_yppp_net
User ->> Browser: Open WebUI
Browser ->> CSS: Load advanced-material-alas.css
CSS -->> Browser: --alas-apple-bg-image = https://api.yppp.net/api.php
Browser ->> ExternalBG: GET /api.php
ExternalBG -->> Browser: background image response
Browser -->> User: Render page with external background image
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的控制面板 来：
Getting HelpOriginal review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates the theme background image handling to use an external random background service instead of a local static file, and aligns the tests to validate the new behavior while still ensuring no external font CDNs are used. Sequence diagram for loading background image from external servicesequenceDiagram
actor User
participant Browser
participant CSS as advanced_material_alas_css
participant ExternalBG as api_yppp_net
User ->> Browser: Open WebUI
Browser ->> CSS: Load advanced-material-alas.css
CSS -->> Browser: --alas-apple-bg-image = https://api.yppp.net/api.php
Browser ->> ExternalBG: GET /api.php
ExternalBG -->> Browser: background image response
Browser -->> User: Render page with external background image
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些整体层面的反馈：
- 建议将外部随机背景图片的 URL 设计成可配置的（例如通过设置项，或由服务端生成的 CSS 变量），而不是直接硬编码为
https://api.yppp.net/api.php，这样不同部署环境可以更方便地控制或替换这个外部依赖。 - 更新后的测试依赖背景图片 URL 的精确字符串匹配；你可能会更倾向于用更宽松的断言方式（例如使用
contains('api.yppp.net/api.php')），以避免在 CSS 发生轻微格式变化时（如空格、引号等）导致脆弱的测试失败。
用于 AI Agents 的提示
请根据这次代码评审中的评论进行修改：
## 整体评论
- 建议将外部随机背景图片的 URL 设计成可配置的（例如通过设置项，或由服务端生成的 CSS 变量），而不是直接硬编码为 `https://api.yppp.net/api.php`，这样不同部署环境可以更方便地控制或替换这个外部依赖。
- 更新后的测试依赖背景图片 URL 的精确字符串匹配；你可能会更倾向于用更宽松的断言方式（例如使用 `contains('api.yppp.net/api.php')`），以避免在 CSS 发生轻微格式变化时（如空格、引号等）导致脆弱的测试失败。
## 单独评论
### 评论 1
<location path="tests/test_webui_static_assets.py" line_range="76" />
<code_context>
self.assertNotIn("cdn.jsdelivr.net", response.text)
- def test_theme_assets_do_not_depend_on_external_font_services(self):
+ def test_theme_keeps_random_background_without_external_font_services(self):
theme_css = (PROJECT_ROOT / "assets/gui/css/advanced-material-alas.css").read_text(
encoding="utf-8"
</code_context>
<issue_to_address>
**suggestion (testing):** 建议增加一个检查，确保除了允许的随机背景 API 外，没有引入其他外部资源主机。
目前，这个测试验证了随机背景使用 `https://api.yppp.net/api.php`，并且避免了外部字体服务。为了更好地落实「自托管资源 + 单一白名单 API」的策略，可以考虑增加一个断言，检查 `theme_css`/`obs_overlay` 中除了 `api.yppp.net` 之外不包含其他 `http`/`https` URL（例如，通过提取 `url(...)` 的值，或扫描 `http` 字样，并只允许白名单主机或相对 URL）。
建议实现：
```python
def test_theme_keeps_random_background_without_external_font_services(self):
theme_css = (PROJECT_ROOT / "assets/gui/css/advanced-material-alas.css").read_text(
encoding="utf-8"
)
# 确保主题 CSS 不依赖外部字体服务
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
# 随机背景必须使用白名单中的 API
self.assertIn('url("https://api.yppp.net/api.php")', theme_css)
# 确保 OBS overlay 不依赖外部字体服务
self.assertNotIn("fonts.googleapis.com", obs_overlay)
self.assertNotIn("fonts.gstatic.com", obs_overlay)
# 强制执行「自托管资源 + 单一白名单 API」策略：
# 任何绝对 http/https URL 必须指向 api.yppp.net；相对 URL 是允许的。
def assert_only_whitelisted_external_host(css_text: str) -> None:
# 查找 CSS 中所有的 http/https URL（例如在 url(...) 内或其他位置）
for match in re.finditer(r"https?://([^/]+)/[^\s\"')]*", css_text):
host = match.group(1)
self.assertEqual(
"api.yppp.net",
host,
msg=f"Unexpected external asset host {host!r} in CSS: {match.group(0)!r}",
)
assert_only_whitelisted_external_host(theme_css)
assert_only_whitelisted_external_host(obs_overlay)
```
为了让这个测试生效，请确保在 `tests/test_webui_static_assets.py` 顶部已导入 `re`：
1. 在现有的标准库导入（例如已有的 `import pathlib`、`import os` 等）附近添加 `import re`。
2. 确认 `obs_overlay` 已在测试模块中提前定义（看起来已经在使用；如果它已经被设置为 overlay 的 CSS 内容，则无需更改）。
</issue_to_address>
### 评论 2
<location path="tests/test_webui_static_assets.py" line_range="86" />
<code_context>
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
- self.assertNotIn("api.yppp.net", theme_css)
+ self.assertIn('url("https://api.yppp.net/api.php")', theme_css)
self.assertNotIn("fonts.googleapis.com", obs_overlay)
self.assertNotIn("fonts.gstatic.com", obs_overlay)
</code_context>
<issue_to_address>
**suggestion (testing):** 对背景 API URL 的断言过于严格；可以考虑稍微放宽一些，同时仍然保证预期行为。
当前断言对 CSS 的具体序列化和路径进行了硬编码，因此小幅改动（例如增加查询参数、改变引号/空白）即使不影响行为，也会导致测试失败。可以考虑使用更灵活的匹配方式（例如检查 `theme_css` 是否在 `url(` 的上下文中包含 `api.yppp.net`，或通过正则表达式确保存在 `https://api.yppp.net/` URL），从而在不依赖格式细节的前提下验证主机和协议约束。
建议实现：
```python
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
# 确保随机背景使用指向 api.yppp.net 的 HTTPS URL，而不依赖精确的 CSS 序列化格式
self.assertRegex(
theme_css,
r'url\(\s*[\'"]https://api\.yppp\.net/[^\'"]*[\'"]\s*\)'
)
self.assertNotIn("fonts.googleapis.com", obs_overlay)
```
如果这个测试类目前没有继承自提供 `assertRegex` 方法的 `unittest.TestCase` 或其子类，你可能需要确保它这样做，或者导入 `unittest` 并作相应调整。该正则有意对 `https://api.yppp.net/` 后面的路径和查询字符串保持宽松；如果你希望约束得更严格（例如只允许 `/api.php`），可以将 `[^\'"]*` 改成 `api\.php[^\'"]*`。
</issue_to_address>帮我变得更有用！请对每条评论点选 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- Consider making the external random background image URL configurable (e.g., via settings or a CSS variable generated server-side) instead of hardcoding
https://api.yppp.net/api.php, so deployments can control or replace the external dependency more easily. - The updated test relies on an exact string match for the background image URL; you might prefer a looser assertion (e.g.,
contains('api.yppp.net/api.php')) to avoid brittle failures if minor formatting changes are made to the CSS.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making the external random background image URL configurable (e.g., via settings or a CSS variable generated server-side) instead of hardcoding `https://api.yppp.net/api.php`, so deployments can control or replace the external dependency more easily.
- The updated test relies on an exact string match for the background image URL; you might prefer a looser assertion (e.g., `contains('api.yppp.net/api.php')`) to avoid brittle failures if minor formatting changes are made to the CSS.
## Individual Comments
### Comment 1
<location path="tests/test_webui_static_assets.py" line_range="76" />
<code_context>
self.assertNotIn("cdn.jsdelivr.net", response.text)
- def test_theme_assets_do_not_depend_on_external_font_services(self):
+ def test_theme_keeps_random_background_without_external_font_services(self):
theme_css = (PROJECT_ROOT / "assets/gui/css/advanced-material-alas.css").read_text(
encoding="utf-8"
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a check that no other external asset hosts are introduced besides the allowed random background API.
Currently, the test confirms the random background uses `https://api.yppp.net/api.php` and that external font services are avoided. To better enforce the “self-hosted assets + one whitelisted API” policy, consider adding an assertion that `theme_css`/`obs_overlay` contain no other `http`/`https` URLs besides `api.yppp.net` (e.g., by extracting `url(...)` values or scanning for `http` and allowing only the whitelisted host or relative URLs).
Suggested implementation:
```python
def test_theme_keeps_random_background_without_external_font_services(self):
theme_css = (PROJECT_ROOT / "assets/gui/css/advanced-material-alas.css").read_text(
encoding="utf-8"
)
# Ensure we don't depend on external font services in the theme CSS
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
# Random background must use the whitelisted API
self.assertIn('url("https://api.yppp.net/api.php")', theme_css)
# Ensure we don't depend on external font services in the OBS overlay
self.assertNotIn("fonts.googleapis.com", obs_overlay)
self.assertNotIn("fonts.gstatic.com", obs_overlay)
# Enforce "self-hosted assets + one whitelisted API" policy:
# any absolute http/https URL must point to api.yppp.net; relative URLs are allowed.
def assert_only_whitelisted_external_host(css_text: str) -> None:
# Find all http/https URLs in the CSS (e.g. inside url(...) or elsewhere)
for match in re.finditer(r"https?://([^/]+)/[^\s\"')]*", css_text):
host = match.group(1)
self.assertEqual(
"api.yppp.net",
host,
msg=f"Unexpected external asset host {host!r} in CSS: {match.group(0)!r}",
)
assert_only_whitelisted_external_host(theme_css)
assert_only_whitelisted_external_host(obs_overlay)
```
To make this test work, ensure `re` is imported at the top of `tests/test_webui_static_assets.py`:
1. Add `import re` alongside the other standard-library imports (e.g., near existing `import pathlib`, `import os`, etc.).
2. Confirm that `obs_overlay` is defined earlier in the test module (it appears to be used already; no changes needed if it's already set to the overlay CSS text).
</issue_to_address>
### Comment 2
<location path="tests/test_webui_static_assets.py" line_range="86" />
<code_context>
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
- self.assertNotIn("api.yppp.net", theme_css)
+ self.assertIn('url("https://api.yppp.net/api.php")', theme_css)
self.assertNotIn("fonts.googleapis.com", obs_overlay)
self.assertNotIn("fonts.gstatic.com", obs_overlay)
</code_context>
<issue_to_address>
**suggestion (testing):** The assertion on the exact background API URL may be brittle; consider loosening it slightly while still enforcing the intended behavior.
This assertion hard-codes the exact CSS serialization and path, so minor changes (e.g. added query params, different quoting/whitespace) will break the test without changing behavior. Consider asserting on a more flexible pattern (e.g. checking that `theme_css` contains `api.yppp.net` with a `url(` guard, or using a regex to ensure an `https://api.yppp.net/` URL) so the test validates the host/protocol constraint without depending on formatting details.
Suggested implementation:
```python
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
# Ensure the random background uses an HTTPS URL to api.yppp.net without depending on exact CSS serialization
self.assertRegex(
theme_css,
r'url\(\s*[\'"]https://api\.yppp\.net/[^\'"]*[\'"]\s*\)'
)
self.assertNotIn("fonts.googleapis.com", obs_overlay)
```
If this test class does not already inherit from `unittest.TestCase` or a subclass that provides `assertRegex`, you may need to ensure it does so, or import `unittest` and adjust accordingly. The regex is intentionally permissive about the path and query string after `https://api.yppp.net/`; if you want to constrain it further (e.g. specifically `/api.php`), you can change `[^\'"]*` to `api\.php[^\'"]*`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self.assertNotIn("cdn.jsdelivr.net", response.text) | ||
|
|
||
| def test_theme_assets_do_not_depend_on_external_font_services(self): | ||
| def test_theme_keeps_random_background_without_external_font_services(self): |
There was a problem hiding this comment.
suggestion (testing): 建议增加一个检查，确保除了允许的随机背景 API 外，没有引入其他外部资源主机。
目前，这个测试验证了随机背景使用 https://api.yppp.net/api.php，并且避免了外部字体服务。为了更好地落实「自托管资源 + 单一白名单 API」的策略，可以考虑增加一个断言，检查 theme_css/obs_overlay 中除了 api.yppp.net 之外不包含其他 http/https URL（例如，通过提取 url(...) 的值，或扫描 http 字样，并只允许白名单主机或相对 URL）。
建议实现：
def test_theme_keeps_random_background_without_external_font_services(self):
theme_css = (PROJECT_ROOT / "assets/gui/css/advanced-material-alas.css").read_text(
encoding="utf-8"
)
# 确保主题 CSS 不依赖外部字体服务
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
# 随机背景必须使用白名单中的 API
self.assertIn('url("https://api.yppp.net/api.php")', theme_css)
# 确保 OBS overlay 不依赖外部字体服务
self.assertNotIn("fonts.googleapis.com", obs_overlay)
self.assertNotIn("fonts.gstatic.com", obs_overlay)
# 强制执行「自托管资源 + 单一白名单 API」策略：
# 任何绝对 http/https URL 必须指向 api.yppp.net；相对 URL 是允许的。
def assert_only_whitelisted_external_host(css_text: str) -> None:
# 查找 CSS 中所有的 http/https URL（例如在 url(...) 内或其他位置）
for match in re.finditer(r"https?://([^/]+)/[^\s\"')]*", css_text):
host = match.group(1)
self.assertEqual(
"api.yppp.net",
host,
msg=f"Unexpected external asset host {host!r} in CSS: {match.group(0)!r}",
)
assert_only_whitelisted_external_host(theme_css)
assert_only_whitelisted_external_host(obs_overlay)为了让这个测试生效，请确保在 tests/test_webui_static_assets.py 顶部已导入 re：
- 在现有的标准库导入（例如已有的
import pathlib、import os等）附近添加import re。 - 确认
obs_overlay已在测试模块中提前定义（看起来已经在使用；如果它已经被设置为 overlay 的 CSS 内容，则无需更改）。
Original comment in English
suggestion (testing): Consider adding a check that no other external asset hosts are introduced besides the allowed random background API.
Currently, the test confirms the random background uses https://api.yppp.net/api.php and that external font services are avoided. To better enforce the “self-hosted assets + one whitelisted API” policy, consider adding an assertion that theme_css/obs_overlay contain no other http/https URLs besides api.yppp.net (e.g., by extracting url(...) values or scanning for http and allowing only the whitelisted host or relative URLs).
Suggested implementation:
def test_theme_keeps_random_background_without_external_font_services(self):
theme_css = (PROJECT_ROOT / "assets/gui/css/advanced-material-alas.css").read_text(
encoding="utf-8"
)
# Ensure we don't depend on external font services in the theme CSS
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
# Random background must use the whitelisted API
self.assertIn('url("https://api.yppp.net/api.php")', theme_css)
# Ensure we don't depend on external font services in the OBS overlay
self.assertNotIn("fonts.googleapis.com", obs_overlay)
self.assertNotIn("fonts.gstatic.com", obs_overlay)
# Enforce "self-hosted assets + one whitelisted API" policy:
# any absolute http/https URL must point to api.yppp.net; relative URLs are allowed.
def assert_only_whitelisted_external_host(css_text: str) -> None:
# Find all http/https URLs in the CSS (e.g. inside url(...) or elsewhere)
for match in re.finditer(r"https?://([^/]+)/[^\s\"')]*", css_text):
host = match.group(1)
self.assertEqual(
"api.yppp.net",
host,
msg=f"Unexpected external asset host {host!r} in CSS: {match.group(0)!r}",
)
assert_only_whitelisted_external_host(theme_css)
assert_only_whitelisted_external_host(obs_overlay)To make this test work, ensure re is imported at the top of tests/test_webui_static_assets.py:
- Add
import realongside the other standard-library imports (e.g., near existingimport pathlib,import os, etc.). - Confirm that
obs_overlayis defined earlier in the test module (it appears to be used already; no changes needed if it's already set to the overlay CSS text).
| self.assertNotIn("fonts.googleapis.com", theme_css) | ||
| self.assertNotIn("fonts.gstatic.com", theme_css) | ||
| self.assertNotIn("api.yppp.net", theme_css) | ||
| self.assertIn('url("https://api.yppp.net/api.php")', theme_css) |
There was a problem hiding this comment.
suggestion (testing): 对背景 API URL 的断言过于严格；可以考虑稍微放宽一些，同时仍然保证预期行为。
当前断言对 CSS 的具体序列化和路径进行了硬编码，因此小幅改动（例如增加查询参数、改变引号/空白）即使不影响行为，也会导致测试失败。可以考虑使用更灵活的匹配方式（例如检查 theme_css 是否在 url( 的上下文中包含 api.yppp.net，或通过正则表达式确保存在 https://api.yppp.net/ URL），从而在不依赖格式细节的前提下验证主机和协议约束。
建议实现：
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
# 确保随机背景使用指向 api.yppp.net 的 HTTPS URL，而不依赖精确的 CSS 序列化格式
self.assertRegex(
theme_css,
r'url\(\s*[\'"]https://api\.yppp\.net/[^\'"]*[\'"]\s*\)'
)
self.assertNotIn("fonts.googleapis.com", obs_overlay)如果这个测试类目前没有继承自提供 assertRegex 方法的 unittest.TestCase 或其子类，你可能需要确保它这样做，或者导入 unittest 并作相应调整。该正则有意对 https://api.yppp.net/ 后面的路径和查询字符串保持宽松；如果你希望约束得更严格（例如只允许 /api.php），可以将 [^\'"]* 改成 api\.php[^\'"]*。
Original comment in English
suggestion (testing): The assertion on the exact background API URL may be brittle; consider loosening it slightly while still enforcing the intended behavior.
This assertion hard-codes the exact CSS serialization and path, so minor changes (e.g. added query params, different quoting/whitespace) will break the test without changing behavior. Consider asserting on a more flexible pattern (e.g. checking that theme_css contains api.yppp.net with a url( guard, or using a regex to ensure an https://api.yppp.net/ URL) so the test validates the host/protocol constraint without depending on formatting details.
Suggested implementation:
self.assertNotIn("fonts.googleapis.com", theme_css)
self.assertNotIn("fonts.gstatic.com", theme_css)
# Ensure the random background uses an HTTPS URL to api.yppp.net without depending on exact CSS serialization
self.assertRegex(
theme_css,
r'url\(\s*[\'"]https://api\.yppp\.net/[^\'"]*[\'"]\s*\)'
)
self.assertNotIn("fonts.googleapis.com", obs_overlay)If this test class does not already inherit from unittest.TestCase or a subclass that provides assertRegex, you may need to ensure it does so, or import unittest and adjust accordingly. The regex is intentionally permissive about the path and query string after https://api.yppp.net/; if you want to constrain it further (e.g. specifically /api.php), you can change [^\'"]* to api\.php[^\'"]*.
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 Sourcery
将高级材质主题的背景图片切换为外部随机背景图片 API，同时保持字体资源自托管，并更新测试以反映新行为。
Enhancements:
advanced-material-alas主题更新为使用外部随机背景图片 API，而不是内置的静态背景资源。Tests:
Original summary in English
Summary by Sourcery
Switch the advanced material theme background image to an external random background API while keeping font assets self-hosted and update tests to reflect the new behavior.
Enhancements:
Tests: