feat(icon): add colorful option for icon rendering#217
feat(icon): add colorful option for icon rendering#217TalexDreamSoul merged 4 commits intomasterfrom
Conversation
Introduces a new `colorful` boolean property to the `ITuffIcon` type and `TuffIcon` component. This allows for explicit control over whether URL-based icons, particularly SVGs, should be rendered in their original colors or as monochrome template icons. The URL provider for browser actions has been updated to set `colorful: false`, ensuring browser icons are displayed monochromatically for a more consistent UI.
This commit refactors the `useClipboard` hook to enhance code quality and readability. - Replaced magic numbers related to autofill logic with named constants (e.g., `AUTOFILL_SHORT_TEXT_LIMIT`, `AUTOFILL_TIMESTAMP_TTL`). - Simplified conditional logic and function implementations for better conciseness. - Revised comments to be more direct and removed verbose JSDoc blocks, improving overall code clarity. - Increased the character limit for short text autofill from 25 to 80.
WalkthroughAdded an optional Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts (3)
46-74: UsenormalizeTimestampincanAutoPasteto avoid runtime errors on invalid timestamps.Right now
canAutoPastedoes:
const copiedTime = new Date(clipboardOptions.last.timestamp).getTime()- Logs
new Date(copiedTime).toISOString()If
clipboardOptions.last.timestampis malformed,copiedTimebecomesNaN, andnew Date(NaN).toISOString()will throw aRangeError, breaking auto‑paste.You already have
normalizeTimestamp; reusing it here makes this path robust and consistent withhandlePaste.function canAutoPaste(): boolean { - if (!clipboardOptions.last || !clipboardOptions.last.timestamp) return false - if (!appSetting.tools.autoPaste.enable) return false + if (!clipboardOptions.last || !clipboardOptions.last.timestamp) return false + if (!appSetting.tools.autoPaste.enable) return false @@ - const limit = appSetting.tools.autoPaste.time - if (limit === -1) return false - if (limit === 0) return true - - // Use database timestamp (real copy time) instead of detectedAt - const copiedTime = new Date(clipboardOptions.last.timestamp).getTime() - const now = Date.now() - const elapsed = now - copiedTime + const limit = appSetting.tools.autoPaste.time + if (limit === -1) return false + if (limit === 0) return true + + // Use database timestamp (real copy time) instead of detectedAt + const copiedTime = normalizeTimestamp(clipboardOptions.last.timestamp) + if (copiedTime === null) { + console.error('[Clipboard] Invalid clipboard timestamp, disabling auto-paste for this item') + return false + } + + const now = Date.now() + const elapsed = now - copiedTime @@ - copiedAt: new Date(copiedTime).toISOString(), + copiedAt: new Date(copiedTime).toISOString(), now: new Date(now).toISOString(), elapsed, limit: limit * 1000, canPaste: elapsed <= limit * 1000 })You may also want to audit any other direct
new Date(...).toISOString()usage for the same issue; herenormalizeTimestampalready covers it.
236-258:clearClipboarddismissal mirroring intoautoPastedTimestampsis consistent; just tweak brace style.The logic to:
- Set
lastClearedTimestampwhenrememberis true, and- Mirror the normalized timestamp into
autoPastedTimestamps,nicely aligns manual dismissals with auto‑paste suppression. The
else if (!remember)branch correctly resetslastClearedTimestampwhen we’re not “remembering” a dismissal.Only style issues remain:
- Line 248
} else if (!remember) {is flagged bybrace-style(prefer}\nelse if).- This function otherwise looks ready.
260-285: System clipboard event handler and newresetAutoPasteStateexport look good; address minor lint.
- The early
if (!data?.type) returnis a good guard against malformed events.- Normalizing
incomingTimestampandlastClearedTimestampreuses the shared timestamp logic.- Resetting
lastClearedTimestampanddetectedAton each new event, and exposingresetAutoPasteStatefrom the hook, aligns with the new auto‑paste state model.Remaining lints:
- Line 262:
if (!data?.type) returnhitsantfu/if-newline.console.debugon Line 277 is anotherno-consoleviolation; consider the same mitigation strategy as inhandleAutoFill.Also applies to: 287-293
🧹 Nitpick comments (2)
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts (1)
76-122: Auto‑fill behavior is clear; double‑check cleanup probability and addressno-consolelints.
- The guard clauses (
!clipboardOptions.last,!canAutoPaste(),autoPastedTimestamps.has(timestamp)) make the auto‑fill path easy to follow and avoid duplicate UI updates.- File handling and short/long text handling look consistent with the new options; using
AUTOFILL_SHORT_TEXT_LIMITandAUTOFILL_LOG_PREVIEW_LENGTHis a good improvement.Two follow‑ups:
Cleanup probability
AUTOFILL_CLEANUP_PROBABILITY = 0.1means a 10% chance per call:if (Math.random() < AUTOFILL_CLEANUP_PROBABILITY) { cleanupAutoPastedRecords() }If your original intent was a rare 0.1% cleanup, this constant should be
0.001instead. Please confirm the desired frequency.
no-consoleESLint violations
Theconsole.debugcalls here (Lines 85, 114, 135, 148) are flagged byno-console(onlywarn/errorallowed). Either:
- Swap them to a project logging helper (preferred), or
- Change them to
console.warn/console.errorwhere appropriate, or- Gate them with an environment check and disable the rule locally.
Also applies to: 124-159
apps/core-app/src/renderer/src/components/base/TuffIcon.vue (1)
93-104:isSvg && colorfulgate changes behavior for existing callers; verify they’re updated or okay with<img>fallback.The new branch:
<template v-else-if="addressable"> <template v-if="isSvg && colorful"> <i class="TuffIcon-Svg colorful" :alt="alt" :style="{ '--un-icon': `url('${dataurl}')` }" /> </template> <template v-else> <img :alt="alt" :src="url" /> </template> </template>means:
- Only icons with both
isSvg === trueandcolorful === trueuse the SVG mask path.- All other addressable icons (including SVGs with
colorfulmissing/false) now render as plain<img>.This is correct for the new URL icons where you set
colorful: false, but it changes behavior for any existing TuffIcon usages that previously relied on SVG masking without passingcolorful. Those will now lose the mask/theming.I recommend quickly scanning other
<TuffIcon>call sites and either:
- Pass
:colorful="true"where you still want the masked SVG behavior, or- Confirm that the
<img>fallback is acceptable there.(Optionally, you could also include
colorfulin thewatchdependencies if you ever want to avoid fetching SVG content whencolorfulis always false.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/core-app/src/main/modules/box-tool/addon/url/url-provider.ts(1 hunks)apps/core-app/src/renderer/src/components/base/TuffIcon.vue(1 hunks)apps/core-app/src/renderer/src/components/render/BoxItem.vue(2 hunks)apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts(11 hunks)packages/utils/types/icon.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts (4)
apps/core-app/src/renderer/src/modules/box/adapter/hooks/index.ts (3)
useClipboard(3-3)IClipboardOptions(1-1)IClipboardHook(1-1)apps/core-app/src/renderer/src/modules/box/adapter/types.ts (1)
IBoxOptions(14-24)apps/core-app/src/renderer/src/modules/box/adapter/hooks/types.ts (2)
IClipboardOptions(27-34)IClipboardHook(39-47)apps/core-app/src/renderer/src/modules/channel/storage/index.ts (1)
appSetting(95-95)
🪛 ESLint
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts
[error] 14-14: Expect newline after if
(antfu/if-newline)
[error] 53-53: Expect newline after if
(antfu/if-newline)
[error] 54-54: Expect newline after if
(antfu/if-newline)
[error] 57-57: Expect newline after if
(antfu/if-newline)
[error] 58-58: Expect newline after if
(antfu/if-newline)
[error] 78-78: Expect newline after if
(antfu/if-newline)
[error] 79-79: Expect newline after if
(antfu/if-newline)
[error] 85-85: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
[error] 114-114: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
[error] 118-118: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 135-135: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
[error] 148-148: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
[error] 192-192: '=' should be placed at the beginning of the line.
(style/operator-linebreak)
[error] 222-222: Expect newline after if
(antfu/if-newline)
[error] 230-230: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 248-248: Closing curly brace appears on the same line as the subsequent block.
(style/brace-style)
[error] 262-262: Expect newline after if
(antfu/if-newline)
🔇 Additional comments (4)
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts (1)
172-218:handlePastetimestamp handling and dismissal logic look correct.Using
normalizeTimestampforclipboard.timestamp,currentTimestamp, andlastClearedTimestampmakes the comparisons safe. TheisSameClipboard/isDismissed/shouldRefreshflags correctly avoid re‑showing dismissed content while still allowing overrides viaoverrideDismissed, and you always fall through tohandleAutoFill()when appropriate.If you haven’t already, it’s worth adding a small test or manual check for the case where
clipboard.timestampis an invalid string to ensure theclearClipboard()early‑return path behaves as expected.apps/core-app/src/main/modules/box-tool/addon/url/url-provider.ts (1)
47-52: Explicitcolorful: falsefor browser URL icons aligns with new TuffIcon behavior.Using:
.setIcon({ type: 'url', value: browser.icon, colorful: false })means these browser icons will bypass the SVG mask path (
isSvg && colorful) and render as plain<img>, preserving their original logo appearance. This fits the newcolorfulflag semantics and the BoxItem binding.If there are other URL icons that should also be de‑colorized, make sure they set
colorful: falseconsistently so their rendering matches this behavior.apps/core-app/src/renderer/src/components/render/BoxItem.vue (1)
118-126: Dynamiccolorfulbinding forTuffIconis reasonable and backwards‑friendly.
:colorful="render?.basic?.icon?.colorful ?? true"ensures:
- Existing icons without a
colorfulflag still behave as “colorful” (true).- Callers like the URL provider can explicitly set
colorful: falseto switch to the non‑colorful path.This matches the new TuffIcon logic that checks
isSvg && colorful.It’s worth sanity‑checking that any non‑URL icons that rely on special coloring also set
render.basic.icon.colorfulas needed, since this binding looks directly at the render metadata rather than thedisplayIconcomputed.packages/utils/types/icon.ts (1)
40-46:ITuffIcon.colorfulextension is clean and backwards‑compatible.Adding an optional
colorful?: booleanwith clear docs keeps existing icons valid while enabling the new rendering mode for URL icons. No issues from a type/interface perspective.
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts
Show resolved
Hide resolved
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts
Outdated
Show resolved
Hide resolved
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts
Show resolved
Hide resolved
Elevates the log level for clipboard operations from `debug` to `warn` to increase their visibility during development and aid in debugging auto-paste behavior. Additionally, this commit improves code formatting by consistently using block statements for conditionals and try-catch blocks, enhancing readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts (1)
210-211: Fix operator linebreak style.The assignment operator should be placed at the beginning of the continuation line per the project's ESLint style guide.
Apply this diff:
- const isDismissed = - !overrideDismissed && dismissedTimestamp !== null && dismissedTimestamp === clipboardTimestamp + const isDismissed + = !overrideDismissed && dismissedTimestamp !== null && dismissedTimestamp === clipboardTimestamp
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts (4)
apps/core-app/src/renderer/src/modules/box/adapter/types.ts (1)
IBoxOptions(14-24)apps/core-app/src/renderer/src/modules/box/adapter/hooks/types.ts (3)
IClipboardOptions(27-34)IClipboardHook(39-47)IClipboardItem(11-22)apps/core-app/src/renderer/src/modules/channel/storage/index.ts (1)
appSetting(95-95)apps/core-app/src/main/modules/clipboard.ts (1)
IClipboardItem(63-74)
🪛 ESLint
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts
[error] 210-210: '=' should be placed at the beginning of the line.
(style/operator-linebreak)
🔇 Additional comments (9)
apps/core-app/src/renderer/src/modules/box/adapter/hooks/useClipboard.ts (9)
7-10: LGTM! Constants improve maintainability.The extracted constants eliminate magic numbers and make the autofill behavior more configurable and maintainable.
12-29: LGTM! Robust timestamp normalization.The function correctly handles multiple timestamp formats (number, Date, string) with proper validation, making the autofill logic more resilient to varied input types.
31-47: LGTM! Clean TTL-based cache management.The autofill cache with TTL-based cleanup and the exposed
resetAutoPasteStatehook provide a solid foundation for managing auto-paste state across CoreBox sessions.
55-87: LGTM! Comprehensive auto-paste eligibility logic.The function correctly handles all autofill modes (-1, 0, positive seconds) and uses the database timestamp for accurate time-based filtering.
89-188: LGTM! Well-structured autofill logic with proper guards.The function correctly handles files, short text (≤80 chars to input), long text (callback only), and images with appropriate guard clauses and duplicate prevention. The probabilistic cleanup (10% chance) provides amortized performance without overhead on every call.
238-255: LGTM! Robust IPC call with error handling.The function correctly applies the clipboard item to the active application with proper try/catch error handling and boolean success indication.
257-280: LGTM! Clear dismissal logic with dual tracking.The function correctly clears clipboard UI state and, when
rememberis true, mirrors the dismissal to bothlastClearedTimestampandautoPastedTimestampsto prevent the item from ever auto-loading again.
282-309: LGTM! Proper system clipboard event handling.The listener correctly guards against invalid items, respects dismissed state, and updates the clipboard options with appropriate logging and callbacks.
311-317: LGTM! New public API member correctly exposed.The
resetAutoPasteStatefunction is now properly exposed as a public hook member, enabling explicit reset behavior on CoreBox mount as documented in the types file.
1 participant
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.