[codex] Add Swift E2E coverage#2408
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a complete Swift XCUITest E2E infrastructure: accessibility identifiers across all SwiftUI feature views, a new macOS UI test target ( ChangesSwift E2E infrastructure and test coverage
API pack category defaulting fix
Social interactions DB migration
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CI
participant runner as run-e2e.ts
participant scheme as Xcode Scheme XML
participant xcodebuild
participant MacUITestCase
Dev->>runner: bun run e2e:swift:mac-smoke
runner->>runner: loadDotEnv(.env.local)
runner->>runner: parseArgs → SwiftTestMode=mac-smoke
runner->>runner: requireGeneratedProject
runner->>runner: requireE2ECredentials (E2E_EMAIL, E2E_PASSWORD)
runner->>scheme: injectSchemeEnv(credentials + E2E_API_BASE_URL)
runner->>xcodebuild: spawn with -testPlan + resultBundlePath
xcodebuild->>MacUITestCase: launch app with --reset-auth --disable-animations
MacUITestCase->>MacUITestCase: loginIfNeeded (reads E2E_EMAIL/E2E_PASSWORD)
MacUITestCase->>MacUITestCase: goToSidebar / waitFor / assertions
xcodebuild-->>runner: stdout/stderr (redactSecrets applied)
runner-->>Dev: exit with xcodebuild status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 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. ✨ Finishing Touches🧪 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 |
andrew-bierman
changed the base branch from
main
to
claude/swift-mac-app-effort-tTGd7
andrew-bierman
force-pushed
the
feat/swift-e2e-coverage
branch
from
1fc47db to
ba2e5e6
Compare
github-actions
Bot
added
documentation
|
Already MERGEABLE — good news, no conflicts. Failing checks are the standard iOS/Android E2E flake plus what looks like the new Swift E2E test job itself running. Before flipping from draft, three asks:
The core direction (Swift XCUITests for the iOS app) is sound and complements Maestro for native-specific flows. Mark ready and tag for review once those three are answered. |
|
This pull request has been automatically marked as stale because it has not had any activity for 30 days. It will be closed in 7 days if no further activity occurs. If this PR is still in progress, please leave a comment or remove the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/swift/Sources/PackRat/Navigation/AppNavigation.swift (1)
107-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winmacOS sidebar loses visual selection feedback.
The new Button-based sidebar implementation correctly updates
state.navItemon tap, but it doesn't show which item is currently selected. The previous selection-based List automatically highlighted the active item. Users won't see which screen they're on.Consider adding visual feedback to indicate the active navigation item. One approach:
🎨 Proposed fix to restore selection highlighting
return List(NavItem.allCases) { item in Button { state.navItem = item } label: { Label(item.label, systemImage: item.symbol) .frame(maxWidth: .infinity, alignment: .leading) } .buttonStyle(.plain) + .listRowBackground( + state.navItem == item ? Color.accentColor.opacity(0.15) : Color.clear + ) .accessibilityIdentifier("sidebar_nav_\(item.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 `@apps/swift/Sources/PackRat/Navigation/AppNavigation.swift` around lines 107 - 138, The macOS sidebar implementation using Button elements does not provide visual feedback for the currently selected navigation item, unlike the non-macOS List-based implementation which has automatic selection highlighting. Add conditional styling to the Button in the macOS sidebar (within the first `#if` os(macOS) block) to visually distinguish the active navigation item by comparing the current item with state.navItem. Apply a background color, overlay, or opacity change to the Label when item equals state.navItem to indicate selection state, matching the visual feedback that users would see in the non-macOS version.
🤖 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 @.github/workflows/swift-e2e.yml:
- Line 67: In the file .github/workflows/swift-e2e.yml, replace all mutable
version tags with full commit SHAs to prevent supply-chain attacks.
Specifically: at lines 67 and 167, replace actions/checkout@v6 with the v6
release commit SHA; at lines 70 and 170, replace maxim-lobanov/setup-xcode@v1
with its commit SHA; at lines 75 and 175, replace oven-sh/setup-bun@v2 with its
commit SHA; and at lines 133, 141, 150, 224, 232, and 241, replace
actions/upload-artifact@v7 with its commit SHA. Use the official GitHub release
pages for each action to find the correct commit SHAs corresponding to each
version tag.
- Around line 66-68: The workflow uses the mutable tag actions/checkout@v6
instead of a pinned commit SHA, which violates security guidelines. At both
occurrences of the actions/checkout step (at lines 66-68 and 166-168 in the
swift-e2e.yml file), replace the version tag with a specific commit SHA, and add
the persist-credentials option set to false. This ensures supply-chain attack
prevention by pinning to an immutable commit, and reduces credential exposure
since this workflow only needs checkout functionality without authenticated git
operations.
- Around line 75-78: Replace the dynamic `bun-version: latest` with a pinned
version in the setup-bun action configuration. Change the bun-version parameter
to use the specific version declared in package.json (1.3.10) instead of latest.
This change must be applied at both occurrences where the setup-bun action is
used in the workflow to ensure reproducible CI runs.
In `@apps/swift/project.yml`:
- Line 101: Verify that the DEVELOPMENT_TEAM migration from 7WV9JYCW55 to
666HGMV2LU is intentional across all affected targets in apps/swift/project.yml
(at lines 101, 152, and 191). If this is a legitimate team migration, update the
PR description to explicitly document this change and explain the reason for the
migration. If this was an accidental commit of local development settings,
revert the DEVELOPMENT_TEAM value back to 7WV9JYCW55 in the PackRat-iOS target,
PackRat-macOS target, and PackRatMacUITests target.
In `@apps/swift/scripts/run-e2e.test.ts`:
- Around line 5-12: The test suite is missing unit test coverage for the
injectSchemeEnv function, which handles high-risk XML scheme mutation
operations. Add test cases to verify the success paths where env block insertion
and replacement work correctly in the scheme XML, and add tests for the failure
path where a missing </TestAction> element causes the function to throw an error
and fail hard. Ensure both the happy path scenarios and the error handling are
properly tested to cover the XML manipulation logic.
In `@apps/swift/scripts/run-e2e.ts`:
- Around line 206-229: The injectSchemeEnv function uses a simple string
replacement for '</TestAction>' that is brittle and depends on exact whitespace
matching. If the scheme file has different formatting, the replacement silently
fails and environment variables aren't injected, causing tests to fail
mysteriously. Replace the string matching with a regex pattern that uses anchors
to match the closing tag more flexibly, and add a check to throw an error if the
replacement doesn't occur, making failures fail-fast and obvious rather than
silent.
In `@apps/swift/Sources/PackRat/PackRatApp.swift`:
- Around line 16-19: The code writes test-specific UserDefaults values to
persistent storage when the --reset-auth flag is present, which leaks into
normal app behavior on subsequent launches. Instead of using
UserDefaults.standard to persist these test-only settings, either scope the
writes to a temporary, non-persistent storage mechanism that only applies during
the current test session, or remove the persistent writes entirely and ensure
these configuration values are only applied during active UI test execution
without being saved across application launches.
In `@apps/swift/Tests/PackRatMacUITests/MacHomeFeatureTests.swift`:
- Around line 14-43: The test testShoppingListTileSupportsAddToggleClearAndDone
creates a shopping item but never cleans it up, causing state pollution across
test runs. Add a private property to track the created shopping item name, store
the itemName value in this property before executing the test logic, and
implement a tearDown method that navigates to the Home sidebar, taps the
shopping list tile, and deletes the tracked item by name (using swipe-to-delete
or API call). Additionally, fix the brittle UI selector on line 39-40 that uses
firstMatch with a prefix predicate—instead of selecting the first toggle button
matching the prefix, identify and tap the toggle specifically for the created
item to avoid targeting the wrong item if multiple shopping items exist.
In `@apps/swift/Tests/PackRatMacUITests/MacPackTripTests.swift`:
- Around line 4-26: The test method testCreateOpenAndAddItemToPack creates a
pack and item without cleaning them up, causing test isolation violations and
data accumulation in the E2E database. Add a createdPackName property to the
test class to track the pack name created in the test, assign the packName value
to this property so it can be tracked for cleanup, and then implement a
tearDown() override that deletes the tracked pack by navigating to the Packs
sidebar, finding the pack cell by name, swiping left, and tapping the Delete
button, then resetting createdPackName to nil. The pack deletion should
cascade-delete the associated item automatically per the database schema.
- Around line 28-45: The testCreateOpenAndDeleteTrip method has unreliable
cleanup because trip deletion is wrapped in nested conditionals within test
assertions. If any UI operation fails (navigation, swipe, button appearance),
the trip persists. Store the created trip name in a property variable
(createdTripName), set it at the beginning of the test, clear it when UI
deletion succeeds, and implement an override tearDown method that uses the API
to delete the trip if the UI deletion failed (createdTripName is still set).
This ensures cleanup happens reliably regardless of UI state.
In `@apps/swift/Tests/PackRatMacUITests/MacSecondaryFeatureTests.swift`:
- Around line 4-29: The test method testAssistantInputSendAndClearControlsOnMac
sends a message but only clears the chat history if the Clear button exists,
which can cause state accumulation across test runs if the button is
unavailable. Replace the conditional check that only taps the Clear button if it
exists with a waitFor call that waits for the Clear button to become available
before tapping it, ensuring the chat is always cleared after the test sends a
message. Alternatively, move the chat clearing logic to a tearDown method to
guarantee it always executes regardless of button availability.
In `@apps/swift/Tests/PackRatMacUITests/MacUITestCase.swift`:
- Around line 6-17: The MacUITestCase base class provides comprehensive setup
but lacks guidance on data cleanup, causing test isolation issues. Add
documentation to the class header explaining that subclasses must clean up
persistent data (packs, trips, items, etc.) created during tests by overriding
tearDown() or tearDownWithError(). Additionally, add a helper method like
deletePack(named:) that encapsulates common UI-based cleanup patterns, reducing
duplication and encouraging proper cleanup in subclasses.
In `@apps/swift/Tests/PackRatUITests/AppUITestCase.swift`:
- Around line 105-119: The labels array in the dismissSystemPrompts function
contains a duplicate "Don't Save" entry on line 106. Remove one of the two
identical "Don't Save" strings from the labels array, keeping only a single
occurrence. This will clean up the code and remove the apparent copy-paste error
without changing the function's behavior.
In `@apps/swift/Tests/PackRatUITests/HomeTileTests.swift`:
- Line 82: The test in HomeTileTests.swift uses the global "Clear Purchased"
action (the tap() call on menuButton with id "shopping_clear_purchased") which
removes all purchased items from the shopping list, creating cross-test
pollution and potential flakiness. Instead of relying on this global action,
implement a tearDown() override method that specifically deletes only the
shopping items created by this test instance. Track the itemName generated
during the test and use either an API call or UI interaction to remove only that
specific item in the tearDown() method, ensuring each test cleans up only its
own state before calling super.tearDown().
- Around line 74-75: The toggle selection uses firstMatch with a prefix
predicate on shopping_toggle_ identifiers, which selects the first matching
button in the view hierarchy rather than the specific item created by this test.
This causes flakiness when orphaned shopping items from previous tests exist.
Fix this by scoping the toggle selection to the specific item created in this
test. Either use a more specific identifier that includes the unique item name
or ID from this test instance (instead of relying on the loose prefix match), or
find the cell containing the itemName variable and select the toggle within that
specific cell. This ensures you toggle the correct item regardless of orphaned
items in the view hierarchy.
In `@apps/swift/Tests/PackRatUITests/ScreenshotSmokeTests.swift`:
- Around line 18-20: The search field lookup in the test contains an unnecessary
fallback that checks for both "Search locations..." (three periods) and "Search
locations…" (Unicode ellipsis). Since the app definition in WeatherView.swift
uses only the Unicode ellipsis variant and other tests like WeatherTests.swift
and NavigationTests.swift correctly use only this variant, remove the ternary
conditional logic and simplify the lookup to directly reference only the
app.textFields["Search locations…"] with the Unicode ellipsis character.
In `@docs/ci/swift-e2e-runner.md`:
- Around line 46-49: Add an inline comment to explain the `caffeinate -dimsu`
flags used in both the mac-smoke and mac-ui e2e test commands. The comment
should clarify that these flags prevent different types of sleep: -d prevents
display sleep, -i prevents idle sleep, -m prevents disk sleep, -s prevents
system sleep, and -u declares the process is user-active. This will help future
maintainers understand the purpose of the caffeinate command without needing to
look up the manual.
In `@packages/api/drizzle/0037_big_archangel.sql`:
- Around line 1-43: The migration creates tables with foreign key constraints
but omits indexes on columns that are frequently queried in the feed route
(post_id and user_id fields). Add CREATE INDEX statements after the ALTER TABLE
statements to index the following columns: posts.user_id, post_likes.post_id,
and post_comments.post_id. Note that post_likes.user_id and
comment_likes.comment_id are already indexed implicitly through their UNIQUE
constraints, so they do not require separate indexes.
In `@packages/api/src/routes/packs/index.ts`:
- Line 118: Remove the redundant fallback operator in the line that assigns the
category variable. Since the CreatePackRequestSchema validation already applies
a default value of DEFAULT_PACK_CATEGORY to the category field, data.category is
guaranteed to be defined after validation. Simply assign data.category directly
without the || DEFAULT_PACK_CATEGORY fallback, or inline the data.category
reference directly where the category variable is used at line 129 to eliminate
the variable altogether.
- Around line 142-145: The catch block handling pack creation errors is missing
the required captureApiException call for proper error tracking. Add an import
for captureApiException from `@packrat/api/utils/sentry` at the top of the file if
not already present, and then in the catch block for the pack creation error
handler, call captureApiException with the error object before or alongside the
console.error statement to ensure structured error logging and monitoring as per
coding guidelines for all route handlers that interact with the database.
---
Outside diff comments:
In `@apps/swift/Sources/PackRat/Navigation/AppNavigation.swift`:
- Around line 107-138: The macOS sidebar implementation using Button elements
does not provide visual feedback for the currently selected navigation item,
unlike the non-macOS List-based implementation which has automatic selection
highlighting. Add conditional styling to the Button in the macOS sidebar (within
the first `#if` os(macOS) block) to visually distinguish the active navigation
item by comparing the current item with state.navItem. Apply a background color,
overlay, or opacity change to the Label when item equals state.navItem to
indicate selection state, matching the visual feedback that users would see in
the non-macOS version.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60bf923e-69e8-4767-a308-98a3d3e25278
📒 Files selected for processing (47)
.github/actionlint.yaml.github/workflows/swift-e2e.yml.gitignoreapps/swift/README.mdapps/swift/Sources/PackRat/Features/Catalog/CatalogView.swiftapps/swift/Sources/PackRat/Features/Chat/ChatView.swiftapps/swift/Sources/PackRat/Features/Feed/ComposePostView.swiftapps/swift/Sources/PackRat/Features/Feed/FeedView.swiftapps/swift/Sources/PackRat/Features/Home/HomeView.swiftapps/swift/Sources/PackRat/Features/PackTemplates/PackTemplateFormView.swiftapps/swift/Sources/PackRat/Features/PackTemplates/PackTemplatesView.swiftapps/swift/Sources/PackRat/Features/Packs/PackFormView.swiftapps/swift/Sources/PackRat/Features/Packs/PackItemFormView.swiftapps/swift/Sources/PackRat/Features/Packs/PacksListView.swiftapps/swift/Sources/PackRat/Features/Shopping/ShoppingListView.swiftapps/swift/Sources/PackRat/Features/TrailConditions/TrailConditionsView.swiftapps/swift/Sources/PackRat/Features/Trips/TripFormView.swiftapps/swift/Sources/PackRat/Features/Trips/TripsListView.swiftapps/swift/Sources/PackRat/Features/Weather/WeatherAlertPreferencesView.swiftapps/swift/Sources/PackRat/Features/Weather/WeatherView.swiftapps/swift/Sources/PackRat/Navigation/AppNavigation.swiftapps/swift/Sources/PackRat/Network/APIClient.swiftapps/swift/Sources/PackRat/PackRatApp.swiftapps/swift/Tests/PackRatMacUITests/MacHomeFeatureTests.swiftapps/swift/Tests/PackRatMacUITests/MacNavigationTests.swiftapps/swift/Tests/PackRatMacUITests/MacPackTripTests.swiftapps/swift/Tests/PackRatMacUITests/MacSecondaryFeatureTests.swiftapps/swift/Tests/PackRatMacUITests/MacSmokeTests.swiftapps/swift/Tests/PackRatMacUITests/MacUITestCase.swiftapps/swift/Tests/PackRatMacUITests/MacWeatherTests.swiftapps/swift/Tests/PackRatUITests/AppUITestCase.swiftapps/swift/Tests/PackRatUITests/AuthTests.swiftapps/swift/Tests/PackRatUITests/HomeTileTests.swiftapps/swift/Tests/PackRatUITests/PackSubFlowTests.swiftapps/swift/Tests/PackRatUITests/ScreenshotSmokeTests.swiftapps/swift/project.ymlapps/swift/scripts/run-e2e.test.tsapps/swift/scripts/run-e2e.tsdocs/ci/swift-e2e-runner.mddocs/plans/2026-05-05-001-feat-swift-e2e-coverage-plan.mdpackage.jsonpackages/api/drizzle/0037_big_archangel.sqlpackages/api/drizzle/meta/0037_snapshot.jsonpackages/api/drizzle/meta/_journal.jsonpackages/api/src/routes/packs/index.tspackages/api/src/schemas/packs.tspackages/api/test/packs.test.ts
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the workflow file and examine the specified lines
fd 'swift-e2e.yml' --type fRepository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
# Read the workflow file to see lines 66-68 and 166-168
cat -n .github/workflows/swift-e2e.yml | sed -n '60,75p; 160,175p'Repository: PackRat-AI/PackRat
Length of output: 1246
🌐 Web query:
actions/checkout persist-credentials false security best practice
💡 Result:
Setting persist-credentials: false in actions/checkout is a recommended security best practice to prevent the automatic persistence of Git credentials (such as the GITHUB_TOKEN or SSH keys) into the runner's local Git configuration [1][2]. By default (persist-credentials: true), the action configures the runner's local Git environment with the necessary authentication credentials to allow subsequent steps to perform authenticated Git operations (like git push or git fetch) automatically [3][4]. However, this also makes these credentials accessible to any subsequent code or action running within the same job [5]. Key points regarding this practice: Security Benefit: Disabling this feature reduces the risk of credential exposure [1]. If a malicious or vulnerable action is executed in your workflow, it cannot easily read the stored credentials from the Git configuration files [2][5]. When to use false: You should set persist-credentials: false in all workflows that do not explicitly require the workflow to perform authenticated Git operations [1][2]. This is the most secure configuration and limits the attack surface of your CI/CD pipeline [2]. When to keep true: You should only leave it as the default (true) if your workflow specifically needs to perform authenticated Git commands, such as pushing commits, creating tags, or fetching from private repositories using the automatically provided credentials [3][4]. Context for Modern Versions: Recent versions of actions/checkout have improved how credentials are handled (e.g., storing them in temporary locations rather than directly in .git/config) to mitigate some risks [3][6][7]. Even with these improvements, explicitly setting persist-credentials: false remains a best practice to follow the principle of least privilege [1][2].
Citations:
- 1: https://cheatsheetseries.owasp.org/cheatsheets/GitHub_Actions_Security_Cheat_Sheet.html
- 2: https://adaptive-enforcement-lab.com/secure/github-actions-security/third-party-actions/common-actions/
- 3: https://github.com/actions/checkout/?tab=readme-ov-file
- 4: https://github.com/actions/checkout/blob/master/adrs/0153-checkout-v2.md
- 5: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 6: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 7: [security] Escalate concerning default
persist-credentials=trueplease actions/checkout#2312
Pin actions/checkout to commit SHA and disable credential persistence in both jobs.
actions/checkout@v6 uses a mutable tag instead of a full commit SHA (required by your coding guidelines to prevent supply-chain attacks). Additionally, set persist-credentials: false to reduce credential exposure—this workflow only needs checkout and doesn't require authenticated git operations.
Suggested patch
- name: Checkout repository
uses: actions/checkout@<full_commit_sha>
+ with:
+ persist-credentials: falseAlso applies to: 166-168
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 66-67: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 67-67: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/swift-e2e.yml around lines 66 - 68, The workflow uses the
mutable tag actions/checkout@v6 instead of a pinned commit SHA, which violates
security guidelines. At both occurrences of the actions/checkout step (at lines
66-68 and 166-168 in the swift-e2e.yml file), replace the version tag with a
specific commit SHA, and add the persist-credentials option set to false. This
ensures supply-chain attack prevention by pinning to an immutable commit, and
reduces credential exposure since this workflow only needs checkout
functionality without authenticated git operations.
Source: Linters/SAST tools
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/swift-e2e.ymlRepository: PackRat-AI/PackRat
Length of output: 9597
Pin all third-party actions to immutable commit SHAs.
Actions currently use mutable tags (@v*), which allows silent updates that can introduce supply-chain attacks. Replace with full commit SHAs across:
actions/checkout@v6(lines 67, 167)maxim-lobanov/setup-xcode@v1(lines 70, 170)oven-sh/setup-bun@v2(lines 75, 175)actions/upload-artifact@v7(lines 133, 141, 150, 224, 232, 241)
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 66-67: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 67-67: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/swift-e2e.yml at line 67, In the file
.github/workflows/swift-e2e.yml, replace all mutable version tags with full
commit SHAs to prevent supply-chain attacks. Specifically: at lines 67 and 167,
replace actions/checkout@v6 with the v6 release commit SHA; at lines 70 and 170,
replace maxim-lobanov/setup-xcode@v1 with its commit SHA; at lines 75 and 175,
replace oven-sh/setup-bun@v2 with its commit SHA; and at lines 133, 141, 150,
224, 232, and 241, replace actions/upload-artifact@v7 with its commit SHA. Use
the official GitHub release pages for each action to find the correct commit
SHAs corresponding to each version tag.
Sources: Coding guidelines, Linters/SAST tools
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: latest | ||
| cache: true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the workflow file exists and read the relevant lines
if [ -f ".github/workflows/swift-e2e.yml" ]; then
echo "=== Lines 75-78 ==="
sed -n '75,78p' ".github/workflows/swift-e2e.yml"
echo ""
echo "=== Lines 175-178 ==="
sed -n '175,178p' ".github/workflows/swift-e2e.yml"
else
echo "File not found: .github/workflows/swift-e2e.yml"
fiRepository: PackRat-AI/PackRat
Length of output: 307
🏁 Script executed:
# Search for the declared Bun version in common config files
echo "=== Searching for bun version declarations ==="
rg "bun.*version|bun@" --type json --type yaml --type toml --type md -iRepository: PackRat-AI/PackRat
Length of output: 989
🏁 Script executed:
# Check for all bun-version references in the workflow file
echo "=== All bun-version references in swift-e2e.yml ==="
rg "bun-version" ".github/workflows/swift-e2e.yml" -nRepository: PackRat-AI/PackRat
Length of output: 181
Pin Bun version in CI instead of latest. Using bun-version: latest makes runs non-reproducible and can break unexpectedly. Pin to the repo's declared toolchain version (bun@1.3.10 from package.json) instead.
Both instances at lines 77 and 177 need to be updated.
Suggested patch
- bun-version: latest
+ bun-version: 1.3.10📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: latest | |
| cache: true | |
| uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: 1.3.10 | |
| cache: true |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 75-75: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/swift-e2e.yml around lines 75 - 78, Replace the dynamic
`bun-version: latest` with a pinned version in the setup-bun action
configuration. Change the bun-version parameter to use the specific version
declared in package.json (1.3.10) instead of latest. This change must be applied
at both occurrences where the setup-bun action is used in the workflow to ensure
reproducible CI runs.
| CURRENT_PROJECT_VERSION: "1" | ||
| CODE_SIGN_STYLE: Automatic | ||
| DEVELOPMENT_TEAM: 7WV9JYCW55 | ||
| DEVELOPMENT_TEAM: 666HGMV2LU |
There was a problem hiding this comment.
Verify that the DEVELOPMENT_TEAM change is intentional.
The DEVELOPMENT_TEAM was updated from 7WV9JYCW55 to 666HGMV2LU across all three targets (PackRat-iOS, PackRat-macOS, and the new PackRatMacUITests). This looks like a team migration, but the PR summary doesn't explicitly mention it. Please confirm this is intentional and not an accidental commit of local development settings.
Also applies to: 152-152, 191-191
🤖 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 `@apps/swift/project.yml` at line 101, Verify that the DEVELOPMENT_TEAM
migration from 7WV9JYCW55 to 666HGMV2LU is intentional across all affected
targets in apps/swift/project.yml (at lines 101, 152, and 191). If this is a
legitimate team migration, update the PR description to explicitly document this
change and explain the reason for the migration. If this was an accidental
commit of local development settings, revert the DEVELOPMENT_TEAM value back to
7WV9JYCW55 in the PackRat-iOS target, PackRat-macOS target, and
PackRatMacUITests target.
| import { | ||
| buildUITestEnv, | ||
| buildXcodeEnv, | ||
| loadDotEnv, | ||
| parseArgs, | ||
| pickIOSDestination, | ||
| redactSecrets, | ||
| } from './run-e2e'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add unit coverage for injectSchemeEnv success and failure paths.
This suite currently skips the highest-risk helper (scheme XML mutation). Add tests that verify env block insertion/replacement and that a missing </TestAction> triggers a hard failure.
Also applies to: 31-157
🤖 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 `@apps/swift/scripts/run-e2e.test.ts` around lines 5 - 12, The test suite is
missing unit test coverage for the injectSchemeEnv function, which handles
high-risk XML scheme mutation operations. Add test cases to verify the success
paths where env block insertion and replacement work correctly in the scheme
XML, and add tests for the failure path where a missing </TestAction> element
causes the function to throw an error and fail hard. Ensure both the happy path
scenarios and the error handling are properly tested to cover the XML
manipulation logic.
| let searchField = app.textFields["Search locations..."].exists | ||
| ? app.textFields["Search locations..."] | ||
| : app.textFields["Search locations…"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the WeatherView search field definition
ast-grep --pattern 'TextField($$$"Search locations"$$$)'Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
cat -n apps/swift/Tests/PackRatUITests/ScreenshotSmokeTests.swift | head -30Repository: PackRat-AI/PackRat
Length of output: 1608
🏁 Script executed:
rg "Search locations" --type swift -B 2 -A 2Repository: PackRat-AI/PackRat
Length of output: 5038
🏁 Script executed:
rg "TextField" --type swift -B 1 -A 1 | grep -A 3 -B 3 "Search"Repository: PackRat-AI/PackRat
Length of output: 2174
Simplify search field lookup to match actual app definition.
The app-side definition in WeatherView.swift uses only the Unicode ellipsis: TextField("Search locations…", text: $viewModel.searchText). Other tests (WeatherTests.swift, NavigationTests.swift) correctly look for only this variant. Remove the fallback to the three-period version, as the app never uses it. The MacWeatherTests includes both as alternates, likely for macOS-specific text handling, but the iOS app is consistent.
🤖 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 `@apps/swift/Tests/PackRatUITests/ScreenshotSmokeTests.swift` around lines 18 -
20, The search field lookup in the test contains an unnecessary fallback that
checks for both "Search locations..." (three periods) and "Search locations…"
(Unicode ellipsis). Since the app definition in WeatherView.swift uses only the
Unicode ellipsis variant and other tests like WeatherTests.swift and
NavigationTests.swift correctly use only this variant, remove the ternary
conditional logic and simplify the lookup to directly reference only the
app.textFields["Search locations…"] with the Unicode ellipsis character.
| ```sh | ||
| caffeinate -dimsu bun run e2e:swift:mac-smoke | ||
| caffeinate -dimsu bun run e2e:swift:mac-ui | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider explaining the caffeinate flags.
The -dimsu flags prevent different types of sleep but aren't self-documenting. A brief inline comment would help maintainers:
d: prevent display sleepi: prevent idle sleepm: prevent disk sleeps: prevent system sleepu: declare process is user-active
🤖 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 `@docs/ci/swift-e2e-runner.md` around lines 46 - 49, Add an inline comment to
explain the `caffeinate -dimsu` flags used in both the mac-smoke and mac-ui e2e
test commands. The comment should clarify that these flags prevent different
types of sleep: -d prevents display sleep, -i prevents idle sleep, -m prevents
disk sleep, -s prevents system sleep, and -u declares the process is
user-active. This will help future maintainers understand the purpose of the
caffeinate command without needing to look up the manual.
| CREATE TABLE "comment_likes" ( | ||
| "id" serial PRIMARY KEY NOT NULL, | ||
| "comment_id" integer NOT NULL, | ||
| "user_id" integer NOT NULL, | ||
| "created_at" timestamp DEFAULT now() NOT NULL, | ||
| CONSTRAINT "comment_likes_comment_id_user_id_unique" UNIQUE("comment_id","user_id") | ||
| ); | ||
| --> statement-breakpoint | ||
| CREATE TABLE "post_comments" ( | ||
| "id" serial PRIMARY KEY NOT NULL, | ||
| "post_id" integer NOT NULL, | ||
| "user_id" integer NOT NULL, | ||
| "content" text NOT NULL, | ||
| "parent_comment_id" integer, | ||
| "created_at" timestamp DEFAULT now() NOT NULL, | ||
| "updated_at" timestamp DEFAULT now() NOT NULL | ||
| ); | ||
| --> statement-breakpoint | ||
| CREATE TABLE "post_likes" ( | ||
| "id" serial PRIMARY KEY NOT NULL, | ||
| "post_id" integer NOT NULL, | ||
| "user_id" integer NOT NULL, | ||
| "created_at" timestamp DEFAULT now() NOT NULL, | ||
| CONSTRAINT "post_likes_post_id_user_id_unique" UNIQUE("post_id","user_id") | ||
| ); | ||
| --> statement-breakpoint | ||
| CREATE TABLE "posts" ( | ||
| "id" serial PRIMARY KEY NOT NULL, | ||
| "user_id" integer NOT NULL, | ||
| "caption" text, | ||
| "images" jsonb NOT NULL, | ||
| "created_at" timestamp DEFAULT now() NOT NULL, | ||
| "updated_at" timestamp DEFAULT now() NOT NULL | ||
| ); | ||
| --> statement-breakpoint | ||
| ALTER TABLE "comment_likes" ADD CONSTRAINT "comment_likes_comment_id_post_comments_id_fk" FOREIGN KEY ("comment_id") REFERENCES "public"."post_comments"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "comment_likes" ADD CONSTRAINT "comment_likes_user_id_users_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."users"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "post_comments" ADD CONSTRAINT "post_comments_post_id_posts_id_fk" FOREIGN KEY ("post_id") REFERENCES "public"."posts"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "post_comments" ADD CONSTRAINT "post_comments_user_id_users_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."users"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "post_comments" ADD CONSTRAINT "post_comments_parent_comment_id_post_comments_id_fk" FOREIGN KEY ("parent_comment_id") REFERENCES "public"."post_comments"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "post_likes" ADD CONSTRAINT "post_likes_post_id_posts_id_fk" FOREIGN KEY ("post_id") REFERENCES "public"."posts"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "post_likes" ADD CONSTRAINT "post_likes_user_id_users_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."users"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "posts" ADD CONSTRAINT "posts_user_id_users_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."users"("id") ON DELETE cascade ON UPDATE no action; No newline at end of file |
There was a problem hiding this comment.
Missing indexes on foreign key columns.
The migration correctly creates the social tables and FK constraints per the schema contract, but it omits indexes on FK columns that will be queried frequently. The feed route (packages/api/src/routes/feed/index.ts:14-88) filters and joins on post_likes.post_id, post_likes.user_id, post_comments.post_id, and posts.user_id. Without indexes on these columns, queries will perform full table scans as the tables grow.
Add indexes for:
posts(user_id)post_likes(post_id)post_likes(user_id)(composite unique constraint already provides this)post_comments(post_id)comment_likes(comment_id)(composite unique constraint already provides this)
Note: The SQLFluff warnings about NOT VALID are false positives—that optimization only applies when adding constraints to tables with existing data, not new empty tables.
🧰 Tools
🪛 SQLFluff (4.2.2)
[error] 36-36: ADD CONSTRAINT ... FOREIGN KEY should use NOT VALID to avoid locking the table while validating existing rows.
(PG01)
[error] 37-37: ADD CONSTRAINT ... FOREIGN KEY should use NOT VALID to avoid locking the table while validating existing rows.
(PG01)
[error] 38-38: ADD CONSTRAINT ... FOREIGN KEY should use NOT VALID to avoid locking the table while validating existing rows.
(PG01)
[error] 39-39: ADD CONSTRAINT ... FOREIGN KEY should use NOT VALID to avoid locking the table while validating existing rows.
(PG01)
[error] 40-40: ADD CONSTRAINT ... FOREIGN KEY should use NOT VALID to avoid locking the table while validating existing rows.
(PG01)
[error] 41-41: ADD CONSTRAINT ... FOREIGN KEY should use NOT VALID to avoid locking the table while validating existing rows.
(PG01)
[error] 42-42: ADD CONSTRAINT ... FOREIGN KEY should use NOT VALID to avoid locking the table while validating existing rows.
(PG01)
[error] 43-43: ADD CONSTRAINT ... FOREIGN KEY should use NOT VALID to avoid locking the table while validating existing rows.
(PG01)
🤖 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 `@packages/api/drizzle/0037_big_archangel.sql` around lines 1 - 43, The
migration creates tables with foreign key constraints but omits indexes on
columns that are frequently queried in the feed route (post_id and user_id
fields). Add CREATE INDEX statements after the ALTER TABLE statements to index
the following columns: posts.user_id, post_likes.post_id, and
post_comments.post_id. Note that post_likes.user_id and comment_likes.comment_id
are already indexed implicitly through their UNIQUE constraints, so they do not
require separate indexes.
| localUpdatedAt: new Date(data.localUpdatedAt as string), | ||
| } as typeof packs.$inferInsert) | ||
| .returning(); | ||
| const category = data.category || DEFAULT_PACK_CATEGORY; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Redundant fallback: schema already provides the default.
The CreatePackRequestSchema applies .default(DEFAULT_PACK_CATEGORY), so data.category is always defined after validation. The || DEFAULT_PACK_CATEGORY fallback is redundant.
♻️ Proposed simplification
- const category = data.category || DEFAULT_PACK_CATEGORY;
+ const category = data.category;Or inline it at line 129:
- const category = data.category || DEFAULT_PACK_CATEGORY;
-
// Zod validates all fields at runtime; cast through the Standard Schema
// inference gap so drizzle's insert accepts the values.
const [newPack] = await db
.insert(packs)
.values({
id: packId,
userId: user.userId,
name: data.name,
description: data.description ?? null,
- category,
+ category: data.category,
isPublic: data.isPublic ?? false,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const category = data.category || DEFAULT_PACK_CATEGORY; | |
| const category = data.category; |
🤖 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 `@packages/api/src/routes/packs/index.ts` at line 118, Remove the redundant
fallback operator in the line that assigns the category variable. Since the
CreatePackRequestSchema validation already applies a default value of
DEFAULT_PACK_CATEGORY to the category field, data.category is guaranteed to be
defined after validation. Simply assign data.category directly without the ||
DEFAULT_PACK_CATEGORY fallback, or inline the data.category reference directly
where the category variable is used at line 129 to eliminate the variable
altogether.
| } catch (error) { | ||
| console.error('Error creating pack:', error); | ||
| return status(500, { error: 'Failed to create pack' }); | ||
| } |
There was a problem hiding this comment.
Missing required captureApiException call in DB error handler.
The catch block must call captureApiException to ensure proper error tracking and structured logging. As per coding guidelines, every route catch block that interacts with the DB must include this call.
🛡️ Proposed fix
+ import { captureApiException } from '`@packrat/api/utils/sentry`';
+
} catch (error) {
+ captureApiException(error, {
+ context: { packId: data.id, userId: user.userId },
+ operation: 'create_pack',
+ });
console.error('Error creating pack:', error);
return status(500, { error: 'Failed to create pack' });
}As per coding guidelines: "Every route catch block and service method in packages/api that interacts with the DB or external API must have a captureApiException call from @packrat/api/utils/sentry"
🤖 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 `@packages/api/src/routes/packs/index.ts` around lines 142 - 145, The catch
block handling pack creation errors is missing the required captureApiException
call for proper error tracking. Add an import for captureApiException from
`@packrat/api/utils/sentry` at the top of the file if not already present, and
then in the catch block for the pack creation error handler, call
captureApiException with the error object before or alongside the console.error
statement to ensure structured error logging and monitoring as per coding
guidelines for all route handlers that interact with the database.
Source: Coding guidelines
Deploying packrat-guides with
|
| Latest commit: |
6056028
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4107bb73.packrat-guides-6gq.pages.dev |
| Branch Preview URL: | https://feat-swift-e2e-coverage.packrat-guides-6gq.pages.dev |
Coverage Report for packages/analytics (./packages/analytics)
File CoverageNo changed files found. |
Coverage Report for packages/units (./packages/units)
File CoverageNo changed files found. |
Coverage Report for packages/overpass (./packages/overpass)
File CoverageNo changed files found. |
Coverage Report for packages/mcp (./packages/mcp)
File CoverageNo changed files found. |
Coverage Report for packages/api (./packages/api)
File CoverageNo changed files found. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packrat-admin | 6056028 | Commit Preview URL Branch Preview URL |
Jun 17 2026, 04:48 AM |
Coverage Report for apps/expo (./apps/expo)
File CoverageNo changed files found. |
Deploying packrat-landing with
|
| Latest commit: |
6056028
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b3c57ba7.packrat-landing.pages.dev |
| Branch Preview URL: | https://feat-swift-e2e-coverage.packrat-landing.pages.dev |
1 participant
Summary
PackRatMacUITeststargete2e:swift:mac-smokeande2e:swift:ios-smokepackrat-e2e, full macOS on push/nightly/manual, Swift iOS nightly/manual onlyxcresulttoolGitHub step summaries,.xcresultuploads, screenshots, and failure triage artifactscaffeinate, secrets, data hygiene, and the Expo-vs-Swift E2E splitValidation
bun run test:swift:runneractionlint .github/workflows/swift-e2e.ymlE2E_API_BASE_URL=http://localhost:8788 bun run e2e:swift:mac-ui-> 14 tests, 0 failuresE2E_API_BASE_URL=http://localhost:8788 bun run e2e:swift:ios-> 78 tests, 0 failuresNotes
packrat-e2ewith Automation Mode/TCC configured and an active logged-in GUI session.Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Tests