Conversation
Removed dark mode styles and adjusted task list styles.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds PWA support (config, manifest, document meta), introduces CSS rules for display-mode: standalone, and a tiny sidebar comment plus file newline; no exported API signatures changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/sidebar.tsx (1)
120-132:⚠️ Potential issue | 🟡 MinorAdd error handling for logout.
The logout function doesn't handle API failures. If the logout request fails, the user is still redirected to
/loginwith potentially stale session state.🔧 Add error handling
const logout = async () => { - await axios.post("/api/auth/logout"); - setLogin({ - userId: 1, - username: "", - displayname: "", - canMakeWorkspace: false, - thumbnail: "", - workspaces: [], - isOwner: false, - }); - router.push("/login"); + try { + await axios.post("/api/auth/logout"); + } catch (error) { + console.error("Logout failed:", error); + } finally { + setLogin({ + userId: 1, + username: "", + displayname: "", + canMakeWorkspace: false, + thumbnail: "", + workspaces: [], + isOwner: false, + }); + router.push("/login"); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/sidebar.tsx` around lines 120 - 132, The logout function currently calls axios.post("/api/auth/logout") and immediately clears local state via setLogin and router.push("/login") even if the API fails; wrap the API call in a try/catch inside logout, only call setLogin(...) and router.push("/login") after a successful response from axios.post("/api/auth/logout"), and in the catch log the error and surface a user-facing error (e.g., via console.error or a toast) so stale session state isn't wiped or the user redirected on failure; update the logout function and related flow accordingly.
🧹 Nitpick comments (4)
next.config.js (1)
2-7: Consider potential manifest conflict.
next-pwamay auto-generate amanifest.jsonat build time, potentially overwriting the manually authoredpublic/manifest.json. To preserve your custom manifest, addpublicExcludes: ['!manifest.json']to the config, or verify the generated manifest matches your requirements.🔧 Prevent manifest overwrite
const withPWA = require('next-pwa')({ dest: 'public', register: true, skipWaiting: true, disable: process.env.NODE_ENV === 'development', + publicExcludes: ['!manifest.json'], });Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@next.config.js` around lines 2 - 7, The PWA config created in the withPWA initialization may overwrite your handwritten public/manifest.json; update the next-pwa options used when constructing withPWA by adding a publicExcludes entry to preserve the file (e.g., include "publicExcludes: ['!manifest.json']") or otherwise ensure the generated manifest matches your custom manifest, keeping this change in the same object passed to require('next-pwa') that initializes withPWA.components/sidebar.tsx (1)
279-279: Consider extracting duplicated active route logic.The
isActivecalculation usingpage.href.replace("[id]", workspace.groupId.toString())appears three times (lines 279, 405, 501). Consider extracting this to a helper function for maintainability.♻️ Extract helper function
// Add near the top of the component or as a utility const getResolvedHref = (href: string) => href.replace("[id]", workspace.groupId.toString()); const isActivePage = (pageHref: string) => router.asPath === getResolvedHref(pageHref);Note: Currently no
hrefcontains"[id]"(they all use template literals withworkspace.groupId), so thereplacecall is effectively a no-op. Consider removing it if not needed.Also applies to: 405-405, 501-501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/sidebar.tsx` at line 279, The duplicated active-route logic (the isActive calculation that compares router.asPath to page.href.replace("[id]", workspace.groupId.toString())) should be extracted into a small helper to improve maintainability: add a getResolvedHref(href: string) that substitutes "[id]" with workspace.groupId.toString() and an isActivePage(pageHref: string) that returns router.asPath === getResolvedHref(pageHref), then replace the three inline calculations with isActivePage(page.href); also verify whether any hrefs actually contain "[id]"—if not, simplify getResolvedHref to return the original href (or remove the replace call) to avoid a no-op.public/manifest.json (1)
9-21: Addpurpose: "any"to at least one icon for broader compatibility.The 192x192 icon is marked as
maskableonly, but maskable icons may be cropped on some platforms. Best practice is to include at least one icon withpurpose: "any"(or both"any maskable") to ensure proper display across all devices.🔧 Suggested fix
"icons": [ { "src": "/icons/icon-192x192.png", "sizes": "192x192", "type": "image/png", - "purpose": "maskable" + "purpose": "any" }, { "src": "/icons/icon-512x512.png", "sizes": "512x512", - "type": "image/png" + "type": "image/png", + "purpose": "any maskable" } ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/manifest.json` around lines 9 - 21, The icons array currently only marks the 192x192 icon as purpose "maskable", which can cause cropping on some platforms; update one or both icon objects (e.g., the entries with "src": "/icons/icon-192x192.png" and/or "/icons/icon-512x512.png") to include purpose "any" or "any maskable" so at least one icon has purpose "any" for broader compatibility; simply add or replace the purpose value on the chosen icon object(s) in the manifest.styles/globals.scss (1)
62-71: Potential z-index conflict with bottom navigation.The CSS sets
z-index: 99999onnav.fixed.bottom-0, but incomponents/sidebar.tsx(line 399), the nav element usesz-[99990]. The CSS override takes precedence, but consider aligning these values for consistency and maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/globals.scss` around lines 62 - 71, The nav.fixed.bottom-0 rule sets z-index: 99999 which conflicts with the nav in components/sidebar.tsx that uses the Tailwind class z-[99990]; update one to match the other for consistency — either change the stylesheet's z-index in nav.fixed.bottom-0 to 99990 or change the Tailwind class in the Sidebar nav to z-[99999], or better yet centralize the value using a shared CSS variable or a single source (e.g., prefer the Tailwind class and remove/avoid the high-specificity override in globals.scss) so both the global rule and the Sidebar nav reference the same z-index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@next.config.js`:
- Around line 2-7: Remove the withPWA setup that requires next-pwa (the const
withPWA = require('next-pwa')({...}) block) and replace it with a PWA solution
compatible with Next.js 16/Turbopack: either integrate `@serwist/next` by
installing and initializing its plugin in next.config.js, or remove the plugin
entirely and add a Next.js-native app/manifest.ts and related meta in your app
layout; ensure you delete the require('next-pwa') call and any usage of withPWA
so next.config.js exports the plain config or the `@serwist/next-enhanced` config
instead.
In `@pages/_document.tsx`:
- Around line 7-15: Add a <link rel="manifest" href="/manifest.json" /> element
to the Head output in the custom Document render (the Head block in
pages/_document.tsx) alongside the existing PWA meta tags so browsers can
discover the app manifest; place it near the apple-mobile-web-app and viewport
meta tags and ensure the href points to the actual manifest file path.
In `@styles/globals.scss`:
- Around line 43-50: The current media rule hides any element using utility
classes `.lg\:flex` and `.w-64` as well as any element matching
`[class*="Sidebar"]` and `aside`, which is too broad; narrow the rule to only
target the sidebar container by replacing the global selectors with a specific
sidebar class (e.g., `.sidebar`, `.AppSidebar`, or whatever unique class wraps
the sidebar) and apply the utility-class combinators scoped to that container
(for example target `.sidebar .lg\:flex` and `.sidebar .w-64` and/or
`.sidebar[class*="Sidebar"]`) so only sidebar elements are hidden in
`display-mode: standalone`.
---
Outside diff comments:
In `@components/sidebar.tsx`:
- Around line 120-132: The logout function currently calls
axios.post("/api/auth/logout") and immediately clears local state via setLogin
and router.push("/login") even if the API fails; wrap the API call in a
try/catch inside logout, only call setLogin(...) and router.push("/login") after
a successful response from axios.post("/api/auth/logout"), and in the catch log
the error and surface a user-facing error (e.g., via console.error or a toast)
so stale session state isn't wiped or the user redirected on failure; update the
logout function and related flow accordingly.
---
Nitpick comments:
In `@components/sidebar.tsx`:
- Line 279: The duplicated active-route logic (the isActive calculation that
compares router.asPath to page.href.replace("[id]",
workspace.groupId.toString())) should be extracted into a small helper to
improve maintainability: add a getResolvedHref(href: string) that substitutes
"[id]" with workspace.groupId.toString() and an isActivePage(pageHref: string)
that returns router.asPath === getResolvedHref(pageHref), then replace the three
inline calculations with isActivePage(page.href); also verify whether any hrefs
actually contain "[id]"—if not, simplify getResolvedHref to return the original
href (or remove the replace call) to avoid a no-op.
In `@next.config.js`:
- Around line 2-7: The PWA config created in the withPWA initialization may
overwrite your handwritten public/manifest.json; update the next-pwa options
used when constructing withPWA by adding a publicExcludes entry to preserve the
file (e.g., include "publicExcludes: ['!manifest.json']") or otherwise ensure
the generated manifest matches your custom manifest, keeping this change in the
same object passed to require('next-pwa') that initializes withPWA.
In `@public/manifest.json`:
- Around line 9-21: The icons array currently only marks the 192x192 icon as
purpose "maskable", which can cause cropping on some platforms; update one or
both icon objects (e.g., the entries with "src": "/icons/icon-192x192.png"
and/or "/icons/icon-512x512.png") to include purpose "any" or "any maskable" so
at least one icon has purpose "any" for broader compatibility; simply add or
replace the purpose value on the chosen icon object(s) in the manifest.
In `@styles/globals.scss`:
- Around line 62-71: The nav.fixed.bottom-0 rule sets z-index: 99999 which
conflicts with the nav in components/sidebar.tsx that uses the Tailwind class
z-[99990]; update one to match the other for consistency — either change the
stylesheet's z-index in nav.fixed.bottom-0 to 99990 or change the Tailwind class
in the Sidebar nav to z-[99999], or better yet centralize the value using a
shared CSS variable or a single source (e.g., prefer the Tailwind class and
remove/avoid the high-specificity override in globals.scss) so both the global
rule and the Sidebar nav reference the same z-index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d697013-6bf4-4b5d-952c-1b9cd38e4627
📒 Files selected for processing (5)
components/sidebar.tsxnext.config.jspages/_document.tsxpublic/manifest.jsonstyles/globals.scss
| const withPWA = require('next-pwa')({ | ||
| dest: 'public', | ||
| register: true, | ||
| skipWaiting: true, | ||
| disable: process.env.NODE_ENV === 'development', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is next-pwa compatible with Next.js 16 and Turbopack?
💡 Result:
No, next-pwa is not compatible with Next.js 16 and Turbopack. next-pwa (both the original shadowwalker/next-pwa v5.6.0 last release Aug 2022, and the maintained fork @ducanh2912/next-pwa v10.2.9 Sep 2024) relies on webpack plugins like workbox-webpack-plugin. Its peer dependency explicitly requires webpack >=5.9.0. Next.js 16 uses Turbopack as the default bundler, which does not support webpack plugins. Users must use --webpack flag for builds to make next-pwa work, but this disables Turbopack compatibility. Evidence includes unmaintained status concerns (GitHub issue #508), build errors with Turbopack/Turbo (issues #419, #5199, #512), and recommendations for alternatives like @serwist/next which is designed for Next.js 16 (LogRocket blog, Next.js PWA guides). Next.js has built-in PWA support via app/manifest.ts, but for advanced service workers, Serwist is preferred over next-pwa.
Citations:
- 1: Is this package still developed? shadowwalker/next-pwa#508
- 2: https://www.npmjs.com/package/next-pwa?activeTab=versions
- 3: https://blog.logrocket.com/nextjs-16-pwa-offline-support/
- 4: Support Next.js 13 and turbopack shadowwalker/next-pwa#419
- 5: next-pwa Build error with turbo vercel/turborepo#5199
- 6: https://github.com/shadowwalker/next-pwa
- 7: https://turbo.build/pack/docs/migrating-from-webpack
- 8: https://www.npmjs.com/package/@ducanh2912/next-pwa
Remove next-pwa and replace with alternative PWA solution—incompatible with Next.js 16 and Turbopack.
next-pwa relies on webpack plugins that Turbopack does not support. Both the original package (v5.6.0, unmaintained since Aug 2022) and the maintained fork @ducanh2912/next-pwa have the same incompatibility. Using --webpack flag as a workaround disables Turbopack entirely, defeating its performance benefits. Replace with @serwist/next (actively maintained, designed for Next.js 16) or leverage Next.js's built-in PWA support via app/manifest.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@next.config.js` around lines 2 - 7, Remove the withPWA setup that requires
next-pwa (the const withPWA = require('next-pwa')({...}) block) and replace it
with a PWA solution compatible with Next.js 16/Turbopack: either integrate
`@serwist/next` by installing and initializing its plugin in next.config.js, or
remove the plugin entirely and add a Next.js-native app/manifest.ts and related
meta in your app layout; ensure you delete the require('next-pwa') call and any
usage of withPWA so next.config.js exports the plain config or the
`@serwist/next-enhanced` config instead.
| {/* PWA / iOS Standalone Mode Tags */} | ||
| <meta name="apple-mobile-web-app-capable" content="yes" /> | ||
| <meta name="apple-mobile-web-app-status-bar-style" content="black-translucent" /> | ||
| <meta name="apple-mobile-web-app-title" content="Orbit" /> | ||
| <meta name="mobile-web-app-capable" content="yes" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1, viewport-fit=cover" /> | ||
|
|
||
| {/* Favicon / Touch Icon (Crucial for iOS to treat it as an app) */} | ||
| <link rel="apple-touch-icon" href="/favicon.png" /> |
There was a problem hiding this comment.
Add manifest link for PWA installation.
The manifest.json file exists but is not linked in the document head. Without the link tag, browsers won't recognize the app as installable.
🔧 Add manifest link
{/* PWA / iOS Standalone Mode Tags */}
<meta name="apple-mobile-web-app-capable" content="yes" />
<meta name="apple-mobile-web-app-status-bar-style" content="black-translucent" />
<meta name="apple-mobile-web-app-title" content="Orbit" />
<meta name="mobile-web-app-capable" content="yes" />
- <meta name="viewport" content="width=device-width, initial-scale=1, viewport-fit=cover" />
{/* Favicon / Touch Icon (Crucial for iOS to treat it as an app) */}
<link rel="apple-touch-icon" href="/favicon.png" />
+ <link rel="manifest" href="/manifest.json" />📝 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.
| {/* PWA / iOS Standalone Mode Tags */} | |
| <meta name="apple-mobile-web-app-capable" content="yes" /> | |
| <meta name="apple-mobile-web-app-status-bar-style" content="black-translucent" /> | |
| <meta name="apple-mobile-web-app-title" content="Orbit" /> | |
| <meta name="mobile-web-app-capable" content="yes" /> | |
| <meta name="viewport" content="width=device-width, initial-scale=1, viewport-fit=cover" /> | |
| {/* Favicon / Touch Icon (Crucial for iOS to treat it as an app) */} | |
| <link rel="apple-touch-icon" href="/favicon.png" /> | |
| {/* PWA / iOS Standalone Mode Tags */} | |
| <meta name="apple-mobile-web-app-capable" content="yes" /> | |
| <meta name="apple-mobile-web-app-status-bar-style" content="black-translucent" /> | |
| <meta name="apple-mobile-web-app-title" content="Orbit" /> | |
| <meta name="mobile-web-app-capable" content="yes" /> | |
| <meta name="viewport" content="width=device-width, initial-scale=1, viewport-fit=cover" /> | |
| {/* Favicon / Touch Icon (Crucial for iOS to treat it as an app) */} | |
| <link rel="apple-touch-icon" href="/favicon.png" /> | |
| <link rel="manifest" href="/manifest.json" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pages/_document.tsx` around lines 7 - 15, Add a <link rel="manifest"
href="/manifest.json" /> element to the Head output in the custom Document
render (the Head block in pages/_document.tsx) alongside the existing PWA meta
tags so browsers can discover the app manifest; place it near the
apple-mobile-web-app and viewport meta tags and ensure the href points to the
actual manifest file path.
| @media all and (display-mode: standalone) { | ||
| aside, | ||
| .lg\:flex, | ||
| .w-64, | ||
| [class*="Sidebar"] { | ||
| display: none !important; | ||
| width: 0 !important; | ||
| } |
There was a problem hiding this comment.
Overly broad selectors may hide unintended elements.
Hiding .lg\:flex and .w-64 globally in standalone mode will affect any element using these utility classes, not just sidebar-related ones. This could break layouts elsewhere in the app.
🔧 Use more specific selectors
`@media` all and (display-mode: standalone) {
- aside,
- .lg\:flex,
- .w-64,
- [class*="Sidebar"] {
+ aside[class*="sidebar"],
+ [class*="Sidebar"],
+ .sidebar-container {
display: none !important;
width: 0 !important;
}Alternatively, add a specific class to the sidebar container and target only that class.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/globals.scss` around lines 43 - 50, The current media rule hides any
element using utility classes `.lg\:flex` and `.w-64` as well as any element
matching `[class*="Sidebar"]` and `aside`, which is too broad; narrow the rule
to only target the sidebar container by replacing the global selectors with a
specific sidebar class (e.g., `.sidebar`, `.AppSidebar`, or whatever unique
class wraps the sidebar) and apply the utility-class combinators scoped to that
container (for example target `.sidebar .lg\:flex` and `.sidebar .w-64` and/or
`.sidebar[class*="Sidebar"]`) so only sidebar elements are hidden in
`display-mode: standalone`.
3 participants
Added PWA for iOS and made it so the new phone UI shows, as my last PR which was closed by me did not support it.
What is a PWA?
A Progressive Web App is just a way to let users "install" Orbit like a native app.
When they tap "Add to Home Screen":
Summary by CodeRabbit
New Features
UI Improvements