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

Refactor rankstep#199 #232

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Refactor rankstep#199 #232

wants to merge 33 commits into from

Conversation

justin-b-yee
Copy link
Contributor

No description provided.

@justin-b-yee justin-b-yee linked an issue Oct 27, 2024 that may be closed by this pull request
Copy link
Contributor

@ddfridley ddfridley 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 taking this one on. I'm sorry it wasn't clear in the spec. The input to rank-step needs to be rank docs from the database, the same format as what this component upsert's into the db.

In the end, this component needs to upsert ranks (docs) into the context, and render based on the ranks (docs) out of the context.

If you look in rerank it works that way, and there's a table for converting the category value into the string to be passed further down. Perhaps that code needs to be in a separate module that can be shared by both.

Also, in the end we need a jest test for the deriver - see app/components/steps/tests/rerank.js

Also, something that I missed in rerank, is that initially, rankstep needs to call the "get-user-ranks", api and put the result into the context - if there is any. I will be going back to rerank to do that.

Thanks.

Copy link
Contributor

@ddfridley ddfridley left a comment

Choose a reason for hiding this comment

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

This is looking good. It's a big change and thanks for taking it on. See the comments in line. Thanks!

app/components/steps/rank-step.js Outdated Show resolved Hide resolved
app/components/steps/rank-step.js Outdated Show resolved Hide resolved
app/components/steps/rank-step.js Outdated Show resolved Hide resolved
app/components/steps/rank-step.js Outdated Show resolved Hide resolved
app/components/steps/rank-step.js Outdated Show resolved Hide resolved
app/components/steps/rank-step.js Outdated Show resolved Hide resolved
stories/rank-points.stories.jsx Outdated Show resolved Hide resolved
stories/rank-points.stories.jsx Show resolved Hide resolved
app/components/steps/__tests__/rank-step.js Outdated Show resolved Hide resolved
stories/rank-step.stories.js Show resolved Hide resolved
@justin-b-yee justin-b-yee marked this pull request as ready for review November 15, 2024 21:20
Copy link
Contributor

@ddfridley ddfridley left a comment

Choose a reason for hiding this comment

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

See the last 2 tests in storybook. Thanks.

return <RankStep {...otherArgs} />
}

export const rankStepWithPartialDataAndUserUpdate = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test, http://localhost:6006/?path=/story/rank-step--rank-step-with-partial-data-and-user-update and the next one
http://localhost:6006/?path=/story/rank-step--rank-step-with-top-down-update are failing the Interaction tests.

One problem I see is that line 187 says pprRankByParentId instead of preRankByParentId

After the test runs, some of the items should have ranks, but currently none do.

@justin-b-yee
Copy link
Contributor Author

The discrepancy between the data and filled in bubbles seems to be a rendering issue that goes back to the tug-of-war I've been working through between getting the 'delete rank' use case and rendering the initial ranks working at the same time. When I remove isInitialRender, the bubbles are filled, but then deleting ranks doesn't work again.

I've been trying for some time to figure if I can position the isInitialRender toggle or add a condition so both work simultaneously, but so far, one breaks when the other is fixed.

// if an item in the updated reviewPoints does not have a rank doc where it previously did, the rank doc will remain.
// deleting a rank is not a use case
}, [reviewPoints])

useEffect(() => {
const newRankByParentId = (pointRankGroupList || []).reduce((rankByParentId, rankPoint) => {
if (rankPoint.rank && (isInitialRender || rankByParentId[rankPoint.point._id]))
rankByParentId[rankPoint.point._id] = rankPoint.rank
return rankByParentId
}, {})
let updated = false
for (const rankDoc of Object.values(newRankByParentId)) {
if (isEqual(rankDoc, rankByParentId[rankDoc.parentId])) {
newRankByParentId[rankDoc.parentId] = rankByParentId[rankDoc.parentId]
} else updated = true
}
if (updated) {
setRankByParentId(newRankByParentId)
// Ranks loaded from context won't display because they're not in rankByParentId at first,
// so track the initial render to make an exception and add them to data
if (isInitialRender) {
setIsInitialRender(false)
}
}
}, [pointRankGroupList])

app/components/steps/rank.js Outdated Show resolved Hide resolved
@justin-b-yee
Copy link
Contributor Author

justin-b-yee commented Dec 4, 2024

I think I went this route because with both ways I tried to clear the ranks with before, the render issue remained. I slightly modified the logic so instead of checking for the initial render, it checks for the one immediately after the 'Clear' button is clicked to make a temporary exception, and the final test case seems to pass now.

