Fix Report- Something when wrong when resize to fullscreen in bar or line view of expense report#87084
Conversation
|
@DylanDylann 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c976df66b
ℹ️ 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".
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Fix is good for product
|
hi @DylanDylann, how's the review coming along? |
|
On my list today |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@borys3kk I think this bug only reproduces on Windows; unfortunately, I don’t have it. Could you ask someone to verify it there? Otherwise, we should ask the QA team to test this PR using an ad-hoc build. |
I was also able to reproduce this bug on macOS so I think you're good to go |
|
@borys3kk The code change looks fine to me Could you update and add unit tests?
|
sure, on it |
|
@DylanDylann pushed newest changes with added tests 🚀 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
can't complete the review today, I will prioritise this PR in tomorrow |
|
@borys3kk I still get a crash Screen.Recording.2026-04-08.at.12.37.12.mov |
looking into it |
…f skia paragraph builder, cleanup
|
taking the pr back to draft for internal review, when that concludes I will ping you |
| * width actually changes — not when the parent passes a new array reference with the same strings. | ||
| */ | ||
| function useChartParagraphs(labels: string[], fontMgr: SkTypefaceFontProvider | null, fontSize: number, labelColor: string, layoutWidth: number): ParagraphWithWidth[] { | ||
| // Join into a single string so useMemo compares label content, not the array reference. |
There was a problem hiding this comment.
Do we actually need this labelsKey workaround?
If we memoize labels in the parent (so the array reference is stable when values don’t change), this hook could just depend on labels directly.
Maybe there's no need to do anything extra if the parent is compiled with React Compiler.
There was a problem hiding this comment.
yeah, I've tried it but the labels array is not stable itself and creates a new reference on every resize even though the values do not change
|
@DylanDylann pr is ready for your re-review 🚀 |
|
hello, @DylanDylann how's the review coming along? 🚀 |
Explanation of Change
Fixed chart crashes caused by excessive re-renders. Moved
activeDataIndexandisOverClickableTargetfrom React state to ReanimatedSharedValues so hover/cursor changes no longer trigger React re-renders. ExtractedChartTooltipLayerto read shared values directly. RefactoreduseChartLabelLayoutto return label widths only; truncation is now handled during render inChartXAxisLabels. AddeduseChartLabelMeasurements,useChartParagraphs, anduseYAxisLabelWidthhooks.Fixed Issues
$ #87066 (I was unable to reproduce the crash when resizing however I've noticed while testing locally that both charts are re-rendered on every mouse hover, chart point / bar enter etc, and this also seems to fix the something went wrong crash)
PROPOSAL:
Tests
Crash fix:
Offline tests
QA Steps
Same as tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Screen.Recording.2026-04-03.at.16.06.23.mov