fix(files): verify files are still accessible before downloading #54313
Conversation
|
/backport to stable31 |
|
/backport to stable30 |
| // Try to reload the current directory to update the view | ||
| const directory = getCurrentDirectory(view, dir)! | ||
| emit('files:node:updated', directory) |
There was a problem hiding this comment.
Feels weird to have this in a download action - meaning all the logic to get the directory. This smells like duplication. We rather should do this properly from either the files list or the store.
But in this case why not just remove that one file, there you do not need any logic not related to downloading here (files:node:deleted).
There was a problem hiding this comment.
But in this case why not just remove that one file, there you do not need any logic not related to downloading here (
files:node:deleted).
For the single action, sure, it make sense! 👍
Regarding the batch one, it's a bit frustrating but yeah, we need to reload the full directory as we don't know which object failed
There was a problem hiding this comment.
meaning all the logic to get the directory. This smells like duplication. We rather should do this properly from either the files list or the store.
Ideally, the directory should be part of the action exec parametters, we discussed it in the past and still haven't done anything with it 🙈
There was a problem hiding this comment.
But that would not really help here as then you still need to fetch the new content.
What I meant is to get rid of all non download related logic, so the only thing that would be needed here is call a trigger for reloading - instead of needing to fetch the directory in unrelated places.
Because currently it relys on the side effect, that if the current directory is changed the content is reloaded, maybe we should somehow provide the fetchContent method (similar like we provide it to the bread crumbs it could be an files:list:reload event).
There was a problem hiding this comment.
Don't get me wrong, I 100% agree, the way we trigger a reload is not great.
Luckily it's still used relatively sparingly, but imho the issue is needing that event in the first place :)
I'd be happy to sit and discuss real solutions on how to avoid the need of such reloads.
skjnldsv
force-pushed
the
fix/download-action
branch
from
f9487ce to
560be2c
Compare
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
For a fix this is fine I guess - as all other solutions would need new events.
skjnldsv
force-pushed
the
fix/download-action
branch
3 times, most recently
from
bc9464d to
bb82dd4
Compare
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
AndyScherzinger
force-pushed
the
fix/download-action
branch
from
bb82dd4 to
6fb7f7f
Compare
|
/compile |
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Summary
If a file have been removed or a share is not accessible anymore, the server will still try to download an the browser will sometimes download an empty index.html
This ensure we show a nice error and refresh the list if something goes wrong
Checklist