fix: Replaced clear button with reset filters button #2703
Conversation
BadriVishalPadhy
requested review from
perkinsjr,
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@BadriVishalPadhy is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
BadriVishalPadhy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filter.tsx (1)
Line range hint
31-201: Consider unifying filter state managementThe current implementation manages each filter's visibility and value separately, which could lead to maintenance challenges as more filters are added. Consider implementing a more unified approach to filter state management.
Some suggestions:
- Create a single filter configuration object
- Implement a generic filter visibility toggle
- Use a reducer pattern for filter state management
Example approach:
type FilterState = { identifier: { visible: boolean; value: string[] }; success: { visible: boolean; value: boolean | null }; timeRange: { visible: boolean; after: Date | null; before: Date | null }; }; type FilterAction = | { type: 'RESET_ALL' } | { type: 'TOGGLE_VISIBILITY'; filter: keyof FilterState } | { type: 'UPDATE_VALUE'; filter: keyof FilterState; value: any };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filter.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/page.tsx(1 hunks)
🔇 Additional comments (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/page.tsx (1)
139-139: LGTM: Removal of redirect link fixes issue #2394
The removal of the filter reset link resolves the incorrect redirection to "/audit" page and consolidates the reset functionality to the "Reset Filters" button, providing a more consistent user experience.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filter.tsx (1)
119-119: LGTM: Button label accurately describes its action
The "Reset Filters" label better communicates the button's functionality of resetting all filters, improving user understanding.
| setSuccessVisible(false); | ||
| setSuccess(null); | ||
| setTimeRangeVisible(false); | ||
| setIdentifier(null); | ||
| startTransition(() => {}); | ||
| }} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize state transition handling
The startTransition with an empty callback is unnecessary. Consider batching the state updates within a single transition.
- setSuccessVisible(false);
- setSuccess(null);
- setTimeRangeVisible(false);
- setIdentifier(null);
- startTransition(() => {});
+ startTransition(() => {
+ setSuccessVisible(false);
+ setSuccess(null);
+ setTimeRangeVisible(false);
+ setIdentifier(null);
+ });📝 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.
| setSuccessVisible(false); | |
| setSuccess(null); | |
| setTimeRangeVisible(false); | |
| setIdentifier(null); | |
| startTransition(() => {}); | |
| }} | |
| > | |
| startTransition(() => { | |
| setSuccessVisible(false); | |
| setSuccess(null); | |
| setTimeRangeVisible(false); | |
| setIdentifier(null); | |
| }); | |
| }} | |
| > |
| setSuccessVisible(false); | ||
| setSuccess(null); | ||
| setTimeRangeVisible(false); |
There was a problem hiding this comment.
💡 Codebase verification
Reset logic is missing localTime state reset
The reset functionality should also clear the localTime state for complete consistency. Current implementation resets:
identifierVisiblesuccessVisiblesuccesstimeRangeVisibleidentifier
But misses:
localTimewhich is used to display the time range information
🔗 Analysis chain
Verify completeness of state reset logic
The state reset logic handles successVisible, success, and timeRangeVisible, but there might be other states that should be reset for consistency.
Let's verify all filter-related states are properly reset:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all state declarations and their reset patterns
rg -A 1 'const \[(.*?), set\1\]' apps/dashboard/app/\(app\)/ratelimits/\[namespaceId\]/logs/filter.tsx
Length of output: 310
Script:
#!/bin/bash
# Search for useState declarations
rg --pcre2 "const \[.+?, set.+?\] = useState" apps/dashboard/app/\(app\)/ratelimits/\[namespaceId\]/logs/filter.tsx
Length of output: 364
Script:
#!/bin/bash
# Let's look at the full context of the reset function to understand all state changes
rg -B 5 -A 5 "setSuccessVisible\(false\)" apps/dashboard/app/\(app\)/ratelimits/\[namespaceId\]/logs/filter.tsx
Length of output: 839
Script:
#!/bin/bash
# Let's check for any other state variables that might be used in the component
rg "setLocalTime|setIdentifier|setSuccess" apps/dashboard/app/\(app\)/ratelimits/\[namespaceId\]/logs/filter.tsx
Length of output: 942
BadriVishalPadhy
changed the title
BadriVishalPadhy
changed the title
BadriVishalPadhy
changed the title
BadriVishalPadhy
changed the title
BadriVishalPadhy
changed the title
|
Hey @chronark do check this |
| @@ -109,11 +109,14 @@ export const Filters: React.FC = () => { | |||
| className="flex items-center h-8 gap-2 bg-background-subtle" | |||
| onClick={() => { | |||
There was a problem hiding this comment.
Let's wrap the whole function in startTransition instead of using an empty one. Also, the 'Clear Filters' function needs to handle 'after' and 'before' dates - they currently aren't being reset when clearing filters.
onClick={() => {
startTransition(() => {
setIdentifierVisible(false);
setSuccessVisible(false);
setSuccess(null);
setTimeRangeVisible(false);
setIdentifier(null);
setAfter(null);
setBefore(null);
});
}}For the sake of readability let's create a handler function for that like handleResetFilters
There was a problem hiding this comment.
ok shall i do the change ?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filter.tsx (2)
Line range hint
149-149: Remove unnecessary empty startTransition callsThere are still empty startTransition calls that serve no purpose and should be removed.
Apply these changes:
- Remove empty startTransition after setIdentifier:
setIdentifier(v); -startTransition(() => {});
- Remove empty startTransition after setSuccess:
setSuccess(v === "true"); -startTransition(() => {});Also applies to: 182-182
111-119: Extract reset logic into a reusable functionThe reset logic is duplicated across multiple button handlers. Consider extracting it into a reusable function for better maintainability and consistency.
Create a handler function like this:
const handleResetFilters = (filterType?: 'all' | 'success' | 'timeRange') => { startTransition(() => { if (filterType === 'all' || !filterType) { setIdentifierVisible(false); setSuccessVisible(false); setSuccess(null); setTimeRangeVisible(false); setIdentifier(null); setAfter(null); setBefore(null); setLocalTime(""); } else if (filterType === 'success') { setSuccessVisible(false); setSuccess(null); } else if (filterType === 'timeRange') { setTimeRangeVisible(false); setAfter(null); setBefore(null); setLocalTime(""); } }); };Then use it in the button handlers:
// Reset all filters onClick={() => handleResetFilters()} // Reset success filter onClick={() => handleResetFilters('success')} // Reset time range filter onClick={() => handleResetFilters('timeRange')}Also applies to: 182-186, 215-220
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filter.tsx(1 hunks)
🔇 Additional comments (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/filter.tsx (2)
111-122: Implementation successfully addresses PR objectives
The changes effectively solve the original issue by:
- Preventing redirection to "/audit" page
- Providing comprehensive filter reset functionality
- Using clearer "Reset Filters" button text for better UX
111-119:
Add missing localTime state reset
The reset functionality should also clear the localTime state for complete consistency. While the implementation properly resets most states, it misses the localTime state which is used to display the time range information.
Apply this diff to include the localTime reset:
startTransition(() => {
setIdentifierVisible(false);
setSuccessVisible(false);
setSuccess(null);
setTimeRangeVisible(false);
setIdentifier(null);
setAfter(null);
setBefore(null);
+ setLocalTime("");
});Likely invalid or redundant comment.
|
@ogzhanolguncu check |
What does this PR do?
replaced Clear-button with reset-filters-button because the clear button function and reset-filters-button function is same .Also clear button was only clearing one item but now resets all.
Fixes #2394
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Screen.Recording.2024-12-04.at.19.36.04.mov
Summary by CodeRabbit