fix[qwik]: ENG-7299 Default value not updating for custom components on ContentEditor #3947
Conversation
🦋 Changeset detectedLatest commit: 7bb3f45 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 2708648 | Triggered | Generic High Entropy Secret | 3c65c71 | examples/angular-gen2/src/environments/environment.ts | View secret |
| 314150 | Triggered | Generic High Entropy Secret | 126bcfc | packages/sdks/src/functions/get-content/snapshots/generate-content-url.test.ts.snap | View secret |
| 11707119 | Triggered | Generic High Entropy Secret | 126bcfc | packages/sdks/snippets/react-sdk-next-14-app/app/custom-child/page.tsx | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
View your CI Pipeline Execution ↗ for commit 7bb3f45.
☁️ Nx Cloud last updated this comment at |
packages/sdks/src/components/block/components/interactive-element.lite.tsx
Show resolved
Hide resolved
…rTimeout for locator assertion as recommended
packages/sdks/mitosis.config.cjs
Outdated
| json: { | ||
| post: (json) => { | ||
| if (json.name === 'InteractiveElement') { | ||
| console.log('json', JSON.stringify(json, null, 2)); |
There was a problem hiding this comment.
| console.log('json', JSON.stringify(json, null, 2)); |
| await expect( | ||
| page.frameLocator('iframe').getByText('Item 1') | ||
| ).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| // Re-query the item1 element as it might have been recreated | ||
| const updatedItem1 = page.frameLocator('iframe').getByText('Item 1'); | ||
| await updatedItem1.click(); |
There was a problem hiding this comment.
we want to avoid hardcoded timeouts unless there's a special reason for them. Playwright has automated waiting and retrying so that we don't need to provide hardcoded timeout values all the time. Also, in this case, the default is already 5000 so this does nothing:
you also don't need to redefine the locator twice:
| await expect( | |
| page.frameLocator('iframe').getByText('Item 1') | |
| ).toBeVisible({ timeout: 5000 }); | |
| // Re-query the item1 element as it might have been recreated | |
| const updatedItem1 = page.frameLocator('iframe').getByText('Item 1'); | |
| await updatedItem1.click(); | |
| // Re-query the item1 element as it might have been recreated | |
| const updatedItem1 = page.frameLocator('iframe').getByText('Item 1'); | |
| await expect(updatedItem1).toBeVisible(); | |
| await updatedItem1.click(); |
|
Hey @samijaber I have went thru lot of changes the test for |
| @@ -9,7 +9,7 @@ const LAST_COMPONENT_REGISTERED_MESSAGE = | |||
|
|
|||
| test.describe('Custom components', () => { | |||
| test('correctly renders custom component', async ({ page, packageName, sdk }) => { | |||
| test.skip(!['angular', 'react'].includes(sdk)); | |||
| test.skip(!['angular', 'react', 'qwik-city'].includes(sdk)); | |||
There was a problem hiding this comment.
this line doesn't have any impact, sdk can be qwik. packageNames are the different e2e/snippet server names and can be qwik-city
There was a problem hiding this comment.
Ahh nice catch....I was testing somethings...I forgot to remove this
|
|
||
| // Re-query the item1 element as it might have been recreated | ||
| const updatedItem1 = page.frameLocator('iframe').getByText('Item 1'); | ||
| await expect(updatedItem1).toBeVisible(); | ||
| await expect(updatedItem1).toBeEnabled(); | ||
| await expect(updatedItem1).toBeInViewport(); | ||
| await updatedItem1.click(); | ||
|
|
||
| const detailElement = page.frameLocator('iframe').getByText(NEW_DETAILS_TEXT); | ||
| await detailElement.waitFor(); | ||
| await expect(detailElement).toBeVisible(); | ||
|
|
||
| const [titleBox, detailBox] = await Promise.all([ | ||
| item1.boundingBox(), | ||
| updatedItem1.boundingBox(), |
There was a problem hiding this comment.
no need to update this right?
| @@ -46,6 +48,25 @@ const builderBlockWithClassNameCustomComponent = { | |||
| ], | |||
| }; | |||
|
|
|||
| const CUSTOM_COMPONENT = [ | |||
There was a problem hiding this comment.
| const CUSTOM_COMPONENT = [ | |
| const CUSTOM_COMPONENTS = [ |
| name: 'Hello', | ||
| component: Hello, | ||
| inputs: [], | ||
| }, |
There was a problem hiding this comment.
I think you added the Hello component for this test:https://github.com/sidmohanty11/builder/blob/general-support-vc/packages/sdks-tests/src/e2e-tests/custom-components.spec.ts#L11 right? you will also need to update the content of it from just hello to hello World
| test('correctly updating custom components when default value is not set', async ({ page, basePort, sdk, packageName }) => { | ||
| test.skip(!['react', 'qwik-city'].includes(packageName)); | ||
| await launchEmbedderAndWaitForSdk({ path: '/custom-components-no-default-value', basePort, page, sdk }); | ||
| const newContent = cloneContent(CUSTOM_COMPONENT_NO_DEFAULT_VALUE); | ||
| newContent.data.blocks[0].component.options.text = "Hello"; | ||
| await sendContentUpdateMessage({ page, newContent, model: 'page' }); | ||
| const helloWorldText = page.frameLocator('iframe').locator('[builder-id="builder-d01784d1ca964e0da958ab4a4a891b08"]') | ||
| await expect(helloWorldText).toHaveText('Hello'); | ||
| }) |
There was a problem hiding this comment.
by default it should render Hello right? because you're passing it as defaultValue? without needing to send the content update message?
There was a problem hiding this comment.
Didn't get you...🤔
I am answering this questions based on my understanding. Yes, by default it should render hello. defaultValue has not been passed yet if you see CUSTOM_COMPONENT_NO_DEFAULT_VALUE the object will look like this
component: {
name: 'Description',
options: {},
}we will be requiring sendContentUpdateMessage to update the text message from "" to "Hello"
There was a problem hiding this comment.
shouldn't this: https://github.com/BuilderIO/builder/pull/3947/files#diff-f17fc2e9e6719ba501c3ab4df9c1fef742dc2c5dd3f709c622f6c79cdffcc1bdR99 add the defaultValue? i'm confused because on initial render if there is no initial value provided and we are providing a default value - we should be fallbacking to that
There was a problem hiding this comment.
@sidmohanty11 your confusion stems from the fact that default values are only used when the block is created in the visual editor to determine the initial value for inputs: they are not used by the SDK at run-time.
@yash-builder one way to clear up such confusion is to avoid using the exact same values for different things. Have a defaultValue: 'FOO' and update the text here to Hello instead of having the exact same string Hello in both cases.
Also, as a general rule, when we add a test that involves editing content, i think it's best that it looks like this:
- load initial content
- check the initial value of a block
- make an update to that block
- check the value of the same block, and see that it changed as expected
In this test you're not checking the initial state of the block. You could add a toBeEmpty() check:
test.skip(!['react', 'qwik-city'].includes(packageName));
await launchEmbedderAndWaitForSdk({ path: '/custom-components-no-default-value', basePort, page, sdk });
// check initial value here
const helloWorldText = page.frameLocator('iframe').locator('[builder-id="builder-d01784d1ca964e0da958ab4a4a891b08"]')
await expect(helloWorldText).toBeEmpty();
const newContent = cloneContent(CUSTOM_COMPONENT_NO_DEFAULT_VALUE);
newContent.data.blocks[0].component.options.text = "Hello";
await sendContentUpdateMessage({ page, newContent, model: 'page' });
const helloWorldText = page.frameLocator('iframe').locator('[builder-id="builder-d01784d1ca964e0da958ab4a4a891b08"]')
await expect(helloWorldText).toHaveText('Hello');Not a huge deal though, more of a general rule of thumb for these types of tests
| test('correctly updating custom components when default value is not set', async ({ page, basePort, sdk, packageName }) => { | ||
| test.skip(!['react', 'qwik-city'].includes(packageName)); | ||
| await launchEmbedderAndWaitForSdk({ path: '/custom-components-no-default-value', basePort, page, sdk }); | ||
| const newContent = cloneContent(CUSTOM_COMPONENT_NO_DEFAULT_VALUE); | ||
| newContent.data.blocks[0].component.options.text = "Hello"; | ||
| await sendContentUpdateMessage({ page, newContent, model: 'page' }); | ||
| const helloWorldText = page.frameLocator('iframe').locator('[builder-id="builder-d01784d1ca964e0da958ab4a4a891b08"]') | ||
| await expect(helloWorldText).toHaveText('Hello'); | ||
| }) |
There was a problem hiding this comment.
@sidmohanty11 your confusion stems from the fact that default values are only used when the block is created in the visual editor to determine the initial value for inputs: they are not used by the SDK at run-time.
@yash-builder one way to clear up such confusion is to avoid using the exact same values for different things. Have a defaultValue: 'FOO' and update the text here to Hello instead of having the exact same string Hello in both cases.
Also, as a general rule, when we add a test that involves editing content, i think it's best that it looks like this:
- load initial content
- check the initial value of a block
- make an update to that block
- check the value of the same block, and see that it changed as expected
In this test you're not checking the initial state of the block. You could add a toBeEmpty() check:
test.skip(!['react', 'qwik-city'].includes(packageName));
await launchEmbedderAndWaitForSdk({ path: '/custom-components-no-default-value', basePort, page, sdk });
// check initial value here
const helloWorldText = page.frameLocator('iframe').locator('[builder-id="builder-d01784d1ca964e0da958ab4a4a891b08"]')
await expect(helloWorldText).toBeEmpty();
const newContent = cloneContent(CUSTOM_COMPONENT_NO_DEFAULT_VALUE);
newContent.data.blocks[0].component.options.text = "Hello";
await sendContentUpdateMessage({ page, newContent, model: 'page' });
const helloWorldText = page.frameLocator('iframe').locator('[builder-id="builder-d01784d1ca964e0da958ab4a4a891b08"]')
await expect(helloWorldText).toHaveText('Hello');Not a huge deal though, more of a general rule of thumb for these types of tests
Description
This PR addresses a critical reactivity issue in the Qwik SDK where deeply nested content updates weren't properly triggering.
The core problem was that when content was updated through the Builder.io editor, the changes were correctly stored in state but weren't causing components to re-render.
I've implemented a key-based approach for the
InteractiveElementcomponent that uses a dynamic key derived from the component options, forcingQwikto update it's component whenoptionschanges. This effectively bypasses limitations in Qwik's resumability model where prop changes to dynamic components aren't always detected. While usingJSON.stringifyin keys isn't ideal for performance, but it provides a reliable solution that ensures content updates are immediately reflected in the UI without requiring page refreshes.JIRA
https://builder-io.atlassian.net/browse/ENG-7299
Loom
https://www.loom.com/share/2b4d1b2872fe4151b1a9f2f131bab780?sid=7fc0f143-0623-41f4-aa94-e947530dde55