BL-15820-dropbox-warning by hatton · Pull Request #7648 · BloomBooks/BloomDesktop

hatton · 2026-02-02T17:56:53Z

This change is

devin-ai-integration

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional flags.

For various reasons simply adding ""Important: synchronization problems can be caused when one or more members of your team have incorrect Dropbox settings. Please ensure all members of your team collection are using the correct settings. See [critical Dropbox settings](https://docs.bloomlibrary.org/critical-dropbox-settings/)."; lead to all these changes!

Copilot

Pull request overview

This PR adds a Dropbox-specific warning message to team collection synchronization errors and improves the handling of links in UI messages. The changes refactor error reporting code into a separate partial class file and extend the React component for rendering links to support multiple links and external URLs.

Changes:

Added Dropbox-specific warning to sync error messages that directs users to critical settings documentation
Refactored error reporting methods from TeamCollection.cs into a new partial class file (TeamCollection.ErrorReporting.cs)
Enhanced StringWithOptionalLink React component to support multiple links and external URLs (http/mailto)
Added test coverage for the enhanced link handling functionality

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.

File	Description
src/BloomExe/TeamCollection/TeamCollection.cs	Made class partial and removed error reporting methods (moved to separate file); updated l10nIdSuffix to be consistent with established pattern
src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs	New partial class file containing error reporting methods; adds Dropbox warning functionality and markdown-to-HTML conversion
src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx	Enhanced to support multiple links and external link handling via 'link' API endpoint
src/BloomBrowserUI/react_components/stringWithOptionalLink.spec.tsx	New test file with tests for multiple links and external link behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-05T21:40:21Z

src/BloomBrowserUI/react_components/stringWithOptionalLink.spec.tsx

+    it("renders spans and anchors for multiple links", () => {
+        const markup = renderToStaticMarkup(
+            <StringWithOptionalLink
+                message={
+                    "Start <a href='/bloom/api/internal'>first</a> middle <a href='http://example.com'>second</a> end"
+                }
+            />,
+        );
+        const temp = document.createElement("div");
+        temp.innerHTML = markup;
+
+        const spans = temp.querySelectorAll("span");
+        const anchors = temp.querySelectorAll("a");
+
+        expect(spans.length).toBe(3);
+        expect(spans[0].textContent).toBe("Start ");
+        expect(spans[1].textContent).toBe(" middle ");
+        expect(spans[2].textContent).toBe(" end");
+
+        expect(anchors.length).toBe(2);
+        expect(anchors[0].textContent).toBe("first");
+        expect(anchors[0].getAttribute("href")).toBe("/bloom/api/internal");
+        expect(anchors[1].textContent).toBe("second");
+        expect(anchors[1].getAttribute("href")).toBe("http://example.com");
+    });
+
+    it("invokes post for internal links", () => {
+        const host = renderIntoDom(
+            "Do <a href='/bloom/api/doThing'>this</a> now",
+        );
+        const anchor = host.querySelector("a");
+        expect(anchor).not.toBeNull();
+
+        anchor?.dispatchEvent(
+            new MouseEvent("click", { bubbles: true, cancelable: true }),
+        );
+
+        expect(postMock).toHaveBeenCalledWith("doThing");
+        expect(postStringMock).not.toHaveBeenCalled();
+    });
+
+    it("invokes postString for external links", () => {
+        const host = renderIntoDom(
+            "Visit <a href='mailto:test@example.com'>email</a>",
+        );
+        const anchor = host.querySelector("a");
+        expect(anchor).not.toBeNull();
+
+        anchor?.dispatchEvent(
+            new MouseEvent("click", { bubbles: true, cancelable: true }),
+        );
+
+        expect(postStringMock).toHaveBeenCalledWith(
+            "link",
+            "mailto:test@example.com",
+        );
+        expect(postMock).not.toHaveBeenCalled();
+    });
+});


The test suite is missing coverage for several important edge cases: (1) messages with no links at all (should render a single span), (2) messages starting or ending with a link (no preceding/trailing text), (3) consecutive links with no text between them, and (4) messages with empty string. Adding these test cases would improve confidence in the component's robustness.

