Prepare public release cleanup#17
Merged
Merged
Conversation
Remove unused HF token from PR regression, scope workflow permissions, fix Socket github.token usage, skip Socket on fork PRs, and guard build.yml from pull_request triggers that would expose signing secrets. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
(Note: The hidden review artifact above must contain every rangeId exactly once; if you see duplicates or unassigned ranges, please re-run the review-stack generator to produce a deterministic mapping.) WalkthroughCancellable calendar OAuth flows (Google and Microsoft) and a cancel API were added; CalendarManager and IPC were updated; renderer tracks per-provider attempts and cancels on focus; the Cloudflare auth worker was refactored with route validation and rate limiting; build/workflow/env handling and analytics consent behavior were adjusted; tests and tooling were added. ChangesCalendar Connection Cancellation and OAuth Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
Socket Security: Dependency OverviewReview the following changes in direct dependencies. Learn more about socket.dev
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/pages/Settings.tsx (1)
120-144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the success path, not just
finally.If focus clears
connectAttemptRef.currentand cancellation races with a late success, this code still runsaddAccount()andsetEvents()for the abandoned attempt. That lets stale async work update state after a newer connect flow has started. CheckconnectAttemptRef.current === attemptIdbefore each post-awaitstate mutation, not only infinally.Suggested fix
try { const account = await window.electronAPI.invoke('calendar:connect', provider) + if (connectAttemptRef.current !== attemptId) return addAccount(account) const events = await window.electronAPI.invoke('calendar:get-events') + if (connectAttemptRef.current !== attemptId) return setEvents(events) } catch (err) {🤖 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 `@src/renderer/src/pages/Settings.tsx` around lines 120 - 144, The success path in handleConnect is not guarded against stale/abandoned attempts: after each await you must verify the attempt is still current before mutating state or calling addAccount/setEvents; update handleConnect to check connectAttemptRef.current === attemptId immediately after the await that returns account and again after the await that returns events (and skip addAccount/setEvents and further state updates if it no longer matches), while leaving the existing finally cleanup as-is; reference symbols: handleConnect, connectAttemptRef, attemptId, addAccount, setEvents, setConnectingProvider, setConnecting.
🤖 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 @.github/workflows/build.yml:
- Line 39: The workflow currently grants elevated permission via the YAML key
actions: write; update the .github/workflows/build.yml entry that contains
actions: write to include a short inline rationale comment explaining why write
access is required (e.g., to upload artifacts, trigger downstream workflows, or
update workflow runs) so the permission-policy linter and auditors have context;
apply the same inline justification wherever the actions: write key appears
across the repo.
In `@src/main/services/__tests__/official-build-config.test.ts`:
- Around line 10-12: The blacklist assertions on configSource are too broad;
replace the three separate .not.toContain checks with a single targeted negative
regex match that looks for the removed git-remote detection pattern (e.g., a
pattern matching "git remote ... get-url") so the test only forbids that
specific behavior. Locate the variable configSource in the test and change the
assertions to use a negative regex assertion (not.toMatch) that covers the
intended sequence like /git\s+remote.*get-url/ to avoid accidentally blocking
unrelated words.
In `@src/main/services/calendar-manager.ts`:
- Around line 113-120: The bug is that cancelConnect() clears this.connecting
prematurely allowing a concurrent connect to run while an earlier connect's
finally can also clear the flag; change the lifecycle to tie the connecting flag
to a specific attempt id/promise: introduce an incrementing attemptId (e.g.,
this.connectAttemptId) that connect() sets at start and captures in its finally,
and only let that finally clear this.connecting if the captured attemptId
matches the current this.connectAttemptId; modify cancelConnect() to call each
provider.cancelConnect?.() but do NOT set this.connecting = false (or instead
mark the current attempt as cancelled without touching the global flag), so only
the originating connect() attempt clears the flag when it settles. Ensure the
identifiers referenced are cancelConnect, connect, this.connecting,
this.connectAttemptId (or similar), and provider.cancelConnect.
In `@src/main/services/calendar.ts`:
- Around line 143-180: The cancel/close logic currently calls server.close() and
then immediately settles (via resolveAndClose/rejectAndClose), which can leave
the port still bound; modify closeServer and the settle flow so server.close's
callback is awaited before finalizing: change closeServer to accept a callback
or return a Promise that calls server.close(err => { /* ignore err */
resolve/continue }); then update cleanup/resolveAndClose/rejectAndClose to wait
for that Promise (or pass a continuation) so settled is set and resolve/reject
run only after server.close's completion; keep references to
pendingCallbackCancel, cancel, timeout, cleanup, resolveAndClose, rejectAndClose
and server.listen/server.on('error') so the server is always closed fully before
settling.
In `@src/renderer/src/pages/Settings.test.tsx`:
- Around line 136-141: The test state uses an explicit any for events ("events:
[] as any[]"); replace this with a concrete event type instead of any — e.g.,
import the project's event type (such as CalendarEvent or EventsState) and
change the state entry to "events: [] as CalendarEvent[]" (or the matching
events array type from your store), ensuring the test imports the correct type
and the empty array is typed properly.
In `@worker/package.json`:
- Line 10: Update the TypeScript version in worker/package.json so it matches
the root package (replace the "typescript": "^5.0.0" entry with the same version
specifier used at the repo root, e.g., "^5.9.3", or remove the explicit entry
and rely on the workspace/hoisted root dependency); locate the "typescript"
field in worker/package.json and change it to the aligned version (or switch to
the workspace protocol like "workspace:^5.9.3") to ensure consistent
type-checking across the monorepo.
In `@worker/src/index.test.ts`:
- Around line 28-206: Add two boundary tests to assert the worker rejects
oversized state and code parameters: call fetchWorker('/auth/google?state=...')
with a state length of MAX_STATE_LENGTH+1 and expect a 400 with an "Invalid
state" response; call fetchWorker('/auth/callback?code=...&state=ok') with a
code length of MAX_CODE_LENGTH+1 and expect a 400 with a message like "Missing
authorization code" (or the worker's code-error text). Reference the constants
MAX_STATE_LENGTH and MAX_CODE_LENGTH and the existing test helpers (fetchWorker)
and endpoints ('/auth/google' and '/auth/callback') so the assertions mirror
style used in other tests and ensure upstream fetch is not stubbed/called for
these early rejections.
- Around line 51-62: Add a new test alongside the existing "exchanges Google
callback codes..." case that verifies provider error handling by stubbing global
fetch (using vi.stubGlobal) to return an error Response (e.g., JSON { error:
'invalid_grant' } with status 400), then call
fetchWorker('/auth/callback?code=bad-code&state=test-state') and assert the
response is a 302 redirect to the local callback (location.origin ===
'http://127.0.0.1:42813'), the redirect preserves state ('test-state'), and the
location.searchParams contains an error parameter that includes 'invalid_grant';
locate the test near the existing test that uses fetchWorker and Response to
ensure consistent setup.
In `@worker/src/index.ts`:
- Line 157: Add timeout protection for external OAuth HTTP calls by implementing
a fetchWithTimeout helper that uses AbortController and a setTimeout (default
~10_000 ms) to abort slow requests, then replace plain fetch usage inside
exchangeGoogleCode, exchangeMicrosoftCode, and the token refresh logic (the
provider refresh flows) to call fetchWithTimeout instead of fetch; ensure you
clear the timeout on completion and propagate the abort error so callers can
handle timeouts gracefully.
- Line 11: Replace the date-like numeric rate-limiter namespace IDs
('2026060801' and '2026060802') with stable, descriptive identifiers (e.g.,
'auth-flow-v1', 'token-exchange-v1') wherever they are used (search for the
literal strings '2026060801' and '2026060802'); update any constants, variables,
or config entries that reference those values so the namespaces remain constant
across deployments and clearly indicate purpose/version.
- Around line 342-344: The current actor fallback sets actor to the literal
'unknown' (from the request.headers lookup for CF-Connecting-IP /
x-forwarded-for) which forces all anonymous clients into one rate-limit bucket;
change the fallback logic in the actor assignment so that instead of the static
'unknown' you derive a more specific identifier using available headers (for
example prefer x-request-id, x-real-ip, a stable session cookie value, or a
deterministic fingerprint/hash of user-agent+accept-language when IP headers are
absent) and only use a random UUID as an absolute last resort; update the code
that sets actor (the variable named actor that reads CF-Connecting-IP and
x-forwarded-for from request.headers) and add a short comment documenting that
environments without client-identifying headers cannot be effectively
rate-limited.
- Around line 358-359: The Content-Length header parsing using Number() allows
malformed values to bypass the size check; update the validation around
contentLength so it only trusts an integer positive header (e.g., validate with
a /^\d+$/ test or parseInt and then check !Number.isNaN(parsed) and parsed >= 0)
before comparing to MAX_JSON_BODY_BYTES, and keep the existing actual-body-size
fallback; specifically change the logic that sets/uses contentLength and the
Number.isFinite check to first ensure the header is a well-formed non-negative
integer (referencing the contentLength variable and MAX_JSON_BODY_BYTES
comparison).
- Around line 259-261: The code trusts url.origin when building workerCallback
for Google flows; ensure the constructed redirect_uri is validated with
isAllowedLocalRedirectUri before using it in exchangeGoogleCode (and anywhere
else you construct `${url.origin}/auth/callback` such as in the /auth/google and
/auth/callback handlers). Concretely, call
isAllowedLocalRedirectUri(workerCallback) and return an error response (same
style as the Microsoft branch) if it returns false, then pass the validated
workerCallback to exchangeGoogleCode; also audit exchangeGoogleCode usage (and
/auth/refresh) to ensure no unvalidated redirect_uri is sent.
In `@worker/tsconfig.json`:
- Line 8: The root typecheck fails because root's `tsc -p worker/tsconfig.json`
cannot resolve worker-local deps like `@cloudflare/workers-types`; update the
root `package.json`'s `typecheck:worker` script to run `tsc` from inside the
worker directory (e.g., change it to run `cd worker && npm run typecheck` or
`pnpm --filter ./worker run typecheck`) and add a `typecheck` script to
`worker/package.json` (e.g., `"typecheck": "tsc --noEmit"`) so that `tsc`
executes with the worker's node_modules and the `@cloudflare/workers-types`
dependency is resolvable.
---
Outside diff comments:
In `@src/renderer/src/pages/Settings.tsx`:
- Around line 120-144: The success path in handleConnect is not guarded against
stale/abandoned attempts: after each await you must verify the attempt is still
current before mutating state or calling addAccount/setEvents; update
handleConnect to check connectAttemptRef.current === attemptId immediately after
the await that returns account and again after the await that returns events
(and skip addAccount/setEvents and further state updates if it no longer
matches), while leaving the existing finally cleanup as-is; reference symbols:
handleConnect, connectAttemptRef, attemptId, addAccount, setEvents,
setConnectingProvider, setConnecting.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a84117a-8799-48de-8c48-d7946444037c
⛔ Files ignored due to path filters (1)
worker/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.env.example.github/workflows/build.yml.github/workflows/pr-regression.yml.github/workflows/socket.ymlelectron.vite.config.tspackage.jsonsrc/main/ipc/__tests__/calendar-ipc.test.tssrc/main/ipc/calendar-ipc.tssrc/main/services/__tests__/calendar-oauth.test.tssrc/main/services/__tests__/official-build-config.test.tssrc/main/services/calendar-manager.tssrc/main/services/calendar-types.tssrc/main/services/calendar.tssrc/main/services/distribution-config.tssrc/main/services/microsoft-calendar.tssrc/preload/ipc.d.tssrc/renderer/src/pages/Settings.test.tsxsrc/renderer/src/pages/Settings.tsxsrc/renderer/src/services/__tests__/analytics.test.tssrc/renderer/src/services/analytics.tsworker/package.jsonworker/src/index.test.tsworker/src/index.tsworker/tsconfig.jsonworker/vitest.config.mtsworker/wrangler.toml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/build.yml:
- Around line 105-107: The macOS build job condition only runs for push branch
events; update the job's if expression (the one on the "Build macOS artifacts
(signed, no notarization)" step that currently reads "if: github.event_name ==
'push' && github.ref_type == 'branch'") to also run for manual dispatches when
the release_tag input is blank — e.g. change it to a combined predicate:
(github.event_name == 'push' && github.ref_type == 'branch') ||
(github.event_name == 'workflow_dispatch' && (github.event.inputs.release_tag ==
'' || github.event.inputs.release_tag == null)); apply the same updated
condition to the corresponding upload step so manual non-release
workflow_dispatch runs also produce artifacts.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fe12486-91a9-411b-ac11-300e8a4fba06
📒 Files selected for processing (2)
.github/workflows/build.ymlpackage.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 @.github/workflows/build.yml:
- Around line 105-107: The macOS build job condition only runs for push branch
events; update the job's if expression (the one on the "Build macOS artifacts
(signed, no notarization)" step that currently reads "if: github.event_name ==
'push' && github.ref_type == 'branch'") to also run for manual dispatches when
the release_tag input is blank — e.g. change it to a combined predicate:
(github.event_name == 'push' && github.ref_type == 'branch') ||
(github.event_name == 'workflow_dispatch' && (github.event.inputs.release_tag ==
'' || github.event.inputs.release_tag == null)); apply the same updated
condition to the corresponding upload step so manual non-release
workflow_dispatch runs also produce artifacts.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fe12486-91a9-411b-ac11-300e8a4fba06
📒 Files selected for processing (2)
.github/workflows/build.ymlpackage.json
🛑 Comments failed to post (1)
.github/workflows/build.yml (1)
105-107:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winManual non-release dispatches now skip the macOS build.
The
release_taginput explicitly allows blank values for non-release runs, but this predicate only matchespushbranches. Aworkflow_dispatchrun with an emptyrelease_tagskips the release build and this fallback build, so the job completes without producing artifacts. Please include the manual non-release case here and mirror the same condition on the upload step.Proposed fix
- name: Build macOS artifacts (signed, no notarization) - if: github.event_name == 'push' && github.ref_type == 'branch' + if: >- + ${{ + (github.event_name == 'push' && github.ref_type == 'branch') || + (github.event_name == 'workflow_dispatch' && inputs.release_tag == '') + }} run: npm run prepare:macos-mlx-runtime && npm run build && npx electron-builder --mac --publish never --config.mac.notarize=false - name: Upload macOS artifacts - if: github.event_name == 'push' && github.ref_type == 'branch' && success() + if: >- + ${{ + success() && + ( + (github.event_name == 'push' && github.ref_type == 'branch') || + (github.event_name == 'workflow_dispatch' && inputs.release_tag == '') + ) + }} uses: actions/upload-artifact@v7.0.1🤖 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 @.github/workflows/build.yml around lines 105 - 107, The macOS build job condition only runs for push branch events; update the job's if expression (the one on the "Build macOS artifacts (signed, no notarization)" step that currently reads "if: github.event_name == 'push' && github.ref_type == 'branch'") to also run for manual dispatches when the release_tag input is blank — e.g. change it to a combined predicate: (github.event_name == 'push' && github.ref_type == 'branch') || (github.event_name == 'workflow_dispatch' && (github.event.inputs.release_tag == '' || github.event.inputs.release_tag == null)); apply the same updated condition to the corresponding upload step so manual non-release workflow_dispatch runs also produce artifacts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/main/services/__tests__/calendar-validity.test.ts`:
- Around line 46-54: The helper createDeferred lacks an explicit return type and
a later test variable uses `any`; update createDeferred to declare its return
type as { promise: Promise<T>; resolve: (value: T) => void; reject: (reason?:
unknown) => void } (so the function signature becomes function
createDeferred<T>(): { promise: Promise<T>; resolve: (value: T) => void; reject:
(reason?: unknown) => void }) and replace the use of `any` at the test (line
~103) with a properly typed alternative (preferably a specific type or `unknown`
and narrow it where used, or make it generic to match createDeferred<T>),
ensuring the variable and any resolve/reject calls are typed consistently with
createDeferred's generic T.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 211d3de5-3c5d-443b-a66d-d2e9f99bed60
📒 Files selected for processing (8)
.github/workflows/build.ymlsrc/main/services/__tests__/calendar-oauth.test.tssrc/main/services/__tests__/calendar-validity.test.tssrc/main/services/calendar-manager.tssrc/main/services/calendar.tssrc/main/services/microsoft-calendar.tssrc/renderer/src/pages/Settings.test.tsxsrc/renderer/src/pages/Settings.tsx
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
Testing