Bug 2044471 - Add loading indicator to embedded dependency tree

[kyoshino]

Bug 2044471 - Add loading indicator to embedded dependency tree

Add a loading indicator in case the tree update takes more than 300ms.

[kyoshino]

Note on the CSS: for simplicity, we could use CSS nesting, which is available in Firefox 117+. However, since Firefox 115 ESR is maintained until August, I have avoided using it.

[dklawren]

Suggestions / Nits

• #UPDATE_TREES_ERROR_MESSAGE should be static. It's an immutable constant; as an instance field it's reallocated per DependencyTree instance. static #UPDATE_TREES_ERROR_MESSAGE = ... (referenced as DependencyTree.#UPDATE_TREES_ERROR_MESSAGE) is more accurate. Minor.
• Empty catch {} swallows the error silently. Adding console.error(...) would aid debugging of intermittent fetch failures without changing user-facing behavior:

  } catch (ex) {
    console.error('Failed to load the dependency tree', ex);
    this.$container.innerHTML = this.#UPDATE_TREES_ERROR_MESSAGE;
  }
  

• top: 100px is a magic number. For short trees (or the error <p>, which isn't a [role="group"] and so won't be dimmed) the badge may sit oddly relative to content. Not blocking, but a comment or a content-relative value (e.g. centering within the visible area) would be more robust.
• Concurrency: rapid successive calls to updateTrees() each register their own timeout + finally. An earlier call's finally can removeAttribute('aria-busy') while a later in-flight call still expects the busy state. This is an edge case and the pre-existing code already raced on innerHTML, so it's not a regression — but if you want it airtight, track an in-flight counter or abort the prior fetch with AbortController.

[kyoshino]

Resolved the feedback 🙏🏼

[dklawren]

Some issues found during Claude code review just to see any JS issues that I am not an expert of. Please take a look and see if they are real issue that can be resolved before merge.

---

Issues & Risks

1. Race condition can orphan the "Updating…" message (correctness — medium)
   showUpdatingMessage() is async and awaits getContainerHeight() (up to 100ms), but it is not awaited when invoked from the setTimeout callback. Consider this timeline:

• t=300ms: timeout fires → showUpdatingMessage() starts, awaiting height
• t=350ms: fetch completes → finally runs clearTimeout (no-op) + hideUpdatingMessage() (removes nothing — message not inserted yet, clears aria-busy)
• t=400ms: height resolves → message is inserted into the container and aria-busy="true" is re-set

Result: a stuck "Updating…" element and a container left in the busy state. This triggers whenever the fetch finishes within ~100ms after the 300ms threshold — plausible in practice. Consider tracking the latest request (e.g. a generation counter or an aborted flag set in finally) and bailing out of showUpdatingMessage() if the fetch already completed, or awaiting the show and checking state before inserting.

2. disableControllers() lacks the defensive null-checks used elsewhere (correctness — low/medium)
   this.$numberInput is accessed with optional chaining everywhere else (this.$numberInput?.addEventListener line 58, this.$numberInput?.value line 93), implying it may be absent. But:

disableControllers() {
  this.$numberInput.disabled = true;   // throws if $numberInput is null
}

and the new this.$numberInput.disabled = false; in updateControllers() have the same exposure. If the input can legitimately be absent, these will throw a TypeError and break the whole update flow. Either confirm it's always present (and drop the ?. elsewhere for consistency) or guard these accesses.

3. Unawaited showErrorMessage() in catch/else (minor)
   Same not-awaited-async pattern as Updating MTV office #1. Less harmful here since it ends with innerHTML = '' + insert, but ordering relative to finally's hideUpdatingMessage() is implicit. Worth confirming the error message reliably survives.

[kyoshino]

Claude has solved the issue 1 (race condition) and 3 (unawaited promise). The issue 2 can be ignored because these input elements always exist, and the existence of the wrapper element is checked earlier with if (!this.$trees) return.

[kyoshino]

Also fixed some bugs on the number input 🐛

[dklawren]

Code review

Found 1 issue:

1. The race condition the loading indicator was meant to solve can still leave a stuck "Updating…" message and a permanently dimmed, non-interactive tree.

updateGeneration is only incremented when a new request starts (const generation = ++this.updateGeneration), never when the current request completes. The finally block calls clearTimeout + hideUpdatingMessage() but does not bump the generation:

https://github.com/mozilla-bteam/bmo/blob/5e213933bef10dd449668feedb305e8e5ebf7a8a/js/dependency-tree.js#L291-L297

So the second generation check in showMessage() only guards against a newer request superseding an older one — not against the current request's own completion:

https://github.com/mozilla-bteam/bmo/blob/5e213933bef10dd449668feedb305e8e5ebf7a8a/js/dependency-tree.js#L179-L195

Timeline when a fetch lands shortly after the 300ms threshold (common on 300–400ms responses):

• t=300ms: timeout fires → showUpdatingMessage(gen) starts → passes check Updating MTV office #1 → awaits getContainerHeight() (resolves ~1 frame to 100ms later)
• fetch completes → innerHTML set → finally runs hideUpdatingMessage() as a no-op (the <p> hasn't been inserted yet) → generation unchanged
• getContainerHeight() resolves → check [fix bug 998236] Privacy policy url fixed #2 still passes (same generation) → the "Updating…" <p> is inserted on top of the loaded tree and aria-busy="true" is set, with no later hideUpdatingMessage()

The tree is then stuck at opacity: 0.5; pointer-events: none (per the [aria-busy="true"] > [role="group"] rule) and aria-busy stays set for screen readers until another update is triggered. Incrementing this.updateGeneration inside the finally block (before hideUpdatingMessage()) would make check #2 fail and close the window.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[kyoshino]

Claude fixed the issue again 😅

[dklawren]

🤖 Generated with Claude Code

• If this code review was useful, please react with 👍. Otherwise, react with 👎.

Sorry bout that. I must have hit Yes too many times as it submitted the review comment itself. But glad to see it was something worth looking into and was a quick fix. Normally I let Claude loose on a new pull request first, then decide if the change is worth worrying about and then summarize in my own words so I understand it myself. Of course if it is something I can do myself I will just do that.

[dklawren]

Issues all resolved and manual testing looks good. r=dkl