Skip to content

fix: add confirmation dialog for extraction history deletion #2497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xxiaoxiong wants to merge 7 commits into
base: main
Choose a base branch
from
+11 −1
Open

fix: add confirmation dialog for extraction history deletion #2497

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6d3051a
fix: add confirmation dialog for extraction history deletion
xxiaoxiong May 21, 2026
78ec9ac
fix: add Chinese translation for extraction delete confirmation
xxiaoxiong May 21, 2026
1672a69
fix: add memoryExtractionDeleteConfirm to Dict type definition
xxiaoxiong May 21, 2026
b740e69
chore: trigger CI re-run
xxiaoxiong May 21, 2026
6d6e2a4
chore: trigger CI status refresh
xxiaoxiong May 21, 2026
7a42c81
test: update MemorySection test to confirm deletion dialog
xxiaoxiong May 21, 2026
6360dec
test: use dialog scope to find confirmation Delete button
xxiaoxiong May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/web/src/components/MemorySection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,7 @@ export function MemorySection({
}, [indexDraft, fireFlash]);

const onDeleteExtraction = useCallback(async (id: string) => {
if (!window.confirm(t('settings.memoryExtractionDeleteConfirm'))) return;
Copy link
Copy Markdown
Contributor

@Siri-Ray Siri-Ray May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call now uses settings.memoryExtractionDeleteConfirm, but that key is still not declared in apps/web/src/i18n/types.ts and is only present in en.ts and zh-CN.ts. The i18n provider types t as keyof Dict and each locale as Dict, so this changed line is outside the typed key set and the other locale dictionaries remain missing the required entry once the key is added. That can keep @open-design/web from typechecking/building even though the confirmation behavior itself is correct. Please add settings.memoryExtractionDeleteConfirm to Dict next to settings.memoryExtractionDelete, then add the corresponding value to every locale file under apps/web/src/i18n/locales/ using localized text or the existing English fallback pattern used nearby.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Copy link
Copy Markdown
Contributor

@Siri-Ray Siri-Ray May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new confirmation branch needs the web tests to opt into the confirmed path. The live Web workspace tests check is currently failing in tests/components/MemorySection.test.tsx at the existing deletes a single extraction row without clearing the whole history case: jsdom reports Not implemented: Window's confirm() method, window.confirm(...) returns a falsy value, this line returns early, and the test still finds Remember I prefer dark mode instead of seeing the row removed. That is a PR-introduced CI failure on the primary regression test for this behavior. Please update the MemorySection tests to stub window.confirm to true for the existing single-delete test, and add the matching false case that asserts no delete request is sent and the row remains visible. That keeps both confirmation outcomes covered and gets pnpm --filter @open-design/web test green again.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Copy link
Copy Markdown
Contributor

@Siri-Ray Siri-Ray May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new confirm guard is correct for users, but the existing apps/web/tests/components/MemorySection.test.tsx case deletes a single extraction row without clearing the whole history still clicks this path without stubbing window.confirm. In jsdom, the unimplemented confirm returns a falsy value, so this line returns before deleteExtraction(id) and the current Web workspace tests check remains red. Please mock window.confirm to return true in that existing single-delete test and add the matching cancel case that returns false, asserts no /api/memory/extractions/ex-1 DELETE request is sent, and keeps the row visible.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Copy link
Copy Markdown
Contributor

@Siri-Ray Siri-Ray May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new confirmation guard changes the existing single-row delete path, but the web component test for deletes a single extraction row without clearing the whole history still clicks the delete button without stubbing window.confirm. In jsdom that confirm path is not implemented and returns a falsy value, so this line returns before deleteExtraction(id) and the row remains visible; the current Web workspace tests check is still failing. Please mock window.confirm to return true in the existing delete test and add the cancel case that returns false, asserts no /api/memory/extractions/ex-1 DELETE request is sent, and verifies the row remains visible.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

// Optimistic removal: drop the row immediately so the click feels
// instant. The SSE 'deleted' event will arrive moments later and is
// a no-op against an already-removed id; if the request fails we
Expand All @@ -1373,7 +1374,7 @@ export function MemorySection({
if (!ok) {
void reloadExtractions();
}
}, [reloadExtractions]);
}, [reloadExtractions, t]);

