Pertempto commented

(Migrated from github.com)

No description provided.

github-actions[bot] commented

2025-11-21 12:38:06 -05:00

(Migrated from github.com)

In frontend/src/components/InvoicesPage.tsx — employees fetch currently does fetch('/api/employees').then(res => res.json()) without checking res.ok. If the API returns a non-2xx response you'll still attempt to setEmployees(data.employees) and might crash or silently show an empty list. Please check the response status and handle error payloads (and network errors). Also consider using an AbortController to cancel the request on unmount to avoid setting state on an unmounted component.

In `frontend/src/components/InvoicesPage.tsx` — employees fetch currently does `fetch('/api/employees').then(res => res.json())` without checking `res.ok`. If the API returns a non-2xx response you'll still attempt to `setEmployees(data.employees)` and might crash or silently show an empty list. Please check the response status and handle error payloads (and network errors). Also consider using an AbortController to cancel the request on unmount to avoid setting state on an unmounted component.

github-actions[bot] commented

2025-11-21 12:39:41 -05:00

(Migrated from github.com)

Changes Requested

Please address the following before merging:

Fix Tailwind/PostCSS setup in the frontend:
- Replace frontend/src/index.css content with Tailwind directives (@tailwind base; @tailwind components; @tailwind utilities;).
- Add a CommonJS PostCSS config file (frontend/postcss.config.cjs or a CommonJS postcss.config.js) that exports tailwindcss and autoprefixer plugins. Vite/PostCSS expect CommonJS by default.
Harden network handling in frontend/src/components/InvoicesPage.tsx:
- Use an AbortController for the invoices fetch and abort on cleanup to avoid setting state after unmount.
- Check res.ok and surface API error payloads (use data.error when available).
- Validate response shapes (ensure data.invoices and data.employees are arrays) before calling setInvoices/setEmployees.
- Guard total rendering (e.g., ensure typeof total === 'number' before calling toFixed) so malformed data does not throw.
Review frontend/package-lock.json changes — regenerate from a clean install if only Tailwind + TanStack were intended to avoid massive transitive changes.

Summary of Changes

backend:
- backend/internal/server/server.go: Added default ordering ORDER BY i.Date ASC, i.id ASC to invoices query.
frontend:
- Added Tailwind/TanStack dependencies in frontend/package.json / frontend/package-lock.json.
- Replaced app CSS with Tailwind-based frontend/src/index.css (currently an @import that must be replaced with Tailwind directives).
- frontend/vite.config.ts: Added @tailwindcss/vite plugin.
- frontend/src/App.tsx: Routes now point to the new InvoicesPage.
- frontend/src/components/InvoicesPage.tsx: New invoices page with date filters, employee selector, debounced fetch, and TanStack table (needs network and shape hardening).
- Removed legacy assets and styles (frontend/src/App.css, some svg assets).
- specs/000-proof-of-concept.md updated with frontend progress.

Overall Feedback

Negative: There are a few blocking issues that will likely break builds or produce runtime errors for users:
- PostCSS/Tailwind is not configured as CommonJS; with the current @import "tailwindcss" and no CommonJS postcss.config, Vite/PostCSS may not process Tailwind and the app will look unstyled or fail to build.
- Frontend fetch flows assume perfect API responses and don't cancel inflight invoice requests. This can cause race conditions, set state on unmounted components, and crash on malformed data (e.g., calling toFixed on undefined). These must be guarded.
Positive: The new frontend is well structured and shows strong UX thinking — good default previous-month date logic, debounced fetching to reduce load, clear form layout, and pragmatic use of TanStack Table for sorting/display. Backend change to add deterministic ordering on invoices is a sensible safety improvement. Nice work! 🎉

@Pertempto — ping me after you push the PostCSS config and the fetch hardening and I’ll re-review the PR quickly and help with any follow-ups.

