fix: wrong permissions directory#6172
Conversation
|
Thank you for opening your first PR! 🎉 We would like to thank you already for your contribution. If everything checks out, we'll make sure to review the PR as soon as possible and give feedback. Please note a few things:
We are glad for every improvement that makes its way into the app, so we look forward to collaborating on merging this PR. |
|
This looks fine in principle! There's just a merge conflict now because I merged the dot-file PR just now. But that should make the PR a bit cleaner I guess, since it only involves a few try/catch instances, no? Aside of that, I'm curious — does this finally re-enable the "DirNotFound" descriptor functionality in the file tree? I implemented that years ago due to a request by someone who had their workspaces on a USB key and who sometimes forgot to plug it in before starting Zettlr. At some point, that got lost (and I never really understood why)… |
Resolved the merge conflict. Indeed, it does bring back the DirNotFound descriptor. |
That sounds like another PR at a different time. It looks good, though! Merging… |
| return await this.getAnyDirectoryDescriptor(absPath) | ||
| } catch (err: any) { | ||
| if (err.code === 'EACCES' || err.code === 'EPERM') { | ||
| return FSALDir.getDirNotFoundDescriptor(absPath) |
There was a problem hiding this comment.
Could this not be moved to the getAnyDirectoryDescriptor? I think it would be more robust to put the check and fallback within that function
| const ignoreDotFiles = !files.dotFiles.showInFilemanager && !files.dotFiles.showInSidebar | ||
|
|
||
| try { | ||
| const contents = (await fs.readdir(directoryPath, { withFileTypes: true, recursive: true })) |
There was a problem hiding this comment.
Is there a reason you reverted this change, removing { recursive: true }?
| ) | ||
| return [ directoryPath, ...contents.flat() ] | ||
| } catch (err: unknown) { | ||
| const code = err instanceof Error ? (err as NodeJS.ErrnoException).code : undefined |
There was a problem hiding this comment.
This is redundant with the below logging and return statement
| try { | ||
| const children = await fs.readdir(absPath, { withFileTypes: true }) | ||
| return await Promise.all( | ||
| children | ||
| .filter(dirent => { | ||
| return (!ignorePath(dirent.name, ignoreDotFiles) && (dirent.isFile() || dirent.isDirectory())) | ||
| }) | ||
| .map(dirent => { | ||
| const childPath = path.join(absPath, dirent.name) | ||
| return this.getDescriptorFor(childPath) | ||
| }) | ||
| ) | ||
| children = await fs.readdir(absPath, { withFileTypes: true }) | ||
|
|
||
| const childPaths = children | ||
| .filter(dirent => !ignorePath(dirent.name, ignoreDotFiles) && (dirent.isFile() || dirent.isDirectory())) | ||
| .map(dirent => path.join(absPath, dirent.name)) | ||
|
|
||
| const results = await Promise.allSettled(childPaths.map(p => this.getDescriptorFor(p))) | ||
| return results | ||
| .filter((r): r is PromiseFulfilledResult<AnyDescriptor> => r.status === 'fulfilled') | ||
| .map(r => r.value) |
There was a problem hiding this comment.
What's the motivation for this refactor? To me it just seems to make things more verbose without any extra guard or tests
|
Sigh, I never actually submitted my comments after writing them. Well, there they are now |
|
My only real concern is the reversion of the Otherwise, the other changes are fine to me |
Description
Fixes an issue with zettlr crashing upon startup because of an unhandled exception when accessing a directory that current use doesn't have permissions towards.
Changes
List directories that aren't accessible as
dirNotFoundFlag-edAdditional information
Fixes #6162
Tested on: openSUSE Tumbleweed, PopOS