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

fix: persist candidateApps in osreceive state (VO-463) #1190

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

acezard
Copy link
Contributor

@acezard acezard commented Mar 13, 2024

🐛 Bug Fixes

  • When uploading files to the app multiple times in a row, the button will correctly show the "next" action instead of the "cancel" one

When resetting the state,
we keep the candidateApps
(e.g. when the user successfully uploads a file or cancels the upload)
This is useful to avoid re-fetching
the candidate apps when the user starts a new upload
after a successful one
This is only persisted in the runtime state and not
in the persistent storage to avoid keeping the candidate apps
when the user closes the app
The candidate apps are fetched again
when the user opens the app after closing it
@acezard acezard requested review from Ldoppea and zatteo as code owners March 13, 2024 14:22
@Crash--
Copy link
Contributor

Crash-- commented Mar 13, 2024

Je comprends pas comment ça corrige le bug du cancel ? C'est le re-render qui posait problème ? Si oui, pourquoi ?

@acezard
Copy link
Contributor Author

acezard commented Mar 14, 2024

Je comprends pas comment ça corrige le bug du cancel ? C'est le re-render qui posait problème ? Si oui, pourquoi ?

non c'était à cause du reset initial state qui enlevait les candidateApps, et ces dernières n'étaient pas refetch à cause du flag useRef() (didCall.current). J'ai enlevé ce flag et j'utilise le state dans ce commit. Je ne refetch jamais les candidateApps dans le cycle de vie de l'application. A priori il n'y a pas de risque de cache obsolète car les candidateApps ne bougent pas fréquemment (voire jamais), et elles sont refetch au lancement de l'app (après kill).

ça enlève donc le bouton cancel et également l'erreur "no candidate apps available" qu'on avait vu

@Crash--
Copy link
Contributor

Crash-- commented Mar 14, 2024

Et pourquoi on passe par un hook custom et pas par un useQuery ? Pourquoi on dispatch des type: OsReceiveActionType.SetCandidateApps, ? un useQuery résout aussi les problèmes ? Ou un useDocument. Ça résoudrait les problèmes et ça permettrait d'avoir le temps réel de branché et la mise à jour des datas. Non ?

@acezard
Copy link
Contributor Author

acezard commented Mar 14, 2024

Et pourquoi on passe par un hook custom et pas par un useQuery ? Pourquoi on dispatch des type: OsReceiveActionType.SetCandidateApps, ? un useQuery résout aussi les problèmes ? Ou un useDocument. Ça résoudrait les problèmes et ça permettrait d'avoir le temps réel de branché et la mise à jour des datas. Non ?

Parce qu'on veut faire cette query uniquement si l'utilisateur a ouvert/wake l'application avec des fichiers (c'est une requête assez longue). On peut réorganiser avec <Query /> ou autre composant conditionnel cela dit

@Crash--
Copy link
Contributor

Crash-- commented Mar 14, 2024

On ne veut plus utiliser <Query. Si on peut pas mettre un hook car conditionnel, ça montrer un manque de "composabilité" de nos composants et donc un problème d'archi.

@acezard
Copy link
Contributor Author

acezard commented Mar 14, 2024

On ne veut plus utiliser <Query. Si on peut pas mettre un hook car conditionnel, ça montrer un manque de "composabilité" de nos composants et donc un problème d'archi.

on peut mettre le useQuery dans la vue en question (avec le bouton next), elle est déjà conditionné à apparaitre s'il y a des fichiers, elle peut gérer elle même la query

Copy link
Member

@Ldoppea Ldoppea left a comment

Choose a reason for hiding this comment

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

During today's daily we chose to merge this PR and handle the query fix in a future task

@acezard acezard merged commit 49fe327 into master Mar 26, 2024
1 check passed
@acezard acezard deleted the fix--persist-candidateApps-in-osreceive-state branch March 26, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants