Feature/dashboard - #74
Conversation
… and related functions
…eturn pointer to slice of AuditLog
MonkyMars
added
enhancement
There was a problem hiding this comment.
Pull Request Overview
This PR improves the dashboard functionality with enhanced subject management, file/folder organization, and comprehensive keyboard navigation features.
- Enhanced dashboard with real-time search, keyboard shortcuts, and improved subject cards
- Implemented folder-based file organization with navigation breadcrumbs
- Added comprehensive keyboard shortcuts for both dashboard and subject detail views
Reviewed Changes
Copilot reviewed 38 out of 45 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/ui/app/types/subject.ts | Updated Subject interface to remove description field and add folder support |
| apps/ui/app/hooks/use-subjects.ts | Enhanced hooks to support folder navigation and updated API endpoints |
| apps/ui/app/components/subjects/subject-detail.tsx | Major overhaul with folder navigation, keyboard shortcuts, and improved UI |
| apps/ui/app/components/dashboard/dashboard.tsx | Added search functionality with keyboard shortcuts and improved filtering |
| apps/server/services/subject_service.go | New service for subject management with database queries |
| apps/server/api/routes/subjects.go | New API routes for subject operations |
Files not reviewed (4)
- apps/server/.idea/.gitignore: Language not supported
- apps/server/.idea/modules.xml: Language not supported
- apps/server/.idea/server.iml: Language not supported
- apps/server/.idea/vcs.xml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!response.success || !response.data) { | ||
| throw new Error(response.message || 'Fout bij ophalen bestanden'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Duplicate error checking logic. The same validation is performed on lines 120-122, making this check redundant.
| if (!response.success || !response.data) { | |
| throw new Error(response.message || 'Fout bij ophalen bestanden'); | |
| } |
| <iframe | ||
| src={file.url} |
There was a problem hiding this comment.
Changed from img to iframe for displaying images. This will not work correctly for image files as iframes are meant for embedding documents, not displaying images directly.
| <iframe | |
| src={file.url} | |
| <img | |
| src={file.url} | |
| alt={file.name} |
| const file = await fetch(`https://drive.usercontent.google.com/download?id=${item.fileId}`); | ||
| const blob = await file.blob(); |
There was a problem hiding this comment.
Direct URL construction for Google Drive downloads without proper authentication or validation. This could expose file IDs and may not work reliably without proper authorization headers.
| const file = await fetch(`https://drive.usercontent.google.com/download?id=${item.fileId}`); | |
| const blob = await file.blob(); | |
| // Fetch the file from a secure backend endpoint that handles Google Drive authentication | |
| const response = await fetch(`/api/download-file?fileId=${encodeURIComponent(item.fileId)}`); | |
| if (!response.ok) { | |
| alert('Kan bestand niet ophalen. Controleer uw rechten of probeer het later opnieuw.'); | |
| return; | |
| } | |
| const blob = await response.blob(); |
| // DEFAULT to DISTINCT to avoid duplicates | ||
| pgQuery = pgQuery.Distinct() |
There was a problem hiding this comment.
Applying DISTINCT to all SELECT queries by default can significantly impact performance, especially on large datasets. This should only be applied when necessary to avoid duplicates.
| // DEFAULT to DISTINCT to avoid duplicates | |
| pgQuery = pgQuery.Distinct() | |
| // Apply DISTINCT only if requested or if JOINs are present (handled below) | |
| // pgQuery = pgQuery.Distinct() |
| {searchValue && filteredSubjects.length > 0 && ( | ||
| <div | ||
| className="flex items-center px-1.5 py-0.5 bg-neutral-100 rounded text-xs text-neutral-500" | ||
| title="Druk Enter om naar eerste resultaat te gaan" |
There was a problem hiding this comment.
Corrected Dutch text: 'Druk' should be 'Druk op' for proper grammar.
| printWindow.focus(); | ||
| printWindow.print(); | ||
| } else { | ||
| alert('Pop-up geblokkeerd. Sta pop-ups toe om te kunnen printen.'); |
There was a problem hiding this comment.
Using alert() for user notifications is outdated and provides poor user experience. Consider using a toast notification or modal dialog instead.
IMPORTANT: Use this template for all PRs to ensure consistency.
You can remove sections that do not apply to your change.
Description & Motivation
Improves the lacking dashboard, not done completely.
Related Issues
Relates to #38
(Only include “Closes” for issues you intend this PR to fully resolve.)
Scope of Change
(Mark or list the relevant ones)