8000 fix: unify storage path for `appGroupIdentifier` across targets by hoppsen · Pull Request #356 · PostHog/posthog-ios · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: unify storage path for appGroupIdentifier across targets #356

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: unify storage path for appGroupIdentifier across targets #356

wants to merge 5 commits into from

Conversation

hoppsen
Copy link
@hoppsen hoppsen commented Jun 12, 2025 •
edited
Loading

💡 Motivation and Context

When integrating PostHog into both my app and its widget extension, I discovered that each target was getting its own AnonymousId. Although I’d set:

let config = PostHogConfig(apiKey: ..., host: ...)
config.appGroupIdentifier = "group.com.hoppsen.current_app"
...

Debugging the code and folders I noticed that the code I now changed appended the bundleIdentifer to the folder path, which in my case resulted in multiple folders (one per target).

Because of this, the widget and the main app weren’t sharing the same AnonymousId. This change removes the bundle-ID suffix so that all targets point to one shared container under the app group.

// Before fix:
.../AppGroup/123456789/Library/Application%20Support/com.hoppsen.current_app/phc_...
.../AppGroup/123456789/Library/Application%20Support/com.hoppsen.current_app.WidgetExtension/phc_...

// After fix:
.../AppGroup/123456789/Library/Application%20Support/group.com.hoppsen.current_app/phc_...

💚 How did you test it?

I tested it within my local environment and ran the updated unit test to confirm the suffix no longer includes the bundle identifier.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

Thanks for an amazing product! ❤️ Please let me know if I’ve misunderstood the bundleIdentifier’s intended role.

@hoppsen hoppsen requested a review from marandaneto as a code owner June 12, 2025 11:59
@marandaneto marandaneto requested a review from ioannisj June 12, 2025 12:12
@marandaneto
Copy link
Member

cc @ioannisj since you did changes around this

@hoppsen
Copy link
Author
hoppsen commented Jun 13, 2025 •
edited
Loading

Hi @ioannisj 👋 —would love your feedback when you get a moment.

@ioannisj
Copy link
Contributor

@hoppsen 👋 Thanx for this fix! Indeed appending bundleIdentifer will create two separate folders which defeats the purpose of getting the container for the app group identifier in the first place 🤦

Using the appGroupIdentifier instead is very reasonable.

I wonder if we should also migrate existing files similar to what we do here so that we don't miss any events? So copying from Library/Application%20Support/<bundle_identifier>/phc_... to Library/Application%20Support/<appGroupIdentifier>/phc_.... This migration would be a bit more tricky since we'll have duplicates - for example the new posthog.queueFolder would be created from main target migration, so extension migration will have to copy all contents in this existing folder

@hoppsen
Copy link
Author
hoppsen commented Jun 13, 2025 •
edited
Loading

I wonder if we should also migrate existing files similar to what we do here so that we don't miss any events? So copying from Library/Application%20Support/<bundle_identifier>/phc_... to Library/Application%20Support/<appGroupIdentifier>/phc_.... This migration would be a bit more tricky since we'll have duplicates - for example the new posthog.queueFolder would be created from main target migration, so extension migration will have to copy all contents in this existing folder

🚀 @ioannisj Migrating existing data into the shared app-group folder makes a lot of sense to avoid lost events. Would you like me to update this PR or are you already looking into it?

@ioannisj
Copy link
Contributor
ioannisj commented Jun 13, 2025 •
edited
Loading

Yeah, if you are willing to update this PR it would be awesome! 🙏

@hoppsen hoppsen force-pushed the fix/appGroupIdentifier-usage branch 3 times, most recently from 28d0907 to 88b9db8 Compare June 13, 2025 11:40
@hoppsen hoppsen force-pushed the fix/appGroupIdentifier-usage branch from 88b9db8 to 5165db1 Compare June 13, 2025 11:43
@hoppsen
Copy link
Author
hoppsen commented Jun 13, 2025

@ioannisj This turned out to be a bigger change than anticipated 🫣 Would love your thoughts. I tested it locally against my fork and everything worked smoothly. 🥳

I ended up randomly selecting which anonymousId to keep based on whichever target migrates first. The same applies to all other files. I did consider inferring the main app target from the bundle identifiers to use the anonymousId of this one, but it felt less reliable.

@ioannisj
Copy link
Contributor

@hoppsen Thank you so much for all the effort you put into this! ❤️ I’ll take a look, run some tests, and get back to you!

@hoppsen hoppsen force-pushed the fix/appGroupIdentifier-usage branch from bfc15e2 to 02135a1 Compare June 13, 2025 13:12
@hoppsen
Copy link
Author
hoppsen commented Jun 13, 2025

@ioannisj Just fixed the SwiftLint issues 🫠

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marandaneto marandaneto Awaiting requested review from marandaneto marandaneto is a code owner

@ioannisj ioannisj Awaiting requested review from ioannisj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoppsen @marandaneto @ioannisj
0