Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a complete static website for jcli: domain config, HTML landing page, dark-themed responsive CSS, and JS for a terminal typewriter, tab switching, and copy-to-clipboard interactions. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
🤖 Fix all issues with AI agents
In `@docs/index.html`:
- Line 29: Replace the incorrect Go module path in the install command: update
the <code id="install-cmd"> value from "go install
github.com/tutuank/jcli@latest" to use the correct GitHub username "tutunak" so
the string becomes "go install github.com/tutunak/jcli@latest"; ensure the <code
id="install-cmd"> element text matches other references (e.g., line 105/GitHub
URLs).
In `@docs/js/terminal.js`:
- Around line 76-83: The copyInstall function currently calls
navigator.clipboard.writeText(cmd) without handling promise rejections; update
copyInstall to attach a .catch handler to navigator.clipboard.writeText(cmd) to
handle failures (e.g., permission denied or API unavailable), and in the catch:
log the error (console.error) and provide UI feedback (use the same button
element queried as btn — e.g., add a distinct error class like 'copy-error' or
change text content, then remove it with setTimeout similar to the success
path). Also consider a graceful fallback path when navigator.clipboard is
undefined (e.g., fallback to creating a temporary textarea and
document.execCommand('copy') or display instructions to manually copy).
🧹 Nitpick comments (3)
docs/css/style.css (1)
36-45: Consider adding a fallback forbackdrop-filter.While
backdrop-filterhas good modern browser support, older browsers will show no blur effect. The semi-transparent background color provides a reasonable fallback, so this is acceptable as-is.docs/js/terminal.js (2)
26-30: DOM queries execute before DOMContentLoaded.The
getElementByIdandquerySelectorcalls on lines 26-27 run immediately when the script loads. Since the script is placed at the end of the body, this should work, but moving these inside theDOMContentLoadedcallback would be more robust and explicit.♻️ Suggested refactor
-const terminalOutput = document.getElementById('terminal-output'); -const cursor = document.querySelector('.cursor'); let currentSequence = 0; let currentChar = 0; let isTyping = false; +let terminalOutput; +let cursor; // ... (keep typeWriter and typeCharacter functions) // Start animation when page loads document.addEventListener('DOMContentLoaded', () => { + terminalOutput = document.getElementById('terminal-output'); + cursor = document.querySelector('.cursor'); setTimeout(typeWriter, 500); });
86-94: Static analysis hint is a false positive.Biome flags lines 88-89 for returning values from
forEachcallbacks, butclassList.remove()andclassList.add()returnundefined, not meaningful values. The code is functionally correct. However, usingfor...ofloops would be slightly more idiomatic and avoid the lint warning.♻️ Optional refactor to avoid lint warning
// Tab switching document.querySelectorAll('.tab').forEach(tab => { tab.addEventListener('click', () => { - document.querySelectorAll('.tab').forEach(t => t.classList.remove('active')); - document.querySelectorAll('.tab-content').forEach(c => c.classList.remove('active')); + for (const t of document.querySelectorAll('.tab')) { + t.classList.remove('active'); + } + for (const c of document.querySelectorAll('.tab-content')) { + c.classList.remove('active'); + } tab.classList.add('active'); document.getElementById(tab.dataset.tab).classList.add('active'); }); });
There was a problem hiding this comment.
Pull request overview
This pull request adds a static marketing website for the jcli CLI tool, hosted on GitHub Pages. The site provides an interactive terminal demo, installation instructions, and feature highlights to help users understand and adopt the tool.
Changes:
- Added a complete static website with HTML, CSS, and JavaScript
- Implemented an animated terminal demonstration showcasing the CLI workflow
- Configured GitHub Pages with custom domain (jcli.tutunak.com)
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/index.html | Main landing page with hero section, features, installation guide, and workflow documentation |
| docs/js/terminal.js | Interactive typewriter animation for terminal demo and UI interaction handlers |
| docs/css/style.css | Complete styling with dark theme, responsive design, and GitHub-inspired aesthetics |
| docs/assets/favicon.svg | Simple SVG favicon with monospace "j>" branding |
| docs/CNAME | GitHub Pages custom domain configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let isTyping = false; | ||
|
|
There was a problem hiding this comment.
The variable isTyping is declared but never used. It appears to be leftover from development. Consider removing it or implementing it if it was meant to prevent multiple simultaneous animations.
| let isTyping = false; |
| <p class="tagline">Select issues. Track work. Generate branches. Stay in flow.</p> | ||
| <div class="install-box"> | ||
| <code id="install-cmd">go install github.com/tutunak/jcli@latest</code> | ||
| <button class="copy-btn" onclick="copyInstall()" aria-label="Copy install command"> |
There was a problem hiding this comment.
Using inline event handlers (onclick="copyInstall()") is considered a security and maintainability concern. It violates Content Security Policy (CSP) best practices and makes the code harder to maintain. Consider moving this to an event listener in the JavaScript file, similar to how the tab switching is implemented (lines 86-94 in terminal.js).
| document.querySelectorAll('.tab').forEach(tab => { | ||
| tab.addEventListener('click', () => { | ||
| document.querySelectorAll('.tab').forEach(t => t.classList.remove('active')); | ||
| document.querySelectorAll('.tab-content').forEach(c => c.classList.remove('active')); | ||
|
|
||
| tab.classList.add('active'); | ||
| document.getElementById(tab.dataset.tab).classList.add('active'); |
There was a problem hiding this comment.
The tab buttons are missing keyboard accessibility. When using keyboard navigation, users should be able to activate tabs with both Enter and Space keys, and arrow keys should navigate between tabs. Consider adding appropriate keyboard event handlers (keydown/keypress) alongside the click handler to ensure full keyboard accessibility.
| document.querySelectorAll('.tab').forEach(tab => { | |
| tab.addEventListener('click', () => { | |
| document.querySelectorAll('.tab').forEach(t => t.classList.remove('active')); | |
| document.querySelectorAll('.tab-content').forEach(c => c.classList.remove('active')); | |
| tab.classList.add('active'); | |
| document.getElementById(tab.dataset.tab).classList.add('active'); | |
| function activateTab(tab, tabs) { | |
| tabs.forEach(t => t.classList.remove('active')); | |
| document.querySelectorAll('.tab-content').forEach(c => c.classList.remove('active')); | |
| tab.classList.add('active'); | |
| var targetId = tab.dataset.tab; | |
| if (targetId) { | |
| var target = document.getElementById(targetId); | |
| if (target) { | |
| target.classList.add('active'); | |
| } | |
| } | |
| } | |
| const tabs = document.querySelectorAll('.tab'); | |
| tabs.forEach((tab, index) => { | |
| tab.addEventListener('click', () => { | |
| activateTab(tab, tabs); | |
| }); | |
| tab.addEventListener('keydown', (event) => { | |
| const key = event.key; | |
| if (key === 'Enter' || key === ' ') { | |
| event.preventDefault(); | |
| activateTab(tab, tabs); | |
| return; | |
| } | |
| let newIndex = -1; | |
| if (key === 'ArrowRight' || key === 'ArrowDown') { | |
| event.preventDefault(); | |
| newIndex = (index + 1) % tabs.length; | |
| } else if (key === 'ArrowLeft' || key === 'ArrowUp') { | |
| event.preventDefault(); | |
| newIndex = (index - 1 + tabs.length) % tabs.length; | |
| } | |
| if (newIndex !== -1) { | |
| const newTab = tabs[newIndex]; | |
| newTab.focus(); | |
| activateTab(newTab, tabs); | |
| } |
| navigator.clipboard.writeText(cmd).then(() => { | ||
| const btn = document.querySelector('.copy-btn'); | ||
| btn.classList.add('copied'); | ||
| setTimeout(() => btn.classList.remove('copied'), 2000); | ||
| }); |
There was a problem hiding this comment.
The clipboard write operation lacks error handling. If navigator.clipboard.writeText() fails (e.g., due to permissions or browser compatibility), the promise rejection is silently ignored. Consider adding a .catch() handler to gracefully handle failures and potentially show an error message to the user.
| navigator.clipboard.writeText(cmd).then(() => { | |
| const btn = document.querySelector('.copy-btn'); | |
| btn.classList.add('copied'); | |
| setTimeout(() => btn.classList.remove('copied'), 2000); | |
| }); | |
| if (navigator.clipboard && typeof navigator.clipboard.writeText === 'function') { | |
| navigator.clipboard.writeText(cmd).then(() => { | |
| const btn = document.querySelector('.copy-btn'); | |
| if (btn) { | |
| btn.classList.add('copied'); | |
| setTimeout(() => btn.classList.remove('copied'), 2000); | |
| } | |
| }).catch((err) => { | |
| // Optional: log the error for debugging; could also show a user-facing message here | |
| console.error('Failed to copy to clipboard:', err); | |
| }); | |
| } else { | |
| // Clipboard API not available; optional: provide a fallback or user notification | |
| console.error('Clipboard API is not available in this browser.'); | |
| } |
| const cmd = document.getElementById('install-cmd').textContent; | ||
| navigator.clipboard.writeText(cmd).then(() => { | ||
| const btn = document.querySelector('.copy-btn'); |
There was a problem hiding this comment.
Missing null check for DOM element selection. If the element with id 'install-cmd' doesn't exist, document.getElementById('install-cmd') will return null, and accessing .textContent on null will throw a TypeError. Consider adding a null check before accessing the textContent property.
| const cmd = document.getElementById('install-cmd').textContent; | |
| navigator.clipboard.writeText(cmd).then(() => { | |
| const btn = document.querySelector('.copy-btn'); | |
| const installCmdElement = document.getElementById('install-cmd'); | |
| if (!installCmdElement) { | |
| return; | |
| } | |
| const cmd = installCmdElement.textContent || ''; | |
| navigator.clipboard.writeText(cmd).then(() => { | |
| const btn = document.querySelector('.copy-btn'); | |
| if (!btn) { | |
| return; | |
| } |
| if (currentChar === 0 && seq.delay > 0) { | ||
| setTimeout(() => { | ||
| currentChar = 0; | ||
| typeCharacter(); | ||
| }, seq.delay); | ||
| } else { | ||
| typeCharacter(); | ||
| } | ||
| } | ||
|
|
||
| function typeCharacter() { | ||
| const seq = sequences[currentSequence]; | ||
|
|
||
| if (currentChar < seq.text.length) { | ||
| const span = document.createElement('span'); | ||
| span.className = seq.class || ''; | ||
| span.textContent = seq.text[currentChar]; | ||
| terminalOutput.appendChild(span); | ||
| currentChar++; | ||
|
|
||
| const charDelay = seq.delay || 30; | ||
| setTimeout(typeCharacter, charDelay); |
There was a problem hiding this comment.
The delay property in the sequences array serves dual purposes: as an initial delay before typing a sequence (line 46) and as the per-character typing delay (line 66). This creates confusion and makes the code harder to maintain. Consider separating these into two distinct properties, such as initialDelay and charDelay, to make the intent clearer. Also, line 48 redundantly sets currentChar = 0 when it's already 0.
| <h1>Jira without leaving your terminal.</h1> | ||
| <p class="tagline">Select issues. Track work. Generate branches. Stay in flow.</p> | ||
| <div class="install-box"> | ||
| <code id="install-cmd">go install github.com/tutunak/jcli@latest</code> |
There was a problem hiding this comment.
The GitHub repository path has a typo. It should be "github.com/tutunak/jcli" (with 'n' after 'tutua'), not "github.com/tutuank/jcli". This is inconsistent with the actual repository path used throughout the codebase (see cmd/config.go:7, main.go:7, etc.).
| btn.classList.add('copied'); | ||
| setTimeout(() => btn.classList.remove('copied'), 2000); |
There was a problem hiding this comment.
Missing null check for DOM element selection. If the element with class 'copy-btn' doesn't exist, document.querySelector('.copy-btn') will return null, and calling .classList.add() on null will throw a TypeError. Consider adding a null check before manipulating the classList.
| btn.classList.add('copied'); | |
| setTimeout(() => btn.classList.remove('copied'), 2000); | |
| if (btn) { | |
| btn.classList.add('copied'); | |
| setTimeout(() => btn.classList.remove('copied'), 2000); | |
| } |
| document.querySelectorAll('.tab-content').forEach(c => c.classList.remove('active')); | ||
|
|
||
| tab.classList.add('active'); | ||
| document.getElementById(tab.dataset.tab).classList.add('active'); |
There was a problem hiding this comment.
Missing null check when accessing tab dataset. If tab.dataset.tab is undefined or if document.getElementById(tab.dataset.tab) returns null, calling .classList.add() on null will throw a TypeError. Consider adding checks to ensure both the dataset property exists and the corresponding element is found.
| document.getElementById(tab.dataset.tab).classList.add('active'); | |
| const contentId = tab.dataset ? tab.dataset.tab : null; | |
| if (!contentId) { | |
| return; | |
| } | |
| const content = document.getElementById(contentId); | |
| if (content) { | |
| content.classList.add('active'); | |
| } |
| function typeWriter() { | ||
| if (currentSequence >= sequences.length) { | ||
| // Reset and loop | ||
| setTimeout(() => { | ||
| terminalOutput.innerHTML = ''; | ||
| currentSequence = 0; | ||
| currentChar = 0; | ||
| typeWriter(); | ||
| }, 3000); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The typewriter animation runs indefinitely without any cleanup mechanism. If the user navigates away from the page and back (in a single-page application context), or if the terminal element is removed from the DOM, the setTimeout callbacks will continue to execute, potentially causing memory leaks. Consider adding a stop/cleanup function that clears pending timeouts, or checking if the terminal element still exists in the DOM before continuing the animation.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.