CI: mobile e2e smoke + drop SKIP_MOBILE from Makefile

[moodyjmz]

Closes #261.

• Bumps web-apps to pick up docs: Add SECURITY.md #158 (removes SKIP_MOBILE), then drops the now-dead pass-through from develop/setup/Makefile.
• Adds e2e/tests/mobile-editor.spec.ts: a runtime smoke test for docx/xlsx/pptx/pdf mobile editors, catching the class verify-deploy.mjs can't — artifacts deploy but the editor breaks at runtime (Regression: mobile editor doesn't load in latest-dev #258).

Scope

The gate verifies the mobile build is present and renders a document. It keys on the loading skeleton (.doc-placeholder) clearing — the real "document loaded" signal — rather than the canvas, which mounts with the SDK bundle before any document loads and so can't distinguish "loaded" from "never loaded".

Infra faults (e.g. an unresponsive docservice channel) are out of scope: CI runs a healthy docservice from the same image and won't produce them. A known false-pass in that mode is deferred — closing it properly needs a content-bearing sample file rather than a blank new document.

Test plan

• Existing desktop suite (example-page.spec.ts) unaffected
• New mobile suite passes on a working build (4/4)
• Reds on a missing mobile bundle (.doc-placeholder never clears)

[MonaAghili]

PR #262 Review Issues

Issue 1 — Canvas toBeVisible is not a loading-completion signal

mobile-editor.spec.ts lines 54–55:

await expect(frame.locator(editor.canvasSelector)).toBeVisible({ timeout: 30_000 });
await expect(frame.locator(ERROR_SURFACE_SELECTOR)).toHaveCount(0, { timeout: 30_000 });

The test's own comment admits #id_viewer / #ws-canvas are "part of the static UI scaffold" that stay visible even when the document fails to load. So the toBeVisible check only confirms the React app shell mounted — it says nothing about whether the document loaded. The real gate is the toHaveCount(0) line below it.

This matters when the test fails: Playwright reports "canvas not visible" when the actual problem is likely an error dialog or a stuck preloader. Compare to the desktop test (example-page.spec.ts) which checks #loading-mask toBeHidden — a direct "document finished loading" signal. If a mobile equivalent selector exists, it would give clearer diagnostics and closer parity with the desktop suite.

---

Issue 2 — Two sequential 30 s timeouts against a 60 s global

Note: You did not introduce the 60 s global timeout — that is pre-existing in playwright.config.ts and unchanged by this branch. This issue is flagged because his new test introduces the two back-to-back timeout: 30_000 assertions that, combined with the existing ceiling, leave no margin. The problem only exists because of what he added; fixing it is his to do.

playwright.config.ts sets timeout: 60_000 per test (pre-existing). The new mobile tests chain two assertions that each allow up to 30 s:

// step 1 — up to 30 s
await expect(frame.locator(editor.canvasSelector)).toBeVisible({ timeout: 30_000 });
// step 2 — up to 30 s
await expect(frame.locator(ERROR_SURFACE_SELECTOR)).toHaveCount(0, { timeout: 30_000 });

Worst-case: step 1 takes 29 s (slow container, slow JS parse), step 2 takes 29 s (slow document load) → 58 s total before the test even returns. Playwright's global timer is already at the ceiling. In practice the canvas check resolves quickly, but on a degraded self-hosted runner there is no margin. Either reduce the canvas timeout (e.g. 15_000, matching the global expect.timeout in config) or raise the per-test timeout for this describe block via test.setTimeout.

[Aiiaiiio]

Nice one! This verbose commenting will make this a bit hard to maintain should anything change that is stated here. But at least it's obvious what is happening.

[moodyjmz]

