feat: implement file sharing from cozy-stack to external apps #1088
Conversation
acezard
commented
Dec 20, 2023
•
acezard
commented
- new cozy intent
- download file
- share file to external app
- display loading screen
acezard
force-pushed
the
feat--add-file-sharing-from-web
branch
from
f4a2aad
to
ac3cd14
Compare
acezard
changed the title
acezard
force-pushed
the
feat--add-file-sharing-from-web
branch
2 times, most recently
from
23eebf9
to
ed53600
Compare
Main functions with typing and unit test. Some refactoring will be applied. The method has been tested and does share files fetched from the stack
acezard
force-pushed
the
feat--add-file-sharing-from-web
branch
from
ed53600
to
c91b54b
Compare
Using blurring lib Have some issues with progressbar
Using OsReceive feature as a backbone
Error message and Toaster
acezard
force-pushed
the
feat--add-file-sharing-from-web
branch
from
b06dcca
to
6146949
Compare
Also update storybooy config a bit for better UI
acezard
force-pushed
the
feat--add-file-sharing-from-web
branch
from
77ebfe3
to
b622021
Compare
shareFiles.api wasn't used anymore the other file didn't have much justification for existing. using another file for the interface declaration instead
| // If the state gets populated with filesToShare, we fetch the files by ids and share them | ||
| // After that, we clear the state to avoid sharing the same files again if the user comes back to the flow | ||
| // We handle errors by clearing the state as well, it's paramount since the loading overlay will stay otherwise | ||
| useEffect(() => { | ||
| if (!client || state.filesToShare.length === 0) return | ||
|
|
||
| const fetchFilesByIdsAndShare = async (): Promise<void> => { | ||
| try { | ||
| await fetchFilesByIds(client, state.filesToShare, () => { | ||
| dispatch({ type: OsReceiveActionType.SetFilesToShare, payload: [] }) | ||
| }) | ||
| } catch (error) { | ||
| // Not really an error, the user just did not share the files | ||
| if (getErrorMessage(error) === 'User did not share') { | ||
| OsReceiveLogger.info('User did not share') | ||
|
|
||
| return dispatch({ | ||
| type: OsReceiveActionType.SetFilesToShare, | ||
| payload: [] | ||
| }) | ||
| } | ||
|
|
||
| // Here we encountered a real error, so we clear the state and display an error message | ||
| return handleError( | ||
| t('errors.shareFiles', { count: state.filesToShare.length }), | ||
| () => { | ||
| OsReceiveLogger.error( | ||
| 'Global failure in files to share, clearing state', | ||
| error | ||
| ) | ||
|
|
||
| dispatch({ type: OsReceiveActionType.SetFilesToShare, payload: [] }) | ||
| } | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| safePromise(fetchFilesByIdsAndShare)() | ||
| }), | ||
| [client, state.filesToShare.length] |
There was a problem hiding this comment.
I'm not sure that we should put the share feature that is about sending files "out" of the app, inside OSReceive feature that is about sending files "in" the app.
The features names are confusing enough, I think we should avoid mixing those concepts
There was a problem hiding this comment.
Well I think it's linked by the fact it's about interacting with files to the OS. I felt creating a whole new domain was overkill. But you're right that the naming is bad. What do you think about keeping all that in a single folder but adapting naming?
I think an important thing for me here was to be able to re-use the structure of the file receiving feature (provider, state reducer, models...). It definitely allowed me to implement this feature much faster than from scratch.
There was a problem hiding this comment.
I think I need a bit more explanation to understand why you implemented this faster with this.
There seems to be no overlap between the 2 features except that for the dispatch({ type: OsReceiveActionType part, so you would have to implement the same things if in a different feature?
Maybe it is too late to question this part, so I guess you can merge it as is.
But let me write a proposal so you can take it or leave it (I leave you the choice):
My understanding of the feature is that it has only 3 states: IDLE, Error and Sending:
IDLEis not a state that needs to be described as this is juste the application running normally- For now it is here because OsReceive is a stateful component
Errorstate is not handled in this feature as you previously extracted it in the ErrorProvider- I find this part great as every feature may need to display an error
Sendingstate is here just to display the newLoadingOverlaycomponent- Maybe we can make a LoadingProvider like we did for the ErrorProvider?
- Every feature may need to display a LoadingOverlay. This is already the case of Konnectors that sometime display an equivalent screen.
If we have an ErrorProvider and a LoadingProvider, then we don't need to store any state for the share feature as it would only need to call something like showLoadingOverlay() then download/share the files in a single method.
The only cons is that the share method is outside of react, so we would need to create an event handler the same way we did for the OAuthLimit dialog or the FlagshipUI and this is not really in the React philosophy.
But with this I'm convinced the entire feature would fit in one single file equivalent to shareFileService.ts with no side effect on OsReceive part (but you would have to create the LoadingOverlay component anyway)
acezard
force-pushed
the
feat--add-file-sharing-from-web
branch
from
1437545
to
5402e7b
Compare
