Feat/tier 1 improvements - #3
Conversation
The codeql-db-js/ directory is a local CodeQL CLI artifact (database snapshot produced by `codeql database create`). It is not source and should not be in version control — it bloats clones and re-creating it is the standard local workflow. - Add codeql-db-js/ and codeql-db-*/ to .gitignore - Untrack the currently checked-in copy
DrumPicker now accepts either string items (existing behavior) or labeled
items of the form { label, value } where `value` can be any typed payload
(string, number, enum, object). The label is what the wheel displays; the
resolved `value` is surfaced on `onChange` as `nativeEvent.item`, fully
typed via the new generic parameter on DrumPicker<T>.
Why: callers currently have to maintain a parallel lookup array to map the
selected index/label back to a domain id. This is the single most common
ergonomics ask for picker components and removes a class of off-by-one
bugs when the items list changes.
Implementation notes:
- The native Fabric spec still receives string[] only. The JS wrapper
extracts labels via getItemLabel() before handing to native and looks
up the resolved value via getItemValue() when the native onValueChange
fires.
- Fully back-compatible: existing string[] callers see no change at
runtime. The native event payload gains an `item` field; for plain
strings, `item === value`, so any code reading either keeps working.
- Date/Time wrappers are unchanged — they already pass strings.
Tests: 6 new behavior tests cover string back-compat, labeled items,
typed object values, mixed arrays, and out-of-bounds index fallback.
Full suite: 42/42 green.
Previously, importing DrumPicker on web (Expo Web, react-native-web, or any SSR context) threw at module load. This made the package unsafe to import from cross-platform code without a Platform.OS branch at every import site. The web variant (src/DrumPicker.tsx) now renders a real HTML <select> element: - SSR-safe: no throw at module evaluation - Keyboard-navigable and screen-reader-friendly by default (browser semantics) - Honors the same value/onChange contract as native — callers read event.nativeEvent.index and event.nativeEvent.value identically - Mirrors native's "no duplicate-index re-emit" behavior so controlled state loops are avoided Props that translate to web (items, selectedIndex, textColor, selectedTextColor, textSize, backgroundColor, visibleItemCount as <select size>, testID, onChange, style.height/width) are honored. Props that do not translate (haptics, custom indicators) are accepted and ignored — no warnings on the hot path. A full drum-style scroll wheel on web is a separate, larger feature. This patch establishes a correct baseline so the lib is usable from shared cross-platform components today. Tests: - 8 new tests in DrumPickerWeb.test.tsx exercise the fallback via react-test-renderer (imported with explicit .tsx extension to bypass the react-native preset's .native.tsx resolution). - Full suite: 44/44 green (36 prior + 8 new). No native code changed; android-build, ios-build, android-instrumented, and ios-unit-tests CI jobs are unaffected.
Composes existing DrumPicker primitives (no native changes) to provide a time-of-day picker that mirrors UIDatePicker.time behavior: - Six modes: hour, minute, hour-minute, hour-minute-second, hour-minute-period (12h), hour-minute-second-period - Always-24h value contract on value / onChange; component handles 12h display + AM/PM internally - minuteInterval and secondInterval (1, 2, 3, 4, 5, 6, 10, 12, 15, 20, 30) matching iOS UIDatePicker - padWithZero, custom amLabel / pmLabel for localization - Controlled + uncontrolled, with the same clamp-and-notify contract as DateDrumPicker (out-of-range minute snaps and emits once) - Per-column testIDs and styles (hour / minute / second / period) Pure TypeScript, reuses DrumPicker so no Android or iOS code is touched. Adds 50 jest tests covering logic and component behavior; full suite is 86/86 green.
|
Warning Review limit reached
More reviews will be available in 58 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe PR extends the drum picker library with generic typed items, a web fallback for SSR environments, and a new composed ChangesDrum Picker Generics + Time Picker Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant DrumPicker
participant Types as getItemLabel/<br/>getItemValue
participant NativeModule as Native Module<br/>(or HTMLSelect)
User->>DrumPicker: Pass items: { label, value }[]
DrumPicker->>Types: Extract labels for display
DrumPicker->>NativeModule: Render with labels
User->>NativeModule: Select item at index
NativeModule-->>DrumPicker: Emit index + native value
DrumPicker->>Types: Resolve item value via getItemValue
DrumPicker-->>User: onChange with nativeEvent.item: T
sequenceDiagram
participant User
participant TimeDrumPicker
participant TimeLogic
participant DrumPicker
User->>TimeDrumPicker: Set value: { hour, minute, second }
TimeDrumPicker->>TimeLogic: Clamp and normalize hour/minute/second
TimeLogic-->>TimeDrumPicker: Clamped values + period (for 12h)
TimeDrumPicker->>DrumPicker: Render columns with clamped indices
User->>DrumPicker: Spin hour column
DrumPicker-->>TimeDrumPicker: onValueChange with index
TimeDrumPicker->>TimeLogic: Convert index to 24h hour + snap minute
TimeLogic-->>TimeDrumPicker: Updated hour/minute/second
TimeDrumPicker-->>User: onChange with clamped 24h value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
281-297:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
nativeEvent.itemin the API table to match the new contract.The
onChangerow still showsnativeEvent: { index, value }, but the new labeled-items contract also exposesnativeEvent.item. Please update this row so the API reference is consistent.🤖 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 `@README.md` around lines 281 - 297, Update the README API table row for onChange to reflect the new event contract: change the nativeEvent shape from { index, value } to { index, value, item } so labeled items expose the full item object; edit the `onChange` row in the props table (the line currently showing `nativeEvent: { index, value }`) to `nativeEvent: { index, value, item }` and adjust any brief description if present to mention the new `item` field.
🤖 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/DrumPicker.tsx`:
- Around line 176-183: The hardcoded 'aria-label': 'Picker' makes every
DrumPicker column identical to assistive tech; update the select element to use
a per-instance accessibility prop (e.g. accept an accessibilityLabel or
ariaLabel prop on the DrumPicker component) and set the element's
aria-label/aria-labelledby to that prop (falling back to the existing testID or
a sensible default) instead of the constant string; locate the select render in
DrumPicker.tsx (the block using safeIndex, handleChange, visibleItemCount,
testID) and wire the new prop through to the select so each
DateDrumPicker/TimeDrumPicker column can provide a distinct label.
In `@src/timeDrumPickerLogic.ts`:
- Around line 69-83: The function snapToInterval currently uses Math.round which
breaks the documented "ties round down" behavior (e.g., 3 with interval 2 goes
to 4); change the snapping logic in snapToInterval to compute the nearest
multiple using floor-based tie-breaking instead of Math.round — e.g., divide
safe by interval and use Math.floor with an adjustment that treats exact halves
as rounding down (for example subtracting a tiny epsilon or using an
integer-safe formula) before multiplying back by interval, then keep the
existing clamping to 0..max; leave clampMinute, interval checks and final
min/max logic intact.
In `@src/types.ts`:
- Around line 16-17: DrumPickerItem<T> currently permits raw string items for
all T, which lets DrumPicker<number> accept plain strings and breaks the
onChange types; update the DrumPickerItem generic so that raw string items are
only allowed when T is string (e.g. conditionally allow string |
DrumPickerLabeledItem<T> only for T = string), otherwise require
DrumPickerLabeledItem<T>; change the type alias named DrumPickerItem and verify
usages in DrumPicker and any onChange/nativeEvent typing still reflect the
tighter constraint.
---
Outside diff comments:
In `@README.md`:
- Around line 281-297: Update the README API table row for onChange to reflect
the new event contract: change the nativeEvent shape from { index, value } to {
index, value, item } so labeled items expose the full item object; edit the
`onChange` row in the props table (the line currently showing `nativeEvent: {
index, value }`) to `nativeEvent: { index, value, item }` and adjust any brief
description if present to mention the new `item` field.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc336822-0c51-4d4a-8bb9-482f0c1d0152
⛔ Files ignored due to path filters (1)
codeql-db-js/log/database-create-20260523.162940.875.logis excluded by!**/*.log
📒 Files selected for processing (18)
.gitignoreREADME.mdcodeql-db-js/baseline-info.jsoncodeql-db-js/codeql-database.ymlcodeql-db-js/diagnostic/cli-diagnostics-add-20260523T112944.961Z.jsoncodeql-db-js/diagnostic/cli-diagnostics-add-20260523T120803.711Z.jsoncodeql-db-js/diagnostic/extractors/javascript/autobuilder-1.jsonlsrc/DrumPicker.native.tsxsrc/DrumPicker.tsxsrc/TimeDrumPicker.tsxsrc/__tests__/DrumPicker.labeledItems.test.tsxsrc/__tests__/DrumPicker.test.tsxsrc/__tests__/DrumPickerWeb.test.tsxsrc/__tests__/TimeDrumPicker.test.tsxsrc/__tests__/timeDrumPickerLogic.test.tssrc/index.tsxsrc/timeDrumPickerLogic.tssrc/types.ts
💤 Files with no reviewable changes (5)
- codeql-db-js/baseline-info.json
- codeql-db-js/diagnostic/cli-diagnostics-add-20260523T120803.711Z.json
- codeql-db-js/codeql-database.yml
- codeql-db-js/diagnostic/cli-diagnostics-add-20260523T112944.961Z.json
- codeql-db-js/diagnostic/extractors/javascript/autobuilder-1.jsonl
Code review — Tier 1 improvementsThanks for the work on labeled items, web fallback, and Summary
Overall the PR delivers what it claims; the main gaps are example demos, line endings / lint, and PR metadata. What looks good
High priority1. Example app has no demos for new APIsChanges are almost entirely under
Ask: Please add a small demo (e.g. a tab or section with one labeled picker and one 2. Lint — CRLF line endingsSeveral files under Fix: yarn eslint "**/*.{js,ts,tsx}" --fixThen commit with LF line endings (editor: LF for the repo). 3. PR description and checklistThe PR body is empty and checklist items (tested on Android/iOS, etc.) are unchecked. Ask: Please fill in:
Medium priority4. Web UX expectationsOn web, 5. Labeled items scope
Suggestion: Add one sentence in README under labeled items so consumers do not expect labeled API on date/time pickers yet. 6. Comment drift in
|
CodeRabbit:
- snapToInterval now rounds half-step ties DOWN as documented (was
Math.round, which rounds halves up); added explicit tie-break tests.
- DrumPickerItem<T> tightened so plain string items are only allowed when
T = string. DrumPicker<number> now requires {label, value} pairs, making
the generic onChange.item contract sound.
- aria-label is no longer hardcoded "Picker": new accessibilityLabel prop
flows to the native view and the web <select>. DateDrumPicker and
TimeDrumPicker give each column a distinct default label
(Day/Month/Year, Hour/Minute/Second/AM-PM) via columnAccessibilityLabels.
Maintainer (scrollDynasty):
- Example app: added "Labeled" (typed-value DrumPicker) and "TimePicker"
(TimeDrumPicker) demo tabs so the new APIs are verifiable on device.
- Extracted shared DRUM_PICKER_DEFAULTS so web and native cannot drift.
- Fixed misleading getItemLabel comment in types.ts.
- README: documented labeled-items scope (DrumPicker only), the
TimeDrumPicker controlled clamp-and-notify behavior, and added an
Accessibility section.
Verification: typecheck, build, and 110 jest tests across 11 suites all
pass locally (added accessibility + tie-break coverage). eslint clean on
src (the only local failures are CRLF from the Windows working copy; the
committed blobs are LF).
- Remove stray scratch text that leaked into the labeled-items section during an earlier edit. - Add the labeled-items scope note (DrumPicker only; Date/Time pickers use their own structured onChange). - Add an Accessibility section documenting accessibilityLabel and per-column columnAccessibilityLabels.
|
@copilot resolve the merge conflicts in this pull request |
|
@Abdullajon1881 исправь конфликты |
2 participants
Summary
What does this PR change and why?
Type of change
Checklist
yarn lint,yarn build,yarn typecheck, andyarn testfrom the repo rootyarn.lockifpackage.jsondependencies changedsrc/,android/, orios/(example app only)CI
PRs to
mainrun the full CI workflow automatically. New tests in standard locations are picked up without editingci.yml.Screenshots / recordings (UI changes)
If applicable, add before/after visuals.
Related issues
Fixes # (issue number)
Summary by CodeRabbit
Release Notes
New Features
TimeDrumPickercomponent for flexible time selection with 12/24-hour format support and configurable minute/second intervals.<select>rendering.Documentation
Tests
Chores