Skip to content

Fixes #32641: scope FeedEditor DOM queries to each instance root - #32710

Merged
ShaileshParmar11 merged 5 commits into
mainfrom
detail/bug-fix/fixes-32641-scope-feededitor-dom-queries-to-each-i-aae2f7
Sep 11, 2026
Merged

ShaileshParmar11 merged 5 commits into
mainfrom
detail/bug-fix/fixes-32641-scope-feededitor-dom-queries-to-each-i-aae2f7

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Detail bug report: View on Detail

Describe your changes:

Fixes #32641

I worked on the Activity Feed rich-text editor (FeedEditor) because it hard-coded id="om-quill-editor" on its root <div> and resolved the emoji panel, the emoji toggle, and the RTL data-dir root through the global document. When two FeedEditor instances mount on the same page (e.g. the always-on Inbox composer plus an inline reply/task-comment edit, or the activity-card composer plus a main-post edit), the duplicate IDs make every document.getElementById/querySelector('#om-quill-editor …') resolve to the first instance, so a later instance's emoji toggle-to-close "bounced" (the panel closed then re-openened in the same gesture) and, in an RTL locale, every editor after the first had its blank-state placeholder left-aligned instead of right-aligned.

What I changed

  • Dropped the hardcoded id="om-quill-editor" from the root and added a per-instance rootRef. handleClickOutside and the RTL useEffect now query rootRef.current (this instance's own root) instead of the global document, so each editor only manages its own DOM.
  • Added a stable feed-editor-root class on the root (via classNames) and re-keyed the two LESS rules that depended on #om-quill-editor — the RTL placeholder right-align rule and the quill-emoji palette tab styling — to .feed-editor-root, preserving their styling and specificity.

Type of change:

  • Bug fix

High-level design:

N/A — small change.

Tests:

Use cases covered

  • Multiple FeedEditor instances mounted on the same page no longer share a root id; each scoped its own DOM.
  • The second-in-DOM editor's emoji toggle-to-close dismisses its panel cleanly (no bounce).
  • In an RTL locale, every mounted editor's blank-state placeholder is right-aligned (each root gets data-dir="rtl" on itself).

Unit tests

  • I added unit tests for the changed logic.
  • Files added: openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/FeedEditor/FeedEditorDuplicateId.test.tsx — a jsdom regression suite (FeedEditor is un-mocked; react-quill-new is mocked with a forwardRef component whose emoji toggle drives the shared #textarea-emoji panel the way the pinned @windmillcode/quill-emoji bundle does). Three cases, one per fix point: (1) no #om-quill-editor id is rendered and .feed-editor-root is present on every instance; (2) the second editor's toggle-to-close no longer bounces; (3) every editor root is marked data-dir="rtl" in an RTL locale. The suite was RED-verified: reverting only FeedEditor.tsx + feed-editor.less makes all three cases fail (duplicate id present, panel still open after the close gesture, RTL data-dir written to the wrong root), and re-applying the fix makes them pass.

Backend integration tests

  • Not applicable (no backend API changes).

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • I added Playwright E2E tests under openmetadata-ui/.../ui/playwright/ for UI changes.
  • Files added: openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/FeedEditorDuplicateId.spec.ts — seeds a thread + reply via the Conversations API, opens the reply's inline editor in the activity-feed thread drawer on /users/admin/activity_feed/all, and asserts (in a real Chromium) that the editor root has class feed-editor-root with no om-quill-editor id, and that the emoji toggle opens then closes the panel cleanly (the exact gesture the duplicate id broke). Verified green against a running OpenMetadata server built from this branch (mysql + Elasticsearch + Flyway migrations + yarn build dist baked into the server jar).

Manual testing performed

  • Ran the full jsdom suite (FeedEditor.test.tsx + FeedEditorDuplicateId.test.tsx): 10/10 pass; FeedEditor-consumer suites (ActivityFeedEditor, CommentCard, ActivityThreadPanel): 10/10 pass.
  • Ran the new Playwright spec end-to-end against a live server: 4/4 pass (including the setup/entity-data-setup/entity-data-teardown projects).
  • Verified the fix ships in the served bundle: grep feed-editor-root on the built assets/FeedEditor-*.js chunk inside the openmetadata-ui jar matches.
  • The two-editor bounce and RTL guarantees are proven in jsdom (with the buggy-code cross-check reproducing the exact bounce); a live two-editor co-mount of the always-on composer + inline reply edit wasn't exercised as a passing Playwright spec because the activity-feed thread drawer enforces a single inline-edit editor at a time, and the Inbox two-editor path needs a running ingestion/airflow + seeded notifications to reach. The jsdom suite covers that multi-instance path; the Playwright spec covers the single-editor live DOM contract and the previously-broken emoji gesture.

UI screen recording / screenshots:

TODO: attach a short screen recording of opening a reply's inline editor in the activity feed, toggling the emoji panel open and closed on the second editor without the bounce, plus an RTL-locale placeholder alignment check. (The change is UI-visible; the recording should be added before review.)

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed. (N/A — no schema changes.)
  • For UI changes: I attached a screen recording and/or screenshots above. (TODO — see the screen-recording section.)
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.
  • I have added a test that covers the exact scenario we are fixing. For complex issues, comment the issue number in the test for future reference.

Automatic Fixes PRs can be configured here.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 2ff2616bbcfb4fb9cb467cb2e4c91cf6c0fd9399 in Playwright run 34475590888, attempt 1.

✅ 885 passed · ❌ 0 failed · 🟡 2 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 52m 14s

⏱️ Max setup 4m 50s · max shard execution 18m 37s · max shard-job elapsed before upload 21m 54s · reporting 7s

🌐 197.45 requests/attempt · 2.70 app boots/UI scenario · 26.41% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 26.41% (convergence target: at most 15%).
  • Application boot ratio was 2.7 per UI scenario (2236 boots / 828 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 146 0 0 0 0 0
✅ Shard chromium-02 150 0 0 0 0 0
✅ Shard chromium-03 133 0 0 0 0 0
🟡 Shard chromium-04 156 0 2 0 0 0
✅ Shard chromium-05 142 0 0 0 0 0
✅ Shard data-asset-rules-01 65 0 0 0 0 0
✅ Shard domain-isolation-01 16 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard ingestion-01 1 0 0 0 0 0
✅ Shard reindex-01 2 0 0 0 0 0
✅ Shard search-01 11 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0
🟡 2 flaky test(s) (passed on retry)
  • Features/ContextCenterPermission.spec.tsuser with view-only permission cannot see create or upload actions (shard chromium-04, 1 retry)
  • Pages/TasksUIFlow.spec.tsCreate and resolve description task for Topic via UI (shard chromium-04, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@detail-app detail-app Bot added the safe to test Add this label to run secure Github workflows on PRs label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

⚠️ UI Checkstyle passed — lint findings in changed files

🔍 ESLint findings in this PR's files — 0 error(s), 2 warning(s)

Errors block the build. Warnings do not yet — they are rules whose backlog is still
being worked down, listed so this PR does not add to it. See docs/ui-code-quality-gate.md.

0 error(s), 2 warning(s) across 1 changed file(s).

Count Rule
1 openmetadata-imports/no-api-calls-in-iteration
1 react-hooks/exhaustive-deps
All findings
Location Rule Message
🟡 src/components/ActivityFeed/FeedEditor/FeedEditor.tsx:131:20 openmetadata-imports/no-api-calls-in-iteration Avoid issuing one API request per item. Fetch at the data owner, use a bulk endpoint, or use useQueries with an intentional concurrency policy.
🟡 src/components/ActivityFeed/FeedEditor/FeedEditor.tsx:246:7 react-hooks/exhaustive-deps React Hook useMemo has missing dependencies: 'renderItems', 't', and 'userSuggestionRenderer'. Either include them or remove the dependency array.

Fix locally (fast - only checks files changed in this branch):

make ui-checkstyle-changed

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 70%
70.66% (95843/135635) 55.26% (56840/102850) 56.6% (18944/33469)

chirag-madlani
chirag-madlani previously approved these changes Sep 9, 2026
Refactor the react-quill-new mock so getFormat/format arrow functions no
longer nest more than 4 levels deep — sonarjs/no-nested-functions was
failing UI Checkstyle on the PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

🔄 Playwright impact map auto-refreshed

This PR touched specs or UI source that changed the source→spec routing map. I regenerated .github/playwright/impact-map.generated.json and pushed the diff to this branch.

- source entries: 758 → 758
- 0 added, 0 removed, 22 changed spec-list

Entries whose spec list changed:
  openmetadata-ui/src/main/resources/ui/playwright/constant/config.ts
  openmetadata-ui/src/main/resources/ui/playwright/constant/service.ts
  openmetadata-ui/src/main/resources/ui/playwright/support/entity/service/DashboardServiceClass.ts
  openmetadata-ui/src/main/resources/ui/playwright/support/entity/service/DatabaseServiceClass.ts
  openmetadata-ui/src/main/resources/ui/playwright/support/entity/service/MessagingServiceClass.ts
  openmetadata-ui/src/main/resources/ui/playwright/support/fixtures/base.ts
  openmetadata-ui/src/main/resources/ui/playwright/utils/admin.ts
  openmetadata-ui/src/main/resources/ui/playwright/utils/common.ts
  openmetadata-ui/src/main/resources/ui/playwright/utils/entity.ts
  openmetadata-ui/src/main/resources/ui/playwright/utils/service.ts
  … and 12 more

What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing getByTestId strings. Hand-authored routing in impact-map.json always wins on conflict.

What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit:

python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit  # or a separate commit

@sonarqubecloud

Copy link
Copy Markdown

@chirag-madlani
chirag-madlani added this pull request to the merge queue Sep 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — failed_checks (2026-09-10T19:33:55Z)

Blocked the queue: playwright-summary

@ShaileshParmar11
ShaileshParmar11 added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit 1b4cbb6 Sep 11, 2026
110 checks passed
@ShaileshParmar11
ShaileshParmar11 deleted the detail/bug-fix/fixes-32641-scope-feededitor-dom-queries-to-each-i-aae2f7 branch September 11, 2026 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Detail Bug] Activity Feed: Multiple rich-text editors interfere due to duplicate DOM id (emoji toggle-to-close bounce, RTL placeholder misaligned)

4 participants