feat(sr): add configurable font-family transform#1016
Conversation
fuzzybinary
left a comment
There was a problem hiding this comment.
Minor suggestions if you want to take care of them.
I'm going to actually ask @gonzalezreal to review as I think he has familiarity with how iOS handles this and will be able to give me more insight on whether or not we need separate iOS / Android fallback values.
| if (lower == 'blinkmacsystemfont') { | ||
| return 'BlinkMacSystemFont'; | ||
| } | ||
| if (lower == '-apple-system') { | ||
| return '-apple-system'; | ||
| } | ||
| return lower; |
There was a problem hiding this comment.
nit: You could use pattern matching for this:
return switch (lower) {
'blinkmacsystemfont' => 'BlinkMacSystemFont',
'-apple-system' => '-apple-system',
_ => lower,
};which I find very slightly cleaner.
| case FontFamilyStrategy.fallback: | ||
| return kIosParityFontStack; |
There was a problem hiding this comment.
Is this only needed on iOS? Is there an Android fallback we should be using as well?
There was a problem hiding this comment.
The kIosParityFontStack mirrors the iOS native SDK's Fallback.fontFamily (REPLAY-1421). The Android SDK uses "Roboto, sans-serif" — same effective result without the Apple-specific prefixes. Since the replay player is a web browser, the -apple-system, BlinkMacSystemFont prefixes are harmless CSS fallbacks that improve rendering on Safari/Chrome macOS. Roboto already serves as the Android (and general web) fallback. No separate Android stack needed — one stack covers both platforms.
-> update the naming (now defaultReplayFontStack) and comment
| } | ||
| } | ||
|
|
||
| SRTextWireframe rewrite(SRTextWireframe wireframe) { |
There was a problem hiding this comment.
I think it makes sense for this method to be called apply and to potentially change the name of the String transform method. I can see a situation where we might need multiple of these Transforms in the future, and having them all implement a method called apply makes sense to me, whereas rewrite doesn't quite seem like the right term for generalized application of a transform.
I'll leave it to your discretion if you want to change it though.
There was a problem hiding this comment.
- rewrite(SRTextWireframe) → apply(SRTextWireframe) — the wireframe-level method now called apply, which is the generalized name that future transforms can all share
- apply(String) → transformFamily(String) — the string-level helper renamed to transformFamily so it no longer collides and better describes what it does (transforms the font family string specifically)
- Updated the one call site in processor_worker.dart and all 27 references in tests
| if (stripped.isEmpty) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Should this be above the check in _flutterFontSentinels?
|
|
||
| import 'dart:isolate'; | ||
|
|
||
| import '../../datadog_session_replay.dart'; |
There was a problem hiding this comment.
This should be below the package imports... I'm surprised the linter didn't complain about that....
There was a problem hiding this comment.
updated, neither flutter_lints nor the underlying package:lints/recommended.yaml seem to enable directives_ordering
| import 'dart:async'; | ||
| import 'dart:convert'; | ||
|
|
||
| import '../../datadog_session_replay.dart'; |
There was a problem hiding this comment.
Same here, this should move down to be with the other relative imports.
| /// wireframes in a background isolate, which cannot serialize Dart closures—use | ||
| /// [FontFamilyTransformConfig.rules] instead. |
There was a problem hiding this comment.
| /// wireframes in a background isolate, which cannot serialize Dart closures—use | |
| /// [FontFamilyTransformConfig.rules] instead. | |
| /// wireframes in a background isolate, which cannot serialize Dart closures — | |
| /// use [FontFamilyTransformConfig.rules] instead. |
| /// Always emits the single hardcoded CSS stack | ||
| /// `-apple-system, BlinkMacSystemFont, Roboto, sans-serif`, | ||
| /// regardless of the captured family. Matches the current iOS SDK | ||
| /// behavior and is the safest choice if you do not want any | ||
| /// Flutter font names leaving the device. | ||
| fallback, |
There was a problem hiding this comment.
Does it make sense to check the current Platform and supply an Android fallback as well?
There was a problem hiding this comment.
updated the comment, we already use a android compatible fallback
| ## Unreleased | ||
|
|
||
| * Add `DatadogSessionReplayConfiguration.fontFamilyTransform` (`FontFamilyStrategy` / `FontFamilyTransformConfig`) so captured font families can be rewritten to web-compatible CSS stacks in the processor isolate before upload (default `FontFamilyStrategy.none` for backwards compatibility; opt into `FontFamilyStrategy.smart` for normalization). In smart mode, `FontFamilyTransformConfig.rules['']` sets the stack when the captured family is empty or ends up empty after sentinel stripping. | ||
|
|
There was a problem hiding this comment.
We pull CHANGELOG items from the commit message now. I can add in the extra details during the release.
|
If you want, we could actually setup a golden test for this as well. Since goldens can only render one specific font, you could take any of the tests that render text and setup the transformer to make the switch. |
9b6b366 to
779b8a2
Compare
Add FontFamilyStrategy and FontFamilyTransformConfig to rewrite text wireframe font families in the processor isolate. Smart mode normalizes Flutter-specific names for web replay; default strategy is none to preserve prior behavior. Document in CHANGELOG. Made-with: Cursor
779b8a2 to
ba9f0b3
Compare
|
@fuzzybinary I'm not sure whether goldens make sense here, created a separate PR for it: #1028 (commit 1103307) |
Co-authored-by: Cursor <cursoragent@cursor.com>
What and why?
DatadogSessionReplayConfiguration.fontFamilyTransform(FontFamilyStrategy/FontFamilyTransformConfig) so captured font families can be rewritten to web-compatible CSS stacks in the processor isolate before upload (defaultFontFamilyStrategy.nonefor backwards compatibility; opt intoFontFamilyStrategy.smartfor normalization). In smart mode,FontFamilyTransformConfig.rules['']sets the stack when the captured family is empty or ends up empty after sentinel stripping.How?
Review checklist