Filter by rule on history and pending tabs #302
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces several significant changes across multiple files in the web application, focusing on enhancing rule-based filtering and pagination functionality. The primary modifications include the introduction of a new Changes
Sequence DiagramsequenceDiagram
participant User
participant RulesSelect
participant API
participant Component
User->>RulesSelect: Select Rule
RulesSelect->>API: Fetch Rules
API-->>RulesSelect: Return Rules List
RulesSelect->>Component: Update ruleId
Component->>API: Fetch Filtered Data
API-->>Component: Return Filtered Results
Poem
Finishing Touches
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 (
|
![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
🧹 Nitpick comments (8)
apps/web/hooks/useRules.tsx (1)
4-6: Consider extracting API path to a constantThe API path
/api/user/rulesshould be extracted to a constant or configuration file to maintain consistency and make updates easier.+const RULES_API_ENDPOINT = "/api/user/rules"; + export function useRules() { - return useSWR<RulesResponse, { error: string }>("/api/user/rules"); + return useSWR<RulesResponse, { error: string }>(RULES_API_ENDPOINT); }apps/web/app/(app)/automation/RulesSelect.tsx (2)
32-35: Enhance Select component accessibilityAdd ARIA labels and proper accessibility attributes to improve usability.
-<Select defaultValue={ruleId} onValueChange={setRuleId}> - <SelectTrigger> - <SelectValue placeholder="Filter by rule" /> - </SelectTrigger> +<Select defaultValue={ruleId} onValueChange={setRuleId} aria-label="Filter rules"> + <SelectTrigger aria-label="Select rule filter"> + <SelectValue placeholder="Filter by rule" aria-label="Selected rule filter" /> + </SelectTrigger>
38-42: Optimize rule list renderingFor large rule sets, consider memoizing the rule items to prevent unnecessary re-renders.
+const RuleItems = memo(({ rules }: { rules: Rule[] }) => ( + <> + {rules?.map((rule) => ( + <SelectItem key={rule.id} value={rule.id}> + {rule.name} + </SelectItem> + ))} + </> +)); + <SelectContent> <SelectItem value="all">All rules</SelectItem> - {rules?.map((rule) => ( - <SelectItem key={rule.id} value={rule.id}> - {rule.name} - </SelectItem> - ))} + <RuleItems rules={rules} /> </SelectContent>apps/web/components/TablePagination.tsx (1)
27-47: Consider adding page numbers between prev/next buttons.The current implementation only shows the current page number. For better UX, consider showing a range of page numbers, especially for tables with many pages.
apps/web/app/api/user/planned/get-executed-rules.ts (1)
15-15: Consider adding an index for ruleId.Adding
ruleIdto the where clause might impact query performance. Consider adding an index if this field is frequently queried.Also applies to: 23-24
apps/web/app/(app)/automation/History.tsx (2)
29-33: Add loading and error states for rule selection.Consider showing a loading state while the data is being refetched after rule selection changes.
const [ruleId, setRuleId] = useQueryState( "ruleId", parseAsString.withDefault("all"), ); + const [isChangingRule, setIsChangingRule] = useState(false); + + const handleRuleChange = async (newRuleId: string) => { + setIsChangingRule(true); + try { + await setRuleId(newRuleId); + } finally { + setIsChangingRule(false); + } + };
41-44: Consider persisting rule selection.The selected rule is lost on page refresh. Consider persisting it in localStorage for a better user experience.
apps/web/app/(app)/automation/ReportMistake.tsx (1)
Line range hint
445-445: Consider implementing auto-fix for static rules.The TODO comment suggests automating static rule fixes, which could improve user experience.
Would you like me to help implement the auto-fix functionality for static rules or create a GitHub issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/web/app/(app)/automation/ExecutedRulesTable.tsx(0 hunks)apps/web/app/(app)/automation/History.tsx(2 hunks)apps/web/app/(app)/automation/Pending.tsx(2 hunks)apps/web/app/(app)/automation/ReportMistake.tsx(3 hunks)apps/web/app/(app)/automation/Rules.tsx(1 hunks)apps/web/app/(app)/automation/RulesSelect.tsx(1 hunks)apps/web/app/(app)/cold-email-blocker/ColdEmailList.tsx(1 hunks)apps/web/app/(app)/cold-email-blocker/ColdEmailRejected.tsx(1 hunks)apps/web/app/api/user/planned/get-executed-rules.ts(1 hunks)apps/web/app/api/user/planned/history/route.ts(1 hunks)apps/web/app/api/user/planned/route.ts(1 hunks)apps/web/components/TablePagination.tsx(1 hunks)apps/web/hooks/useRules.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/app/(app)/automation/ExecutedRulesTable.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/(app)/cold-email-blocker/ColdEmailList.tsx
🔇 Additional comments (8)
apps/web/app/api/user/planned/route.ts (1)
Line range hint
6-7: Address TODO comment regarding request durationThe comment suggests that requests taking more than 15s is problematic, yet the maxDuration is set to 30s. This needs investigation and proper handling.
Consider:
- Adding monitoring for request duration
- Implementing pagination limits
- Adding caching mechanisms
apps/web/app/api/user/planned/get-executed-rules.ts (1)
20-25: Add validation for ruleId format.The
ruleIdparameter should be validated before being used in the database query to prevent potential SQL injection or invalid queries.apps/web/app/(app)/cold-email-blocker/ColdEmailRejected.tsx (1)
16-17: LGTM! Clean component reorganization.The changes appropriately utilize the new shared TablePagination component and reuse existing components.
apps/web/app/(app)/automation/History.tsx (1)
54-62: LGTM! Clear user feedback.The conditional messaging based on rule selection provides clear feedback to users.
apps/web/app/(app)/automation/Pending.tsx (3)
4-4: LGTM! Good use of URL state management.Using
useQueryStatefor managing page and ruleId in the URL is a good practice, making the filtered state shareable and bookmarkable.Also applies to: 37-40
42-43: LGTM! API request URL properly includes filtering parameters.The URL is correctly constructed to include both pagination and rule filtering parameters.
49-52: LGTM! RulesSelect component is properly integrated.The component is well-positioned and correctly receives the necessary props for state management.
apps/web/app/(app)/automation/ReportMistake.tsx (1)
50-50: LGTM! Consistent use of the useRules hook.The changes align with the modifications in Rules.tsx, maintaining consistency in data fetching across components.
Also applies to: 65-65
| const ruleId = url.searchParams.get("ruleId") || "all"; | ||
| const messages = await getExecutedRules( | ||
| ExecutedRuleStatus.APPLIED, | ||
| page, | ||
| ruleId, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for page and ruleId parameters
The current implementation lacks validation for the input parameters:
- Page number should be validated to ensure it's positive
- RuleId format should be validated when not "all"
const url = new URL(request.url);
-const page = Number.parseInt(url.searchParams.get("page") || "1");
-const ruleId = url.searchParams.get("ruleId") || "all";
+const pageParam = url.searchParams.get("page") || "1";
+const page = Number.parseInt(pageParam);
+if (isNaN(page) || page < 1) {
+ return NextResponse.json({ error: "Invalid page number" }, { status: 400 });
+}
+
+const ruleId = url.searchParams.get("ruleId") || "all";
+if (ruleId !== "all" && !ruleId.match(/^[a-zA-Z0-9-_]+$/)) {
+ return NextResponse.json({ error: "Invalid rule ID format" }, { status: 400 });
+}📝 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.
| const ruleId = url.searchParams.get("ruleId") || "all"; | |
| const messages = await getExecutedRules( | |
| ExecutedRuleStatus.APPLIED, | |
| page, | |
| ruleId, | |
| ); | |
| const url = new URL(request.url); | |
| const pageParam = url.searchParams.get("page") || "1"; | |
| const page = Number.parseInt(pageParam); | |
| if (isNaN(page) || page < 1) { | |
| return NextResponse.json({ error: "Invalid page number" }, { status: 400 }); | |
| } | |
| const ruleId = url.searchParams.get("ruleId") || "all"; | |
| if (ruleId !== "all" && !ruleId.match(/^[a-zA-Z0-9-_]+$/)) { | |
| return NextResponse.json({ error: "Invalid rule ID format" }, { status: 400 }); | |
| } | |
| const messages = await getExecutedRules( | |
| ExecutedRuleStatus.APPLIED, | |
| page, | |
| ruleId, | |
| ); |
| const ruleId = url.searchParams.get("ruleId") || "all"; | ||
| const messages = await getExecutedRules( | ||
| ExecutedRuleStatus.PENDING, | ||
| page, | ||
| ruleId, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract shared route parameter handling
The parameter handling logic is duplicated between history and pending routes. Consider extracting this to a shared utility function.
// utils/routeHelpers.ts
export function getRouteParams(url: URL) {
const page = Number.parseInt(url.searchParams.get("page") || "1");
if (isNaN(page) || page < 1) {
throw new Error("Invalid page number");
}
const ruleId = url.searchParams.get("ruleId") || "all";
if (ruleId !== "all" && !ruleId.match(/^[a-zA-Z0-9-_]+$/)) {
throw new Error("Invalid rule ID format");
}
return { page, ruleId };
}|
|
||
| export function TablePagination({ totalPages }: { totalPages: number }) { | ||
| const searchParams = useSearchParams(); | ||
| const page = Number.parseInt(searchParams.get("page") || "1"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for page number.
The page number from URL params should be validated to ensure it's within valid bounds.
- const page = Number.parseInt(searchParams.get("page") || "1");
+ const rawPage = Number.parseInt(searchParams.get("page") || "1");
+ const page = !isNaN(rawPage) && rawPage > 0 ? Math.min(rawPage, totalPages) : 1;📝 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.
| const page = Number.parseInt(searchParams.get("page") || "1"); | |
| const rawPage = Number.parseInt(searchParams.get("page") || "1"); | |
| const page = !isNaN(rawPage) && rawPage > 0 ? Math.min(rawPage, totalPages) : 1; |
| @@ -49,12 +48,10 @@ import { isActionError } from "@/utils/error"; | |||
| import { Tooltip } from "@/components/Tooltip"; | |||
| import { cn } from "@/utils"; | |||
| import { type RiskLevel, getRiskLevel } from "@/utils/risk"; | |||
| import { useRules } from "@/hooks/useRules"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of useRules hook detected
The useRules hook abstraction is not being used consistently. Found direct useSWR usage in:
apps/web/app/(app)/automation/ProcessRules.tsx
Consider updating ProcessRules.tsx to use the useRules hook for better maintainability and consistency.
🔗 Analysis chain
LGTM! Good abstraction of data fetching logic.
The replacement of direct useSWR usage with a custom useRules hook improves code reusability and maintains consistency across components.
Let's verify if the useRules hook is being used consistently across the codebase:
Also applies to: 54-54
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct useSWR calls for rules
rg "useSWR.*api/user/rules"
# Search for useRules hook usage
rg "useRules"
Length of output: 840
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates