Fix #173 followup: address CodeRabbit review items#175
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe Xcode project file is updated to fix the Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
… target, Foundation path, hardcoded test value - Fix BlueprintIdentifier in scheme to match actual test target ID (4F0E6636...) - Lower test target MACOSX_DEPLOYMENT_TARGET 14.0 → 13.0 to match app - Replace hardcoded SDK path for Foundation.framework with SDKROOT-relative - Use DEFAULT_IGNORED_APP_BUNDLE_ID constant instead of hardcoded bundle ID in test
pablopunk
force-pushed
the
fix/173-review-followup
branch
from
9ae345e to
ebf0a45
Compare
![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 (3)
Swift Shift.xcodeproj/project.pbxproj (2)
181-183: ⚡ Quick winFix
SwiftShiftTestsgroup contents to include test files.The
SwiftShiftTestsgroup currently contains app source manager/constants files, while actual test files are attached elsewhere. Repoint this group toKeyboardShortcutTests.swift,PreferencesManagerTests.swift, andWindowManagerTests.swiftfor consistent project structure.Also applies to: 235-243
🤖 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 `@Swift` Shift.xcodeproj/project.pbxproj around lines 181 - 183, The SwiftShiftTests group in the Xcode project file is incorrectly configured to include app source manager and constants files instead of test files. Update the group definition to properly reference the three test files: KeyboardShortcutTests.swift, PreferencesManagerTests.swift, and WindowManagerTests.swift by ensuring these file references are correctly placed within the SwiftShiftTests group children array. Remove any non-test files from this group and apply the same correction to all other occurrences of the SwiftShiftTests group configuration throughout the project file.
76-86: ⚡ Quick winConsolidate duplicate
PBXFileReferenceentries and product entries.There are duplicate logical references (including two
SwiftShiftTests.xctestfile refs and both listed underProducts). This increases pbxproj churn and merge-conflict risk. Keep a single canonical file reference per logical file and remove stale duplicates from groups/products.Also applies to: 191-193
🤖 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 `@Swift` Shift.xcodeproj/project.pbxproj around lines 76 - 86, Remove the duplicate PBXFileReference entries in the project file. Specifically, eliminate one of the two SwiftShiftTests.xctest file references (keeping either 2F7068A35E872B916122F622 or B4FAEC4EF8BA9AFD9FABBA1F), remove one of the duplicate WindowManager.swift references (keeping either 3C0E83B215533AA53B9BD58A or C9075C2AB4A5BD52358038BF), and apply the same consolidation approach to the duplicate entries noted at the referenced lines. Maintain only a single canonical PBXFileReference per logical file and ensure all product and group references point to the same canonical entry to minimize project file churn and reduce merge-conflict risk.SwiftShiftTests/PreferencesManagerTests.swift (1)
51-55: ⚡ Quick winReplace hardcoded system bundle IDs with the system constant.
At Line 54 and related locations, the string
"com.apple.notificationcenterui"is repeated directly in tests. UsingSYSTEM_IGNORED_APP_BUNDLE_IDkeeps tests aligned with the production source of truth and avoids drift when defaults change.Also applies to: 66-67, 118-125, 136-145, 147-155
🤖 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 `@SwiftShiftTests/PreferencesManagerTests.swift` around lines 51 - 55, Replace all hardcoded system bundle ID strings like "com.apple.notificationcenterui" with the SYSTEM_IGNORED_APP_BUNDLE_ID constant throughout the test file. This includes the testGetIgnoredApps_includesSystemApps function and all other test methods that reference system bundle IDs (at the locations mentioned: 66-67, 118-125, 136-145, 147-155). Using the constant instead of hardcoded strings ensures the tests remain synchronized with the production source of truth and prevents test failures if the constant values change.
🤖 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 `@SwiftShiftTests/PreferencesManagerTests.swift`:
- Around line 194-208: The test testMigration_addsDefaultAppsWhenMissing does
not actually verify that default apps were merged into the ignored apps list
during migration. Currently it only checks that the custom app is retained and
the migration flag is set, but a regression in the default app merging logic
would not be caught. Add assertions after the getUserIgnoredApps() call to
verify that the returned apps list contains the expected default apps in
addition to the custom app that was set. This ensures the test validates the
core behavior described by its name.
---
Nitpick comments:
In `@Swift` Shift.xcodeproj/project.pbxproj:
- Around line 181-183: The SwiftShiftTests group in the Xcode project file is
incorrectly configured to include app source manager and constants files instead
of test files. Update the group definition to properly reference the three test
files: KeyboardShortcutTests.swift, PreferencesManagerTests.swift, and
WindowManagerTests.swift by ensuring these file references are correctly placed
within the SwiftShiftTests group children array. Remove any non-test files from
this group and apply the same correction to all other occurrences of the
SwiftShiftTests group configuration throughout the project file.
- Around line 76-86: Remove the duplicate PBXFileReference entries in the
project file. Specifically, eliminate one of the two SwiftShiftTests.xctest file
references (keeping either 2F7068A35E872B916122F622 or
B4FAEC4EF8BA9AFD9FABBA1F), remove one of the duplicate WindowManager.swift
references (keeping either 3C0E83B215533AA53B9BD58A or
C9075C2AB4A5BD52358038BF), and apply the same consolidation approach to the
duplicate entries noted at the referenced lines. Maintain only a single
canonical PBXFileReference per logical file and ensure all product and group
references point to the same canonical entry to minimize project file churn and
reduce merge-conflict risk.
In `@SwiftShiftTests/PreferencesManagerTests.swift`:
- Around line 51-55: Replace all hardcoded system bundle ID strings like
"com.apple.notificationcenterui" with the SYSTEM_IGNORED_APP_BUNDLE_ID constant
throughout the test file. This includes the
testGetIgnoredApps_includesSystemApps function and all other test methods that
reference system bundle IDs (at the locations mentioned: 66-67, 118-125,
136-145, 147-155). Using the constant instead of hardcoded strings ensures the
tests remain synchronized with the production source of truth and prevents test
failures if the constant values change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af3e0bd0-ad7f-4b16-b04e-b88b705908fe
📒 Files selected for processing (8)
.github/workflows/build.yml.github/workflows/www.ymlMakefileSwift Shift.xcodeproj/project.pbxprojSwift Shift.xcodeproj/xcshareddata/xcschemes/Swift Shift.xcschemeSwiftShiftTests/KeyboardShortcutTests.swiftSwiftShiftTests/PreferencesManagerTests.swiftSwiftShiftTests/WindowManagerTests.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Swift Shift.xcodeproj/project.pbxproj (2)
181-183: ⚡ Quick winFix
SwiftShiftTestsgroup contents to include test files.The
SwiftShiftTestsgroup currently contains app source manager/constants files, while actual test files are attached elsewhere. Repoint this group toKeyboardShortcutTests.swift,PreferencesManagerTests.swift, andWindowManagerTests.swiftfor consistent project structure.Also applies to: 235-243
🤖 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 `@Swift` Shift.xcodeproj/project.pbxproj around lines 181 - 183, The SwiftShiftTests group in the Xcode project file is incorrectly configured to include app source manager and constants files instead of test files. Update the group definition to properly reference the three test files: KeyboardShortcutTests.swift, PreferencesManagerTests.swift, and WindowManagerTests.swift by ensuring these file references are correctly placed within the SwiftShiftTests group children array. Remove any non-test files from this group and apply the same correction to all other occurrences of the SwiftShiftTests group configuration throughout the project file.
76-86: ⚡ Quick winConsolidate duplicate
PBXFileReferenceentries and product entries.There are duplicate logical references (including two
SwiftShiftTests.xctestfile refs and both listed underProducts). This increases pbxproj churn and merge-conflict risk. Keep a single canonical file reference per logical file and remove stale duplicates from groups/products.Also applies to: 191-193
🤖 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 `@Swift` Shift.xcodeproj/project.pbxproj around lines 76 - 86, Remove the duplicate PBXFileReference entries in the project file. Specifically, eliminate one of the two SwiftShiftTests.xctest file references (keeping either 2F7068A35E872B916122F622 or B4FAEC4EF8BA9AFD9FABBA1F), remove one of the duplicate WindowManager.swift references (keeping either 3C0E83B215533AA53B9BD58A or C9075C2AB4A5BD52358038BF), and apply the same consolidation approach to the duplicate entries noted at the referenced lines. Maintain only a single canonical PBXFileReference per logical file and ensure all product and group references point to the same canonical entry to minimize project file churn and reduce merge-conflict risk.SwiftShiftTests/PreferencesManagerTests.swift (1)
51-55: ⚡ Quick winReplace hardcoded system bundle IDs with the system constant.
At Line 54 and related locations, the string
"com.apple.notificationcenterui"is repeated directly in tests. UsingSYSTEM_IGNORED_APP_BUNDLE_IDkeeps tests aligned with the production source of truth and avoids drift when defaults change.Also applies to: 66-67, 118-125, 136-145, 147-155
🤖 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 `@SwiftShiftTests/PreferencesManagerTests.swift` around lines 51 - 55, Replace all hardcoded system bundle ID strings like "com.apple.notificationcenterui" with the SYSTEM_IGNORED_APP_BUNDLE_ID constant throughout the test file. This includes the testGetIgnoredApps_includesSystemApps function and all other test methods that reference system bundle IDs (at the locations mentioned: 66-67, 118-125, 136-145, 147-155). Using the constant instead of hardcoded strings ensures the tests remain synchronized with the production source of truth and prevents test failures if the constant values change.
🤖 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 `@SwiftShiftTests/PreferencesManagerTests.swift`:
- Around line 194-208: The test testMigration_addsDefaultAppsWhenMissing does
not actually verify that default apps were merged into the ignored apps list
during migration. Currently it only checks that the custom app is retained and
the migration flag is set, but a regression in the default app merging logic
would not be caught. Add assertions after the getUserIgnoredApps() call to
verify that the returned apps list contains the expected default apps in
addition to the custom app that was set. This ensures the test validates the
core behavior described by its name.
---
Nitpick comments:
In `@Swift` Shift.xcodeproj/project.pbxproj:
- Around line 181-183: The SwiftShiftTests group in the Xcode project file is
incorrectly configured to include app source manager and constants files instead
of test files. Update the group definition to properly reference the three test
files: KeyboardShortcutTests.swift, PreferencesManagerTests.swift, and
WindowManagerTests.swift by ensuring these file references are correctly placed
within the SwiftShiftTests group children array. Remove any non-test files from
this group and apply the same correction to all other occurrences of the
SwiftShiftTests group configuration throughout the project file.
- Around line 76-86: Remove the duplicate PBXFileReference entries in the
project file. Specifically, eliminate one of the two SwiftShiftTests.xctest file
references (keeping either 2F7068A35E872B916122F622 or
B4FAEC4EF8BA9AFD9FABBA1F), remove one of the duplicate WindowManager.swift
references (keeping either 3C0E83B215533AA53B9BD58A or
C9075C2AB4A5BD52358038BF), and apply the same consolidation approach to the
duplicate entries noted at the referenced lines. Maintain only a single
canonical PBXFileReference per logical file and ensure all product and group
references point to the same canonical entry to minimize project file churn and
reduce merge-conflict risk.
In `@SwiftShiftTests/PreferencesManagerTests.swift`:
- Around line 51-55: Replace all hardcoded system bundle ID strings like
"com.apple.notificationcenterui" with the SYSTEM_IGNORED_APP_BUNDLE_ID constant
throughout the test file. This includes the
testGetIgnoredApps_includesSystemApps function and all other test methods that
reference system bundle IDs (at the locations mentioned: 66-67, 118-125,
136-145, 147-155). Using the constant instead of hardcoded strings ensures the
tests remain synchronized with the production source of truth and prevents test
failures if the constant values change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af3e0bd0-ad7f-4b16-b04e-b88b705908fe
📒 Files selected for processing (8)
.github/workflows/build.yml.github/workflows/www.ymlMakefileSwift Shift.xcodeproj/project.pbxprojSwift Shift.xcodeproj/xcshareddata/xcschemes/Swift Shift.xcschemeSwiftShiftTests/KeyboardShortcutTests.swiftSwiftShiftTests/PreferencesManagerTests.swiftSwiftShiftTests/WindowManagerTests.swift
🛑 Comments failed to post (1)
SwiftShiftTests/PreferencesManagerTests.swift (1)
194-208:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMigration test does not verify the behavior named by the test.
At Line 194,
testMigration_addsDefaultAppsWhenMissingonly checks custom-app retention and migration flag state; it never checks that defaults were actually merged. A regression in default-app merging would still pass this test.Suggested assertion tightening
let apps = PreferencesManager.getUserIgnoredApps() XCTAssertTrue(apps.contains("com.example.only")) + for bundleId in DEFAULT_IGNORED_APP_BUNDLE_ID { + XCTAssertTrue(apps.contains(bundleId), "Missing migrated default bundle ID: \(bundleId)") + } // After migration, the migration flag should be set XCTAssertTrue(UserDefaults.standard.bool(forKey: PreferenceKey.didMigrateDefaultIgnoredApps.rawValue))🤖 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 `@SwiftShiftTests/PreferencesManagerTests.swift` around lines 194 - 208, The test testMigration_addsDefaultAppsWhenMissing does not actually verify that default apps were merged into the ignored apps list during migration. Currently it only checks that the custom app is retained and the migration flag is set, but a regression in the default app merging logic would not be caught. Add assertions after the getUserIgnoredApps() call to verify that the returned apps list contains the expected default apps in addition to the custom app that was set. This ensures the test validates the core behavior described by its name.
1 participant
Addresses review comments from #174 merge:
D76A70D7instead of actual4F0E6636DEFAULT_IGNORED_APP_BUNDLE_IDconstant instead of hardcoded bundle IDAll 49 tests pass after changes.
Summary by CodeRabbit
Chores
Tests