Skip to content

feat(desktop): show archived-task cleanup notices outside Settings - #5961

Merged
liugddx merged 3 commits into
apache:mainfrom
liugddx:feat/retention-notices
Oct 8, 2026
Merged

liugddx merged 3 commits into
apache:mainfrom
liugddx:feat/retention-notices

Conversation

@liugddx

@liugddx liugddx commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

Archived-task cleanup results and clock pauses are currently visible only in Storage settings. This follow-up shows a Host-scoped notice in the application shell, with an action that opens that Host's Archived tasks settings.

The observer reads only already-connected Hosts while the window is visible and focused. Hosts with retention enabled are checked at most once a minute; disabled Hosts are checked at most once every 15 minutes, including across focus changes. A policy enabled on another Client can therefore take up to 15 minutes to be discovered. Focus and visibility changes trigger refreshes; blur does not.

The last announced result is persisted per Host on this Desktop. Successive cleanup batches are coalesced with a 15-minute reminder cooldown. Clock warnings bypass that cooldown and are dismissed when the Host clears them or when the user views them in Settings. Settings acknowledgements trigger an immediate observer refresh, and are stored separately from announcement tokens so a warning is not withdrawn merely because it was announced. Hidden windows and stale responses do not acknowledge results. Same-name Hosts retain separate notices.

Depends on #5902. This PR currently includes its commits because the base PR has not merged. The notification changes are commits 0109b73f4, ab5651665 and bd5b62650 (inventory totals); review the incremental diff. I will rebase onto main after #5902 merges. It adds no Host protocol operation or retention policy change.

Part of #5776 / #5899. Idle archiving remains tracked separately in #5919.

The AppShell alias/destructuring changes are behavior-preserving. They reduce its non-trivia token count from 7333 to 7330 while adding the notification mount, keeping this frozen root within its existing renderer architecture budget. The budget is not raised.

Validation: Desktop main build and all four typechecks pass; 22 focused tests pass, covering persistence, cooldown, clock warnings, Settings acknowledgements and toast withdrawal, disabled-Host backoff across focus changes, event subscriptions and cleanup, visibility, disconnection, stale responses, disposal, and real toast navigation in all three locales for same-name Hosts. Format, lint, Knip (Desktop), ASF headers, locale hygiene, app-shell hooks, and strict renderer architecture checks pass. Hosted checks passed on c682972c7; the review-fix commit requires a fresh CI run.

Limits: the notices are in-app rather than native OS notifications. The Host reports only its latest deletion batch, so the notice states the latest cleanup result rather than claiming a total across missed batches.

@github-actions github-actions Bot added the effort/XXL Over 2500 readable lines label Oct 4, 2026
@liugddx
liugddx marked this pull request as ready for review October 4, 2026 06:49

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Review of exact head c682972c.

This PR is stacked on #5902. Its 8 commits are #5902's 7 (c1caceb2...717841a0, the same SHAs) plus c682972c "notify about archived-task cleanup outside Settings", and c682972c^ == 717841a0. This review covers only c682972c: 13 files, +620/-17. #5902 is reviewed separately. This PR can only merge after #5902, and it inherits #5902's epoch-205 collision with #5709.

What it does. ArchiveRetentionNotices is mounted in the app shell. While the window is visible and focused, it polls storage.retention.query on every enabled, ready Host profile, once a minute and again on profile changes or focus/blur/visibility changes. It shows three kinds of toast:

  • an info toast for a new lastDeletion, throttled to one per Host every 15 minutes by Client time;
  • a sticky warning toast for a hold;
  • a sticky warning toast for a backward-clock pause.

Results are identified by Host time and stored in localStorage per hostId. When the Settings retention section loads, it acknowledges the results it showed, so they are not announced again.

Correctness. I found no P0-P2 issues.

  • Observing never starts a Host: only enabled && readiness === 'ready' entries are polled.
  • A Host whose query fails is skipped, and the other Hosts are still checked.
  • When the Host list changes or the observer is disposed mid-flight, late results are discarded (version/closed checks).
  • Duplicate profiles for one hostId are queried once.
  • Warnings are cleared when the Host clears them or the Host goes away.
  • Corrupt localStorage decodes to {}, so it cannot suppress notices.
  • The toast action goes to archived-tasks, which renders ArchiveRetentionSection (tasks-settings-page.tsx:92,101).
  • The services object is created once, so the useEffect([services]) does not restart.
  • A clock-skew edge: if a persisted deletionAt comes from a Host clock that was ahead, it does not hide later deletions, because the Host itself pauses until real time passes its recorded observedAt.
  • The new test passes locally 11/11, and CI is green.

