Fix: Edit Cluster Target Repository #152
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe changes introduce cluster update functionality with a new ClusterUpdate model and PATCH endpoint to modify mutable cluster fields (github_repo_url). A frontend component now supports inline editing of the Target Repo field, with a corresponding API route handler to forward updates to the backend. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as ClusterHeader
participant APIRoute as /api/clusters/[id]
participant Backend as /clusters/{id} PATCH
User->>Frontend: Click Edit on Target Repo
Frontend->>Frontend: Show input field
User->>Frontend: Enter repo URL & press Enter
Frontend->>APIRoute: PATCH /api/clusters/{id}<br/>(ClusterUpdate payload)
APIRoute->>Backend: Forward PATCH request<br/>(with projectId header)
Backend->>Backend: Validate cluster exists<br/>Apply updates<br/>Set updated_at
Backend-->>APIRoute: Return updated cluster
APIRoute-->>Frontend: Return cluster data
Frontend->>Frontend: Update local state<br/>Reload display
Frontend-->>User: Show updated repo link
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly Related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (3)
backend/tests/test_planner.py (1)
68-68: Consider removing or replacing the commented assertion.Commented-out code should generally be removed. If the error description format changed to be user-friendly, consider adding an assertion that verifies the new expected behavior (e.g., checking for a generic error message pattern) rather than leaving dead code.
🔎 Suggested options
Option 1: Remove the commented assertion entirely if the title check is sufficient:
plan = generate_plan(cluster, []) assert plan.title.startswith("Error planning fix") - # assert "API Broken" in plan.description # User-friendly message hides detailsOption 2: Replace with a positive assertion for the new behavior:
plan = generate_plan(cluster, []) assert plan.title.startswith("Error planning fix") - # assert "API Broken" in plan.description # User-friendly message hides details + assert plan.description # Verify error description is populatedbackend/main.py (1)
1832-1855: LGTM with optional enhancement suggestion.The PATCH endpoint correctly:
- Validates project ownership
- Uses
exclude_unset=Truefor proper partial update semantics- Auto-updates
updated_attimestamp- Returns the updated cluster
🔎 Optional: Add URL format validation
Consider validating
github_repo_urlformat to prevent storing malformed URLs:+import re + +_GITHUB_URL_PATTERN = re.compile(r"^https?://github\.com/[^/\s]+/[^/\s]+/?$", re.IGNORECASE) @app.patch("/clusters/{cluster_id}") def update_cluster_details( cluster_id: str, payload: ClusterUpdate, project_id: Optional[str] = Query(None), ): pid = _require_project_id(project_id) pid_str = str(pid) cluster = get_cluster(pid_str, cluster_id) if not cluster: raise HTTPException(status_code=404, detail="Cluster not found") updates = payload.model_dump(exclude_unset=True) if not updates: return {"status": "ok", "id": cluster_id, "project_id": pid_str} + # Validate github_repo_url format if provided + if "github_repo_url" in updates and updates["github_repo_url"]: + if not _GITHUB_URL_PATTERN.match(updates["github_repo_url"]): + raise HTTPException(status_code=400, detail="Invalid GitHub repository URL format") + updates["updated_at"] = datetime.now(timezone.utc) updated_cluster = update_cluster(pid_str, cluster_id, **updates) return updated_clusterAlternatively, use Pydantic's
field_validatoronClusterUpdatefor cleaner validation.dashboard/app/(dashboard)/dashboard/clusters/[id]/components/ClusterHeader.tsx (1)
123-144: Consider avoiding full page reload on successful save.Using
window.location.reload()(line 133) works but discards any client-side state and requires a full round-trip. A better approach would be to pass anonUpdatecallback prop or use a state management solution to refresh the cluster data.🔎 Suggested approach
Add a callback prop to notify the parent of updates:
interface ClusterHeaderProps { cluster: ClusterDetail; selectedRepo: string | null; onRepoSelect: (repo: string | null) => void; + onClusterUpdate?: () => void; } export default function ClusterHeader({ cluster, selectedRepo, onRepoSelect, + onClusterUpdate, }: ClusterHeaderProps) {Then replace the reload:
- window.location.reload(); + onClusterUpdate?.();The parent page can then re-fetch or use SWR/React Query's
mutate()to refresh the data.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dashboard/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
backend/main.py(2 hunks)backend/tests/test_datadog_integration.py(1 hunks)backend/tests/test_planner.py(1 hunks)dashboard/app/(dashboard)/dashboard/clusters/[id]/components/ClusterHeader.tsx(3 hunks)dashboard/app/api/clusters/[id]/route.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
backend/**/*.py: Use FastAPI for backend API routes handling ingestion, clustering, and job orchestration
Use Pydantic models for data validation in FastAPI (e.g.,FeedbackItem,IssueCluster,AgentJob)
Use redis-py and Upstash REST API for Redis operations with abstraction instore.py
backend/**/*.py: Backend code must follow Black code formatting and Ruff linting standards
Use snake_case for Python function and module names in backend
Use PascalCase for Pydantic model names in backend
Use UPPER_SNAKE_CASE for constants in backend
Files:
backend/tests/test_planner.pybackend/tests/test_datadog_integration.pybackend/main.py
backend/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Run backend tests using pytest with proper test file organization in
backend/tests/Backend test files should be organized in
backend/tests/directory and run withpytest backend/tests -q --cov=backend
Files:
backend/tests/test_planner.pybackend/tests/test_datadog_integration.py
dashboard/app/**
📄 CodeRabbit inference engine (CLAUDE.md)
Use Next.js 16 App Router for dashboard routes and API endpoints
Files:
dashboard/app/api/clusters/[id]/route.tsdashboard/app/(dashboard)/dashboard/clusters/[id]/components/ClusterHeader.tsx
dashboard/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
dashboard/**/*.{ts,tsx}: Use TypeScript for all dashboard code
Use Tailwind CSS for styling in the dashboard
Files:
dashboard/app/api/clusters/[id]/route.tsdashboard/app/(dashboard)/dashboard/clusters/[id]/components/ClusterHeader.tsx
dashboard/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint and Prettier for dashboard code formatting and linting
dashboard/**/*.{ts,tsx,js,jsx}: Dashboard React components must use PascalCase naming
Dashboard code must follow ESLint and Prettier formatting standards
Colocate Tailwind styling with React components in dashboard
Files:
dashboard/app/api/clusters/[id]/route.tsdashboard/app/(dashboard)/dashboard/clusters/[id]/components/ClusterHeader.tsx
backend/main.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend FastAPI application entry point is
main.pywithappASGI application
Files:
backend/main.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: altock/soulcaster PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-20T23:31:28.182Z
Learning: Use Conventional Commits with subject line <=72 characters and reference cluster IDs/GitHub issues when relevant
📚 Learning: 2025-12-20T23:31:28.182Z
Learnt from: CR
Repo: altock/soulcaster PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-20T23:31:28.182Z
Learning: Applies to dashboard/app/api/clusters/run*.{ts,js} : Dashboard-triggered clustering routes (`/api/clusters/run*`) return 410 status unless `ENABLE_DASHBOARD_CLUSTERING=true` is explicitly set
Applied to files:
dashboard/app/api/clusters/[id]/route.ts
🧬 Code graph analysis (2)
dashboard/app/api/clusters/[id]/route.ts (2)
dashboard/lib/project.ts (1)
requireProjectId(42-48)dashboard/scripts/reset_clusters.js (2)
response(50-50)fetch(4-4)
backend/main.py (1)
backend/store.py (6)
get_cluster(486-496)get_cluster(1642-1683)get_cluster(2709-2713)update_cluster(506-512)update_cluster(1713-1718)update_cluster(2745-2746)
🔇 Additional comments (4)
backend/tests/test_datadog_integration.py (1)
221-221: LGTM!The assertion correctly aligns with the backend implementation in
main.pywhich returns"filtered"status when a monitor is not in the configured list (lines 517-521).backend/main.py (1)
1658-1660: LGTM!The
ClusterUpdatemodel follows project conventions (PascalCase, Pydantic BaseModel) and is appropriately minimal for partial updates.dashboard/app/(dashboard)/dashboard/clusters/[id]/components/ClusterHeader.tsx (2)
17-19: Good state management setup.The three state variables cleanly separate edit mode, input value, and loading state.
163-192: LGTM!The non-editing state display is well implemented:
- Shows formatted repo link or "None set" placeholder
- Edit button appears on hover with good accessibility (title attribute)
- Clean visual hierarchy
| export async function PATCH(request: Request, { params }: { params: Promise<{ id: string }> }) { | ||
| try { | ||
| const { id } = await params; | ||
| const projectId = await requireProjectId(request); | ||
| const body = await request.json(); | ||
|
|
||
| if (!id || !/^[a-zA-Z0-9-]+$/.test(id)) { | ||
| return NextResponse.json({ error: 'Invalid cluster ID' }, { status: 400 }); | ||
| } | ||
|
|
||
| const response = await fetch(`${backendUrl}/clusters/${id}?project_id=${projectId}`, { | ||
| method: 'PATCH', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| console.error(`Backend returned ${response.status} for cluster update ${id}: ${errorText}`); | ||
| return NextResponse.json({ error: 'Failed to update cluster' }, { status: response.status }); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| return NextResponse.json(data); | ||
| } catch (error: any) { | ||
| console.error('Error updating cluster:', error); | ||
| return NextResponse.json({ error: 'Failed to update cluster' }, { status: 500 }); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Add timeout and align error handling with GET handler.
The PATCH handler is missing:
- Request timeout (GET uses
AbortSignal.timeout(10000)) - Specific handling for
project_id is requirederror - Timeout error handling
🔎 Suggested fix
export async function PATCH(request: Request, { params }: { params: Promise<{ id: string }> }) {
try {
const { id } = await params;
const projectId = await requireProjectId(request);
const body = await request.json();
if (!id || !/^[a-zA-Z0-9-]+$/.test(id)) {
return NextResponse.json({ error: 'Invalid cluster ID' }, { status: 400 });
}
const response = await fetch(`${backendUrl}/clusters/${id}?project_id=${projectId}`, {
method: 'PATCH',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify(body),
+ signal: AbortSignal.timeout(10000),
});
if (!response.ok) {
const errorText = await response.text();
console.error(`Backend returned ${response.status} for cluster update ${id}: ${errorText}`);
- return NextResponse.json({ error: 'Failed to update cluster' }, { status: response.status });
+ const status = response.status >= 500 ? 502 : response.status;
+ return NextResponse.json({ error: 'Failed to update cluster' }, { status });
}
const data = await response.json();
return NextResponse.json(data);
} catch (error: any) {
+ if (error?.message === 'project_id is required') {
+ return NextResponse.json({ error: 'project_id is required' }, { status: 400 });
+ }
+ if (error?.name === 'AbortError' || error?.message?.includes('timeout')) {
+ return NextResponse.json({ error: 'Backend request timed out' }, { status: 503 });
+ }
console.error('Error updating cluster:', error);
return NextResponse.json({ error: 'Failed to update cluster' }, { status: 500 });
}
}🤖 Prompt for AI Agents
In dashboard/app/api/clusters/[id]/route.ts around lines 46 to 76, the PATCH
handler needs the same timeout and aligned error handling as the GET: add
AbortSignal.timeout(10000) to the fetch call, pass it as the signal option, and
after a non-ok response inspect the response text; if it includes "project_id is
required" return a 400 with that message, otherwise keep returning the backend
status with a generic failure message. In the catch block detect a timeout/abort
(error.name === 'AbortError' or equivalent) and return a 504 with a
timeout-specific error, and for other errors return 500 as before. Ensure the
fetch headers/body stay the same and the timeout signal is cleaned up by using
AbortSignal.timeout(10000) directly in the fetch options.
1 participant
Edit Cluster Target Repository
Summary
This pull request addresses the need for users to manage the target GitHub repository URL associated with a cluster. Previously, associating feedback with specific code repositories was challenging, especially when the source didn't explicitly provide this information.
This change introduces the ability to add and edit a cluster's
github_repo_urldirectly from the cluster detail page. This enables accurate categorization of feedback by linking it to its intended target repository, improving the overall feedback management workflow.Changes Made
backend/main.py:ClusterUpdatePydantic model for updating cluster fields.PATCH /clusters/{cluster_id}endpoint to allow partial updates of cluster details, specifically thegithub_repo_url.backend/tests/test_datadog_integration.py:status == "skipped"tostatus == "filtered"to align with a minor behavior change.backend/tests/test_planner.py:dashboard/app/(dashboard)/dashboard/clusters/[id]/components/ClusterHeader.tsx:github_repo_url(if present) for a cluster.github_repo_urlby making aPATCHrequest to the new backend endpoint.Test Plan
Navigate to a Cluster Detail Page:
/dashboard/clusters/[id]for any existing cluster.Verify "Target Repo" Section:
Test Adding a Repository URL:
github_repo_urlis currently set for the cluster, you should see "None set".https://github.com/myorg/myrepo).Enter.myorg/myrepo).Test Editing an Existing Repository URL:
github_repo_urlis already set, hover over it and click the "Edit" icon.Enter.Test Cancelling Edit:
Escape.Test Clearing a Repository URL:
github_repo_url.Enter.github_repo_urlis removed and "None set" is displayed.Verify Link Functionality:
github_repo_url.Backend Verification (Optional):
PATCHrequest to/api/clusters/{cluster_id}with a JSON body like{"github_repo_url": "https://github.com/newowner/newrepo"}.github_repo_urlis updated in the database and reflected in subsequentGET /clusters/{cluster_id}responses.updated_attimestamp for the cluster is also updated.🤖 Generated with Soulcaster Agent