-
Notifications
You must be signed in to change notification settings - Fork 3
Initial UI fixes - Display artist name, album name, durations, various other stuff #71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
will need to be cleaned up and deployed when the new Lens SDK version is published. |
There was a problem hiding this 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 implements various UI fixes to improve the display and handling of artist names, album information, track durations, and metadata across the music player application. The changes focus on enhancing the user experience by showing more relevant information and fixing data display issues.
Key changes:
- Replace category ID checks with category slug lookups for proper content type detection
- Add comprehensive track metadata handling including ID3 tag verification and duration calculation
- Improve release form field mapping and metadata parsing throughout the application
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| releasePage.vue | Updates category checks to use slugs instead of IDs for proper content type detection |
| categoryPage.vue | Fixes category filtering to use actual category IDs instead of slugs |
| albumViewer.vue | Major enhancement adding track metadata display, ID3 tag verification, and duration calculations |
| releaseForm.vue | Improves metadata field handling with better parsing and user-friendly field labels |
| featuredSlider.vue | Updates category checks to use slug-based lookups for proper content display |
| router.ts | Adds data transformation for prefetched releases and categories to parse JSON metadata |
| hooks.ts | Adds cache invalidation for featured releases when editing releases |
| maintenanceManagement.vue | Implements category slug-to-ID mapping for data import functionality |
| preload/index.ts | Adds editRelease method to the electron API bridge |
| main/index.ts | Implements IPC handler for the editRelease functionality |
| package.json | Updates lens-sdk dependency and adds jsmediatags library |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| const categorySlug = computed(() => { | ||
| if (!targetRelease.value || !contentCategories.value) return null; | ||
| const category = contentCategories.value.find(cat => cat.id === targetRelease.value.categoryId); | ||
| return category?.categoryId || null; |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The computed property returns category?.categoryId but should return category?.categoryId (slug) for consistency with the variable name categorySlug. However, the logic appears to be looking for the category's slug field, but it's accessing categoryId which might be the ID, not the slug.
| return category?.categoryId || null; | |
| return category?.slug || null; |
| try { | ||
| const ipfsFiles: AudioTrack[] = []; | ||
| const response = await fetch(url); | ||
| const response = await fetch(url, { method: 'HEAD' }); |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using HEAD request to check content-type but then making another full GET request on line 301. This creates unnecessary network calls. Consider using a single GET request or handle the HEAD response properly.
| const response = await fetch(url, { method: 'HEAD' }); | |
| const response = await fetch(url, { method: 'GET' }); |
| // Check if user is admin or moderator | ||
| const { data: accountStatus } = useAccountStatusQuery(); | ||
| const isAdmin = computed(() => accountStatus.value?.isAdmin || false); | ||
| const isModerator = computed(() => accountStatus.value?.hasRole?.('moderator') || false); |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hasRole method call syntax hasRole?.('moderator') suggests hasRole might be a function, but the optional chaining is applied incorrectly. If hasRole is a function, it should be accountStatus.value?.hasRole?.('moderator') or if it's a property containing roles, the syntax should be different.
| const isModerator = computed(() => accountStatus.value?.hasRole?.('moderator') || false); | |
| const isModerator = computed(() => typeof accountStatus.value?.hasRole === 'function' && accountStatus.value.hasRole('moderator') || false); |
| }; | ||
| // Only update fields that are defined in the schema | ||
| if (selectedContentCategory.value && fieldName in selectedContentCategory.value) { |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition checks if fieldName exists in selectedContentCategory.value, but this could allow setting arbitrary fields. Consider validating against a whitelist of expected field names or the schema definition to prevent unexpected metadata fields.
| if (selectedContentCategory.value && fieldName in selectedContentCategory.value) { | |
| if ( | |
| selectedContentCategory.value && | |
| Object.keys(selectedContentCategory.value).includes(fieldName) | |
| ) { |
| import { parseUrlOrCid } from '/@/utils'; | ||
| import type { ReleaseItem } from '/@/types'; | ||
| import { useAccountStatusQuery, useEditReleaseMutation } from '/@/plugins/lensService/hooks'; | ||
| // @ts-ignore |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using @ts-ignore to suppress TypeScript errors for jsmediatags import. Consider adding proper type definitions or using a more specific import statement to avoid suppressing all TypeScript checks.
| ipcMain.handle('peerbit:add-release', async (_event, releaseData: ReleaseData) => | ||
| lensService?.addRelease(releaseData), | ||
| ); | ||
| ipcMain.handle('peerbit:edit-release', async (_event, releaseData: ReleaseData) => |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type annotation uses ReleaseData but the preload file imports EditInput<ReleaseData>. The main process should use the same type for consistency and proper type safety.
| ipcMain.handle('peerbit:edit-release', async (_event, releaseData: ReleaseData) => | |
| ipcMain.handle('peerbit:edit-release', async (_event, releaseData: EditInput<ReleaseData>) => |
| "dependencies": { | ||
| "@esbuild-plugins/node-globals-polyfill": "^0.2.3", | ||
| "@riffcc/lens-sdk": "^0.1.32", | ||
| "@riffcc/lens-sdk": "file:../lens-sdk", |
Copilot
AI
Aug 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a local file path dependency instead of a published version. This should be changed back to a proper version number before production deployment to ensure reproducible builds.
| "@riffcc/lens-sdk": "file:../lens-sdk", | |
| "@riffcc/lens-sdk": "^1.0.0", |
No description provided.