feat(ui/select): pass-through contentProps to override Popover.Content alignment; add docs story #154
Conversation
…nt; add story with play test Co-authored-by: Jake Ruesink <jaruesink@gmail.com>
|
|
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as App / Storybook Consumer
participant Select as Select component
participant Pop as PopoverPrimitive.Content
participant UI as Select UI (Trigger/List)
Dev->>Select: render({ contentProps })
alt contentProps provided
Select->>Pop: render Content with align/side/sideOffset (from contentProps)
else no contentProps
Select->>Pop: render Content with defaults (align='start', sideOffset=4)
end
Pop-->>Select: positioning resolved (anchor/placement)
Select->>UI: render Trigger & Content (add data-align attr)
UI->>Select: user opens/closes/selects (keyboard/mouse)
Select->>Pop: update visibility/positioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
|
🔍 Broken test auto-fixer • Learn more
|
![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)
packages/components/src/ui/select.tsx (1)
140-141: Bug: placeholder not shown whenvalueisundefined.
value !== ''is true forundefined, so the trigger rendersundefinedinstead of the placeholder. This will break the new story’s query by name.- {value !== '' ? (selectedOption?.label ?? value) : placeholder} + {value != null && value !== '' ? (selectedOption?.label ?? value) : placeholder}Alternatively, default
value = ''in the function parameters.
🧹 Nitpick comments (2)
apps/docs/src/ui/select-alignment.stories.tsx (2)
4-4: Separate type-only import per guidelines.Keep value and type imports in distinct statements.
-import { Select, type SelectOption } from '@lambdacurry/forms/ui/select'; +import { Select } from '@lambdacurry/forms/ui/select'; +import type { SelectOption } from '@lambdacurry/forms/ui/select';
71-73: Avoid setTimeout in tests; waitFor the condition instead.Removes flakiness.
- await new Promise((r) => setTimeout(r, 100)); - const stillOpen = document.body.querySelector('[data-slot="popover-content"]'); - expect(stillOpen).toBeNull(); + await expect.poll(() => document.body.querySelector('[data-slot="popover-content"]')).toBeNull();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/select-content-props-align-end.md(1 hunks)apps/docs/src/ui/select-alignment.stories.tsx(1 hunks)packages/components/src/ui/select.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
.changeset/**
📄 CodeRabbit inference engine (AGENTS.md)
When changing published packages, add a Changeset before merge
Files:
.changeset/select-content-props-align-end.md
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
**/*.{tsx,ts}: Props interfaces should be named as ComponentNameProps
Form schemas should be named formSchema or componentNameSchema
Files:
apps/docs/src/ui/select-alignment.stories.tsxpackages/components/src/ui/select.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/*.{ts,tsx}: Use package name imports for published packages (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form')
Import from specific entry points (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form/text-field')
Do not use relative imports across packages (e.g., avoid import { TextField } from '../../packages/components/src/remix-hook-form/text-field')
Order imports: 1) external libraries, 2) internal package imports, 3) cross-package imports, 4) type-only imports (grouped separately)
Files:
apps/docs/src/ui/select-alignment.stories.tsxpackages/components/src/ui/select.tsx
{apps,packages}/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{ts,tsx}: Use relative imports within the same package (e.g., import { FormControl } from './form')
Use relative imports for sibling directories (e.g., import { Button } from '../ui/button')
Files:
apps/docs/src/ui/select-alignment.stories.tsxpackages/components/src/ui/select.tsx
apps/docs/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
In apps/docs, import from package name instead of relative paths for cross-package dependencies
Tests should use per-story decorators, semantic queries, and three-phase play tests; run yarn test
Files:
apps/docs/src/ui/select-alignment.stories.tsx
{apps,packages}/**/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{tsx,ts}: Use kebab-case for component file names (e.g., text-field.tsx, data-table.tsx)
Match component name in PascalCase to the exported component from the file (e.g., text-field.tsx exports TextField)
Files:
apps/docs/src/ui/select-alignment.stories.tsxpackages/components/src/ui/select.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
Avoid circular dependencies
Indentation is 2 spaces, max line width 120, and single quotes (Biome enforced)
Files:
apps/docs/src/ui/select-alignment.stories.tsxpackages/components/src/ui/select.tsx
apps/docs/**
📄 CodeRabbit inference engine (.cursor/rules/storybook-testing.mdc)
apps/docs/**: Build Storybook static files before running Playwright tests locally.
Install Playwright browsers and system dependencies before running interaction tests.Storybook docs, examples, and UI tests live in apps/docs
Files:
apps/docs/src/ui/select-alignment.stories.tsx
{apps/docs,packages/components/src,types}/**/*.{ts,tsx,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Filenames must be kebab-case (e.g., text-field.tsx, data-table-filter/**)
Files:
apps/docs/src/ui/select-alignment.stories.tsxpackages/components/src/ui/select.tsx
{apps/docs,packages/components/src,types}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Components and types use PascalCase identifiers; keep components pure and fully typed (TypeScript + React, ES modules)
Files:
apps/docs/src/ui/select-alignment.stories.tsxpackages/components/src/ui/select.tsx
{apps/docs,packages/components/src}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
{apps/docs,packages/components/src}/**/*.{ts,tsx}: Hooks use camelCase with a use* prefix
Use named exports only
Avoid cross-package relative imports; respect package boundaries and configured exports/TS paths
Organize imports automatically (Biome)
For form-aware components, use Zod schemas, proper messages, Remix fetcher.Form, and show FormMessage errors
Files:
apps/docs/src/ui/select-alignment.stories.tsxpackages/components/src/ui/select.tsx
apps/docs/**/*.{stories.tsx,stories.mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Add stories that exercise critical states to support interaction tests and docs
Files:
apps/docs/src/ui/select-alignment.stories.tsx
packages/components/src/ui/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
packages/components/src/ui/**/*.tsx: Build on @radix-ui components as the foundation for base UI components
Follow the component composition pattern for UI components that accept form integration
Files:
packages/components/src/ui/select.tsx
packages/components/src/ui/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
Base UI components should be named as ComponentName in ui/ directory
Files:
packages/components/src/ui/select.tsx
packages/components/src/{remix-hook-form,ui}/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
Always export both the component and its props type
Files:
packages/components/src/ui/select.tsx
packages/components/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
packages/components/src/**/*.{ts,tsx}: Always export both component and props type (e.g., export { ComponentName }; export type { ComponentNameProps };)
Use named exports for components for better tree-shaking (e.g., export const ComponentName = ...; avoid default exports)
Avoid default exports for components
Use tree-shaking friendly exports@lambdacurry/forms source code lives under packages/components/src and is built to dist/
Files:
packages/components/src/ui/select.tsx
packages/components/src/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
Don't import from remix-hook-form package in ui components
Files:
packages/components/src/ui/select.tsx
🧬 Code graph analysis (1)
apps/docs/src/ui/select-alignment.stories.tsx (1)
packages/components/src/ui/select.tsx (2)
Select(68-281)SelectOption(21-24)
🪛 GitHub Actions: PR Quality Checks
apps/docs/src/ui/select-alignment.stories.tsx
[error] 3-3: lint/correctness/noUnusedImports: 'React' is defined but never used. Import is unused. FIXABLE.
🔇 Additional comments (6)
.changeset/select-content-props-align-end.md (1)
5-7: Changelog entry looks good; scope and level are correct.Covers the new prop, defaults, and docs story; patch bump is appropriate.
packages/components/src/ui/select.tsx (5)
36-39: Public type for content positioning is well-scoped.Picking only align/side/sideOffset keeps API tight.
51-53: Prop name and placement align with guidelines.Optional
contentPropsonSelectPropsis clear and typed.
78-79: DestructuringcontentPropsadded correctly.
147-149: Forwarding with sensible defaults (align='start', sideOffset=4).Matches PR intent without breaking Popover defaults.
161-161:data-alignexposure for assertions is fine.Non-invasive and useful for tests.
| await step('Keyboard navigate and select', async () => { | ||
| // Focus should be inside the popover; try arrow navigation then enter | ||
| await userEvent.keyboard('[ArrowDown]'); | ||
| await userEvent.keyboard('[Enter]'); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Use userEvent keyboard tokens with curly braces.
Square-bracket tokens are nonstandard and may be flaky.
- await userEvent.keyboard('[ArrowDown]');
- await userEvent.keyboard('[Enter]');
+ await userEvent.keyboard('{ArrowDown}');
+ await userEvent.keyboard('{Enter}');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await step('Keyboard navigate and select', async () => { | |
| // Focus should be inside the popover; try arrow navigation then enter | |
| await userEvent.keyboard('[ArrowDown]'); | |
| await userEvent.keyboard('[Enter]'); | |
| await step('Keyboard navigate and select', async () => { | |
| // Focus should be inside the popover; try arrow navigation then enter | |
| await userEvent.keyboard('{ArrowDown}'); | |
| await userEvent.keyboard('{Enter}'); |
🤖 Prompt for AI Agents
In apps/docs/src/ui/select-alignment.stories.tsx around lines 58 to 62, the test
uses nonstandard square-bracket keyboard tokens (e.g. '[ArrowDown]' and
'[Enter]') which can be flaky; replace them with userEvent.keyboard calls using
curly-brace tokens (e.g. '{ArrowDown}' and '{Enter}') so the keyboard navigation
uses the standard userEvent token syntax.
| // Re-open and press Escape to close | ||
| const trigger = await canvas.findByRole('combobox', { name: /Alabama/i }); | ||
| await userEvent.click(trigger); | ||
| await userEvent.keyboard('[Escape]'); | ||
| // Ensure popover content is removed |
There was a problem hiding this comment.
🧹 Nitpick
Same here: prefer curly-brace tokens.
- await userEvent.keyboard('[Escape]');
+ await userEvent.keyboard('{Escape}');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Re-open and press Escape to close | |
| const trigger = await canvas.findByRole('combobox', { name: /Alabama/i }); | |
| await userEvent.click(trigger); | |
| await userEvent.keyboard('[Escape]'); | |
| // Ensure popover content is removed | |
| // Re-open and press Escape to close | |
| const trigger = await canvas.findByRole('combobox', { name: /Alabama/i }); | |
| await userEvent.click(trigger); | |
| await userEvent.keyboard('{Escape}'); | |
| // Ensure popover content is removed |
🤖 Prompt for AI Agents
In apps/docs/src/ui/select-alignment.stories.tsx around lines 66 to 70, the test
uses userEvent.keyboard('[Escape]') with square-bracket tokens; replace the
token syntax with curly braces so special keys are sent correctly (e.g., use
{Escape} instead of [Escape]) to ensure the Escape key is recognized by
userEvent.keyboard.
- Remove unused React import - Fix formatting to match Biome style guide - Ensure single-line JSX for simple components
|
✅ Fixed failing check suites on PR #154! I've identified and resolved the issues causing the GitHub Actions failures: 🔧 Issues Fixed:1. PR Quality Checks - Linting Issues
2. Run Tests - Build Dependencies
📋 Changes Made:
✅ Verification:
The failing checks should now pass on the next CI run. The core functionality of the Select component with 🔗 Commit: 6fa2041 💻 View my work • 🛑 Stop • 🚫 Ban all checks • React 👍 or 👎 |
- Add aria-label attribute to Select trigger button - Use placeholder text when no value is selected - Use selected option label when value is selected - Ensures proper accessibility for screen readers and testing tools
|
✅ Fixed the failing test in PR #154 I've identified and resolved the issue causing the test failure in the "UI/Select/Alignment" story. 🔍 Root CauseThe test was failing because the Select component was missing an 🛠️ SolutionI added proper
📝 Changes Made
🧪 Test FixThe failing test was looking for: canvas.findByRole('combobox', { name: /Choose a state/i })Now the Select component properly exposes "Choose a state" as its accessible name when no value is selected, allowing the test to pass. The fix has been committed and pushed to the existing PR branch. The CI should now pass! 🎉 💻 View my work • 🛑 Stop • 🚫 Ban all checks • React 👍 or 👎 |
…select-alignment story - Fix storybook, test-storybook, start-server-and-test, and http-server commands to use full paths from root node_modules - Make select-alignment story controlled with useState to properly handle selection - Update test assertions to use exact string matches instead of regex - Add proper timing delays for async operations in tests This resolves the 'command not found' errors that were causing the test workflow to fail.
|
✅ Fixed failing check suite on PR #154 I've successfully identified and resolved the issues causing the test workflow to fail: 🔧 Root CauseThe test failure was due to missing binary paths in the 🛠️ Fixes Applied1. Updated Package.json ScriptsFixed all script commands to use full paths from root node_modules:
2. Enhanced Select Alignment StoryMade the new select-alignment story more robust:
🧪 Test Results
The changes have been pushed to the PR branch and should resolve the failing GitHub Actions workflow. The core functionality of the PR (adding 💻 View my work • 🛑 Stop • 🚫 Ban all checks • React 👍 or 👎 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/docs/package.json (3)
5-11: Build Storybook before tests to avoid 404s on ./storybook-staticEnsure a static build exists prior to
serve/test-storybook. Easiest: add lifecycle hooks so CI/local runs don’t rely on manual ordering.Apply outside-range additions:
{ "scripts": { "pretest": "npm run build", "preserve": "npm run build" } }Optionally make
testresilient without hooks:- "test": "../../node_modules/.bin/start-server-and-test serve http://127.0.0.1:6006 '../../node_modules/.bin/test-storybook --url http://127.0.0.1:6006'", + "test": "../../node_modules/.bin/start-server-and-test \"npm run build && npm run serve\" http://127.0.0.1:6006 '../../node_modules/.bin/test-storybook --url http://127.0.0.1:6006'",
5-11: Avoid hardcoded ../../node_modules/.bin paths; rely on script bin resolutionThese relative paths are brittle across workspace depth and can bypass the package’s declared versions. Script PATH already resolves local bins.
Apply:
- "dev": "../../node_modules/.bin/storybook dev -p 6006", - "build": "../../node_modules/.bin/storybook build", - "build-storybook": "../../node_modules/.bin/storybook build", - "storybook": "../../node_modules/.bin/storybook dev -p 6006", - "serve": "../../node_modules/.bin/http-server ./storybook-static -p 6006 -s", - "test": "../../node_modules/.bin/start-server-and-test serve http://127.0.0.1:6006 '../../node_modules/.bin/test-storybook --url http://127.0.0.1:6006'", - "test:local": "../../node_modules/.bin/test-storybook", + "dev": "storybook dev -p 6006", + "build": "storybook build", + "build-storybook": "storybook build", + "storybook": "storybook dev -p 6006", + "serve": "http-server ./storybook-static -p 6006 -s", + "test": "start-server-and-test serve http://127.0.0.1:6006 'test-storybook --url http://127.0.0.1:6006'", + "test:local": "test-storybook",
5-11: Install Playwright browsers before interaction testsStorybook’s test runner requires browsers/system deps. Add a prepare step so local and CI runs don’t flake on missing browsers.
Apply outside-range additions:
{ "scripts": { "test:prepare": "playwright install --with-deps || playwright install" }, "devDependencies": { "playwright": "^1.48.0" } }If CI already installs browsers, ignore adding the script and keep it in pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
apps/docs/package.json(1 hunks)apps/docs/src/ui/select-alignment.stories.tsx(1 hunks)packages/components/src/ui/select.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/src/ui/select.tsx
- apps/docs/src/ui/select-alignment.stories.tsx
🧰 Additional context used
📓 Path-based instructions (4)
{package.json,packages/**/package.json,apps/**/package.json}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
Use consistent versioning across packages
Files:
apps/docs/package.json
apps/**/package.json
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
Use workspace version for internal dependencies in apps (e.g., "@lambdacurry/forms": "*")
Files:
apps/docs/package.json
{packages/**/package.json,apps/**/package.json}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
Define peerDependencies, dependencies, and devDependencies appropriately in package.json
Files:
apps/docs/package.json
apps/docs/**
📄 CodeRabbit inference engine (.cursor/rules/storybook-testing.mdc)
apps/docs/**: Build Storybook static files before running Playwright tests locally.
Install Playwright browsers and system dependencies before running interaction tests.Storybook docs, examples, and UI tests live in apps/docs
Files:
apps/docs/package.json
| "dev": "../../node_modules/.bin/storybook dev -p 6006", | ||
| "build": "../../node_modules/.bin/storybook build", | ||
| "build-storybook": "../../node_modules/.bin/storybook build", | ||
| "storybook": "../../node_modules/.bin/storybook dev -p 6006", | ||
| "serve": "../../node_modules/.bin/http-server ./storybook-static -p 6006 -s", | ||
| "test": "../../node_modules/.bin/start-server-and-test serve http://127.0.0.1:6006 '../../node_modules/.bin/test-storybook --url http://127.0.0.1:6006'", | ||
| "test:local": "../../node_modules/.bin/test-storybook", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Storybook 9 runner/test package compatibility
You’re on storybook@^9.0.6 but runner libs may need matching major ranges. Please confirm recommended versions.
🏁 Script executed:
#!/bin/bash
# Show local versions for alignment
jq -r '.dependencies.storybook, .devDependencies["@storybook/test-runner"], .devDependencies["@storybook/test"]' apps/docs/package.json
# (Optional) check other workspaces for pinned Storybook packages
fd package.json -a apps packages | xargs -I{} jq -r '"FILE:{}\n" + (try .dependencies.storybook // "none")'Length of output: 22
🌐 Web query:
What versions of @storybook/test-runner and @storybook/test are recommended for Storybook 9.0.x?
💡 Result:
Short answer: don't use @storybook/test-runner or @storybook/test with Storybook 9.x. Storybook 9 consolidates @storybook/test into storybook/test and replaces the old test-runner with the new addon-vitest test experience — migrate to addon-vitest (storybook/test for APIs) per the Storybook 9 migration docs. [1][2]
Sources:
- Storybook 9 migration guide (packages consolidated; test renamed to addon-vitest). [1]
- Test runner docs / compatibility table (shows compatibility up to Storybook 8 and recommends migrating to addon-vitest). [2]
Migrate Storybook test packages for Storybook 9
apps/docs/package.json currently pins storybook ^9.0.6 but still lists @storybook/test @8.6.14 and @storybook/test-runner @0.22.1 — Storybook 9 consolidates/renames the test packages and replaces the old test-runner. Remove those devDependencies and migrate tests to the Storybook 9 testing setup (install addon-vitest and use the new storybook/test APIs per the Storybook 9 migration guide).
🤖 Prompt for AI Agents
In apps/docs/package.json around lines 5 to 11, the project still lists legacy
@storybook/test and @storybook/test-runner packages for Storybook 8 while
package.json pins Storybook ^9.0.6; remove the old devDependencies
(@storybook/test and @storybook/test-runner) and replace them with the Storybook
9 testing setup by installing @storybook/addon-vitest (or addon-vitest
equivalent from the migration guide) and updating test scripts to use the new
storybook/test APIs (adjust test: and test:local scripts to invoke the Storybook
9 test runner via addon-vitest or vitest as documented), then run npm/yarn
install and verify tests run under the new Storybook 9 testing API.
|
Requested by Jake Ruesink. High-level review + why tests are failing here vs the sibling PR: Summary
Key issues causing the failing test
Minor note on Select itself
Recommended path
Portal testing rule of thumb (to apply across similar stories)
Happy to push a quick fix here or consolidate with #155—just say the word. |
Requester: Jake Ruesink
Summary
Key Changes
Notes
Compliance
Refs
💻 View my work • 👤 Initiated by @jaruesink • About Codegen
⛔ Remove Codegen from PR • 🚫 Ban action checks
Summary by CodeRabbit
New Features
Accessibility
Documentation
Tests
Chores