fix: open popup on the same display as the source window in multi-monitor setups#410
fix: open popup on the same display as the source window in multi-monitor setups#410Copilot wants to merge 23 commits into
Conversation
In a multi-display environment, getScreenSize() was calling chrome.windows.getCurrent() from the service worker context to determine which display to use for bounds-checking. This could return a window on the primary/main display rather than the display where the user was actually working, causing adjustWindowPosition() to reposition the popup to the main display. Fix: add an optional `hint` parameter to getScreenSize(). When a hint (top, left) is provided, use those coordinates directly to find the correct display. Callers openPopupWindow() and openPopupWindowMultiple() now pass the popup target coordinates as the hint, so the display where the user is working is always correctly identified. Add screen.test.ts with 6 tests covering single/multi-display scenarios. Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/c383e197-1956-435f-902e-69f98e6282bf Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
Drag events produce screen-absolute coordinates (screenX/screenY), so the same hint mechanism used in chrome.ts now applies here to correctly identify the display when clamping the popup position. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
==========================================
+ Coverage 27.84% 28.00% +0.16%
==========================================
Files 317 317
Lines 32448 32480 +32
Branches 1815 1840 +25
==========================================
+ Hits 9034 9097 +63
+ Misses 23414 23383 -31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add jlumbroso/free-disk-space@main before the Playwright browser install step to prevent extraction from hanging due to insufficient disk space. Also add timeout-minutes: 10 to the install step to fail fast if it hangs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The original assertion `toHaveBeenCalledTimes(1)` depended on `setInterval(fn, 100)` firing when advanced by exactly 100ms, which is a boundary condition that is flaky on Ubuntu/CI environments. Replace with a behavior-focused assertion that verifies postMessage was never called with a new command ID, which is the actual invariant the test needs to protect. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Separates system dependency installation (always runs) from Chromium binary download (cache miss only), removing --with-deps conflation and the timeout that caused the e2e job to hang. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the browser install steps (which hang on Node.js 24.16.0 due to an extract-zip bug) with the pre-built mcr.microsoft.com/playwright container image. Also removes the now-unnecessary free-disk-space and cache steps. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Acquire a CF_Authorization cookie via Service Token headers before navigating to the Hub URL, so hub.spec.ts tests pass through the Cloudflare Access authentication wall. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pass VITE_NEW_HUB_URL, CF_ACCESS_CLIENT_ID, and CF_ACCESS_CLIENT_SECRET from repository secrets so the e2e job can build the extension with the correct Hub URL and authenticate through Cloudflare Access at test time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… detection - useCommandHubBridge: use window.location.origin for postMessage to avoid silent discard when VITE_NEW_HUB_URL differs from the actual Hub URL (e.g. staging vs prod) - useCommandHubBridge: set data-extension-installed="true" on <html> after commands load, giving tests a reliable DOM signal that the content script is ready - hub.spec.ts: wait for html[data-extension-installed='true'] before clicking any download button so the Hub has received SyncInstalledCommand first; prevents the "Chrome extension required" dialog caused by clicking before the content script finishes its async chrome.storage read Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Always emit SW/page console logs in e2e (remove PWDEBUG guard) - Capture Hub page console in E2E-90 test - Log command count before/after download button click - Warn on origin mismatch in onMessageExternal (was silently dropped) - Debug-log command type and save result in handleAddCommand - Debug-log installed IDs sent from useCommandHubBridge Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add test boundary markers, Hub page console listeners for all tests, and per-step logging (URL, commands count, button attributes, poll state) so failures can be correlated with SW logs by test ID. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- fixtures.ts: attach SW console logging to every new service worker
instance via context.on("serviceworker"), not just the initial one.
MV3 SWs can be killed and restarted at any time; without this, all
logs from a restarted SW are silently dropped.
- background.ts: include hubOrigin value in initHubExternalListener
startup log so the next CI run shows whether the expected origin
matches the Hub page's actual origin.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Hub page's sendExtensionMessage uses chrome.runtime.sendMessage(id, ...) which requires knowing the extension ID. In production the ID is fixed, but in E2E tests the loaded extension gets a different ID, so the message is silently dropped and onMessageExternal is never triggered. By including extensionId: chrome.runtime.id in the SyncInstalledCommand window.postMessage, the Hub page can dynamically discover the correct extension ID and use it for AddCommand/DeleteCommand requests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Moving context.on("serviceworker") before serviceWorkers()/waitForEvent
ensures the very first SW startup logs (e.g. initHubExternalListener,
BgData initialized) are captured even if the SW has already started by
the time attachSWConsole is called on the direct reference.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
production/e2e ビルドでは env の EXTENSION_KEY を manifest の key に注入し、 yarn dev では key を省略することで拡張機能IDを分離する。 GitHub Actions の build/release ワークフローには EXTENSION_KEY シークレットを渡すよう追加。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8bd8202 to
562b851
Compare
…-attach, debug logs - cfAuth.ts: replace https.request with fetch (follows redirects, simpler cookie parsing) - background.test.ts SH-10: add toHaveBeenCalled() to catch zero-call regression masked by the existing double-negative not.toHaveBeenCalledWith assertion - fixtures.ts: prevent double attachSWConsole when SW starts after listener registration; explicit call now only runs when SW was already running at fixture init time - hub.spec.ts: remove 36 diagnostic console.log calls added during E2E-90 debugging Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SH-10 tests the case where updateCommandId fails after DUPLICATE_COMMAND_ID. In this scenario postMessage may legitimately be called 0 times (share stops), so toHaveBeenCalled() was wrong and caused a flaky CI failure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In multi-display environments, popup windows were always opening on the primary display instead of the display containing the source window.
Root Cause
getScreenSize()in the service worker context callschrome.windows.getCurrent()to determine which display to use for bounds-checking. From a service worker,getCurrent()unreliably returns the last-focused window — often one on the primary display — rather than the window the user was actually interacting with. This causedadjustWindowPosition()to recalculate the popup position using the wrong display's bounds, snapping it to the primary monitor.Changes
screen.ts: Add optionalhint?: { top: number; left: number }togetScreenSize(). When provided, the hint coordinates are used to identify the target display directly, bypassinggetCurrent(). Falls back to existinggetCurrent()behavior when no hint is given.chrome.ts: Pass the popup's target{ top, left }coordinates as the hint when callinggetScreenSize()in bothopenPopupWindowandopenPopupWindowMultiple. These coordinates already originate from the content script's correct window position, so they reliably identify the right display.screen.test.ts(new): Tests covering single/multi-display hint resolution, fallback to primary when hint is out-of-bounds, and non-service-worker path.