feat: add strategy edit flow, notification center, and UI polish#17
feat: add strategy edit flow, notification center, and UI polish#17ianchen0119 merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds strategy editing capabilities, replaces the toast notification system with a persistent notification center, and includes UI improvements for Users and Roles management.
Changes:
- Implements backend and frontend strategy update flow with ownership validation and intent regeneration
- Replaces auto-dismissing toast notifications with a toggleable notification center that stores up to 50 notifications
- Enhances Users and Roles cards with expandable view, tabbed interface (roles/permissions), and automatic data loading
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| manager/service/strategy_svc.go | Adds UpdateScheduleStrategy service method with ownership validation, intent regeneration, and DM synchronization |
| manager/rest/strategy_hdl.go | Adds PUT /strategies handler with UpdateScheduleStrategyRequest DTO |
| manager/rest/routes.go | Registers PUT /strategies route with ScheduleStrategyUpdate permission check |
| manager/repository/strategy_repo.go | Adds UpdateStrategy repository method for MongoDB updates |
| manager/domain/interface.go | Adds UpdateStrategy to Repository interface and UpdateScheduleStrategy to Service interface |
| manager/domain/enums.go | Adds ScheduleStrategyUpdate permission key |
| manager/domain/mock_domain.go | Adds mock implementations for UpdateStrategy and UpdateScheduleStrategy |
| manager/migration/005_add_schedule_strategy_update_permission.up.json | Migration to add schedule_strategy.update permission and assign to admin role |
| manager/migration/005_add_schedule_strategy_update_permission.down.json | Rollback migration for schedule_strategy.update permission |
| web/src/styles/index.css | Adds notification center styles, small button variants, and user/role/permission display styles |
| web/src/context/AppContext.jsx | Updates toast system to include timestamps and clearToasts function, limits to 50 notifications |
| web/src/components/ToastContainer.jsx | Replaces toast component with notification center panel with persistent storage and manual dismiss |
| web/src/components/cards/StrategiesCard.jsx | Adds inline edit form for strategies with delete functionality per strategy |
| web/src/components/cards/UsersCard.jsx | Refactors to display users in expandable list with auto-load on authentication |
| web/src/components/cards/RolesCard.jsx | Refactors to show roles/permissions in tabbed view with expandable details and auto-load |
| web/src/components/cards/IntentsCard.jsx | Removes global delete button (unused import cleanup) |
| web/src/components/cards/HealthCard.jsx | Changes auto-refresh default from false to true |
| web/src/components/Header.jsx | Updates tagline from "eBPF Scheduler Control Interface" to "Next generation scheduling platform" |
| web/src/components/modals/DeleteStrategyModal.jsx | Deleted - functionality moved to inline strategy card actions |
| web/src/components/modals/DeleteIntentsModal.jsx | Deleted - functionality no longer needed |
| web/src/App.jsx | Removes deleted modal components from imports and render |
| web/README.md | Updates component structure documentation |
| getUsers(); | ||
| } | ||
| }, [isAuthenticated, makeAuthenticatedRequest]); | ||
| }, [isAuthenticated]); |
There was a problem hiding this comment.
The useEffect dependency array is missing getUsers. This violates the React exhaustive-deps rule. Since this callback is defined with useCallback and includes its dependencies, it should be added to the useEffect dependency array to ensure the effect re-runs when the callback changes. Either add it to the dependency array, or remove the useCallback wrapper if it's only used in this effect.
| }, [isAuthenticated]); | |
| }, [isAuthenticated, getUsers]); |
| func (svc *Service) UpdateScheduleStrategy(ctx context.Context, operator *domain.Claims, strategyID string, strategy *domain.ScheduleStrategy) error { | ||
| strategyObjID, err := bson.ObjectIDFromHex(strategyID) | ||
| if err != nil { | ||
| return errors.WithMessagef(err, "invalid strategy ID %s", strategyID) | ||
| } | ||
|
|
||
| operatorID, err := operator.GetBsonObjectUID() | ||
| if err != nil { | ||
| return errors.WithMessagef(err, "invalid operator ID %s", operator.UID) | ||
| } | ||
|
|
||
| // Validate ownership and load existing strategy | ||
| queryOpt := &domain.QueryStrategyOptions{ | ||
| IDs: []bson.ObjectID{strategyObjID}, | ||
| CreatorIDs: []bson.ObjectID{operatorID}, | ||
| } | ||
| if err := svc.Repo.QueryStrategies(ctx, queryOpt); err != nil { | ||
| return err | ||
| } | ||
| if len(queryOpt.Result) == 0 { | ||
| return errs.NewHTTPStatusError(http.StatusNotFound, "strategy not found or you don't have permission to update it", nil) | ||
| } | ||
| currentStrategy := queryOpt.Result[0] | ||
|
|
||
| // Query pods based on new strategy criteria before making changes | ||
| queryPodsOpt := &domain.QueryPodsOptions{ | ||
| K8SNamespace: strategy.K8sNamespace, | ||
| LabelSelectors: strategy.LabelSelectors, | ||
| CommandRegex: strategy.CommandRegex, | ||
| } | ||
| pods, err := svc.K8SAdapter.QueryPods(ctx, queryPodsOpt) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(pods) == 0 { | ||
| return errs.NewHTTPStatusError(http.StatusNotFound, "no pods match the strategy criteria", fmt.Errorf("no pods found for the given namespaces and label selectors, opts:%+v", queryPodsOpt)) | ||
| } | ||
|
|
||
| // Load existing intents for DM cleanup | ||
| oldIntentQuery := &domain.QueryIntentOptions{ | ||
| StrategyIDs: []bson.ObjectID{strategyObjID}, | ||
| } | ||
| if err := svc.Repo.QueryIntents(ctx, oldIntentQuery); err != nil { | ||
| return fmt.Errorf("query intents for strategy: %w", err) | ||
| } | ||
|
|
||
| // Update strategy document | ||
| now := time.Now().UnixMilli() | ||
| update := bson.M{ | ||
| "$set": bson.M{ | ||
| "strategyNamespace": strategy.StrategyNamespace, | ||
| "labelSelectors": strategy.LabelSelectors, | ||
| "k8sNamespace": strategy.K8sNamespace, | ||
| "commandRegex": strategy.CommandRegex, | ||
| "priority": strategy.Priority, | ||
| "executionTime": strategy.ExecutionTime, | ||
| "updaterID": operatorID, | ||
| "updatedTime": now, | ||
| }, | ||
| } | ||
| if err := svc.Repo.UpdateStrategy(ctx, strategyObjID, update); err != nil { | ||
| return fmt.Errorf("update strategy: %w", err) | ||
| } | ||
|
|
||
| // Replace intents for the strategy | ||
| if err := svc.Repo.DeleteIntentsByStrategyID(ctx, strategyObjID); err != nil { | ||
| return fmt.Errorf("delete intents by strategy ID: %w", err) | ||
| } | ||
|
|
||
| strategy.ID = strategyObjID | ||
| strategy.CreatedTime = currentStrategy.CreatedTime | ||
| strategy.CreatorID = currentStrategy.CreatorID | ||
| strategy.UpdaterID = operatorID | ||
| strategy.UpdatedTime = now | ||
|
|
||
| intents := make([]*domain.ScheduleIntent, 0, len(pods)) | ||
| nodeIDsMap := make(map[string]struct{}) | ||
| nodeIDs := make([]string, 0) | ||
| for _, pod := range pods { | ||
| intent := domain.NewScheduleIntent(strategy, pod) | ||
| intents = append(intents, &intent) | ||
| if _, exists := nodeIDsMap[pod.NodeID]; !exists { | ||
| nodeIDsMap[pod.NodeID] = struct{}{} | ||
| nodeIDs = append(nodeIDs, pod.NodeID) | ||
| } | ||
| } | ||
|
|
||
| if err := svc.Repo.InsertIntents(ctx, intents); err != nil { | ||
| return fmt.Errorf("insert intents into repository: %w", err) | ||
| } | ||
|
|
||
| // Notify decision makers to remove old intents | ||
| if len(oldIntentQuery.Result) > 0 { | ||
| oldNodeIDsMap := make(map[string]struct{}) | ||
| oldPodIDsMap := make(map[string]struct{}) | ||
| for _, intent := range oldIntentQuery.Result { | ||
| oldNodeIDsMap[intent.NodeID] = struct{}{} | ||
| oldPodIDsMap[intent.PodID] = struct{}{} | ||
| } | ||
| oldNodeIDs := make([]string, 0, len(oldNodeIDsMap)) | ||
| for nodeID := range oldNodeIDsMap { | ||
| oldNodeIDs = append(oldNodeIDs, nodeID) | ||
| } | ||
| oldPodIDs := make([]string, 0, len(oldPodIDsMap)) | ||
| for podID := range oldPodIDsMap { | ||
| oldPodIDs = append(oldPodIDs, podID) | ||
| } | ||
|
|
||
| dmLabel := domain.LabelSelector{ | ||
| Key: "app", | ||
| Value: "decisionmaker", | ||
| } | ||
| dmQueryOpt := &domain.QueryDecisionMakerPodsOptions{ | ||
| DecisionMakerLabel: dmLabel, | ||
| NodeIDs: oldNodeIDs, | ||
| } | ||
| dmPods, err := svc.K8SAdapter.QueryDecisionMakerPods(ctx, dmQueryOpt) | ||
| if err != nil { | ||
| logger.Logger(ctx).Warn().Err(err).Msg("failed to query decision maker pods for update deletion notification") | ||
| } else if len(oldPodIDs) > 0 { | ||
| deleteReq := &domain.DeleteIntentsRequest{PodIDs: oldPodIDs} | ||
| for _, dmPod := range dmPods { | ||
| if err := svc.DMAdapter.DeleteSchedulingIntents(ctx, dmPod, deleteReq); err != nil { | ||
| logger.Logger(ctx).Warn().Err(err).Msgf("failed to notify decision maker %s to delete intents", dmPod.NodeID) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Send new intents to decision makers | ||
| dmLabel := domain.LabelSelector{ | ||
| Key: "app", | ||
| Value: "decisionmaker", | ||
| } | ||
| dmQueryOpt := &domain.QueryDecisionMakerPodsOptions{ | ||
| DecisionMakerLabel: dmLabel, | ||
| NodeIDs: nodeIDs, | ||
| } | ||
| dms, err := svc.K8SAdapter.QueryDecisionMakerPods(ctx, dmQueryOpt) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(dms) == 0 { | ||
| logger.Logger(ctx).Warn().Msgf("no decision maker pods found for scheduling intents, opts:%+v", dmQueryOpt) | ||
| return nil | ||
| } | ||
|
|
||
| nodeIDIntentsMap := make(map[string][]*domain.ScheduleIntent) | ||
| nodeIDIntentIDsMap := make(map[string][]bson.ObjectID) | ||
| nodeIDDMap := make(map[string]*domain.DecisionMakerPod) | ||
| for _, dmPod := range dms { | ||
| for _, intent := range intents { | ||
| if intent.NodeID == dmPod.NodeID { | ||
| nodeIDIntentIDsMap[dmPod.Host] = append(nodeIDIntentIDsMap[dmPod.Host], intent.ID) | ||
| nodeIDIntentsMap[dmPod.Host] = append(nodeIDIntentsMap[dmPod.Host], intent) | ||
| nodeIDDMap[dmPod.Host] = dmPod | ||
| } | ||
| } | ||
| } | ||
| for host, intents := range nodeIDIntentsMap { | ||
| dmPod := nodeIDDMap[host] | ||
| err = svc.DMAdapter.SendSchedulingIntent(ctx, dmPod, intents) | ||
| if err != nil { | ||
| return fmt.Errorf("send scheduling intents to decision maker %s: %w", host, err) | ||
| } | ||
| err = svc.Repo.BatchUpdateIntentsState(ctx, nodeIDIntentIDsMap[host], domain.IntentStateSent) | ||
| if err != nil { | ||
| return fmt.Errorf("update intents state: %w", err) | ||
| } | ||
| logger.Logger(ctx).Info().Msgf("sent %d scheduling intents to decision maker %s", len(intents), host) | ||
| } | ||
|
|
||
| logger.Logger(ctx).Info().Msgf("updated strategy %s and regenerated intents", strategyID) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The UpdateScheduleStrategy service method lacks test coverage. While there are tests for CreateScheduleStrategy and DeleteScheduleStrategy in strategy_hdl_test.go, no test exists for the update flow. This is a critical operation that validates ownership, regenerates intents, and coordinates with Decision Makers - all aspects that should be verified through integration tests.
| func (r *repo) UpdateStrategy(ctx context.Context, strategyID bson.ObjectID, update bson.M) error { | ||
| if update == nil { | ||
| return errors.New("nil update") | ||
| } | ||
| _, err := r.db.Collection(scheduleStrategyCollection).UpdateOne(ctx, bson.M{"_id": strategyID}, update) | ||
| return err | ||
| } |
There was a problem hiding this comment.
The UpdateStrategy repository method doesn't validate that the update was successful. Unlike UpdateUser in repo_rbac.go which checks res.MatchedCount == 0 and returns domain.ErrNotFound, this method doesn't verify whether any document was actually modified. This could silently fail if the strategyID doesn't exist in the database (though the service layer does pre-validate).
| } | ||
| } | ||
|
|
||
| if err := svc.Repo.InsertIntents(ctx, intents); err != nil { |
There was a problem hiding this comment.
The UpdateScheduleStrategy operation is not atomic and could leave the system in an inconsistent state if it fails partway through. The sequence is: (1) update strategy document, (2) delete old intents, (3) insert new intents, (4) notify DMs. If steps 2-4 fail after step 1 succeeds, the strategy document will be updated but intents won't match. Consider wrapping the repository operations in a transaction or implementing compensating logic to rollback the strategy update on failure.
| if err := svc.Repo.InsertIntents(ctx, intents); err != nil { | |
| if err := svc.Repo.InsertIntents(ctx, intents); err != nil { | |
| // Best-effort rollback: try to restore previous intents if insertion of new intents fails. | |
| logger.Logger(ctx).Error().Err(err).Msg("failed to insert new schedule intents, attempting rollback to previous intents") | |
| if len(oldIntentQuery.Result) > 0 { | |
| if rollbackErr := svc.Repo.InsertIntents(ctx, oldIntentQuery.Result); rollbackErr != nil { | |
| logger.Logger(ctx).Error().Err(rollbackErr).Msg("failed to rollback to previous schedule intents after insert failure") | |
| } | |
| } |
| export default function ToastContainer() { | ||
| const { toasts, removeToast } = useApp(); | ||
| const { toasts, removeToast, clearToasts } = useApp(); | ||
| const [isOpen, setIsOpen] = useState(true); |
There was a problem hiding this comment.
The notification panel is always open by default (isOpen initialized to true). This creates a cluttered initial UX, especially on mobile where the panel takes full width. Consider initializing isOpen to false so users can opt-in to viewing notifications, or persist the open/closed state in localStorage.
| </button> | ||
| </div> | ||
| </div> | ||
| <div className="strategies-actions" style={{ display: 'flex' }}> |
There was a problem hiding this comment.
The inline style style={{ display: 'flex' }} is inconsistent with the codebase's CSS-first approach. All other styling in this file uses CSS classes. This should be moved to a CSS class like strategies-actions or the existing class definition should be updated to include display: flex.
| <div className="strategies-actions" style={{ display: 'flex' }}> | |
| <div className="strategies-actions"> |
| e.stopPropagation(); | ||
| handleDeleteStrategy(strategy.ID); | ||
| }} | ||
| title="Delete Strategy" |
There was a problem hiding this comment.
The disabled attribute on the edit button uses !isAuthenticated, but the delete button doesn't have a disabled attribute at all. This creates inconsistent UX where the edit button is disabled when not authenticated, but the delete button is always clickable. The delete button should also have disabled={!isAuthenticated} for consistency.
| title="Delete Strategy" | |
| title="Delete Strategy" | |
| disabled={!isAuthenticated} |
| getPermissions(); | ||
| } | ||
| }, [isAuthenticated, makeAuthenticatedRequest]); | ||
| }, [isAuthenticated]); |
There was a problem hiding this comment.
The useEffect dependencies are missing getRoles and getPermissions. This violates the React exhaustive-deps rule. Since these callbacks are defined with useCallback and include their dependencies, they should be added to the useEffect dependency array to ensure the effect re-runs when the callbacks change. Either add them to the dependency array, or remove the useCallback wrapper if they're only used in this effect.
| }, [isAuthenticated]); | |
| }, [isAuthenticated, getRoles, getPermissions]); |
No description provided.