src-improvements.md — Advanced Ads – Ad Manager & AdSense 2.0.25

advanced-ads / docs / src-improvements.md

advanced-ads / docs Last commit date

superpowers 1 day ago addons.md 1 week ago src-improvements.md 1 day ago tech-stack.md 1 day ago vulberability.md 1 week ago

src-improvements.md

184 lines

1	# src/ Code Improvements
2
3	Findings from a review of `src/admin/`. Ordered by priority.
4
5	---
6
7	## Bug / Correctness
8
9	### 1. `useFetch.js` always sends POST instead of GET
10
11	File: `src/admin/hooks/useFetch.js`
12
13	`extraArgs` defaults to `{}` which is always truthy, so the `if (extraArgs)` branch always fires — every call becomes a POST with empty data even when a GET was intended.
14
15	Fix: check `Object.keys(extraArgs).length > 0`.
16	Also: `extraArgs` is missing from the `useEffect` dependency array, so stale args won't trigger a re-fetch.
17
18	```js
19	// current (broken)
20	if ( extraArgs ) {
21	args = { ...args, method: 'POST', data: extraArgs };
22	}
23
24	// fix
25	if ( extraArgs && Object.keys( extraArgs ).length > 0 ) {
26	args = { ...args, method: 'POST', data: extraArgs };
27	}
28	// and add extraArgs to useEffect deps
29	```
30
31	---
32
33	### 2. Duplicate `STORE_NAME` in `licenses-api.js`
34
35	File: `src/admin/screen-licenses/hooks/licenses-api.js` line 6
36
37	`STORE_NAME` is hardcoded as `'advanced-ads/store'` locally instead of being imported from `@admin/store`. If the name ever changes, this copy will silently break.
38
39	```js
40	// remove this line
41	const STORE_NAME = 'advanced-ads/store';
42
43	// add to imports
44	import { STORE_NAME } from '@admin/store';
45	```
46
47	---
48
49	## Dead Code to Remove
50
51	### 3. `activateActionsDisabled` in `AddonRowActions.jsx`
52
53	File: `src/admin/screen-licenses/components/AddonRowActions.jsx`
54
55	`const activateActionsDisabled = false` is hardcoded and never changes. It's used as a `disabled` condition but can never actually disable anything.
56
57	Remove the constant and the `\|\| activateActionsDisabled` conditions that depend on it.
58
59	---
60
61	### 4. `isComponentsBusy` prop in `DownloadActivateButton.jsx`
62
63	File: `src/admin/screen-licenses/components/DownloadActivateButton.jsx`
64
65	`isComponentsBusy` is accepted as a prop (default `false`) and used in button label/disabled logic, but `LicenseItem.jsx` never passes it — so it is always `false`.
66
67	Remove the prop, simplify the label and `isDisabled` logic.
68
69	---
70
71	### 5. `sitesTotal` alias in `LicenseItem.jsx`
72
73	File: `src/admin/screen-licenses/components/LicenseItem.jsx` line 73
74
75	```js
76	const sitesTotal = availableSites; // unnecessary alias
77	```
78
79	Replace all uses of `sitesTotal` with `availableSites` directly and remove the line.
80
81	---
82
83	### 6. Unused `licenseType` prop in `LicenseItem.jsx`
84
85	File: `src/admin/screen-licenses/components/LicenseItem.jsx` and `License.jsx`
86
87	`licenseType` is destructured from props and passed from `License.jsx` as `licenseType={license.name}` but is never used inside `LicenseItem`. Remove the prop from both files.
88
89	---
90
91	## Simplifications
92
93	### 7. Redundant early-return in `isRichLicenseActive`
94
95	File: `src/admin/screen-licenses/utils.js`
96
97	The negative-check block is unnecessary — the final `return` already returns `false` for anything that isn't `'active'` or `'valid'`.
98
99	```js
100	// current — the if-block adds no value
101	if ( normalized === 'expired' \|\| normalized === 'inactive' \|\| ... ) {
102	return false;
103	}
104	return normalized === 'active' \|\| normalized === 'valid';
105
106	// fix
107	return normalized === 'active' \|\| normalized === 'valid';
108	```
109
110	---
111
112	### 8. `Date.now()` called twice + repeated magic number
113
114	File: `src/admin/screen-licenses/utils.js`
115
116	`isLicenseExpiringSoon` and `getDaysUntilLicenseExpiry` both call `Date.now()` twice in the same function (once for the guard, once for the math). Capture it once. Also, `24 * 60 * 60 * 1000` is repeated — extract to a module-level constant.
117
118	```js
119	const MS_PER_DAY = 24 * 60 * 60 * 1000;
120
121	export function isLicenseExpiringSoon( expiryDate, days = 30 ) {
122	const ts = getLicenseExpiryTimestamp( expiryDate );
123	const now = Date.now();
124	if ( ! ts \|\| ts <= now ) return false;
125	return ts - now <= days * MS_PER_DAY;
126	}
127	```
128
129	---
130
131	### 9. Double lookup table in `addonIdForLicenseProductName`
132
133	File: `src/admin/screen-licenses/utils.js`
134
135	The function has two arrays — `manifest` and `labels` — that encode the same addon-name-to-id mapping with overlapping entries. Consolidate into one unified lookup object.
136
137	---
138
139	### 10. Repeated `safeObject` pattern in `normalizeLicensesResponse`
140
141	File: `src/admin/screen-licenses/hooks/licenses-api.js`
142
143	The same 3-line guard appears 4 times:
144
145	```js
146	payload?.foo && typeof payload.foo === 'object' ? payload.foo : {};
147	```
148
149	Extract a helper:
150
151	```js
152	function safeObject( val ) {
153	return val && typeof val === 'object' ? val : {};
154	}
155	```
156
157	---
158
159	### 11. Two patterns for getting the license list in `AddonRowActions.jsx`
160
161	File: `src/admin/screen-licenses/components/AddonRowActions.jsx`
162
163	`getLicenseListForSave()` calls `select(STORE_NAME)` imperatively, while `handleDeactivate` uses the `licensesFromStore` value from `useSelect`. They do the same thing. Remove `getLicenseListForSave` and use `licensesFromStore` consistently.
164
165	---
166
167	### 12. Duplicate `pricingId` validation in two files
168
169	Files: `src/admin/screen-licenses/utils.js` and `components/PricingTable.jsx`
170
171	The check `plan.id === 'all-access-five' && pricingId === 2` appears in both `getCheckoutIdsForPlan` (utils.js) and the `checkoutReady` condition in `PricingTable.jsx`. Since `getCheckoutIdsForPlan` already enforces it, remove the redundant check from `PricingTable.jsx`.
172
173	---
174
175	## Complexity
176
177	### 13. Break up `getDisplayLicenseStatus`
178
179	File: `src/admin/screen-licenses/utils.js`
180
181	This function is 90+ lines with deeply nested branches. It also has two `// eslint-disable-next-line @wordpress/no-unused-vars-before-return` comments because variables are computed before early returns — a sign the logic isn't well structured.
182
183	Break it into named sub-functions (e.g. `getDisplayStatusForAllAccess`, `getDisplayStatusWithAppliedMap`) so each path is readable on its own and the eslint suppression can be removed.
184