diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md new file mode 100644 index 00000000..77437c9e --- /dev/null +++ b/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 `` but closes with `` 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 `` instead of ``. +- Non-linked branch closes with `` instead of ``. + +--- + +## 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 `
` 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 `
`. 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. diff --git a/packages/scripts/dist/auto-country-select.js b/packages/scripts/dist/auto-country-select.js index 4fd770e9..642b772b 100644 --- a/packages/scripts/dist/auto-country-select.js +++ b/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 { diff --git a/packages/scripts/dist/country-disable.js b/packages/scripts/dist/country-disable.js index bc39615d..ea3c1dbf 100644 --- a/packages/scripts/dist/country-disable.js +++ b/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()) || diff --git a/packages/scripts/dist/engrid.js b/packages/scripts/dist/engrid.js index 4215c18f..d4c398b1 100644 --- a/packages/scripts/dist/engrid.js +++ b/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) { diff --git a/packages/scripts/dist/media-attribution.js b/packages/scripts/dist/media-attribution.js index ffa4dcec..8dc14289 100644 --- a/packages/scripts/dist/media-attribution.js +++ b/packages/scripts/dist/media-attribution.js @@ -41,10 +41,10 @@ export class MediaAttribution { decodeURIComponent(attributionSourceLink) + '" target="_blank" tabindex="-1">' + attributionSource + - ""); + ""); } else { - mediaWithAttributionElement.insertAdjacentHTML("afterend", "" + attributionSource + ""); + mediaWithAttributionElement.insertAdjacentHTML("afterend", "" + attributionSource + ""); } const attributionSourceTooltip = "attributionSourceTooltip" in mediaWithAttributionElement.dataset ? mediaWithAttributionElement.dataset.attributionSourceTooltip diff --git a/packages/scripts/dist/min-max-amount.d.ts b/packages/scripts/dist/min-max-amount.d.ts index 7fd35173..84360238 100644 --- a/packages/scripts/dist/min-max-amount.d.ts +++ b/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; } diff --git a/packages/scripts/dist/min-max-amount.js b/packages/scripts/dist/min-max-amount.js index e5962226..3cca7192 100644 --- a/packages/scripts/dist/min-max-amount.js +++ b/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); } } } diff --git a/packages/scripts/dist/remember-me.js b/packages/scripts/dist/remember-me.js index c1fa5549..41a60f2f 100644 --- a/packages/scripts/dist/remember-me.js +++ b/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 } })); diff --git a/packages/scripts/src/auto-country-select.ts b/packages/scripts/src/auto-country-select.ts index dba4ab33..8f7e92ee 100644 --- a/packages/scripts/src/auto-country-select.ts +++ b/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(); diff --git a/packages/scripts/src/country-disable.ts b/packages/scripts/src/country-disable.ts index 7de74ba6..a8acfc5f 100644 --- a/packages/scripts/src/country-disable.ts +++ b/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) => { diff --git a/packages/scripts/src/engrid.ts b/packages/scripts/src/engrid.ts index 7cb3ed89..1d9c9623 100644 --- a/packages/scripts/src/engrid.ts +++ b/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(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) { diff --git a/packages/scripts/src/media-attribution.ts b/packages/scripts/src/media-attribution.ts index e0ac15d9..e43e77e0 100644 --- a/packages/scripts/src/media-attribution.ts +++ b/packages/scripts/src/media-attribution.ts @@ -55,12 +55,12 @@ export class MediaAttribution { decodeURIComponent(attributionSourceLink) + '" target="_blank" tabindex="-1">' + attributionSource + - "" + "" ); } else { mediaWithAttributionElement.insertAdjacentHTML( "afterend", - "" + attributionSource + "" + "" + attributionSource + "" ); } const attributionSourceTooltip = diff --git a/packages/scripts/src/min-max-amount.ts b/packages/scripts/src/min-max-amount.ts index aa0b888c..dac96134 100644 --- a/packages/scripts/src/min-max-amount.ts +++ b/packages/scripts/src/min-max-amount.ts @@ -99,6 +99,37 @@ export class MinMaxAmount { } } + private updateFamntRange(freq: string) { + 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}` + ); + } + private setValidationConfigFromEN() { if ( !ENGrid.getOption("UseAmountValidatorFromEN") || @@ -145,36 +176,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); } } } diff --git a/packages/scripts/src/remember-me.ts b/packages/scripts/src/remember-me.ts index 3a20d139..dcdc475a 100644 --- a/packages/scripts/src/remember-me.ts +++ b/packages/scripts/src/remember-me.ts @@ -281,7 +281,12 @@ export class RememberMe { tippy("#rememberme-learn-more-toggle", { content: rememberMeInfo }); } } else if (this.rememberMeOptIn) { - rememberMeOptInField.checked = true; + const rememberMeCheckbox = document.getElementById( + "remember-me-checkbox" + ) as HTMLInputElement; + if (rememberMeCheckbox) { + rememberMeCheckbox.checked = true; + } } this._events.dispatchLoad(false); window.dispatchEvent(