Skip to content

Use createPlDataTableV3#10

Open
erohinaelena wants to merge 1 commit into
mainfrom
erohinaelena/update-table-to-v3
Open

Use createPlDataTableV3#10
erohinaelena wants to merge 1 commit into
mainfrom
erohinaelena/update-table-to-v3

Conversation

@erohinaelena

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request migrates the summaryTable output to use createPlDataTableV3, refactoring the column selection and visibility logic while also updating several SDK and internal package dependencies. The review feedback identifies two potential regressions: first, the new enrichment-based selector may exclude signal columns that were previously included if they possess additional axes; second, File-type columns that were explicitly filtered out in the previous implementation are now included by the discovery process and require a specific visibility rule to remain hidden.

Comment thread model/src/index.ts
tableState: ctx.data.tableState,
columns: {
anchors: { main: kdCol.spec },
selector: { mode: "enrichment", maxHops: 0 },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of mode: "enrichment" with maxHops: 0 in the selector only discovers columns whose axes are a subset of the anchor axes (kdCol). In the previous implementation, signalCols (from signalPf) were included explicitly without this restriction. If signalCols have additional axes (e.g., they are per-concentration data as suggested by the comments), they will no longer be discovered and will be missing from the table. Consider using a selector that includes extensions or explicitly adding these columns if they are required for the "export per-concentration data" feature mentioned in the comments.

Comment thread model/src/index.ts
Comment on lines +441 to +473
return createPlDataTableV3(ctx, {
tableState: ctx.data.tableState,
columns: {
anchors: { main: kdCol.spec },
selector: { mode: "enrichment", maxHops: 0 },
},
displayOptions: {
visibility: [
// Plot-only sentinels: keep fully hidden even in this table.
{
match: (spec) =>
spec.name === "pl7.app/vdj/kdPlotPosition" ||
spec.name === "pl7.app/vdj/hillPlotPosition",
visibility: "hidden",
},
// Own outputs that the workflow hides from downstream pickers
// (fitFailureReason, signal columns) should be visible in this table.
// The sentinel names above are matched first, so they are not affected.
{
match: (spec) =>
!!spec.annotations?.["pl7.app/trace"]?.includes("milaboratories.titeseq-analysis") &&
spec.annotations?.["pl7.app/table/visibility"] === "hidden",
visibility: "default",
},
// Columns from other blocks: available in the column picker, hidden by default.
{
match: (spec) =>
!spec.annotations?.["pl7.app/trace"]?.includes("milaboratories.titeseq-analysis"),
visibility: "optional",
},
],
},
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The previous implementation explicitly filtered out columns with valueType: "File". The new discovery-based approach will include them if they match the anchor axes, making them appear in the column picker as optional (due to the last visibility rule). To maintain the previous behavior of hiding File columns, you should add an explicit visibility rule.

    return createPlDataTableV3(ctx, {
      tableState: ctx.data.tableState,
      columns: {
        anchors: { main: kdCol.spec },
        selector: { mode: "enrichment", maxHops: 0 },
      },
      displayOptions: {
        visibility: [
          // Plot-only sentinels: keep fully hidden even in this table.
          {
            match: (spec) =>
              spec.name === "pl7.app/vdj/kdPlotPosition" ||
              spec.name === "pl7.app/vdj/hillPlotPosition",
            visibility: "hidden",
          },
          // Hide File columns
          {
            match: (spec) => spec.valueType === "File",
            visibility: "hidden",
          },
          // Own outputs that the workflow hides from downstream pickers
          // (fitFailureReason, signal columns) should be visible in this table.
          // The sentinel names above are matched first, so they are not affected.
          {
            match: (spec) =>
              !!spec.annotations?.["pl7.app/trace"]?.includes("milaboratories.titeseq-analysis") &&
              spec.annotations?.["pl7.app/table/visibility"] === "hidden",
            visibility: "default",
          },
          // Columns from other blocks: available in the column picker, hidden by default.
          {
            match: (spec) =>
              !spec.annotations?.["pl7.app/trace"]?.includes("milaboratories.titeseq-analysis"),
            visibility: "optional",
          },
        ],
      },
    });

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