From b7002c0fe3d744d65197f6346c4761cff0820c34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o?= Date: Wed, 30 Sep 2026 09:55:29 +0200 Subject: [PATCH] fix(email): import spinnerModule in the modules that use it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six of the seven modules call `spinnerModule.createWhirlpool(...)` and none of them imported it. Everything still parsed, every module still loaded, and the Email Settings page threw `ReferenceError: spinnerModule is not defined` the moment `settings.js` mounted it — the panel rendered its loading spinner and stopped there. The cause is worth writing down because it is the same shape as the bug: the import statements were collected with a pattern that was not anchored to the start of a line, and the header comment says "the old import path keeps resolving". That prose matched first and swallowed the real `import spinnerModule from '../spinner.js';` that followed it. `test_no_module_uses_a_package_name_it_never_bound` closes the class. Nothing else here can: `node --check` parses without resolving scope, and loading a module does not run the function body where the throw lives. It checks the package's own vocabulary — every name any module in it binds — rather than trying to model the browser's globals, so it has no false positives and still catches the one mistake a split actually makes: the declaration stays behind and the use moves. --- static/js/emailLibrary/aiReply.js | 1 + static/js/emailLibrary/attachments.js | 1 + static/js/emailLibrary/menus.js | 1 + static/js/emailLibrary/reader.js | 1 + static/js/emailLibrary/settingsPage.js | 1 + static/js/emailLibrary/unsubscribe.js | 1 + tests/test_email_library_module_graph_js.py | 78 +++++++++++++++++++++ 7 files changed, 84 insertions(+) diff --git a/static/js/emailLibrary/aiReply.js b/static/js/emailLibrary/aiReply.js index bbbcb0e86..3133afaf0 100644 --- a/static/js/emailLibrary/aiReply.js +++ b/static/js/emailLibrary/aiReply.js @@ -8,6 +8,7 @@ // The context-draft key is versioned (`…:v2:`) and scoped by account, folder // and uid, so a draft cannot leak from one message to another. +import spinnerModule from '../spinner.js'; import { topPortalZ } from '../toolWindowZOrder.js'; import { showToast } from '../ui.js?v=20260916largetoolscroll1'; import { state } from './state.js'; diff --git a/static/js/emailLibrary/attachments.js b/static/js/emailLibrary/attachments.js index deab2981d..1af364e15 100644 --- a/static/js/emailLibrary/attachments.js +++ b/static/js/emailLibrary/attachments.js @@ -8,6 +8,7 @@ // metadata: a card can know it *has* attachments without knowing what they // are, so the chips are rendered late and the card icon repaired afterwards. +import spinnerModule from '../spinner.js'; import * as Modals from '../modalManager.js'; import { state } from './state.js'; import { _esc } from './utils.js'; diff --git a/static/js/emailLibrary/menus.js b/static/js/emailLibrary/menus.js index 94352f43c..78a5cc32f 100644 --- a/static/js/emailLibrary/menus.js +++ b/static/js/emailLibrary/menus.js @@ -9,6 +9,7 @@ // document, task, session and memory menus. `tests/test_action_menu_order.py` // pins that. +import spinnerModule from '../spinner.js'; import { SELECT_MENU_ICON, actionMenuRank, orderActionMenuItems } from '../actionMenuOrder.js'; import { bindMenuDismiss, dismissOrRemove } from '../escMenuStack.js'; import { topPortalZ } from '../toolWindowZOrder.js'; diff --git a/static/js/emailLibrary/reader.js b/static/js/emailLibrary/reader.js index 0f13581af..b4765f870 100644 --- a/static/js/emailLibrary/reader.js +++ b/static/js/emailLibrary/reader.js @@ -9,6 +9,7 @@ // tab 2 until it closes, even if tab 1 closes first. The slot map here is what // holds that. +import spinnerModule from '../spinner.js'; import * as Modals from '../modalManager.js'; import { showToast } from '../ui.js?v=20260916largetoolscroll1'; import { state } from './state.js'; diff --git a/static/js/emailLibrary/settingsPage.js b/static/js/emailLibrary/settingsPage.js index 406b1f166..a5283158a 100644 --- a/static/js/emailLibrary/settingsPage.js +++ b/static/js/emailLibrary/settingsPage.js @@ -10,6 +10,7 @@ // delete sync against /api/calendar. And the inline-image preference is read by // ./bodyRender.js while rendering a message, not only by the settings form. +import spinnerModule from '../spinner.js'; import { emailApiUrl } from '../emailShared.js'; import { showToast } from '../ui.js?v=20260916largetoolscroll1'; import { state } from './state.js'; diff --git a/static/js/emailLibrary/unsubscribe.js b/static/js/emailLibrary/unsubscribe.js index 37efe3208..c359e939f 100644 --- a/static/js/emailLibrary/unsubscribe.js +++ b/static/js/emailLibrary/unsubscribe.js @@ -10,6 +10,7 @@ // agent-browser run finish. Nothing else in the package reads any of that, // which is why this is the one module with a single exported entry point. +import spinnerModule from '../spinner.js'; import { emailApiUrl } from '../emailShared.js'; import { showToast, styledConfirm } from '../ui.js?v=20260916largetoolscroll1'; import { state } from './state.js'; diff --git a/tests/test_email_library_module_graph_js.py b/tests/test_email_library_module_graph_js.py index aa2c7411b..3cd2566e4 100644 --- a/tests/test_email_library_module_graph_js.py +++ b/tests/test_email_library_module_graph_js.py @@ -127,6 +127,84 @@ def test_wrapper_exposes_every_statically_imported_name(): assert checked, "no module imports names from static/js/emailLibrary.js" +_IMPORT_STATEMENT = re.compile( + r"^import\s+(\{[^}]*\}|\*\s+as\s+[\w$]+|[\w$]+)\s+from\s+'[^']+';", re.M | re.S +) +_TOP_LEVEL_DECL = re.compile( + r"^(?:export\s+)?(?:async\s+)?(?:function|const|let|var)\s+([A-Za-z_$][\w$]*)", re.M +) + + +def _bound_names(source: str) -> set[str]: + """Local names a module binds: imports plus top-level declarations.""" + names = set(_TOP_LEVEL_DECL.findall(source)) + for clause in _IMPORT_STATEMENT.findall(source): + clause = clause.strip() + if clause.startswith("{"): + names |= { + part.strip().split(" as ")[-1].strip() + for part in clause.strip("{}").split(",") + if part.strip() + } + else: + names.add(clause.split(" as ")[-1].strip()) + return names + + +_REEXPORT = re.compile(r"^export\s*\{[^}]*\}\s*from\s*'[^']+';", re.M | re.S) +_WHOLE_LINE_COMMENT = re.compile(r"^\s*(?://|/\*|\*/|\*(?!/)).*$", re.M) + + +def _executable_body(source: str) -> str: + """The part of a module that actually runs a name. + + Imports are dropped because a name in an import clause is the binding, not a + use. Re-export lists go for the same reason. Whole-line comments go because + prose says things like "no shared state" and `state` is a real binding here; + only full lines are stripped, so a `//` inside a URL literal is left alone. + """ + ends = [m.end() for m in _IMPORT_STATEMENT.finditer(source)] + body = source[max(ends):] if ends else source + body = _REEXPORT.sub("", body) + return _WHOLE_LINE_COMMENT.sub("", body) + + +def test_no_module_uses_a_package_name_it_never_bound(): + """A name used but never imported is a runtime ReferenceError, nothing less. + + It is invisible to `node --check`, which parses without resolving scope, and + invisible to loading the module, because the throw happens inside a function + body the loader never calls. It is also the single most likely mistake when + code moves between modules — the declaration stays behind and the use comes + along. + + The vocabulary checked is the package's own: every name any module here + binds. That is narrow on purpose. It is not a general no-undef pass, so it + never has to model the browser's globals and never produces a false + positive; it catches exactly the shape a split produces. + """ + sources = {p.name: p.read_text(encoding="utf-8") for p in email_library_paths(True)} + vocabulary: set[str] = set() + for source in sources.values(): + vocabulary |= _bound_names(source) + + unbound: dict[str, list[str]] = {} + for name, source in sources.items(): + bound = _bound_names(source) + body = _executable_body(source) + missing = sorted( + word + for word in vocabulary - bound + if re.search(r"(?