Add multi-language Git cheatsheet with copy functionality#22
Add multi-language Git cheatsheet with copy functionality#22TanNhatCMS wants to merge 6 commits intomasterfrom
Conversation
…opy functionality
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe GitCheatsheet component was refactored to render locale-specific Markdown (English/Vietnamese) with a markdown-it + shiki pipeline and per-line copy-to-clipboard controls; the old single Markdown file was removed and replaced by language files in a Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as "Cheatsheet UI"
participant Component as "GitCheatsheet\n(index.vue)"
participant Renderer as "markdown-it + shiki"
participant Clipboard as "Clipboard API"
User->>UI: View page / click locale
UI->>Component: request memo for locale
Component->>Renderer: render markdown -> html + highlighted code
Renderer-->>Component: rendered DOM
Component->>UI: inject rendered content
User->>UI: Click per-line copy button
UI->>Component: handle copy event
Component->>Clipboard: writeText(code line)
Clipboard-->>Component: success/failure
Component->>UI: update button icon/text (copied state), schedule reset
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Git cheatsheet tool by introducing multi-language support and a convenient copy-to-clipboard feature for code snippets. The content itself has also been expanded to cover a broader range of Git commands and best practices, making the tool more comprehensive and user-friendly for a diverse audience. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-language Git cheatsheet with copy-to-clipboard functionality. While the markdown-it extension is well-implemented, there are critical security and maintainability issues. The use of html: true in the markdown renderer and inline onclick handlers creates XSS and CSP vulnerabilities. Additionally, a potential memory leak exists in src/render/components/Tools/GitCheatsheet/index.vue due to an uncleaned function attached to the window object. It is strongly recommended to adopt a safer markdown configuration, use standard event handling mechanisms, and ensure proper cleanup of global resources to prevent security risks and memory leaks. Event delegation is suggested for future event handling improvements.
|
|
||
| const i18n = AppI18n() | ||
| const md = markdownit({ | ||
| html: true, |
There was a problem hiding this comment.
The markdown-it instance is configured with html: true, which allows raw HTML within the markdown content to be rendered. This is a security risk as it can lead to Cross-Site Scripting (XSS) if the markdown content is ever influenced by untrusted sources. Furthermore, it causes rendering bugs where text that resembles HTML tags (e.g., <type> in the commit message format description) is treated as a tag and hidden from the user. It is recommended to set html: false unless raw HTML support is strictly necessary and the input is fully sanitized.
|
|
||
| return ` | ||
| <div class="code-block-wrapper"> | ||
| <button class="copy-button" data-code="${encodeURIComponent(code)}" onclick="window.gitCheatsheetCopy(this)"> |
There was a problem hiding this comment.
The copy functionality is implemented by injecting a button with an onclick inline event handler into the rendered HTML. This approach bypasses Vue's security model and would be blocked by a restrictive Content Security Policy (CSP) that disallows inline scripts (unsafe-inline). A more secure and robust approach would be to use event delegation on the container element or a custom markdown-it plugin that integrates with Vue's event system.
| // Expose copy function to window for the onclick handler | ||
| if (typeof window !== 'undefined') { | ||
| ;(window as any).gitCheatsheetCopy = async (btn: HTMLButtonElement) => { | ||
| const code = decodeURIComponent(btn.getAttribute('data-code') || '') | ||
| try { | ||
| await navigator.clipboard.writeText(code) | ||
| const span = btn.querySelector('span') | ||
| const icon = btn.querySelector('i') | ||
| if (span && icon) { | ||
| span.innerText = I18nT('base.copySuccess') | ||
| icon.className = 'bi bi-check2' | ||
| btn.classList.add('copied') | ||
| setTimeout(() => { | ||
| span.innerText = I18nT('base.copy') | ||
| icon.className = 'bi bi-clipboard' | ||
| btn.classList.remove('copied') | ||
| }, 2000) | ||
| } | ||
| } catch (err) { | ||
| console.error('Failed to copy: ', err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Attaching a function to the global window object can lead to memory leaks if not cleaned up when the component is destroyed. The function will remain in memory even after navigating away from this view.
To prevent this, you should remove the function from the window object when the component is unmounted. You can use Vue's onUnmounted lifecycle hook for this.
Please also add onUnmounted to your vue import on line 18:
import { computed, onUnmounted } from 'vue' // Expose copy function to window for the onclick handler
const gitCheatsheetCopyFn = async (btn: HTMLButtonElement) => {
const code = decodeURIComponent(btn.getAttribute('data-code') || '')
try {
await navigator.clipboard.writeText(code)
const span = btn.querySelector('span')
const icon = btn.querySelector('i')
if (span && icon) {
span.innerText = I18nT('base.copySuccess')
icon.className = 'bi bi-check2'
btn.classList.add('copied')
setTimeout(() => {
span.innerText = I18nT('base.copy')
icon.className = 'bi bi-clipboard'
btn.classList.remove('copied')
}, 2000)
}
} catch (err) {
console.error('Failed to copy: ', err)
}
}
if (typeof window !== 'undefined') {
;(window as any).gitCheatsheetCopy = gitCheatsheetCopyFn
}
onUnmounted(() => {
if (typeof window !== 'undefined' && (window as any).gitCheatsheetCopy === gitCheatsheetCopyFn) {
delete (window as any).gitCheatsheetCopy
}
})
![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 (1)
src/render/components/Tools/GitCheatsheet/index.vue (1)
45-66: Avoid inlineonclickand globalwindowcopy handler.Line 45 (inline handler) + Line 66 (global mutation) makes the behavior harder to test and less CSP-friendly. Prefer delegated click handling inside the component.
♻️ Suggested refactor
- <article class="select-text prose prose-slate dark:prose-invert" v-html="result"></article> + <article class="select-text prose prose-slate dark:prose-invert" v-html="result" `@click`="onArticleClick"></article> @@ - <button class="copy-button" data-code="${encodeURIComponent(code)}" onclick="window.gitCheatsheetCopy(this)"> + <button type="button" class="copy-button" data-code="${encodeURIComponent(code)}"> @@ - // Expose copy function to window for the onclick handler - if (typeof window !== 'undefined') { - ;(window as any).gitCheatsheetCopy = async (btn: HTMLButtonElement) => { + const copyCode = async (btn: HTMLButtonElement) => { const code = decodeURIComponent(btn.getAttribute('data-code') || '') try { await navigator.clipboard.writeText(code) @@ } catch (err) { console.error('Failed to copy: ', err) } - } - } + } + + const onArticleClick = (e: MouseEvent) => { + const btn = (e.target as HTMLElement).closest<HTMLButtonElement>('.copy-button') + if (btn) void copyCode(btn) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/components/Tools/GitCheatsheet/index.vue` around lines 45 - 66, Remove the inline onclick and global mutation of window.gitCheatsheetCopy: implement the copy logic as a local function inside the component (e.g., a setup function named gitCheatsheetCopy) or attach a delegated click listener to the component root (targeting elements with class "copy-button" and reading their data-code attribute), then update the rendered button to rely on Vue's event handling (or the internal delegated listener) instead of window mutation; ensure references to md.render, memo/result remain unchanged and use the local gitCheatsheetCopy handler so no global window properties are created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/render/components/Tools/GitCheatsheet/index.vue`:
- Around line 94-133: The .copy-button is only made visible on hover which hides
it from keyboard users; update the stylesheet around the .copy-button block (the
existing &:hover { .copy-button { ... } } and the .copy-button rules) to also
set visibility and a clear focus ring when focused—add :focus-visible and
:focus-within selectors for .copy-button that set opacity: 1, a visible outline
or box-shadow (accessible contrast), and keep the same hover background/border;
ensure the actual interactive element rendered by the component receives
keyboard focus (e.g., button or tabindex) so these focus styles apply.
In `@src/render/components/Tools/GitCheatsheet/lang/git-memo.en.md`:
- Around line 78-81: Replace the deprecated stash command usage `git stash save
"message"` with the current syntax `git stash push -m "message"` in the English
memo (update the line containing `git stash save "message"`) and make the same
replacement in the Vietnamese memo file that contains `git-memo.vi.md` to keep
both translations consistent.
---
Nitpick comments:
In `@src/render/components/Tools/GitCheatsheet/index.vue`:
- Around line 45-66: Remove the inline onclick and global mutation of
window.gitCheatsheetCopy: implement the copy logic as a local function inside
the component (e.g., a setup function named gitCheatsheetCopy) or attach a
delegated click listener to the component root (targeting elements with class
"copy-button" and reading their data-code attribute), then update the rendered
button to rely on Vue's event handling (or the internal delegated listener)
instead of window mutation; ensure references to md.render, memo/result remain
unchanged and use the local gitCheatsheetCopy handler so no global window
properties are created.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/render/components/Tools/GitCheatsheet/git-memo.content.mdsrc/render/components/Tools/GitCheatsheet/index.vuesrc/render/components/Tools/GitCheatsheet/lang/git-memo.en.mdsrc/render/components/Tools/GitCheatsheet/lang/git-memo.vi.md
💤 Files with no reviewable changes (1)
- src/render/components/Tools/GitCheatsheet/git-memo.content.md
| &:hover { | ||
| .copy-button { | ||
| opacity: 1; | ||
| } | ||
| } | ||
|
|
||
| pre { | ||
| margin: 0 !important; | ||
| padding-top: 2.5rem !important; | ||
| } | ||
|
|
||
| .copy-button { | ||
| position: absolute; | ||
| top: 0.5rem; | ||
| right: 0.5rem; | ||
| z-index: 10; | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 0.4rem; | ||
| padding: 0.3rem 0.6rem; | ||
| font-size: 0.75rem; | ||
| color: #94a3b8; | ||
| background: rgba(30, 41, 59, 0.7); | ||
| border: 1px solid rgba(71, 85, 105, 0.5); | ||
| border-radius: 6px; | ||
| cursor: pointer; | ||
| opacity: 0; | ||
| transition: all 0.2s ease-in-out; | ||
| backdrop-filter: blur(4px); | ||
|
|
||
| i { | ||
| font-size: 0.9rem; | ||
| } | ||
|
|
||
| &:hover { | ||
| color: #f1f5f9; | ||
| background: rgba(51, 65, 85, 0.9); | ||
| border-color: #64748b; | ||
| } | ||
|
|
There was a problem hiding this comment.
Copy button needs keyboard-visible focus states.
Lines 94-121 only reveal the control on hover; keyboard users can tab to an invisible button. Add :focus-within / :focus-visible visibility and outline styles.
♿ Suggested CSS fix
.code-block-wrapper {
@@
&:hover {
.copy-button {
opacity: 1;
}
}
+ &:focus-within {
+ .copy-button {
+ opacity: 1;
+ }
+ }
@@
&:hover {
color: `#f1f5f9`;
background: rgba(51, 65, 85, 0.9);
border-color: `#64748b`;
}
+ &:focus-visible {
+ opacity: 1;
+ outline: 2px solid `#60a5fa`;
+ outline-offset: 2px;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/components/Tools/GitCheatsheet/index.vue` around lines 94 - 133,
The .copy-button is only made visible on hover which hides it from keyboard
users; update the stylesheet around the .copy-button block (the existing &:hover
{ .copy-button { ... } } and the .copy-button rules) to also set visibility and
a clear focus ring when focused—add :focus-visible and :focus-within selectors
for .copy-button that set opacity: 1, a visible outline or box-shadow
(accessible contrast), and keep the same hover background/border; ensure the
actual interactive element rendered by the component receives keyboard focus
(e.g., button or tabindex) so these focus styles apply.
| - **Save changes to stash**: | ||
| ```shell | ||
| git stash save "message" | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
git stash save deprecated current documentation
💡 Result:
git stash save is deprecated in current Git documentation; the git stash manpage explicitly says: “This option is deprecated in favour of git stash push.” [1]
Use instead
git stash push -m "message"(preferred; more consistent CLI) [1]git stash(no args) is effectively a shortcut forgit stash push[1]
Why it was deprecated
pushsupports a cleaner interface (e.g.,-mfor the message) and can take pathspecs;savecan’t take pathspecs and treats non-option arguments as the message. [1]
Since when
- Release notes for Git 2.16.0 state: “
git stash savehas been deprecated in favour ofgit stash push.” [2]
Sources
[1] Git manual: git-stash documentation (git-scm.com)
[2] Git v2.16.0 release notes (as published by LWN.net)
🏁 Script executed:
# Find the Vietnamese memo file mentioned in the review
find . -type f -name "*git-memo*" | head -20Repository: TanNhatCMS/FlyEnv
Length of output: 188
🏁 Script executed:
# Check the current English file and surrounding context
head -n 85 src/render/components/Tools/GitCheatsheet/lang/git-memo.en.md | tail -n 10Repository: TanNhatCMS/FlyEnv
Length of output: 229
🏁 Script executed:
# Check the Vietnamese file for the same section
head -n 85 src/render/components/Tools/GitCheatsheet/lang/git-memo.vi.md | tail -n 10Repository: TanNhatCMS/FlyEnv
Length of output: 250
Update to current stash syntax.
Line 80 uses git stash save "message", which has been deprecated since Git 2.16.0. Replace with git stash push -m "message". Apply the same update to the Vietnamese memo (git-memo.vi.md) for consistency.
✏️ Suggested update
- git stash save "message"
+ git stash push -m "message"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/components/Tools/GitCheatsheet/lang/git-memo.en.md` around lines
78 - 81, Replace the deprecated stash command usage `git stash save "message"`
with the current syntax `git stash push -m "message"` in the English memo
(update the line containing `git stash save "message"`) and make the same
replacement in the Vietnamese memo file that contains `git-memo.vi.md` to keep
both translations consistent.
and expose copy function to window.
…ton accessibility
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-language Git cheatsheet with a copy-to-clipboard feature. A critical security concern was identified where enabling raw HTML rendering in the markdown-it configuration could lead to Cross-Site Scripting (XSS) if the content source becomes untrusted. Furthermore, a significant reactivity bug prevents the cheatsheet from updating when the language changes, and there are opportunities to improve the code's scalability and robustness.
| const memo = computed(() => { | ||
| const locale = i18n.global.locale | ||
| if (locale === 'vi') { | ||
| return MemoVi | ||
| } | ||
| return MemoEn | ||
| }) |
There was a problem hiding this comment.
The memo computed property has two issues:
- Reactivity Bug: It depends on
i18n.global.locale, which is not a reactive source in your current setup. This means when the app's language is changed, the cheatsheet content will not update. This is a significant bug for the multi-language feature. You should use a reactive source for the locale. - Scalability: The
if/elsestructure for selecting the language is not easily scalable. If you add more languages, you'll need to modify this logic every time.
I suggest refactoring this to use a map for scalability and ensuring you use a reactive locale.
const memos = {
en: MemoEn,
vi: MemoVi
}
const memo = computed(() => {
// Ensure i18n.global.locale is reactive. For example, if it's a ref, use i18n.global.locale.value
const locale = i18n.global.locale as keyof typeof memos
return memos[locale] ?? MemoEn
})
| const md = markdownit({ | ||
| html: true, | ||
| linkify: true, | ||
| typographer: true | ||
| }) |
There was a problem hiding this comment.
The markdown-it instance is configured with html: true, which allows raw HTML to be rendered from markdown content. This content is then rendered into the DOM using the v-html directive on line 12. While the current markdown sources are local files, this configuration introduces a security risk if the files are ever updated with untrusted content or if the component is modified to accept external input. In an Electron environment, XSS can have a significant impact. It is recommended to set html: false unless raw HTML support is explicitly required for the cheatsheet content.
const md = markdownit({
html: false,
linkify: true,
typographer: true
})
| if (span && icon) { | ||
| const originalText = span.innerText | ||
| const originalIcon = icon.className | ||
|
|
||
| span.innerText = I18nT('base.copySuccess') | ||
| icon.className = 'bi bi-check2' | ||
| btn.classList.add('copied') | ||
|
|
||
| setTimeout(() => { | ||
| span.innerText = originalText | ||
| icon.className = originalIcon | ||
| btn.classList.remove('copied') | ||
| }, 2000) | ||
| } |
There was a problem hiding this comment.
The current implementation of the copy feedback using setTimeout is not robust. If a user clicks the copy button multiple times in quick succession, multiple timeouts will be created, which can lead to unexpected UI behavior. Also, if the component unmounts, the timeout can try to access a DOM element that no longer exists.
To fix this, you should clear any existing timeout for a button before setting a new one. A WeakMap is a good way to manage timeouts for individual buttons.
You'll need to declare const copyTimeoutMap = new WeakMap<HTMLButtonElement, number>() at the component scope.
if (span && icon) {
const existingTimeout = copyTimeoutMap.get(btn)
if (existingTimeout) {
clearTimeout(existingTimeout)
}
span.innerText = I18nT('base.copySuccess')
icon.className = 'bi bi-check2'
btn.classList.add('copied')
const newTimeout = window.setTimeout(() => {
span.innerText = I18nT('base.copy')
icon.className = 'bi bi-clipboard'
btn.classList.remove('copied')
copyTimeoutMap.delete(btn)
}, 2000)
copyTimeoutMap.set(btn, newTimeout)
}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/render/components/Tools/GitCheatsheet/index.vue (1)
12-12:⚠️ Potential issue | 🟠 MajorDisable raw HTML rendering in markdown-it configuration.
Line 12 injects rendered markdown via
v-html, and line 26 enables HTML parsing withhtml: true. Since the memo files (git-memo.en.md,git-memo.vi.md) contain no raw HTML tags, this is an unnecessary XSS surface that should be disabled.Suggested fix
const md = markdownit({ - html: true, + html: false, linkify: true, typographer: true })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/components/Tools/GitCheatsheet/index.vue` at line 12, The component currently injects rendered HTML via v-html="result" while the markdown-it instance is created with html: true; change the markdown-it config to disable raw HTML (set html: false) where the markdown-it instance is initialized (the code that creates the MarkdownIt instance or options object used to render memo files) so the renderer will not parse/allow raw HTML, keeping v-html="result" safe for these markdown memos.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/render/components/Tools/GitCheatsheet/lang/git-memo.en.md`:
- Around line 463-470: The cheat-sheet currently recommends the insecure
"credential.helper store" under the "Credential Storage (Permanent)" section;
replace that single-line recommendation with a recommended "Credential Manager"
entry that suggests using "git credential manager" (git config --global
credential.helper manager) and call out OS-specific secure helpers (osxkeychain
for macOS, libsecret for Linux) as alternatives, keep the existing
"credential.helper 'cache --timeout=3600'" temporary caching section unchanged,
and apply the same edits to the Vietnamese cheatsheet where "credential.helper
store" appears; locate occurrences by searching for the heading "Credential
Storage (Permanent)" and the literal string "credential.helper store" in the
markdown files.
---
Outside diff comments:
In `@src/render/components/Tools/GitCheatsheet/index.vue`:
- Line 12: The component currently injects rendered HTML via v-html="result"
while the markdown-it instance is created with html: true; change the
markdown-it config to disable raw HTML (set html: false) where the markdown-it
instance is initialized (the code that creates the MarkdownIt instance or
options object used to render memo files) so the renderer will not parse/allow
raw HTML, keeping v-html="result" safe for these markdown memos.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/render/components/Tools/GitCheatsheet/git-memo.content.mdsrc/render/components/Tools/GitCheatsheet/index.vuesrc/render/components/Tools/GitCheatsheet/lang/git-memo.en.mdsrc/render/components/Tools/GitCheatsheet/lang/git-memo.vi.md
💤 Files with no reviewable changes (1)
- src/render/components/Tools/GitCheatsheet/git-memo.content.md
There was a problem hiding this comment.
Code Review
This pull request adds a multi-language Git cheatsheet with a new copy-to-clipboard functionality. However, the implementation introduces security and functional concerns due to how markdown and HTML are handled. Enabling raw HTML support in the markdown renderer with "html: true" and using v-html creates a potential XSS vector and can hide text like <type>. Furthermore, manual HTML string construction in custom markdown rules lacks proper escaping for translation strings (I18nT('base.copy')), which could lead to broken UI or injection. Additionally, there are areas for improvement regarding maintainability and UI robustness, including refactoring language selection for scalability and fixing a minor UI bug in the copy button's behavior on rapid clicks.
| const md = markdownit({ | ||
| html: true, | ||
| linkify: true, | ||
| typographer: true | ||
| }) |
There was a problem hiding this comment.
Enabling html: true in markdown-it allows raw HTML in the markdown content to be rendered. This is a security risk when combined with v-html and also causes functional issues where text enclosed in angle brackets (like <type>) is treated as an HTML tag and hidden by the browser. It is recommended to set this to false unless raw HTML support is explicitly required.
const md = markdownit({
html: false,
linkify: true,
typographer: true
})
| md.renderer.rules.fence = function (tokens, idx, options, env, self) { | ||
| const token = tokens[idx] | ||
| const code = token.content.trim() | ||
| const origRendered = defaultRender(tokens, idx, options, env, self) | ||
|
|
||
| return ` | ||
| <div class="code-block-wrapper"> | ||
| <button class="copy-button" data-code="${encodeURIComponent(code)}" type="button" aria-label="${I18nT('base.copy')}"> | ||
| <i class="bi bi-clipboard"></i> | ||
| <span>${I18nT('base.copy')}</span> | ||
| </button> | ||
| ${origRendered} | ||
| </div> | ||
| ` | ||
| } |
There was a problem hiding this comment.
When manually constructing HTML strings, all dynamic content should be properly escaped to prevent HTML injection or broken attributes. The translation string I18nT('base.copy') is inserted directly into the aria-label attribute and <span> content. If the translation contains double quotes or other special characters, it will break the HTML structure. Use md.utils.escapeHtml to safely insert these strings.
md.renderer.rules.fence = function (tokens, idx, options, env, self) {
const token = tokens[idx]
const code = token.content.trim()
const origRendered = defaultRender(tokens, idx, options, env, self)
const copyText = md.utils.escapeHtml(I18nT('base.copy'))
return `
<div class="code-block-wrapper">
<button class="copy-button" data-code="${encodeURIComponent(code)}" type="button" aria-label="${copyText}">
<i class="bi bi-clipboard"></i>
<span>${copyText}</span>
</button>
${origRendered}
</div>
`
}
| const handleContentClick = async (e: MouseEvent) => { | ||
| const target = e.target as HTMLElement | ||
| const btn = target.closest('.copy-button') as HTMLButtonElement | null | ||
| if (!btn) return |
There was a problem hiding this comment.
There's a potential UI bug here. If a user clicks the copy button multiple times quickly, the button's text might get stuck on the "Copied" state. This is because originalText is read from the DOM after it might have been changed by a previous click. A simple way to prevent this is to ignore clicks while the button is in the "copied" state.
if (!btn || btn.classList.contains('copied')) return
…opy behavior
This commit refactors the git cheatsheet component to:
- Update credential helper documentation in both English and Vietnamese.
- Enhance the code copy functionality by:
- Storing copy timeouts to prevent overlapping reset animations.
- Disabling HTML parsing in markdown-it for security.
- Filtering out comment lines from code snippets before copying.
- Using a reactive approach for language memo selection.
…tton functionality
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-language Git cheatsheet with copy-to-clipboard functionality for code snippets, implemented using markdown-it and shiki for rendering and syntax highlighting. A critical Cross-Site Scripting (XSS) vulnerability was identified where a success message, influenced by user-provided language files, is rendered as raw HTML, potentially leading to code execution; escaping the translation string is advised. Furthermore, high-severity reactivity issues are present, affecting initial syntax highlighting and language switching, along with medium-severity opportunities to enhance type safety and error handling in the new Vue component.
| const memo = computed(() => { | ||
| // Ensure i18n.global.locale is reactive. For example, if it's a ref, use i18n.global.locale.value | ||
| const locale = i18n.global.locale as keyof typeof memos | ||
| return memos[locale] ?? MemoEn | ||
| }) |
There was a problem hiding this comment.
The memo computed property depends on i18n.global.locale. With the legacy vue-i18n API being used, i18n.global.locale is a simple string property and not reactive. This means when the application's language is changed, this component will not update to display the cheatsheet in the new language. The code comment on line 104 suggests awareness of this potential issue.
To fix this, you need to make the computed property depend on a reactive source for the locale. For example, if you have a global reactive state for the current language (e.g., in a Pinia store), you should use that.
Example with a reactive store:
// Example with a reactive store
import { useLanguageStore } from '@/stores/language'
const languageStore = useLanguageStore()
const memo = computed(() => {
const locale = languageStore.locale as keyof typeof memos
return memos[locale] ?? MemoEn
})| const result = computed(() => { | ||
| return md.render(memo.value) | ||
| }) |
There was a problem hiding this comment.
The result computed property is calculated before the shiki highlighter is initialized asynchronously in onMounted. Since result does not depend on highlighter, it is not re-evaluated when the highlighter becomes available. This causes code blocks to not be syntax-highlighted on initial load. They will only be highlighted if the locale changes, which forces a re-render.
const result = computed(() => {
return highlighter.value ? md.render(memo.value) : ''
})
|
|
||
| const copy = (v: string) => { | ||
| clipboard.writeText(v) | ||
| MessageSuccess(I18nT('base.copySuccess')) |
There was a problem hiding this comment.
The MessageSuccess utility (defined in src/render/util/Element.ts) uses dangerouslyUseHTMLString: true, which renders the provided message as raw HTML. This call uses a translation string (I18nT('base.copySuccess')) as input. Since the application supports custom user-defined languages (via loadCustomerLang in NodeFn.ts), an attacker could provide a malicious language file that injects HTML or JavaScript into this translation key. When a user clicks the copy button, the malicious code would be executed. In an Electron environment, this can lead to Remote Code Execution (RCE).
To mitigate this, you should sanitize the translation string before passing it to the message utility, or preferably, modify the MessageSuccess utility to disable HTML rendering by default.
MessageSuccess(md.utils.escapeHtml(I18nT('base.copySuccess')))
| import { createHighlighter } from 'shiki' | ||
|
|
||
| const i18n = AppI18n() | ||
| const highlighter = ref<any>(null) |
There was a problem hiding this comment.
The highlighter ref is typed as any. Using any bypasses TypeScript's static type checking, reducing code safety and maintainability. You can import the Highlighter type from shiki for better type safety and code clarity.
import { createHighlighter, type Highlighter } from 'shiki'
const i18n = AppI18n()
const highlighter = ref<Highlighter | null>(null)
| highlighted = match[1] | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| } catch (e) {} |
There was a problem hiding this comment.
The catch block for syntax highlighting is empty. This practice, known as "swallowing errors," can make debugging very difficult if shiki fails to highlight code for any reason. The eslint-disable-next-line comment suggests this was intentional, but it's better to at least log the error for development and debugging purposes.
} catch (e) { console.error('Shiki highlighting failed:', e) }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/render/components/Tools/GitCheatsheet/index.vue`:
- Around line 136-143: The click handler currently spawns a new setTimeout each
click and never clears timers on unmount; modify the onClick logic to keep a
single timer id (e.g., a local ref or per-button Map) and before creating a new
setTimeout call clearTimeout(existingTimerId), then store the new timer id; also
add an onBeforeUnmount hook that clears the stored timer(s) with clearTimeout to
avoid orphaned timers. Update references to btnIcon, markRaw(Check),
markRaw(CopyDocument), and renderBtn to remain the same—only change how the
timer is managed and cleaned up.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/render/components/Tools/GitCheatsheet/index.vuesrc/render/components/Tools/GitCheatsheet/lang/git-memo.en.mdsrc/render/components/Tools/GitCheatsheet/lang/git-memo.vi.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/render/components/Tools/GitCheatsheet/lang/git-memo.vi.md
| onClick: () => { | ||
| copy(code) | ||
| btnIcon.value = markRaw(Check) | ||
| renderBtn() | ||
| setTimeout(() => { | ||
| btnIcon.value = markRaw(CopyDocument) | ||
| renderBtn() | ||
| }, 2000) |
There was a problem hiding this comment.
Keep one icon-reset timer per button and clear on unmount.
Rapid clicks queue multiple timers; older timers can reset the icon too early. Also, timers are not cleared on unmount.
💡 Suggested fix
- let observer: MutationObserver | null = null
+ let observer: MutationObserver | null = null
+ const resetTimers = new Map<Element, ReturnType<typeof setTimeout>>()
onMounted(async () => {
@@
onUnmounted(() => {
observer?.disconnect()
+ resetTimers.forEach((id) => clearTimeout(id))
+ resetTimers.clear()
})
@@
onClick: () => {
copy(code)
btnIcon.value = markRaw(Check)
renderBtn()
- setTimeout(() => {
+ const existing = resetTimers.get(p)
+ if (existing) clearTimeout(existing)
+ const timeoutId = setTimeout(() => {
btnIcon.value = markRaw(CopyDocument)
renderBtn()
+ resetTimers.delete(p)
}, 2000)
+ resetTimers.set(p, timeoutId)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/components/Tools/GitCheatsheet/index.vue` around lines 136 - 143,
The click handler currently spawns a new setTimeout each click and never clears
timers on unmount; modify the onClick logic to keep a single timer id (e.g., a
local ref or per-button Map) and before creating a new setTimeout call
clearTimeout(existingTimerId), then store the new timer id; also add an
onBeforeUnmount hook that clears the stored timer(s) with clearTimeout to avoid
orphaned timers. Update references to btnIcon, markRaw(Check),
markRaw(CopyDocument), and renderBtn to remain the same—only change how the
timer is managed and cleaned up.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const renderBtn = () => { | ||
| const vnode = h( | ||
| ElButton, | ||
| { | ||
| link: true, | ||
| icon: btnIcon.value, | ||
| onClick: () => { | ||
| copy(code) | ||
| btnIcon.value = markRaw(Check) | ||
| renderBtn() |
There was a problem hiding this comment.
Bug: Dynamically mounted button components using render() are not unmounted when the locale changes, causing a memory leak as old component instances accumulate with each switch.
Severity: MEDIUM
Suggested Fix
Implement cleanup logic for the dynamically rendered components. Before the v-html content is replaced, or within the onUnmounted hook, iterate through the mounted components and explicitly unmount them by calling render(null, container) for each one. This will ensure their memory is properly released.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/render/components/Tools/GitCheatsheet/index.vue#L134-L143
Potential issue: The `mountButtons` function uses Vue's `render()` to dynamically mount
button components into placeholders. When the locale changes, the `v-html` directive
replaces the entire content, removing the old placeholder elements from the DOM.
However, the components rendered into those placeholders are not explicitly unmounted.
This results in orphaned component instances, refs, and event listeners remaining in
memory. Each language switch accumulates more orphaned components, leading to a gradual
memory leak that can degrade application performance over time for users who frequently
change languages.
Did we get this right? 👍 / 👎 to inform future reviews.
1 participant
Summary by CodeRabbit
New Features
Documentation
Removed