new-log-viewer: Avoid redundant loadPage after loadFile when loading a URL with a logEventNum specified.#55
Conversation
…EventNum` is specified in the URL.
|
@Henry8192 kindly help review this |
loadPage after loadFile when logEventNum is specified in the URL.loadPage after loadFile when logEventNum is specified in the URL.
| if (STATE_DEFAULT.pageNum !== pageNumRef.current) { | ||
| if (newPageNum === pageNumRef.current) { | ||
| // Don't need to switch pages so just update `logEventNum` in the URL. | ||
| updateLogEventNumInUrl(numEvents, logEventNumRef.current); | ||
| } else { |
There was a problem hiding this comment.
| if (STATE_DEFAULT.pageNum !== pageNumRef.current) { | |
| if (newPageNum === pageNumRef.current) { | |
| // Don't need to switch pages so just update `logEventNum` in the URL. | |
| updateLogEventNumInUrl(numEvents, logEventNumRef.current); | |
| } else { | |
| if (STATE_DEFAULT.pageNum === pageNumRef.current) { | |
| return; | |
| } else if (newPageNum === pageNumRef.current) { | |
| // Don't need to switch pages so just update `logEventNum` in the URL. | |
| updateLogEventNumInUrl(numEvents, logEventNumRef.current); | |
| } else { |
How about making this an early return? We can avoid if inside ifs
There was a problem hiding this comment.
Hmm... How about
| if (STATE_DEFAULT.pageNum !== pageNumRef.current) { | |
| if (newPageNum === pageNumRef.current) { | |
| // Don't need to switch pages so just update `logEventNum` in the URL. | |
| updateLogEventNumInUrl(numEvents, logEventNumRef.current); | |
| } else { | |
| if (STATE_DEFAULT.pageNum !== pageNumRef.current) { | |
| pageNumRef.current = newPageNum; | |
| return; | |
| } else if (newPageNum === pageNumRef.current) { | |
| // Don't need to switch pages so just update `logEventNum` in the URL. | |
| updateLogEventNumInUrl(numEvents, logEventNumRef.current); | |
| } else { |
? i.e. update pageNumRef.current before returning.
There was a problem hiding this comment.
I was thinking about avoiding duplicated code when I attempted to add the early return. If we agree this style is better, I'm fine duplicating the line to update pageNumRef.current.
There was a problem hiding this comment.
I changed the !== to ===, will that save pageNumRef.current = newPageNum;?
There was a problem hiding this comment.
Makes sense. There's no update pageNumRef.current = newPageNum if they're the same.
There was a problem hiding this comment.
Sorry for misreading the code. I think we still need the pageNumRef.current = newPageNum update before returning, right? Say what if newPageNum !== STATE_DEFAULT.pageNum?
Co-authored-by: Henry8192 <50559854+Henry8192@users.noreply.github.com>
This reverts commit d7de3c1.
|
For the PR title, how about: new-log-viewer: Avoid redundant |
loadPage after loadFile when logEventNum is specified in the URL.loadPage after loadFile when loading a URL with a logEventNum specified.
References
new-log-viewer series: #45 #46 #48 #51 #52 #53
#48
new-log-viewer: Add UrlContextProvider to provide URL parameters and use them in the StateContextProvider.It was found that when
logEventNumis specified in the web application URL, a redundantloadPagerequest is sent to the main service worker after an initialloadFilerequest.Debug prints which show a redundant
loadPagerequest is sent afterloadFileDescription
loadPageafterloadFilewhenlogEventNumis specified in the URL.Validation performed
loadPagerequest is sent afterloadFile.Screenshot showing no
loadPagerequest is sent afterloadFile