Repository navigation
feat(app): remember collapsed pinned filters (#3227) - #3605
kodiakhq[bot] merged 3 commits into
Conversation
A pinned field collapsed in the search sidebar now stays collapsed the next time the source loads, instead of always opening. The state is personal and per source, in localStorage (hdx-collapsed-pinned-fields). A pinned field with an active filter still opens.
🦋 Changeset detectedLatest commit: 8576157 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
Hi @bferreirq, thanks for the pull request! Before we review code from a first-time contributor, a maintainer vouches for them, and you're not on our list yet. There's nothing else to file and this PR stays open — a maintainer will look at it, usually within a day or two, and vouch for you right here if it's a genuine attempt at the problem. To make that quick, check the PR description has:
More detail in our contributing guide. |
| hasSelection) | ||
| ); | ||
| })()} | ||
| onExpandedChange={expanded => { |
There was a problem hiding this comment.
🔵 minor — Collapsed state stays stored after the field is unpinned, so it comes back when the field is pinned again
onExpandedChange only writes while the field is pinned, and nothing removes the entry when the pin goes away. That includes toggleFieldPin, toggleSharedFieldPin and resetPersonalPins in searchFilters.tsx. So if you pin X, collapse it, unpin it, and later pin X again, it reloads collapsed even though you never collapsed it as a pinned field that time. Old entries also pile up in hdx-collapsed-pinned-fields. Fix: call setPinnedFieldCollapsed(key, false) when a field is unpinned and on reset, or ignore stored entries for fields that are no longer pinned.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Fixed in f1a4b17: unpinning a field (once no personal or shared pin is left) or resetting pins now clears its collapsed state, so pinning it again opens it.
| expect(result[sourceB]).toEqual({ level: ['error'] }); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🔵 minor — Tests only cover the localStorage hook, not the new expand rules in the sidebar
The new tests only check useCollapsedPinnedFields. The real behaviour lives in the isDefaultExpanded / onExpandedChange changes in DBSearchPageFilters.tsx, and none of it is tested: a collapsed pinned field renders collapsed, it still opens when it has a selection, the collapsed state now takes priority over forceExpanded in the Shared Filters section, and unpinned fields are never stored. Any of these could break and the tests would still pass. Add a component test that renders DBSearchPageFilters with a pinned field, collapses it, remounts, and checks the filter-group-control expanded state, both with and without an active filter.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Addressed in f1a4b17: the expand-on-load rule now lives in getFilterGroupDefaultExpanded, with tests for a collapsed pinned field staying closed, an active filter still opening it, precedence over the Shared Filters forceExpanded, and stale entries ignored once unpinned. I didn't add a full DBSearchPageFilters render test since no existing test mounts the whole sidebar (it needs metadata, facets and pinned-filters API mocks); happy to add one if a maintainer wants it.
PR Review
2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor 1 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. The core logic was traced end-to-end by the correctness, kieran-typescript, and julik-frontend-races reviewers and holds up: the 🔵 P3 nitpicks (3)
Reviewers (9): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, kieran-typescript, julik-frontend-races, previous-comments. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
- Unpinning a field (personal or shared, once no pin is left) or resetting pins clears its collapsed state, so pinning it again opens it. - Move the expand-on-load rule to getFilterGroupDefaultExpanded and test it: collapsed pinned field stays closed, active filter still opens it, it wins over the Shared Filters section's forceExpanded, stale entries are ignored once unpinned.
|
@bferreirq is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
|
|
||
| const makeFieldPins = (key: string): FieldPinHandlers => ({ | ||
| onFieldPinClick: () => toggleFieldPin(key), | ||
| onFieldPinClick: () => { |
There was a problem hiding this comment.
🔵 minor — Collapsed state is only cleared on unpins made in this tab, so a re-pinned field can come back collapsed
The cleanup runs inside four hand-wired handlers: the two unpin handlers here and the two reset handlers at lines 1794–1802. Shared pins are stored on the team (usePinnedFiltersApi, searchFilters.tsx:503), so the cleanup never runs in these cases: a teammate unpins a shared field, another tab unpins it, or someone unpins through the NestedFilterGroup handlers at lines 1608–1610. The stale entry stays in hdx-collapsed-pinned-fields. When the field is pinned again, getFilterGroupDefaultExpanded sees isPinned && isCollapsed and opens it collapsed, which goes against the comment's promise that re-pinning "opens it like any newly pinned field". Fix: put the rule in one place instead of in the callers. Either clear the entry when a field becomes pinned, or have useCollapsedPinnedFields drop entries that are not in the merged pinned set (getPinnedFields). Then remove the four call-site checks.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
brandon-pereira
left a comment
There was a problem hiding this comment.
LGTM - thanks for the contribution!
|
/vouch @bferreirq |
Triggered by [comment](#3605 (comment)) from @brandon-pereira. Vouch: @bferreirq
Summary
Closes #3227
We pin a few fields on our main logs source, and every time we open search they all come back expanded, so we collapse them again each time. This keeps a pinned field collapsed across reloads.
useCollapsedPinnedFields(sourceId)hook insearchFilters.tsx: per-source list of collapsed pinned fields in localStorage (hdx-collapsed-pinned-fields), personal like the other sidebar expand states (hdx-filters-expanded,hdx-shared-filters-expanded).LogAttributes.*, ...) are unchanged: they keep their own expand logic. Happy to follow up if wanted.Screenshots or video
Pinned
SeverityText, collapsed, then page reloaded (yarn app:dev:local, demo ClickHouse,otel.otel_logs).How to reproduce
Starting point:
yarn app:dev:localconnected to the demo server, Logs source on/search.On
main:SeverityTextfield (pin icon on its row).SeverityText.SeverityTextis expanded again.On this branch:
SeverityTextstays collapsed.How to test on Vercel preview
Preview routes: /search
Steps:
SeverityTextfield.SeverityTextheader to collapse it.SeverityTextfilter group is collapsed.References
packages/app/src/__tests__/pinnedFilters.test.ts(state survives a remount, cleared on expand, kept per source, nothing stored without a source)