From f0af9251cc8e723b27d8073073b201cf548b1d9c Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Thu, 20 Aug 2026 17:01:41 -0500 Subject: [PATCH 1/2] Let people choose their push-to-talk key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Push-to-talk was Shift, for everyone, forever. Shift is a key people are already pressing all day, which is why the gesture needed a 350ms hold delay and a selection check to tell talking from typing a capital — and why it still couldn't work at all for anyone whose browser or OS already spends the key. It's now a setting: Ctrl+Space (the new default), Shift, Option/Alt, or off. Presets rather than a free capture, because each one behaves differently on purpose. A chord is nobody's accident, so it records from the press — no delay to sit out and nothing to guess. A bare modifier keeps all the old hedging, because the ambiguity is real. Everyone who was already here is written down as a Shift user by a backfill, so no hands have to relearn anything on deploy; "no preference recorded" now means "arrived after the setting existed". The mic names its own key on hover and to a screen reader — with the shortcut configurable, the button is the only thing on the page that can say which one yours is. The server renders it; the controller rewrites it knowing whether that key says Option or Alt on this machine. Also folds the theme switcher's CSS into a shared .segmented control rather than growing a second copy of it. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 1 + ...ackfill_voice_hotkey_preference.co_plan.rb | 32 +++++ db/schema.rb | 2 +- .../assets/stylesheets/coplan/application.css | 59 +++++++-- .../coplan/settings/settings_controller.rb | 9 ++ engine/app/helpers/coplan/plans_helper.rb | 10 ++ .../controllers/coplan/voice_controller.js | 113 ++++++++++++++---- .../coplan/voice_hotkey_controller.js | 29 +++++ engine/app/models/coplan/user.rb | 32 +++++ .../coplan/plans/_voice_control.html.erb | 7 +- .../coplan/settings/settings/_voice.html.erb | 37 ++++++ .../coplan/settings/settings/index.html.erb | 16 +-- engine/config/routes.rb | 1 + ...000000_backfill_voice_hotkey_preference.rb | 31 +++++ spec/requests/settings_spec.rb | 28 +++++ spec/system/nav_stimulus_spec.rb | 2 +- spec/system/voice_commenting_spec.rb | 99 +++++++++++++++ 17 files changed, 468 insertions(+), 40 deletions(-) create mode 100644 db/migrate/20260820195225_backfill_voice_hotkey_preference.co_plan.rb create mode 100644 engine/app/javascript/controllers/coplan/voice_hotkey_controller.js create mode 100644 engine/app/views/coplan/settings/settings/_voice.html.erb create mode 100644 engine/db/migrate/20260820000000_backfill_voice_hotkey_preference.rb diff --git a/AGENTS.md b/AGENTS.md index 118ff111..9e776935 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -167,6 +167,7 @@ reader to an apparently empty page. - `a` — accept the current pending thread - `d` — discard the current pending thread - `Enter` — submit reply; `Shift+Enter` — newline +- Push-to-talk (hold to dictate a comment) is a per-user setting — `Ctrl+Space` by default, or Shift / Option / off (`CoPlan::User::VOICE_HOTKEYS`, `voice_controller.js`). A bare modifier has to be held past a delay to tell talking from typing; a chord records from the press. ### How thread data flows - Thread data is **server-rendered** as hidden `[data-anchor-text]` elements in `#plan-threads` (via `_thread_popover.html.erb`) diff --git a/db/migrate/20260820195225_backfill_voice_hotkey_preference.co_plan.rb b/db/migrate/20260820195225_backfill_voice_hotkey_preference.co_plan.rb new file mode 100644 index 00000000..44ff9273 --- /dev/null +++ b/db/migrate/20260820195225_backfill_voice_hotkey_preference.co_plan.rb @@ -0,0 +1,32 @@ +# This migration comes from co_plan (originally 20260820000000) +class BackfillVoiceHotkeyPreference < ActiveRecord::Migration[8.1] + # Push-to-talk used to be Shift, for everyone, with no way to change it. + # It is now a setting, and the default for anyone new is Ctrl+Space — + # a deliberate chord that can open the microphone the instant it's + # pressed, where a bare Shift has to wait out a hold delay to tell + # talking from typing a capital. + # + # Nobody's hands should have to relearn that on a deploy, so everyone + # who already exists is written down as a Shift user explicitly. From + # here on, "no preference recorded" means "arrived after the setting + # existed" and reads as Ctrl+Space. + # + # Idempotent: only users with no voice_hotkey recorded are touched. + def up + CoPlan::User.find_each do |user| + metadata = user.metadata || {} + next if metadata.key?("voice_hotkey") + + user.update_column(:metadata, metadata.merge("voice_hotkey" => "shift")) # rubocop:disable Rails/SkipsModelValidations + end + end + + def down + CoPlan::User.find_each do |user| + metadata = user.metadata + next unless metadata.is_a?(Hash) && metadata["voice_hotkey"] == "shift" + + user.update_column(:metadata, metadata.except("voice_hotkey")) # rubocop:disable Rails/SkipsModelValidations + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 847d2021..897163a2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_19_144056) do +ActiveRecord::Schema[8.1].define(version: 2026_08_20_195225) do create_table "active_admin_comments", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.bigint "author_id" t.string "author_type" diff --git a/engine/app/assets/stylesheets/coplan/application.css b/engine/app/assets/stylesheets/coplan/application.css index 2986626e..a2a94ac8 100644 --- a/engine/app/assets/stylesheets/coplan/application.css +++ b/engine/app/assets/stylesheets/coplan/application.css @@ -832,6 +832,7 @@ img, svg { } .voice-control .voice-btn { + position: relative; display: inline-flex; align-items: center; justify-content: center; @@ -852,6 +853,40 @@ img, svg { transform: scale(1.06); } +/* Hovering names the key. Push-to-talk is chosen in Settings and nothing + else on the page says what it ended up as — a mic you have to hold the + right key for is only discoverable if the mic tells you which one. + Written by the controller, which knows the platform: the same + preference reads as ⌥ Option on a Mac and Alt everywhere else. */ +.voice-control .voice-btn[data-tooltip]::after { + content: attr(data-tooltip); + position: absolute; + bottom: calc(100% + 8px); + right: 0; + background: var(--color-text); + color: var(--color-surface); + padding: 4px 8px; + border-radius: var(--radius); + font-size: 0.75rem; + font-weight: 500; + white-space: nowrap; + pointer-events: none; + opacity: 0; + transition: opacity 0.15s; + z-index: 10; +} + +.voice-control .voice-btn[data-tooltip]:hover::after { + opacity: 1; +} + +/* Mid-take the button is a stop button, and the status text is already + saying what's happening — an instruction for starting would contradict + both. */ +.voice-control .voice-btn--listening[data-tooltip]::after { + content: none; +} + /* Recording reads as recording: the button fills red, and a ring grows with how loud you are. The fill says "the mic is open"; only the ring says "it can hear you", which is the question you actually have while @@ -2531,29 +2566,35 @@ img.avatar { margin-top: var(--space-xs); } -/* Theme switcher (segmented control) */ -.theme-switcher { +/* Segmented control (theme switcher, push-to-talk key) */ +.segmented { display: inline-flex; + /* The control is the row's point; the explanation next to it is what + gives way when the window is narrow. */ + flex: 0 0 auto; border: 1px solid var(--color-border); border-radius: var(--radius); overflow: hidden; } -.theme-switcher__option { +.segmented__option { cursor: pointer; margin: 0; } -.theme-switcher__option input[type="radio"] { +.segmented__option input[type="radio"] { position: absolute; opacity: 0; pointer-events: none; } -.theme-switcher__btn { +.segmented__btn { display: inline-flex; align-items: center; gap: var(--space-xs); + /* Segments are labels for one choice each — a wrapped one reads as two + options and leaves the control ragged. */ + white-space: nowrap; padding: var(--space-sm) var(--space-md); font-size: var(--text-sm); font-weight: 500; @@ -2562,20 +2603,20 @@ img.avatar { border-right: 1px solid var(--color-border); } -.theme-switcher__option:last-child .theme-switcher__btn { +.segmented__option:last-child .segmented__btn { border-right: none; } -.theme-switcher__btn svg { +.segmented__btn svg { flex-shrink: 0; } -.theme-switcher__option:hover .theme-switcher__btn { +.segmented__option:hover .segmented__btn { background: var(--color-bg-muted); color: var(--color-text); } -.theme-switcher__option:has(input:checked) .theme-switcher__btn { +.segmented__option:has(input:checked) .segmented__btn { background: var(--color-primary-light); color: var(--color-primary); font-weight: 600; diff --git a/engine/app/controllers/coplan/settings/settings_controller.rb b/engine/app/controllers/coplan/settings/settings_controller.rb index f5604fd7..098a04e1 100644 --- a/engine/app/controllers/coplan/settings/settings_controller.rb +++ b/engine/app/controllers/coplan/settings/settings_controller.rb @@ -14,6 +14,15 @@ def update_theme end head :ok end + + def update_voice_hotkey + hotkey = params[:voice_hotkey] + if CoPlan::User::VOICE_HOTKEYS.include?(hotkey) + current_user.voice_hotkey = hotkey + current_user.save! + end + head :ok + end end end end diff --git a/engine/app/helpers/coplan/plans_helper.rb b/engine/app/helpers/coplan/plans_helper.rb index d9da7ca0..5a43459d 100644 --- a/engine/app/helpers/coplan/plans_helper.rb +++ b/engine/app/helpers/coplan/plans_helper.rb @@ -87,6 +87,16 @@ def plan_type_icon(plan, size: :md) aria: { label: "#{plan_type.name} document" }) end + # What the mic button says about itself. The push-to-talk key is a + # setting, so the button is the only place on the page that can tell + # you which key yours ended up being — and "hold something to talk" + # is worse than saying nothing. + def voice_button_description(hotkey) + return "Comment by voice" if hotkey == "off" + + "Comment by voice — or hold #{User::VOICE_HOTKEY_LABELS[hotkey]} to talk" + end + def plan_content_preview(plan, limit: 200) stub = plan.current_version_stub return nil if stub.nil? diff --git a/engine/app/javascript/controllers/coplan/voice_controller.js b/engine/app/javascript/controllers/coplan/voice_controller.js index 705dcea2..9cef8ada 100644 --- a/engine/app/javascript/controllers/coplan/voice_controller.js +++ b/engine/app/javascript/controllers/coplan/voice_controller.js @@ -3,9 +3,10 @@ import { Controller } from "@hotwired/stimulus" /* * coplan--voice * - * Push-to-talk commenting. Hold Shift (or tap the mic), say "this section - * is way too formal", and what you said is posted as a comment pinned to - * the passage you were looking at. + * Push-to-talk commenting. Hold the key (or tap the mic), say "this + * section is way too formal", and what you said is posted as a comment + * pinned to the passage you were looking at. Which key is a setting — + * see HOTKEYS below, and CoPlan::User::VOICE_HOTKEYS for the list. * * Two ways to capture, and which one is in play matters: * @@ -32,17 +33,38 @@ import { Controller } from "@hotwired/stimulus" */ export default class extends Controller { static targets = ["button", "status"] - static values = { url: String, dictationUrl: String, transcription: Boolean } + static values = { url: String, dictationUrl: String, transcription: Boolean, hotkey: String } // Peak deviation from silence, on the 0–128 scale of the time-domain // samples. Room tone sits in the low single digits; speech is well // past 20. Anything under this never made it to the microphone. static SILENCE_PEAK = 6 - // How long Shift must be held before the press means "talk" — a tap - // that short is someone typing a capital letter. + // How long a bare modifier must be held before the press means "talk" + // — a tap that short is someone typing a capital letter. static HOLD_DELAY = 350 + // The push-to-talk keys, and the two kinds of gesture they are. + // + // A bare modifier is a key people are already pressing for other + // reasons all day, so it has to earn the microphone: held past the + // delay, with nothing else down and nothing selected. A chord is + // nobody's accident, so it starts recording on the press — no delay to + // sit out, and no guessing about what was meant. + static HOTKEYS = { + ctrl_space: { + label: "Ctrl+Space", + matches: (event) => event.ctrlKey && event.code === "Space", + // Either half going up ends the take: whichever finger lifts + // first, the gesture is over. + releases: (event) => event.code === "Space" || event.key === "Control" + }, + shift: { label: "Shift", modifier: "Shift" }, + alt: { label: "Alt", macLabel: "⌥ Option", modifier: "Alt" } + } + + static MODIFIER_FLAGS = { Shift: "shiftKey", Alt: "altKey", Control: "ctrlKey", Meta: "metaKey" } + connect() { this.mode = this._chooseMode() if (!this.mode) { @@ -50,6 +72,11 @@ export default class extends Controller { return } + // "off" (and anything unrecognized) leaves the mic button as the only + // way in, which is a legitimate choice rather than a broken state. + this.hotkey = this.constructor.HOTKEYS[this.hotkeyValue] || null + this._describeButton() + if (this.mode === "recognize") this._setUpRecognition() this.listening = false @@ -92,27 +119,60 @@ export default class extends Controller { this._setStatus(e.error === "no-speech" ? "Didn't catch that" : "Mic error", true) } + // The mic names its own key, on hover and to a screen reader. The + // server renders a platform-neutral version of the same sentence; by + // here we know whether the key in front of this person is labelled + // Option or Alt, and a shortcut named after the wrong key is worse + // than no shortcut at all. + _describeButton() { + const label = this._hotkeyLabel() + const description = label ? `Comment by voice — or hold ${label} to talk` : "Comment by voice" + this.buttonTarget.dataset.tooltip = description + this.buttonTarget.setAttribute("aria-label", description) + } + + _hotkeyLabel() { + if (!this.hotkey) return null + + const platform = navigator.userAgentData?.platform || navigator.platform || navigator.userAgent + const isMac = /mac|iphone|ipad/i.test(platform) + return (isMac && this.hotkey.macLabel) || this.hotkey.label + } + // ── The gesture ──────────────────────────────────────────────────── toggle() { this.listening ? this._stop() : this._start() } - // Hold Shift to talk, release to send. Bound declaratively on the + // Hold the key to talk, release to send. Bound declaratively on the // control's element (keydown@document / keyup@document / blur@window) // — document and window are stable targets, so Stimulus owns the // listener lifecycle. - // - // Shift is also held while typing capitals and extending a selection, - // so a bare press isn't enough of a signal. Three guards keep it from - // firing by accident: it must be Shift alone with no other key down, it - // must be held past a short delay (capitals are a tap), and any other - // keystroke or a text selection cancels without posting. keyDown(event) { - if (!this.mode) return + if (!this.mode || !this.hotkey) return + if (this.hotkey.modifier) return this._modifierDown(event) - if (event.key !== "Shift") { - // Shift+something is a shortcut or a capital letter, not talking. + if (!this.hotkey.matches(event)) return + if (event.repeat || this.pushToTalk || this.listening) return + if (this._isTyping()) return + + // A chord is unambiguous, so it takes the key outright — nothing + // else on the page gets to treat this press as its own. + event.preventDefault() + this.pushToTalk = true + this._start() + } + + // A bare modifier is also held while typing capitals, holding down + // Option for a special character, and extending a selection, so the + // press alone isn't enough of a signal. Three guards keep it from + // firing by accident: it must be that modifier alone with no other key + // down, it must be held past a short delay (capitals are a tap), and + // any other keystroke or a text selection cancels without posting. + _modifierDown(event) { + if (event.key !== this.hotkey.modifier) { + // Modifier+something is a shortcut or a capital letter, not talking. if (this.pushToTalk) this._cancel() else if (this.ear) { clearTimeout(this.holdTimer) @@ -121,7 +181,7 @@ export default class extends Controller { return } if (event.repeat || this.pushToTalk || this.listening) return - if (this._isTyping() || event.metaKey || event.ctrlKey || event.altKey) return + if (this._isTyping() || this._otherModifierHeld(event)) return // Capture from the instant of the press. Deciding whether the press // means "talk" takes 350ms and opening the microphone takes a couple @@ -133,7 +193,8 @@ export default class extends Controller { this.holdTimer = setTimeout(() => { // Extending a selection with Shift+arrow or Shift+click also - // holds Shift; if text got selected, that's what was happening. + // holds the modifier; if text got selected, that's what was + // happening. if (!window.getSelection()?.isCollapsed) { this._closeEar() return @@ -144,7 +205,8 @@ export default class extends Controller { } keyUp(event) { - if (!this.mode || event.key !== "Shift") return + if (!this.mode || !this.hotkey) return + if (!this._releasesHotkey(event)) return clearTimeout(this.holdTimer) if (!this.pushToTalk) { @@ -156,6 +218,15 @@ export default class extends Controller { this._stop() } + _releasesHotkey(event) { + return this.hotkey.modifier ? event.key === this.hotkey.modifier : this.hotkey.releases(event) + } + + _otherModifierHeld(event) { + return Object.entries(this.constructor.MODIFIER_FLAGS) + .some(([key, flag]) => key !== this.hotkey.modifier && event[flag]) + } + // Losing the window means we never see the keyup — don't leave the // mic open, and don't post something half-said. That includes losing // it during the hold delay, before push-to-talk is confirmed: the @@ -390,8 +461,8 @@ export default class extends Controller { // // The meter's verdict only counts while `meterLive` is true. An // AudioContext starts suspended unless the browser saw a qualifying - // user gesture, and Chrome does not count a bare Shift keydown as one — - // so on the push-to-talk path the context routinely comes up dead, and + // user gesture, and Chrome does not count a bare modifier keydown as + // one — so on the push-to-talk path the context routinely comes up dead, and // a suspended analyser reads as perfect silence. Trusting that reading // meant refusing recordings people were audibly speaking into. _meterLevels() { diff --git a/engine/app/javascript/controllers/coplan/voice_hotkey_controller.js b/engine/app/javascript/controllers/coplan/voice_hotkey_controller.js new file mode 100644 index 00000000..f8d5dc1e --- /dev/null +++ b/engine/app/javascript/controllers/coplan/voice_hotkey_controller.js @@ -0,0 +1,29 @@ +import { Controller } from "@hotwired/stimulus" + +/* + * coplan--voice-hotkey + * + * Saves the push-to-talk key as soon as it's picked — settings here don't + * have a save button. Nothing on this page reads the key, so there is no + * local state to update; the plan page picks it up on its next load. + */ +export default class extends Controller { + static targets = ["macNote"] + static values = { url: String } + + select(event) { + const hotkey = event.target.value + + if (this.hasMacNoteTarget) this.macNoteTarget.hidden = hotkey !== "ctrl_space" + + const csrfToken = document.querySelector('meta[name="csrf-token"]')?.content + fetch(this.urlValue, { + method: "PATCH", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + "X-CSRF-Token": csrfToken + }, + body: `voice_hotkey=${hotkey}` + }) + } +} diff --git a/engine/app/models/coplan/user.rb b/engine/app/models/coplan/user.rb index 140f5785..101f3b5d 100644 --- a/engine/app/models/coplan/user.rb +++ b/engine/app/models/coplan/user.rb @@ -2,6 +2,25 @@ module CoPlan class User < ApplicationRecord THEME_PREFERENCES = %w[system light dark].freeze + # Which key holds the microphone open for push-to-talk commenting. + # A short list rather than a free capture: each one carries its own + # feel in the browser (a bare modifier needs a hold delay, because + # people press it while typing; a chord doesn't), and the tooltip on + # the mic has to be able to name it. + VOICE_HOTKEYS = %w[ctrl_space shift alt off].freeze + DEFAULT_VOICE_HOTKEY = "ctrl_space".freeze + + # Names for the keys, for the settings row and the mic's own tooltip. + # Alt is written out both ways because the server can't know which + # keyboard is in front of the person; the voice controller narrows it + # to ⌥ Option on a Mac once it's running. + VOICE_HOTKEY_LABELS = { + "ctrl_space" => "Ctrl+Space", + "shift" => "Shift", + "alt" => "Option / Alt", + "off" => "Off" + }.freeze + has_many :api_tokens, dependent: :destroy has_many :created_plans, class_name: "CoPlan::Plan", foreign_key: :created_by_user_id, dependent: :nullify, inverse_of: :created_by_user has_many :created_folders, class_name: "CoPlan::Folder", foreign_key: :created_by_user_id, dependent: :nullify, inverse_of: :created_by_user @@ -42,5 +61,18 @@ def theme_preference=(value) self.metadata["theme_preference"] = value end + # Unset means Ctrl+Space. Everyone who was already here when the + # setting arrived had their old key (Shift) written down by the + # backfill, so "no preference" only ever means "new here". + def voice_hotkey + key = metadata&.dig("voice_hotkey") + VOICE_HOTKEYS.include?(key) ? key : DEFAULT_VOICE_HOTKEY + end + + def voice_hotkey=(value) + self.metadata ||= {} + self.metadata["voice_hotkey"] = value + end + end end diff --git a/engine/app/views/coplan/plans/_voice_control.html.erb b/engine/app/views/coplan/plans/_voice_control.html.erb index f3c4ffcb..49499ba2 100644 --- a/engine/app/views/coplan/plans/_voice_control.html.erb +++ b/engine/app/views/coplan/plans/_voice_control.html.erb @@ -18,10 +18,15 @@ data-coplan--voice-url-value="<%= plan_comment_threads_path(plan) %>" data-coplan--voice-dictation-url-value="<%= plan_dictations_path(plan) %>" data-coplan--voice-transcription-value="<%= CoPlan::Ai.available? %>" + data-coplan--voice-hotkey-value="<%= current_user.voice_hotkey %>" class="voice-control"> + <%# The tooltip and label name the push-to-talk key, which is a per-user + setting: rendered here for the server's part of it, then rewritten by + the controller, which knows whether this is a Mac. %> diff --git a/engine/app/views/coplan/settings/settings/_voice.html.erb b/engine/app/views/coplan/settings/settings/_voice.html.erb new file mode 100644 index 00000000..95ccb664 --- /dev/null +++ b/engine/app/views/coplan/settings/settings/_voice.html.erb @@ -0,0 +1,37 @@ +<%# Which key holds the microphone open. Push-to-talk was Shift and only + Shift, which is a poor fit for anyone who types capitals near the mic + and impossible for anyone whose browser or OS already spends the key. + + Presets rather than a free capture: each one behaves differently on + purpose (a bare modifier has to be held past a delay to tell talking + from typing; a chord starts recording on the press), and the mic's + tooltip has to be able to name whichever one is in force. %> +
+
+
+ Hold to talk +

