[No QA] VictoryChart: strict types and parsing - #92960
Conversation
|
Conflicts |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
| const categories = parseArrayAttribute<RawChartData>(tnode.attributes.data).filter(isNonNullObject<RawChartData>); | ||
| const colorScale = parseArrayAttribute<Color>(tnode.attributes.colorscale); | ||
| const categories = parseRawChartData(tnode.attributes.data); | ||
| const colorScale = parseAttributeAsStringArray(tnode.attributes.colorscale); |
There was a problem hiding this comment.
Skia's Color accepts numbers, and the old path kept them. Now any numeric entry in colorscale is filtered out, so that slice falls back to VictoryTheme.colors.default.
Is colorscale always hex strings in practice?
There was a problem hiding this comment.
The backend is sending hex
| function parseAttributeAsStringArray(attribute: string): string[] | undefined { | ||
| const parsedValue = parseAttribute(attribute); | ||
| if (Array.isArray(parsedValue)) { | ||
| return parsedValue.filter((element): element is string => typeof element === 'string'); |
There was a problem hiding this comment.
String-array attributes with bare-number entries are dropped.
i.e.
- pie
labels (VictoryChartPie):labels="[1, 2, 3]"→[]/undefined→customLabelfalls back toslice.label. - axis
tickformat(victoryAxisParser): numerictickformatentries dropped →formatLabelfalls back toString(label).
Just confirming we don't have any non-strings for these kinds of properties.
There was a problem hiding this comment.
Right now, labels nor tickformat should always be strings. We might want to update this if that changes.
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.4.2-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: no changes required This PR is an internal code-safety refactor of the VictoryChart renderer — it removes unsafe type assertions and replaces them with runtime type checks when parsing chart node attributes ( There is no user-facing behavior, UI, feature, or workflow change:
Because nothing in the public Expensify help site describes this internal parsing/rendering logic, no help site article needs to be created or updated, and no draft docs PR was opened. @s77rt, if you believe a user-facing surface is affected that I may have missed, let me know and I'll re-evaluate. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.4.4-1 🚀
Bundle Size Analysis (Sentry): |
Help site reviewNo help site changes are required for this PR. This PR ( The change is entirely in the parsing/rendering layer:
There is no change to any user-facing feature, UI label, tab, setting, button, or workflow — and no behavioral change a customer would observe (charts render identically; the only difference is bad input now fails safely instead of crashing). The articles under Because no documentation update is needed, I did not create a draft help site PR. @s77rt, please confirm you agree no help site updates are needed. If you believe a customer-facing behavior here should be documented, let me know and I'll draft the PR. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.4-3 🚀
|
Explanation of Change
Currently when parsing VictoryChart nodes, some inputs are used as is and we used type assertion to pass them to the underlaying victory native component. However this is not safe as bad data types can still be passed as if it was valid and that may lead to a crash.
This PR fixes that by removing all the used type assertions and instead peform runtime type check.
Fixed Issues
$ #91776
PROPOSAL:
Tests
Offline tests
n/a
QA Steps
n/a
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari