Skip to content

feat: render expanded chart at native resolution and add pinch-zoom via MultiGestureCanvas - #97698

Open
abbasifaizan70 wants to merge 31 commits into
Expensify:mainfrom
abbasifaizan70:92969-followup-zoom
Open

feat: render expanded chart at native resolution and add pinch-zoom via MultiGestureCanvas#97698
abbasifaizan70 wants to merge 31 commits into
Expensify:mainfrom
abbasifaizan70:92969-followup-zoom

Conversation

@abbasifaizan70

@abbasifaizan70 abbasifaizan70 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Re-renders the expanded chart natively at its target size (via a scaled chart context) instead of raster-upscaling the design-size render, so charts stay sharp on large screens.
Adds pinch/double-tap zoom and pan to the expanded chart using MultiGestureCanvas — the same gestures as the image attachment viewer, as agreed in the follow-up discussion on #95249.
Includes unit tests for the new context-scaling logic.

Fixed Issues

$ #92969
PROPOSAL:#92969 (comment)

Tests

  • Verify that no errors appear in the JS console
  1. Tap the expand button on the inline chart to open the full-screen modal.
  2. Verify the expanded chart is sharp on a large screen (text, axis labels, and lines are crisp, not blurry) and matches the inline chart's layout, only larger.
  3. Double-tap (or double-click) the chart and verify it zooms in; double-tap again to zoom back out.
  4. Pinch (mobile/trackpad) and verify the chart zooms smoothly within limits.
  5. While zoomed in, drag to pan around the chart and verify it stays within bounds.
  6. Close the modal and verify the chat is unchanged and no flicker appears.
  7. Repeat with a pie chart and verify zoom/pan work and the chart stays clipped/centered like inline.

Offline tests

Same as tests.

QA Steps

  • Verify that no errors appear in the JS console

Same as tests.

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Screen_Recording_20260825_014636_New.Expensify.Dev.mp4
Android: mWeb Chrome
Screen_Recording_20260825_015755_Chrome.mp4
iOS: Native
Screen.Recording.2026-08-25.at.12.38.37.AM.mov
iOS: mWeb Safari
Screen.Recording.2026-08-25.at.12.53.26.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-08-22.at.5.33.50.AM.mp4

@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
src/CONST/index.ts 91.52% <ø> (ø)
...ctoryChartRenderer/context/VictoryChartContext.tsx 96.00% <100.00%> (+90.44%) ⬆️
...yChartRenderer/utils/computeExpandedChartLayout.ts 100.00% <100.00%> (ø)
...er/utils/getStaticChartCanvasProps/index.native.ts 100.00% <100.00%> (ø)
...enderers/VictoryChartRenderer/utils/parseOffset.ts 100.00% <100.00%> (ø)
...enderers/VictoryChartRenderer/utils/scalePixels.ts 100.00% <100.00%> (ø)
src/components/MultiGestureCanvas/index.tsx 1.20% <ø> (ø)
src/styles/index.ts 63.86% <ø> (ø)
...ryChartRenderer/components/VictoryChartContent.tsx 11.11% <0.00%> (ø)
...Renderer/context/VictoryChartRenderArgsContext.tsx 25.00% <50.00%> (ø)
... and 19 more
... and 8 files with indirect coverage changes

@abbasifaizan70 abbasifaizan70 changed the title feat: render expanded chart at native resolution and add pinch-zoom v… feat: render expanded chart at native resolution and add pinch-zoom via MultiGestureCanvas Aug 3, 2026
@abbasifaizan70
abbasifaizan70 marked this pull request as ready for review August 4, 2026 16:23
@abbasifaizan70
abbasifaizan70 requested review from a team as code owners August 4, 2026 16:23
@melvin-bot
melvin-bot Bot requested a review from situchan August 4, 2026 16:23
@melvin-bot

melvin-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

@situchan Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 24d931aac7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@abbasifaizan70
abbasifaizan70 requested a review from a team as a code owner August 5, 2026 00:33
@luacmartins
luacmartins self-requested a review August 6, 2026 20:54
@situchan

situchan commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari

@luacmartins

Copy link
Copy Markdown
Contributor

@situchan how's review going?

@quinthar quinthar added the #ai label Aug 11, 2026
@luacmartins

Copy link
Copy Markdown
Contributor

@situchan any updates on the review?

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@situchan Fixed the above-mentioned issues; here videos.

Screen.Recording.2026-09-03.at.3.49.28.AM.mov

Screen.Recording.2026-09-03.at.3.52.11.AM.mov

@situchan

situchan commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Lint failing

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a7a3104b5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/hooks/useClickZoomPan.ts Outdated
@MelvinBot

Copy link
Copy Markdown
Contributor

Review

Both issues from my last two comments are fixed — zoomRange is now capped to the render headroom, and the card radius is scaled through scalePixels. Two things left, one of them the lint failure situchan flagged.

1. ESLint — 8 errors (blocker)

The failing run breaks down into three groups:

Alias-for-sibling import (@dword-design/import-alias/prefer-alias) — both files live in utils/, so the import must be ./scalePixels:

no-param-reassign on the new hook — useClickZoomPan.ts:145-146 and :158-159. This is the extraction's fault: the same writes were legal in ImageView because scrollableRef was a local, and became a violation the moment it turned into a hook parameter. Read it into a local first (const scrollable = scrollableRef.current;) and write through that.

rulesdir/no-raw-typography on test fixture data — scaleVictoryChartContextValueTest.ts:20 and :37. The rule is firing on arbitrary numbers in a fixture, not on real styling — a scoped eslint-disable-next-line with a reason is the right call here.

2. Double-tap still upscales past the rendered resolution on touch

zoomRange={{max: zoomHeadroom}} at BaseVictoryChartExpandedContent.tsx:34 only constrains pinch. useTapGestures never receives zoomRange — it computes its own doubleTapScale = Math.max(DOUBLE_TAP_SCALE, maxContentScale / minContentScale), floored at 3.

Because contentSize is the design box × fitScale × zoomHeadroom and the canvas is the same area fitScale was derived from, minContentScale works out to exactly 1 / zoomHeadroom. With the default MAX_ZOOM_HEADROOM = 2 that makes double-tap land at totalScale = 3 × 1/2 = 1.5 — a 1.5× raster upscale of the canvas, which is the blur this PR set out to remove. Pinching afterwards then springs back down to 2 (usePinchGesture.ts:197), so the two gestures also disagree on the ceiling.

Simplest fix: set MAX_ZOOM_HEADROOM to DOUBLE_TAP_SCALE (3). Then double-tap lands at exactly 1.0 of the rendered pixels, pinch's cap agrees, and MAX_CANVAS_DIMENSION still bounds the surface. The alternative is clamping doubleTapScale to zoomRange.max in useTapGestures, but that touches a shared component.

Nit and testing note

useExpandedChartLayout runs twice per render — once in VictoryChartExpandModal.tsx:43 for hasLayout/backgroundColor/borderRadius, and again inside whichever expanded-content variant renders. Passing the layout down as a prop would drop the duplicate theme and context reads.

I also tried to verify the three bugs situchan reported (blank content before close, zoom flicker, browser back closing the report) on web, but the test account could not produce an inline chart — Concierge returned a text summary plus an "Insight" link for every phrasing, both before and after adding real expense data. Those three still need manual confirmation.

Everything else looks right: the inline path is untouched at pixelScale === 1, ImageView behaviour survives the hook extraction (offsetX / zoomScale became offsetX * (1 / zoomScale), plus a guard for the not-yet-loaded case), and the desktop click-zoom maps the clicked point correctly now that centering is dropped while zoomed.


Next Steps: Reply with @MelvinBot <your feedback> to refine this review.

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

Lint and code feedbacks are fixed. @situchan

@situchan

situchan commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Please pull main

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e1bc2494f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Regression review — one regression found in the expand-modal refactor.

The expanded chart's Skia canvas is never unmounted anymore

