-
Notifications
You must be signed in to change notification settings - Fork 6
RG-T117 Unit chat fix #266
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
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,5 +1,24 @@ | ||
| import '@testing-library/react-native/extend-expect'; | ||
|
|
||
| // Mock react-native-safe-area-context — its source build reads StyleSheet at import time, | ||
| // which explodes in suites that stub react-native with a minimal factory (e.g. navigation tests). | ||
| jest.mock('react-native-safe-area-context', () => { | ||
| const React = require('react'); | ||
|
|
||
| const SafeAreaView = ({ children }: any) => React.createElement(React.Fragment, null, children); | ||
|
|
||
| return { | ||
| SafeAreaView, | ||
| SafeAreaProvider: ({ children }: any) => children, | ||
| useSafeAreaInsets: jest.fn(() => ({ top: 0, bottom: 0, left: 0, right: 0 })), | ||
| useSafeAreaFrame: jest.fn(() => ({ x: 0, y: 0, width: 375, height: 667 })), | ||
| initialWindowMetrics: { | ||
| insets: { top: 0, bottom: 0, left: 0, right: 0 }, | ||
| frame: { x: 0, y: 0, width: 375, height: 667 }, | ||
| }, | ||
|
Comment on lines
+5
to
+18
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. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
# Expect: every runtime import is provided by the mock.
rg -n -C 2 "react-native-safe-area-context" \
--glob '*.ts' --glob '*.tsx' --glob '*.js' .Repository: Resgrid/Unit Length of output: 7338 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- jest-setup.ts ---'
cat -n jest-setup.ts | sed -n '1,35p'
printf '%s\n' '--- named imports from react-native-safe-area-context ---'
python3 - <<'PY'
import re
from pathlib import Path
for path in Path('.').rglob('*'):
if not path.is_file() or path.suffix not in {'.ts', '.tsx', '.js'}:
continue
try:
text = path.read_text()
except UnicodeDecodeError:
continue
for match in re.finditer(
r"""(?:import\s+(?:type\s+)?\{([^}]+)\}\s+from|require\()\s*['"]react-native-safe-area-context['"]""",
text,
):
if match.group(1) is not None:
names = [
re.sub(r'\s+', ' ', item.strip())
for item in match.group(1).split(',')
if item.strip()
]
print(f'{path}: {", ".join(names)}')
else:
print(f'{path}: require(...)')
PY
printf '%s\n' '--- module export references in source and tests ---'
rg -n -C 1 \
"SafeArea(View|Provider)|useSafeArea(Insets|Frame)|initialWindowMetrics|SafeAreaListener|useSafeArea" \
--glob '*.ts' --glob '*.tsx' --glob '*.js' .Repository: Resgrid/Unit Length of output: 20016 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- non-type safe-area imports outside the repository source ---'
rg -n -C 2 \
"from ['\"]react-native-safe-area-context['\"]|require\(['\"]react-native-safe-area-context['\"]\)" \
node_modules \
--glob '!**/*.d.ts' \
--glob '!**/example/**' \
| sed -n '1,240p'
printf '%s\n' '--- package resolution and Jest configuration ---'
rg -n -C 3 \
"jest-setup|setupFiles|setupFilesAfterEnv|react-native-screens|expo-router|jest-expo" \
package.json jest.config.* app.json app.config.* \
--glob '!node_modules/**' 2>/dev/null || trueRepository: Resgrid/Unit Length of output: 50369 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
pattern = re.compile(
r"""(?:import\s+(?:(?:type\s+)?\{([^}]+)\}|([^;\n]+?))\s+from|require\()\s*['"]react-native-safe-area-context['"]"""
)
seen = set()
for path in Path('node_modules').rglob('*'):
if not path.is_file() or path.suffix in {'.map', '.d.ts'}:
continue
try:
text = path.read_text(errors='ignore')
except OSError:
continue
for line_no, line in enumerate(text.splitlines(), 1):
if 'react-native-safe-area-context' not in line:
continue
match = pattern.search(line)
if match:
names = (match.group(1) or match.group(2) or 'require(...)').strip()
item = (str(path), line_no, names, line.strip())
if item not in seen:
seen.add(item)
print(f'{item[0]}:{item[1]}: {item[3]}')
PY
printf '%s\n' '--- Jest configuration ---'
for file in package.json jest.config.js jest.config.ts jest.config.cjs jest.config.mjs; do
if [ -f "$file" ]; then
printf '%s\n' "--- $file ---"
rg -n -C 4 \
"jest-setup|setupFiles|setupFilesAfterEnv|preset|react-native-screens|expo-router" \
"$file" || true
fi
doneRepository: Resgrid/Unit Length of output: 8749 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import re
from pathlib import Path
from collections import defaultdict
uses = defaultdict(set)
for path in Path('node_modules').rglob('*'):
if not path.is_file() or path.suffix in {'.map', '.d.ts'}:
continue
if 'example' in path.parts:
continue
try:
text = path.read_text(errors='ignore')
except OSError:
continue
if 'react-native-safe-area-context' not in text:
continue
for name in re.findall(r'_reactNativeSafeAreaContext\.([A-Za-z_$][\w$]*)', text):
uses[str(path)].add(name)
for line in text.splitlines():
if 'react-native-safe-area-context' in line and (
'import ' in line or 'require(' in line
):
uses[str(path)].add(f'IMPORT: {line.strip()}')
for path, names in sorted(uses.items()):
print(path)
for name in sorted(names):
print(f' {name}')
PY
printf '%s\n' '--- direct references to missing candidate exports ---'
rg -n \
"SafeAreaInsetsContext|SafeAreaFrameContext|useSafeAreaFrame|initialWindowMetrics|SafeAreaProvider|SafeAreaView|useSafeAreaInsets" \
node_modules/expo-router/build node_modules/react-native-css/dist node_modules/@gluestack-ui/core/lib node_modules/react-native-screens/lib \
--glob '!**/*.map' \
--glob '!**/*.d.ts' \
| sed -n '1,240p'Repository: Resgrid/Unit Length of output: 25314 Add 🤖 Prompt for AI Agents |
||
| }; | ||
| }); | ||
|
|
||
| // Mock @sentry/react-native — native module (RNSentry) is unavailable in Jest | ||
| jest.mock('@sentry/react-native', () => ({ | ||
| captureException: jest.fn(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,8 @@ describe('useSignalRLifecycle', () => { | |
| const mockDisconnectUpdateHub = jest.fn(); | ||
| const mockConnectGeolocationHub = jest.fn(); | ||
| const mockDisconnectGeolocationHub = jest.fn(); | ||
| const mockConnectChatHub = jest.fn(); | ||
| const mockDisconnectChatHub = jest.fn(); | ||
|
Comment on lines
+20
to
+21
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: #!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 '\b(connectChatHub|disconnectChatHub)\b' src/hooks src/storesRepository: Resgrid/Unit Length of output: 13383 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- test file ---'
cat -n src/hooks/__tests__/use-signalr-lifecycle.test.tsx | sed -n '1,190p'
printf '%s\n' '--- hook implementation ---'
cat -n src/hooks/use-signalr-lifecycle.ts | sed -n '1,155p'
printf '%s\n' '--- relevant test assertions ---'
rg -n -C 4 'mockConnectChatHub|mockDisconnectChatHub|Promise\.allSettled|reject|resolve|connectChatHub|disconnectChatHub' \
src/hooks/__tests__/use-signalr-lifecycle.test.tsxRepository: Resgrid/Unit Length of output: 16623 🏁 Script executed: node - <<'JS'
const plainMockResult = undefined;
const promiseMockResult = Promise.resolve(undefined);
Promise.allSettled([plainMockResult, promiseMockResult]).then((results) => {
console.log(JSON.stringify(results));
});
JSRepository: Resgrid/Unit Length of output: 198 Make the Chat hub mocks Promise-compatible. The hook currently handles plain 🤖 Prompt for AI Agents |
||
|
|
||
| // Create shared state for app lifecycle that can be updated | ||
| let appLifecycleState = { | ||
|
|
@@ -48,15 +50,21 @@ describe('useSignalRLifecycle', () => { | |
| disconnectUpdateHub: mockDisconnectUpdateHub, | ||
| connectGeolocationHub: mockConnectGeolocationHub, | ||
| disconnectGeolocationHub: mockDisconnectGeolocationHub, | ||
| connectChatHub: mockConnectChatHub, | ||
| disconnectChatHub: mockDisconnectChatHub, | ||
| isUpdateHubConnected: false, | ||
| isGeolocationHubConnected: false, | ||
| isChatHubConnected: false, | ||
| } as any) : { | ||
| connectUpdateHub: mockConnectUpdateHub, | ||
| disconnectUpdateHub: mockDisconnectUpdateHub, | ||
| connectGeolocationHub: mockConnectGeolocationHub, | ||
| disconnectGeolocationHub: mockDisconnectGeolocationHub, | ||
| connectChatHub: mockConnectChatHub, | ||
| disconnectChatHub: mockDisconnectChatHub, | ||
| isUpdateHubConnected: false, | ||
| isGeolocationHubConnected: false, | ||
| isChatHubConnected: false, | ||
| } as any); | ||
|
|
||
| // Also mock getState for direct store access | ||
|
|
@@ -65,8 +73,11 @@ describe('useSignalRLifecycle', () => { | |
| disconnectUpdateHub: mockDisconnectUpdateHub, | ||
| connectGeolocationHub: mockConnectGeolocationHub, | ||
| disconnectGeolocationHub: mockDisconnectGeolocationHub, | ||
| connectChatHub: mockConnectChatHub, | ||
| disconnectChatHub: mockDisconnectChatHub, | ||
| isUpdateHubConnected: false, | ||
| isGeolocationHubConnected: false, | ||
| isChatHubConnected: false, | ||
| }); | ||
|
|
||
| // Mock useAppLifecycle to return shared state | ||
|
|
||
| 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); | ||
|
|
||
|
|
@@ -75,6 +76,18 @@ describe('chat hub invocations', () => { | |
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'JoinChannel', 'channel-1', 42); | ||
| }); | ||
|
|
||
| it('sends both SetActiveChannel arguments', () => { | ||
| useChatStore.getState().setActiveChannel('channel-1'); | ||
|
|
||
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'SetActiveChannel', 'channel-1', 42); | ||
| }); | ||
|
|
||
| it('clears the active channel marker with a null channelId and the unit id', () => { | ||
| useChatStore.getState().setActiveChannel(null); | ||
|
|
||
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'SetActiveChannel', null, 42); | ||
| }); | ||
|
|
||
| it('sends all four Typing arguments in hub order', () => { | ||
| useChatStore.getState().sendTyping('channel-1', true); | ||
|
|
||
|
|
@@ -114,6 +127,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', 42); | ||
| }); | ||
|
|
||
| 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, 42); | ||
| }); | ||
|
|
||
| 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()); | ||
|
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 -e
file="src/stores/chat/__tests__/hub-invoke-args.test.ts"
printf '%s\n' "== file context =="
sed -n '130,185p' "$file"
printf '%s\n' "== relevant symbols and calls =="
rg -n -C 3 "SetActiveChannel|mockInvoke|expect\\.anything|reconnect|disconnect|activeChannel" "$file" src/stores/chat src --glob '*.ts' --glob '*.tsx' | head -n 240
printf '%s\n' "== Jest matcher probe =="
node - <<'JS'
const matchers = require('expect/build/matchers');
const equals = require('`@jest/expect-utils`').equals;
const matcher = matchers.toHaveBeenCalledWith;
function check(actualArgs, expectedArgs) {
const result = matcher.call({ equals, isNot: false, promise: '' }, { mock: { calls: [actualArgs] } }, expectedArgs);
return result.pass;
}
for (const args of [[null, 42], ['channel', 42], [undefined, 42]]) {
console.log(JSON.stringify(args), {
negativeAnything: !check(args, ['chatHub', 'SetActiveChannel', expectAnything(), expectAnything()]),
negativeNull42: !check(args, ['chatHub', 'SetActiveChannel', null, 42]),
});
}
function expectAnything() {
return {
asymmetricMatch(value) {
return value !== null && value !== undefined;
},
toString() { return 'Anything'; },
getExpectedType() { return 'anything'; },
};
}
JSRepository: Resgrid/Unit Length of output: 21777 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' "== active-channel synchronization implementation =="
sed -n '235,330p' src/stores/chat/store.ts
printf '%s\n' "== reconnect handler references =="
rg -n -C 8 "handleChatConnected|pendingActiveChannelSync|syncActiveChannelMarker" src/stores/chat/store.ts src/stores/chat --glob '*.ts' --glob '*.tsx'Repository: Resgrid/Unit Length of output: 22712 🌐 Web query:
💡 Result: In Jest, the asymmetric matcher expect.anything is explicitly designed to match any value except for null or undefined [1][2]. Because null and undefined are considered "nothing" in this context, expect.anything acts as a validator that an argument is present and defined [1][3]. Regarding toHaveBeenCalledWith (and its alias toBeenCalledWith): 1. Null and Undefined Behavior: When using toHaveBeenCalledWith, these matchers utilize equality logic similar to toEqual rather than strict equality (toStrictEqual) [4][5][6]. As a result, certain comparisons may behave in ways that seem non-strict regarding undefined properties or arguments [4][5][7]. 2. Limitations: Because expect.anything specifically excludes null and undefined, it cannot be used to match those values [1][3]. If you need to match a parameter that could be null, undefined, or any other value, you cannot use expect.anything for that position [3]. 3. Intended Usage: The primary purpose of expect.anything is to assert that a mock function was called with a valid, non-null, and non-undefined argument [1][8]. If you find that your assertions are failing or passing unexpectedly due to the handling of null or undefined, consider using toStrictEqual if testing values directly, or creating a custom matcher if you require specific behavior that handles these values differently than the built-in Jest matchers [4][5][3]. Citations:
Match the null marker explicitly
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| }); | ||
| }); | ||
|
|
||
| describe('incoming message normalization', () => { | ||
| beforeEach(() => { | ||
| useChatStore.setState({ messagesByChannel: {}, channels: [] }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -245,6 +245,23 @@ async function safeInvoke(method: string, ...args: unknown[]): Promise<void> { | |
| } | ||
| } | ||
|
|
||
| /** Active-channel marker the hub has not confirmed yet. Kept — null included, since | ||
| * null means "clear the marker" — until an invoke succeeds, so a send that failed | ||
| * while offline can be replayed on reconnect. */ | ||
| let pendingActiveChannelSync: { channelId: string | null } | null = null; | ||
|
|
||
| async function syncActiveChannelMarker(channelId: string | null): Promise<void> { | ||
| const marker = { channelId }; | ||
| pendingActiveChannelSync = marker; | ||
| try { | ||
| await signalRService.invoke(Env.CHAT_HUB_NAME, 'SetActiveChannel', channelId, activeUnitIdNumber() ?? null); | ||
| // Only clear if no newer marker superseded this one while in flight. | ||
| if (pendingActiveChannelSync === marker) pendingActiveChannelSync = null; | ||
| } catch (error) { | ||
| logger.debug({ message: 'chat: invoke SetActiveChannel skipped', context: { error } }); | ||
|
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. Non-compliant error log in the catch block embeds the operation name in the message string and omits relevant identifiers (channelId, unitId). Rule [3] requires error logs to include the operation name and relevant identifiers as structured fields for searchable telemetry; restructure to logger.error('SetActiveChannel failed', { op: 'SetActiveChannel', channelId, error }); or include channelId in the context object. Kody rule violation: Include error context in structured logs Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| } | ||
| } | ||
|
|
||
| export const useChatStore = create<ChatState>()( | ||
| persist( | ||
| (set, get) => ({ | ||
|
|
@@ -280,6 +297,10 @@ export const useChatStore = create<ChatState>()( | |
|
|
||
| setActiveChannel: (channelId: string | null) => { | ||
| set({ activeChannelId: channelId }); | ||
| // Tell the hub which conversation is on screen so it suppresses chat push | ||
| // notifications for that channel — including the unit-device push when the | ||
| // active unit id is supplied. A null channelId clears the marker. | ||
| void syncActiveChannelMarker(channelId ?? null); | ||
| }, | ||
|
|
||
| // ------------------------------------------------------------------ | ||
|
|
@@ -854,6 +875,12 @@ export const useChatStore = create<ChatState>()( | |
| void get().joinChannel(activeChannelId); | ||
| void get().loadNewerMessages(activeChannelId); | ||
| } | ||
| // Re-assert the active-channel marker so push suppression survives reconnects. | ||
| // A pending null (screen closed while offline) is flushed too, so the server | ||
| // stops suppressing push for a channel no longer on screen. | ||
| if (activeChannelId !== null || pendingActiveChannelSync !== null) { | ||
| void syncActiveChannelMarker(activeChannelId); | ||
| } | ||
| }, | ||
|
|
||
| reset: () => { | ||
|
|
@@ -862,6 +889,7 @@ export const useChatStore = create<ChatState>()( | |
| lastTypingSentAt.clear(); | ||
| lastMarkedSeq.clear(); | ||
| pendingChatbotMessages.clear(); | ||
| pendingActiveChannelSync = null; | ||
| clearChatbotTypingTimeout(); | ||
| if (outboxDrainTimer) { | ||
| clearTimeout(outboxDrainTimer); | ||
|
|
||
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a typed interface for mock children.
The two mock components use
any. This removes type checking forchildren. Define oneMockChildrenPropsinterface withchildren?: ReactNodeand use it for both components.As per coding guidelines,
**/*.{ts,tsx}requires precise types and interfaces and forbidsany.Proposed fix
🤖 Prompt for AI Agents
Source: Coding guidelines