Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement webapp routing #899

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 30 additions & 23 deletions src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ import { useNetService } from '/libs/services/NetService'
import { withSentry } from '/libs/monitoring/Sentry'
import { ThemeProvider } from '/app/theme/ThemeProvider'
import { useInitI18n } from '/locales/useInitI18n'
import { SharingProvider } from '/app/view/sharing/SharingProvider'
import { SharingProvider } from '/app/view/Sharing/SharingProvider'
import { ErrorProvider } from '/app/view/Error/ErrorProvider'

// Polyfill needed for cozy-client connection
if (!global.btoa) {
Expand All @@ -57,7 +58,11 @@ const App = ({ setClient }) => {

const { initialRoute, isLoading } = useAppBootstrap(client)

useGlobalAppState()
useGlobalAppState({
onNavigationRequest: route => {
RootNavigation.navigate(route)
}
})
useSecureBackgroundSplashScreen()
useCookieResyncOnResume()
useNotifications()
Expand All @@ -75,24 +80,28 @@ const Nav = ({ client, setClient }) => {

return (
<NavigationContainer ref={RootNavigation.navigationRef}>
<NativeIntentProvider localMethods={localMethods(client)}>
<View
style={[
styles.view,
{
backgroundColor: colors.primaryColor
}
]}
>
<StatusBar
barStyle="light-content"
backgroundColor="transparent"
translucent
/>
<IconChangedModal />
<App setClient={setClient} />
</View>
</NativeIntentProvider>
<ErrorProvider>
<SharingProvider>
<NativeIntentProvider localMethods={localMethods(client)}>
<View
style={[
styles.view,
{
backgroundColor: colors.primaryColor
}
]}
>
<StatusBar
barStyle="light-content"
backgroundColor="transparent"
translucent
/>
<IconChangedModal />
<App setClient={setClient} />
</View>
</NativeIntentProvider>
</SharingProvider>
</ErrorProvider>
</NavigationContainer>
)
}
Expand Down Expand Up @@ -159,9 +168,7 @@ const Wrapper = () => {
<SplashScreenProvider>
<NetStatusBoundary>
<ThemeProvider>
<SharingProvider>
<WrappedApp />
</SharingProvider>
<WrappedApp />
</ThemeProvider>
</NetStatusBoundary>
</SplashScreenProvider>
Expand Down
2 changes: 1 addition & 1 deletion src/AppRouter.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { ManagerScreen } from '/app/view/Manager/ManagerScreen'
import { OnboardingScreen } from '/screens/login/OnboardingScreen'
import { WelcomeScreen } from '/screens/welcome/WelcomeScreen'
import { routes } from '/constants/routes'
import { SharingScreen } from '/app/view/sharing/SharingScreen'
import { SharingScreen } from '/app/view/Sharing/SharingScreen'
import { PasswordPrompt } from '/app/view/Secure/PasswordPrompt'
import { PinPrompt } from '/app/view/Secure/PinPrompt'
import { SetPasswordView } from '/app/view/Secure/SetPasswordView'
Expand Down
8 changes: 6 additions & 2 deletions src/app/domain/authorization/services/SecurityService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { navigateToApp } from '/libs/functions/openApp'
import { hideSplashScreen } from '/app/theme/SplashScreenService'
import { SecurityNavigationService } from '/app/domain/authorization/services/SecurityNavigationService'
import { getData, StorageKeys } from '/libs/localStore'
import { SharingIntentStatus } from '/app/domain/sharing/models/SharingState'

// Can use mock functions in dev environment
const fns = getDevModeFunctions(
Expand Down Expand Up @@ -79,15 +80,18 @@ export const determineSecurityFlow = async (
href: string
slug: string
},
isCallerHandlingSplashscreen?: boolean
isCallerHandlingSplashscreen?: boolean,
sharingIntentStatus?: SharingIntentStatus
): Promise<void> => {
const openedWithSharing =
sharingIntentStatus === SharingIntentStatus.OpenedViaSharing
SecurityNavigationService.startListening()

const callbackNav = async (): Promise<void> => {
try {
if (navigationObject) {
await navigateToApp(navigationObject)
} else navigate(routes.home)
} else navigate(openedWithSharing ? routes.sharing : routes.home)
} catch (error) {
devlog('🔏', 'Error navigating to app, defaulting to home', error)
navigate(routes.home)
Expand Down
21 changes: 21 additions & 0 deletions src/app/domain/sharing/models/SharingCozyApp.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
export interface SharingCozyApp {
_id: string
_type: string
accept_documents_from_flagship?: {
accepted_mime_types: string[]
max_number_of_files: number
max_size_per_file_in_MB: number
route_to_upload: string
}
accept_from_flagship?: boolean
attributes: {
accept_documents_from_flagship?: {
accepted_mime_types: string[]
max_number_of_files: number
max_size_per_file_in_MB: number
route_to_upload: string
}
accept_from_flagship?: boolean
}
slug: string
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitly stop adding types here but adding types to cozy-client. We have the type for the konnector https://github.com/cozy/cozy-client/blob/master/packages/cozy-client/types/types.d.ts#L47 we should add the one for the app.

This current type miss a lot of stuff. You can find the all stuff here https://github.com/cozy/cozy-doctypes/blob/master/docs/io.cozy.apps.md and there https://docs.cozy.io/en/cozy-apps-registry/#properties-meaning-reference

By the way, you should update the documentation in order to add the new stuff in there.

18 changes: 17 additions & 1 deletion src/app/domain/sharing/models/SharingState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,30 @@ export enum SharingIntentStatus {

export enum SharingActionType {
SetIntentStatus = 'SET_INTENT_STATUS',
SetFilesToUpload = 'SET_FILES_TO_UPLOAD'
SetFilesToUpload = 'SET_FILES_TO_UPLOAD',
SetRouteToUpload = 'SET_ROUTE_TO_UPLOAD',
SetFlowErrored = 'SET_FLOW_ERRORED',
SetRecoveryState = 'SET_RECOVERY_STATE'
}

export interface SharingState {
sharingIntentStatus: SharingIntentStatus
filesToUpload: ReceivedFile[]
routeToUpload?: { href: string; slug: string }
errored: boolean
}

export type SharingAction =
| { type: SharingActionType.SetIntentStatus; payload: SharingIntentStatus }
| { type: SharingActionType.SetFilesToUpload; payload: ReceivedFile[] }
| {
type: SharingActionType.SetRouteToUpload
payload: { href: string; slug: string }
}
| { type: SharingActionType.SetFlowErrored; payload: boolean }
| { type: SharingActionType.SetRecoveryState }

export interface ServiceResponse<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Seems like you cannot have both result and error filled simultaneously, so I would reflect that in the type:

interface ServiceResponseOk<T> {
  result: T
}

interface ServiceResponseError {
  error: string
}

export type ServiceResponse<T> = ServiceResponseOk<T> | ServiceResponseError

Hower this would complexify a bit type checks (exemple here and here) but it ensures not type overlap is possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would be better.

  • Updated

result?: T
error?: string
}
59 changes: 59 additions & 0 deletions src/app/domain/sharing/services/SharingNetwork.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import CozyClient from 'cozy-client'

import { SharingCozyApp } from '/app/domain/sharing/models/SharingCozyApp'
import { getRouteToUpload } from '/app/domain/sharing/services/SharingNetwork'

describe('getRouteToUpload', () => {
const mockCozyClient = new CozyClient({
uri: 'http://cozy.local',
capabilities: { flat_subdomains: true }
})

it('returns empty object if no client is provided', () => {
const result = getRouteToUpload()
expect(result).toEqual({})
})

it('returns empty object if cozyApps is not an array or empty', () => {
// @ts-expect-error Testing invalid input
const result = getRouteToUpload(null, mockCozyClient)
expect(result).toEqual({})
})

it('returns empty object if no matching app is found', () => {
const cozyApps = [{ slug: 'wrong-app' } as SharingCozyApp]

const result = getRouteToUpload(cozyApps, mockCozyClient)
expect(result).toEqual({})
})

it('returns the correct href and slug', () => {
const cozyApps = [
{
slug: 'drive',
accept_documents_from_flagship: {
route_to_upload: '/upload-route'
},
attributes: {
accept_documents_from_flagship: {
route_to_upload: '/upload-route'
}
}
} as SharingCozyApp
]
const result = getRouteToUpload(cozyApps, mockCozyClient, 'drive')
expect(result).toEqual({
result: {
href: 'http://cozy-drive.local/#/upload-route',
slug: 'drive'
}
})
})

it('handles an error gracefully', () => {
const brokenClient = new CozyClient({ uri: undefined })
const cozyApps = [{ slug: 'drive' } as SharingCozyApp]
const result = getRouteToUpload(cozyApps, brokenClient)
expect(result).toEqual({ error: 'Error determining route to upload.' })
})
})
48 changes: 48 additions & 0 deletions src/app/domain/sharing/services/SharingNetwork.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import CozyClient, { generateWebLink, Q } from 'cozy-client'

import { sharingLogger } from '/app/domain/sharing'
import { SharingCozyApp } from '/app/domain/sharing/models/SharingCozyApp'
import { ServiceResponse } from '/app/domain/sharing/models/SharingState'

export const fetchSharingCozyApps = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Crash-- can you check this part? Will this work if we request io.cozy.apps instead of the registry?

My intuition is that io.cozy.apps will only list apps that are installed on the cozy but not candidate apps that are not yet installed on the cozy. Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, by default, Q('io.cozy.apps') will only have the source === stack https://github.com/cozy/cozy-client/blob/master/packages/cozy-stack-client/src/AppCollection.js#L55 . So you'll only request the installed applications.

Before you should have just add the registry to the source, but now, you have to make a different Query : Q('io.cozy.apps_registry') that will list the apps from the registry. cozy/cozy-client#1376

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback from our latest meeting: We just changed the specs to optimize development delays

  • We will start to list only installed app
  • And then list "non installed" app only after implementing mespapiers.

So for this PR, listing io.cozy.apps is expected

cc @Crash--

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I will still update index/alias)

definition: Q('io.cozy.apps').where({
'accept_documents_from_flagship.route_to_upload': { $exists: true },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not add the $exists:true but instead you should index on this attribute.

https://docs.cozy.io/en/tutorials/data/advanced/#indexes-why-is-my-document-not-being-retrieved

accept_from_flagship: true
}),
options: {
as: 'io.cozy.apps/fetchSharingCozyApps'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not call the query by the functional meaning but by what it does: https://github.com/cozy/cozy-guidelines/#naming-of-queries

}
}

export const getRouteToUpload = (
cozyApps?: SharingCozyApp[],
client?: CozyClient | null,
appName = 'drive'
): ServiceResponse<{ href: string; slug: string }> => {
try {
if (!client || !Array.isArray(cozyApps) || cozyApps.length === 0) return {}

const cozyApp = cozyApps.find(cozyApp => cozyApp.slug === appName)
if (!cozyApp) return {}
const hash =
cozyApp.accept_documents_from_flagship?.route_to_upload ??
cozyApp.attributes.accept_documents_from_flagship?.route_to_upload
const slug = cozyApp.slug
if (!hash || !slug) return {}

const href = generateWebLink({
cozyUrl: client.getStackClient().uri,
pathname: '',
slug,
subDomainType: client.capabilities.flat_subdomains ? 'flat' : 'nested',
hash: hash.replace(/^\/?#?\//, ''),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to make this replace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's to handle route_to_upload looking like that: "route_to_upload": "/#/createfromFlagshipUpload=true". I did not find a way to handle it directly with the generateWebLink(). I agree it doesn't look great overall. Probably best to remove the leading /# in the manifest. I don't think the manifest has to be aware we are using hash router anyway

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, actually in the manifest, we can define a few routes. Those routes are used by the server, so if you create a /test route, then the url will be : foo.mycozy.cloud/test.

I don't know if it's a good idea to use /createfromFlagship directly in the manifest, since we can think this will be related to the server.

The manifest is at the intersection between the server and the front end app.

The manifest is linked to the app, so the manifest knows about the structure of the app and knows if the app use an hash router or not.

@Ldoppea do you have a better idea?

searchParams: []
})

sharingLogger.info('routeToUpload is', { href, slug })
return { result: { href, slug: cozyApp.slug } }
} catch (error) {
sharingLogger.error('Error when getting routeToUpload', error)
return { error: 'Error determining route to upload.' }
}
}
4 changes: 1 addition & 3 deletions src/app/domain/sharing/services/SharingStatus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ export const handleSharing = (
: SharingIntentStatus.NotOpenedViaSharing

sharingLogger.info(
`${
isAndroid ? 'Android' : 'iOS'
} App was opened or resumed via sharing, setting status to ${newStatus}`
`App was opened or resumed via sharing, setting status to ${newStatus}`
)

setStatus(newStatus)
Expand Down
62 changes: 62 additions & 0 deletions src/app/view/Error/ErrorProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import React, { ReactNode, useCallback, useEffect } from 'react'

import { ErrorToaster } from '/app/view/Error/ErrorToaster'

interface ErrorState {
message: string | null
}

export const ErrorStateContext = React.createContext<ErrorState>({
message: null
})
export const ErrorDispatchContext = React.createContext<
React.Dispatch<React.SetStateAction<string | null>>
>(() => {
// eslint-disable-next-line no-console
console.warn('ErrorDispatchContext used outside of ErrorProvider.')
})

export const ErrorProvider = ({
children
}: {
children: ReactNode
}): JSX.Element => {
const [error, setError] = React.useState<string | null>(null)

useEffect(() => {
if (error) {
const timer = setTimeout(() => setError(null), 3000)
return () => clearTimeout(timer)
}
}, [error])

return (
<ErrorStateContext.Provider value={{ message: error }}>
<ErrorDispatchContext.Provider value={setError}>
{error ? <ErrorToaster /> : null}
{children}
</ErrorDispatchContext.Provider>
</ErrorStateContext.Provider>
)
}

interface ErrorHook {
error: ErrorState
setError: React.Dispatch<React.SetStateAction<string | null>>
handleError: (errorMessage: string, callback?: () => void) => void
}

export const useError = (): ErrorHook => {
const error = React.useContext(ErrorStateContext)
const setError = React.useContext(ErrorDispatchContext)

const handleError = useCallback(
(errorMessage: string, callback?: () => void): void => {
setError(errorMessage)
callback?.()
},
[setError]
)

return { error, setError, handleError }
}
Loading
Loading