Feat: Send push notification when human input is required but browser is not in focus

[jayshankarshahu]

Feature Details

1. Show notification when human input is required but browser is not in focus
2. Click the notification to open the browser
3. Opening the browser clears all the pending notifications

[jayshankarshahu]

I have read the CLA Document and I hereby sign the CLA

[github-actions]

CLA Assistant Lite bot ✅ All contributors have signed the CLA. Thank you for helping make BrowserOS better!
_{Posted by the CLA Assistant Lite bot.}

[jayshankarshahu]

recheck

[jayshankarshahu]

recheck

[jayshankarshahu]

I have read the CLA Document and I hereby sign the CLA

[felarof99]

@greptileai

[jayshankarshahu]

Greptile encountered an error while reviewing this PR. Please reach out to [email protected] for assistance.

Why does this occur? I sent an email but no reply yet. Does anyone know?

[jayshankarshahu]

@greptileai

[jayshankarshahu]

@greptileai
Why does this keep failing ?

[greptile-apps]

I don't see any obvious reasons why this implementation would fail. The code looks structurally sound. However, let me point out a few potential issues that could cause problems:

1. Notification permissions - The notifications permission is added to the manifest, but Chrome should grant this automatically for extensions. This shouldn't fail unless there's a browser-level issue.

2. document.hidden reliability - You've already noted this in your comment that it may have incorrect values in some Linux distros. This could cause notifications to not appear when expected.

3. Async notification creation - In usePushNotification.ts, you're using async in the Promise constructor callback which is redundant (line 11 & 13), but shouldn't cause failures.

4. Window ID might be undefined - In index.ts line 399, you're using window.id! with a non-null assertion. If getCurrent fails or returns a window without an ID, this could throw.

Are you experiencing specific failures? If so, what error messages are you seeing? That would help me diagnose the actual issue. Otherwise, the implementation looks reasonable and should work in most cases.

[jayshankarshahu]

@greptileai review this pr

[jayshankarshahu]

@greptileai why does reviewing this pr never work?

[greptile-apps]

I apologize for the frustration! Let me look at the PR changes to help identify any issues.

Looking at your PR, the code appears to have several issues that could cause problems:

Main Issues:

1. Missing semicolons - Your code is inconsistent with semicolon usage, which could cause parsing issues in some contexts.

2. Incorrect notification options type - In usePushNotification.ts, you're using chrome.notifications.NotificationOptions<true> which isn't the correct TypeScript type. It should just be chrome.notifications.NotificationOptions.

3. Race condition in onFocusChanged - The listener clears notifications immediately when focus changes, but this fires even when switching between browser windows/tabs, which might clear notifications prematurely.

4. Unnecessary Promise wrapper in sendNotification - The chrome.notifications.create already works with callbacks, wrapping it in a Promise with async is redundant.

5. Memory leak potential - Event listeners in registerNotificationListeners() are registered globally but never cleaned up.

6. Icon path issue - 'assets/icon48.png' might not resolve correctly in the extension context. It should probably be chrome.runtime.getURL('assets/icon48.png').

Regarding "why reviewing this PR never works" - if you mean the automated review:

• The mix of formatting changes (whitespace) with functional changes makes diffs harder to parse
• Missing type definitions could cause TypeScript analysis to fail
• The linting issues might cause the review pipeline to timeout or error

Would you like me to provide a corrected version of any specific file?

[jayshankarshahu]

@greptileai review the latest commit

[jayshankarshahu]

I apologize for the frustration! Let me look at the PR changes to help identify any issues.

Looking at your PR, the code appears to have several issues that could cause problems:

Main Issues:

1. Missing semicolons - Your code is inconsistent with semicolon usage, which could cause parsing issues in some contexts.
2. Incorrect notification options type - In usePushNotification.ts, you're using chrome.notifications.NotificationOptions<true> which isn't the correct TypeScript type. It should just be chrome.notifications.NotificationOptions.
3. Race condition in onFocusChanged - The listener clears notifications immediately when focus changes, but this fires even when switching between browser windows/tabs, which might clear notifications prematurely.
4. Unnecessary Promise wrapper in sendNotification - The chrome.notifications.create already works with callbacks, wrapping it in a Promise with async is redundant.
5. Memory leak potential - Event listeners in registerNotificationListeners() are registered globally but never cleaned up.
6. Icon path issue - 'assets/icon48.png' might not resolve correctly in the extension context. It should probably be chrome.runtime.getURL('assets/icon48.png').

Regarding "why reviewing this PR never works" - if you mean the automated review:

• The mix of formatting changes (whitespace) with functional changes makes diffs harder to parse
• Missing type definitions could cause TypeScript analysis to fail
• The linting issues might cause the review pipeline to timeout or error

