feat(webui): split dashboard into module pages for 3.0.1#112
Open
EterUltimate wants to merge 2 commits into
Open
feat(webui): split dashboard into module pages for 3.0.1#112EterUltimate wants to merge 2 commits into
EterUltimate wants to merge 2 commits into
Conversation
Reviewer's GuideClassifies WebUI provider configuration and dashboard controls by normalized AstrBot provider types, enriching provider metadata with type labels and sourcing options from both runtime provider instances and AstrBot provider config, while adding backend and frontend tests to ensure correct grouping and filtering. Sequence diagram for dashboard provider field rendering by normalized typesequenceDiagram
participant Browser
participant DashboardJS as dashboard_html_js
participant Backend as ConfigService
Browser->>DashboardJS: requestConfigSchema()
DashboardJS->>Backend: get_config_schema
Backend-->>DashboardJS: payload with provider_options_by_type
DashboardJS->>DashboardJS: normalizeProviderType(field.provider_type)
DashboardJS->>DashboardJS: providerOptionsForField(field)
DashboardJS->>DashboardJS: providerTypeLabel(field.provider_type)
DashboardJS->>Browser: render select with
DashboardJS-->>Browser: option.label + providerTypeLabel(option.provider_type)
Flow diagram for backend provider option aggregation by normalized typeflowchart TD
A["_provider_options(expected_type)"] --> B["normalize_provider_type(expected_type)"]
B --> C[get_service_factory]
C --> D[get context]
D --> E{expected in chat_completion group?}
E -->|yes| F[_provider_option for context.get_all_providers]
E -->|yes| G[_provider_option for provider_manager.provider_insts]
D --> H{expected in embedding group?}
H -->|yes| I[_provider_option for context.get_all_embedding_providers]
H -->|yes| J[_provider_option for provider_manager.embedding_provider_insts]
D --> K{expected in rerank group?}
K -->|yes| L[_provider_option for rerank providers]
D --> M["_provider_source_types(provider_manager)"]
M --> N["_provider_option_from_config for provider_manager.providers_config"]
N --> O[filter by expected type]
F --> P[_dedupe_options]
G --> P
I --> P
J --> P
L --> P
O --> P
P --> Q[(provider options)]
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 - I've found 3 issues, and left some high level feedback:
- The provider option construction logic in
_provider_optionand_provider_option_from_configis very similar; consider extracting a shared helper to avoid subtle drift in how labels and type metadata are built for different provider sources. - The provider type label mappings are duplicated between
_PROVIDER_TYPE_LABELSin Python andPROVIDER_TYPE_LABELSin the dashboard JS; if possible, derive the labels from the backend‐supplied schema to avoid future divergence when types or labels change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The provider option construction logic in `_provider_option` and `_provider_option_from_config` is very similar; consider extracting a shared helper to avoid subtle drift in how labels and type metadata are built for different provider sources.
- The provider type label mappings are duplicated between `_PROVIDER_TYPE_LABELS` in Python and `PROVIDER_TYPE_LABELS` in the dashboard JS; if possible, derive the labels from the backend‐supplied schema to avoid future divergence when types or labels change.
## Individual Comments
### Comment 1
<location path="web_res/static/html/dashboard.html" line_range="3174-3176" />
<code_context>
badges.push('重启后生效');
}
if (field.widget === 'provider') {
- badges.push('Provider');
+ badges.push(providerTypeLabel(field.provider_type || field.provider_type_label));
}
if (field.nullable) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Re-wrapping provider_type_label with providerTypeLabel is redundant and may be fragile.
Using `providerTypeLabel(field.provider_type || field.provider_type_label)` means we may pass an already-localized `provider_type_label` back into a function that expects a type key, which can break if labels are non-lowercase or change. Instead, use `field.provider_type_label` directly and only call `providerTypeLabel(field.provider_type)` as a fallback when the label is absent.
```suggestion
if (field.widget === 'provider') {
// Prefer the pre-localized label if present; fall back to deriving it from provider_type
badges.push(field.provider_type_label || providerTypeLabel(field.provider_type));
}
```
</issue_to_address>
### Comment 2
<location path="web_res/static/html/dashboard.html" line_range="3228-3237" />
<code_context>
+ } else if (field.widget === 'provider' && providerOptionsForField(field).length) {
</code_context>
<issue_to_address>
**suggestion:** providerOptionsForField re-computes payload and filtering on every render and could be cached or simplified.
In the provider branch of `renderConfigField`, `providerOptionsForField(field)` is invoked twice (once for the length check and once for the options themselves), each time re-reading the schema payload and re-filtering. Consider calling it once, storing the result in a local variable, and reusing it to simplify control flow and avoid any future inconsistency if the payload becomes mutable.
Suggested implementation:
```
if (field.nullable) {
badges.push('可空');
${field.editable ? '' : 'disabled'}
>${selectOptions.join('')}</select>
`;
} else if (field.widget === 'provider') {
const options = providerOptionsForField(field);
if (!options.length) {
return '';
}
const currentValue = displayValue || '';
const optionSet = options.some((option) => option.value === currentValue);
const selectOptions = [
`<option value="">未设置</option>`,
...options.map((option) => {
const optionValue = option.value ?? '';
const label = option.label || optionValue;
```
If `renderConfigField` should not early-return an empty string for provider fields without options (for example, if surrounding code expects a specific default markup instead of nothing), adjust the `return ''` line accordingly. You may instead no-op or fall back to the standard text widget rendering, depending on your existing conventions in this function.
</issue_to_address>
### Comment 3
<location path="web_res/static/html/dashboard.html" line_range="3261" />
<code_context>
type="text"
value="${escapeHtml(displayValue)}"
- placeholder="未配置"
+ placeholder="未找到 ${escapeHtml(providerTypeLabel(field.provider_type))} Provider，可手动填写 Provider ID"
data-config-key="${escapeHtml(field.key)}"
data-config-type="${escapeHtml(field.type)}"
</code_context>
<issue_to_address>
**suggestion:** Placeholder ignores provider_type_label and may show a generic or raw type instead of the localized label.
Here you rely only on `providerTypeLabel(field.provider_type)` for the placeholder, but other parts of the UI also use `field.provider_type_label`. If the backend only sets `provider_type_label` or the type isn’t recognized, this will fall back to a generic label or internal key. To stay consistent with the badge/select labels and avoid confusing text, use `field.provider_type_label || providerTypeLabel(field.provider_type)` instead.
```suggestion
placeholder="未找到 ${escapeHtml(field.provider_type_label || providerTypeLabel(field.provider_type))} Provider，可手动填写 Provider ID"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
3174
to
3176
| if (field.widget === 'provider') { | ||
| badges.push('Provider'); | ||
| badges.push(providerTypeLabel(field.provider_type || field.provider_type_label)); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Re-wrapping provider_type_label with providerTypeLabel is redundant and may be fragile.
Using providerTypeLabel(field.provider_type || field.provider_type_label) means we may pass an already-localized provider_type_label back into a function that expects a type key, which can break if labels are non-lowercase or change. Instead, use field.provider_type_label directly and only call providerTypeLabel(field.provider_type) as a fallback when the label is absent.
Suggested change
| if (field.widget === 'provider') { | |
| badges.push('Provider'); | |
| badges.push(providerTypeLabel(field.provider_type || field.provider_type_label)); | |
| } | |
| if (field.widget === 'provider') { | |
| // Prefer the pre-localized label if present; fall back to deriving it from provider_type | |
| badges.push(field.provider_type_label || providerTypeLabel(field.provider_type)); | |
| } |
Comment on lines
+3228
to
+3237
| } else if (field.widget === 'provider' && providerOptionsForField(field).length) { | ||
| const currentValue = displayValue || ''; | ||
| const options = safeArray(field.options || []); | ||
| const options = providerOptionsForField(field); | ||
| const optionSet = options.some((option) => option.value === currentValue); | ||
| const selectOptions = [ | ||
| `<option value="">未设置</option>`, | ||
| ...options.map((option) => { | ||
| const optionValue = option.value ?? ''; | ||
| const label = option.label || optionValue; | ||
| return `<option value="${escapeHtml(optionValue)}" ${optionValue === currentValue ? 'selected' : ''}>${escapeHtml(label)}</option>`; | ||
| const optionType = providerTypeLabel(option.provider_type || field.provider_type); |
There was a problem hiding this comment.
suggestion: providerOptionsForField re-computes payload and filtering on every render and could be cached or simplified.
In the provider branch of renderConfigField, providerOptionsForField(field) is invoked twice (once for the length check and once for the options themselves), each time re-reading the schema payload and re-filtering. Consider calling it once, storing the result in a local variable, and reusing it to simplify control flow and avoid any future inconsistency if the payload becomes mutable.
Suggested implementation:
if (field.nullable) {
badges.push('可空');
${field.editable ? '' : 'disabled'}
>${selectOptions.join('')}</select>
`;
} else if (field.widget === 'provider') {
const options = providerOptionsForField(field);
if (!options.length) {
return '';
}
const currentValue = displayValue || '';
const optionSet = options.some((option) => option.value === currentValue);
const selectOptions = [
`<option value="">未设置</option>`,
...options.map((option) => {
const optionValue = option.value ?? '';
const label = option.label || optionValue;
If renderConfigField should not early-return an empty string for provider fields without options (for example, if surrounding code expects a specific default markup instead of nothing), adjust the return '' line accordingly. You may instead no-op or fall back to the standard text widget rendering, depending on your existing conventions in this function.
| type="text" | ||
| value="${escapeHtml(displayValue)}" | ||
| placeholder="未配置" | ||
| placeholder="未找到 ${escapeHtml(providerTypeLabel(field.provider_type))} Provider，可手动填写 Provider ID" |
There was a problem hiding this comment.
suggestion: Placeholder ignores provider_type_label and may show a generic or raw type instead of the localized label.
Here you rely only on providerTypeLabel(field.provider_type) for the placeholder, but other parts of the UI also use field.provider_type_label. If the backend only sets provider_type_label or the type isn’t recognized, this will fall back to a generic label or internal key. To stay consistent with the badge/select labels and avoid confusing text, use field.provider_type_label || providerTypeLabel(field.provider_type) instead.
Suggested change
| placeholder="未找到 ${escapeHtml(providerTypeLabel(field.provider_type))} Provider，可手动填写 Provider ID" | |
| placeholder="未找到 ${escapeHtml(field.provider_type_label || providerTypeLabel(field.provider_type))} Provider，可手动填写 Provider ID" |
EterUltimate
changed the title
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
Tests