PM-22836 Import popout crashes unexpectedly #16928
Conversation
• added popout dialog for file imports inspired by file type send popout dialog • TODO: fix bug whereby a user may dismiss popout dialog and attempt to select a file outside of popout in Send and Import
harr1424
changed the title
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16928 +/- ##
==========================================
- Coverage 41.23% 41.23% -0.01%
==========================================
Files 3543 3548 +5
Lines 101963 102053 +90
Branches 15295 15304 +9
==========================================
+ Hits 42048 42082 +34
- Misses 58152 58206 +54
- Partials 1763 1765 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…en-after-closing-extension-pop-up-and-selecting-choose-file
|
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
There was a problem hiding this comment.
question: Why is this a separate component rather than just living inside the import-browser component? It seems quite weird to have a child component popup a dialog that redirects the parent page.
| component: SendAddEditV2Component, | ||
| canActivate: [authGuard], | ||
| data: { elevation: 1 } satisfies RouteDataProperties, | ||
| data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties, |
There was a problem hiding this comment.
question: Why are we not saving the url here?
There was a problem hiding this comment.
Copying from Claude's comment (below) which correctly notes this logic wasn't applied to the import path:
Without this flag, the import page URL is saved in popup history
If a user navigates away and the popup restores this URL, they could still access the file selector outside the popout
This defeats the purpose of the Firefox crash fix
…en-after-closing-extension-pop-up-and-selecting-choose-file
|
Claude finished @harr1424's task —— View job PR Review CompleteI've reviewed the Firefox file selector crash fix. The implementation correctly addresses the core issue, but there are some concerns to address before merging. Critical FindingsFinding 1: ❌ Incorrect i18n key creates confusing dialog title
Required fixAdd a new i18n key to "importPopoutDialogTitle": {
"message": "Pop out to import"
}Then update the template: <div bitTypography="h3">
{{ "importPopoutDialogTitle" | i18n }}
</div>Finding 2: ❌ Comment references wrong feature
Correct comment: // If the user selects "cancel" when presented the dialog, navigate back to the Vault Settings viewArchitecture & PatternsFinding 3: 💭 Inconsistent dialog implementations The Import and Send dialogs are nearly identical (same structure, buttons, behavior) but implemented separately. Consider: Could these be unified into a single reusable Finding 4: 💭 Different container patterns create maintenance burden
Both solve the same problem. This inconsistency makes the codebase harder to maintain. Consider aligning to one pattern (preferably the Signals approach for new code). Finding 5: 💭 Navigation pattern differences
The author explained this is because Import is nested (has back button) while Send is root-level. While valid, this creates maintenance confusion. Recommendation: Add clarifying comments in both files explaining why the approaches differ. Outstanding Reviewer QuestionsFinding 6: @Hinton's comment suggests using Angular Signals with Question: Should this PR adopt the Signals approach for both Send and Import to avoid relying on Finding 7: 💭 Dialog template duplication unaddressed @Hinton's comment asked if the Import and Send dialogs should be combined since they're nearly identical. The author responded but didn't commit to a decision. Recommendation: Clarify whether unifying these dialogs is in scope for this PR or should be tracked separately. PR Description QualityFinding 8: 🎨 Good problem description, could improve test plan Strengths:
Improvement opportunity:
SummaryThe core fix is sound - preventing file selectors from opening outside popout windows addresses the Firefox crash. However, the implementation introduces inconsistencies:
The PR solves the stated problem but creates technical debt through pattern inconsistencies. Consider addressing the architectural concerns now or tracking them as follow-up work. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async close() { | ||
| this.dialogService.closeAll(); | ||
| // the current view exposes a file selector, ensure the view is popped to avoid using it outside of a popout | ||
| await this.popupRouterCacheService.back(); |
There was a problem hiding this comment.
The popupRouterCacheService.back() method will navigate to the root vault page if history is empty. This could be confusing UX if the user arrived at the import page through a direct link or bookmark rather than navigation.
Consider:
- Explicitly navigating to a known safe route (e.g.,
/vault-settings) instead of relying on back() - Or add error handling if back() returns false or fails
Context from PopupRouterCacheService
async back() {
// ...
if (this.hasNavigated && history.length) {
this.location.back();
return;
}
// if no history is present, fallback to vault page
await this.router.navigate([""]);
}This fallback behavior may not match user expectations when canceling an import.
|
|
||
| ngOnInit() { | ||
| if (this.filePopoutUtilsService.showFilePopoutMessage(window)) { | ||
| this.dialogService.open(ImportFilePopoutDialogComponent); |
There was a problem hiding this comment.
❌ Finding 2: Dialog opened in ngOnInit without cleanup handling
Opening a dialog in ngOnInit without storing a reference prevents proper cleanup and can cause issues:
- Memory leak: If component is destroyed while dialog is open, no cleanup occurs
- Multiple dialogs: If component is re-initialized, multiple dialogs could open
- No unsubscribe: Dialog subscription isn't tracked for cleanup
Recommended fix
import { Component, OnDestroy, OnInit } from "@angular/core";
import { DialogRef } from "@bitwarden/components";
export class ImportFilePopoutDialogContainerComponent implements OnInit, OnDestroy {
private dialogRef: DialogRef | null = null;
ngOnInit() {
if (this.filePopoutUtilsService.showFilePopoutMessage(window)) {
this.dialogRef = this.dialogService.open(ImportFilePopoutDialogComponent);
}
}
ngOnDestroy() {
this.dialogRef?.close();
}
}
| async close() { | ||
| this.dialogService.closeAll(); | ||
| // the current view exposes a file selector, ensure the view is popped to avoid using it outside of a popout | ||
| await this.router.navigate(["/tabs/send"]); |
There was a problem hiding this comment.
♻️ Finding 3: Inconsistent navigation patterns create technical debt
The Send dialog uses Router.navigate() to an absolute path while the Import dialog uses popupRouterCacheService.back(). This inconsistency creates confusion and maintenance burden.
Issue: Both components solve the same problem (prevent file selector outside popout) but use different navigation strategies.
Recommendation: Standardize on one approach:
- Option A: Both use explicit navigation to a known route
- Option B: Both use the router cache service (requires refactoring Send)
- Option C: Extract shared navigation logic to FilePopoutUtilsService
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
There was a problem hiding this comment.
💭 Finding 4: Component placement and responsibilities need clarification
The <import-file-popout-dialog-container> is embedded in the template but has no visual representation (empty template). This creates unclear component boundaries.
Questions for consideration:
- Why is this a separate component rather than logic in the parent
ImportBrowserV2Component? - The container component opens a dialog that navigates the parent - this violates typical component encapsulation
- Should this logic live in a route guard instead, since it's checking conditions on route entry?
Alternative approaches:
- Move the dialog-opening logic to
ImportBrowserV2Component.ngOnInit() - Or use a route guard that opens the dialog before route activation
- Or keep as child component but have it emit events rather than directly navigating
This addresses the reviewer's question about architecture.
| component: SendAddEditV2Component, | ||
| canActivate: [authGuard], | ||
| data: { elevation: 1 } satisfies RouteDataProperties, | ||
| data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties, |
There was a problem hiding this comment.
💭 Finding 5: Missing route configuration for import path
The add-send route has doNotSaveUrl: true to prevent saving the URL when the popout dialog might be shown. However, there's no corresponding route configuration change for the import page.
Questions:
- What is the route path for
ImportBrowserV2Component? (Not visible in the diff) - Does that route also need
doNotSaveUrl: truefor consistency? - Without
doNotSaveUrl, users who close the popout dialog will have the import page saved in history, which could lead to the file selector issue recurring
This addresses the reviewer's question about why we're not saving the URL here.
| </div> | ||
| <ng-container bitDialogContent> | ||
| <div bitTypography="h3"> | ||
| {{ "sendFilePopoutDialogText" | i18n }} |
There was a problem hiding this comment.
Line 7 uses "sendFilePopoutDialogText" (Send-related) while line 9 correctly uses "importPopoutDialogDesc" (Import-related). This creates a confusing message that says "Send file" in the title but talks about importing in the description.
Expected behavior: The title should use an import-specific i18n key.
Fix needed:
<div bitTypography="h3">
{{ "importPopoutDialogTitle" | i18n }}
</div>Then add to messages.json:
"importPopoutDialogTitle": {
"message": "Pop out required"
}
| "message": "This setting is disabled by your organization's policy.", | ||
| "description": "This hint text is displayed when a user setting is disabled due to an organization policy." | ||
| }, | ||
| "importPopoutDialogDesc": { |
There was a problem hiding this comment.
🎨 Finding 7: Missing i18n key for dialog title
Only the description key was added, but the template reuses the Send dialog's title (sendFilePopoutDialogText). This should have a dedicated import title key.
Suggested addition:
"importPopoutDialogTitle": {
"message": "Pop out required"
},
"importPopoutDialogDesc": {
"message": "To import from a file, you need to pop out the extension to a new window."
}
| component: SendAddEditV2Component, | ||
| canActivate: [authGuard], | ||
| data: { elevation: 1 } satisfies RouteDataProperties, | ||
| data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties, |
There was a problem hiding this comment.
❌ Finding 8: Import route missing doNotSaveUrl: true flag
The import route (line 246-249) should also have data: { elevation: 1, doNotSaveUrl: true } for consistency with the Send route change.
Why this matters:
- Without this flag, the import page URL is saved in popup history
- If a user navigates away and the popup restores this URL, they could still access the file selector outside the popout
- This defeats the purpose of the Firefox crash fix
Required change:
{
path: "import",
component: ImportBrowserV2Component,
canActivate: [authGuard],
data: { elevation: 1, doNotSaveUrl: true } satisfies RouteDataProperties,
}
| </div> | ||
| <ng-container bitDialogContent> | ||
| <div bitTypography="h3"> | ||
| {{ "sendFilePopoutDialogText" | i18n }} |
There was a problem hiding this comment.
❌ Finding 1: Incorrect i18n key creates confusing dialog title
This uses sendFilePopoutDialogText which displays "Pop out extension?" with a description about Send. However, line 9 correctly uses importPopoutDialogDesc for the description, creating inconsistent messaging.
Required fix:
| {{ "sendFilePopoutDialogText" | i18n }} | |
| {{ "importPopoutDialogTitle" | i18n }} |
Then add to messages.json:
"importPopoutDialogTitle": {
"message": "Pop out to import"
}
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
There was a problem hiding this comment.
💭 Finding 2: Component architecture violates encapsulation principles
This container component has no visual output (empty template) but opens a dialog that navigates the parent page, creating unclear boundaries.
Questions:
- Why is this a separate component rather than logic in
ImportBrowserV2Component.ngOnInit()? - Should this be a route guard instead since it checks conditions on route entry?
Alternative: Move dialog logic directly to parent component or implement as canActivate guard.
| async close() { | ||
| this.dialogService.closeAll(); | ||
| // the current view exposes a file selector, ensure the view is popped to avoid using it outside of a popout | ||
| await this.popupRouterCacheService.back(); |
There was a problem hiding this comment.
♻️ Finding 3: Inconsistent navigation patterns create technical debt
Import uses popupRouterCacheService.back() while Send uses Router.navigate(["/tabs/send"]) (send-file-popout-dialog.component.ts:30). Both solve the same problem with different approaches.
Recommendation: Standardize navigation strategy across both features or extract shared logic to FilePopoutUtilsService.
| private dialogService: DialogService, | ||
| private router: Router, | ||
| ) {} | ||
|
|
| } | ||
|
|
||
| close() { | ||
| async close() { |
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
| private popupRouterCacheService: PopupRouterCacheService, | ||
| ) {} | ||
|
|
||
| async popOutWindow() { |
There was a problem hiding this comment.
| async popOutWindow() { | |
| async popOutWindow(): Promise<void> { |
| await BrowserPopupUtils.openCurrentPagePopout(window); | ||
| } | ||
|
|
||
| async close() { |
There was a problem hiding this comment.
| async close() { | |
| async close(): Promise<void> { |
| > | ||
| </tools-import> | ||
|
|
||
| <import-file-popout-dialog-container></import-file-popout-dialog-container> |
There was a problem hiding this comment.
Self-closing components don't need closing tags in Angular.
| <import-file-popout-dialog-container></import-file-popout-dialog-container> | |
| <import-file-popout-dialog-container /> |
| }) | ||
| export class SendFilePopoutDialogComponent { | ||
| constructor(private dialogService: DialogService) {} | ||
| constructor( |
There was a problem hiding this comment.
| constructor( | |
| private dialogService = inject(DialogService); | |
| private router = inject(Router); |
| private router: Router, | ||
| ) {} | ||
|
|
||
| async popOutWindow() { |
There was a problem hiding this comment.
| async popOutWindow() { | |
| async popOutWindow(): Promise<void> { |
| } | ||
|
|
||
| close() { | ||
| async close() { |
There was a problem hiding this comment.
| async close() { | |
| async close(): Promise<void> { |
apps/browser/src/popup/main.ts
Outdated
|
|
||
| function init() { | ||
| void platformBrowserDynamic().bootstrapModule(AppModule); | ||
| platformBrowserDynamic() |
There was a problem hiding this comment.
💭 Finding 9: Unrelated bootstrap configuration changes
This explicit bootstrap configuration and error handling appears unrelated to the Firefox file selector crash fix. Was this intentional or debugging code that should be removed?
…en-after-closing-extension-pop-up-and-selecting-choose-file
|
@harr1424 This PR seems to do several things, what parts of this actually resolves the stated issue, i.e. popout crashes. |
@Hinton Depending upon user behavior the following file changes are required to prevent a pop-out crash on Firefox. See the root cause of the problem this PR defends against.
The following files don't directly address the Firefox issue, but were used to resolve a situation where the extension never loaded. @MGibson1 has since merged changes that resolves this on
|
Hinton
left a comment
Hinton
left a comment
There was a problem hiding this comment.
I have not reviewed the platform owned files since I don't have enough context to understand the impact of the changes.
There was a problem hiding this comment.
question: Why are these needed when we show a dialog on the pages when opened in a popup.
There was a problem hiding this comment.
Import
With doNotSaveUrl: true : Navigating to the import view, and then closing the pop-out, and then re-opening the main extension view shows the penultimate view to the import menu, the "Vault Options" view
Without doNotSaveUrl: true : Following the same workflow as above, instead of the "Vault Options" view, the pop-out dialog is instead displayed. Clicking cancel returns to the "Vault Options" view, and clicking "Pop Out" will trigger the pop-out.
Send
With doNotSaveUrl: true : Navigating towards send file view, confirming the pop-out, and then closing the pop-out, re-opening the extension (default not popped out state) shows the main "Send" view
Without doNotSaveUrl: true : The same workflow as above results in the "Send File" view being presented outside of the pop-out context. This allows the situation the PR is intended to avoid: a user can attempt to open a file selection dialog outside of a popped out context and the extensiuon will crash
Since only the Send functionality requires this, why was it added to Import as well?
For consistency, it can be safely removed from the import routing logic if this is preferred.
There was a problem hiding this comment.
I think I understand why you've resorted to adding doNotTrack. Since SendFilePopoutDialogContainerComponent relies on ngOnInit there is a race condition that primarily occurs when navigating directly to the add-send url without first landing on sends. Where config does not have the necessary values to trigger the open condition.
Is this accurate? And if so should we resolve the root cause rather than rely on the race condition?
@Component({
selector: "send-file-popout-dialog-container",
templateUrl: "./send-file-popout-dialog-container.component.html",
imports: [JslibModule, CommonModule],
})
export class SendFilePopoutDialogContainerComponent {
private readonly dialogService = inject(DialogService);
private readonly filePopoutUtilsService = inject(FilePopoutUtilsService);
readonly config = input.required<SendFormConfig>();
/**
* Tracks if the dialog has already been opened. This prevents multiple dialogs from opening if config is updated.
*/
private readonly dialogOpened = signal(false);
constructor() {
effect(() => {
if (this.dialogOpened()) {
return;
}
if (
this.config().sendType === SendType.File &&
this.config().mode === "add" &&
this.filePopoutUtilsService.showFilePopoutMessage(window)
) {
this.dialogService.open(SendFilePopoutDialogComponent, {
positionStrategy: new CenterPositionStrategy(),
});
this.dialogOpened.set(true);
}
});
}
}
...browser/src/tools/popup/send-v2/send-file-popout-dialog/send-file-popout-dialog.component.ts
Outdated
Show resolved
Hide resolved
apps/browser/src/tools/popup/services/file-popout-utils.service.ts
Outdated
Show resolved
Hide resolved
apps/browser/src/tools/popup/settings/import/import-file-popout-dialog-container.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
suggestion: This seems pretty much identical to the import popout dialog. Should they be combined and you just feed it the title/body to use?
There was a problem hiding this comment.
Sorry, but I am not sure I completely understand:
This seems pretty much identical to the import popout dialog
Which file are you referring to here?
There was a problem hiding this comment.
Apologies, SendFilePopoutDialogComponent
There was a problem hiding this comment.
@Hinton this definitley seems possible at first, and there is actually a FilePopoutComponent that curiously wasn't being used on the existing Send File workflow.
When I started work on this PR I began to suspect it is because FilePopoutComponent doesn't have any logic controlling cancellation behavior. Maybe this is preferred?
I've used the cancellation logic in ImportFilePopoutDialogComponent and SendFilePopoutDialogComponent to ensure the user is navigated away from a view that exposes a file selector, but since these components are at different view hierarchies, the navigation logic needs to be handled differently (this touches on another review comment):
export class ImportFilePopoutDialogComponent {
...
// If the user selects "cancel" when presented the dialog, navigate back to the main Send tab
async close() {
this.dialogService.closeAll();
await this.popupRouterCacheService.back();
}
}...
// If the user selects "cancel" when presented the dialog, navigate back to the main Send tab
async close() {
this.dialogService.closeAll();
await this.router.navigate(["/tabs/send"]);
}
}
apps/browser/src/tools/popup/settings/import/import-file-popout-dialog.component.html
Outdated
Show resolved
Hide resolved
| // the current view exposes a file selector, ensure the view is popped to avoid using it outside of a popout | ||
| await this.popupRouterCacheService.back(); |
There was a problem hiding this comment.
question: This behaves differently from send, any reason why?
There was a problem hiding this comment.
This has been changed for consistency.
There was a problem hiding this comment.
This has been changed for consistency.
This has been reverted back to use popupRouterCacheService.back(). Using router to instead navigate to a new views piles on views that can be "popped" off the stack. This creates a UI bug where clicking back repeatedly circles back to the same dialog presentation.
There was a problem hiding this comment.
The difference in behavior is because the "Send" tab/view is on the root of the extension, this view doesn't present a back button in the navigation tab. Whereas the "Import Data" view is nested inside of the "Vault Settings" view which does present a back button in the navigation tab.
…en-after-closing-extension-pop-up-and-selecting-choose-file
| <i bitDialogIcon class="bwi bwi-info-circle tw-text-info tw-text-3xl" aria-hidden="true"></i> | ||
| <ng-container bitDialogContent> | ||
| <div bitTypography="h3"> | ||
| {{ "sendFilePopoutDialogText" | i18n }} |
There was a problem hiding this comment.
❌ Wrong i18n key used
This reuses the Send dialog's title (sendFilePopoutDialogText) instead of an import-specific key. Line 7 correctly uses importPopoutDialogDesc, creating inconsistent messaging.
Fix needed:
| {{ "sendFilePopoutDialogText" | i18n }} | |
| {{ "importPopoutDialogTitle" | i18n }} |
Then add to messages.json:
"importPopoutDialogTitle": {
"message": "Pop out to import"
}
| async close() { | ||
| this.dialogService.closeAll(); | ||
| // the current view exposes a file selector, ensure the view is popped to avoid using it outside of a popout | ||
| await this.popupRouterCacheService.back(); |
There was a problem hiding this comment.
💭 Navigation pattern differs from Send implementation
The Send dialog uses this.router.navigate(["/tabs/send"]) while this uses popupRouterCacheService.back().
While the author explained this is because Import is nested (has a back button) and Send is root-level, this difference creates maintenance burden.
Suggestion: Add a comment explaining why the navigation approach differs:
// Navigate back using router cache since Import view is nested under Vault Settings
// (Send uses explicit navigation because it's at root level with no back history)
await this.popupRouterCacheService.back();
| filter((message) => message.action === "initialization"), | ||
| ); | ||
|
|
||
| const timeout$ = timer(2000).pipe(map((): MemoryStoragePortMessage | null => null)); |
There was a problem hiding this comment.
💭 Unrelated improvement or required for fix?
The timeout and logging improvements here appear unrelated to the Firefox file selector crash fix.
Questions:
- Is this debugging code needed for diagnosing the popout issue?
- Or is this a general improvement that should be in a separate PR?
The author mentioned these changes were added to resolve an extension loading issue that has since been fixed on main. Consider removing if not directly related to the Firefox crash fix.
There was a problem hiding this comment.
@coroiu My apologies, this is left over from when I was troubleshooting the breaking change introduced by a security path last week. I've now ensured none of those changes remain in this PR.
There was a problem hiding this comment.
@harr1424 alright, thanks! I went ahead and removed the review requests from platform since we no longer own any code in this PR 👍
| template: "", | ||
| imports: [JslibModule, CommonModule], | ||
| }) | ||
| export class ImportFilePopoutDialogContainerComponent implements OnInit, OnDestroy { |
There was a problem hiding this comment.
💭 Component architecture question
This container component has an empty template and only opens a dialog that navigates the parent page. This creates unclear component boundaries.
Alternative approaches to consider:
- Move dialog logic directly to
ImportBrowserV2Component.ngOnInit() - Implement as a route guard (
canActivate) that checks popout conditions - Keep current approach but add JSDoc explaining the design choice
The current approach works but is unusual compared to typical Angular patterns.
…en-after-closing-extension-pop-up-and-selecting-choose-file
| <i bitDialogIcon class="bwi bwi-info-circle tw-text-info tw-text-3xl" aria-hidden="true"></i> | ||
| <ng-container bitDialogContent> | ||
| <div bitTypography="h3"> | ||
| {{ "sendFilePopoutDialogText" | i18n }} |
There was a problem hiding this comment.
❌ Wrong i18n key
This uses sendFilePopoutDialogText (Send-specific) but line 7 correctly uses importPopoutDialogDesc (Import-specific). This creates inconsistent messaging.
Required fix:
| {{ "sendFilePopoutDialogText" | i18n }} | |
| {{ "importPopoutDialogTitle" | i18n }} |
Then add to messages.json:
"importPopoutDialogTitle": {
"message": "Pop out to import"
}
| await BrowserPopupUtils.openCurrentPagePopout(window); | ||
| } | ||
|
|
||
| // If the user selects "cancel" when presented the dialog, navigate back to the main Send tab |
There was a problem hiding this comment.
This comment references "Send tab" but this is the Import dialog component.
| // If the user selects "cancel" when presented the dialog, navigate back to the main Send tab | |
| // If the user selects "cancel" when presented the dialog, navigate back to the Vault Settings view |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22836?atlOrigin=eyJpIjoiNTUwZTI2NmZiMzRhNDIzNTlmMTAzMGJkZWZjZDhjOGMiLCJwIjoiaiJ9
📔 Objective
• Determine reason why extension crashes and fix this
Fix:
• Firefox has a known issue that is relevant here
• To avoid the crash it is necessary to only open modal dialogs (file selectors) from within a popout window
• The extension UI involving file type Sends already implemented this logic (mostly)
• File type Send logic was extended to the Import UI
• Both Send and Import UI logic were extended such that a user can't close the popout and still open a file selector in the original extension panel (file selector will NEVER show outside of popout)
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes