BL-12977 OneDrive error dialog

[nabalone]

Catch OneDrive exceptions and give a clear notice with no report button

---

This change is 

[nabalone]

Reviewable status: 0 of 15 files reviewed, 5 unresolved discussions

---

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 23 at r1 (raw file):

    secondaryLabel?: string; // The localized text that goes on the secondary action button. Omit or pass "" to disable the secondary action button.
}> = props => {
    if ([ProblemKind.Notify, ProblemKind.OneDriveNonFatal, ProblemKind.OneDriveFatal].includes(props.level)) { // TODO? is there a better way to do this?

Would it be better to do

if (
    props.level === ProblemKind.Notify 
    || props.level === ProblemKind. OneDriveNonFatal
    || props.level === ProblemKind.ONeDriveFatal
)

---

src/BloomBrowserUI/utils/linkHandler.ts line 16 at r1 (raw file):

        return;
    }
    if (href.startsWith("http") || href.startsWith("mailto") || href.startsWith('file')) { // TODO? will it cause problems elsewhere to add file here?

I looked around and didn't see anything that looked like it would break because we changed file:// handling but wasnt sure

---

src/BloomExe/Book/BookInfo.cs line 1014 at r1 (raw file):

                    id = GetIdFromDamagedMetaDataString(RobustFile.ReadAllText(metaDataPath));
                }
                // TODO? should I replace all calls to RobustFile.Read... with wrapped versions?

i.e. make method(s) which call the RobustFile.ReadAll() functions but wrap them in a try/catch and convert IOExceptions to FileExceptions? And then put those everywhere? yagni?

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 742 at r1 (raw file):

            // }

            // TODO? is it worth splitting?

There's a lot of code shared between ShowProblemDialog and CheckForAndHandleOneDriveExceptions. I was trying to factor out the repeated dialog related code, but it would mean a helper method called twice that takes in a lot of parameters. Maybe we want to be able to arbitrarily adjust the two dialog behaviors independently of each other anyway?

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 862 at r1 (raw file):

        )
        {
            // TODO? do we want to localize any of these?

I am generally not sure how much is worth localizing

[andrew-polk]

Reviewed 14 of 15 files at r1, all commit messages.
Reviewable status: 14 of 15 files reviewed, 18 unresolved discussions (waiting on @nabalone)

---

DistFiles/localization/en/Bloom.xlf line 5140 at r1 (raw file):

        <note>ID: ReportProblemDialog.WhatDoing</note>
        <note>This is the label for the text field where the user enters what they were doing at the time of the problem.</note>
      </trans-unit>

Generally, we are trying not to add things to Bloom.xlf.
New strings should be added to the -Medium or -Low versions.
(I guess high-priority ones still go in Bloom.xlf??)

These can probably go in -Low.

---

DistFiles/localization/en/Bloom.xlf line 5146 at r1 (raw file):

      </trans-unit>
      <trans-unit id="ReportProblemDialog.OneDriveProblem" translate="no">
        <source xml:lang="en">OneDriveProblem</source>

Shouldn't this source string be "OneDrive Problem"?

---

src/BloomBrowserUI/problemDialog/NotifyDialog.tsx line 27 at r1 (raw file):

console.log(props);

Let's remove this.

---

src/BloomBrowserUI/problemDialog/NotifyDialog.tsx line 67 at r1 (raw file):

#f0f0f0

I think it would be an improvement to use colorUtils' kFormBackground.

---

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 23 at r1 (raw file):

Previously, nabalone (Noel) wrote…

Would it be better to do

if (
    props.level === ProblemKind.Notify 
    || props.level === ProblemKind. OneDriveNonFatal
    || props.level === ProblemKind.ONeDriveFatal
)

Either syntax is fine.
But as I say below, let's discuss the idea of having new "kinds" for OneDrive.

---

src/BloomBrowserUI/problemDialog/theme.ts line 41 at r1 (raw file):

#f44336

Since this is designed to be the same as the fatal color, let's extract that code as a const.

---

src/BloomBrowserUI/utils/linkHandler.ts line 16 at r1 (raw file):

Previously, nabalone (Noel) wrote…

I looked around and didn't see anything that looked like it would break because we changed file:// handling but wasnt sure

I'm not sure what else we can do but look around (as you did) and test.
Let's remove the TODO comment.

---

src/BloomExe/Program.cs line 1301 at r1 (raw file):

string filePath

Looks like this isn't used.

---

src/BloomExe/Program.cs line 1306 at r1 (raw file):

            // Normally NotifyUserOfProblem would check for special exceptions and handle them appropriately.
            // But, the code below doesn't use the usual NotifyUserOfProblem path (because we may want to SendReportWithoutUI below)

Sorry; I'm not processing this comment well.
Is this accurate?
Normally, NotifyUserOfProblem would take an exception and do this special-exception processing for us. But in this case, we don't pass the exception to NotifyUserOfProblem because we may subsequently end up calling SendReportWithoutUI. Therefore, we must check for the special exception independently.

---

src/BloomExe/Program.cs line 1307 at r1 (raw file):

There

Is this supposed to be "Therefore"?

---

src/BloomExe/Book/BookInfo.cs line 1014 at r1 (raw file):

Previously, nabalone (Noel) wrote…

i.e. make method(s) which call the RobustFile.ReadAll() functions but wrap them in a try/catch and convert IOExceptions to FileExceptions? And then put those everywhere? yagni?

Putting them everywhere is definitely out of scope from what I understood.
If there was more than one place we were specifically wrapping a RobustFile. call, I would probably say it was worth a wrapper. But looks like other place you throw FileException is wrapping a bunch of stuff.
So, I could go either way. If you think there will likely be other places we wrap RobustFile. calls, specifically, you could make a wrapper now. But we could also wait until we do.

---

src/BloomExe/Collection/CollectionSettings.cs line 500 at r1 (raw file):

outerError

Rather than outer, isn't this inner? Or maybe "original" is better? (That matches the constructor signature for FileException.)

---

src/BloomExe/CollectionTab/CollectionTabView.cs line 288 at r1 (raw file):

        }

        private void CheckForDuplicates()

