docs: add expanded verifiable bug review artifacts

CODE_REVIEW.md

@@ -0,0 +1,171 @@
+# Senior Code Review — Verifiable Bugs
+
+## Scope
+Reviewed TypeScript source in `packages/scripts/src` for runtime bugs that can be verified from code paths and current behavior.
+
+---
+
+## 1) `ENGrid.deepMerge` throws when `target` is undefined
+- **Severity:** High (runtime crash)
+- **Location:** `packages/scripts/src/engrid.ts` (`deepMerge`)
+- **Why this is a bug:** The implementation dereferences `target[key]` before ensuring `target` exists. Passing `undefined` as `target` throws `TypeError`.
+
+### Reproduction
+```bash
+node - <<'NODE'
+const {ENGrid}=require('./packages/scripts/dist/engrid.js');
+try { console.log(ENGrid.deepMerge(undefined,{a:{b:1}})); }
+catch(e){ console.error('ERR', e.message); }
+NODE
+```
+
+### Actual result
+```text
+ERR Cannot read properties of undefined (reading 'a')
+```
+
+### Expected result
+A deep merge utility should safely initialize missing objects instead of throwing on `undefined` input.
+
+---
+
+## 2) `ENGrid.getOption` drops valid falsy option values
+- **Severity:** High (configuration corruption)
+- **Location:** `packages/scripts/src/engrid.ts` (`getOption`)
+- **Why this is a bug:** `getOption` returns `window.EngridOptions[key] || null`, which converts `false`, `0`, and `""` to `null`. This makes it impossible to distinguish explicit values from missing options.
+
+### Reproduction
+```bash
+node - <<'NODE'
+const {ENGrid}=require('./packages/scripts/dist/engrid.js');
+global.window={EngridOptions:{Debug:false, MinAmount:0, Label:''}};
+console.log('Debug', ENGrid.getOption('Debug'));
+console.log('MinAmount', ENGrid.getOption('MinAmount'));
+console.log('Label', ENGrid.getOption('Label'));
+NODE
+```
+
+### Actual result
+```text
+Debug null
+MinAmount null
+Label null
+```
+
+### Expected result
+Should return `false`, `0`, and `""` respectively.
+
+---
+
+## 3) `ENGrid.getUrlParameter("name[]")` matches unintended keys by prefix
+- **Severity:** Medium (incorrect data mapping)
+- **Location:** `packages/scripts/src/engrid.ts` (`getUrlParameter` array branch)
+- **Why this is a bug:** The current logic uses `key.startsWith(name.replace("[]", ""))`, so querying `foo[]` also captures unrelated keys like `fooBar`.
+
+### Reproduction
+```bash
+node - <<'NODE'
+const {ENGrid}=require('./packages/scripts/dist/engrid.js');
+global.window={location:{search:'?fooBar=1&foo[0]=a&foo[1]=b'}};
+console.log(JSON.stringify(ENGrid.getUrlParameter('foo[]')));
+NODE
+```
+
+### Actual result
+```text
+[{"fooBar":"1"},{"foo[0]":"a"},{"foo[1]":"b"}]
+```
+
+### Expected result
+Only `foo[...]` keys should be returned.
+
+---
+
+## 4) Malformed closing tags when injecting attribution markup
+- **Severity:** Medium (DOM/rendering risk)
+- **Location:** `packages/scripts/src/media-attribution.ts`
+- **What is wrong:** The code inserts `<figattribution>` but closes with `</figure>` in both branches.
+- **Why this is a bug:** This produces invalid/mismatched HTML and can yield unexpected DOM trees or rendering issues depending on browser parser behavior.
+
+### Evidence
+- Linked branch closes with `</figure>` instead of `</figattribution>`.
+- Non-linked branch closes with `</figure>` instead of `</figattribution>`.
+
+---
+
+## 5) Auto-country initialization is skipped on network/parse failure
+- **Severity:** Medium (initialization gap)
+- **Location:** `packages/scripts/src/auto-country-select.ts`
+- **What is wrong:** `this.init()` is called only inside the happy-path `fetch(...).then(...).then(...)` chain or in the non-fetch `else` branch.
+- **Why this is a bug:** If `/cdn-cgi/trace` fails (network error, non-200, malformed response, JSON parse error), there is no `.catch(...)` and `this.init()` is never called, so downstream initialization logic is skipped entirely.
+
+### Evidence
+- No `.catch(...)` is attached to the fetch chain.
+- `this.init()` only appears inside success path or `else` block.
+
+---
+
+## 6) `RememberMe` attempts to set `checked` on a wrapper `<div>` instead of the checkbox input
+- **Severity:** Medium (UX/state consistency)
+- **Location:** `packages/scripts/src/remember-me.ts`
+- **Relevant code path:** `insertRememberMeOptin()`
+- **Issue:** The code queries `#remember-me-opt-in` and casts it to `HTMLInputElement`, but that node is created as a `<div>`. In the `else if (this.rememberMeOptIn)` branch, it assigns `rememberMeOptInField.checked = true`, which does not update the real checkbox (`#remember-me-checkbox`).
+- **Why this is a bug:** Returning users with remembered data can see inconsistent state (opt-in intended to be checked, UI checkbox not actually checked).
+
+### Verification steps
+1. Ensure the `remember-me-opt-in` wrapper already exists in DOM (e.g., re-init path).
+2. Set `this.rememberMeOptIn = true` before entering the `else if` branch.
+3. Run `insertRememberMeOptin()`.
+4. Observe that `#remember-me-checkbox.checked` is unchanged, while only a non-semantic property may be attached to the wrapper div.
+
+### Suggested fix
+In the existing-element branch, target `#remember-me-checkbox` and set its `checked` property instead of mutating the wrapper element.
+
+---
+
+## 7) Frequency-based min/max amount validation is not initialized on load
+- **Severity:** Medium (validation correctness)
+- **Location:** `packages/scripts/src/min-max-amount.ts`
+- **Relevant code path:** `setValidationConfigFromEN()` for validator type `FAMNT`
+- **Issue:** For `FAMNT`, min/max are only updated inside `this._frequency.onFrequencyChange.subscribe(...)`. There is no immediate initialization for the current selected frequency when the class starts.
+- **Why this is a bug:** Until the user toggles frequency, validation can use stale defaults (`MinAmount`/`MaxAmount` options), causing incorrect accept/reject behavior on first submit.
+
+### Verification steps
+1. Configure an EN validator with `FAMNT` format (e.g., `SINGLE:10~100000|MONTHLY:5~100000`).
+2. Load page with initial frequency already selected (e.g., monthly) and do **not** change frequency.
+3. Enter an amount valid for monthly but invalid for defaults (or vice versa).
+4. Trigger validation and observe it uses non-`FAMNT` initialized values until frequency changes.
+
+### Suggested fix
+After wiring the subscription, immediately compute/apply min/max for the current frequency (or refactor shared logic to an `applyRangeForFrequency(freq)` helper and call it both on init and on change).
+
+---
+
+## 8) `CountryDisable` can throw when option is not configured
+- **Severity:** High (runtime crash)
+- **Location:** `packages/scripts/src/country-disable.ts`
+- **Relevant code path:** constructor
+- **Issue:** `const CountryDisable = ENGrid.getOption("CountryDisable") as string[];` is followed by `CountryDisable.length` without null/undefined guard.
+- **Why this is a bug:** If the option is absent, accessing `.length` on `undefined` throws and can break downstream script initialization.
+
+### Verification steps
+1. Run page without setting `CountryDisable` option.
+2. Instantiate `CountryDisable`.
+3. Observe runtime error similar to: `Cannot read properties of undefined (reading 'length')`.
+
+### Suggested fix
+Default to an empty array and guard the option shape:
+- `const countryDisable = ENGrid.getOption("CountryDisable") as string[] | undefined;`
+- `const disabled = Array.isArray(countryDisable) ? countryDisable : [];`
+
+---
+
+## Recommended Priority
+1. **High:** `ENGrid.deepMerge` undefined target crash.
+2. **High:** `ENGrid.getOption` falsy-value loss.
+3. **High:** `country-disable.ts` unguarded option access.
+4. **Medium:** `ENGrid.getUrlParameter` prefix overmatch in array branch.
+5. **Medium:** `min-max-amount.ts` FAMNT init gap.
+6. **Medium:** `remember-me.ts` checkbox state mismatch.
+7. **Medium:** `media-attribution.ts` malformed closing tags.
+8. **Medium:** `auto-country-select.ts` missing fetch failure fallback.

