fix: enhance NetworkChart with user login state and period selection#54
fix: enhance NetworkChart with user login state and period selection#54hamster1963 merged 3 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughIntroduces login-aware, period-based monitoring to NetworkChart: adds MonitorPeriod type and TIME_RANGE_OPTIONS; fetchLoginUser and fetchMonitor(server_id, period) calls; NetworkChartClient gains period, onPeriodChange, and isLogin props; chartStats (min/max delay) and UI controls updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant NC as NetworkChart
participant API as nezha-api
participant UI as Renderer
User->>NC: Mount / interact
NC->>API: fetchLoginUser()
API-->>NC: isLogin
alt isLogin = true
NC->>NC: default period = "30d"
NC->>UI: enable full TIME_RANGE_OPTIONS
else isLogin = false
NC->>NC: set period = "1d" (locked)
NC->>UI: lock non-1d options
end
User->>NC: change period (if allowed)
NC->>NC: onPeriodChange(period)
NC->>API: fetchMonitor(server_id, period)
API-->>NC: monitor data (period-scoped)
NC->>NC: compute chartStats (min/max), sort keys
NC->>UI: render chart, buttons, and stats
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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. 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 |
|
Linting and formatting issues were automatically fixed. Please review the changes. |
There was a problem hiding this comment.
Pull request overview
This PR enhances the NetworkChart component to support user login state awareness and period selection for monitoring data. Non-authenticated users are restricted to viewing 1-day data, while logged-in users can access 7-day and 30-day periods.
Changes:
- Added MonitorPeriod type and period parameter to the fetchMonitor API function
- Updated API endpoint from
/api/v1/service/${server_id}to/api/v1/server/${server_id}/service - Implemented login state detection and period restriction logic in NetworkChart
- Added time range selector UI with visual indicators for locked periods
- Enhanced monitor button display with min/max delay statistics
- Improved monitor sorting by ID for consistent ordering
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/lib/nezha-api.ts | Added MonitorPeriod type and updated fetchMonitor to support period parameter with modified API endpoint |
| src/components/NetworkChart.tsx | Integrated login state checking, period selection UI, period-based API fetching, monitor sorting by ID, and min/max delay statistics display |
| bun.lock | Updated lockfile configuration version (standard Bun lockfile format update) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| period === option.value | ||
| ? "bg-primary text-primary-foreground shadow-sm" | ||
| : "text-muted-foreground hover:bg-muted hover:text-foreground" | ||
| } ${isLocked ? "cursor-not-allowed opacity-50" : ""}`} |
There was a problem hiding this comment.
The locked period buttons lack an accessible explanation for why they are disabled. Screen reader users and keyboard users would benefit from an aria-label or title attribute explaining that login is required to access these time periods.
Consider adding: aria-label={isLocked ? "Login required for ${option.label} period" : option.label} or title={isLocked ? "Login required" : undefined}
| } ${isLocked ? "cursor-not-allowed opacity-50" : ""}`} | |
| } ${isLocked ? "cursor-not-allowed opacity-50" : ""}`} | |
| aria-label={ | |
| isLocked | |
| ? `Login required for ${option.label} period` | |
| : option.label | |
| } | |
| title={isLocked ? "Login required" : undefined} |
| onClick={() => { | ||
| if (!isLocked) { | ||
| onPeriodChange(option.value); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The onClick handler includes a redundant check for !isLocked on line 555, since the button is already disabled when isLocked is true. The disabled attribute already prevents click events, making this check unnecessary.
You can simplify this to: onClick={() => onPeriodChange(option.value)}
| onClick={() => { | |
| if (!isLocked) { | |
| onPeriodChange(option.value); | |
| } | |
| }} | |
| onClick={() => onPeriodChange(option.value)} |
| const monitorData = chartData[key]; | ||
| const lastDelay = monitorData[monitorData.length - 1].avg_delay; |
There was a problem hiding this comment.
There's a potential crash if monitorData is an empty array. Accessing monitorData[monitorData.length - 1] would try to access index -1, which returns undefined, and then accessing .avg_delay on undefined would throw an error.
While this is unlikely given the context, consider adding a safety check or ensuring that chartDataKey is only created when valid data exists.
| const monitorData = chartData[key]; | |
| const lastDelay = monitorData[monitorData.length - 1].avg_delay; | |
| const monitorData = chartData[key] || []; | |
| const lastDelay = | |
| monitorData.length > 0 && | |
| monitorData[monitorData.length - 1]?.avg_delay !== undefined | |
| ? monitorData[monitorData.length - 1].avg_delay | |
| : 0; |
| ): Promise<MonitorResponse> => { | ||
| const response = await fetch(`/api/v1/service/${server_id}`); | ||
| const query = period ? `?period=${period}` : ""; | ||
| const response = await fetch(`/api/v1/server/${server_id}/service${query}`); |
There was a problem hiding this comment.
The API endpoint has been changed from /api/v1/service/${server_id} to /api/v1/server/${server_id}/service${query}. This is a breaking change that modifies the API path structure. Please verify that:
- The backend API supports this new endpoint format
- The old endpoint path was incorrect and this is a fix
- All callers of this function are compatible with this change
If this is intentional and the backend has been updated, this is fine. Otherwise, this could cause API calls to fail.
| const response = await fetch(`/api/v1/server/${server_id}/service${query}`); | |
| const response = await fetch(`/api/v1/service/${server_id}${query}`); |
| const [period, setPeriod] = React.useState<MonitorPeriod>("30d"); | ||
| const { data: userData, isError: isLoginError } = useQuery({ | ||
| queryKey: ["login-user"], | ||
| queryFn: () => fetchLoginUser(), | ||
| refetchOnMount: false, | ||
| refetchOnWindowFocus: true, | ||
| refetchIntervalInBackground: true, | ||
| refetchInterval: 1000 * 30, | ||
| retry: 0, | ||
| }); | ||
| const isLogin = isLoginError | ||
| ? false | ||
| : userData | ||
| ? !!userData?.data?.id && !!document.cookie | ||
| : false; | ||
|
|
||
| React.useEffect(() => { | ||
| if (!isLogin && period !== "1d") { | ||
| setPeriod("1d"); | ||
| } | ||
| }, [isLogin, period]); |
There was a problem hiding this comment.
The default period is set to "30d" but non-logged-in users are restricted to "1d". This creates an inconsistent initial state where non-logged-in users will start with period="30d" until the login query completes, at which point it gets reset to "1d". This could cause:
- An unnecessary API call with period="30d" before the reset
- A jarring UI state change when the period suddenly changes
Consider setting the initial period state based on whether the user is likely to be logged in (e.g., check for cookie presence) or default to "1d" for all users initially.
| const minDelay = Math.min(...delays); | ||
| const maxDelay = Math.max(...delays); |
There was a problem hiding this comment.
The chartStats calculation uses Math.min(...delays) and Math.max(...delays) with the spread operator. For large datasets (e.g., 30 days of data with frequent polling), this could cause performance issues or even stack overflow errors, as the spread operator has limitations with very large arrays.
Consider using a more efficient approach like a reduce operation or a simple loop to find min/max values:
const delays = data.map((item) => item.avg_delay);
let minDelay = delays[0], maxDelay = delays[0];
for (const delay of delays) {
if (delay < minDelay) minDelay = delay;
if (delay > maxDelay) maxDelay = delay;
}| const minDelay = Math.min(...delays); | |
| const maxDelay = Math.max(...delays); | |
| let minDelay = delays[0]; | |
| let maxDelay = delays[0]; | |
| for (const delay of delays) { | |
| if (delay < minDelay) minDelay = delay; | |
| if (delay > maxDelay) maxDelay = delay; | |
| } |
| @@ -166,6 +209,9 @@ export function NetworkChart({ | |||
| chartData={transformedData} | |||
| serverName={monitorData.data[0].server_name} | |||
There was a problem hiding this comment.
There's a potential issue where monitorData.data could be an empty array, which would cause monitorData.data[0].server_name on line 210 to throw an error when trying to access the server_name property of undefined.
The check on line 161 only verifies if monitorData.data is falsy, but doesn't check if it's an empty array. Consider adding a check for empty arrays or using optional chaining: monitorData.data[0]?.server_name ?? 'Unknown Server'
| serverName={monitorData.data[0].server_name} | |
| serverName={monitorData.data[0]?.server_name ?? "Unknown Server"} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/components/NetworkChart.tsx (1)
296-346: Guard against empty monitor series to prevent crashes.
monitorData[monitorData.length - 1]will throw if a series is empty (e.g., new monitors or short periods). Add a fallback so the UI renders safely.🐛 Suggested fix
- const monitorData = chartData[key]; - const lastDelay = monitorData[monitorData.length - 1].avg_delay; - const stats = chartStats[key]; + const monitorData = chartData[key] ?? []; + const lastPoint = monitorData[monitorData.length - 1]; + const lastDelay = lastPoint?.avg_delay ?? 0; + const stats = chartStats[key] ?? { minDelay: 0, maxDelay: 0 };
🤖 Fix all issues with AI agents
In `@src/components/NetworkChart.tsx`:
- Around line 128-153: The monitor query can fire with period "30d" before login
resolves; create an effectivePeriod (e.g., const effectivePeriod = !isLogin ?
"1d" : period) and use that in the useQuery queryKey and in the fetchMonitor
call instead of raw period so logged-out users never trigger >1d requests; keep
the existing period state and useEffect (setPeriod) as-is so the UI still
updates when login state changes.
🧹 Nitpick comments (1)
src/components/NetworkChart.tsx (1)
545-579: Consider adding toggle semantics for accessibility.These buttons behave like toggles; adding
aria-pressed(andaria-disabled) improves screen‑reader feedback.♿ Suggested tweak
<button key={option.value} type="button" disabled={isLocked} + aria-pressed={period === option.value} + aria-disabled={isLocked} onClick={() => { if (!isLocked) { onPeriodChange(option.value); } }}
| const [period, setPeriod] = React.useState<MonitorPeriod>("30d"); | ||
| const { data: userData, isError: isLoginError } = useQuery({ | ||
| queryKey: ["login-user"], | ||
| queryFn: () => fetchLoginUser(), | ||
| refetchOnMount: false, | ||
| refetchOnWindowFocus: true, | ||
| refetchIntervalInBackground: true, | ||
| refetchInterval: 1000 * 30, | ||
| retry: 0, | ||
| }); | ||
| const isLogin = isLoginError | ||
| ? false | ||
| : userData | ||
| ? !!userData?.data?.id && !!document.cookie | ||
| : false; | ||
|
|
||
| React.useEffect(() => { | ||
| if (!isLogin && period !== "1d") { | ||
| setPeriod("1d"); | ||
| } | ||
| }, [isLogin, period]); | ||
|
|
||
| const { data: monitorData } = useQuery({ | ||
| queryKey: ["monitor", server_id], | ||
| queryFn: () => fetchMonitor(server_id), | ||
| queryKey: ["monitor", server_id, period], | ||
| queryFn: () => fetchMonitor(server_id, period), | ||
| enabled: show, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the NetworkChart.tsx file
fd NetworkChart.tsx --type fRepository: hamster1963/nezha-dash-v1
Length of output: 100
🏁 Script executed:
# Read the component around the lines in question
cat -n src/components/NetworkChart.tsx | head -180 | tail -60Repository: hamster1963/nezha-dash-v1
Length of output: 1988
🏁 Script executed:
# Search for usages of NetworkChart component
rg "NetworkChart" --type tsx --type ts -A 2 -B 2Repository: hamster1963/nezha-dash-v1
Length of output: 96
🏁 Script executed:
# Find and examine the fetchMonitor function
rg "fetchMonitor" --type tsx --type ts -A 5Repository: hamster1963/nezha-dash-v1
Length of output: 96
🏁 Script executed:
# Search for NetworkChart and fetchMonitor without type restrictions
rg "NetworkChart" -A 2 -B 2Repository: hamster1963/nezha-dash-v1
Length of output: 4278
🏁 Script executed:
# Search for fetchMonitor definition and usage
rg "fetchMonitor" -A 5Repository: hamster1963/nezha-dash-v1
Length of output: 1162
🏁 Script executed:
# Read the full NetworkChart component to understand the control flow
cat -n src/components/NetworkChart.tsx | head -250Repository: hamster1963/nezha-dash-v1
Length of output: 8269
🏁 Script executed:
# Search for any period-related authorization or comments
rg "period|1d|30d" src/components/NetworkChart.tsx -B 2 -A 2Repository: hamster1963/nezha-dash-v1
Length of output: 1604
🏁 Script executed:
# Look for API documentation or comments about period restrictions
rg "service|monitor|period" src/lib/nezha-api.ts -A 3 -B 1Repository: hamster1963/nezha-dash-v1
Length of output: 569
🏁 Script executed:
# Search for any authorization/permission handling in the codebase
rg "unauthorized|forbidden|403|permission|restrict" -iRepository: hamster1963/nezha-dash-v1
Length of output: 50382
Use an effective period to prevent initial 30d fetch before login resolves.
On first render, period initializes to "30d", and if the tab becomes active before login status is fully resolved, the monitor query could fire with "30d" before the useEffect resets it to "1d". While mitigated by the enabled: show flag, the suggested fix prevents unnecessary query key churn and ensures logged-out users never request periods beyond 1d.
Suggested fix
+ const effectivePeriod = isLogin ? period : "1d";
const { data: monitorData } = useQuery({
- queryKey: ["monitor", server_id, period],
- queryFn: () => fetchMonitor(server_id, period),
+ queryKey: ["monitor", server_id, effectivePeriod],
+ queryFn: () => fetchMonitor(server_id, effectivePeriod),
enabled: show,
refetchOnMount: true,
refetchOnWindowFocus: true,
refetchInterval: 10000,
});🤖 Prompt for AI Agents
In `@src/components/NetworkChart.tsx` around lines 128 - 153, The monitor query
can fire with period "30d" before login resolves; create an effectivePeriod
(e.g., const effectivePeriod = !isLogin ? "1d" : period) and use that in the
useQuery queryKey and in the fetchMonitor call instead of raw period so
logged-out users never trigger >1d requests; keep the existing period state and
useEffect (setPeriod) as-is so the UI still updates when login state changes.
Summary by CodeRabbit
New Features
UI/UX Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.