Skip to content

fixed screenshot function - #128

Open
veronika-tseleva-cleantalk wants to merge 2 commits into
devfrom
fix_55346-2
Open

fixed screenshot function#128
veronika-tseleva-cleantalk wants to merge 2 commits into
devfrom
fix_55346-2

Conversation

@veronika-tseleva-cleantalk

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI lite review requested due to automatic review settings September 4, 2026 12:23

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The updated screenshot error-handling currently returns early in the dom-to-image failure path (skipping fallback) and swallows fallback failures, causing incorrect behavior and making the caller’s error handling ineffective.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the widget’s automatic screenshot capture flow and enhances screenshot error handling in the FileUploader, including corresponding updates to the bundled dist artifact.

Changes:

  • Passes a boolean flag to makeScreenshot(...) during automatic widget open to suppress end-user error UI.
  • Adds an in-page toast notification helper and adjusts dom-to-image screenshot capture logic (with html2canvas fallback).
  • Regenerates/updates dist/doboard-widget-bundle.js to reflect source changes.
File summaries
File Description
js/src/widget.js Updates automatic screenshot call to pass an argument intended to suppress user-facing errors.
js/src/fileuploader.js Adds toast notification helper and changes screenshot capture/error-handling flow.
dist/doboard-widget-bundle.js Bundled output updated to match the new screenshot/toast behavior.
Review details
  • Files reviewed: 2/5 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread js/src/fileuploader.js
Comment thread js/src/fileuploader.js
Comment thread js/src/fileuploader.js
Copilot AI review requested due to automatic review settings September 4, 2026 12:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

makeScreenshot() currently rethrows in user-notified failure paths, which can trigger unhandled promise rejections in existing callers that don’t .catch() the returned promise, and the new toast needs basic ARIA attributes for accessibility.

Review details

Suppressed comments (3)

js/src/fileuploader.js:557

  • In the html2canvas fallback path, errors are rethrown even after showing the toast, which can lead to unhandled promise rejections for callers that don’t handle the returned promise. If showError is true, consider returning after notifying; only throw when showError === false.
            } catch (error) {
                console.error(error);
                if (showError) {
                    if (typeof this.showErrorNotification === 'function') {
                        this.showErrorNotification('Unable to take a screenshot due to the site\'s security settings (CORS).');
                    } else {
                        alert('Unable to take a screenshot due to the site\'s security settings (CORS).');
                    }
                }
                throw error;
            }

js/src/fileuploader.js:507

  • makeScreenshot() rejects even after showing the user-facing toast, which can create unhandled promise rejections for callers that don’t .catch() (e.g., click handlers) and also prevents graceful failure when the error is already surfaced to the user. Consider only rejecting when showError === false (so background/auto screenshot callers can still handle failures via .catch()), and otherwise return after notifying.
            } catch (error) {
                if (showError) {
                    if (typeof this.showErrorNotification === 'function') {
                        this.showErrorNotification('Unable to take a screenshot due to the site\'s security settings (CORS).');
                    } else {
                        alert('Unable to take a screenshot due to the site\'s security settings (CORS).');
                    }
                }
                throw new Error('Screenshot failed due to CORS');
            }

js/src/fileuploader.js:420

  • The toast notification is not announced to assistive technologies. Adding role="alert" / aria-live="assertive" will make the error message accessible to screen reader users.
        const toast = document.createElement('div');
        toast.textContent = message;
  • Files reviewed: 2/5 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants