Added playwright test for memories page#29737
Open
Rohit0301 wants to merge 2 commits into
Open
Conversation
Rohit0301
added
the
safe to test
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| <Typography ellipsis size="text-lg" weight="semibold"> | ||
| {modalTitle} | ||
| </Typography> | ||
| <HookForm form={form} onSubmit={form.handleSubmit(handleSubmit)}> |
There was a problem hiding this comment.
💡 Bug: Submit handler wired both to onSubmit and onClick
The RHF submit handler is attached in two places: the HookForm's onSubmit={form.handleSubmit(handleSubmit)} (line 615) and the submit Button's onClick={() => form.handleSubmit(handleSubmit)()} (lines 1132-1134). If the core-components Button renders a native type="submit" element, clicking it would both trigger the form's native submit AND the onClick, invoking handleSubmit (and thus createContextMemory/updateContextMemory) twice, creating duplicate memories. This appears safe only if Button defaults to type="button". Recommend driving submission from a single source — e.g. make the button type="submit" and rely on the form's onSubmit, or keep the onClick and remove the form-level onSubmit — to avoid an accidental double network call.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsIntegrates Playwright E2E tests for the memories page and migrates the memory creation modal to React Hook Form for better validation. Consolidate the form submission handler to remove redundant calls on both onSubmit and onClick. 💡 Bug: Submit handler wired both to onSubmit and onClick📄 openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/CreateMemoryModal/CreateMemoryModal.component.tsx:615 📄 openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/CreateMemoryModal/CreateMemoryModal.component.tsx:1132-1134 The RHF submit handler is attached in two places: the 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
357
to
366
| @@ -332,7 +365,7 @@ const CreateMemoryModal: FC<CreateMemoryModalProps> = ({ | |||
| onClose(); | |||
| }; | |||
Contributor
There was a problem hiding this comment.
memoryTab not reset on close
handleClose resets all other UI state (linkedAssets, selectedTags, showTagForm, modalError, isEditingVisibility, isViewOnly) but never calls setMemoryTab('edit'). The same omission exists in the useEffect that fires when memoryToEdit changes (line ~316). If a user switches to the Preview tab then clicks Cancel, the next time they open the modal they will be dropped into Preview mode and the textarea won't be visible.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+407
to
+454
| test('row shows linked entity badge when memory has a primary entity', async ({ | ||
| browser, | ||
| page, | ||
| }) => { | ||
| test.slow(); | ||
|
|
||
| const table = await fetchFirstTable(page); | ||
| if (!table?.id) { | ||
| test.skip(); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const { apiContext, afterAction } = await createNewPage(browser); | ||
| const linkedName = `cc_memory_linked_${uuid()}`; | ||
| const createRes = await apiContext.post(MEMORIES_API, { | ||
| data: { | ||
| name: linkedName, | ||
| title: `Linked Entity Memory ${uuid()}`, | ||
| question: 'Memory with linked entity', | ||
| answer: 'Linked to a data asset.', | ||
| shareConfig: { visibility: 'Shared' }, | ||
| primaryEntity: { | ||
| id: table.id, | ||
| type: table.entityType ?? 'table', | ||
| name: table.name, | ||
| displayName: table.displayName, | ||
| fullyQualifiedName: table.fullyQualifiedName, | ||
| }, | ||
| }, | ||
| }); | ||
| expect(createRes.status()).toBe(201); | ||
| const created = await createRes.json(); | ||
| const linkedMemoryId = created.id; | ||
| await afterAction(); | ||
|
|
||
| await navigateToMemories(page); | ||
|
|
||
| const row = page.getByTestId(`memory-row-${linkedMemoryId}`); | ||
| await row.scrollIntoViewIfNeeded(); | ||
| await expect(row).toBeVisible(); | ||
| await expect(row).toContainText(table.displayName ?? table.name); | ||
|
|
||
| const { apiContext: cleanCtx, afterAction: cleanAfter } = | ||
| await createNewPage(browser); | ||
| await cleanCtx.delete(`${MEMORIES_API}/${linkedMemoryId}?hardDelete=true`); | ||
| await cleanAfter(); | ||
| }); |
Contributor
There was a problem hiding this comment.
The memory created at line 422 (linkedMemoryId) is cleaned up at the bottom of the test body (lines 450–453). If any assertion between creation and cleanup fails, the memory is never deleted. The same pattern appears in the "changing visibility from Shared to Private shows 'visible only to you' after save" test (around line 1209) and the "linked asset card shows remove button" test (around line 1502). Wrapping these in a scoped afterEach or tracking the ID in globalMemoryIds would ensure cleanup runs even on failure.
Comment on lines
+892
to
+895
| // The copy-link button is the first button inside the row's actions area | ||
| // (before the edit and delete actions) | ||
| await row.getByRole('button').first().click(); | ||
|
|
Contributor
There was a problem hiding this comment.
row.getByRole('button').first() selects the copy-link button by position. The comment acknowledges the assumption, but if the row's button order ever changes (e.g., an icon is prepended, or the action buttons are reordered), this test will silently click the wrong button and produce a confusing failure. Prefer a stable data-testid selector like row.getByTestId('copy-link-btn') if one exists, or add one.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
❌ Playwright - ESLint + Prettier + Organise ImportsOne or more Playwright test files have linting or formatting issues. Affected files
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
Contributor
🔴 Playwright Results — 25 failure(s), 20 flaky✅ 4506 passed · ❌ 25 failed · 🟡 20 flaky · ⏭️ 41 skipped
Genuine Failures (failed on all attempts)❌
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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:
Fixes #
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
CreateMemoryModalstate management toreact-hook-formfor improved form logic and validation.CreateMemoryModalto useFieldPropfor consistent component rendering and field definitions.ContextCenterMemories.spec.tsPlaywright test suite to cover memory creation flows, form validation, and markdown preview functionality.This will update automatically on new commits.
Greptile Summary
This PR adds a Playwright E2E test suite for the Context Center Memories page and refactors
CreateMemoryModalto usereact-hook-formfor form state management.ContextCenterMemories.spec.ts): 1,700+ lines covering form validation, memory CRUD, filters/search, sort options, copy-link, pagination, and visibility enforcement — including a non-admin permission scenario.CreateMemoryModal.component.tsx): Replaces manualuseState-based form tracking withuseForm+FormField/getFieldprimitives from the UI library, and introducesFieldPropdefinitions for the title and type fields.memoryTabis not reset inhandleClose(leaves the modal in Preview mode on reopen), andtag.data.styleis accessed without optional chaining (can throw if a tag option omitsdata).Confidence Score: 3/5
The component refactor introduces two bugs that can surface in the existing UI; both are straightforward single-line fixes but should be resolved before merging.
The modal will silently stay on the Preview tab after a user closes it during preview mode — affecting every subsequent create-memory flow in that session. Separately, accessing
tag.data.stylewithout optional chaining will throw whenever a tag option omits thedataproperty, crashing the tag save handler. The test suite is well-structured but three tests can leave orphaned API data on failure, which can pollute subsequent runs.CreateMemoryModal.component.tsx — the handleClose function and handleTagSave need one-line fixes each before this is production-ready.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User opens modal] --> B{memoryToEdit?} B -- Yes --> C[useEffect: form.reset with existing data] B -- No --> D[Default form values] C --> E[Edit / View mode] D --> E E --> F{User action} F -- Switch tab --> G[memoryTab: edit / preview] F -- Submit --> H[handleSubmit] F -- Cancel --> I[handleClose] H -- Create --> J[createContextMemory API] H -- Edit --> K[updateContextMemory PATCH API] J --> L[onCreated callback] K --> M[onUpdated callback] I --> N[Reset: form, linkedAssets, tags, showTagForm, modalError, isEditingVisibility, isViewOnly] I -. missing .-> O[❌ memoryTab NOT reset] N --> P[onClose] L --> P M --> P%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[User opens modal] --> B{memoryToEdit?} B -- Yes --> C[useEffect: form.reset with existing data] B -- No --> D[Default form values] C --> E[Edit / View mode] D --> E E --> F{User action} F -- Switch tab --> G[memoryTab: edit / preview] F -- Submit --> H[handleSubmit] F -- Cancel --> I[handleClose] H -- Create --> J[createContextMemory API] H -- Edit --> K[updateContextMemory PATCH API] J --> L[onCreated callback] K --> M[onUpdated callback] I --> N[Reset: form, linkedAssets, tags, showTagForm, modalError, isEditingVisibility, isViewOnly] I -. missing .-> O[❌ memoryTab NOT reset] N --> P[onClose] L --> P M --> PComments Outside Diff (1)
openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/CreateMemoryModal/CreateMemoryModal.component.tsx, line 383-397 (link)tag.data.stylewill throw aTypeErrorat runtime whentag.dataisundefined. Thetypeof tag === 'string'guard above only protectstagFQN; the style access has no such protection. Adding optional chaining makes it safe regardless of whatTagSelectFormprovides.Reviews (1): Last reviewed commit: "Merge branch 'main' into memories-page-p..." | Re-trigger Greptile