chore: update nextjs to v15 #2513
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dosubot
bot
added
size:L
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (25)
keep-ui/app/workflows/[workflow_id]/page.tsx (1)
5-8: Consider adding error boundaries for promise rejection handlingWhile the implementation correctly uses the
usehook for the new promise-based params, consider adding error boundaries to gracefully handle potential promise rejections.+'use client'; +import { use } from "react"; +import { ErrorBoundary } from "react-error-boundary"; +import WorkflowDetailPage from "./executions"; + +function WorkflowContent(props:{params: Promise<{ workflow_id: string }>}) { + const params = use(props.params); + return <WorkflowDetailPage params={params}/>; +} + +export default function Page(props:{params: Promise<{ workflow_id: string }>}) { + return ( + <ErrorBoundary fallback={<div>Something went wrong</div>}> + <WorkflowContent params={props.params} /> + </ErrorBoundary> + ); +}keep-ui/app/incidents/[id]/route.tsx (1)
9-10: LGTM! Consider enhancing the legacy route documentation.The async/await implementation is correct for handling the Promise-wrapped params.
Consider expanding the comment to provide more context about this legacy route:
-// This is just a redirect from legacy route +// This route handler maintains backward compatibility by redirecting +// from the legacy /incidents/[id] route to the new /incidents/[id]/alerts structurekeep-ui/app/workflows/[workflow_id]/runs/[workflow_execution_id]/page.tsx (1)
Line range hint
10-14: Consider adding error boundaries for promise rejection handlingThe implementation is clean and follows Next.js 15's patterns correctly. However, consider wrapping this in an error boundary to gracefully handle potential promise rejections.
"use client"; import React, { use } from "react"; +import { ErrorBoundary } from "react-error-boundary"; import WorkflowExecutionResults from "app/workflows/builder/workflow-execution-results"; +function ErrorFallback({ error }: { error: Error }) { + return ( + <div role="alert"> + <p>Something went wrong:</p> + <pre>{error.message}</pre> + </div> + ); +} export default function WorkflowExecutionPage( props: { params: Promise<{ workflow_id: string; workflow_execution_id: string }>; } ) { const params = use(props.params); return ( + <ErrorBoundary FallbackComponent={ErrorFallback}> <WorkflowExecutionResults workflow_id={params.workflow_id} workflow_execution_id={params.workflow_execution_id} /> + </ErrorBoundary> ); }keep-ui/app/workflows/builder/[workflowId]/page.tsx (2)
11-11: Verify error handling for params resolutionWhile the await syntax is correct, consider adding error handling for the params resolution to gracefully handle potential failures.
- const params = await props.params; + try { + const params = await props.params; + } catch (error) { + console.error('Failed to resolve workflow params:', error); + throw new Error('Failed to load workflow details'); + }
Line range hint
15-24: Consider caching strategy for workflow dataThe
cache: "no-store"setting means the workflow data is always fetched fresh. With Next.js 15's improved caching capabilities, consider if a more optimized caching strategy would be beneficial.You might want to explore:
- Using
cache: 'force-cache'for static data- Using
next/cachefor more granular control- Implementing revalidation if the workflow data changes frequently but not on every request
keep-ui/app/incidents/[id]/alerts/page.tsx (1)
Line range hint
20-31: Consider parallel metadata fetching.The implementation is correct, but we could optimize by fetching the incident data in parallel with other metadata operations.
Consider this optimization:
export async function generateMetadata(props: PageProps) { const params = await props.params; - const incident = await getIncidentWithErrorHandling(params.id); + const incidentPromise = getIncidentWithErrorHandling(params.id); + const incident = await incidentPromise; const incidentName = getIncidentName(incident); const incidentDescription = incident.user_summary || incident.generated_summary; return { title: `Keep — ${incidentName} — Alerts`, description: incidentDescription, }; }keep-ui/app/incidents/[id]/timeline/page.tsx (1)
20-21: Consider parallel data fetching optimizationThe implementation correctly handles async params, but we could potentially optimize by fetching data in parallel.
Consider this optimization:
export async function generateMetadata(props: PageProps) { const params = await props.params; - const incident = await getIncidentWithErrorHandling(params.id); + const [incident] = await Promise.all([ + getIncidentWithErrorHandling(params.id) + ]); const incidentName = getIncidentName(incident); const incidentDescription = incident.user_summary || incident.generated_summary;keep-ui/app/incidents/[id]/chat/page.tsx (1)
9-15: LGTM! Consider adding error boundaryThe async params handling is implemented correctly. However, since we're dealing with async operations, consider adding an error boundary to gracefully handle potential promise rejections.
+import { ErrorBoundary } from '@/components/ErrorBoundary'; export default async function IncidentChatPage(props: PageProps) { const params = await props.params; const { id } = params; const incident = await getIncidentWithErrorHandling(id); - return <IncidentChatClientPage incident={incident} />; + return ( + <ErrorBoundary fallback={<div>Error loading incident chat</div>}> + <IncidentChatClientPage incident={incident} /> + </ErrorBoundary> + ); }keep-ui/app/incidents/[id]/workflows/page.tsx (1)
9-15: LGTM! Consider adding error boundaryThe async/await pattern is correctly implemented for handling the Promise-based params. The code structure is clean and maintainable.
Consider wrapping the component with an error boundary to gracefully handle any Promise rejections:
import { ErrorBoundary } from '@/components/error-boundary'; export default async function IncidentWorkflowsPage(props: PageProps) { return ( <ErrorBoundary fallback={<div>Error loading workflow page</div>}> {/* existing code */} </ErrorBoundary> ); }keep-ui/app/incidents/[id]/layout.tsx (1)
7-12: Consider extracting the props type definitionThe function signature correctly implements the async params pattern required for Next.js 15. However, since this props structure might be reused across other incident-related layouts, consider extracting it into a shared type.
+type IncidentLayoutProps = { + children: ReactNode; + params: Promise<{ id: string }>; +}; + -export default async function Layout( - props: { - children: ReactNode; - params: Promise<{ id: string }>; - } -) { +export default async function Layout(props: IncidentLayoutProps) {keep-ui/app/incidents/[id]/topology/page.tsx (1)
27-28: Consider parallel metadata fetchingThe implementation correctly handles async params. However, since both
paramsandincidentfetching are independent operations, they could potentially be parallelized for better performance.Consider this optimization:
export async function generateMetadata(props: PageProps) { - const params = await props.params; - const incident = await getIncidentWithErrorHandling(params.id); + const [params, incident] = await Promise.all([ + props.params, + props.params.then(p => getIncidentWithErrorHandling(p.id)) + ]); const incidentName = getIncidentName(incident);keep-ui/app/workflows/preview/[workflowId]/page.tsx (3)
12-14: Replaceanytype with a proper interfaceUsing
anytype reduces type safety. Based on the component usage, we can define a proper interface.+interface WorkflowPreviewData { + name: string; + workflow_raw?: string; +} -const [workflowPreviewData, setWorkflowPreviewData] = useState<any>(null); +const [workflowPreviewData, setWorkflowPreviewData] = useState<WorkflowPreviewData | null>(null);
Line range hint
16-25: Fix incorrect state update in else clauseThere's a bug in the else clause where
workflowPreviewDatais called as a function instead of using the setter.useEffect(() => { if (key) { const data = localStorage.getItem("preview_workflow"); if (data) { setWorkflowPreviewData(JSON.parse(data)); } } else { - workflowPreviewData({}); + setWorkflowPreviewData({}); } }, [params.workflowId]);
Line range hint
27-52: Consider extracting UI components for better organizationThe render logic contains multiple conditional branches. Consider extracting these into separate components for better maintainability.
+const NotFoundUI = () => ( + <> + <Link + className="p-2 bg-orange-500 text-white hover:bg-orange-600 rounded" + href="/workflows" + > + Go Back + </Link> + <div className="flex items-center justify-center min-h-screen"> + <div className="text-center text-red-500">Workflow not found!</div> + </div> + </> +); +const PreviewUI = ({ workflow }: { workflow: string }) => ( + <PageClient + workflow={workflow} + isPreview={true} + /> +); return ( <> {!workflowPreviewData && <Loading />} {workflowPreviewData && workflowPreviewData.name === key && ( - <PageClient - workflow={workflowPreviewData.workflow_raw || ""} - isPreview={true} - /> + <PreviewUI workflow={workflowPreviewData.workflow_raw || ""} /> )} {workflowPreviewData && workflowPreviewData.name !== key && ( - <> - <Link - className="p-2 bg-orange-500 text-white hover:bg-orange-600 rounded" - href="/workflows" - > - Go Back - </Link> - <div className="flex items-center justify-center min-h-screen"> - <div className="text-center text-red-500">Workflow not found!</div> - </div> - </> + <NotFoundUI /> )} </> );keep-ui/app/topology/page.tsx (2)
24-24: Consider adding error handling for searchParams resolution.While the await usage is correct, consider adding error handling for cases where searchParams resolution fails.
- const searchParams = await props.searchParams; + const searchParams = await props.searchParams.catch(error => { + console.error('Failed to resolve search parameters:', error); + return {}; // Provide default values + });
23-24: Consider implementing Suspense boundaries for async data fetching.Since this is part of the Next.js 15 upgrade, consider implementing Suspense boundaries around the TopologyPageClient component to handle loading states more elegantly. This aligns with Next.js 15's improved server components and streaming capabilities.
Example implementation:
import { Suspense } from 'react'; // In your JSX <Suspense fallback={<LoadingUI />}> <TopologyPageClient applications={applications || undefined} topologyServices={topologyServices || undefined} /> </Suspense>keep-ui/app/workflows/preview/page.tsx (3)
12-14: Consider extracting async data resolutionThe direct usage of
usehook in the component body could be refactored for better maintainability:-export default function Page(props: PageProps) { - const searchParams = use(props.searchParams); - const params = use(props.params); +async function resolveParams(props: PageProps) { + const [params, searchParams] = await Promise.all([ + props.params, + props.searchParams + ]); + return { params, searchParams }; +} + +export default function Page(props: PageProps) { + const { params, searchParams } = use(resolveParams(props));
Line range hint
18-26: Add error handling for localStorage operationsThe localStorage operations should include error handling for cases where:
- Storage quota is exceeded
- Storage is not available
- JSON parsing fails
useEffect(() => { if (key) { - const data = localStorage.getItem("preview_workflow"); - if (data) { - setWorkflowPreviewData(JSON.parse(data) || {}); + try { + const data = localStorage.getItem("preview_workflow"); + if (data) { + setWorkflowPreviewData(JSON.parse(data) || {}); + } + } catch (error) { + console.error('Failed to load workflow preview:', error); + setWorkflowPreviewData({}); } } else { setWorkflowPreviewData({});
Line range hint
29-54: Consider extracting UI components for better maintainabilityThe conditional rendering logic could be simplified by extracting components:
const LoadingState = () => <Loading />; const WorkflowPreview = ({ workflow, workflowId }: { workflow: any, workflowId: string }) => ( <PageClient workflow={workflow} workflowId={workflowId} isPreview={true} /> ); const NotFoundState = () => ( <> <Link className="p-2 bg-orange-500 text-white hover:bg-orange-600 rounded" href="/workflows" > Go Back </Link> <div className="flex items-center justify-center min-h-screen"> <div className="text-center text-red-500">Workflow not found!</div> </div> </> );This would make the main component more readable:
return ( <> {!workflowPreviewData && <LoadingState />} {workflowPreviewData && workflowPreviewData.name === key && ( <WorkflowPreview workflow={workflowPreviewData?.Workflow_raw} workflowId={params?.workflowId} /> )} {workflowPreviewData && workflowPreviewData.name !== key && <NotFoundState />} </> );keep-ui/app/providers/oauth2/[providerType]/page.tsx (2)
13-17: Consider updating the API URL handlingThe comment "this is server so we can use the old getApiURL" suggests technical debt. Consider updating the API URL handling to align with Next.js v15 best practices.
Line range hint
23-45: Consider enhancing error handling and securityTwo suggestions for improvement:
The error handling could be more specific. Instead of passing the raw response text in the URL, consider mapping error responses to user-friendly messages.
The redirect URI contains sensitive information. Consider encoding the error messages before including them in the URL.
Example improvement:
- redirect(`/providers?oauth=failure&reason=${responseText}`); + const errorMessage = encodeURIComponent( + mapErrorToUserFriendly(responseText) + ); + redirect(`/providers?oauth=failure&reason=${errorMessage}`);keep-ui/package.json (2)
Line range hint
44-44: Critical: Update Next.js to v15The
nextpackage is still on version ^14.2.13 while the PR aims to upgrade to v15. This version mismatch could cause compatibility issues.Apply this change:
- "next": "^14.2.13", + "next": "^15.0.0",
Line range hint
28-28: Update eslint-config-next to match Next.js versionFor compatibility, eslint-config-next should be updated to match the Next.js version.
Apply this change:
- "eslint-config-next": "^14.2.1", + "eslint-config-next": "^15.0.0",keep-ui/app/workflows/builder/page.tsx (2)
6-7: Remove unusedsearchParamspropertyThe
searchParamsproperty is defined but not used in thePagecomponent. If it's not required, consider removing it to simplify the code.Apply this diff:
type PageProps = { params: Promise<{ workflow: string; workflowId: string }>; - searchParams: Promise<{ [key: string]: string | string[] | undefined }>; };
10-11: Destructure and awaitparamsfor improved readabilityYou can destructure
paramsfrompropsand directly await it within the destructuring assignment. This makes the code cleaner and more concise.Apply this diff:
-export default async function Page(props: PageProps) { - const params = await props.params; +export default async function Page({ params }: PageProps) { + const { workflow, workflowId } = await params; return ( <Suspense fallback={<Loading />}> - <PageClient workflow={params.workflow} workflowId={params.workflowId} /> + <PageClient workflow={workflow} workflowId={workflowId} /> </Suspense> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
keep-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
keep-ui/app/alerts/[id]/page.tsx(1 hunks)keep-ui/app/incidents/[id]/activity/page.tsx(2 hunks)keep-ui/app/incidents/[id]/alerts/page.tsx(1 hunks)keep-ui/app/incidents/[id]/chat/page.tsx(1 hunks)keep-ui/app/incidents/[id]/layout.tsx(1 hunks)keep-ui/app/incidents/[id]/route.tsx(1 hunks)keep-ui/app/incidents/[id]/timeline/page.tsx(1 hunks)keep-ui/app/incidents/[id]/topology/page.tsx(2 hunks)keep-ui/app/incidents/[id]/workflows/page.tsx(1 hunks)keep-ui/app/layout.tsx(1 hunks)keep-ui/app/providers/oauth2/[providerType]/page.tsx(1 hunks)keep-ui/app/providers/page.tsx(1 hunks)keep-ui/app/topology/page.tsx(1 hunks)keep-ui/app/workflows/[workflow_id]/page.tsx(1 hunks)keep-ui/app/workflows/[workflow_id]/runs/[workflow_execution_id]/page.tsx(1 hunks)keep-ui/app/workflows/builder/[workflowId]/page.tsx(1 hunks)keep-ui/app/workflows/builder/page.tsx(1 hunks)keep-ui/app/workflows/preview/[workflowId]/page.tsx(1 hunks)keep-ui/app/workflows/preview/page.tsx(1 hunks)keep-ui/package.json(2 hunks)keep-ui/shared/ui/PostHogPageView.client.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- keep-ui/shared/ui/PostHogPageView.client.tsx
🔇 Additional comments (33)
keep-ui/app/workflows/[workflow_id]/page.tsx (1)
1-2: LGTM! Proper usage of React's use hook
The addition of the use hook import is appropriate for handling the new promise-based params in Next.js 15.
keep-ui/app/incidents/[id]/route.tsx (1)
4-5: LGTM! Verify consistent Promise type usage across routes.
The type changes align with Next.js 15's async data fetching patterns.
Let's verify this pattern is consistently applied across other route handlers:
keep-ui/app/providers/page.tsx (2)
1-1: Verify the client component setup
The import statement suggests a client/server component split pattern. Let's verify the client component exists and follows Next.js conventions.
✅ Verification successful
Client component setup is correctly implemented
The client component exists and follows Next.js conventions:
- Contains the required
'use client'directive at the top - Follows consistent naming pattern (
page.client.tsx) used across the app - Properly implements client-side functionality (React hooks, interactivity)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify client component setup
# Check if client component exists and uses 'use client' directive
rg -l "^'use client'" --glob "keep-ui/app/providers/page.client.tsx"
# Check for consistent naming pattern across the app
fd -e tsx "page.client.tsx" "keep-ui/app/"
Length of output: 443
Script:
#!/bin/bash
# Check content of the client component and 'use client' directive
rg -A 5 "^'use client'" "keep-ui/app/providers/page.client.tsx"
# Check if there are any other client components with similar patterns
rg -A 5 "^'use client'" "keep-ui/app/incidents/[id]/chat/page.client.tsx"
rg -A 5 "^'use client'" "keep-ui/app/workflows/builder/page.client.tsx"
Length of output: 229
Script:
#!/bin/bash
# Check if the client component file exists and get its content
cat "keep-ui/app/providers/page.client.tsx"
# List all files in the providers directory to understand the context
ls -la "keep-ui/app/providers/"
Length of output: 6869
3-7: Verify the Promise wrapper on searchParams against Next.js documentation
The type Promise<{ [key: string]: string }> for searchParams is unusual for Next.js. Typically, Next.js provides searchParams as a plain object, not a Promise, even in async components.
Let's verify this implementation against Next.js documentation and other files:
keep-ui/app/workflows/[workflow_id]/runs/[workflow_execution_id]/page.tsx (2)
1-3: LGTM! Proper setup for client-side component with async data handling
The imports and client directive are correctly configured for Next.js 15's client component requirements.
5-9: Verify the impact of async params migration
The type changes align with Next.js 15's new patterns for handling route parameters. However, we should verify that all consumers of this component are prepared for the async nature of the params.
✅ Verification successful
Async params pattern is consistently implemented
The codebase shows consistent implementation of async params across similar page components:
keep-ui/app/workflows/preview/[workflowId]/page.tsxkeep-ui/app/incidents/[id]/activity/page.tsxkeep-ui/app/workflows/builder/[workflowId]/page.tsx
All these components properly handle Promise-based params using either use() or await, following Next.js 15's patterns. The implementation in WorkflowExecutionPage aligns with the codebase's architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other components that might be affected by this change
# Look for direct imports of WorkflowExecutionPage
rg -l "WorkflowExecutionPage" --type typescript
# Look for similar workflow-related pages that might need the same update
fd -e tsx -e ts "workflow.*page\.tsx$"
Length of output: 123
Script:
#!/bin/bash
# Search for any files importing or using WorkflowExecutionPage
rg "WorkflowExecutionPage"
# Find workflow-related page components
fd "workflow.*page\.(tsx|ts)$"
# Look for similar page components that might need async params
ast-grep --pattern 'export default function $_(props: { params: $_; }) {
$$$
}'
Length of output: 7911
keep-ui/app/workflows/builder/[workflowId]/page.tsx (1)
6-10: LGTM: Function signature correctly updated for Next.js 15
The change to handle params as a Promise aligns with Next.js 15's new patterns for server components. This is a necessary update as part of the framework upgrade.
keep-ui/app/incidents/[id]/alerts/page.tsx (2)
9-15: LGTM! Clean async implementation.
The changes correctly handle the async params while maintaining good code structure and error handling.
6-6: LGTM! Verify all consumers of this type.
The change to make params async aligns with Next.js 15's server component changes.
Let's verify all consumers of this type:
✅ Verification successful
All consumers are already using the async pattern correctly ✅
All files in the incidents directory that use the PageProps type have been updated to use Promise<{ id: string }> and properly await the params, including:
- keep-ui/app/incidents/[id]/route.tsx
- keep-ui/app/incidents/[id]/topology/page.tsx
- keep-ui/app/incidents/[id]/timeline/page.tsx
- keep-ui/app/incidents/[id]/workflows/page.tsx
- keep-ui/app/incidents/[id]/alerts/page.tsx
- keep-ui/app/incidents/[id]/chat/page.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all files that might be using this PageProps type
rg -l "PageProps" "keep-ui/app/incidents/"
Length of output: 297
Script:
#!/bin/bash
# Check the PageProps usage in each file to verify if they're using the async pattern
for file in $(rg -l "PageProps" "keep-ui/app/incidents/"); do
echo "=== $file ==="
rg "PageProps" "$file" -B 2 -A 2
done
Length of output: 3821
keep-ui/app/incidents/[id]/timeline/page.tsx (2)
9-15: LGTM! Clean implementation of async params handling
The changes correctly implement Next.js 15's async params pattern:
- Properly awaits the params promise
- Correctly destructures after awaiting
- Maintains type safety with PageProps
6-6: Verify the impact of async params type change
The change from sync to async params aligns with Next.js 15's architecture. However, we should verify that all consumers of this type are updated accordingly.
✅ Verification successful
Async params type change is consistent across the codebase
Based on the search results, the change to async params in the timeline page is consistent with the Next.js 15 migration pattern seen across the codebase. All other pages in the incidents section (alerts, chat, topology, workflows) and other parts of the application are already using Promise<{ id: string }> for their params type. The type is not exported or imported by other files, as it's locally scoped to each page component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files that might be affected by the PageProps type change
# Look for files importing or using PageProps from this module
# Search for imports of PageProps
rg -l "import.*PageProps.*from.*timeline/page"
# Search for direct usage of PageProps
rg -l ":\s*PageProps[\s{]"
Length of output: 77
Script:
#!/bin/bash
# Let's try a broader search to understand the usage of PageProps in the codebase
# and specifically look at the timeline page component structure
# Look for any PageProps type definitions or usages
rg -A 3 "PageProps"
# Check the structure of the timeline page component
cat "keep-ui/app/incidents/[id]/timeline/page.tsx"
# Look for any files that might be importing from this page
rg -l "from ['\"]\.\.\/.*timeline\/page['\"]"
rg -l "from ['\"].*\/incidents\/\[id\]\/timeline\/page['\"]"
Length of output: 9822
keep-ui/app/incidents/[id]/chat/page.tsx (2)
20-21: LGTM! Metadata generation properly handles async params
The changes correctly implement async params handling while maintaining the existing metadata generation logic.
6-6: Verify all consumers handle async params correctly
The change from synchronous to asynchronous params aligns with Next.js 13+ app router patterns. However, this is a breaking change that requires careful verification.
✅ Verification successful
Change is properly handled across the codebase
The search results show that all Next.js pages in the incidents directory are consistently handling the async params correctly:
- All page components are using
await props.paramsto access the params - Both the main page components and
generateMetadatafunctions properly await the params - The pattern is consistently implemented across all incident-related pages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files that might be affected by the params type change
# Look for files that use PageProps or directly access params
# Search for files importing or using PageProps
rg -l "PageProps" --type typescript
# Search for direct params access patterns that might need updating
rg -l "params: *{ *id: *string" --type typescript
# Search for potential usage of params without await
rg "params\." --type typescript -A 2 | grep -v "await"
Length of output: 329
Script:
#!/bin/bash
# Let's try a different approach without typescript file type specification
# Search for files importing or using PageProps
rg -l "PageProps"
# Search for direct params access patterns
rg -l "params.*id.*string"
# Search for potential usage of params
rg "params\." -A 2
Length of output: 12241
Script:
#!/bin/bash
# Let's examine how params are used in the identified Next.js pages to check for async/await usage
# Check the implementation of params usage in relevant files
rg -A 5 "async.*Page.*Props" keep-ui/app/incidents/
rg -A 5 "export.*async.*function.*Page" keep-ui/app/incidents/
# Check for await usage with params
rg -B 2 -A 2 "await.*params" keep-ui/app/incidents/
Length of output: 16746
keep-ui/app/incidents/[id]/workflows/page.tsx (3)
20-21: LGTM! Metadata handling is correct
The metadata generation function correctly implements the async/await pattern for the new Promise-based params while maintaining proper SEO metadata structure.
Line range hint 1-3: Verify import compatibility with Next.js 15
Let's ensure the imported utilities and components are compatible with Next.js 15.
✅ Verification successful
No Next.js 15 compatibility issues found in imports
The imports in the file are compatible with Next.js 15:
getIncidentNameis a local utility function with no Next.js dependenciesgetIncidentWithErrorHandlinguses standard Next.js navigation features (notFound) that are supported in Next.js 15- The component imports are local React components without Next.js-specific dependencies
The code follows Next.js 15 patterns for server components and data fetching.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any Next.js specific imports and their patterns
# Search for potential Next.js specific imports that might need updates
rg "from\s+['\"]next/" -t typescript
# Check if getIncidentWithErrorHandling is using any Next.js specific APIs
fd getIncidentWithErrorHandling.ts -t ts -x cat {}
Length of output: 394
Script:
#!/bin/bash
# Search for Next.js imports with correct ripgrep syntax
rg "from ['\"]next/"
# Find and show content of getIncidentWithErrorHandling file
fd getIncidentWithErrorHandling -x cat {}
# Check imports in the specific file
cat "keep-ui/app/incidents/[id]/workflows/page.tsx"
# Look for these utility functions to verify their implementation
fd -g "**/utils.ts" -x cat {}
Length of output: 14098
6-6: Verify Next.js 15 type pattern for params
The change to make params a Promise type aligns with Next.js 15's move towards async components. However, let's verify if this is the recommended type pattern.
✅ Verification successful
Let's try a different approach to understand the type patterns being used in the codebase, particularly focusing on Next.js page components and their params typing. This will help us verify if the Promise type pattern is consistent with other files that might have been migrated to Next.js 15.
Promise type for params is correctly implemented
The codebase shows a consistent pattern of using Promise<{ id: string }> for params in Next.js pages after the v15 migration:
- All incident-related pages (
/incidents/[id]/*) usePromise<{ id: string }> - Similar Promise-wrapped param types are used in other async pages like
/alerts/[id],/providers/oauth2/[providerType], and/workflows/[workflow_id] - The few files still using synchronous params (e.g.,
dashboard/[id]/page.tsx) likely need to be updated as part of the migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar params type patterns across the codebase
# and verify against Next.js 15 documentation
# Search for other Promise<{id: string}> type definitions
rg "params:\s*Promise<\{\s*id:\s*string\s*\}>" -t typescript
# Search for potential inconsistencies in params typing
rg "params:\s*\{\s*id:\s*string\s*\}" -t typescript
Length of output: 394
Script:
#!/bin/bash
# Search for params type patterns in TypeScript files
rg "params.*:\s*.*id.*string" --type-add 'ts:*.{ts,tsx}' -t ts
# Search for files that might be using Next.js page props
fd -e ts -e tsx | xargs grep -l "NextPage\|PageProps"
Length of output: 2164
keep-ui/app/incidents/[id]/activity/page.tsx (2)
26-31: LGTM! Metadata generation properly adapted.
The generateMetadata function correctly implements the new params handling while maintaining its existing metadata generation logic. The Promise-based params type and await usage are consistent with Next.js 15's patterns.
7-16: LGTM! Verify pattern consistency across routes.
The changes correctly implement Next.js 15's new pattern for handling route parameters as Promises. The two-step extraction of id with proper async/await is implemented correctly.
Let's verify this pattern is consistently applied across other route handlers:
✅ Verification successful
Pattern is consistently implemented across dynamic routes ✅
The verification shows that all dynamic route handlers in the codebase are correctly handling route parameters as Promises in Next.js 15 style. We can see consistent patterns across files:
- All dynamic route handlers properly type params as
Promise<T> - They correctly use either
awaitorusehook to handle the Promise - The pattern is consistently implemented across various features (incidents, workflows, alerts, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in params handling across Next.js route handlers
# Expected: All route handlers should handle params as Promise
# Search for page components with route parameters
ast-grep --pattern 'export default async function $_(props: { params: Promise<$_> })'
# Search for potential inconsistencies where params might not be handled as Promise
ast-grep --pattern 'export default function $_(props: { params: { $_ } })'
Length of output: 164
Script:
#!/bin/bash
# Let's try a different approach to find route handlers and their params handling
# Find all page.tsx files in the codebase
fd "page.tsx$" --type f --exec sh -c '
echo "=== File: {}"
# Look for export default function declarations and their context
rg "export default (async )?function.*params" -A 5 "{}"
'
Length of output: 1921
Script:
#!/bin/bash
# Let's specifically check the route parameter handling in dynamic route files
# Find all files in dynamic route folders (containing []) and check their content
fd "page\.tsx$" -p ".*/\[.*\]/" --type f --exec sh -c '
echo "\n=== File: {}"
# Show the entire content of these files as they are likely smaller and contain route params
cat "{}"
'
Length of output: 293
Script:
#!/bin/bash
# Let's try one more approach to find dynamic route handlers
# Find files in directories containing square brackets
fd "page\.tsx$" --type f | grep '\[.*\]' | while read -r file; do
echo "=== File: $file"
# Show export default function declarations and their context
rg "export default (async )?function" -B 2 -A 10 "$file"
echo "----------------------------------------"
done
Length of output: 8123
keep-ui/app/incidents/[id]/topology/page.tsx (2)
10-16: LGTM! Clean implementation of async parameter handling
The two-step approach to handling the async params (await first, then destructure) is clean and maintainable. This follows best practices for handling Promise-based parameters in Next.js 15.
7-7: Verify usage of PageProps type across the codebase
The change to make params a Promise type aligns with Next.js 15's parameter handling. However, we should verify that all components using this type have been updated accordingly.
✅ Verification successful
PageProps changes are consistent across the codebase
The search results show that all components using PageProps are already handling parameters asynchronously with async function declarations, except for two cases:
keep-ui/app/workflows/preview/page.tsxkeep-ui/app/dashboard/[id]/page.tsx
However, these are simple synchronous page components that don't need async parameter handling, as evidenced by their straightforward usage pattern ({ params }: PageProps). The change in the topology page aligns with the codebase's consistent handling of async parameters in Next.js 15.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files that might be using PageProps type
# to ensure they handle the Promise-based params correctly
# Search for PageProps usage
rg -l "PageProps" --type typescript
# Search for potential missed async handling of params
ast-grep --pattern 'function $_(props: PageProps) {
$$$
props.params
$$$
}'
Length of output: 154
Script:
#!/bin/bash
# Let's try different approaches to find PageProps usage
# Search for PageProps string in all files
rg "PageProps"
# Find TypeScript/JavaScript files that might use PageProps
fd "\.(ts|tsx|js|jsx)$" --exec grep -l "PageProps" {} \;
# Search for params access patterns in TypeScript/JavaScript files
rg "props\.params" --type ts --type tsx --type js --type jsx
# Look for async function components that might be handling params
rg "async.*function.*params.*\{" --type ts --type tsx
Length of output: 3187
keep-ui/app/workflows/preview/[workflowId]/page.tsx (1)
2-2: Verify React version compatibility for use hook
The use hook is part of React's newer APIs. Ensure your React version is compatible (18.0.0 or higher) since this hook is still considered experimental.
✅ Verification successful
React version is compatible with use hook
The project is using React version ^18.3.1, which fully supports the use hook. The usage is safe and aligns with Next.js 14's architecture for handling async data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check React version in package.json
jq '.dependencies.react' package.json
Length of output: 108
Script:
#!/bin/bash
# Try to find package.json files and check their content
fd package.json --type f --exec sh -c 'echo "=== {} ==="; cat {}'
Length of output: 4196
keep-ui/app/topology/page.tsx (2)
23-23: LGTM! Function signature properly updated for async params handling.
The change to accept props instead of destructured parameters is correct, maintaining proper typing and async handling.
16-20: LGTM! Type changes align with Next.js 15 patterns.
The conversion of searchParams to a Promise type is correct and follows Next.js 15's new patterns for server components.
✅ Verification successful
Let me gather more information to verify the pattern across the codebase.
The Promise-wrapped searchParams type is correctly implemented and consistent with other pages
The verification shows that other page components in the codebase also use Promise-wrapped searchParams types, following the same pattern required by Next.js 15. The implementation in topology/page.tsx is consistent with this pattern, with proper typing of the optional properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of Promise-wrapped searchParams across the codebase
# Expect: All page components should use Promise-wrapped searchParams
# Search for other page components using searchParams
ast-grep --pattern 'type PageProps = {
searchParams: $_
}'
Length of output: 426
Script:
#!/bin/bash
# Search for all page.tsx files and their searchParams usage
fd "page.tsx$" --exec rg -A 5 "searchParams" {}
Length of output: 2792
keep-ui/app/workflows/preview/page.tsx (2)
2-2: Verify the usage of experimental React features
The use hook and Promise-based params are part of React's concurrent features. While this aligns with Next.js 15's architecture, ensure your team is aware of the following:
- The
usehook is still experimental in React - This change introduces concurrent rendering patterns
- Error boundaries should be in place to handle Promise rejections
Also applies to: 8-9
1-1: Verify Next.js 15 migration completeness
Since this file is part of the Next.js 15 upgrade, verify that all required changes are in place:
- "use client" directive is correctly positioned
- Async component patterns are properly implemented
- Client/Server component separation is maintained
✅ Verification successful
Next.js 15 migration patterns are correctly implemented
The file follows all the required Next.js 15 patterns:
- "use client" directive is correctly positioned at the top
- Client component patterns are properly implemented with hooks (useState, useEffect)
- Uses the new
usehook for handling promises from params/searchParams - Proper component separation with client components imported from separate files
- Suspense boundaries are correctly implemented for loading states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper Next.js 15 patterns across the codebase
# Verify "use client" directive placement
rg -l "^\"use client\"" --type typescript
# Check for proper async component patterns
ast-grep --pattern 'async function $name($_) {
$$$
}'
# Verify client components are properly marked
rg -l "^\"use client\".*\nexport.*function.*Page" --type typescript
Length of output: 394
Script:
#!/bin/bash
# Check for proper Next.js 15 patterns across the codebase
# Verify "use client" directive placement
rg -l "^\"use client\"" --type ts --type tsx
# Check for async component patterns
rg "async (function|const) .*(Page|Component|Layout)" --type ts --type tsx -A 2
# Verify client components are properly marked
rg -U "^\"use client\"[\s\S]*?export.*function.*Page" --type ts --type tsx
Length of output: 285
Script:
#!/bin/bash
# Let's try without file type filters since ripgrep doesn't recognize ts/tsx
# Check for "use client" directive placement
find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "^\"use client\"" {} \;
# Check for async component patterns and client components
find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -A 2 "async.*function.*Page\|export.*function.*Page" {} \;
# Look at the specific file content
cat "keep-ui/app/workflows/preview/page.tsx"
Length of output: 9390
keep-ui/app/providers/oauth2/[providerType]/page.tsx (2)
7-12: LGTM: Proper implementation of Next.js v15 async params
The type definitions correctly implement Next.js v15's async route parameters pattern, ensuring type safety for both params and searchParams.
18-21: LGTM: Proper async cookie handling
The cookie store is correctly awaited and the cookie retrieval pattern is well-structured.
keep-ui/app/layout.tsx (3)
14-14: LGTM! The import change follows Next.js best practices.
The change from dynamic import to a dedicated client component file (.client.tsx) follows Next.js recommended patterns for explicit client/server component separation.
Line range hint 29-65: Consider leveraging Next.js 15 features for optimization.
Since this PR is upgrading to Next.js 15, consider these potential optimizations:
- Evaluate if any providers (
PHProvider,NextAuthProvider, etc.) have Next.js 15 specific optimizations - Consider using Partial Prerendering (PPR) for the dynamic parts of the layout
Let's check for provider usage patterns:
Line range hint 39-39: Consider removing @ts-ignore by properly typing the component.
The @ts-ignore-error Server Component comment suggests a type conflict between server and client components. With Next.js 15's improved typing system, we should be able to resolve this without ignoring TypeScript errors.
Let's check if there are similar patterns in the codebase:
keep-ui/package.json (1)
7-7: LGTM: Turbopack enabled for development
The addition of --turbopack flag aligns with the PR objectives to leverage the stable Turbopack Dev feature in Next.js v15, which should provide significant performance improvements in development.
keep-ui/app/alerts/[id]/page.tsx (2)
4-5: Confirm the necessity of defining params and searchParams as Promise types.
Defining params and searchParams as Promise types may not be necessary unless they are provided asynchronously by the framework. Please verify if this aligns with Next.js v15 conventions.
8-9: Asynchronous handling of params is appropriate.
Awaiting props.params in the async function aligns with the definition of params as a Promise. Ensure consistent usage throughout the application.
| export default function Page(props:{params: Promise<{ workflow_id: string }>}) { | ||
| const params = use(props.params); | ||
| return <WorkflowDetailPage params={params}/>; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Promise-based params handling is inconsistent across route handlers
The verification reveals inconsistent patterns in handling route parameters across the codebase:
-
Some pages use the
usehook with Promise-based params:/workflows/[workflow_id]/page.tsx/workflows/[workflow_id]/runs/[workflow_execution_id]/page.tsx/workflows/preview/[workflowId]/page.tsx/workflows/preview/page.tsx
-
While others use async/await pattern:
/workflows/builder/[workflowId]/page.tsx/incidents/[id]/*pages/dashboard/[id]/page.tsx- Several other route handlers
For consistency, consider standardizing the approach across all route handlers, preferably using the async/await pattern which is more commonly used in the codebase.
🔗 Analysis chain
Verify consistent implementation across route handlers
Let's verify that this pattern of handling promise-based params is consistently implemented across other route handlers in the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent implementation of promise-based params across route handlers
# Expected: All page components should handle Promise<params> similarly
# Search for page components with params prop type
ast-grep --pattern 'export default function Page($props: { params: $type }) {
$$$
}'
# Search for usage of the 'use' hook with params
rg "const params = use\(.*params.*\)" -t tsx
Length of output: 163
Script:
#!/bin/bash
# Let's try a different approach to find route handlers and their params handling
# Find all page.tsx files in the app directory
fd page.tsx
# Then let's examine their content
fd page.tsx --exec cat {}
# Also search for use of the 'use' hook without file type restriction
rg "const params = use\("
Length of output: 20371
| const searchParams = await props.searchParams; | ||
| return <ProvidersPage searchParams={searchParams} />; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and optimize Promise resolution
The current implementation could be improved with proper error handling and optimization:
- Missing error handling for Promise rejection
- Unnecessary Promise resolution when searchParams is undefined
Consider this implementation:
- const searchParams = await props.searchParams;
- return <ProvidersPage searchParams={searchParams} />;
+ try {
+ const searchParams = props.searchParams ? await props.searchParams : undefined;
+ return <ProvidersPage searchParams={searchParams} />;
+ } catch (error) {
+ console.error('Failed to resolve search parameters:', error);
+ // Handle error appropriately
+ return <ProvidersPage searchParams={undefined} />;
+ }📝 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 searchParams = await props.searchParams; | |
| return <ProvidersPage searchParams={searchParams} />; | |
| try { | |
| const searchParams = props.searchParams ? await props.searchParams : undefined; | |
| return <ProvidersPage searchParams={searchParams} />; | |
| } catch (error) { | |
| console.error('Failed to resolve search parameters:', error); | |
| // Handle error appropriately | |
| return <ProvidersPage searchParams={undefined} />; | |
| } |
| export default function PageWithId( | ||
| props: { | ||
| params: Promise<{ workflowId: string }>; | ||
| } | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error boundary for Promise rejection
The async params could potentially reject, but there's no explicit error handling. Consider wrapping this component with an error boundary or implementing try-catch with the use hook.
+import { ErrorBoundary } from 'react-error-boundary';
+function ErrorFallback({error}: {error: Error}) {
+ return (
+ <div className="text-red-500">
+ <p>Something went wrong:</p>
+ <pre>{error.message}</pre>
+ </div>
+ );
+}
export default function PageWithId(
props: {
params: Promise<{ workflowId: string }>;
}
) {
+ return (
+ <ErrorBoundary FallbackComponent={ErrorFallback}>
+ <PageContent {...props} />
+ </ErrorBoundary>
+ );
+}
+function PageContent(props: {
+ params: Promise<{ workflowId: string }>;
+}) {
// ... rest of the component logicCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
keep-ui/utils/fetcher.ts (1)
6-13: Consider adding type validation for headersWhile the current implementation works, adding runtime validation for critical headers could prevent potential issues.
Consider adding a type guard:
type SafeHeaders = Omit<HeadersInit, 'Authorization'>; export const fetcher = async ( url: string, accessToken: string | undefined, { headers: requestInitHeaders, ...requestInit }: Omit<RequestInit, 'headers'> & { headers?: SafeHeaders } = {} ) => {keep-ui/entities/incidents/api/incidents.ts (2)
60-65: Reconsider the caching strategy for incident dataWhile caching can improve performance, using
force-cachefor incident data might lead to stale information since incidents are typically dynamic entities that change state frequently. Consider using a more appropriate caching strategy:Here are some alternatives to consider:
- Use
revalidatewith a shorter TTL to ensure data freshness- Use
no-storefor real-time incident data- Implement stale-while-revalidate pattern using
next: { revalidate: 0 }Example implementation with revalidation:
return (await fetcher(url, session?.accessToken, { headers: { - cache: "force-cache", + next: { revalidate: 300 }, // Revalidate every 5 minutes }, })) as IncidentDto;
61-65: Improve type safety for the fetcher responseThe type assertion to
IncidentDtomight not be safe with cached responses. Consider adding runtime validation.Consider using a validation library like Zod:
import { z } from 'zod'; const IncidentSchema = z.object({ // define your incident schema here }); return IncidentSchema.parse( await fetcher(url, session?.accessToken, { headers: { next: { revalidate: 300 }, }, }) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
keep-ui/entities/incidents/api/incidents.ts(1 hunks)keep-ui/utils/fetcher.ts(1 hunks)keep-ui/utils/hooks/useAlerts.ts(0 hunks)keep-ui/utils/hooks/useSearchAlerts.ts(0 hunks)
💤 Files with no reviewable changes (2)
- keep-ui/utils/hooks/useAlerts.ts
- keep-ui/utils/hooks/useSearchAlerts.ts
🔇 Additional comments (2)
keep-ui/utils/fetcher.ts (2)
6-6: LGTM: Improved parameter destructuring pattern
The destructuring pattern provides better clarity by explicitly separating headers from other request initialization options. This change maintains type safety while making the code more maintainable.
11-11: Verify header precedence behavior
The spread operator placement means custom headers will override the default Authorization header. Ensure this is the intended behavior, as it could potentially allow overwriting of critical authentication headers.
Consider adding a safeguard to prevent overriding critical headers:
headers: {
Authorization: `Bearer ${accessToken}`,
- ...requestInitHeaders,
+ ...(requestInitHeaders || {}),
+ // Ensure Authorization can't be overridden
+ ...(requestInitHeaders?.Authorization ? { Authorization: `Bearer ${accessToken}` } : {})
},✅ Verification successful
Let me analyze the fetcher usage further to understand the complete context.
Let me try a different pattern to find the fetcher implementation.
Based on the analysis of both the fetcher implementation and its usage across the codebase, I can now provide the final response:
Header precedence is correctly implemented and safe
The current implementation is actually the desired behavior. From the codebase analysis:
- All existing usages of the fetcher only add non-conflicting headers like 'Content-Type', 'Accept', or 'cache'
- No instances were found where the Authorization header is being overridden
- The spread operator allows for extending headers while maintaining the authentication, which is a common and valid pattern
The suggested safeguard is unnecessary as the current implementation follows standard practices and the codebase shows disciplined usage of headers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all fetcher usage to verify if any calls might override the Authorization header
rg -l 'fetcher\(' | xargs rg -B 3 -A 3 'headers:'
Length of output: 7657
Script:
#!/bin/bash
# Get the implementation of the fetcher function
ast-grep --pattern 'export async function fetcher'
Length of output: 52
Script:
#!/bin/bash
# Try to find the fetcher implementation using ripgrep
rg -A 20 "export (const|async function) fetcher" keep-ui/utils/fetcher.ts
Length of output: 754
Kiryous
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
keep-ui/components/navbar/Search.tsx (2)
259-260: Consider using a more robust platform detection approach.While the current implementation works, consider these improvements for more reliable platform detection:
- Use the modern
navigator.userAgentData.platformAPI when available- Add fallback for Safari and older browsers
const isMac = () => { + // Try modern API first + if (navigator.userAgentData?.platform) { + return navigator.userAgentData.platform === 'macOS'; + } + // Fallback for Safari and older browsers const platform = navigator.platform.toLowerCase(); const userAgent = navigator.userAgent.toLowerCase(); return ( platform.includes("mac") || (platform.includes("iphone") && !userAgent.includes("windows")) ); };
Line range hint
266-272: Optimize performance with useMemo.Since
isMac()is used in auseEffectand its result depends on browser APIs that don't change during the session, consider memoizing the result:-const isMac = () => { +const isMac = useMemo(() => { const platform = navigator.platform.toLowerCase(); const userAgent = navigator.userAgent.toLowerCase(); return ( platform.includes("mac") || (platform.includes("iphone") && !userAgent.includes("windows")) ); -}; +}, []);Also, as noted in the TODO comment, consider implementing a context provider for user agent detection to make it reusable across components.
|
@Kiryous i'm closing this one for now. if it's relevant lets reopen it |
Closes #2512
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
package.jsonto reflect new development scripts and dependency versions.Chores