feat(media): init#188
Draft
coryrylan wants to merge 3 commits into
Draft
Conversation
Signed-off-by: Cory Rylan <crylan@nvidia.com>
github-actions
Bot
added
scope(core)
scope(ci)
scope(internals)
scope(lint)
scope(docs)
dependencies
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces a media controller and playback controls for audio/video elements, including buttons, ranges, playback-rate selection, state and command contracts, accessibility and form behavior, build/test integration, metadata generation, lint support, examples, and documentation. Existing Lighthouse JavaScript-size thresholds are also adjusted. ChangesMedia component system
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MediaControl
participant MediaController
participant HTMLMediaElement
MediaControl->>MediaController: dispatch media command
MediaController->>HTMLMediaElement: apply playback, seek, volume, rate, or fullscreen operation
HTMLMediaElement-->>MediaController: emit native media event
MediaController-->>MediaControl: dispatch media-state-change snapshot
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Comment on lines
+54
to
+57
| :host([readonly]) { | ||
| --text-decoration: none; | ||
| --cursor: cursor; | ||
| } |
Comment on lines
+347
to
+350
| function getSourceNumber(source: MediaCommandSource | null) { | ||
| const value = source?.valueAsNumber ?? source?.value; | ||
| return Number(value); | ||
| } |
Comment on lines
+75
to
+81
| <input | ||
| type="range" | ||
| min=${this.min} | ||
| max=${this.max} | ||
| step=${this.step} | ||
| .value=${`${this.value}`} | ||
| ?disabled=${this.disabled || this.readOnly || this.mediaDisabled} |
Comment on lines
+140
to
+144
| function getInputStep(min: number, max: number, step: number) { | ||
| const range = max - min; | ||
| const intervals = Math.ceil(range / step); | ||
| return range > 0 && Number.isFinite(intervals) && intervals > 0 ? (range / intervals) * (1 - Number.EPSILON) : step; | ||
| } |
![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
Comment on lines
+43
to
+47
| it('should prioritize exact matches over fuzzy matches', async () => { | ||
| const results = await ApiService.search('nve-button'); | ||
|
|
||
| expect(results[0]?.name).toBe('nve-button'); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make this test exercise exact-match promotion.
ApiService.search('nve-button') may naturally return the exact item at index 0, allowing this test to pass even if the exactMatchIndex > 0 promotion logic regresses. Use controlled search data or a query whose exact result initially appears after a fuzzy result.
🤖 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 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.
|
|
||
| const results = await runAxe([MediaController.metadata.tag]); | ||
| expect(results.violations.length).toBe(0); | ||
| removeFixture(fixture); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Always remove the fixture in a finally block.
If elementIsStable, runAxe, or the assertion fails, Line [30] is never reached and the mounted fixture can contaminate later tests. Wrap the test body in try/finally so cleanup is guaranteed.
Proposed fix
const fixture = await createFixture(html`
...
`);
- await elementIsStable(fixture.querySelector(MediaController.metadata.tag));
-
- const results = await runAxe([MediaController.metadata.tag]);
- expect(results.violations.length).toBe(0);
- removeFixture(fixture);
+ try {
+ await elementIsStable(fixture.querySelector(MediaController.metadata.tag));
+ const results = await runAxe([MediaController.metadata.tag]);
+ expect(results.violations.length).toBe(0);
+ } finally {
+ removeFixture(fixture);
+ }📝 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
| removeFixture(fixture); | |
| try { | |
| await elementIsStable(fixture.querySelector(MediaController.metadata.tag)); | |
| const results = await runAxe([MediaController.metadata.tag]); | |
| expect(results.violations.length).toBe(0); | |
| } finally { | |
| removeFixture(fixture); | |
| } |
🤖 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.axe.ts` at 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.
| --text-transform: initial; | ||
| --line-height: 1.1em; | ||
| --_icon-color: var(--color); | ||
| display: inline-flex; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the reported Stylelint violations before merge.
projects/media/src/internal/media-button.css#L23-L23: add the required blank line beforedisplay.projects/media/src/internal/media-button.css#L171-L171: changecurrentColorto the configured lowercase form,currentcolor.projects/media/src/internal/media-button.css#L225-L225: add the required blank line beforepointer-events.projects/media/src/internal/media-range.css#L17-L17: add the required blank line beforedisplay.
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 23-23: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
📍 Affects 2 files
projects/media/src/internal/media-button.css#L23-L23(this comment)projects/media/src/internal/media-button.css#L171-L171projects/media/src/internal/media-button.css#L225-L225projects/media/src/internal/media-range.css#L17-L17
🤖 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/media-button.css` at 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.
Source: Linters/SAST tools
| --picker-border: var(--nve-ref-border-width-sm) solid var(--nve-ref-border-color-muted); | ||
| --width: 32px; | ||
| --cursor: pointer; | ||
| display: block; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line before the first standard declaration in these blocks — Stylelint declaration-empty-line-before (CI-failing). Both blocks transition from custom properties to standard declarations without the required blank line.
projects/media/src/playback-rate-select/playback-rate-select.css#L16-L16: insert an empty line beforedisplay: block;.projects/media/src/playback-rate-select/playback-rate-select.css#L31-L31: insert an empty line beforebackground: var(--background);.
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 16-16: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
📍 Affects 1 file
projects/media/src/playback-rate-select/playback-rate-select.css#L16-L16(this comment)projects/media/src/playback-rate-select/playback-rate-select.css#L31-L31
🤖 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.css` at 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.
Source: Linters/SAST tools
Comment on lines
+41
to
+45
| line-height: var(--height) !important; | ||
| box-shadow: none; | ||
| color: var(--color); | ||
| cursor: var(--cursor); | ||
| font: inherit; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
font: inherit (Line 45) resets line-height set on Line 41 — Stylelint error.
The font shorthand includes line-height, so declaring it after line-height: var(--height) !important trips declaration-block-no-shorthand-property-overrides and fails the lint step. Declare font before line-height to resolve the ordering conflict.
🎨 Proposed fix
block-size: var(--height);
- line-height: var(--height) !important;
box-shadow: none;
color: var(--color);
cursor: var(--cursor);
font: inherit;
+ line-height: var(--height) !important;
inline-size: 100%;📝 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
| line-height: var(--height) !important; | |
| box-shadow: none; | |
| color: var(--color); | |
| cursor: var(--cursor); | |
| font: inherit; | |
| box-shadow: none; | |
| color: var(--color); | |
| cursor: var(--cursor); | |
| font: inherit; | |
| line-height: var(--height) !important; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 45-45: Overridden property "line-height" by shorthand "font" (declaration-block-no-shorthand-property-overrides)
(declaration-block-no-shorthand-property-overrides)
🤖 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.css` around
lines 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.
Source: Linters/SAST tools
Comment on lines
+121
to
+129
| #handleMediaState = (state: MediaState | null) => { | ||
| if (!state) { | ||
| return; | ||
| } | ||
|
|
||
| const playbackRate = state.playbackRate; | ||
| if (Number.isFinite(playbackRate) && playbackRate > 0) { | ||
| this.value = `${playbackRate}`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Render media rates not present in rates.
Line 128 can assign a valid rate such as 1.25, but Lines 82-84 render no matching <option> with default rates. The select then appears unselected despite an active media rate. Include the active valid state rate in the rendered options (or define a documented fallback).
🤖 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.ts` around lines
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.
Comment on lines
+49
to
+53
| @property({ type: String, reflect: true }) command: string = mediaCommands.seek; | ||
|
|
||
| @property({ type: String, attribute: 'commandfor', reflect: true }) commandfor: string | null = null; | ||
|
|
||
| @property({ attribute: false }) commandForElement: HTMLElement | null = null; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd command-target.ts
cat -n projects/media/src/internal/command-target.ts 2>/dev/null
rg -nP '\bcommand(for|ForElement)?\b\s*[:=]' projects/media/src/volume-range/volume-range.ts 2>/dev/nullRepository: NVIDIA/elements
Length of output: 1247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant files and surrounding definitions.
sed -n '1,220p' projects/media/src/time-range/time-range.ts
printf '\n--- volume-range ---\n'
sed -n '1,220p' projects/media/src/volume-range/volume-range.ts
printf '\n--- pause-button ---\n'
sed -n '1,220p' projects/media/src/pause-button/pause-button.ts
printf '\n--- command-target ---\n'
sed -n '1,220p' projects/media/src/internal/command-target.ts
printf '\n--- search for shared command contract ---\n'
rg -n "commandForElement|commandfor|command-target|getCommandTarget|ButtonFormControlMixin" projects/media/srcRepository: NVIDIA/elements
Length of output: 31399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any shared mixin/utility that declares these properties or a broader command contract.
rg -n --glob '!**/dist/**' --glob '!**/build/**' --glob '!**/node_modules/**' \
"class .*Mixin|interface .*Command|type .*Command|commandForElement|attribute: 'commandfor'|attribute: false.*commandForElement|`@property`\\(\\{ type: String, attribute: 'commandfor'" \
projects/media/srcRepository: NVIDIA/elements
Length of output: 4884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the button control file and any mixins that may be the shared contract.
fd -a "pause-button.ts|ButtonFormControlMixin|*mixin*.ts" projects/media/srcRepository: NVIDIA/elements
Length of output: 521
Extract the shared command contract. command, commandfor, and commandForElement are duplicated across time-range, volume-range, and playback-rate-select; projects/media/src/internal/command-target.ts only resolves the target, so a shared mixin or base contract would keep these controls in sync.
🤖 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 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.
Comment on lines
+63
to
+65
| @state() protected mediaDisabled = false; | ||
|
|
||
| #mediaStateController = new MediaStateController(this, mediaState => this.#handleMediaState(mediaState)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Synchronize mediaDisabled from media state.
mediaDisabled starts as false and is never updated. Consequently, the disabled input state and null form-value path at Lines 81 and 99 cannot activate from target state; #handleMediaState only syncs volume. Update this state from the relevant media-state capability/disabled field and test it.
Also applies to: 96-105, 124-130
🤖 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/volume-range/volume-range.ts` around lines 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.
Comment on lines
+88
to
+103
| ### State Attributes | ||
|
|
||
| The controller reflects media state for styling and inspection. | ||
|
|
||
| | Attribute | Type | Source | | ||
| | -------------- | ------- | --------------------------------------- | | ||
| | `paused` | boolean | `media.paused` | | ||
| | `muted` | boolean | `media.muted` | | ||
| | `ended` | boolean | `media.ended` | | ||
| | `seeking` | boolean | `media.seeking` | | ||
| | `fullscreen` | boolean | Controller host is in full-screen mode. | | ||
| | `current-time` | number | `media.currentTime` | | ||
| | `duration` | number | `media.duration` when finite. | | ||
| | `volume` | number | `media.volume` from `0` to `1`. | | ||
|
|
||
| The controller exposes the latest immutable state through `mediaState`. It dispatches a `media-state-change` event with the complete state snapshot when native media, command, slot, or full-screen updates change that state. The event does not bubble. Controls read `mediaState` when they bind to a command target and listen for later state events. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the playback-rate state contract consistent across both documents.
projects/site/src/docs/api-design/media.md#L88-L103: AddplaybackRateand its reflectedplayback-rateattribute, or explicitly remove them from the controller API.projects/site/src/docs/media/controller.md#L32-L45: Keep the documentedmediaStatefields and reflected attributes aligned with the API design.
📍 Affects 2 files
projects/site/src/docs/api-design/media.md#L88-L103(this comment)projects/site/src/docs/media/controller.md#L32-L45
🤖 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 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.
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
Comment on lines
+82
to
+84
| ${getRateOptions(this.rates).map( | ||
| rate => html`<option value=${rate}>${formatI18n(this.i18n.playbackRateOption, 'rate', rate)}</option>` | ||
| )} |
Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
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
+27
to
+34
| 'nve-copy-button', | ||
| 'nve-media-fullscreen-button', | ||
| 'nve-media-mute-button', | ||
| 'nve-media-pause-button', | ||
| 'nve-media-playback-rate-select', | ||
| 'nve-media-seek-button', | ||
| 'nve-media-time-range', | ||
| 'nve-media-volume-range' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== rule file ==\n'
sed -n '1,220p' projects/lint/src/eslint/rules/no-invalid-invoker-triggers.ts
printf '\n== tests mentioning the rule ==\n'
rg -n "no-invalid-invoker-triggers|commandfor|popovertarget|interestfor|BUTTON_TYPE_ELEMENTS|INVOKER_ATTRIBUTES" projects/lint -g '*.{ts,js,tsx,jsx}'Repository: NVIDIA/elements
Length of output: 19859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' projects/lint/src/eslint/rules/no-invalid-invoker-triggers.tsRepository: NVIDIA/elements
Length of output: 2635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('projects/lint/src/eslint/rules/no-invalid-invoker-triggers.ts')
text = p.read_text()
for needle in ['INVOKER_ATTRIBUTES', 'BUTTON_TYPE_ELEMENTS', 'commandfor', 'popovertarget', 'interestfor']:
print(f'\n== {needle} ==')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: NVIDIA/elements
Length of output: 876
Keep the invoker allowlist attribute-specific. BUTTON_TYPE_ELEMENTS gates all entries in INVOKER_ATTRIBUTES, so adding nve-media-playback-rate-select, nve-media-time-range, and nve-media-volume-range makes them valid for popovertarget and interestfor too, not just commandfor. Split the allowlist by attribute, or add coverage for the intended support matrix.
🤖 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/lint/src/eslint/rules/no-invalid-invoker-triggers.ts` around lines
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.
Comment on lines
+325
to
+345
| "test:visual": { | ||
| "command": "vitest run --config=vitest.visual.ts", | ||
| "clean": false, | ||
| "files": [ | ||
| "dist/**/*.js", | ||
| "src/**/*.test.visual.ts", | ||
| "vitest.visual.html", | ||
| "vitest.visual.ts" | ||
| ], | ||
| "output": [ | ||
| ".visual/*.png", | ||
| "coverage/visual/**" | ||
| ], | ||
| "dependencies": [ | ||
| "../internals/vite:ci", | ||
| { | ||
| "script": "build", | ||
| "cascade": false | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
test:visual doesn't declare a build dependency on themes/styles, unlike test:ssr.
vitest.visual.html directly @imports @nvidia-elements/themes/fonts/inter.css, .../index.css, .../dark.css, and @nvidia-elements/styles/typography.css/layout.css — all resolved from those packages' dist output. test:ssr (lines 369-395) correctly tracks this by including "../themes/dist/**/*.css" in files and {"script": "../themes:build", "cascade": false} in dependencies, plus tsconfig.json in files. test:visual has none of this: its dependencies only include ../internals/vite:ci and a non-cascading build, and its files array omits both tsconfig.json and any themes/styles glob.
Per wireit semantics, a script only runs a dependency's build (or reacts to its output changes) if that dependency is explicitly declared — omitting it here means wireit has no guarantee that ../themes:build (and likely ../styles:build) has run before test:visual, and no cache invalidation when those packages' CSS changes, risking stale/incorrect visual snapshots being treated as passing.
🔧 Proposed fix to align test:visual with test:ssr's dependency tracking
"test:visual": {
"command": "vitest run --config=vitest.visual.ts",
"clean": false,
"files": [
"dist/**/*.js",
"src/**/*.test.visual.ts",
"vitest.visual.html",
- "vitest.visual.ts"
+ "vitest.visual.ts",
+ "tsconfig.json",
+ "../themes/dist/**/*.css",
+ "../styles/dist/**/*.css"
],
"output": [
".visual/*.png",
"coverage/visual/**"
],
"dependencies": [
"../internals/vite:ci",
{
"script": "build",
"cascade": false
+ },
+ {
+ "script": "../themes:build",
+ "cascade": false
+ },
+ {
+ "script": "../styles:build",
+ "cascade": false
}
]
},Also applies to: 369-395
🤖 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/package.json` around lines 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.
Comment on lines
+93
to
+100
| <script type="module"> | ||
| const form = document.querySelector('#controller-form'); | ||
| const pre = document.querySelector('#controller-form pre'); | ||
| form.addEventListener('input', () => { | ||
| const values = Object.fromEntries(new FormData(form).entries()); | ||
| pre.innerText = JSON.stringify(values, null, 2); | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant region.
ast-grep outline projects/media/src/controller/controller.examples.ts --view expanded
printf '\n---\n'
sed -n '1,220p' projects/media/src/controller/controller.examples.tsRepository: NVIDIA/elements
Length of output: 8166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the example harness and any usage of controller examples.
rg -n "controller.examples|controller-form|FormValues|<script type=\"module\">" projects/media -SRepository: NVIDIA/elements
Length of output: 2249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller visual test and how examples are rendered in this package.
sed -n '1,220p' projects/media/src/controller/controller.test.visual.ts
printf '\n---\n'
rg -n "controller.examples|render: \(\) => html|storiesOf|Meta|StoryObj|playground|examples" projects/media/src -SRepository: NVIDIA/elements
Length of output: 3556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller visual test and how examples are consumed in this package.
sed -n '1,220p' projects/media/src/controller/controller.test.visual.ts
printf '\n---\n'
rg -n "controller.examples|render: \(\) => html|storiesOf|Meta|StoryObj|playground|examples" projects/media/src -SRepository: NVIDIA/elements
Length of output: 3556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how media examples are consumed and whether they are rendered through Lit.
rg -n "controller\.examples|examples\.ts|render\(\): => html|html`|TemplateResult|unsafeHTML|lit-html|storybook|docs" projects/media -S
printf '\n--- files ---\n'
git ls-files projects/media/src/controller | sortRepository: NVIDIA/elements
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the consumer path for examples in the media package.
rg -n "controller\.examples|examples\.ts|render\(\): => html|html`|TemplateResult|unsafeHTML|lit-html|storybook|docs" projects/media -S
printf '\n--- files ---\n'
git ls-files projects/media/src/controller | sortRepository: NVIDIA/elements
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find who imports or references the controller examples and how example modules are rendered.
rg -n "FormValues|controller\.examples|examples\.ts" projects/media src . -S --glob '!**/dist/**' --glob '!**/build/**'
printf '\n---\n'
git ls-files | rg '(^|/)(stories|story|examples|docs|playground|mdx|test\.visual|test\.lighthouse)\.(ts|tsx|js|jsx|mdx|html)$'Repository: NVIDIA/elements
Length of output: 6128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the docs-site example renderer and the generated examples format.
sed -n '1,220p' projects/site/src/examples/examples.11ty.js
printf '\n---\n'
sed -n '1,260p' projects/internals/vite/src/plugins/examples.js
printf '\n---\n'
sed -n '1,220p' projects/site/src/docs/media/controller.mdRepository: NVIDIA/elements
Length of output: 9781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the examples viewer to see how generated example HTML is injected.
sed -n '1,260p' projects/site/src/examples/examples.ts
printf '\n---\n'
rg -n "iframe|srcdoc|innerHTML|insertAdjacentHTML|examples.json|renderTemplate" projects/site/src/examples projects/site/src/docs -SRepository: NVIDIA/elements
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the example page route that consumes the generated JSON.
rg -n "examples\.json|entrypoint|iframe\.src|innerHTML|srcdoc|template\.template|renderTemplate\(" projects/site/src/examples projects/site/src/docs -S --glob '!**/dist/**' | head -n 200
printf '\n---\n'
sed -n '1,220p' projects/site/src/examples/index.tsRepository: NVIDIA/elements
Length of output: 30628
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the shortcode/renderer that turns example JSON into page HTML.
rg -n "shortcode.*example|addShortcode\\(['\"]example|example shortcode|examples\\.json|json', 'FormValues'|controller/examples\\.json" projects/site/src projects/site/.eleventy* projects/site -S --glob '!**/dist/**' | head -n 200Repository: NVIDIA/elements
Length of output: 29461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the example shortcode that renders example JSON into the iframe page.
sed -n '1,260p' projects/site/src/_11ty/shortcodes/example.js
printf '\n---\n'
sed -n '1,220p' projects/site/src/examples/index.11ty.jsRepository: NVIDIA/elements
Length of output: 9884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how the canvas/component used by example shortcodes handles template content and scripts.
rg -n "setHTMLUnsafe|nvd-canvas|template>.*script|cloneNode|createElement\\('script'\\)|innerHTML = doc.body.innerHTML|querySelector\\('template'\\)" projects/site projects/internals projects/media -S --glob '!**/dist/**' | head -n 200Repository: NVIDIA/elements
Length of output: 2399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the generated JSON for the controller examples if it exists in the repo.
fd -a "controller.examples.json" projects/media -t f
printf '\n---\n'
if [ -f projects/media/dist/controller/controller.examples.json ]; then
sed -n '1,220p' projects/media/dist/controller/controller.examples.json
fiRepository: NVIDIA/elements
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the nvd-canvas component to see how example templates are stamped.
sed -n '1,320p' projects/site/src/_internal/canvas/canvas.ts
printf '\n---\n'
sed -n '1,320p' projects/site/src/_internal/canvas-editable/canvas-editable.tsRepository: NVIDIA/elements
Length of output: 15704
Move the listener out of the example template. The inline nve-canvas preview won’t run the embedded module script here, so the input handler never attaches and the <pre> stays static. Bind it in Lit or attach it from the example harness instead.
🤖 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.examples.ts` around lines 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.
Comment on lines
+36
to
+49
| * @command --play - Play the slotted media element. | ||
| * @command --pause - Pause the slotted media element. | ||
| * @command --toggle-play - Toggle play and pause. | ||
| * @command --mute - Mute the media element. | ||
| * @command --unmute - Unmute the media element. | ||
| * @command --toggle-mute - Toggle media muted state. | ||
| * @command --seek - Move playback to the source valueAsNumber. | ||
| * @command --seek-start - Move playback to zero seconds. | ||
| * @command --seek-end - Move playback to the finite duration. | ||
| * @command --set-volume - Set media volume to the source valueAsNumber. | ||
| * @command --set-playback-rate - Set media playback rate to the source valueAsNumber. | ||
| * @command --enter-fullscreen - Request full-screen mode on the controller. | ||
| * @command --exit-fullscreen - Exit full-screen mode. | ||
| * @command --toggle-fullscreen - Toggle full-screen mode. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing @command docs for backward/forward seek.
#commandHandlers registers mediaCommands.seekBackward and mediaCommands.seekForward (Lines 84-85), but the @command JSDoc block omits --seek-backward and --seek-forward. Since @audit() drives generated API metadata and docs, these two supported commands will be absent from the published surface.
📝 Proposed doc additions
* `@command` --seek-start - Move playback to zero seconds.
* `@command` --seek-end - Move playback to the finite duration.
+ * `@command` --seek-backward - Move playback backward by the source valueAsNumber.
+ * `@command` --seek-forward - Move playback forward by the source valueAsNumber.
* `@command` --set-volume - Set media volume to the source valueAsNumber.📝 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
| * @command --play - Play the slotted media element. | |
| * @command --pause - Pause the slotted media element. | |
| * @command --toggle-play - Toggle play and pause. | |
| * @command --mute - Mute the media element. | |
| * @command --unmute - Unmute the media element. | |
| * @command --toggle-mute - Toggle media muted state. | |
| * @command --seek - Move playback to the source valueAsNumber. | |
| * @command --seek-start - Move playback to zero seconds. | |
| * @command --seek-end - Move playback to the finite duration. | |
| * @command --set-volume - Set media volume to the source valueAsNumber. | |
| * @command --set-playback-rate - Set media playback rate to the source valueAsNumber. | |
| * @command --enter-fullscreen - Request full-screen mode on the controller. | |
| * @command --exit-fullscreen - Exit full-screen mode. | |
| * @command --toggle-fullscreen - Toggle full-screen mode. | |
| * `@command` --play - Play the slotted media element. | |
| * `@command` --pause - Pause the slotted media element. | |
| * `@command` --toggle-play - Toggle play and pause. | |
| * `@command` --mute - Mute the media element. | |
| * `@command` --unmute - Unmute the media element. | |
| * `@command` --toggle-mute - Toggle media muted state. | |
| * `@command` --seek - Move playback to the source valueAsNumber. | |
| * `@command` --seek-start - Move playback to zero seconds. | |
| * `@command` --seek-end - Move playback to the finite duration. | |
| * `@command` --seek-backward - Move playback backward by the source valueAsNumber. | |
| * `@command` --seek-forward - Move playback forward by the source valueAsNumber. | |
| * `@command` --set-volume - Set media volume to the source valueAsNumber. | |
| * `@command` --set-playback-rate - Set media playback rate to the source valueAsNumber. | |
| * `@command` --enter-fullscreen - Request full-screen mode on the controller. | |
| * `@command` --exit-fullscreen - Exit full-screen mode. | |
| * `@command` --toggle-fullscreen - Toggle full-screen mode. |
🤖 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` around lines 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`.
Comment on lines
+87
to
+91
| #syncPressedState = (state: MediaState | null) => { | ||
| if (state) { | ||
| this.pressed = state.fullscreen; // eslint-disable-line local/stateless-property -- Full-screen button mirrors the target controller state. | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset pressed when the target has no media state.
MediaStateController can provide null when the target is removed or no longer exposes state, but this callback preserves the prior fullscreen value. The control can remain labeled “exit fullscreen” with no fullscreen target.
Proposed fix
`#syncPressedState` = (state: MediaState | null) => {
- if (state) {
- this.pressed = state.fullscreen; // eslint-disable-line local/stateless-property -- Full-screen button mirrors the target controller state.
- }
+ this.pressed = state?.fullscreen ?? false; // eslint-disable-line local/stateless-property -- Full-screen button mirrors the target controller state.
};📝 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
| #syncPressedState = (state: MediaState | null) => { | |
| if (state) { | |
| this.pressed = state.fullscreen; // eslint-disable-line local/stateless-property -- Full-screen button mirrors the target controller state. | |
| } | |
| }; | |
| `#syncPressedState` = (state: MediaState | null) => { | |
| this.pressed = state?.fullscreen ?? false; // eslint-disable-line local/stateless-property -- Full-screen button mirrors the target controller state. | |
| }; |
🤖 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/fullscreen-button/fullscreen-button.ts` around lines 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.
Comment on lines
+86
to
+97
| const commandFixture = await createFixture(html` | ||
| <nve-media-seek-button commandfor="seek-target" command="--seek" action="end"></nve-media-seek-button> | ||
| <div id="seek-target"></div> | ||
| `); | ||
| const explicitButton = getElement<MediaSeekButton>(commandFixture, MediaSeekButton.metadata.tag); | ||
| const explicitTarget = getElement(commandFixture, '#seek-target'); | ||
| await elementIsStable(explicitButton); | ||
|
|
||
| const event = untilEvent<Event & { command: string }>(explicitTarget, 'command'); | ||
| await emulateClick(explicitButton); | ||
| expect((await event).command).toBe(mediaCommands.seek); | ||
| removeFixture(commandFixture); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ensure nested fixtures are always cleaned up.
A failed assertion before the final removeFixture() leaves DOM state behind and can contaminate later tests. Use try/finally or register cleanup with the test lifecycle.
projects/media/src/seek-button/seek-button.test.ts#L86-L97: clean upcommandFixtureinfinally.projects/media/src/time-range/time-range.test.ts#L141-L153: clean upformFixtureinfinally.
📍 Affects 2 files
projects/media/src/seek-button/seek-button.test.ts#L86-L97(this comment)projects/media/src/time-range/time-range.test.ts#L141-L153
🤖 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/seek-button/seek-button.test.ts` around lines 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.
| version: '0.0.0' | ||
| }; | ||
|
|
||
| @property({ type: String, reflect: true }) action: MediaSeekAction = 'forward'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline projects/media/src/seek-button/seek-button.ts --items all
rg -n -C3 --type=ts 'MediaSeekAction|getSeekCommand|action.*invalid|action=.*forward' projects/media/srcRepository: NVIDIA/elements
Length of output: 13992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' projects/media/src/seek-button/seek-button.ts
printf '\n--- TESTS ---\n'
sed -n '1,220p' projects/media/src/seek-button/seek-button.test.ts
printf '\n--- SEARCH ---\n'
rg -n -C2 "seek-button|action=\"|MediaSeekAction|getSeekCommand|command = this.#derivedCommand|`#hasExplicitCommand`" projects/media/src/seek-buttonRepository: NVIDIA/elements
Length of output: 27294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('projects/media/src/seek-button/seek-button.ts')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 35 <= i <= 180:
print(f"{i:4}: {line}")
PYRepository: NVIDIA/elements
Length of output: 4462
Validate action before deriving the seek command. action is a reflected string attribute, so malformed values can still reach runtime; here they produce undefined command state while the button falls back to the start icon/null label. Normalize unsupported values to a safe default or reject them, and add a test for the attribute path.
🤖 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/seek-button/seek-button.ts` at 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.
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.
Comment on lines
+111
to
+127
| | Command | Behavior | Source value | | ||
| | --------------------- | -------------------------------------------- | ---------------------- | | ||
| | `--play` | Calls `media.play()`. | None | | ||
| | `--pause` | Calls `media.pause()`. | None | | ||
| | `--toggle-play` | Plays when paused, pauses when playing. | None | | ||
| | `--mute` | Sets `media.muted` to `true`. | None | | ||
| | `--unmute` | Sets `media.muted` to `false`. | None | | ||
| | `--toggle-mute` | Toggles `media.muted`. | None | | ||
| | `--seek` | Sets `media.currentTime`. | `source.valueAsNumber` | | ||
| | `--seek-start` | Sets `media.currentTime` to `0`. | None | | ||
| | `--seek-end` | Sets `media.currentTime` to duration. | None | | ||
| | `--seek-backward` | Decreases current time. | `source.valueAsNumber` | | ||
| | `--seek-forward` | Increases current time. | `source.valueAsNumber` | | ||
| | `--set-volume` | Sets `media.volume`. | `source.valueAsNumber` | | ||
| | `--enter-fullscreen` | Requests full-screen mode on the controller. | None | | ||
| | `--exit-fullscreen` | Exits full-screen mode. | None | | ||
| | `--toggle-fullscreen` | Toggles full-screen mode. | None | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document all implemented playback-rate surfaces.
The proposal also needs to:
- Add
--set-playback-ratewithsource.valueAsNumberto the command table. - List
nve-media-playback-rate-selectamong form-associated controls. - Add its package directory and implementation-plan step.
These surfaces are already used by projects/media/src/internal/media-command.ts and documented in projects/site/src/docs/media/playback-rate-select.md.
Also applies to: 143-157, 381-421
🤖 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 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.
Comment on lines
+16
to
+22
| `nve-media-volume-range` sends `--set-volume` on user input and syncs its value from the target controller's `mediaState` events. The value uses the native media volume scale from `0` to `1`. | ||
|
|
||
| ```html | ||
| <nve-media-controller id="controller"> | ||
| <video src="/static/video/particle.mp4" playsinline></video> | ||
| <nve-media-volume-range commandfor="controller" name="volume" value="0.8"></nve-media-volume-range> | ||
| </nve-media-controller> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the misleading initial value="0.8" from the example.
The controller is the source of truth, and the range synchronizes from mediaState. This markup does not initialize the native video’s volume, so the control will be reset to the media default of 1. Either omit value or initialize video.volume explicitly.
Suggested correction
- <nve-media-volume-range commandfor="controller" name="volume" value="0.8"></nve-media-volume-range>
+ <nve-media-volume-range commandfor="controller" name="volume"></nve-media-volume-range>📝 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
| `nve-media-volume-range` sends `--set-volume` on user input and syncs its value from the target controller's `mediaState` events. The value uses the native media volume scale from `0` to `1`. | |
| ```html | |
| <nve-media-controller id="controller"> | |
| <video src="/static/video/particle.mp4" playsinline></video> | |
| <nve-media-volume-range commandfor="controller" name="volume" value="0.8"></nve-media-volume-range> | |
| </nve-media-controller> | |
| `nve-media-volume-range` sends `--set-volume` on user input and syncs its value from the target controller's `mediaState` events. The value uses the native media volume scale from `0` to `1`. | |
🤖 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/media/volume-range.md` around lines 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.
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).
| return null; | ||
| } | ||
|
|
||
| return getFlattenedDOMTree(host.getRootNode() as HTMLElement).find(element => element.id === id) ?? null; |
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
New Features
nve-media-controllerwith new media control components: fullscreen, mute, pause, seek, time-range, playback-rate select, and volume-range, including command handling and state synchronization.Documentation
Tests / Quality