added sorting feature on button click#202
added sorting feature on button click#202schourasia750 wants to merge 2 commits intoc2siorg:mainfrom
Conversation
ivantha
left a comment
ivantha
left a comment
There was a problem hiding this comment.
◎ Argus Review
This PR implements a well-conceived tri-state sorting feature with correct use of useMemo and a clean visual design. However, it contains a high-severity data-corruption bug: the tbody iterates over sortedData while still using rowIndex (the sorted position) as the canonical row identifier, meaning any cell-edit or mutation operation will silently target the wrong row whenever a sort is active. There are also medium-severity concerns around null/undefined handling in the comparator, missing sort-state reset on data refresh, and the S.No. serial-number column being inadvertently sortable. The feature needs the row-index bug fixed and a basic test suite covering the sort-then-edit flow before it is safe to merge.
Suggested Version Bump: minor
⚠️ Breaking Changes
- [INFO] No public API or component interface changes — semver minor: Previous behavior:
Tableaccepted{ projectId, data: externalData }and rendered rows in original order with non-interactive column header buttons.
New behavior: Column headers now respond to click interactions; rows render in sorted order when a sort is active. Component props, exported types, and the data contract are unchanged.
Who is affected: No external consumers are impacted. The change is additive and entirely internal to Table's render behavior.
Migration path: None required. However, parent components that rely on rowIndex values emitted from Table (e.g., via edit callbacks) should verify compatibility with sorted row positions, given the high-severity bug noted above.
Suggestion: Classify as a minor version bump (new feature, no removed APIs). Resolve the row-index bug before shipping to prevent potential data mutations on wrong rows.
PR Description Issues
- [MEDIUM] PR description is missing a testing section: The description thoroughly explains what the feature does (tri-state cycling, numeric detection,
useMemo) but contains no information about how it was tested. There is no mention of browsers tested, whether existing tests were run, what edge cases were manually verified (empty cells, numeric columns, large datasets), or whether any automated tests were written. This is especially important because the feature modifies row rendering logic and could silently corrupt cell edits.Suggestion: Add a Testing section:
## Testing
- [ ] Manually verified tri-state cycle on string column
- [ ] Verified numeric sort order (2 before 10) on a numeric column
- [ ] Verified that removing sort restores original row order
- [ ] Verified cell editing targets correct row after sorting is applied
- [ ] Ran existing test suite — all tests pass
- [ ] Tested in Chrome and Firefox- [LOW] Description claim about index preservation is misleading: The description states: "your original row indexes and mappings are perfectly preserved when sorting is turned off." While turning off the sort does restore original order, the phrasing implies row indexes are always safe — but as documented in the high-severity finding above,
rowIndexin the rendered view diverges from the original data index while sort is active, which can cause editing operations to target the wrong row.Suggestion: Revise the claim for accuracy:
State Management: Sorting is implemented as a
useMemooverlay. The originaldataarray is never mutated, so removing the sort instantly restores the original order. While sorting is active, the rendered row positions reflect sorted order — any row-mutation operations should use original-index tracking rather than rendered position.
| </th> | ||
| ))} | ||
| </tr> | ||
| </thead> |
There was a problem hiding this comment.
[HIGH] Row index mismatch breaks cell editing when sort is active
The tbody now iterates over sortedData but continues to use rowIndex (the position in the sorted array) as the key and — critically — as the identifier passed to editing state handlers like setEditingCell. When sorting is active, rowIndex refers to the row's position in the sorted view, not its position in the original data array. Any operation that writes back to the server or updates data using this index will target the wrong row, potentially causing silent data corruption.
For example: if a row originally at index 5 becomes index 0 after sorting, clicking "edit" stores rowIndex = 0 in editingCell. The save handler then writes to data[0] (the original first row), not data[5] — corrupting unrelated data.
Previous behavior: data.map((row, rowIndex) => …) — rowIndex always matched the row's canonical position.
New behavior: sortedData.map((row, rowIndex) => …) — rowIndex is the sorted position, diverging from the canonical index whenever sorting is active.
Suggestion:
Attach the original index to each row before sorting, then use it for all mutation operations:
```jsx
const sortedData = useMemo(() => {
const indexed = data.map((row, i) => ({ ...row, _originalIndex: i }));
if (!sortConfig) return indexed;
return [...indexed].sort((a, b) => { /* same comparator */ });
}, [data, sortConfig]);
// In tbody:
{sortedData.map((row) => (
<tr key={row._originalIndex} ...>
{/* pass row._originalIndex to all edit/delete handlers */}
</tr>
))}
| if (aValue > bValue) { | ||
| return sortConfig.direction === "ascending" ? 1 : -1; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
[MEDIUM] Sort comparator does not handle null, undefined, or mixed-type values
The numeric guard only checks aValue !== "" and bValue !== "", but table cells can realistically hold null or undefined (e.g., newly added empty rows).
Number(null)→0, so a null cell silently sorts as zero.Number(undefined)→NaN, causing the guard to fire and falling through to a string comparison of"undefined"vs real values.- When one value is numeric and the other is a string, comparison degrades to JS coercion between a
Numberand aString, producing non-deterministic ordering.
This produces visually wrong orderings that are difficult to debug.
Suggestion:
Normalize values before comparison and push nullish entries to the bottom:
```js
sortableData.sort((a, b) => {
let aValue = a[sortConfig.key] ?? "";
let bValue = b[sortConfig.key] ?? "";
if (aValue === "") return 1;
if (bValue === "") return -1;
const numA = Number(aValue);
const numB = Number(bValue);
const bothNumeric = !isNaN(numA) && !isNaN(numB);
const cmp = bothNumeric
? numA - numB
: String(aValue).localeCompare(String(bValue));
return sortConfig.direction === "ascending" ? cmp : -cmp;
});
| const [columns, setColumns] = useState([]); | ||
| const [editingCell, setEditingCell] = useState(null); | ||
| const [editValue, setEditValue] = useState(""); | ||
| const [sortConfig, setSortConfig] = useState(null); |
There was a problem hiding this comment.
[MEDIUM] sortConfig is not reset when external data changes
When externalData (the prop) is refreshed — after a server sync, row addition, or project switch — sortConfig persists. The user will continue to see a sorted view over a new dataset, and the sort arrow indicator stays visible. If columns are removed or reordered between refreshes, sortConfig.key (a column index) will silently point to the wrong column.
Suggestion:
Reset `sortConfig` when the project (or data identity) changes:
```js
useEffect(() => {
setSortConfig(null);
}, [projectId]);
If columns can change independently, also reset on a column-identity change.
| if (direction) { | ||
| setSortConfig({ key: columnIndex, direction }); | ||
| } else { | ||
| setSortConfig(null); |
There was a problem hiding this comment.
[MEDIUM] S.No. (serial number) column is inadvertently sortable
The handleSort handler is attached to every column header, including S.No.. Sorting by a serial-number column is meaningless (ascending = original order, descending = full reverse) and potentially confusing — users may think the table is in its natural order when it is actually reversed. The code already special-cases S.No. to suppress the DtypeBadge, confirming it is a synthetic column outside the real data schema.
Suggestion:
Skip sort activation for the S.No. column:
```jsx
<button
onClick={() => column !== "S.No." ? handleSort(columnIndex) : undefined}
className={`... ${
column === "S.No." ? "cursor-default pointer-events-none" : ""
}`}
>
Or render a plain <span> instead of a <button> for that column.
| if (aValue > bValue) { | ||
| return sortConfig.direction === "ascending" ? 1 : -1; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
[MEDIUM] useMemo unnecessarily copies the full array when sort is inactive
Even when sortConfig is null, the memo always executes [...data] — allocating a brand-new array reference. This defeats referential stability for any consumers downstream of sortedData and adds unnecessary GC pressure on large tables. It also means child rows re-render on every parent render even when there is no sort active.
Suggestion:
Return the original `data` reference when no sort is configured:
```js
const sortedData = useMemo(() => {
if (!sortConfig) return data; // stable reference, no copy
return [...data].sort((a, b) => { /* … */ });
}, [data, sortConfig]);
| if (direction) { | ||
| setSortConfig({ key: columnIndex, direction }); | ||
| } else { | ||
| setSortConfig(null); |
There was a problem hiding this comment.
[LOW] handleSort logic is verbose and uses an indirect null-direction pattern
The function sets direction = null to signal a reset, then re-checks if (direction) to decide what to set. This two-step indirection (set a local variable to null, then re-test it) obscures the tri-state machine and requires the reader to trace through two conditional blocks to understand the transitions.
Suggestion:
Express the three states as three direct branches:
```js
const handleSort = (columnIndex) => {
if (!sortConfig || sortConfig.key !== columnIndex) {
setSortConfig({ key: columnIndex, direction: "ascending" });
} else if (sortConfig.direction === "ascending") {
setSortConfig({ key: columnIndex, direction: "descending" });
} else {
setSortConfig(null);
}
};
| if (aValue > bValue) { | ||
| return sortConfig.direction === "ascending" ? 1 : -1; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
[LOW] No tests added for the new sorting feature
The PR introduces a non-trivial sorting feature with three distinct states, a custom comparator with numeric type detection, and useMemo dependency tracking — none of which are covered by new tests. Given that the row-index bug identified above can cause silent data corruption, test coverage here is important rather than optional.
Critical cases that should be covered:
- Clicking once → ascending, twice → descending, three times → cleared
- Numeric columns sort numerically (2 before 10)
- Empty/null cells are pushed to the bottom
- Editing a row after sorting targets the correct original row
Suggestion:
Add tests using React Testing Library:
```jsx
it('cycles through ascending → descending → unsorted on header click', () => {
render(<Table projectId="1" data={mockData} />);
const header = screen.getByRole('button', { name: /name/i });
fireEvent.click(header); expect(header).toHaveTextContent('▲');
fireEvent.click(header); expect(header).toHaveTextContent('▼');
fireEvent.click(header);
expect(header).not.toHaveTextContent('▲');
expect(header).not.toHaveTextContent('▼');
});
| )} | ||
| </button> | ||
| </th> | ||
| ))} |
There was a problem hiding this comment.
[LOW] Missing ARIA aria-sort attributes reduce accessibility
The WAI-ARIA specification defines aria-sort on <th> elements specifically so screen readers can communicate sort state to assistive technology users. Without it, the ▲/▼ indicators are purely visual and inaccessible. Valid values are "ascending", "descending", and "none".
Suggestion:
Add `aria-sort` to the `<th>` element:
```jsx
<th
aria-sort={
sortConfig?.key === columnIndex
? sortConfig.direction // 'ascending' or 'descending'
: 'none'
}
...
>
Also wrap the arrow character in a visually-hidden label: <span className="sr-only">sorted ascending</span> so it reads meaningfully rather than as a raw Unicode triangle.
| if (aValue > bValue) { | ||
| return sortConfig.direction === "ascending" ? 1 : -1; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
[INFO] Positive: useMemo is correctly applied for sort computation
Placing the sort inside useMemo with [data, sortConfig] dependencies is the correct React pattern. It avoids storing a redundant sorted copy in state (which would require careful synchronization), keeps the original data array unmutated, and ensures the sort only re-runs when its inputs actually change. The implementation is architecturally sound even though the comparator needs hardening.
Suggestion:
No change needed here — the pattern is correct and should be preserved.
| )} | ||
| </button> | ||
| </th> | ||
| ))} |
There was a problem hiding this comment.
[INFO] Positive: Sort indicator rendering is clean and well-structured
The conditional rendering of ▲/▼ only for sortConfig.key === columnIndex correctly scopes the indicator to the active column. Using flex items-center justify-between cleanly positions the indicator to the right without disrupting the DtypeBadge layout. The visual design accurately reflects the described Excel-like tri-state UX.
Suggestion:
No change needed — the rendering structure is well done.
PR ReviewFrontend
RebaseMerge commits detected — please use rebase instead of merge: SquashYour PR has 2 commits. Please squash into a single commit. How to fixgit fetch origin
git rebase -i origin/main # mark all but first commit as "squash"
git push --force-with-leaseThis comment updates automatically on each push. |
2 participants
Interactive Column Headers: When you click on any column header button, it will now sort the table based on the data in that column.
Tri-State Sorting: Just like Excel, the sorting cycles through three states when clicked:
Click 1: Sorts Ascending (indicated by an ▲ upward arrow)
Click 2: Sorts Descending (indicated by a ▼ downward arrow)
Click 3: Removes sorting, returning the table to its original, unsorted state.
Smart Sorting: The sorting logic is smart enough to detect whether the cell contains numbers or plain string text and will sort them naturally (e.g. 2 comes before 10 numerically instead of alphabetically).
State Management: Sorting is now handled as an efficient useMemo operation overlaid on your original data set, so your original row indexes and mappings are perfectly preserved when sorting is turned off!
You can test this out in your UI by clicking any of the column names at the top of the table.
fixes: #200
Screen.Recording.2026-03-14.002519.mp4