-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor services viewer into card grid #216
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
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.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
audits/scripts/viewer.js
Outdated
| return; | ||
| } | ||
| const data = await loadAudit(latestEntry.file); | ||
| renderServices(data.services || []); |
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.
[P0] Init no longer renders system metrics
The new init routine fetches the latest audit and only calls renderServices and renderDocker. Previously this function also rendered load average, CPU, memory, disks, top processes and other sections. Because the markup for those sections still exists in index.html, they will never be populated after this change, leaving most of the dashboard blank. Reintroducing the removed rendering calls (or removing the unused UI) is necessary to keep the viewer functional.
Useful? React with 👍 / 👎.
| export function renderServices(list = []) { | ||
| const grid = document.getElementById('servicesGrid'); | ||
| const empty = document.getElementById('servicesEmpty'); | ||
| const countSpan = document.getElementById('servicesCount'); |
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.
[P1] Services strings render as unnamed cards
The refactored renderServices assumes every entry already exposes fields like id, unit_name, etc. and otherwise displays '—'. The repository’s existing audit JSON (e.g. /workspace/audit-server/audits/archives/audit_2025-08-26_23-00.json) still provides services as an array of plain strings, so the new implementation will render a grid of blank cards with default icons instead of the service names. Either coerce string values into objects before rendering or maintain support for the old format to avoid breaking existing reports.
Useful? React with 👍 / 👎.
audits/scripts/modules/services.js
Outdated
| <span class="badge">${type}</span> | ||
| <span class="badge ${catClass}">${category}</span> | ||
| </div> | ||
| <p class="card-desc" title="${desc}">${desc}</p> |
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.
[P1] Unescaped service fields allow HTML injection
Service attributes are interpolated directly into an HTML string (id, unit, type, category, description) and later inserted with grid.innerHTML. If any of these values contains markup or quotes, it will be interpreted as HTML, enabling XSS or breaking the card layout. The previous implementation used DOM nodes and textContent, which escaped these values automatically. Sanitising the strings or using DOM APIs would prevent injection.
Useful? React with 👍 / 👎.
|
@codex review |
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.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
audits/scripts/modules/services.js
Outdated
| const normalized = (list || []).map((s) => | ||
| typeof s === 'string' ? { id: s } : s || {} | ||
| ); | ||
| const sorted = [...normalized].sort((a, b) => |
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.
[P2] Fall back to unit_name when sorting services
The new card renderer sorts services with String(a.id || '').localeCompare(...) but the report format you documented allows objects to omit id and only provide unit_name. In that case every such object is compared as an empty string, so their relative order depends on the input array rather than the displayed name, breaking the deterministic A→Z ordering promised in the summary. Consider using a.id ?? a.unit_name (and likewise for b) for the sort key so entries that only specify unit_name are still ordered alphabetically.
Useful? React with 👍 / 👎.
|
@codex review |
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.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| async function init() { | ||
| try { | ||
| showStatus("Chargement\u2026", "loading"); | ||
| const list = await fetchIndex(); | ||
| parseIndex(list); | ||
| if (!latestEntry) { | ||
| showStatus("Aucun rapport", "empty"); | ||
| return; | ||
| } | ||
| const data = await loadAudit(latestEntry.file); | ||
| renderInfo(data); | ||
| renderServices(data.services || []); | ||
| renderDocker(data.docker || []); | ||
| showStatus(""); | ||
| } catch (err) { | ||
| console.error(err); | ||
| showStatus("Impossible de charger les rapports: " + err.message, "error"); | ||
| } | ||
| } catch (err) { | ||
| console.error('refresh error', err); | ||
| } finally { | ||
| dot.classList.remove('active'); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| let closeMenu; | ||
|
|
||
| document.addEventListener('DOMContentLoaded', () => { | ||
| const sidebar = document.getElementById('sidebar'); | ||
| const overlay = document.getElementById('menuOverlay'); | ||
| const toggle = document.getElementById('menuToggle'); | ||
| const icon = toggle.querySelector('i'); | ||
|
|
||
| closeMenu = () => { | ||
| sidebar.classList.remove('open'); | ||
| toggle.classList.remove('open'); | ||
| icon.classList.remove('fa-xmark'); | ||
| icon.classList.add('fa-bars'); | ||
| }; | ||
|
|
||
| toggle.addEventListener('click', () => { | ||
| const isOpen = sidebar.classList.toggle('open'); | ||
| toggle.classList.toggle('open', isOpen); | ||
| icon.classList.toggle('fa-bars', !isOpen); | ||
| icon.classList.toggle('fa-xmark', isOpen); | ||
| }); | ||
|
|
||
| overlay.addEventListener('click', closeMenu); | ||
| }); | ||
|
|
||
| document.addEventListener('DOMContentLoaded', init); No newline at end of file | ||
| // audits/scripts/main.js | ||
| document.addEventListener("DOMContentLoaded", init); |
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.
[P0] Wire up sidebar navigation and menu controls
The new init function now just fetches the latest audit and renders services/docker, but it never attaches listeners or populates the sidebar controls (#btnLatest, day buttons, timeline) nor does it bind the menu toggle/overlay. The HTML still renders those elements (audits/index.html lines 48‑79), so after this change users cannot browse older reports or open/close the sidebar because the controls are inert. Please restore the event handling or remove the dead UI.
Useful? React with 👍 / 👎.
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.
@codex fix comments
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.
Summary
- Added helpers to track the selected day, populate timeline chips, and load reports so users can browse archived audits directly from the sidebar controlsaudits/scripts/modules/audits.jsL65-L104
- Restored sidebar menu behavior by wiring up the toggle button and overlay, and connected date buttons and timeline chips to the new selection logic for proper navigationaudits/scripts/modules/audits.jsL106-L125audits/scripts/modules/audits.jsL133-L199
Testing
- ✅
npm run build - ✅
./tests/run.sh
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.
@codex review
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.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| function renderServiceCard(svc = {}) { | ||
| const id = svc.id || svc.unit_name || "\u2014"; | ||
| const meta = getMeta(id); | ||
| const category = svc.category || meta.category; | ||
| const catClass = "cat-" + category.toLowerCase().replace(/[\s/]+/g, "-"); |
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.
[P2] Normalize category names before creating badge class
Service badges build the CSS class via "cat-" + category.toLowerCase().replace(/[\s/]+/g, "-"), but the category strings contain diacritics (e.g. "Système", "Sécurité"). The stylesheet defines colors using accentless classes like .badge.cat-systeme and .badge.cat-securite, so badges rendered with cat-système never match the rules and remain unstyled. Removing accents (e.g. category.normalize('NFD').replace(/[\u0300-\u036f]/g, '')) before building the class keeps the badge colors working for all French categories.
Useful? React with 👍 / 👎.
| async function init() { | ||
| try { | ||
| showStatus("Chargement\u2026", "loading"); | ||
| const list = await fetchIndex(); | ||
| parseIndex(list); | ||
| if (!latestEntry) { | ||
| showStatus("Aucun rapport", "empty"); | ||
| return; | ||
| } | ||
| initNav(); |
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.
[P1] Automatic audit refresh removed
The new init() only fetches the audit index once (fetchIndex → loadAndRender) and then exits; there is no setInterval or other polling to load subsequent reports. The previous implementation refreshed every minute and drove the “automatic update” indicator (#refreshDot/#updateBadge). With this change the page never discovers new audits unless the user reloads the whole app, despite the UI still presenting auto-refresh controls. Consider reinstating a periodic refresh to keep the view current.
Useful? React with 👍 / 👎.
|
@codex fix comment |
Summary
Testing
npm run build./tests/run.shhttps://chatgpt.com/codex/tasks/task_e_68b04e3d35c8832dad3547962755318a