Skip to content

Chore: Fixes #14834: Mobile: JSDOM scroll polyfill in ExtendedWebView#14896

Open
divyanshkhurana06 wants to merge 3 commits intolaurent22:devfrom
divyanshkhurana06:new-issue-upstream-dev
Open

Chore: Fixes #14834: Mobile: JSDOM scroll polyfill in ExtendedWebView#14896
divyanshkhurana06 wants to merge 3 commits intolaurent22:devfrom
divyanshkhurana06:new-issue-upstream-dev

Conversation

@divyanshkhurana06
Copy link
Copy Markdown
Contributor

Fixes #14834

Problem
Running the mobile test suite could finish with Jest reporting “Cannot log after tests are done” after Note.test.tsx. The underlying error was TypeError: scrollIntoView is not a function inside the ExtendedWebView Jest mock’s nested JSDOM: renderer code schedules scrolling (e.g. hash / afterFullPageRender) on a timer, but JSDOM does not implement layout APIs like scrollIntoView. That led to uncaught errors after the test had already completed.

Separately, the Note screen could leave a focus polling interval running briefly after unmount in tests, which contributed to stray logging / teardown issues.

Solution
Scoped polyfill, not global: scrollIntoView / scrollTo / scrollBy are polyfilled only on the nested JSDOM window used by the ExtendedWebView test mock (and again for iframe contentWindows when the existing iframe polyfill runs). This fixes the renderer timers without patching the global Jest/jsdom Element prototype, which would break tests that assign their own scrollIntoView mock (e.g. index.handleAnchorClick.test.ts).

Lifecycle: clear the Note editor focus update interval in componentWillUnmount so it cannot keep firing after the screen is torn down in tests.

No change to production mobile behaviour; test / mock behaviour only (plus safe unmount cleanup).

Test Plan
yarn workspace @joplin/app-mobile test components/screens/Note/Note.test.tsx
yarn workspace @joplin/app-mobile test contentScripts/rendererBundle/contentScript/index.handleAnchorClick.test.ts

…edWebView mock

Polyfill scrollIntoView/scrollTo/scrollBy only on the nested JSDOM window used by the ExtendedWebView Jest mock (and iframe contentWindows), avoiding global Element patches that break handleAnchorClick tests.

Clear the note focus polling interval on unmount to avoid stray logs and open handles after tests.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 64316d3d-8480-48ff-b3ce-202306985c17

📥 Commits

Reviewing files that changed from the base of the PR and between b762731 and d505f9d.

📒 Files selected for processing (1)
  • packages/app-mobile/components/ExtendedWebView/index.jest.tsx
✅ Files skipped from review due to trivial changes (1)
  • packages/app-mobile/components/ExtendedWebView/index.jest.tsx

📝 Walkthrough

Walkthrough

Adds a Jest JSDOM polyfill to define missing scroll methods on element prototypes (applied to window and iframe contentWindows) and clears the Note component's focus-update interval on unmount to prevent post-unmount callbacks.

Changes

Cohort / File(s) Summary
ExtendedWebView Jest polyfill
packages/app-mobile/components/ExtendedWebView/index.jest.tsx
Adds window.__joplinJestPolyfillScroll(win) which ensures scrollIntoView, scrollTo, and scrollBy exist on Element, HTMLElement, and SVGElement prototypes; applied to main window and to each iframe's contentWindow before message/postMessage shims.
Note component unmount cleanup
packages/app-mobile/components/screens/Note/Note.tsx
In componentWillUnmount(), clears focusUpdateIID_ via shim.clearInterval and nulls the reference to stop focus-update callbacks after unmount.

Sequence Diagram(s)

sequenceDiagram
participant JestInit
participant Window
participant Iframe
participant ElementProto

JestInit->>Window: define __joplinJestPolyfillScroll(win)
JestInit->>Window: call polyfill on Window
Window->>ElementProto: ensure scrollIntoView/scrollTo/scrollBy (no-op if missing)
JestInit->>Iframe: create/patch iframe contentWindow
Iframe->>ElementProto: apply same polyfill to contentWindow
JestInit->>Iframe: attach message / parent.postMessage shims
Loading

Possibly related PRs

Suggested reviewers

  • laurent22
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarises the main change: adding a JSDOM scroll polyfill to ExtendedWebView to fix issue #14834.
Description check ✅ Passed The description clearly explains the problem, root cause, solution, and testing approach, all relevant to the changeset.
Linked Issues check ✅ Passed The pull request fully addresses the objectives in #14834: adds scoped scroll polyfills to the nested JSDOM window, avoids global patches, and clears the focus interval on unmount.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the reported issue: scroll polyfill in ExtendedWebView mock and interval cleanup in Note componentWillUnmount.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Pr Description Must Follow Guidelines ✅ Passed The PR description includes all three required sections: problem statement with user-impact, high-level solution explanation with concrete changes, and specific test plan verification steps.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai bot added bug It's a bug mobile All mobile platforms labels Mar 25, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/app-mobile/components/ExtendedWebView/index.jest.tsx`:
- Around line 59-67: The polyfill unconditionally overwrites
Element/HTMLElement/SVGElement.scrollIntoView inside
window.__joplinJestPolyfillScroll; change it to only assign scrollNoop when the
method is missing to avoid clobbering existing implementations or test
spies—inside the function that iterates Ctor (used in
window.__joplinJestPolyfillScroll) check if Ctor.prototype.scrollIntoView is
falsy before setting it, keeping the existing conditional assignments for
scrollTo and scrollBy intact.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 262c2a52-7530-4bc1-bbe7-a432ef082445

📥 Commits

Reviewing files that changed from the base of the PR and between dfdc0f3 and b762731.

📒 Files selected for processing (2)
  • packages/app-mobile/components/ExtendedWebView/index.jest.tsx
  • packages/app-mobile/components/screens/Note/Note.tsx

In the ExtendedWebView Jest JSDOM polyfill, only define scrollIntoView/scrollTo/scrollBy when missing, so existing test spies are preserved.
@laurent22
Copy link
Copy Markdown
Owner

laurent22 commented Mar 27, 2026

When you copy and paste from your AI bot, make sure you include the formatting, or fix it afterwards. Pasting things like this makes it hard to read and it looks unprofessional

@divyanshkhurana06
Copy link
Copy Markdown
Contributor Author

When you copy and paste from your AI bot, make sure you include the formatting, or fix it afterwards. Pasting things like this makes it hard to read and it looks unprofessional

hello laurent, sorry didn't mean to do this on purpose, just was in a hurry so might have mixed things up.

I will keep this in mind from the next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug It's a bug mobile All mobile platforms

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: Warning logged by Note.test.tsx tests

2 participants