TLS certificates page: count reads upfront and keep partial ACME rows - #1261
Merged
Conversation
The counters were incremented inside the loop and decremented through a helper clamped with Math.max(0, n - 1). The clamp hides two things: the counter reaches zero between two iterations, so the table can leave its loading state while the next task is still being created, and a surplus decrement is silently swallowed instead of being impossible. Set the counter to the number of instances before the loop and decrement it plainly. clearTaskListeners() already makes a stale event unable to reach a handler, so nothing is left to clamp. Release listCertificatesNum when list-installed-modules is aborted: the certificates chain never runs, and the skeleton rows stayed for good when the abort followed a reconnection.
error.getAcmeServer is a single shared string and NsDataTable renders its error notification instead of the rows, so one unreachable node hid the settings of every node that did answer. Show the notification above the table when there are rows to keep, and leave isErrorShown to the case where there is nothing else to display.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Targets
sdl-8099, the branch of #1257, notmain.Description
A review pass on #1258 (HTTP routes page) raised two issues that apply here as well, since both pages share the same read pattern. This PR brings the two branches to the same shape, so the shared mixin planned as a follow-up on #1257 has one behaviour to extract instead of two.
Counters are set before the loop.
listCertificates()andgetAcmeServer()incremented inside the loop and decremented through a helper clamped withMath.max(0, n - 1). Two problems hide behind that clamp. The counter reaches zero between two iterations, soloadingCertificates/loadingServerscan go false while the nextcreateModuleTaskForAppis still awaited — the table drops out of its loading state with an empty or partial list. And a surplus decrement is swallowed rather than made impossible, which is what let the counter go out of balance in the first place. The counter is nowthis.traefikInstances.lengthbefore the loop and the decrements are plain--. Nothing is left to clamp:clearTaskListeners()already$offs the handlers of the previous round, so an event delivered late reaches nothing.listCertificatesNumis released when the instance lookup is aborted.listInstalledModulesAbortedresetloading.listInstalledModulesbut left the certificates counter untouched. On a reconnection the watcher restarts fromlistInstalledModules(), so if that task aborts the certificates chain never runs and whatever the counter held stays there — skeleton rows for good, with no way out but a manual reload.One offline node no longer hides the others on the ACME tab.
error.getAcmeServeris a single shared string, andNsDataTablerenders its error notification instead of the rows (v-if="isErrorShown"…v-else-if="isLoading"… inNsDataTable.vue). On a cluster where one node is unreachable, the settings of every node that answered were replaced by the banner. The notification now renders above the table when there are rows to keep, andisErrorShownis left for the case where there is genuinely nothing to display. The Certificates tab already handles this throughofflineTraefikInstancesandlistCertificatesErrors, so it needs no change.Commits
fix(ui): count ACME and certificate reads upfrontfix(ui): keep the ACME rows readable when one node failsTesting
core/uihas no test runner, so this was checked with lint and a production build, on each commit. Both pass.To verify on a cluster:
list-installed-modulesaborts after a reconnection: the certificates table shows its error instead of loading for ever