Production release: security fixes, admin activity metrics, and the dev-vs-main review#121
Merged
Conversation
…n prerendered HTML
Changes:
- Render locked nav items as anchors carrying their real href, marked aria-disabled with tabIndex -1 and a click guard, instead of hrefless disabled buttons
- Keep the plain disabled button only for items with no page behind them, and add a shared PLACEHOLDER_HREF constant for that check
- Wrap SidebarProductNav in its own Suspense boundary with a matching skeleton, so its useSearchParams call stops taking the whole sidebar out of static HTML
An SEO audit of production found two problems. Every destination worth crawling rendered as a disabled button with no href, so /map, /statistics, /updates and /suggestions sat in sitemap.xml with zero internal links pointing at them. Separately, the landing page shipped an empty aside carrying a BAILOUT_TO_CLIENT_SIDE_RENDERING marker, because useSearchParams inside the product nav bailed the layout's fallback={null} boundary and dropped the entire sidebar from every prerendered route.
Notes:
- Access is unchanged. isLocked still gates who can navigate, non-admins get pointer-events-none and opacity-50 from the existing cva in components/ui/sidebar.tsx, and the coming-soon badges all stay. This is preparation, not a launch.
- The disabled styling needed no new CSS, sidebarMenuButtonVariants already carries aria-disabled:pointer-events-none and aria-disabled:opacity-50.
- Query-string hrefs are safe here, /products?chain=konzum already canonicals to /products.
- /map, /statistics and /updates live inside SidebarProductNav, so they are still client-filled on statically prerendered routes. They are crawlable on the dynamic ones. Footer nav links were considered and deliberately left out.
- The bailout only happens in a production build, so the Suspense fix cannot be reproduced against a dev server and needs a post-deploy check.
fix(seo): Make coming-soon nav items crawlable and keep the sidebar in prerendered HTML
Bumps [lucide-react](https://github.com/lucide-icons/lucide/tree/HEAD/packages/lucide-react) from 0.561.0 to 1.26.0. - [Release notes](https://github.com/lucide-icons/lucide/releases) - [Commits](https://github.com/lucide-icons/lucide/commits/1.26.0/packages/lucide-react) --- updated-dependencies: - dependency-name: lucide-react dependency-version: 1.26.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…ntend/lucide-react-1.26.0 chore(deps): Bump lucide-react from 0.561.0 to 1.26.0 in /frontend
Changes: - Bump org.projectlombok:lombok 1.18.30 to 1.18.46, in both the dependency and the annotationProcessorPaths entry - Bump io.sentry:sentry-spring-boot-starter-jakarta 8.16.0 to 8.50.1 - Widen the springdoc Dependabot ignore from major-only to every update type Taken from the backend group Dependabot proposed, minus its springdoc bump. springdoc publishes a line per Spring Boot line and 2.8.17 is built against Boot 3.5.13, so on this project's Boot 3.1.0 it would compile but could call Spring APIs added after 3.1 and fail at runtime. The matrix-correct pairing for Boot 3.1.x is 2.2.x, which is what stays pinned. The ignore only covered majors, so that minor slipped through; it now covers every update type. Notes: - lombok compile-verifies through the annotation processor, and the changelog shows no generated-code changes for the annotations used here. The codebase has no records and no lombok.config, so the 1.18.34 `@lombok.Generated` default is a no-op. - Sentry is configured declaratively only; none of the five `sentry.*` keys in application.properties were renamed, deprecated or removed across 8.16.0 to 8.50.1, and Boot 3.x support was never dropped. - The backend has no tests, so CI proves compilation only. Smoke-test /api-docs and confirm Sentry still receives events after deploying to dev.
Changes: - Bump the typescript devDependency from `^5.7` to `^6.0.3` TypeScript 6.0 is the last release built on the JavaScript codebase and is explicitly API-compatible with 5.9, so it is a drop-in here. Verified locally against this branch: `next typegen` generates route types successfully and `tsc --noEmit` is clean, with eslint reporting no errors. Worth recording why the Dependabot PR for this failed. On that branch `next typegen` aborted with "trying to use TypeScript but do not have the required package(s) installed", emitted nothing, and `tsc` then failed on missing `PageProps` and `RouteContext`. That was its regenerated lockfile rather than TypeScript 6, which a clean `pnpm add` resolve does not reproduce. Note the typegen step still reports success in that failure mode, because the unhandled rejection sets no exit code, which disguises the cause as a typecheck error. Notes: - TypeScript 7 is deliberately not taken: it is the Go port, whose package drops the `lib/typescript.js` Compiler API that Next's checker loads, and Next 16.2.x needs an `experimental.useTypeScriptCli` opt-in for it. - typescript-eslint 8.64 declares support for `>=4.8.4 <6.1.0`, so a future 6.1 would fall outside its tested range and emit an unsupported-version warning. Dependabot will surface that bump for review when it lands.
chore(deps): Bump lombok and Sentry, hold springdoc at 2.2.x
chore(deps): Move TypeScript to 6.0.3
…range Changes: - Move @types/node from `^25` to `^24.13.3` - Narrow typescript from `^6.0.3` to `~6.0.3` @types/node majors track the Node major they describe, and this project runs Node 24 in CI and in the Docker image. It was pinned to `^25`, a short-lived Current-only line that reached end of life on 2026-06-01 and was never the runtime here, so the compiler was being told about APIs that do not exist at run time. That mismatch is invisible to tsc by construction. typescript-eslint 8.64 declares support for `>=4.8.4 <6.1.0`, and the caret range would have let a future 6.1 install straight past that ceiling without review. The tilde keeps 6.0.x patches flowing and leaves the 6.1 decision to a Dependabot PR. Notes: - Both flagged by a full version-compatibility audit of the stack. - `eslint-config-next` is one release behind `next` (16.2.2 against 16.2.11); the pending frontend group PR already carries that bump, so it is deliberately not touched here to avoid a conflict.
…r-button height Changes: - Add components/custom/common/sort-select.tsx, a label plus dropdown generic over its mode type, rendering coming-soon options itself - Reduce product-chain-sort-select and store-optimize-select to their option lists, dropping the duplicated wrapper markup and the string casts at both call sites - Left align the control, keep the label and dropdown inline on mobile instead of wrapping to full width, and raise the trigger to min-h-10 so it matches the Filteri button The two sort controls carried identical wrapper markup, so the same three layout problems had to be fixed twice. They were right aligned, the trigger went full width below the label on mobile, and size="sm" left it visibly thinner than every neighbouring control. Notes: - The trigger keeps the primitive's w-fit rather than a fixed width. SelectValue carries line-clamp-1, whose overflow:hidden collapses the flex item's automatic minimum size, so any fixed width plus shrinking ellipsises the option text. min-w-52 only sets a floor, so the control does not resize as options change. - The height override is min-h-10 rather than h-10 on purpose. select.tsx sizes itself with data-[size=default]:h-9, whose attribute selector out-specifies a plain h-10 utility, so only min-height wins. min-h-* is also how MultiSelectTrigger already sizes itself. - flex-wrap on the row is an overflow valve for viewports under roughly 320px, where the label and a 13rem dropdown no longer fit side by side. - StoreOptimizeSelect's value prop tightened from string to StoreOptimizeMode, so its caller's handler no longer casts.
… price cells Changes: - Route countChainsAtExtreme through getPriceExtreme instead of comparing against a raw Math.min/Math.max result - Take the wanted extreme as a "min" or "max" argument rather than a pickExtreme callback, since the shared helper now decides A store chain card could show both the cheapest and the most expensive arrow while none of its products were marked. The badges counted any chain whose average price equalled the extreme, so a product priced identically at every chain made every chain both the cheapest and the most expensive for it. The price cells never had that problem because getPriceExtreme returns null when min equals max, which is exactly the guard the badge counter was missing. Notes: - countCheapestByChain also ranks chains in the "Zasebnim proizvodima" mode, so that ordering shifts slightly. Uniformly priced products no longer pad every chain's cheapest count, which is the intended reading. - Still outstanding, in the other direction: the badges count only unchecked items while the table colours checked ones too, so ticking off the product behind a badge leaves a highlighted row with no badge.
…-ts-range chore(deps): Match @types/node to the runtime and cap the TypeScript range
…badges fix(ui): Share one sort select and stop chain price badges appearing with no marked row
Changes: - better-auth 1.6.14 to 1.6.25 and kysely 0.28.17 to 0.29.4, together - react and react-dom 19.2.4 to 19.2.8, @types/react to 19.2.17 - eslint-config-next 16.2.2 to 16.2.11, matching the pinned next - pg 8.21.0 to 8.22.0 and recharts 3.9.0 to 3.10.0 - Drop the stale kysely notes from AGENTS.md and docs/DEPLOYMENT.md Applied by hand rather than merging the Dependabot group PR, which failed to rebase onto dev three times. kysely could finally move because the jwtClient() plugin was removed earlier on this branch line. better-auth widened its kysely range in 1.6.12 before fixing the code, so 1.6.12 through 1.6.14 genuinely broke on 0.29; the fix landed in 1.6.15. With the plugin gone and better-auth on 1.6.25, the pin has no reason to exist, so the notes explaining it are removed. eslint-config-next is published in lockstep with next, so leaving it nine releases behind the pinned next was drift rather than a deliberate pin. Notes: - Scoped to the exactly-pinned dependencies only. The caret-ranged deps are deliberately untouched, so the loose specifiers such as `^4` and `^9` keep their intended meaning. - Verified locally: `next typegen` succeeds, `tsc --noEmit` is clean, prettier passes and eslint reports 0 errors. - This touches the auth stack, so smoke-test login, an authenticated API call and JWT issuance on dev before promoting.
chore(deps): Bump the frontend dependency group
Changes: - Give recharts a `^3.10.0` range instead of an exact pin - Drop the pinning convention note from AGENTS.md An exact version only controls what Dependabot and `pnpm add`/`update` may move you onto, since the committed lockfile already fixes what actually installs. That review gate is worth it for RSC-adjacent, auth and data-layer packages, and not for a chart library, where it just produced a pull request per patch release. The AGENTS.md note is removed rather than reworded, to keep the file lean.
chore(deps): Free recharts from the exact-pin set
Changes: - Remove the "Versions live in frontend/package.json" line from the frontend Tech stack section - Remove the "Versions are managed by the Spring Boot parent (3.1.0)" lead-in from the backend Tech stack section, keeping "Non-obvious notes:" for the bullets below Both lines only restated that versions live in the manifests, which the "Installed libs" line right below already says. The backend one also hardcoded the Spring Boot version, which would have gone stale with the Boot 4 migration. Notes: - Boot 4 migration is tracked in #110.
…users Changes: - Add app_user.last_active_at, stamped throttled in ensureActiveProfile on every authenticated request - Expose lastLoginAt and lastActiveAt on UserDto, sourced from better-auth session rows via the existing email backfill query - Add AuthIdentity record so the admin list reads email, last login and last seen in one native query - Add AdminUsersStats and AdminActivityCard with per-card trailing-window selects (default 7 and 30 days) - Extract AdminUserRow from AdminUsersTable and add the Zadnja prijava column - Rename SortSelect to LabeledSelect and repoint its two call sites The admin dashboard had no way to see whether an account was still in use. Last login answers that per user, and the WAU/MAU counters give the same answer in aggregate over a window the admin picks. Notes: - Login and activity are deliberately separate signals: a long-lived session means last login can be weeks old while the user is active daily, so the counters read last_active_at, not last_login_at - Session rows are removed on sign-out and expire after 7 days, so the durable stamped column is what makes MAU trustworthy; existing users are covered by latestOf(stamped, MAX(session.updated_at)) until the stamp catches up - The windows are trailing presets rather than a two-ended calendar range because only the latest activity per user is stored, so a past-to-past range would undercount - ddl-auto=update adds the nullable column on the next backend restart
Changes: - Add a General-section bullet permitting dev server and build commands during framework or dependency migrations, on both frontend and backend The Spring Boot 4 migration cannot be verified by reading alone, so the blanket ban on build commands would have left an agent unable to check its own work. Placed in General rather than a stack section, since the same reasoning applies to a future Next.js or React major. Notes: - Scoped to migrations, not general development - The backend still has no tests, so a green "mvn -B verify" proves compilation only
Changes: - Add "sharp@<0.35.0": "^0.35.3" to the pnpm overrides - Add "fast-uri@<3.1.4": "^3.1.4" to the pnpm overrides - Regenerate pnpm-lock.yaml, dropping the duplicate sharp 0.34.5 tree Dependabot alerts 13 and 14 flag two high-severity transitive deps that it cannot patch itself, since neither is a direct dependency. sharp 0.34.5 came in as Next 16.2.11's own optional dependency and carries the libvips CVEs (CVE-2026-33327, -33328, -35590, -35591); it is the copy Next resolves for production image optimization, because our own sharp is a devDependency and is pruned from the runtime image. fast-uri 3.1.3 arrives via react-email -> conf/schema-utils -> ajv and is build-time only. Notes: - Next declares sharp as optionalDependencies "^0.34.5", so forcing 0.35.3 is deliberate. Verified safe: its image optimizer only calls sharp(), touching none of the APIs 0.35.0 removed (failOnError, paletteBitDepth, jp2k) and Node 24 clears the new >= 20.9.0 floor. - The override collapses the two sharp copies into one, so the lockfile loses 24 @img/sharp-* platform binary entries and shrinks by ~276 lines. - Same range-selector pattern as the existing shell-quote/postcss/esbuild overrides, so already-safe copies are untouched.
…ansitive-deps fix(deps): Override the vulnerable transitive sharp and fast-uri copies
Changes: - Add `versions: [">=2.3.0"]` to the springdoc ignore entry - Note in the comment why the bound exists A bare `dependency-name` ignore suppresses Dependabot security updates as well as version updates. SecurityConfig serves /api-docs/**, /v3/api-docs/** and /swagger-ui/** with permitAll(), so an unauthenticated Swagger surface frozen at springdoc 2.2.0 would never receive a security PR. Bounding the ignore at >=2.3.0 keeps the Boot-4-only line blocked while letting 2.2.x patches and security fixes through. Notes: - Review finding 1 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Inert until this reaches main; Dependabot reads the config from the default branch only.
Changes: - Add a private nowUtc() helper to UserService and use it for all six timestamp writes - Stamp createdAt via LocalDateTime.now(ZoneOffset.UTC) in User.onCreate latestOf compares app_user.last_active_at against MAX(session.updated_at), which drizzle always writes in UTC. Stamping with LocalDateTime.now() used the JVM default zone, so the two operands only shared a clock base while the JVM happened to run UTC. Off Docker (local dev on Europe/Zagreb) the stamped column sat 1-2h ahead and always won the merge, and the same column held a different clock base per environment. Notes: - Review finding 6 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Prerequisite for finding 4: serialising these as UTC on the wire is only correct once the stored values genuinely are UTC. - Scoped to the user domain. Other domains still stamp via BaseEntity in the JVM zone, which is harmless today since nothing compares them to better-auth.
Changes: - Change UserDto.lastLoginAt / lastActiveAt and both AuthIdentity components to Instant - Convert the app_user column with toUtcInstant() in convertToUserDto - Replace toLocalDateTime with toInstant, handling Timestamp, Instant, OffsetDateTime and LocalDateTime, and logging an unmapped type instead of returning null Jackson serialized LocalDateTime without an offset, so the browser parsed "2026-07-24T13:05:11.123" as local time. In Zagreb that made a just-signed-in user render as "prije 2 sata", and shifted the dashboard's WAU/MAU window by the viewer's UTC offset so window-edge users landed in the wrong bucket and counts differed per admin. The wire format is now "...Z". The zod schemas stay z.string(), so parsing is unaffected; only the admin dashboard reads these two fields. Notes: - Review findings 4 and 10 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - toInstant converts a Timestamp via toLocalDateTime() first on purpose: the driver reads a zone-less column in the JVM zone, so calling toInstant() directly would re-apply that offset and shift the value again. - Depends on the preceding UTC-stamping commit; converting with ZoneOffset.UTC is only correct because the column is now written in UTC.
Changes:
- Add a <lombok.version> property set to 1.18.46
- Drop the hard-coded version from the lombok dependency
- Reference ${lombok.version} from the annotation processor path
The version was hard-coded in two places, so the last bump had to edit both
and a future bump that missed one would compile against a different lombok
than it declares.
Notes:
- Review finding 28 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md)
- The property name is the one spring-boot-dependencies uses, so declaring it
overrides the parent-managed version. Simply deleting both version elements
would have downgraded lombok to whatever Boot 3.1.0 pins, undoing 3c40b6a.
Changes: - Add AuthIdentityDao holding findByUserIds, deleteById and the timestamp coercion - Inject it into UserService and drop the direct EntityManager dependency - Remove the now-unused Timestamp, HashMap, OffsetDateTime and persistence imports Reading and deleting rows in better-auth's `user` / `session` tables is a different concern from the app_user profile service: that schema is owned by the frontend's auth library and has no JPA entities, so it needs native queries. Keeping it inline left UserService at 316 lines with two raw SQL statements sitting next to the profile logic. Notes: - Review finding 34 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - UserService is now 291 lines, still above the AGENTS.md 50-100 target. The remaining bulk is genuine profile logic and would need a separate split.
Changes: - Export MILLISECONDS_PER_DAY from utils/date.ts - Import it in user-activity.ts instead of redeclaring it AGENTS.md requires checking utils/ for an existing helper before writing a new one. The constant already existed but was module-private, so the dashboard util declared a second copy of the same value. Notes: - Review finding 31 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md)
…ending Changes: - Read isLoading and isError from useGetAllUsers in AdminUsersStats - Pass both into AdminActivityCard and render a Skeleton or the error text - Show the count only once the query has resolved successfully countActiveUsers(undefined, ...) returns 0, so both cards asserted a confident "0 tjedno aktivnih korisnika" while the request was still in flight or had failed, sitting right next to the users table's own error message. Zero was indistinguishable from unknown. Notes: - Review finding 5 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Flagged independently by all three engines, the joint-highest consensus in the review. - Error copy matches the users table so the panel reads consistently.
Changes: - Track isDefaultWindow and show the specific title plus abbreviation only then - Fall back to the generic "Aktivni korisnici" for any other window The heading was fixed while the window is user-selectable, so picking "Zadnjih 90 dana" left "Tjedno aktivni korisnici (WAU)" sitting over a 90-day number, and both cards could be set to the same window and show the identical figure under two different labels. Notes: - Review finding 38 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md)
Changes: - Add formatDateTime to utils/strings, reusing formatDate for the date half - Add a RelativeTime component that guards an unparsable value and waits for mount - Render the last-login cell through it formatRelativeTime has no invalid-date guard: it falls through its DIVISIONS loop on NaN and calls Intl.RelativeTimeFormat.format(NaN, "years"), which throws a RangeError and takes down the client tree. useGetAllUsers returns raw axios data with no zod parse, unlike the contact and digital-card services, so an unparsable lastLoginAt really can reach the cell. The same line also read Date.now() during the server render with no mount guard, so the relative label could mismatch on hydration and then never refresh. RelativeTime follows the last-synced-label precedent and uses useMinuteTick, rendering the absolute timestamp until mounted. Notes: - Review findings 7 and 39 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - The hydration half was not in the review; it was found while fixing finding 7. - The tooltip now carries the time too, which finding 39 asked for: formatDate emits only dd.mm.yyyy., so hovering "prije 5 minuta" revealed nothing.
Changes: - Split the shared comment so each field describes itself - Note that lastActiveAt is present on every UserDto, not just the admin list The single "Admin list only" comment covered both fields but was true only for lastLoginAt: convertToUserDto sets lastActiveAt, so GET and PATCH /api/users/me return it too. The comment is the only contract doc for these fields, so it would have sent the next consumer to the admin list for data /me already has. Notes: - Review finding 27 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md)
Changes: - Point admin-activity-card and user-activity at @/lib/api/schemas/auth-user The dashboard was split two-vs-two: the newer WAU/MAU files went through the @/lib/api/types barrel while the user-table files imported the schema module directly. Both resolve to the same type, but one feature folder using two paths for one type is churn waiting to happen. Notes: - Review finding 40 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Only the dashboard is normalised here. The contact-side files also use the barrel, but they are outside this review's scope.
Changes: - Move ILabeledSelectOption to @/typings and make it generic over TValue - Type options as readonly ILabeledSelectOption<TValue>[] - Resolve the picked option instead of casting the raw string back to TValue - Type each call site's array against its own union, adding ActivityWindowDays options accepted any string while onValueChange promised TValue, and the gap was closed by an unchecked `next as TValue`. A typo like "totl" compiled cleanly and arrived at setStoreOptimizeMode as a mode that silently never matches anything. Moving the type also removes the coupling that made a dashboard util import from a component module; AGENTS.md puts shared UI types in @/typings. Notes: - Review findings 13 and 32 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Finding 13 was flagged by all three engines. - The default type parameter keeps ILabeledSelectOption usable unparameterised.
Changes: - Generate a useId for the label span - Reference it from SelectTrigger via aria-labelledby The label was only visually adjacent, so the Radix role="combobox" trigger took its accessible name from the current value. A screen reader announced "Trgovinama, combobox" with no indication of what the control does, at every call site. On the dashboard it was worse: two sibling cards each announced only their selected period, with nothing tying either to WAU or MAU. Notes: - Review finding 14 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Flagged by all three engines, the broadest consensus in the review. - aria-labelledby rather than aria-label so the announced name stays in sync with the visible text.
Changes: - Put justify-end back on the wrapper and accept a className override - Restore the trigger's w-full sm:w-60 and size="sm" - Drop the comment explaining the min-h workaround it no longer needs Extracting sort-select into LabeledSelect silently lost three layout classes that both original call sites had: the control stopped being right-aligned, stopped filling the width on mobile, and grew from the small trigger to the default height. That is a visible regression on the product detail page and the shopping-list stores panel. The activity cards opt out of the right alignment, since they sit under a left-aligned number rather than above a list. Notes: - Review findings 12 and 36 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Only Codex caught this one. - Verified the wrapper and trigger classes now match main, modulo the tailwind plugin's class ordering.
Changes: - Restore the sort wording on the product detail chain control The LabeledSelect extraction swapped in "Optimiziraj po:", borrowed from the shopping-list optimize control, which is a different concept. This picks how the chains carrying one product are ordered, and its options are still the dative "Trgovinama / Cijeni / Udaljenosti" written for "Sortiraj po", so the control mislabelled itself on every product detail page. Notes: - Review finding 11 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Keeps the trailing colon, which is now the convention for every label rendered through LabeledSelect.
Changes: - Change the child skeletons from h-7 to h-8 The real child row is SidebarFilterMenu, whose collapsible trigger is a plain SidebarMenuButton at the default h-8, not a SidebarMenuSubButton at h-7. Every child row therefore jumped 4px when the Suspense boundary resolved, which is exactly what the file's own "Mirrors the real row heights so nothing shifts" comment promises it does not do. Notes: - Review finding 17 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md)
Changes: - Add SidebarProductNavShell holding the group, label and menu wrapper - Render both the real nav and the skeleton through it The group label "Istraži" and the SidebarGroup / SidebarGroupContent / SidebarMenu structure were copy-pasted between the two files, so the skeleton would silently stop matching the real nav as soon as either changed shape. The h-7 versus h-8 mismatch fixed in the previous commit is what that drift looks like in practice. Notes: - Review finding 35 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md)
Changes: - Add useClientSearchParams, a client-only query-string read - Use it in SidebarProductNav so the group no longer bails out of prerendering - Wrap only the filter menus in Suspense, with a new SidebarFilterMenuSkeleton - Drop the whole-group boundary from AppSidebar and delete its skeleton SidebarProductNav called useSearchParams purely for active-state and filter params, so the Suspense boundary around it put a skeleton, rather than the Popusti / Karta / Statistika / Novosti anchors, into the static HTML. Those hrefs come from a constant and need no request data, so this partly undid the crawlable-nav fix in c11ea04. The anchors now render statically and only SidebarFilterMenu, which genuinely needs the URL, sits behind a boundary. Notes: - Review findings 16, 17 and 35 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Agreed trade: the ?discounted=true highlight on Popusti now resolves just after hydration instead of in the first paint. Navigating from /products to /products?discounted=true keeps the same pathname, so the highlight there settles on the next navigation; every other entry point is correct. - This supersedes the whole-group skeleton that findings 17 and 35 had just fixed and deduplicated. The h-8 correction from 17 carries over into SidebarFilterMenuSkeleton, which is the only skeleton left.
…e extremes Changes: - Add getChainAvgPriceRange to product-price-utils and call it from all three duplicates (store-chain-extremes, shopping-list-items-table-utils, store-sorting-utils) - Memoise productsData through useQueries' combine option - Delete findCompleteStoresAnalysis, extremeByAvgPrice, ICompleteStoresAnalysis and the prop threading behind them - Give the store-card header arrows aria-hidden plus sr-only text - Split the price row into StoreCardPriceStat and add an sr-only extreme label - Put the optimize option labels in the locative The "parse every chain's avg_price, drop NaN, take min/max" block existed three times with three different empty-case behaviours, which is exactly how the badge and cell rules drifted apart before. productsData was rebuilt outside any memo, so it took a new identity every render and invalidated the allChains, cheapestCountByChain and storesWithHighestPriceItems memos plus the two memo() components downstream. completeStoresAnalysis was computed, threaded through two components and never destructured, implying a "best basket" marker that was not wired up. Cheapest and most-expensive were signalled by icon and colour only, with no text alternative, even though shopping-list-item-row already sets that precedent; the price row was colour-only, which fails WCAG 1.4.1. Notes: - Review findings 20, 21, 22, 23, 33, 36, 37, 40 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - combine is the repo's first use of that option; it is memoised by TanStack. - computeAbsolutePrices and chainsStockingEveryItem are still live and untouched. - "Optimiziraj po:" governs the locative, so three of the four labels were in the wrong case.
Changes: - Raise the sharp devDependency to ^0.35.3 via pnpm add - Note in AGENTS.md that kysely is a direct dep only to pin better-auth's peer - Rewrite the stale DEPLOYMENT.md row that said pnpm overrides are not honored The sharp override declares everything below 0.35.0 unsafe and forces ^0.35.3, but the direct devDependency still asked for ^0.35.2, which the selector does not match. That copy sat outside the override's protection and landed on a safe version only by luck of "latest". Three reviewers independently read kysely as dead weight because nothing imports it. It is not: @better-auth/core declares kysely as a NON-optional peer and the root entry is what satisfies it, with drizzle-orm resolving its own optional peer against the same copy. Removing it would not break the build, since auto-install-peers defaults to true, but the version would float within "^0.28.5 || ^0.29.0" on an auth-critical package. The DEPLOYMENT.md gotcha was the source of that confusion: it told readers overrides do not work and to use a direct dependency instead. Notes: - Review findings 3, 24 and 25 (reviews/REVIEW-2026-07-24-DEV-VS-MAIN.md) - Finding 24 asked for kysely to be removed. Rejected after checking the peer declarations; the real defect was the missing rationale, which is now recorded.
Changes: - Collapse the multi-line explanatory comments added across this branch - Run prettier over three shopping-list files the batch gate missed AGENTS.md asks for single-line comments only where genuinely needed, and says a multi-line explanation usually means the code should be rewritten instead. Several comments introduced earlier on this branch broke that. Notes: - Comment-only for the backend; no behaviour change.
…chParams Changes: - AUTH.md: add AuthIdentityDao to the key-files map and note UserService stamps UTC - AUTH.md: add three gotchas covering the naive-timestamp stack, why the two dashboard fields are Instant, and that lastLoginAt is derived rather than stored - STATE-PERSISTENCE.md: expand the useSearchParams gotcha to explain that the Suspense fallback is what gets prerendered, and point at useClientSearchParams - STATE-PERSISTENCE.md: correct the stale next version in the library table The docs described the code as it was before this branch. The timezone fix and the sidebar prerender fix both turned on behaviour that was written down nowhere, which is why each took a full review to surface. Notes: - Docs only, no code change.
…2026-07 fix: Apply the dev-vs-main review findings
❌ Deploy Preview for disscount failed. Why did it fail? →
|
Contributor
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (46)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Production promotion. 35 commits, 47 files,
+1909 / -1645.Two high-severity security alerts, one new admin feature, one SEO regression repaired, and 31 findings from a 7-reviewer sweep of this exact diff.
1. Security
Closes Dependabot #13 and #14
Both are high severity and both are transitive, which is why Dependabot could not open a PR for either: it only edits versions you declared. They are fixed with
pnpm-workspace.yamloverrides, matching the existingshell-quote/postcss/esbuildentries.sharp@0.34.5(libvips CVE-2026-33327 / -33328 / -35590 / -35591)optionalDependenciesfast-uri@3.1.3(host confusion via literal backslash authority delimiter)react-emailtoconf/schema-utilstoajvsharpis the one that mattered. Our ownsharpis a devDependency, so it is pruned from the runtime image; the copy Next resolved for production image optimization was the vulnerable 0.34.5 nested undernext. The override collapses both into one 0.35.3 copy and drops 24 stale@img/sharp-*platform entries.Next declares
sharpasoptionalDependencies: "^0.34.5", so forcing 0.35.3 is deliberate. Verified safe:next/dist/server/image-optimizer.jscallssharp()and nothing else, touching none of the APIs 0.35.0 removed (failOnError,paletteBitDepth,format.jp2k), and Node 24 clears the new>= 20.9.0floor.These alerts stay open until this merges. Dependabot only rescans the default branch.
The springdoc ignore was silently blocking security updates
Earlier in this cycle the springdoc ignore was widened and lost its
update-typesfilter. A baredependency-nameignore suppresses security updates too. SinceSecurityConfig.java:51-55serves/api-docs/**,/v3/api-docs/**and/swagger-ui/**withpermitAll(), an unauthenticated Swagger surface pinned at 2.2.0 would never have been told it was vulnerable. Now bounded atversions: [">=2.3.0"], so the Boot-4-only line stays blocked while 2.2.x patches get through.2. The timezone bug (highest-confidence finding in the review)
Three reviewers reached this independently from both ends.
UserDto.lastLoginAtandlastActiveAtwere JavaLocalDateTime, so Jackson emitted"2026-07-24T13:05:11.123"with no offset.spring.jackson.time-zoneis unset and the container JVM runs UTC, so the value really was UTC, butnew Date(...)in the browser parses an offset-less string as local time.In Zagreb that meant:
The fix is two dependent commits, in order:
1450b55addsnowUtc()and stamps everyapp_usertimestamp in UTC. This was a real bug on its own:last_active_atwas written withLocalDateTime.now()(JVM zone) but compared againstMAX(session.updated_at), which Drizzle writes in UTC. The two only shared a clock base while the JVM happened to be UTC, so running the backend off Docker made the stamped column always win the merge.a2e2197then changes the two DTO fields andAuthIdentitytoInstant. Converting to UTC is only correct because step 1 landed first.The wire format gains a
Z. The zod schemas stayz.string(), so nothing breaks in parsing, and only the admin dashboard reads these two fields.One subtlety worth knowing for future work:
toInstantconverts aTimestampviatoLocalDateTime()first on purpose. The JDBC driver builds aTimestampfor a zone-less column by reading it in the JVM zone, so callingtoInstant()directly would re-apply that offset and shift the value again.3. New feature: admin activity metrics
ec05803adds a last-login column and WAU/MAU counters to the admin users table. It had never been reviewed by any tool before this sweep, and it accounted for the largest share of findings. What shipped is the hardened version:0while the query is pending or has failed (flagged by all three engines) — a skeleton shows insteadformatRelativeTimehas no invalid-date guard and throwsRangeErroronNaN, anduseGetAllUsersreturns raw axios data with no zod parse, unlike the contact and digital-card servicesDate.now()-derived text with no mount guard, so it could mismatch on hydration and then never refresh. Now usesuseMinuteTick, matching thelast-synced-labelprecedent4. SEO and rendering
c11ea04made coming-soon nav items crawlable. The subsequent sidebar work partly undid it:SidebarProductNavcalleduseSearchParamspurely for active state, and during a static prerender a Suspense boundary emits its fallback, so the skeleton, not thePopusti/Karta/Statistika/Novostianchors, went into the served HTML.c0c050cintroducesuseClientSearchParams, which reads the query string after mount so the caller stays in the prerender. OnlySidebarFilterMenu, which genuinely needs the URL, sits behind a boundary now.Known trade, agreed during review: the
?discounted=truehighlight on Popusti resolves just after hydration rather than in the first paint. Navigating/productsto/products?discounted=truekeeps the same pathname, so that one case settles on the next navigation. Every other entry point is correct.5. UI regressions repaired
The
sort-selecttolabeled-selectextraction changed behaviour at its call sites. All three are user-visible and were verified againstmainby hand:justify-end,w-full sm:w-60andsize="sm"were dropped, so all three selects lost right alignment and mobile full width and grew talleroptionsaccepted anystringwhileonValueChangepromisedTValue, closed by an unchecked cast, so a typo'd mode compiled and silently never matchedrole="combobox"trigger announced only its current value. On the dashboard, two sibling cards each announced just their period with nothing tying either to WAU or MAU6. Correctness and accessibility elsewhere
avg_price, drop NaN, take min/max" block existed three times with three different empty-case behaviours, which is how the badge and cell rules drifted apart originally. OnegetChainAvgPriceRangehelper now.productsDatawas rebuilt outside any memo, taking a new identity every render and invalidating four downstream memos plus twomemo()components. Now built throughuseQueries'combine, which TanStack memoises. First use of that option in the repo.completeStoresAnalysiswas computed, threaded through two components and never destructured, implying a "best basket" marker that was never wired up.sr-onlytext, matching the precedent already set inshopping-list-item-row.7. Dependencies
Next 16.2.11, TypeScript 6.0.3, lucide-react 1.26.0,
@types/node24, lombok 1.18.46, Sentry 8.50.1, plus the grouped frontend bump.rechartswas freed from the exact-pin set;kyselystays pinned.One reviewer finding was rejected here. It called the direct
kyselydependency dead weight because nothing imports it. It is not:@better-auth/core@1.6.25declareskyselyas a non-optional peer and the root entry is what satisfies it, withdrizzle-ormresolving its optional peer against the same copy. Removing it would not break the build, sinceauto-install-peersdefaults to true, but the version would then float within^0.28.5 || ^0.29.0on an auth-critical package. The real defect was that nothing recorded why it exists, which is why three reviewers read it as dead. Now noted inAGENTS.md, and the staleDEPLOYMENT.mdrow that claimed pnpm overrides "weren't honored" (and told readers to use a direct dependency instead) has been rewritten.8. Review provenance
7 independent reviewers over this diff: 5 Claude Opus 5 subagents at high effort, Codex
gpt-5.6-solat high, and CodeRabbit. Cursor was out of usage and did not run, so this diff has one fewer independent opinion than a full sweep; its folders were absorbed by the other three.40 findings ranked. 31 fixed, 6 filed as issues, 1 rejected, 2 informational.
Rejected: Codex's only backend finding claimed
last_active_atwas added without a migration.application.properties:11setsspring.jpa.hibernate.ddl-auto=update, so Hibernate adds the column itself.Also worth flagging: CodeRabbit reported zero backend findings across all four Java files, while the Claude reviewers found six there including the confirmed timezone bug. Treat CR's backend pass as weak evidence.
Filed rather than fixed: #115, #116 (both pre-existing, since one reviewer widened past the branch diff), #117, #118, #119, #120.
Deploy notes
ddl-auto=updateaddsapp_user.last_active_aton boot.lastLoginAtandlastActiveAtnow serialise with aZsuffix. Only the admin dashboard consumes them, and it ships in the same deploy.maincarries a scanner hotfix (25cbef1) thatdevnever received. Verified no file overlap and a clean merge, so it is preserved.Verification
prettier --checkandtsc --noEmitclean;mvn -B verifygreen on every commit of the fix branch.dev.disscount.me: the timezone fix end to end, the select layout at 375px, and the sidebar anchors in the served HTML. Worth a pass on dev before merging, since the timezone change alters a wire format.