RD-T39 Fixing build #72
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request encompasses a major refactoring of the codebase configuration and type system, alongside new API modules and state management updates. Changes include ESLint/TypeScript configuration migration, GitHub Actions workflow expansion to support web platform builds, new calendar and shifts API endpoints, router navigation type safety improvements, Jest mock updates, substantial test file removals, and store state enhancements including lockscreen lifecycle tracking and SignalR event type definitions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/features/livekit-call/store/__tests__/useLiveKitCallStore.test.ts (1)
502-549: Duplicate test blocks detected.There's a nested
describe('Error Handling')block (lines 503-536) inside the outerdescribe('Error Handling')(line 502), and the test'should handle basic error state management'is duplicated at lines 525-535 and 538-548. This appears to be a merge artifact or copy-paste error.Consider removing the duplicate nested block:
🔧 Suggested fix
describe('Error Handling', () => { - describe('Error Handling', () => { it('should handle room initialization errors', async () => { // Make the Room constructor throw an error MockedRoom.mockImplementationOnce(() => { throw new Error('Failed to initialize room'); }); const { result } = renderHook(() => useLiveKitCallStore()); await act(async () => { await result.current.actions.connectToRoom('test-room', 'test-participant'); }); expect(mockLogger.error).toHaveBeenCalledWith({ message: 'Failed to connect to LiveKit room', context: { error: expect.any(Error), roomId: 'test-room', participantIdentity: 'test-participant' }, }); expect(result.current.error).toBe('Failed to initialize room'); expect(result.current.isConnecting).toBe(false); expect(result.current.isConnected).toBe(false); }); it('should handle basic error state management', async () => { const { result } = renderHook(() => useLiveKitCallStore()); // Test basic error clearing functionality since token fetching isn't implemented act(() => { // Set an error state and then clear it result.current.actions._clearError(); }); expect(result.current.error).toBeNull(); }); - }); - - it('should handle basic error state management', async () => { - const { result } = renderHook(() => useLiveKitCallStore()); - - // Test basic error clearing functionality since token fetching isn't implemented - act(() => { - // Set an error state and then clear it - result.current.actions._clearError(); - }); - - expect(result.current.error).toBeNull(); - }); });src/stores/calendar/store.ts (5)
109-116: Property name mismatch:response.Datawill be undefined.The API function
getCalendarItemsForDateRange(insrc/api/calendar/calendar.ts, lines 45-51) returnsresponse.data(lowercase). However, this code accessesresponse.Data(uppercase), which will beundefined, causing the.filter()call on line 113 to throw a runtime error.🐛 Proposed fix
- const todayItems = response.Data.filter((item: CalendarItemResultData) => { + const todayItems = (response.Data ?? response.data ?? []).filter((item: CalendarItemResultData) => {Alternatively, if the API consistently returns lowercase
data:- const todayItems = response.Data.filter((item: CalendarItemResultData) => { + const todayItems = response.data.filter((item: CalendarItemResultData) => {
153-162: Same property name mismatch inloadUpcomingCalendarItems.
response.Dataon lines 155 and 161 should beresponse.datato match what the API function returns.🐛 Proposed fix
const response = await getCalendarItemsForDateRange(startDate, endDate); set({ - upcomingCalendarItems: response.Data, + upcomingCalendarItems: response.data, isUpcomingLoading: false, updateCalendarItems: false, }); logger.info({ message: 'Upcoming calendar items loaded successfully', - context: { count: response.Data.length, startDate, endDate }, + context: { count: response.data.length, startDate, endDate }, });
179-188: Same property name mismatch inloadCalendarItems.
response.Dataon lines 181 and 187 should beresponse.data.
201-210: Same property name mismatch inloadCalendarItemsForDateRange.
response.Dataon lines 203 and 209 should beresponse.data.
285-290: Property name mismatch infetchItemTypes.The API function
getCalendarItemTypesreturnsresponse.data, but lines 286 and 289 accessresponse.Data.🐛 Proposed fix
const response = await getCalendarItemTypes(); - set({ itemTypes: response.Data, isTypesLoading: false }); + set({ itemTypes: response.data, isTypesLoading: false }); logger.info({ message: 'Calendar item types fetched successfully', - context: { count: response.Data.length }, + context: { count: response.data.length }, });
🤖 Fix all issues with AI agents
In @.github/workflows/react-native-cicd.yml:
- Around line 449-454: The NOTES filtering pipeline can fail under set -eo
pipefail when all lines are removed (causing grep to exit 1); update the
pipeline that builds NOTES (the command that assigns NOTES and uses multiple
grep -v filters) to tolerate no matches by ensuring the grep pipeline does not
cause a non-zero exit (e.g., append a fallback like "|| true" to the pipeline or
capture the pipeline output into a subshell and handle empty result) so the
assignment to NOTES never fails under set -eo pipefail.
- Around line 475-493: The release step ("📦 Create Release" using
ncipollo/release-action@v1) unconditionally references
'./web-artifacts/ResgridDispatch-web.zip' even though the download step ("📦
Download Web Artifacts") can skip/fail (it uses continue-on-error), so the
release can fail when the web artifact is absent; fix by making the release
artifacts list conditional or computed at runtime (e.g., add an earlier step
that checks for './web-artifacts/ResgridDispatch-web.zip' and sets an output/env
var or writes a comma-separated ARTIFACTS string, then reference that variable
in the Create Release step, or split into two Create Release steps guarded by
if: the first includes web-artifacts when present, the second omits it when
absent), and ensure the Create Release step only uses the
'./web-artifacts/ResgridDispatch-web.zip' path when that file actually exists.
In `@src/app/__tests__/lockscreen.test.tsx`:
- Around line 60-100: Add unit tests covering the missing critical flows: add
tests that toggle the password visibility by interacting with the showPassword
toggle control and assert input type changes; test the unlock submission by
filling the password and invoking the onSubmit flow (mock the unlock handler or
API) and assert expected calls and UI changes; cover error handling by mocking a
failing unlock response and asserting the error message is rendered; cover
loading state by forcing isUnlocking=true (or mocking the unlock promise to
delay) and assert a loading indicator / disabled button; and cover logout by
triggering the handleLogout control and asserting the logout handler is called.
Reference the Lockscreen component, its showPassword state, onSubmit handler,
isUnlocking flag, and handleLogout function when locating where to add these
tests.
In `@src/stores/calendar/store.ts`:
- Around line 267-268: In fetchCalendarItem, the code reads response.Data but
the API getCalendarItem returns response.data; update the property access to
response.data when setting viewCalendarItem and ensure set({ viewCalendarItem:
response.data, isItemLoading: false }) is used so fetchCalendarItem,
getCalendarItem, viewCalendarItem and set all reference the correct payload
property.
In `@src/stores/calls/__tests__/store.test.ts`:
- Around line 106-109: The tests are leaking per-category state because setState
merges, so update the test suite's beforeEach (the block that calls setState) to
explicitly reset the types-specific and priorities-specific fields — e.g., set
typesError to null/'' , isLoadingTypes to false, callTypes to [], and likewise
reset prioritiesError, isLoadingPriorities and the priorities list — so each
test starts with a clean per-category state and is order-independent.
In `@src/stores/lockscreen/store.tsx`:
- Around line 53-61: The setLockTimeout handler currently accepts any number and
computes _cachedTimeoutMs directly, which can produce negative or NaN timeouts;
update setLockTimeout to validate the minutes parameter (use
Number.isFinite(minutes) and clamp to a non-negative value, e.g. minutes =
Math.max(0, Number(minutes) || 0)), log a warning via logger.warn if an invalid
value was provided, and then call set with lockTimeout set to the sanitized
minutes and _cachedTimeoutMs computed as minutes * 60 * 1000 (or 0 when clamped)
to ensure no negative/NaN cached timeout values.
🧹 Nitpick comments (16)
src/services/bluetooth-audio/index.ts (1)
11-14: Import statement should be placed before exports for consistency.While JavaScript hoists imports, placing the import after the re-exports is unconventional. Consider moving it to the top for better readability.
Suggested reordering
export * from './base.service'; export * from './factory.service'; export * from './native.service'; export * from './web.service'; -// Export a singleton instance for convenience -import { createBluetoothAudioService } from './factory.service'; -export const bluetoothAudioService = createBluetoothAudioService(); +import { createBluetoothAudioService } from './factory.service'; + +// Export a singleton instance for convenience +export const bluetoothAudioService = createBluetoothAudioService();src/components/status/__tests__/status-gps-integration.test.tsx (2)
51-54: Remove debug console.log statement.This console.log appears to be a leftover from debugging and will add noise to test output.
🧹 Suggested fix
mockUseLocationStore.mockImplementation(() => { - console.log('useLocationStore called, returning:', mockLocationStore); return mockLocationStore; });
58-59: Remove duplicate mock setup.
mockUseCoreStore.mockReturnValue(mockCoreStore)is called twice consecutively. Remove the redundant call.🧹 Suggested fix
mockUseCoreStore.mockReturnValue(mockCoreStore); - mockUseCoreStore.mockReturnValue(mockCoreStore); // Also mock getState for the status store logic.github/workflows/react-native-cicd.yml (3)
277-284: Add directory existence check before zipping.The step assumes
distdirectory exists afterexpo export. If the export fails silently or produces output elsewhere, this step would fail with a confusing error.Suggested improvement
- name: 📦 Zip Web Export if: (matrix.platform == 'web' && (github.event.inputs.buildType == 'all' || github.event_name == 'push' || github.event.inputs.platform == 'web' || github.event.inputs.platform == 'all')) run: | + if [ ! -d "dist" ]; then + echo "Error: dist directory not found after web export" + exit 1 + fi cd dist zip -r ../ResgridDispatch-web.zip . cd .. echo "Web export zipped successfully" ls -la ResgridDispatch-web.zip
509-521: Unused variables in JSON payload construction.The
--arg repo,--arg commit,--arg actor, and--arg run_urlparameters are defined but not used in the final JSON object. These appear to be leftover from a more detailed payload design.Either remove unused args or include them in the payload
# Create JSON payload using --arg for proper escaping PAYLOAD=$(jq -n \ --arg version "$VERSION" \ --arg date "$RELEASE_DATE" \ --arg notes "$NOTES_CONTENT" \ - --arg repo "${{ github.repository }}" \ - --arg commit "${{ github.sha }}" \ - --arg actor "${{ github.actor }}" \ - --arg run_url "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" \ '{ "version": $version, "title": ("Release v" + $version), "content": $notes }')
528-533: Add timeout to curl to prevent job hangs.The curl command lacks explicit timeouts. If the Changerawr API is unresponsive, this step could hang indefinitely even with
continue-on-error: true.Add connection and max-time limits
# Send to Changerawr API HTTP_STATUS=$(curl -s -o /tmp/changerawr_response.json -w "%{http_code}" \ + --connect-timeout 10 \ + --max-time 30 \ -X POST \ -H "Content-Type: application/json" \ -H "Authorization: Bearer ${CHANGERAWR_API_KEY}" \ -d "$PAYLOAD" \ "${CHANGERAWR_API_URL}")src/app/(app)/_layout.tsx (1)
407-407: Confirm always-on scrollbars are intended on web.overflow: 'scroll'forces scrollbars even when content fits; if this was only to satisfy typing, consider preservingauto.💡 Optional tweak to preserve auto-scroll behavior
- <View style={{ flex: 1, overflow: 'scroll' as 'visible' | 'hidden' | 'scroll' }}> + <View style={{ flex: 1, overflow: 'auto' as any }}>package.json (1)
38-38: Consider a separate lint target for non-srcfiles.Line 38 now limits linting to
src, which may skip config/scripts at the repo root. If those files still need linting, consider adding a secondary script (e.g.,lint:root) or extending the pattern.tsconfig.json (1)
32-42: Redundantnode_modulesexclusion patterns.The exclude array contains overlapping patterns:
"node_modules"already excludes the top-level node_modules"node_modules/**/*"and"**/node_modules/*"are partially redundantTypeScript excludes
node_modulesby default whenoutDiris set or no explicitincludeis provided for it. Consider simplifying to just"**/node_modules"if you need to exclude nested node_modules in monorepo scenarios.♻️ Suggested simplification
"exclude": [ - "node_modules", - "node_modules/**/*", - "**/node_modules/*", + "**/node_modules", "docs", "cli", "android", "lib", "ios", "storybookDocsComponents" ],src/stores/calendar/store.ts (1)
8-8: Unused import.
CalendarItemsResultis imported but not used in this file.🧹 Proposed fix
-import { type CalendarItemsResult } from '@/models/v4/calendar/calendarItemsResult';src/api/common/client.tsx (1)
124-125: Type loosening fromRecord<string, unknown>toobjectreduces type safety.The change from
Record<string, unknown>toobjectallows any non-primitive type, including arrays, class instances, and other objects that may not serialize correctly as JSON payloads. This could mask type errors at compile time.Consider keeping the stricter type or using a more precise alternative if array payloads are needed:
♻️ Suggested alternatives
- post: <T,>(data: object, signal?: AbortSignal) => api.post<T>(endpoint, data, { signal }), - put: <T,>(data: object, signal?: AbortSignal) => api.put<T>(endpoint, data, { signal }), + post: <T,>(data: Record<string, unknown> | unknown[], signal?: AbortSignal) => api.post<T>(endpoint, data, { signal }), + put: <T,>(data: Record<string, unknown> | unknown[], signal?: AbortSignal) => api.put<T>(endpoint, data, { signal }),Or revert to the original stricter typing if arrays aren't needed:
- post: <T,>(data: object, signal?: AbortSignal) => api.post<T>(endpoint, data, { signal }), - put: <T,>(data: object, signal?: AbortSignal) => api.put<T>(endpoint, data, { signal }), + post: <T,>(data: Record<string, unknown>, signal?: AbortSignal) => api.post<T>(endpoint, data, { signal }), + put: <T,>(data: Record<string, unknown>, signal?: AbortSignal) => api.put<T>(endpoint, data, { signal }),src/stores/signalr/signalr-store.ts (1)
175-227: Consider reusing a single timestamp per handler for consistency.Several handlers call
Date.now()multiple times; using onenowavoids tiny drift betweenlastUpdateTimestampand the specific last-* timestamp.♻️ Example pattern to apply
- updateHubHandlers.personnelStatusUpdated = (message: unknown) => { + updateHubHandlers.personnelStatusUpdated = (message: unknown) => { + const now = Date.now(); logger.info({ message: 'personnelStatusUpdated', context: { message }, }); - set({ lastUpdateMessage: JSON.stringify(message), lastUpdateTimestamp: Date.now(), lastEventType: 'personnelStatusUpdated', lastPersonnelUpdateTimestamp: Date.now() }); + set({ lastUpdateMessage: JSON.stringify(message), lastUpdateTimestamp: now, lastEventType: 'personnelStatusUpdated', lastPersonnelUpdateTimestamp: now }); };.eslintrc.js (1)
38-40: Consider re-enabling@typescript-eslint/no-unused-vars.Disabling this rule entirely can mask dead code and unused imports. If the goal is to fix build errors incrementally, consider using
warnseverity instead, or narrowing the disable to specific patterns.- '@typescript-eslint/no-unused-vars': 'off', + '@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_', varsIgnorePattern: '^_' }],This allows prefixing intentionally unused variables with
_while still catching accidental dead code.src/stores/shifts/__tests__/store.test.ts (1)
103-103: Remove unusedmockSignupResult.This mock data is no longer used since
signupForShiftDaynow returns void.🧹 Suggested cleanup
-const mockSignupResult = { ...createBaseMockResult(), Id: 'signup1' };src/stores/shifts/store.ts (2)
104-111: Consider removing redundant inline type annotations.The API functions (
getAllShifts,getTodaysShifts) already return typed promises (Promise<ShiftsResult>,Promise<ShiftDaysResult>). The inline type annotations add verbosity without additional type safety.♻️ Suggested simplification
await Promise.all([ - getAllShifts().then((response: { Data: ShiftResultData[] }) => { - set((state) => ({ ...state, shifts: response.Data })); + getAllShifts().then((response) => { + set({ shifts: response.Data }); }), - getTodaysShifts().then((response: { Data: ShiftDaysResultData[] }) => { - set((state) => ({ ...state, todaysShiftDays: response.Data })); + getTodaysShifts().then((response) => { + set({ todaysShiftDays: response.Data }); }), ]);
139-140: Replaceconsole.logwith logger.For consistency with the rest of the codebase, use the structured logger instead of
console.log.🧹 Suggested fix
} catch (error) { - console.log('fetchAllShifts error:', error); set({ error: 'Failed to fetch shifts', isLoading: false, }); + logger.error({ + message: 'Failed to fetch all shifts', + context: { error }, + });
| # Remove CodeRabbit auto-generated lines | ||
| NOTES="$(printf '%s\n' "$NOTES" \ | ||
| | grep -v "Summary by CodeRabbit" \ | ||
| | grep -v "✏️ Tip: You can customize this high-level summary" \ | ||
| | grep -v "<!-- This is an auto-generated comment: release notes by coderabbit.ai -->" \ | ||
| | grep -v "<!-- end of auto-generated comment: release notes by coderabbit.ai -->")" |
There was a problem hiding this comment.
Potential pipeline failure when filtering CodeRabbit lines.
With set -eo pipefail enabled (line 437), if the notes contain only CodeRabbit-generated content and all lines are filtered out by grep -v, the command will return exit code 1 and fail the step.
Suggested fix using `grep ... || true` or checking result
# Remove CodeRabbit auto-generated lines
- NOTES="$(printf '%s\n' "$NOTES" \
- | grep -v "Summary by CodeRabbit" \
- | grep -v "✏️ Tip: You can customize this high-level summary" \
- | grep -v "<!-- This is an auto-generated comment: release notes by coderabbit.ai -->" \
- | grep -v "<!-- end of auto-generated comment: release notes by coderabbit.ai -->")"
+ NOTES="$(printf '%s\n' "$NOTES" \
+ | grep -v "Summary by CodeRabbit" \
+ | grep -v "✏️ Tip: You can customize this high-level summary" \
+ | grep -v "<!-- This is an auto-generated comment: release notes by coderabbit.ai -->" \
+ | grep -v "<!-- end of auto-generated comment: release notes by coderabbit.ai -->" || true)"🤖 Prompt for AI Agents
In @.github/workflows/react-native-cicd.yml around lines 449 - 454, The NOTES
filtering pipeline can fail under set -eo pipefail when all lines are removed
(causing grep to exit 1); update the pipeline that builds NOTES (the command
that assigns NOTES and uses multiple grep -v filters) to tolerate no matches by
ensuring the grep pipeline does not cause a non-zero exit (e.g., append a
fallback like "|| true" to the pipeline or capture the pipeline output into a
subshell and handle empty result) so the assignment to NOTES never fails under
set -eo pipefail.
| - name: 📦 Download Web Artifacts | ||
| if: ${{ matrix.platform == 'android' && (github.event.inputs.buildType == 'all' || github.event_name == 'push' || github.event.inputs.buildType == 'prod-apk') }} | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: app-builds-web | ||
| path: ./web-artifacts | ||
| continue-on-error: true | ||
|
|
||
| - name: 📦 Create Release | ||
| if: ${{ matrix.platform == 'android' && (github.event.inputs.buildType == 'all' || github.event_name == 'push' || github.event.inputs.buildType == 'prod-apk') }} | ||
| uses: ncipollo/release-action@v1 | ||
| with: | ||
| tag: '10.${{ github.run_number }}' | ||
| tag: '1.${{ github.run_number }}' | ||
| commit: ${{ github.sha }} | ||
| makeLatest: true | ||
| allowUpdates: true | ||
| name: '10.${{ github.run_number }}' | ||
| artifacts: './ResgridRespond-prod.apk' | ||
| bodyFile: 'RELEASE_NOTES.md' | ||
| name: '1.${{ github.run_number }}' | ||
| artifacts: './ResgridDispatch-prod.apk,./web-artifacts/ResgridDispatch-web.zip' | ||
| bodyFile: 'RELEASE_NOTES.md' |
There was a problem hiding this comment.
Release may fail if web artifacts are unavailable.
The download step uses continue-on-error: true (line 481), but the release step still references ./web-artifacts/ResgridDispatch-web.zip unconditionally. If the web build didn't run or failed, this file won't exist and the release action may fail.
Consider dynamically building the artifacts list or adding a fallback:
Suggested approach
- name: 📦 Create Release
if: ${{ matrix.platform == 'android' && (github.event.inputs.buildType == 'all' || github.event_name == 'push' || github.event.inputs.buildType == 'prod-apk') }}
+ id: release
+ run: |
+ ARTIFACTS="./ResgridDispatch-prod.apk"
+ if [ -f "./web-artifacts/ResgridDispatch-web.zip" ]; then
+ ARTIFACTS="${ARTIFACTS},./web-artifacts/ResgridDispatch-web.zip"
+ fi
+ echo "artifacts=${ARTIFACTS}" >> $GITHUB_OUTPUT
+
+ - name: 📦 Create Release
+ if: ${{ matrix.platform == 'android' && (github.event.inputs.buildType == 'all' || github.event_name == 'push' || github.event.inputs.buildType == 'prod-apk') }}
uses: ncipollo/release-action@v1
with:
tag: '1.${{ github.run_number }}'
commit: ${{ github.sha }}
makeLatest: true
allowUpdates: true
name: '1.${{ github.run_number }}'
- artifacts: './ResgridDispatch-prod.apk,./web-artifacts/ResgridDispatch-web.zip'
+ artifacts: ${{ steps.release.outputs.artifacts }}
bodyFile: 'RELEASE_NOTES.md'🤖 Prompt for AI Agents
In @.github/workflows/react-native-cicd.yml around lines 475 - 493, The release
step ("📦 Create Release" using ncipollo/release-action@v1) unconditionally
references './web-artifacts/ResgridDispatch-web.zip' even though the download
step ("📦 Download Web Artifacts") can skip/fail (it uses continue-on-error), so
the release can fail when the web artifact is absent; fix by making the release
artifacts list conditional or computed at runtime (e.g., add an earlier step
that checks for './web-artifacts/ResgridDispatch-web.zip' and sets an output/env
var or writes a comma-separated ARTIFACTS string, then reference that variable
in the Create Release step, or split into two Create Release steps guarded by
if: the first includes web-artifacts when present, the second omits it when
absent), and ensure the Create Release step only uses the
'./web-artifacts/ResgridDispatch-web.zip' path when that file actually exists.
| it('should render lockscreen correctly', () => { | ||
| render(<Lockscreen />); | ||
| render( | ||
| <TestWrapper> | ||
| <Lockscreen /> | ||
| </TestWrapper> | ||
| ); | ||
|
|
||
| expect(screen.getByText('lockscreen.title')).toBeTruthy(); | ||
| expect(screen.getByText('lockscreen.message')).toBeTruthy(); | ||
| expect(screen.getByText('lockscreen.unlock_button')).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should render password input field', () => { | ||
| render(<Lockscreen />); | ||
| render( | ||
| <TestWrapper> | ||
| <Lockscreen /> | ||
| </TestWrapper> | ||
| ); | ||
|
|
||
| expect(screen.getByPlaceholderText('lockscreen.password_placeholder')).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should toggle password visibility', () => { | ||
| render(<Lockscreen />); | ||
|
|
||
| const passwordInput = screen.getByPlaceholderText('lockscreen.password_placeholder'); | ||
| expect(passwordInput.props.secureTextEntry).toBe(true); | ||
|
|
||
| // Find and click the eye icon button | ||
| const eyeButton = screen.getByTestId('password-toggle'); | ||
| fireEvent.press(eyeButton); | ||
| it('should render welcome back message when authenticated', () => { | ||
| render( | ||
| <TestWrapper> | ||
| <Lockscreen /> | ||
| </TestWrapper> | ||
| ); | ||
|
|
||
| expect(passwordInput.props.secureTextEntry).toBe(false); | ||
| expect(screen.getByText('lockscreen.welcome_back')).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should handle unlock submission', async () => { | ||
| render(<Lockscreen />); | ||
|
|
||
| const passwordInput = screen.getByPlaceholderText('lockscreen.password_placeholder'); | ||
| const unlockButton = screen.getByText('lockscreen.unlock_button'); | ||
|
|
||
| fireEvent.changeText(passwordInput, 'testpassword'); | ||
| fireEvent.press(unlockButton); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockUnlock).toHaveBeenCalled(); | ||
| expect(mockReplace).toHaveBeenCalledWith('/(app)'); | ||
| }); | ||
| }); | ||
|
|
||
| it('should show error for empty password', async () => { | ||
| render(<Lockscreen />); | ||
|
|
||
| const unlockButton = screen.getByText('lockscreen.unlock_button'); | ||
| fireEvent.press(unlockButton); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText(/required/i)).toBeTruthy(); | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle logout', async () => { | ||
| render(<Lockscreen />); | ||
|
|
||
| const logoutLink = screen.getByText('lockscreen.not_you'); | ||
| fireEvent.press(logoutLink); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockUnlock).toHaveBeenCalled(); | ||
| expect(mockLogout).toHaveBeenCalled(); | ||
| expect(mockReplace).toHaveBeenCalledWith('/login'); | ||
| }); | ||
| }); | ||
|
|
||
| it('should display loading state while unlocking', async () => { | ||
| render(<Lockscreen />); | ||
|
|
||
| const passwordInput = screen.getByPlaceholderText('lockscreen.password_placeholder'); | ||
| const unlockButton = screen.getByText('lockscreen.unlock_button'); | ||
|
|
||
| fireEvent.changeText(passwordInput, 'testpassword'); | ||
| fireEvent.press(unlockButton); | ||
| it('should display logout link', () => { | ||
| render( | ||
| <TestWrapper> | ||
| <Lockscreen /> | ||
| </TestWrapper> | ||
| ); | ||
|
|
||
| expect(screen.getByText('lockscreen.unlocking')).toBeTruthy(); | ||
| expect(screen.getByText('lockscreen.not_you')).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Significant reduction in test coverage.
The current tests only verify basic rendering (title, message, button, password input, welcome message, logout link). Based on the component's functionality described in the relevant code snippets, the following scenarios are no longer covered:
- Password visibility toggle (
showPasswordstate) - Unlock submission flow (
onSubmithandler) - Error handling and display
- Loading state (
isUnlocking) - Logout flow execution (
handleLogout)
Consider adding these tests back to maintain coverage of critical user flows.
Would you like me to help generate additional test cases for the missing scenarios?
🤖 Prompt for AI Agents
In `@src/app/__tests__/lockscreen.test.tsx` around lines 60 - 100, Add unit tests
covering the missing critical flows: add tests that toggle the password
visibility by interacting with the showPassword toggle control and assert input
type changes; test the unlock submission by filling the password and invoking
the onSubmit flow (mock the unlock handler or API) and assert expected calls and
UI changes; cover error handling by mocking a failing unlock response and
asserting the error message is rendered; cover loading state by forcing
isUnlocking=true (or mocking the unlock promise to delay) and assert a loading
indicator / disabled button; and cover logout by triggering the handleLogout
control and asserting the logout handler is called. Reference the Lockscreen
component, its showPassword state, onSubmit handler, isUnlocking flag, and
handleLogout function when locating where to add these tests.
| const response = await getCalendarItem(calendarItemId); | ||
| set({ viewCalendarItem: response.Data, isItemLoading: false }); |
There was a problem hiding this comment.
Property name mismatch in fetchCalendarItem.
The API function getCalendarItem returns response.data, but line 268 accesses response.Data.
🐛 Proposed fix
const response = await getCalendarItem(calendarItemId);
- set({ viewCalendarItem: response.Data, isItemLoading: false });
+ set({ viewCalendarItem: response.data, isItemLoading: false });🤖 Prompt for AI Agents
In `@src/stores/calendar/store.ts` around lines 267 - 268, In fetchCalendarItem,
the code reads response.Data but the API getCalendarItem returns response.data;
update the property access to response.data when setting viewCalendarItem and
ensure set({ viewCalendarItem: response.data, isItemLoading: false }) is used so
fetchCalendarItem, getCalendarItem, viewCalendarItem and set all reference the
correct payload property.
| await waitFor(() => { | ||
| expect(result.current.error).toBe('Failed to fetch call types'); | ||
| expect(result.current.isLoading).toBe(false); | ||
| expect(result.current.typesError).toBe('API Error'); | ||
| expect(result.current.isLoadingTypes).toBe(false); | ||
| expect(result.current.callTypes).toEqual([]); |
There was a problem hiding this comment.
Reset per-category fields in beforeEach to prevent state leakage.
setState merges, so typesError/isLoadingTypes (and priorities equivalents) can persist across tests, making the suite order‑dependent. Update the reset block accordingly.
🔧 Suggested update
useCallsStore.setState({
calls: [],
callPriorities: [],
callTypes: [],
isLoading: false,
error: null,
+ isLoadingTypes: false,
+ typesError: null,
+ isLoadingPriorities: false,
+ prioritiesError: null,
});🤖 Prompt for AI Agents
In `@src/stores/calls/__tests__/store.test.ts` around lines 106 - 109, The tests
are leaking per-category state because setState merges, so update the test
suite's beforeEach (the block that calls setState) to explicitly reset the
types-specific and priorities-specific fields — e.g., set typesError to null/''
, isLoadingTypes to false, callTypes to [], and likewise reset prioritiesError,
isLoadingPriorities and the priorities list — so each test starts with a clean
per-category state and is order-independent.
| setLockTimeout: (minutes: number) => { | ||
| logger.info({ | ||
| message: 'Setting lock timeout', | ||
| context: { minutes }, | ||
| }); | ||
| set({ | ||
| lockTimeout: minutes, | ||
| _cachedTimeoutMs: minutes * 60 * 1000, | ||
| }); |
There was a problem hiding this comment.
Guard against negative/NaN lock timeout values.
Without validation, a negative/NaN timeout can yield immediate or undefined locking behavior.
✅ Safer timeout handling
- setLockTimeout: (minutes: number) => {
+ setLockTimeout: (minutes: number) => {
logger.info({
message: 'Setting lock timeout',
context: { minutes },
});
+ const safeMinutes = Number.isFinite(minutes) ? Math.max(0, minutes) : 0;
set({
- lockTimeout: minutes,
- _cachedTimeoutMs: minutes * 60 * 1000,
+ lockTimeout: safeMinutes,
+ _cachedTimeoutMs: safeMinutes * 60 * 1000,
});
},📝 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.
| setLockTimeout: (minutes: number) => { | |
| logger.info({ | |
| message: 'Setting lock timeout', | |
| context: { minutes }, | |
| }); | |
| set({ | |
| lockTimeout: minutes, | |
| _cachedTimeoutMs: minutes * 60 * 1000, | |
| }); | |
| setLockTimeout: (minutes: number) => { | |
| logger.info({ | |
| message: 'Setting lock timeout', | |
| context: { minutes }, | |
| }); | |
| const safeMinutes = Number.isFinite(minutes) ? Math.max(0, minutes) : 0; | |
| set({ | |
| lockTimeout: safeMinutes, | |
| _cachedTimeoutMs: safeMinutes * 60 * 1000, | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In `@src/stores/lockscreen/store.tsx` around lines 53 - 61, The setLockTimeout
handler currently accepts any number and computes _cachedTimeoutMs directly,
which can produce negative or NaN timeouts; update setLockTimeout to validate
the minutes parameter (use Number.isFinite(minutes) and clamp to a non-negative
value, e.g. minutes = Math.max(0, Number(minutes) || 0)), log a warning via
logger.warn if an invalid value was provided, and then call set with lockTimeout
set to the sanitized minutes and _cachedTimeoutMs computed as minutes * 60 *
1000 (or 0 when clamped) to ensure no negative/NaN cached timeout values.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.