Skip to content

task/mobile ux and touch support - #849

Merged
lsm merged 2 commits into
devfrom
task/mobile-ux-and-touch-support
Mar 25, 2026
Merged

task/mobile ux and touch support#849
lsm merged 2 commits into
devfrom
task/mobile-ux-and-touch-support

Conversation

@lsm

@lsm lsm commented Mar 25, 2026

Copy link
Copy Markdown
Owner

lsm
lsm commented Mar 25, 2026
View reviewed changes

@lsm lsm left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤖 Review by glm-5-turbo (GLM)

Model: glm-5-turbo | Client: NeoKai | Provider: GLM

Summary

This PR adds mobile UX improvements to ReferenceAutocomplete, integrates the reference autocomplete into InputTextarea (new props), and wires up the useReferenceAutocomplete hook in MessageInput. The implementation is clean and follows the existing patterns well. All tests pass (76/76), lint is clean, and typecheck passes.

What works well

  • The touchend close handler in ReferenceAutocomplete.tsx (line 139) correctly checks containerRef.current.contains(e.target) so taps on items inside the dropdown do NOT close it -- only outside taps dismiss it. The existing test coverage for this is solid.
  • The handleReferenceInsert callback in MessageInput.tsx (lines 122-134) uses a regex /((?:^|\s))@(\S*)$/ that aligns well with extractActiveAtQuery in the hook. The replacement always appends a trailing space to prevent the @ref{...} token from re-triggering autocomplete.
  • The mobile-specific adaptations (full-width, larger py-3 touch targets, active:bg-dark-700/70 feedback, behavior: 'auto' scroll, "Tap to select" footer) are all sensible defaults for touch devices.
  • The isMobileDevice ref in InputTextarea.tsx avoids causing re-renders on mount, while still allowing the send button tooltip to adapt.
  • The guard rendering {showReferenceAutocomplete && onReferenceSelect && onReferenceClose && ...} in InputTextarea.tsx matches the pattern already used for CommandAutocomplete.

Issues found

P1: CommandAutocomplete lacks touch support (inconsistency)

ReferenceAutocomplete.tsx now has touchend close handling and mobile-adaptive UI, but CommandAutocomplete.tsx still only uses mousedown and has no mobile-specific adaptations (fixed minWidth: '250px'/maxWidth: '400px', py-2 items, keyboard-only footer hint). This isn't introduced by this PR (the file is untouched), but it creates an inconsistency: on mobile, the command autocomplete will not close on touch-outside, may overflow the viewport, and shows keyboard shortcuts irrelevant to touch users.

This is a pre-existing gap, not a regression, but worth flagging as follow-up work since this PR sets the precedent.

P2: No test for handleReferenceInsert replacement logic

There are tests for InputTextarea rendering the reference autocomplete, and tests for the hook's extractActiveAtQuery function, but no test specifically verifying that handleReferenceInsert in MessageInput correctly replaces @query with @ref{type:id} in the textarea content. The replacement logic (regex match, slice, prefix reconstruction) has non-trivial string manipulation that would benefit from a unit test. For example, verifying that "hello @fix" becomes "hello @ref{task:t-1} " after selection.

P2: handleReferenceInsert regex is subtly different from extractActiveAtQuery

The handleReferenceInsert callback (line 126) uses content.match(/((?:^|\s))@(\S*)$/) which is a simpler regex than extractActiveAtQuery in the hook. Specifically, extractActiveAtQuery walks backwards to find the last @ preceded by whitespace or start-of-string, while the replacement regex matches the last occurrence of @\S* at the end. These are functionally equivalent for all practical cases, but there's a subtle difference: extractActiveAtQuery correctly handles @@ (treating the inner @ as query text), while the replacement regex would also match @@\S*$ and replace the second @ onwards. Since extractActiveAtQuery would return "@" as the query for @@, the hook would search for "@" which would return empty results, so this case would never reach handleReferenceInsert. No bug in practice, but worth a comment for clarity.

P3: Mobile Enter behavior recomputes touch detection on every keydown

In MessageInput.tsx line 260-262, isTouchDevice is recomputed inside handleKeyDown on every keydown event:

const isTouchDevice =
    window.matchMedia('(pointer: coarse)').matches ||
    ('ontouchstart' in window && window.innerWidth < 768);

This runs on every keypress, even on desktop where the value never changes. While matchMedia is fast, this could be memoized (e.g., via a ref or module-level variable) for consistency with the pattern used in InputTextarea.tsx (lines 96-104) and ReferenceAutocomplete.tsx (lines 117-119).

Recommendation

APPROVE -- The implementation is correct, well-tested, and follows existing patterns. The P2 issues are meaningful but non-blocking. The missing handleReferenceInsert test (P2) and the CommandAutocomplete mobile gap (P1) are the most actionable follow-ups.

lsm
lsm commented Mar 25, 2026
View reviewed changes

@lsm lsm left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review by glm-5-turbo (GLM)

Model: glm-5-turbo | Client: NeoKai | Provider: GLM

All 4 issues from the first review have been addressed cleanly. The fix commit (9ece4df) introduces no regressions and the implementation is correct.

Issue-by-issue verification

P1 - CommandAutocomplete touch support: Resolved.
The touch-outside-to-close pattern now matches ReferenceAutocomplete exactly: same mousedown + touchend shared handler (line 41-52 of CommandAutocomplete.tsx), same contains check, same cleanup in the effect return. The mobile-specific adaptations are also consistent: py-3 touch targets (line 102), active:bg-dark-700/70 feedback (line 103), adaptive scrollIntoView behavior (line 34), full-width on mobile (line 72), and the conditional footer with "Tap to select" (lines 114-122). The two new test cases (touchend outside/inside, lines 242-275 of the test file) confirm the behavior.

P2 - replaceActiveAtQuery extraction and tests: Resolved.
The utility is exported from MessageInput.tsx (lines 43-53) as a pure function with a clear JSDoc. The regex /((?:^|\s))@(\S*)$/ is well-documented with comments explaining each group. The 11 unit tests in MessageInput.replaceActiveAtQuery.test.ts cover all key edge cases: bare @, query after text, prefix preservation, only-last-active-query semantics, no-match passthrough, empty content, trailing space guarantee, paths with slashes, and different reference types. All pass.

P2 - Regex alignment: Resolved.
The replacement regex /((?:^|\s))@(\S*)$/ in replaceActiveAtQuery now matches the logic in extractActiveAtQuery (which iterates backwards looking for @ preceded by whitespace/start-of-string with no trailing whitespace). I verified the alignment manually against several edge cases (@@, email@example.com, @done fix @new, @foo bar @baz qux) and both functions agree on all of them.

P3 - isTouchDevice cached in useRef: Resolved.
The detection at MessageInput.tsx line 104-107 uses useRef with a comment explaining the rationale: using a ref (not a module constant) allows tests to mock matchMedia before render. The ref value is accessed via .current in the handleKeyDown callback (line 281) and correctly excluded from the dependency array (line 287) since refs are stable.

Pre-existing failures

7 tests fail in useSessionActions.test.ts due to a missing resetSessionQuery mock -- these are pre-existing (30 failures at the original commit, down to 7 at HEAD) and unrelated to this PR.

No regressions detected

All 42 tests in the two directly affected test files pass. The 5571 other tests that pass are unchanged.

Recommendation: APPROVE

lsm added 2 commits March 24, 2026 22:22
@lsm
feat(web): mobile UX, touch support, and full autocomplete integratio…
5af8780 
…n for @ references

- ReferenceAutocomplete: detect touch device (pointer:coarse), show full-width menu
  on mobile, larger touch targets (py-3), touchend outside-click to close, adaptive
  scroll behavior (auto vs smooth), and mobile footer showing "Tap to select" instead
  of keyboard hints
- InputTextarea: add reference autocomplete props and render ReferenceAutocomplete
  alongside CommandAutocomplete
- MessageInput: wire up useReferenceAutocomplete hook, replace @query with
  @ref{type:id} on selection, reference autocomplete takes keyboard precedence
- Tests: touchend close, reference autocomplete rendering in InputTextarea,
  hook mock for queue-mode tests
@lsm
fix(web): address review feedback for mobile UX autocomplete PR
09199f3 
- P1: Add touchend close support to CommandAutocomplete, matching the pattern
  already applied to ReferenceAutocomplete (full-width mobile, py-3 touch
  targets, adaptive scroll, "Tap to select" footer hint)
- P2: Extract replaceActiveAtQuery() as exported utility from MessageInput and
  add 11 unit tests covering all replacement cases (@query, prefix preservation,
  last-active-only, empty content, trailing space)
- P2: handleReferenceInsert now delegates to replaceActiveAtQuery, making both
  uses of the replacement logic share one implementation
- P3: Cache isTouchDevice detection per component instance via useRef (computed
  on first render, not on every keydown) — uses useRef instead of module-level
  constant so existing tests that mock matchMedia before render continue to work
@lsm
lsm force-pushed the task/mobile-ux-and-touch-support branch from 9ece4df to 09199f3 Compare March 25, 2026 02:24
@lsm
lsm changed the base branch from main to dev March 25, 2026 02:24
@lsm
lsm merged commit a0a757c into dev Mar 25, 2026
32 of 64 checks passed
This was referenced Mar 25, 2026
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lsm