[codex] Add Swift E2E coverage#2596
Merged
Merged
Conversation
github-actions
Bot
added
documentation
Contributor
Coverage Report for packages/analytics (./packages/analytics)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/overpass (./packages/overpass)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/api (./packages/api)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/mcp (./packages/mcp)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/units (./packages/units)
File CoverageNo changed files found. |
Contributor
Coverage Report for apps/expo (./apps/expo)
File CoverageNo changed files found. |
Contributor
WalkthroughAdds a complete Swift E2E test infrastructure: a macOS app delegate with ChangesPack Schema Nullable Fixes and Social Posts Migration
Swift E2E Infrastructure and CI Workflow
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Runner as Self-hosted macOS Runner
participant APIClient as PackRat APIClient
participant XCTest as XCUITest (MacUITestCase)
participant API as E2E API
CI->>Runner: trigger swift-e2e.yml (PR=smoke / push=full)
Runner->>Runner: xcodegen generate, bun install
Runner->>API: seed E2E user
Runner->>XCTest: xcodebuild test (caffeinate)
XCTest->>XCTest: setUpWithError() -- reset-auth, forward E2E_API_BASE_URL
XCTest->>APIClient: app launch -- resolvedBaseURL reads E2E_API_BASE_URL
APIClient->>API: requests routed to E2E worker
XCTest->>XCTest: loginIfNeeded() -- fill E2E_EMAIL/E2E_PASSWORD
XCTest->>XCTest: run test (goToSidebar / interactions)
XCTest->>Runner: on failure -- attach screenshot + hierarchy dump
Runner->>CI: upload xcresult, screenshots, triage bundle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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 |
Contributor
Deploying packrat-guides with
|
| Latest commit: |
5d9cd6d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://443b1398.packrat-guides-6gq.pages.dev |
| Branch Preview URL: | https://feat-swift-e2e-coverage.packrat-guides-6gq.pages.dev |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packrat-admin | 5d9cd6d | Commit Preview URL Branch Preview URL |
Jun 18 2026, 04:59 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
apps/swift/Sources/PackRat/PackRatApp.swift (1)
14-27:⚠️ Potential issue | 🔴 CriticalMerge the two
init()declarations; this currently won't compile.
PackRatAppdeclaresinit()twice with the same signature, which is an invalid redeclaration in Swift. Combine both initializers:Proposed fix
- init() { - `#if` os(macOS) - if ProcessInfo.processInfo.arguments.contains("--reset-auth") { - UserDefaults.standard.set(true, forKey: "ApplePersistenceIgnoreState") - UserDefaults.standard.set(false, forKey: "NSQuitAlwaysKeepsWindows") - } - `#endif` - } - - init() { - // Telemetry has to start before any view is mounted so launch-time - // errors are captured. A missing DSN silently disables the SDK. - SentryConfig.start() - } + init() { + // Telemetry has to start before any view is mounted so launch-time + // errors are captured. A missing DSN silently disables the SDK. + SentryConfig.start() + + `#if` os(macOS) + if ProcessInfo.processInfo.arguments.contains("--reset-auth") { + UserDefaults.standard.set(true, forKey: "ApplePersistenceIgnoreState") + UserDefaults.standard.set(false, forKey: "NSQuitAlwaysKeepsWindows") + } + `#endif` + }🤖 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/PackRatApp.swift` around lines 14 - 27, The PackRatApp has two init() declarations with identical signatures, which causes a compiler error as Swift does not allow duplicate initializer declarations. Combine both initializers into a single init() method that includes both the conditional macOS-specific UserDefaults configuration (checking for --reset-auth argument and setting ApplePersistenceIgnoreState and NSQuitAlwaysKeepsWindows) and the SentryConfig.start() telemetry initialization call. Ensure the macOS-specific code remains wrapped in the `#if` os(macOS) preprocessor conditional, while the SentryConfig.start() call executes unconditionally.apps/swift/Tests/PackRatUITests/PackSubFlowTests.swift (1)
127-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix stale button identifier in
createPackhelper.Line 127 still targets
packs_new_pack_button, but the view now exposesnew_pack_button. This will fail before pack creation starts.Suggested fix
- waitFor(app.buttons["packs_new_pack_button"]).tap() + waitFor(app.buttons["new_pack_button"]).tap()🤖 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/PackSubFlowTests.swift` at line 127, In the createPack helper method, update the button identifier in the waitFor call that taps the new pack button. Change the identifier string from "packs_new_pack_button" to "new_pack_button" to match the current exposed identifier in the view. This will ensure the test can properly locate and interact with the correct button when creating a new pack.
🤖 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:
- Around line 83-95: The secret validation in the Verify Swift E2E secrets step
is missing a check for PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN, which is a required
secret consumed by the workflow. Add a validation check for this secret using
the same pattern as the existing checks (checking if the variable is empty with
[ -z "${PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN:-}" ] &&
missing+=("PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN")), and ensure it is also included
in the env section mapping it from secrets.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN.
This same validation gap exists in the second location mentioned at lines
183-195, so apply the same fix there as well.
- Around line 66-67: The checkout action references use a mutable tag (v6)
instead of a pinned full SHA, and the persist-credentials option is not set,
which weakens supply-chain security. Replace the mutable tag reference in the
actions/checkout action with a full commit SHA, and add the persist-credentials:
false parameter to disable storing credentials in git config. Apply this same
fix pattern to all other third-party action references in the workflow file that
currently use mutable tags (v1, v2, v7, etc.) at the affected lines listed in
the comment.
In `@apps/swift/README.md`:
- Around line 10-15: The mkdir command in the temp-directory workaround uses a
machine and user-specific absolute path that is not portable across different
developers and CI runners. Replace the hardcoded path
/Volumes/CrucialX10/tmp/andrewbierman with a portable environment variable
reference that uses the system's configured temp directory, falling back to a
standard location if needed. This will make the instruction work universally
regardless of the user's specific machine configuration.
In `@apps/swift/Sources/PackRat/Features/Feed/FeedView.swift`:
- Around line 39-42: The accessibilityIdentifier for the new post button has
been changed from "feed_new_post_button" to "new_post_button" in FeedView.swift,
but the test file VisualScreenshotTests.swift still references the old
identifier at lines 459-473. Update the test selector in
VisualScreenshotTests.swift to use the new identifier "new_post_button" instead
of "feed_new_post_button" to ensure the UI tests can properly locate and
interact with the new post button element.
In `@apps/swift/Sources/PackRat/Features/Packs/PacksListView.swift`:
- Around line 65-67: Remove the duplicate accessibilityIdentifier call on line
65 that sets "packs_new_pack_button" in the toolbar button configuration,
keeping only the "new_pack_button" identifier on line 67 as the effective ID.
Then update all UI test files including VisualScreenshotTests and
PackSubFlowTests to replace their queries for "packs_new_pack_button" with
"new_pack_button" to match the renamed accessibility identifier in
PacksListView.
In `@apps/swift/Sources/PackRat/Features/Trips/TripsListView.swift`:
- Around line 43-45: The button in TripsListView has duplicate
accessibilityIdentifier modifiers that conflict with each other. The modifier on
line 45 with identifier "plan_trip_button" overwrites the one on line 43 with
"trips_plan_trip_button", but the old identifier is required by 11+ existing
test assertions across multiple test files. Remove the duplicate
accessibilityIdentifier modifier on line 45 to preserve the
"trips_plan_trip_button" identifier that the tests depend on.
In `@apps/swift/Sources/PackRat/PackRatApp.swift`:
- Around line 16-19: The code in the `--reset-auth` argument check block is
setting UserDefaults values that persist permanently across app launches,
causing test runs to affect subsequent app behavior. Remove the
UserDefaults.standard.set calls for "ApplePersistenceIgnoreState" and
"NSQuitAlwaysKeepsWindows" from this conditional block, or if these mutations
are necessary for the reset-auth functionality, ensure the original values are
saved before modification and then restored after the reset-auth operation
completes so the changes do not persist beyond the reset cycle.
In `@apps/swift/Tests/PackRatMacUITests/MacHomeFeatureTests.swift`:
- Around line 42-43: The test is using macTapIfExists() on the "shopping_done"
button, which allows the test to pass even when the button is missing, masking
missing functionality. Replace macTapIfExists() with an assertion that the
button exists (using XCTAssertTrue or similar) followed by a regular tap()
method call to ensure the Done flow is properly validated and will fail if the
button is not present.
In `@apps/swift/Tests/PackRatMacUITests/MacPackTripTests.swift`:
- Around line 37-44: The nested if statements checking cell.waitForExistence and
deleteButton.waitForExistence allow the test to pass silently when either the
trip cell or delete button never appears, instead of failing the test. Remove
the conditional wrappers around these waitForExistence calls so that the test
will fail explicitly if the cell or deleteButton do not appear within the
specified timeout periods, rather than allowing the test to continue when these
prerequisites are missing.
In `@apps/swift/Tests/PackRatMacUITests/MacSecondaryFeatureTests.swift`:
- Around line 111-115: The waitForSearchToSettle function ignores the result
returned by XCTWaiter.wait(), which masks timeout or failure conditions and
allows tests to continue even when the expectation is not satisfied. Instead of
assigning the result to an underscore, capture the actual result from
XCTWaiter.wait() and add an assertion to verify that it equals
XCTWaiter.Result.completed, ensuring that the test fails properly if the search
indicator does not disappear within the timeout period.
In `@apps/swift/Tests/PackRatMacUITests/MacSmokeTests.swift`:
- Around line 15-16: Replace the `.exists` property checks with
`waitForExistence()` method calls for the login controls to eliminate race
conditions in macOS UI testing. Specifically, change the assertions for
app.secureTextFields["login_password"] and app.buttons["login_submit"] to use
waitForExistence instead of checking the exists property directly. Apply the
same fix to the similar assertions located at lines 40-45 to ensure all login
control interactions properly wait for elements to be available before
proceeding.
In `@apps/swift/Tests/PackRatMacUITests/MacUITestCase.swift`:
- Around line 19-22: The tearDownWithError() method in MacUITestCase terminates
the app before calling the parent class's tearDownWithError(), which prevents
the parent from capturing screenshot and hierarchy artifacts for failure
diagnostics. Reorder the operations in tearDownWithError() to call
super.tearDownWithError() first (to allow artifact capture), then call
app.terminate() afterward. Apply the same fix to the other tearDown-related
method mentioned (lines 142-157) to ensure consistent behavior across all test
teardown paths.
- Around line 38-40: The test is tapping and typing into the login_password
secure text field without first waiting for it to exist, which can cause UI race
condition failures. Add a waitForExistence check on the passwordField element
(assigned from app.secureTextFields["login_password"]) with an appropriate
timeout before calling tap() and typeText(password) methods to ensure the
element is ready for interaction.
In `@apps/swift/Tests/PackRatUITests/HomeTileTests.swift`:
- Around line 9-21: The Wildlife ID tile entry in the navigationTiles array is
being unconditionally included, but it should be gated behind the same feature
flag used elsewhere in the codebase. Identify the feature flag that controls
Wildlife ID functionality in other UI tests, then apply that same conditional
check to include the Tile with id "home_tile_wildlife_id" only when the feature
is enabled. This ensures that testEveryHomeNavigationTileOpensDestination
behaves correctly when the feature is disabled.
In `@apps/swift/Tests/PackRatUITests/PackSubFlowTests.swift`:
- Around line 133-139: The category button selection is using brittle
NSPredicate matching on visible labels like "Category", "None", and "Hiking",
which breaks with localization changes. Instead, add stable accessibility
identifiers to the category control button and the "Hiking" option in the source
code, then update the test to target those accessibility IDs directly using
app.buttons with the accessibilityIdentifier instead of the NSPredicate label
format matching. This will make the UI test more resilient and maintainable
across language and UI copy changes.
In `@docs/ci/swift-e2e-runner.md`:
- Line 94: Update the documentation in docs/ci/swift-e2e-runner.md line 94 to
accurately reflect what variables the injectScheme() function in
apps/swift/scripts/run-e2e.ts actually injects into the Xcode schemes. Remove
E2E_API_BASE_URL and E2E_SCREENSHOT_DIR from the claim about scheme injection
since the injectScheme() function only injects E2E_EMAIL and E2E_PASSWORD into
TestAction EnvironmentVariables, or clarify how these other variables are passed
to the tests if they are used elsewhere in the runner.
In `@packages/api/drizzle/0037_big_archangel.sql`:
- Around line 9-17: The post_comments table migration is missing database
indexes on frequently-used foreign key columns. Add indexes for
post_comments.post_id (used in feed aggregation and filtering) and
post_comments.parent_comment_id (used for threaded lookups) to prevent table
scans as data volume grows. Update the schema definition to include these
indexes and regenerate the migration file to reflect these index creations in
the SQL.
- Around line 1-43: The social content tables (comment_likes, post_comments,
post_likes, and posts) lack soft-delete support and use hard-delete cascading
constraints, violating the coding guideline that all user-generated content must
use soft deletes. Update the Drizzle schema file (not this SQL migration) by
adding a nullable deleted_at timestamp column to each of the four tables
mentioned above, and modify all foreign key constraints in these tables to use
onDelete: 'no action' instead of cascade to prevent hard-deletion chains. After
updating the schema, regenerate this migration file from the updated schema
definition rather than hand-editing the SQL.
In `@packages/api/src/routes/packs/index.ts`:
- Around line 185-187: The captureApiException call in the create-pack catch
block is using incorrect invocation shape with positional arguments. Instead of
calling captureApiException(error, { route: 'packs.create' }) with two separate
arguments, refactor it to pass a single object parameter that contains both the
error and the operation context together, following the pattern expected by the
captureApiException utility from `@packrat/api/utils/sentry`.
In `@packages/api/test/packs.test.ts`:
- Around line 182-189: The test 'defaults missing category to custom' uses
non-deterministic wall-clock time values (Date.now() for the id and new
Date().toISOString() for localCreatedAt and localUpdatedAt), which violates the
determinism requirement. Replace these dynamic date/time values with fixed,
mocked constants so the test produces consistent results regardless of when it
runs. Additionally, tighten the status code assertions at lines 193 and 213 to
expect a specific HTTP status code (likely 200 or 201) instead of allowing
multiple status codes, which will improve regression detection and enforce a
stricter contract. Apply the same fixes to the related test at lines 201-209.
---
Outside diff comments:
In `@apps/swift/Sources/PackRat/PackRatApp.swift`:
- Around line 14-27: The PackRatApp has two init() declarations with identical
signatures, which causes a compiler error as Swift does not allow duplicate
initializer declarations. Combine both initializers into a single init() method
that includes both the conditional macOS-specific UserDefaults configuration
(checking for --reset-auth argument and setting ApplePersistenceIgnoreState and
NSQuitAlwaysKeepsWindows) and the SentryConfig.start() telemetry initialization
call. Ensure the macOS-specific code remains wrapped in the `#if` os(macOS)
preprocessor conditional, while the SentryConfig.start() call executes
unconditionally.
In `@apps/swift/Tests/PackRatUITests/PackSubFlowTests.swift`:
- Line 127: In the createPack helper method, update the button identifier in the
waitFor call that taps the new pack button. Change the identifier string from
"packs_new_pack_button" to "new_pack_button" to match the current exposed
identifier in the view. This will ensure the test can properly locate and
interact with the correct button when creating a new pack.
🪄 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: f84bea86-4484-44d5-a855-5c2d79586003
📒 Files selected for processing (28)
.github/actionlint.yaml.github/workflows/swift-e2e.ymlapps/swift/README.mdapps/swift/Sources/PackRat/Features/Chat/ChatView.swiftapps/swift/Sources/PackRat/Features/Feed/FeedView.swiftapps/swift/Sources/PackRat/Features/Packs/PacksListView.swiftapps/swift/Sources/PackRat/Features/Trips/TripsListView.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.swiftdocs/ci/swift-e2e-runner.mddocs/plans/2026-05-05-001-feat-swift-e2e-coverage-plan.mdpackage.jsonpackages/api/drizzle/0037_big_archangel.sqlpackages/api/src/routes/packs/index.tspackages/api/test/packs.test.tspackages/schemas/src/packs.ts
Comment on lines
+66
to
+67
| - name: Checkout repository | ||
| uses: actions/checkout@v6 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/swift-e2e.yml"
echo "== Unpinned uses (not full SHA) =="
rg -n '^\s*uses:\s*\S+' "$file" | rg -v '@[0-9a-fA-F]{40}$' || true
echo
echo "== Checkout steps missing persist-credentials: false =="
python - <<'PY'
import re, pathlib
p = pathlib.Path(".github/workflows/swift-e2e.yml")
text = p.read_text()
blocks = re.finditer(r'(?ms)^\s*-\s+name:.*?(?=^\s*-\s+name:|\Z)', text)
for m in blocks:
block = m.group(0)
if "uses: actions/checkout@" in block and "persist-credentials: false" not in block:
line = text[:m.start()].count("\n") + 1
print(f"Checkout step near line {line} is missing persist-credentials: false")
PYRepository: PackRat-AI/PackRat
Length of output: 800
Pin third-party actions to full SHAs and disable persisted checkout credentials.
All action references use mutable tags (v6, v1, v2, v7). Both checkout steps keep credentials in git config. This weakens CI supply-chain and token-hardening posture.
Suggested patch pattern
- - name: Checkout repository
- uses: actions/checkout@v6
+ - name: Checkout repository
+ uses: actions/checkout@<FULL_40_CHAR_COMMIT_SHA>
+ with:
+ persist-credentials: false
- - name: Setup Xcode
- uses: maxim-lobanov/setup-xcode@v1
+ - name: Setup Xcode
+ uses: maxim-lobanov/setup-xcode@<FULL_40_CHAR_COMMIT_SHA>
- - name: Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: Setup Bun
+ uses: oven-sh/setup-bun@<FULL_40_CHAR_COMMIT_SHA>
- - name: Upload macOS xcresult
- uses: actions/upload-artifact@v7
+ - name: Upload macOS xcresult
+ uses: actions/upload-artifact@<FULL_40_CHAR_COMMIT_SHA>Affects lines: 66–67, 70, 75, 133, 141, 150, 166–167, 170, 175, 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 around lines 66 - 67, The checkout action
references use a mutable tag (v6) instead of a pinned full SHA, and the
persist-credentials option is not set, which weakens supply-chain security.
Replace the mutable tag reference in the actions/checkout action with a full
commit SHA, and add the persist-credentials: false parameter to disable storing
credentials in git config. Apply this same fix pattern to all other third-party
action references in the workflow file that currently use mutable tags (v1, v2,
v7, etc.) at the affected lines listed in the comment.
Sources: Coding guidelines, Linters/SAST tools
Comment on lines
+83
to
+95
| - name: Verify Swift E2E secrets | ||
| run: | | ||
| missing=() | ||
| [ -z "${E2E_EMAIL:-}" ] && missing+=("E2E_TEST_EMAIL") | ||
| [ -z "${E2E_PASSWORD:-}" ] && missing+=("E2E_TEST_PASSWORD") | ||
| [ -z "${E2E_API_BASE_URL:-}" ] && missing+=("SWIFT_E2E_API_BASE_URL") | ||
| [ -z "${NEON_DATABASE_URL:-}" ] && missing+=("NEON_DEV_DATABASE_URL") | ||
| if [ ${#missing[@]} -gt 0 ]; then | ||
| echo "::error::Required Swift E2E secrets missing: ${missing[*]}" | ||
| exit 1 | ||
| fi | ||
| env: | ||
| NEON_DATABASE_URL: ${{ secrets.NEON_DEV_DATABASE_URL }} |
Contributor
There was a problem hiding this comment.
Fail fast on all required Swift E2E secrets.
The workflow validates several secrets but skips PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN even though it is consumed via workflow env. Missing it will fail later with less actionable errors.
Suggested fix
- name: Verify Swift E2E secrets
run: |
missing=()
[ -z "${E2E_EMAIL:-}" ] && missing+=("E2E_TEST_EMAIL")
[ -z "${E2E_PASSWORD:-}" ] && missing+=("E2E_TEST_PASSWORD")
[ -z "${E2E_API_BASE_URL:-}" ] && missing+=("SWIFT_E2E_API_BASE_URL")
[ -z "${NEON_DATABASE_URL:-}" ] && missing+=("NEON_DEV_DATABASE_URL")
+ [ -z "${PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN:-}" ] && missing+=("PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN")
if [ ${`#missing`[@]} -gt 0 ]; then
echo "::error::Required Swift E2E secrets missing: ${missing[*]}"
exit 1
fiAlso applies to: 183-195
🤖 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 83 - 95, The secret validation
in the Verify Swift E2E secrets step is missing a check for
PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN, which is a required secret consumed by the
workflow. Add a validation check for this secret using the same pattern as the
existing checks (checking if the variable is empty with [ -z
"${PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN:-}" ] &&
missing+=("PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN")), and ensure it is also included
in the env section mapping it from secrets.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN.
This same validation gap exists in the second location mentioned at lines
183-195, so apply the same fix there as well.
Comment on lines
+10
to
+15
| If Xcode or SwiftPM reports a temporary-directory error on this machine, ensure | ||
| the configured temp directory exists: | ||
|
|
||
| ```sh | ||
| mkdir -p /Volumes/CrucialX10/tmp/andrewbierman | ||
| ``` |
Contributor
There was a problem hiding this comment.
Generalize the temp-directory workaround instruction.
The command uses a machine/user-specific absolute path, which is not portable for other contributors or runners.
Suggested wording update
-If Xcode or SwiftPM reports a temporary-directory error on this machine, ensure
-the configured temp directory exists:
+If Xcode or SwiftPM reports a temporary-directory error, ensure your configured
+temporary directory exists (for example, `TMPDIR`):
```sh
-mkdir -p /Volumes/CrucialX10/tmp/andrewbierman
+mkdir -p "${TMPDIR:-/tmp}"
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
If Xcode or SwiftPM reports a temporary-directory error, ensure your configured
temporary directory exists (for example, `TMPDIR`):
🤖 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/README.md` around lines 10 - 15, The mkdir command in the
temp-directory workaround uses a machine and user-specific absolute path that is
not portable across different developers and CI runners. Replace the hardcoded
path /Volumes/CrucialX10/tmp/andrewbierman with a portable environment variable
reference that uses the system's configured temp directory, falling back to a
standard location if needed. This will make the instruction work universally
regardless of the user's specific machine configuration.
Comment on lines
39
to
+42
| .accessibilityIdentifier("feed_new_post_button") | ||
| .disabled(!authManager.isAuthenticated) | ||
| .keyboardShortcut("n", modifiers: .command) | ||
| .accessibilityIdentifier("new_post_button") |
Contributor
There was a problem hiding this comment.
Update UI-test consumers for the renamed identifier.
Line 42 makes the effective identifier new_post_button, but apps/swift/Tests/PackRatUITests/VisualScreenshotTests.swift (Line 459-473 snippet) still looks up feed_new_post_button. This will break that flow unless the test selector is migrated in the same PR.
Suggested fix
- destinationIdentifier: "feed_new_post_button"
+ destinationIdentifier: "new_post_button"
- tapAndCapture(identifier: "feed_new_post_button", fallbackButton: "New Post", name: "\(prefix)-feed-compose-sheet")
+ tapAndCapture(identifier: "new_post_button", fallbackButton: "New Post", name: "\(prefix)-feed-compose-sheet")🤖 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/Features/Feed/FeedView.swift` around lines 39 -
42, The accessibilityIdentifier for the new post button has been changed from
"feed_new_post_button" to "new_post_button" in FeedView.swift, but the test file
VisualScreenshotTests.swift still references the old identifier at lines
459-473. Update the test selector in VisualScreenshotTests.swift to use the new
identifier "new_post_button" instead of "feed_new_post_button" to ensure the UI
tests can properly locate and interact with the new post button element.
Comment on lines
65
to
+67
| .accessibilityIdentifier("packs_new_pack_button") | ||
| .keyboardShortcut("n", modifiers: .command) | ||
| .accessibilityIdentifier("new_pack_button") |
Contributor
There was a problem hiding this comment.
Renamed toolbar ID is out of sync with active UI tests.
Line 67 makes new_pack_button the effective ID, but current test consumers still query packs_new_pack_button (including VisualScreenshotTests and PackSubFlowTests in this PR context). Update those selectors together to avoid immediate UI-test failures.
🤖 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/Features/Packs/PacksListView.swift` around lines
65 - 67, Remove the duplicate accessibilityIdentifier call on line 65 that sets
"packs_new_pack_button" in the toolbar button configuration, keeping only the
"new_pack_button" identifier on line 67 as the effective ID. Then update all UI
test files including VisualScreenshotTests and PackSubFlowTests to replace their
queries for "packs_new_pack_button" with "new_pack_button" to match the renamed
accessibility identifier in PacksListView.
| E2E_API_BASE_URL=http://localhost:8788 bun run e2e:swift:ios | ||
| ``` | ||
|
|
||
| The runner injects `E2E_EMAIL`, `E2E_PASSWORD`, `E2E_API_BASE_URL`, and `E2E_SCREENSHOT_DIR` into the generated Xcode schemes at runtime. It redacts credential-like values from `xcodebuild` output. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'E2E_API_BASE_URL|E2E_SCREENSHOT_DIR|environmentVariableXml|EnvironmentVariables|injectScheme' apps/swift/scripts/run-e2e.tsRepository: PackRat-AI/PackRat
Length of output: 1232
🏁 Script executed:
cat -n apps/swift/scripts/run-e2e.ts | head -250Repository: PackRat-AI/PackRat
Length of output: 10316
🏁 Script executed:
sed -n '250,350p' apps/swift/scripts/run-e2e.tsRepository: PackRat-AI/PackRat
Length of output: 2185
🏁 Script executed:
sed -n '85,100p' docs/ci/swift-e2e-runner.mdRepository: PackRat-AI/PackRat
Length of output: 836
Remove E2E_API_BASE_URL and E2E_SCREENSHOT_DIR from the scheme injection claim.
The injectScheme() function in apps/swift/scripts/run-e2e.ts only injects E2E_EMAIL and E2E_PASSWORD into the scheme's TestAction EnvironmentVariables. The variables E2E_API_BASE_URL and E2E_SCREENSHOT_DIR do not appear in the injection logic and are not added to the scheme. Update the documentation to reflect only what is actually injected, or clarify how these variables are passed to the tests if they are used.
🤖 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` at line 94, Update the documentation in
docs/ci/swift-e2e-runner.md line 94 to accurately reflect what variables the
injectScheme() function in apps/swift/scripts/run-e2e.ts actually injects into
the Xcode schemes. Remove E2E_API_BASE_URL and E2E_SCREENSHOT_DIR from the claim
about scheme injection since the injectScheme() function only injects E2E_EMAIL
and E2E_PASSWORD into TestAction EnvironmentVariables, or clarify how these
other variables are passed to the tests if they are used elsewhere in the
runner.
Comment on lines
+1
to
+43
| 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 |
Contributor
There was a problem hiding this comment.
Add soft-delete support for social content tables before shipping.
These tables model user-generated content, but there is no soft-delete column strategy, and FK cascades enforce hard-deletion chains. Please add deleted_at-based soft-delete support in the Drizzle schema and regenerate this migration rather than hand-editing SQL.
As per coding guidelines, “All user-generated content must use soft deletes (never hard DELETE).”
🧰 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 social
content tables (comment_likes, post_comments, post_likes, and posts) lack
soft-delete support and use hard-delete cascading constraints, violating the
coding guideline that all user-generated content must use soft deletes. Update
the Drizzle schema file (not this SQL migration) by adding a nullable deleted_at
timestamp column to each of the four tables mentioned above, and modify all
foreign key constraints in these tables to use onDelete: 'no action' instead of
cascade to prevent hard-deletion chains. After updating the schema, regenerate
this migration file from the updated schema definition rather than hand-editing
the SQL.
Source: Coding guidelines
Comment on lines
+9
to
+17
| 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 | ||
| ); |
Contributor
There was a problem hiding this comment.
Index post_comments foreign keys used on read paths.
post_comments.post_id is used in feed aggregation/filtering, but this migration creates no index for it (same for parent_comment_id for threaded lookups). Add indexes in schema and regenerate migration to avoid scan-heavy comment queries as volume grows.
As per coding guidelines, “Flag N+1 query patterns and missing database indexes on foreign keys or frequently filtered columns.”
Also applies to: 38-40
🤖 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 9 - 17, The
post_comments table migration is missing database indexes on frequently-used
foreign key columns. Add indexes for post_comments.post_id (used in feed
aggregation and filtering) and post_comments.parent_comment_id (used for
threaded lookups) to prevent table scans as data volume grows. Update the schema
definition to include these indexes and regenerate the migration file to reflect
these index creations in the SQL.
Source: Coding guidelines
Comment on lines
+182
to
+189
| it('defaults missing category to custom', async () => { | ||
| const newPack = { | ||
| id: `pack_test_no_category_${Date.now()}`, | ||
| name: 'Swift Created Pack', | ||
| isPublic: false, | ||
| localCreatedAt: new Date().toISOString(), | ||
| localUpdatedAt: new Date().toISOString(), | ||
| }; |
Contributor
There was a problem hiding this comment.
Make these new POST /packs tests deterministic and tighten the status contract assertion.
Lines 184-188 and 201-208 depend on wall-clock time (Date.now() / new Date()), and Lines 193/213 allow two status codes, which weakens regression detection.
🧪 Proposed test hardening
it('defaults missing category to custom', async () => {
+ const fixedTimestamp = '2026-01-01T00:00:00.000Z';
const newPack = {
- id: `pack_test_no_category_${Date.now()}`,
+ id: 'pack_test_no_category',
name: 'Swift Created Pack',
isPublic: false,
- localCreatedAt: new Date().toISOString(),
- localUpdatedAt: new Date().toISOString(),
+ localCreatedAt: fixedTimestamp,
+ localUpdatedAt: fixedTimestamp,
};
const res = await apiWithAuth('/packs', httpMethods.post(newPack));
- expect([200, 201]).toContain(res.status);
+ expect(res.status).toBe(200);
const data = await expectJsonResponse(res, ['id', 'category']);
expect(data.id).toBe(newPack.id);
expect(data.category).toBe('custom');
});
it('normalizes nullable native-client fields', async () => {
+ const fixedTimestamp = '2026-01-01T00:00:00.000Z';
const newPack = {
- id: `pack_test_null_category_${Date.now()}`,
+ id: 'pack_test_null_category',
name: 'Swift Nullable Pack',
description: null,
category: null,
tags: null,
isPublic: false,
- localCreatedAt: new Date().toISOString(),
- localUpdatedAt: new Date().toISOString(),
+ localCreatedAt: fixedTimestamp,
+ localUpdatedAt: fixedTimestamp,
};
const res = await apiWithAuth('/packs', httpMethods.post(newPack));
- expect([200, 201]).toContain(res.status);
+ expect(res.status).toBe(200);
const data = await expectJsonResponse(res, ['id', 'category']);
expect(data.id).toBe(newPack.id);
expect(data.description).toBeNull();As per coding guidelines, "Tests must be deterministic — mock all external services and clocks."
Also applies to: 193-193, 201-209, 213-213
🤖 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/test/packs.test.ts` around lines 182 - 189, The test 'defaults
missing category to custom' uses non-deterministic wall-clock time values
(Date.now() for the id and new Date().toISOString() for localCreatedAt and
localUpdatedAt), which violates the determinism requirement. Replace these
dynamic date/time values with fixed, mocked constants so the test produces
consistent results regardless of when it runs. Additionally, tighten the status
code assertions at lines 193 and 213 to expect a specific HTTP status code
(likely 200 or 201) instead of allowing multiple status codes, which will
improve regression detection and enforce a stricter contract. Apply the same
fixes to the related test at lines 201-209.
Source: Coding guidelines
Contributor
Deploying packrat-landing with
|
| Latest commit: |
5d9cd6d
|
| Status: | 🚫 Build failed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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:
- Around line 59-63: The `if` condition that gates the macOS job currently
excludes the `pull_request` event type, making the PR smoke path unreachable and
skipping macOS E2E coverage for pull requests. Add `github.event_name ==
'pull_request'` to the condition using an OR operator (`||`) alongside the
existing checks for `schedule`, `push`, and the `workflow_dispatch` with
`inputs.run_macos_ui` gate.
🪄 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: 06e44059-4f81-4eab-a1fc-3a9b7aeb3dbc
📒 Files selected for processing (4)
.github/workflows/swift-e2e.ymlpackages/api/src/routes/packs/index.tspackages/app/src/features/pack/create/queries.tspackages/cli/src/commands/packs/create.ts
Comment on lines
+59
to
+63
| if: > | ||
| github.event_name == 'schedule' || | ||
| github.event_name == 'push' || | ||
| (github.event_name == 'workflow_dispatch' && inputs.run_macos_ui) | ||
|
|
Contributor
There was a problem hiding this comment.
Include pull_request in the macOS job gate.
Line 62 excludes pull_request, so the PR smoke path at Lines 112-114 is unreachable and PR macOS E2E coverage is skipped.
Suggested fix
if: >
+ github.event_name == 'pull_request' ||
github.event_name == 'schedule' ||
github.event_name == 'push' ||
(github.event_name == 'workflow_dispatch' && inputs.run_macos_ui)🤖 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 59 - 63, The `if` condition
that gates the macOS job currently excludes the `pull_request` event type,
making the PR smoke path unreachable and skipping macOS E2E coverage for pull
requests. Add `github.event_name == 'pull_request'` to the condition using an OR
operator (`||`) alongside the existing checks for `schedule`, `push`, and the
`workflow_dispatch` with `inputs.run_macos_ui` gate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #2408 after #2376 was merged and GitHub closed the dependent PR.\n\nThis branch has been synced onto development after the macOS Swift app merge.\n\nValidation run locally:\n- bun test:swift:scripts\n- bun test:swift:runner\n- bun run check:package-json\n- bun run lint:custom\n- bun run check:casts:strict\n- pre-push clean-checks
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Tests
Documentation
Chores