research: evaluate Store-backed extension update prompts#83
Draft
hoangsvit wants to merge 6 commits into
Draft
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
Thank you for creating this pull request and helping make the project better. We will review / merge it when we are online. |
eplus-bot
added
needs-review
eplus-bot
self-requested a review
July 24, 2026 01:08
PR Summary by QodoPrototype Store-backed extension update banner in popup header
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Contributor
|
Approved by @eplus-bot after all pull request checks passed. Approval refresh: #1 CI run: https://github.com/ePlus-DEV/gmail-alias-toolkit/actions/runs/30058155366 |
eplus-bot
approved these changes
Jul 24, 2026
eplus-bot
left a comment
eplus-bot
left a comment
Contributor
There was a problem hiding this comment.
Approved by @eplus-bot after all pull request checks passed.
CI run: https://github.com/ePlus-DEV/gmail-alias-toolkit/actions/runs/30058155366
Code Review by Qodo
1. Snooze state can be clobbered
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+100
to
+187
| const syncState = useCallback( | ||
| async (nextState: ExtensionUpdateState) => { | ||
| const usableState = clearInstalledUpdate(nextState, installedVersion); | ||
| setUpdateState(usableState); | ||
| if (usableState !== nextState) await writeUpdateState(usableState); | ||
| }, | ||
| [installedVersion], | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| let active = true; | ||
|
|
||
| const initialize = async () => { | ||
| const storedState = await readUpdateState(); | ||
| if (active) await syncState(storedState); | ||
|
|
||
| const checkedState = await checkBrowserStoreForUpdate(installedVersion); | ||
| if (active) await syncState(checkedState); | ||
| }; | ||
|
|
||
| const handleUpdateAvailable = async (details: { version: string }) => { | ||
| const currentState = await readUpdateState(); | ||
| const nextState = markExtensionUpdateReady( | ||
| currentState, | ||
| details.version, | ||
| Date.now(), | ||
| ); | ||
| await writeUpdateState(nextState); | ||
| if (active) setUpdateState(nextState); | ||
| }; | ||
|
|
||
| const handleStorageChange = ( | ||
| changes: Record<string, { newValue?: unknown }>, | ||
| areaName: string, | ||
| ) => { | ||
| if (areaName !== "local" || !changes[EXTENSION_UPDATE_STORAGE_KEY]) { | ||
| return; | ||
| } | ||
| const nextState = normalizeExtensionUpdateState( | ||
| changes[EXTENSION_UPDATE_STORAGE_KEY].newValue, | ||
| ); | ||
| if (active) setUpdateState(clearInstalledUpdate(nextState, installedVersion)); | ||
| }; | ||
|
|
||
| updateRuntime.onUpdateAvailable?.addListener(handleUpdateAvailable); | ||
| browser.storage.onChanged.addListener(handleStorageChange); | ||
| void initialize(); | ||
|
|
||
| return () => { | ||
| active = false; | ||
| updateRuntime.onUpdateAvailable?.removeListener(handleUpdateAvailable); | ||
| browser.storage.onChanged.removeListener(handleStorageChange); | ||
| }; | ||
| }, [installedVersion, syncState]); | ||
|
|
||
| const availableVersion = updateState.availableVersion; | ||
| const shouldShow = shouldShowExtensionUpdatePrompt( | ||
| updateState, | ||
| installedVersion, | ||
| Date.now(), | ||
| ); | ||
| if (!shouldShow || !availableVersion) return null; | ||
|
|
||
| const message = formatUpdatePromptCopy( | ||
| updateState.status === "ready" ? copy.ready : copy.available, | ||
| { current: installedVersion, latest: availableVersion }, | ||
| ); | ||
|
|
||
| const handleUpdate = async () => { | ||
| if (updateState.status === "ready") { | ||
| updateRuntime.reload(); | ||
| return; | ||
| } | ||
|
|
||
| setIsChecking(true); | ||
| const nextState = await checkBrowserStoreForUpdate(installedVersion, true); | ||
| setUpdateState(nextState); | ||
| setIsChecking(false); | ||
|
|
||
| if (nextState.status === "ready") updateRuntime.reload(); | ||
| }; | ||
|
|
||
| const handleLater = async () => { | ||
| const nextState = snoozeExtensionUpdate(updateState, Date.now()); | ||
| setUpdateState(nextState); | ||
| await writeUpdateState(nextState); | ||
| }; | ||
|
|
There was a problem hiding this comment.
1. syncstate lacks jsdoc 📘 Rule violation ⚙ Maintainability
Several new arrow functions in ExtensionUpdateBanner.tsx are missing JSDoc comments, which violates the requirement that all functions/arrow functions/exported components be documented and can trigger DeepSource JS-D1001 warnings.
Agent Prompt
## Issue description
New arrow functions in `ExtensionUpdateBanner.tsx` are missing required JSDoc comments.
## Issue Context
PR Compliance requires JSDoc for every function declaration and arrow function to avoid DeepSource `JS-D1001` warnings.
## Fix Focus Areas
- src/components/alias/ExtensionUpdateBanner.tsx[100-107]
- src/components/alias/ExtensionUpdateBanner.tsx[112-143]
- src/components/alias/ExtensionUpdateBanner.tsx[168-187]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+144
to
+147
| updateRuntime.onUpdateAvailable?.addListener(handleUpdateAvailable); | ||
| browser.storage.onChanged.addListener(handleStorageChange); | ||
| void initialize(); | ||
|
|
There was a problem hiding this comment.
2. void initialize() deepsource issue 📘 Rule violation ⚙ Maintainability
The new void initialize(); statement is a known code-quality anti-pattern called out by the DeepSource compliance requirement and may be flagged as a DeepSource issue.
Agent Prompt
## Issue description
`ExtensionUpdateBanner` uses `void initialize();`, which can be flagged by DeepSource as an anti-pattern.
## Issue Context
PR Compliance requires DeepSource to report zero issues, including avoiding flagged anti-patterns such as `void` statements.
## Fix Focus Areas
- src/components/alias/ExtensionUpdateBanner.tsx[144-147]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+141
to
+161
| export function applyRuntimeUpdateCheckResult( | ||
| state: ExtensionUpdateState, | ||
| result: RuntimeUpdateCheckResult, | ||
| checkedAt: number, | ||
| ): ExtensionUpdateState { | ||
| if (result.status === "no_update") { | ||
| return { status: "idle", lastCheckedAt: checkedAt }; | ||
| } | ||
|
|
||
| if (result.status === "throttled") { | ||
| return { ...state, lastCheckedAt: checkedAt }; | ||
| } | ||
|
|
||
| const availableVersion = | ||
| parseVersion(result.version)?.normalized ?? state.availableVersion; | ||
| return { | ||
| ...state, | ||
| status: availableVersion ? "available" : state.status, | ||
| availableVersion, | ||
| lastCheckedAt: checkedAt, | ||
| }; |
There was a problem hiding this comment.
3. Ready state downgraded 🐞 Bug ≡ Correctness
applyRuntimeUpdateCheckResult() can overwrite an already "ready" state to "available" (or even "idle" on "no_update"), hiding the "Apply update" path despite the browser reporting the update is downloaded. This can block users from being offered runtime.reload() until another onUpdateAvailable event re-marks the state as ready.
Agent Prompt
## Issue description
`applyRuntimeUpdateCheckResult()` overwrites `state.status` to `"available"` whenever it sees an available version, and to `"idle"` on `"no_update"`, even if `state.status` was already `"ready"`. Once `markExtensionUpdateReady()` sets `status: "ready"`, subsequent Store checks should not demote that ready state; readiness should only be cleared when the installed version catches up.
## Issue Context
The UI behavior in `ExtensionUpdateBanner` depends on `updateState.status === "ready"` to show the apply messaging and allow `runtime.reload()`.
## Fix Focus Areas
- src/utils/extensionUpdate.ts[141-179]
Suggested direction:
- If `state.status === "ready"`, do not change `status` to `"available"` or `"idle"` in `applyRuntimeUpdateCheckResult()`; only update `lastCheckedAt` (and possibly `availableVersion` if you intentionally want to track a newer version without demoting readiness).
- Ensure `no_update` doesn’t clear a ready state; readiness should be cleared by the installed-version catch-up logic (already handled elsewhere).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+59
to
+86
| async function checkBrowserStoreForUpdate( | ||
| installedVersion: string, | ||
| force = false, | ||
| ): Promise<ExtensionUpdateState> { | ||
| const existing = clearInstalledUpdate(await readUpdateState(), installedVersion); | ||
| const now = Date.now(); | ||
|
|
||
| if (!shouldCheckForExtensionUpdate(existing, now, force)) return existing; | ||
| if (!updateRuntime.requestUpdateCheck) return existing; | ||
| if (activeUpdateCheck) return activeUpdateCheck; | ||
|
|
||
| activeUpdateCheck = (async () => { | ||
| try { | ||
| const result = await updateRuntime.requestUpdateCheck?.(); | ||
| if (!result) return existing; | ||
|
|
||
| const nextState = clearInstalledUpdate( | ||
| applyRuntimeUpdateCheckResult(existing, result, now), | ||
| installedVersion, | ||
| ); | ||
| await writeUpdateState(nextState); | ||
| return nextState; | ||
| } catch { | ||
| return existing; | ||
| } finally { | ||
| activeUpdateCheck = null; | ||
| } | ||
| })(); |
There was a problem hiding this comment.
4. Snooze state can be clobbered 🐞 Bug ☼ Reliability
checkBrowserStoreForUpdate() reads a snapshot of storage state and later writes a derived nextState, so a concurrent "Later" snooze or onUpdateAvailable-ready write can be overwritten by the in-flight Store-check write. This can cause a snoozed prompt to reappear immediately, or lose the ready-to-apply state.
Agent Prompt
## Issue description
The Store check path computes `nextState` from a stale `existing` snapshot and writes it back after awaiting `requestUpdateCheck()`. While that await is in-flight, other flows (`handleLater` snooze, `onUpdateAvailable` ready event, or even another surface writing state) can update storage; the later Store-check write can then clobber those newer fields (dismissal/ready).
## Issue Context
This is a classic last-writer-wins race: read -> await -> write.
## Fix Focus Areas
- src/components/alias/ExtensionUpdateBanner.tsx[59-89]
- src/components/alias/ExtensionUpdateBanner.tsx[120-129]
- src/components/alias/ExtensionUpdateBanner.tsx[182-186]
Suggested direction (pick one):
- Re-read storage immediately before persisting and apply the runtime result to the latest state (or merge fields so that `dismissedVersion/dismissedUntil` and `status: "ready"` cannot be lost).
- Alternatively, funnel *all* writes through a single storage-update helper that always reads the latest state first.
- Optionally also disable the "Later" button while `isChecking` to reduce user-triggered races, but the core fix should be on the persistence side.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+63
to
+85
| const existing = clearInstalledUpdate(await readUpdateState(), installedVersion); | ||
| const now = Date.now(); | ||
|
|
||
| if (!shouldCheckForExtensionUpdate(existing, now, force)) return existing; | ||
| if (!updateRuntime.requestUpdateCheck) return existing; | ||
| if (activeUpdateCheck) return activeUpdateCheck; | ||
|
|
||
| activeUpdateCheck = (async () => { | ||
| try { | ||
| const result = await updateRuntime.requestUpdateCheck?.(); | ||
| if (!result) return existing; | ||
|
|
||
| const nextState = clearInstalledUpdate( | ||
| applyRuntimeUpdateCheckResult(existing, result, now), | ||
| installedVersion, | ||
| ); | ||
| await writeUpdateState(nextState); | ||
| return nextState; | ||
| } catch { | ||
| return existing; | ||
| } finally { | ||
| activeUpdateCheck = null; | ||
| } |
There was a problem hiding this comment.
5. Checks retry on failures 🐞 Bug ☼ Reliability
When requestUpdateCheck is missing or throws, checkBrowserStoreForUpdate() returns the previous state without persisting lastCheckedAt, so the once-per-24h gate may never engage after failures. This can lead to repeated Store-check attempts on every popup open in error/unavailable-API scenarios.
Agent Prompt
## Issue description
Automatic check throttling depends on `state.lastCheckedAt`, but the failure/unavailable-API paths return without updating/persisting it. If the stored state has `lastCheckedAt` undefined (common on first run), any transient error will cause subsequent popup opens to attempt another update check immediately.
## Issue Context
`shouldCheckForExtensionUpdate()` gates checks using `lastCheckedAt`.
## Fix Focus Areas
- src/components/alias/ExtensionUpdateBanner.tsx[63-85]
- src/utils/extensionUpdate.ts[127-138]
Suggested direction:
- On `requestUpdateCheck` missing, null result, or thrown error, write a state update that sets `lastCheckedAt: now` (keeping other fields intact) so the 24h throttle still applies.
- If you want quicker retries after failures, consider a separate shorter backoff timestamp (but don’t leave it undefined).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+168
to
+180
| const handleUpdate = async () => { | ||
| if (updateState.status === "ready") { | ||
| updateRuntime.reload(); | ||
| return; | ||
| } | ||
|
|
||
| setIsChecking(true); | ||
| const nextState = await checkBrowserStoreForUpdate(installedVersion, true); | ||
| setUpdateState(nextState); | ||
| setIsChecking(false); | ||
|
|
||
| if (nextState.status === "ready") updateRuntime.reload(); | ||
| }; |
There was a problem hiding this comment.
6. No mount-guard in handler 🐞 Bug ☼ Reliability
handleUpdate() awaits an async check and then calls setUpdateState/setIsChecking without checking whether the component is still mounted. If the popup closes during the check, the handler can attempt UI state updates after unmount (the effect path uses an explicit active flag, but this handler does not).
Agent Prompt
## Issue description
`handleUpdate()` performs async work and then sets React state without a mounted guard. The effect initialization path already uses an `active` boolean to prevent post-unmount updates; `handleUpdate()` should follow the same pattern.
## Issue Context
This is about preventing post-unmount UI updates; storage persistence can still proceed if desired.
## Fix Focus Areas
- src/components/alias/ExtensionUpdateBanner.tsx[109-118]
- src/components/alias/ExtensionUpdateBanner.tsx[168-180]
Suggested direction:
- Use a `useRef` mounted flag (or reuse the existing `active` approach via a shared ref) and check it before calling `setUpdateState`/`setIsChecking` after awaits.
- Optionally disable both buttons while checking to prevent overlapping user actions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
hoangsvit
marked this pull request as draft
July 24, 2026 03:48
hoangsvit
changed the title
16 tasks
Member
Author
16 tasks
hoangsvit
marked this pull request as ready for review
July 24, 2026 14:32
eplus-bot
self-requested a review
July 24, 2026 14:32
hoangsvit
marked this pull request as draft
July 24, 2026 14:39
Comment on lines
+98
to
+137
| export function normalizeExtensionUpdateState( | ||
| value: unknown, | ||
| ): ExtensionUpdateState { | ||
| if (!value || typeof value !== "object") return { status: "idle" }; | ||
|
|
||
| const candidate = value as Partial<ExtensionUpdateState>; | ||
| const status: ExtensionUpdateStatus = ["idle", "available", "ready"].includes( | ||
| candidate.status ?? "", | ||
| ) | ||
| ? (candidate.status as ExtensionUpdateStatus) | ||
| : "idle"; | ||
|
|
||
| return { | ||
| status, | ||
| lastCheckedAt: | ||
| typeof candidate.lastCheckedAt === "number" | ||
| ? candidate.lastCheckedAt | ||
| : undefined, | ||
| availableVersion: | ||
| parseVersion(candidate.availableVersion)?.normalized ?? undefined, | ||
| dismissedVersion: | ||
| parseVersion(candidate.dismissedVersion)?.normalized ?? undefined, | ||
| dismissedUntil: | ||
| typeof candidate.dismissedUntil === "number" | ||
| ? candidate.dismissedUntil | ||
| : undefined, | ||
| }; | ||
| } | ||
|
|
||
| /** Limits automatic Store checks while still allowing an explicit user retry. */ | ||
| export function shouldCheckForExtensionUpdate( | ||
| state: ExtensionUpdateState, | ||
| now: number, | ||
| force = false, | ||
| ): boolean { | ||
| if (force) return true; | ||
| return ( | ||
| state.lastCheckedAt === undefined || | ||
| now - state.lastCheckedAt >= UPDATE_CHECK_INTERVAL_MS | ||
| ); |
There was a problem hiding this comment.
1. Nan disables auto checks 🐞 Bug ☼ Reliability
normalizeExtensionUpdateState() accepts NaN/Infinity timestamps because it only checks `typeof === "number"; shouldCheckForExtensionUpdate() then computes now - lastCheckedAt` as NaN and the 24h gate never opens again unless a forced check is used. This can prevent automatic Store re-checks after storage corruption, despite the function being documented as validating “untrusted persisted state”.
Agent Prompt
### Issue description
`normalizeExtensionUpdateState()` currently treats any `number` (including `NaN`, `Infinity`, `-Infinity`) as a valid persisted timestamp. When `lastCheckedAt` is `NaN` or positive `Infinity`, `shouldCheckForExtensionUpdate()` will never allow automatic checks again because `now - lastCheckedAt` becomes `NaN`/`-Infinity` and fails the `>= UPDATE_CHECK_INTERVAL_MS` comparison.
### Issue Context
This module explicitly claims to “validate untrusted persisted state”, so it should reject non-finite numeric values.
### Fix Focus Areas
- src/utils/extensionUpdate.ts[97-138]
- tests/utils/extensionUpdate.test.ts[1-163]
### Suggested fix
- In `normalizeExtensionUpdateState`, replace the `typeof ... === "number"` guards for `lastCheckedAt` and `dismissedUntil` with `Number.isFinite(...)`.
- Add a regression test covering `lastCheckedAt: NaN` and `lastCheckedAt: Infinity` to ensure they normalize to `undefined` (and thus do not block future automatic checks).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+155
to
+166
| export function getUpdatePromptCopy(locale?: string): UpdatePromptCopy { | ||
| const resolvedLocale = ( | ||
| locale ?? browser.i18n.getUILanguage?.() ?? "en" | ||
| ) | ||
| .toLowerCase() | ||
| .replace("-", "_"); | ||
| const baseLocale = resolvedLocale.split("_")[0]; | ||
| return ( | ||
| UPDATE_PROMPT_COPY[resolvedLocale] ?? | ||
| UPDATE_PROMPT_COPY[baseLocale] ?? | ||
| UPDATE_PROMPT_COPY.en | ||
| ); |
There was a problem hiding this comment.
2. Unguarded i18n api access 🐞 Bug ⚙ Maintainability
getUpdatePromptCopy() dereferences browser.i18n without a guard, and ExtensionUpdateBanner calls it with no locale, so rendering this component in a non-extension or partially mocked environment throws at runtime. This is inconsistent with the existing t() helper which explicitly try/catches browser.i18n access, and it will also fail under the current Vitest browser mock (no i18n).
Agent Prompt
### Issue description
`getUpdatePromptCopy()` directly dereferences `browser.i18n`, which throws if `browser.i18n` is missing (non-extension runtime or incomplete test mocks). `ExtensionUpdateBanner` calls `getUpdatePromptCopy()` without passing a locale, so it exercises this path.
### Issue Context
- The repo already has a precedent for guarding i18n access: `lib/i18n.ts` wraps `browser.i18n.getMessage()` in a `try/catch`.
- Vitest setup currently defines `globalThis.browser.storage...` only; no `browser.i18n` is provided.
### Fix Focus Areas
- src/i18n/updatePrompt.ts[154-167]
- src/components/alias/ExtensionUpdateBanner.tsx[91-99]
- lib/i18n.ts[1-13]
- tests/setup.ts[1-19]
### Suggested fix (pick one)
1) **Code hardening**: Wrap the locale resolution in `try/catch` (like `t()`), falling back to `"en"` if any i18n access fails.
2) **Testability**: Extend `tests/setup.ts` to include a minimal `browser.i18n.getUILanguage` mock (and, if you plan to test the component, `browser.runtime.getManifest` as well).
Either approach prevents runtime crashes when the i18n namespace is absent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 8c8694c |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
This pull request is intentionally kept as a draft research branch and is not ready to merge.
The browser-managed update flow is harder to verify safely because a complete end-to-end test requires a separate Store-hosted beta/private extension with two published versions. Keep this PR isolated until that test setup exists.
Prototype scope
runtime.requestUpdateCheck();runtime.onUpdateAvailableandruntime.reload()flow;Research still required
update_availablereliably includes the target version across browsers;onUpdateAvailablefires;Existing validation
Notes
CHANGELOG.md.