Skip to content

fix: treat frozenRow: 0 as no freeze (input clamp) - #1266

Merged
6pac merged 2 commits into
masterfrom
fix/frozen-row-zero-clamp
Aug 9, 2026
Merged

fix: treat frozenRow: 0 as no freeze (input clamp)#1266
6pac merged 2 commits into
masterfrom
fix/frozen-row-zero-clamp

Conversation

@6pac-ai

@6pac-ai 6pac-ai commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

The bug (Q23/Q24 in discussion #1247)

frozenRow is a count, but setFrozenOptions gated on frozenRow > -1, so frozenRow: 0 activated the full frozen-row machinery around an empty band:

  • hasFrozenRows = true, split panes shown — a visible empty band strip;
  • actualFrozenRow computed to 0, so every row rendered in the bottom canvas in top mode;
  • with frozenBottom: true, actualFrozenRow = dataLength — the whole body rendered in the top canvas while bottom-mode offset math measured it (the degenerate case behind several downstream contortions).

The fix

One-line clamp: the gate is now frozenRow > 0. Zero pinned rows is no freeze. A repo-wide grep (examples/, cypress/, src/) confirms nothing passes frozenRow: 0 today, so no exercised behavior changes.

Repro

new Slick.Grid('#c', data, cols, { frozenRow: 0 }) → pre-fix: all rows live in .grid-canvas-bottom under a visible empty band; post-fix: identical to an unfrozen grid.

Test

cypress/e2e/quirk-frozen-row-zero.cy.tsself-hosting (the two-variant harness is served from within the spec via cy.intercept; the spec does not depend on any example page). Asserts both frozenRow: 0 variants are observably identical to an unfrozen grid (all rows in the top canvas, no bottom pane visible). Verified to fail pre-fix (top=0 bottom=12, phantom panes visible) and pass with the fix; frozen suites ran green as a regression gate.

⚠️ Temporary example page

examples/example-quirk-frozen-row-zero.html is a human-review repro (three grids: both variants + an unfrozen control) intended to be deleted before merge. The cypress test is fully independent of it.

(Part of the quirks-triage series — remaining-items wave: see discussion #1247.)

frozenRow is a COUNT, but setFrozenOptions gated on frozenRow > -1, so
frozenRow: 0 activated the full frozen-row machinery around an EMPTY band:
hasFrozenRows true, a visible empty band strip, every row routed to the
BOTTOM canvas in top mode (actualFrozenRow computed to 0), and with
frozenBottom: true the whole body rendered in the top canvas while
bottom-mode offset math measured it.

The gate is now frozenRow > 0: zero pinned rows IS no freeze. Nothing
meaningful is lost, and a whole degenerate family disappears from every
frozen-row code path. A repo-wide grep confirms nothing passes
frozenRow: 0 today, so no existing behavior changes.

Adds a permanent SELF-HOSTING regression test (cypress/e2e/quirk-frozen-
row-zero.cy.ts - the harness is served from within the spec via
cy.intercept, no dependency on any example page) covering both variants.
Verified to fail pre-fix (12/12 rows in the bottom canvas for the top
variant; phantom bottom panes visible in both) and pass with the fix;
frozen suites ran green as a regression gate.

NOTE: examples/example-quirk-frozen-row-zero.html is a TEMPORARY human-
review repro page (with an unfrozen control grid for comparison) intended
to be deleted before merge - the cypress test does not depend on it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ghiscoding

ghiscoding commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

@6pac Claude keeps adding so much unwanted comments. We can probably avoid this by creating a Claude.md file in the repo root or a SKILLS.md file. You can ask Claude to create it too.

For example, you create Claude.md in the root and put this code, then anytime Claude would run, it would read this file (and/or skill files)

# Claude Instructions

- Make the smallest correct code change.
- Do not add comments that explain a fix, bug, or change you made.
- Code comments are okay only when they explain the code’s behavior, intent, or structure.
- Preserve existing comment style and only edit comments when necessary for correctness.
- Keep output limited to the requested code.

I can do it for you, but I'm trying to teach you some basics as I learn it myself... We can also expand this Claude file to describe what the project is and provide some basics info so that we don't have to provide that info every time. But again, these file are better created by AI, AI is good at writing code for AI (just provide some guidance)

@6pac

6pac commented Aug 7, 2026

Copy link
Copy Markdown
Owner

It's OK, my root Claude.md is already about 9 pages long with instructions for everything. Plus I have a bunch of other project-specific files Clause has written as instructions to itself, and I've tweaked.   If you don't give it enough specific instructions, its a disaster.

The reason I haven't done this tweak is because sometimes I think the comments are good, and other times they are noise.
I haven't worked out how I can tell it to include the good ones.

Perhaps if it's a bug fix, no comments, but if the comment is explaining a technical issue that the developer might miss, it's fine.
But I don't think I'd want to omit comments altogether. In general, comments are good as long as they are concise.

@ghiscoding

Copy link
Copy Markdown
Collaborator

well I think what the AI gave me above is valid and could help with that, especially this line
- Do not add comments that explain a fix, bug, or change you made.

@6pac

6pac commented Aug 7, 2026

Copy link
Copy Markdown
Owner

yeah have added a few of those lines.

@ghiscoding

ghiscoding commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

I tried replicating this PR but I have a few unit tests that failed on my side, so I'll double check the change at work, but now it's time for bed (almost 1am so). If you open a few PRs, I can probably check them all tomorrow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ghiscoding

ghiscoding commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

@6pac ok I was able to replicate this PR and fix my unit tests, AI confirmed it's also valid on my side. So it's good to merge now. Thanks

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.

3 participants