feat(media): init - #188
Open
coryrylan wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
scope(core)
scope(ci)
scope(internals)
scope(lint)
scope(docs)
dependencies
📝 WalkthroughWalkthroughAdds a media controller and seven media controls with shared state, commands, form integration, accessibility, SSR, visual, and Lighthouse tests. Updates package exports, build/test wiring, metadata generation, lint validation, playground imports, documentation navigation, and media component documentation. ChangesMedia contracts and controller
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
There was a problem hiding this comment.
Pull request overview
This PR initializes the new @nvidia-elements/media package and wires it into the Elements monorepo ecosystem (docs site, tooling, linting, metadata), adding a media controller plus a first set of media control components.
Changes:
- Add
@nvidia-elements/mediacomponents (controller, playback controls) with unit/axe/visual/SSR/lighthouse coverage and baselines. - Add documentation pages + docs navigation entries for the new media components.
- Integrate the new package into monorepo tooling (Wireit, metadata aggregation, playground import map, lint rule allowlists) and adjust lighthouse size thresholds.
Reviewed changes
Copilot reviewed 167 out of 170 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| knip.config.js | Include example entrypoints in Knip analysis to account for new examples. |
| pnpm-lock.yaml | Add workspace dependency link for @nvidia-elements/forms under the media importer. |
| projects/core/src/accordion/accordion.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/alert/alert.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/combobox/combobox.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/color/color.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/copy-button/copy-button.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/datetime/datetime.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/dialog/dialog.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/drawer/drawer.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/dropdown/dropdown.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/dropdown-group/dropdown-group.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/dropzone/dropzone.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/index.test.lighthouse.ts | Adjust bundle size threshold for lighthouse baseline. |
| projects/core/src/internal/controllers/i18n.controller.test.ts | Update expected i18n registry shape with new media strings. |
| projects/core/src/internal/services/i18n.service.test.ts | Update expected i18n defaults and update behavior assertions. |
| projects/core/src/internal/services/i18n.service.ts | Add new default i18n strings used by media components. |
| projects/core/src/month/month.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/notification/notification.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/pagination/pagination.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/panel/panel.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/password/password.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/preferences-input/preferences-input.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/resize-handle/resize-handle.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/search/search.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/select/select.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/sort-button/sort-button.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/steps/steps.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/tag/tag.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/time/time.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/toast/toast.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/toggletip/toggletip.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/tree/tree.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/core/src/week/week.test.lighthouse.ts | Adjust JS size threshold for lighthouse baseline. |
| projects/internals/metadata/package.json | Include media in metadata build inputs/coverage aggregation and tasks. |
| projects/internals/metadata/src/services/api.service.test.ts | Add test ensuring exact-match API search ordering. |
| projects/internals/metadata/src/services/projects.service.test.ts | Assert ProjectsService caching behavior. |
| projects/internals/metadata/src/services/releases.service.test.ts | Assert ReleasesService caching behavior. |
| projects/internals/metadata/src/tasks/api.utils.test.ts | Add assertions for projected mixin API attributes on media elements. |
| projects/internals/metadata/src/tasks/api.utils.ts | Add media package to API aggregation inputs. |
| projects/internals/metadata/static/adoption.json | Update published metadata snapshot (LFS pointer). |
| projects/internals/metadata/static/lighthouse.json | Update published metadata snapshot (LFS pointer). |
| projects/internals/metadata/static/releases.json | Update published metadata snapshot (LFS pointer). |
| projects/internals/metadata/static/tests.json | Update published metadata snapshot (LFS pointer). |
| projects/internals/tools/src/api/service.test.ts | Extend API service tests for mixin attribute projection and naming. |
| projects/internals/tools/src/playground/utils.ts | Add @nvidia-elements/media to the playground import map. |
| projects/lint/src/eslint/rules/no-invalid-invoker-triggers.test.ts | Update rule tests to include media invoker elements. |
| projects/lint/src/eslint/rules/no-invalid-invoker-triggers.ts | Allow media components as valid invoker-trigger elements. |
| projects/media/.visual/media-controller.dark.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-controller.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-fullscreen-button.dark.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-fullscreen-button.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-mute-button.dark.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-mute-button.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-pause-button.dark.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-pause-button.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-playback-rate-select.dark.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-playback-rate-select.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-seek-button.dark.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-seek-button.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-time-range.dark.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-time-range.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-volume-range.dark.png | Add visual baseline asset (LFS pointer). |
| projects/media/.visual/media-volume-range.png | Add visual baseline asset (LFS pointer). |
| projects/media/package.json | Add exports for media components and wire up Wireit tasks (SSR/visual), plus dependency updates. |
| projects/media/tsconfig.lib.json | Exclude *.test.visual.ts from library compilation. |
| projects/media/vite.config.ts | Add local aliasing for @nvidia-elements/media during build/dev. |
| projects/media/vitest.ssr.ts | Add SSR test config with dist aliasing and junit output. |
| projects/media/vitest.visual.html | Add visual test HTML harness for the media package. |
| projects/media/vitest.visual.ts | Add visual test config + junit output for media package. |
| projects/media/src/controller/controller.css | Add controller layout styles for slotted media + controls. |
| projects/media/src/controller/controller.examples.ts | Add controller examples (default composition, commands, form values, card). |
| projects/media/src/controller/controller.test.axe.ts | Add axe coverage for controller compositions. |
| projects/media/src/controller/controller.test.lighthouse.ts | Add lighthouse baseline for controller. |
| projects/media/src/controller/controller.test.ssr.ts | Add SSR baseline test for controller. |
| projects/media/src/controller/controller.test.ts | Add unit tests for controller command handling and state syncing. |
| projects/media/src/controller/controller.test.visual.ts | Add visual regression coverage for controller. |
| projects/media/src/controller/controller.ts | Implement media controller command handling + state projection. |
| projects/media/src/controller/define.ts | Register nve-media-controller. |
| projects/media/src/controller/index.ts | Export controller + media state event types. |
| projects/media/src/declarations.d.ts | Add CSS module typings for the media project. |
| projects/media/src/fullscreen-button/fullscreen-button.css | Add fullscreen button stylesheet header. |
| projects/media/src/fullscreen-button/fullscreen-button.examples.ts | Add fullscreen button example. |
| projects/media/src/fullscreen-button/fullscreen-button.test.axe.ts | Add axe coverage for fullscreen button. |
| projects/media/src/fullscreen-button/fullscreen-button.test.lighthouse.ts | Add lighthouse coverage for fullscreen button. |
| projects/media/src/fullscreen-button/fullscreen-button.test.ssr.ts | Add SSR baseline for fullscreen button. |
| projects/media/src/fullscreen-button/fullscreen-button.test.ts | Add unit tests for fullscreen button behavior. |
| projects/media/src/fullscreen-button/fullscreen-button.test.visual.ts | Add visual coverage for fullscreen button. |
| projects/media/src/fullscreen-button/fullscreen-button.ts | Implement fullscreen command button synced to controller state. |
| projects/media/src/fullscreen-button/define.ts | Register nve-media-fullscreen-button. |
| projects/media/src/fullscreen-button/index.ts | Export fullscreen button. |
| projects/media/src/internal/button-form-control-usage.test.ts | Add shared tests validating ButtonFormControlMixin behavior in media buttons. |
| projects/media/src/internal/command-target.ts | Implement commandfor / commandForElement target resolution. |
| projects/media/src/internal/controllers/media-state.controller.test.ts | Add unit tests for MediaStateController retargeting + sync behavior. |
| projects/media/src/internal/controllers/media-state.controller.ts | Add reactive controller to sync host from media-state-change events. |
| projects/media/src/internal/media-button.css | Add shared button styles for media controls. |
| projects/media/src/internal/media-command.ts | Add media command constants and types. |
| projects/media/src/internal/media-range.css | Add shared slider styling for time/volume ranges. |
| projects/media/src/internal/media-state.ts | Add MediaState model + helpers (validation, equality, event constant). |
| projects/media/src/mute-button/mute-button.css | Add mute button stylesheet header. |
| projects/media/src/mute-button/mute-button.examples.ts | Add mute button example. |
| projects/media/src/mute-button/mute-button.test.axe.ts | Add axe coverage for mute button. |
| projects/media/src/mute-button/mute-button.test.lighthouse.ts | Add lighthouse coverage for mute button. |
| projects/media/src/mute-button/mute-button.test.ssr.ts | Add SSR baseline for mute button. |
| projects/media/src/mute-button/mute-button.test.ts | Add unit tests for mute button state + commands + forms. |
| projects/media/src/mute-button/mute-button.test.visual.ts | Add visual coverage for mute button. |
| projects/media/src/mute-button/mute-button.ts | Implement mute toggle button synced to controller muted state. |
| projects/media/src/mute-button/define.ts | Register nve-media-mute-button. |
| projects/media/src/mute-button/index.ts | Export mute button. |
| projects/media/src/pause-button/pause-button.css | Add pause button stylesheet header. |
| projects/media/src/pause-button/pause-button.examples.ts | Add pause button example. |
| projects/media/src/pause-button/pause-button.test.axe.ts | Add axe coverage for pause button. |
| projects/media/src/pause-button/pause-button.test.lighthouse.ts | Add lighthouse coverage for pause button. |
| projects/media/src/pause-button/pause-button.test.ssr.ts | Add SSR baseline for pause button. |
| projects/media/src/pause-button/pause-button.test.ts | Add unit tests for pause button state + commands + forms. |
| projects/media/src/pause-button/pause-button.test.visual.ts | Add visual coverage for pause button. |
| projects/media/src/pause-button/pause-button.ts | Implement pause toggle button synced to paused/ended state. |
| projects/media/src/pause-button/define.ts | Register nve-media-pause-button. |
| projects/media/src/pause-button/index.ts | Export pause button. |
| projects/media/src/playback-rate-select/playback-rate-select.css | Add select styles for playback-rate select. |
| projects/media/src/playback-rate-select/playback-rate-select.examples.ts | Add playback rate select examples (default + disabled). |
| projects/media/src/playback-rate-select/playback-rate-select.test.axe.ts | Add axe coverage for playback rate select. |
| projects/media/src/playback-rate-select/playback-rate-select.test.lighthouse.ts | Add lighthouse coverage for playback rate select. |
| projects/media/src/playback-rate-select/playback-rate-select.test.ssr.ts | Add SSR baseline for playback rate select. |
| projects/media/src/playback-rate-select/playback-rate-select.test.ts | Add unit tests for playback rate select API + sync + forms. |
| projects/media/src/playback-rate-select/playback-rate-select.test.visual.ts | Add visual coverage for playback rate select. |
| projects/media/src/playback-rate-select/playback-rate-select.ts | Implement playback rate select synced to controller rate. |
| projects/media/src/playback-rate-select/define.ts | Register nve-media-playback-rate-select. |
| projects/media/src/playback-rate-select/index.ts | Export playback rate select. |
| projects/media/src/seek-button/seek-button.css | Add seek button stylesheet header. |
| projects/media/src/seek-button/seek-button.examples.ts | Add seek button toolbar example. |
| projects/media/src/seek-button/seek-button.test.axe.ts | Add axe coverage for seek button. |
| projects/media/src/seek-button/seek-button.test.lighthouse.ts | Add lighthouse coverage for seek button. |
| projects/media/src/seek-button/seek-button.test.ssr.ts | Add SSR baseline for seek button. |
| projects/media/src/seek-button/seek-button.test.ts | Add unit tests for derived command behavior + labels/icons. |
| projects/media/src/seek-button/seek-button.test.visual.ts | Add visual coverage for seek button. |
| projects/media/src/seek-button/seek-button.ts | Implement seek button with derived commands and i18n-driven labels. |
| projects/media/src/seek-button/define.ts | Register nve-media-seek-button. |
| projects/media/src/seek-button/index.ts | Export seek button. |
| projects/media/src/time-range/define.ts | Register nve-media-time-range. |
| projects/media/src/time-range/index.ts | Export time range. |
| projects/media/src/time-range/time-range.css | Add time range stylesheet header. |
| projects/media/src/time-range/time-range.examples.ts | Add time range example. |
| projects/media/src/time-range/time-range.test.axe.ts | Add axe coverage for time range. |
| projects/media/src/time-range/time-range.test.lighthouse.ts | Add lighthouse coverage for time range. |
| projects/media/src/time-range/time-range.test.ssr.ts | Add SSR baseline for time range. |
| projects/media/src/time-range/time-range.test.ts | Add unit tests for time range sync/commands/forms. |
| projects/media/src/time-range/time-range.test.visual.ts | Add visual coverage for time range. |
| projects/media/src/time-range/time-range.ts | Implement time scrubbing range synced to controller time/duration. |
| projects/media/src/volume-range/define.ts | Register nve-media-volume-range. |
| projects/media/src/volume-range/index.ts | Export volume range. |
| projects/media/src/volume-range/volume-range.css | Add volume range stylesheet header. |
| projects/media/src/volume-range/volume-range.examples.ts | Add volume range example. |
| projects/media/src/volume-range/volume-range.test.axe.ts | Add axe coverage for volume range. |
| projects/media/src/volume-range/volume-range.test.lighthouse.ts | Add lighthouse coverage for volume range. |
| projects/media/src/volume-range/volume-range.test.ssr.ts | Add SSR baseline for volume range. |
| projects/media/src/volume-range/volume-range.test.ts | Add unit tests for volume range sync/commands/forms. |
| projects/media/src/volume-range/volume-range.test.visual.ts | Add visual coverage for volume range. |
| projects/media/src/volume-range/volume-range.ts | Implement volume range synced to controller volume. |
| projects/site/src/_11ty/layouts/common.js | Add Media section and entries to docs navigation tree. |
| projects/site/src/docs/media/controller.md | Add docs page for media controller. |
| projects/site/src/docs/media/fullscreen-button.md | Add docs page for fullscreen button. |
| projects/site/src/docs/media/mute-button.md | Add docs page for mute button. |
| projects/site/src/docs/media/pause-button.md | Add docs page for pause button. |
| projects/site/src/docs/media/playback-rate-select.md | Add docs page for playback rate select. |
| projects/site/src/docs/media/seek-button.md | Add docs page for seek button. |
| projects/site/src/docs/media/time-range.md | Add docs page for time range. |
| projects/site/src/docs/media/volume-range.md | Add docs page for volume range. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@projects/internals/metadata/src/services/api.service.test.ts`:
- Around line 43-47: Update the test “should prioritize exact matches over fuzzy
matches” to control the search results so a fuzzy match appears before the exact
“nve-button” match, then assert that exact-match promotion moves “nve-button” to
index 0. Ensure the test would fail if the exactMatchIndex > 0 promotion logic
regresses.
In `@projects/media/src/controller/controller.test.axe.ts`:
- Line 30: Wrap the test body that mounts the fixture and calls elementIsStable,
runAxe, and the assertion in a try/finally block, moving removeFixture(fixture)
into finally. Preserve the existing test behavior while guaranteeing fixture
cleanup when any step fails.
In `@projects/media/src/internal/media-button.css`:
- Line 23: Resolve the Stylelint violations in
projects/media/src/internal/media-button.css at lines 23-23, 171-171, and
225-225, and projects/media/src/internal/media-range.css at line 17-17: add the
required blank lines before display and pointer-events declarations, and change
currentColor to lowercase currentcolor.
- Around line 54-57: Update the readonly cursor custom property in both readonly
blocks to use a valid CSS cursor keyword, replacing `cursor` with `default` or
the intended keyword while preserving the existing text-decoration behavior.
In `@projects/media/src/playback-rate-select/playback-rate-select.css`:
- Line 16: In projects/media/src/playback-rate-select/playback-rate-select.css,
add a blank line before the standard declarations display: block; at lines 16-16
and background: var(--background); at lines 31-31, separating them from the
preceding custom-property declarations to satisfy Stylelint.
- Around line 41-45: In the playback-rate select style rule, move the font:
inherit declaration before line-height: var(--height) !important so the font
shorthand does not override the later line-height declaration. Preserve the
existing values and ordering of the other declarations.
In `@projects/media/src/playback-rate-select/playback-rate-select.ts`:
- Around line 121-129: Update `#handleMediaState` and the option-rendering flow to
ensure a valid positive state.playbackRate such as 1.25 is included in the
rendered rates when it is absent from rates, so the select displays the active
value. Preserve existing default-rate rendering and avoid adding invalid or
non-finite values.
In `@projects/media/src/time-range/time-range.ts`:
- Around line 49-53: Extract the shared command properties contract used by
time-range, volume-range, and playback-rate-select into a reusable mixin or base
contract, including command, commandfor, and commandForElement with their
existing types and defaults. Update each control to consume that shared
contract, while keeping internal/command-target.ts focused solely on target
resolution.
In `@projects/media/src/volume-range/volume-range.ts`:
- Around line 63-65: Update the volume-range media-state handling so the
mediaDisabled state is synchronized from the media state’s relevant disabled
capability, rather than remaining false after initialization. Extend
`#handleMediaState` alongside its existing volume synchronization, ensuring the
disabled input and null form-value paths react to target state while preserving
current volume behavior; add coverage for the disabled-state transition.
In `@projects/site/src/docs/api-design/media.md`:
- Around line 88-103: Keep the playback-rate contract consistent in both
documented locations: update projects/site/src/docs/api-design/media.md lines
88-103 and projects/site/src/docs/media/controller.md lines 32-45 to either
document playbackRate in mediaState and the reflected playback-rate attribute at
both sites, or explicitly remove both from the controller API documentation at
both sites.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a4feeb98-2a99-409c-a5d7-aea26ea0f502
⛔ Files ignored due to path filters (18)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/media/.visual/media-controller.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-controller.pngis excluded by!**/*.pngprojects/media/.visual/media-fullscreen-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-fullscreen-button.pngis excluded by!**/*.pngprojects/media/.visual/media-mute-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-mute-button.pngis excluded by!**/*.pngprojects/media/.visual/media-pause-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-pause-button.pngis excluded by!**/*.pngprojects/media/.visual/media-playback-rate-select.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-playback-rate-select.pngis excluded by!**/*.pngprojects/media/.visual/media-seek-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-seek-button.pngis excluded by!**/*.pngprojects/media/.visual/media-time-range.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-time-range.pngis excluded by!**/*.pngprojects/media/.visual/media-volume-range.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-volume-range.pngis excluded by!**/*.pngprojects/site/public/static/video/particle.mp4is excluded by!**/*.mp4
📒 Files selected for processing (152)
knip.config.jsprojects/core/src/accordion/accordion.test.lighthouse.tsprojects/core/src/alert/alert.test.lighthouse.tsprojects/core/src/color/color.test.lighthouse.tsprojects/core/src/combobox/combobox.test.lighthouse.tsprojects/core/src/copy-button/copy-button.test.lighthouse.tsprojects/core/src/datetime/datetime.test.lighthouse.tsprojects/core/src/dialog/dialog.test.lighthouse.tsprojects/core/src/drawer/drawer.test.lighthouse.tsprojects/core/src/dropdown-group/dropdown-group.test.lighthouse.tsprojects/core/src/dropdown/dropdown.test.lighthouse.tsprojects/core/src/dropzone/dropzone.test.lighthouse.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/internal/controllers/i18n.controller.test.tsprojects/core/src/internal/services/i18n.service.test.tsprojects/core/src/internal/services/i18n.service.tsprojects/core/src/month/month.test.lighthouse.tsprojects/core/src/notification/notification.test.lighthouse.tsprojects/core/src/pagination/pagination.test.lighthouse.tsprojects/core/src/panel/panel.test.lighthouse.tsprojects/core/src/password/password.test.lighthouse.tsprojects/core/src/preferences-input/preferences-input.test.lighthouse.tsprojects/core/src/resize-handle/resize-handle.test.lighthouse.tsprojects/core/src/search/search.test.lighthouse.tsprojects/core/src/select/select.test.lighthouse.tsprojects/core/src/sort-button/sort-button.test.lighthouse.tsprojects/core/src/steps/steps.test.lighthouse.tsprojects/core/src/tag/tag.test.lighthouse.tsprojects/core/src/time/time.test.lighthouse.tsprojects/core/src/toast/toast.test.lighthouse.tsprojects/core/src/toggletip/toggletip.test.lighthouse.tsprojects/core/src/tree/tree.test.lighthouse.tsprojects/core/src/week/week.test.lighthouse.tsprojects/internals/metadata/package.jsonprojects/internals/metadata/src/services/api.service.test.tsprojects/internals/metadata/src/services/projects.service.test.tsprojects/internals/metadata/src/services/releases.service.test.tsprojects/internals/metadata/src/tasks/api.utils.test.tsprojects/internals/metadata/src/tasks/api.utils.tsprojects/internals/metadata/static/adoption.jsonprojects/internals/metadata/static/lighthouse.jsonprojects/internals/metadata/static/releases.jsonprojects/internals/metadata/static/tests.jsonprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/playground/utils.tsprojects/lint/src/eslint/rules/no-invalid-invoker-triggers.test.tsprojects/lint/src/eslint/rules/no-invalid-invoker-triggers.tsprojects/media/package.jsonprojects/media/src/controller/controller.cssprojects/media/src/controller/controller.examples.tsprojects/media/src/controller/controller.test.axe.tsprojects/media/src/controller/controller.test.lighthouse.tsprojects/media/src/controller/controller.test.ssr.tsprojects/media/src/controller/controller.test.tsprojects/media/src/controller/controller.test.visual.tsprojects/media/src/controller/controller.tsprojects/media/src/controller/define.tsprojects/media/src/controller/index.tsprojects/media/src/declarations.d.tsprojects/media/src/fullscreen-button/define.tsprojects/media/src/fullscreen-button/fullscreen-button.cssprojects/media/src/fullscreen-button/fullscreen-button.examples.tsprojects/media/src/fullscreen-button/fullscreen-button.test.axe.tsprojects/media/src/fullscreen-button/fullscreen-button.test.lighthouse.tsprojects/media/src/fullscreen-button/fullscreen-button.test.ssr.tsprojects/media/src/fullscreen-button/fullscreen-button.test.tsprojects/media/src/fullscreen-button/fullscreen-button.test.visual.tsprojects/media/src/fullscreen-button/fullscreen-button.tsprojects/media/src/fullscreen-button/index.tsprojects/media/src/internal/button-form-control-usage.test.tsprojects/media/src/internal/command-target.tsprojects/media/src/internal/controllers/media-state.controller.test.tsprojects/media/src/internal/controllers/media-state.controller.tsprojects/media/src/internal/media-button.cssprojects/media/src/internal/media-command.tsprojects/media/src/internal/media-range.cssprojects/media/src/internal/media-state.tsprojects/media/src/mute-button/define.tsprojects/media/src/mute-button/index.tsprojects/media/src/mute-button/mute-button.cssprojects/media/src/mute-button/mute-button.examples.tsprojects/media/src/mute-button/mute-button.test.axe.tsprojects/media/src/mute-button/mute-button.test.lighthouse.tsprojects/media/src/mute-button/mute-button.test.ssr.tsprojects/media/src/mute-button/mute-button.test.tsprojects/media/src/mute-button/mute-button.test.visual.tsprojects/media/src/mute-button/mute-button.tsprojects/media/src/pause-button/define.tsprojects/media/src/pause-button/index.tsprojects/media/src/pause-button/pause-button.cssprojects/media/src/pause-button/pause-button.examples.tsprojects/media/src/pause-button/pause-button.test.axe.tsprojects/media/src/pause-button/pause-button.test.lighthouse.tsprojects/media/src/pause-button/pause-button.test.ssr.tsprojects/media/src/pause-button/pause-button.test.tsprojects/media/src/pause-button/pause-button.test.visual.tsprojects/media/src/pause-button/pause-button.tsprojects/media/src/playback-rate-select/define.tsprojects/media/src/playback-rate-select/index.tsprojects/media/src/playback-rate-select/playback-rate-select.cssprojects/media/src/playback-rate-select/playback-rate-select.examples.tsprojects/media/src/playback-rate-select/playback-rate-select.test.axe.tsprojects/media/src/playback-rate-select/playback-rate-select.test.lighthouse.tsprojects/media/src/playback-rate-select/playback-rate-select.test.ssr.tsprojects/media/src/playback-rate-select/playback-rate-select.test.tsprojects/media/src/playback-rate-select/playback-rate-select.test.visual.tsprojects/media/src/playback-rate-select/playback-rate-select.tsprojects/media/src/seek-button/define.tsprojects/media/src/seek-button/index.tsprojects/media/src/seek-button/seek-button.cssprojects/media/src/seek-button/seek-button.examples.tsprojects/media/src/seek-button/seek-button.test.axe.tsprojects/media/src/seek-button/seek-button.test.lighthouse.tsprojects/media/src/seek-button/seek-button.test.ssr.tsprojects/media/src/seek-button/seek-button.test.tsprojects/media/src/seek-button/seek-button.test.visual.tsprojects/media/src/seek-button/seek-button.tsprojects/media/src/time-range/define.tsprojects/media/src/time-range/index.tsprojects/media/src/time-range/time-range.cssprojects/media/src/time-range/time-range.examples.tsprojects/media/src/time-range/time-range.test.axe.tsprojects/media/src/time-range/time-range.test.lighthouse.tsprojects/media/src/time-range/time-range.test.ssr.tsprojects/media/src/time-range/time-range.test.tsprojects/media/src/time-range/time-range.test.visual.tsprojects/media/src/time-range/time-range.tsprojects/media/src/volume-range/define.tsprojects/media/src/volume-range/index.tsprojects/media/src/volume-range/volume-range.cssprojects/media/src/volume-range/volume-range.examples.tsprojects/media/src/volume-range/volume-range.test.axe.tsprojects/media/src/volume-range/volume-range.test.lighthouse.tsprojects/media/src/volume-range/volume-range.test.ssr.tsprojects/media/src/volume-range/volume-range.test.tsprojects/media/src/volume-range/volume-range.test.visual.tsprojects/media/src/volume-range/volume-range.tsprojects/media/tsconfig.lib.jsonprojects/media/vite.config.tsprojects/media/vitest.ssr.tsprojects/media/vitest.visual.htmlprojects/media/vitest.visual.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/api-design/media.mdprojects/site/src/docs/media/controller.mdprojects/site/src/docs/media/fullscreen-button.mdprojects/site/src/docs/media/mute-button.mdprojects/site/src/docs/media/pause-button.mdprojects/site/src/docs/media/playback-rate-select.mdprojects/site/src/docs/media/seek-button.mdprojects/site/src/docs/media/time-range.mdprojects/site/src/docs/media/volume-range.md
Merged
coryrylan
force-pushed
the
topic-media-feat
branch
from
July 22, 2026 21:52
2d1a9a5 to
6a3cf4c
Compare
coryrylan
marked this pull request as draft
July 22, 2026 21:59
coryrylan
force-pushed
the
topic-media-feat
branch
from
July 22, 2026 22:03
6a3cf4c to
f37dc62
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
projects/site/src/docs/api-design/media.md (1)
92-103: 🗄️ Data Integrity & Integration | 🟡 MinorThe state table still omits
playback-rate.
mediaStateandprojects/site/src/docs/media/controller.mddocument playback rate, but this reflected-attribute table does not. Add aplayback-raterow sourced frommedia.playbackRate; this is the same unresolved contract drift raised in the previous review.🤖 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 `@projects/site/src/docs/api-design/media.md` around lines 92 - 103, Add the missing playback-rate entry to the reflected-attribute table, using a number type and sourcing it from media.playbackRate. Keep the existing mediaState and event documentation unchanged.
🤖 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 `@projects/lint/src/eslint/rules/no-invalid-invoker-triggers.ts`:
- Around line 27-34: Update the invoker validation around BUTTON_TYPE_ELEMENTS
and INVOKER_ATTRIBUTES so the newly added media elements are allowed only for
commandfor, while popovertarget and interestfor retain their intended element
restrictions. Split the allowlist by invoker attribute or otherwise encode the
attribute-specific support matrix, and add or update coverage to verify each
attribute independently.
In `@projects/media/package.json`:
- Around line 325-345: Update the test:visual script configuration to track the
assets imported by vitest.visual.html: add tsconfig.json and the relevant
themes/styles dist CSS globs to files, and explicitly declare the themes and
styles build scripts as non-cascading dependencies alongside the existing
dependencies. Align this dependency and invalidation tracking with test:ssr
without changing the visual test command.
In `@projects/media/src/controller/controller.examples.ts`:
- Around line 93-100: Move the controller-form input listener out of the inline
module script in the example template and bind it through the Lit component or
example harness instead. Ensure the handler associated with the controller form
updates the preview <pre> with current FormData values so the inline nve-canvas
preview remains dynamic.
In `@projects/media/src/controller/controller.ts`:
- Around line 36-49: The controller’s command documentation omits the supported
backward and forward seek commands. Update the JSDoc command block near the
existing seek entries to add `@command` entries for --seek-backward and
--seek-forward, matching the registered mediaCommands.seekBackward and
mediaCommands.seekForward handlers in `#commandHandlers`.
In `@projects/media/src/fullscreen-button/fullscreen-button.ts`:
- Around line 87-91: Update `#syncPressedState` to reset this.pressed to false
when state is null, while preserving the existing state.fullscreen assignment
for non-null MediaState values.
In `@projects/media/src/internal/media-state.ts`:
- Around line 48-59: Update mediaStatesEqual to compare numeric fields using
NaN-safe equality, so identical NaN values such as an unknown duration are
treated as equal while preserving normal equality for other values. Apply this
to the relevant numeric MediaState properties, including currentTime, duration,
playbackRate, and volume.
In `@projects/media/src/pause-button/pause-button.test.ts`:
- Around line 126-167: Update the affected tests to wrap the console.warn spy
and form fixture lifecycle in try/finally blocks, ensuring warn.mockRestore()
and removeFixture(formFixture) run even when assertions or setup fail. Preserve
the existing test behavior and assertions.
In `@projects/media/src/seek-button/seek-button.test.ts`:
- Around line 86-97: Ensure fixture cleanup runs even when assertions fail: wrap
the commandFixture test flow in
projects/media/src/seek-button/seek-button.test.ts lines 86-97 with try/finally
and remove commandFixture in finally; apply the same pattern to formFixture in
projects/media/src/time-range/time-range.test.ts lines 141-153. Preserve the
existing test assertions and interaction flow.
In `@projects/media/src/seek-button/seek-button.ts`:
- Line 44: Validate the reflected action value before deriving the seek command
in the seek-button component. Update the action-handling logic around the action
property to normalize unsupported runtime strings to the safe default forward
action (or reject them consistently), ensuring command state, icon, and label
remain valid. Add coverage for an invalid action provided through the reflected
attribute path.
In `@projects/media/src/time-range/time-range.ts`:
- Around line 123-126: Update `#handleMediaState` to handle a null mediaState by
disabling the range and resetting its form value before returning. Ensure the
previous target’s enabled state and time value are cleared so the range cannot
display or submit stale data.
In `@projects/site/src/docs/api-design/media.md`:
- Around line 111-127: Update the media API proposal to document all
playback-rate surfaces: add --set-playback-rate using source.valueAsNumber to
the command table, list nve-media-playback-rate-select among form-associated
controls, and include its package directory plus an implementation-plan step.
Use the existing media-command.ts and playback-rate-select documentation as the
authoritative references.
In `@projects/site/src/docs/media/volume-range.md`:
- Around line 16-22: Remove the misleading value="0.8" attribute from the
nve-media-volume-range example, leaving the controller-driven mediaState
synchronization as the source of truth.
---
Duplicate comments:
In `@projects/site/src/docs/api-design/media.md`:
- Around line 92-103: Add the missing playback-rate entry to the
reflected-attribute table, using a number type and sourcing it from
media.playbackRate. Keep the existing mediaState and event documentation
unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 89f75859-dcd1-4343-b8b7-f8845377f1a9
⛔ Files ignored due to path filters (18)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/media/.visual/media-controller.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-controller.pngis excluded by!**/*.pngprojects/media/.visual/media-fullscreen-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-fullscreen-button.pngis excluded by!**/*.pngprojects/media/.visual/media-mute-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-mute-button.pngis excluded by!**/*.pngprojects/media/.visual/media-pause-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-pause-button.pngis excluded by!**/*.pngprojects/media/.visual/media-playback-rate-select.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-playback-rate-select.pngis excluded by!**/*.pngprojects/media/.visual/media-seek-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-seek-button.pngis excluded by!**/*.pngprojects/media/.visual/media-time-range.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-time-range.pngis excluded by!**/*.pngprojects/media/.visual/media-volume-range.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-volume-range.pngis excluded by!**/*.pngprojects/site/public/static/video/particle.mp4is excluded by!**/*.mp4
📒 Files selected for processing (146)
knip.config.jsprojects/core/src/accordion/accordion.test.lighthouse.tsprojects/core/src/alert/alert.test.lighthouse.tsprojects/core/src/color/color.test.lighthouse.tsprojects/core/src/combobox/combobox.test.lighthouse.tsprojects/core/src/copy-button/copy-button.test.lighthouse.tsprojects/core/src/datetime/datetime.test.lighthouse.tsprojects/core/src/dialog/dialog.test.lighthouse.tsprojects/core/src/drawer/drawer.test.lighthouse.tsprojects/core/src/dropdown-group/dropdown-group.test.lighthouse.tsprojects/core/src/dropdown/dropdown.test.lighthouse.tsprojects/core/src/dropzone/dropzone.test.lighthouse.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/month/month.test.lighthouse.tsprojects/core/src/notification/notification.test.lighthouse.tsprojects/core/src/pagination/pagination.test.lighthouse.tsprojects/core/src/panel/panel.test.lighthouse.tsprojects/core/src/password/password.test.lighthouse.tsprojects/core/src/preferences-input/preferences-input.test.lighthouse.tsprojects/core/src/resize-handle/resize-handle.test.lighthouse.tsprojects/core/src/search/search.test.lighthouse.tsprojects/core/src/select/select.test.lighthouse.tsprojects/core/src/sort-button/sort-button.test.lighthouse.tsprojects/core/src/steps/steps.test.lighthouse.tsprojects/core/src/tag/tag.test.lighthouse.tsprojects/core/src/time/time.test.lighthouse.tsprojects/core/src/toast/toast.test.lighthouse.tsprojects/core/src/toggletip/toggletip.test.lighthouse.tsprojects/core/src/tree/tree.test.lighthouse.tsprojects/core/src/week/week.test.lighthouse.tsprojects/internals/metadata/package.jsonprojects/internals/metadata/src/tasks/api.utils.test.tsprojects/internals/metadata/src/tasks/api.utils.tsprojects/internals/metadata/static/adoption.jsonprojects/internals/metadata/static/lighthouse.jsonprojects/internals/metadata/static/releases.jsonprojects/internals/metadata/static/tests.jsonprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/playground/utils.tsprojects/lint/src/eslint/rules/no-invalid-invoker-triggers.test.tsprojects/lint/src/eslint/rules/no-invalid-invoker-triggers.tsprojects/media/package.jsonprojects/media/src/controller/controller.cssprojects/media/src/controller/controller.examples.tsprojects/media/src/controller/controller.test.axe.tsprojects/media/src/controller/controller.test.lighthouse.tsprojects/media/src/controller/controller.test.ssr.tsprojects/media/src/controller/controller.test.tsprojects/media/src/controller/controller.test.visual.tsprojects/media/src/controller/controller.tsprojects/media/src/controller/define.tsprojects/media/src/controller/index.tsprojects/media/src/declarations.d.tsprojects/media/src/fullscreen-button/define.tsprojects/media/src/fullscreen-button/fullscreen-button.cssprojects/media/src/fullscreen-button/fullscreen-button.examples.tsprojects/media/src/fullscreen-button/fullscreen-button.test.axe.tsprojects/media/src/fullscreen-button/fullscreen-button.test.lighthouse.tsprojects/media/src/fullscreen-button/fullscreen-button.test.ssr.tsprojects/media/src/fullscreen-button/fullscreen-button.test.tsprojects/media/src/fullscreen-button/fullscreen-button.test.visual.tsprojects/media/src/fullscreen-button/fullscreen-button.tsprojects/media/src/fullscreen-button/index.tsprojects/media/src/internal/button-form-control-usage.test.tsprojects/media/src/internal/command-target.tsprojects/media/src/internal/controllers/media-state.controller.test.tsprojects/media/src/internal/controllers/media-state.controller.tsprojects/media/src/internal/media-button.cssprojects/media/src/internal/media-command.tsprojects/media/src/internal/media-range.cssprojects/media/src/internal/media-state.tsprojects/media/src/mute-button/define.tsprojects/media/src/mute-button/index.tsprojects/media/src/mute-button/mute-button.cssprojects/media/src/mute-button/mute-button.examples.tsprojects/media/src/mute-button/mute-button.test.axe.tsprojects/media/src/mute-button/mute-button.test.lighthouse.tsprojects/media/src/mute-button/mute-button.test.ssr.tsprojects/media/src/mute-button/mute-button.test.tsprojects/media/src/mute-button/mute-button.test.visual.tsprojects/media/src/mute-button/mute-button.tsprojects/media/src/pause-button/define.tsprojects/media/src/pause-button/index.tsprojects/media/src/pause-button/pause-button.cssprojects/media/src/pause-button/pause-button.examples.tsprojects/media/src/pause-button/pause-button.test.axe.tsprojects/media/src/pause-button/pause-button.test.lighthouse.tsprojects/media/src/pause-button/pause-button.test.ssr.tsprojects/media/src/pause-button/pause-button.test.tsprojects/media/src/pause-button/pause-button.test.visual.tsprojects/media/src/pause-button/pause-button.tsprojects/media/src/playback-rate-select/define.tsprojects/media/src/playback-rate-select/index.tsprojects/media/src/playback-rate-select/playback-rate-select.cssprojects/media/src/playback-rate-select/playback-rate-select.examples.tsprojects/media/src/playback-rate-select/playback-rate-select.test.axe.tsprojects/media/src/playback-rate-select/playback-rate-select.test.lighthouse.tsprojects/media/src/playback-rate-select/playback-rate-select.test.ssr.tsprojects/media/src/playback-rate-select/playback-rate-select.test.tsprojects/media/src/playback-rate-select/playback-rate-select.test.visual.tsprojects/media/src/playback-rate-select/playback-rate-select.tsprojects/media/src/seek-button/define.tsprojects/media/src/seek-button/index.tsprojects/media/src/seek-button/seek-button.cssprojects/media/src/seek-button/seek-button.examples.tsprojects/media/src/seek-button/seek-button.test.axe.tsprojects/media/src/seek-button/seek-button.test.lighthouse.tsprojects/media/src/seek-button/seek-button.test.ssr.tsprojects/media/src/seek-button/seek-button.test.tsprojects/media/src/seek-button/seek-button.test.visual.tsprojects/media/src/seek-button/seek-button.tsprojects/media/src/time-range/define.tsprojects/media/src/time-range/index.tsprojects/media/src/time-range/time-range.cssprojects/media/src/time-range/time-range.examples.tsprojects/media/src/time-range/time-range.test.axe.tsprojects/media/src/time-range/time-range.test.lighthouse.tsprojects/media/src/time-range/time-range.test.ssr.tsprojects/media/src/time-range/time-range.test.tsprojects/media/src/time-range/time-range.test.visual.tsprojects/media/src/time-range/time-range.tsprojects/media/src/volume-range/define.tsprojects/media/src/volume-range/index.tsprojects/media/src/volume-range/volume-range.cssprojects/media/src/volume-range/volume-range.examples.tsprojects/media/src/volume-range/volume-range.test.axe.tsprojects/media/src/volume-range/volume-range.test.lighthouse.tsprojects/media/src/volume-range/volume-range.test.ssr.tsprojects/media/src/volume-range/volume-range.test.tsprojects/media/src/volume-range/volume-range.test.visual.tsprojects/media/src/volume-range/volume-range.tsprojects/media/tsconfig.lib.jsonprojects/media/vite.config.tsprojects/media/vitest.ssr.tsprojects/media/vitest.visual.htmlprojects/media/vitest.visual.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/api-design/media.mdprojects/site/src/docs/media/controller.mdprojects/site/src/docs/media/fullscreen-button.mdprojects/site/src/docs/media/mute-button.mdprojects/site/src/docs/media/pause-button.mdprojects/site/src/docs/media/playback-rate-select.mdprojects/site/src/docs/media/seek-button.mdprojects/site/src/docs/media/time-range.mdprojects/site/src/docs/media/volume-range.md
Comment on lines
+123
to
+126
| #handleMediaState = (mediaState: MediaState | null) => { | ||
| if (!mediaState) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset the range when its media target disappears.
Returning on null preserves the prior enabled slider and its stale form value. Disable and reset the range on this transition so it cannot submit or display the last target’s time.
Proposed fix
`#handleMediaState` = (mediaState: MediaState | null) => {
if (!mediaState) {
+ this.mediaDisabled = true;
+ this.min = 0;
+ this.max = 0;
+ this.valueAsNumber = 0;
return;
}📝 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.
Suggested change
| #handleMediaState = (mediaState: MediaState | null) => { | |
| if (!mediaState) { | |
| return; | |
| } | |
| `#handleMediaState` = (mediaState: MediaState | null) => { | |
| if (!mediaState) { | |
| this.mediaDisabled = true; | |
| this.min = 0; | |
| this.max = 0; | |
| this.valueAsNumber = 0; | |
| return; | |
| } |
🤖 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 `@projects/media/src/time-range/time-range.ts` around lines 123 - 126, Update
`#handleMediaState` to handle a null mediaState by disabling the range and
resetting its form value before returning. Ensure the previous target’s enabled
state and time value are cleared so the range cannot display or submit stale
data.
coryrylan
commented
Jul 22, 2026
Collaborator
Author
There was a problem hiding this comment.
todo: select should have inline style disable like icon button here
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 167 out of 170 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (3)
projects/media/src/internal/media-button.css:57
--cursor: cursor;is not a valid CSScursorvalue. Because[internal-host]appliescursor: var(--cursor), this will fall back to the browser default in unpredictable ways. Use a valid value such asdefaultfor readonly state.
projects/media/src/internal/media-button.css:150--cursor: cursor;is not a valid CSScursorvalue, so the inline readonly button state won’t reliably present the intended cursor. Use a valid value (e.g.default).
projects/media/src/playback-rate-select/playback-rate-select.ts:84- If
i18n.playbackRateOptionis missing/undefined (for example, a consumer provides a partial i18n object),formatI18n(...)returnsundefinedand the<option>label renders blank. Provide a fallback label (e.g. the rawrate).
coryrylan
force-pushed
the
topic-media-feat
branch
from
July 28, 2026 13:41
f37dc62 to
ed3f4a1
Compare
coryrylan
force-pushed
the
topic-media-feat
branch
from
July 28, 2026 20:27
ed3f4a1 to
3ad0af1
Compare
coryrylan
force-pushed
the
topic-media-feat
branch
from
July 28, 2026 21:08
3ad0af1 to
76ba16c
Compare
coryrylan
force-pushed
the
topic-media-feat
branch
from
July 28, 2026 21:41
76ba16c to
e6610ce
Compare
Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-media-feat
branch
from
July 28, 2026 21:44
e6610ce to
8730c65
Compare
coryrylan
marked this pull request as ready for review
July 28, 2026 22:48
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@projects/internals/metadata/src/services/api.service.test.ts`:
- Around line 45-50: Wrap the ApiService.search call in a try/finally block so
the MiniSearch.prototype spy created by searchSpy is always restored, including
when the search throws. Keep the existing search arguments and result assertions
unchanged, and move searchSpy.mockRestore() into the finally block.
In `@projects/media/src/controller/controller.test.ts`:
- Around line 358-386: Restore the original descriptors for
document.fullscreenElement and document.exitFullscreen after the “should handle
full-screen commands” test, including deleting properties that were not
originally present and reinstating captured descriptors when they were. Use a
finally block or dedicated cleanup so restoration runs even if an assertion
fails, without changing the test’s fullscreen behavior.
In `@projects/media/src/controller/controller.ts`:
- Line 66: Remove the static formAssociated flag from MediaController unless it
is intended to submit form data; if form participation is required, instead add
attachInternals() and setFormValue() plumbing and connect it to the MediaState
value.
In `@projects/media/src/declarations.d.ts`:
- Around line 9-12: Update the `*.css?inline` module declaration to expose
`content` through a default export instead of `export =`, matching the package’s
default-import usage and Vite’s inline CSS module shape.
In `@projects/media/src/internal/button-form-control-usage.test.ts`:
- Around line 77-84: Correct the property assignment in the test setup to use
the ButtonFormControlMixinInstance’s readOnly property, matching the existing
button.readOnly assertions and other usage. Keep the rest of the readonly
behavior test unchanged.
In
`@projects/media/src/playback-rate-select/playback-rate-select.test.lighthouse.ts`:
- Line 9: Update the report name passed to lighthouseRunner.getReport in the
playback-rate-select test to use the full element tag, matching sibling
lighthouse tests and the generated artifact directory naming convention.
In `@projects/media/vite.config.ts`:
- Line 2: Update the Vite import in the configuration to import UserConfig via a
type-only import, while keeping defineConfig and mergeConfig as regular runtime
imports.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 715e2861-665b-4895-9cdb-be7e63c470c4
⛔ Files ignored due to path filters (18)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/media/.visual/media-controller.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-controller.pngis excluded by!**/*.pngprojects/media/.visual/media-fullscreen-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-fullscreen-button.pngis excluded by!**/*.pngprojects/media/.visual/media-mute-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-mute-button.pngis excluded by!**/*.pngprojects/media/.visual/media-pause-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-pause-button.pngis excluded by!**/*.pngprojects/media/.visual/media-playback-rate-select.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-playback-rate-select.pngis excluded by!**/*.pngprojects/media/.visual/media-seek-button.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-seek-button.pngis excluded by!**/*.pngprojects/media/.visual/media-time-range.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-time-range.pngis excluded by!**/*.pngprojects/media/.visual/media-volume-range.dark.pngis excluded by!**/*.pngprojects/media/.visual/media-volume-range.pngis excluded by!**/*.pngprojects/site/public/static/video/particle.mp4is excluded by!**/*.mp4
📒 Files selected for processing (117)
knip.config.jsprojects/internals/metadata/package.jsonprojects/internals/metadata/src/services/api.service.test.tsprojects/internals/metadata/src/tasks/api.utils.test.tsprojects/internals/metadata/src/tasks/api.utils.tsprojects/internals/metadata/static/adoption.jsonprojects/internals/metadata/static/releases.jsonprojects/internals/metadata/static/tests.jsonprojects/internals/tools/src/api/service.test.tsprojects/internals/tools/src/playground/utils.tsprojects/lint/src/eslint/rules/no-invalid-invoker-triggers.test.tsprojects/lint/src/eslint/rules/no-invalid-invoker-triggers.tsprojects/media/package.jsonprojects/media/src/controller/controller.cssprojects/media/src/controller/controller.examples.tsprojects/media/src/controller/controller.test.axe.tsprojects/media/src/controller/controller.test.lighthouse.tsprojects/media/src/controller/controller.test.ssr.tsprojects/media/src/controller/controller.test.tsprojects/media/src/controller/controller.test.visual.tsprojects/media/src/controller/controller.tsprojects/media/src/controller/define.tsprojects/media/src/controller/index.tsprojects/media/src/declarations.d.tsprojects/media/src/fullscreen-button/define.tsprojects/media/src/fullscreen-button/fullscreen-button.cssprojects/media/src/fullscreen-button/fullscreen-button.examples.tsprojects/media/src/fullscreen-button/fullscreen-button.test.axe.tsprojects/media/src/fullscreen-button/fullscreen-button.test.lighthouse.tsprojects/media/src/fullscreen-button/fullscreen-button.test.ssr.tsprojects/media/src/fullscreen-button/fullscreen-button.test.tsprojects/media/src/fullscreen-button/fullscreen-button.test.visual.tsprojects/media/src/fullscreen-button/fullscreen-button.tsprojects/media/src/fullscreen-button/index.tsprojects/media/src/internal/button-form-control-usage.test.tsprojects/media/src/internal/command-target.tsprojects/media/src/internal/controllers/media-state.controller.test.tsprojects/media/src/internal/controllers/media-state.controller.tsprojects/media/src/internal/media-button.cssprojects/media/src/internal/media-command.tsprojects/media/src/internal/media-range.cssprojects/media/src/internal/media-state.tsprojects/media/src/mute-button/define.tsprojects/media/src/mute-button/index.tsprojects/media/src/mute-button/mute-button.cssprojects/media/src/mute-button/mute-button.examples.tsprojects/media/src/mute-button/mute-button.test.axe.tsprojects/media/src/mute-button/mute-button.test.lighthouse.tsprojects/media/src/mute-button/mute-button.test.ssr.tsprojects/media/src/mute-button/mute-button.test.tsprojects/media/src/mute-button/mute-button.test.visual.tsprojects/media/src/mute-button/mute-button.tsprojects/media/src/pause-button/define.tsprojects/media/src/pause-button/index.tsprojects/media/src/pause-button/pause-button.cssprojects/media/src/pause-button/pause-button.examples.tsprojects/media/src/pause-button/pause-button.test.axe.tsprojects/media/src/pause-button/pause-button.test.lighthouse.tsprojects/media/src/pause-button/pause-button.test.ssr.tsprojects/media/src/pause-button/pause-button.test.tsprojects/media/src/pause-button/pause-button.test.visual.tsprojects/media/src/pause-button/pause-button.tsprojects/media/src/playback-rate-select/define.tsprojects/media/src/playback-rate-select/index.tsprojects/media/src/playback-rate-select/playback-rate-select.cssprojects/media/src/playback-rate-select/playback-rate-select.examples.tsprojects/media/src/playback-rate-select/playback-rate-select.test.axe.tsprojects/media/src/playback-rate-select/playback-rate-select.test.lighthouse.tsprojects/media/src/playback-rate-select/playback-rate-select.test.ssr.tsprojects/media/src/playback-rate-select/playback-rate-select.test.tsprojects/media/src/playback-rate-select/playback-rate-select.test.visual.tsprojects/media/src/playback-rate-select/playback-rate-select.tsprojects/media/src/seek-button/define.tsprojects/media/src/seek-button/index.tsprojects/media/src/seek-button/seek-button.cssprojects/media/src/seek-button/seek-button.examples.tsprojects/media/src/seek-button/seek-button.test.axe.tsprojects/media/src/seek-button/seek-button.test.lighthouse.tsprojects/media/src/seek-button/seek-button.test.ssr.tsprojects/media/src/seek-button/seek-button.test.tsprojects/media/src/seek-button/seek-button.test.visual.tsprojects/media/src/seek-button/seek-button.tsprojects/media/src/time-range/define.tsprojects/media/src/time-range/index.tsprojects/media/src/time-range/time-range.cssprojects/media/src/time-range/time-range.examples.tsprojects/media/src/time-range/time-range.test.axe.tsprojects/media/src/time-range/time-range.test.lighthouse.tsprojects/media/src/time-range/time-range.test.ssr.tsprojects/media/src/time-range/time-range.test.tsprojects/media/src/time-range/time-range.test.visual.tsprojects/media/src/time-range/time-range.tsprojects/media/src/volume-range/define.tsprojects/media/src/volume-range/index.tsprojects/media/src/volume-range/volume-range.cssprojects/media/src/volume-range/volume-range.examples.tsprojects/media/src/volume-range/volume-range.test.axe.tsprojects/media/src/volume-range/volume-range.test.lighthouse.tsprojects/media/src/volume-range/volume-range.test.ssr.tsprojects/media/src/volume-range/volume-range.test.tsprojects/media/src/volume-range/volume-range.test.visual.tsprojects/media/src/volume-range/volume-range.tsprojects/media/tsconfig.lib.jsonprojects/media/vite.config.tsprojects/media/vitest.ssr.tsprojects/media/vitest.visual.htmlprojects/media/vitest.visual.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/_11ty/layouts/docs.cssprojects/site/src/docs/media/controller.mdprojects/site/src/docs/media/fullscreen-button.mdprojects/site/src/docs/media/mute-button.mdprojects/site/src/docs/media/pause-button.mdprojects/site/src/docs/media/playback-rate-select.mdprojects/site/src/docs/media/seek-button.mdprojects/site/src/docs/media/time-range.mdprojects/site/src/docs/media/volume-range.md
Comment on lines
+45
to
+50
| const searchSpy = vi.spyOn(MiniSearch.prototype, 'search').mockReturnValue([ | ||
| { id: 'nve-button-group', terms: ['button'], queryTerms: ['button'], score: 2, match: {} }, | ||
| { id: 'nve-button', terms: ['button'], queryTerms: ['button'], score: 1, match: {} } | ||
| ]); | ||
| const results = await ApiService.search('nve-button'); | ||
| searchSpy.mockRestore(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 3 'restoreMocks|mockRestore|spyOn\(MiniSearch\.prototype' \
projects/internals/metadataRepository: NVIDIA/elements
Length of output: 2118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file context and repo-level Vitest config/cleanup.
sed -n '1,140p' projects/internals/metadata/src/services/api.service.test.ts
printf '\n--- vitest config ---\n'
rg -n -C 2 'restoreMocks|clearMocks|mockReset|afterEach\\(|beforeEach\\(' \
projects/internals/metadata \
-g 'vitest*.{ts,js,mjs,cjs}' \
-g 'vite*.{ts,js,mjs,cjs}' \
-g 'test*.{ts,js,mjs,cjs}'
printf '\n--- package configs ---\n'
rg -n -C 2 '"test"|vitest|restoreMocks|clearMocks|mockReset' \
projects/internals/metadata/package.json \
projects/internals/metadata/**/package.json \
projects/internals/metadata/**/vitest*.{ts,js,mjs,cjs} \
projects/internals/metadata/**/vite*.{ts,js,mjs,cjs}Repository: NVIDIA/elements
Length of output: 2541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- api.service.test.ts ---\n'
sed -n '1,120p' projects/internals/metadata/src/services/api.service.test.ts
printf '\n--- restoreMocks / cleanup references ---\n'
rg -n -C 2 'restoreMocks|clearMocks|mockReset|mockRestore|afterEach\\(|beforeEach\\(' projects/internals/metadata
printf '\n--- vitest/vite config files ---\n'
fd -a 'vitest*.{ts,js,mjs,cjs}' projects/internals/metadata
fd -a 'vite*.{ts,js,mjs,cjs}' projects/internals/metadata
fd -a 'test*.{ts,js,mjs,cjs}' projects/internals/metadataRepository: NVIDIA/elements
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'restoreMocks|clearMocks|mockReset|mockRestore|afterEach\\(|beforeEach\\(' \
. \
-g 'vitest*.{ts,js,mjs,cjs}' \
-g 'vite*.{ts,js,mjs,cjs}' \
-g 'test*.{ts,js,mjs,cjs}' \
-g 'package.json'
printf '\n--- candidate config files ---\n'
fd -a 'vitest*.{ts,js,mjs,cjs}' .
fd -a 'vite*.{ts,js,mjs,cjs}' .
fd -a 'test*.{ts,js,mjs,cjs}' .Repository: NVIDIA/elements
Length of output: 345
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l projects/internals/metadata/src/services/api.service.test.ts
sed -n '1,220p' projects/internals/metadata/src/services/api.service.test.tsRepository: NVIDIA/elements
Length of output: 2414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Repo-wide Vitest/Vite/package config references without regex grouping.
rg -n -C 2 -e 'restoreMocks' -e 'clearMocks' -e 'mockReset' -e 'mockRestore' -e 'afterEach(' -e 'beforeEach(' \
. \
-g 'vitest*.ts' -g 'vitest*.js' -g 'vitest*.mjs' -g 'vitest*.cjs' \
-g 'vite*.ts' -g 'vite*.js' -g 'vite*.mjs' -g 'vite*.cjs' \
-g 'package.json'
printf '\n--- package files ---\n'
fd -a 'package.json' projects/internals/metadata
fd -a 'vitest*.ts' projects/internals/metadata
fd -a 'vite*.ts' projects/internals/metadata
printf '\n--- api.service implementation ---\n'
sed -n '1,220p' projects/internals/metadata/src/services/api.service.tsRepository: NVIDIA/elements
Length of output: 378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' projects/internals/metadata/src/services/api.service.tsRepository: NVIDIA/elements
Length of output: 1770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 -F 'restoreMocks' . -g 'package.json' -g 'vitest*.ts' -g 'vitest*.js' -g 'vitest*.mjs' -g 'vitest*.cjs' -g 'vite*.ts' -g 'vite*.js' -g 'vite*.mjs' -g 'vite*.cjs'
rg -n -C 2 -F 'clearMocks' . -g 'package.json' -g 'vitest*.ts' -g 'vitest*.js' -g 'vitest*.mjs' -g 'vitest*.cjs' -g 'vite*.ts' -g 'vite*.js' -g 'vite*.mjs' -g 'vite*.cjs'
rg -n -C 2 -F 'mockReset' . -g 'package.json' -g 'vitest*.ts' -g 'vitest*.js' -g 'vitest*.mjs' -g 'vitest*.cjs' -g 'vite*.ts' -g 'vite*.js' -g 'vite*.mjs' -g 'vite*.cjs'
rg -n -C 2 -F 'mockRestore' projects/internals/metadata/srcRepository: NVIDIA/elements
Length of output: 153
Restore the prototype spy in finally. Wrap the ApiService.search('nve-button') call so searchSpy.mockRestore() still runs if the test throws.
🤖 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 `@projects/internals/metadata/src/services/api.service.test.ts` around lines 45
- 50, Wrap the ApiService.search call in a try/finally block so the
MiniSearch.prototype spy created by searchSpy is always restored, including when
the search throws. Keep the existing search arguments and result assertions
unchanged, and move searchSpy.mockRestore() into the finally block.
Comment on lines
+358
to
+386
| it('should handle full-screen commands', async () => { | ||
| const requestFullscreen = vi.fn().mockResolvedValue(undefined); | ||
| const exitFullscreen = vi.fn().mockResolvedValue(undefined); | ||
| Object.defineProperty(controller, 'requestFullscreen', { value: requestFullscreen, configurable: true }); | ||
| Object.defineProperty(globalThis.document, 'exitFullscreen', { value: exitFullscreen, configurable: true }); | ||
|
|
||
| dispatchCommand(controller, mediaCommands.enterFullscreen); | ||
| expect(requestFullscreen).toHaveBeenCalled(); | ||
|
|
||
| Object.defineProperty(globalThis.document, 'fullscreenElement', { value: controller, configurable: true }); | ||
| const stateChange = untilEvent<MediaStateChangeEvent>(controller, mediaStateChange); | ||
| globalThis.document.dispatchEvent(new Event('fullscreenchange')); | ||
| expect((await stateChange).detail.fullscreen).toBe(true); | ||
| expect(controller.hasAttribute('fullscreen')).toBe(true); | ||
|
|
||
| dispatchCommand(controller, mediaCommands.exitFullscreen); | ||
| expect(exitFullscreen).toHaveBeenCalled(); | ||
|
|
||
| Object.defineProperty(globalThis.document, 'fullscreenElement', { | ||
| value: document.createElement('div'), | ||
| configurable: true | ||
| }); | ||
| dispatchCommand(controller, mediaCommands.exitFullscreen); | ||
| expect(exitFullscreen).toHaveBeenCalledTimes(1); | ||
|
|
||
| Object.defineProperty(globalThis.document, 'fullscreenElement', { value: null, configurable: true }); | ||
| dispatchCommand(controller, mediaCommands.exitFullscreen); | ||
| expect(exitFullscreen).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the document overrides after the full-screen test.
vi.restoreAllMocks() in afterEach (Line 49) only reverts vi.spyOn/vi.fn mocks; the raw Object.defineProperty writes to globalThis.document for fullscreenElement and exitFullscreen persist for the rest of the environment. fullscreenElement is left as a static null value descriptor, shadowing the native accessor, so any later test that relies on real full-screen state (or a reordering of tests in this file) silently sees a frozen value.
🧪 Suggested cleanup
it('should handle full-screen commands', async () => {
+ const fullscreenElementDescriptor = Object.getOwnPropertyDescriptor(Document.prototype, 'fullscreenElement');
+ const exitFullscreenDescriptor = Object.getOwnPropertyDescriptor(Document.prototype, 'exitFullscreen');
const requestFullscreen = vi.fn().mockResolvedValue(undefined);then delete the ad-hoc own properties (and reinstate the captured descriptors when present) in a finally block or a dedicated afterEach.
🤖 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 `@projects/media/src/controller/controller.test.ts` around lines 358 - 386,
Restore the original descriptors for document.fullscreenElement and
document.exitFullscreen after the “should handle full-screen commands” test,
including deleting properties that were not originally present and reinstating
captured descriptors when they were. Use a finally block or dedicated cleanup so
restoration runs even if an assertion fails, without changing the test’s
fullscreen behavior.
| version: '0.0.0' | ||
| }; | ||
|
|
||
| static formAssociated = true; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how formAssociated is used elsewhere in the repo and whether media controls rely on the controller being form-associated.
rg -nP --type=ts -C4 '\bformAssociated\b' projects | head -100
rg -nP --type=ts -C3 'attachInternals\s*\(' projects | head -60Repository: NVIDIA/elements
Length of output: 11485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the media controller and its related usages.
ast-grep outline projects/media/src/controller/controller.ts --view expanded || true
printf '\n--- file slice ---\n'
cat -n projects/media/src/controller/controller.ts | sed -n '1,220p'
printf '\n--- media form semantics usage ---\n'
rg -n --context 3 '\bformAssociated\b|\battachInternals\s*\(|setFormValue\s*\(|ElementInternals|\bMediaController\b' projects/media/src projects/media -g '*.ts' || trueRepository: NVIDIA/elements
Length of output: 50371
🌐 Web query:
formAssociated custom element attachInternals required ElementInternals static formAssociated behavior disabled inheritance form owner
💡 Result:
To create a form-associated custom element, you must define a static formAssociated property with a value of true within your custom element class [1][2]. This informs the browser that the element should behave like a standard form control [1][2]. Key components and behaviors include: ElementInternals: By calling this.attachInternals in the custom element's constructor, you obtain an ElementInternals object [3][4]. This object provides access to form-specific APIs, such as setFormValue, setValidity, and properties like form (which returns the associated HTMLFormElement) [4][2][5]. Form Association and Owner: The browser automatically handles associating the custom element with a
ancestor. The form owner can be retrieved via ElementInternals.form [1][6]. When the form association changes, the custom element's formAssociatedCallback(form) is triggered [4][7]. Disabled State and Inheritance: Form-associated custom elements react to being disabled, including inheritance from parent fieldsets [8]. When the element's disabled state changes, the formDisabledCallback(disabled) is invoked [7]. Note that simply checking the disabled attribute is often insufficient for determining the element's actual disabled state; libraries or implementations must handle the state correctly (often by checking the element's:disabled pseudo-class or internal state) [8]. Lifecycle Callbacks: - formAssociatedCallback(form): Called when the element is associated with a form [4][7]. - formDisabledCallback(disabled): Called when the element's disabled state changes (e.g., due to a disabled fieldset) [7]. - formResetCallback: Called when the associated form is reset [8]. - formStateRestoreCallback(state, mode): Called when the browser attempts to restore the element's state (e.g., after a page reload) [8].Citations:
- 1: https://html.spec.whatwg.org/dev/custom-elements.html
- 2: https://web.dev/articles/more-capable-form-controls
- 3: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/attachInternals
- 4: https://webkit.org/blog/13711/elementinternals-and-form-associated-custom-elements/
- 5: https://html.spec.whatwg.org/multipage/custom-elements.html
- 6: https://developer.mozilla.org/docs/Web/API/ElementInternals
- 7: The exact
formDisabledCallbackandformAssociatedCallbacksemantics are not specified, only derivable from informative comments whatwg/html#12169 - 8: https://registry.npmjs.org/@lit-labs/forms
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any controller documentation or tests indicating form participation.
rg -n --context 2 '\bformAssociated\b|attachInternals\s*\(|setFormValue\s*\(|<form\b|name=' projects/media -g '*.ts' -g '*.md' -g '*.mdx' -g '*.html' || true
printf '\n--- controller docs refs ---\n'
rg -n --context 2 'nve-media-controller|media/controller' projects/media -g '*.md' -g '*.mdx' -g '*.json' -g '*.ts' || trueRepository: NVIDIA/elements
Length of output: 50371
Drop formAssociated unless this controller is meant to submit form data. MediaController never calls attachInternals() or setFormValue(), and value is just a MediaState snapshot. If it’s only a command/state host, remove the flag; if it should participate in forms, add the internals plumbing.
🤖 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 `@projects/media/src/controller/controller.ts` at line 66, Remove the static
formAssociated flag from MediaController unless it is intended to submit form
data; if form participation is required, instead add attachInternals() and
setFormValue() plumbing and connect it to the MediaState value.
Comment on lines
+9
to
+12
| declare module '*.css?inline' { | ||
| const content: string; | ||
| export = content; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== declarations.d.ts ==\n'
cat -n projects/media/src/declarations.d.ts
printf '\n== search for css?inline imports ==\n'
rg -n --glob '*.{ts,tsx,js,jsx}' "css\\?inline" projects/media/src || true
printf '\n== search for default imports from inline css ==\n'
rg -n --glob '*.{ts,tsx,js,jsx}' "import\\s+.*from\\s+['\"].*css\\?inline['\"]" projects/media/src || true
printf '\n== search for require/import assignment use of inline css ==\n'
rg -n --glob '*.{ts,tsx,js,jsx}' "require\\(.*css\\?inline|import\\s*=\\s*require\\(" projects/media/src || trueRepository: NVIDIA/elements
Length of output: 3842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect local TypeScript settings and any relevant module-resolution flags.
for f in projects/media/tsconfig.json projects/media/tsconfig.*.json tsconfig.json; do
if [ -f "$f" ]; then
printf '\n== %s ==\n' "$f"
cat -n "$f"
fi
doneRepository: NVIDIA/elements
Length of output: 1709
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== paths with css?inline ==\n'
rg -n --glob '*.{ts,tsx,js,jsx,d.ts}' "css\\?inline" .
printf '\n== declaration file ==\n'
sed -n '1,40p' projects/media/src/declarations.d.tsRepository: NVIDIA/elements
Length of output: 19161
Declare *.css?inline with a default export. This module is imported with default syntax throughout the package, and export = models the wrong shape for Vite’s ?inline CSS modules. Switch to export default content;.
🤖 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 `@projects/media/src/declarations.d.ts` around lines 9 - 12, Update the
`*.css?inline` module declaration to expose `content` through a default export
instead of `export =`, matching the package’s default-import usage and Vite’s
inline CSS module shape.
Comment on lines
+77
to
+84
| button.disabled = false; | ||
| button.readonly = true; | ||
| await elementIsStable(button); | ||
| expect(button.readOnly).toBe(true); | ||
| expect(button.hasAttribute('readonly')).toBe(true); | ||
| expect(button._internals.role).toBe('none'); | ||
| expect(button._internals.ariaDisabled).toBe(null); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Typo: button.readonly should be button.readOnly.
Line 78 sets button.readonly = true, but ButtonFormControlMixinInstance exposes the property as readOnly (confirmed by the correct casing on lines 80 and 117). This assigns to a nonexistent property on ButtonElement, silently failing to set the intended readonly state and not exercising the behavior this test claims to cover.
🐛 Proposed fix
button.disabled = false;
- button.readonly = true;
+ button.readOnly = true;
await elementIsStable(button);📝 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.
Suggested change
| button.disabled = false; | |
| button.readonly = true; | |
| await elementIsStable(button); | |
| expect(button.readOnly).toBe(true); | |
| expect(button.hasAttribute('readonly')).toBe(true); | |
| expect(button._internals.role).toBe('none'); | |
| expect(button._internals.ariaDisabled).toBe(null); | |
| }); | |
| button.disabled = false; | |
| button.readOnly = true; | |
| await elementIsStable(button); | |
| expect(button.readOnly).toBe(true); | |
| expect(button.hasAttribute('readonly')).toBe(true); | |
| expect(button._internals.role).toBe('none'); | |
| expect(button._internals.ariaDisabled).toBe(null); | |
| }); |
🤖 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 `@projects/media/src/internal/button-form-control-usage.test.ts` around lines
77 - 84, Correct the property assignment in the test setup to use the
ButtonFormControlMixinInstance’s readOnly property, matching the existing
button.readOnly assertions and other usage. Keep the rest of the readonly
behavior test unchanged.
|
|
||
| describe('media playback rate select lighthouse', () => { | ||
| test('should pass lighthouse check', async () => { | ||
| const report = await lighthouseRunner.getReport('media-playback-rate-select', /* html */ ` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Align the report name with the element tag.
Sibling lighthouse tests pass the full tag (nve-media-controller, nve-media-time-range) as the report name, which becomes the generated page/report directory. Using media-playback-rate-select here makes the emitted artifacts inconsistent.
♻️ Proposed change
- const report = await lighthouseRunner.getReport('media-playback-rate-select', /* html */ `
+ const report = await lighthouseRunner.getReport('nve-media-playback-rate-select', /* html */ `📝 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.
Suggested change
| const report = await lighthouseRunner.getReport('media-playback-rate-select', /* html */ ` | |
| const report = await lighthouseRunner.getReport('nve-media-playback-rate-select', /* html */ ` |
🤖 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
`@projects/media/src/playback-rate-select/playback-rate-select.test.lighthouse.ts`
at line 9, Update the report name passed to lighthouseRunner.getReport in the
playback-rate-select test to use the full element tag, matching sibling
lighthouse tests and the generated artifact directory naming convention.
| @@ -1,4 +1,13 @@ | |||
| import { mergeConfig } from 'vite'; | |||
| import { resolve } from 'path'; | |||
| import { UserConfig, defineConfig, mergeConfig } from 'vite'; | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Import UserConfig as a type.
UserConfig is type-only. Split it into import type { UserConfig } from 'vite'; Vite recommends explicit type-only imports to avoid transformer and bundling problems. (main.vite.dev)
Proposed fix
-import { UserConfig, defineConfig, mergeConfig } from 'vite';
+import { defineConfig, mergeConfig } from 'vite';
+import type { UserConfig } from 'vite';📝 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.
Suggested change
| import { UserConfig, defineConfig, mergeConfig } from 'vite'; | |
| import { defineConfig, mergeConfig } from 'vite'; | |
| import type { UserConfig } from 'vite'; |
🤖 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 `@projects/media/vite.config.ts` at line 2, Update the Vite import in the
configuration to import UserConfig via a type-only import, while keeping
defineConfig and mergeConfig as regular runtime imports.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit