User-controlled refresh for read-only lists, stop auto-refetch on tab refocus - #1911
Open
RotemCDos wants to merge 11 commits into
Open
User-controlled refresh for read-only lists, stop auto-refetch on tab refocus#1911RotemCDos wants to merge 11 commits into
RotemCDos wants to merge 11 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
…ate select) Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
…e alias Co-Authored-By: Claude Opus 4.8 <[email protected]>
…val menu) Replaces the separate Refresh button + rate dropdown with a single joined control: the left segment triggers a manual refresh (spinner while fetching), the right segment shows the active auto-refresh interval and opens a menu to change it. Interval still persists under the same storage key, so useRefreshRate and the query polling are unaffected. RefreshRateSelect remains for its other consumers (consumer-group lag pollers). Co-Authored-By: Claude Opus 4.8 <[email protected]>
kapybro
Bot
added
status/triage/manual
|
AI Summary Read-only list tables previously auto-refetched whenever the browser tab regained focus, causing unwanted reloads. This change disables involuntary refetching globally by setting |
kapybro
Bot
changed the title
kapybro
Bot
added
area/connect
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a reusable ChangesRefresh polling flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant TableRefresh
participant LocalStorage
participant useRefreshRate
participant ReactQuery
Operator->>TableRefresh: Select auto-refresh interval
TableRefresh->>LocalStorage: Store rate in seconds
useRefreshRate->>LocalStorage: Read stored rate
useRefreshRate->>ReactQuery: Configure refetchInterval
Operator->>TableRefresh: Click Refresh
TableRefresh->>ReactQuery: Trigger refetch
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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: 1
🧹 Nitpick comments (2)
frontend/src/components/common/Icons/RefreshIcon.tsx (1)
5-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-hidden="true"to the decorative SVG.The icon is used inside a button with
aria-label="Refresh", making the SVG purely decorative. Addingaria-hidden="true"prevents redundant screen reader announcements.♿ Proposed fix
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512" width="14" height="14" fill="currentColor" + aria-hidden="true" + focusable="false" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/common/Icons/RefreshIcon.tsx` around lines 5 - 14, Add aria-hidden="true" to the decorative SVG element in RefreshIcon, preserving the existing button aria-label and icon markup.frontend/src/lib/hooks/api/topics.ts (1)
66-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider omitting
placeholderDatafromqueryOptionsto prevent accidental override.
...queryOptionsis spread afterplaceholderData: (previousData) => previousData, so a caller could override it and break the "keep existing data visible during refetches" guarantee. The current caller only passesrefetchInterval/refetchIntervalInBackground, so this isn't an active bug, but adding'placeholderData'to theOmitlist would make the contract safer.♻️ Suggested fix
export function useTopics( props: GetTopicsRequest, queryOptions?: Omit< UseQueryOptions<TopicsResponse, ServerResponse>, - 'queryKey' | 'queryFn' + 'queryKey' | 'queryFn' | 'placeholderData' > ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/hooks/api/topics.ts` around lines 66 - 80, Update the useTopics queryOptions type to omit placeholderData alongside queryKey and queryFn, preventing callers from overriding the hook’s existing-data behavior while preserving the current option spread and refetch configuration support.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/common/TableRefresh/TableRefresh.tsx`:
- Around line 51-58: Update the interval toggle in the TableRefresh Dropdown so
Dropdown’s open state is passed to DropdownArrowIcon via isOpen, and ensure the
button’s accessible name includes the current interval label instead of being
hidden by the static aria-label. Preserve the existing rate display and toggle
behavior.
---
Nitpick comments:
In `@frontend/src/components/common/Icons/RefreshIcon.tsx`:
- Around line 5-14: Add aria-hidden="true" to the decorative SVG element in
RefreshIcon, preserving the existing button aria-label and icon markup.
In `@frontend/src/lib/hooks/api/topics.ts`:
- Around line 66-80: Update the useTopics queryOptions type to omit
placeholderData alongside queryKey and queryFn, preventing callers from
overriding the hook’s existing-data behavior while preserving the current option
spread and refetch configuration support.
🪄 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: efc15020-b723-4c85-b241-c490ce611722
📒 Files selected for processing (19)
frontend/src/components/App.tsxfrontend/src/components/Connect/List/ListPage.tsxfrontend/src/components/Connect/List/__tests__/ListPage.spec.tsxfrontend/src/components/ConsumerGroups/List.tsxfrontend/src/components/ConsumerGroups/__test__/List.spec.tsxfrontend/src/components/Schemas/List/List.tsxfrontend/src/components/Schemas/List/__test__/List.spec.tsxfrontend/src/components/Topics/List/ListPage.tsxfrontend/src/components/Topics/List/TopicTable.tsxfrontend/src/components/Topics/List/__tests__/ListPage.spec.tsxfrontend/src/components/__tests__/queryClientConfig.spec.tsfrontend/src/components/common/Icons/RefreshIcon.tsxfrontend/src/components/common/RefreshRateSelect/RefreshRateSelect.tsxfrontend/src/components/common/TableRefresh/TableRefresh.styled.tsfrontend/src/components/common/TableRefresh/TableRefresh.tsxfrontend/src/components/common/TableRefresh/__tests__/TableRefresh.spec.tsxfrontend/src/lib/hooks/__tests__/useRefreshRate.spec.tsfrontend/src/lib/hooks/api/topics.tsfrontend/src/lib/hooks/useRefreshRate.ts
Comment on lines
+51
to
+58
| <Dropdown | ||
| aria-label="Auto refresh interval" | ||
| openBtnEl={ | ||
| <S.IntervalButton type="button"> | ||
| {rate > 0 && <S.Rate>{current?.label}</S.Rate>} | ||
| <DropdownArrowIcon isOpen={false} /> | ||
| </S.IntervalButton> | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the Dropdown component to verify openBtnEl rendering behavior.
# Find the Dropdown component implementation
ast-grep outline frontend/src/components/common/Dropdown --items all --type function,component --match 'Dropdown'
# Then read the relevant rendering logic
# Look for how openBtnEl is rendered: is it wrapped in a <button>?
# Is isOpen injected via cloneElement? Is aria-label passed to openBtnEl?Repository: kafbat/kafka-ui
Length of output: 200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the Dropdown implementation and related props/usages.
fd -a 'Dropdown*' frontend/src/components/common
rg -n "openBtnEl|isOpen|aria-label|cloneElement|<button|DropdownArrowIcon" frontend/src/components/common/Dropdown frontend/src/components/common/TableRefresh -S
# Map the Dropdown component structure before reading it.
ast-grep outline frontend/src/components/common/Dropdown --match 'Dropdown' --view expanded || true
# Read the likely implementation files if present.
for f in \
frontend/src/components/common/Dropdown/Dropdown.tsx \
frontend/src/components/common/Dropdown/index.ts \
frontend/src/components/common/Dropdown/DropdownButton.tsx
do
[ -f "$f" ] && { echo "===== $f ====="; cat -n "$f"; }
doneRepository: kafbat/kafka-ui
Length of output: 5568
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n frontend/src/components/common/Icons/DropdownArrowIcon.tsx
echo "===== TableRefresh ====="
cat -n frontend/src/components/common/TableRefresh/TableRefresh.tsxRepository: kafbat/kafka-ui
Length of output: 4128
Pass the open state into the interval toggle
DropdownclonesopenBtnElinto a<span>, so nested buttons aren’t an issue.- The arrow still never receives
isOpen, and the clonedaria-labelhides the current interval from the button’s accessible name. Include the rate in the label or usearia-labelledby.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/common/TableRefresh/TableRefresh.tsx` around lines 51
- 58, Update the interval toggle in the TableRefresh Dropdown so Dropdown’s open
state is passed to DropdownArrowIcon via isOpen, and ensure the button’s
accessible name includes the current interval label instead of being hidden by
the static aria-label. Preserve the existing rate display and toggle behavior.
- RefreshIcon: mark the decorative SVG aria-hidden/focusable=false (icon sits in a button that already has an accessible name) - TableRefresh: make the interval toggle's accessible name convey the active interval (e.g. 'Auto refresh interval: every 5s') instead of a static label that hid the current value from screen readers - useTopics: omit placeholderData from the accepted queryOptions so callers can't override the keep-previous-data behavior Co-Authored-By: Claude Opus 4.8 <[email protected]>
Contributor
Author
|
Some images from the change: Please note that the refresh picker of the consumer groups is also changed to better match the rest of the pickers |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
The Topics list reused 'topics-refresh-rate', which the topic-detail Consumer Groups sub-tab (TopicConsumerGroups) already uses for its lag-polling interval. Sharing the key made the two unrelated controls overwrite each other's value. Use a distinct 'topics-list-refresh-rate' key for the list page. Co-Authored-By: Claude Opus 4.8 <[email protected]>
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.
Closes #1009, closes #4027, closes #472
What changes did you make? (Give an overview)
Read-only list tables used to refetch on their own — most noticeably every time you switched away from the tab and came back (TanStack Query's default
refetchOnWindowFocus), which #1009 reported as random, unsolicited reloads. At the same time #4027/#472 asked for a manual refresh button and an opt-in auto-refresh. This PR resolves both by putting refreshing under the user's control:refetchOnWindowFocus: falseandrefetchOnReconnect: falseon the globalQueryClient(App.tsx).refetchOnMountis intentionally left at its default (true) so navigating to a page still loads fresh data.useRefreshRate(storageKey)hook that turns the persisted rate into arefetchInterval; list query hooks now accept that interval; each page's loader guard was changed fromisLoading || isRefetchingtoisLoadingso auto/manual refresh no longer blanks the whole table (data stays put viaplaceholderData).Is there anything you'd like reviewers to focus on?
refetchOnWindowFocus/refetchOnReconnect: falsechange inApp.tsxis app-wide by design, not just the list pages. Detail/metrics pages that previously freshened on tab-refocus now update on remount or via their own polling (e.g. the consumer-lag pollers set their ownrefetchInterval). KeepingrefetchOnMount: truewas a deliberate choice.useTopics) into a child component, so the control drives refresh viauseIsFetching/queryClient.refetchQueriesscoped withtype: 'active'on thetopicKeys.allprefix — this targets only the mounted list query, not cached topic-detail queries.How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
Manually: ran the frontend dev server against the
documentation/compose/kafbat-ui.yamlstack and verified on all four pages that the manual button refetches (spinner, no full-page blank), the interval selector auto-polls and "Off" stops it, and returning to the tab no longer triggers a refetch. Unit: added/updated Jest + RTL tests for the split button (incl. menu selection persisting the interval),useRefreshRate, the global query defaults (assertingrefetchOnMountis preserved), and each page's control; all suites pass locally.Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)
Check out Contributing and Code of Conduct
A picture of a cute animal (not mandatory but encouraged)
Summary by CodeRabbit
New Features
Improvements
Tests