Develop #202
Develop #202
Conversation
📝 WalkthroughWalkthroughExpands multi-platform delivery: adds Docker runtime injection and compose, introduces Electron desktop app and web build artifacts, extends CI/CD with web/docker/electron jobs and release-notes handling, implements a unified platform-aware Mapbox wrapper (web + native), web shims for native modules, and platform-specific push notification services and service worker. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Container Start
participant Entrypoint as docker-entrypoint.sh
participant Nginx as Nginx Server
participant Browser as Browser/Client
Client->>Entrypoint: Container starts (ENV vars present)
Entrypoint->>Entrypoint: Generate /usr/share/nginx/html/env-config.js
Entrypoint->>Entrypoint: Inject script tag into index.html if missing
Entrypoint->>Nginx: exec nginx (foreground)
Browser->>Nginx: HTTP GET /
Nginx->>Browser: Serve index.html + env-config.js
Browser->>Browser: window.__ENV__ available (runtime config)
sequenceDiagram
participant PushServer as Push Server
participant SW as Service Worker
participant OS as System Notification
participant Client as Web App (Window)
participant User
PushServer->>SW: Push event (payload)
SW->>SW: Parse payload, build notification options
SW->>OS: showNotification(...)
OS->>User: Display notification
User->>OS: Click notification
OS->>SW: notificationclick event
SW->>Client: PostMessage NOTIFICATION_CLICK (open/focus)
Client->>Client: Handle payload (navigate/show modal)
sequenceDiagram
participant AppCode as App (renderer)
participant MapboxModule as `@/components/maps/mapbox`
participant PlatformDetect as Platform.OS check
participant WebImpl as map-view.web.tsx
participant NativeImpl as map-view.native.tsx
AppCode->>MapboxModule: import Mapbox
MapboxModule->>PlatformDetect: detect platform
alt isWeb
PlatformDetect->>WebImpl: require web impl
WebImpl->>MapboxModule: export MapView/Camera/.. (mapbox-gl)
else
PlatformDetect->>NativeImpl: require native impl
NativeImpl->>MapboxModule: export MapView/Camera/.. (`@rnmapbox/maps`)
end
MapboxModule->>AppCode: return unified Mapbox API
AppCode->>AppCode: use Mapbox.* with consistent refs/types
sequenceDiagram
participant Renderer as Electron Renderer
participant Preload as Preload Script
participant Main as Main Process
participant System as OS Notification
participant User
Renderer->>Preload: electronAPI.showNotification({title,body})
Preload->>Main: ipcRenderer.invoke('show-notification', payload)
Main->>System: create Notification
System->>User: display
User->>System: click
System->>Main: notification click event
Main->>Renderer: ipc message 'notification-clicked' with data
Renderer->>Renderer: handle click (focus/navigate)
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/maps/full-screen-location-picker.tsx (2)
14-15: Missing null check for Mapbox access token.Unlike
location-picker.tsxwhich validates the token before callingsetAccessToken, this file calls it unconditionally. IfUNIT_MAPBOX_PUBKEYis undefined, this could cause issues.📝 Suggested fix
// Ensure Mapbox access token is set before using any Mapbox components +if (!Env.UNIT_MAPBOX_PUBKEY) { + console.error('Mapbox access token is not configured. Please set UNIT_MAPBOX_PUBKEY in your environment.'); +} else { + Mapbox.setAccessToken(Env.UNIT_MAPBOX_PUBKEY); +} -Mapbox.setAccessToken(Env.UNIT_MAPBOX_PUBKEY);
184-184: Hardcoded string should use translation function.Per coding guidelines, all user-facing text should be wrapped in
t()from react-i18next. Thetitleprop uses a hardcoded string "Selected Location".📝 Suggested fix
- <Mapbox.PointAnnotation id="selectedLocation" coordinate={[currentLocation.longitude, currentLocation.latitude]} title="Selected Location"> + <Mapbox.PointAnnotation id="selectedLocation" coordinate={[currentLocation.longitude, currentLocation.latitude]} title={t('common.selected_location')}>
🤖 Fix all issues with AI agents
In @.github/workflows/react-native-cicd.yml:
- Around line 531-536: The step "🐳 Log in to Docker Hub" currently uses an if:
expression referencing the secrets context which is not available in step-level
ifs; change the workflow to compute a boolean flag at the job level or via a
preceding step and reference that flag instead: add a job-level env like
HAS_DOCKER_CREDS computed from the secrets (or create a setup step that sets an
output HAS_DOCKER_CREDS) and then update the "🐳 Log in to Docker Hub" step's if
to check that env/output flag before invoking docker/login-action@v3 to avoid
using secrets in the step-level if.
In `@docker/docker-entrypoint.sh`:
- Around line 7-28: The heredoc writes raw environment values into env-config.js
which can break JS or allow injection; add a shell escaper (e.g., a function
named js_escape or escape_js) that JSON-escapes quotes, backslashes and newlines
(or use jq -Rn --arg to produce safe JSON strings), and call it for every string
field in the window.__ENV__ block (replace "${APP_ENV:-...}" with "$(js_escape
"${APP_ENV:-production}")", do likewise for NAME, SCHEME, VERSION, BASE_API_URL,
API_VERSION, RESGRID_API_URL, CHANNEL_HUB_NAME, REALTIME_GEO_HUB_NAME,
LOGGING_KEY, APP_KEY, UNIT_MAPBOX_PUBKEY, SENTRY_DSN, COUNTLY_APP_KEY,
COUNTLY_SERVER_URL) so the generated env-config.js contains valid, escaped JS
string literals.
In `@docs/build-quick-reference.md`:
- Around line 37-41: Update the "Electron Builds" Windows entry to match README
wording: replace the ambiguous ".exe, .msi" with a clear phrasing like "Portable
.exe and NSIS installer (.exe)" (or "Portable .exe, NSIS .exe") in the "Electron
Builds" section so it no longer implies NSIS produces .msi; edit the Windows
list item in that section accordingly.
In `@electron/main.js`:
- Around line 27-28: The frame option currently uses a tautological expression
(process.platform !== 'darwin' || true) so it always evaluates to true; change
the logic in the window creation options (the frame property) to reflect the
intended behavior—use a boolean expression that is false on macOS and true
elsewhere (e.g., frame: process.platform !== 'darwin') or invert it if you
intended to hide the frame on macOS (e.g., frame: process.platform === 'darwin'
? false : true); update the frame assignment where the BrowserWindow is
constructed to remove the stray "|| true" and use the correct platform check.
- Around line 5-8: The code uses require('electron-squirrel-startup') in main.js
to handle Windows installer shortcuts but the package is missing from
dependencies; add "electron-squirrel-startup" to package.json dependencies (not
devDependencies) so require('electron-squirrel-startup') resolves at runtime, or
delete the require/app.quit() block if you are not shipping Squirrel.Windows
installers; ensure package.json is updated and install (npm/yarn/pnpm) so the
app.quit() branch works correctly on Windows.
In `@electron/preload.js`:
- Around line 12-18: The current onNotificationClicked registration uses
ipcRenderer.on and returns a cleanup that calls removeAllListeners which will
unregister other subscribers; instead register the listener as a named function
(e.g., const listener = (event, data) => callback(data)) when calling
ipcRenderer.on inside onNotificationClicked and return a cleanup that calls
ipcRenderer.removeListener('notification-clicked', listener) so only that
specific handler is removed; update the onNotificationClicked implementation to
use this specific listener reference rather than removeAllListeners.
In `@nginx.conf`:
- Around line 38-48: The security headers declared with add_header in the server
block (X-Frame-Options, X-Content-Type-Options, X-XSS-Protection) are not
inherited by the static assets location; update the location ~*
\.(js|css|png|jpg|jpeg|gif|ico|svg|woff|woff2|ttf|eot)$ block to explicitly
include the same add_header directives (with always) so static files are served
with the same security headers as the parent.
In `@public/service-worker.js`:
- Around line 84-91: The arbitrary 1s setTimeout before calling
client.postMessage({type: 'NOTIFICATION_CLICK', ...}) is fragile; implement a
handshake instead: after clients.openWindow(...) store the pending notification
data keyed to the returned client (or its id) and remove the timeout; add a
service worker message listener that waits for a readiness signal (e.g.
message.type === 'SW_READY' or a handshake response) from that client and only
then call client.postMessage with 'NOTIFICATION_CLICK'; alternatively implement
a short poll/handshake (send PING and wait for PONG) using the same
client.postMessage/message event flow so the notification is sent reliably when
the page is initialized.
In `@src/components/maps/map-view.web.tsx`:
- Around line 246-294: PointAnnotation currently ignores React children and the
anchor prop: instead of writing a hardcoded innerHTML it should render actual
React children into the marker container (use ReactDOM.createPortal or
ReactDOM.render into containerRef.current inside the effect, referencing
PointAnnotation's containerRef / markerRef) and it should apply the anchor prop
when creating the mapbox marker (if anchor is a string/valid mapbox anchor pass
it to new mapboxgl.Marker's options; if anchor is an {x,y} object convert to
pixel offset and call marker.setOffset([xOffset, yOffset]) after creation).
Ensure you also attach the click listener to containerRef.current and clean up /
unmount the portal on effect teardown.
- Around line 306-327: The useEffect that creates a mapboxgl.GeolocateControl
(geolocate) currently only calls geolocate.trigger() inside map.on('load'),
which misses the case where map is already loaded; update the effect (in the
useEffect that references map, visible, showsUserHeadingIndicator) to: add
geolocate to the map as before, then if map.loaded() is true call
geolocate.trigger() immediately, otherwise attach a named load handler (e.g.
onMapLoad) that calls geolocate.trigger(), and remove that handler in the
cleanup along with removing the control; this ensures geolocate.trigger() runs
whether the map was already loaded or loads later.
In `@src/lib/native-module-shims.web.ts`:
- Around line 63-82: The current export defines messaging as a function and then
assigns messaging.AuthorizationStatus which breaks TypeScript; change messaging
to a callable object with a proper type and attach AuthorizationStatus via
Object.assign. Define an interface/type (e.g., Messaging) that declares a call
signature returning the shim methods and a static AuthorizationStatus property,
then replace the export with something like: const messaging =
Object.assign((/*...*/) => ({ ... }), { AuthorizationStatus: { NOT_DETERMINED:
-1, DENIED: 0, AUTHORIZED: 1, PROVISIONAL: 2 } }) and export that const so
TypeScript understands both the callable behavior and the static
AuthorizationStatus property.
In `@src/lib/platform.ts`:
- Around line 10-11: The current isElectron export uses window.process?.type
which fails with contextIsolation and nodeIntegration disabled; update the
detection in the isElectron constant (in src/lib/platform.ts) to prefer the
preload-exposed electronAPI (e.g., check for typeof window !== 'undefined' &&
!!(window as any).electronAPI) and fall back to the existing
window.process?.type check only if present. Replace the current export with a
combined check that first looks for the preload-exposed electronAPI and then
falls back to the old process.type test so detection works when contextIsolation
is enabled.
In `@src/services/bluetooth-audio.service.ts`:
- Around line 919-931: In setupButtonEventMonitoring, replace the unsafe any in
the peripheralInfo.services map with the proper Service type from
react-native-ble-manager: import { Service } from 'react-native-ble-manager' and
change the map callback signature from (s: any) => s.uuid to (s: Service) =>
s.uuid; ensure the peripheralInfo.services is treated as Service[] (or cast it
where needed) so the logger's availableServices remains a string[] and
TypeScript compiles without using any.
In `@src/services/push-notification.electron.ts`:
- Around line 48-54: Update the notification click registration to capture and
store the unsubscribe function returned by the preload API: change the type
signature of onNotificationClicked to return a () => void, update the call in
the service to assign the returned function to cleanupListener instead of
setting it to null, and ensure cleanup() (and any re-init/HMR logic) invokes
cleanupListener when present; key symbols: onNotificationClicked,
cleanupListener, handleNotificationClick, cleanup().
In `@src/stores/app/livekit-store.ts`:
- Line 1: Replace the magic number [2] used when starting the Android foreground
service with the imported enum value from AndroidForegroundServiceType (use the
appropriate member name for the service type), i.e. update the notifee call that
currently passes [2] to pass AndroidForegroundServiceType.<appropriate_member>
instead; keep the existing import of AndroidForegroundServiceType and consult
the library docs to pick the exact enum member that matches the numeric 2.
In `@types/docker-env.d.ts`:
- Line 19: The declared type for IS_MOBILE_APP in types/docker-env.d.ts is a
boolean but runtime env vars are strings; change the type of IS_MOBILE_APP to
string (update the IS_MOBILE_APP declaration) and ensure callers parse it (e.g.,
where window.__ENV__ or IS_MOBILE_APP is consumed, convert with a strict
equality check against 'true' or run a small parser) so runtime values like
"false" do not evaluate truthily.
🧹 Nitpick comments (34)
docs/android-ptt-foreground-service-fix.md (1)
6-8: Add language specifier to the fenced code block.The error message code block is missing a language specifier. While this is a plain text error, using a language hint improves rendering.
Suggested fix
-``` +```text java.lang.SecurityException: Starting FGS with type microphone callerApp=ProcessRecord{...} targetSDK=35 requires permissions: all of the permissions allOf=true [android.permission.FOREGROUND_SERVICE_MICROPHONE] any of the permissions allOf=false [android.permission.RECORD_AUDIO, ...]</details> </blockquote></details> <details> <summary>src/services/bluetooth-audio.service.ts (1)</summary><blockquote> `940-955`: **Multiple `any` type assertions should be replaced with proper types.** This helper method has good defensive logic, but uses `any` type assertions in multiple places (lines 948, 950, 954), which violates the coding guideline to "avoid using `any`; strive for precise types." <details> <summary>Suggested improvement</summary> Define a local interface for the service/characteristic structure returned by `BleManager.retrieveServices()`: ```typescript interface BleService { uuid: string; characteristics?: Array<{ characteristic: string; // add other properties as needed }>; } private hasCharacteristic(peripheralInfo: PeripheralInfo, serviceUuid: string, characteristicUuid: string): boolean { if (!peripheralInfo?.services) { return false; } const services = peripheralInfo.services as BleService[]; const service = services.find((s) => s.uuid?.toUpperCase() === serviceUuid.toUpperCase()); if (!service?.characteristics) { return false; } return service.characteristics.some((c) => c.characteristic?.toUpperCase() === characteristicUuid.toUpperCase()); }As per coding guidelines, strive for precise types instead of
any..dockerignore (1)
56-62: Duplicate*.pempattern.The
*.pempattern appears twice: on line 24 (under Misc) and line 58 (under Secrets and keys). While harmless, consider removing the duplicate for cleaner configuration.🧹 Suggested fix
# Misc .DS_Store -*.pem .env .env.localpackage.json (1)
109-109: Move@types/mapbox-glto devDependencies.Type definition packages (
@types/*) are only needed during development/build time and should be indevDependenciesrather thandependenciesto avoid shipping unnecessary packages in production.♻️ Suggested fix
Move from
dependencies:- "@types/mapbox-gl": "3.4.1",Add to
devDependencies:+ "@types/mapbox-gl": "3.4.1",electron/entitlements.mac.plist (1)
22-24: Consider deferring unused entitlements until needed.The camera entitlement is marked as "future use." Apple's App Store review guidelines recommend requesting only permissions that are actively used. If this app will be distributed via the Mac App Store, preemptively declaring camera access without using it could lead to rejection or user trust concerns.
Consider removing this entitlement until the camera feature is implemented, or ensure there's a clear usage description in your
Info.plist..github/workflows/react-native-cicd.yml (5)
477-517: Add explicit permissions block for security hardening.CodeQL flags that the new jobs lack explicit
permissionsblocks. Following the principle of least privilege, add minimal permissions to each job.♻️ Proposed fix for build-web job
build-web: needs: test if: (github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master')) || github.event_name == 'workflow_dispatch' runs-on: ubuntu-latest environment: RNBuild + permissions: + contents: read steps:
519-570: Add permissions block for build-docker job.This job requires additional permissions for pushing to GHCR.
♻️ Proposed fix
build-docker: needs: build-web if: (github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master')) || github.event_name == 'workflow_dispatch' runs-on: ubuntu-latest environment: RNBuild + permissions: + contents: read + packages: write steps:
571-600: Add permissions block for build-electron job.This job creates releases and needs appropriate permissions.
♻️ Proposed fix
build-electron: needs: test if: (github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master')) || github.event_name == 'workflow_dispatch' strategy: matrix: os: [windows-latest, macos-15, ubuntu-latest] runs-on: ${{ matrix.os }} environment: RNBuild + permissions: + contents: write steps:
492-500: Update actions/cache to v4.Static analysis indicates
actions/cache@v3is outdated. Update toactions/cache@v4for the latest features and fixes.♻️ Proposed fix
- name: 📦 Setup yarn cache - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: |Also applies to: 589-597
685-806: Duplicated release notes extraction logic.The
extract_release_notesfunction and surrounding logic is duplicated betweenbuild-mobile(lines 296-416) andbuild-electron(lines 685-806). Consider extracting this into a reusable composite action or a shared script.You could create a shared script file (e.g.,
scripts/extract-release-notes.sh) or a reusable composite action to avoid maintaining the same code in two places.src/lib/platform.ts (2)
53-65: Window augmentation should remove process.type since it won't be available.Given that
nodeIntegrationis disabled,window.processwon't exist. The type augmentation forprocessis misleading and could cause confusion.♻️ Proposed cleanup
// Augment Window interface for Electron API declare global { interface Window { - process?: { - type?: string; - }; electronAPI?: { showNotification: (options: { title: string; body: string; data: any }) => Promise<boolean>; onNotificationClicked: (callback: (data: any) => void) => void; platform: string; isElectron: boolean; }; } }
59-64: Improve type safety by avoidingany.Per coding guidelines, avoid using
any. Consider defining a proper interface for notification options and callback data.♻️ Proposed improvement
+interface NotificationOptions { + title: string; + body: string; + data?: Record<string, unknown>; +} + +interface NotificationData { + [key: string]: unknown; +} + declare global { interface Window { electronAPI?: { - showNotification: (options: { title: string; body: string; data: any }) => Promise<boolean>; - onNotificationClicked: (callback: (data: any) => void) => void; + showNotification: (options: NotificationOptions) => Promise<boolean>; + onNotificationClicked: (callback: (data: NotificationData) => void) => void; platform: string; isElectron: boolean; }; } }electron/preload.js (1)
36-37: Consider removing or conditionalizing the console.log for production.This log statement will appear in the DevTools console for every app launch. While useful during development, it may be unnecessary noise in production.
♻️ Proposed fix
-// Log that preload script has been loaded -console.log('Electron preload script loaded'); +// Log that preload script has been loaded (development only) +if (process.env.NODE_ENV === 'development') { + console.log('Electron preload script loaded'); +}Note:
process.env.NODE_ENVmay not be available in preload. You might need to checkprocess.env.NODE_ENVbefore the app is packaged, or simply remove the log.electron/main.js (1)
55-59: Avoid redundantrequireinside the handler.
shellis available from the Electron import at the top of the file but wasn't destructured. Add it to the top-level import instead of callingrequireinline.♻️ Proposed fix
At line 2, add
shellto the imports:-const { app, BrowserWindow, ipcMain, Notification, nativeTheme, Menu } = require('electron'); +const { app, BrowserWindow, ipcMain, Notification, nativeTheme, Menu, shell } = require('electron');Then update the handler:
// Handle external links mainWindow.webContents.setWindowOpenHandler(({ url }) => { - require('electron').shell.openExternal(url); + shell.openExternal(url); return { action: 'deny' }; });src/services/push-notification.electron.ts (1)
9-17: Replaceanywith a typed notification payload.
anyremoves type safety across the Electron API contract and handlers. Define a shared payload interface and reuse it in the API surface and methods.♻️ Proposed refactor
import { logger } from '@/lib/logging'; import { isElectron } from '@/lib/platform'; import { usePushNotificationModalStore } from '@/stores/push-notification/store'; +interface ElectronNotificationPayload { + eventCode?: string; + title?: string; + body?: string; + message?: string; + [key: string]: unknown; +} + // Type declaration for Electron API exposed via preload declare global { interface Window { electronAPI?: { - showNotification: (options: { title: string; body: string; data: any }) => Promise<boolean>; - onNotificationClicked: (callback: (data: any) => void) => void; + showNotification: (options: { title: string; body: string; data: ElectronNotificationPayload }) => Promise<boolean>; + onNotificationClicked: (callback: (data: ElectronNotificationPayload) => void) => void; platform: string; isElectron: boolean; }; } } @@ - private handleNotificationClick = (data: any): void => { + private handleNotificationClick = (data: ElectronNotificationPayload): void => { @@ - async showNotification(title: string, body: string, data?: any): Promise<boolean> { + async showNotification(title: string, body: string, data?: ElectronNotificationPayload): Promise<boolean> {As per coding guidelines, avoid
anyfor TypeScript types.Also applies to: 62-83
src/lib/native-module-shims.web.ts (1)
40-75: Avoid returning fake tokens/permissions in web shims.Returning
'web-token'andAUTHORIZEDcan register invalid devices or show push-enabled UI on web. Prefernull/NOT_DETERMINED(orDENIED) so unsupported behavior is explicit.♻️ Proposed refactor
export const notifee = { createChannel: async () => 'web-channel', displayNotification: async () => 'web-notification', setBadgeCount: async () => {}, getBadgeCount: async () => 0, incrementBadgeCount: async () => {}, decrementBadgeCount: async () => {}, - requestPermission: async () => ({ authorizationStatus: AuthorizationStatus.AUTHORIZED }), - getPermissionSettings: async () => ({ authorizationStatus: AuthorizationStatus.AUTHORIZED }), + requestPermission: async () => ({ authorizationStatus: AuthorizationStatus.NOT_DETERMINED }), + getPermissionSettings: async () => ({ authorizationStatus: AuthorizationStatus.NOT_DETERMINED }), @@ export const messaging = () => ({ - getToken: async () => 'web-token', + getToken: async () => null, deleteToken: async () => {}, - hasPermission: async () => 1, - requestPermission: async () => 1, + hasPermission: async () => AuthorizationStatus.NOT_DETERMINED, + requestPermission: async () => AuthorizationStatus.NOT_DETERMINED, onMessage: () => () => {}, onNotificationOpenedApp: () => () => {}, getInitialNotification: async () => null, setBackgroundMessageHandler: () => {}, subscribeToTopic: async () => {}, unsubscribeFromTopic: async () => {}, });metro.config.js (1)
23-39: Hoist the native module list to module scope.The array is recreated on every resolve; move it to a top-level
Setand use.has()to reduce allocations.♻️ Proposed refactor
const config = getSentryExpoConfig(__dirname, { isCSSEnabled: true, }); @@ -// Web-specific module resolution - redirect native modules to web shims +// Web-specific module resolution - redirect native modules to web shims +const nativeModules = new Set([ + '@notifee/react-native', + '@react-native-firebase/messaging', + '@react-native-firebase/app', + 'react-native-callkeep', + 'react-native-ble-manager', + '@livekit/react-native', + '@livekit/react-native-webrtc', + '@livekit/react-native-expo-plugin', +]); + const originalResolveRequest = config.resolver.resolveRequest; config.resolver.resolveRequest = (context, moduleName, platform) => { if (platform === 'web') { - // Native modules that need web shims - const nativeModules = [ - '@notifee/react-native', - '@react-native-firebase/messaging', - '@react-native-firebase/app', - 'react-native-callkeep', - 'react-native-ble-manager', - '@livekit/react-native', - '@livekit/react-native-webrtc', - '@livekit/react-native-expo-plugin', - ]; - - if (nativeModules.includes(moduleName)) { + if (nativeModules.has(moduleName)) { return { filePath: path.resolve(__dirname, 'src/lib/native-module-shims.web.ts'), type: 'sourceFile', }; }docker-compose.yml (1)
21-26: Consider adding placeholder instructions for required secrets.Several environment variables are left empty (UNIT_LOGGING_KEY, UNIT_APP_KEY, UNIT_MAPBOX_PUBKEY, etc.). While this is fine for a template, consider adding comments indicating which variables are required vs optional for the application to function correctly.
📝 Suggested improvement
- UNIT_CHANNEL_HUB_NAME=eventingHub - UNIT_REALTIME_GEO_HUB_NAME=geolocationHub - - UNIT_LOGGING_KEY= - - UNIT_APP_KEY= - - UNIT_MAPBOX_PUBKEY= - - UNIT_SENTRY_DSN= - - UNIT_COUNTLY_APP_KEY= - - UNIT_COUNTLY_SERVER_URL= + - UNIT_LOGGING_KEY= # Optional: Logging service key + - UNIT_APP_KEY= # Required: Application key + - UNIT_MAPBOX_PUBKEY= # Required: Mapbox public key for maps + - UNIT_SENTRY_DSN= # Optional: Sentry DSN for error tracking + - UNIT_COUNTLY_APP_KEY= # Optional: Countly analytics key + - UNIT_COUNTLY_SERVER_URL= # Optional: Countly server URLsrc/components/maps/location-picker.tsx (1)
40-41: Avoid usinganyforcameraRef- consider a more precise type.Per coding guidelines, avoid using
anyand strive for precise types. The comment indicates this is due to imperative handle access, but a more specific type would improve type safety.📝 Suggested improvement
Consider defining an interface for the camera's imperative API:
interface CameraRef { setCamera: (options: { centerCoordinate: [number, number]; zoomLevel: number; animationDuration?: number; }) => void; } const cameraRef = useRef<CameraRef>(null);Alternatively, if the wrapper exports proper types, use those directly.
src/lib/env.js (1)
32-44: Consider reducing@ts-ignoreusage with proper type definitions.Three
@ts-ignorecomments suppress type errors. Since this file handles both Docker runtime (window.__ENV__) and native builds (Constants.expoConfig?.extra), consider defining a union type or using proper type guards to maintain type safety.📝 Suggested approach
/** * `@typedef` {import('../../env.js').ClientEnv} EnvConfig */ /** * `@returns` {EnvConfig} */ const getEnvConfig = () => { if (isDockerRuntime()) { // `@ts-ignore` - window.__ENV__ typed in types/docker-env.d.ts return /** `@type` {EnvConfig} */ (window.__ENV__); } return /** `@type` {EnvConfig} */ (Constants.expoConfig?.extra ?? {}); }; /** `@type` {EnvConfig} */ export const Env = getEnvConfig();src/components/maps/full-screen-location-picker.tsx (1)
38-39: Avoid usinganyforcameraRef.Same issue as in
location-picker.tsx- usinganybypasses type safety. Consider defining a shared camera ref type that can be reused across map components.src/components/maps/pin-marker.tsx (2)
18-18: UnusedmarkerRefprop.The
markerRefprop is declared inPinMarkerProps(line 14) but is not destructured or used in the component. If it's intended for parent components to access, it should be forwarded; otherwise, consider removing it from the interface.
25-25: Potential runtime error with non-null assertion onicon.The non-null assertion
icon!.uriassumes the fallbackMAP_ICONS['call']always exists. IfMAP_ICONSdoesn't contain a'call'key or the lookup fails, this will throw at runtime.Proposed defensive fix
- <Image fadeDuration={0} source={icon!.uri} style={[styles.image, { width: size, height: size }]} /> + <Image fadeDuration={0} source={icon?.uri} style={[styles.image, { width: size, height: size }]} />Alternatively, add a guard or ensure
MAP_ICONS['call']is guaranteed to exist via TypeScript strict checks.nginx.conf (2)
41-41:X-XSS-Protectionheader is deprecated.The
X-XSS-Protectionheader is no longer recommended and has been removed from modern browsers. Consider removing it and implementing aContent-Security-Policyheader instead for better XSS protection.
56-60: Reorderadd_headerbeforereturnfor clarity.While nginx processes directives correctly, placing
add_headerafterreturnis unconventional and can confuse readers. Reordering improves readability.Proposed fix
# Health check endpoint location /health { access_log off; + add_header Content-Type text/plain; return 200 "healthy\n"; - add_header Content-Type text/plain; }src/app/(app)/index.tsx (2)
34-34: Consider typingcameraRefmore precisely.Using
anyviolates the coding guideline "Avoid usingany; strive for precise types." If the imperative handle API is known, consider defining an interface for the expected methods (setCamera, etc.).Proposed interface for camera ref
interface CameraHandle { setCamera: (config: { centerCoordinate: [number, number]; zoomLevel?: number; animationDuration?: number; heading?: number; pitch?: number; }) => void; } const cameraRef = useRef<CameraHandle>(null);Based on coding guidelines, avoid using
any.
398-399: Use ternary operator for conditional rendering.Per coding guidelines, use
? :instead of&&for conditional rendering. Lines 398 and 411 use&&.Example fix for line 398-399
- {location.latitude && location.longitude && ( + {location.latitude && location.longitude ? ( <Mapbox.PointAnnotation ...> ... </Mapbox.PointAnnotation> - )} + ) : null}Based on coding guidelines.
Also applies to: 411-411
public/service-worker.js (1)
8-9: UnusedCACHE_NAMEconstant.
CACHE_NAMEis declared but never used. Either implement offline caching or remove the constant to avoid confusion.docs/cicd-build-system.md (2)
188-194: Add language specifier to fenced code block.The ASCII diagram code block lacks a language specifier. Use
textorplaintextto satisfy linting.Proposed fix
-``` +```text test (all platforms) ├─→ build-mobile (android, ios) ├─→ build-web │ └─→ build-docker └─→ build-electron (windows, macos, linux)</details> --- `242-242`: **Capitalize "GitHub" consistently.** Minor: The reference should use "GitHub" with capital H for consistency with the official branding. <details> <summary>Proposed fix</summary> ```diff -- Workflow file: [.github/workflows/react-native-cicd.yml](../.github/workflows/react-native-cicd.yml) +- Workflow file: [GitHub workflow](../.github/workflows/react-native-cicd.yml)Or keep the path as-is since it's a literal filesystem path (
.githubis the actual directory name).docker/README.md (2)
70-72: Consider updating Docker Compose version specification.The
version: '3.8'field is deprecated in modern Docker Compose (v2+). While it still works for backward compatibility, you can simply remove the version line to use the latest Compose specification.📝 Suggested update
-version: '3.8' - services: resgrid-unit:
148-157: Consider adding a note about external secret management for Kubernetes.While the example is instructive, it shows secrets in plaintext within
stringData. Consider adding a brief note recommending external secret management solutions (e.g., HashiCorp Vault, AWS Secrets Manager, Sealed Secrets) for production deployments.src/components/maps/mapbox.ts (1)
9-32: Consider simplifying the repeated platform checks and require calls.Each export performs the same
Platform.OS === 'web'check andrequire()call. SinceMapboxImplis already resolved to the correct platform module, you can destructure from it directly. This improves maintainability and reduces redundancy.♻️ Proposed simplification
const MapboxImpl = Platform.OS === 'web' ? require('./map-view.web').default : require('./map-view.native').default; +const platformModule = Platform.OS === 'web' ? require('./map-view.web') : require('./map-view.native'); // Re-export all components export const MapView = MapboxImpl.MapView || MapboxImpl; -export const Camera = Platform.OS === 'web' ? require('./map-view.web').Camera : require('./map-view.native').Camera; -export const PointAnnotation = Platform.OS === 'web' ? require('./map-view.web').PointAnnotation : require('./map-view.native').PointAnnotation; -export const UserLocation = Platform.OS === 'web' ? require('./map-view.web').UserLocation : require('./map-view.native').UserLocation; -export const MarkerView = Platform.OS === 'web' ? require('./map-view.web').MarkerView : require('./map-view.native').MarkerView; -export const ShapeSource = Platform.OS === 'web' ? require('./map-view.web').ShapeSource : require('./map-view.native').ShapeSource; -export const SymbolLayer = Platform.OS === 'web' ? require('./map-view.web').SymbolLayer : require('./map-view.native').SymbolLayer; -export const CircleLayer = Platform.OS === 'web' ? require('./map-view.web').CircleLayer : require('./map-view.native').CircleLayer; -export const LineLayer = Platform.OS === 'web' ? require('./map-view.web').LineLayer : require('./map-view.native').LineLayer; -export const FillLayer = Platform.OS === 'web' ? require('./map-view.web').FillLayer : require('./map-view.native').FillLayer; -export const Images = Platform.OS === 'web' ? require('./map-view.web').Images : require('./map-view.native').Images; -export const Callout = Platform.OS === 'web' ? require('./map-view.web').Callout : require('./map-view.native').Callout; +export const { Camera, PointAnnotation, UserLocation, MarkerView, ShapeSource, SymbolLayer, CircleLayer, LineLayer, FillLayer, Images, Callout, StyleURL, UserTrackingMode, setAccessToken } = platformModule; -// Export style URL constants -export const StyleURL = Platform.OS === 'web' ? require('./map-view.web').StyleURL : require('./map-view.native').StyleURL; - -// Export UserTrackingMode -export const UserTrackingMode = Platform.OS === 'web' ? require('./map-view.web').UserTrackingMode : require('./map-view.native').UserTrackingMode; - -// Export setAccessToken -export const setAccessToken = Platform.OS === 'web' ? require('./map-view.web').setAccessToken : require('./map-view.native').setAccessToken; - // Default export matching Mapbox structure export default MapboxImpl;src/components/maps/map-view.web.tsx (1)
345-351: Placeholder components useanytype.Per coding guidelines, avoid using
any. Consider defining minimal prop interfaces for these placeholder components to maintain type safety.♻️ Suggested typing
-export const ShapeSource: React.FC<any> = ({ children }) => <>{children}</>; -export const SymbolLayer: React.FC<any> = () => null; -export const CircleLayer: React.FC<any> = () => null; -export const LineLayer: React.FC<any> = () => null; -export const FillLayer: React.FC<any> = () => null; -export const Images: React.FC<any> = () => null; -export const Callout: React.FC<any> = ({ children }) => <>{children}</>; +interface PlaceholderProps { + children?: React.ReactNode; + [key: string]: unknown; +} +export const ShapeSource: React.FC<PlaceholderProps> = ({ children }) => <>{children}</>; +export const SymbolLayer: React.FC<PlaceholderProps> = () => null; +export const CircleLayer: React.FC<PlaceholderProps> = () => null; +export const LineLayer: React.FC<PlaceholderProps> = () => null; +export const FillLayer: React.FC<PlaceholderProps> = () => null; +export const Images: React.FC<PlaceholderProps> = () => null; +export const Callout: React.FC<PlaceholderProps> = ({ children }) => <>{children}</>;
| 5. ✅ **Electron Builds** | ||
| - Windows: .exe, .msi | ||
| - macOS: .dmg, .zip (Universal) | ||
| - Linux: .AppImage, .deb, .rpm | ||
| - All added to GitHub Release |
There was a problem hiding this comment.
Minor inconsistency: Windows Electron build output formats.
Line 38 lists .exe, .msi for Windows, but README.md (line 61) mentions "Portable exe and NSIS installer". NSIS typically produces .exe installers, not .msi. Consider aligning the documentation.
📝 Suggested fix
5. ✅ **Electron Builds**
- - Windows: .exe, .msi
+ - Windows: .exe (portable and NSIS installer)
- macOS: .dmg, .zip (Universal)
- Linux: .AppImage, .deb, .rpm📝 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.
| 5. ✅ **Electron Builds** | |
| - Windows: .exe, .msi | |
| - macOS: .dmg, .zip (Universal) | |
| - Linux: .AppImage, .deb, .rpm | |
| - All added to GitHub Release | |
| 5. ✅ **Electron Builds** | |
| - Windows: .exe (portable and NSIS installer) | |
| - macOS: .dmg, .zip (Universal) | |
| - Linux: .AppImage, .deb, .rpm | |
| - All added to GitHub Release |
🤖 Prompt for AI Agents
In `@docs/build-quick-reference.md` around lines 37 - 41, Update the "Electron
Builds" Windows entry to match README wording: replace the ambiguous ".exe,
.msi" with a clear phrasing like "Portable .exe and NSIS installer (.exe)" (or
"Portable .exe, NSIS .exe") in the "Electron Builds" section so it no longer
implies NSIS produces .msi; edit the Windows list item in that section
accordingly.
| private async setupButtonEventMonitoring(device: Device, peripheralInfo: PeripheralInfo): Promise<void> { | ||
| try { | ||
| const peripheralInfo = await BleManager.getDiscoveredPeripherals(); | ||
| const deviceInfo = peripheralInfo.find((p) => p.id === device.id); | ||
|
|
||
| if (!deviceInfo) { | ||
| logger.warn({ | ||
| message: 'Device not found in discovered peripherals', | ||
| context: { deviceId: device.id }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| logger.info({ | ||
| message: 'Setting up button event monitoring', | ||
| context: { | ||
| deviceId: device.id, | ||
| deviceName: device.name, | ||
| availableServices: peripheralInfo?.services?.map((s: any) => s.uuid) || [], | ||
| }, | ||
| }); | ||
|
|
||
| // Start notifications for known button control characteristics | ||
| await this.startNotificationsForButtonControls(device.id); | ||
| await this.startNotificationsForButtonControls(device.id, peripheralInfo); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the bluetooth-audio.service.ts file
fd "bluetooth-audio.service.ts" --type fRepository: Resgrid/Unit
Length of output: 95
🏁 Script executed:
# Check the structure around lines 919-931
cat -n src/services/bluetooth-audio.service.ts | sed -n '915,935p'Repository: Resgrid/Unit
Length of output: 942
🏁 Script executed:
# Search for PeripheralInfo type definition in the codebase
rg "PeripheralInfo" --type ts --type tsx -A 5 | head -50Repository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
# Check react-native-ble-manager in package.json or node_modules types
fd "react-native-ble-manager" --type f | head -20Repository: Resgrid/Unit
Length of output: 93
🏁 Script executed:
# Examine the mock file for type definitions
cat -n __mocks__/react-native-ble-manager.tsRepository: Resgrid/Unit
Length of output: 3117
🏁 Script executed:
# Search for PeripheralInfo type definition
rg "type PeripheralInfo|interface PeripheralInfo" --type ts -A 10Repository: Resgrid/Unit
Length of output: 1321
🏁 Script executed:
# Search for the import of PeripheralInfo in bluetooth-audio.service.ts
head -50 src/services/bluetooth-audio.service.ts | grep -n "PeripheralInfo\|import"Repository: Resgrid/Unit
Length of output: 709
🌐 Web query:
react-native-ble-manager PeripheralInfo type services array definition
💡 Result:
In react-native-ble-manager, PeripheralInfo.services is typed as an optional array of Service objects, where each Service is:
export interface Service {
uuid: string;
}
export interface PeripheralInfo extends Peripheral {
serviceUUIDs?: string[];
characteristics?: Characteristic[];
services?: Service[];
}So services is Service[] (objects with a uuid field), not string[]—that’s what serviceUUIDs is for. [1] [2]
Sources:
[1] src/types.ts (v12.4.1) https://app.unpkg.com/react-native-ble-manager@12.4.1/files/src/types.ts
[2] dist/esm/types.d.ts (v12.1.2) https://app.unpkg.com/react-native-ble-manager@12.1.2/files/dist/esm/types.d.ts
Replace any with the proper Service type from react-native-ble-manager.
Line 926 uses (s: any) which violates the coding guideline to avoid using any. Since PeripheralInfo.services is typed as Service[] in the react-native-ble-manager library, the parameter should be explicitly typed as Service instead.
Suggested improvement
- availableServices: peripheralInfo?.services?.map((s: any) => s.uuid) || [],
+ availableServices: peripheralInfo?.services?.map((s: Service) => s.uuid) || [],The Service type is exported from react-native-ble-manager and contains the uuid property. Keep the .map() operation as it intentionally extracts UUIDs into a string array for cleaner logging.
📝 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.
| private async setupButtonEventMonitoring(device: Device, peripheralInfo: PeripheralInfo): Promise<void> { | |
| try { | |
| const peripheralInfo = await BleManager.getDiscoveredPeripherals(); | |
| const deviceInfo = peripheralInfo.find((p) => p.id === device.id); | |
| if (!deviceInfo) { | |
| logger.warn({ | |
| message: 'Device not found in discovered peripherals', | |
| context: { deviceId: device.id }, | |
| }); | |
| return; | |
| } | |
| logger.info({ | |
| message: 'Setting up button event monitoring', | |
| context: { | |
| deviceId: device.id, | |
| deviceName: device.name, | |
| availableServices: peripheralInfo?.services?.map((s: any) => s.uuid) || [], | |
| }, | |
| }); | |
| // Start notifications for known button control characteristics | |
| await this.startNotificationsForButtonControls(device.id); | |
| await this.startNotificationsForButtonControls(device.id, peripheralInfo); | |
| private async setupButtonEventMonitoring(device: Device, peripheralInfo: PeripheralInfo): Promise<void> { | |
| try { | |
| logger.info({ | |
| message: 'Setting up button event monitoring', | |
| context: { | |
| deviceId: device.id, | |
| deviceName: device.name, | |
| availableServices: peripheralInfo?.services?.map((s: Service) => s.uuid) || [], | |
| }, | |
| }); | |
| // Start notifications for known button control characteristics | |
| await this.startNotificationsForButtonControls(device.id, peripheralInfo); |
🤖 Prompt for AI Agents
In `@src/services/bluetooth-audio.service.ts` around lines 919 - 931, In
setupButtonEventMonitoring, replace the unsafe any in the
peripheralInfo.services map with the proper Service type from
react-native-ble-manager: import { Service } from 'react-native-ble-manager' and
change the map callback signature from (s: any) => s.uuid to (s: Service) =>
s.uuid; ensure the peripheralInfo.services is treated as Service[] (or cast it
where needed) so the logger's availableServices remains a string[] and
TypeScript compiles without using any.
types/docker-env.d.ts
Outdated
| LOGGING_KEY: string; | ||
| APP_KEY: string; | ||
| UNIT_MAPBOX_PUBKEY: string; | ||
| IS_MOBILE_APP: boolean; |
There was a problem hiding this comment.
IS_MOBILE_APP type may not match runtime value.
Environment variables injected via shell scripts are strings. If docker-entrypoint.sh writes IS_MOBILE_APP: "${UNIT_IS_MOBILE_APP:-false}", the runtime value will be the string "false", not the boolean false. This can cause issues like if (window.__ENV__.IS_MOBILE_APP) evaluating to true for the string "false".
Proposed fix: use string type and parse at consumption
- IS_MOBILE_APP: boolean;
+ IS_MOBILE_APP: string;Then parse it when consuming: const isMobile = window.__ENV__?.IS_MOBILE_APP === 'true';
📝 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.
| IS_MOBILE_APP: boolean; | |
| IS_MOBILE_APP: string; |
🤖 Prompt for AI Agents
In `@types/docker-env.d.ts` at line 19, The declared type for IS_MOBILE_APP in
types/docker-env.d.ts is a boolean but runtime env vars are strings; change the
type of IS_MOBILE_APP to string (update the IS_MOBILE_APP declaration) and
ensure callers parse it (e.g., where window.__ENV__ or IS_MOBILE_APP is
consumed, convert with a strict equality check against 'true' or run a small
parser) so runtime values like "false" do not evaluate truthily.
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In @.github/workflows/react-native-cicd.yml:
- Around line 828-834: The artifact download step currently in the matrix can
run before Windows/macOS jobs finish; move the "Download all Electron artifacts"
logic out of the matrix and into a dedicated release job (e.g.,
release-electron) that uses needs: build-electron so it only runs after all
matrix runs complete; in that release job include the checkout step, prepare
release notes step, and then the actions/download-artifact@v4 step (using the
same path/pattern/merge-multiple settings) and guard the job with the same
if-condition used for releases (push to main/master or workflow_dispatch) so
artifact downloads and subsequent release steps always see all built artifacts.
- Around line 836-846: The "Prepare Electron release artifacts" step can
silently produce an empty release-artifacts/ directory because the find ...
-exec cp ... command succeeds even when no files match; update this step to
detect when no Electron artifacts are found before copying (e.g. run find to
count or list matches and fail the step if none are returned) and emit an
explicit error message and non-zero exit to stop the workflow, referencing the
existing find invocation and the release-artifacts directory so you only proceed
to cp/ls when artifacts exist.
In `@package.json`:
- Around line 198-200: The dependency spec for electron-squirrel-startup should
use an exact version like the other Electron deps; update the package.json entry
for "electron-squirrel-startup" to remove the caret and pin it to "1.0.1" so it
matches the exact-version style used by "electron" and "electron-builder".
In `@src/components/maps/map-view.web.tsx`:
- Around line 260-313: Replace deprecated ReactDOM.render and
ReactDOM.unmountComponentAtNode by creating and storing a React root for the
marker container: when mounting (where ReactDOM.render currently is) call
ReactDOM.createRoot(container) and root.render(<>{children}</>), store that root
(e.g., containerRootRef) so it can be used later; on cleanup (where
unmountComponentAtNode is used) call containerRootRef.current?.unmount() instead
of unmountComponentAtNode. Keep the rest of the logic (markerRef, containerRef,
anchor handling, onSelected listener, popup) the same and ensure you only create
the root when children exist and clear containerRootRef after unmount.
In `@src/lib/platform.ts`:
- Around line 12-15: Remove the unnecessary "as any" casts and fix Prettier
formatting: update the isElectron export to use the augmented Window type
directly (no cast) and ensure its expression is formatted correctly, and
likewise replace (window as any).electronAPI?.platform with
window.electronAPI?.platform in the platformName function; keep the same logic
but use the typed window.electronAPI access to satisfy the no-any rule and run
Prettier to normalize spacing/line breaks.
In `@src/services/push-notification.electron.ts`:
- Around line 10-19: Update the declaration of
Window.electronAPI.onNotificationClicked to match the implemented cleanup
pattern: change its type from returning void to returning a cleanup function,
i.e. onNotificationClicked: (callback: (data: any) => void) => () => void; in
the global Window.electronAPI interface so the callback signature and return
type align with the implementation that captures and calls the returned cleanup
function.
In `@src/services/push-notification.web.ts`:
- Around line 171-189: The unsubscribe() method returns early when
this.pushSubscription is null but must still locate any persisted browser
subscription and clear it; update unsubscribe() so that if this.pushSubscription
is falsy it attempts to retrieve the active subscription via the Service Worker
registration (use registration.pushManager.getSubscription() or equivalent) and
assign it to this.pushSubscription before proceeding, then call
this.pushSubscription.unsubscribe(), set this.pushSubscription = null, and clear
any persisted client-side record of the subscription (localStorage/IndexedDB
entry if used). Ensure errors from getSubscription() and unsubscribe() are
caught and logged via logger.error and the method still returns a boolean
indicating success/failure.
- Around line 79-97: The handler handleServiceWorkerMessage currently assumes
event.data.data exists and then accesses data.eventCode, which can throw if the
SW sends a NOTIFICATION_CLICK without a payload; guard against undefined by
reading const data = event.data?.data ?? undefined (or return early if missing)
and only proceed to log and call
usePushNotificationModalStore.getState().showNotificationModal when data is an
object with the expected fields (check data && data.eventCode), ensuring you
don't access data.eventCode, data.title, data.body, or data.message when data is
undefined.
- Around line 250-251: The static isSupported() method in the PushNotification
class (or module) directly accesses navigator and window which breaks in
SSR/Node; update isSupported() to first guard that typeof window !== 'undefined'
and typeof navigator !== 'undefined' before checking 'serviceWorker' in
navigator and 'PushManager'/'Notification' in window (or reuse the existing
platform guard helper used elsewhere) so the method returns false in non‑browser
environments and never throws.
- Around line 65-74: sendClientReadyHandshake currently silently skips sending
CLIENT_READY when navigator.serviceWorker.controller is null (first-install
case); update sendClientReadyHandshake to: if controller exists postMessage and
log; else if this.registration?.active exists postMessage to
this.registration.active and log; additionally add a one-time 'controllerchange'
listener that when navigator.serviceWorker.controller becomes available posts
CLIENT_READY and logs then removes the listener to avoid duplicate sends;
reference the sendClientReadyHandshake method,
navigator.serviceWorker.controller, this.registration?.active and the
'controllerchange' event in your changes.
In `@src/stores/app/livekit-store.ts`:
- Line 1: The build fails because the code uses an invalid enum member
AndroidForegroundServiceType.MICROPHONE; update the usage to the correct Notifee
enum name AndroidForegroundServiceType.FOREGROUND_SERVICE_TYPE_MICROPHONE (or,
if the original numeric intent was the media playback flag, use
AndroidForegroundServiceType.FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK) wherever
AndroidForegroundServiceType.MICROPHONE is referenced (e.g., in the livekit
foreground service setup code) so TypeScript compiles cleanly.
♻️ Duplicate comments (2)
.github/workflows/react-native-cicd.yml (1)
609-617: Updateactions/cacheto v4 for runner compatibility.Same issue as flagged for other jobs. The
actions/cache@v3action should be updated to v4.electron/main.js (1)
5-8: Verifyelectron-squirrel-startupis in package.json dependencies.As noted in past reviews,
require('electron-squirrel-startup')will fail at runtime on Windows if the package is not installed. Ensure it's added to dependencies if shipping with Squirrel.Windows installers.#!/bin/bash # Check if electron-squirrel-startup is in package.json rg "electron-squirrel-startup" package.json
🧹 Nitpick comments (16)
src/lib/native-module-shims.web.ts (1)
182-186: Consider using a type for the permission status.The hardcoded
'granted'string works for a shim, but for better type safety and alignment with Expo's actual API, consider using a more precise type.Optional: Add type annotation for permission response
+type PermissionResponse = { + granted: boolean; + status: 'granted' | 'denied' | 'undetermined'; +}; + // Expo modules that may have issues on web export const expoAudioShim = { - getRecordingPermissionsAsync: async () => ({ granted: true, status: 'granted' }), - requestRecordingPermissionsAsync: async () => ({ granted: true, status: 'granted' }), + getRecordingPermissionsAsync: async (): Promise<PermissionResponse> => ({ granted: true, status: 'granted' }), + requestRecordingPermissionsAsync: async (): Promise<PermissionResponse> => ({ granted: true, status: 'granted' }), };.github/workflows/react-native-cicd.yml (3)
302-423: Significant code duplication with build-electron job.The
extract_release_notesfunction and surrounding logic (lines 302-423) is nearly identical to lines 705-826 inbuild-electron. This violates DRY and creates maintenance burden.Consider extracting this into:
- A composite action in
.github/actions/prepare-release-notes/action.yml- Or a shared script in the repository that both jobs call
♻️ Example composite action structure
# .github/actions/prepare-release-notes/action.yml name: 'Prepare Release Notes' description: 'Extract and prepare release notes from PR body or commits' inputs: version: description: 'Version string for the release' required: true outputs: notes-file: description: 'Path to the generated release notes file' value: ${{ steps.prepare.outputs.notes-file }} runs: using: 'composite' steps: - id: prepare shell: bash run: | # ... shared release notes logic here ... env: GH_TOKEN: ${{ github.token }}Then use in both jobs:
- uses: ./.github/actions/prepare-release-notes with: version: '7.${{ github.run_number }}'
498-506: Updateactions/cacheto v4.The static analysis indicates
actions/cache@v3may have runner compatibility issues. This also applies to lines 104, 147, 200, and 610 in other jobs.♻️ Proposed fix
- name: 📦 Setup yarn cache - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: |
537-544: Quote secret interpolations to prevent word splitting.The static analysis flags SC2086 at the secret checks. While the values are unlikely to contain spaces, quoting is a defensive practice.
♻️ Proposed fix
- name: 🔍 Check Docker Hub credentials availability id: docker-creds run: | - if [[ -n "${{ secrets.DOCKER_USERNAME }}" && -n "${{ secrets.DOCKER_PASSWORD }}" ]]; then + if [[ -n "${{ secrets.DOCKER_USERNAME || '' }}" && -n "${{ secrets.DOCKER_PASSWORD || '' }}" ]]; then echo "available=true" >> $GITHUB_OUTPUT else echo "available=false" >> $GITHUB_OUTPUT fiAlternatively, use explicit quoting in the shell:
run: | - if [[ -n "${{ secrets.DOCKER_USERNAME }}" && -n "${{ secrets.DOCKER_PASSWORD }}" ]]; then + USERNAME="${{ secrets.DOCKER_USERNAME }}" + PASSWORD="${{ secrets.DOCKER_PASSWORD }}" + if [[ -n "$USERNAME" && -n "$PASSWORD" ]]; then echo "available=true" >> $GITHUB_OUTPUTpackage.json (1)
109-109: Move@types/mapbox-glto devDependencies.Type definition packages (
@types/*) are only needed at compile time and should typically be indevDependenciesrather thandependencies. This avoids unnecessarily including them in production builds.♻️ Suggested change
Move this entry from
dependenciestodevDependencies:"dependencies": { ... - "@types/mapbox-gl": "3.4.1", ... }, "devDependencies": { ... + "@types/mapbox-gl": "3.4.1", ... }src/components/maps/map-view.web.tsx (5)
59-59: Avoid usinganyfor the ref type.Per coding guidelines, strive for precise types instead of
any. Define a proper interface for the imperative handle.♻️ Suggested fix
+// Define the imperative handle interface +interface MapViewRef { + getMap: () => mapboxgl.Map | null; +} -export const MapView = forwardRef<any, MapViewProps>( +export const MapView = forwardRef<MapViewRef, MapViewProps>(
167-187: Avoidanytypes in Camera component.The ref type and
flyTooptions parameter useany, which violates coding guidelines.♻️ Suggested types
+interface CameraRef { + setCamera: (options: { + centerCoordinate?: [number, number]; + zoomLevel?: number; + heading?: number; + pitch?: number; + animationDuration?: number; + }) => void; + flyTo: (options: mapboxgl.FlyToOptions) => void; +} -export const Camera = forwardRef<any, CameraProps>(({ centerCoordinate, zoomLevel, heading, pitch, animationDuration = 1000, followUserLocation, followZoomLevel }, ref) => { +export const Camera = forwardRef<CameraRef, CameraProps>(({ centerCoordinate, zoomLevel, heading, pitch, animationDuration = 1000, followUserLocation, followZoomLevel }, ref) => {- flyTo: (options: any) => { + flyTo: (options: mapboxgl.FlyToOptions) => {
218-221: ArbitrarysetTimeoutdelay is fragile.Using a fixed 100ms timeout to trigger geolocation is a race condition waiting to happen. The control may not be ready, or it may be ready sooner.
♻️ Consider using the geolocate control's event
- // Trigger tracking after control is added - setTimeout(() => { - geolocateControl.current?.trigger(); - }, 100); + // Trigger tracking once the map is idle (control is ready) + const triggerOnIdle = () => { + geolocateControl.current?.trigger(); + }; + + if (map.loaded()) { + map.once('idle', triggerOnIdle); + } else { + map.once('load', () => { + map.once('idle', triggerOnIdle); + }); + }
349-366: Confusing control flow with multiple conditional returns.The nested conditional returns make cleanup logic hard to follow. Both branches remove the control, but via different return statements.
♻️ Cleaner structure
+ let onMapLoad: (() => void) | null = null; + // Auto-trigger to show user location if (map.loaded()) { geolocate.trigger(); } else { - const onMapLoad = () => { + onMapLoad = () => { geolocate.trigger(); }; map.on('load', onMapLoad); - - return () => { - map.off('load', onMapLoad); - map.removeControl(geolocate); - }; } return () => { + if (onMapLoad) { + map.off('load', onMapLoad); + } map.removeControl(geolocate); };
377-381: Consider more robust ID generation for MarkerView.Using
coordinate.join('-')creates IDs like"-98.5795--39.8283"for negative coordinates, which could be confusing or cause issues if used as element IDs.♻️ Suggested improvement
- <PointAnnotation id={`marker-${coordinate.join('-')}`} coordinate={coordinate}> + <PointAnnotation id={`marker-${coordinate[0]}_${coordinate[1]}`} coordinate={coordinate}>Or use a more explicit format:
marker-lng${coordinate[0]}-lat${coordinate[1]}src/services/push-notification.electron.ts (1)
68-83: Consider validating notification data structure before accessing store.The
handleNotificationClickmethod accessesdata?.eventCodebut then passes potentially undefined values to the store. Consider adding validation or defaults.♻️ Suggested improvement
private handleNotificationClick = (data: any): void => { logger.info({ message: 'Electron notification clicked', context: { data }, }); // Show the notification modal - if (data?.eventCode) { + if (data?.eventCode && data?.title) { usePushNotificationModalStore.getState().showNotificationModal({ eventCode: data.eventCode, title: data.title, - body: data.body || data.message, + body: data.body || data.message || '', data, }); } };public/service-worker.js (2)
11-12: Consider memory management forpendingNotificationsMap.The
pendingNotificationsMap stores notification data indefinitely. If a client opens but never sendsCLIENT_READY, entries will leak. Consider adding a timeout-based cleanup or maximum size limit.♻️ Optional: Add cleanup mechanism
// Store pending notification data with timestamp for cleanup const pendingNotifications = new Map(); const PENDING_TIMEOUT_MS = 30000; // 30 seconds // Clean up stale pending notifications periodically setInterval(() => { const now = Date.now(); for (const [clientId, entry] of pendingNotifications) { if (now - entry.timestamp > PENDING_TIMEOUT_MS) { pendingNotifications.delete(clientId); console.log('[Service Worker] Cleaned up stale pending notification:', clientId); } } }, 60000); // When storing: pendingNotifications.set(client.id, { data, timestamp: Date.now() });
15-28: Avoid logging potentially sensitive notification data in production.The
console.logon line 16 logs the entire push event which may contain sensitive information. Consider removing or reducing verbosity for production.♻️ Suggested change
self.addEventListener('push', function (event) { - console.log('[Service Worker] Push received:', event); + console.log('[Service Worker] Push received');src/services/push-notification.web.ts (1)
202-231: Replaceanywith a typed notification payload.
You already expecteventCode,title, andbody; using a typed payload keeps this safe (prefer a sharedPushNotificationDataif one exists). As per coding guidelines, avoidany.♻️ Suggested refactor
import { logger } from '@/lib/logging'; import { usePushNotificationModalStore } from '@/stores/push-notification/store'; +interface PushNotificationData { + eventCode?: string; + title?: string; + body?: string; + message?: string; + [key: string]: unknown; +} + @@ - async showLocalNotification(title: string, body: string, data?: any): Promise<void> { + async showLocalNotification(title: string, body: string, data?: PushNotificationData): Promise<void> {src/lib/platform.ts (2)
29-54: Avoidas anywhen reading electronAPI in platformName.
Window is augmented below, so the cast isn’t necessary. As per coding guidelines, avoidany.♻️ Suggested fix
- const electronPlatform = (window as any).electronAPI?.platform; + const electronPlatform = window.electronAPI?.platform;
57-68: Prefer a concrete orunknownpayload type instead ofany.
This keeps Electron interop type-safe across the app. As per coding guidelines, avoidany.♻️ Suggested fix
- showNotification: (options: { title: string; body: string; data: any }) => Promise<boolean>; - onNotificationClicked: (callback: (data: any) => void) => void; + showNotification: (options: { title: string; body: string; data: unknown }) => Promise<boolean>; + onNotificationClicked: (callback: (data: unknown) => void) => void;
| - name: � Download all Electron artifacts | ||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| path: electron-artifacts/ | ||
| pattern: electron-* | ||
| merge-multiple: false |
There was a problem hiding this comment.
Artifact download may fail if Windows/macOS matrix jobs haven't completed.
The download-artifact@v4 step attempts to download artifacts from all matrix jobs (electron-windows, electron-macos, electron-linux), but matrix jobs run in parallel. When the ubuntu-latest runner reaches this step, the Windows and macOS builds may still be in progress.
This will cause the release to be created with incomplete artifacts, or the download step to fail.
🐛 Recommended fix: Split into a separate release job
Create a dedicated release job that depends on build-electron completing all matrix runs:
release-electron:
needs: build-electron
if: (github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master')) || github.event_name == 'workflow_dispatch'
runs-on: ubuntu-latest
permissions:
contents: write
steps:
- name: 🏗 Checkout repository
uses: actions/checkout@v4
- name: 📋 Prepare Release Notes file
run: |
# ... release notes logic ...
env:
GH_TOKEN: ${{ github.token }}
- name: 📦 Download all Electron artifacts
uses: actions/download-artifact@v4
with:
path: electron-artifacts/
pattern: electron-*
merge-multiple: false
# ... rest of release steps ...This ensures all matrix jobs complete before attempting to download and release artifacts.
🤖 Prompt for AI Agents
In @.github/workflows/react-native-cicd.yml around lines 828 - 834, The artifact
download step currently in the matrix can run before Windows/macOS jobs finish;
move the "Download all Electron artifacts" logic out of the matrix and into a
dedicated release job (e.g., release-electron) that uses needs: build-electron
so it only runs after all matrix runs complete; in that release job include the
checkout step, prepare release notes step, and then the
actions/download-artifact@v4 step (using the same path/pattern/merge-multiple
settings) and guard the job with the same if-condition used for releases (push
to main/master or workflow_dispatch) so artifact downloads and subsequent
release steps always see all built artifacts.
| - name: 📦 Prepare Electron release artifacts | ||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||
| run: | | ||
| # Create release-artifacts directory | ||
| mkdir -p release-artifacts | ||
| # Copy all Electron artifacts to a flat directory for release | ||
| find electron-artifacts/ -type f \( -name "*.exe" -o -name "*.msi" -o -name "*.dmg" -o -name "*.zip" -o -name "*.AppImage" -o -name "*.deb" -o -name "*.rpm" \) -exec cp {} release-artifacts/ \; | ||
| echo "Release artifacts prepared:" | ||
| ls -lh release-artifacts/ |
There was a problem hiding this comment.
Add error handling for empty artifact collection.
The find command with -exec cp will succeed silently even if no matching files are found, resulting in an empty release-artifacts/ directory and a release with no downloadable files.
🐛 Proposed fix
- name: 📦 Prepare Electron release artifacts
if: ${{ matrix.os == 'ubuntu-latest' }}
run: |
# Create release-artifacts directory
mkdir -p release-artifacts
# Copy all Electron artifacts to a flat directory for release
find electron-artifacts/ -type f \( -name "*.exe" -o -name "*.msi" -o -name "*.dmg" -o -name "*.zip" -o -name "*.AppImage" -o -name "*.deb" -o -name "*.rpm" \) -exec cp {} release-artifacts/ \;
+ # Verify artifacts were collected
+ ARTIFACT_COUNT=$(find release-artifacts/ -type f | wc -l)
+ if [ "$ARTIFACT_COUNT" -eq 0 ]; then
+ echo "Error: No release artifacts found" >&2
+ exit 1
+ fi
+
echo "Release artifacts prepared:"
ls -lh release-artifacts/📝 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.
| - name: 📦 Prepare Electron release artifacts | |
| if: ${{ matrix.os == 'ubuntu-latest' }} | |
| run: | | |
| # Create release-artifacts directory | |
| mkdir -p release-artifacts | |
| # Copy all Electron artifacts to a flat directory for release | |
| find electron-artifacts/ -type f \( -name "*.exe" -o -name "*.msi" -o -name "*.dmg" -o -name "*.zip" -o -name "*.AppImage" -o -name "*.deb" -o -name "*.rpm" \) -exec cp {} release-artifacts/ \; | |
| echo "Release artifacts prepared:" | |
| ls -lh release-artifacts/ | |
| - name: 📦 Prepare Electron release artifacts | |
| if: ${{ matrix.os == 'ubuntu-latest' }} | |
| run: | | |
| # Create release-artifacts directory | |
| mkdir -p release-artifacts | |
| # Copy all Electron artifacts to a flat directory for release | |
| find electron-artifacts/ -type f \( -name "*.exe" -o -name "*.msi" -o -name "*.dmg" -o -name "*.zip" -o -name "*.AppImage" -o -name "*.deb" -o -name "*.rpm" \) -exec cp {} release-artifacts/ \; | |
| # Verify artifacts were collected | |
| ARTIFACT_COUNT=$(find release-artifacts/ -type f | wc -l) | |
| if [ "$ARTIFACT_COUNT" -eq 0 ]; then | |
| echo "Error: No release artifacts found" >&2 | |
| exit 1 | |
| fi | |
| echo "Release artifacts prepared:" | |
| ls -lh release-artifacts/ |
🤖 Prompt for AI Agents
In @.github/workflows/react-native-cicd.yml around lines 836 - 846, The "Prepare
Electron release artifacts" step can silently produce an empty
release-artifacts/ directory because the find ... -exec cp ... command succeeds
even when no files match; update this step to detect when no Electron artifacts
are found before copying (e.g. run find to count or list matches and fail the
step if none are returned) and emit an explicit error message and non-zero exit
to stop the workflow, referencing the existing find invocation and the
release-artifacts directory so you only proceed to cp/ls when artifacts exist.
| "electron": "40.0.0", | ||
| "electron-builder": "26.4.0", | ||
| "electron-squirrel-startup": "^1.0.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron version 40.0.0 latest release
💡 Result:
As of January 25, 2026, the latest stable Electron major release is 40.0.0. It was announced as available on January 13, 2026 and is installable via npm install electron@latest. [1][2]
Release automation for the v40.0.0 stable branch completed around January 15, 2026, and Electron’s lifecycle trackers list 40.0.0 as the latest 40.x release (published Jan 16, 2026). [3][4]
Install: npm i electron@latest (or pin: npm i electron@40.0.0). [1]
Sources: [1] [2] [3] [4]
Align version specifier for electron-squirrel-startup.
electron-squirrel-startup uses caret (^1.0.1) while electron and electron-builder use exact versions. For consistency with the other newly added Electron dependencies, consider using an exact version specifier.
🤖 Prompt for AI Agents
In `@package.json` around lines 198 - 200, The dependency spec for
electron-squirrel-startup should use an exact version like the other Electron
deps; update the package.json entry for "electron-squirrel-startup" to remove
the caret and pin it to "1.0.1" so it matches the exact-version style used by
"electron" and "electron-builder".
| // Render React children into the container using createPortal | ||
| if (children) { | ||
| ReactDOM.render(<>{children}</>, container); | ||
| } | ||
|
|
||
| // Determine marker options based on anchor prop | ||
| const markerOptions: mapboxgl.MarkerOptions = { | ||
| element: container, | ||
| }; | ||
|
|
||
| // Handle anchor prop - if it's a string, use it as mapbox anchor | ||
| if (typeof anchor === 'string') { | ||
| markerOptions.anchor = anchor as mapboxgl.Anchor; | ||
| } | ||
|
|
||
| markerRef.current = new mapboxgl.Marker(markerOptions) | ||
| .setLngLat(coordinate) | ||
| .addTo(map); | ||
|
|
||
| // If anchor is an {x, y} object, convert to pixel offset | ||
| if (typeof anchor === 'object' && anchor !== null && 'x' in anchor && 'y' in anchor) { | ||
| // Calculate offset based on container size | ||
| // Mapbox expects offset in pixels, anchor is typically 0-1 range | ||
| // Convert anchor position to offset (center is anchor {0.5, 0.5}) | ||
| const rect = container.getBoundingClientRect(); | ||
| const xOffset = (anchor.x - 0.5) * rect.width; | ||
| const yOffset = (anchor.y - 0.5) * rect.height; | ||
| markerRef.current.setOffset([xOffset, yOffset]); | ||
| } | ||
|
|
||
| if (title) { | ||
| markerRef.current.setPopup(new mapboxgl.Popup().setText(title)); | ||
| } | ||
|
|
||
| // Attach click listener to container | ||
| if (onSelected && containerRef.current) { | ||
| containerRef.current.addEventListener('click', onSelected); | ||
| } | ||
|
|
||
| return () => { | ||
| // Clean up click listener | ||
| if (onSelected && containerRef.current) { | ||
| containerRef.current.removeEventListener('click', onSelected); | ||
| } | ||
|
|
||
| // Unmount React children from the portal | ||
| if (children && containerRef.current) { | ||
| ReactDOM.unmountComponentAtNode(containerRef.current); | ||
| } | ||
|
|
||
| // Remove marker from map | ||
| markerRef.current?.remove(); | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Replace deprecated ReactDOM.render and unmountComponentAtNode with React 18+ createRoot API.
The pipeline is warning that ReactDOM.render (line 262) and ReactDOM.unmountComponentAtNode (line 307) are deprecated since React 18. Given you're using React 19, these must be updated to use the createRoot API.
🔧 Suggested fix using createRoot
import React, { forwardRef, useCallback, useEffect, useImperativeHandle, useRef, useState } from 'react';
-import ReactDOM from 'react-dom';
+import { createRoot, Root } from 'react-dom/client';
// ... in PointAnnotation component:
export const PointAnnotation: React.FC<PointAnnotationProps> = ({ id, coordinate, title, children, anchor = { x: 0.5, y: 0.5 }, onSelected }) => {
const map = React.useContext(MapContext);
const markerRef = useRef<mapboxgl.Marker | null>(null);
const containerRef = useRef<HTMLDivElement | null>(null);
+ const rootRef = useRef<Root | null>(null);
useEffect(() => {
if (!map || !coordinate) return;
// Create a container for React children
const container = document.createElement('div');
container.style.cursor = 'pointer';
containerRef.current = container;
// Render React children into the container using createRoot
if (children) {
- ReactDOM.render(<>{children}</>, container);
+ rootRef.current = createRoot(container);
+ rootRef.current.render(<>{children}</>);
}
// ... marker creation code ...
return () => {
// ... click listener cleanup ...
// Unmount React children
- if (children && containerRef.current) {
- ReactDOM.unmountComponentAtNode(containerRef.current);
+ if (rootRef.current) {
+ rootRef.current.unmount();
+ rootRef.current = null;
}
// Remove marker from map
markerRef.current?.remove();
};
}, [map, coordinate, id, children, anchor, onSelected, title]);🧰 Tools
🪛 GitHub Actions: React Native CI/CD
[warning] 262-262: ReactDOM.render is deprecated since React 18.0.0, use createRoot instead.
[warning] 275-275: prettier/prettier: Replace long chain with .setLngLat(coordinate)
307-307: ReactDOM.unmountComponentAtNode is deprecated since React 18.0.0, use root.unmount instead
🪛 GitHub Check: test
[warning] 307-307:
ReactDOM.unmountComponentAtNode is deprecated since React 18.0.0, use root.unmount instead, see https://reactjs.org/link/switch-to-createroot
[warning] 275-275:
Replace ⏎······.setLngLat(coordinate)⏎······ with .setLngLat(coordinate)
[warning] 262-262:
ReactDOM.render is deprecated since React 18.0.0, use createRoot instead, see https://reactjs.org/link/switch-to-createroot
🤖 Prompt for AI Agents
In `@src/components/maps/map-view.web.tsx` around lines 260 - 313, Replace
deprecated ReactDOM.render and ReactDOM.unmountComponentAtNode by creating and
storing a React root for the marker container: when mounting (where
ReactDOM.render currently is) call ReactDOM.createRoot(container) and
root.render(<>{children}</>), store that root (e.g., containerRootRef) so it can
be used later; on cleanup (where unmountComponentAtNode is used) call
containerRootRef.current?.unmount() instead of unmountComponentAtNode. Keep the
rest of the logic (markerRef, containerRef, anchor handling, onSelected
listener, popup) the same and ensure you only create the root when children
exist and clear containerRootRef after unmount.
| export const isElectron = | ||
| typeof window !== 'undefined' && | ||
| (!!(window as any).electronAPI || window.process?.type === 'renderer'); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file content
cat -n src/lib/platform.tsRepository: Resgrid/Unit
Length of output: 2552
🏁 Script executed:
# Search for Window interface augmentations and type definitions
fd -e "d.ts" -e "ts" -e "tsx" | xargs rg -l "Window\s*\{|interface\s+Window|declare\s+global" | head -20Repository: Resgrid/Unit
Length of output: 164
🏁 Script executed:
# Look for electronAPI references in the codebase
rg "electronAPI" --type ts --type tsx -B 2 -A 2Repository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
# Check Prettier configuration
fd -e ".prettierrc" -e "prettier.config" -e "prettierrc.json" -e "prettierrc.js"Repository: Resgrid/Unit
Length of output: 38
Remove as any cast and fix Prettier formatting in isElectron.
The Window interface is properly augmented for electronAPI, so the as any cast is unnecessary and violates the no-any guideline. Update to use the augmented type directly. Note: there's a similar as any cast at line 32 in platformName() that should also be removed.
♻️ Suggested fix
-export const isElectron =
- typeof window !== 'undefined' &&
+export const isElectron =
+ typeof window !== 'undefined' &&
(!!(window.electronAPI) || window.process?.type === 'renderer');Additionally, update line 32 from (window as any).electronAPI?.platform to window.electronAPI?.platform to maintain consistency.
🧰 Tools
🪛 GitHub Actions: React Native CI/CD
[warning] 12-12: prettier/prettier: Replace newline+typeof window !== 'undefined' with typeof window !== 'undefined' &&
🪛 GitHub Check: test
[warning] 12-12:
Replace ⏎··typeof·window·!==·'undefined'·&&·⏎· with typeof·window·!==·'undefined'·&&
🤖 Prompt for AI Agents
In `@src/lib/platform.ts` around lines 12 - 15, Remove the unnecessary "as any"
casts and fix Prettier formatting: update the isElectron export to use the
augmented Window type directly (no cast) and ensure its expression is formatted
correctly, and likewise replace (window as any).electronAPI?.platform with
window.electronAPI?.platform in the platformName function; keep the same logic
but use the typed window.electronAPI access to satisfy the no-any rule and run
Prettier to normalize spacing/line breaks.
| private sendClientReadyHandshake(): void { | ||
| if (navigator.serviceWorker.controller) { | ||
| navigator.serviceWorker.controller.postMessage({ | ||
| type: 'CLIENT_READY', | ||
| }); | ||
| logger.info({ | ||
| message: 'Sent CLIENT_READY handshake to service worker', | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's confirm the file exists and view the relevant section
cat -n src/services/push-notification.web.ts | head -100Repository: Resgrid/Unit
Length of output: 3774
🌐 Web query:
service worker controller null after ready resolves first install
💡 Result:
On the very first install/load, it’s normal for navigator.serviceWorker.ready to resolve while navigator.serviceWorker.controller is still null.
readyonly waits until there is an active service worker for the page’s registration; it does not guarantee the current page is already controlled by it. [1]controlleris only set when the current page is actually controlled by an active/activating worker; otherwise it’snull(alsonullon a force refresh). [2][3]
What to do
-
Most common/expected: reload or navigate once after install (control is gained on the next navigation).
-
Take control immediately (recommended for first install UX):
- In the SW, claim clients on activate (and often also skip waiting):
// sw.js
self.addEventListener("install", () => self.skipWaiting());
self.addEventListener("activate", (event) => event.waitUntil(self.clients.claim()));This is the standard fix when you want controller to become available without requiring a manual refresh. [4][5]
- In the page, wait for control explicitly:
Listen forcontrollerchangeand proceed when it fires (or whencontrolleris already non-null). [4]
Also verify:
- the SW scope actually includes the page URL (otherwise
controllerwill stay null). [4] - you’re not testing during a hard/force refresh, which can keep
controllernull for that load. [2][3]
Sources:
[1] MDN: ServiceWorkerContainer.ready [2] MDN: ServiceWorkerContainer.controller [3] W3C Service Workers spec note on force refresh and controller [4] Stack Overflow: controller null until refresh / controllerchange / scope [5] Stack Overflow: clients.claim() making controller available
Guarantee CLIENT_READY delivery by handling the first-install case where controller is null.
After navigator.serviceWorker.ready resolves, the service worker is active but the current page may not yet be controlled by it (especially on first install). This causes navigator.serviceWorker.controller to remain null, and the handshake is skipped silently.
Use this.registration?.active as a fallback to reach the active worker immediately, and listen for controllerchange to handle the case when the page gains control after initialization:
Suggested fix
private sendClientReadyHandshake(): void {
- if (navigator.serviceWorker.controller) {
- navigator.serviceWorker.controller.postMessage({
+ const controller = navigator.serviceWorker.controller ?? this.registration?.active;
+ if (controller) {
+ controller.postMessage({
type: 'CLIENT_READY',
});
logger.info({
message: 'Sent CLIENT_READY handshake to service worker',
});
+ return;
}
+
+ navigator.serviceWorker.addEventListener(
+ 'controllerchange',
+ () => {
+ if (navigator.serviceWorker.controller) {
+ navigator.serviceWorker.controller.postMessage({ type: 'CLIENT_READY' });
+ logger.info({ message: 'Sent CLIENT_READY handshake to service worker' });
+ }
+ },
+ { once: true }
+ );
}🤖 Prompt for AI Agents
In `@src/services/push-notification.web.ts` around lines 65 - 74,
sendClientReadyHandshake currently silently skips sending CLIENT_READY when
navigator.serviceWorker.controller is null (first-install case); update
sendClientReadyHandshake to: if controller exists postMessage and log; else if
this.registration?.active exists postMessage to this.registration.active and
log; additionally add a one-time 'controllerchange' listener that when
navigator.serviceWorker.controller becomes available posts CLIENT_READY and logs
then removes the listener to avoid duplicate sends; reference the
sendClientReadyHandshake method, navigator.serviceWorker.controller,
this.registration?.active and the 'controllerchange' event in your changes.
| private handleServiceWorkerMessage = (event: MessageEvent): void => { | ||
| if (event.data?.type === 'NOTIFICATION_CLICK') { | ||
| const data = event.data.data; | ||
| logger.info({ | ||
| message: 'Notification clicked from service worker', | ||
| context: { data }, | ||
| }); | ||
|
|
||
| // Show the notification modal | ||
| if (data.eventCode) { | ||
| usePushNotificationModalStore.getState().showNotificationModal({ | ||
| eventCode: data.eventCode, | ||
| title: data.title, | ||
| body: data.body || data.message, | ||
| data, | ||
| }); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and view the relevant lines
if [ -f "src/services/push-notification.web.ts" ]; then
echo "=== File found. Showing lines 75-100 for context ==="
head -100 "src/services/push-notification.web.ts" | tail -30
else
echo "File not found at src/services/push-notification.web.ts"
# Try to locate the file
find . -name "push-notification*" -type f 2>/dev/null | head -20
fiRepository: Resgrid/Unit
Length of output: 893
🏁 Script executed:
# Check the entire file for type definitions and context
wc -l "src/services/push-notification.web.ts"
# Look for any MessageEvent type definitions or interfaces in the file
rg -A 5 -B 5 "MessageEvent|event\.data" "src/services/push-notification.web.ts" | head -80
# Check if there are type definitions for the message payload
rg -B 10 -A 10 "NOTIFICATION_CLICK" "src/services/push-notification.web.ts"Repository: Resgrid/Unit
Length of output: 1058
🏁 Script executed:
# View the full file to understand context and check for type definitions
head -100 "src/services/push-notification.web.ts"
# Check if there's a type guard or interface for the event payload elsewhere
rg "interface.*Message|type.*Message" "src/services/push-notification.web.ts"
# Look for any assertion or type narrowing
ast-grep --pattern 'event.data.data'Repository: Resgrid/Unit
Length of output: 3151
Guard against undefined event.data.data before accessing properties.
The optional chaining on event.data?.type does not protect event.data.data from being undefined. If the service worker sends a NOTIFICATION_CLICK message without a data payload, accessing data.eventCode will throw a TypeError.
Suggested fix
- if (data.eventCode) {
+ if (data && data.eventCode) {
usePushNotificationModalStore.getState().showNotificationModal({
eventCode: data.eventCode,
title: data.title,
body: data.body || data.message,
data,
});
}🤖 Prompt for AI Agents
In `@src/services/push-notification.web.ts` around lines 79 - 97, The handler
handleServiceWorkerMessage currently assumes event.data.data exists and then
accesses data.eventCode, which can throw if the SW sends a NOTIFICATION_CLICK
without a payload; guard against undefined by reading const data =
event.data?.data ?? undefined (or return early if missing) and only proceed to
log and call usePushNotificationModalStore.getState().showNotificationModal when
data is an object with the expected fields (check data && data.eventCode),
ensuring you don't access data.eventCode, data.title, data.body, or data.message
when data is undefined.
| async unsubscribe(): Promise<boolean> { | ||
| if (!this.pushSubscription) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| await this.pushSubscription.unsubscribe(); | ||
| this.pushSubscription = null; | ||
| logger.info({ | ||
| message: 'Successfully unsubscribed from push notifications', | ||
| }); | ||
| return true; | ||
| } catch (error) { | ||
| logger.error({ | ||
| message: 'Failed to unsubscribe from push notifications', | ||
| context: { error }, | ||
| }); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "push-notification.web.ts" -type fRepository: Resgrid/Unit
Length of output: 95
🏁 Script executed:
wc -l src/services/push-notification.web.tsRepository: Resgrid/Unit
Length of output: 97
🏁 Script executed:
cat -n src/services/push-notification.web.tsRepository: Resgrid/Unit
Length of output: 8887
Unsubscribe must retrieve and clear persisted browser subscriptions on every call.
After a page reload, this.pushSubscription becomes null but the browser's push subscription persists. The current code returns true without actually unsubscribing, leaving users subscribed despite successful method return.
Suggested fix
async unsubscribe(): Promise<boolean> {
- if (!this.pushSubscription) {
- return true;
- }
-
try {
+ if (!this.registration) {
+ await this.initialize();
+ }
+ if (!this.pushSubscription && this.registration) {
+ this.pushSubscription = await this.registration.pushManager.getSubscription();
+ }
+ if (!this.pushSubscription) {
+ return true;
+ }
await this.pushSubscription.unsubscribe();
this.pushSubscription = null;
logger.info({
message: 'Successfully unsubscribed from push notifications',
});
return true;📝 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.
| async unsubscribe(): Promise<boolean> { | |
| if (!this.pushSubscription) { | |
| return true; | |
| } | |
| try { | |
| await this.pushSubscription.unsubscribe(); | |
| this.pushSubscription = null; | |
| logger.info({ | |
| message: 'Successfully unsubscribed from push notifications', | |
| }); | |
| return true; | |
| } catch (error) { | |
| logger.error({ | |
| message: 'Failed to unsubscribe from push notifications', | |
| context: { error }, | |
| }); | |
| return false; | |
| } | |
| async unsubscribe(): Promise<boolean> { | |
| try { | |
| if (!this.registration) { | |
| await this.initialize(); | |
| } | |
| if (!this.pushSubscription && this.registration) { | |
| this.pushSubscription = await this.registration.pushManager.getSubscription(); | |
| } | |
| if (!this.pushSubscription) { | |
| return true; | |
| } | |
| await this.pushSubscription.unsubscribe(); | |
| this.pushSubscription = null; | |
| logger.info({ | |
| message: 'Successfully unsubscribed from push notifications', | |
| }); | |
| return true; | |
| } catch (error) { | |
| logger.error({ | |
| message: 'Failed to unsubscribe from push notifications', | |
| context: { error }, | |
| }); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/services/push-notification.web.ts` around lines 171 - 189, The
unsubscribe() method returns early when this.pushSubscription is null but must
still locate any persisted browser subscription and clear it; update
unsubscribe() so that if this.pushSubscription is falsy it attempts to retrieve
the active subscription via the Service Worker registration (use
registration.pushManager.getSubscription() or equivalent) and assign it to
this.pushSubscription before proceeding, then call
this.pushSubscription.unsubscribe(), set this.pushSubscription = null, and clear
any persisted client-side record of the subscription (localStorage/IndexedDB
entry if used). Ensure errors from getSubscription() and unsubscribe() are
caught and logged via logger.error and the method still returns a boolean
indicating success/failure.
| static isSupported(): boolean { | ||
| return 'serviceWorker' in navigator && 'PushManager' in window && 'Notification' in window; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "push-notification" --type fRepository: Resgrid/Unit
Length of output: 495
🏁 Script executed:
wc -l src/services/push-notification.web.tsRepository: Resgrid/Unit
Length of output: 97
🏁 Script executed:
sed -n '245,255p' src/services/push-notification.web.tsRepository: Resgrid/Unit
Length of output: 340
🏁 Script executed:
cat package.json | grep -A5 -B5 "next\|ssr"Repository: Resgrid/Unit
Length of output: 855
🏁 Script executed:
grep -r "push-notification.web" --include="*.ts" --include="*.tsx" --include="*.js"Repository: Resgrid/Unit
Length of output: 38
🏁 Script executed:
cat -n src/services/push-notification.tsRepository: Resgrid/Unit
Length of output: 23309
🏁 Script executed:
cat -n src/services/push-notification.web.tsRepository: Resgrid/Unit
Length of output: 8887
🏁 Script executed:
cat -n src/services/__tests__/push-notification.test.tsRepository: Resgrid/Unit
Length of output: 21008
🏁 Script executed:
grep -r "webPushNotificationService\|push-notification.web\|WebPushNotificationService" --include="*.ts" --include="*.tsx" --include="*.js" --include="*.json"Repository: Resgrid/Unit
Length of output: 722
🏁 Script executed:
grep -E "react-native-web|ssr|server" package.jsonRepository: Resgrid/Unit
Length of output: 272
🏁 Script executed:
rg "typeof window|typeof navigator" --type ts --type tsxRepository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
rg "typeof window|typeof navigator" --include="*.ts" --include="*.tsx"Repository: Resgrid/Unit
Length of output: 497
🏁 Script executed:
grep -r "typeof window\|typeof navigator" src/ --include="*.ts" --include="*.tsx" | head -20Repository: Resgrid/Unit
Length of output: 356
🏁 Script executed:
cat -n src/lib/platform.tsRepository: Resgrid/Unit
Length of output: 2552
Guard isSupported for non-browser contexts.
Direct access to navigator and window without typeof checks will throw during SSR or in Node.js test environments. The codebase already uses this pattern in src/lib/platform.ts and src/components/ui/gluestack-ui-provider/index.web.tsx.
🐛 Suggested fix
static isSupported(): boolean {
- return 'serviceWorker' in navigator && 'PushManager' in window && 'Notification' in window;
+ return (
+ typeof window !== 'undefined' &&
+ typeof navigator !== 'undefined' &&
+ 'serviceWorker' in navigator &&
+ 'PushManager' in window &&
+ 'Notification' in window
+ );
}📝 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.
| static isSupported(): boolean { | |
| return 'serviceWorker' in navigator && 'PushManager' in window && 'Notification' in window; | |
| static isSupported(): boolean { | |
| return ( | |
| typeof window !== 'undefined' && | |
| typeof navigator !== 'undefined' && | |
| 'serviceWorker' in navigator && | |
| 'PushManager' in window && | |
| 'Notification' in window | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@src/services/push-notification.web.ts` around lines 250 - 251, The static
isSupported() method in the PushNotification class (or module) directly accesses
navigator and window which breaks in SSR/Node; update isSupported() to first
guard that typeof window !== 'undefined' and typeof navigator !== 'undefined'
before checking 'serviceWorker' in navigator and 'PushManager'/'Notification' in
window (or reuse the existing platform guard helper used elsewhere) so the
method returns false in non‑browser environments and never throws.
| @@ -1,4 +1,4 @@ | |||
| import notifee, { AndroidImportance } from '@notifee/react-native'; | |||
| import notifee, { AndroidForegroundServiceType, AndroidImportance } from '@notifee/react-native'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if node_modules exists and find notifee type definitions
if [ -d "node_modules" ]; then
echo "=== Searching for notifee types ==="
find node_modules/@notifee -name "*.d.ts" -o -name "*.ts" | head -20
fi
# Check package.json for `@notifee/react-native` version
echo ""
echo "=== Package version ==="
if [ -f "package.json" ]; then
cat package.json | jq '.dependencies["@notifee/react-native"] // .devDependencies["@notifee/react-native"] // "Not found"'
fiRepository: Resgrid/Unit
Length of output: 199
🌐 Web query:
AndroidForegroundServiceType enum values @notifee/react-native documentation
💡 Result:
AndroidForegroundServiceType enum values in @notifee/react-native are:(docs.page)
FOREGROUND_SERVICE_TYPE_MANIFEST = -1FOREGROUND_SERVICE_TYPE_DATA_SYNC = 1FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK = 2FOREGROUND_SERVICE_TYPE_PHONE_CALL = 4FOREGROUND_SERVICE_TYPE_LOCATION = 8FOREGROUND_SERVICE_TYPE_CONNECTED_DEVICE = 16FOREGROUND_SERVICE_TYPE_MEDIA_PROJECTION = 32FOREGROUND_SERVICE_TYPE_CAMERA = 64FOREGROUND_SERVICE_TYPE_MICROPHONE = 128FOREGROUND_SERVICE_TYPE_HEALTH = 256FOREGROUND_SERVICE_TYPE_REMOTE_MESSAGING = 512FOREGROUND_SERVICE_TYPE_SYSTEM_EXEMPTED = 1024FOREGROUND_SERVICE_TYPE_SHORT_SERVICE = 2048FOREGROUND_SERVICE_TYPE_MEDIA_PROCESSING = 8192FOREGROUND_SERVICE_TYPE_SPECIAL_USE = 1073741824
Citations:
- 1: https://docs.page/invertase/notifee/react-native/reference/Enumeration.AndroidForegroundServiceType
TypeScript build failure: AndroidForegroundServiceType.MICROPHONE is not a valid enum member.
The @notifee/react-native enum uses the naming convention FOREGROUND_SERVICE_TYPE_*. The correct member is FOREGROUND_SERVICE_TYPE_MICROPHONE (value 128), not MICROPHONE.
Update line 229 to use the correct enum member: AndroidForegroundServiceType.FOREGROUND_SERVICE_TYPE_MICROPHONE.
Note: If the original magic number [2] was intentional, it maps to FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK—verify which service type is actually needed.
🤖 Prompt for AI Agents
In `@src/stores/app/livekit-store.ts` at line 1, The build fails because the code
uses an invalid enum member AndroidForegroundServiceType.MICROPHONE; update the
usage to the correct Notifee enum name
AndroidForegroundServiceType.FOREGROUND_SERVICE_TYPE_MICROPHONE (or, if the
original numeric intent was the media playback flag, use
AndroidForegroundServiceType.FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK) wherever
AndroidForegroundServiceType.MICROPHONE is referenced (e.g., in the livekit
foreground service setup code) so TypeScript compiles cleanly.
Summary by CodeRabbit
New Features
Documentation
Build & Infrastructure
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.