Skip to content

Fix missing MAUI Shell navigation breadcrumbs on iOS #4006

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

Conversation

albyrock87
Copy link
Contributor

Fixes #3999

@albyrock87 albyrock87 force-pushed the fix-maui-navigation-events-missing-on-ios branch from e7f3b63 to 3d5def7 Compare February 28, 2025 15:20
Copy link

codecov bot commented Mar 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.57%. Comparing base (495e742) to head (460e748).
Report is 509 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4006      +/-   ##
==========================================
+ Coverage   75.73%   76.57%   +0.84%     
==========================================
  Files         357      397      +40     
  Lines       13466    14517    +1051     
  Branches     2671     2929     +258     
==========================================
+ Hits        10198    11116     +918     
- Misses       2593     2685      +92     
- Partials      675      716      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jamescrosswell
Copy link
Collaborator

Thanks for the PR @albyrock87 ! This looks great ❤️

Can think of any way we could add automated tests for this? I'm thinking something like these... not sure how easy/feasible it is to test this particular case though.

The only other thing is we need a small description in the CHANGELOG.md file to explain to other users what has changed. In this case, I think the change is a Feature... some people might argue it's a Fix. I'm OK with either of those - it's all just how you squint at it.

@albyrock87 albyrock87 force-pushed the fix-maui-navigation-events-missing-on-ios branch from 3d5def7 to 7c12652 Compare March 3, 2025 15:58
@albyrock87
Copy link
Contributor Author

@jamescrosswell I'm not able to compile the solution locally, but I've done my best to add a unit test which should pass and cover this PR entirely.

image

@aritchie
Copy link
Collaborator

aritchie commented Mar 3, 2025

@albyrock87 Looks like the git submodules didn't pull for you. I'll take a look and push this through. Thanks for all the help on this.

@bruno-garcia
Copy link
Member

@jamescrosswell I'm not able to compile the solution locally, but I've done my best to add a unit test which should pass and cover this PR entirely.

image

could you try: git submodule update --init --recursive ?

@bruno-garcia
Copy link
Member

@albyrock87 looks like CI is unhappy with:

Error: /home/runner/work/sentry-dotnet/sentry-dotnet/test/Sentry.Maui.Tests/MauiEventsBinderTests.Application.cs(154,9): error CS0618: 'Application.MainPage.set' is obsolete: 'This property is deprecated. Initialize your application by overriding Application.CreateWindow rather than setting MainPage. To modify the root page in an active application, use Windows[0].Page for applications with a single window. For applications with multiple windows, use Application.Windows to identify and update the root page on the correct window. Additionally, each element features a Window property, accessible when it's part of the current window.' [/home/runner/work/sentry-dotnet/sentry-dotnet/test/Sentry.Maui.Tests/Sentry.Maui.Tests.csproj::TargetFramework=net9.0]

If you could help with this bit so CI is OK we could merge the PR already it seems

@albyrock87 albyrock87 force-pushed the fix-maui-navigation-events-missing-on-ios branch from 7c12652 to 76f3632 Compare March 10, 2025 09:35
@albyrock87 albyrock87 changed the title Fix missing iOS navigation events and removes duplicate page (dis)appearing events Fix missing MAUI Shell navigation breadcrumbs on iOS Mar 10, 2025
@albyrock87
Copy link
Contributor Author

@bruno-garcia I've:

  • rebased PR onto main
  • added changelog
  • fixed unit test project build

git submodule update --init --recursive fixed the build, indeed, but now I see these two folders in my git changes, so I think those folders should be added to .gitignore.

image

@bruno-garcia
Copy link
Member

bruno-garcia commented Mar 16, 2025

git submodule update --init --recursive fixed the build, indeed, but now I see these two folders in my git changes, so I think those folders should be added to .gitignore.

The submodules don't need to go into .gitignore. It takes sometime to get used to working with it to be honest, yesterday I spent way too much time on that and I worked with these for years.

If you run git diff, does it show a change in commit sha? If so, you need to revert that.

If the diff shows a replaced sha and a new one one, copy the old one, go into the submodule director (which is essentially a different git repo) and git checkout old-sha. For example:

bruno@K29GRQ6W7J modules % git diff
diff --git a/modules/Ben.Demystifier b/modules/Ben.Demystifier
index 0a6b30f6..d7d639a6 160000
--- a/modules/Ben.Demystifier
+++ b/modules/Ben.Demystifier
@@ -1 +1 @@
-Subproject commit 0a6b30f6ac6892ff45b9dcd5b07d710e5228256e
+Subproject commit d7d639a60aada53e494b392a61c2eb6eeca379c2

Once you cd .. to get back to sentry-dotnet, if you do git status there won't be any diff anymore to correct.

Or simply:

git submodule update --init --recursive --checkout

@jamescrosswell jamescrosswell self-requested a review March 16, 2025 22:21
@jamescrosswell
Copy link
Collaborator

So this PR is actually ready to merge.

The only thing stopping it now appears to be an issue with dotnet format, which may be related to this:

I can't see how this would break in CI though as we're still using 9.0.100 in CI.

Additionally, when I use 9.0.201 locally I still have issues.

... the joys of fragile tooling 😞

@jamescrosswell jamescrosswell merged commit ac4f837 into getsentry:main Mar 17, 2025
23 checks passed
@jamescrosswell
Copy link
Collaborator

Managed to get it over the line this time simply by running dotnet format from a windows machine. It looks like they broke something with LF line endings at some point. If this pops up again, one possible workaround might be to change the format workflow to run on Windows instead of macOS. Ideally this would get fixed however.

@albyrock87 big thank you for the PR - it's very much appreciated!

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.

MAUI Shell breadcrumbs not working on iOS
4 participants