Fix music-metadata-browser import resolution and lazy-load parser for build stability#12
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces a brittle static import of music-metadata-browser’s parseBlob with a lazy dynamic loader and slightly enhances metadata collection to include the filename, while preserving existing metadata hardening and MP3 writing behavior. Sequence diagram for lazy loading music_metadata_browser parseBlobsequenceDiagram
actor User
participant App
participant MetadataUtils
participant MusicMetadataBrowser
User->>App: selectFile(file)
App->>MetadataUtils: readFileMetadata(file)
activate MetadataUtils
MetadataUtils->>MetadataUtils: getParseBlob()
alt parseBlobLoader not initialized
MetadataUtils->>MusicMetadataBrowser: import(music-metadata-browser)
activate MusicMetadataBrowser
MusicMetadataBrowser-->>MetadataUtils: module
deactivate MusicMetadataBrowser
MetadataUtils->>MetadataUtils: resolve module.parseBlob or module.default.parseBlob
MetadataUtils-->>MetadataUtils: cache Promise in parseBlobLoader
else parseBlobLoader already set
MetadataUtils-->>MetadataUtils: reuse cached parseBlobLoader
end
MetadataUtils-->>MetadataUtils: await parseBlob
MetadataUtils->>MusicMetadataBrowser: parseBlob(file)
MusicMetadataBrowser-->>MetadataUtils: metadata or error
MetadataUtils->>MetadataUtils: collectStrings(metadata, file.name)
MetadataUtils-->>App: { format, detectedMarkers, parseError }
deactivate MetadataUtils
App-->>User: show metadata and AI marker detection result
Class diagram for updated metadata utilities moduleclassDiagram
class MetadataUtils {
+getParseBlob() Promise
+escapeRegex(value) string
+markerToRegex(marker) RegExp
+collectStrings(metadata, fileName) string
+readFileMetadata(file) Promise
}
class parseBlobLoaderHolder {
-parseBlobLoader Promise
}
MetadataUtils --> parseBlobLoaderHolder : uses
class MusicMetadataBrowserModule {
+parseBlob(file) Promise
}
MetadataUtils ..> MusicMetadataBrowserModule : dynamic_import
class readFileMetadataDetails {
-parsed any
-parseError Error
+AI_MARKERS string[]
+MARKER_REGEX_CACHE Map
}
MetadataUtils --> readFileMetadataDetails : maintains_state
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
readFileMetadata,parseErroris captured but never used, so consider either removing it or surfacing it (e.g., via logging or return shape) to avoid dead code and make error handling intent clearer. - The
getParseBlobloader caches the promise, which means a transient import failure will permanently poison future calls; consider resettingparseBlobLoaderon rejection or adding a retry path so the app can recover from one-off load errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `readFileMetadata`, `parseError` is captured but never used, so consider either removing it or surfacing it (e.g., via logging or return shape) to avoid dead code and make error handling intent clearer.
- The `getParseBlob` loader caches the promise, which means a transient import failure will permanently poison future calls; consider resetting `parseBlobLoader` on rejection or adding a retry path so the app can recover from one-off load errors.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc05817267
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (parseBlobLoader) return parseBlobLoader; | ||
| parseBlobLoader = import('music-metadata-browser').then((mod) => { |
There was a problem hiding this comment.
Clear cached loader when dynamic import rejects
If import('music-metadata-browser') fails once (for example a transient chunk/network load error), parseBlobLoader is left pointing at a rejected promise and every later readFileMetadata() call will immediately fail in the same session, even after conditions recover. This regresses resilience because the code never retries module resolution unless the page is reloaded; resetting the cache on rejection would allow a subsequent attempt to succeed.
Useful? React with 👍 / 👎.
a7c14bb
into
codex/harden-readfilemetadata-function
Motivation
music-metadata-browserexport shape or bundling behavior is different between environments, so the staticimport { parseBlob }is brittle and caused unresolved-import build failures.Description
parseBlobimport with a lazygetParseBlob()that usesimport('music-metadata-browser')and resolvesmod.parseBlob || mod.default.parseBlob, throwing if the export is not found to fail fast.readFileMetadata()to use the lazy loader, to catch parse failures, and to include the filename in the metadata searchable content while keeping boundary-aware marker detection and short-marker protection intact.writeMP3Metadata()behavior intact while keeping text sanitization, frame writes for title/artist/genre, returning anaudio/mpegBlob, and not triggering auto-downloads.Testing
npm installand the install completed successfully withmusic-metadata-browser@2.5.11present inpackage.jsonandpackage-lock.json.npm run buildand the build completed successfully, producing the application chunks and static assets without unresolved-import errors.music-metadata-browserusing a Node check and confirmed the version2.5.11was present.Codex Task
Summary by Sourcery
Lazy-load music-metadata-browser’s parseBlob to improve build robustness and extend metadata search to include filenames when detecting AI markers.
Bug Fixes:
Enhancements: