Skip to content

TipTap: Load stylesheet parallel to extensions#23024

Open
nielslyngsoe wants to merge 1 commit into
v17/devfrom
v17/feature/do-not-await-stylesheet-loading
Open

TipTap: Load stylesheet parallel to extensions#23024
nielslyngsoe wants to merge 1 commit into
v17/devfrom
v17/feature/do-not-await-stylesheet-loading

Conversation

@nielslyngsoe
Copy link
Copy Markdown
Member

@nielslyngsoe nielslyngsoe commented May 31, 2026

It should not be necessary to await loading of the stylesheet before loading TipTap Extensions. So this makes them load in parallel.


This item has been added to our backlog AB#68798

Copilot AI review requested due to automatic review settings May 31, 2026 18:40
@nielslyngsoe nielslyngsoe requested a review from iOvergaard May 31, 2026 18:40
@nielslyngsoe nielslyngsoe enabled auto-merge (squash) May 31, 2026 18:41
@claude
Copy link
Copy Markdown

claude Bot commented May 31, 2026

Claude finished @nielslyngsoe's task in 5m 56s —— View job


PR Review

Target: origin/v17/dev · Based on commit: f46a66c

Refactors umb-input-tiptap to load stylesheets concurrently with extensions, and consolidates all stylesheet-related logic into #loadStylesheetPath() (previously split across #loadStylesheetPath() and #loadEditor()).

Important

  • input-tiptap.element.ts:146: _stylesheets is mutated via .add() — the object reference stays the same, so Lit's @state() change detection (===) never schedules a re-render for this mutation. If #loadStylesheetPath() completes after _extensionStyles is set and the first render has already run, configured stylesheets silently never appear in the shadow DOM. In practice the context observable resolves faster than extension loading, but the code is fragile and the decorator implies reactivity it doesn't deliver for in-place Set mutations. Fix: replace the reference in one assignment — collect hrefs into an array then do this._stylesheets = new Set([...this._stylesheets, ...hrefs]);.

Approved with Suggestions for improvement

The parallelisation is sound and the consolidation of stylesheet logic is a clean improvement. Please address the @state() + Set mutation to ensure correctness if loading order ever changes.


Labels applied: area/frontend, category/performance

@nielslyngsoe nielslyngsoe added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label May 31, 2026
@nielslyngsoe nielslyngsoe changed the title TipTap: Let the stylesheet load parallel to tiptap-extensions TipTap: Load stylesheet parallel to extensions May 31, 2026
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — see inline comment below and summary in the Claude comment.

stylesheet.startsWith('http') || stylesheet.startsWith(this.#stylesheetRootPath)
? stylesheet
: `${this.#stylesheetRootPath}${stylesheet}`;
this._stylesheets.add(linkHref);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Important] _stylesheets is mutated via .add() — the object reference stays the same, so Lit's @state() change detection (===) never schedules a re-render for this mutation. If #loadStylesheetPath() happens to complete after _extensionStyles is set and the first render has already run, configured stylesheets silently never appear in the shadow DOM. In practice the context observable resolves faster than extension loading, but the code is fragile and the decorator implies reactivity it does not deliver for in-place Set mutations. Suggestion: replace the reference in one assignment so Lit detects the change correctly. For example: collect hrefs in a plain array then do this._stylesheets = new Set([...this._stylesheets, ...hrefs]);

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors <umb-input-tiptap> so that resolving the stylesheet root path and building the configured stylesheet <link> list runs in parallel with Tiptap extension loading, rather than blocking it.

Changes:

  • Removes the await on #loadStylesheetPath() in firstUpdated() so it executes concurrently with #loadExtensions()/#loadEditor().
  • Moves the configured-stylesheets URL building from #loadEditor() into #loadStylesheetPath() (after the root path observable resolves).
  • Promotes the #stylesheets private field to a reactive @state() _stylesheets and updates #renderStyles() to read it.

Comment on lines +140 to +148
if (stylesheets?.length) {
stylesheets.forEach((stylesheet) => {
const linkHref =
stylesheet.startsWith('http') || stylesheet.startsWith(this.#stylesheetRootPath)
? stylesheet
: `${this.#stylesheetRootPath}${stylesheet}`;
this._stylesheets.add(linkHref);
});
}
@claude claude Bot added the category/performance Fixes for performance (generally cpu or memory) fixes label May 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/frontend category/performance Fixes for performance (generally cpu or memory) fixes state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants