feat: add easy test data for reviews, nearby locations, and mapbox in local-editor#1256
feat: add easy test data for reviews, nearby locations, and mapbox in local-editor#1256benlife5 wants to merge 5 commits into
Conversation
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
auto-screenshot-update: true
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/visual-editor/src/local-editor/LocalEditorShell.tsx (2)
435-463: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
onClick: () => anyshould be() => void.The return value of
onClickisn't used;anyhere is looser than needed and could mask an accidental non-void return elsewhere.♻️ Suggested fix
const ToggleButton = (props: { children: React.ReactNode; - onClick: () => any; + onClick: () => void; }) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor/src/local-editor/LocalEditorShell.tsx` around lines 435 - 463, The ToggleButton props typing is too loose because onClick is declared as returning any even though the return value is unused. Update the ToggleButton component in LocalEditorShell so its onClick prop is typed as void, keeping the signature aligned with the button’s actual usage and preventing accidental non-void returns from being accepted.
178-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
currentEnvresolution logic.The
_envextraction/guard logic is duplicated verbatim between the Mapbox and Nearby Locations blocks. Also note the two blocks are inconsistent: the Mapbox block skips creating a new object when the value hasn't changed, while the Nearby Locations block always recreates the whole document unconditionally, even if the injected values are already identical.♻️ Suggested extraction
+ const getCurrentEnv = (doc: typeof nextDocument) => + "_env" in doc && doc._env && typeof doc._env === "object" + ? (doc._env as Record<string, unknown>) + : undefined; + if (typeof mapboxKey === "string") { - const currentEnv = - "_env" in nextDocument && - nextDocument._env && - typeof nextDocument._env === "object" - ? (nextDocument._env as Record<string, unknown>) - : undefined; + const currentEnv = getCurrentEnv(nextDocument);Also applies to: 197-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor/src/local-editor/LocalEditorShell.tsx` around lines 178 - 195, The `_env` parsing/update logic in LocalEditorShell is duplicated between the Mapbox and Nearby Locations sections and the two branches behave inconsistently. Extract the shared `currentEnv` resolution and env-merging behavior into a small helper near the existing update logic, then use it for both Mapbox and Nearby Locations so each only clones `nextDocument` when the injected value actually changes. Make sure the helper preserves the existing `nextDocument` shape and update semantics used by the `mapboxKey` block.packages/visual-editor/src/local-editor/LocalEditorShell.test.tsx (1)
174-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
afterEach(() => vi.restoreAllMocks())over manualmockRestore().If an assertion throws before reaching
promptSpy.mockRestore(), the spy leaks into subsequent tests. A globalafterEachavoids this class of test-isolation bug.♻️ Suggested fix
-describe("LocalEditorShell", () => { +describe("LocalEditorShell", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + beforeEach(() => {(and drop the individual
promptSpy.mockRestore()calls)Also applies to: 218-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor/src/local-editor/LocalEditorShell.test.tsx` around lines 174 - 193, The prompt spy in LocalEditorShell.test.tsx is being restored manually, which can leak mocks if an assertion fails before cleanup. Add a shared afterEach(() => vi.restoreAllMocks()) in the test file and remove the individual promptSpy.mockRestore() calls in the affected tests, including the ones around renderShell, fireEvent.click, and the Add Mapbox Key / Update Mapbox Key assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/visual-editor/src/local-editor/LocalEditorShell.tsx`:
- Around line 157-238: The memoized local value named document in
LocalEditorShell.tsx shadows the global DOM document object throughout the
component body. Rename the React.useMemo result to a non-conflicting name (for
example, previewDocument or editedDocument) and update every downstream
reference in this component, especially the consumers near the render logic, so
any future document/global DOM access remains unambiguous.
In `@packages/visual-editor/src/vite-plugin/local-editor/scaffold.ts`:
- Around line 59-62: The new scaffold entries in scaffold.ts are not actually
commented out because the `//` is inside the string literals, so
`baseLocationStream.fields` will contain invalid field ids like
"//ref_reviewsAgg.topReviews". Update the scaffold generation near the existing
placeholder entries to match the surrounding commented-item pattern by moving
the comment marker outside the string, using the same style as the nearby
entries in the scaffold builder.
---
Nitpick comments:
In `@packages/visual-editor/src/local-editor/LocalEditorShell.test.tsx`:
- Around line 174-193: The prompt spy in LocalEditorShell.test.tsx is being
restored manually, which can leak mocks if an assertion fails before cleanup.
Add a shared afterEach(() => vi.restoreAllMocks()) in the test file and remove
the individual promptSpy.mockRestore() calls in the affected tests, including
the ones around renderShell, fireEvent.click, and the Add Mapbox Key / Update
Mapbox Key assertions.
In `@packages/visual-editor/src/local-editor/LocalEditorShell.tsx`:
- Around line 435-463: The ToggleButton props typing is too loose because
onClick is declared as returning any even though the return value is unused.
Update the ToggleButton component in LocalEditorShell so its onClick prop is
typed as void, keeping the signature aligned with the button’s actual usage and
preventing accidental non-void returns from being accepted.
- Around line 178-195: The `_env` parsing/update logic in LocalEditorShell is
duplicated between the Mapbox and Nearby Locations sections and the two branches
behave inconsistently. Extract the shared `currentEnv` resolution and
env-merging behavior into a small helper near the existing update logic, then
use it for both Mapbox and Nearby Locations so each only clones `nextDocument`
when the injected value actually changes. Make sure the helper preserves the
existing `nextDocument` shape and update semantics used by the `mapboxKey`
block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 808b8ce3-4f73-4cbf-a167-cda8b0e2eabc
📒 Files selected for processing (3)
packages/visual-editor/src/local-editor/LocalEditorShell.test.tsxpackages/visual-editor/src/local-editor/LocalEditorShell.tsxpackages/visual-editor/src/vite-plugin/local-editor/scaffold.ts
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/visual-editor/src/vite-plugin/local-editor/scaffold.ts (1)
59-62: 🎯 Functional Correctness | 🟠 MajorDuplicate: the new scaffold entries still aren't actually commented out.
The
//is inside the string literal, so the generated scaffold will emit invalid field ids instead of commented-outref_reviewsAgglines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor/src/vite-plugin/local-editor/scaffold.ts` around lines 59 - 62, The new scaffold entries in scaffold.ts are being emitted as string literals that include //, so they are not actually commented out in the generated editor scaffold. Update the scaffold generation logic around the ref_reviewsAgg entries in the scaffold template to produce real commented lines, using the existing scaffold-building code in scaffold.ts rather than embedding // inside the field id strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/visual-editor/src/local-editor/LocalEditorShell.tsx`:
- Around line 205-230: The `nextDocument` update in `LocalEditorShell`
overwrites `__` instead of merging it, which can drop existing document metadata
when “Add Nearby Locations Key” runs. Update the object spread in that block so
`__` is built from the current `nextDocument.__` (similar to how `_env` uses
`currentEnv`) and only sets `isPrimaryLocale: true` while preserving any other
existing keys.
---
Duplicate comments:
In `@packages/visual-editor/src/vite-plugin/local-editor/scaffold.ts`:
- Around line 59-62: The new scaffold entries in scaffold.ts are being emitted
as string literals that include //, so they are not actually commented out in
the generated editor scaffold. Update the scaffold generation logic around the
ref_reviewsAgg entries in the scaffold template to produce real commented lines,
using the existing scaffold-building code in scaffold.ts rather than embedding
// inside the field id strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9405300c-6bfc-427d-8d36-f5d1822683d1
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/PhotoGallerySection/[desktop] version 59 with showSectionHeading false.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (3)
packages/visual-editor/src/local-editor/LocalEditorShell.test.tsxpackages/visual-editor/src/local-editor/LocalEditorShell.tsxpackages/visual-editor/src/vite-plugin/local-editor/scaffold.ts
| }, | ||
| _env: { | ||
| ...currentEnv, | ||
| YEXT_PUBLIC_VISUAL_EDITOR_APP_API_KEY: nearbyLocationsKey, |
There was a problem hiding this comment.
Does this key need to be a secret? Would be easier if you didn't need to enter it manually
benlife5
requested review from
asanehisa,
briantstephan,
jwartofsky-yext and
mkilpatrick
1 participant
DataDemo.mp4
(video pauses when I enter the api keys)