Assuming that it actually does, how about CheckForDuplicatesAndRepair? Or similar.

Same for BookInfo's version.

---

src/BloomExe/ErrorReporter/HtmlErrorReporter.cs line 234 at r1 (raw file):

                    {
                        System.Threading.Monitor.Exit(_lock);
                    }

                    if (!wasAlreadyLocked)
                    {
                        System.Threading.Monitor.Exit(_lock);
                    }

Unless I'm missing something, this isn't needed. The finally block will do it.

---

src/BloomExe/Utils/FileException.cs line 24 at r1 (raw file):

        }

        public static string FilePathIfPresent(Exception exception)

Should this be GetFilePathIfPresent?

---

src/BloomExe/Utils/FileException.cs line 30 at r1 (raw file):

                return ((FileException)exception).FilePath;
            }
            return "";

Is there a reason this is empty string rather than null?

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 742 at r1 (raw file):

Previously, nabalone (Noel) wrote…

There's a lot of code shared between ShowProblemDialog and CheckForAndHandleOneDriveExceptions. I was trying to factor out the repeated dialog related code, but it would mean a helper method called twice that takes in a lot of parameters. Maybe we want to be able to arbitrarily adjust the two dialog behaviors independently of each other anyway?

Let's discuss.
It may be part of a bigger discussion which is worth having about having different report types just based on OneDrive or not.

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 862 at r1 (raw file):

Previously, nabalone (Noel) wrote…

I am generally not sure how much is worth localizing

Yeah, always a hard call.
Doesn't seem worth it at this time.
Maybe worth replacing the TODO comment with something like
// We decided not to localize this for now.

[andrew-polk]

Reviewed 15 of 16 files at r2, all commit messages.
Reviewable status: 16 of 17 files reviewed, 6 unresolved discussions (waiting on @nabalone)

---

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 20 at r2 (raw file):

    reportLabel?: string; // The localized text that goes on the Report button. Omit or pass "" to disable Report button.
    secondaryLabel?: string; // The localized text that goes on the secondary action button. Omit or pass "" to disable the secondary action button.
    notifyOnly?: boolean; // If true, use the NotifyDialog with no report button regardless of the level.

