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

Bug: links does not show the page and redirect to the react props names #2185

Closed
2 tasks done
a0m0rajab opened this issue Nov 19, 2023 · 12 comments · Fixed by #2204
Closed
2 tasks done

Bug: links does not show the page and redirect to the react props names #2185

a0m0rajab opened this issue Nov 19, 2023 · 12 comments · Fixed by #2204
Assignees
Labels
🐛 bug Something isn't working

Comments

@a0m0rajab
Copy link
Contributor

Describe the bug

When I tried to use the link from this issue: #2177
I was redirected to https://app.opensauced.pizza/[pageId]/[toolName]/filter/[...selectedFilter]?range=30
Instead of: https://app.opensauced.pizza/ai/dashboard/filter/top-100-repos?range=30

Steps to reproduce

  1. go to app.opensauced.pizza/ai/dashboard/filter/top-100-repos?range=30
  2. Tada 🎉

Browsers

Chrome

Additional context (Is this in dev or production?)

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct

Contributing Docs

  • I agree to follow this project's Contribution Docs
@a0m0rajab a0m0rajab added 🐛 bug Something isn't working 👀 needs triage labels Nov 19, 2023
Copy link
Contributor

Thanks for the issue, our team will look into it as soon as possible! If you would like to work on this issue, please wait for us to decide if it's ready. The issue will be ready to work on once we remove the "needs triage" label.

To claim an issue that does not have the "needs triage" label, please leave a comment that says ".take". If you have any questions, please reach out to us on Discord or follow up on the issue itself.

For full info on how to contribute, please check out our contributors guide.

@nickytonline nickytonline added core team work Work that the OpenSauced core team takes on and removed 👀 needs triage labels Nov 21, 2023
@nickytonline
Copy link
Member

I have a feeling the info is not being read from the URL, e.g. language.

@a0m0rajab
Copy link
Contributor Author

a0m0rajab commented Nov 21, 2023 via email

@nickytonline
Copy link
Member

I looked into this a bit and it appears to be the ?range=30 that causes it to behave the way the bug is described. However, if you navigate to https://app.opensauced.pizza/javascript/dashboard/filter/recent without the querystring ?range=30, it's all good.

This is probably related to changes in #2047.

@a0m0rajab
Copy link
Contributor Author

Could I work on this? I would like to debug this.

just trying if .take works.

Copy link
Contributor

The issue you are trying to assign yourself is blocked until it can be triaged or by another label on the issue.

@nickytonline nickytonline removed the core team work Work that the OpenSauced core team takes on label Nov 24, 2023
@nickytonline
Copy link
Member

nickytonline commented Nov 24, 2023

I've assigned it to you @a0m0rajab. @OgDev-01 worked on #2047 and can probably provide some insight.

@a0m0rajab
Copy link
Contributor Author

Thanks for that, I will check this on live stream and connect with him if needed

@a0m0rajab
Copy link
Contributor Author

Dev notes:
When I opened app.opensauced.pizza/javascript/dashboard/filter/recent it just gave the wrong names then get back to the normal one. I think it's a useEffect issue.

@JabSYsEmb
Copy link

I just found the function call that causing in the redirecting after the page being rendered, I share it here for further investigation
B3sQK0z

@a0m0rajab
Copy link
Contributor Author

a0m0rajab commented Nov 24, 2023

Thanks @JabSYsEmb for checking this, if you could find the related code in the dev it would be great, I could find that.

As for my investigation today:

The issue is happening in the next function, if we remove the merge logic it will work well.

function setQueryParams(params: Record<string, string>, paramsToRemove: Array<string> = [], pathname?: string) {
const presentQueryParams = deleteKeys(Router.query, paramsToRemove);
// Merge final query and set URL
Router.push({
pathname: pathname || Router.pathname,
query: {
...presentQueryParams,
...params,
},
});
}

@a0m0rajab
Copy link
Contributor Author

I found the issue, I think it was due the prerendering since the next provides and empty query object.
ref: https://nextjs.org/docs/pages/api-reference/functions/use-router

I will open a PR in few minutes.

Another few issues I noticed while working on this, I will check them later and open an issue for them:

  • the tool selector is showing an empty page if no tool were selected: related code case "Dashboard":
  • There is a warning related to style when you open the app in the debugger:
Warning: Updating a style property during rerender (overflowY) when a conflicting property is set (overflow) can lead to styling bugs. To avoid this, don't mix shorthand and non-shorthand properties for the same value; instead, replace the shorthand with separate values.
  • the name of the range selector should be limit to set the information
  • Range is not setting in the page it's just showing 10 per page always.
  • when you click on showing 10 per page it will break the layout a bit
  • when limit/range is selected this does not appear on the button in the UI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants