Skip to content

Comments

fix: prevent NoSuchElementException in session chain selection#212

Open
developerfred wants to merge 1 commit intoreown-com:developfrom
developerfred:issue/211
Open

fix: prevent NoSuchElementException in session chain selection#212
developerfred wants to merge 1 commit intoreown-com:developfrom
developerfred:issue/211

Conversation

@developerfred
Copy link

close #211

  • Replace first() with firstOrNull() in getSelectedChain extension function
  • Add null safety check in onSessionApproved to handle empty chains case
  • This resolves crash when AppKit.chains list is empty during session approval

@github-actions
Copy link

github-actions bot commented Oct 9, 2025

Thank you for your contribution! We ask that you please read and sign our CTA Document before we can accept your contribution. You can sign the CTA simply by posting a Pull Request Comment with the following text:


I have read the CTA Document and I hereby sign the CTA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 9, 2025

@developerfred
Copy link
Author

I have read the CTA Document and I hereby sign the CTA

@developerfred
Copy link
Author

recheck

Copy link
Collaborator

@jakubuid jakubuid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Request Changes

Thanks for the contribution and for identifying a real crash — the NoSuchElementException from first() on an empty AppKit.chains is a valid bug. However, this PR has several issues that need to be addressed before it can be merged.


P0 — Won't Compile

1. Broken braces/indentation in AppKitDelegate.kt

The onSessionApproved method has its indentation shifted and introduces an extra closing } that would prematurely close the AppKitDelegate object class, leaving onSessionAuthenticateResponse and all subsequent methods outside the class body:

    override fun onSessionApproved(approvedSession: Modal.Model.ApprovedSession) {
    scope.launch {           // ← wrong indent (4sp instead of 8sp)
        val chain = ...
        ...
    }
}                            // ← extra brace closes the object class

2. Missing import android.util.Log

Log.e(...) is used but the import is never added.

3. Nullable return type breaks all other callers

Changing getSelectedChain from ?: first() to ?: firstOrNull() changes the return type from Modal.Model.Chain to Modal.Model.Chain?. Only onSessionApproved is updated, but the same function is called in several other places that still assume a non-null return:

  • AppKitDelegate.onSessionAuthenticateResponse — accesses chain.id without null check
  • AppKitDelegate.onSessionUpdate — passes chain to toSession() without null check
  • AppKitEngine.connectCoinbase — passes chain to toSession() without null check
  • AppKitState.mapToAccountButtonState — accesses selectedChain.chainImage, .chainName etc. without null check

P1 — Behavioral Issues

4. Silent session event drop

When chain is null in onSessionApproved, the event emission (_wcEventModels.emit(approvedSession)) is skipped. Downstream listeners (UI, state management) will never learn the session was approved, which could cause the modal to hang or appear broken. Silently swallowing events can be harder to debug than the original crash.

5. getChain() still throws

The change from AppKit.chains.first() to AppKit.chains.firstOrNull() ?: throw IllegalStateException(...) just swaps one exception type for another — not a meaningful improvement.


P2 — Unnecessary Changes

6. getDefaultChain() refactoring is a no-op

The original code already returns emptyList() when there are no valid chains, since mapNotNull on an empty list returns an empty list. The added if (chainIds.isNotEmpty()) check is redundant.


Suggestions for a revised approach

  1. Keep getSelectedChain non-nullable and guard at specific call sites, OR make it nullable and update all callers consistently — not just one.
  2. Don't silently drop session events — either always emit the event, or propagate the error through onError() so the UI can react.
  3. Investigate the root cause: why is AppKit.chains empty during onSessionApproved? This may indicate an initialization ordering issue that should be fixed upstream rather than papered over with null checks at every call site.
  4. Maintain the existing 4-space indentation consistent with the rest of the file.

@developerfred Please address the feedback, or let me know so I can push it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New pairing fails over deeplinks if a pairing exists

2 participants