Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds network/transient vs credential-rejection predicates and related logging/handling across client, auth, store, and UI; patches RNFBMessaging header import in the iOS plugin; and updates CLAUDE.md and .gitignore. ChangesAuth & Network error classification
iOS Build Error Fix Enhancement
Docs & Repo Ignore
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (2)
plugins/withWebRTCFrameworkFix.js (2)
93-107: ⚡ Quick winConsider logging or warning when file doesn't exist for consistency.
The first two fix phases throw explicit errors when they can't apply their patches (e.g., lines 55 and 72-75), but this third phase silently skips if the file doesn't exist. While this might be intentional if the dependency is optional, it creates an inconsistency in error handling that could make debugging harder.
Consider adding a console warning or logging when the file is not found, so developers know whether the patch was applied.
📢 Proposed enhancement to add logging
if (fs.existsSync(appDelegatePath)) { let appDelegate = fs.readFileSync(appDelegatePath, 'utf-8'); const rnfbImport = '#import <RNFBApp/RNFBAppModule.h>'; if (!appDelegate.includes(rnfbImport)) { // Insert after the last `#import` line. const lastImportIdx = appDelegate.lastIndexOf('#import'); const endOfLine = appDelegate.indexOf('\n', lastImportIdx); appDelegate = appDelegate.slice(0, endOfLine + 1) + rnfbImport + '\n' + appDelegate.slice(endOfLine + 1); fs.writeFileSync(appDelegatePath, appDelegate); } + } else { + console.warn( + '[withWebRTCFrameworkFix] RNFBMessaging+AppDelegate.h not found — skipping phase 3 patch' + ); }🤖 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 `@plugins/withWebRTCFrameworkFix.js` around lines 93 - 107, The code currently silently skips patching when fs.existsSync(appDelegatePath) is false; add a warning in the else branch to make this consistent with earlier phases: after the existing if (fs.existsSync(appDelegatePath)) { ... } block add an else { console.warn(`withWebRTCFrameworkFix: appDelegate file not found at ${appDelegatePath}, skipping RNFBApp import patch`); } so developers see when appDelegatePath could not be patched (reference symbols: fs.existsSync, appDelegatePath, rnfbImport).
1-115: 💤 Low valueFilename uses camelCase instead of kebab-case per coding guidelines.
The coding guidelines specify that directory and file names should be lowercase and hyphenated (e.g.,
user-profile,chat-screen). This file is namedwithWebRTCFrameworkFix.jsusing camelCase.However, it's worth noting that this naming pattern (
withX) is idiomatic for Expo config plugins and aligns with ecosystem conventions. Consider whether project-level consistency or ecosystem conventions should take precedence. As per coding guidelines, the pattern**/*requires lowercase and hyphenated names.🤖 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 `@plugins/withWebRTCFrameworkFix.js` around lines 1 - 115, The file name uses camelCase (withWebRTCFrameworkFix.js) but our guidelines require lowercase kebab-case; rename the file to with-webrtc-framework-fix.js and update any imports/requires that reference this module (look for require/import of withWebRTCFrameworkFix and the module.exports = withWebRTCFrameworkFix line to ensure callers point to the new filename), and verify any Expo config plugin registration or package.json/plugin lists are updated to the new kebab-case path so the exported function withWebRTCFrameworkFix continues to be resolved.
🤖 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 `@plugins/withWebRTCFrameworkFix.js`:
- Around line 98-104: The code assumes lastImportIdx =
appDelegate.lastIndexOf('#import') is found; add validation so if lastImportIdx
=== -1 you choose a safe insertion point (e.g., set endOfLine to the index of
the first newline or 0 to prepend the import) instead of using indexOf with -1;
also guard the computed endOfLine (from appDelegate.indexOf('\n',
lastImportIdx)) so if endOfLine === -1 set it to appDelegate.length - 1 (or 0
for prepend) before building the new string; update the logic around
lastImportIdx, endOfLine, appDelegate and rnfbImport to use these validated
values to avoid duplicating or corrupting the file.
---
Nitpick comments:
In `@plugins/withWebRTCFrameworkFix.js`:
- Around line 93-107: The code currently silently skips patching when
fs.existsSync(appDelegatePath) is false; add a warning in the else branch to
make this consistent with earlier phases: after the existing if
(fs.existsSync(appDelegatePath)) { ... } block add an else {
console.warn(`withWebRTCFrameworkFix: appDelegate file not found at
${appDelegatePath}, skipping RNFBApp import patch`); } so developers see when
appDelegatePath could not be patched (reference symbols: fs.existsSync,
appDelegatePath, rnfbImport).
- Around line 1-115: The file name uses camelCase (withWebRTCFrameworkFix.js)
but our guidelines require lowercase kebab-case; rename the file to
with-webrtc-framework-fix.js and update any imports/requires that reference this
module (look for require/import of withWebRTCFrameworkFix and the module.exports
= withWebRTCFrameworkFix line to ensure callers point to the new filename), and
verify any Expo config plugin registration or package.json/plugin lists are
updated to the new kebab-case path so the exported function
withWebRTCFrameworkFix continues to be resolved.
🪄 Autofix (Beta)
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
Run ID: a45a1119-49d3-4766-ac44-0128015091f3
📒 Files selected for processing (1)
plugins/withWebRTCFrameworkFix.js
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 `@plugins/withWebRTCFrameworkFix.js`:
- Around line 100-110: The insertion logic in the block using
lastImportIdx/endOfLine/appDelegate can produce a merged import when the file
ends without a trailing newline; change the insertion so that when endOfLine ===
-1 you still ensure a newline separator is inserted before rnfbImport (either by
adjusting insertAt to append a '\n' or prefixing rnfbImport with '\n' when
appDelegate[appDelegate.length-1] !== '\n'). Update the code around insertAt and
the concatenation of appDelegate/rnfbImport to guarantee a single newline
between the existing last import and rnfbImport, preserving existing behavior in
other cases.
🪄 Autofix (Beta)
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
Run ID: 240b9b55-bd8f-47bd-9959-b5375fd9bfec
📒 Files selected for processing (1)
plugins/withWebRTCFrameworkFix.js
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/(app)/index.tsx (1)
205-245:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse null checks for coordinates instead of truthiness.
0is a valid latitude/longitude, so these guards skip camera reset/follow for units on the equator or prime meridian. The rest of this file already uses!= nullfor coordinates; these two touched checks should do the same.🛠️ Suggested fix
- if (isMapReady && locationLatitude && locationLongitude) { + if (isMapReady && locationLatitude != null && locationLongitude != null) { @@ - if (isScreenFocused && isMapReady && locationLatitude && locationLongitude) { + if (isScreenFocused && isMapReady && locationLatitude != null && locationLongitude != null) {Also applies to: 274-298
🤖 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)/index.tsx around lines 205 - 245, The current truthy checks skip valid coordinates of 0; update the guard inside the useCallback (the if that checks isMapReady, locationLatitude, locationLongitude) to use explicit null/undefined checks (e.g., locationLatitude != null && locationLongitude != null) while still requiring isMapReady; do the same for the other similar guard later in this file (the one around lines ~274-298) so functions like the camera reset (cameraRef.current?.setCamera / cameraConfig) run for coordinates of 0.
🧹 Nitpick comments (1)
src/stores/app/core-store.ts (1)
122-129: ⚡ Quick winConsolidate warning logs at one layer per failure path.
src/stores/app/core-store.tswarns infetchConfig()and then warns again ininit()before the bubbled error reachessrc/app/(app)/_layout.tsx, andsrc/lib/auth/api.tsxwarns beforesrc/api/common/client.tsxwarns again with the classified refresh outcome. The shared root cause is logging recoverable failures in lower layers that immediately rethrow; keep the warn at the layer that owns the classification/user action and let the lower layer just propagate the error.🤖 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/app/core-store.ts` around lines 122 - 129, The init() path in core-store is emitting a duplicate logger.warn for network errors even though fetchConfig() already logs that condition; remove the redundant logger.warn call inside the init() error branch (the block that checks isNetworkError(error) and calls logger.warn) and let the error propagate so the owning layer that classifies user action (e.g., fetchConfig()/the layout or auth layer) performs the single warn. Specifically, update the init() method (and its catch/rethrow logic) to stop logging recoverable errors locally and simply rethrow/propagate them, keeping logging only at the higher-level handler that owns the classification.
🤖 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/lib/auth/__tests__/token-refresh.test.ts`:
- Around line 12-25: The predicate is too broad: isRefreshCredentialRejection
currently treats any 400 as an invalid refresh and causes logout; update
isRefreshCredentialRejection to only return true for status 400 when the OAuth
error payload explicitly indicates "invalid_grant" (inspect
error.response?.data?.error or similar) or for 401 as before, then add a unit
test using axiosErrorWithStatus to assert false for a 400 response whose body
does not contain error: "invalid_grant" and a positive test for a 400 with
error: "invalid_grant"; also ensure the logout callsite in
src/api/common/client.tsx still calls isRefreshCredentialRejection and requires
no other changes.
In `@src/stores/app/__tests__/core-store.test.ts`:
- Around line 282-291: The test leaves the rendered hook subscription mounted;
update the renderHook call to also capture the unmount helper (e.g., const {
result, unmount } = renderHook(() => useCoreStore())) and invoke unmount() after
your assertions (after checking logger calls and result.current.error) to clean
up the hook subscription and avoid leaking Zustand listeners across tests.
In `@src/utils/network.ts`:
- Line 13: The current isNetworkError implementation treats any AxiosError with
no response as a network error; change isNetworkError to only return true for
explicit network-related signs (e.g., axios.isAxiosError(error) && (error.code
=== 'ERR_NETWORK' || error.code === 'ECONNABORTED' || (error.request &&
!error.response && <explicit network indicator if used>))), removing the blanket
!error.response clause so non-transient client errors (like new
AxiosError('Something failed')) are not misclassified; update the function named
isNetworkError and any checks of error.code/error.response accordingly, and
adjust tests in src/utils/__tests__/network.test.ts to reflect the narrower
contract (ensure tests assert that an AxiosError with no response but without
network codes is NOT considered a network error, while errors with
'ERR_NETWORK'/'ECONNABORTED' still are).
---
Outside diff comments:
In `@src/app/`(app)/index.tsx:
- Around line 205-245: The current truthy checks skip valid coordinates of 0;
update the guard inside the useCallback (the if that checks isMapReady,
locationLatitude, locationLongitude) to use explicit null/undefined checks
(e.g., locationLatitude != null && locationLongitude != null) while still
requiring isMapReady; do the same for the other similar guard later in this file
(the one around lines ~274-298) so functions like the camera reset
(cameraRef.current?.setCamera / cameraConfig) run for coordinates of 0.
---
Nitpick comments:
In `@src/stores/app/core-store.ts`:
- Around line 122-129: The init() path in core-store is emitting a duplicate
logger.warn for network errors even though fetchConfig() already logs that
condition; remove the redundant logger.warn call inside the init() error branch
(the block that checks isNetworkError(error) and calls logger.warn) and let the
error propagate so the owning layer that classifies user action (e.g.,
fetchConfig()/the layout or auth layer) performs the single warn. Specifically,
update the init() method (and its catch/rethrow logic) to stop logging
recoverable errors locally and simply rethrow/propagate them, keeping logging
only at the higher-level handler that owns the classification.
🪄 Autofix (Beta)
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
Run ID: 9f0d47f5-57db-4d25-b1f1-cb22679cf73c
📒 Files selected for processing (12)
.gitignoreCLAUDE.mdsrc/api/common/client.tsxsrc/app/(app)/_layout.tsxsrc/app/(app)/index.tsxsrc/lib/auth/__tests__/token-refresh.test.tssrc/lib/auth/api.tsxsrc/lib/auth/token-refresh.tssrc/stores/app/__tests__/core-store.test.tssrc/stores/app/core-store.tssrc/utils/__tests__/network.test.tssrc/utils/network.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/auth/token-refresh.ts
Comment on lines
+12
to
+25
| it('returns true when the token endpoint rejects the refresh token (400 invalid_grant)', () => { | ||
| expect(isRefreshCredentialRejection(axiosErrorWithStatus(400))).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true for a 401 from the token endpoint', () => { | ||
| expect(isRefreshCredentialRejection(axiosErrorWithStatus(401))).toBe(true); | ||
| }); | ||
|
|
||
| it.each([500, 502, 503, 504, 429])('returns false for transient server status %i (preserve session)', (status) => { | ||
| expect(isRefreshCredentialRejection(axiosErrorWithStatus(status))).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false for 403 (ambiguous, not a definitive credential rejection)', () => { | ||
| expect(isRefreshCredentialRejection(axiosErrorWithStatus(403))).toBe(false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List relevant files
fd -a "token-refresh.ts" src/lib/auth || true
fd -a "token-refresh.test.ts" src/lib/auth/__tests__ || true
fd -a "client.tsx" src/api/common || true
# Print the test file section
echo "---- token-refresh.test.ts (lines 1-220) ----"
sed -n '1,220p' src/lib/auth/__tests__/token-refresh.test.ts
# Print predicate implementation
echo "---- token-refresh.ts (lines 1-240) ----"
sed -n '1,240p' src/lib/auth/token-refresh.ts
# Print usage in client
echo "---- client.tsx (lines 1-220) ----"
sed -n '1,220p' src/api/common/client.tsx
echo "---- client.tsx (search for isRefreshCredentialRejection) ----"
rg -n "isRefreshCredentialRejection" src/api/common/client.tsx src/lib/auth -SRepository: Resgrid/Unit
Length of output: 10220
Don’t treat every HTTP 400 from the token endpoint as an invalid refresh credential.
isRefreshCredentialRejectionreturnstruefor any400(status-only), andsrc/api/common/client.tsxlogs the user out whenever it returnstrue.- The JSDoc contract says “400 (
invalid_grant) or 401”, but the predicate/tests don’t check forinvalid_grant, so the suite currently locks in “logout-on-any-400”. - Add a negative test for a 400 response that’s not
invalid_grant(and/or update the predicate to inspect the OAuth error payload), or adjust the JSDoc contract.
🤖 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/lib/auth/__tests__/token-refresh.test.ts` around lines 12 - 25, The
predicate is too broad: isRefreshCredentialRejection currently treats any 400 as
an invalid refresh and causes logout; update isRefreshCredentialRejection to
only return true for status 400 when the OAuth error payload explicitly
indicates "invalid_grant" (inspect error.response?.data?.error or similar) or
for 401 as before, then add a unit test using axiosErrorWithStatus to assert
false for a 400 response whose body does not contain error: "invalid_grant" and
a positive test for a 400 with error: "invalid_grant"; also ensure the logout
callsite in src/api/common/client.tsx still calls isRefreshCredentialRejection
and requires no other changes.
Comment on lines
+282
to
+291
| const { result } = renderHook(() => useCoreStore()); | ||
|
|
||
| await act(async () => { | ||
| await expect(result.current.fetchConfig()).rejects.toBe(networkError); | ||
| }); | ||
|
|
||
| expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({ message: expect.stringContaining('network connectivity') })); | ||
| expect(logger.error).not.toHaveBeenCalled(); | ||
| expect(result.current.error).toBe('Failed to fetch config'); | ||
| }); |
There was a problem hiding this comment.
Call unmount() for this hook instance.
This new case leaves the rendered store subscription alive after the assertions. In a persisted Zustand test file like this one, skipping cleanup can leak listeners across cases and hide state-isolation bugs.
As per coding guidelines, "Use TestWrapper components for test providers and always call unmount() to clean up" and "Always call unmount() in tests to clean up after assertions."
🤖 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/app/__tests__/core-store.test.ts` around lines 282 - 291, The test
leaves the rendered hook subscription mounted; update the renderHook call to
also capture the unmount helper (e.g., const { result, unmount } = renderHook(()
=> useCoreStore())) and invoke unmount() after your assertions (after checking
logger calls and result.current.error) to clean up the hook subscription and
avoid leaking Zustand listeners across tests.
Source: Coding guidelines
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Stability & UX
Behavior
New Features & Tests
Chores