-
Notifications
You must be signed in to change notification settings - Fork 319
feat: Add dashboard duplication feature #1264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Implements the ability to duplicate dashboards from the options dropdown menu. As per requirements: - Alerts are NOT copied to the duplicated dashboard - All tile IDs are regenerated to be unique - Dashboard name gets (Copy) suffix - All tiles, tags, and filters are copied Backend changes: - Added duplicateDashboard controller function in dashboard.ts - Added POST /dashboards/:id/duplicate API endpoint Frontend changes: - Added useDuplicateDashboard hook in dashboard.ts - Added Duplicate Dashboard menu item to options dropdown - Shows success/error notifications and redirects to new dashboard Tests: - Added comprehensive test coverage for dashboard duplication - Tests verify unique tile IDs - Tests verify alerts are not copied - Tests verify filters are preserved - Tests verify 404 for non-existent dashboards Closes #1253 Co-authored-by: Tom Alexander <[email protected]>
🦋 Changeset detectedLatest commit: 630510d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Pull Request Review: Dashboard Duplication FeatureThank you for implementing this feature! The implementation is generally solid and well-tested. Below are my findings: Strengths
Critical Issues1. Weak Tile ID Generation Algorithm (dashboard.ts:206) The current tile ID generation using Math.random().toString(36) has collision risks. With Math.random(), collision probability increases with more tiles and there is no guarantee of uniqueness across the system. Recommendation: Use crypto.randomBytes() or MongoDB ObjectId for more robust ID generation. Consider creating a shared utility function in packages/common-utils. Medium Priority Issues2. Inconsistent Error Handling (dashboards.ts:123) The route handler checks if the dashboard exists twice - once in the router and once in the controller. Remove the redundant check. 3. Tile Data Structure Spread (dashboard.ts:204-212) Using spread operator ...tile may inadvertently copy Mongoose document metadata. Use toObject() before spreading. 4. Missing Team-Based Access Control Test Tests don't verify that users can't duplicate dashboards from other teams. Add a test case for team isolation. Minor Suggestions
SecurityNo security concerns identified. Implementation properly uses team-based access control, validates input with Zod schemas, and follows authentication middleware patterns. SummaryOverall Assessment: Good implementation with solid test coverage. Main concern is the tile ID generation algorithm. Recommendation: Address the tile ID generation issue before merging. Other issues can be addressed in follow-up PRs. Estimated Risk: Low-Medium (due to potential tile ID collisions) |
E2E Test Results✅ All tests passed • 25 passed • 3 skipped • 223s
|
@claude run |
PR Review: Dashboard Duplication FeatureThanks for implementing this feature! Overall, the implementation is solid and well-tested. Here's my feedback: ✅ Strengths
🔍 Issues & Recommendations1. Tile ID Generation Consistency (Minor)Location: The tile ID generation uses: id: Math.floor(100000000 * Math.random()).toString(36) This matches the frontend pattern in
Recommendation: Consider using a more robust ID generation approach: import { randomBytes } from 'crypto';
// Option 1: Use crypto module
id: randomBytes(8).toString('hex')
// Option 2: Use ObjectId-style generation
id: new mongoose.Types.ObjectId().toString() While collision probability is low with the current approach, using a cryptographically secure method would be more robust for production. 2. getDashboard Return Type Inconsistency (Minor)Location: The duplicate endpoint calls Current code: const dashboard = await getDashboard(dashboardId, teamId);
if (dashboard == null) {
return res.sendStatus(404);
}
const newDashboard = await duplicateDashboard(dashboardId, teamId, userId); Recommendation: Remove the redundant const newDashboard = await duplicateDashboard(dashboardId, teamId, userId);
if (newDashboard == null) {
return res.sendStatus(404);
}
res.json(newDashboard.toJSON()); Then update the controller to return export async function duplicateDashboard(...) {
const dashboard = await Dashboard.findOne({
_id: dashboardId,
team: teamId,
});
if (dashboard == null) {
return null; // Instead of throwing
}
// ... rest of implementation
} 3. Alert Removal Implementation (Observation)Location: The current implementation manually removes alerts from tile configs: config: {
...tile.config,
alert: undefined,
} This works correctly, but consider documenting WHY alerts aren't copied. The comment says "per requirement" but future developers might wonder about the business logic. Recommendation: Add a more descriptive comment: // Alerts are intentionally excluded from duplicated dashboards to prevent
// unintended notification spam. Users must explicitly configure alerts
// for the new dashboard if needed.
config: {
...tile.config,
alert: undefined,
} 4. Error Handling in Frontend (Minor)Location: The error notification shows a generic message. Consider including the actual error message for better debugging: onError: (error) => {
notifications.show({
color: 'red',
title: 'Failed to duplicate dashboard',
message: error?.message || 'An error occurred while duplicating the dashboard',
});
} 🎯 Minor Observations
🔒 Security✅ No security concerns. The implementation properly:
📊 Performance✅ No performance concerns. The operation is:
🎨 Code Style✅ Follows HyperDX conventions:
📝 VerdictApprove with minor suggestions. The core implementation is solid and meets all requirements. The issues mentioned above are minor improvements that could be addressed in a follow-up or before merging, but they don't block the PR. Great work on the comprehensive test coverage! 🎉 |
Co-authored-by: Tom Alexander <[email protected]>
Implements the ability to duplicate dashboards from the options dropdown menu.
Closes #1253
Changes
Requirements Met
Generated with Claude Code