new-log-viewer: Avoid redundant loadPage after loadFile when loading a URL with a logEventNum specified.

[junhaoliao]

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 logEventNum is specified in the web application URL, a redundant loadPage request is sent to the main service worker after an initial loadFile request.

Debug prints which show a redundant loadPage request is sent after loadFile

Description

1. new-log-viewer: Avoid redundant loadPage after loadFile when logEventNum is specified in the URL.

Validation performed

1. Referred to the validation steps in Add support for loading files, decoding them as JSONL, and formatting them using a Logback-like format string. #46 to start the debug server.
2. Loaded http://localhost:3010/?filePath=http://localhost:3010/test/example.jsonl#logEventNum=10 and observed no loadPage request is sent after loadFile.
   
   Screenshot showing no loadPage request is sent after loadFile

[junhaoliao]

@Henry8192 kindly help review this

[Henry8192]

Suggested change

	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

[junhaoliao]

Hmm... How about

Suggested change

	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.

[junhaoliao]

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.

[Henry8192]

I changed the !== to ===, will that save pageNumRef.current = newPageNum;?

[junhaoliao]

Makes sense. There's no update pageNumRef.current = newPageNum if they're the same.

[junhaoliao]

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?

[kirkrodrigues]

For the PR title, how about:

new-log-viewer: Avoid redundant loadPage after loadFile when loading a URL with a logEventNum specified.