Next 15 upgrade #309
Merged
Next 15 upgrade #309
+2,785
−3,431
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| props: { | ||
| // Only accessible in PageComponent rather than layout component | ||
| // @see https://github.com/vercel/next.js/issues/43704 | ||
| searchParams: Promise<Record<string, string | string[] | undefined>> |
There was a problem hiding this comment.
The searchParams prop is incorrectly typed as a Promise. In Next.js page components, searchParams is provided directly as a Record, not as a Promise. This will cause type errors and runtime issues when trying to await a non-Promise value.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| searchParams: Promise<Record<string, string | string[] | undefined>> | ||
| } | ||
| ) { | ||
| const searchParams = await props.searchParams; |
There was a problem hiding this comment.
Attempting to await searchParams which is not actually a Promise in Next.js page components. This line should be removed as searchParams can be accessed directly from props.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| // TODO: Can we use cookies-next to read cookie in this environment? | ||
| const cookie = cookies().get(kConnectSession) | ||
| const cookie = (await cookies()).get(kConnectSession) |
There was a problem hiding this comment.
The cookies() function from next/headers returns a ReadonlyRequestCookies object directly, not a Promise. It should not be awaited. The correct usage is: const cookie = cookies().get(kConnectSession)
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| props: { | ||
| // Search params in Next.js are only accessible in PageComponent and no accessible in layout component, too bad | ||
| // @see https://github.com/vercel/next.js/issues/43704 | ||
| searchParams: Promise<Record<string, string | string[] | undefined>> |
There was a problem hiding this comment.
The searchParams parameter in Next.js pages should not be typed as a Promise. Next.js automatically provides searchParams as a plain Record<string, string | string[] | undefined>. Making it a Promise adds unnecessary complexity and is incorrect according to Next.js page props typing.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| searchParams: Promise<Record<string, string | string[] | undefined>> | ||
| } | ||
| ) { | ||
| const searchParams = await props.searchParams; |
There was a problem hiding this comment.
Since searchParams should not be a Promise, awaiting it is unnecessary and incorrect. This line should be removed and props.searchParams should be used directly.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| props: { | ||
| // Search params in Next.js are only accessible in PageComponent and no accessible in layout component, too bad | ||
| // @see https://github.com/vercel/next.js/issues/43704 | ||
| searchParams: Promise<Record<string, string | string[] | undefined>> |
There was a problem hiding this comment.
The searchParams property should not be a Promise type. In Next.js pages, searchParams is a direct object containing query parameters, not a Promise. The correct type should be Record<string, string | string[] | undefined>
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| searchParams: Promise<Record<string, string | string[] | undefined>> | ||
| } | ||
| ) { | ||
| const searchParams = await props.searchParams; |
There was a problem hiding this comment.
Since searchParams should not be a Promise, awaiting it is incorrect and unnecessary. This line should be removed and props.searchParams should be used directly.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| export default async function OrgLayout( | ||
| props: { | ||
| children: React.ReactNode | ||
| params: Promise<{slug: string}> |
There was a problem hiding this comment.
The params property should not be of type Promise. In Next.js page/layout components, params are directly passed as objects from the router, not as Promises. This will cause type errors and potential runtime issues. It should be params: {slug: string}
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
|
||
| // export const Success2 = stories['success'] | ||
|
|
||
| // module.exports = stories |
There was a problem hiding this comment.
The code uses ES modules (import/export) syntax throughout but attempts to use CommonJS 'module.exports' at the end. This mixing of module systems is incorrect. In an ES module, you should only use 'export' statements.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| } satisfies Meta<typeof StatusBadge> | ||
|
|
||
| export default meta | ||
| type Story = StoryObj<typeof meta> |
There was a problem hiding this comment.
The Story type is incorrectly typed as StoryObj when it should be StoryObj. The meta object is the configuration, but the Story type should reference the component type directly.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
packages/ui/v1/StatusBadge.tsx
Outdated
| | 'info' | ||
| | 'default' | ||
|
|
||
| export interface StatusBadgeProps extends React.HTMLAttributes<HTMLDivElement> { |
There was a problem hiding this comment.
The StatusBadge component uses the Badge component which might not render a div element. The HTMLAttributes type should match the actual rendered element type. Consider using a more specific type or HTMLAttributes if the underlying element type is unknown.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
😱 Found 11 issues. Time to roll up your sleeves! 😱 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to d238ae3 in 2 minutes and 28 seconds
More details
- Looked at
577lines of code in16files - Skipped
1files when reviewing. - Skipped posting
35drafted comments based on config settings.
1. apps/web/app/api/proxy/[[...path]]/route.ts:4
- Draft comment:
Ensure that re-exporting proxyHandler complies with Next.js conventions. Verify if components really use 'params' or 'searchParams' so that proxyHandler is set up correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify if components useparamsorsearchParamsto ensureproxyHandleris set up correctly. This falls under asking the author to double-check things, which is against the rules. The comment does not provide a specific suggestion or point out a clear issue.
2. apps/web/app/connect/(oauth)/callback/page.tsx:40
- Draft comment:
Casting 'value' as string may lose data if searchParams values are arrays. Consider handling array values properly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. apps/web/app/connect/file-picker/page.tsx:38
- Draft comment:
Updated parameter type for searchParams now uses a Promise. Ensure that all consuming components supply a Promise that resolves to the appropriate searchParams object. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that all consuming components supply aPromise. This falls under the rule of not asking the author to ensure behavior is intended or tested. The comment does not provide a specific suggestion or point out a specific issue with the code.
4. apps/web/app/connect/portal/page.tsx:39
- Draft comment:
Similar to file-picker, the async searchParams prop is used. Confirm that all callers pass a Promise and that 'await props.searchParams' is safe. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to confirm that all callers pass aPromiseand thatawait props.searchParamsis safe. This falls under asking the author to confirm their intention or to ensure the behavior is intended, which is against the rules.
5. apps/web/app/dashboard/(authenticated)/connector-configs/ConnectorConfigForm.tsx:17
- Draft comment:
The 'formRef' type now allows null. Make sure that consumers properly handle the possibility of a null ref. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that consumers handle a null ref, which is similar to asking them to double-check or ensure behavior. This violates the rule against asking the author to ensure behavior is intended or tested.
6. apps/web/app/dashboard/org/[slug]/layout.tsx:6
- Draft comment:
Awaiting a Promise for route parameters is now enforced. Verify that all downstream consumers properly handle the awaited 'params' value. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify that downstream consumers handle the awaited 'params' value properly. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
7. apps/web/lib-server/server-helpers.ts:43
- Draft comment:
The type for cookies() is updated to return a Promise. Ensure that all invocations of cookies() (e.g., in callback handlers) use 'await' to prevent race conditions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that all invocations ofcookies()useawait. This is a request for the author to double-check their code, which violates the rule against asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.
8. apps/web/package.json:10
- Draft comment:
Switching from '--turbo' to '--turbopack' and version bumps to Next.js and React may introduce breaking changes. Confirm that all tooling and build processes are compatible with these upgrades. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm compatibility with tooling and build processes after a version change, which is against the rules. It does not provide a specific code suggestion or ask for a specific test to be written.
9. packages/ui/hooks/useConstant.ts:8
- Draft comment:
Consider initializing the ref with null (i.e. useRef<ResultBox | null>(null)) instead of undefined to align with common React patterns and improve type clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While it's true that React examples often use null, using undefined here works perfectly fine and doesn't cause any issues. The type checking and runtime behavior would be identical. This seems like a purely stylistic preference without any real technical benefit. The current code is clear and functional.
Perhaps there are subtle TypeScript type inference benefits to using null that I'm not considering. Maybe there's a React internal optimization for null refs that I'm unaware of.
Even if there were minor benefits, they would be so minimal as to not justify a code change. The current code is perfectly functional and clear.
This comment suggests a style change without providing any meaningful benefit. It should be deleted as it doesn't highlight any actual issues.
10. packages/ui/v1/StatusBadge.stories.ts:35
- Draft comment:
Clean up commented-out story code if it's no longer needed to improve clarity. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
11. apps/web/app/api/proxy/[[...path]]/route.ts:4
- Draft comment:
Codemod comment: verify if proxyHandler needs special handling of 'params' or 'searchParams'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify ifproxyHandlerneeds special handling ofparamsorsearchParams. This falls under asking the author to confirm or verify something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue.
12. apps/web/app/connect/(oauth)/callback/page.tsx:25
- Draft comment:
Converting searchParams prop to a Promise and awaiting it is intentional; ensure cookies() is async and all async changes are consistently applied. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure thatcookies()is async and that all async changes are consistently applied. This falls under asking the author to ensure behavior, which is against the rules. However, the first part of the comment is informative, explaining that convertingsearchParamsto aPromiseis intentional. Overall, the comment does not provide a specific code suggestion or ask for a specific test, so it should be removed.
13. apps/web/app/connect/file-picker/page.tsx:38
- Draft comment:
Changing searchParams to a Promise and awaiting it is consistent; consider adding error handling for potential promise rejections. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is a Next.js page component where searchParams is provided by the framework. The change to make it a Promise seems intentional based on the comment about Next.js issues. Since this is a framework-provided value, we should trust that Next.js handles Promise rejections appropriately. Adding error handling here would be defensive programming against framework internals.
The comment raises a valid point about Promise error handling in general. What if the Promise does reject and we're not catching it?
Since searchParams is provided by Next.js framework itself, we should trust the framework to handle Promise rejections. Adding error handling here would be over-engineering.
Delete the comment. Error handling for framework-provided Promises should be handled by the framework itself.
14. apps/web/app/connect/portal/page.tsx:39
- Draft comment:
The update to use a Promise for searchParams aligns with new patterns; ensure all consumers expect the asynchronous behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that all consumers expect the asynchronous behavior when using a Promise. This falls under the category of asking the author to ensure behavior is intended or tested, which violates the rules.
15. apps/web/app/dashboard/(authenticated)/connector-configs/ConnectorConfigForm.tsx:17
- Draft comment:
Allowing formRef to be null improves type safety for uninitialized refs; verify consuming code handles 'null' appropriately. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is suggesting to verify that the consuming code handlesnullappropriately. This falls under asking the PR author to ensure behavior is intended or to double-check things, which is against the rules. The comment does not provide a specific code suggestion or ask for a specific test to be written.
16. apps/web/app/dashboard/org/[slug]/layout.tsx:5
- Draft comment:
Converting 'params' to be a Promise and awaiting it could introduce slight delay; ensure all callers now provide a promise for params. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
The comment is suggesting a potential issue with convertingparamsto a Promise and awaiting it, which could introduce a delay. However, it asks the author to ensure that all callers provide a promise, which is against the rules as it asks for confirmation and verification. The comment does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
17. apps/web/lib-server/server-helpers.ts:42
- Draft comment:
Changing the cookies accessor to return a Promise affects consuming code; double-check that all calls properly await cookies(). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to double-check that all calls properly await thecookies()function after changing it to return a Promise. This falls under the rule of not asking the author to double-check things. The comment does not provide a specific suggestion or point out a specific issue, but rather asks for a general confirmation.
18. apps/web/package.json:10
- Draft comment:
Upgrading Next.js (14.2.13 -> 15.2.1) and React (18.3.1 -> 19.0.0) as well as switching to turbopack; ensure compatibility with plugins and custom configurations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is related to dependency changes, specifically upgrading Next.js and React versions. It also mentions ensuring compatibility with plugins and custom configurations, which falls under the category of asking the PR author to ensure something is tested or verified. This violates the rules.
19. packages/ui/hooks/useConstant.ts:8
- Draft comment:
Explicitly initializing the ref with undefined clarifies the type; confirm that this aligns with expected behavior for constant values. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to confirm their intention regarding the initialization of arefwithundefined. This violates the rule against asking the author to confirm their intention or expected behavior. The comment does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
20. packages/ui/hooks/useWhyDidYouUpdate.ts:7
- Draft comment:
Initializing previousProps with undefined prevents type issues; ensure this doesn't affect the hook's comparison logic. - Reason this comment was not posted:
Comment looked like it was already resolved.
21. packages/ui/package.json:66
- Draft comment:
The addition of 'concurrently' as a devDependency is acceptable; verify that its version is compatible within the monorepo context. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is about a dependency change, specifically the addition of 'concurrently' as a devDependency. It asks the PR author to verify compatibility, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue with the code, making it purely informative and not actionable.
22. packages/ui/shadcn/Breadcrumb.tsx:1
- Draft comment:
Breadcrumb component has been deleted; ensure no downstream consumers depend on this removed component. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
23. packages/ui/shadcn/index.ts:3
- Draft comment:
Removed export of Breadcrumb from the shadcn barrel; confirm that this removal does not break any imports elsewhere. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that the removal of an export does not break any imports elsewhere. This falls under asking the author to ensure the behavior is intended or to double-check things, which is against the rules.
24. packages/ui/v1/StatusBadge.tsx:53
- Draft comment:
StatusBadge’s configuration mapping is clear; verify that the imported Badge component behaves consistently with the intended styling. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify the behavior of the imported Badge component, which violates the rule against asking for confirmation or verification of intended behavior. It does not provide a specific suggestion or point out a specific issue.
25. apps/web/app/connect/(oauth)/callback/page.tsx:20
- Draft comment:
Typo: 'ahve' should be corrected to 'have'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
26. apps/web/app/connect/(oauth)/callback/page.tsx:37
- Draft comment:
Typo: 'accomodate' should be corrected to 'accommodate'. - Reason this comment was not posted:
Comment was on unchanged code.
27. apps/web/app/connect/(oauth)/callback/page.tsx:57
- Draft comment:
Typo in event type: 'AUTHORIZATION_SUCEEDED' should be 'AUTHORIZATION_SUCCEEDED'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
28. apps/web/app/connect/file-picker/page.tsx:17
- Draft comment:
Typo found in comment: Replace 'ahve' with 'have' on line 17. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
29. apps/web/app/connect/file-picker/page.tsx:28
- Draft comment:
Typographical error in the commented URL on line 28: Remove the stray backtick () in '692499'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
30. apps/web/app/connect/file-picker/page.tsx:40
- Draft comment:
Typographical error in comment on line 40: Replace 'no accessible' with 'not accessible'. - Reason this comment was not posted:
Comment was on unchanged code.
31. apps/web/app/connect/portal/page.tsx:18
- Draft comment:
Typo detected: 'Will ahve to check' should be corrected to 'Will have to check'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
32. apps/web/app/connect/portal/page.tsx:41
- Draft comment:
Typo in comment: 'no accessible in layout component' should be corrected to 'not accessible in layout component'. - Reason this comment was not posted:
Comment was on unchanged code.
33. apps/web/app/connect/portal/page.tsx:29
- Draft comment:
Typographical error in the URL: remove the backtick from '69249`9' to make it '69249'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
34. apps/web/app/dashboard/(authenticated)/connector-configs/ConnectorConfigForm.tsx:9
- Draft comment:
Typo detected in the import: 'useCurrengOrg' on line 9. Consider renaming it to 'useCurrentOrg' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
35. apps/web/lib-server/server-helpers.ts:162
- Draft comment:
Typo found in the inline comment on line 162: 'ignoroing' should be corrected to 'ignoring'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_qHXukMRHSx7LV8Ia
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| args: {status: 'error'}, | ||
| } | ||
|
|
||
| // Not working yet |
There was a problem hiding this comment.
Commented-out experimental story code remains in the file; consider cleaning up unused blocks to maintain clarity.
openint-bot
force-pushed
the
next-upgrade
branch
from
d238ae3 to
6170ab5
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 6170ab5 in 2 minutes and 56 seconds
More details
- Looked at
338lines of code in9files - Skipped
1files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. kits/file-picker/package.json:55
- Draft comment:
Avoid using '*' for react/react-dom/@types/react. Wildcards can lead to unpredictable updates; prefer specific version ranges. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
Using "" for React versions can indeed cause issues since different React versions can have breaking changes. However, this appears to be in a component library package, where React is likely a peer dependency. In component libraries, it's actually common practice to use "" for React to allow the consuming application to determine the React version.
I might be wrong about this being a component library, or there might be specific requirements for version pinning in this project that I'm not aware of.
Even if I'm wrong about the specific context, using "*" for React in dependencies is a common and valid pattern for libraries, and without stronger evidence that this is problematic, we should trust the author's intention.
The comment should be deleted as it suggests changing what appears to be an intentional and valid pattern for component libraries.
2. kits/file-picker/package.json:10
- Draft comment:
Removed Next.js specific 'dev' and 'start' scripts. Confirm that these removals are intentional now that the package is no longer a Next.js app. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment violates our rules by asking for confirmation ("Confirm that..."). It's asking the author to verify their intention, which is explicitly against our rules. The removal of Next.js scripts along with Next.js dependencies is clearly intentional as part of converting this from a Next.js app to a regular package.
Maybe the removal of Next.js scripts could break something for developers who were using this package? Maybe there should be documentation about the change?
The package.json clearly shows this is being converted to a standard npm package with appropriate scripts. Documentation updates would be handled separately, not in a PR comment.
Delete this comment as it violates our rule against asking authors to confirm their intentions, and the changes are clearly intentional given the context.
3. kits/file-picker/package.json:4
- Draft comment:
Consider updating the description to use the correct capitalization for official product names. 'Sharepoint' should be 'SharePoint' and 'One Drive' should likely be 'OneDrive'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_06erhY4ZSVpgKrYy
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| "next": "^14.1.0", | ||
| "next-themes": "^0.2.1", | ||
| "react": "^18.2.0", | ||
| "react": "*", |
There was a problem hiding this comment.
Changed React versions to * (also for react-dom and @types/react). Using * can lead to unpredictable dependency resolutions. Consider specifying a semver range or moving these to peerDependencies.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on cfa104b in 1 minute and 24 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. packages/ui/stories/examples/Image.stories.skip.tsx:4
- Draft comment:
Link the Next.js 15 upgrade issue for context. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. packages/ui/stories/examples/Image.stories.skip.tsx:1
- Draft comment:
Renaming the file to .skip disables the story. Confirm that this is the intended temporary workaround for Next.js 15 issues. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. packages/ui/stories/examples/Image.stories.skip.tsx:4
- Draft comment:
Consider adding a reference or issue link about the Next.js 15 upgrade problem for better context. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to add a reference or issue link for context. It seems to be asking for an update to the PR description or additional context, which is not allowed by the rules.
4. packages/ui/stories/examples/Image.stories.skip.tsx:4
- Draft comment:
Typo: Consider capitalizing 'Next.js' on line 4. Instead of 'next.js 15 upgrade....', it might be more readable and consistent to write 'Next.js 15 upgrade...'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a purely stylistic suggestion about capitalization in a comment. It doesn't affect functionality, readability of the code itself, or introduce any technical issues. The comment is about something that changed (the new comment was added), but suggesting proper capitalization of product names in comments is extremely low value.
The proper capitalization of product names could be considered part of maintaining professional documentation standards. The comment is technically correct about the official capitalization of Next.js.
While technically correct, this level of nitpicking about capitalization in internal code comments violates our rule about not making obvious or unimportant suggestions. Code comments are informal documentation and don't need this level of polish.
Delete this comment as it's too trivial and doesn't meaningfully improve the code quality.
Workflow ID: wflow_JWty8UpWxVF6Fz4m
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6f9a539 in 42 seconds
More details
- Looked at
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. apps/web/app/api/proxy/[[...path]]/route.ts:3
- Draft comment:
Removed redundant codemod comments. Make sure any hints regardingparamsandsearchParamsare not needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that hints regardingparamsandsearchParamsare not needed. This falls under the category of asking the author to double-check or ensure something, which is against the rules. Therefore, this comment should be removed.
2. apps/web/app/api/proxy/[[...path]]/route.ts:4
- Draft comment:
Removed Next codemod error comments for proxyHandler. Confirm that proxyHandler handles params/searchParams correctly for all HTTP methods. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/web/app/api/proxy/[[...path]]/route.ts:4
- Draft comment:
Re-exporting proxyHandler for all HTTP methods is intentional. Ensure that method-specific nuances (if needed) are addressed within proxyHandler. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_iqoWBkRDMuFFIN0o
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on adf39a1 in 48 seconds
More details
- Looked at
46lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. apps/web/app/api/v0-edge/[...trpc]/route.ts:1
- Draft comment:
Confirm intentional removal of the edge route handler. Ensure that any client or server references to this endpoint are updated since this file handled important request/response modifications (e.g., header adjustments for chunked transfers). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. apps/web/app/api/v0-edge/[...trpc]/route.ts:1
- Draft comment:
Ensure the complete removal of this legacy Edge route handler is intended and that no dependencies remain on this endpoint. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_JJfpBJieFR97LfF9
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 48d315e in 2 minutes and 5 seconds
More details
- Looked at
39lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/web/next.config.js:21
- Draft comment:
Verified addition of file-picker to transpilePackages; ensure this is intentional. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/next.config.js:21
- Draft comment:
Added file-picker to transpilePackages. Confirm that transpiling TS source here is intended versus using a pre-built dist. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm their intention regarding the transpiling of a package. This falls under the rule of not asking the author to confirm their intention or to ensure the behavior is intended. Therefore, this comment should be removed.
3. apps/web/next.config.js:63
- Draft comment:
The multi-line formatting for the rewrite destination is a stylistic change; consider keeping it consistent with similar rewrite entries. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
4. kits/file-picker/package.json:5
- Draft comment:
Changing 'main' to point to the TypeScript source ('./src/index.ts') may affect consumers that expect a compiled JavaScript file. Verify if this is intentional for internal use. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The presence of prepublishOnly script means the package will always be built before being published. The change to point to src/index.ts suggests this is an internal package where TypeScript source is preferred. The @OpenInt scope and 0.0.2 version suggest this is indeed internal. The comment is asking for verification rather than pointing out a clear issue.
I might be wrong about the internal nature of the package. Even if internal, other teams might still expect compiled JavaScript.
The prepublishOnly script ensures compiled code for npm consumers, and internal consumers likely prefer TypeScript source for better type information and debugging.
Delete the comment. It's asking for verification rather than pointing out a clear issue, and the setup appears intentional with prepublishOnly handling npm publishing correctly.
5. kits/file-picker/package.json:2
- Draft comment:
Removal of 'types' and 'files' fields may lead to missing type declarations or unintended publishing of extra files. Confirm that these changes align with your packaging strategy. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the PR author to confirm their intention regarding the removal of 'types' and 'files' fields. This violates the rule against asking for confirmation of intention. However, it does point out a potential issue with missing type declarations or unintended publishing, which could be useful. The comment could be rephrased to suggest checking the packaging strategy without asking for confirmation.
6. kits/file-picker/package.json:4
- Draft comment:
In the package.json description, consider correcting the casing and spacing of provider names for consistency and accuracy: 'Sharepoint' should be 'SharePoint' and 'One Drive' should be 'OneDrive'. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_9umLNrmFKuovN7aO
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| "files": [ | ||
| "dist" | ||
| ], | ||
| "main": "./src/index.ts", |
There was a problem hiding this comment.
Changing 'main' to point to src may expose TS source instead of compiled output; removal of 'types' & 'files' might be unintentional.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3af4b30 in 59 seconds
More details
- Looked at
307lines of code in7files - Skipped
2files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. apps/connect-dev/.env.example:1
- Draft comment:
Verify removal of NEXT_PUBLIC_OPENINT_TOKEN is intentional and that its removal doesn't break token usage. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. apps/connect-dev/app/globals.css:1
- Draft comment:
Removal of all global Tailwind and custom CSS may impact styling; confirm new styling strategy is in place. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. apps/connect-dev/app/layout.tsx:1
- Draft comment:
Deleting the RootLayout component may break app structure; ensure an alternative layout is provided. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. apps/connect-dev/app/page.tsx:1
- Draft comment:
Removal of the page component eliminates the home page; confirm this is an intentional change. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. apps/connect-dev/next-env.d.ts:1
- Draft comment:
Removal of next-env.d.ts might affect TypeScript type resolution; verify if it's no longer needed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. apps/connect-dev/next.config.mjs:1
- Draft comment:
The deletion of next.config.mjs, including SVG loader config and merging logic, could impact webpack configurations. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. apps/connect-dev/package.json:1
- Draft comment:
Deleting package.json removes build and dependency configuration; confirm the connect-dev app is being deprecated or restructured. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. apps/connect-dev/.env.example:1
- Draft comment:
Removed NEXT_PUBLIC_OPENINT_TOKEN from .env.example. Confirm no dependencies remain. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. apps/connect-dev/app/globals.css:1
- Draft comment:
globals.css (including Tailwind base, components, and custom utilities) was removed. Ensure new styling is provided. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. apps/connect-dev/app/layout.tsx:1
- Draft comment:
RootLayout component was removed. Verify that layout handling has been properly migrated. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. apps/connect-dev/app/page.tsx:1
- Draft comment:
The main page (including the OpenIntConnectEmbed integration) was deleted; ensure a new entry point is provided. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. apps/connect-dev/next-env.d.ts:1
- Draft comment:
next-env.d.ts was removed. Confirm that Next.js now auto-generates necessary TypeScript definitions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. apps/connect-dev/next.config.mjs:1
- Draft comment:
Custom next.config.mjs (including webpack SVG rule and experimental settings) was removed. Verify migration of these configurations. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. apps/connect-dev/package.json:1
- Draft comment:
package.json was deleted. Ensure that dependency management and scripts are maintained elsewhere. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_YuATjDN0BvX3NTNK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 845f18b in 2 minutes and 19 seconds
More details
- Looked at
38lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. apps/web/next-env.d.ts:5
- Draft comment:
Verify the updated URL is correct for TypeScript config docs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages/ui/components/CalendarBooking.tsx:1
- Draft comment:
Warning: The import for getCalApi is commented out but is used later in useEffect (lines ~25-38). This will cause a runtime error. Please either re-enable the import or update the useEffect. - Reason this comment was not posted:
Marked as duplicate.
3. apps/web/next-env.d.ts:5
- Draft comment:
The reference URL has been updated. Verify that the new URL (https://nextjs.org/docs/app/api-reference/config/typescript) is the intended one. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_2tewmyeiywduTPO5
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -1,4 +1,4 @@ | |||
| import Cal, {getCalApi} from '@calcom/embed-react' | |||
| // import Cal, {getCalApi} from '@calcom/embed-react' | |||
There was a problem hiding this comment.
Commenting out the import of getCalApi causes its usage in useEffect to be undefined.
Suggested change
| // import Cal, {getCalApi} from '@calcom/embed-react' | |
| import Cal, {getCalApi} from '@calcom/embed-react' |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4c28dbb in 43 seconds
More details
- Looked at
13lines of code in1files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. apps/web/package.json:31
- Draft comment:
Verify that the migration from Sentry v8 to v9 is complete. Check breaking changes & documentation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify the migration and check documentation, which is against the rules as it asks for confirmation and verification.
2. apps/web/package.json:31
- Draft comment:
Upgrade @sentry/nextjs to v9.5.0—verify config compatibility and review breaking changes per the release notes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is related to a dependency change, specifically upgrading a library version. The comment suggests verifying config compatibility and reviewing breaking changes, which falls under the category of asking the PR author to double-check things. According to the rules, comments on dependency changes or asking for verification are not allowed.
Workflow ID: wflow_yC2A2ES5VkuYUoMO
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c07cd96 in 34 seconds
More details
- Looked at
13lines of code in1files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. packages/ui/package.json:15
- Draft comment:
Upgrade @calcom/embed-react to ^1.5.2 looks fine. Ensure tests pass with new version. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages/ui/package.json:15
- Draft comment:
Minor bump of '@calcom/embed-react' from ^1.5.1 to ^1.5.2 looks safe. Ensure that any updated bug fixes don't affect existing behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_h3vQbNpYJgg1th2a
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c6285cd in 1 minute and 22 seconds
More details
- Looked at
43lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. packages/ui/components/CalendarBooking.tsx:1
- Draft comment:
Unused import: getCalApi is imported but not used. Remove it to clean up the code. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/ui/components/CalendarBooking.tsx:1
- Draft comment:
Uncommented import for Cal and getCalApi. Verify if getCalApi is still necessary given that is now active. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. packages/ui/components/CalendarBooking.tsx:45
- Draft comment:
JSX formatting improvement: onClick attribute is now inline. This is cosmetic but confirm consistency with team style. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. packages/ui/components/CalendarBooking.tsx:57
- Draft comment:
Uncommented the component. Ensure this change does not result in duplicate initialization logic with the useEffect block calling getCalApi. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_9TME1k67ub5oGyWh
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
snrondina
pushed a commit
that referenced
this pull request
Mar 11, 2025
* next.js 15 upgrade * Fix type errors etc. * Removing next.js deps from previous file picker * Skip next/image broken in next 15 * Resolve next-codemod issues * Remove v0-edge * fix file picker reference * Removing separate connect-dev app * [TEMP] Calendar booking crash * Upgrade sentry * upgrade version see if it helps * Add missing
openint-bot
added a commit
that referenced
this pull request
Mar 11, 2025
* next.js 15 upgrade * Fix type errors etc. * Removing next.js deps from previous file picker * Skip next/image broken in next 15 * Resolve next-codemod issues * Remove v0-edge * fix file picker reference * Removing separate connect-dev app * [TEMP] Calendar booking crash * Upgrade sentry * upgrade version see if it helps * Add missing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Upgrade to Next.js 15 with necessary code and dependency adjustments across multiple files.
package.jsonforapps/webandkits/file-picker.reactandreact-domto version 19.0.0 inpackage.jsonforapps/webandkits/file-picker.next.config.jsto include new paths and configurations for Next.js 15.ConnectCallback,PortalPage, andOrgLayoutfunctions to handlesearchParamsas promises inapps/web.useConstantanduseWhyDidYouUpdatehooks to initializeuseRefwithundefinedinpackages/ui/hooks.Image.stories.tsxdue to issues with Next.js 15 upgrade inpackages/ui/stories/examples.@sentry/nextjsto version 9.5.0 inapps/web/package.json.@calcom/embed-reactto version 1.5.2 inpackages/ui/package.json.apps/connect-devandkits/file-picker.Breadcrumb.tsxfrompackages/ui/shadcn.next-env.d.tsforapps/web.pnpmoverrides for@types/reactand@types/react-dominpackage.json.This description was created by
for c6285cd. It will automatically update as commits are pushed.