# Changes Requested Please address the following before merging: - Fix Tailwind/PostCSS setup in the frontend: - Replace `frontend/src/index.css` content with Tailwind directives (`@tailwind base; @tailwind components; @tailwind utilities;`). - Add a CommonJS PostCSS config file (`frontend/postcss.config.cjs` or a CommonJS `postcss.config.js`) that exports `tailwindcss` and `autoprefixer` plugins. Vite/PostCSS expect CommonJS by default. - Harden network handling in `frontend/src/components/InvoicesPage.tsx`: - Use an `AbortController` for the invoices fetch and abort on cleanup to avoid setting state after unmount. - Check `res.ok` and surface API error payloads (use `data.error` when available). - Validate response shapes (ensure `data.invoices` and `data.employees` are arrays) before calling `setInvoices`/`setEmployees`. - Guard `total` rendering (e.g., ensure `typeof total === 'number'` before calling `toFixed`) so malformed data does not throw. - Review `frontend/package-lock.json` changes — regenerate from a clean install if only Tailwind + TanStack were intended to avoid massive transitive changes. ## Summary of Changes - backend: - `backend/internal/server/server.go`: Added default ordering `ORDER BY i.Date ASC, i.id ASC` to invoices query. - frontend: - Added Tailwind/TanStack dependencies in `frontend/package.json` / `frontend/package-lock.json`. - Replaced app CSS with Tailwind-based `frontend/src/index.css` (currently an `@import` that must be replaced with Tailwind directives). - `frontend/vite.config.ts`: Added `@tailwindcss/vite` plugin. - `frontend/src/App.tsx`: Routes now point to the new `InvoicesPage`. - `frontend/src/components/InvoicesPage.tsx`: New invoices page with date filters, employee selector, debounced fetch, and TanStack table (needs network and shape hardening). - Removed legacy assets and styles (`frontend/src/App.css`, some svg assets). - `specs/000-proof-of-concept.md` updated with frontend progress. ## Overall Feedback - Negative: There are a few blocking issues that will likely break builds or produce runtime errors for users: - PostCSS/Tailwind is not configured as CommonJS; with the current `@import "tailwindcss"` and no CommonJS `postcss.config`, Vite/PostCSS may not process Tailwind and the app will look unstyled or fail to build. - Frontend fetch flows assume perfect API responses and don't cancel inflight invoice requests. This can cause race conditions, set state on unmounted components, and crash on malformed data (e.g., calling `toFixed` on undefined). These must be guarded. - Positive: The new frontend is well structured and shows strong UX thinking — good default previous-month date logic, debounced fetching to reduce load, clear form layout, and pragmatic use of TanStack Table for sorting/display. Backend change to add deterministic ordering on invoices is a sensible safety improvement. Nice work! 🎉 @Pertempto — ping me after you push the PostCSS config and the fetch hardening and I’ll re-review the PR quickly and help with any follow-ups.

github-actions[bot] commented

2025-11-21 12:48:22 -05:00

(Migrated from github.com)

frontend/src/index.css — please replace the single @import "tailwindcss"; with the three Tailwind directives and ensure PostCSS is configured. Minimal changes I expect:

Replace file contents with:

@tailwind base;
@tailwind components;
@tailwind utilities;

Add a PostCSS config using CommonJS (name it postcss.config.cjs or postcss.config.js but export with CommonJS). Example minimal file (postcss.config.cjs):
```
module.exports = {
  plugins: {
    tailwindcss: {},
    autoprefixer: {},
  },
};
```

Reason: Vite/PostCSS expect CommonJS for postcss.config.js unless the package.json has type: "module". Right now I can't find a PostCSS config in the change — builds will break or Tailwind won't be applied. Please add the config (or rename to .cjs) and update index.css as above.

`frontend/src/index.css` — please replace the single `@import "tailwindcss";` with the three Tailwind directives and ensure PostCSS is configured. Minimal changes I expect: - Replace file contents with: ``` @tailwind base; @tailwind components; @tailwind utilities; ``` - Add a PostCSS config using CommonJS (name it `postcss.config.cjs` or `postcss.config.js` but export with CommonJS). Example minimal file (`postcss.config.cjs`): ```js module.exports = { plugins: { tailwindcss: {}, autoprefixer: {}, }, }; ``` Reason: Vite/PostCSS expect CommonJS for `postcss.config.js` unless the package.json has `type: "module"`. Right now I can't find a PostCSS config in the change — builds will break or Tailwind won't be applied. Please add the config (or rename to `.cjs`) and update `index.css` as above.

Rows
Columns

feat: simple frontend #4

Changes Requested

Summary of Changes

Overall Feedback