You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "SBIN2010 (via GitHub)" <gi...@apache.org> on 2026/10/06 18:18:49 UTC

[PR] feat(pivot-table): collapse row groups by default [superset]

SBIN2010 opened a new pull request, #45030:
URL: https://github.com/apache/superset/pull/45030

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   This was mentioned in the discussion - https://github.com/apache/superset/discussions/39954
   Adds a **Collapse rows by default** checkbox to Pivot Table v2. It appears only when **Show rows subtotal** is enabled, and it is off by default, so existing charts stay fully expanded.
   When the option is on, row groups start collapsed and show their subtotal. The existing arrows still expand or collapse a single group. Nested levels stay collapsed until they are opened. The control uses `renderTrigger`, so toggling it does not rerun the query.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   1. Open a Pivot Table with at least two row dimensions.
   2. Enable **Show rows subtotal** and confirm **Collapse rows by default** appears beneath it.
   3. Turn **Collapse rows by default** on and run or refresh the chart. Only the top-level groups and their subtotals should be visible.
   4. Expand one group with its arrow. That group opens, sibling groups stay collapsed, and the next nested level stays collapsed until its own arrow is used.
   5. Collapse the same group again with its arrow.
   6. Turn **Show rows subtotal** off and confirm the collapse option is hidden and all row values are shown.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [X ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ X] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev merged PR #45030:
URL: https://github.com/apache/superset/pull/45030


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45030:
URL: https://github.com/apache/superset/pull/45030#discussion_r4199002305


##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -161,6 +163,26 @@ const parseLabel = (value: unknown): string | number => {
   return String(value);
 };
 
