search

c3founder / ccc-roam-pdf-2

4 stars 1 forks source link

commenting on highlights does not work #5

Open iff opened 2 years ago

iff commented 2 years ago

I did not dive into the code but it seems like commenting on a highlight fails if no "Display Name" (I guess that is :edit/user?) is set for the graph? Maybe falling back to "Anonymous" would be an option?

20359eb3-467d-4d54-8638-62b93ecb501c:1218 Uncaught (in promise) TypeError: Cannot read properties of null (reading ':edit/user')
    at getLastEditedUser (20359eb3-467d-4d54-8638-62b93ecb501c:1218:24)
    at 20359eb3-467d-4d54-8638-62b93ecb501c:564:36
    at Array.map (<anonymous>)
    at commentHighlight (20359eb3-467d-4d54-8638-62b93ecb501c:563:30)

Should be reproducible on a new graph (both Browser and Electron App, tested only on Linux) without setting the "Display Name" in the settings.

Even when testing in a graph where the Display Name is set I get an error when commenting:

main.js:9771 no component
App.js:426 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'getReplies')
    at App.js:426:39
    at s (main.f5ae81af.chunk.js:1:2552)
    at Generator._invoke (main.f5ae81af.chunk.js:1:2305)
    at Generator.next (main.f5ae81af.chunk.js:1:2911)
    at r (asyncToGenerator.js:3:1)
    at l (asyncToGenerator.js:25:1)
    at asyncToGenerator.js:32:1
    at new Promise (<anonymous>)
    at asyncToGenerator.js:21:1
    at App.js:66:12

Not sure if I'm doing something wrong, but commenting does not work on all in my graphs. If it helps I can try to record a video..

c3founder commented 2 years ago

There was a bug in roam. The display name was not persistent. https://roamresearch.slack.com/archives/CN5MK4D2M/p1666158544341889

Now if you "check for update" and change your display name once (and then revert it if you want) it should fix it.

But I'm going to add the "Anonymous" option.

iff commented 2 years ago

Thanks! (I posted to bug-reports a week ago: https://roamresearch.slack.com/archives/CN2L1UUHY/p1665074596738059).

Could you reproduce the second issue? I still get that even if I install the extension in a fresh graph and set the "Display Name"..

c3founder commented 2 years ago

Have you tested it today? Because for some reason before the roam's fix even with a Display Name it was not working for me either.

iff commented 2 years ago

Yep, just after the fix was deployed. Tested on a new graph with a fresh installation of the plugin and on an existing graph.