fix(comments): restore rounded corners on inline video player#4706
Conversation
This comment has been minimized.
This comment has been minimized.
realmeylisdev
left a comment
There was a problem hiding this comment.
Review summary (detailed findings inline)
The code I traced is correct and the comments path is tested + analyzer-clean. The main concern isn't a bug — it's that the PR description hides the larger half of the change.
Net change vs main: VideoCommentPlayer's internal ClipRRect(12) is removed (in this PR, commit 4176d1742) and re-added at the comment caller → net-zero for comments; and the DM collaborator-invite card is visually redesigned (16px container clip, foreground border, edge-to-edge gradient). VideoCommentPlayer is shared by exactly these two callers, so bundling them in one PR is correct — the issue is purely the description.
Top asks (see inline):
- Rewrite the description to cover the invite-card redesign and attach a before/after screenshot of the card — the body currently calls it a "switch to
EdgeInsetsDirectional… for RTL." - Add a test/golden for
collaborator_invite_card.dart; its redesign is uncovered by CI today. - (Consider) keep the clip inside
VideoCommentPlayerbehind a radius param to avoid the encapsulation footgun that caused this very regression.
Description nits: the cited ancestor commit a95b8d78 doesn't exist in the repo; "7/7 pass" is actually 5/5 (I ran the file).
Housekeeping: branch is 1 commit behind origin/main (ff9356f0d, unrelated message_input_bar — no file overlap, clean rebase). Please rebase before merge.
Verified locally: flutter test test/screens/comments/comment_item_test.dart ✅ all pass; flutter analyze on all 4 changed files ✅ no issues; invite-card video still clipped via the container's clipBehavior: Clip.antiAlias.
Mobile PR PreviewPreview refreshed for Last refresh:
|
realmeylisdev
left a comment
There was a problem hiding this comment.
Re-review — approving ✅
Thanks for the quick turnaround on 16aab7b. Verified locally: comment_item_test.dart + the new collaborator_invite_card_test.dart both pass (9 tests total) and flutter analyze is clean on both.
Resolved from my review:
- ✅ Invite-card redesign is now documented in the PR body and covered by
collaborator_invite_card_test.dart(container clip @ 16, foreground border, sent/received alignment). - ✅ Fragile
ClipRRectfinder now guarded withfindsOneWidget. - ✅ Caller-owned clip design — agreed, and the new tests guard both call sites against the regression I flagged.
Correction on my end: I incorrectly said the before/after screenshots were the comments view — they're the invite card, and they show the redesign clearly (inset gradient panel → edge-to-edge). Apologies for that.
Optional, non-blocking (won't hold the merge):
- PR body still cites "follow-up to commit
a95b8d78" — that SHA isn't in the branch; worth dropping or correcting. - Verification line says
comment_item_test.dart → 7/7; it's 5 tests. - Please rebase onto
origin/mainbefore merge (1 commit behindff9356f0d, no conflicts).
Nice work — approving.
Closes #4707
Description
A previous refactor of
VideoCommentPlayerremoved its outerClipRRect, which inadvertently dropped the rounded corners on inline video previews inside comment items. This PR restores the rounded corners at the caller side and includes a visual redesign ofCollaboratorInviteCard._CardChrome.comment_item.dart: wrapVideoCommentPlayerinClipRRect(borderRadius: BorderRadius.circular(12))inside the existingAlign+ConstrainedBox(maxWidth: 248), restoring the previous visual.collaborator_invite_card.dart:BoxDecorationnow includesborderRadius: BorderRadius.circular(16)so the fill is rounded.decorationtoforegroundDecoration— it now paints on top of the content, which means the border is never obscured by video content bleeding to the edge.clipBehavior: Clip.antiAliason the outerContainer— clips the video player (and all other children) to the 16 px rounded corners so no content overflows the card boundary.EdgeInsetsGeometry.fromSTEBtoEdgeInsetsDirectional.fromSTEB(12, 48, 12, 12)so padding mirrors correctly in RTL locales.comment_item_test.dart: add a regression test (wraps the inline video player with rounded corners) that asserts theClipRRectancestor and its 12 px radius. SetsVisibilityDetectorController.instance.updateInterval = Duration.zeroinsetUpAllto prevent the visibility_detector timer from leaking into other tests (matches the existing pattern invideo_comment_player_test.dartandvisibility_aware_video_test.dart).collaborator_invite_card_test.dart: add regression tests for the_CardChromevisual redesign — assertsClip.antiAliasis set,foregroundDecorationhas a border, and sent/received cards align to the correct end of the row.Related Issue: N/A — follow-up to commit
a95b8d78on this branch.Out of Scope
None.
Verification
flutter test test/screens/comments/comment_item_test.dart→ 7/7 passflutter test test/screens/inbox/conversation/widgets/collaborator_invite_card_test.dart→ 4/4 passflutter analyze lib test integration_testType of Change