Conversation
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a new Vitest test suite for common utility functions in src/utils/common.test.ts to improve coverage and validate edge cases. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughA new Vitest test file was added to verify the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Test coverage is currently narrow (single happy-path case), which leaves common input shapes and edge cases unprotected. The test also doesn’t specify whether unescapeHTML should mutate its input or return a new value, which can lead to subtle regressions.
Summary of changes
Added test coverage for unescapeHTML
- Introduced a new Vitest suite in
src/utils/common.test.ts. - Added a single test asserting that
unescapeHTML()converts<,>, and&in a node’schildrenstring into their literal characters (e.g.,"<b>"→"<b>").
| it("unescapes HTML entities in VNode children", () => { | ||
| const node = { | ||
| type: "div", | ||
| children: "<b>bold</b> & beautiful", | ||
| } | ||
| const result = unescapeHTML(node) | ||
| expect(result.children).toBe("<b>bold</b> & beautiful") | ||
| }) |
There was a problem hiding this comment.
This test only covers the happy path where children is a simple string containing a few entities. If unescapeHTML is meant to be a general utility, it’s easy for regressions to slip in without covering common shapes (e.g., missing/undefined children, array children, nested nodes, already-unescaped strings, or other entities). Adding a small table-driven set of cases would make this much harder to break accidentally.
Suggestion
Expand coverage with a small set of additional cases (table-driven) that reflect the function’s expected contract, e.g.:
childrenmissing /null/ non-stringchildrenarray (if supported) and nested nodes- idempotency (calling twice yields same result)
- a few more entities (e.g.,
",', numeric entities) if supported
If you want, reply with "@CharlieHelps yes please" and I’ll add a commit that introduces a describe.each/it.each matrix covering these cases.
| const node = { | ||
| type: "div", | ||
| children: "<b>bold</b> & beautiful", | ||
| } | ||
| const result = unescapeHTML(node) | ||
| expect(result.children).toBe("<b>bold</b> & beautiful") |
There was a problem hiding this comment.
The test doesn’t clarify whether unescapeHTML is expected to mutate the input node or return a new node. That behavior matters to callers and is easy to change unintentionally. Consider asserting the intended behavior explicitly (either result === node for mutation or result !== node for immutability).
Suggestion
Add an explicit assertion for mutation vs immutability based on the intended contract, e.g.:
- If immutable:
expect(result).not.toBe(node) - If mutating:
expect(result).toBe(node)
Optionally, deep-freeze node to ensure immutability is enforced if that’s the goal. Reply with "@CharlieHelps yes please" if you’d like me to add a commit with the assertion that matches the intended behavior.
There was a problem hiding this comment.
Pull request overview
Adds a Vitest unit test to improve coverage for a shared utility in src/utils, focusing on HTML entity unescaping behavior.
Changes:
- Added
src/utils/common.test.tswith a unit test forunescapeHTMLhandling common HTML entities inchildren.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { describe, expect, it } from "vitest" | ||
| import { unescapeHTML } from "./common" | ||
|
|
||
| describe("common utilities", () => { | ||
| it("unescapes HTML entities in VNode children", () => { | ||
| const node = { | ||
| type: "div", | ||
| children: "<b>bold</b> & beautiful", | ||
| } | ||
| const result = unescapeHTML(node) | ||
| expect(result.children).toBe("<b>bold</b> & beautiful") |
There was a problem hiding this comment.
This test file uses double quotes for imports/strings, but the repo’s formatter config prefers single quotes (see .prettierrc singleQuote:true) and existing utils tests follow that convention. Please reformat (or run pnpm format) so this file matches the enforced style and doesn’t fail format:check.
| import { describe, expect, it } from "vitest" | |
| import { unescapeHTML } from "./common" | |
| describe("common utilities", () => { | |
| it("unescapes HTML entities in VNode children", () => { | |
| const node = { | |
| type: "div", | |
| children: "<b>bold</b> & beautiful", | |
| } | |
| const result = unescapeHTML(node) | |
| expect(result.children).toBe("<b>bold</b> & beautiful") | |
| import { describe, expect, it } from 'vitest' | |
| import { unescapeHTML } from './common' | |
| describe('common utilities', () => { | |
| it('unescapes HTML entities in VNode children', () => { | |
| const node = { | |
| type: 'div', | |
| children: '<b>bold</b> & beautiful', | |
| } | |
| const result = unescapeHTML(node) | |
| expect(result.children).toBe('<b>bold</b> & beautiful') |
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/daggers-projects-26434d0b?upgradeToPro=build-rate-limit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/common.test.ts`:
- Around line 4-13: Add expanded tests for unescapeHTML in
src/utils/common.test.ts to cover the missing edge cases: assert additional
entity types (", ', /, =) are decoded by unescapeHTML;
verify array children are processed recursively (arrays with strings and
VNode-like objects passed to unescapeHTML); verify nested VNode children
(objects with children that are VNodes) are unescaped recursively; and add
edge-case tests for null/undefined node, empty children, and nodes with no
children property. Reference the unescapeHTML function in your new test cases
and ensure each scenario has an expectation that matches the implementation's
return shape (e.g., result.children is the unescaped string or array).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| describe("common utilities", () => { | ||
| it("unescapes HTML entities in VNode children", () => { | ||
| const node = { | ||
| type: "div", | ||
| children: "<b>bold</b> & beautiful", | ||
| } | ||
| const result = unescapeHTML(node) | ||
| expect(result.children).toBe("<b>bold</b> & beautiful") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Expand test coverage to include edge cases and additional entity types.
The PR description states this adds tests "to cover missing edge cases," but currently only one basic happy-path test exists. The unescapeHTML implementation handles several scenarios not covered here:
Missing test cases:
- Additional entity types:
",',/,=(implementation handles 7 entity types total, test only covers 3) - Array children: Recursive processing of arrays containing strings and VNodes
- Nested VNode children: Recursive unescaping of nested object children
- Edge cases: null/undefined node, empty children, no children property
📝 Proposed additional test cases to improve coverage
describe("common utilities", () => {
it("unescapes HTML entities in VNode children", () => {
const node = {
type: "div",
children: "<b>bold</b> & beautiful",
}
const result = unescapeHTML(node)
expect(result.children).toBe("<b>bold</b> & beautiful")
})
+
+ it("unescapes all supported HTML entities", () => {
+ const node = {
+ type: "div",
+ children: "<>"'/=&",
+ }
+ const result = unescapeHTML(node)
+ expect(result.children).toBe("<>\"'/=&")
+ })
+
+ it("unescapes HTML entities in array children", () => {
+ const node = {
+ type: "div",
+ children: [
+ "<text>",
+ { type: "span", children: "& more "text"" },
+ ],
+ }
+ const result = unescapeHTML(node)
+ expect(result.children).toEqual([
+ "<text>",
+ { type: "span", children: "& more \"text\"" },
+ ])
+ })
+
+ it("handles null or undefined nodes", () => {
+ expect(unescapeHTML(null as any)).toBe(null)
+ expect(unescapeHTML(undefined as any)).toBe(undefined)
+ })
+
+ it("handles nodes without children", () => {
+ const node = { type: "div" }
+ const result = unescapeHTML(node)
+ expect(result).toEqual({ type: "div" })
+ })
+
+ it("handles deeply nested VNode children", () => {
+ const node = {
+ type: "div",
+ children: {
+ type: "p",
+ children: {
+ type: "span",
+ children: "<nested> & deep",
+ },
+ },
+ }
+ const result = unescapeHTML(node)
+ expect(result.children).toEqual({
+ type: "p",
+ children: {
+ type: "span",
+ children: "<nested> & deep",
+ },
+ })
+ })Would you like me to open an issue to track these additional test cases?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("common utilities", () => { | |
| it("unescapes HTML entities in VNode children", () => { | |
| const node = { | |
| type: "div", | |
| children: "<b>bold</b> & beautiful", | |
| } | |
| const result = unescapeHTML(node) | |
| expect(result.children).toBe("<b>bold</b> & beautiful") | |
| }) | |
| }) | |
| describe("common utilities", () => { | |
| it("unescapes HTML entities in VNode children", () => { | |
| const node = { | |
| type: "div", | |
| children: "<b>bold</b> & beautiful", | |
| } | |
| const result = unescapeHTML(node) | |
| expect(result.children).toBe("<b>bold</b> & beautiful") | |
| }) | |
| it("unescapes all supported HTML entities", () => { | |
| const node = { | |
| type: "div", | |
| children: "<>"'/=&", | |
| } | |
| const result = unescapeHTML(node) | |
| expect(result.children).toBe("<>\"'/=&") | |
| }) | |
| it("unescapes HTML entities in array children", () => { | |
| const node = { | |
| type: "div", | |
| children: [ | |
| "<text>", | |
| { type: "span", children: "& more "text"" }, | |
| ], | |
| } | |
| const result = unescapeHTML(node) | |
| expect(result.children).toEqual([ | |
| "<text>", | |
| { type: "span", children: "& more \"text\"" }, | |
| ]) | |
| }) | |
| it("handles null or undefined nodes", () => { | |
| expect(unescapeHTML(null as any)).toBe(null) | |
| expect(unescapeHTML(undefined as any)).toBe(undefined) | |
| }) | |
| it("handles nodes without children", () => { | |
| const node = { type: "div" } | |
| const result = unescapeHTML(node) | |
| expect(result).toEqual({ type: "div" }) | |
| }) | |
| it("handles deeply nested VNode children", () => { | |
| const node = { | |
| type: "div", | |
| children: { | |
| type: "p", | |
| children: { | |
| type: "span", | |
| children: "<nested> & deep", | |
| }, | |
| }, | |
| } | |
| const result = unescapeHTML(node) | |
| expect(result.children).toEqual({ | |
| type: "p", | |
| children: { | |
| type: "span", | |
| children: "<nested> & deep", | |
| }, | |
| }) | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/common.test.ts` around lines 4 - 13, Add expanded tests for
unescapeHTML in src/utils/common.test.ts to cover the missing edge cases: assert
additional entity types (", ', /, =) are decoded by
unescapeHTML; verify array children are processed recursively (arrays with
strings and VNode-like objects passed to unescapeHTML); verify nested VNode
children (objects with children that are VNodes) are unescaped recursively; and
add edge-case tests for null/undefined node, empty children, and nodes with no
children property. Reference the unescapeHTML function in your new test cases
and ensure each scenario has an expectation that matches the implementation's
return shape (e.g., result.children is the unescaped string or array).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
Co-authored-by: daggerstuff <261005129+daggerstuff@users.noreply.github.com>
💡 What: Added tests for common utilities\n🎯 Why: Covers missing edge cases / improves coverage\n✅ Verification: Runs successfully in Vitest
PR created automatically by Jules for task 3634795303099416474 started by @daggerstuff
Summary by Sourcery
Tests:
Summary by cubic
Add a
vitesttest suite insrc/utils/common.test.tsforunescapeHTML. Verifies HTML entities in VNode children are unescaped (e.g., '<b>bold</b> & beautiful' -> 'bold & beautiful').Written for commit 30738df. Summary will update on new commits.
Summary by CodeRabbit