Would you like me to provide a corrected version of any specific file?

@greptileai I have made the changes addressing these problems , please check

[greptile-apps]

Looking at your updated code, I can see you've addressed most of the issues! However, there are still a few problems:

Remaining Issues:

1. In src/background/index.ts line 405 - You have a space in the condition:

   if( !notificationIds.has(windowId) ) {

   Should be:

   if (!notificationIds.has(windowId)) {

2. Map iteration efficiency - In the onFocusChanged listener (lines 417-423), you're iterating over all notification IDs to clear them. This works but could be optimized.

3. Inconsistent spacing - Throughout the file you have inconsistent spacing after if statements and around parentheses. For example:

   • Line 396: if( windowId )
   • Line 382: if (windowId == chrome.windows.WINDOW_ID_NONE)

4. Type safety - In usePushNotification.ts, the type is correct now, but you're not handling the case where chrome.windows.getCurrent() might fail to get a window ID.

5. Redundant check - Line 407 in src/background/index.ts:

   const windowId = windowIds.get(notificationId);
   
   if( windowId ) {

   The map lookup already returns undefined if not found, so you should use strict equality:

   if (windowId !== undefined) {

Most Critical Issue:
The document.hidden check in src/sidepanel/App.tsx (line 107) might not work reliably. The sidepanel's visibility state is separate from the browser window focus. You should check if the window has focus, not if the document is hidden:

const { id: windowId } = await chrome.windows.getCurrent();
const window = await chrome.windows.get(windowId);
if (humanInputRequest && !window.focused) {
  // send notification
}

Would you like me to provide complete corrected versions of the affected sections?

[jayshankarshahu]

Looking at your updated code, I can see you've addressed most of the issues! However, there are still a few problems:

Remaining Issues:

1. In src/background/index.ts line 405 - You have a space in the condition:

   if( !notificationIds.has(windowId) ) {

   Should be:

   if (!notificationIds.has(windowId)) {

2. Map iteration efficiency - In the onFocusChanged listener (lines 417-423), you're iterating over all notification IDs to clear them. This works but could be optimized.

3. Inconsistent spacing - Throughout the file you have inconsistent spacing after if statements and around parentheses. For example:

   • Line 396: if( windowId )
   • Line 382: if (windowId == chrome.windows.WINDOW_ID_NONE)

4. Type safety - In usePushNotification.ts, the type is correct now, but you're not handling the case where chrome.windows.getCurrent() might fail to get a window ID.

5. Redundant check - Line 407 in src/background/index.ts:

   const windowId = windowIds.get(notificationId);
   
   if( windowId ) {

   The map lookup already returns undefined if not found, so you should use strict equality:

   if (windowId !== undefined) {

Most Critical Issue: The document.hidden check in src/sidepanel/App.tsx (line 107) might not work reliably. The sidepanel's visibility state is separate from the browser window focus. You should check if the window has focus, not if the document is hidden:

const { id: windowId } = await chrome.windows.getCurrent();
const window = await chrome.windows.get(windowId);
if (humanInputRequest && !window.focused) {
  // send notification
}

Would you like me to provide complete corrected versions of the affected sections?

@greptileai I have made the necessary changes in the latest commit

[greptile-apps]

Great! I can see you've addressed the critical issues. Let me verify the changes you made:

✅ Fixed:

1. Spacing issues - You've corrected the inconsistent spacing around if statements
2. Document visibility check - You've replaced document.hidden with window.focused check using Chrome APIs
3. Type safety - Added proper undefined checks

However, I still see a couple of minor issues in src/background/index.ts:

Line 398 - The getNotificationIds function has unnecessary spacing:

const getNotificationIds = (windowId: number): Array<string> => {

    if( !notificationIds.has(windowId) ) {
      return [];
    }

    return notificationIds.get(windowId)!;

  }

The spacing after if and around the parentheses should be consistent with the rest of the file.

Line 433 - In the listener, you have:

chrome.notifications.create(request.options, async (notificationId) => {

The callback doesn't need to be async since you're not using await inside it.

Line 436 - Redundant check:

let existingNotificationIds = getNotificationIds(windowId!);

You're already checking if (windowId) on line 432, so you don't need the non-null assertion operator ! here.

These are minor style issues that don't affect functionality. The core logic looks good now! The notification system should work correctly:

• ✅ Notifications sent when browser loses focus and human input is needed
• ✅ Clicking notification brings window back into focus
• ✅ All notifications cleared when window regains focus
• ✅ Proper type safety with undefined checks

Would you like me to provide suggestions for these minor style improvements?