According to the comment on reportLabel, leaving that off should be enough to prevent the report button.
So if that isn't true, we should either make it true and remove this prop, or update the comment.

---

src/BloomExe/ErrorReporter/HtmlErrorReporter.cs line 231 at r2 (raw file):

                )
                {
                    return; // TODO make sure we are letting go of the lock

// TODO

I know you're still working on this. I'm just adding this to make sure it doesn't get lost or left in.

---

src/BloomExe/Utils/FileException.cs line 24 at r1 (raw file):

Previously, andrew-polk wrote…

Should this be GetFilePathIfPresent?

Oops. Should be capital Get

---

src/BloomExe/Utils/OneDriveUtils.cs line 30 at r2 (raw file):

        };

        public static bool isOneDriveErrorCode(string errorCode)

Remember, unlike js/ts, in C#, we capitalize method names.

---

src/BloomExe/Utils/OneDriveUtils.cs line 40 at r2 (raw file):

                    "ReportProblemDialog.OneDriveErrorMessage",
                    "There is a problem with your Microsoft OneDrive which is preventing Bloom from accessing files."
                ) + Environment.NewLine; // TODO I thought I got rid of this...

// TODO

I know you're still working on this. I'm just adding this to make sure it doesn't get lost or left in.

[nabalone]

Reviewable status: 12 of 18 files reviewed, 5 unresolved discussions (waiting on @andrew-polk)

---

DistFiles/localization/en/Bloom.xlf line 5140 at r1 (raw file):

Previously, andrew-polk wrote…

Generally, we are trying not to add things to Bloom.xlf.
New strings should be added to the -Medium or -Low versions.
(I guess high-priority ones still go in Bloom.xlf??)

These can probably go in -Low.

Oh ok, Done. I was just trying to keep them together with other problem dialog text.

---

DistFiles/localization/en/Bloom.xlf line 5146 at r1 (raw file):

Previously, andrew-polk wrote…

Shouldn't this source string be "OneDrive Problem"?

Done. oops

---

src/BloomBrowserUI/problemDialog/NotifyDialog.tsx line 27 at r1 (raw file):

Previously, andrew-polk wrote…

console.log(props);

Let's remove this.

Done. Oops 🤦‍♀️I searched my diff for leftover TODOs but forgot to search for console.logs. Sorry about that

---

src/BloomBrowserUI/problemDialog/NotifyDialog.tsx line 67 at r1 (raw file):

Previously, andrew-polk wrote…

#f0f0f0

I think it would be an improvement to use colorUtils' kFormBackground.

Done.

---

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 20 at r2 (raw file):

Previously, andrew-polk wrote…

According to the comment on reportLabel, leaving that off should be enough to prevent the report button.
So if that isn't true, we should either make it true and remove this prop, or update the comment.

Ah ok, I tried to clarify

---

src/BloomExe/Program.cs line 1301 at r1 (raw file):

Previously, andrew-polk wrote…

string filePath

Looks like this isn't used.

Done. But do we eventually want to add filePaths to the error message or log now that we might have them here

---

src/BloomExe/Program.cs line 1306 at r1 (raw file):

Previously, andrew-polk wrote…

Sorry; I'm not processing this comment well.
Is this accurate?
Normally, NotifyUserOfProblem would take an exception and do this special-exception processing for us. But in this case, we don't pass the exception to NotifyUserOfProblem because we may subsequently end up calling SendReportWithoutUI. Therefore, we must check for the special exception independently.

Done. Yes, that is what I was trying to say! much better

---

src/BloomExe/Book/BookInfo.cs line 1014 at r1 (raw file):

Previously, andrew-polk wrote…

Putting them everywhere is definitely out of scope from what I understood.
If there was more than one place we were specifically wrapping a RobustFile. call, I would probably say it was worth a wrapper. But looks like other place you throw FileException is wrapping a bunch of stuff.
So, I could go either way. If you think there will likely be other places we wrap RobustFile. calls, specifically, you could make a wrapper now. But we could also wait until we do.

Ok, I'll leave it is as is for now

---

src/BloomExe/Collection/CollectionSettings.cs line 500 at r1 (raw file):

Previously, andrew-polk wrote…

outerError

Rather than outer, isn't this inner? Or maybe "original" is better? (That matches the constructor signature for FileException.)