Minor (P3):

  1. Polling cost on hosts where retention is off. storage.retention.query runs countArchiveRetentionCandidates, a GROUP BY over all archived rows joined to the projection. The notices observer runs it every 60s for every ready Host while the window is focused, and also on each focus/blur. That includes Hosts where retention is disabled, which is the default and covers nearly everyone. Those Hosts can never produce a hold or pause, and produce a lastDeletion only if retention was on before. Options: back off when enabled === false && !lastDeletion, drop blur from the triggers (blur cannot make a notice presentable), or add a preview-free query mode.
  2. Settings acknowledgement leaves sticky warnings on screen. acknowledgeRetentionResults writes the seen-state, but a warning toast already shown with duration: 0 stays until the Host clears the hold (up to a day) or the user closes it. That is a small UX inconsistency.
  3. Unrelated refactors in app-shell.tsx: the activeCatalogSession to activeSession rename, the destructuring changes, and the inlined parentId. They look like they exist to stay within the nonTriviaTokens budget (7333 to 7330). They are behaviour-neutral but enlarge the diff. Consider splitting them out or noting them in the description.

CI / merge: all checks pass. The PR is MERGEABLE and BLOCKED on review. It is gated on #5902.

Comment thread apps/desktop/src/renderer/platform/desktop/create-storage-usage-services.ts Outdated
Comment thread apps/desktop/src/renderer/features/storage-usage/model/retention-notices.ts Outdated

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review of exact head 3f1409c0 (previous review: c682972c).

Stacking. The PR is still stacked on #5902's current head 717841a0, with no rebase (merge-base with main is unchanged). c682972c is identical. The only new commit is 3f1409c0 "address retention notice review feedback" (5 files, +149/-14), and this review covers only that commit. It can merge only after #5902.

Previous findings

  1. Polling cost when retention is off: fixed. After the observer reads a disabled Host, it skips that Host for 15 minutes, including on focus and visibility changes (retention-notices.ts:135-139,148-149). A Client clock rollback forces a read, and entries for removed Hosts are pruned.
  2. blur trigger that did nothing: fixed. The listener is removed.
  3. Sticky warning toast stays after Settings shows it: fixed. A new acknowledgedWarning is set only by the Settings acknowledgement, never by the observer's own announcement. Writing it notifies the change listeners, so the observer refreshes and dismisses the matching active toast (L129-134, L168). The persisted acknowledgement survives the observer's state merge (L157).
  4. Unrelated app-shell.tsx renames: unchanged, since this commit doesn't touch that file. Not repeated inline.

New: one low-priority P3, inline: the disabled backoff isn't reset when retention is enabled from Settings. No P0-P2.

Verification. CI is all green, and the PR is MERGEABLE (blocked on review). In a local build at this head, the archive-retention-notices, -section and -candidates tests pass 19/19, including the three new tests. The new value import of the feature barrel from platform/desktop creates no import cycle.

const elapsed = disabledAt === undefined ? undefined : now() - disabledAt;
// Focus changes do not turn the default-disabled policy into a full
// candidate scan every minute. A Client clock rollback forces a read.
if (elapsed !== undefined && elapsed >= 0 && elapsed < DISABLED_POLL_MS) continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3 (low): the disabled backoff for a Host is cleared only when a later read shows it enabled (L148). Enabling retention in Settings (archive-retention-section.tsx:123) only reloads the section, and acknowledgeRetentionResults notifies listeners only for a new acknowledgedWarning. So for up to 15 minutes after enabling, the observer won't read that Host. The impact is small: the Host deletes nothing before enabledAt + days, so only a hold or pause toast could be late, and Settings shows that banner in the meantime. The same delay applies when another client enables retention. If you want to close the gap, clear disabledReadAt for the Host after a successful setRetention (for example, by notifying the observer's listeners).

@liugddx
liugddx force-pushed the feat/retention-notices branch from 3f1409c to bd5b626 Compare October 6, 2026 04:55

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review: 3f1409c0 (our last reviewed head) to bd5b6265. The PR is restacked on #5902's new head 3fc3970b: that head is an ancestor, and the 3 commits after it are this PR's own. I reviewed only 3fc3970b..bd5b6265 and compared it with the previous own range 717841a0..3f1409c0. The two diffs are identical except for regenerated surface inventory totals (330 to 331 files, aligned 326 to 327) and hunk offsets/blob indexes in the inventory files. The notice observer, the Settings acknowledgement path, the ports and the tests are byte-identical to the previous own range.

Findings. Nothing new at P0-P2. The P3 we raised earlier about the disabled-host backoff (retention-notices.ts:139) is untouched, and so is the earlier unrelated app-shell.tsx rename note, so I am not raising either again. The PR inherits #5902's epoch, so the epoch-206 coordination note on #5902 applies here too.

