feat: add ability to switch app mode #229
Conversation
WalkthroughThis release includes significant updates across multiple files within the project. Key changes involve shifting npm commands to pnpm in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant HomeScreen
participant SplashScreenGuard
participant WaitingListPage
User->>App: Launch app
App->>SplashScreenGuard: Render loading animation
SplashScreenGuard-->>HomeScreen: Check start mode
HomeScreen-->>HomeScreen: Determine start mode and redirect
HomeScreen->>WaitingListPage: Navigate based on start mode
WaitingListPage-->>User: Render WaitingList component with actions
sequenceDiagram
participant Developer
participant README
Developer->>README: Follow installation and setup steps
README-->>Developer: Instructions with `pnpm` commands and `.env` setup
sequenceDiagram
participant Dev
participant Gitignore
participant LsLint
Dev->>Gitignore: Add '**/.env' pattern
Dev->>LsLint: Update regex for '.dir' files
Gitignore-->>Dev: Updated ignore patterns
LsLint-->>Dev: Updated linting rules
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| <Pressable onLongPress={() => onHiddenPressAction()}> | ||
| <Image | ||
| style={{ width: 73, height: 73 }} | ||
| source={require('@/assets/Pixel BTC.png')} |
There was a problem hiding this comment.
Does this work with a space in the filename? assets/Pixel BTC.png')
There was a problem hiding this comment.
There should be an easier way of setting an initial route. We can use Redirect component if the env variable is set to prelaunch.
Ref: https://docs.expo.dev/router/reference/redirects/#using-redirect-component
I'm also getting a delay of switching the initial routes in the current implementation. We shouldn't have that issue with the Redirect component
delay.mov
| const appStartModes = ['prelaunch', 'live'] as const; | ||
|
|
||
| export type AppStartMode = (typeof appStartModes)[number]; | ||
|
|
||
| type AppStartModeMap<T = any> = Record<AppStartMode, T>; | ||
|
|
||
| export function whenAppStartMode<K extends AppStartMode>(mode: K) { | ||
| if (!appStartModes.includes(mode)) | ||
| throw new Error('App start mode must be one of: ' + appStartModes.join(', ')); | ||
|
|
||
| return <M extends AppStartModeMap>(appStartModeMap: M): M[K] => appStartModeMap[mode]; | ||
| } |
There was a problem hiding this comment.
For this kind of stuff we can use enum, less typescript complexity. (didn't check the code, it might have some issues, but this is the gist of it)
| const appStartModes = ['prelaunch', 'live'] as const; | |
| export type AppStartMode = (typeof appStartModes)[number]; | |
| type AppStartModeMap<T = any> = Record<AppStartMode, T>; | |
| export function whenAppStartMode<K extends AppStartMode>(mode: K) { | |
| if (!appStartModes.includes(mode)) | |
| throw new Error('App start mode must be one of: ' + appStartModes.join(', ')); | |
| return <M extends AppStartModeMap>(appStartModeMap: M): M[K] => appStartModeMap[mode]; | |
| } | |
| enum AppStartModes { | |
| prelaunch = 'prelaunch', | |
| live = 'live' | |
| } | |
| type AppStartModeMap<T = any> = Record<AppStartMode, T>; | |
| export function whenAppStartMode<K extends AppStartMode>(mode: K) { | |
| if (!(mode in AppStartModes)) | |
| throw new Error('App start mode must be one of: ' + Object.values(AppStartModes).join(', ')); | |
| return <M extends AppStartModeMap>(appStartModeMap: M): M[K] => appStartModeMap[mode]; | |
| } |
There was a problem hiding this comment.
I disagree here, typescript string literals offer all the same type safety of enums, and all the developer-friendly features (autocomplete), without the need for an additional import of the enum, and use of a non-standard js feature.
Agree with this analysis https://nikolasbarwicki.com/articles/enums-vs-string-literal-types/
I could reduce the complexity here by creating type AppStartModes = 'prelaunch' | 'live' if that's the problem, though I've found it a helpful pattern to infer the type value from the const array. It's then easy to consume either the type or runtime const.
I understand that the Typescript team see enum as a mistake from the early TS days, before they aligned on the vision being 1:1 with the ts39 spec (just with types), and no extras.
There was a problem hiding this comment.
Nice stuff! Spent some time researching more the enums vs string literals topic. Didn't expect enums to be such a pain. There might be a question of having this one with array
const appStartModes = ['prelaunch', 'live'] as const;
export type AppStartMode = (typeof appStartModes)[number];or with object
const appStartModes = {
prelaunch: 'prelaunch',
live : 'live'
} as const
export type AppStartMode = keyof typeof appStartModesIn the second method, checking can be done by appStartModes[mode].
I would probably lean more towards the second method, but don't have any strong opinion on both methods
There was a problem hiding this comment.
I'm not so opinionated on these two approaches. The former is cleaner/less repetitive imo. No material difference using .includes() vs appStartModes[mode].
apps/mobile/src/app/(home)/use-force-initial-app-start-route.ts
Outdated
Show resolved
Hide resolved
|
Thanks for review @edgarkhanzadian. Not sure why you get the flash of the other view, and I didn't in my demo |
kyranjamie
force-pushed
the
feat/add-home-route
branch
from
23b5cd2
to
37c100b
Compare
| if (process.env.EXPO_PUBLIC_APP_START_MODE && !hasRun) { | ||
| hasRun = true; | ||
| return <Redirect href={startRoute} />; | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe something like this? So we avoid the variable mutation
| if (process.env.EXPO_PUBLIC_APP_START_MODE && !hasRun) { | |
| hasRun = true; | |
| return <Redirect href={startRoute} />; | |
| } | |
| return whenAppStartMode(process.env.EXPO_PUBLIC_APP_START_MODE as AppStartMode)({ | |
| live: <ThisIndexComponent /> | |
| prelaunch: <Redirect href={startRoute} /> | |
| }); | |
![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
Outside diff range and nitpick comments (1)
apps/mobile/src/queries/use-submit-waiting-list-form-email.ts (1)
Line range hint
3-17: Enhance error handling in email form submission.The mutation function for submitting emails is implemented correctly; however, it could benefit from more detailed error handling. For instance, logging the error or providing more specific error messages based on the response status could improve debugging and user feedback.
- if (!response.ok) throw new Error('Email submission failed'); + if (!response.ok) { + const errorData = await response.json(); + throw new Error(`Email submission failed: ${errorData.message}`); + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (18)
- .gitignore (1 hunks)
- .ls-lint.yml (1 hunks)
- apps/mobile/README.md (2 hunks)
- apps/mobile/src/app/(home)/index.tsx (1 hunks)
- apps/mobile/src/app/_layout.tsx (2 hunks)
- apps/mobile/src/app/index.tsx (1 hunks)
- apps/mobile/src/app/waiting-list/index.tsx (1 hunks)
- apps/mobile/src/app/waiting-list/waiting-list.tsx (6 hunks)
- apps/mobile/src/components/animations/leather-splash.tsx (1 hunks)
- apps/mobile/src/components/spinner.tsx (1 hunks)
- apps/mobile/src/components/splash-screen-guard/splash-screen-guard.tsx (1 hunks)
- apps/mobile/src/locales/en/messages.po (1 hunks)
- apps/mobile/src/locales/pseudo-locale/messages.po (1 hunks)
- apps/mobile/src/queries/query.ts (1 hunks)
- apps/mobile/src/queries/use-submit-waiting-list-form-email.ts (2 hunks)
- apps/mobile/src/utils/when-app-start-mode.ts (1 hunks)
- packages/ui/package.json (1 hunks)
- packages/ui/src/hooks/use-load-fonts.native.ts (1 hunks)
Files not reviewed due to errors (1)
- apps/mobile/src/components/spinner.tsx (no review received)
Files skipped from review due to trivial changes (5)
- .gitignore
- apps/mobile/src/locales/pseudo-locale/messages.po
- apps/mobile/src/queries/query.ts
- packages/ui/package.json
- packages/ui/src/hooks/use-load-fonts.native.ts
Additional context used
LanguageTool
apps/mobile/README.md
[grammar] ~20-~20: The operating system from Apple is written “macOS”.
Context: ...watchman/docs/install#buildinstall) for MacOS and Linux users. -pnpm iin the mono...(MAC_OS)
[style] ~37-~37: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...on your device. - Scan the QR code that is going to be shown in the terminal after running ...(GOING_TO_WILL)
[style] ~42-~42: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...on your device. - Scan the QR code that is going to be shown in the terminal after running ...(GOING_TO_WILL)
[style] ~48-~48: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... or iOS device -pnpm run iosif you want to run it on iOS simulator - `pnpm run an...(REP_WANT_TO_VB)
[style] ~50-~50: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... simulator -pnpm run androidif you want to run it on Android emulator ## License ...(REP_WANT_TO_VB)
Markdownlint
apps/mobile/README.md
11-11: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
Additional comments not posted (10)
apps/mobile/src/app/index.tsx (1)
1-5: Approved: Simplified Root ComponentThe refactoring to use
Stackdirectly in theRootfunction simplifies the component structure and aligns well with the PR's objectives to enhance navigation handling.apps/mobile/src/app/waiting-list/index.tsx (1)
1-8: Verify Navigation Logic in WaitingListPageThe use of
router.replace('/')in theonHiddenPressActionprop effectively resets the navigation stack to the home route. Ensure this behavior aligns with the intended user flow and does not disrupt the user experience by unexpectedly navigating away from relevant content.Verification successful
Navigation Logic in WaitingListPage Verified
The use of
router.replace('/')in theonHiddenPressActionprop ofWaitingListPageis unique within the codebase. No other occurrences of this navigation logic were found, indicating that it does not conflict with other parts of the application.
apps/mobile/src/app/waiting-list/index.tsxScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check all occurrences of `router.replace('/')` to ensure they are used appropriately across the application. rg --type tsx --type ts "router.replace('/')"Length of output: 81
Script:
#!/bin/bash # Description: Check all occurrences of `router.replace('/')` to ensure they are used appropriately across the application. rg "router.replace('/')" -g "*.tsx" -g "*.ts"Length of output: 53
.ls-lint.yml (1)
5-5: Approved: Updated Regex for .dir FilesThe inclusion of parentheses in the regex pattern for
.dirfiles is a thoughtful update to accommodate potential special cases in directory naming conventions. This change should prevent linting issues with directories that include parentheses in their names.apps/mobile/src/utils/when-app-start-mode.ts (1)
1-12: Approved: Robust Implementation of App Start Mode HandlingThe implementation of
whenAppStartModeusing TypeScript's type safety features ensures that only valid app start modes are used. The ongoing discussion regarding the use of enums vs. string literals is noted, and both approaches have their merits. The current implementation using string literals is in line with TypeScript best practices and avoids the overhead of enums.apps/mobile/src/components/animations/leather-splash.tsx (1)
1-19: Verify Comprehensive Handling of Animation Events in LeatherSplashThe
LeatherSplashcomponent handles both animation end and failure by invokingonAnimationEnd. Verify that this approach adequately addresses all scenarios where the animation might fail or end, ensuring a smooth user experience.Verification successful
Comprehensive Handling of Animation Events Verified
The
LeatherSplashcomponent consistently handles both animation end and failure by invokingonAnimationEnd. This approach is verified to be adequate for managing the animation end state uniformly across the codebase.
apps/mobile/src/components/animations/leather-splash.tsxapps/mobile/src/components/splash-screen-guard/splash-screen-guard.tsxScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other uses of `onAnimationEnd` to ensure consistent handling of animation events across components. rg --type tsx --type ts "onAnimationEnd"Length of output: 68
Script:
#!/bin/bash # Description: Search for other uses of `onAnimationEnd` to ensure consistent handling of animation events across components. rg --glob '*.ts' --glob '*.tsx' "onAnimationEnd"Length of output: 620
apps/mobile/src/components/splash-screen-guard/splash-screen-guard.tsx (1)
10-22: Well implemented splash screen guard.The
SplashScreenGuardcomponent is implemented correctly and uses state effectively to manage the display based on animation completion. This approach ensures that the splash screen remains visible until the animation has finished, which can enhance user experience by ensuring that necessary resources are loaded before the app becomes interactive.apps/mobile/src/locales/en/messages.po (1)
16-44: Localization keys updated correctly.The updates to the localization keys reflect their new usage within the
waiting-listcomponent. This ensures that the translations will continue to function correctly after the refactoring.apps/mobile/README.md (2)
9-15: Clarify.envSetup InstructionsThe addition of
.envsetup instructions is crucial for new developers. Consider adding more details or examples to ensure clarity on how to configure different modes.Tools
Markdownlint
11-11: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
46-50: Update Running InstructionsThe update to use
pnpmfor running commands is well-noted. Ensure that all scripts inpackage.jsonare compatible withpnpmto avoid any execution issues.Verification successful
Update Running Instructions
The update to use
pnpmfor running commands is well-noted. Based on thepackage.jsonscripts, it appears that the commands are compatible withpnpm. The presence ofpnpmin some scripts suggests that the environment is set up to handlepnpmcommands correctly.
lint:fixscript usespnpm lint --fix.lingui:watchscript usespnpm lingui:extract:watchandpnpm lingui:compile:watch.All other tools invoked within the scripts (
expo,detox,prettier, etc.) are generally compatible withpnpm.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all scripts in `package.json` are compatible with `pnpm`. # Test: Check `package.json` for script compatibility. cat apps/mobile/package.json | jq '.scripts'Length of output: 1607
Tools
LanguageTool
[style] ~48-~48: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... or iOS device -pnpm run iosif you want to run it on iOS simulator - `pnpm run an...(REP_WANT_TO_VB)
[style] ~50-~50: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... simulator -pnpm run androidif you want to run it on Android emulator ## License ...(REP_WANT_TO_VB)
apps/mobile/src/app/waiting-list/waiting-list.tsx (1)
10-10: Refactor touseSubmitWaitingListEmailFormThe renaming and refactoring of
useFormSubmissiontouseSubmitWaitingListEmailFormimproves the semantic clarity of the hook. Ensure that all references to the old hook name are updated across the project.Verification successful
All references to
useFormSubmissionhave been updated.The search for the old hook name
useFormSubmissiondid not return any occurrences, confirming that all references have been successfully updated touseSubmitWaitingListEmailForm.
- No instances of
useFormSubmissionfound in.tsand.tsxfiles.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to the old hook name are updated. # Test: Search for the old hook name. Expect: No occurrences. rg --type tsx --type ts 'useFormSubmission'Length of output: 71
Script:
#!/bin/bash # Description: Verify that all references to the old hook name are updated. # Test: Search for the old hook name. Expect: No occurrences. rg --type ts --type tsx 'useFormSubmission'Length of output: 71
Script:
#!/bin/bash # Description: Verify that all references to the old hook name are updated. # Test: Search for the old hook name. Expect: No occurrences. rg 'useFormSubmission' --glob '**/*.ts' --glob '**/*.tsx'Length of output: 57
| const startRoute = whenAppStartMode(process.env.EXPO_PUBLIC_APP_START_MODE as AppStartMode)({ | ||
| live: '/', | ||
| prelaunch: '/waiting-list', | ||
| }); | ||
|
|
||
| export default function Home() { | ||
| const router = useRouter(); | ||
|
|
||
| if (process.env.EXPO_PUBLIC_APP_START_MODE && !hasRun) { | ||
| hasRun = true; | ||
| return <Redirect href={startRoute} />; | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider simplifying redirection logic based on previous feedback.
The current implementation uses a flag to ensure the redirection logic runs only once. However, the suggestion from edgarkhanzadian to use a guard function or encapsulate the logic in a utility function might make the code cleaner and more maintainable.
| const startRoute = whenAppStartMode(process.env.EXPO_PUBLIC_APP_START_MODE as AppStartMode)({ | |
| live: '/', | |
| prelaunch: '/waiting-list', | |
| }); | |
| export default function Home() { | |
| const router = useRouter(); | |
| if (process.env.EXPO_PUBLIC_APP_START_MODE && !hasRun) { | |
| hasRun = true; | |
| return <Redirect href={startRoute} />; | |
| } | |
| // Using a guard function to handle redirection logic | |
| if (!hasRun && process.env.EXPO_PUBLIC_APP_START_MODE) { | |
| hasRun = true; | |
| return <Redirect href={startRoute} />; | |
| } |
| import { initiateI18n } from '@/i18n'; | ||
| import { queryClient } from '@/queries/query'; | ||
| import { usePersistedStore, useProtectedStore } from '@/state'; | ||
| import { i18n } from '@lingui/core'; | ||
| import { I18nProvider } from '@lingui/react'; | ||
| import { QueryClientProvider } from '@tanstack/react-query'; | ||
| import { Stack } from 'expo-router'; | ||
| import { Slot } from 'expo-router'; |
There was a problem hiding this comment.
Tip
Codebase Verification
Incomplete Migration from <Stack /> to <Slot />
The migration from <Stack /> to <Slot /> is incomplete. The following occurrence of <Stack /> needs to be updated to <Slot /> to ensure consistency and compatibility with the new routing structure:
apps/mobile/src/app/index.tsx
Analysis chain
Migrate from <Stack /> to <Slot />
The replacement of <Stack /> with <Slot /> in the AppRouter component appears to align with the new routing structure. Ensure that all dependent components and functionalities are compatible with this change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all dependent components and functionalities are compatible with the change from `<Stack />` to `<Slot />`.
# Test: Search for usage of `<Stack />`. Expect: No occurrences.
rg --type tsx --type ts '<Stack />'
Length of output: 65
Script:
#!/bin/bash
# Description: Verify that all dependent components and functionalities are compatible with the change from `<Stack />` to `<Slot />`.
# Test: Search for usage of `<Stack />`. Expect: No occurrences.
rg --glob '*.tsx' --glob '*.ts' '<Stack />'
Length of output: 95
| <Pressable onLongPress={() => onHiddenPressAction()}> | ||
| <Image | ||
| style={{ width: 73, height: 73 }} | ||
| source={require('@/assets/Pixel BTC.png')} | ||
| /> | ||
| </Pressable> |
There was a problem hiding this comment.
Handle Space in Asset Filename
Ensure that the asset filename with a space (Pixel BTC.png) does not cause issues in different environments or platforms. Consider renaming the file to remove spaces.
- require('@/assets/Pixel BTC.png')
+ require('@/assets/Pixel_BTC.png')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.
| <Pressable onLongPress={() => onHiddenPressAction()}> | |
| <Image | |
| style={{ width: 73, height: 73 }} | |
| source={require('@/assets/Pixel BTC.png')} | |
| /> | |
| </Pressable> | |
| <Pressable onLongPress={() => onHiddenPressAction()}> | |
| <Image | |
| style={{ width: 73, height: 73 }} | |
| source={require('@/assets/Pixel_BTC.png')} | |
| /> | |
| </Pressable> |
This PR scaffolds some of the new routes we'll need for the main app. It also introduces the need for a
.envfile to toggle between the two App start modes, eitherliveorprelaunch.Expo Router is kind of confusing, and was stumped by this issue expo/router#723, so setting the initial route involves first going to one, then navigating. Not ideal but none of the
initialRouteNamesettings work.You can long press the Bitcoin icon on the welcome screen to get to the main app page.
2024-06-20-000202.mp4
Summary by CodeRabbit
New Features
SplashScreenGuard,WaitingListPage,LeatherSplash, andSpinner.whenAppStartModefor enforcing app start modes.Improvements
README.mdto usepnpminstead ofnpm.package.json.Bug Fixes
Spinnercomponent.Documentation
.envfile in the mobile appREADME.md.Refactor
StackwithSlotin the app's layout.Chores
.gitignoreand.ls-lint.ymlconfigurations.