Closed
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughProgressive Web App (PWA) integration is configured using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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: Add next-pwa to package.json dependencies and update
next.config.js so the PWA plugin can run under webpack: install "next-pwa" and
then keep using withPWA(nextConfig) but ensure your Next build uses webpack
rather than Turbopack by removing the deprecated experimental.turbopack block
and instead set the top-level turbopack configuration to false (or otherwise
disable Turbopack) in nextConfig so workbox-webpack-plugin can run; finally
verify module.exports = withPWA(nextConfig) remains and that withPWA is
imported/required correctly.
In `@public/manifest.json`:
- Around line 11-19: The manifest.json references missing icon assets at the
"src" fields "/icons/icon-192x192.png" and "/icons/icon-512x512.png"; either add
the two PNG files into the public/icons/ directory with those exact names or
update the "src" values in manifest.json to point to the actual existing asset
locations (e.g., /assets/icons/... or public/...). Make the change in the
manifest.json entries for the 192x192 and 512x512 icons so install surfaces stop
requesting non-existent files.
- Line 6: The manifest currently sets the display mode to "standalone" which
hides mobile browser chrome; to preserve browser controls update the "display"
key in public/manifest.json from "standalone" to either "browser" or
"minimal-ui" (choose "browser" to keep full browser UI or "minimal-ui" for
limited UI) so the installed PWA will retain phone/browser controls; ensure the
manifest remains valid JSON after the change.
- Around line 1-22: pages/_document.tsx is missing the HTML link to the web app
manifest so the browser can't discover public/manifest.json; open
pages/_document.tsx (the custom Document component, e.g., class MyDocument
extends Document or the default export Document) and add a <link rel="manifest"
href="/manifest.json"> inside the <Head> element (ensure Head is imported from
'next/document' and placed in the render or getInitialProps output) so the
manifest in public/manifest.json becomes discoverable by browsers and next-pwa.
🪄 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: c76999f6-4ba5-4217-911f-121b11bfb76f
📒 Files selected for processing (2)
next.config.jspublic/manifest.json
Comment on lines
+2
to
+7
| const withPWA = require('next-pwa')({ | ||
| dest: 'public', | ||
| register: true, | ||
| skipWaiting: true, | ||
| disable: process.env.NODE_ENV === 'development', | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, pathlib, re
pkg = pathlib.Path("package.json")
data = json.loads(pkg.read_text())
deps = {**data.get("dependencies", {}), **data.get("devDependencies", {})}
next_ver = deps.get("next")
pwa_ver = deps.get("next-pwa")
print("next =", next_ver)
print("next-pwa =", pwa_ver)
print("dev script =", data.get("scripts", {}).get("dev"))
print("build script =", data.get("scripts", {}).get("build"))
m = re.search(r'(\d+)', next_ver or "")
print("next major =", m.group(1) if m else "unknown")
PY
rg -n -C2 "withPWA|next-pwa|experimental\\.turbopack|experimental\\.turbo|\\bturbopack\\b" next.config.js package.jsonRepository: PlanetaryOrbit/orbit
Length of output: 668
Add next-pwa to dependencies and resolve Turbopack incompatibility before merging.
next-pwa is not in your package.json—line 2 will fail with "Cannot find module" at runtime. Once added, there are two additional blockers:
next-pwadepends onworkbox-webpack-plugin, but Next.js 16+ uses Turbopack by default fornext build/next dev, and Turbopack does not support webpack plugins. The service worker will not be generated unless you opt back into webpack.- Line 13 uses the deprecated
experimental.turbopackconfig location. Next.js 16+ moved this to top-levelturbopack.
Applies to: 2–7, 12–14, 62
Current state at these lines:
const withPWA = require('next-pwa')({
dest: 'public',
register: true,
skipWaiting: true,
disable: process.env.NODE_ENV === 'development',
});
experimental: {
turbopack: {},
},
module.exports = withPWA(nextConfig);
🤖 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, Add next-pwa to package.json dependencies
and update next.config.js so the PWA plugin can run under webpack: install
"next-pwa" and then keep using withPWA(nextConfig) but ensure your Next build
uses webpack rather than Turbopack by removing the deprecated
experimental.turbopack block and instead set the top-level turbopack
configuration to false (or otherwise disable Turbopack) in nextConfig so
workbox-webpack-plugin can run; finally verify module.exports =
withPWA(nextConfig) remains and that withPWA is imported/required correctly.
Comment on lines
+1
to
+22
| { | ||
| "name": "Orbit Dashboard", | ||
| "short_name": "Orbit", | ||
| "description": "The ultimate management platform for gaming communities.", | ||
| "start_url": "/", | ||
| "display": "standalone", | ||
| "background_color": "#000000", | ||
| "theme_color": "#5865f2", | ||
| "icons": [ | ||
| { | ||
| "src": "/icons/icon-192x192.png", | ||
| "sizes": "192x192", | ||
| "type": "image/png", | ||
| "purpose": "maskable" | ||
| }, | ||
| { | ||
| "src": "/icons/icon-512x512.png", | ||
| "sizes": "512x512", | ||
| "type": "image/png" | ||
| } | ||
| ] | ||
| } |
Contributor
There was a problem hiding this comment.
Link the manifest from pages/_document.tsx before shipping this.
pages/_document.tsx still lacks <link rel="manifest" href="/manifest.json">, so this file is not currently discoverable from the document and the install metadata here is effectively unused. Browsers discover web app manifests through that link, and next-pwa's own setup docs require adding it in <Head>. (developer.mozilla.org)
🔧 Minimal fix
<Head>
+ <link rel="manifest" href="/manifest.json" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/manifest.json` around lines 1 - 22, pages/_document.tsx is missing the
HTML link to the web app manifest so the browser can't discover
public/manifest.json; open pages/_document.tsx (the custom Document component,
e.g., class MyDocument extends Document or the default export Document) and add
a <link rel="manifest" href="/manifest.json"> inside the <Head> element (ensure
Head is imported from 'next/document' and placed in the render or
getInitialProps output) so the manifest in public/manifest.json becomes
discoverable by browsers and next-pwa.
| "short_name": "Orbit", | ||
| "description": "The ultimate management platform for gaming communities.", | ||
| "start_url": "/", | ||
| "display": "standalone", |
Contributor
There was a problem hiding this comment.
Line 6 will remove the mobile browser chrome once this manifest is wired in.
display: "standalone" launches an installed PWA without the normal navigation UI. If the goal is to keep the phone/browser controls, switch this to browser or minimal-ui instead. (developer.mozilla.org)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/manifest.json` at line 6, The manifest currently sets the display mode
to "standalone" which hides mobile browser chrome; to preserve browser controls
update the "display" key in public/manifest.json from "standalone" to either
"browser" or "minimal-ui" (choose "browser" to keep full browser UI or
"minimal-ui" for limited UI) so the installed PWA will retain phone/browser
controls; ensure the manifest remains valid JSON after the change.
Comment on lines
+11
to
+19
| "src": "/icons/icon-192x192.png", | ||
| "sizes": "192x192", | ||
| "type": "image/png", | ||
| "purpose": "maskable" | ||
| }, | ||
| { | ||
| "src": "/icons/icon-512x512.png", | ||
| "sizes": "512x512", | ||
| "type": "image/png" |
Contributor
There was a problem hiding this comment.
These icon paths currently point at missing assets.
The manifest references /icons/icon-192x192.png and /icons/icon-512x512.png, but the provided repo context says public/icons/ does not exist. Once the manifest is linked, install surfaces will request missing files until those PNGs are added or the paths are updated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/manifest.json` around lines 11 - 19, The manifest.json references
missing icon assets at the "src" fields "/icons/icon-192x192.png" and
"/icons/icon-512x512.png"; either add the two PNG files into the public/icons/
directory with those exact names or update the "src" values in manifest.json to
point to the actual existing asset locations (e.g., /assets/icons/... or
public/...). Make the change in the manifest.json entries for the 192x192 and
512x512 icons so install surfaces stop requesting non-existent files.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
styles/globals.scss (1)
126-127: Bottom padding should include nav height, not only safe-area inset.Line 127 adds only safe-area padding. If standalone forces bottom nav on larger breakpoints (
maincurrently haslg:pb-0), content can still sit under the fixed bar.Proposed padding fix
body { - padding-bottom: env(safe-area-inset-bottom, 20px) !important; + padding-bottom: calc(4rem + env(safe-area-inset-bottom, 20px)) !important; /* h-16 nav + safe area */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/globals.scss` around lines 126 - 127, The body padding-bottom only uses the safe-area inset which still allows content to sit under a fixed bottom nav; update the CSS to add the nav height to the safe-area inset (e.g., change the body rule that currently sets padding-bottom: env(safe-area-inset-bottom, 20px) !important to use a calc combining the safe-area inset and the bottom nav height such as calc(env(safe-area-inset-bottom, 20px) + var(--bottom-nav-height, 56px))) and ensure the bottom nav height variable is defined (or use the known nav height constant) and/or remove/adjust the conflicting rule that sets main to lg:pb-0 so large breakpoints still reserve space for the fixed nav.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@styles/globals.scss`:
- Around line 108-123: The CSS hides the sidebar but the mobile nav selectors
don't match the actual DOM (the nav in components/sidebar.tsx uses
className="flex-1 space-y-0.5 mt-2" and the sidebar wrapper relies on responsive
classes like "fixed lg:static" with translate-x), so update either the markup or
the CSS: add a stable class/attribute (e.g., "mobile-nav" or data-mobile-nav) to
the actual nav element in components/sidebar.tsx (near the element with
className="flex-1 space-y-0.5 mt-2") or change the selectors to target the
sidebar wrapper's responsive classes/transform behavior (the wrapper using
"fixed lg:static" and translate-x) and use responsive rules to show the
bottom/mobile navigation; ensure selectors align with the wrapper/nav class
names so mobile layout toggles reliably.
---
Nitpick comments:
In `@styles/globals.scss`:
- Around line 126-127: The body padding-bottom only uses the safe-area inset
which still allows content to sit under a fixed bottom nav; update the CSS to
add the nav height to the safe-area inset (e.g., change the body rule that
currently sets padding-bottom: env(safe-area-inset-bottom, 20px) !important to
use a calc combining the safe-area inset and the bottom nav height such as
calc(env(safe-area-inset-bottom, 20px) + var(--bottom-nav-height, 56px))) and
ensure the bottom nav height variable is defined (or use the known nav height
constant) and/or remove/adjust the conflicting rule that sets main to lg:pb-0 so
large breakpoints still reserve space for the fixed nav.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Removed dark mode styles and adjusted task list styles.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/sidebar.tsx`:
- Around line 205-232: The collapsed sidebar drops visible text for controls
like the workspace switcher Listbox.Button (uses
workspace.groupName/groupThumbnail and IconChevronDown) when isCollapsed is
true, so add accessible names: when isCollapsed is true provide an aria-label
(or aria-labelledby) on the Listbox.Button describing the control (e.g.,
"Workspace switcher: {workspace.groupName}"), and apply the same pattern for the
collapsed page buttons and account menu components referenced elsewhere (the
collapsed icon-only controls at the other blocks). Ensure the aria-label updates
to include the current workspace or account name and is only added when
isCollapsed is true so screen readers get a meaningful name for the icon-only
buttons.
- Around line 83-87: The More sheet currently is just layered divs and loses
modal semantics: update the component in components/sidebar.tsx to behave like a
real modal by adding role="dialog" and aria-modal="true" on the sheet container,
implement focus management (trap focus into the sheet and restore focus on
close) and handle Escape to close the sheet, and ensure the body scroll lock
useEffect (currently using isMobileMenuOpen) is tied to the sheet's open state
(e.g., the More sheet's open boolean) so background scrolling stops while the
sheet is open and is cleaned up on unmount; modify the event handlers and the
useEffect cleanup accordingly (look for isMobileMenuOpen and the More sheet
render logic to update).
- Around line 58-69: The PWA detection currently sets isStandalone for any
installed PWA (including desktop), which forces mobile layout UI
(mobileMoreOpen/mobileMoreVisible and mobile bottom bar) even on desktop PWAs;
change the detection in the useEffect(s) that call setIsStandalone so they only
mark isStandalone true for mobile PWAs (e.g. require
window.matchMedia("(display-mode: standalone)").matches AND a mobile check such
as a mobile user-agent test like /Mobi|Android/i.test(navigator.userAgent) or a
small screen width check), and apply the same change to the other instances
mentioned so desktop-installed PWAs keep the desktop sidebar and don’t fall back
to mobile-only pieces hidden by lg:hidden. Ensure you update the logic that
toggles mobileMoreOpen/mobileMoreVisible to rely on the revised isStandalone
value.
- Around line 140-169: The three useEffect hooks in the sidebar keep previous
workspace flags/counts if the next workspace's fetch is slow or fails; update
each effect (the configuration fetch that sets
setDocsEnabled/setAlliesEnabled/setSessionsEnabled/setNoticesEnabled/setPoliciesEnabled,
the policies pending count effect that sets setPendingPolicyCount, and the
notices pending count effect that sets setPendingNoticesCount) to immediately
reset the related state to safe defaults at the start of the effect, and use an
AbortController to cancel the in-flight fetch on cleanup so stale responses
cannot overwrite state; also guard the fetch response handlers to ignore results
when aborted and ensure .catch handlers set the defaults (false or 0) when
errors occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+58
to
+69
| const [mobileMoreOpen, setMobileMoreOpen] = useState(false); | ||
| const [mobileMoreVisible, setMobileMoreVisible] = useState(false); | ||
| const [isStandalone, setIsStandalone] = useState(false); // PWA Check | ||
| const workspaceListboxWrapperRef = useRef<HTMLDivElement>(null); | ||
| const router = useRouter() | ||
| const router = useRouter(); | ||
|
|
||
| // Detect PWA Mode | ||
| useEffect(() => { | ||
| if (isMobileMenuOpen) { | ||
| document.body.classList.add("overflow-hidden") | ||
| } else { | ||
| document.body.classList.remove("overflow-hidden") | ||
| } | ||
| return () => { | ||
| document.body.classList.remove("overflow-hidden") | ||
| if (typeof window !== "undefined" && window.matchMedia("(display-mode: standalone)").matches) { | ||
| setIsStandalone(true); | ||
| } | ||
| }, [isMobileMenuOpen]) | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Keep desktop-installed PWAs on the desktop layout.
display-mode: standalone is not mobile-only. In a desktop-installed PWA, this hides the desktop sidebar, forces the mobile bottom bar on, and still keeps the More sheet hidden behind lg:hidden, so pages after the first four become unreachable.
💡 Minimal fix
- const [isStandalone, setIsStandalone] = useState(false); // PWA Check
-
- // Detect PWA Mode
- useEffect(() => {
- if (typeof window !== "undefined" && window.matchMedia("(display-mode: standalone)").matches) {
- setIsStandalone(true);
- }
- }, []);
-
...
className={clsx(
"hidden fixed lg:static top-0 left-0 h-screen z-[99999] flex-col transition-[transform,width] duration-300 ease-out",
- !isStandalone && "lg:flex", // Only show on large screens if NOT a PWA
+ "lg:flex",
isCollapsed ? "w-[72px]" : "w-56"
)}
...
<nav className={clsx(
"fixed bottom-0 inset-x-0 z-[99990] bg-white/60 dark:bg-zinc-950/95 backdrop-blur-xl border-t border-zinc-200/50 dark:border-zinc-800/80 safe-area-bottom",
- isStandalone ? "flex" : "lg:hidden" // Always flex in PWA mode, else hidden on desktop
+ "lg:hidden"
)}>Also applies to: 173-178, 398-401, 441-455, 463-476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/sidebar.tsx` around lines 58 - 69, The PWA detection currently
sets isStandalone for any installed PWA (including desktop), which forces mobile
layout UI (mobileMoreOpen/mobileMoreVisible and mobile bottom bar) even on
desktop PWAs; change the detection in the useEffect(s) that call setIsStandalone
so they only mark isStandalone true for mobile PWAs (e.g. require
window.matchMedia("(display-mode: standalone)").matches AND a mobile check such
as a mobile user-agent test like /Mobi|Android/i.test(navigator.userAgent) or a
small screen width check), and apply the same change to the other instances
mentioned so desktop-installed PWAs keep the desktop sidebar and don’t fall back
to mobile-only pieces hidden by lg:hidden. Ensure you update the logic that
toggles mobileMoreOpen/mobileMoreVisible to rely on the revised isStandalone
value.
Comment on lines
+83
to
+87
| useEffect(() => { | ||
| if (isMobileMenuOpen) document.body.classList.add("overflow-hidden"); | ||
| else document.body.classList.remove("overflow-hidden"); | ||
| return () => document.body.classList.remove("overflow-hidden"); | ||
| }, [isMobileMenuOpen]); |
Contributor
There was a problem hiding this comment.
Restore modal behavior for the More sheet.
This drawer is now just layered divs, so focus can escape to the page, there is no dialog semantics, and Escape is ignored. The scroll lock on Line 84 is also still wired to isMobileMenuOpen, so the background keeps scrolling while the sheet is open.
💡 Minimum wiring fix
useEffect(() => {
- if (isMobileMenuOpen) document.body.classList.add("overflow-hidden");
+ if (mobileMoreOpen) document.body.classList.add("overflow-hidden");
else document.body.classList.remove("overflow-hidden");
return () => document.body.classList.remove("overflow-hidden");
-}, [isMobileMenuOpen]);
+}, [mobileMoreOpen]);The sheet still needs real modal semantics: a dialog role, aria-modal, focus management, and Escape handling.
Also applies to: 459-557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/sidebar.tsx` around lines 83 - 87, The More sheet currently is
just layered divs and loses modal semantics: update the component in
components/sidebar.tsx to behave like a real modal by adding role="dialog" and
aria-modal="true" on the sheet container, implement focus management (trap focus
into the sheet and restore focus on close) and handle Escape to close the sheet,
and ensure the body scroll lock useEffect (currently using isMobileMenuOpen) is
tied to the sheet's open state (e.g., the More sheet's open boolean) so
background scrolling stops while the sheet is open and is cleaned up on unmount;
modify the event handlers and the useEffect cleanup accordingly (look for
isMobileMenuOpen and the More sheet render logic to update).
Comment on lines
140
to
169
| useEffect(() => { | ||
| fetch(`/api/workspace/${workspace.groupId}/settings/general/configuration`) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| setDocsEnabled(data.value.guides?.enabled ?? false); | ||
| setAlliesEnabled(data.value.allies?.enabled ?? false); | ||
| setSessionsEnabled(data.value.sessions?.enabled ?? false); | ||
| setNoticesEnabled(data.value.notices?.enabled ?? false); | ||
| setPoliciesEnabled(data.value.policies?.enabled ?? false); | ||
| .then((res) => res.json()) | ||
| .then((data) => { | ||
| setDocsEnabled(data.value?.guides?.enabled ?? false); | ||
| setAlliesEnabled(data.value?.allies?.enabled ?? false); | ||
| setSessionsEnabled(data.value?.sessions?.enabled ?? false); | ||
| setNoticesEnabled(data.value?.notices?.enabled ?? false); | ||
| setPoliciesEnabled(data.value?.policies?.enabled ?? false); | ||
| }) | ||
| .catch(() => setDocsEnabled(false)); | ||
| }, [workspace.groupId]); | ||
|
|
||
| useEffect(() => { | ||
| if (policiesEnabled) { | ||
| fetch(`/api/workspace/${workspace.groupId}/policies/pending`) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| if (data.success) { | ||
| setPendingPolicyCount(data.count); | ||
| } | ||
| }) | ||
| .then((res) => res.json()) | ||
| .then((data) => data.success && setPendingPolicyCount(data.count)) | ||
| .catch(() => setPendingPolicyCount(0)); | ||
| } | ||
| }, [workspace.groupId, policiesEnabled]); | ||
|
|
||
| useEffect(() => { | ||
| if (noticesEnabled) { | ||
| if (workspace.yourPermission?.includes("approve_notices") || workspace.yourPermission?.includes("manage_notices") || workspace.yourPermission?.includes("admin")) { | ||
| fetch(`/api/workspace/${workspace.groupId}/activity/notices/count`) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| if (data.success) { | ||
| setPendingNoticesCount(data.count || 0); | ||
| } | ||
| }) | ||
| .catch(() => setPendingNoticesCount(0)); | ||
| } | ||
| if (noticesEnabled && (workspace.yourPermission?.includes("approve_notices") || workspace.yourPermission?.includes("manage_notices") || workspace.yourPermission?.includes("admin"))) { | ||
| fetch(`/api/workspace/${workspace.groupId}/activity/notices/count`) | ||
| .then((res) => res.json()) | ||
| .then((data) => data.success && setPendingNoticesCount(data.count || 0)) | ||
| .catch(() => setPendingNoticesCount(0)); | ||
| } | ||
| }, [workspace.groupId, noticesEnabled, workspace.yourPermission]); |
Contributor
There was a problem hiding this comment.
Reset and cancel workspace-scoped sidebar state.
These effects keep the previous workspace's flags and counts unless the next request fully succeeds. A slow or failed workspace switch can render the new workspace with the old workspace's pages or badges.
💡 Safer pattern
useEffect(() => {
- fetch(`/api/workspace/${workspace.groupId}/settings/general/configuration`)
- .then((res) => res.json())
+ const controller = new AbortController();
+
+ setDocsEnabled(false);
+ setAlliesEnabled(false);
+ setSessionsEnabled(false);
+ setNoticesEnabled(false);
+ setPoliciesEnabled(false);
+
+ fetch(`/api/workspace/${workspace.groupId}/settings/general/configuration`, {
+ signal: controller.signal,
+ })
+ .then((res) =>
+ res.ok ? res.json() : Promise.reject(new Error("Failed to load configuration"))
+ )
.then((data) => {
setDocsEnabled(data.value?.guides?.enabled ?? false);
setAlliesEnabled(data.value?.allies?.enabled ?? false);
setSessionsEnabled(data.value?.sessions?.enabled ?? false);
setNoticesEnabled(data.value?.notices?.enabled ?? false);
setPoliciesEnabled(data.value?.policies?.enabled ?? false);
})
- .catch(() => setDocsEnabled(false));
+ .catch(() => {
+ if (controller.signal.aborted) return;
+ setDocsEnabled(false);
+ setAlliesEnabled(false);
+ setSessionsEnabled(false);
+ setNoticesEnabled(false);
+ setPoliciesEnabled(false);
+ });
+
+ return () => controller.abort();
}, [workspace.groupId]);Apply the same reset/abort pattern to the two pending-count effects as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/sidebar.tsx` around lines 140 - 169, The three useEffect hooks in
the sidebar keep previous workspace flags/counts if the next workspace's fetch
is slow or fails; update each effect (the configuration fetch that sets
setDocsEnabled/setAlliesEnabled/setSessionsEnabled/setNoticesEnabled/setPoliciesEnabled,
the policies pending count effect that sets setPendingPolicyCount, and the
notices pending count effect that sets setPendingNoticesCount) to immediately
reset the related state to safe defaults at the start of the effect, and use an
AbortController to cancel the in-flight fetch on cleanup so stale responses
cannot overwrite state; also guard the fetch response handlers to ignore results
when aborted and ensure .catch handlers set the defaults (false or 0) when
errors occur.
Comment on lines
+205
to
+232
| <Listbox.Button | ||
| className={clsx( | ||
| "w-9 h-9 rounded-lg object-cover ring-1 ring-zinc-200/50 dark:ring-zinc-600/50 transition-all duration-300", | ||
| isCollapsed && "scale-90 opacity-80" | ||
| )} | ||
| /> | ||
| {!isCollapsed && ( | ||
| <div className="flex-1 min-w-0 text-left transition-all duration-300"> | ||
| <p className="text-sm font-semibold truncate text-zinc-800 dark:text-white max-w-full"> | ||
| {workspace.groupName} | ||
| </p> | ||
| <p className="text-xs text-zinc-500 dark:text-zinc-400 truncate max-w-full"> | ||
| Switch workspace | ||
| </p> | ||
| </div> | ||
| )} | ||
| {!isCollapsed && ( | ||
| <IconChevronDown className="w-4 h-4 text-zinc-400 dark:text-zinc-500 transition-all duration-300 flex-shrink-0" /> | ||
| )} | ||
| </Listbox.Button> | ||
|
|
||
| <Listbox.Options | ||
| className={clsx( | ||
| "absolute top-full left-0 z-50 w-full mt-2 bg-white dark:bg-zinc-800 rounded-xl shadow-lg border border-zinc-200 dark:border-zinc-600 max-h-60 overflow-y-auto py-1" | ||
| )} | ||
| > | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| workspaceListboxWrapperRef.current?.querySelector('button')?.click() | ||
| router.push('/') | ||
| }} | ||
| className={clsx( | ||
| "w-full flex items-center gap-3 px-3 py-2.5 mx-1 rounded-lg text-sm font-medium transition duration-200", | ||
| "text-zinc-600 dark:text-zinc-400 hover:bg-zinc-100 dark:hover:bg-zinc-700 hover:text-zinc-900 dark:hover:text-white" | ||
| "w-full flex items-center gap-3 rounded-2xl p-2.5 transition-colors duration-200 outline-none", | ||
| "text-zinc-700 dark:text-zinc-200 hover:bg-zinc-100/80 dark:hover:bg-zinc-800/60", | ||
| isCollapsed && "justify-center p-2" | ||
| )} | ||
| > | ||
| <IconChevronLeft className="w-4 h-4 flex-shrink-0" /> | ||
| Back to menu | ||
| </button> | ||
| <div className="my-1 border-t border-zinc-200 dark:border-zinc-600" /> | ||
| {login?.workspaces && login.workspaces.length > 1 ? ( | ||
| login.workspaces | ||
| .filter(ws => ws.groupId !== workspace.groupId) | ||
| .map((ws) => ( | ||
| <Listbox.Option | ||
| key={ws.groupId} | ||
| value={ws.groupId} | ||
| className={({ active }) => | ||
| clsx( | ||
| "flex items-center gap-3 px-3 py-2.5 mx-1 cursor-pointer rounded-lg transition duration-200", | ||
| active && "bg-[color:rgb(var(--group-theme)/0.1)] text-[color:rgb(var(--group-theme))]" | ||
| ) | ||
| } | ||
| > | ||
| <img | ||
| src={ws.groupThumbnail || "/placeholder.svg"} | ||
| alt="" | ||
| className="w-8 h-8 rounded-lg object-cover transition duration-200" | ||
| /> | ||
| <span className="flex-1 truncate text-sm dark:text-white">{ws.groupName}</span> | ||
| {workspace.groupId === ws.groupId && <IconCheck className="w-5 h-5 text-primary" />} | ||
| </Listbox.Option> | ||
| )) | ||
| ) : ( | ||
| <div className="px-3 py-2 text-sm text-zinc-500 dark:text-zinc-400"> | ||
| No other workspaces | ||
| </div> | ||
| )} | ||
| </Listbox.Options> | ||
| <span className={clsx("flex shrink-0 overflow-hidden rounded-xl bg-zinc-100 dark:bg-zinc-800", isCollapsed ? "w-9 h-9" : "w-10 h-10")}> | ||
| <img | ||
| src={workspace.groupThumbnail || "/favicon-32x32.png"} | ||
| alt="" | ||
| width={isCollapsed ? 36 : 40} | ||
| height={isCollapsed ? 36 : 40} | ||
| className="h-full w-full object-contain" | ||
| /> | ||
| </span> | ||
| {!isCollapsed && ( | ||
| <> | ||
| <div className="flex-1 min-w-0 text-left"> | ||
| <p className="text-sm font-semibold truncate text-zinc-900 dark:text-white"> | ||
| {workspace.groupName} | ||
| </p> | ||
| <p className="text-[11px] text-zinc-500 dark:text-zinc-400 truncate">Workspace</p> | ||
| </div> | ||
| <IconChevronDown className="w-4 h-4 text-zinc-400 shrink-0" stroke={1.5} /> | ||
| </> | ||
| )} | ||
| </Listbox.Button> |
Contributor
There was a problem hiding this comment.
Add accessible names for collapsed icon-only controls.
When isCollapsed is true, the workspace switcher, page buttons, and account menu all lose their visible text without gaining an aria-label. That makes the collapsed sidebar largely unusable with assistive tech.
💡 Small accessibility patch
<Listbox.Button
+ aria-label={isCollapsed ? `Switch workspace: ${workspace.groupName}` : undefined}
className={clsx(
...
<button
key={page.name}
type="button"
+ aria-label={isCollapsed ? page.name : undefined}
onClick={() => gotopage(page.href)}
className={clsx(
...
<Menu.Button
+ aria-label={isCollapsed ? "Account menu" : undefined}
className={clsx(Also applies to: 277-323, 335-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/sidebar.tsx` around lines 205 - 232, The collapsed sidebar drops
visible text for controls like the workspace switcher Listbox.Button (uses
workspace.groupName/groupThumbnail and IconChevronDown) when isCollapsed is
true, so add accessible names: when isCollapsed is true provide an aria-label
(or aria-labelledby) on the Listbox.Button describing the control (e.g.,
"Workspace switcher: {workspace.groupName}"), and apply the same pattern for the
collapsed page buttons and account menu components referenced elsewhere (the
collapsed icon-only controls at the other blocks). Ensure the aria-label updates
to include the current workspace or account name and is only added when
isCollapsed is true so screen readers get a meaningful name for the icon-only
buttons.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
i added pwa for mobile, however it removes the phone UI, so yeah
(i messed up the commit name somehow)
Summary by CodeRabbit
New Features
Removed