Auto-save text written in DismissibleTextForm #6884

kumar303 · 2018-11-09T23:07:04Z

Fixes mozilla/addons#12400 by auto-saving text in DismissibleTextForm (which fixes mozilla/addons#12643).

I made two separate issues so that QA knows which scenarios to cover.

kumar303 · 2018-11-09T23:13:29Z

src/ui/components/DismissibleTextForm/index.js

@@ -211,14 +264,14 @@ export class DismissibleTextFormBase extends React.Component<
    return (
      <form className={makeClassName('DismissibleTextForm-form', className)}>
        <Textarea
-          defaultValue={this.state.text}


I honestly don't know how this worked before but it did. I read the react-textarea-autosize source twice. I have no idea! It no longer works with my new call to setState({ text: ... }) but I don't know what's different. In any case, it should have always been setting the value prop, not defaultValue.

That's an uncontrolled component, no? https://reactjs.org/docs/uncontrolled-components.html#default-values A weird one though, like a semi-controlled/semi-uncontrolled component 😄

Since you can supply any props to Textarea and therefore to the underlying textarea, that defaultValue prop is passed to the textarea and the textarea becomes an uncontrolled component. In theory, one would need to use a ref to read the textarea content but... the Textarea component implements an onChange handler on the underlying textarea.

According to the code, onChange is always called in Textarea: https://github.com/andreypopp/react-textarea-autosize/blob/f4f49e84c4beccf29188c72c2a3b21899bc0e85a/src/index.js#L113, so the onTextChange callback is also always called, and when it updates the state, the defaultValue gets updated too (because of the re-render).

The implementation of onChange in Textarea is not correct IMO, it should early return in if (!this._controlled) { ... }, so that developers would know that defaultValue and onChange don't play nice together.

codecov-io · 2018-11-09T23:24:40Z

Codecov Report

Merging #6884 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#6884      +/-   ##
==========================================
+ Coverage   97.85%   97.85%   +<.01%     
==========================================
  Files         250      250              
  Lines        6707     6726      +19     
  Branches     1267     1269       +2     
==========================================
+ Hits         6563     6582      +19     
  Misses        130      130              
  Partials       14       14

Impacted Files	Coverage Δ
src/amo/components/ReportUserAbuse/index.js	`100% <ø> (ø)`	⬆️
...rc/amo/components/EditableCollectionAddon/index.js	`100% <100%> (ø)`	⬆️
src/core/localState.js	`100% <100%> (ø)`	⬆️
src/ui/components/DismissibleTextForm/index.js	`100% <100%> (ø)`	⬆️
src/amo/components/AddonReviewCard/index.js	`100% <100%> (ø)`	⬆️
src/amo/components/AddonReviewManager/index.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cc7038...fa04ee6. Read the comment docs.

package.json

src/amo/components/AddonReviewCard/index.js

willdurand · 2018-11-12T15:28:22Z

src/core/localState.js

@@ -43,14 +44,14 @@ export class LocalState {
        return data;
      })
      .catch((error) => {
-        log.error(`Error with localForage.getItem("${this.id}"): ${error}`);
+        log.info(`Error with localForage.getItem("${this.id}"): ${error}`);


Why do you change info to error? That should be logged as errors so that the level is correctly set in syslog/es/kibana.

This component was written prior to having error boundaries. Since the errors get thrown, they will automatically go to Sentry, right? I think so. That's why I turned it down to info.

I still don't understand, sorry. Kibana aggregates those log calls, it is important to respect the different levels.

If I leave them as error then Sentry would get two entries for the same error. This would add a lot of noise. My thought was to only send the error to Sentry once but still log these messages for debugging purposes.

Then, you should probably use debug, no? End-users get those info messages. It'd be nice to not spam them in the console.

I think we also need an issue to review our Sentry configurationand usage.

Sure, I can use debug.

End-users get those info messages.

You mean because we don't hide the console messages? That's fine by me. I would rather have them there in the console when we need them.

But, yeah, I suppose this is more suitable for the debug level.

src/ui/components/DismissibleTextForm/index.js

willdurand · 2018-11-12T15:58:14Z

src/ui/components/DismissibleTextForm/index.js

@@ -211,14 +264,14 @@ export class DismissibleTextFormBase extends React.Component<
    return (
      <form className={makeClassName('DismissibleTextForm-form', className)}>
        <Textarea
-          defaultValue={this.state.text}


That's an uncontrolled component, no? https://reactjs.org/docs/uncontrolled-components.html#default-values A weird one though, like a semi-controlled/semi-uncontrolled component 😄

Since you can supply any props to Textarea and therefore to the underlying textarea, that defaultValue prop is passed to the textarea and the textarea becomes an uncontrolled component. In theory, one would need to use a ref to read the textarea content but... the Textarea component implements an onChange handler on the underlying textarea.

According to the code, onChange is always called in Textarea: https://github.com/andreypopp/react-textarea-autosize/blob/f4f49e84c4beccf29188c72c2a3b21899bc0e85a/src/index.js#L113, so the onTextChange callback is also always called, and when it updates the state, the defaultValue gets updated too (because of the re-render).

The implementation of onChange in Textarea is not correct IMO, it should early return in if (!this._controlled) { ... }, so that developers would know that defaultValue and onChange don't play nice together.

willdurand · 2018-11-12T16:11:14Z

src/ui/components/DismissibleTextForm/index.js

          disabled={isSubmitting}
          className="DismissibleTextForm-textarea"
          inputRef={(ref) => {
            this.textarea = ref;
          }}
          onChange={this.onTextChange}
          placeholder={text.placeholder}
+          value={this.state.text}


src/ui/components/DismissibleTextForm/index.js

kumar303 · 2018-11-12T22:25:15Z

Thanks. This is ready for another look.

willdurand

That works for me, let's give enough time to QA :) r+

Could you please file two issues:

I noticed the report form on an add-on detail page does not have this auto-save feature (ReportAbuseButton), unless there is a good explanation for not doing it (but it is done for reporting a user now)
A "code quality" issue for reviewing our Sentry configuration and (non-)usage. Our project in Sentry is a mess currently, we don't sort/close/resolve anything and finding something is a miracle. Now that we have a decent logging setup, we should see how to leverage Sentry. That probably means: maybe fine-tuning Sentry to not get duplicates between Sentry and Kibana, maybe pass the request ID to Sentry, reviewing what is sent to Sentry.

willdurand · 2018-11-12T23:10:26Z

I noticed the report form on an add-on detail page does not have this auto-save feature (ReportAbuseButton), unless there is a good explanation for not doing it (but it is done for reporting a user now)

Or maybe it is just this one... mozilla/addons#10834

kumar303 · 2018-11-12T23:12:04Z

Bah! I was searching ReportAbuseButton before I filed mozilla/addons#12655. I'll dupe it. I thought we had an issue.

willdurand · 2018-11-12T23:12:38Z

Bah! I was searching ReportAbuseButton before I filed mozilla/addons-frontend#6907. I'll dupe it. I thought we had an issue.

I was browsing the code quality issues and then 🤦‍♂️ sorry for that

kumar303 · 2018-11-12T23:14:36Z

So much for Github related issues 🍌

kumar303 · 2018-11-12T23:27:05Z

...pass the request ID to Sentry

Great idea! I filed mozilla/addons#12656

Our project in Sentry is a mess currently, we don't sort/close/resolve anything and finding something is a miracle.

There is a lot of noise. You filter by Firefox, right? That's the only sane way to look at it. We don't have any formal process to review / triage errors but we probably should.

fine-tuning Sentry to not get duplicates between Sentry and Kibana

I'm not sure I want to prevent duplicates between the two. Sentry is optimized for errors so it has some nicer features. I find it difficult to use Kibana so I'm not sure I want to go "all in" on it for errors just yet. If I'm already in Kibana, I wouldn't want to have errors hidden from me.

...reviewing what is sent to Sentry

I'm not sure how to write an actionable issue for this. What do you want to review? Sentry captures all server errors and on the client it captures all window.onerror calls (i.e. any uncaught error) and all console.error calls, which include error boundaries due to the call to log.error().

kumar303 · 2018-11-12T23:34:39Z

You can also see only server errors by specifying a logger name.

There are a lot of client errors I don't understand. I don't understand why we have errors triggered by external sites (I thought CSP would prevent this) and I don't understand why some of the errors seem to be triggered by moz-extension:// (an add-on that the user is running) because I thought AMO can't run add-ons. Hmm, maybe those are built-in add-ons.

willdurand · 2018-11-13T09:52:40Z

There is a lot of noise. You filter by Firefox, right? That's the only sane way to look at it. We don't have any formal process to review / triage errors but we probably should.

We should probably start sorting those entries, no? For example, reviewing the entries periodically, creating GH issues from Sentry, etc. Otherwise, we have a huge amount of data no one sees, or only when it is too late. Can we talk about doing this? I think the backend team does that pretty well, we should get inspiration from them :)

I'm not sure how to write an actionable issue for this. What do you want to review? Sentry captures all server errors and on the client it captures all window.onerror calls (i.e. any uncaught error) and all console.error calls, which include error boundaries due to the call to log.error().

Oh, I see. Can we agree on some guidelines related to that? I think no React component should have a log.error statement given what you said above. We should not have too much log.info but on the other hand, log.debug may not appear in Kibana so we should distinguish dev-related logs vs useful information.

For instance, I think that your log statements should use .warn instead of .debug now because if we get those logs in Kibana with a "warn" severity and an error in Sentry, both tools become super useful and work together (which was one of my suggestions).

kumar303 · 2018-11-13T15:26:17Z

We should probably start sorting those [Sentry] entries, no?

Everything takes time and spending time on it takes away from development of new features but, yeah, I agree. We should put a process in place to make sure we don't miss critical production errors.

I am not familiar with how the backend team does it. Do you want to start an email thread to discuss it? I am hesitant to put more meetings on the calendar but we could probably benefit from a meeting to triage these errors (unless someone has a better idea).

Can we agree on some guidelines related to that?

Sure. Let me know if anything here should change: mozilla/addons#12662

willdurand · 2018-11-13T15:32:03Z

I am not familiar with how the backend team does it. Do you want to start an email thread to discuss it? I am hesitant to put more meetings on the calendar but we could probably benefit from a meeting to triage these errors (unless someone has a better idea).

I'll add this topic for the All Hands, WDYT?

Sure. Let me know if anything here should change: mozilla/addons#12662

I'll take a look, thanks

kumar303 · 2018-11-13T15:42:46Z

I'll add this topic for the All Hands, WDYT?

Yep, sounds good

Auto-save text written in DismissibleTextForm

9565b44

kumar303 commented Nov 9, 2018

View reviewed changes

Clean up tests, bump bundlesize

c604aa7

kumar303 requested review from bobsilverberg, willdurand and rebmullin November 9, 2018 23:25

willdurand self-assigned this Nov 12, 2018

willdurand reviewed Nov 12, 2018

View reviewed changes

kumar303 added 2 commits November 12, 2018 16:12

Clear the form when dismissing

8e3de9d

Switch dependency injection to underscore prefix style

cec3498

kumar303 requested a review from willdurand November 12, 2018 22:25

Switch log level to debug

fa04ee6

willdurand approved these changes Nov 12, 2018

View reviewed changes

kumar303 merged commit 803beb8 into mozilla:master Nov 12, 2018

kumar303 deleted the form-autosave-6883 branch November 12, 2018 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-save text written in DismissibleTextForm #6884

Auto-save text written in DismissibleTextForm #6884

kumar303 commented Nov 9, 2018

kumar303 Nov 9, 2018 •

edited

Loading

willdurand Nov 12, 2018

codecov-io commented Nov 9, 2018 •

edited

Loading

willdurand Nov 12, 2018

kumar303 Nov 12, 2018

willdurand Nov 12, 2018

kumar303 Nov 12, 2018

willdurand Nov 12, 2018

kumar303 Nov 12, 2018 •

edited

Loading

willdurand Nov 12, 2018

willdurand Nov 12, 2018

kumar303 commented Nov 12, 2018

willdurand left a comment

willdurand commented Nov 12, 2018

kumar303 commented Nov 12, 2018

willdurand commented Nov 12, 2018

kumar303 commented Nov 12, 2018

kumar303 commented Nov 12, 2018

kumar303 commented Nov 12, 2018

willdurand commented Nov 13, 2018

kumar303 commented Nov 13, 2018

willdurand commented Nov 13, 2018

kumar303 commented Nov 13, 2018

Auto-save text written in DismissibleTextForm #6884

Auto-save text written in DismissibleTextForm #6884

Conversation

kumar303 commented Nov 9, 2018

kumar303 Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 9, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 commented Nov 12, 2018

willdurand left a comment

Choose a reason for hiding this comment

willdurand commented Nov 12, 2018

kumar303 commented Nov 12, 2018

willdurand commented Nov 12, 2018

kumar303 commented Nov 12, 2018

kumar303 commented Nov 12, 2018

kumar303 commented Nov 12, 2018

willdurand commented Nov 13, 2018

kumar303 commented Nov 13, 2018

willdurand commented Nov 13, 2018

kumar303 commented Nov 13, 2018

kumar303 Nov 9, 2018 •

edited

Loading

codecov-io commented Nov 9, 2018 •

edited

Loading

kumar303 Nov 12, 2018 •

edited

Loading