Phase 0: ElectroBun scaffold with tray app and Swift FFI bridge#1
Merged
Phase 0: ElectroBun scaffold with tray app and Swift FFI bridge#1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces the foundational build and application infrastructure for AgentTap: a macOS application combining a system tray interface (via electrobun/Bun), a Swift native bridge exporting system information and capability checks, a build pipeline compiling architecture-specific dylibs, and a web-based main view. Includes configuration for TypeScript, npm/Bun package management, and GitHub Actions CI. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
electrobun.config.ts (1)
36-41: Gate packaging security flags by environment.Unconditionally disabling ASAR/signing/notarization is risky for release builds. Keep this dev-only via env-based config.
Proposed fix
import type { ElectrobunConfig } from "electrobun"; +const isRelease = process.env.BUILD_CHANNEL === "release"; + export default { @@ - useAsar: false, + useAsar: isRelease, @@ mac: { - codesign: false, - notarize: false, + codesign: isRelease, + notarize: isRelease, bundleCEF: false, defaultRenderer: "native", icons: "icon.iconset", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electrobun.config.ts` around lines 36 - 41, The config currently sets useAsar: false and mac.codesign/notarize/bundleCEF all false unconditionally; change these to be environment-gated so they remain disabled only in development. Update electrobun config where useAsar and the mac object (codesign, notarize, bundleCEF) are defined to read an env check (e.g. NODE_ENV === 'production' or a specific ELECTROBUN_DISABLE_SIGNING flag) and set useAsar = true and mac.codesign/mac.notarize/mac.bundleCEF = true for production, otherwise false for dev; ensure the logic is centralized so release builds get ASAR and signing/notarization enabled while local/dev builds keep them off.package.json (1)
13-16: Make app build depend on native bridge build.
buildcurrently skipsbuild:native. This can produce app artifacts without the expected dylib state.Proposed fix
- "build": "bunx electrobun build", + "build": "bun run build:native && bunx electrobun build",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 13 - 16, The build script currently runs electrobun build without building the native bridge first; update the package.json "build" script so it invokes the "build:native" script before running "build" (i.e., run "build:native" then the existing electrobun build command) so artifacts include the native dylib; ensure you reference the existing script names "build" and "build:native" when making the change and preserve the "build:stable" and "clean" scripts behavior.src/views/mainview/index.ts (1)
8-16: Use safe DOM APIs instead ofinnerHTMLfor better maintainability.While the provider data is currently hardcoded, using
innerHTMLwith string interpolation is a code anti-pattern. Refactor to usetextContentandappend()for clarity and future-proofing.Proposed fix
for (const provider of DEFAULT_PROVIDERS) { const li = document.createElement("li"); - li.innerHTML = ` - <div> - <strong>${provider.name}</strong> - <span class="domain">${provider.domains[0]}</span> - </div> - <span class="badge ${provider.enabled ? "on" : "off"}"> - ${provider.enabled ? "ON" : "OFF"} - </span> - `; + const left = document.createElement("div"); + const name = document.createElement("strong"); + name.textContent = provider.name; + const domain = document.createElement("span"); + domain.className = "domain"; + domain.textContent = provider.domains[0] ?? "—"; + left.append(name, domain); + + const badge = document.createElement("span"); + badge.className = `badge ${provider.enabled ? "on" : "off"}`; + badge.textContent = provider.enabled ? "ON" : "OFF"; + + li.append(left, badge); providerList.appendChild(li); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/mainview/index.ts` around lines 8 - 16, The current code sets li.innerHTML using string interpolation which risks injection and is hard to maintain; replace the innerHTML block by creating and appending DOM nodes explicitly: create a container div, create a strong element and set its textContent to provider.name, create a span.domain and set its textContent to provider.domains[0], append those to the div, create a badge span, set its className to `badge ${provider.enabled ? "on" : "off"}` and its textContent to `ON` or `OFF` based on provider.enabled, then clear li (e.g., li.textContent = "" or remove children) and append the new elements to li (references: the local variable li, and the provider properties provider.name, provider.domains, provider.enabled).src/bun/native-bridge.ts (4)
1-1: Unused import:ptr.
ptris imported but never used in this file.Remove unused import
-import { dlopen, FFIType, ptr, suffix } from "bun:ffi"; +import { dlopen, FFIType, suffix } from "bun:ffi";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/native-bridge.ts` at line 1, The import list in src/bun/native-bridge.ts includes an unused symbol `ptr`; remove `ptr` from the import statement (the line importing dlopen, FFIType, ptr, suffix) so it only imports the actually used symbols (dlopen, FFIType, suffix) to eliminate the unused import warning.
56-61: Dead code:readCStringis never called.This method is defined but unused. Both
getVersionandgetSystemInfouseBun.mmapdirectly instead.Remove or use it
- private readCString(pointer: ReturnType<typeof ptr>): string { - if (!pointer) return ""; - const buf = Buffer.from(pointer as unknown as ArrayBuffer); - const str = new TextDecoder().decode(buf); - return str; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/native-bridge.ts` around lines 56 - 61, The private helper readCString is dead code and should be removed or actually used; to fix, delete the readCString(pointer) method from the class in native-bridge.ts (removing its Buffer/TextDecoder logic) and ensure no other code references it, or alternatively refactor getVersion and getSystemInfo to call readCString with the pointer returned by Bun.mmap instead of performing their own decoding—update references to use the function name readCString (or remove it entirely) and run tests/TS build to confirm no unused symbol errors remain.
38-43: Redundant candidate path on macOS.On macOS,
suffixis"dylib", so lines 39 and 40 produce identical paths. The redundancy is harmless but unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/native-bridge.ts` around lines 38 - 43, The candidates array contains a redundant macOS path because suffix is "dylib", so the second entry duplicates the first; update the construction of candidates (the candidates constant that uses join and import.meta.dir with suffix) to avoid adding duplicate "libAgentTapBridge.dylib" — either remove the explicit duplicate entry or build the array conditionally (e.g., only push the literal "libAgentTapBridge.dylib" when suffix !== "dylib" or dedupe the array after creation) so the list contains unique paths.
68-76: Unused variable and hardcoded buffer length.Line 68 creates
viewbut never uses it. The 256-byte mmap length is arbitrary; if native returns longer strings, they'll be truncated to 64 chars (fallback on line 74).Remove unused variable
// Read null-terminated C string from pointer - const view = new DataView(new ArrayBuffer(0)); const cStr = new Uint8Array(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/native-bridge.ts` around lines 68 - 76, Remove the unused local "view" and stop using the hardcoded mmap length; instead query the native for the returned string length (e.g., call a length helper like this.lib.symbols.agenttap_string_length(result) or an equivalent native API) and pass that length to Bun.mmap when creating cStr, then compute nullIdx and slice with cStr.subarray(0, nullIdx >= 0 ? nullIdx : cStr.length) (do not fallback to 64), and keep calling this.lib.symbols.agenttap_free_string(result) afterwards; update the null check to use >= 0 so an empty string is handled correctly.src/shared/types.ts (1)
10-19: Consider marking DEFAULT_PROVIDERS as readonly.The array is spread-copied in
src/bun/index.ts, but marking itreadonlyprevents accidental direct mutation elsewhere.Proposed change
-export const DEFAULT_PROVIDERS: Provider[] = [ +export const DEFAULT_PROVIDERS: readonly Provider[] = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types.ts` around lines 10 - 19, Mark DEFAULT_PROVIDERS as readonly to prevent accidental mutation: change its type to a readonly tuple/array (e.g., ReadonlyArray<Provider> or readonly Provider[]) and update the declaration of DEFAULT_PROVIDERS accordingly so callers (like the spread in index.ts) still can copy it but cannot mutate the original; ensure the symbol DEFAULT_PROVIDERS and its export remain unchanged except for the readonly type annotation.src/bun/index.ts (2)
126-129: Abrupt exit without cleanup.
process.exit(0)terminates immediately. If future phases add resources (open files, network connections, the NativeBridge), they won't be cleaned up gracefully.Fine for Phase 0 scaffold; flag for later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/index.ts` around lines 126 - 129, When handling TrayActions.QUIT, avoid calling process.exit(0) directly; instead implement and invoke a graceful shutdown sequence (e.g., shutdownAgent / cleanupResources) that closes/flushes the NativeBridge, network connections, and open files, removes listeners, and then exits; replace the direct process.exit(0) in the if (action === TrayActions.QUIT) block with a call to that shutdown function and only call process.exit(0) after cleanup completes.
96-106: Toggle doesn't handle "error" state.If
captureStatusis"error", toggling sets it to"active". This may be intentional (retry behavior), but consider whether error state should require explicit acknowledgment or different handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/index.ts` around lines 96 - 106, The toggle handler currently treats an "error" captureStatus the same as "inactive" and immediately starts capture; change TrayActions.TOGGLE_CAPTURE logic to explicitly handle state.captureStatus === "error" by clearing or resetting the error instead of starting capture (e.g., set state.captureStatus = "inactive", console.log a message about clearing the error, update tray.setTitle and call buildTrayMenu, and return) so error requires an explicit retry path rather than automatically activating; keep existing behavior for "inactive" and "active" branches and reference TrayActions.TOGGLE_CAPTURE, state.captureStatus, tray.setTitle, and buildTrayMenu when applying the change.src/views/mainview/index.html (1)
17-40: Several status elements are not wired to JS updates.
#status-indicator,#capture-status,#traces-count, and#bridge-statusare defined butindex.tsonly updates#provider-listand#providers-count. These will show stale/initial values indefinitely.If intentional placeholders for future phases, consider adding
data-todoattributes or comments to clarify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/mainview/index.html` around lines 17 - 40, The status UI elements (`#status-indicator`, `#capture-status`, `#traces-count`, `#bridge-status`) are declared in the template but never updated by the script that currently only updates `#provider-list` and `#providers-count` in index.ts; update index.ts to select these DOM nodes and set their values/state when the underlying data changes (e.g., update capture state text and indicator class in the capture flow, set traces-count from the traces metric, and set bridge-status from the native bridge health check), or if they are intentionally left as placeholders add data-todo attributes or an inline comment next to each element to indicate they are pending implementation (refer to the DOM IDs above and the update logic in index.ts where `#provider-list/`#providers-count are handled).native/AgentTapBridge.swift (1)
5-9: strdup can return NULL on allocation failure.If memory allocation fails,
strdupreturnsNULL. The FFI consumer checks for null results, but for completeness, consider a fallback.Defensive fallback
`@_cdecl`("agenttap_version") public func agenttap_version() -> UnsafeMutablePointer<CChar>? { let version = "0.1.0" - return strdup(version) + return strdup(version) // Returns nil on OOM; handled by caller }Or return optional type explicitly. The TypeScript side already handles null.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/AgentTapBridge.swift` around lines 5 - 9, In agenttap_version, strdup may return NULL on allocation failure; update the function to check the strdup return value and handle NULL defensively: if strdup(version) returns nil, attempt a safe fallback (e.g., try strdup on an empty string or allocate a minimal C string) and if that also fails, return a clear NULL pointer so the FFI consumer can detect the failure; ensure the check and fallback are implemented around the call to strdup in agenttap_version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 54-59: The upload step named "Upload app artifact" currently uses
actions/upload-artifact@v4 with if-no-files-found: warn which allows the
workflow to pass when .build/ is missing; change the if-no-files-found value to
error in that step so the CI fails if the artifact directory is absent, ensuring
missing build outputs cause a failed job.
- Around line 37-40: The CI workflow currently pins Bun via the setup step named
"Install Bun" using oven-sh/setup-bun@v2 with bun-version: latest; change that
bun-version value from "latest" to the specific stable release "1.3.12" to
ensure reproducible builds, i.e., update the bun-version field in the Install
Bun step accordingly.
In @.gitignore:
- Around line 24-25: Remove bun.lockb from .gitignore and add/commit the actual
lockfile to the repo so CI has a deterministic dependency graph; specifically
delete the bun.lockb entry from the .gitignore file and run git add bun.lockb &&
git commit -m "chore: commit bun.lockb lockfile" (ensure the file is tracked so
bun-version: latest uses the committed lockfile in CI).
In `@native/AgentTapBridge.swift`:
- Around line 13-29: The current agenttap_system_info function includes
info.hostName which can leak PII; update agenttap_system_info to stop exposing
raw hostnames by either removing the "hostname" key entirely or replacing it
with a non-identifying value (e.g., "REDACTED" or a deterministic hash).
Concretely, edit agenttap_system_info to (a) drop the "hostname" entry from the
json dictionary OR (b) import CryptoKit and replace info.hostName with a
hashed/trimmed representation (e.g., SHA256 hex and maybe first 8 chars) so the
value is non-identifying, then proceed with JSONSerialization as before; ensure
the code references agenttap_system_info and info.hostName when making the
change.
In `@src/bun/index.ts`:
- Around line 7-12: The AppState initialization uses providers:
[...DEFAULT_PROVIDERS] which only shallow-copies the array and leaves the
provider objects shared with DEFAULT_PROVIDERS; change state initialization to
deep-clone the provider objects (for example map over DEFAULT_PROVIDERS and
create new objects for each provider) so toggling provider.enabled (the code
that mutates provider.enabled) does not mutate the originals in
DEFAULT_PROVIDERS; update the initializer that sets state to create fresh
provider instances (and consider doing the same pattern wherever you
reinitialize defaults) so the app works with independent provider objects.
In `@src/views/mainview/index.css`:
- Around line 180-186: The footer link styles only handle hover; add keyboard
focus styling by defining a :focus-visible rule for the same selector (e.g.,
footer a:focus-visible) so keyboard users see focus; update the footer a CSS
block to include an accessible visible focus indicator (matching or
complementing footer a:hover color and an outline or box-shadow) to ensure links
are perceivable via keyboard navigation.
---
Nitpick comments:
In `@electrobun.config.ts`:
- Around line 36-41: The config currently sets useAsar: false and
mac.codesign/notarize/bundleCEF all false unconditionally; change these to be
environment-gated so they remain disabled only in development. Update electrobun
config where useAsar and the mac object (codesign, notarize, bundleCEF) are
defined to read an env check (e.g. NODE_ENV === 'production' or a specific
ELECTROBUN_DISABLE_SIGNING flag) and set useAsar = true and
mac.codesign/mac.notarize/mac.bundleCEF = true for production, otherwise false
for dev; ensure the logic is centralized so release builds get ASAR and
signing/notarization enabled while local/dev builds keep them off.
In `@native/AgentTapBridge.swift`:
- Around line 5-9: In agenttap_version, strdup may return NULL on allocation
failure; update the function to check the strdup return value and handle NULL
defensively: if strdup(version) returns nil, attempt a safe fallback (e.g., try
strdup on an empty string or allocate a minimal C string) and if that also
fails, return a clear NULL pointer so the FFI consumer can detect the failure;
ensure the check and fallback are implemented around the call to strdup in
agenttap_version.
In `@package.json`:
- Around line 13-16: The build script currently runs electrobun build without
building the native bridge first; update the package.json "build" script so it
invokes the "build:native" script before running "build" (i.e., run
"build:native" then the existing electrobun build command) so artifacts include
the native dylib; ensure you reference the existing script names "build" and
"build:native" when making the change and preserve the "build:stable" and
"clean" scripts behavior.
In `@src/bun/index.ts`:
- Around line 126-129: When handling TrayActions.QUIT, avoid calling
process.exit(0) directly; instead implement and invoke a graceful shutdown
sequence (e.g., shutdownAgent / cleanupResources) that closes/flushes the
NativeBridge, network connections, and open files, removes listeners, and then
exits; replace the direct process.exit(0) in the if (action ===
TrayActions.QUIT) block with a call to that shutdown function and only call
process.exit(0) after cleanup completes.
- Around line 96-106: The toggle handler currently treats an "error"
captureStatus the same as "inactive" and immediately starts capture; change
TrayActions.TOGGLE_CAPTURE logic to explicitly handle state.captureStatus ===
"error" by clearing or resetting the error instead of starting capture (e.g.,
set state.captureStatus = "inactive", console.log a message about clearing the
error, update tray.setTitle and call buildTrayMenu, and return) so error
requires an explicit retry path rather than automatically activating; keep
existing behavior for "inactive" and "active" branches and reference
TrayActions.TOGGLE_CAPTURE, state.captureStatus, tray.setTitle, and
buildTrayMenu when applying the change.
In `@src/bun/native-bridge.ts`:
- Line 1: The import list in src/bun/native-bridge.ts includes an unused symbol
`ptr`; remove `ptr` from the import statement (the line importing dlopen,
FFIType, ptr, suffix) so it only imports the actually used symbols (dlopen,
FFIType, suffix) to eliminate the unused import warning.
- Around line 56-61: The private helper readCString is dead code and should be
removed or actually used; to fix, delete the readCString(pointer) method from
the class in native-bridge.ts (removing its Buffer/TextDecoder logic) and ensure
no other code references it, or alternatively refactor getVersion and
getSystemInfo to call readCString with the pointer returned by Bun.mmap instead
of performing their own decoding—update references to use the function name
readCString (or remove it entirely) and run tests/TS build to confirm no unused
symbol errors remain.
- Around line 38-43: The candidates array contains a redundant macOS path
because suffix is "dylib", so the second entry duplicates the first; update the
construction of candidates (the candidates constant that uses join and
import.meta.dir with suffix) to avoid adding duplicate "libAgentTapBridge.dylib"
— either remove the explicit duplicate entry or build the array conditionally
(e.g., only push the literal "libAgentTapBridge.dylib" when suffix !== "dylib"
or dedupe the array after creation) so the list contains unique paths.
- Around line 68-76: Remove the unused local "view" and stop using the hardcoded
mmap length; instead query the native for the returned string length (e.g., call
a length helper like this.lib.symbols.agenttap_string_length(result) or an
equivalent native API) and pass that length to Bun.mmap when creating cStr, then
compute nullIdx and slice with cStr.subarray(0, nullIdx >= 0 ? nullIdx :
cStr.length) (do not fallback to 64), and keep calling
this.lib.symbols.agenttap_free_string(result) afterwards; update the null check
to use >= 0 so an empty string is handled correctly.
In `@src/shared/types.ts`:
- Around line 10-19: Mark DEFAULT_PROVIDERS as readonly to prevent accidental
mutation: change its type to a readonly tuple/array (e.g.,
ReadonlyArray<Provider> or readonly Provider[]) and update the declaration of
DEFAULT_PROVIDERS accordingly so callers (like the spread in index.ts) still can
copy it but cannot mutate the original; ensure the symbol DEFAULT_PROVIDERS and
its export remain unchanged except for the readonly type annotation.
In `@src/views/mainview/index.html`:
- Around line 17-40: The status UI elements (`#status-indicator`, `#capture-status`,
`#traces-count`, `#bridge-status`) are declared in the template but never updated by
the script that currently only updates `#provider-list` and `#providers-count` in
index.ts; update index.ts to select these DOM nodes and set their values/state
when the underlying data changes (e.g., update capture state text and indicator
class in the capture flow, set traces-count from the traces metric, and set
bridge-status from the native bridge health check), or if they are intentionally
left as placeholders add data-todo attributes or an inline comment next to each
element to indicate they are pending implementation (refer to the DOM IDs above
and the update logic in index.ts where `#provider-list/`#providers-count are
handled).
In `@src/views/mainview/index.ts`:
- Around line 8-16: The current code sets li.innerHTML using string
interpolation which risks injection and is hard to maintain; replace the
innerHTML block by creating and appending DOM nodes explicitly: create a
container div, create a strong element and set its textContent to provider.name,
create a span.domain and set its textContent to provider.domains[0], append
those to the div, create a badge span, set its className to `badge
${provider.enabled ? "on" : "off"}` and its textContent to `ON` or `OFF` based
on provider.enabled, then clear li (e.g., li.textContent = "" or remove
children) and append the new elements to li (references: the local variable li,
and the provider properties provider.name, provider.domains, provider.enabled).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72736a8d-79c5-4b77-a899-ceb295c66349
⛔ Files ignored due to path filters (10)
icon.iconset/icon_128x128.pngis excluded by!**/*.pngicon.iconset/icon_128x128@2x.pngis excluded by!**/*.pngicon.iconset/icon_16x16.pngis excluded by!**/*.pngicon.iconset/icon_16x16@2x.pngis excluded by!**/*.pngicon.iconset/icon_256x256.pngis excluded by!**/*.pngicon.iconset/icon_256x256@2x.pngis excluded by!**/*.pngicon.iconset/icon_32x32.pngis excluded by!**/*.pngicon.iconset/icon_32x32@2x.pngis excluded by!**/*.pngicon.iconset/icon_512x512.pngis excluded by!**/*.pngsrc/views/mainview/assets/tray-icon-template.pngis excluded by!**/*.png
📒 Files selected for processing (13)
.github/workflows/build.yml.gitignoreelectrobun.config.tsnative/AgentTapBridge.swiftnative/Makefilepackage.jsonsrc/bun/index.tssrc/bun/native-bridge.tssrc/shared/types.tssrc/views/mainview/index.csssrc/views/mainview/index.htmlsrc/views/mainview/index.tstsconfig.json
.gitignore
Outdated
Comment on lines
+24
to
+25
| # Bun | ||
| bun.lockb |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify lockfile is ignored and CI is using floating dependency resolution.
rg -n 'bun\.lockb|bun\.lock' .gitignore
rg -n 'bun install|bun-version:\s*latest' .github/workflows/build.yml package.jsonRepository: GeiserX/AgentTap
Length of output: 189
Remove bun.lockb from .gitignore and commit the lockfile.
The lockfile is currently ignored while CI uses bun-version: latest, creating non-deterministic builds. Commit the lockfile to ensure consistent dependency resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 24 - 25, Remove bun.lockb from .gitignore and
add/commit the actual lockfile to the repo so CI has a deterministic dependency
graph; specifically delete the bun.lockb entry from the .gitignore file and run
git add bun.lockb && git commit -m "chore: commit bun.lockb lockfile" (ensure
the file is tracked so bun-version: latest uses the committed lockfile in CI).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/bun/index.ts (1)
87-94: Type assertion lacks runtime validation.Line 88 casts
e.datawithout checking the shape. If ElectroBun's event structure changes, this fails silently.Consider a type guard or at least defensive access:
Safer event data access
tray.on("tray-clicked", (e) => { - const { action } = e.data as { id: number; action: string }; + const data = e.data as Record<string, unknown>; + const action = typeof data?.action === "string" ? data.action : ""; if (action === "" || action === undefined) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/index.ts` around lines 87 - 94, The handler registered with tray.on("tray-clicked") is unsafely asserting e.data's shape; add runtime validation instead of direct type assertion: in the tray.on callback (and before using buildTrayMenu), check that e and e.data exist and that e.data has an 'action' property of type string (e.g., typeof e.data === 'object' && 'action' in e.data && typeof (e.data as any).action === 'string'); if validation fails, handle gracefully (early return or a safe default) so tray-clicked cannot throw due to unexpected event shapes. Ensure references to e.data and action in this handler are guarded accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@native/AgentTapBridge.swift`:
- Around line 32-41: The capability checks are currently over-reporting support;
update the functions agenttap_can_manage_firewall and
agenttap_can_manage_keychain to conservatively return false until proper runtime
checks are implemented: change agenttap_can_manage_firewall to stop relying only
on FileManager.fileExists(atPath:) and return false (or perform a real
privileged-helper verification later), and change agenttap_can_manage_keychain
to return false instead of always true (add TODO comments in both functions to
implement real checks such as verifying the privileged helper/service for pfctl
and performing an actual Security.framework / SecItem access test).
---
Nitpick comments:
In `@src/bun/index.ts`:
- Around line 87-94: The handler registered with tray.on("tray-clicked") is
unsafely asserting e.data's shape; add runtime validation instead of direct type
assertion: in the tray.on callback (and before using buildTrayMenu), check that
e and e.data exist and that e.data has an 'action' property of type string
(e.g., typeof e.data === 'object' && 'action' in e.data && typeof (e.data as
any).action === 'string'); if validation fails, handle gracefully (early return
or a safe default) so tray-clicked cannot throw due to unexpected event shapes.
Ensure references to e.data and action in this handler are guarded accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 317f8df6-be7f-489c-a5c3-876671b3300a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/build.yml.gitignorenative/AgentTapBridge.swiftpackage.jsonsrc/bun/index.tssrc/bun/native-bridge.tssrc/shared/types.tssrc/views/mainview/index.css
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- src/views/mainview/index.css
- .github/workflows/build.yml
- package.json
- src/shared/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bun/native-bridge.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/bun/native-bridge.ts (2)
84-99: Free native strings infinallyblocks.If string decoding throws,
agenttap_free_stringis skipped and leaks native memory. Wrap decode withtry/finallyin both methods.Suggested safety diff
getVersion(): string { if (!this.lib) return "native bridge unavailable"; const result = this.lib.symbols.agenttap_version(); if (!result) return "unknown"; - const str = this.readCString(result as unknown as number); - this.lib.symbols.agenttap_free_string(result); - return str; + try { + return this.readCString(result as unknown as number); + } finally { + this.lib.symbols.agenttap_free_string(result); + } } getSystemInfo(): string { if (!this.lib) return "{}"; const result = this.lib.symbols.agenttap_system_info(); if (!result) return "{}"; - const str = this.readCString(result as unknown as number); - this.lib.symbols.agenttap_free_string(result); - return str; + try { + return this.readCString(result as unknown as number); + } finally { + this.lib.symbols.agenttap_free_string(result); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/native-bridge.ts` around lines 84 - 99, Both getVersion and getSystemInfo can leak native memory if readCString throws because agenttap_free_string is only called after decoding; modify both methods (getVersion and getSystemInfo) to store the native pointer (result) and perform the readCString call inside a try block with agenttap_free_string invoked in a finally block via this.lib.symbols.agenttap_free_string(result) so the native string is always freed even on decode errors; keep the same early-return checks for this.lib and null result.
67-75: Capture finaldlopenfailure for diagnostics.The empty
catchmakes packaging/runtime failures opaque. Keep trying candidates, but retain the last error and emit one warning after exhaustion.Suggested observability diff
+ let lastError: unknown = null; for (const path of candidates) { try { this.lib = dlopen(path, FFI_SYMBOLS); this.available = true; return; - } catch { - // Try next candidate + } catch (error) { + lastError = error; } } + console.warn("[native-bridge] failed to load dylib", { candidates, lastError }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/native-bridge.ts` around lines 67 - 75, The loop that calls dlopen over candidates swallows all errors, making failures opaque; modify the code in native-bridge.ts (the block that iterates candidates and calls dlopen with FFI_SYMBOLS, setting this.lib and this.available) to capture the last thrown error (e.g., lastError variable) instead of an empty catch, continue trying other candidates, and after the loop completes with no success emit a single diagnostic warning/error that includes the lastError message and any candidate info so consumers can diagnose why dlopen failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun/native-bridge.ts`:
- Around line 36-65: The current load() routine uses findProjectRoot() and the
first candidates entry to load a dylib from process.cwd(), which creates a
library-hijack vector; restrict this behavior to explicit dev-only mode by
gating the project-root-derived candidate behind a dev flag (e.g., an isDev or
ENABLE_NATIVE_DEV lookup) and reorder candidates to try bundled paths first
(those using import.meta.dir and Resources) before any cwd-derived path; update
the load() function to only include the join(projectRoot, "native", "build",
`libAgentTapBridge.${suffix}`) candidate when that dev flag is true and keep
findProjectRoot() unchanged but only called when dev mode is active.
---
Nitpick comments:
In `@src/bun/native-bridge.ts`:
- Around line 84-99: Both getVersion and getSystemInfo can leak native memory if
readCString throws because agenttap_free_string is only called after decoding;
modify both methods (getVersion and getSystemInfo) to store the native pointer
(result) and perform the readCString call inside a try block with
agenttap_free_string invoked in a finally block via
this.lib.symbols.agenttap_free_string(result) so the native string is always
freed even on decode errors; keep the same early-return checks for this.lib and
null result.
- Around line 67-75: The loop that calls dlopen over candidates swallows all
errors, making failures opaque; modify the code in native-bridge.ts (the block
that iterates candidates and calls dlopen with FFI_SYMBOLS, setting this.lib and
this.available) to capture the last thrown error (e.g., lastError variable)
instead of an empty catch, continue trying other candidates, and after the loop
completes with no success emit a single diagnostic warning/error that includes
the lastError message and any candidate info so consumers can diagnose why
dlopen failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fffd489-3a77-44ab-b974-8365d19b68d1
📒 Files selected for processing (3)
README.mdROADMAP.mdsrc/bun/native-bridge.ts
✅ Files skipped from review due to trivial changes (2)
- README.md
- ROADMAP.md
GeiserX
force-pushed
the
phase-0/electrobun-scaffold
branch
from
2e8c330 to
d466fef
Compare
- ElectroBun project structure (electrobun.config.ts, package.json, tsconfig) - Menu bar tray app with provider toggles and capture control - Swift FFI bridge skeleton (universal dylib, C-compatible exports) - Main window view with dark theme status dashboard - GitHub Actions CI workflow (build native + app) - Shared types for providers, app state, and tray actions
- Pin bun-version to 1.3.12 in CI (reproducible builds) - Fail CI when app artifact is missing (if-no-files-found: error) - Commit bun.lock for deterministic dependency resolution - Remove hostname from system_info (PII concern) - Deep-clone providers in state init (prevent DEFAULT_PROVIDERS mutation) - Mark DEFAULT_PROVIDERS as readonly - Clean up native-bridge.ts (remove unused imports, dead code, dedup paths) - Add focus-visible styling for footer links (accessibility) - Chain build:native into build script
…very - Replace Bun.mmap (takes file path) with CString (reads C pointer) - Walk up from CWD to find project root for dylib path resolution - Add Resources/ as additional candidate for bundled dylib
- Gate CWD-derived dylib paths to dev mode only (prevent library hijack) - Capability checks return false until real implementations (Phase 1/2) - Safe runtime type check on tray event data instead of blind assertion
GeiserX
force-pushed
the
phase-0/electrobun-scaffold
branch
from
d466fef to
b9c3d4e
Compare
This was referenced Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
electrobun.config.ts,package.json,tsconfig.jsonwith menu bar app configuration (exitOnLastWindowClosed: false)Trayclass fromelectrobun/bun@_cdecl) for version, system info, firewall/keychain capability checks. Bun FFI bindings in TypeScript viabun:ffiArchitecture
Phase 0 Checklist (from ROADMAP.md)
.dylibwith C-compatible exportsTest plan
cd native && make universalcompiles without errorsnm -gU native/build/libAgentTapBridge.dylib | grep agenttapshows all 5 symbolslipo -info native/build/libAgentTapBridge.dylibshows both arm64 and x86_64bun install && bun run devlaunches the tray app (requires ElectroBun)Summary by CodeRabbit
New Features
Chores