Repository navigation
fix: address bulk block review comments - #3363
Conversation
- share MIN/MAX bulk block constants via @llmgateway/shared so the admin UI gate cannot drift from what the API enforces - keep the preview dialog out of a stuck loading state if the preview server action rejects - keep the confirmation form visible after a failed bulk block and re-resolve the preview, so a retry confirms against current numbers instead of resubmitting a stale count Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SVus1fjS6z6xppaEai34Uj
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR centralizes bulk-block limits in the shared package. The API and admin UI consume these constants. The bulk-block button now tracks preview and block errors separately and reloads preview data after failed operations. ChangesBulk-block flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Admin as Organizations page
participant Button as BulkBlockOrgsButton
participant API as Admin bulk-block API
Admin->>Button: Open bulk-block dialog
Button->>API: Load organization preview
API-->>Button: Return preview or preview error
Admin->>Button: Confirm bulk block
Button->>API: Submit bulk-block request
API-->>Button: Return result or block error
Button->>API: Reload preview after block error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up robustness pass on the admin “bulk block filtered organizations” feature, focused on preventing UI/server drift for safety limits and improving error/loading handling in the admin confirmation dialog.
Changes:
- Centralizes bulk-block safety limits (
MIN_BULK_BLOCK_SEARCH_LENGTH,MAX_BULK_BLOCK_ORGANIZATIONS) in@llmgateway/sharedand updates API, admin UI, and tests to import them. - Refactors the admin bulk-block dialog to avoid getting stuck in a loading state when preview fails.
- Adjusts confirm behavior so the result summary only appears on success, and failures re-preview the current target set.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/index.ts | Re-exports the new shared bulk-block limit constants. |
| packages/shared/src/bulk-block.ts | Adds shared constants for bulk-block min search length and max org cap. |
| ee/admin/src/components/bulk-block-orgs-button.tsx | Improves preview loading/error handling and confirm flow robustness. |
| ee/admin/src/app/organizations/page.tsx | Uses shared MIN_BULK_BLOCK_SEARCH_LENGTH instead of a local hard-coded value. |
| apps/api/src/routes/admin.ts | Imports shared bulk-block limit constants and removes local duplicates. |
| apps/api/src/routes/admin-bulk-block.spec.ts | Imports shared max-cap constant to prevent test drift. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| const response = await onPreview(trimmedSearch); | ||
| if (response.success && response.preview) { | ||
| setPreview(response.preview); | ||
| } else { | ||
| setError(response.error ?? "Failed to preview bulk block"); | ||
| } |
There was a problem hiding this comment.
Fixed in bbd391f, though not by clearing the shared error — that would have broken the other half of this flow. handleConfirm deliberately calls loadPreview() after a failure, so clearing the error on a successful preview would have wiped the block error the reload is meant to accompany.
Split into previewError and blockError instead. loadPreview clears and sets only previewError, so a successful preview drops the stale preview error; the block error survives the reload and both render if they coexist.
Generated by Claude Code
![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
🤖 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 `@ee/admin/src/components/bulk-block-orgs-button.tsx`:
- Around line 109-114: Update the exception handling around onBulkBlock in the
bulk-block action to call await loadPreview() after setting the error, matching
the existing resolved-failure path. Ensure rejected requests refresh
preview.blockable before the next confirmation.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99ee324e-8754-44e4-8031-aa3280d883c2
📒 Files selected for processing (6)
apps/api/src/routes/admin-bulk-block.spec.tsapps/api/src/routes/admin.tsee/admin/src/app/organizations/page.tsxee/admin/src/components/bulk-block-orgs-button.tsxpackages/shared/src/bulk-block.tspackages/shared/src/index.ts
A failed block re-resolves the preview so the admin confirms against current numbers. With a single error state that reload cleared the block error it was meant to accompany, so preview and block failures are now tracked separately and rendered independently. Also re-resolve the preview when the block request throws: it may have been applied server-side before the connection failed, leaving preview.blockable stale for the next confirmation.
Follow-up to #3358, which merged before these review-comment fixes could be pushed. Three small robustness fixes; no change to the bulk-block safety guards themselves.
Changes
Share the bulk block limits instead of duplicating them (
packages/shared/src/bulk-block.ts)MIN_BULK_BLOCK_SEARCH_LENGTHandMAX_BULK_BLOCK_ORGANIZATIONSnow live in@llmgateway/shared, imported by both the API and the admin dashboard. Previously the UI hard-coded3with a comment asking future readers to keep it in sync — if the server value changed, the UI would silently show or hide the bulk action for filters the server disagrees about. The spec imports the shared constant too, so the over-cap test can no longer drift from the real limit.Don't strand the preview dialog in a loading state (
bulk-block-orgs-button.tsx)handleOpenawaited the preview server action without atry/finally. A rejection (network error, server crash) leftpreviewLoadingstuck attruewith no error surfaced. Now wrapped, so the loading flag always clears and the error is shown.Keep the confirmation form usable after a failed block (
bulk-block-orgs-button.tsx)handleConfirmsetresultunconditionally, which hid the preview and the count input while still rendering an enabled destructive button with a staleconfirmationMatches. A second click resubmitted the same stale count. Nowresultis set only on success; on failure the dialog stays on the confirmation step and re-resolves the preview, so the admin retypes against current numbers. The most likely failure is exactly the409the server returns when the set no longer matches the confirmed count, so re-previewing is what makes a retry meaningful.Testing
apps/api/src/routes/admin-bulk-block.spec.ts— 10/10 passing against mergedmain.turbo run build --filter=api --filter=admingreen.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements