Conversation
Forem returns 404 when username/slug is encodeURIComponent'd as one segment (%2F). Encode each segment separately. search/feed_content is dead on live Forem; use official GET /articles/search instead. Unit tests cover both rules without network.
📝 WalkthroughWalkthroughThe PR adds validated article and search URL helpers, exposes ChangesDEV.to URL construction and API integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DevToAPI
participant URLHelpers
participant DEVTOAPI
Caller->>DevToAPI: call getArticle or searchArticles
DevToAPI->>URLHelpers: build request URL
URLHelpers-->>DevToAPI: return endpoint URL
DevToAPI->>DEVTOAPI: send API request
DEVTOAPI-->>DevToAPI: return response
DevToAPI-->>Caller: return API result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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)
src/devto-api.ts (1)
178-184: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExpose the helper’s
topparameter throughsearchArticles.
buildSearchArticlesUrlsupportstop, but this public method’s argument type omits it, preventing typed callers from using that supported search parameter.async searchArticles(args: { q: string; page?: number; per_page?: number; + top?: number; search_fields?: string; }): Promise<unknown> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/devto-api.ts` around lines 178 - 184, Update the searchArticles argument type to include the optional top parameter supported by buildSearchArticlesUrl, preserving its existing type and forwarding behavior through args so typed callers can use it.
🧹 Nitpick comments (1)
src/devto-api.test.ts (1)
67-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise
getArticlerather than only rebuilding its expected URL.This test never calls the public method, so it cannot detect a regression in the helper wiring. Mock
globalThis.fetch, callawait api.getArticle(...), and assert the URL passed to the mock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/devto-api.test.ts` around lines 67 - 75, Update the test in “DevToAPI URL helpers via public methods” to mock globalThis.fetch, invoke api.getArticle with the article path, and assert the URL passed to the fetch mock. Keep the existing URL expectations while ensuring the test exercises the public getArticle method and its helper wiring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/devto-api.ts`:
- Around line 90-93: Update the DevTo API class’s baseUrl getter to return a new
URL copy derived from `#baseUrl` rather than the private URL instance, while
preserving the existing URL value and getter interface.
- Around line 30-46: Update the article path validation around the cleaned path
construction to split on "/" and require exactly two non-empty segments,
rejecting values such as "user/slug/extra" before URL encoding. Preserve
independent encoding of the username and slug, and add a test covering the
rejected three-segment path.
---
Outside diff comments:
In `@src/devto-api.ts`:
- Around line 178-184: Update the searchArticles argument type to include the
optional top parameter supported by buildSearchArticlesUrl, preserving its
existing type and forwarding behavior through args so typed callers can use it.
---
Nitpick comments:
In `@src/devto-api.test.ts`:
- Around line 67-75: Update the test in “DevToAPI URL helpers via public
methods” to mock globalThis.fetch, invoke api.getArticle with the article path,
and assert the URL passed to the fetch mock. Keep the existing URL expectations
while ensuring the test exercises the public getArticle method and its helper
wiring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54e4b15a-e282-45d5-bd7a-761d09f7f9ae
📒 Files selected for processing (2)
src/devto-api.test.tssrc/devto-api.ts
| if (args.path) { | ||
| const cleaned = args.path.replace(/^\/+/, "").replace(/\/+$/, ""); | ||
| if (!cleaned || cleaned.includes("..") || cleaned.includes("://")) { | ||
| throw new Error("Invalid article path"); | ||
| } | ||
| const slash = cleaned.indexOf("/"); | ||
| if (slash <= 0 || slash === cleaned.length - 1) { | ||
| throw new Error( | ||
| 'Article path must be "username/slug" (two path segments)', | ||
| ); | ||
| } | ||
| const username = cleaned.slice(0, slash); | ||
| const slug = cleaned.slice(slash + 1); | ||
| // Encode each segment independently so "/" remains a path separator. | ||
| return new URL( | ||
| `articles/${encodeURIComponent(username)}/${encodeURIComponent(slug)}`, | ||
| baseUrl, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject paths containing more than two segments.
user/slug/extra passes validation and becomes user/slug%2Fextra, despite the documented two-segment contract. Split first and require exactly two non-empty segments.
Proposed fix
- const slash = cleaned.indexOf("/");
- if (slash <= 0 || slash === cleaned.length - 1) {
+ const segments = cleaned.split("/");
+ if (segments.length !== 2 || segments.some((segment) => !segment)) {
throw new Error(
'Article path must be "username/slug" (two path segments)',
);
}
- const username = cleaned.slice(0, slash);
- const slug = cleaned.slice(slash + 1);
+ const [username, slug] = segments;Also add a rejection test for "user/slug/extra".
📝 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.
| if (args.path) { | |
| const cleaned = args.path.replace(/^\/+/, "").replace(/\/+$/, ""); | |
| if (!cleaned || cleaned.includes("..") || cleaned.includes("://")) { | |
| throw new Error("Invalid article path"); | |
| } | |
| const slash = cleaned.indexOf("/"); | |
| if (slash <= 0 || slash === cleaned.length - 1) { | |
| throw new Error( | |
| 'Article path must be "username/slug" (two path segments)', | |
| ); | |
| } | |
| const username = cleaned.slice(0, slash); | |
| const slug = cleaned.slice(slash + 1); | |
| // Encode each segment independently so "/" remains a path separator. | |
| return new URL( | |
| `articles/${encodeURIComponent(username)}/${encodeURIComponent(slug)}`, | |
| baseUrl, | |
| if (args.path) { | |
| const cleaned = args.path.replace(/^\/+/, "").replace(/\/+$/, ""); | |
| if (!cleaned || cleaned.includes("..") || cleaned.includes("://")) { | |
| throw new Error("Invalid article path"); | |
| } | |
| const segments = cleaned.split("/"); | |
| if (segments.length !== 2 || segments.some((segment) => !segment)) { | |
| throw new Error( | |
| 'Article path must be "username/slug" (two path segments)', | |
| ); | |
| } | |
| const [username, slug] = segments; | |
| // Encode each segment independently so "/" remains a path separator. | |
| return new URL( | |
| `articles/${encodeURIComponent(username)}/${encodeURIComponent(slug)}`, | |
| baseUrl, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/devto-api.ts` around lines 30 - 46, Update the article path validation
around the cleaned path construction to split on "/" and require exactly two
non-empty segments, rejecting values such as "user/slug/extra" before URL
encoding. Preserve independent encoding of the username and slug, and add a test
covering the rejected three-segment path.
| /** Exposed for unit tests and smoke scripts. */ | ||
| get baseUrl(): URL { | ||
| return this.#baseUrl; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Return a copy instead of exposing mutable request state.
URL is mutable, so api.baseUrl.hostname = ... silently redirects all subsequent API requests despite #baseUrl being private.
Proposed fix
get baseUrl(): URL {
- return this.#baseUrl;
+ return new URL(this.#baseUrl);
}📝 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.
| /** Exposed for unit tests and smoke scripts. */ | |
| get baseUrl(): URL { | |
| return this.#baseUrl; | |
| } | |
| /** Exposed for unit tests and smoke scripts. */ | |
| get baseUrl(): URL { | |
| return new URL(this.#baseUrl); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/devto-api.ts` around lines 90 - 93, Update the DevTo API class’s baseUrl
getter to return a new URL copy derived from `#baseUrl` rather than the private
URL instance, while preserving the existing URL value and getter interface.
1 participant
Summary
user/slugwithencodeURIComponentproduces a single segment with%2F, which Forem 404s.GET /articles/searchinstead of deadsearch/feed_content(live 404).Context
Hit these in production against live Forem while operating a forked MCP. Happy to contribute the fix upstream so other agents don't get silent 404s.
Test plan
npm run test:ci(vitest) greengetArticle({ path: "user/slug" })→ 200searchArticles({ q: "python" })→ 200Summary by CodeRabbit