[Bug]: (Windows) After new file/folder creation, directory tree is blank

thombruce commented 3 months ago

What happened?

From the root directory I attempted to created a new folder:

A toast informs me that the action succeeded; the toast text describes the correct location (with full path: C:/Users/...
No new folder appears to actually have been created
The directory tree is emptied and shows blank

From a subdirectory I created a new file:

A toast informs me that the action succeeded; the toast text describes the correct location (this time without the full path: Writing/myfile.todo)
The file was created in the appropriate location!
The directory tree is emptied again and shows blank

The above is true of folders created in subdirectories too.

I deleted the files/folders created to test this:

A toast informs me that the action succeeded
The files were moved to my recycle bin (correct behaviour)
The directory tree is emptied again and shows blank

Sounds like an issue with the root path, right?

The problem is most pronounced at root
Subsequent fetches of the directory tree are empty unless I refresh the page

Both of those facts are consistent with an incorrect value being obtained for the root directory.

Version

0.10.x (Default)

What browsers are you seeing the problem on?

No response

Relevant log output

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct

thombruce commented 3 months ago

A couple of relevant code pieces from the store:

const root = computed(() => tree.value.path)

fetchDirectory(root.value)

And an example of code used to create a folder:

createFolder(`${props.files.path.replace(`${root.value}/`, '')}/${newFolder.value}`)

Remember: This works in nested directories. It is root directory where the problem exists.

My first hunch is that this is the problem: props.files.path.replace(${root.value}/, '') See the / at the end of root.value? Yeah. I suspect when we're working in the root directory itself, this is not present... hence the replace fails; it simply does not match the path. Whereas when we are in a nested directory, of course the / is there, separating the root directory from the subdirectory.

This error was noticed in an alpha build of Toodles, by the way: v0.5.0-alpha.0

I only did this for Toodles, however, not for TNT. We really should release alphas of TNT as well before we're certain the code works on all target platforms (currently only Windows, officially).

thombruce commented 3 months ago

fe771008239755519ca32fdeaf5f208b0d8332cb fixes most of my problems.

An issue persists however with renaming files. Doesn't matter where the file/folder is (root or a subdirectory), renaming does not succeed.

The issue is slightly different now in that we don't get a toast announcing the rename either - suggests the initial rename promise is not resolving. Look into that.

thombruce commented 3 months ago

Pushing out what I think fixes the renaming issue: e12cc0afae7f77efbeef782d5c516ba39729de90

Didn't realise though... we have an issue with links - I can't navigate to documents; the pages are blank.

Scratch that! After quickly reviewing my changes, I think that commit fixes the linking issue but does not actually resolve the renaming problem.

Our renaming strategy is still this:

function rename() {
  const regex = new RegExp(`${name.value}$`)
  const newPath = path.value.replace(regex, newName.value)
  renameFile(path.value, newPath)
  path.value = newPath
  name.value = newName.value
  editing.value = false
}

We're generating the new path from just path.value, replacing the name part. path is a prop but what we're passing into it is also the path obtained from files.path attribute... so it's going to be the full path. Thus, the problem persists.

We could:

renameFile(path.value.replace(fullRootRegExp, ''), newPath.replace(fullRootRegExp, ''))

That should do the trick. It's consistent with what I'm doing elsewhere... It is SCREAMING at me to be refactored though; like this is a data concern and should be handled in the store, not across each different renderer component.

Plenty of other refactoring to do elsewhere too though, so I'm gonna kick that down the road and just do the simple thing for now.

thombruce commented 3 months ago

Accidentally published alpha release as v0.32.1-y.0 instead of -alpha.N. It's fine. Just noting the oddity here.

thombruce commented 3 months ago

I think this does fix the issue though: b61058b59052caf5857330f9bb9725fc6741a29b

Waiting on Toodles v0.5.0-alpha.3 to finish so that I can test a production-ready build. If it doesn't work, I'm stumped and I'm going to bed. If it does, I'll quickly publish the prod-ready versions.

thombruce commented 3 months ago

Links are restored but renaming remains a bust. Ah, well; this won't be resolved tonight then.

Any theories?

Well, we're still not getting a toast notification either, so the renaming just isn't succeeding in the first instance. The path being passed to the rename function must not exist, which means we're still passing it the wrong path.

This code remains consistent with what I'm doing elsewhere:

renameFile(path.value.replace(fullRootRegExp, ''), newPath.replace(fullRootRegExp, ''))

Maybe we're being inconsistent somewhere else. The store? tntApi or the Electron API? Some part of this is handling the current or new path in an unexpected way and it's causing the promise to silently fail.... Actually, let me check...

Nope, not silent. Here's an error:

Uncaught (in promise) Error: Error invoking remote method 'rename-file': Error: ENOENT: no such file or directory, rename 'C:\Users\thom\Documents\Notes\C:\Users\thom\Documents\Notes\zummy' -> 'C:\Users\thom\Documents\Notes\zummyRenamed'

Notice the doubling up of the path to be renamed. Notice the backslashes (may or may not be relevant) in the path to be renamed and the rename.

The doubling is the big issue here - that's what we're trying to strip. But I'd make the case that... we are stripping it, right? So is it being readded somewhere or is the regex incorrect actually?

Notice this too:

fs.rename(join(String(process.env.PORTABLE_EXECUTABLE_DIR || ""), path), name, function() {})

This is from the Electron-specific api.ts. For one, yes we are re-adding the directory - it's superfluous, but it's how we're doing this... What I specifically want to point out though is that we do that for path but not for name. On the rename function, we should be doing it for both! The arguments ought to be consistent.

Doesn't solve the actual issue, but is something that needs to be handled.

Gotta keep digging to find the stray double of the route though.

thombruce commented 3 months ago

Figured it out!

When in <script>, we need to get the value of our computed property like this:

fullRootRegExp.value

I know this! It's just easy to forget. This commit should fix the issue: 03fd81c93dc6b8f11ac9554498aa9dd1cb2b95cf

Getting very late though so that still will be something for me to test tomorrow, but it is running through the build now and will be ready by the time I am.

thombruce commented 3 months ago

Changes published.

thombruce / tnt