Closed
Conversation
- Make success flash a Pressable with tap-to-dismiss, hint text, a11y, and timer cleanup - Document Expo Go vs dev build, frontend npm scripts, and iOS CocoaPods in README and QUICK_START Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces comprehensive updates to the PolyBuys frontend, including revised testing documentation for device/simulator workflows, a new flash notification system via FlashContext, systematic replacement of Alert-based error/success modals with in-screen banners across multiple screens, a new error normalization utility, full iOS native project configuration with Xcode setup, and new npm scripts for native builds. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen
participant FlashProvider
participant Banner
User->>Screen: Action (e.g., create listing)
activate Screen
Screen->>Screen: Attempt operation
alt Success
Screen->>FlashProvider: setFlash('Operation succeeded')
activate FlashProvider
FlashProvider->>Banner: render with message
activate Banner
Banner->>User: Display banner
Banner->>FlashProvider: Auto-dismiss (2s)
deactivate Banner
FlashProvider->>Banner: clear
deactivate FlashProvider
else Error
Screen->>Screen: catch error
Screen->>Screen: getConvexErrorDisplay(error)
Screen->>Screen: setState(errorState)
Screen->>Banner: Render error banner
activate Banner
Banner->>User: Display error message
User->>Banner: Press dismiss or action
Banner->>Screen: Clear error state
deactivate Banner
end
deactivate Screen
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/(tabs)/settings.tsx (1)
149-165:⚠️ Potential issue | 🟡 MinorRedundant
updateProfilecall aftercreateProfile.When
!profile, the code callscreateProfileand then unconditionally falls through to callupdateProfilewith the same data. This results in two mutations for new profiles. Either return aftercreateProfileor wrapupdateProfilein anelseblock.Proposed fix
if (!profile) { await createProfile({ name: trimmedName, email: normalizedEmail, bio: trimmedBio || undefined, major: trimmedMajor, year: parsedYear, }); - } - - await updateProfile({ + setSaveSuccess(true); + } else { + await updateProfile({ name: trimmedName, email: normalizedEmail, bio: trimmedBio, major: trimmedMajor, year: parsedYear, }); + setSaveSuccess(true); + } - setSaveSuccess(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`(tabs)/settings.tsx around lines 149 - 165, The code currently calls createProfile(...) when profile is falsy and then always calls updateProfile(...), causing duplicate mutations for new users; modify the save flow in the handler in settings.tsx so that after calling createProfile(...) you immediately return (or otherwise skip the rest), or wrap the subsequent updateProfile(...) call in an else block so updateProfile only runs when profile exists, referencing the createProfile and updateProfile calls shown in the diff.
🧹 Nitpick comments (8)
frontend/ios/PolyBuys/Info.plist (1)
45-46: Align minimum iOS version metadata with Podfile target.Line 46 says
12.0, whilefrontend/ios/Podfilesets iOS15.1. Keep them aligned to avoid confusing release/build expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/ios/PolyBuys/Info.plist` around lines 45 - 46, The Info.plist's LSMinimumSystemVersion key currently specifies "12.0" which conflicts with the Podfile iOS target of 15.1; update the value of LSMinimumSystemVersion in Info.plist to "15.1" so it matches the Podfile target and avoids mismatched build/release expectations, ensuring the change references the LSMinimumSystemVersion entry in Info.plist and aligns with the iOS target specified in the Podfile.frontend/app/(tabs)/index.tsx (1)
197-203: Consider usingencodeURIComponentfor consistency.The
returnToparameter works correctly here since/listings/newhas no special characters, but other files in the codebase (e.g.,frontend/app/listings/[id].tsxline 79,frontend/app/conversations/[id].tsxline 140) useencodeURIComponent()for thereturnTovalue. Using it consistently protects against future changes where the path might include special characters.Suggested change for consistency
const handleCreateListing = () => { if (!isAuthenticated) { - router.push('/auth/login?returnTo=/listings/new'); + router.push(`/auth/login?returnTo=${encodeURIComponent('/listings/new')}`); return; } router.push('/listings/new'); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`(tabs)/index.tsx around lines 197 - 203, In handleCreateListing replace the raw '/listings/new' in the router.push call that sets the returnTo query with an encoded value: build the URL using encodeURIComponent('/listings/new') for the returnTo parameter (same pattern used in other files like listings/[id].tsx and conversations/[id].tsx) so the router.push call uses something like router.push(`/auth/login?returnTo=${encodeURIComponent('/listings/new')}`) to ensure consistency and future-proof against special characters.frontend/components/HiddenBanner.tsx (1)
7-14: Consider using a boolean for the error state.The
mailErrorstate is typed asstring | nullbut used as a boolean flag—you set it to'show'and check truthiness, while the actual message is hardcoded in the JSX. This works but is slightly inconsistent with other error states in the codebase that store the actual message.Cleaner approach using boolean
-const [mailError, setMailError] = useState<string | null>(null); +const [mailError, setMailError] = useState(false); const onAppealPress = () => { - setMailError(null); + setMailError(false); Linking.openURL(`mailto:${SUPPORT_EMAIL}`).catch(() => { - setMailError('show'); + setMailError(true); }); };Also applies to: 22-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/HiddenBanner.tsx` around lines 7 - 14, The mailError state is currently string | null but used as a boolean flag; change mailError to a boolean (useState<boolean>(false)) and update setMailError calls in onAppealPress (setMailError(false) before opening mail and setMailError(true) in the catch) and any corresponding conditional rendering to check mailError truthiness; also apply the same boolean change to the other similar state referenced around the 22-29 block so all error flags consistently use boolean values.frontend/lib/convexError.ts (1)
55-59: RegularErrormessages always fall back to generic message.The current logic extracts messages from regular
Errorinstances (lines 55-56) but then discards them at line 72-75 because onlyConvexErrorinstances pass the final check. This means network errors like"Network request failed"will always show the generic message.This is likely intentional for security, but worth confirming it's the desired UX—some fetch-related errors could be informative to users (e.g., "No internet connection").
Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/convexError.ts` around lines 55 - 59, The code extracts rawMessage from Error instances into rawMessage but then only uses ConvexError-specific data later, discarding plain Error messages; update the final error-handling logic in frontend/lib/convexError.ts (the block that checks for ConvexError) to fall back to the previously-captured rawMessage for non-ConvexError instances (or optionally treat known network-related messages specially), i.e., if the error is not an instance of ConvexError, return/format rawMessage instead of the generic "Unknown error"; reference the rawMessage variable and the ConvexError type/check so the plain Error path surfaces useful messages like "Network request failed".frontend/app/conversations/[id].tsx (1)
342-355: Consider addingaccessibilityRole="alert"to the error banner.The flash banner in
FlashContext.tsxusesaccessibilityRole="alert"for screen reader announcements. Adding the same to this error banner would improve accessibility consistency.Add accessibility role
{sendError ? ( - <View style={styles.errorBanner}> + <View style={styles.errorBanner} accessibilityRole="alert"> <Text style={styles.errorText} numberOfLines={2}> {sendError} </Text>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/conversations/`[id].tsx around lines 342 - 355, The error banner rendered when sendError is truthy (the View using styles.errorBanner in frontend/app/conversations/[id].tsx) should include accessibilityRole="alert" so screen readers announce it like FlashContext; update that View element to add accessibilityRole="alert" while keeping the existing Pressable and setSendError logic unchanged.frontend/app/(tabs)/settings.tsx (1)
265-274: Consider adding accessibility roles for screen readers.The error and success banners lack accessibility roles. For better screen reader support, consider adding
accessibilityRole="alert"to the error banner Views andaccessibilityRole="status"to the success banner, similar to the FlashContext implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`(tabs)/settings.tsx around lines 265 - 274, The error and success banner Views shown when saveError/saveSuccess are truthy lack accessibility roles; update the two conditional View components that use styles.errorBanner and styles.successBanner to include accessibilityRole="alert" on the error View and accessibilityRole="status" on the success View (the same components rendering the <Text> for saveError and "Profile saved."), ensuring screen readers announce these messages.frontend/contexts/FlashContext.tsx (1)
57-58: Consider memoizing the context value.The inline
{ setFlash }object is recreated on every render, which could cause unnecessary re-renders in consumers. SincesetFlashis already memoized withuseCallback, wrap the value inuseMemo.Proposed fix
+ const value = React.useMemo(() => ({ setFlash }), [setFlash]); + return ( - <FlashContext.Provider value={{ setFlash }}> + <FlashContext.Provider value={value}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/contexts/FlashContext.tsx` around lines 57 - 58, The FlashContext.Provider currently passes an inline object { setFlash } which is recreated each render; wrap that value in useMemo so the provider value is stable (e.g., memoize { setFlash } with useMemo depending on setFlash) to prevent unnecessary consumer re-renders; update the FlashContext.Provider usage in the FlashContext component to compute the provider value via useMemo and return that memoized value to the provider.frontend/components/ImageUploader.tsx (1)
411-445: Consider hiding or disabling "Add Image" when source picker is shown.When
showSourcePickeris true, the user sees Camera/Photo Library/Cancel buttons, but the "Add Image" button below remains enabled and visible. Tapping it again is a no-op (sinceshowSourcePickeris already true), but it could confuse users. Consider conditionally hiding or disabling it.Proposed fix
- <Pressable + {!showSourcePicker ? ( + <Pressable style={[ styles.addButton, images.length + pendingUploads.length >= maxImages && styles.addButtonDisabled, ]} onPress={() => { void addImage(); }} disabled={images.length + pendingUploads.length >= maxImages} > <Text style={styles.addButtonText}>Add Image</Text> </Pressable> + ) : null}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ImageUploader.tsx` around lines 411 - 445, The "Add Image" Pressable remains visible and active while showSourcePicker is true, which is confusing; update the UI so the add control is either hidden or disabled whenever showSourcePicker is true by gating the Add Image render or its disabled prop on !showSourcePicker (or add showSourcePicker to the disabled expression) and apply the addButtonDisabled style when hidden/disabled; locate the Pressable using the addImage callback and styles addButton/addButtonDisabled to implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/ios/PolyBuys.xcodeproj/project.pbxproj`:
- Around line 98-120: The xcscheme references a missing test target
(BlueprintIdentifier "00E356ED1AD99517003FC87E", BlueprintName "PolyBuysTests")
which is not present in the PBXNativeTarget section (e.g., targets like
13B07F861A680F5B00A75B9A for PolyBuys exist); either add a PBXNativeTarget entry
for PolyBuysTests with a unique productReference and matching GUID,
buildConfigurationList and appropriate buildPhases, or edit the
PolyBuys.xcscheme to remove or update the <TestableReference> that points to
BlueprintIdentifier "00E356ED1AD99517003FC87E" so the scheme no longer
references a non-existent test target.
In `@frontend/ios/PolyBuys.xcodeproj/xcshareddata/xcschemes/PolyBuys.xcscheme`:
- Around line 25-41: The scheme contains a stale TestableReference for the test
target "PolyBuysTests" inside the TestAction/BuildableReference
(BlueprintIdentifier "00E356ED1AD99517003FC87E"); either remove the entire
TestableReference/BuildableReference block or replace its
BlueprintIdentifier/BlueprintName/BuildableName with the correct values for the
actual test target in the project so the TestAction references a real blueprint;
update the TestableReference under TestAction accordingly to restore valid
scheme test builds.
In `@frontend/ios/PolyBuys/AppDelegate.swift`:
- Around line 34-41: The URL open handler application(_:open:options:) currently
uses short-circuiting with "super.application(... ) ||
RCTLinkingManager.application(...)" which can prevent
RCTLinkingManager.application from being called if super returns true; change it
to call RCTLinkingManager.application(app, open: url, options: options) first,
store its Bool result (e.g., rctHandled), then call super.application(app, open:
url, options: options) and return rctHandled || superHandled so both handlers
run and the final return preserves previous semantics; update the
application(_:open:options:) implementation accordingly.
- Around line 1-3: The AppDelegate.swift uses the Swift-6-only declaration
"internal import Expo" which conflicts with the project's SWIFT_VERSION = 5.0;
either update the project's Swift version to 6.0+ in the Xcode project settings,
or change the import to a standard Swift 5-compatible form by removing the
access modifier (replace "internal import Expo" with a regular "import Expo")
and ensure other similar lines ("import React", "import
ReactAppDependencyProvider") remain standard imports; modify whichever approach
you choose consistently across AppDelegate.swift and any other files using
"internal import".
In `@frontend/ios/PolyBuys/PolyBuys.entitlements`:
- Around line 5-6: The entitlements file hardcodes aps-environment to
"development"; change the value in PolyBuys.entitlements to use a build setting
variable (e.g., replace the literal string with the build variable reference
$(APS_ENVIRONMENT) for the <key>aps-environment</key> entry) and then define
APS_ENVIRONMENT in your Xcode build configurations (set
APS_ENVIRONMENT=development for Debug and APS_ENVIRONMENT=production for
Release) so the correct APNs environment is injected at build time.
In `@frontend/ios/PolyBuys/SplashScreen.storyboard`:
- Around line 19-24: The storyboard contains constraints referencing a missing
view (EXPO-SplashScreen) which creates dangling references against
EXPO-ContainerView; remove or fix these constraints by either adding the missing
subview with the matching id (EXPO-SplashScreen) under the container
(EXPO-ContainerView) or deleting/rewiring the two constraints (ids 0VC-Wk-OaO
and zR4-NK-mVN) so they reference an existing view; ensure the safeArea/layout
hierarchy remains valid after the change and validate the storyboard loads in
Interface Builder.
In `@QUICK_START.md`:
- Around line 40-50: The documentation is ambiguous about where `npm run
run:ios` and `npm run run:android` should be executed; update the QUICK_START.md
rows that list `npm run run:ios` and `npm run run:android` (from the scripts
table referencing frontend/package.json) to either prefix them with the correct
workspace context (e.g., instruct users to run `cd frontend && npm run run:ios`
/ `cd frontend && npm run run:android`) or explicitly state that equivalent
passthrough scripts exist at the repository root if you added them; ensure the
table text clearly references `frontend/package.json` and the exact script names
(`npm run run:ios`, `npm run run:android`) so copy-pasting works without
failure.
In `@README.md`:
- Line 87: The README contains a broken anchor link pointing to
QUICK_START.md#expo-go-development-build-and-npm-scripts; update the link target
to match the actual heading which includes "vs" by changing it to
QUICK_START.md#expo-go-vs-development-build-and-npm-scripts so the anchor
resolves correctly.
---
Outside diff comments:
In `@frontend/app/`(tabs)/settings.tsx:
- Around line 149-165: The code currently calls createProfile(...) when profile
is falsy and then always calls updateProfile(...), causing duplicate mutations
for new users; modify the save flow in the handler in settings.tsx so that after
calling createProfile(...) you immediately return (or otherwise skip the rest),
or wrap the subsequent updateProfile(...) call in an else block so updateProfile
only runs when profile exists, referencing the createProfile and updateProfile
calls shown in the diff.
---
Nitpick comments:
In `@frontend/app/`(tabs)/index.tsx:
- Around line 197-203: In handleCreateListing replace the raw '/listings/new' in
the router.push call that sets the returnTo query with an encoded value: build
the URL using encodeURIComponent('/listings/new') for the returnTo parameter
(same pattern used in other files like listings/[id].tsx and
conversations/[id].tsx) so the router.push call uses something like
router.push(`/auth/login?returnTo=${encodeURIComponent('/listings/new')}`) to
ensure consistency and future-proof against special characters.
In `@frontend/app/`(tabs)/settings.tsx:
- Around line 265-274: The error and success banner Views shown when
saveError/saveSuccess are truthy lack accessibility roles; update the two
conditional View components that use styles.errorBanner and styles.successBanner
to include accessibilityRole="alert" on the error View and
accessibilityRole="status" on the success View (the same components rendering
the <Text> for saveError and "Profile saved."), ensuring screen readers announce
these messages.
In `@frontend/app/conversations/`[id].tsx:
- Around line 342-355: The error banner rendered when sendError is truthy (the
View using styles.errorBanner in frontend/app/conversations/[id].tsx) should
include accessibilityRole="alert" so screen readers announce it like
FlashContext; update that View element to add accessibilityRole="alert" while
keeping the existing Pressable and setSendError logic unchanged.
In `@frontend/components/HiddenBanner.tsx`:
- Around line 7-14: The mailError state is currently string | null but used as a
boolean flag; change mailError to a boolean (useState<boolean>(false)) and
update setMailError calls in onAppealPress (setMailError(false) before opening
mail and setMailError(true) in the catch) and any corresponding conditional
rendering to check mailError truthiness; also apply the same boolean change to
the other similar state referenced around the 22-29 block so all error flags
consistently use boolean values.
In `@frontend/components/ImageUploader.tsx`:
- Around line 411-445: The "Add Image" Pressable remains visible and active
while showSourcePicker is true, which is confusing; update the UI so the add
control is either hidden or disabled whenever showSourcePicker is true by gating
the Add Image render or its disabled prop on !showSourcePicker (or add
showSourcePicker to the disabled expression) and apply the addButtonDisabled
style when hidden/disabled; locate the Pressable using the addImage callback and
styles addButton/addButtonDisabled to implement the change.
In `@frontend/contexts/FlashContext.tsx`:
- Around line 57-58: The FlashContext.Provider currently passes an inline object
{ setFlash } which is recreated each render; wrap that value in useMemo so the
provider value is stable (e.g., memoize { setFlash } with useMemo depending on
setFlash) to prevent unnecessary consumer re-renders; update the
FlashContext.Provider usage in the FlashContext component to compute the
provider value via useMemo and return that memoized value to the provider.
In `@frontend/ios/PolyBuys/Info.plist`:
- Around line 45-46: The Info.plist's LSMinimumSystemVersion key currently
specifies "12.0" which conflicts with the Podfile iOS target of 15.1; update the
value of LSMinimumSystemVersion in Info.plist to "15.1" so it matches the
Podfile target and avoids mismatched build/release expectations, ensuring the
change references the LSMinimumSystemVersion entry in Info.plist and aligns with
the iOS target specified in the Podfile.
In `@frontend/lib/convexError.ts`:
- Around line 55-59: The code extracts rawMessage from Error instances into
rawMessage but then only uses ConvexError-specific data later, discarding plain
Error messages; update the final error-handling logic in
frontend/lib/convexError.ts (the block that checks for ConvexError) to fall back
to the previously-captured rawMessage for non-ConvexError instances (or
optionally treat known network-related messages specially), i.e., if the error
is not an instance of ConvexError, return/format rawMessage instead of the
generic "Unknown error"; reference the rawMessage variable and the ConvexError
type/check so the plain Error path surfaces useful messages like "Network
request failed".
🪄 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: CHILL
Plan: Pro
Run ID: b267467e-a0d2-475d-b195-c533a2d41692
⛔ Files ignored due to path filters (1)
frontend/ios/PolyBuys/Images.xcassets/AppIcon.appiconset/App-Icon-1024x1024@1x.pngis excluded by!**/*.png
📒 Files selected for processing (30)
QUICK_START.mdREADME.mdfrontend/app/(tabs)/index.tsxfrontend/app/(tabs)/settings.tsxfrontend/app/_layout.tsxfrontend/app/conversations/[id].tsxfrontend/app/listings/[id].tsxfrontend/app/listings/[id]/edit.tsxfrontend/app/listings/new.tsxfrontend/components/HiddenBanner.tsxfrontend/components/ImageUploader.tsxfrontend/components/ReportModal.tsxfrontend/contexts/FlashContext.tsxfrontend/ios/.gitignorefrontend/ios/.xcode.envfrontend/ios/Podfilefrontend/ios/Podfile.properties.jsonfrontend/ios/PolyBuys.xcodeproj/project.pbxprojfrontend/ios/PolyBuys.xcodeproj/xcshareddata/xcschemes/PolyBuys.xcschemefrontend/ios/PolyBuys/AppDelegate.swiftfrontend/ios/PolyBuys/Images.xcassets/AppIcon.appiconset/Contents.jsonfrontend/ios/PolyBuys/Images.xcassets/Contents.jsonfrontend/ios/PolyBuys/Images.xcassets/SplashScreenBackground.colorset/Contents.jsonfrontend/ios/PolyBuys/Info.plistfrontend/ios/PolyBuys/PolyBuys-Bridging-Header.hfrontend/ios/PolyBuys/PolyBuys.entitlementsfrontend/ios/PolyBuys/SplashScreen.storyboardfrontend/ios/PolyBuys/Supporting/Expo.plistfrontend/lib/convexError.tsfrontend/package.json
Comment on lines
+25
to
+41
| <TestAction | ||
| buildConfiguration = "Debug" | ||
| selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB" | ||
| selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" | ||
| shouldUseLaunchSchemeArgsEnv = "YES"> | ||
| <Testables> | ||
| <TestableReference | ||
| skipped = "NO"> | ||
| <BuildableReference | ||
| BuildableIdentifier = "primary" | ||
| BlueprintIdentifier = "00E356ED1AD99517003FC87E" | ||
| BuildableName = "PolyBuysTests.xctest" | ||
| BlueprintName = "PolyBuysTests" | ||
| ReferencedContainer = "container:PolyBuys.xcodeproj"> | ||
| </BuildableReference> | ||
| </TestableReference> | ||
| </Testables> |
Contributor
There was a problem hiding this comment.
Remove or fix stale test target reference in scheme.
Lines 35-37 point to PolyBuysTests, but the project target list shows no matching blueprint. This will break scheme test builds.
Suggested fix (if tests target is not yet added)
<TestAction
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
shouldUseLaunchSchemeArgsEnv = "YES">
- <Testables>
- <TestableReference
- skipped = "NO">
- <BuildableReference
- BuildableIdentifier = "primary"
- BlueprintIdentifier = "00E356ED1AD99517003FC87E"
- BuildableName = "PolyBuysTests.xctest"
- BlueprintName = "PolyBuysTests"
- ReferencedContainer = "container:PolyBuys.xcodeproj">
- </BuildableReference>
- </TestableReference>
- </Testables>
+ <Testables/>
</TestAction>📝 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
| <TestAction | |
| buildConfiguration = "Debug" | |
| selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB" | |
| selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" | |
| shouldUseLaunchSchemeArgsEnv = "YES"> | |
| <Testables> | |
| <TestableReference | |
| skipped = "NO"> | |
| <BuildableReference | |
| BuildableIdentifier = "primary" | |
| BlueprintIdentifier = "00E356ED1AD99517003FC87E" | |
| BuildableName = "PolyBuysTests.xctest" | |
| BlueprintName = "PolyBuysTests" | |
| ReferencedContainer = "container:PolyBuys.xcodeproj"> | |
| </BuildableReference> | |
| </TestableReference> | |
| </Testables> | |
| <TestAction | |
| buildConfiguration = "Debug" | |
| selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB" | |
| selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" | |
| shouldUseLaunchSchemeArgsEnv = "YES"> | |
| <Testables/> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/ios/PolyBuys.xcodeproj/xcshareddata/xcschemes/PolyBuys.xcscheme`
around lines 25 - 41, The scheme contains a stale TestableReference for the test
target "PolyBuysTests" inside the TestAction/BuildableReference
(BlueprintIdentifier "00E356ED1AD99517003FC87E"); either remove the entire
TestableReference/BuildableReference block or replace its
BlueprintIdentifier/BlueprintName/BuildableName with the correct values for the
actual test target in the project so the TestAction references a real blueprint;
update the TestableReference under TestAction accordingly to restore valid
scheme test builds.
Comment on lines
+1
to
+3
| internal import Expo | ||
| import React | ||
| import ReactAppDependencyProvider |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Swift internal import syntax minimum version requirement
💡 Result:
The "internal import" syntax in Swift, introduced by SE-0409 (Access-level modifiers on import declarations), requires Swift 6.0 or later. In Swift 6.0, it is available without flags. In earlier versions like Swift 5.9/5.10, it required the experimental flag -enable-experimental-feature AccessLevelOnImport, which was later lifted as the feature stabilized for Swift 6. There is no @_internalImport attribute; "internal import ModuleName" is the stable syntax replacing the previous @_implementationOnly import for hiding dependencies from public interfaces. Default import access level is public in Swift 5 and Swift 6 language modes, with an upcoming InternalImportsByDefault flag (opt-in in Swift 5, future default) to make it internal.
Citations:
- 1: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0409-access-level-on-imports.md
- 2: https://github.com/apple/swift-evolution/blob/main/proposals/0409-access-level-on-imports.md
- 3: https://hackingwithswift.com/swift/6.0/access-level-import
- 4: https://github.com/apple/swift/blob/main/docs/ReferenceGuides/UnderscoredAttributes.md
- 5: Sema: Lift flag requirement for access levels on imports now that SE-0409 has been accepted swiftlang/swift#68967
🏁 Script executed:
# Search for SWIFT_VERSION in project.pbxproj
fd -e pbxproj | head -5Repository: codebox-calpoly/PolyBuys
Length of output: 115
🏁 Script executed:
# Once file is found, check SWIFT_VERSION setting
find . -name "project.pbxproj" -type f | head -1 | xargs grep -i "SWIFT_VERSION"Repository: codebox-calpoly/PolyBuys
Length of output: 117
🏁 Script executed:
# Also check for experimental feature flags in build settings
find . -name "project.pbxproj" -type f | head -1 | xargs grep -i "enable-experimental-feature"Repository: codebox-calpoly/PolyBuys
Length of output: 52
🏁 Script executed:
cat -n frontend/ios/PolyBuys/AppDelegate.swift | head -20Repository: codebox-calpoly/PolyBuys
Length of output: 830
internal import requires Swift 6.0+, but project is configured for Swift 5.0.
The internal import Expo syntax on line 1 requires Swift 6.0 or later. The project's project.pbxproj sets SWIFT_VERSION = 5.0, which will cause a compilation error.
Either update SWIFT_VERSION to 6.0 or later, or change to a standard import:
Proposed fix (if keeping Swift 5.0)
-internal import Expo
+import Expo📝 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
| internal import Expo | |
| import React | |
| import ReactAppDependencyProvider | |
| import Expo | |
| import React | |
| import ReactAppDependencyProvider |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/ios/PolyBuys/AppDelegate.swift` around lines 1 - 3, The
AppDelegate.swift uses the Swift-6-only declaration "internal import Expo" which
conflicts with the project's SWIFT_VERSION = 5.0; either update the project's
Swift version to 6.0+ in the Xcode project settings, or change the import to a
standard Swift 5-compatible form by removing the access modifier (replace
"internal import Expo" with a regular "import Expo") and ensure other similar
lines ("import React", "import ReactAppDependencyProvider") remain standard
imports; modify whichever approach you choose consistently across
AppDelegate.swift and any other files using "internal import".
Comment on lines
+34
to
+41
| // Linking API | ||
| public override func application( | ||
| _ app: UIApplication, | ||
| open url: URL, | ||
| options: [UIApplication.OpenURLOptionsKey: Any] = [:] | ||
| ) -> Bool { | ||
| return super.application(app, open: url, options: options) || RCTLinkingManager.application(app, open: url, options: options) | ||
| } |
Contributor
There was a problem hiding this comment.
Short-circuit evaluation may prevent RCTLinkingManager from handling URLs.
On line 40, if super.application(...) returns true, the || short-circuit prevents RCTLinkingManager.application(...) from being called. This could cause deep links to be missed by React Native.
The universal links handler (lines 49-50) correctly calls RCTLinkingManager first and stores the result before combining. Apply the same pattern here for consistency:
Proposed fix
public override func application(
_ app: UIApplication,
open url: URL,
options: [UIApplication.OpenURLOptionsKey: Any] = [:]
) -> Bool {
- return super.application(app, open: url, options: options) || RCTLinkingManager.application(app, open: url, options: options)
+ let result = RCTLinkingManager.application(app, open: url, options: options)
+ return super.application(app, open: url, options: options) || result
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/ios/PolyBuys/AppDelegate.swift` around lines 34 - 41, The URL open
handler application(_:open:options:) currently uses short-circuiting with
"super.application(... ) || RCTLinkingManager.application(...)" which can
prevent RCTLinkingManager.application from being called if super returns true;
change it to call RCTLinkingManager.application(app, open: url, options:
options) first, store its Bool result (e.g., rctHandled), then call
super.application(app, open: url, options: options) and return rctHandled ||
superHandled so both handlers run and the final return preserves previous
semantics; update the application(_:open:options:) implementation accordingly.
Comment on lines
+5
to
+6
| <key>aps-environment</key> | ||
| <string>development</string> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the same entitlements file is used in both Debug/Release
rg -n -C2 'CODE_SIGN_ENTITLEMENTS|APS_ENVIRONMENT|PolyBuys\.entitlements' frontend/ios/PolyBuys.xcodeproj/project.pbxproj
rg -n -C1 'aps-environment' frontend/ios/PolyBuys/PolyBuys.entitlementsRepository: codebox-calpoly/PolyBuys
Length of output: 513
Avoid hardcoding APNs environment to development.
Line 6 hardcodes aps-environment to development. Both Debug and Release builds use the same entitlements file, so production releases will incorrectly use the development push environment, breaking push notifications. Use a build-setting-driven value instead.
Suggested fix
- <key>aps-environment</key>
- <string>development</string>
+ <key>aps-environment</key>
+ <string>$(APS_ENVIRONMENT)</string>Then configure the build setting in your Debug and Release configurations (e.g., APS_ENVIRONMENT = development for Debug, APS_ENVIRONMENT = production for Release).
📝 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
| <key>aps-environment</key> | |
| <string>development</string> | |
| <key>aps-environment</key> | |
| <string>$(APS_ENVIRONMENT)</string> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/ios/PolyBuys/PolyBuys.entitlements` around lines 5 - 6, The
entitlements file hardcodes aps-environment to "development"; change the value
in PolyBuys.entitlements to use a build setting variable (e.g., replace the
literal string with the build variable reference $(APS_ENVIRONMENT) for the
<key>aps-environment</key> entry) and then define APS_ENVIRONMENT in your Xcode
build configurations (set APS_ENVIRONMENT=development for Debug and
APS_ENVIRONMENT=production for Release) so the correct APNs environment is
injected at build time.
Comment on lines
+19
to
+24
| <subviews/> | ||
| <viewLayoutGuide key="safeArea" id="Rmq-lb-GrQ"/> | ||
| <constraints> | ||
| <constraint firstItem="EXPO-SplashScreen" firstAttribute="centerY" secondItem="EXPO-ContainerView" secondAttribute="centerY" id="0VC-Wk-OaO"/> | ||
| <constraint firstItem="EXPO-SplashScreen" firstAttribute="centerX" secondItem="EXPO-ContainerView" secondAttribute="centerX" id="zR4-NK-mVN"/> | ||
| </constraints> |
Contributor
There was a problem hiding this comment.
Fix invalid constraints referencing a missing view.
Line 19 has no splash subview, but Lines 22-23 constrain EXPO-SplashScreen. This leaves dangling references and can break iOS build/runtime loading of the launch storyboard.
Suggested fix
- <subviews/>
+ <subviews>
+ <imageView clipsSubviews="YES" userInteractionEnabled="NO" contentMode="scaleAspectFit" image="SplashScreenLogo" translatesAutoresizingMaskIntoConstraints="NO" id="EXPO-SplashScreen"/>
+ </subviews>
<viewLayoutGuide key="safeArea" id="Rmq-lb-GrQ"/>
<constraints>
<constraint firstItem="EXPO-SplashScreen" firstAttribute="centerY" secondItem="EXPO-ContainerView" secondAttribute="centerY" id="0VC-Wk-OaO"/>
<constraint firstItem="EXPO-SplashScreen" firstAttribute="centerX" secondItem="EXPO-ContainerView" secondAttribute="centerX" id="zR4-NK-mVN"/>
</constraints>📝 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
| <subviews/> | |
| <viewLayoutGuide key="safeArea" id="Rmq-lb-GrQ"/> | |
| <constraints> | |
| <constraint firstItem="EXPO-SplashScreen" firstAttribute="centerY" secondItem="EXPO-ContainerView" secondAttribute="centerY" id="0VC-Wk-OaO"/> | |
| <constraint firstItem="EXPO-SplashScreen" firstAttribute="centerX" secondItem="EXPO-ContainerView" secondAttribute="centerX" id="zR4-NK-mVN"/> | |
| </constraints> | |
| <subviews> | |
| <imageView clipsSubviews="YES" userInteractionEnabled="NO" contentMode="scaleAspectFit" image="SplashScreenLogo" translatesAutoresizingMaskIntoConstraints="NO" id="EXPO-SplashScreen"/> | |
| </subviews> | |
| <viewLayoutGuide key="safeArea" id="Rmq-lb-GrQ"/> | |
| <constraints> | |
| <constraint firstItem="EXPO-SplashScreen" firstAttribute="centerY" secondItem="EXPO-ContainerView" secondAttribute="centerY" id="0VC-Wk-OaO"/> | |
| <constraint firstItem="EXPO-SplashScreen" firstAttribute="centerX" secondItem="EXPO-ContainerView" secondAttribute="centerX" id="zR4-NK-mVN"/> | |
| </constraints> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/ios/PolyBuys/SplashScreen.storyboard` around lines 19 - 24, The
storyboard contains constraints referencing a missing view (EXPO-SplashScreen)
which creates dangling references against EXPO-ContainerView; remove or fix
these constraints by either adding the missing subview with the matching id
(EXPO-SplashScreen) under the container (EXPO-ContainerView) or
deleting/rewiring the two constraints (ids 0VC-Wk-OaO and zR4-NK-mVN) so they
reference an existing view; ensure the safeArea/layout hierarchy remains valid
after the change and validate the storyboard loads in Interface Builder.
Comment on lines
+40
to
+50
| Scripts are in `frontend/package.json` (root `npm run dev` starts the frontend workspace): | ||
|
|
||
| | Script | What it does | | ||
| | --------------------------- | -------------------------------------------------------------------- | | ||
| | `npm run dev` / `npm start` | Starts Metro (Expo dev server). | | ||
| | `npm run ios` | `expo start --ios` — simulator + Metro (**no** full native compile). | | ||
| | `npm run android` | `expo start --android` — same for Android. | | ||
| | `npm run web` | `expo start --web` | | ||
| | `npm run run:ios` | `expo run:ios` — **native** Xcode build (needs CocoaPods, signing). | | ||
| | `npm run run:android` | `expo run:android` — **native** Gradle build. | | ||
|
|
Contributor
There was a problem hiding this comment.
Clarify command scope for run:ios / run:android to avoid broken copy-paste.
Line 40 states scripts live in frontend/package.json, but Lines 48-49 show root-level npm run run:ios / npm run run:android. If those scripts are not mirrored at root, these commands fail. Make these rows explicitly cd frontend && npm run run:ios / cd frontend && npm run run:android (or explicitly document root passthrough scripts).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@QUICK_START.md` around lines 40 - 50, The documentation is ambiguous about
where `npm run run:ios` and `npm run run:android` should be executed; update the
QUICK_START.md rows that list `npm run run:ios` and `npm run run:android` (from
the scripts table referencing frontend/package.json) to either prefix them with
the correct workspace context (e.g., instruct users to run `cd frontend && npm
run run:ios` / `cd frontend && npm run run:android`) or explicitly state that
equivalent passthrough scripts exist at the repository root if you added them;
ensure the table text clearly references `frontend/package.json` and the exact
script names (`npm run run:ios`, `npm run run:android`) so copy-pasting works
without failure.
|
|
||
| **iOS troubleshooting:** If `expo run:ios` fails on CocoaPods, install the [CocoaPods CLI](https://cocoapods.org/), then run `pod install` inside `frontend/ios`. For signing errors, open the `.xcworkspace` in Xcode, set your **Team** under Signing & Capabilities, then build from Xcode or retry `run:ios`. | ||
|
|
||
| More detail: [QUICK_START.md](QUICK_START.md#expo-go-development-build-and-npm-scripts). |
Contributor
There was a problem hiding this comment.
Fix broken QUICK_START anchor link.
The link target appears incorrect: #expo-go-development-build-and-npm-scripts likely won’t resolve because the heading includes “vs”. Use #expo-go-vs-development-build-and-npm-scripts to match the section title.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 87, The README contains a broken anchor link pointing to
QUICK_START.md#expo-go-development-build-and-npm-scripts; update the link target
to match the actual heading which includes "vs" by changing it to
QUICK_START.md#expo-go-vs-development-build-and-npm-scripts so the anchor
resolves correctly.
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
2 participants
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.
Linked Issues
Closes #
Linear: (e.g., POLY-123)
Summary
Briefly explain the change and why.
How to Test
Steps to verify locally:
npm run lintnpm run typechecknpm testnpm run dev:backend(in terminal A)npm run dev(in terminal B)Checklist
npm run lint)devScreenshots / Demos
(if UI or visible behavior - attach images, videos, or GIFs)
Summary by CodeRabbit
New Features
Documentation
Chores
run:ios,run:android)