feat: add long press to translate #859
Conversation
🦋 Changeset detectedLatest commit: c286101 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dosubot
bot
added
the
size:L
|
Related Documentation Checked 7 published document(s) in 0 knowledge base(s). No updates required. How did I do? Any feedback? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/locales/tr.yml">
<violation number="1" location="src/locales/tr.yml:20">
P3: Inconsistent capitalization. Similar entries like `hover: Üzerine Gel` capitalize the second word. Consider `Uzun Basma` for consistency with the established pattern in this locale file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
src/locales/tr.yml
Outdated
| alwaysTranslate: Bu siteyi her zaman çevir | ||
| options: Seçenekler | ||
| hover: Üzerine Gel | ||
| longPress: Uzun basma |
There was a problem hiding this comment.
P3: Inconsistent capitalization. Similar entries like hover: Üzerine Gel capitalize the second word. Consider Uzun Basma for consistency with the established pattern in this locale file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/locales/tr.yml, line 20:
<comment>Inconsistent capitalization. Similar entries like `hover: Üzerine Gel` capitalize the second word. Consider `Uzun Basma` for consistency with the established pattern in this locale file.</comment>
<file context>
@@ -17,6 +17,7 @@ popup:
alwaysTranslate: Bu siteyi her zaman çevir
options: Seçenekler
hover: Üzerine Gel
+ longPress: Uzun basma
translateParagraph: paragrafı çevirmek için
discord:
</file context>
|
@claude review this PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@claude review this PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
mengxi-ream
left a comment
mengxi-ream
left a comment
There was a problem hiding this comment.
PR Review: feat: add long press to translate
Overview
This PR adds a "Long Press" option as an alternative trigger for paragraph translation. Instead of requiring a modifier key + hover, users can now long-press (1 second) on text to translate it. The feature includes:
- New
LongPresshotkey option - Mouse event handlers (mousedown, mouseup, mousemove)
- Movement tolerance (6px) to cancel accidental triggers
- Translations for 8 locales
Taste Score
🟢 Good taste - Clean implementation following existing patterns
Strengths
- Follows existing patterns: Uses the same hotkey infrastructure and config system
- Good UX considerations: 6px movement tolerance prevents accidental triggers during normal clicking
- Proper guards: Checks for editable elements, button type, and enabled state
- Complete i18n: All 8 locale files updated consistently
Issues & Suggestions
🔴 Potential Bug: Event listener on document vs document.body
- document.body.addEventListener('mousemove', ...)
+ document.addEventListener('mousemove', ...)The original mousemove was on document.body, now it's on document. This could cause behavior differences on pages where body doesn't cover the viewport. Consider keeping document.body for the position tracking or verify this change is intentional.
🟡 Minor: Consider extracting shared position update
The mousePosition update in mousedown duplicates the one in mousemove. This seems intentional for the timing, but worth noting.
🟡 Missing test coverage
The PR mentions only manual testing. Consider adding unit tests for:
- Long press timer triggering at 1000ms
- Movement tolerance cancellation at 6px
- Editable element exclusion
- Button type filtering (only left click)
🟢 Good: Async config fetch inside timer
The getLocalConfig() call inside the timeout is correct - it fetches fresh config at trigger time, not stale config from mousedown.
Nitpicks
- Magic number:
LONG_PRESS_TRIGGER_MS = 1000- Consider if this should be user-configurable in the future
Security & Performance
✅ No security concerns - purely UI interaction logic
✅ Lightweight - only adds event listeners, no expensive computations
Summary
Recommendation: Approve with minor suggestions
The implementation is clean and follows project conventions. The main suggestion is to verify the document vs document.body change for mousemove events is intentional. Adding unit tests would strengthen the PR but isn't blocking.
dosubot
bot
added
size:XL
There was a problem hiding this comment.
1 issue found across 17 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/utils/config/migration-scripts/v041-to-v042.ts">
<violation number="1" location="src/utils/config/migration-scripts/v041-to-v042.ts:21">
P2: Migration fallback may silently reset valid hotkey values to 'control'. If `oldHotkey` is not in the migration map (e.g., already lowercase like `'alt'` or an unmapped key), the user's setting will be lost. Consider preserving unknown values: `HOTKEY_MIGRATION[oldHotkey] ?? oldHotkey ?? 'control'`</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| export function migrate(oldConfig: any): any { | ||
| const oldHotkey = oldConfig.translate?.node?.hotkey | ||
| const newHotkey = oldHotkey ? (HOTKEY_MIGRATION[oldHotkey] ?? 'control') : 'control' |
There was a problem hiding this comment.
P2: Migration fallback may silently reset valid hotkey values to 'control'. If oldHotkey is not in the migration map (e.g., already lowercase like 'alt' or an unmapped key), the user's setting will be lost. Consider preserving unknown values: HOTKEY_MIGRATION[oldHotkey] ?? oldHotkey ?? 'control'
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/config/migration-scripts/v041-to-v042.ts, line 21:
<comment>Migration fallback may silently reset valid hotkey values to 'control'. If `oldHotkey` is not in the migration map (e.g., already lowercase like `'alt'` or an unmapped key), the user's setting will be lost. Consider preserving unknown values: `HOTKEY_MIGRATION[oldHotkey] ?? oldHotkey ?? 'control'`</comment>
<file context>
@@ -0,0 +1,33 @@
+
+export function migrate(oldConfig: any): any {
+ const oldHotkey = oldConfig.translate?.node?.hotkey
+ const newHotkey = oldHotkey ? (HOTKEY_MIGRATION[oldHotkey] ?? 'control') : 'control'
+
+ return {
</file context>
3 participants
Type of Changes
Description
add long press to translate
Related Issue
Closes #671
How Has This Been Tested?
Screenshots
https://cap.so/s/21xn880saryr37j
Checklist
Additional Information