fix(ui): enrich link previews for uppercase URL schemes#2843
Conversation
📝 WalkthroughWalkthroughThe message composer normalizes URL schemes before link-preview enrichment, selects the first eligible URL, clears invalid OG attachments, and adds widget tests for URL casing and embedded links. ChangesURL enrichment normalization
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
daef717 to
27915b8
Compare
xsahil03x
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/stream_chat_flutter/test/src/message_input/message_composer_url_enrichment_test.dart (2)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid the relative
fakes.dartimport.The documented exception covers
../mocks.dart, not../fakes.dart. Use a package import or add an explicit test-helper exception if this file must remain test-only. As per coding guidelines,packages/**/{lib,test}/**/*.dart: Use package imports instead of relative imports.🤖 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 `@packages/stream_chat_flutter/test/src/message_input/message_composer_url_enrichment_test.dart` at line 9, Replace the relative ../fakes.dart import in the message composer URL enrichment test with the appropriate package import, following the package’s existing import conventions; only add an explicit test-helper exception if a package import is not viable.Sources: Coding guidelines, Learnings
51-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse fake test time for the debounce.
runAsyncis intended for real asynchronous work; this test only needs timer advancement. Pumping 500 ms avoids wall-clock delay and reduces timing sensitivity. (api.flutter.dev)Proposed change
- // Enrichment runs behind a real-clock debounce, so drive the flow with - // real timers via runAsync. - await tester.runAsync(() async { - await tester.pumpWidget( - MaterialApp( - home: StreamChat( - client: client, - connectivityStream: Stream.value([ConnectivityResult.mobile]), - child: StreamChannel( - channel: channel, - child: Scaffold(body: StreamMessageComposer()), - ), - ), - ), - ); - await tester.pumpAndSettle(); - - await tester.enterText(find.byType(TextField), text); - await Future<void>.delayed(const Duration(milliseconds: 500)); - await tester.pumpAndSettle(); - }); + await tester.pumpWidget( + MaterialApp( + home: StreamChat( + client: client, + connectivityStream: Stream.value([ConnectivityResult.mobile]), + child: StreamChannel( + channel: channel, + child: Scaffold(body: StreamMessageComposer()), + ), + ), + ), + ); + await tester.pumpAndSettle(); + await tester.enterText(find.byType(TextField), text); + await tester.pump(const Duration(milliseconds: 500)); + await tester.pumpAndSettle();🤖 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 `@packages/stream_chat_flutter/test/src/message_input/message_composer_url_enrichment_test.dart` around lines 51 - 72, Update enrichUrlFrom to use Flutter’s fake test clock for the debounce instead of tester.runAsync and a real Future.delayed. After entering the text, advance the test duration by 500 milliseconds with the tester’s pump mechanism, preserving the existing widget setup and settling behavior.Source: MCP tools
🤖 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.
Nitpick comments:
In
`@packages/stream_chat_flutter/test/src/message_input/message_composer_url_enrichment_test.dart`:
- Line 9: Replace the relative ../fakes.dart import in the message composer URL
enrichment test with the appropriate package import, following the package’s
existing import conventions; only add an explicit test-helper exception if a
package import is not viable.
- Around line 51-72: Update enrichUrlFrom to use Flutter’s fake test clock for
the debounce instead of tester.runAsync and a real Future.delayed. After
entering the text, advance the test duration by 500 milliseconds with the
tester’s pump mechanism, preserving the existing widget setup and settling
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf9d5fa9-45d0-4568-952a-76d4fb451bf0
📒 Files selected for processing (3)
packages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/src/message_input/stream_message_composer.dartpackages/stream_chat_flutter/test/src/message_input/message_composer_url_enrichment_test.dart
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2843 +/- ##
==========================================
+ Coverage 71.48% 71.71% +0.22%
==========================================
Files 426 426
Lines 26892 26891 -1
==========================================
+ Hits 19224 19285 +61
+ Misses 7668 7606 -62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Uppercase schemes like `HTTPS://` were detected but forwarded verbatim to the enrichment endpoint, which failed to return link-preview data. Normalize the scheme before enriching so `HTTPS://` behaves like `https://`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
27915b8 to
c9d2e5d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/stream_chat_flutter/test/src/message_input/message_composer_url_enrichment_test.dart (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a package import for
fakes.dart.The relative-import exception applies to
../mocks.dart, not../fakes.dart. As per coding guidelines, “Use package imports instead of relative imports.” Based on learnings, only the sharedmocks.darthelper is exempt.🤖 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 `@packages/stream_chat_flutter/test/src/message_input/message_composer_url_enrichment_test.dart` at line 9, Update the import in message composer URL enrichment tests to use the package-qualified path for fakes.dart instead of a relative import, while preserving the existing ../mocks.dart exception if present.Sources: Coding guidelines, Learnings
🤖 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.
Nitpick comments:
In
`@packages/stream_chat_flutter/test/src/message_input/message_composer_url_enrichment_test.dart`:
- Line 9: Update the import in message composer URL enrichment tests to use the
package-qualified path for fakes.dart instead of a relative import, while
preserving the existing ../mocks.dart exception if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15b5728b-5f74-4818-b7ed-cc3a0311f2d2
📒 Files selected for processing (3)
packages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/src/message_input/stream_message_composer.dartpackages/stream_chat_flutter/test/src/message_input/message_composer_url_enrichment_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_chat_flutter/CHANGELOG.md
| }).toList(); | ||
| // Find the first url to preview, normalizing the scheme so links like | ||
| // `HTTPS://` enrich the same as `https://`. | ||
| String? firstMatchedUrl; |
There was a problem hiding this comment.
Question: Wouldn't it be possible to end up without an enriched URL, it the /og endpoint doesn't return in time before sending the message, or if we send the message before this method fires (350ms debounce).
I am curious if maybe the bug report was for that reason 🤔 Because I can see some messages getting enriched even with HTTPS:// prefix
There was a problem hiding this comment.
Yes, it's a possibility. But when i tried reproducing it in the sample app i wasn't able to get any og for a link with uppercase HTTPS://. One way to solve it is to use url_enrichment flag on channel but it will still fail as the backend extracts the url from text and the text will still be with Uppercase letters. So this also needs to be fixed in the backend code in order to work correctly.
There was a problem hiding this comment.
I see, I was afraid that we would still see reports of this issue after the fix. But nevertheless, I see no reason to block this PR. But we should probably bring this up with the BE as well.
4 participants
Summary
Fixes FLU-625. URLs with an uppercase scheme (e.g.
HTTPS://example.com) were not enriched into link previews, while lowercasehttps://worked.The composer's URL regex already matched uppercase schemes, but the raw matched text was forwarded to
client.enrichUrl(...)verbatim — so the/ogendpoint receivedHTTPS://…and failed to enrich. The fix normalizes the scheme (viaUri.parse, which lowercases it) before enriching, soHTTPS://behaves the same ashttps://.Flutter counterpart to the React (#3226) and Angular (#734) fixes.
Scope
This fixes the composer link-preview enrichment path. Display-side linkification of uppercase schemes in rendered message text lives in
stream_core_flutterand is out of scope here.Testing
Added
message_composer_url_enrichment_test.dart— assertsenrichUrlis called with the normalizedhttps://example.comfor both uppercase and lowercase input. Fails before the fix, passes after.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
HTTPS://) by normalizing the scheme before enrichment.Tests