+ Hold this key on a plan to dictate a comment, and release to post it. + The mic button works whichever you pick. + <%# Shown only while Ctrl+Space is the choice, and toggled live by the + controller so picking it here explains itself immediately. %> + > + macOS uses Ctrl+Space to switch input sources — if nothing happens when + you hold it, turn that off in System Settings or choose another key here. + +

+
+
+ <% CoPlan::User::VOICE_HOTKEYS.each do |hotkey| %> + + <% end %> +
+
+
diff --git a/engine/app/views/coplan/settings/settings/index.html.erb b/engine/app/views/coplan/settings/settings/index.html.erb index 7ce666ca..bbc3cd2d 100644 --- a/engine/app/views/coplan/settings/settings/index.html.erb +++ b/engine/app/views/coplan/settings/settings/index.html.erb @@ -5,24 +5,24 @@
Theme -
-
+<%= render "voice" %> + <%= render "notifications" %> <%= render "coplan/settings/tokens/tokens", api_tokens: @api_tokens %> diff --git a/engine/config/routes.rb b/engine/config/routes.rb index 3c955e61..237a8218 100644 --- a/engine/config/routes.rb +++ b/engine/config/routes.rb @@ -33,6 +33,7 @@ root "settings#index" resources :tokens, only: [:index, :create, :destroy] patch "theme", to: "settings#update_theme" + patch "voice_hotkey", to: "settings#update_voice_hotkey" end # Web folder creation (sidebar "New folder" input) and reparenting (drag diff --git a/engine/db/migrate/20260820000000_backfill_voice_hotkey_preference.rb b/engine/db/migrate/20260820000000_backfill_voice_hotkey_preference.rb new file mode 100644 index 00000000..e8bf8870 --- /dev/null +++ b/engine/db/migrate/20260820000000_backfill_voice_hotkey_preference.rb @@ -0,0 +1,31 @@ +class BackfillVoiceHotkeyPreference < ActiveRecord::Migration[8.1] + # Push-to-talk used to be Shift, for everyone, with no way to change it. + # It is now a setting, and the default for anyone new is Ctrl+Space — + # a deliberate chord that can open the microphone the instant it's + # pressed, where a bare Shift has to wait out a hold delay to tell + # talking from typing a capital. + # + # Nobody's hands should have to relearn that on a deploy, so everyone + # who already exists is written down as a Shift user explicitly. From + # here on, "no preference recorded" means "arrived after the setting + # existed" and reads as Ctrl+Space. + # + # Idempotent: only users with no voice_hotkey recorded are touched. + def up + CoPlan::User.find_each do |user| + metadata = user.metadata || {} + next if metadata.key?("voice_hotkey") + + user.update_column(:metadata, metadata.merge("voice_hotkey" => "shift")) # rubocop:disable Rails/SkipsModelValidations + end + end + + def down + CoPlan::User.find_each do |user| + metadata = user.metadata + next unless metadata.is_a?(Hash) && metadata["voice_hotkey"] == "shift" + + user.update_column(:metadata, metadata.except("voice_hotkey")) # rubocop:disable Rails/SkipsModelValidations + end + end +end diff --git a/spec/requests/settings_spec.rb b/spec/requests/settings_spec.rb index ccf50df0..0bd759e6 100644 --- a/spec/requests/settings_spec.rb +++ b/spec/requests/settings_spec.rb @@ -18,4 +18,32 @@ expect(alice.reload.theme_preference).not_to eq("hotdog-stand") end end + + describe "PATCH /settings/voice_hotkey" do + it "persists an allowed push-to-talk key" do + patch settings_voice_hotkey_path, params: { voice_hotkey: "shift" } + expect(response).to have_http_status(:ok) + expect(alice.reload.voice_hotkey).to eq("shift") + end + + # Nobody has a key, which is a real answer: the mic button stays and + # the keyboard is left alone. + it "persists turning the hotkey off" do + patch settings_voice_hotkey_path, params: { voice_hotkey: "off" } + expect(alice.reload.voice_hotkey).to eq("off") + end + + it "silently ignores keys that aren't on the list" do + patch settings_voice_hotkey_path, params: { voice_hotkey: "F13" } + expect(response).to have_http_status(:ok) + expect(alice.reload.voice_hotkey).to eq(CoPlan::User::DEFAULT_VOICE_HOTKEY) + end + + # A preference nobody has set means "arrived after the setting did", + # which reads as Ctrl+Space — the backfill gave everyone who came + # before an explicit Shift. + it "defaults to Ctrl+Space" do + expect(create(:coplan_user).voice_hotkey).to eq("ctrl_space") + end + end end diff --git a/spec/system/nav_stimulus_spec.rb b/spec/system/nav_stimulus_spec.rb index 0f189e35..f2edb2ea 100644 --- a/spec/system/nav_stimulus_spec.rb +++ b/spec/system/nav_stimulus_spec.rb @@ -57,7 +57,7 @@ def sign_in(u) describe "theme switcher" do it "applies the chosen theme immediately and persists it across reload" do visit settings_root_path - find(".theme-switcher__option", text: "Dark").click + find(".segmented__option", text: "Dark").click expect(page.evaluate_script("document.documentElement.getAttribute('data-theme')")).to eq("dark") expect(user.reload.theme_preference).to eq("dark") diff --git a/spec/system/voice_commenting_spec.rb b/spec/system/voice_commenting_spec.rb index 3734a317..382daea3 100644 --- a/spec/system/voice_commenting_spec.rb +++ b/spec/system/voice_commenting_spec.rb @@ -140,10 +140,108 @@ def stub_speech_recognition(transcript, emit_on_stop: false) expect(body).to include("too vague") end + # The default key, and the reason it's the default: a chord is nobody's + # accident, so it needs none of the hold-delay hedging a bare modifier + # does — the press means talk. + describe "hold Ctrl+Space to talk" do + before do + allow(CoPlan::Ai).to receive(:call) + .and_return({ "text" => "Held to talk.", "span" => nil }.to_json) + end + + after { page.driver.browser.action.release_actions } + + def hold_ctrl_space + page.driver.browser.action.key_down(:control).key_down(:space).perform + yield if block_given? + ensure + page.driver.browser.action.key_up(:space).key_up(:control).perform + end + + it "records while Ctrl+Space is held and posts on release" do + stub_speech_recognition("held to talk", emit_on_stop: true) + visit plan_path(plan) + + hold_ctrl_space { expect(page).to have_css(".voice-btn--listening") } + + expect(page).to have_css(".voice-status", text: /Comment added/, wait: 10) + thread = CoPlan::CommentThread.where(plan_id: plan.id).sole + expect(thread.comments.first.body_markdown).to eq("🎙️ Held to talk.") + end + + # No hold delay on a chord: waiting 350ms to decide would clip the + # first word off every short remark for no gain, because there is + # nothing to decide. + it "starts on the press rather than waiting out a hold" do + stub_speech_recognition("held to talk", emit_on_stop: true) + visit plan_path(plan) + + page.driver.browser.action.key_down(:control).key_down(:space).perform + expect(page).to have_css(".voice-btn--listening", wait: 0.3) + ensure + page.driver.browser.action.release_actions + end + + # Space alone scrolls the page and Ctrl alone is half a shortcut; + # neither is somebody asking to talk. + it "ignores either half of the chord on its own" do + stub_speech_recognition("should never be sent", emit_on_stop: true) + visit plan_path(plan) + + page.driver.browser.action.key_down(:control).key_up(:control).perform + page.driver.browser.action.send_keys(:space).perform + + expect(page).to have_no_css(".voice-btn--listening", wait: 2) + expect(CoPlan::CommentThread.where(plan_id: plan.id).count).to eq(0) + end + end + + # The key is a per-user setting, so the mic is the only thing on the + # page that can say which one yours is. Hovering has to answer it, or + # push-to-talk is a feature you have to remember you configured. + describe "the mic naming its own key" do + it "shows the chosen shortcut on hover" do + stub_speech_recognition("never sent") + visit plan_path(plan) + + expect(page).to have_css(".voice-btn[data-tooltip*='Ctrl+Space']") + + author.update!(voice_hotkey: "shift") + visit plan_path(plan) + expect(page).to have_css(".voice-btn[data-tooltip*='Shift']") + + # Nothing to hold, nothing to promise. + author.update!(voice_hotkey: "off") + visit plan_path(plan) + expect(page).to have_css(".voice-btn[data-tooltip='Comment by voice']") + end + end + + # "Off" is a supported answer, not a broken state: the mic button is + # still there, and the keyboard belongs to the page again. + describe "with the hotkey turned off" do + before { author.update!(voice_hotkey: "off") } + + it "leaves the keyboard alone but keeps the button" do + stub_speech_recognition("should never be sent", emit_on_stop: true) + visit plan_path(plan) + + page.driver.browser.action.key_down(:control).key_down(:space).perform + expect(page).to have_no_css(".voice-btn--listening", wait: 2) + page.driver.browser.action.release_actions + + expect(CoPlan::CommentThread.where(plan_id: plan.id).count).to eq(0) + expect(page).to have_css(".voice-btn") + end + end + + # What everyone who was here before the setting existed still has, + # because the backfill wrote it down for them. describe "hold Shift to talk" do HOLD = 0.7 # comfortably past the controller's 350ms hold delay before do + author.update!(voice_hotkey: "shift") allow(CoPlan::Ai).to receive(:call) .and_return({ "text" => "Held to talk.", "span" => nil }.to_json) end @@ -404,6 +502,7 @@ def stub_recorder(peak: 30, context_state: "running") # ear now opens at the keydown itself, and confirming the hold adopts # a capture already in progress. it "captures from the press when talking with Shift held" do + author.update!(voice_hotkey: "shift") allow(CoPlan::Ai).to receive(:transcribe).and_return("oh I meant both of them") allow(CoPlan::Ai).to receive(:call) .and_return({ "text" => "Oh — I meant both of them.", "span" => nil }.to_json) From 2975da3467c0370818cb8c4491cfc213fba7b8ea Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Thu, 20 Aug 2026 17:16:56 -0500 Subject: [PATCH 2/2] Say when a hotkey save fails, and fit the picker on a phone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things from review. The PATCH was fire-and-forget: a refused or dropped save left the page lit up on a key the server never took, discovered only on the next visit when push-to-talk silently wasn't what the settings said. The click stays the feedback — no spinner narrating a round trip that takes milliseconds — but a failure now puts the previous choice back and says so. And the picker didn't fit a phone. Its no-shrink rule kept the strip at full width, which crushed the explanation into one word per line and ran the last option off the screen. Below 640px the row stacks and the strip takes the width; segments size to content rather than splitting it evenly, because "Ctrl+Space" has nowhere to break and an equal quarter clips it; below about 350px the strip wraps to a second line rather than hiding "Off" off the edge. Fixes the Theme row at those widths too. Co-Authored-By: Claude Opus 5 --- .../assets/stylesheets/coplan/application.css | 50 ++++++++++++++ .../coplan/voice_hotkey_controller.js | 66 +++++++++++++++--- .../coplan/settings/settings/_voice.html.erb | 4 ++ spec/system/voice_hotkey_setting_spec.rb | 69 +++++++++++++++++++ 4 files changed, 178 insertions(+), 11 deletions(-) create mode 100644 spec/system/voice_hotkey_setting_spec.rb diff --git a/engine/app/assets/stylesheets/coplan/application.css b/engine/app/assets/stylesheets/coplan/application.css index a2a94ac8..1d0c2e2a 100644 --- a/engine/app/assets/stylesheets/coplan/application.css +++ b/engine/app/assets/stylesheets/coplan/application.css @@ -2503,6 +2503,10 @@ img.avatar { line-height: 1.5; } +.settings-row__hint--error { + color: var(--color-danger); +} + .settings-row__action { flex: 0 0 auto; display: flex; @@ -2622,6 +2626,52 @@ img.avatar { font-weight: 600; } +/* On a phone there is no width to negotiate over, so the label goes above + the control and the strip takes the whole row. Side by side, the four + push-to-talk options either run off the screen or crush the explanation + into one word per line. + Segments share the leftover width rather than splitting it evenly: + "Ctrl+Space" has no space to break at, so an equal quarter of a 320px + screen clips it, while sized-to-content segments fit with room over. */ +@media (max-width: 640px) { + .settings-row { + flex-direction: column; + align-items: stretch; + gap: var(--space-sm); + } + + .settings-row__action { + align-items: stretch; + } + + .segmented { + display: flex; + width: 100%; + /* Below about 350px even sized-to-content segments run out of room, + so the strip takes a second line rather than hiding the last + option off the edge of the screen. */ + flex-wrap: wrap; + } + + .segmented__option { + flex: 1 1 auto; + } + + /* A wrapped row needs a rule above it, which the horizontal strip + never did. */ + .segmented__btn { + border-top: 1px solid var(--color-border); + margin-top: -1px; + } + + .segmented__btn { + justify-content: center; + width: 100%; + padding: var(--space-sm) var(--space-xs); + font-size: var(--text-xs); + } +} + /* Thread popover data containers (hidden, provide data for JS) */ .thread-popover-data { display: contents; diff --git a/engine/app/javascript/controllers/coplan/voice_hotkey_controller.js b/engine/app/javascript/controllers/coplan/voice_hotkey_controller.js index f8d5dc1e..44a06300 100644 --- a/engine/app/javascript/controllers/coplan/voice_hotkey_controller.js +++ b/engine/app/javascript/controllers/coplan/voice_hotkey_controller.js @@ -6,24 +6,68 @@ import { Controller } from "@hotwired/stimulus" * Saves the push-to-talk key as soon as it's picked — settings here don't * have a save button. Nothing on this page reads the key, so there is no * local state to update; the plan page picks it up on its next load. + * + * The click is the feedback: the segment lights up straight away and no + * spinner narrates a round trip that normally takes a few milliseconds. + * What that owes the reader is honesty when it doesn't land — a settings + * page showing a preference the server never took is a lie that only + * comes out on the next visit, when the key silently isn't what it says. + * So a failure puts the old choice back and says so. */ export default class extends Controller { - static targets = ["macNote"] + static targets = ["macNote", "error"] static values = { url: String } - select(event) { + connect() { + this.saved = this.element.querySelector("input[name='voice_hotkey']:checked")?.value + } + + async select(event) { const hotkey = event.target.value + const previous = this.saved - if (this.hasMacNoteTarget) this.macNoteTarget.hidden = hotkey !== "ctrl_space" + this._reflect(hotkey) + this._setError("") const csrfToken = document.querySelector('meta[name="csrf-token"]')?.content - fetch(this.urlValue, { - method: "PATCH", - headers: { - "Content-Type": "application/x-www-form-urlencoded", - "X-CSRF-Token": csrfToken - }, - body: `voice_hotkey=${hotkey}` - }) + let response = null + try { + response = await fetch(this.urlValue, { + method: "PATCH", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + "X-CSRF-Token": csrfToken + }, + body: `voice_hotkey=${encodeURIComponent(hotkey)}` + }) + } catch { + // Offline or the request never landed — same outcome as a refusal. + } + + if (response?.ok) { + this.saved = hotkey + return + } + + // Put the page back to what the server actually has, so what's lit is + // always the key that will work. + if (previous) { + const restored = this.element.querySelector(`input[name='voice_hotkey'][value='${previous}']`) + if (restored) restored.checked = true + this._reflect(previous) + } + this._setError("Couldn't save that — try again.") + } + + // Bits of the row that follow the choice rather than the server: the + // macOS caveat only applies to Ctrl+Space. + _reflect(hotkey) { + if (this.hasMacNoteTarget) this.macNoteTarget.hidden = hotkey !== "ctrl_space" + } + + _setError(message) { + if (!this.hasErrorTarget) return + this.errorTarget.textContent = message + this.errorTarget.hidden = message === "" } } diff --git a/engine/app/views/coplan/settings/settings/_voice.html.erb b/engine/app/views/coplan/settings/settings/_voice.html.erb index 95ccb664..f81d7f3d 100644 --- a/engine/app/views/coplan/settings/settings/_voice.html.erb +++ b/engine/app/views/coplan/settings/settings/_voice.html.erb @@ -22,6 +22,10 @@ you hold it, turn that off in System Settings or choose another key here.

+ <%# Only ever filled in when a save didn't land — the successful case + is the segment lighting up, which already happened. %> +
<% CoPlan::User::VOICE_HOTKEYS.each do |hotkey| %> diff --git a/spec/system/voice_hotkey_setting_spec.rb b/spec/system/voice_hotkey_setting_spec.rb new file mode 100644 index 00000000..5e24eec0 --- /dev/null +++ b/spec/system/voice_hotkey_setting_spec.rb @@ -0,0 +1,69 @@ +require "rails_helper" + +# Picking a push-to-talk key has no save button: the click is the save. +# That only works if what's lit is what the server has — a settings page +# showing a preference that never persisted is a lie you don't discover +# until the key silently doesn't work. +RSpec.describe "Push-to-talk key setting", type: :system do + let(:user) { create(:coplan_user, email: "picker@example.com") } + + def sign_in(user) + visit sign_in_path + fill_in "Email address", with: user.email + click_button "Sign In" + expect(page).to have_current_path(root_path) + end + + before { sign_in(user) } + + it "saves the choice as it's clicked" do + visit settings_root_path + + find(".segmented__option", text: "Shift").click + + Timeout.timeout(5) { sleep 0.1 until user.reload.voice_hotkey == "shift" } + + # And the page agrees with the server on the next visit. + visit settings_root_path + expect(page).to have_css(".segmented__option:has(input:checked)", text: "Shift") + end + + # The macOS caveat only applies to one of the keys, so it appears the + # moment that key is picked rather than on the next page load. + it "explains the Ctrl+Space caveat as soon as it's picked" do + user.update!(voice_hotkey: "shift") + visit settings_root_path + expect(page).to have_no_content(/switch input sources/i) + + find(".segmented__option", text: "Ctrl+Space").click + + expect(page).to have_content(/switch input sources/i) + end + + # When the save doesn't land, the lit segment goes back to the key that + # will actually work, and says why. + context "when the server refuses the save" do + before do + page.driver.browser.execute_cdp("Page.addScriptToEvaluateOnNewDocument", source: <<~JS) + const realFetch = window.fetch + window.fetch = (url, options) => + String(url).includes("voice_hotkey") + ? Promise.resolve(new Response("", { status: 500 })) + : realFetch(url, options) + JS + end + + it "puts the previous key back and says it didn't save" do + user.update!(voice_hotkey: "shift") + visit settings_root_path + + find(".segmented__option", text: "Ctrl+Space").click + + expect(page).to have_content("Couldn't save that") + expect(page).to have_css(".segmented__option:has(input:checked)", text: "Shift") + # The caveat belongs to a key that isn't in force, so it goes too. + expect(page).to have_no_content(/switch input sources/i) + expect(user.reload.voice_hotkey).to eq("shift") + end + end +end