fix: Show delete_key in UI after URL shortening - #5
Conversation
manishym
commented
Mar 19, 2026
manishym
commented
- Display delete key below shortened URL
- Add copy button for delete key
- Pre-fill delete path and key in delete form
- Require delete key for deletion
- Display delete key below shortened URL - Add copy button for delete key - Pre-fill delete path and key in delete form - Require delete key for deletion
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 improves the user experience and security for managing shortened URLs. It introduces the immediate display and easy copying of the delete key after URL shortening, and integrates this key into a more intuitive and secure deletion workflow. Users can now effortlessly delete their links with the required authentication, making the system more robust and user-friendly. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
|
/gemini review |
![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 updates the frontend to display the delete key after a URL is shortened, along with a button to copy it. It also pre-fills the deletion form with the short path and delete key, and makes the delete key a required field for deletion. The changes are functionally correct and implement the described features. I've added a few comments with suggestions to improve code quality and maintainability by avoiding inline styles, using safer DOM manipulation methods, and reducing code duplication.
| <div class="result-row" style="margin-top: 0.75rem;"> | ||
| <span style="color: var(--text-muted); font-size: 0.8rem;">Delete key:</span> | ||
| <span class="short-url" id="delete-key-display" style="font-size: 0.8rem;"></span> | ||
| <button type="button" class="btn-ghost copy-btn" id="copy-key-btn">Copy Key</button> |
There was a problem hiding this comment.
For better maintainability and separation of concerns, it's recommended to avoid inline styles. Consider creating dedicated CSS classes in the <style> block for these elements. For example, you could create classes for the delete key row, label, and value, and apply them here instead of using style attributes. This makes the styling more consistent and easier to manage.
| document.getElementById('short-url-display').innerHTML = | ||
| '<a href="' + shortFull + '" target="_blank" rel="noopener">' + shortFull + '</a>'; |
There was a problem hiding this comment.
To prevent potential Cross-Site Scripting (XSS) vulnerabilities, it's safer to avoid using innerHTML to construct DOM elements. Even though shortFull is constructed from what seems to be safe data, it's a best practice to create and append elements programmatically using document.createElement and element.appendChild (or element.replaceChildren). This approach is inherently safer against injection attacks.
| document.getElementById('copy-key-btn').addEventListener('click', () => { | ||
| if (!lastDeleteKey) return; | ||
| navigator.clipboard.writeText(lastDeleteKey).then(() => { | ||
| const btn = document.getElementById('copy-key-btn'); | ||
| const orig = btn.textContent; | ||
| btn.textContent = 'Copied!'; | ||
| setTimeout(() => { btn.textContent = orig; }, 2000); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This logic for updating the button text to 'Copied!' and back is duplicated from the 'copy-btn' event listener (lines 276-279). To improve maintainability and follow the Don't Repeat Yourself (DRY) principle, consider extracting this UI feedback logic into a shared helper function. For example, a function like showCopiedFeedback(buttonElement) could be created and called from both event listeners.
There was a problem hiding this comment.
Code Review
This pull request successfully adds the functionality to display and use a delete key for shortened URLs. The backend changes and associated tests are well-implemented. My review focuses on the frontend changes in gateway/static/index.html, where I've identified opportunities to improve maintainability by removing inline styles and reducing code duplication, and to enhance security by avoiding the use of innerHTML.
| <div class="result-row" style="margin-top: 0.75rem;"> | ||
| <span style="color: var(--text-muted); font-size: 0.8rem;">Delete key:</span> | ||
| <span class="short-url" id="delete-key-display" style="font-size: 0.8rem;"></span> | ||
| <button type="button" class="btn-ghost copy-btn" id="copy-key-btn">Copy Key</button> | ||
| </div> |
There was a problem hiding this comment.
Using inline styles makes the code harder to maintain and violates the separation of concerns principle. It's better to define styles in CSS classes. Please consider moving the inline styles for margin-top, color, and font-size into dedicated CSS classes within the <style> block at the top of the file.
For example, you could add classes like .result-row--secondary, .result-label, and .result-value to your stylesheet and apply them here. This will make your HTML cleaner and your styles easier to manage.
| document.getElementById('short-url-display').innerHTML = | ||
| '<a href="' + shortFull + '" target="_blank" rel="noopener">' + shortFull + '</a>'; |
There was a problem hiding this comment.
Using innerHTML to set content from an API response can introduce Cross-Site Scripting (XSS) vulnerabilities if the data is not properly sanitized or if the API's output changes unexpectedly. A safer approach is to programmatically create DOM elements.
| document.getElementById('short-url-display').innerHTML = | |
| '<a href="' + shortFull + '" target="_blank" rel="noopener">' + shortFull + '</a>'; | |
| const displayEl = document.getElementById('short-url-display'); | |
| displayEl.textContent = ''; // Clear existing content | |
| const linkEl = document.createElement('a'); | |
| linkEl.href = shortFull; | |
| linkEl.target = '_blank'; | |
| linkEl.rel = 'noopener'; | |
| linkEl.textContent = shortFull; | |
| displayEl.appendChild(linkEl); |
| document.getElementById('copy-key-btn').addEventListener('click', () => { | ||
| if (!lastDeleteKey) return; | ||
| navigator.clipboard.writeText(lastDeleteKey).then(() => { | ||
| const btn = document.getElementById('copy-key-btn'); | ||
| const orig = btn.textContent; | ||
| btn.textContent = 'Copied!'; | ||
| setTimeout(() => { btn.textContent = orig; }, 2000); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The logic to show 'Copied!' on the button is nearly identical to the event listener for copy-btn (lines 271-281). To avoid code duplication and improve maintainability, this common logic should be extracted into a shared helper function.
For example, you could create a function like this:
function animateCopied(button) {
const originalText = button.textContent;
button.textContent = 'Copied!';
setTimeout(() => { button.textContent = originalText; }, 2000);
}Then, both click handlers could simply call animateCopied(event.currentTarget) inside their .then() block. This would make the code cleaner and easier to update in the future.
- Replace inline styles with CSS classes (.result-label, .result-value) - Use safe DOM manipulation instead of innerHTML to prevent XSS - Extract duplicated 'Copied!' logic into animateCopied() helper
- Move zql.png to gateway/static - Add favicon link to HTML head - Add logo image to page header
- Change from number input (seconds) to datetime-local picker - Convert selected datetime to seconds from current time
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the UI to display and manage the delete_key for shortened URLs. The changes include showing the delete key after creation, adding a button to copy it, and pre-filling the deletion form. The code is well-structured, for instance by extracting the animateCopied function. I've added a couple of suggestions to improve the robustness of the clipboard copy functionality by handling potential errors.
| btn.textContent = 'Copied!'; | ||
| setTimeout(() => { btn.textContent = orig; }, 2000); | ||
| animateCopied(document.getElementById('copy-btn')); | ||
| }); |
There was a problem hiding this comment.
The navigator.clipboard.writeText method returns a promise that can reject if the clipboard operation fails (e.g., due to browser permissions or other issues). It's a good practice to handle potential errors using .catch() to provide feedback to the user or log the error.
| }); | |
| }).catch(err => console.error('Failed to copy URL:', err)); |
| if (!lastDeleteKey) return; | ||
| navigator.clipboard.writeText(lastDeleteKey).then(() => { | ||
| animateCopied(document.getElementById('copy-key-btn')); | ||
| }); |
There was a problem hiding this comment.
Similar to the other clipboard operation, it's important to handle potential errors when writing to the clipboard. Adding a .catch() block will make the feature more robust.
| }); | |
| }).catch(err => console.error('Failed to copy delete key:', err)); |
- Add .catch() to clipboard.writeText() for better error handling
1 participant
