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

Fixing broken cache in text filter #72

Open
wants to merge 5 commits into
base: development
Choose a base branch
from

Conversation

Sadaf-A
Copy link

@Sadaf-A Sadaf-A commented Jul 12, 2024

Resolves #66

@Sadaf-A Sadaf-A requested a review from 0x4007 as a code owner July 12, 2024 12:37
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Fix your formatting

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Actually this might be correct if you just moved the whole thing in the block. Although that doesn't seem optimal. Can you move only the relevant part inside the try catch please?

Telling CI to do a deploy so I can test now.

@ubiquity-os-deployer
Copy link

@0x4007
Copy link
Member

0x4007 commented Jul 12, 2024

As it turns out I can't login and test from my phone. Hope somebody else can take over!

@Sadaf-A
Copy link
Author

Sadaf-A commented Jul 12, 2024

@0x4007 how does it look now?

@0x4007
Copy link
Member

0x4007 commented Jul 12, 2024

I didn't test that it works but the code looks good.

@Sadaf-A
Copy link
Author

Sadaf-A commented Jul 12, 2024

@Keyrxng could you please provide your inputs here?

@Keyrxng
Copy link
Member

Keyrxng commented Jul 12, 2024

I put a very specific search query, so I expect it to only filter down to a single task. However it seems to be broken.

It could be related to an invalid issue preview in my cache, as you can see that the error was thrown.

@Sadaf-A as far as I understand the spec the following needs to be resolved:

  • When you search for an issue the preview box should be filled with the issue details not "No task found for preview id 2292677641" which is happening on the preview deployment as well as locally for me

so I expect it to only filter down to a single task.

It's unclear to me whether the spec aims to have the listed issues on the left become a single item if you enter a very specific term but the preview box should display similar details as when you manually click on the issue

P.S With PRs that change/fix UI problems remember to include a screenshot of that issue after it has been resolved

@Sadaf-A
Copy link
Author

Sadaf-A commented Jul 12, 2024

I put a very specific search query, so I expect it to only filter down to a single task. However it seems to be broken.

It could be related to an invalid issue preview in my cache, as you can see that the error was thrown.

@Sadaf-A as far as I understand the spec the following needs to be resolved:

  • When you search for an issue the preview box should be filled with the issue details not "No task found for preview id 2292677641" which is happening on the preview deployment as well as locally for me

so I expect it to only filter down to a single task.

It's unclear to me whether the spec aims to have the listed issues on the left become a single item if you enter a very specific term but the preview box should display similar details as when you manually click on the issue

P.S With PRs that change/fix UI problems remember to include a screenshot of that issue after it has been resolved

I wrapped the whole function responsible for rendering from cache with a try and catch block so if it threw an error it will render from network. If the error is still being thrown then it could be possible that the error is thrown when we fetch from the network

@Keyrxng
Copy link
Member

Keyrxng commented Jul 13, 2024

@Sadaf-A I had another look at things.

  • The list of issues should reduce down to one item, when the error modal is visible it does not remove items from the list.

  • The error box appears when you arrive to the page with a fresh cache and you try to search. If you refresh the page and try again the error will not appear.

  • I manually looked through the cached issue IDs (may have been after refresh, you should also check before too) and could not see the ID which is in the error box maybe you'll want to manually confirm this too

  • I'd focus on where the error is thrown from and work back from there debugging until you can resolve or elegantly handle it.

but the preview box should display similar details as when you manually click on the issue

Forgive me here, I mistook the error notification for the issue preview modal because I was using a different screen size. When you search toy "demo" you should see only one issue which you can select via keyboard nav or clicking and it'll render the preview modal.

I hope this is of more help to you, sorry if not.

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.

Text Filter Seems Broken
3 participants