feat(grid)!: replace frozen panes with pinning and sticky docking - #1302
Conversation
|
@6pac I think you should close your previous PR #1238 since this is the new approach that includes Pinning and Sticky. Please note that I would ask if you can ask Claude to audit and verify the entire PR to detect any possible problem, there's a progress file written by AI and read by AI to keep it focused, you should tell Claude to read that file Side note, with the code now you can at least start testing it out (including the new Also important, the +/- 1000px that we carried from the original SlickGrid lib is officially gone in this PR, I'm pretty sure that it was in place to support legacy IE browser back in the day but there's no reason to keep such old code and approach that caused alignment issues when implementing this PR and so I told the AI to remove it all, which is a lot easier to read the DOM now |
|
OK, Claude Fable is done with the evaluation. There's a lot of it! |
Evaluation of 6pac/SlickGrid PR #1302 — "feat(grid)!: replace frozen panes with pinning and sticky docking"
1. VerdictNot mergeable as it stands. The architecture is sound and the big-ticket claims (one viewport, one row per data row with left/centre/right regions, one horizontal scroll owner, Top issues (details in §4):
Nothing found requires abandoning the design. Most items are local fixes; the largest are the row-reference model (§4.1 group B) and the hit-testing/wheel routing for docked content (group D). 2. What was verified and how
Not done: no unit tests exist in this repository (the 3. Cypress results (local run, Windows)
The one failure is
Otherwise the suite is green locally, which matches CI. Note that green CI does not cover findings 1–7 above: the spreadsheet spec asserts the wrong pinned count, no spec drags a range over pinned cells, no spec uses non-contiguous row pins, 4. FindingsSeverity: Blocker = wrong behaviour for ordinary configurations or silent regression for existing users; High = wrong behaviour for documented pinning/sticky configurations; Medium = correctness edge cases, performance, API hygiene; Low/Nit = polish. Status: Confirmed (re-verified in source or browser), Observed (seen in the browser), Reasoned (reviewer, from code only). 4.1 Blockers and HighA. Column pinning referencesA1. Blocker — Numeric index references are also matched against A2. High — Numeric shorthands count hidden columns; B. Row references (pinned and sticky rows)B1. High — B2. High — The id→index cache is never invalidated on a count-preserving DataView sort/filter. Confirmed in B3. High — Custom DataView B4. High — Bottom-pinned rows keep their natural slot in the canvas. Reasoned ( B5. High — Non-contiguous top pins break hit-testing and active-cell tracking. Reasoned. Unpinned rows render at B6. High — Sticky-row thresholds ignore the permanent top band and subtract the bottom band twice. Confirmed in B7. High — B8. Medium — C. Regressions for grids that do not use pinning at allC1. Blocker — Every C2. Blocker — Vertical wheel now scrolls one row per notch on every grid. Confirmed by diff. C3. Blocker — Ctrl/Meta+drag multi-selection regressed. Confirmed by diff. Base C4. High — Undocumented rename C5. High — C6. High — Plain grids pay O(columns) per rendered cell in C7. Medium — Keyboard/focus contract changes not listed as breaking. Reasoned. Focus sinks moved outside the container with C8. Blocker — D. Interaction with docked contentD1. High — D2. High — Wheel over a pinned/sticky row scrolls the page. Reasoned. D3. High — Column reorder throws when a sticky column is docked (LTR proxy path). Reasoned. D4. High — Forwarded chrome D5. Medium — Docked rows outside the vertical rendered range never receive new centre cells on horizontal scroll, and in-range docked rows are never cell-cleaned. Reasoned ( D6. Medium — D7. Medium — Pinning cannot be switched off at runtime; the proxy scroller and chrome regions are created lazily but never removed. Confirmed by reading D8. Medium — Lazy docking activation empties header/header-row/footer without firing the D9. Medium — Cross-band colspan fragments freeze the host's E. Legacy surface and claimsE1. High — Legacy frozen options remain declared with live JSDoc; the Grid Menu still branches on them. Confirmed. E2. High — PR description and progress file claim things that do not exist in this repository. Confirmed by grep/diff.
E3. High — Test integrity. Confirmed by diff.
4.2 MediumM1. Performance on the per-scroll path. Reasoned by two reviewers (consistent with each other):
M2. M3. Public API drift not listed as breaking. Reasoned/confirmed by call-site diff:
M4. slickgrid-universal leakage into public types. Confirmed in the model diff. M5. Dead file that ships as an empty bundle. Confirmed. M6. Docked-row overlay artifact with zero-width scrollbars. Observed only in headless Chrome with scrollbars hidden (which is what overlay-scrollbar platforms such as macOS report): the last digit of each docked sticky row's rightmost cell is painted a second time, offset down-right, in the strip between the overlay clip and the grid border ( M7. Small controller/geometry issues. M8. Examples and docs. 4.3 Low / Nits
5. Verified sound
6. Recommended actions before merge
7. Reproducing the confirmed findingsAll steps use the repository's own scripts on a clean checkout of the PR branch (
|
|
Hang on a minute, there's quite a bit of stuff in there that's specific to my computer and its environment. I'm just gonna remove that and repost. |
|
OK the evaluation has been updated |
|
wow that is a lot.... providing this to Codex, and we'll see what it's able to fix. Just curious, do you also have access to Fable 5.1? Seems like an improvement, probably more expensive though Side note I also fixed colspan just now which can now spread on both side of the column pinning and also updated data Grouping which also spreads its grouping title (see above). |
|
Yep, this review was done with Fable 5.1. It did take up about 35% of my weekly quota though! Which is fine, I usually don't use more than about 30% of it anyway. |
| const queueMicrotaskPolyfill = (callback: () => void) => typeof queueMicrotask === 'function' ? queueMicrotask(callback) : setTimeout(callback, 0); | ||
| const destroyAllElementProps = (_target: object) => undefined; | ||
| const destroyAllElementProps = (target: object): void => { | ||
| const elementProperties = [ |
There was a problem hiding this comment.
not really sure why it added all of these, this seems very overkill. Shouldn't it be able to destroy and remove whatever it needs without us having to name all functions? I assume it came from Claude report
|
@6pac ok the AI is done with the audit report, the remaining things it said was basically verifying the UI myself... can you do a final audit to make sure it fixed everything. Also, can you ask it to see if it there's any areas to decrease LOC (I usually ask the AI if it's the most minimalist it can do without regressing). I'm especially concerned about the comment I left just above, I don't understand the point of listing all function names to loop and and destroy (this seems ridiculous and not minimalist to do this way). If there's anything else, I'd prefer you let it fix the rest... having a different AI model to double-check is actually a very good exercise, this will be used for the next few years, so better be good :) ahh wait, last commit caused a few test failures, let me fix them
|
|
@6pac ok I'm done and fixed Cypress failures, so would you mind addressing what I wrote above |
|
In the middle of a very busy workday, but I'll point Claude at it and see how it goes. The destroyAllElementProps issue looks like a reversion rather than something Claude suggested (it found an issue with that function, but that's not the suggestion it made), from what I can see. I'll query it. |
|
ah yeah it might have been my old code actually, but I think we can remove that or lighten it at least. Thanks. There's no rush on it, need to make sure that we cover all angles and that the UI/UX works for you :) |
|
Here's the eval. Let me know if you're happy to go ahead and I can get Fable to do the final commits. |
…hidden-column scan Follows the audit posted on #1302. applyDockingProxyScrollOffsets() wrote an inline transform to every permanently pinned header, filter and footer cell on each horizontal scroll; the proxy-mode stylesheet rules already set those transforms from --slick-docking-scroll-left with !important, so the writes had no effect. scrollToX() now publishes the variable alone. updateColumnPositionCaches() rescanned a hidden block once per hidden column (25 million reads of `hidden` for 5,000 columns, 197 ms on every step of a column-resize drag); each block is resolved once. The class-keyed measurement cache in applyDockingToColumnChrome() never hit, because filter and footer cells carry per-column l/r classes, and a working one saved nothing measurable, so it is gone. paneBottomH and viewportBottomH were frozen-pane remnants with no readers. appendColspanFragments() passes the row's cached regions. The column-group examples skip their group-header sync when scrollLeft is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@ghiscoding I checked each item of the audit against this repository and measured it in the browser (Chrome, a local build of Performance items
LOC items
CI on |
|
@6pac-ai so my AI on my Slickgrid-Universal on ghiscoding/slickgrid-universal#2782, found a few divergences from both repos and it replied with this report, can you please investigate and apply changes if necessary (in the report, when it says "fork", it means your SlickGrid project): Here is an updated comparison for the SlickGrid fork. Compare fork Fork changes worth investigating
Reproduce before porting
Differences already addressed in UniversalPlease do not report these as outstanding Universal gaps unless a fresh comparison shows the current code still differs:
Keep the fork’s current implementationDo not copy Universal’s empty- The |
Follows the comparison ghiscoding posted on #1302. Pins whose widths add up to the available width are rejected, since they leave the centre band no width. A rejected setColumns() no longer records the request's columns in the saved pinning state, where a flag carried only by the rejected request could later come back as a column's original pin. setColumnPinning() records every reference as a column id, so the pins follow their columns through a reorder and the changed column can be unpinned afterwards. syncDockingChromeRegions() appends chrome elements only when their order differs from the sorted one. The scrollLeft clamp, two early-return guards and one statement order are aligned so the methods read the same in both projects. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…first layout resizeCanvas() re-applied the docked chrome geometry only when the docking layout changed, so after the container changed width the right-pinned header, filter and footer cells stayed at the old edge while the body moved. It now re-applies it on every resize, as Universal does. With implicit initialisation the headers were built before applyRTL() set the container's own direction, so an rtl grid on a left-to-right page laid its trailing band out at the wrong edge; the direction is applied first. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…acts spec The resize and rtl cases compared the docked chrome with the viewport's outer box, which includes the vertical scrollbar, so a correctly placed band read as one scrollbar width off on Linux. The cases now compare the chrome with the band's body cells and only check that the band itself lies within the scrollbar of the viewport edge. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Fork vs Slickgrid-Universal (round 12): found and fixedghiscoding's request on #1302 (issuecomment-5896068728, 2026-09-29): verify a Universal-side AI Sources. The clone at Tool. Evidence. In Method-level result
The report's four fork items — confirmed and fixed
Found by the comparison — fixed
Items checked and left as they are
Verification
Change set (committed and pushed 2026-09-30)
|
Fork vs Slickgrid-Universal: open items after the comparisonFollow-up to the comparison report on #1302 (fork Questions to settle1. Numeric boundary semantics: visible columns or all columns?
The fork keeps the pinned count stable when a pinned column is hidden; Universal keeps the 2. Grid Menu with right pins on a 0 px scrollbar: port the allowance?With a Grid Menu, a header row and two right pins, the fork's menu button (16 px) sits on top of For UniversalWhich side should change: Universal, for each of these. The fork's version carries a spec.
Not testable on macOS
Deferred, cosmetic
|
|
@ghiscoding please comment on the most recent post when you have time. |
So it seems to agree with most of the things including the section "For Universal", so my AI is currently executing those changes and I will report back when it's done Side note, I have no MacOS and don't know anyone who could help testing this either, so I can't really help answering these questions. I would probably go with the AI's suggestions... What I have though is Windows for Work and personal but I also converted my personal laptop to Linux and so I'm testing on that too and that's probably why Linux came in the discussion here and there because that's my main machine at home |
|
@6pac-ai so here's the final update from the AI after updates
... so perhaps we're finally done? hmm, I think I also need to make an update and not activate docking for empty pinning |
|
Ok great. I'm running these sessions on MacOS, so it's not asking for MacOS, it's asking for not-MacOS! |
|
it seems that there's still this one to make a decision:
my AI replied with: I meant a measurement difference that hasn’t shown to cause a bug yet. In the current code, Universal’s With overlay scrollbars, those measurements can be the same. With a classic scrollbar, they may differ by the scrollbar gutter, which could make right-pinned headers, filters, or footers end a few pixels away from the body cells. I meant we should compare those edges with a real vertical scrollbar before changing either implementation; the report didn’t establish that either side is wrong. |
|
Just noting also that I'm also cooking up a resolution to the Auto Column Sizing dilemma. It's more complex than I initially thought, so I'm still working through the issues before I push any proposed PR. I have created a simply bugfix PR for the existing code to V6 that I'll merge (it's technically breaking but actually bugfixes). |
You are much more active in this area than me, so I'm happy to leave the decision to you. |
I have no idea, too tired, going to bed... you can ask Claude 😉 I also left a comment for the docs the other day, when you get a chance to look at. Thanks EDIT I was going to say, you should restrain yourself from pushing separate PRs because you're probably gonna conflicts and that's what happened haha 😄 |
|
@6pac-ai found another bug, below is the summary of what the AI changed and fixed in Universal (it updated could of SlickGrid functions) In SlickGrid Universal, removing and restoring permanent column pinning while a sticky column is docked caused pinned cells to collapse and could displace trailing pinned headers. The fix uses rendered band widths when rebuilding pinned cell and colspan-fragment offsets, and clears inactive native chrome scroll offsets before measuring pinned header placement. See You might also want to check this other commit Also found yet another small bug, fixed in Universal commit |
|
Also for the other unanswered question above:
after some testing, I can see that both repos are working perfectly fine with both approaches, so to align both repos, I asked my AI and decided to go with his suggestion which was to go with your calculation approach:
So I pushed a change to align with SlickGrid and I think that after the fixes requested above is done then I'm assuming that we are (hopefully) done |
…e resizing Ported from Slickgrid-Universal (ghiscoding/slickgrid-universal#2782, c4112428 and a490c253): - applyColumnWidths() and the colspan-fragment offsets size pinned bands from the rendered band widths, so restoring permanent pinning while a sticky column is docked no longer collapses the leading pinned cell - applyDockingToColumnChrome() clears inactive native chrome scroll offsets before measuring pinned header placement - updateCanvasWidth() re-applies column widths, chrome and row band sizes whenever the grid has docked columns, so a resize drag that leaves the canvas width unchanged keeps filter, footer, cells and row bands in step Adds quirk-pinning-restore-and-resize-geometry.cy.ts (LTR and RTL), which fails 6/6 without the fix and passes with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ported from Slickgrid-Universal f5dbce68: - share z-index on the combined pinned-left/right cells rule - drop the .slick-docking-chrome-right transform override; the grid sets that class under the same condition as slick-column-pinned-right with a zero chrome offset, so the generic pinned rule already applies the same transform - merge two identical rules in the financial report example Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@ghiscoding thanks, I've ported all three commits. Both bugs reproduced here exactly as you described, in LTR and RTL. c4112428 (restore pinning while a sticky column is docked)
a490c253 (resize when the canvas width doesn't change) One cost to be aware of: f5dbce6 (styling simplification)
I checked the removed override before dropping it. The grid sets Test
All 6 fail on the PR head and pass with the fix. On viewport width: thanks for aligning Universal to the fork's calculation. With these three ported, I don't have anything else open on my side. |
|
@6pac-ai great, I made a final audit with ChatGPT Astra in Slickgrid-Universal PR ghiscoding/slickgrid-universal#2782 and it found 2 things but I was able to only fix 1 of them, here's the audit report: I found one performance fix worth making before merge. I found no substantial LOC cleanup worth risking the current behavior.
So far the AI was able to: As for number 2, the first attempt to resolve this issue didn't yield anything better, so it was reverted. Then, I ran out of time to investigate further and so far I don't really see any perf problem in Firefox vs Brave/Chrome so perhaps that might not resolve to anything. If Claude could investigate then that could be good, but I wouldn't want more LOC added without real perf improvements (probably need benchmark to prove it)... I had a chance to continue and second pass still didn't find a smaller replacement that justifies merging. Three smaller CSS alternatives produced no consistent benefit... So in the end, it's possible that there's nothing to do for this second part and it might be ok to just abandon it Finally there's still merge conflicts to resolve from last PR... when that is all done, I think we'll be able to merge |
|
@6pac-ai I asked ChatPT Astra to investigate this comment you left:
It replied with: Yes—there’s a small, measurable improvement with a minimal fix. Changed Firefox measurements:
Overall filtering improved roughly 0.8–2.7% in the paired run, with variation between runs.
I’d keep this small optimization. Evidence is recorded in the progress file. Nothing committed or pushed. protected updateCanvasWidth() {
// ...
- if (widthChanged || forceColumnWidthsUpdate || this.hasDockedColumns()) {
+ // Row-count updates pass false; resizing docked columns still needs a refresh
+ // even when the rendered canvas width stays unchanged.
+ if (widthChanged || forceColumnWidthsUpdate || (forceColumnWidthsUpdate !== false && this.hasDockedColumns())) {
this.applyColumnWidths();
this.applyDockingToColumnChrome();
this.applyDockingDimensionsToRows();
}
}Below is what it recorded in the pinning progress file: Row-count geometry performance investigation (2026-10-02)
|
…changed updateCanvasWidth() refreshes column widths, chrome and row bands whenever the grid has docked columns, and updateRowCount() calls it on every row-count change, for example on each filter keystroke. updateRowCount() passes an explicit false; that now means "no refresh unless a canvas width changed". Calls without an argument, as during a column-resize drag, still refresh a docked grid. Proposed by the audit on #1302; measured here at 0.7 ms (Firefox) and 2.1 ms (Chrome) per row-count update on a pinned grid, with identical docked geometry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
resolveDockingRowIndex() cached only the references it could resolve, so a pinned or sticky row id that is not in the data was searched for again on every vertical scroll: a full findIndex() over a plain array, or a getRowById() call on a DataView. An unresolved reference is now cached as undefined until the existing invalidation paths clear the cache, as in Slickgrid-Universal 8b6d8d1. The spec counts the searches during 60 scroll steps (120 before, none now) and checks that the pins resolve once their rows exist and the rows are invalidated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@ghiscoding four things. The conflicts are resolved. Row-count refresh: adopted as you proposed,
The docked geometry was identical in every filter state I compared, including one that leaves two rows so the vertical scrollbar goes away and comes back, and the round-13 geometry spec with its mid-drag cases still passes. I kept your two comment lines so the method reads the same in both projects. Missing row ids: ported, Firefox and the inherited scroll offset: measured, and it is real. Firefox 157 and Chrome 154 on macOS, a pinned grid scrolled horizontally, each step timed with a forced style and layout flush:
Driven one step per frame, the larger grid ran at 33 ms per frame in Firefox as it is now and at 16–17 ms with targeted writes. That is about 3.6 µs per grid element per scroll step in Firefox and 1.6 µs in Chrome, so it is not Firefox-only; Firefox is about twice as bad. A single stylesheet rule does not help, because Firefox restyles everything when a rule's declaration changes. Vertical scrolling does not pay this cost. This one is on the fork: until CI on |
|
@6pac I think we finally reached the end of the process and I believe it's finally time to hit the merge button (make sure it's "Squash and merge" to merge it as 1 commit). Even if this was quite a long process, I think it was very interesting to get 2 different AIs working on it, it covers more grounds. Please note that Claude left some pending tasks as follow-up PRs (not sure if it documented them in the pinning progress file), so that would have to come next I guess Side note, I'm writing again as a reminder in case you didn't see it earlier. I left a comment for the docs the other day, when you get a chance to look at it. Thanks |

supersede #1238
fixes #410
fixes #443
fixes #739
fixes #1219
Summary
Introduce a single-viewport docking architecture for permanent pinned columns/rows and
scroll-activated sticky columns/rows.
This is an intentional v6 breaking change. The previous multi-pane frozen implementation has
been removed from the runtime and replaced with one virtualized body viewport, one vertical
scroll owner, one horizontal scroll owner, and stable per-row left/center/right cell regions.
Why
The legacy frozen-pane implementation required multiple synchronized panes and scroll
containers. This increased complexity around scrolling, resizing, virtualization, editing,
grouping, and framework integrations.
The new architecture provides a simpler and more predictable model:
Unlike the previous freeze-until-column/row behavior, users can now pin individual columns or
rows independently. For example, columns 0 and 2 can be pinned while column 1 remains in the
center region.
Changes
GridOption.pinningsupport for:columns.left/columns.right;rows.top/rows.bottom.Column.pinnedandCurrentColumn.pinningstate support.Column.stickyandGridOption.stickyRowsfor scroll-activated docking.DockingControllerfor permanent and sticky column/row resolution.conveyor/clampoverflow strategies.
fragments.
continue to work as before without pinning or sticky columns; RTL combined with docking is
a known gap (see Follow-up work).
example-sticky-financial-report.htmlgrid.setColumnPinning(columnId, side)andgrid.setColumnStickiness(columnId, side)so an application can build its own pinning menu commands. This repository has no Header Menu
pinning command and no Grid State plugin, so neither is added here and
pinningis notserialized for you; read it back from
grid.getOptions().is intentionally not serialized.
.slick-horizontal-scrollerand.slick-vertical-scrollerselectors.scroll branches, redundant viewport/canvas aliases, and old pane CSS classes.
-1000pxheader coordinate workaround andHEADER_WIDTH_SLACK.pinning-stickyskill as implementation and documentation guidance.Breaking changes
The old frozen-pane configuration and APIs are removed.
The canonical configuration is now:
Legacy flat pinning options and temporary aliases are no longer supported.
Sticky state is not serialized because it changes with scrolling.
The old multi-pane DOM structure and pane selectors are no longer available.
Column reordering remains within each docking band; moving a column between pinned and center
bands is an explicit pinning operation.
Legacy names and theme variables are retained only as migration documentation references.
References
Ag-Grid Column Pinning was used as key concept reference for the idea of a single horizontal scroller and single vertical scroller, also for its declaration of left/center/right cell docking regions
Validation
The following checks pass on this repository:
tsc --noEmit.eslint src.npm run build:prod(bundles, declarations and Sass).editing, selection, grouping, reordering, variable-row-height and RTL specs.
The accessibility review found no pinning/sticky-specific semantic-tree or keyboard-navigation
regressions. Automated axe/WCAG integration and manual screen-reader validation are not included
in this PR.
Audit
The branch was audited in three rounds and the findings were fixed on it. Highlights of what the
audit changed: row references by dataset id (including
{ id }for numeric ids),pinning: nullclearing pinning likeundefined, the bottom band nesting sticky rows insidepermanent ones like the top band,
setColumns()validating before it mutates and returning aboolean, restoration of the
applyHtmlCode/trigger/set*Visibility/onHeaderKeyDowncontracts, removal of the fork's keyboard focus routing, and the docking hot paths taken off
O(n²) chrome lookups and per-column forced layout. Weakened Cypress assertions were restored.
The third round closed the rest of that list. A colspan that crosses a pinned boundary is now
clipped to its own band, with each continuation carrying the remainder of the content at the
right offset, so it no longer hides the columns scrolling beneath it. A centre column resized
past the right edge now scrolls the grid to follow it, instead of freezing at roughly a viewport
width with the handle off screen. Four docking measurements were corrected: a vertical scrollbar
subtracted twice in scroll-into-view, two different overflow tests deciding whether a horizontal
scrollbar exists, screen pixels mixed with layout pixels in the right-pinned chrome, and a
colspan validation that only looked at rendered rows. The horizontal scroll offset is published
once on the container rather than on every docked region, sticky cell and chrome element.
Implementation status
The single-viewport rewrite and legacy runtime cleanup are complete. This is no longer a POC
that runs alongside the old frozen-pane implementation.
Measured against the base commit
179c8ac3,src/is+6,621 / -3,888across 26 files(
+2,733net), of whichsrc/slick.grid.tsis+5,463 / -3,406; that file is now 11,558 lines.These figures exclude demos, tests and generated output.
Follow-up work
The following items are intentionally separate from the v6 implementation:
non-proxy geometry path, so a right-pinned column and an activated sticky column are placed
outside the viewport, and the docked hit-test path is skipped for RTL. RTL grids without
docking are unaffected. Documented as a limitation in
docs/pinning-sticky.md.None of these requires restoring the legacy pane architecture or changing the current pinning/sticky
runtime design.
AI / LLM assistance
(Opus 5 / Fable 5.1) for the two audit rounds and their fixes
documentation updates, test maintenance, and validation support.
Checklist
documentation, tests, and cleanup.
Print Screens