The diff introduces mechanism for setting pendingThread in navInfo while first page load,, so it match activeChatThreadID value.
The reason is to load pending thread correctly in navigation after page refresh - when we know only pending thread ID from URL.
The solution doesn't work for sidebars right now. Creating pending sidebar would require addidtional data (like parent thread info) that should be found efficiently using only sourceMessageID, and it could be introduced in the future.
Details
Details
Run web app and using search find user, that has no chat with our user. Clicking that user will create pending personal chat. After refreshing the page, the pending chat should be loaded automatically and user is not redirected to first chat from list.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Looks like there was a network issue pulling from DockerHub, restarted just that job and it seems to have succeeded.
But, it looks like permissions (Harbormaster) to restart individual jobs weren't enabled for everyone. Just fixed that
Comment Actions
(Not super familiar with this part of the codebase, so please forgive any confusion. Requesting changes, but feel free to re-request review if I'm misunderstanding something.)
If I understand correctly, if activeChatThreadID is pending (eg startsWith("pending")) and pendingThread doesn't correspond to the activeChatThreadID, then we create a new pending thread with createPendingThread({...}).
In what situation do we expect a mismatch between a pending activeChatThreadID and pendingThread to occur?
Since we can always construct pendingThread from a pending activeChatThreadID (via parseLocallyUniqueThreadID), shouldn't we be able to keep them in sync?
There are places in the codebase where we dispatch something like the following (taken from robotext-message.react.js:176-181):
this.props.dispatch({
type: updateNavInfoActionType,
payload: {
activeChatThreadID: id,
},
});Instead of depending on the logic in reduceNavInfo to clean up navInfo.pendingThread for us, could we explicitly unset it here (assuming the new activeChatThreadID is for a "realized" thread)? Something like:
this.props.dispatch({
type: updateNavInfoActionType,
payload: {
activeChatThreadID: id,
pendingThread: undefined,
},
});Obviously this isn't great because the developer would need to remember to unset pendingThread every time that we update activeChatThreadID with a realized thread ID.
So maybe instead of exclusively using UPDATE_NAV_INFO to modify navInfo, we can create a new action like UPDATE_ACTIVE_THREAD_ID (with activeChatThreadID as payload) that handles setting/unsetting of the pendingThread object?
All of this is working under the assumption that we always want
- pendingThread to be unset if pendingThread.id !== activeChatThreadID
- pendingThread to be set and pendingThread.id === activeChatThreadID if threadIsPending(activeChatThreadID)
Is that an incorrect assumption? Is there a case where we might want to keep a pendingThread around after activeChatThreadID has changed? EG if we navigate away from a pending thread, is there any reason to keep navInfo.pendingThread around?
It's also a little strange that we have a single "action handler" in nav-reducer:
if (action.type === updateNavInfoActionType) {
state = {
...state,
...action.payload,
};
}and then "fix things up" based on various conditions outside of that if block that "get hit" on every single action. I think we should handle the setting/unsetting of things more explicitly with additional action(s) specific to NavInfo.
| web/redux/nav-reducer.js | ||
|---|---|---|
| 51–52 ↗ | (On Diff #14220) | Can we collapse these w/ optional chaining? |
Comment Actions
Thanks @atul, for the detailed comment!
If I understand correctly, if activeChatThreadID is pending (eg startsWith("pending")) and pendingThread doesn't correspond to the activeChatThreadID, then we create a new pending thread with createPendingThread({...}).
Exactly - if activeChatThreadID indicates we are in a pending thread, and the pending thread object in navigation doesn't exist or contains pending thread with mismatching ID, we want to correct it in reducer, to make these two fields consistent.
In what situation do we expect a mismatch between a pending activeChatThreadID and pendingThread to occur?
Since we can always construct pendingThread from a pending activeChatThreadID (via parseLocallyUniqueThreadID), shouldn't we be able to keep them in sync?
Until now, I think there isn't any case the mismatch could happen. As far as I know, in every place we dispatch putting pending activeChatThreadID in navInfo, we also provide corresponding threadInfo into pendingThread field. So if there are no errors in the code, the mismatch should not occur, but the change could prevent such a problem if it appears in the future.
However, the new case, that would introduce such mismatch is described in summary:
"The reason is to load pending thread correctly in navigation after page refresh - when we know only pending thread ID from URL."
So when we are initially loading the page (like after refresh), the activeChatThreadID could be pending (after a few changes - e.g. in web responder on server - introduced in separate diffs) basing on the URL, and we don't have pendingThread object calculated anywhere, so the navReducer seemed for me to be a good place to do it (because it also prevents problems that could occur if we dispatch navInfo update without data accidentally, or these IDs in navInfo would be somehow desynchronized)
So maybe instead of exclusively using UPDATE_NAV_INFO to modify navInfo, we can create a new action like UPDATE_ACTIVE_THREAD_ID (with activeChatThreadID as payload) that handles setting/unsetting of the pendingThread object?
It's also a little strange that we have a single "action handler" in nav-reducer
and then "fix things up" based on various conditions outside of that if block that "get hit" on every single action. I think we should handle the setting/unsetting of things more explicitly with additional action(s) specific to NavInfo.
It sounds reasonable. However, it would not cover the case when state was set without dispatching any action - so while first state calculation during initial page load. I'm curious what is @palys-swm opinion, what would be the best solution.
All of this is working under the assumption that we always want
- pendingThread to be unset if pendingThread.id !== activeChatThreadID
- pendingThread to be set and pendingThread.id === activeChatThreadID if threadIsPending(activeChatThreadID)
Is that an incorrect assumption?
Yes, the first one obviously only if activeChatThreadID is not pending, otherwise trying to set correct pendingThread if there is a mismatch.
Is there a case where we might want to keep a pendingThread around after activeChatThreadID has changed? EG if we navigate away from a pending thread, is there any reason to keep navInfo.pendingThread around?
I cannot find any. If fact, keeping mismatched pendingThread caused problems before. It's why A few weeks ago I introduced clearing the pendingThread if we display non-pending one: (https://phab.comm.dev/D4095).
| web/redux/nav-reducer.js | ||
|---|---|---|
| 51–52 ↗ | (On Diff #14220) | I think we cannot: If pendingThreadData is null, |
Comment Actions
However, the new case, that would introduce such mismatch is described in summary:
"The reason is to load pending thread correctly in navigation after page refresh - when we know only pending thread ID from URL."
So when we are initially loading the page (like after refresh), the activeChatThreadID could be pending (after a few changes - e.g. in web responder on server - introduced in separate diffs) basing on the URL, and we don't have pendingThread object calculated anywhere, so the navReducer seemed for me to be a good place to do it (because it also prevents problems that could occur if we dispatch navInfo update without data accidentally, or these IDs in navInfo would be somehow desynchronized)
It looks like there are two ways that activeChatThread is set/modified.
- In website_responders via navInfoFromURL, navInfoPromise, etc. when the page is "initially" loaded
- updateNavInfoActionType actions dispatched to the Redux store when the app is "running"
We could modify website_responders to properly construct pendingThread from activeChatThreadID to handle the "initial" page load.
And we can handle (2) via web/redux/nav-reducer.js where we should be able to handle "fixing things up" within the updateNavInfoActionType action handler.
As for any mismatch that happens "outside" of the updateNavInfoActionType action, we could introduce invariants in reduceNavInfo(...) to enforce that activeChatThreadID and pendingThreadID remain "in sync," though I don't think that's a huge concern?
I think these changes should lead to a cleaner and clearer redux/nav-reducer.
I think the best way to sequence things would be:
- Introduce a diff that properly constructs pendingThread in website_responders so we "start off" with correct and consistent values for activeChatThreadID and pendingThread
- Basically this diff, but modified such that all of the logic is "within" an action handler
- Optional: Introduce new action types to separate out the logic in reduceNavInfo(...)
Sorry for taking so long to respond to your reply. I'm open to scheduling a call or something this week to discuss synchronously if you think that'll help speed things up.
Comment Actions
- Introduce a diff that properly constructs pendingThread in website_responders so we "start off" with correct and consistent values for activeChatThreadID and pendingThread
- Basically this diff, but modified such that all of the logic is "within" an action handler
So the idea you suggest is to perform pending thread creation during first load in websiteResponder instead of using navReducer for this purpose? This makes sense to me, as this action should be necessary only during first load.
- Optional: Introduce new action types to separate out the logic in reduceNavInfo(...)
I'm not sure if it's necessary for the case here, but it may be good to cover it with a separate task, as it may help in the future to keep the navigation state consistent.
Comment Actions
So the idea you suggest is to perform pending thread creation during first load in websiteResponder instead of using navReducer for this purpose? This makes sense to me, as this action should be necessary only during first load.
Yeah, that was what I was thinking made more sense
I'm not sure if it's necessary for the case here, but it may be good to cover it with a separate task, as it may help in the future to keep the navigation state consistent.
Yeah, I agree that makes sense
Comment Actions
This looks good to me, thanks for addressing feedback.
Would be good for @tomek to take a look as well since he has more context (he's already set as a blocking reviewer).
| keyserver/src/responders/website-responders.js | ||
|---|---|---|
| 253 ↗ | (On Diff #14714) | Wouldn't this remain the same before/after creating newPendingThread? |
| keyserver/src/responders/website-responders.js | ||
|---|---|---|
| 253 ↗ | (On Diff #14714) | Yes, it should. The only case it may be different is when user manually changes sequence of IDs in the URL (e.g. from pending/type6/89218+83810/ to pending/type6/83810+89218/) and in this case the IDs won't match, so I think it's better to leave this assignment to make sure they're matching |
Comment Actions
- Optional: Introduce new action types to separate out the logic in reduceNavInfo(...)
I'm not sure if it's necessary for the case here, but it may be good to cover it with a separate task, as it may help in the future to keep the navigation state consistent.
Created follow-up task:
https://linear.app/comm/issue/ENG-1448/refactor-nav-reducer-by-introducing-new-action-types-to-separate-its