Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe PR refactors desktop onboarding navigation from platform-detection-based routing to route-based search parameters. Auth callbacks are wrapped in useCallback and context values in useMemo for performance optimization. Navigation logic consolidates into getNext and getBack functions, with step identifiers exported as constants. The Search type unifies platform, local, and pro flags into a single state object. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ 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: 1
🧹 Nitpick comments (1)
apps/desktop/src/components/onboarding/config.tsx (1)
17-30: Consider adding exhaustiveness check.The switch statement covers all current step IDs. However, TypeScript's exhaustiveness checking relies on proper narrowing. Adding an explicit check would catch bugs if a new step is added but not handled here:
🔎 Optional: Add exhaustiveness guard
export function getNext(ctx: Search): Search["step"] | "done" { switch (ctx.step) { case STEP_ID_WELCOME: if (ctx.local) return STEP_ID_CONFIGURE_NOTICE; if (ctx.platform === "macos") return STEP_ID_PERMISSIONS; return STEP_ID_LOGIN; case STEP_ID_PERMISSIONS: return ctx.local ? STEP_ID_CONFIGURE_NOTICE : STEP_ID_LOGIN; case STEP_ID_LOGIN: return ctx.pro ? "done" : STEP_ID_CONFIGURE_NOTICE; case STEP_ID_CONFIGURE_NOTICE: return "done"; + default: { + const _exhaustive: never = ctx.step; + return _exhaustive; + } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src/auth.tsx(6 hunks)apps/desktop/src/components/onboarding/calendar.tsx(2 hunks)apps/desktop/src/components/onboarding/config.tsx(1 hunks)apps/desktop/src/components/onboarding/configure-notice.tsx(2 hunks)apps/desktop/src/components/onboarding/login.tsx(3 hunks)apps/desktop/src/components/onboarding/permissions.tsx(4 hunks)apps/desktop/src/components/onboarding/welcome.tsx(3 hunks)apps/desktop/src/routes/app/onboarding/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Format using
dprint fmtfrom the root. Do not usecargo fmt.
Files:
apps/desktop/src/components/onboarding/configure-notice.tsxapps/desktop/src/components/onboarding/calendar.tsxapps/desktop/src/components/onboarding/permissions.tsxapps/desktop/src/components/onboarding/login.tsxapps/desktop/src/components/onboarding/config.tsxapps/desktop/src/routes/app/onboarding/index.tsxapps/desktop/src/components/onboarding/welcome.tsxapps/desktop/src/auth.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props. Just inline them.
Never do manual state management for form/mutation. UseuseFormfrom tanstack-form anduseQuery/useMutationfrom tanstack-query for 99% cases.
Files:
apps/desktop/src/components/onboarding/configure-notice.tsxapps/desktop/src/components/onboarding/calendar.tsxapps/desktop/src/components/onboarding/permissions.tsxapps/desktop/src/components/onboarding/login.tsxapps/desktop/src/components/onboarding/config.tsxapps/desktop/src/routes/app/onboarding/index.tsxapps/desktop/src/components/onboarding/welcome.tsxapps/desktop/src/auth.tsx
**/*.{ts,tsx,rs,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
By default, avoid writing comments at all. If you write one, it should be about 'Why', not 'What'.
Files:
apps/desktop/src/components/onboarding/configure-notice.tsxapps/desktop/src/components/onboarding/calendar.tsxapps/desktop/src/components/onboarding/permissions.tsxapps/desktop/src/components/onboarding/login.tsxapps/desktop/src/components/onboarding/config.tsxapps/desktop/src/routes/app/onboarding/index.tsxapps/desktop/src/components/onboarding/welcome.tsxapps/desktop/src/auth.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: If there are many classNames with conditional logic, usecn(import from@hypr/utils). Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/components/onboarding/configure-notice.tsxapps/desktop/src/components/onboarding/calendar.tsxapps/desktop/src/components/onboarding/permissions.tsxapps/desktop/src/components/onboarding/login.tsxapps/desktop/src/components/onboarding/config.tsxapps/desktop/src/routes/app/onboarding/index.tsxapps/desktop/src/components/onboarding/welcome.tsxapps/desktop/src/auth.tsx
🧬 Code graph analysis (6)
apps/desktop/src/components/onboarding/configure-notice.tsx (2)
apps/desktop/src/components/onboarding/config.tsx (2)
getBack(32-44)getNext(17-30)apps/desktop/src/routes/app/onboarding/index.tsx (1)
Route(24-27)
apps/desktop/src/components/onboarding/calendar.tsx (2)
apps/desktop/src/components/onboarding/config.tsx (2)
StepProps(13-15)getNext(17-30)apps/desktop/src/routes/app/onboarding/index.tsx (1)
Route(24-27)
apps/desktop/src/components/onboarding/config.tsx (5)
apps/desktop/src/routes/app/onboarding/index.tsx (1)
Search(22-22)apps/desktop/src/components/onboarding/welcome.tsx (1)
STEP_ID_WELCOME(9-9)apps/desktop/src/components/onboarding/configure-notice.tsx (1)
STEP_ID_CONFIGURE_NOTICE(5-5)apps/desktop/src/components/onboarding/permissions.tsx (1)
STEP_ID_PERMISSIONS(10-10)apps/desktop/src/components/onboarding/login.tsx (1)
STEP_ID_LOGIN(16-16)
apps/desktop/src/routes/app/onboarding/index.tsx (1)
apps/desktop/src/components/onboarding/config.tsx (3)
STEP_IDS(51-56)NavigateTarget(9-11)STEP_CONFIGS(58-63)
apps/desktop/src/components/onboarding/welcome.tsx (3)
apps/desktop/src/components/onboarding/config.tsx (2)
StepProps(13-15)getNext(17-30)apps/desktop/src/routes/app/onboarding/index.tsx (1)
Route(24-27)apps/web/src/routes/auth.tsx (1)
Route(17-20)
apps/desktop/src/auth.tsx (2)
apps/desktop/src/utils/index.ts (1)
getScheme(8-17)packages/store/src/schema-external.ts (1)
Session(169-169)
⏰ 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). (5)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: fmt
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
🔇 Additional comments (24)
apps/desktop/src/auth.tsx (7)
108-129: LGTM!The memoization is correct. Empty dependency array is appropriate since
supabaseis a module-level constant and React setState functions have stable identities.
131-145: LGTM!Correct dependency on
setSessionFromTokens.
249-253: LGTM!Empty dependency array is correct as all references (
env,getScheme,openUrl) are stable.
255-282: LGTM!Correct memoization with stable references.
284-298: LGTM!Correct memoization with stable references.
327-349: LGTM!Good use of
useMemoto stabilize the context value and prevent unnecessary re-renders of consumers. The dependency array correctly includes all dynamic values whilesupabaseis intentionally omitted as a module constant.
14-21: LGTM!apps/desktop/src/components/onboarding/welcome.tsx (2)
6-9: LGTM!The imports and constant export align well with the new route-based navigation pattern established across the onboarding flow.
40-53: LGTM!The navigation logic correctly preserves search state while computing the next step. The "Proceed without account" path properly sets
local: truebefore callinggetNext, ensuring the navigation flow respects the local mode configuration.apps/desktop/src/components/onboarding/configure-notice.tsx (2)
7-16: LGTM!The back navigation logic correctly handles the conditional case where
backStepmay benull(e.g., when this is the first step in a local flow). The patternbackStep ? () => onNavigate(...) : undefinedensuresonBackis only provided when a back step exists.
30-36: LGTM!Forward navigation correctly spreads the existing search state and computes the next step dynamically.
apps/desktop/src/components/onboarding/calendar.tsx (1)
53-58: LGTM!The skip navigation correctly preserves search state and computes the next step.
apps/desktop/src/components/onboarding/permissions.tsx (3)
6-11: LGTM!The imports and constant export follow the established pattern. The
STEP_ID_PERMISSIONSconstant is correctly exported for use in the navigation config.
76-102: LGTM!The navigation setup correctly uses
Route.useSearch()to get current state, computes the back step conditionally, and providesonBackonly when navigation backward is possible. The pattern is consistent with other onboarding components.
139-152: LGTM!Forward navigation correctly preserves the search state while computing the next step. The button remains disabled until all permissions are granted, which is appropriate UX.
apps/desktop/src/components/onboarding/login.tsx (3)
10-17: LGTM!The imports are well-organized and the
STEP_ID_LOGINconstant export follows the established pattern.
78-88: LGTM!The success handler correctly updates the
proflag in the search state before computing the next step, ensuring the navigation logic can make the right decision based on trial status. The error handler provides a reasonable fallback to the configure notice step.
99-113: No changes needed —signInis already properly memoized.The
signInfunction in the auth context is already wrapped inuseCallbackwith an empty dependency array, ensuring it maintains a stable reference and won't cause repeated effect executions. The dependency array in the effect is correctly specified.Likely an incorrect or invalid review comment.
apps/desktop/src/routes/app/onboarding/index.tsx (3)
40-50: LGTM!The navigation callback correctly destructures the
NavigateTarget, handles the "done" case, and preserves the remaining search parameters during navigation. The dependency array is correct.
52-67: LGTM!The step lookup gracefully handles unknown steps by returning
null, and the component rendering pattern is clean.
15-20:platform()is safely synchronous.The
platform()function returns a string describing the operating system, with the value set at compile time. This makes it a synchronous function that can be safely invoked at module load time. No lazy evaluation or deferred initialization is needed, as the value is determined at compile time, not at runtime.apps/desktop/src/components/onboarding/config.tsx (3)
3-11: LGTM!The
NavigateTargettype cleanly extendsSearchto include "done" as a valid step target, enabling type-safe navigation including flow completion.
32-44: LGTM!The back navigation logic correctly mirrors the forward flow, properly accounting for platform and local mode variations.
51-63: LGTM!The
STEP_IDSarray withas constensures proper tuple typing for zod'sz.enum(), andSTEP_CONFIGScorrectly maps each step ID to its component.
1 participant
No description provided.