Conversation
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThe change permits group creation without a name, updates localized labels, synchronizes active channels with the Chat hub across reconnects, and adds Jest mocks for native and navigation dependencies. ChangesChat updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChatStore
participant ChatHub
participant ChatConnection
ChatStore->>ChatHub: SetActiveChannel(channelId, unitId)
ChatConnection->>ChatStore: reconnect notification
ChatStore->>ChatHub: reassert active-channel marker
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/stores/chat/store.ts (1)
860-861: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWait for
joinChannelbefore invokingSetActiveChannel
handleChatConnectedinvokes both operations concurrently. ChainSetActiveChannelafterjoinChannelso marker execution does not depend on the hub'sMaximumParallelInvocationsPerClientsetting. Add a regression test that keepsJoinChannelpending.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/chat/store.ts` around lines 860 - 861, Update handleChatConnected to await the joinChannel operation before invoking SetActiveChannel, rather than starting both concurrently; preserve the active-channel and unit arguments for SetActiveChannel. Add a regression test that keeps JoinChannel pending and verifies SetActiveChannel is not invoked until JoinChannel completes.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jest-setup.ts`:
- Around line 8-12: Replace the any-typed props in SafeAreaView and
SafeAreaProvider with a shared MockChildrenProps interface defining optional
children as ReactNode, and use that interface for both mock component
parameters.
- Around line 5-18: Add SafeAreaInsetsContext to the
react-native-safe-area-context mock in jest.mock, exposing a Consumer compatible
with the zero-inset mock values so Expo Router can render safely. Preserve the
existing SafeAreaView, SafeAreaProvider, hook, and initialWindowMetrics
behavior.
In `@src/hooks/__tests__/use-signalr-lifecycle.test.tsx`:
- Around line 20-21: Update the mockConnectChatHub and mockDisconnectChatHub
Jest mocks to use mockResolvedValue(undefined), preserving the production
Promise<void> contract and enabling async completion or rejection tests.
---
Nitpick comments:
In `@src/stores/chat/store.ts`:
- Around line 860-861: Update handleChatConnected to await the joinChannel
operation before invoking SetActiveChannel, rather than starting both
concurrently; preserve the active-channel and unit arguments for
SetActiveChannel. Add a regression test that keeps JoinChannel pending and
verifies SetActiveChannel is not invoked until JoinChannel completes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f37a2984-ef0c-47e9-b23a-3c775d5a35cc
📒 Files selected for processing (16)
jest-setup.tssrc/components/chat/new-conversation-sheet.tsxsrc/hooks/__tests__/use-signalr-lifecycle.test.tsxsrc/lib/__tests__/navigation.test.tssrc/services/__tests__/push-notification.test.tssrc/stores/chat/__tests__/hub-invoke-args.test.tssrc/stores/chat/store.tssrc/translations/ar.jsonsrc/translations/de.jsonsrc/translations/en.jsonsrc/translations/es.jsonsrc/translations/fr.jsonsrc/translations/it.jsonsrc/translations/pl.jsonsrc/translations/sv.jsonsrc/translations/uk.json
| 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 }, | ||
| }, |
There was a problem hiding this comment.
🩺 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 SafeAreaInsetsContext to the global safe-area mock. Expo Router accesses SafeAreaInsetsContext.Consumer; the mock currently returns undefined, which can fail navigation suites during rendering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jest-setup.ts` around lines 5 - 18, Add SafeAreaInsetsContext to the
react-native-safe-area-context mock in jest.mock, exposing a Consumer compatible
with the zero-inset mock values so Expo Router can render safely. Preserve the
existing SafeAreaView, SafeAreaProvider, hook, and initialWindowMetrics
behavior.
| const SafeAreaView = ({ children }: any) => React.createElement(React.Fragment, null, children); | ||
|
|
||
| return { | ||
| SafeAreaView, | ||
| SafeAreaProvider: ({ children }: any) => children, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a typed interface for mock children.
The two mock components use any. This removes type checking for children. Define one MockChildrenProps interface with children?: ReactNode and use it for both components.
As per coding guidelines, **/*.{ts,tsx} requires precise types and interfaces and forbids any.
Proposed fix
+import type { ReactNode } from 'react';
+
+interface MockChildrenProps {
+ children?: ReactNode;
+}
+
const SafeAreaView = ({ children }: any) => React.createElement(React.Fragment, null, children);
+const SafeAreaView = ({ children }: MockChildrenProps) => React.createElement(React.Fragment, null, children);
- SafeAreaProvider: ({ children }: any) => children,
+ SafeAreaProvider: ({ children }: MockChildrenProps) => children,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jest-setup.ts` around lines 8 - 12, Replace the any-typed props in
SafeAreaView and SafeAreaProvider with a shared MockChildrenProps interface
defining optional children as ReactNode, and use that interface for both mock
component parameters.
Source: Coding guidelines
| const mockConnectChatHub = jest.fn(); | ||
| const mockDisconnectChatHub = jest.fn(); |
There was a problem hiding this comment.
🎯 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 undefined values with Promise.allSettled, so existing tests do not fail. However, the production methods return Promise<void>. Use jest.fn().mockResolvedValue(undefined) to preserve the async contract and support completion or rejection tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/__tests__/use-signalr-lifecycle.test.tsx` around lines 20 - 21,
Update the mockConnectChatHub and mockDisconnectChatHub Jest mocks to use
mockResolvedValue(undefined), preserving the production Promise<void> contract
and enabling async completion or rejection tests.
| // 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 safeInvoke('SetActiveChannel', channelId ?? null, activeUnitIdNumber() ?? null); |
There was a problem hiding this comment.
WHAT: The void prefix discards the return value of safeInvoke, which (given the void) likely returns a Promise whose rejections are now unhandled. WHY: Rule [1] requires every async operation be guarded; a fire-and-forget void with no .catch() leaves rejections unhandled and can crash the app or hide failures. HOW: Either await inside a try/catch (if the caller can be async) or chain .catch(err => logger.error('SetActiveChannel failed', { channelId, err })) before discarding, or ensure safeInvoke itself swallows/logs internally and document that.
Also found in:
src/stores/chat/store.ts:861-861
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/stores/chat/store.ts:
Line 286:
WHAT: The `void` prefix discards the return value of `safeInvoke`, which (given the `void`) likely returns a Promise whose rejections are now unhandled. WHY: Rule [1] requires every async operation be guarded; a fire-and-forget `void` with no `.catch()` leaves rejections unhandled and can crash the app or hide failures. HOW: Either `await` inside a try/catch (if the caller can be async) or chain `.catch(err => logger.error('SetActiveChannel failed', { channelId, err }))` before discarding, or ensure `safeInvoke` itself swallows/logs internally and document that.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/stores/chat/__tests__/hub-invoke-args.test.ts`:
- Line 175: Update the SetActiveChannel assertion in the hub invocation test to
explicitly match null and 42 instead of using broad expect.anything() matchers,
ensuring the not-called check covers the null marker case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8604c708-742f-46e8-bde5-ed5442309806
📒 Files selected for processing (2)
src/stores/chat/__tests__/hub-invoke-args.test.tssrc/stores/chat/store.ts
| useChatStore.getState().handleChatConnected(); | ||
| await flush(); | ||
|
|
||
| expect(mockInvoke).not.toHaveBeenCalledWith('chatHub', 'SetActiveChannel', expect.anything(), expect.anything()); |
There was a problem hiding this comment.
🎯 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:
Jest expect.anything null undefined toHaveBeenCalledWith documentation
💡 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:
- 1: https://jestjs.io/docs/expect
- 2: https://jestjs.io/docs/next/expect
- 3: https://stackoverflow.com/questions/47027011/using-expect-anyobject-or-expect-anything-does-not-work-to-match-undefi
- 4: toHaveBeenCalledWith indifferent to parameters that have
undefinedornullas a value jestjs/jest#8386 - 5: [Bug]:
toBeCalledWithincorrectly succeeds when matching{}withfoo: undefinedjestjs/jest#12095 - 6: toHaveBeenCalledWith indifferent to parameters that have
undefinedornullas a value jestjs/jest#8386 - 7: [Bug]: toHaveBeenCalledWith does not differentiate [undefined] and [] arguments jestjs/jest#15446
- 8: https://github.com/jestjs/jest/blob/main/docs/ExpectAPI.md
Match the null marker explicitly
expect.anything() excludes null, so this assertion passes when SetActiveChannel(null, 42) is called. Replace the broad matcher with null and 42.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/chat/__tests__/hub-invoke-args.test.ts` at line 175, Update the
SetActiveChannel assertion in the hub invocation test to explicitly match null
and 42 instead of using broad expect.anything() matchers, ensuring the
not-called check covers the null marker case.
Source: Coding guidelines
| // 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.
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 LLM
File src/stores/chat/store.ts:
Line 261:
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.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
PR Description
This pull request addresses chat-related fixes and improvements (RG-T117) with the following changes:
Push Notification Suppression for Active Chat Channel
The chat store now notifies the SignalR hub when a channel becomes active (or is cleared) via a new
SetActiveChannelinvocation. This allows the server to suppress chat push notifications for the conversation currently displayed on screen, preventing duplicate notifications for messages the user is already viewing. The active-channel marker is also re-asserted after a hub reconnection to ensure push suppression survives connection drops.Optional Group Name for New Conversations
When creating a new group conversation, a group name is no longer required. The submit button and validation now only require at least one member to be selected. When no name is provided, the server auto-names the group based on its members.
Test Infrastructure Improvements
react-native-safe-area-contextin the global Jest setup to prevent crashes in suites that use minimal React Native stubs.expo-routerand@/lib/navigationin navigation and push notification tests to avoid import chain failures.SetActiveChannelbehavior.Summary by CodeRabbit
New Features
Bug Fixes
Tests