Skip to content

add components - #441

Merged
richardiphillips merged 4 commits into
mainfrom
addcomps1
Oct 8, 2026
Merged

richardiphillips merged 4 commits into
mainfrom
addcomps1

Conversation

@richardiphillips

Copy link
Copy Markdown
Contributor

No description provided.

@richardiphillips
richardiphillips requested review from a team and a balanced review from Copilot October 8, 2026 08:08

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Enter handling, result normalization, autocomplete blur behavior, and search-input accessibility have unresolved defects.

4 open findings
What changed in this PR

Adds reusable autocomplete and editable DataGrid search capabilities to the component library.

Changes:

  • Adds AutoComplete with object and ID/label modes.
  • Adds useEditableSearchGrid, documentation, and tests.
  • Exports both APIs and releases version 34.5.0.
File Description
src/​hooks/​useEditableSearchGrid.mdx Documents the grid-search hook.
src/​hooks/​useEditableSearchGrid.js Implements editable grid search behavior.
src/​hooks/​__tests__/​useEditableSearchGrid.test.js Tests validation and Tab commits.
src/​components/​AutoComplete.stories.js Adds component stories and examples.
src/​components/​AutoComplete.js Implements the autocomplete component.
src/​components/​__tests__/​AutoComplete.test.js Tests both value modes.
package.json Bumps the package version.
package-lock.json Synchronizes the version metadata.
index.js Exports the new public APIs.
CHANGELOG.md Records the 34.5.0 release.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread src/hooks/useEditableSearchGrid.js Outdated
Comment thread src/components/AutoComplete.js Outdated
Comment thread src/hooks/useEditableSearchGrid.js Outdated
Comment thread src/hooks/useEditableSearchGrid.js Outdated
Copilot AI balanced review requested due to automatic review settings October 8, 2026 11:13

@lewisrenfrew lewisrenfrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My claudes have been trying to wrtite their own autocompletes for ages and I grudgingly ask them to use our 'Search' instead... so I'll let them know this exists now!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Grid event handling, callback semantics, and search-result ID preservation contain correctness issues.

0 open findings

4 resolved since last review
Previously missed (5)

In code that hasn't changed since last review

Medium severity Mark MUI grid events as handled after custom traversal

src/​hooks/​useEditableSearchGrid.js:160

preventDefault() only suppresses the browser action; MUI's grid event handlers use defaultMuiPrevented. Without setting it here, the grid's own Tab handler can run after this custom commit/traversal and override focus or process the edit twice. Mark the MUI event handled, as the Enter branch already does.

Medium severity Invoke lookup callback only when a search field changes

src/​hooks/​useEditableSearchGrid.js:209

This callback also fires when an editable non-search column is committed, with changedField undefined. That contradicts the documented search-cell-only contract and can unexpectedly trigger consumer lookup/validation logic; invoke it only when a search field actually changed.

Medium severity Preserve existing result IDs during lookup normalization

src/​hooks/​useEditableSearchGrid.js:259

This normalization overwrites a result's existing id whenever the lookup code comes from another field. Consequently, an updateFields entry with from: 'id' copies the lookup code rather than the result's original ID, despite the API promising to copy result[from]. Preserve an existing id and update writeResultToRow to fall back through column.field before id.

Low severity Test idField matching with the option code

src/​components/​__tests__/​AutoComplete.test.js:46

This test types the option label, so it never exercises the new idField matching branch that lets users type a code and commit on blur/Tab. Type the code here to cover that advertised behavior; object mode already covers label matching.

Low severity Import useState in the standalone usage example

src/​hooks/​useEditableSearchGrid.mdx:36

The standalone usage example calls useState but does not import it, so copying the documented example fails to compile.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot AI balanced review requested due to automatic review settings October 8, 2026 12:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The hook currently imports a non-exported MUI symbol and mishandles keyboard navigation with paginated grids.

1 open finding
Previously missed (2)

In code that hasn't changed since last review

Medium severity Keyboard navigation ignores active pagination page

src/​hooks/​useEditableSearchGrid.js:147

This selector includes every filtered/sorted row, not just rows on the active client-side pagination page. At the last cell of a page, Tab therefore targets the first row of the next (unrendered) page, so focus is lost instead of leaving/wrapping within the displayed page. Derive the IDs from gridVisibleRowsSelector, which accounts for pagination when enabled and returns all rows otherwise.

Low severity Usage example omits the useState import

src/​hooks/​useEditableSearchGrid.mdx:35

The usage example calls useState but does not import it, so the documented copy-and-paste example fails with useState is not defined. Add the React hook import to the snippet.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

@@ -0,0 +1,320 @@
import React, { useRef, useState } from 'react';
import { gridExpandedSortedRowIdsSelector, useGridApiRef, GridSearchIcon } from '@mui/x-data-grid';
Copilot AI balanced review requested due to automatic review settings October 8, 2026 12:55
@richardiphillips
richardiphillips merged commit 451bec9 into main Oct 8, 2026
1 check passed
@richardiphillips
richardiphillips deleted the addcomps1 branch October 8, 2026 12:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Grid traversal breaks at pagination boundaries, and the callback documentation conflicts with runtime behavior.

1 open finding
Previously missed (2)

In code that hasn't changed since last review

Medium severity Limit keyboard navigation to rows on the active pagination page

src/​hooks/​useEditableSearchGrid.js:146

This selector includes every filtered/sorted row, not just rows on the active pagination page. On the last cell of a page, the handler therefore prevents the browser's Tab behavior and tries to focus a row rendered on another page; Shift-Tab has the same problem at a page boundary. Base traversal on the current page's rendered row IDs (for example, MUI's paginated-visible-row selector), and allow default Tab behavior at that page's boundaries.

Low severity Document onRowChange behavior for non-search field edits

src/​hooks/​useEditableSearchGrid.mdx:105

This contract does not match the implementation: processRowUpdate calls onRowChange for every editable column, including the example's amount, with changedField undefined for non-search edits. Document that behavior so consumers do not accidentally run lookup or persistence logic on unrelated edits.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants