feat: implement sorting functionality in OrdersGrid with pagination a… - #5
Merged
Conversation
…nd update related components
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds full sorting, filtering, and pagination support to the Order Grid: new sorting utilities and types, filter normalization and UI (text/select/date), presenter to build a view model, toolbar and filter bar UI, updated grid head/body to surface sort state and empty messages, and service-level sorting/filtering/pagination. Changes
Sequence DiagramsequenceDiagram
actor User
participant Server as OrderGridWithPagination (Server)
participant Presenter as Presenter
participant Service as Order Service
participant Client as OrderGridWithPaginationClient (Client)
participant Toolbar as OrdersGridToolbar
participant FiltersBar as FiltersBar
participant Grid as OrdersGrid
User->>Server: request page with query params
Server->>Presenter: buildOrderGridWithPaginationViewModel(searchParams)
Presenter->>Presenter: resolveOrderGridQuery(searchParams)
Presenter->>Service: getPaginatedOrdersForGrid(page, sortBy, sortDir, filters)
Service->>Service: filterOrders(...) -> getSortedOrders(...)
Service-->>Presenter: paginated sorted results + prev/next pages
Presenter->>Presenter: buildOrderGridSortLinks(sortBy, sortDir, filters)
Presenter-->>Server: viewModel{orders, filters, pagination, sortState}
Server->>Client: render client with viewModel
Client->>Toolbar: render toolbar (pagination + filter toggle)
User->>Toolbar: click Filter
Toolbar->>Client: toggleFilters()
Client->>FiltersBar: render FiltersBar (formState from controller)
User->>FiltersBar: change fields, click Aplicar
FiltersBar->>Client: applyFilters() -> navigate(newUrl)
Client->>Server: new request with updated query params
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/orders/orders.service.ts (1)
31-40:⚠️ Potential issue | 🟠 MajorNormalize pagination inputs to prevent invalid page math.
Lines 31-40 use
pageSizeandrequestedPagedirectly. A non-positive or fractionalpageSizecan produce broken metadata (totalPages/slice boundaries), and fractional pages can yield ambiguous slicing.Proposed change
export const getPaginatedOrdersForGrid = async ( requestedPage: number, pageSize = DEFAULT_PAGE_SIZE, sortBy: OrdersSortBy = DEFAULT_ORDERS_SORT_BY, sortDir: OrdersSortDir = DEFAULT_ORDERS_SORT_DIR, ): Promise<PaginatedOrdersResult> => { const orders = getSortedOrders(await findAllOrders(), sortBy, sortDir); - const safeRequestedPage = Number.isFinite(requestedPage) ? requestedPage : 1; - const totalPages = Math.max(1, Math.ceil(orders.length / pageSize)); + const safePageSize = + Number.isFinite(pageSize) && pageSize > 0 ? Math.floor(pageSize) : DEFAULT_PAGE_SIZE; + const safeRequestedPage = + Number.isFinite(requestedPage) && requestedPage > 0 + ? Math.floor(requestedPage) + : 1; + const totalPages = Math.max(1, Math.ceil(orders.length / safePageSize)); const currentPage = Math.min(Math.max(1, safeRequestedPage), totalPages); - const startIndex = (currentPage - 1) * pageSize; - const items = orders.slice(startIndex, startIndex + pageSize); + const startIndex = (currentPage - 1) * safePageSize; + const items = orders.slice(startIndex, startIndex + safePageSize);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/orders/orders.service.ts` around lines 31 - 40, Normalize pagination inputs before computing totals: validate and coerce pageSize and requestedPage to positive integers (e.g., set pageSize = Math.floor(Number(pageSize)) and if pageSize < 1 set to DEFAULT_PAGE_SIZE; set requestedPage = Math.floor(Number(requestedPage)) and if requestedPage < 1 set to 1) before calling getSortedOrders/findAllOrders and before computing totalPages, startIndex, slice bounds; update code around the parameters and variables (pageSize, requestedPage, currentPage) in the function that returns PaginatedOrdersResult so rounding/guarding prevents fractional or non-positive page math.
🧹 Nitpick comments (1)
components/OrderGrid/__tests__/order-grid.navigation.test.ts (1)
1-4: Prefer a partial mock to keep this test resilient.Line 1 fully replaces
@/lib/orders/orders.sort. Iforder-grid.navigation.tslater imports another export from that module, this test can fail for unrelated reasons. Keep real exports and mock onlygetDefaultSortDirForField.Proposed change
-jest.mock('@/lib/orders/orders.sort', () => ({ - getDefaultSortDirForField: (field: string) => - field === 'timestamp' ? 'desc' : 'asc', -})); +jest.mock('@/lib/orders/orders.sort', () => { + const actual = jest.requireActual('@/lib/orders/orders.sort'); + return { + ...actual, + getDefaultSortDirForField: (field: string) => + field === 'timestamp' ? 'desc' : 'asc', + }; +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/OrderGrid/__tests__/order-grid.navigation.test.ts` around lines 1 - 4, The test currently fully replaces the module '@/lib/orders/orders.sort', which is brittle; update the mock to use the real module and only override getDefaultSortDirForField by calling jest.requireActual('@/lib/orders/orders.sort') (or jest.requireActual with the same specifier) and spreading its exports, then replace only getDefaultSortDirForField with a mocked implementation that returns 'desc' for 'timestamp' and 'asc' otherwise; this preserves other real exports while mocking just the targeted function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/OrderGrid/order-grid.query.ts`:
- Around line 22-23: The page parsing accepts partial numeric strings (e.g.,
"2abc") because Number.parseInt is permissive; change the logic around
parsedPage/requestedPage to first validate query.page with a strict integer
check (e.g., regex /^\d+$/) and only then parse it, defaulting to 1 for anything
that fails validation; update the variables referenced (parsedPage,
requestedPage) so parsedPage is derived from a validated numeric string and
requestedPage uses Math.max(1, parsedPage) as before.
In `@components/OrderGrid/OrderGridWithPagination.tsx`:
- Around line 23-24: The parent component's searchParams type is missing sortBy
and sortDir causing OrderGridWithPagination's resolveOrderGridQuery (which calls
resolveOrdersSortBy and resolveOrdersSortDir) to receive undefined and reset
sorting; update the parent's HomeProps searchParams type in app/page.tsx to
include optional sortBy?: string and sortDir?: string so the values are passed
through to OrderGridWithPagination and preserve user-selected sorting state.
In `@components/OrderGrid/parts/Head.tsx`:
- Around line 16-23: The header currently sets aria-sort for every column via
the computed ariaSort variable (using isActive and sortState), which should be
applied only to the actively sorted column; update the rendering in the
component that returns the <th> (referencing isActive, sortState, ariaSort and
column.key) so that the aria-sort attribute is omitted unless isActive is true
(e.g. conditionally add the attribute or spread an object only when isActive),
ensuring only the active column exposes aria-sort.
In `@lib/orders/orders.sort.ts`:
- Around line 50-51: The comparator for the 'timestamp' case in orders.sort.ts
currently does new Date(a.timestamp).getTime() - new Date(b.timestamp).getTime()
and can produce NaN if either timestamp is invalid; update the comparator (and
the other identical occurrence around the later block referenced) to parse the
timestamps (Date.parse or new Date(...).getTime()), check isNaN for each parsed
value, and replace invalid values with a deterministic fallback (e.g.,
Number.NEGATIVE_INFINITY or 0 depending on desired sort direction) before
subtracting so the comparator always returns a finite number; locate the
'timestamp' case and the duplicated block and apply the same guard logic to
both.
---
Outside diff comments:
In `@lib/orders/orders.service.ts`:
- Around line 31-40: Normalize pagination inputs before computing totals:
validate and coerce pageSize and requestedPage to positive integers (e.g., set
pageSize = Math.floor(Number(pageSize)) and if pageSize < 1 set to
DEFAULT_PAGE_SIZE; set requestedPage = Math.floor(Number(requestedPage)) and if
requestedPage < 1 set to 1) before calling getSortedOrders/findAllOrders and
before computing totalPages, startIndex, slice bounds; update code around the
parameters and variables (pageSize, requestedPage, currentPage) in the function
that returns PaginatedOrdersResult so rounding/guarding prevents fractional or
non-positive page math.
---
Nitpick comments:
In `@components/OrderGrid/__tests__/order-grid.navigation.test.ts`:
- Around line 1-4: The test currently fully replaces the module
'@/lib/orders/orders.sort', which is brittle; update the mock to use the real
module and only override getDefaultSortDirForField by calling
jest.requireActual('@/lib/orders/orders.sort') (or jest.requireActual with the
same specifier) and spreading its exports, then replace only
getDefaultSortDirForField with a mocked implementation that returns 'desc' for
'timestamp' and 'asc' otherwise; this preserves other real exports while mocking
just the targeted function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70efe348-0fea-412a-be04-d3f7b6e0f0e5
⛔ Files ignored due to path filters (1)
public/assets/add_box_60dp.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
components/OrderGrid/OrderGrid.styles.csscomponents/OrderGrid/OrderGridWithPagination.tsxcomponents/OrderGrid/OrderGridWithPaginationClient.tsxcomponents/OrderGrid/__tests__/OrderGrid.test.tsxcomponents/OrderGrid/__tests__/OrderGridWithPaginationClient.test.tsxcomponents/OrderGrid/__tests__/order-grid.navigation.test.tscomponents/OrderGrid/__tests__/order-grid.query.test.tscomponents/OrderGrid/columns.tscomponents/OrderGrid/index.tsxcomponents/OrderGrid/order-grid.navigation.tscomponents/OrderGrid/order-grid.query.tscomponents/OrderGrid/parts/Head.tsxcomponents/OrderGrid/types.tslib/orders/__tests__/orders.service.test.tslib/orders/orders.constants.tslib/orders/orders.service.tslib/orders/orders.sort.tslib/orders/orders.sort.types.ts
|
✅ Created PR with unit tests: #6 |
…proved type safety
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
components/OrderGrid/parts/ActionButton.tsx (1)
3-8: Avoid enabled-but-inert buttons when no handler is provided.With optional
onClick, the button is still rendered as actionable on Line 17. Consider auto-disabling whenonClickis missing (or makeonClickrequired).Suggested patch
type OrdersGridActionButtonProps = { label: string; iconAlt: string; iconSrc: string; onClick?: () => void; }; @@ export const OrdersGridActionButton = ({ label, iconAlt, iconSrc, onClick, }: OrdersGridActionButtonProps) => { + const isDisabled = !onClick; return ( <button aria-label={label} className="orders-grid__action-button" + disabled={isDisabled} onClick={onClick} type="button" >Also applies to: 17-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/OrderGrid/parts/ActionButton.tsx` around lines 3 - 8, The prop type OrdersGridActionButtonProps allows an optional onClick which results in an enabled-but-inert button; update the ActionButton component that renders this prop (the button at the rendering around the previous Line 17) to either make onClick required in OrdersGridActionButtonProps or, preferred, keep it optional and set the rendered <button> to disabled when onClick is missing (e.g., disabled={!onClick}) and avoid attaching pointer/click handlers when undefined; also ensure any CSS/class that indicates interactivity is removed for the disabled state so the button is not perceived as actionable when no handler exists.components/OrderGrid/parts/__tests__/Toolbar.test.tsx (1)
5-21: Add click-wiring assertions for toolbar actions.This test validates presence, but not that
onCreateOrderClick/onFilterClickare invoked. Adding click assertions here would harden the new toolbar integration path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/OrderGrid/parts/__tests__/Toolbar.test.tsx` around lines 5 - 21, The test currently only checks that OrdersGridToolbar renders UI elements; add assertions that onCreateOrderClick and onFilterClick are invoked by passing jest.fn() mocks as props to OrdersGridToolbar, simulate user clicks on the "Criar ordem" and "Filtro" buttons (using userEvent.click or fireEvent) and expect the corresponding mock functions to have been called (e.g., expect(onCreateOrderClick).toHaveBeenCalled()). Ensure you import and use userEvent (or fireEvent) and reference the component name OrdersGridToolbar and prop names onCreateOrderClick and onFilterClick to locate where to update the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/OrderGrid/OrderGrid.styles.css`:
- Around line 38-47: The Biome CSS parser is failing on Tailwind `@apply` usages
(e.g., selectors .orders-grid__sort-link, .orders-grid__sort-link--active,
.orders-grid__sort-indicator) because Tailwind directives are not enabled; open
your biome.json and under the css.parser settings add "tailwindDirectives": true
to enable parsing of `@apply` and other Tailwind directives, then re-run the
linter/formatter to confirm the parse errors across the OrdersGrid stylesheet
are resolved.
In `@styles/components/layout.css`:
- Around line 10-12: The .home__title rule in styles/components/layout.css
(which only contains "@apply mb-8;") is unused; either delete that CSS rule or
add the class to the H2 element in app/page.tsx that renders "Gerenciamento de
ordens" (add className="home__title" to that H2). If you keep the `@apply` rule
and want to avoid the Biome static analysis warning about Tailwind syntax,
configure Biome to ignore Tailwind-processed CSS or enable a Tailwind-aware
parser for CSS files so the `@apply` directive is accepted.
---
Nitpick comments:
In `@components/OrderGrid/parts/__tests__/Toolbar.test.tsx`:
- Around line 5-21: The test currently only checks that OrdersGridToolbar
renders UI elements; add assertions that onCreateOrderClick and onFilterClick
are invoked by passing jest.fn() mocks as props to OrdersGridToolbar, simulate
user clicks on the "Criar ordem" and "Filtro" buttons (using userEvent.click or
fireEvent) and expect the corresponding mock functions to have been called
(e.g., expect(onCreateOrderClick).toHaveBeenCalled()). Ensure you import and use
userEvent (or fireEvent) and reference the component name OrdersGridToolbar and
prop names onCreateOrderClick and onFilterClick to locate where to update the
test.
In `@components/OrderGrid/parts/ActionButton.tsx`:
- Around line 3-8: The prop type OrdersGridActionButtonProps allows an optional
onClick which results in an enabled-but-inert button; update the ActionButton
component that renders this prop (the button at the rendering around the
previous Line 17) to either make onClick required in OrdersGridActionButtonProps
or, preferred, keep it optional and set the rendered <button> to disabled when
onClick is missing (e.g., disabled={!onClick}) and avoid attaching pointer/click
handlers when undefined; also ensure any CSS/class that indicates interactivity
is removed for the disabled state so the button is not perceived as actionable
when no handler exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64ecf9c6-2fa1-45fb-8212-a1f162ae9312
⛔ Files ignored due to path filters (1)
public/assets/filter_list_60dp.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
app/page.tsxcomponents/OrderGrid/OrderGrid.styles.csscomponents/OrderGrid/OrderGridWithPaginationClient.tsxcomponents/OrderGrid/parts/ActionButton.tsxcomponents/OrderGrid/parts/Toolbar.tsxcomponents/OrderGrid/parts/__tests__/ActionButton.test.tsxcomponents/OrderGrid/parts/__tests__/Toolbar.test.tsxstyles/base.cssstyles/components/layout.css
✅ Files skipped from review due to trivial changes (1)
- components/OrderGrid/parts/tests/ActionButton.test.tsx
Comment on lines
+38
to
+47
| .orders-grid__sort-link { | ||
| @apply inline-flex w-full items-center justify-between gap-2 text-white no-underline; | ||
| } | ||
|
|
||
| .orders-grid__sort-link--active { | ||
| @apply font-semibold; | ||
| } | ||
|
|
||
| .orders-grid__sort-indicator { | ||
| @apply text-grid-xm; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Biome config files =="
fd -HI 'biome\.jsonc?$' .
CONFIG="$(fd -HI 'biome\.jsonc?$' . | head -n1 || true)"
if [ -n "${CONFIG:-}" ]; then
echo
echo "== Tailwind-related parser settings in ${CONFIG} =="
rg -n -C2 'tailwindDirectives|css|parser' "$CONFIG" || true
else
echo "No biome.json/biome.jsonc found."
fi
echo
echo "== Tailwind `@apply` usage in components/OrderGrid/OrderGrid.styles.css =="
rg -n -C1 '@apply' components/OrderGrid/OrderGrid.styles.cssRepository: atelesjr/base-orders
Length of output: 3036
Enable Tailwind directives in Biome config to stop parse failures on these CSS rules.
The Biome CSS parser is not configured to support Tailwind's @apply directive. Add "tailwindDirectives": true to the css.parser settings in biome.json to resolve parse errors being reported across lines 39, 43, 47, 79, 83, 87, 95, and 125 (30+ uses of @apply throughout this file).
🧰 Tools
🪛 Biome (2.4.6)
[error] 39-39: Tailwind-specific syntax is disabled.
(parse)
[error] 43-43: Tailwind-specific syntax is disabled.
(parse)
[error] 47-47: Tailwind-specific syntax is disabled.
(parse)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/OrderGrid/OrderGrid.styles.css` around lines 38 - 47, The Biome
CSS parser is failing on Tailwind `@apply` usages (e.g., selectors
.orders-grid__sort-link, .orders-grid__sort-link--active,
.orders-grid__sort-indicator) because Tailwind directives are not enabled; open
your biome.json and under the css.parser settings add "tailwindDirectives": true
to enable parsing of `@apply` and other Tailwind directives, then re-run the
linter/formatter to confirm the parse errors across the OrdersGrid stylesheet
are resolved.
- Added filtering capabilities for orders based on id, instrument, status, side, and date. - Introduced a new date picker component for selecting date filters. - Created a filter select component for status and side filters. - Implemented a filters bar to manage filter states and actions. - Enhanced the order grid query to include filters in the request. - Updated the order service to apply filters before sorting and pagination. - Added tests for the new filtering functionality and components. - Refactored existing components to accommodate the new filtering system.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
components/OrderGrid/OrderGrid.styles.css (1)
38-259:⚠️ Potential issue | 🟠 MajorBiome parse failures remain due to disabled Tailwind directives.
Line 39 and multiple subsequent
@applydeclarations are still parse-failing under Biome. This is the same unresolved root cause: Tailwind directives must be enabled in Biome config (css.parser.tailwindDirectives: true), otherwise CI/lint remains broken for this stylesheet.#!/bin/bash set -euo pipefail CONFIG="$(fd -HI 'biome\.jsonc?$' . | head -n1 || true)" if [ -z "${CONFIG:-}" ]; then echo "No biome.json/biome.jsonc found" exit 1 fi echo "Biome config: $CONFIG" rg -n -C2 'css|parser|tailwindDirectives' "$CONFIG" || true echo echo "Tailwind `@apply` usage in stylesheet:" rg -n '@apply' components/OrderGrid/OrderGrid.styles.css🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/OrderGrid/OrderGrid.styles.css` around lines 38 - 259, The stylesheet components/OrderGrid/OrderGrid.styles.css is failing Biome parsing because Tailwind `@apply` directives are disabled; open your Biome config (biome.json or biome.jsonc) and add or update the css.parser.tailwindDirectives setting to true (ensure the css and parser objects exist and merge this key rather than replacing unrelated config), save and commit the config change so Biome will accept the `@apply` rules used by classes like orders-grid__filter-field and orders-grid__date-picker.
🧹 Nitpick comments (1)
components/OrderGrid/parts/FilterSelect.tsx (1)
65-90: Align ARIA roles with actual interaction model.Line 65-75 declares
listbox/option, but interaction is implemented as nested buttons with tab/enter behavior, not listbox keyboard semantics. This can confuse assistive tech users.♿ Suggested direction
- <ul - aria-labelledby={id} - className="orders-grid__filter-select-menu" - role="listbox" - > + <ul + aria-labelledby={id} + className="orders-grid__filter-select-menu" + role="menu" + > {options.map((option) => ( <li - aria-selected={option.value === value} key={option.value} - role="option" + role="none" > <button + aria-checked={option.value === value} className={`orders-grid__filter-select-option ${ option.value === value ? 'orders-grid__filter-select-option--selected' : '' }`.trim()} + role="menuitemradio"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/OrderGrid/parts/FilterSelect.tsx` around lines 65 - 90, The component FilterSelect uses listbox/option ARIA roles but implements interaction as clickable buttons; update the ARIA to match the actual model by changing the container role from "listbox" to "menu" and each item role from "option" to "menuitem" (remove aria-selected on items), and keep the existing click handlers (onChange and setIsOpen) intact in the FilterSelect component so assistive tech sees a menu/menuitem pattern rather than a listbox; ensure the container still references id via aria-labelledby if needed and that keyboard behavior remains handled by the existing buttons or is adjusted to support menu semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/OrderGrid/order-grid.query.ts`:
- Around line 14-23: Update the OrderGridQueryParams type to allow string |
string[] | undefined for each field (instead of only string) to match Next.js
searchParams, and at the resolver entry point (where searchParams is received)
coerce each param to a single string by taking the first element when it's an
array (e.g., for id, instrument, status, sortBy, sortDir, side, date): perform
Array.isArray checks and assign the first value or undefined so downstream code
receives plain strings/numbers and not arrays; keep the symbol
OrderGridQueryParams and apply normalization immediately in the resolver that
consumes searchParams.
In `@components/OrderGrid/OrderGridWithPaginationClient.tsx`:
- Around line 46-56: The emptyStateMessage computed in the useMemo
(emptyStateMessage) assumes filters are active in its final fallback; change the
logic to first detect whether any filter is actually set (e.g., derive
hasActiveFilters via checking filters fields or
Object.values(filters).some(Boolean)) and only return "Nenhuma ordem encontrada
para os filtros aplicados." when hasActiveFilters is true—otherwise return a
neutral empty-account message like "Nenhuma ordem encontrada." Keep the checks
for filters.instrument and filters.id, keep the memo dependency on filters, and
update the fallback branch to use the new hasActiveFilters condition.
In `@components/OrderGrid/parts/FilterDatePicker.tsx`:
- Around line 146-148: The dialog created in FilterDatePicker (the div with
className "orders-grid__date-picker-popover" and role="dialog") is missing an
accessible name; update that element to include an accessible label by adding
either aria-label using the existing ariaLabel prop or aria-labelledby
referencing the month heading (monthLabel element) so assistive tech announces
the dialog name; ensure the month heading has an id if you use aria-labelledby
and reuse the visible <strong>{monthLabel}</strong> as the label source.
In `@components/OrderGrid/parts/FilterSelect.tsx`:
- Around line 43-60: selectedOption can be undefined and the hidden input still
submits the original invalid value; update FilterSelect to guard against empty
or missing matches by deriving selectedOption safely from options (e.g., const
selectedOption = options.find(o => o.value === value) ?? options[0] ?? { value:
'', label: '' }) and use selectedOption.value for the hidden input value and
selectedOption.label for the UI; ensure functions/JSX that reference
selectedOption (the span, the hidden input name/value, and any onClick handlers
relying on it) handle the empty fallback so the component never reads
.label/.value on undefined.
In `@lib/orders/orders.filter.ts`:
- Around line 45-51: The date filtering is inconsistent between UTC and local
time: update getTimestampDateKey to return the calendar day in the browser's
local timezone (use parsedDate.getFullYear(), parsedDate.getMonth()+1,
parsedDate.getDate() to produce YYYY-MM-DD) and update normalizeDateFilter to
parse the picker value as local midnight (do not append 'Z' or construct with
Date.parse('${normalized}T00:00:00.000Z'); instead construct a local Date via
new Date(year, month-1, day) or Date.parse('${normalized}T00:00:00') so both
getTimestampDateKey and normalizeDateFilter use the same local-time convention).
---
Duplicate comments:
In `@components/OrderGrid/OrderGrid.styles.css`:
- Around line 38-259: The stylesheet components/OrderGrid/OrderGrid.styles.css
is failing Biome parsing because Tailwind `@apply` directives are disabled; open
your Biome config (biome.json or biome.jsonc) and add or update the
css.parser.tailwindDirectives setting to true (ensure the css and parser objects
exist and merge this key rather than replacing unrelated config), save and
commit the config change so Biome will accept the `@apply` rules used by classes
like orders-grid__filter-field and orders-grid__date-picker.
---
Nitpick comments:
In `@components/OrderGrid/parts/FilterSelect.tsx`:
- Around line 65-90: The component FilterSelect uses listbox/option ARIA roles
but implements interaction as clickable buttons; update the ARIA to match the
actual model by changing the container role from "listbox" to "menu" and each
item role from "option" to "menuitem" (remove aria-selected on items), and keep
the existing click handlers (onChange and setIsOpen) intact in the FilterSelect
component so assistive tech sees a menu/menuitem pattern rather than a listbox;
ensure the container still references id via aria-labelledby if needed and that
keyboard behavior remains handled by the existing buttons or is adjusted to
support menu semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6264f475-4277-442e-807d-410f4f8b552c
📒 Files selected for processing (34)
components/OrderGrid/OrderGrid.styles.csscomponents/OrderGrid/OrderGridWithPagination.tsxcomponents/OrderGrid/OrderGridWithPaginationClient.tsxcomponents/OrderGrid/__tests__/OrderGrid.test.tsxcomponents/OrderGrid/__tests__/OrderGridWithPaginationClient.test.tsxcomponents/OrderGrid/__tests__/order-grid.navigation.test.tscomponents/OrderGrid/__tests__/order-grid.query.test.tscomponents/OrderGrid/columns.tscomponents/OrderGrid/index.tsxcomponents/OrderGrid/order-grid-with-pagination.presenter.tscomponents/OrderGrid/order-grid.navigation.tscomponents/OrderGrid/order-grid.query.tscomponents/OrderGrid/parts/ActionButton.tsxcomponents/OrderGrid/parts/Body.tsxcomponents/OrderGrid/parts/FilterDatePicker.tsxcomponents/OrderGrid/parts/FilterSelect.tsxcomponents/OrderGrid/parts/FiltersActions.tsxcomponents/OrderGrid/parts/FiltersBar.tsxcomponents/OrderGrid/parts/FiltersFields.tsxcomponents/OrderGrid/parts/Toolbar.tsxcomponents/OrderGrid/parts/__tests__/Body.test.tsxcomponents/OrderGrid/parts/__tests__/FiltersBar.test.tsxcomponents/OrderGrid/parts/__tests__/Toolbar.test.tsxcomponents/OrderGrid/parts/filters.config.tscomponents/OrderGrid/parts/useFiltersBarController.tscomponents/OrderGrid/types.tscomponents/ui/Button.tsxcomponents/ui/FormField.tsxcomponents/ui/TextInput.tsxlib/orders/__tests__/orders.filter.test.tslib/orders/__tests__/orders.service.test.tslib/orders/orders.filter.tslib/orders/orders.filter.types.tslib/orders/orders.service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- components/OrderGrid/tests/OrderGrid.test.tsx
- lib/orders/tests/orders.service.test.ts
- components/OrderGrid/parts/ActionButton.tsx
- components/OrderGrid/parts/tests/Toolbar.test.tsx
…ve empty state handling
…and empty state handling
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/orders/orders.filter.ts (1)
95-108: Optional: precompute lowercase needles once per filter pass.You can avoid repeated lowercase conversions per row and slightly simplify the predicate logic.
♻️ Suggested refactor
export const filterOrders = ( orders: Order[], filters: OrdersGridFilters, ): Order[] => { const normalizedFilters = resolveOrdersGridFilters(filters); + const idNeedle = normalizedFilters.id?.toLowerCase(); + const instrumentNeedle = normalizedFilters.instrument?.toLowerCase(); return orders.filter((order) => { if ( - normalizedFilters.id && - !order.id.toLowerCase().includes(normalizedFilters.id.toLowerCase()) + idNeedle && + !order.id.toLowerCase().includes(idNeedle) ) { return false; } if ( - normalizedFilters.instrument && - !order.instrument - .toLowerCase() - .includes(normalizedFilters.instrument.toLowerCase()) + instrumentNeedle && + !order.instrument.toLowerCase().includes(instrumentNeedle) ) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/orders/orders.filter.ts` around lines 95 - 108, Precompute lowercase "needles" from normalizedFilters once before calling orders.filter to avoid repeating toLowerCase per row: create local variables like idNeedle = normalizedFilters.id ? normalizedFilters.id.toLowerCase() : undefined and instrumentNeedle = normalizedFilters.instrument ? normalizedFilters.instrument.toLowerCase() : undefined, then update the predicate inside the orders.filter callback to check idNeedle and instrumentNeedle (e.g., if (idNeedle && !order.id.toLowerCase().includes(idNeedle)) return false) and similarly for instrument using order.instrument; this keeps the same logic but avoids repeated lowercase conversions and simplifies the filter checks.components/OrderGrid/__tests__/order-grid-empty-state.test.ts (1)
3-24: Good test coverage of the main branches.Consider adding a test case verifying the priority order when both
instrumentandidfilters are set simultaneously, to ensure instrument takes precedence as intended.💡 Suggested additional test case
+ it('prioritizes instrument over id when both filters are active', () => { + expect( + getOrderGridEmptyStateMessage({ instrument: 'PETR4', id: '123' }), + ).toBe('PETR4 nao encontrado.'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/OrderGrid/__tests__/order-grid-empty-state.test.ts` around lines 3 - 24, Add a test that verifies priority when both instrument and id filters are provided: update the test suite for getOrderGridEmptyStateMessage to include a case calling getOrderGridEmptyStateMessage({ instrument: 'READL3', id: '1001' }) and assert it returns the instrument-specific message 'READL3 nao encontrado.' to ensure the instrument filter takes precedence over id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/OrderGrid/__tests__/order-grid-empty-state.test.ts`:
- Around line 3-24: Add a test that verifies priority when both instrument and
id filters are provided: update the test suite for getOrderGridEmptyStateMessage
to include a case calling getOrderGridEmptyStateMessage({ instrument: 'READL3',
id: '1001' }) and assert it returns the instrument-specific message 'READL3 nao
encontrado.' to ensure the instrument filter takes precedence over id.
In `@lib/orders/orders.filter.ts`:
- Around line 95-108: Precompute lowercase "needles" from normalizedFilters once
before calling orders.filter to avoid repeating toLowerCase per row: create
local variables like idNeedle = normalizedFilters.id ?
normalizedFilters.id.toLowerCase() : undefined and instrumentNeedle =
normalizedFilters.instrument ? normalizedFilters.instrument.toLowerCase() :
undefined, then update the predicate inside the orders.filter callback to check
idNeedle and instrumentNeedle (e.g., if (idNeedle &&
!order.id.toLowerCase().includes(idNeedle)) return false) and similarly for
instrument using order.instrument; this keeps the same logic but avoids repeated
lowercase conversions and simplifies the filter checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b62c682b-08f9-44e3-ba08-46aa9cc010a9
📒 Files selected for processing (11)
components/OrderGrid/OrderGridWithPaginationClient.tsxcomponents/OrderGrid/__tests__/OrderGridWithPaginationClient.test.tsxcomponents/OrderGrid/__tests__/order-grid-empty-state.test.tscomponents/OrderGrid/__tests__/order-grid.query.test.tscomponents/OrderGrid/order-grid-empty-state.tscomponents/OrderGrid/order-grid.query.tscomponents/OrderGrid/parts/FilterDatePicker.tsxcomponents/OrderGrid/parts/FilterSelect.tsxcomponents/OrderGrid/parts/__tests__/FilterSelect.test.tsxcomponents/OrderGrid/useOrderGridWithPaginationController.tslib/orders/orders.filter.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- components/OrderGrid/order-grid.query.ts
- components/OrderGrid/parts/FilterDatePicker.tsx
- components/OrderGrid/tests/order-grid.query.test.ts
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.
…nd update related components
Summary by CodeRabbit
New Features
Improvements
Tests