Skip to content

Support PBXFileSystemSynchronizedRootGroup merges - #139

Closed
ivoidcat wants to merge 1 commit into
Lightricks:mainfrom
ivoidcat:main
Closed

ivoidcat wants to merge 1 commit into
Lightricks:mainfrom
ivoidcat:main

Conversation

@ivoidcat

@ivoidcat ivoidcat commented Mar 5, 2026

Copy link
Copy Markdown

Summary

  • add handling for PBXFileSystemSynchronizedRootGroup in add_child_to_component
  • support attaching synchronized root groups under PBXNativeTarget.file_system_synchronized_groups
  • allow group handling paths to accept PBXFileSystemSynchronizedRootGroup where appropriate

Test plan

  • Build and install patched gem locally
  • Reproduce merge on a project containing fileSystemSynchronizedGroups
  • Verify conflict resolution no longer fails with unsupported component errors

Handle additions under PBXNativeTarget and group-like containers so Xcode 16 synchronized folder changes no longer fail as unsupported components.
@1880akshay

Akshay Jain (1880akshay) commented Jul 30, 2026 •

Copy link
Copy Markdown

Ben Yohay (@byohay) Barak Weiss (@barakwei) Yonathan Oren (@oryonatan) are we planning to merge this?

@noamfreeman

Copy link
Copy Markdown
Member

Thanks for picking this up!

can you share the pbxproj (or the diff) that motivated it? I want to
confirm whether the PBXFileSystemSynchronizedRootGroup is also referenced as a child of
mainGroup, because in the case I ran where it is, apply_group_additions drops it (it filters on
PBXGroup/PBXVariantGroup) and the folder ends up attached to the target but absent from the group
tree.

Relatedly, the PBXNativeTarget branch always calls project.new(...), whereas every other add path
here resolves the existing object (add_file_reference via hierarchy_path, add_reference_proxy
via find_reference_proxy) — if the group arrives from the group tree too, doesn't this end up with
two root groups for one folder? It's also missing the Settings.allow_duplicates guard the sibling
paths have.

Finally, could you add specs (every other supported isa has them), and drop the
respond_to?(:file_system_synchronized_groups) fallback plus the PBXGroup/sync-group no-op
branches — only PBXNativeTarget declares that has_many, and a sync root group has no children,
so none of those paths can fire.

@ivoidcat

Copy link
Copy Markdown
Author

Thanks for picking this up!

can you share the pbxproj (or the diff) that motivated it? I want to confirm whether the PBXFileSystemSynchronizedRootGroup is also referenced as a child of mainGroup, because in the case I ran where it is, apply_group_additions drops it (it filters on PBXGroup/PBXVariantGroup) and the folder ends up attached to the target but absent from the group tree.

Relatedly, the PBXNativeTarget branch always calls project.new(...), whereas every other add path here resolves the existing object (add_file_reference via hierarchy_path, add_reference_proxy via find_reference_proxy) — if the group arrives from the group tree too, doesn't this end up with two root groups for one folder? It's also missing the Settings.allow_duplicates guard the sibling paths have.

Finally, could you add specs (every other supported isa has them), and drop the respond_to?(:file_system_synchronized_groups) fallback plus the PBXGroup/sync-group no-op branches — only PBXNativeTarget declares that has_many, and a sync root group has no children, so none of those paths can fire.

Thanks for the detailed review.

Unfortunately, quite a bit of time has passed and I no longer have the original .pbxproj or diff that reproduced the issue, so I cannot confirm the exact object graph from that project.

As far as I remember, the project had been created or modified with Xcode 16 and contained a PBXFileSystemSynchronizedRootGroup. When Kintsugi applied the merge diff, that object appeared under PBXNativeTarget.fileSystemSynchronizedGroups, but add_child_to_component did not handle its isa, so the merge failed with an unsupported component error.

This patch was a narrow fix for that failure. It added dispatching for PBXFileSystemSynchronizedRootGroup and created and attached the object to PBXNativeTarget#file_system_synchronized_groups. That was sufficient to make the affected project merge successfully.

