Skip to content

Conversation

@ovitrif
Copy link
Collaborator

@ovitrif ovitrif commented Dec 23, 2025

Resolves #480
Closes #180

Description

This PR migrates the entire navigation system from legacy Compose Navigation (NavController) to the newly stable Navigation 3 (Nav3).

Key changes:

  • Updated all navigation usage with Nav3 primitives
  • Moved all navigation code to its dedicated nav package
  • Moved all type-safe route definitions to Routes.kt sealed interface
  • Introduced Navigator class centralising navigation logic
  • Added SheetSceneStrategy for custom bottom sheets
  • Replaced material3 BottomSheetScaffold with custom sheet implementation
  • Defines all entry handlers in nav/entries/ for all screen and sheet routes
  • Removed legacy code and old sheet implementations
  • Updated 55+ screens from NavController to Navigator pattern
  • Net reduction of +3k lines of code
  • TabBar now slides out below the viewport when showing sheets or non-wallet screens

Preview

nav3.mp4

QA Notes

  • Verify all screen navigation works (back buttons, forward navigation)
  • Test all bottom sheets open and close correctly
  • Verify sheet dismiss behavior (swipe down, back button, overlay tap)
  • Test deep link handling
  • Test timed sheets (backup reminder, app update, etc.)
  • Verify drawer menu navigation
  • Test QR scanner navigation flow
  • Verify settings screens navigation hierarchy
  • Verify app with pin on launch enabled
  • Verify auto-read clipboard enabled

@ovitrif ovitrif force-pushed the feat/nav3 branch 2 times, most recently from 02c3452 to d8a27a2 Compare December 23, 2025 15:34
@ovitrif ovitrif force-pushed the feat/nav3 branch 5 times, most recently from 30cfb0d to ff8c3df Compare December 24, 2025 10:13
@ovitrif ovitrif requested a review from jvsena42 December 24, 2025 12:58
@ovitrif ovitrif marked this pull request as ready for review December 24, 2025 12:58
@ovitrif ovitrif removed the request for review from jvsena42 December 24, 2025 13:01
@ovitrif ovitrif marked this pull request as draft December 24, 2025 13:01
@ovitrif
Copy link
Collaborator Author

ovitrif commented Dec 24, 2025

re-drafted because it's not yet ready

jvsena42 and others added 3 commits December 24, 2025 10:14
… into fix/uniffy-timed-sheet-behavior

# Conflicts:
#	app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
@piotr-iohk
Copy link
Collaborator

@ovitrif I noticed there is a feat/nav3 branch on bitkit-e2e-tests. I made a PR of it and added some comments: synonymdev/bitkit-e2e-tests#81

Unfortunately the changes there (that were made by Claude I presume) are not too good. Some are unrelated and should not be added IMHO (changes to wdio.conf file). Some changes try to cover a potential bugs that were introduced in this PR, see: https://github.com/synonymdev/bitkit-e2e-tests/pull/81/files#r2651133046

I see this refactor touches a lot of code and, unfortunately, introduces quite a lot of regressions. It would be good for the future to have more targeted and scoped changes that can be reviewed and tested thoroughly and with understanding. This one feels like AI introducing massive refactoring and then adjusting the e2e tests such that it is green, but unfortunatelly it covers some bugs it introduces with some workarounds in e2e tests. This is too much.

@ovitrif
Copy link
Collaborator Author

ovitrif commented Dec 29, 2025

@piotr-iohk The issues we get are mostly due to critical changes in UX which have long been desired.

I understand your frustration and totally resonate with your request to make changes more contained, but I don't think it's targeted in this specific case, entirely accurate 🙃.

The funny thing is that most of the relevant issues are stemming from targeted changes, and then there's the little flaws here and there which claude did, those need simple fixes like the one in this commit: e6a9de6.


At least so far my understanding is that many issues with e2e stem from the way sheets are getting dismissed now, which needs more swipe-down or a different API call to swipe down.

This was fully my own intent to chage the way sheets are getting dismissed, because the default material3 implementation from Google/Android is too rigid and not flexible enough for our UX needs.

Maybe there's other reasons which we can get to once we polish this more 🤷🏻

PS. The changes in the e2e were never meant to be pushed even, I was only playing with AI and trying to get it to understand how to run the suite so I don't need CI to figure out if my changes are breaking the e2e or not.

It's normal that most of it could be slop, I can even intuitively tell because it fails in over 50% of times when it tries to run them locally. So there's clearly lack of understanding for Claude on what needs to be done and how.

@ovitrif
Copy link
Collaborator Author

ovitrif commented Dec 29, 2025

@piotr-iohk IDK if it makes sense but I would love to have more readily-available setup for AI to be able to work with the 2 repos nicer. Right now it's too bad, it can't really reliably run e2e for some big changes, and it doesn't understand what is broken and whether it's because of the changes in Android or because of some other setup issues; it keeps defaulting to hallucinations about the e2e not working correctly ever, LOL.

I'd very much appreciate if you can take time one of these days to setup a proper AGENTS.md file where you'd put some of your "secret knowledge" so that we can get more success rates with adjusting the e2e properly by other non-test engineer devs like myself 🙏🏻

Plus, I need to repeat the same story over and over again, telling to Claude to read the CI workflows and figure out the mirroring of branch names and the likes.

@ovitrif ovitrif linked an issue Dec 29, 2025 that may be closed by this pull request
@ovitrif
Copy link
Collaborator Author

ovitrif commented Dec 29, 2025

There's unnecessary Backup sheet after doing "Backup your wallet" from Settings (shouldn't be there according to design). Also the wallet shows 24-word phrase consisting from only secret words. Tap to reveal state is not reseted as well.

There was a bug at the foundational layer of in-sheet navigation which is the root of all of the issues of this type.

The fix has landed in f8ccbbb

@piotr-iohk
Copy link
Collaborator

The funny thing is that most of the relevant issues are stemming from targeted changes, and then there's the little flaws here and there which claude did, those need simple fixes like the one in this commit: e6a9de6.

I'm not seeing it that way. A lot of different issues surfaced just from the e2e runs, and I haven't had time to analyze every failure yet - so there may be more. Also, there could be regressions e2e won't catch. Given the size of the refactor, I'd expect to see unit tests added or updated alongside it. I'm not seeing much of that, which makes me worry we're leaning heavily on e2e coverage.

At least so far my understanding is that many issues with e2e stem from the way sheets are getting dismissed now, which needs more swipe-down or a different API call to swipe down.

From what I saw, the failures I commented on don't seem related to sheet dismissal changes. You can check my comments.

PS. The changes in the e2e were never meant to be pushed even, I was only playing with AI and trying to get it to understand how to run the suite so I don't need CI to figure out if my changes are breaking the e2e or not.

"e2e were never meant to be pushed even" - that's a bit concerning from a process standpoint. For changes that land in the main repos (especially prod code), it'd be good to keep commits intentional and reviewable.
On running e2e locally: I don't think it is productive to run all e2e tests locally to check if anything breaks. I think locally you might want to run some individual tests or smaller groups.

It's normal that most of it could be slop, I can even intuitively tell because it fails in over 50% of times when it tries to run them locally. So there's clearly lack of understanding for Claude on what needs to be done and how.

This is a bit surprising. e2e has quite extensive README in terms of how to run the tests locally, what are the prerequisites, steps or helper scripts. Codex has no issue running e2e tests locally. But If there's anything missing we can surely document it. 👍

I'd very much appreciate if you can take time one of these days to setup a proper AGENTS.md file where you'd put some of your "secret knowledge" so that we can get more success rates with adjusting the e2e properly by other non-test engineer devs like myself 🙏🏻

Honestly, I don't think there's "secret knowledge" - it's mostly standard debugging:

  • identify exactly where the test fails (line + error),
  • use CI recordings/screenshots when available,
  • rerun locally and observe what happens. 👀

Where AI tends to struggle is that it doesn't reliably understand what's happening on-screen (👀), so it can end up "fixing" tests in the wrong direction. I've hit this too when AI-made product changes break tests and the suggested test changes don't match reality. That is why I'm a bit reluctant to fully trust AI to adjust the test code after it's changes.

That said, we can definitely improve clarity in the tests and docs; there’s backlog for it (e.g. synonymdev/bitkit-e2e-tests#63)

My main ask is just to be more conservative about trusting AI for broad refactors. More scoped, targeted changes are easier to review, test, and reason about - and we’re ultimately the ones who need to understand and approve them. 🤷‍♂️

@piotr-iohk
Copy link
Collaborator

Receive -> Edit takes whole screen (should be contained in the sheet) and cannot go back.

Screen.Recording.2025-12-30.at.09.19.40.mov

@piotr-iohk
Copy link
Collaborator

There's unnecessary Backup sheet after doing "Backup your wallet" from Settings (shouldn't be there according to design). Also the wallet shows 24-word phrase consisting from only secret words. Tap to reveal state is not reseted as well.

There was a bug at the foundational layer of in-sheet navigation which is the root of all of the issues of this type.

The fix has landed in f8ccbbb

This doesn't fix the problem. Issue still observed.
It's a bit worse now, as the tap to reveal expands on the whole screen and one cannot swipe down to fold it...

Screen.Recording.2025-12-30.at.11.03.21.mov

@ovitrif
Copy link
Collaborator Author

ovitrif commented Dec 30, 2025

There's unnecessary Backup sheet after doing "Backup your wallet" from Settings (shouldn't be there according to design). Also the wallet shows 24-word phrase consisting from only secret words. Tap to reveal state is not reseted as well.

There was a bug at the foundational layer of in-sheet navigation which is the root of all of the issues of this type.
The fix has landed in f8ccbbb

This doesn't fix the problem. Issue still observed. It's a bit worse now, as the tap to reveal expands on the whole screen and one cannot swipe down to fold it...

Screen.Recording.2025-12-30.at.11.03.21.mov

Yeah, still fixing on it.

@ovitrif
Copy link
Collaborator Author

ovitrif commented Dec 30, 2025

I'm not seeing it that way. A lot of different issues surfaced just from the e2e runs, and I haven't had time to analyze every failure yet - so there may be more. Also, there could be regressions e2e won't catch. Given the size of the refactor, I'd expect to see unit tests added or updated alongside it. I'm not seeing much of that, which makes me worry we're leaning heavily on e2e coverage.

This is all UI changes, I won't add unit tests for UI.

@piotr-iohk
Copy link
Collaborator

I'm not seeing it that way. A lot of different issues surfaced just from the e2e runs, and I haven't had time to analyze every failure yet - so there may be more. Also, there could be regressions e2e won't catch. Given the size of the refactor, I'd expect to see unit tests added or updated alongside it. I'm not seeing much of that, which makes me worry we're leaning heavily on e2e coverage.

This is all UI changes, I won't add unit tests for UI.

Fair enough.
Although, given the scope of the refactor, perhaps it would warrant to revive and expand ui-tests.yml. I think that some targeted nav ui tests (without the full infra of e2e) would be good to have. I get that full unit testing doesn't make sense here but having a thinner UI layer of checks could still help.

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.

refactor: use Nav3, pass rust types to screens and add navigation viewmodel Decrease bottom sheet swipe-to-dismiss sensitivity

4 participants