Skip to content

ECHOES-1452 Add and rely on an id for FilterDropdownCategory - #756

Merged
kimphanh merged 1 commit into
mainfrom
kim/filter-category-id/ECHOES-1452
Aug 26, 2026
Merged

ECHOES-1452 Add and rely on an id for FilterDropdownCategory#756
kimphanh merged 1 commit into
mainfrom
kim/filter-category-id/ECHOES-1452

Conversation

@kimphanh

@kimphanh kimphanh commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Part of


Summary by Gitar

  • Components:
    • Added required id property to FilterDropdownCategory types and updated component keys and callbacks to rely on it instead of label

This will update automatically on new commits.

@netlify

netlify Bot commented Aug 25, 2026

Copy link
Copy Markdown

Deploy Preview for echoes-react ready!

Name Link
🔨 Latest commit d29ffd8
🔍 Latest deploy log https://app.netlify.com/projects/echoes-react/deploys/6a8da93a6adf300008014cf0
😎 Deploy Preview https://deploy-preview-756--echoes-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Aug 25, 2026

Copy link
Copy Markdown

ECHOES-1452

Comment thread src/components/filters/FilterDropdown.tsx
@gitar-bot

gitar-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 1 resolved / 1 findings

Adds a required id property to FilterDropdownCategory for component keys and callbacks. Consider updating the right panel to use the new id instead of the active category index.

✅ 1 resolved
Quality: Right panel still keyed by index after id was introduced

📄 src/components/filters/FilterDropdown.tsx:88 📄 src/components/filters/FilterDropdown.tsx:110 📄 src/components/filters/FilterDropdown.tsx:253 📄 src/components/filters/FilterDropdownTypes.ts:62-66 📄 src/components/filters/FilterDropdownTypes.ts:115-119
The PR makes id the category identity (React key at line 240, onCategorySelect payload at lines 98/111), but the right panel is still remounted on key={activeCategoryIndex} and the active category is still resolved by index (categories[activeCategoryIndex]). If a consumer reorders or prepends to categories while the popover is open, the index stays the same, so the key does not change and the right panel keeps the previous category's internal state (search query, item focus) while rendering the new category's items. Keying on the stable activeCategory?.id makes the panel reset consistent with the identity this PR introduces.

Implementation Status ✅ 2 of 2 objectives covered
ECHOES-1452 - 2 of 2 objectives covered

This PR adds an id property to FilterDropdownCategory and updates FilterDropdown to rely on the category id instead of its label.

✅ 2 covered here
  • ✅ Add an id property to FilterDropdownCategory
  • ✅ Rely on the category id instead of label in FilterDropdown
Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

@kimphanh
kimphanh marked this pull request as ready for review August 26, 2026 10:00
@kimphanh
kimphanh requested a review from gregaubert August 26, 2026 10:00

@gregaubert gregaubert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great!

@kimphanh
kimphanh merged commit d977342 into main Aug 26, 2026
15 checks passed
@kimphanh
kimphanh deleted the kim/filter-category-id/ECHOES-1452 branch August 26, 2026 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants