Skip to content

Move section collapse state to URL query params#1958

Open
alex-fedotyev wants to merge 4 commits intomainfrom
feat/url-based-collapse-state
Open

Move section collapse state to URL query params#1958
alex-fedotyev wants to merge 4 commits intomainfrom
feat/url-based-collapse-state

Conversation

@alex-fedotyev
Copy link
Contributor

@alex-fedotyev alex-fedotyev commented Mar 21, 2026

Summary

  • Section expand/collapse is now tracked in URL query params (collapsed/expanded) instead of persisting to the DB on every chevron click
  • The DB-stored collapsed field on DashboardContainer becomes the default fallback — what viewers see when opening a dashboard fresh (no URL state)
  • Chevron click updates URL state only (per-viewer, shareable via link)
  • "Collapse by Default" / "Expand by Default" menu action in the section header saves to the DB (via setDashboard), setting the default for all viewers
  • SectionHeader now accepts separate collapsed/defaultCollapsed props and onToggle/onToggleDefaultCollapsed handlers
  • Adds 7 unit tests for SectionHeader

Implements Drew's review feedback on PR #1926:

IMO, expanding/collapsing should not be persisted to the dashboard UNLESS this option is used. [...] I think it would be nice to persist normal expand collapse states in the URL, and then fallback to the default state (saved in the DB based on this option here) if there is no URL state.

Demo

Section collapse via URL params demo

Shows: expanded sections → chevron click collapses first section (URL updates to ?collapsed=...) → menu shows "Collapse by Default" (DB action, separate from view state)

Test plan

  • Open a dashboard with sections — collapse/expand via chevron click, verify URL updates (?collapsed=... / ?expanded=...) without saving to DB
  • Copy the URL with collapse state and open in a new tab — verify sections reflect the URL state
  • Open the section menu and click "Collapse by Default" — verify this saves to DB (persists after page refresh without URL params)
  • Verify "Expand by Default" / "Collapse by Default" label reflects the DB default, not current view state
  • Run yarn ci:unit --testPathPatterns='SectionHeader' — all 7 tests pass

🤖 Generated with Claude Code

Section expand/collapse state is now tracked in URL query params
(collapsed/expanded) instead of persisting directly to the DB on
every toggle. The DB-stored collapsed field on DashboardContainer
becomes the default fallback for when someone opens a dashboard
fresh (no URL state).

- Chevron click updates URL state only (per-viewer, shareable)
- "Collapse by Default" / "Expand by Default" menu action saves to DB
- SectionHeader accepts separate collapsed/defaultCollapsed props
  and onToggle/onToggleDefaultCollapsed handlers
- Add 7 unit tests for SectionHeader
@changeset-bot
Copy link

changeset-bot bot commented Mar 21, 2026

⚠️ No Changeset found

Latest commit: f092446

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Mar 21, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Mar 23, 2026 7:09pm

Request Review

@alex-fedotyev alex-fedotyev marked this pull request as draft March 21, 2026 00:38
@github-actions
Copy link
Contributor

github-actions bot commented Mar 21, 2026

PR Review

  • ⚠️ Stale URL state on section deletehandleDeleteSection doesn't remove the deleted section's ID from urlCollapsedIds/urlExpandedIds. IDs are random so collisions are near-impossible, but deleted section IDs accumulate as URL cruft. Add removeFromUrlSet(setUrlCollapsedIds, containerId) and removeFromUrlSet(setUrlExpandedIds, containerId) calls inside handleDeleteSection.

  • ⚠️ isSectionCollapsed(section) called twice per section in render (lines 1827 and 1839) → Extract to a const isCollapsed = isSectionCollapsed(section) at the top of the sections.map callback to avoid double computation.

  • ℹ️ DBDashboardPage.tsx is 1921 lines, well over the 300-line guideline in AGENTS.md — pre-existing issue, but this PR adds ~60 more lines to it.

Otherwise the implementation is clean: URL state logic with nuqs is correct, the collapsed/expanded dual-set approach properly handles overrides in both directions, tests cover the key cases, and the separation of URL-state vs DB-default concerns is well-structured.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 21, 2026

E2E Test Results

All tests passed • 91 passed • 3 skipped • 959s

Status Count
✅ Passed 91
❌ Failed 0
⚠️ Flaky 2
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@alex-fedotyev alex-fedotyev marked this pull request as ready for review March 21, 2026 00:45
@alex-fedotyev alex-fedotyev requested review from a team and knudtty and removed request for a team March 21, 2026 00:55
@alex-fedotyev alex-fedotyev self-assigned this Mar 21, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 23, 2026

Knip - Unused Code Analysis

0 change in total issues (240 on main → 240 on PR)

Category main PR Diff
Unused files 9 9 0
Unused dependencies 14 14 0
Unused devDependencies 20 20 0
Unlisted dependencies 14 14 0
Unresolved imports 2 2 0
Unlisted binaries 2 2 0
Unused exports 132 132 0
Unused exported types 41 41 0
Unused enum members 2 2 0
Duplicate exports 4 4 0
What is this?

Knip finds unused files, dependencies, and exports in your codebase.
This comment compares the PR branch against main to detect regressions.

Run yarn knip locally to see full details.

@@ -1102,16 +1102,11 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) {
const [editedTile, setEditedTile] = useState<undefined | Tile>();

const onAddTile = (containerId?: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this whole hook should be moved below the isSectionCollapsed hook to avoid potential weird behavior due to hoisting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done — moved onAddTile below isSectionCollapsed and handleToggleSection so it can reference both without hoisting concerns. See c72626e.

Comment on lines +1332 to +1351
setUrlExpandedIds(prev => {
const next = [...(prev ?? [])];
if (!next.includes(containerId)) next.push(containerId);
return next.length > 0 ? next : null;
});
setUrlCollapsedIds(prev => {
const next = (prev ?? []).filter(id => id !== containerId);
return next.length > 0 ? next : null;
});
} else {
// Collapsing: add to collapsed list, remove from expanded list
setUrlCollapsedIds(prev => {
const next = [...(prev ?? [])];
if (!next.includes(containerId)) next.push(containerId);
return next.length > 0 ? next : null;
});
setUrlExpandedIds(prev => {
const next = (prev ?? []).filter(id => id !== containerId);
return next.length > 0 ? next : null;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

These functions are similar enough they should be abstracted.
1 function that pushes a new container id to the existing array
1 function that filters out the old containerId

Additionally, it would be worth using produce via immer as we do elsewhere in the app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done — extracted addToUrlSet and removeFromUrlSet helpers using produce via immer. handleToggleSection now calls these instead of duplicating the add/filter logic, and uses isSectionCollapsed directly instead of re-implementing the lookup. See c72626e.

…with immer

- Move onAddTile below isSectionCollapsed/handleToggleSection to avoid
  hoisting issues (knudtty review comment)
- Extract addToUrlSet/removeFromUrlSet helpers using immer produce,
  eliminating duplicated add/filter logic in handleToggleSection
- Use isSectionCollapsed directly instead of re-implementing the lookup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants