Conversation
|
WalkthroughThis pull request extends the Address component with 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
asanehisa
requested review from
benlife5,
briantstephan,
jwartofsky-yext and
mkilpatrick
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/pages-components/src/components/address/address.test.tsx (2)
103-117: Assertion logic may pass incorrectly if only one element exists.The expression
expect(countryEl && regionEl).toBeTruthy()will only be truthy if both elements exist. If only one is present (e.g.,countryElis truthy butregionElis null), the test would fail even though it's testing that custom lines bypass filtering. This is actually correct behavior for this test case, but separate assertions would make the intent clearer and provide better failure diagnostics.Suggested improvement for clarity
- expect(countryEl && regionEl).toBeTruthy(); + expect(countryEl).toBeTruthy(); + expect(regionEl).toBeTruthy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pages-components/src/components/address/address.test.tsx` around lines 103 - 117, The combined assertion using `expect(countryEl && regionEl).toBeTruthy()` can hide which element is missing; update the test "does not apply showCountry/showRegion when custom lines are provided" to assert each element separately by replacing the single boolean expression with two assertions `expect(countryEl).toBeTruthy()` and `expect(regionEl).toBeTruthy()` so failures clearly indicate whether `countryEl` or `regionEl` (queried in the test) is missing when rendering the `Address` component with `lines={[["region"], ["countryCode"]]}`.
94-101: Assertion logic may not verify both elements as intended.The expression
expect(countryEl && regionEl).toBeFalsy()will pass if either element is null/falsy, not necessarily both. IfcountryElisnull, the expression short-circuits and evaluates tonull(falsy) regardless ofregionEl.Consider using separate assertions for clarity:
Proposed fix
- expect(countryEl && regionEl).toBeFalsy(); + expect(countryEl).toBeFalsy(); + expect(regionEl).toBeFalsy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pages-components/src/components/address/address.test.tsx` around lines 94 - 101, The test "hides both country and region in default format when both are false" currently uses a combined assertion that can short-circuit; update the assertion in address.test.tsx to assert each element individually: check that the result of screen.queryByText("US") (countryEl) is null/falsy and separately that screen.queryByText("AL") (regionEl) is null/falsy so both absence conditions are verified for the Address component when showCountry and showRegion are false.packages/pages-components/CHANGELOG.md (1)
5-9: Missing changelog entry for the newshowCountryandshowRegionfeature.This PR adds new
showCountryandshowRegionprops to the Address component, but the changelog only contains formatting changes. A new entry should be added under "New Features" to document this addition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pages-components/CHANGELOG.md` around lines 5 - 9, Changelog is missing an entry for the new Address component props; add a "New Features" bullet documenting the addition of showCountry and showRegion props for the Address component (mention the prop names showCountry and showRegion and the Address component), following the existing changelog style (short description, PR link and commit hash format) and place it under the "New Features" section alongside the other entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/pages-components/CHANGELOG.md`:
- Around line 5-9: Changelog is missing an entry for the new Address component
props; add a "New Features" bullet documenting the addition of showCountry and
showRegion props for the Address component (mention the prop names showCountry
and showRegion and the Address component), following the existing changelog
style (short description, PR link and commit hash format) and place it under the
"New Features" section alongside the other entries.
In `@packages/pages-components/src/components/address/address.test.tsx`:
- Around line 103-117: The combined assertion using `expect(countryEl &&
regionEl).toBeTruthy()` can hide which element is missing; update the test "does
not apply showCountry/showRegion when custom lines are provided" to assert each
element separately by replacing the single boolean expression with two
assertions `expect(countryEl).toBeTruthy()` and `expect(regionEl).toBeTruthy()`
so failures clearly indicate whether `countryEl` or `regionEl` (queried in the
test) is missing when rendering the `Address` component with
`lines={[["region"], ["countryCode"]]}`.
- Around line 94-101: The test "hides both country and region in default format
when both are false" currently uses a combined assertion that can short-circuit;
update the assertion in address.test.tsx to assert each element individually:
check that the result of screen.queryByText("US") (countryEl) is null/falsy and
separately that screen.queryByText("AL") (regionEl) is null/falsy so both
absence conditions are verified for the Address component when showCountry and
showRegion are false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 261fb068-c8be-4ab9-b5a9-0ba2c8a29044
⛔ Files ignored due to path filters (3)
packages/pages-components/.storybook/snapshots/__snapshots__/components-address--address-hide-country-and-region.pngis excluded by!**/*.pngpackages/pages-components/.storybook/snapshots/__snapshots__/components-address--address-hide-country.pngis excluded by!**/*.pngpackages/pages-components/.storybook/snapshots/__snapshots__/components-address--address-hide-region.pngis excluded by!**/*.png
📒 Files selected for processing (5)
packages/pages-components/CHANGELOG.mdpackages/pages-components/src/components/address/address.stories.tsxpackages/pages-components/src/components/address/address.test.tsxpackages/pages-components/src/components/address/address.tsxpackages/pages-components/src/components/address/types.ts
| expect(countryEl && regionEl).toBeFalsy(); | ||
| }); | ||
|
|
||
| it("does not apply showCountry/showRegion when custom lines are provided", () => { |
There was a problem hiding this comment.
Is this what we want? Obviously an ambiguous situation, could see it either way
There was a problem hiding this comment.
Oh i had assumed yes based on:
Update pages-components to allow for hiding both country and region (separately) when using the standard formatting.
but might've misunderstood
There was a problem hiding this comment.
Hmm, I wasn't specifically thinking about this when I wrote that. I feel like we should honor the options even if you use custom. It would be dumb to use both but if you have the option then it makes sense that it works.
There was a problem hiding this comment.
I don't think you want a comma if there's no region
| expect(countryEl && regionEl).toBeFalsy(); | ||
| }); | ||
|
|
||
| it("does not apply showCountry/showRegion when custom lines are provided", () => { |
There was a problem hiding this comment.
Hmm, I wasn't specifically thinking about this when I wrote that. I feel like we should honor the options even if you use custom. It would be dumb to use both but if you have the option then it makes sense that it works.
3 participants
No description provided.