Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThree new JSON asset configuration files (Journey, DaVinci, OpenID) are added to the sample app's assets directory. ChangesConfiguration Presets, Asset Loading, and JSON Migration
Token and UserInfo Exception Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 detekt (1.23.8)samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.ktException in thread "main" com.beust.jcommander.ParameterException: Provided path 'detekt-config.yaml' does not exist! samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/userprofile/UserProfileViewModel.ktException in thread "main" com.beust.jcommander.ParameterException: Provided path 'detekt-config.yaml' does not exist! samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/TokenViewModel.ktException in thread "main" com.beust.jcommander.ParameterException: Provided path 'detekt-config.yaml' does not exist! 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 |
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt`:
- Around line 183-186: The userinfoEndpoint field is being parsed with an
incorrect JSON key name that has a capital I in "userInfoEndpoint", but the
actual key in the openIdJsonConfig.json file uses lowercase i as
"userinfoEndpoint". Change the JSON key parameter in the line assigning
userinfoEndpoint from "userInfoEndpoint" to "userinfoEndpoint" to match the
actual configuration file key and ensure the value is correctly populated
instead of defaulting to an empty string.
- Around line 135-137: The loadAssetConfigs() method opens file streams via
context.assets.open(fileName).bufferedReader() without closing them, causing
resource leaks in the loop. Wrap the bufferedReader().readText() call with the
Kotlin .use { } function to ensure the stream is automatically closed after
reading, preventing file descriptor leaks on each iteration of the loop.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4eb46e48-cd4a-4f03-9ff2-1f5dd43e1cba
📒 Files selected for processing (4)
samples/pingsampleapp/src/main/assets/DaVinciJsonConfig.jsonsamples/pingsampleapp/src/main/assets/journeyJsonConfig.jsonsamples/pingsampleapp/src/main/assets/openIdJsonConfig.jsonsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt
| authorizationEndpoint = openIdObj?.str("authorizationEndpoint") ?: "", | ||
| tokenEndpoint = openIdObj?.str("tokenEndpoint") ?: "", | ||
| userinfoEndpoint = openIdObj?.str("userInfoEndpoint") ?: "", | ||
| endSessionEndpoint = openIdObj?.str("endSessionEndpoint") ?: "", |
There was a problem hiding this comment.
Device-auth userinfoEndpoint is parsed with the wrong JSON key.
openIdJsonConfig.json uses userinfoEndpoint, but this parser reads userInfoEndpoint (capital I), so the value is always empty and the loaded device-auth preset is incomplete.
Suggested fix
- userinfoEndpoint = openIdObj?.str("userInfoEndpoint") ?: "",
+ userinfoEndpoint = openIdObj?.str("userinfoEndpoint") ?: "",🤖 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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt`
around lines 183 - 186, The userinfoEndpoint field is being parsed with an
incorrect JSON key name that has a capital I in "userInfoEndpoint", but the
actual key in the openIdJsonConfig.json file uses lowercase i as
"userinfoEndpoint". Change the JSON key parameter in the line assigning
userinfoEndpoint from "userInfoEndpoint" to "userinfoEndpoint" to match the
actual configuration file key and ensure the value is correctly populated
instead of defaulting to an empty string.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #212 +/- ##
=============================================
+ Coverage 44.12% 44.14% +0.02%
Complexity 1393 1393
=============================================
Files 316 316
Lines 9756 9756
Branches 1495 1495
=============================================
+ Hits 4305 4307 +2
Misses 4887 4887
+ Partials 564 562 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
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
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt`:
- Around line 198-204: The web.add() method call with OidcConfigState parameters
(containing clientId, discoveryEndpoint, scopes, redirectUri, and display
fields) is executing unconditionally after the when block, causing config
entries to be added to multiple lists when they should only appear in one.
Delete this entire web.add() block (lines 198-204) so that each config type is
only added to its appropriate list as determined by the when expression.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80b5a3ad-dea8-4d15-9a67-093a168e1705
📒 Files selected for processing (5)
samples/pingsampleapp/src/main/assets/DaVinciJsonConfig.jsonsamples/pingsampleapp/src/main/assets/journeyJsonConfig.jsonsamples/pingsampleapp/src/main/assets/openIdJsonConfig.jsonsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/Env.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- samples/pingsampleapp/src/main/assets/DaVinciJsonConfig.json
- samples/pingsampleapp/src/main/assets/journeyJsonConfig.json
- samples/pingsampleapp/src/main/assets/openIdJsonConfig.json
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
vibhorgoswami
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (3)
samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt (1)
477-480: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider moving SDK instance builds off Main dispatcher.
The SDK instance builders (
buildJourneyInstance, etc.) execute onDispatchers.Mainafter the context switch. If these perform any blocking I/O (secure storage reads, network setup), this could cause UI jank on app launch.For a sample app this may be acceptable, but production code should consider:
- Building instances on IO dispatcher
- Using a loading state to defer UI interaction until ready
🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt` around lines 477 - 480, Move the SDK instance builder calls (buildJourneyInstance, buildDaVinciInstance, buildWebInstance, buildDeviceAuthInstance) off the Main dispatcher to prevent blocking UI operations. Use Dispatchers.IO for these blocking I/O operations by wrapping the builder calls in a withContext(Dispatchers.IO) block. Consider implementing a loading state variable that is set to true before starting the builders and false after completion, allowing the UI to defer interactions until all SDK instances are fully initialized.samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/TokenViewModel.kt (1)
122-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging exceptions before discarding them.
All catch blocks silently clear state without any logging, making it difficult to diagnose unexpected failures during development. The user sees a generic "No token information available" message with no way to distinguish between "no session exists" and "an unexpected error occurred."
Adding a log statement would aid debugging while preserving the graceful degradation behavior:
} catch (e: Exception) { + Log.w("TokenViewModel", "Unexpected error during token operation", e) state.update { it.copy(journeyToken = null, journeyError = null) } }Also applies to: 152-154, 172-174, 202-204, 222-224, 252-254, 272-274, 302-304
🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/TokenViewModel.kt` around lines 122 - 124, In the TokenViewModel class, there are multiple catch blocks that silently discard exceptions without logging them, making it difficult to diagnose unexpected failures during development. Add a logging statement to each catch block (lines 122-124, 152-154, 172-174, 202-204, 222-224, 252-254, 272-274, and 302-304) that logs the exception details before the state.update call that clears the error state. This will preserve the graceful degradation behavior while providing diagnostic information by capturing the actual exception message and stack trace.samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/userprofile/UserProfileViewModel.kt (1)
114-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging exceptions before discarding them.
Same as TokenViewModel—exceptions are silently discarded, making unexpected failures invisible during development. Adding a log would help diagnose issues:
} catch (e: Exception) { + Log.w("UserProfileViewModel", "Unexpected error fetching user info", e) state.update { s -> s.copy(journeyUser = null, journeyError = null) } }Also applies to: 139-141, 164-166, 189-191
🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/userprofile/UserProfileViewModel.kt` around lines 114 - 116, The catch blocks in UserProfileViewModel are silently discarding exceptions without logging them, making it difficult to diagnose unexpected failures during development. Add logging statements in all the catch blocks (the one shown in the diff and the additional ones at the other specified locations) to log the exception details before updating the state. Use an appropriate logger to record the exception with context about which operation failed, so that unexpected errors are visible and debuggable.
🤖 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.
Nitpick comments:
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.kt`:
- Around line 477-480: Move the SDK instance builder calls
(buildJourneyInstance, buildDaVinciInstance, buildWebInstance,
buildDeviceAuthInstance) off the Main dispatcher to prevent blocking UI
operations. Use Dispatchers.IO for these blocking I/O operations by wrapping the
builder calls in a withContext(Dispatchers.IO) block. Consider implementing a
loading state variable that is set to true before starting the builders and
false after completion, allowing the UI to defer interactions until all SDK
instances are fully initialized.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/TokenViewModel.kt`:
- Around line 122-124: In the TokenViewModel class, there are multiple catch
blocks that silently discard exceptions without logging them, making it
difficult to diagnose unexpected failures during development. Add a logging
statement to each catch block (lines 122-124, 152-154, 172-174, 202-204,
222-224, 252-254, 272-274, and 302-304) that logs the exception details before
the state.update call that clears the error state. This will preserve the
graceful degradation behavior while providing diagnostic information by
capturing the actual exception message and stack trace.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/userprofile/UserProfileViewModel.kt`:
- Around line 114-116: The catch blocks in UserProfileViewModel are silently
discarding exceptions without logging them, making it difficult to diagnose
unexpected failures during development. Add logging statements in all the catch
blocks (the one shown in the diff and the additional ones at the other specified
locations) to log the exception details before updating the state. Use an
appropriate logger to record the exception with context about which operation
failed, so that unexpected errors are visible and debuggable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4ddf9e6-2a9a-4ad4-90c3-cc1af9ec8bf7
📒 Files selected for processing (3)
samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/config/EnvViewModel.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/token/TokenViewModel.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/userprofile/UserProfileViewModel.kt
vibhorgoswami
had a problem deploying
to
github-pages
GitHub Actions
Error
3 participants
JIRA Ticket
SDKS-5184
Description
This PR provides an option to load json files of configuration.
Summary by CodeRabbit