MILAB-6002: fix PlDataTable column visibility reset on re-run with changed filters#1666
Open
PaulNewling wants to merge 13 commits into
Open
MILAB-6002: fix PlDataTable column visibility reset on re-run with changed filters#1666PaulNewling wants to merge 13 commits into
PaulNewling wants to merge 13 commits into
Conversation
Persist column visibility as the user's explicit show/hide deviations from the block-defined default (pl7.app/table/visibility) instead of the absolute hidden-column set. Once a saved absolute set existed it overrode the block default, so a column whose default flipped between runs (e.g. a filter/ranking column reverting to optional) was not re-hidden and leaked visible. makeColDef and createPlDataTableV3.computeHiddenColumns now reconcile the current default with user deviations; persisted state migrates v7 -> v8 (one-time reset of custom column show/hide).
🦋 Changeset detectedLatest commit: effebc4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors column visibility persistence in PlDataTable to store user deviations (explicit show/hide overrides) from block-defined defaults rather than absolute hidden-column sets. This prevents visibility from resetting when a block re-runs with modified configurations. It also introduces a state migration from v7 to v8. The feedback highlights a defensive programming opportunity in PlAgDataTableV2.vue to use optional chaining when checking hiddenColIds.length to avoid potential runtime errors from corrupted or partially defined persisted state.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1666 +/- ##
==========================================
- Coverage 53.18% 51.38% -1.80%
==========================================
Files 270 280 +10
Lines 15729 16636 +907
Branches 3413 3628 +215
==========================================
+ Hits 8365 8548 +183
- Misses 6249 6937 +688
- Partials 1115 1151 +36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- makeColDef: guard isColumnOptional to column specs, restoring prior axis behaviour and matching computeVisibilityDeviations (greptile). - stateForReloadCompare: optional-chain hiddenColIds.length for a defensive read of persisted state (gemini). - Document that visibility deviations are column-scoped by design: axes have no table/visibility default, are always shown when displayed, and their hide/show is not persisted (greptile). - Tighten the changeset and the columnVisibility doc comment.
…idden; add tests Extract the default-vs-override precedence into a single resolveColumnHidden helper used by both the model (computeHiddenColumns) and the UI (makeColDef), so the two cannot drift — the duplication that let this bug reproduce in two places. Move the deviation derivation out of the Vue component into a pure, testable column-visibility module. Tests cover resolveColumnHidden (precedence), computeHiddenColumns (reconciliation plus preserved sorted/filtered columns), computeVisibilityDeviations (inverse derivation), and the v7->v8 migration reset, each with a MILAB-6002 regression case.
Pairs the show-override list with the existing hiddenColIds: null in createDefaultPTableParams, so fresh state spells out both override lists explicitly. shownColIds is optional on the null-source variant, so this is a clarity-only change with no behavioural effect.
- Bump @platforma-sdk/model to minor: resolveColumnHidden is a new public export. - makeColDef takes ReadonlySet lookups built once in calculateGridOptions, replacing per-column Array.includes and mirroring the model's Set-based computeHiddenColumns. - Clarify the resolveColumnHidden doc comment: only the default-vs-override precedence is shared; each caller layers preserved/forced-hidden handling on top.
Contributor
Author
Contributor
Author
The shared grid UI now stores column visibility as user deviations (hiddenColIds + shownColIds), and createPlDataTableV3 reads them. createPlDataTableV2 still read hiddenColIds as an absolute set and ignored shownColIds, so the ~29 blocks still on the V2 path over-included columns once a user changed visibility (worst case: a single show-override -> empty hide list -> every column fetched). Extract computeHiddenColumnsV2, reconciling each column's block default with the user's show/hide deviations via the shared resolveColumnHidden -- same precedence as the V3 model and the grid's makeColDef. forcedHidden is false, matching makeColDef and V2's historical behaviour (V2 never auto-hid visibility:"hidden" columns). Add createPlDataTableV2.test.ts: deviation interpretation, the empty-hiddenColIds regression, shownColIds honoured, the MILAB-6002 default-flip case, and a guard that forced-hidden columns stay visible (forcedHidden: false).
…ort deviations - deriveColumnVisibility falls back to the prior persisted deviations when the live grid reports no columns (mounting before column defs arrive, or teardown), so a not-ready grid no longer overwrites the user's show/hide with empties. - computeVisibilityDeviations sorts both deviation arrays, so a pure column reorder no longer reads as a state change and forces a spurious grid reload. - Tests cover both, and pin the known limitation where an explicit hide is absorbed when a column's default flips to optional and back.
- migrateV7toV8 copies pTableParams for the null-sourceId case instead of returning the input by reference, so migrated state no longer aliases its source. - Migration tests cover the full v6->v7->v8 chain, a sourceId with no matching cache entry, and the fresh-object guarantee. - A V3 test pins that a sorted column the user explicitly hid stays force-visible (preservation wins over the hide).
Contributor
Author
makePartialState read the module-scope gridState ref to supply the fallback "prior deviations". Pass it as an explicit `prev` parameter instead, so the function is a pure (state, api, prev) transform and each call site shows what it uses. The reload watch passes the watched gridState, making the "selfState falls back to gridState when the grid has no columns" relationship visible right at the comparison.
migrateV7toV8's null-source branch returned a shallow copy while the adjacent migrateV6toV7 passes pTableParams by reference. The copy added an inconsistency without real benefit: it only froze the top-level object, leaving the nested hiddenColIds/sorting/filters arrays aliased anyway. Both migration steps now pass pTableParams by reference. Drop the object-identity test; keep the v6->v7->v8 chain and no-matching-cache-entry coverage.
Delegate computeHiddenColumnsV2 to the shared computeHiddenColumns so the deprecated V2 path uses the same reconciliation as the V3 model and the grid's makeColDef -- removing the duplicated logic and honouring forced-hidden (visibility:"hidden") columns in V2 as they are in V3 (previously V2 left them in the visible table). Update the V2 forced-hidden test and the changeset wording to match.
hiddenColIds is a required field on columnVisibility, so the ?. and ?? 0 guard was dead code (greptile). shownColIds stays optional and keeps its guard.
Drop the vague "full and visible table handles reconcile the same way" clause -- the full table handle holds every column, so it doesn't reconcile visibility. The sentence is sharper without it.
Contributor
Author
vadimpiven
reviewed
Jun 16, 2026
| * | ||
| * Exported for unit testing. | ||
| */ | ||
| export function computeHiddenColumnsV2( |
Member
There was a problem hiding this comment.
why separate function? this is a one-liner, let's inline it
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PlDataTablestored column visibility as an absolute hidden set, which replaced theblock's default visibility once saved. When a column's default flipped between runs —
e.g. a filter/ranking column reverting to
optionalafter its filter is removed — itleaked visible (MILAB-6002).
Fix
Persist the user's explicit show/hide deviations from the block default, not the
absolute set. Untouched columns follow their current default, so a flipped default now
resolves correctly. One
resolveColumnHiddenhelper reconciles default + overrides forboth the model (
computeHiddenColumns) and the grid (makeColDef), keeping them inlockstep;
deriveColumnVisibilityis its save-path inverse, reading deviations back offthe live grid.
Two derivation edge cases are handled. An empty live grid — mounting before column defs
arrive, or tearing down — falls back to the prior saved state instead of wiping it. And
the deviation arrays are sorted, so a pure column reorder no longer reads as a state
change and forces a spurious reload.
State migrates v7 → v8, resetting visibility once (the absolute set can't be
converted without per-column defaults).
The deprecated
createPlDataTableV2shares this grid UI, so it now reconciles the samedeviations through the shared
computeHiddenColumnsrather than readinghiddenColIdsas an absoluteset — otherwise the ~29 blocks still on V2 would over-include columns once a user changed
visibility.
Tests
resolveColumnHidden,computeHiddenColumns,computeHiddenColumnsV2,computeVisibilityDeviations, and the v7→v8 migration — each with a MILAB-6002regression case. New coverage:
deriveColumnVisibility: the empty-grid guard, fresh derivation, and collapse toundefined.computeVisibilityDeviations: order-stability across a column reorder.computeHiddenColumns: a sorted column the user explicitly hid stays force-visible (preservation wins over the hide).optionaland back.Verify
antibody-tcr-lead-selection: 2 filters → 8 columns; remove one, re-run → 7 (ex-filterhidden). Previously stayed 8 (or errored and showed all).
Notes
minoronmodel(adds the publicresolveColumnHiddenexport);patchonui-vue.createPlDataTableV2in lockstep (~29 blocks share the grid UI).migrateV7toV8returns a freshpTableParamsrather than aliasing its input.Greptile Summary
This PR fixes MILAB-6002 by replacing the absolute hidden-column set with a deviation model — only the user's explicit show/hide choices (relative to each column's block-defined default) are persisted. Untouched columns now follow their current default on every run, so a filter/ranking column whose default reverts to
optionalafter removal correctly re-hides instead of staying visible. Av7→v8state migration resets all old absolute visibility state; block defaults then apply cleanly going forward.resolveColumnHiddenas the single reconciliation function shared by the model (computeHiddenColumns) and the grid (makeColDef), preventing the model/UI divergence that made the bug reproducible in two places simultaneously.deriveColumnVisibility/computeVisibilityDeviationson the UI side to derive deviations from the live grid; the empty-grid guard prevents teardown or pre-column-def state events from wiping saved preferences.createPlDataTableV2path through the sharedcomputeHiddenColumnsso ~29 blocks on V2 get the same deviation logic as V3 without a separate implementation.Confidence Score: 5/5
Safe to merge. The deviation model is correctly implemented end-to-end, the v7→v8 migration is a clean reset rather than a lossy conversion, and the test suite covers the MILAB-6002 regression, the empty-grid timing edge case, and the documented limitation of explicit hides being absorbed when a default flips.
The core fix is well-contained:
resolveColumnHiddenis the single source of precedence shared by both the model and the grid, eliminating the drift that caused the original bug. The migration strategy (reset rather than attempt a lossless conversion that would require per-column defaults not available at migration time) is the correct call. The empty-grid guard inderiveColumnVisibilityprevents a subtle timing hazard during grid teardown and initialization. No new public API contracts are broken;makeColDef/calculateGridOptionsare internal to the package. The two known behavioral gaps (axis visibility not persisted, explicit hide absorbed when default flips to optional and back) are both documented and pinned in tests.No files require special attention. The two pre-existing review threads about axis column visibility behavior remain open discussion points but do not block the core correctness of this change.
Important Files Changed
resolveColumnHidden— the single canonical precedence function (forced-hidden → user-shown → user-hidden → optional default); well-documented and fully tested.shownColIdsthrough tocomputeHiddenColumns; refactors from absolute-set logic toresolveColumnHidden-per-column. Correctly exports for unit testing.computeHiddenColumnsV2(delegates to the shared V3 function), correctly bypassing sort/filter preservation that V2 handles separately.migrateV7toV8that resetscolumnVisibilityandhiddenColIds/shownColIdsto null/undefined; returns fresh objects to avoid aliasing. The null-sourceId fast path is intentional and documented.shownColIdsas an optional field in bothPlDataTableGridStateCore.columnVisibilityandPTableParamsV2. The optional annotation is intentional for backward-compat with old serialized data.computeVisibilityDeviations(sorted output for stable JSON comparison) andderiveColumnVisibility(empty-grid guard). Clean, well-tested, and correctly scoped to column specs only.normalizeColumnVisibilitywithderiveColumnVisibility;makePartialStatenow acceptsapiandprevto handle empty-grid timing. Axis column visibility is intentionally not persisted (documented limitation).shownColIdsparameter tocalculateGridOptions/makeColDef; changeshiddenColIdsfrom array toReadonlySetfor O(1) lookup; usesresolveColumnHiddenfor visibility.getShownColIdsmirroringgetHiddenColIds; correctly uses optional chaining for backward compatibility with old persistedcolumnVisibilitythat lacksshownColIds.Sequence Diagram
sequenceDiagram participant Block as Block (server) participant Model as createPlDataTableV3 participant State as table-state-v2 participant Vue as PlAgDataTableV2.vue participant Grid as AG Grid participant CV as column-visibility.ts Note over Block,CV: Save path (user toggles column visibility) Grid->>Vue: onStateUpdated(event) Vue->>CV: readGridColumnVisibility(api) CV-->>Vue: ColumnVisibilityState[] Vue->>CV: deriveColumnVisibility(cols, prev) CV->>CV: computeVisibilityDeviations(cols) CV-->>Vue: columnVisibility (deviations or undefined) Vue->>State: "gridState.value = {columnVisibility}" State->>Block: "pTableParams {hiddenColIds, shownColIds}" Note over Block,CV: Load path (block re-runs, state restored) Block->>Model: computeHiddenColumns(cols, sort, filters, hiddenSpecs, shownSpecs) Model->>Model: resolveColumnHidden per column Model-->>Block: hidden column set (visible table) Block->>Vue: gridState.value (with columnVisibility deviations) Vue->>Grid: "calculateGridOptions({hiddenColIds, shownColIds})" Grid->>Grid: makeColDef → resolveColumnHidden (same precedence) Grid-->>Vue: Column defs with correct hide flagsReviews (5): Last reviewed commit: "MILAB-6002: tighten changeset wording" | Re-trigger Greptile