Before this PR, both render branches of the expand modal were gated on isVisible, with a comment saying exactly why:

The Skia canvas is removed as soon as closing starts: WebGL canvases can flash white when re-composited during the close animation (visible on dark themes).

That guard is gone on every path:

Two consequences:

  1. The white flash on close is unprotected again — the exact failure the deleted comment describes, on dark themes.
  2. The canvas leaks past close, and it is now much bigger. VictoryChartExpandable latches shouldRenderModal to true permanently (VictoryChartExpandable.tsx:70), so after the user closes the modal the chart keeps rendering. Pre-PR that cost nothing (the guard unmounted it); now it holds a canvas rendered at up to MAX_ZOOM_HEADROOM = DOUBLE_TAP_SCALE = 3× the fitted size, capped at 2048 px — up to ~9× the pixel area of the old design-size render, for every chart the user has ever expanded in that report.

Fix: restore the gate. Pass isVisible into ExpandedChartBox (or wrap it in {isVisible && …}) in both BaseVictoryChartExpandedContent and DesktopVictoryChartExpandedContent, and put {isVisible && <VictoryChartContent />} back in the fallback branch.

Worth measuring before merge

The 3× render is a deliberate sharpness tradeoff, but it is unmeasured here. At DPR 2–3 a 2048 px-wide chart canvas has a backing store in the tens of MB, allocated on open whether or not the user zooms, and (per the bug above) retained afterwards. Please check memory and open latency on a low-end Android device with a couple of charts expanded — useExpandedChartLayout.ts:14-18.

What I checked that came back clean
  • Inline charts are unaffected. pixelScale is 1 on the inline path, and every new call site degrades to identity (scalePixels(x, 1) === x, parseCornerRadius(attr, 1), parseOffset(…, 1)). No inline behavior change.
  • The ImageView extraction into useClickZoomPan is behavior-preserving. offsetX / zoomScale became offsetX * zoomFactor with zoomFactor = 1 / zoomScale — identical, and the new zoomScale > 0 guard removes a divide-by-zero that used to produce Infinity scroll offsets before the image loaded. trackPointerPosition's instanceof Node check matches the old contains(target as Node) semantics. resetZoom additionally clears zoomDelta, which is a strict improvement.
  • Dropping useCallback is safe. npm run react-compiler-compliance-check check passes on useClickZoomPan.ts and the new expanded-content files, so both compilers memoize them — the mousemove/mouseup listeners are not re-subscribed every render.
  • Expanded-chart tooltips were not lost. The expand modal renders VictoryChartContent, not VictoryChartInteractiveContent, both before and after — bar hover/press was already absent there.
  • No orphaned styles. styles.chartExpandedContent has no remaining references after its removal.
  • shouldHandleNavigationBack is a real Modal prop (Modal/types.ts:73).
  • Tests: all 8 suites under tests/unit/components/HTMLEngineProvider pass locally (94 tests), including the two new ones. Despite their non-standard *Test.ts filenames they do match Jest's tests/unit/**/*.ts?(x) pattern, so they really run. CI is green apart from the checklist and independent-approval checks.
Runtime verification was blocked (no web test run)

I could not exercise either changed surface in the browser:

  • Image attachment viewer — reaching it needs a file upload. agent-device --platform web has no upload primitive (the OS file picker is not drivable), pasting an image URL posts a plain link rather than an attachment, and the fresh test account has no pre-existing image attachments to open instead.
  • Expanded chart — needs a chart-bearing message in the account, which the test account does not have.

So the findings above are code-level only. The white-flash regression in particular should be confirmed by eye: open a chart in dark mode, expand it, and close it.


Next Steps: Reply with @MelvinBot <your feedback> to refine this review.

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@situchan Updated with latest main branch.

@luacmartins

Copy link
Copy Markdown
Contributor

@situchan bump for review

@situchan situchan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: modal background flashes

You can see white briefly when closing modal

Screen.Recording.2026-09-10.at.7.13.55.AM.mov

Let's target to get this shipped upon one-time review.
So please make sure to perform full test before requesting re-review. Every time request review, another bugs happen.
Do not purely rely on code generated by Claude.

