feat(frontend/recs): per-column show/hide with Columns toolbar dropdown#326
Merged
cristim merged 8 commits intoMay 11, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds per-column show/hide to the recommendations table: ColumnDef model, in-memory hidden-columns accessors, versioned localStorage load/save/reset APIs, a visibility popover UI, render updates honoring visible columns, CSS, and tests verifying behavior. ChangesColumn Visibility Feature
Sequence DiagramsequenceDiagram
participant User
participant UI as Visibility Popover
participant State as State Module
participant Storage as localStorage
participant Render as Render Engine
User->>UI: Click "Columns" button
UI->>State: getHiddenColumns()
State-->>UI: current hidden set
UI->>User: Display popover with toggles
User->>UI: Toggle checkbox
UI->>State: setHiddenColumns(updated set)
State->>Storage: saveColumnVisibility(updated set)
Storage-->>State: persisted
UI->>Render: Request re-render
Render->>State: getHiddenColumns()
State-->>Render: current hidden set
Render->>Render: Filter COLUMN_DEFS → visibleCols
Render->>User: Updated table with visibleCols
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
cristim
added
triaged
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/recommendations.ts (1)
2068-2121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHidden toggleable columns still appear in collapsed summary rows
Line 2120 and Line 2121 always render Region / Savings / Upfront / Term text even when those columns are hidden, so hide/show is not consistently applied across all row
<tr>variants.Proposed fix
const visibleToggleableCols = visibleCols.filter((c) => TOGGLEABLE_COLUMN_KEYS.has(c.key)); const summaryColspan = 1 + visibleToggleableCols.length; + const visibleKeys = new Set(visibleCols.map((c) => c.key)); @@ - rows.push(` + const identityParts: string[] = [ + `${escapeHtml(rep.resource_type)}${rep.engine ? ` (${escapeHtml(rep.engine)})` : ''}`, + ]; + if (visibleKeys.has('region')) identityParts.push(escapeHtml(rep.region)); + identityParts.push(`${variants.length} variants`); + + const rangeParts: string[] = []; + if (visibleKeys.has('savings')) rangeParts.push(`${savingsDisplay}/mo`); + if (visibleKeys.has('upfront_cost')) rangeParts.push(`upfront: ${upfrontDisplay}`); + if (visibleKeys.has('term')) rangeParts.push(`term: ${termDisplay}`); + + rows.push(` <tr class="rec-cell-summary-row" data-cell-key="${escapeHtml(key)}"> @@ <td colspan="${summaryColspan}" class="rec-cell-summary-content"> - <span class="rec-cell-identity">${escapeHtml(rep.resource_type)}${rep.engine ? ` (${escapeHtml(rep.engine)})` : ''} — ${escapeHtml(rep.region)} — ${variants.length} variants</span> - <span class="rec-cell-range">${savingsDisplay} · upfront: ${upfrontDisplay} · term: ${termDisplay}</span> + <span class="rec-cell-identity">${identityParts.join(' — ')}</span> + ${rangeParts.length > 0 ? `<span class="rec-cell-range">${rangeParts.join(' · ')}</span>` : ''} </td> </tr>`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/recommendations.ts` around lines 2068 - 2121, Summary: The collapsed summary row always renders region/savings/upfront/term text regardless of column visibility; update rec-cell-summary-content rendering to respect visibleCols/TOGGLEABLE_COLUMN_KEYS and summaryColspan. Fix: inside the multi-variant branch where you build the summary row (code using variables summaryColspan, summary, savingsDisplay, upfrontDisplay, termDisplay, chevron, and rep), conditionally include the region span and the rec-cell-range elements only if their corresponding column keys are present in visibleCols (use visibleCols.some(c => c.key === '<key>') or TOGGLEABLE_COLUMN_KEYS.has(...) on the right keys), and recompute summaryColspan based on visibleToggleableCols so colspan matches; ensure selectedVariant logic is preserved but only injects savings/upfront/term markup when those columns are visible.
🧹 Nitpick comments (1)
frontend/src/state.ts (1)
167-169: 💤 Low valueReturn a defensive copy for consistency with other getters.
Other state getters in this file (
getSelectedRecommendationIDs,getVisibleRecommendations,getRecommendationsColumnFilters) return defensive copies to prevent callers from mutating internal state.getHiddenColumnsreturns the internal Set reference directly. WhileReadonlySetprevents TypeScript-level mutations, at runtime a caller could bypass this via casting.Suggested fix
export function getHiddenColumns(): ReadonlySet<RecommendationsColumnId> { - return hiddenColumns; + return new Set(hiddenColumns); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/state.ts` around lines 167 - 169, getHiddenColumns currently returns the internal Set reference (hiddenColumns) which can be mutated at runtime; change it to return a defensive copy by constructing and returning a new Set from hiddenColumns (while preserving the declared return type ReadonlySet<RecommendationsColumnId>) so callers receive a copy and cannot mutate internal state; update the return in getHiddenColumns to return a new Set created from hiddenColumns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/recommendations.ts`:
- Around line 1458-1548: The tab deactivation MutationObserver currently only
calls closePopover(), leaving the visibility popover (managed by
openVisibilityPopover and closeVisibilityPopover) open after navigation; update
the tab deactivation/observer callback to also call closeVisibilityPopover()
wherever closePopover() is invoked so the visibility popover is closed and its
listeners removed when the recommendations tab deactivates. Ensure you reference
the openVisibilityPopover and closeVisibilityPopover symbols when making the
change so the visibility popover lifecycle is cleaned up consistently.
---
Outside diff comments:
In `@frontend/src/recommendations.ts`:
- Around line 2068-2121: Summary: The collapsed summary row always renders
region/savings/upfront/term text regardless of column visibility; update
rec-cell-summary-content rendering to respect visibleCols/TOGGLEABLE_COLUMN_KEYS
and summaryColspan. Fix: inside the multi-variant branch where you build the
summary row (code using variables summaryColspan, summary, savingsDisplay,
upfrontDisplay, termDisplay, chevron, and rep), conditionally include the region
span and the rec-cell-range elements only if their corresponding column keys are
present in visibleCols (use visibleCols.some(c => c.key === '<key>') or
TOGGLEABLE_COLUMN_KEYS.has(...) on the right keys), and recompute summaryColspan
based on visibleToggleableCols so colspan matches; ensure selectedVariant logic
is preserved but only injects savings/upfront/term markup when those columns are
visible.
---
Nitpick comments:
In `@frontend/src/state.ts`:
- Around line 167-169: getHiddenColumns currently returns the internal Set
reference (hiddenColumns) which can be mutated at runtime; change it to return a
defensive copy by constructing and returning a new Set from hiddenColumns (while
preserving the declared return type ReadonlySet<RecommendationsColumnId>) so
callers receive a copy and cannot mutate internal state; update the return in
getHiddenColumns to return a new Set created from hiddenColumns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edcab9ca-4520-4c48-9f44-aea3cb656a7a
📒 Files selected for processing (4)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsfrontend/src/state.tsfrontend/src/styles/components.css
Member
Author
|
Addressed the latest CodeRabbit comments in 6eeb34b:
Verification:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review of the latest commit now. [review] |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Line 3627: The test sets a sticky mock implementation for
state.getHiddenColumns via (state.getHiddenColumns as
jest.Mock).mockReturnValue(...) which isn't cleared by jest.clearAllMocks();
update the describe block's beforeEach to explicitly reset or re-mock
getHiddenColumns to return a new empty Set (e.g., call (state.getHiddenColumns
as jest.Mock).mockReturnValue(new Set()) or .mockReset() and then
.mockReturnValue(...)) so loadRecommendations() and other tests don't inherit
the previous sticky return value.
In `@frontend/src/recommendations.ts`:
- Around line 1545-1549: The deferred setTimeout may try to add a null
visOutsideClickHandler because closeVisibilityPopover() can clear it before the
callback runs; within openVisibilityPopover_ capture the current
visOutsideClickHandler into a local const (e.g., const handler =
visOutsideClickHandler) and use that captured reference inside the setTimeout
callback, and guard adding only if handler is non-null before calling
document.addEventListener; this preserves the intended tick delay while avoiding
the race and the non-null assertion.
In `@frontend/src/state.ts`:
- Around line 171-172: setHiddenColumns currently accepts any
RecommendationsColumnId which allows callers to hide fixed anchor columns;
change it to enforce the same whitelist used by loadColumnVisibility by either
(a) narrowing the parameter type to the existing ToggleableColumnId (or create
one) so callers cannot pass fixed columns, or (b) filter the incoming hidden set
inside setHiddenColumns to remove provider, account, service, and resource_type
before assigning hiddenColumns; update the function signature or perform the
filter and keep the assignment to hiddenColumns = new Set(filtered) so fixed
columns remain visible.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efa60bc2-361a-4e0b-be9c-f641b47cfaa8
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsfrontend/src/state.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
cristim
force-pushed
the
feat/recs-column-visibility
branch
from
8635816 to
92af34c
Compare
cristim
force-pushed
the
feat/recs-column-visibility
branch
from
92af34c to
4641b47
Compare
Member
Author
|
Rebased onto current
Re: the 3 actionable findings from the prior review — verified each against current code; all already addressed:
Plus the test sticky-mock concern — Full suite: 1551/1551 passing. Build clean. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Good to see the three prior findings are confirmed present in current code, and the test [review] |
cristim
force-pushed
the
feat/recs-column-visibility
branch
from
4641b47 to
1a1e78e
Compare
Member
Author
|
Rebased again — #328 (cost-period selector) merged into base, which created fresh conflicts. Resolved across 3 files:
Note: the prior fixup commit Full suite: 1592/1592 passing. Build clean. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review now. [review] |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/recommendations.ts (1)
1756-1761:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale outside-click handler attachment after fast close.
At Line 1757, the deferred callback can still attach an old
handleraftercloseVisibilityPopover()ran, which leaves an untracked document listener and can make later popovers close unexpectedly.Proposed fix
const handler = visOutsideClickHandler; setTimeout(() => { - if (handler) { + if (handler && openVisibilityPopover && visOutsideClickHandler === handler) { document.addEventListener('mousedown', handler); } }, 0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/recommendations.ts` around lines 1756 - 1761, The deferred attachment may add a stale visOutsideClickHandler after closeVisibilityPopover() runs; to fix, capture the handler as you do (visOutsideClickHandler) but before calling document.addEventListener verify that the current visOutsideClickHandler still equals that captured handler (or that the popover is still open) so you don't attach an outdated listener—update the setTimeout callback in the block that sets handler to perform this equality/check against visOutsideClickHandler (and ensure closeVisibilityPopover clears visOutsideClickHandler when closing).
🧹 Nitpick comments (2)
frontend/src/__tests__/recommendations.test.ts (1)
4752-4767: ⚡ Quick winTighten the toggleable-columns assertion to pin the full contract.
This test currently checks exclusions and several inclusions, but not the full set size or
on_demand_monthly. Pinning the exact set will catch future drift/rebase misses earlier.Suggested test hardening
test('TOGGLEABLE_COLUMNS excludes fixed identity columns', () => { const keys = TOGGLEABLE_COLUMNS.map((c) => c.key); @@ expect(keys).toContain('monthly_cost'); + expect(keys).toContain('on_demand_monthly'); expect(keys).toContain('effective_savings_pct'); + expect(keys.length).toBe(9); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/recommendations.test.ts` around lines 4752 - 4767, The test 'TOGGLEABLE_COLUMNS excludes fixed identity columns' currently asserts a few inclusions/exclusions but not the full contract; update it to assert the exact toggleable keys set by building an expectedKeys array (including on_demand_monthly plus the other toggleables like region, count, term, payment, savings, upfront_cost, monthly_cost, effective_savings_pct) and then assert equality with TOGGLEABLE_COLUMNS.map(c => c.key) (either by comparing sorted arrays or checking length and set equality) so the test pins the full contract for TOGGLEABLE_COLUMNS.frontend/src/recommendations.ts (1)
1660-1663: ⚡ Quick winSync
aria-expanded/aria-haspopupfor the Columns trigger.The popover trigger currently uses
aria-pressed, but it doesn’t expose open/closed dialog state. Addingaria-haspopup="dialog"and togglingaria-expandedon open/close improves keyboard/screen-reader behavior.Proposed refactor
interface VisibilityPopoverState { el: HTMLDivElement; checkboxes: Map<state.RecommendationsColumnId, HTMLInputElement>; + trigger: HTMLElement; } @@ function closeVisibilityPopover(): void { if (!openVisibilityPopover) return; - openVisibilityPopover.el.remove(); + const { el, trigger } = openVisibilityPopover; + el.remove(); + trigger.setAttribute('aria-expanded', 'false'); openVisibilityPopover = null; @@ function openVisibilityPopover_(anchor: HTMLElement): void { @@ - openVisibilityPopover = { el: popover, checkboxes }; + openVisibilityPopover = { el: popover, checkboxes, trigger: anchor }; + anchor.setAttribute('aria-expanded', 'true'); positionPopover(popover, anchor); @@ if (!btn) { @@ btn = document.createElement('button'); btn.type = 'button'; btn.className = 'column-visibility-btn'; + btn.setAttribute('aria-haspopup', 'dialog'); + btn.setAttribute('aria-expanded', 'false'); bar.appendChild(btn);Also applies to: 1669-1681, 1683-1697, 1766-1787
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/recommendations.ts` around lines 1660 - 1663, The Columns popover trigger should expose dialog state to assistive tech: on the popover trigger element (the element associated with VisibilityPopoverState.el / the Columns trigger in frontend/src/recommendations.ts) add aria-haspopup="dialog" and ensure aria-expanded is set to "true" when the popover opens and "false" when it closes; update the open/close/toggle handlers that show the popover (e.g., the functions that create/show and hide the VisibilityPopoverState.el or the toggle handler currently using aria-pressed) to set aria-expanded accordingly (and you may stop relying on aria-pressed for open state if present). Ensure both the initial render of the trigger and every path that hides/shows the popover update aria-expanded so screen readers receive consistent state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/styles/components.css`:
- Around line 907-910: The .recommendations-filter-status .sort-hidden-note rule
uses low-contrast color `#888` and small font-size; update the CSS to use a darker
color token/value (e.g., `#666` or your design system's dark-muted token) so the
note meets readability/accessibility contrast requirements while keeping the
existing font-size and font-style; locate the rule for
.recommendations-filter-status .sort-hidden-note and replace the color value
accordingly.
---
Duplicate comments:
In `@frontend/src/recommendations.ts`:
- Around line 1756-1761: The deferred attachment may add a stale
visOutsideClickHandler after closeVisibilityPopover() runs; to fix, capture the
handler as you do (visOutsideClickHandler) but before calling
document.addEventListener verify that the current visOutsideClickHandler still
equals that captured handler (or that the popover is still open) so you don't
attach an outdated listener—update the setTimeout callback in the block that
sets handler to perform this equality/check against visOutsideClickHandler (and
ensure closeVisibilityPopover clears visOutsideClickHandler when closing).
---
Nitpick comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 4752-4767: The test 'TOGGLEABLE_COLUMNS excludes fixed identity
columns' currently asserts a few inclusions/exclusions but not the full
contract; update it to assert the exact toggleable keys set by building an
expectedKeys array (including on_demand_monthly plus the other toggleables like
region, count, term, payment, savings, upfront_cost, monthly_cost,
effective_savings_pct) and then assert equality with TOGGLEABLE_COLUMNS.map(c =>
c.key) (either by comparing sorted arrays or checking length and set equality)
so the test pins the full contract for TOGGLEABLE_COLUMNS.
In `@frontend/src/recommendations.ts`:
- Around line 1660-1663: The Columns popover trigger should expose dialog state
to assistive tech: on the popover trigger element (the element associated with
VisibilityPopoverState.el / the Columns trigger in
frontend/src/recommendations.ts) add aria-haspopup="dialog" and ensure
aria-expanded is set to "true" when the popover opens and "false" when it
closes; update the open/close/toggle handlers that show the popover (e.g., the
functions that create/show and hide the VisibilityPopoverState.el or the toggle
handler currently using aria-pressed) to set aria-expanded accordingly (and you
may stop relying on aria-pressed for open state if present). Ensure both the
initial render of the trigger and every path that hides/shows the popover update
aria-expanded so screen readers receive consistent state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8491b282-a5e2-4a18-a952-4d8b09eb1712
📒 Files selected for processing (4)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsfrontend/src/state.tsfrontend/src/styles/components.css
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/state.ts
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
cristim
added a commit
that referenced
this pull request
May 11, 2026
- recommendations.ts: in openVisibilityPopover_, the deferred document.addEventListener could attach a stale outside-click handler if closeVisibilityPopover() ran in the same tick (clearing visOutsideClickHandler before the setTimeout callback fired). Guard the attach with a re-check that the popover is still open and the live handler still equals the captured one. - recommendations.test.ts: pin the full TOGGLEABLE_COLUMNS contract (include on_demand_monthly and assert length === 9) so future column additions can't quietly drift past the test. Addresses CodeRabbit feedback on PR #326.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
5 tasks
cristim
added a commit
that referenced
this pull request
May 11, 2026
PR #333 (closes #332) landed a botched merge of `unset_falls_through` in TestNewSenderFromEnvironment_EmailEnabled — two overlapping copies of the same sub-test ended up concatenated (one missing its closing braces, one preceded by stray fragments), plus duplicate `ctx :=` / `sender, err :=` lines. The result didn't parse: gofmt -l internal/email/factory_test.go internal/email/factory_test.go:309:2: missing ',' before newline ... internal/email/factory_test.go:312:6: expected '(', found TestNewSenderWithConfig_AWS This trips `gofmt` and `go vet` in the pre-commit workflow on every open PR against `feat/multicloud-web-frontend` (e.g. #326, #335, #336). Keep the `prev/hadPrev` version of the sub-test (the one that actually does `os.Unsetenv` first, which is the case the test name describes), drop the orphaned `orig/hadOrig` fragment, and remove the duplicate ctx/sender declarations. Verified locally: gofmt clean, `go vet ./internal/email/...` clean, `go test ./internal/email/...` 306 tests pass.
Replace three separate column enumerations (SORT_HEADER_LABELS, NUMERIC_COLUMNS, FILTERABLE_COLUMNS) with a single COLUMN_DEFS array that carries key, label, and kind (numeric | categorical) for each of the 12 data columns. Derive SORT_HEADER_LABELS and NUMERIC_COLUMNS from COLUMN_DEFS so future column additions/removals are a one-line edit in COLUMN_DEFS with no separate label or type maintenance. Introduce renderColumnCell() that maps a column key to its <td> HTML, and rework buildVariantRowMarkup to iterate COLUMN_DEFS (or a caller-provided subset) rather than a hardcoded template literal. The optional `cols` parameter (default COLUMN_DEFS) is the extension point for the per-column visibility feature (#318) landing in the next commit. Replace the hardcoded TABLE_COL_COUNT = 13 constant with `1 + COLUMN_DEFS.length` so the summary-row colspan stays correct automatically. Behaviour is unchanged: same columns, same order, same HTML output. All 1515 existing tests pass.
…opdown Add a "Columns ▾" button in the recommendations filter-status bar that opens a portal popover with per-column checkboxes for the 8 toggleable columns (region, count, term, payment, savings, upfront_cost, monthly_cost, effective_savings_pct). Provider / Account / Service / Resource Type are "cell identity anchors" and always shown (v1). - state.ts: hiddenColumns Set + getHiddenColumns/setHiddenColumns - TOGGLEABLE_COLUMNS / TOGGLEABLE_COLUMN_KEYS from COLUMN_DEFS - loadColumnVisibility / saveColumnVisibility with versioned localStorage schema (cudly.recs.columnVisibility.v1, schemaVersion: 1); unknown keys are whitelisted-out for forward compatibility - resetColumnVisibilityState() exported for tests (seed flag + state) - visibleColumns() computed once per renderRecommendationsList call, passed through to buildListMarkup + buildVariantRowMarkup - summaryColspan = 1 + visible-toggleable-count (replaces hardcoded TABLE_COL_COUNT - 4; TABLE_COL_COUNT removed as no longer needed) - mountColumnsButton: mount-once lifecycle, label shows "(N hidden)", aria-pressed reflects active state; re-syncs open-popover checkboxes - Sort-hidden indicator in filter-status bar warns when the sort column is currently hidden - Full keyboard accessibility: Escape closes and returns focus; Enter/ Space opens; click-outside dismisses - 14 new tests covering load/save/edge-cases via localStorageMock Closes #318
Update recommendation summary rows to only include values for visible columns, close the column visibility popover when the tab deactivates, and return a defensive copy of hidden column state.
Fixed 3 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…hidden-note Commit 64cc856 (CodeRabbit auto-fix) introduced two bugs in the .sort-hidden-note block: - backtick-quoted color value (`#666` → #666) - stray extra closing brace that broke webpack's CSS parser Both caused the frontend build and pre-commit checks to fail.
- recommendations.ts: in openVisibilityPopover_, the deferred document.addEventListener could attach a stale outside-click handler if closeVisibilityPopover() ran in the same tick (clearing visOutsideClickHandler before the setTimeout callback fired). Guard the attach with a re-check that the popover is still open and the live handler still equals the captured one. - recommendations.test.ts: pin the full TOGGLEABLE_COLUMNS contract (include on_demand_monthly and assert length === 9) so future column additions can't quietly drift past the test. Addresses CodeRabbit feedback on PR #326.
Add `aria-haspopup="dialog"` to the "Columns ▾" button at mount time, and toggle `aria-expanded` on the trigger as the popover opens/closes. Stash the trigger in VisibilityPopoverState so close-from-any-path (toggle, outside click, Escape, navigation tear-down) flips the attribute back. `aria-pressed` is intentionally kept for the orthogonal "any columns currently hidden?" indicator — different semantics from open/closed. Addresses CodeRabbit a11y nitpick on PR #326.
cristim
force-pushed
the
feat/recs-column-visibility
branch
from
be348de to
b665e1b
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
1 participant
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.
Summary
localStoragewith a versioned schema (cudly.recs.columnVisibility.v1); unknown/stale keys are silently filtered for forward compatibilityImplementation details
state.ts:hiddenColumnsSet +getHiddenColumns/setHiddenColumnsTOGGLEABLE_COLUMNS/TOGGLEABLE_COLUMN_KEYSderived fromCOLUMN_DEFS(Commit 1 refactor)loadColumnVisibility/saveColumnVisibilitywith whitelisting guardresetColumnVisibilityState()exported for test isolationvisibleColumns()computed once per render, passed tobuildListMarkup+buildVariantRowMarkupsummaryColspan= 1 + visible-toggleable-count (replaces hardcodedTABLE_COL_COUNT - 4)mountColumnsButton: mount-once lifecycle, label shows "(N hidden)",aria-pressedreflects active state; re-syncs open-popover checkboxes on each renderlocalStorageMockcovering all load/save edge-casesTest plan
npm run buildpasses with no TS errorsnpm test -- --testPathPattern=recommendations— 181 tests passCloses #318
Summary by CodeRabbit
New Features
Style
Tests