WebUI: Restore previously used tab on load

[Piccirello]

This PR restores the users previously used tab (Transfer, Search, RSS, etc.) when the Web UI is reloaded.

Screen.Recording.2024-04-14.at.23.33.19.mov

[Chocobo1]

Suggested change

	console.error(`Unexpected 'selected_tab' value: ${previouslyUsedTab}`)
	$('transfersTabLink').click();
	console.error(`Unexpected 'selected_tab' value: ${previouslyUsedTab}`);
	$('transfersTabLink').click();
	break;

Would be nice to have a break for the last case.

[Piccirello]

I did a quick search before pushing this change and MDN's examples don't include a break in the default case. It makes sense since there's nothing further that can run.

[Chocobo1]

It is for 'defensive' coding that it will prevent someone adding another case below and forget to add a break up here.
https://softwareengineering.stackexchange.com/questions/201777/break-on-default-case-in-switch

[Piccirello]

It wouldn't make any sense to add a case below the default. This seems like a stylistic choice that is low impact enough to not warrant spending more time on.

[Chocobo1]

It wouldn't make any sense to add a case below the default.

You wouldn't believe the kind of stupid mistakes that happens when code changes, especially by naive coders (and even by veteran coders).

This seems like a stylistic choice that is low impact enough to not warrant spending more time on.

I agree about the 'not warrant spending more time' part but not about the 'low impact' part.

Also I would like to reiterate that I'll not be responsible for any breakage of WebUI and I only help review in best effort. So please don't ever ping me about it when it happens. I've done my best (concerning long-term maintainability) but they wouldn't even move their finger for a trivial change. I'll just approve it so everyone is happy.

[Piccirello]

I understand your desire to optimize for long-term maintainability and reduce the likelihood of future errors. Under that goal, adding break is a sensible change.

However, I've been working on a codebase over the last several years that enforces all style with eslint and prettier. That combination removes any need for stylistic comments and allows reviewers to focus exclusively on structure and business logic. Having gotten used to that, stylistic comments now feel burdensome and time consuming for minimal impact. Thus, I don't think your comment is incorrect; I just want us both to be able to spend our time on more substantive things.

Maybe enforcing eslint's no-fallthrough would help here?

[Chocobo1]

Maybe enforcing eslint's no-fallthrough would help here?

This rule is already enabled by the recommended rule set. It helps but it doesn't satisfy the semantic in our case here IMO.

I understand your desire to optimize for long-term maintainability and reduce the likelihood of future errors. Under that goal, adding break is a sensible change.

However, I've been working on a codebase over the last several years that enforces all style with eslint and prettier. That combination removes any need for stylistic comments and allows reviewers to focus exclusively on structure and business logic. Having gotten used to that, stylistic comments now feel burdensome and time consuming for minimal impact. Thus, I don't think your comment is incorrect; I just want us both to be able to spend our time on more substantive things.

We all are used to something but that something is mostly localized to the individual. The open source (as a hobby) and real world business is actually two different worlds and the expectations are different too. One has tight time deadlines and the other sometimes don't. One prioritize feature rollout and the other prioritize code quality and long-term maintainability.

I believe the consensus between the main devs here is that, the lint tool (or any helper tool) is just auxiliary to the programmer and should not be regarded as the ground truth. There are still things that cannot be easily enforced by existing tools and relies on common sense (maybe not so common for some) and good practices. I wouldn't point out something if I think it isn't has an impact, whether it is stylistic or some feature logic. And I would still do so in future because it is a responsibility of a reviewer. Yes I get it that it might put burden on the PR author but we all have our roles to play. And it is quite a mystery to me that why a simplistic change (even with code provided) would induce stress on the other side. I would very much like to hear about it.

Last, having break in the last case is actually common among some major programming languages and not just some stylistic decoration:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es79-use-default-to-handle-common-cases-only
https://stackoverflow.com/questions/47938715/why-in-c-sharp-break-is-required-in-switch-loop-after-default-case
https://stackoverflow.com/questions/2729813/switch-statements-do-you-need-the-last-break-javascript-mainly
https://en.wikipedia.org/wiki/Switch_statement#Fallthrough
The gist is that, if the switch statement (of some language) has fall through behavior then case should have break, otherwise in general it is regarded as implicit fallthrough which is often not the programmer want.