Feature/gating #328
Feature/gating #328
Conversation
…grid row index and directly access the id instead of calling getItem in mapRowsToIds
…ht mode and fix find and replace dialog title
… back to table component and rename sortedIndices to sortedFilteredIndices
…internally and explicitly serialised.
…a to avoid unexpected side-effects
- 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
… manage gates, rename and delete gates
📝 WalkthroughWalkthroughAdds a React-based SlickGrid table chart with find/replace and inline editing, introduces gate management (create/rename/delete/export) and visualization layers, adds fixed-length "unique" string support across Python and JS, many new hooks/components/utilities, CSS for SlickGrid, and extensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableComp as TableChartReactComponent
participant SlickGrid
participant DataStore
participant GateMgr as GateManager
User->>TableComp: trigger Find/Replace, Edit, or Save Gate
TableComp->>SlickGrid: focus/select cell(s) / open dialog
SlickGrid->>TableComp: selection/edit events
TableComp->>DataStore: read indices / apply edits / request persistence
DataStore->>TableComp: confirm data persisted
TableComp->>GateMgr: add/update/delete gate (on save)
GateMgr->>DataStore: persist gates & rebuild gate column
GateMgr-->>TableComp: updated gates for layers/labels
TableComp->>SlickGrid: refresh / scroll to match results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@docs/extradocs/table_react.md`:
- Around line 90-97: The table currently mislabels numeric storage: update the
rows for "double" and "int32/integer" so that "double" and "integer" list
Float32Array as their storage and "int32" remains Int32Array; locate the markup
lines containing the Type values "double", "int32/integer", and "int32" in the
table (within table_react.md) and change the Storage column entries accordingly
to reflect Float32Array for double/integer and Int32Array for int32.
In `@package.json`:
- Line 112: Replace the PR-preview dependency for "slickgrid-react" that points
to the pkg.pr.new URL with a stable npm release; update the package.json
dependency entry for "slickgrid-react" to a released semver (for example
"9.13.0") so builds use the published package instead of the ephemeral PR
preview.
In `@src/charts/ChartManager.js`:
- Around line 1121-1132: The current unique-column branch in ChartManager.js
(when cl.datatype === "unique") returns { metadata: md, data: [] } if
cl.stringLength is invalid, which breaks row alignment; change this to produce a
placeholder array sized to the number of rows (use dataStore.size) so the column
length matches others (e.g., an array of empty strings or nulls) or
alternatively abort/save-fail explicitly; update the return in that branch to
return { metadata: md, data: placeholderArray } so callers using c and md see a
column with correct row count instead of zero-length.
In `@src/react/components/FindAndReplaceDialog.tsx`:
- Line 63: The computed title string in FindAndReplaceDialog (variable title)
inserts an extra space when isColumnEditable is false; change the template to
concatenate the segments without hardcoded spaces and only include leading
spaces inside the conditional parts — e.g., make "And Replace" prefixed with a
space when included and " in \"...\"" prefixed with a space when columnName
exists — so the resulting title uses Find + optional " And Replace" + optional "
in \"columnName\"" with no double spaces.
- Line 168: The Enter key handler on the Replace input currently calls
handleReplace unconditionally; modify the onKeyDown handler for the Replace
field so it only invokes handleReplace(findText, replaceText) when the same
validation used to enable the Replace button passes (e.g., replaceText is
non-empty/trimmed). Update the anonymous handler on the Replace input (the one
with onKeyDown) to check the replaceText condition before calling handleReplace
(and optionally preventDefault when invoking), matching the logic that controls
the Replace button disabled state.
In `@src/react/components/GateNameDialog.tsx`:
- Around line 11-35: The GateNameDialog can fall out of sync when reopened with
a different name; add a useEffect inside the GateNameDialog component that
watches the open and name props and calls setGateName(name ?? "") and
setError("") (or calls handleClose-like reset) whenever open becomes true or
name changes so the local gateName state is reset to the incoming prop;
reference the GateNameDialog component, gateName, setGateName, setError, open,
name and handleClose when implementing.
In `@src/react/components/ManageGateDialog.tsx`:
- Around line 169-193: The rename and delete dialogs aren't being closed after
the confirm handlers; update the onSaveGate handler passed to GateNameDialog to
call setRenameGateId(null) after invoking onRenameGate(renameGateId, gateName)
(use the existing renameGateId and setRenameGateId), and update the onConfirm
handler passed to ConfirmDialog to call setDeleteGateId(null) after invoking
onDelete(deleteGateId) (use deleteGateId and setDeleteGateId) so the dialogs are
dismissed once the actions complete.
In `@src/react/gates/GateManager.ts`:
- Around line 34-51: The addGate flow currently proceeds to clone/update cells
even if ensureGatesColumnDataLoaded failed and gateColumn.data may be undefined;
change ensureGatesColumnDataLoaded to surface failure (return boolean success or
throw) and update callers (addGate and rebuildGatesColumnWhenReady) to
early-return on failure: call ensureGatesColumnDataLoaded, check its result (or
catch the thrown error) and if it failed do not call updateCellsWithGate,
updateDataStoreWithGates, or this.dataStore.dataChanged([GATES_COLUMN_NAME]) —
similarly guard any code paths that access gateColumn or gateColumn.data so they
only run when gateColumn and gateColumn.data are defined.
- Around line 53-80: The updateGate implementation only adds the updated gate
and never removes the old gate membership, leaving stale tags; fix by ensuring
you remove prior memberships before adding the updated gate: when computing
updatedGate in updateGate, if cellUpdatedNeeded is true first call
updateCellsWithGate(original gate object retrieved as gate, false) to remove the
old name/geometry/column membership, then apply the mobx action and set
this.gates.set(gateId, updatedGate) (keep the geometry deep-clone logic), and
finally call updateCellsWithGate(updatedGate, true) to add new memberships;
ensure this sequence handles renames and geometry/column changes so stale tags
are cleared (references: updateGate, this.gates, updatedGate,
updateCellsWithGate).
In `@src/react/hooks/useGateActions.ts`:
- Around line 19-64: Trim and validate gate names in both onSaveGate and
onRenameGate: immediately compute const trimmedName = (gateName || "").trim(),
throw a validation error if trimmedName is empty, then use trimmedName when
calling gateManager.hasGateName(trimmedName) and when constructing the Gate
(replace name: gateName.trim() with name: trimmedName) or updating the gate via
gateManager.updateGate(gateId, { name: trimmedName }); ensure you use the
normalized trimmedName for all uniqueness checks and storage to prevent
duplicates caused by whitespace or empty input.
In `@src/react/hooks/useSlickGridReact.ts`:
- Around line 160-179: Uncomment and execute the initial sort logic inside the
handleGridCreated callback so the grid applies config.sort immediately when
created: after setting gridRef and before rendering, check if config.sort is
set, set suppressSortSyncRef.current = true, call
grid.setSortColumn(config.sort.columnId, config.sort.ascending), then set
suppressSortSyncRef.current = false; also add config.sort to the useCallback
dependency array for handleGridCreated so the callback updates when the sort
config changes (ensure you reference grid from e.detail.slickGrid and keep
existing dataProvider handling).
In `@src/react/hooks/useSortedFilteredIndices.ts`:
- Line 129: The effect's dependency array currently includes config.sort which
causes unnecessary re-runs; remove config.sort from the useEffect dependency
list so React doesn't dispose/recreate the MobX autorun when the sort object
reference changes. Keep filteredIndices and dataStore in the array, rely on the
autorun inside useSortedFilteredIndices (which already accesses config.sort) to
reactively track sort changes instead of listing config.sort as a dependency.
In `@src/react/utils/valueReplacementUtil.ts`:
- Around line 165-173: The numeric branch currently parses all numeric types
with Number.parseFloat, which allows decimals for int32/integer columns and can
corrupt Int32Array data; update the handling in valueReplacementUtil.ts inside
the block that checks datatype === "double" || "int32" || "integer" (and
references newValue, data, dataIndex, column.field) to: parse the value to a
number, and if datatype is "int32" or "integer" additionally verify
Number.isInteger(numValue) (or equivalent integer check) before assignment; if
the check fails, throw an Error like `Invalid integer value "<value>" for
${datatype} column: ${column.field}` so decimals are rejected rather than
truncated.
🧹 Nitpick comments (19)
src/charts/dialogs/ReusableDialog.tsx (2)
1-1: Unused import:DialogCloseIconButton.
DialogCloseIconButtonis imported but never used in the component. This appears to be a leftover from refactoring.♻️ Proposed fix
-import { DialogCloseIconButton } from "@/catalog/ProjectRenameModal"; import { Button, Container, Dialog, DialogActions, DialogContent, } from "@mui/material";
14-14: Unused prop:isAlertErrorComponent.The
isAlertErrorComponentprop is still defined in the interface and destructured in the component, but it no longer affects any behavior after removing the conditionalmaxWidthlogic.♻️ Proposed fix
export interface ReusableDialogProps { open: boolean; handleClose: () => void; component: JSX.Element; - isAlertErrorComponent?: boolean; isConfirmButton?: boolean; confirmText?: string; onConfirmClick?: () => void; } const ReusableDialog = ({ open, handleClose, component, - isAlertErrorComponent, isConfirmButton, confirmText, onConfirmClick, }: ReusableDialogProps) => {Also applies to: 24-24
src/charts/ChartManager.js (1)
1137-1146: Avoid per-row console errors during bounds checks.Line 1140 logs an error for every out-of-bounds row. For large datasets this can severely spam the console and slow the UI. Consider logging once and filling the remaining entries.
💡 Suggested adjustment
- for (let i = 0; i < numRows; i++) { + let warned = false; + for (let i = 0; i < numRows; i++) { const baseIndex = i * stringLength; if (!cl.data || baseIndex + stringLength > cl.data.length) { - console.error( - `Index out of bounds for column ${c} at row ${i}. Skipping.` - ); + if (!warned) { + console.error( + `Index out of bounds for column ${c}. Filling remaining rows with empty strings.` + ); + warned = true; + } arr[i] = ""; continue; }python/mdvtools/mdvproject.py (1)
653-659: Consider explicit length check for robustness.Line 655 uses
if raw_data and isinstance(raw_data[0], ...), which technically would fail with "The truth value of an array is ambiguous" if a NumPy array were passed. While the current code only receives Python lists from JSON deserialization, an explicit length check would be more defensive:- if raw_data and isinstance(raw_data[0], (int, float)): + if len(raw_data) > 0 and isinstance(raw_data[0], (int, float)): raise ValueError( f"Column {cid} of type 'unique' expects array of strings, " f"but received numeric data." )This improves clarity and guards against future code changes that might pass different data types to this method.
src/react/utils/SlickGridDataProvider.ts (1)
36-51: Unreachable code:if (!item) return nullwill never execute.The
itemobject is initialized as{}on line 36 and is never reassigned. An empty object is truthy, so the check on line 41 will never returnnull. This dead code can be safely removed.🧹 Proposed fix to remove unreachable code
const item: any = {}; for (const column of this.columns) { if (column) item[column.field] = column.getValue(dataIndex); } - if (!item) return null; // Add id for SlickGrid item.id = dataIndex;src/datastore/DataStore.js (1)
79-79: Consider defaultinggatesto an empty array for safer consumers.If gate consumers assume an array, initializing to
[]avoids repetitive null checks. Ifundefinedis semantically important, feel free to ignore.♻️ Optional tweak
- this.gates = config.gates; + this.gates = config.gates ?? [];src/react/components/ManageGateDialog.tsx (1)
114-118: Remove the stale TODO comment.
The onClick is already wired, so the TODO is now noise.src/react/components/TableChartReactComponent.tsx (1)
21-21: Track the integration-test TODO.
Consider filing a follow-up task so this doesn’t get lost. I can draft a test plan if helpful.src/tests/table_react/hooks/useSlickGridReact.test.ts (1)
158-175: Grid creation test validates initialization behavior.The test correctly verifies that
setDataandrenderare called when the grid is created. Consider adding assertions for the event subscription setup (e.g., verifying thatonSelectedRowsChangedhandlers are registered).src/react/components/FeedbackAlertComponent.tsx (2)
25-26: Consider returningnullinstead of empty fragment.Returning
nullis the idiomatic React pattern for rendering nothing and is slightly more efficient than an empty fragment.Suggested change
const FeedbackAlertComponent = ({ feedbackAlert }: FeedbackAlertComponentType) => { - if (!feedbackAlert) return <></>; + if (!feedbackAlert) return null;
40-48: Title fallback uses lowercase type value.When
feedbackAlert.titleis undefined, the fallbackfeedbackAlert.typewill be lowercase (e.g., "error", "warning"). Consider capitalizing for better UX.Capitalize the type fallback
return ( <AlertErrorComponent message={feedbackAlert.message} - title={feedbackAlert.title || feedbackAlert.type} + title={feedbackAlert.title || feedbackAlert.type.charAt(0).toUpperCase() + feedbackAlert.type.slice(1)} alertType={feedbackAlert.type} /> );src/react/components/SelectionOverlay.tsx (1)
191-199: Divider styling could use theme-aware colors.The Divider uses
color: "inherit"which is fine, but consider if the hard-coded dimensions (width: "5px",height: "35px") should be responsive or theme-based.src/tests/table_react/hooks/useFindReplace.test.ts (1)
373-376: Consider separating find and replace into distinctact()calls.While combining
handleFindandhandleReplacein oneact()block works, separating them would better reflect real user interaction flow and make debugging easier if tests fail.Suggested change
act(() => { result.current.handleFind("cells"); + }); + + act(() => { result.current.handleReplace("cells", "new"); });src/react/hooks/useSortedFilteredIndices.ts (2)
42-49: Index column sorting could use a comparator for consistency.While
indices.sort()followed byindices.reverse()works, using a comparator function would be more consistent with the other sorting paths and slightly more efficient.Use comparator for index sorting
// Handle index column if (columnId === "__index__") { - indices.sort(); - if (!ascending) { - indices.reverse(); - } + indices.sort((a, b) => ascending ? a - b : b - a); setSortedFilteredIndices(indices); return; }
79-88: Defensive bounds checking with graceful degradation.The error logging with continuation (using empty string) allows the UI to remain functional even with data issues. However, the optional chaining on
data?.slice?.()(line 86) is redundant sincedataexistence is already verified in the condition.Remove redundant optional chaining
decodedData[dataIndex] = textDecoder.decode( - data?.slice?.(dataIndex * length, dataIndex * length + length), + data.slice(dataIndex * length, dataIndex * length + length), );src/react/components/FindAndReplaceDialog.tsx (1)
118-123: Simplify match count display condition.The condition
foundMatches || typeof foundMatches === "number"handles zero, but can be simplified sincefoundMatchesis typed asnumber | null | undefined.Simplified condition
- {/* foundMatches || typeof foundMatches === "number" - This condition is for zero */} - {findText && (foundMatches || typeof foundMatches === "number") ? ( + {findText && foundMatches != null ? ( <Typography>{`Found ${foundMatches} matches`}</Typography> ) : ( - <></> + null )}src/react/hooks/useGateLayers.ts (1)
18-21: Avoid callinguseParamColumnstwice.
Read once and destructure to prevent duplicate work and keep a consistent snapshot.♻️ Proposed refactor
- const [cx, cy] = useParamColumns() as LoadedDataColumn<"double">[]; - const cz = useParamColumns()[2] as LoadedDataColumn<"double">; + const paramColumns = useParamColumns() as LoadedDataColumn<"double">[]; + const [cx, cy, cz] = paramColumns;src/react/gates/gateUtils.ts (1)
36-39: Use-Infinityfor max bounds.
Number.MIN_VALUEis a tiny positive number, which makes the bounding box less precise for negative coordinates.♻️ Suggested fix
- let maxX = Number.MIN_VALUE; - let maxY = Number.MIN_VALUE; + let maxX = Number.NEGATIVE_INFINITY; + let maxY = Number.NEGATIVE_INFINITY;src/react/gates/GateManager.ts (1)
223-262: Avoid recomputing polygon coords per cell.Line 233 precomputes coords, but
isPointInGatere-extracts them for every row. Consider precomputing once and using a helper that accepts coords to avoid repeated extraction in large datasets.
| | Type | Storage | Notes | | ||
| |------|---------|-------| | ||
| | text | Uint8Array index to values[] | Max 256 values | | ||
| | text16 | Uint16Array index to values[] | Max 65536 values | | ||
| | double | Float64Array | IEEE 754 | | ||
| | int32/integer | Int32Array | 32-bit signed | | ||
| | multitext | Uint16Array[len] to values[] | Multiple per cell | | ||
| | unique | Uint8Array[len] UTF-8 | Fixed-length string | |
There was a problem hiding this comment.
Fix numeric storage types in the data-type table.
Lines 94-96 list double as Float64Array and int32/integer as Int32Array. In MDV, double/integer are stored as Float32Array (see numeric handling in the data pipeline), while int32 is the only Int32Array. The table should reflect the actual storage to avoid misleading consumers.
📝 Suggested correction
-| double | Float64Array | IEEE 754 |
-| int32/integer | Int32Array | 32-bit signed |
+| double | Float32Array | IEEE 754 (float32) |
+| integer | Float32Array | 32-bit float storage |
+| int32 | Int32Array | 32-bit signed |🤖 Prompt for AI Agents
In `@docs/extradocs/table_react.md` around lines 90 - 97, The table currently
mislabels numeric storage: update the rows for "double" and "int32/integer" so
that "double" and "integer" list Float32Array as their storage and "int32"
remains Int32Array; locate the markup lines containing the Type values "double",
"int32/integer", and "int32" in the table (within table_react.md) and change the
Storage column entries accordingly to reflect Float32Array for double/integer
and Int32Array for int32.
| "rehype-sanitize": "^6.0.0", | ||
| "remark-breaks": "^4.0.0", | ||
| "remark-gfm": "^4.0.1", | ||
| "slickgrid-react": "https://pkg.pr.new/ghiscoding/slickgrid-universal/slickgrid-react@2228", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
slickgrid-react npm package latest stable version
💡 Result:
The latest stable slickgrid-react version on npm is 9.13.0. [1]
(For reference, npm’s package page has recently shown 9.8.0 in some cached results.) [2]
Sources: jsDelivr npm index for slickgrid-react (shows 9.13.0) [1]; npm package page snippet (shows 9.8.0 in cached crawl) [2].
Replace PR preview package with a stable release.
The slickgrid-react dependency uses a pkg.pr.new URL pointing to PR #2228 preview. Stable versions are available on npm (latest is 9.13.0). PR preview packages are ephemeral and can be removed at any time, breaking builds. Replace with a stable version from npm once the upstream PR is merged and released, or immediately if a suitable release is available.
🤖 Prompt for AI Agents
In `@package.json` at line 112, Replace the PR-preview dependency for
"slickgrid-react" that points to the pkg.pr.new URL with a stable npm release;
update the package.json dependency entry for "slickgrid-react" to a released
semver (for example "9.13.0") so builds use the published package instead of the
ephemeral PR preview.
| const [findText, setFindText] = useState(""); | ||
| const [replaceText, setReplaceText] = useState(""); | ||
|
|
||
| const title = `Find ${isColumnEditable ? "And Replace" : ""} ${columnName ? `in "${columnName}"` : ""}`; |
There was a problem hiding this comment.
Title template has trailing space when replace is disabled.
When isColumnEditable is false, the title becomes "Find in ..." with an extra space. Use template literals more carefully or handle the spacing conditionally.
Fix title spacing
- const title = `Find ${isColumnEditable ? "And Replace" : ""} ${columnName ? `in "${columnName}"` : ""}`;
+ const title = `Find${isColumnEditable ? " And Replace" : ""}${columnName ? ` in "${columnName}"` : ""}`;🤖 Prompt for AI Agents
In `@src/react/components/FindAndReplaceDialog.tsx` at line 63, The computed title
string in FindAndReplaceDialog (variable title) inserts an extra space when
isColumnEditable is false; change the template to concatenate the segments
without hardcoded spaces and only include leading spaces inside the conditional
parts — e.g., make "And Replace" prefixed with a space when included and " in
\"...\"" prefixed with a space when columnName exists — so the resulting title
uses Find + optional " And Replace" + optional " in \"columnName\"" with no
double spaces.
| size="small" | ||
| value={replaceText} | ||
| onChange={(e) => setReplaceText(e.target.value)} | ||
| onKeyDown={(e) => e.key === "Enter" && handleReplace(findText, replaceText)} |
There was a problem hiding this comment.
Enter key on Replace field bypasses disabled state check.
The Replace button is disabled when replaceText is empty (line 177), but pressing Enter on the Replace field (line 168) will call handleReplace even with empty replaceText. Add the same validation.
Add validation for Enter key handler
onChange={(e) => setReplaceText(e.target.value)}
- onKeyDown={(e) => e.key === "Enter" && handleReplace(findText, replaceText)}
+ onKeyDown={(e) => e.key === "Enter" && replaceText.trim() && handleReplace(findText, replaceText)}
sx={{ flexGrow: 1 }}📝 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.
| onKeyDown={(e) => e.key === "Enter" && handleReplace(findText, replaceText)} | |
| onChange={(e) => setReplaceText(e.target.value)} | |
| onKeyDown={(e) => e.key === "Enter" && replaceText.trim() && handleReplace(findText, replaceText)} | |
| sx={{ flexGrow: 1 }} |
🤖 Prompt for AI Agents
In `@src/react/components/FindAndReplaceDialog.tsx` at line 168, The Enter key
handler on the Replace input currently calls handleReplace unconditionally;
modify the onKeyDown handler for the Replace field so it only invokes
handleReplace(findText, replaceText) when the same validation used to enable the
Replace button passes (e.g., replaceText is non-empty/trimmed). Update the
anonymous handler on the Replace input (the one with onKeyDown) to check the
replaceText condition before calling handleReplace (and optionally
preventDefault when invoking), matching the logic that controls the Replace
button disabled state.
| updateGate(gateId: string, updates: Partial<Gate>) { | ||
| const gate = this.gates.get(gateId); | ||
| if (!gate) { | ||
| console.error(`Gate ${gateId} not found`); | ||
| return; | ||
| } | ||
|
|
||
| const updatedGate = { | ||
| ...gate, | ||
| ...updates, | ||
| geometry: updates.geometry ? JSON.parse(JSON.stringify(updates.geometry)) : gate.geometry, | ||
| }; | ||
|
|
||
| action(() => { | ||
| this.gates.set(gateId, updatedGate); | ||
| })(); | ||
|
|
||
| const cellUpdatedNeeded = "geometry" in updates || "name" in updates || "columns" in updates; | ||
|
|
||
| if (cellUpdatedNeeded) { | ||
| this.updateCellsWithGate(updatedGate, true); | ||
| } | ||
|
|
||
| this.updateDataStoreWithGates(); | ||
|
|
||
| if (this.gateColumn) { | ||
| this.dataStore.dataChanged([GATES_COLUMN_NAME]); | ||
| } |
There was a problem hiding this comment.
Update path leaves stale gate membership on rename/geometry/column changes.
Line 70 only adds the updated gate; it never removes the prior gate name or old geometry membership. Renames and geometry/column changes will leave stale gate tags in cells.
🐛 Proposed fix
const gate = this.gates.get(gateId);
if (!gate) {
console.error(`Gate ${gateId} not found`);
return;
}
+ const previousGate = gate;
const updatedGate = {
...gate,
...updates,
geometry: updates.geometry ? JSON.parse(JSON.stringify(updates.geometry)) : gate.geometry,
};
@@
const cellUpdatedNeeded = "geometry" in updates || "name" in updates || "columns" in updates;
if (cellUpdatedNeeded) {
- this.updateCellsWithGate(updatedGate, true);
+ this.updateCellsWithGate(previousGate, false);
+ this.updateCellsWithGate(updatedGate, true);
}📝 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.
| updateGate(gateId: string, updates: Partial<Gate>) { | |
| const gate = this.gates.get(gateId); | |
| if (!gate) { | |
| console.error(`Gate ${gateId} not found`); | |
| return; | |
| } | |
| const updatedGate = { | |
| ...gate, | |
| ...updates, | |
| geometry: updates.geometry ? JSON.parse(JSON.stringify(updates.geometry)) : gate.geometry, | |
| }; | |
| action(() => { | |
| this.gates.set(gateId, updatedGate); | |
| })(); | |
| const cellUpdatedNeeded = "geometry" in updates || "name" in updates || "columns" in updates; | |
| if (cellUpdatedNeeded) { | |
| this.updateCellsWithGate(updatedGate, true); | |
| } | |
| this.updateDataStoreWithGates(); | |
| if (this.gateColumn) { | |
| this.dataStore.dataChanged([GATES_COLUMN_NAME]); | |
| } | |
| updateGate(gateId: string, updates: Partial<Gate>) { | |
| const gate = this.gates.get(gateId); | |
| if (!gate) { | |
| console.error(`Gate ${gateId} not found`); | |
| return; | |
| } | |
| const previousGate = gate; | |
| const updatedGate = { | |
| ...gate, | |
| ...updates, | |
| geometry: updates.geometry ? JSON.parse(JSON.stringify(updates.geometry)) : gate.geometry, | |
| }; | |
| action(() => { | |
| this.gates.set(gateId, updatedGate); | |
| })(); | |
| const cellUpdatedNeeded = "geometry" in updates || "name" in updates || "columns" in updates; | |
| if (cellUpdatedNeeded) { | |
| this.updateCellsWithGate(previousGate, false); | |
| this.updateCellsWithGate(updatedGate, true); | |
| } | |
| this.updateDataStoreWithGates(); | |
| if (this.gateColumn) { | |
| this.dataStore.dataChanged([GATES_COLUMN_NAME]); | |
| } |
🤖 Prompt for AI Agents
In `@src/react/gates/GateManager.ts` around lines 53 - 80, The updateGate
implementation only adds the updated gate and never removes the old gate
membership, leaving stale tags; fix by ensuring you remove prior memberships
before adding the updated gate: when computing updatedGate in updateGate, if
cellUpdatedNeeded is true first call updateCellsWithGate(original gate object
retrieved as gate, false) to remove the old name/geometry/column membership,
then apply the mobx action and set this.gates.set(gateId, updatedGate) (keep the
geometry deep-clone logic), and finally call updateCellsWithGate(updatedGate,
true) to add new memberships; ensure this sequence handles renames and
geometry/column changes so stale tags are cleared (references: updateGate,
this.gates, updatedGate, updateCellsWithGate).
| const onSaveGate = useCallback( | ||
| (gateName: string) => { | ||
| if (gateManager.hasGateName(gateName)) { | ||
| throw new Error("A gate with this name already exists"); | ||
| } | ||
|
|
||
| // Get X and Y columns | ||
| const [xCol, yCol] = paramColumns.slice(0, 2); | ||
| if (!xCol || !yCol) { | ||
| throw new Error("Chart must have X and Y axes"); | ||
| } | ||
|
|
||
| // Create gate | ||
| const gate: Gate = { | ||
| id: generateGateId(), | ||
| name: gateName.trim(), | ||
| geometry: selectionFeatureCollection, | ||
| columns: [xCol.field, yCol.field], | ||
| createdAt: Date.now(), | ||
| }; | ||
|
|
||
| // Add to gate store | ||
| gateManager.addGate(gate); | ||
|
|
||
| // Clear selection | ||
| action(() => { | ||
| chart.config.selectionFeatureCollection = getEmptyFeatureCollection(); | ||
| })(); | ||
| }, | ||
| [gateManager, paramColumns, selectionFeatureCollection, chart.config], | ||
| ); | ||
|
|
||
| const onDeleteGate = useCallback( | ||
| (gateId: string) => { | ||
| gateManager.deleteGate(gateId); | ||
| }, | ||
| [gateManager], | ||
| ); | ||
|
|
||
| const onRenameGate = useCallback( | ||
| (gateId: string, newName: string) => { | ||
| if (gateManager.hasGateName(newName)) { | ||
| throw new Error("A gate with this name already exists"); | ||
| } | ||
| gateManager.updateGate(gateId, { name: newName }); | ||
| }, |
There was a problem hiding this comment.
Normalize and validate gate names before uniqueness checks.
Trim and guard empty names first to prevent duplicates via whitespace and empty entries.
🛠️ Suggested fix
const onSaveGate = useCallback(
(gateName: string) => {
- if (gateManager.hasGateName(gateName)) {
+ const normalizedName = gateName.trim();
+ if (!normalizedName) {
+ throw new Error("Gate name cannot be empty");
+ }
+ if (gateManager.hasGateName(normalizedName)) {
throw new Error("A gate with this name already exists");
}
@@
const gate: Gate = {
id: generateGateId(),
- name: gateName.trim(),
+ name: normalizedName,
geometry: selectionFeatureCollection,
columns: [xCol.field, yCol.field],
createdAt: Date.now(),
};
@@
const onRenameGate = useCallback(
(gateId: string, newName: string) => {
- if (gateManager.hasGateName(newName)) {
+ const normalizedName = newName.trim();
+ if (!normalizedName) {
+ throw new Error("Gate name cannot be empty");
+ }
+ if (gateManager.hasGateName(normalizedName)) {
throw new Error("A gate with this name already exists");
}
- gateManager.updateGate(gateId, { name: newName });
+ gateManager.updateGate(gateId, { name: normalizedName });
},
[gateManager],
);📝 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.
| const onSaveGate = useCallback( | |
| (gateName: string) => { | |
| if (gateManager.hasGateName(gateName)) { | |
| throw new Error("A gate with this name already exists"); | |
| } | |
| // Get X and Y columns | |
| const [xCol, yCol] = paramColumns.slice(0, 2); | |
| if (!xCol || !yCol) { | |
| throw new Error("Chart must have X and Y axes"); | |
| } | |
| // Create gate | |
| const gate: Gate = { | |
| id: generateGateId(), | |
| name: gateName.trim(), | |
| geometry: selectionFeatureCollection, | |
| columns: [xCol.field, yCol.field], | |
| createdAt: Date.now(), | |
| }; | |
| // Add to gate store | |
| gateManager.addGate(gate); | |
| // Clear selection | |
| action(() => { | |
| chart.config.selectionFeatureCollection = getEmptyFeatureCollection(); | |
| })(); | |
| }, | |
| [gateManager, paramColumns, selectionFeatureCollection, chart.config], | |
| ); | |
| const onDeleteGate = useCallback( | |
| (gateId: string) => { | |
| gateManager.deleteGate(gateId); | |
| }, | |
| [gateManager], | |
| ); | |
| const onRenameGate = useCallback( | |
| (gateId: string, newName: string) => { | |
| if (gateManager.hasGateName(newName)) { | |
| throw new Error("A gate with this name already exists"); | |
| } | |
| gateManager.updateGate(gateId, { name: newName }); | |
| }, | |
| const onSaveGate = useCallback( | |
| (gateName: string) => { | |
| const normalizedName = gateName.trim(); | |
| if (!normalizedName) { | |
| throw new Error("Gate name cannot be empty"); | |
| } | |
| if (gateManager.hasGateName(normalizedName)) { | |
| throw new Error("A gate with this name already exists"); | |
| } | |
| // Get X and Y columns | |
| const [xCol, yCol] = paramColumns.slice(0, 2); | |
| if (!xCol || !yCol) { | |
| throw new Error("Chart must have X and Y axes"); | |
| } | |
| // Create gate | |
| const gate: Gate = { | |
| id: generateGateId(), | |
| name: normalizedName, | |
| geometry: selectionFeatureCollection, | |
| columns: [xCol.field, yCol.field], | |
| createdAt: Date.now(), | |
| }; | |
| // Add to gate store | |
| gateManager.addGate(gate); | |
| // Clear selection | |
| action(() => { | |
| chart.config.selectionFeatureCollection = getEmptyFeatureCollection(); | |
| })(); | |
| }, | |
| [gateManager, paramColumns, selectionFeatureCollection, chart.config], | |
| ); | |
| const onDeleteGate = useCallback( | |
| (gateId: string) => { | |
| gateManager.deleteGate(gateId); | |
| }, | |
| [gateManager], | |
| ); | |
| const onRenameGate = useCallback( | |
| (gateId: string, newName: string) => { | |
| const normalizedName = newName.trim(); | |
| if (!normalizedName) { | |
| throw new Error("Gate name cannot be empty"); | |
| } | |
| if (gateManager.hasGateName(normalizedName)) { | |
| throw new Error("A gate with this name already exists"); | |
| } | |
| gateManager.updateGate(gateId, { name: normalizedName }); | |
| }, |
🤖 Prompt for AI Agents
In `@src/react/hooks/useGateActions.ts` around lines 19 - 64, Trim and validate
gate names in both onSaveGate and onRenameGate: immediately compute const
trimmedName = (gateName || "").trim(), throw a validation error if trimmedName
is empty, then use trimmedName when calling gateManager.hasGateName(trimmedName)
and when constructing the Gate (replace name: gateName.trim() with name:
trimmedName) or updating the gate via gateManager.updateGate(gateId, { name:
trimmedName }); ensure you use the normalized trimmedName for all uniqueness
checks and storage to prevent duplicates caused by whitespace or empty input.
| const handleGridCreated = useCallback( | ||
| (e: CustomEvent<SlickgridReactInstance>) => { | ||
| console.log("Grid created"); | ||
| gridRef.current = e.detail; | ||
|
|
||
| // Store gridRef in chart instance for getConfig() access | ||
| (chart as any).gridRef = gridRef; | ||
|
|
||
| const grid = e.detail.slickGrid; | ||
| if (grid && dataProvider) { | ||
| grid.setData(dataProvider, true); | ||
| 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; | ||
| // } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/react/hooks/useSlickGridReact.ts | sed -n '160,190p'Repository: Taylor-CCB-Group/MDV
Length of output: 1467
🏁 Script executed:
cat -n src/react/hooks/useSlickGridReact.ts | sed -n '310,360p'Repository: Taylor-CCB-Group/MDV
Length of output: 2126
🏁 Script executed:
cat -n src/react/hooks/useSlickGridReact.ts | sed -n '1,80p'Repository: Taylor-CCB-Group/MDV
Length of output: 4952
🏁 Script executed:
rg "gridRef" src/react/hooks/useSlickGridReact.ts -A 2 -B 2Repository: Taylor-CCB-Group/MDV
Length of output: 1431
🏁 Script executed:
cat -n src/react/hooks/useSlickGridReact.ts | sed -n '200,250p'Repository: Taylor-CCB-Group/MDV
Length of output: 3101
🏁 Script executed:
rg "handleGridCreated" src/react/hooks/useSlickGridReact.ts -B 5 -A 10Repository: Taylor-CCB-Group/MDV
Length of output: 949
Uncomment and apply initial sort on grid creation; add config.sort to handleGridCreated dependencies.
The sync effect (line 320) depends only on [config.sort] and exits early if gridRef is null. If this effect runs before the grid is created, it will never re-evaluate when gridRef is later set, leaving initial sort unapplied. Uncomment the sort code in handleGridCreated to apply it immediately upon grid creation, and add config.sort to the callback's dependency array so the grid updates when the sort config changes.
🐛 Suggested fix
grid.setData(dataProvider, true);
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;
- // }
+ if (config.sort) {
+ suppressSortSyncRef.current = true;
+ grid.setSortColumn(config.sort.columnId, config.sort.ascending);
+ suppressSortSyncRef.current = false;
+ }
attachEventHandlers(e.detail);
}
},
- [dataProvider, chart],
+ [dataProvider, chart, config.sort],
);🤖 Prompt for AI Agents
In `@src/react/hooks/useSlickGridReact.ts` around lines 160 - 179, Uncomment and
execute the initial sort logic inside the handleGridCreated callback so the grid
applies config.sort immediately when created: after setting gridRef and before
rendering, check if config.sort is set, set suppressSortSyncRef.current = true,
call grid.setSortColumn(config.sort.columnId, config.sort.ascending), then set
suppressSortSyncRef.current = false; also add config.sort to the useCallback
dependency array for handleGridCreated so the callback updates when the sort
config changes (ensure you reference grid from e.detail.slickGrid and keep
existing dataProvider handling).
| return; | ||
| }); | ||
| return () => disposer(); | ||
| }, [filteredIndices, dataStore, config.sort]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/react/hooks/useSortedFilteredIndices.ts | head -150Repository: Taylor-CCB-Group/MDV
Length of output: 6184
🏁 Script executed:
rg -n "config\.sort" -A2 -B2 src/react/Repository: Taylor-CCB-Group/MDV
Length of output: 8186
🏁 Script executed:
rg -n "sort:" -A2 src/react/components/TableChartReactWrapper.tsxRepository: Taylor-CCB-Group/MDV
Length of output: 46
🏁 Script executed:
rg -n "useConfig" -A5 src/react/hooks/ | head -100Repository: Taylor-CCB-Group/MDV
Length of output: 3550
🏁 Script executed:
fd useConfig src/react/ -x cat {}Repository: Taylor-CCB-Group/MDV
Length of output: 46
🏁 Script executed:
rg -n "export.*useConfig|function useConfig|const useConfig" src/react/Repository: Taylor-CCB-Group/MDV
Length of output: 118
🏁 Script executed:
rg -n "useConfig" src/react/hooks/index.ts -A10Repository: Taylor-CCB-Group/MDV
Length of output: 128
🏁 Script executed:
cat -n src/react/hooks.ts | head -50Repository: Taylor-CCB-Group/MDV
Length of output: 2434
🏁 Script executed:
rg -n "config.*=.*observable|makeObservable|observable" src/react/components/TableChartReactWrapper.tsx -B3 -A3Repository: Taylor-CCB-Group/MDV
Length of output: 1072
🏁 Script executed:
rg -n "class.*Config|interface TableChartReactConfig" src/react/components/TableChartReactWrapper.tsx -A10Repository: Taylor-CCB-Group/MDV
Length of output: 702
🏁 Script executed:
cat -n src/react/components/TableChartReactWrapper.tsx | sed -n '1,60p'Repository: Taylor-CCB-Group/MDV
Length of output: 2936
🏁 Script executed:
rg -n "makeAutoObservable|makeObservable" src/charts/BaseChart.ts -B2 -A2Repository: Taylor-CCB-Group/MDV
Length of output: 671
🏁 Script executed:
rg -n "makeAutoObservable|makeObservable" src/react/components/BaseReactChart.tsx -B3 -A3Repository: Taylor-CCB-Group/MDV
Length of output: 779
🏁 Script executed:
cat -n src/react/components/BaseReactChart.tsx | head -100Repository: Taylor-CCB-Group/MDV
Length of output: 5639
🏁 Script executed:
rg -n "constructor.*BaseChart|this\.config\s*=" src/charts/BaseChart.ts | head -30Repository: Taylor-CCB-Group/MDV
Length of output: 191
🏁 Script executed:
rg -n "function initialiseChartConfig|export.*initialiseChartConfig" src/charts/BaseChart.ts -A20Repository: Taylor-CCB-Group/MDV
Length of output: 46
🏁 Script executed:
rg -n "initialiseChartConfig" src/ -B2 -A15 | head -80Repository: Taylor-CCB-Group/MDV
Length of output: 5493
🏁 Script executed:
rg -n "export.*initialiseChartConfig|function initialiseChartConfig" src/charts/ -A30Repository: Taylor-CCB-Group/MDV
Length of output: 2552
🏁 Script executed:
rg -n "useMobx|observer\|makeAutoObservable" src/charts/BaseChart.ts -B3 -A3 | head -50Repository: Taylor-CCB-Group/MDV
Length of output: 46
🏁 Script executed:
cat -n src/charts/BaseChart.ts | sed -n '95,130p'Repository: Taylor-CCB-Group/MDV
Length of output: 2175
🏁 Script executed:
rg -n "makeAutoObservable|makeObservable" src/charts/BaseChart.ts -B5 -A5Repository: Taylor-CCB-Group/MDV
Length of output: 1268
The dependency array entry for config.sort creates redundant tracking and causes unnecessary re-runs.
The autorun() at line 29 already reactively tracks config.sort when accessed at line 33. Including config.sort in the dependency array causes React to re-run the entire useEffect each time config.sort changes to a new object reference (as happens in useSlickGridReact.ts), unnecessarily disposing and recreating the autorun. Since MobX automatically handles the reactivity, remove config.sort from the dependency array and let only the autorun manage the updates.
🤖 Prompt for AI Agents
In `@src/react/hooks/useSortedFilteredIndices.ts` at line 129, The effect's
dependency array currently includes config.sort which causes unnecessary
re-runs; remove config.sort from the useEffect dependency list so React doesn't
dispose/recreate the MobX autorun when the sort object reference changes. Keep
filteredIndices and dataStore in the array, rely on the autorun inside
useSortedFilteredIndices (which already accesses config.sort) to reactively
track sort changes instead of listing config.sort as a dependency.
| // 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}`); | ||
| } | ||
|
|
||
| data[dataIndex] = numValue; | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the valueReplacementUtil.ts file
find . -name "valueReplacementUtil.ts" -type fRepository: Taylor-CCB-Group/MDV
Length of output: 105
🏁 Script executed:
# Read the valueReplacementUtil.ts file to see the actual code
cat -n ./src/react/utils/valueReplacementUtil.ts | head -200 | tail -100Repository: Taylor-CCB-Group/MDV
Length of output: 3980
🏁 Script executed:
# Let's check the beginning of the file to understand the data structure and types
cat -n ./src/react/utils/valueReplacementUtil.ts | head -50Repository: Taylor-CCB-Group/MDV
Length of output: 2203
🏁 Script executed:
# Let's check the type definition of LoadedDataColumn and DataType to understand the data structure
grep -r "LoadedDataColumn" --include="*.ts" --include="*.tsx" | head -10Repository: Taylor-CCB-Group/MDV
Length of output: 1201
🏁 Script executed:
# Let's check the charts.d.ts file for DataType definitions
grep -A 5 -B 5 "type DataType" ./src/charts/charts.d.ts | head -50Repository: Taylor-CCB-Group/MDV
Length of output: 336
🏁 Script executed:
# Let's get the full DataType definition
grep -A 10 "export type DataType" ./src/charts/charts.d.tsRepository: Taylor-CCB-Group/MDV
Length of output: 334
🏁 Script executed:
# Let's check if there are any tests for this function to understand the expected behavior
find . -name "*.test.*" -o -name "*.spec.*" | xargs grep -l "setCellValueFromString" 2>/dev/nullRepository: Taylor-CCB-Group/MDV
Length of output: 124
🏁 Script executed:
# Let's examine the test file to understand the expected behavior
cat -n ./src/tests/table_react/utils/valueReplacementUtils.test.tsxRepository: Taylor-CCB-Group/MDV
Length of output: 12165
🏁 Script executed:
# Let's check what the data array type is for numeric columns
grep -B 20 "data.*:" ./src/charts/charts.d.ts | grep -A 5 "interface DataColumn"Repository: Taylor-CCB-Group/MDV
Length of output: 46
🏁 Script executed:
# Let's find and read the charts.d.ts file more thoroughly
cat -n ./src/charts/charts.d.ts | head -100Repository: Taylor-CCB-Group/MDV
Length of output: 5018
🏁 Script executed:
# Let's verify the behavior of TypedArrays with decimal values
python3 << 'EOF'
# Simulate what happens when you assign a decimal to an Int32Array
# JavaScript behavior: Int32Array truncates decimals
print("Testing TypedArray behavior with decimal values:")
print("Int32Array([3.7]) would store: 3 (truncated)")
print("Uint32Array([5.9]) would store: 5 (truncated)")
print("Float32Array([2.5]) would store: 2.5 (preserved)")
EOFRepository: Taylor-CCB-Group/MDV
Length of output: 252
🏁 Script executed:
# Check if there are any other usages of setCellValueFromString to understand impact
rg "setCellValueFromString" --type ts --type tsx -B 2 -A 2Repository: Taylor-CCB-Group/MDV
Length of output: 91
🏁 Script executed:
# Check usages of setCellValueFromString
rg "setCellValueFromString" -B 2 -A 2Repository: Taylor-CCB-Group/MDV
Length of output: 9525
🏁 Script executed:
# Check useEditCell.ts to see if there are UI-level input restrictions
cat -n ./src/react/hooks/useEditCell.ts | grep -A 30 "setCellValueFromString"Repository: Taylor-CCB-Group/MDV
Length of output: 3582
🏁 Script executed:
# Look for grid configuration or input validation related to numeric types
rg "int32|integer" -A 3 -B 3 | head -60Repository: Taylor-CCB-Group/MDV
Length of output: 50376
🏁 Script executed:
# Search for grid column configuration or editor configuration that might validate input
rg "editable|editor|validator" --max-count=20Repository: Taylor-CCB-Group/MDV
Length of output: 45872
🏁 Script executed:
# Check if there are existing tests for int32 or integer decimal input
cat -n ./src/tests/table_react/utils/valueReplacementUtils.test.tsx | grep -A 5 "int32\|integer"Repository: Taylor-CCB-Group/MDV
Length of output: 46
Add integer validation for int32/integer columns.
parseFloat accepts decimals, which silently truncate when stored in Int32Array/Uint32Array, corrupting data without user feedback. Validate that input is an integer before assignment.
🐛 Suggested fix
// 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}`);
}
+ if ((datatype === "int32" || datatype === "integer") && !Number.isInteger(numValue)) {
+ throw new Error(`Invalid integer value "${newValue}" for ${datatype} column: ${column.field}`);
+ }
data[dataIndex] = numValue;
return; 📝 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.
| // 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}`); | |
| } | |
| data[dataIndex] = numValue; | |
| return; | |
| // 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}`); | |
| } | |
| if ((datatype === "int32" || datatype === "integer") && !Number.isInteger(numValue)) { | |
| throw new Error(`Invalid integer value "${newValue}" for ${datatype} column: ${column.field}`); | |
| } | |
| data[dataIndex] = numValue; | |
| return; |
🤖 Prompt for AI Agents
In `@src/react/utils/valueReplacementUtil.ts` around lines 165 - 173, The numeric
branch currently parses all numeric types with Number.parseFloat, which allows
decimals for int32/integer columns and can corrupt Int32Array data; update the
handling in valueReplacementUtil.ts inside the block that checks datatype ===
"double" || "int32" || "integer" (and references newValue, data, dataIndex,
column.field) to: parse the value to a number, and if datatype is "int32" or
"integer" additionally verify Number.isInteger(numValue) (or equivalent integer
check) before assignment; if the check fails, throw an Error like `Invalid
integer value "<value>" for ${datatype} column: ${column.field}` so decimals are
rejected rather than truncated.
…ring length is invalid
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/react/components/ManageGateDialog.tsx`:
- Around line 73-74: The ManageGateDialog currently renders <Dialog open={open}
onClose={onClose} ...> with <DialogContent> but no <DialogTitle>, causing a
missing visible and accessible heading; add a <DialogTitle> inside the <Dialog>
(directly before <DialogContent>) that uses an appropriate title string (e.g.,
the gate name or a static "Manage Gate") and ensure it reads from available
props/state in ManageGateDialog so screen readers and the visual layout
(divider) are correct.
- Line 114: Remove the stale JSX comment "{/* todo: add this onClick*/}" in
ManageGateDialog.tsx: the onClick is already wired via the onRenameClick handler
on the rename button, so delete that comment to avoid confusion and keep the
component code clean (look for the rename button that calls onRenameClick).
In `@src/react/hooks/useGateLayers.ts`:
- Around line 35-46: The effect on gates currently recreates and replaces the
entire labelPositions Map, wiping out any user-dragged positions; change the
logic in the useEffect that calls setLabelPositions so it merges into the
existing Map instead of replacing it: use the functional updater form
setLabelPositions(prev => { const next = new Map(prev); for each gate in gates,
if gate.labelPosition then next.set(gate.id, gate.labelPosition) else if
!next.has(gate.id) computeCentroid(gate.geometry) and next.set(gate.id,
centroid); also remove entries from next for gate IDs no longer present in gates
to avoid stale keys; reference the useEffect that iterates gates,
computeCentroid, and setLabelPositions when implementing this merge behavior.
- Around line 13-16: truncateGateLabel currently slices name with name.slice(0,
maxLen - 1) then appends "..." which can exceed MAX_LABEL_LENGTH; update the
logic in truncateGateLabel to slice to maxLen - 3 before appending the ellipsis
so the final string length ≤ maxLen, and also guard for tiny maxLen values
(e.g., if maxLen <= 3, return name.slice(0, maxLen) without adding "...") to
avoid negative slice indices.
🧹 Nitpick comments (5)
src/react/components/ManageGateDialog.tsx (1)
107-140: Consider an empty-state message when there are no gates.If
gatesArrayis empty, the table renders headers with an empty body, which may confuse users. A short message like "No gates created yet" would improve the UX.src/react/hooks/useGateLayers.ts (3)
20-21:useParamColumns()is called twice, creating redundant work.Both calls execute the same hook independently. Call it once and destructure:
Proposed fix
- const [cx, cy] = useParamColumns() as LoadedDataColumn<"double">[]; - const cz = useParamColumns()[2] as LoadedDataColumn<"double">; + const paramColumns = useParamColumns(); + const [cx, cy] = paramColumns as LoadedDataColumn<"double">[]; + const cz = paramColumns[2] as LoadedDataColumn<"double"> | undefined;
55-137: Layer is fully recomputed on every drag frame due todragPosdependency.
dragPosis updated viasetDragPoson everyonDragevent (Line 114), which triggers theuseMemo(Line 137) to rebuild theTextLayereach frame. While deck.gl diffs layer instances efficiently, theuseMemobody — includingfilter,map,computeCentroid, and object allocations — runs on every pointer-move during a drag.Consider removing
dragPosfrom the dependency array (it's not read directly in the layer data construction —labelPositionsalready captures position updates) or debouncing the drag state updates. IfdragPosis only used inside theonDragcallback's guard check (Line 104), you could replace it with a ref to avoid triggering re-renders.
88-94:updateTriggersmay miss content changes when gate count stays the same.
gates.lengthandlabelPositions.sizewon't change when a gate is renamed or its geometry is edited without adding/removing gates. Consider using a more granular trigger (e.g., thegatesarray reference itself, or a derived hash/version).src/react/components/VivScatterComponent.tsx (1)
215-226:getCursor: theisHoveringbranch is redundant — both it and the default return"grab".If the intent is to show a different cursor when hovering a gate label (vs. the map background),
isHoveringLabelfrom the hook should also be checked here. Otherwise, simplify.Possible simplification or intended distinction
const getCursor = useCallback(({isDragging, isHovering}: {isDragging: boolean, isHovering: boolean}) => { if (draggingId) return "grabbing"; - if (isDragging) - return "grabbing" - - if (isHovering) - return "grab"; - - return "grab"; + return "grabbing"; + return isHovering ? "pointer" : "grab"; }, [draggingId]);If
isHoveringshould show"grab"(same as default), remove the branch entirely.
| <Dialog open={open} onClose={onClose} fullWidth maxWidth="sm"> | ||
| <DialogContent dividers> |
There was a problem hiding this comment.
Missing DialogTitle — dialog opens without a visible heading.
The Dialog uses DialogContent with dividers (which renders a top border expecting a title above) but has no DialogTitle. This leaves the dialog without an accessible heading, which is both a visual gap and an a11y concern (screen readers won't announce a dialog title).
Suggested fix
<Dialog open={open} onClose={onClose} fullWidth maxWidth="sm">
+ <DialogTitle>Manage Gates</DialogTitle>
<DialogContent dividers>📝 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.
| <Dialog open={open} onClose={onClose} fullWidth maxWidth="sm"> | |
| <DialogContent dividers> | |
| <Dialog open={open} onClose={onClose} fullWidth maxWidth="sm"> | |
| <DialogTitle>Manage Gates</DialogTitle> | |
| <DialogContent dividers> |
🤖 Prompt for AI Agents
In `@src/react/components/ManageGateDialog.tsx` around lines 73 - 74, The
ManageGateDialog currently renders <Dialog open={open} onClose={onClose} ...>
with <DialogContent> but no <DialogTitle>, causing a missing visible and
accessible heading; add a <DialogTitle> inside the <Dialog> (directly before
<DialogContent>) that uses an appropriate title string (e.g., the gate name or a
static "Manage Gate") and ensure it reads from available props/state in
ManageGateDialog so screen readers and the visual layout (divider) are correct.
| <Typography variant="subtitle1">{gate.name}</Typography> | ||
| </TableCell> | ||
| <TableCell align="right" sx={{ width: 48, minWidth: 48 }}> | ||
| {/* todo: add this onClick*/} |
There was a problem hiding this comment.
Stale TODO comment — the onClick handler is already wired.
Line 114's {/* todo: add this onClick*/} is outdated since onClick is already provided on line 117 via onRenameClick. Please remove it to avoid confusion.
Suggested fix
<TableCell align="right" sx={{ width: 48, minWidth: 48 }}>
- {/* todo: add this onClick*/}
<IconWithTooltip📝 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.
| {/* todo: add this onClick*/} | |
| <TableCell align="right" sx={{ width: 48, minWidth: 48 }}> | |
| <IconWithTooltip |
🤖 Prompt for AI Agents
In `@src/react/components/ManageGateDialog.tsx` at line 114, Remove the stale JSX
comment "{/* todo: add this onClick*/}" in ManageGateDialog.tsx: the onClick is
already wired via the onRenameClick handler on the rename button, so delete that
comment to avoid confusion and keep the component code clean (look for the
rename button that calls onRenameClick).
| function truncateGateLabel(name: string, maxLen: number = MAX_LABEL_LENGTH) { | ||
| if (name.length <= maxLen) return name; | ||
| return `${name.slice(0, maxLen - 1)}...`; | ||
| } |
There was a problem hiding this comment.
truncateGateLabel produces strings longer than maxLen.
name.slice(0, maxLen - 1) yields 17 chars, then appending "..." gives 20 — exceeding the intended 18-char limit. Slice to maxLen - 3 instead.
Proposed fix
function truncateGateLabel(name: string, maxLen: number = MAX_LABEL_LENGTH) {
if (name.length <= maxLen) return name;
- return `${name.slice(0, maxLen - 1)}...`;
+ return `${name.slice(0, maxLen - 3)}…`;
}🤖 Prompt for AI Agents
In `@src/react/hooks/useGateLayers.ts` around lines 13 - 16, truncateGateLabel
currently slices name with name.slice(0, maxLen - 1) then appends "..." which
can exceed MAX_LABEL_LENGTH; update the logic in truncateGateLabel to slice to
maxLen - 3 before appending the ellipsis so the final string length ≤ maxLen,
and also guard for tiny maxLen values (e.g., if maxLen <= 3, return
name.slice(0, maxLen) without adding "...") to avoid negative slice indices.
| useEffect(() => { | ||
| const positions = new Map<string, [number, number]>(); | ||
| gates.forEach((gate) => { | ||
| if (gate.labelPosition) { | ||
| positions.set(gate.id, gate.labelPosition); | ||
| } else { | ||
| const centroid = computeCentroid(gate.geometry); | ||
| positions.set(gate.id, [centroid[0], centroid[1]]); | ||
| } | ||
| }); | ||
| setLabelPositions(positions); | ||
| }, [gates]); |
There was a problem hiding this comment.
Label positions are fully reset whenever gates reference changes.
This useEffect overwrites all label positions — including any user-dragged positions — whenever the gates array reference updates (e.g., adding a new gate resets all existing dragged positions). Consider merging new/changed gates into the existing map instead of replacing it.
Proposed fix — merge instead of replace
useEffect(() => {
- const positions = new Map<string, [number, number]>();
- gates.forEach((gate) => {
- if (gate.labelPosition) {
- positions.set(gate.id, gate.labelPosition);
- } else {
- const centroid = computeCentroid(gate.geometry);
- positions.set(gate.id, [centroid[0], centroid[1]]);
- }
- });
- setLabelPositions(positions);
+ setLabelPositions((prev) => {
+ const positions = new Map<string, [number, number]>();
+ gates.forEach((gate) => {
+ // Keep existing dragged position if present, otherwise use gate metadata or centroid
+ const existing = prev.get(gate.id);
+ if (gate.labelPosition) {
+ positions.set(gate.id, gate.labelPosition);
+ } else if (existing) {
+ positions.set(gate.id, existing);
+ } else {
+ const centroid = computeCentroid(gate.geometry);
+ positions.set(gate.id, [centroid[0], centroid[1]]);
+ }
+ });
+ return positions;
+ });
}, [gates]);📝 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.
| useEffect(() => { | |
| const positions = new Map<string, [number, number]>(); | |
| gates.forEach((gate) => { | |
| if (gate.labelPosition) { | |
| positions.set(gate.id, gate.labelPosition); | |
| } else { | |
| const centroid = computeCentroid(gate.geometry); | |
| positions.set(gate.id, [centroid[0], centroid[1]]); | |
| } | |
| }); | |
| setLabelPositions(positions); | |
| }, [gates]); | |
| useEffect(() => { | |
| setLabelPositions((prev) => { | |
| const positions = new Map<string, [number, number]>(); | |
| gates.forEach((gate) => { | |
| // Keep existing dragged position if present, otherwise use gate metadata or centroid | |
| const existing = prev.get(gate.id); | |
| if (gate.labelPosition) { | |
| positions.set(gate.id, gate.labelPosition); | |
| } else if (existing) { | |
| positions.set(gate.id, existing); | |
| } else { | |
| const centroid = computeCentroid(gate.geometry); | |
| positions.set(gate.id, [centroid[0], centroid[1]]); | |
| } | |
| }); | |
| return positions; | |
| }); | |
| }, [gates]); |
🤖 Prompt for AI Agents
In `@src/react/hooks/useGateLayers.ts` around lines 35 - 46, The effect on gates
currently recreates and replaces the entire labelPositions Map, wiping out any
user-dragged positions; change the logic in the useEffect that calls
setLabelPositions so it merges into the existing Map instead of replacing it:
use the functional updater form setLabelPositions(prev => { const next = new
Map(prev); for each gate in gates, if gate.labelPosition then next.set(gate.id,
gate.labelPosition) else if !next.has(gate.id) computeCentroid(gate.geometry)
and next.set(gate.id, centroid); also remove entries from next for gate IDs no
longer present in gates to avoid stale keys; reference the useEffect that
iterates gates, computeCentroid, and setLabelPositions when implementing this
merge behavior.
2 participants
Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests