feat(session-replay): add configurable font-family transform#1016
feat(session-replay): add configurable font-family transform#1016nottmey wants to merge 1 commit intoDataDog:developfrom
Conversation
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
nottmey
changed the title
fuzzybinary
left a comment
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?
| } | ||
| } | ||
|
|
||
| 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.
| 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....
| 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?
| ## 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. |
3 participants
What and why?
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.
How?
Review checklist