Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. To trigger a review, include You can disable this status message by setting the Use the checkbox below for a quick retry:
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.
🧹 Nitpick comments (2)
ui/advancedFilter/filters/AddressFilter.tsx (1)
41-56: Preferinterfaceovertypefor object shapes.The
InputPropsandAddressFilterdefinitions usetypebut should useinterfaceper coding guidelines.♻️ Suggested refactor
-type InputProps = { +interface InputProps { address?: string; mode?: AddressFilterMode; isLast: boolean; onModeChange: ({ value }: { value: Array<string> }) => void; onChange: (event: ChangeEvent<HTMLInputElement>) => void; onBlur: () => void; onClear: () => void; onAddFieldClick: () => void; isInvalid: boolean; -}; +} -type AddressFilter = { +interface AddressFilter { address: string; mode: AddressFilterMode; -}; +}As per coding guidelines: "Prefer interfaces over types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/advancedFilter/filters/AddressFilter.tsx` around lines 41 - 56, Replace the object-shape type aliases with interfaces: change the declarations for InputProps and AddressFilter to use the interface keyword (keep the same property names/signatures: address, mode, isLast, onModeChange, onChange, onBlur, onClear, onAddFieldClick, isInvalid for InputProps; address and mode for AddressFilter) and update any references/imports if needed so the rest of the file (components that accept InputProps and any code constructing AddressFilter) continues to compile unchanged.ui/advancedFilter/FilterByColumn.pw.tsx (1)
58-58: Avoid CSS class selectors in Playwright tests.The selector
.chakra-popover__contentuses a CSS class, which violates coding guidelines. Use a role, test ID, or accessible attribute instead.♻️ Suggested fix using role selector
- const popover = page.locator('.chakra-popover__content'); + const popover = page.getByRole('dialog');As per coding guidelines: "In Playwright tests, use roles, test IDs, and text content as selectors — never CSS class selectors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/advancedFilter/FilterByColumn.pw.tsx` at line 58, The test is using a CSS class selector for the popover (const popover = page.locator('.chakra-popover__content')), which violates the guideline; replace that locator with an accessible selector such as page.getByRole(...) or a test id for the popover (or use getByTestId/getByText as appropriate) so the popover variable is located via role/test-id/text instead of a CSS class; update the locator expression for the popover variable in FilterByColumn.pw.tsx to use the chosen role or data-testid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/advancedFilter/FilterByColumn.pw.tsx`:
- Line 58: The test is using a CSS class selector for the popover (const popover
= page.locator('.chakra-popover__content')), which violates the guideline;
replace that locator with an accessible selector such as page.getByRole(...) or
a test id for the popover (or use getByTestId/getByText as appropriate) so the
popover variable is located via role/test-id/text instead of a CSS class; update
the locator expression for the popover variable in FilterByColumn.pw.tsx to use
the chosen role or data-testid.
In `@ui/advancedFilter/filters/AddressFilter.tsx`:
- Around line 41-56: Replace the object-shape type aliases with interfaces:
change the declarations for InputProps and AddressFilter to use the interface
keyword (keep the same property names/signatures: address, mode, isLast,
onModeChange, onChange, onBlur, onClear, onAddFieldClick, isInvalid for
InputProps; address and mode for AddressFilter) and update any
references/imports if needed so the rest of the file (components that accept
InputProps and any code constructing AddressFilter) continues to compile
unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
ui/advancedFilter/__screenshots__/FilterByColumn.pw.tsx_dark-color-mode_from-filter-dark-mode-1.pngis excluded by!**/*.pngui/advancedFilter/__screenshots__/FilterByColumn.pw.tsx_dark-color-mode_to-filter-dark-mode-1.pngis excluded by!**/*.pngui/advancedFilter/__screenshots__/FilterByColumn.pw.tsx_default_from-filter-dark-mode-1.pngis excluded by!**/*.pngui/advancedFilter/__screenshots__/FilterByColumn.pw.tsx_default_to-filter-dark-mode-1.pngis excluded by!**/*.png
📒 Files selected for processing (2)
ui/advancedFilter/FilterByColumn.pw.tsxui/advancedFilter/filters/AddressFilter.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ui/advancedFilter/FilterByColumn.pw.tsx (1)
58-58: Avoid CSS class selectors in Playwright tests.The selector
.chakra-popover__contentuses a CSS class, which violates the testing guidelines. Consider using a role-based selector or adding adata-testidto the popover content.Suggested fix using role selector
- const popover = page.locator('.chakra-popover__content'); + const popover = page.getByRole('dialog');If the popover doesn't have dialog semantics, consider adding a test ID to the component:
- const popover = page.locator('.chakra-popover__content'); + const popover = page.getByTestId('filter-popover-content');As per coding guidelines: "In Playwright tests, use roles, test IDs, and text content as selectors — never CSS class selectors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/advancedFilter/FilterByColumn.pw.tsx` at line 58, The test currently uses a CSS class selector in the popover locator (the popover variable assigned with page.locator('.chakra-popover__content')), which violates Playwright selector guidelines; update the test to use a semantic selector (e.g., role-based locator like page.getByRole('dialog') or page.locator('role=dialog') / page.getByRole('dialog', { name: ... }) if the popover exposes dialog semantics) or change the component to add a stable data-testid (e.g., data-testid="filter-popover") and then locate it with page.getByTestId('filter-popover') instead of the '.chakra-popover__content' selector so the popover variable no longer relies on class names.ui/advancedFilter/filters/AddressFilter.tsx (1)
161-162: Consider renamingisTouchedtohasChangesorisModifiedfor clarity.The variable on line 162 checks whether the current values differ from the original
valueprop, not whether fields have been "touched" (blurred). This naming could cause confusion with thetouchedstate array which tracks actual user interaction. A name likehasChangesorisModifiedwould better convey the intent.Suggested rename for clarity
- const isTouched = !isEqual(currentValue.filter(i => i.address).map(addressFilterToKey).sort(), value.map(addressFilterToKey).sort()); + const hasChanges = !isEqual(currentValue.filter(i => i.address).map(addressFilterToKey).sort(), value.map(addressFilterToKey).sort()); return ( <TableColumnFilter title={ type === 'from' ? 'From address' : 'To address' } isFilled={ Boolean(currentValue[0].address) } - isTouched={ isTouched && !hasErrors } + isTouched={ hasChanges && !hasErrors } onFilter={ onFilter }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/advancedFilter/filters/AddressFilter.tsx` around lines 161 - 162, Rename the misleading variable isTouched in AddressFilter to a clearer name like hasChanges (or isModified) because it compares currentValue vs value rather than tracking user touch state; update the declaration that computes the boolean (the line using currentValue.filter(i => i.address).map(addressFilterToKey).sort() and value.map(addressFilterToKey).sort() with isEqual) and replace all usages of isTouched throughout the AddressFilter component (including places that read it to enable/disable buttons or pass into child components) to the new identifier, preserving the exact boolean logic and imports for currentValue, value, addressFilterToKey and isEqual.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/advancedFilter/FilterByColumn.pw.tsx`:
- Line 58: The test currently uses a CSS class selector in the popover locator
(the popover variable assigned with page.locator('.chakra-popover__content')),
which violates Playwright selector guidelines; update the test to use a semantic
selector (e.g., role-based locator like page.getByRole('dialog') or
page.locator('role=dialog') / page.getByRole('dialog', { name: ... }) if the
popover exposes dialog semantics) or change the component to add a stable
data-testid (e.g., data-testid="filter-popover") and then locate it with
page.getByTestId('filter-popover') instead of the '.chakra-popover__content'
selector so the popover variable no longer relies on class names.
In `@ui/advancedFilter/filters/AddressFilter.tsx`:
- Around line 161-162: Rename the misleading variable isTouched in AddressFilter
to a clearer name like hasChanges (or isModified) because it compares
currentValue vs value rather than tracking user touch state; update the
declaration that computes the boolean (the line using currentValue.filter(i =>
i.address).map(addressFilterToKey).sort() and
value.map(addressFilterToKey).sort() with isEqual) and replace all usages of
isTouched throughout the AddressFilter component (including places that read it
to enable/disable buttons or pass into child components) to the new identifier,
preserving the exact boolean logic and imports for currentValue, value,
addressFilterToKey and isEqual.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
ui/advancedFilter/__screenshots__/FilterByColumn.pw.tsx_dark-color-mode_from-filter-dark-mode-1.pngis excluded by!**/*.pngui/advancedFilter/__screenshots__/FilterByColumn.pw.tsx_dark-color-mode_to-filter-dark-mode-1.pngis excluded by!**/*.pngui/advancedFilter/__screenshots__/FilterByColumn.pw.tsx_default_from-filter-dark-mode-1.pngis excluded by!**/*.pngui/advancedFilter/__screenshots__/FilterByColumn.pw.tsx_default_to-filter-dark-mode-1.pngis excluded by!**/*.png
📒 Files selected for processing (2)
ui/advancedFilter/FilterByColumn.pw.tsxui/advancedFilter/filters/AddressFilter.tsx
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.
Description and Related Issue(s)
resolves #2463
Checklist for PR author