Process Club Images #11
Conversation
WalkthroughAdds a GitHub Actions step that copies converted club images from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GH as GitHub Actions
participant WF as list-img.yml
participant FS as File System
GH->>WF: Run workflow
WF->>FS: Convert images to WebP (existing)
note over WF,FS: New guarded copy+cleanup step
WF->>FS: test for files in converted/img/club (compgen / conditional)
alt files exist
WF->>FS: mkdir -p converted/club
WF->>FS: cp -rv --update=none converted/img/club/* -> converted/club/
WF->>FS: rm -rf converted/img/club/*
end
WF-->>GH: Continue remaining steps (unchanged)
sequenceDiagram
autonumber
participant CLI as list-img.js
participant FS as File System
participant OUT as images.json
CLI->>FS: Read subfolders under images root
FS-->>CLI: Folder list
CLI->>CLI: Filter folders (excludeDirs = ['club'])
CLI->>FS: Read images in remaining folders
CLI->>OUT: Write images.json
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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: 1
🧹 Nitpick comments (3)
.github/workflows/list-img.yml (1)
48-51: Confirm data-retention intent for duplicates.This logic deletes same-named files from
converted/img/clubif they already exist inconverted/club. If you intend to keep the newer duplicates for manual review instead of discarding them, switch torsync -a --ignore-existingwithout the finalrm. Otherwise, current behavior permanently drops those files.list-img.js (2)
7-8: Use a Set for O(1) exclusion checks.Minor improvement and clearer intent.
-const excludeDirs = ['club']; +const excludeDirs = new Set(['club']);
12-12: Align predicate with Set-based exclusion (and keep behavior identical).- return fs.statSync(itemPath).isDirectory() && !excludeDirs.includes(item); + return fs.statSync(itemPath).isDirectory() && !excludeDirs.has(item);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/list-img.yml(1 hunks)list-img.js(1 hunks)
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…pdate=none` while moving club img
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…eep error when rm is failed
…n error code 1 I don't want to ignore other error (by adding `|| true`) If there are no better solution, I still want to use cp
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/list-img.yml (1)
48-54: Non-portable cp flag will fail; switch to mv -n with guard
cp --update=noneisn’t supported on ubuntu-latest and will break the job. Use a guardedmv -nand only clean leftovers if any.Apply this diff:
- name: 移動已轉換的社團圖片到 converted/club (已存在者除外) run: | mkdir -p converted/club if [ -d converted/img/club ] && compgen -G "converted/img/club/*" > /dev/null; then - cp -rv --update=none converted/img/club/* converted/club/ - rm -rf converted/img/club/* + mv -vn converted/img/club/* converted/club/ + rm -rf converted/img/club/* || true fiAlternative: rsync for idempotent copy + cleanup (also portable):
mkdir -p converted/club if [ -d converted/img/club ] && compgen -G "converted/img/club/*" > /dev/null; then rsync -a --ignore-existing converted/img/club/ converted/club/ find converted/img/club -type f -delete find converted/img/club -type d -empty -delete fi
🧹 Nitpick comments (1)
list-img.js (1)
7-13: Use a Set for exclusions; consider Dirent and sorting for determinism
- Swap array includes for a Set to avoid linear scans.
- Optional: use
withFileTypesto avoid extrastatcalls and sort folders/files to keepimages.jsonstable across runs.Apply this minimal diff:
-const excludeDirs = ['club']; +const excludeDirs = new Set(['club']); - return fs.statSync(itemPath).isDirectory() && !excludeDirs.includes(item); + return fs.statSync(itemPath).isDirectory() && !excludeDirs.has(item);Optional outside this range (deterministic output):
// sort folders const imageFolders = readFilesInFolder(imgFolder).sort(); // later, sort files per folder const images = fs.readdirSync(folderPath).sort();Optional alternative (fewer syscalls):
function readFilesInFolder(folderPath) { return fs.readdirSync(folderPath, { withFileTypes: true }) .filter(d => d.isDirectory() && !excludeDirs.has(d.name)) .map(d => d.name); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/list-img.yml(1 hunks)list-img.js(1 hunks)
YuevUwU
force-pushed
the
fix-club-img
branch
from
af14250 to
e543436
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
list-img.js (4)
7-7: Make exclusions configurable and O(1) lookupUse an env-driven Set so CI or local runs can tweak excluded dirs without code changes; also normalize case.
-const excludeDirs = ['club']; +const excludeDirSet = new Set( + (process.env.EXCLUDE_DIRS || 'club') + .split(',') + .map(s => s.trim().toLowerCase()) + .filter(Boolean) +);
9-13: Avoid per-entry statSync; use withFileTypes + stable orderingThis removes extra syscalls and ensures deterministic folder order. Adjust for the Set from above.
-function readFilesInFolder(folderPath) { - return fs.readdirSync(folderPath).filter(item => { - const itemPath = path.join(folderPath, item); - return fs.statSync(itemPath).isDirectory() && !excludeDirs.includes(item); - }); -} +function readFilesInFolder(folderPath) { + return fs + .readdirSync(folderPath, { withFileTypes: true }) + .filter(d => d.isDirectory() && !excludeDirSet.has(d.name.toLowerCase())) + .map(d => d.name) + .sort(); +}
20-24: Filter to image files and sort for deterministic images.jsonPrevents subfolders/hidden files from leaking into the JSON and stabilizes diff noise.
- imageFolders.forEach(folder => { - const folderPath = path.join(imgFolder, folder); - const images = fs.readdirSync(folderPath); - imageJson[folder] = images; - }); + const exts = /\.(?:avif|webp|png|jpe?g|gif)$/i; + imageFolders.forEach(folder => { + const folderPath = path.join(imgFolder, folder); + const dirents = fs.readdirSync(folderPath, { withFileTypes: true }); + const images = dirents + .filter(d => d.isFile() && exts.test(d.name)) + .map(d => d.name) + .sort(); + imageJson[folder] = images; + });
26-28: Atomic write to avoid partial filesWrite to a temp file then rename.
- const jsonString = JSON.stringify(imageJson, null, 2); - fs.writeFileSync(jsonFilePath, jsonString); + const jsonString = JSON.stringify(imageJson, null, 2); + const tmpPath = `${jsonFilePath}.tmp`; + fs.writeFileSync(tmpPath, jsonString); + fs.renameSync(tmpPath, jsonFilePath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/list-img.yml(1 hunks)list-img.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/list-img.yml
1 participant
Before
本PR旨在替代以下手動流程：
img/club/xxx.jpgconverted/img/club/xxx.jpgconverted/img/club/*.jpg手動移到converted/club/*.jpgAfter
converted/img/club將被忽略img/club，就會轉換為WebP丟到converted/club已知問題
img/club下的原始圖片，則不會覆蓋掉現有的 webp (多此一舉結果反直覺)img/**的圖片不會被轉換或刪除 #13