fix(egress-composite): adjust PaginatedGrid sizing#1966
Open
arnautov-anton wants to merge 10 commits intomainfrom
Open
fix(egress-composite): adjust PaginatedGrid sizing#1966arnautov-anton wants to merge 10 commits intomainfrom
arnautov-anton wants to merge 10 commits intomainfrom
Conversation
arnautov-anton
force-pushed
the
fix/egress-composite/paginated-grid-sizing-and-refactors
branch
2 times, most recently
from
08e2701 to
a3f8662
Compare
arnautov-anton
force-pushed
the
fix/egress-composite/paginated-grid-sizing-and-refactors
branch
from
a4d02bd to
1d46205
Compare
…nd-refactors # Conflicts: # sample-apps/react/egress-composite/src/hooks/useInitializeClient.ts
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/react-sdk/src/core/components/CallLayout/PaginatedGridLayout.tsx (1)
35-66: Consider memoizingPaginatedGridLayoutGroupfor large participant lists.This component renders within
PaginatedGridLayoutand iterates over participants. Wrapping it withReact.memocould prevent unnecessary re-renders when parent state changes but props remain the same.♻️ Optional: Add React.memo
-const PaginatedGridLayoutGroup = ({ +const PaginatedGridLayoutGroup = React.memo(function PaginatedGridLayoutGroup({ group, mirror, VideoPlaceholder, PictureInPicturePlaceholder, ParticipantViewUI, -}: PaginatedGridLayoutGroupProps) => { +}: PaginatedGridLayoutGroupProps) { return ( // ... component body unchanged ); -}; +});As per coding guidelines: "Use React.memo for components rendered in large participant lists to optimize re-render performance"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-sdk/src/core/components/CallLayout/PaginatedGridLayout.tsx` around lines 35 - 66, Wrap the PaginatedGridLayoutGroup component with React.memo to avoid unnecessary re-renders when its props (group, mirror, VideoPlaceholder, PictureInPicturePlaceholder, ParticipantViewUI) haven't changed; locate the PaginatedGridLayoutGroup function and export (or assign) a memoized version using React.memo(PaginatedGridLayoutGroup) so the component only re-renders on prop changes while continuing to render ParticipantView for each participant.sessionId as before.sample-apps/react/egress-composite/src/hooks/options/useVideoStyles.ts (1)
17-37: Avoid emitting optional overrides with empty values.This block is always generated, so unset options still serialize
background-color/object-fitdeclarations with no value. Browsers will ignore them, but it makes the generated CSS harder to inspect and diverges from the conditionalcx([...])pattern already used in the sibling option hooks.♻️ Possible cleanup
const styles = [ - css` - `@layer` overrides-layer { - & .str-video__video { - background-color: ${videoBackgroundColor}; - } - - & .str-video__video { - object-fit: ${typeof videoScaleMode === 'undefined' - ? undefined - : objectFitMap[videoScaleMode]}; - } - - & .str-video__video.str-video__video--screen-share { - object-fit: ${objectFitMap[videoScreenshareScaleMode]}; - } - } - `, + videoBackgroundColor && + css` + `@layer` overrides-layer { + & .str-video__video { + background-color: ${videoBackgroundColor}; + } + } + `, + typeof videoScaleMode !== 'undefined' && + css` + `@layer` overrides-layer { + & .str-video__video { + object-fit: ${objectFitMap[videoScaleMode]}; + } + } + `, + css` + `@layer` overrides-layer { + & .str-video__video.str-video__video--screen-share { + object-fit: ${objectFitMap[videoScreenshareScaleMode]}; + } + } + `, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/egress-composite/src/hooks/options/useVideoStyles.ts` around lines 17 - 37, The generated CSS always emits empty background-color/object-fit declarations; update useVideoStyles to only add the corresponding css fragments when values are present: check videoBackgroundColor before adding the block that sets background-color, check typeof videoScaleMode !== 'undefined' (or presence) before adding the object-fit block that uses objectFitMap[videoScaleMode], and check videoScreenshareScaleMode before adding the screenshare object-fit block. Modify the styles array construction (where css and cx are used) to push these conditional css snippets (or build separate css calls) so undefined values are never serialized; keep using css, cx, objectFitMap, videoScaleMode, videoScreenshareScaleMode, and videoBackgroundColor to locate and change the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@sample-apps/react/egress-composite/src/components/layouts/Spotlight/Spotlight.scss`:
- Line 12: Insert a blank line immediately above the single-line SCSS comment
"// max-width: unset;" in Spotlight.scss so there is an empty line before that
comment, which will satisfy the stylelint comment-spacing rule; locate the
comment text "// max-width: unset;" and add one empty line above it.
In `@sample-apps/react/egress-composite/src/components/LogoAndTitle.scss`:
- Around line 7-10: Remove the unexpected empty lines before the declarations in
LogoAndTitle.scss: open the selector/block that contains the width and height
properties (the lines with "width" and "height") and delete the blank lines
immediately preceding those declarations so the properties appear directly under
the selector with no empty lines between the selector and "width" and between
"width" and "height".
In `@sample-apps/react/egress-composite/src/CompositeApp.scss`:
- Around line 8-9: The typography block currently violates stylelint rules: add
a blank line before the declarations to satisfy declaration-empty-line-before
and normalize keyword casing for values to satisfy value-keyword-case;
specifically, in CompositeApp.scss update the block containing the
font-synthesis and text-rendering properties so there is an empty line before
those declarations and change the text-rendering value to the linter-expected
case (use lowercase keyword form) while keeping font-synthesis: none as-is.
In `@sample-apps/react/egress-composite/src/global.d.ts`:
- Line 5: The declaration for setupLayout references the type ConfigurationValue
but it isn't imported; update global.d.ts to import ConfigurationValue from
./ConfigurationContext.tsx and then use that imported type in the setupLayout
signature (the symbol names to edit are ConfigurationValue and setupLayout in
global.d.ts, and the source is ConfigurationContext.tsx).
---
Nitpick comments:
In `@packages/react-sdk/src/core/components/CallLayout/PaginatedGridLayout.tsx`:
- Around line 35-66: Wrap the PaginatedGridLayoutGroup component with React.memo
to avoid unnecessary re-renders when its props (group, mirror, VideoPlaceholder,
PictureInPicturePlaceholder, ParticipantViewUI) haven't changed; locate the
PaginatedGridLayoutGroup function and export (or assign) a memoized version
using React.memo(PaginatedGridLayoutGroup) so the component only re-renders on
prop changes while continuing to render ParticipantView for each
participant.sessionId as before.
In `@sample-apps/react/egress-composite/src/hooks/options/useVideoStyles.ts`:
- Around line 17-37: The generated CSS always emits empty
background-color/object-fit declarations; update useVideoStyles to only add the
corresponding css fragments when values are present: check videoBackgroundColor
before adding the block that sets background-color, check typeof videoScaleMode
!== 'undefined' (or presence) before adding the object-fit block that uses
objectFitMap[videoScaleMode], and check videoScreenshareScaleMode before adding
the screenshare object-fit block. Modify the styles array construction (where
css and cx are used) to push these conditional css snippets (or build separate
css calls) so undefined values are never serialized; keep using css, cx,
objectFitMap, videoScaleMode, videoScreenshareScaleMode, and
videoBackgroundColor to locate and change the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a1170ea-ab79-41b1-a266-c51228159a95
⛔ Files ignored due to path filters (8)
packages/audio-filters-web/src/krispai/dist/krispsdk.es5.jsis excluded by!**/dist/**packages/audio-filters-web/src/krispai/dist/krispsdk.jsis excluded by!**/dist/**packages/audio-filters-web/src/krispai/dist/krispsdk.mjsis excluded by!**/dist/**packages/audio-filters-web/src/krispai/dist/usermedia.jsis excluded by!**/dist/**packages/audio-filters-web/src/krispai/dist/usermedia.mjsis excluded by!**/dist/**sample-apps/react/egress-composite/tests/__screenshots__/layouts.spec.ts/Layouts-Should-render-layout---grid-1.pngis excluded by!**/*.pngsample-apps/react/egress-composite/tests/__screenshots__/layouts.spec.ts/Layouts-Should-render-layout---grid-with-size-constraints-1.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
packages/react-sdk/src/core/components/CallLayout/PaginatedGridLayout.tsxsample-apps/react/egress-composite/package.jsonsample-apps/react/egress-composite/src/CompositeApp.scsssample-apps/react/egress-composite/src/CompositeApp.tsxsample-apps/react/egress-composite/src/ConfigurationContext.tsxsample-apps/react/egress-composite/src/components/LogoAndTitle.scsssample-apps/react/egress-composite/src/components/layouts/DominantSpeaker/DominantSpeaker.scsssample-apps/react/egress-composite/src/components/layouts/DominantSpeaker/DominantSpeakerScreenShare.scsssample-apps/react/egress-composite/src/components/layouts/PaginatedGrid/PaginatedGrid.scsssample-apps/react/egress-composite/src/components/layouts/PaginatedGrid/PaginatedGrid.tsxsample-apps/react/egress-composite/src/components/layouts/Spotlight/Spotlight.scsssample-apps/react/egress-composite/src/global.d.tssample-apps/react/egress-composite/src/hooks/options/useGenericLayoutStyles.tssample-apps/react/egress-composite/src/hooks/options/useLogoAndTitleStyles.tssample-apps/react/egress-composite/src/hooks/options/useParticipantLabelStyles.tssample-apps/react/egress-composite/src/hooks/options/useParticipantStyles.tssample-apps/react/egress-composite/src/hooks/options/useVideoStyles.tssample-apps/react/egress-composite/src/main.scsssample-apps/react/egress-composite/src/main.tsxsample-apps/react/egress-composite/tests/layouts.spec.ts
💤 Files with no reviewable changes (1)
- sample-apps/react/egress-composite/package.json
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
3 participants
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.
💡 Overview
Ref: https://getstream.slack.com/archives/C090P1YS3U6/p1760375157855869
📝 Implementation notes
As a side-quest this PR refactors styling overrides and adds layers for easier CSS adjustments, removes
clsx(in favour ofcxfrom@emotion/css) and replacesdecodefromjs-base64withatob.🎫 Ticket: https://linear.app/stream/issue/XYZ-123
📑 Docs: https://github.com/GetStream/docs-content/pull/1204
Summary by CodeRabbit
New Features
Chores