From a4e65cfb871492bc8e8a9449cff446c801bb065c Mon Sep 17 00:00:00 2001
From: Hitesh Kumar <hik@microsoft.com>
Date: Sat, 9 May 2026 17:46:17 +0530
Subject: [PATCH] fix: rename `let app` in firebaseConfig.js to stop shadowing
 window.app in inline event handlers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Found via Playwright interaction-driver in the staging clone repo
(DDancingDeath/AadhatManagementApp-staging). Full RCA: STG-WALK-5 in
that repo's docs/REVIEW_ISSUES.md.

## The bug

firebaseConfig.js declared `let app = firebase.initializeApp(...)` at
the top level of a classic <script>. In that context, `let app`
creates a binding in the **global lexical environment** that lives
"above" `window` in the scope chain inline event handlers see. So
`onclick="app.X()"` in any HTML template resolves `app` to the
**Firebase App instance** (which has no `.X` method) — even though
`window.app.X` is defined and works fine from console / module code.

The shadow is invisible from DevTools (`window.app` looks correct) so
the bug is silent: every inline `onclick="app.foo()"` button throws
`Cannot read properties of undefined (reading 'foo')` and does
nothing.

In the staging repo this took out the entire AI Assistant feature
(every quick-question button + the Send button). Once the AI
Assistant lands in prod (separate work), the same shadow would break
it instantly.

## The fix

1. Rename `let app` -> `let firebaseApp` in firebaseConfig.js (4 lines
   touched). 16-line warning comment added above the declaration so
   future maintainers don't accidentally rename it back.
2. New `www/js/__tests__/firebase-app-shadow.test.js` Jest test that
   FAILS if any future commit reintroduces `let|const|var app` as a
   top-level binding in firebaseConfig.js. The test also asserts the
   Firebase init result is bound to a name other than `app`.

## What does NOT change

- No HTML template was modified. All existing inline handlers in prod
  already use `window.app.nav.X()` form (see navigation.html), so
  this rename is a pure no-op for the running app — nothing was
  resolving against the shadowed binding.
- No public API. `window.app` continues to be set by main.js exactly
  as before. The `auth`, `db`, `firebase` exports of firebaseConfig.js
  are unchanged.
- No build, deploy, or service worker change.

## Testing

- `npx jest`: 105/105 passing (was 103; +2 new regression tests)
- Manual: opened the deployed staging URL with the same fix applied,
  every existing inline `window.app.nav.showTab(...)` continues to
  work; AI Assistant Send button — broken in staging by this exact
  bug — now sends and receives real replies.

## Why this is a separate PR (not bundled with the chat fix)

The chat module itself doesn't exist in prod (WALK-1: AI Assistant
non-functional). Porting the staging chat module is a much larger
follow-up. This PR ships ONLY the firebaseConfig.js shadowing fix +
its regression test, so the future chat port doesn't have to also
debug a years-old SDK shadowing trap.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 www/firebaseConfig.js                        | 21 +++++-
 www/js/__tests__/firebase-app-shadow.test.js | 68 ++++++++++++++++++++
 2 files changed, 86 insertions(+), 3 deletions(-)
 create mode 100644 www/js/__tests__/firebase-app-shadow.test.js

diff --git a/www/firebaseConfig.js b/www/firebaseConfig.js
index e60f98d..a0e1861 100644
--- a/www/firebaseConfig.js
+++ b/www/firebaseConfig.js
@@ -95,12 +95,27 @@ if (APP_ENV !== 'production') {
 }
 
 // Initialize Firebase
