Fixing naming pattern adding {Subtitle}#579
Conversation
Improve pre-commit and pre-push hooks: add Windows Node PATH workaround, switch pre-commit to run node scripts/lint-staged.mjs (instead of npm), and add enforcement checks for layering violations and banning `async void`. Update pre-push to sync frontend version from csproj, run dotnet format verification, and run frontend type-check and tests. Adjust scripts/lint-staged.mjs to invoke local eslint/prettier via process.execPath (avoid npx) so tooling runs reliably across platforms/CI.
Stop implicitly appending the audiobook subtitle to Title when naming files/folders and instead expose Subtitle as its own token. Frontend: add Subtitle and several other metadata tokens to displayBasePath replacement so patterns containing {Subtitle} render correctly. Backend: remove legacy Title+Subtitle concatenation, update BuildNamingVariables signature and RenameService call to use the raw Title and separate Subtitle. Tests: add unit tests covering title-only patterns for manual import and preview rename, and a frontend spec to assert the subtitle is replaced in the estimated base path. Files changed include AudiobookDetailView, RenameService, ManualImportController, and their corresponding tests.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef1c05a5a7
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR fixes file/folder naming pattern token behavior so configured patterns are applied literally: {Title} no longer implicitly includes the subtitle, and {Subtitle} must be explicitly included to appear. It also extends path-estimation token replacement in the audiobook detail view and adds regression coverage around these behaviors.
Changes:
- Backend: removed implicit title+subtitle merging in manual import path generation and organize/rename preview variables.
- Tests: added backend regression tests for manual import + rename preview using
{Title}when subtitles exist, plus a frontend test covering{Subtitle}replacement in the estimated path. - Tooling: adjusted staged lint execution and expanded Husky pre-commit/pre-push hook checks.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Features/Api/Services/RenameServiceTests.cs | Adds regression test ensuring rename preview {Title} does not append subtitle. |
| tests/Features/Api/Controllers/ManualImportControllerTests.cs | Adds regression tests ensuring manual import {Title} patterns don’t append subtitle (single + multi-file). |
| scripts/lint-staged.mjs | Runs ESLint/Prettier via the current Node executable instead of npx. |
| listenarr.api/Services/RenameService.cs | Removes implicit title/subtitle concatenation from rename preview variable building. |
| listenarr.api/Controllers/ManualImportController.cs | Removes implicit title/subtitle concatenation from manual import naming variables. |
| fe/src/views/library/AudiobookDetailView.vue | Expands token replacement for estimated paths to include additional backend-supported tokens. |
| fe/src/tests/AudiobookDetailView.spec.ts | Adds unit test asserting {Subtitle} replacement appears in the estimated path. |
| .husky/pre-push | Replaces npm test with version sync, dotnet format verify, FE typecheck, and FE tests. |
| .husky/pre-commit | Replaces npm run lint:staged with direct node script and adds repo enforcement greps. |
Comments suppressed due to low confidence (1)
scripts/lint-staged.mjs:80
lint-staged.mjsswitched to running ESLint/Prettier viaprocess.execPath(good for reduced PATH environments), but the Vue template handler check still hardcodesrun('node', ...). In the same reduced-PATH scenario on Windows, this can fail even though the other commands are now robust. Use the samenodeCommandfor the Vue handler check as well (or otherwise resolve Node consistently).
run(nodeCommand, ['node_modules/eslint/bin/eslint.js', ...frontendLintFiles], { cwd: 'fe' })
}
if (frontendVueFiles.length > 0) {
console.log('Checking staged Vue template handlers...')
run('node', ['scripts/check-vue-template-handlers.mjs', ...frontendVueFiles], { cwd: 'fe' })
}
Refactor folder/file naming pattern handling to properly handle optional/empty tokens and normalize paths. Split folderNamingPattern and fileNamingPattern selection, introduce replacePatternVariable, sanitizeOptionalPathComponent and cleanupEmptyPatternVariables to replace variables with placeholders, remove empty tokens and surrounding separators, and normalize slashes. Ensure folder pattern returns the full combined path while file patterns continue to return the parent directory. Updated tests to assert exact expected paths and add a new test to verify empty optional tokens (e.g. {Subtitle}) are removed from the estimated base path.
therobbiedavis
added
the
patch
Replace inline shell logic in .husky/pre-commit and .husky/pre-push with a resolve_node helper and invoke scripts/run-hook-check.mjs (pre-commit/pre-push). Add a new scripts/check-architecture.mjs to encapsulate grafted git-grep enforcement checks (layering and async void) and exit non‑zero on violations. Add related npm scripts (version:check, type-check:frontend, test:frontend:hook, test:arch, check:pre-commit, check:pre-push) to package.json to expose the new checks and frontend tooling. These changes centralize hook behavior in Node-based scripts and improve cross-platform node resolution.
Centralize folder/file naming and path preview logic on the backend and update the frontend to consume the preview. Introduces IAudiobookPathPreviewService/INamingPatternService and registers AudiobookPathPreviewService/NamingPatternService; LibraryController now delegates base-path computation to the path preview service and returns a PreviewPathResponse. FileNamingService no longer contains pattern parsing/sanitization internals and delegates to INamingPatternService. Frontend AudiobookDetailView now requests the backend preview for displayBasePath (removes local pattern expansion) and handles preview updates; tests and builders updated to use new builders and test infrastructure, and mocks adjusted accordingly. Also removes obsolete controller base-path unit tests and updates various API controller tests to align with the new services.
T4g1
left a comment
T4g1
left a comment
There was a problem hiding this comment.
I believe we can improve the separation of concerns and thus avoid code duplication even further here
| @@ -161,174 +156,15 @@ public async Task<string> GenerateFilePathAsync( | |||
| /// </summary> | |||
| public string ApplyNamingPattern(string pattern, Dictionary<string, object> variables, bool treatAsFilename = false) | |||
There was a problem hiding this comment.
We probably want to drop this method as it's no longer the responsability of this file
There was a problem hiding this comment.
i'd say this should go in Naming directory too but this will probably conflict with the PR that takes care of that then :x
There was a problem hiding this comment.
As this will be merged after the file re-structuration, i leave this open as a reminder to make sure we no longer have a Services directory once this has been rebased
| { | ||
| Task<PathPreviewResult> PreviewAsync( | ||
| Audiobook audiobook, | ||
| string? destinationRoot = null, |
There was a problem hiding this comment.
I find this a bit confusing, I assume this is the prefered root folder the user wants for this audiobook ? Maybe add a comment or explicitely name it selectedRootFolder ? Also, why do we allow null values ? To create a base path for an audiobook, we absolutely need a root folder right ? The controller should probably safeguard that so user "trust" component stays in API layer
There was a problem hiding this comment.
Also, we could directly use the RootFolder object if controller does the preliminary checks
| : namingPatternService.SanitizePathComponent(value); | ||
| } | ||
|
|
||
| private static string BuildDirectoryPattern(string fileNamingPattern, Audiobook audiobook) |
There was a problem hiding this comment.
This feels strange to have here, i expect the responsibility of AudioobookPathPreviewService to be limited to building a path from a pre-configured pattern.
This method should probably be used while configuring those patterns instead to validate them eventually (I haven't look at that part of the code yet but I expect that to be in the IConfigurationService probably)
| return directoryPattern; | ||
| } | ||
|
|
||
| private static string ResolvePathWithOptionalBase(string? basePath, string candidatePath) |
There was a problem hiding this comment.
I would put that in FileUtils or something as this is static
Also, why not put basePath as an optional argument instead of a mandatory null when we do not use it ? (method name should then be updated as ResolvePath)
Move and split naming pattern functionality into a Common implementation and a new INamingPatternService interface under Application.Interfaces, adding ApplyAudiobookNamingPattern and audiobook-specific variable building. Update consumers (FileNamingService, AudiobookPathPreviewService, AppServiceRegistrationExtensions, LibraryController) to depend on the new interface and adjust sanitization/variable handling so empty folder patterns return the selected root (no synthesized "Unknown" segments) and file naming patterns do not affect base directories. Normalize folder pattern processing and simplify path preview computation. Add resolve-node.sh and source it from husky pre-commit/pre-push hooks. Add and adjust tests covering path preview, naming behavior, and a test builder fix (QualityProfileBuilder IdCounter).
Summary
Fixes #571 by making configured naming patterns apply exactly as written.
{Title}now resolves to only the audiobook title instead of implicitly appending the subtitle, while{Subtitle}remains available when users explicitly include it.Changes
Added
{Title}with audiobook subtitles present.{Subtitle}replacement in the audiobook detail path estimate.Changed
{Subtitle},{Edition},{Narrator},{Publisher},{Language},{Asin}, and{Quality}.Fixed
Testing
dotnet test tests/Listenarr.Tests.csproj --filter "FullyQualifiedName~ManualImportControllerTests|FullyQualifiedName~RenameServiceTests|FullyQualifiedName~FileNamingService_PatternSelectionTests" /p:DefaultItemExcludes=**/obj/**npm run test:unit -- --run src/__tests__/AudiobookDetailView.spec.tsNotes
{Subtitle}was already listed in the File Management settings help. This change makes the backend and detail-view preview honor that token explicitly without changing{Title}output.