Add channel-backed Sessions for agent conversations - #77
Conversation
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934bda830d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested. Reviewed head 934bda830dda048947f7d393545f60e15a95929f against merge-base f0c6ac892fa3b5296e983ec7b02999dada9c9b6f.
- P1: unintended agent admission. I independently reproduced the existing recipient-precedence finding, rather than duplicating its inline comment. With picker agent B and explicit mention A, the mounted composer sends only to A but admits B too. Child creation adds B to both parent and session; standalone creation invites B separately. This grants private-history access outside the effective recipient choice. Resolve the effective recipient set once and use it for admission and sending, including retries.
- P2: older session message links time out. New inline finding below: removing the thread fallback leaves no target lookup for flat-session messages outside the loaded head.
Validation: two mounted recipient cases reproduce P1 (child and standalone). Production-frontend browser fixtures reproduce P2 in Chromium and WebKit: ordinary-channel controls open the target; equivalent session cases time out after 15 seconds without an exact-target read. Product files remained unchanged. Three independent review lanes completed; an unverified generic-relay paginator claim was retracted and is not a finding.
Separate CI note: both failed browser shards stop at the stale expected page list in tests/browser/layout.spec.mjs:646, which omits Sessions. Update that expectation; those failures are not the navigation reproduction. Hosted DCO Check is green; commit metadata is not a code blocker in this review. Native GUI and live-relay behavior were not exercised by me.
Exit criteria: admission matches mention precedence in new child/standalone sessions and retry paths; valid older session targets are fetched and revealed without manual paging; update the stale navigation-page expectation.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b6c730a5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 219dbb856c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested. Reviewed head 219dbb856c0b428455c987a45aba24f95f5671c3 against exact base a9194b24a3cc208abf6f9a9a05eed77583e6c0fa (merge-base f0c6ac892fa3b5296e983ec7b02999dada9c9b6f).
The prior unintended-picker admission, stale-library admission and unloaded-message navigation repairs are present, and the stale page-list CI failure is resolved. The remaining findings below independently confirm existing inline reports rather than duplicating them.
-
P2: Internal Buzz links cannot open from the standalone Sessions page.
src/bundled/sessions/SessionsPage.tsx:164,185-193installs anonOpenLinkthat always returns false.MessageLink.tsx:82-111prevents default navigation forbuzz:links and relies on that callback, so clicking a valid channel/message link in a saved session always reports “This Buzz link couldn’t be opened here.” The same conversation opened through Messages has the working shared navigation dispatcher. Inject and use that existing dispatcher in Sessions, and cover an actual internal-link click from this page. Existing finding. -
P2: Session replies have no unread indication in either session list.
src/bundled/sessions/SessionsWorkspace.tsx:37-51and child rows insrc/bundled/channels/ChannelSidebarRow.tsx:149-164render only titles. The only badge supplied byChannelsPage.tsx:600-602targets the parent channel; the shared unread selector counts the session under its own channel ID (src/features/relay/unread.ts:246-301). Therefore an observed agent reply to an unselected session never marks its history/nested row, and the parent cannot substitute for it. Reuse the existing session-owned per-channel selectors in both lists; retain observed/unknown semantics and exercise an unselected session reply. This restores reply discovery, not a new notification service. Existing finding. -
P2: The existing-session picker suppresses history-access disclosure for newly admitted agents.
src/features/sessions/SessionAgentControl.tsx:51supplies noallowedroster for standalone sessions and only the parent roster for children.AgentChoice.tsx:109-147therefore hides its existing admission/history warning for every standalone nonmember and for an agent in the parent but absent from the child. Selecting that agent and sending still runsMessageComposer.tsx:330-385→work-sessions.ts:186-260, granting access to the session's prior private history. Compare session membership separately and disclose that access before sending whenever the selected agent is absent; retain the parent warning only when parent admission is also needed. An existing session member must not trigger a new-admission warning merely because it is absent from the parent. This is a disclosure defect, not an unauthorized invitation or a request to change independent memberships. Existing finding.
Validation and scope
Existing CI run 35113668481 succeeded at this exact head, including native checks and Chromium/WebKit browser shards. This review is source/metadata-only on the authorized laptop: no checkout, code execution, local test rerun, CI rerun, native GUI or live-relay acceptance.
Independent parent/session membership remains the accepted product behavior. No inheritance redesign, new relay protocol, or new unread owner is requested. GitHub currently also reports a merge conflict against the pinned base; that integration gate is separate from these three code findings. The old missing-DCO report is contradicted by the commit trailers and successful hosted DCO Check. The loose recovered-UUID validator has no identified malformed value producer in normal operation, so it is not a release blocker here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d544ef3fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 remains in the unread repair. Reviewed exact head 3d544ef3fc00d4b68c6705f2380e4a0d4d2528a9 against exact base/merge-base c5cf3b71271d2f0e5102a645036937d98120cc97, including the repairs since 219dbb856c0b428455c987a45aba24f95f5671c3.
The previous internal Buzz-link dispatch and session/parent history-access disclosure findings are resolved. Independent parent/session membership remains the accepted contract.
P2: Ordinary unread sessions still have no visible row indication
The new badge wiring in SessionsWorkspace.tsx:51-53 and ChannelSidebarRow.tsx:169-171 does not supply a visible ordinary-unread state.
Source-derived reproduction: an unselected session receives a non-broadcast message without a viewer mention or participating-thread reply. Once observed, its selector can report observedCount > 0, attentionCount = 0, manual = "none", and no thread activity. UnreadBadge.tsx:41-74 then renders only unreadState, which Channels.module.css:381-390 visually clips. Both session title spans lack an unread-dependent style, so the visible row looks read in both lists. Priority/mention and thread-activity dots still work; this is not a claim that every agent reply is invisible. The accessibility marker also remains present.
Minimal exit criteria: give ordinary observed unread a visible session-title treatment in both lists, preserving quiet unknown/zero states and selected-row styling. Exercise the real UnreadBadge ordinary state, not the arbitrary visible <span>Unread</span> used by the new list tests. No new unread owner, store, or notification service is needed. Merely restoring channelLabel is insufficient: its current weight/color equal the inherited button baseline. I am not attributing that pre-existing top-level Channels styling issue to this PR.
Validation and scope
Source-only review and a fresh independent challenge completed on the pinned Blox host with managed Git routing preserved. The challenge narrowed the unread finding and rejected the broader top-level regression claim. Existing exact-head hosted CI reports 12/12 successful checks, including Chromium/WebKit journeys, native checks, and both required gates. No checkout, build, test, PR-code execution, CI rerun, or live/native UI acceptance was performed for this review. The unrelated synthetic-squash DCO comment is not a code blocker; actual-commit metadata and hosted DCO evidence do not support it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 743dee0675
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: the remaining ordinary-unread blocker is resolved. No new actionable blockers found.
Reviewed head 743dee06759e1a98f5c432f68d5c6546e82090a3 against base c5cf3b71271d2f0e5102a645036937d98120cc97. This is a convergent re-review of the nine-file repair since 3d544ef3fc00d4b68c6705f2380e4a0d4d2528a9, plus independent verification of the two new bot claims. It carries forward the previous review’s resolved link-dispatch/history-access findings and accepted independent parent/session membership contract; it is not a fresh audit of every unchanged feature path.
Previous blocker: resolved
UnreadBadge.tsx:49–64 now exposes ordinary observed unread through the session title. Both Sessions history and nested Channels session rows render that title with unselected-only unread styling. Unknown and observed-zero states stay quiet, selected styling remains authoritative, and ordinary unread does not gain a priority dot. Both component-test files now exercise the real badge with a stable unread capability double instead of an arbitrary visible stand-in. This meets the previous review’s exit criteria without adding another state owner.
New bot claims: not blockers under the current contract/runtime
- Sole-parent-agent default: the empty-picker path does not inherit the parent’s agent. Recipient fallback examines the destination session’s own roster; the zero-agent case explicitly returns no recipients (
recipients.test.ts:29–37). Independent membership is the accepted contract (docs/sessions/README.md:16–18). Automatically importing a parent agent is not a required repair. Optional documentation clarification: “a sole agent already in the session” would remove ambiguity at README line 28. - Sessions-first historical unread: opening Sessions before Messages is reachable, and the global initializer does skip
unsupportedread-state capability. However, the in-tree descriptor producer advertisesreadState: true(dev/relay-broker.mjs:594); the app-global initializer is bound independently of the mounted page (src/app/services.ts:43,src/features/notifications/messages.ts:156–165). The unsupported-host precondition is not established for the current in-tree runtime. This conclusion does not cover external/future adapters that omitreadState; that latent coupling should be addressed at the global initializer if such a host is supported, not by adding a Sessions-specific owner.
Validation and limits
Source-only review on the pinned Blox host, with an independent challenge and managed Git routing preserved. Existing exact-head CI snapshot: 12/12 checks successful, including Chromium/WebKit journeys, Rust/tool integration, Windows native notifications, and required gates. CI metadata does not establish the specific visual or membership assertions by itself. No checkout, installation, build, test execution, CI rerun, or live/native UI acceptance was performed. CSS/DOM behavior here is a source assessment, not a computed-style browser observation. Cosmetic/documentation follow-ups do not block this repair.
This is a clear COMMENTED review, not a GitHub approval or a change to merge policy.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 shared-API boundary issue from the additional architecture review requested by Wes. Reviewed head 743dee06759e1a98f5c432f68d5c6546e82090a3 against base c5cf3b71271d2f0e5102a645036937d98120cc97.
P2: Keep Sessions default-recipient selection out of generic send/reply
session.ts:686–695 unconditionally supplies sessionRecipients to the shared messaging capability. messages.ts:27–35 applies it even when a caller explicitly supplies an empty recipient list.
For a private stream classified as a Session, another editor calling messages.send(channelId, text, []) can therefore emit a p tag for the sole known agent, throw while participant identities are incomplete, or reject because multiple agents exist. reply has the same behavior. Zero known agents correctly returns no recipients. Ordinary non-session channels are unaffected.
The automatic-agent behavior is appropriate for the agreed Sessions UX, but making it implicit for every shared-API caller is broader than necessary. The existing shared contract describes exact caller-provided recipients (docs/agents.md:85–96), independent editors may use that session directly (docs/plugin-architecture.md:361–364), and Sessions composition/recipient selection has its own owner (docs/sessions/README.md:40–43). The new mentions.test.ts:231–264 intentionally asserts global default routing; its passing does not settle that ownership decision.
Minimal exit criteria: resolve the Sessions default in its composition/caller layer and pass explicit recipients to shared send/reply. Preserve shared membership validation, pre-sign/pre-publish checks, signing, durable retries, and the existing Sessions UX. Cover an independent caller passing [] as well as Sessions automatic selection. Alternatively, if Wes explicitly intends automatic routing for every shared send/reply caller targeting a session channel, document that as an owner-approved shared-contract change rather than assuming it from the UI requirement.
Non-blocking ownership recommendation
MessageComposer.tsx:342–364,390–394,633–641 directly owns Sessions admission and chooser behavior. Prefer a Sessions-owned wrapper using ordinary props; this PR already adds chooser/submission seams. This is not an independent blocking defect and does not require a new registry or framework.
What should stay shared
Necessary FOUNDATION changes are welcome. Host signing and its narrow allowlist, verified membership, durable create/invite recovery, and bounded flat-history support have legitimate shared owners. Plugin unload must not discard durable writes. No broad channel-management API, lifecycle redesign, or change to accepted independent parent/session membership is requested.
This does not reopen the resolved unread, link-navigation, or history-disclosure findings. My earlier same-head clear review covered the convergent unread repair, not this subsequently requested architecture assessment. Three complementary source-review lanes informed this review; key findings were independently traced. Existing exact-head CI is 12/12 successful. No local tests, live notification reproduction, or native acceptance were performed for this assessment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3945646d28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: two P2 regressions. The shared send/reply recipient-boundary repair is correct.
Reviewed head 3945646d28e20858a006dc82cf63ca0e17955048 against base c5cf3b71271d2f0e5102a645036937d98120cc97, concentrating on the incremental repair since 743dee06759e1a98f5c432f68d5c6546e82090a3 and its affected callers. Independent parent/session membership remains accepted; this does not reopen resolved unread, link, or disclosure findings.
P2: Preserve typing layout when the indicator becomes idle
TypingIndicator.tsx:23–40 now returns null when there are no matching entries. Previously the .typing wrapper stayed mounted and only its status text was conditional. That wrapper reserves 1lh; its flex-column parent also adds a gap. Removing it makes the shared composer grow and the history shrink whenever typing starts, then reverse on completion/expiry.
This is reproduced by existing exact-head Chromium CI: both unchanged tests/browser/typing.spec.mjs:62 cases fail the stable-history-bounds assertion at line 117 immediately after typing begins. Channel history changes 700 → 676 px; thread history changes 714 → 690 px. The 24 px delta matches the removed 18 px line plus 6 px gap. The job reports 2 failed, 101 passed, not an infrastructure failure.
Minimal repair: retain the reserved wrapper and conditionally render only the status text, or preserve equivalent stable geometry if retaining the tighter layout. Keep the existing channel/thread completion-and-expiry assertions; do not weaken them to accommodate the jump.
P2: Resolve recipients in recovered new-session submission too
NewSessionComposer.tsx:173–191 still loads participant profiles/library data but passes the original constant recipient list to messages.send. With inference correctly removed from the generic writer, an empty draft selection remains [] even if the destination now contains a sole agent.
The concrete source-reachable case is recovery: start without a picker/mention, interrupt after durable session creation but before the first message, admit one agent to that created session from another client, then retry the saved prompt. creationId and draft survive (:114–140); refresh reads the existing session; the preparation block sees its roster; nevertheless the first message has no agent p tag. That agent is not notified, contrary to the documented sole-agent-already-in-session rule. Fresh sessions with no selected or admitted agent are correct. This is not a request to inherit an agent from the parent.
Minimal repair: apply the existing sessionRecipients policy in this Sessions-owned caller after participant preparation and pass the resolved list to the generic writer. Preserve explicit mention-over-picker precedence, zero-agent [], multiple/unknown rejection, and stored-event retry semantics. Add a recovered-empty-draft test with channelType: "session" and an intervening sole-agent admission that asserts the actual recipient argument. The current profile-wait test only checks that send was called. This recovery defect is source-derived, not a live reproduction.
Boundary result, scope and validation
The generic writer is back to its base behavior: explicit/omitted empty recipients stay empty, while the ordinary saved-session composer resolves before both send and reply. Membership validation, signing/publication checks, exact signed-event retry and independent memberships remain intact. Keep this ownership split; do not restore relay-layer inference to fix the recovery path.
Independent review also identified that the media-review composer does not opt into Sessions automatic routing (MediaReviewViewer.tsx:286–297). That surface now requires an explicit mention. Its inclusion in the Sessions UX contract is not established clearly enough to add another blocker here; treat extending automatic selection there as an explicit product decision, not a reason to make all generic callers infer recipients again. Missing caller-level multi/unknown error tests are coverage follow-up, not a separate demonstrated defect.
Source-only review on the pinned Blox host, with independent source challenge and integration verification. No checkout, installation, build, test execution, CI rerun/monitoring, or live/native acceptance was performed. The 14:48 UTC CI snapshot had the Chromium failure above and four pending gates; it is not a current all-checks verdict. Exit criteria are the two bounded repairs above with owning regression coverage, preserving the repaired generic boundary and accepted product decisions.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
3945646 to
ee0432e
Compare
|
🤖 Addressed both P2 findings in b95173d, now published at ee0432e after rebasing onto main (f6b331c):
The empty-composer caret fix is also included. Main's shared icon migration is integrated. Validation at the published head: 1,154 selected unit tests, 12 focused Chromium/WebKit cases, 42 design tests, app/design builds, type checks, and design guards passed. All 14 PR commits have sign-offs; hosted DCO is successful. Hosted CI is running; the PR has no merge conflicts. The two separate bot findings about collapsed sidebar sessions remain open. — Codex, Kenneth's AI agent |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee0432ec0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 search defect. Both previous blockers are repaired in source.
Re-reviewed head ee0432ec0c022c5881de47c5e04c0d4321087a72 against base f6b331cd19a9832abefb70f5631d4196dbbcf497, integrating all three independent source-review lanes. This review is limited to the previous repairs, their affected contracts, and the two new sidebar bot findings; accepted independent session membership and generic-recipient boundaries remain accepted.
P2: Reveal a matching child session despite saved collapse
At ChannelsPage.tsx:748–750, child visibility still uses persisted collapse during search. The filter at lines 664–675 includes a parent when only its child matches, and lines 756–762 retain that matching child. But ChannelSidebarRow.tsx:146 keeps the result inside hidden={collapsed}.
Reproduction from source: collapse a channel’s sessions, then search for text found only in one child session name. The visible result is the nonmatching parent; the actual matching session is hidden and cannot be selected without manually expanding the unrelated-looking parent. This independently confirms bot finding r4063285599. Reviewer correction: this search/filter/collapse path was already present unchanged at my previous reviewed head 3945646d28e20858a006dc82cf63ca0e17955048. I missed it in that review; it is not a regression introduced by the author’s latest repairs. Both previously requested repairs are cleared. I should have consolidated this earlier rather than adding another repair round.
Bounded exit criteria: search must expose matching child rows even when their group was previously collapsed, without changing the saved collapse preference. Add a regression covering a child-only match after collapse and restoration after clearing search. An enclosing collapsed sidebar section must not hide the result either; avoid persisting temporary search expansion through its onToggle. No new search or unread architecture is needed.
Previous repairs and other bot finding
- The recovered-session caller now applies
sessionRecipientsafter refreshed roster/profile preparation (NewSessionComposer.tsx:174–203). Explicit mention-over-picker precedence and zero/one/multiple/unknown handling remain owned by Sessions. An existingmessageIdskips recomputation and retries the existing signed event. Shared send/reply still consumes only the supplied recipient list. - The typing wrapper’s zero height and negative margin cancel the composer flex gap, retaining compact idle geometry. Existing channel/thread start/completion/expiry geometry assertions remain unchanged. The empty-editor caret marker is rendering-only and excluded from source offsets; new browser assertions cover send/refocus/delete. These source repairs close the previous two findings; runtime confirmation is deferred.
- Bot finding
r4063285593(roll child unread up onto a collapsed parent) is non-blocking. The parent badge represents that parent; aggregate collapsed-group unread was not an established contract, existing collapsed top-level sections have a similar visibility limitation, and flat Sessions rows retain access. Do not add an unread aggregation subsystem to satisfy this review. Long typing labels losing ellipsis is cosmetic follow-up, not another exit criterion.
Validation: source-only exact-object inspection and independent challenge, not a browser reproduction or runtime pass. No PR checkout, installation, build, tests, Blox use, or CI monitoring/reruns in this re-review. I am not certifying current CI or native behavior. Resolve the bounded search defect above while preserving the cleared repairs and existing assertions.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
|
🤖 Addressed the bounded search finding in dded347. Matching child sessions now remain visible through both collapsed sidebar levels, and clearing search restores the saved layout. Explicit summary activation owns persistence, avoiding queued native toggle events saving temporary search expansion. Pointer, Enter/Space, navigation/remount, and clear-search behavior are covered. Also passed the active viewer into the Sessions timeline for the new membership-wording finding. Closed the aggregate unread bot thread as non-blocking per your ruling; no unread architecture was added. Validation on a clean checkout of dded347: 20 Chromium/WebKit cases, 1,154 related unit tests, TypeScript, and design checks passed. All 15 PR commits carry DCO trailers. The original worktree’s separate, uncommitted chat-layout edits remain untouched; the first push attempt there exposed their media-viewer avatar-test mismatch, so final validation and push used the clean commit. Hosted CI for the new head is pending; this is not an approval or merge-readiness claim. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dded347f08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| settings: ReadOptions, | ||
| ) { | ||
| if (!transport) throw new Error("Relay is unavailable"); | ||
| if (isSession(channelId)) { |
There was a problem hiding this comment.
When Messages opens a saved or deep-linked session during initial discovery, applyDiscovery() publishes the roster before fetching metadata, so this check is initially false and the window is loaded with the ordinary top_level filter. The later metadata pass changes isSession(channelId) without evicting or refetching the existing window, and useChannelWindow does not rerun while the channel ID is unchanged, leaving historical thread replies absent until a manual refresh. Invalidate or reload windows when their session classification changes.
Useful? React with 👍 / 👎.
| channelId={channel.id} | ||
| window={window} | ||
| revealMessageId={sent} | ||
| onOpenLink={openLink} |
There was a problem hiding this comment.
Although onOpenLink is now wired, the exact-head call still omits canOpenLink; MessageRow gates author-avatar and rendered profile-mention controls on that predicate. Consequently, when the standalone Sessions page has a navigator, profile targets in the latest timeline remain noninteractive even though the composer and exact-message view on the same page support them. Pass canOpenLink to ChannelTimeline here.
Useful? React with 👍 / 👎.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Approval-ready: the outstanding search blocker is resolved. No material code blockers found in this bounded re-review. This is a comment, not an approval or permission to merge.
Reviewed head dded347f08400ed6ff1468f9d386de1d5371ab61 against current base f6b331cd19a9832abefb70f5631d4196dbbcf497, concentrating on the delta from ee0432ec0c022c5881de47c5e04c0d4321087a72 and the previous review’s exit criteria. Integrated independent browser/accessibility source review.
- Search now opens both enclosing sidebar sections and matching child-session groups. Saved collapse state changes through explicit user activation outside search, not queued native toggle events; clear-search and remount preserve the saved layout. The real-browser regression covers both levels, child selection, remount/restoration, pointer, Enter and Space. This clears the P2 in review 5268681523.
- The standalone Sessions timeline now receives
session.viewer; the previous recipient/retry ownership and typing-geometry repairs are unchanged. Independent memberships and the non-blocking disposition of aggregate child unread remain accepted. - All 12 checks attached to this head are successful, including
CI required, Rust/tool integration, JavaScript, DCO and all four browser shards. Hosted run 35622938962 reports 1,769 Vitest tests and 418 functional browser cases passing. I read the logs and confirmed search, session navigation and channel/thread typing geometry passed in both Chromium and WebKit. CI exercised GitHub’s synthetic merge of this head into6201ab6aa5e916b95ca22f48ab1457e8812e2792, not a pristine local head checkout or the later current base.
Non-blocking coverage correction: the added membership assertion in tests/browser/session-navigation.spec.mjs:104–109 opens a conversation target. src/app/navigation.ts:35–39 routes that target through Messages, not standalone SessionsPage. It therefore does not directly guard the new standalone viewer prop. The one-line production repair is correct in source; a direct standalone-page assertion is follow-up, not another repair gate.
Validation was pinned-object source review plus existing hosted CI evidence. No local tests, live/native acceptance, code edits, approval or merge were performed. My previous code objections are resolved; Wes may replace the existing changes-requested decision with approval.
2 participants
Adds a Sessions workspace and child sessions under channels in Messages. Conversations use a centered 55% column and the normal composer, with the agent picker beside Send. Choosing an agent changes the default recipient; sending refreshes membership and invites missing recipients. Explicit mentions override that default, including creation and retries. Replies appear inline.
Sessions use ordinary private channels with independent membership; nesting is app metadata. No relay-server changes or migrations. Links to older messages use the existing verified lookup and open inline with Back to latest; sending returns to the latest conversation.
Validation
At
8b6c730: 1,641 unit tests, 38 Node integration tests, 406 Chromium/WebKit browser cases, 20 design browser cases, TypeScript, builds, formatting, design checks, and Clippy passed.just scanreached its final native test command: 11/12 passed;real_spawn_fences_secrets_and_preserves_login_contexttimed out on this Mac's interactive ClickFix shell acknowledgment. That local environment blocker is unrelated to Sessions; hosted checks remain authoritative for the new head and are pending.Recipient regressions: six failures before the fix; all 12 cases pass afterward. Six mounted tests cover exact-target failure, retry, cancellation, disposal, and post-open removal. Independent review found no remaining blocker.
Browser coverage: two journeys added, none removed. Chromium and WebKit exercise verified lookup → real row focus/visibility → navigation completion, loaded virtual replies, and send/return-to-latest. Lifecycle permutations stay in component tests. Existing channel-navigation and page-layout coverage is retained; page lists now include Sessions.
Local timing for the new journeys on macOS arm64, production frontend with fixture HTTP: build 1.13s Chromium / 1.00s WebKit; four executions 5.8s total. Cold root/reply opens 78/51ms Chromium and 108/97ms WebKit; warm replies 45/92ms. These are local diagnostics, not hosted CI measurements; no comparable fixture-setup baseline was recorded.