feat: implement webapp routing #899

acezard · 2023-08-18T14:06:41Z

description to be completed

temp

removed some race conditions removed faulty redirections cleaned state management

Ldoppea · 2023-08-22T14:41:01Z

src/app/domain/sharing/services/SharingNetwork.ts

+import { SharingCozyApp } from '/app/domain/sharing/models/SharingCozyApp'
+import { ServiceResponse } from '/app/domain/sharing/models/SharingState'
+
+export const fetchSharingCozyApps = {


@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?

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

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--

(I will still update index/alias)

Ldoppea · 2023-08-22T15:03:19Z

src/app/domain/sharing/models/SharingState.ts

+  | { type: SharingActionType.SetFlowErrored; payload: boolean }
+  | { type: SharingActionType.SetRecoveryState }
+
+export interface ServiceResponse<T> {


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

I agree it would be better.

Updated

Ldoppea · 2023-08-22T15:12:13Z

src/hooks/useGlobalAppState.ts


  useEffect(() => {
    let subscription: NativeEventSubscription | undefined

    // If there's no client, we don't need to listen to app state changes
    // because we can't lock the app anyway
-    if (client && !subscription) {
+    if (!hasExecuted.current && client && !subscription) {


Does this mean that if client changes (or any other array dependency) the subscription will be removed but will never register again (as hasExecuted.current is never set back to false)? Is that expected?

I will double-check the lifecycle of this event listener, I didn't notice a removal that broke my appStatus change flow but it's better to be safe

Crash-- · 2023-08-23T06:08:03Z

src/App.js

@@ -132,7 +132,9 @@ const WrappedApp = () => {
  if (client)
    return (
      <CozyProvider client={client}>
-        <Nav client={client} setClient={setClient} />
+        <SharingProvider>


No way we call that SharingProvider, like I said you yesterday. We already have a SharingProvider https://github.com/cozy/cozy-libs/blob/master/packages/cozy-sharing/src/SharingProvider.jsx that doesn't do the same thing.

Crash-- · 2023-08-23T06:12:04Z

src/app/domain/sharing/models/SharingCozyApp.ts

+    accept_from_flagship?: boolean
+  }
+  slug: string
+}


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.

Crash-- · 2023-08-23T06:14:13Z

src/app/domain/sharing/services/SharingNetwork.ts

+
+export const fetchSharingCozyApps = {
+  definition: Q('io.cozy.apps').where({
+    'accept_documents_from_flagship.route_to_upload': { $exists: true },


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

Crash-- · 2023-08-23T06:18:07Z

src/app/domain/sharing/services/SharingNetwork.ts

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


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

Crash-- · 2023-08-23T06:18:54Z

src/app/domain/sharing/services/SharingNetwork.ts

+      pathname: '',
+      slug,
+      subDomainType: session.subDomainType,
+      hash: hash.replace(/^\/?#?\//, ''),


Why do we have to make this replace?

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

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?

Crash-- · 2023-08-23T06:20:29Z

src/app/view/sharing/SharingProvider.tsx

+    if (client === null) return
+
+    const route = getRouteToUpload(
+      fetchedApps as SharingCozyApp[],


Are we ok to call this getRouteToUpload with a fetchedApps equals to null because fetchStatus===loading/pending/errored ?

In my mind yes because it defers the null check to the low-level function (it will handle null or empty array data). That way the SharingProvider (to be renamed as we said) can focus on the user flow logic without too much coupling to data handling.

we can see the check here https://github.com/cozy/cozy-flagship-app/pull/899/files#diff-01d469baef9fd23e2cab7b642078293aea5526fbacdeb12c5c58c30f3c9f16a9R23

if you say so, but you have to deal with the fact that the request can fail (fetchStatus===errored) somewhere.

if you say so, but you have to deal with the fact that the request can fail (fetchStatus===errored) somewhere.

true, I didn't test for this scenario.

To update

acezard · 2023-08-30T09:04:49Z

closing for better readability, every commit can be seen on the last branch/pr: #906 (every comment remains meaningful and will be updated on the main branch)

acezard added 4 commits August 18, 2023 16:05

feat: implement webapp routing

48c98b5

temp

feat: implement error handling

769dc6d

refactor: handle full flow lifecycle

ce481b3

removed some race conditions removed faulty redirections cleaned state management

chore: fix tests

5557ddc

acezard marked this pull request as ready for review August 22, 2023 08:05

acezard requested review from Ldoppea and zatteo as code owners August 22, 2023 08:05

Ldoppea reviewed Aug 22, 2023

View reviewed changes

Crash-- reviewed Aug 23, 2023

View reviewed changes

acezard closed this Aug 30, 2023

acezard mentioned this pull request Aug 30, 2023

feat: Handle file upload from mobile OS to Drive and Mespapiers #906

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement webapp routing #899

feat: implement webapp routing #899

acezard commented Aug 18, 2023 •

edited

Loading

Ldoppea Aug 22, 2023

Crash-- Aug 23, 2023

Ldoppea Aug 23, 2023

acezard Aug 23, 2023

Ldoppea Aug 22, 2023

acezard Aug 22, 2023

Ldoppea Aug 22, 2023

acezard Aug 22, 2023

Crash-- Aug 23, 2023

Crash-- Aug 23, 2023

Crash-- Aug 23, 2023

Crash-- Aug 23, 2023

Crash-- Aug 23, 2023

acezard Aug 23, 2023

Crash-- Aug 23, 2023

Crash-- Aug 23, 2023

acezard Aug 23, 2023 •

edited

Loading

Crash-- Aug 23, 2023

acezard Aug 23, 2023

acezard commented Aug 30, 2023

feat: implement webapp routing #899

feat: implement webapp routing #899

Conversation

acezard commented Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acezard Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acezard commented Aug 30, 2023

acezard commented Aug 18, 2023 •

edited

Loading

acezard Aug 23, 2023 •

edited

Loading