Run 5 Entity-Grid Evolution — Byproducts Log

Discovered Issues

B1: DataGrid molecule needs further extension for advanced AG Grid props

The DataGrid molecule (src/components/canary/molecules/data-grid/data-grid.tsx) was designed as a thin wrapper with a curated set of AG Grid props. Run 5 added onCellEditingStopped, getRowClass, pagination, paginationPageSize, paginationPageSizeSelector, and domLayout as new passthrough props.

Observation: The molecule’s prop surface is growing. Future runs should evaluate whether entity-data-grid should bypass DataGrid and use AgGridReact directly (as the callil item-grid does), or continue extending DataGrid. The callil approach is arguably cleaner for organisms that need full AG Grid control.

Impact for Run 6: Item-grid integration. When item-grid is refactored to use entity-data-grid as its base, it may need additional AG Grid props not yet exposed through DataGrid (e.g., context for cell renderer communication). Budget time for DataGrid extension or evaluate bypassing it.

B2: useDirtyTracking is now orphaned

useDirtyTracking remains in the codebase at use-dirty-tracking.ts and is re-exported from index.ts and canary.ts. It is no longer used by any component in the canary directory.

TODO: Confirm no external consumers import useDirtyTracking before removing it. If the extras directory consumers are migrated in Run 6+, useDirtyTracking can be deleted entirely. Flag for Run 7 cleanup.

B3: entity-data-grid-shim still references old-style server pagination

The shim’s WithPagination story passes paginationData without paginationMode: 'server' in the factory config. This works due to the backward-compat fallback (treating paginationMode === undefined as 'server'). However, future consumers should explicitly set paginationMode for clarity.

TODO: Update shim’s WithPagination story to use a factory with paginationMode: 'server' explicitly. Low priority.

B4: WithPagination story creates factory inside render function

The updated WithPagination story (server-driven mode) calls createEntityDataGrid inside the render function. This means the factory and its component are re-created on every render, which is not ideal for production use. It was done to demonstrate isolated server pagination alongside client pagination.

TODO: Move to a top-level factory instantiation pattern as in all other stories. This is a stories-only issue, no production impact.

B5: isDown variable in drag-to-scroll effect

The isDown variable in the drag-to-scroll useEffect is declared as let in the closure. If the component unmounts while isDown === true (user is mid-drag), the onMouseUp listener on window will still fire and try to remove the click suppressor from el. Since el is a stale closure ref and the component is unmounted, this is harmless but slightly untidy.

Mitigation: Already handled — the useEffect cleanup removes all three listeners. The onMouseUp handler checks if (!isDown) return. Not a memory leak.

B6: Search countLabel shows “items” even when searchConfig.fields targets non-item entities

The count label hardcodes “item(s)” as the noun. For a supplier grid or department grid, this would show “3 items” when the grid has 3 suppliers.

TODO (Run 8 or later): Add countLabel?: (count: number, total: number) => string to searchConfig to allow custom labels. Current behavior is acceptable for the consolidation scope.

B7: handleRef in useRowAutoPublish is typed as Ref<RowAutoPublishHandle> but bound as MutableRefObject in tests

In the test file, handleRef is created as { current: null } as React.MutableRefObject<any>. This works but is a test-only workaround. The hook’s API accepts Ref<RowAutoPublishHandle> which includes RefCallback and RefObject. The tests use the mutable ref pattern which is fine.

B8: publish called with entity: undefined from saveAll

When saveAll() is called imperatively (e.g., via toolbar button), the onRowPublish callback receives entity: undefined. This is documented and intentional (matches the callil useItemGridEditing.saveAll behavior). Consumers of onRowPublish must handle entity?: T.

Observation: Some server implementations may need the entity snapshot to compute the diff. Consider adding a currentData: T[] parameter to saveAll() in a future API version that allows entity lookup by row ID.

TODOs for Future Runs

#	Run	Description
T1	Run 6	When item-grid is refactored to use entity-data-grid, evaluate bypassing DataGrid molecule for direct AgGridReact usage.
T2	Run 6	Item-grid’s context prop (for onNotesClick communication to cell renderers) needs to be passed through entity-data-grid. Add gridContext?: Record<string, unknown> to EntityDataGridConfig.
T3	Run 7	Remove useDirtyTracking after confirming no external consumers.
T4	Run 7	Remove legacy ref API aliases (saveAllDrafts, discardAllDrafts, getHasUnsavedChanges) after all consumers migrated.
T5	Run 8	Add countLabel customization to searchConfig for non-item entities.
T6	Run 8	Capture VRT baselines for row visual states (.ag-row-saving, .ag-row-error). These should be done in the pre-push validation gate.
T7	Pre-push	Run ./tools/generate-vrt-baselines.sh or npx playwright test --project=vrt to capture baselines for new stories. Per project memory, this is required before pushing.

Observations

Observation 1: useRowAutoPublish hook design

The hook’s use of stable callback refs (onRowPublishRef, onDirtyChangeRef) is the correct pattern for async callbacks that should not cause unnecessary handler recreation. This pattern should be used as the standard for any new hooks that accept async callbacks in entity-data-grid.

Observation 2: AG Grid getRowClass reactivity

AG Grid calls getRowClass when getRowClass itself changes (via useCallback returning a new function when rowStates changes). This is efficient because AG Grid only refreshes the class for rows that might have changed. The implementation correctly uses rowStates as a dependency of getRowClass’s useCallback.

Observation 3: Test coverage gap in use-row-auto-publish.test.ts

The test 'getDirtyRowIds returns IDs of rows with pending changes' is a placeholder — it only verifies the hook initializes correctly. A better test would use handleRef to check getDirtyRowIds() after adding changes. This is covered in the 'imperative handle getDirtyRowIds returns correct IDs' test below, so the placeholder is redundant. Can be removed in cleanup.

Observation 4: Client-side pagination and search are compatible

The filteredData (search result) is passed as rowData to DataGrid, which then applies AG Grid client pagination on top. This means client pagination pages the filtered result, which is the expected behavior (search narrows the dataset, pagination navigates within it).

Observation 5: isDown in drag scroll is not in React state

The drag-to-scroll implementation intentionally uses plain let variables (not React state) for isDown, hasDragged, etc. These are transient interaction state that don’t need to trigger re-renders. This is the correct pattern for pointer event handlers.