docs: align Xcode app guidance with strict MVVM#115
Conversation
📝 WalkthroughWalkthroughThis PR aligns Apple/Xcode app guidance across the apple-dev-skills plugin toward a strict Apple-app MVVM source layout (per-view ChangesStrict MVVM structure guidance and tooling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 190bf77cae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "Sources/Views/macOS/.gitkeep", | ||
| "Sources/Views/iOS/.gitkeep", | ||
| "Sources/Models/.gitkeep", | ||
| "Sources/Services/Consumed/.gitkeep", | ||
| "Sources/Services/Provided/.gitkeep", |
There was a problem hiding this comment.
These new .gitkeep files are created inside Sources, but the generated XcodeGen spec includes the entire Sources synced folder as the app target while only excluding **/.gitkeep for the separate Shared source entry. In a fresh scaffold the empty macOS/iOS/model/service placeholders can therefore be picked up by the app target until a user manually removes them or the Sources entry also excludes them, which leaves generated projects with unintended placeholder target membership.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
|
|
||
| def audit_xcode_app_structure(repo_root: Path) -> dict: |
There was a problem hiding this comment.
Dry-run and report-only modes return the audit from this wrapper without invoking sync_xcode_project_guidance.py, but this implementation only checks required directories and Sources/Controllers. The full helper audit introduced in the same change catches unpaired FooViewModel.swift, misplaced +Model/+Controller, and missing app view-model/service files, so check-only inventory runs can incorrectly report structure_audit.status == "passed" for drift they are supposed to detect.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
plugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/run_workflow.py (1)
38-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated required-directories list and audit logic across two scripts.
The
required_directorieslist inrun_workflow.py(lines 39-47) is identical toREQUIRED_XCODE_APP_DIRECTORIESinsync_xcode_project_guidance.py(lines 30-37). The entireaudit_xcode_app_structurefunction is also duplicated with different levels of completeness. Extracting both the constant and the function into a shared module (e.g.,structure_audit.py) would eliminate the drift risk and resolve the inconsistency flagged above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/run_workflow.py` around lines 38 - 47, The required-directories list and audit logic are duplicated between run_workflow.py and sync_xcode_project_guidance.py, so move the shared REQUIRED_XCODE_APP_DIRECTORIES constant and the audit_xcode_app_structure behavior into a common module such as structure_audit.py. Update run_workflow.py to import and use the shared helper instead of maintaining its own copy, and ensure both scripts reference the same source of truth so future changes stay consistent.plugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/sync_xcode_project_guidance.py (1)
155-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComprehensive audit looks correct; duplication concern noted.
The
audit_xcode_app_structureimplementation here is more thorough than the one inrun_workflow.py— it covers directory presence, legacy controllers, unpaired view-model naming,+Model/+Controllerplacement, app-entry-point+ViewModelpairing, and internal app service. The finding codes and messages are clear and actionable.The duplication with
run_workflow.py's simpler version is flagged in therun_workflow.pyreview. Extracting to a shared module would ensure both scripts produce identical audit results.One minor note: line 216 (
not path.name.endswith("+ViewModel.swift")) is redundant because the*App.swiftglob pattern cannot match*+ViewModel.swiftfilenames. Harmless but unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/sync_xcode_project_guidance.py` around lines 155 - 245, The `audit_xcode_app_structure` function contains a redundant `not path.name.endswith("+ViewModel.swift")` check in the `app_files` collection, since the `*App.swift` glob cannot match `*+ViewModel.swift` names. Remove that unnecessary condition and keep the `sources_dir.glob("*App.swift")` filtering focused on actual app entry points.plugins/apple-dev-skills/tests/test_xcode_guidance_sync_workflow.py (1)
208-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests only cover the simple audit; comprehensive audit naming/placement checks are untested.
The two new tests (
test_structure_audit_flags_legacy_controllers_directoryandtest_structure_audit_passes_strict_source_layout) both run with--dry-run, which exercisesrun_workflow.py's simpleraudit_xcode_app_structure(directory + legacy controller checks only). The comprehensive checks insync_xcode_project_guidance.py's version —unpaired-view-model-file,view-model-outside-views,controller-outside-views,missing-app-view-model, andmissing-internal-app-service— have no test coverage.Consider adding tests that create files like
Sources/Models/FooViewModel.swift(unpaired),Sources/Models/Foo+Model.swift(outside Views), orSources/FooApp.swiftwithout a pairedFooApp+ViewModel.swift, then run the full sync path (not--dry-run) and assert the corresponding finding codes appear instructure_audit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/apple-dev-skills/tests/test_xcode_guidance_sync_workflow.py` around lines 208 - 255, The new tests only cover the lightweight dry-run audit, so the comprehensive structure checks in sync_xcode_project_guidance.py are not exercised. Add full sync-path tests alongside test_structure_audit_flags_legacy_controllers_directory and test_structure_audit_passes_strict_source_layout that create fixtures for unpaired view models, model files outside Views, controllers outside Views, and missing app view model/internal service pairs, then run the non-dry-run workflow and assert the expected structure_audit finding codes appear. Use the existing structure_audit/findings assertions and the sync entrypoint path to cover the richer audit logic.plugins/apple-dev-skills/skills/structure-swift-sources/SKILL.md (1)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIncomplete sentence: missing verb.
"ensure important app-facing source directories such as
Views/,Models/, andServices/, and do not preserve..." is missing a verb (e.g., "exist" or "use").✏️ Proposed fix
- - for Xcode app projects, ensure important app-facing source directories such as `Views/`, `Models/`, and `Services/`, and do not preserve a root `Controllers/` directory + - for Xcode app projects, use the strict app-facing source directories `Views/`, `Models/`, and `Services/`, and do not preserve a root `Controllers/` directory🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/apple-dev-skills/skills/structure-swift-sources/SKILL.md` at line 72, Fix the incomplete sentence in the Xcode app projects guidance within SKILL.md by adding the missing verb so the instruction reads naturally and grammatically. Update the sentence in the app project directory-preservation guidance to clearly state that important app-facing source directories such as Views/, Models/, and Services/ should exist or be used, and that a root Controllers/ directory should not be preserved. Refer to the existing bullet in the structure-swift-sources skill so the wording stays consistent with the surrounding rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/run_workflow.py`:
- Around line 38-68: The audit logic is duplicated and inconsistent between
run_workflow.py and sync_xcode_project_guidance.py, causing dry-run/report-only
and real sync modes to produce different results. Refactor the comprehensive
audit_xcode_app_structure from sync_xcode_project_guidance.py into a shared
module, then have run_workflow.py import and use that same function everywhere
it builds audit payloads so all modes return identical findings, including the
view-model/model/controller pairing and internal service checks.
---
Nitpick comments:
In `@plugins/apple-dev-skills/skills/structure-swift-sources/SKILL.md`:
- Line 72: Fix the incomplete sentence in the Xcode app projects guidance within
SKILL.md by adding the missing verb so the instruction reads naturally and
grammatically. Update the sentence in the app project directory-preservation
guidance to clearly state that important app-facing source directories such as
Views/, Models/, and Services/ should exist or be used, and that a root
Controllers/ directory should not be preserved. Refer to the existing bullet in
the structure-swift-sources skill so the wording stays consistent with the
surrounding rules.
In
`@plugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/run_workflow.py`:
- Around line 38-47: The required-directories list and audit logic are
duplicated between run_workflow.py and sync_xcode_project_guidance.py, so move
the shared REQUIRED_XCODE_APP_DIRECTORIES constant and the
audit_xcode_app_structure behavior into a common module such as
structure_audit.py. Update run_workflow.py to import and use the shared helper
instead of maintaining its own copy, and ensure both scripts reference the same
source of truth so future changes stay consistent.
In
`@plugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/sync_xcode_project_guidance.py`:
- Around line 155-245: The `audit_xcode_app_structure` function contains a
redundant `not path.name.endswith("+ViewModel.swift")` check in the `app_files`
collection, since the `*App.swift` glob cannot match `*+ViewModel.swift` names.
Remove that unnecessary condition and keep the `sources_dir.glob("*App.swift")`
filtering focused on actual app entry points.
In `@plugins/apple-dev-skills/tests/test_xcode_guidance_sync_workflow.py`:
- Around line 208-255: The new tests only cover the lightweight dry-run audit,
so the comprehensive structure checks in sync_xcode_project_guidance.py are not
exercised. Add full sync-path tests alongside
test_structure_audit_flags_legacy_controllers_directory and
test_structure_audit_passes_strict_source_layout that create fixtures for
unpaired view models, model files outside Views, controllers outside Views, and
missing app view model/internal service pairs, then run the non-dry-run workflow
and assert the expected structure_audit finding codes appear. Use the existing
structure_audit/findings assertions and the sync entrypoint path to cover the
richer audit logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91f38270-7598-4f10-b583-15ccf431f594
📒 Files selected for processing (43)
ROADMAP.mddocs/maintainers/apple-swift-structure-guidance-alignment.mddocs/maintainers/automation-suitability.mdplugins/apple-dev-skills/ROADMAP.mdplugins/apple-dev-skills/shared/agents-snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/appkit-app-architecture-workflow/SKILL.mdplugins/apple-dev-skills/skills/appkit-app-architecture-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/apple-typography-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/apple-ui-accessibility-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/avaudio-engine-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/avfaudio-session-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/avfoundation-media-pipeline-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/bootstrap-xcode-app-project/SKILL.mdplugins/apple-dev-skills/skills/bootstrap-xcode-app-project/assets/AGENTS.mdplugins/apple-dev-skills/skills/bootstrap-xcode-app-project/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/bootstrap-xcode-app-project/scripts/bootstrap_xcode_app_project.pyplugins/apple-dev-skills/skills/core-animation-layer-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/coreaudio-modernization-repair-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/coremedia-timing-samplebuffer-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/devicecheck-app-attest-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/explore-apple-swift-docs/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/format-swift-sources/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/migrate-xcode-project-to-xcodegen/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/safari-extension-control-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/sf-symbols-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/structure-swift-sources/SKILL.mdplugins/apple-dev-skills/skills/structure-swift-sources/references/layout-rules.mdplugins/apple-dev-skills/skills/structure-swift-sources/references/source-organization-rules.mdplugins/apple-dev-skills/skills/swiftui-animation-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/swiftui-app-architecture-workflow/SKILL.mdplugins/apple-dev-skills/skills/swiftui-app-architecture-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/sync-xcode-project-guidance/SKILL.mdplugins/apple-dev-skills/skills/sync-xcode-project-guidance/assets/AGENTS.mdplugins/apple-dev-skills/skills/sync-xcode-project-guidance/assets/append-section.mdplugins/apple-dev-skills/skills/sync-xcode-project-guidance/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/run_workflow.pyplugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/sync_xcode_project_guidance.pyplugins/apple-dev-skills/skills/xcode-app-project-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/xcode-build-run-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/xcode-coding-intelligence-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/skills/xcode-testing-workflow/references/snippets/apple-xcode-project-core.mdplugins/apple-dev-skills/tests/test_xcode_app_bootstrap_workflow.pyplugins/apple-dev-skills/tests/test_xcode_guidance_sync_workflow.py
| def audit_xcode_app_structure(repo_root: Path) -> dict: | ||
| required_directories = [ | ||
| "Sources/Views/Shared", | ||
| "Sources/Views/macOS", | ||
| "Sources/Views/iOS", | ||
| "Sources/Models", | ||
| "Sources/Services/Consumed", | ||
| "Sources/Services/Internal", | ||
| "Sources/Services/Provided", | ||
| ] | ||
| findings = [ | ||
| { | ||
| "code": "missing-directory", | ||
| "path": relative_path, | ||
| "message": f"Expected Xcode app structure directory is missing: {relative_path}", | ||
| } | ||
| for relative_path in required_directories | ||
| if not (repo_root / relative_path).is_dir() | ||
| ] | ||
| if (repo_root / "Sources" / "Controllers").exists(): | ||
| findings.append( | ||
| { | ||
| "code": "legacy-controllers-directory", | ||
| "path": "Sources/Controllers", | ||
| "message": "Move UIKit/AppKit controller files beside their matching view under Sources/Views as <ViewName>+Controller.swift.", | ||
| } | ||
| ) | ||
| return { | ||
| "status": "passed" if not findings else "needs-attention", | ||
| "findings": findings, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Inconsistent audit_xcode_app_structure between run_workflow.py and sync_xcode_project_guidance.py.
run_workflow.py defines a simpler audit_xcode_app_structure (lines 38-68) that only checks required directories and legacy Sources/Controllers. The companion script sync_xcode_project_guidance.py defines a more comprehensive version (lines 155-245) that additionally checks unpaired view-model naming, +Model/+Controller placement outside Sources/Views/, app-entry-point +ViewModel pairing, and missing internal app service.
Because run_workflow.py uses its own simpler audit for --dry-run (line 224) and writeMode=report-only (line 240) payloads, but delegates to sync_xcode_project_guidance.py (which uses the comprehensive audit) for actual syncs, users get different audit results depending on the mode. A dry-run may report passed while an actual sync reports needs-attention for the same repo.
Consider extracting the comprehensive audit_xcode_app_structure into a shared module that both scripts import, so all modes produce identical audit results.
Also applies to: 121-121, 224-224, 240-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/apple-dev-skills/skills/sync-xcode-project-guidance/scripts/run_workflow.py`
around lines 38 - 68, The audit logic is duplicated and inconsistent between
run_workflow.py and sync_xcode_project_guidance.py, causing dry-run/report-only
and real sync modes to produce different results. Refactor the comprehensive
audit_xcode_app_structure from sync_xcode_project_guidance.py into a shared
module, then have run_workflow.py import and use that same function everywhere
it builds audit payloads so all modes return identical findings, including the
view-model/model/controller pairing and internal service checks.
1 participant
Summary
Verification
Note: root-wide ruff still reports an unrelated pre-existing unused sys import in migrate_xcode_project_to_xcodegen.py.
Summary by CodeRabbit
Documentation
Sources/structure for views, models, and services.New Features
Tests