fix: ux enhance for report insertion#460
Open
Mateeb-Haider wants to merge 2 commits intofossasia:mainfrom
Open
fix: ux enhance for report insertion#460Mateeb-Haider wants to merge 2 commits intofossasia:mainfrom
Mateeb-Haider wants to merge 2 commits intofossasia:mainfrom
Conversation
Contributor
Reviewer's GuideRefactors and unifies the popup toast/notification system with inline, theme-aware styling and adds explicit error handling and user feedback for the "Insert to Email" flow and org validation failures. Sequence diagram for Insert to Email error handling and toast feedbacksequenceDiagram
actor User
participant PopupScript
participant ChromeTabsAPI
participant EmailTabContentScript
User->>PopupScript: Click insertToEmail button
PopupScript->>ChromeTabsAPI: sendMessage(action insertReportToEmail)
ChromeTabsAPI-->>PopupScript: Callback with chrome.runtime.lastError set
alt Runtime error (no supported email tab)
PopupScript->>PopupScript: Build errorMsg from i18n insertInEmailFailed
PopupScript->>PopupScript: showPopupMessage(errorMsg, error)
PopupScript-->>User: Toast shown: open email tab (Gmail/Outlook/Yahoo)
else No runtime error but response not success
ChromeTabsAPI-->>PopupScript: response.success = false, response.error
PopupScript->>PopupScript: errorMsg = response.error or i18n insertInEmailFailed
PopupScript->>PopupScript: showPopupMessage(errorMsg, error)
PopupScript-->>User: Toast shown: insert to email failed
else Success
ChromeTabsAPI-->>PopupScript: response.success = true
PopupScript-->>User: Report inserted, no error toast
end
Sequence diagram for unified org validation error toastssequenceDiagram
actor User
participant PopupScript
participant GitHubAPI
User->>PopupScript: Blur org input field
PopupScript->>GitHubAPI: Fetch organisation details
alt Organisation not found (404)
GitHubAPI-->>PopupScript: Response status 404
PopupScript->>PopupScript: message = i18n orgNotFoundMessage
PopupScript->>PopupScript: showPopupMessage(message, error)
PopupScript-->>User: Error toast: organisation not found
else Network or other error
GitHubAPI-->>PopupScript: Promise rejected
PopupScript->>PopupScript: message = i18n orgValidationErrorMessage
PopupScript->>PopupScript: showPopupMessage(message, error)
PopupScript-->>User: Error toast: validation problem
else Organisation exists
GitHubAPI-->>PopupScript: Response status 200
PopupScript->>PopupScript: Remove caches and trigger repo fetch
PopupScript-->>User: No error toast
end
Flow diagram for unified toast creation and displayflowchart TD
A[showPopupMessage called
with message and type] --> B{message present?}
B -- No --> C[Return without action]
B -- Yes --> D[Remove existing
scrum-helper-toast element]
D --> E[Create new div
id scrum-helper-toast]
E --> F[Check body has
class dark-mode]
F --> G[Apply base positioning,
padding, radius, typography,
transform, opacity 0]
G --> H[Apply boxShadow
depending on theme]
H --> I[Apply background,
borderColor, text color
for light or dark mode]
I --> J[Set textContent
to message]
J --> K[Append toast to body]
K --> L[requestAnimationFrame
animate opacity to 1 and
translateY to 0]
L --> M[setTimeout 4000ms
start hide animation]
M --> N[Animate opacity to 0
and translateY -20px]
N --> O[After 300ms remove
toast from DOM if present]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
the
javascript
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
typeparameter onshowPopupMessageis never used to alter styling or behavior; either wire it up to differentiate error/info toasts or remove it to avoid confusion. - The new implementation completely drops the
window.Materialize.toastfallback, which changes behavior in environments where Materialize is present; consider preserving the existing fallback and only using the custom toast when Materialize is unavailable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `type` parameter on `showPopupMessage` is never used to alter styling or behavior; either wire it up to differentiate error/info toasts or remove it to avoid confusion.
- The new implementation completely drops the `window.Materialize.toast` fallback, which changes behavior in environments where Materialize is present; consider preserving the existing fallback and only using the custom toast when Materialize is unavailable.
## Individual Comments
### Comment 1
<location path="src/scripts/popup.js" line_range="285" />
<code_context>
}
- function showPopupMessage(message) {
+ function showPopupMessage(message, type = 'info') {
if (!message) return;
</code_context>
<issue_to_address>
**suggestion:** The `type` parameter is currently unused and could either be removed or leveraged for variant-specific styling.
Since callers always pass `'error'` but the implementation ignores `type`, consider either using it to drive variant styling (e.g., warning/success/error colors and borders) or dropping it for now to keep the API clear and avoid misleading future callers.
```suggestion
function showPopupMessage(message) {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| function showPopupMessage(message) { | ||
| function showPopupMessage(message, type = 'info') { |
Contributor
There was a problem hiding this comment.
suggestion: The type parameter is currently unused and could either be removed or leveraged for variant-specific styling.
Since callers always pass 'error' but the implementation ignores type, consider either using it to drive variant styling (e.g., warning/success/error colors and borders) or dropping it for now to keep the API clear and avoid misleading future callers.
Suggested change
| function showPopupMessage(message, type = 'info') { | |
| function showPopupMessage(message) { |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the UX of the “Insert to Email” flow in the extension popup by surfacing connection/unsupported-tab failures to the user via a self-contained toast message, and introduces a new localized string for that failure case.
Changes:
- Replace the popup toast implementation with an inline-styled, theme-aware toast utility and reuse it for org validation errors.
- Add user-visible error handling for “Insert to Email” when content scripts aren’t reachable or return a failure.
- Add a new i18n message key (
insertInEmailFailed) for the new error toast.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/scripts/popup.js |
Adds toast UX + wiring to show user-friendly errors for “Insert to Email” failures and org validation failures. |
src/_locales/en/messages.json |
Adds a new English i18n string used by the new error handling path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+285
to
289
| function showPopupMessage(message, type = 'info') { | ||
| if (!message) return; | ||
|
|
||
| // Materialize toast if available | ||
| if (window.Materialize && typeof window.Materialize.toast === 'function') { | ||
| window.Materialize.toast(message, 4000); | ||
| return; | ||
| } | ||
|
|
||
| const old = document.getElementById('scrum-cache-toast'); | ||
| const old = document.getElementById('scrum-helper-toast'); | ||
| if (old) old.remove(); |
| toast.style.fontWeight = 'bold'; | ||
| toast.style.padding = '12px 24px'; | ||
| toast.style.borderRadius = '8px'; | ||
| toast.id = 'scrum-helper-toast'; |
Comment on lines
+307
to
310
| "insertInEmailFailed": { | ||
| "message": "Open an email tab (Gmail/Outlook/Yahoo) to use this feature.", | ||
| "description": "Error message shown when the extension cannot connect to an email tab." | ||
| } |
Contributor
Author
|
@vedansh-5 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Fixes
Fixes #459
📝 Summary of Changes
Error Handling: Added robust error detection for the "Insert to Email" feature. It now catches connection errors when a supported email tab (Gmail/Outlook/Yahoo) is not active.
📸 Screenshots / Demo (if UI-related)
✅ Checklist
👀 Reviewer Notes
The toast system is now completely self-contained with inline styles, ensuring it works reliably even if external CSS is missing. It prioritizes visibility with high z-index and optimized shadows for both themes.
Summary by Sourcery
Improve popup toast notifications and error messaging for email report insertion and org validation.
Bug Fixes:
Enhancements: