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

Remove ContentPage.getDerviedStateFromProps to fix navigation bug (#644) #888

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

ssciolla
Copy link
Contributor

@ssciolla ssciolla commented Feb 14, 2023

This PR aims to resolve #644.

Explanation

The getDerivedStateFromProps method in the ContentPage component was introduced in this commit: baf5f0e My colleague from University of Michigan @jonespm did a good job analyzing and explaining the issue, but I'll do so here for completeness.

Inside this method, there is a usage of issue.id as an index for finding an issue in report.issues with the same ID. Order in this array and ID are not equivalent. The result is that, in scenarios where the ID is small enough that it could also be an index (if it's not small enough, in JavaScript, accessing an invalid index results in undefined), the method ends up comparing two different issues. If one of the issues is fixed or resolved (i.e. it has a different status), the issue from the incorrect indexing supersedes, leading to a jumping of indexes in the UI to a completely unrelated issue.

@jonespm proposed initially in PR #853 that we use find as a replacement for the logic here. That would likely work fine, but I haven't yet figured out what value this method adds. It's supposed to update the activeIssue state of ContentPage if what it receives from props is different from what it has in internal state. Okay, cool, but shouldn't that state already be updated by ContentPage.handleActiveissue, which is called in several places in UFixitModal and its children? Since this method hasn't been functioning as intended, I'm inclined to think it's not necessary at all, but let me know if I'm missing something here.

Note: there are other issues with the navigation that lead to index jumping, but usually it's just the result of recently state not being kept beyond the first (see #887) or within a content item grouping (something I think @taheralfayad was working on and I will try to document). I feel these should be addressed separately.

Local testing instructions

  1. Clear out the database volumes (you may want to record your institution record for easy reference beforehand). This will ensure that issue IDs are low enough to overlap with report.issues indexes.

    docker volume rm udoit_db_data
    
  2. Start with main and run the application as normal, launching a course with a decent number of issues. Remember you'll have to set up the application database again.

    git checkout main
    docker compose -f docker-compose.nginx.yaml build
    docker compose -f docker-compose.nginx.yaml up
    docker compose -f docker-compose.nginx.yml run php php bin/console doctrine:migrations:migrate
    # Then add your institution record back
    
  3. Open the UFixit modal with only "Active Issues" checked (show all content item types). Then resolve or fix an issue. Note that the active issue changes and write down the index number. If you navigate then through the list, eventually the index will jump again back to the same index number.

    There may be a case where the ID is the length of the array, so it may not be a valid index. If this issue is selected, the issue should not jump and should show resolved or fixed. If this happens, fix or resolve another issue.

    You may find it useful to apply the following diff so that the issue ID is visible in the modal, in the header. Choosing a low ID number will make it easier to find the problematic issue/index again.

    diff --git a/assets/js/Components/UfixitModal.js b/assets/js/Components/UfixitModal.js
    index cfa1e03f..f05c43d4 100644
    --- a/assets/js/Components/UfixitModal.js
    +++ b/assets/js/Components/UfixitModal.js
    @@ -106,7 +106,7 @@ class UfixitModal extends React.Component {
               <Modal.Header padding="0 medium">
                 <Flex>
                   <Flex.Item shouldGrow shouldShrink>
    -                <Heading>{this.props.t(`rule.label.${activeIssue.scanRuleId}`)}</Heading>
    +                <Heading>{this.props.t(`rule.label.${activeIssue.scanRuleId}`)} (issue ID: {activeIssue.id})</Heading>
                   </Flex.Item>
                   <Flex.Item>
                     <CloseButton
    
  4. Spin the application down, checkout this issue branch, then repeat step 3, checking to see if the behavior has changed.

    gh pr checkout 888   # With GitHub CLI
    docker compose -f docker-compose.nginx.yaml build
    docker compose -f docker-compose.nginx.yaml up
    

Resources

@ssciolla ssciolla added bug javascript Pull requests that update Javascript code labels Feb 15, 2023
@cidilabs
Copy link
Contributor

Great job in identifying this issue. As you mention, this isn't THE answer to the modal index issue, but it's a good start.

@cidilabs
Copy link
Contributor

My only concern here is we don't know what we don't know. There was a use case for getDerivedStateFromProps() to be added in the first place. It was attempting to solve a problem, but we don't know what that problem was. I'll ask around to see if I can find out why it was added, but I agree that the logic is broken and should at least be fixed, if not removed.

@ssciolla
Copy link
Contributor Author

ssciolla commented Feb 17, 2023

Thanks, @cidilabs , for taking a look and following up with your team on the purpose of this method. I appreciate it. I'm open to changing it to the report.issues.find(...) approach if we hear something that suggests it's needed.

I don't know this application as well as you all do, but my reading and experience with React applications suggest that the side effect nature of this method can make it somewhat hard to reason about versus relying on ContentPage.handleActiveState, which can be explicitly called in API callbacks. Even better, following the logic of the blog post I listed in the description, it may make sense to lift the activeIssue state up to the same level as the report in App, so that there's a "single source of truth" for this data and ContentPage becomes controlled with respect to activeIssue. (Or perhaps ContentPage just has an active ID in state, and relies on the report to pull the correct issue data). I understand the latter suggestion would require a more significant refactor, so I've opted for the lighter weight solution here.

@ssciolla
Copy link
Contributor Author

ssciolla commented Feb 23, 2023

I was taking a look at another issue, and I experimented a little with the alternative solution (basically what @jonespm had in #853).

  static getDerivedStateFromProps(props, state) {
    const stateActiveIssue = state.activeIssue
    const propsActiveIssue = stateActiveIssue && props.report.issues.find(i => i.id === stateActiveIssue.id)
    if (propsActiveIssue && propsActiveIssue.status !== stateActiveIssue.status) {
      return {
        activeIssue: propsActiveIssue
      }
    }
    return null
  }

It may fix the issue jumping but I saw an another problem with this approach; the loading indicator was not showing up while issues are being resolved. This seems to be because the issue gets marked with a resolved status right away, leading to a difference in state with what is being passed to props. Thus the props version wins out, and we also lose the pending state.

Screen.Recording.2023-02-23.at.12.02.25.PM.mov

@jonespm
Copy link
Contributor

jonespm commented Mar 3, 2023

Thanks for looking into that @ssciolla , now I do remember that side effect and I didn't figure out why it was happening. The logic seemed more correct but when it was doing this status check I couldn't get it to sync up.

I feel like I kept trying to modify the if statement in here as well to not switch to the props issue but couldn't see a combination that worked, so it just seemed like this needed something else.

@ssciolla ssciolla marked this pull request as ready for review April 10, 2023 14:58
@dmols dmols self-requested a review June 29, 2023 20:39
Copy link
Contributor

@dmols dmols left a comment

Choose a reason for hiding this comment

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

Both the loading indicator error mentioned by @ssciolla and the unreachable issue on UFIXIT error (mentioned in #644) do not occur on my end.

Screen.Recording.2023-06-29.at.5.32.29.PM.mov

All good to push!

@ssciolla
Copy link
Contributor Author

I don't have commit permissions (which is fine), please feel free to commit if you think it's good to go.

@dmols dmols self-assigned this Jul 23, 2024
@dmols dmols changed the base branch from main to dev July 24, 2024 20:13
@dmols dmols merged commit 0839145 into ucfopen:dev Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue navigation not working
4 participants