Slickgrid React Table #307
Conversation
📝 WalkthroughWalkthroughAdds a React-based SlickGrid table chart with find/replace and in-cell editing, new SlickGrid data provider and utilities, unique-datatype encoding/decoding support, theme/CSS for SlickGrid, a ReusableAlertDialog refactor, numerous hooks/components/tests/docs, and three package dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dialog as FindAndReplaceDialog
participant Hook as useFindReplace
participant DataStore
participant Grid as SlickGrid
participant UI as FeedbackAlert
User->>Dialog: Enter search + Click Find
Dialog->>Hook: handleFind(searchColumn, findText)
Hook->>DataStore: read column values (decode "unique" if needed)
DataStore-->>Hook: column values
Hook->>Hook: Build FoundMatch list
Hook->>Grid: gotoCell(firstMatch.rowIndex, columnIndex)
Grid-->>Hook: selection updated
Hook->>UI: setFeedbackAlert("Found X matches")
User->>Dialog: Click Replace
Dialog->>Hook: handleReplace(replaceText)
Hook->>DataStore: setCellValue (via setCellValueFromString)
alt success
DataStore-->>Hook: updated
Hook->>Grid: invalidate & render
Hook->>Hook: re-run find
Hook->>UI: setFeedbackAlert("Replacement successful")
else error
Hook->>UI: setFeedbackAlert(error with metadata)
end
sequenceDiagram
participant User
participant Grid as SlickGrid
participant EditHook as useEditCell
participant Util as valueReplacementUtil
participant DataStore
participant UI as FeedbackAlert
User->>Grid: start edit (double-click)
Grid->>EditHook: beforeEditCell(event)
EditHook->>EditHook: store oldValue
User->>Grid: finish edit (Enter)
Grid->>EditHook: cellChange(event with newValue)
EditHook->>Util: setCellValueFromString(column, dataIndex, newValue)
alt success
Util->>DataStore: write encoded value
DataStore-->>EditHook: confirm
EditHook->>DataStore: dataChanged(column)
EditHook->>Grid: invalidate/refresh
EditHook->>UI: setFeedbackAlert(success)
else error
Util-->>EditHook: throw error
EditHook->>UI: setFeedbackAlert(error with metadata)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (21)
python/mdvtools/mdvproject.py (2)
653-664: Potential IndexError whenraw_datais empty.The check
raw_data and isinstance(raw_data[0], (int, float))will correctly short-circuit whenraw_datais empty due to the truthiness check. However, ifraw_datais an empty list/array,len(raw_data)will be 0, and creating a dataset with 0 rows may not be the intended behavior.Consider adding explicit validation for empty data:
🔎 Suggested validation
if column["datatype"] == "unique": + if not raw_data or len(raw_data) == 0: + raise ValueError( + f"Column {cid} of type 'unique' requires non-empty data" + ) # If numeric data is received instead of array of strings, raise error if raw_data and isinstance(raw_data[0], (int, float)):
660-663: Minor: Redundant branch can be simplified.Both branches compute
num_rows = len(raw_data)identically. Consider simplifying:🔎 Suggested simplification
- if column["datatype"] == "unique": - # If numeric data is received instead of array of strings, raise error - if raw_data and isinstance(raw_data[0], (int, float)): - raise ValueError( - f"Column {cid} of type 'unique' expects array of strings, " - f"but received numeric data." - ) - # Use the actual length of the array (number of rows) - num_rows = len(raw_data) - else: - num_rows = len(raw_data) + # Validate unique columns expect strings + if column["datatype"] == "unique" and raw_data and isinstance(raw_data[0], (int, float)): + raise ValueError( + f"Column {cid} of type 'unique' expects array of strings, " + f"but received numeric data." + ) + num_rows = len(raw_data)src/charts/ChartManager.js (1)
1153-1159: Consider usingArray.from()for cleaner data copying.The element-by-element loop works correctly, but could be simplified.
🔎 Suggested simplification
} else { // For other datatypes, get the values from data array - arr = new Array(cl.data.length); - for (let i = 0; i < cl.data.length; i++) { - arr[i] = cl.data[i]; - } + arr = Array.from(cl.data); }src/react/hooks/useSortedIndices.ts (2)
5-5: Remove unused importtrace.The
tracefunction is imported from MobX but never used in the hook.🔎 Suggested fix
-import { autorun, trace } from "mobx"; +import { autorun } from "mobx";
112-115: Remove unnecessaryreturnstatement.The
return;at line 112 serves no purpose at the end of the autorun callback.🔎 Suggested fix
setSortedIndices(new Uint32Array(indices)); } - return; }); return () => disposer();src/react/components/FindAndReplaceDialog.tsx (2)
90-101: Consider reusingDialogCloseIconButtonfrom ProjectRenameModal.The close button implementation here duplicates the pattern from
@/catalog/ProjectRenameModal. SinceReusableDialog.tsxin this PR already imports and usesDialogCloseIconButton, this component could follow the same pattern for consistency.🔎 Suggested refactor
+import { DialogCloseIconButton } from "@/catalog/ProjectRenameModal"; import { Close as CloseIcon } from "@mui/icons-material";Then replace lines 90-101:
- <IconButton - aria-label="close" - onClick={onDialogClose} - sx={{ - position: "absolute", - right: 8, - top: 8, - color: (theme) => theme.palette.grey[500], - }} - > - <CloseIcon /> - </IconButton> + <DialogCloseIconButton onClose={onDialogClose} />Note: This would also allow removing the
CloseIconimport if no longer needed elsewhere.
116-121: Consider simplifying the matches display condition.The condition
foundMatches || typeof foundMatches === "number"correctly handles zero matches, but could be simplified.🔎 Suggested simplification
- {findText && (foundMatches || typeof foundMatches === "number") ? ( + {findText && foundMatches != null ? ( <Typography>{`Found ${foundMatches} matches`}</Typography> ) : ( <></> )}The
!= nullcheck handles bothnullandundefinedwhile allowing0through.Alternatively, the empty fragment could be replaced with
null:- ) : ( - <></> - )} + ) : null}src/react/hooks/useSlickGridReact.ts (2)
1-10: Unused import detected.
replaceMatchesis imported fromvalueReplacementUtilbut never used in this file.🔎 Proposed fix
import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import type { TableChartReactConfig } from "../components/TableChartReactWrapper"; import { useChart, useDataStore } from "../context"; import { useChartID, useConfig, useOrderedParamColumns } from "../hooks"; import { useHighlightedIndex } from "../selectionHooks"; import useSortedIndices from "./useSortedIndices"; import { type Column, Editors, type GridOption, type SlickgridReactInstance } from "slickgrid-react"; import SlickGridDataProvider from "../utils/SlickGridDataProvider"; import { runInAction } from "mobx";Note: The import statement for
replaceMatchesshould be removed if it was previously imported and is no longer needed.
122-138: Consider a more robust approach than fixed timeout for selection flag reset.Using
setTimeout(100ms)to resetisSelectingRefis fragile. If MobX reactions take longer than 100ms (e.g., under heavy load), the flag may reset prematurely, causing unintended grid updates. This same pattern appears at lines 217-219.Consider using a MobX
reactioncompletion callback or a more deterministic approach.🔎 Suggested approach
Extract the timeout value to a named constant for consistency and easier tuning:
+const SELECTION_DEBOUNCE_MS = 100; + // In the selection handler: setTimeout(() => { isSelectingRef.current = false; -}, 100); +}, SELECTION_DEBOUNCE_MS);Alternatively, consider using
requestAnimationFrameorqueueMicrotaskif you only need to defer until after the current event loop.src/react/components/TableChartReactWrapper.tsx (2)
44-49: Placeholder implementation in download menu.The download menu icon is registered but the handler only logs to console. Consider adding a TODO comment or implementing the functionality before merge.
🔎 Suggested improvement
this.addMenuIcon("fas fa-download", "Download data", { func: () => { - console.log("Download Data"); - // this.downloadData(); + // TODO: Implement data download functionality + console.warn("Download Data - not yet implemented"); }, });
97-99: Unused parameter in init function.The
ds(DataStore) parameter is passed but not used. If unused intentionally, prefix with underscore for clarity.🔎 Proposed fix
- init: (config, ds, ec) => { + init: (config, _ds, ec) => { config.include_index = ec.include_index; },src/react/hooks/useEditCell.ts (2)
4-4: Unused import:replaceMatches.
replaceMatchesis imported but not used in this hook.🔎 Proposed fix
-import { replaceMatches, setCellValueFromString } from "../utils/valueReplacementUtil"; +import { setCellValueFromString } from "../utils/valueReplacementUtil";
80-89: Redundant optional chaining and separate checks.Line 86 uses
editedCol?.editablebuteditedColis already confirmed to exist at line 81. The separate checks for existence and editability could be combined.🔎 Proposed simplification
try { - if (!editedCol) { - console.error("Column not found"); - throw new Error("Column not found"); - } - - if (!editedCol?.editable) { - console.error(`Column ${columnName} not editable`); - throw new Error(`Column ${columnName} not editable`); + if (!editedCol || !editedCol.editable) { + const reason = !editedCol ? "Column not found" : `Column ${columnName} not editable`; + console.error(reason); + throw new Error(reason); }src/react/utils/SlickGridDataProvider.ts (2)
73-80: Inconsistent indentation inmapRowsToIds.The method body uses different indentation than the rest of the class.
🔎 Proposed fix
- mapRowsToIds(rows: number[]): number[] { - return rows - .filter(row => row >= 0 && row < this.indices.length) - .map(row => { - const item = this.getItem(row); - return item?.id ?? row; - }); -} + mapRowsToIds(rows: number[]): number[] { + return rows + .filter(row => row >= 0 && row < this.indices.length) + .map(row => { + const item = this.getItem(row); + return item?.id ?? row; + }); + }
53-98: Consider removing commented-out code.Multiple commented-out method implementations add noise. If these are being considered for future use, consider adding TODO comments instead, or remove them entirely and rely on version control.
src/react/utils/valueReplacementUtil.ts (1)
177-206: Multitext handling silently truncates excess values.When the user provides more comma-separated values than
stringLengthallows, the excess values are silently discarded. Consider warning the user.The
//! review thiscomment at line 177 suggests this needs attention.🔎 Proposed improvement
// Set new values + if (parts.length > stringLength) { + console.warn( + `Multitext value has ${parts.length} parts but column "${column.field}" only supports ${stringLength}. Excess values will be truncated.` + ); + } for (let i = 0; i < Math.min(parts.length, stringLength); i++) { const valueIndex = getValueIndex(parts[i], values, maxValues); data[baseIndex + i] = valueIndex; }Also consider removing or addressing the
//! review thiscomment before merge.src/react/components/TableChartReactComponent.tsx (1)
147-154: ClarifyisAlertErrorComponentlogic.The
isAlertErrorComponentprop on line 152 has complex logic:isAlertErrorComponent={!feedbackAlert.metadata || feedbackAlert.type !== "error"}This means
isAlertErrorComponentistruewhen:
- There's no metadata, OR
- The type is not "error"
This seems counterintuitive given the naming. Please verify this logic is correct, or consider:
- Renaming the prop to better reflect its purpose
- Adding a comment explaining the logic
- Extracting to a named variable for clarity
🔎 Suggested clarification
+ const isSimpleAlert = !feedbackAlert.metadata || feedbackAlert.type !== "error"; + {feedbackAlert && ( <ReusableDialog open={alertDialogOpen} handleClose={onFeedbackDialogClose} component={<FeedbackAlertComponent feedbackAlert={feedbackAlert} />} - isAlertErrorComponent={!feedbackAlert.metadata || feedbackAlert.type !== "error"} + isAlertErrorComponent={isSimpleAlert} /> )}src/react/hooks/useFindReplace.ts (4)
19-28: Consider parameter object for improved maintainability.The hook accepts 8 parameters, which makes the function signature complex. Consider grouping related parameters into objects for better maintainability and easier testing.
🔎 Suggested refactor
type UseFindReplaceParams = { grid: { orderedParamColumns: LoadedDataColumn<DataType>[]; sortedIndices: Uint32Array; config: { include_index?: boolean }; gridRef: React.MutableRefObject<SlickgridReactInstance | null>; isSelectingRef: React.MutableRefObject<boolean>; }; data: { dataStore: DataStore; searchColumn: string | null; }; callbacks: { setFeedbackAlert: (alert: FeedbackAlert) => void; }; }; const useFindReplace = ({ grid, data, callbacks }: UseFindReplaceParams) => { // ... implementation };
197-197: Address TODO: Add replace value validation.The comment on line 197 indicates missing validation for the replace value. Consider adding validation to:
- Ensure replace value is appropriate for the column data type
- Check if the replacement would create invalid data
- Warn users if replacing with empty string
Do you want me to generate validation logic for the replace value based on column data types?
294-294: Remove commented code.Line 294 contains commented-out code (
// let valuesReplaced = false;) that should be removed to keep the codebase clean.
295-311: Silent error suppression in replaceAll loop.The loop (lines 295-311) silently catches and counts errors without logging them:
} catch (err) { errorCount++; }While the error count is reported to users, consider logging individual errors for debugging purposes. This would help developers troubleshoot issues with specific cells.
🔎 Suggested improvement
for (const match of foundMatches) { try { const matchReplaced = replaceMatches( searchColumn, column, findValue, replaceValue, match.dataIndex, ); if (matchReplaced) { successCount++; } } catch (err) { + console.error(`Failed to replace in row ${match.rowIndex}:`, err); errorCount++; } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.json(2 hunks)python/mdvtools/mdvproject.py(1 hunks)src/charts/ChartManager.js(1 hunks)src/charts/css/charts.css(5 hunks)src/charts/dialogs/AlertErrorComponent.tsx(2 hunks)src/charts/dialogs/DebugErrorComponent.tsx(1 hunks)src/charts/dialogs/ReusableDialog.tsx(2 hunks)src/modules/static_index.ts(1 hunks)src/react/HmrHack.ts(1 hunks)src/react/components/FindAndReplaceDialog.tsx(1 hunks)src/react/components/TableChartReactComponent.tsx(1 hunks)src/react/components/TableChartReactWrapper.tsx(1 hunks)src/react/hooks/useEditCell.ts(1 hunks)src/react/hooks/useFindReplace.ts(1 hunks)src/react/hooks/useSlickGridReact.ts(1 hunks)src/react/hooks/useSortedIndices.ts(1 hunks)src/react/utils/SlickGridDataProvider.ts(1 hunks)src/react/utils/valueReplacementUtil.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The PR titled "column link" introduces the concept of dynamic column links to allow charts to reactively update based on operations in linked data sources, replacing string-based column identifiers with more robust `FieldSpec` and `ColumnQuery` systems.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:22-22
Timestamp: 2025-06-04T10:03:08.161Z
Learning: In the MDV project, React hooks provide a more natural and simpler pattern for MobX integration compared to class-based charts, as hooks automatically handle lifecycle management and cleanup in a way that aligns better with reactive programming patterns.
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 202
File: src/charts/TableChart.js:18-19
Timestamp: 2025-05-22T11:21:43.866Z
Learning: In the TableChart component, the condition for including the index column was changed from `config.include_index !== false` to `config?.include_index`, making it opt-in rather than opt-out. This means the index column is only included when explicitly enabled rather than being included by default.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The project uses MobX for state management in chart configurations, with `mobxAutorun` and reaction disposers to manage reactive updates based on changes to linked data sources.
📚 Learning: 2025-05-22T11:21:43.866Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 202
File: src/charts/TableChart.js:18-19
Timestamp: 2025-05-22T11:21:43.866Z
Learning: In the TableChart component, the condition for including the index column was changed from `config.include_index !== false` to `config?.include_index`, making it opt-in rather than opt-out. This means the index column is only included when explicitly enabled rather than being included by default.
Applied to files:
src/charts/dialogs/DebugErrorComponent.tsxsrc/react/hooks/useSortedIndices.tssrc/react/components/TableChartReactComponent.tsxsrc/react/HmrHack.tssrc/react/components/TableChartReactWrapper.tsxsrc/charts/ChartManager.js
📚 Learning: 2025-12-19T11:26:47.659Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 232
File: src/react/components/DeckScatterReactWrapper.tsx:45-47
Timestamp: 2025-12-19T11:26:47.659Z
Learning: In DeckScatterReactWrapper.tsx, the adaptConfig function incorrectly assigns config.param[2] to contourParameter without considering chart types: DeckDensity uses param[2] for density fields (_multi_column:number), DeckScatter3D uses it for z-axis (numeric), and DeckScatter 2D has no param[2]. This initialization should be removed entirely and contourParameter should remain undefined unless explicitly configured. Tracked in issue #303.
Applied to files:
src/charts/dialogs/DebugErrorComponent.tsxsrc/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-03-20T13:39:49.385Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 150
File: src/react/components/ColumnDropdownComponent.tsx:110-110
Timestamp: 2025-03-20T13:39:49.385Z
Learning: In ColumnDropdownComponent.tsx, when handling type issues with setValue in the handleSelectAll function, type assertions like (setValue as (v: DataColumn<DataType>[]) => void)([]) should be used instead of ts-expect-error comments when TypeScript can't automatically infer that the function should accept different parameter types based on a runtime condition.
Applied to files:
src/charts/dialogs/DebugErrorComponent.tsxsrc/react/utils/valueReplacementUtil.ts
📚 Learning: 2025-12-19T11:28:13.803Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 232
File: src/react/contour_state.ts:291-320
Timestamp: 2025-12-19T11:28:13.803Z
Learning: In the MDV codebase, when fixing settings-related logic, prefer keeping the logic localized inside settings functions rather than moving it into constructors (e.g., chart constructors). Returning disposers from settings functions is a good pattern because it preserves locality while ensuring proper cleanup, and it aligns with future node-graph interface plans. This guideline applies to TypeScript files across the project (especially settings modules); avoid moving settings logic into constructors and instead return disposable cleanup functions from the appropriate settings functions.
Applied to files:
src/modules/static_index.tssrc/react/hooks/useSortedIndices.tssrc/react/hooks/useEditCell.tssrc/react/HmrHack.tssrc/react/hooks/useFindReplace.tssrc/react/utils/SlickGridDataProvider.tssrc/react/utils/valueReplacementUtil.tssrc/react/hooks/useSlickGridReact.ts
📚 Learning: 2025-06-04T10:03:08.161Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:22-22
Timestamp: 2025-06-04T10:03:08.161Z
Learning: In the MDV project, React hooks provide a more natural and simpler pattern for MobX integration compared to class-based charts, as hooks automatically handle lifecycle management and cleanup in a way that aligns better with reactive programming patterns.
Applied to files:
src/react/hooks/useSortedIndices.tssrc/react/hooks/useEditCell.tssrc/react/components/TableChartReactComponent.tsxsrc/react/HmrHack.tssrc/react/hooks/useFindReplace.tssrc/react/components/TableChartReactWrapper.tsxsrc/react/hooks/useSlickGridReact.ts
📚 Learning: 2025-03-17T15:08:26.492Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The project uses MobX for state management in chart configurations, with `mobxAutorun` and reaction disposers to manage reactive updates based on changes to linked data sources.
Applied to files:
src/react/hooks/useSortedIndices.tssrc/react/components/TableChartReactComponent.tsxsrc/react/HmrHack.tssrc/react/components/TableChartReactWrapper.tsxsrc/react/hooks/useSlickGridReact.ts
📚 Learning: 2025-04-09T14:32:53.708Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T14:32:53.708Z
Learning: In React components that don't directly access MobX observables or stores, the `observer()` wrapper is unnecessary. Components that only receive data via props and use standard React hooks for internal state don't need to be MobX observers, as they will re-render appropriately when props change.
Applied to files:
src/react/components/TableChartReactComponent.tsxsrc/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-04-09T14:32:53.708Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T14:32:53.708Z
Learning: When implementing React components, the MobX `observer()` wrapper is only necessary for components that directly access MobX observables or stores. Components that exclusively use props and local React state (useState, useRef, etc.) don't need to be wrapped with observer() as they will re-render appropriately when props change through normal React mechanisms.
Applied to files:
src/react/components/TableChartReactComponent.tsxsrc/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-03-17T12:30:48.592Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 144
File: src/charts/dialogs/AddViewDialog.tsx:51-90
Timestamp: 2025-03-17T12:30:48.592Z
Learning: When replacing .apply() calls in MDV dialog components, use direct method calls on the chart manager instance (e.g., `cm.removeAllCharts()`) rather than calling destructured methods directly (e.g., `removeAllCharts()`). This ensures proper `this` context is maintained without the verbosity of .apply().
Applied to files:
src/react/HmrHack.tssrc/charts/dialogs/ReusableDialog.tsx
📚 Learning: 2025-03-17T12:01:42.608Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 144
File: src/charts/dialogs/AddViewDialog.tsx:51-90
Timestamp: 2025-03-17T12:01:42.608Z
Learning: When refactoring MDV dialog components that interact with the chart manager (cm), prefer direct method calls (e.g., `cm.getState()`) over destructuring methods and using .apply() (e.g., `const { getState } = cm; getState.apply(cm, [])`). Direct method calls maintain the correct context and are more readable.
Applied to files:
src/react/HmrHack.tssrc/charts/dialogs/ReusableDialog.tsx
📚 Learning: 2025-06-04T10:19:55.042Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:0-0
Timestamp: 2025-06-04T10:19:55.042Z
Learning: In the MDV codebase, BaseChart.ts provides automatic disposer management through the `mobxAutorun()` method which tracks disposers in a `reactionDisposers` array and automatically disposes them in the `remove()` method. When using direct `reaction()` calls instead of `mobxAutorun()`, manual disposer management is required.
Applied to files:
src/react/HmrHack.tssrc/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-04-09T20:29:56.245Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T20:29:56.245Z
Learning: When working with D3.js in React components, it's appropriate to intentionally exclude certain dependencies from useEffect dependency arrays to prevent unnecessary recreation of D3 elements. This approach should be documented with explanatory comments and ESLint suppression comments like `// eslint-disable-next-line react-hooks/exhaustive-deps` to clarify the intentional exclusion.
Applied to files:
src/react/HmrHack.tssrc/react/hooks/useSlickGridReact.ts
📚 Learning: 2025-06-04T09:41:18.914Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:17-17
Timestamp: 2025-06-04T09:41:18.914Z
Learning: In the MDV project, BaseChart.mobxAutorun automatically stores the disposer and runs it when the chart is destroyed, eliminating the need for manual cleanup of MobX autorun reactions in derived chart classes.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-03-03T15:53:04.505Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 115
File: src/react/components/ColumnSelectionComponent.tsx:165-165
Timestamp: 2025-03-03T15:53:04.505Z
Learning: In complex MobX-React components like those in the MDV codebase, creating intermediate objects (like `props2 = ...`) without making them observable can lead to read-only props in child components. This can be intentional to control the flow of state updates and avoid circular reactions.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-03-17T15:08:26.492Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The PR titled "column link" introduces the concept of dynamic column links to allow charts to reactively update based on operations in linked data sources, replacing string-based column identifiers with more robust `FieldSpec` and `ColumnQuery` systems.
Applied to files:
src/charts/ChartManager.js
📚 Learning: 2025-05-29T10:13:55.438Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 208
File: src/charts/dialogs/ChatDialog.tsx:37-37
Timestamp: 2025-05-29T10:13:55.438Z
Learning: The ChatDialog component in MDV has a popout feature that creates a new dialog instance in a separate window, but the conversation state is lost because React component state is not shared between windows. The solution requires implementing a shared state management system using React Context and localStorage to maintain conversation continuity across window boundaries.
Applied to files:
src/charts/dialogs/ReusableDialog.tsx
📚 Learning: 2025-03-17T12:33:33.266Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 144
File: src/charts/dialogs/AddViewDialog.tsx:51-90
Timestamp: 2025-03-17T12:33:33.266Z
Learning: When refactoring the MDV codebase, `.apply()` should only be replaced with direct method calls in dialog components and similar patterns where methods are destructured from an object and then `.apply()` is used to restore the original context. Method wrapping/replacement cases should continue to use `.apply()` to maintain proper behavior.
Applied to files:
src/charts/dialogs/ReusableDialog.tsx
🧬 Code graph analysis (9)
src/react/hooks/useSortedIndices.ts (3)
src/react/hooks.ts (2)
useConfig(26-29)useSimplerFilteredIndices(375-383)src/react/components/TableChartReactWrapper.tsx (1)
TableChartReactConfig(14-19)src/react/context.tsx (1)
useDataStore(36-41)
src/react/hooks/useEditCell.ts (3)
src/charts/charts.d.ts (1)
LoadedDataColumn(123-124)src/react/components/TableChartReactComponent.tsx (1)
FeedbackAlert(12-19)src/react/utils/valueReplacementUtil.ts (1)
setCellValueFromString(143-237)
src/react/hooks/useFindReplace.ts (3)
src/charts/charts.d.ts (1)
LoadedDataColumn(123-124)src/react/components/TableChartReactComponent.tsx (1)
FeedbackAlert(12-19)src/react/utils/valueReplacementUtil.ts (1)
replaceMatches(251-301)
src/react/utils/SlickGridDataProvider.ts (1)
src/charts/charts.d.ts (1)
LoadedDataColumn(123-124)
src/react/utils/valueReplacementUtil.ts (2)
src/utilities/clustering.js (1)
regex(244-244)src/charts/charts.d.ts (1)
LoadedDataColumn(123-124)
src/react/components/TableChartReactWrapper.tsx (1)
src/charts/BaseChart.ts (1)
BaseConfig(22-30)
python/mdvtools/mdvproject.py (1)
src/datastore/catWorker.js (8)
len(36-36)len(71-71)len(130-130)len(164-167)len(203-203)len(238-238)data(8-8)dtype(4-7)
src/charts/dialogs/ReusableDialog.tsx (2)
src/catalog/ProjectRenameModal.tsx (1)
DialogCloseIconButton(28-44)src/charts/dialogs/FileUploadDialog.tsx (2)
Container(43-49)Button(147-168)
src/react/hooks/useSlickGridReact.ts (4)
src/react/hooks.ts (3)
useConfig(26-29)useChartID(57-71)useOrderedParamColumns(203-248)src/react/components/TableChartReactWrapper.tsx (1)
TableChartReactConfig(14-19)src/react/context.tsx (2)
useDataStore(36-41)useChart(30-35)src/react/selectionHooks.ts (1)
useHighlightedIndex(9-23)
🪛 ast-grep (0.40.0)
src/react/utils/valueReplacementUtil.ts
[warning] 20-20: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(escapedFind, "gi")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (32)
src/charts/dialogs/DebugErrorComponent.tsx (1)
64-67: LGTM!The change from null-checks to truthiness checks appropriately filters out empty strings and other falsy values from the metadata payload, keeping the error details cleaner.
src/charts/dialogs/AlertErrorComponent.tsx (2)
49-50: LGTM!The simplified rendering is cleaner and correctly handles the optional title with inline conditional rendering.
11-13: CSS variables are properly defined and the implementation is correct.The AlertErrorComponent correctly references CSS variables for all alert types (error, info, success, warning). Verification confirms all required variables—
--background_color_error,--background_color_success,--background_color_info,--background_color_warning,--text_color_error,--text_color_success,--text_color_info, and--text_color_warning—are defined insrc/charts/css/charts.cssacross multiple theme scopes (light, dark, and system preference).src/react/HmrHack.ts (1)
14-15: LGTM!The import follows the established pattern for ensuring HMR works correctly with registered chart modules.
src/modules/static_index.ts (1)
3-3: LGTM!The SlickGrid Material theme CSS import is correctly placed as a global import, ensuring the theme is available for the new SlickGrid React table component.
src/charts/ChartManager.js (2)
1114-1118: LGTM!Good addition of
stringLengthto the metadata for unique columns. This ensures the server receives the necessary information to properly handle fixed-length text data.
1121-1152: Verify the early return behavior on invalid stringLength.The fallback returns
{ metadata: md, data: [] }when stringLength is invalid, which silently prevents the column data from being saved. Consider whether this should surface a more visible warning to the user or whether the error log at line 1127-1129 is sufficient.src/charts/dialogs/ReusableDialog.tsx (2)
1-7: LGTM!The imports are correctly structured. The
DialogCloseIconButtonreuse fromProjectRenameModalpromotes consistency across dialogs, and MUI'sContainerprovides appropriate layout constraints.
29-49: LGTM!Clean dialog implementation with proper MUI patterns. The
PaperPropsstyling, conditionalDialogActions, and close button positioning are well-structured.src/react/hooks/useSortedIndices.ts (2)
66-78: Decoding logic is correct with proper bounds checking.The defensive coding with bounds checking at line 69 and optional chaining at line 76 ensures safe data access. The decoded strings are correctly stored keyed by data index for lookup during sort.
86-111: LGTM!Well-implemented sorting logic with proper handling of edge cases. Null/undefined and NaN values are correctly pushed to the end regardless of sort direction, which provides consistent and predictable sorting behavior.
src/react/components/FindAndReplaceDialog.tsx (2)
33-44: LGTM!Good implementation of the draggable paper component. The
nodeRefpattern withuseRefis the correct approach for React 18+ with react-draggable to avoid findDOMNode deprecation warnings.
156-192: LGTM!The replace section correctly conditionally renders based on
isColumnEditable, and the button disabled states properly prevent actions without valid input. The Enter key handler on the replace TextField provides good keyboard UX.src/react/hooks/useSlickGridReact.ts (4)
26-39: LGTM - Ref synchronization pattern.The refs are properly synchronized with their corresponding state values via useEffect. This pattern correctly enables callbacks to access current values without triggering unnecessary re-renders or recreations.
41-79: LGTM - Column definitions properly constructed.The column definitions correctly handle the opt-in index column (based on learnings about
include_indexbeing opt-in rather than opt-out), apply editors for editable columns, and configure header menus for find/replace.
224-252: Potential conflict betweengridHeightandautoHeight.
gridHeight: "600px"is set alongsideautoHeight: truein theautoResizeconfig. These may conflict -autoHeight: trueshould calculate the height from content/container, making the fixedgridHeightpotentially redundant or overridden.Additionally,
darkModeis computed once fromwindow?.mdv?.chartManager?.theme- if the theme changes dynamically, the grid won't update.Consider whether
gridHeightshould be removed when usingautoHeight, or if you want a minimum height instead. For theme reactivity, this value may need to come from a reactive source.
259-278: LGTM - Comprehensive return API.The hook returns a well-structured API that enables integration with editing and find/replace functionality. Exposing refs is intentional for cross-hook communication.
src/react/components/TableChartReactWrapper.tsx (1)
102-103: LGTM - HMR side-effect export.The
export default 42pattern for HMR is consistent with the codebase patterns mentioned in the AI summary.src/react/hooks/useEditCell.ts (2)
21-49: LGTM - Pre-edit value capture.The callback correctly captures the current cell value before editing, handling null/undefined cases appropriately for later comparison in the change handler.
103-107: LGTM - Grid refresh after edit.The grid is properly invalidated and re-rendered after a successful cell value update, and
dataStore.dataChangedis called to notify other components.src/react/utils/SlickGridDataProvider.ts (2)
22-42: LGTM - getItem correctly maps grid rows to data rows.The method properly handles bounds checking, maps grid indices to data indices via
this.indices, and adds the optional__index__field for display when enabled.
100-111: LGTM - No-op methods for SlickGrid interface compliance.Empty
sort,reSort, andrefreshmethods are intentional to satisfy the SlickGrid data provider interface while handling sorting externally via MobX config state.src/react/utils/valueReplacementUtil.ts (3)
12-23: Regex escaping mitigates ReDoS risk.The static analysis tool flagged
new RegExp(escapedFind, "gi")as a potential ReDoS vector. However, line 18 properly escapes all regex metacharacters before constructing the RegExp, which neutralizes the attack surface. The warning is a false positive in this case.
251-301: LGTM - Well-structured replace operation with proper error handling.The function correctly separates "no match found" (returns false) from error conditions (throws). Error messages include context (column name) for debugging, and the case-insensitive match check aligns with the case-insensitive replacement.
60-133: LGTM - Comprehensive cell value extraction.The function handles all datatypes with proper validation, bounds checking, and decoding. The
uniquetype correctly strips null padding, andmultitextproperly filters empty markers.src/charts/css/charts.css (1)
25-27: LGTM! Consistent theming variables added.The new CSS custom properties for row backgrounds, header menu backgrounds, and alert colors are consistently defined across all theme variants (dark/light media queries and classes).
Also applies to: 53-55, 77-79, 99-101, 106-111
src/react/components/TableChartReactComponent.tsx (3)
12-23: LGTM! Well-structured type definitions.The
FeedbackAlertandFeedbackAlertComponentTypetypes provide a comprehensive structure for handling different alert types (error, success, warning, info) with optional debugging information.
25-49: LGTM! Clean alert rendering logic.The
FeedbackAlertComponentappropriately distinguishes between debug errors (with stack/traceback) and simple alerts, rendering the appropriate component for each case.
51-51: Theobserver()wrapper on TableChartReactComponent is necessary and correctly placed. The component receives MobX observables fromuseSlickGridReact()(config, dataStore, orderedParamColumns, sortedIndices, highlightedIndex) which are used in rendering and effect dependencies. The wrapper ensures the component re-renders when these observables change.src/react/hooks/useFindReplace.ts (3)
8-14: LGTM! Well-structured match type.The
FoundMatchtype appropriately captures all necessary information for tracking search matches, including both row and column indices plus the actual value.
33-39: LGTM! Correct disable logic with proper memoization.The disable flags correctly prevent navigation beyond array bounds and use
useMemoto avoid unnecessary recalculations.
198-269: LGTM! Solid replace logic with good error handling.The
handleReplacefunction properly:
- Validates column editability
- Checks for current match validity
- Provides appropriate feedback (success/warning/error)
- Updates the grid and refreshes the search
The separation of "no match" (warning) from actual errors is good UX.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/react/components/TableChartReactWrapper.tsx (1)
24-35: Move default value assignments to usethis.configaftersuper()call.The mutations to
config.column_widthsandconfig.include_indexon lines 27-28 target the original parameter, butBaseChart.initialiseChartConfig()makes a deep copy. These defaults should be set onthis.config:constructor(dataStore: DataStore, div: HTMLDivElement, config: TableChartReactConfig) { super(dataStore, div, config, TableChartComponent); - if (!config.column_widths) config.column_widths = {}; - if (config.include_index === undefined || config.include_index === null) config.include_index = false; + if (!this.config.column_widths) this.config.column_widths = {}; + if (this.config.include_index === undefined || this.config.include_index === null) this.config.include_index = false; // Extending config to make config.sort observablesrc/react/utils/valueReplacementUtil.ts (1)
166-175:parseFloatmay accept fractional values for integer columns.Using
Number.parseFloatforint32andintegerdatatypes will accept and store fractional values like"3.14", which may not be the intended behavior.🔎 Proposed fix for integer validation
// numeric if (datatype === "double" || datatype === "int32" || datatype === "integer") { const numValue = Number.parseFloat(newValue); if (!Number.isFinite(numValue)) { throw new Error(`Invalid number value "${newValue}" for ${datatype} column: ${column.field}`); } + // For integer types, truncate to whole number + if (datatype === "int32" || datatype === "integer") { + data[dataIndex] = Math.trunc(numValue); + return; + } + data[dataIndex] = numValue; return; }
🧹 Nitpick comments (3)
src/tests/table_react/hooks/useEditCell.test.ts (1)
66-92: Test should verify that the old cell value was stored.The
handleBeforeEditCelltest invokes the handler but doesn't assert any outcome. Consider verifying the stored value, perhaps by exposing it through the hook's return value or by testing the subsequenthandleCellChangebehavior.act(() => { result.current.handleBeforeEditCell(mockEvent); }); + + // Verify old value storage by attempting a cell change and checking the feedback + // contains the old value, or expose oldCellValue from the hook for direct assertion });src/tests/table_react/utils/valueReplacementUtils.test.tsx (1)
62-89: Consider adding test for integer type to complement the past review feedback.Given the past review flagged that
parseFloataccepts fractional values for integer columns, consider adding a test case forint32orintegerdatatypes to document expected behavior:test("should handle integer column", () => { column = { field: "Col1", data: [1, 2, 3], datatype: "integer", } as any; setCellValueFromString(column, 0, "5"); expect(column.data[0]).toBe(5); }); test("should handle fractional value for integer column", () => { column = { field: "Col1", data: [1, 2, 3], datatype: "integer", } as any; // Document current behavior or expected behavior after fix setCellValueFromString(column, 0, "3.14"); // expect(...) based on desired behavior });src/react/utils/valueReplacementUtil.ts (1)
12-23: Consider adding length limit to mitigate potential ReDoS.While the special character escaping at line 18 prevents regex injection, the static analysis tool correctly notes that extremely long input strings could still cause performance issues. Consider adding a length guard:
export const replaceValueInString = (findValue: string, replaceValue: string, cellValue: string) => { if (!findValue || !cellValue) { return cellValue; } + + // Guard against excessively long search terms + const MAX_FIND_LENGTH = 1000; + if (findValue.length > MAX_FIND_LENGTH) { + console.warn(`Find value exceeds ${MAX_FIND_LENGTH} characters, truncating`); + findValue = findValue.slice(0, MAX_FIND_LENGTH); + } // Escape special characters const escapedFind = findValue.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonsrc/react/components/TableChartReactComponent.tsxsrc/react/components/TableChartReactWrapper.tsxsrc/react/hooks/useFindReplace.tssrc/react/utils/valueReplacementUtil.tssrc/tests/table_react/hooks/useEditCell.test.tssrc/tests/table_react/hooks/useFindReplace.test.tssrc/tests/table_react/hooks/useSlickGridReact.test.tssrc/tests/table_react/hooks/useSortedIndices.test.tssrc/tests/table_react/utils/valueReplacementUtils.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- src/react/hooks/useFindReplace.ts
- src/react/components/TableChartReactComponent.tsx
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The project uses MobX for state management in chart configurations, with `mobxAutorun` and reaction disposers to manage reactive updates based on changes to linked data sources.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:22-22
Timestamp: 2025-06-04T10:03:08.161Z
Learning: In the MDV project, React hooks provide a more natural and simpler pattern for MobX integration compared to class-based charts, as hooks automatically handle lifecycle management and cleanup in a way that aligns better with reactive programming patterns.
📚 Learning: 2025-03-20T13:39:49.385Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 150
File: src/react/components/ColumnDropdownComponent.tsx:110-110
Timestamp: 2025-03-20T13:39:49.385Z
Learning: In ColumnDropdownComponent.tsx, when handling type issues with setValue in the handleSelectAll function, type assertions like (setValue as (v: DataColumn<DataType>[]) => void)([]) should be used instead of ts-expect-error comments when TypeScript can't automatically infer that the function should accept different parameter types based on a runtime condition.
Applied to files:
src/tests/table_react/hooks/useEditCell.test.tssrc/react/utils/valueReplacementUtil.ts
📚 Learning: 2025-12-19T11:28:13.803Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 232
File: src/react/contour_state.ts:291-320
Timestamp: 2025-12-19T11:28:13.803Z
Learning: In the MDV codebase, when fixing settings-related logic, prefer keeping the logic localized inside settings functions rather than moving it into constructors (e.g., chart constructors). Returning disposers from settings functions is a good pattern because it preserves locality while ensuring proper cleanup, and it aligns with future node-graph interface plans. This guideline applies to TypeScript files across the project (especially settings modules); avoid moving settings logic into constructors and instead return disposable cleanup functions from the appropriate settings functions.
Applied to files:
src/tests/table_react/hooks/useEditCell.test.tssrc/tests/table_react/hooks/useSlickGridReact.test.tssrc/tests/table_react/hooks/useFindReplace.test.tssrc/tests/table_react/hooks/useSortedIndices.test.tssrc/react/utils/valueReplacementUtil.ts
📚 Learning: 2025-06-04T10:03:08.161Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:22-22
Timestamp: 2025-06-04T10:03:08.161Z
Learning: In the MDV project, React hooks provide a more natural and simpler pattern for MobX integration compared to class-based charts, as hooks automatically handle lifecycle management and cleanup in a way that aligns better with reactive programming patterns.
Applied to files:
src/tests/table_react/hooks/useSlickGridReact.test.tssrc/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-03-17T15:08:26.492Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The project uses MobX for state management in chart configurations, with `mobxAutorun` and reaction disposers to manage reactive updates based on changes to linked data sources.
Applied to files:
src/tests/table_react/hooks/useSlickGridReact.test.tssrc/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-03-10T17:21:53.090Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 143
File: src/react/components/ColumnDropdownComponent.tsx:99-99
Timestamp: 2025-03-10T17:21:53.090Z
Learning: In this codebase, `ts-expect-error` comments are used as temporary markers to flag TypeScript issues that need proper solutions before merging to stable branches. They indicate work-in-progress code rather than permanent workarounds. The objective is to resolve all of these issues with proper typing solutions.
Applied to files:
src/react/utils/valueReplacementUtil.ts
📚 Learning: 2025-05-22T11:21:43.866Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 202
File: src/charts/TableChart.js:18-19
Timestamp: 2025-05-22T11:21:43.866Z
Learning: In the TableChart component, the condition for including the index column was changed from `config.include_index !== false` to `config?.include_index`, making it opt-in rather than opt-out. This means the index column is only included when explicitly enabled rather than being included by default.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-12-19T11:26:47.659Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 232
File: src/react/components/DeckScatterReactWrapper.tsx:45-47
Timestamp: 2025-12-19T11:26:47.659Z
Learning: In DeckScatterReactWrapper.tsx, the adaptConfig function incorrectly assigns config.param[2] to contourParameter without considering chart types: DeckDensity uses param[2] for density fields (_multi_column:number), DeckScatter3D uses it for z-axis (numeric), and DeckScatter 2D has no param[2]. This initialization should be removed entirely and contourParameter should remain undefined unless explicitly configured. Tracked in issue #303.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-03-17T12:01:42.608Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 144
File: src/charts/dialogs/AddViewDialog.tsx:51-90
Timestamp: 2025-03-17T12:01:42.608Z
Learning: When refactoring MDV dialog components that interact with the chart manager (cm), prefer direct method calls (e.g., `cm.getState()`) over destructuring methods and using .apply() (e.g., `const { getState } = cm; getState.apply(cm, [])`). Direct method calls maintain the correct context and are more readable.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-03-17T12:30:48.592Z
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 144
File: src/charts/dialogs/AddViewDialog.tsx:51-90
Timestamp: 2025-03-17T12:30:48.592Z
Learning: When replacing .apply() calls in MDV dialog components, use direct method calls on the chart manager instance (e.g., `cm.removeAllCharts()`) rather than calling destructured methods directly (e.g., `removeAllCharts()`). This ensures proper `this` context is maintained without the verbosity of .apply().
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-03-17T11:33:53.258Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 144
File: src/charts/dialogs/AddViewDialog.tsx:51-90
Timestamp: 2025-03-17T11:33:53.258Z
Learning: In the MDV codebase, when working with ChartManager methods, it's better to use direct method calls on the ChartManager instance (e.g., `cm.getState()`) rather than destructuring the methods and using `.apply()` (e.g., `const { getState } = cm; getState.apply(cm, [])`). Direct method calls maintain the correct `this` context more cleanly.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-12-19T11:28:13.803Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 232
File: src/react/contour_state.ts:291-320
Timestamp: 2025-12-19T11:28:13.803Z
Learning: In src/react/contour_state.ts, the getDensitySettings function creates a mobxAutorun on every call (each time settings panel builds), causing reaction leaks. The preferred fix is to have getDensitySettings return a disposer that the settings dialog can call when it closes, keeping the logic localized while properly handling cleanup. This is tracked in issue #304.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-06-04T09:48:26.189Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:17-17
Timestamp: 2025-06-04T09:48:26.189Z
Learning: In the MDV project, while BaseChart.mobxAutorun automatically cleans up disposers on chart destruction, manual disposer management is still required in complex scenarios where disposers are changed at runtime (e.g., ColumnQueryMapper in decorateColumnMethods.ts). Even though remaining disposers will be cleaned up when the chart is destroyed, proper housekeeping during runtime is important to prevent memory leaks.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-04-23T08:48:20.916Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/DeckScatterReactWrapper.tsx:42-44
Timestamp: 2025-04-23T08:48:20.916Z
Learning: In DeckScatterReactWrapper.tsx, properties like maxZoom and minZoom in viewState are currently treated as constants rather than user-configurable settings, and are not preserved during serialization in getConfig().
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-12-19T11:28:13.803Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 232
File: src/react/contour_state.ts:291-320
Timestamp: 2025-12-19T11:28:13.803Z
Learning: When fixing settings-related code in the MDV codebase, xinaesthete prefers keeping the logic localized within settings functions rather than moving it to chart constructors, in preparation for future node-graph interface plans. Returning disposers from settings functions is a good pattern that maintains locality while ensuring proper cleanup.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-04-23T09:26:52.252Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 162
File: src/react/components/AxisComponent.tsx:46-46
Timestamp: 2025-04-23T09:26:52.252Z
Learning: In React dependency arrays, conditional expressions (e.g., `is2d && config.axis.x.size`) can be necessary for type safety when some properties might not exist on all possible types. In these cases, extracting safe values before useMemo or adding a lint exception with explanation is preferred.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-04-09T14:32:53.708Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T14:32:53.708Z
Learning: When implementing React components, the MobX `observer()` wrapper is only necessary for components that directly access MobX observables or stores. Components that exclusively use props and local React state (useState, useRef, etc.) don't need to be wrapped with observer() as they will re-render appropriately when props change through normal React mechanisms.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-04-09T14:32:53.708Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T14:32:53.708Z
Learning: In React components that don't directly access MobX observables or stores, the `observer()` wrapper is unnecessary. Components that only receive data via props and use standard React hooks for internal state don't need to be MobX observers, as they will re-render appropriately when props change.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-06-04T10:19:55.042Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:0-0
Timestamp: 2025-06-04T10:19:55.042Z
Learning: In the MDV codebase, BaseChart.ts provides automatic disposer management through the `mobxAutorun()` method which tracks disposers in a `reactionDisposers` array and automatically disposes them in the `remove()` method. When using direct `reaction()` calls instead of `mobxAutorun()`, manual disposer management is required.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
📚 Learning: 2025-06-04T09:41:18.914Z
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:17-17
Timestamp: 2025-06-04T09:41:18.914Z
Learning: In the MDV project, BaseChart.mobxAutorun automatically stores the disposer and runs it when the chart is destroyed, eliminating the need for manual cleanup of MobX autorun reactions in derived chart classes.
Applied to files:
src/react/components/TableChartReactWrapper.tsx
🧬 Code graph analysis (7)
src/tests/table_react/utils/valueReplacementUtils.test.tsx (2)
src/react/utils/valueReplacementUtil.ts (3)
replaceValueInString(12-23)setCellValueFromString(143-237)replaceMatches(251-301)src/charts/charts.d.ts (1)
LoadedDataColumn(123-124)
src/tests/table_react/hooks/useEditCell.test.ts (1)
src/charts/charts.d.ts (1)
LoadedDataColumn(123-124)
src/tests/table_react/hooks/useSlickGridReact.test.ts (2)
src/react/components/TableChartReactWrapper.tsx (1)
TableChartReactConfig(14-19)src/charts/charts.d.ts (1)
LoadedDataColumn(123-124)
src/tests/table_react/hooks/useFindReplace.test.ts (1)
src/charts/charts.d.ts (1)
LoadedDataColumn(123-124)
src/tests/table_react/hooks/useSortedIndices.test.ts (1)
src/react/components/TableChartReactWrapper.tsx (1)
TableChartReactConfig(14-19)
src/react/utils/valueReplacementUtil.ts (1)
src/charts/charts.d.ts (1)
LoadedDataColumn(123-124)
src/react/components/TableChartReactWrapper.tsx (1)
src/charts/BaseChart.ts (1)
BaseConfig(22-30)
🪛 ast-grep (0.40.3)
src/react/utils/valueReplacementUtil.ts
[warning] 20-20: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(escapedFind, "gi")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (11)
src/tests/table_react/hooks/useSortedIndices.test.ts (1)
1-81: LGTM! Well-structured test suite for sorting hook.The tests cover default (unsorted), ascending, and descending sort scenarios with correct expected indices. The MobX observable config pattern and mock setup are appropriate.
Consider adding edge case tests for robustness:
- When
sort.columnIdreferences a non-existent column- When
mockFilteredIndicesis emptysrc/tests/table_react/hooks/useSlickGridReact.test.ts (1)
1-255: LGTM! Comprehensive test coverage for the SlickGrid React integration hook.The test suite effectively covers:
- Initialization and default state
- Column definitions with/without index column
- Header menu configuration
- Column width application from config
- Editable column detection
- Grid creation and initialization
- Find/replace dialog state management
- Reactive config updates
The mock setup is thorough and the test organization is clear.
src/react/components/TableChartReactWrapper.tsx (2)
45-59: LGTM! Settings implementation follows project patterns.The
getSettingsmethod correctly extends parent settings and uses MobXactionwrapper for the checkbox callback, following the project's established patterns for reactive state management.
63-83: LGTM! Chart type registration is properly configured.The registration includes appropriate params, extra controls, and init function to propagate the
include_indexsetting.src/tests/table_react/hooks/useFindReplace.test.ts (1)
1-496: LGTM! Thorough test coverage for find/replace functionality.The test suite comprehensively covers:
- Finding matches in text and numeric columns
- Empty and invalid search handling with proper error feedback
- Navigation between matches with boundary conditions
- Single replace with editability validation
- Replace all with success/error feedback
- State reset functionality
The mock setup accurately simulates the grid and data store interactions.
src/tests/table_react/hooks/useEditCell.test.ts (1)
94-217: LGTM! Cell change handler tests cover key scenarios.The tests properly verify:
- Successful edit with feedback, dataChanged call, and grid refresh
- Error handling for non-existent columns
- Error handling for non-editable columns
src/tests/table_react/utils/valueReplacementUtils.test.tsx (1)
1-288: LGTM! Comprehensive utility function test coverage.The test suite thoroughly covers:
- String replacement with empty/valid/edge cases
- All supported datatypes (text, numeric, multitext, unique)
- Error conditions for missing data, invalid values, and bounds checking
replaceMatchessuccess and error pathssrc/react/utils/valueReplacementUtil.ts (4)
36-50: LGTM! Value index management with proper bounds checking.The
getValueIndexhelper correctly handles adding new values to the values array while respecting the maximum limit for each datatype.
60-133: LGTM! Comprehensive cell value retrieval with robust error handling.The
getCurrentCellValuefunction properly handles all supported datatypes with appropriate validation:
- Text/text16: bounds checking on value index
- Numeric: NaN handling
- Multitext: proper slice and filter with bounds validation
- Unique: TextDecoder with null-padding strip
251-301: LGTM! Well-structured replace operation with comprehensive error handling.The
replaceMatchesfunction properly:
- Validates column existence and data availability
- Handles case-insensitive matching
- Returns false for no-match scenarios instead of throwing
- Wraps errors from helper functions with context
177-206: Remove the "//! review this" comment — the implementation is correct and aligns with the data structure.The multitext logic is consistent across reading and writing operations:
stringLengthcorrectly defines fixed slots per row (baseIndex calculation)- Empty slots marked with
65535is confirmed by the reading function filter- Comma-separated input parsing with
.trim()and.filter()handles spacing correctlymaxValues = 65536matches the Uint16Array constraint and test patternsAll assumptions are validated by test cases and by the corresponding read-back logic at lines 110–130.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/react/hooks/useFindReplace.ts`:
- Around line 179-199: The handleReplace callback lacks validation for
replaceValue; add a guard in handleReplace to trim and ensure replaceValue is
not an empty string (e.g., const safeReplace = replaceValue?.trim()) and if
empty either throw a descriptive Error or return early depending on UX (choose
one and apply consistently). Keep this check near the existing guards that
validate searchColumn, column.editable, and current match (using searchColumn,
orderedParamColumns, foundMatches, currentMatchIndex) and update the thrown
error/log message to reference replaceValue so callers can see why the replace
was rejected.
♻️ Duplicate comments (3)
src/react/hooks/useFindReplace.ts (1)
69-86: Search still risks mismatching displayed values.
String(value)fromcolumn.getValue()can diverge from the formatted cell text (e.g., separators/precision). Consider using the display text (e.g.,dataStore.getRowText) with a fallback. This mirrors the earlier numeric-search concern.🛠️ Possible adjustment
- const value = column.getValue(dataIndex); - if (value === null) continue; - - const displayedValue = String(value); + const rawValue = column.getValue(dataIndex); + if (rawValue == null) continue; + const displayedValue = + dataStore.getRowText?.(dataIndex, column.field) ?? String(rawValue);src/react/hooks/useSlickGridReact.ts (2)
92-103:grid.invalidate()should be followed bygrid.render().Other grid update paths render after invalidation; this path currently doesn’t, which can leave stale UI. This repeats a prior issue.
🛠️ Fix
grid.setData(dataProvider, true); grid.invalidate(); + grid.render(); console.log("Grid updated");
135-150: Selection clearing isn’t propagated to the datastore.When
selectedRowsis empty, the handler returns without clearing highlights, so clearing selection in the grid won’t clear highlights elsewhere. This was flagged previously and appears regressed.🛠️ Suggested fix
- slickEventHandler.subscribe(grid.onSelectedRowsChanged, (_e, args) => { - const selectedRows = args.rows; + slickEventHandler.subscribe(grid.onSelectedRowsChanged, (_e, args) => { + const selectedRows = args.rows ?? []; // Skip if we're programmatically updating if (selectionSourceRef.current === 'programmatic') { selectionSourceRef.current = null; return; } - if (selectedRows.length > 0) { - selectionSourceRef.current = 'user'; - const indices = selectedRows.map((row) => sortedFilteredIndicesRef.current[row]); - dataStoreRef.current.dataHighlighted(indices, chartRef.current); - // Reset immediately - the effect will handle any needed updates - selectionSourceRef.current = null; - } + selectionSourceRef.current = 'user'; + const indices = selectedRows + .map((row) => sortedFilteredIndicesRef.current[row]) + .filter((idx): idx is number => idx !== undefined); + dataStoreRef.current.dataHighlighted(indices, chartRef.current); + selectionSourceRef.current = null; });
src/react/hooks/useFindReplace.ts
Outdated
| //! Need a check for replace value | ||
| const handleReplace = useCallback( | ||
| (findValue: string, replaceValue: string) => { | ||
| try { | ||
| if (!searchColumn) { | ||
| console.log("No column selected for replace"); | ||
| throw new Error("No column selected for replace"); | ||
| } | ||
|
|
||
| const column = orderedParamColumns.find((col) => col.field === searchColumn); | ||
|
|
||
| if (!column?.editable) { | ||
| console.error(`Column ${searchColumn} is not editable`); | ||
| throw new Error(`Column ${searchColumn} is not editable`); | ||
| } | ||
|
|
||
| if (foundMatches.length === 0 || currentMatchIndex < 0 || currentMatchIndex >= foundMatches.length) { | ||
| console.log("No current match found for replace value: ", replaceValue); | ||
| throw new Error(`No current match found for replace value: ${replaceValue}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
Resolve the replace-value TODO before merge.
There’s an explicit //! Need a check for replace value comment but no validation. Decide whether empty replace is allowed and enforce it consistently.
If you want, I can draft the exact guard based on the intended UX.
🤖 Prompt for AI Agents
In `@src/react/hooks/useFindReplace.ts` around lines 179 - 199, The handleReplace
callback lacks validation for replaceValue; add a guard in handleReplace to trim
and ensure replaceValue is not an empty string (e.g., const safeReplace =
replaceValue?.trim()) and if empty either throw a descriptive Error or return
early depending on UX (choose one and apply consistently). Keep this check near
the existing guards that validate searchColumn, column.editable, and current
match (using searchColumn, orderedParamColumns, foundMatches, currentMatchIndex)
and update the thrown error/log message to reference replaceValue so callers can
see why the replace was rejected.
…internally and explicitly serialised.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/react/components/TableChartReactWrapper.tsx`:
- Around line 78-82: The loop that writes into columnWidths uses col.field
without checking it, causing entries like columnWidths[undefined]; in the block
iterating "columns" (in TableChartReactWrapper.tsx) guard against missing fields
by only assigning columnWidths[col.field] when col.field is present/valid (e.g.,
truthy string) and col.width !== 100; update the loop that references "columns"
and the "columnWidths" object to skip columns with no field (e.g.,
checkbox/row-number columns) to avoid persisting undefined keys.
♻️ Duplicate comments (2)
src/react/hooks/useSlickGridReact.ts (2)
139-149: Addgrid.render()after invalidation to ensure UI updates.
Line 148 only invalidates; without a render pass, updates can remain visually stale.🛠️ Proposed fix
grid.setData(dataProvider, true); grid.invalidate(); + grid.render(); console.log("Grid updated");
185-201: Propagate cleared selections to keep store in sync.
Right now, clearing selection doesn’t emit, so highlights can remain stuck in the datastore.🛠️ Proposed fix
- slickEventHandler.subscribe(grid.onSelectedRowsChanged, (_e, args) => { - const selectedRows = args.rows; + slickEventHandler.subscribe(grid.onSelectedRowsChanged, (_e, args) => { + const selectedRows = args.rows ?? []; // Skip if we're programmatically updating if (selectionSourceRef.current === 'programmatic') { selectionSourceRef.current = null; return; } - if (selectedRows.length > 0) { - selectionSourceRef.current = 'user'; - const indices = selectedRows.map((row) => sortedFilteredIndicesRef.current[row]); - dataStoreRef.current.dataHighlighted(indices, chartRef.current); - // Reset immediately - the effect will handle any needed updates - selectionSourceRef.current = null; - } + selectionSourceRef.current = 'user'; + const indices = selectedRows + .map((row) => sortedFilteredIndicesRef.current[row]) + .filter((idx): idx is number => idx !== undefined); + // Emit even empty selection to clear highlights + dataStoreRef.current.dataHighlighted(indices, chartRef.current); + selectionSourceRef.current = null; });
🧹 Nitpick comments (1)
src/react/components/TableChartReactWrapper.tsx (1)
129-132: Consider explicit default in init to match adaptConfig behavior.The comment on line 130 notes that
ec.include_indexmay be undefined. WhileadaptConfigwill normalize this tofalselater, using an explicit default here improves clarity and consistency:♻️ Optional improvement
init: (config, ds, ec) => { - // nb - may be undefined if user hasn't touched that control - config.include_index = ec.include_index; + config.include_index = ec.include_index ?? false; },
| for (const col of columns) { | ||
| if (col.width && col.width !== 100) { | ||
| columnWidths[col.field] = col.width; | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against columns without a field property.
Some SlickGrid columns (e.g., checkbox selector, row number columns) may not have a field property, leading to columnWidths[undefined] entries being persisted.
🔧 Suggested fix
for (const col of columns) {
- if (col.width && col.width !== 100) {
+ if (col.field && col.width && col.width !== 100) {
columnWidths[col.field] = col.width;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const col of columns) { | |
| if (col.width && col.width !== 100) { | |
| columnWidths[col.field] = col.width; | |
| } | |
| } | |
| for (const col of columns) { | |
| if (col.field && col.width && col.width !== 100) { | |
| columnWidths[col.field] = col.width; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/react/components/TableChartReactWrapper.tsx` around lines 78 - 82, The
loop that writes into columnWidths uses col.field without checking it, causing
entries like columnWidths[undefined]; in the block iterating "columns" (in
TableChartReactWrapper.tsx) guard against missing fields by only assigning
columnWidths[col.field] when col.field is present/valid (e.g., truthy string)
and col.width !== 100; update the loop that references "columns" and the
"columnWidths" object to skip columns with no field (e.g., checkbox/row-number
columns) to avoid persisting undefined keys.
…a to avoid unexpected side-effects
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/react/selectionHooks.ts`:
- Around line 19-22: The initial state for highlightedIndices (set via useState)
currently calls dataStore.highightedData?.slice() which will throw if
highightedData is not an array; change the initializer to normalize the value
the same way the listener does by checking
Array.isArray(dataStore.highightedData) (or using a safe cast) and using
dataStore.highightedData.slice() only when it is an array, otherwise default to
an empty array; update the useState call that sets highlightedIndices and keep
setHighlightedIndices behavior unchanged.
♻️ Duplicate comments (1)
src/charts/css/charts.css (1)
756-757: Selection color still maps to the error token.Line 756 uses
--background_color_errorfor selection, which reads as a semantic mismatch. Consider switching to the new info token (or a dedicated selection token).♻️ Minimal tweak (uses existing token)
.slick-cell.selected { - background-color: var(--background_color_error) !important; + background-color: var(--background_color_info) !important; }
🧹 Nitpick comments (1)
src/charts/css/charts.css (1)
689-691: Add WebKit scrollbar styling to avoid light-mode dark scrollbars.Line 689’s
scrollbar-coloronly affects Firefox. If the reported light-mode scrollbar mismatch is in Chromium, add::-webkit-scrollbar*rules so the track/thumb use the new variables.♻️ Suggested patch
.slick-viewport { scrollbar-color: var(--scroll_bar_thumb) var(--scroll_bar_track) !important; + scrollbar-width: thin; } + + .slick-viewport::-webkit-scrollbar { + width: 12px; + height: 12px; + } + .slick-viewport::-webkit-scrollbar-track { + background: var(--scroll_bar_track); + } + .slick-viewport::-webkit-scrollbar-thumb { + background-color: var(--scroll_bar_thumb); + border-radius: 8px; + border: 3px solid var(--scroll_bar_track); + }
| // Create a copy to avoid a reference to mutable state - not that we anticipate an actual issue in this case. | ||
| const [highlightedIndices, setHighlightedIndices] = useState<number[]>( | ||
| dataStore.highightedData?.slice() || [] | ||
| ); |
There was a problem hiding this comment.
Guard initial highlighted state against non-array shape.
If dataStore.highightedData can be an object (the listener already handles that), .slice() will throw at mount. Normalize the initial value the same way as the listener to avoid a runtime crash.
🐛 Proposed fix
- const [highlightedIndices, setHighlightedIndices] = useState<number[]>(
- dataStore.highightedData?.slice() || []
- );
+ const [highlightedIndices, setHighlightedIndices] = useState<number[]>(() => {
+ const initial = dataStore.highightedData;
+ if (!initial) return [];
+ return Array.isArray(initial) ? initial.slice() : Object.values(initial);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create a copy to avoid a reference to mutable state - not that we anticipate an actual issue in this case. | |
| const [highlightedIndices, setHighlightedIndices] = useState<number[]>( | |
| dataStore.highightedData?.slice() || [] | |
| ); | |
| // Create a copy to avoid a reference to mutable state - not that we anticipate an actual issue in this case. | |
| const [highlightedIndices, setHighlightedIndices] = useState<number[]>(() => { | |
| const initial = dataStore.highightedData; | |
| if (!initial) return []; | |
| return Array.isArray(initial) ? initial.slice() : Object.values(initial); | |
| }); |
🤖 Prompt for AI Agents
In `@src/react/selectionHooks.ts` around lines 19 - 22, The initial state for
highlightedIndices (set via useState) currently calls
dataStore.highightedData?.slice() which will throw if highightedData is not an
array; change the initializer to normalize the value the same way the listener
does by checking Array.isArray(dataStore.highightedData) (or using a safe cast)
and using dataStore.highightedData.slice() only when it is an array, otherwise
default to an empty array; update the useState call that sets highlightedIndices
and keep setHighlightedIndices behavior unchanged.
|
State management changed somewhat to avoid contention between internal This currently applies particularly to One thing that is still pending is the handling of scrolling the view to reflect changes to highlighted rows, and I also need to test edit functionality with an appropriate project. |
- refactor useSlickGridReact to have all event handlers created when grid is created - comment the setting sort column in handleGridCreated - replace gridInstance state with gridReady as we already have gridRef - remove the redundant observer in the wrapper - move FeedbackAlert component to a new file
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/tests/table_react/hooks/useEditCell.test.ts`:
- Around line 83-109: The test currently invokes useEditCell and calls
handleBeforeEditCell but has no assertions; update the test to verify the old
value was stored by asserting the hook's exposed storage (e.g.,
result.current.oldCellValueRef or result.current.previousCellValuesRef) contains
sample_count === 10 after act(), or if the hook doesn't expose such a ref,
modify useEditCell to return a previousCellValuesRef/getter and then assert that
previousCellValuesRef.current['sample_count'] === 10; use the
handleBeforeEditCell and useEditCell symbols to locate where to read or expose
the stored old value.
🧹 Nitpick comments (2)
src/tests/table_react/hooks/useEditCell.test.ts (2)
65-81: Test does not verify graceful handling behavior.The test name suggests it should verify the hook handles a null grid reference gracefully, but it only asserts that the handlers are defined. Consider actually invoking the handlers to verify they don't throw or crash when the grid is null.
♻️ Suggested improvement
test("should handle null grid reference gracefully", () => { const gridRefWithNull = { current: null } as any; const { result } = renderHook(() => useEditCell( orderedParamColumnsRef, sortedIndicesRef, dataStore, gridRefWithNull, setFeedbackAlert, ), ); // Should not crash when grid is null expect(result.current.handleBeforeEditCell).toBeDefined(); expect(result.current.handleCellChange).toBeDefined(); + + // Actually invoke handlers to verify no crash + const mockEvent = new CustomEvent("test", { + detail: { + eventData: {}, + args: { + item: { sample_count: 10 }, + column: { field: "sample_count" }, + }, + }, + }) as any; + + expect(() => { + act(() => { + result.current.handleBeforeEditCell(mockEvent); + }); + }).not.toThrow(); });
165-198: Error tests don't require priorbeforeEditCellcalls for functional correctness, but adding them would improve realism.The "column not found" and "column is not editable" tests skip the
handleBeforeEditCellsetup. However, the hook's error detection logic (lines 104–112 in the implementation) does not depend on state frombeforeEditCell—it validates the column directly fromorderedParamColumnsRefand checks theeditableflag. TheoldValueis only used in the success message and error metadata, not in the error path logic.The tests are functionally accurate without prior
beforeEditCellcalls. Still, adding them would improve consistency with the actual event flow, wherebeforeEditCellfires beforehandleCellChangein real usage.
| describe("handleBeforeEditCell", () => { | ||
| test("should store old cell value", () => { | ||
| const { result } = renderHook(() => | ||
| useEditCell( | ||
| orderedParamColumnsRef, | ||
| sortedIndicesRef, | ||
| dataStore, | ||
| gridRef, | ||
| setFeedbackAlert, | ||
| ), | ||
| ); | ||
|
|
||
| const mockEvent = new CustomEvent("beforeEditCell", { | ||
| detail: { | ||
| eventData: {}, | ||
| args: { | ||
| item: { sample_count: 10 }, | ||
| column: { field: "sample_count" }, | ||
| } as OnBeforeEditCellEventArgs, | ||
| }, | ||
| }) as any; | ||
|
|
||
| act(() => { | ||
| result.current.handleBeforeEditCell(mockEvent); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test has no assertions — it cannot verify correctness.
This test calls handleBeforeEditCell but never asserts that the old cell value was actually stored. Without assertions, the test will always pass regardless of the hook's behavior, making it ineffective for catching regressions.
🧪 Suggested fix: add meaningful assertions
If the hook exposes the stored old value (e.g., via a ref or return value), assert against it. Alternatively, verify the subsequent handleCellChange behavior depends on the stored value:
test("should store old cell value", () => {
const { result } = renderHook(() =>
useEditCell(
orderedParamColumnsRef,
sortedIndicesRef,
dataStore,
gridRef,
setFeedbackAlert,
),
);
const mockEvent = new CustomEvent("beforeEditCell", {
detail: {
eventData: {},
args: {
item: { sample_count: 10 },
column: { field: "sample_count" },
} as OnBeforeEditCellEventArgs,
},
}) as any;
act(() => {
result.current.handleBeforeEditCell(mockEvent);
});
+
+ // Verify old value is stored by triggering a cell change
+ // and checking that the edit logic has access to the previous value
+ const changeEvent = new CustomEvent("cellChange", {
+ detail: {
+ eventData: {},
+ args: {
+ row: 0,
+ column: { field: "sample_count" },
+ item: { sample_count: 15 },
+ } as OnCellChangeEventArgs,
+ },
+ }) as any;
+
+ act(() => {
+ result.current.handleCellChange(changeEvent);
+ });
+
+ // Assert that the edit succeeded (implying old value was correctly stored)
+ expect(setFeedbackAlert).toHaveBeenCalledWith(
+ expect.objectContaining({
+ type: "success",
+ }),
+ );
});🤖 Prompt for AI Agents
In `@src/tests/table_react/hooks/useEditCell.test.ts` around lines 83 - 109, The
test currently invokes useEditCell and calls handleBeforeEditCell but has no
assertions; update the test to verify the old value was stored by asserting the
hook's exposed storage (e.g., result.current.oldCellValueRef or
result.current.previousCellValuesRef) contains sample_count === 10 after act(),
or if the hook doesn't expose such a ref, modify useEditCell to return a
previousCellValuesRef/getter and then assert that
previousCellValuesRef.current['sample_count'] === 10; use the
handleBeforeEditCell and useEditCell symbols to locate where to read or expose
the stored old value.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/react/hooks/useSlickGridReact.ts`:
- Around line 201-217: The handler registered with slickEventHandler.subscribe
for grid.onSelectedRowsChanged currently ignores empty selections due to the
selectedRows.length > 0 guard; update the callback so that when
selectionSourceRef.current === 'programmatic' you still return as before, but
otherwise treat selectedRows.length === 0 as a valid “clear” event: set
selectionSourceRef.current = 'user' (or a transient value), call
dataStore.dataHighlighted([] , chart) to propagate the cleared highlight, and
then reset selectionSourceRef.current = null; keep mapping selectedRows via
sortedFilteredIndicesRef.current and the existing behavior for non-empty
selections.
🧹 Nitpick comments (3)
src/react/hooks/useSortedFilteredIndices.ts (1)
28-129: MobX autorun inside useEffect with overlapping dependencies causes redundant re-runs.The
autorunautomatically tracks MobX observables (likeconfig.sort), but you've also includedconfig.sortin the useEffect dependency array. Whenconfig.sortchanges:
- MobX triggers the autorun callback
- React re-runs the effect (creating a new autorun)
This causes the sorting logic to run twice on every sort change. Since
filteredIndicescomes from React state (viauseSimplerFilteredIndices), you need the effect, but you should removeconfig.sortfrom the dependency array and let autorun handle MobX tracking.♻️ Proposed fix
}, [filteredIndices, dataStore, config.sort]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [filteredIndices, dataStore]); + // Note: config.sort is intentionally excluded - MobX autorun tracks it automaticallysrc/react/hooks/useSlickGridReact.ts (2)
173-179: Consider removing commented code.The commented block with the question "Do we need this?" can be removed. The sort sync is handled by the
useEffectat line 320-346 which watchesconfig.sortand updates the grid's visual sort state. This effect runs after grid creation and handles initial sort as well.🧹 Proposed cleanup
grid.render(); - //? Do we need this? We are syncing the sort in the useEffect below which does the same. - // Apply initial sort if config.sort is set - // if (config.sort) { - // suppressSortSyncRef.current = true; - // grid.setSortColumn(config.sort.columnId, config.sort.ascending); - // suppressSortSyncRef.current = false; - // } - attachEventHandlers(e.detail);
384-386: Linear search in loop may be slow for large datasets.
sortedFilteredIndices.indexOf(index)is O(n), called for each highlighted index. With many highlights and large datasets, this becomes O(n*m). Consider building a reverse lookup map.♻️ Proposed optimization
+ // Build reverse lookup map for O(1) lookups + const indexToPosition = new Map<number, number>(); + sortedFilteredIndices.forEach((idx, pos) => { + indexToPosition.set(idx, pos); + }); + const positions = validIndices - .map(index => sortedFilteredIndices.indexOf(index)) + .map(index => indexToPosition.get(index) ?? -1) .filter(pos => pos !== -1);
| slickEventHandler.subscribe(grid.onSelectedRowsChanged, (_e, args) => { | ||
| const selectedRows = args.rows; | ||
|
|
||
| // Skip if we're programmatically updating | ||
| if (selectionSourceRef.current === 'programmatic') { | ||
| selectionSourceRef.current = null; | ||
| return; | ||
| } | ||
|
|
||
| if (selectedRows.length > 0) { | ||
| selectionSourceRef.current = 'user'; | ||
| const indices = selectedRows.map((row) => sortedFilteredIndicesRef.current[row]); | ||
| dataStore.dataHighlighted(indices, chart); | ||
| // Reset immediately - the effect will handle any needed updates | ||
| selectionSourceRef.current = null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Empty selection not propagated to datastore.
When users clear selection (click empty area or Escape), the handler ignores it because of the selectedRows.length > 0 check. This means highlights won't clear in other charts/components that sync via the datastore.
The past review suggested propagating empty selections, but the current implementation still skips them.
🐛 Proposed fix to propagate cleared selections
slickEventHandler.subscribe(grid.onSelectedRowsChanged, (_e, args) => {
- const selectedRows = args.rows;
+ const selectedRows = args.rows ?? [];
// Skip if we're programmatically updating
if (selectionSourceRef.current === 'programmatic') {
selectionSourceRef.current = null;
return;
}
- if (selectedRows.length > 0) {
- selectionSourceRef.current = 'user';
- const indices = selectedRows.map((row) => sortedFilteredIndicesRef.current[row]);
- dataStore.dataHighlighted(indices, chart);
- // Reset immediately - the effect will handle any needed updates
- selectionSourceRef.current = null;
- }
+ selectionSourceRef.current = 'user';
+ const indices = selectedRows
+ .map((row) => sortedFilteredIndicesRef.current[row])
+ .filter((idx): idx is number => idx !== undefined);
+
+ // Emit even empty selection to clear highlights in other components
+ dataStore.dataHighlighted(indices, chart);
+ selectionSourceRef.current = null;
});🤖 Prompt for AI Agents
In `@src/react/hooks/useSlickGridReact.ts` around lines 201 - 217, The handler
registered with slickEventHandler.subscribe for grid.onSelectedRowsChanged
currently ignores empty selections due to the selectedRows.length > 0 guard;
update the callback so that when selectionSourceRef.current === 'programmatic'
you still return as before, but otherwise treat selectedRows.length === 0 as a
valid “clear” event: set selectionSourceRef.current = 'user' (or a transient
value), call dataStore.dataHighlighted([] , chart) to propagate the cleared
highlight, and then reset selectionSourceRef.current = null; keep mapping
selectedRows via sortedFilteredIndicesRef.current and the existing behavior for
non-empty selections.
There was a problem hiding this comment.
There isn't currently any way of unselecting the rows in the table
There was a problem hiding this comment.
@nawabfurquan, thank you for the clarification! That makes sense—if there's no way to clear the selection in the UI currently, then handling empty selections isn't needed.
If you later decide to add the ability to unselect rows (for example, by clicking an empty area, pressing ESC, or adding a "Clear Selection" button), you'll want to update this handler to propagate empty selections to the datastore so that highlights are properly cleared across other charts and components.
For now, the current implementation is fine given the existing functionality. 👍
🧠 Learnings used
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-03-17T15:08:26.492Z
Learning: The project uses MobX for state management in chart configurations, with `mobxAutorun` and reaction disposers to manage reactive updates based on changes to linked data sources.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 220
File: src/charts/CategoryChart.js:22-22
Timestamp: 2025-06-04T10:03:08.161Z
Learning: In the MDV project, React hooks provide a more natural and simpler pattern for MobX integration compared to class-based charts, as hooks automatically handle lifecycle management and cleanup in a way that aligns better with reactive programming patterns.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T20:29:56.245Z
Learning: When working with D3.js in React components, it's appropriate to intentionally exclude certain dependencies from useEffect dependency arrays to prevent unnecessary recreation of D3 elements. This approach should be documented with explanatory comments and ESLint suppression comments like `// eslint-disable-next-line react-hooks/exhaustive-deps` to clarify the intentional exclusion.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 190
File: src/react/components/avivatorish/MDVivViewer.tsx:174-174
Timestamp: 2025-04-30T12:28:45.385Z
Learning: In React class components, when a function returns a cleanup function (like many hooks do in functional components), it should be:
1. Stored as a class instance property
2. Called before setting up new effects when dependencies change
3. Called in the componentWillUnmount lifecycle method
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T14:32:53.708Z
Learning: When implementing React components, the MobX `observer()` wrapper is only necessary for components that directly access MobX observables or stores. Components that exclusively use props and local React state (useState, useRef, etc.) don't need to be wrapped with observer() as they will re-render appropriately when props change through normal React mechanisms.
Learnt from: xinaesthete
Repo: Taylor-CCB-Group/MDV PR: 0
File: :0-0
Timestamp: 2025-04-09T14:32:53.708Z
Learning: In React components that don't directly access MobX observables or stores, the `observer()` wrapper is unnecessary. Components that only receive data via props and use standard React hooks for internal state don't need to be MobX observers, as they will re-render appropriately when props change.
Learnt from: nawabfurquan
Repo: Taylor-CCB-Group/MDV PR: 150
File: src/react/components/ColumnDropdownComponent.tsx:110-110
Timestamp: 2025-03-20T13:39:49.385Z
Learning: In ColumnDropdownComponent.tsx, when handling type issues with setValue in the handleSelectAll function, type assertions like (setValue as (v: DataColumn<DataType>[]) => void)([]) should be used instead of ts-expect-error comments when TypeScript can't automatically infer that the function should accept different parameter types based on a runtime condition.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/extradocs/table_react.md`:
- Around line 51-52: Fix the subject-verb agreement in the sentence mentioning
refs: change "useEditCell and useSlickGridReact uses refs" to "useEditCell and
useSlickGridReact use refs" (the sentence referencing orderedParamColumnsRef and
sortedFilteredIndicesRef should use the plural verb "use"). Ensure the updated
phrasing still mentions the reason (SlickGrid stores callback references and we
need to avoid stale closures).
🧹 Nitpick comments (1)
src/react/hooks/useSortedFilteredIndices.ts (1)
87-90: Trim null padding when decodinguniquestrings to keep sort order consistent.Null padding can skew
localeCompareordering; stripping\0aligns with other decode paths and yields more intuitive ordering.♻️ Suggested tweak
- decodedData[dataIndex] = textDecoder.decode( - data?.slice?.(dataIndex * length, dataIndex * length + length), - ); + decodedData[dataIndex] = textDecoder + .decode(data?.slice?.(dataIndex * length, dataIndex * length + length)) + .replace(/\0/g, "");
| - `useEditCell` and `useSlickGridReact` uses refs (`orderedParamColumnsRef`, `sortedFilteredIndicesRef`) because SlickGrid stores callback references and we need to avoid stale closures. | ||
|
|
There was a problem hiding this comment.
Fix subject‑verb agreement in the refs section.
“useEditCell and useSlickGridReact uses refs” → “use … use refs”.
✏️ Suggested edit
- - `useEditCell` and `useSlickGridReact` uses refs (`orderedParamColumnsRef`, `sortedFilteredIndicesRef`) because SlickGrid stores callback references and we need to avoid stale closures.
+ - `useEditCell` and `useSlickGridReact` use refs (`orderedParamColumnsRef`, `sortedFilteredIndicesRef`) because SlickGrid stores callback references and we need to avoid stale closures.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `useEditCell` and `useSlickGridReact` uses refs (`orderedParamColumnsRef`, `sortedFilteredIndicesRef`) because SlickGrid stores callback references and we need to avoid stale closures. | |
| - `useEditCell` and `useSlickGridReact` use refs (`orderedParamColumnsRef`, `sortedFilteredIndicesRef`) because SlickGrid stores callback references and we need to avoid stale closures. |
🤖 Prompt for AI Agents
In `@docs/extradocs/table_react.md` around lines 51 - 52, Fix the subject-verb
agreement in the sentence mentioning refs: change "useEditCell and
useSlickGridReact uses refs" to "useEditCell and useSlickGridReact use refs"
(the sentence referencing orderedParamColumnsRef and sortedFilteredIndicesRef
should use the plural verb "use"). Ensure the updated phrasing still mentions
the reason (SlickGrid stores callback references and we need to avoid stale
closures).
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores
Documentation