Done.

---

src/BloomExe/CollectionTab/CollectionTabView.cs line 288 at r1 (raw file):

Previously, andrew-polk wrote…

Assuming that it actually does, how about CheckForDuplicatesAndRepair? Or similar.

Same for BookInfo's version.

Done.

---

src/BloomExe/ErrorReporter/HtmlErrorReporter.cs line 234 at r1 (raw file):

Previously, andrew-polk wrote…

                    if (!wasAlreadyLocked)
                    {
                        System.Threading.Monitor.Exit(_lock);
                    }

Unless I'm missing something, this isn't needed. The finally block will do it.

You're right, looks like I shouldn't need it. But I put it in in response to the lock not getting released somewhere. So hopefully I did eventually fix that problem...

---

src/BloomExe/Utils/FileException.cs line 24 at r1 (raw file):

Previously, andrew-polk wrote…

Should this be GetFilePathIfPresent?

Done.

---

src/BloomExe/Utils/FileException.cs line 30 at r1 (raw file):

Previously, andrew-polk wrote…

Is there a reason this is empty string rather than null?

Done.

---

src/BloomExe/Utils/OneDriveUtils.cs line 30 at r2 (raw file):

Previously, andrew-polk wrote…

Remember, unlike js/ts, in C#, we capitalize method names.

Done.

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 862 at r1 (raw file):

Previously, andrew-polk wrote…

Yeah, always a hard call.
Doesn't seem worth it at this time.
Maybe worth replacing the TODO comment with something like
// We decided not to localize this for now.

Done.

[nabalone]

Reviewable status: 12 of 18 files reviewed, 6 unresolved discussions (waiting on @andrew-polk)

---

src/BloomExe/ErrorReporter/HtmlErrorReporter.cs line 231 at r2 (raw file):

Previously, andrew-polk wrote…

// TODO

I know you're still working on this. I'm just adding this to make sure it doesn't get lost or left in.

Done.

---

src/BloomExe/Utils/OneDriveUtils.cs line 40 at r2 (raw file):

Previously, andrew-polk wrote…

// TODO

I know you're still working on this. I'm just adding this to make sure it doesn't get lost or left in.

Done.

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 629 at r3 (raw file):

        // ENHANCE: Reduce duplication in HtmlErrorReporter and ProblemReportApi code. Some of the ProblemReportApi code can move to HtmlErrorReporter code.

        // ENHANCE: I think levelOfProblem would benefit from being required and being an enum.

I see we have ProblemLevel.cs (https://github.com/BloomBooks/BloomDesktop/blob/master/src/BloomExe/ErrorReporter/ProblemLevel.cs)
It feels bad to still use string literals here, in the defaults for ShowProblemDialog and CheckForAndHandleOneDriveExceptions below. Is there an easy way to improve that now?

[nabalone]

Reviewable status: 12 of 18 files reviewed, 5 unresolved discussions (waiting on @andrew-polk)

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 629 at r3 (raw file):

Previously, nabalone (Noel) wrote…

I see we have ProblemLevel.cs (https://github.com/BloomBooks/BloomDesktop/blob/master/src/BloomExe/ErrorReporter/ProblemLevel.cs)
It feels bad to still use string literals here, in the defaults for ShowProblemDialog and CheckForAndHandleOneDriveExceptions below. Is there an easy way to improve that now?

I guess I'm not sure whether to problemLevel to an enum now and if so how far up/down the callstacks to make that change

[andrew-polk]

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nabalone)

---

DistFiles/localization/en/Bloom.xlf line 5140 at r1 (raw file):

Previously, nabalone (Noel) wrote…

Oh ok, Done. I was just trying to keep them together with other problem dialog text.

Ah, yeah, I guess that would have made sense, too.
But putting them in low seems right for these. Thanks.

---

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 20 at r2 (raw file):

Previously, nabalone (Noel) wrote…

Ah ok, I tried to clarify

Sigh. I see now why you introducing a new ProblemKind(s). We had conflated the idea of which dialog to present and which theme to use. It worked previously because there was no overlap but now there is.
It seems like the ideal would be to separate them. So I'm trying to figure out how worth it it is....
One option is to add a NotifyFatal ProblemKind, but that just perpetuates the problem.
Another option is to make two separate props, one for which problem kind, one for which theme.
A third option is to have a theme override such that you would pass Notify as the kind in this case but pass in a theme override of Fatal. I'm leaning toward option #3. What do you think? Happy to discuss in person...

As a somewhat separate concern... if we have all these props which are actually specific to the NotifyDialog, it seems like maybe we should define INotifyDialogProps in NotifyDialog.tsx and make the type here level:ProblemKind | INotifyDialogProps or whatever the syntax is. I can pair with you on this if needed.

---

src/BloomExe/Program.cs line 1301 at r1 (raw file):

Previously, nabalone (Noel) wrote…

Done. But do we eventually want to add filePaths to the error message or log now that we might have them here

Yes, adding it to the log sounds right. I don't think we want to add it to the error message which already has the path to the collection.

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 633 at r3 (raw file):

Show Rpoblem Dialog

Show Problem Dialog

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 641 at r3 (raw file):

        /// <param name="controlForScreenshotting"></param>
        /// <param name="exception"></param>
        /// <param name="height"></param>

Let's remove all these empty param tags.

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 867 at r3 (raw file):

        }

        private static void ShowOneDriveExceptionFallbackDialog(

Should we move this and the next method into OneDriveUtils?

[nabalone]

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 20 at r2 (raw file):

Previously, andrew-polk wrote…

Sigh. I see now why you introducing a new ProblemKind(s). We had conflated the idea of which dialog to present and which theme to use. It worked previously because there was no overlap but now there is.
It seems like the ideal would be to separate them. So I'm trying to figure out how worth it it is....
One option is to add a NotifyFatal ProblemKind, but that just perpetuates the problem.
Another option is to make two separate props, one for which problem kind, one for which theme.
A third option is to have a theme override such that you would pass Notify as the kind in this case but pass in a theme override of Fatal. I'm leaning toward option #3. What do you think? Happy to discuss in person...

As a somewhat separate concern... if we have all these props which are actually specific to the NotifyDialog, it seems like maybe we should define INotifyDialogProps in NotifyDialog.tsx and make the type here level:ProblemKind | INotifyDialogProps or whatever the syntax is. I can pair with you on this if needed.

Do you mean that the level or the notify dialog props should be a single argument? As in, if the level is "notify", then instead of the string "notify", the "level" should just be an INotifyDialogPropsObject? Is this better than making a second, optional argument?

[andrew-polk]

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 20 at r2 (raw file):

Previously, nabalone (Noel) wrote…

Do you mean that the level or the notify dialog props should be a single argument? As in, if the level is "notify", then instead of the string "notify", the "level" should just be an INotifyDialogPropsObject? Is this better than making a second, optional argument?

Sorry, I confused you because I didn't use the right syntax.
I'm suggesting that the type of the props for ProblemDialog be the union of level:ProblemKind and INotifyDialogProps.

[andrew-polk]

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 20 at r2 (raw file):

Previously, andrew-polk wrote…

Sorry, I confused you because I didn't use the right syntax.
I'm suggesting that the type of the props for ProblemDialog be the union of level:ProblemKind and INotifyDialogProps.

I played with it a little just now in code. I would require an intersection (&) to do what I'm talking about.
But I'm second-guessing that it is worth it. I was just trying to document via type that the other props are really just for the notify dialog. But it doesn't feel clean as I try to make it work.

[nabalone]

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 20 at r2 (raw file):

Previously, andrew-polk wrote…

I played with it a little just now in code. I would require an intersection (&) to do what I'm talking about.
But I'm second-guessing that it is worth it. I was just trying to document via type that the other props are really just for the notify dialog. But it doesn't feel clean as I try to make it work.

Oh. Do you not like this?

export const ProblemDialog: React.FC<{level:ProblemKind} & Partial<INotifyDialogProps>> = props => {

    if (props.level === ProblemKind.Notify) {

        return <NotifyDialog {...props} />;

    } else {

        return <ReportDialog kind\={props.level} />;

    }

};

[andrew-polk]

Reviewed 3 of 4 files at r4, all commit messages.
Reviewable status: 17 of 18 files reviewed, 2 unresolved discussions (waiting on @nabalone)

---

src/BloomBrowserUI/problemDialog/ProblemDialog.tsx line 20 at r2 (raw file):

Previously, nabalone (Noel) wrote…

Oh. Do you not like this?

export const ProblemDialog: React.FC<{level:ProblemKind} & Partial<INotifyDialogProps>> = props => {

    if (props.level === ProblemKind.Notify) {

        return <NotifyDialog {...props} />;

    } else {

        return <ReportDialog kind\={props.level} />;

    }

};

That works!
You did a better job of figuring it out.

---

src/BloomExe/Program.cs line 1301 at r1 (raw file):

Previously, andrew-polk wrote…

Yes, adding it to the log sounds right. I don't think we want to add it to the error message which already has the path to the collection.

Sorry, I meant to suggest we go ahead and log it now.

[nabalone]

I am testing throwing exceptions and catching them and I am getting a "Invoke or BeginInvoke cannot be called on a control until the window handle has been created." error while trying to display the error dialog. I am worried that this looks like a timing thing and that users could reach it while encountering OneDrive errors. I have not yet figured out how to fix it

Reviewable status: 15 of 18 files reviewed, 2 unresolved discussions (waiting on @andrew-polk)

---

src/BloomExe/Program.cs line 1301 at r1 (raw file):

Previously, andrew-polk wrote…

Sorry, I meant to suggest we go ahead and log it now.

Done.

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 633 at r3 (raw file):

Previously, andrew-polk wrote…

Show Rpoblem Dialog

Show Problem Dialog

Done. (will push )

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 641 at r3 (raw file):

Previously, andrew-polk wrote…

Let's remove all these empty param tags.

Done. (will push)

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 867 at r3 (raw file):

Previously, andrew-polk wrote…

Should we move this and the next method into OneDriveUtils?

I'm not sure it makes any sense outside the context of CheckForAndHandleOneDriveExceptions, and I feel like they should stay together since they have a lot of the same text. Can I put it inside CheckForAndHandleOneDriveExceptions instead? I also factored out the text they share

[nabalone]

Reviewable status: 15 of 18 files reviewed, 3 unresolved discussions (waiting on @andrew-polk)

---

src/BloomBrowserUI/problemDialog/NotifyDialog.tsx line 75 at r5 (raw file):

                                    margin-bottom: 20px; 
                                    a {
                                        color: ${kBloomBlue}; // we are passing links in the text

This feels like a band aid. Wondering if it is worth it try and handle links and styling better

[nabalone]

src/BloomExe/web/controllers/ProblemReportApi.cs line 657 at r6 (raw file):

                        // this one, while apparently redundant might be wise to keep since closing the splash screen
                        // needs to be done on the UI thread.
                        try

Here I catch the Invoke or BeginInvoke cannot be called on a control until the window handle has been created. exception that was happening, at least this should catch it in OneDrive error cases

[nabalone]

I am testing throwing exceptions and catching them and I am getting a "Invoke or BeginInvoke cannot be called on a control until the window handle has been created." error while trying to display the error dialog. I am worried that this looks like a timing thing and that users could reach it while encountering OneDrive errors. I have not yet figured out how to fix it

Was happening on an attempt to close the splash screen. I added an additional try/catch to catch it.

[andrew-polk]

Reviewed 1 of 3 files at r5, 1 of 2 files at r6, all commit messages.
Reviewable status: 17 of 18 files reviewed, 3 unresolved discussions (waiting on @nabalone)

---

src/BloomExe/web/controllers/ProblemReportApi.cs line 867 at r3 (raw file):

Previously, nabalone (Noel) wrote…

I'm not sure it makes any sense outside the context of CheckForAndHandleOneDriveExceptions, and I feel like they should stay together since they have a lot of the same text. Can I put it inside CheckForAndHandleOneDriveExceptions instead? I also factored out the text they share

Sorry; I actually meant to move both ShowOneDriveExceptionFallbackDialog and CheckForAndHandleOneDriveExceptions to OneDriveUtils. Is that feasible?

[andrew-polk]

Reviewed 5 of 5 files at r7, 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

[andrew-polk]

Dismissed @nabalone from 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nabalone)