Copy link
Contributor

@ddfridley ddfridley left a comment

Choose a reason for hiding this comment

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

This is looking really good. I'm sorry its so messy but I hope it's interesting, and there's a little more to go. 😄

Can you create a new test case for this, and then figure out how to get them all cleared.
I think the problem is that in the case onDone is going to get called 3 times in a row, without rerendering - and something along the path is using a state value returned by setstate, instead of getting the state value in a function passed to the setter. Or it could be something else.

see inline comment too.

Thanks!

if (clearRanksBlocked) {
newRankByParentId[rankPoint.point._id] = rankPoint.rank
} else if (newRankByParentId[rankPoint.point._id]) {
newRankByParentId[rankPoint.point._id] = { ...rankPoint.rank, category: '' }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think clearRanksBlocked is needed. I set a breakpoint on the false case, and it's never triggered. Can you take it out, or describe the case where it's needed so I can look at it.

Copy link
Contributor Author

@justin-b-yee justin-b-yee Dec 9, 2024

Choose a reason for hiding this comment

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

clearRanksBlocked fixed the issue where clearing ranks and display the new ranks are mutually exclusive.

In rerank.js, it's commented that ranks removed from reviewPoints reappear because deleting ranks isn't a use case. I added this case by only re-adding those ranks to newRankByParentId if it's still in the original, and skipping it if else not.

However, in the initial render (and after every change to the ranks as I found out), rankByParentId isn't fully initialized, so no ranks are rendered from the data and the bubbles are empty.

That's where clearRanksBlocked came in, which is initialized to true, so every rank in pointRankGroupList is put into rankByParentId and rendered even if it hasn't been ported to rankByParentId yet. I checked again and saw the second condition never ran as you mentioned, so I removed the redundancies and turned it into this.

if (rankPoint.rank && clearRanksBlocked) {
  // add to newRankByParentId, which renders the rank properly
} 
// do nothing if else

Then when the Clear All button is clicked, it sets clearRanksBlocked to false, and the above conditional relies solely on rankPoint.rank being true (i.e. it exists). If it doesn't, then it isn't added to newRankByParentId, it's not in the data and doesn't get rendered subsequently, and therefore the rank is now cleared.

clearRanksBlocked is reverted to true immediately after that so the ranks can properly render again from the data.

I'm honestly still a bit confused myself, but that was the solution I arrived at to make them work concurrently.

@ddfridley
Copy link
Contributor

@justin-b-yee following up from the call today, If I remove the clearRanksBlocked stuff it works fine in rank-points. The problem in in some of the cases of rank-step. Also, In rank step, after I clear, and look at what's in the context (when you scroll down in storybook) the category's after a clear aren't set. The should be. I suspect there is a problem in handleOnDone of RankStep not getting delta, or at least not upserting the changes. Or not getting the onDone's..

@justin-b-yee
Copy link
Contributor Author

I suspect there is a problem in handleOnDone of RankStep not getting delta, or at least not upserting the changes. Or not getting the onDone's..

Yeah I think you're right - I did some more testing and when I commented out the onDone line, the ranks didn't reappear after clearing (though of course, they didn't get upserted either). I went back to the previous attempt and called handleRankPoint on all points to clear them. I am seeing the delta with category: '' reach the onDone function, but I also see another call right after that seems to re-write in the last value, which would explain the brief flicker. I'm still trying to figure out where and why this other call is happening though.

@ddfridley
Copy link
Contributor

@justin-b-yee I made a change to your change to app/components/ranking.jsx so that it would not send onDone if initally empty. I think it will still work for you but let me know. It's merged into master now.

@justin-b-yee
Copy link
Contributor Author

justin-b-yee commented Dec 15, 2024

@justin-b-yee I made a change to your change to app/components/ranking.jsx so that it would not send onDone if initally empty. I think it will still work for you but let me know. It's merged into master now.

Thanks! Not only does it still work - the change partially fixed the flicker problem after resolving conflicts. It's now just a problem where only one point is cleared each time the Clear All button is clicked that I can hopefully investigate further.

@justin-b-yee
Copy link
Contributor Author

I wonder - does this happen because we can only upsert one rank at a time, so the clear operation isn't atomic and causes a race condition? I was thinking before that the ranks could be rerendered on data where one is changed to empty and the rest are the previous values, and the data coming back down overwrites the immediate local changes where all are cleared.

  • rankByParentId categories cleared via setState, triggers rerender
  • Default value of first ranking is '', change triggers onDone. (other rankings trigger their own onDone as well)
  • Concurrently, those onDones runs handleRankPoint which upserts the single rank.
  • Rank with category: '' is upserted into context, but the rest of the categories in that scope are the previous values.
  • Context change triggers rerender, overwrites the emptied rankByParentId to the rankByParentId where all categories are still there except the first.

@ddfridley
Copy link
Contributor

Yes! You bring up a good point. Up until Friday night, the upsert into the context would create a clone, so all the refs to all the rank objects would be new, causing react to see it as change through the react chain.

With the change I pushed Friday night, upsert was redone so that only the changed objects, and their parent refs are changed. See app/lib/set-or-delete-by-mutate-path.js and it's test. And I'm gong to be talking about this on Monday in the dev meeting. But happy to talk about it and dig into this at a time that works for you.

Within the deriver, it needs to make sure it's not changing the ref of ranks that aren't changed.

Another possibility I'm thinking about, is whether the category going down to Ranking and coming back up are both the same - meaning ''. Or, could one be undefined and something else be doing ''.

There is a case where Rank could get called with each context change, with only one rank changed. But the Rank component shouldn't pass new props to any of it's children. It should see that the new rank coming down is the same as the current rank and stop

But it's also the case that React bunches up state changes that are triggered by the same event, and there may be several or all between retenders.

So inside the useEffect on chanaged to rankByParentId, it should ignore ranks if oldRank===newRank.

Does this make any sense, and does it help? Let me know. And happy to have a call and talk through it.

This is the really deep stuff about how React works, and what we are trying to do to make bidirection data updats work within React.

Thanks for your perseverance!

@ddfridley
Copy link
Contributor

And it's more complicated than that. I think the lesson is that Rank is keeping state in the middle, but this system is designed for changes to come from the bottom/user or top / data base. But clear is changing the state in the middle first.

One solution might be for clear create it's own delta's and onDone() the changes up, so that they come back down to change the it's state and then rerender the ranking

But this won't work for the Rank story by itself, it need the RankStep and context. But maybe that's okay for now.

Copy link
Contributor

@ddfridley ddfridley left a comment

Choose a reason for hiding this comment

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

I pushed a few changes to the deriver test - you will need to git pull.
these tests don't pass, but I think the define what's needed.

I admit that what you had to start with in rerank had similar problems. I will take care of fixing those separately.

Also, the way that group is part of the pointRankGroup list is different than it was previously, and not clearly documented in the spreadsheet. But the change is reflected in the test.

app/components/steps/rank.js Outdated Show resolved Hide resolved
app/components/steps/rank.js Outdated Show resolved Hide resolved
@justin-b-yee
Copy link
Contributor Author

Hm, I managed to get two tests to pass, but I'm a bit confused by the behavior I've observed in the last one with my current understanding. Seems like the rank category is not being updated because local.preRankByParentId already has the new values when the deriver is called again, and the conditional checking for changes is false:

 data.preRankByParentId[1].rank = { _id: '4', category: 'neutral', parentId: '1', stage: 'pre' }
 const newPointRankGroupList = derivePointRankGroupList(data).pointRankGroupList 

 LOCAL DATA {
        '1': { _id: '4', category: 'neutral', parentId: '1', stage: 'pre' },
        '2': { _id: '5', category: 'neutral', parentId: '2', stage: 'pre' },
        '3': { _id: '6', category: 'least', parentId: '3', stage: 'pre' }
      }

So rankPointsById[point._id] doesn't get set to the new rank which should be neutral.

         "rank": Object {
            "_id": "4",
    -       "category": "neutral",
    +       "category": "most",
            "parentId": "1",
            "stage": "pre",
          },
        },

I haven't yet found where local.preRankByParentId would get updated with { _id: '4', category: 'neutral', parentId: '1', stage: 'pre' } before this call though, as I'd thought it could only get updated here:

  if (local.preRankByParentId !== preRankByParentId) {
    for (const rank of Object.values(preRankByParentId)) {
      if (rankPointsById[rank.parentId]) {
        if (rankPointsById[rank.parentId].rank !== rank) {
          rankPointsById[rank.parentId] = { rank: rank, ...rankPointsById[rank.parentId] }
          updated = true
        }
      }
    }

    local.preRankByParentId = preRankByParentId
  }

@ddfridley
Copy link
Contributor

See changes here d7703e6
The tricky part was that in the deriver, after local.preRankByParentId is set to data.preRankByParentId, if the test then changes data.preRankByParentId[1], it's being changed in local too.

Also, did you know you can do: npm run dbtest app/components/steps/__tests__/rank.js

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.

Refactor RankStep
2 participants