Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR refactors the desktop app by extracting scattered helper logic into modular, reusable components. It modularizes image dimension parsing, browser URL handling, app control command rewriting, VM persistence, and usage ledger scanning, while adding per-run budget cap scoping and conditional auto-update scheduling based on package state. ChangesService modularization and helper extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Owner
Author
|
@copilot review but do not make fixes |
Contributor
Reviewed the latest commit and didn’t find any significant bugs, security issues, or logic regressions to fix. |
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 9
🧹 Nitpick comments (4)
apps/desktop/src/main/services/onboarding/onboardingSuggestedConfig.ts (1)
121-131: 💤 Low valueNaive whitespace split can mangle quoted/
--CI commands.
cmd.split(/\s+/)will fragment arguments likenpm test -- --grep "a b", producing a semantically incorrectcommandarray. Since these are user-reviewed suggestions the impact is limited, but a quote-aware tokenizer (or keeping the matched commands shell-form and letting the runner parse them) would yield more accurate suggestions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/onboarding/onboardingSuggestedConfig.ts` around lines 121 - 131, The code in onboardingSuggestedConfig.ts builds ciCandidates and calls addTest(id, `CI: ${cmd}`, cmd.split(/\s+/), ".", ["custom"]) which incorrectly splits quoted or `--` arguments (e.g., npm test -- --grep "a b"); change the third argument so it is produced by a quote-aware tokenizer or keep the command as a single shell string for the runner to parse: replace cmd.split(/\s+/) with a proper shell-words parse (or pass cmd unchanged) when calling addTest, updating tests and imports as needed to use the chosen tokenizer and preserving the id (`ci-${idx + 1}`), addTest, and ciCandidates flow.apps/desktop/src/main/services/automations/automationService.test.ts (1)
1101-1163: ⚡ Quick winConsider verifying
checkBudgetcall signature for consistency.The test at line 1391 now verifies that
checkBudgetis called with the newrunScopeIdparameter (lines 1454-1456). For consistency and better test coverage, this test should also verify the same signature to document the contract and ensure the parameter is threaded correctly in both code paths.📋 Suggested addition
Add this assertion after line 1158:
await expect(service.triggerManually({ id: "agent-budget" })).rejects.toThrow("Budget exceeded"); + expect(budgetCapService.checkBudget).toHaveBeenCalledWith( + "automation-rule", + "agent-budget", + expect.any(String), + { runScopeId: expect.any(String) } + ); expect(createSession).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/automations/automationService.test.ts` around lines 1101 - 1163, The test omits asserting the checkBudget call signature; after creating the service via createAutomationService and invoking service.triggerManually({ id: "agent-budget" }), add an assertion that budgetCapService.checkBudget was called with the expected run scope argument (e.g. include a check that budgetCapService.checkBudget was calledWith an object containing runScopeId matching the new run id or scope used by triggerManually). Locate the mock on budgetCapService (budgetCapService: { checkBudget: vi.fn(...) }) and add the expectation alongside the existing createSession assertion so the test verifies the new runScopeId parameter is threaded through triggerManually → checkBudget.apps/desktop/src/main/services/appControl/appControlLaunchCommand.ts (2)
54-54: 💤 Low valueConsider documenting the complex regex.
The regex on line 54 is quite complex with multiple named groups. Adding a comment explaining the pattern structure would improve maintainability.
📝 Example documentation
+// Matches: [cd ... &&] [ENV=val ...] <manager> [run] <script> +// Groups: prefix (cd commands), env (environment vars), manager (npm/pnpm/yarn/bun), script (script name) const match = command.trim().match(/^(?<prefix>.*?)(?<env>(?:[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|[^\s;&|]+)\s+)*)(?<manager>npm|pnpm|yarn|bun)\s+(?:run\s+)?(?<script>[A-Za-z0-9:_./-]+)\s*$/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/appControl/appControlLaunchCommand.ts` at line 54, Add a concise explanatory comment above the complex regex used to parse launch commands (the pattern assigned to the "match" variable in appControlLaunchCommand.ts) that describes its purpose, the named capture groups (prefix, env, manager, script), what each group matches (e.g., prefix = leading text, env = zero-or-more env VAR=VALUE pairs with quoted/unquoted support, manager = package manager names npm|pnpm|yarn|bun, script = script name allowing characters A-Za-z0-9:_./-), and a couple of example input strings and expected captures; place the comment immediately above the regex so future readers can quickly understand/maintain it.
79-81: 💤 Low valueConsider logging parse failures for debugging.
The catch block silently returns
nullon any error (file read, JSON parse, etc.). While the fallback behavior is correct, logging parse failures would help diagnose issues when rewriting unexpectedly fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/appControl/appControlLaunchCommand.ts` around lines 79 - 81, The catch block in apps/desktop/src/main/services/appControl/appControlLaunchCommand.ts currently swallows all errors and returns null; update that catch to capture the error (e.g., catch (err)) and log the failure before returning null so parse/read failures are visible for debugging — use the module's existing logger (or console.error if no logger is available) and include context like "failed to parse launch command" plus the error and any relevant input (refer to the try/catch in appControlLaunchCommand where the parse/read happens).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/pnpm-workspace.yaml`:
- Line 7: Replace the placeholder string values with boolean literals: set
allowBuilds.opencode-ai to either true or false (not the text "set this to true
or false") in the desktop workspace, and in the ade-cli workspace set
allowBuilds entries for esbuild, node-pty, opencode-ai, and sqlite3 to explicit
true or false booleans; update the YAML keys named allowBuilds.opencode-ai,
allowBuilds.esbuild, allowBuilds.node-pty and allowBuilds.sqlite3 accordingly so
they are valid booleans before merging.
In `@apps/desktop/src/main/services/builtInBrowser/builtInBrowserNavigation.ts`:
- Around line 5-13: The code treats inputs like "example.com:3000" as having a
scheme because hasScheme uses /^[a-zA-Z][a-zA-Z\d+.-]*:/. Fix this by only
recognizing true URI schemes (those followed by "//") so host:port is treated as
scheme-less: change the hasScheme test to require :// (e.g.
/^[a-zA-Z][a-zA-Z\d+.-]*:\/\//) and keep the existing localhostLike and
candidate logic so that inputs without a true scheme (including host:port) get
prefixed with "https://" before calling new URL().
In `@apps/desktop/src/main/services/macosVm/macosVmStores.test.ts`:
- Around line 59-80: Add assertions to the "round-trips VNC credentials under
stable keys" test to verify malformed entries are filtered by
readVncCredentialStore: after the existing happy-path write/read, overwrite the
store file with a JSON object that contains both a malformed credential (missing
required fields or wrong types) and a valid credential keyed by
vncCredentialKey("lane-2","vm-2"); then call readVncCredentialStore and assert
the malformed key is undefined and the valid key returns the expected password.
This ensures readVncCredentialStore validation logic correctly removes invalid
entries.
- Around line 17-81: Add unit test coverage for generateVncPassword in the
macosVmStores.test.ts suite: create a new it block (e.g., "generates 8-character
base64url passwords") that calls generateVncPassword(), asserts the returned
string has length 8, and asserts it matches the base64url-safe character class
/^[A-Za-z0-9_-]+$/. Place the test alongside the existing cases (near other
VNC-related tests) and import or reference generateVncPassword and any necessary
test helpers so the test runs in CI.
In `@apps/desktop/src/main/services/macosVm/macosVmStores.ts`:
- Around line 89-99: readVncCredentialStore currently unsafely casts
parsed.credentials; instead validate that parsed.credentials is a record and
that each entry has the expected shape (e.g., an object with required fields
like id and other credential properties of the correct types) before casting.
Update readVncCredentialStore to use isRecord(parsed.credentials) and then
iterate Object.entries(parsed.credentials), filter or map only entries where the
value passes a shape-check (e.g., typeof value === "object" && typeof value.id
=== "string"), and build the returned credentials object from those validated
entries; keep the same return signature and fallback to an empty credentials
object on parse/validation failure. Ensure you reference readVncCredentialStore
and reuse the existing isRecord helper or the same validation logic used by
readStoreFile for consistency.
- Around line 113-115: The vncCredentialKey function currently builds keys as
`${laneId}:${vmName}` which can collide if inputs contain colons; update
vncCredentialKey to produce an unambiguous key (for example by returning a
JSON-stringified array of [laneId, vmName] or by URL-safe encoding/escaping both
laneId and vmName and joining with a delimiter) so that inputs containing ":"
cannot create identical keys — locate the vncCredentialKey function and replace
the string interpolation with a collision-free encoding approach.
In `@apps/desktop/src/main/services/shared/imageDimensions.ts`:
- Around line 6-13: pngDimensions currently only checks bytes 1–3 for "PNG" and
can accept malformed buffers; update pngDimensions to validate the full 8‑byte
PNG signature (\x89 P N G \r \n \x1a \n) before reading dimensions, i.e., ensure
buffer.length is at least 24 and then compare buffer[0..7] against the exact
signature (or Buffer.from([0x89,0x50,0x4E,0x47,0x0D,0x0A,0x1A,0x0A])) and only
then call buffer.readUInt32BE(16) and readUInt32BE(20) to return width/height.
In `@apps/desktop/src/main/services/usage/budgetCapService.ts`:
- Around line 225-230: The IPC handler in registerIpc.ts is calling
ctx.budgetCapService.checkBudget(...) without the context object so
budgetCapService’s usd-per-run branch (which reads context.runScopeId) never
sees the run id; update the call site in registerIpc.ts (the call to
ctx.budgetCapService.checkBudget) to pass a context object containing the run
scope id (e.g. { runScopeId: arg.runScopeId } or the appropriate arg field) so
budgetCapService (which reads context.runScopeId) can evaluate usd-per-run caps
correctly, and adjust types/signature usage if needed to match
checkBudget(...)’s expected second/context parameter.
In `@apps/desktop/src/main/services/usage/ledgers/localUsageLedgers.ts`:
- Around line 812-854: discoverCopilotTranscriptFiles collects candidate files
then slices by LOCAL_COST_SCAN_MAX_FILES in traversal order, which can drop
newer files; change it to collect entries as objects with their stat.mtimeMs
(use the existing LOCAL_COST_SCAN_MAX_FILE_BYTES check before pushing), then
after all discovery sort the array by mtimeMs descending and slice to
LOCAL_COST_SCAN_MAX_FILES, finally return the sliced paths; apply the identical
change to the analogous discovery helpers referenced in the review (the other
discovery blocks at the ranges called out) so they also sort by stat.mtimeMs
before trimming.
---
Nitpick comments:
In `@apps/desktop/src/main/services/appControl/appControlLaunchCommand.ts`:
- Line 54: Add a concise explanatory comment above the complex regex used to
parse launch commands (the pattern assigned to the "match" variable in
appControlLaunchCommand.ts) that describes its purpose, the named capture groups
(prefix, env, manager, script), what each group matches (e.g., prefix = leading
text, env = zero-or-more env VAR=VALUE pairs with quoted/unquoted support,
manager = package manager names npm|pnpm|yarn|bun, script = script name allowing
characters A-Za-z0-9:_./-), and a couple of example input strings and expected
captures; place the comment immediately above the regex so future readers can
quickly understand/maintain it.
- Around line 79-81: The catch block in
apps/desktop/src/main/services/appControl/appControlLaunchCommand.ts currently
swallows all errors and returns null; update that catch to capture the error
(e.g., catch (err)) and log the failure before returning null so parse/read
failures are visible for debugging — use the module's existing logger (or
console.error if no logger is available) and include context like "failed to
parse launch command" plus the error and any relevant input (refer to the
try/catch in appControlLaunchCommand where the parse/read happens).
In `@apps/desktop/src/main/services/automations/automationService.test.ts`:
- Around line 1101-1163: The test omits asserting the checkBudget call
signature; after creating the service via createAutomationService and invoking
service.triggerManually({ id: "agent-budget" }), add an assertion that
budgetCapService.checkBudget was called with the expected run scope argument
(e.g. include a check that budgetCapService.checkBudget was calledWith an object
containing runScopeId matching the new run id or scope used by triggerManually).
Locate the mock on budgetCapService (budgetCapService: { checkBudget: vi.fn(...)
}) and add the expectation alongside the existing createSession assertion so the
test verifies the new runScopeId parameter is threaded through triggerManually →
checkBudget.
In `@apps/desktop/src/main/services/onboarding/onboardingSuggestedConfig.ts`:
- Around line 121-131: The code in onboardingSuggestedConfig.ts builds
ciCandidates and calls addTest(id, `CI: ${cmd}`, cmd.split(/\s+/), ".",
["custom"]) which incorrectly splits quoted or `--` arguments (e.g., npm test --
--grep "a b"); change the third argument so it is produced by a quote-aware
tokenizer or keep the command as a single shell string for the runner to parse:
replace cmd.split(/\s+/) with a proper shell-words parse (or pass cmd unchanged)
when calling addTest, updating tests and imports as needed to use the chosen
tokenizer and preserving the id (`ci-${idx + 1}`), addTest, and ciCandidates
flow.
🪄 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: 38e97b12-7d39-4f2b-95b9-69b0befffaf1
⛔ Files ignored due to path filters (6)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/automations/guardrails.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/computer-use/README.mdis excluded by!docs/**docs/features/computer-use/app-control.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**
📒 Files selected for processing (27)
apps/desktop/pnpm-workspace.yamlapps/desktop/src/main/main.tsapps/desktop/src/main/services/appControl/appControlLaunchCommand.test.tsapps/desktop/src/main/services/appControl/appControlLaunchCommand.tsapps/desktop/src/main/services/appControl/appControlService.tsapps/desktop/src/main/services/automations/automationService.test.tsapps/desktop/src/main/services/automations/automationService.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserNavigation.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserNavigation.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserPermissions.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserPermissions.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsapps/desktop/src/main/services/ios/iosSimulatorService.tsapps/desktop/src/main/services/macosVm/macosVmService.tsapps/desktop/src/main/services/macosVm/macosVmStores.test.tsapps/desktop/src/main/services/macosVm/macosVmStores.tsapps/desktop/src/main/services/onboarding/onboardingService.tsapps/desktop/src/main/services/onboarding/onboardingSuggestedConfig.tsapps/desktop/src/main/services/shared/imageDimensions.test.tsapps/desktop/src/main/services/shared/imageDimensions.tsapps/desktop/src/main/services/updates/autoUpdateService.test.tsapps/desktop/src/main/services/updates/autoUpdateService.tsapps/desktop/src/main/services/usage/budgetCapService.test.tsapps/desktop/src/main/services/usage/budgetCapService.tsapps/desktop/src/main/services/usage/ledgers/localUsageLedgers.tsapps/desktop/src/main/services/usage/usagePricing.tsapps/desktop/src/main/services/usage/usageTrackingService.ts
arul28
force-pushed
the
ade-74-platform-vm-deeplinks-budgetcap-per-run-miscount-dev-auto-update-noise-keychain-argv-cross-ref-and-platform-god-file-decomposition
branch
4 times, most recently
from
fd787d4 to
1bfc83a
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/arul28s-projects?upgradeToPro=build-rate-limit |
arul28
force-pushed
the
ade-74-platform-vm-deeplinks-budgetcap-per-run-miscount-dev-auto-update-noise-keychain-argv-cross-ref-and-platform-god-file-decomposition
branch
from
1bfc83a to
2235d25
Compare
arul28
deleted the
ade-74-platform-vm-deeplinks-budgetcap-per-run-miscount-dev-auto-update-noise-keychain-argv-cross-ref-and-platform-god-file-decomposition
branch
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
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.
Refs ADE-74
Summary
Validation
Windows release/build jobs intentionally skipped per request.
Linked Linear issues
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
This PR fixes the
usd-per-runbudget cap to scope cost lookups against the active run ID rather than historical rule-level usage, disables scheduled auto-update polling in unpackaged/dev Electron sessions, and splits a large platform "god-file" into focused modules (browser navigation/permissions, macOS VM stores, onboarding suggested config, image dimensions, and local usage ledgers).evaluateCapnow usescontext.runScopeId(the active run's DB ID) as the lookup key forusd-per-run, so accumulated cost is isolated per run; a missingrunScopeIdlogs a warning and returnsallowed: truerather than silently applying zero-cost logic.autoCheckEnabled: app.isPackagedpreventssetTimeout/setIntervalupdate timers from firing in dev/unpackaged sessions, eliminating noisy update-check noise during development.usageTrackingService.tsintoledgers/localUsageLedgers.ts; browser URL normalisation/permission helpers and macOS VM store helpers each moved into their own testable modules with new test coverage.Confidence Score: 5/5
Safe to merge — the budget-cap fix is well-scoped, the refactoring is mechanical, and new test coverage validates the corrected per-run semantics.
The usd-per-run cost-lookup fix is targeted and the changed code path is covered by updated and new unit tests. The god-file splits are copy/paste extractions with no logic changes. The auto-update guard is a single boolean flag driven by app.isPackaged, a reliable Electron API. No correctness issues were found in the changed paths.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant AS as automationService participant DB as RunsDB participant BCS as budgetCapService participant EV as EventEmitter AS->>DB: insertRun(rule, trigger) → run.id AS->>BCS: "checkBudget(scope, rule.id, provider, {runScopeId: run.id})" BCS->>DB: getCumulativeUsage(scope, run.id, provider, weekKey) DB-->>BCS: accumulated cost for this run alt budget exceeded BCS-->>AS: "{allowed: false, reason}" AS->>DB: "updateRun(run.id, {status: failed})" AS->>EV: emit runs-updated AS-->>AS: throw Error(reason) else budget ok BCS-->>AS: "{allowed: true}" AS->>AS: continue run execution endReviews (6): Last reviewed commit: "Address ADE-74 review feedback" | Re-trigger Greptile