Skip to content

feat: touch-optimize new-chat participant chips + NewChatDraft mobile tests#1745

Open
Gandy2025 wants to merge 2 commits into
mainfrom
feat/mobile-newchat-chips-and-test
Open

feat: touch-optimize new-chat participant chips + NewChatDraft mobile tests#1745
Gandy2025 wants to merge 2 commits into
mainfrom
feat/mobile-newchat-chips-and-test

Conversation

@Gandy2025

Copy link
Copy Markdown
Contributor

What

Closes the two non-blocking follow-ups from #1744's reviews, finishing the mobile New task surface.

1. Participant-chip controls are touch-usable (AgentToken + the [+] picker):

  • The chip remove × was opacity-0 group-hover:opacity-100 — invisible on touch (no hover). On mobile it's now always visible with a roomier tap target; desktop keeps the compact hover-revealed ×.
  • The [+] add-participant button is enlarged to a real tap target on mobile. Its desktop className stays byte-equivalent (the justify-center is gated on mobile).
  • Uses the existing mobile prop, threaded NewChatDraft → ParticipantChips → AgentToken. AgentToken is only used by NewChatDraft, so no other surface changes.

2. NewChatDraft regression tests (the coverage gap both reviewers flagged):

  • New render tests (via CenterPanel draft, reusing the chat-view harness mocks) assert the mobile contract — 44px send, Enter-does-not-create, chip × visible, [+] enlarged — and the unchanged desktop branch (compact send, hover-revealed ×).

Desktop is unchanged; every visual/behavior change is mobile-gated (default false).

Verification

  • tsc + lint:tokens + biome clean
  • ✅ 304 tests green across components/chat, mention-autocomplete, center, conversations (incl. the 2 new NewChatDraft cases)

With this, all three mobile composers and the new-task recipient controls are touch-complete and consistent. Same QA caveat as #1742/#1744: no live authed on-device render from my environment; low risk by construction (constant mobile prop, same elements restyled).

…bile tests

