Skip to content

fix: 🐛 table column resize nan when headers reordered#908

Merged
punkbit merged 15 commits intomainfrom
fix/table-col-resize-nan-when-headers-reordered
Mar 11, 2026
Merged

fix: 🐛 table column resize nan when headers reordered#908
punkbit merged 15 commits intomainfrom
fix/table-col-resize-nan-when-headers-reordered

Conversation

@punkbit
Copy link
Copy Markdown
Collaborator

@punkbit punkbit commented Mar 11, 2026

Why?

The useLayoutEffect watching headers.length wasn't triggered when headers were reordered (e.g., in sysadmin EntitiesTable with column selection), causing Column widths to be misaligned after reordering, NaN values appearing during resize operations and an incorrect null check that was always true.

How?

  • Use Map (string, number) keyed by column label instead of array indexed by position
  • Track columnLabel and nextColumnLabel in resize state instead of numeric indices
  • Add proper guards for header label type checking
  • Watch complete headers array in dependencies instead of just the length
  • Add new story with reordering control
  • Add new test with reordering

Preview?

demo-table-reorder-resize-fix.mov

punkbit added 5 commits March 10, 2026 21:00
… but keep the same length. The useLayoutEffect was only watching headers.length, so column widths weren't recalculated when headers were reordered (as in EntitiesTable with column selection). Also fixed incorrect null check (-1 !== null) and added missing isSelectable dependencies to resize callbacks.
Replace fragile array-index based column width tracking with a Map using
header labels as keys. This eliminates index synchronization bugs when
columns are reordered, isSelectable changes, or new column types are added.
- Change columnWidths from number[] to Map<string, number>
- Store column labels in ResizeState instead of indices
- Remove isSelectable dependencies from resize handlers
- Position-agnostic width lookups prevent NaN on column reordering"
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 11, 2026

🦋 Changeset detected

Latest commit: 0c763f6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@punkbit punkbit requested a review from rndD March 11, 2026 09:23
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 11, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 11, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 11, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 11, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 11, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 11, 2026

Claude finished @punkbit's task in 1m 21s —— View job


PR Review

  • Gather context
  • Read changed files
  • Analyze and post review

The fix is well-reasoned and correctly solves the root cause. Switching from position-indexed arrays to a label-keyed Map is the right approach — reordering no longer invalidates width lookups. The two-phase reset (headerKey memo → useEffect nulls the map → useLayoutEffect re-measures) is clean and correct. Tests and story added. Nice work overall! 🙌

💡 Suggestions (non-blocking)

  • Table.tsx:820headerKey separator collision: Joining labels with "," means two header sets can produce identical keys: ["A,B", "C"]"A,B,C" and ["A", "B,C"]"A,B,C". A separator that can't appear in column names (e.g. "\0") or using JSON.stringify(labels) would be more robust. Unlikely to matter in practice but easy to tighten. Fix this →

  • Table.tsx:814headers dep in useLayoutEffect is redundant: columnWidths is already a dep, and the null-reset from the useEffect already triggers re-execution. Adding headers to this effect's deps doesn't add correctness and is slightly confusing alongside the intentional headerKey-based reset mechanism.

  • Table.stories.tsx:606 — Story name vs. behavior mismatch: ResizableColumnsWithReordering controls toggle column visibility, not order. Since the PR description mentions the actual use case is column selection (show/hide), consider naming it ResizableColumnsWithColumnSelection for clarity.

  • Non-string label fallback is still position-dependent (Table.tsx:805, 909, 978): The __index_N key for non-string label values still uses the current position index. Reordering columns with non-string labels would still produce stale lookups. This is an acceptable edge case — just worth a comment noting the limitation.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 11, 2026

Claude finished @punkbit's task in 1m 29s —— View job


PR Review

  • Gather context
  • Read changed files
  • Analyze and post review

The fix is well-reasoned and correctly solves the root cause. Switching from position-indexed arrays to a label-keyed Map is the right approach — reordering no longer invalidates width lookups. The two-phase reset (headerKey memo → useEffect nulls the map → useLayoutEffect re-measures) is clean and correct. The follow-up commits cleanly addressed several suggestions from the prior review: redundant headers dep removed, dead guards removed, story renamed, key simplified. Nice work! 🙌

💡 Suggestions (non-blocking)

  • Table.tsx:820headerKey separator collision: Joining labels with "," means two header sets can produce identical keys: ["A,B", "C"]"A,B,C" and ["A", "B,C"]"A,B,C", so a reorder wouldn't be detected. A separator that can't appear in column names (e.g. "\0") or using JSON.stringify(labels) would be more robust. Unlikely to matter in practice but easy to tighten. Fix this →

  • Table.tsx:805, 909, 978 — Duplicate string label edge case: If two columns share the same string label, the Map will silently overwrite the first width with the second, causing both columns to share one entry. Since label uniqueness isn't enforced by the type system, worth a code comment noting the assumption (or guarding with a console.warn in dev).

  • useInputModality.ts / useInputModality.test.tsx — The function → arrow function refactor is unrelated to the PR goal. Totally fine to include but worth splitting into a separate commit/PR to keep the diff focused.

@workflow-authentication-public
Copy link
Copy Markdown
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-ppsrxzk80-clickhouse.vercel.app

Built from commit: e98477c039e101c7bd3e2bf7c11a67dd44ad41f2

