fix: path resolution for non-existent files when using :org-mode/insert-file-link

[FrederickGeek8]

This PR fixes file path resolution with using :org-mode/insert-file-link when a file does not already exist on-disk (for both new pages and aliases). As a side-effect, this PR fixes #9342.

Problem Statement

When using :org-mode/insert-file-link in Logseq, linking to a page that does not exist yet creates a file:// path to that page that does not make sense. This not only breaks Org-Roam's file link support, but also prevents Aliases from working from within Logseq.

Expected:
From a page in journals/:
[[New Page]] -> [[../pages/new page.org]]

Actual:
From a page in journals/
[[New Page]] -> [[file:..//Users/user/logseq-home/pages/new page.org][New Page]]

This problem not only exists for files that have yet to be created, but also Aliases that have already been defined. Note that aliases are, in a way, a type of missing file.

Isolation & Explanation

There are two errors that I see here

1. We are resolving what should be a relative path to an absolute path. This breaks platform-independence of the Logseq graph. This would become a problem when copying graphs between computers, but I imagine it is a problem for anyone who syncs their graphs between different devices.
2. Even if we wanted to resolve to absolute paths, what is specified here is an incorrect format. file:..//Users/user/ should instead read file:/Users/user/. I'm honestly not sure why Logseq handles creation of files using these paths file, because every application I know of fails to parse this strange url. That being said, it seems like there is a lot of work that could be done on path resolution in the code base, so there is probably some weird stuff there that makes it work.

The problem that causes the absolute path resolution lies in platform-specific code for handling creating file paths for non-existent pages. This code is platform specific -- the bug lies in Electron and Mobile apps. When running the code through unit tests, you will not see this file-path resolution bug.

Link to offending lines:

logseq/src/main/frontend/handler/page.cljs

Line 697 in b6382d5

(if (or (util/electron?) (mobile-util/native-platform?))

In my manual testing, the both the mobile (iOS) and Electron app are able to handle relative paths when trying to create a new page. Thus, it seems to me that this platform-specific handling is not necessary. Even if there was special sauce there, I think the proper resolution should happen internally in the creation API itself, not in the file reference that's in the text file.

For issue (2), the absolute path is being resolved to a file:// path incorrectly because the function get-relative-path will append a .. to the path, even if the first character is a /. When the path is parsed by this function, the first element will be "", since we are splitting the path string based on /. A simple check to see if the first split character is "" resolves this issue.

Test Cases

I wrote unit tests for both situations here. My get-relative-path tests are a little sparse, but the major change there was just related to the incorrectly prepended .. to absolute paths.

More robust/complete units tests are provided for the get-ref-text function.

I did not add E2E tests, but I don't think it would be unreasonable to add. That being said, from my brief look at the existing E2E tests they seem to be in a bit of an immature state and it might be a bit of work to set everything up that's necessary for them.

Besides the unit tests, I tested proper resolution and subsequent creation of files via the correct relative link in Electron on macOS and the app on iOS. In that testing, the alias issues in #9342 seems to be resolved too.

Future Improvements

1. While units tests have been defined for the mobile platform and the "native" testing environment, electron is not tested through the unit tests. While it works in practice in the GUI, trying to run the unit tests with the electron environment mocked throws a Error: No protocol method IDeref.-deref defined for type null: error. I don't think this is related to my changes. The test function is in my commit, but it is commented out. In case there is a better solution for unit tests relevant to electron in the future, it should be able to be uncommented.
2. I have never worked with this codebase before, nor have I worked with Clojure before. I couldn't figure out some of the DB structure, and I had to write this "hack" to define :block/page when the page is pulled from the db. It's only used for the unit tests though.

;; A hacky way of setting :block/page. I'm not sure why it isn't set by default
;; in this test case
(defn- add-block-page
  [block]
  (assoc block :block/page block))

3. E2E tests might want to be written in the future. That being said, E2E tests seem to be in early stages for this project.
4. I had some thoughts (not specifically relevant to this PR) regarding how Aliases are handled when the language is set to Org in Logseq. I post it in the issue relevant to aliases. When :org-mode/insert-file-link is on links for page aliases are broken #9342 (comment)
5. This isn't really related to my PR, but rather my experience in developing it: something that made my testing extremely hard on iOS (besides the constant crashing in debug mode) was related to (a) the lack of global settings so that I can enable Org for all future graphs and (b) the inability for Logseq to rename contents.md to contents.org, even if the graph is fresh and I have not personally touched contents.md. While this is annoying on desktop, it is a punch in the gut on iOS. I was working over the (classically broken) iCloud Drive so I could inspect files from macOS, and so I was unable to access the Logseq files through Files on iOS, and through macOS I could only access that folder through the terminal. The contents.md problem cause me a bunch of headaches as a result since I could only remove it from the Terminal on macOS...

Thanks everyone!

[FrederickGeek8]

Should I be keeping this up-to-date with the base branch? Or should I just wait for a code review?

[FrederickGeek8]

I'm marking this PR as a draft now until I have time to understand the failing tests better.

Even just running yarn test on my system (something that somehow I neglected to do before) is throwing an error related to a js function utilizing the undefined document variable. I do not quite understand this variable and how the tests are utilizing js at all. It's not even clear to be what test is actually throwing this error or utilizing the get-first-block-by-id function that is failing... Trying to invoke specific namespaces seems to work, unless I invoke the full handler namespace... Sigh.

Testing frontend.handler.plugin-config-test
/Users/fred/Development/logseq/node_modules/mldoc/index.js:961
throw a}function
^

ReferenceError: document is not defined
    at frontend$util$get_first_block_by_id (/Users/fred/Development/logseq/.shadow-cljs/builds/test/dev/out/cljs-runtime/frontend/util.cljc:1013:30)
    at frontend$handler$editor$property$get_edit_input_id_with_block_id (/Users/fred/Development/logseq/.shadow-cljs/builds/test/dev/out/cljs-runtime/frontend/handler/editor/property.cljs:21:26)
    at Function.frontend.handler.editor.property/edit-block! [as cljs$core$IFn$_invoke$arity$4] (/Users/fred/Development/logseq/.shadow-cljs/builds/test/dev/out/cljs-runtime/frontend/handler/editor/property.cljs:50:32)
    at frontend.handler.editor.property/edit-block! (/Users/fred/Development/logseq/.shadow-cljs/builds/test/dev/out/cljs-runtime/frontend/handler/editor/property.cljs:38:1)
    at Function.frontend.handler.editor.property/edit-block! [as cljs$core$IFn$_invoke$arity$3] (/Users/fred/Development/logseq/.shadow-cljs/builds/test/dev/out/cljs-runtime/frontend/handler/editor/property.cljs:40:5)
    at frontend.handler.editor/edit-block! (/Users/fred/Development/logseq/.shadow-cljs/builds/test/dev/out/cljs-runtime/frontend/handler/editor/property.cljs:38:1)
    at Timeout._onTimeout (/Users/fred/Development/logseq/.shadow-cljs/builds/test/dev/out/cljs-runtime/frontend/handler/editor.cljs:645:34)
    at listOnTimeout (node:internal/timers:569:17)
    at processTimers (node:internal/timers:512:7)

If anyone has the time, I'd appreciate some help...