Conversation
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThe PR updates chat metadata serialization and parsing, message rendering and scrolling, assistant actions, optional composer attachments, SignalR event delivery, chat realtime handling, session initialization guards, related tests, and Sentry debug logging. ChangesChat UI and metadata
SignalR chat integration
Session initialization lifecycle
Sentry debug configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SignalRHub
participant SignalRService
participant SignalRStore
participant ChatStore
SignalRHub->>SignalRService: Send positional event arguments
SignalRService->>SignalRStore: Forward all arguments
SignalRStore->>ChatStore: Invoke chat handler
ChatStore->>ChatStore: Parse and normalize chat payload
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/components/chat/chat-utils.ts`:
- Around line 83-115: Add Jest coverage in chat-utils.test.ts for
buildLocationMetadata and buildGifMetadata nested JSON output, and for both
parsers handling legacy flat PascalCase input, malformed JSON, invalid
NaN/Infinity coordinates, and missing GIF URL fields. Assert valid results and
null returns according to the existing parseLocationMetadata and
parseGifMetadata behavior without changing production code.
In `@src/services/signalr.service.ts`:
- Around line 541-545: Add a service-level test for the callback path handled by
handleMessage: register a hub method callback and a signalRService.on()
listener, invoke the callback with two arguments, and assert the listener
receives both arguments in order. Extend the existing single-argument coverage
without changing chat hub tests or production behavior.
In `@src/stores/chat/__tests__/hub-invoke-args.test.ts`:
- Around line 160-176: Update the “chat typing events” test suite around
handleTyping to enable Jest fake timers before each test and restore real timers
after each test. Reset the chat store before calling jest.useRealTimers() so
scheduled typing-expiry callbacks cannot mutate state after tests complete.
- Around line 72-114: Extend the tests around joinChannel, sendTyping, and
markChannelRead to cover a null active-unit ID, configuring the store state or
mock context accordingly. Assert each hub invocation retains its full positional
argument list and passes null as the final argument for JoinChannel, Typing, and
MarkRead, while preserving the existing active-unit assertions.
🪄 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: 23b33851-5d76-4526-9b4a-340bb3cb4471
📒 Files selected for processing (12)
src/app/(app)/chatbot.tsxsrc/app/_layout.tsxsrc/app/chat/[channelId].tsxsrc/app/chat/thread/[messageId].tsxsrc/components/chat/chat-utils.tssrc/components/chat/message-actions-sheet.tsxsrc/components/chat/message-bubble.tsxsrc/components/chat/new-conversation-sheet.tsxsrc/services/signalr.service.tssrc/stores/chat/__tests__/hub-invoke-args.test.tssrc/stores/chat/store.tssrc/stores/signalr/signalr-store.ts
| export function buildLocationMetadata(latitude: number, longitude: number, label?: string): string { | ||
| return JSON.stringify({ location: { latitude, longitude, label } }); | ||
| } | ||
|
|
||
| export function buildGifMetadata(gif: { GifUrl: string; PreviewUrl?: string; Width?: number; Height?: number }): string { | ||
| return JSON.stringify({ gif: { url: gif.GifUrl, previewUrl: gif.PreviewUrl, width: gif.Width, height: gif.Height } }); | ||
| } | ||
|
|
||
| export function parseLocationMetadata(metadataJson?: string | null): ChatLocationMetadata | null { | ||
| return parseMetadata<ChatLocationMetadata>(metadataJson); | ||
| const raw = parseMetadata<Record<string, unknown>>(metadataJson); | ||
| if (!raw) return null; | ||
| const nested = (raw.location ?? raw.Location) as Record<string, unknown> | undefined; | ||
| const source = nested ?? raw; | ||
| const latitude = readNumber(source.latitude ?? source.Latitude); | ||
| const longitude = readNumber(source.longitude ?? source.Longitude); | ||
| if (latitude === undefined || longitude === undefined) return null; | ||
| return { Latitude: latitude, Longitude: longitude, Label: readString(source.label ?? source.Label) }; | ||
| } | ||
|
|
||
| export function parseGifMetadata(metadataJson?: string | null): ChatGifMetadata | null { | ||
| return parseMetadata<ChatGifMetadata>(metadataJson); | ||
| const raw = parseMetadata<Record<string, unknown>>(metadataJson); | ||
| if (!raw) return null; | ||
| const nested = (raw.gif ?? raw.Gif) as Record<string, unknown> | undefined; | ||
| const source = nested ?? raw; | ||
| const url = readString(source.url ?? source.Url ?? source.gifUrl ?? source.GifUrl); | ||
| if (!url) return null; | ||
| return { | ||
| GifUrl: url, | ||
| PreviewUrl: readString(source.previewUrl ?? source.PreviewUrl), | ||
| Width: readNumber(source.width ?? source.Width), | ||
| Height: readNumber(source.height ?? source.Height), | ||
| Title: readString(source.title ?? source.Title), | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/components/chat/chat-utils.ts --items all
ast-grep outline src/components/chat/__tests__/chat-utils.test.ts --items all
rg -n -C 3 \
'build(Location|Gif)Metadata|parse(Location|Gif)Metadata|legacy|NaN|Infinity|MetadataJson' \
src/components/chat/__tests__/chat-utils.test.tsRepository: Resgrid/Unit
Length of output: 2791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== chat-utils.ts relevant code =="
sed -n '50,122p' src/components/chat/chat-utils.ts | cat -n -v
echo
echo "== chat-utils test imports and content =="
sed -n '1,140p' src/components/chat/__tests__/chat-utils.test.ts | cat -n -v
echo
echo "== metadata metadata contract tests across repo =="
rg -n -C 4 \
'buildLocationMetadata|buildGifMetadata|parseLocationMetadata|parseGifMetadata|parseImageMetadata|parseMetadata|buildImageMetadata' \
src/components/chat/__tests__ src --glob '*.{test,Ts,ts,tsx}' || true
echo
echo "== tests using readNumber/readString/error/null return edge cases =="
rg -n -C 3 \
'NaN|Infinity|malform|invalid.coordinates|missing|metadataJson|MetadataJson|readNumber|readString' \
src/components/chat/__tests__ src --glob '*.{test,Ts,ts,tsx}' || trueRepository: Resgrid/Unit
Length of output: 50368
Add Jest coverage for the metadata wire contract.
buildLocationMetadata, buildGifMetadata, parseLocationMetadata, and parseGifMetadata handle persisted message metadata and web-client compatibility, but src/components/chat/__tests__/chat-utils.test.ts does not cover these functions. Add cases for nested JSON output, legacy flat PascalCase input, malformed JSON, invalid coordinates such as NaN/Infinity, and missing GIF URL fields.
🤖 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/components/chat/chat-utils.ts` around lines 83 - 115, Add Jest coverage
in chat-utils.test.ts for buildLocationMetadata and buildGifMetadata nested JSON
output, and for both parsers handling legacy flat PascalCase input, malformed
JSON, invalid NaN/Infinity coordinates, and missing GIF URL fields. Assert valid
results and null returns according to the existing parseLocationMetadata and
parseGifMetadata behavior without changing production code.
Source: Coding guidelines
| private handleMessage(_hubName: string, method: string, args: unknown[]): void { | ||
| // Emit event for subscribers using the method name as the event name. Hub methods | ||
| // can send more than one argument (chatPresenceChanged sends `userId, isOnline`), | ||
| // so forward every argument to the listeners. | ||
| this.emit(method, ...args); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/services/__tests__/signalr.service.test.ts --items all
rg -n -C 4 'connection\.on|SignalREventListener|handleMessage|eventListeners|chatPresenceChanged|args' \
src/services/__tests__/signalr.service.test.ts \
src/stores/chat/__tests__/hub-invoke-args.test.tsRepository: Resgrid/Unit
Length of output: 2933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l src/services/signalr.service.ts src/services/__tests__/signalr.service.test.ts
echo '--- service occurrences'
rg -n -C 3 'class SignalRService|handleMessage|emit\(|on\(|removeListener|removeListeners|eventListeners' src/services/signalr.service.ts
echo '--- service test occurrences broad'
rg -n -C 3 'SignalRService|handleMessage|emit\(|on\(|removeListener|removeListeners|chatPresenceChanged|presence|args|args:' src/services/__tests__/signalr.service.test.ts
echo '--- test files around signalr service'
fd -t f 'signalr.*test.*|hub-invoke-args.test.*' src/stores src/services -x sh -c 'echo "--- {}"; wc -l "{}"; sed -n "1,180p" "{}"'Repository: Resgrid/Unit
Length of output: 38148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- signalRService tests around event listener invocation'
sed -n '440,515p' src/services/__tests__/signalr.service.test.ts
sed -n '800,905p' src/services/__tests__/signalr.service.enhanced.test.ts
echo '--- programmatic discovery of variadic listener tests'
python3 - <<'PY'
from pathlib import Path
for path in Path('src/services/__tests__').glob('*.ts'):
text = path.read_text()
if 'on(' not in text and 'connection.on' not in text:
continue
has_multi = any(s in text for s in ['userId, isOnline', '"user-2", true', "'user-2', true", 'userId', 'isOnline', ...args, '(userId, isOnline)'])
has_callback_call = '__called' in text.lower() or '.mock.calls' in text or '__listener' in text or 'calledWith' in text or '.toHaveBeenCalledWith' in text
print(path, 'has_multi_like=', has_multi, 'has_mock_call_assert=', has_callback_call)
PYRepository: Resgrid/Unit
Length of output: 3022
Add variadic SignalR service coverage.
The existing SignalR service tests only verify a single message argument. Add a service-level test that registers a hub method callback and an signalRService.on() listener, then invokes that callback with two arguments and asserts the listener receives both positional arguments. The chat hub tests do not cover this service emitter path.
🤖 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/services/signalr.service.ts` around lines 541 - 545, Add a service-level
test for the callback path handled by handleMessage: register a hub method
callback and a signalRService.on() listener, invoke the callback with two
arguments, and assert the listener receives both arguments in order. Extend the
existing single-argument coverage without changing chat hub tests or production
behavior.
Source: Coding guidelines
| it('sends both JoinChannel arguments', async () => { | ||
| await useChatStore.getState().joinChannel('channel-1'); | ||
|
|
||
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'JoinChannel', 'channel-1', 42); | ||
| }); | ||
|
|
||
| it('sends all four Typing arguments in hub order', () => { | ||
| useChatStore.getState().sendTyping('channel-1', true); | ||
|
|
||
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'Typing', 'channel-1', 'Engine 6', true, 42); | ||
| }); | ||
|
|
||
| it('sends all three MarkRead arguments', async () => { | ||
| useChatStore.setState({ | ||
| messagesByChannel: { | ||
| 'channel-1': [ | ||
| { | ||
| ChatMessageId: 'm1', | ||
| ChatChannelId: 'channel-1', | ||
| MessageSeq: 42, | ||
| SenderParticipantType: 0, | ||
| SenderUserId: 'user-2', | ||
| SenderDisplayName: 'Other', | ||
| Body: 'hi', | ||
| MessageType: 0, | ||
| Priority: 0, | ||
| ThreadRootMessageId: null, | ||
| ThreadReplyCount: 0, | ||
| AlsoSendToChannel: false, | ||
| MetadataJson: null, | ||
| ClientMessageId: 'c1', | ||
| SentOn: new Date(0).toISOString(), | ||
| Reactions: [], | ||
| Attachments: [], | ||
| }, | ||
| ], | ||
| }, | ||
| } as unknown as Parameters<typeof useChatStore.setState>[0]); | ||
|
|
||
| await useChatStore.getState().markChannelRead('channel-1'); | ||
|
|
||
| expect(mockInvoke).toHaveBeenCalledWith('chatHub', 'MarkRead', 'channel-1', 42, 42); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test nullable asUnitId arguments.
These tests only cover active unit ID 42. Add no-active-unit cases for JoinChannel, Typing, and MarkRead. Assert that each invocation passes null as the final positional argument instead of omitting it.
As per coding guidelines, “Generate tests for all components, services, and logic generated.”
🤖 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` around lines 72 - 114,
Extend the tests around joinChannel, sendTyping, and markChannelRead to cover a
null active-unit ID, configuring the store state or mock context accordingly.
Assert each hub invocation retains its full positional argument list and passes
null as the final argument for JoinChannel, Typing, and MarkRead, while
preserving the existing active-unit assertions.
Source: Coding guidelines
| describe('chat typing events', () => { | ||
| beforeEach(() => { | ||
| useChatStore.setState({ typingByChannel: {} }); | ||
| }); | ||
|
|
||
| it('reads the hub payload ChannelId field', () => { | ||
| useChatStore.getState().handleTyping({ ChannelId: 'channel-1', UserId: 'user-2', DisplayName: 'Other', IsTyping: true }); | ||
|
|
||
| expect(useChatStore.getState().typingByChannel['channel-1']?.[0]?.displayName).toBe('Other'); | ||
| }); | ||
|
|
||
| it('reads a camelCase hub payload', () => { | ||
| useChatStore.getState().handleTyping({ channelId: 'channel-1', userId: 'user-2', displayName: 'Other', isTyping: true }); | ||
|
|
||
| expect(useChatStore.getState().typingByChannel['channel-1']?.[0]?.userId).toBe('user-2'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use fake timers for typing tests.
handleTyping() schedules a typing-expiry timer. Use jest.useFakeTimers() and reset the chat store before jest.useRealTimers() so timers cannot mutate state after a test completes.
As per coding guidelines, “Use jest.useFakeTimers() and jest.useRealTimers() for time-dependent 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/stores/chat/__tests__/hub-invoke-args.test.ts` around lines 160 - 176,
Update the “chat typing events” test suite around handleTyping to enable Jest
fake timers before each test and restore real timers after each test. Reset the
chat store before calling jest.useRealTimers() so scheduled typing-expiry
callbacks cannot mutate state after tests complete.
Source: Coding guidelines
| showSender={false} | ||
| currentUserId={currentUserId} | ||
| onLongPress={setActionsMessage} | ||
| onToggleReaction={() => undefined} |
There was a problem hiding this comment.
Performance impact from inline arrow functions in JSX props, which create new function instances on every render. Move the onToggleReaction definition outside the render method.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File src/app/(app)/chatbot.tsx:
Line 76:
Performance impact from inline arrow functions in JSX props, which create new function instances on every render. Move the `onToggleReaction` definition outside the render method.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| maintainVisibleContentPosition={{ startRenderingFromBottom: true }} | ||
| // autoscrollToBottomThreshold is off by default in FlashList v2; without it a | ||
| // sent/incoming message lands below the viewport, hidden behind the composer. | ||
| maintainVisibleContentPosition={{ startRenderingFromBottom: true, autoscrollToBottomThreshold: 0.2 }} |
There was a problem hiding this comment.
Unnamed magic number for the autoscrollToBottomThreshold ratio complicates future tuning and code review. Extract the 0.2 literal into a named constant like const AUTOSCROLL_TO_BOTTOM_THRESHOLD = 0.2; at the module or component scope.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File src/app/chat/[channelId].tsx:
Line 317:
Unnamed magic number for the `autoscrollToBottomThreshold` ratio complicates future tuning and code review. Extract the `0.2` literal into a named constant like `const AUTOSCROLL_TO_BOTTOM_THRESHOLD = 0.2;` at the module or component scope.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Signing out while initializeApp was still awaiting left the stale run free to connect the SignalR hubs and mark the app initialized for a session that had already ended. Capture a generation token at the start of each run, bump it whenever the session leaves the signed-in state, and bail at every checkpoint that is no longer current. Clearing the in-progress guard on sign-out also stops a fast sign-out then sign-in from being skipped as "already initializing", which previously left the new session uninitialized with no retry pending. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X8YKbDjQeSLXJs4kU1qdXe
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: 4
🧹 Nitpick comments (3)
src/components/chat/message-composer.tsx (1)
134-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse theme-resolved semantic colors for the new icons.
Lines 134 and 139 use the fixed color
#6b7280. This color does not adapt to the active color scheme. Resolve a semantic typography token from the theme instead.As per coding guidelines, “Use semantic color tokens from Tailwind config (
primary,secondary,background,typography, etc.), not hardcoded hex values.”🤖 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/components/chat/message-composer.tsx` around lines 134 - 139, Update the ImagePlus and Sparkles icon color props in the message composer to use the theme-resolved semantic typography color instead of the hardcoded `#6b7280` value, preserving the existing icon behavior and layout.Source: Coding guidelines
src/components/chat/__tests__/message-composer.test.tsx (2)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove imports below the native-module mocks.
Lines 6-7 import modules before the Expo native-module mocks at Lines 9 and 14. Place all imports after the mock declarations.
As per coding guidelines, “Mock native modules at the top of test files before imports.”
🤖 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/components/chat/__tests__/message-composer.test.tsx` around lines 6 - 7, Move the React and testing-library imports below the Expo native-module mock declarations in message-composer.test.tsx, keeping the mocks at the top of the test file before any imports.Source: Coding guidelines
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplicitly unmount each rendered composer.
Each test creates a rendered tree but does not call
unmount(). Capture therender()result and callunmount()after assertions.As per coding guidelines, “Always call
unmount()in tests to clean up.”Also applies to: 62-62, 72-72, 79-79, 87-87
🤖 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/components/chat/__tests__/message-composer.test.tsx` at line 54, Update each MessageComposer test using render() to capture its render result and call unmount() after the assertions, including the cases at the referenced render calls. Ensure every rendered composer is explicitly cleaned up.Source: Coding guidelines
🤖 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/app/`(app)/__tests__/init-session-generation.test.tsx:
- Line 72: Update each renderHook invocation in the tests using useInitGuard to
destructure its unmount function, including the instances at the referenced
locations. Call every unmount after the assertions so all hook instances are
cleaned up before each test completes.
In `@src/app/`(app)/_layout.tsx:
- Around line 293-301: Reset the completed-initialization state when the auth
status leaves the signed-in state: set hasInitialized.current and isInitComplete
to their uninitialized values alongside the existing initGeneration and
isInitializing cleanup. Keep the existing retry reset and signed-in early return
behavior unchanged so a subsequent sign-in runs initializeApp again and does not
render as initialized prematurely.
In `@src/stores/signalr/signalr-store.ts`:
- Around line 546-554: Invalidate any in-flight chat arm operation from
onChatDisconnected by advancing a connection generation or cancellation token,
and have runChatArm verify it after signalRService.invoke() and before starting
heartbeat, resync, or scheduling retries. Ensure a subsequent connection does
not await or reuse the stale chatArmOperation and can invoke Connect normally.
Add a regression test covering disconnect during pending Connect followed by
reconnect.
- Around line 102-104: Update the setTimeout callback that invokes
armChatSession so it explicitly handles rejected promises, attaching a rejection
handler that logs the failure consistently with runChatArm before or while
preserving the existing retry scheduling behavior.
---
Nitpick comments:
In `@src/components/chat/__tests__/message-composer.test.tsx`:
- Around line 6-7: Move the React and testing-library imports below the Expo
native-module mock declarations in message-composer.test.tsx, keeping the mocks
at the top of the test file before any imports.
- Line 54: Update each MessageComposer test using render() to capture its render
result and call unmount() after the assertions, including the cases at the
referenced render calls. Ensure every rendered composer is explicitly cleaned
up.
In `@src/components/chat/message-composer.tsx`:
- Around line 134-139: Update the ImagePlus and Sparkles icon color props in the
message composer to use the theme-resolved semantic typography color instead of
the hardcoded `#6b7280` value, preserving the existing icon behavior and layout.
🪄 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: 32859a94-2412-4283-bf5f-346c460152f1
📒 Files selected for processing (6)
src/app/(app)/__tests__/init-session-generation.test.tsxsrc/app/(app)/_layout.tsxsrc/app/chat/thread/[messageId].tsxsrc/components/chat/__tests__/message-composer.test.tsxsrc/components/chat/message-composer.tsxsrc/stores/signalr/signalr-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/chat/thread/[messageId].tsx
|
|
||
| it('abandons an in-flight run when the session ends mid-initialization', async () => { | ||
| const gate = deferred(); | ||
| const { result } = renderHook(() => useInitGuard(gate, effects)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unmount every hook instance.
Destructure unmount from each renderHook result. Call it after assertions. This prevents hook state from remaining mounted between tests.
As per coding guidelines, “Always call unmount() in tests to clean up.”
Also applies to: 96-96, 115-115
🤖 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/app/`(app)/__tests__/init-session-generation.test.tsx at line 72, Update
each renderHook invocation in the tests using useInitGuard to destructure its
unmount function, including the instances at the referenced locations. Call
every unmount after the assertions so all hook instances are cleaned up before
each test completes.
Source: Coding guidelines
| if (status === 'signedIn') return; | ||
|
|
||
| // Leaving the signed-in state retires any initialization still in flight, and frees | ||
| // the guard it no longer owns so the next sign-in is not skipped as "already | ||
| // initializing". | ||
| initGeneration.current += 1; | ||
| isInitializing.current = false; | ||
|
|
||
| if (initRetryCount > 0) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset completed initialization state on sign-out.
When TabLayout remains mounted through a sign-out and sign-in, hasInitialized.current remains true. Line 313 then skips initializeApp for the new session. isInitComplete also remains true, so the UI can render as initialized before the new session setup completes.
Proposed fix
initGeneration.current += 1;
isInitializing.current = false;
+ hasInitialized.current = false;
+ setIsInitComplete(false);
if (initRetryCount > 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (status === 'signedIn') return; | |
| // Leaving the signed-in state retires any initialization still in flight, and frees | |
| // the guard it no longer owns so the next sign-in is not skipped as "already | |
| // initializing". | |
| initGeneration.current += 1; | |
| isInitializing.current = false; | |
| if (initRetryCount > 0) { | |
| if (status === 'signedIn') return; | |
| // Leaving the signed-in state retires any initialization still in flight, and frees | |
| // the guard it no longer owns so the next sign-in is not skipped as "already | |
| // initializing". | |
| initGeneration.current += 1; | |
| isInitializing.current = false; | |
| hasInitialized.current = false; | |
| setIsInitComplete(false); | |
| if (initRetryCount > 0) { |
🤖 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/app/`(app)/_layout.tsx around lines 293 - 301, Reset the
completed-initialization state when the auth status leaves the signed-in state:
set hasInitialized.current and isInitComplete to their uninitialized values
alongside the existing initGeneration and isInitializing cleanup. Keep the
existing retry reset and signed-in early return behavior unchanged so a
subsequent sign-in runs initializeApp again and does not render as initialized
prematurely.
| chatArmRetryTimer = setTimeout(() => { | ||
| void armChatSession(); | ||
| }, CHAT_ARM_RETRY_MS); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle rejected retry promises.
The timer callback discards the promise from armChatSession(). If a retry fails, the rejection is unhandled after runChatArm() logs it and schedules another retry. Attach a rejection handler in this callback.
Proposed fix
chatArmRetryTimer = setTimeout(() => {
- void armChatSession();
+ void armChatSession().catch(() => {
+ // runChatArm logs failures and schedules the next retry.
+ });
}, CHAT_ARM_RETRY_MS);As per coding guidelines, “All async operations must have proper try/catch with logging.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chatArmRetryTimer = setTimeout(() => { | |
| void armChatSession(); | |
| }, CHAT_ARM_RETRY_MS); | |
| chatArmRetryTimer = setTimeout(() => { | |
| void armChatSession().catch(() => { | |
| // runChatArm logs failures and schedules the next retry. | |
| }); | |
| }, CHAT_ARM_RETRY_MS); |
🤖 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/signalr/signalr-store.ts` around lines 102 - 104, Update the
setTimeout callback that invokes armChatSession so it explicitly handles
rejected promises, attaching a rejection handler that logs the failure
consistently with runChatArm before or while preserving the existing retry
scheduling behavior.
Source: Coding guidelines
| const onChatDisconnected = () => { | ||
| stopChatHeartbeat(); | ||
| stopChatArmRetry(); | ||
| // The debounce only guards duplicates within one connection; carrying the marker | ||
| // across the gap would swallow the resync that backfills the outage. | ||
| lastChatResyncAt = 0; | ||
| // Clearing the flag is what lets connectChatHub repair the session later; while it | ||
| // stayed true the hub could never be re-announced. | ||
| set({ isChatHubConnected: false }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Invalidate pending arm operations on disconnect.
Lines 546-554 stop timers but do not invalidate an active chatArmOperation. If Connect settles after disconnect, runChatArm() can start a heartbeat, resync chat, or schedule a retry for the closed connection. A later connection can also await that stale operation at Lines 134-135 and skip Connect for its new connection.
Use a connection generation or cancellation token. Increment it on disconnect. Check it after await signalRService.invoke() and before scheduling retries or starting heartbeat and resync. Add a regression test that disconnects while Connect is pending, then reconnects.
As per coding guidelines, “Generate tests for all components, services, and logic; ensure tests run without errors.”
🤖 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/signalr/signalr-store.ts` around lines 546 - 554, Invalidate any
in-flight chat arm operation from onChatDisconnected by advancing a connection
generation or cancellation token, and have runChatArm verify it after
signalRService.invoke() and before starting heartbeat, resync, or scheduling
retries. Ensure a subsequent connection does not await or reuse the stale
chatArmOperation and can invoke Connect normally. Add a regression test covering
disconnect during pending Connect followed by reconnect.
Source: Coding guidelines
| const isCurrentRun = () => initGeneration.current === generation; | ||
|
|
||
| try { | ||
| await gate.promise; |
There was a problem hiding this comment.
Unhandled rejection from await gate.promise inside a try/finally block lacking a catch clause. Add a catch clause or wrap the await in its own try/catch before the finally cleanup to handle rejections explicitly.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/app/(app)/__tests__/init-session-generation.test.tsx:
Line 38:
Unhandled rejection from `await gate.promise` inside a try/finally block lacking a catch clause. Add a catch clause or wrap the await in its own try/catch before the finally cleanup to handle rejections explicitly.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| await act(async () => { | ||
| first.resolve(); | ||
| await Promise.all([pending, second]); |
There was a problem hiding this comment.
Promise.all fails fast on the first rejection, discarding results for independent tasks. Use Promise.allSettled and handle per-item results to support partial failures in batch operations.
Kody rule violation: Use Promise.allSettled for batch operations with partial failures
Prompt for LLM
File src/app/(app)/__tests__/init-session-generation.test.tsx:
Line 133:
`Promise.all` fails fast on the first rejection, discarding results for independent tasks. Use `Promise.allSettled` and handle per-item results to support partial failures in batch operations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <MessageComposer onSendText={handleSendText} onSendImage={() => undefined} onSendLocation={handleSendLocation} onOpenGif={handleSendGif} onTyping={() => undefined} placeholder={t('chat.reply_placeholder')} allowUrgent={false} /> | ||
| {/* Threads carry text and location only; omitting the image/GIF callbacks keeps | ||
| those actions out of the composer instead of showing dead buttons. */} | ||
| <MessageComposer onSendText={handleSendText} onSendLocation={handleSendLocation} onTyping={() => undefined} placeholder={t('chat.reply_placeholder')} allowUrgent={false} /> |
There was a problem hiding this comment.
Inline arrow function onTyping={() => undefined} creates a new function reference on every render, degrading performance. Move the function definition outside the render method to avoid unnecessary child re-renders.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File src/app/chat/thread/[messageId].tsx:
Line 139:
Inline arrow function `onTyping={() => undefined}` creates a new function reference on every render, degrading performance. Move the function definition outside the render method to avoid unnecessary child re-renders.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| void armChatSession({ resetAttempts: true }).catch(() => { | ||
| // runChatArm already logged and scheduled its retry. | ||
| }); |
There was a problem hiding this comment.
Empty .catch handler on the armChatSession call silently swallows rejection, violating the no-silent-swallow rule and obscuring the rejection path in the reconnect handler. Add a minimal structured log such as logger.debug({ message: 'Chat re-arm failed on reconnect; retry scheduled' }).
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File src/stores/signalr/signalr-store.ts:
Line 539 to 541:
Empty `.catch` handler on the `armChatSession` call silently swallows rejection, violating the no-silent-swallow rule and obscuring the rejection path in the reconnect handler. Add a minimal structured log such as `logger.debug({ message: 'Chat re-arm failed on reconnect; retry scheduled' })`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| stopChatHeartbeat(); | ||
| chatHeartbeatTimer = setInterval(() => { | ||
| signalRService.invoke(Env.CHAT_HUB_NAME, 'Heartbeat').catch(() => { |
There was a problem hiding this comment.
String literal 'Heartbeat' duplicates the centralized CHAT_HUB_METHODS constant already declared in this file, risking drift and typos. Replace the inline string with CHAT_HUB_METHODS.HEARTBEAT.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/stores/signalr/signalr-store.ts:
Line 113:
String literal `'Heartbeat'` duplicates the centralized `CHAT_HUB_METHODS` constant already declared in this file, risking drift and typos. Replace the inline string with `CHAT_HUB_METHODS.HEARTBEAT`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| * every reconnect issues a fresh connection id. Without re-arming, the websocket stays | ||
| * open but the client receives nothing. | ||
| */ | ||
| async function runChatArm(): Promise<void> { |
There was a problem hiding this comment.
Missing @returns and @throws tags on the runChatArm JSDoc leave callers unaware that the function rejects when signalRService.invoke fails. Add @returns {Promise<void>} and @throws {Error} documenting the rejection condition.
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File src/stores/signalr/signalr-store.ts:
Line 90:
Missing `@returns` and `@throws` tags on the `runChatArm` JSDoc leave callers unaware that the function rejects when `signalRService.invoke` fails. Add `@returns {Promise<void>}` and `@throws {Error}` documenting the rejection condition.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
Summary
This PR fixes several critical bugs in the chat/realtime system and refines the chatbot UX, bringing the
developbranch tomaster.Realtime (SignalR) Communication Fixes
Multi-argument hub event support: The SignalR service previously forwarded only the first argument from hub events to listeners. Since the server sends some events as multiple positional arguments (e.g.,
chatPresenceChangedsendsuserId, isOnline), presence tracking was completely broken. The service now captures and forwards all arguments.Correct hub invocation arguments: Three chat hub methods were being invoked with wrong or missing arguments, causing SignalR to silently reject the calls and leave the client outside channel groups:
Typing— was placingisTypingin thedisplayNameslot and dropping two arguments; now correctly sends(channelId, displayName, isTyping, asUnitId), including a newcurrentDisplayName()helper that uses the active unit's name.JoinChannelandMarkRead— were conditionally omitting theasUnitIdargument; now always send it asnullwhen there is no active unit, matching the hub's positional binding requirements.Typing event field name: The hub payload uses
ChannelId, but the handler only checkedChatChannelId; both spellings are now accepted.Message Data Normalization
Realtime payloads omit empty collections (
Reactions,Attachments) even though the DTO types them as required. A newwithCollections()helper normalizes every incoming message to ensure these arrays always exist, and preserves existing collections when a partial update arrives so on-screen reactions are never lost. Defensive?? []checks were also added toMessageBubbleand the channel screen for older persisted messages.Metadata Format Alignment
Location and GIF message metadata is now serialized in a nested camelCase format (
{ location: { latitude, longitude, label } },{ gif: { url, previewUrl, width, height } }) to match the web client's contract. The parsers were updated to accept both the new format and the legacy flat PascalCase format for historical messages.Chatbot Screen
MessageActionsSheetnow hides the edit option whenassistantmode is active).UI/UX Fixes
autoscrollToBottomThreshold: 0.2to all three chat list views (channel, thread, chatbot) so new messages automatically scroll into view instead of being hidden behind the composer.AvatarFallbackText) fallback from message bubbles and the new conversation sheet, since the avatar endpoint always returns a silhouette placeholder.Sentry Logging
Disabled Sentry's
debugflag (previously__DEV__) because watchdog-termination tracking floods the Metro console with hundreds of log lines per second. ASENTRY_DEBUGconstant is available to toggle it on for diagnostics.Tests
Added
hub-invoke-args.test.tscovering hub invocation argument counts (JoinChannel, Typing, MarkRead), incoming message collection normalization, presence event handling (both positional and object forms), and typing event field-name parsing.Summary by CodeRabbit
New Features
Bug Fixes
Tests