fix(core): select imported/shared components in the inspector - #367
fix(core): select imported/shared components in the inspector#367stantheman0128 wants to merge 2 commits into
Conversation
Allow fiber hit-testing to resolve slide call sites for hosts authored outside slides/, and prefer that match over a tagged ancestor wrapper. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@stantheman0128 is attempting to deploy a commit to the open-slide Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe inspector now resolves imported and nested shared components through React fiber source locations, while preserving direct and ancestor location-tag handling. Overlay selection, target recovery, fixtures, unit tests, end-to-end tests, and release metadata are updated. ChangesShared component inspector selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant InspectorOverlay
participant findSlideSource
participant ReactFiber
participant InspectorPanel
InspectorOverlay->>findSlideSource: resolve clicked shared-component element
findSlideSource->>ReactFiber: walk debug source chain
ReactFiber-->>findSlideSource: return slide call-site and host anchor
findSlideSource-->>InspectorOverlay: return selection hit
InspectorOverlay->>InspectorPanel: provide selected target
InspectorPanel->>findSlideSource: resolve or recover target anchor
findSlideSource-->>InspectorPanel: return source line, column, and anchor
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/app/components/inspector/inspector-panel.tsx (1)
1096-1107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
findElementByLinefallback loop ignorescolumn, and this PR increases the odds of collisions.The tagged lookup at Line 1099 matches on the full
${line}:${column}selector, but the fallback loop at Line 1102-1105 only checkshit.line === line. Now thatfindSlideSourceresolves shared/nested component call sites withouthostOnly, multiple elements rendered from different call sites on the same line but different columns (e.g.<Heading>A</Heading><Heading>B</Heading>on one line) can all satisfyhit.line === line, and the loop returns the first DOM-order match rather than the correct one — potentially re-selecting the wrong element after an edit.🐛 Proposed fix
for (const el of candidates) { const hit = findSlideSource(el, slideId); - if (hit && hit.line === line) return hit.anchor; + if (hit && hit.line === line && hit.column === column) return hit.anchor; }🤖 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/core/src/app/components/inspector/inspector-panel.tsx` around lines 1096 - 1107, Update the fallback loop in findElementByLine to require both hit.line === line and hit.column === column before returning hit.anchor. Preserve the existing full line:column tagged lookup and continue scanning candidates until the exact source location is found.
🧹 Nitpick comments (1)
packages/core/src/app/lib/inspector/fiber.test.ts (1)
176-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a real DOM host for the shared-component anchor tests.
The shared-component tests stub
HTMLElementto a non-extends class, so in the Node test environment the productionfiber.stateNode instanceof HTMLElementcheck is not exercised.findViaFiber()still returnselasanchorbecause it was initialized toelandhostOnlystays false, soexpect(hit?.anchor).toBe(el)passes without testing the intended host-anchor path. Since the core production code relies on the realHTMLElementconstructor, use real DOM nodes in these cases (document.createElement('div')) instead of the fakes.🤖 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/core/src/app/lib/inspector/fiber.test.ts` around lines 176 - 199, Update the shared-component anchor test around findSlideSource to use real DOM elements created with document.createElement('div') for hostEl and the clicked element, rather than fake HTMLElement instances. Keep the fiber setup and anchor assertions intact so the production stateNode instanceof HTMLElement path in findViaFiber is exercised.
🤖 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.
Outside diff comments:
In `@packages/core/src/app/components/inspector/inspector-panel.tsx`:
- Around line 1096-1107: Update the fallback loop in findElementByLine to
require both hit.line === line and hit.column === column before returning
hit.anchor. Preserve the existing full line:column tagged lookup and continue
scanning candidates until the exact source location is found.
---
Nitpick comments:
In `@packages/core/src/app/lib/inspector/fiber.test.ts`:
- Around line 176-199: Update the shared-component anchor test around
findSlideSource to use real DOM elements created with
document.createElement('div') for hostEl and the clicked element, rather than
fake HTMLElement instances. Keep the fiber setup and anchor assertions intact so
the production stateNode instanceof HTMLElement path in findViaFiber is
exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de631e5c-451b-41f8-b7f2-66d53fb2f6a6
📒 Files selected for processing (9)
.changeset/shared-inspector-select.mdpackages/core/e2e/fixture/components/shared.tsxpackages/core/e2e/fixture/slides/shared-select/index.tsxpackages/core/e2e/fixture/tsconfig.jsonpackages/core/e2e/tests/inspector.spec.tspackages/core/src/app/components/inspector/inspect-overlay.tsxpackages/core/src/app/components/inspector/inspector-panel.tsxpackages/core/src/app/lib/inspector/fiber.test.tspackages/core/src/app/lib/inspector/fiber.ts
Dropping `hostOnly` from the inspector's re-resolution scan widened the set of elements that can answer for a given source line, and that scan only compared the line. Several elements routinely originate on one line, so after an HMR round that detached the selected node the inspector could hand back whichever element the DOM walk reached first. Prefer a line and column hit, and keep a line-only hit as a last resort so an edit that shifts a column degrades to the previous behavior rather than dropping the selection. Also add a fiber test that distinguishes host-anchor resolution from the seeded anchor. The existing imported-call-site test points the host fiber at the clicked element, which is also `anchor`'s initial value, so it passes whether or not the host check runs; forcing `isHost` to false leaves it green. The new test uses a separate host element and fails under that mutation. Document the inspector's three resolution strategies and why they are ordered the way they are.
|
Addressed CodeRabbit feedback on Stan Shih (@stantheman0128) |
|
Follow-up: fork already had the column-aware fallback at \5f7678e\ (exact column first, line-only fallback); no additional push needed. |
2 participants
Summary
Fixes #327.
In inspect mode, clicking an element rendered by an imported/shared component produced no selection (or, when nested under a tagged slide host, selected the wrong ancestor). Host JSX authored outside
slides/never getsdata-slide-loc, and the fiber fallback required a host fiber whose_debugSourcepointed at the slide file, so component call sites were skipped.data-slide-locon the clicked elementhostOnlyat inspector call sites), anchoring highlight to the nearest descendant hostEvidence (product path)
Scratch e2e fixture
shared-selectwithcomponents/shared.tsx(Heading/Card), Playwright against the live inspector.Before (main HEAD): click Shared heading click target with Inspect on -> inspector panel stays closed (
panelVisible: false).After (this branch): same click opens the panel with Element text
Shared heading click target(host styles 64px / weight 700). Nested Card > Heading (Nested shared heading) also selects.Screenshots / logs live under
handoff/evidence-open-slide-327/on the contributor machine (before unselectable, after shared + nested selected).What was not tested
style/ props (selection + source mapping are fixed; forwarding is still per-component)FORWARDING_COMPONENTS(issue alternative); not needed for selectionAI assistance
This change was prepared with AI assistance (Cursor/Grok). I reviewed the diff, ran the unit and Playwright checks above, and verified the inspector click path before/after on the shared-component fixture.
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests