Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Review result: GO
I reviewed exact head 0f37edb566e5d9889e03a82fde31d4a0c98e120d against base 3924c6557e657059ae65812e15d8385ccbc57fb2. I found no independently reachable P0–P3 correctness, ownership, permission, lifecycle, or compatibility finding in this change.
The dialog target/controller now has one production owner in apps/desktop/src/renderer/features/session-collaboration/ui/session-collaboration-dialog-root.tsx:25-37; apps/desktop/src/renderer/app-shell.tsx:255-261,285-303 receives only the shell projection. use-session-collaboration-dialog.ts:29-48 keeps the opener/closer stable, while app-shell.tsx:1297,2314-2322 preserves modal coordination and the title-bar share action. The dialog is keyed by session identity at session-collaboration-dialog-root.tsx:34-36, so replacing the target clears the previous invitation/access state. Polling cleanup and late-result fencing are in session-collaboration-dialog.tsx:112-130; the feature tests cover target replacement, stale reads, StrictMode cleanup, invitation confirmation/copying, revocation, and turn-request decisions.
Bridge and clipboard access are behind the feature port (features/session-collaboration/ports.ts:38-105) and Desktop adapter (platform/desktop/create-session-collaboration-services.ts:23-69). Local remote access is checked before reading local sharing state and again before invitation creation (session-collaboration-dialog.tsx:72-79,140-150), and insecure invitations retain explicit confirmation (:151-159). The architecture ledger registers the single controller owner and the shell hook gate no longer permits the migrated hook. No schema or migration files changed.
Hosted required checks are green: test run 35440128474 / job 105889383579, and label run 35440128479 / job 105889384397. The merge tree and git diff --check are clean.
Limits: this worktree did not have a usable local tsc, so I did not claim an independent local typecheck or full local test run. I did not run a real Electron window, native clipboard interaction, screen-reader/visual smoke, cross-platform font/scaling smoke, or a live reconnect scenario. The hosted exact-head pipeline passed its build, typecheck, lint, architecture, and affected-suite stages.
Report: reports/pr5505-0f37edb5-review.md
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.
e3d8a9e to
ce5d83f
Compare
|
@Astro-Han Thanks for the earlier GO review. Could you please take another look at the rebased head The conflicts are resolved. The AppShell merge preserves main's current onboarding/bootstrap ownership while retaining the feature-owned sharing dialog and narrow shell projection. I regenerated the architecture ledger and Astryx inventory from the merged source; all six existing controller owners and the Session Collaboration owner remain registered. There were no inline findings to address in the earlier review. Local validation passed with Node 24.19.0 / npm 11.19.0: clean build, typecheck, lint, formatting, strict-base renderer architecture (112 fixture tests), Desktop tests (2,814/2,814), Astryx/Windows inventories, AppShell hook gate, ASF headers, Knip, |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-verification on rebased head ce5d83fcf: GO stands. The PR diff is byte-identical to the 0f37edb5 diff I approved, modulo context drift and the architecture ledger's own counters — the AppShell resolution keeps main's current onboarding/bootstrap ownership while the dialog moves under SessionCollaborationDialogRoot unchanged. One blocker though: the branch now conflicts with main on apps/desktop/renderer-architecture.json (the generated ledger, which every rebase here collides on). A rebase + ledger regen should make it mergeable; CI on this head is green (test run 35696541050).
ce5d83f to
625a5ad
Compare
|
@Astro-Han Could you please take another look at rebased head The new merge conflict is resolved. Only the generated renderer architecture ledger conflicted; I regenerated it and the Astryx inventory from the merged source. All existing controller owners and the Session Collaboration owner remain registered. The range-diff confirms that the PR's source changes are unchanged, with only the generated counts adjusted for current main. Merge-tree and Local validation passed with Node 24.19.0 / npm 11.19.0: clean full build, workspace typecheck, Desktop tests (2,824/2,824), strict-base renderer architecture (112/112 fixtures), lint, formatting, Desktop/UI Knip, ASF headers, Windows/Astryx inventories, and the AppShell hook gate. Native Electron sharing and clipboard acceptance were not rerun. Hosted CI must validate this new head. Automated update: posted by OpenAI Codex at the contributor's request. Independent human review is still required. |
Move the collaboration target and sharing surface into its feature owner, inject Desktop services, and keep only modal visibility and stable open commands in the shell. Cover target isolation, invitation confirmation, access mutations, and polling cleanup. Generated-by: Codex
625a5ad to
796daa5
Compare
Summary
The sharing dialog's target still lived in
AppShellContent, while its UI called the Desktop bridge directly from a legacy file.SessionCollaborationDialogRootnow owns the target and dialog insidefeatures/session-collaboration, with a registered controller owner and injected sharing/clipboard services. The shell receives only the stable opener, localized action label, and modal-open projection; target changes and dialog polling do not re-render the shell. AppShell hook call sites decrease from 61 to 60, and registered owners increase from six to seven.The existing invitation, access-management, Turn-approval, and remote-access Settings flows remain intact. Changing the sharing target now starts a fresh dialog so an old invitation cannot appear under another Session. Redundant shell props, the legacy UI file, and unused public exports are removed.
This completes the Session Collaboration dialog sub-slice only. Session settings / Plan intent is being developed separately; the full Session controls checklist remains open. Refs #4582.
Verification
upstream/mainpassed. Merge-tree against3924c6557is clean.AI use
Tool(s) and scope: OpenAI Codex implemented the migration, tests, documentation, and validation under the contributor's direction. The commit carries
Generated-by: Codex; independent human review is still required.Checklist
Does this PR entail a change in behavior?