Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive staff management system with role-based access control (RBAC), adding team management capabilities for managers and redesigning the staff dashboard UI with a cleaner, more modern aesthetic.
Changes:
- Added staff management modals for adding and managing team members with role-based permissions
- Implemented staff profile context with manager/staff role differentiation
- Redesigned staff dashboard and layout with minimalist UI components and improved animations
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/ManageTeamModal.tsx | New modal component for viewing, editing, and deleting team members with role-based actions |
| src/components/AddStaffModal.tsx | New modal component for inviting staff members via email |
| context/AppContext.tsx | Added staff profile state management and enhanced orders loading with role checks |
| Pages/StaffDashboard.tsx | Complete UI overhaul with stats cards, manager actions, and team management features |
| Pages/Splash.tsx | Simplified splash screen animations from 3D effects to fade transitions |
| Pages/Auth.tsx | Enhanced authentication flow with staff profile fetching and activation |
| Layouts/StaffLayout.tsx | Redesigned staff layout with minimalist navigation and floating action button |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| const saveEdit = async (uid: string) => { | ||
| if (!editEmail.includes("@")) { |
There was a problem hiding this comment.
The email validation check only verifies that the email contains an "@" character, which is insufficient. This allows invalid email formats like "@", "user@", or "@domain.com" to pass validation. Consider using a more robust email validation pattern or a library like validator.js to ensure the email format is valid.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const token = await auth.currentUser?.getIdToken(true); | ||
| if (!token) return; | ||
|
|
||
| const res = await fetch("http://localhost:8000/user/orders", { |
There was a problem hiding this comment.
The API endpoint uses a hardcoded localhost URL which will fail in production. This should be replaced with an environment variable or a configurable base URL to support different deployment environments. Consider using process.env.REACT_APP_API_URL or a similar configuration approach.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| }; | ||
|
|
||
| const handleDelete = async (uid: string) => { | ||
| if (!confirm("Are you sure you want to remove this staff member?")) return; |
There was a problem hiding this comment.
Using the native confirm dialog creates an inconsistent user experience in a modern React application. The confirm dialog blocks the main thread and doesn't match the polished UI design of the rest of the application. Consider implementing a custom modal confirmation dialog for consistency.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| await api.post( | ||
| '/auth/verify-staff', | ||
| null, | ||
| { headers: { Authorization: `Bearer ${token}` } } | ||
| ) | ||
| await api.post( | ||
| '/staff/activate', | ||
| null, | ||
| { headers: { Authorization: `Bearer ${token}` } } | ||
| ) | ||
|
|
||
| // Fetch staff profile | ||
| const res = await api.get( | ||
| '/staff/me', | ||
| { headers: { Authorization: `Bearer ${token}` } } | ||
| ) | ||
|
|
||
| const profile = res.data | ||
|
|
||
| setStaffProfile({ | ||
| role: profile.role, | ||
| stallId: profile.stall_id, | ||
| email: profile.email, | ||
| }) |
There was a problem hiding this comment.
Multiple API calls are made sequentially without proper error handling between them. If 'verify-staff' succeeds but 'activate' fails, the user will be left in an inconsistent state. Consider wrapping all related API calls in a transaction-like pattern or implement proper rollback/cleanup logic if any step fails.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const StaffDashboard: React.FC = () => { | ||
| const { deals, orders, cafeterias, managedCafeteriaId, toggleCafeteriaStatus } = useApp() | ||
| const [backendOrders, setBackendOrders] = useState<any[]>([]) | ||
| const [loading, setLoading] = useState(true) |
There was a problem hiding this comment.
Unused variable loading.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…-again Replace insufficient email validation with regex pattern
Remove unused loading state and Settings import from StaffDashboard
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 15 comments.
Comments suppressed due to low confidence (1)
Pages/StaffDashboard.tsx:278
- The StatCard component is defined at the bottom but isn't exported. While it's used internally in this file, the component signature suggests it could be reusable. Consider either moving it to a shared components file or adding a comment indicating it's intentionally file-scoped.
<span className="text-xs font-semibold text-gray-400 uppercase tracking-wide">{label}</span>
<div className="w-8 h-8 rounded-lg bg-gray-50 flex items-center justify-center">
{icon}
</div>
</div>
<span style={{ fontFamily: 'Geom' }} className="text-2xl font-bold text-gray-900">
{value}
</span>
</div>
)
export default StaffDashboard
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <input | ||
| type="email" | ||
| placeholder="staff@tint.edu.in" | ||
| value={email} | ||
| onChange={(e) => setEmail(e.target.value)} | ||
| className="w-full bg-gray-50 border border-gray-200 rounded-xl px-4 py-3 text-sm focus:outline-none focus:ring-2 focus:ring-emerald-500/20 focus:border-emerald-500 transition-all" | ||
| /> |
There was a problem hiding this comment.
The email input in AddStaffModal doesn't have the same focus styles as ManageTeamModal. Inconsistent UI styling across similar components reduces maintainability. Consider extracting common input styles to a shared component or CSS class.
| // we need to handle cleanup | ||
| if (staffVerified && !staffActivated) { | ||
| console.error('Staff verification succeeded but activation failed. Manual cleanup may be required.'); | ||
| // Note: Ideally, the backend should handle this transactionally | ||
| // or provide a deactivate/rollback endpoint |
There was a problem hiding this comment.
The error handling logs a message about manual cleanup being required when staff verification succeeds but activation fails. However, there's no actual cleanup mechanism implemented. This creates a potential inconsistent state where a user is verified but not activated. Consider either implementing automatic rollback or providing clear documentation on manual cleanup procedures.
| // we need to handle cleanup | |
| if (staffVerified && !staffActivated) { | |
| console.error('Staff verification succeeded but activation failed. Manual cleanup may be required.'); | |
| // Note: Ideally, the backend should handle this transactionally | |
| // or provide a deactivate/rollback endpoint | |
| // we need to handle cleanup to avoid inconsistent staff state | |
| if (staffVerified && !staffActivated) { | |
| console.error('Staff verification succeeded but activation failed. Attempting automatic rollback of staff verification.'); | |
| try { | |
| // Best-effort rollback: inform backend to deactivate/rollback staff verification | |
| await api.post( | |
| '/staff/deactivate', | |
| null, | |
| { headers: { Authorization: `Bearer ${token}` } } | |
| ); | |
| console.info('Automatic rollback of staff verification requested successfully.'); | |
| } catch (rollbackError: any) { | |
| console.error('Failed to rollback staff verification after activation failure. Manual cleanup may be required.', rollbackError); | |
| // Note: Backend should ideally handle this transactionally or provide | |
| // a robust rollback mechanism to guarantee consistency. | |
| } |
| <button | ||
| onClick={onClose} | ||
| className="w-8 h-8 rounded-full bg-gray-200 flex items-center justify-center text-gray-500 hover:bg-gray-300 transition-colors" | ||
| > | ||
| <X size={16} /> | ||
| </button> |
There was a problem hiding this comment.
The modal buttons lack proper ARIA labels and keyboard navigation support. The close button with the X icon should have an aria-label like "Close modal" for screen reader users, and the modal should support closing with the Escape key for better accessibility.
| <button | ||
| onClick={() => saveEdit(staff.uid)} | ||
| disabled={actionLoading} | ||
| className="w-8 h-8 flex items-center justify-center rounded-lg bg-emerald-100 text-emerald-600 hover:bg-emerald-200 transition-colors" |
There was a problem hiding this comment.
The disabled state on action buttons only prevents onClick events but doesn't communicate the disabled state to screen readers. Add aria-disabled or the disabled attribute, and ensure proper visual feedback with reduced opacity or cursor-not-allowed styling.
|
|
||
| const saveEdit = async (uid: string) => { | ||
| // Robust email validation using standard regex pattern | ||
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; |
There was a problem hiding this comment.
The email validation regex pattern /^[^\s@]+@[^\s@]+\.[^\s@]+$/ is overly simplistic and will accept invalid email addresses. For example, it will accept "test@com..com" or "test@@test.com". Consider using a more robust email validation library or a stricter regex pattern.
| <span className="text-[10px] font-black text-orange-600 uppercase">{deal.timeLeftMinutes}m</span> | ||
| </div> | ||
| {/* Manager Actions */} | ||
| {/* Manager Actions */} |
There was a problem hiding this comment.
Duplicate comment "Manager Actions" appears consecutively. Remove one of the duplicate comment lines.
| {/* Manager Actions */} |
| <p className="text-xs font-bold text-gray-400 uppercase tracking-wider mb-1"> | ||
| Control Panel | ||
| </p> | ||
| <h1 style={{ fontFamily: 'Geom' }} className="text-3xl font-bold text-gray-900"> | ||
| {myCafe.name} | ||
| </h1> | ||
| <div className="flex items-center gap-2 mt-2"> | ||
| <span className="text-muted-foreground font-bold uppercase tracking-widest text-[10px]">Terminal:</span> | ||
| <span className="text-emerald-600 font-black uppercase tracking-widest text-[10px] bg-emerald-50 px-3 py-1 rounded-lg border border-emerald-100">{myCafe.name}</span> | ||
| <span className={`w-2 h-2 rounded-full ${myCafe.isOpen ? 'bg-emerald-500 animate-pulse' : 'bg-red-500'}`} /> | ||
| <span className="text-sm font-medium text-gray-500"> | ||
| {myCafe.isOpen ? "Store is Live" : "Store is Closed"} | ||
| </span> | ||
| </div> | ||
| </div> | ||
| <button onClick={() => toggleCafeteriaStatus(myCafe.id)} className={`w-14 h-14 rounded-2xl flex items-center justify-center shadow-lg transition-all active:scale-90 ${myCafe.isOpen ? "bg-emerald-600 text-white shadow-emerald-200" : "bg-muted text-muted-foreground"}`}> | ||
| <Power size={24} /> | ||
|
|
||
| <button | ||
| onClick={() => toggleCafeteriaStatus(myCafe.id)} | ||
| className={`w-12 h-12 rounded-xl flex items-center justify-center transition-all ${ | ||
| myCafe.isOpen | ||
| ? "bg-emerald-50 text-emerald-600 hover:bg-red-50 hover:text-red-600" | ||
| : "bg-gray-100 text-gray-400 hover:bg-emerald-600 hover:text-white" | ||
| }`} | ||
| > | ||
| <Power size={22} strokeWidth={2.5} /> | ||
| </button> |
There was a problem hiding this comment.
The numbered section comments like "1. Header Section", "2. Main Scrollable Content", "3. Floating Action Button", "4. Minimalist Bottom 'Box' Navbar" are helpful but could be more descriptive about what each section does rather than just labeling them.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: AkibDa <179389698+AkibDa@users.noreply.github.com>
…-9577b083-adf0-423f-8065-3fb32fe82a85 [WIP] WIP Address feedback on Jeet main pull request
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type="button" | ||
| onClick={() => window.alert("Delete menu item functionality will be available soon.")} | ||
| className="w-9 h-9 flex items-center justify-center rounded-lg bg-gray-50 text-gray-600 hover:bg-red-50 hover:text-red-500 transition-colors" | ||
| > |
There was a problem hiding this comment.
These button elements appear outside the menu items loop and are structurally orphaned in the JSX. Lines 185-196 contain two button elements that reference editing and deleting menu items, but they are positioned after the closing tag of the menu management section (line 184) and before the AnimatePresence component (line 197). These buttons will either not render properly or cause JSX parsing errors. They should likely be moved inside the mapped deal items around line 182 to be associated with each menu item.
| > | |
| > | |
| <Trash2 size={16} /> | |
| </button> |
| <button | ||
| type="button" | ||
| onClick={() => window.alert("Edit menu item functionality will be available soon.")} | ||
| className="w-9 h-9 flex items-center justify-center rounded-lg bg-gray-50 text-gray-600 hover:bg-blue-50 hover:text-blue-600 transition-colors" | ||
| > | ||
| <Edit2 size={16} /> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => window.alert("Delete menu item functionality will be available soon.")} | ||
| className="w-9 h-9 flex items-center justify-center rounded-lg bg-gray-50 text-gray-600 hover:bg-red-50 hover:text-red-500 transition-colors" | ||
| > | ||
| <AnimatePresence> |
There was a problem hiding this comment.
Missing closing brace for the opening div element. Looking at the structure, the closing tag at line 184 appears to close the space-y-3 div that starts at line 157. However, the outer Menu Management div that starts at line 147 is missing its closing tag before the modals section begins. This creates an invalid JSX structure.
| <button | |
| type="button" | |
| onClick={() => window.alert("Edit menu item functionality will be available soon.")} | |
| className="w-9 h-9 flex items-center justify-center rounded-lg bg-gray-50 text-gray-600 hover:bg-blue-50 hover:text-blue-600 transition-colors" | |
| > | |
| <Edit2 size={16} /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => window.alert("Delete menu item functionality will be available soon.")} | |
| className="w-9 h-9 flex items-center justify-center rounded-lg bg-gray-50 text-gray-600 hover:bg-red-50 hover:text-red-500 transition-colors" | |
| > | |
| <AnimatePresence> | |
| <button | |
| type="button" | |
| onClick={() => window.alert("Edit menu item functionality will be available soon.")} | |
| className="w-9 h-9 flex items-center justify-center rounded-lg bg-gray-50 text-gray-600 hover:bg-blue-50 hover:text-blue-600 transition-colors" | |
| > | |
| <Edit2 size={16} /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => window.alert("Delete menu item functionality will be available soon.")} | |
| className="w-9 h-9 flex items-center justify-center rounded-lg bg-gray-50 text-gray-600 hover:bg-red-50 hover:text-red-500 transition-colors" | |
| > | |
| <Trash2 size={16} /> | |
| </button> | |
| </div> | |
| <AnimatePresence> |
| const handleAddStaff = async () => { | ||
| if (!email) { | ||
| setError("Please enter an email"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing email validation before submitting the add staff request. While the input has type="email", this only provides basic browser validation. A proper email format check should be performed in the handleAddStaff function to ensure data quality and provide clear user feedback, similar to the validation in ManageTeamModal.tsx at line 96.
| import { auth } from '@/firebaseConfig'; | ||
|
|
||
| type StaffProfile = { | ||
| role:"manager" | "staff"; |
There was a problem hiding this comment.
Inconsistent spacing in type definition. There should be a space after the colon for consistency with the rest of the codebase.
| role:"manager" | "staff"; | |
| role: "manager" | "staff"; |
| setUserRole: (role: UserRole | null) => void; | ||
| setOnboarded: (val: boolean) => void; | ||
| setVerified: (val: boolean) => void; | ||
| staffProfile: StaffProfile | null; //this is the new staff rbac |
There was a problem hiding this comment.
The comment 'this is the new staff rbac' uses lowercase and lacks proper punctuation. It should be capitalized and end with a period for consistency with professional code documentation standards.
| staffProfile: StaffProfile | null; //this is the new staff rbac | |
| staffProfile: StaffProfile | null; // This is the new staff RBAC. |
| } catch (staffError: any) { | ||
| // Rollback: If activation or profile fetch failed after verification, | ||
| // we need to handle cleanup | ||
| if (staffVerified && !staffActivated) { | ||
| console.error('Staff verification succeeded but activation failed. Manual cleanup may be required.'); | ||
| // Note: Ideally, the backend should handle this transactionally | ||
| // or provide a deactivate/rollback endpoint | ||
| } | ||
|
|
||
| // Re-throw the error to be handled by the outer catch block | ||
| throw staffError; |
There was a problem hiding this comment.
The error handling in this catch block attempts to differentiate between verification failure and activation failure, but the logic is flawed. The variable 'staffActivated' will always be false at this point because it's only set to true inside the try block on line 82. If an error occurs during activation, the code never reaches that assignment, so the condition on line 105 will always be true when there's an error, making the check redundant. Additionally, the comment mentions cleanup but no actual cleanup occurs - Firebase auth state remains logged in even if staff verification/activation fails.
|
|
||
| useEffect(() => { | ||
| // Increased duration to allow for slower, more graceful animations | ||
| // 3 seconds total duration for splash |
There was a problem hiding this comment.
The comment states "3 seconds total duration for splash" but the code only waits 3000ms before transitioning to the ecosystem stage, not before calling onFinish. The actual splash screen duration appears to be 3 seconds plus the time the ecosystem stage is displayed before onFinish is called (which is not shown in this code). The comment should clarify this is the duration for the branding stage only, or the code should be updated to match the comment.
| // 3 seconds total duration for splash | |
| // 3 seconds duration for branding stage before showing ecosystem |
| <span className="text-[10px] font-black text-orange-600 uppercase">{deal.timeLeftMinutes}m</span> | ||
| </div> | ||
| {/* Manager Actions */} | ||
| {/* Manager Actions */} |
There was a problem hiding this comment.
Duplicate comment on consecutive lines. Line 111 appears to be a duplicate of line 110, both saying 'Manager Actions'. One of these comments should be removed.
| {/* Manager Actions */} |
…/sub-pr-36-yet-again Revert "[WIP] Address feedback from review on pull request #36"
No description provided.