fix(swift): resolve the iOS QA issue batch (#2708-#2717) - #2722
Merged
Conversation
The banner was only mounted inside splitLayout, which renders on iPad regular width and macOS. Every iPhone runs the compact branch, so no offline indicator ever appeared there. Hoist it into navigationBody via safeAreaInset so both layouts get it from one place and it cannot drift out of one again. Fixes #2714
ChatView applied keyboardDoneButton, whose ToolbarItemGroup(.keyboard) renders a full-width accessory bar pinned directly above the keyboard — exactly where the chat composer already sits. Its trailing Done button landed on top of the send button. Chat already has both dismissal paths the modifier exists to provide: dismissesKeyboardOnScroll on the message list, and send() clearing isInputFocused. So the accessory bar was pure overlap. Left the ~12 Form-based call sites alone; there the bar is correct and is the only way to dismiss a multi-line field that never fires onSubmit. Fixes #2713
The confirm action existed only as ToolbarItem(.primaryAction). On iPhone that competes for the navigation bar with the always-visible search drawer and gets collapsed, so after ticking items the selection bar offered only "N selected" and "Clear" — no way to finish. Put an Add (N) button in the selection bar itself, where the user is already looking, calling the same addSelected() path. Fixes #2715
Two problems produced the generic "Temporarily Unavailable" copy when scanning gear from a photo in airplane mode: - The scan sheet had no connectivity check, so it started a doomed upload and surfaced whatever transport error came back. - FriendlyErrorPresentation classified errors by sniffing English localizedDescription text. A real offline request usually throws .cannotFindHost or .dnsLookupFailed, whose descriptions match none of the connectivity keywords, so they fell through to the generic bucket. Add an .offline phase with a pre-check, and classify by URLError.Code instead of by message text. The typed check also fixes the latent locale bug: on a non-English device every network failure in the app read as "Temporarily Unavailable". Also drop the ^[...](inflect: true) markup from the add-items toast and the scan header. That markup only resolves through a localization catalog and this target ships no .xcstrings, so it rendered verbatim as "Added ^[3 item](inflect: true) from the catalog". Fixes #2717 Fixes #2716
Home displays Packs/Trips/Items counts but had no loader of its own — it read arrays that only PacksListView and TripsListView populate from their .task. TabView builds tabs lazily, so a fresh sign-in landed on Home with empty arrays and rendered zeros until the user visited another tab, which is exactly the reported workaround. Give Home its own .task, loading only when empty so returning from another tab doesn't refetch what that tab just loaded. Also fix two related state bugs: - loadMore() appended pages without deduping while load() could reassign the array concurrently, so the same pack or trip could appear twice. Duplicate ids collide in ForEach identity, which is why the duplication was visible rather than harmless. - CachedPack/CachedTrip carry no user column and survived sign-out, so the next user to sign in on the device saw the previous user's packs and trips flash up from cache before the network replaced them. Purge both on signOut, and add reset() to the view models. Fixes #2708
The setting was write-only. AppPreferences wrapped @AppStorage in an ObservableObject, which does not publish objectWillChange, and nothing outside PreferencesView ever read the key. Weights were formatted by four separate hard-coded g/kg helpers that took no unit at all, so switching g -> lb changed nothing and converted nothing. Put the conversion on AppWeightUnit (gramsPerUnit + display) as a single source of truth, mirroring how SpeedUnit/TemperatureUnit already work, and collapse the duplicated formatters into it. Propagate it with an EnvironmentValues.weightUnit injected once at the app root, rather than ~20 separate @AppStorage declarations that are easy to forget — forgetting one is the bug being fixed. Spelled as an explicit EnvironmentKey because @entry needs iOS 18 and this target deploys to 17. Pack totals keep their existing "%.2f kg" output, so the assertions in ModelTests are unchanged. Three compact labels in GearInventory and the chat tool result previously emitted "1.5kg"/"800g"; they now match the rest of the app. Fixes #2711
The system prompt framed PackRat AI as an assistant "for hikers" whose job is "hiking packs ... using ultralight principles", and instructed it to suggest multi-purpose items to reduce pack weight unconditionally. Asking "any packing tips for a 3-day tour?" therefore returned tent, sleeping bag, sleeping pad and ultralight advice, even though the app supports city travel, beach, water sports, skiing and more. Treat trip type as an input rather than a default, keep the ultralight expertise for when the trip really is a carry-everything activity, and allow one brief clarifying question when the context is ambiguous. The Schema Info block, Context block and contextType/location appends are untouched. Fixes #2709
Asking the assistant to act on "my Japan Trip pack" returned "I couldn't find a pack named 'Japan Trip'" for packs that plainly exist. There was no tool to list or resolve a pack by name at all — getPackDetails requires an id and is client-executed — so that sentence was the model narrating a failure it had no way to avoid. Add a read-only listUserPacks tool scoped to the signed-in user and excluding soft-deleted rows, with the query in a service alongside executeSqlAiTool. Explicit column projection; capped at 50 rows. Also fix a latent bug in the executeSql guard: it rejected any query whose text merely contained a forbidden keyword as a substring. Because packs carries a deleted column, the correct soft-delete-aware query was always rejected as a mutation. created_at and updated_at hit the same trap via 'create' and 'update'. Match on word boundaries instead, which still rejects the real statements across whitespace, newlines and casing — covered by 14 new tests. Note this closes only the lookup half of #2710. The assistant still has no mutation tool, so it cannot add an item to a pack. Refs #2710
answerClientTool ignored the tool call's own arguments and returned the
conversation's scoped pack. In a general chat there is no scoped pack,
so context.toolPayload was nil and every getPackDetails call — whatever
id it carried — came back {"success": false, "error": "Pack not found"}.
That is the signal behind the assistant insisting an existing pack
could not be found.
Fall back to resolving the requested packId against the local store.
Injected as a closure from AppState so ChatViewModel stays decoupled
from PacksViewModel.
Refs #2710
Contributor
Coverage Report for packages/utils (./packages/utils)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/units (./packages/units)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/overpass (./packages/overpass)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/analytics (./packages/analytics)
File CoverageNo changed files found. |
Contributor
WalkthroughThe PR adds persisted weight-unit formatting across SwiftUI views, offline scan and cache handling, local pack tools for chat, broader trip guidance, SQL read-only validation, and App Store Connect API-key upload support. ChangesWeight preference propagation
Offline and cached state
Pack catalog interactions
Pack-aware chat tools
Read-only SQL validation
TestFlight authentication
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
Coverage Report for apps/expo (./apps/expo)
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. |
![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: 5
🤖 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 `@apps/swift/Sources/PackRat/Features/Chat/ChatViewModel.swift`:
- Around line 161-165: Update the payload selection in the getPackDetails
handling around requestedPackPayload: parse the requested packId first, use
context.toolPayload only when its packId matches context.packId, and otherwise
resolve the requested ID through resolvePack so responses never return scoped
data for a different pack.
In `@apps/swift/Sources/PackRat/Models/Pack.swift`:
- Around line 145-148: Update Pack.init(from:) to stop defaulting unknown API
weight units to .g; reject unsupported values during decoding or preserve the
raw unit so conversion and totals never present them as grams. Match the
unrecognized-unit behavior used by SeasonSuggestionItem.displayWeight(in:) while
retaining normal decoding for supported units.
In `@apps/swift/Sources/PackRat/Network/AuthManager.swift`:
- Around line 303-305: The sign-out flow around
AuthManager.purgeCachedUserContent must also clear the in-memory packs and trips
held by AppState’s stable PacksViewModel and TripsViewModel instances.
Coordinate the reset through the session root on the main actor if AuthManager
cannot access AppState directly, ensure it runs before the next session renders,
and add a regression test covering sign-out followed by sign-in with a different
account.
In `@packages/api/src/services/executeSqlAiTool.ts`:
- Around line 9-28: Update isReadOnlyQuery to use a SQL-aware parser rather than
FORBIDDEN_KEYWORD_PATTERNS alone: require exactly one read-only SELECT
statement, reject SELECT ... INTO, locking clauses, and multiple statements,
while allowing forbidden words inside string literals and quoted identifiers.
Add regression tests covering these accepted and rejected cases before sql.raw
execution, while retaining the read-only database role.
In `@packages/api/src/utils/ai/tools.ts`:
- Around line 27-37: Update the catch block in the execute handler for
listUserPacksAiTool to call captureApiException({ error, operation, extra })
before returning the failure response. Provide an operation name identifying
list-user-packs and only non-sensitive context in extra, while preserving the
existing error response behavior.
🪄 Autofix
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 Plus
Run ID: fbf06c96-549d-4628-9d89-03bdf41f202b
📒 Files selected for processing (37)
apps/swift/Sources/PackRat/AppState.swiftapps/swift/Sources/PackRat/Features/Catalog/CatalogItemDetailView.swiftapps/swift/Sources/PackRat/Features/Catalog/CatalogView.swiftapps/swift/Sources/PackRat/Features/Chat/ChatView.swiftapps/swift/Sources/PackRat/Features/Chat/ChatViewModel.swiftapps/swift/Sources/PackRat/Features/Chat/ToolResultView.swiftapps/swift/Sources/PackRat/Features/GearInventory/GearInventoryView.swiftapps/swift/Sources/PackRat/Features/Home/HomeView.swiftapps/swift/Sources/PackRat/Features/PackTemplates/PackTemplatesView.swiftapps/swift/Sources/PackRat/Features/Packs/PackCatalogBrowserSheet.swiftapps/swift/Sources/PackRat/Features/Packs/PackDetailView.swiftapps/swift/Sources/PackRat/Features/Packs/PackItemDetailView.swiftapps/swift/Sources/PackRat/Features/Packs/PackItemRow.swiftapps/swift/Sources/PackRat/Features/Packs/PackItemsScanSheet.swiftapps/swift/Sources/PackRat/Features/Packs/PackWeightAnalysisView.swiftapps/swift/Sources/PackRat/Features/Packs/PackWeightChart.swiftapps/swift/Sources/PackRat/Features/Packs/PacksListView.swiftapps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swiftapps/swift/Sources/PackRat/Features/Packs/RecentPacksView.swiftapps/swift/Sources/PackRat/Features/SeasonSuggestions/SeasonSuggestionsView.swiftapps/swift/Sources/PackRat/Features/Trips/TripDetailView.swiftapps/swift/Sources/PackRat/Features/Trips/TripFormView.swiftapps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swiftapps/swift/Sources/PackRat/Models/Catalog.swiftapps/swift/Sources/PackRat/Models/Pack.swiftapps/swift/Sources/PackRat/Models/PackTemplate.swiftapps/swift/Sources/PackRat/Models/SeasonSuggestions.swiftapps/swift/Sources/PackRat/Navigation/AppNavigation.swiftapps/swift/Sources/PackRat/Network/AuthManager.swiftapps/swift/Sources/PackRat/PackRatApp.swiftapps/swift/Sources/PackRat/Shared/ErrorView.swiftapps/swift/Sources/PackRat/Shared/WeightUnitEnvironment.swiftpackages/api/src/routes/chat.tspackages/api/src/services/executeSqlAiTool.tspackages/api/src/services/listUserPacksAiTool.tspackages/api/src/utils/ai/tools.tspackages/api/test/executeSqlAiTool.test.ts
Comment on lines
+161
to
+165
| // Prefer the scoped context, then fall back to looking up whichever pack | ||
| // the model actually asked for. Ignoring the requested id is what made a | ||
| // general chat insist that an existing pack could not be found. | ||
| let payload = context.toolPayload ?? requestedPackPayload(for: invocation) | ||
| let output: [String: Any] = if let payload { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the scoped payload to the requested packId.
Line 164 returns context.toolPayload for every getPackDetails call in a pack-scoped chat. If the model requests another pack, the client returns the scoped pack data under the other pack ID.
Parse the requested packId before selecting a payload. Use context.toolPayload only when it equals context.packId. Otherwise, resolve the requested ID with resolvePack.
🤖 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/Chat/ChatViewModel.swift` around lines
161 - 165, Update the payload selection in the getPackDetails handling around
requestedPackPayload: parse the requested packId first, use context.toolPayload
only when its packId matches context.packId, and otherwise resolve the requested
ID through resolvePack so responses never return scoped data for a different
pack.
Comment on lines
+145
to
+148
| init(from decoder: any Decoder) throws { | ||
| let raw = try decoder.singleValueContainer().decode(String.self) | ||
| self = Self(apiValue: raw) ?? .g | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not convert an unknown API unit to grams.
Line 147 maps every unsupported weightUnit to .g. The new conversion path then displays an unknown source value as grams and includes that false value in calculated totals. For example, an unsupported "stone" unit becomes "g".
Reject the payload or preserve the raw unit for unconverted display. SeasonSuggestionItem.displayWeight(in:) already preserves unrecognized units instead of claiming a conversion.
Proposed safe decoding change
init(from decoder: any Decoder) throws {
- let raw = try decoder.singleValueContainer().decode(String.self)
- self = Self(apiValue: raw) ?? .g
+ let container = try decoder.singleValueContainer()
+ let raw = try container.decode(String.self)
+ guard let unit = Self(apiValue: raw) else {
+ throw DecodingError.dataCorruptedError(
+ in: container,
+ debugDescription: "Unsupported weight unit: \(raw)"
+ )
+ }
+ self = unit
}📝 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.
Suggested change
| init(from decoder: any Decoder) throws { | |
| let raw = try decoder.singleValueContainer().decode(String.self) | |
| self = Self(apiValue: raw) ?? .g | |
| } | |
| init(from decoder: any Decoder) throws { | |
| let container = try decoder.singleValueContainer() | |
| let raw = try container.decode(String.self) | |
| guard let unit = Self(apiValue: raw) else { | |
| throw DecodingError.dataCorruptedError( | |
| in: container, | |
| debugDescription: "Unsupported weight unit: \(raw)" | |
| ) | |
| } | |
| self = unit | |
| } |
🤖 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/Models/Pack.swift` around lines 145 - 148, Update
Pack.init(from:) to stop defaulting unknown API weight units to .g; reject
unsupported values during decoding or preserve the raw unit so conversion and
totals never present them as grams. Match the unrecognized-unit behavior used by
SeasonSuggestionItem.displayWeight(in:) while retaining normal decoding for
supported units.
Comment on lines
+303
to
+305
| // `signOut` is reachable from non-main contexts (see `MainActor.run` | ||
| // callers), while the SwiftData container is main-actor isolated. | ||
| Task { @MainActor in Self.purgeCachedUserContent() } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Reset the in-memory pack and trip state during sign-out.
purgeCachedUserContent() only removes SwiftData records. AppState keeps stable PacksViewModel and TripsViewModel instances, so their arrays still contain the previous user's content after sign-out. HomeView.loadSummaryData() then skips loading because both arrays are non-empty.
Reset both view models on the main actor before a new session can render. Coordinate this through the session root if AuthManager cannot access AppState directly. Add a sign-out/sign-in regression test for this account-switch flow.
🤖 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/Network/AuthManager.swift` around lines 303 - 305,
The sign-out flow around AuthManager.purgeCachedUserContent must also clear the
in-memory packs and trips held by AppState’s stable PacksViewModel and
TripsViewModel instances. Coordinate the reset through the session root on the
main actor if AuthManager cannot access AppState directly, ensure it runs before
the next session renders, and add a regression test covering sign-out followed
by sign-in with a different account.
Comment on lines
+9
to
+28
| // Mutating keywords rejected by isReadOnlyQuery. These are matched on WORD | ||
| // BOUNDARIES, not as bare substrings: a plain `includes('delete')` also | ||
| // matches the `deleted` column that every soft-deleted table carries, so the | ||
| // correct `SELECT ... WHERE deleted = false` was always rejected as a | ||
| // mutation. `\b` still catches the real statements regardless of surrounding | ||
| // whitespace, newlines, parens or casing (the input is lowercased first). | ||
| const FORBIDDEN_KEYWORD_PATTERNS = Object.freeze([ | ||
| /\binsert\b/, | ||
| /\bupdate\b/, | ||
| /\bdelete\b/, | ||
| /\bdrop\b/, | ||
| /\bcreate\b/, | ||
| /\balter\b/, | ||
| /\btruncate\b/, | ||
| /\bgrant\b/, | ||
| /\brevoke\b/, | ||
| /\bcommit\b/, | ||
| /\brollback\b/, | ||
| ] as const); | ||
|
|
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="$(fd -t f 'executeSqlAiTool\.ts$' . | head -n 1)"
printf '%s\n' "FILE=$file"
wc -l "$file"
ast-grep outline "$file" --view expanded
printf '%s\n' '--- relevant source ---'
sed -n '1,180p' "$file"
printf '%s\n' '--- related tests and usages ---'
rg -n --glob '!node_modules' 'isReadOnlyQuery|FORBIDDEN_KEYWORD_PATTERNS|executeSqlAiTool|sql\.raw' .Repository: PackRat-AI/PackRat
Length of output: 13685
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from pathlib import Path
paths = list(Path('.').rglob('executeSqlAiTool.ts'))
if not paths:
raise SystemExit('executeSqlAiTool.ts not found')
path = paths[0]
text = path.read_text()
print(f'FILE={path}')
for name in ('isReadOnlyQuery', 'FORBIDDEN_KEYWORD_PATTERNS'):
for match in re.finditer(rf'\b{name}\b', text):
start = max(0, text.rfind('\n', 0, match.start()-1) + 1)
end = text.find('\n', match.end())
print(f'{name}: {text.count(chr(10), 0, start)+1}: {text[start:end if end != -1 else len(text)]}')
print('--- validator context ---')
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if 'isReadOnlyQuery' in line or 'FORBIDDEN_KEYWORD_PATTERNS' in line or 'sql.raw' in line:
lo, hi = max(1, i-8), min(len(lines), i+12)
print(f'[{lo}-{hi}]')
for j in range(lo, hi+1):
print(f'{j}: {lines[j-1]}')
PYRepository: PackRat-AI/PackRat
Length of output: 6896
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- executeSqlAiTool tests ---'
sed -n '1,230p' packages/api/test/executeSqlAiTool.test.ts
printf '%s\n' '--- read-only database construction ---'
rg -n -A35 -B10 'function createReadOnlyDb|const createReadOnlyDb|export .*createReadOnlyDb|createReadOnlyDb' packages/api/src packages/api/test
printf '%s\n' '--- SQL parser dependencies and existing validation helpers ---'
rg -n --glob 'package.json' --glob 'bun.lock*' --glob 'pnpm-lock.yaml' --glob 'yarn.lock' --glob 'package-lock.json' 'parser|pgsql|postgres.*parse|sql-ast|read.?only'Repository: PackRat-AI/PackRat
Length of output: 35812
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
const patterns = [
/\binsert\b/, /\bupdate\b/, /\bdelete\b/, /\bdrop\b/, /\bcreate\b/,
/\balter\b/, /\btruncate\b/, /\bgrant\b/, /\brevoke\b/, /\bcommit\b/,
/\brollback\b/,
];
function isReadOnlyQuery(query) {
const normalizedQuery = query.trim().toLowerCase();
if (!normalizedQuery.startsWith('select')) return false;
return !patterns.some((pattern) => pattern.test(normalizedQuery));
}
const cases = [
"SELECT id INTO archive FROM packs",
"SELECT 1 LIMIT 1; CALL dangerous_proc()",
"SELECT 'delete'",
'SELECT "delete" FROM packs',
'SELECT id FROM packs WHERE deleted = false',
'SELECT 1; DROP TABLE packs',
'SELECT update_count FROM packs',
];
for (const query of cases) {
console.log(JSON.stringify({ query, accepted: isReadOnlyQuery(query) }));
}
JSRepository: PackRat-AI/PackRat
Length of output: 575
Use SQL-aware validation before executing sql.raw.
isReadOnlyQuery accepts SELECT ... INTO and multiple statements such as SELECT 1 LIMIT 1; CALL dangerous_proc(). It also rejects valid queries such as SELECT 'delete' and quoted identifiers. Parse exactly one read-only SELECT, reject INTO and locking clauses, and add regression tests. The read-only database role remains defense in depth.
🤖 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/services/executeSqlAiTool.ts` around lines 9 - 28, Update
isReadOnlyQuery to use a SQL-aware parser rather than FORBIDDEN_KEYWORD_PATTERNS
alone: require exactly one read-only SELECT statement, reject SELECT ... INTO,
locking clauses, and multiple statements, while allowing forbidden words inside
string literals and quoted identifiers. Add regression tests covering these
accepted and rejected cases before sql.raw execution, while retaining the
read-only database role.
Comment on lines
+27
to
+37
| execute: async ({ nameQuery }) => { | ||
| try { | ||
| const data = await listUserPacksAiTool({ userId, nameQuery }); | ||
| return { success: true, data }; | ||
| } catch (error) { | ||
| console.error('listUserPacks tool error', error); | ||
| return { | ||
| success: false, | ||
| error: error instanceof Error ? error.message : 'Failed to list packs', | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Capture the swallowed service error.
The catch returns a failure result but only writes to console.error. Send the error to captureApiException with an operation name and non-sensitive context before returning the tool response.
Proposed fix
+import { captureApiException } from '`@packrat/api/utils/sentry`';
+
} catch (error) {
- console.error('listUserPacks tool error', error);
+ captureApiException({
+ error,
+ operation: 'aiTool.listUserPacks',
+ userId,
+ extra: { hasNameQuery: nameQuery !== undefined },
+ });
return {As per coding guidelines: “For catches that swallow errors, call captureApiException({ error, operation, extra }).”
📝 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.
Suggested change
| execute: async ({ nameQuery }) => { | |
| try { | |
| const data = await listUserPacksAiTool({ userId, nameQuery }); | |
| return { success: true, data }; | |
| } catch (error) { | |
| console.error('listUserPacks tool error', error); | |
| return { | |
| success: false, | |
| error: error instanceof Error ? error.message : 'Failed to list packs', | |
| }; | |
| } | |
| import { captureApiException } from '@packrat/api/utils/sentry'; | |
| execute: async ({ nameQuery }) => { | |
| try { | |
| const data = await listUserPacksAiTool({ userId, nameQuery }); | |
| return { success: true, data }; | |
| } catch (error) { | |
| captureApiException({ | |
| error, | |
| operation: 'aiTool.listUserPacks', | |
| userId, | |
| extra: { hasNameQuery: nameQuery !== undefined }, | |
| }); | |
| return { | |
| success: false, | |
| error: error instanceof Error ? error.message : 'Failed to list packs', | |
| }; | |
| } |
🤖 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/utils/ai/tools.ts` around lines 27 - 37, Update the catch
block in the execute handler for listUserPacksAiTool to call
captureApiException({ error, operation, extra }) before returning the failure
response. Provide an operation name identifying list-user-packs and only
non-sensitive context in extra, while preserving the existing error response
behavior.
Source: Coding guidelines
The new tool had no test, dropping packages/api function coverage from 100% to 99% and failing the coverage ratchet. Renders the Drizzle condition to real SQL and asserts on the text and bound params, so the two load-bearing predicates are pinned by name: the user scope (without it the tool leaks other users' packs) and the soft-delete filter (without it deleted packs come back). Substring matching on a stringified condition could not express either. Note the unit config only collects src/**/__tests__/**, so a test placed in packages/api/test/ would run in the integration suite and not count toward the ratchet at all.
`vi.fn(() => …)` infers a zero-argument signature, so `mock.calls` is typed as the empty tuple and every `calls[0]?.[0]` read was a TS2493. Root tsc caught it; the vitest run did not. Declare the captured parameter on each builder mock so the calls are real tuples, which also drops an `as Record<string, unknown>` cast.
CI's Xcode rejected it: reading the main-actor `appState` inside the implicitly-async `async let` closures needs an explicit `await`, so both branches failed with "expression is 'async' but is not marked with 'await'". My local toolchain accepted it, which is why this only showed up in the macOS and iOS smoke builds. The concurrency bought nothing anyway — both view models are main-actor isolated, so the two loads could never overlap. Sequential is correct and simpler. Verified with clean-DerivedData `build-for-testing` runs against both the macOS-Smoke and iOS-Smoke test plans.
Adds App Store Connect API key auth (`--apiKey`/`--apiIssuer`) as an alternative to APPLE_ID + APPLE_APP_PASSWORD, so an upload needs no interactive Apple account and no app-specific password. The two forms are mutually exclusive in altool, so only one set is passed. nodeEnv is an explicit allowlist, so the new APPLE_ASC_API_KEY_ID and APPLE_ASC_API_ISSUER_ID keys are declared in the schema and forwarded from process.env. MARKETING_VERSION was documented in the script header and read via nodeEnv but never declared, so it was silently always undefined — added alongside. Also pass the resolved team id into the dry-run preflight, which previously always printed DEVELOPMENT_TEAM=<APPLE_TEAM_ID> and so hid exactly the misconfiguration a dry run exists to catch.
mikib0
force-pushed
the
fix/swift-ios-qa-batch
branch
from
August 12, 2026 13:32
df32850 to
3486335
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@apps/swift/scripts/upload-testflight.ts`:
- Around line 185-191: Update the authentication setup around ascApiKeyId,
ascApiIssuer, and usesApiKey to reject configurations where only one API-key
variable is set, while preserving VERIFY_ARCHIVE_ONLY as an allowed exception.
Ensure Apple ID credentials are requested only when archive verification is
disabled and both API-key variables are absent.
- Around line 185-187: Add optional APPLE_ASC_API_KEY_ID and
APPLE_ASC_API_ISSUER_ID fields to nodeEnvSchema and include both in its parser
mapping, using APPLE_ASC_API_ISSUER_ID consistently. Ensure the existing
usesApiKey logic in the upload flow reads the newly parsed shared environment
values.
In `@apps/swift/Sources/PackRat/Features/Home/HomeView.swift`:
- Around line 53-61: Update loadSummaryData to start the independent
packsVM.load and tripsVM.load operations concurrently using structured
concurrency, while checking appState caches and capturing modelContext on the
main actor before creating the concurrent tasks. Preserve the existing
empty-cache guards and await both loads without introducing unstructured tasks.
- Around line 56-62: The loadSummaryData flow should stop using empty
packs/trips collections as a loading guard. Add per-view-model loaded/retry
state and single-flight coordination to PacksViewModel and TripsViewModel so
concurrent load(context:) calls share one in-flight request, then update
AuthManager.signOut() to invoke each view model’s reset method and clear
in-memory, cached, and in-flight state for the next user.
🪄 Autofix
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 Plus
Run ID: 6b328efc-b549-4d7b-bd7f-e1ca73750642
📒 Files selected for processing (3)
apps/swift/Sources/PackRat/Features/Home/HomeView.swiftapps/swift/scripts/upload-testflight.tspackages/api/src/services/__tests__/listUserPacksAiTool.test.ts
Comment on lines
+185
to
+191
| const ascApiKeyId = nodeEnv.APPLE_ASC_API_KEY_ID; | ||
| const ascApiIssuer = nodeEnv.APPLE_ASC_API_ISSUER_ID; | ||
| const usesApiKey = Boolean(ascApiKeyId && ascApiIssuer); | ||
|
|
||
| const appleId = VERIFY_ARCHIVE_ONLY || usesApiKey ? undefined : req({ name: 'APPLE_ID' }); | ||
| const appPassword = | ||
| VERIFY_ARCHIVE_ONLY || usesApiKey ? undefined : req({ name: 'APPLE_APP_PASSWORD' }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject incomplete API-key configuration.
When exactly one API-key variable is set, usesApiKey is false. The script then requests APPLE_ID and APPLE_APP_PASSWORD, and may use Apple ID authentication unexpectedly. Treat the API-key variables as an all-or-none pair. Preserve the VERIFY_ARCHIVE_ONLY exception.
Proposed validation
const ascApiKeyId = nodeEnv.APPLE_ASC_API_KEY_ID;
const ascApiIssuer = nodeEnv.APPLE_ASC_ISSUER_ID;
-const usesApiKey = Boolean(ascApiKeyId && ascApiIssuer);
+const hasApiKeyId = Boolean(ascApiKeyId);
+const hasApiIssuer = Boolean(ascApiIssuer);
+if (!VERIFY_ARCHIVE_ONLY && hasApiKeyId !== hasApiIssuer) {
+ console.error(
+ 'APPLE_ASC_API_KEY_ID and APPLE_ASC_ISSUER_ID must be set together.',
+ );
+ process.exit(1);
+}
+const usesApiKey = hasApiKeyId && hasApiIssuer;🤖 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/upload-testflight.ts` around lines 185 - 191, Update the
authentication setup around ascApiKeyId, ascApiIssuer, and usesApiKey to reject
configurations where only one API-key variable is set, while preserving
VERIFY_ARCHIVE_ONLY as an allowed exception. Ensure Apple ID credentials are
requested only when archive verification is disabled and both API-key variables
are absent.
Comment on lines
+53
to
+61
| /// Sequential rather than `async let`: both view models are main-actor | ||
| /// isolated, so the two loads cannot overlap anyway, and reading `appState` | ||
| /// from inside an implicitly-async closure needs an extra actor hop. | ||
| private func loadSummaryData() async { | ||
| if appState.packsVM.packs.isEmpty { | ||
| await appState.packsVM.load(context: modelContext) | ||
| } | ||
| if appState.tripsVM.trips.isEmpty { | ||
| await appState.tripsVM.load(context: modelContext) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'`@MainActor`|ModelContext|func load\(context:|service\.listPacks|service\.listTrips' \
apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift \
apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swiftRepository: PackRat-AI/PackRat
Length of output: 32404
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- HomeView ---'
cat -n apps/swift/Sources/PackRat/Features/Home/HomeView.swift | sed -n '1,90p'
printf '%s\n' '--- PacksViewModel.load ---'
cat -n apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift | sed -n '35,100p'
printf '%s\n' '--- TripsViewModel.load ---'
cat -n apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swift | sed -n '45,105p'
printf '%s\n' '--- ModelContext and loadSummaryData call sites ---'
rg -n -C 4 'loadSummaryData|modelContext|ModelContainer|ModelContext' \
apps/swift/Sources/PackRat/Features/Home \
apps/swift/Sources/PackRat/Sources \
apps/swift/Sources/PackRat 2>/dev/null | head -240Repository: PackRat-AI/PackRat
Length of output: 33373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AppState and service isolation ---'
rg -n -C 5 \
'`@MainActor`|final class AppState|struct AppState|class .*Service|actor .*Service|func listPacks|func listTrips' \
apps/swift/Sources/PackRat \
-g '*.swift' | head -320
printf '%s\n' '--- Swift concurrency configuration ---'
rg -n -C 3 \
'SWIFT_VERSION|SWIFT_STRICT_CONCURRENCY|SWIFT_DEFAULT_ACTOR_ISOLATION|swift-tools-version|StrictConcurrency' \
apps/swift -g 'project.pbxproj' -g 'Package.swift' -g '*.xcconfig' -g '*.yml' -g '*.yaml' | head -160
printf '%s\n' '--- Load implementations and state guards ---'
sed -n '1,125p' apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift
sed -n '1,120p' apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swiftRepository: PackRat-AI/PackRat
Length of output: 39761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AppState declaration ---'
rg -l 'class AppState|struct AppState|`@Observable`.*AppState' apps/swift/Sources/PackRat -g '*.swift' |
xargs -r -n1 sh -c 'echo "--- $0"; cat -n "$0" | sed -n "1,180p"'
printf '%s\n' '--- Pack and trip services ---'
rg -n -C 8 \
'final class PackService|func listPacks|final class TripService|func listTrips|final class APIClient|func send' \
apps/swift/Sources/PackRat/Services \
apps/swift/Sources/PackRat/Network -g '*.swift' | head -240
printf '%s\n' '--- Read-only source invariant check ---'
python3 - <<'PY'
from pathlib import Path
home = Path("apps/swift/Sources/PackRat/Features/Home/HomeView.swift").read_text()
packs = Path("apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift").read_text()
trips = Path("apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swift").read_text()
assert "`@MainActor`" in packs and "`@MainActor`" in trips
assert "try await service.listPacks" in packs
assert "try await service.listTrips" in trips
assert home.index("await appState.packsVM.load") < home.index("await appState.tripsVM.load")
print("Both view models are `@MainActor-isolated` and each load awaits an independent service request.")
print("HomeView invokes the pack load before the trip load.")
PYRepository: PackRat-AI/PackRat
Length of output: 10420
Run the pack and trip loads concurrently.
@MainActor serializes cache and state access, but each load releases the actor while awaiting its independent network request. The current order delays the trip request until the pack request completes. Use structured concurrency and keep modelContext access on the main actor.
🤖 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/Home/HomeView.swift` around lines 53 -
61, Update loadSummaryData to start the independent packsVM.load and
tripsVM.load operations concurrently using structured concurrency, while
checking appState caches and capturing modelContext on the main actor before
creating the concurrent tasks. Preserve the existing empty-cache guards and
await both loads without introducing unstructured tasks.
Comment on lines
+56
to
+62
| private func loadSummaryData() async { | ||
| if appState.packsVM.packs.isEmpty { | ||
| await appState.packsVM.load(context: modelContext) | ||
| } | ||
| if appState.tripsVM.trips.isEmpty { | ||
| await appState.tripsVM.load(context: modelContext) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'loadSummaryData|\.load\(context:|isLoading|isCacheLoaded|writeCachePacks|writeCacheTrips' \
apps/swift/Sources/PackRatRepository: PackRat-AI/PackRat
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate files ---'
fd -i -t f 'PacksViewModel|TripsViewModel|HomeView|AuthManager' apps/swift/Sources/PackRat
echo '--- view-model declarations and load/reset symbols ---'
rg -n -C 12 \
'class PacksViewModel|class TripsViewModel|func load\(|isCacheLoaded|writeCachePacks|writeCacheTrips|func signOut|packsVM|tripsVM' \
apps/swift/Sources/PackRat/Features \
apps/swift/Sources/PackRat/Shared \
apps/swift/Sources/PackRat | head -n 500Repository: PackRat-AI/PackRat
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift \
apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swift \
apps/swift/Sources/PackRat/Network/AuthManager.swift \
apps/swift/Sources/PackRat/Features/Home/HomeView.swift
do
echo "--- $f ($(wc -l < "$f") lines) ---"
case "$f" in
*PacksViewModel.swift) cat -n "$f" ;;
*TripsViewModel.swift) cat -n "$f" ;;
*AuthManager.swift) sed -n '280,345p' "$f" | cat -n -v ;;
*HomeView.swift) sed -n '1,70p' "$f" | cat -n -v ;;
esac
done
echo '--- exact load call sites ---'
rg -n -C 3 'packsVM\.load\(context:|tripsVM\.load\(context:' apps/swift/Sources/PackRatRepository: PackRat-AI/PackRat
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- direct packs/trips load callers ---'
rg -n -C 2 \
'packsVM\.load\(context:|tripsVM\.load\(context:|packsVM\.reset\(context:|tripsVM\.reset\(context:' \
apps/swift/Sources/PackRat
echo '--- all reset references ---'
rg -n -C 2 \
'\.reset\(context:|func reset\(context:' \
apps/swift/Sources/PackRat
echo '--- structural verifier ---'
python3 - <<'PY'
from pathlib import Path
packs = Path("apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift").read_text()
trips = Path("apps/swift/Sources/PackRat/Features/Trips/TripsViewModel.swift").read_text()
auth = Path("apps/swift/Sources/PackRat/Network/AuthManager.swift").read_text()
home = Path("apps/swift/Sources/PackRat/Features/Home/HomeView.swift").read_text()
def section(source, start, end):
return source[source.index(start):source.index(end, source.index(start))]
packs_load = section(packs, "func load(context:", " func loadMore")
trips_load = section(trips, "func load(context:", " func loadMore")
signout = section(auth, "func signOut()", " /// Drops")
summary = section(home, "private func loadSummaryData()", " private var compactBody")
for name, text in (("PacksViewModel.load", packs_load), ("TripsViewModel.load", trips_load)):
print(f"{name}: service page-1 calls = {text.count('page: 1')}")
print(f"{name}: has in-flight guard = {('guard !isLoading' in text) or ('isLoading else' in text)}")
print(f"{name}: assigns collection after await = {('packs = fresh' in text) or ('trips = fresh' in text)}")
print(f"{name}: writes cache after response = {('writeCachePacks' in text) or ('writeCacheTrips' in text)}")
print(f"AuthManager.signOut calls packs reset = {'packsVM.reset' in signout}")
print(f"AuthManager.signOut calls trips reset = {'tripsVM.reset' in signout}")
print(f"Home summary gates on packs.isEmpty = 'packs.isEmpty' in summary")
print(f"Home summary gates on trips.isEmpty = 'trips.isEmpty' in summary")
PYRepository: PackRat-AI/PackRat
Length of output: 4586
Make pack and trip loading single-flight and reset it on sign-out.
- Multiple views can call the same
load(context:)while a request is in flight. Add per-view-model single-flight coordination and explicit loaded/retry state instead of using collection emptiness. AuthManager.signOut()does not callPacksViewModel.resetorTripsViewModel.reset. Reset in-memory, cache, and in-flight state on sign-out so the next user cannot reuse the previous user's data.
🤖 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/Home/HomeView.swift` around lines 56 -
62, The loadSummaryData flow should stop using empty packs/trips collections as
a loading guard. Add per-view-model loaded/retry state and single-flight
coordination to PacksViewModel and TripsViewModel so concurrent load(context:)
calls share one in-flight request, then update AuthManager.signOut() to invoke
each view model’s reset method and clear in-memory, cached, and in-flight state
for the next user.
Closes the write half of #2710. The assistant could find a pack by name but had no tool to act on it, so "add a T-shirt to my Japan Trip pack" ended with it describing the steps instead of doing them. Adds an addItemToPack tool and moves every tool that touches the user's own packs to the client: listUserPacks, getPackDetails, getPackItemDetails and addItemToPack are now declared server-side with no `execute` and answered from the device's local store. That placement is the point, not a detail. The local store is what the user is looking at and it is the write path — mutations land there first and sync outward through the outbox. A server-side addItemToPack would write Postgres behind the UI, so the item would stay invisible until the next refresh and nothing would work offline. Reads move for the same reason: a pack created offline is now findable, and the names the model matches against are the ones on screen. Writes reuse PacksViewModel.addItem, so an item the assistant adds is indistinguishable from one added by tapping through the UI — optimistic insert, write-through when online, outbox mutation when not. Removes listUserPacksAiTool and its test, which are now dead. Tools over shared or external data (weather, catalog, guides, web search) stay server-side, where the API keys are. Prompt guidance added: resolve names before acting, never guess a pack id, look up a catalog weight so pack totals stay meaningful, and confirm or report failure honestly afterwards. Verified against the real model on a local API: - "add a T-shirt to my Japan Trip pack" -> listUserPacks resolves the id, addItemToPack writes the item with a 150 g catalog weight, assistant confirms "Added a T-shirt (150 g) to your Japan Trip pack." - "add a sleeping bag to my Everest Basecamp pack" (no such pack) -> listUserPacks returns empty, nothing is written, and it says so rather than inventing an id. Note executeSql still reads user rows from Postgres and ignores its own userId param, so it is untenanted. Out of scope here; flagged for the hardening follow-up its own comments already reference.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/swift/Sources/PackRat/AppState.swift`:
- Around line 31-34: Update AppState.init() and the LocalChatPackTools
construction to inject the active ModelContext instead of relying on its nil
default. Ensure offline or failed remote additions persist through both
OutboxService.enqueue and upsertCachedPack, preserving the existing shared
PacksViewModel data path.
In `@packages/env/src/node.ts`:
- Around line 125-128: Update the App Store Connect credential validation around
APPLE_ASC_API_KEY_ID and APPLE_ASC_API_ISSUER_ID so configurations supplying
only one field are rejected with a direct configuration error. Ensure both
fields must be present together, while preserving optional behavior when neither
is provided and the existing complete-credentials path.
🪄 Autofix
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 Plus
Run ID: 2de3978c-64aa-4bcc-93e4-69a73cab98a8
📒 Files selected for processing (7)
apps/swift/Sources/PackRat/AppState.swiftapps/swift/Sources/PackRat/Features/Chat/ChatPackTools.swiftapps/swift/Sources/PackRat/Features/Chat/ChatViewModel.swiftapps/swift/Sources/PackRat/Features/Chat/LocalChatPackTools.swiftpackages/api/src/routes/chat.tspackages/api/src/utils/ai/tools.tspackages/env/src/node.ts
Comment on lines
+31
to
+34
| // Back the assistant's pack tools with the local store, so it can find | ||
| // packs by name and add items to them against the same data the Packs tab | ||
| // shows. Without this every pack reads as missing, even ones on screen. | ||
| chatVM = ChatViewModel(packTools: LocalChatPackTools(packsViewModel: packsVM)) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'func (enqueue|upsertCachedPack)\b|context: ModelContext\?' apps/swift/Sources/PackRatRepository: PackRat-AI/PackRat
Length of output: 34153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'LocalChatPackTools|packTools|func addItem\b|modelContext' \
apps/swift/Sources/PackRat/AppState.swift \
apps/swift/Sources/PackRatRepository: PackRat-AI/PackRat
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '280,333p' apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swift
sed -n '51,78p' apps/swift/Sources/PackRat/Services/OutboxService.swift
sed -n '610,628p' apps/swift/Sources/PackRat/Features/Packs/PacksViewModel.swiftRepository: PackRat-AI/PackRat
Length of output: 4647
Inject the active ModelContext into global chat pack tools.
AppState.init() uses the default nil context. Offline or failed remote additions then skip both OutboxService.enqueue and upsertCachedPack, so the item exists only in memory and is lost on relaunch. Pass the active context or move persistence ownership into PacksViewModel.
🤖 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/AppState.swift` around lines 31 - 34, Update
AppState.init() and the LocalChatPackTools construction to inject the active
ModelContext instead of relying on its nil default. Ensure offline or failed
remote additions persist through both OutboxService.enqueue and
upsertCachedPack, preserving the existing shared PacksViewModel data path.
Comment on lines
+125
to
+128
| // App Store Connect API key auth, as an alternative to APPLE_ID + | ||
| // APPLE_APP_PASSWORD (apps/swift/scripts/upload-testflight.ts). | ||
| APPLE_ASC_API_KEY_ID: z.string().min(1).optional(), | ||
| APPLE_ASC_API_ISSUER_ID: z.string().uuid().optional(), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject partial App Store Connect credentials.
APPLE_ASC_API_KEY_ID and APPLE_ASC_API_ISSUER_ID are validated independently. In apps/swift/scripts/upload-testflight.ts Lines 188-190, API-key authentication activates only when both values exist. A partial configuration therefore falls back to the Apple ID path and can report misleading missing APPLE_ID or APPLE_APP_PASSWORD errors. Validate both fields together, or fail in upload preflight with a direct configuration error.
🤖 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/env/src/node.ts` around lines 125 - 128, Update the App Store
Connect credential validation around APPLE_ASC_API_KEY_ID and
APPLE_ASC_API_ISSUER_ID so configurations supplying only one field are rejected
with a direct configuration error. Ensure both fields must be present together,
while preserving optional behavior when neither is provided and the existing
complete-credentials path.
Collaborator
Author
|
Thanks — reviewed all five. Two were real and are fixed; three no longer apply because the tool architecture changed after this review ran. Fixed
Obsolete — the code no longer exists
Declining, with reasoning
Unrelated to this PR
|
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.
Fixes the batch of iOS QA issues from #2708–#2717.
Each fix is its own commit with the root cause in the message. Several of these were not what the report described, so the notes below are worth reading.
Fixed
OfflineBanner()was only mounted insplitLayout(iPad regular width / macOS). Every iPhone runs the compact branch, so the banner was never in the view tree.NetworkMonitorwas working correctly. Hoisted intonavigationBodyso both layouts get it from one place.ChatViewappliedkeyboardDoneButton; itsToolbarItemGroup(.keyboard)renders an accessory bar exactly where the pinned composer already is. Removed — chat already had both dismissal paths the modifier exists to provide. The ~12Formcall sites keep it, where it is correct.^[3 item](inflect: true).xcstrings. Pluralized in Swift. Same bug fixed in the scan sheet header.ToolbarItem(.primaryAction), which collapses on iPhone against the always-visible search drawer. AddedAdd (N)to the selection bar itself.localizedDescription. Real offline requests throw.cannotFindHost/.dnsLookupFailed, which matched no keyword bucket. Now classified byURLError.Code..task, andTabViewbuilds tabs lazily. Hence "fixed by switching tabs". Gave Home its own.task.loadMore()appended without deduping whileload()could reassign concurrently; duplicate ids then collide inForEachidentity, making it visible. Deduped by id.AppPreferenceswrapped@AppStoragein anObservableObject(which does not publish), and nothing outsidePreferencesViewever read the key. Four hard-coded g/kg formatters took no unit at all. Conversion now lives onAppWeightUnit, propagated via anEnvironmentValues.weightUnitinjected once at the root.Also fixed along the way
CachedPack/CachedTriphave no user column and survived sign-out, so the next user to sign in on a device saw the previous user's packs and trips from cache before the network replaced them. Purged onsignOut.executeSqlguard false positives. It rejected any query merely containing a forbidden keyword as a substring. Becausepackshas adeletedcolumn, the correct soft-delete-aware query was always rejected as a mutation;created_atandupdated_athit the same trap viacreate/update. Now matched on word boundaries, with 14 new tests confirming realDELETE/DROP/mixed-case variants are still rejected.Not fixed — needs a decision
#2710 is fully closed, client-side. The assistant had no way to look up a pack by name, so "I couldn't find a pack named 'Japan Trip'" was the model narrating a failure it could not avoid — and it had no tool to add an item either.
Both halves now work, and every tool that touches the user's own packs (
listUserPacks,getPackDetails,getPackItemDetails,addItemToPack) is declared server-side with noexecuteand answered from the device's local store.That placement is deliberate. The local store is what the user is looking at and it is the write path — mutations land there first and sync outward through the outbox. A server-side
addItemToPackwould write Postgres behind the UI, so an item the assistant "added" would stay invisible until the next refresh and nothing would work offline. Reads move for the same reason: a pack created offline is now findable, and the names the model matches against are the ones on screen. Writes reusePacksViewModel.addItem, so an assistant-added item is indistinguishable from one added by tapping through the UI.Tools over shared or external data (weather, catalog, guides, web search) stay server-side, where the API keys are. Only the Swift client is wired here;
apps/expois a follow-up.Still open, flagged not fixed:
executeSqlreads user rows from Postgres and ignores its ownuserIdparameter, so it is untenanted and can reach the auth/session tables. Out of scope for this PR — the file's own comments already defer this to a hardening plan — but it should get an issue.Verification
xcodebuild build— BUILD SUCCEEDED for bothPackRat-iOS(iPhone 17 simulator, iOS 26.4) andPackRat-macOSbun test:api:unit— 627 passed / 44 files, plus 8 new tests forlistUserPacksAiToolbun check:coverage—packages/apiimproves (functions back to 100%)bun check-types— cleanbiome check— clean on all touched filesorigin/developmentshowing no banner under identical conditions.CI notes
PackRat-iOS (smoke)/PackRat-macOS (smoke)red runs on this PR were not code failures — one wassetup-bun@v2dying withTypeError: fetch failed11s in, the others were runs cancelled at "Install dependencies" by a newer push superseding them. Nothing of this branch had compiled at that point. Note also that Swift CI fails intermittently ondevelopmentitself (most recent run before this PR was red).Pre-existing failures, not from this branch
AuthTests/testGuestSeesNativeSignInStateForAIToolsand.../ForAccountBackedFeaturesfail on the Home action rows "Season Suggestions" and "Pack Templates". Verified against a cleanorigin/developmentworktree on the same simulator:failedTests: 2, passedTests: 0— same two tests, same assertions. Untouched by this branch and left alone.Summary by CodeRabbit