refactor: Migrate navigation to nav3 #554
Conversation
ovitrif
force-pushed
the
feat/nav3
branch
2 times, most recently
from
02c3452 to
d8a27a2
Compare
ovitrif
force-pushed
the
feat/nav3
branch
5 times, most recently
from
30cfb0d to
ff8c3df
Compare
|
re-drafted because it's not yet ready |
… into fix/uniffy-timed-sheet-behavior # Conflicts: # app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
|
@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. |
|
@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. |
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 |
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.
From what I saw, the failures I commented on don't seem related to sheet dismissal changes. You can check my comments.
"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.
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. 👍
Honestly, I don't think there's "secret knowledge" - it's mostly standard debugging:
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. 🤷♂️ |
|
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 |
This doesn't fix the problem. Issue still observed. Screen.Recording.2025-12-30.at.11.03.21.mov |
Yeah, still fixing on it. |
This is all UI changes, I won't add unit tests for UI. |
Fair enough. |
On the contrary, I want to remove them. Countless times again and again changes that were not done by myself warranted more work that nobody sees which only I have to do. So unless we teach all devs making implementation changes on Android to build this way, I personally I am better off without that code. It's been > 6 months and it's not adding any value, but on the contrary it's adding more work ONLY for me. 
|
|
What I really think is needed here but there's no reason to draw conclusions earlier than needed:
This is what I always do, there's no reason to impose limits on the scope and reach of any refactoring, either done by AI or manually. This is simply a DRAFT PR which is not ready, thus expect anything in here, even it getting dropped if there's no time or real ROI in pursuing to get it merged vs. the cost it will take to do so. Same about the e2e changes on the sibling branch. There's no reason to think and setup limits to the scope of what experiments should be allowed to do. The fact that Joao wants to help to get this PR merged sooner is simply a nice gesture that has to do with what I am open to, which is to help. I don't need lessons about how many things I should be allowed to change and how to approach my changes. Sorry but let's get back to reality: Sure, it will look like hell if you test it, sure you can start drawing conclusions about AI doing too much or relying too much on AI. It's like starting to review my 1st iteration on some manual implementation. You'll be free to say it's sheet, because it is. Then we only create unnecessary anxiety like my attempts to one-shot a "fix" without manually testing, like here. I knew the fix is irrelevant yesterday after writing that comment and actually doing a quick sanity check thru manual testing. I was thinking to write a follow-up reply saying this, but then I also still think from the same angle: it doesn't even matter, because it's a draft PR, so I didn't mind being wrong and won't ever feel as having to be right on a draft PR. |
Fair enough. My impression was that it was not a draft (it wasn't in draft state I think) and there were also a lot of other PRs with various fixes pointing to merge to this branch. I thought it is ready for review, since there was some review already, so started to analyze e2e test results and adding comments. Sorry for misunderstanding 🙏 Apologies if my comments were insensitive and if I drawn false conclusions. My primary care is good quality of the code and product and that's where I'm coming from 🙇 If there is impression I'm pointing fingers - I'm not, I want to focus on fixing the issue if I think I see it. Ofc, I may be wrong trying to, and always happy to be proven wrong... Note, I should be grimacing and unhappy, most of the time, that's what testers do ;P |
No worries, it's indeed much harder than I was expecting it to be, as I've been dreaming of finishing this last Tuesday and another week passed by while I still have to fix the most obvious 1st glance issues 😢 |
|
I think that if it is a Draft/experimentation kind of thing, then no problem. But then also we shouldn't merge more (unrelated to refactor) fixes/changes into it. If it is a crucial refactor before the release, I guess it is a bit of a risky item at this point (may be wrong, but just feels like it). What do you think @ovitrif @jvsena42 ? |
Will see how it evolves, It's definitely not ready yet 🙃 My reason for actually doing this work is that I keep bashing on Android core devs for over 3+ years about not having a proper navigation library. Now they finally did, and I suffered enough with the old version of Compose Navigation to even want to do any more screens with that lame setup 😄 So, that's why I started this. If this Nav3 would've been available like 1 year ago, at least, we would've spent 1/3 of the time lost on on UI/ inter-screen nav works. It's THAT critical… But yeah, not the current version of this PR, not good at all yet, needs much more iteration still, apparently. |
We will play safe, so I'll reopen the PRs merged in this branch, now targeting master |
|
The issues related to in-sheet navigation and sheets overall might be fixed in 3ad4a88, at least it appears so in my brief manual testing session. |
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:
navpackageRoutes.ktsealed interfaceNavigatorclass centralising navigation logicSheetSceneStrategyfor custom bottom sheetsBottomSheetScaffoldwith custom sheet implementationnav/entries/for all screen and sheet routesNavControllertoNavigatorpatternPreview
nav3.mp4
QA Notes