Skip to content

test(renderer): add html block tests#1067

Merged
johbaxter merged 1 commit intodevfrom
852-html-block
May 9, 2025
Merged

test(renderer): add html block tests#1067
johbaxter merged 1 commit intodevfrom
852-html-block

Conversation

@stelbailey
Copy link
Copy Markdown
Contributor

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes

@stelbailey stelbailey requested a review from a team as a code owner May 8, 2025 15:48
@stelbailey stelbailey linked an issue May 8, 2025 that may be closed by this pull request
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 8, 2025

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

test(renderer): add html block tests


User description

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes


PR Type

Tests


Description

  • Test HTMLBlock sanitized content rendering

  • Test empty HTML returns no block

  • Test unsafe HTML attributes are stripped


Changes walkthrough 📝

Relevant files
Tests
HTMLBlock.spec.tsx
Add HTMLBlock spec tests                                                                 

libs/renderer/src/testing/block-defaults/HTMLBlock.spec.tsx

  • Introduces HTMLBlock rendering tests
  • Verifies sanitized HTML output
  • Checks no render on empty content
  • Ensures XSS attributes are removed
  • +76/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented May 8, 2025

    @CodiumAI-Agent /review

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Sanitization Coverage

    Tests only verify removal of onerror attributes but don’t cover other XSS vectors like <script> tags, javascript: URIs, or inline event handlers. Add tests for broader sanitization edge cases.

        it("sanitizes potentially unsafe HTML", async () => {
            const unsafeBlocks = {
                html: {
                    data: {
                        html: `<img src="x" onerror="alert('XSS')" />`,
                    },
                    id: "html",
                    widget: "html",
                    slots: {},
                    listeners: {},
                },
            };
    
            const { container } = render(<HTMLBlock id="html" />, {
                blocks: unsafeBlocks,
            });
    
            const block = container.querySelector("[data-block='html']");
            expect(block).not.toBeNull();
    
            const shadowRoot = block.shadowRoot;
            expect(shadowRoot).not.toBeNull();
            expect(shadowRoot.innerHTML).toContain(`<img src="x">`);
            expect(shadowRoot.innerHTML).not.toContain(`onerror="alert('XSS')"`);
        });
    });
    Shadow DOM Assumption

    Tests directly access shadowRoot; ensure the component always attaches to shadow DOM or provide fallback behavior in environments where shadow DOM may not be available.

    const block = container.querySelector("[data-block='html']");
    expect(block).not.toBeNull();
    
    const shadowRoot = block.shadowRoot;
    expect(shadowRoot).not.toBeNull();
    expect(shadowRoot.innerHTML).toContain("<p>Hello, World!</p>");

    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented May 8, 2025

    @CodiumAI-Agent /improve

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use correct selector for empty block

    In the "renders nothing when HTML content is empty" test, you should query for the
    emptyHtml block ID instead of "html", since you're rendering <HTMLBlock
    id="emptyHtml" />. This will correctly assert that the empty block is not rendered.

    libs/renderer/src/testing/block-defaults/HTMLBlock.spec.tsx [47]

    -const block = container.querySelector("[data-block='html']");
    +const block = container.querySelector("[data-block='emptyHtml']");
    Suggestion importance[1-10]: 8

    __

    Why: The test currently queries the wrong selector and should use emptyHtml to accurately verify that the empty block isn’t rendered.

    Medium
    General
    Assert attribute removal directly

    Rather than checking the raw HTML string, use a DOM assertion to verify that the
    onerror attribute has been stripped from the element. This makes the test more
    robust against serialization differences.

    libs/renderer/src/testing/block-defaults/HTMLBlock.spec.tsx [74]

    -expect(shadowRoot.innerHTML).not.toContain(`onerror="alert('XSS')"`);
    +expect(shadowRoot.querySelector('img')).not.toHaveAttribute('onerror');
    Suggestion importance[1-10]: 6

    __

    Why: Checking for the absence of the onerror attribute via a DOM matcher is more robust than string matching the HTML.

    Low
    Use toContainHTML matcher

    Use the toContainHTML matcher provided by jest-dom for a clearer assertion of HTML
    content, improving readability and intent.

    libs/renderer/src/testing/block-defaults/HTMLBlock.spec.tsx [39]

    -expect(shadowRoot.innerHTML).toContain("<p>Hello, World!</p>");
    +expect(shadowRoot).toContainHTML('<p>Hello, World!</p>');
    Suggestion importance[1-10]: 5

    __

    Why: The toContainHTML matcher expresses intent more clearly than matching raw innerHTML strings.

    Low
    Remove unnecessary async

    Since none of the tests use await, you can remove the async keyword to avoid
    confusion about asynchronous behavior.

    libs/renderer/src/testing/block-defaults/HTMLBlock.spec.tsx [29]

    -it("renders sanitized HTML content", async () => {
    +it("renders sanitized HTML content", () => {
    Suggestion importance[1-10]: 3

    __

    Why: Declaring the test async without using await is unnecessary and may confuse readers about asynchronous behavior.

    Low

    @johbaxter johbaxter merged commit c0953c0 into dev May 9, 2025
    4 checks passed
    @johbaxter johbaxter deleted the 852-html-block branch May 9, 2025 19:49
    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented May 9, 2025

    @CodiumAI-Agent /update_changelog

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

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    HTML Block

    4 participants