Skip to content

fix: open popup on the same display as the source window in multi-monitor setups#410

Draft
Copilot wants to merge 23 commits into
mainfrom
copilot/fix-popup-window-display-issue
Draft

fix: open popup on the same display as the source window in multi-monitor setups#410
Copilot wants to merge 23 commits into
mainfrom
copilot/fix-popup-window-display-issue

Conversation

Copilot AI commented May 26, 2026

Copy link
Copy Markdown
Contributor

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 calls chrome.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 caused adjustWindowPosition() to recalculate the popup position using the wrong display's bounds, snapping it to the primary monitor.

Changes

  • screen.ts: Add optional hint?: { top: number; left: number } to getScreenSize(). When provided, the hint coordinates are used to identify the target display directly, bypassing getCurrent(). Falls back to existing getCurrent() behavior when no hint is given.

  • chrome.ts: Pass the popup's target { top, left } coordinates as the hint when calling getScreenSize() in both openPopupWindow and openPopupWindowMultiple. These coordinates already originate from the content script's correct window position, so they reliably identify the right display.

// Before
const screenSize = await getScreenSize()

// After — use popup target coordinates to find the correct display
const screenSize = await getScreenSize({ top, left })
  • 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.

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>
Copilot AI changed the title [WIP] Fix popup window opening on main display in multi-display setup fix: open popup on the same display as the source window in multi-monitor setups May 26, 2026
Copilot AI requested a review from ujiro99 May 26, 2026 04:08
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

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 79.71014% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 28.00%. Comparing base (a8b8dc9) to head (cbb656c).
⚠️ Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
packages/extension/src/services/hub/background.ts 86.66% 6 Missing ⚠️
...ackages/extension/src/hooks/useCommandHubBridge.ts 0.00% 5 Missing ⚠️
packages/extension/src/services/chrome.ts 0.00% 2 Missing ⚠️
...ckages/extension/src/hooks/useDetectLinkCommand.ts 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ujiro99 and others added 13 commits June 17, 2026 22:17
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>
@gitguardian

gitguardian Bot commented Jun 24, 2026

Copy link
Copy Markdown

️✅ 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.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 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>
@ujiro99 ujiro99 force-pushed the copilot/fix-popup-window-display-issue branch from 8bd8202 to 562b851 Compare June 24, 2026 13:24
ujiro99 and others added 4 commits June 24, 2026 22:37
…-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>
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