Skip to content

fix: stabilize selection capture target tab#82

Merged
olgahaha merged 3 commits into
mainfrom
fix/selection-mode-injection
May 6, 2026
Merged

fix: stabilize selection capture target tab#82
olgahaha merged 3 commits into
mainfrom
fix/selection-mode-injection

Conversation

@numbers-official
Copy link
Copy Markdown
Collaborator

@numbers-official numbers-official commented May 6, 2026

Summary

  • Capture the popup-selected tab/window when the user starts a screenshot capture
  • Pass the explicit target tab to the background service worker instead of re-resolving currentWindow
  • Preserve the pending selection target through overlay injection and selection completion
  • Preserve popup/non-popup state when replacing an existing pending selection

Why

Selection mode could fail with Failed to start selection mode. Make sure you are on a valid web page. because the service worker re-resolved the active tab after focus moved to the extension popup. The popup now sends the intended target tab explicitly, and the background keeps that same target through the selection completion flow.

This also avoids a follow-up regression where replacing an existing pending selection could reset the new capture's fromPopup state.

Asset edit modal and auto-upload behavior

  • Popup-initiated captures send fromPopup: true
  • Popup captures are saved as drafts and open the asset edit modal for headline/caption
  • Register to Numbers saves metadata first, then queues upload via UPLOAD_ASSET
  • Non-popup captures still respect settings.autoUpload && !fromPopup, so auto-upload after capture remains intact

Verification

  • npm run lint (0 errors; existing warnings only)
  • npx tsc --noEmit
  • npm run build
  • GitHub CI passes

@numbers-official
Copy link
Copy Markdown
Collaborator Author

Code review note: PR 82 stabilizes the tab used for injecting the selection overlay, but handleSelectionComplete() still re-resolves the active tab with currentWindow before calling captureVisibleTab(). That leaves the same class of focus/window ambiguity in the second half of the flow: the overlay can be injected into the intended tab, then the final screenshot can be taken from a different active tab/window if focus changes while selecting.\n\nPlease carry the pending selection target tab/window through to completion and use that for captureVisibleTab() and source metadata.

@numbers-official
Copy link
Copy Markdown
Collaborator Author

Updated in commit e8c41fb. Selection completion now reuses the pending target tab captured when the overlay is injected, clears that target on success/cancel/timeout/error, and no longer re-resolves the active tab with currentWindow before captureVisibleTab.

@numbers-official numbers-official force-pushed the fix/selection-mode-injection branch from e8c41fb to 034a47c Compare May 6, 2026 04:05
@numbers-official
Copy link
Copy Markdown
Collaborator Author

Follow-up: amended the fix into commit 034a47c to also clear pending selection state if overlay injection fails before the pending promise is created.

Copy link
Copy Markdown
Collaborator Author

@numbers-official numbers-official left a comment

Choose a reason for hiding this comment

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

One remaining edge case: if a previous selection is still pending and the user starts a new popup-initiated selection, sets before calling , but may immediately call , which resets back to . The new selection then continues with the wrong popup/non-popup state, so it can incorrectly show the auto-upload/notification behavior for a popup flow.\n\nA small fix would be to pass into and assign only after rejecting any existing pending selection, together with .

@numbers-official
Copy link
Copy Markdown
Collaborator Author

Correction for the review note above: one remaining edge case is when a previous selection is still pending and the user starts a new popup-initiated selection.

handleScreenshotCapture() sets pendingSelectionFromPopup = fromPopup before calling handleSelectionCapture(), but handleSelectionCapture() may immediately call rejectPendingSelection(), which resets pendingSelectionFromPopup back to false. The new selection then continues with the wrong popup/non-popup state, so it can incorrectly show the auto-upload/notification behavior for a popup flow.

A small fix would be to pass fromPopup into handleSelectionCapture() and assign pendingSelectionFromPopup only after rejecting any existing pending selection, together with pendingSelectionTarget.

@numbers-official
Copy link
Copy Markdown
Collaborator Author

Updated in commit 934be84: fromPopup is now passed into handleSelectionCapture() and assigned only after any existing pending selection has been rejected, so rejectPendingSelection() cannot clear the new popup/non-popup state. I also rechecked the other pending selection reset paths; no similar ordering issue remains.

@numbers-official
Copy link
Copy Markdown
Collaborator Author

Re-reviewed commit 934be84. No further regression issues found.

I also checked the popup capture flow against AssetEditModal and auto-upload-after-capture:

  • popup capture sends fromPopup: true, so background saves the asset as a draft and suppresses auto-upload while the edit modal is shown;
  • Register to Numbers saves headline/caption first, then queues the asset via UPLOAD_ASSET;
  • non-popup capture still uses settings.autoUpload && !fromPopup, so the existing auto-upload-after-capture path remains intact.

Local lint, tsc --noEmit, and npm run build pass; GitHub CI is green.

@olgahaha olgahaha merged commit ebf34f5 into main May 6, 2026
1 check passed
@olgahaha olgahaha deleted the fix/selection-mode-injection branch May 6, 2026 05:48
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.

2 participants