fix: prefer the focused frame, bound model length, drop dead flag

background/background.js

@@ -10,6 +10,17 @@ const COMMAND_ID = "ask-chatgpt";
 // A commands invocation confers activeTab (ext-commands.js grants it before
 // firing onCommand), so executeScript works with no host permission.
 //
+// Every frame keeps its own independent Selection, and a selection made in a
+// non-focused frame is not cleared when the user selects elsewhere. Without a
+// filter, a stale selection in an unfocused (possibly cross-origin) iframe
+// could win over — or be pulled alongside — the selection the user actually
+// made, with the winner among multiple non-empty results decided by an
+// executeScript ordering that isn't specified. The injected snippet is
+// therefore gated on document.hasFocus(), which is true for the focused
+// document and all of its ancestors: the top frame wins when the user
+// selected there, and an iframe's selection is only picked up when it is
+// itself the focused frame.
+//
 // Failure has three measured shapes, all handled here: a thrown
 // "Missing host permission for the tab", a thrown variant naming frames, and a
 // SILENT resolution to [null] on parent-process about: pages. Never index into
@@ -32,7 +43,7 @@ async function readSelection() {
   let results;
   try {
     results = await browser.tabs.executeScript(tabId, {
-      code: "window.getSelection().toString()",
+      code: "document.hasFocus() ? window.getSelection().toString() : ''",
       allFrames: true,
       matchAboutBlank: true,
     });

lib/defaults.js

@@ -4,6 +4,13 @@
 
 export const OPEN_IN_VALUES = ["new-tab", "background-tab", "new-window"];
 
+// An unbounded model string can, on its own, exceed MAX_URL_CHARS in
+// lib/build-url.js — fitToBudget() only ever trims `q`, so a long enough
+// model silently pushes the whole URL over budget and drops the query
+// entirely with no signal to the user. 200 is generous for any real model
+// slug while keeping the URL budget meaningful.
+export const MAX_MODEL_CHARS = 200;
+
 export const DEFAULT_SETTINGS = {
   model: "",
   temporaryChat: false,

lib/settings.js

@@ -1,4 +1,4 @@
-import { DEFAULT_SETTINGS, OPEN_IN_VALUES } from "./defaults.js";
+import { DEFAULT_SETTINGS, OPEN_IN_VALUES, MAX_MODEL_CHARS } from "./defaults.js";
 
 // Pure. Layers stored values over the defaults, dropping unknown keys and
 // falling back on wrong types or out-of-range values. A fresh or signed-out
@@ -8,7 +8,7 @@ export function mergeSettings(stored) {
   if (stored === null || typeof stored !== "object") {
     return merged;
   }
-  if (typeof stored.model === "string") {
+  if (typeof stored.model === "string" && stored.model.length <= MAX_MODEL_CHARS) {
     merged.model = stored.model;
   }
   if (typeof stored.temporaryChat === "boolean") {

options/options.html

@@ -9,7 +9,7 @@
 <form id="settings" autocomplete="off">
   <div class="field">
     <label for="model">Model <span class="tag">experimental</span></label>
-    <input type="text" id="model" placeholder="auto" spellcheck="false" />
+    <input type="text" id="model" placeholder="auto" spellcheck="false" maxlength="200" />
     <p class="hint">
       ChatGPT may ignore this and use your account default. Leave blank to
       always use your account default.

options/options.js

@@ -8,12 +8,8 @@ const field = (id) => document.getElementById(id);
 
 let saveTimer = null;
 let statusTimer = null;
-// Suppresses the change/input handlers while we are programmatically writing
-// values into the form, so a remote sync cannot trigger a save loop.
-let applying = false;
 
 function render(settings) {
-  applying = true;
   for (const [id, value] of Object.entries(settings)) {
     const element = field(id);
     // Never overwrite the control the user is actively editing — this page's
@@ -27,7 +23,6 @@ function render(settings) {
       element.value = value;
     }
   }
-  applying = false;
 }
 
 function collect() {
@@ -60,9 +55,6 @@ async function persist(settings) {
 
 // Debounced so typing in a text field cannot approach any write-rate quota.
 function scheduleSave() {
-  if (applying) {
-    return;
-  }
   clearTimeout(saveTimer);
   saveTimer = setTimeout(() => persist(collect()), SAVE_DEBOUNCE_MS);
 }

test/settings.test.js

@@ -1,7 +1,7 @@
 import test from "node:test";
 import assert from "node:assert/strict";
 
-import { DEFAULT_SETTINGS } from "../lib/defaults.js";
+import { DEFAULT_SETTINGS, MAX_MODEL_CHARS } from "../lib/defaults.js";
 import { mergeSettings } from "../lib/settings.js";
 
 test("empty object yields the defaults", () => {
@@ -78,3 +78,17 @@ test("mergeSettings does not mutate DEFAULT_SETTINGS", () => {
   mergeSettings({ model: "mutated" });
   assert.equal(DEFAULT_SETTINGS.model, "");
 });
+
+// An unbounded model string can, on its own, push the finished URL over
+// MAX_URL_CHARS (lib/build-url.js), which silently drops `q` — the user's
+// selection vanishes with no signal. mergeSettings() must bound `model` the
+// same way it already bounds an out-of-range openIn: fall back to the default.
+test("a model at exactly MAX_MODEL_CHARS is accepted", () => {
+  const model = "m".repeat(MAX_MODEL_CHARS);
+  assert.equal(mergeSettings({ model }).model, model);
+});
+
+test("a model one character over MAX_MODEL_CHARS falls back to the default", () => {
+  const model = "m".repeat(MAX_MODEL_CHARS + 1);
+  assert.equal(mergeSettings({ model }).model, DEFAULT_SETTINGS.model);
+});