packages/scripts/dist/auto-country-select.js

@@ -29,6 +29,9 @@ export class AutoCountrySelect {
                 this.country = jsondata.loc;
                 this.init();
                 // console.log("Country:", this.country);
+            })
+                .catch(() => {
+                this.init();
             });
         }
         else {

packages/scripts/dist/country-disable.js

@@ -4,10 +4,13 @@ export class CountryDisable {
     constructor() {
         this.logger = new EngridLogger("CountryDisable", "#f0f0f0", "#333333", "🌎");
         const countries = document.querySelectorAll('select[name="supporter.country"], select[name="transaction.shipcountry"], select[name="supporter.billingCountry"], select[name="transaction.infcountry"]');
-        const CountryDisable = ENGrid.getOption("CountryDisable");
+        const countryDisable = ENGrid.getOption("CountryDisable");
+        const disabledCountries = Array.isArray(countryDisable)
+            ? countryDisable
+            : [];
         // Remove the countries from the dropdown list
-        if (countries.length > 0 && CountryDisable.length > 0) {
-            const countriesLower = CountryDisable.map((country) => country.toLowerCase());
+        if (countries.length > 0 && disabledCountries.length > 0) {
+            const countriesLower = disabledCountries.map((country) => country.toLowerCase());
             countries.forEach((country) => {
                 country.querySelectorAll("option").forEach((option) => {
                     if (countriesLower.includes(option.value.toLowerCase()) ||

packages/scripts/dist/engrid.js

@@ -19,8 +19,9 @@ export class ENGrid {
         // Add support for array on the name ending with []
         if (name.endsWith("[]")) {
             let values = [];
+            const arrayPrefix = `${name.replace("[]", "")}[`;
             searchParams.forEach((value, key) => {
-                if (key.startsWith(name.replace("[]", ""))) {
+                if (key.startsWith(arrayPrefix)) {
                     values.push(new Object({ [key]: value }));
                 }
             });
@@ -256,7 +257,10 @@ export class ENGrid {
     }
     // Return the option value
     static getOption(key) {
-        return window.EngridOptions[key] || null;
+        if (window.EngridOptions && key in window.EngridOptions) {
+            return window.EngridOptions[key];
+        }
+        return null;
     }
     // Load an external script
     static loadJS(url, onload = null, head = true) {
@@ -387,11 +391,12 @@ export class ENGrid {
     }
     // Deep merge two objects
     static deepMerge(target, source) {
+        target = target || {};
         for (const key in source) {
             if (source[key] instanceof Object)
                 Object.assign(source[key], ENGrid.deepMerge(target[key], source[key]));
         }
-        Object.assign(target || {}, source);
+        Object.assign(target, source);
         return target;
     }
     static setError(element, errorMessage) {

packages/scripts/dist/media-attribution.js

@@ -41,10 +41,10 @@ export class MediaAttribution {
                             decodeURIComponent(attributionSourceLink) +
                             '" target="_blank" tabindex="-1">' +
                             attributionSource +
-                            "</a></figure>");
+                            "</a></figattribution>");
                     }
                     else {
-                        mediaWithAttributionElement.insertAdjacentHTML("afterend", "<figattribution>" + attributionSource + "</figure>");
+                        mediaWithAttributionElement.insertAdjacentHTML("afterend", "<figattribution>" + attributionSource + "</figattribution>");
                     }
                     const attributionSourceTooltip = "attributionSourceTooltip" in mediaWithAttributionElement.dataset
                         ? mediaWithAttributionElement.dataset.attributionSourceTooltip

packages/scripts/dist/min-max-amount.d.ts

@@ -12,5 +12,6 @@ export declare class MinMaxAmount {
     shouldRun(): boolean;
     enOnValidate(): void;
     liveValidate(): void;
+    private updateFamntRange;
     private setValidationConfigFromEN;
 }

packages/scripts/dist/min-max-amount.js

@@ -71,6 +71,27 @@ export class MinMaxAmount {
             ENGrid.removeError(".en__field--withOther");
         }
     }
+    updateFamntRange(freq) {
+        if (!this.enAmountValidator || !this.enAmountValidator.format)
+            return;
+        // In the validator, "onetime" is written as "SINGLE"
+        // Validator format for FAMNT is like SINGLE:10~100000|MONTHLY:5~100000|QUARTERLY:25~100000|ANNUAL:25~100000
+        const frequency = freq === "onetime" ? "SINGLE" : freq.toUpperCase();
+        const validationRange = this.enAmountValidator.format
+            .split("|")
+            .find((range) => range.startsWith(frequency));
+        if (!validationRange) {
+            this.logger.log(`No validation range found for frequency: ${frequency}`);
+            return;
+        }
+        const amounts = validationRange.split(":")[1].split("~");
+        this.minAmount = Number(amounts[0]);
+        this.maxAmount = Number(amounts[1]);
+        this.minAmountMessage = this.enAmountValidator.errorMessage;
+        this.maxAmountMessage = this.enAmountValidator.errorMessage;
+        this.logger.log(`Frequency changed to ${frequency}, updating min and max amounts`, validationRange);
+        this.logger.log(`Setting new values - Min Amount: ${this.minAmount}, Max Amount: ${this.maxAmount}, Error Message: ${this.minAmountMessage}`);
+    }
     setValidationConfigFromEN() {
         if (!ENGrid.getOption("UseAmountValidatorFromEN") ||
             !window.EngagingNetworks.validators) {
@@ -101,26 +122,9 @@ export class MinMaxAmount {
         // Frequency-based amount validator
         if (this.enAmountValidator.type === "FAMNT") {
             this._frequency.onFrequencyChange.subscribe((freq) => {
-                if (!this.enAmountValidator || !this.enAmountValidator.format)
-                    return;
-                // In the validator, "onetime" is written as "SINGLE"
-                // Validator format for FAMNT is like SINGLE:10~100000|MONTHLY:5~100000|QUARTERLY:25~100000|ANNUAL:25~100000
-                const frequency = freq === "onetime" ? "SINGLE" : freq.toUpperCase();
-                const validationRange = this.enAmountValidator.format
-                    .split("|")
-                    .find((range) => range.startsWith(frequency));
-                if (!validationRange) {
-                    this.logger.log(`No validation range found for frequency: ${frequency}`);
-                    return;
-                }
-                const amounts = validationRange.split(":")[1].split("~");
-                this.minAmount = Number(amounts[0]);
-                this.maxAmount = Number(amounts[1]);
-                this.minAmountMessage = this.enAmountValidator.errorMessage;
-                this.maxAmountMessage = this.enAmountValidator.errorMessage;
-                this.logger.log(`Frequency changed to ${frequency}, updating min and max amounts`, validationRange);
-                this.logger.log(`Setting new values - Min Amount: ${this.minAmount}, Max Amount: ${this.maxAmount}, Error Message: ${this.minAmountMessage}`);
+                this.updateFamntRange(freq);
             });
+            this.updateFamntRange(this._frequency.frequency);
         }
     }
 }

packages/scripts/dist/remember-me.js

@@ -208,7 +208,10 @@ export class RememberMe {
             }
         }
         else if (this.rememberMeOptIn) {
-            rememberMeOptInField.checked = true;
+            const rememberMeCheckbox = document.getElementById("remember-me-checkbox");
+            if (rememberMeCheckbox) {
+                rememberMeCheckbox.checked = true;
+            }
         }
         this._events.dispatchLoad(false);
         window.dispatchEvent(new CustomEvent("RememberMe_Loaded", { detail: { withData: false } }));

packages/scripts/src/auto-country-select.ts

@@ -45,6 +45,9 @@ export class AutoCountrySelect {
           this.country = jsondata.loc;
           this.init();
           // console.log("Country:", this.country);
+        })
+        .catch(() => {
+          this.init();
         });
     } else {
       this.init();

packages/scripts/src/country-disable.ts

@@ -13,10 +13,15 @@ export class CountryDisable {
     const countries = document.querySelectorAll(
       'select[name="supporter.country"], select[name="transaction.shipcountry"], select[name="supporter.billingCountry"], select[name="transaction.infcountry"]'
     );
-    const CountryDisable = ENGrid.getOption("CountryDisable") as string[];
+    const countryDisable = ENGrid.getOption("CountryDisable") as
+      | string[]
+      | undefined;
+    const disabledCountries = Array.isArray(countryDisable)
+      ? countryDisable
+      : [];
     // Remove the countries from the dropdown list
-    if (countries.length > 0 && CountryDisable.length > 0) {
-      const countriesLower = CountryDisable.map((country) =>
+    if (countries.length > 0 && disabledCountries.length > 0) {
+      const countriesLower = disabledCountries.map((country) =>
         country.toLowerCase()
       );
       countries.forEach((country) => {

packages/scripts/src/engrid.ts

@@ -25,8 +25,9 @@ export abstract class ENGrid {
     // Add support for array on the name ending with []
     if (name.endsWith("[]")) {
       let values: Object[] = [];
+      const arrayPrefix = `${name.replace("[]", "")}[`;
       searchParams.forEach((value, key) => {
-        if (key.startsWith(name.replace("[]", ""))) {
+        if (key.startsWith(arrayPrefix)) {
           values.push(new Object({ [key]: value }));
         }
       });
@@ -307,7 +308,10 @@ export abstract class ENGrid {
 
   // Return the option value
   static getOption<K extends keyof Options>(key: K): Options[K] | null {
-    return window.EngridOptions[key] || null;
+    if (window.EngridOptions && key in window.EngridOptions) {
+      return window.EngridOptions[key];
+    }
+    return null;
   }
   // Load an external script
   static loadJS(
@@ -454,11 +458,12 @@ export abstract class ENGrid {
 
   // Deep merge two objects
   static deepMerge(target: any, source: any) {
+    target = target || {};
     for (const key in source) {
       if (source[key] instanceof Object)
         Object.assign(source[key], ENGrid.deepMerge(target[key], source[key]));
     }
-    Object.assign(target || {}, source);
+    Object.assign(target, source);
     return target;
   }
   static setError(element: string | HTMLElement, errorMessage: string) {