-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor logging setup in lifespan, enhance AdminLogs with new featur… #1
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
Conversation
…es and filters - Moved logging setup after cache service connection in lifespan. - Updated AdminLogs component to include new log entry fields and improved filtering. - Added functionality for saving filter states in localStorage. - Implemented keyboard shortcut for search input. - Enhanced table with dynamic column visibility and improved row expansion. - Introduced toast notifications for clipboard actions. - Added animations for new log entries and improved loading states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the logging infrastructure and significantly enhances the AdminLogs UI with advanced filtering and user experience improvements. The backend now stores logs in Redis for real-time viewing, while the frontend provides a sophisticated log viewing interface with persistent filter states, dynamic column visibility, and smooth animations for new log entries.
Key changes:
- Moved logging setup after cache service initialization to enable Redis-based log storage
- Enhanced AdminLogs component with localStorage-backed filters, column toggles, keyboard shortcuts, and copy-to-clipboard functionality
- Improved UX with scroll position preservation during refresh, collapsible filter panel, and toast notifications
- Changed many demo adapter logs from INFO to DEBUG level to reduce noise
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/src/main.py | Moved logging setup after cache service connection to enable Redis log handler |
| apps/api/src/logging_config.py | Added RedisLogHandler with async storage, centered level names in formatter, and improved filtering |
| apps/web/src/pages/AdminLogs.tsx | Major enhancement with filter persistence, dynamic columns, keyboard shortcuts, scroll preservation, and detailed log expansion with copy functionality |
| apps/web/src/index.css | Added pulse-green animation for new log entries with dark mode variant |
| apps/web/src/components/Layout.tsx | Modified layout to prevent scrolling on AdminLogs page for better table viewing |
| apps/api/src/adapters/enedis.py | Changed debug/info log levels to reduce verbosity, removed Saturday-specific debug logging |
| apps/api/src/adapters/demo_adapter.py | Changed INFO logs to DEBUG to reduce noise in production-like environments |
Comments suppressed due to low confidence (1)
apps/web/src/pages/AdminLogs.tsx:161
- Superfluous argument passed to function getAdminLogs.
const response = await getAdminLogs(undefined, linesCount, 0) as GetLogsResponse;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/web/src/pages/AdminLogs.tsx
Outdated
| </tr> | ||
| {isExpanded && ( | ||
| <tr className={`border-b border-gray-800 ${getRowBgColor(log.level)}`}> | ||
| <td colSpan={4} className="px-2 py-3"> |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The colSpan={4} is hardcoded but the actual number of visible columns is dynamic based on visibleColumns. If fewer than 4 columns are visible, this will cause the expanded row content to not span the full table width correctly.
Replace with colSpan={visibleColumns.size} to dynamically match the number of visible columns.
| <td colSpan={4} className="px-2 py-3"> | |
| <td colSpan={visibleColumns.size} className="px-2 py-3"> |
| const toggleColumn = (column: string) => { | ||
| const newColumns = new Set(visibleColumns); | ||
| if (newColumns.has(column)) { | ||
| // Ne pas permettre de masquer toutes les colonnes |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The comment "Ne pas permettre de masquer toutes les colonnes" (Don't allow hiding all columns) is in French while most code comments in the file appear to be in English or minimal. This creates inconsistency in documentation language.
Consider using English for code comments to maintain consistency: "Don't allow hiding all columns".
| // Ne pas permettre de masquer toutes les colonnes | |
| // Don't allow hiding all columns |
| // Load level filters from localStorage or use default | ||
| const [levelFilters, setLevelFilters] = useState<Set<string>>(() => { | ||
| const saved = localStorage.getItem('adminLogs_levelFilters'); | ||
| return saved ? new Set(JSON.parse(saved)) : new Set(['INFO', 'WARNING', 'ERROR', 'DEBUG']); |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The default level filters now include 'DEBUG' logs (line 37), which could significantly increase the volume of logs displayed and may impact performance. Previously, only 'INFO' was selected by default.
Consider whether including DEBUG logs by default is intentional, as this may overwhelm users with verbose logging information. A more conservative default might be ['INFO', 'WARNING', 'ERROR'].
| return saved ? new Set(JSON.parse(saved)) : new Set(['INFO', 'WARNING', 'ERROR', 'DEBUG']); | |
| return saved ? new Set(JSON.parse(saved)) : new Set(['INFO', 'WARNING', 'ERROR']); |
apps/web/src/pages/AdminLogs.tsx
Outdated
| setTimeout(() => { | ||
| if (container) { | ||
| if (isAtTop) { | ||
| // If user was at the top, stay at top to see new logs | ||
| container.scrollTop = 0; | ||
| } else if (anchorTimestamp) { | ||
| // Find the anchor element and restore position relative to it | ||
| const anchorRow = container.querySelector(`tbody tr[data-timestamp="${anchorTimestamp}"]`); | ||
| if (anchorRow) { | ||
| const rect = anchorRow.getBoundingClientRect(); | ||
| const containerRect = container.getBoundingClientRect(); | ||
| const currentOffsetTop = rect.top - containerRect.top; | ||
| const offsetDiff = currentOffsetTop - anchorOffsetTop; | ||
|
|
||
| // Adjust scroll to maintain the same visual position | ||
| container.scrollTop = container.scrollTop + offsetDiff; | ||
| } | ||
| } else { | ||
| // Fallback: restore exact scroll position | ||
| container.scrollTop = scrollTop; | ||
| } | ||
| } | ||
| }, 0); |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scroll restoration logic in the setTimeout callback (lines 228-250) may execute after the component unmounts if the user navigates away during the refresh. This could cause errors when trying to access DOM elements that no longer exist.
Add a cleanup flag or use useRef to track if the component is still mounted, and check this flag before attempting to restore scroll position.
apps/api/src/logging_config.py
Outdated
| # Filter out logs from /admin/logs and /ping endpoints | ||
| message = record.getMessage() | ||
| if '/admin/logs' in message or '/ping ' in message or '/ping"' in message: | ||
| return |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter condition uses in checks for /admin/logs and /ping, which can produce false positives. For example, a log message containing the text "I'm going to /admin/logs page" would be filtered out even if it's not an HTTP request log.
Consider using more specific patterns like checking if the message starts with certain HTTP method prefixes (e.g., 'GET /admin/logs', 'POST /admin/logs') or using regex patterns to match HTTP request logs more precisely.
apps/web/src/pages/AdminLogs.tsx
Outdated
| const copyToClipboard = (text: string, fieldName: string) => { | ||
| navigator.clipboard.writeText(text); | ||
| setToastMessage(`${fieldName} copié`); |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for the clipboard API. If navigator.clipboard.writeText() fails (e.g., due to permissions or browser compatibility), the promise rejection is not handled, which could result in unhandled promise rejections.
Add .catch() handler or wrap in try-catch with async/await to handle potential clipboard API failures gracefully.
| const copyToClipboard = (text: string, fieldName: string) => { | |
| navigator.clipboard.writeText(text); | |
| setToastMessage(`${fieldName} copié`); | |
| const copyToClipboard = async (text: string, fieldName: string) => { | |
| try { | |
| await navigator.clipboard.writeText(text); | |
| setToastMessage(`${fieldName} copié`); | |
| } catch (err) { | |
| setToastMessage(`Échec de la copie de ${fieldName}`); | |
| } |
| copyToClipboard(JSON.stringify(log, null, 2), 'JSON'); | ||
| }} | ||
| className="text-blue-400 hover:text-blue-300 text-xs px-2 py-1 rounded bg-gray-800 hover:bg-gray-700" | ||
| title="Copier le JSON" |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The copy button label "Copier le JSON" is in French while other copy operations show "Copier" consistently. This creates an inconsistency in the UI language.
Change to just "Copier" to match the other copy buttons, or ensure all buttons use the full phrase for consistency.
| title="Copier le JSON" | |
| title="Copier" |
| }; | ||
|
|
||
| const toggleModuleFilter = (module: string) => { | ||
| // Toggle module in multi-select mode |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "Toggle module in multi-select mode" is misleading since clicking a module in the table row (lines 771-777) actually implements single-select behavior (selecting only one module at a time), not multi-select.
Update the comment to reflect the actual behavior: "Toggle module filter (allows multiple selections)" or remove it since it's now unclear which behavior this refers to.
| // Toggle module in multi-select mode | |
| // Toggle module filter (allows multiple selections) |
apps/web/src/pages/AdminLogs.tsx
Outdated
| // On initial load, select src and httpx modules by default | ||
| if (isInitialLoad) { | ||
| const defaultModules = response.all_modules.filter(m => { | ||
| const response = await getAdminLogs(undefined, linesCount, 0) as GetLogsResponse; |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getAdminLogs function is called with 3 arguments (line 161: getAdminLogs(undefined, linesCount, 0)), but the function signature only accepts 2 parameters: level?: string, lines?: number. The third argument 0 will be ignored, which may indicate incomplete refactoring.
Either remove the third argument from the call, or update the function signature in admin.ts if a new parameter was intended to be added.
| const response = await getAdminLogs(undefined, linesCount, 0) as GetLogsResponse; | |
| const response = await getAdminLogs(undefined, linesCount) as GetLogsResponse; |
| <main className={`flex-1 bg-gray-50 dark:bg-gray-900 ${location.pathname === '/admin/logs' ? 'overflow-hidden' : 'overflow-y-auto'}`}> | ||
| <div className={`container mx-auto px-3 sm:px-4 lg:px-6 max-w-[1920px] ${location.pathname === '/admin/logs' ? 'h-full pb-0' : 'pb-6'}`}> | ||
| {children} | ||
|
|
||
| {/* Spacer to push footer down - only on desktop */} | ||
| <div className="hidden md:block pb-20"></div> | ||
| {location.pathname !== '/admin/logs' && <div className="hidden md:block pb-20"></div>} |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The condition location.pathname === '/admin/logs' is repeated three times with different purposes (overflow control, height control, and spacer). This creates maintenance burden if the path changes or if other pages need similar layout behavior.
Consider extracting this into a constant or computed value like const isAdminLogsPage = location.pathname === '/admin/logs' to improve readability and maintainability.
…gs dans Redis Interface AdminLogs: - Ajout de l'icône FileText et sous-titre dans le header selon les guidelines design - Remplacement des styles inline par des classes Tailwind - Ajout complet du support dark mode sur tous les éléments (tableau, filtres, boutons) - Migration vers le système de couleurs primary au lieu de blue - Header "Filtres" cliquable pour déplier/replier le panneau - Support du raccourci Ctrl+K pour auto-expansion du panneau de filtres - Blocs de détails cliquables pour copie directe sans passer par le bouton - Correction du bug d'animation verte sur les logs existants lors du refresh - Ajout d'un espacement entre le header et le contenu des filtres - Gestion de l'overflow conditionnelle dans Layout pour scroll unique Optimisations backend: - Thread pool du RedisLogHandler étendu de 2 à 8 workers pour gérer les pics de logs - Ajout de timeouts rapides (1s) sur les connexions Redis pour éviter les blocages - Utilisation de aclose() au lieu de close() pour une meilleure gestion async - Réduction significative de la perte de logs lors des bursts d'activité 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…t interactif - Ajouter un préfixe [PDL] à tous les logs liés à un PDL dans enedis.py et pdl.py - Créer des fonctions helper log_with_pdl() et log_if_debug() avec support PDL - Optimiser Redis logging avec thread pool de 8 workers et timeouts courts - Corriger la préservation de la position de scroll lors du refresh automatique - Implémenter un header réduit avec contrôles inline (recherche, filtres de niveau) - Ajouter un badge cliquable pour indiquer les filtres modules actifs - Badge modules avec animation pulse et hover effect pour déplier le panneau - Améliorer la gestion des clics sur le header pour toggle collapse/expand - Layout optimisé: recherche (gauche) → filtres niveau (centre) → badge modules (droite) - Corriger les coins arrondis au survol du header - Supprimer les variables TypeScript inutilisées 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Détailler le nouveau panneau de filtres avec header réduit interactif - Documenter l'identification PDL dans les logs ([XXXXXXXXXXXXXX]) - Ajouter la section Architecture backend des logs (Redis, thread pool) - Expliquer les fonctions helper log_with_pdl() et log_if_debug() - Documenter la préservation de scroll et le badge modules cliquable - Mettre à jour les technologies utilisées et fichiers liés - Clarifier la rétention des logs (24h dans Redis) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Issues critiques corrigées: - colSpan dynamique basé sur visibleColumns.size au lieu de hardcodé à 4 - Suppression du 3e argument superflu dans getAdminLogs() - Pruning de seenTimestamps (max 2x linesCount) pour éviter memory leak - Vérification isMountedRef avant scroll restoration pour éviter erreurs après unmount Issues importantes corrigées: - Gestion d'erreur async/await pour clipboard API avec message d'échec - Amélioration du filtre Redis logs avec patterns HTTP spécifiques (GET, POST, etc.) au lieu de simple 'in' check pour éviter faux positifs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…es and filters