Fix: Bugs in valueset page #10738
Fix: Bugs in valueset page #10738
Conversation
WalkthroughThis pull request introduces enhancements across localization and valueset components. It adds new translation keys to the localization file and integrates internationalization into forms. The valueset editor's create mutation now navigates to a general admin page without using a data parameter, and redundant imports have been removed. The valueset form’s validation schema and UI messages are updated for multilingual support, while the valueset list leverages a new hook for dynamic query filtering and pagination. Additionally, the valueset type definition is expanded to accommodate extra status options. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant API
participant Router
User->>Editor: Submit new valueset
Editor->>API: Trigger createMutation
API-->>Editor: Return success
Editor->>Router: Navigate to admin valuesets page
sequenceDiagram
participant User
participant ListComp as ValueSetList
participant Filter as useFilters
participant API
participant Pager as Pagination
User->>ListComp: Load valuesets page
ListComp->>Filter: Retrieve query parameters (page, limit)
ListComp->>API: Fetch valuesets with queryParams
API-->>ListComp: Return valuesets data
ListComp->>Pager: Render pagination with total count
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…on fix on creating new
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/ValueSet/ValueSetList.tsx (1)
89-97: 🛠️ Refactor suggestionUpdate badge styling for new status values.
The badge styling only handles "active" status. Consider updating the styling to handle the new status values: "draft", "retired", and "unknown".
- className={ - valueset.status === "active" - ? "bg-green-100 text-green-800 hover:bg-green-200" - : "bg-red-100 text-red-800 hover:bg-red-200" - } + className={ + { + active: "bg-green-100 text-green-800 hover:bg-green-200", + draft: "bg-yellow-100 text-yellow-800 hover:bg-yellow-200", + retired: "bg-red-100 text-red-800 hover:bg-red-200", + unknown: "bg-gray-100 text-gray-800 hover:bg-gray-200", + }[valueset.status] + }
🧹 Nitpick comments (1)
public/locale/en.json (1)
1909-1911: New Translation Key for "retired" Status Added
The addition of"retired": "Retired"appears to correctly introduce a new label for the retired status, which aligns with the expanded status options on the ValueSet page. Please ensure that this key is referenced in the UI components (e.g., in the form validation or status dropdown) so that it displays properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(2 hunks)src/components/ValueSet/ValueSetEditor.tsx(1 hunks)src/components/ValueSet/ValueSetForm.tsx(7 hunks)src/components/ValueSet/ValueSetList.tsx(4 hunks)src/types/valueset/valueset.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (5)
src/types/valueset/valueset.ts (1)
29-29:❓ Verification inconclusive
Verify handling of new status values across the application.
The status options have been expanded from "active"/"inactive" to include "draft", "retired", and "unknown". This is a breaking change that requires verification.
Run this script to find all usages of the status field:
🏁 Script executed:
#!/bin/bash # Search for status field usage in components and tests rg -A 5 'status[^a-zA-Z].*(?:active|inactive|draft|retired|unknown)'Length of output: 21682
Review Update: Verify Status Handling Across Consumers
The status property in
src/types/valueset/valueset.tshas been updated from"active" | "inactive"to"active" | "draft" | "retired" | "unknown". Our verification shows that:
Valueset Components
src/components/ValueSet/ValueSetForm.tsxnow usesz.enum(["active", "draft", "retired", "unknown"]), reflecting the new options.src/components/ValueSet/ValueSetList.tsxchecks ifvalueset.status === "active"and applies alternative styling otherwise.Other Usages
- Multiple files (e.g., locale files and several questionnaire components) still reference legacy statuses such as
"inactive". However, these appear to be in a different context (e.g., locations or questionnaire statuses) than the updated valueset type.Action Required:
- Review and Confirm: Verify that all consumers of the updated valueset status are handling the new states (
"draft","retired", and"unknown") correctly. In particular, double-check any binary logic (e.g., comparisons solely against"active") to ensure they address the expanded lifecycle adequately.- Testing: Ensure thorough testing of valueset workflows with the full set of status options to catch any unforeseen side-effects.
src/components/ValueSet/ValueSetEditor.tsx (1)
37-40: LGTM! Navigation flow simplified.The change to navigate directly to the valuesets list after creation is a good improvement that simplifies the user flow.
src/components/ValueSet/ValueSetList.tsx (1)
19-21: LGTM! Pagination implemented correctly.The useFilters hook is properly configured with a default limit of 15 items per page.
src/components/ValueSet/ValueSetForm.tsx (2)
347-401: LGTM! Comprehensive i18n integration.The form has been properly internationalized with:
- Translated validation messages
- Translated field labels
- Proper handling of all status options
425-425: LGTM! Clear indication of required fields.The required attribute has been correctly added to mandatory fields: name, slug, and status.
Also applies to: 439-439, 467-467
|
@nihal467, I’ve made the changes based on my understanding of the expected behaviors. Please confirm if these changes align with the expected behavior for bugs other than the pagination one. If they do, kindly acknowledge this comment. Thanks! |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
1 similar comment
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/ValueSet/ValueSetList.tsx (4)
30-38: Add error handling to the query configuration.While the query setup is correct, consider adding error handling to improve user experience.
const { data: response, isLoading } = useQuery({ queryKey: ["valuesets", qParams], queryFn: query(valuesetApi.list, { queryParams: { limit: resultsPerPage, offset: ((qParams.page ?? 1) - 1) * resultsPerPage, }, }), + onError: (error) => { + // Handle error appropriately (e.g., show toast notification) + console.error('Failed to fetch valuesets:', error); + } });
111-113: Optimize description cell CSS classes.The description cell has potentially redundant CSS classes. Consider simplifying:
-<TableCell className="px-6 py-4 truncate text-sm text-gray-900 break-words whitespace-normal"> +<TableCell className="px-6 py-4 text-sm text-gray-900 break-words">The
truncateclass is unnecessary when usingbreak-wordsandwhitespace-normal.
96-109: Consider extracting status badge styles and translations.The current implementation directly uses status values as translation keys and inline styles. Consider extracting these into constants for better maintainability.
+const STATUS_STYLES = { + active: "bg-green-100 text-green-800 hover:bg-green-200", + draft: "bg-yellow-100 text-yellow-800 hover:bg-yellow-200", + retired: "bg-red-100 text-red-800 hover:bg-red-200", + unknown: "bg-gray-100 text-gray-800 hover:bg-gray-200", +} as const; + +const STATUS_TRANSLATION_KEYS = { + active: "status.active", + draft: "status.draft", + retired: "status.retired", + unknown: "status.unknown", +} as const; <Badge - className={ - { - active: "bg-green-100 text-green-800 hover:bg-green-200", - draft: "bg-yellow-100 text-yellow-800 hover:bg-yellow-200", - retired: "bg-red-100 text-red-800 hover:bg-red-200", - unknown: "bg-gray-100 text-gray-800 hover:bg-gray-200", - }[valueset.status] - } + className={STATUS_STYLES[valueset.status]} > - {t(valueset.status)} + {t(STATUS_TRANSLATION_KEYS[valueset.status])} </Badge>
135-135: Add loading state to pagination.Consider disabling the pagination component while data is loading to prevent user interaction during fetch operations.
-<Pagination totalCount={response?.count ?? 0} /> +<Pagination totalCount={response?.count ?? 0} disabled={isLoading} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/locale/en.json(2 hunks)src/components/ValueSet/ValueSetEditor.tsx(4 hunks)src/components/ValueSet/ValueSetForm.tsx(7 hunks)src/components/ValueSet/ValueSetList.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (9)
src/components/ValueSet/ValueSetList.tsx (2)
8-15: LGTM! Well-structured imports and hook setup.The imports are logically organized, and the useFilters hook is properly configured with a reasonable default limit of 15 items per page.
Also applies to: 19-19, 27-29
27-29: LGTM! Pagination implementation meets PR objectives.The pagination implementation is well-structured with:
- Proper hook setup with default limit
- Correct query parameter handling
- Integration with the UI through the Pagination component
Also applies to: 30-38, 135-135
src/components/ValueSet/ValueSetEditor.tsx (2)
37-40: LGTM! Navigation simplified after valueset creation.The change simplifies the navigation logic by directly routing to the valuesets list page instead of a specific valueset page, which is a more consistent approach.
48-52: Cache invalidation ensures UI consistency.Good addition of cache invalidation after successful update. This ensures the valuesets list is refreshed with the latest data.
src/components/ValueSet/ValueSetForm.tsx (5)
5-5: LGTM! i18n support added correctly.The useTranslation hook is properly imported and initialized for internationalization support.
Also applies to: 347-347
475-479: Status options expanded appropriately.The status options have been expanded to include 'draft', 'retired', and 'unknown', replacing the previous 'inactive' option. This provides more granular control over the valueset lifecycle.
425-425: Required field indicators added consistently.Good addition of required field indicators for name, slug, and status fields. This improves form usability by clearly indicating which fields are mandatory.
Also applies to: 439-439, 453-453, 467-467
492-492: Button text internationalized.The submit button text is now properly internationalized using the translation function.
349-401:✅ Verification successful
Verify translation keys exist in locale files.
The schema now uses translation keys for validation messages. Please ensure all these keys exist in the locale files:
- field_required
- name
- slug
- description
- status
- active
- draft
- retired
- unknown
- saving
- save_valueset
🏁 Script executed:
#!/bin/bash # Check if all required translation keys exist in locale files rg -l "field_required|name|slug|description|status|active|draft|retired|unknown|saving|save_valueset" public/locale/Length of output: 250
Translation Keys Verified in Locale Files
The verification script indicates that all required translation keys:
- field_required
- name
- slug
- description
- status
- active
- draft
- retired
- unknown
- saving
- save_valueset
are present across the locale files (
public/locale/en.json,kn.json,mr.json,ml.json,ta.json,hi.json). No further changes are necessary.
| TableHead, | ||
| TableHeader, | ||
| TableRow, | ||
| } from "@/components/ui/table"; |
There was a problem hiding this comment.
@AdityaJ2305 u can remove this , i have added this in my UI enhancement PR #10712
|
@rithviknishad @Jacobjeevan , Ready for review and testing PS: #10695 (comment) |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Refactor