Status: MERGEABLE (merge state BLOCKED). It merges cleanly with main 3597abe8 (git merge-tree). CI: 12 pass, 1 skipped, and test was still pending when I checked. Merge after #5902.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Fresh review of this PR's own range 3fc3970b..bd5b6265 (stacked on #5902).

No P0-P2 findings. There are no protocol, preload or main-process changes: the observer reuses #5902's storage.retention.query and the existing runtimeHostProfiles bridge, so it stays within #5902's epoch. Dedupe by Host time, the Settings acknowledgement race (re-read after await, plus a version bump on ack) and Host removal mid-query all look correct and are tested. The notice wording reuses the Settings strings and is accurate in en, zh-CN and zh-TW.

P3s are inline: focus-triggered query cost for enabled Hosts, the disabled backoff not clearing on enable, cross-pass deletion under-reporting, the missing locale in formatBytes, timing for the 12s actionable toast, and the untested Settings ack wiring.

CI is green, and the PR merges cleanly with main. Merge after #5902.

const elapsed = disabledAt === undefined ? undefined : now() - disabledAt;
// Focus changes do not turn the default-disabled policy into a full
// candidate scan every minute. A Client clock rollback forces a read.
if (elapsed !== undefined && elapsed >= 0 && elapsed < DISABLED_POLL_MS) continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3: Only disabled Hosts back off. Every enabled, ready Host is re-queried on each focus/visibilitychange and every 60s while focused, and storage.retention.query runs countArchiveRetentionCandidates (a SQLite GROUP BY aggregate; a network round-trip for remote Hosts). That is the same scan the comment above calls too costly per minute. Consider a minimum interval since the last read for focus-triggered refreshes. Related: this backoff is not cleared when the user enables retention in Settings (setRetention fires no change listener), so a just-enabled Host stays unread for up to 15 minutes.

const deletion = retention.lastDeletion;
const time = now();
const cooled = state.notifiedAt === undefined || time < state.notifiedAt || time - state.notifiedAt >= RETENTION_NOTICE_COOLDOWN_MS;
if (deletion && deletion.count > 0 && deletion.at > (state.deletionAt ?? -1) && cooled) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3: lastDeletion is per sweep pass, so passes that finish while the app is closed or unfocused, or that are superseded during the cooldown, are never announced, and the toast shows only the latest pass's count. The "Last automatic cleanup" wording is literally accurate, but users may read N as the total. Consider accumulating unannounced counts, or wording that makes the scope explicit.

const description = notice.kind === 'deletion'
? copy.lastCleanup(
notice.deletion.count,
notice.deletion.bytes === undefined ? undefined : formatBytes(notice.deletion.bytes),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3: Settings formats this with the UI locale (formatBytes(bytes, props.locale), archive-retention-section.tsx:227). Here the locale is omitted, so the same deletion can show its size differently in the toast and in Settings. Suggest formatBytes(notice.deletion.bytes, locale).

diagnosticTarget: { profileId: host.profileId },
description,
variant: notice.kind === 'deletion' ? 'info' : 'warning',
duration: notice.kind === 'deletion' ? 12_000 : 0,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3 (a11y): A 12s auto-hiding toast with an action may be hard to reach by keyboard or screen reader unless the toast pauses on hover/focus (WCAG 2.2.1). The information remains in Settings, so this is low impact. Consider a longer duration, or confirm that the Astryx toast pauses.

(retention) => {
if (current) {
setScoped({ hostKey, state: { status: 'ready', retention } });
acknowledgeRetentionResults(services, host, retention);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3 (tests): This Settings-to-acknowledgement wiring is not covered by a component test. The notice tests call acknowledgeRetentionResults directly, and archive-retention-section.test.ts is unchanged. A small assertion that loading the section writes the seen state would lock this path in.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved at @Astro-Han's explicit request. Two independent automated review passes on this head found no P0–P2 in this PR's own changes (remaining items are P3 and are listed in the earlier comments), and CI is green. This PR is stacked on #5902 and should merge after it.

@liugddx
liugddx force-pushed the feat/retention-notices branch from bd5b626 to 8d05fd7 Compare October 8, 2026 04:33
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/XXL Over 2500 readable lines labels Oct 8, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review bd5b626 -> 8d05fd7: this is a pure rebase onto main 72afbf1. git range-diff shows all 3 commits as =, and the PR diff is byte-identical to the content previously approved at bd5b626. Nothing new was added; the earlier P3 notes still apply and are non-blocking. CI test is still pending on this head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants