-
Notifications
You must be signed in to change notification settings - Fork 9
RG-T117 Chat fix #126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RG-T117 Chat fix #126
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { Check, Search, Users } from 'lucide-react-native'; | ||
| import { Check, Search, Truck, Users } from 'lucide-react-native'; | ||
| import React, { useCallback, useEffect, useMemo, useState } from 'react'; | ||
| import { useTranslation } from 'react-i18next'; | ||
|
|
||
|
|
@@ -34,8 +34,15 @@ function recipientUserId(recipient: RecipientsResultData): string { | |
| } | ||
|
|
||
| function isPersonRecipient(recipient: RecipientsResultData): boolean { | ||
| // Recipients with an empty Type are the server's pseudo-entries | ||
| // ({ Id: "0", Name: "Everyone" } / { Id: "-1", Name: "Nobody" }) — never DM targets. | ||
| const type = (recipient.Type ?? '').toLowerCase(); | ||
| return type === 'personnel' || type === 'person' || type === 'user' || type === 'p' || type === ''; | ||
| return type === 'personnel' || type === 'person' || type === 'user' || type === 'p'; | ||
| } | ||
|
|
||
| function isUnitRecipient(recipient: RecipientsResultData): boolean { | ||
| const type = (recipient.Type ?? '').toLowerCase(); | ||
| return type === 'unit' || type === 'units' || type === 'u'; | ||
| } | ||
|
|
||
| export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewConversationSheetProps) { | ||
|
|
@@ -56,10 +63,13 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo | |
| setQuery(''); | ||
| setLoadError(false); | ||
| setLoading(true); | ||
| getRecipients(true, false) | ||
| // DM mode also offers units (Dispatch can open a 1:1 with a unit); | ||
| // group membership only supports users, so group mode stays people-only. | ||
| const includeUnits = mode === 'dm'; | ||
| getRecipients(true, includeUnits) | ||
| .then((result) => { | ||
| if (cancelled) return; | ||
| setRecipients((result.Data ?? []).filter(isPersonRecipient)); | ||
| setRecipients((result.Data ?? []).filter((r) => isPersonRecipient(r) || (includeUnits && isUnitRecipient(r)))); | ||
| }) | ||
| .catch((error) => { | ||
| if (cancelled) return; | ||
|
|
@@ -73,7 +83,7 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo | |
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [isOpen]); | ||
| }, [isOpen, mode]); | ||
|
|
||
| const filtered = useMemo(() => { | ||
| const q = query.trim().toLowerCase(); | ||
|
|
@@ -94,7 +104,8 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo | |
| async (recipient: RecipientsResultData) => { | ||
| setSubmitting(true); | ||
| try { | ||
| const response = await createDirectMessage({ TargetUserId: recipientUserId(recipient) }); | ||
| const targetId = recipientUserId(recipient); | ||
| const response = await createDirectMessage(isUnitRecipient(recipient) ? { TargetUnitId: parseInt(targetId, 10) } : { TargetUserId: targetId }); | ||
|
Comment on lines
+107
to
+108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
rg --glob '!node_modules/**' --glob '!dist/**' -n -C 8 \
'TargetUnitId|CreateDirectMessageInput|recipientUserId|RecipientsResultData|getRecipients' .Repository: Resgrid/Dispatch Length of output: 17490 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- relevant implementation ---'
sed -n '1,125p' src/components/chat/new-conversation-sheet.tsx
printf '%s\n' '--- recipient model and API types ---'
cat src/models/v4/messages/recipientsResultData.ts
sed -n '1,90p' src/models/v4/chat/chatInputs.ts
printf '%s\n' '--- repository references to recipient ID formats and unit targets ---'
rg --glob '!node_modules/**' --glob '!dist/**' -n -C 3 \
'includeUnits|RecipientsResultData|TargetUnitId|CreateDirectMessage|GetRecipients|Id: "[^"]*[:][^"]*"' .Repository: Resgrid/Dispatch Length of output: 15783 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
node - <<'JS'
function recipientUserId(id) {
return id.includes(':') ? (id.split(':').pop() ?? id) : id;
}
const cases = ['U:12', 'U:12abc', 'U:12.5', 'U:', 'U:abc', '12', ' 12 '];
for (const raw of cases) {
const targetId = recipientUserId(raw);
const parsed = Number.parseInt(targetId, 10);
const isDecimalInteger = /^[0-9]+$/.test(targetId);
console.log(JSON.stringify({ raw, targetId, parsed, isDecimalInteger }));
}
JS
printf '%s\n' '--- repository origin ---'
git remote get-url origin 2>/dev/null || trueRepository: Resgrid/Dispatch Length of output: 706 Validate the unit ID before building the request.
🤖 Prompt for AI Agents |
||
| if (response.Data?.ChatChannelId) { | ||
| onCreated(response.Data.ChatChannelId); | ||
| onClose(); | ||
|
|
@@ -110,9 +121,10 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo | |
| ); | ||
|
|
||
| const createGroup = useCallback(async () => { | ||
| if (!groupName.trim() || selected.size === 0) return; | ||
| if (selected.size === 0) return; | ||
| setSubmitting(true); | ||
| try { | ||
| // Name is optional — the server auto-names the group after its members. | ||
| const response = await createAdHocChannel({ Name: groupName.trim(), MemberUserIds: Array.from(selected) }); | ||
|
Comment on lines
+124
to
128
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WHAT: createGroup now sends // Make Name optional in CreateAdHocChannelInput and only send it when provided:
const input: CreateAdHocChannelInput = { MemberUserIds: Array.from(selected) };
const name = groupName.trim();
if (name) input.Name = name;
const response = await createAdHocChannel(input);Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| if (response.Data?.ChatChannelId) { | ||
| onCreated(response.Data.ChatChannelId); | ||
|
|
@@ -139,7 +151,7 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo | |
|
|
||
| {mode === 'group' ? ( | ||
| <Input> | ||
| <InputField placeholder={t('chat.group_name')} value={groupName} onChangeText={setGroupName} /> | ||
| <InputField placeholder={t('chat.group_name_optional')} value={groupName} onChangeText={setGroupName} /> | ||
| </Input> | ||
| ) : null} | ||
|
|
||
|
|
@@ -167,16 +179,28 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo | |
| {filtered.map((recipient) => { | ||
| const userId = recipientUserId(recipient); | ||
| const isSelected = selected.has(userId); | ||
| const isUnit = isUnitRecipient(recipient); | ||
| return ( | ||
| <Pressable key={recipient.Id} className="py-2" onPress={() => (mode === 'dm' ? startDirectMessage(recipient) : toggle(userId))} disabled={submitting}> | ||
| <HStack className="items-center justify-between"> | ||
| <HStack className="flex-1 items-center" space="sm"> | ||
| <Avatar size="sm"> | ||
| <AvatarImage source={{ uri: getAvatarUrl(userId) }} /> | ||
| </Avatar> | ||
| {isUnit ? ( | ||
| <Center className="size-8 rounded-full bg-secondary-200"> | ||
| <Truck size={16} color="#6b7280" /> | ||
| </Center> | ||
| ) : ( | ||
| <Avatar size="sm"> | ||
| <AvatarImage source={{ uri: getAvatarUrl(userId) }} /> | ||
| </Avatar> | ||
| )} | ||
| <Text className="flex-1 text-typography-900" numberOfLines={1}> | ||
| {recipient.Name} | ||
| </Text> | ||
| {isUnit ? ( | ||
| <Box className="rounded-full bg-secondary-200 px-2 py-0.5"> | ||
| <Text className="text-xs text-typography-600">{t('chat.unit')}</Text> | ||
| </Box> | ||
| ) : null} | ||
| </HStack> | ||
| {mode === 'group' && isSelected ? ( | ||
| <Box className="rounded-full bg-primary-600 p-1"> | ||
|
|
@@ -191,7 +215,7 @@ export function NewConversationSheet({ isOpen, onClose, mode, onCreated }: NewCo | |
| )} | ||
|
|
||
| {mode === 'group' ? ( | ||
| <Button className="mb-2 w-full bg-primary-600" onPress={createGroup} isDisabled={submitting || !groupName.trim() || selected.size === 0}> | ||
| <Button className="mb-2 w-full bg-primary-600" onPress={createGroup} isDisabled={submitting || selected.size === 0}> | ||
| <Users size={18} color="#ffffff" /> | ||
| <ButtonText className="ml-2">{t('chat.create_group_with', { count: selected.size })}</ButtonText> | ||
| </Button> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| * JoinChannel(string channelId, int? asUnitId) | ||
| * Typing(string channelId, string displayName, bool isTyping, int? asUnitId) | ||
| * MarkRead(string channelId, long seq, int? asUnitId) | ||
| * SetActiveChannel(string channelId, int? asUnitId) | ||
| */ | ||
| const mockInvoke = jest.fn().mockResolvedValue(undefined); | ||
|
|
||
|
|
@@ -71,6 +72,15 @@ describe('chat hub invocations', () => { | |
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'JoinChannel', 'channel-1', null); | ||
| }); | ||
|
|
||
| it('sends both SetActiveChannel arguments, with null clearing the marker', () => { | ||
| useChatStore.getState().setActiveChannel('channel-1'); | ||
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'SetActiveChannel', 'channel-1', null); | ||
|
|
||
| mockInvoke.mockClear(); | ||
| useChatStore.getState().setActiveChannel(null); | ||
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'SetActiveChannel', null, null); | ||
| }); | ||
|
|
||
| it('sends all four Typing arguments in hub order', () => { | ||
| useChatStore.getState().sendTyping('channel-1', true); | ||
|
|
||
|
|
@@ -110,6 +120,55 @@ describe('chat hub invocations', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('active-channel marker resynchronization', () => { | ||
| // syncActiveChannelMarker settles on the microtask queue; two ticks drain it. | ||
| const flush = async () => { | ||
| await Promise.resolve(); | ||
| await Promise.resolve(); | ||
| }; | ||
|
|
||
| beforeEach(async () => { | ||
| mockInvoke.mockClear(); | ||
| mockInvoke.mockResolvedValue(undefined); | ||
| useChatStore.getState().reset(); | ||
| await flush(); | ||
| mockInvoke.mockClear(); | ||
| }); | ||
|
|
||
| it('re-asserts a non-null marker on reconnect', async () => { | ||
| useChatStore.getState().setActiveChannel('channel-1'); | ||
| await flush(); | ||
| mockInvoke.mockClear(); | ||
|
|
||
| useChatStore.getState().handleChatConnected(); | ||
|
|
||
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'SetActiveChannel', 'channel-1', null); | ||
| }); | ||
|
|
||
| it('retries a null marker that failed to send once reconnected', async () => { | ||
| mockInvoke.mockRejectedValue(new Error('disconnected')); | ||
| useChatStore.getState().setActiveChannel(null); | ||
| await flush(); | ||
| mockInvoke.mockClear(); | ||
| mockInvoke.mockResolvedValue(undefined); | ||
|
|
||
| useChatStore.getState().handleChatConnected(); | ||
|
|
||
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'SetActiveChannel', null, null); | ||
| }); | ||
|
|
||
| it('does not resend a null marker the hub already confirmed', async () => { | ||
| useChatStore.getState().setActiveChannel(null); | ||
| await flush(); | ||
| mockInvoke.mockClear(); | ||
|
|
||
| useChatStore.getState().handleChatConnected(); | ||
| await flush(); | ||
|
|
||
| expect(mockInvoke).not.toHaveBeenCalledWith('chatHub', 'SetActiveChannel', expect.anything(), expect.anything()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Verify Jest documents that expect.anything() excludes null and undefined.
curl -fsSL https://jestjs.io/docs/expect |
rg -i -C 2 'anything.*null|null.*anything'Repository: Resgrid/Dispatch Length of output: 37902 🏁 Script executed: #!/bin/bash
set -euo pipefail
file="src/stores/chat/__tests__/hub-invoke-args.test.ts"
printf '%s\n' '--- test context ---'
cat -n "$file" | sed -n '130,185p'
printf '%s\n' '--- related symbols and calls ---'
rg -n -C 3 'handleChatConnected|SetActiveChannel|mockInvoke' src/stores/chatRepository: Resgrid/Dispatch Length of output: 12193 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- package and Jest configuration ---'
rg -n -C 2 '"jest"|jest.config|testMatch|testRegex' package.json '*jest*' 2>/dev/null || true
printf '%s\n' '--- exact test structure ---'
cat -n src/stores/chat/__tests__/hub-invoke-args.test.ts | sed -n '1,210p'
printf '%s\n' '--- handler definitions and call sites ---'
rg -n -C 8 'handleChatConnected|SetActiveChannel|mockInvoke' src --glob '*.{ts,tsx}'Repository: Resgrid/Dispatch Length of output: 25762 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("src/stores/chat/__tests__/hub-invoke-args.test.ts")
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if "SetActiveChannel" in line or "handleChatConnected" in line:
print(f"{i}: {line}")
PY
printf '%s\n' '--- tracked candidate files ---'
git ls-files | rg '(^|/)(chat|hub).*|src/stores/chat' | head -80Repository: Resgrid/Dispatch Length of output: 1896 🏁 Script executed: #!/bin/bash
set -euo pipefail
file="src/stores/chat/__tests__/hub-invoke-args.test.ts"
cat -n "$file" | sed -n '130,185p'
printf '\n--- related code ---\n'
rg -n -C 8 'handleChatConnected|SetActiveChannel|mockInvoke' src/stores/chatRepository: Resgrid/Dispatch Length of output: 17346 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete reconnect handler ---'
cat -n src/stores/chat/store.ts | sed -n '860,890p'
printf '%s\n' '--- matcher behavior for the calls relevant to this test ---'
node - <<'JS'
const anything = value => value !== null && value !== undefined;
const calls = [
['chatHub', 'SetActiveChannel', null, null],
['chatHub', 'SetActiveChannel', 'channel-1', null],
];
const matchesOriginal = call =>
call[0] === 'chatHub' &&
call[1] === 'SetActiveChannel' &&
anything(call[2]) &&
anything(call[3]);
const matchesNullFix = call =>
call[0] === 'chatHub' &&
call[1] === 'SetActiveChannel' &&
call[2] === null &&
call[3] === null;
for (const call of calls) {
console.log(JSON.stringify(call), {
originalNot: !matchesOriginal(call),
nullFixNot: !matchesNullFix(call),
});
}
JSRepository: Resgrid/Dispatch Length of output: 1791 Assert that the null marker is not resent.
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| }); | ||
| }); | ||
|
|
||
| describe('incoming message normalization', () => { | ||
| beforeEach(() => { | ||
| useChatStore.setState({ messagesByChannel: {}, channels: [] }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use recipient-neutral copy in direct-message mode.
When
includeUnitsis true, the list contains people and units. The component still useschat.search_people,chat.no_people, andchat.load_people_failed. A unit-only result then shows incorrect text. Add recipient-neutral keys or select mode-specific keys, and add them to every locale.🤖 Prompt for AI Agents