fix(#860): filter out empty strings for endpoints#866
Conversation
📝 WalkthroughWalkthroughThis PR filters empty lines from the scraper's endpoint parsing, refreshes many export cache hashes, updates item metadata and counts in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
🧹 Nitpick comments (1)
build/scraper.mjs (1)
70-73: Fix is correct —.filter((d) => d !== '')cleanly removes the trailing/stray empty entries.The chained call correctly handles all line-ending variants (
\r\n/\n) and manifest positions (start, middle, end of file). The empty endpoint that produced the invalid URL is reliably dropped before any downstream URL construction.One cosmetic nit: the
gflag on thesplitregex at line 72 is ignored byString.prototype.split(), which always splits on all occurrences regardless of flags. It's harmless but misleading.♻️ Drop the redundant `g` flag
- .split(/\r?\n/g) + .split(/\r?\n/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/scraper.mjs` around lines 70 - 73, The split call uses a regex with a redundant 'g' flag which is ignored by String.prototype.split; update the chain that currently calls .split(/\r?\n/g) to use .split(/\r?\n/) instead so the pattern remains the same but the misleading global flag is removed (locate the expression that returns decompressed.replace(manifestRegex, '').split(...).filter(...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@build/scraper.mjs`:
- Around line 70-73: The split call uses a regex with a redundant 'g' flag which
is ignored by String.prototype.split; update the chain that currently calls
.split(/\r?\n/g) to use .split(/\r?\n/) instead so the pattern remains the same
but the misleading global flag is removed (locate the expression that returns
decompressed.replace(manifestRegex, '').split(...).filter(...)).
DopiGFX
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/static.yaml (3)
83-83: Tests can now pass whilelintis still running or failing.Dropping
lintfromneedsmeanstestandlintrun concurrently. Thetypesjob at line 67 still retainsneeds: [build, lint], so there's an asymmetry: a PR with lint failures shows ✅ tests but ❌ types. This is fine if branch-protection rules still require thelintjob to pass before merging; otherwise, the lint gate is effectively weakened for test consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/static.yaml at line 83, The workflow currently runs the test job without waiting for lint, causing tests to pass while lint can still be running or failing; update the test job's needs to include lint (make the test job depend on [build, lint], matching the types job) so lint must complete before test finishes—look for the workflow job named "test" in .github/workflows/static.yaml and add "lint" to its needs array alongside "build".
100-101: Clarify thenpm cibug with a specific issue reference.The comment "bug on lower versions of node/npm with the current manifest version" is vague. There are legitimate known bugs — for example, npm ci intermittently refuses to install because the lockfile generated by a prior
npm installis not considered "in-sync", and npm ci can fail unexpectedly when a new version of a transitive dependency is published. Both were reported against the latest npm as recently as late 2025, so the workaround may well be justified.However, without linking to the upstream issue (e.g.,
npm/cli#8726ornpm/cli#8693), future maintainers have no way to know when it's safe to revert. Please add a reference so the workaround can be removed when the upstream bug is fixed.✏️ Suggested comment improvement
- # have to use install due to bug on lower versions of node/npm with the current manifest version + # npm ci is unreliable with certain lockfile states on npm 10.x (npm/cli#8726, npm/cli#8693); + # revert to npm ci once those issues are resolved upstream run: npm install🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/static.yaml around lines 100 - 101, Update the inline comment before the "run: npm install" step to cite the specific upstream npm issue(s) that justify using npm install instead of npm ci (for example reference issues like npm/cli#8726 or npm/cli#8693); replace the vague phrase "bug on lower versions of node/npm with the current manifest version" with a short note pointing to the exact GitHub issue(s) and date so future maintainers know when to reevaluate reverting to "npm ci".
83-83: Tests can now pass while lint is still failing.Removing
lintfromneedslets thetestjob run concurrently withlint. Thetypesjob at line 67 still hasneeds: [build, lint], so there's an asymmetry: a PR with lint errors will show ✅ tests but ❌ types. If the intent is purely a speed improvement (get test signal earlier), this is fine, but it's worth confirming that branch protection rules still require thelintjob to pass before merging, or the lint gate is effectively weakened.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/static.yaml at line 83, The workflow allows the test job to run without waiting for lint (needs: [build]) which can yield passing tests while lint fails; either add "lint" back into the test job's needs array so the "test" job uses needs: [build, lint], or if the intention is to surface tests earlier, explicitly document/configure branch protection to require the "lint" job before merge; update the "test" job's needs (or the branch protection rules) and ensure the "types" job dependency remains consistent with the chosen behavior (job names: test, lint, types, build).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/static.yaml:
- Around line 100-101: Replace the vague comment and the CI command "run: npm
install" with the standard CI behavior: use "npm ci" to ensure reproducible
installs and fail-fast on lockfile drift; if there really is a known
incompatibility, replace the comment with a concise note linking to the upstream
npm issue/PR and list the affected Node/npm version range instead of the current
vague text, and update the workflow step (the CI run step currently shown as
"run: npm install") accordingly.
---
Nitpick comments:
In @.github/workflows/static.yaml:
- Line 83: The workflow currently runs the test job without waiting for lint,
causing tests to pass while lint can still be running or failing; update the
test job's needs to include lint (make the test job depend on [build, lint],
matching the types job) so lint must complete before test finishes—look for the
workflow job named "test" in .github/workflows/static.yaml and add "lint" to its
needs array alongside "build".
- Around line 100-101: Update the inline comment before the "run: npm install"
step to cite the specific upstream npm issue(s) that justify using npm install
instead of npm ci (for example reference issues like npm/cli#8726 or
npm/cli#8693); replace the vague phrase "bug on lower versions of node/npm with
the current manifest version" with a short note pointing to the exact GitHub
issue(s) and date so future maintainers know when to reevaluate reverting to
"npm ci".
- Line 83: The workflow allows the test job to run without waiting for lint
(needs: [build]) which can yield passing tests while lint fails; either add
"lint" back into the test job's needs array so the "test" job uses needs:
[build, lint], or if the intention is to surface tests earlier, explicitly
document/configure branch protection to require the "lint" job before merge;
update the "test" job's needs (or the branch protection rules) and ensure the
"types" job dependency remains consistent with the chosen behavior (job names:
test, lint, types, build).
|
🎉 This PR is included in version 1.1272.120 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
What did you fix?
closes #860
I think the Issue behavior changed between https://github.com/WFCD/warframe-items/actions/runs/22107055430/job/63892686833 and https://github.com/WFCD/warframe-items/actions/runs/22112879116/job/63913381682
I've looked into it and now it is not caused by a bad control character anymore, but by an empty string getting passed into the endpoint concatenation causing and invalid url
Reproduction steps
Build cannot proceed, because an empty string is passed to the endpoint
Evidence/screenshot/link to line
warframe-items/build/scraper.mjs
Line 70 in e9dc76e
the .replace results in an empty string which gets passed to the endpoint and results in an invalid url.
I added a .filter to get rid of any empty strings.
Considerations
Summary by CodeRabbit
Bug Fixes
Data Updates
Chores