feat: Add locale to Sign In #304
Conversation
📝 WalkthroughWalkthroughTwo files are modified to add locale support to sign-in operations. In Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes involve straightforward additions of locale support across two files following a consistent pattern. While Pre-merge checks✅ Passed checks (3 passed)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
source/api/src/main/kotlin/com/clerk/api/passkeys/GoogleCredentialAuthenticationService.kt(1 hunks)source/api/src/main/kotlin/com/clerk/api/signin/SignIn.kt(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
source/api/src/main/kotlin/com/clerk/api/signin/SignIn.ktsource/api/src/main/kotlin/com/clerk/api/passkeys/GoogleCredentialAuthenticationService.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
source/api/src/main/kotlin/com/clerk/api/passkeys/GoogleCredentialAuthenticationService.kt (1)
156-161: LGTM! Locale addition aligns with PR objectives.The addition of locale to the sign-in parameter map is straightforward and consistent with the changes in SignIn.kt. The use of
.orEmpty()provides a safe fallback for null values.Please verify:
- That the backend API handles empty locale strings gracefully when
Clerk.locale.valueis null- Whether it would be preferable to omit the locale key entirely when the value is empty/null, rather than sending an empty string
As per coding guidelines, please add tests to cover this locale addition. Consider testing:
- Sign-in with a valid locale set
- Sign-in with null/empty locale
- Verification that the locale is correctly included in the API request
source/api/src/main/kotlin/com/clerk/api/signin/SignIn.kt (1)
3-3: LGTM! Import required for locale access.The import is necessary to access
Clerk.localein the refactoredcreate()method below.
| else -> { | ||
| val baseMap = | ||
| if (params is CreateParams.Strategy.Transfer) { | ||
| mapOf(TRANSFER to "true") | ||
| } else { | ||
| params.toMap() | ||
| } | ||
| val paramMap = baseMap + ("locale" to Clerk.locale.value.orEmpty()) | ||
| ClerkApi.signIn.createSignIn(paramMap) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Transfer strategy handling remains correct.
The refactoring successfully unifies locale addition across all non-Passkey sign-in strategies. However, the special handling for the Transfer strategy should be verified.
The Transfer strategy is handled differently from other strategies:
- Transfer: Creates
mapOf(TRANSFER to "true")- sends as a boolean flag - Other strategies: Uses
params.toMap()- serializes all strategy properties
Please verify:
Additionally, as per coding guidelines, please add tests covering:
- Sign-in with Transfer strategy including locale
- Sign-in with other strategies (EmailCode, PhoneCode, Password) including locale
- Verification that locale is correctly appended to the parameter map for each strategy type
🏁 Script executed:
#!/bin/bash
# Search for Transfer strategy usage and any existing tests
rg -nP -C5 'CreateParams\.Strategy\.Transfer|TRANSFER.*true'
# Look for any documentation or comments explaining Transfer strategy
rg -nP -C3 'Transfer.*strategy|strategy.*Transfer'Length of output: 2308
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for toMap() implementation on Strategy classes
rg -nP 'fun.*toMap|\.toMap\(' source/api/src/main/kotlin/com/clerk/api/signin/SignIn.kt -A3 -B3
# Search for TRANSFER constant
rg -nP 'const val TRANSFER|val TRANSFER' source/api/src/main/kotlin/com/clerk/api/signin/SignIn.kt -A1 -B1
# Look for Strategy interface and any custom serialization
rg -nP 'interface Strategy|sealed class Strategy|abstract class Strategy' source/api/src/main/kotlin/com/clerk/api/signin/SignIn.kt -A5
Length of output: 1833
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for toMap() extension function definition across entire codebase
rg -nP 'fun.*Strategy.*\.toMap|fun.*\.toMap.*Strategy' -A5
# Search for TRANSFER constant across entire codebase
rg -nP 'TRANSFER\s*=' -B2 -A2
# Find all properties of Transfer data class
rg -nP 'data class Transfer' -A10
# Search for any tests related to Transfer strategy
fd -e 'kt' -e 'java' | xargs grep -l 'Transfer.*strategy\|Strategy\.Transfer' | grep -i test
Length of output: 1571
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for @AutoMap annotation definition
rg -nP '@AutoMap|class AutoMap|interface AutoMap|annotation class AutoMap' -A3
# Get complete Transfer class definition with all annotations
rg -nP 'data class Transfer' -B5 -A8
# Get complete Ticket class definition to compare
rg -nP 'data class Ticket' -B5 -A15
# Check all Strategy subclasses for @AutoMap
rg -nP 'data class.*Strategy|@Serializable.*data class|@AutoMap.*data class' -B3 -A5
# Search for tests with Transfer strategy
fd . -type f \( -name '*Test.kt' -o -name '*Test.java' \) | xargs grep -l 'Transfer' 2>/dev/null | head -5
Length of output: 26550
Add @AutoMap annotation to Transfer class or clarify the "true" value requirement.
The Transfer strategy class is missing the @AutoMap annotation that all similar Strategy subclasses have (EmailCode, PhoneCode, Password, Ticket, etc.). The special-case handling with mapOf(TRANSFER to "true") appears to work around this:
- Issue: Transfer lacks @AutoMap/Serializable annotations, so
Transfer().toMap()would fail - Special-case value: Sends
"transfer": "true"instead of"transfer": "transfer" - Inconsistency: Other strategies use proper annotations + automatic serialization
Verify one of:
- Add
@AutoMap @Serializableto Transfer class (line 540) like other strategies, then update special-case to useparams.toMap() - Document why Transfer intentionally uses
"true"value and lacks these annotations
Additionally, add tests covering Transfer strategy with locale parameter to ensure the special-case handling remains correct.
🤖 Prompt for AI Agents
In source/api/src/main/kotlin/com/clerk/api/signin/SignIn.kt around lines 621 to
630, the Transfer strategy is being special-cased to send mapOf("transfer" to
"true") because Transfer lacks the @AutoMap/@Serializable annotations other
Strategy subclasses have; fix by adding @AutoMap and @Serializable to the
Transfer class (around line 540) so params.toMap() works and then remove the
special-case branch so the code always uses params.toMap() + ("locale" to
Clerk.locale.value.orEmpty()), or alternatively, if "true" is intentionally
required, add a comment in the Transfer class and in this branch documenting why
the literal "true" must be sent and add unit tests that cover sending Transfer
with locale to verify behavior remains correct.
There was a problem hiding this comment.
@swolfand I think this is right as-is, but I'll leave that to you.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Summary of changes
Adds
localeto the sign-in flow similarly to that of the sign-up flow.(First-ever commit to a Kotlin codebase. 😄)
Summary by CodeRabbit