feat(connections): re-assign widgets to a different connection (#510) - #588
Merged
Conversation
Closes #510 — users can now migrate widgets to another connection instead of having to "Delete anyway" and break them. Server: - POST /api/connections/{id}/reassign with { targetConnectionId } - reassignConnectionWidgets() walks editable dashboards (owner + editor shares, admin sees all-in-tenant) and swaps connectionId on matching widgets via a single jsonb_set UPDATE - Guards: source ownership, target exists in same tenant, same type - Returns { dashboardsUpdated, widgetsReassigned } Client: - useReassignConnection hook - Delete-connection dialog exposes a "Re-assign widgets to another connection…" button when the connection is in use - Secondary dialog lists compatible (same-type) connections; shows a helpful message when none are available Tests: - 10 new route tests (auth, validation, ownership, type-mismatch, admin-path, happy-path, error propagation) - Full suite: 2036 pass Closes #510 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 11 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR implements a feature to re-assign widgets from one connection to another before deletion. It adds a new API endpoint ( Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Frontend Dialog
participant API as API Route<br/>/reassign
participant DB as Database
User->>UI: Selects target connection
UI->>UI: Validate selection
User->>UI: Clicks "Re-assign"
UI->>API: POST /api/connections/{id}/reassign<br/>{ targetConnectionId }
rect rgba(100, 150, 200, 0.5)
API->>API: Authenticate user
API->>API: Validate body & rules
API->>DB: Check source/target exist<br/>& ownership
API->>DB: Verify type match
end
DB->>DB: Update layoutJson:<br/>swap connectionId across<br/>editable dashboards
DB-->>API: ReassignResult
API-->>UI: { dashboardsUpdated,<br/>widgetsReassigned }
UI->>UI: Close dialog
UI->>UI: Invalidate queries
UI-->>User: Success feedback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/app/api/connections/[id]/reassign/__tests__/route.test.ts (1)
54-58: Avoidanyfor the route handler type.The exported route type is available, so this can stay strict without an eslint suppression.
♻️ Proposed fix
- let POST: ( - req: Request, - ctx: { params: Promise<{ id: string }> }, - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - ) => Promise<any>; + let POST: typeof import("../route").POST;As per coding guidelines,
**/*.{ts,tsx}: TypeScript strict mode enforced. Noanytype without a comment explaining why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/app/api/connections/`[id]/reassign/__tests__/route.test.ts around lines 54 - 58, The test declares a POST variable using Promise<any> and disables eslint; instead import and use the exported route handler type from the route implementation so the type is strict. Replace the ad-hoc signature for the test-level POST with the actual exported POST type from the route module (reference the test variable name POST and the exported POST in the route file) and remove the eslint-disable comment; ensure the ctx param type matches the route's declared params type rather than using any.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/app/api/connections/`[id]/reassign/route.ts:
- Around line 43-45: The route currently only checks role/isAdmin (via
requireSession() and isAdmin) but does not enforce the user's can_write
permission before performing the mutation; add a server-side check using the
session's canWrite (or can_write) property immediately after obtaining the
session (where requireSession() is called) and before any reassign logic
(including the code around the reassign handlers referenced later at lines
~94-100), and if can_write is false return a 403/forbidden response (or throw an
appropriate error) to prevent read-only sessions from modifying dashboard/widget
layout.
In `@app/src/lib/db/connection-reassign.ts`:
- Around line 13-39: The SQL predicate in editableDashboardsScope incorrectly
checks for an 'owner' share role (s.role IN ('editor', 'owner')) even though
shareRoleEnum only permits 'viewer' and 'editor'; update the WHERE clause in
editableDashboardsScope to remove 'owner' (use s.role IN ('editor') or s.role =
'editor' as appropriate) and make the same change to any other SQL that queries
the "dashboard_share" table for s.role IN ('editor','owner') (e.g., the
reassignment/query logic that references "dashboard_share" and s.role) so the
query uses only valid roles.
- Around line 120-152: In the UPDATE "dashboard" statement (the db.execute block
that sets "layoutJson"), wrap the inner jsonb_agg that builds the widgets array
with COALESCE(..., '[]'::jsonb) so pages with no widgets get an empty array
instead of null; also preserve array order by changing the
jsonb_array_elements(...) calls to use WITH ORDINALITY and re-aggregate ordering
(use the ordinality index in the outer jsonb_agg ORDER BY) for both the widgets
aggregation and the pages aggregation to maintain original ordering when calling
jsonb_set on page->'widgets' and when aggregating pages.
---
Nitpick comments:
In `@app/src/app/api/connections/`[id]/reassign/__tests__/route.test.ts:
- Around line 54-58: The test declares a POST variable using Promise<any> and
disables eslint; instead import and use the exported route handler type from the
route implementation so the type is strict. Replace the ad-hoc signature for the
test-level POST with the actual exported POST type from the route module
(reference the test variable name POST and the exported POST in the route file)
and remove the eslint-disable comment; ensure the ctx param type matches the
route's declared params type rather than using any.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28553b97-8756-4ee2-80b8-ace571f4c751
📒 Files selected for processing (5)
app/src/app/(dashboard)/connections/page.tsxapp/src/app/api/connections/[id]/reassign/__tests__/route.test.tsapp/src/app/api/connections/[id]/reassign/route.tsapp/src/hooks/use-connections.tsapp/src/lib/db/connection-reassign.ts
8 new tests: same-id no-op, unused-source no-op, non-admin + admin success paths, null/empty count handling, error propagation from both the count and UPDATE queries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Users can now migrate widgets to another connection instead of having to pick between "Leave alone" or "Delete anyway, break them all".
Server
POST /api/connections/{id}/reassignbody{ targetConnectionId }reassignConnectionWidgets()walks editable dashboards (owner + editor-shared; admin sees all-in-tenant) and swapsconnectionIdvia a singlejsonb_setUPDATEtype(Cypher won't run on PG and vice versa){ dashboardsUpdated, widgetsReassigned }Client
useReassignConnectionhook (invalidatesconnection-usage+dashboardson success)Scope notes (from issue)
Test plan
Closes #510
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests