Skip to content

[WC-3504][WC-3264] update dependencies and add subscript and superscript - #2406

Open
gjulivan wants to merge 1 commit into
mainfrom
markdown/update
Open

[WC-3504][WC-3264] update dependencies and add subscript and superscript#2406
gjulivan wants to merge 1 commit into
mainfrom
markdown/update

Conversation

@gjulivan

Copy link
Copy Markdown
Collaborator

Pull request type


Description

@gjulivan
gjulivan requested a review from a team as a code owner August 31, 2026 22:49
@github-actions

Copy link
Copy Markdown
Contributor

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed


What was reviewed

File Change
packages/pluggableWidgets/markdown-web/CHANGELOG.md Unreleased entries for dep update + subscript/superscript support
packages/pluggableWidgets/markdown-web/package.json markdown-it bumped 14→15, added markdown-it-sub and markdown-it-sup
packages/pluggableWidgets/markdown-web/src/Markdown.tsx .use(sub).use(sup) wired to the module-level parser
packages/pluggableWidgets/markdown-web/typings/markdown-it-sub.d.ts New ambient type declaration for markdown-it-sub
packages/pluggableWidgets/markdown-web/typings/markdown-it-sup.d.ts New ambient type declaration for markdown-it-sup

Skipped (out of scope): pnpm-lock.yaml


Findings

🔶 Medium — No tests for the new subscript/superscript functionality

File: packages/pluggableWidgets/markdown-web/src/__tests__/Markdown.spec.tsx
Problem: The PR adds two new rendering plugins but the test file has no cases verifying that subscript (~text~) or superscript (^text^) syntax actually produces <sub> or <sup> elements. Per the checklist, new features require new tests. The snapshot tests also need to be regenerated to capture the changed output of the updated markdown-it v15.
Fix:

it("renders subscript syntax", () => {
    defaultProps.stringAttribute = new EditableValueBuilder<string>()
        .withValue("H~2~O")
        .build();
    const { container } = render(<Markdown {...defaultProps} />);
    expect(container.querySelector("sub")).toBeInTheDocument();
    expect(container.querySelector("sub")?.textContent).toBe("2");
});

it("renders superscript syntax", () => {
    defaultProps.stringAttribute = new EditableValueBuilder<string>()
        .withValue("x^2^")
        .build();
    const { container } = render(<Markdown {...defaultProps} />);
    expect(container.querySelector("sup")).toBeInTheDocument();
    expect(container.querySelector("sup")?.textContent).toBe("2");
});

Also run pnpm run test -u to update the existing snapshots after the markdown-it v15 upgrade.


⚠️ Low — @types/markdown-it version does not track the runtime major

File: packages/pluggableWidgets/markdown-web/package.json line 65
Note: @types/markdown-it@14.2.0 is a DefinitelyTyped package authored for markdown-it v14. The runtime was bumped to v15. If markdown-it v15 ships its own bundled types (or the DefinitelyTyped entry was updated for v15), this resolves naturally — but it's worth verifying with pnpm why @types/markdown-it that no dual-version situation exists. If v15 bundles types, the @types/markdown-it dev dependency can be dropped entirely.


⚠️ Low — PR description is empty

Note: None of the PR template sections were uncommented (PR type, description, testing notes). Since this adds user-visible behavior (subscript/superscript syntax support) and bumps a major version of markdown-it, a brief description of what changed and how to test it would help reviewers and QA.


Positives

  • Ambient declaration files added for both plugins (typings/markdown-it-sub.d.ts, typings/markdown-it-sup.d.ts) rather than relying on // @ts-ignore — correct approach for untyped packages.
  • CHANGELOG entries are user-facing and well-worded; both the dependency security update and the new feature are clearly documented in the right Changed/Added sections.
  • Plugin wiring is minimal and idiomatic — .use(sub).use(sup) on the existing module-level singleton keeps the diff small and clean.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant