Pre-release hardening: parseAsset, sheet model, navigation contract, delete default - #3491
Merged
Conversation
It indexed the global Symbol constructor, so Symbol['HIVE'] was undefined for all input. Delegate the string case to the SDK's parseAsset, which owns the enum. Keep the local non-string guard rather than re-exporting the SDK function: the SDK version treats anything non-string as an SMTAsset and reads sval.amount, and postParser calls this on max_accepted_payout and the payout fields, which the search API omits. Closes #3488
CLAUDE.md was fixed in #3467 but the comments written against the old claim were not. In actions-sheet 0.9.7 SheetProvider renders the sheet only while visible, so sheets mount on show and unmount on hide. Read each file's cleanups and reset-on-payload logic against the correct model. Only composeTranslateModal needed a code change: closedRef, which cancels the in-flight translation chain, was set from onClose alone, so a sheet that went away by any other route left the chain paging the translation service. Set it from unmount as well. The other five are comment-only; their cleanups touch no native or Expo shared object, which is what made the dictation crash fatal. Closes #3476
…target Leftovers from the IconButton consolidation in #3461. The typed IconButton's base style is a fixed 30x30 with no padding, so style={{ paddingRight: 0 }} cancelled nothing; the commented-out paddingRight in closeIcon was stale for the same reason. 30x30 is below the 44pt guideline and the button sits in a row of transfers where a mis-tap cancels the wrong schedule, so add a hitSlop pass-through to IconButton and opt in here. The prop is optional and unset everywhere else, so no other icon button changes. Closes #3463
RootNavigation.navigate took 'any' and cast the ref, so all 51 callers
bypassed AppParamList, including deep-link dispatch, the PIN screen's
forwarding, wallet and asset details. Type it against AppParamList.
ForwardedNavigation left navigateParams optional for every route, so a
paramless forward to a required-param destination type-checked. Make it
required exactly when the destination's params exclude undefined.
Deriving that optionality makes PINCODE circular, since PinCodeParams embeds
ForwardedNavigation, so ParamsOptional answers PINCODE without probing it.
Fixing the call sites surfaced four dead or wrong params, none of them read
by their destination:
- commentBodyView passed a navigation key inside params; postBodyView does
the same navigation without it
- useLinkProcessor passed accessToken to PINCODE and DRAWER.MAIN
- useInitApplication passed activePage to the wallet tab
- postHeaderDescription fell through to navigate({}) when content matched
none of its four branches
deepLinkParser's route name is now RouteName rather than string, so a route
that does not exist fails at the branch that built it.
Closes #3462
_deletePost called navigation.goBack() unless the consumer passed onDelete. That default is wrong for any consumer owning a surrounding list, and it has been hit three times: waves, the comment surfaces (#3405, #3406) and the feed list (#3407). Invert it into an explicit popScreenOnDelete opt-in. Forgetting it leaves the user on the screen, which is mildly wrong and visible in testing; forgetting onDelete under the old default navigated them away, which is badly wrong and easy to miss. Content shape cannot answer this, as #3408 showed: postScreen renders comments and waves as primary content, so a comment in a list and a comment as the screen are the same object. postScreen is the only consumer that needs the opt-in. Its modal is opened solely from the header dropdown, which always acts on the post the screen is; comments below it carry their own modal and onDelete. The four list consumers already pass onDelete and are unaffected. Closes #3409
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds typed route navigation contracts, updates navigation call sites, prevents stale translation updates, makes post-deletion navigation configurable, expands a button touch target, clarifies sheet lifecycle comments, and replaces manual asset parsing with SDK parsing and tests. ChangesNavigation contracts and call sites
Sheet lifecycle handling
UI interaction and deletion behavior
Asset parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 `@src/components/accountsBottomSheet/view/accountsBottomSheetView.tsx`:
- Line 30: Require route parameters conditionally using ParamsOptional<K> so
routes such as WEB_BROWSER cannot be called without their required params.
Update navigateToRoute in
src/components/accountsBottomSheet/view/accountsBottomSheetView.tsx#L30-L30 and
_navigateToRoute in
src/components/accountsBottomSheet/container/accountsBottomSheetContainer.tsx#L70-L75
to use the conditional rest-tuple signature; in
src/screens/perks/children/spendOptions.tsx#L12-L12 replace params?: any with a
route-correlated route-and-params discriminated union, and apply the same
conditional signature to _navigate at
src/screens/perks/children/spendOptions.tsx#L38-L40.
In `@src/components/composeTranslateModal/composeTranslateModal.tsx`:
- Around line 59-61: Update the translation flow around translateMarkdown and
the existing close/unmount handlers to use a per-run generation token. Increment
or otherwise invalidate the generation on close, unmount, payload changes, and
before starting a new translation, then require the captured run generation to
match before every setProgress, setTranslated, setFailed, and setTranslating
call so stale requests cannot update a reopened sheet.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 289d66f6-251c-4374-bdb9-928195f74fac
📒 Files selected for processing (25)
src/components/accountsBottomSheet/container/accountsBottomSheetContainer.tsxsrc/components/accountsBottomSheet/view/accountsBottomSheetView.tsxsrc/components/aiAssistModal/aiAssistModal.tsxsrc/components/balanceAnalyticsSheet/balanceAnalyticsSheet.tsxsrc/components/communityRoleEditSheet/communityRoleEditSheet.tsxsrc/components/composeTranslateModal/composeTranslateModal.tsxsrc/components/foregroundNotification/foregroundNotification.tsxsrc/components/iconButton/view/iconButtonView.tsxsrc/components/post-translation-modal/postTranslationModal.tsxsrc/components/postElements/body/view/commentBodyView.tsxsrc/components/postElements/headerDescription/view/postHeaderDescription.tsxsrc/components/postOptionsModal/container/postOptionsModal.tsxsrc/components/qrModal/qrModalView.tsxsrc/hooks/useLinkProcessor.tsxsrc/navigation/rootNavigation.tsxsrc/navigation/types.tssrc/screens/application/hook/useInitApplication.tsxsrc/screens/assetDetails/children/children.styles.tssrc/screens/assetDetails/children/recurrentTransfersModal.tsxsrc/screens/assetDetails/screen/assetDetailsScreen.tsxsrc/screens/perks/children/spendOptions.tsxsrc/screens/post/screen/postScreen.tsxsrc/utils/deepLinkParser.tssrc/utils/parseAsset.test.tssrc/utils/parseAsset.ts
💤 Files with no reviewable changes (2)
- src/components/postElements/body/view/commentBodyView.tsx
- src/screens/assetDetails/children/children.styles.ts
Review follow-up. The two (route, params) helpers used params?: AppParamList[K], whose ? makes the argument optional for every route regardless of contract, so a paramless call to WEB_BROWSER or VOTERS type-checked and the NavigateOptions cast then swallowed it. That is the same hole this PR closed in ForwardedNavigation, reintroduced one layer up. Add NavigateArgs, a conditional tuple that makes params required exactly when ParamsOptional is false, and use it for both helpers. spendOptions now carries a whole NavigateOptions per entry instead of a route plus params?: any, so each entry is checked against its own destination in the literal and the helper and its cast are gone.
… current Review follow-up. The closed flag added earlier stopped the sheet paging the service after it went away, but it did not stop a run writing its result back. setTranslated was not gated on it at all, and the reset effect cleared it again on re-show. The visible failure is not the reopen case, where a fresh mount gets a fresh ref: it is switching target language mid-run. The effect above clears translated for exactly that reason, then the in-flight run resolves and writes the old pair's result into the new one. Replace the flag with a run id, bumped on close, unmount, re-show, language pair change and each new translation, and gate every write on it. That covers cancellation and staleness with one mechanism.
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 `@src/components/composeTranslateModal/composeTranslateModal.tsx`:
- Around line 130-136: Update the language-pair cancellation useEffect around
runIdRef.current, setTranslated, and setFailed to also call
setTranslating(false). Preserve the existing run invalidation and
result-clearing behavior so changing source or target immediately clears the
loading state.
In `@src/navigation/types.ts`:
- Around line 34-36: The NavigateArgs<K> conditional in types.ts must distribute
over union route names so each route is checked against its own ParamsOptional
and parameter type. Update the type using a distributive K-based conditional,
preserving optional params only for routes where ParamsOptional<K> is true, and
add a compile-only negative test proving a RouteName-typed WEB_BROWSER route
cannot be paired with undefined instead of the required { url: string }
parameters.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88faeeeb-50ad-49ae-b38b-ae5516844769
📒 Files selected for processing (5)
src/components/accountsBottomSheet/container/accountsBottomSheetContainer.tsxsrc/components/accountsBottomSheet/view/accountsBottomSheetView.tsxsrc/components/composeTranslateModal/composeTranslateModal.tsxsrc/navigation/types.tssrc/screens/perks/children/spendOptions.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/accountsBottomSheet/view/accountsBottomSheetView.tsx
- src/components/accountsBottomSheet/container/accountsBottomSheetContainer.tsx
- src/screens/perks/children/spendOptions.tsx
Review follow-up. The conditional checked ParamsOptional<K>, not K, so it did not distribute. Instantiated with the whole RouteName union it collapsed to 'boolean extends true' = false, yielding [RouteName, AppParamList[RouteName]], whose params union already includes undefined. A route held in a RouteName variable therefore paired with undefined even for WEB_BROWSER. Distributing produces one tuple per route, so a union-typed route matches none of them. Literal call sites are unaffected: K infers to the literal and distribution over a single member is the same type.
…es a run Review follow-up, and a regression from the run-id change. Gating every write on isCurrent() meant a run cancelled by a source/target change could no longer clear translating in its finally block, where the previous closed-flag check did. The spinner stayed up and both action buttons stayed disabled until the sheet was reopened. Clear it in the effect that retires the run, alongside the state it already resets.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bundles four hardening issues filed after the recent wallet and sheet work, plus the review leftovers from #3461. One commit per issue.
#3488 parseAsset returned undefined for every symbol
src/utils/parseAsset.tsindexed the globalSymbolconstructor, soSymbol['HIVE']wasundefinedfor all input. Latent, since all 11 call sites read.amountonly.The string case now delegates to the SDK's
parseAsset, which owns the enum. The local non-string guard stays: the SDK version treats anything non-string as an SMTAsset and readssval.amount, andpostParser.tsx:142calls this onmax_accepted_payout, which the search API omits (the_isNumericPayoutOnlybranch right below it exists for exactly that shape). Re-exporting the SDK function directly would throw there. Adds the first test coverage for the helper.#3476 six sheets documented the wrong mount model
CLAUDE.mdwas corrected in #3467; the comments were not. Sheets mount on show and unmount on hide.Each file's unmount cleanups and reset-on-payload logic were read against the correct model. Five are comment-only.
composeTranslateModalneeded a code change:closedRef, which cancels the in-flight translation chain, was set fromonClosealone, so a sheet leaving by any other route left the chain paging the translation service. It is now set from unmount as well.#3463 recurrentTransfersModal close button
style={{ paddingRight: 0 }}cancelled nothing (the typed IconButton's base style is a fixed 30x30 with no padding), and the commented-outpaddingRightincloseIconwas stale for the same reason. Both removed.30x30 is below the 44pt guideline and the button sits in a row where a mis-tap cancels the wrong schedule, so
IconButtongains an optionalhitSloppass-through, opted into at this one call site. Unset everywhere else, so no other icon button changes.#3462 navigation contract bypass gaps
RootNavigation.navigatetookanyand cast the ref, so all 51 callers bypassedAppParamList. Now typed against it.ForwardedNavigationleftnavigateParamsoptional for every route; it is now required exactly when the destination's params excludeundefined.Deriving that optionality makes
PINCODEcircular, sincePinCodeParamsembedsForwardedNavigation, soParamsOptionalanswersPINCODEwithout probing its params.Typing it surfaced four dead or wrong params, none read by their destination:
commentBodyViewparams;postBodyViewdoes the same navigation without ituseLinkProcessoraccessTokento PINCODE and DRAWER.MAINuseInitApplicationactivePageto the wallet tabpostHeaderDescriptionnavigate({})whencontentmatched none of its four branchesdeepLinkParser's route name is nowRouteNamerather thanstring, so a nonexistent route fails at the branch that built it. Where a route name and its params are separate locals by the time they reach dispatch, the pairing cannot be checked and the cast is commented as the seam.#3409 options sheet delete now fails safe
_deletePostcallednavigation.goBack()unless the consumer passedonDelete, wrong for any consumer owning a surrounding list, and hit three times (waves, the comment surfaces in #3405/#3406, the feed list in #3407). Inverted into an explicitpopScreenOnDeleteopt-in.postScreenis the only consumer needing it: its modal is opened solely from the header dropdown, which always acts on the post the screen is, and comments below carry their own modal andonDelete. Note the issue's table listseditorScreenas a sixth consumer, butsrc/screens/editor/children/postOptionsModal.tsxis an unrelated component (beneficiaries, scheduling, thumbnails), so there are five.Verification
tsc --noEmitclean against the empty baseline, eslint 0 errors (one fewer warning thandevelopment), 821 unit tests pass. No device pass, and none of this is on a screen the wallet device pass covers.Summary by CodeRabbit
New Features
Bug Fixes
Tests