chore: adding milestone models and resources#29
Conversation
📝 WalkthroughWalkthroughAdds a Milestones API resource with CRUD, listing, and milestone–work-item management; integrates it into PlaneClient and public exports; adds TypeScript models and a unit test suite covering lifecycle and work-item association. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🤖 Fix all issues with AI agents
In `@src/api/Milestones.ts`:
- Around line 1-126: Format src/api/Milestones.ts with Prettier to satisfy CI:
run prettier --write on the file (or apply your project's Prettier config) and
ensure the file's formatting for the Milestones class and its methods
(constructor, create, retrieve, update, delete, list, addWorkItems,
removeWorkItems, listWorkItems) matches the repo style; stage and commit the
updated file so the Prettier check passes.
In `@tests/unit/milestone.test.ts`:
- Around line 111-124: The test uses the unreliable global fail() which can be
undefined in recent Jest versions; update the "should delete a milestone" test
to throw an explicit error when retrieval unexpectedly succeeds (e.g., replace
fail("Expected an error when retrieving a deleted milestone") with throw new
Error("Expected an error when retrieving a deleted milestone")) or alternatively
assert via expect(client.milestones.retrieve(...)).rejects, ensuring the error
path for client.milestones.retrieve(workspaceSlug, projectId, milestone.id) is
correctly detected and keep the cleanup line milestone = undefined as is.
🧹 Nitpick comments (3)
src/models/Milestone.ts (2)
38-42: Consider making response fields non-optional onMilestoneWorkItem.When returned from
listWorkItems,id,issue, andmilestoneshould always be present. Having all fields optional weakens type safety for consumers. Consider splitting into a request type (optional fields) and a response type (required fields), or making the response fields required.
11-20: Remove unusedMilestoneLiteinterface or defer its addition until it's needed.
MilestoneLiteis exported but never imported or used anywhere in the codebase—not in the API resource, request/response types, or tests. Consider removing it if it's not part of the immediate scope.src/api/Milestones.ts (1)
71-80: Replaceparams?: anywith a typed parameter.Using
anybypasses TypeScript's type checking. ConsiderRecord<string, unknown>or a dedicated query-params interface for listing endpoints.♻️ Proposed fix
async list( workspaceSlug: string, projectId: string, - params?: any + params?: Record<string, unknown> ): Promise<PaginatedResponse<Milestone>> {async listWorkItems( workspaceSlug: string, projectId: string, milestoneId: string, - params?: any + params?: Record<string, unknown> ): Promise<PaginatedResponse<MilestoneWorkItem>> {Also applies to: 115-125
| it("should delete a milestone", async () => { | ||
| await client.milestones.delete(workspaceSlug, projectId, milestone.id); | ||
|
|
||
| // Verify it's deleted by trying to retrieve it | ||
| try { | ||
| await client.milestones.retrieve(workspaceSlug, projectId, milestone.id); | ||
| fail("Expected an error when retrieving a deleted milestone"); | ||
| } catch (error) { | ||
| expect(error).toBeDefined(); | ||
| } | ||
|
|
||
| // Prevent afterAll from trying to delete again | ||
| milestone = undefined as any; | ||
| }); |
There was a problem hiding this comment.
fail() is not a reliable Jest global — use throw new Error() instead.
fail() is not defined in newer Jest versions (27+) and can cause ReferenceError at runtime, which would be caught by the surrounding catch block and make the test pass incorrectly even when the retrieve succeeds.
🐛 Proposed fix
try {
await client.milestones.retrieve(workspaceSlug, projectId, milestone.id);
- fail("Expected an error when retrieving a deleted milestone");
+ throw new Error("Expected an error when retrieving a deleted milestone");
} catch (error) {
- expect(error).toBeDefined();
+ expect(error).toBeDefined();
+ // Ensure we didn't catch our own thrown error
+ expect((error as Error).message).not.toBe("Expected an error when retrieving a deleted milestone");
}🧰 Tools
🪛 ESLint
[error] 111-111: 'it' is not defined.
(no-undef)
[error] 117-117: 'fail' is not defined.
(no-undef)
[error] 119-119: 'expect' is not defined.
(no-undef)
🤖 Prompt for AI Agents
In `@tests/unit/milestone.test.ts` around lines 111 - 124, The test uses the
unreliable global fail() which can be undefined in recent Jest versions; update
the "should delete a milestone" test to throw an explicit error when retrieval
unexpectedly succeeds (e.g., replace fail("Expected an error when retrieving a
deleted milestone") with throw new Error("Expected an error when retrieving a
deleted milestone")) or alternatively assert via
expect(client.milestones.retrieve(...)).rejects, ensuring the error path for
client.milestones.retrieve(workspaceSlug, projectId, milestone.id) is correctly
detected and keep the cleanup line milestone = undefined as is.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/api/Milestones.ts`:
- Line 55: Replace the use of the `any` type for query params in the Milestones
API methods by defining and using a proper type (e.g., a dedicated interface
like MilestoneQueryParams or at minimum Record<string, unknown>) and update the
method signatures where `params?: any` appears (notably in the `list` method and
the other occurrence around line 92); ensure any downstream usage of `params` is
typed or guarded (use typed properties or type-narrowing) so callers and
callers' code compile without `any`.
- Around line 48-50: The method currently named delete in the Milestones class
should be renamed to del to match the standard resource method pattern; locate
the async method delete(workspaceSlug, projectId, milestoneId) (and any places
referencing it) and rename the function to async del(workspaceSlug: string,
projectId: string, milestoneId: string): Promise<void>, keeping the same
implementation (return this.httpDelete(...)); also update any internal or
external callers/tests/imports that reference Milestones.delete to use
Milestones.del to avoid breakage.
- Around line 65-68: Rename the parameter issueIds to workItemIds in the
addWorkItems method signature (async addWorkItems(workspaceSlug, projectId,
milestoneId, workItemIds): Promise<void>) and pass workItemIds as the value for
the existing payload key "issues" when calling this.post so the backend contract
stays unchanged; also apply the same
rename-to-workItemIds-but-keep-payload-key="issues" pattern to the other
method(s) in this file around the 74-82 region to match Modules.ts/Cycles.ts
conventions.
🧹 Nitpick comments (1)
src/api/Milestones.ts (1)
1-1: File name should use kebab-case.
Milestones.tsuses PascalCase but the coding guidelines specify**/*.tsfiles should use kebab-case. Rename tomilestones.tsand update all import paths that reference this file.
| async delete(workspaceSlug: string, projectId: string, milestoneId: string): Promise<void> { | ||
| return this.httpDelete(`/workspaces/${workspaceSlug}/projects/${projectId}/milestones/${milestoneId}/`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename delete to del to match the standard resource method pattern.
Per coding guidelines, standard resource methods should follow the pattern: list, create, retrieve, update, del. As per coding guidelines, "Standard resource methods should follow the pattern: list, create, retrieve, update, del".
♻️ Proposed fix
- async delete(workspaceSlug: string, projectId: string, milestoneId: string): Promise<void> {
+ async del(workspaceSlug: string, projectId: string, milestoneId: string): Promise<void> {
return this.httpDelete(`/workspaces/${workspaceSlug}/projects/${projectId}/milestones/${milestoneId}/`);
}📝 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.
| async delete(workspaceSlug: string, projectId: string, milestoneId: string): Promise<void> { | |
| return this.httpDelete(`/workspaces/${workspaceSlug}/projects/${projectId}/milestones/${milestoneId}/`); | |
| } | |
| async del(workspaceSlug: string, projectId: string, milestoneId: string): Promise<void> { | |
| return this.httpDelete(`/workspaces/${workspaceSlug}/projects/${projectId}/milestones/${milestoneId}/`); | |
| } |
🤖 Prompt for AI Agents
In `@src/api/Milestones.ts` around lines 48 - 50, The method currently named
delete in the Milestones class should be renamed to del to match the standard
resource method pattern; locate the async method delete(workspaceSlug,
projectId, milestoneId) (and any places referencing it) and rename the function
to async del(workspaceSlug: string, projectId: string, milestoneId: string):
Promise<void>, keeping the same implementation (return this.httpDelete(...));
also update any internal or external callers/tests/imports that reference
Milestones.delete to use Milestones.del to avoid breakage.
| /** | ||
| * List milestones with optional filtering | ||
| */ | ||
| async list(workspaceSlug: string, projectId: string, params?: any): Promise<PaginatedResponse<Milestone>> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any with proper typing for query params.
params?: any on Lines 55 and 92 violates the guideline to avoid any types. Use Record<string, unknown> or a dedicated query-params interface.
♻️ Proposed fix
- async list(workspaceSlug: string, projectId: string, params?: any): Promise<PaginatedResponse<Milestone>> {
+ async list(workspaceSlug: string, projectId: string, params?: Record<string, unknown>): Promise<PaginatedResponse<Milestone>> {- params?: any
+ params?: Record<string, unknown>As per coding guidelines, "Avoid any types; use proper typing or unknown with type guards".
Also applies to: 92-92
🤖 Prompt for AI Agents
In `@src/api/Milestones.ts` at line 55, Replace the use of the `any` type for
query params in the Milestones API methods by defining and using a proper type
(e.g., a dedicated interface like MilestoneQueryParams or at minimum
Record<string, unknown>) and update the method signatures where `params?: any`
appears (notably in the `list` method and the other occurrence around line 92);
ensure any downstream usage of `params` is typed or guarded (use typed
properties or type-narrowing) so callers and callers' code compile without
`any`.
| async addWorkItems(workspaceSlug: string, projectId: string, milestoneId: string, issueIds: string[]): Promise<void> { | ||
| return this.post<void>(`/workspaces/${workspaceSlug}/projects/${projectId}/milestones/${milestoneId}/work-items/`, { | ||
| issues: issueIds, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/api/Milestones.ts | head -90Repository: makeplane/plane-node-sdk
Length of output: 3413
🏁 Script executed:
# Check if "issues" is a backend API contract by looking at other API usages and responses
rg -n "issues" --type=ts src/api/ -B 2 -A 2 | head -100Repository: makeplane/plane-node-sdk
Length of output: 5317
🏁 Script executed:
# Check if there are any type definitions or models that define the payload structure
fd -e ts -e json . src/models src/types 2>/dev/null | head -20Repository: makeplane/plane-node-sdk
Length of output: 515
🏁 Script executed:
# Check Modules.ts to see the full pattern of how payload is structured
sed -n '75,90p' src/api/Modules.tsRepository: makeplane/plane-node-sdk
Length of output: 456
🏁 Script executed:
# Check Cycles.ts to see the same pattern
sed -n '100,110p' src/api/Cycles.tsRepository: makeplane/plane-node-sdk
Length of output: 346
🏁 Script executed:
# Search for any backend response or test fixtures that might show the expected payload structure
rg -n "work_items|work-items" --type=ts src/ | head -20Repository: makeplane/plane-node-sdk
Length of output: 2863
Rename "issue" parameter to "work item" terminology; keep backend API payload contract.
The parameter issueIds violates the coding guideline (never use "Issue" in names). Rename it to workItemIds to align with the rest of the codebase.
However, the payload key issues must remain unchanged—it is the backend API contract. Other similar methods in the codebase (Modules.ts, Cycles.ts) already follow this pattern: parameter uses workItemIds, but the payload key stays as issues.
♻️ Proposed fix
- async addWorkItems(workspaceSlug: string, projectId: string, milestoneId: string, issueIds: string[]): Promise<void> {
+ async addWorkItems(workspaceSlug: string, projectId: string, milestoneId: string, workItemIds: string[]): Promise<void> {
return this.post<void>(`/workspaces/${workspaceSlug}/projects/${projectId}/milestones/${milestoneId}/work-items/`, {
- issues: issueIds,
+ issues: workItemIds,
});
} async removeWorkItems(
workspaceSlug: string,
projectId: string,
milestoneId: string,
- issueIds: string[]
+ workItemIds: string[]
): Promise<void> {
return this.httpDelete(`/workspaces/${workspaceSlug}/projects/${projectId}/milestones/${milestoneId}/work-items/`, {
- issues: issueIds,
+ issues: workItemIds,
});
}Also applies to: 74-82
🤖 Prompt for AI Agents
In `@src/api/Milestones.ts` around lines 65 - 68, Rename the parameter issueIds to
workItemIds in the addWorkItems method signature (async
addWorkItems(workspaceSlug, projectId, milestoneId, workItemIds): Promise<void>)
and pass workItemIds as the value for the existing payload key "issues" when
calling this.post so the backend contract stays unchanged; also apply the same
rename-to-workItemIds-but-keep-payload-key="issues" pattern to the other
method(s) in this file around the 74-82 region to match Modules.ts/Cycles.ts
conventions.
2 participants
Description
New files
src/models/Milestone.ts—Milestone,MilestoneLite,CreateMilestoneRequest,UpdateMilestoneRequest,MilestoneWorkIteminterfacessrc/api/Milestones.ts—MilestonesAPI resource with 8 endpointstests/unit/milestone.test.ts— Unit tests covering full CRUD and work item operationsModified files
src/client/plane-client.ts— Registermilestonesresource onPlaneClientsrc/index.ts— ExportMilestonesAPI classsrc/models/index.ts— Export milestone model interfacesAPI Endpoints
/workspaces/{slug}/projects/{id}/milestones//workspaces/{slug}/projects/{id}/milestones//workspaces/{slug}/projects/{id}/milestones/{mid}//workspaces/{slug}/projects/{id}/milestones/{mid}//workspaces/{slug}/projects/{id}/milestones/{mid}//workspaces/{slug}/projects/{id}/milestones/{mid}/work-items//workspaces/{slug}/projects/{id}/milestones/{mid}/work-items//workspaces/{slug}/projects/{id}/milestones/{mid}/work-items/Type of Change
Test Scenarios
pnpm test:unitpasses with new milestone testsnpx tsc --noEmit)Summary by CodeRabbit
New Features
Tests