Add OPF sidecar metadata and hardlink file placement support - #220
Add OPF sidecar metadata and hardlink file placement support#220pharrside89 wants to merge 5 commits into
Conversation
…hardlinking Feature/opf metadata support and hardlinking
kikootwo
left a comment
kikootwo
left a comment
There was a problem hiding this comment.
Thanks for this. It's a feature I've wanted for a while and the overall shape is right: a generic placeFile utility, a standalone OPF writer, and defaults that preserve existing behavior. I ran the full test suite on your branch and everything passes.
Requesting changes on a few things before this can go in:
- There's a real state bug in the settings UI (inline comment on PathsTab). Two updatePath calls in one handler clobber each other, so the guardrail silently reverts your write mode selection.
- "Fail the import" doesn't actually fail the import in every case (inline comment in file-organizer).
- Repo conventions: new files need the header comment linking to documentation, the docs need updating (documentation/phase3/file-organization.md and TABLEOFCONTENTS.md should cover the new config keys and the RMAB_* env vars), and the new utils need tests. file-placement and opf-writer are both pure and easy to cover, and there are examples to crib from in tests/utils.
- The useRequests changes are mostly unrelated to this feature (inline comment).
Smaller stuff inline. Also some diff noise worth cleaning up: the metadata tagging toggle block got re-indented for no functional reason and now sits at the wrong depth relative to its siblings, the Plex Format Coercion comment is duplicated, and both new files are missing trailing newlines.
One more note for the docs: hardlinked files share an inode with the seeded download, so we intentionally skip chmod on them (otherwise we'd be changing the seed file's permissions too). That means the fileChmod setting is quietly ignored in hardlink mode, which is worth a sentence in the UI help text.
| updatePath('metadataWriteMode', nextMode); | ||
|
|
||
| if (paths.filePlacementMode === 'hardlink' && nextMode !== 'opf') { | ||
| updatePath('filePlacementMode', 'copy'); |
There was a problem hiding this comment.
This has a stale-state bug. updatePath builds its object from the paths prop captured at render, and the parent onChange is a plain setSettings. React batches these two calls, so the second one wins with an object that doesn't include the first change. Net effect: with OPF + hardlink selected, picking "Embedded audio tags" flips placement to copy but silently reverts the write mode. It only sticks the second time you select it.
Build one merged object and call onChange once (or add a variant of updatePath that accepts multiple fields).
| : filePlacementMode; | ||
|
|
||
| if (effectivePlacementMode === 'hardlink' && taggedFilePath) { | ||
| throw new Error('Hardlink mode is incompatible with embedded metadata tagging'); |
There was a problem hiding this comment.
This throw is unreachable: taggedFilePath forces effectivePlacementMode to 'copy' a few lines above, so the condition can never be true. Drop it, or move the check somewhere it can actually fire.
| throw new Error('Hardlink mode is incompatible with embedded metadata tagging'); | ||
| } | ||
|
|
||
| const placementResult = await placeFile(sourcePath, targetFilePath, { |
There was a problem hiding this comment.
"Fail the import" doesn't reliably fail the import. When placeFile throws here, the catch below pushes to result.errors and moves on to the next file, and success is just audioFiles.length > 0. The "already exists, skipping" path earlier also pushes into audioFiles, so on a retry after a partial run the import succeeds even though hardlinks are still failing.
Either make a placement failure in fail mode abort the organization, or relabel the setting to match what it actually does.
| ); | ||
| } | ||
|
|
||
| const effectiveFilePlacementMode = |
There was a problem hiding this comment.
Two issues in this area. First, the coercion is silent: the client submitted hardlink and we store copy without telling anyone. The UI already guards this combination, so I'd rather return a 400 here than quietly store something different from what was sent.
Second, the upserts below always write, with || 'embedded' / || 'copy' defaults. Any PUT that omits these fields resets the stored modes to defaults, and once those DB rows exist they permanently override the RMAB_* env vars (DB wins over env everywhere we read these). Only upsert when the field is actually present in the request.
| fileRenameTemplate?: string; | ||
| fileChmod?: string; | ||
| dirChmod?: string; | ||
| metadataWriteMode: 'embedded' | 'opf' | 'both' | 'disabled'; |
There was a problem hiding this comment.
'disabled' never appears in the API or stored config (the PUT validation rejects it). It's derived inside the organizer when tagging is off. Keep this union to the three real values and keep 'disabled' internal to file-organizer.
| import fs from 'fs/promises'; | ||
| import { copyFile } from './copy-file'; | ||
|
|
||
| export type FilePlacementMode = 'copy' | 'hardlink' | 'move'; |
There was a problem hiding this comment.
'move' isn't reachable from any setting, and fs.rename throws EXDEV across filesystems anyway. I'd drop it until something actually needs it.
| ]; | ||
|
|
||
| if (audiobook.narrator) { | ||
| lines.push(` <meta name="narrator" content="${escapeXml(audiobook.narrator)}"/>`); |
There was a problem hiding this comment.
Are you sure ABS picks this up? Audiobookshelf's OPF parser reads narrators from <dc:creator opf:role="nrt"> and ASIN from a dc:identifier with an ASIN scheme, and I don't believe it consumes or . Your validation may have covered title/author/series only. Worth emitting the role-based creator and the scheme-based identifier as well.
| useSWRInfinite<RequestPage>(getKey, fetcher, { | ||
| revalidateFirstPage: true, | ||
| revalidateOnFocus: false, | ||
| revalidateOnFocus: true, |
There was a problem hiding this comment.
This flips a deliberate earlier choice and changes request-list traffic for every user, and it isn't related to OPF or hardlinks. Adding 'downloaded' to the active statuses makes sense alongside this feature, but please pull the revalidateOnFocus change into its own PR so it can be discussed on its own.
Hey, thanks for getting to this. I'll take a look and update asap. BTW I've been running this for the last month or so and its been working great! |
2 participants
Summary
Adds configurable OPF sidecar metadata writing and hardlink-based file placement for organized audiobook imports.
This allows ReadMeABook to avoid duplicating large audiobook files when downloads and the audiobook library are on the same filesystem, while also writing
metadata.opfsidecar files that Audiobookshelf can consume during library scans.Closes: #68
Changes
RMAB_METADATA_WRITE_MODERMAB_FILE_PLACEMENT_MODERMAB_HARDLINK_FALLBACK_MODEdownloadedstatus, so the UI can update when items become available.Validation
Tested in Docker with a shared
/mediamount containing both/media/downloadsand/media/audiobooks.Validated hardlink behavior by comparing device, inode, and link count between the download file and organized library file.
Expected hardlink result:
2Validated OPF sidecar creation in the organized audiobook directory by confirming
metadata.opfwas created.Validated Audiobookshelf consumption of OPF metadata after library scan.
Validated the GUI guardrail:
OPF sidecar file+Hardlink filesis allowed.Embedded audio tags+Hardlink filesis not allowed.Embedded tags + OPF sidecar+Hardlink filesis not allowed.Notes
Hardlinking requires the download path and library path to be on the same filesystem.
For Docker deployments, this usually means mounting a shared parent path such as
/media, then using subdirectories like/media/downloadsand/media/audiobooks.Using separate Docker bind mounts for downloads and audiobooks may prevent hardlinking, even when both paths point to the same NAS or host filesystem.