Repository navigation
feat(ios): iOS OAuth improvements with native Keychain and SPM - #245
Merged
Merged
Conversation
Replace CocoaPods dependency management with native Swift Package Manager for simpler builds and better Xcode integration. - Remove Podfile and Podfile.lock - Remove xcworkspace (SPM uses xcodeproj directly) - Add CapApp-SPM package for Capacitor dependencies - Add debug.xcconfig for build configuration - Update project.pbxproj for SPM package references Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace @aparajita/capacitor-secure-storage with a custom native Swift plugin for more reliable iOS Keychain access. - Add PageSpaceKeychainPlugin.swift native implementation - Add TypeScript interfaces for iOS and web platforms - Update auth-bridge.ts to use new plugin API - Remove @aparajita/capacitor-secure-storage dependency The custom plugin provides simpler get/set/remove operations without the complexity of third-party abstractions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address two issues with iOS OAuth: 1. Force account picker display: - Add forcePrompt: true to SocialLogin.login() options - Prevents auto-selection of cached Google credentials 2. Fix post-login navigation: - Use window.location.href instead of router.replace() - Resolves navigation failure in Capacitor WebView Additional improvements: - Skip GoogleOneTap on Capacitor native apps (use native SDK instead) - Add iOS Keychain session clearing on logout - Add iOS session token retrieval in auth store - Remove obsolete NativeAppRedirect component - Add clearStoredSession and getSessionToken helpers Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update capacitor.config.ts to load /dashboard directly (skip landing) - Update Main.storyboard to use custom MyViewController - Add NEXT_PUBLIC_GOOGLE_OAUTH_IOS_CLIENT_ID to Dockerfile build args Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughMigrate iOS from CocoaPods to a local Swift Package (CapApp‑SPM); add a PageSpaceKeychain iOS Capacitor plugin (Swift) and TypeScript bridge; replace SecureStorage with PageSpaceKeychain across iOS/web auth flows; update Xcode/project configs, package manifests, and navigation/link handling for Capacitor contexts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp as Web App (TS)
participant CapacitorBridge as Capacitor Bridge
participant iOSPlugin as PageSpaceKeychain Plugin (Swift)
participant Keychain as iOS Keychain
participant Server
User->>WebApp: completes Google sign-in (native)
WebApp->>CapacitorBridge: PageSpaceKeychain.set(sessionKey, sessionData)
CapacitorBridge->>iOSPlugin: invoke set(...)
iOSPlugin->>Keychain: SecItemAdd / SecItemDelete -> store data
Keychain-->>iOSPlugin: success
iOSPlugin-->>CapacitorBridge: promise resolved
CapacitorBridge-->>WebApp: success
alt app start / token retrieval
WebApp->>CapacitorBridge: PageSpaceKeychain.get(sessionKey)
CapacitorBridge->>iOSPlugin: invoke get(...)
iOSPlugin->>Keychain: SecItemCopyMatching -> retrieve data
Keychain-->>iOSPlugin: value
iOSPlugin-->>CapacitorBridge: return value
CapacitorBridge-->>WebApp: token returned
WebApp->>Server: use Bearer token for API calls
end
User->>WebApp: logout
WebApp->>CapacitorBridge: PageSpaceKeychain.remove(sessionKey)
CapacitorBridge->>iOSPlugin: invoke remove(...)
iOSPlugin->>Keychain: SecItemDelete
Keychain-->>iOSPlugin: success
iOSPlugin-->>CapacitorBridge: resolved
CapacitorBridge-->>WebApp: removal confirmed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ios/src/auth-bridge.ts (1)
34-59: Persist migration failure flag to avoid repeated retries on next launch.
In the catch path you only set the in-memorymigrationComplete.MIGRATED_KEYstays unset, so the migration will rerun after app restart and can spam logs if Keychain access is permanently failing. Consider persisting the flag (or a separate failure flag) in the catch path.💡 Suggested fix
} catch (error) { // Log error but don't crash - user can still log in fresh console.error('[PageSpace iOS] Keychain migration failed:', error); // Mark as migrated to prevent retry loop - migrationComplete = true; + try { + await Preferences.set({ key: MIGRATED_KEY, value: 'true' }); + } catch (prefError) { + console.error('[PageSpace iOS] Failed to persist migration flag:', prefError); + } + migrationComplete = true; }
🤖 Fix all issues with AI agents
In `@apps/ios/ios/App/App.xcodeproj/project.pbxproj`:
- Line 319: The project currently hardcodes DEVELOPMENT_TEAM = M96WTV3CKX in
project.pbxproj which will break builds for other contributors; remove the
hardcoded DEVELOPMENT_TEAM entries and move the setting into an xcconfig (e.g.,
add DEVELOPMENT_TEAM = $(DEVELOPMENT_TEAM) or set the actual team in a new
local.xcconfig or extend debug.xcconfig), update the Xcode project
target/project configuration to use that xcconfig, add the local xcconfig to
.gitignore, and update README or docs to instruct developers to create their own
local.xcconfig with their Apple team ID if needed; refer to the DEVELOPMENT_TEAM
key and the project configuration in project.pbxproj when making the changes.
In `@apps/ios/ios/App/App/Plugins/PageSpaceKeychainPlugin.swift`:
- Around line 39-66: The set(_ call: CAPPluginCall) method force-unwraps
value.data(using: .utf8) which can crash; replace the force unwrap with a safe
optional binding (e.g., guard let data = value.data(using: .utf8) else {
call.reject("Failed to encode value as UTF-8"); return }) and use that data
variable when building addQuery so the call gracefully rejects instead of
crashing if encoding fails; update the references to data in SecItemAdd
accordingly.
In `@apps/ios/ios/App/CapApp-SPM/README.md`:
- Line 3: Fix the grammar typo in the README sentence "This SPM is used to host
SPM dependencies for you Capacitor project" by replacing "you" with "your" so it
reads "This SPM is used to host SPM dependencies for your Capacitor project"
(update the README.md content where that sentence appears).
In `@apps/ios/package.json`:
- Around line 15-25: Update the dependency version constraints in
apps/ios/package.json so they require newer secure minima: bump all `@capacitor/`*
packages from ^7.0.0 to at least ^7.0.3 (or a specific patched release like
^7.4.5 for `@capacitor/core`) and set `@capgo/capacitor-social-login` to ^7.20.0;
after changing the version strings for `@capacitor/app`, `@capacitor/browser`,
`@capacitor/core`, `@capacitor/ios`, `@capacitor/keyboard`, `@capacitor/preferences`,
`@capacitor/splash-screen`, `@capacitor/status-bar` and
`@capgo/capacitor-social-login`, run your package manager (npm/pnpm) to update the
lockfile, run npm audit / pnpm audit and resolve any remaining vulnerabilities,
and then run the iOS build/tests to verify no breaking changes.
In `@apps/web/src/lib/ios-google-auth.ts`:
- Around line 165-176: clearStoredSession only removes pagespace_session from
the iOS Keychain which leaves pagespace_csrf stale; update the function
(clearStoredSession) to also remove the pagespace_csrf entry via
PageSpaceKeychain.remove (call remove for both 'pagespace_session' and
'pagespace_csrf', e.g., sequentially or with Promise.all) so it matches
apps/ios/src/auth-bridge.ts behavior and still wraps the calls in the existing
try/catch.
🧹 Nitpick comments (1)
apps/web/src/app/auth/signin/page.tsx (1)
103-121: Prefer clearingauthFailedPermanentlyvia the store API instead of manual localStorage edits.
This avoids tight coupling to the persisted JSON shape and keeps persistence logic centralized.♻️ Proposed refactor
-import { useAuth } from "@/hooks/useAuth"; +import { useAuth } from "@/hooks/useAuth"; +import { useAuthStore } from "@/stores/useAuthStore"; @@ - // Clear authFailedPermanently flag directly in localStorage - // This prevents dashboard from skipping auth validation - try { - const stored = localStorage.getItem('auth-storage'); - if (stored) { - const parsed = JSON.parse(stored); - if (parsed.state?.authFailedPermanently) { - parsed.state.authFailedPermanently = false; - localStorage.setItem('auth-storage', JSON.stringify(parsed)); - } - } - } catch { - // Ignore localStorage errors - } + // Clear authFailedPermanently via the store (persists safely) + try { + useAuthStore.setState({ authFailedPermanently: false }); + } catch { + // Ignore storage errors + }
Relative URLs with window.location.href don't navigate reliably in Capacitor iOS WKWebView. Using window.location.origin ensures explicit navigation to the dashboard after Google Sign-In. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Owner
Author
Code reviewFound 1 issue:
PageSpace/apps/web/src/app/auth/signin/page.tsx Lines 102 to 117 in d55ed2a The useAuthStore.getState().setAuthFailedPermanently(false);🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
The native Google OAuth route was returning the session token as JSON but not setting it as a cookie. This caused the middleware to redirect to /auth/signin because it couldn't find the session cookie. Changes: - Set session cookie via appendSessionCookie in native OAuth response - Return user info from native OAuth for immediate auth state update - Use router.replace() instead of window.location.href for navigation - Set auth store state directly to prevent race conditions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…e manipulation Replace direct localStorage parsing/writing with the existing setAuthFailedPermanently() store action for clearing the auth failure flag after successful native Google Sign-In. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Clear CSRF token on logout in ios-google-auth.ts - Fix force unwrap crash in PageSpaceKeychainPlugin.swift - Persist migration failure flag to prevent retry loops - Update Capacitor dependencies to fix vulnerabilities - Move DEVELOPMENT_TEAM to xcconfig for contributor builds - Fix typo in CapApp-SPM README Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/ios/ios/App/App/Plugins/PageSpaceKeychainPlugin.swift`:
- Around line 55-62: The keychain entry currently uses
kSecAttrAccessibleAfterFirstUnlock which allows backup/restore; update the
addQuery dictionary to use a device-only accessibility constant (e.g., set
kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly) for
auth/session tokens, or choose kSecAttrAccessibleWhenPasscodeSetThisDeviceOnly
if you want to restrict to devices with a passcode; change the value in the
addQuery assignment where kSecAttrAccessible is set so the item is not
recoverable via iCloud backups.
♻️ Duplicate comments (1)
apps/ios/ios/debug.xcconfig (1)
1-5: Hardcoded DEVELOPMENT_TEAM still risks contributor build failures.
The comment suggests a local override, butlocal.xcconfigisn’t included, so the shared team ID remains the default. Consider making the local override optional and removing the hardcoded ID.🛠️ Suggested tweak
+#include? "local.xcconfig" CAPACITOR_DEBUG = true // Development team - contributors should override this in their local environment // or create a local.xcconfig file that is git-ignored -DEVELOPMENT_TEAM = M96WTV3CKX +DEVELOPMENT_TEAM =
🧹 Nitpick comments (2)
apps/web/src/lib/ios-google-auth.ts (2)
96-110: Avoid implicitanyfromresponse.json(); type the payload before use.
This aligns with the “noany” guideline and makes field access safer.♻️ Proposed typing tweak
export interface GoogleAuthResult { success: boolean; error?: string; isNewUser?: boolean; user?: { id: string; name: string | null; email: string | null; image?: string | null; }; } + +type GoogleNativeAuthResponse = { + sessionToken?: string; + csrfToken?: string | null; + isNewUser?: boolean; + user?: GoogleAuthResult['user']; +}; @@ - const { sessionToken, csrfToken, isNewUser, user } = await response.json(); + const payload = (await response.json()) as GoogleNativeAuthResponse; + const { sessionToken, csrfToken, isNewUser, user } = payload;
143-169: Validate parsed Keychain payload to avoid implicitanyand malformed sessions.
JSON.parsecurrently returnsany; a small guard keeps the return type reliable.♻️ Proposed safe parse
-export async function getStoredSession(): Promise<{ - sessionToken: string; - csrfToken: string | null; - deviceId: string; -} | null> { +type StoredSession = { + sessionToken: string; + csrfToken: string | null; + deviceId: string; +}; + +export async function getStoredSession(): Promise<StoredSession | null> { @@ - return JSON.parse(value); + const parsed = JSON.parse(value) as Partial<StoredSession>; + if (typeof parsed.sessionToken !== 'string' || typeof parsed.deviceId !== 'string') { + return null; + } + return { + sessionToken: parsed.sessionToken, + csrfToken: parsed.csrfToken ?? null, + deviceId: parsed.deviceId, + };
- Use kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly for device-bound auth tokens - Add GoogleNativeAuthResponse type for response.json() to avoid implicit any - Add StoredSession type and runtime validation in getStoredSession() - Use optional include for local.xcconfig and clear hardcoded DEVELOPMENT_TEAM - Add local.xcconfig to .gitignore for contributor flexibility Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…for external Internal @mention links and other internal navigation now use router.push() to stay within the Capacitor WebView instead of opening Safari externally. External links use Capacitor's Browser plugin which opens Safari View Controller (an in-app browser) keeping users in the app context. Changes: - Add navigation utilities (app-navigation.ts) for platform-aware navigation - Add AppLink component for mobile-aware link handling - Update StreamingMarkdown to use router.push() for mention links - Update MessagePartRenderer to use router.push() for mentions - Remove target="_blank" from TipTap mentions, use navigation events - Update CanvasPageView to use openExternalUrl() for external links - Add safe area insets to Sheet component for iOS notch Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…for external Internal @mention links and other internal navigation now use router.push() to stay within the Capacitor WebView instead of opening Safari externally. External links use Capacitor's Browser plugin which opens Safari View Controller (an in-app browser) keeping users in the app context. Changes: - Add navigation utilities (app-navigation.ts) for platform-aware navigation - Add AppLink component for mobile-aware link handling - Update StreamingMarkdown to use router.push() for mention links - Update MessagePartRenderer to use router.push() for mentions - Remove target="_blank" from TipTap mentions, use navigation events - Update CanvasPageView to use openExternalUrl() for external links - Add safe area insets to Sheet component for iOS notch Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/lib/editor/tiptap-mention-config.tsx`:
- Around line 176-183: The comment claims a fallback to window.location exists,
but the click handler only calls dispatchInternalNavigation(href) and does not
set window.location; update the comment above dom.addEventListener(...) to
remove or correct the "Falls back to window.location for non-React contexts"
text, leaving a concise note that the handler dispatches an internal navigation
event (via dispatchInternalNavigation) for parent components/routers to handle
and does not perform a direct navigation itself.
🧹 Nitpick comments (7)
apps/web/src/lib/ios-google-auth.ts (1)
109-128: Consider validating theuserobject from the backend response.The
sessionTokenis validated (line 112), but theuserobject is returned directly without structure validation. If the backend returns a malformed user object, it could propagate unexpected data types to consumers.This is a minor robustness concern since the backend is trusted, but adding defensive validation would align with the pattern used in
getStoredSession.♻️ Optional: Add user validation
const { sessionToken, csrfToken, isNewUser, user } = (await response.json()) as GoogleNativeAuthResponse; if (!sessionToken) { throw new Error('No session token received from server'); } + // Validate user object structure if present + const validatedUser = user && typeof user.id === 'string' ? user : undefined; + // Store tokens in iOS Keychain via PageSpaceKeychain plugin await PageSpaceKeychain.set({ key: 'pagespace_session', value: JSON.stringify({ sessionToken, csrfToken: csrfToken || null, deviceId, }), }); console.log('[iOS Google Auth] Sign-in successful, tokens stored'); - return { success: true, isNewUser, user }; + return { success: true, isNewUser, user: validatedUser };apps/web/src/lib/navigation/app-navigation.ts (2)
37-51: Consider logging errors in the catch block for debugging.The silent catch hides potential issues that could help debug problems in Capacitor environments (e.g., plugin misconfiguration). Even with a fallback, knowing why it failed is valuable.
🔧 Suggested improvement
} catch { // Fallback if Browser plugin isn't available + console.warn('[openExternalUrl] Capacitor Browser plugin unavailable, falling back to window.open'); window.open(url, '_blank', 'noopener,noreferrer'); }
57-59:navigateInternalwrapper adds minimal value.This function simply delegates to
routerPush. Consider removing it or adding value (e.g., URL validation, analytics). Currently, callers could just userouterPushdirectly.apps/web/src/components/messages/MessagePartRenderer.tsx (1)
48-58: Consider extracting the shared mention link rendering.The mention link rendering (lines 48-58 and 102-112) is duplicated between
textandrich-textcases. A shared helper could reduce duplication.♻️ Possible extraction
const renderPageMentionLink = (key: string, id: string, label: string) => ( <a key={key} href={`/p/${id}`} onClick={(e) => handleMentionClick(e, id)} className="bg-primary/10 text-primary dark:bg-primary/20 dark:text-primary px-1 rounded hover:bg-primary/20 dark:hover:bg-primary/30 transition-colors no-underline" > @{label} </a> );apps/web/src/components/ai/shared/chat/StreamingMarkdown.tsx (2)
142-153: Minor: Props spread order could override onClick.The
{...props}spread is placed afteronClick, which means ifpropscontains anonClick, it would override your handler. Consider moving the spread beforeonClickto ensure your handler always takes precedence.Suggested reorder
return ( <a href={pageHref} + {...props} onClick={(e) => { e.preventDefault(); router.push(pageHref); }} className="inline-flex items-center px-2 py-0.5 rounded-md bg-primary/20 text-primary dark:bg-primary/30 dark:text-primary text-sm font-medium mx-1 hover:bg-primary/30 dark:hover:bg-primary/40 transition-colors cursor-pointer no-underline" - {...props} > @{label} </a> );
165-174: Minor: Same props spread order concern for regular links.Similar to the page mention anchor, the
{...props}spread afteronClickcould override the click handler if props contains one.Suggested reorder
return ( <a href={href} + {...props} onClick={handleClick} className="min-w-0 max-w-full break-all [overflow-wrap:anywhere] inline-block" - {...props} > {children} </a> );apps/ios/ios/App/App/Plugins/PageSpaceKeychainPlugin.swift (1)
16-37: Handle UTF‑8 decode failures explicitly.This avoids reporting a “success” OSStatus as a generic failure if the stored value isn’t valid UTF‑8.
💡 Suggested tweak
- if status == errSecSuccess, let data = result as? Data, - let value = String(data: data, encoding: .utf8) { - call.resolve(["value": value]) - } else if status == errSecItemNotFound { - call.resolve(["value": NSNull()]) - } else { - call.reject("Keychain read failed: \(status)") - } + if status == errSecSuccess, let data = result as? Data { + if let value = String(data: data, encoding: .utf8) { + call.resolve(["value": value]) + } else { + call.reject("Keychain value is not valid UTF-8") + } + } else if status == errSecItemNotFound { + call.resolve(["value": NSNull()]) + } else { + call.reject("Keychain read failed: \(status)") + }
Comment on lines
+176
to
183
| // Handle click events - dispatch navigation event for React router handling | ||
| // Falls back to window.location for non-React contexts | ||
| dom.addEventListener('click', (event) => { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| window.open(href, '_blank', 'noopener,noreferrer'); | ||
| // Dispatch event for parent component to handle with router.push() | ||
| dispatchInternalNavigation(href); | ||
| }); |
Contributor
There was a problem hiding this comment.
Misleading comment: no fallback is implemented.
The comment on line 177 mentions "Falls back to window.location for non-React contexts" but no such fallback exists in the code. The event is dispatched and nothing else happens if no listener catches it.
🔧 Suggested fix - either add fallback or update comment
Option 1 - Update comment:
- // Handle click events - dispatch navigation event for React router handling
- // Falls back to window.location for non-React contexts
+ // Handle click events - dispatch navigation event for React router handling
+ // Parent components must subscribe to 'pagespace:navigate' events
dom.addEventListener('click', (event) => {Option 2 - Add actual fallback:
dom.addEventListener('click', (event) => {
event.preventDefault();
event.stopPropagation();
// Dispatch event for parent component to handle with router.push()
- dispatchInternalNavigation(href);
+ const handled = dispatchInternalNavigation(href);
+ if (!handled) {
+ window.location.href = href;
+ }
});📝 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
| // Handle click events - dispatch navigation event for React router handling | |
| // Falls back to window.location for non-React contexts | |
| dom.addEventListener('click', (event) => { | |
| event.preventDefault(); | |
| event.stopPropagation(); | |
| window.open(href, '_blank', 'noopener,noreferrer'); | |
| // Dispatch event for parent component to handle with router.push() | |
| dispatchInternalNavigation(href); | |
| }); | |
| // Handle click events - dispatch navigation event for React router handling | |
| // Parent components must subscribe to 'pagespace:navigate' events | |
| dom.addEventListener('click', (event) => { | |
| event.preventDefault(); | |
| event.stopPropagation(); | |
| // Dispatch event for parent component to handle with router.push() | |
| dispatchInternalNavigation(href); | |
| }); |
🤖 Prompt for AI Agents
In `@apps/web/src/lib/editor/tiptap-mention-config.tsx` around lines 176 - 183,
The comment claims a fallback to window.location exists, but the click handler
only calls dispatchInternalNavigation(href) and does not set window.location;
update the comment above dom.addEventListener(...) to remove or correct the
"Falls back to window.location for non-React contexts" text, leaving a concise
note that the handler dispatches an internal navigation event (via
dispatchInternalNavigation) for parent components/routers to handle and does not
perform a direct navigation itself.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/components/ai/shared/chat/StreamingMarkdown.tsx`:
- Around line 112-173: handleClick currently always calls e.preventDefault()
which blocks modifier/middle clicks and the inline onClick in the mention page
link also unconditionally prevents default; update both to only prevent default
and call router.push for primary left-clicks without modifiers (check e.button
=== 0 and !e.metaKey && !e.ctrlKey && !e.shiftKey && !e.altKey) so
Cmd/Ctrl/Shift/middle-click still work and open-in-new-tab behavior is
preserved; also move {...props} before explicit onClick/className (or explicitly
compose handlers) so external props cannot override the component's onClick and
className—apply these changes to the handleClick function and the mention page
link's onClick handler.
Comment on lines
+112
to
+173
| const handleClick = async (e: MouseEvent<HTMLAnchorElement>) => { | ||
| if (!href) return; | ||
|
|
||
| e.preventDefault(); | ||
|
|
||
| if (isInternalUrl(href)) { | ||
| router.push(href); | ||
| } else { | ||
| await openExternalUrl(href); | ||
| } | ||
| }; | ||
|
|
||
| // Check if this is a mention link | ||
| if (typeof href === 'string' && href.startsWith('mention://')) { | ||
| // Extract the label from children (format: "mention:Label") | ||
| const label = typeof children === 'string' | ||
| ? children.replace(/^mention:/, '') | ||
| : Array.isArray(children) && typeof children[0] === 'string' | ||
| ? children[0].replace(/^mention:/, '') | ||
| : children; | ||
|
|
||
| // Parse the mention URL: mention://id/type | ||
| const mentionPath = href.replace('mention://', ''); | ||
| const [id, type] = mentionPath.split('/'); | ||
|
|
||
| // Only page mentions should be clickable links | ||
| // User and other mention types render as non-clickable badges | ||
| if (type === 'page') { | ||
| const pageHref = `/p/${id}`; | ||
| return ( | ||
| <a | ||
| href={pageHref} | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| router.push(pageHref); | ||
| }} | ||
| className="inline-flex items-center px-2 py-0.5 rounded-md bg-primary/20 text-primary dark:bg-primary/30 dark:text-primary text-sm font-medium mx-1 hover:bg-primary/30 dark:hover:bg-primary/40 transition-colors cursor-pointer no-underline" | ||
| {...props} | ||
| > | ||
| @{label} | ||
| </a> | ||
| ); | ||
| } | ||
|
|
||
| // Non-page mentions (user, agent, etc.) render as styled badges without links | ||
| return ( | ||
| <span className="inline-flex items-center px-2 py-0.5 rounded-md bg-primary/20 text-primary dark:bg-primary/30 dark:text-primary text-sm font-medium mx-1"> | ||
| @{label} | ||
| </span> | ||
| ); | ||
| } | ||
|
|
||
| // Regular link with mobile-aware click handling | ||
| return ( | ||
| <a | ||
| href={href} | ||
| onClick={handleClick} | ||
| className="min-w-0 max-w-full break-all [overflow-wrap:anywhere] inline-block" | ||
| {...props} | ||
| > | ||
| {children} | ||
| </a> |
Contributor
There was a problem hiding this comment.
Preserve modifier/middle‑click behavior and avoid prop overrides.
preventDefault() runs for all clicks, which blocks Cmd/Ctrl/Shift/middle‑click and can break expected “open in new tab” behavior. Also, {...props} after onClick/className can override the handler and styles, bypassing the mobile‑aware routing.
🛠️ Suggested fix
function createCustomAnchor(router: RouterLike) {
return function CustomAnchor({ href, children, ...props }: AnchorHTMLAttributes<HTMLAnchorElement> & { children?: ReactNode }) {
+ const { className: classNameProp, onClick: onClickProp, target, ...rest } = props;
const handleClick = async (e: MouseEvent<HTMLAnchorElement>) => {
if (!href) return;
+ if (
+ e.defaultPrevented ||
+ e.button !== 0 ||
+ e.metaKey ||
+ e.ctrlKey ||
+ e.shiftKey ||
+ e.altKey ||
+ target === '_blank'
+ ) {
+ return;
+ }
+
e.preventDefault();
if (isInternalUrl(href)) {
router.push(href);
} else {
await openExternalUrl(href);
}
};
@@
return (
<a
- href={href}
- onClick={handleClick}
- className="min-w-0 max-w-full break-all [overflow-wrap:anywhere] inline-block"
- {...props}
+ {...rest}
+ href={href}
+ target={target}
+ onClick={(e) => {
+ onClickProp?.(e);
+ if (!e.defaultPrevented) void handleClick(e);
+ }}
+ className={`min-w-0 max-w-full break-all [overflow-wrap:anywhere] inline-block ${classNameProp ?? ''}`}
>
{children}
</a>
);
};
}🤖 Prompt for AI Agents
In `@apps/web/src/components/ai/shared/chat/StreamingMarkdown.tsx` around lines
112 - 173, handleClick currently always calls e.preventDefault() which blocks
modifier/middle clicks and the inline onClick in the mention page link also
unconditionally prevents default; update both to only prevent default and call
router.push for primary left-clicks without modifiers (check e.button === 0 and
!e.metaKey && !e.ctrlKey && !e.shiftKey && !e.altKey) so
Cmd/Ctrl/Shift/middle-click still work and open-in-new-tab behavior is
preserved; also move {...props} before explicit onClick/className (or explicitly
compose handlers) so external props cannot override the component's onClick and
className—apply these changes to the handleClick function and the mention page
link's onClick handler.
…missions - Fix @capacitor/browser import in app-navigation.ts by using a variable for the module path with @vite-ignore comment, preventing Vite's static import analysis from failing during tests - Fix usePermissions.test.ts mock path from '../use-auth' to '@/hooks/useAuth' - Fix usePermissions test timing by separating onSuccess callback invocation from SWR mock to respect useEffect execution order Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/lib/navigation/app-navigation.ts`:
- Around line 18-23: The isInternalUrl function currently uses
url.startsWith(window.location.origin) which is vulnerable to URL confusion;
update isInternalUrl to: treat empty/relative paths as before, then (after
confirming typeof window !== 'undefined') construct a URL object via new
URL(url, window.location.origin) inside a try/catch and compare the resulting
urlObj.origin strictly to window.location.origin (instead of startsWith); return
true only when origins match, and return false on URL constructor errors —
reference the isInternalUrl function and window.location.origin when making the
change.
🧹 Nitpick comments (2)
apps/web/src/hooks/__tests__/usePermissions.test.ts (1)
37-38: Consider scopingcapturedConfigwithin each test.The shared mutable variable at describe-level works in sequential execution since each mock overwrites it, but could lead to subtle issues if tests run in parallel or if a test fails before setting it. Moving the declaration inside each test improves isolation.
♻️ Suggested refactor
describe('SWR editing protection', () => { - let capturedConfig: { isPaused?: () => boolean; onSuccess?: (data: unknown, key: string, config: never) => void }; - it('given user is editing a document, should pause permission revalidation', () => { + let capturedConfig: { isPaused?: () => boolean; onSuccess?: (data: unknown, key: string, config: never) => void }; // Arrange: User is actively editingApply the same pattern to the other two tests.
apps/web/src/lib/navigation/app-navigation.ts (1)
37-52: Consider logging the error for debugging.Silent error handling makes it harder to diagnose issues when the Capacitor Browser plugin fails unexpectedly. A debug log would help during development.
💡 Optional: Add error logging
} catch { // Fallback if Browser plugin isn't available + console.debug('[openExternalUrl] Browser plugin unavailable, using window.open fallback'); window.open(url, '_blank', 'noopener,noreferrer'); }
Comment on lines
+18
to
+23
| export function isInternalUrl(url: string): boolean { | ||
| if (!url) return false; | ||
| if (url.startsWith('/')) return true; | ||
| if (typeof window !== 'undefined' && url.startsWith(window.location.origin)) return true; | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
URL origin check is vulnerable to URL confusion attacks.
The check url.startsWith(window.location.origin) will incorrectly classify https://example.com.evil.com as internal when the origin is https://example.com. This could allow external URLs to bypass the intended navigation behavior.
🔒 Proposed fix to validate URL boundary
export function isInternalUrl(url: string): boolean {
if (!url) return false;
if (url.startsWith('/')) return true;
- if (typeof window !== 'undefined' && url.startsWith(window.location.origin)) return true;
+ if (typeof window !== 'undefined') {
+ const origin = window.location.origin;
+ if (url === origin || url.startsWith(origin + '/')) return true;
+ }
return false;
}🤖 Prompt for AI Agents
In `@apps/web/src/lib/navigation/app-navigation.ts` around lines 18 - 23, The
isInternalUrl function currently uses url.startsWith(window.location.origin)
which is vulnerable to URL confusion; update isInternalUrl to: treat
empty/relative paths as before, then (after confirming typeof window !==
'undefined') construct a URL object via new URL(url, window.location.origin)
inside a try/catch and compare the resulting urlObj.origin strictly to
window.location.origin (instead of startsWith); return true only when origins
match, and return false on URL constructor errors — reference the isInternalUrl
function and window.location.origin when making the change.
5 tasks
This was referenced Feb 16, 2026
Merged
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.
Summary
Changes
1. Swift Package Manager Migration
2. Custom PageSpaceKeychain Plugin
3. Google Sign-In Fixes
forcePrompt: trueto always show account picker (prevents auto-login with cached credentials)window.location.hreffor navigation (fixes router.replace failure in Capacitor WebView)4. Configuration Updates
Test plan
pnpm --filter web build🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes
Removed
Chores
✏️ Tip: You can customize this high-level summary in your review settings.