Conversation
WalkthroughAdded basic-ssl plugin and enabled HTTPS in Vite; updated React Native platform detection to prefer URL param and persist only when URL-provided; expanded Pillar Wallet messaging to support RN-style Changes
Sequence Diagram(s)sequenceDiagram
participant URL as URL params
participant Storage as localStorage
participant Main as Main.tsx
participant App as App
Main->>URL: read devicePlatformFromUrl
Main->>Storage: read devicePlatformFromStorage
alt devicePlatformFromUrl present
Main->>Main: resolved = devicePlatformFromUrl
Main->>Storage: write DEVICE_PLATFORM
else
Main->>Main: resolved = devicePlatformFromStorage
end
Main->>App: pass resolved devicePlatform
Main->>Main: log devicePlatformFromUrl, devicePlatformFromStorage, resolved
sequenceDiagram
participant RNWebview as RN Webview
participant Document as document
participant Window as window (custom)
participant Handler as message handler
RNWebview->>Document: dispatch 'message' (string payload)
Document->>Document: parse payload
Document->>Handler: forward parsed message (e.g., pillarWalletPkResponse)
RNWebview->>Window: emit custom event 'pillarWalletMessage' (detail payload)
Window->>Window: parse event.detail
Window->>Handler: forward parsed message
Handler->>Handler: invoke onSuccess / onError (no change to core success path)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.39.6)src/utils/pillarWalletMessaging.ts[ ... [truncated 5313 characters] ... "style": "secondary" 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/pillarWalletMessaging.ts (1)
244-246: Silent error handling may complicate debugging.The empty catch blocks intentionally ignore JSON parsing errors. While this prevents non-JSON messages from breaking the handler, it makes debugging harder when legitimate messages fail to parse.
Consider logging parse failures at a lower severity level:
} catch (e) { - // Ignore parsing errors + // Non-JSON message, ignore + Sentry.addBreadcrumb({ + category: 'authentication', + message: 'Non-JSON message ignored in document handler', + level: 'debug', + }); }Also applies to: 266-268
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/containers/Main.tsx(3 hunks)src/utils/pillarWalletMessaging.ts(3 hunks)vite.config.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T10:31:06.760Z
Learnt from: IAmKio
PR: pillarwallet/x#432
File: src/containers/Main.tsx:135-141
Timestamp: 2025-10-15T10:31:06.760Z
Learning: In src/containers/Main.tsx, the React Native webview messaging setup intentionally only activates when the `devicePlatform` URL parameter is present ('ios' or 'android'). On reloads or direct navigation without URL params, the site should behave normally and not attempt to re-establish RN messaging. This is by design to ensure the site functions properly in both RN webview and standard browser contexts.
Applied to files:
src/containers/Main.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: unit-tests
🔇 Additional comments (10)
vite.config.js (2)
6-6: LGTM! SSL plugin correctly integrated.The @vitejs/plugin-basic-ssl import and plugin registration are correctly implemented.
Also applies to: 9-9
29-32: Note: Dev server exposed on all network interfaces.Setting
host: '0.0.0.0'makes the HTTPS dev server accessible from any network interface. While this is necessary for testing with mobile devices and React Native webviews, ensure this configuration is only used in development environments and not in production.src/containers/Main.tsx (3)
138-144: Verify: Platform detection behavior change may conflict with documented approach.The code now uses localStorage as a fallback when the URL parameter is absent (
devicePlatform = devicePlatformFromUrl || devicePlatformFromStorage). This means React Native messaging will activate on page reloads even without the URL parameter, as long as localStorage contains the value.According to retrieved learnings, "the React Native webview messaging setup intentionally only activates when the
devicePlatformURL parameter is present" and "on reloads or direct navigation without URL params, the site should behave normally and not attempt to re-establish RN messaging."The new behavior appears to contradict this design. If this is an intentional enhancement (consistent with the PR's goal of "enhanced messaging transport"), please confirm and update the documentation. Based on learnings.
151-152: Good addition: Enhanced logging visibility.Including both
devicePlatformFromUrlanddevicePlatformFromStoragein the breadcrumb improves debugging and visibility into the platform detection logic.
161-163: Note the conditional persistence behavior.localStorage is now only updated when
devicePlatformFromUrlexists, preventing unconditional writes of the resolved platform value. This ensures the stored value always originates from an explicit URL parameter.src/utils/pillarWalletMessaging.ts (4)
100-100: Good enhancement: Raw data logging.Adding
rawData: event.datato the breadcrumb improves debugging capabilities by preserving the original message payload.
229-248: Broader compatibility but note the non-standard API usage.The
documentMessageHandlerexpands React Native webview message compatibility. However,document.addEventListener('message', ...)(line 252) is non-standard and may not work consistently across all React Native webview implementations.Consider logging when these alternative handlers successfully receive messages to confirm they're actually being used in your target environments:
if (data?.type === 'pillarWalletPkResponse') { + Sentry.addBreadcrumb({ + category: 'authentication', + message: 'Message received via document event handler', + level: 'info', + data: { authRequestId }, + }); // Create a proper MessageEvent and call our handler const messageEvent = new MessageEvent('message', { data: event.data, }); messageHandler(messageEvent); }
250-275: Custom event handler adds another compatibility layer.The
customEventHandlerlistening for'pillarWalletMessage'events provides an additional message channel. This is a reasonable approach for environments where standard messaging doesn't work.
283-290: Cleanup correctly removes all listeners.The cleanup function properly removes all three event listeners (window message, document message, and custom event) and clears the timeout, preventing memory leaks.
package.json (1)
114-114: No issues found—package version is valid and has no known security vulnerabilities.Version 2.1.0 of @vitejs/plugin-basic-ssl is a valid published version, and there are no known security vulnerabilities for this package. The dependency as specified is appropriate.
github-actions
bot
temporarily deployed
to
Preview (fix/rn-messaging-layer)
Deploying x with
|
| Latest commit: |
94a4cec
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://87deba03.x-e62.pages.dev |
| Branch Preview URL: | https://fix-rn-messaging-layer.x-e62.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/pillarWalletMessaging.ts (2)
46-55: Document interface augmentation is non-standard - consider documenting the constraint.The
Documentinterface augmentation for'message'events is specific to React Native webview behavior and deviates from standard Web APIs. While this enables the necessary functionality, consider adding a JSDoc comment explaining this is a React Native-specific workaround to prevent confusion for future maintainers.Apply this diff to add clarifying documentation:
+ /** + * React Native webview-specific augmentation. + * Standard Document interface does not support 'message' events. + * This augmentation enables document-level message handling in RN webviews. + */ interface Document { addEventListener( type: 'message', listener: (event: MessageEvent) => void ): void; removeEventListener( type: 'message', listener: (event: MessageEvent) => void ): void; }
240-286: Refactor duplicated message forwarding logic.The
documentMessageHandler(lines 240-259) andcustomEventHandler(lines 265-286) contain nearly identical logic: parse stringified data, check forpillarWalletPkResponsetype, create a MessageEvent, and forward to the main handler. This duplication increases maintenance burden.Extract the common pattern into a helper function:
+ // Helper to forward messages to the main handler + const forwardMessage = (rawData: string) => { + try { + const data = JSON.parse(rawData); + if (data?.type === 'pillarWalletPkResponse') { + const messageEvent = new MessageEvent('message', { data: rawData }); + messageHandler(messageEvent); + } + } catch (e) { + // Ignore parsing errors - message not intended for us + } + }; + // For React Native webview, we also need to listen for direct document events // React Native webview messages sometimes come through document events const documentMessageHandler = (event: MessageEvent) => { - // Check if this is a React Native webview message if (event.data && typeof event.data === 'string') { - try { - const data = JSON.parse(event.data); - - if (data?.type === 'pillarWalletPkResponse') { - // Create a proper MessageEvent and call our handler - const messageEvent = new MessageEvent('message', { - data: event.data, - }); - messageHandler(messageEvent); - } - } catch (e) { - // Ignore parsing errors - } + forwardMessage(event.data); } }; // Listen for document events (React Native webview specific) // Note: document.addEventListener('message') is not standard, but some React Native webviews use it document.addEventListener('message', documentMessageHandler); // Also listen for custom events that might be triggered by React Native const customEventHandler = (event: CustomEvent) => { if (event.detail && typeof event.detail === 'string') { - try { - const data = JSON.parse(event.detail); - - if (data?.type === 'pillarWalletPkResponse') { - const messageEvent = new MessageEvent('message', { - data: event.detail, - }); - messageHandler(messageEvent); - } - } catch (e) { - // Ignore parsing errors - } + forwardMessage(event.detail); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/containers/Main.tsx(3 hunks)src/utils/pillarWalletMessaging.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/containers/Main.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (1)
src/utils/pillarWalletMessaging.ts (1)
294-302: LGTM: Cleanup properly removes all event listeners.The cleanup function correctly removes all three event listeners (window message, document message, and custom pillarWalletMessage) and clears the timeout, ensuring no memory leaks from lingering event handlers.
github-actions
bot
temporarily deployed
to
Preview (fix/rn-messaging-layer)
| }, | ||
| server: { | ||
| https: true, | ||
| host: '0.0.0.0', |
There was a problem hiding this comment.
do we want the local host to always be 0.0.0.0? Or leave it as it was before?
src/utils/pillarWalletMessaging.ts
Outdated
| messageHandler(messageEvent); | ||
| } | ||
| } catch (e) { | ||
| // Ignore parsing errors |
There was a problem hiding this comment.
Are we sure we do not want to add anything in the catch?
src/utils/pillarWalletMessaging.ts
Outdated
| messageHandler(messageEvent); | ||
| } | ||
| } catch (e) { | ||
| // Ignore parsing errors |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/pillarWalletMessaging.ts (1)
121-202: Implement deduplication to prevent multiple onSuccess invocations.Three event listeners (
window.addEventListener('message'),document.addEventListener('message'),window.addEventListener('pillarWalletMessage')) handle the same incoming message concurrently. A single RN webview message can trigger all three listeners, causingonSuccessto be called multiple times with identical credentials.While the previous review marked deduplication as "addressed," the current code only removed Sentry logging (which eliminated duplicate log entries) but did not implement actual message deduplication. The
onSuccesscallback can still be invoked multiple times.Add a deduplication guard using one of these approaches:
Option 1: Single-use flag (simplest):
export const setupPillarWalletMessaging = ( onSuccess: OnPrivateKeyReceivedCallback, onError: OnErrorCallback ): (() => void) => { let processed = false; const wrappedOnSuccess: OnPrivateKeyReceivedCallback = (address, privateKey) => { if (processed) return; processed = true; // Immediately remove all listeners cleanup(); onSuccess(address, privateKey); }; const messageHandler = createWebViewMessageHandler(wrappedOnSuccess, onError); // ... rest of setupOption 2: Track by message content (more robust):
const processedMessages = new Set<string>(); const wrappedOnSuccess: OnPrivateKeyReceivedCallback = (address, privateKey) => { const messageKey = `${address}:${privateKey.slice(0, 10)}`; if (processedMessages.has(messageKey)) return; processedMessages.add(messageKey); onSuccess(address, privateKey); };Option 1 is recommended for single authentication flows.
♻️ Duplicate comments (1)
src/utils/pillarWalletMessaging.ts (1)
172-178: Same infinite retry loop issue as documentMessageHandler.This catch block has the identical unbounded retry logic as the
documentMessageHandler(lines 146-152). Apply the same fix to prevent infinite retries on persistent parse errors.
🧹 Nitpick comments (1)
src/utils/pillarWalletMessaging.ts (1)
60-73: Consider reintroducing request correlation for better tracking.Removing the
authRequestIdparameter eliminates the ability to correlate requests with responses. While this simplifies the API, it makes it impossible to:
- Implement request/response matching for deduplication
- Debug which request triggered which response
- Handle concurrent authentication flows
If only one authentication flow is active at a time, this is acceptable. Otherwise, consider reintroducing a request ID mechanism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/pillarWalletMessaging.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (3)
src/utils/pillarWalletMessaging.ts (3)
44-54: Verify runtime support for non-standard Document 'message' event.The Document interface augmentation enables TypeScript compilation, but
document.addEventListener('message')is not a standard DOM API. While some React Native webview implementations may support this, behavior is platform-specific and could break across different RN versions or platforms (iOS vs Android).Consider testing on target platforms and adding runtime checks:
if (typeof (document as any).addEventListener === 'function') { try { document.addEventListener('message', documentMessageHandler); } catch (e) { console.warn('document.addEventListener("message") not supported:', e); } }Alternatively, document which RN webview versions/platforms require this approach.
81-113: LGTM! Clean implementation of message handling and validation.The private key processing logic is sound:
- Proper parsing with error handling
- Uses
viemto derive and validate the address- Appropriate error callbacks for cryptographic failures
- Silent catch for parsing errors (expected for non-target messages)
193-201: LGTM! Comprehensive cleanup of all event listeners.The cleanup function properly removes all three event listeners (window, document, custom) and clears the timer. This prevents memory leaks and ensures clean teardown.
| } catch (e) { | ||
| console.error('Error parsing document message, retrying in 5s:', e); | ||
| // Retry request every 5 seconds on parsing errors | ||
| setTimeout(() => { | ||
| requestPrivateKey(); | ||
| }, 5000); | ||
| } |
There was a problem hiding this comment.
Risk of infinite retry loop on persistent parse errors.
The catch block retries requestPrivateKey() every 5 seconds on any parsing error. If the error is persistent (e.g., malformed data from another message source), this creates an unbounded retry loop that:
- Continuously sends authentication requests
- Consumes resources
- May spam the console with errors
Consider these alternatives:
Option 1: Remove retry logic (recommended for parse errors):
} catch (e) {
console.error('Error parsing document message:', e);
- // Retry request every 5 seconds on parsing errors
- setTimeout(() => {
- requestPrivateKey();
- }, 5000);
}Option 2: Add bounded retry with exponential backoff:
let retryCount = 0;
const MAX_RETRIES = 3;
// In catch block:
if (retryCount < MAX_RETRIES) {
retryCount++;
setTimeout(() => requestPrivateKey(), Math.min(1000 * Math.pow(2, retryCount), 30000));
}Parsing errors typically indicate wrong message format, not transient failures, so Option 1 is preferred.
🤖 Prompt for AI Agents
In src/utils/pillarWalletMessaging.ts around lines 146 to 152, the catch block
currently retries requestPrivateKey() every 5s on any parse error which can
create an infinite retry loop; remove the unconditional retry and instead either
(preferred) drop the retry logic entirely for parse errors (just log and return)
or (alternative) implement a bounded retry: add a retry counter (module/closure
scope) and MAX_RETRIES, increment on each failure, schedule requestPrivateKey()
with exponential backoff only while retryCount < MAX_RETRIES, reset retryCount
on a successful parse, and emit a final error log when the max is reached so
retries stop and the system doesn’t spam requests.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
Bug Fixes
Chores