+/**
+ * Whether a row prefix is collapsed. An explicit toggle wins. Otherwise the
+ * prefix follows the default. The empty prefix is always expanded, because the
+ * ancestor walk visits it for every key.
+ */
+function isEffectivelyCollapsed(
+  key: string[],
+  collapsed: Record<string, boolean>,
+  collapseByDefault: boolean,
+): boolean {
+  if (key.length === 0) {
+    return false;
+  }
+  const keyId = flatKey(key);
+  if (keyId in collapsed) {
+    return Boolean(collapsed[keyId]);
+  }

Review Comment:
   **Suggestion:** Because `in` includes `Object.prototype`, a group named `constructor` or `toString` is treated as collapsed even when the default is off, hiding its descendants.
   
   **Assessment:** 🟠 `Major` Β· πŸ” `Occurrence: Rarely` Β· 🏷️ `Incorrect condition logic`
   
   [![Use CodeAnt Skill](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/use-codeant-skill-flat-v2.svg)](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=858a28591bdf499796a0feb94806de2f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=858a28591bdf499796a0feb94806de2f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   <details>
   <summary><b>Prompt for AI Agent πŸ€– </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx
   **Line:** 180:182
   **Comment:**
   	*Incorrect Condition Logic: Because `in` includes `Object.prototype`, a group named `constructor` or `toString` is treated as collapsed even when the default is off, hiding its descendants.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
   Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45030&comment_hash=cf93dfb4b4745eda0c20f02e9e5b7613c34c49ce259135b1ef14046c70dbdbd8&reaction=like'>πŸ‘</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45030&comment_hash=cf93dfb4b4745eda0c20f02e9e5b7613c34c49ce259135b1ef14046c70dbdbd8&reaction=dislike'>πŸ‘Ž</a>



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #45030:
URL: https://github.com/apache/superset/pull/45030#discussion_r4200777546


##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -357,13 +388,24 @@ export function TableRenderer(props: TableRendererProps) {
   const [collapsedRows, setCollapsedRows] = useState<Record<string, boolean>>(
     {},
   );
+  // Filled once row keys are known. The toggle reads it at click time so the
+  // default matches the prefixes that have a renderable subtotal row.
+  const collapsibleRowPrefixesRef = useRef<ReadonlySet<string>>(
+    NO_COLLAPSIBLE_PREFIXES,
+  );
   const [collapsedCols, setCollapsedCols] = useState<Record<string, boolean>>(
     {},
   );
   const [sortingOrder, setSortingOrder] = useState<string[]>([]);
   const [activeSortColumn, setActiveSortColumn] = useState<number | null>(null);
   const [sortedRowKeys, setSortedRowKeys] = useState<string[][] | null>(null);
 
+  // Default collapse only applies while row subtotals are on. Without them
+  // there is no subtotal row to show and no arrow to expand a hidden group.
+  const collapseRowsByDefault = Boolean(

Review Comment:
   Explicit toggles in `collapsedRows` always beat this default, so the Explore preview can drift from what viewers get. Expand US, untick and re-tick "Collapse rows by default", and US stays open, while the saved chart opens with everything collapsed.
   
   Could we clear `collapsedRows` when `collapseRowsByDefault` flips? An effect keyed on it that resets the state to `{}` would cover it, plus a test that toggles the prop after an expand.



##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -788,19 +844,37 @@ export function TableRenderer(props: TableRendererProps) {
 
   const rowKeys = effectiveRowKeys;
 
+  // Subtotal rows are the only prefixes default collapse may hide children
+  // under. A shorter key that was never produced (metric-only rollups are
+  // omitted for metrics-on-rows) must stay expanded.
+  const collapsibleRowPrefixes = useMemo(() => {
+    const prefixes = new Set<string>();
+    basePivotSettings.rowKeys.forEach(key => {
+      if (key.length > 0 && key.length < basePivotSettings.rowAttrs.length) {
+        prefixes.add(flatKey(key));
+      }
+    });
+    return prefixes;
+  }, [basePivotSettings]);
+  collapsibleRowPrefixesRef.current = collapsibleRowPrefixes;

Review Comment:
   Nit, take it or leave it. Writing the ref during render works, but `renderTableRow` already has `collapseRowsByDefault` and `collapsibleRowPrefixes` in hand when it builds the arrow handler.
   
   Passing the default in, e.g. `toggleRowKey(flatRowKeySlice, collapseRowsByDefault && collapsibleRowPrefixes.has(flatRowKeySlice))`, lets the updater use it directly and drops both the ref and the extra `useCallback` dep.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45030:
URL: https://github.com/apache/superset/pull/45030#issuecomment-6022750669

   ### <span aria-hidden="true">βœ…</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">πŸ”¨</span> Latest commit | d3cde50b3ebf3cccb7ed7774ffc536c222b8fc14 |
   |<span aria-hidden="true">πŸ”</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ac53bb082379e0008876e38 |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45030--superset-docs-preview.netlify.app](https://deploy-preview-45030--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">πŸ“±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQ1MDMwLS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.NpxDCZp_Ul-PfYKbH267q9DwA4Y5svkmnhP2NW7Yi7A)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">πŸ€–</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEADAv-w5SJLS-kGvkFU3CqxdTIqI_p63OcwLknmXAhNAAz7jEdbqhbC0sl1cXEGfyAVOCeUkl_muMZIkfuZY64Ch194r22kzklLKWjOQtvD9) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-45030--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on code in PR #45030:
URL: https://github.com/apache/superset/pull/45030#discussion_r4199993170


##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -161,6 +163,26 @@ const parseLabel = (value: unknown): string | number => {
   return String(value);
 };
 
+/**
+ * Whether a row prefix is collapsed. An explicit toggle wins. Otherwise the
+ * prefix follows the default. The empty prefix is always expanded, because the
+ * ancestor walk visits it for every key.
+ */
+function isEffectivelyCollapsed(
+  key: string[],
+  collapsed: Record<string, boolean>,
+  collapseByDefault: boolean,
+): boolean {
+  if (key.length === 0) {
+    return false;
+  }
+  const keyId = flatKey(key);
+  if (keyId in collapsed) {
+    return Boolean(collapsed[keyId]);
+  }

Review Comment:
   fix in [6fd5b41](https://github.com/apache/superset/pull/45030/commits/6fd5b41d0b778aa65f9e4b1be2d455c46d3234ac)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on code in PR #45030:
URL: https://github.com/apache/superset/pull/45030#discussion_r4199991058


##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -729,17 +761,23 @@ export function TableRenderer(props: TableRendererProps) {
       collapsed: Record<string, boolean>,
       numAttrs: number,
       subtotalDisplay: SubtotalDisplay,
+      collapseByDefault: boolean,
     ) =>
       keys.filter(
         (key: string[]) =>
           // Is the key hidden by one of its parents?
-          !key.some(
-            (_k: string, j: number) => collapsed[flatKey(key.slice(0, j))],
+          !key.some((_k: string, j: number) =>
+            isEffectivelyCollapsed(
+              key.slice(0, j),
+              collapsed,
+              collapseByDefault,
+            ),

Review Comment:
   fix in [6fd5b41](https://github.com/apache/superset/pull/45030/commits/6fd5b41d0b778aa65f9e4b1be2d455c46d3234ac)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45030:
URL: https://github.com/apache/superset/pull/45030#issuecomment-6023021552

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45030?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `94.44444%` with `1 line` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 66.20%. Comparing base ([`7b3e3ad`](https://app.codecov.io/gh/apache/superset/commit/7b3e3ad6f7fa9d332d5d7148298d07e995f77782?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`75aee79`](https://app.codecov.io/gh/apache/superset/commit/75aee7957c7c5a0ef09105e5a72cb4a67edb82a0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 4 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45030?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...ugin-chart-pivot-table/src/plugin/controlPanel.tsx](https://app.codecov.io/gh/apache/superset/pull/45030?src=pr&el=tree&filepath=superset-frontend%2Fplugins%2Fplugin-chart-pivot-table%2Fsrc%2Fplugin%2FcontrolPanel.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtcGl2b3QtdGFibGUvc3JjL3BsdWdpbi9jb250cm9sUGFuZWwudHN4) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45030?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   > :exclamation:  There is a different number of reports uploaded between BASE (7b3e3ad) and HEAD (75aee79). Click for more details.
   > 
   > <details><summary>HEAD has 4 uploads less than BASE</summary>
   >
   >| Flag | BASE (7b3e3ad) | HEAD (75aee79) |
   >|------|------|------|
   >|python|7|3|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #45030       +/-   ##
   ===========================================
   - Coverage   82.47%   66.20%   -16.27%     
   ===========================================
     Files        3002     3002               
     Lines      186172   186186       +14     
     Branches    43128    43134        +6     
   ===========================================
   - Hits       153550   123272    -30278     
   - Misses      29867    60229    +30362     
   + Partials     2755     2685       -70     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/45030/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ” | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/45030/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.27% <ΓΈ> (ΓΈ)` | |
   | [javascript](https://app.codecov.io/gh/apache/superset/pull/45030/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.23% <94.44%> (+0.02%)` | :arrow_up: |
   | [mysql](https://app.codecov.io/gh/apache/superset/pull/45030/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/45030/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/45030/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.13% <ΓΈ> (ΓΈ)` | |
   | [python](https://app.codecov.io/gh/apache/superset/pull/45030/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.08% <ΓΈ> (-31.24%)` | :arrow_down: |
   | [sqlite](https://app.codecov.io/gh/apache/superset/pull/45030/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `54.96% <ΓΈ> (ΓΈ)` | |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/45030/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45030?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45030:
URL: https://github.com/apache/superset/pull/45030#issuecomment-6022689344

   ### <span aria-hidden="true">βœ…</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">πŸ”¨</span> Latest commit | 9b991d2061f9ac7d4d7c4f639a099196744b9264 |
   |<span aria-hidden="true">πŸ”</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ac53b8a841eac0007897d90 |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45030--superset-docs-preview.netlify.app](https://deploy-preview-45030--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">πŸ“±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQ1MDMwLS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.NpxDCZp_Ul-PfYKbH267q9DwA4Y5svkmnhP2NW7Yi7A)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">πŸ€–</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8w6yOijdYNOIaNOFFgzmBQR3T13b_FekMS7ZJgAKnAJD78WL4S5lu3ibDO6SNZzjCgn2cR3iYEk8jOHUgf0fesMmq4h9EopbUYdRgXfDw) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-45030--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45030:
URL: https://github.com/apache/superset/pull/45030#issuecomment-6022781727

   ## CodeAnt PR Risk: Medium Risk
   
   - The PR needs attention before merging.
   - The isEffectivelyCollapsed lookup includes inherited properties; if flatKey returns constructor or toString, matching row groups appear collapsed and hide descendants.
   
   **Assessed commit:** `75aee7957c7c`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45030:
URL: https://github.com/apache/superset/pull/45030#discussion_r4199985523


##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -161,6 +163,26 @@ const parseLabel = (value: unknown): string | number => {
   return String(value);
 };
 
+/**
+ * Whether a row prefix is collapsed. An explicit toggle wins. Otherwise the
+ * prefix follows the default. The empty prefix is always expanded, because the
+ * ancestor walk visits it for every key.
+ */
+function isEffectivelyCollapsed(
+  key: string[],
+  collapsed: Record<string, boolean>,
+  collapseByDefault: boolean,
+): boolean {
+  if (key.length === 0) {
+    return false;
+  }
+  const keyId = flatKey(key);
+  if (keyId in collapsed) {
+    return Boolean(collapsed[keyId]);
+  }

Review Comment:
   βœ… **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `262e8f9`.
   
   The explicit-collapse check now uses `Object.hasOwn(collapsed, keyId)`, so inherited properties such as `constructor` and `toString` are ignored.
   
   <sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
   
   <!-- codeant-auto-resolve-reply -->



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45030:
URL: https://github.com/apache/superset/pull/45030#issuecomment-6022784660

   ### <span aria-hidden="true">βœ…</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">πŸ”¨</span> Latest commit | 75aee7957c7c5a0ef09105e5a72cb4a67edb82a0 |
   |<span aria-hidden="true">πŸ”</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ac53c4a75ef33000894509b |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45030--superset-docs-preview.netlify.app](https://deploy-preview-45030--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">πŸ“±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQ1MDMwLS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.NpxDCZp_Ul-PfYKbH267q9DwA4Y5svkmnhP2NW7Yi7A)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">πŸ€–</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEADAv-w5SFAr-0GvkNU2CqxdTIqI_p63OcwLknmXAiNAAyHjEdfqhbC0sl1cfMGQyEdOCeUkn_mucSZJ_ExzrR1Gq6PB3tKitVJqcMYqF-D7AQ) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-45030--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45030:
URL: https://github.com/apache/superset/pull/45030#discussion_r4199096780


##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -729,17 +761,23 @@ export function TableRenderer(props: TableRendererProps) {
       collapsed: Record<string, boolean>,
       numAttrs: number,
       subtotalDisplay: SubtotalDisplay,
+      collapseByDefault: boolean,
     ) =>
       keys.filter(
         (key: string[]) =>
           // Is the key hidden by one of its parents?
-          !key.some(
-            (_k: string, j: number) => collapsed[flatKey(key.slice(0, j))],
+          !key.some((_k: string, j: number) =>
+            isEffectivelyCollapsed(
+              key.slice(0, j),
+              collapsed,
+              collapseByDefault,
+            ),

Review Comment:
   Default collapse treats every nonempty ancestor as a collapsible group, even when no subtotal row exists for it. With metrics on Rows, Combine metrics off, Show columns total off, and non-percentage values, `buildGroupbyCombinations` omits the empty-row rollup. `PivotTableChart` then produces keys such as `['value', 'US']` but no `['value']` subtotal. This walk treats that missing ancestor as collapsed and filters out every row. With Combine metrics on, intermediate row rollups are also omitted, so leaf keys such as `['US', 'NYC', 'value']` disappear too. Both cases leave no row-group arrow to reopen the data. Restrict default collapse to prefixes with renderable subtotal rows, use that same eligibility check for arrow state and toggle defaults, and add regression tests for both production metric layouts.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(pivot-table): collapse row groups by default [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45030:
URL: https://github.com/apache/superset/pull/45030#discussion_r4199835733


##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -161,6 +163,26 @@ const parseLabel = (value: unknown): string | number => {
   return String(value);
 };
 
+/**
+ * Whether a row prefix is collapsed. An explicit toggle wins. Otherwise the
+ * prefix follows the default. The empty prefix is always expanded, because the
+ * ancestor walk visits it for every key.
+ */
+function isEffectivelyCollapsed(
+  key: string[],
+  collapsed: Record<string, boolean>,
+  collapseByDefault: boolean,
+): boolean {
+  if (key.length === 0) {
+    return false;
+  }
+  const keyId = flatKey(key);
+  if (keyId in collapsed) {
+    return Boolean(collapsed[keyId]);
+  }

Review Comment:
   βœ… **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `6fd5b41`.
   
   The explicit-collapse check now uses `Object.hasOwn(collapsed, keyId)`, so inherited properties such as `constructor` and `toString` are ignored.
   
   <sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
   
   <!-- codeant-auto-resolve-reply -->



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org