feat: add configurable channel folder template for storage paths#1085
Merged
Zibbp merged 19 commits intoZibbp:mainfrom Mar 10, 2026
Merged
feat: add configurable channel folder template for storage paths#1085Zibbp merged 19 commits intoZibbp:mainfrom
Zibbp merged 19 commits intoZibbp:mainfrom
Conversation
Add ChannelFolderTemplate field to StorageTemplate struct with a default
of {{channel}} to preserve backward compatibility. This is the foundation
for allowing users to customize channel-level directory naming.
Closes Zibbp#1019
…utils Extract GetChannelFolderName and ChannelTemplateInput into a new storagetemplate package to avoid circular dependencies between archive and tasks. Update archive/utils.go to delegate to the shared package and add channel_id/channel_display_name to StorageTemplateInput and the variable map for folder/file templates.
Replace hardcoded channel.Name/platformChannel.Login with resolved channel folder name from template in ArchiveChannel, ArchiveVideo, ArchiveClip, and ArchiveLivestream. Also pass channel_id and channel_display_name to StorageTemplateInput for folder/file templates.
Update CreateDirectory, SaveVideoInfo, sprite thumbnail generation, and periodic channel update tasks to resolve the channel folder name from the template instead of using the channel login name directly.
Resolve the channel folder name from the template when checking for and downloading channel profile images.
Enhance StorageMigration with a two-phase approach: Phase 1 renames channel-level directories and updates profile image paths when the channel folder template changes. Phase 2 migrates VOD subfolders and files using the resolved channel folder name. Old empty channel directories are cleaned up after migration.
Detect the old VIDEOS_DIR from channel image paths instead of relying on channel.Name as a path delimiter. This is more robust when channel folders use custom templates like channel_id. Falls back to the legacy approach if no channel image paths are found.
…riables Add TestGetChannelFolderName with cases for default, channel_id, display_name, mixed, and invalid variable templates. Extend existing folder/file template tests with channel_id and channel_display_name fields and test cases.
Add channel_folder_template to the StorageTemplate TypeScript interface.
Add a Textarea input for the channel folder template in the storage
template settings section with a prefilled default of {{channel}}.
Consolidate all template variables into a single reference section
indicating which variables are available for which templates.
Sanitize the resolved channel folder name through SanitizeFileName to strip path separators, dot-dot sequences, and other unsafe filesystem characters. Also sanitize channel_display_name in the folder/file template variable map. This prevents directory traversal attacks via malicious template values or channel display names.
- Add missing YYYY/MM/DD/HH fields to StorageTemplateInput so migration produces correct paths for templates using date-component variables. - Add rollback for Phase 1 channel folder migration: if the DB update fails after moving the profile image, the file is moved back. - Add nil check on video.Edges.Channel to guard against orphaned videos causing a nil pointer dereference during migration.
Add test cases verifying that path traversal in template literals (../../etc) and path separators (foo/bar) are sanitized. Add test for display names containing path traversal characters.
Fix two failing test cases: SanitizeFileName strips path separators (/ and \) but preserves dots, so '../../etc' becomes '.._.._etc' — this is safe because traversal requires path separators. Update test expectations to match actual SanitizeFileName behavior. Also pre-sanitize channel_display_name in getChannelVariableMap for consistency with the VOD-level getVariableMap (defense-in-depth on top of the existing post-sanitization of the full resolved name).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds configurable channel-folder templating: new config field and frontend setting, a dedicated storagetemplate package to render/sanitize channel folder names, propagation of channel metadata, and widespread updates (paths, archive, migration, tasks, thumbnails, and tests) to use template-resolved channel folders with fallbacks. Changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/app/admin/settings/page.tsx (1)
422-431: Channel Folder Template UI looks good.The new Textarea with
requiredvalidation and helpful description text is well-integrated into the existing settings layout.Minor observation: the title "Channel Folder Template" (line 423) and description (line 426) are hardcoded English strings rather than using the
t()translation function. This is consistent with other similar strings in this file (e.g., "Available Variables" at line 459), but worth noting if i18n coverage is a goal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/admin/settings/page.tsx` around lines 422 - 431, Replace the hardcoded English strings for the title and description with i18n keys using the t() translation function: update the Title caption "Channel Folder Template" and the Textarea description text ("Controls the top-level channel directory name...") to use t('...') calls, keeping the same meaning and leaving the Textarea props (key via form.key('storage_templates.channel_folder_template') and form.getInputProps('storage_templates.channel_folder_template')) unchanged so only the display strings are translated.internal/database/migrate.go (1)
16-31: Good robustness improvement for custom templates.The two-level
filepath.Dirapproach to derive the videos directory from channel image paths is a solid strategy that works regardless of how the channel folder is named. The fallback to legacy video-path-based detection preserves backward compatibility.One edge case to be aware of: if an
ImagePathwere ever a bare filename (no directory components),filepath.Dir(filepath.Dir("profile.png"))would yield".", which would pass the non-empty check on line 49 and trigger an erroneous migration. This is unlikely given the path construction elsewhere always uses absolute paths, but a defensive check could help.🛡️ Optional defensive check
for _, c := range channels { if c.ImagePath != "" { // {VIDEOS_DIR}/{channel_folder}/profile.png -> {VIDEOS_DIR}/{channel_folder} -> {VIDEOS_DIR} - oldVideoPath = filepath.Dir(filepath.Dir(c.ImagePath)) - break + candidate := filepath.Dir(filepath.Dir(c.ImagePath)) + if filepath.IsAbs(candidate) { + oldVideoPath = candidate + break + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/database/migrate.go` around lines 16 - 31, When deriving oldVideoPath in the loop over channels (variable oldVideoPath and the code using filepath.Dir(filepath.Dir(c.ImagePath))) add a defensive check so we only accept a computed path that is not "." (and optionally is absolute or contains a path separator). Concretely, after computing candidate := filepath.Dir(filepath.Dir(c.ImagePath)), skip it unless candidate != "." (and/or filepath.IsAbs(c.ImagePath) or strings.Contains(c.ImagePath, string(os.PathSeparator))). This prevents bare filenames like "profile.png" producing "." and triggering an incorrect migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/storagetemplate/storagetemplate.go`:
- Around line 27-57: GetChannelFolderName currently replaces template variables
even when their resolved values are empty which, after utils.SanitizeFileName,
can create the generic "unnamed_file" collision; update the loop that iterates
templateVariableRegex matches to check the resolved valueString (from
variableMap[variableName] / fmt.Sprintf) and if valueString == "" return a
descriptive error (e.g., "variable X resolved to empty string") instead of
performing strings.ReplaceAll, so callers can handle missing data; keep the
later SanitizeFileName call and the existing empty-after-sanitize check but
ensure the early per-variable emptiness guard prevents silent fallback to
unnamed_file.
In `@internal/tasks/thumbnail.go`:
- Around line 133-142: The code rebuilds rootVideoPath from the current channel
template (using storagetemplate.GetChannelFolderName and channelFolderName)
which will break if channel_folder_template changes after archiving; instead
derive the video folder from the stored video.VideoPath (use
filepath.Dir(video.VideoPath) to get the folder that actually contains the
archived file) and set rootVideoPath to that; if video.VideoPath is empty or
invalid, fall back to the existing template-based construction so functions
referencing rootVideoPath (e.g., where sprites/thumbnails are written) continue
to work.
---
Nitpick comments:
In `@frontend/app/admin/settings/page.tsx`:
- Around line 422-431: Replace the hardcoded English strings for the title and
description with i18n keys using the t() translation function: update the Title
caption "Channel Folder Template" and the Textarea description text ("Controls
the top-level channel directory name...") to use t('...') calls, keeping the
same meaning and leaving the Textarea props (key via
form.key('storage_templates.channel_folder_template') and
form.getInputProps('storage_templates.channel_folder_template')) unchanged so
only the display strings are translated.
In `@internal/database/migrate.go`:
- Around line 16-31: When deriving oldVideoPath in the loop over channels
(variable oldVideoPath and the code using
filepath.Dir(filepath.Dir(c.ImagePath))) add a defensive check so we only accept
a computed path that is not "." (and optionally is absolute or contains a path
separator). Concretely, after computing candidate :=
filepath.Dir(filepath.Dir(c.ImagePath)), skip it unless candidate != "." (and/or
filepath.IsAbs(c.ImagePath) or strings.Contains(c.ImagePath,
string(os.PathSeparator))). This prevents bare filenames like "profile.png"
producing "." and triggering an incorrect migration.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
frontend/app/admin/settings/page.tsxfrontend/app/hooks/useConfig.tsinternal/archive/archive.gointernal/archive/utils.gointernal/archive/utils_test.gointernal/channel/channel.gointernal/config/config.gointernal/database/migrate.gointernal/storagetemplate/storagetemplate.gointernal/task/task.gointernal/tasks/common.gointernal/tasks/periodic/periodic.gointernal/tasks/thumbnail.go
… collisions
If a template variable like {{channel_id}} resolves to an empty string
(e.g. channel has no ExtID), return a descriptive error instead of
silently substituting and letting SanitizeFileName produce the generic
'unnamed_file' — which would cause all affected channels to collide
into the same directory.
Use filepath.Dir(video.ThumbnailPath) to determine rootVideoPath instead of re-resolving the channel folder template. This prevents writing sprites to the wrong directory when channel_folder_template was changed after the video was originally archived. Falls back to template-based construction only when ThumbnailPath is empty.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/tasks/thumbnail.go (1)
118-122:channelis only needed in the fallback branch — consider deferring the fetch.
channelis exclusively used inside theelseblock (lines 143–147). As-is, every sprite-generation job incurs thevideoChannel.Only(ctx)round-trip even whenvideo.ThumbnailPathis set and the fallback is never entered.♻️ Proposed refactor to defer the channel fetch
- videoChannel := video.QueryChannel() - channel, err := videoChannel.Only(ctx) - if err != nil { - return err - } - logger.Info().Str("video_id", video.ID.String()).Msg("generating sprite thumbnails for video") env := config.GetEnvConfig() // ...temp dir creation... var rootVideoPath string if video.ThumbnailPath != "" { rootVideoPath = filepath.Dir(video.ThumbnailPath) } else { + channel, err := video.QueryChannel().Only(ctx) + if err != nil { + return err + } channelFolderName, chErr := storagetemplate.GetChannelFolderName(storagetemplate.ChannelTemplateInput{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/thumbnail.go` around lines 118 - 122, The current code eagerly calls videoChannel.Only(ctx) and assigns channel even when video.ThumbnailPath is present; move the videoChannel.Only(ctx) call (and its error handling) into the fallback branch where channel is actually used (the else block that runs when video.ThumbnailPath is empty), so you only perform the round-trip when needed; keep the same variable name (channel) and preserve the existing error return behavior inside that branch.internal/storagetemplate/storagetemplate.go (1)
65-73: Store rawchannel_display_nameto prevent silent collisions when sanitization produces sentinel values.If a channel's display name is all emojis or other fully-stripped characters,
SanitizeFileNamereturns"unnamed_file"(a non-empty fallback). The empty-value check inGetChannelFolderName(line 48) operates on this post-sanitization value, so it never catches the collision: multiple channels with emoji-only display names would all resolve to the same folder name.Move sanitization to the caller—store the raw value in the map and let the shared empty check plus final sanitization in
GetChannelFolderNamehandle it, consistent with howchannelandchannel_idare handled:Proposed fix
func getChannelVariableMap(input ChannelTemplateInput) map[string]interface{} { - safeDisplayName := utils.SanitizeFileName(input.ChannelDisplayName) - return map[string]interface{}{ "channel": input.ChannelName, "channel_id": input.ChannelID, - "channel_display_name": safeDisplayName, + "channel_display_name": input.ChannelDisplayName, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storagetemplate/storagetemplate.go` around lines 65 - 73, getChannelVariableMap currently stores a sanitized display name which can hide collisions when SanitizeFileName returns a sentinel like "unnamed_file"; change getChannelVariableMap to return the raw input.ChannelDisplayName (remove the call to utils.SanitizeFileName and the safeDisplayName variable) so the map contains the original value, and rely on GetChannelFolderName to perform the empty-value check and final sanitization using utils.SanitizeFileName; update any callers or tests that expect the sanitized key from getChannelVariableMap accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/storagetemplate/storagetemplate.go`:
- Around line 65-73: getChannelVariableMap currently stores a sanitized display
name which can hide collisions when SanitizeFileName returns a sentinel like
"unnamed_file"; change getChannelVariableMap to return the raw
input.ChannelDisplayName (remove the call to utils.SanitizeFileName and the
safeDisplayName variable) so the map contains the original value, and rely on
GetChannelFolderName to perform the empty-value check and final sanitization
using utils.SanitizeFileName; update any callers or tests that expect the
sanitized key from getChannelVariableMap accordingly.
In `@internal/tasks/thumbnail.go`:
- Around line 118-122: The current code eagerly calls videoChannel.Only(ctx) and
assigns channel even when video.ThumbnailPath is present; move the
videoChannel.Only(ctx) call (and its error handling) into the fallback branch
where channel is actually used (the else block that runs when
video.ThumbnailPath is empty), so you only perform the round-trip when needed;
keep the same variable name (channel) and preserve the existing error return
behavior inside that branch.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/archive/utils_test.gointernal/storagetemplate/storagetemplate.gointernal/tasks/thumbnail.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/archive/utils_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/archive/utils.go (1)
56-71:⚠️ Potential issue | 🟠 Major
GetFolderNameandGetFileNamelack the output sanitization and empty-result guard added toGetChannelFolderName.The PR objective states "Adds path sanitization for all template output to prevent directory traversal", but only
storagetemplate.GetChannelFolderName(called on line 113) sanitizes its final output.GetFolderNameandGetFileNamereturn the raw substituted string. Concretely:
- The template literal itself (read from config) can contain
../or/, which survives unmodified.- Unsanitized variables like
date,type, andidthat are inserted verbatim could inject path separators if their values are unexpectedly formatted (e.g.,date = "2024/01/15").- Both functions can return
("", nil)if all substituted values happen to be empty, producing silent empty-path components downstream.
storagetemplate.GetChannelFolderNameguards against all three; these two functions should be brought into parity.🛡️ Proposed fix – sanitize and validate the resolved output
func GetFolderName(uuid uuid.UUID, input StorageTemplateInput) (string, error) { ... for _, match := range res { ... folderTemplate = strings.ReplaceAll(folderTemplate, match[0], folderString) } + folderTemplate = utils.SanitizeFileName(folderTemplate) + if folderTemplate == "" { + return "", fmt.Errorf("resolved folder name is empty after sanitization") + } return folderTemplate, nil } func GetFileName(uuid uuid.UUID, input StorageTemplateInput) (string, error) { ... for _, match := range res { ... fileTemplate = strings.ReplaceAll(fileTemplate, match[0], fileString) } + fileTemplate = utils.SanitizeFileName(fileTemplate) + if fileTemplate == "" { + return "", fmt.Errorf("resolved file name is empty after sanitization") + } return fileTemplate, nil }Also applies to: 90-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/archive/utils.go` around lines 56 - 71, GetFolderName and GetFileName currently return the raw substituted template (folderTemplate/fileTemplate) and can produce unsafe or empty paths; update both functions to run the final resolved string through the same path sanitization used by GetChannelFolderName (sanitize the substituted folderTemplate/fileTemplate after all replacements) and return a non-nil error if the sanitized result is empty. Ensure you still use storageTemplateVariableRegex and variableMap for substitution, replace variables first, then call the shared sanitizer used by GetChannelFolderName, and add an error like "sanitized path is empty" when the sanitizer yields an empty string.
🧹 Nitpick comments (1)
internal/archive/utils.go (1)
119-136:channel_idis not sanitized – acceptable for Twitch (numeric), but a potential gap for future platforms.
titleandchannel_display_nameare explicitly sanitized, creating an implicit contract that user-controlled values are safe.channel_idis currently Twitch-numeric, so safe in practice, but if the platform scope ever widens a caller could pass an arbitrary string here without sanitization.♻️ Proposed defensive sanitization
func getVariableMap(uuid uuid.UUID, input StorageTemplateInput) (map[string]interface{}, error) { safeTitle := utils.SanitizeFileName(input.Title) safeDisplayName := utils.SanitizeFileName(input.ChannelDisplayName) + safeChannelID := utils.SanitizeFileName(input.ChannelID) variables := map[string]interface{}{ "uuid": uuid.String(), "id": input.ID, "channel": input.Channel, - "channel_id": input.ChannelID, + "channel_id": safeChannelID, "channel_display_name": safeDisplayName, ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/archive/utils.go` around lines 119 - 136, The map built in getVariableMap currently inserts input.ChannelID as "channel_id" without sanitization; to defensively prevent future platform changes from allowing unsafe values, sanitize input.ChannelID (e.g., via utils.SanitizeFileName) before adding it to the variables map so "channel_id" stores the cleaned value; update getVariableMap to compute a safeChannelID and use that when setting the "channel_id" entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/archive/utils.go`:
- Around line 56-71: GetFolderName and GetFileName currently return the raw
substituted template (folderTemplate/fileTemplate) and can produce unsafe or
empty paths; update both functions to run the final resolved string through the
same path sanitization used by GetChannelFolderName (sanitize the substituted
folderTemplate/fileTemplate after all replacements) and return a non-nil error
if the sanitized result is empty. Ensure you still use
storageTemplateVariableRegex and variableMap for substitution, replace variables
first, then call the shared sanitizer used by GetChannelFolderName, and add an
error like "sanitized path is empty" when the sanitizer yields an empty string.
---
Nitpick comments:
In `@internal/archive/utils.go`:
- Around line 119-136: The map built in getVariableMap currently inserts
input.ChannelID as "channel_id" without sanitization; to defensively prevent
future platform changes from allowing unsafe values, sanitize input.ChannelID
(e.g., via utils.SanitizeFileName) before adding it to the variables map so
"channel_id" stores the cleaned value; update getVariableMap to compute a
safeChannelID and use that when setting the "channel_id" entry.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/archive/utils.gointernal/storagetemplate/storagetemplate.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/storagetemplate/storagetemplate.go
…nsively sanitize channel_id Add SanitizeFileName + empty-result guard to GetFolderName and GetFileName output, bringing them into parity with GetChannelFolderName. Also defensively sanitize channel_id in the variable map to guard against future non-numeric platform IDs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/archive/utils_test.go (2)
36-96:expectError: truepath is never exercised inTestGetFolderName/TestGetFileNameThe
expectErrorfield is wired into both test loops but all cases set it tofalse. The functions' error paths (unknown variable, empty result) are therefore uncovered.TestGetChannelFolderNamedoes include error cases — adding equivalent cases here would both document the contract and would, in fact, expose the dead-code guard issue flagged inutils.go.Suggested additions for
TestGetFolderName:{ name: "unknown variable returns error", template: "{{unknown_var}}-{{id}}", expected: "", expectError: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/archive/utils_test.go` around lines 36 - 96, Add tests that exercise the error path by including cases with expectError: true in TestGetFolderName (and similarly TestGetFileName) so unknown-variable and empty-result branches are covered; e.g., add a case with template "{{unknown_var}}-{{id}}" (expected "" and expectError true) and a case that produces an empty result, update the config via config.Get()/config.UpdateConfig(...) before calling archive.GetFolderName/archive.GetFileName, and assert that an error is returned when expectError is true; this will surface the error paths in archive.GetFolderName and archive.GetFileName.
36-36: Localtestsvariable shadows the importedtestspackage in all three test functionsThe local slice variable
testsdeclared at line 36 (and similarly inTestGetFileNameat line 122 andTestGetChannelFolderNameat line 185) shadows thetestspackage imported on line 10. Becausetests.Setup(t)is called before the declaration in each function this compiles and runs correctly, butgo vet -shadowandstaticcheckwill flag it, and it can confuse readers scanning the loop body.♻️ Suggested rename
- tests := []struct { + testCases := []struct { name string template string expected string expectError bool }{Apply the same rename in
TestGetFileNameandTestGetChannelFolderName, and updatefor _, tt := range teststofor _, tt := range testCases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/archive/utils_test.go` at line 36, The local slice variable named `tests` in the test functions shadows the imported `tests` package; rename the local variable to `testCases` (or similar) in the three functions (the one around line 36, TestGetFileName, and TestGetChannelFolderName) and update the loop headers from `for _, tt := range tests` to `for _, tt := range testCases`; keep the calls to the imported package like `tests.Setup(t)` unchanged so the package reference is not shadowed.internal/archive/utils.go (1)
130-147: Inconsistent per-variable sanitization ingetVariableMap
title,channel_display_name, andchannel_idare pre-sanitized before being placed in the map, butchannel,id,date,type,YYYY,MM,DD, andHHare inserted raw. The finalSanitizeFileNamecall on the resolved template neutralizes any unsafe content, so there is no security gap — but the inconsistency can mislead future readers into thinking only the pre-sanitized fields are guarded.Consider either sanitizing all user-supplied fields up front (aligning with the
GetChannelFolderNamepattern instoragetemplate) or adding a comment explaining why those fields are intentionally left raw.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/archive/utils.go` around lines 130 - 147, The getVariableMap function builds variables with only title, channel_display_name and channel_id sanitized via utils.SanitizeFileName, which is inconsistent and confusing; either sanitize all user-supplied fields (e.g., id, channel, date, type, YYYY, MM, DD, HH) before inserting them into the variables map (following the same pattern as GetChannelFolderName in storagetemplate), or add a clear comment above the variables map explaining that final template resolution will call SanitizeFileName and that these fields are intentionally left raw for formatting reasons; update getVariableMap to apply utils.SanitizeFileName to each user-controllable field or add the explanatory comment to remove ambiguity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/archive/utils.go`:
- Around line 71-74: GetFolderName and GetFileName currently call
utils.SanitizeFileName and then check for an empty result, but SanitizeFileName
never returns empty so those guards are dead and allow templates like
"{{channel_id}}" to silently become "unnamed_file"; instead, validate template
input variables before performing substitution: in GetFolderName and GetFileName
inspect the incoming StorageTemplateInput fields referenced by
folderTemplate/fileTemplate (e.g., ChannelID, ChannelName, MessageID, UserID)
and return an error if any required variable is empty, then perform
template.Execute and SanitizeFileName; remove the post-sanitization empty-string
checks and mirror the pre-substitution validation pattern used in
storagetemplate.GetChannelFolderName.
---
Nitpick comments:
In `@internal/archive/utils_test.go`:
- Around line 36-96: Add tests that exercise the error path by including cases
with expectError: true in TestGetFolderName (and similarly TestGetFileName) so
unknown-variable and empty-result branches are covered; e.g., add a case with
template "{{unknown_var}}-{{id}}" (expected "" and expectError true) and a case
that produces an empty result, update the config via
config.Get()/config.UpdateConfig(...) before calling
archive.GetFolderName/archive.GetFileName, and assert that an error is returned
when expectError is true; this will surface the error paths in
archive.GetFolderName and archive.GetFileName.
- Line 36: The local slice variable named `tests` in the test functions shadows
the imported `tests` package; rename the local variable to `testCases` (or
similar) in the three functions (the one around line 36, TestGetFileName, and
TestGetChannelFolderName) and update the loop headers from `for _, tt := range
tests` to `for _, tt := range testCases`; keep the calls to the imported package
like `tests.Setup(t)` unchanged so the package reference is not shadowed.
In `@internal/archive/utils.go`:
- Around line 130-147: The getVariableMap function builds variables with only
title, channel_display_name and channel_id sanitized via utils.SanitizeFileName,
which is inconsistent and confusing; either sanitize all user-supplied fields
(e.g., id, channel, date, type, YYYY, MM, DD, HH) before inserting them into the
variables map (following the same pattern as GetChannelFolderName in
storagetemplate), or add a clear comment above the variables map explaining that
final template resolution will call SanitizeFileName and that these fields are
intentionally left raw for formatting reasons; update getVariableMap to apply
utils.SanitizeFileName to each user-controllable field or add the explanatory
comment to remove ambiguity.
…r sanitization SanitizeFileName never returns empty (it falls back to "unnamed_file"), so the post-sanitization empty guards were dead code. Move the emptiness check to per-variable pre-substitution (matching GetChannelFolderName's pattern) so that empty inputs like a missing channel_id return an error instead of silently producing colliding folder names. Also remove per-variable pre-sanitization from getVariableMap since the final SanitizeFileName on the resolved output already handles path safety, and pre-sanitizing masked the emptiness check.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/archive/utils.go (1)
22-27: Avoid dual UUID sources of truth.
StorageTemplateInput.UUID(Line 22) isn’t used, whilegetVariableMapuses the separateuuidargument (Line 132). Consider consolidating to one source (e.g., rely oninput.UUIDand drop the extra param, or remove the field) to prevent divergence.Also applies to: 130-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/archive/utils.go` around lines 22 - 27, StorageTemplateInput currently defines a UUID field that isn’t used while getVariableMap takes a separate uuid parameter, creating two sources of truth; pick one source and remove the other. Either (A) remove the uuid parameter from getVariableMap and use input.UUID throughout (update getVariableMap signature and all callers to pass StorageTemplateInput and read input.UUID), or (B) remove the UUID field from StorageTemplateInput and keep the uuid parameter (delete the field and update any code that referenced input.UUID). Update any tests/call sites accordingly so only one identifier (StorageTemplateInput.UUID or the uuid parameter) is the canonical source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/archive/utils.go`:
- Around line 22-27: StorageTemplateInput currently defines a UUID field that
isn’t used while getVariableMap takes a separate uuid parameter, creating two
sources of truth; pick one source and remove the other. Either (A) remove the
uuid parameter from getVariableMap and use input.UUID throughout (update
getVariableMap signature and all callers to pass StorageTemplateInput and read
input.UUID), or (B) remove the UUID field from StorageTemplateInput and keep the
uuid parameter (delete the field and update any code that referenced
input.UUID). Update any tests/call sites accordingly so only one identifier
(StorageTemplateInput.UUID or the uuid parameter) is the canonical source.
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
Hey @Zibbp, did you have time to review it or is there anything blocking it? |
Zibbp
approved these changes
Mar 10, 2026
Owner
Zibbp
left a comment
Zibbp
left a comment
There was a problem hiding this comment.
I tested out the migrate task locally and it works fine there. Thanks!
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
channel_folder_templateconfig option that controls how channel-level directories are named on disk, resolving the problem of archives scattering across folders when a Twitch channel renames.{{channel}}(login name),{{channel_id}}(Twitch User ID — stable, never changes),{{channel_display_name}}.{{channel}}which preserves existing behavior — no breaking changes.Details
Problem: Channel folders are hardcoded to use the Twitch login name. When a streamer changes their username, new archives go to a new folder while old ones remain in the old one.
Solution: A
channel_folder_templatesetting (alongside the existingfolder_templateandfile_template) lets users choose what names their channel directories. Using{{channel_id}}gives a stable identifier that never changes across renames.What's included:
channel_folder_templateconfig field with{{channel}}default{{channel_id}}and{{channel_display_name}}also available in existing folder/file templatesVideosDirMigrateimproved to detect the videos directory from channel image paths (more robust with custom templates)Closes #1019