Repository navigation
feat: add auto-refresh frequency dropdown to dashboard - #394
Conversation
- Added dropdown for user to select refresh intervals - Added options: Off, 5s, 10s, 30s, 1min - Implemented auto-refresh functionality Fixes FailproofAI#371
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an auto-refresh feature to RunsTable: defines REFRESH_OPTIONS and RefreshMs, adds refreshInterval state (default 0), starts/stops a polling timer via useEffect to call loadRuns(currentPage, pageSize) when refreshInterval > 0, and replaces the standalone Refresh button with a grouped Auto-refresh select plus Refresh button. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant RunsTable
participant Timer
participant DataSource as loadRuns()
User->>RunsTable: choose Auto-refresh option (e.g., 10s)
RunsTable->>RunsTable: setState(refreshInterval)
Note over RunsTable: useEffect watches refreshInterval, currentPage, pageSize
RunsTable->>Timer: start setInterval(refreshInterval)
Timer-->>RunsTable: tick
RunsTable->>DataSource: loadRuns(currentPage, pageSize)
DataSource-->>RunsTable: runs data
RunsTable-->>User: table updates
alt Interval changed or component unmounts
RunsTable->>Timer: clearInterval
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Kalra-V, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant enhancement to the dashboard's real-time monitoring capabilities by providing users with control over the data auto-refresh frequency. This feature allows for a more tailored and efficient user experience when tracking graph execution progress, enabling users to choose between various polling intervals or disable auto-refresh entirely.
Highlights
- Auto-refresh frequency dropdown: A new UI element has been added to the dashboard, allowing users to select their preferred auto-refresh interval for real-time data updates.
- Dynamic polling mechanism: The system now dynamically adjusts the data polling frequency based on the user's selection, including an "Off" option to disable auto-refresh.
- State management for refresh interval: A new state variable
refreshIntervalhas been introduced to manage the currently selected auto-refresh frequency.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces an auto-refresh feature to the dashboard's runs table, which is a great addition for real-time monitoring. The implementation is mostly solid, but I've identified a potential race condition in the polling logic that could occur if API requests take longer than the refresh interval. I've provided a more robust solution using setTimeout to prevent this. Additionally, I've suggested a small accessibility improvement for the new dropdown menu. Overall, these are good changes that enhance the user experience.
| useEffect(() => { | ||
| if (refreshInterval === 0) return; | ||
|
|
||
| const interval = setInterval(() => { | ||
| loadRuns(currentPage, pageSize); | ||
| }, refreshInterval); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, [refreshInterval, currentPage, pageSize, loadRuns]); |
There was a problem hiding this comment.
Using setInterval for polling asynchronous data can lead to issues. If the loadRuns API call takes longer than refreshInterval, new requests will be initiated before previous ones complete. This can cause race conditions and an inconsistent loading state. A more robust approach is to use a recursive setTimeout, which schedules the next fetch only after the current one has completed. This ensures that requests don't overlap.
useEffect(() => {
if (refreshInterval === 0) {
return;
}
let isCancelled = false;
let timeoutId: ReturnType<typeof setTimeout>;
const poll = () => {
loadRuns(currentPage, pageSize).finally(() => {
if (!isCancelled) {
timeoutId = setTimeout(poll, refreshInterval);
}
});
};
// To match the original behavior, we start polling after the first interval.
timeoutId = setTimeout(poll, refreshInterval);
return () => {
isCancelled = true;
clearTimeout(timeoutId);
};
}, [refreshInterval, currentPage, pageSize, loadRuns]);
| <label className="text-sm text-gray-700">Auto-refresh:</label> | ||
| <select | ||
| value={refreshInterval} | ||
| onChange={(e) => setRefreshInterval(Number(e.target.value))} | ||
| className="border border-gray-300 rounded-md px-2 py-1 text-sm" | ||
| > | ||
| {REFRESH_OPTIONS.map((option) => ( | ||
| <option key={option.value} value={option.value}> | ||
| {option.label} | ||
| </option> | ||
| ))} | ||
| </select> |
There was a problem hiding this comment.
For better accessibility, the <label> should be explicitly associated with the <select> element. You can achieve this by adding an id to the select and a corresponding htmlFor attribute to the label. This helps screen reader users understand which label belongs to which form control. For components like this, using the useId hook from React is the recommended practice to generate a unique ID.
<label htmlFor="auto-refresh-select" className="text-sm text-gray-700">Auto-refresh:</label>
<select
id="auto-refresh-select"
value={refreshInterval}
onChange={(e) => setRefreshInterval(Number(e.target.value))}
className="border border-gray-300 rounded-md px-2 py-1 text-sm"
>
{REFRESH_OPTIONS.map((option) => (
<option key={option.value} value={option.value}>
{option.label}
</option>
))}
</select>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
dashboard/src/components/RunsTable.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
dashboard/src/components/RunsTable.tsx
[error] 168-168: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
| useEffect(() => { | ||
| if (refreshInterval === 0) return; | ||
|
|
||
| const interval = setInterval(() => { | ||
| loadRuns(currentPage, pageSize); | ||
| }, refreshInterval); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, [refreshInterval, currentPage, pageSize, loadRuns]); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider server push for real-time updates
Polling is fine for v1, but if “graph execution progress” needs near real-time and scale, evaluate SSE or WebSocket to reduce load and latency. Keep the dropdown as a fallback.
🛠️ Refactor suggestion
Avoid overlapping fetches; trigger immediate refresh on enable
Two issues:
- setInterval can fire while a previous loadRuns is in-flight → stale data race.
- Enabling auto-refresh waits for the first tick; trigger an immediate load.
Minimal in-hunk improvement adds an immediate call and skips polling when the tab is hidden. For the overlap race, see follow-up snippet to add an in-flight guard.
useEffect(() => {
if (refreshInterval === 0) return;
- const interval = setInterval(() => {
- loadRuns(currentPage, pageSize);
- }, refreshInterval);
+ // Immediate refresh when enabling auto-refresh
+ loadRuns(currentPage, pageSize);
+ const intervalId = setInterval(() => {
+ if (!document.hidden) {
+ loadRuns(currentPage, pageSize);
+ }
+ }, refreshInterval);
- return () => clearInterval(interval);
+ return () => clearInterval(intervalId);
}, [refreshInterval, currentPage, pageSize, loadRuns]);Outside this hunk, add a concurrency guard (choose one):
Option A — Abort previous request (preferred if clientApiService supports AbortController):
// imports: useRef
const inFlight = useRef<AbortController | null>(null);
const loadRuns = useCallback(async (page: number, size: number) => {
inFlight.current?.abort();
const ac = new AbortController();
inFlight.current = ac;
setIsLoading(true);
setError(null);
try {
const data = await clientApiService.getRuns(namespace, page, size, { signal: ac.signal });
setRunsData(data);
} catch (err: any) {
if (err?.name !== 'AbortError') {
setError(err instanceof Error ? err.message : 'Failed to load runs');
}
} finally {
if (inFlight.current === ac) inFlight.current = null;
setIsLoading(false);
}
}, [namespace]);Option B — Last-wins gate via requestId (if AbortController isn’t available):
const reqIdRef = useRef(0);
const loadRuns = useCallback(async (page: number, size: number) => {
const id = ++reqIdRef.current;
setIsLoading(true);
setError(null);
try {
const data = await clientApiService.getRuns(namespace, page, size);
if (id === reqIdRef.current) setRunsData(data);
} catch (err) {
if (id === reqIdRef.current) {
setError(err instanceof Error ? err.message : 'Failed to load runs');
}
} finally {
if (id === reqIdRef.current) setIsLoading(false);
}
}, [namespace]);🤖 Prompt for AI Agents
In dashboard/src/components/RunsTable.tsx around lines 66 to 75, the current
setInterval-based auto-refresh can overlap requests and doesn't trigger an
immediate load when enabled; update the effect to call loadRuns immediately when
refreshInterval > 0, avoid scheduling when document.hidden, and keep the
interval ref so you clear it properly on unmount/changes. Additionally,
implement one of the concurrency guards outside this hunk: preferred Option A
(useRef<AbortController> to abort previous request before starting a new one and
pass its signal to clientApiService), or Option B (useRef requestId last-wins
gate to ignore stale responses), and ensure loading/error state is only updated
for the active request.
|
Hey @Kalra-V I would request to please go through all the comments by AI agents, please take necessary actions (Resolve or make changes). Do ping me once done, I will do the final approval. |
- enhanced auto-polling useEffect to avoid requests overlapping - added htmlFor and id for select dropdown for better accessibility - made REFRESH_OPTIONS constant type-safe
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
dashboard/src/components/RunsTable.tsx (3)
26-32: Derive a RefreshMs union from REFRESH_OPTIONS (type-safety).You already used
as const; add a union for values to type state/handlers.const REFRESH_OPTIONS = [ { label: "Off", value: 0 }, { label: "5 seconds", value: 5000 }, { label: "10 seconds", value: 10000 }, { label: "30 seconds", value: 30000 }, { label: "1 minute", value: 60000 }, ] as const; + +type RefreshMs = typeof REFRESH_OPTIONS[number]['value'];
44-44: Strongly type refreshInterval.Constrain to allowed values.
- const [refreshInterval, setRefreshInterval] = useState(0); + const [refreshInterval, setRefreshInterval] = useState<RefreshMs>(0);
181-195: Associate label via a stable unique id (avoid duplicate ids); narrow onChange type.Works as-is; prefer
useIdto avoid collisions if multiple tables render.- <div className="flex items-center space-x-2"> - <label htmlFor='auto-refresh-select' className="text-sm text-gray-700">Auto-refresh:</label> - <select - id='auto-refresh-select' + <div className="flex items-center space-x-2"> + <label htmlFor={autoRefreshId} className="text-sm text-gray-700">Auto-refresh:</label> + <select + id={autoRefreshId} value={refreshInterval} - onChange={(e) => setRefreshInterval(Number(e.target.value))} + onChange={(e) => setRefreshInterval(Number(e.target.value) as RefreshMs)} className="border border-gray-300 rounded-md px-2 py-1 text-sm" >Add outside this hunk:
// import: useId import React, { useState, useEffect, useCallback, useRef, useId } from 'react'; // inside component const autoRefreshId = useId();
| useEffect(() => { | ||
| if (refreshInterval === 0) { | ||
| return; | ||
| } | ||
|
|
||
| let isCancelled = false; | ||
| let timeoutId: ReturnType<typeof setTimeout>; | ||
|
|
||
| const poll = () => { | ||
| loadRuns(currentPage, pageSize).finally(() => { | ||
| if (!isCancelled) { | ||
| timeoutId = setTimeout(poll, refreshInterval); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| timeoutId = setTimeout(poll, refreshInterval); | ||
|
|
||
| return () => { | ||
| isCancelled = true; | ||
| clearTimeout(timeoutId); | ||
| }; | ||
| }, [refreshInterval, currentPage, pageSize, loadRuns]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Trigger an immediate refresh and pause work when tab is hidden; keep non-overlapping loop.
Current recursive timeout is good (no overlaps), but it waits one interval before first fetch and does work while the tab is hidden.
useEffect(() => {
if (refreshInterval === 0) {
return;
}
let isCancelled = false;
let timeoutId: ReturnType<typeof setTimeout>;
const poll = () => {
- loadRuns(currentPage, pageSize).finally(() => {
- if (!isCancelled) {
- timeoutId = setTimeout(poll, refreshInterval);
- }
- });
+ if (document.hidden) {
+ timeoutId = setTimeout(poll, refreshInterval);
+ return;
+ }
+ loadRuns(currentPage, pageSize).finally(() => {
+ if (!isCancelled) {
+ timeoutId = setTimeout(poll, refreshInterval);
+ }
+ });
};
- timeoutId = setTimeout(poll, refreshInterval);
+ // Immediate refresh on enable
+ poll();
return () => {
isCancelled = true;
clearTimeout(timeoutId);
};
}, [refreshInterval, currentPage, pageSize, loadRuns]);Optional but recommended: add a last-wins gate to ignore stale responses from manual clicks vs. pollers.
// imports: useRef
const reqIdRef = useRef(0);
const loadRuns = useCallback(async (page: number, size: number) => {
const id = ++reqIdRef.current;
setIsLoading(true);
setError(null);
try {
const data = await clientApiService.getRuns(namespace, page, size);
if (id === reqIdRef.current) setRunsData(data);
} catch (err) {
if (id === reqIdRef.current) {
setError(err instanceof Error ? err.message : 'Failed to load runs');
}
} finally {
if (id === reqIdRef.current) setIsLoading(false);
}
}, [namespace]);🤖 Prompt for AI Agents
In dashboard/src/components/RunsTable.tsx around lines 66 to 88, change the
polling effect so it triggers an immediate fetch on mount, pauses scheduling
when document.visibilityState === 'hidden', and preserves the non-overlapping
recursive timeout loop; implement a request-id last-wins gate using a useRef
counter that increments per loadRuns call and only applies results/error/loading
state when the id matches the latest ref. Specifically: import and use useRef,
increment reqIdRef before each fetch, set isLoading/error state only when ids
match, call loadRuns immediately (not after first timeout), keep the
setTimeout-based poll scheduling inside finally only when the tab is visible and
not cancelled, and clear/stop timeouts and set cancel flag in the cleanup.
Ensure currentPage/pageSize/loadRuns remain in deps.
- Added type for refresh interval state
NiveditJain
left a comment
NiveditJain
left a comment
There was a problem hiding this comment.
Hey @Kalra-V,
This PR is lgtm, I have following questions in mind:
1 - Can we make Auto-Refresh settings near to refresh button and align it properly for aesthetically pleasing look.
2 - At label "Off" the value of refresh is 5 secs, which if fine but probably assigning a value when it doesn't hold a significance could be not the best practice, should we make it Null when label is "Off"
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
dashboard/src/components/RunsTable.tsx (2)
68-91: Trigger immediate poll and pause when tab is hidden; keep non-overlapping loopAvoid waiting one interval before the first fetch and skip work when the page is hidden.
useEffect(() => { if (refreshInterval === 0) { return; } let isCancelled = false; let timeoutId: ReturnType<typeof setTimeout>; const poll = () => { - loadRuns(currentPage, pageSize).finally(() => { - if (!isCancelled) { - timeoutId = setTimeout(poll, refreshInterval); - } - }); + if (document.hidden) { + timeoutId = setTimeout(poll, refreshInterval); + return; + } + loadRuns(currentPage, pageSize).finally(() => { + if (!isCancelled) { + timeoutId = setTimeout(poll, refreshInterval); + } + }); }; - timeoutId = setTimeout(poll, refreshInterval); + // Immediate refresh on enable + poll(); return () => { isCancelled = true; clearTimeout(timeoutId); }; }, [refreshInterval, currentPage, pageSize, loadRuns]);Additionally, consider a last-wins gate to prevent manual clicks overlapping with polls from showing stale data:
// add near component top const reqIdRef = React.useRef(0); // refactor loadRuns const loadRuns = useCallback(async (page: number, size: number) => { const id = ++reqIdRef.current; setIsLoading(true); setError(null); try { const data = await clientApiService.getRuns(namespace, page, size); if (id === reqIdRef.current) setRunsData(data); } catch (err) { if (id === reqIdRef.current) setError(err instanceof Error ? err.message : 'Failed to load runs'); } finally { if (id === reqIdRef.current) setIsLoading(false); } }, [namespace]);
185-192: Prefer useId for uniqueness over a hardcoded idPrevents duplicate ids if this component is rendered multiple times.
- <label - htmlFor="auto-refresh-select" + <label + htmlFor={selectId} className="text-xs font-medium text-gray-600" > Auto-refresh </label> - <select - id="auto-refresh-select" + <select + id={selectId}Outside this hunk:
// import import React, { useState, useEffect, useCallback, useId } from 'react'; // inside component const selectId = useId();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
dashboard/src/components/RunsTable.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
dashboard/src/components/RunsTable.tsx
[error] 205-208: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🔇 Additional comments (3)
dashboard/src/components/RunsTable.tsx (3)
26-35: Type-safe interval options + union alias look goodLiterals are locked and the union type is correct.
46-46: Typed state for refreshInterval is correctKeeps values constrained to allowed intervals.
183-204: A11y label-control association fixed — LGTMThe label now references the select via htmlFor/id.
| <button | ||
| onClick={() => loadRuns(currentPage, pageSize)} | ||
| className="flex items-center space-x-2 px-4 py-2 bg-[#031035] text-white rounded-lg hover:bg-[#0a1a4a] transition-colors shadow-sm" | ||
| > | ||
| <RefreshCw className="w-4 h-4" /> | ||
| <span>Refresh</span> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit button type to avoid unintended form submission (Biome error)
- <button
+ <button
+ type="button"
onClick={() => loadRuns(currentPage, pageSize)}
className="flex items-center space-x-2 px-4 py-2 bg-[#031035] text-white rounded-lg hover:bg-[#0a1a4a] transition-colors shadow-sm"
>Apply the same type="button" to other non-submit buttons in this file.
📝 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.
| <button | |
| onClick={() => loadRuns(currentPage, pageSize)} | |
| className="flex items-center space-x-2 px-4 py-2 bg-[#031035] text-white rounded-lg hover:bg-[#0a1a4a] transition-colors shadow-sm" | |
| > | |
| <RefreshCw className="w-4 h-4" /> | |
| <span>Refresh</span> | |
| </button> | |
| </div> | |
| <button | |
| type="button" | |
| onClick={() => loadRuns(currentPage, pageSize)} | |
| className="flex items-center space-x-2 px-4 py-2 bg-[#031035] text-white rounded-lg hover:bg-[#0a1a4a] transition-colors shadow-sm" | |
| > | |
| <RefreshCw className="w-4 h-4" /> | |
| <span>Refresh</span> | |
| </button> | |
| </div> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 205-208: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
In dashboard/src/components/RunsTable.tsx around lines 205 to 212, the Refresh
button (and other non-submit buttons in this file) lack an explicit type, which
can cause unintended form submissions; update the Refresh button to include
type="button" and scan the file to add type="button" to every other button
element that is not intended to submit a form so they won't trigger form submit
behavior.
2 participants
Description
Added a dropdown that allows users to select polling frequency for real-time updates of graph execution progress.
Changes Made
refreshIntervalTesting
Closes #371