-let app;
+//
+// IMPORTANT: variable is intentionally NOT named `app`. A `let app`
+// declared at the top level of a classic <script> creates a binding in
+// the global lexical environment that SHADOWS `window.app` for inline
+// event handlers (`onclick="app.x()"`) — even though it is not visible
+// as a `window.app` property in DevTools. That broke every
+// `onclick="app.chat.X()"` button in www/templates/chat.html: the AI
+// Assistant Send button silently threw `Cannot read properties of
+// undefined (reading 'sendFromInput')` because inline handlers
+// resolved `app` to *this* Firebase App instance instead of the
+// application's `window.app` object.
+//
+// Found via Playwright interaction-driver in the staging clone repo
+// (DDancingDeath/AadhatManagementApp-staging). Full RCA in that repo's
+// docs/REVIEW_ISSUES.md → STG-WALK-5.
+let firebaseApp;
 try {
-  app = firebase.initializeApp(firebaseConfig);
+  firebaseApp = firebase.initializeApp(firebaseConfig);
 } catch (error) {
   if (error.code === 'app/duplicate-app') {
-    app = firebase.app();
+    firebaseApp = firebase.app();
   } else {
     console.error('Firebase initialization error:', error);
     throw error;
diff --git a/www/js/__tests__/firebase-app-shadow.test.js b/www/js/__tests__/firebase-app-shadow.test.js
new file mode 100644
index 0000000..bba6d26
--- /dev/null
+++ b/www/js/__tests__/firebase-app-shadow.test.js
@@ -0,0 +1,68 @@
+/**
+ * Regression test for STG-WALK-5 — pinned 2026-01-08.
+ *
+ * `firebaseConfig.js` is loaded as a *classic* (non-module) `<script>`. In
+ * that context, `let app` or `const app` at the top level creates a binding
+ * in the **global lexical environment** that lives "above" `window` in the
+ * scope chain seen by inline event handlers (`onclick="app.x()"`).
+ *
+ * That binding then SHADOWS `window.app` — even though it is *not* exposed
+ * as `window.app` to JavaScript code. So `typeof window.app === 'object'`
+ * and `window.app.chat.sendFromInput` is a function, but inline handlers
+ * resolve `app` to the Firebase App instance instead. Result: every
+ * `onclick="app.chat.X()"` button silently throws "Cannot read properties
+ * of undefined (reading 'X')" because the Firebase App has no `.chat`.
+ *
+ * This bug took out the entire AI Assistant feature without anyone
+ * noticing because `chat-render.test.js` only string-matches the HTML —
+ * it doesn't actually click anything.
+ *
+ * To prevent recurrence:
+ *   - Don't name any top-level binding in any `<script>` tag `app`.
+ *   - chat.html (and any future inline handlers) must use `window.app.*`.
+ */
+const fs = require('fs');
+const path = require('path');
+
+const STAGING_ROOT = path.resolve(__dirname, '..', '..', '..');
+
+describe('firebaseConfig.js — STG-WALK-5 regression', () => {
+    let src;
+    beforeAll(() => {
+        src = fs.readFileSync(
+            path.join(STAGING_ROOT, 'www/firebaseConfig.js'),
+            'utf8'
+        );
+    });
+
+    test('does NOT use `let app` or `const app` at top level (would shadow window.app for inline handlers)', () => {
+        // Match `let app` / `const app` / `var app` *only when followed by
+        // = / ; / EOL* — i.e. the declaration form. Excludes accidental
+        // matches inside strings or comments via simple heuristics.
+        const lines = src.split(/\r?\n/);
+        const offenders = [];
+        lines.forEach((line, i) => {
+            // Strip line comments.
+            const code = line.replace(/\/\/.*$/, '');
+            if (/^\s*(?:let|const|var)\s+app\s*[;=]/.test(code)) {
+                offenders.push((i + 1) + ': ' + line.trim());
+            }
+        });
+        if (offenders.length) {
+            throw new Error(
+                'firebaseConfig.js declares a top-level `app` binding. This shadows ' +
+                '`window.app` for inline event handlers (`onclick="app.x()"`) and ' +
+                'breaks the AI Assistant Send button. Rename to `firebaseApp` or ' +
+                'similar. Offenders:\n' + offenders.join('\n')
+            );
+        }
+    });
+
+    test('Firebase init result is bound to a name OTHER than `app`', () => {
+        // Sanity: the file does still call initializeApp(), and assigns its
+        // result to *some* variable.
+        expect(src).toMatch(/firebase\.initializeApp\s*\(/);
+        // A common safe name; this test just pins our convention.
+        expect(src).toMatch(/(?:let|const|var)\s+(?:firebaseApp|fbApp|_app)\b/);
+    });
+});