@MelvinBot

MelvinBot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

🤖 Regression + code-quality review — one blocker, one touch-device defect.

1. Blocker: the expanded chart's Skia canvas is still never unmounted

This is the same regression I flagged on 2026-09-08 and it is unchanged at bbb7508 — the branch has only been merged with main since.

Before this PR both render branches were gated on isVisible, with a comment saying why: "The Skia canvas is removed as soon as closing starts: WebGL canvases can flash white when re-composited during the close animation (visible on dark themes)." That gate is gone everywhere:

VictoryChartExpandable latches shouldRenderModal to true permanently (VictoryChartExpandable.tsx:70), so every chart the user has ever expanded in a report keeps a live canvas after close — and it is now up to 3× the fitted size, so roughly 9× the pixel area of the old design-size render. The white flash on dark themes is also unprotected again.

Fix: wrap ExpandedChartBox in {isVisible && …} in both content components, and restore {isVisible && <VictoryChartContent />} in the fallback branch.

2. Double-tap still overshoots the rendered resolution on tablets

Setting MAX_ZOOM_HEADROOM = DOUBLE_TAP_SCALE fixes the phone case, but zoomHeadroom collapses below 3 as soon as the fitted chart's long side exceeds ~683 px (useExpandedChartLayout.ts:70MAX_CANVAS_DIMENSION / max(targetWidth, targetHeight)). That is every tablet in landscape.

useTapGestures floors doubleTapScale at 3 regardless of zoomRange (useTapGestures.ts:52), and getCanvasFitScale clamps the fit scale to ≤ 1 (utils.ts:17-21), so minContentScale works out to 1.001 / zoomHeadroom and double-tap lands at a total scale of 3 / zoomHeadroom. On a 12.9" iPad that is ≈ 1.9× — the raster upscale this PR exists to remove. It is also above the pinch cap of 1.0 set by zoomRange={{max: zoomHeadroom}}, so the next pinch snaps the chart back down.

Fix: clamp the double tap to the render headroom — either honour zoomRange.max in useTapGestures, or don't let zoomHeadroom fall below DOUBLE_TAP_SCALE on the touch path.

3. Code quality

  • Skia fonts are rebuilt and leaked on every layout change. scaleFont allocates a new Skia.Font per axis and nothing disposes the old one. On desktop web providerScale changes on every window resize, so resizing a report with an open chart allocates fonts indefinitely.
  • useExpandedChartLayout still runs twice per renderVictoryChartExpandModal.tsx:42 plus again inside whichever content variant renders — and hasLayout is checked in three places, two of which are unreachable. Pass the layout down as a prop instead.
  • useClickZoomPan is a web-only hook without the web-only file convention. It lives at src/hooks/useClickZoomPan.ts with no platform suffix, but references document and types its ref as View & HTMLDivElement. Nothing stops a native file importing it; only a runtime canUseTouchScreen() check keeps it inert. Split it into index.ts / index.native.ts.
  • Border-radius scaling is split across two owners. The context carries pixelScale and scaleVictoryChartContextValue scales everything else, but chartContainerStyles.borderRadius is scaled by hand at ExpandedChartBox.tsx:47. Pick one.
  • zoomHeadroom has no unit test. It is the load-bearing number in this PR and the source of finding Fix spaces #2; the two new test files cover the easier half.
What I checked that came back clean
  • Inline charts are unaffected. pixelScale is 1 on the inline path and every new call site is the identity at 1 (scalePixels, parseCornerRadius, parseOffset).
  • The ImageViewuseClickZoomPan extraction is behavior-preserving. offsetX / zoomScale became offsetX * (1 / zoomScale) with a zoomScale > 0 guard that removes an Infinity scroll offset before the image loads. trackPointerPosition's instanceof Node check matches the old contains(target as Node) semantics.
  • scaleFont rebuilds with the right typeface. getVictoryChartTreeTypeface is the same typeface victoryAxisParser.ts:51 used to build the original axis fonts, so weights don't change when scaled.
  • The GestureHandlerRootView comment is accurateAttachmentModalBaseContent/index.tsx:340 wraps its modal content the same way.
  • No orphaned styleschartExpandedContent has no remaining references.
  • CI is green apart from checklist and Check independent approval.
Web testing — image viewer verified, chart still unreachable

The refactored useClickZoomPan path was exercised through the image preview (same AttachmentViewImage → ImageView → useClickZoomPan chain). Click-to-zoom and click-to-unzoom both work, and the unzoomed frame is byte-identical to the original fitted frame, so there is no drift or stuck state.

Step Status
Viewer opens with no console errorsFull-screen viewer rendered; network dump showed only 200s.
Image viewer fitted
Single click zooms in near the clicked pointClicked above/left of centre; framing shifted toward the clicked region.
Zoomed in after single click
Drag and scroll pan while zoomedUntestable: agent-device --platform web has no pan/drag primitive, and wheel scroll did not reach the nested overflow container. Needs a manual pass.
⚠️
Second click zooms back out to fitted sizePixel diff against the original fitted frame was empty.
Zoomed back out to fitted size
Expand an inline chart and zoomNot reachable — this test account has no chart message, same as the two previous runs.

So findings #1 and #2 are code-level only. The white flash in particular is worth a quick manual look: open a chart in dark mode, expand it, close it.


Next Steps: Reply with @MelvinBot <your feedback> to refine this review.


view run · no recording available

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ This PR is possibly changing native code and/or updating libraries, it may cause problems with HybridApp. Please check if any patch updates are required in the HybridApp repo and run an AdHoc build to verify that HybridApp will not break. Ask Contributor Plus for help if you are not sure how to handle this. ⚠️

@situchan

Copy link
Copy Markdown
Contributor

⚠️ This PR is possibly changing native code and/or updating libraries, it may cause problems with HybridApp. Please check if any patch updates are required in the HybridApp repo and run an AdHoc build to verify that HybridApp will not break. Ask Contributor Plus for help if you are not sure how to handle this. ⚠️

There's Podfile.lock change which seems unrelated

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@situchan Sorry about that — the Podfile.lock change was accidental. I ran pod install locally to fix an iOS build issue, which rewrote a few pod checksums, and it got staged along with the lint fix. There are no native changes in this PR; I've reverted the file to match main.

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@situchan Quick note on the new victory-native patch, since it's a judgment call I'd like your view on.

The two close bugs conflict: keeping the WebGL canvas mounted during the close animation causes the white flash, while unmounting it causes the blank card you reported. The clean fix is to render the expanded chart as a static bitmap via Skia's __destroyWebGLContextAfterRender, but victory-native doesn't expose its props — hence the small patch (a canvasProps passthrough, JS-only, no native changes).

If you'd rather avoid a patch, the alternatives are: (a) restore the isVisible gate and accept the brief blank card on close, or (b) make the close animation near-instant so neither shows. Both are compromises; the patch is the only one that fixes both properly. Happy to switch if you prefer one of those.

@situchan

Copy link
Copy Markdown
Contributor

Do you think the patch in #100714 fixes that bug or nothing related?

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@situchan I don't think so —
#100714 only changes WebGLRenderer.dispose(), which in Skia 2.4.18 runs from the [tick] effect cleanup, i.e. only on unmount (or an Activity/StrictMode re-run). During our close animation the canvas stays mounted and no cleanup runs, so that code path isn't hit. It fixes a permanent blank after the context is lost, while ours is a transient flash while a live WebGL canvas is composited during the fade.

@abbasifaizan70

abbasifaizan70 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

@situchan After adding the patch I rebuilt Android locally and re-tested everything end to end — double-tap now stops at the render cap, a small pinch after that no longer snaps back, swipe-down closes the modal, and closing shows no blank or flash. Demo videos attached below. If you still think any improvement, let me know i will take care for that.

Screen_Recording_20260911_182050_New.Expensify.Dev.mp4
Screen.Recording.2026-09-11.at.6.24.42_PM-compressed.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants