Skip to content
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

Feat/multiple delete subscriber profile #124

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Alonza0314
Copy link
Contributor

Added multiple delete button on both subscriber and profile list pages.

subscriber-webconsole-multiple-delete-button subscriber-webconsole-multiple-delete-confirm-box profile-webconsole-multiple-delete-button profile-webconsole-multiple-confirm-box

@Alonza0314 Alonza0314 marked this pull request as ready for review December 12, 2024 19:02
@Alonza0314 Alonza0314 requested a review from andy89923 December 12, 2024 19:02
@andy89923 andy89923 requested a review from yccodr December 13, 2024 02:30
@yccodr
Copy link
Collaborator

yccodr commented Dec 13, 2024

Here's a review from Claude.ai


The PR introduces several key improvements to the free5GC WebConsole frontend, focusing on adding bulk delete functionality for subscribers and profiles. Here are my observations and recommendations:

Key Changes

  • Added multiple delete support for both Subscribers and Profiles
  • Introduced new TypeScript interface and formatting utility functions
  • Enhanced UI with checkbox selection for bulk actions
  • Updated backend routes and API methods to support multiple deletions

Code Review Highlights

  1. JSON Formatting Utility

    The new jsonFormating.ts is an excellent addition:

    export function formatMultipleDeleteSubscriberToJson(subscribers: MultipleDeleteSubscriberData[]) {
      return subscribers.map(sub => ({
        ueId: sub.ueId,
        plmnID: sub.plmnID
      }));
    }

    ✅ Good Practice: Separating JSON formatting logic increases maintainability

  2. Selection Handling in Lists
    The implementation of selection logic in SubscriberList and ProfileList is robust:

    const handleSelectAllClick = (event: React.ChangeEvent<HTMLInputElement>) => {
      if (event.target.checked) {
        const newSelected = filteredData.map(row => ({
          ueId: row.ueId!,
          plmnID: row.plmnID!
        }));
        setSelected(newSelected);
        return;
      }
      setSelected([]);
    };

    ✅ Good Practice: Comprehensive selection management with checkbox interactions

Recommendations

  1. Error Handling

    Consider creating a centralized error handling mechanism. Current error handling is repetitive:

    .catch((err) => {
      setIsDeleteError(true);
      if (err.response) {
        const msg = "Status: " + err.response.status;
        if (err.response.data.cause) {
          alert(msg + ", cause: " + err.response.data.cause);
        } else {
          alert(msg);
        }
      } else {
        alert(err.message);
      }
      console.log(err);
      return;
    });

    Suggestion: Create a utility function like handleApiError(err) to standardize error processing.

  2. Confirmation Dialog

    Current implementation uses window.confirm(), which is basic:

    const confirmMessage = `Are you sure you want to delete...`;
    const result = window.confirm(confirmMessage);

    Suggestion: Replace with a more user-friendly Material-UI dialog for better UX.

  3. Performance Optimization

    For large lists, consider implementing:

    • Virtualization (react-window/react-virtualized)
    • Pagination improvements
    • Memoization of list items
  4. TypeScript Improvements

    • Add more comprehensive type definitions
    • Use stricter null checks
    • Consider using discriminated unions for API responses
  5. Accessibility

    • Ensure proper keyboard navigation for checkboxes
    • Add appropriate ARIA attributes
    • Improve color contrast for selected rows

Security Considerations

✅ Good: Requires authentication token for deletion
✅ Good: Server-side validation of deletion requests

Backend Observations

The backend changes in api_webui.go demonstrate careful implementation of multiple delete operations with appropriate error logging.

Overall Assessment

Strengths:

  • Clean implementation of bulk delete
  • Good separation of concerns
  • TypeScript type safety
  • Consistent UI pattern

Areas for Potential Improvement:

  • Error handling standardization
  • More sophisticated confirmation dialogs
  • Performance optimization for large datasets

Recommendation

👍 Approve PR with Minor Suggestions
The changes significantly improve user experience by adding bulk management capabilities. Implement the suggested optimizations in future iterations.

@yccodr
Copy link
Collaborator

yccodr commented Dec 13, 2024

For FE error handling, you can check out MUI Snackbar, which provides a more user-friendly UI. We used it in this project as well.

@Alonza0314
Copy link
Contributor Author

For FE error handling, you can check out MUI Snackbar, which provides a more user-friendly UI. We used it in this project as well.

I have updated the API error for multiple deletes to reuse the Snackbar error display from single delete. I also tried replacing the window.confirm part with a Snackbar (as suggested in the review to use other UI options), but I feel it looks very user-unfriendly. So, for now, I’ve kept the direct use of window.confirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants