Skip to content
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

Ensure /start navigates to /starter-pack when clicking a link internally #4745

Merged
merged 2 commits into from
Jul 7, 2024

Conversation

haileyok
Copy link
Contributor

@haileyok haileyok commented Jul 7, 2024

without whitespace

Why

Links that lead to /start should just navigate to /starter-pack when clicking them within the app. Although the current behavior is generally fine, it is possible to trigger this crash with the current behavior:

<RNSNavigationController: 0x111824200> is pushing the same view controller instance (<RNSScreen: 0x159982600>) more than once which is not supported and is most likely an error in the application : xyz.blueskyweb.app. It seems to only affect iOS users as well.

How

Whenever we call convertBskyAppUrlIfNeeded(), we can add a check to replace /start/ with /starter-pack/ if the link is indeed pointing toward /start/. While this is generally not needed after #4699 (which handles SP embeds natively instead of leaving them as external links), it would be good to ensure that even if someone pastes in a raw /start/ link that it gets handled nicely.

Test Plan

Weirdly, it took me a long time to actually repro the crash. It turns out you need to do the following to reproduce it (at least, this is how I was able to)

  1. Post a starter pack link (do it from 1.87.0, not from main)
  2. Restart the app to make sure the navigation state is clear
  3. Find the new post you just made in Following tab. Press the image. It should take you to the starter pack
  4. Press back
  5. Press the starter pack again

After this change, test the same. The crash no longer reproduces.

Copy link

render bot commented Jul 7, 2024

Copy link

github-actions bot commented Jul 7, 2024

Old size New size Diff
6.61 MB 6.61 MB 66 B (0.00%)

@gaearon gaearon merged commit a6b3c97 into main Jul 7, 2024
6 checks passed
estrattonbailey added a commit that referenced this pull request Jul 9, 2024
…cial-proof

* origin/main: (126 commits)
  Update README.md (#4394)
  tweak top padding external (#4755)
  change `contentVisibility` to `contain` (#4752)
  Fix RTL text rendering for display names (#4747)
  Reduce the size of the inner logo in the QR code (#4746)
  Fix misplaced '@' in RTL post meta. (#4531)
  Remove broken and void back button (#4744)
  Ensure `/start` navigates to `/starter-pack` when clicking a link internally (#4745)
  Add missing `to` in StarterPackScreen.tsx string (#4743)
  Video compression in composer (#4638)
  fix slop (#4739)
  Update stats
  Run intl:extract
  Update Japanese translation (#4665)
  Update Korean localization (#4646)
  Update Chinese Localization (#4695)
  Update catalan (#4702)
  Update Indonesian translation (#4706)
  Tweak checkmark size
  Show feedback for Follow button in interstitials (#4738)
  ...
@haileyok haileyok deleted the hailey/ensure-navigation-external-sp-link branch September 2, 2024 20:07
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.

2 participants