@MonaAghili — thanks for the review. Both points addressed in 4e3a267 (landed after your comment, so it's not visible in the version you read):

Issue 1 (canvas isn't a load-completion signal): the gate is now .doc-placeholder, not the canvas. That's the real load signal — it renders while !isDocReady and is removed once the doc loads (apps/documenteditor/mobile/src/page/main.jsx:56), making it the mobile analog of the desktop suite's #loading-mask, which is the parity you asked for. Canvas toBeVisible is demoted to a trailing 5 s sanity check. A stuck load now fails as "placeholder never cleared" rather than the misleading "canvas not visible".

Issue 2 (two back-to-back 30 s timeouts): gone. Sequence is now 15 s (placeholder appears) + 30 s (placeholder clears) + 5 s (error surface) + 5 s (canvas) = 55 s worst case, under the 60 s global. Only the genuinely-slow document-load step keeps 30 s; the tail checks dropped to 5 s.

Mind re-reviewing?

[MonaAghili]

Hi @moodyjmz
These are some findings for you to investigate

PR Review: ci/mobile-e2e-smoke

Resolves #261 ("CI: mobile e2e smoke + drop SKIP_MOBILE from Makefile").

Scope reviewed: develop/setup/Makefile (SKIP_MOBILE removal), e2e/tests/mobile-editor.spec.ts (new), web-apps submodule pointer bump (content already merged to main via PR #158, no new diff to review). Uncommitted local changes to develop/README.md are not part of this branch's commits and are out of scope.

Acceptance criteria (from #261)

• ✅ "A mobile editor that loads to a blank/404 page fails e2e." Satisfied, but indirectly: the test doesn't assert on HTTP status or page emptiness directly. It fails via timeout when .doc-placeholder never appears/clears or the canvas selector never becomes visible — a blank/404/stuck-loading mobile page would hang on one of those expect(...).toBeVisible()/toHaveCount() calls until its timeout, which Playwright reports as a failure. Functionally equivalent to the ask, just implemented as "the positive signal never arrives" rather than "the negative signal is detected."
• ✅ "develop/setup/Makefile no longer references SKIP_MOBILE." Confirmed clean — no occurrences left in the Makefile or anywhere else in the repo.
• Deviation, but intentional and reasonable: CI: mobile e2e smoke + drop SKIP_MOBILE from Makefile #261 says "opens a mobile editor URL with a mobile UA / viewport." The PR uses ?type=mobile + a mobile viewport, not a mobile User-Agent. Checked web/documentserver-example/nodejs/app.js:1144-1150 — UA sniffing only runs when ?type= is absent; passing type=mobile explicitly is a more deterministic way to force the same code path a real mobile UA would hit, so this satisfies the intent (verify the mobile bundle loads) without depending on brittle UA emulation.
• Note on scope vs. estimate: CI: mobile e2e smoke + drop SKIP_MOBILE from Makefile #261 estimates "cost: ~one editor load," but the PR added four (docx/xlsx/pptx/pdf). The issue's description of Regression: mobile editor doesn't load in latest-dev #258 ("shipped a broken mobile editor") isn't editor-specific, so covering all the deployable mobile editor types is a defensible reading of "does it actually load" rather than scope creep — worth being aware of as a 4x CI-time increase versus the original estimate, not a defect.

Findings

1. Medium — Dangling reference to a file that doesn't exist, mobile-editor.spec.ts:19

The comment cites cm-findings/DocumentServer/pr-262-ws-hang-false-pass.md as the source for scoping "infra faults are out of scope." I searched the full repo history and working tree — this file has never existed in this repo. If it lives in an external tracker, the comment should say so explicitly; as written it reads as a repo-relative path a future reader will find for and not locate.

Why it matters: comments citing evidence should be verifiable by the next engineer; an unverifiable/broken citation undermines the justification for the scoping decision it's attached to.

Suggested fix: either commit the referenced findings doc alongside this change, or reword the comment to point at the actual location (issue link, PR discussion, etc.) rather than a path.

2. Low — Potential flakiness on fast-loading documents, mobile-editor.spec.ts:39-40

await expect(placeholder.first()).toBeVisible({ timeout: 15_000 });
await expect(placeholder).toHaveCount(0, { timeout: 30_000 });

If the document loads fast enough that the placeholder mounts and clears entirely between Playwright's polling ticks, the first assertion could time out looking for something already gone (never observed as visible). Given real conversion/network latency this is unlikely in practice, but it's a real theoretical race the "gate on skeleton, not canvas" fix (per the commit message) introduces as a side effect of the more precise signal.

Suggested fix: consider asserting not.toHaveCount(0) OR already-cleared as an acceptable pass (e.g., poll for "either was visible-then-gone, or already gone") if this proves flaky in CI; not blocking as-is.

3. Low — Visio (vsdx) coverage gap is disclosed but unaddressed

The comment correctly documents that vsdx is excluded because there's no blank create-new template, and notes the build-time check still gates the visio mobile bundle. Confirmed directly: build/scripts/verify-deploy.mjs:50 — MOBILE_EDITORS = ['documenteditor', 'spreadsheeteditor', 'presentationeditor', 'visioeditor'] — visio is in that list, pdfeditor is not (consistent with finding elsewhere that pdfeditor has no mobile bundle at all). This is an honest, accurate limitation — flagging only because it means the runtime "#258 class of failure" (deploys but fails at runtime) is not actually covered for visio by this PR, unlike the other four editors. Not a defect in the diff, just a residual gap worth tracking in a follow-up issue if not already.

Verified as correct (checked against actual web-apps source at the bumped submodule commit, not assumed)

• .doc-placeholder exists in documenteditor, presentationeditor, and spreadsheeteditor mobile bundles (page/main.jsx + less/app.less) for the docx/pptx/xlsx rows.
• The PDF row is correctly modeled: pdfeditor has no mobile bundle at all in web-apps, but apps/api/documents/api.js's getAppPath() routes type=mobile + fileType pdf to documenteditor (appType stays 'word' when corrected_type === 'mobile'), which is why canvasSelector: '#id_viewer' for PDF is right, not a mistake.
• #id_viewer / #ws-canvas aren't in the web-apps JSX/less at all — they're injected at runtime by sdkjs (cell/api.js, common/text_input2.js), consistent across mobile/desktop, so the selectors are legitimate rather than guessed.
• SKIP_MOBILE removal in the Makefile is clean — no other references anywhere in the repo (docs, CI, other Makefiles) rely on it.
• The dual error-surface selector (.dialog.modal-in, .notification.modal-in) is accurate: apps/documenteditor/mobile/src/controller/Error.jsx:27 uses f7.notification.create for LoadingScriptError, and line 321 uses f7.dialog.create for the other fatal-error branches — same split confirmed in the other three editors' Error.jsx.

Positive observations

• Good historical iteration: the prior commit (de01619) gated on canvas-mounts, which the author correctly identified as a false-negative risk (#id_viewer mounts with the SDK bundle before any doc loads) and fixed in the follow-up commit — this shows real testing of the test itself, not just writing it once.
• Comments carry real signal (why PDF routes through documenteditor's canvas id, why visio is excluded, why two error surfaces) rather than restating the code.
• Matches the existing sibling spec's (example-page.spec.ts) structure and conventions (iframe assertions, frameLocator usage, table-driven editor types).
• Scoped, minimal Makefile cleanup — removes genuinely dead pass-through with no orphaned references.

Overall assessment

Solid, well-considered addition that closes a real coverage gap (#258-class runtime failures the build-time check can't catch). The one dangling doc reference should be fixed since it's asserted as evidence for a scoping decision; the flakiness point is worth a mental note but not blocking.

[moodyjmz]

@MonaAghili — thanks, addressed in c65c29e and a807886.

Finding 1 (dangling cm-findings/... reference): fixed in a807886. That path is never committed to this repo, so it could never resolve for anyone else — reworded the comment to state the dead-channel limitation directly instead of citing an unreachable file.

Findings 2 (placeholder race) and 3 (Visio gap): agreed non-blocking per your own framing, leaving as-is.

One more thing not in your review: c65c29e drops the PDF row from the test matrix entirely (docx/xlsx/pptx now, no pdf). You'd verified the PDF row as correct, so flagging explicitly rather than sneaking it past you — ?type=mobile coerces PDF's appType to 'word' (api.js), routing it through the same documenteditor/#id_viewer bundle as docx, and there's no pdfeditor/mobile bundle to speak of. It was exercising zero code the Document row doesn't already cover, so cutting it trims CI time without losing coverage. Shout if you disagree.

[MonaAghili]

@moodyjmz
Second pass on top of the earlier review — the placeholder-based gate design is solid and the Makefile cleanup is clean. Found two things I'd want addressed before merge, plus a few non-blocking notes for follow-up.

1. The test can false-pass on a post-load JS exception (blocking)

mobile-editor.spec.ts never registers page.on('pageerror', ...) or a console listener. It only fails if the placeholder never clears or an f7 error dialog appears — an exception thrown after the shell paints (.doc-placeholder gone, canvas visible) passes silently. That's exactly the failure class this gate exists for ("artifacts deploy but the editor breaks at runtime"), so I think this needs an uncaught-exception check before merge, not just as a fast-follow. It'd also fix diagnostics — right now a failure gives you a generic timeout message; the actual JS error is only recoverable by downloading the retry's trace.zip.

Suggested fix in mobile-editor.spec.ts (around the test body, ~line 64-83):

const errors: string[] = [];
page.on('pageerror', e => errors.push(e.message));
// ... existing assertions ...
expect(errors).toEqual([]);

2. This doubles concurrent document-load pressure on one shared container with no worker pinning (blocking)

playwright.config.ts sets fullyParallel: false but not workers: 1, and global-setup.ts starts a single Docker container that every spec file shares. Adding 3 more full document loads that can run in parallel with example-page.spec.ts's 3, against one docservice instance, on a runner the workflow's own comments already call out as contended ("racing the previous one on the single self-hosted e2e runner") — this seems likely to introduce timeout flakiness unrelated to mobile bundle health. Worth pinning workers: 1 in playwright.config.ts (matches the "one shared container" model) or confirming the runner has real headroom for ~2x the conversions.

3. No real mobile device emulation (fast-follow)

mobile-editor.spec.ts:61 only resizes the desktop Chromium viewport (test.use({ viewport: { width: 414, height: 896 } })) — isMobile/hasTouch are never set, so this still isn't a real mobile browser context (no touch input, no WebKit engine). ?type=mobile correctly forces the app's mobile bundle server-side, but that's a separate concern from browser emulation, and touch-path/WebKit-specific bugs stay untested. Consider test.use({ ...devices['iPhone 12'], viewport: {...} }) to close that gap.

4. retries: 1 plus stacked timeouts undermines this as a strict regression gate (fast-follow)

retries: 1 combined with the new test's worst-case ~70s of chained timeouts per editor means a real, deterministic regression costs ~7 min of pure waiting across 3 editors before CI goes red, and a borderline-flaky one gets a free retry — the opposite of what you want from a regression canary. Might be worth retries: 0 for the mobile smoke describe block specifically.

5. No tagging/isolation mechanism for the mobile suite (fast-follow)

Fine at 3 tests, but the PR's own framing suggests more editors will be added here later. Cheaper to add a tag/grep mechanism (@mobile, or a separate Playwright project) now than to retrofit once it's flaky and blocking unrelated desktop-only PRs.

---

Summary

#	Severity	Area	Issue
1	Major	Test correctness	No pageerror/console capture — post-load JS exceptions pass silently
2	Major	CI reliability	New spec adds concurrent load to one shared container with no workers: 1 pin, on a runner already documented as contended
3	Medium	Mobile validation	No real device emulation (isMobile/hasTouch unset) — only viewport resize on Desktop Chrome
4	Medium	CI reliability	retries: 1 + ~70s/editor worst-case timeouts risks masking the exact flakiness this canary should catch, and adds real CI time
5	Medium	Maintainability	No tagging/isolation mechanism for the mobile suite as it's expected to grow

Findings 1 and 2 are the ones I'd push to have addressed before merge; 3-5 are reasonable fast-follows.

[chrip]

Verification

Claim / Item	Reality	Status
SKIP_MOBILE fully removed from develop/setup/Makefile	git show a807886:develop/setup/Makefile — zero occurrences (base had 5)	✓
Dropping SKIP_MOBILE breaks nothing else in DocumentServer	git grep SKIP_MOBILE at head → no DocumentServer-side references (docs, CI, other Makefiles)	✓
#158 genuinely removes SKIP_MOBILE upstream	In new submodule commit 04b95b1: build/scripts/build-pipeline.js and .github/workflows/e2e.yml both have zero SKIP_MOBILE refs (old be0c7c0 still had them)	✓
PR modifies no CI workflow; test is auto-discovered	Diff touches only Makefile, spec, submodule. e2e job runs npm test (build.yml:493) → Playwright picks up tests/*.spec.ts	✓
New test ran green in CI	e2e job = SKIPPED (needs: [build, manifest]; build failed). The new test never executed in CI.	❌
Build failure is caused by this PR	Fails in cluster-docs//core C++ build: Common/3dParty/build_3rdparty.py → V8 gclient_paths.patch "patch does not apply"; cmake v4.3.3 missing; stale vcpkg/nuget cache (libiconv, hunspell). None of the PR's files touch /core, cmake, vcpkg or V8	✓ not the PR's fault (pre-existing infra drift)
.doc-placeholder gate is the right load signal	Author cites apps/documenteditor/mobile/src/page/main.jsx !isDocReady; matches desktop #loading-mask pattern in example-page.spec.ts	✓
PDF row correctly dropped as duplicate of docx bundle	?type=mobile coerces PDF appType to 'word' → routes through documenteditor/#id_viewer; no pdfeditor/mobile bundle. Matrix now docx/xlsx/pptx	✓
Author's "55 s worst-case < 60 s global" reassurance	Omits the two iframe-level checks: toBeAttached 15 s + toHaveAttribute 15 s + placeholder-visible 15 s + placeholder-clear 30 s + error 5 s + canvas 5 s ≈ 85 s nominal, above the 60 s per-test cap — the 60 s global is the real ceiling	⚠️ author's math understated it
DCO / sign-off	DCO check green; every commit Signed-off-by: James Manuel	✓

Issues & Suggestions

🔴 Blocking

• The deliverable has never run in CI. The build job is red on all four legs
  (amd64/arm64 × euro-office/nextcloud-office) and e2e (needs: [build, manifest], build.yml:438)
  was SKIPPED. A PR whose sole purpose is "add a mobile e2e smoke test" cannot be approved while
  that test has never been observed to run — let alone pass — in CI. The build break itself is
  not this PR's fault (it dies in the /core C++ build: V8 gclient_paths.patch no longer
  applies, cmake v4.3.3 missing, and a stale vcpkg/nuget cache — none of which this PR touches),
  but it is a hard gate on verifying the change. Action: get a green build (fix/refresh the core
  build cache on the self-hosted runner, or rebase onto a main where it's green) and produce one run
  where the e2e job actually executes and this spec passes, before merge.
• No pageerror/console capture — @MonaAghili 07-20 #1, still open (mobile-editor.spec.ts).
  The test registers no page.on('pageerror', …) and no console listener. It fails only if
  .doc-placeholder never clears or an f7 error surface appears; a JS exception thrown after the
  shell paints (placeholder gone, canvas visible) passes silently. That is precisely the
  "deploys but breaks at runtime" (#258) class this gate exists to catch, so the test does not fully
  do the job it advertises. It also improves diagnostics (real error message vs a generic timeout).
  The reviewer's suggested const errors: string[] = []; page.on('pageerror', e => errors.push(e.message)); … expect(errors).toEqual([])
  is the right shape. This should land before merge.

⚠️ Major

• No workers: 1 pin — @MonaAghili 07-20 #2, still open. e2e/playwright.config.ts sets
  fullyParallel: false but not workers: 1, and global-setup.ts starts a single shared Docker
  container. This spec adds 3 more full document conversions that can run concurrently with
  example-page.spec.ts's 3 against one docservice, on a runner the workflow header itself flags as
  contended ("racing the previous one on the single self-hosted e2e runner", build.yml:45). Real
  flakiness risk unrelated to mobile-bundle health. Pin workers: 1 (matches the one-shared-container
  model) or confirm runner headroom.

ℹ️ Minor / 💡 Suggestions

• No device emulation — @MonaAghili 07-20 #3, open. The chromium project is Desktop Chrome
  and the spec only resizes the viewport (414×896); isMobile/hasTouch are unset, so touch-path
  and WebKit-specific behaviour stay untested. ?type=mobile forces the mobile bundle server-side,
  which is a different concern from browser emulation. test.use({ ...devices['iPhone 12'] }) closes
  the gap. Fast-follow.
• retries: 1 + stacked timeouts — @MonaAghili 07-20 #4, open. With the 60 s per-test cap and a
  free retry, a genuine deterministic regression costs up to ~2 min/editor before going red — weak
  canary semantics. Consider retries: 0 scoped to the mobile smoke block. Fast-follow.
• No tag/isolation for the mobile suite — @MonaAghili 07-20 #5, open. Fine at 3 tests; cheaper to
  add a @mobile grep tag or separate Playwright project now than after it grows and starts blocking
  desktop-only PRs. Fast-follow.
• Author's timeout reassurance was optimistic. The "55 s < 60 s" reply omitted the two
  iframe-level 15 s assertions; the nominal per-assertion budget (~85 s) exceeds the 60 s per-test
  ceiling, so on a slow-but-eventually-successful load the global timeout — not the tuned
  per-assertion values — is what fires. Not independently blocking (the retries point covers the
  practical impact), just worth correcting the record.
• No SPDX header on the new spec. Consistent with the sibling example-page.spec.ts (also
  headerless), so not a deviation from local convention — noting only for completeness.
• Verbose inline comments (per @Aiiaiiio) — genuinely informative here (why PDF routes through
  documenteditor, why visio is excluded, why two error surfaces), but they encode facts about the
  web-apps bundle that will drift; acceptable, low-priority maintenance note.

Verdict

Request changes — the mobile smoke test has never actually run in CI (build red on all four
legs → e2e skipped), so the PR's entire deliverable is unverified; the build break is unrelated
infra drift in the /core C++ build, but a green run that exercises this spec is a prerequisite to
merge. On top of that, the prior review's strongest still-open point — no pageerror capture, so a
post-load runtime exception (the exact #258 failure class) passes silently — should be fixed before
merge, and the missing workers: 1 pin is a real flakiness risk. The design and the SKIP_MOBILE
cleanup are otherwise solid.

Assisted-by: ClaudeCode:claude-opus-4-8