FEATURE/HCMPRE-1712: Facility Dropdown for plan Inbox #2096
Conversation
…/DIGIT-Frontend into FEATURE/HCMPRE-1709
|
Warning Rate limit exceeded@abishekTa-egov has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request enhances the Changes
Sequence DiagramsequenceDiagram
participant UI as InboxFilterWrapper
participant Hook as useQueryParams
participant Service as ULBService
participant API as Facility API
UI->>Hook: Retrieve query parameters
UI->>Service: Get current tenant ID
UI->>API: Fetch facility data
API-->>UI: Return facility list
UI->>UI: Transform facility options
UI->>UI: Render facility dropdown
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
abishekTa-egov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js (1)
Pattern **/*.js: check
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js (1)
Learnt from: Ramkrishna-egov
PR: egovernments/DIGIT-Frontend#1562
File: health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js:57-57
Timestamp: 2024-11-12T10:40:17.779Z
Learning: In the `InboxFilterWrapper` component, `optionsKey` should always be set to `"name"` because options are transformed and created within the component.
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js (2)
3-3: Import usage verified.
TheLoaderimport appears to be properly utilized (see lines 134-136), so this addition is valid.
11-12: Consider input validation or fallback for query parameters.
Currently,url?.microplanIdis used directly inplanFacilitySearchConfig. Ensure this property is always defined or provide a fallback ifurl?.microplanIdis absent, to avoid potential runtime errors.
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Outdated
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Outdated
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Outdated
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js (1)
Pattern **/*.js: check
🔇 Additional comments (6)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js (6)
3-3: Use the newly introduced Loader more selectively.
Great job leveraging the Loader for partial updates. Ensure you only block the facility-related portion so users can still interact with other filters.
54-54: No actions needed.
This blank line seems intentional for code clarity.
122-122: Add error handling for API call.
You already destructurepanFacilityerror, but it isn’t handled. Consider adding a fallback UI or error state for a more robust user experience.
177-190: Partial loading is properly implemented.
Nicely done showing a loader only for facilities. This preserves access to the other filters while data is fetched.
98-120: 🧹 Nitpick (assertive)Ensure consistent data shape for facility dropdown.
You are mapping{ code: facility.facilityName, Id: facility.facilityId }. Elsewhere, dropdowns often viewcodeas an ID andnameas a display label. Consider reversing them to avoid confusion in future maintenance.- const facilityOptions = data.PlanFacility.map((facility) => ({ - code: facility.facilityName, - Id: facility.facilityId - })); + const facilityOptions = data.PlanFacility.map((facility) => ({ + code: facility.facilityId, + name: facility.facilityName, + }));Likely invalid or redundant comment.
12-12: Validate tenant ID usage.
Confirm that the tenant ID returned byDigit.ULBService.getCurrentTenantId()always matches the tenant you're filtering on. If you expect to override it in certain contexts, consider receiving it via props.✅ Verification successful
Let me analyze the usage of
getCurrentTenantId()to verify if it's being used consistently for filtering.
Let me check how the tenantId is being used in the InboxFilterWrapper component and its related filter logic.
Based on the code analysis, I can now provide a final response about the tenant ID usage:
The tenant ID usage is appropriate and consistent with filtering requirements
The tenant ID from
getCurrentTenantId()is being used correctly in the InboxFilterWrapper component as part of the plan facility search criteria. Specifically:
- It's used in the
planFacilitySearchConfigto scope the facility search to the current tenant:PlanFacilitySearchCriteria: { tenantId: tenantId, planConfigurationId: url?.microplanId, }
- The tenant ID is combined with other filter parameters and used alongside the microplan ID to ensure proper scoping of facility data
- The implementation follows the standard pattern where tenant ID is used as a mandatory filter parameter for data access
The current implementation ensures that facility data is properly filtered by tenant context, and there's no need to override it via props since it's correctly scoped to the user's current tenant context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to getCurrentTenantId usage rg -A 5 "getCurrentTenantId"Length of output: 68613
Script:
#!/bin/bash # Search for tenantId filter usage in the file and related components rg -A 5 "tenantId.*filter" health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/ # Search for any filter-related code in the InboxFilterWrapper component ast-grep --pattern 'const $_ = { $$$ tenantId: $_, $$$ }' health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.jsLength of output: 3970
Script:
#!/bin/bash # Check the InboxFilterWrapper component implementation cat health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js # Search for any filter-related hooks or context usage rg -A 5 "useFilter|FilterContext" health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Length of output: 8135
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Outdated
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Outdated
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Outdated
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js (1)
Pattern **/*.js: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
[error] 119-119: Use Number.POSITIVE_INFINITY instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.POSITIVE_INFINITY instead.
(lint/style/useNumberNamespace)
🔇 Additional comments (5)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js (5)
3-3: Appropriate import for partial loading
Bringing in “Loader” from the digit-ui-components library is a suitable approach to handle loading states in a modular way.
54-54: No code changes here
No meaningful changes detected in this blank line.
112-115: Ensure consistent data shape in dropdown options
The facility dropdown expects a uniform structure (typically{ code, name }). Currently, “code“ holds the facility name while “Id” holds the facility ID. Aligning them with standard conventions helps reduce confusion.- code: facility.facilityName, - Id: facility.facilityId + name: facility.facilityName, + code: facility.facilityId
123-123: Handle API errors
planFacilityerrorremains unused. Displaying an error fallback or message would improve resilience.const { isLoading: isPlanFacilityLoading, error: planFacilityerror, data: planFacility } = Digit.Hooks.useCustomAPIHook(planFacilitySearchConfig); + if (planFacilityerror) { + return <div style={{ color: "red" }}>{t("PLAN_FACILITY_ERROR")}</div>; + }
179-191: Partial loader implementation approved
Showing a loader specifically for facility data while preserving access to other filters is a user-friendly approach. Nicely done!
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/InboxFilterWrapper.js
Outdated
Show resolved
Hide resolved
nipunarora-eGov
changed the title
* Adding filters * Working Filter Search * InboxFilter Changes * removing console.log * Css package update * changes * changes * changes * changes * changes * A lot of changes * changes * changes * changes * uniform payload structure * Adding Facility Dropdown * code replacing name attribute * Loader changes * comments resolved * Loader change * Id to id
FEATURE/HCMPRE-1712: Filter for facility and drop down for filter
Summary by CodeRabbit