Copilot · 2026-02-05T21:40:21Z

src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx

-                </a>
-                <span>{match[4]}</span>
-            </React.Fragment>
+    const linkRegex = /<a[^>]*?href='([^>']+)'[^>]*>(.*?)<\/a>/g;


The regex with the 'g' flag is being used in a while loop with exec(), which mutates the regex's lastIndex property. This is correct for the current implementation, but if the component re-renders with the same props (e.g., due to parent re-render), the regex needs to be reset. Consider moving the regex creation inside the component function (after line 13) to ensure it's fresh for each render, or explicitly resetting lastIndex to 0 before the loop.

Copilot · 2026-02-05T21:40:21Z

src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx

+        const isExternalLink =
+            rawHref.startsWith("http") || rawHref.startsWith("mailto");


The detection of external links only checks for 'http' and 'mailto' prefixes. This will incorrectly classify 'https' URLs as external (which is correct) but will miss other protocols like 'ftp', 'tel', etc. More importantly, it won't handle URLs that start with '//' (protocol-relative URLs). Consider using a more comprehensive check, such as checking if the URL starts with 'http://', 'https://', 'mailto:', or '//' to be more precise.

Suggested change

const isExternalLink =

rawHref.startsWith("http") || rawHref.startsWith("mailto");

const lowerHref = rawHref.toLowerCase();

const isExternalLink =

lowerHref.startsWith("http://") ||

lowerHref.startsWith("https://") ||

lowerHref.startsWith("mailto:") ||

rawHref.startsWith("//");

Not needed.

Copilot · 2026-02-05T21:40:22Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+        string CoreReportProblemSyncingBook(
+            IWebSocketProgress progress,
+            ProgressKind kind,
+            string l10nIdSuffix,
+            string message,
+            string param0 = null,
+            string param1 = null
+        )
+        {
+            var warning =
+                GetBackendType() == "DropBox"
+                    ? LocalizationManager.GetString(
+                        "TeamCollection.DropboxSyncSettingsWarning",
+                        kDropboxSettingsWarningEnglish
+                    )
+                    : null;
+            if (string.IsNullOrEmpty(warning))
+            {
+                ReportProgressAndLog(progress, kind, l10nIdSuffix, message, param0, param1);
+                var msg = string.Format(message, param0, param1);
+                Analytics.Track(
+                    "TeamCollectionError",
+                    new Dictionary<string, string> { { "message", msg } }
+                );
+                return msg;
+            }
+
+            var fullL10nId = string.IsNullOrEmpty(l10nIdSuffix)
+                ? ""
+                : "TeamCollection." + l10nIdSuffix;
+            var localizedTemplate = string.IsNullOrEmpty(l10nIdSuffix)
+                ? message
+                : LocalizationManager.GetString(fullL10nId, message);
+            var formattedMessage = string.Format(localizedTemplate, param0, param1);
+            var fullMessage = $"{formattedMessage}\n\n{warning}";
+            var progressMessage = ConvertMarkdownLinksToHtml(fullMessage);
+
+            // Keep a single user-facing message by logging the combined, already-localized text.
+            progress.MessageWithoutLocalizing(progressMessage, kind);
+            var messageType =
+                (kind == ProgressKind.Progress)
+                    ? MessageAndMilestoneType.History
+                    : MessageAndMilestoneType.ErrorNoReload;
+            _tcLog.WriteMessage(messageType, "", fullMessage, null, null);
+
+            var msg =
+                $"{string.Format(message, param0, param1)}\n\n{kDropboxSettingsWarningEnglish}";
+            Analytics.Track(
+                "TeamCollectionError",
+                new Dictionary<string, string> { { "message", msg } }
+            );
+            return msg;
+        }


The new Dropbox warning functionality introduced in CoreReportProblemSyncingBook lacks test coverage. Given that TeamCollection has comprehensive test coverage using NUnit and Moq (as seen in TeamCollectionTests.cs), consider adding tests to verify: (1) the warning is shown only for DropBox backend type, (2) markdown links are correctly converted to HTML, (3) the warning is correctly appended to error messages, and (4) the localized vs. English versions are handled appropriately in Analytics and logging.