@punkbit punkbit merged commit beb4fc4 into main Mar 11, 2026
8 checks passed
@punkbit punkbit deleted the fix/table-col-resize-nan-when-headers-reordered branch March 11, 2026 09:59
punkbit added a commit that referenced this pull request Mar 11, 2026
* fix: 🐛 Fixes resize breaking when column headers change order/content but keep the same length. The useLayoutEffect was only watching headers.length, so column widths weren't recalculated when headers were reordered (as in EntitiesTable with column selection). Also fixed incorrect null check (-1 !== null) and added missing isSelectable dependencies to resize callbacks.

* refactor: Refactor Table column resize to use Map-based width storage

Replace fragile array-index based column width tracking with a Map using
header labels as keys. This eliminates index synchronization bugs when
columns are reordered, isSelectable changes, or new column types are added.
- Change columnWidths from number[] to Map<string, number>
- Store column labels in ResizeState instead of indices
- Remove isSelectable dependencies from resize handlers
- Position-agnostic width lookups prevent NaN on column reordering"

* chore: 🤖 add story with reordering control to test NaN fix

* test: 💍 add story for testing reordering NaN

* chore: 🤖 add changeset

* chore: 🤖 lint amends

* chore: 🤖 format

* chore: 🤖 reset when headers change

* fix: 🐛 NaN explicit intent, remove misleading key

* fix: 🐛 remove width (unused)

* fix: 🐛 remove dead guards

* fix: 🐛 remove dead guards

* fix: 🐛 inline dep

* fix: 🐛 remove headers from deps

* refactor: 💡 rename story
punkbit added a commit that referenced this pull request Mar 24, 2026
…937)

* chore: 🤖 on v0.1.0.-rc.70 (updated with latest, dismissed previous history to avoid resolving conflicts and put back latest knowne rc version)

* fix: 🐛 latest is actually *-rc.71

* chore: 🤖 release v0.1.0-rc.72 (rc) (#889)

Co-authored-by: punkbit <punkbit@users.noreply.github.com>

* chore: 🤖 release v0.1.0-rc.73 (rc) (#893)

Co-authored-by: punkbit <punkbit@users.noreply.github.com>

* chore: 🤖 release v0.1.0-rc.74 (rc) (#899)

Co-authored-by: punkbit <punkbit@users.noreply.github.com>

* refactor: 💡 exposed asset names (revision 2) (#900)

* refactor: 💡 rename assets

* refactor: 💡 prefer planetscale

* chore: 🤖 add changeset

* refactor: 💡 normalise brand names

* refactor: 💡 normalise names

* chore: 🤖 release v0.1.0-rc.75 (rc) (#901)

Co-authored-by: punkbit <punkbit@users.noreply.github.com>

* fix: 🐛  table column resize nan when headers reordered (#908)

* fix: 🐛 Fixes resize breaking when column headers change order/content but keep the same length. The useLayoutEffect was only watching headers.length, so column widths weren't recalculated when headers were reordered (as in EntitiesTable with column selection). Also fixed incorrect null check (-1 !== null) and added missing isSelectable dependencies to resize callbacks.

* refactor: Refactor Table column resize to use Map-based width storage

Replace fragile array-index based column width tracking with a Map using
header labels as keys. This eliminates index synchronization bugs when
columns are reordered, isSelectable changes, or new column types are added.
- Change columnWidths from number[] to Map<string, number>
- Store column labels in ResizeState instead of indices
- Remove isSelectable dependencies from resize handlers
- Position-agnostic width lookups prevent NaN on column reordering"

* chore: 🤖 add story with reordering control to test NaN fix

* test: 💍 add story for testing reordering NaN

* chore: 🤖 add changeset

* chore: 🤖 lint amends

* chore: 🤖 format

* chore: 🤖 reset when headers change

* fix: 🐛 NaN explicit intent, remove misleading key

* fix: 🐛 remove width (unused)

* fix: 🐛 remove dead guards

* fix: 🐛 remove dead guards

* fix: 🐛 inline dep

* fix: 🐛 remove headers from deps

* refactor: 💡 rename story

* chore: 🤖 release v0.1.0-rc.76 (rc) (#909)

Co-authored-by: punkbit <punkbit@users.noreply.github.com>

* chore: 🤖 release v0.1.0-rc.77 (rc) (#914)

Co-authored-by: punkbit <punkbit@users.noreply.github.com>

* chore: 🤖 release v0.1.0-rc.78 (rc) (#921)

Co-authored-by: punkbit <punkbit@users.noreply.github.com>

* chore: 🤖 remove a user's personal text editor (vscode) directory (#920)

* chore: 🤖 remove user personal text-editor directory

* chore: 🤖 add to .gitignore

* chore: 🤖 split slack notifications (general vs pull requests)#919

* chore: 🤖 release v0.1.0-rc.79 (rc) (#927)

Co-authored-by: punkbit <punkbit@users.noreply.github.com>

* chore: 🤖 github actions maintenance (nodejs v20 action deprecation) (#922)

* chore: 🤖 update step actions to use latest's

* chore: 🤖 add deprecation note

* fix: 🐛  missing imports file extension for dayjs in ESM environments (#933)

* refactor: 💡 move days to date utils to help enforce extension for ESM consumer imports

* chore: 🤖 add note and TODO

* chore: 🤖 add changeset

* chore: 🤖 release v0.1.0-rc.80 (rc) (#934)

Co-authored-by: punkbit <236752+punkbit@users.noreply.github.com>

* chore: 🤖 release v0.1.0 (stable) (#935)

Co-authored-by: punkbit <236752+punkbit@users.noreply.github.com>

---------

Co-authored-by: workflow-authentication-public[bot] <212800359+workflow-authentication-public[bot]@users.noreply.github.com>
Co-authored-by: punkbit <punkbit@users.noreply.github.com>
Co-authored-by: punkbit <236752+punkbit@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants