Add Route Manager Hub shadow integration - #3485
Closed
yuheng-888 wants to merge 7 commits into
Closed
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Route Manager Hub: UI panel and dashboard integration, sidebar entry and admin toggle, hub state/hooks and data loading, helpers for status/navigation/dashboard/iframe/option updates, Home/About embedding changes, extensive tests, and i18n strings across locales. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Hook as useDashboardData
participant Status as Global StatusState
participant API as Hub API
participant Helpers as Hub Helpers
UI->>Status: read hub_status (configured/reachable)
Note right of Status: buildRouteManagerHubAvailabilitySignature
UI->>Hook: trigger loadHubData (on signature change / refresh)
Hook->>Status: verify configured & reachable
alt hub configured & reachable
Hook->>API: GET /hub/api/dashboard/summary (skipErrorHandler)
API-->>Hook: summary payload
Hook->>Helpers: buildRouteManagerHubDashboardSnapshot(payload)
Helpers-->>Hook: normalized snapshot, highlights, links
Hook-->>UI: set hubSummary, hubNodes, hubTasks, hubAlerts, hubSchedules
else not configured/reachable
Hook-->>UI: clear hub state / set hubError
end
UI->>Helpers: postIframeContext(theme/lang) on iframe load
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ Actions performedReview triggered.
|
![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: 7
🧹 Nitpick comments (9)
web/src/helpers/hubAvailability.test.mjs (1)
9-23: Add the missing0:1signature case for full matrix coverage.This test currently validates 3 of 4 boolean combinations for
configured/reachable.🧪 Suggested test addition
test('builds a stable signature for each availability combination', () => { expect(buildRouteManagerHubAvailabilitySignature()).toBe('0:0'); + expect( + buildRouteManagerHubAvailabilitySignature({ + configured: false, + reachable: true, + }), + ).toBe('0:1'); expect( buildRouteManagerHubAvailabilitySignature({ configured: true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/helpers/hubAvailability.test.mjs` around lines 9 - 23, Add the missing test assertion for the configured=false, reachable=true case so the test covers all four boolean combinations; specifically, in the test for buildRouteManagerHubAvailabilitySignature add an expect call for buildRouteManagerHubAvailabilitySignature({ configured: false, reachable: true }) toBe('0:1') so the full matrix (0:0, 1:0, 1:1, 0:1) is validated.web/src/helpers/hubStatusState.test.mjs (1)
87-142: Add a trim-focused fallback test for URL normalization.The helper trims URL inputs, but current cases only use already-clean strings.
🧪 Suggested test addition
describe('buildRouteManagerHubCheckFallbackStatus', () => { + test('treats whitespace-only URLs as empty', () => { + expect( + buildRouteManagerHubCheckFallbackStatus({ + routeManagerURL: ' ', + pendingRouteManagerURL: ' ', + }), + ).toEqual({ + configured: false, + reachable: false, + source: 'sync-fallback', + }); + }); + test('marks the hub as configured but unreachable when Route Manager URL is present', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/helpers/hubStatusState.test.mjs` around lines 87 - 142, The tests for buildRouteManagerHubCheckFallbackStatus don't cover trimming/normalization of URLs; add a new test case that supplies a URL with surrounding whitespace (e.g., routeManagerURL or pendingRouteManagerURL containing leading/trailing spaces) and assert the same result as the existing "configured but unreachable" case ({ configured: true, reachable: false, source: 'sync-fallback' }) to verify the helper trims input before deciding configuration status.web/src/helpers/hubNavigation.test.mjs (1)
20-38: Add a visibility test for “configured but unreachable”.Line 21 and Line 30 allow
configured && reachablelogic to pass; add{ configured: true, reachable: false }to lock intended degraded-state behavior.🧪 Suggested test addition
describe('shouldShowRouteManagerHubEntry', () => { test('shows the hub entry when the hub is configured', () => { expect( shouldShowRouteManagerHubEntry({ configured: true, reachable: true, }), ).toBe(true); }); + + test('shows the hub entry when configured but temporarily unreachable', () => { + expect( + shouldShowRouteManagerHubEntry({ + configured: true, + reachable: false, + }), + ).toBe(true); + }); test('hides the hub entry when the hub is not configured', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/helpers/hubNavigation.test.mjs` around lines 20 - 38, The tests miss the "configured but unreachable" case for shouldShowRouteManagerHubEntry; add a new test in the describe block that calls shouldShowRouteManagerHubEntry({ configured: true, reachable: false }) and asserts it returns false so degraded-state behavior is covered. Place this test alongside the existing ones (referencing the shouldShowRouteManagerHubEntry function) and follow the same test naming style and expect(...).toBe(false) assertion.web/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx (1)
42-72: Consolidate duplicated sidebar-admin default schema into a single source.The same default object is repeated in state init/reset/hydration fallback. This is easy to drift from
useSidebar’s canonical defaults (e.g., future modules likeconsole.hub).♻️ Proposed refactor
-import React, { useState, useEffect, useContext } from 'react'; +import React, { useState, useEffect, useContext } from 'react'; import { useTranslation } from 'react-i18next'; @@ import { API, showSuccess, showError } from '../../../helpers'; import { StatusContext } from '../../../context/Status'; +import { + DEFAULT_ADMIN_CONFIG, + mergeAdminConfig, +} from '../../../hooks/common/useSidebar'; @@ +const cloneAdminConfig = () => JSON.parse(JSON.stringify(DEFAULT_ADMIN_CONFIG)); + export default function SettingsSidebarModulesAdmin(props) { @@ - const [sidebarModulesAdmin, setSidebarModulesAdmin] = useState({ - chat: { ... }, - console: { ... }, - personal: { ... }, - admin: { ... }, - }); + const [sidebarModulesAdmin, setSidebarModulesAdmin] = useState( + cloneAdminConfig(), + ); @@ function resetSidebarModules() { - const defaultModules = { ... }; - setSidebarModulesAdmin(defaultModules); + setSidebarModulesAdmin(cloneAdminConfig()); showSuccess(t('已重置为默认配置')); } @@ try { const modules = JSON.parse(props.options.SidebarModulesAdmin); - setSidebarModulesAdmin({ - chat: { ... }, - console: { ... }, - personal: { ... }, - admin: { ... }, - }); + setSidebarModulesAdmin(mergeAdminConfig(modules)); } catch (error) { - const defaultModules = { ... }; - setSidebarModulesAdmin(defaultModules); + setSidebarModulesAdmin(cloneAdminConfig()); } } }, [props.options]);Based on learnings: Sidebar module permissions are intended to be centrally governed by
SidebarModulesAdmin; keeping one canonical schema helps preserve that model.Also applies to: 102-134, 179-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx` around lines 42 - 72, Create a single canonical default object (e.g., DEFAULT_SIDEBAR_MODULES_ADMIN) and use it everywhere instead of duplicating the literal: replace the inline object passed to useState for sidebarModulesAdmin, the reset logic that calls setSidebarModulesAdmin(...), and the hydration fallback that currently repeats the shape; export/import the constant if other modules (like useSidebar) need to share it so all places (sidebarModulesAdmin state init, setSidebarModulesAdmin reset/hydration usage) reference the same symbol and stay in sync when new module keys (e.g., console.hub) are added.web/src/i18n/hubLocaleCoverage.test.mjs (1)
123-131: Prefer auto-discovering locale files instead of maintaining a hardcoded list.A static
LOCALE_FILESlist is easy to forget when adding new locales, which silently reduces test coverage.♻️ Proposed refactor
-import { readFileSync } from 'node:fs'; +import { readdirSync, readFileSync } from 'node:fs'; @@ -const LOCALE_FILES = [ - 'en.json', - 'fr.json', - 'ja.json', - 'ru.json', - 'vi.json', - 'zh-CN.json', - 'zh-TW.json', -]; +const LOCALE_FILES = readdirSync(LOCALES_DIR) + .filter((filename) => filename.endsWith('.json')) + .sort();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/i18n/hubLocaleCoverage.test.mjs` around lines 123 - 131, Replace the hardcoded LOCALE_FILES array by auto-discovering locale JSON files at test runtime: use the test file's file-system access (e.g., fs.readdirSync or a glob) to read the locales directory, filter for *.json (and any locale filename pattern like zh-*.json), sort or normalize the list, and assign that result to LOCALE_FILES so new locale files are automatically included; update any assertions that expect a specific order to operate on sets or sorted lists if necessary (refer to LOCALE_FILES and the test assertions in hubLocaleCoverage.test.mjs).web/src/pages/About/index.jsx (1)
176-181: Add an iframetitlefor accessibility.The embedded iframe should include a descriptive
titleso assistive technologies can identify it.♿ Suggested tweak
{isEmbeddableAboutPageURL(about) ? ( <iframe src={about} + title={t('关于')} style={{ width: '100%', height: '100vh', border: 'none' }} onLoad={handleAboutIframeLoad} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/About/index.jsx` around lines 176 - 181, Add an accessible title attribute to the embedded iframe: when rendering the iframe in the isEmbeddableAboutPageURL(about) branch (the element that currently sets src={about}, style and onLoad={handleAboutIframeLoad}), include a descriptive title prop (e.g., title="About page preview" or similar) so assistive technologies can identify the iframe content.web/src/pages/Home/index.jsx (1)
115-136: Consider memoizing or hoisting static quick links array.The
embeddedHubQuickLinksarray is recreated on every render but contains static hrefs. Moving it outside the component or wrapping withuseMemowould avoid unnecessary allocations.💡 Suggested optimization
// Outside component const EMBEDDED_HUB_QUICK_LINKS = [ { key: 'ai', href: getRouteManagerHubAIFleetHref() }, { key: 'network', href: getRouteManagerHubNetworkPanelHref('mihomo') }, { key: 'dns', href: getRouteManagerHubDNSSecurityHref() }, { key: 'egress', href: getRouteManagerHubEgressHref() }, { key: 'ha', href: getRouteManagerHubHomeAssistantEntitiesHref() }, ]; // Inside component const embeddedHubQuickLinks = EMBEDDED_HUB_QUICK_LINKS;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Home/index.jsx` around lines 115 - 136, The embeddedHubQuickLinks array is recreated every render; move it out of the component or memoize it to avoid allocations: extract the static array and its entries (keys and hrefs produced by getRouteManagerHubAIFleetHref, getRouteManagerHubNetworkPanelHref('mihomo'), getRouteManagerHubDNSSecurityHref, getRouteManagerHubEgressHref, getRouteManagerHubHomeAssistantEntitiesHref) into a top-level constant (e.g., EMBEDDED_HUB_QUICK_LINKS) or wrap the current embeddedHubQuickLinks in useMemo so the component uses a stable reference on each render.web/src/components/settings/OtherSetting.jsx (1)
603-610: Consider extracting the tone-to-type mapping.The nested ternary for Banner
typeprop is readable but could be cleaner with a mapping object.💡 Suggested simplification
+const BANNER_TONE_TO_TYPE = { + success: 'success', + warning: 'warning', + danger: 'danger', +}; <Banner fullMode={false} - type={hubStatus.tone === 'success' ? 'success' : hubStatus.tone === 'warning' ? 'warning' : 'danger'} + type={BANNER_TONE_TO_TYPE[hubStatus.tone] || 'danger'} description={hubStatus.message} closeIcon={null} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/OtherSetting.jsx` around lines 603 - 610, The Banner component's type prop uses a nested ternary based on hubStatus.tone which is hard to read; replace the ternary in the JSX (where Banner is rendered with type={...}) by creating a small mapping object (e.g., const TONE_TO_TYPE = { success: 'success', warning: 'warning', default: 'danger' } or similar) and compute the prop value via a lookup like TONE_TO_TYPE[hubStatus?.tone] || TONE_TO_TYPE.default before rendering; update the render to pass that variable to Banner and keep hubStatus, Banner, and the type prop names unchanged.web/src/pages/Home/homePageContent.js (1)
101-110: Consider handling non-stringdatain the success path.If the API returns non-string data (e.g.,
null, an object, or an array), it will be passed through asresolvedContent. This could cause issues when the caller attempts to use it as an iframesrcor render it as HTML viadangerouslySetInnerHTML.💡 Suggested defensive check
+ if (typeof data !== 'string') { + return { + content: fallbackContent, + errorMessage: fallbackContent, + shouldPersist: false, + }; + } + const resolvedContent = - typeof data === 'string' && !isEmbeddableHomePageURL(data) + !isEmbeddableHomePageURL(data) ? marked.parse(data) : data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Home/homePageContent.js` around lines 101 - 110, The current success path lets non-string data flow into resolvedContent which can break iframe src or dangerouslySetInnerHTML; update the assignment for resolvedContent (the variable computed using data, isEmbeddableHomePageURL, and marked.parse) to explicitly handle non-string values — e.g., if typeof data === 'string' keep the existing logic, else set resolvedContent to an empty string (or a safe fallback) and adjust any related flags (like shouldPersist or errorMessage) as needed so callers never receive null/object/array as content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/layout/SiderBar.jsx`:
- Around line 78-93: The sidebar currently gates Hub links using
shouldShowRouteManagerHubEntry(statusState?.status?.hub_status) which only
checks configured; change the gate to require the Hub be reachable too
(configured && reachable) so links are hidden when hub_status.reachable is
false—either update shouldShowRouteManagerHubEntry to include reachable or
replace the check with statusState?.status?.hub_status?.configured &&
statusState?.status?.hub_status?.reachable; apply the same change for both uses
(the routeManagerHubVisible computation and the other spot that mirrors lines
~133-138) and ensure fullReloadRouteMap is only built when the Hub is both
configured and reachable.
In `@web/src/helpers/optionUpdate.js`:
- Around line 1-4: The helper ensureOptionUpdateSucceeded currently hardcodes an
English fallback ('Option update failed'); update it to avoid hardcoded UI text
by either (A) removing the English literal default and making fallbackMessage
optional so callers must pass a translated string, or (B) import and call
i18next.t('optionUpdate.failed') inside ensureOptionUpdateSucceeded to supply a
localized default; change the function signature and defaulting logic in
ensureOptionUpdateSucceeded (and any other similar helpers around lines 19-22)
to use i18next.t(...) or require a caller-provided translated fallback.
In `@web/src/hooks/dashboard/useDashboardData.js`:
- Around line 303-362: The client mapper in setHubSummary uses only snake_case
keys (e.g., online_nodes, primary_node, home_assistant, ai.model_count,
network.mihomo_configured) which drops camelCase payloads; update the mapping to
accept both snake_case and camelCase variants (e.g., check
summaryRes.data?.data?.online_nodes || summaryRes.data?.data?.onlineNodes) or
pass the payload through the shared normalizer from
web/src/helpers/hubDashboard.js before mapping; specifically update accesses
inside setHubSummary for onlineNodes, busyNodes, pendingTasks, activeSchedules,
criticalAlerts, unacknowledgedAlerts,
ai.modelCount/aiCapableNodes/onlineAINodes, network.*
(mihomoConfigured/mihomoReachable/dnsConfigured/dnsReachable/dnsProtectionEnabled/egress*),
homeAssistant.* (configured/reachable/entityCount) and primaryNode.*
(nodeID/hostname/status/ipAddress) to read both naming styles.
- Around line 383-391: The code currently clears hub state and then
unconditionally calls setHubError(''), which discards the Hub API's failure
message; update the failure branch that calls
setHubSummary(createEmptyHubSummary()) / setHubNodes([]) / setHubSchedules([]) /
setHubTasks([]) / setHubAlerts([]) so that it preserves or sets the API error
message instead of wiping it—use the API response's error/message field (e.g.,
response.message or response.error) with setHubError(...) when success is false,
and only call setHubError('') when the response indicates success.
In `@web/src/i18n/hubLocaleCoverage.test.mjs`:
- Around line 10-121: HUB_LOCALE_KEYS is missing newly added hub/shortcut locale
keys which causes missing translations; update the HUB_LOCALE_KEYS array in
web/src/i18n/hubLocaleCoverage.test.mjs (the constant named HUB_LOCALE_KEYS) to
include the new keys such as "全屏打开中枢", "打开中枢", "中枢总览", "打开新窗口", "页面不存在", and
"当前快捷入口不存在，或你没有访问权限。" (and any other newly introduced hub/shortcut strings) so
the test covers all current hub translations; locate the HUB_LOCALE_KEYS
constant and append the missing strings ensuring duplicates are not introduced.
In `@web/src/i18n/locales/vi.json`:
- Line 964: Remove the duplicate JSON entry for the key "激活" in the vi locale so
the original translation isn't overwritten, and replace/merge it with the
correct key "已激活" (keeping the intended Vietnamese value "Đang hoạt động") —
then update the UI usage to call t('已激活') where the Hub status label is rendered
(search for existing t('激活') usages and change them to t('已激活')).
In `@web/src/pages/About/aboutPageContent.js`:
- Around line 14-19: The returned object is wrongly using fallbackContent as the
error message; update the code paths that set errorMessage (where currently
errorMessage: message || fallbackContent) to instead use the explicit error
message or a safe default (e.g., message || 'An error occurred' or null) so
callers don't receive stale HTML/iframe strings; change all occurrences that
assign fallbackContent to errorMessage (referencing the variables
fallbackContent, message, and the returned property errorMessage in this module)
to use message || DEFAULT_ERROR_MESSAGE (or null) while leaving content:
fallbackContent unchanged and preserving shouldPersist behavior.
---
Nitpick comments:
In `@web/src/components/settings/OtherSetting.jsx`:
- Around line 603-610: The Banner component's type prop uses a nested ternary
based on hubStatus.tone which is hard to read; replace the ternary in the JSX
(where Banner is rendered with type={...}) by creating a small mapping object
(e.g., const TONE_TO_TYPE = { success: 'success', warning: 'warning', default:
'danger' } or similar) and compute the prop value via a lookup like
TONE_TO_TYPE[hubStatus?.tone] || TONE_TO_TYPE.default before rendering; update
the render to pass that variable to Banner and keep hubStatus, Banner, and the
type prop names unchanged.
In `@web/src/helpers/hubAvailability.test.mjs`:
- Around line 9-23: Add the missing test assertion for the configured=false,
reachable=true case so the test covers all four boolean combinations;
specifically, in the test for buildRouteManagerHubAvailabilitySignature add an
expect call for buildRouteManagerHubAvailabilitySignature({ configured: false,
reachable: true }) toBe('0:1') so the full matrix (0:0, 1:0, 1:1, 0:1) is
validated.
In `@web/src/helpers/hubNavigation.test.mjs`:
- Around line 20-38: The tests miss the "configured but unreachable" case for
shouldShowRouteManagerHubEntry; add a new test in the describe block that calls
shouldShowRouteManagerHubEntry({ configured: true, reachable: false }) and
asserts it returns false so degraded-state behavior is covered. Place this test
alongside the existing ones (referencing the shouldShowRouteManagerHubEntry
function) and follow the same test naming style and expect(...).toBe(false)
assertion.
In `@web/src/helpers/hubStatusState.test.mjs`:
- Around line 87-142: The tests for buildRouteManagerHubCheckFallbackStatus
don't cover trimming/normalization of URLs; add a new test case that supplies a
URL with surrounding whitespace (e.g., routeManagerURL or pendingRouteManagerURL
containing leading/trailing spaces) and assert the same result as the existing
"configured but unreachable" case ({ configured: true, reachable: false, source:
'sync-fallback' }) to verify the helper trims input before deciding
configuration status.
In `@web/src/i18n/hubLocaleCoverage.test.mjs`:
- Around line 123-131: Replace the hardcoded LOCALE_FILES array by
auto-discovering locale JSON files at test runtime: use the test file's
file-system access (e.g., fs.readdirSync or a glob) to read the locales
directory, filter for *.json (and any locale filename pattern like zh-*.json),
sort or normalize the list, and assign that result to LOCALE_FILES so new locale
files are automatically included; update any assertions that expect a specific
order to operate on sets or sorted lists if necessary (refer to LOCALE_FILES and
the test assertions in hubLocaleCoverage.test.mjs).
In `@web/src/pages/About/index.jsx`:
- Around line 176-181: Add an accessible title attribute to the embedded iframe:
when rendering the iframe in the isEmbeddableAboutPageURL(about) branch (the
element that currently sets src={about}, style and
onLoad={handleAboutIframeLoad}), include a descriptive title prop (e.g.,
title="About page preview" or similar) so assistive technologies can identify
the iframe content.
In `@web/src/pages/Home/homePageContent.js`:
- Around line 101-110: The current success path lets non-string data flow into
resolvedContent which can break iframe src or dangerouslySetInnerHTML; update
the assignment for resolvedContent (the variable computed using data,
isEmbeddableHomePageURL, and marked.parse) to explicitly handle non-string
values — e.g., if typeof data === 'string' keep the existing logic, else set
resolvedContent to an empty string (or a safe fallback) and adjust any related
flags (like shouldPersist or errorMessage) as needed so callers never receive
null/object/array as content.
In `@web/src/pages/Home/index.jsx`:
- Around line 115-136: The embeddedHubQuickLinks array is recreated every
render; move it out of the component or memoize it to avoid allocations: extract
the static array and its entries (keys and hrefs produced by
getRouteManagerHubAIFleetHref, getRouteManagerHubNetworkPanelHref('mihomo'),
getRouteManagerHubDNSSecurityHref, getRouteManagerHubEgressHref,
getRouteManagerHubHomeAssistantEntitiesHref) into a top-level constant (e.g.,
EMBEDDED_HUB_QUICK_LINKS) or wrap the current embeddedHubQuickLinks in useMemo
so the component uses a stable reference on each render.
In `@web/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx`:
- Around line 42-72: Create a single canonical default object (e.g.,
DEFAULT_SIDEBAR_MODULES_ADMIN) and use it everywhere instead of duplicating the
literal: replace the inline object passed to useState for sidebarModulesAdmin,
the reset logic that calls setSidebarModulesAdmin(...), and the hydration
fallback that currently repeats the shape; export/import the constant if other
modules (like useSidebar) need to share it so all places (sidebarModulesAdmin
state init, setSidebarModulesAdmin reset/hydration usage) reference the same
symbol and stay in sync when new module keys (e.g., console.hub) are added.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02f0006f-c6c9-4242-af1d-e6f0ee2d7c64
📒 Files selected for processing (36)
web/src/components/dashboard/RouteManagerHubPanel.jsxweb/src/components/dashboard/index.jsxweb/src/components/layout/SiderBar.jsxweb/src/components/settings/OtherSetting.jsxweb/src/helpers/hubAvailability.jsweb/src/helpers/hubAvailability.test.mjsweb/src/helpers/hubDashboard.jsweb/src/helpers/hubDashboard.test.mjsweb/src/helpers/hubNavigation.jsweb/src/helpers/hubNavigation.test.mjsweb/src/helpers/hubStatus.jsweb/src/helpers/hubStatus.test.mjsweb/src/helpers/hubStatusState.jsweb/src/helpers/hubStatusState.test.mjsweb/src/helpers/iframeContext.jsweb/src/helpers/iframeContext.test.mjsweb/src/helpers/index.jsweb/src/helpers/optionUpdate.jsweb/src/helpers/optionUpdate.test.mjsweb/src/hooks/common/useSidebar.jsweb/src/hooks/dashboard/useDashboardData.jsweb/src/i18n/hubLocaleCoverage.test.mjsweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh-CN.jsonweb/src/i18n/locales/zh-TW.jsonweb/src/pages/About/aboutPageContent.jsweb/src/pages/About/aboutPageContent.test.mjsweb/src/pages/About/index.jsxweb/src/pages/Home/homePageContent.jsweb/src/pages/Home/homePageContent.test.mjsweb/src/pages/Home/index.jsxweb/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx
Comment on lines
+78
to
+93
| const routeManagerHubVisible = shouldShowRouteManagerHubEntry( | ||
| statusState?.status?.hub_status, | ||
| ); | ||
| const routeManagerHubSidebarItems = useMemo( | ||
| () => getRouteManagerHubSidebarItems(t), | ||
| [t], | ||
| ); | ||
| const fullReloadRouteMap = useMemo( | ||
| () => | ||
| routeManagerHubVisible | ||
| ? routeManagerHubSidebarItems.reduce((result, item) => { | ||
| result[item.itemKey] = item.to; | ||
| return result; | ||
| }, {}) | ||
| : {}, | ||
| [routeManagerHubSidebarItems, routeManagerHubVisible], |
Contributor
There was a problem hiding this comment.
Gate Hub navigation on reachable, not only configured.
Line 78 uses shouldShowRouteManagerHubEntry(statusState?.status?.hub_status), and that helper only checks configured. In the { configured: true, reachable: false } state, this sidebar still renders full-reload Hub links even though web/src/hooks/dashboard/useDashboardData.js treats the Hub as unavailable and clears its data.
💡 Suggested fix
- const routeManagerHubVisible = shouldShowRouteManagerHubEntry(
- statusState?.status?.hub_status,
- );
+ const hubStatus = statusState?.status?.hub_status;
+ const routeManagerHubVisible = Boolean(
+ hubStatus?.configured && hubStatus?.reachable,
+ );Also applies to: 133-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/layout/SiderBar.jsx` around lines 78 - 93, The sidebar
currently gates Hub links using
shouldShowRouteManagerHubEntry(statusState?.status?.hub_status) which only
checks configured; change the gate to require the Hub be reachable too
(configured && reachable) so links are hidden when hub_status.reachable is
false—either update shouldShowRouteManagerHubEntry to include reachable or
replace the check with statusState?.status?.hub_status?.configured &&
statusState?.status?.hub_status?.reachable; apply the same change for both uses
(the routeManagerHubVisible computation and the other spot that mirrors lines
~133-138) and ensure fullReloadRouteMap is only built when the Hub is both
configured and reachable.
Contributor
There was a problem hiding this comment.
@yuheng-888, that's a reasonable UX rationale — preserving the sidebar entry as a recovery path during transient outages is a valid design choice, especially when the upstream reachability check can be flaky. I'll note this intent for future reviews.
✏️ Learnings added
Learnt from: yuheng-888
Repo: QuantumNous/new-api PR: 3485
File: web/src/components/layout/SiderBar.jsx:78-93
Timestamp: 2026-03-28T13:33:28.052Z
Learning: In `web/src/components/layout/SiderBar.jsx`, `routeManagerHubVisible` is intentionally gated only on `hub_status.configured` (via `shouldShowRouteManagerHubEntry`), NOT on `hub_status.reachable`. The sidebar hub entry is kept visible when the hub is configured-but-unreachable so users retain a direct navigation path to `/hub/` for retry, diagnostics, and recovery during transient outages. Do not suggest adding a `reachable` check to this gate.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+14
to
+19
| if (!success) { | ||
| return { | ||
| content: fallbackContent, | ||
| errorMessage: message || fallbackContent, | ||
| shouldPersist: false, | ||
| }; |
Contributor
There was a problem hiding this comment.
Don't use fallbackContent as the error message.
fallbackContent is the render fallback, not a reliable toast string. On Line 17 and Line 35, a failed request can return stale HTML/iframe URLs or '' via errorMessage, which makes the caller's showError(...) noisy or useless.
💡 Suggested split between content fallback and error fallback
export async function loadAboutPageContent(
requestAboutContent,
fallbackContent = '',
+ fallbackErrorMessage = '',
) {
try {
const { success, message, data } = await requestAboutContent();
if (!success) {
return {
content: fallbackContent,
- errorMessage: message || fallbackContent,
+ errorMessage: message || fallbackErrorMessage,
shouldPersist: false,
};
}
@@
- } catch {
+ } catch (error) {
return {
content: fallbackContent,
- errorMessage: fallbackContent,
+ errorMessage:
+ error?.response?.data?.message || fallbackErrorMessage,
shouldPersist: false,
};
}
}Also applies to: 32-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/About/aboutPageContent.js` around lines 14 - 19, The returned
object is wrongly using fallbackContent as the error message; update the code
paths that set errorMessage (where currently errorMessage: message ||
fallbackContent) to instead use the explicit error message or a safe default
(e.g., message || 'An error occurred' or null) so callers don't receive stale
HTML/iframe strings; change all occurrences that assign fallbackContent to
errorMessage (referencing the variables fallbackContent, message, and the
returned property errorMessage in this module) to use message ||
DEFAULT_ERROR_MESSAGE (or null) while leaving content: fallbackContent unchanged
and preserving shouldPersist behavior.
Contributor
There was a problem hiding this comment.
@yuheng-888, understood — thanks for the clarification. Since fallbackContent is always a translated failure string (t('加载关于内容失败...')) at the call site, it's safe to reuse it as errorMessage here and the concern about leaking stale HTML or iframe URLs doesn't apply. Makes sense to keep it as-is.
✏️ Learnings added
Learnt from: yuheng-888
Repo: QuantumNous/new-api PR: 3485
File: web/src/pages/About/aboutPageContent.js:14-19
Timestamp: 2026-03-28T13:33:30.276Z
Learning: In `web/src/pages/About/aboutPageContent.js` (`loadAboutPageContent`), `fallbackContent` is intentionally reused as `errorMessage` in failure paths. The caller always passes a localized failure string (e.g., `t('加载关于内容失败...')`) as `fallbackContent`, so it is safe to use as the error message — it does not leak stale HTML or iframe URL content.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/settings/OtherSetting.jsx (1)
411-423:⚠️ Potential issue | 🟠 MajorHydrating a cleared
RouteManagerURLshould also invalidate cached hub state.Line 422 updates
persistedRouteManagerURL, but Lines 434-445 only derivehubStatusfromstatusStateplus the saved snapshot. If the option was cleared server-side while localStorage still contains an older hub snapshot, this page can keep showing the stale banner until another status check runs. Please clear/sync the cached hub state when the fetched URL is empty, or make the banner derivation depend onpersistedRouteManagerURL.Also applies to: 434-445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/OtherSetting.jsx` around lines 411 - 423, The fetched options hydrate persistedRouteManagerURL but doesn’t clear the cached hub state when the RouteManagerURL is empty; after setPersistedRouteManagerURL(newInputs.RouteManagerURL || '') inside getOptions, add logic to clear/sync the cached hub status used to compute hubStatus (e.g. reset the statusState or persisted hub snapshot variable—the same state used in the hubStatus derivation block around where hubStatus is computed) when newInputs.RouteManagerURL is falsy; alternatively, make the banner derivation depend on persistedRouteManagerURL so a cleared URL forces recomputation—update the getOptions flow and the hubStatus derivation to reference persistedRouteManagerURL and reset the cached snapshot/state (via the existing setStatusState or setPersistedHubSnapshot setter) when the URL is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/settings/OtherSetting.jsx`:
- Around line 303-305: The code assumes any 2xx response has usable hub data;
update the API.get handling to check the response envelope's success flag and
only pass real data when res.data?.success === true (otherwise pass
null/fallback) before calling syncGlobalHubStatus and
formatRouteManagerHubStatus; specifically, when handling the result of
API.get('/api/hub/status') inspect res.data?.success, set a local hubData =
res.data?.success ? res.data.data : null, then call syncGlobalHubStatus(hubData)
and formatRouteManagerHubStatus(hubData, t) so stale state/banner data is not
retained when the API returns success: false.
---
Outside diff comments:
In `@web/src/components/settings/OtherSetting.jsx`:
- Around line 411-423: The fetched options hydrate persistedRouteManagerURL but
doesn’t clear the cached hub state when the RouteManagerURL is empty; after
setPersistedRouteManagerURL(newInputs.RouteManagerURL || '') inside getOptions,
add logic to clear/sync the cached hub status used to compute hubStatus (e.g.
reset the statusState or persisted hub snapshot variable—the same state used in
the hubStatus derivation block around where hubStatus is computed) when
newInputs.RouteManagerURL is falsy; alternatively, make the banner derivation
depend on persistedRouteManagerURL so a cleared URL forces recomputation—update
the getOptions flow and the hubStatus derivation to reference
persistedRouteManagerURL and reset the cached snapshot/state (via the existing
setStatusState or setPersistedHubSnapshot setter) when the URL is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c04b5687-fab2-4596-a39f-d6528f631330
📒 Files selected for processing (3)
web/src/components/settings/OtherSetting.jsxweb/src/helpers/hubStatusState.jsweb/src/helpers/hubStatusState.test.mjs
✅ Files skipped from review due to trivial changes (1)
- web/src/helpers/hubStatusState.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/helpers/hubStatusState.js
Comment on lines
+303
to
+305
| const res = await API.get('/api/hub/status'); | ||
| syncGlobalHubStatus(res.data?.data || null); | ||
| const nextStatus = formatRouteManagerHubStatus(res.data?.data, t); |
Contributor
There was a problem hiding this comment.
Handle { success: false } hub responses before entering the success path.
This assumes any 2xx payload contains usable hub data. If /api/hub/status returns the project’s usual envelope with success: false, you'll skip the fallback branch and may keep stale hub state/banner data cached.
Possible normalization
const res = await API.get('/api/hub/status');
- syncGlobalHubStatus(res.data?.data || null);
- const nextStatus = formatRouteManagerHubStatus(res.data?.data, t);
+ const payload = res.data;
+ if (payload?.success === false) {
+ throw new Error(payload.message || t('Route Manager 状态检查失败'));
+ }
+ const hubData = payload?.data ?? payload;
+ syncGlobalHubStatus(hubData || null);
+ const nextStatus = formatRouteManagerHubStatus(hubData, t);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const res = await API.get('/api/hub/status'); | |
| syncGlobalHubStatus(res.data?.data || null); | |
| const nextStatus = formatRouteManagerHubStatus(res.data?.data, t); | |
| const res = await API.get('/api/hub/status'); | |
| const payload = res.data; | |
| if (payload?.success === false) { | |
| throw new Error(payload.message || t('Route Manager 状态检查失败')); | |
| } | |
| const hubData = payload?.data ?? payload; | |
| syncGlobalHubStatus(hubData || null); | |
| const nextStatus = formatRouteManagerHubStatus(hubData, t); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/settings/OtherSetting.jsx` around lines 303 - 305, The
code assumes any 2xx response has usable hub data; update the API.get handling
to check the response envelope's success flag and only pass real data when
res.data?.success === true (otherwise pass null/fallback) before calling
syncGlobalHubStatus and formatRouteManagerHubStatus; specifically, when handling
the result of API.get('/api/hub/status') inspect res.data?.success, set a local
hubData = res.data?.success ? res.data.data : null, then call
syncGlobalHubStatus(hubData) and formatRouteManagerHubStatus(hubData, t) so
stale state/banner data is not retained when the API returns success: false.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/layout/SiderBar.jsx (1)
63-80:⚠️ Potential issue | 🟡 MinorUse the saved hub snapshot while status is still loading.
Line 78 reads only
statusState?.status?.hub_status.StatusContextinitializes withstatus: undefined, so the hub section renders hidden on every hard refresh until the async status load completes. That means the sidebar doesn't actually reuse the cached hub availability this PR introduces, and users get a visible menu jump.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/layout/SiderBar.jsx` around lines 63 - 80, The sidebar currently computes routeManagerHubVisible from statusState?.status?.hub_status which is undefined during initial load; change the value fed to shouldShowRouteManagerHubEntry to first try statusState?.status?.hub_status and fall back to the cached snapshot field (e.g. statusState?.savedHubSnapshot?.hub_status or the existing snapshot property on StatusContext) so the sidebar uses the saved hub snapshot while status is loading; update the expression around routeManagerHubVisible to use this fallback (keep the same shouldShowRouteManagerHubEntry call).
🧹 Nitpick comments (4)
web/src/i18n/locales/fr.json (1)
947-957: Consider adding pluralization forms for count-based translations.Several new entries with
{{count}}interpolation don't follow the pluralization pattern used elsewhere in this file (e.g., lines 601-602 use_one/_many/_othersuffixes). For proper French grammar, singular forms should use "modèle", "nœud", "entité" instead of the plural forms.💡 Example fix for line 947-948
-"{{count}} 模型": "{{count}} modèles", +"{{count}} 模型_one": "{{count}} modèle", +"{{count}} 模型_many": "{{count}} modèles", +"{{count}} 模型_other": "{{count}} modèles",Similar changes could be applied to lines 949 (
{{count}} 在线 AI 节点) and 957 ({{count}} 实体).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/i18n/locales/fr.json` around lines 947 - 957, The count-based French translations like "{{count}} 模型", "{{count}} 在线 AI 节点", and "{{count}} 实体" must be converted to pluralization entries (e.g., use keys with suffixes such as "_one" and "_other" or your project's plural convention) and provide singular forms for French ("{{count}} modèle", "{{count}} nœud IA en ligne", "{{count}} entité") for the singular variant and plural forms ("{{count}} modèles", "{{count}} nœuds IA en ligne", "{{count}} entités") for the plural/other variant; update the JSON keys and values to follow the same pluralization pattern used elsewhere (see existing _one/_many/_other entries) so the i18n runtime can select singular vs plural.web/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx (1)
42-72: Deduplicate the admin default config.
hubnow has to stay aligned across this component's initial state, reset path, JSON-merge path, andweb/src/hooks/common/useSidebar.js. Pulling these defaults from one shared constant/helper will prevent the settings UI and the runtime sidebar checks from drifting the next time a module is added.Also applies to: 102-137, 179-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx` around lines 42 - 72, Extract the hard-coded sidebar defaults into a single shared constant (e.g., SIDEBAR_MODULES_DEFAULTS) and import it into SettingsSidebarModulesAdmin.jsx; replace the inline initial state object passed to useState (sidebarModulesAdmin) with SIDEBAR_MODULES_DEFAULTS and update the reset logic and any JSON-merge code in this file (references: sidebarModulesAdmin, setSidebarModulesAdmin) to use that constant; also update the other locations mentioned (the other ranges in this file and web/src/hooks/common/useSidebar.js) to import and reference the same SIDEBAR_MODULES_DEFAULTS so the admin "hub" and all module defaults remain consistent across initialization, reset, and runtime checks.web/src/helpers/optionUpdate.js (1)
1-27: Keep the fallback message out of the helper default.Line 3 and Line 21 bake user-facing English into a shared frontend helper. That makes it easy for future call sites to bypass i18n accidentally. Requiring callers to pass a translated fallback is safer than shipping an English default here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/helpers/optionUpdate.js` around lines 1 - 27, Remove the hardcoded English fallback by deleting the default values for fallbackMessage in both ensureOptionUpdateSucceeded and getOptionUpdateErrorMessage so callers must supply a translated fallback; in ensureOptionUpdateSucceeded keep the same logic to choose message vs fallbackMessage and throw new Error(normalizedMessage) (assume fallbackMessage is provided), and in getOptionUpdateErrorMessage return the trimmed error.message or the provided fallbackMessage (no internal default). Update all call sites to pass an explicit i18n/translated fallback string when invoking ensureOptionUpdateSucceeded and getOptionUpdateErrorMessage.web/src/helpers/hubDashboard.test.mjs (1)
82-320: Add regression tests fornullarrays and0-value alias precedence.Great coverage overall; two edge cases from
buildRouteManagerHubDashboardSnapshotare still unprotected and are worth locking down.✅ Suggested test additions
describe('buildRouteManagerHubDashboardSnapshot', () => { + test('handles null collections without throwing', () => { + const snapshot = buildRouteManagerHubDashboardSnapshot({ + nodes: null, + schedules: null, + tasks: null, + alerts: null, + }); + + expect(snapshot.recentTasks).toEqual([]); + expect(snapshot.recentSchedules).toEqual([]); + expect(snapshot.recentAlerts).toEqual([]); + }); + + test('preserves explicit zero in camelCase numeric fields when snake_case also exists', () => { + const snapshot = buildRouteManagerHubDashboardSnapshot({ + ai: { + modelCount: 0, + model_count: 7, + aiCapableNodes: 0, + ai_capable_nodes: 4, + onlineAINodes: 0, + online_ai_nodes: 3, + }, + homeAssistant: { + entityCount: 0, + }, + home_assistant: { + entity_count: 9, + }, + }); + + expect(snapshot.ai).toEqual({ + modelCount: 0, + aiCapableNodes: 0, + onlineAINodes: 0, + }); + expect(snapshot.homeAssistant.entityCount).toBe(0); + }); + test('builds node and task summaries for dashboard cards', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/helpers/hubDashboard.test.mjs` around lines 82 - 320, The test suite misses regression checks for handling null/undefined array inputs and for ensuring numeric zero values take precedence over aliases in buildRouteManagerHubDashboardSnapshot; add tests that call buildRouteManagerHubDashboardSnapshot with nodes/tasks/schedules/alerts set to null (or omitted) to assert no exceptions and that recent* arrays resolve to empty arrays, and add a test where an alias field (e.g., onlineNodes/busyNodes/pendingTasks) is 0 while a fallback value exists to assert the snapshot preserves the 0 value rather than using the fallback; reference buildRouteManagerHubDashboardSnapshot, and assert properties like recentTasks.length === 0 and onlineNodes === 0 to lock behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/dashboard/index.jsx`:
- Around line 146-148: The effect calling dashboardData.loadHubData on
hubAvailabilitySignature can allow stale responses to overwrite newer state;
update the implementation so requests are cancellable or sequenced: add an
AbortController param or a per-call sequence token (e.g., a requestId stored on
a ref inside the useDashboardData hook) and have dashboardData.loadHubData
accept the signal or capture the requestId and early-return when aborted or when
requestId mismatches; then change the useEffect that calls
dashboardData.loadHubData to create and pass an AbortController (and call
controller.abort() in the cleanup) or increment the requestId so only the latest
pipeline in useDashboardData (lines around loadHubData and its internal
multi-request pipeline) sets state.
- Around line 146-148: The dashboard currently always calls
dashboardData.loadHubData() and always renders the hub panel regardless of the
admin config; wrap both the fetch and the JSX that renders the hub panel with a
check against SidebarModulesAdmin.console.hub (use the same boolean that
controls the sidebar entry) so that dashboardData.loadHubData() is only invoked
inside the useEffect when SidebarModulesAdmin.console.hub is true (in the effect
that depends on hubAvailabilitySignature) and the hub panel JSX (the block
rendering the hub UI around where HubPanel is returned) is conditional on the
same flag.
In `@web/src/components/dashboard/RouteManagerHubPanel.jsx`:
- Around line 120-128: The icon-only refresh Button in RouteManagerHubPanel.jsx
(the <Button ... onClick={loadHubData} loading={hubLoading} ... />) lacks an
accessible label; add an aria-label like "Refresh routes" to the Button
component and also provide a visible tooltip/title attribute for hover (e.g.,
title="Refresh") so assistive tech and sighted users understand the control;
update the Button JSX to include aria-label and title props near the existing
icon/onClick/loading props.
In `@web/src/components/settings/OtherSetting.jsx`:
- Around line 303-306: The code is treating any 200 response as success; change
the API.get('/api/hub/status') handling to check res.data?.success === true
before updating or clearing cached hub status: only call
syncGlobalHubStatus(res.data.data || null) and
setHubStatus(formatRouteManagerHubStatus(res.data.data, t)) when success is
true; if success is false, do not call syncGlobalHubStatus(null) (preserve the
existing cached hub_status) and avoid overwriting hub UI—either leave the prior
hubStatus unchanged or set an explicit error state instead. Ensure you update
the branch around API.get('/api/hub/status') that calls syncGlobalHubStatus,
formatRouteManagerHubStatus, and setHubStatus accordingly.
In `@web/src/helpers/hubDashboard.js`:
- Around line 251-301: The computed metrics and recent lists
(computedOnlineNodes, computedBusyNodes, computedPendingTasks,
computedActiveSchedules, computedCriticalAlerts, computedUnacknowledgedAlerts,
recentTasks, recentSchedules, recentAlerts) call .filter()/.slice() on nodes,
schedules, tasks, and alerts without guarding against null (API may return
null); update the code to ensure each collection is an array before use (e.g.,
replace direct uses of nodes/schedules/tasks/alerts with a safe value like
Array.isArray(nodes) ? nodes : [] or a default []), and keep the existing
mappings (including extractRouteManagerTaskPreview) unchanged so downstream
fields still map as before. Ensure this normalization happens once before the
computed variables are derived.
- Around line 321-323: Replace the falsy-checking || fallbacks for numeric
metric aliases with nullish coalescing so explicit zeros are preserved: update
the assignments for modelCount, aiCapableNodes, onlineAINodes (and similarly the
entityCount block around the later lines) to use ?? between ai.modelCount /
ai.model_count etc., then wrap the final selected value with Number(...) as
before; locate these by the identifiers modelCount, aiCapableNodes,
onlineAINodes and entityCount and change their fallback operators from || to ??.
In `@web/src/helpers/hubNavigation.js`:
- Around line 96-100: The helper getRouteManagerHubTaskHref currently returns
getRouteManagerHubHref() when taskID is invalid, which navigates users to /hub/;
change the fallback to return the tasks list route (use
getRouteManagerHubTasksHref()) so broken/invalid task deep links stay inside the
task center; update the return in getRouteManagerHubTaskHref to call
getRouteManagerHubTasksHref() while keeping the existing normalizedTaskID check.
In `@web/src/helpers/hubStatus.js`:
- Around line 82-95: The current selection logic in resolvedStatusSnapshot
treats any object-shaped statusSnapshot as authoritative even when it lacks
hub_status; change the logic so that statusSnapshot is only used if it is an
object AND has a valid hub_status object, otherwise fall back to
fallbackStatusSnapshot (if it is an object with hub_status); update the code
that computes resolvedStatusSnapshot in web/src/helpers/hubStatus.js to prefer
statusSnapshot only when it contains hub_status, and otherwise assign
fallbackStatusSnapshot (or null if neither have hub_status) — reference the
resolvedStatusSnapshot variable and the statusSnapshot/fallbackStatusSnapshot
and hub_status fields to locate where to adjust the conditional.
In `@web/src/helpers/hubStatusState.test.mjs`:
- Around line 26-83: Update the test expectations to match the helper's return
shape (an object with a status property) for buildStatusSnapshotWithHubStatus
and buildStatusSnapshotWithClearedHubStatus: where the tests currently expect
the raw snapshot or null, change them to expect { status: ... } (e.g., { status:
{ system_name: 'AI Gateway', hub_status: nextHubStatus, docs_link:
'https://docs.example.com' } }) and specifically change the assertion that
currently expects null to expect { status: null } when calling
buildStatusSnapshotWithHubStatus(null, { configured: true }) or
buildStatusSnapshotWithClearedHubStatus(null, savedStatus); adjust the other
expectations that compare savedStatus snapshots to wrap those snapshots in a {
status: ... } object as well.
In `@web/src/helpers/iframeContext.js`:
- Around line 1-16: postIframeContext currently uses
iframeWindow.postMessage(..., '*') which broadcasts theme/lang to any origin;
change it to derive and validate the iframe origin from iframe.src (or
iframe.contentWindow.location if same-origin) and pass that origin as the
targetOrigin to iframeWindow.postMessage. In the postIframeContext function,
compute const targetOrigin = new URL(iframe.src).origin (guard against
missing/invalid src), verify targetOrigin is a valid http(s) origin, and replace
the '*' calls for themeMode and lang with iframeWindow.postMessage({...},
targetOrigin); if origin cannot be resolved or is unsafe, do not post and return
false.
In `@web/src/hooks/dashboard/useDashboardData.js`:
- Around line 383-391: The fetch for /hub/api/dashboard/summary uses
skipErrorHandler: true and currently the failure branch clears hub state
(setHubSummary, setHubNodes, setHubSchedules, setHubTasks, setHubAlerts) but
then unconditionally calls setHubError(''), erasing the error; change the logic
so setHubError('') is only called on successful responses and on failure you set
a meaningful error (e.g., setHubError(response.error || 'Failed to load hub
summary')) so the error state is surfaced; update the code paths around
setHubSummary and setHubError in useDashboardData (referencing setHubSummary,
setHubError and the success:false branch) so failures leave hubError populated
and successes clear it.
In `@web/src/i18n/hubLocaleCoverage.test.mjs`:
- Around line 10-121: The HUB_LOCALE_KEYS array is missing newly added
hub/shortcut strings causing false negatives; update the HUB_LOCALE_KEYS
constant to include the new entries (e.g., "打开 Route Manager Hub 中枢", "中枢总览",
"打开中枢", "主站内可直接查看中枢最近运行情况", "主站内快速切换中枢能力区", "打开新窗口", "页面不存在",
"当前快捷入口不存在，或你没有访问权限。") so the hubLocaleCoverage.test.mjs covers the new Route
Manager Hub and shortcut copy; modify the HUB_LOCALE_KEYS definition to append
these exact strings.
In `@web/src/i18n/locales/en.json`:
- Around line 952-964: The listed count-sensitive keys ("{{count}} 模型",
"{{count}} 在线 AI 节点", "{{count}} 实体", "家庭桥接 {{count}} 实体") currently hard-code
plural English; update each value to use ICU pluralization so singular and
plural render correctly (e.g. replace the plain "{{count}} models" with a plural
string like "{{count, plural, one {# model} other {# models}}}" and similarly
for "AI nodes" and "entities" / "entities in home bridge"); ensure the exact
keys remain unchanged and only their values are replaced with the ICU plural
pattern to preserve lookup behavior.
In `@web/src/i18n/locales/vi.json`:
- Line 964: The JSON contains a duplicate key "激活" with two different Vietnamese
translations; replace the two identical keys with distinct keys (e.g., "激活_状态"
for the status translation "Đang hoạt động" and "激活_操作" for the action
translation "Kích hoạt"), update the JSON entries accordingly, and then update
all code references that use "激活" to reference the new keys ("激活_状态" where the
term denotes a state/status and "激活_操作" where it denotes the activate action) so
each context uses the correct translation.
In `@web/src/i18n/locales/zh-CN.json`:
- Line 1246: The zh-CN locale added the new translation key "缓存命中率" but the
zh-TW locale is missing it; open the zh-TW.json and add the corresponding
"缓存命中率" entry (copy the Simplified text or provide the Traditional Chinese
equivalent) so the key exists in both locale files, then run the i18n sync/lint
pipeline (bun run i18n:extract, bun run i18n:sync, bun run i18n:lint) to
validate and propagate the change; ensure the key name matches exactly "缓存命中率"
in zh-TW.json so consumers fallback is avoided.
In `@web/src/pages/About/aboutPageContent.js`:
- Around line 3-5: isEmbeddableAboutPageURL currently only allows strings
starting with "https://" which rejects "http://", protocol-relative ("//...")
and same-origin root-relative ("/...") URLs causing them to render as HTML
instead of iframes; update isEmbeddableAboutPageURL to return true for strings
that are absolute http/https ("http://" or "https://"), protocol-relative
("//"), or root-relative (start with "/") so local/non-TLS and internal-route
embeds are treated as embeddable; keep the type check (typeof value ===
'string') and trim() usage and update the startsWith checks accordingly in
isEmbeddableAboutPageURL.
In `@web/src/pages/About/index.jsx`:
- Around line 41-49: The iframe only receives context on its onLoad
(handleAboutIframeLoad) so theme/lang updates are never re-posted; add a ref for
the iframe element used by handleAboutIframeLoad (and the similar handler at the
other location) and create a useEffect that watches actualTheme and
i18n.language and calls postIframeContext(iframeRef.current, { themeMode:
actualTheme, lang: i18n.language }) when the ref is present; ensure the same
change is applied for the other iframe load handler referenced in the file so
both iframes get updated context when theme or language changes.
---
Outside diff comments:
In `@web/src/components/layout/SiderBar.jsx`:
- Around line 63-80: The sidebar currently computes routeManagerHubVisible from
statusState?.status?.hub_status which is undefined during initial load; change
the value fed to shouldShowRouteManagerHubEntry to first try
statusState?.status?.hub_status and fall back to the cached snapshot field (e.g.
statusState?.savedHubSnapshot?.hub_status or the existing snapshot property on
StatusContext) so the sidebar uses the saved hub snapshot while status is
loading; update the expression around routeManagerHubVisible to use this
fallback (keep the same shouldShowRouteManagerHubEntry call).
---
Nitpick comments:
In `@web/src/helpers/hubDashboard.test.mjs`:
- Around line 82-320: The test suite misses regression checks for handling
null/undefined array inputs and for ensuring numeric zero values take precedence
over aliases in buildRouteManagerHubDashboardSnapshot; add tests that call
buildRouteManagerHubDashboardSnapshot with nodes/tasks/schedules/alerts set to
null (or omitted) to assert no exceptions and that recent* arrays resolve to
empty arrays, and add a test where an alias field (e.g.,
onlineNodes/busyNodes/pendingTasks) is 0 while a fallback value exists to assert
the snapshot preserves the 0 value rather than using the fallback; reference
buildRouteManagerHubDashboardSnapshot, and assert properties like
recentTasks.length === 0 and onlineNodes === 0 to lock behavior.
In `@web/src/helpers/optionUpdate.js`:
- Around line 1-27: Remove the hardcoded English fallback by deleting the
default values for fallbackMessage in both ensureOptionUpdateSucceeded and
getOptionUpdateErrorMessage so callers must supply a translated fallback; in
ensureOptionUpdateSucceeded keep the same logic to choose message vs
fallbackMessage and throw new Error(normalizedMessage) (assume fallbackMessage
is provided), and in getOptionUpdateErrorMessage return the trimmed
error.message or the provided fallbackMessage (no internal default). Update all
call sites to pass an explicit i18n/translated fallback string when invoking
ensureOptionUpdateSucceeded and getOptionUpdateErrorMessage.
In `@web/src/i18n/locales/fr.json`:
- Around line 947-957: The count-based French translations like "{{count}} 模型",
"{{count}} 在线 AI 节点", and "{{count}} 实体" must be converted to pluralization
entries (e.g., use keys with suffixes such as "_one" and "_other" or your
project's plural convention) and provide singular forms for French ("{{count}}
modèle", "{{count}} nœud IA en ligne", "{{count}} entité") for the singular
variant and plural forms ("{{count}} modèles", "{{count}} nœuds IA en ligne",
"{{count}} entités") for the plural/other variant; update the JSON keys and
values to follow the same pluralization pattern used elsewhere (see existing
_one/_many/_other entries) so the i18n runtime can select singular vs plural.
In `@web/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx`:
- Around line 42-72: Extract the hard-coded sidebar defaults into a single
shared constant (e.g., SIDEBAR_MODULES_DEFAULTS) and import it into
SettingsSidebarModulesAdmin.jsx; replace the inline initial state object passed
to useState (sidebarModulesAdmin) with SIDEBAR_MODULES_DEFAULTS and update the
reset logic and any JSON-merge code in this file (references:
sidebarModulesAdmin, setSidebarModulesAdmin) to use that constant; also update
the other locations mentioned (the other ranges in this file and
web/src/hooks/common/useSidebar.js) to import and reference the same
SIDEBAR_MODULES_DEFAULTS so the admin "hub" and all module defaults remain
consistent across initialization, reset, and runtime checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63979c6c-59f1-4c4e-9681-e6bf1eaf5704
📒 Files selected for processing (36)
web/src/components/dashboard/RouteManagerHubPanel.jsxweb/src/components/dashboard/index.jsxweb/src/components/layout/SiderBar.jsxweb/src/components/settings/OtherSetting.jsxweb/src/helpers/hubAvailability.jsweb/src/helpers/hubAvailability.test.mjsweb/src/helpers/hubDashboard.jsweb/src/helpers/hubDashboard.test.mjsweb/src/helpers/hubNavigation.jsweb/src/helpers/hubNavigation.test.mjsweb/src/helpers/hubStatus.jsweb/src/helpers/hubStatus.test.mjsweb/src/helpers/hubStatusState.jsweb/src/helpers/hubStatusState.test.mjsweb/src/helpers/iframeContext.jsweb/src/helpers/iframeContext.test.mjsweb/src/helpers/index.jsweb/src/helpers/optionUpdate.jsweb/src/helpers/optionUpdate.test.mjsweb/src/hooks/common/useSidebar.jsweb/src/hooks/dashboard/useDashboardData.jsweb/src/i18n/hubLocaleCoverage.test.mjsweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh-CN.jsonweb/src/i18n/locales/zh-TW.jsonweb/src/pages/About/aboutPageContent.jsweb/src/pages/About/aboutPageContent.test.mjsweb/src/pages/About/index.jsxweb/src/pages/Home/homePageContent.jsweb/src/pages/Home/homePageContent.test.mjsweb/src/pages/Home/index.jsxweb/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx
Comment on lines
+146
to
+148
| useEffect(() => { | ||
| void dashboardData.loadHubData(); | ||
| }, [hubAvailabilitySignature]); |
Contributor
There was a problem hiding this comment.
This effect can race stale hub responses back into state.
Every availability change now starts a fresh loadHubData() call, but web/src/hooks/dashboard/useDashboardData.js:286-390 does not cancel or sequence its multi-request pipeline. If an older request resolves after a later “unavailable” update, it can repopulate stale hub nodes/alerts/summary on the dashboard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboard/index.jsx` around lines 146 - 148, The effect
calling dashboardData.loadHubData on hubAvailabilitySignature can allow stale
responses to overwrite newer state; update the implementation so requests are
cancellable or sequenced: add an AbortController param or a per-call sequence
token (e.g., a requestId stored on a ref inside the useDashboardData hook) and
have dashboardData.loadHubData accept the signal or capture the requestId and
early-return when aborted or when requestId mismatches; then change the
useEffect that calls dashboardData.loadHubData to create and pass an
AbortController (and call controller.abort() in the cleanup) or increment the
requestId so only the latest pipeline in useDashboardData (lines around
loadHubData and its internal multi-request pipeline) sets state.
Respect console.hub visibility before fetching or rendering the hub panel.
Line 147 loads hub data and Line 183 renders the panel unconditionally, so disabling SidebarModulesAdmin.console.hub only hides the sidebar entry. The dashboard still exposes the module and its data.
💡 Suggested fix
+import { useSidebar } from '../../hooks/common/useSidebar';
import { useDashboardData } from '../../hooks/dashboard/useDashboardData';
@@
const Dashboard = () => {
+ const { isModuleVisible } = useSidebar();
+ const hubModuleVisible = isModuleVisible('console', 'hub');
+
@@
useEffect(() => {
- void dashboardData.loadHubData();
- }, [hubAvailabilitySignature]);
+ if (!hubModuleVisible) {
+ return;
+ }
+ void dashboardData.loadHubData();
+ }, [hubAvailabilitySignature, hubModuleVisible]);
@@
- <div className='mb-4'>
- <RouteManagerHubPanel
- hubStatus={statusState?.status?.hub_status}
- hubNodes={dashboardData.hubNodes}
- hubSchedules={dashboardData.hubSchedules}
- hubTasks={dashboardData.hubTasks}
- hubAlerts={dashboardData.hubAlerts}
- hubSummary={dashboardData.hubSummary}
- hubLoading={dashboardData.hubLoading}
- hubError={dashboardData.hubError}
- loadHubData={dashboardData.loadHubData}
- CARD_PROPS={CARD_PROPS}
- t={dashboardData.t}
- />
- </div>
+ {hubModuleVisible && (
+ <div className='mb-4'>
+ <RouteManagerHubPanel
+ hubStatus={statusState?.status?.hub_status}
+ hubNodes={dashboardData.hubNodes}
+ hubSchedules={dashboardData.hubSchedules}
+ hubTasks={dashboardData.hubTasks}
+ hubAlerts={dashboardData.hubAlerts}
+ hubSummary={dashboardData.hubSummary}
+ hubLoading={dashboardData.hubLoading}
+ hubError={dashboardData.hubError}
+ loadHubData={dashboardData.loadHubData}
+ CARD_PROPS={CARD_PROPS}
+ t={dashboardData.t}
+ />
+ </div>
+ )}Based on learnings: The sidebar management system introduced in this codebase uses SidebarModulesAdmin configuration to control admin user permissions. Admin access to console.* modules should be governed by this configuration system, not bypassed with hardcoded allowlists.
Also applies to: 182-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboard/index.jsx` around lines 146 - 148, The dashboard
currently always calls dashboardData.loadHubData() and always renders the hub
panel regardless of the admin config; wrap both the fetch and the JSX that
renders the hub panel with a check against SidebarModulesAdmin.console.hub (use
the same boolean that controls the sidebar entry) so that
dashboardData.loadHubData() is only invoked inside the useEffect when
SidebarModulesAdmin.console.hub is true (in the effect that depends on
hubAvailabilitySignature) and the hub panel JSX (the block rendering the hub UI
around where HubPanel is returned) is conditional on the same flag.
Comment on lines
+120
to
+128
| <Button | ||
| icon={<RefreshCw size={14} />} | ||
| onClick={loadHubData} | ||
| loading={hubLoading} | ||
| size='small' | ||
| theme='borderless' | ||
| type='tertiary' | ||
| className='text-gray-500 hover:text-blue-500 hover:bg-blue-50 !rounded-full' | ||
| /> |
Contributor
There was a problem hiding this comment.
Label the icon-only refresh control.
Line 120 renders a button with only the refresh icon, so assistive tech gets an unlabeled control. Add an aria-label (and ideally a tooltip) for the refresh action.
♿ Minimal fix
<Button
+ aria-label={t('刷新')}
icon={<RefreshCw size={14} />}
onClick={loadHubData}
loading={hubLoading}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button | |
| icon={<RefreshCw size={14} />} | |
| onClick={loadHubData} | |
| loading={hubLoading} | |
| size='small' | |
| theme='borderless' | |
| type='tertiary' | |
| className='text-gray-500 hover:text-blue-500 hover:bg-blue-50 !rounded-full' | |
| /> | |
| <Button | |
| aria-label={t('刷新')} | |
| icon={<RefreshCw size={14} />} | |
| onClick={loadHubData} | |
| loading={hubLoading} | |
| size='small' | |
| theme='borderless' | |
| type='tertiary' | |
| className='text-gray-500 hover:text-blue-500 hover:bg-blue-50 !rounded-full' | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboard/RouteManagerHubPanel.jsx` around lines 120 -
128, The icon-only refresh Button in RouteManagerHubPanel.jsx (the <Button ...
onClick={loadHubData} loading={hubLoading} ... />) lacks an accessible label;
add an aria-label like "Refresh routes" to the Button component and also provide
a visible tooltip/title attribute for hover (e.g., title="Refresh") so assistive
tech and sighted users understand the control; update the Button JSX to include
aria-label and title props near the existing icon/onClick/loading props.
Comment on lines
+303
to
+306
| const res = await API.get('/api/hub/status'); | ||
| syncGlobalHubStatus(res.data?.data || null); | ||
| const nextStatus = formatRouteManagerHubStatus(res.data?.data, t); | ||
| setHubStatus(nextStatus); |
Contributor
There was a problem hiding this comment.
Treat success: false from /api/hub/status as a failed check.
Line 303 only distinguishes transport errors. If /api/hub/status returns HTTP 200 with { success: false }, Line 304 clears the cached hub_status and Line 305 formats undefined as “Route Manager 地址未配置”, which is misleading when the URL is still set and also hides hub-dependent UI that reads statusState.status.hub_status.
💡 Proposed fix
}));
const res = await API.get('/api/hub/status');
- syncGlobalHubStatus(res.data?.data || null);
- const nextStatus = formatRouteManagerHubStatus(res.data?.data, t);
+ if (!res.data?.success) {
+ throw new Error(res.data?.message || t('Route Manager 状态检查失败'));
+ }
+ const nextHubStatus = res.data?.data || null;
+ syncGlobalHubStatus(nextHubStatus);
+ const nextStatus = formatRouteManagerHubStatus(nextHubStatus, t);
setHubStatus(nextStatus);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const res = await API.get('/api/hub/status'); | |
| syncGlobalHubStatus(res.data?.data || null); | |
| const nextStatus = formatRouteManagerHubStatus(res.data?.data, t); | |
| setHubStatus(nextStatus); | |
| const res = await API.get('/api/hub/status'); | |
| if (!res.data?.success) { | |
| throw new Error(res.data?.message || t('Route Manager 状态检查失败')); | |
| } | |
| const nextHubStatus = res.data?.data || null; | |
| syncGlobalHubStatus(nextHubStatus); | |
| const nextStatus = formatRouteManagerHubStatus(nextHubStatus, t); | |
| setHubStatus(nextStatus); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/settings/OtherSetting.jsx` around lines 303 - 306, The
code is treating any 200 response as success; change the
API.get('/api/hub/status') handling to check res.data?.success === true before
updating or clearing cached hub status: only call
syncGlobalHubStatus(res.data.data || null) and
setHubStatus(formatRouteManagerHubStatus(res.data.data, t)) when success is
true; if success is false, do not call syncGlobalHubStatus(null) (preserve the
existing cached hub_status) and avoid overwriting hub UI—either leave the prior
hubStatus unchanged or set an explicit error state instead. Ensure you update
the branch around API.get('/api/hub/status') that calls syncGlobalHubStatus,
formatRouteManagerHubStatus, and setHubStatus accordingly.
Comment on lines
+251
to
+301
| const computedOnlineNodes = nodes.filter( | ||
| (node) => node?.status === 'online', | ||
| ).length; | ||
| const computedBusyNodes = nodes.filter((node) => { | ||
| const browserRuntime = node?.browser_runtime; | ||
| return ( | ||
| (browserRuntime?.active_tasks || 0) > 0 || | ||
| (browserRuntime?.pending_tasks || 0) > 0 | ||
| ); | ||
| }).length; | ||
| const computedPendingTasks = tasks.filter( | ||
| (task) => task?.status === 'pending', | ||
| ).length; | ||
| const computedActiveSchedules = schedules.filter( | ||
| (schedule) => schedule?.enabled, | ||
| ).length; | ||
| const computedCriticalAlerts = alerts.filter( | ||
| (alert) => alert?.severity === 'critical', | ||
| ).length; | ||
| const computedUnacknowledgedAlerts = alerts.filter( | ||
| (alert) => !alert?.acknowledged, | ||
| ).length; | ||
|
|
||
| const recentTasks = tasks.slice(0, 3).map((task) => ({ | ||
| id: task?.id, | ||
| type: task?.type || '', | ||
| status: task?.status || 'unknown', | ||
| targetNodeID: task?.target_node_id || '', | ||
| preview: extractRouteManagerTaskPreview(task), | ||
| })); | ||
| const recentSchedules = schedules.slice(0, 3).map((schedule) => ({ | ||
| id: schedule?.id, | ||
| name: schedule?.name || '', | ||
| type: schedule?.type || '', | ||
| targetNodeID: schedule?.target_node_id || '', | ||
| dailyAt: schedule?.daily_at || '', | ||
| enabled: Boolean(schedule?.enabled), | ||
| nextRunAt: schedule?.next_run_at || '', | ||
| })); | ||
| const recentAlerts = alerts.slice(0, 3).map((alert) => ({ | ||
| id: alert?.id || '', | ||
| kind: alert?.kind || '', | ||
| severity: alert?.severity || 'unknown', | ||
| title: alert?.title || '', | ||
| subtitle: alert?.subtitle || '', | ||
| preview: alert?.preview || '', | ||
| metaText: alert?.metaText || alert?.meta_text || '', | ||
| targetNodeID: alert?.targetNodeID || alert?.target_node_id || '', | ||
| acknowledged: Boolean(alert?.acknowledged), | ||
| })); | ||
|
|
Contributor
There was a problem hiding this comment.
Guard collection inputs before calling .filter() / .slice().
nodes, schedules, tasks, and alerts only default when undefined; if API returns null, this path throws and breaks the hub panel render.
💡 Proposed fix
export function buildRouteManagerHubDashboardSnapshot({
@@
} = {}) {
+ const normalizedNodes = Array.isArray(nodes) ? nodes : [];
+ const normalizedSchedules = Array.isArray(schedules) ? schedules : [];
+ const normalizedTasks = Array.isArray(tasks) ? tasks : [];
+ const normalizedAlerts = Array.isArray(alerts) ? alerts : [];
+
@@
- const computedOnlineNodes = nodes.filter(
+ const computedOnlineNodes = normalizedNodes.filter(
(node) => node?.status === 'online',
).length;
- const computedBusyNodes = nodes.filter((node) => {
+ const computedBusyNodes = normalizedNodes.filter((node) => {
@@
- const computedPendingTasks = tasks.filter(
+ const computedPendingTasks = normalizedTasks.filter(
(task) => task?.status === 'pending',
).length;
- const computedActiveSchedules = schedules.filter(
+ const computedActiveSchedules = normalizedSchedules.filter(
(schedule) => schedule?.enabled,
).length;
- const computedCriticalAlerts = alerts.filter(
+ const computedCriticalAlerts = normalizedAlerts.filter(
(alert) => alert?.severity === 'critical',
).length;
- const computedUnacknowledgedAlerts = alerts.filter(
+ const computedUnacknowledgedAlerts = normalizedAlerts.filter(
(alert) => !alert?.acknowledged,
).length;
@@
- const recentTasks = tasks.slice(0, 3).map((task) => ({
+ const recentTasks = normalizedTasks.slice(0, 3).map((task) => ({
@@
- const recentSchedules = schedules.slice(0, 3).map((schedule) => ({
+ const recentSchedules = normalizedSchedules.slice(0, 3).map((schedule) => ({
@@
- const recentAlerts = alerts.slice(0, 3).map((alert) => ({
+ const recentAlerts = normalizedAlerts.slice(0, 3).map((alert) => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/helpers/hubDashboard.js` around lines 251 - 301, The computed metrics
and recent lists (computedOnlineNodes, computedBusyNodes, computedPendingTasks,
computedActiveSchedules, computedCriticalAlerts, computedUnacknowledgedAlerts,
recentTasks, recentSchedules, recentAlerts) call .filter()/.slice() on nodes,
schedules, tasks, and alerts without guarding against null (API may return
null); update the code to ensure each collection is an array before use (e.g.,
replace direct uses of nodes/schedules/tasks/alerts with a safe value like
Array.isArray(nodes) ? nodes : [] or a default []), and keep the existing
mappings (including extractRouteManagerTaskPreview) unchanged so downstream
fields still map as before. Ensure this normalization happens once before the
computed variables are derived.
Comment on lines
+321
to
+323
| modelCount: Number(ai?.modelCount || ai?.model_count || 0), | ||
| aiCapableNodes: Number(ai?.aiCapableNodes || ai?.ai_capable_nodes || 0), | ||
| onlineAINodes: Number(ai?.onlineAINodes || ai?.online_ai_nodes || 0), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all numeric alias merges in this helper that currently use `||`.
# Expected: lines in this file showing falsy-coalescing on numeric aliases.
rg -n -C2 --type=js 'modelCount:\s*Number\(|aiCapableNodes:\s*Number\(|onlineAINodes:\s*Number\(|entityCount:\s*Number\(' web/src/helpers/hubDashboard.js
rg -n -C2 --type=js '\|\|\s*ai\?\.model_count|\|\|\s*ai\?\.ai_capable_nodes|\|\|\s*ai\?\.online_ai_nodes|\|\|\s*normalizedHomeAssistant\?\.entity_count' web/src/helpers/hubDashboard.jsRepository: QuantumNous/new-api
Length of output: 908
🏁 Script executed:
# Search for all other numeric coalescing patterns in the file
rg -n 'Number\(' web/src/helpers/hubDashboard.js | head -20Repository: QuantumNous/new-api
Length of output: 539
Use nullish coalescing for numeric alias fields to preserve explicit 0 values.
Using || operator treats 0 as falsy and incorrectly triggers fallback to alternate field names, causing data loss when numeric counters are explicitly set to zero.
Apply the fix at lines 321-323 (ai metrics) and 353-356 (entityCount):
Proposed changes
ai: {
- modelCount: Number(ai?.modelCount || ai?.model_count || 0),
- aiCapableNodes: Number(ai?.aiCapableNodes || ai?.ai_capable_nodes || 0),
- onlineAINodes: Number(ai?.onlineAINodes || ai?.online_ai_nodes || 0),
+ modelCount: Number(ai?.modelCount ?? ai?.model_count ?? 0),
+ aiCapableNodes: Number(ai?.aiCapableNodes ?? ai?.ai_capable_nodes ?? 0),
+ onlineAINodes: Number(ai?.onlineAINodes ?? ai?.online_ai_nodes ?? 0),
},
entityCount: Number(
- normalizedHomeAssistant?.entityCount ||
- normalizedHomeAssistant?.entity_count ||
- 0,
+ normalizedHomeAssistant?.entityCount ??
+ normalizedHomeAssistant?.entity_count ??
+ 0,
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| modelCount: Number(ai?.modelCount || ai?.model_count || 0), | |
| aiCapableNodes: Number(ai?.aiCapableNodes || ai?.ai_capable_nodes || 0), | |
| onlineAINodes: Number(ai?.onlineAINodes || ai?.online_ai_nodes || 0), | |
| modelCount: Number(ai?.modelCount ?? ai?.model_count ?? 0), | |
| aiCapableNodes: Number(ai?.aiCapableNodes ?? ai?.ai_capable_nodes ?? 0), | |
| onlineAINodes: Number(ai?.onlineAINodes ?? ai?.online_ai_nodes ?? 0), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/helpers/hubDashboard.js` around lines 321 - 323, Replace the
falsy-checking || fallbacks for numeric metric aliases with nullish coalescing
so explicit zeros are preserved: update the assignments for modelCount,
aiCapableNodes, onlineAINodes (and similarly the entityCount block around the
later lines) to use ?? between ai.modelCount / ai.model_count etc., then wrap
the final selected value with Number(...) as before; locate these by the
identifiers modelCount, aiCapableNodes, onlineAINodes and entityCount and change
their fallback operators from || to ??.
Comment on lines
+952
to
+964
| "{{count}} 模型": "{{count}} models", | ||
| "{{count}} 在线 AI 节点": "{{count}} AI nodes online", | ||
| "净网已开启": "Clean network enabled", | ||
| "净网待确认": "Clean network pending", | ||
| "代理可达": "Proxy reachable", | ||
| "代理待确认": "Proxy pending", | ||
| "DNS 可达": "DNS reachable", | ||
| "DNS 待确认": "DNS pending", | ||
| "桥接在线": "Bridge online", | ||
| "桥接待确认": "Bridge pending", | ||
| "{{count}} 实体": "{{count}} entities", | ||
| "主算力 {{name}}": "Primary compute {{name}}", | ||
| "家庭桥接 {{count}} 实体": "Home bridge {{count}} entities", |
Contributor
There was a problem hiding this comment.
Add singular variants for the new count-sensitive labels.
Lines 952, 953, 962, and 964 hard-code plural English, so a value of 1 will render as 1 models, 1 AI nodes online, or 1 entities. The locale already uses separate singular/plural entries elsewhere, so these new labels need the same treatment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/i18n/locales/en.json` around lines 952 - 964, The listed
count-sensitive keys ("{{count}} 模型", "{{count}} 在线 AI 节点", "{{count}} 实体",
"家庭桥接 {{count}} 实体") currently hard-code plural English; update each value to
use ICU pluralization so singular and plural render correctly (e.g. replace the
plain "{{count}} models" with a plural string like "{{count, plural, one {#
model} other {# models}}}" and similarly for "AI nodes" and "entities" /
"entities in home bridge"); ensure the exact keys remain unchanged and only
their values are replaced with the ICU plural pattern to preserve lookup
behavior.
| "暂无中枢告警": "Chưa có cảnh báo hub", | ||
| "中枢连接正常后，这里会显示最近告警和失败原因入口": "Khi hub kết nối thành công, các cảnh báo gần đây và lối vào nguyên nhân lỗi sẽ hiển thị tại đây", | ||
| "关键计划": "Lịch chính", | ||
| "激活": "Đang hoạt động", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all occurrences of the key "激活" in the Vietnamese locale file
rg -n '"激活"' web/src/i18n/locales/vi.jsonRepository: QuantumNous/new-api
Length of output: 122
Duplicate key "激活" detected at lines 964 and 2222.
The key "激活" appears twice in the file with different Vietnamese translations:
- Line 964:
"Đang hoạt động"(status/state: "being active") - Line 2222:
"Kích hoạt"(action: "to activate")
In JSON, duplicate keys cause the last occurrence to overwrite previous values. Since these represent different contexts, use distinct keys instead:
"激活_状态"for "Đang hoạt động" (status context)"激活_操作"for "Kích hoạt" (action context)
Then update all component usages to reference the appropriate key.
🧰 Tools
🪛 Biome (2.4.9)
[error] 964-964: The key 激活 was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/i18n/locales/vi.json` at line 964, The JSON contains a duplicate key
"激活" with two different Vietnamese translations; replace the two identical keys
with distinct keys (e.g., "激活_状态" for the status translation "Đang hoạt động"
and "激活_操作" for the action translation "Kích hoạt"), update the JSON entries
accordingly, and then update all code references that use "激活" to reference the
new keys ("激活_状态" where the term denotes a state/status and "激活_操作" where it
denotes the activate action) so each context uses the correct translation.
| "提示：链接中的{key}将被替换为API密钥，{address}将被替换为服务器地址": "提示：链接中的{key}将被替换为API密钥，{address}将被替换为服务器地址", | ||
| "提示价格：{{symbol}}{{price}} / 1M tokens": "提示价格：{{symbol}}{{price}} / 1M tokens", | ||
| "提示缓存倍率": "提示缓存倍率", | ||
| "缓存命中率": "缓存命中率", |
Contributor
There was a problem hiding this comment.
Sync 缓存命中率 into zh-TW.json.
This new source key is present here, but the Traditional Chinese locale in this PR still lacks it, so zh-TW users will fall back to the Simplified Chinese label.
As per coding guidelines "Use CLI tools: bun run i18n:extract, bun run i18n:sync, bun run i18n:lint."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/i18n/locales/zh-CN.json` at line 1246, The zh-CN locale added the new
translation key "缓存命中率" but the zh-TW locale is missing it; open the zh-TW.json
and add the corresponding "缓存命中率" entry (copy the Simplified text or provide the
Traditional Chinese equivalent) so the key exists in both locale files, then run
the i18n sync/lint pipeline (bun run i18n:extract, bun run i18n:sync, bun run
i18n:lint) to validate and propagate the change; ensure the key name matches
exactly "缓存命中率" in zh-TW.json so consumers fallback is avoided.
Comment on lines
+3
to
+5
| export function isEmbeddableAboutPageURL(value) { | ||
| return typeof value === 'string' && value.trim().startsWith('https://'); | ||
| } |
Contributor
There was a problem hiding this comment.
About-page embeds only accept https:// right now.
Line 4 rejects http://... and same-origin /... URLs, so Line 23 sends them through marked.parse() and web/src/pages/About/index.jsx:174-188 renders them as HTML instead of an iframe. That makes About-page embeds behave differently from Home and breaks local/non-TLS or internal-route embeds.
💡 Suggested fix
export function isEmbeddableAboutPageURL(value) {
- return typeof value === 'string' && value.trim().startsWith('https://');
+ if (typeof value !== 'string') {
+ return false;
+ }
+
+ const trimmed = value.trim();
+ if (!trimmed) {
+ return false;
+ }
+
+ return (
+ trimmed.startsWith('https://') ||
+ trimmed.startsWith('http://') ||
+ trimmed.startsWith('/')
+ );
}Also applies to: 22-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/About/aboutPageContent.js` around lines 3 - 5,
isEmbeddableAboutPageURL currently only allows strings starting with "https://"
which rejects "http://", protocol-relative ("//...") and same-origin
root-relative ("/...") URLs causing them to render as HTML instead of iframes;
update isEmbeddableAboutPageURL to return true for strings that are absolute
http/https ("http://" or "https://"), protocol-relative ("//"), or root-relative
(start with "/") so local/non-TLS and internal-route embeds are treated as
embeddable; keep the type check (typeof value === 'string') and trim() usage and
update the startsWith checks accordingly in isEmbeddableAboutPageURL.
Comment on lines
+41
to
+49
| const handleAboutIframeLoad = useCallback( | ||
| (event) => { | ||
| postIframeContext(event.currentTarget, { | ||
| themeMode: actualTheme, | ||
| lang: i18n.language, | ||
| }); | ||
| }, | ||
| [actualTheme, i18n.language], | ||
| ); |
Contributor
There was a problem hiding this comment.
Re-post iframe context when theme or language changes.
onLoad only fires once. If the user toggles dark mode or switches locale while the iframe stays mounted, the embedded page never receives the updated themeMode / lang values and remains stale until a full reload.
💡 Suggested fix
-import React, { useCallback, useEffect, useState } from 'react';
+import React, { useCallback, useEffect, useRef, useState } from 'react';
...
const [about, setAbout] = useState('');
const [aboutLoaded, setAboutLoaded] = useState(false);
const currentYear = new Date().getFullYear();
const actualTheme = useActualTheme();
+ const aboutIframeRef = useRef(null);
const handleAboutIframeLoad = useCallback(
(event) => {
postIframeContext(event.currentTarget, {
themeMode: actualTheme,
lang: i18n.language,
});
},
[actualTheme, i18n.language],
);
+
+ useEffect(() => {
+ postIframeContext(aboutIframeRef.current, {
+ themeMode: actualTheme,
+ lang: i18n.language,
+ });
+ }, [actualTheme, i18n.language]);
...
<iframe
+ ref={aboutIframeRef}
src={about}
style={{ width: '100%', height: '100vh', border: 'none' }}
onLoad={handleAboutIframeLoad}
/>Also applies to: 176-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/About/index.jsx` around lines 41 - 49, The iframe only receives
context on its onLoad (handleAboutIframeLoad) so theme/lang updates are never
re-posted; add a ref for the iframe element used by handleAboutIframeLoad (and
the similar handler at the other location) and create a useEffect that watches
actualTheme and i18n.language and calls postIframeContext(iframeRef.current, {
themeMode: actualTheme, lang: i18n.language }) when the ref is present; ensure
the same change is applied for the other iframe load handler referenced in the
file so both iframes get updated context when theme or language changes.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
web/src/pages/About/aboutPageContent.js (2)
14-19:⚠️ Potential issue | 🟡 MinorDo not reuse fallback content as the error message.
Lines 17, 25, and 41 still set
errorMessagefromfallbackContent, which can surface stale HTML/iframe strings in UI error handling.💡 Suggested fix
export async function loadAboutPageContent( requestAboutContent, fallbackContent = '', + fallbackErrorMessage = '', ) { try { const { success, message, data } = await requestAboutContent(); if (!success) { return { content: fallbackContent, - errorMessage: message || fallbackContent, + errorMessage: message || fallbackErrorMessage, shouldPersist: false, }; } if (typeof data !== 'string') { return { content: fallbackContent, - errorMessage: message || fallbackContent, + errorMessage: message || fallbackErrorMessage, shouldPersist: false, }; } @@ - } catch { + } catch (error) { return { content: fallbackContent, - errorMessage: fallbackContent, + errorMessage: + error?.response?.data?.message || fallbackErrorMessage, shouldPersist: false, }; } }Also applies to: 22-27, 38-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/About/aboutPageContent.js` around lines 14 - 19, The errorMessage field must not reuse fallbackContent (which can contain stale HTML/iframe strings); locate the return objects that set errorMessage: message || fallbackContent (references: fallbackContent, errorMessage, message, shouldPersist) and change them to use message or a safe non-HTML default (e.g., message || 'Content failed to load' or message || null) so errorMessage never contains fallbackContent; update all occurrences where errorMessage currently falls back to fallbackContent.
3-5:⚠️ Potential issue | 🟠 MajorBroaden embeddable URL detection to avoid misrouting valid iframe sources.
Line 4 still only accepts
https://, sohttp://,//..., and root-relative/...values will be parsed as markdown instead of embedded.💡 Suggested fix
export function isEmbeddableAboutPageURL(value) { - return typeof value === 'string' && value.trim().startsWith('https://'); + if (typeof value !== 'string') { + return false; + } + + const trimmed = value.trim(); + if (!trimmed) { + return false; + } + + return ( + trimmed.startsWith('https://') || + trimmed.startsWith('http://') || + trimmed.startsWith('//') || + trimmed.startsWith('/') + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/About/aboutPageContent.js` around lines 3 - 5, The current isEmbeddableAboutPageURL function only allows 'https://' and incorrectly treats other valid iframe sources as markdown; update isEmbeddableAboutPageURL to accept strings that start with 'https://', 'http://', protocol-relative '//' and root-relative '/' (i.e., trim the value and check startsWith for each of these prefixes) so http, protocol-relative and root-relative URLs are treated as embeddable iframe sources.
🧹 Nitpick comments (1)
web/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx (1)
46-48: Consider simplifying redundant merge calls.The calls to
mergeAdminConfig(DEFAULT_ADMIN_CONFIG)on lines 47 and 80 are technically redundant sincemergeAdminConfigalready clonesDEFAULT_ADMIN_CONFIGinternally before merging. Passing the default as the saved config just spreads defaults over themselves.Line 124 (
mergeAdminConfig(modules)) is the correct usage pattern for merging saved config with defaults.♻️ Optional simplification
- const [sidebarModulesAdmin, setSidebarModulesAdmin] = useState(() => - mergeAdminConfig(DEFAULT_ADMIN_CONFIG), - ); + const [sidebarModulesAdmin, setSidebarModulesAdmin] = useState( + () => mergeAdminConfig({}), + );Or extract a helper for the default case:
// In useSidebar.js or locally export const getDefaultAdminConfig = () => mergeAdminConfig({});Also applies to: 80-80, 124-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx` around lines 46 - 48, The code calls mergeAdminConfig(DEFAULT_ADMIN_CONFIG) redundantly when initializing and resetting state; change those calls to mergeAdminConfig({}) or use a small helper like getDefaultAdminConfig that returns mergeAdminConfig({}) so you only merge defaults once and avoid spreading the same defaults over themselves—update the initializer for sidebarModulesAdmin (useState), the reset/update location that currently passes DEFAULT_ADMIN_CONFIG, and keep mergeAdminConfig(modules) when merging saved configs intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/src/pages/About/aboutPageContent.js`:
- Around line 14-19: The errorMessage field must not reuse fallbackContent
(which can contain stale HTML/iframe strings); locate the return objects that
set errorMessage: message || fallbackContent (references: fallbackContent,
errorMessage, message, shouldPersist) and change them to use message or a safe
non-HTML default (e.g., message || 'Content failed to load' or message || null)
so errorMessage never contains fallbackContent; update all occurrences where
errorMessage currently falls back to fallbackContent.
- Around line 3-5: The current isEmbeddableAboutPageURL function only allows
'https://' and incorrectly treats other valid iframe sources as markdown; update
isEmbeddableAboutPageURL to accept strings that start with 'https://',
'http://', protocol-relative '//' and root-relative '/' (i.e., trim the value
and check startsWith for each of these prefixes) so http, protocol-relative and
root-relative URLs are treated as embeddable iframe sources.
---
Nitpick comments:
In `@web/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx`:
- Around line 46-48: The code calls mergeAdminConfig(DEFAULT_ADMIN_CONFIG)
redundantly when initializing and resetting state; change those calls to
mergeAdminConfig({}) or use a small helper like getDefaultAdminConfig that
returns mergeAdminConfig({}) so you only merge defaults once and avoid spreading
the same defaults over themselves—update the initializer for sidebarModulesAdmin
(useState), the reset/update location that currently passes
DEFAULT_ADMIN_CONFIG, and keep mergeAdminConfig(modules) when merging saved
configs intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b5eb115-3f37-4e7f-8f5b-a09d15788e07
📒 Files selected for processing (7)
web/src/pages/About/aboutPageContent.jsweb/src/pages/About/aboutPageContent.test.mjsweb/src/pages/About/index.jsxweb/src/pages/Home/homePageContent.jsweb/src/pages/Home/homePageContent.test.mjsweb/src/pages/Home/index.jsxweb/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx
✅ Files skipped from review due to trivial changes (2)
- web/src/pages/About/aboutPageContent.test.mjs
- web/src/pages/About/index.jsx
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/pages/Home/homePageContent.test.mjs
- web/src/pages/Home/homePageContent.js
- web/src/pages/Home/index.jsx
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
web/src/components/dashboard/RouteManagerHubPanel.jsx (2)
120-128:⚠️ Potential issue | 🟡 MinorLabel the icon-only refresh button.
This control is still announced as an unlabeled button by assistive tech. Add an
aria-label, and ideally atitle, using the existing refresh copy.♿ Minimal fix
<Button + aria-label={t('刷新')} + title={t('刷新')} icon={<RefreshCw size={14} />} onClick={loadHubData} loading={hubLoading}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboard/RouteManagerHubPanel.jsx` around lines 120 - 128, The refresh Button (the icon-only control using the RefreshCw icon, onClick={loadHubData}, loading={hubLoading}) is missing accessible labeling; add an aria-label (e.g., "Refresh") and a title attribute with the same text to the Button component so screen readers and hover tooltips announce its purpose — update the Button props to include aria-label="Refresh" and title="Refresh".
222-241:⚠️ Potential issue | 🟠 MajorRender node rows from normalized data too.
This is the only panel still reading raw
hubNodesandnode.node_iddirectly. If/hub/api/dashboard/summarycomes back camelCase or mixed-case again, the node links/text blank out here while alerts, schedules, and tasks keep working off normalized snapshot data. Reuse a normalized node list in this block as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboard/RouteManagerHubPanel.jsx` around lines 222 - 241, The node rows currently render from raw hubNodes and access node.node_id and node.hostname directly; change this block to render from the normalized node list used elsewhere (the same normalized snapshot that alerts/schedules/tasks use) instead of hubNodes.slice(0,3). Replace occurrences of node.node_id/node.hostname with the normalized keys (e.g., nodeId or node.node_id depending on your normalizer) and pass that normalized id into getRouteManagerHubNodeHref, NODE_STATUS_COLOR lookup, and formatRouteManagerHubNodeStatus so the link, label and tag consistently use the normalized node objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/hooks/dashboard/useDashboardData.js`:
- Around line 293-300: The early return in loadHubData when hubConfigured ||
hubReachable is false prevents manual retries; change loadHubData to accept a
boolean parameter (e.g., forceRefresh = false) and when forceRefresh is true
skip the guard that returns early (so you still clear state but proceed to
attempt requests). Update the caller wired to the refresh button
(RouteManagerHubPanel.jsx) to call loadHubData(true) for manual refreshes; keep
existing callers defaulting to loadHubData() so they preserve current behavior.
Ensure the function signature and internal checks reference hubConfigured and
hubReachable but short-circuit only when forceRefresh is false.
- Around line 303-367: This fetch needs to ignore late responses when hub
config/status changes: before calling API.get in the block around
setHubLoading(true) / API.get('/hub/api/dashboard/summary'), capture a request
token (e.g., incrementing ref like dashboardReqIdRef.current++) or create an
AbortController and store it in a ref, then attach the token/abort to the
request; when the response returns (both in the success branch and catch),
verify the token matches the latest ref (or that the controller was not aborted)
before calling setHubSummary, setHubNodes, setHubSchedules, setHubTasks,
setHubAlerts, setHubError or setHubLoading; also ensure you abort or bump the
token whenever hub URL / hub_status changes (cleanup in the effect that triggers
this fetch) so stale responses are discarded.
---
Duplicate comments:
In `@web/src/components/dashboard/RouteManagerHubPanel.jsx`:
- Around line 120-128: The refresh Button (the icon-only control using the
RefreshCw icon, onClick={loadHubData}, loading={hubLoading}) is missing
accessible labeling; add an aria-label (e.g., "Refresh") and a title attribute
with the same text to the Button component so screen readers and hover tooltips
announce its purpose — update the Button props to include aria-label="Refresh"
and title="Refresh".
- Around line 222-241: The node rows currently render from raw hubNodes and
access node.node_id and node.hostname directly; change this block to render from
the normalized node list used elsewhere (the same normalized snapshot that
alerts/schedules/tasks use) instead of hubNodes.slice(0,3). Replace occurrences
of node.node_id/node.hostname with the normalized keys (e.g., nodeId or
node.node_id depending on your normalizer) and pass that normalized id into
getRouteManagerHubNodeHref, NODE_STATUS_COLOR lookup, and
formatRouteManagerHubNodeStatus so the link, label and tag consistently use the
normalized node objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 517c4ef6-e2ab-4360-8f33-ab1608aa6b33
📒 Files selected for processing (13)
web/src/components/dashboard/RouteManagerHubPanel.jsxweb/src/helpers/hubDashboard.jsweb/src/helpers/hubDashboard.test.mjsweb/src/helpers/optionUpdate.jsweb/src/hooks/dashboard/useDashboardData.jsweb/src/i18n/hubLocaleCoverage.test.mjsweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh-CN.jsonweb/src/i18n/locales/zh-TW.json
✅ Files skipped from review due to trivial changes (5)
- web/src/i18n/hubLocaleCoverage.test.mjs
- web/src/i18n/locales/zh-TW.json
- web/src/helpers/hubDashboard.test.mjs
- web/src/helpers/hubDashboard.js
- web/src/i18n/locales/ja.json
🚧 Files skipped from review as they are similar to previous changes (5)
- web/src/helpers/optionUpdate.js
- web/src/i18n/locales/zh-CN.json
- web/src/i18n/locales/fr.json
- web/src/i18n/locales/vi.json
- web/src/i18n/locales/en.json
Comment on lines
+293
to
+300
| if (!hubConfigured || !hubReachable) { | ||
| setHubNodes([]); | ||
| setHubSchedules([]); | ||
| setHubTasks([]); | ||
| setHubAlerts([]); | ||
| setHubSummary(createEmptyHubSummary()); | ||
| setHubError(''); | ||
| return; |
Contributor
There was a problem hiding this comment.
Don't make Hub refresh a no-op when reachability is stale.
Line 293 returns before any request whenever cached hub_status.reachable is false. Because RouteManagerHubPanel.jsx wires its refresh button straight to loadHubData, users cannot retry after a transient outage or a stale /api/hub/status result; the panel only recovers if some other code updates reachability first. At minimum, the manual refresh path should bypass this gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/hooks/dashboard/useDashboardData.js` around lines 293 - 300, The
early return in loadHubData when hubConfigured || hubReachable is false prevents
manual retries; change loadHubData to accept a boolean parameter (e.g.,
forceRefresh = false) and when forceRefresh is true skip the guard that returns
early (so you still clear state but proceed to attempt requests). Update the
caller wired to the refresh button (RouteManagerHubPanel.jsx) to call
loadHubData(true) for manual refreshes; keep existing callers defaulting to
loadHubData() so they preserve current behavior. Ensure the function signature
and internal checks reference hubConfigured and hubReachable but short-circuit
only when forceRefresh is false.
Comment on lines
+303
to
+367
| setHubLoading(true); | ||
| try { | ||
| const summaryRes = await API.get('/hub/api/dashboard/summary', { | ||
| skipErrorHandler: true, | ||
| }); | ||
|
|
||
| if (summaryRes.data?.success) { | ||
| const snapshot = buildRouteManagerHubDashboardSnapshot( | ||
| summaryRes.data?.data || {}, | ||
| ); | ||
| setHubSummary({ | ||
| onlineNodes: snapshot.onlineNodes, | ||
| busyNodes: snapshot.busyNodes, | ||
| pendingTasks: snapshot.pendingTasks, | ||
| activeSchedules: snapshot.activeSchedules, | ||
| criticalAlerts: snapshot.criticalAlerts, | ||
| unacknowledgedAlerts: snapshot.unacknowledgedAlerts, | ||
| ai: snapshot.ai, | ||
| network: snapshot.network, | ||
| homeAssistant: snapshot.homeAssistant, | ||
| primaryNode: snapshot.primaryNode, | ||
| }); | ||
| setHubNodes( | ||
| Array.isArray(summaryRes.data?.data?.nodes) | ||
| ? summaryRes.data.data.nodes | ||
| : [], | ||
| ); | ||
| setHubSchedules( | ||
| Array.isArray(summaryRes.data?.data?.schedules) | ||
| ? summaryRes.data.data.schedules | ||
| : [], | ||
| ); | ||
| setHubTasks( | ||
| Array.isArray(summaryRes.data?.data?.tasks) | ||
| ? summaryRes.data.data.tasks | ||
| : [], | ||
| ); | ||
| setHubAlerts( | ||
| Array.isArray(summaryRes.data?.data?.alerts) | ||
| ? summaryRes.data.data.alerts | ||
| : [], | ||
| ); | ||
| } else { | ||
| setHubSummary(createEmptyHubSummary()); | ||
| setHubNodes([]); | ||
| setHubSchedules([]); | ||
| setHubTasks([]); | ||
| setHubAlerts([]); | ||
| setHubError( | ||
| summaryRes.data?.message || t('家域中枢摘要加载失败，请稍后重试'), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| setHubError(''); | ||
| } catch (error) { | ||
| console.error('加载家域中枢摘要失败', error); | ||
| setHubSummary(createEmptyHubSummary()); | ||
| setHubNodes([]); | ||
| setHubSchedules([]); | ||
| setHubTasks([]); | ||
| setHubAlerts([]); | ||
| setHubError(t('家域中枢摘要加载失败，请稍后重试')); | ||
| } finally { | ||
| setHubLoading(false); |
Contributor
There was a problem hiding this comment.
Discard late Hub summary responses after status/config changes.
This request has no sequencing or cancellation. If a fetch starts while the hub is configured, then the Route Manager URL is cleared or hub_status flips before the response comes back, the older response can still repopulate hubSummary and the list state with stale data after a newer render already cleared them.
💡 Minimal direction
+ const hubRequestSeqRef = useRef(0);
...
const loadHubData = useCallback(async () => {
+ const requestSeq = ++hubRequestSeqRef.current;
+
setHubLoading(true);
try {
const summaryRes = await API.get('/hub/api/dashboard/summary', {
skipErrorHandler: true,
});
+ if (requestSeq !== hubRequestSeqRef.current) return;
if (summaryRes.data?.success) {
...
} else {
...
return;
}
setHubError('');
} catch (error) {
+ if (requestSeq !== hubRequestSeqRef.current) return;
...
} finally {
- setHubLoading(false);
+ if (requestSeq === hubRequestSeqRef.current) {
+ setHubLoading(false);
+ }
}
}, [hubConfigured, hubReachable, t]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/hooks/dashboard/useDashboardData.js` around lines 303 - 367, This
fetch needs to ignore late responses when hub config/status changes: before
calling API.get in the block around setHubLoading(true) /
API.get('/hub/api/dashboard/summary'), capture a request token (e.g.,
incrementing ref like dashboardReqIdRef.current++) or create an AbortController
and store it in a ref, then attach the token/abort to the request; when the
response returns (both in the success branch and catch), verify the token
matches the latest ref (or that the controller was not aborted) before calling
setHubSummary, setHubNodes, setHubSchedules, setHubTasks, setHubAlerts,
setHubError or setHubLoading; also ensure you abort or bump the token whenever
hub URL / hub_status changes (cleanup in the effect that triggers this fetch) so
stale responses are discarded.
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
new-apishadow integration/api/hub/statusand Route Manager URL state back into cached status so settings and navigation can reuse the latest hub availabilityRouteManagerURLno longer preserves a staleconfigured=truefallbackTest Plan
cd /Users/luyuyuan/.config/superpowers/worktrees/new-api/shadow-hub-integration/web && bun test src/helpers/optionUpdate.test.mjs src/helpers/hubStatusState.test.mjs src/helpers/hubAvailability.test.mjs src/helpers/hubDashboard.test.mjs src/helpers/hubNavigation.test.mjs src/helpers/hubStatus.test.mjs src/helpers/iframeContext.test.mjs src/pages/Home/homePageContent.test.mjs src/pages/About/aboutPageContent.test.mjs src/i18n/hubLocaleCoverage.test.mjs98 pass / 0 failcd /Users/luyuyuan/.config/superpowers/worktrees/new-api/shadow-hub-integration/web && npm run buildcd /Users/luyuyuan/Desktop/开发/api聚合/route-manager && go test ./... -count=1curl -sS http://localhost:13000/api/hub/statuscurl -sS http://localhost:13000/hub/api/dashboard/summarycurl -sS http://localhost:19080/api/healthSummary by CodeRabbit
New Features
Documentation