fix(frontend): resolve the inbox flickering search issue #922
Conversation
WalkthroughThe changes replace the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as FilterTextfield
participant H as useSearch Hook
participant R as Router
U->>F: Types search input
F->>H: Triggers onChange (debounced)
H->>H: Updates searchText state and manages isActive state
H->>R: useEffect updates router query (updateQueryParams)
R-->>H: Acknowledges query update
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/hooks/useSearch.tsx (1)
81-85: Consider handling search text clearing.The effect correctly updates query params when searchText changes, but only if searchText is truthy. This means when a user clears the search field completely, the query parameter won't be updated.
useEffect(() => { - if (searchText) { updateQueryParams(searchText); - } }, [searchText]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/hooks/useSearch.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Frontend Tests
- GitHub Check: API-Tests
🔇 Additional comments (4)
frontend/src/hooks/useSearch.tsx (4)
60-60: Good addition of isActive state.Adding a state variable to track interaction with the search field is a clean approach to fixing the flickering issue. This will help control when the input field gets updated with the searchText value.
74-79: Good implementation of debounce for onSearch.Debouncing the onSearch function reduces the frequency of state updates while typing, which helps address the flickering issue. The 300ms delay is appropriate for search inputs.
Note that you're already using debounce for
updateQueryParams. Having both functions debounced creates a double delay effect, which might make the search feel slightly less responsive but will definitely fix the flickering.
107-116: Good implementation of textFieldProps with clever flickering prevention.The approach you've taken with the conditional value in textFieldProps is elegant:
- When the user interacts with the field (
isActiveis true), the input is allowed to be controlled by its internal state- When not interacting (
isActiveis false), the input shows the debouncedsearchTextvalueThis effectively prevents the field from resetting while typing, eliminating the flickering issue.
74-116:❓ Verification inconclusive
Verify this fix works across all browsers and interaction patterns.
The overall approach to fixing the flickering issue is solid. The combination of:
- Using debounce to limit state updates
- Managing input field control with the isActive state
- Decoupling the input field from direct router query synchronization
Should effectively resolve the flickering issue mentioned in PR #892, but it's worth testing across different browsers and interaction patterns to ensure it's consistently fixed.
🏁 Script executed:
#!/bin/bash # Check if this solution has browser-specific issues or compatibility concerns # Look for any browser-related issues in the codebase echo "Checking for browser-specific issues in useSearch or related components:" rg -i "browser|safari|firefox|chrome|edge" --glob "*.{tsx,ts}" -A 3 -B 3 "src/hooks/useSearch" || echo "No browser-specific issues found." # Check how this pattern might be used elsewhere echo "Checking for similar patterns to prevent flickering in other components:" rg "isActive.*setIsActive" --glob "*.{tsx,ts}" -A 3 -B 3 || echo "No similar patterns found."Length of output: 2209
Action Required: Ensure Comprehensive Cross-Browser Verification
The debounce and state management approach in
frontend/src/hooks/useSearch.tsxnow appears solid and consistent with similar patterns used elsewhere (e.g., inLegendItem.tsxandEmojiButton.tsx). Static analysis didn’t reveal any browser-specific code or potential compatibility issues. However, please perform manual testing across all target browsers and interaction scenarios to confirm that the flickering issue is resolved universally.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/hooks/useSearch.tsx (1)
66-66: Consider alternative todeleteoperatorThe
deleteoperator can impact performance as noted by static analysis.- delete query.search; + query.search = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/hooks/useSearch.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/hooks/useSearch.tsx
[error] 66-66: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: API-Tests
- GitHub Check: Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
frontend/src/hooks/useSearch.tsx (4)
60-60: Effective state management for flickering preventionAdding this state variable is a key part of fixing the inbox flickering issue. It helps control when the input field's value is synchronized with the state.
61-78: Good refinement of the query parameter handlingThe updated implementation more cleanly handles query parameter updates, especially the empty search case removal. This approach reduces unnecessary URL changes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
79-84: Excellent use of debouncing for search performanceImplementing debounce with a 300ms delay is a crucial improvement to prevent excessive re-renders during typing, directly addressing the flickering issue.
110-119: Smart solution for flickering preventionThis implementation cleverly controls input behavior by:
- Setting the value to
undefinedwhen active (letting the DOM handle the input value)- Using the controlled value only when not active (preventing input lag)
- Managing the active state with mouse events
This effectively prevents flickering by avoiding value synchronization during typing.
|
This PR works as expected the only thing that can be an issue we can only search if the cursor on search input which might lead to bad UX. If we can find a fix that would be awesome!. |
Thank you for the feedback 🙏 |
| @@ -105,5 +103,15 @@ export const useSearch = <T,>(params: TParamItem<T>) => { | |||
| }), | |||
| }, | |||
| }, | |||
| textFieldProps: { | |||
There was a problem hiding this comment.
What is the motive behind adding this prop?
There was a problem hiding this comment.
The main motivation is to group all the fields needed by the Search Text component in one object
|
When i re-tested this pr i found out that sometimes the flickering still happens. |
Thank you for the feedback, i have converted the PR to a draft status |
Motivation
The main motivation is to fix the inbox flickering search issue.
Fixes #892
Checklist:
Summary by CodeRabbit
FilterTextfieldcomponent for enhanced filtering capabilities.