Add loading status to site settings data related dropdown#1446
Add loading status to site settings data related dropdown#1446
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1446 +/- ##
==========================================
- Coverage 47.21% 46.91% -0.30%
==========================================
Files 272 274 +2
Lines 9720 9783 +63
Branches 2862 2895 +33
==========================================
+ Hits 4589 4590 +1
- Misses 4900 4959 +59
- Partials 231 234 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Do we know why the site-settings response is slow? Can we address that as well? |
it might be related to both frontend and backend. in another branch I have some updates on reducing calls of site-settings, once that got merged, I'll updated related pages to use context provider instead of making new calls every page |
|
Claude finished @erinz2020's task in 1m 38s —— View job PR ReviewI've completed a review of this PR that adds loading spinners to site settings-dependent dropdowns. Todo List
SummaryThis PR successfully addresses the issue where slow site-settings responses cause dropdowns to briefly render with empty options. The solution adds loading spinners to affected selects on the Encounter and Search Encounter pages, providing clear visual feedback that options are still loading. Implementation ReviewNew Components:
Store Changes: Both
This is a good pattern that allows components to reactively update when site settings finish loading. Component Integration: The implementation wraps affected dropdowns with
Strengths
Observations & Suggestions
Minor IssuesMatchCriteria.jsx - This file appears to have the most lines missing coverage (18 missing + 2 partials). Consider reviewing the conditional logic in this file. GroupRole field (AttributesSectionEdit.jsx:58-62) - I notice the "GROUP_ROLE" field is a TextInput, not wrapped in ContainerWithSpinner, while other fields like "BEHAVIOR" are. Is this intentional? If groupRole should also be populated from site settings, it should probably be wrapped as well. VerdictThis is a solid PR that addresses a real UX issue with a clean, consistent solution. The implementation is straightforward and follows React best practices. The main areas for improvement are around test coverage and ensuring complete consistency in which fields are wrapped with the loading spinner. The PR is ready to merge from a code quality perspective. The 47% → 46.91% coverage decrease is minimal and expected for UI-focused changes. | |
…tings_data_related_dropdown Add loading status to site settings data related dropdown
in some environments, the site-settings response can be slow, causing certain dropdowns to briefly render with empty options. this PR adds loading spinners to the affected selects on the Encounter and Search Encounter pages to clearly indicate the options are still loading and avoid confusion
PR fixes #1443