search

edrlab / thorium-reader

A cross platform desktop reading app, based on the Readium Desktop toolkit
https://www.edrlab.org/software/thorium-reader/
BSD 3-Clause "New" or "Revised" License
1.68k stars 148 forks source link

delete single bookmark in list (while in edit mode or not) causes crash (works fine with annotations editor) #2329

Closed danielweck closed 2 months ago

danielweck commented 2 months ago
Screenshot 2024-05-23 at 14 40 59
arthur-lemeur commented 2 months ago

I can't reproduce this bug. On my end it works fine either with the mouse or with the keyboard

danielweck commented 2 months ago

To reproduce, make sure there is only a single bookmark in the list. No need to enter edit mode (actually), simply press the delete icon

arthur-lemeur commented 2 months ago

It seems the problem comes from " dispatch(readerActions.bookmark.pop.build(bookmark));" (ReaderMenu.tsx, line 678). If you comment it there is no error but I don't know the cause of it.. @panaC can you have a look ? But when I click the delete bookmark icon in the reader header to delete a bookmark (when there is only one in the list) it works fine even if it the the same "dispatch" that's being called..

panaC commented 2 months ago

culprit :

https://github.com/edrlab/thorium-reader/blob/daebd059121cc0fe10dfcb7eb8794d15d245f9ba/src/renderer/reader/components/ReaderMenu.tsx#L879C1-L881C6

panaC commented 2 months ago

same here :

https://github.com/edrlab/thorium-reader/blob/daebd059121cc0fe10dfcb7eb8794d15d245f9ba/src/renderer/reader/components/ReaderMenu.tsx#L568C1-L570C6

danielweck commented 2 months ago

if (!r2Publication || !annotationsQueue)

...this should be

if (!r2Publication || !annotationsQueue?.length)

danielweck commented 2 months ago

just need to hoist conditional early render() exit AFTER all hooks called?

danielweck commented 2 months ago

https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level