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.

Sign up for GitHub

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

Jump to bottom

[#2016] Remove hash symbol from URL when decoding hash #2086

Merged
merged 9 commits into from
Jan 31, 2024
Merged

[#2016] Remove hash symbol from URL when decoding hash #2086

merged 9 commits into from
Jan 31, 2024

Conversation

jonasongg
Copy link
Contributor

@jonasongg jonasongg commented Jan 20, 2024 •
edited
Loading

Fixes #2016

Proposed commit message

Fix bug where some params were not retained after refresh

The issue pertains to breakdown of file types disappearing after a
refresh, but it seems to apply to all params that were last in the URL
due to symbols not being filtered out correctly.

Let's fix this issue to make sure the state of the report stays the
same when refreshing.

Other information

The bug seems to have been caused by the URL containing #/ at the end, which would cause the last param to not be parsed correctly. I couldn't definitively find what caused the anchor to appear in the URL, so I filtered it out of the URL when decoding hashParams. Let me know if this behaviour should be changed!

Regarding the Cypress tests, #2017 seems to be outdated/some of the tests don't seem to be checking for the correct thing, at least on my machine. If I should investigate this further in anticipation of its being merged, let me know as well!

@jonasongg jonasongg requested a review from a team January 20, 2024 09:41
@sopa301
Copy link
Contributor

sopa301 commented Jan 20, 2024 •
edited
Loading

Googling a bit suggests that this appending of #/ is caused by Vue's Hash Mode, which appends a # followed by the route / (the default one) at the end of the URL. This could be a regression from the implementation of Vue Router.

I tested this using the 2324S1 CS2103T RepoSense dashboard using the default view. Some evidence for this hypothesis:

  • Using #/widget gives a different view of the webpage.
  • Using #/test gives a blank webpage

That said, the current fix is good enough to make it work. I propose 2 possible paths in the future:

  1. Rewrite the code to ensure Vue Router's internal routing is finished and the URL is fixed before it is processed by api.ts (not recommended due to complexity, but could become necessary if we want to keep Vue router from coupling with the internal logic)
  2. Refactor api.ts to capture this consideration more distinctly.

@sopa301 sopa301 mentioned this pull request Jan 22, 2024
Closed
@jonasongg
Copy link
Contributor Author

@sopa301 Thanks for the suggestions!

Just wanted to link these related issues here since it seems they are caused by this same problem: #2058 #1987

sopa301
sopa301 approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@sopa301 sopa301 left a comment

Choose a reason for hiding this comment

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

LGTM. As commented above, this is sufficient to fix the bug, but may warrant further refactoring.

@sopa301 sopa301 requested a review from a team January 22, 2024 16:53
ckcherry23
ckcherry23 reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

Thanks for your inputs @jonasongg and @sopa301. I think this temporary fix is a reasonable solution for now as it resolves the current issue without introducing new issues. We can switch to the Vue Router HTML5 mode in the future.

@jonasongg Since this fix is somewhat unconventional, can you add a comment in the code explaining the rationale behind the change? This will help future developers understand the context and reasoning behind this decision.

Regarding the Cypress tests, we should ideally be adding a test case for the bug fixed in this PR itself. Let us wait to see the progress of PR #2017 over the next few days and make a call over the changes needed.

MarcusTXK
MarcusTXK reviewed Jan 23, 2024
View reviewed changes
Copy link
Member

@MarcusTXK MarcusTXK left a comment

Choose a reason for hiding this comment

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

Nice fix. Agree with @ckcherry23 that a comment to explain the logic for future developers would be useful.

Regarding the tests, I think perhaps @jonasongg could reference the code written in #2017 and add the updated tests, then co-author credit could be added to @nseah21.

@jonasongg
Copy link
Contributor Author

@ckcherry23 @MarcusTXK Thanks for the reviews! Just to confirm, I should add the codeView_renderFilterHash.cy.js file from #2017 here and add tests that test this new expected behaviour?

@MarcusTXK MarcusTXK mentioned this pull request Jan 23, 2024
Merged
@ckcherry23
Copy link
Member

@jonasongg #2017 was just merged. You can look into what additions/changes are required on the tests now, thanks!

@MarcusTXK
Copy link
Member

@jonasongg After discussion, we decided to merge in #2017, so you can work on the merged test directly on your branch.

@jonasongg
Copy link
Contributor Author

Just added the comment and some test cases! For checked file types, I added some tests to make sure it was testing the last file type to appear in the URL (yml) as that was what was being affected by the bug. Also added some non exhaustive test cases for the code panel options including one for authorshipFileGlob that is most likely the cause for #2058 and #1987. Let me know if I should add more/change anything!

@ckcherry23 ckcherry23 requested review from ckcherry23 and a team January 23, 2024 17:21
@sopa301 sopa301 mentioned this pull request Jan 24, 2024
Open
ckcherry23
ckcherry23 approved these changes Jan 28, 2024
View reviewed changes
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

LGTM! Just have a few minor comments

frontend/cypress/tests/codeView/codeView_renderFilterHash.cy.js Show resolved Hide resolved
frontend/cypress/tests/codeView/codeView_renderFilterHash.cy.js Show resolved Hide resolved
@ckcherry23 ckcherry23 requested review from MarcusTXK and a team January 28, 2024 07:59
@jonasongg
Copy link
Contributor Author

Should be clarified now!

MarcusTXK
MarcusTXK approved these changes Jan 29, 2024
View reviewed changes
Copy link
Member

@MarcusTXK MarcusTXK left a comment

Choose a reason for hiding this comment

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

LGTM, I think you did a great job

@ckcherry23 ckcherry23 merged commit 0f682a0 into reposense:master Jan 31, 2024
10 checks passed
@github-actions GitHub Actions
Copy link
Contributor

The following links are for previewing this pull request:

@jonasongg jonasongg mentioned this pull request Feb 13, 2024
Closed
@ckcherry23 ckcherry23 mentioned this pull request Mar 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcusTXK MarcusTXK MarcusTXK approved these changes

@ckcherry23 ckcherry23 ckcherry23 approved these changes

@sopa301 sopa301 sopa301 approved these changes

Assignees
No one assigned
Labels
s.ToMerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Breakdown by file type not retained after refresh
5 participants
@jonasongg @sopa301 @ckcherry23 @MarcusTXK @vvidday