fix: enhance category selection for ios devices - #16603
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe mobile resource definition category picker configures its ChangesMobile Picker Handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Greptile SummaryUpdates the category picker’s focus behavior for iOS devices.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/components/Common/ResourceDefinitionCategoryPicker.tsx | Adds iOS-aware autofocus behavior to the category picker’s search input and drawer search tab without introducing an eligible follow-up issue. |
Reviews (7): Last reviewed commit: "fix: add autoFocus prop to TabsContent f..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
Improves touch/pen interaction handling in ResourceDefinitionCategoryPicker to make category/definition selection more reliable on iOS/touch devices.
Changes:
- Added
onPointerUphandlers to category, subcategory, and definitionCommandItems to manually trigger selection for non-mouse pointer types. - Added a guard to ignore taps on the favorite toggle button inside definition items.
Comments suppressed due to low confidence (2)
src/components/Common/ResourceDefinitionCategoryPicker.tsx:623
- Same issue as above: this
onPointerUphandler can lead to duplicatehandleCategorySelect(...)execution on touch/pen ifonSelectalso fires via a compatibilityclick. Please ensure the select logic is invoked only once per interaction (e.g., via auseRefguard shared between pointer + onSelect handlers).
onPointerUp={(e) => {
if (e.pointerType !== "mouse") {
e.preventDefault();
handleCategorySelect(category.slug, category.title);
}
src/components/Common/ResourceDefinitionCategoryPicker.tsx:661
handleDefinitionSelectis non-idempotent (mutates recent list + toggles selection). With bothonPointerUpandonSelectpresent, touch/pen interactions can potentially trigger selection twice if a compatibilityclickstill fires. Please add a single-invocation guard between pointer andonSelectpaths to ensurehandleDefinitionSelectruns exactly once per user action.
onPointerUp={(e) => {
if (e.pointerType !== "mouse") {
// Ignore taps on the favorite toggle button
if ((e.target as HTMLElement).closest("button")) return;
e.preventDefault();
handleDefinitionSelect(definition);
}
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Common/ResourceDefinitionCategoryPicker.tsx`:
- Around line 586-591: Prevent duplicate selection handling in the CommandItem
onPointerUp handlers at
src/components/Common/ResourceDefinitionCategoryPicker.tsx:586-591, 619-624, and
655-663 by removing their selection calls or gating them so pointer activation
does not also trigger the existing onSelect callbacks; ensure definition
selection cannot toggle the same multiple-selection value twice or mutate the
history list unexpectedly.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 152b54b4-5b77-42f0-9555-00461228762e
📒 Files selected for processing (1)
src/components/Common/ResourceDefinitionCategoryPicker.tsx
🎭 Playwright Test ResultsStatus: ✅ Passed
📊 Detailed results are available in the playwright-final-report artifact. Run: #10482 |
ohc-project-manager
Bot
added
Work in Progress
and removed
needs testing
needs review
labels
Deploying care-preview with
|
| Latest commit: |
0202f81
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7263fa9f.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://fix-category-selection-ios.care-preview-a7w.pages.dev |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/components/Common/ResourceDefinitionCategoryPicker.tsx:170
pointerSelectedRefcan remaintruewhenonPointerUptriggers a state change that re-renders/unmounts theCommandItembefore the subsequentonSelectfires. In that case the nextonSelect(e.g. keyboard/mouse, or after reopening) will be ignored once, causing a missed selection. Reset the ref on relevant state changes (likeopen/currentParent) so it can’t get stuck.
This issue also appears on line 673 of the same file.
// Set on touch/pen pointerup to avoid double selection from the following onSelect.
const pointerSelectedRef = useRef(false);
// Sync open state with defaultOpen prop for controlled auto-open behavior
useEffect(() => {
if (defaultOpen) {
setOpen(true);
}
}, [defaultOpen]);
src/components/Common/ResourceDefinitionCategoryPicker.tsx:680
- This change is specifically addressing touch/stylus interaction quirks, but there doesn’t appear to be a Playwright spec covering touch selection or tapping the favorite (star) control without selecting the definition. Adding a regression test using
locator.tap()/page.touchscreen.tap()would help prevent reintroducing double-selection and the favorite-tap mis-selection bug.
onPointerUp={(e) => {
if (e.pointerType !== "mouse") {
// Ignore taps on the favorite toggle button
if (e.target instanceof Element && e.target.closest("button"))
return;
pointerSelectedRef.current = true;
handleDefinitionSelect(definition);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/components/Common/ResourceDefinitionCategoryPicker.tsx:635
- Same issue as above:
pointerSelectedRefcan remaintrueifhandleCategorySelectre-renders/unmounts the item before cmdk triggersonSelect, which can cause a subsequent keyboard/mouse selection to be dropped. Reset the ref after a tick in theonPointerUppath as a fallback.
onPointerUp={(e) => {
if (e.pointerType !== "mouse") {
pointerSelectedRef.current = true;
handleCategorySelect(category.slug, category.title);
}
src/components/Common/ResourceDefinitionCategoryPicker.tsx:680
pointerSelectedRefis set totruewhen selecting a definition via touch/pen, buthandleDefinitionSelectcloses the picker (unmounting items) so cmdk may never callonSelectto clear the ref. That can cause the next (keyboard/mouse)onSelectto be ignored once. Add a timeout fallback reset after handling the pointer selection.
if (e.target instanceof Element && e.target.closest("button"))
return;
pointerSelectedRef.current = true;
handleDefinitionSelect(definition);
}
src/components/Common/ResourceDefinitionCategoryPicker.tsx:597
pointerSelectedRefis set totrueon touch/penpointerup, but ifhandleCategorySelectcauses the item to unmount before cmdk firesonSelect, the ref may never get reset. That can make the next (keyboard/mouse)onSelectget ignored once. Consider resetting the ref after the event loop tick in the pointer handler as a fallback.
This issue also appears in the following locations of the same file:
- line 631
- line 676
onPointerUp={(e) => {
if (e.pointerType !== "mouse") {
pointerSelectedRef.current = true;
handleCategorySelect(category.slug, category.title);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/components/Common/ResourceDefinitionCategoryPicker.tsx:623
- Touch/pen-specific selection behavior is now handled via pointer events + a guard ref, but there’s no automated regression test to ensure (a) tapping a category/definition triggers exactly one navigation/selection and (b) tapping the favorite button does not select/close the item. There are existing Playwright helpers/tests covering this picker via mouse clicks; adding a touch-mode spec would prevent future regressions, especially on iOS Safari.
onSelect={() => {
if (pointerSelectedRef.current) {
pointerSelectedRef.current = false;
return;
}
…s for improved usability
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/components/Common/ResourceDefinitionCategoryPicker.tsx:471
isIOSDeviceonly matchesiPhone|iPad|iPodin the user agent; on modern iPadOS Safari the UA can appear asMacintosh(desktop mode), so this still enables autofocus on iPads and reintroduces the keyboard/scroll issues this PR is trying to avoid. Consider extending the check to also treatMac+ touch (navigator.maxTouchPoints > 1) as iPadOS when deciding autofocus.
autoFocus={!isIOSDevice}
src/components/Common/ResourceDefinitionCategoryPicker.tsx:472
- This change is explicitly iOS/touch-input specific, but Playwright is currently configured to run only Desktop Chromium (Mobile Safari/WebKit projects are commented out), so there’s no automated regression coverage for the iOS behavior being adjusted here. Please add/enable a Playwright project that exercises touch/WebKit (or add a targeted spec using a touch-enabled context) to cover the iOS interaction regression this PR fixes.
autoFocus={!isIOSDevice}
/>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/components/Common/ResourceDefinitionCategoryPicker.tsx:472
- To ensure iPadOS devices using a desktop-style user agent (often detected as
Mac) don’t autofocus and pop the keyboard unexpectedly, consider disabling autofocus whenisMacDevice && isTouchDeviceas well.
placeholder={t(`search_${translationBaseKey}`)}
value={searchQuery}
onValueChange={setSearchQuery}
className="h-9 border-0 focus:ring-0 text-base sm:text-sm"
autoFocus={!isIOSDevice}
/>
src/components/Common/ResourceDefinitionCategoryPicker.tsx:53
isIOSDevicerelies on a UA match for iPhone/iPad/iPod. On iPadOS, Safari can report a desktop-style UA containingMacintosh(especially with “Request Desktop Website”), which would makeisIOSDevicefalse and re-enable autofocus on iPads. Consider expanding the import so you can treatisMacDevice && isTouchDeviceas iPadOS and disable autofocus there too.
This issue also appears on line 467 of the same file.
import { isIOSDevice } from "@/Utils/utils";
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/components/Common/ResourceDefinitionCategoryPicker.tsx:895
TabsContentpreviously used theautoFocusprop, but this change replaces it with aclassNameof "autoFocus", which has no effect (there’s no such Tailwind/util class in the repo). As a result, the search tab content will never autofocus on non‑iOS devices, and the intended conditional behavior won’t work.
Prefer keeping the actual autoFocus prop and gating it with !isIOSDevice, similar to the CommandInput change above.
<TabsContent
value="search"
className={cn("h-full mt-0", !isIOSDevice && "autoFocus")}
>
089033f to
3bd016e
Compare
…gory-selection-ios
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/components/Common/ResourceDefinitionCategoryPicker.tsx:472
isIOSDeviceis derived from/iPhone|iPad|iPod/onnavigator.userAgent, which misses iPadOS Safari when it uses the desktop UA (often reports as "Macintosh"). In that caseautoFocuswill still be enabled and the iOS keyboard/focus bug may persist on iPads. Consider switching to a more robust iPadOS/iOS detection (ideally by updating the util) before relying on it to gate focus behavior.
placeholder={t(`search_${translationBaseKey}`)}
value={searchQuery}
onValueChange={setSearchQuery}
className="h-9 border-0 focus:ring-0 text-base sm:text-sm"
autoFocus={!isIOSDevice}
/>
src/components/Common/ResourceDefinitionCategoryPicker.tsx:896
renderMainContent()already renders aCommandInputwithautoFocus, so keepingautoFocusonTabsContentis redundant and can lead to competing autofocus targets (and focus landing on a non-interactive container). Prefer removingautoFocusfromTabsContentand letting the input control focus.
<TabsContent
value="search"
className="h-full mt-0"
autoFocus={!isIOSDevice}
>
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)
src/components/Common/ResourceDefinitionCategoryPicker.tsx (1)
471-471: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an iPadOS-aware device predicate.
isIOSDeviceonly checksiPhone|iPad|iPodin the user agent, so desktop-class iPadOS presenting as a Mac reaches theseautoFocusattributes as well. Use an Apple/iOS/iPadOS-aware predicate for both the command input and the search tabs content.
src/components/Common/ResourceDefinitionCategoryPicker.tsx#L471;src/components/Common/ResourceDefinitionCategoryPicker.tsx#L895.🤖 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 `@src/components/Common/ResourceDefinitionCategoryPicker.tsx` at line 471, Replace the isIOSDevice checks controlling autoFocus in ResourceDefinitionCategoryPicker.tsx at lines 471 and 895 with an Apple/iOS/iPadOS-aware device predicate, ensuring autoFocus is disabled for desktop-class iPadOS as well as traditional iOS devices at both the command input and search tabs content.Source: MCP tools
🤖 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 `@src/components/Common/ResourceDefinitionCategoryPicker.tsx`:
- Line 471: Replace the isIOSDevice checks controlling autoFocus in
ResourceDefinitionCategoryPicker.tsx at lines 471 and 895 with an
Apple/iOS/iPadOS-aware device predicate, ensuring autoFocus is disabled for
desktop-class iPadOS as well as traditional iOS devices at both the command
input and search tabs content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea50fd40-d1ea-4dc8-955f-14a6b6398fd6
📒 Files selected for processing (1)
src/components/Common/ResourceDefinitionCategoryPicker.tsx
…initionCategoryPicker
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/components/Common/ResourceDefinitionCategoryPicker.tsx`:
- Around line 797-800: Update the `repositionInputs` decision in
`ResourceDefinitionCategoryPicker` to also identify iPadOS desktop-mode
sessions, using a `MacIntel` platform combined with touch support alongside the
existing `isIOSDevice` check. Disable repositioning for either detected iOS or
iPadOS desktop-mode environments while preserving the current behavior on other
platforms.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6d4dba89-32cc-4e1d-b8d1-12591af9b11d
📒 Files selected for processing (1)
src/components/Common/ResourceDefinitionCategoryPicker.tsx
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/components/Common/ResourceDefinitionCategoryPicker.tsx:797
repositionInputs={false}is applied for all mobile devices here, but the PR description indicates the issue is iOS-specific. Disabling Vaul input repositioning globally on mobile could regress Android behavior (keyboard/scroll handling). Consider making this conditional (e.g.,repositionInputs={!isIOSDevice}) likeTagConfigFormDrawerdoes.
<Drawer
open={open}
repositionInputs={false}
onOpenChange={(newOpen) => {
This pull request updates the
ResourceDefinitionCategoryPickercomponent to improve compatibility with iOS devices by adjusting focus behaviour.Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary