Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-09-26] Dev- Console warning whenever type in composer on Android and iOS Dev #48268

Closed
2 of 6 tasks
m-natarajan opened this issue Aug 29, 2024 · 20 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Needs Reproduction Reproducible steps needed Reviewing Has a PR in review

Comments

@m-natarajan
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number:
Reproducible in staging?: Issue occurs in dev
Reproducible in production?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @ishpaul777
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1724876364667229

Action Performed:

  1. Open Any chat and type something in composer

Expected Result:

No warnings or error message

Actual Result:

There's a warning

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
WhatsApp Image 2024-08-29 at 1 47 50 AM
WhatsApp Image 2024-08-29 at 1 50 07 AM

View all open jobs on GitHub

@m-natarajan m-natarajan added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@bernhardoj
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Console warning on every type on composer.

What is the root cause of that problem?

Based on the error message and the reanimated doc, we are trying to change the object current property after it's copied to a worklet function.

In our case, we are trying to set the ref here,

ReportActionComposeFocusManager.composerRef.current = el;
textInputRef.current = el;
if (typeof animatedRef === 'function') {
animatedRef(el);
}

and we also use it inside the clear function which is a worklet.

const clear = useCallback(() => {
'worklet';
forceClearInput(animatedRef, textInputRef);
}, [animatedRef]);

What changes do you think we should make in order to solve the problem?

The doc suggested using useSharedValue and if we look at the ReportActionCompose, we did that.

// Note: using JS refs is not well supported in reanimated, thus we need to store the function in a shared value
// useSharedValue on web doesn't support functions, so we need to wrap it in an object.
const composerRefShared = useSharedValue<{
clear: (() => void) | undefined;
}>({clear: undefined});
const handleSendMessage = useCallback(() => {
'worklet';
const clearComposer = composerRefShared.value.clear;

So, there are 2 options. Follow the pattern in ReportActionCompose by making a shared value to store the clear function and pass it to forceClearInput.

OR

No need to pass textInputRef to forceClearInput

const clear = useCallback(() => {
'worklet';
forceClearInput(animatedRef, textInputRef);
}, [animatedRef]);

and just use animatedRef.

function forceClearInput(_: AnimatedRef<Component>, textInputRef: React.RefObject<TextInput>) {
'worklet';
textInputRef.current?.clear();
}

animatedRef.current?.clear();

@perunt
Copy link
Contributor

perunt commented Aug 30, 2024

@bernhardoj we already store clear as a sharedValue. So in this case, the only option is to use animatedRef directly, as you pointed out in your second solution.
Since we attach the same ref from the composer to animatedRef, there should be no difference which one we use for clearing it.
It's important to note that for native platforms, useAnimatedRef would point to the regular .clear() function, which is not what we want.

@ishpaul777 , can I create an issue for the remaining errors that I'm encountering in the composer? I'm keen on making our new RN75 update more stable, as these issues somewhat block me from delivering this feature.

so we can use

function forceClearInput(animatedInputRef: AnimatedRef<Component>) {
    'worklet';

    const input = animatedInputRef.current;
    if (input && 'clear' in input && typeof input.clear === 'function') {
        input.clear();
    }
}

instead of

function forceClearInput(_: AnimatedRef<Component>, textInputRef: React.RefObject<TextInput>) {
    'worklet';

    textInputRef.current?.clear();
}

@ishpaul777
Copy link
Contributor

@ishpaul777 , can I create an issue for the remaining errors that I'm encountering in the composer? I'm keen on making our new RN75 update more stable, as these issues somewhat block me from delivering #42143.

Asked for the process for agency members to crete GH issues, will let you know once i got a reply

@mallenexpensify
Copy link
Contributor

@perunt plz follow the steps in the agency dev process doc. We wanna make sure there's 👀 before PRs are created.

For updates or fixes, first create a GitHub issue. Best practice is to post the issue link and details in #expensify-open-source, and cross-post in relevant rooms. If the issue is specific to a project, and relevant to a broader audience, share it in the room with the most visibility for that project (and skip posting in #expensify-open-source). Do not create PRs without an issue being created and reviewed, even for minor changes.

@m-natarajan
Copy link
Author

@miljakljajic Can you please assign this to @perunt as per this comment

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
@perunt
Copy link
Contributor

perunt commented Sep 2, 2024

posted it to #expensify-open-source
https://expensify.slack.com/archives/C01GTK53T8Q/p1725283671428529

Copy link

melvin-bot bot commented Sep 3, 2024

@miljakljajic Huh... This is 4 days overdue. Who can take care of this?

@perunt
Copy link
Contributor

perunt commented Sep 4, 2024

I discussed this in Slack, and there is no other idea so far.
I'll raise a PR with my fix

Copy link

melvin-bot bot commented Sep 8, 2024

Triggered auto assignment to @srikarparsi, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@miljakljajic miljakljajic removed the Bug Something is broken. Auto assigns a BugZero manager. label Sep 20, 2024
@miljakljajic miljakljajic removed their assignment Sep 20, 2024
@miljakljajic miljakljajic added the Bug Something is broken. Auto assigns a BugZero manager. label Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added the Daily KSv2 label Sep 20, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Sep 20, 2024
@miljakljajic
Copy link
Contributor

Heading out on parental leave so assigning to another BZ team member. Thank you @stephanieelliott

@perunt
Copy link
Contributor

perunt commented Sep 23, 2024

The PR with the fix has been merged, link

@stephanieelliott
Copy link
Contributor

PR was deployed to prod on 9/19 but it looks like the automation didn't work. Updating manually to add payment date

@stephanieelliott stephanieelliott added the Awaiting Payment Auto-added when associated PR is deployed to production label Sep 24, 2024
@stephanieelliott stephanieelliott changed the title Dev- Console warning whenever type in composer on Android and iOS Dev [HOLD for payment 2024-09-26] Dev- Console warning whenever type in composer on Android and iOS Dev Sep 24, 2024
@mananjadhav
Copy link
Collaborator

I don't have any offending PR for this one. There is no regression test as well as it's only dev related console.warn.

@stephanieelliott This is ready for payout.

Copy link

melvin-bot bot commented Sep 26, 2024

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

@mananjadhav
Copy link
Collaborator

I had helped with the PR Review, so I am eligible for a payout here.

@stephanieelliott
Copy link
Contributor

Summarizing payment on this issue:

  • Contributor: @perunt no payment due (agency)
  • Contributor+: @mananjadhav $250 please request on ND

No Upwork job for this

@garrettmknight
Copy link
Contributor

$250 approved fro @mananjadhav

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Needs Reproduction Reproducible steps needed Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests