Conversation
📝 WalkthroughWalkthroughAdds a HotkeyActivationMode (toggle vs hold), refactors HotkeyManager to emit onToggle/onHoldStart/onHoldEnd and support hold/toggle behavior, wires mode selection into Settings with persistence, updates app wiring to use new hotkey callbacks, and expands CI workflow pull_request event types. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Settings as SettingsView
participant HotkeyMgr as HotkeyManager
participant App as openwispherApp
participant Svc as RecordingService
User->>Settings: select activation mode
Settings->>HotkeyMgr: updateActivationMode(newMode)
HotkeyMgr->>HotkeyMgr: save mode to UserDefaults
alt Toggle mode
User->>HotkeyMgr: press hotkey
HotkeyMgr->>App: onToggle()
App->>App: check state (idle/recording)
alt idle
App->>Svc: start recording
else recording
App->>Svc: stop recording
end
else Hold mode
User->>HotkeyMgr: press & hold hotkey
HotkeyMgr->>App: onHoldStart()
App->>Svc: start recording (if idle)
User->>HotkeyMgr: release hotkey
HotkeyMgr->>App: onHoldEnd()
App->>Svc: stop recording (if recording)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 7-9: The workflow currently uses the pull_request_target trigger
and checks out untrusted fork HEAD (pull_request_target), which can execute
attacker-controlled code; change the workflow trigger to pull_request for any
steps that build or checkout PR code (i.e., move build/checkout steps out of
pull_request_target), or split into two workflows: keep pull_request_target only
for metadata actions (labeling/commenting) and create a separate pull_request or
workflow_run flow to perform checkouts/builds and consume artifacts with
secrets; ensure no step in the pull_request_target workflow (the one named by
pull_request_target) performs actions that checkout or run head PR code.
- Around line 30-31: The POSTHOG_API_KEY and POSTHOG_HOST env lines use a faulty
`&& ||` expression that never blanks secrets for pull_request_target; replace
that expression with an explicit conditional expression (e.g. use the `format`
or ternary-like idiom) or move secret population into a prior step that sets the
env only when `github.event_name != 'pull_request_target'`; specifically, update
the lines referencing POSTHOG_API_KEY and POSTHOG_HOST so they evaluate to an
empty string when `github.event_name == 'pull_request_target'` (for example
using `format('{0}', github.event_name == 'pull_request_target' && '' ||
secrets.POSTHOG_API_KEY)` corrected to `github.event_name ==
'pull_request_target' && '' || secrets.POSTHOG_API_KEY` replaced by an explicit
if/else via `format` or a step that conditionally sets env based on
`github.event_name`) and ensure the secret is never directly expanded when the
event is pull_request_target.
In `@dhavnii/Features/Hotkeys/HotkeyManager.swift`:
- Around line 312-315: When changing modes in updateActivationMode, ensure any
active hold is properly ended: if the current activationMode is .hold (or if
isHoldActive is true) and newMode is not .hold, call onHoldEnd() before
resetting isHoldActive and updating activationMode; this guarantees recordings
started by onHoldStart are stopped when switching to toggle. Reference
updateActivationMode, onHoldEnd, onHoldStart, activationMode, and isHoldActive.
🧹 Nitpick comments (1)
dhavnii/Features/Hotkeys/HotkeyManager.swift (1)
176-178: Callbacks are exposed as publicletproperties — consider restricting access.
onToggle,onHoldStart, andonHoldEndare implicitlyinternal. Since they're only expected to be invoked by the event handler within this class, they could beprivateto prevent external callers from triggering them directly.Proposed fix
- let onToggle: () -> Void - let onHoldStart: () -> Void - let onHoldEnd: () -> Void + private let onToggle: () -> Void + private let onHoldStart: () -> Void + private let onHoldEnd: () -> Void
Comment on lines
+30
to
+31
| POSTHOG_API_KEY: ${{ github.event_name == 'pull_request_target' && '' || secrets.POSTHOG_API_KEY }} | ||
| POSTHOG_HOST: ${{ github.event_name == 'pull_request_target' && '' || secrets.POSTHOG_HOST }} |
There was a problem hiding this comment.
Critical: Ternary expressions don't actually blank secrets for pull_request_target.
In GitHub Actions expressions, && returns the last truthy operand (or the first falsy one), and || returns the first truthy operand. When github.event_name == 'pull_request_target':
true && '' || secrets.POSTHOG_API_KEY
→ '' || secrets.POSTHOG_API_KEY
→ secrets.POSTHOG_API_KEY ← empty string is falsy, so || falls through!
The secrets are never blanked — they always resolve to the secret value. Combined with the pull_request_target checkout of fork code (Line 26), this exposes secrets to attacker-controlled builds.
Use an explicit if/else via the format function or a dedicated step:
🔒 Proposed fix
- name: Build (unsigned)
env:
- POSTHOG_API_KEY: ${{ github.event_name == 'pull_request_target' && '' || secrets.POSTHOG_API_KEY }}
- POSTHOG_HOST: ${{ github.event_name == 'pull_request_target' && '' || secrets.POSTHOG_HOST }}
+ POSTHOG_API_KEY: ${{ github.event_name != 'pull_request_target' && secrets.POSTHOG_API_KEY || '' }}
+ POSTHOG_HOST: ${{ github.event_name != 'pull_request_target' && secrets.POSTHOG_HOST || '' }}This way, when the event is pull_request_target, the condition is false && secret || '' → '' , which correctly yields empty. When it's not pull_request_target, it's true && secret || '' → secret (assuming the secret is non-empty).
📝 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.
Suggested change
| POSTHOG_API_KEY: ${{ github.event_name == 'pull_request_target' && '' || secrets.POSTHOG_API_KEY }} | |
| POSTHOG_HOST: ${{ github.event_name == 'pull_request_target' && '' || secrets.POSTHOG_HOST }} | |
| POSTHOG_API_KEY: ${{ github.event_name != 'pull_request_target' && secrets.POSTHOG_API_KEY || '' }} | |
| POSTHOG_HOST: ${{ github.event_name != 'pull_request_target' && secrets.POSTHOG_HOST || '' }} |
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 30 - 31, The POSTHOG_API_KEY and
POSTHOG_HOST env lines use a faulty `&& ||` expression that never blanks secrets
for pull_request_target; replace that expression with an explicit conditional
expression (e.g. use the `format` or ternary-like idiom) or move secret
population into a prior step that sets the env only when `github.event_name !=
'pull_request_target'`; specifically, update the lines referencing
POSTHOG_API_KEY and POSTHOG_HOST so they evaluate to an empty string when
`github.event_name == 'pull_request_target'` (for example using `format('{0}',
github.event_name == 'pull_request_target' && '' || secrets.POSTHOG_API_KEY)`
corrected to `github.event_name == 'pull_request_target' && '' ||
secrets.POSTHOG_API_KEY` replaced by an explicit if/else via `format` or a step
that conditionally sets env based on `github.event_name`) and ensure the secret
is never directly expanded when the event is pull_request_target.
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 by CodeRabbit