Skip to content

Conversation

@rosacry
Copy link
Contributor

@rosacry rosacry commented Dec 15, 2025

  • Add audio player modal for selecting preview time
  • Add /get_audio_info and /serve_audio routes for audio playback
  • Add /get_image_preview route for background image preview
  • Add Beatmap Customization section with preview time and background
  • Display preview time in human-readable format (m:ss)
  • Support seeking in audio and setting preview point
  • Add background image preview thumbnail

- Add audio player modal for selecting preview time
- Add /get_audio_info and /serve_audio routes for audio playback
- Add /get_image_preview route for background image preview
- Add Beatmap Customization section with preview time and background
- Display preview time in human-readable format (m:ss)
- Support seeking in audio and setting preview point
- Add background image preview thumbnail
Copilot AI review requested due to automatic review settings December 15, 2025 20:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a preview time picker feature with an audio player modal, allowing users to select a specific point in a song for beatmap preview. It also adds background image customization with preview thumbnails.

Key changes include:

  • New backend routes for serving audio files with range request support and retrieving image previews via base64 encoding
  • Interactive audio player modal with seek functionality and millisecond precision for setting preview points
  • Background image preview with automatic thumbnail generation

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.

File Description
web-ui.py Adds three new Flask routes: /get_audio_info for audio metadata, /serve_audio for streaming audio with HTTP range support, and /get_image_preview for base64-encoded image previews
template/index.html Adds Beatmap Customization fieldset with preview time picker input, audio player modal trigger button, and background image selector with preview thumbnail display
static/style.css Defines styles for preview time picker UI, modal overlay and dialog, audio player controls, and background image preview thumbnail
static/app.js Implements CustomizationManager with audio player modal, time display formatting, seek controls, and background image preview functionality

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ext = os.path.splitext(image_path)[1].lower()
if ext not in ['.jpg', '.jpeg', '.png', '.gif', '.bmp']:
return jsonify({"success": False, "message": "Unsupported image format"})

Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading entire files into memory with base64 encoding could cause memory issues with large images. Consider adding file size validation before encoding, or implementing a streaming/chunked approach for larger files.

Suggested change
# File size validation (e.g., 5 MB limit)
max_size = 5 * 1024 * 1024 # 5 MB
file_size = os.path.getsize(image_path)
if file_size > max_size:
return jsonify({"success": False, "message": "Image file too large (max 5 MB)"})

Copilot uses AI. Check for mistakes.
Comment on lines +1113 to +1125
audio.addEventListener('loadedmetadata', () => {
if (this.lastPickerPosition > 0) {
audio.currentTime = this.lastPickerPosition / 1000;
}
});

// Update time display
audio.addEventListener('timeupdate', () => {
const ms = Math.floor(audio.currentTime * 1000);
const minutes = Math.floor(audio.currentTime / 60);
const secs = Math.floor(audio.currentTime % 60);
$('#preview-current-time').text(`${minutes}:${secs.toString().padStart(2, '0')} (${ms}ms)`);
});
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audio event listeners are not cleaned up when the modal is closed. The 'loadedmetadata' and 'timeupdate' event listeners remain attached to the audio element even after closePreviewModal removes it from the DOM. Consider using addEventListener with { once: true } for loadedmetadata or explicitly removing listeners in closePreviewModal.

Copilot uses AI. Check for mistakes.
</div>
</div>
<div id="background-preview" class="background-preview" style="display: none;">
<img id="background-preview-img" src="" alt="Background preview" />
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The background preview image lacks appropriate alt text. While 'Background preview' is provided, it would be more helpful to provide more descriptive alt text or use an empty alt attribute (alt="") if the image is purely decorative, with the context provided by the surrounding elements.

Suggested change
<img id="background-preview-img" src="" alt="Background preview" />
<img id="background-preview-img" src="" alt="" />

Copilot uses AI. Check for mistakes.
Comment on lines +191 to +194
audio_path = request.args.get('path', '')

