Add Maintenance menu, track name fixer #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR adds a track name fixer feature that extracts metadata from ID3 tags and updates track names in music releases. It also restructures the CLI menu to include a new "Maintenance" submenu that organizes administrative tasks.
- Adds functionality to read ID3 tags from audio files and update track metadata in releases
- Reorganizes CLI menu by consolidating maintenance tasks under a dedicated "Maintenance" submenu
- Adds new dependencies for ID3 tag reading and HTML parsing
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/cli/commands/updateTrackNames.ts | New command that fetches music releases, reads ID3 tags from IPFS audio files, and updates track metadata with user confirmation |
| src/cli/commands/run.ts | Refactors main menu to include "Maintenance" submenu and integrates the new track name update feature |
| package.json | Adds dependencies for jsmediatags, node-html-parser, and their type definitions |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| import { LensService } from '@riffcc/lens-sdk'; | ||
| import { logger, logError } from '../logger.js'; | ||
| import { input, select } from '@inquirer/prompts'; | ||
| // @ts-ignore |
There was a problem hiding this comment.
Using @ts-ignore suppresses TypeScript errors without addressing the underlying issue. Consider using proper type declarations or importing with proper types instead.
| logger.info(`\nProcessing: ${release.name}`); | ||
|
|
||
| // Parse existing metadata | ||
| let metadata: any = {}; |
There was a problem hiding this comment.
Using 'any' type reduces type safety. Consider defining a proper interface for the metadata structure.
Suggested change
| let metadata: any = {}; | |
| let metadata: ReleaseMetadata = {}; |
| const release = musicReleases.find(r => r.id === releaseId)!; | ||
|
|
||
| // Get current track metadata | ||
| let metadata: any = {}; |
There was a problem hiding this comment.
Using 'any' type reduces type safety. Consider defining a proper interface for the metadata structure.
Suggested change
| let metadata: any = {}; | |
| let metadata: ReleaseMetadata = {}; |
| throw new Error(`Release ${releaseId} not found`); | ||
| } | ||
|
|
||
| let metadata: any = {}; |
There was a problem hiding this comment.
Using 'any' type reduces type safety. Consider defining a proper interface for the metadata structure.
Suggested change
| let metadata: any = {}; | |
| let metadata: ReleaseMetadata = {}; |
|
|
||
| // Find all links to audio files | ||
| const links = root.querySelectorAll('a'); | ||
| links.forEach((link: any) => { |
There was a problem hiding this comment.
Using 'any' type reduces type safety. Consider using the proper type from node-html-parser for the link element.
Suggested change
| links.forEach((link: any) => { | |
| const { parse, HTMLElement } = await import('node-html-parser'); | |
| const root = parse(responseText); | |
| // Find all links to audio files | |
| const links = root.querySelectorAll('a'); | |
| links.forEach((link: HTMLElement) => { |
| async function readID3Tags(url: string): Promise<{ title?: string; artist?: string }> { | ||
| return new Promise((resolve) => { | ||
| jsmediatags.read(url, { | ||
| onSuccess: (tag: any) => { |
There was a problem hiding this comment.
Using 'any' type reduces type safety. Consider defining a proper interface for the jsmediatags tag structure.
Suggested change
| onSuccess: (tag: any) => { | |
| interface JsMediaTagsResult { | |
| tags: { | |
| title?: string; | |
| artist?: string; | |
| [key: string]: unknown; | |
| }; | |
| [key: string]: unknown; | |
| } | |
| async function readID3Tags(url: string): Promise<{ title?: string; artist?: string }> { | |
| return new Promise((resolve) => { | |
| jsmediatags.read(url, { | |
| onSuccess: (tag: JsMediaTagsResult) => { |
|
|
||
| async function fetchIPFSTracks(contentCID: string): Promise<{ filename: string; url: string; cid: string }[]> { | ||
| const tracks: { filename: string; url: string; cid: string }[] = []; | ||
| const ipfsGateway = process.env.IPFS_GATEWAY || 'http://localhost:8080'; |
There was a problem hiding this comment.
The hardcoded default IPFS gateway URL 'http://localhost:8080' is a magic value. Consider defining this as a named constant.
Suggested change
| const ipfsGateway = process.env.IPFS_GATEWAY || 'http://localhost:8080'; | |
| const ipfsGateway = process.env.IPFS_GATEWAY || DEFAULT_IPFS_GATEWAY; |
| const href = link.getAttribute('href'); | ||
| if (href && href.includes('/ipfs/')) { | ||
| const filename = link.innerText; | ||
| if (filename && ['mp3', 'flac', 'ogg', 'm4a', 'wav'].some(ext => filename.toLowerCase().endsWith('.' + ext))) { |
There was a problem hiding this comment.
The audio file extensions array is hardcoded. Consider defining this as a named constant for better maintainability.
Suggested change
| if (filename && ['mp3', 'flac', 'ogg', 'm4a', 'wav'].some(ext => filename.toLowerCase().endsWith('.' + ext))) { | |
| if (filename && AUDIO_FILE_EXTENSIONS.some(ext => filename.toLowerCase().endsWith('.' + ext))) { |
Critical bug fix: The relay parameter in Peerbit.create() was using argv.relay (which enables running AS a relay server) instead of argv.useRelays (which enables using circuit relay for NAT traversal as a client). This caused --light --onlyReplicate nodes to fail to connect through relays because relay client mode was never enabled. Also clarified that --light only affects connectivity (no listening), not replication strategy. Light nodes now use full replication (factor: 1) by default instead of dynamic replication.
Restored getSyncDetails() and getPeerCount() calls that were temporarily disabled. These are needed to verify that the node has actually replicated content, not just that it's alive.
Alpine was causing mysterious Peerbit store loading failures that don't occur on Debian-based systems. Changed from node:22-alpine to node:22-trixie-slim to match the host OS environment more closely. Note: Docker still has issues with 'Failed to load store' that don't occur when running the same code locally. This is under investigation.
- Change base image from Alpine to Debian Trixie to match host environment - Install build dependencies (python3, make, g++) needed for native modules - Add explicit --allow-build flags for native modules that need compilation: - @ipshipyard/node-datachannel - better-sqlite3 - classic-level - protobufjs Previously, pnpm was silently skipping build scripts for these native modules, causing 'Failed to load store' errors when the container tried to use them. The --allow-build flags explicitly whitelist each package's build scripts.
…ative modules in Dockerfile - Document that local build works reliably with the test site - Add instructions about environment differences to check when Docker fails - Update Dockerfile to explicitly rebuild classic-level and better-sqlite3 from source - These modules use prebuilts by default which may not be compatible
Changed from global npm install to local build approach: - Copy package.json and pnpm-lock.yaml - Run pnpm install --frozen-lockfile for deterministic builds - Copy source and run pnpm build - Execute with node dist/cli/bin.js This ensures Docker uses the exact same dependency versions and build process as local development, fixing "Failed to load store" errors that occurred with published npm packages. Tested working on production (Bunny). 🚀 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Includes retryDelay: 5000ms (prevents health check death spiral) - Includes seekTimeout: 5000ms (faster dead node cleanup) - Critical fix for peer table pollution and SeekDelivery failures
- Announce on /dns4//tcp/443/wss for browser connections - Announce on /dns4//tcp/ for direct P2P - Fixes Mixed Content errors where browsers couldn't connect to ws:// endpoints - Enables proper WSS connectivity when --domain flag is provided
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.