fix: blank share url in share dialog [PT-188104620]

[kswenson]

The ShareDialogView is responsible for displaying the various sharing urls, embedding urls, etc. that result from sharing a document. It constructs these urls from constituent parts passed into it as props, e.g. sharedDocumentId and sharedDocumentUrl. Utility methods in the form of getShareUrl(), getShareLink(), and getEmbed() construct these urls from the provided props. These urls are also stored in state through an asynchronous mechanism that is triggered by the user pressing the Enable Sharing button. These state variables are set to the return values of the getShareLink() and getEmbed() utility methods, which construct them from props, as previously stated. The bug occurs because in v3, the setState callback occurs before the props have been updated, so the state variables are set to empty, and then later the dialog renders the url fields from state, even though the props have been updated by that time. So the explanation for the bug is that there is essentially a race condition and that in v3, perhaps due to the update from React 16 to 18, the ordering of events has changed. The justification for the fix is that since the state is set from the props, the props are always at least as current as the state, and so it is safer/preferable to use the props instead of the state when rendering, and so calling getShareLink() and getShareUrl() when rendering fixes the bug.

[kswenson]

@tealefristoe Doug is the logical person to review this but since he's unavailable, I'm nominating you in his stead. Hopefully, the explanation is sufficient to lend some credence to the fix.

[tealefristoe]

Seems ok to me, though I'll admit that this is a subtle bug I'm pretty unfamiliar with. I guess my only question is do you ever use this.state.link or this.state.embed any more?

[kswenson]

Seems ok to me, though I'll admit that this is a subtle bug I'm pretty unfamiliar with. I guess my only question is do you ever use this.state.link or this.state.embed any more?

Essentially not, and I thought about removing them, but I'm concerned that the setState call that sets them may be triggering a necessary re-render, even if we never actually use the resulting state values, so I'm inclined to not make any further changes than necessary at this point.