build(sdkjs): fix webpack migration bundle bloat, CI, and coverage gaps

[MonaAghili]

Summary

Fixes four bugs/gaps found in review of the Grunt→webpack migration (feature/migration-to-webpack-sdkjs), scoped strictly to build tooling / CI — no application code is touched in this PR.

Fixes

1. Terser was duplicating every file's license header into the bundle

build/webpack.sdk.factory.mjs configured Terser to preserve comments matching /AGPL|Copyright|Ascensio|License/i, intending to keep just the single license banner injected by BannerPlugin. But every one of the ~400+ concatenated source files carries an identical AGPL/Copyright header, so the regex preserved all of them instead of just the one banner.

Verified before/after on the word bundle: 307 → 1 occurrences of the copyright text in sdk-all.js, banner still present and intact.

Fix: added a unique sentinel (@@license-banner@@) to build/license.header and match only that in Terser's format.comments.

Note: BannerPlugin injects the banner before Terser runs (webpack stage ADDITIONS (-100) vs Terser's OPTIMIZE_SIZE (400)), so a naive comments: false strips the banner too — confirmed by building and grepping the output. The sentinel is required, not optional.

2. Desktop/mobile silently lost their lighter minification tier

Old build-desktop.bat/build-mobile.command ran Closure's WHITESPACE_ONLY; the new pipeline funneled all platforms through the same Terser compress pass, meaning desktop/mobile now get more aggressive minification than before with no explicit decision behind it.

Fix: compress is now false for SDK_PLATFORM=desktop|mobile, restoring the old lighter tier. Verified by building with SDK_PLATFORM=desktop: output is larger than the web build (2.44 MiB vs 2.29 MiB min chunk), confirming the lighter pass is active.

3. check-build.yml PR trigger filter was dropped

pull_request.branches (fork, develop, release/**, hotfix/**) was removed during the CI-runner rewrite, so the workflow now runs on PRs targeting any branch. Restored.

4. Only word had a compiled-bundle regression test

The only CI check that runs QUnit against the actual webpack-built output (tests/common/api/api.html, under COMPILED=1) covered word only. cell/slide/visio had no equivalent — a bundle-specific regression (bad concat order, a stripped directive, etc.) in any of those three could pass CI undetected.

Verified the four editors' bootstrap ("min") chunks are genuinely distinct file sets (word: 42 files incl. pdf/api.js; cell: 38 files incl. cell/api.js, cell/model/CollaborativeEditing.js; etc.) — so this isn't redundant coverage.

Fix: added tests/common/api/api-cell.html, api-slide.html, api-visio.html, reusing the existing api.js test (it only exercises editor-agnostic AscCommon.* APIs, not AscWord/AscCommonExcel, so it's safe against the documented COMPILED=1 limitation), wired into the same CI step.

Also documented (no functional change)

• output.publicPath is intentionally unset today (no code-splitting/import() in this config); added a comment so the next person adding splitChunks knows to set it then.

Test plan

• npm test --prefix build — 32/32 build-tooling unit tests pass
• npm run build --prefix build — all four editors (word/cell/slide/visio) build successfully
• SDK_PLATFORM=desktop npm run build --prefix build — builds successfully, confirms lighter minify branch is exercised
• COMPILED=1 npm run develop --prefix build — regenerates all four scripts.js, each correctly bootstrap-only
• New smoke-test HTML files verified to resolve to real on-disk bundles
• check-build.yml YAML validated
• CI run on this PR (Puppeteer/QUnit execution not run locally — no root node_modules in this environment)

Out of scope

Four other findings from the same review are not included here since they're unrelated application-code changes riding along in the migration branch (a spreadsheet checkbox feature, its CControl controller-initialization guards, dead code in TableId.js, and a branding string change) — these belong in a separate PR reviewed on their own merits.

[moodyjmz]

TL;DR

Approve with reservations. Build-tooling-only PR (no editor/application code touched); the four bugs it claims to fix are real fixes. One concrete defect ships in every output file and should be fixed before merge: the @@license-banner@@ sentinel added to distinguish the license banner from per-file headers is never stripped, so it's now visible in every shipped bundle and asset. I also traced through the new cross-chunk global-sharing model empirically (built the real webpack config, no fabricated claims) — it's not broken today, but it depends on an undocumented webpack optimization that a future change (code-splitting, an external, import()) would silently break, with no test currently able to catch it. build/DEVELOPER-GUIDE.md is referenced four times and doesn't exist. Details below, ranked by severity.

Full review

1. Every shipped file's license banner literally contains the text @@license-banner@@ — confirmed, trivial fix

build/license.header:1 adds the sentinel /* @@license-banner@@ so Terser's format.comments regex can distinguish the single injected license banner from the ~400 per-source-file AGPL headers that also match /AGPL|Copyright|Ascensio|License/i (this is bug #1 the PR fixes — good catch on the original problem). But nothing strips the sentinel back out before the text ships:

• build/webpack.sdk.factory.mjs builds licenseText via four .replace() calls (@@AppCopyright, @@PublisherUrl, @@Version, @@Build) and passes it verbatim into webpack.BannerPlugin({ banner: licenseText, raw: true }). Terser is then told to preserve comments matching /@@license-banner@@/ — i.e. keep it verbatim, sentinel included.
• build/scripts/deploy-assets.cjs does the same four replaces, then does licenseText + '\n' + code for every individually-deployed JS file (device_scale.js, path-boolean-min.js, etc.).

A whole-tree grep for license-banner returns exactly two hits (the header source and the Terser preserve-regex) — no strip step anywhere. Every sdk-all.js, sdk-all-min.js, and every individually-deployed JS asset will visibly show @@license-banner@@ in its header comment, forever.

Fix: strip the sentinel out of licenseText after it's built (before injecting), and keep it only in the separate string Terser matches against — don't conflate "the marker used to find the banner" with "the text that ships."

2. The cross-chunk global-sharing model rests on an undocumented, untested webpack optimization — not a bug today, but a landmine

sdkjs relies on ~287 places (cell/view/EventsController.js, word/Drawing/DrawingDocument.js, pdf/src/viewer.js, etc.) reading a bare GlobalSkin global with zero window.GlobalSkin assignment anywhere in the codebase — by design (common/skin.js's own comment: "не скрываем переменные, скин используется напрямую в sdk-all.js" — "we don't hide the variables, the skin is used directly in sdk-all.js"). This only works because a top-level var in a plain <script> tag becomes a window property automatically.

I flagged this initially as broken, on the theory that webpack always wraps module code in a function regardless of output.iife. I verified this empirically by building the actual config with the real webpack (borrowed from ../web-apps/build/node_modules, no new downloads) — that premise turned out to be wrong for this specific setup: with iife:false and a single module with no imports (exactly what sdk-concat-loader produces for each chunk), webpack inlines the code at true top level with no wrapper, so bare vars still land on window, matching the old Grunt output exactly.

So it works — but only because of that single-module/no-runtime special case. The moment this config gains import(), splitChunks, or an external, webpack switches to its wrapped bootstrap form and all 287 bare-global references would silently break at runtime (TypeError: Cannot read properties of undefined), with a green build and no warning. Nothing in the code currently connects this scoping dependency to the publicPath/no-code-splitting comment that's already sitting right next to it in webpack.sdk.factory.mjs.

Suggestion: add a comment on output.iife: false (or near it) spelling out that correctness here depends on staying single-module/no-runtime per chunk, so the next person touching this config doesn't unknowingly break it.

3. The CI check meant to catch exactly that kind of regression never actually ran on this PR

.github/workflows/check-build.yml's pull_request.branches filter (fork/develop/release/**/hotfix/**) doesn't include main — and this PR's base branch is main. So neither the unit-tests job nor the new build-test job (which includes the COMPILED=1 QUnit smoke test added here as bug-fix #4) ran in CI for this PR. The test-plan checkbox for it is unchecked in the PR body ("CI run on this PR — Puppeteer/QUnit execution not run locally, no root node_modules in this environment").

Separately: even if it had run, tests/common/api/api.js only exercises AscCommon.*/AscUrlType.* (explicit-namespace APIs) — it never touches GlobalSkin or any other bare-global-style symbol, so it wouldn't have caught the scenario in finding #2 anyway.

Net effect: the coverage gap this PR claims to close (bug #4) is real and a good fix, but it's narrower than the description implies, and unverified for this specific submission. Recommend actually running COMPILED=1 npm run --prefix build develop + the new api-cell/slide/visio.html suites locally before merge, rather than relying on the (currently non-triggering) CI job.

4. build/DEVELOPER-GUIDE.md is referenced four times but doesn't exist anywhere in the PR or on main

AGENTS.md (twice), build/Readme.md (once), and — this one's worse — build/scripts/build-pipeline.cjs, which prints the path in a user-facing error message shown to anyone who passes a stale Grunt CLI flag. Either add the file or drop the references.

5. Minor: Babel transpile cache key has two staleness gaps (low severity, dev-loop impact only)

build/loaders/sdk-concat.cjs's BABEL_OPTIONS_KEY is a hand-maintained JSON literal mirroring the actual options passed to transpileToES5, despite the adjacent comment implying it's derived from them — if someone edits the Babel preset/target options without updating this constant, cached entries go stale silently. It also doesn't fold in the @babel/preset-env package version, so a dependency bump alone (loader file unchanged) would keep serving stale cached transpiles. Cheap fix: include require('@babel/preset-env/package.json').version in the key.

Checked and held up (no issue found)

• compress: false restore for desktop/mobile (bug Update from master branch in upstream repository #2) — correct, verified against the Terser options.
• check-build.yml branch-filter restore (bug feat(smartpicker): API extension to support plain text insertion in word documents #3) — correct in isolation; just doesn't help this PR per finding feat(smartpicker): API extension to support plain text insertion in word documents #3 above.
• Filesystem cache: no cross-module (word/cell/slide/visio) collision — webpack namespaces its cache by the factory's own name: field per compiler.
• .docker/sdkjs.bake.Dockerfile: SDK_PLATFORM=desktop is correctly scoped to the parallel sdkjs-desktop stage only; doesn't leak into the web stage.
• assertSafeBuildRoot in build/scripts/build-pipeline.cjs: realpath-resolves symlinks, rejects the checkout/build dir and any ancestor containing the source tree — no bypass found.

[MonaAghili]

@moodyjmz please review again.

[moodyjmz]

TL;DR

Approve. All five findings from my 23 July review are properly fixed — I verified each against the diff rather than taking the re-review request on trust, and CI is green on the exact head SHA (c3abc4d121, run 30011288062, event=pull_request), which also independently demonstrates that finding #3 is resolved rather than merely claimed. The sentinel fix in particular is done the right way round: I re-derived webpack's stage constants from its own source to confirm the strip provably runs after Terser, and enumerated every consumer of license.header to confirm there's no third leak path.

My remaining reservations are not about this PR's code. They're merge sequencing: a certain file collision with #68, and the interaction with the 584-commit upstream sync sitting in #21. Details below.

Verification of the five earlier findings

#1 sentinel leaking into every shipped file — fixed, and fixed correctly.

StripLicenseSentinelPlugin (build/webpack.sdk.factory.mjs:122) strips post-minification; build/scripts/deploy-assets.cjs:55 does the same for individually-deployed assets. Two things I checked rather than assumed:

• Stage ordering holds. Read from webpack 5.107.2's own lib/Compilation.js: PROCESS_ASSETS_STAGE_ADDITIONS = -100, OPTIMIZE_SIZE = 400, DEV_TOOLING = 500, REPORT = 5000. BannerPlugin injects at −100, Terser runs at 400, the strip runs at 5000. Correct by construction, and the inline comment explaining why it can't strip before injection is accurate.
• No third consumer. I enumerated all 30 build-directory scripts and grepped each for license.header / license-banner. Exactly two readers (deploy-assets.cjs:46, factory.mjs:179), both stripping. Nothing else can leak it.

#2 bare-global fragility — addressed as suggested. The WARNING block above iife: false names the three triggers (import(), splitChunks, externals) and states the failure mode. That's exactly what was needed.

#3 CI not running on this PR — fixed and proven. main added to both push.branches and pull_request.branches. All four checks now green on the head commit, so the COMPILED=1 QUnit coverage added by bug-fix #4 has actually executed.

#4 build/DEVELOPER-GUIDE.md — added (261 lines). The benchmark table and the HMR-impossibility rationale are a genuinely useful addition beyond just satisfying the dangling references.

#5 Babel cache key — fixed. presetEnvVersion: require('@babel/preset-env/package.json').version folded into BABEL_OPTIONS_KEY, and the comment now describes what the code actually does rather than what it aspired to.

New since my last pass, checked: c3abc4d121 sets SDK_ADDONS=../../sdkjs-forms. Path arithmetic is right — npm run --prefix build puts cwd at sdkjs/build, so ../../sdkjs-forms resolves to the workspace-root sibling checkout.

Looked like a bug, isn't — recording so nobody "fixes" it later. The sentinel strip at stage 5000 runs after SourceMapDevToolPlugin at stage 500, so it mutates the .js after its map is computed. Harmless: it removes 19 characters from line 1 and no newline; line 1 is the injected banner comment, which carries no mappings; later lines don't shift. And since output filenames are fixed (sdk-all.js) rather than [contenthash], the realContentHash pass at stage 2500 isn't affected either.

Merge sequencing — two collisions

1. .eslintignore is a certain add/add conflict with #68. This PR creates it (16 lines: deploy/, vendor/, pdf/build/, tests/, …). #68 also creates it (one line: common/Native/jquery_native.js). Disjoint content, same new file — whichever lands second conflicts, and the reflexive "take theirs" resolution silently discards the other side's intent.

Worth knowing: the .eslintignore added here is currently dead configuration. Nothing in this PR runs eslint — check-build.yml's code-style job is still only python tests/code-style/check.py. #68 is what actually invokes eslint. So the merged file wants the union of both lists, not either one alone.

2. check-build.yml overlaps #68, probably benignly. This PR doesn't touch the code-style job at all (only the on: triggers and the other two jobs), whereas #68 appends two steps to it. Different regions, so git should cope — though #68's insertion point sits immediately before unit-tests:, which this PR does modify, so the context lines may brush. Lower confidence than the .eslintignore collision, which is certain.

The bigger sequencing question

main vs upstream master:      ahead_by=584  behind_by=76  status=diverged
#21 (update-from-master):     CONFLICTING / DIRTY, open since 2026-05-19

This PR deletes build/Gruntfile.js (561 lines, upstream-owned) and rewrites the build layer. Landing it before the upstream sync in #21 turns the build-layer portion of that already-conflicted 884-file PR from "hard" into "nobody will hand-resolve this."

I'd merge #21 first, then this. Not a criticism of the work — a consequence of the order.

Related: #66 (chore(deps): update dependency grunt to v1.6.3) directly contradicts this PR. It bumps grunt in build/npm-shrinkwrap.json; this PR deletes the Gruntfile and rewrites that lockfile. One of the two is waste regardless of the outcome. Suggest closing #66 if this lands.

Caveats on this review

I did not run an independent second pass on this one. The conclusion I'd most want a cold reader on is my earlier finding #2 — the webpack single-module inlining behaviour that the bare-global model depends on. That rests on empirical build observation rather than plain source reading, and the WARNING comment added here is only as good as that premise.

Disclosure: #67 and #68 are mine, so read the sequencing notes with that in mind.

Nice work on the fixes — the sentinel one especially, since the obvious shortcut (strip before injection) would have quietly re-broken bug #1.

[moodyjmz]

Request: port web-apps' verify stage

Not a blocker, and not a criticism of the approach — the opposite. Now that I understand this is deliberately modelled on web-apps rather than a fresh toolchain choice, the pattern-match holds up well: per-product configs plus a shared factory (webpack.{word,cell,slide,visio}.mjs + webpack.sdk.factory.mjs ↔ webpack.{documenteditor,spreadsheeteditor,…}.mjs + webpack.editor.factory.mjs), and build-pipeline.cjs ↔ build-pipeline.js. Even the mangle: false decision matches, for the same reason — web-apps' factory documents it as "117 source files use var Common = Common || {}", and sdkjs has the same bare-global exposure.

But the copy stops one phase short. web-apps has a verification tier this PR doesn't:

web-apps/build/scripts/
  verify-bundles.mjs
  verify-deploy.mjs
  verify-browser-target.mjs
  verify-replacements.mjs

verify-replacements runs as a Preflight phase before any build (build-pipeline.js:246, with the rationale "no point building if load-bearing idioms have drifted"), and the rest run as a post-build phase. build-pipeline.cjs here has the deploy-assets/webpack phase and the build-develop phase, then finishes.

The specific ask: an sdkjs verify-deploy equivalent. The highest-value assertion for this repo is that every path referenced by DocumentServer/build/configs/core/DoctRenderer.config actually exists in deploy/ after a build. That config is what the converter reads at runtime — core:DesktopEditor/doctrenderer/config.h:117-140 parses the <file> entries into m_arrFiles, and editors.cpp:69-75 (GetAllScript) concatenates them ahead of sdk-all.js. A missing or renamed entry there produces a converter that fails at runtime with a green build, which is the exact failure class this repo just spent a week on (see Euro-Office/DocumentServer#307 — a converter-only defect in common/Native/native.js that no browser test could ever have caught).

Worth noting Native/*.js is in this PR's deploy-assets.cjs path ('Native/*.js', and neither native nor native_graphics is in IGNORE_NAMES), so those files' handling changes here. A verify-deploy wouldn't validate their contents, but it would at least assert they're present and where DoctRenderer.config expects them.

Cheap version if a full port is too much scope for this PR: a single script that parses DoctRenderer.config for <file> entries, resolves each against deploy/, and exits non-zero on the first miss. Wire it as a step after Run build sdkjs in check-build.yml. Happy for that to be a follow-up PR rather than growing this one — say which you'd prefer.

---

Separately, and flagging it now rather than springing it later: there's a second gap I'd like to discuss, which is that nothing in this PR's CI exercises DoctRenderer or x2t at all — every check is browser QUnit via node-qunit-puppeteer, while deploy-assets.cjs adds a Babel preset-env {targets:{ie:'11'}} pass over files including the Emscripten glue that contains async/await (common/zlib/engine/zlib.js, common/spell/spell/spell.js, common/hash/hash/engine.js, common/libfont/engine/fonts.js, pdf/src/engine/drawingfile.js). That may well be like-for-like with what Closure's --rewrite_polyfills=true (build/Gruntfile.js:460-463) was already doing — I haven't established whether it's a new risk or a faithful port, and I don't want to raise it as an objection until I have. Mentioning it so it's not a surprise. Euro-Office/sdkjs#34 (the e2e overlay job) looks like the natural gate for it.

[moodyjmz]

IE support is being dropped — two consequences for this PR

@MonaAghili — decision from our side: we can drop IE support entirely. That resolves a question left open in the earlier review and has two consequences here, one immediate and one worth a discussion.

1. The Babel ES5 pass can go — independent of anything else

build/scripts/deploy-assets.cjs runs @babel/preset-env with {targets:{ie:'11'}} over every non-ignored JS file. IGNORE_NAMES covers only the _ie variants, so the non-_ie files go through it — including the Emscripten glue that contains async/await: common/zlib/engine/zlib.js, common/spell/spell/spell.js, common/hash/hash/engine.js, common/libfont/engine/fonts.js, pdf/src/engine/drawingfile.js. common/Native/native.js and native_graphics.js are in that path too.

With no IE target, that transpile has no consumer. Removing it:

• deletes an async→regenerator downlevel that runs on the DoctRenderer path with no @babel/runtime present — the sharpest unverified risk in the diff, and the one I could not resolve in review
• removes @babel/* from the build dependency set
• drops the BABEL_OPTIONS_KEY cache-staleness surface in loaders/sdk-concat.cjs entirely

This is a subtraction, needs no toolchain change, and I would take it on its own merits.

2. With ES5 off the table, esbuild becomes viable

Three of the more intricate parts of this PR exist to make webpack behave like a concatenator: output.iife: false plus the single-module sdk-concat-loader (so bare top-level vars reach window — the ~287 GlobalSkin-style reads), StripBootstrapStrictModePlugin, and BannerPlugin + the @@license-banner@@ sentinel + StripLicenseSentinelPlugin.

esbuild 0.25.12 does all three natively:

$ esbuild sdkjs-like.js --minify-whitespace --minify-syntax --banner:js='/* LICENSE BANNER */'
/* LICENSE BANNER */
var GlobalSkin={RulerDark:"#0f0f0f"},AscCommon={};window.AscCommon=AscCommon,...

True top level, no wrapper, identifiers untouched, banner prepended — no sentinel required. The three minify flags are separate (--minify-whitespace / --minify-syntax / --minify-identifiers), so mangle: false is a native mode rather than a Terser options object.

ES5 was the blocker and it is now gone:

✘ [ERROR] Transforming let to the configured target environment ("es5") is not supported yet
✘ [ERROR] Transforming async functions to the configured target environment ("es5") is not supported yet

Caveats, so this is a fair comparison rather than a pitch:

• My test was a synthetic file mimicking sdkjs's bare-global pattern, not a real 32 MB sdk-all.js. Indicative only.
• --define:AscCommon.g_cIsBeta='"false"' did not substitute — esbuild will not rewrite a member expression rooted at a symbol declared in the same file. Safer than DefinePlugin (same root cause as the LHS-assignment breakage your DEVELOPER-GUIDE documents), but defines would need doing as a text pass pre-concatenation. Upstream does exactly that: build.py:146 apply_defines(), a regex over window.AscCommon.<name> = "...".
• I have no size or wall-clock numbers against your actual output.

The parallelism win and the per-product-config + shared-factory pattern stand either way — those are the strongest parts of this PR and I am not questioning them. The question is narrower: whether the concatenation half is worth ~5,500 lines of webpack machinery now that the ES5 constraint is gone.

Your call on whether that is worth exploring in this PR, a follow-up, or not at all. If the webpack route is still better on grounds I have not considered, I would rather hear that than have you re-open a settled decision.

3. Follow-up, needs care

The _ie variants (zlib_ie, spell_ie, engine_ie, drawingfile_ie, fonts_ie) become dead weight with IE dropped. DoctRenderer.config references none of them, so the converter path is unaffected — but something selects between _ie and non-_ie at runtime, so worth tracing before removal. Separate PR.

[moodyjmz]

Copyright headers on the new build files

Separate, small point — and to be clear up front, this is inherited from web-apps rather than anything introduced here. I've raised the equivalent issue against web-apps so it gets fixed in both places.

Every Euro-Office-authored file added by this PR carries:

 * (c) Copyright Ascensio System SIA 2010-2024

build/lib/env.cjs, build/scripts/build-pipeline.cjs, build/scripts/deploy-assets.cjs, build/webpack.sdk.factory.mjs, build/loaders/sdk-concat.cjs, build/webpack.{word,cell,slide,visio}.mjs and the rest. These are new files written in 2026; Ascensio System has no authorship claim on them, and the date range is stale even by ONLYOFFICE's own current headers (2009-2026 / 2012-2025).

There's a CI catch that has to be fixed first. tests/code-style/check.py:37:

license_header = b'Copyright Ascensio System'

check_file_without_license() raises if any file lacks that exact string — and this PR extends that check to .cjs/.mjs. So right now a new Euro-Office file cannot pass code-style without attributing itself to Ascensio System. The header can't be corrected until the check is.

Suggested order:

1. Widen check.py to accept either an Ascensio header or an SPDX identifier. It can't simply be flipped — the ~400 genuine upstream source files legitimately carry Ascensio headers and should keep them. The check's purpose is "no unlicensed files", which either form satisfies.
2. Replace the headers on the files added here with an SPDX Euro-Office header, e.g.

/**
 * SPDX-FileCopyrightText: 2026 Euro-Office contributors
 * SPDX-License-Identifier: AGPL-3.0-or-later
 */

I couldn't find an established SPDX example in the Euro-Office tree to copy exactly, so treat that string as a proposal rather than the house form — worth confirming before applying it across files.

Note there's already a dual-attribution string in this PR at build/scripts/deploy-assets.cjs:43, used for generated output banners:

Copyright (C) Ascensio System SIA 2012-2025. All rights reserved; Euro-Office contributors 2026 - <year>

That's the right shape for generated bundles, which genuinely contain both parties' work. For new source files that contain only Euro-Office work, a plain Euro-Office header is the accurate one.

Happy for this to be a follow-up rather than growing this PR — but the check.py widening is a two-line change and it unblocks everything else.