Skip to content

fix(desktop): offer to restore an archived project when re-adding its directory - #4936

Merged
Astro-Han merged 4 commits into
apache:mainfrom
faga295:fix/create_archived_project
Oct 10, 2026
Merged

Astro-Han merged 4 commits into
apache:mainfrom
faga295:fix/create_archived_project

Conversation

@faga295

@faga295 faga295 commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Passed npm run lint and npm run format:check.
  • Passed npm run build and npm run typecheck.
  • Passed npx knip --workspace apps/desktop and npx knip --workspace packages/ui.
  • Passed Desktop build:test and 41 tests across task-entry controller, task-entry services adapter, project-add-restore, project-management-service, and app-shell-project-actions suites.
  • Regression assertions verify restore result forwarding, selection changing from A to B, no error report, and cleared pending state through the production Desktop adapter.
  • Full desktop end-to-end acceptance was not run; no screenshot or recording is attached.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 6, 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.

I reviewed this PR at head 9a27002c (17 files, +262/−21) against base 2310035a.

Gate is not ready — two blockers:

  1. Protocol epoch regression on merge. The head's packages/runtime-host/src/protocol/index.ts declares RUNTIME_HOST_COMPATIBILITY_EPOCH = 112, while the live base and current main are at 121 (nine versions newer). The branch forked from epoch-112-era main and 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 behind main. 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 current main (no epoch bump is needed afterwards, since the protocol is untouched) and push the new head for re-review.
  2. CI has not run on this head. Only the label check exists; the test workflow was never scheduled (first-contributor approval gate). The merge gate cannot close until test runs 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 existing projects:restore bridge and projectManagement.restore path.
  • toast.confirm is a pre-existing @maka/ui API, 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 generalizedErrorMessage for the summary; one recorded P3: the raw error object is also passed as the second console.error argument, 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.

@faga295
faga295 force-pushed the fix/create_archived_project branch from 9a27002 to 9669fcc Compare September 7, 2026 02:50
@faga295
faga295 requested a review from Astro-Han September 7, 2026 02:52

@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.

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.

Comment thread apps/desktop/src/renderer/app-shell-project-actions.ts Outdated
Comment thread apps/desktop/src/renderer/settings/projects-settings-page.tsx Outdated
@faga295
faga295 requested a review from Astro-Han September 8, 2026 05:19

@me2seeks me2seeks 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 (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 transition

Astryx 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.

  1. 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.
  2. Production code that can be deleted? The unreachable archived branch in app-shell-project-actions.ts:170-180.
  3. Low-quality tests to delete or replace? app-shell-project-actions.test.ts around 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.
  4. Deeper refactor required? A modest one: have every directory-registration path (including registerDirectory) return the same archived result, and centralize confirm→restore→select in one renderer helper. That removes the divergence that produced P1 and the P3s.
  5. 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.
  6. Residual risks / verification gaps: I did not run the app; the P1 conclusion rests on Astryx's own startTransition call 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.

@faga295

faga295 commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor Author

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 transition

Astryx 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.

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.
The provider deletion example differs: it calls toast.confirm() before any preceding await.
I also tested the original Settings handler: the restore dialog rendered, and confirming it reached projects.restore(). Switching to onClick with explicit loading state is reasonable, but this should not be classified as a confirmed P1 deadlock

@faga295
faga295 force-pushed the fix/create_archived_project branch from a1faaa0 to c3841f3 Compare September 10, 2026 20:23
@faga295
faga295 requested a review from me2seeks September 10, 2026 20:23
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 11, 2026

@me2seeks me2seeks 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.

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.)

@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.

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.

@faga295
faga295 force-pushed the fix/create_archived_project branch from 66b077b to 1cc7700 Compare September 23, 2026 04:42

@hqhq1025 hqhq1025 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.

[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 me2seeks 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 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

  1. [P1] test fails 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 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: 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.ts is modify/delete. Main deleted it in #5937, and main no longer has the dead addProject, so take main's side.
  • use-task-entry-controller.ts conflicts only by adjacency. Main's folder-open commands don't overlap the moved recovery functions.
  • settings-pages.stories.tsx has a content conflict.
  • docs/astryx-surface-file-inventory.md has 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.add and adds the two features/task-entry/index.js edges. Per the coordinating agent's local run, the strict-base check against 518fd529 passes. It must be regenerated again after the rebase.
  • me2seeks [P1] Settings confirm awaited inside an Astryx clickAction transition: resolved. The Add button now uses onClick plus ProjectRegistrationBoundary, and the confirm runs the restore through the new toast onConfirm hook. The dialog stays busy and can't be dismissed while the restore runs, and a failed restore rejects the confirm() promise.
  • Astro-Han [P2] AppShell does not select the restored project (withdrawn later for Settings): moot. The AppShell addProject path was removed as dead code.
  • Astro-Han P3, project-management-service.test.ts asserting cancelled: fixed. :163 now 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-2945 annotates projects.add as {ok:true…} | {ok:false; reason:'cancelled'}, while bridge-contract.d.ts declares the archived variant. 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 resolveProjectRegistration directly.

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):

  1. zh-TW mixes 封存/還原 with the 歸檔/恢復 used elsewhere for projects. This happens inside one dialog.
  2. 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.
  3. The archivedProject* keys added to settings-projects-copy.ts are 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.

