Skip to content

Conversation

@mpabarca
Copy link
Contributor

@mpabarca mpabarca commented Oct 4, 2024

This PR resolves the issue of outdated or inconsistent project data being displayed when loading a project’s details. Previously, error messages such as "Page not found" or "The selected branch or tag was not found" would appear prematurely while data was still being refreshed. The new approach introduces a refreshed state, ensuring that the loading screen remains until the updated project data is fully available.

Additionally, we've added logic to compare new project data with the existing one using deep equality checks to prevent unnecessary updates, further improving performance and reliability.

Loading screens

Screenshot 2024-10-07 at 15 36 15 Screenshot 2024-10-07 at 15 36 05 Screenshot 2024-10-07 at 15 35 40 Screenshot 2024-10-07 at 15 35 07

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@mpabarca mpabarca self-assigned this Oct 4, 2024
@mpabarca mpabarca changed the base branch from bugfix/delays-error-messages-until-data-refreshed to develop October 4, 2024 14:33
Copy link
Contributor

@simonbs simonbs left a comment

Choose a reason for hiding this comment

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

This appears to solve the original issue -- that's amazing! 🙌 However, it also seems to introduce the two issues shown below.

Besides the two issues below, I've also added a small comment that describes a concern about the changes 😊

1. Missing skeleton in header while loading

As shown in the screenshot below, it seems that the skeleton is missing from the header while refreshing projects. That is, we're no longer showing placeholders for the branches, files, and the edit button.

@mpabarca Can you add the skeleton to this page again?

loading

2. Header skeleton shown on "Not Found" page

The skeleton that isn't shown in issue no. 1 is, however, displayed on the "Not Found" page, where it doesn't serve any purpose.

@mpabarca Can you remove it from this page?

project-not-found

@mpabarca
Copy link
Contributor Author

mpabarca commented Oct 7, 2024

This appears to solve the original issue -- that's amazing! 🙌 However, it also seems to introduce the two issues shown below.

Besides the two issues below, I've also added a small comment that describes a concern about the changes 😊

1. Missing skeleton in header while loading

As shown in the screenshot below, it seems that the skeleton is missing from the header while refreshing projects. That is, we're no longer showing placeholders for the branches, files, and the edit button.

@mpabarca Can you add the skeleton to this page again?

2. Header skeleton shown on "Not Found" page

The skeleton that isn't shown in issue no. 1 is, however, displayed on the "Not Found" page, where it doesn't serve any purpose.

@mpabarca Can you remove it from this page?

@simonbs Yes, the UI and logic showing the Skeleton is the part that I'm fixing now. When that's done I'll let you know. But thanks for catching up those two! if you find any other one just let me know to fix it!

P.S Another bug that I found is duplication on the messages on not found the version or specification. I'll fix this one too

Screen.Recording.2024-10-07.at.10.51.01.mov

@mpabarca mpabarca force-pushed the bugfix/delays-error-messages-until-data-refreshed-mpabarca branch from 2f3b552 to a4d4315 Compare October 7, 2024 13:16
@mpabarca
Copy link
Contributor Author

mpabarca commented Oct 7, 2024

This appears to solve the original issue -- that's amazing! 🙌 However, it also seems to introduce the two issues shown below.

Besides the two issues below, I've also added a small comment that describes a concern about the changes 😊

1. Missing skeleton in header while loading

As shown in the screenshot below, it seems that the skeleton is missing from the header while refreshing projects. That is, we're no longer showing placeholders for the branches, files, and the edit button.

@mpabarca Can you add the skeleton to this page again?

2. Header skeleton shown on "Not Found" page

The skeleton that isn't shown in issue no. 1 is, however, displayed on the "Not Found" page, where it doesn't serve any purpose.

@mpabarca Can you remove it from this page?

@simonbs Fixed too 🙌🏼

@mpabarca mpabarca force-pushed the bugfix/delays-error-messages-until-data-refreshed-mpabarca branch from 661e96a to 1bfdcc2 Compare October 7, 2024 13:32
@mpabarca mpabarca marked this pull request as ready for review October 7, 2024 13:32
@mpabarca mpabarca requested a review from simonbs October 7, 2024 13:32
@simonbs
Copy link
Contributor

simonbs commented Oct 7, 2024

Tested the issues with the extraneous and missing skeleton and it looks great now! ✨

@mpabarca I'd appreciate your input on the possible issue mentioned in this comment. Once we've determined that it's either not an issue or have it addressed, I think we should merge this PR 🚀😃

@mpabarca mpabarca force-pushed the bugfix/delays-error-messages-until-data-refreshed-mpabarca branch from 1bfdcc2 to 6dcbe69 Compare October 14, 2024 08:02
@mpabarca mpabarca force-pushed the bugfix/delays-error-messages-until-data-refreshed-mpabarca branch from 6dcbe69 to 14d9711 Compare October 14, 2024 08:46
@mpabarca mpabarca merged commit eea56a3 into develop Oct 14, 2024
5 checks passed
@mpabarca mpabarca deleted the bugfix/delays-error-messages-until-data-refreshed-mpabarca branch October 14, 2024 09:33
@simonbs simonbs requested a review from Copilot November 19, 2024 19:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no suggestions.

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.

2 participants