Gustavohsdp/lec 223 create base table component presentation layer#251
Conversation
…type table component
WalkthroughThis update introduces a set of reusable table and modal components to standardize UI patterns across the application. Custom table and modal implementations are replaced with generic components, and table action buttons are consolidated into a single, configurable component. The codebase is refactored for consistency, maintainability, and improved accessibility. Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent
participant GenericTable
participant TableActions
participant DimensionsModal
ParentComponent->>GenericTable: Pass columns, data, selection, update handler
ParentComponent->>TableActions: Pass actions array (add, remove, move, etc.)
ParentComponent->>DimensionsModal: Pass open state, type, dimensions, handlers
GenericTable->>ParentComponent: Call update handler on cell edit
TableActions->>ParentComponent: Invoke action handlers on button click
DimensionsModal->>ParentComponent: Invoke callbacks on save/cancel/type change
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (8)
src/renderer/components/_molecules/data-types/array/table/index.tsx (2)
35-55:⚠️ Potential issueMemoisation array misses
handleBlur(and captures a stale closure)
handleBluris defined after thisuseMemoand is recreated on every render, but it is not included in the dependency list.
Consequently,columnsmay hold on to an outdated version ofhandleBlur, and therefore to staletableData/updateDatatypereferences, leading to silently‐dropped updates.-const columns = React.useMemo( - () => [ +const handleBlur = React.useCallback( + (rowIndex: number) => { + /* ...existing body... */ + }, + [tableData, name, updateDatatype, setArrayTable], +) + +const columns = React.useMemo( + () => [and update the deps to:
- [name, selectedRow], + [name, selectedRow, handleBlur],This keeps
columnsand the captured callback safely in sync.
57-90: 🛠️ Refactor suggestionAvoid direct DOM queries inside React components
handleBlurreaches for the input withdocument.getElementById.
Because the component already owns the value via React state (DimensionCell), the same operation can be performed without breaking out of the React data-flow, e.g.:cell: (cellProps) => ( <DimensionCell /* … */ onBlur={(value) => handleBlur(cellProps.row.index, value)} /> )and pass the edited value directly, removing both the
getElementByIdcall and the need to trim again.This reduces coupling to the DOM structure and eliminates a potential source of bugs when IDs collide.
src/renderer/components/_molecules/data-types/structure/table/elements/array-modal.tsx (2)
65-78:⚠️ Potential issue
dimensions.splicemutates state in place – causes unpredictable behaviour
spliceoperates on the same array instance stored in React state, yetsetDimensionsis not called afterwards.
AlthoughsetSelectedInputtriggers a re-render, React still sees the same array reference and may bail out of updates or produce subtle bugs.-const [removed] = dimensions.splice(index, 1) -dimensions.splice(index - 1, 0, removed) -setSelectedInput((index - 1).toString()) +setDimensions((prev) => { + const next = [...prev] + const [removed] = next.splice(index, 1) + next.splice(index - 1, 0, removed) + return next +}) +setSelectedInput((index - 1).toString())Do the same for the down branch.
This preserves immutability, plays well with React dev-tools, and avoids side-effects in render logic.
102-111: 🛠️ Refactor suggestionGuard against empty or whitespace-only dimensions earlier
dimensionToSavefilters only empty strings, so a value like" "passes through.
arrayValidationalready rejects this, but calling it once per dimension (during blur) can miss the scenario where all rows are blank due to mass deletion.Simplest fix:
-const dimensionToSave = dimensions.filter((value) => value !== '') +const dimensionToSave = dimensions.map((v) => v.trim()).filter(Boolean)This rejects whitespace-only entries and avoids malformed PLC type strings.
src/renderer/components/_molecules/global-variables-table/index.tsx (1)
28-33:⚠️ Potential issue
cell: 'Global'is typed as an accessor, not a renderer
createColumnHelper().accessorexpects a renderer function forcell; passing a raw string is interpreted as an accessor key, which will look forrow.original['Global']and renderundefined. Use a constant renderer instead:- columnHelper.accessor('class', { - header: 'Class', - enableResizing: true, - cell: 'Global', - }), + columnHelper.accessor('class', { + header: 'Class', + enableResizing: true, + cell: () => 'Global', + }),src/renderer/components/_molecules/data-types/enumerated/index.tsx (2)
25-31:⚠️ Potential issue
initialValueDatanever updates when the prop changesThe first
useEffectruns only on mount. If the parent editsdata.initialValuelater, the local state will be stale.-useEffect(() => { - setInitialValueData(data.initialValue || '') -}, []) +useEffect(() => { + setInitialValueData(data.initialValue || '') +}, [data.initialValue])
88-91: 🛠️ Refactor suggestionAvoid mutating
prevRowsinsidesetStateInside the
moveRowUpcallback you reassignprevRows = newRows.
Even though it “works”, mutating the parameter defies React’s immutability expectations and may hide bugs in the future.Replace the whole callback with a pure calculation and return the new array only once.
src/renderer/components/_organisms/variables-editor/index.tsx (1)
189-203:⚠️ Potential issueDeleting the last (or only) row can leave
selectedRowat ‑2When the table has a single variable,
selectedRowis 0.
After deletion we computeselectedRow - 1(→ -1) and immediately store it, which later becomesparseInt(selectedRow)→ -1 → out-of-bounds access.Guard against the empty-table case:
- updateModelVariables({ display: 'table', selectedRow: selectedRow - 1 }) + updateModelVariables({ + display: 'table', + selectedRow: Math.max(selectedRow - 1, ROWS_NOT_SELECTED), + })(or skip the field altogether when no rows remain).
🧹 Nitpick comments (13)
src/renderer/components/_molecules/data-types/array/index.tsx (1)
207-254: Well-executed refactoring of array dimensions UI sectionThe consolidation of table action buttons using the
TableActionscomponent and the restructured layout improves code organization and maintainability.However, the use of a fixed width (
w-[600px]) on line 207 may limit responsiveness on smaller screens. Consider using a relative width (e.g.,w-fullwith a max-width constraint) for better adaptability across different screen sizes.- <div className='flex w-[600px] flex-col gap-3'> + <div className='flex w-full max-w-[600px] flex-col gap-3'>src/renderer/components/_molecules/data-types/array/table/index.tsx (1)
92-127: Border-reset logic duplicates behaviour now owned byGenericDataTypeTableAll the manual class-name juggling (
resetBorders,setBorders) replicates selection styling that the new generic table could expose internally.
Consider moving the styling concern intoGenericDataTypeTable(or a shared hook) to:
- Keep this component declarative and focused on data.
- Eliminate two imperative
useEffectruns per row selection and the repeatedclassNamestring concat.Reducing DOM mutations will also improve rendering performance on large tables.
src/renderer/components/_atoms/dimensions-modal/array-dimensions-input/index.tsx (1)
40-55: Accessibility nit – input lacks an explicit label and usefularia-*attributesBecause the surrounding
<div>is clickable and the<input>can be non-interactive (pointer-events-none), screen-reader users might have trouble understanding what the field represents.Consider:
-<input +<input + aria-label={`Array dimension ${id}`} + aria-disabled={selectedInput !== id}or tying it to a visually-hidden
<label htmlFor={id}>Dimension {id}</label>for better semantics.src/renderer/components/_atoms/generic-table/index.tsx (2)
102-106:maxSize==sizeprevents growth
defaultColumnsetssizeandmaxSizeto128, effectively disabling expansion while still allowing shrink tominSize.
If the design should allow the user to widen columns, set a largermaxSizeor omit it (react-table falls back to ∞).
117-118: DisabledebugTablein production builds
debugTable: truefloods the console and ships unnecessary code. Flip it tofalse(or gate behindprocess.env.NODE_ENV !== 'production').src/renderer/components/_molecules/data-types/enumerated/index.tsx (2)
123-125: Hard-coded width (600px) hinders re-useEmbedding magic numbers inside the component makes it impossible to adapt to different layouts/break-points.
Expose it as a prop or rely on flex/grid to size naturally:
-<div className='w-[600px]'> +<div className={cn('w-full', props.fixedWidth && `max-w-[${props.fixedWidth}px]`)}>
152-165: Duplicate enable/disable logic – consider extractingThe up/down buttons repeat the same long boolean expressions.
Extract a helper:const canMoveUp = selectedRow > 0; const canMoveDown = selectedRow > -1 && selectedRow < tableData.length - 1;This improves readability and de-duplicates the predicates.
src/renderer/components/_molecules/data-types/structure/table/index.tsx (1)
57-82: Heavy re-mapping on every cell edit
updateDatamaps the entiretableDataarray for each change, causing O(n) work per keystroke.
For large structures this may lag.Consider:
- Updating only the changed item (
producefrom immer is handy).- Debouncing
updateDatatypeif it triggers expensive store writes.src/renderer/components/_molecules/variables-table/index.tsx (2)
11-60: Consider deduplicating the two column-sets to remove maintenance overhead
columnsProgramsandcolumnsare 95 % identical. Every time a header is renamed, a size is tuned, or a cell component evolves, both arrays must be kept in sync, which is error-prone.A tiny helper that injects / removes the
locationcolumn would avoid the duplication:-const columnsPrograms = [...] -const columns = [...] +const baseColumns = [ + idCol(), + nameCol(), + classCol(), + typeCol(), + initialValueCol(), + documentationCol(), + debugCol(), +] + +const columnsPrograms = baseColumns // program POU – no location +const columns = [ + ...baseColumns.slice(0, 4), + locationCol(), // insert location + ...baseColumns.slice(4), +]Besides being DRY this makes both tables evolve uniformly.
137-160: Return value fromupdateDatais ignored – consider making itvoid
updateDatacurrently doesreturn updateVariable(...), butGenericTablenever consumes that value.
IfupdateVariableis asynchronous this could create a false expectation of “fire-and-await”.
Explicitly discarding the return value clarifies intent:- updateData={(rowIndex, columnId, value) => { + updateData={(rowIndex, columnId, value): void => { if (columnId === 'class' && filterValue !== undefined && filterValue !== 'all' && filterValue !== value) { return } - return updateVariable({ ... }) + updateVariable({ ... }) }}src/renderer/components/_organisms/variables-editor/index.tsx (2)
347-421: Action button set duplicated – extract to a helper to keep desktop & mobile in syncThe
actionsarray forTableActionsis copy-pasted for the XL and the mobile view. Any tweak (e.g. disabling logic, tool-tips, icon change) must be performed twice.A quick win is to memoise the array once and reuse it:
const commonActions = useMemo(() => [ { ariaLabel: 'Add table row button', onClick: handleCreateVariable, icon: <PlusIcon className='!stroke-brand' />, id: 'add-variable-button' }, { ariaLabel: 'Remove table row button', onClick: handleRemoveVariable, disabled: selectedRowInvalid || forbiddenVariableToBeRemoved, icon: <MinusIcon />, id: 'remove-variable-button' }, { ariaLabel: 'Move table row up button', onClick: () => handleRearrangeVariables(-1), disabled: selectedRowInvalid || selectedRow === 0, icon: <StickArrowIcon direction='up' className='stroke-[#0464FB]' />, id: 'move-variable-up-button' }, { ariaLabel: 'Move table row down button', onClick: () => handleRearrangeVariables(1), disabled: selectedRowInvalid || selectedRow === tableData.length - 1, icon: <StickArrowIcon direction='down' className='stroke-[#0464FB]' />, id: 'move-variable-down-button' }, ], [editorVariables, tableData.length, forbiddenVariableToBeRemoved])…and then
<TableActions actions={commonActions} />in both places.
That guarantees perfect parity and makes further maintenance simpler.
205-215:handleFilterChangemixes “All”/'all'cases – normalise onceThe function lower-cases the filter value only when adding it to
columnFilters, buteditorVariables.classFilterstores the original cased enum.
Later comparisons keep juggling.toLowerCase().Consider storing the canonical lower-case in the model and formatting for display only in the UI, reducing the number of conversions required.
src/renderer/components/_atoms/dimensions-modal/index.tsx (1)
68-101: Minor accessibility improvement – connect label & select withhtmlFor/idThe “Base type”
<label>is not programmatically associated with the<SelectTrigger>(no matchinghtmlFor/id).
Screen-reader users will not know what the combo-box does.-<label ...>Base type</label> -<SelectTrigger id='array-base-type' .../> +<label htmlFor='array-base-type' ...>Base type</label> +<SelectTrigger id='array-base-type' .../>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/renderer/components/_atoms/dimensions-modal/array-dimensions-input/index.tsx(1 hunks)src/renderer/components/_atoms/dimensions-modal/index.tsx(1 hunks)src/renderer/components/_atoms/generic-data-type-table/index.tsx(1 hunks)src/renderer/components/_atoms/generic-table/index.tsx(1 hunks)src/renderer/components/_atoms/table-actions/index.tsx(1 hunks)src/renderer/components/_molecules/data-types/array/index.tsx(2 hunks)src/renderer/components/_molecules/data-types/array/table/index.tsx(3 hunks)src/renderer/components/_molecules/data-types/enumerated/index.tsx(2 hunks)src/renderer/components/_molecules/data-types/enumerated/table/index.tsx(3 hunks)src/renderer/components/_molecules/data-types/structure/index.tsx(2 hunks)src/renderer/components/_molecules/data-types/structure/table/elements/array-modal.tsx(2 hunks)src/renderer/components/_molecules/data-types/structure/table/index.tsx(3 hunks)src/renderer/components/_molecules/global-variables-table/index.tsx(2 hunks)src/renderer/components/_molecules/instances-table/index.tsx(2 hunks)src/renderer/components/_molecules/task-table/index.tsx(2 hunks)src/renderer/components/_molecules/variables-table/elements/array-input.tsx(0 hunks)src/renderer/components/_molecules/variables-table/elements/array-modal.tsx(2 hunks)src/renderer/components/_molecules/variables-table/index.tsx(3 hunks)src/renderer/components/_organisms/explorer/index.tsx(2 hunks)src/renderer/components/_organisms/global-variables-editor/index.tsx(2 hunks)src/renderer/components/_organisms/instances-editor/index.tsx(2 hunks)src/renderer/components/_organisms/task-editor/index.tsx(3 hunks)src/renderer/components/_organisms/variables-editor/index.tsx(9 hunks)
💤 Files with no reviewable changes (1)
- src/renderer/components/_molecules/variables-table/elements/array-input.tsx
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/renderer/components/_molecules/variables-table/elements/array-modal.tsx (1)
src/renderer/components/_atoms/dimensions-modal/index.tsx (1)
DimensionsModal(34-174)
src/renderer/components/_molecules/data-types/structure/table/elements/array-modal.tsx (1)
src/renderer/components/_atoms/dimensions-modal/index.tsx (1)
DimensionsModal(34-174)
src/renderer/components/_molecules/task-table/index.tsx (2)
src/renderer/components/_atoms/generic-table/index.tsx (1)
GenericTable(177-177)src/types/PLC/open-plc.ts (1)
PLCTask(327-327)
src/renderer/components/_atoms/generic-table/index.tsx (2)
src/utils/index.ts (1)
cn(3-3)src/renderer/components/_atoms/table/index.tsx (6)
Table(132-132)TableHeader(132-132)TableRow(132-132)TableHead(132-132)TableBody(132-132)TableCell(132-132)
src/renderer/components/_molecules/data-types/structure/table/index.tsx (2)
src/renderer/components/_atoms/generic-table/index.tsx (1)
GenericTable(177-177)src/types/PLC/open-plc.ts (1)
PLCStructureVariable(326-326)
src/renderer/components/_organisms/variables-editor/index.tsx (1)
src/utils/index.ts (1)
cn(3-3)
src/renderer/components/_atoms/dimensions-modal/index.tsx (2)
src/types/PLC/open-plc.ts (1)
BaseType(311-311)src/renderer/components/_atoms/dimensions-modal/array-dimensions-input/index.tsx (1)
ArrayDimensionsInput(65-65)
🔇 Additional comments (18)
src/renderer/components/_organisms/explorer/index.tsx (1)
69-69: LGTM! Good UI enhancement.The addition of the
max-w-lgclass to the ResizablePanel helps maintain a consistent UI by preventing the explorer panel from becoming too wide on larger screens.src/renderer/components/_organisms/global-variables-editor/index.tsx (1)
165-199: Great refactoring to improve component reusability.Replacing multiple individual action buttons with the consolidated
TableActionscomponent significantly improves code maintainability while preserving the original functionality. This change follows good component design principles by:
- Reducing code duplication
- Creating a consistent UI pattern for table actions
- Maintaining proper accessibility with aria labels
- Preserving all original functionality and disabled states
This aligns well with the PR's goal of standardizing table implementations.
src/renderer/components/_organisms/instances-editor/index.tsx (1)
155-189: Consistent implementation of the TableActions pattern.The refactoring maintains consistency with the same pattern used in other editor components, which is excellent for codebase uniformity. This approach ensures that:
- All table interactions follow the same UI pattern
- Future modifications to table actions will be easier to implement
- The code remains DRY (Don't Repeat Yourself)
This standardization aligns perfectly with the PR objective of creating reusable and modular components.
src/renderer/components/_organisms/task-editor/index.tsx (1)
156-192: Good TableActions implementation with improved spacing.The refactoring successfully implements the standardized
TableActionspattern while also improving the UI with better spacing through the addedmr-2class. This change:
- Maintains consistency with other editor components
- Improves the visual appearance with proper spacing
- Preserves all the original functionality and disabled states
This comprehensive approach to standardization across all table-based components will significantly enhance maintainability.
src/renderer/components/_molecules/data-types/structure/index.tsx (1)
192-223: Well-executed refactoring of table actions!Consolidating the individual action buttons into a single
TableActionscomponent significantly improves code maintainability. The approach keeps all the same functionality while creating a more consistent UI pattern throughout the application.src/renderer/components/_molecules/data-types/enumerated/table/index.tsx (3)
1-1: Good import addition for new generic table componentThe import of
GenericDataTypeTablealigns with the goal of standardizing table implementations across the application.
7-8: Clean import refactoringStreamlining the imports to include only the necessary functions from
@tanstack/react-tableimproves code clarity.
190-197: Excellent simplification of table rendering logicReplacing the custom table implementation with the reusable
GenericDataTypeTablecomponent reduces code duplication and improves maintainability while preserving all the original functionality.src/renderer/components/_molecules/variables-table/elements/array-modal.tsx (2)
1-1: Good import addition for new dimensions modal componentThe import of
DimensionsModalaligns with the standardization goals of the PR.
151-166: Excellent modal implementation refactoringThe replacement of custom modal implementation with the reusable
DimensionsModalcomponent significantly simplifies the code while preserving all functionality. All necessary props are correctly passed to maintain the same behavior.src/renderer/components/_molecules/data-types/array/index.tsx (2)
4-4: Good import addition for new table actions componentThe import of
TableActionsaligns with the standardization goals of the PR.
216-243: Well-executed implementation of TableActions componentThe refactoring of multiple individual action buttons into a single
TableActionscomponent with an array of configuration objects maintains all functionality while promoting consistency and reusability across the application.src/renderer/components/_atoms/generic-data-type-table/index.tsx (1)
34-36:handleRowClicktyping likely breaks with customTableRowcomponent
You declarehandleRowClick: (target: HTMLTableRowElement) => voidbut passe.currentTargetcoming from a customTableRowcomponent, not a native<tr>. UnlessTableRowforwards the ref to an actual<tr>, this violates the contract and can produce runtime‐only bugs when consumers access native properties (e.g..cells). Consider:type HandleRowClick = (rowElement: HTMLElement) => voidor expose the row’s logical data instead of the DOM element altogether.
src/renderer/components/_molecules/global-variables-table/index.tsx (1)
73-82: Row index ≠ stable row identifier – confirmupdateVariableexpectation
updateVariable({ scope: 'global', rowId: rowIndex, ... })passes the visible row index. If rows can be sorted, filtered, or removed, the index may not match the variable’s persistentid, causing the wrong row to be updated.Please verify what
rowIdrepresents in the store and pass a stable key instead (e.g.tableData[rowIndex].id).src/renderer/components/_molecules/task-table/index.tsx (1)
61-70: Validate row identifier passed toupdateTask
As with the variables table,rowIdis the array index, not a unique task identifier. Re-ordering tasks (e.g. by priority) will risk updating the wrong store entry. PasstableData[rowIndex].idor similar to guarantee correctness.src/renderer/components/_atoms/generic-table/index.tsx (1)
86-97: Effect dependencies look incomplete
filterValueis compared by reference; when the parent supplies a new string instance with the same value the effect will still run.
More importantly, if you ever switch to controlled filtering (columnFilters) the borders will not reset unless you remember to add that state to this array.-}, [filterValue]) +}, [filterValue, columnFilters])Please double-check the intended re-paint triggers.
src/renderer/components/_molecules/data-types/enumerated/index.tsx (1)
54-70:selectedRow !== nullcheck is inconsistent with the sentinel-1
selectedRowis initialised with-1, so the condition passes even when nothing is selected.
Prefer a unified sentinel (eithernullor-1) and test explicitly:-if (arrayTable.selectedRow !== null) { +if (arrayTable.selectedRow > -1) {This applies to
removeRow,moveRowUp, andmoveRowDown.src/renderer/components/_molecules/data-types/structure/table/index.tsx (1)
11-19:idaccessor may break if the data lacks anidfield
PLCStructureVariable(seeopen-plc.ts) has no explicitidproperty.
React-Table will therefore readundefined, and the column will render empty while still relying on the value for keys.If the intention is to show the row index, switch to:
columnHelper.display({ id: '#', header: '#', cell: (info) => info.row.index + 1, size: 64, enableResizing: true, })
| <GenericTable<PLCInstance> | ||
| columns={columns} | ||
| tableData={tableData} | ||
| selectedRow={selectedRow} | ||
| handleRowClick={handleRowClick} | ||
| // @ts-expect-error - The data value is a literal type that need to be parsed | ||
| updateData={(rowIndex, columnId, value) => updateInstance({ rowId: rowIndex, data: { [columnId]: value } })} | ||
| tableContext='Instances' | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ts-expect-error hides a typing mismatch that is easy to solve
The generic updateData lambda is re-created on every render and suppresses the TypeScript error instead of fixing it.
Two quick wins:
-
Memoise the callback:
const updateData = React.useCallback( (rowIndex: number, columnId: keyof PLCInstance, value: PLCInstance[keyof PLCInstance]) => updateInstance({ rowId: rowIndex, data: { [columnId]: value } }), [updateInstance], )
-
Pass it without the
@ts-expect-error.
Removing the suppression improves static safety and avoids unnecessary re-renders.
🤖 Prompt for AI Agents
In src/renderer/components/_molecules/instances-table/index.tsx around lines 49
to 57, the updateData callback is recreated on every render and uses
@ts-expect-error to suppress a typing mismatch. To fix this, memoize the
updateData function using React.useCallback with the correct parameter types
(rowIndex as number, columnId as keyof PLCInstance, and value as
PLCInstance[keyof PLCInstance]) and include updateInstance in the dependency
array. Then remove the @ts-expect-error comment and pass the memoized updateData
callback to the GenericTable component to improve type safety and prevent
unnecessary re-renders.
| <TableActionButton | ||
| key={index} | ||
| aria-label={action.ariaLabel} | ||
| onClick={action.onClick} | ||
| disabled={action.disabled} | ||
| id={action.id} | ||
| {...buttonProps} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure per-action disabled/id props are not accidentally overridden
{...buttonProps} is spread after the action-specific attributes, so any disabled or id coming from buttonProps will silently win over the values coming from action. This makes the component harder to reason about and can break row-level logic.
- disabled={action.disabled}
- id={action.id}
- {...buttonProps}
+ {...buttonProps}
+ disabled={action.disabled}
+ id={action.id}Re-ordering keeps the override direction intuitive: generic defaults first, then action specifics.
📝 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.
| <TableActionButton | |
| key={index} | |
| aria-label={action.ariaLabel} | |
| onClick={action.onClick} | |
| disabled={action.disabled} | |
| id={action.id} | |
| {...buttonProps} | |
| > | |
| <TableActionButton | |
| key={index} | |
| aria-label={action.ariaLabel} | |
| onClick={action.onClick} | |
| {...buttonProps} | |
| disabled={action.disabled} | |
| id={action.id} | |
| > |
🤖 Prompt for AI Agents
In src/renderer/components/_atoms/table-actions/index.tsx around lines 22 to 29,
the spread of {...buttonProps} comes after the action-specific props like
disabled and id, causing any disabled or id in buttonProps to override the
action values. To fix this, reorder the props so that {...buttonProps} is spread
first, followed by the action-specific props, ensuring that action.disabled and
action.id take precedence and prevent accidental overrides.
| {actions.map((action, index) => ( | ||
| <TableActionButton | ||
| key={index} | ||
| aria-label={action.ariaLabel} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array index as React key
key={index} will force React to re-mount buttons when the order of actions changes, losing focus/animation state. Prefer a stable identifier (action.id or ariaLabel).
- <TableActionButton
- key={index}
+ <TableActionButton
+ key={action.id ?? action.ariaLabel}📝 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.
| {actions.map((action, index) => ( | |
| <TableActionButton | |
| key={index} | |
| aria-label={action.ariaLabel} | |
| {actions.map((action, index) => ( | |
| <TableActionButton | |
| key={action.id ?? action.ariaLabel} | |
| aria-label={action.ariaLabel} |
🤖 Prompt for AI Agents
In src/renderer/components/_atoms/table-actions/index.tsx around lines 21 to 24,
replace the use of the array index as the React key in the TableActionButton
component with a stable and unique identifier such as action.id or
action.ariaLabel to prevent unnecessary remounting and preserve component state
during reordering.
| {table.getRowModel().rows.map((row, index: number) => ( | ||
| <TableRow | ||
| id={`${index}`} | ||
| key={index} | ||
| className='h-8' | ||
| selected={selectedRow === index} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stable row keys improve diffing & accessibility
key={index} and id={${index}} are brittle when the data order changes (sort, filter, drag-reorder). Use row.id (from TanStack) or a domain identifier if available.
- id={`${index}`}
- key={index}
+ id={String(row.id)}
+ key={row.id}📝 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.
| {table.getRowModel().rows.map((row, index: number) => ( | |
| <TableRow | |
| id={`${index}`} | |
| key={index} | |
| className='h-8' | |
| selected={selectedRow === index} | |
| {table.getRowModel().rows.map((row, index: number) => ( | |
| <TableRow | |
| - id={`${index}`} | |
| - key={index} | |
| + id={String(row.id)} | |
| + key={row.id} | |
| className='h-8' | |
| selected={selectedRow === index} |
🤖 Prompt for AI Agents
In src/renderer/components/_atoms/generic-data-type-table/index.tsx around lines
28 to 33, replace the use of index for the key and id props with a stable
identifier like row.id provided by TanStack. This ensures keys remain consistent
across data operations like sorting or filtering, improving React's diffing
performance and accessibility. Update both key and id attributes to use row.id
or another unique domain-specific identifier if available.
| // @ts-expect-error - The data value is a literal type that need to be parsed | ||
| updateData={(rowIndex, columnId, value) => updateTask({ rowId: rowIndex, data: { [columnId]: value } })} | ||
| tableContext='Tasks' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid silencing type errors with @ts-expect-error; fix the typing instead
The comment hides a genuine mismatch between value’s literal type and the expected field type, which will propagate unchecked data into the store.
Suggestion: overload updateData generically or cast value as PLCTask[keyof PLCTask] to satisfy the compiler without suppressing it globally.
- // @ts-expect-error - The data value is a literal type that need to be parsed
- updateData={(rowIndex, columnId, value) => updateTask({ rowId: rowIndex, data: { [columnId]: value } })}
+ updateData={(rowIndex, columnId, value) =>
+ updateTask({ rowId: rowIndex, data: { [columnId]: value as PLCTask[keyof PLCTask] } })
+ }📝 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.
| // @ts-expect-error - The data value is a literal type that need to be parsed | |
| updateData={(rowIndex, columnId, value) => updateTask({ rowId: rowIndex, data: { [columnId]: value } })} | |
| tableContext='Tasks' | |
| updateData={(rowIndex, columnId, value) => | |
| updateTask({ rowId: rowIndex, data: { [columnId]: value as PLCTask[keyof PLCTask] } }) | |
| } | |
| tableContext='Tasks' |
🤖 Prompt for AI Agents
In src/renderer/components/_molecules/task-table/index.tsx around lines 67 to
69, the use of @ts-expect-error to suppress type errors on the updateData prop
hides a real type mismatch between the literal type of value and the expected
field type. To fix this, remove the @ts-expect-error comment and instead either
create a generic overload for updateData that properly types value or explicitly
cast value as PLCTask[keyof PLCTask] to satisfy the TypeScript compiler while
preserving type safety.
| const resetBorders = () => { | ||
| const parent = tableBodyRef.current | ||
| const header = tableHeaderRef.current | ||
| if (!parent?.children || !header?.children) return | ||
|
|
||
| const rows = Array.from(parent.children) | ||
| const headers = Array.from(header.children) | ||
| rows.forEach((row) => { | ||
| row.className = cn( | ||
| row.className, | ||
| '[&:last-child>td]:border-b-neutral-500 [&>td:first-child]:border-l-neutral-500 [&>td:last-child]:border-r-neutral-500 [&>td]:border-b-neutral-300', | ||
| 'dark:[&>td:first-child]:border-l-neutral-500 dark:[&>td:last-child]:border-r-neutral-500 dark:[&>td]:border-b-neutral-800', | ||
| 'shadow-none dark:shadow-none', | ||
| ) | ||
| }) | ||
| headers.forEach((header) => { | ||
| header.className = cn(header.className, '[&>th]:border-neutral-300 dark:[&>th]:border-neutral-800') | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid costly DOM class mutations inside resetBorders
resetBorders iterates over every row and header element and blindly appends utility-classes to the existing className.
On tables with hundreds of rows this runs on every selectedRow / filterValue change, leading to unnecessary layout thrashing and ever-growing className strings (duplicates are never removed).
Consider a render-time approach instead:
-rows.forEach((row) => { row.className = cn(row.className, /* … */) })
+// Prefer: add a CSS class conditionally in JSX (e.g. `selected` / `unselected`)
+// and let Tailwind / CSS handle the style, avoiding imperative DOM updates.This removes a whole class of performance issues and makes the visual state declarative.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/renderer/components/_atoms/generic-table/index.tsx around lines 43 to 61,
the resetBorders function inefficiently mutates the className of every row and
header on each call, causing performance issues and className bloat. Refactor to
remove this DOM manipulation by applying the border utility classes
declaratively during render, using conditional className logic or CSS-in-JS to
ensure classes are set once based on state, avoiding repeated appends and
improving performance.
| const setBorders = () => { | ||
| const row = tableBodyRowRef.current | ||
| const parent = tableBodyRef.current | ||
| const header = tableHeaderRef.current | ||
| if (!row || !parent || !header) return | ||
|
|
||
| const headerRow = row === parent?.firstChild ? header?.lastElementChild : null | ||
| const element = headerRow ?? row?.previousElementSibling | ||
|
|
||
| if (element) { | ||
| element.className = cn( | ||
| element.className, | ||
| '[&>td]:border-b-brand dark:[&>td]:border-b-brand', | ||
| '[&>th]:border-b-brand dark:[&>th]:border-b-brand', | ||
| ) | ||
| } | ||
| row.className = cn( | ||
| row.className, | ||
| '[&:last-child>td]:border-b-brand [&>td:first-child]:border-l-brand [&>td:last-child]:border-r-brand [&>td]:border-b-brand', | ||
| 'dark:[&>td:first-child]:border-l-brand dark:[&>td:last-child]:border-r-brand dark:[&>td]:border-b-brand', | ||
| ) | ||
| } |
There was a problem hiding this comment.
Risk of stale border-highlight & duplicated classes in setBorders
- The method only adds the “brand” border utilities;
resetBordersnever strips them, so toggling the same row several times can accumulate duplicate class names. row === parent?.firstChildworks only if React never re-orders DOM nodes; virtualised tables or future re-sorts will break the logic.- Direct string concatenation bypasses React’s diffing and may collide with Tailwind’s class-dedupe.
If the visual emphasis must remain imperative, at least use classList APIs:
-element.className = cn(element.className, '[&>td]:border-b-brand', …)
+element.classList.add('border-brand-highlight')…and remove the class first to guarantee idempotency.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/renderer/components/_atoms/generic-table/index.tsx around lines 63 to 84,
the setBorders function adds border classes by concatenating strings, which can
cause duplicate classes and stale highlights when toggling rows multiple times.
To fix this, replace the string concatenation with classList.add and
classList.remove calls to ensure classes are added and removed idempotently.
Also, avoid relying on row === parent?.firstChild for header detection; instead,
use a more reliable method to identify the header row to support dynamic table
reordering or virtualization.
| <TableRow | ||
| id={row.id} | ||
| key={row.id} | ||
| className='h-8 cursor-pointer' | ||
| onClick={(e) => handleRowClick(e.currentTarget)} | ||
| selected={selectedRow === parseInt(row.id)} | ||
| ref={selectedRow === parseInt(row.id) ? tableBodyRowRef : null} | ||
| > | ||
| {row.getVisibleCells().map((cell) => ( |
There was a problem hiding this comment.
row.id is not guaranteed to be numeric
parseInt(row.id) assumes row.id is "0" | "1" | …".
As soon as you add grouping or pagination, ids become "0_1" or "row_42", breaking selection logic.
-selected={selectedRow === parseInt(row.id)}
-ref={selectedRow === parseInt(row.id) ? tableBodyRowRef : null}
+selected={selectedRow === row.index}
+ref={selectedRow === row.index ? tableBodyRowRef : null}React-Table exposes row.index explicitly for exactly this purpose.
📝 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.
| <TableRow | |
| id={row.id} | |
| key={row.id} | |
| className='h-8 cursor-pointer' | |
| onClick={(e) => handleRowClick(e.currentTarget)} | |
| selected={selectedRow === parseInt(row.id)} | |
| ref={selectedRow === parseInt(row.id) ? tableBodyRowRef : null} | |
| > | |
| {row.getVisibleCells().map((cell) => ( | |
| <TableRow | |
| id={row.id} | |
| key={row.id} | |
| className='h-8 cursor-pointer' | |
| onClick={(e) => handleRowClick(e.currentTarget)} | |
| - selected={selectedRow === parseInt(row.id)} | |
| - ref={selectedRow === parseInt(row.id) ? tableBodyRowRef : null} | |
| + selected={selectedRow === row.index} | |
| + ref={selectedRow === row.index ? tableBodyRowRef : null} | |
| > | |
| {row.getVisibleCells().map((cell) => ( |
🤖 Prompt for AI Agents
In src/renderer/components/_atoms/generic-table/index.tsx around lines 145 to
153, the code uses parseInt(row.id) assuming row.id is numeric, but row.id can
be non-numeric with grouping or pagination, breaking selection logic. Replace
all instances of parseInt(row.id) with row.index, which React-Table provides
explicitly as a reliable numeric identifier for rows, to fix the selection and
ref assignment logic.
| <TableActions | ||
| actions={[ | ||
| { | ||
| ariaLabel: 'Add table row button', | ||
| onClick: onAddDimension, | ||
| icon: <PlusIcon className='!stroke-brand' />, | ||
| id: 'add-new-row-button', | ||
| }, | ||
| { | ||
| ariaLabel: 'Remove table row button', | ||
| onClick: () => onRemoveDimension(selectedInput), | ||
| icon: <MinusIcon className='stroke-[#0464FB]' />, | ||
| }, | ||
| { | ||
| ariaLabel: 'Move table row up button', | ||
| onClick: () => onRearrangeDimensions(Number(selectedInput), 'up'), | ||
| icon: <StickArrowIcon direction='up' className='stroke-[#0464FB]' />, | ||
| }, | ||
| { | ||
| ariaLabel: 'Move table row down button', | ||
| onClick: () => onRearrangeDimensions(Number(selectedInput), 'down'), | ||
| icon: <StickArrowIcon direction='down' className='stroke-[#0464FB]' />, | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
Disable “remove / move” actions when no dimension is selected
onRemoveDimension(selectedInput) and Number(selectedInput) execute even when selectedInput === '', which will pass an empty string / NaN to your handlers.
Add disabled flags mirroring the variables editor pattern:
-{
- ariaLabel: 'Remove table row button',
- onClick: () => onRemoveDimension(selectedInput),
- icon: <MinusIcon ... />,
-},
+{
+ ariaLabel: 'Remove table row button',
+ onClick: () => onRemoveDimension(selectedInput),
+ disabled: selectedInput === '',
+ icon: <MinusIcon ... />,
+},…and the same for the up/down actions.
📝 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.
| <TableActions | |
| actions={[ | |
| { | |
| ariaLabel: 'Add table row button', | |
| onClick: onAddDimension, | |
| icon: <PlusIcon className='!stroke-brand' />, | |
| id: 'add-new-row-button', | |
| }, | |
| { | |
| ariaLabel: 'Remove table row button', | |
| onClick: () => onRemoveDimension(selectedInput), | |
| icon: <MinusIcon className='stroke-[#0464FB]' />, | |
| }, | |
| { | |
| ariaLabel: 'Move table row up button', | |
| onClick: () => onRearrangeDimensions(Number(selectedInput), 'up'), | |
| icon: <StickArrowIcon direction='up' className='stroke-[#0464FB]' />, | |
| }, | |
| { | |
| ariaLabel: 'Move table row down button', | |
| onClick: () => onRearrangeDimensions(Number(selectedInput), 'down'), | |
| icon: <StickArrowIcon direction='down' className='stroke-[#0464FB]' />, | |
| }, | |
| ]} | |
| /> | |
| <TableActions | |
| actions={[ | |
| { | |
| ariaLabel: 'Add table row button', | |
| onClick: onAddDimension, | |
| icon: <PlusIcon className='!stroke-brand' />, | |
| id: 'add-new-row-button', | |
| }, | |
| { | |
| ariaLabel: 'Remove table row button', | |
| onClick: () => onRemoveDimension(selectedInput), | |
| disabled: selectedInput === '', | |
| icon: <MinusIcon className='stroke-[#0464FB]' />, | |
| }, | |
| { | |
| ariaLabel: 'Move table row up button', | |
| onClick: () => onRearrangeDimensions(Number(selectedInput), 'up'), | |
| disabled: selectedInput === '', | |
| icon: <StickArrowIcon direction='up' className='stroke-[#0464FB]' />, | |
| }, | |
| { | |
| ariaLabel: 'Move table row down button', | |
| onClick: () => onRearrangeDimensions(Number(selectedInput), 'down'), | |
| disabled: selectedInput === '', | |
| icon: <StickArrowIcon direction='down' className='stroke-[#0464FB]' />, | |
| }, | |
| ]} | |
| /> |
🤖 Prompt for AI Agents
In src/renderer/components/_atoms/dimensions-modal/index.tsx around lines 114 to
138, the remove and move actions are enabled even when no dimension is selected,
causing handlers to receive invalid inputs like empty string or NaN. Fix this by
adding a disabled property to the remove, move up, and move down actions that is
true when selectedInput is empty or invalid, preventing these actions from being
triggered without a valid selection.
…-component-presentation-layer
| onBlur={() => handleUpdate()} | ||
| onBlur={handleUpdate} | ||
| value={inputValue} | ||
| aria-selected={selectedInput === id ? 'true' : 'false'} |
There was a problem hiding this comment.
Q: Does this need to be a string?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Pull request info
Link to Linear task
linear task
Description of the changes proposed
DOD checklist
Summary by CodeRabbit
New Features
Refactor
Style
Chores