Feature/theme logo favicons #11
Conversation
|
@Helder-Lucau is attempting to deploy a commit to the Eshita's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds PWA metadata and icons, duplicates a CSS keyframes block, introduces theme-aware header logo switching, moves script loading to the head with defer, refactors invoice preview rendering to DOM operations, adds a PDF download loader/spinner and stronger PDF generation error handling, and restructures invoice preview markup and SVGs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Toggle as Theme Toggle
participant JS as script.js
participant DOM as Document
participant Logo as #theme-logo
User->>Toggle: Click
Toggle->>JS: onClick handler
JS->>DOM: Toggle body class (dark/light)
JS->>DOM: Toggle sun/moon icon visibility
JS->>Logo: updateLogo(logo, isDark)
JS->>JS: Persist theme (localStorage)
sequenceDiagram
autonumber
actor User
participant Btn as Download Button
participant JS as script.js
participant Loader as .pdf-loader/.spinner
participant PDF as html2pdf
User->>Btn: Click "Download PDF"
Btn->>JS: downloadPDF()
JS->>Loader: Show loader + disable button
JS->>PDF: Generate PDF (async, options validated)
PDF-->>JS: Resolve/Reject
JS->>Loader: Hide loader + enable button
JS-->>User: Trigger download / show error message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script.js (1)
297-353: Hook up the fullscreen loader and ensure cleanup on success/errorCurrently only the button shows a spinner; the overlay exists but is never toggled. Also ensure we revert UI in all branches.
function downloadPDF() { updatePreview(); const downloadBtn = document.getElementById("downloadPDF"); + const loader = document.getElementById("pdfLoader"); const originalButtonContent = downloadBtn.innerHTML; downloadBtn.disabled = true; downloadBtn.innerHTML = ` @@ requestAnimationFrame(() => { setTimeout(() => { const invoiceBox = document.getElementById("invoiceBox"); if (!invoiceBox || typeof html2pdf === "undefined") { + if (loader) loader.classList.remove("visible"); alert("PDF download functionality is not available."); downloadBtn.disabled = false; downloadBtn.innerHTML = originalButtonContent; return; } + if (loader) loader.classList.add("visible"); const opt = { @@ html2pdf() .set(opt) .from(invoiceBox) .save() .then(() => { + if (loader) loader.classList.remove("visible"); downloadBtn.disabled = false; downloadBtn.innerHTML = originalButtonContent; }) .catch((error) => { console.error("Error generating PDF:", error); alert("Failed to generate PDF. Please try again."); + if (loader) loader.classList.remove("visible"); downloadBtn.disabled = false; downloadBtn.innerHTML = originalButtonContent; }); }, 300); }); }Also consider reusing the div-based spinner (see home.css comment) for visual consistency.
🧹 Nitpick comments (4)
home.css (1)
324-338: Spinner class conflicts: div vs. SVGThe CSS spinner renders a border-based div. The button uses an SVG with the same class, which won’t show the intended effect. Either reuse the div-based spinner in the button or style the SVG path.
Option A (reuse div spinner in button): change the button’s innerHTML to use
<div class="spinner spinner--btn"></div>and add:.spinner { border: 5px solid #eee; @@ margin-bottom: 15px; } + +/* smaller variant for buttons */ +.spinner.spinner--btn { + width: 16px; + height: 16px; + border-width: 3px; + margin: 0 8px 0 0; +}Option B (animate the SVG you inject into the button): add
/* SVG spinner support */ .spinner .path { stroke: #a78bfa; stroke-linecap: round; stroke-dasharray: 90, 150; stroke-dashoffset: 0; animation: spin 1s linear infinite, dash 1.5s ease-in-out infinite; } @keyframes dash { 0% { stroke-dasharray: 1, 200; stroke-dashoffset: 0; } 50% { stroke-dasharray: 90, 150; stroke-dashoffset: -35; } 100% { stroke-dasharray: 90, 150; stroke-dashoffset: -124; } }site.webmanifest (1)
11-23: Verify 192px maskable asset really is maskableBoth 192px entries reference the same file. If it isn’t truly maskable, provide a dedicated maskable asset to avoid unwanted cropping on Android.
I can generate a safe-mask guide grid or template if you want.
index.html (1)
36-62: Add an accessible name and pressed state to the theme toggleThe button has only icons; screen readers need a label. JS can toggle aria-pressed.
- <button id="theme-toggle" class="theme-toggle-btn"> + <button id="theme-toggle" class="theme-toggle-btn" aria-label="Toggle dark mode" aria-pressed="false">Then in JS, after computing isDark:
themeToggleBtn.setAttribute("aria-pressed", String(isDark));invoice.html (1)
47-73: Mirror the a11y improvement on the theme toggleSame as index.html: add aria-label and toggle aria-pressed in JS.
- <button id="theme-toggle" class="theme-toggle-btn"> + <button id="theme-toggle" class="theme-toggle-btn" aria-label="Toggle dark mode" aria-pressed="false">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
apple-touch-icon.pngis excluded by!**/*.pngfavicon-96x96.pngis excluded by!**/*.pngfavicon.icois excluded by!**/*.icofavicon.svgis excluded by!**/*.svgscreenshots/app-mobile.pngis excluded by!**/*.pngscreenshots/app-wide.pngis excluded by!**/*.pngweb-app-manifest-192x192.pngis excluded by!**/*.pngweb-app-manifest-512x512.pngis excluded by!**/*.png
📒 Files selected for processing (5)
home.css(2 hunks)index.html(3 hunks)invoice.html(9 hunks)script.js(6 hunks)site.webmanifest(1 hunks)
🔇 Additional comments (3)
script.js (3)
26-31: Logo theming toggle looks goodSimple, null-safe, and scoped. Nice.
256-271: Good XSS hardening in preview row renderingSwitching from innerHTML to createElement + textContent removes injection risk from user inputs.
247-251: Quantities are parsed as integersIf fractional quantities (e.g., 1.5 hrs) are expected, use parseFloat and ensure the qty input allows decimals.
- const quantity = parseInt(inputs[2].value) || 0; + const quantity = parseFloat(inputs[2].value) || 0;
home.css
Outdated
| .pdf-loader { | ||
| display: none; /* hidden by default */ | ||
| position: fixed; | ||
| top: 0; | ||
| left: 0; | ||
| width: 100%; | ||
| height: 100%; | ||
| background: rgba(255, 255, 255, 0.8); | ||
| backdrop-filter: blur(3px); | ||
| z-index: 9999; | ||
| align-items: center; | ||
| justify-content: center; | ||
| flex-direction: column; | ||
| font-family: "DM Sans", sans-serif; | ||
| color: #444; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wire up overlay visibility and flex layout
The overlay has flex props but no way to display as flex when shown, and it’s never toggled in JS. Add a visible class to switch it on as flex.
.pdf-loader {
- display: none; /* hidden by default */
+ display: none; /* hidden by default */
position: fixed;
@@
color: #444;
}
+
+/* Show overlay */
+.pdf-loader.visible {
+ display: flex;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .pdf-loader { | |
| display: none; /* hidden by default */ | |
| position: fixed; | |
| top: 0; | |
| left: 0; | |
| width: 100%; | |
| height: 100%; | |
| background: rgba(255, 255, 255, 0.8); | |
| backdrop-filter: blur(3px); | |
| z-index: 9999; | |
| align-items: center; | |
| justify-content: center; | |
| flex-direction: column; | |
| font-family: "DM Sans", sans-serif; | |
| color: #444; | |
| } | |
| .pdf-loader { | |
| display: none; /* hidden by default */ | |
| position: fixed; | |
| top: 0; | |
| left: 0; | |
| width: 100%; | |
| height: 100%; | |
| background: rgba(255, 255, 255, 0.8); | |
| backdrop-filter: blur(3px); | |
| z-index: 9999; | |
| align-items: center; | |
| justify-content: center; | |
| flex-direction: column; | |
| font-family: "DM Sans", sans-serif; | |
| color: #444; | |
| } | |
| /* Show overlay */ | |
| .pdf-loader.visible { | |
| display: flex; | |
| } |
🤖 Prompt for AI Agents
In home.css around lines 307 to 322, the .pdf-loader overlay defines flex
properties but remains display:none and is never toggled; add a modifier
selector (e.g., .pdf-loader.visible) that sets display:flex to enable the flex
layout when shown, and update the JS to add/remove that visible class to toggle
the overlay (ensure the JS sets focus/traps or aria-hidden appropriately when
showing/hiding).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
invoice.html (1)
456-471: Spinner markup added; ensure CSS is loaded on this page.The button injects
<svg class="spinner">…</svg>, but style.css doesn’t include spinner rules. Add the spinner CSS to style.css (see home.css comment) so the animation shows here.
🧹 Nitpick comments (10)
home.css (1)
307-312: Spinner keyframes are defined here but not scoped with any class; ensure they’re applied where the spinner is used.Right now the SVG gets class="spinner" from script.js, but there are no
.spinnerrules here or in style.css. Either move the spin animation and.spinnerrules to style.css (used by invoice.html), or add a small spinner block there so the button animation actually runs.Add this to style.css (preferred):
.spinner { width: 20px; height: 20px; margin-right: 8px; animation: spin 1s linear infinite; vertical-align: -3px; } .spinner .path { stroke: currentColor; stroke-linecap: round; opacity: 0.35; } @media (prefers-reduced-motion: reduce) { .spinner { animation: none; } }script.js (4)
26-31: Avoid hardcoding logo filenames in JS; prefer data attributes.This couples script logic to specific filenames. Read from
data-light/data-darkon theso content can change without code edits.
Apply in HTML (see invoice.html comment) and then:
-function updateLogo(logo, isDark) { - if (!logo) return; - logo.src = isDark ? "gilogo.png" : "gil.png"; -} +function updateLogo(logo, isDark) { + if (!logo) return; + const darkSrc = logo.dataset.dark || "gilogo.png"; + const lightSrc = logo.dataset.light || "gil.png"; + logo.src = isDark ? darkSrc : lightSrc; +}
34-44: Set aria-pressed on load to reflect saved theme.Accessibility: the toggle’s pressed state isn’t updated during applySavedTheme(), so it can be out of sync until the first click.
Apply this diff:
- body.classList.toggle("dark-theme", isDark); - toggleThemeIcons(moonIcon, sunIcon, isDark); - updateLogo(logo, isDark); + body.classList.toggle("dark-theme", isDark); + toggleThemeIcons(moonIcon, sunIcon, isDark); + updateLogo(logo, isDark); + if (themeToggleBtn) themeToggleBtn.setAttribute("aria-pressed", String(isDark));
251-253: Permit fractional quantities if desired.Current code uses parseInt; typical invoices allow fractional qty (e.g., 1.5 hrs). If that’s intended, switch to parseFloat.
- const quantity = parseInt(inputs[2].value) || 0; + const quantity = parseFloat(inputs[2].value) || 0;
60-65: Icon visibility via inline styles can fight CSS; consider a class toggle.Toggling
style.displaycan conflict with responsive styles. A small.hidden { display:none !important; }class toggle is more robust.site.webmanifest (1)
11-22: Add maskable icon variants for better PWA install quality.Include a maskable 512×512 entry so icons crop correctly on Android home screens.
"icons": [ { "src": "/web-app-manifest-192x192.png", "sizes": "192x192", "type": "image/png" }, { "src": "/web-app-manifest-512x512.png", "sizes": "512x512", - "type": "image/png" + "type": "image/png", + "purpose": "any" + }, + { + "src": "/web-app-manifest-512x512-maskable.png", + "sizes": "512x512", + "type": "image/png", + "purpose": "maskable" } ],invoice.html (4)
31-31: Make logo sources configurable via data attributes.Pairs with the script.js refactor so content/branding changes don’t require JS edits.
- <img id="theme-logo" src="gil.png" alt="GENINVOICE Logo" /> + <img + id="theme-logo" + src="gil.png" + data-light="gil.png" + data-dark="gilogo.png" + alt="GENINVOICE Logo" + />
38-43: Tie initial aria-pressed to saved theme.Default is false; ensure applySavedTheme updates it (see script.js suggestion).
After applying the change, verify devtools: the button’s aria-pressed matches the body class on first load.
353-357: Consider theming the invoice preview logo too.Header logo switches with theme, but the preview logo is fixed to gilogo.png. If you want the PDF to reflect theme branding, add an id (e.g.,
invoice-preview-logo) and update it via updateLogo as well.- <img - src="gilogo.png" - alt="GENINVOICE Logo" - class="invoice-logo" - /> + <img + id="invoice-preview-logo" + src="gilogo.png" + data-light="gil.png" + data-dark="gilogo.png" + alt="GENINVOICE Logo" + class="invoice-logo" + />And in script.js, include it in getThemeElements() and call updateLogo on load/toggle.
206-223: Non-negative numeric inputs.Consider adding
min="0"to qty and total fields (qty already lacks it; total is disabled but can keep without min). Helps validation and mobile numeric keyboards.- <input + <input type="number" id="taxRate" value="10" min="0" step="0.01" /> @@ - <td><input type="number" class="qty" placeholder="1" /></td> + <td><input type="number" class="qty" placeholder="1" min="0" /></td> @@ - <td><input type="text" class="total" disabled /></td> + <td><input type="text" class="total" disabled /></td>Also applies to: 257-259, 282-284
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
home.css(2 hunks)index.html(3 hunks)invoice.html(9 hunks)script.js(6 hunks)site.webmanifest(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- index.html
🔇 Additional comments (4)
script.js (1)
261-273: Good DOM-based rendering; eliminates XSS risk from user inputs.invoice.html (2)
14-15: Load order is correct (html2pdf before script.js).This prevents undefined reference issues in downloadPDF(). Good.
15-26: Asset path consistency: verified — assets exist at repo root. All referenced files (favicon.svg, favicon.ico, favicon-96x96.png, apple-touch-icon.png, site.webmanifest) are present at the repository root per the provided script.site.webmanifest (1)
1-9: Verify manifest id/start_url and referenced icons
- site.webmanifest (lines 1–9): using "id": "/index.html" is valid but uncommon — consider using "id": "/" to make app identity independent of the start document and align with "scope": "/". Confirm "start_url": "/index.html" is intentional.
- I ran fd -tf 'web-app-manifest-.png' -x ls -l {} and found no matches. Confirm the icon files referenced by the manifest (or generated web-app-manifest-.png assets) exist in the repo or build output, or update the manifest to reference existing assets.
| }); | ||
| } | ||
|
|
||
| //Handle download PDF button |
There was a problem hiding this comment.
🧩 Analysis chain
Spinner UX is fine; ensure CSS exists and restore button safely.
Logic disables button, shows feedback, and restores state on success/failure—good. Just confirm spinner CSS is loaded on this page (see home.css note).
Run to check for spinner styles:
Also applies to: 313-356
🏁 Script executed:
#!/bin/bash
rg -n "(\.spinner|@keyframes\s+spin)" -g '!node_modules/**' -S --type=css -C2Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Listing stylesheet files (css/scss/sass/less/styl)"
find . -type f \( -iname "*.css" -o -iname "*.scss" -o -iname "*.sass" -o -iname "*.less" -o -iname "*.styl" \) -not -path "./node_modules/*" -print || true
echo
echo "2) Searching repo for spinner-related patterns (CSS classes, keyframes, common spinner libs)"
rg -n --hidden --no-ignore -S '(\.spinner\b|@keyframes\s+spin\b|@keyframes\s+spinner\b|animation:\s*spin\b|fa-spinner|lds-spinner|\.lds-|\bspinner\b)' -g '!node_modules/**' -C3 || true
echo
echo "3) Searching for explicit reference to home.css"
rg -n --hidden --no-ignore -S 'home\.css' -g '!node_modules/**' -C3 || trueLength of output: 517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) CSS files present:"
ls -1 ./style.css ./home.css || true
echo
echo "2) Search style.css and home.css for spinner/loader/keyframes/rotate patterns"
grep -nE '(\.spinner\b|@keyframes[[:space:]]+spin\b|@keyframes[[:space:]]+spinner\b|animation:[[:space:]]*(spin|rotate)\b|fa-spinner|fa-spin|lds-spinner|\.lds-|loader\b|loading\b|\bspinner\b|transform:[[:space:]]*rotate)' style.css home.css || true
echo
echo "3) Show home.css (first 400 lines) for manual inspection"
sed -n '1,400p' home.css || true
echo
echo "4) Show style.css (first 400 lines) for manual inspection"
sed -n '1,400p' style.css || true
echo
echo "5) Search repository for references to home.css (HTML/JS linking)"
grep -RIn --exclude-dir=node_modules --exclude-dir=.git --exclude-dir=.cache 'home.css' || true
echo
echo "6) Inspect script.js around the download-PDF handler (lines 280-380) and search for spinner usage in JS"
sed -n '280,380p' script.js || true
grep -nE 'spinner|loading|loader|fa-spin|fa-spinner|classList\.add|classList\.remove' script.js || trueLength of output: 16757
Spinner CSS missing — add .spinner/.path rules; button restore is OK.
home.css contains @Keyframes spin (line ~307) but no .spinner or .spinner .path selectors; script.js's downloadPDF injects (downloadPDF, ~lines 300–380; spinner SVG at line 308). Add minimal CSS in home.css (or the loaded stylesheet), e.g. .spinner {width:24px;height:24px;animation:spin 1s linear infinite;} .spinner .path {stroke:currentColor;stroke-width:4;stroke-linecap:round;} so the SVG animates. The button-disable/restore logic is correct (original innerHTML saved and restored on both success and error).
🤖 Prompt for AI Agents
In script.js around line 300 the downloadPDF flow injects an <svg
class="spinner"> but the stylesheet (home.css, keyframes spin around line ~307)
lacks .spinner and .spinner .path selectors; add minimal CSS to the loaded
stylesheet (home.css or equivalent) such as: .spinner { width:24px; height:24px;
animation:spin 1s linear infinite; } and .spinner .path { stroke: currentColor;
stroke-width:4; stroke-linecap:round; } so the SVG visibly animates; keep the
existing button disable/restore logic unchanged.
1 participant
Summary by CodeRabbit
New Features
Refactor
Style
Chores