Added Search By Title and status filters to questionnaires page #10591
Conversation
|
Warning Rate limit exceeded@hrit2773 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 39 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 ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThis pull request introduces changes to enhance user interaction with questionnaires. It adds three new entries in the English locale file for improved user prompts regarding filter adjustments, search functionality, and status indication. Additionally, the Changes
Possibly related PRs
Suggested labels
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Nitpick comments (2)
src/components/Questionnaire/index.tsx (2)
44-48: Consider adding type safety for qParams.The destructured qParams object lacks TypeScript type definitions which could lead to runtime errors.
+interface QueryParams { + status?: string; + title?: string; + page?: number; +} -const { qParams, updateQuery, Pagination, resultsPerPage } = useFilters({ +const { qParams, updateQuery, Pagination, resultsPerPage } = useFilters<QueryParams>({
62-70: Consider extracting search options to a constant.The searchOptions array could be moved outside the component to prevent recreating it on every render.
+const QUESTIONNAIRE_SEARCH_OPTIONS = [ + { + key: "title", + label: "Title", + type: "text" as const, + placeholder: t("search_by_questionnaire_title"), + }, +]; export function QuestionnaireList() { // ... const searchOptions = [ - { - key: "title", - label: "Title", - type: "text" as const, - placeholder: t("search_by_questionnaire_title"), - value: title || "", - }, + { + ...QUESTIONNAIRE_SEARCH_OPTIONS[0], + value: title || "", + }, ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(4 hunks)src/components/Questionnaire/index.tsx(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint Code Base
src/components/Questionnaire/index.tsx
[error] 1-1: Unresolved import: '@/src/components/Common/SearchByMultipleFields'
[error] 2-2: Unresolved import: '@/src/components/Common/SkeletonLoading'
[error] 3-3: Unresolved import: '@/src/components/ui/popover'
[error] 4-4: Unresolved import: '@/src/components/ui/tabs'
🪛 GitHub Actions: Cypress Tests
src/components/Questionnaire/index.tsx
[error] 1-1: Could not load /home/runner/work/care_fe/care_fe/src/src/components/Common/SearchByMultipleFields: ENOENT: no such file or directory.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: eslint
- GitHub Check: OSSAR-Scan
🔇 Additional comments (5)
src/components/Questionnaire/index.tsx (4)
27-41: LGTM! Well-structured EmptyState component.The EmptyState component is well-designed with clear visual hierarchy and proper use of i18n translations.
86-127: LGTM! Well-implemented search functionality.The search implementation using Popover is clean and follows best practices:
- Proper keyboard handling with
onEscapeKeyDown- Clear visual feedback with icons
- Debounced search updates
128-182: LGTM! Well-structured filter tabs implementation.The filter tabs implementation is clean and follows best practices:
- Clear visual states for active/inactive tabs
- Consistent state management with updateQuery
- Proper type handling for status values
184-245: LGTM! Well-implemented loading and empty states.The component handles different states appropriately:
- Shows skeleton loader during loading
- Shows empty state when no results
- Clean table implementation for results
public/locale/en.json (1)
328-329: LGTM! Well-structured translations.The new translation entries are:
- Properly formatted and consistent with existing entries
- Support the new search and filter functionality
- Provide clear user guidance
Also applies to: 1915-1915
| import { cn } from "@/lib/utils"; | ||
|
|
||
| import CareIcon from "@/CAREUI/icons/CareIcon"; | ||
|
|
||
| import { Badge } from "@/components/ui/badge"; | ||
| import { Button } from "@/components/ui/button"; | ||
|
|
||
| import Loading from "@/components/Common/Loading"; | ||
| import { Card } from "@/components/ui/card"; | ||
|
|
||
| import useFilters from "@/hooks/useFilters"; | ||
|
|
||
| import query from "@/Utils/request/query"; | ||
| import SearchByMultipleFields from "@/src/components/Common/SearchByMultipleFields"; | ||
| import { TableSkeleton } from "@/src/components/Common/SkeletonLoading"; | ||
| import { | ||
| Popover, | ||
| PopoverContent, | ||
| PopoverTrigger, | ||
| } from "@/src/components/ui/popover"; | ||
| import { Tabs, TabsList, TabsTrigger } from "@/src/components/ui/tabs"; |
There was a problem hiding this comment.
Fix import paths to resolve pipeline failures.
The import statements are using incorrect paths which is causing build failures. The imports should not include /src/ in the path when importing from the source directory.
-import SearchByMultipleFields from "@/src/components/Common/SearchByMultipleFields";
-import { TableSkeleton } from "@/src/components/Common/SkeletonLoading";
-import { Popover, PopoverContent, PopoverTrigger } from "@/src/components/ui/popover";
-import { Tabs, TabsList, TabsTrigger } from "@/src/components/ui/tabs";
+import SearchByMultipleFields from "@/components/Common/SearchByMultipleFields";
+import { TableSkeleton } from "@/components/Common/SkeletonLoading";
+import { Popover, PopoverContent, PopoverTrigger } from "@/components/ui/popover";
+import { Tabs, TabsList, TabsTrigger } from "@/components/ui/tabs";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { cn } from "@/lib/utils"; | |
| import CareIcon from "@/CAREUI/icons/CareIcon"; | |
| import { Badge } from "@/components/ui/badge"; | |
| import { Button } from "@/components/ui/button"; | |
| import Loading from "@/components/Common/Loading"; | |
| import { Card } from "@/components/ui/card"; | |
| import useFilters from "@/hooks/useFilters"; | |
| import query from "@/Utils/request/query"; | |
| import SearchByMultipleFields from "@/src/components/Common/SearchByMultipleFields"; | |
| import { TableSkeleton } from "@/src/components/Common/SkeletonLoading"; | |
| import { | |
| Popover, | |
| PopoverContent, | |
| PopoverTrigger, | |
| } from "@/src/components/ui/popover"; | |
| import { Tabs, TabsList, TabsTrigger } from "@/src/components/ui/tabs"; | |
| import { cn } from "@/lib/utils"; | |
| import CareIcon from "@/CAREUI/icons/CareIcon"; | |
| import { Badge } from "@/components/ui/badge"; | |
| import { Button } from "@/components/ui/button"; | |
| import { Card } from "@/components/ui/card"; | |
| import useFilters from "@/hooks/useFilters"; | |
| import query from "@/Utils/request/query"; | |
| import SearchByMultipleFields from "@/components/Common/SearchByMultipleFields"; | |
| import { TableSkeleton } from "@/components/Common/SkeletonLoading"; | |
| import { | |
| Popover, | |
| PopoverContent, | |
| PopoverTrigger, | |
| } from "@/components/ui/popover"; | |
| import { Tabs, TabsList, TabsTrigger } from "@/components/ui/tabs"; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/Questionnaire/index.tsx (4)
45-61: Consider adding delay parameter to debounced query.The debounced query function might benefit from an explicit delay parameter to prevent excessive API calls during rapid user input.
- queryFn: query.debounced(questionnaireApi.list, { + queryFn: query.debounced(questionnaireApi.list, 300, {
63-71: Consider memoizing search options.The searchOptions array is recreated on every render. Consider memoizing it with useMemo to optimize performance.
- const searchOptions = [ + const searchOptions = useMemo(() => [ { key: "title", label: "Title", type: "text" as const, placeholder: t("search_by_questionnaire_title"), value: title || "", }, - ]; + ], [title]);
191-206: Improve table header semantics.The table headers should use scope attribute for better accessibility and semantic structure.
<th + scope="col" className="px-6 py-3 text-left text-xs font-medium uppercase tracking-wider text-gray-500" >
228-237: Standardize badge colors for all status types.Currently, only the "active" status has custom colors. Consider adding distinct colors for "draft" and "retired" statuses as well.
<Badge className={ - questionnaire.status === "active" - ? "bg-green-100 text-green-800 hover:bg-green-200" - : "" + { + active: "bg-green-100 text-green-800 hover:bg-green-200", + draft: "bg-yellow-100 text-yellow-800 hover:bg-yellow-200", + retired: "bg-gray-100 text-gray-800 hover:bg-gray-200", + }[questionnaire.status] || "" } >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Questionnaire/index.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (1)
src/components/Questionnaire/index.tsx (1)
28-42: LGTM! Well-structured empty state component.The EmptyState component follows best practices with proper internationalization, clear visual hierarchy, and helpful user feedback.
| <div className="flex flex-wrap items-center my-2 gap-2"> | ||
| <Popover> | ||
| <PopoverTrigger asChild> | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| className={cn("h-8 min-w-[120px] justify-start")} | ||
| > | ||
| <CareIcon icon="l-search" className="mr-2 h-4 w-4" /> | ||
| {t("search")} | ||
| </Button> | ||
| </PopoverTrigger> | ||
| <PopoverContent | ||
| className="w-[20rem] p-3" | ||
| align="start" | ||
| onEscapeKeyDown={(event) => event.preventDefault()} | ||
| > | ||
| <div className="space-y-4"> | ||
| <h4 className="font-medium leading-none"> | ||
| {t("search_questionnaire")} | ||
| </h4> | ||
| <SearchByMultipleFields | ||
| id="questionnaire-search" | ||
| options={searchOptions} | ||
| initialOptionIndex={0} | ||
| onFieldChange={() => | ||
| updateQuery({ | ||
| status, | ||
| title: undefined, | ||
| }) | ||
| } | ||
| onSearch={(key, value) => | ||
| updateQuery({ | ||
| status, | ||
| [key]: value || undefined, | ||
| }) | ||
| } | ||
| className="w-full border-none shadow-none" | ||
| /> | ||
| </div> | ||
| </PopoverContent> | ||
| </Popover> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility of search functionality.
The search button and popover need proper ARIA labels for better accessibility.
<Button
variant="outline"
size="sm"
+ aria-label={t("search_questionnaire")}
className={cn("h-8 min-w-[120px] justify-start")}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="flex flex-wrap items-center my-2 gap-2"> | |
| <Popover> | |
| <PopoverTrigger asChild> | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| className={cn("h-8 min-w-[120px] justify-start")} | |
| > | |
| <CareIcon icon="l-search" className="mr-2 h-4 w-4" /> | |
| {t("search")} | |
| </Button> | |
| </PopoverTrigger> | |
| <PopoverContent | |
| className="w-[20rem] p-3" | |
| align="start" | |
| onEscapeKeyDown={(event) => event.preventDefault()} | |
| > | |
| <div className="space-y-4"> | |
| <h4 className="font-medium leading-none"> | |
| {t("search_questionnaire")} | |
| </h4> | |
| <SearchByMultipleFields | |
| id="questionnaire-search" | |
| options={searchOptions} | |
| initialOptionIndex={0} | |
| onFieldChange={() => | |
| updateQuery({ | |
| status, | |
| title: undefined, | |
| }) | |
| } | |
| onSearch={(key, value) => | |
| updateQuery({ | |
| status, | |
| [key]: value || undefined, | |
| }) | |
| } | |
| className="w-full border-none shadow-none" | |
| /> | |
| </div> | |
| </PopoverContent> | |
| </Popover> | |
| <div className="flex flex-wrap items-center my-2 gap-2"> | |
| <Popover> | |
| <PopoverTrigger asChild> | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| aria-label={t("search_questionnaire")} | |
| className={cn("h-8 min-w-[120px] justify-start")} | |
| > | |
| <CareIcon icon="l-search" className="mr-2 h-4 w-4" /> | |
| {t("search")} | |
| </Button> | |
| </PopoverTrigger> | |
| <PopoverContent | |
| className="w-[20rem] p-3" | |
| align="start" | |
| onEscapeKeyDown={(event) => event.preventDefault()} | |
| > | |
| <div className="space-y-4"> | |
| <h4 className="font-medium leading-none"> | |
| {t("search_questionnaire")} | |
| </h4> | |
| <SearchByMultipleFields | |
| id="questionnaire-search" | |
| options={searchOptions} | |
| initialOptionIndex={0} | |
| onFieldChange={() => | |
| updateQuery({ | |
| status, | |
| title: undefined, | |
| }) | |
| } | |
| onSearch={(key, value) => | |
| updateQuery({ | |
| status, | |
| [key]: value || undefined, | |
| }) | |
| } | |
| className="w-full border-none shadow-none" | |
| /> | |
| </div> | |
| </PopoverContent> | |
| </Popover> |
| <h4 className="font-medium leading-none"> | ||
| {t("search_questionnaire")} | ||
| </h4> | ||
| <SearchByMultipleFields |
There was a problem hiding this comment.
I think using a simple Input would be better instead of SearchByMultipleFields since there’s only one search option.
@hrit2773, let’s wait for the maintainer’s confirmation.
There was a problem hiding this comment.
Hey in future If we want to include multiple fields it would be easy and is hence maintainable thats why I added searchByMultipleFields similar to that of ResourceList page
There was a problem hiding this comment.
I agree with @AdityaJ2305 here.
let's switch to using search by multiple fields when we need it only.
There was a problem hiding this comment.
@rithviknishad ok then I'll change it. Or lets just wait for @Jacobjeevan also to confirm.
There was a problem hiding this comment.
As I said in the call, go with single input. We can switch to SearchByMultiple if we need to in the future.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Questionnaire/index.tsx (1)
63-71: Consider moving searchOptions outside the component.The searchOptions array is static and doesn't depend on component state. Moving it outside would prevent unnecessary recreation on each render.
+const searchOptions = [ + { + key: "title", + label: "Title", + type: "text" as const, + placeholder: t("search_by_questionnaire_title"), + value: "", + }, +]; export function QuestionnaireList() { - const searchOptions = [ - { - key: "title", - label: "Title", - type: "text" as const, - placeholder: t("search_by_questionnaire_title"), - value: title || "", - }, - ]; + const options = searchOptions.map(opt => ({ + ...opt, + value: opt.key === "title" ? title || "" : opt.value, + }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(4 hunks)src/components/Questionnaire/index.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 (3)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/components/Questionnaire/index.tsx (2)
28-42: LGTM! Well-structured EmptyState component.The EmptyState component follows good practices with proper visual hierarchy, translations, and reusable structure.
88-131: Replace SearchByMultipleFields with simple Input component.As confirmed by the maintainers in previous discussions, since there's only one search option, using a simple Input component would be more appropriate.
Add accessibility attributes to interactive elements.
The search button and popover need proper ARIA labels for better accessibility.
<PopoverTrigger asChild> <Button variant="outline" size="sm" + aria-label={t("search_questionnaire")} className={cn( "h-8 min-w-[120px] justify-start", title && "bg-primary/10 text-primary hover:bg-primary/20", )} >
| <tr | ||
| key={questionnaire.id} | ||
| onClick={() => | ||
| navigate(`/questionnaire/${questionnaire.slug}`) | ||
| } | ||
| className="cursor-pointer hover:bg-gray-50" | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance keyboard accessibility for clickable rows.
Add keyboard support for table rows to ensure accessibility for keyboard users.
<tr
key={questionnaire.id}
+ role="button"
+ tabIndex={0}
onClick={() => navigate(`/questionnaire/${questionnaire.slug}`)}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ navigate(`/questionnaire/${questionnaire.slug}`);
+ }
+ }}
className="cursor-pointer hover:bg-gray-50"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <tr | |
| key={questionnaire.id} | |
| onClick={() => | |
| navigate(`/questionnaire/${questionnaire.slug}`) | |
| } | |
| className="cursor-pointer hover:bg-gray-50" | |
| > | |
| <tr | |
| key={questionnaire.id} | |
| role="button" | |
| tabIndex={0} | |
| onClick={() => | |
| navigate(`/questionnaire/${questionnaire.slug}`) | |
| } | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| navigate(`/questionnaire/${questionnaire.slug}`); | |
| } | |
| }} | |
| className="cursor-pointer hover:bg-gray-50" | |
| > |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Questionnaire/index.tsx (1)
44-61: Add error handling for the query.While the query implementation looks good, consider adding error handling to provide feedback to users when the API request fails.
const { data: response, isLoading } = useQuery({ queryKey: ["questionnaires", qParams], queryFn: query.debounced(questionnaireApi.list, { queryParams: { status, title, limit: resultsPerPage, offset: ((qParams.page ?? 1) - 1) * resultsPerPage, }, }), + onError: (error) => { + // Show error notification to user + console.error("Failed to fetch questionnaires:", error); + }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Questionnaire/index.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/components/Questionnaire/index.tsx (3)
28-42: LGTM! Well-structured empty state component.The EmptyState component follows best practices with proper semantic HTML, consistent styling, and internationalization support.
77-115: Enhance accessibility of search functionality.The search implementation needs accessibility improvements:
<Button variant="outline" size="sm" + aria-label={t("search_questionnaire")} className={cn( "h-8 min-w-[120px] justify-start", title && "bg-primary/10 text-primary hover:bg-primary/20", )} >Also, add keyboard support for the Popover:
<PopoverContent className="w-[20rem] p-3" align="start" - onEscapeKeyDown={(event) => event.preventDefault()} + onOpenAutoFocus={(event) => { + event.preventDefault(); + document.getElementById("questionnaire-search")?.focus(); + }} >
197-203: Enhance keyboard accessibility for clickable rows.Add keyboard support for table rows to ensure accessibility for keyboard users.
<tr key={questionnaire.id} + role="button" + tabIndex={0} onClick={() => navigate(`/questionnaire/${questionnaire.slug}`)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + navigate(`/questionnaire/${questionnaire.slug}`); + } + }} className="cursor-pointer hover:bg-gray-50" >
|
@rithviknishad @Jacobjeevan Switched to simple input |
| <div className="items-center"> | ||
| <Tabs value={status || "all"} className="w-full"> | ||
| <TabsList className="bg-transparent p-0 h-8"> | ||
| <TabsTrigger |
There was a problem hiding this comment.
We can use an array of values and map those to a TabsTrigger for this, since code is the same.
| variant="outline" | ||
| size="sm" | ||
| className={cn( | ||
| "h-8 min-w-[120px] justify-start", |
There was a problem hiding this comment.
switch to rem.
Also add some spacing between the tabs and search.
|
@Jacobjeevan agreed..... will update these changes now |
|
@Jacobjeevan I have made changes used calc(), switched to using rem instead of px and used status tabs and mapped in tab list. |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Enhancements