Copilot · 2026-02-05T21:40:22Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+        static string ConvertMarkdownLinksToHtml(string message)
+        {
+            if (string.IsNullOrEmpty(message))
+                return message;
+            return Regex.Replace(message, @"\[(.*?)\]\((.*?)\)", "<a href='$2'>$1</a>");
+        }


The ConvertMarkdownLinksToHtml method is a static utility function that would benefit from unit tests. Consider adding tests to verify it handles: (1) single and multiple markdown links, (2) links with special characters in the text or URL, (3) nested brackets or parentheses, (4) empty or null input, and (5) markdown links mixed with other markdown syntax. This would help ensure the regex is robust and prevent regressions.

no change at this time

Copilot · 2026-02-05T21:40:23Z

src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx

 // Currently the href is assumed to be something to send to our API,
 // but NOT to actually navigate to. We could support more options as needed.


The comment states "Currently the href is assumed to be something to send to our API, but NOT to actually navigate to." However, the code now handles external links (http/mailto) by sending them to the 'link' API endpoint, which then opens them in an external browser. This comment should be updated to reflect that external links are now supported and will be opened externally.

Suggested change

// Currently the href is assumed to be something to send to our API,

// but NOT to actually navigate to. We could support more options as needed.

// For internal links, the href is treated as something to send to our API rather than

// something for the browser to navigate to directly. External http/mailto hrefs are

// sent to the 'link' API endpoint, which opens them in an external browser.

Copilot · 2026-02-05T21:40:23Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+            if (string.IsNullOrEmpty(warning))
+            {
+                ReportProgressAndLog(progress, kind, l10nIdSuffix, message, param0, param1);
+                var msg = string.Format(message, param0, param1);


The format string ignores this supplied value.
The format string ignores this supplied value.
The format string ignores this supplied value.

No idea what this means. Ignoring.

Copilot · 2026-02-05T21:40:23Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+            var localizedTemplate = string.IsNullOrEmpty(l10nIdSuffix)
+                ? message
+                : LocalizationManager.GetString(fullL10nId, message);
+            var formattedMessage = string.Format(localizedTemplate, param0, param1);


The format string ignores this supplied value.
The format string ignores this supplied value.
The format string ignores this supplied value.

No idea what this means. Ignoring.

Copilot · 2026-02-05T21:40:23Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+            var msg =
+                $"{string.Format(message, param0, param1)}\n\n{kDropboxSettingsWarningEnglish}";


The format string ignores this supplied value.
The format string ignores this supplied value.
The format string ignores this supplied value.

Suggested change

var msg =

$"{string.Format(message, param0, param1)}\n\n{kDropboxSettingsWarningEnglish}";

var msg = $"{formattedMessage}\n\n{kDropboxSettingsWarningEnglish}";

No idea what this means. Ignoring.

Copilot · 2026-02-05T21:40:24Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+                    var fullMsg =
+                        $"{standardUserInfo} \n(Admins: {admins}):\n\nThere was a book synchronization problem that required putting a version in Lost and Found:\n{msg}\n\nSee {lostAndFoundUrl}.\n\n{extraInfo}";
+                    var issueId = issue.SubmitToYouTrack("Book synchronization failed", fullMsg);
+                    var issueLink = "https://issues.bloomlibrary.org/youtrack/issue/" + issueId;


This assignment to issueLink is useless, since its value is never read.

Suggested change

var issueLink = "https://issues.bloomlibrary.org/youtrack/issue/" + issueId;

copies existing code; ignoring

andrew-polk

@andrew-polk reviewed 4 files and all commit messages, made 14 comments, resolved 11 discussions, and dismissed @copilot[bot] from 11 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hatton).

andrew-polk · 2026-02-05T22:05:24Z

src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx

 // Currently the href is assumed to be something to send to our API,
 // but NOT to actually navigate to. We could support more options as needed.


andrew-polk · 2026-02-05T22:05:24Z

src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx

-                </a>
-                <span>{match[4]}</span>
-            </React.Fragment>
+    const linkRegex = /<a[^>]*?href='([^>']+)'[^>]*>(.*?)<\/a>/g;


andrew-polk · 2026-02-05T22:05:25Z

src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx

