EDM-3204: Do not allow invalid search values for name/alias fields#579
Open
celdrake wants to merge 1 commit intoflightctl:mainfrom
Open
EDM-3204: Do not allow invalid search values for name/alias fields#579celdrake wants to merge 1 commit intoflightctl:mainfrom
celdrake wants to merge 1 commit intoflightctl:mainfrom
Conversation
WalkthroughRefactors TableTextSearch to add internal state and validation preventing spaces in search input, exposes setValue to parent, and shows a localized error message. Adds a new English translation key. Adds aria-checked attributes to two device table switches for accessibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant TableSearch as TableTextSearch
participant SearchInput as SearchInput
participant Parent as ParentComponent
participant I18n as i18n
User->>SearchInput: type input
SearchInput->>TableSearch: onChange(value)
TableSearch->>TableSearch: validate (contains spaces?)
alt contains spaces
TableSearch->>TableSearch: set hasError = true, actualValue = value
TableSearch->>I18n: get "Search value cannot contain spaces"
TableSearch->>SearchInput: render error (HelperText)
TableSearch-->>Parent: (do not call setValue with invalid value)
else valid
TableSearch->>TableSearch: set hasError = false, actualValue = value
TableSearch->>Parent: setValue(value)
end
User->>TableSearch: click clear
TableSearch->>TableSearch: handleClear -> actualValue = '', hasError = false
TableSearch->>Parent: setValue('')
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ui-components/src/components/Table/TableTextSearch.tsx (1)
15-21: Consider syncing with all external value changes, not just empty.The effect only resets state when
value === ''. If a parent component ever sets a non-empty value programmatically,actualValuewon't sync. Current usage patterns only clear to empty, so this works, but for robustness consider:Optional: Full sync with external value
React.useEffect(() => { - // When the value is cleared externally, reset the component state - if (value === '') { - setActualValue(''); + // Sync with external value changes + if (value !== undefined) { + setActualValue(value); setHasError(false); } }, [value]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Table/TableTextSearch.tsx` around lines 15 - 21, The effect in TableTextSearch.tsx currently only resets state when value === '' causing actualValue to become out of sync when a parent sets a non-empty value; update the React.useEffect watching value to synchronize actualValue with value on any external change (e.g., if value !== actualValue then call setActualValue(value)) and clear validation state as appropriate (setHasError(false) or similar) so the component mirrors external updates; target the useEffect that references value, and the state setters setActualValue and setHasError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/ui-components/src/components/Table/TableTextSearch.tsx`:
- Around line 48-58: The SearchInput spread order allows callers to override
internal handlers: move the {...rest} spread before the explicit props in
TableTextSearch so internal handlers win; specifically, in the TableTextSearch
return JSX place {...rest} before aria-label={placeholder},
onChange={handleChange}, value={actualValue}, placeholder={placeholder}, and
onClear={handleClear} so handleClear continues to reset error state and
handleChange/other props aren’t accidentally overridden.
---
Nitpick comments:
In `@libs/ui-components/src/components/Table/TableTextSearch.tsx`:
- Around line 15-21: The effect in TableTextSearch.tsx currently only resets
state when value === '' causing actualValue to become out of sync when a parent
sets a non-empty value; update the React.useEffect watching value to synchronize
actualValue with value on any external change (e.g., if value !== actualValue
then call setActualValue(value)) and clear validation state as appropriate
(setHasError(false) or similar) so the component mirrors external updates;
target the useEffect that references value, and the state setters setActualValue
and setHasError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 007c4259-e342-40c9-af41-956b7062e93a
📒 Files selected for processing (2)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Table/TableTextSearch.tsx
celdrake
force-pushed
the
EDM-3204-invalid-search
branch
from
d232beb to
f032c85
Compare
celdrake
commented
Mar 18, 2026
| <Switch | ||
| id="decommissioned-devices-switch" | ||
| label={<span className="fctl-switch__label">{t('Show decommissioned devices')}</span>} | ||
| aria-checked |
Collaborator
Author
There was a problem hiding this comment.
Adds the required accessibility field for checkboxes (same for the enrolledDevicesTable)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/ui-components/src/components/Table/TableTextSearch.tsx (1)
15-21: External non-empty value changes are not synchronized to internal state.The
useEffectonly resetsactualValuewhenvalue === ''. When a parent component likeFleetsPageupdates the value prop to a non-empty string (e.g., via URL search parameters),actualValuewon't update to reflect it. This causes the input to display a stale value while the parent state has moved on.For example, in
FleetsPage, when the URL changes from?name=abcto?name=xyz, thenameprop updates butactualValueremains at'abc', creating a desynchronization.Consider syncing on all external value changes:
Suggested refactor
React.useEffect(() => { - // When the value is cleared externally, reset the component state - if (value === '') { - setActualValue(''); - setHasError(false); - } + // Sync internal state when external value changes + if (value !== undefined && value !== actualValue) { + setActualValue(value); + setHasError(value.includes(' ')); + } }, [value]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Table/TableTextSearch.tsx` around lines 15 - 21, The effect in the TableTextSearch component only resets internal state when value === '' so external non-empty updates (e.g., from FleetsPage/URL params) aren't reflected; change the useEffect that currently watches [value] to setActualValue(value) on any change (and also setHasError(false) when appropriate) so the internal actualValue always syncs to the incoming prop value instead of only clearing on empty strings; update the effect referencing actualValue/setActualValue/hasError/setHasError in TableTextSearch accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/ui-components/src/components/Table/TableTextSearch.tsx`:
- Around line 15-21: The effect in the TableTextSearch component only resets
internal state when value === '' so external non-empty updates (e.g., from
FleetsPage/URL params) aren't reflected; change the useEffect that currently
watches [value] to setActualValue(value) on any change (and also
setHasError(false) when appropriate) so the internal actualValue always syncs to
the incoming prop value instead of only clearing on empty strings; update the
effect referencing actualValue/setActualValue/hasError/setHasError in
TableTextSearch accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc9e1bc6-d8cf-434f-9c6e-5f4ec8e12c37
📒 Files selected for processing (4)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Device/DevicesPage/DecommissionedDevicesTable.tsxlibs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsxlibs/ui-components/src/components/Table/TableTextSearch.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/i18n/locales/en/translation.json
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.
Fields for "search by name" should not accept spaces as the API responses return error 400.
Summary by CodeRabbit