ENG-775 - Add keyboard shortcut for base discourse tool #378
Conversation
… Tool - Added a new setting for the Discourse Tool keyboard shortcut in HomePersonalSettings. - Updated uiOverrides to utilize the custom keyboard shortcut for the Discourse Tool. - Improved warning message formatting in Tldraw component for better readability. - Introduced DISCOURSE_TOOL_SHORTCUT_KEY in userSettings for better configuration management.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
…ssibility - Replaced the div wrapper with Label to enhance semantic structure. - Maintained existing functionality while improving the component's accessibility and styling consistency.
mdroidian
changed the title
📝 WalkthroughWalkthroughAdds a user-configurable keyboard shortcut for the Discourse Tool: introduces a settings input component, persists the shortcut in extension settings, converts between Blueprint and tldraw formats, and displays the shortcut in the tldraw UI. Also adds the corresponding settings key and applies minor console warning formatting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SettingsUI as Settings UI (HomePersonalSettings)
participant KSInput as KeyboardShortcutInput
participant ExtSettings as Extension Settings Store
participant Utils as keyboardShortcutUtils
User->>SettingsUI: Open Personal Settings
SettingsUI->>KSInput: Render with DISCOURSE_TOOL_SHORTCUT_KEY
KSInput->>ExtSettings: getSetting(key)
ExtSettings-->>KSInput: IKeyCombo or empty
User->>KSInput: Press key(s) to set shortcut
KSInput->>KSInput: Normalize/validate (single key for discourse tool)
KSInput->>ExtSettings: setSetting(key, IKeyCombo)
note over KSInput,ExtSettings: Clear button resets to empty combo
sequenceDiagram
autonumber
participant Canvas as tldraw Canvas
participant UIOverrides as uiOverrides
participant ExtSettings as Extension Settings Store
participant Utils as keyboardShortcutUtils
Canvas->>UIOverrides: createUiOverrides()
UIOverrides->>ExtSettings: getSetting(DISCOURSE_TOOL_SHORTCUT_KEY)
ExtSettings-->>UIOverrides: IKeyCombo
UIOverrides->>Utils: convertComboToTldrawFormat(combo)
Utils-->>UIOverrides: tldraw shortcut string
UIOverrides->>Canvas: Set discourse-tool kbd label
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit full review |
|
@mdroidian I'll perform a full review of all the changes in this PR. ✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
apps/roam/src/utils/keyboardShortcutUtils.ts (1)
27-56: Consider adding input validation for the reverse conversion function.The function handles empty strings correctly but doesn't validate the input format. Consider adding validation to ensure the input follows the expected tldraw format pattern.
export const convertTldrawFormatToCombo = (shortcut: string): IKeyCombo => { if (!shortcut) return { modifiers: 0, key: "" }; + // Basic validation - tldraw shortcuts should not contain invalid characters + if (!/^[?$^@]*[A-Za-z0-9]?$/.test(shortcut)) { + console.warn(`Invalid tldraw shortcut format: ${shortcut}`); + return { modifiers: 0, key: "" }; + } let modifiers = 0; let key = shortcut;apps/roam/src/components/canvas/uiOverrides.tsx (2)
334-341: Consider using the imported utility function for consistency.While the current implementation works correctly for single keys, using the imported
convertComboToTldrawFormatfunction would be more consistent and handle edge cases better.- // For discourse tool, just use the key directly since we don't allow modifiers - const discourseToolShortcut = discourseToolCombo?.key?.toUpperCase() || ""; + // Convert the combo to tldraw format (handles edge cases and is consistent with the utility) + const discourseToolShortcut = convertComboToTldrawFormat(discourseToolCombo);
333-347: Verified: Discourse Tool Shortcut Logic is Correct
- The custom logic for the discourse tool correctly ignores any modifier keys, only captures single-character keys, and uppercases them for Tldraw’s UI.
- Falling back to an empty string when no key is set is appropriate.
- The
convertComboToTldrawFormatimport isn’t used in this code path; you may remove it to eliminate dead code (or retain it if you plan to extend support to special keys in the future).apps/roam/src/components/settings/KeyboardShortcutInput.tsx (4)
95-103: Avoid accepting Space/whitespace as the Discourse Tool keySpace is a single character and will be accepted, but it often conflicts with canvas panning and other global shortcuts. Filter out whitespace-only keys.
Apply this diff:
// Only allow single character keys if (e.key.length === 1) { + // Disallow whitespace (e.g., Space) to avoid conflicts with canvas panning + if (e.key.trim().length === 0) return; const comboObj = { key: e.key.toLowerCase(), modifiers: 0 }; setComboKey(comboObj); extensionAPI.settings .set(settingKey, comboObj) .catch(() => console.error("Failed to set setting")); }If Product explicitly wants to allow Space, please confirm and we’ll drop this filter.
118-125: Minor UX: uppercase printable key in the display stringBlueprint’s key combo UI typically renders printable keys uppercase. Uppercasing single-character keys improves readability.
Apply this diff:
const shortcut = useMemo(() => { if (!comboKey.key) return ""; const modifiers = getModifiersFromCombo(comboKey); - const comboString = [...modifiers, comboKey.key].join("+"); + const printable = comboKey.key.length === 1 ? comboKey.key.toUpperCase() : comboKey.key; + const comboString = [...modifiers, printable].join("+"); return normalizeKeyCombo(comboString).join("+"); }, [comboKey]);
145-151: Add an accessible name to the clear buttonProvide an aria-label/title so screen readers announce the action.
Apply this diff:
rightElement={ <Button hidden={!comboKey.key} icon="remove" onClick={handleClear} minimal + aria-label="Clear shortcut" + title="Clear shortcut" /> }I can add tests for basic a11y with react-aria or testing-library if you’d like.
28-33: Avoid duplicating Blueprint’s hotkey constants; import from the libraryYou’re maintaining local copies of ModifierBitMasks and Aliases, which Blueprint already exports. Importing them lowers drift risk when upgrading Blueprint. You can still post-process to render “cmd”/“win” labels.
For example:
- Import:
import { ModifierBitMasks, Aliases, normalizeKeyCombo as bpNormalizeKeyCombo } from "@blueprintjs/core";- Remove local MODIFIER_BIT_MASKS and ALIASES, and adapt
normalizeKeyComboto wrapbpNormalizeKeyComboand then map"meta"to"cmd"/"win".References: Blueprint exports these symbols and uses the same bit masks (alt:1, ctrl:2, meta:4, shift:8). (app.unpkg.com, unpkg.com)
Also applies to: 35-45, 47-53, 55-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/roam/src/components/canvas/Tldraw.tsx(1 hunks)apps/roam/src/components/canvas/uiOverrides.tsx(3 hunks)apps/roam/src/components/settings/HomePersonalSettings.tsx(2 hunks)apps/roam/src/components/settings/KeyboardShortcutInput.tsx(1 hunks)apps/roam/src/data/userSettings.ts(1 hunks)apps/roam/src/utils/keyboardShortcutUtils.ts(1 hunks)
🔇 Additional comments (7)
apps/roam/src/data/userSettings.ts (1)
7-7: LGTM!The new constant follows the established naming pattern and provides a clear, descriptive key for the Discourse Tool keyboard shortcut setting.
apps/roam/src/utils/keyboardShortcutUtils.ts (1)
1-25: LGTM!The conversion from Blueprint
IKeyComboto tldraw format is well-implemented with proper null checks and clear documentation of the format mapping.apps/roam/src/components/canvas/Tldraw.tsx (1)
148-150: LGTM!The multi-line formatting improves readability without changing the warning message content or behavior.
apps/roam/src/components/settings/HomePersonalSettings.tsx (2)
16-20: LGTM!The imports are correctly added and the new constant is properly imported from the userSettings module.
46-52: LGTM!The
KeyboardShortcutInputcomponent is properly integrated with clear props. The description accurately conveys the single-key limitation and the component placement is logical within the settings flow.apps/roam/src/components/canvas/uiOverrides.tsx (1)
35-51: LGTM!The imports are correctly added and support the new keyboard shortcut functionality.
apps/roam/src/components/settings/KeyboardShortcutInput.tsx (1)
88-104: Good constraint: single-key, no-modifier capture for the Discourse ToolClear separation of the Discourse Tool path from general shortcuts keeps UX simple and avoids accidental multi-modifier combos. With the whitespace filter above, this looks solid.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
mdroidian
deleted the
eng-775-allow-keyboard-shortcut-for-base-discourse-tool
branch
Summary by CodeRabbit