Add year-month segmented folders for the Github image hosting.#62
Add year-month segmented folders for the Github image hosting.#62D0n9 wants to merge 1 commit intozWingz:masterfrom
Conversation
📝 WalkthroughWalkthroughThe PR adds date-based path organization for GitHub uploads. When the new pathByDate configuration option is enabled, uploaded files are stored in year/month subdirectories. Changes include conditional path retrieval logic, a new recursive path-gathering method, enhanced upload result handling with optional fileName properties, and minor configuration defaults adjustments. Changes
Sequence DiagramsequenceDiagram
actor User
participant Config as Configuration
participant Octo as Octo Class
participant GitHubAPI as GitHub API
User->>Config: Upload image with pathByDate enabled
Config->>Octo: Initialize with pathByDate = true
Octo->>Octo: Store pathByDate flag
User->>Octo: Call upload(imgInfo)
Octo->>Octo: Check pathByDate flag
alt pathByDate is true
Octo->>Octo: Get current date (year/month)
Octo->>Octo: Compute date-based path
Octo->>Octo: Create effectiveRelativePath
else pathByDate is false
Octo->>Octo: Use standard path
end
Octo->>GitHubAPI: PUT file content to computed path
GitHubAPI->>Octo: Return response (sha, url)
alt pathByDate is true
Octo->>Octo: Extract fileName from result
Octo->>User: Return {imgUrl, sha, fileName}
else
Octo->>User: Return {imgUrl, sha}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/octokit.ts (1)
174-176:⚠️ Potential issue | 🟡 Minor
throw dthrows a falsy value — use a properError.This line is reached only when
dis falsy (theif (d)on Line 168 didn't match). Throwingnull/undefinedproduces an unhelpful error that's difficult to catch meaningfully downstream.Proposed fix
/* istanbul ignore next */ - throw d + throw new Error(`Upload failed for ${fileName}: unexpected empty response`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/octokit.ts` around lines 174 - 176, The code currently does "throw d" when "d" is falsy (after the "if (d)" check), which can throw null/undefined; replace that with throwing a proper Error instance: when "d" is truthy keep rethrowing it, otherwise throw new Error with a clear message (e.g. "Unexpected empty error from Octokit response") and optionally attach any available context (status, request info) if present; update the throw site that currently uses the variable "d" in lib/octokit.ts to throw a new Error when d is falsy so downstream catchers always receive a real Error object.
🧹 Nitpick comments (1)
index.ts (1)
243-243: Unrelated bug fix:userConfig.type→userConfig.origin.This corrects the default to read the right property, but it's a separate fix from the
pathByDatefeature. Consider noting it in the PR description or splitting it into its own commit for a cleaner history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` at line 243, The default assignment currently references the wrong property; change any use of userConfig.type to userConfig.origin (e.g., the default: userConfig.origin || 'github' expression) so the fallback reads the correct field, and update the commit/PR notes to indicate this is an unrelated bug fix (or split it into its own commit) for clearer history.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/octokit.ts`:
- Around line 156-158: The path-building uses dayjs() twice in the expression
that sets effectiveRelativePath (when this.pathByDate is true), which can
produce inconsistent year/month across a boundary; fix by calling dayjs() once,
store it in a local variable (e.g., now or currentDate) and then use
currentDate.format('YYYY') and currentDate.format('MM') when building the path
for effectiveRelativePath so both year and month come from the same instant.
- Around line 70-92: getTree's declared return type lacks the item.type field
used by getPathTreeRecursive; update the type signature of getTree to return
items with a type property (e.g., an interface/typedef like { path: string; sha:
string; type: 'blob' | 'tree' | string }) and adjust any usages accordingly so
item.type is typed, then ensure getPathTreeRecursive continues to consume that
typed list from getTree; locate the getTree and getPathTreeRecursive functions
in lib/octokit.ts to apply the change.
---
Outside diff comments:
In `@lib/octokit.ts`:
- Around line 174-176: The code currently does "throw d" when "d" is falsy
(after the "if (d)" check), which can throw null/undefined; replace that with
throwing a proper Error instance: when "d" is truthy keep rethrowing it,
otherwise throw new Error with a clear message (e.g. "Unexpected empty error
from Octokit response") and optionally attach any available context (status,
request info) if present; update the throw site that currently uses the variable
"d" in lib/octokit.ts to throw a new Error when d is falsy so downstream
catchers always receive a real Error object.
---
Nitpick comments:
In `@index.ts`:
- Line 243: The default assignment currently references the wrong property;
change any use of userConfig.type to userConfig.origin (e.g., the default:
userConfig.origin || 'github' expression) so the fallback reads the correct
field, and update the commit/PR notes to indicate this is an unrelated bug fix
(or split it into its own commit) for clearer history.
| /** Recursively get all files under path (for Pull when pathByDate). Returns flat list with paths relative to this.path. */ | ||
| async getPathTreeRecursive (): Promise<{ tree: { path: string; sha: string }[] }> { | ||
| const { tree } = await this.getPathTree() | ||
| const flat: { path: string; sha: string }[] = [] | ||
| const collect = async (sha: string, prefix: string): Promise<void> => { | ||
| const list = await this.getTree(sha) | ||
| for (const item of list) { | ||
| if (item.type === 'tree') { | ||
| await collect(item.sha, prefix + item.path + '/') | ||
| } else { | ||
| flat.push({ path: prefix + item.path, sha: item.sha }) | ||
| } | ||
| } | ||
| } | ||
| for (const item of tree) { | ||
| if (item.type === 'tree') { | ||
| await collect(item.sha, item.path + '/') | ||
| } else { | ||
| flat.push({ path: item.path, sha: item.sha }) | ||
| } | ||
| } | ||
| return { tree: flat } | ||
| } |
There was a problem hiding this comment.
getTree return type is missing the type field used here.
Line 77 accesses item.type on results from this.getTree(sha) (Line 75), but getTree (Line 47) declares its return type as { path: string; sha: string }[] — there's no type property. The GitHub API does return it at runtime, but the type declaration should be fixed to avoid silent undefined checks if TypeScript strict mode is ever enabled.
Proposed fix
- async getTree (sha): Promise<{ path: string; sha: string }[]> {
+ async getTree (sha): Promise<{ path: string; sha: string; type?: string }[]> {Additionally, the recursion is sequential (await in a for loop). For repos with many subdirectories this could be slow, but it's acceptable for typical image hosting use cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/octokit.ts` around lines 70 - 92, getTree's declared return type lacks
the item.type field used by getPathTreeRecursive; update the type signature of
getTree to return items with a type property (e.g., an interface/typedef like {
path: string; sha: string; type: 'blob' | 'tree' | string }) and adjust any
usages accordingly so item.type is typed, then ensure getPathTreeRecursive
continues to consume that typed list from getTree; locate the getTree and
getPathTreeRecursive functions in lib/octokit.ts to apply the change.
| const effectiveRelativePath = this.pathByDate | ||
| ? pathJoin(dayjs().format('YYYY'), dayjs().format('MM'), fileName) | ||
| : fileName |
There was a problem hiding this comment.
Call dayjs() once to avoid inconsistent year/month at a boundary.
dayjs() is invoked twice on Line 157. If the clock ticks past midnight on a month boundary between the two calls, you could get a path like 2026/02 when you intended 2026/01 (or vice versa). Store the result once:
Proposed fix
- const effectiveRelativePath = this.pathByDate
- ? pathJoin(dayjs().format('YYYY'), dayjs().format('MM'), fileName)
- : fileName
+ const now = dayjs()
+ const effectiveRelativePath = this.pathByDate
+ ? pathJoin(now.format('YYYY'), now.format('MM'), fileName)
+ : fileName🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/octokit.ts` around lines 156 - 158, The path-building uses dayjs() twice
in the expression that sets effectiveRelativePath (when this.pathByDate is
true), which can produce inconsistent year/month across a boundary; fix by
calling dayjs() once, store it in a local variable (e.g., now or currentDate)
and then use currentDate.format('YYYY') and currentDate.format('MM') when
building the path for effectiveRelativePath so both year and month come from the
same instant.
1 participant
Hi, I really like this extension. I have a new feature request and have already developed the code for the implemented functionality. Requesting a merge.
Summary by CodeRabbit
Release Notes
New Features
Improvements