const onClearExtractions = useCallback(async () => {
if (!window.confirm(t('settings.memoryExtractionsClearConfirm'))) return;
Expand Down
2 changes: 2 additions & 0 deletions apps/web/src/i18n/locales/en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2148,6 +2148,8 @@ export const en: Dict = {
'settings.memoryExtractionWritten': 'written',
'settings.memoryExtractionDuration': 'in',
'settings.memoryExtractionDelete': 'Delete',
'settings.memoryExtractionDeleteConfirm':
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Declare new translation key in Dict before using it

The new settings.memoryExtractionDeleteConfirm key is added to en.ts, but this commit does not add the key to apps/web/src/i18n/types.ts. In this repo, t is typed with DictKey and locale dictionaries are typed as Dict, so introducing an undeclared key causes TypeScript errors (invalid key at call sites and/or extra property in the locale object), which breaks @open-design/web typecheck for all environments.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@Siri-Ray Siri-Ray May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new key is only added to en.ts, but the web i18n layer requires every key to be declared in apps/web/src/i18n/types.ts and implemented by each locale object. The changed line introduces settings.memoryExtractionDeleteConfirm, while Dict still jumps from settings.memoryExtractionDelete to settings.memoryExtractionsClear, and the other locale files do not define this key. Because useT() accepts keyof Dict and every locale export is typed as Dict, this can break @open-design/web typecheck and block builds. Please add the key to Dict and to all locale files in the same change, using localized text or the repo's existing fallback-English pattern for nearby untranslated strings.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

'Delete this extraction record? This cannot be undone.',
'settings.memoryExtractionsClear': 'Clear',
'settings.memoryExtractionsClearTitle': 'Clear all extraction history',
'settings.memoryExtractionsClearConfirm':
Expand Down
1 change: 1 addition & 0 deletions apps/web/src/i18n/locales/zh-CN.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2114,6 +2114,7 @@ export const zhCN: Dict = {
'settings.memoryExtractionWritten': '写入',
'settings.memoryExtractionDuration': '耗时',
'settings.memoryExtractionDelete': '删除',
'settings.memoryExtractionDeleteConfirm': '确定要删除此抽取记录吗？此操作无法撤销。',
'settings.memoryExtractionsClear': '清空',
'settings.memoryExtractionsClearTitle': '清空整个抽取历史',
'settings.memoryExtractionsClearConfirm': '确定要清空整个抽取历史吗？此操作无法撤销。',
Expand Down
1 change: 1 addition & 0 deletions apps/web/src/i18n/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ export interface Dict {
'settings.memoryExtractionWritten': string;
'settings.memoryExtractionDuration': string;
'settings.memoryExtractionDelete': string;
'settings.memoryExtractionDeleteConfirm': string;
Copy link
Copy Markdown
Contributor

@Siri-Ray Siri-Ray May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line makes settings.memoryExtractionDeleteConfirm a required member of every Dict, but the PR only adds values in en.ts and zh-CN.ts; the other locale files under apps/web/src/i18n/locales/ still lack this property. Because each locale export is typed as Dict, adding the key here without filling every locale keeps the typed locale contract incomplete and can keep the web validation/typecheck jobs red. Please add the same key to all remaining locale dictionaries, using localized text or the existing English-fallback pattern used nearby.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Copy link
Copy Markdown
Contributor

@Siri-Ray Siri-Ray May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding settings.memoryExtractionDeleteConfirm here makes it a required member of every locale dictionary, but this PR only adds values in en.ts and zh-CN.ts. The remaining locale files under apps/web/src/i18n/locales/ still do not define the key, while each locale export is typed as Dict; that leaves the typed i18n contract incomplete and keeps validation/typecheck red on this head. Please add the same key to every locale file, using localized text where available or the repo's existing English-fallback pattern for nearby strings.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Copy link
Copy Markdown
Contributor

@Siri-Ray Siri-Ray May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding settings.memoryExtractionDeleteConfirm here makes it a required member of every locale dictionary, but this PR only adds values in en.ts and zh-CN.ts. The repo has locale objects for the rest of apps/web/src/i18n/locales/ as well, and each export is typed as Dict, so the changed interface line leaves those dictionaries incomplete even though the new key is valid at the call site. Please add settings.memoryExtractionDeleteConfirm next to settings.memoryExtractionDelete in every remaining locale file, using localized copy where available or the existing English-fallback pattern used by nearby untranslated strings.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

'settings.memoryExtractionsClear': string;
'settings.memoryExtractionsClearTitle': string;
'settings.memoryExtractionsClearConfirm': string;
Expand Down
Loading