Fix mobile input focus loss during tooltip repositioning#3441
Conversation
|
Someone is attempting to deploy a commit to the shipshapecode Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe pull request gates tooltip focus behavior to occur only after the first render. A ChangesFocus-gate for tooltip positioning
Dev dependency pin
Sequence DiagramsequenceDiagram
participant setupTooltip
participant setPosition
participant computePosition
participant stepElement
setupTooltip->>setPosition: first call (shouldFocusAfterRender=true)
setPosition->>computePosition: computePosition(target, stepElement)
computePosition-->>setPosition: placement & coords
setPosition->>stepElement: set tabIndex & focus()
setupTooltip->>setupTooltip: shouldFocusAfterRender = false
Note over setupTooltip: autoUpdate callback fires
setupTooltip->>setPosition: subsequent call (shouldFocusAfterRender=false)
setPosition->>computePosition: computePosition(target, stepElement)
computePosition-->>setPosition: placement & coords
Note over setPosition: skip focus sequence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shepherd.js/package.json (1)
96-96: ⚡ Quick winClarify the rationale for pinning
eslint-plugin-cypressto version 5.3.0.This change pins
eslint-plugin-cypressto exactly5.3.0, removing the caret range (^) used by other dev dependencies. While version 5.3.0 has no known security advisories, it is 2 major versions behind the current release (6.4.1). If this pin is intentional—such as to resolve the pnpm install conflicts mentioned in the PR summary—document the reason in the commit message or add a comment explaining why this package uses a different versioning strategy than peers.🤖 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 `@shepherd.js/package.json` at line 96, The package.json change pins eslint-plugin-cypress to exactly 5.3.0; either document why (e.g., to resolve pnpm install conflicts) or revert to caret semantics to match peers. Update the commit message and PR description to state the rationale for pinning eslint-plugin-cypress@5.3.0 (and any follow-up plan to upgrade), or change the package.json entry for eslint-plugin-cypress back to using ^5.3.0 if the pin was accidental; ensure the explanation references eslint-plugin-cypress and package.json so reviewers can find the intent.
🤖 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.
Nitpick comments:
In `@shepherd.js/package.json`:
- Line 96: The package.json change pins eslint-plugin-cypress to exactly 5.3.0;
either document why (e.g., to resolve pnpm install conflicts) or revert to caret
semantics to match peers. Update the commit message and PR description to state
the rationale for pinning eslint-plugin-cypress@5.3.0 (and any follow-up plan to
upgrade), or change the package.json entry for eslint-plugin-cypress back to
using ^5.3.0 if the pin was accidental; ensure the explanation references
eslint-plugin-cypress and package.json so reviewers can find the intent.
Fixes #1143.
This keeps the existing focus-after-render behavior for the initial step render, but skips the delayed
step.el.focus()on later Floating UI auto-update/reposition calls. That prevents mobile viewport/keyboard resize updates from stealing focus back from an input attached to the active step.The regression test covers the focus behavior directly: the step element is focused once after initial render, then a focused input remains focused after a later reposition update.
Checks run:
Notes:
CYPRESS_INSTALL_BINARY=0 npx pnpm install --frozen-lockfileis currently blocked by a duplicate@vitest/pretty-format@4.1.5mapping inpnpm-lock.yaml.Summary by CodeRabbit
Bug Fixes
Tests
Chores