feat(trash): archive instead of delete with recycle bin and retention purge - #16746
feat(trash): archive instead of delete with recycle bin and retention purge#16746DeJeune wants to merge 41 commits into
Conversation
… purge
Route deletes for topics, messages, agents, agent sessions, assistants, and
paintings through soft-delete (deletedAt) instead of hard delete, add a
Settings recycle bin (restore / permanent-delete / empty), and a daily
trash.purge job gated by the data.trash.retention_days preference.
- Schema: painting + agent_session gain deletedAt (migration 0018).
- DataApi: existing DELETE endpoints archive by default; ?permanent=true hard
deletes; new POST /{resource}/:id/restore + bulk restore; ?inTrash=true list.
- Archive writes only the container row (children stay intact for lossless
restore); pins/tags purged on archive to avoid list-JOIN hiding.
- Purge: JobManager 'trash.purge' handler + IpcApi trash.purge_now, batched
synchronous transactions, post-commit disk sweeps (failures logged, not
thrown).
- UI: Settings > Data > recycle bin per DESIGN.md, i18n across all locales.
- Read-path guards: session search/read and message parent/source existence
checks now filter out archived rows so live data can't attach to trash.
Note: file-blob reclamation for purged chat attachments / painting images is
delegated to PR #16727 (file_entry cleanup_policy); notes and knowledge are
out of scope.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: suyao <sy20010504@gmail.com>
There was a problem hiding this comment.
4 findings submitted as inline comments (3 must-fix, 1 non-blocking suggestion).
Note on Finding 1 originally flagged (AgentSessionService cascade filter): retracted after re-reading the PR diff. AgentService.deleteAgent already routes permanent: true through deleteByAgentIdTx intentionally (JSDoc at the call site: "Existence check up front (no isNull gate — trashed rows must be permanently deletable)"), and the soft-delete path uses the PR's new archiveByAgentIdTx (with isNull(deletedAt) filter at line 425-446 in the new file). This is design intent, not a bug.
Other 5 candidates refuted (sync transaction atomicity, RFC §4.2 separation, test-asserted ordering, etc.) — not surfaced to avoid noise.
| } | ||
|
|
||
| /** Format a deleted-at timestamp as `YYYY-MM-DD HH:mm`; missing/invalid → "—". */ | ||
| export function formatDeletedTime(ms: number | undefined): string { |
There was a problem hiding this comment.
[A2] Library-first violation: hand-rolled padStart instead of using dayjs.
dayjs@^1.11.11 is already a runtime dependency (package.json:303) and is used by every sibling file in this directory: LocalBackupSettings.tsx:163, NutstoreSettings.tsx:189, WebDavSettings.tsx:78, S3Settings.tsx:87. TrashSettings is the lone outlier.
CLAUDE.md Operational Rules: "Library-first, custom-last: Before writing custom code, check library/framework docs for built-in options or existing solutions."
Minimal patch (verified byte-for-byte equivalent across undefined/NaN/epoch 0/local-tz dates, including locale-stable digit output):
+ import dayjs from 'dayjs'
export function formatDeletedTime(ms: number | undefined): string {
if (ms === undefined) return '—'
- const date = new Date(ms)
- if (Number.isNaN(date.getTime())) return '—'
-
- const pad = (value: number) => value.toString().padStart(2, '0')
- return `${date.getFullYear()}-${pad(date.getMonth() + 1)}-${pad(date.getDate())} ${pad(date.getHours())}:${pad(
- date.getMinutes()
- )}`
+ if (!dayjs(ms).isValid()) return '—'
+ return dayjs(ms).format('YYYY-MM-DD HH:mm')
}Return type string is preserved; same import pattern as the four sibling files.
| * Returns `null` when retention is 0 (keep forever) or `deletedAtMs` is missing; | ||
| * otherwise `Math.ceil` of the remaining days clamped to >= 0. | ||
| */ | ||
| export function computeDaysRemaining( |
There was a problem hiding this comment.
[A3] Semantic mismatch: daysRemaining === 0 renders "Less than 1 day left" in an expired context.
computeDaysRemaining only returns 0 when remainingMs <= 0 (just expired or overdue), but the UI at TrashItemRow.tsx:32-39 routes daysRemaining < 1 to the days_remaining_lt_one i18n key ("Less than 1 day left" / "剩余不足 1 天") — implying time remains when actually it's expired.
Meanwhile the legitimate "< 1 day remaining but not yet expired" state has no UI representation at all.
Minimal patch: split computeDaysRemaining into two distinguishable sentinels so the UI can render the right copy.
if (retentionDays <= 0 || deletedAtMs === undefined) return null
const remainingMs = deletedAtMs + retentionDays * MS_PER_DAY - now
- return Math.max(0, Math.ceil(remainingMs / MS_PER_DAY))
+ if (remainingMs <= 0) return 0 // expired / overdue
+ if (remainingMs < MS_PER_DAY) return null // < 1 day remaining (not yet expired)
+ return Math.ceil(remainingMs / MS_PER_DAY)Plus in TrashItemRow.tsx split the branch:
- {daysRemaining < 1
- ? t('settings.data.trash.days_remaining_lt_one')
- : t('settings.data.trash.days_remaining', { count: daysRemaining })}
+ {daysRemaining === 0
+ ? t('settings.data.trash.days_remaining_expired')
+ : daysRemaining === null
+ ? t('settings.data.trash.days_remaining_lt_one')
+ : t('settings.data.trash.days_remaining', { count: daysRemaining })}Add settings.data.trash.days_remaining_expired ("Expired" / "已过期") to en-us.json, zh-cn.json, zh-tw.json.
| } | ||
| } | ||
|
|
||
| const handleDelete = (item: TrashItem) => |
There was a problem hiding this comment.
[A4] Restore + Delete race: Delete button not gated while restore is in flight.
This race applies to all six sections (TopicTrashSection:76, AgentTrashSection:134, SessionTrashSection:197, AssistantTrashSection:262, PaintingTrashSection:326, FileTrashSection:385) because they share the same shape.
Why it's reachable end-to-end:
pendingRestoreIdonly flows to the Restore button atTrashItemRow.tsx:46(loading={isRestoring}); the Delete button atTrashItemRow.tsx:51-60has nodisabledprop.useMutationdoes NOT cancel in-flight requests (useDataApi.ts:530-540is a bareawait swrTrigger(...); theinFlightParamsRefwarning at lines 506-522 is dev-only).- Restore and Delete are two distinct hook instances (different paths/methods), so the dev-only in-flight warning doesn't even cover them.
Failure trace: User clicks Restore on row X (pendingRestoreId = X.id, restore in flight). User clicks Delete on the same row X — Delete button allows the click, ConfirmDialog opens. Restore completes ("Restored" toast). User confirms Delete ("Permanently deleted" toast). End state: permanent: true routes to purgeManyByIdsTx hard-delete; row is permanently gone; user saw two contradictory toasts.
Minimal patch (symmetric with the existing Restore-button pattern):
// TrashItemRow.tsx (line ~52)
- <Button onClick={() => onDelete(item)}>
+ <Button onClick={() => onDelete(item)} disabled={isRestoring}>isRestoring is already passed down via TrashSection.tsx:85 — extend its consumer at TrashItemRow.tsx to apply disabled to the Delete button as well, not just loading to the Restore button.
| } | ||
| } | ||
|
|
||
| export const TopicTrashSection: FC<TrashDomainSectionProps> = ({ retentionDays, onRequestDelete }) => { |
There was a problem hiding this comment.
[B1] Suggestion (non-blocking): six section components share ~95% of TrashSection rendering shape but a useSoftDeletableSection factory would absorb only ~150-180 lines (≈40% reduction), not the ~280 → ~80 claimed in initial analysis.
Why the savings are smaller than they look: the six sections vary on six orthogonal axes that the factory would have to parameterize —
pagination:cursorvsoffset(4 fields vs 6)idParam::id/:agentId/:sessionId/ none — these match the backend route params (agents.ts:57/agentSessions.ts:61/topics.ts:52etc.), so they're not drift, they're a backend decision the factory must absorbnameField: 5 distinct getters —topic.name,agent.name,session.name,assistant.name,painting.prompt,entry.name + entry.extrefreshPaths: sessions additionally refresh/agents/*useIpc:FileTrashSectionusesipcApi.request('file.batch_*', { ids: [...] })instead ofuseMutation, withuseInvalidateCacheinstead of mutation refresh — a real dual-branchFileTrashSectionalso short-circuits onentry.origin === 'internal'fordeletedAt
No existing factory (useEntityListSection, useSoftDeletableSection, etc.) is present in the codebase, and this pattern is unique to TrashSettings/ — it's not a cross-page repetition.
Recommendation: defer to a follow-up refactor PR. The current copy is verbose but each section reads top-to-bottom without indirection. The factory would save ~40% LOC but introduce TS generics + dual branches + pagination-shape conversion that approach the duplication size.
…lete-domains Signed-off-by: suyao <sy20010504@gmail.com>
…entId The main merge moved the Settings primitives to @renderer/components/SettingsPrimitives and ErrorCode to @shared/data/api/errors, and made JobContext.parentId required. Repoint the trash settings + PaintingService test imports and add parentId to the trash purge job-context test mock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
The main merge moved FileHandle types into src/shared/data/types/file.ts and collapsed src/main/services/file/watcher/ into watcher.ts; repoint the two references so build:check's doc-link check passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…omains Replace the six stacked domain sections with a SelectDropdown category filter (topics default) that renders exactly one section at a time, and drop each section's now-redundant title header since the dropdown already names the selected category. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…ctions The context menu's single Delete action soft-deleted to the trash while its confirm dialog claimed the operation was irreversible. Add an Archive action (soft delete, no confirm) and repoint Delete at permanent deletion so the existing destructive confirm copy is accurate. The delete handler takes an archive|permanent mode; the inline quick-delete button keeps archiving. History rows share the registry, so its Delete now hard-deletes too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…le bin The Files page and Settings recycle bin rendered the same trashed-file data with different empty-trash scopes. Remove the files-page trash library, its restore/empty/permanent-delete flows, and the isTrash branches in the list/grid/context-menu; trashed files are managed solely in Settings > Data. Active-file deletion still archives internal files via file.batch_trash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
Signed-off-by: suyao <sy20010504@gmail.com>
Signed-off-by: suyao <sy20010504@gmail.com>
Code Review SummaryThis PR implements soft-delete (trash/archive) for six domains: topics, messages, agents, agent sessions, assistants, and paintings. Verified Implementation Quality:
No blocking issues found. The implementation correctly follows repository patterns for service layer transactions, error handling, lifecycle service patterns, and soft-delete guards. Tests are comprehensive and verify the key behaviors including purge ordering, batch processing, and error handling. 🤖 Generated with Claude Code |
Signed-off-by: suyao <sy20010504@gmail.com>
Code Review SummaryThis PR implements soft-delete (trash/archive) for six domains: topics, messages, agents, agent sessions, assistants, and paintings. Verified Implementation Quality:
No blocking issues found. The implementation correctly follows repository patterns for service layer transactions, error handling, lifecycle service patterns, and soft-delete guards. Tests are comprehensive and verify the key behaviors including purge ordering, batch processing, and error handling. 🤖 Generated with Claude Code |
…state
The sweep read only the direct children of `{userData}/Data/Agents` and
deleted everything its keep-set did not claim. The four app-owned runtime
roots live exactly there — `.claude/`, `.pi/`, `.dsh/`, `system/` — and
`agent_workspace.path` points at grandchildren (`system/{date}/{sessionId}`),
so the first scheduled run would have removed every session workspace plus
the Claude config and credentials.
Reclamation is now claim-based: only artifacts whose name resolves to a live
row are candidates, and anything unrecognized is left alone.
- agent dirs: only uuid-named children, matched against `agent.id`
- session workspaces: `system/{date}/{sessionId}` against `agent_workspace.path`
- runtime session state: each driver reclaims its own, keyed by the resume
tokens on surviving `agent_session_message` rows
The keep-set is re-read from the database after the purge transactions
commit, so nothing is handed out of the delete path and archived rows keep
their state for the whole retention window. A 5-minute mtime gate defers
artifacts an in-flight session is still writing.
Per-runtime layouts, each taken from the runtime that owns it:
- pi writes flat `{timestamp}_{token}.jsonl`
- dsh writes `{projectKey(cwd)}/{sessionId}/session.jsonl.zstd`, reclaimed a
project at a time because subagent runs get their own session directories
under ids Cherry never records and share the parent's cwd
- claude-code writes `{projects}/{cwd-slug}/{id}.jsonl` plus a `{id}/`
subagent-transcript directory, swept only under Cherry's own config dir —
the SDK's `deleteSession` resolves `CLAUDE_CONFIG_DIR` from the calling
process, which in main reaches the user's personal `~/.claude`, and the
Claude-login provider deliberately writes sessions there
Verified against a copy of a real dev instance: 51 live tokens, pi and dsh
reclaimed nothing, claude reclaimed 24 artifacts that sqlite confirms belong
to sessions no longer in the database, and the live transcripts sharing those
directories survived.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: suyao <sy20010504@gmail.com>
Messages are never soft-deleted, so the `message` purge domain and `MessageService.purgeExpiredTx` could not have had any rows to act on. Drop both, along with the test that hand-seeded a state no code path produces, and correct the RFC and breaking-changes notes that promised message recovery. Make `permanent` opt-in everywhere. `AgentService.deleteAgentForDelivery` (`?? true`) and the `AgentSessionService` delete family (`=== false`) still defaulted to a hard delete, so a caller that omitted the flag destroyed data while the identical topic call archived. Archiving a session now detaches its bound task schedule and fails pending cross-session deliveries, matching the delete path — a trashed session is as unreachable as a deleted one, and senders waiting on a completion would otherwise wait forever. Restoring an agent brings back the sessions archived with it, matched on the shared archive timestamp so separately trashed sessions stay in the trash. UI: - add the retention control; the docs told users to set it to 0 but nothing could change `data.trash.retention_days` - the topic context menu's permanent delete now says so, instead of sitting next to Archive under the same "Delete" label and icon - the history row's trash button archives, like its counterpart in the topic list, rather than hard-deleting Also translate the 49 trash keys left in English across 10 locales, give `retention_hint` a plural form, and remove the `file.empty_trash` route and i18n keys orphaned by dropping the files-page trash view. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code Review SummaryThis PR implements soft-delete (trash/archive) for six domains: topics, messages, agents, agent sessions, assistants, and paintings. Verified Implementation Quality:
No blocking issues found. The implementation correctly follows repository patterns for service layer transactions, error handling, lifecycle service patterns, and soft-delete guards. Tests are comprehensive and verify the key behaviors including purge ordering, batch processing, and edge cases like restoring an agent whose sessions were separately trashed. 🤖 Generated with Claude Code |
Both sides landed a migration at index 0011 — `0011_rare_vertigo` (agent_channel_session) upstream and the agent_session/painting `deleted_at` columns here. Resolved by taking main's chain and regenerating this branch's migration as `0012_mature_miss_america` rather than renumbering it, so the snapshot keeps a fresh id instead of forking the chain. `db:migrations:check` passes and the regenerated SQL is identical to the original two ALTERs. Everything else merged cleanly. None of the runtime layouts the agent orphan sweep depends on were touched upstream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
|
Code Review Findings Summary: The core archive/purge/restore logic is correct and safe. No blocking data integrity issues found. Consistency Issues Found (Warning): Three methods in AgentSessionService.ts are missing isNull(sessionsTable.deletedAt) guards:
Before this PR, sessions did not have deletedAt, so these methods did not need guards. Now that sessions support soft-delete, these methods should be updated for consistency. Fix: Add isNull(sessionsTable.deletedAt) to the WHERE clauses. Verified Safe:
|
…sessions Sessions had no `deletedAt` before this branch, so these three write paths never needed a soft-delete filter; now that they do, they were the only ones left mutating a trashed row where the `TopicService` counterparts refuse. - `advanceLastActivityAtTx` bumped activity for a session in the trash. An in-flight turn can still persist its final message after the archive commits, so the write now fails the transaction instead of landing on a trashed row. - `ensureTraceIdTx` minted a trace id for an archived session. - `reorderTx` accepted an archived session as the reorder target, and neither it nor `reorderBatchTx` scoped `applyMoves`, so trashed rows still acted as ordering neighbours for live ones. Reported by the automated reviewer on #16746; each path was checked for a real caller first — all three are reachable (message persistence, `AgentChatContextProvider`, and the `/agent-sessions/:id/order` endpoint). `touchUpdatedAtTx` is deliberately left unguarded: it has no `TopicService` counterpart to match, it only bumps metadata on a row that is already invisible, and it runs on teardown paths where throwing would be a regression. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
Code Review SummaryThis PR implements soft-delete (trash/archive) for six domains: topics, messages, agents, agent sessions, assistants, and paintings. Verified Implementation QualityArchive/Restore Logic:
Write-Path Guards:
Read-Path Guards:
Purge Flow:
API Endpoints:
ConclusionNo blocking issues found. The implementation is correct and complete. The previously flagged consistency issues have been addressed in commit 🤖 Generated with Claude Code |
![cherry-ai-bot[bot]](https://avatars.githubusercontent.com/in/3013849?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cherry Review · 阻塞
1 blocker · 10 warning · 2 notice
逐条见行内评论。
以下 2 条落在本次 diff 的行内定位之外，改为摘要送达（位置见每条开头）：
src/main/data/services/MessageService.ts:1894建议修复：Child message/session operations do not consistently enforce that their parent topic or session is active, so stale callers can mutate archived containers and make restoration lossy.inv_20add8818aa4b4d2#c1src/main/data/services/PaintingService.ts:390建议修复：Painting, agent, and newly created session order-key calculations include archived rows in their live neighborhood scans, so archived keys can affect first/last and relative ordering.inv_20add8818aa4b4d2#c2
| @@ -0,0 +1 @@ | |||
| export { TrashService } from './TrashService' | |||
There was a problem hiding this comment.
建议修复：The headless trash service and its private satellites do not meet the documented promotion bar for a features module, and the job handler is not organized under tasks/.
inv_20add8818aa4b4d2#c4
| const restoreMutation = useMutation('POST', '/topics/:id/restore', { refresh: ['/topics', '/topics/*'] }) | ||
| const deleteMutation = useMutation('DELETE', '/topics/:id', { refresh: ['/topics', '/topics/*'] }) | ||
|
|
||
| const handleRestore = async (item: TrashItem) => { |
There was a problem hiding this comment.
建议修复：Each trash section shares one template-path useMutation instance across rows, but only the currently restoring row is disabled, allowing concurrent row mutations to share and clobber mutation state.
inv_20add8818aa4b4d2#c8
The keep-sets are a single snapshot, so a dir created before its claiming row commits read as an orphan and was deleted outright. FRESHNESS_GATE_MS already existed but only reached the runtime-session pass; route the agent-dir and workspace-dir passes through reclaimStale so warm artifacts survive, and drop the emptied date dir with a non-recursive rmdir that fails on repopulation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
Permanent delete invalidated by-id wildcards, so SWR refetched rows that were just purged and cached the 404 (same hazard useTopic.ts already documents); restore keeps its wildcard since the row survives. Each section also backs every row with one useMutation instance, so a second in-flight action clobbered its state — freeze the section while one is pending. Also repoint the non-existent text-foreground-muted/border-border-muted classes at real tokens. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…he last topic The row button archives (recoverable) but was labelled Delete. The menu was also inverted: permanent delete was allowed on the last topic — documented as safe because the handler opens a fresh one — while the recoverable archive was hidden there. Both now gate on pinned only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
Bulk topic restore, permanent on the topic collection delete, purge_now.jobId, and permanent on ai.agent.sessions.delete were all added by this branch and have no production caller — the trash UI restores and purges one row at a time and reads only the terminal status. Cover the routing that does ship instead: permanent vs archive, restore, and inTrash forwarding at the handler seam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…up claims Pre-upgrade items are not all past the window — only those older than the retention period are purged, by the daily 03:00 job rather than at upgrade, and retention 0 skips the purge entirely. Per-item permanent delete is DB-only and leaves blobs to a background sweep; only empty-trash and the Files trash reclaim promptly. Also record the introducing PR and the assistant group lost on archive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…claude transcripts Underscore is a legal resume-token character, so splitting the pi filename stem on the last one truncated the token and made a live session read as an orphan — its transcript was then reclaimed. Split on the first instead; the timestamp prefix pi owns is dash-only. Claude's subagent dir had the mirror problem: it was only reachable via its jsonl sibling, so it leaked once that file was gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…le entries Every other destructive sweep stands aside on hasPendingRestore() — a staged restore puts artifacts on disk that the live DB does not claim yet, which is exactly the shape this sweep deletes. The retention purge had the matching hole: it filtered on deletedAt alone, so a file trashed from the Files page while a live painting still referenced it was hard-deleted, and the ref row FK-cascaded, stripping the image out from under the painting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…rt residue The purge only checked the abort signal once per domain, so a cancel landing mid-batch or during the filesystem sweeps went unseen — and because the terminal write matched on id alone, the late completion then overwrote a forced 'cancelled' with 'completed'. Check between batches and before each sweep, and let the first terminal write win. The tx-scoped purges stay silent by design, so the handler now owes the read-model refresh: without it a 03:00 run left every open list rendering deleted rows. Surface a non-clean sweep instead of swallowing it, so leftover blobs are not reported as success. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
… paths prompt_binding is polymorphic, so nothing but an explicit purge reclaims it. The archive path purges bindings, but permanently deleting a live assistant skips that path entirely, and the retention purge never purged them at all — which also strands bindings for assistants archived before this release. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
… restore refresh Trashing an internal file, or bulk-deleting agent sessions from history, moves rows to the recycle bin — the irreversible Delete wording promised otherwise. Restore mutations refreshed by-id wildcards, revalidating every cached row of the collection rather than the one restored. Also drop the permanent=true claim from the collection DELETE contract, which no longer accepts it, and record the prompt bindings an assistant loses on archive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
There was a problem hiding this comment.
Cherry Review · 阻塞
2 blocker · 10 warning · 2 notice
逐条见行内评论。
以下 3 条落在本次 diff 的行内定位之外，改为摘要送达（位置见每条开头）：
src/main/data/services/MessageService.ts:1504建议修复：Soft-deleted topics and agent sessions are not write barriers for in-flight message persistence. Topic message updates lack a live-topic predicate; session message upserts touch sessions without a deletedAt predicate, so pending-to-pending streaming writes can commit after archive while only a later terminal activity transition fails.inv_f91ca7f9c68cb3fa#c3src/main/data/services/PaintingService.ts:390建议修复：Painting create and reorder operations include archived paintings in the order-key neighborhood. Live target lookup filters deletedAt, but insertWithOrderKey and both applyMoves calls omit the live-row scope.inv_f91ca7f9c68cb3fa#c4src/main/ai/agentSession/AgentSessionDeliveryService.ts:169必须修复：Archiving a session does not settle all in-flight deliveries before the retry sweep runs. The no-argument kick discovers recoverable deliveries without filtering archived sessions; a later live-only lookup can fail an accepted delivery while archived, while restoring before retry can allow it to replay.inv_f91ca7f9c68cb3fa#c5
| // otherwise go unobserved until they finish. | ||
| let reclaimed = true | ||
| try { | ||
| const report = await application.get('FileManager').runSweep() |
There was a problem hiding this comment.
建议修复：Empty Trash can be reported and documented as complete while file reclamation remains incomplete. Entry cleanup handles only 100 candidates per pass and returns completed; FileManager does not fold entry-cleanup saturation or failures into the umbrella outcome, while the UI treats completed as success and the release note says cleanup happens immediately.
inv_f91ca7f9c68cb3fa#c0
| const handleRestore = async (item: TrashItem) => { | ||
| setPendingRestoreId(item.id) | ||
| try { | ||
| await runAction('restore', async () => { |
There was a problem hiding this comment.
建议修复：File restore and permanent-delete actions show success even when the requested entry failed. The batch IPC returns per-ID failures without rejecting, but the trash UI ignores succeeded and failed and displays a success toast for any resolved request.
inv_f91ca7f9c68cb3fa#c1
| ) | ||
| const totalPages = Math.ceil(total / 50) | ||
|
|
||
| const restoreMutation = useMutation('POST', '/agents/:agentId/restore', { |
There was a problem hiding this comment.
必须修复：Restoring an agent can leave its sessions visible in the stale session-trash list, after which permanent-delete from that list can hard-delete the restored session.
inv_f91ca7f9c68cb3fa#c2
Review update (based on head Confirmed resolved and marked as Resolved:
Still unresolved:
This round of review found no other new issues beyond existing comments. There are still real open items, so the conclusion remains Request changes. Original Content复审更新（基于 head 已确认解决并标记为 Resolved：
仍未解决：
本轮复审没有发现现有评论之外的其他新问题。当前仍有真实开放项，因此结论继续为 Request changes。 |
…le failures Restoring an agent also restores the sessions archived with it, but the session trash list was never refreshed — the stale rows still offered a purge that would hard-delete a now-live session. The file routes resolve with per-id outcomes rather than rejecting, so a failed restore or purge was toasted as success. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…rminal state The purge only checked the signal between DB batches, so a cancel arriving during the directory walk waited for the whole tree; thread it through the agent sweep and let an abort propagate instead of being logged as residue. finalizeJob also still published and resolved a second time after the guarded terminal write became a no-op, emitting a state that contradicted the forced cancellation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
The row action archives — permanent is never sent on that path — but it was labelled and confirmed as an irreversible delete, and last round's history relabel left the label and the dialog disagreeing outright. Give it its own archive copy that says the session can be restored from the trash. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
The RFC still read as an unimplemented design, pointed at a moved naming-conventions path, called the schema and drizzle SQL throwaway, and documented bulk restore plus a collection permanent-delete that were dropped for having no caller. The release note promised immediate reclamation, said nothing about restore differing by domain, and told users to do nothing right after telling them to set retention before upgrading. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
There was a problem hiding this comment.
Cherry Review · 阻塞
5 blocker · 11 warning · 1 notice
逐条见行内评论。
以下 6 条落在本次 diff 的行内定位之外，改为摘要送达（位置见每条开头）：
src/main/data/services/AgentSessionMessageService.ts:1534必须修复：Archiving a session can roll back when a completion-policy sender is already archived, because deletion preparation treats the archived sender as existent and writes a user message into it.inv_286279945f9c9c25#c1src/main/data/services/AgentSessionMessageService.ts:1048建议修复：Agent-session message reads and writes continue to address archived sessions through unscoped session lookups, allowing delivery and DataApi operations to mutate trash contents.inv_286279945f9c9c25#c2src/main/data/services/MessageService.ts:1896建议修复：Message creation and deletion can mutate an archived topic's message tree because topic ownership is loaded without requiring the topic to be live.inv_286279945f9c9c25#c3src/main/data/services/PaintingService.ts:390建议修复：Painting create and reorder operations calculate order keys using archived paintings, unlike the other soft-deleted ordered domains.inv_286279945f9c9c25#c4src/main/ai/agentSession/AgentSessionDeliveryService.ts:590必须修复：Delivery recovery and idle kicks continue to process accepted or delivering messages belonging to archived sessions, changing their status and breaking lossless restore.inv_286279945f9c9c25#c8src/renderer/components/history/AssistantHistoryRecords.tsx:231建议修复：History bulk-delete presents an irreversible Delete action even though its handler performs archive-only deletion.inv_286279945f9c9c25#c12
已采纳的说明（上一轮的回复经查证成立，本轮不再计入）：
src/main/ai/runtime/pi/PiRuntimeDriver.ts:102Accepted that Pi resume-token parsing now splits at the first underscore; the current parser and verifier verdict establish that the original token-truncation issue is resolved.src/main/data/services/AssistantService.ts:654Accepted that assistant permanent deletion and retention purge clear prompt bindings while archive retains them; the current service implementation establishes this.src/main/features/trash/trashPurgeJobHandler.ts:136Accepted that scheduled purge now emits post-commit per-domain read-model notifications; the current handler establishes this.src/main/data/services/FileEntryService.ts:842Accepted that retention purge protects file entries with persistent references; the current purge predicate establishes this.src/main/features/trash/agentOrphanSweep.ts:49Accepted that agent orphan sweeping honors the restore journal fail-safe; the current sweep guard establishes this.src/main/ai/runtime/registerDrivers.ts:66Accepted that Claude runtime reclamation treats leftover transcript directories as independently identifiable artifacts; the current driver implementation establishes this.src/renderer/pages/files/FilesPage.tsx:580Accepted that recoverable file actions use Archive wording; the current Files page establishes this.src/renderer/pages/settings/DataSettings/TrashSettings/TrashDomainSections.tsx:64Accepted that restore refresh was narrowed to specific entity paths; the current mutation configuration establishes this.src/shared/data/api/schemas/topics.ts:202Accepted that collection topic deletion is documented as archive-only; the current contract documentation establishes this.v2-refactor-temp/docs/breaking-changes/2026-07-04-topic-delete-moves-to-trash.md:16Accepted that assistant restore documentation now states group and prompt bindings do not return; the current breaking-change document establishes this.
| .get('DbService') | ||
| .getDb() | ||
| .update(sessionsTable) | ||
| .set({ deletedAt: null }) |
There was a problem hiding this comment.
建议修复：Restoring an individual agent session can leave it active while its parent agent remains archived, making it visible but unusable at runtime.
inv_286279945f9c9c25#c0
| * done. | ||
| */ | ||
| export const trashPurgeJobHandler: JobHandlerFor<'trash.purge'> = { | ||
| recovery: 'singleton', |
There was a problem hiding this comment.
建议修复：Singleton recovery treats scheduled retention purges and manual empty-trash jobs as interchangeable, so restart recovery can cancel an empty-trash operation.
inv_286279945f9c9c25#c5
| @@ -0,0 +1 @@ | |||
| export { TrashService } from './TrashService' | |||
There was a problem hiding this comment.
建议修复：The trash modules are placed under features/trash even though the documented promotion rule routes this small headless capability through services/trash.
inv_286279945f9c9c25#c7
| const runAction = useTrashActionRunner() | ||
| const [pendingRestoreId, setPendingRestoreId] = useState<string | null>(null) | ||
|
|
||
| const { pages, isLoading, isRefreshing, error, hasNext, loadNext, refresh } = useInfiniteQuery('/topics', { |
There was a problem hiding this comment.
必须修复：Stale paginated or cross-window trash rows can permanently delete entities that have since been restored or purged.
inv_286279945f9c9c25#c11
|
|
||
| ## Notes for release manager | ||
|
|
||
| Part of the archive-instead-of-delete rollout (RFC: v2-refactor-temp/docs/archive/rfc-archive.md). One aggregated release-note entry should cover all domains (topics, assistants, agents, sessions, paintings, files — not single messages); this fragment is the canonical one — sibling workstreams intentionally do not add their own to avoid duplicates. Out of scope by design decision: knowledge (excluded) and notes (deferred, see RFC §4.5). |
There was a problem hiding this comment.
建议修复：The release-manager note leaves an existing file-cleanup fragment that still says deleting topics or paintings immediately reclaims files, contradicting this PR's archive-first behavior.
inv_286279945f9c9c25#c14
| * promises during shutdown, so a request pending at quit never resolves; | ||
| * acceptable for this fire-from-UI path. | ||
| */ | ||
| async purgeNow(): Promise<{ status: TerminalJobStatus }> { |
There was a problem hiding this comment.
必须修复：Manual empty-trash can report success after database purge while file reclamation is incomplete.
inv_286279945f9c9c25#c15
The previous guard compared statuses, so a handler that resolved as 'cancelled' after a forced cancel still republished and re-resolved the waiters. Have setTerminalTx report whether it actually wrote — it only touches a non-terminal row — and key the guard off that instead. Also relocates trash to services/trash: three files is a topic subdirectory, and features/ is earned only by a large multi-file domain (main-process §2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…etention off Empty Trash toasted unqualified success: the sweeps are batch-capped and stand aside during a restore, so the rows being gone does not mean the space came back. Carry the job's reclaimed flag out through purgeNow and say so. Retention 0 also returned before the sweeps, which left residue from permanent deletes stranded forever — it disables the row purge, not disk reclamation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
Clearing deletedAt on a session whose agent is still archived brings it back visible but unusable — the agent it needs is gone. Refuse it and point at the agent restore, which brings its sessions back anyway. Also corrects two stale contracts: the pi comment still described the last-underscore split the parser no longer uses, and the reclaim contract called the keep-set live sessions when it deliberately covers archived ones so restore stays lossless. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
The trash lists never subscribed to DataApi change notifications, so a restore or purge from another window left rows on screen whose permanent-delete would hard-delete a live entity. Subscribe each section to its own collection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
…fragment The upgrade steps told users to set trash retention to 0 before upgrading, but that setting ships with this release. Point them at first launch instead. The sibling file-cleanup fragment still claimed deleting a topic or painting reclaims its files immediately, which archiving defers to the purge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: suyao <sy20010504@gmail.com>
Signed-off-by: suyao <sy20010504@gmail.com>
Signed-off-by: suyao <sy20010504@gmail.com>
There was a problem hiding this comment.
Cherry Review · 阻塞
1 blocker · 5 warning
逐条见行内评论。
以下 1 条落在本次 diff 的行内定位之外，改为摘要送达（位置见每条开头）：
src/main/data/services/MessageService.ts:377建议修复：Archived topics remain readable through message APIs.inv_258932a21e9b5ead#c2
已采纳的说明（上一轮的回复经查证成立，本轮不再计入）：
src/main/ai/runtime/pi/PiRuntimeDriver.ts:102Accepted that Pi resume-token parsing now splits at the first underscore; the current parser and verifier verdict establish that the original token-truncation issue is resolved.src/main/data/services/AssistantService.ts:654Accepted that assistant permanent deletion and retention purge clear prompt bindings while archive retains them; the current service implementation establishes this.src/main/features/trash/trashPurgeJobHandler.ts:136Accepted that scheduled purge now emits post-commit per-domain read-model notifications; the current handler establishes this.src/main/data/services/FileEntryService.ts:842Accepted that retention purge protects file entries with persistent references; the current purge predicate establishes this.src/main/features/trash/agentOrphanSweep.ts:49Accepted that agent orphan sweeping honors the restore journal fail-safe; the current sweep guard establishes this.src/main/ai/runtime/registerDrivers.ts:66Accepted that Claude runtime reclamation treats leftover transcript directories as independently identifiable artifacts; the current driver implementation establishes this.src/renderer/pages/files/FilesPage.tsx:580Accepted that recoverable file actions use Archive wording; the current Files page establishes this.src/renderer/pages/settings/DataSettings/TrashSettings/TrashDomainSections.tsx:64Accepted that restore refresh was narrowed to specific entity paths; the current mutation configuration establishes this.src/shared/data/api/schemas/topics.ts:202Accepted that collection topic deletion is documented as archive-only; the current contract documentation establishes this.v2-refactor-temp/docs/breaking-changes/2026-07-04-topic-delete-moves-to-trash.md:16Accepted that assistant restore documentation now states group and prompt bindings do not return; the current breaking-change document establishes this.
| return agent | ||
| } | ||
|
|
||
| purgeExpiredTx(tx: DbOrTx, cutoffMs: number, limit: number): string[] { |
There was a problem hiding this comment.
必须修复：Retention purge hard-deletes agents without deleting their enabled agent.task schedules.
inv_258932a21e9b5ead#c0
| .where( | ||
| and( | ||
| agentFilter, | ||
| inTrash ? isNotNull(sessionsTable.deletedAt) : isNull(sessionsTable.deletedAt), |
There was a problem hiding this comment.
建议修复：Sessions retained when their agent is archived still appear in default active session reads.
inv_258932a21e9b5ead#c1
| * the `painting_file_ref` rows, so the returned entity's files are intact. | ||
| * NOT_FOUND when the painting doesn't exist or is not in the trash. | ||
| */ | ||
| restore(id: string): Painting { |
There was a problem hiding this comment.
建议修复：Archive and restore mutations for several trash domains do not broadcast DataApi changes, leaving other windows stale.
inv_258932a21e9b5ead#c3
| { | ||
| name: 'topic', | ||
| purgeExpiredTx: (tx, cutoffMs, limit) => topicService.purgeExpiredTx(tx, cutoffMs, limit), | ||
| notifyPurged: (ids) => topicService.notifyReadModelChange(ids, 'membership') |
There was a problem hiding this comment.
建议修复：Trash purge notifications omit the child message read models deleted with topics and sessions.
inv_258932a21e9b5ead#c4
| @@ -758,7 +768,12 @@ export class AgentSessionService { | |||
| if (reassigned && current.taskScheduleId) { | |||
There was a problem hiding this comment.
建议修复：A failed PATCH against an archived task-bound session still clears its task relation.
inv_258932a21e9b5ead#c5
Signed-off-by: suyao <sy20010504@gmail.com>
Signed-off-by: suyao <sy20010504@gmail.com>
Signed-off-by: suyao <sy20010504@gmail.com>
2 participants
What this PR does
Before this PR:
deletedAtcolumn already existed on several tables but was only actually written by assistants.After this PR:
DELETEendpoints setdeletedAtand route the row to a recycle bin (Settings → Data → Recently deleted) with per-domain restore / permanent-delete / empty-trash.?permanent=truestill hard-deletes,?inTrash=truelists trashed rows, andPOST /{resource}/:id/restore(+ bulk) recovers them.trash.purgeJobManager task gated by a newdata.trash.retention_dayspreference (default 30,0= never), plus antrash.purge_nowIpcApi command for "empty trash"; disk reclamation runs post-commit and failures are logged rather than thrown.Fixes #
Why we need it and why it was done in this way
Soft-delete plumbing (
deletedAt+isNullread filters) already existed for topics/messages/agents, so archiving was the minimum-surface way to make deletes recoverable without changing renderer delete call sites.The following tradeoffs were made:
The following alternatives were considered: a dedicated
archivedAtcolumn (rejected — reusesdeletedAt); a.trashdirectory move for notes (deferred, seev2-refactor-temp/docs/archive/rfc-archive.md§4.5).Links to places where the discussion took place: design RFC at
v2-refactor-temp/docs/archive/rfc-archive.md.Breaking changes
Deleting these entities no longer removes them immediately — they go to the recycle bin and are auto-purged after
data.trash.retention_daysdays (default 30). Pins and tags are not restored when an item is recovered. Details inv2-refactor-temp/docs/breaking-changes/2026-07-04-topic-delete-moves-to-trash.md.Special notes for your reviewer
file_entry.cleanup_policy); both branches add migration0018and touchMessageService/PaintingService/TopicService/FileEntryService/orphanSweep.ts, so whichever lands second must regenerate its migration (never rename) and resolve those conflicts. See RFC §6.typecheck:node+typecheck:webclean,i18n:checkpass,db:migrations:checkpass, and the touched service/handler/UI test suites green.Checklist
mainfor active development,v1for v1 maintenance fixes/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note