Comment thread apps/desktop/src/renderer/remote-project-directory-dialog.tsx Outdated
Comment thread apps/desktop/src/renderer/locales/settings-projects-copy.ts Outdated
@github-actions github-actions Bot added effort/XL Under 2500 readable lines and removed effort/L Under 1000 readable lines labels Oct 7, 2026
@faga295
faga295 force-pushed the fix/create_archived_project branch from 67c0eb7 to 248246e Compare October 7, 2026 06:48

@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: 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), and 67c0eb7e already called useToast() 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. TaskEntryRoot is mounted inside Diagnostics.DiagnosticReportToastProvider in app-shell.tsx.
  • The fix belongs in the test fixture: wrap the probe's tree in ToastProvider, as task-entry-controller.test.ts already does. The new input.confirm injection cannot avoid the error, because useToast() 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 (dead addProject) and in its test are gone, which is right.
  • use-task-entry-controller.ts: main's folders service destructure is kept, and only sessions was dropped, as the PR intends. Main's folder-open commands are intact.
  • settings-pages.stories.tsx: main's new DiagnosticsServicesProvider wrapper is kept, with TaskEntryServicesProvider nested inside it.
  • docs/astryx-surface-file-inventory.{md,paths}: regenerated against @astryxdesign/core@0.6.3. The totals went from 329 to 330 (the new project-registration-boundary.tsx, aligned).
  • renderer-architecture.json: regenerated. The delta against main is now minimal, adding only the two features/task-entry/index.js edges (from remote-project-directory-dialog.tsx and settings/projects-settings-page.tsx). The dead app-shell-project-actions.ts entries are no longer a factor.
  • CI steps Check renderer architecture, Astryx surface inventory, Typecheck, Lint, Check formatting, Check locale hygiene and Guard the protocol compatibility epoch all 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.

  1. Restore naming. remote-project-directory-dialog.tsx:178 → acceptRegisteredProject (use-task-entry-controller.ts:431) still renames a restored archived project to the pending host.projectName. The local and Settings paths keep the original name.
  2. zh-TW terminology. session-workspace-recovery-copy.ts:53-64 still uses 封存/還原, while shell-copy.ts uses 歸檔/恢復 in the same dialog.
  3. Product terminology. session-workspace-recovery-copy.ts still says "session" / 会话 / 工作階段, where the shell says "task" / 任务 / 任務.
  4. Unused keys. The settings-projects-copy.ts:287-290 archivedProject* keys (and their values at 650-653, 993-996 and 1354-1357) are still never read. The controller and the boundary read getShellCopy(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 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: 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). The WorkBoardCompositionProbe tree is now wrapped in ToastProvider (workbar-controller.test.ts:318), the same way task-entry-controller.test.ts does it. CI has not yet confirmed this (see below).
  • P3 restore naming. The remote dialog now passes restored through onRegistered (remote-project-directory-dialog.tsx:178). acceptRegisteredProject skips the pending rename when restored is set (use-task-entry-controller.ts:433), so the remote path keeps the archived name like the local and Settings paths. resolveProjectRegistration tags a restored success with restored: true. Tests cover both outcomes.
  • P3 zh-TW wording. session-workspace-recovery-copy.ts now uses 歸檔/恢復, matching shell-copy.ts.
  • P3 session vs task. The recovery copy now says task / 任务 / 任務 in all three locales.
  • P3 unused keys. The archivedProject* keys are removed from settings-projects-copy.ts (type and all three locales). Nothing references them. The live keys are in shell-copy.ts.
  • P3 preload annotation. preload.ts:3026 now 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 main 72afbf11. #5902 added features/storage-usage/ui/archive-retention-section.tsx and changed the Totals line from 329 to 330. This PR made an identical 329 -> 330 edit for project-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 reports docs/astryx-surface-file-inventory.md does not match generator output.
  • This is not caused by c89fcd83; the previous head would hit it too against current main.
  • Fix: merge or rebase onto current main, then run npm 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.

Comment thread docs/astryx-surface-file-inventory.md Outdated
liuzhaochen03 added 2 commits October 8, 2026 14:48
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
liuzhaochen03 added 2 commits October 8, 2026 14:50
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
@faga295
faga295 force-pushed the fix/create_archived_project branch from c89fcd8 to 6e2f8c6 Compare October 8, 2026 06:53

@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: 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 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: the automated review of this exact head found no blocking (P0–P2) issues, and CI is green.

@Astro-Han
Astro-Han merged commit f2eb758 into apache:main Oct 10, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding a directory that belongs to an archived project fails with no way to restore it

4 participants