ui-core Fix multiple definition fetch issue

Hello. I updated codes to show multiple items in Workspace when the API returns multiple terms/types. This will fix https://github.com/unisonweb/ui-core/issues/68

You can try it in the storybook with mocked API responses. However, I haven't tested it with actual UCM.

May 30 '24 18:05 yoching

With Storybook, you can try with these buttons Screenshot 2024-05-30 at 20 40 13

May 30 '24 18:05 yoching

I have updated the codebase, now it works to some extent. However, I found a bug.

After opening PositiveInt2, when you click the link to type PositiveInt2in the term declaration, it tries to fetch the definition infinitely somehow. I tried to debug it, but quite difficult. Were there similar issues before?

Jun 21 '24 14:06 yoching

@yoching thanks for taking this on! I haven't seen that infinite loop bug before. Any thoughts on how we might attempt to verify it on Share (I'm not sure where PositiveInt2 is from?)

Jun 21 '24 17:06 hojberg

I found it! It doesn't seem to loop for me on Share: https://share.unison-lang.org/@kristiannotari/psql/code/main/latest/terms/examples/API/types/PositiveInt2

Jun 21 '24 17:06 hojberg

@hojberg The infinite fetch happens after this change. I'm testing local-ui with ui-core on this branch.

Jun 22 '24 15:06 yoching

I found the reason for the loop request:

With new logic, it makes References of new items in workspaceItems based on the API response. A reference used for request (and loading state) is removed when handling FetchItemFinished. However, a reference in App.Model.page, which is set by UrlChanged event, keeps being the reference used for request, and in this condition below, it goes to else case, then sending the same request again. https://github.com/yoching/ui-core/blob/fix-multiple-definition/src/Code/Workspace.elm#L410-L412

Jun 22 '24 15:06 yoching

It seems to make sense to make references based on the API response for items in workspaceItems, as API might response with multiple items for one request (not handling this was the source of the original issue #68).

Jun 22 '24 16:06 yoching

One of the potential issues might be here in local-ui: https://github.com/unisonweb/unison-local-ui/blob/main/src/UnisonLocal/Page/CodePage.elm#L131-L137

Regardless of the Msg sent, it's calling updateSubpage, leading to the call of Workspace.open. It might be better to change the logic based on Msg received.

Jun 22 '24 16:06 yoching

This is where reference is constructed based on API response https://github.com/unisonweb/ui-core/pull/90#discussion_r1621271414

Jun 22 '24 16:06 yoching

Working on this issue, might take a bit more

Jul 06 '24 20:07 yoching

Ah yeah I've dealt with loops from updateSubpage, its not fun!

Jul 07 '24 20:07 hojberg

Fixed loop fetch issue with a different approach!

Aug 09 '24 15:08 yoching

I cleaned some decoding logic for clarity.

Also, can you check this PR on ui-core-scripts too? I've already been using this, but it's helpful to debug ui-core changes with local-ui. Thx! https://github.com/unisonweb/ui-core-scripts/pull/2

Aug 15 '24 13:08 yoching

@hojberg It seems UI.Switch is not used anymore, which triggers the Elm review error on CI. Is it ok to remove it from codebase?

Nov 22 '24 14:11 yoching

@yoching yeah I think thats good to remove. Sorry for sitting on this PR for so long. Lets get this passing and merged

Nov 22 '24 14:11 hojberg

Hello. I've removed UI.Switch. I think it's ready for review. I also have an open PR here: https://github.com/unisonweb/ui-core-scripts/pull/2, but it seems local-ui doesn't work with latest ui-core anymore?

Dec 06 '24 11:12 yoching

Thank you @yoching !! Sorry for taking so long to get this merged. local-ui is meant to continue to work with ui-core, though it will soon be replaced by https://github.com/unisonweb/ucm-desktop

Dec 12 '24 18:12 hojberg