Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update standardizes URL generation for assistant-related pages by shifting from path-based to query-parameter-based URLs and introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useAssistantNavigation
participant prefixPath
participant NextRouter
Component->>useAssistantNavigation: useAssistantNavigation(emailAccountId)
useAssistantNavigation->>prefixPath: prefixPath(emailAccountId, "/assistant")
Component->>useAssistantNavigation: createAssistantUrl({tab, ruleId, ...})
useAssistantNavigation->>Component: returns constructed URL with query params
Component->>NextRouter: router.replace(URL) or <Link href={URL}>
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/web/app/(app)/[emailAccountId]/assistant/ProcessResultDisplay.tsxOops! Something went wrong! :( ESLint: 9.24.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by apps/web/app/(app)/[emailAccountId]/assistant/ExecutedRulesTable.tsxOops! Something went wrong! :( ESLint: 9.24.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by apps/web/app/(app)/[emailAccountId]/assistant/ReportMistake.tsxOops! Something went wrong! :( ESLint: 9.24.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (6)
apps/web/hooks/useAssistantNavigation.ts (1)
24-28: Consider improving type safety for parameters.The current parameter type allows any string key with
[key: string]: string | undefined, which could lead to typos or inconsistencies.Consider creating a more specific type interface:
- (params: { - tab?: string; - ruleId?: string; - [key: string]: string | undefined; - }) => { + (params: { + tab?: string; + ruleId?: string; + chatId?: string; + mode?: string; + page?: string; + search?: string; + custom?: string; + }) => {apps/web/components/GroupedTable.tsx (1)
437-437: Consider using the new useAssistantNavigation hook for consistency.While the URL change is correct, this component manually constructs the assistant URL instead of using the new
useAssistantNavigationhook. This could lead to inconsistencies if URL patterns change in the future.Consider refactoring to use the hook:
+import { useAssistantNavigation } from "@/hooks/useAssistantNavigation"; // In the component: +const { createAssistantUrl } = useAssistantNavigation(emailAccountId); // Then replace the manual URL construction: - href={prefixPath( - emailAccountId, - `/assistant?tab=rule&ruleId=${rule.id}`, - )} + href={createAssistantUrl({ + tab: "rule", + ruleId: rule.id.toString(), + })}apps/web/app/(app)/[emailAccountId]/assistant/rule/[ruleId]/examples/page.tsx (1)
47-47: Consider using the new useAssistantNavigation hook for consistency.The URL change correctly adopts the new query parameter pattern, but like other components, this could benefit from using the
useAssistantNavigationhook for consistency and future maintainability.Consider refactoring to use the hook:
+import { useAssistantNavigation } from "@/hooks/useAssistantNavigation"; // In the component: +const { createAssistantUrl } = useAssistantNavigation(emailAccountId); // Then replace the manual URL construction: - `/assistant?tab=rule&ruleId=${params.ruleId}`, + createAssistantUrl({ + tab: "rule", + ruleId: params.ruleId, + }).replace(prefixPath(emailAccountId, ""), ""),Or better yet, update the hook to handle prefixPath internally, then:
- href={prefixPath( - emailAccountId, - `/assistant?tab=rule&ruleId=${params.ruleId}`, - )} + href={createAssistantUrl({ + tab: "rule", + ruleId: params.ruleId, + })}apps/web/app/(app)/[emailAccountId]/assistant/ProcessResultDisplay.tsx (1)
89-93: Consider using theuseAssistantNavigationhook for consistency.The URL structure change is correct and aligns with the PR objectives. However, according to the AI summary, a new
useAssistantNavigationhook was introduced to centralize URL construction. Consider using this hook instead of manually constructing the URL for better maintainability.apps/web/app/(app)/[emailAccountId]/assistant/group/Groups.tsx (1)
99-106: URL structure update looks good.The change correctly updates the URL from path-based to query-parameter format, maintaining consistency with other components. As with other files in this PR, consider using the
useAssistantNavigationhook if available for centralized URL management.apps/web/app/(app)/[emailAccountId]/assistant/ReportMistake.tsx (1)
1093-1114: Consistent URL update in EditRuleButton component.The URL structure change is correct and consistent with the other files. Since
EditRuleButtonis used multiple times throughout this file, using theuseAssistantNavigationhook here would be particularly beneficial for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/app/(app)/[emailAccountId]/assistant/ExecutedRulesTable.tsx(3 hunks)apps/web/app/(app)/[emailAccountId]/assistant/ProcessResultDisplay.tsx(1 hunks)apps/web/app/(app)/[emailAccountId]/assistant/ReportMistake.tsx(1 hunks)apps/web/app/(app)/[emailAccountId]/assistant/RuleForm.tsx(2 hunks)apps/web/app/(app)/[emailAccountId]/assistant/Rules.tsx(4 hunks)apps/web/app/(app)/[emailAccountId]/assistant/group/Groups.tsx(1 hunks)apps/web/app/(app)/[emailAccountId]/assistant/rule/[ruleId]/examples/page.tsx(1 hunks)apps/web/components/GroupedTable.tsx(1 hunks)apps/web/hooks/useAssistantNavigation.ts(1 hunks)version.txt(1 hunks)
🔇 Additional comments (7)
version.txt (1)
1-1: LGTM: Standard version increment.The version bump from v1.2.4 to v1.2.5 appropriately reflects the URL structure changes and new navigation hook implementation.
apps/web/hooks/useAssistantNavigation.ts (3)
6-14: LGTM: Well-structured parameter serializer.The serializer configuration correctly handles all assistant-related parameters using
parseAsString. This provides a consistent foundation for URL parameter management.
16-22: LGTM: Proper hook setup with clear intent.The hook correctly uses Next.js navigation hooks and includes a helpful comment explaining the pathname matching logic. The
endsWith("/assistant")check ensures it only matches the main assistant page, not sub-pages.
23-44: LGTM: Smart URL generation logic with proper optimization.The conditional logic intelligently preserves existing search parameters when already on the assistant page, which provides a good user experience. The
useCallbackoptimization with correct dependencies prevents unnecessary re-renders.apps/web/app/(app)/[emailAccountId]/assistant/ExecutedRulesTable.tsx (1)
21-21: LGTM! URL standardization properly implemented.The introduction of
useAssistantNavigationhook and the migration to query parameter-based URLs is correctly implemented. This change improves consistency across the assistant navigation.Also applies to: 89-90, 116-119
apps/web/app/(app)/[emailAccountId]/assistant/RuleForm.tsx (1)
289-290: Good improvements to form field naming.The normalization of the name attribute to lowercase and the more descriptive label improve consistency and clarity.
apps/web/app/(app)/[emailAccountId]/assistant/Rules.tsx (1)
61-61: Excellent implementation of the new URL pattern.The
useAssistantNavigationhook is properly integrated and consistently used for both rule editing and history URLs. The special handling for cold email blocker rules is correctly preserved.Also applies to: 70-70, 201-204, 296-299
| router.replace( | ||
| prefixPath(emailAccountId, `/assistant/rule/${res.data.rule.id}`), | ||
| prefixPath( | ||
| emailAccountId, | ||
| `/assistant?tab=rule&ruleId=${res.data.rule.id}`, | ||
| ), | ||
| ); | ||
| router.push(prefixPath(emailAccountId, "/assistant?tab=rules")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using useAssistantNavigation for consistency.
While the URL pattern correctly follows the new query parameter format, this component still uses prefixPath directly instead of the useAssistantNavigation hook that other components are using. Consider refactoring to use createAssistantUrl for consistency:
+ const { createAssistantUrl } = useAssistantNavigation(emailAccountId);
// ...
router.replace(
- prefixPath(
- emailAccountId,
- `/assistant?tab=rule&ruleId=${res.data.rule.id}`,
- ),
+ createAssistantUrl({
+ tab: "rule",
+ ruleId: res.data.rule.id,
+ }),
);📝 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.
| router.replace( | |
| prefixPath(emailAccountId, `/assistant/rule/${res.data.rule.id}`), | |
| prefixPath( | |
| emailAccountId, | |
| `/assistant?tab=rule&ruleId=${res.data.rule.id}`, | |
| ), | |
| ); | |
| router.push(prefixPath(emailAccountId, "/assistant?tab=rules")); | |
| } | |
| // add the navigation helper | |
| const { createAssistantUrl } = useAssistantNavigation(emailAccountId); | |
| // ... | |
| router.replace( | |
| createAssistantUrl({ | |
| tab: "rule", | |
| ruleId: res.data.rule.id, | |
| }), | |
| ); | |
| router.push(prefixPath(emailAccountId, "/assistant?tab=rules")); | |
| } |
🤖 Prompt for AI Agents
In apps/web/app/(app)/[emailAccountId]/assistant/RuleForm.tsx around lines 210
to 217, the code directly uses prefixPath to generate URLs for router.replace
and router.push, which is inconsistent with other components that use the
useAssistantNavigation hook. Refactor these lines to use the createAssistantUrl
function from useAssistantNavigation for URL generation to ensure consistency
across the codebase.
1 participant
Summary by CodeRabbit
New Features
Refactor
Chores