Repository navigation
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🤖 Android test steps verification results: ✅ pass After seeding a workspace with 6 varied expenses and submitting the report, Concierge's "Top categories" request rendered a real inline donut chart. The chart survived two full background/resume cycles — both in the expanded Details modal and back in the inline chat view — with content fully painted, confirming the Skia-canvas-on-resume fix. No redbox or crash was observed at any point. Two caveats worth your attention,
|
|
🤖 Web test steps verification results: Concierge chat opens and "Top categories" produces a fully-rendered inline donut chart that expands correctly to a detail view. The background/foreground page-visibility check and the console-log capture could not be executed, because the web automation surface for this session exposes no tab/visibility/blur verb and no console-log verb. Three things worth your attention,
The web account had no expense data, so I seeded a workspace with 6 expenses across distinct categories to get a chart to render. The prestart onboarding replay also failed ( |
|
@MelvinBot pull main |
…ountOnForeground Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
I merged the latest |
|
This doesn't fix the root cause. This rebuilds every mounted Victory chart and its GL surface on every resume. I am going with my own solution which fixes the real root cause - #103398. |










Explanation of Change
Draft PR requested by
situchanto validate the remount solution proposed on the issue. Not verified on a device by Melvin — see below.Android destroys the
TextureViewsurface backing a SkiaCanvaswhen the activity is stopped. On resume the surface is recreated, but nothing pushes a new picture into it, so the canvas comes back blank. Chart props are static, so React never re-renders and the canvas is never asked to repaint. Upstream bug: [android] Canvas disappear in Modal when app is resumed from background (open since January 2024).This adds the same workaround the App already uses for Lottie, which breaks the same way (
src/components/Lottie/index.tsx:108-128): a newuseSkiaCanvasRemountKeyhook returns a value that changes on everybackground → activetransition, passed as akeyso the chart subtree unmounts and remounts against a live surface.useSkiaCanvasRemountKey/index.native.tswatchesuseAppStateand bumps a counter on resume.useSkiaCanvasRemountKey/index.ts(web) returns a constant, so web behavior is unchanged.keyis applied insideVictoryChartContentandVictoryChartInteractiveContent. Those two components are the only entry points that mount a chart, so this covers all three call sites — the inline chart, the fluid chart inVictoryChartExpandModal, and the sized chart inExpandedChartBox— without touching each parent.Two deliberate choices worth a reviewer's attention:
backgroundtransition counts, notinactive. iOS reports a transientinactivestate for things like pulling down the notification shade; remounting on those would churn the canvas for no reason.VictoryChartExpandable. Keying higher up would remountVictoryChartExpandableitself and reset itsisExpandedstate, closing an open expand modal on resume.Remounting is cheap here: the chart model is re-derived from the report action HTML on every render anyway (
BaseVictoryChartRenderer.tsx:23), and the Skia font cache is module-level so no typefaces are reloaded.What Melvin did not verify: this bug is Android-app-only, and the web test session for this run failed during onboarding, so no App-level verification ran at all. The remount hypothesis itself is unconfirmed on a device — that is what this PR is for.
AI Tests
Run locally:
npx eslint <changed files>— passednpm run typecheck— passednpm test -- --testPathPattern "(chart|Chart|Victory)"— passed, 23 suites / 307 tests (includes 4 new tests foruseSkiaCanvasRemountKey)npm run react-compiler-compliance-check -- check <changed files>— passed, all files compile under both Babel and OXC with no memoization divergencenpm run spell-changed— passedCould not run:
app-web-testerverification of chart rendering — the handed-off web session was stuck on a blank page after an onboarding replay failure, so the Concierge chart flow was never reachable.Fixed Issues
$ #101396
PROPOSAL: #101396 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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