feat(rn-journey): useJourneyForm DX improvements (SDKS-5210) - #66
feat(rn-journey): useJourneyForm DX improvements (SDKS-5210)#66pingidentity-gaurav wants to merge 1 commit into
Conversation
- Typed JourneyNormalizedField discriminated union with named fields per callback type - Typed JourneyCallback union in node.types.ts - form.attempted flag + markAttempted() with automatic reset on node change - DeviceProfileCallback reclassified as integration_required - Dev-mode warning when integration_required callbacks missing from handledCallbackTypes - Node-change value reset moved from render-time setState to useEffect - Sample app updated to use typed field API (field.type shorthand, typed casts over field.raw) - Remove resolvePromptText / resolveContentText from valueReaders (inlined) - check-native-sdk-versions.mjs script + lefthook pre-push hook - Remove top-level headers from all README files Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (47)
✨ 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 |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (66.15%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
============================================
- Coverage 71.96% 71.87% -0.09%
Complexity 230 230
============================================
Files 164 203 +39
Lines 20856 22439 +1583
Branches 748 1043 +295
============================================
+ Hits 15008 16128 +1120
- Misses 5742 6162 +420
- Partials 106 149 +43
... and 39 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
| if (prevFields !== fields) { | ||
| setPrevFields(fields); | ||
| setValuesState(hydrateValues(fields, {})); | ||
| setFormState({ values: hydrateValues(fields, {}), attempted: false }); |
There was a problem hiding this comment.
Would not be better if we move this reset into an effect keyed by fields? This seems to still update state during render.
| * @remarks | ||
| * Extends shared callback shape while widening `type` so native-extension callbacks | ||
| * are represented without type assertions. | ||
| * `output` is the AM-level output array (escape hatch to raw AM data). |
There was a problem hiding this comment.
Does the native mappers do emit a top-level output? I believe the AM output is nested in the raw payload. If the mappers don't do it, since NodeCallback makes this field look available, a consumer could reasonably call callback.output.forEach(...) and hit a runtime error.
| const selected = readNumber( | ||
| currentValue, | ||
| field.type === 'ChoiceCallback' | ||
| ? (field as JourneyChoiceField).defaultChoice |
There was a problem hiding this comment.
Do we need the cast here? I don't think we do, because with your changes, field is already a discriminated union, so typescript should narrow it to JourneyChoiceField in this branch.
| for (const file of gradleFiles) { | ||
| const versions = extractGradleVersions(file); | ||
| if (versions.length > 0) { | ||
| versionByFile.set(file, versions[0]); |
There was a problem hiding this comment.
Should we validate every version found in each file instead of keeping only version[0]?
| } catch { | ||
| return ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
Just a nit: should we also demonstrate gating error display until submission is attempted? :)
|
Good work! Left some minor comments. |
2 participants
Summary
JourneyNormalizedFieldis now a discriminated union narrowed byfield.type; each callback type (ChoiceCallback, TermsAndConditionsCallback, PollingWaitCallback, etc.) exposes named typed fields directly, removing the need forfield.rawcastsform.attempted+markAttempted()— headless flag for gating validation error display on first submit attempt; resets automatically on node changeuseEffectto React-idiomatic "adjust during render" pattern (matchesuseDavinciForm), eliminating the cascading-render lint erroruseJourneyFormwarns whenintegration_requiredcallbacks are present but not listed inhandledCallbackTypesfield.ref.type→field.typeshorthand andfield.rawcasts → typed narrowed access;resolvePromptText/resolveContentTextremoved and inlinedcheck-native-sdk-versions.mjs— script to verify native SDK versions stay in sync; wired to lefthook pre-commit#headings stripped from all package/app READMEsTest plan
yarn test --filter=@ping-identity/rn-journey— 71 tests passyarn typecheck— cleanyarn lint— clean (no errors in journey package)yarn prettier— cleanSuspendedTextOutputCallback,ConsentMappingCallback,FidoRegistrationCallback/FidoAuthenticationCallbackform.attemptedstarts false,markAttempted()flips to true, resets on node change, dev-warn fires/suppressed correctlySummary by CodeRabbit