+        const isExternalLink =
+            rawHref.startsWith("http") || rawHref.startsWith("mailto");


Not needed.

andrew-polk · 2026-02-05T22:05:25Z

src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx

+                    post(href);
+                }}
+            >
+                {match[2]}


No change needed.

andrew-polk · 2026-02-05T22:05:25Z

src/BloomBrowserUI/react_components/stringWithOptionalLink.tsx

+            <a
+                key={`link-${segmentIndex}`}
+                // We don't currently use the href, but to get link formatting it
+                // has to be present. May also be helpful for accessibility.
+                href={rawHref}
+                onClick={(e) => {
+                    e.preventDefault(); // so it doesn't try to follow the link
+                    if (isExternalLink) {
+                        postString("link", rawHref);
+                        return;
+                    }
+                    post(href);
+                }}
+            >
+                {match[2]}
+            </a>,


Not needed.

andrew-polk · 2026-02-05T22:05:27Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+            var localizedTemplate = string.IsNullOrEmpty(l10nIdSuffix)
+                ? message
+                : LocalizationManager.GetString(fullL10nId, message);
+            var formattedMessage = string.Format(localizedTemplate, param0, param1);


No idea what this means. Ignoring.

andrew-polk · 2026-02-05T22:05:27Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+                (kind == ProgressKind.Progress)
+                    ? MessageAndMilestoneType.History
+                    : MessageAndMilestoneType.ErrorNoReload;
+            _tcLog.WriteMessage(messageType, "", fullMessage, null, null);


incorrect assessment; ignoring

andrew-polk · 2026-02-05T22:05:27Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+            var msg =
+                $"{string.Format(message, param0, param1)}\n\n{kDropboxSettingsWarningEnglish}";


No idea what this means. Ignoring.

andrew-polk · 2026-02-05T22:05:27Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+        string CoreReportProblemSyncingBook(
+            IWebSocketProgress progress,
+            ProgressKind kind,
+            string l10nIdSuffix,
+            string message,
+            string param0 = null,
+            string param1 = null
+        )
+        {
+            var warning =
+                GetBackendType() == "DropBox"
+                    ? LocalizationManager.GetString(
+                        "TeamCollection.DropboxSyncSettingsWarning",
+                        kDropboxSettingsWarningEnglish
+                    )
+                    : null;
+            if (string.IsNullOrEmpty(warning))
+            {
+                ReportProgressAndLog(progress, kind, l10nIdSuffix, message, param0, param1);
+                var msg = string.Format(message, param0, param1);
+                Analytics.Track(
+                    "TeamCollectionError",
+                    new Dictionary<string, string> { { "message", msg } }
+                );
+                return msg;
+            }
+
+            var fullL10nId = string.IsNullOrEmpty(l10nIdSuffix)
+                ? ""
+                : "TeamCollection." + l10nIdSuffix;
+            var localizedTemplate = string.IsNullOrEmpty(l10nIdSuffix)
+                ? message
+                : LocalizationManager.GetString(fullL10nId, message);
+            var formattedMessage = string.Format(localizedTemplate, param0, param1);
+            var fullMessage = $"{formattedMessage}\n\n{warning}";
+            var progressMessage = ConvertMarkdownLinksToHtml(fullMessage);
+
+            // Keep a single user-facing message by logging the combined, already-localized text.
+            progress.MessageWithoutLocalizing(progressMessage, kind);
+            var messageType =
+                (kind == ProgressKind.Progress)
+                    ? MessageAndMilestoneType.History
+                    : MessageAndMilestoneType.ErrorNoReload;
+            _tcLog.WriteMessage(messageType, "", fullMessage, null, null);
+
+            var msg =
+                $"{string.Format(message, param0, param1)}\n\n{kDropboxSettingsWarningEnglish}";
+            Analytics.Track(
+                "TeamCollectionError",
+                new Dictionary<string, string> { { "message", msg } }
+            );
+            return msg;
+        }


andrew-polk · 2026-02-05T22:05:28Z

src/BloomExe/TeamCollection/TeamCollection.ErrorReporting.cs

