feat(model): primaryColumns + enrichFromPool for createPlDataTableV3#1606
feat(model): primaryColumns + enrichFromPool for createPlDataTableV3#1606PoslavskySV wants to merge 2 commits into
Conversation
Adds a simple-case sugar for the most common table pattern: "show columns
from my workflow output, plus joinable columns from the result pool as
optional extras." Existing `columns: { sources, anchors, selector }`
form unchanged.
```ts
.outputWithStatus('mainTable', (ctx) =>
createPlDataTableV3(ctx, {
tableState: ctx.data.tableState,
primaryColumns: ctx.outputs?.resolve('mainPf'),
enrichFromPool: 'optional',
}),
)
```
`primaryColumns` accepts `TreeNodeAccessor`, `PColumn[]`, or
`TableColumnVariant[]`. `enrichFromPool` accepts `false`, `true`, a
visibility shorthand (`'optional' | 'hidden' | 'default'`), or a full
options object. Mutually exclusive with `columns`.
Other changes:
- TableColumnVariant: `path`, `qualifications`, `originalId`, `isPrimary`
are now optional with sensible defaults. Common case needs only `column`.
- Extensive JSDoc on createPlDataTableV3 and dependent types.
- TooltipableColumn.id widened to `PObjectId | DiscoveredPColumnId`
(DiscoveredPColumnId still assignable — backward compatible).
Backward-compatible: existing callers compile and run unchanged.
🦋 Changeset detectedLatest commit: 52c2be1 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}
There was a problem hiding this comment.
Code Review
This pull request enhances createPlDataTableV3 by adding primaryColumns and enrichFromPool options, which streamline the creation of data tables from workflow outputs and joinable pool columns. The changes include new normalization and discovery logic, updated type definitions, extensive JSDoc documentation, and corresponding unit tests. I have no feedback to provide.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Internal pipeline (split / annotate / build defs / etc.) |
There was a problem hiding this comment.
PColumn[] from TableColumnVariant[]
The heuristic "column" in first relies on PColumn not having a column property. If PColumn ever gains such a field, this silently misclassifies the input — treating a PColumn[] as a TableColumnVariant[] and wrapping it incorrectly. Consider using "spec" in first && "data" in first instead, which targets structural requirements of PColumn that are absent from TableColumnVariant.
| // --------------------------------------------------------------------------- | |
| // Internal pipeline (split / annotate / build defs / etc.) | |
| const first = input[0] as object; | |
| if ("spec" in first && "data" in first) { | |
| return new ArrayColumnProvider(input as PColumn<PColumnDataUniversal>[]) | |
| .getAllColumns() | |
| .map(snapshotToPrimaryVariant); | |
| } | |
| return (input as TableColumnVariant[]).map(normalizeTableColumnVariant); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/model/src/components/PlDataTable/createPlDataTable/createPlDataTableV3.ts
Line: 719-721
Comment:
**Fragile duck-typing to distinguish `PColumn[]` from `TableColumnVariant[]`**
The heuristic `"column" in first` relies on `PColumn` not having a `column` property. If `PColumn` ever gains such a field, this silently misclassifies the input — treating a `PColumn[]` as a `TableColumnVariant[]` and wrapping it incorrectly. Consider using `"spec" in first && "data" in first` instead, which targets structural requirements of `PColumn` that are absent from `TableColumnVariant`.
```suggestion
const first = input[0] as object;
if ("spec" in first && "data" in first) {
return new ArrayColumnProvider(input as PColumn<PColumnDataUniversal>[])
.getAllColumns()
.map(snapshotToPrimaryVariant);
}
return (input as TableColumnVariant[]).map(normalizeTableColumnVariant);
```
How can I resolve this? If you propose a fix, please make it concise.| * direct columns (the typical case). | ||
| * @default [] | ||
| */ | ||
| readonly path?: { linker: ColumnSnapshot<PObjectId> }[]; | ||
| /** |
There was a problem hiding this comment.
columns: null
Nil is undefined | null, and columns is typed as Nil | DiscoverTableColumnOptions | TableColumnVariant[]. Passing { primaryColumns: ..., columns: null } trips the guard (null !== undefined is true) and throws — even though null is a documented valid value meaning "no columns". Using !isNil(options.columns) would let null fall through correctly.
| * direct columns (the typical case). | |
| * @default [] | |
| */ | |
| readonly path?: { linker: ColumnSnapshot<PObjectId> }[]; | |
| /** | |
| if (options.primaryColumns !== undefined && !isNil(options.columns)) { | |
| throw new Error( | |
| "createPlDataTableV3: `primaryColumns` and `columns` are mutually exclusive — pass one or the other.", | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/model/src/components/PlDataTable/createPlDataTable/createPlDataTableV3.ts
Line: 461-465
Comment:
**Mutual-exclusion check fires on `columns: null`**
`Nil` is `undefined | null`, and `columns` is typed as `Nil | DiscoverTableColumnOptions | TableColumnVariant[]`. Passing `{ primaryColumns: ..., columns: null }` trips the guard (`null !== undefined` is `true`) and throws — even though `null` is a documented valid value meaning "no columns". Using `!isNil(options.columns)` would let `null` fall through correctly.
```suggestion
if (options.primaryColumns !== undefined && !isNil(options.columns)) {
throw new Error(
"createPlDataTableV3: `primaryColumns` and `columns` are mutually exclusive — pass one or the other.",
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1606 +/- ##
==========================================
- Coverage 53.91% 52.90% -1.01%
==========================================
Files 265 269 +4
Lines 15031 15332 +301
Branches 3162 3240 +78
==========================================
+ Hits 8104 8112 +8
- Misses 5889 6183 +294
+ Partials 1038 1037 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The pool walk inside `discoverEnrichmentColumns` calls into pframes-rs-wasm through `pframeSpec`. Discovered in field testing: when the result pool contains certain column shapes (e.g. after a parameter change that restructures the workflow output), the WASM layer can trap with "table index is out of bounds", which propagates as a thrown error and aborts whatever task is evaluating the model — including mutate-block-storage, blocking parameter changes entirely. Catch the error, log a warning, and return an empty enrichment list. The table still renders with its primary columns; only the optional pool-discovered extras are missing until the underlying WASM bug is fixed. The collection is still disposed via the existing finally. The WASM trap itself needs a separate fix in pframes-rs-wasm; this change is purely a robustness boundary at the SDK layer.
|
Hardening commit added: Field testing surfaced a WASM trap ( The WASM bug needs a separate fix in This is a defensive boundary, not a workaround for incorrect logic in our own code. No behavior change in the success path. |
| * - `false` (default) — no enrichment. | ||
| * - `true` — equivalent to `'optional'`. |
There was a problem hiding this comment.
those two options only complicate contract without benefits
| * Mutually exclusive with `columns`. When neither is set or this resolves | ||
| * to an empty list, the function returns `undefined`. | ||
| */ | ||
| primaryColumns?: TreeNodeAccessor | PColumn<PColumnDataUniversal>[] | TableColumnVariant[]; |
There was a problem hiding this comment.
we already have mechanism for this, you can mark isPrimary = true in columns or you can make this columns as anchors
| return { | ||
| column: { | ||
| id: discoveredId, | ||
| spec: withVisibilityAnnotation(v.column.spec, visibility), |
There was a problem hiding this comment.
wrong, visibility shouldn't influence to id. ID concept is uniquity of data, not uniquity of representation
2 participants
Summary
Adds a simple-case sugar for the most common table pattern in blocks — show columns from my workflow output, plus joinable columns from the result pool as optional extras:
Three lines instead of the ~15 lines of
ColumnCollectionBuilderplumbing this case currently requires. The advancedcolumns: { sources, anchors, selector }form is untouched and stays available for cases likeantibody-tcr-lead-selectionthat need explicit anchor selection or custom-filtered sources.Why
Today, blocks that just want to display their workflow output in a data table have two unappealing options:
createPlDataTableV2— flickers when result-pool label columns transition between "ready" and "not ready", because V2 returnsundefinedif any single label column isn't ready (seeallPColumnsReadygate). This causes column headers to mount/unmount rapidly when the user adds metadata + re-runs upstream blocks.createPlDataTableV3withcolumns: TableColumnVariant[]— works, but requires constructing variants withpath,qualifications,originalId,isPrimaryfor every column. Bulk of the code is wrappingOutputColumnProvider(accessor).getAllColumns().map(snap => ({ column: ..., path: [], qualifications: { forHit: [], forQueries: {} }, originalId: snap.id, isPrimary: true })).Neither matches the user's actual mental model: here are MY columns; please attach anything else from the pool that joins to them, optionally hideable.
What
New options on
createPlDataTableOptionsV3primaryColumnsandcolumnsare mutually exclusive — passing both throws.Relaxation on
TableColumnVariantpath,qualifications,originalId, andisPrimaryare now all optional with documented defaults ([],{ forHit: [], forQueries: {} },column.id,true). For the common case onlycolumnis required.Implementation strategy
primaryColumnsis normalized to variants taggedisPrimary: true.TreeNodeAccessoris wrapped viaOutputColumnProvider(preserves per-columndataStatusfor partial-data rendering).enrichFromPoolruns anchored discovery viaColumnCollectionBuilder, with primary columns registered as the FIRST source. The builder's existingNativePObjectIddedup (first-source-wins) handles the case where the workflow output is also re-published to the result pool — no manual exclusion needed.visibilityset as apl7.app/table/visibilityannotation on its spec. User-supplieddisplayOptions.visibilityrules still take precedence per the existing first-match-wins evaluation inevaluateRules.Documentation
@defaulton every option.createPlDataTableV3showing common usage.ColumnsSelectorConfig,ColumnsDisplayOptions,ColumnOrderRule,ColumnVisibilityRule,ColumnMatcher,DiscoverTableColumnOptions, and the internal pipeline functions (splitDiscoveredColumns,annotateColumnGroups,buildSecondaryGroups, etc.).Backward compatibility
Verified clean. Every change is either additive (new optional fields), a relaxation (required → optional), or a type widening (DiscoveredPColumnId → DiscoveredPColumnId | PObjectId, where DiscoveredPColumnId still assignable). All existing callers in the monorepo compile and run unchanged. Specifically:
clonotype-browser(usescolumns: { ... }discovery andcolumns: TableColumnVariant[]) — untouched.antibody-tcr-lead-selection(usescolumns: { sources, anchors, selector }with custom-filtered pool sources, custom anchor selection, multi-rule visibility/ordering,labelsOptions.formatters.linker) — every advanced lever they use is preserved.etc/blocks/table-test— usescolumns: { ... }form, untouched.TooltipableColumn.idwidened fromDiscoveredPColumnIdtoPObjectId | DiscoveredPColumnId. Backward-compatible becauseDiscoveredPColumnIdextendsPObjectIdvia branding — both still accepted.Test plan
pnpm types:checkclean for@platforma-sdk/model.pnpm test— 384 passed (374 existing + 10 new).pnpm buildclean (declaration generation works).clonotype-distribution,clonotype-browser, andantibody-tcr-lead-selection(in progress separately).Files changed
sdk/model/src/components/PlDataTable/createPlDataTable/createPlDataTableV3.ts— main implementation + extensive JSDoc.sdk/model/src/components/PlDataTable/createPlDataTable/discoverColumns.ts— JSDoc only.sdk/model/src/components/PlDataTable/createPlDataTable/utils.ts— single-field type widening onTooltipableColumn.id.sdk/model/src/components/PlDataTable/createPlDataTable/createPlDataTableV3.test.ts— new test file (10 tests)..changeset/createpldatatablev3-primary-enrichment.md— minor bump for@platforma-sdk/model.Greptile Summary
This PR adds
primaryColumnsandenrichFromPoolas ergonomic sugar oncreatePlDataTableV3, reducing the typical "show my workflow output + joinable pool extras" pattern from ~15 lines ofColumnCollectionBuilderplumbing to 3. All existing callers are unaffected;TableColumnVariantfields are relaxed to optional with sensible defaults, and the advancedcolumns: { sources, anchors, selector }form is preserved unchanged.Confidence Score: 4/5
Safe to merge — no P0/P1 findings; two P2 issues are low-impact edge cases.
Only P2 findings present: a fragile duck-typing check to distinguish PColumn[] from TableColumnVariant[], and a mutual-exclusion guard that incorrectly fires when columns: null (a valid Nil value) is combined with primaryColumns. Neither affects the primary happy-path flows, which are covered by 10 new tests.
createPlDataTableV3.ts — the normalizePrimaryColumns array-type discriminant and the mutual-exclusion null check.
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(model): primaryColumns + enrichFrom..." | Re-trigger Greptile