RD-T39 Login page update, header fix, working on mapbox issue in prod. #81
Conversation
📝 WalkthroughWalkthroughOnboarding is now disabled on web; map layer rendering adds defensive Array.isArray checks for Features; new web-only LoginWeb component and tests added; StatItem gains configurable background colors; translation keys for login were expanded across languages. Changes
Sequence DiagramsequenceDiagram
actor User
participant LoginWeb as LoginWeb\r\nComponent
participant Form as Form\r\n(Validation)
participant ServerModal as Server URL\r\nModal
participant Auth as Authentication\r\nService
participant Analytics as Analytics
participant Store as Zustand\r\nStore
participant App as App
User->>LoginWeb: Load page
LoginWeb->>Analytics: track view
LoginWeb->>User: render form + rotating features
alt open server URL modal
User->>ServerModal: open
User->>ServerModal: enter URL
ServerModal->>Form: validate URL
ServerModal->>Store: save server URL
ServerModal->>User: close
end
User->>Form: enter credentials
User->>LoginWeb: submit (click/enter)
LoginWeb->>Form: validate (zod)
alt validation fails
Form->>User: show inline errors
else
LoginWeb->>LoginWeb: set loading
LoginWeb->>Analytics: track attempt
LoginWeb->>Auth: submit credentials
alt auth succeeds
Auth->>LoginWeb: return token
LoginWeb->>Store: update auth
LoginWeb->>Analytics: track success
LoginWeb->>App: redirect
else auth fails
Auth->>LoginWeb: return error
LoginWeb->>User: show error modal
LoginWeb->>Analytics: track failure
LoginWeb->>LoginWeb: clear loading
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@src/app/login/index.web.tsx`:
- Around line 1-17: Imports at the top of index.web.tsx are unsorted and failing
the linter; run the ESLint autofix (npx eslint --fix) or your repo's lint:fix
script to reorder them, or manually sort the import groups so that external
packages (e.g., zodResolver, expo-router, lucide-react-native, nativewind,
react, react-hook-form, react-i18next, react-native, react-native-reanimated,
zod) come before internal aliases (e.g., Text, useAnalytics, useAuth, Env,
logger, useServerUrlStore), preserving alphabetical order within groups to
satisfy the import/order rule. Ensure no other import statements are duplicated
or incorrectly grouped and re-run the pipeline to confirm the issue is resolved.
🧹 Nitpick comments (7)
src/components/dispatch-console/stats-header.tsx (1)
30-30: Consider handling partial prop scenarios for robustness.The current logic
bgColor || darkBgColor ? (isDark ? darkBgColor : bgColor) : undefinedcould returnundefinedin light mode if onlydarkBgColoris provided, or in dark mode if onlybgColoris provided. While the current usage always provides both values, this could cause unexpected behavior for future callers.♻️ Optional: Fallback to the other color if one is missing
- const backgroundColor = bgColor || darkBgColor ? (isDark ? darkBgColor : bgColor) : undefined; + const backgroundColor = bgColor || darkBgColor ? (isDark ? (darkBgColor ?? bgColor) : (bgColor ?? darkBgColor)) : undefined;src/app/(app)/map.web.tsx (1)
489-492: Avoidgapproperty in StyleSheet for web compatibility.Per the coding guidelines, the
gapproperty has inconsistent support on web. Consider using margin properties instead.♻️ Replace gap with margin
layersPanelActions: { flexDirection: 'row', padding: 12, - gap: 8, }, actionButton: { flex: 1, paddingVertical: 8, paddingHorizontal: 12, borderRadius: 8, alignItems: 'center', + marginHorizontal: 4, },src/app/login/__tests__/index.web.test.tsx (2)
106-109: Module-level mock is unused.The
mockLoginat line 106 is never invoked becauseMockLoginWebcreates its own internalmockLogin(line 18). If you intend to verify that login was called with correct arguments, you need to wire the module-level mock intoMockLoginWeb.♻️ Suggested approach to use the module-level mock
// Mock the entire web login module jest.mock('../index.web', () => { const React = require('react'); const { View, Text, TextInput, Pressable } = require('react-native'); + const { mockLogin } = require('../__mocks__/loginHelpers'); // or pass via closure const MockLoginWeb = () => { const [username, setUsername] = React.useState(''); const [password, setPassword] = React.useState(''); const [showPassword, setShowPassword] = React.useState(false); const [isLoading, setIsLoading] = React.useState(false); const [showServerUrlModal, setShowServerUrlModal] = React.useState(false); const [showErrorModal, setShowErrorModal] = React.useState(false); - const mockLogin = jest.fn(); const mockOnSubmit = () => { if (username && password) { setIsLoading(true); - mockLogin({ username, password }); + // Use the external mock for assertions setTimeout(() => setIsLoading(false), 100); } };Alternatively, if the internal mock suffices for current test scope, consider removing the unused
mockLoginat line 106 to avoid confusion.
83-91: Error modal display is untestable with current mock.The
showErrorModalstate is initialized tofalseand never set totrueinMockLoginWeb, making it impossible to test error modal visibility. Consider adding a mechanism to trigger the error state if error handling tests are needed.src/app/login/index.web.tsx (3)
78-100: Consider adding accessibility attributes to native input.The native HTML input lacks proper ARIA attributes for screen reader support. While placeholders provide visual context, they don't serve as accessible labels.
♿ Suggested accessibility improvements
<input type={type === 'password' && !showPassword ? 'password' : 'text'} className="web-input-accessible" + aria-label={placeholder} + aria-invalid={!!error} + aria-describedby={error ? `${placeholder}-error` : undefined} style={{ flex: 1, border: 'none', // ... rest of styles }} placeholder={placeholder} // ... rest of props />And for the error container:
{error ? ( - <Animated.View entering={FadeIn.duration(200)} style={styles.errorContainer}> + <Animated.View entering={FadeIn.duration(200)} style={styles.errorContainer} id={`${placeholder}-error`} role="alert"> <AlertCircle size={14} color="#ef4444" /> <Text style={styles.errorText}>{error}</Text> </Animated.View> ) : null}
815-817: Multiple uses ofgapproperty in StyleSheet.Per coding guidelines, the
gapproperty has inconsistent support. However, since this is a web-only component (.web.tsx), modern browser support should be adequate. If older browser support is needed, consider using margin properties instead.Occurrences:
formContainer(line 816),errorContainer(line 853),loadingContainer(line 885),serverUrlButton(line 893),footerContainer(line 925),modalButtonRow(line 1035),featuresGrid(line 670).
356-418: Inconsistent indentation in form container section.The static analysis flagged extra indentation in this section (lines 356-418). Running the formatter should fix this automatically.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Localization
✏️ Tip: You can customize this high-level summary in your review settings.