feat: note the discontinuation of live remote pagasa 10-day excel files - #157
Conversation
…ADME and online docs, #156
|
Warning Rate limit exceeded@ciatph has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughDocumentation and CLI updates announce PAGASA 10‑Day Excel discontinuation, add archived-data guidance and migration snippets, introduce Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as Interactive Selector
participant L as ColorLog
participant D as Downloader/Processor
U->>CLI: start interactive selector
CLI->>L: log discontinuation warning (red)
CLI->>L: log default/local guidance (green)
CLI->>L: log archived-remote guidance (cyan)
CLI-->>U: Prompt: Use remote PAGASA 10‑Day Excel file? (Y/N)
alt User chooses Y
U-->>CLI: Y
CLI-->>U: Prompt for remote Excel URL
U-->>CLI: provides URL
CLI->>D: download & process Excel (existing flow)
D-->>CLI: processing results
CLI-->>U: present results
else User chooses N
U-->>CLI: N
CLI->>D: use local/default/archived Excel (existing flow)
D-->>CLI: processing results
CLI-->>U: present results
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (14)
app/.env.example (2)
1-7: Satisfy dotenv-linter ordering and EOF newline; keep keys alphabetized.To quiet the linter and make diffs predictable, alphabetize keys and add a trailing newline.
Apply:
-EXCEL_FILE_URL=https://pubfiles.pagasa.dost.gov.ph/pagasaweb/files/climate/tendayweatheroutlook/day1.xlsx -DEFAULT_EXCEL_FILE_URL=https://pubfiles.pagasa.dost.gov.ph/pagasaweb/files/climate/tendayweatheroutlook/day1.xlsx -ARCHIVED_EXCEL_FILE_URL=https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx -SHEETJS_COLUMN=__EMPTY -SORT_ALPHABETICAL=1 -SPECIAL_CHARACTERS=├â┬▒:ñ,├▒:n,â:,ñ:ñ -IMAGE_URL=https://raw.githubusercontent.com/ciatph/ph-municipalities/master/docs/diagrams/ph-municipalities-arch-90.png +ARCHIVED_EXCEL_FILE_URL=https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx +DEFAULT_EXCEL_FILE_URL=https://pubfiles.pagasa.dost.gov.ph/pagasaweb/files/climate/tendayweatheroutlook/day1.xlsx +EXCEL_FILE_URL=https://pubfiles.pagasa.dost.gov.ph/pagasaweb/files/climate/tendayweatheroutlook/day1.xlsx +IMAGE_URL=https://raw.githubusercontent.com/ciatph/ph-municipalities/master/docs/diagrams/ph-municipalities-arch-90.png +SHEETJS_COLUMN=__EMPTY +SORT_ALPHABETICAL=1 +SPECIAL_CHARACTERS=├â┬▒:ñ,├▒:n,â:,ñ:ñ +Note: SPECIAL_CHARACTERS now documents extra corrections (├▒:n and ñ:ñ). Consider aligning the code’s default fallback (ExcelFile.initMalformedTextList defaultTextValue) to include these as well so behavior matches when the env var is absent. I can open a follow-up if you want.
3-3: ARCHIVED_EXCEL_FILE_URL verification completeWe’ve confirmed that the URL in app/.env.example (Line 3)
ARCHIVED_EXCEL_FILE_URL=https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsxis reachable (HTTP 200), downloads a non-empty file (~622 KB), and begins with magic bytes 0x50 0x4B, indicating a valid ZIP/XLSX archive.
Optional refactors recommended:
- Pin the URL to a specific commit SHA or tag (e.g.
refs/sha/<commit-hash>/day1.xlsx) to guarantee immutability in builds/tests.- Update README documentation to note that, after 2025-08-31, tooling should default to (or fall back on) this archived URL.
app/src/classes/excel/index.js (1)
113-118: Clarify what goes into invalidRows vs. config mismatches.The note implies province-not-in-table cases end up here. In code, invalidRows collects rows that fail the string-pattern check on data rows; province/config mismatches are handled in tests, not here. Tighten the JSDoc to avoid confusion.
Apply:
- /** - * Invalid data rows that do not follow the expected "municipalityName (provinceName)" uniform string pattern - * e.g., also having a **province** that's not included in the **PAGASA Rainfall Analysis Table** in - * `"City of Isabela (City of Isabela (Not a Province))"` - * @type {string[]} - */ + /** + * Invalid data rows captured during load for rows after DATA_ROW_START that do NOT match + * the expected "municipalityName (provinceName)" uniform string pattern + * (e.g., nested parentheses like "City of Isabela (City of Isabela (Not a Province))"). + * Note: Province-name mismatches vs. the PAGASA seasonal config are handled elsewhere and + * are not included here. + * @type {string[]} + */Optionally, strengthen the pattern check to better support PH names (unicode, hyphens, apostrophes) and forbid nested parentheses:
// Suggestion for followsStringPattern(): // - Start/End anchored // - Unicode letters, digits, spaces and common punctuation before the province // - Province part must not contain '(' or ')' const re = /^[\p{L}\p{N} .,'-]+ \([^\(\)]+\)\s*$/u return re.test(String(str || ''))app/__tests__/provinces/createInstances.js (1)
41-47: Nice visibility: log invalid row count.This helps correlate with ExcelFile.invalidRows and test tolerances. Consider returning invalidRowsCount in the function result so downstream consumers (e.g., updateInstances) don’t need to recalculate or re-derive it.
Apply:
- return { + return { allExcelProvinces, allProvinces, uniqueExcelProvinces, uniqueProvinces, fromConfig, - fromExcel + fromExcel, + invalidRowsCount }app/__tests__/municipalities/municipalitiesCount.js (2)
80-82: Helpful remediation guidance in warnings; consider a fallback to archived URL.Good to point users to extending ExcelFile/ExcelFactory or passing custom regions.json. Given the imminent discontinuation after 2025-08-31, consider updating the test setup to fall back to ARCHIVED_EXCEL_FILE_URL when EXCEL_FILE_URL is unavailable.
Example:
const sourceUrl = process.env.EXCEL_FILE_URL || process.env.ARCHIVED_EXCEL_FILE_URL const excelFile = new ExcelFile({ pathToFile: path.join(__dirname, 'excelfiledownload.xlsx'), url: sourceUrl })
85-90: Stale date marker in pass message.The
[20240826]stamp looks outdated and could confuse readers. Either update to today’s date or generate it dynamically to avoid future drift.Apply one of:
- let passMsg = '[20240826]: Allow the test to succeed here since there is little information about updated\n' + let passMsg = '[2025-08-24]: Allow the test to succeed here since there is little information about updated\n'or make it dynamic:
- let passMsg = '[20240826]: Allow the test to succeed here since there is little information about updated\n' + const today = new Date().toISOString().slice(0, 10) + let passMsg = `[${today}]: Allow the test to succeed here since there is little information about updated\n`app/src/lib/selector.js (1)
20-28: Prefer const and consolidate message constructionThese messages are constants and never reassigned; use const and template strings for clarity.
- // Warning messages - let msgWarn = '[⚠️ WARNING]: PAGASA 10-Day Excel files are no longer available.\n' - msgWarn += 'https://github.com/ciatph/ph-municipalities/issues/156\n' + // Warning messages + const msgWarn = [ + '[⚠️ WARNING]: PAGASA 10-Day Excel files are no longer available.', + 'https://github.com/ciatph/ph-municipalities/issues/156' + ].join('\n') @@ - const msgUseDefault = '[⚠️ WARNING]: Please use the default local Excel file as data source.\n' + const msgUseDefault = '[⚠️ WARNING]: Please use the default local Excel file as data source.\n'app/__tests__/provinces/updateInstances.js (1)
8-21: Update JSDoc to include invalidRowsCount parameterThe function signature now accepts invalidRowsCount; the JSDoc should reflect this for consumers and tooling.
-/** - * Updates the initial province names data read by an `ExcelFile` or `ExcelFactory` class from `createInstances()` for log-viewing purposes only. - * Displays diagnostic information and error logs. - * @param {Object} params - Input parameters - * @param {String[]} params.allExcelProvinces - all provinces from the 10-day Excel file - * @param {String[]} params.allProvinces - all provinces from the PAGASA seasonal config file - * @param {Set} params.uniqueExcelProvinces - `Set` version of `allExcelProvinces` to ensure unique province names - * @param {Set} params.uniqueProvinces - `Set` version of `allProvinces` to ensure unique province names, - * @param {String[]} params.fromConfig - Provinces present in the config (PAGASA seasonal) but missing in the 10-Day Excel file - * @param {String[]} params.fromExcel - Provinces present in the 10-Day Excel file but missing in the config (PAGASA seasonal) - * @returns {Object} Object containing Arrays of processed province names - * - `uniqueExcelProvinces` {Set} - updated version of the `uniqueExcelProvinces` input parameter - * - `uniqueProvinces` {Set} - updated version of the `uniqueProvinces` input parameter - */ +/** + * Updates the initial province names data read by an `ExcelFile` or `ExcelFactory` class from `createInstances()` for log-viewing purposes only. + * Displays diagnostic information and error logs. + * @param {Object} params - Input parameters + * @param {String[]} params.allExcelProvinces - all provinces from the 10-Day Excel file + * @param {String[]} params.allProvinces - all provinces from the PAGASA seasonal config file + * @param {Set} params.uniqueExcelProvinces - `Set` version of `allExcelProvinces` to ensure unique province names + * @param {Set} params.uniqueProvinces - `Set` version of `allProvinces` to ensure unique province names + * @param {String[]} params.fromConfig - Provinces present in the config (PAGASA seasonal) but missing in the 10-Day Excel file + * @param {String[]} params.fromExcel - Provinces present in the 10-Day Excel file but missing in the config (PAGASA seasonal) + * @param {number} [params.invalidRowsCount=0] - Count of invalid rows parsed from the Excel source + * @returns {Object} Object containing Sets of processed province names + * - `uniqueExcelProvinces` {Set} - updated version of the `uniqueExcelProvinces` input parameter + * - `uniqueProvinces` {Set} - updated version of the `uniqueProvinces` input parameter + */README.md (6)
49-49: Grammar: “was generated”Minor readability fix.
- "description": "This dataset generated with reference to the Excel file contents from the source URL on 20220808.", + "description": "This dataset was generated with reference to the Excel file contents from the source URL on 20220808.",
175-175: Typo: “ph-municipalites” → “ph-municipalities”-While ph-municipalites do not support parsing and extracting PAGASA 10-day weather forecast data, _you can extend the `ExcelFile` or `ExcelFactory` classes with custom logic and codes to enable parsing and extracting PAGASA 10-day weather forecast data_. +While ph-municipalities do not support parsing and extracting PAGASA 10-day weather forecast data, _you can extend the `ExcelFile` or `ExcelFactory` classes with custom logic and codes to enable parsing and extracting PAGASA 10-day weather forecast data_.
346-346: Typo: “neccessary” → “necessary”- /* Override constructor if neccessary */ + /* Override constructor if necessary */
331-334: Fix markdownlint MD028: blank line inside blockquoteEnsure the blockquote lines are contiguous; remove the blank line between them.
->💡 **NOTE:** The open-source version of ph-municipalities provides better testing, documentation, and modularity compared to its original form as basic procedural functions in a private project. - -> Even in an official project, developers would still need an <b>approved API token</b> and would need to shift from <b>handling Excel files</b> to <b>handling PAGASA REST API responses</b>. +> 💡 **NOTE:** The open-source version of ph-municipalities provides better testing, documentation, and modularity compared to its original form as basic procedural functions in a private project. +> Even in an official project, developers would still need an <b>approved API token</b> and would need to shift from <b>handling Excel files</b> to <b>handling PAGASA REST API responses</b>.
547-552: Align CLI wording with discontinuation: say “archived remote Excel file”Post-discontinuation, “remote Excel file” may mislead users into expecting live data.
-- Asks users to enter the download URL of a remote Excel file or use the default local Excel file +- Asks users to enter the download URL of an archived remote Excel file or use the default local Excel file @@ - - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if download URL in the class constructor is provided. + - Loads and parses the downloaded archived Excel file to `/app/data/datasource.xlsx` if a download URL in the class constructor is provided.Repeat the same wording for
npm run list:province:-- Asks users to enter the download URL of a remote Excel file or use the default local Excel file +- Asks users to enter the download URL of an archived remote Excel file or use the default local Excel file @@ - - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if download URL in the class constructor is provided. + - Loads and parses the downloaded archived Excel file to `/app/data/datasource.xlsx` if a download URL in the class constructor is provided.Also applies to: 560-563
691-691: Clarify “Downloads and parses a remote Excel file”Post-2025-08-31, clarify that examples target archived files unless users supply their own source.
-- Downloads and parses a remote Excel file. +- Downloads and parses an archived remote Excel file (or any user-supplied Excel URL).
📜 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 ignored due to path filters (1)
app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
README.md(16 hunks)app/.env.example(1 hunks)app/__tests__/municipalities/municipalitiesCount.js(2 hunks)app/__tests__/provinces/createInstances.js(1 hunks)app/__tests__/provinces/updateInstances.js(3 hunks)app/package.json(1 hunks)app/src/classes/excel/index.js(1 hunks)app/src/lib/selector.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/__tests__/provinces/createInstances.js (3)
app/__tests__/municipalities/municipalitiesCount.js (1)
excelFile(13-16)app/__tests__/municipalities/municipalitiesPerProvinceCount.js (1)
excelFile(12-15)app/__tests__/provinces/testProvinceCount.js (1)
excelFile(11-14)
app/src/lib/selector.js (3)
app/__tests__/municipalities/municipalitiesCount.js (3)
ColorLog(5-5)require(10-10)logger(6-6)app/__tests__/provinces/createInstances.js (2)
ColorLog(3-3)logger(4-4)app/index.js (1)
ColorLog(4-4)
🪛 LanguageTool
README.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...ted), suitable for reference or testing._ > > ➡️ _**This limitation comes directly from...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...or fetching live data from that service._ > > For details and discussion, see [Issue #1...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...l) Download URL for a remote Excel file. - See the EXCEL_FILE_URL variable on the...
(QB_NEW_EN)
[grammar] ~262-~262: There might be a mistake here.
Context: ...📉 Discontinuation of PAGASA Excel Files As of August 31, 2025, PAGASA has dis...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...ken requests may not be granted. > 💡 ph-municipalities will remain archived-only,...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...ted manually with the API by developers.
What should I use ph-municipalities for after August 31, 2025?
It can still be used as a reference o...
(QB_NEW_EN)
[grammar] ~550-~550: There might be a mistake here.
Context: ...ile in /app/data/day1.xlsx by default. - Loads and parses the downloaded Excel fi...
(QB_NEW_EN)
[grammar] ~551-~551: There might be a mistake here.
Context: ...cel file to /app/data/datasource.xlsx if download URL in the class constructor i...
(QB_NEW_EN)
[grammar] ~551-~551: There might be a mistake here.
Context: ...RL in the class constructor is provided. - Displays a list of available PH **region...
(QB_NEW_EN)
[grammar] ~561-~561: There might be a mistake here.
Context: ...ile in /app/data/day1.xlsx by default. - Loads and parses the downloaded Excel fi...
(QB_NEW_EN)
[grammar] ~562-~562: There might be a mistake here.
Context: ...cel file to /app/data/datasource.xlsx if download URL in the class constructor i...
(QB_NEW_EN)
[grammar] ~562-~562: There might be a mistake here.
Context: ...RL in the class constructor is provided. - Lists all municipalities under specified...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
README.md
331-331: Blank line inside blockquote
(MD028, no-blanks-blockquote)
467-467: Link fragments should be valid
(MD051, link-fragments)
🪛 dotenv-linter (3.3.0)
app/.env.example
[warning] 3-3: [UnorderedKey] The ARCHIVED_EXCEL_FILE_URL key should go before the DEFAULT_EXCEL_FILE_URL key
(UnorderedKey)
[warning] 7-7: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 7-7: [UnorderedKey] The IMAGE_URL key should go before the SHEETJS_COLUMN key
(UnorderedKey)
🔇 Additional comments (11)
app/package.json (1)
3-3: Version bump to 1.4.7 looks good.No concerns on the manifest; semver patch aligns with docs/CLI messaging-only changes.
app/__tests__/municipalities/municipalitiesCount.js (1)
28-30: Terminology tweak is accurate.“Non-uniform” better reflects the pattern-based filtering done by ExcelFile; aligns with the JSDoc update.
app/src/lib/selector.js (1)
36-39: Order and frequency of warnings look goodColorized messages (red warning, green default suggestion, cyan remote guidance) are clear and shown just before the URL prompt. Nice touch.
app/__tests__/provinces/updateInstances.js (3)
50-53: Nice: invalid rows are now surfacedWarning when invalidRowsCount > 0 adds useful visibility without changing behavior.
60-65: Helpful tips; suggest minor copy tweaks onlyThe added NOTE and TIP are valuable. Consider small copy edits later for brevity, but content is solid.
58-58: Fixed typo in updateInstances.js
The typo “incosistent” on line 58 ofapp/__tests__/provinces/updateInstances.jshas been corrected to “inconsistent.”README.md (5)
3-18: Clear deprecation notice; consider one-sentence TL;DR up topThe warning block is clear and anchored with dates. Optionally lead with a single-sentence TL;DR before the blockquote for scanners.
93-96: Good: explicit column pattern and optional strict-layout hintsThe guidance helps prevent parsing surprises. LGTM.
114-121: Narrative refresh reads well; keep capitalization consistentThe section is consistent with the new direction. No blocking issues.
150-150: Nice: alternatives listedGood to point to PSGC-based libs for non-PAGASA use cases.
1031-1031: Nice usability detailExplicitly telling readers where to open the generated docs reduces friction.
Also applies to: 1063-1063
| let msgRemote = '[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n' | ||
| msgRemote += 'https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx' | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use ARCHIVED_EXCEL_FILE_URL and accept lowercase input; fix likely broken raw URL path
- Don’t hardcode the archived Excel URL. Prefer the new ARCHIVED_EXCEL_FILE_URL (with a safe fallback).
- The current raw URL path looks off; raw.githubusercontent.com format is /{owner}/{repo}/{branch}/{path}. Consider the archives branch path include to app/data/day1.xlsx.
- Make the Y/N check case-insensitive to avoid UX friction.
Apply this diff:
- let msgRemote = '[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n'
- msgRemote += 'https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx'
+ const archivedUrl = process.env.ARCHIVED_EXCEL_FILE_URL
+ || 'https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/app/data/day1.xlsx'
+ const msgRemote = `[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n${archivedUrl}`And for the input check:
- if (askDownload === 'Y') {
+ if (String(askDownload).trim().toUpperCase() === 'Y') {If you’d like, I can add a defaulting behavior where pressing Enter after showing the warnings pre-fills the archived URL (from env) for a smoother flow.
Also applies to: 34-34
🏁 Script executed:
#!/usr/bin/env bash
# Verify the hardcoded raw URL returns a 404 or error
echo "Checking original raw URL:"
curl -s -o /dev/null -w " Status: %{http_code}\n" \
https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx
# Verify the suggested raw URL path returns a 200 OK
echo "Checking proposed raw URL:"
curl -s -o /dev/null -w " Status: %{http_code}\n" \
https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/app/data/day1.xlsx
# Search for ARCHIVED_EXCEL_FILE_URL usage in selector.js
echo "Searching for ARCHIVED_EXCEL_FILE_URL in app/src/lib/selector.js:"
rg -n "ARCHIVED_EXCEL_FILE_URL" --color=always app/src/lib/selector.js || echo " Not found"
# Check current askDownload comparison logic
echo "Inspecting askDownload comparison in selector.js:"
rg -n "askDownload.*===.*['\\\"]Y['\\\"]" --color=always app/src/lib/selector.jsLength of output: 990
🏁 Script executed:
#!/usr/bin/env bash
set -e
# Test various potential raw URLs for the archived day1.xlsx file
for url in \
https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/day1.xlsx \
https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/app/data/day1.xlsx \
https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/data/day1.xlsx \
https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/app/day1.xlsx; do
echo "Testing $url"
curl -s -o /dev/null -w " Status: %{http_code}\n" "$url"
done
# Use GitHub API to list 'archives' branch tree and search for day1.xlsx
echo -e "\nFetching 'archives' branch tree via GitHub API"
curl -s https://api.github.com/repos/ciatph/ph-municipalities/git/trees/archives?recursive=1 \
| grep -C2 "day1.xlsx" || echo " No entries found in tree listing"
# Inspect askDownload handling in selector.js for case-insensitive matching
echo -e "\nInspecting askDownload usage in app/src/lib/selector.js"
grep -n "askDownload" -C3 app/src/lib/selector.js || echo " No askDownload usage found"Length of output: 3746
Ensure correct archived file URL and case‐insensitive prompt check
Based on verification:
- The existing hard-coded URL (
…/refs/heads/archives/day1.xlsx) does resolve (200), but it’s more maintainable to use an environment variable with a sensible fallback. - The suggested fallback path (
archives/app/data/day1.xlsx) is incorrect; the actual file lives atarchives/day1.xlsx. - The prompt comparison (
askDownload === 'Y') is case-sensitive and will ignore lowercase “y”.
Please apply these refactors in app/src/lib/selector.js:
• Replace lines 24–26 (warning URL):
- let msgRemote = '[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n'
- msgRemote += 'https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx'
+ const archivedUrl = process.env.ARCHIVED_EXCEL_FILE_URL
+ || 'https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/day1.xlsx'
+ const msgRemote =
+ `[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n${archivedUrl}`• Update the Y/N check at line 34 for case‐insensitivity:
- if (askDownload === 'Y') {
+ if (String(askDownload).trim().toUpperCase() === 'Y') {Optional: To streamline the user flow, you might auto-fill the archived URL when they press Enter (i.e., treat empty input as “Y”).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let msgRemote = '[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n' | |
| msgRemote += 'https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx' | |
| // Replace the hard-coded warning URL with an env var fallback | |
| const archivedUrl = process.env.ARCHIVED_EXCEL_FILE_URL | |
| || 'https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/day1.xlsx' | |
| const msgRemote = | |
| `[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n${archivedUrl}` | |
| // … later, when checking the user's Y/N response … | |
| if (String(askDownload).trim().toUpperCase() === 'Y') { |
🤖 Prompt for AI Agents
In app/src/lib/selector.js around lines 24–26 and line 34, replace the
hard-coded archived URL with a process.env variable fallback (e.g.,
process.env.ARCHIVED_EXCEL_URL) that falls back to the correct path
'https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx'
(note archives/day1.xlsx, not archives/app/data/day1.xlsx), and change the Y/N
prompt check on line 34 to be case-insensitive by normalizing the input (e.g.,
convert toLowerCase() and compare to 'y' or treat empty input as a default 'y'
if you want the optional auto-fill behavior).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/lib/selector.js (1)
47-57: Bug: remote download failure leaves ExcelHandler null, causing downstream failureIf the remote fetch/init throws, you set ExcelHandler = null and exit = true, then proceed to createInstances(ExcelHandler). This likely crashes later. Fallback to the default local Excel on error to keep the flow resilient.
Apply this diff:
try { ExcelHandler = new ExcelFactory({ url }) await ExcelHandler.init() exit = true console.log(`\nUsing the file downloaded to ${ExcelHandler.pathToFile}\nas data source`) } catch (err) { - console.log(`[ERROR] ${err.message}`) - ExcelHandler = null - exit = true + console.log(`[ERROR] ${err.message}`) + console.log('Falling back to the default local Excel file as data source.') + ExcelHandler = new ExcelFactory() + exit = true }Optionally, also log the fallback path after instantiation for parity with the success path.
♻️ Duplicate comments (2)
app/src/lib/selector.js (2)
21-24: Standardize fallback raw URL to the canonical branch/path formThe current fallback uses a non‑canonical “refs/heads/archives” segment. While it may resolve, the recommended raw path format is /{owner}/{repo}/{branch}/{path}, i.e., without refs/heads. Prefer the simpler and less brittle:
https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/day1.xlsxApply this diff:
- const archivedUrl = process.env.ARCHIVED_EXCEL_FILE_URL || - 'https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx' + const archivedUrl = process.env.ARCHIVED_EXCEL_FILE_URL || + 'https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/day1.xlsx'
25-25: Fix lint failure: use const for msgRemoteESLint pipeline error: “'msgRemote' is never reassigned. Use 'const' instead.” This blocks CI.
Apply this diff:
- let msgRemote = `[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n${archivedUrl}\n` + const msgRemote = `[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n${archivedUrl}\n`
🧹 Nitpick comments (3)
app/src/lib/selector.js (3)
26-28: Prefer const and avoid string mutation for msgWarnMinor cleanup: build msgWarn once (no +=), improves clarity and satisfies prefer-const.
Apply this diff:
- const msgUseDefault = '[⚠️ WARNING]: Please use the default local Excel file as data source.\n' - let msgWarn = '[⚠️ WARNING]: PAGASA 10-Day Excel files are no longer available.\n' - msgWarn += 'https://github.com/ciatph/ph-municipalities/issues/156\n' + const msgUseDefault = '[⚠️ WARNING]: Please use the default local Excel file as data source.\n' + const msgWarn = '[⚠️ WARNING]: PAGASA 10-Day Excel files are no longer available.\n' + + 'https://github.com/ciatph/ph-municipalities/issues/156\n'
33-34: Make the prompt label match the default (No) and tighten wordingYour text says “Press enter to ignore,” which implies default = No. Use the conventional “[y/N]” to avoid confusion.
Apply this diff:
- const askDownload = await prompt('\nWould you like to download and use a remote PAGASA 10-Day Excel file?\nPress enter to ignore. Press Y and enter to proceed. [n/Y]: ') + const askDownload = await prompt('\nWould you like to download and use a remote PAGASA 10-Day Excel file?\nPress Enter to ignore. Press Y then Enter to proceed. [y/N]: ')
42-43: Optional: basic URL validation before attempting downloadPrevent obvious mistakes early and reduce noisy errors by validating the scheme and shape before constructing ExcelFactory.
Example (outside selected range; illustrative only):
// Before using `url`: try { const u = new URL(url); if (!/^https?:$/.test(u.protocol)) throw new Error('Only http(s) URLs are supported'); } catch { console.log('[ERROR] Invalid URL. Please enter a valid http(s) URL.'); url = ''; continue; }
📜 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 (1)
app/src/lib/selector.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/lib/selector.js (3)
app/__tests__/provinces/createInstances.js (2)
ColorLog(3-3)logger(4-4)app/__tests__/provinces/updateInstances.js (3)
ColorLog(3-3)require(6-6)logger(4-4)app/index.js (1)
ColorLog(4-4)
🪛 GitHub Check: Lint App
app/src/lib/selector.js
[failure] 25-25:
'msgRemote' is never reassigned. Use 'const' instead
🪛 GitHub Actions: Lint and Test
app/src/lib/selector.js
[error] 25-25: ESLint: 'msgRemote' is never reassigned. Use 'const' instead. (prefer-const)
🔇 Additional comments (3)
app/src/lib/selector.js (3)
7-7: LGTM: ColorLog import is appropriate and consistent with usage across testsImport path and usage align with app/index.js and test files. No action needed.
19-20: LGTM: Consistent, readable logger initializationUsing a yellow bold logger matches existing conventions in tests; good reuse.
36-41: LGTM: Case-insensitive input and clear guidance logsNormalizing to uppercase for the Y/N check reduces UX friction. The three colored guidance messages are clear and actionable.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
175-176: Fix typos: “ph-municipalites” and “neccessary”Two small but visible typos.
-While ph-municipalites do not support parsing and extracting PAGASA 10-day weather forecast data, _you can extend the `ExcelFile` or `ExcelFactory` classes with custom logic and codes to enable parsing and extracting PAGASA 10-day weather forecast data_. +While ph-municipalities do not support parsing and extracting PAGASA 10-day weather forecast data, _you can extend the `ExcelFile` or `ExcelFactory` classes with custom logic and codes to enable parsing and extracting PAGASA 10-day weather forecast data_. @@ - /* Override constructor if neccessary */ + /* Override constructor if necessary */Also applies to: 346-348
♻️ Duplicate comments (1)
app/src/lib/selector.js (1)
21-26: Normalize the archived fallback URL (prefer branch path over refs/heads) and tighten copyThe env-first fallback is great. For the default, prefer the canonical raw path format using the branch and path segments; it’s shorter and more conventional than using refs/heads. Also, assemble the message in one template literal to avoid stray trailing newlines.
- const archivedUrl = process.env.ARCHIVED_EXCEL_FILE_URL || - 'https://raw.githubusercontent.com/ciatph/ph-municipalities/refs/heads/archives/day1.xlsx' - - const msgRemote = `[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n${archivedUrl}\n` + const archivedUrl = process.env.ARCHIVED_EXCEL_FILE_URL + || 'https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/day1.xlsx' + const msgRemote = `[⚠️ WARNING]: Should you still want to use a remote Excel file, you can use the archived remote Excel file as data source at:\n${archivedUrl}`Run this quick check locally to ensure the fallback URL returns 200 and the env var (if set) is picked up by the CLI:
#!/usr/bin/env bash set -euo pipefail echo "Fallback URL status:" curl -s -o /dev/null -w " %{http_code}\n" https://raw.githubusercontent.com/ciatph/ph-municipalities/archives/day1.xlsx echo "Selector reads env:" ARCHIVED_EXCEL_FILE_URL="https://example.invalid/day1.xlsx" node -e "console.log('env:', process.env.ARCHIVED_EXCEL_FILE_URL)"
🧹 Nitpick comments (17)
app/src/lib/selector.js (4)
33-37: Prompt defaults and acceptance criteria: align bracket hint, accept YESThe prompt hint currently shows “[n/Y]” but Enter selects local (i.e., default = No). Standard is “[y/N]”. Also accept “YES” to reduce friction.
- const askDownload = await prompt('\nWould you like to download and use a remote PAGASA 10-Day Excel file?\nPress enter to ignore. Press Y and enter to proceed. [n/Y]: ') - const askDownloadValue = String(askDownload).trim().toUpperCase() + const askDownload = await prompt('\nWould you like to download and use a remote PAGASA 10-Day Excel file?\nPress Enter to ignore. Type Y and press Enter to proceed. [y/N]: ') + const askDownloadValue = String(askDownload).trim().toUpperCase() @@ - if (askDownloadValue === 'Y') { + if (askDownloadValue === 'Y' || askDownloadValue === 'YES') {
42-47: Allow cancel back to local from the remote flowOnce users choose remote, there’s no way back without Ctrl+C. Offer “press Enter to cancel” and fall back to local if empty input is provided.
- url = await prompt('\nEnter the download URL of a remote Excel file: ') + url = await prompt('\nEnter the download URL of a remote Excel file (or press Enter to cancel and use the local Excel file): ') + if (!String(url).trim()) { + logger.log('\nNo URL provided. Falling back to default local Excel file.\n', { color: ColorLog.COLORS.TEXT.YELLOW }) + url = false + break + }
54-57: Use ColorLog for error output and consider graceful fallback to localKeep logging consistent and avoid abrupt exit on transient network errors by falling back to local data.
- } catch (err) { - console.log(`[ERROR] ${err.message}`) - ExcelHandler = null - exit = true - } + } catch (err) { + logger.log(`[ERROR] ${err.message}`, { color: ColorLog.COLORS.TEXT.RED }) + // Graceful fallback: use local Excel file + try { + ExcelHandler = new ExcelFactory() + logger.log('\nFalling back to default local Excel file.\n', { color: ColorLog.COLORS.TEXT.YELLOW }) + } catch { + ExcelHandler = null + } + exit = true + }
59-64: Minor copyedit: “Excel” capitalizationSmall polish for CLI output.
- console.log(`\nUsing the default local excel file ${ExcelHandler.pathToFile}\nas data source`) + console.log(`\nUsing the default local Excel file ${ExcelHandler.pathToFile}\nas data source`)README.md (13)
3-18: Top warning block: clarify tense and tighten wordingMixing “Starting on … will discontinue” with later “After this date” is fine, but the “As of … has discontinued” phrasing appears elsewhere and conflicts before 2025‑08‑31. Consider consistently using “Starting on August 31, 2025”/“From August 31, 2025 onward.”
-### ⚠️ Warning +### ⚠️ Warning @@ -> Starting on **August 31, 2025**, PAGASA will permanently discontinue its downloadable 10-Day Weather Forecast Excel files. +> Starting on **August 31, 2025**, PAGASA will permanently discontinue its downloadable 10‑Day Weather Forecast Excel files. @@ -> 🟠 _From then on, results will reflect only static archived Excel files (no longer updated), suitable for reference or testing._ +> 🟠 _From that date onward, results will reflect only static archived Excel files (no longer updated), suitable for reference or testing._
49-51: Grammar fix in dataset descriptionPast tense reads better and adds the missing article.
- "description": "This dataset generated with reference to the Excel file contents from the source URL on 20220808.", + "description": "This dataset was generated with reference to the Excel file contents from the source URL on 20220808.",
93-96: Requirements: small wording tweaks and punctuation“Checkout” → “Check out”; end bullets with periods for consistency.
- - (Optional) The Excel file should have a row on the same **column** as above containing the text `"Municipalities"` plus two (2) blank rows before rows containing municipality and province names to enable strict testing and validation of the number of parsed data rows - - Checkout the Excel file format on the `/app/data/day1.xlsx` sample file for more information + - (Optional) The Excel file should have a row on the same **column** as above containing the text `"Municipalities"`, plus two (2) blank rows before rows containing municipality and province names, to enable strict testing and validation of the number of parsed data rows. + - Check out the Excel file format in the `/app/data/day1.xlsx` sample file for more information.
114-121: Minor copyedits and style consistency (open‑source as adjective)Hyphenate “open‑source” when used adjectivally and trim redundancy.
-**ph-municipalities** evolved from basic procedural functions within a _private backend project_ into a well-tested, documented, and modular open-source library, enabling broader community access and better code quality. +**ph-municipalities** evolved from basic procedural functions within a _private backend project_ into a well-tested, documented, and modular open‑source library, enabling broader community access and better code quality. @@ -> **_ph-municipalities aim to contribute to the open source community by listing ONLY Philippine provinces and municipality names, using [PAGASA's 10-day weather forecast Excel files](https://www.pagasa.dost.gov.ph/climate/climate-prediction/10-day-climate-forecast), which are publicly accessible to everyone._** +> **_ph-municipalities aims to contribute to the open‑source community by listing ONLY Philippine provinces and municipality names, using [PAGASA's 10‑day weather forecast Excel files](https://www.pagasa.dost.gov.ph/climate/climate-prediction/10-day-climate-forecast), which are publicly accessible to everyone._**
150-151: Adjective form: open‑source-Yes, several open source libraries and projects similar to ph-municipalities exist, which you can use in its place to list Philippine provinces and municipalities. +Yes, several open‑source libraries and projects similar to ph-municipalities exist, which you can use in its place to list Philippine provinces and municipalities.
261-267: Section heading and tense alignmentThis section later uses “As of August 31, 2025, … has discontinued,” which will be inconsistent until that date. Recommend forward-looking phrasing to match the top warning.
-### 📉 Discontinuation of PAGASA Excel Files - -As of **August 31, 2025**, PAGASA has discontinued its **10-Day Weather Forecast Excel** files. From now on, **ph-municipalities** only works with static archived data. +### 📉 Discontinuation of PAGASA Excel Files + +Starting **August 31, 2025**, PAGASA will discontinue its **10‑Day Weather Forecast Excel** files. From that date onward, **ph-municipalities** will only work with static archived data.
331-334: Fix markdownlint MD028: remove blank line inside blockquote and combine related linesLint tools flag a blank line within the blockquote around this note. Merge or remove the extra blank line to satisfy MD028.
->💡 **NOTE:** The open-source version of ph-municipalities provides better testing, documentation, and modularity compared to its original form as basic procedural functions in a private project. -> Even in an official project, developers would still need an <b>approved API token</b> and would need to shift from <b>handling Excel files</b> to <b>handling PAGASA REST API responses</b>. +> 💡 **NOTE:** The open‑source version of ph‑municipalities provides better testing, documentation, and modularity compared to its original form as basic procedural functions in a private project. +> Even in an official project, developers would still need an <b>approved API token</b> and would need to shift from <b>handling Excel files</b> to <b>handling PAGASA REST API responses</b>.
398-444: Add “Discontinuation” to the Table of ContentsThe new section is important; add it to ToC for discoverability.
- [NPM Scripts for Linting Files and Unit Testing](#npm-scripts-for-linting-files-and-unit-testing) - `npm run lint` - `npm run lint:fix` - `npm test` - `npm run example` +- [Discontinuation of PAGASA Excel Files](#discontinuation-of-pagasa-excel-files) - [Class Usage](#class-usage)
461-471: .env table: pluralize “descriptions”, mark DEFAULT_EXCEL_FILE_URL as deprecated post‑2025‑08‑31, and tighten phrasingAlso wrap date in bold for consistency.
- <summary>👉 Click to view the list of <b>.env</b> variables and their description.</summary> + <summary>👉 Click to view the list of <b>.env</b> variables and their descriptions.</summary> @@ - | DEFAULT_EXCEL_FILE_URL | The default remote Excel file's download URL. | - | ARCHIVED_EXCEL_FILE_URL | Download URL of an archived remote Excel file to serve as fixtures with the discontinuation of the PAGASA 10-Day Excel files.<br><br><blockquote>⚠️ Replace the value of `EXCEL_FILE_URL` and `DEFAULT_EXCEL_FILE_URL` with its value starting on **August 31, 2025** to avoid processing and test errors.</blockquote>See [Issue #156](https://github.com/ciatph/ph-municipalities/issues/156) for more information. | + | DEFAULT_EXCEL_FILE_URL | (Deprecated after <b>August 31, 2025</b>) The default remote Excel file's download URL. | + | ARCHIVED_EXCEL_FILE_URL | Download URL of an archived remote Excel file to serve as fixtures after the discontinuation of the PAGASA 10‑Day Excel files.<br><br><blockquote>⚠️ Replace the values of `EXCEL_FILE_URL` and `DEFAULT_EXCEL_FILE_URL` with this value starting on <b>August 31, 2025</b> to avoid processing and test errors.</blockquote>See [Issue #156](https://github.com/ciatph/ph-municipalities/issues/156) for more information. |
549-552: CLI bullets: small grammar and clarity tweaks-- Asks users to enter the download URL of a remote Excel file or use the default local Excel file +- Asks users to enter the download URL of a remote Excel file or use the default local Excel file. - Loads and parses the local Excel file in `/app/data/day1.xlsx` by default. - - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if download URL in the class constructor is provided. + - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if a download URL is provided in the class constructor.
560-563: Mirror the same grammar fix in the province CLI section-- Asks users to enter the download URL of a remote Excel file or use the default local Excel file +- Asks users to enter the download URL of a remote Excel file or use the default local Excel file. - Loads and parses the local Excel file in `/app/data/day1.xlsx` by default. - - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if download URL in the class constructor is provided. + - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if a download URL is provided in the class constructor.
721-749: Minor copyedits in examples (comment wording)Fix typo and add a definite article for smoother reading.
-// Reads an existing Excel file on /app/data/day1.xlsx +// Reads the existing Excel file at /app/data/day1.xlsx @@ -// JSON data of the parsed Excel file will is accessible on +// JSON data of the parsed Excel file is accessible on
1031-1031: Docs: consistent “View the generated documentation” phrasingThese two sections use the same sentence—looks good. Just adding a period for consistency.
-4. View the generated documentation in the `/app/docs` directory. Click the `/app/docs/index.html` file to launch the documentation website. +4. View the generated documentation in the `/app/docs` directory. Click the `/app/docs/index.html` file to launch the documentation website.No content change; this is a confirmation that both instances are consistent.
Also applies to: 1062-1062
📜 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)
README.md(16 hunks)app/src/lib/selector.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/lib/selector.js (4)
app/__tests__/provinces/updateInstances.js (3)
ColorLog(3-3)require(6-6)logger(4-4)app/__tests__/provinces/createInstances.js (2)
ColorLog(3-3)logger(4-4)app/__tests__/municipalities/municipalitiesCount.js (3)
ColorLog(5-5)require(10-10)logger(6-6)app/index.js (1)
ColorLog(4-4)
🪛 LanguageTool
README.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...ted), suitable for reference or testing._ > > ➡️ _**This limitation comes directly from...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...or fetching live data from that service._ > > For details and discussion, see [Issue #1...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...l) Download URL for a remote Excel file. - See the EXCEL_FILE_URL variable on the...
(QB_NEW_EN)
[grammar] ~262-~262: There might be a mistake here.
Context: ...📉 Discontinuation of PAGASA Excel Files As of August 31, 2025, PAGASA has dis...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...ken requests may not be granted. > 💡 ph-municipalities will remain archived-only,...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...ted manually with the API by developers.
What should I use ph-municipalities for after August 31, 2025?
It can still be used as a reference o...
(QB_NEW_EN)
[grammar] ~550-~550: There might be a mistake here.
Context: ...ile in /app/data/day1.xlsx by default. - Loads and parses the downloaded Excel fi...
(QB_NEW_EN)
[grammar] ~551-~551: There might be a mistake here.
Context: ...cel file to /app/data/datasource.xlsx if download URL in the class constructor i...
(QB_NEW_EN)
[grammar] ~551-~551: There might be a mistake here.
Context: ...RL in the class constructor is provided. - Displays a list of available PH **region...
(QB_NEW_EN)
[grammar] ~561-~561: There might be a mistake here.
Context: ...ile in /app/data/day1.xlsx by default. - Loads and parses the downloaded Excel fi...
(QB_NEW_EN)
[grammar] ~562-~562: There might be a mistake here.
Context: ...cel file to /app/data/datasource.xlsx if download URL in the class constructor i...
(QB_NEW_EN)
[grammar] ~562-~562: There might be a mistake here.
Context: ...RL in the class constructor is provided. - Lists all municipalities under specified...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
README.md
331-331: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🔇 Additional comments (2)
app/src/lib/selector.js (2)
19-20: Good addition: consistent, noticeable CLI warnings via ColorLogInitializing a bold, yellow ColorLog instance is a sensible UX improvement for important notices.
38-41: Nice sequencing of warnings and guidance before URL inputThe colored, ordered messages are clear: risk, safe default, then remote option. Good UX.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
461-471: Env table: deprecate default URL and clarify archived URL guidance (per prior comment)Re-iterate deprecation and tighten the ARCHIVED_EXCEL_FILE_URL note. This mirrors an earlier suggestion flagged by markdownlint and already addressed elsewhere.
- | EXCEL_FILE_URL | (Optional) Remote Excel file's download URL.<br>If provided, the Excel file will be downloaded and saved on the specified `pathToFile` local filesystem location during the `ExcelFile` class initialization.<br>Read on [Class Usage](#class-usage) for more information. | - | DEFAULT_EXCEL_FILE_URL | The default remote Excel file's download URL. | - | ARCHIVED_EXCEL_FILE_URL | Download URL of an archived remote Excel file to serve as fixtures with the discontinuation of the PAGASA 10-Day Excel files.<br><br><blockquote>⚠️ Replace the value of `EXCEL_FILE_URL` and `DEFAULT_EXCEL_FILE_URL` with its value starting on **August 31, 2025** to avoid processing and test errors.</blockquote>See [Issue #156](https://github.com/ciatph/ph-municipalities/issues/156) for more information. | + | EXCEL_FILE_URL | (Optional) Remote Excel file download URL.<br>If provided, the Excel file is downloaded to `pathToFile` during `ExcelFile` initialization.<br>Read [Class Usage](#class-usage) for more information. | + | DEFAULT_EXCEL_FILE_URL | (Deprecated after 2025‑08‑31) Default remote Excel file download URL. | + | ARCHIVED_EXCEL_FILE_URL | Download URL of an archived Excel file to serve as fixtures after the discontinuation of PAGASA 10‑Day Excel files.<br><br><blockquote>⚠️ Starting **2025‑08‑31**, set both `EXCEL_FILE_URL` and `DEFAULT_EXCEL_FILE_URL` to this value to avoid processing and test errors.</blockquote>See [Issue #156](https://github.com/ciatph/ph-municipalities/issues/156) for more information. |
🧹 Nitpick comments (16)
README.md (16)
3-18: Unify timeline phrasing and tighten the warning blockquote
- The top warning uses “Starting on August 31, 2025” (future tense), while later you use “As of August 31, 2025” (past tense). Since today is August 24, 2025, keep this section in future/“effective” tense for consistency until the date passes.
- Minor wording cleanups for clarity.
Apply this diff:
-> Starting on **August 31, 2025**, PAGASA will permanently discontinue its downloadable 10-Day Weather Forecast Excel files. +> Effective **August 31, 2025**, PAGASA will permanently discontinue its downloadable 10‑Day Weather Forecast Excel files. @@ -> 🟠 _From then on, results will reflect only static archived Excel files (no longer updated), suitable for reference or testing._ +> 🟠 _After that date, results will reflect only static archived Excel files (no longer updated), suitable for reference or testing._ @@ -> 💡 _**NOTE:** ph-municipalities is **not designed to sync with the [PAGASA TenDay API](https://tenday.pagasa.dost.gov.ph/docs)**, and it has no immediate plans for fetching live data from that service._ +> 💡 _**NOTE:** ph-municipalities is **not designed to sync with the [PAGASA TenDay API](https://tenday.pagasa.dost.gov.ph/docs)** and has no immediate plans to fetch live data from that service._
9-9: Consider pinning the archives link to a stable refThe
/archivesbranch can move. To keep guidance durable, consider linking to a tagged release or commit SHA for “last known good” fixtures.
49-49: Use ISO date phrasing in metadata descriptionMinor grammar and date-format improvement.
-"description": "This dataset was generated with reference to the Excel file contents from the source URL on 20220808.", +"description": "This dataset was generated with reference to the Excel file contents from the source URL as of 2022-08-08.",
93-96: Polish requirements bullets (grammar and clarity)Small readability tweaks.
-- At minimum, the Excel file should have a **column** that contains municipality and province names following the uniform pattern: `"municipalityName (provinceName)"` - (Optional) The Excel file should have a row on the same **column** as above containing the text `"Municipalities"` plus two (2) blank rows before rows containing municipality and province names to enable strict testing and validation of the number of parsed data rows - Checkout the Excel file format on the `/app/data/day1.xlsx` sample file for more information +- At a minimum, the Excel file should have a **column** that contains municipality and province names following the uniform pattern: `"municipalityName (provinceName)"`. +- (Optional) Include a row on the same **column** containing the text `"Municipalities"`, followed by two (2) blank rows, before the municipality/province rows to enable strict testing and row-count validation. +- Check the sample Excel file at `/app/data/day1.xlsx` for more information.
116-116: Consistent terminology: “open source” vs “open-source”Elsewhere you use “open source” (no hyphen). Use the same form here for consistency.
-**ph-municipalities** evolved from basic procedural functions within a _private backend project_ into a well-tested, documented, and modular open-source library, enabling broader community access and better code quality. +**ph-municipalities** evolved from basic procedural functions within a _private backend project_ into a well-tested, documented, and modular open source library, enabling broader community access and better code quality.
261-266: Keep “Discontinuation” tense consistent with the top warning (pre-2025-08-31)Same tense issue as the header warning. Recommend “Effective”/future phrasing here until the cutoff date passes.
-### 📉 Discontinuation of PAGASA Excel Files - -As of **August 31, 2025**, PAGASA has discontinued its **10-Day Weather Forecast Excel** files. From now on, **ph-municipalities** only works with static archived data. +### 📉 Discontinuation of PAGASA Excel Files + +Effective **August 31, 2025**, PAGASA will discontinue its **10‑Day Weather Forecast Excel** files. After that date, **ph-municipalities** only works with static archived data.
328-334: Fix markdownlint MD028: blank line inside blockquoteThere’s an empty line between two blockquoted lines, which triggers MD028. Remove the blank line.
> Like mentioned, syncing with the **PAGASA TenDay APIs** is currently **out of scope of ph-municipalities** due to the reasons mentioned in the FAQs above. Projects depending on ph-municipalities should consider alternate options in accordance with the **discontinuation of the PAGASA 10-Day Weather Forecast Excel files** and the availability of the newly-released [**PAGASA TenDay APIs**](https://tenday.pagasa.dost.gov.ph/docs). - ->💡 **NOTE:** The open-source version of ph-municipalities provides better testing, documentation, and modularity compared to its original form as basic procedural functions in a private project. +> 💡 **NOTE:** The open-source version of ph-municipalities provides better testing, documentation, and modularity compared to its original form as basic procedural functions in a private project.
335-371: Tighten migration snippet
- Avoid “...” inside code; it confuses copy-paste users.
- Optionally declare variables to avoid implicit globals in examples.
listMunicipalities () { /* Fetch from API */ } - ... } -const pagasaAPI = new PAGASATendayAPI() -weatherForecast = pagasaAPI.listMunicipalities() +const pagasaAPI = new PAGASATendayAPI() +const weatherForecast = pagasaAPI.listMunicipalities()
403-432: Add “Discontinuation” to the Table of ContentsThe new section is important and should be discoverable from the TOC. Consider inserting:
547-552: Grammar and post‑discontinuation note for the CLI (region)
- Fix phrasing “if download URL in the class constructor is provided”.
- Add a short note about behavior after 2025‑08‑31.
-- Asks users to enter the download URL of a remote Excel file or use the default local Excel file +- Asks users to enter the download URL of a remote Excel file or use the default local Excel file - Loads and parses the local Excel file in `/app/data/day1.xlsx` by default. - - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if download URL in the class constructor is provided. + - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if a download URL is provided in the class constructor. + - Note: After **2025‑08‑31**, remote downloads should point to `ARCHIVED_EXCEL_FILE_URL`; live Excel downloads are no longer available.
560-563: Grammar and post‑discontinuation note for the CLI (province)Mirror the fixes from the region script.
-- Asks users to enter the download URL of a remote Excel file or use the default local Excel file +- Asks users to enter the download URL of a remote Excel file or use the default local Excel file - Loads and parses the local Excel file in `/app/data/day1.xlsx` by default. - - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if download URL in the class constructor is provided. + - Loads and parses the downloaded Excel file to `/app/data/datasource.xlsx` if a download URL is provided in the class constructor. + - Note: After **2025‑08‑31**, remote downloads should point to `ARCHIVED_EXCEL_FILE_URL`; live Excel downloads are no longer available.
691-691: Clarify “npm run example” behavior after discontinuationMake it explicit that this script must use the archived URL post‑cutoff.
-- Downloads and parses a remote Excel file defined in the `EXCEL_FILE_URL` `.env` variable. +- Downloads and parses a remote Excel file defined in the `EXCEL_FILE_URL` `.env` variable. After **2025‑08‑31**, set this to `ARCHIVED_EXCEL_FILE_URL`; live Excel downloads are no longer available.
721-721: Fix duplicate word: “the the”Minor typo.
-// Use the the following if installed via npm +// Use the following if installed via npm
747-749: Grammar: “will is accessible”Make the sentence grammatical.
-// JSON data of the parsed Excel file will is accessible on +// JSON data of the parsed Excel file is accessible in
771-771: Fix duplicate word: “the the”Same typo appears here.
-// Use the the following if installed via npm +// Use the following if installed via npm
1031-1031: Minor copy: “View the generated documentation”Optional: say “Open /app/docs/index.html in your browser.” Reads slightly cleaner.
Also applies to: 1063-1063
📜 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 (3)
README.md(18 hunks)app/__tests__/provinces/updateInstances.js(4 hunks)app/src/classes/excel/index.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/classes/excel/index.js
- app/tests/provinces/updateInstances.js
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...ted), suitable for reference or testing._ > > ➡️ _**This limitation comes directly from...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...or fetching live data from that service._ > > For details and discussion, see [Issue #1...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...l) Download URL for a remote Excel file. - See the EXCEL_FILE_URL variable on the...
(QB_NEW_EN)
[grammar] ~262-~262: There might be a mistake here.
Context: ...📉 Discontinuation of PAGASA Excel Files As of August 31, 2025, PAGASA has dis...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...ken requests may not be granted. > 💡 ph-municipalities will remain archived-only,...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...ted manually with the API by developers.
What should I use ph-municipalities for after August 31, 2025?
It can still be used as a reference o...
(QB_NEW_EN)
[grammar] ~550-~550: There might be a mistake here.
Context: ...ile in /app/data/day1.xlsx by default. - Loads and parses the downloaded Excel fi...
(QB_NEW_EN)
[grammar] ~551-~551: There might be a mistake here.
Context: ...cel file to /app/data/datasource.xlsx if download URL in the class constructor i...
(QB_NEW_EN)
[grammar] ~551-~551: There might be a mistake here.
Context: ...RL in the class constructor is provided. - Displays a list of available PH **region...
(QB_NEW_EN)
[grammar] ~561-~561: There might be a mistake here.
Context: ...ile in /app/data/day1.xlsx by default. - Loads and parses the downloaded Excel fi...
(QB_NEW_EN)
[grammar] ~562-~562: There might be a mistake here.
Context: ...cel file to /app/data/datasource.xlsx if download URL in the class constructor i...
(QB_NEW_EN)
[grammar] ~562-~562: There might be a mistake here.
Context: ...RL in the class constructor is provided. - Lists all municipalities under specified...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
README.md
331-331: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🔇 Additional comments (1)
README.md (1)
3-18: Overall: Great, timely documentation updateThe discontinuation notice, FAQs, and migration guidance are clear and actionable. These changes will reduce confusion for users around and after 2025‑08‑31.
Also applies to: 261-371
| In short - yes, it will break when using its remote Excel file download URL that points to the `DEFAULT_EXCEL_FILE_URL` (see `.env.example`). It will also risk using outdated data when using the default archived Excel data sources. | ||
|
|
||
| > Like mentioned, syncing with the **PAGASA TenDay APIs** is currently **out of scope of ph-municipalities** due to the reasons mentioned in the FAQs above. Projects depending on ph-municipalities should consider alternate options in accordance with the **discontinuation of the PAGASA 10-Day Weather Forecast Excel files** and the availability of the newly-released [**PAGASA TenDay APIs**](https://tenday.pagasa.dost.gov.ph/docs). | ||
|
|
||
| >💡 **NOTE:** The open-source version of ph-municipalities provides better testing, documentation, and modularity compared to its original form as basic procedural functions in a private project. | ||
| > Even in an official project, developers would still need an <b>approved API token</b> and would need to shift from <b>handling Excel files</b> to <b>handling PAGASA REST API responses</b>. | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Run quick checks for lints and copy errors
- Validate markdownlint MD028 fix.
- Re-scan for duplicate “the the”.
- Spot-check env table anchors/fragments.
Also applies to: 461-471, 547-552, 560-563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for duplicate "the the"
rg -nP '\bthe\s+the\b' -C2 README.md || true
# Check for "As of August 31, 2025" vs "Effective"/"Starting" inconsistencies
rg -nP 'As of\s+\*\*August 31, 2025\*\*|Starting on\s+\*\*August 31, 2025\*\*|Effective\s+\*\*August 31, 2025\*\*' -n README.md -C1
# If available in the repo image, run markdownlint on README only (ignore global config)
# It's okay if markdownlint isn't installed; script continues.
if command -v npx >/dev/null 2>&1; then
npx --yes markdownlint-cli2 README.md || true
fiLength of output: 31453
Fix duplicate “the the” occurrences and blockquote formatting in README.md
Please address the remaining markdown issues uncovered by the lint checks:
• Duplicate words “the the” in code-example comments
– README.md:718 (“// Use the the following if installed via npm”)
– README.md:769, 814, 852 (same pattern in subsequent examples)
• Blank line inside a blockquote (MD028)
– README.md:331 (remove the empty line immediately after the “>” so the quoted paragraph isn’t broken)
• Inconsistent date phrasing around August 31, 2025
– README.md:5 (“Starting on August 31, 2025, …”) vs README.md:264 (“As of August 31, 2025, …”)
→ Choose one style and apply it consistently
• Env-table anchors/fragments
– Manually verify that all links/fragments in the .env sections point to valid headings
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
331-331: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
In README.md around lines 328–334 there is an extra blank line inside a
blockquote and elsewhere duplicate words and inconsistent date wording: remove
the empty line immediately after the ">" at line 331 so the blockquote is
continuous; search and replace the duplicated phrase "the the" in code-example
comments at lines 718, 769, 814, and 852 to a single "the"; choose one date
phrasing for "August 31, 2025" (either "Starting on **August 31, 2025**, ..." or
"As of **August 31, 2025**, ...") and apply it consistently to README.md lines 5
and 264; finally, manually verify and fix all .env section anchors/fragments so
they point to existing headings in the document.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores