-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: Migrate navigation to nav3 #554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
02c3452 to
d8a27a2
Compare
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
|
@ovitrif I noticed there is a 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. |
fix: unify timed sheets behavior
…bled-widgets fix: skip auto refresh for disabled widgets
fix: qr code decoding error message
|
@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. |
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