You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
For UI changes: I attached a screen recording and/or screenshots above.
I have added tests (unit / integration / Playwright as applicable) and listed them above.
Summary by Gitar
Test reliability:
Added input-data-testid to FileUploadDropZone and UploadDocumentModal to enable stable selection of file inputs.
Updated ContextCenter.spec.ts to use file-upload-input instead of non-deterministic selectors.
Diagnostic instrumentation:
Added granular debug logs and browser-to-node event bridge in ContextCenter.spec.ts to capture file input state and change event data for troubleshooting test failures.
Test data management:
Updated file upload tests to dynamically generate and clean up a physical temporary file during execution.
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.
Updates file upload components to improve Playwright test reliability, but leaves significant debugging instrumentation and introduces improper production styling for file inputs.
⚠️Bug: Shared file-upload input changed from hidden to sr-only for a test fix
To make the Playwright test pass, the production shared core component swaps the hidden <input type="file"> from the hidden attribute to className="tw:peer tw:sr-only" (file-upload.tsx:243-245). This is a behavioral change to a component used across the whole app: hidden removes the element from layout and accessibility tree entirely, whereas sr-only keeps it in the layout (visually clipped, still focusable/announced to screen readers and tab order). For a setInputFiles call Playwright only needs the input attached, so the hidden attribute should not have blocked the original test — meaning the real test failure cause may be elsewhere, and this component change may be unnecessary. If the change is intended, verify it does not introduce a stray focus stop / duplicated screen-reader announcement (the visible label/button already triggers the input via inputRef.current?.click()), and attach UI verification. Consider keeping hidden (or tw:hidden) unless sr-only is specifically required.
✅ 2 resolved✅ Quality: Debug instrumentation left in committed Playwright test
📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ContextCenter.spec.ts:1040-1054
The most recent commit ("added debugging lines to check the aut issue") leaves a large amount of throwaway debugging code in ContextCenter.spec.ts: multiple console.log('[upload-test] ...') statements, an attribute-snapshot evaluate, and a page.exposeFunction('__uploadTestChangeEvent', ...) browser→node bridge with a manual change listener (lines ~1040-1101). None of this is needed to assert the upload behavior — the only functional assertions are setInputFiles + the toBeVisible() checks. This noise pollutes CI logs, slows the test, and adds maintenance burden. Remove the console logging, the inputAttrs snapshot, the exposeFunction bridge, and the extra change-event evaluate before merging, keeping only setInputFiles(uploadFilePath) and the visibility assertions.
✅ Bug: exposeFunction re-registration guard is ineffective
📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ContextCenter.spec.ts:1062-1075
The guard if (!(page as unknown as Record<string, unknown>)[bridgeFn]) checks for a property named __uploadTestChangeEvent on the Playwright Page object, but page.exposeFunction registers the function on the browser window, not on the page object. The property checked is never set, so the guard is always truthy-negative and provides no protection: on a test retry that reuses the same page context, exposeFunction will throw "Function already registered". Since this whole bridge should be removed (see the debug-instrumentation finding), the cleanest fix is deletion; otherwise the guard would need to query the browser context (e.g. via a window flag set inside exposeFunction).
🤖 Prompt for agents
Code Review: Updates file upload components to improve Playwright test reliability, but leaves significant debugging instrumentation and introduces improper production styling for file inputs.
1. ⚠️ Bug: Shared file-upload input changed from hidden to sr-only for a test fix
Files: openmetadata-ui-core-components/src/main/resources/ui/src/components/application/file-upload/file-upload.tsx:243-253
To make the Playwright test pass, the production shared core component swaps the hidden `<input type="file">` from the `hidden` attribute to `className="tw:peer tw:sr-only"` (file-upload.tsx:243-245). This is a behavioral change to a component used across the whole app: `hidden` removes the element from layout and accessibility tree entirely, whereas `sr-only` keeps it in the layout (visually clipped, still focusable/announced to screen readers and tab order). For a `setInputFiles` call Playwright only needs the input `attached`, so the `hidden` attribute should not have blocked the original test — meaning the real test failure cause may be elsewhere, and this component change may be unnecessary. If the change is intended, verify it does not introduce a stray focus stop / duplicated screen-reader announcement (the visible label/button already triggers the input via `inputRef.current?.click()`), and attach UI verification. Consider keeping `hidden` (or `tw:hidden`) unless `sr-only` is specifically required.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
safe to testAdd this label to run secure Github workflows on PRs
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes:
Added some debug lines in the context center spec, to find the root cause of test failure on AUT
Fixes 28736
I worked on ... because ...
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Summary by Gitar
input-data-testidtoFileUploadDropZoneandUploadDocumentModalto enable stable selection of file inputs.ContextCenter.spec.tsto usefile-upload-inputinstead of non-deterministic selectors.ContextCenter.spec.tsto capture file input state and change event data for troubleshooting test failures.This will update automatically on new commits.