Fix replace.bat when CWD is wrong; alert on unknown firmware build#10
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves reliability and debuggability of the AdBreak workflow by making the Windows replacement script independent of the current working directory and by adding a clear user-facing alert when the current Kindle build ID is not present in the offset table.
Changes:
- Anchor
replace.bat’s recursive search to the script directory (%~dp0) and print per-file replacements plus a final summary. - Add a missing-offset guard in
adbreak.htmlto alert on unsupported/unknown build IDs. - Update
guide.txtwith expected Windows output and add a minimal Windows-focused.gitignore.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
adbreak/replace.bat |
Makes the replacement scan deterministic regardless of CWD and adds console output + counts. |
adbreak/guide.txt |
Documents the new Windows script behavior/output so users know what to expect. |
adbreak/adbreak.html |
Adds a clear unsupported-build alert before dereferencing missing offset entries. |
.gitignore |
Ignores common Windows metadata files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for /r "%_root%" %%F in (details.html) do ( | ||
| if exist "%%~fF" ( | ||
| copy /y "%~dp0adbreak.html" "%%~fF" >nul | ||
| copy /y "%_root%adbreak.html" "%%~fF" >nul | ||
| if not errorlevel 1 ( | ||
| set /a _count+=1 | ||
| echo Replaced: %%~fF | ||
| ) | ||
| ) | ||
| ) | ||
| if !_count! equ 0 ( | ||
| echo No details.html files found under: | ||
| echo %_root% | ||
| echo Put replace.bat next to adbreak.html inside your copied .assets tree, then run again. |
There was a problem hiding this comment.
The final summary uses _count (successful copies) to decide whether to print "No details.html files found". If details.html files exist but the copy fails (e.g., adbreak.html missing or permissions issue), _count stays 0 and the script reports that no files were found, which is misleading. Consider separately counting matches vs. successful replacements and/or checking %_root%adbreak.html exists up front; also emit an explicit error when a copy fails so users can diagnose why nothing was replaced.
| setlocal EnableDelayedExpansion | ||
| rem Always search from this script's folder. "Run as administrator" and some | ||
| rem launch methods leave the current directory as System32, so for /r "." | ||
| rem would find nothing and fail silently. | ||
| set "_root=%~dp0" | ||
| set /a _count=0 |
There was a problem hiding this comment.
EnableDelayedExpansion isn't needed here (you never read _count inside the for block), and it can break when %~dp0 or any matched path contains ! (literal exclamation marks get treated as delayed-expansion delimiters after %_root% is expanded). Consider dropping delayed expansion and using %_count% for the final summary (or enabling delayed expansion only in the narrowest scope where it's required).
Problem
replace.batused for/r ".", so if the batch was started with the wrong working directory (e.g. Run as administrator → oftenSystem32), nodetails.htmlfiles were found and copies were silent (>nul).offsets, the script threw after the first alert with no clear reason.Changes
%~dp0so the tree is always anchored to the folder that containsreplace.batandadbreak.html.offsets, show an alert instead of failing silently.guide.txt: note the expected console output..gitignorefor common Windows junk files.