+                    var fullMsg =
+                        $"{standardUserInfo} \n(Admins: {admins}):\n\nThere was a book synchronization problem that required putting a version in Lost and Found:\n{msg}\n\nSee {lostAndFoundUrl}.\n\n{extraInfo}";
+                    var issueId = issue.SubmitToYouTrack("Book synchronization failed", fullMsg);
+                    var issueLink = "https://issues.bloomlibrary.org/youtrack/issue/" + issueId;


copies existing code; ignoring

andrew-polk

@andrew-polk reviewed 4 files and all commit messages, made 14 comments, resolved 11 discussions, and dismissed @copilot[bot] from 11 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hatton).

Copilot · 2026-02-05T22:05:42Z

@andrew-polk I've opened a new pull request, #7661, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-02-05T22:05:47Z

@andrew-polk I've opened a new pull request, #7662, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: andrew-polk <5847219+andrew-polk@users.noreply.github.com>

Address review feedback: improve StringWithOptionalLink robustness (#7661) Co-Authored-By: andrew-polk <5847219+andrew-polk@users.noreply.github.com>

andrew-polk

@andrew-polk reviewed 2 files and all commit messages, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hatton).

devin-ai-integration bot reviewed Feb 2, 2026

View reviewed changes

hatton force-pushed the Bl-15820TC-new-msg branch from d077db1 to 9519ee7 Compare February 3, 2026 22:57

hatton force-pushed the Bl-15820TC-new-msg branch from 9519ee7 to eefecb1 Compare February 3, 2026 23:18

hatton marked this pull request as ready for review February 5, 2026 21:34

Copilot AI review requested due to automatic review settings February 5, 2026 21:34

Copilot started reviewing on behalf of hatton February 5, 2026 21:34 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

andrew-polk reviewed Feb 5, 2026

View reviewed changes

This was referenced Feb 5, 2026

Address review feedback: improve StringWithOptionalLink robustness #7661

Merged

Address review feedback: fix regex mutation, update docs, add edge case tests #7662

Closed

Copilot AI and others added 2 commits February 5, 2026 15:28

Address review feedback: update comment, move regex, add edge case tests

3b76aad

Co-authored-by: andrew-polk <5847219+andrew-polk@users.noreply.github.com>

Merge pull request #7661 from BloomBooks/copilot/sub-pr-7648

122c9b1

Address review feedback: improve StringWithOptionalLink robustness (#7661) Co-Authored-By: andrew-polk <5847219+andrew-polk@users.noreply.github.com>

andrew-polk approved these changes Feb 5, 2026

View reviewed changes

andrew-polk merged commit 826cbba into Version6.3 Feb 5, 2026
2 checks passed

andrew-polk deleted the Bl-15820TC-new-msg branch February 5, 2026 22:40

		const isExternalLink =
		rawHref.startsWith("http") \|\| rawHref.startsWith("mailto");

-        const isExternalLink =
-            rawHref.startsWith("http") || rawHref.startsWith("mailto");
+        const lowerHref = rawHref.toLowerCase();
+        const isExternalLink =
+            lowerHref.startsWith("http://") ||
+            lowerHref.startsWith("https://") ||
+            lowerHref.startsWith("mailto:") ||
+            rawHref.startsWith("//");

		// Currently the href is assumed to be something to send to our API,
		// but NOT to actually navigate to. We could support more options as needed.

-// Currently the href is assumed to be something to send to our API,
-// but NOT to actually navigate to. We could support more options as needed.
+// For internal links, the href is treated as something to send to our API rather than
+// something for the browser to navigate to directly. External http/mailto hrefs are
+// sent to the 'link' API endpoint, which opens them in an external browser.

		var msg =
		$"{string.Format(message, param0, param1)}\n\n{kDropboxSettingsWarningEnglish}";

	var msg =
	$"{string.Format(message, param0, param1)}\n\n{kDropboxSettingsWarningEnglish}";
	var msg = $"{formattedMessage}\n\n{kDropboxSettingsWarningEnglish}";

Uh oh!

Conversation

hatton commented Feb 2, 2026 • edited by devin-ai-integration bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

✅ Devin Review: No Issues Found

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hatton commented Feb 2, 2026 •

edited by devin-ai-integration bot

Loading