Repository navigation
fix(desktop): offer to restore an archived project when re-adding its directory - #4936
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this PR at head 9a27002c (17 files, +262/−21) against base 2310035a.
Gate is not ready — two blockers:
- Protocol epoch regression on merge. The head's
packages/runtime-host/src/protocol/index.tsdeclaresRUNTIME_HOST_COMPATIBILITY_EPOCH = 112, while the live base and currentmainare at 121 (nine versions newer). The branch forked from epoch-112-eramainand has never been rebased: the merge-base (cbeb1a93c) is at 112, the head's first parent is that same old commit, and the branch is 62 commits behindmain. The PR makes no protocol change, so there is no epoch bump in the diff — but merging this head would regress the protocol epoch from 121 to 112. Please rebase onto the currentmain(no epoch bump is needed afterwards, since the protocol is untouched) and push the new head for re-review. - CI has not run on this head. Only the
labelcheck exists; thetestworkflow was never scheduled (first-contributor approval gate). The merge gate cannot close untiltestruns green on the exact head.
Code review (the feature itself):
- The Add-project flow now detects an archived project after catalog registration and returns
{ ok: false, reason: 'archived', projectId }instead of failing opaquely; all three entry points (app-shell project actions, task-entry workspace picker, projects settings page) present a confirm dialog and restore through the existingprojects:restorebridge andprojectManagement.restorepath. toast.confirmis a pre-existing@maka/uiAPI, and the type wiring is sound; copy covers en/zh-CN/zh-TW for the dialog keys.- Tests cover the archived detection, the confirm → restore → select flow, and restore-failure retry (pending state released).
- The added runtime-host error logging uses
generalizedErrorMessagefor the summary; one recorded P3: the rawerrorobject is also passed as the secondconsole.errorargument, which may still carry paths in the main-process log (not a user-facing surface, acceptable).
No P0–P2 code findings. This is a bugfix for #4935 and the direction is right, but the current head must not be merged: rebase onto current main and get CI approved/run first.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
9a27002 to
9669fcc
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for reusing the existing restore operation. Two normal entry paths still fail the intended restore-and-use contract: AppShell does not commit the selected project, and Settings waits for confirmation inside an Astryx async transition. Details inline. Please keep the repair at the existing selection and interaction boundaries rather than adding another restore coordinator.
中文
复用已有restore操作的方向正确,但AppShell没有提交项目选择,Settings则在Astryx异步transition内等待确认,两个入口都未完成预期操作。建议修在已有选择/交互接缝,不新增恢复协调器。
AI-assisted review at 9669fcce8. The coordinating Codex agent independently reproduced the Button/ToastProvider behavior in a browser through DOM-dispatched actions and checked the AppShell/Host selection calls; this was a component integration probe, not full-app acceptance.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review (Command Code) — not an approval
The root-cause fix is right: ProjectManagementService.add() now returns a structured reason: 'archived' instead of throwing, and the task-entry surface — the repro path in the issue — handles it correctly. The problem is that the response was copied into three surfaces with inconsistent quality: one cannot work, one is dead code, one is untouched.
P1 (Must-Fix) — the Settings restore dialog can never render, and the Add handler hangs.
The client-directory branch runs inside clickAction and awaits a confirm dialog:
// apps/desktop/src/renderer/settings/projects-settings-page.tsx:268-286
clickAction={capabilities.chooseHostDirectory
? () => { ... }
: async () => {
if (!props.runtimeHostTargetVerified) return;
const result = await window.maka.projects.add(host);
if (!result.ok && result.reason === 'archived') {
const ok = await toast.confirm({ ... }); // <- awaited inside a transitionAstryx runs clickAction inside startTransition:
// node_modules/@astryxdesign/core/dist/Button/Button.js:360, 409-411
const [isPending, startTransition] = useTransition();
...
startTransition(async () => { await clickAction(e); });React defers state commits made during an async transition until the action settles, and toast.confirm resolves only through state commits — setConfirmState({phase:'mounting'}), then an rAF effect to open, then closing (packages/ui/src/toast.tsx:250-292). So the dialog never mounts and the promise never settles.
This repository already documents exactly this hazard and the required workaround, in the same directory:
// apps/desktop/src/renderer/settings/provider-connection-detail.tsx:133-140
{/* onClick, not clickAction: this handler awaits `toast.confirm`, and
clickAction runs inside startTransition. React defers state commits
made during an async transition until the action settles, so the
confirm dialog — which is React state, and needs four commits to
resolve its promise — can never render, and the action waits forever
on a dialog that waits on the action. */}
Failure mode: on a client-directory host, Settings → Projects → Add project, choosing the directory of an archived project shows no dialog, restores nothing, and leaves the button stuck in isPending. This is newly introduced here — the base branch only did add + reload under clickAction — and the page's other confirm flow (remove) deliberately avoids clickAction for this reason. No pre-existing clickAction in the tree awaits a confirm.
Smallest sound fix: move this branch to onClick={() => void add()} with a local busy flag passed to isLoading (as provider-connection-detail.tsx:141 does), or route it through the page's existing runRowAction helper.
P2 (Should-Fix) — the app-shell archived branch is unreachable, and its new test validates dead code.
app-shell-project-actions.ts:158-180 adds the archived handling and returns addProject from its context value (:422), but the only consumer destructure omits it:
// apps/desktop/src/renderer/app-shell.tsx:1269-1286
projectInfo, projects, projectCapabilities, activeProjectCapabilities, localProjects,
currentProjectId, currentProject, projectPickerPendingRef, projectPickerRequestRef,
refreshProjects, relinkProject, renameProject, archiveProject, restoreProject,
openProjectFolder, openWorkspaceFolder, openSkillsFolder,
} = useAppShellProjectContext({ ... });
No addProject. The only addProject reference in the shell is the unrelated taskEntry.commands.addProject (app-shell.tsx:3069), which is a different provider. Outside tests, addProject has no caller other than the task-entry and settings paths. So the new branch plus its "Host selection truth and UI claims must agree" test exercise code nothing can reach, and the PR description's "all three entry points" is inaccurate — the app shell's Add is the task-entry picker. Delete the branch and its test, or actually wire the surface.
P3 (Nice-to-have) — a failed restore in Settings is silent, and a successful one does not select.
projects-settings-page.tsx:274-288 has no try/catch: if window.maka.projects.restore(...) rejects, the rejection is unhandled, no toast.error shows, and reload() never runs — unlike every other async action on the page. Also, add selects the newly registered project (project-management-service.ts:130-133) while the archived path only reloads, so Add behaves differently depending on which branch it took. Fixing P1 with onClick gives a natural place for both.
P3 (Nice-to-have) — the new coordinator logging leaks the error it claims to redact.
packages/runtime-host/src/server/project-catalog-coordinator.ts:147-150 and skill-catalog-coordinator.ts:270 call console.error(<generalizedErrorMessage(error)>, error). generalizedErrorMessage only classifies (packages/core/src/redaction.ts:292-294 → 'Operation failed'), so the first argument carries no detail while the second serializes the unredacted Error including its message and stack — typically absolute paths. This contradicts the stated intent of using generalizedErrorMessage to avoid leaking paths. reportUnexpectedOperation(scope, error) (redaction.ts:305-315) is the helper that actually redacts.
P3 (Nice-to-have) — the host-directory add path still bypasses the restore offer. For host-workspace hosts the Add control registers a directory (projects:registerDirectory), which reaches catalog.register(...) and returns the existing project with archivedAt intact; acceptRegisteredProject then selects it with no archived check (use-task-entry-controller.ts:367-383), while the picker filters archived projects out of its list. Result: no restore offer and an inconsistent selection. Traced by hand, not executed — flagging as unverified.
Checked and clean (so the fix stays narrow): no false-positive prompt — the reason is gated on the catalog's own archivedAt; declining persists no decision; re-entrancy is guarded per surface and restore/archive are idempotent writes, so two concurrent restores converge; and I found no "flag cleared but location not registered" path.
Review-relevant risks. No public contract, wire shape, dependency or licensing effect identified. The P3 logging item is a privacy-surface concern that warrants a maintainer's eye.
Required conclusion.
- Optimal for the actual problem? No. The root cause is fixed correctly, but the response is duplicated across three surfaces with divergent semantics — one broken, one dead, one untouched.
- Production code that can be deleted? The unreachable archived branch in
app-shell-project-actions.ts:170-180. - Low-quality tests to delete or replace?
app-shell-project-actions.test.tsaround line 175 validates dead code and gives false confidence that the app-shell surface is covered. Replace it with a test of the settings page. - Deeper refactor required? A modest one: have every directory-registration path (including
registerDirectory) return the samearchivedresult, and centralize confirm→restore→select in one renderer helper. That removes the divergence that produced P1 and the P3s. - Ready to merge? No — P1 breaks a user-facing surface this PR claims to fix, and the Settings page has no test coverage at all, which is why it slipped through.
- Residual risks / verification gaps: I did not run the app; the P1 conclusion rests on Astryx's own
startTransitioncall plus this repository's documented precedent for the identical pattern. This branch has no CI checks reported, so nothing mechanical has exercised it.
Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.
I don’t think this P1 is supported. In this handler, toast.confirm() runs after await window.maka.projects.add(host). React does not automatically mark state updates after an await as transition updates, so the claimed circular wait does not follow. |
a1faaa0 to
c3841f3
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Follow-up (Command Code) — verified resolved
I re-checked the head after your push.
Two of the three findings are fixed. The catalog failure logging now goes through reportUnexpectedOperation, so the raw error (message and stack, typically absolute paths) is no longer emitted next to a redacted classifier — that was my privacy note. And the unreachable app-shell addProject branch and its test have been removed, which was the dead-code item.
If the P1 (the Settings restore dialog awaited inside clickAction) landed in the same pass, everything I raised is closed; if it is still pending, that one is worth finishing, since it decides whether the Settings surface can restore at all.
Thanks. (Automated review; not an approval.)
c3e57b0 to
87d3cf9
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Verified against head 66b077b. The fix is correct on every supported add path and me2seeks' P1 is resolved structurally — the Settings surface now goes through the same registration.run/restore boundary as the remote dialog rather than awaiting a dialog inside clickAction.
Traced the invariants: match is by canonical project identity (folder:realpath / git:realpath through the same realpath+resolve pipeline the catalog already trusts — symlinks, trailing slashes, case-insensitive FS all consistent); declining is a clean cancel everywhere; duplicates are structurally impossible since the offer is the upsert's own identity match; restore preserves sessions/history by identity (clears only archivedAt); BEGIN IMMEDIATE write txn + per-key merge covers archive/rename races; double-confirm is idempotent. The 27-file size is honest plumbing — the variant type must traverse storage→IPC→preload→ports→controller→UI plus tri-locale copy.
P3s worth one pass, non-blocking:
- Restored project is silently renamed on the remote path but keeps its name on the local path — acceptRegisteredProject unconditionally applies the pending handoff name after the user confirms 'Restore it?' (use-task-entry-controller.ts:249-262), while the local path returns before options.name is applied. Same offer, different naming outcome; pick one semantics.
- project-management-service.test.ts:231 asserts {ok:false, reason:'cancelled'} for the archived match — the contract is 'archived'; the helper doesn't discriminate so it passes while documenting the wrong result.
- preload.ts:2929-2931 annotates projects.add without the 'archived' variant that bridge-contract.d.ts:1471-1475 now declares.
- ProjectRegistrationBoundary (render-prop + hard TaskEntryServicesProvider requirement inside ProjectsSettingsPage) is heavier than a useProjectRegistrationActions() hook would be — it does legitimately centralize confirm+restore for two consumers, so keep-or-simplify is your call, not a defect.
66b077b to
1cc7700
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
[P2] Please regenerate the renderer architecture ledger for the bridge/hook changes in this PR. The current-head CI test run fails its strict-base architecture check: app-shell-project-actions.ts removes projects.add, remote-project-directory-dialog.tsx adds projects.restore/useToast, and projects-settings-page.tsx adds projects.restore/useState, but apps/desktop/renderer-architecture.json still records the old counts. Build, typecheck, affected tests, and E2E are skipped after that failure. Update the ledger and rerun the full check on the new head.
The functional change gives archived project matches an explicit result and routes restoration through task entry, the remote directory dialog, and Settings. The added tests cover confirmation, cancellation, restore failure, and retry; I did not independently execute the suite or manually exercise Electron. No schema migration is involved. Earlier reviews and approval are attached to older commits, not this head.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
Offers to restore an archived project when re-adding its directory: the project-management service detects that a directory being added matches an archived project and surfaces a restore offer instead of creating a duplicate, threaded through the task-entry controller/services adapter, preload bridge, and locales, with a 323-line project-add-restore test covering the restore path. The issue (archived projects being re-added as fresh duplicates, losing their history) is real and the restore-at-add seam is the right place.
Findings
- [P1]
testfails on this head — the required merge gate is red. The failing lane is very likely the new project-add-restore or task-entry suite; pull the log, fix, and show green before merge.
Verdict
needs-changes — correct restore-at-add direction with a real test, but the red test gate must be fixed first.
Astro-Han
left a comment
There was a problem hiding this comment.
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: 1cc77002 (the last head reviewed by hqhq1025's agent) to 67c0eb7e. The merge-base with main is still 518fd529, so comparing old head vs merge-base against new head vs merge-base gives exactly one new commit: 67c0eb7e "share archived-project restore across registration callers" (+1330/-217, 20 files). The full PR is now 33 files, +1815/-210.
Merge state (not a code finding): needs a rebase, and no CI has run on this head. GitHub reports CONFLICTING/DIRTY, and git merge-tree against current main 3597abe8 finds 4 conflicts:
app-shell-project-actions.tsis modify/delete. Main deleted it in #5937, and main no longer has the deadaddProject, so take main's side.use-task-entry-controller.tsconflicts only by adjacency. Main's folder-open commands don't overlap the moved recovery functions.settings-pages.stories.tsxhas a content conflict.docs/astryx-surface-file-inventory.mdhas a content conflict.
No check runs exist on 67c0eb7e, so the required test gate has not run. Please rebase onto main and regenerate apps/desktop/renderer-architecture.json after the rebase, because main has rewritten that ledger since 518fd529. Then get test green on the new head.
Prior findings.
- hqhq1025 [P2] stale renderer architecture ledger: fixed at this head. The ledger drops
window.maka.projects.addand adds the twofeatures/task-entry/index.jsedges. Per the coordinating agent's local run, the strict-base check against518fd529passes. It must be regenerated again after the rebase. - me2seeks [P1] Settings confirm awaited inside an Astryx
clickActiontransition: resolved. The Add button now usesonClickplusProjectRegistrationBoundary, and the confirm runs the restore through the new toastonConfirmhook. The dialog stays busy and can't be dismissed while the restore runs, and a failed restore rejects theconfirm()promise. - Astro-Han [P2] AppShell does not select the restored project (withdrawn later for Settings): moot. The AppShell
addProjectpath was removed as dead code. - Astro-Han P3,
project-management-service.test.tsassertingcancelled: fixed.:163now asserts{ ok: false, reason: 'archived', projectId }. - Astro-Han P3, inconsistent naming on restore: still open. See the inline comment on
remote-project-directory-dialog.tsx:178. - Astro-Han P3, preload annotation: still open.
preload.ts:2944-2945annotatesprojects.addas{ok:true…} | {ok:false; reason:'cancelled'}, whilebridge-contract.d.tsdeclares thearchivedvariant. It typechecks only because the declared contract is wider. This line is not in the diff, so there is no inline comment. - Astro-Han P3, render-prop boundary weight: unchanged and still the author's call. It now has two consumers (Settings and the remote directory dialog), and the controller and recovery hook call
resolveProjectRegistrationdirectly.
Data safety (checked, no issue). An archived match is found through the catalog's own identity upsert (project-catalog.ts upsertResolvedProject). That upsert returns the existing record with its id and archivedAt intact. It only bumps lastUsedAt or appends a location, the same as before this PR. So re-adding the directory cannot create a duplicate.
Restoring goes through the existing projects:restore → catalog.restore, which only clears archivedAt. The project id is preserved, so its earlier sessions reattach. Project archive and restore never touch sessions.
Declining is a clean cancel on every path: no refresh, no selection change, and the action can be retried. The remote path's archivedAt === undefined check is valid because runtime-host-project-catalog.ts:113 maps the protocol's null to absent.
Session-workspace recovery restores first and relocates second. It distinguishes register, restore, and relocate failures, never retries automatically, and doesn't let a refresh failure roll back a restore.
Runtime-host protocol. The PR touches only project-catalog-coordinator.ts and skill-catalog-coordinator.ts, adding reportUnexpectedOperation logging calls. protocol/index.ts is untouched, so no epoch bump is needed. The merge-base is at epoch 177 and main is at 205, but a 3-way merge keeps 205.
i18n. All new keys exist in zh-CN, zh-TW, and en. There are three P3 wording issues (inline):
- zh-TW mixes 封存/還原 with the 歸檔/恢復 used elsewhere for projects. This happens inside one dialog.
- The new recovery copy calls the item a "session" (会话/工作階段), while the product calls it a "task" (任务/任務), including in the error title it is prefixed under.
- The
archivedProject*keys added tosettings-projects-copy.tsare never read. Settings goes through the boundary, which reads shell copy.
Tests. New and extended coverage in project-add-restore.test.ts (+600), task-entry-controller.test.ts, and task-entry-provider-scope.test.ts exercises confirm, cancel, restore failure, retry, invalidation (unmount, Host change, request replacement), single-flight, and toast busy/rejection behavior. Per the coordinating agent's local run at 67c0eb7e, these all pass: build, desktop + ui typecheck, 86/86 affected desktop tests, the strict-base renderer architecture check, locale hygiene, biome lint on changed files, and the surface inventory check. Hosted CI has not run.
Verdict: the feature logic is sound and the earlier P1 and P2 findings are resolved. It is blocked on the rebase and on green CI on the rebased head. The rest are P3s.
67c0eb7 to
248246e
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
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: 67c0eb7e (our last reviewed head) to 248246e5. This is a rebase. The merge-base moved from 518fd529 to current main 3597abe8, and the same three commits were replayed (f4c09889, 7ad178c8, 248246e5). git range-diff shows only conflict resolution and context changes. There is no new feature logic. The PR is now 31 files, +1814/-165.
Blocking (P2): CI test fails on this head. One test fails: workbar-controller.test.ts, "drops a claim when Task Entry changes project within the same surface". The error is useToast must be used inside a <ToastProvider>, thrown from useTaskEntryController via WorkBoardCompositionProbe.
- This is not a rebase regression. The probe already existed at the old base (
518fd529), and67c0eb7ealready calleduseToast()in the controller. CI simply never ran on the previous head, and the earlier local run covered only the 86 directly affected tests. - Production is fine.
TaskEntryRootis mounted insideDiagnostics.DiagnosticReportToastProviderinapp-shell.tsx. - The fix belongs in the test fixture: wrap the probe's tree in
ToastProvider, astask-entry-controller.test.tsalready does. The newinput.confirminjection cannot avoid the error, becauseuseToast()is called unconditionally. See the inline comment.
Rebase and conflict resolution: correct.
app-shell-project-actions.ts(modify/delete): main's deletion was taken. The PR's earlier hunks there (deadaddProject) and in its test are gone, which is right.use-task-entry-controller.ts: main'sfoldersservice destructure is kept, and onlysessionswas dropped, as the PR intends. Main's folder-open commands are intact.settings-pages.stories.tsx: main's newDiagnosticsServicesProviderwrapper is kept, withTaskEntryServicesProvidernested inside it.docs/astryx-surface-file-inventory.{md,paths}: regenerated against@astryxdesign/core@0.6.3. The totals went from 329 to 330 (the newproject-registration-boundary.tsx, aligned).renderer-architecture.json: regenerated. The delta againstmainis now minimal, adding only the twofeatures/task-entry/index.jsedges (fromremote-project-directory-dialog.tsxandsettings/projects-settings-page.tsx). The deadapp-shell-project-actions.tsentries are no longer a factor.- CI steps
Check renderer architecture,Astryx surface inventory,Typecheck,Lint,Check formatting,Check locale hygieneandGuard the protocol compatibility epochall pass on this head.
Callers after the rebase. No new projects.add caller arrived from main. Every consumer of the archived result goes through resolveProjectRegistration or ProjectRegistrationBoundary:
- the task-entry controller (
:353) - session-workspace recovery (
:151) - Settings (
projects-settings-page.tsx:119) - the remote directory dialog
The PR doesn't touch the runtime-host protocol (only reportUnexpectedOperation logging in two coordinators), so no epoch claim is needed.
Prior P3s: all four still open, with no reply yet. They are not repeated inline because the earlier inline comments still apply at the same locations.
- Restore naming.
remote-project-directory-dialog.tsx:178→acceptRegisteredProject(use-task-entry-controller.ts:431) still renames a restored archived project to the pendinghost.projectName. The local and Settings paths keep the original name. - zh-TW terminology.
session-workspace-recovery-copy.ts:53-64still uses 封存/還原, whileshell-copy.tsuses 歸檔/恢復 in the same dialog. - Product terminology.
session-workspace-recovery-copy.tsstill says "session" / 会话 / 工作階段, where the shell says "task" / 任务 / 任務. - Unused keys. The
settings-projects-copy.ts:287-290archivedProject*keys (and their values at 650-653, 993-996 and 1354-1357) are still never read. The controller and the boundary readgetShellCopy(locale).projectActions.
There is one more P3, which is not in the diff and so has no inline comment. preload.ts:3026 still annotates projects.add as ... | { ok: false; reason: 'cancelled' }, without the archived variant that bridge-contract.d.ts and the :1984-1985 invoke declare.
Status: GitHub reports MERGEABLE, with merge state BLOCKED. git merge-tree against main 3597abe8 is clean. CI test FAILS (run 37583604029), on the one test above.
Verdict: the rebase is done correctly and the feature logic is unchanged from the version we reviewed as sound. The PR is blocked only on the red test gate, which needs a one-line fixture fix. The P3s remain the author's call. This review is not merge approval.
Astro-Han
left a comment
There was a problem hiding this comment.
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: 248246e5 (our last reviewed head) to c89fcd83. The base is unchanged (3597abe8). One new commit, "preserve archived project names during remote registration". The PR is now 33 files, +1884/-176.
All prior findings are addressed in code:
- P2 (CI,
workbar-controller.test.ts). TheWorkBoardCompositionProbetree is now wrapped inToastProvider(workbar-controller.test.ts:318), the same waytask-entry-controller.test.tsdoes it. CI has not yet confirmed this (see below). - P3 restore naming. The remote dialog now passes
restoredthroughonRegistered(remote-project-directory-dialog.tsx:178).acceptRegisteredProjectskips the pending rename whenrestoredis set (use-task-entry-controller.ts:433), so the remote path keeps the archived name like the local and Settings paths.resolveProjectRegistrationtags a restored success withrestored: true. Tests cover both outcomes. - P3 zh-TW wording.
session-workspace-recovery-copy.tsnow uses 歸檔/恢復, matchingshell-copy.ts. - P3 session vs task. The recovery copy now says task / 任务 / 任務 in all three locales.
- P3 unused keys. The
archivedProject*keys are removed fromsettings-projects-copy.ts(type and all three locales). Nothing references them. The live keys are inshell-copy.ts. - P3 preload annotation.
preload.ts:3026now includes the{ ok: false; reason: 'archived'; projectId }variant.
The refactor that extracts useProjectRegistration from ProjectRegistrationBoundary moves the code unchanged: same fences, single-flight, and unmount handling. The boundary is now a thin adapter. The app-shell hook gate and the renderer architecture check pass.
New blocking issue (P2): CI test fails on Astryx surface inventory, from a semantic merge with #5902.
- CI runs on the merge with
main72afbf11. #5902 addedfeatures/storage-usage/ui/archive-retention-section.tsxand changed the Totals line from 329 to 330. This PR made an identical 329 -> 330 edit forproject-registration-boundary.tsx. - Because the edits are identical, git merges them silently. The merged doc lists 331 file rows but still says
330 files ... aligned 326, so the check reportsdocs/astryx-surface-file-inventory.md does not match generator output. - This is not caused by
c89fcd83; the previous head would hit it too against currentmain. - Fix: merge or rebase onto current
main, then runnpm run astryx:surface-inventory:write(expect 331 / aligned 327). - The job stopped at that step, so Build, Typecheck and the unit tests (including the workbar fix and the removed copy keys) did not run on this head. Please confirm they pass once the inventory is regenerated.
Status: MERGEABLE, with merge state BLOCKED. git merge-tree against main is clean textually. CI test FAILS (run 37725085215), at the inventory step.
Verdict: the feature work and all prior findings are resolved. The PR is blocked only on the inventory regeneration after syncing with main, plus a green run of the remaining steps. This review is not merge approval.
Offer to restore archived projects from task entry, project settings, and remote directory registration. Preserve project naming, show loading state, and report add, restore, and refresh failures. Remove the unused app-shell add action and route runtime-host catalog failures through redacted diagnostics. Add regression coverage for archived-project restoration and error handling. Generated-by: Codex
Reuse the result-bearing newTasks restore bridge and remove conflicting service and fake definitions. Cover restored-project selection through the Desktop adapter. Generated-by: OpenAI Codex
Extract the register/confirm/restore transaction into resolveProjectRegistration and a render-prop ProjectRegistrationBoundary owning single-flight pending state and staleness fences. Settings and the remote directory picker route through it, so the confirm dialog no longer awaits inside Astryx clickAction transitions. Session workspace recovery moves to useSessionWorkspaceRecovery with request-identity and attempt fencing, and toast confirm gains an onConfirm hook that keeps the dialog open while the accepted operation runs. Generated-by: Codex
Extract shared project registration recovery into a hook and carry the restored result through the remote directory handoff. Align workspace recovery copy with task terminology and expose archived add results in the preload bridge. Generated-by: Codex
c89fcd8 to
6e2f8c6
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
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: c89fcd83 to 6e2f8c6a. The PR is rebased onto main f10a74d8, which includes #5902 and #5961. A range-diff shows the only content change is the Astryx inventory Totals line. There is no new feature code.
Prior P2 (Astryx surface inventory): fixed. main now has 331 files (aligned 327), and this PR adds project-registration-boundary.tsx, so the correct total is 332. I checked this by applying the generator's tree, extension and exclusion rules to git ls-tree for both the head and its merge with main; both give 332 files. .paths matches that set exactly, and the .md lists 328 aligned and 4 polish rows. docs/astryx-surface-file-inventory.md:9 now reads 332 files ... aligned 328, and the CI inventory step passes.
Other prior findings: all remain fixed. The workbar-controller ToastProvider fix is now confirmed in CI: the unit suites ran and reported 0 failures.
The runtime-host changes only add reportUnexpectedOperation diagnostics, with no wire change, so no protocol epoch bump is needed.
Status: CI test passes (run 37740189306). The PR is MERGEABLE and its merge state is CLEAN. I have no open findings. This review is not merge approval.
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: the automated review of this exact head found no blocking (P0–P2) issues, and CI is green.
Summary
Fixes #4935
Re-adding a directory belonging to an archived project now offers to restore it. The task-entry picker selects the restored project, Settings refreshes its project list, and the remote directory picker restores before accepting the project. Cancel and failure paths release pending state.
The Desktop task-entry adapter preserves the restore result from the preload bridge. Conflicting restore declarations and duplicate test defaults are removed, and the regression exercises the real adapter while switching selection from project A to restored project B. Unused AppShell add logic is removed, and catalog failures use redacted diagnostic logging.
Verification
npm run lintandnpm run format:check.npm run buildandnpm run typecheck.npx knip --workspace apps/desktopandnpx knip --workspace packages/ui.build:testand 41 tests across task-entry controller, task-entry services adapter, project-add-restore, project-management-service, and app-shell-project-actions suites.AI use
Tool(s) and scope: OpenAI Codex reviewed and fixed the restore adapter/result contract, updated regression tests, ran verification, and updated this description. The follow-up commit includes
Generated-by: OpenAI Codex. This description was updated by an automated agent; it is not an independent human review.Checklist
Does this PR entail a change in behavior?