Feat(webapp): collapsible side menu #2939
Conversation
|
WalkthroughThis PR adds a collapsible side-menu feature and threads an Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (6)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (2)apps/webapp/app/components/AskAI.tsx (4)
apps/webapp/app/components/navigation/SideMenu.tsx (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (12)
✏️ Tip: You can disable this entire section by setting 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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/webapp/app/routes/resources.incidents.tsx (2)
58-110: Prevent hidden incident controls from remaining tabbable.Both panels are only hidden via height/opacity, so the LinkButton / PopoverTrigger can stay in the tab order when invisible. Consider adding
aria-hiddenand removing focusability when hidden.♿ Suggested a11y hardening
- <motion.div + <motion.div initial={false} animate={{ height: isCollapsed ? 0 : "auto", opacity: isCollapsed ? 0 : 1, }} transition={{ duration: 0.15 }} - className="overflow-hidden" + aria-hidden={isCollapsed} + className={`overflow-hidden ${isCollapsed ? "pointer-events-none" : ""}`} > @@ - <LinkButton + <LinkButton variant="secondary/small" to="https://status.trigger.dev" target="_blank" fullWidth className="border-warning/20 bg-warning/10 hover:!border-warning/30 hover:!bg-warning/20" + tabIndex={isCollapsed ? -1 : undefined} > @@ - <motion.div + <motion.div initial={false} animate={{ height: isCollapsed ? "auto" : 0, opacity: isCollapsed ? 1 : 0, }} transition={{ duration: 0.15 }} - className="overflow-hidden" + aria-hidden={!isCollapsed} + className={`overflow-hidden ${isCollapsed ? "" : "pointer-events-none"}`} > @@ - <PopoverTrigger className="flex !h-8 w-full items-center justify-center rounded border border-warning/20 bg-warning/10 transition-colors hover:border-warning/30 hover:bg-warning/20"> + <PopoverTrigger + className="flex !h-8 w-full items-center justify-center rounded border border-warning/20 bg-warning/10 transition-colors hover:border-warning/30 hover:bg-warning/20" + tabIndex={!isCollapsed ? -1 : undefined} + >
34-46: IncludefetchIncidentsin the effect dependencies.The effect uses
fetchIncidentson lines 41 and 43 but omits it from the dependency array, which violates ESLint's exhaustive-deps rule. AlthoughfetchIncidentsis memoized and remains stable, the linter cannot verify this.🔧 Suggested fix
- }, []); + }, [fetchIncidents]);
🤖 Fix all issues with AI agents
In `@apps/webapp/app/components/AskAI.tsx`:
- Around line 118-135: The TooltipTrigger currently uses asChild to wrap a
non-focusable div, preventing keyboard focus from triggering the tooltip; move
the layout div (the container that uses isCollapsed to set "w-full" vs
"inline-flex") outside of TooltipProvider/TooltipTrigger and have TooltipTrigger
(with asChild) directly wrap the Button so the Button (which forwards refs)
becomes the trigger; ensure you keep the Button props (variant, data-action,
shortcut, hideShortcutKey, data-modal-override-open-class-ask-ai, onClick
calling openAskAI, and className conditional on isCollapsed) and preserve the
surrounding motion.div and TooltipProvider structure.
In `@apps/webapp/app/components/navigation/SideMenu.tsx`:
- Around line 984-996: The toggle button lacks a type and accessible name;
update the button element rendered alongside AnimatedChevron to include
type="button" and an appropriate accessible label (e.g., aria-label="Toggle
sidebar" or aria-label that reflects its action) and also add
aria-expanded={isCollapsed ? "false" : "true"} (or the inverse if your collapsed
logic differs) so screen readers get state; modify the button where
onClick={onToggle}, onMouseEnter/onMouseLeave and isHovering/isCollapsed are
used to include these attributes.
- Around line 566-605: The tooltip content currently interpolates project.name
directly and can show "undefined" when missing; update the SimpleTooltip's
content prop to use the same fallback as the button label (e.g., use
project.name ?? "Select a project") so the content reads `${organization.title}
/ ${project.name ?? "Select a project"}`; locate SimpleTooltip in SideMenu.tsx
and adjust its content prop accordingly to reference project.name with the
fallback.
- Around line 135-193: On unmount flush any pending debounced preference changes
so the last toggle isn’t lost: in the cleanup returned by the useEffect add
logic that (using pendingPreferencesRef and debounceTimeoutRef) first clears the
timeout (if any), then if pendingPreferencesRef.current has isCollapsed or
manageSectionCollapsed and user.isImpersonating is false, build a FormData the
same way persistSideMenuPreferences does and call preferencesFetcher.submit with
method "POST" and action "/resources/preferences/sidemenu", then clear
pendingPreferencesRef.current; keep the existing clearTimeout behavior and
ensure you reference persistSideMenuPreferences's pendingPreferencesRef,
debounceTimeoutRef and preferencesFetcher.submit to mirror the debounced submit.
🧹 Nitpick comments (2)
apps/webapp/app/components/Shortcuts.tsx (1)
84-87: Minor formatting inconsistency.Line 85 is missing a space before the closing braces in the shortcut object, inconsistent with other similar usages in this file (e.g., line 75).
✨ Suggested fix
<Shortcut name="Toggle side menu"> - <ShortcutKey shortcut={{ modifiers: ["mod"]}} variant="medium/bright" /> + <ShortcutKey shortcut={{ modifiers: ["mod"] }} variant="medium/bright" /> <ShortcutKey shortcut={{ key: "b" }} variant="medium/bright" /> </Shortcut>apps/webapp/app/routes/resources.incidents.tsx (1)
67-92: Deduplicate the incident panel markup.The expanded panel and
IncidentPopoverContentrender identical content, which is easy to drift. Reuse the helper component for both.♻️ Suggested refactor
- <div className="flex flex-col gap-2 rounded border border-warning/20 bg-warning/5 p-2 pt-1.5"> - {/* Header */} - <div className="flex items-center gap-1 border-b border-warning/20 pb-1 text-warning"> - <ExclamationTriangleIcon className="size-4" /> - <Paragraph variant="small/bright" className="text-warning"> - Active incident - </Paragraph> - </div> - - {/* Description */} - <Paragraph variant="extra-small/bright" className="text-warning/80"> - Our team is working on resolving the issue. Check our status page for more - information. - </Paragraph> - - {/* Button */} - <LinkButton - variant="secondary/small" - to="https://status.trigger.dev" - target="_blank" - fullWidth - className="border-warning/20 bg-warning/10 hover:!border-warning/30 hover:!bg-warning/20" - > - <span className="text-warning">View status page</span> - </LinkButton> - </div> + <IncidentPopoverContent />Also applies to: 126-147
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/webapp/app/components/AskAI.tsxapps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/environments/EnvironmentLabel.tsxapps/webapp/app/components/navigation/AccountSideMenu.tsxapps/webapp/app/components/navigation/EnvironmentSelector.tsxapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/components/navigation/SideMenuHeader.tsxapps/webapp/app/components/navigation/SideMenuItem.tsxapps/webapp/app/components/navigation/SideMenuSection.tsxapps/webapp/app/components/primitives/Popover.tsxapps/webapp/app/hooks/useUser.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsxapps/webapp/app/routes/resources.incidents.tsxapps/webapp/app/routes/resources.preferences.sidemenu.tsxapps/webapp/app/services/dashboardPreferences.server.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/components/navigation/AccountSideMenu.tsxapps/webapp/app/components/environments/EnvironmentLabel.tsxapps/webapp/app/components/navigation/EnvironmentSelector.tsxapps/webapp/app/routes/resources.preferences.sidemenu.tsxapps/webapp/app/hooks/useUser.tsapps/webapp/app/services/dashboardPreferences.server.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/SideMenuSection.tsxapps/webapp/app/routes/resources.incidents.tsxapps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/navigation/SideMenuHeader.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/components/primitives/Popover.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/components/navigation/SideMenuItem.tsxapps/webapp/app/components/AskAI.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/navigation/AccountSideMenu.tsxapps/webapp/app/components/environments/EnvironmentLabel.tsxapps/webapp/app/components/navigation/EnvironmentSelector.tsxapps/webapp/app/routes/resources.preferences.sidemenu.tsxapps/webapp/app/hooks/useUser.tsapps/webapp/app/services/dashboardPreferences.server.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/SideMenuSection.tsxapps/webapp/app/routes/resources.incidents.tsxapps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/navigation/SideMenuHeader.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/components/primitives/Popover.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/components/navigation/SideMenuItem.tsxapps/webapp/app/components/AskAI.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/components/navigation/AccountSideMenu.tsxapps/webapp/app/components/environments/EnvironmentLabel.tsxapps/webapp/app/components/navigation/EnvironmentSelector.tsxapps/webapp/app/routes/resources.preferences.sidemenu.tsxapps/webapp/app/hooks/useUser.tsapps/webapp/app/services/dashboardPreferences.server.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/SideMenuSection.tsxapps/webapp/app/routes/resources.incidents.tsxapps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/navigation/SideMenuHeader.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/components/primitives/Popover.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/components/navigation/SideMenuItem.tsxapps/webapp/app/components/AskAI.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/navigation/AccountSideMenu.tsxapps/webapp/app/components/environments/EnvironmentLabel.tsxapps/webapp/app/components/navigation/EnvironmentSelector.tsxapps/webapp/app/routes/resources.preferences.sidemenu.tsxapps/webapp/app/hooks/useUser.tsapps/webapp/app/services/dashboardPreferences.server.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/SideMenuSection.tsxapps/webapp/app/routes/resources.incidents.tsxapps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/navigation/SideMenuHeader.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/components/primitives/Popover.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/components/navigation/SideMenuItem.tsxapps/webapp/app/components/AskAI.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/components/navigation/AccountSideMenu.tsxapps/webapp/app/components/environments/EnvironmentLabel.tsxapps/webapp/app/components/navigation/EnvironmentSelector.tsxapps/webapp/app/routes/resources.preferences.sidemenu.tsxapps/webapp/app/hooks/useUser.tsapps/webapp/app/services/dashboardPreferences.server.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/SideMenuSection.tsxapps/webapp/app/routes/resources.incidents.tsxapps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/navigation/SideMenuHeader.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/components/primitives/Popover.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/components/navigation/SideMenuItem.tsxapps/webapp/app/components/AskAI.tsx
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/components/navigation/AccountSideMenu.tsxapps/webapp/app/components/environments/EnvironmentLabel.tsxapps/webapp/app/components/navigation/EnvironmentSelector.tsxapps/webapp/app/routes/resources.preferences.sidemenu.tsxapps/webapp/app/hooks/useUser.tsapps/webapp/app/services/dashboardPreferences.server.tsapps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxapps/webapp/app/components/navigation/SideMenuSection.tsxapps/webapp/app/routes/resources.incidents.tsxapps/webapp/app/components/Shortcuts.tsxapps/webapp/app/components/navigation/SideMenuHeader.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/components/primitives/Popover.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/components/navigation/SideMenuItem.tsxapps/webapp/app/components/AskAI.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/hooks/useUser.tsapps/webapp/app/services/dashboardPreferences.server.ts
apps/webapp/app/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Separate testable services from configuration files; follow the pattern of
realtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp
Files:
apps/webapp/app/services/dashboardPreferences.server.ts
🧠 Learnings (5)
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/**/*.{ts,tsx} : Access all environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` in the Trigger.dev webapp
Applied to files:
apps/webapp/app/components/navigation/EnvironmentSelector.tsx
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
apps/webapp/app/routes/resources.preferences.sidemenu.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsx
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from `trigger.dev/core` in the webapp, use subpath exports from the package.json instead of importing from the root path
Applied to files:
apps/webapp/app/hooks/useUser.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Use `useRun`, `useRealtimeRun` and other SWR/realtime hooks from `trigger.dev/react-hooks` for data fetching
Applied to files:
apps/webapp/app/routes/resources.incidents.tsx
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsx
🧬 Code graph analysis (7)
apps/webapp/app/components/navigation/AccountSideMenu.tsx (2)
apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx (1)
HelpAndFeedback(32-254)apps/webapp/app/components/AskAI.tsx (1)
AskAI(41-67)
apps/webapp/app/routes/resources.preferences.sidemenu.tsx (3)
apps/webapp/app/services/session.server.ts (1)
requireUser(39-62)packages/core/src/v3/apps/http.ts (1)
json(65-75)apps/webapp/app/services/dashboardPreferences.server.ts (1)
updateSideMenuPreferences(111-155)
apps/webapp/app/hooks/useUser.ts (1)
apps/webapp/app/models/user.server.ts (1)
UserWithDashboardPreferences(306-308)
apps/webapp/app/services/dashboardPreferences.server.ts (1)
apps/webapp/app/services/session.server.ts (1)
UserFromSession(37-37)
apps/webapp/app/components/Shortcuts.tsx (2)
apps/webapp/app/hooks/useShortcutKeys.tsx (1)
Shortcut(7-12)apps/webapp/app/components/primitives/ShortcutKey.tsx (1)
ShortcutKey(38-55)
apps/webapp/app/components/primitives/Popover.tsx (2)
apps/webapp/app/hooks/useShortcutKeys.tsx (2)
useShortcutKeys(28-57)ShortcutDefinition(14-19)apps/webapp/app/components/primitives/ShortcutKey.tsx (1)
ShortcutKey(38-55)
apps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsx (2)
apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx (1)
HelpAndFeedback(32-254)apps/webapp/app/components/AskAI.tsx (1)
AskAI(41-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
🔇 Additional comments (19)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam/route.tsx (1)
22-22: Layout update looks good.The auto/1fr column split aligns with a collapsible side menu without affecting structure.
apps/webapp/app/components/environments/EnvironmentLabel.tsx (1)
83-90: Looks good: disableTooltip gating is clear and safe.The prop addition and conditional render are straightforward and keep the default behavior intact.
Also applies to: 122-122
apps/webapp/app/components/Shortcuts.tsx (2)
2-4: LGTM!Import reorganization looks good. All imports are used in the file.
28-29: LGTM!Styling adjustments for button padding and icon spacing look fine.
apps/webapp/app/hooks/useUser.ts (1)
1-8: LGTM!Clean type-level refactoring that introduces the
Usertype alias forUserWithDashboardPreferences. This maintains backward compatibility for existing consumers while extending the user type to include dashboard preferences. The type-only imports are correctly applied.apps/webapp/app/components/primitives/Popover.tsx (1)
152-191: LGTM!The
hideShortcutKeyprop is well-implemented—it correctly hides the visual shortcut indicator while preserving the keyboard shortcut functionality (viauseShortcutKeys). The conditional layout adjustments are appropriate for the collapsed state.apps/webapp/app/components/navigation/SideMenuItem.tsx (1)
36-91: LGTM!The tooltip-wrapped collapsible menu item implementation is clean. The framer-motion animations for width/opacity transitions provide smooth UX when toggling the collapsed state. The tooltip correctly appears only when collapsed (
hidden={!isCollapsed}).Minor note: The
&& !isCollapsedchecks on lines 63 and 75 are defensive since the parentmotion.divalready animates to zero width/opacity, but this doesn't cause issues.apps/webapp/app/services/dashboardPreferences.server.ts (2)
6-11: LGTM!The Zod schema for
SideMenuPreferencesfollows project conventions. Usingz.inferfor the type export is the correct pattern.
111-155: LGTM!The
updateSideMenuPreferencesfunction follows established patterns in this file:
- Respects impersonation state with early return
- Provides sensible defaults for missing
sideMenu- Supports partial updates (either
isCollapsedormanageSectionCollapsed)- Avoids unnecessary database writes with change detection
apps/webapp/app/routes/resources.preferences.sidemenu.tsx (1)
1-39: LGTM!The Remix action route correctly follows conventions:
- Uses
requireUserfor authentication- Validates with Zod as per coding guidelines
- Handles FormData string-to-boolean conversion appropriately
The manual boolean parsing before Zod validation is a practical approach for FormData, and the service layer handles the no-op case when nothing changes.
apps/webapp/app/components/navigation/EnvironmentSelector.tsx (2)
56-93: LGTM!The refactored trigger with SimpleTooltip integration handles the collapsed state elegantly:
- CSS transitions for width/opacity are lightweight and smooth
disableTooltipon EnvironmentLabel prevents tooltip conflicts- Tooltip shows full environment title only when collapsed
asChildanddisableHoverableContentare correctly configured for popover interaction
94-99: LGTM!Smart positioning adjustment: placing the popover to the right when collapsed (where there's more space) and below when expanded follows standard UX patterns for collapsible sidebars.
apps/webapp/app/components/navigation/SideMenuSection.tsx (1)
5-107: SideMenuSection collapse wiring looks solid.The new
isSideMenuCollapsedhandling, divider animation, and item spacing hooks read cleanly.apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx (1)
32-251: Nice collapsed trigger + shortcut integration.The tooltip + popover trigger wiring and layout adjustments look consistent with the new collapsed UX.
apps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsx (1)
148-151: Bottom bar composition looks good.Placing Help & Feedback and Ask AI side-by-side reads well for the new toolbar layout.
apps/webapp/app/components/navigation/AccountSideMenu.tsx (1)
59-62: Bottom bar alignment looks solid.The Help & Feedback + Ask AI pairing fits the new toolbar layout nicely.
apps/webapp/app/components/navigation/SideMenuHeader.tsx (1)
7-52: Animated header collapse behavior looks solid.The split-title and fade behavior reads clearly and aligns with the new collapsed UX.
apps/webapp/app/components/navigation/SideMenu.tsx (2)
227-529: Collapsed UI wiring across sections is cohesive.Nice, consistent propagation of
isCollapsedand layout transitions across header, items, and footer.
828-966: Helper components + animated chevron are clean and readable.The collapsible helpers and chevron animation are well-scoped and easy to follow.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <button | ||
| onClick={onToggle} | ||
| onMouseEnter={() => setIsHovering(true)} | ||
| onMouseLeave={() => setIsHovering(false)} | ||
| className={cn( | ||
| "group flex h-12 w-6 items-center justify-center rounded-md text-text-dimmed transition-all duration-200 focus-custom", | ||
| isHovering | ||
| ? "border border-grid-bright bg-background-bright shadow-md hover:bg-charcoal-750 hover:text-text-bright" | ||
| : "border border-transparent bg-transparent" | ||
| )} | ||
| > | ||
| <AnimatedChevron isHovering={isHovering} isCollapsed={isCollapsed} /> | ||
| </button> |
There was a problem hiding this comment.
Add a button type and accessible label.
This button has no accessible name and can default to submit inside forms.
✅ Suggested fix
- <button
+ <button
+ type="button"
+ aria-label={isCollapsed ? "Expand side menu" : "Collapse side menu"}
onClick={onToggle}
onMouseEnter={() => setIsHovering(true)}
onMouseLeave={() => setIsHovering(false)}🤖 Prompt for AI Agents
In `@apps/webapp/app/components/navigation/SideMenu.tsx` around lines 984 - 996,
The toggle button lacks a type and accessible name; update the button element
rendered alongside AnimatedChevron to include type="button" and an appropriate
accessible label (e.g., aria-label="Toggle sidebar" or aria-label that reflects
its action) and also add aria-expanded={isCollapsed ? "false" : "true"} (or the
inverse if your collapsed logic differs) so screen readers get state; modify the
button where onClick={onToggle}, onMouseEnter/onMouseLeave and
isHovering/isCollapsed are used to include these attributes.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!isManagedCloud) { | ||
| return null; |
There was a problem hiding this comment.
🔴 React hooks called after early return violates Rules of Hooks
The IncidentStatusPanel component calls React hooks (useFetcher, useCallback, useEffect) after an early return statement, which violates React's Rules of Hooks.
Click to expand
Issue Details
At resources.incidents.tsx:26-46, the component has this structure:
export function IncidentStatusPanel({ isCollapsed = false }) {
const { isManagedCloud } = useFeatures();
if (!isManagedCloud) {
return null; // Early return BEFORE other hooks
}
const fetcher = useFetcher<typeof loader>(); // Hook after conditional return!
const fetchIncidents = useCallback(...); // Hook after conditional return!
useEffect(...); // Hook after conditional return!React's Rules of Hooks require that hooks must be called in the same order on every render. When isManagedCloud is false, the component returns early and the subsequent hooks are not called. When isManagedCloud becomes true, the hooks are suddenly called, breaking React's expectation of consistent hook call order.
Impact
This can cause:
- React errors about hooks being called in different order between renders
- Unexpected component behavior when
isManagedCloudvalue changes - Potential crashes in development mode with React's strict hook checking
(Refers to lines 28-46)
Recommendation: Move all hooks to the top of the component before any conditional returns. Either move the early return after all hooks, or refactor the component to have the conditional rendering inside the JSX return statement.
Was this helpful? React with 👍 or 👎 to provide feedback.
CleanShot.2026-01-25.at.19.53.13.mp4