Develop #202
Develop #202
Conversation
📝 WalkthroughWalkthroughAdds multi-platform delivery: Docker runtime injection and compose, web service worker and web/Electron push services, Electron app and build config, CI/CD jobs for web/docker/electron with release-note extraction, unified Mapbox wrapper (web + native) with web shims and metro resolver, many runtime/type adjustments, and supporting docs/scripts. 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
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 (2 warnings)
✅ 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;
| "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".
| 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.
| 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.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
179-200: Add@types/react-domto devDependencies to fix type-checking failures.
react-dom(19.0.0) is imported in the codebase but lacks TypeScript type declarations. Withstrict: trueenabled in tsconfig.json and type-checking run in CI viayarn check-all, TypeScript will report TS7016 and block the build. Add@types/react-domaligned with the react-dom version (e.g.,"@types/react-dom": "~19.0.0")..github/workflows/react-native-cicd.yml (1)
353-354: Remove the replacement character in the step name.There’s a garbled character (
�) in the step label; it will show up in logs.🛠️ Proposed fix
- - name: �📦 Create Release + - name: 📦 Create Release
🤖 Fix all issues with AI agents
In @.github/workflows/react-native-cicd.yml:
- Around line 420-424: The step name contains a garbled replacement character in
the step label "� Check Docker Hub credentials availability" (step id:
docker-creds); edit the workflow to remove the invalid character and replace the
label with a clean string such as "Check Docker Hub credentials availability" so
the step name displays correctly in logs.
- Around line 609-622: The current pipeline stores matching paths in FILES and
pipes them through echo | xargs which breaks filenames with spaces; replace the
existence check and copying with a null-delimited find/xargs or a read -d ''
loop: first check for any artifact with find electron-artifacts/ -type f (same
name globs) -print -quit and exit if none, then mkdir -p release-artifacts and
copy using find ... -print0 | xargs -0 cp -t release-artifacts/ (or: find ...
-print0 | while IFS= read -r -d '' file; do cp "$file" release-artifacts/; done)
so FILES, find, xargs, cp and release-artifacts are used safely with filenames
that contain spaces.
In `@package.json`:
- Around line 46-54: The electron dev flow is racing because Expo starts web on
port 19006 by default while electron:dev and electron/main.js expect 8081;
update the "electron:dev" npm script to set EXPO_WEB_PORT=8081 when launching
the web server (i.e., prefix the web start command that "yarn web" triggers with
cross-env EXPO_WEB_PORT=8081) so Expo serves on 8081 and matches the hardcoded
port used by electron/main.js, or alternatively configure Expo's web port in
your Expo web settings to 8081 to achieve the same alignment.
In `@scripts/extract-release-notes.sh`:
- Around line 4-9: The script currently assumes a "7." prefix when building the
release-notes header; instead accept and use the full version string passed as
an argument (the existing positional vars RUN_NUMBER, EVENT_NAME, REPOSITORY,
SHA, and GH_TOKEN indicate arguments are already used) and remove any hardcoded
"7." concatenation where the release header is assembled (also update the other
occurrence around the later section noted). Replace the code that prepends "7."
with direct use of the version argument so the header uses the exact version
string supplied by the workflow.
- Around line 49-50: The current final trim line removes leading whitespace on
every line in the variable notes (the sed invocation that uses s/^[[:space:]]*//
- s/[[:space:]]*$//), which mangles Markdown indentation; change the trimming
logic for notes to preserve per-line leading indentation by removing only
trailing whitespace (keep s/[[:space:]]*$//) or otherwise trim only the overall
leading/trailing blank lines instead of stripping leading spaces from each line
so nested lists and code blocks remain intact.
In `@src/components/maps/map-view.web.tsx`:
- Around line 218-227: The setTimeout that calls
geolocateControl.current?.trigger() should be stored and cleared during cleanup
to avoid it firing after the control is removed: create a local variable (e.g.,
triggerTimeoutId) to hold the ID returned by setTimeout where the trigger is
scheduled, and in the return cleanup block call clearTimeout(triggerTimeoutId)
before removing/setting geolocateControl.current to null; ensure the timeout
variable is in the same scope as the cleanup so it can be referenced.
- Around line 278-280: The chained Mapbox call should keep setLngLat on the same
line for Prettier consistency: update the expression that assigns
markerRef.current (where you call new
mapboxgl.Marker(markerOptions).setLngLat(coordinate).addTo(map)) so that
setLngLat is chained on the same line as new mapboxgl.Marker(...), preserving
the same call order (new
mapboxgl.Marker(markerOptions).setLngLat(coordinate).addTo(map)) and no other
logic changes.
- Around line 8-10: Replace the default ReactDOM import with the client entry
and typed root: import { createRoot, type Root } from 'react-dom/client'; update
any calls of ReactDOM.createRoot(...) to use createRoot(...) and remove any
(any) casts; type the ref that holds the root as containerRootRef:
React.MutableRefObject<Root | null> (or Root | null where used) so you no longer
need to cast to any and TS7016 is resolved; ensure any cleanup/unmount logic
uses containerRootRef.current?.unmount() accordingly.
In `@src/services/push-notification.web.ts`:
- Around line 224-228: Remove the trailing whitespace from the lines around the
unsubscribe block: the line with "const success = await
this.pushSubscription.unsubscribe();" and the line setting
"this.pushSubscription = null;" (inside the if (success) block) so they have no
trailing spaces; then run Prettier/formatting to ensure CI passes.
🧹 Nitpick comments (4)
src/services/push-notification.web.ts (1)
267-267: Avoid usinganytype for thedataparameter.Per coding guidelines, prefer precise types over
any. Consider defining an interface that matches the expected notification data structure used by the push notification modal store.♻️ Suggested interface
interface NotificationData { eventCode?: string; title?: string; body?: string; message?: string; [key: string]: unknown; } async showLocalNotification(title: string, body: string, data?: NotificationData): Promise<void> {src/lib/platform.ts (1)
62-66: Prefer a typed payload overanyinelectronAPI.
data: anyand(data: any) => voidweaken type safety. Considerunknownor a concrete notification payload interface to keep call sites typed.As per coding guidelines, avoid
any.src/components/maps/map-view.web.tsx (2)
141-144: Use ternary conditional rendering instead of&&.This file uses
&&for rendering, but the guidelines require?:for conditional UI.♻️ Suggested change
- <div ref={mapContainer} data-testid={testID} style={{ width: '100%', height: '100%', ...style }}> - {isLoaded && <MapContext.Provider value={map.current}>{children}</MapContext.Provider>} - </div> + <div ref={mapContainer} data-testid={testID} style={{ width: '100%', height: '100%', ...style }}> + {isLoaded ? <MapContext.Provider value={map.current}>{children}</MapContext.Provider> : null} + </div>As per coding guidelines, use the conditional operator for rendering.
58-84: Prefer typed ref handles overanyfor MapView/Camera.
forwardRef<any>andflyTo(options: any)reduce type safety. Consider defining handle interfaces and usingmapboxgl.FlyToOptionsfor stronger typing.As per coding guidelines, avoid
any.Also applies to: 167-186
| - name: � Check Docker Hub credentials availability | ||
| id: docker-creds | ||
| run: | | ||
| if [[ -n "${{ secrets.DOCKER_USERNAME }}" && -n "${{ secrets.DOCKER_PASSWORD }}" ]]; then | ||
| echo "available=true" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Remove the replacement character in the step name.
There’s a garbled character (�) in the step label; it will show up in logs.
🛠️ Proposed fix
- - name: � Check Docker Hub credentials availability
+ - name: Check Docker Hub credentials availability🧰 Tools
🪛 actionlint (1.7.10)
422-422: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
422-422: shellcheck reported issue in this script: SC2086:info:4:29: Double quote to prevent globbing and word splitting
(shellcheck)
🤖 Prompt for AI Agents
In @.github/workflows/react-native-cicd.yml around lines 420 - 424, The step
name contains a garbled replacement character in the step label "� Check Docker
Hub credentials availability" (step id: docker-creds); edit the workflow to
remove the invalid character and replace the label with a clean string such as
"Check Docker Hub credentials availability" so the step name displays correctly
in logs.
| - name: 📦 Prepare Electron release artifacts | ||
| run: | | ||
| # Find all matching Electron artifacts | ||
| FILES=$(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" \)) | ||
| if [ -z "$FILES" ]; then | ||
| echo "Error: No Electron artifacts found in electron-artifacts/ directory." | ||
| exit 1 | ||
| fi | ||
| # Create release-artifacts directory and copy files | ||
| mkdir -p release-artifacts | ||
| echo "$FILES" | xargs -I {} cp {} release-artifacts/ | ||
There was a problem hiding this comment.
Handle artifact filenames with spaces when copying.
echo "$FILES" | xargs ... splits on whitespace, so Electron artifacts with spaces (common for installers) can be dropped. Use a null-delimited pipeline and a separate existence check.
🛠️ Proposed fix
- # Find all matching Electron artifacts
- FILES=$(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" \))
-
- if [ -z "$FILES" ]; then
- echo "Error: No Electron artifacts found in electron-artifacts/ directory."
- exit 1
- fi
-
- # Create release-artifacts directory and copy files
- mkdir -p release-artifacts
- echo "$FILES" | xargs -I {} cp {} release-artifacts/
+ # Find all matching Electron artifacts (safe for spaces)
+ if ! 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" \) -print -quit | grep -q .; then
+ echo "Error: No Electron artifacts found in electron-artifacts/ directory."
+ exit 1
+ fi
+
+ # Create release-artifacts directory and copy files
+ mkdir -p release-artifacts
+ 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" \) -print0 \
+ | xargs -0 -I {} cp {} release-artifacts/🤖 Prompt for AI Agents
In @.github/workflows/react-native-cicd.yml around lines 609 - 622, The current
pipeline stores matching paths in FILES and pipes them through echo | xargs
which breaks filenames with spaces; replace the existence check and copying with
a null-delimited find/xargs or a read -d '' loop: first check for any artifact
with find electron-artifacts/ -type f (same name globs) -print -quit and exit if
none, then mkdir -p release-artifacts and copy using find ... -print0 | xargs -0
cp -t release-artifacts/ (or: find ... -print0 | while IFS= read -r -d '' file;
do cp "$file" release-artifacts/; done) so FILES, find, xargs, cp and
release-artifacts are used safely with filenames that contain spaces.
| "e2e-test": "maestro test .maestro/ -e APP_ID=com.obytes.development", | ||
| "web:build": "cross-env EXPO_NO_DOTENV=1 expo export --platform web", | ||
| "web:staging": "cross-env APP_ENV=staging yarn run web", | ||
| "web:production": "cross-env APP_ENV=production yarn run web", | ||
| "electron:dev": "concurrently \"yarn web\" \"wait-on http://localhost:8081 && electron .\"", | ||
| "electron:build": "yarn web:build && electron-builder --config electron-builder.config.js", | ||
| "electron:build:mac": "yarn web:build && electron-builder --config electron-builder.config.js --mac", | ||
| "electron:build:win": "yarn web:build && electron-builder --config electron-builder.config.js --win", | ||
| "electron:build:linux": "yarn web:build && electron-builder --config electron-builder.config.js --linux" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full package.json to understand the setup
cat -n package.json | head -100Repository: Resgrid/Unit
Length of output: 6372
🏁 Script executed:
# Look for expo or electron configuration files
fd -e config.js -e config.json -e config.ts | grep -E "(expo|electron|app)" | head -20Repository: Resgrid/Unit
Length of output: 96
🏁 Script executed:
# Search for any EXPO_WEB_PORT or port-related configuration
rg "EXPO_WEB_PORT|8081|19006" --type json --type js --type ts --type envRepository: Resgrid/Unit
Length of output: 83
🏁 Script executed:
# Check if there's an expo.config.js or app.json
fd -e "expo.config.js" -o -e "app.json" -o -e ".env" -o -e ".env.local" | head -20Repository: Resgrid/Unit
Length of output: 172
🏁 Script executed:
# Check app.config.ts for web/electron port configuration
cat -n app.config.tsRepository: Resgrid/Unit
Length of output: 7596
🏁 Script executed:
# Check electron-builder.config.js
cat -n electron-builder.config.jsRepository: Resgrid/Unit
Length of output: 2913
🏁 Script executed:
# Search for any port-related configuration in the codebase
rg "8081|19006" --type ts --type js | grep -v node_modules | head -30Repository: Resgrid/Unit
Length of output: 181
🏁 Script executed:
# Search for any EXPO_WEB_PORT or port configuration that might set it to 8081
rg "EXPO_WEB_PORT|expo.*port|web.*port" --type ts --type js --type json | head -40Repository: Resgrid/Unit
Length of output: 1397
🏁 Script executed:
# Check env.ts or env configuration files
fd -e "env.ts" -o -e "env.js" | head -10Repository: Resgrid/Unit
Length of output: 172
🏁 Script executed:
# Search for env files more carefully
find . -maxdepth 2 -name "env*" -type f | grep -v node_modules | head -20Repository: Resgrid/Unit
Length of output: 64
🏁 Script executed:
# Check if there's a .env or .env.development file
ls -la | grep -i envRepository: Resgrid/Unit
Length of output: 460
🏁 Script executed:
# Look at the electron main.js to understand the port usage better
cat -n electron/main.js | head -50Repository: Resgrid/Unit
Length of output: 1941
🏁 Script executed:
# Check env.js to see if EXPO_WEB_PORT is configured there
cat -n env.js | grep -i "port\|8081\|19006"Repository: Resgrid/Unit
Length of output: 778
🏁 Script executed:
# Check if there's any Metro or bundler configuration
fd -e "metro.config.js" -o -e "rn-cli.config.js" | head -10Repository: Resgrid/Unit
Length of output: 172
🏁 Script executed:
# Look at the full electron/main.js to confirm the port usage
wc -l electron/main.jsRepository: Resgrid/Unit
Length of output: 76
🏁 Script executed:
# Let me check if there's a custom expo configuration that sets the web port
grep -r "webPort\|WEB_PORT\|8081" --include="*.js" --include="*.ts" --include="*.json" . --exclude-dir=node_modules | grep -v ".next\|dist\|build" | head -20Repository: Resgrid/Unit
Length of output: 167
Align Expo web port with electron dev server expectation.
The electron:dev script waits on http://localhost:8081 and electron/main.js (line 36) hardcodes this port, but expo start --web defaults to port 19006 when no port is explicitly configured. Set EXPO_WEB_PORT=8081 in the script or configure the port in Expo's web settings to prevent connection failures.
🤖 Prompt for AI Agents
In `@package.json` around lines 46 - 54, The electron dev flow is racing because
Expo starts web on port 19006 by default while electron:dev and electron/main.js
expect 8081; update the "electron:dev" npm script to set EXPO_WEB_PORT=8081 when
launching the web server (i.e., prefix the web start command that "yarn web"
triggers with cross-env EXPO_WEB_PORT=8081) so Expo serves on 8081 and matches
the hardcoded port used by electron/main.js, or alternatively configure Expo's
web port in your Expo web settings to 8081 to achieve the same alignment.
| # Argument: github_run_number, github_event_name, github_repository, github_sha, github_token | ||
| RUN_NUMBER=$1 | ||
| EVENT_NAME=$2 | ||
| REPOSITORY=$3 | ||
| SHA=$4 | ||
| export GH_TOKEN=$5 |
There was a problem hiding this comment.
Avoid hardcoding the “7.” prefix in the release-notes header.
The workflow passes a full version string, so prefixing again yields 7.7.x and couples the script to one scheme. Use the version argument directly.
🛠️ Proposed fix
-# Argument: github_run_number, github_event_name, github_repository, github_sha, github_token
-RUN_NUMBER=$1
+# Argument: version, github_event_name, github_repository, github_sha, github_token
+VERSION=$1- echo "## Version 7.$RUN_NUMBER - $(date +%Y-%m-%d)"
+ echo "## Version $VERSION - $(date +%Y-%m-%d)"Also applies to: 116-120
🤖 Prompt for AI Agents
In `@scripts/extract-release-notes.sh` around lines 4 - 9, The script currently
assumes a "7." prefix when building the release-notes header; instead accept and
use the full version string passed as an argument (the existing positional vars
RUN_NUMBER, EVENT_NAME, REPOSITORY, SHA, and GH_TOKEN indicate arguments are
already used) and remove any hardcoded "7." concatenation where the release
header is assembled (also update the other occurrence around the later section
noted). Replace the code that prepends "7." with direct use of the version
argument so the header uses the exact version string supplied by the workflow.
| # Final trim | ||
| notes="$(printf '%s\n' "$notes" | sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//')" |
There was a problem hiding this comment.
Preserve indentation to avoid mangling Markdown.
Stripping leading whitespace from every line can break nested lists or code blocks in release notes. Since blank lines are already trimmed, consider dropping the per-line trim.
🛠️ Proposed fix
- # Final trim
- notes="$(printf '%s\n' "$notes" | sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//')"🤖 Prompt for AI Agents
In `@scripts/extract-release-notes.sh` around lines 49 - 50, The current final
trim line removes leading whitespace on every line in the variable notes (the
sed invocation that uses s/^[[:space:]]*// - s/[[:space:]]*$//), which mangles
Markdown indentation; change the trimming logic for notes to preserve per-line
leading indentation by removing only trailing whitespace (keep
s/[[:space:]]*$//) or otherwise trim only the overall leading/trailing blank
lines instead of stripping leading spaces from each line so nested lists and
code blocks remain intact.
| import React, { forwardRef, useCallback, useEffect, useImperativeHandle, useRef, useState } from 'react'; | ||
| import ReactDOM from 'react-dom'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/maps/map-view.web.tsx | head -50Repository: Resgrid/Unit
Length of output: 2011
🏁 Script executed:
grep -n "containerRootRef\|createRoot\|react-dom" src/components/maps/map-view.web.tsxRepository: Resgrid/Unit
Length of output: 464
🏁 Script executed:
wc -l src/components/maps/map-view.web.tsxRepository: Resgrid/Unit
Length of output: 96
🏁 Script executed:
grep -E "\"react\"|'react'" package.json | head -5Repository: Resgrid/Unit
Length of output: 78
🏁 Script executed:
sed -n '260,270p' src/components/maps/map-view.web.tsxRepository: Resgrid/Unit
Length of output: 414
🏁 Script executed:
grep -n " && " src/components/maps/map-view.web.tsx | head -10Repository: Resgrid/Unit
Length of output: 494
🏁 Script executed:
sed -n '300,320p' src/components/maps/map-view.web.tsxRepository: Resgrid/Unit
Length of output: 664
Use react-dom/client + typed createRoot (fixes TS7016).
The current code imports ReactDOM from 'react-dom' and casts it to any to access createRoot, which doesn't exist in that module. React 19 is installed, and createRoot is only available from react-dom/client. Import from the correct module and type containerRootRef as Root | null to remove the type casting and satisfy the type checker.
🛠 Proposed fix
-import ReactDOM from 'react-dom';
+import { createRoot, Root } from 'react-dom/client';
- const containerRootRef = useRef<any>(null);
+ const containerRootRef = useRef<Root | null>(null);
- const root = (ReactDOM as any).createRoot(container);
+ const root = createRoot(container);
root.render(<>{children}</>);
containerRootRef.current = root;🧰 Tools
🪛 GitHub Actions: React Native CI/CD
[error] 9-9: TS7016: Could not find a declaration file for module 'react-dom'. '/home/runner/work/Unit/Unit/node_modules/react-dom/index.js' implicitly has an 'any' type.
🪛 GitHub Check: test
[failure] 9-9:
Could not find a declaration file for module 'react-dom'. '/home/runner/work/Unit/Unit/node_modules/react-dom/index.js' implicitly has an 'any' type.
🤖 Prompt for AI Agents
In `@src/components/maps/map-view.web.tsx` around lines 8 - 10, Replace the
default ReactDOM import with the client entry and typed root: import {
createRoot, type Root } from 'react-dom/client'; update any calls of
ReactDOM.createRoot(...) to use createRoot(...) and remove any (any) casts; type
the ref that holds the root as containerRootRef: React.MutableRefObject<Root |
null> (or Root | null where used) so you no longer need to cast to any and
TS7016 is resolved; ensure any cleanup/unmount logic uses
containerRootRef.current?.unmount() accordingly.
| // Trigger tracking after control is added | ||
| setTimeout(() => { | ||
| geolocateControl.current?.trigger(); | ||
| }, 100); | ||
|
|
||
| return () => { | ||
| if (geolocateControl.current) { | ||
| map.removeControl(geolocateControl.current); | ||
| geolocateControl.current = null; | ||
| } |
There was a problem hiding this comment.
Clear the geolocate trigger timeout on cleanup.
If the component unmounts quickly, the timeout can fire after the control is removed. Store the timeout ID and clear it in the cleanup.
🛠 Proposed fix
- setTimeout(() => {
- geolocateControl.current?.trigger();
- }, 100);
+ const triggerTimeout = setTimeout(() => {
+ geolocateControl.current?.trigger();
+ }, 100);
return () => {
+ clearTimeout(triggerTimeout);
if (geolocateControl.current) {
map.removeControl(geolocateControl.current);
geolocateControl.current = null;
}
};🤖 Prompt for AI Agents
In `@src/components/maps/map-view.web.tsx` around lines 218 - 227, The setTimeout
that calls geolocateControl.current?.trigger() should be stored and cleared
during cleanup to avoid it firing after the control is removed: create a local
variable (e.g., triggerTimeoutId) to hold the ID returned by setTimeout where
the trigger is scheduled, and in the return cleanup block call
clearTimeout(triggerTimeoutId) before removing/setting geolocateControl.current
to null; ensure the timeout variable is in the same scope as the cleanup so it
can be referenced.
| markerRef.current = new mapboxgl.Marker(markerOptions) | ||
| .setLngLat(coordinate) | ||
| .addTo(map); |
There was a problem hiding this comment.
Prettier: chain setLngLat on one line.
CI flags the chained call formatting here.
🎨 Formatting fix
- markerRef.current = new mapboxgl.Marker(markerOptions)
- .setLngLat(coordinate)
- .addTo(map);
+ markerRef.current = new mapboxgl.Marker(markerOptions).setLngLat(coordinate).addTo(map);📝 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.
| markerRef.current = new mapboxgl.Marker(markerOptions) | |
| .setLngLat(coordinate) | |
| .addTo(map); | |
| markerRef.current = new mapboxgl.Marker(markerOptions).setLngLat(coordinate).addTo(map); |
🧰 Tools
🪛 GitHub Actions: React Native CI/CD
[warning] 278-278: prettier/prettier: Replace ⏎······.setLngLat(coordinate)⏎······ with .setLngLat(coordinate)
🪛 GitHub Check: test
[warning] 278-278:
Replace ⏎······.setLngLat(coordinate)⏎······ with .setLngLat(coordinate)
🤖 Prompt for AI Agents
In `@src/components/maps/map-view.web.tsx` around lines 278 - 280, The chained
Mapbox call should keep setLngLat on the same line for Prettier consistency:
update the expression that assigns markerRef.current (where you call new
mapboxgl.Marker(markerOptions).setLngLat(coordinate).addTo(map)) so that
setLngLat is chained on the same line as new mapboxgl.Marker(...), preserving
the same call order (new
mapboxgl.Marker(markerOptions).setLngLat(coordinate).addTo(map)) and no other
logic changes.
| const success = await this.pushSubscription.unsubscribe(); | ||
|
|
||
| if (success) { | ||
| this.pushSubscription = null; | ||
|
|
There was a problem hiding this comment.
Fix trailing whitespace causing pipeline failure.
Lines 225 and 228 contain trailing whitespace that's causing the prettier check to fail in CI.
🔧 Proposed fix
const success = await this.pushSubscription.unsubscribe();
-
+
if (success) {
this.pushSubscription = null;
-
+
// Clear any potential persisted client-side records📝 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.
| const success = await this.pushSubscription.unsubscribe(); | |
| if (success) { | |
| this.pushSubscription = null; | |
| const success = await this.pushSubscription.unsubscribe(); | |
| if (success) { | |
| this.pushSubscription = null; | |
| // Clear any potential persisted client-side records |
🧰 Tools
🪛 GitHub Actions: React Native CI/CD
[warning] 225-225: prettier/prettier: Delete ······
[warning] 228-228: prettier/prettier: Delete ········
🪛 GitHub Check: test
[warning] 228-228:
Delete ········
[warning] 225-225:
Delete ······
🤖 Prompt for AI Agents
In `@src/services/push-notification.web.ts` around lines 224 - 228, Remove the
trailing whitespace from the lines around the unsubscribe block: the line with
"const success = await this.pushSubscription.unsubscribe();" and the line
setting "this.pushSubscription = null;" (inside the if (success) block) so they
have no trailing spaces; then run Prettier/formatting to ensure CI passes.
Summary by CodeRabbit
New Features
Documentation
Build & Infrastructure
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.