Looking at the implementation again, I agree that it may be incomplete if the same synchronized root group is also referenced from mainGroup. In that case, apply_group_additions currently filters it out, while the PBXNativeTarget path creates a new object instead of resolving an existing one, which could result in a missing group-tree entry or duplicate objects. I also agree that the respond_to? fallback and the group no-op branches are unnecessary.

I no longer have the original fixture to verify whether the synchronized root group was present in both locations, so this should be treated as a project-specific workaround rather than a complete implementation. A proper fix should handle the group in the mainGroup addition flow, reuse the same project object when attaching it to the target, respect Settings.allow_duplicates, and include specs for both references.

@noamfreeman

Copy link
Copy Markdown
Member

fair. it was a long time. is your project still using anything but buildable folders? if so, i'l try to add a proper fix.

@rzulkoski

Copy link
Copy Markdown
Contributor

Hi Noam Freeman (@noamfreeman) ivoidcat — jumping in from the sidelines. We hit this same failure on an Xcode 26 project that uses buildable folders (introduced in Xcode 16), and used ivoidcat's patch here plus Noam Freeman (@noamfreeman)'s review as the spec to build out a complete implementation (with Claude Code's help — the resulting commit is co-authored accordingly).

It follows the review checklist directly:

  • PBXFileSystemSynchronizedRootGroup is now handled in the main-group addition/removal pipeline, so it's no longer dropped by apply_group_additions — the missing-from-the-group-tree case you flagged.
  • The target attachment resolves and reuses the object created in the group tree instead of project.new, so one folder never yields two root groups — guarded by Settings.allow_duplicates.
  • Dropped the respond_to? fallback and the no-op branches.
  • Specs cover both references (main group + target), plus explicit file types/folders, exceptions, removal, and the duplicate guard, and there's an end-to-end integration spec that reproduces the original failure.

It also handles exceptions (target/build-phase membership), which needed a workaround for an xcodeproj bug where PBXFileSystemSynchronizedGroupBuildPhaseMembershipExceptionSet#display_name calls the nonexistent build_phase.name (still present in 1.28.1 and master). bundle exec rake is green on the pinned xcodeproj range.

Since you mentioned you might take a crack at a proper fix, I didn't want to step on that — would a PR be welcome, or are you already mid-fix? Happy to open one (crediting this thread) whenever suits.

@noamfreeman

Copy link
Copy Markdown
Member

hi Ryan Zulkoski (@rzulkoski)!
a PR would be welcome

Ryan Zulkoski (rzulkoski) added a commit to hallow-inc/Kintsugi that referenced this pull request Sep 1, 2026
Xcode 16 "buildable folders" are stored as PBXFileSystemSynchronizedRootGroup
objects, referenced both from the main group tree and from a target's
fileSystemSynchronizedGroups. Kintsugi had no handling for this isa, so merging
any conflict touching such a folder failed with "Trying to add unsupported
component type PBXFileSystemSynchronizedRootGroup".

This implements complete support, addressing the review feedback on
Lightricks#139:

- Handle the folder in the main group addition/removal pipeline instead of
  dropping it (also fixing a phantom empty-children key that blocked removals).
- Attach it to a target by resolving and reusing the object already created in
  the group tree, identifying it by hierarchy path so same-named folders in
  different parent groups (and folders whose path contains "/") resolve
  correctly; dedup by object identity so one target can link several distinct
  folders that share a name.
- Defer exception target/build-phase resolution to a final pass, so references
  to targets created in the same change resolve; resolve a build phase via its
  owning target, avoiding same-named ("Sources") collisions across targets.
- Dedup exception sets (which arrive via multiple graph paths), and detach a
  target's folder reference on unlink instead of deleting the shared object.
- Support target-membership and build-phase-membership exceptions, with
  to_tree_hash monkeypatches that break the target/build_phase reference cycle
  (which otherwise infinitely recurses) and work around an xcodeproj 1.27.0 bug
  where the membership exception set's display_name raises.

Adds extensive specs (both references, exceptions, removal/unlink, dedup,
same-named and slashed-path folders, deferred resolution) plus an integration
spec reproducing the original merge failure.

Known limitation: relocating a folder between parent groups on one side can drop
its target membership on merge, since folders have no stable identity in the
diff; re-verify membership after such merges.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

4 participants