[SM PROTOTYPE - DO NOT MERGE] Group Desktop - #1237
Conversation
|
Claude finished @shanemac's task —— View job PR Review CompleteI've completed a comprehensive review of PR #1237. This is a significant prototype that introduces the Group Desktop experience alongside a well-designed design system foundation. Below are my findings organized by severity. 🔴 Critical Issues1. Work Menu Layout Broken on Standard DevicesLocation: The
This causes the bubble preview to scale by a negative ratio, breaking the entire work menu layout. This is already flagged by Macroscope. Fix: Remove the duplicate 2. Things Page Shows Group UI for 1-on-1 ConversationsLocation: The
Fix: Gate 3. Third Pager Page Always PresentLocation: Every Fix: Conditionally include the 🟡 High-Priority Issues4. Hard-Coded Demo Content in Production ViewsLocation: The
Any group opening Things sees this fabricated content as if it's theirs. These rows are tappable and route into real flows (outputs, notes, reminders). Fix: Populate from actual conversation data or show an explicit empty/onboarding state. Macroscope identified this. 5. Work Menu Button Shows X but Doesn't CloseLocation: When Fix: Call 6. Work Menu Not Updated on Membership ChangesLocation:
Fix: Update 7. Type-Check Performance RiskLocation: The view body is 729 lines with:
This structure is extremely close to triggering the 100ms type-check budget (repository's Recommendation: Extract sections like 🟠 Medium-Priority Issues8. Start Chip Titles IgnoredLocation: All Fix: Pass the title through 9. "Tahoe Weekend" Fallback for Unnamed GroupsLocation: The Fix: Use a neutral placeholder like 10. "Ask Zoe" Text Not a Separate ButtonLocation: The Fix: Make 11. Lost Draft When No AgentLocation: When the work menu's Fix: Preserve the draft or show a prompt before presenting the builder. Macroscope identified this. 12.
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| - Preview snapshots for the catalog in light/dark and accessibility sizes | ||
| - A lint rule that flags new raw RGB colors and arbitrary layout values | ||
| - Accessibility UI checks for tap targets and clipped text | ||
| - A debug-only route to the catalog for device review, if the team wants it |
There was a problem hiding this comment.
🟢 Low docs/design-system-audit.md:153
The migration backlog lists "A debug-only route to the catalog for device review" under "Add later when build tooling is available," but the same document already records that route as introduced in the framework inventory on line 122. This leaves maintainers with a false outstanding task that contradicts the documented current state. Remove the duplicate backlog item or update it to reflect that the route already exists.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @docs/design-system-audit.md around line 153:
The migration backlog lists "A debug-only route to the catalog for device review" under "Add later when build tooling is available," but the same document already records that route as introduced in the framework inventory on line 122. This leaves maintainers with a false outstanding task that contradicts the documented current state. Remove the duplicate backlog item or update it to reflect that the route already exists.
shanemac
changed the title
| "Connect this group to the service needed for this message from \(sender): \(context)\n\n" | ||
| + "Explain why the connection helps and ask for the minimum access needed." | ||
| ) | ||
| case .askAgentPrivately: |
There was a problem hiding this comment.
🟡 Medium Conversation Detail/ConversationView.swift:1072
When the work menu's .askAgentPrivately action is used in a group with no agent, openAgentSideChat presents the agent builder but discards the composed draft, so after the user adds an agent the selected message and requested private-agent action are lost. The work menu is enabled for any qualifying group regardless of whether an agent exists, so this path is reachable.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/ConversationView.swift around line 1072:
When the work menu's `.askAgentPrivately` action is used in a group with no agent, `openAgentSideChat` presents the agent builder but discards the composed draft, so after the user adds an agent the selected message and requested private-agent action are lost. The work menu is enabled for any qualifying group regardless of whether an agent exists, so this path is reachable.
| ) | ||
| } | ||
|
|
||
| var thingsPage: some View { |
There was a problem hiding this comment.
🟠 High Conversation Detail/ConversationView.swift:991
thingsPage now unconditionally renders GroupDesktopView for every conversation, so one-on-one conversations lose their prior Things page and instead show group-only content and controls (member lists, agent-side-chat, group setup actions). The property returns GroupDesktopView without checking isGroupConversation. Gate this replacement to group conversations or preserve the existing files/links page for non-groups.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/ConversationView.swift around line 991:
`thingsPage` now unconditionally renders `GroupDesktopView` for every conversation, so one-on-one conversations lose their prior Things page and instead show group-only content and controls (member lists, agent-side-chat, group setup actions). The property returns `GroupDesktopView` without checking `isGroupConversation`. Gate this replacement to group conversations or preserve the existing files/links page for non-groups.
| @@ -622,10 +600,12 @@ struct ConversationView<MessagesBottomBar: View>: View { | |||
| .onAppear { | |||
There was a problem hiding this comment.
🟡 Medium Conversation Detail/ConversationView.swift:600
contextMenuState.isWorkMenuEnabled is set only in .onAppear from viewModel.conversation.isGroupConversation, so it never updates when membership changes while the conversation stays open. A conversation that becomes a group after appearing keeps group work menu actions disabled, and one that drops below the group threshold retains them enabled, until the view is left and re-entered. Consider recomputing isWorkMenuEnabled on membership changes (e.g., via .onChange(of: viewModel.conversation.members)).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/ConversationView.swift around line 600:
`contextMenuState.isWorkMenuEnabled` is set only in `.onAppear` from `viewModel.conversation.isGroupConversation`, so it never updates when membership changes while the conversation stays open. A conversation that becomes a group after appearing keeps group work menu actions disabled, and one that drops below the group threshold retains them enabled, until the view is left and re-entered. Consider recomputing `isWorkMenuEnabled` on membership changes (e.g., via `.onChange(of: viewModel.conversation.members)`).
| public var agentPhone: String? { | ||
| let value = metadata?[Constant.phoneKey]?.stringValue | ||
| ?? metadata?[Constant.phoneNumberKey]?.stringValue | ||
| return value.flatMap { rawValue in | ||
| let trimmed = rawValue.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| return trimmed.isEmpty ? nil : trimmed | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium Models/Profile.swift:276
agentPhone reads phone and falls back to phone_number via ?? before trimming, so when phone is present but whitespace-only and phone_number holds a valid number, the whitespace value wins and is then coerced to nil — the valid fallback is never checked. During a rollout where both keys coexist, this hides a valid SMS address. Move the trim/empty-coercion inside each branch so the fallback only triggers when the first key is actually absent rather than whitespace-only.
| public var agentPhone: String? { | |
| let value = metadata?[Constant.phoneKey]?.stringValue | |
| ?? metadata?[Constant.phoneNumberKey]?.stringValue | |
| return value.flatMap { rawValue in | |
| let trimmed = rawValue.trimmingCharacters(in: .whitespacesAndNewlines) | |
| return trimmed.isEmpty ? nil : trimmed | |
| } | |
| } | |
| public var agentPhone: String? { | |
| let trim: (String?) -> String? = { rawValue in | |
| rawValue.flatMap { value in | |
| let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines) | |
| return trimmed.isEmpty ? nil : trimmed | |
| } | |
| } | |
| return trim(metadata?[Constant.phoneKey]?.stringValue) | |
| ?? trim(metadata?[Constant.phoneNumberKey]?.stringValue) | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Storage/Models/Profile.swift around lines 276-283:
`agentPhone` reads `phone` and falls back to `phone_number` via `??` *before* trimming, so when `phone` is present but whitespace-only and `phone_number` holds a valid number, the whitespace value wins and is then coerced to `nil` — the valid fallback is never checked. During a rollout where both keys coexist, this hides a valid SMS address. Move the trim/empty-coercion inside each branch so the fallback only triggers when the first key is actually absent rather than whitespace-only.
saulmc
changed the title
| @@ -0,0 +1,698 @@ | |||
| import ConvosCore | |||
| import SwiftUI | |||
| import UIKit | |||
There was a problem hiding this comment.
🟡 Medium Conversation Detail/GroupDesktopView.swift:3
sharedWork and activity render hard-coded Tahoe demo content for every conversation. sharedWork always shows a fixed cabin comparison with Google Docs status, Zoe/Jimmy prompts, and suggestions, while activity always shows fixed rows for Shane, Brent, Julie, and Kai. Any real or newly created group opening the Things page will present this fabricated work and activity as if it belonged to that group, and these rows are tappable, routing users into the group's real outputs/notes/reminders/agents flows as if that content existed. These sections should be populated from the actual conversation data or replaced with an explicit empty/onboarding state.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/GroupDesktopView.swift around line 3:
`sharedWork` and `activity` render hard-coded Tahoe demo content for every conversation. `sharedWork` always shows a fixed cabin comparison with Google Docs status, Zoe/Jimmy prompts, and suggestions, while `activity` always shows fixed rows for Shane, Brent, Julie, and Kai. Any real or newly created group opening the Things page will present this fabricated work and activity as if it belonged to that group, and these rows are tappable, routing users into the group's real outputs/notes/reminders/agents flows as if that content existed. These sections should be populated from the actual conversation data or replaced with an explicit empty/onboarding state.
| .foregroundStyle(.secondary) | ||
| } | ||
|
|
||
| HStack { |
There was a problem hiding this comment.
🟡 Medium Conversation Detail/GroupDesktopView.swift:388
The Ask Zoe text in sharedWork is not a separate button — it is plain text inside the card Button whose handler always calls onAction(.openOutputs). Tapping the Ask Zoe text therefore opens the work log instead of performing the ask action that the label implies. Consider making Ask Zoe a separately wired Button (e.g., with .askEveryone) or removing the accent-colored action treatment so it does not read as a distinct interactive action.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/GroupDesktopView.swift around line 388:
The `Ask Zoe` text in `sharedWork` is not a separate button — it is plain text inside the card `Button` whose handler always calls `onAction(.openOutputs)`. Tapping the `Ask Zoe` text therefore opens the work log instead of performing the ask action that the label implies. Consider making `Ask Zoe` a separately wired `Button` (e.g., with `.askEveryone`) or removing the accent-colored action treatment so it does not read as a distinct interactive action.
There was a problem hiding this comment.
🟡 Medium
https://github.com/xmtplabs/convos-ios/blob/9b26cfeaf11c24d74312e918b6350559b5fbf0d0/Convos/Conversation Detail/ConversationView.swift#L573
Every ConversationView mounts a third pager page for a group-agent side chat, including one-to-one conversations and conversations with no agent. When groupAgent is nil the agentPage closure still builds a side-chat view with a placeholder "Mountain Guide" agent, and ConversationPager unconditionally renders all three pages with three page dots, so users can swipe or tap into a group-agent surface backed by no real agent. The nil guard in openAgentSideChat only blocks the send button, not direct pager navigation. Consider suppressing the agent page (and its dot) for conversations that don't have a group agent, falling back to the prior two-page pager.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/ConversationView.swift around line 573:
Every `ConversationView` mounts a third pager page for a group-agent side chat, including one-to-one conversations and conversations with no agent. When `groupAgent` is nil the `agentPage` closure still builds a side-chat view with a placeholder "Mountain Guide" agent, and `ConversationPager` unconditionally renders all three pages with three page dots, so users can swipe or tap into a group-agent surface backed by no real agent. The nil guard in `openAgentSideChat` only blocks the send button, not direct pager navigation. Consider suppressing the `agent` page (and its dot) for conversations that don't have a group agent, falling back to the prior two-page pager.
| Button { | ||
| UIImpactFeedbackGenerator(style: .light).impactOccurred() | ||
| contextMenuState.present( | ||
| message: message, | ||
| bubbleFrame: currentBubbleFrame, | ||
| bubbleStyle: bubbleStyle, | ||
| isExpanded: isExpanded, | ||
| segment: segment, | ||
| presentation: .work | ||
| ) |
There was a problem hiding this comment.
🟡 Medium MessagesListView/MessageContextMenuWrapper.swift:110
The work button shows an xmark icon when isSourceBubble is true, but tapping it calls contextMenuState.present(...) again instead of dismiss(). Tapping the apparent close control re-presents the same work menu rather than closing it, so the menu can't be dismissed via the button. Consider calling contextMenuState.dismiss() when isSourceBubble is true.
| Button { | |
| UIImpactFeedbackGenerator(style: .light).impactOccurred() | |
| contextMenuState.present( | |
| message: message, | |
| bubbleFrame: currentBubbleFrame, | |
| bubbleStyle: bubbleStyle, | |
| isExpanded: isExpanded, | |
| segment: segment, | |
| presentation: .work | |
| ) | |
| Button { | |
| UIImpactFeedbackGenerator(style: .light).impactOccurred() | |
| if isSourceBubble { | |
| contextMenuState.dismiss() | |
| } else { | |
| contextMenuState.present( | |
| message: message, | |
| bubbleFrame: currentBubbleFrame, | |
| bubbleStyle: bubbleStyle, | |
| isExpanded: isExpanded, | |
| segment: segment, | |
| presentation: .work | |
| ) | |
| } | |
| } label: { |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosComposer/MessagesListView/MessageContextMenuWrapper.swift around lines 110-119:
The work button shows an `xmark` icon when `isSourceBubble` is true, but tapping it calls `contextMenuState.present(...)` again instead of `dismiss()`. Tapping the apparent close control re-presents the same work menu rather than closing it, so the menu can't be dismissed via the button. Consider calling `contextMenuState.dismiss()` when `isSourceBubble` is true.
There was a problem hiding this comment.
🟠 High
endBubbleRect double-counts the work menu height, producing a negative maxContentHeight and an invalid preview on standard devices. When state.presentation == .work, menuHeight is set to C.workMenuEstimatedHeight (510) and passed in. Inside endBubbleRect, bottomPadding includes menuHeight plus C.verticalBreathingRoom (80), and then maxContentHeight subtracts an additional C.verticalBreathingRoom:
maxContentHeight = screenSize.height - minY - (sectionSpacing + menuHeight + verticalBreathingRoom) - verticalBreathingRoom
On an 852pt iPhone with a 59pt safe top this is 852 - 187 - 598 - 80 = -13; on a 667pt device it is -151. The endHeight > maxContentHeight branch then scales endWidth/endHeight by a negative ratio, and the work menu is positioned from that invalid endBubble. The work menu layout is therefore broken on normal-sized phones. Remove the duplicate C.verticalBreathingRoom subtraction from maxContentHeight, and clamp maxContentHeight to a positive minimum so the preview never receives negative geometry on compact screens.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosComposer/MessagesListView/MessageContextMenu/MessageContextMenuOverlay.swift around line 337:
`endBubbleRect` double-counts the work menu height, producing a negative `maxContentHeight` and an invalid preview on standard devices. When `state.presentation == .work`, `menuHeight` is set to `C.workMenuEstimatedHeight` (510) and passed in. Inside `endBubbleRect`, `bottomPadding` includes `menuHeight` plus `C.verticalBreathingRoom` (80), and then `maxContentHeight` subtracts an additional `C.verticalBreathingRoom`:
`maxContentHeight = screenSize.height - minY - (sectionSpacing + menuHeight + verticalBreathingRoom) - verticalBreathingRoom`
On an 852pt iPhone with a 59pt safe top this is `852 - 187 - 598 - 80 = -13`; on a 667pt device it is `-151`. The `endHeight > maxContentHeight` branch then scales `endWidth`/`endHeight` by a negative ratio, and the work menu is positioned from that invalid `endBubble`. The work menu layout is therefore broken on normal-sized phones. Remove the duplicate `C.verticalBreathingRoom` subtraction from `maxContentHeight`, and clamp `maxContentHeight` to a positive minimum so the preview never receives negative geometry on compact screens.
| ) { | ||
| VStack(alignment: .leading, spacing: 8) { | ||
| HStack(spacing: 8) { | ||
| startChip("A trip") |
There was a problem hiding this comment.
🟡 Medium Conversation Detail/GroupDesktopView.swift:162
The startChip("A trip"), startChip("My family"), and other chips all emit the same .describeGroup action, discarding the chip title. Tapping any starting point produces identical behavior — the selected group purpose never reaches the handler, so these distinct onboarding choices do not perform their advertised selection.
startChip ignores its title parameter and always calls onAction(.describeGroup). Consider passing the title through Action (e.g., via an associated value like .describeGroup(String)) so the handler receives the user's selection.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/GroupDesktopView.swift around line 162:
The `startChip("A trip")`, `startChip("My family")`, and other chips all emit the same `.describeGroup` action, discarding the chip title. Tapping any starting point produces identical behavior — the selected group purpose never reaches the handler, so these distinct onboarding choices do not perform their advertised selection.
`startChip` ignores its `title` parameter and always calls `onAction(.describeGroup)`. Consider passing the title through `Action` (e.g., via an associated value like `.describeGroup(String)`) so the handler receives the user's selection.
| let name = conversation.name?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
| return name.isEmpty ? "Tahoe Weekend" : name | ||
| } |
There was a problem hiding this comment.
🟡 Medium Conversation Detail/GroupDesktopView.swift:47
The groupName fallback uses the hardcoded literal "Tahoe Weekend" whenever the conversation has no name or a whitespace-only name. This makes the people header mislabel any unnamed group as a Tahoe trip, even though unnamed groups are a supported state (the codebase uses "this group" elsewhere for the same condition). Consider using a neutral placeholder consistent with the rest of the app instead of the hardcoded demo string.
| let name = conversation.name?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | |
| return name.isEmpty ? "Tahoe Weekend" : name | |
| } | |
| let name = conversation.name?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | |
| return name.isEmpty ? "This group" : name |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/GroupDesktopView.swift around lines 47-49:
The `groupName` fallback uses the hardcoded literal `"Tahoe Weekend"` whenever the conversation has no name or a whitespace-only name. This makes the people header mislabel any unnamed group as a Tahoe trip, even though unnamed groups are a supported state (the codebase uses `"this group"` elsewhere for the same condition). Consider using a neutral placeholder consistent with the rest of the app instead of the hardcoded demo string.
| agentAvatar(agent, isGroupAgent: index == 0) | ||
|
|
||
| VStack(alignment: .leading, spacing: 3) { | ||
| Text(agent.profile.displayName) |
There was a problem hiding this comment.
🟡 Medium Conversation Detail/GroupAgentsView.swift:88
The directory row renders agent.profile.displayName directly instead of agent.displayName, so an unverified agent can display its raw profile-supplied name instead of the intentional "Agent" fallback. This bypasses the ConversationMember.displayName guard that prevents unverified agents from presenting arbitrary names, allowing impersonation in the agent directory. The same raw name is also used for the avatar initial (agentAvatar) and groupAgentName. Consider using agent.displayName in all three places.
| Text(agent.profile.displayName) | |
| Text(agent.displayName) |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/GroupAgentsView.swift around line 88:
The directory row renders `agent.profile.displayName` directly instead of `agent.displayName`, so an unverified agent can display its raw profile-supplied name instead of the intentional "Agent" fallback. This bypasses the `ConversationMember.displayName` guard that prevents unverified agents from presenting arbitrary names, allowing impersonation in the agent directory. The same raw name is also used for the avatar initial (`agentAvatar`) and `groupAgentName`. Consider using `agent.displayName` in all three places.
| private var agentName: String { | ||
| agents.first?.profile.displayName ?? "Mountain Guide" | ||
| } |
There was a problem hiding this comment.
🟡 Medium Conversation Detail/GroupDesktopView.swift:42
agentName falls back to the hardcoded string "Mountain Guide" when the group has no agent, so the entire screen refers to a nonexistent agent — e.g. "tell Mountain Guide anything", "Mountain Guide can privately ask each person", and "ask Mountain Guide to send something" — and surfaces agent-dependent actions like "Research it" and "Ask everyone" as if an agent exists. A group with only human members still reaches this view, so the fallback presents a fabricated agent name and agent-dependent CTAs instead of a no-agent/setup state. Consider falling back to an empty or neutral label (or gating the agent-dependent sections) when agents is empty, so the UI doesn't reference an agent that doesn't exist.
| private var agentName: String { | |
| agents.first?.profile.displayName ?? "Mountain Guide" | |
| } | |
| private var agentName: String { | |
| agents.first?.profile.displayName ?? "" | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/GroupDesktopView.swift around lines 42-44:
`agentName` falls back to the hardcoded string `"Mountain Guide"` when the group has no agent, so the entire screen refers to a nonexistent agent — e.g. "tell Mountain Guide anything", "Mountain Guide can privately ask each person", and "ask Mountain Guide to send something" — and surfaces agent-dependent actions like "Research it" and "Ask everyone" as if an agent exists. A group with only human members still reaches this view, so the fallback presents a fabricated agent name and agent-dependent CTAs instead of a no-agent/setup state. Consider falling back to an empty or neutral label (or gating the agent-dependent sections) when `agents` is empty, so the UI doesn't reference an agent that doesn't exist.
| .font(.headline) | ||
| .foregroundStyle(.colorTextPrimary) | ||
|
|
||
| Text(index == 0 |
There was a problem hiding this comment.
🟡 Medium Conversation Detail/GroupAgentsView.swift:92
The agentDirectory section displays hardcoded ownership text ("Shane's Convos agent · Group use allowed" / "Shane's agent · Private power") regardless of which member actually owns each agent, and determines group-agent status solely from array position (index == 0). For any conversation whose agents are owned by other members, or whose first agent is private, the directory shows false ownership and access-permission information. ConversationMember.invitedBy is available but never consulted; the per-row label and the isGroupAgent flag should derive from member/verification/ownership data instead of a fixed name and index.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @Convos/Conversation Detail/GroupAgentsView.swift around line 92:
The `agentDirectory` section displays hardcoded ownership text ("Shane's Convos agent · Group use allowed" / "Shane's agent · Private power") regardless of which member actually owns each agent, and determines group-agent status solely from array position (`index == 0`). For any conversation whose agents are owned by other members, or whose first agent is private, the directory shows false ownership and access-permission information. `ConversationMember.invitedBy` is available but never consulted; the per-row label and the `isGroupAgent` flag should derive from member/verification/ownership data instead of a fixed name and index.
1 participant
Summary
Builds the new Group Desktop vision on top of a reusable Convos design-system foundation.
Why
The first experience should make a Convo feel useful before anyone understands agents. Messages become work, the work returns to a shared place, and every result invites the group to add context, edit, research, or keep improving it. Agents remain visible and attributable without turning the product into an agent-management interface.
User impact
A group now has three connected spaces:
The desktop is action-oriented, shows who and what can help, and makes connected services part of the group rather than a separate settings concept.
Validation
git diff --checkpassedReview notes
This remains a draft product exploration. The new screens intentionally use the existing iOS visual language and shared design tokens while rethinking the product model around the Group Desktop.