if not audio_path or not os.path.exists(audio_path):
return "Audio file not found", 404
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The audio file path comes directly from user input without validation beyond existence check. This creates a path traversal vulnerability where a malicious user could access any file on the system that the application has permissions to read. Add validation to ensure the path is within allowed directories, or use a whitelist approach.

Copilot uses AI. Check for mistakes.
Comment on lines +232 to +236
data = request.get_json() or {}
image_path = data.get('path', '')

if not image_path or not os.path.exists(image_path):
return jsonify({"success": False, "message": "Image file not found"})
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The image file path comes directly from user input without validation beyond existence check. This creates a path traversal vulnerability where a malicious user could access any file on the system (e.g., /etc/passwd), have it base64 encoded, and sent back. Add validation to ensure the path is within allowed directories, or use a whitelist approach.

Copilot uses AI. Check for mistakes.
Comment on lines +211 to +213
byte_range = range_header.replace('bytes=', '').split('-')
start = int(byte_range[0]) if byte_range[0] else 0
end = int(byte_range[1]) if byte_range[1] else file_size - 1
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The byte range parsing doesn't handle malformed Range headers properly. If byte_range[0] or byte_range[1] contains non-numeric values, int() will raise a ValueError. Add proper error handling or validation for the Range header values.

Copilot uses AI. Check for mistakes.
Comment on lines +216 to +224
with open(audio_path, 'rb') as f:
f.seek(start)
data = f.read(length)

response = Response(data, 206, mimetype=content_type)
response.headers['Content-Range'] = f'bytes {start}-{end}/{file_size}'
response.headers['Accept-Ranges'] = 'bytes'
response.headers['Content-Length'] = length
return response
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file is opened but not properly closed in case of exceptions during read operations. While Python's garbage collector will eventually close it, it's better practice to use a context manager or ensure proper cleanup. Consider moving the response creation inside the with block or using try-finally.

Copilot uses AI. Check for mistakes.
Comment on lines +1098 to +1102
$('.preview-modal-close, .preview-modal-overlay').on('click', (e) => {
if (e.target === e.currentTarget) {
this.closePreviewModal();
}
});
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Event handlers for modal close are attached multiple times if the modal is opened repeatedly. The click handler on '.preview-modal-overlay' is attached inside setupPreviewAudio which is called every time the modal opens, leading to multiple handlers being bound. Move this event handler attachment to createPreviewModal or use event delegation to avoid duplicate handlers.

Copilot uses AI. Check for mistakes.
Comment on lines +169 to +172
<input type="text" id="background_path" name="background_path" placeholder="None" />
<button type="button" class="clear-input-btn" data-target="background_path" style="display: none;">×</button>
</div>
<button type="button" class="browse-button" data-browse-type="file" data-target="background_path">Browse...</button>
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The background image input field and button lack proper accessibility attributes. Add aria-label or aria-describedby attributes to the input field for screen reader users, and ensure the button has descriptive text or aria-label since 'Browse...' may not clearly indicate it's for selecting a background image.

Suggested change
<input type="text" id="background_path" name="background_path" placeholder="None" />
<button type="button" class="clear-input-btn" data-target="background_path" style="display: none;">×</button>
</div>
<button type="button" class="browse-button" data-browse-type="file" data-target="background_path">Browse...</button>
<input type="text" id="background_path" name="background_path" placeholder="None" aria-describedby="background_path_desc" />
<button type="button" class="clear-input-btn" data-target="background_path" style="display: none;">×</button>
<span id="background_path_desc" class="visually-hidden">Enter the path to the background image for the beatmap, or use the browse button to select a file.</span>
</div>
<button type="button" class="browse-button" data-browse-type="file" data-target="background_path" aria-label="Browse for background image">Browse...</button>

Copilot uses AI. Check for mistakes.
try:
return jsonify({
"success": True,
"url": f"/serve_audio?path={audio_path}",
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The audio_path parameter from user input is passed directly to the URL without URL encoding. This could cause issues with file paths containing special characters (spaces, quotes, etc.) and also poses a potential security risk. Consider using URL encoding for the path parameter or using a more secure approach like generating temporary tokens that map to file paths server-side.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant