Skip to content

added sorting feature on button click #202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
schourasia750 wants to merge 2 commits into
base: main
Choose a base branch
from
+58 −5
Open

added sorting feature on button click #202

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4fe098
added sorting feature
schourasia750 Mar 13, 2026
3c0b888
Merge branch 'main' into sort-on-click
schourasia750 Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 58 additions & 5 deletions dataloom-frontend/src/Components/Table.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import ContextMenu from "./ContextMenu";
import { useContextMenu } from "../hooks/useContextMenu";
import { useState, useEffect } from "react";
import { useState, useEffect, useMemo } from "react";
import { transformProject } from "../api";
import { useProjectContext } from "../context/ProjectContext";
import {
Expand Down Expand Up @@ -36,6 +36,7 @@ const Table = ({ projectId, data: externalData }) => {
const [columns, setColumns] = useState([]);
const [editingCell, setEditingCell] = useState(null);
const [editValue, setEditValue] = useState("");
const [sortConfig, setSortConfig] = useState(null);
Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

const { isOpen, position, contextData, open, close } = useContextMenu();

const [inputConfig, setInputConfig] = useState(null);
Expand Down Expand Up @@ -63,6 +64,48 @@ const Table = ({ projectId, data: externalData }) => {
updateData(columns, rows, newDtypes);
};

const handleSort = (columnIndex) => {
let direction = "ascending";
if (sortConfig && sortConfig.key === columnIndex && sortConfig.direction === "ascending") {
direction = "descending";
} else if (sortConfig && sortConfig.key === columnIndex && sortConfig.direction === "descending") {
direction = null;
}

if (direction) {
setSortConfig({ key: columnIndex, direction });
} else {
setSortConfig(null);
Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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);
  }
};

}
};

const sortedData = useMemo(() => {
let sortableData = [...data];
if (sortConfig !== null) {
sortableData.sort((a, b) => {
let aValue = a[sortConfig.key];
let bValue = b[sortConfig.key];

// Attempt to parse as numbers for natural numerical sorting
const numA = Number(aValue);
const numB = Number(bValue);
if (aValue !== "" && bValue !== "" && !isNaN(numA) && !isNaN(numB)) {
aValue = numA;
bValue = numB;
}

if (aValue < bValue) {
return sortConfig.direction === "ascending" ? -1 : 1;
}
Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] String comparison is not locale-aware

When values are not numeric, the comparator uses raw < / > operators on strings. JavaScript string comparison uses Unicode code point order, not locale-aware collation. For example, "ä" > "z" is true in code-point order but should be false alphabetically in most locales. This is especially problematic for tables containing non-ASCII names, city names, or any international text.

Suggestion:

Replace raw `<`/`>` string comparison with `localeCompare`:

```js
// Replace the if (aValue < bValue) / if (aValue > bValue) block with:
const cmp = (typeof aValue === "string" && typeof bValue === "string")
  ? aValue.localeCompare(bValue)
  : (aValue < bValue ? -1 : aValue > bValue ? 1 : 0);
return sortConfig.direction === "ascending" ? cmp : -cmp;

if (aValue > bValue) {
return sortConfig.direction === "ascending" ? 1 : -1;
}
return 0;
Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 Number and a String, 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;
});

Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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]);

Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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('▼');
});

Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

});
}
return sortableData;
}, [data, sortConfig]);

const handleAddRow = async (index) => {
try {
const response = await transformProject(projectId, {
Expand Down Expand Up @@ -231,17 +274,27 @@ const Table = ({ projectId, data: externalData }) => {
className="py-1.5 px-3 border-b border-gray-200 text-left text-xs font-medium text-gray-500 uppercase tracking-wider"
onContextMenu={(e) => open(e, { type: "column", columnIndex })}
>
<button className="w-full text-left text-gray-500 hover:text-gray-700 hover:bg-gray-100 py-0.5 px-1.5 rounded-md transition-colors duration-150">
{column}
{column !== "S.No." && <DtypeBadge dtype={dtypes[column]} />}
<button
onClick={() => handleSort(columnIndex)}
className="w-full flex items-center justify-between text-left text-gray-500 hover:text-gray-700 hover:bg-gray-100 py-0.5 px-1.5 rounded-md transition-colors duration-150"
>
<span className="flex items-center gap-1.5">
{column}
{column !== "S.No." && <DtypeBadge dtype={dtypes[column]} />}
</span>
{sortConfig && sortConfig.key === columnIndex && (
<span className="text-[10px] ml-1">
{sortConfig.direction === "ascending" ? "▲" : "▼"}
</span>
)}
</button>
</th>
))}
Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

</tr>
</thead>
Copy link
Copy Markdown
Member

@ivantha ivantha Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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>
))}


<tbody>
{data.map((row, rowIndex) => (
{sortedData.map((row, rowIndex) => (
<tr
key={rowIndex}
className="border-b border-gray-100 hover:bg-gray-50 transition-colors duration-150"
Expand Down
Loading