Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multiple definition fetch issue #90

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

yoching
Copy link
Contributor

@yoching yoching commented May 30, 2024

Hello. I updated codes to show multiple items in Workspace when the API returns multiple terms/types. This will fix #68

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

Comment on lines 869 to 870
ref =
Reference.fromFQN Reference.TypeReference d.name
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this part, ref is constructed based on bestTypeName (d.name) of the API response.
I'm not sure if this works in all cases. Do you think this is fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't work with actual projects. I'll check and fix it soon

makeTerm ( hash_, d ) =
let
ref =
Reference.fromFQN Reference.TermReference d.name
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. The ref is constructed based on bestTermName of the API response

@yoching
Copy link
Contributor Author

yoching commented May 30, 2024

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

@yoching
Copy link
Contributor Author

yoching commented Jun 21, 2024

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?

Screenshot 2024-06-21 at 16 33 18

@hojberg
Copy link
Member

hojberg commented Jun 21, 2024

@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?)

@hojberg
Copy link
Member

hojberg commented Jun 21, 2024

@@ -542,7 +562,7 @@ fetchDefinition config ref =
in
endpoint
|> config.toApiEndpoint
|> HttpApi.toRequest (WorkspaceItem.decodeItem ref) (FetchItemFinished ref)
|> HttpApi.toRequest WorkspaceItem.decodeList (FetchItemFinished ref)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding a comment somewhere about why this sometimes returns more than 1 item.

@yoching
Copy link
Contributor Author

yoching commented Jun 22, 2024

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

@yoching
Copy link
Contributor Author

yoching commented Jun 22, 2024

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

Screenshot 2024-06-22 at 18 00 20

@yoching
Copy link
Contributor Author

yoching commented Jun 22, 2024

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).

@yoching
Copy link
Contributor Author

yoching commented Jun 22, 2024

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.

@yoching
Copy link
Contributor Author

yoching commented Jun 22, 2024

This is where reference is constructed based on API response
#90 (comment)

@yoching
Copy link
Contributor Author

yoching commented Jul 6, 2024

Working on this issue, might take a bit more

@hojberg
Copy link
Member

hojberg commented Jul 7, 2024

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

@yoching
Copy link
Contributor Author

yoching commented Aug 9, 2024

Fixed loop fetch issue with a different approach!

@yoching
Copy link
Contributor Author

yoching commented Aug 15, 2024

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!
unisonweb/ui-core-scripts#2

@yoching
Copy link
Contributor Author

yoching commented Nov 22, 2024

@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?

@hojberg
Copy link
Member

hojberg commented Nov 22, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When getDefinition returns more than 1 result, only the first is displayed
2 participants