Bug 2058236: Show a notice when column filters hide result rows - #1090
Open
kala-moz wants to merge 6 commits into
Open
Bug 2058236: Show a notice when column filters hide result rows#1090kala-moz wants to merge 6 commits into
kala-moz wants to merge 6 commits into
Conversation
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…vanced options and add expanded-row option plumbing Rename the "Advanced columns" dropdown to "Advanced options" (AdvancedColumnsMenu -> AdvancedOptionsMenu) and give it two grouped sections: "Columns" (Cliff's Delta, CLES, Significance) and "Expanded row" (effect size & confidence intervals, mode analysis, statistics table, data warnings). Add the state plumbing for the expanded-row options, mirroring the columns feature: an ExpandedRowOptions type, an `advanced_expanded` URL param (expandedRowUrl util), the columnPrefs slice field + updateExpandedRow action, a useExpandedRowOptions selector hook, and a shared useSeedAdvancedOptionsFromUrl hook that seeds both params on mount. All options default off (the simplified view) and persist in the URL so shared links reproduce the selection. No expanded-row rendering changes yet; that lands in a follow-up commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…op and gate mode controls Move the Base/New legend from the middle of the chart to the top so the labels read as a header for the whole graph, and add 16px of spacing beneath it (with the scatter strip and overall height adjusted to match). Add an optional `showModeControls` prop to CommonGraph (default true, so Student-T is unaffected) that hides the mode-analysis controls (valley-depth slider + "Show modes" checkbox) as a unit. The Mann-Whitney-U simplified view uses it to keep those controls out of the default view until the "Mode analysis" expanded-row option is enabled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Restructure the Mann-Whitney-U expanded row into a simplified default view: a full-width density graph with a how-to-read blurb plus the always-on summary (platform, single-run note, Base/New application, comparison result). The heavier statistical components are opt-in via the "Advanced options -> Expanded row" checkboxes. - Lay the four opt-in components out in a two-column grid below the graph (effect size + mode analysis on one row, statistics table + data warnings on the next) rather than stacking them full-width; the stats table and warnings fill their cell (dropped hard-coded 85%/55% widths). - Gate the mode-analysis controls and on-chart overlays behind the "Mode analysis" option (via CommonGraph's showModeControls) so they're absent from the default view. - Show a "No mode analysis available" placeholder when Mode analysis is on but the comparison yields no mode breakdown, so the cell isn't left empty. - Drop the now-unused options plumbing from the strategy's renderExpandedBottom (the components are rendered directly by the expanded grid). Student-T is untouched. Adds coverage for the default hidden state, per-option reveals, the mode-controls gating, and the empty-state placeholder. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kala-moz
force-pushed
the
show-feedback-for-filtered-rows
branch
from
August 27, 2026 22:37
461c4bc to
9d6e2c4
Compare
esanuandra
approved these changes
Sep 4, 2026
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.
Show a notice when column filters hide result rowscommit, and merge this after phase-2 lands.See Bug 2058236
Main result preview
Subtests preview
Problem
A comparison that showed fewer rows than expected gave no indication that anything had been filtered out, which was confusing — a row hidden because it was "Not Significant" just silently disappeared.
Solution
Add an info notice above the results (main and subtests tables, both the regular and over-time views) that reports how many rows the active column filters are hiding and why, e.g. "2 rows hidden by filters — Status: No changes". It counts only rows removed by column filters, not ones the search
term already removed (the search box already shows the user their query), and uses each column's configured labels so the wording stays in sync with the header. When nothing is hidden it renders nothing.
getFilterHiddenSummary()in useTableFilters returns the active filters (with their excluded value labels) and the hidden-row count, reusingfilterResultsfor a "search only" pass so both tables share one implementation.FilteredRowsNoticerenders the MUI info Alert; gated on the count at the call site so no element enters the tree (and no row ids shift) when there'snothing to show.
Wired into
TableContentandSubtestsResultsTable, above the rows and aboveNoResultsFound.Test Coverage
Tests cover the count/plural/reasons rendering and the end-to-end main-table flow (filter toggled -> notice appears with count and reason -> cleared -> notice gone). The integration test is placed last in its file because React's useId counter is global across a jest run, so running extra renders earlier
would shift other tests' snapshot ids.
Co-Authored-By: Claude Opus 4.8