Completes the mobile new-task surface (follow-up to #1744 review notes):

- Participant chips (`AgentToken`): on mobile the remove × is always visible
  (touch has no hover) with a roomier tap target; desktop keeps the compact
  hover-revealed ×.
- The [+] add-participant button is enlarged to a real tap target on mobile;
  desktop className stays byte-equivalent (justify-center gated on mobile).
- Threads the existing `mobile` prop through NewChatDraft -> ParticipantChips
  -> AgentToken.

Tests: adds NewChatDraft render coverage (via CenterPanel draft) asserting the
mobile contract — 44px send, Enter-does-not-create, chip × visible, [+]
enlarged — and the unchanged desktop branch (compact send, hover-revealed ×).

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by baixiaohang-code-reviewer - PR #1745

Recommendation: request changes

  • Rationale: The route gating and regression coverage are sound, but the participant controls remain below the mobile touch minimum this follow-up is intended to complete.

Risk level: C

  • Path baseline: only packages/web/** is changed.
  • Semantic lift: none; this is route-gated Web interaction and sizing with no subscription, WebSocket, or data-layer change.

PR summary

  • Author / repo: Gandy2025 / agent-team-foundation/first-tree
  • Problem: On phones, people starting a new task cannot reliably remove a participant or open the participant picker because the existing controls depend on hover or use desktop-sized hit areas.
  • Approach: Thread the explicit mobile presentation into participant chips, keep remove controls visible on mobile, enlarge both participant actions, and add focused mobile/desktop draft render coverage while preserving the desktop branch.
  • Impacted modules: packages/web/src/components/mention-autocomplete.tsx, packages/web/src/pages/workspace/conversations/new-chat-draft.tsx, and the CenterPanel DOM test harness.

Review findings
❌ 1. The two participant actions still do not reach the existing 44×44 mobile touch minimum: the remove button is 26×26, and Add participant is only 40×32. These are the actual standalone button boxes, not icons inside a larger clickable control, so making the × visible fixes discoverability but leaves the hit targets materially undersized. The surrounding NewChatDraft/ChatView/AskTakeover mobile controls consistently call 44×44 the touch minimum. Please give both actions a true touch-sized hit area while keeping the visual chip compact if needed, and make the mobile regression assert both dimensions; the current test checks only Add participant's 40px width and remove visibility, which would lock in the incomplete contract. [mention-autocomplete.tsx:217, new-chat-draft.tsx:1066, chat-view-dom.test.tsx:2051]

✅ The explicit presentation === "mobile" signal continues to isolate mobile behavior from desktop and generic narrow Workspace.
✅ The new draft render cases cover mobile Enter behavior and the unchanged desktop hover branch without altering chat-creation semantics.

Action taken

  • Submitted request changes.

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

Review summary

The implementation goal is sound: carry the explicit mobile presentation into the New task participant controls, keep chip removal visible on touch devices, enlarge the picker affordance, and add mobile/desktop regression coverage without changing desktop behavior.

The route gating and behavioral tests are correct, but I found one blocker before this can be called touch-complete:

  • The two participant actions still miss the 44×44 mobile touch minimum already established by the surrounding composers. The remove button's actual hit box is 26×26 (mention-autocomplete.tsx:217), while Add participant is 40×32 (new-chat-draft.tsx:1066). These are standalone button boxes, not small icons inside a larger clickable control, so the remove action is now discoverable but both actions remain materially undersized for touch. Please give each action a 44×44 hit area while keeping the visible chip compact if necessary, and update the mobile regression to assert both dimensions for both controls. The current test expects the incomplete 40px width and checks only visibility for remove (chat-view-dom.test.tsx:2051-2056).

The explicit presentation === "mobile" signal otherwise keeps desktop and generic narrow Workspace isolated, and Enter still follows the intended mobile button-only submission contract. There are no database, schema, WebSocket/subscription, or core-data-structure changes.

Local verification on 55caaac0: 39/39 targeted chat-view-dom tests passed; Web typecheck and design-token guardrails passed; Biome passed on all three changed files; git diff --check was clean. The DOM suite still emits its pre-existing localhost ECONNREFUSED stderr noise, but exits green.

Review blocker: the chip remove × (26x26) and the [+] add-participant button
(40x32) were below the 44x44 touch minimum the composer controls establish.
Both now get a true 44x44 hit area on mobile (glyphs stay visually small,
centered); desktop is unchanged. The mobile regression asserts both dimensions
for both controls. Also gates the previously-unconditional justifyContent on
the remove button so the desktop style stays byte-equivalent.
@Gandy2025

Copy link
Copy Markdown
Contributor Author

Fixed in 9e3d07ee — both of you were right; I'd undershot the established 44×44 contract.

  • Chip remove ×: 26×26 → 44×44 hit area (AgentToken), glyph stays small/centered, always-visible on mobile.
  • [+] add-participant: 40×32 → 44×44 (ParticipantChips).
  • The mobile regression now asserts both dimensions for both controls (not just [+] width + remove visibility).
  • Also gated the remove button's justifyContent so the desktop style stays byte-equivalent (per the inline-review note).

Green locally: tsc + lint:tokens + biome clean, 68 targeted tests pass. Re-requesting review.

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by baixiaohang-code-reviewer - PR #1745 (follow-up)

Recommendation: approve

  • Rationale: Commit 9e3d07ee fully resolves the touch-target blocker without widening the behavioral scope or changing the desktop branch.

Risk level: C

  • Path baseline: the PR remains within packages/web/**.
  • Semantic lift: none; the follow-up only corrects route-gated control sizing and its DOM assertions.

PR summary

  • Author / repo: Gandy2025 / agent-team-foundation/first-tree
  • Problem: People starting a task on a phone need participant add/remove actions that are both discoverable and reliably tappable, while desktop keeps its compact hover interaction.
  • Approach: Keep removal visible on mobile, give both participant actions the same 44×44 touch target used by the surrounding mobile composers, preserve the small centered glyphs and desktop styles, and lock the mobile/desktop split with focused render assertions.
  • Impacted modules: packages/web/src/components/mention-autocomplete.tsx, packages/web/src/pages/workspace/conversations/new-chat-draft.tsx, and the CenterPanel DOM test harness.

Review findings
✅ The previous blocker is resolved: both the chip remove button and Add participant now expose a true 44×44 mobile hit area.
✅ The regression now asserts width and height for both controls, so it no longer codifies the undersized intermediate state.
justifyContent is mobile-gated on the remove button, preserving the desktop style branch; the explicit presentation signal still isolates mobile from generic narrow Workspace.
✅ The increment does not alter participant selection, mention routing, Enter submission, or chat-creation semantics.

Action taken

  • Approved the updated head.

@yuezengwu yuezengwu 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 review

Re-reviewed the 55caaac0..9e3d07ee increment. The previous blocker is fully resolved:

  • Both standalone mobile participant actions now expose the established 44×44 hit area; the remove glyph and plus glyph remain visually small and centered.
  • The mobile regression asserts width and height for both Add participant and chip removal, so it no longer locks in the undersized intermediate state.
  • justifyContent for chip removal is mobile-gated, preserving the desktop style branch; the explicit presentation signal still isolates mobile behavior from generic narrow Workspace.

The increment does not change participant selection, mention routing, Enter submission, chat creation, database/schema, WebSocket/subscription, or core-data-structure behavior. I found no remaining blocker.

Local verification on 9e3d07ee: 39/39 targeted chat-view-dom tests passed; Web typecheck and design-token guardrails passed; Biome passed on all three changed files; the incremental git diff --check was clean. The DOM suite's pre-existing localhost ECONNREFUSED stderr noise remains non-failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants