Development#7
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Trivy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
dasuncodes
force-pushed
the
development
branch
11 times, most recently
from
79a17c1 to
6ed0f09
Compare
Hasintha/dev hardware service
Fix actions
…g and update dependencies
feat(client): introduce web frontend with shadcn/ui component library
feat(notification-service): integrate NATS for notification publishing and update dependencies
Hasintha/dev hardware service
Backend: - Add nil-checks and connection state validation for MQTT client in hardware-service handlers. - Prevent service panics when MQTT brokers are unreachable or slow to initialize. - Ensure service readiness state is correctly managed during dependency timeouts. Frontend: - Fix "Cannot read properties of undefined (reading 'toFixed')" in agronomist dashboard. - Replace strict null checks with loose checks (!= null) to handle undefined telemetry values. - Harden weather and threshold pages against missing sensor data. Infrastructure: - Finalize seeding script (seed_data.ps1) for automated resource provisioning.
fix: resolve hardware service panics and frontend dashboard crashes
📝 WalkthroughWalkthroughThis pull request introduces a complete Next.js frontend client application with user authentication, role-based dashboards, and API integration, while simultaneously integrating NATS JetStream event publishing across backend services for notifications. Infrastructure changes include per-service PostgreSQL databases, gateway routing for Swagger UI, and ESP32 firmware for IoT telemetry collection. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/iam-service/main.go (1)
131-149:⚠️ Potential issue | 🟠 MajorDefault-admin creation can stop before startup ever finishes.
The DB init loop now retries indefinitely, but this goroutine still gives up after 60 seconds. If Postgres or migrations need longer, the service can become healthy without the admin account and never retry creating it. Trigger admin creation from the successful migration path instead of a fixed retry window.
Suggested refactor
dbStatus.SetReady(db) dbStatus.SetMigrated() + if err := createDefaultAdmin(db); err != nil { + log.Printf("[WARN] Could not create default admin: %v", err) + } log.Println("[INFO] IAM Service fully ready") return } }() - - // Start background job to create default admin user once DB is ready - go func() { - for i := 0; i < 30; i++ { - if dbStatus.IsReady() && dbStatus.IsMigrated() { - if err := createDefaultAdmin(dbStatus.GetDB()); err != nil { - log.Printf("[WARN] Could not create default admin: %v", err) - } - break - } - time.Sleep(2 * time.Second) - } - }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/iam-service/main.go` around lines 131 - 149, Currently a background goroutine with a 60s retry window tries to run createDefaultAdmin and can give up too early; instead invoke admin creation from the successful DB init/migration path so it won’t time out. Remove the separate timed goroutine and, immediately after dbStatus.SetMigrated() (where DB init succeeds), call createDefaultAdmin(db) — spawn it as a fire-and-forget goroutine if you don’t want to block startup, or call it synchronously if startup should wait; ensure you log errors with log.Printf("[WARN] Could not create default admin: %v", err) and reference dbStatus, SetMigrated, SetReady, and createDefaultAdmin when making the change.docker-compose.yml (1)
230-235:⚠️ Potential issue | 🟠 MajorReplace the healthcheck with the official NATS /healthz endpoint.
The current
nats-server --helpcommand only verifies the binary exists, not broker readiness or JetStream availability. All services usingdepends_on.condition: service_healthywill proceed before the broker is actually serving connections.Use the official HTTP health endpoint instead:
Recommended healthcheck command
healthcheck: test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://localhost:8222/healthz?js-enabled-only=true"] interval: 5s timeout: 3s retries: 5 start_period: 5sThe
/healthz?js-enabled-only=trueendpoint checks if JetStream is enabled without performing full asset checks. See NATS monitoring docs for alternative parameters like?js-server-only=true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 230 - 235, Replace the healthcheck that runs the `nats-server --help` command with an HTTP probe against the NATS monitoring /healthz endpoint so readiness (and JetStream) is actually validated; in the `healthcheck` block (the `test` entry that currently runs `nats-server --help`) change it to run a one-shot HTTP probe (for example using `wget --no-verbose --tries=1 --spider "http://localhost:8222/healthz?js-enabled-only=true"`) and keep the existing interval/timeout/retries/start_period settings so services depending on `service_healthy` wait for the broker to be fully ready.
🟠 Major comments (26)
client/components/ui/chart.tsx-235-238 (1)
235-238:⚠️ Potential issue | 🟠 MajorZero values are incorrectly hidden in tooltip.
The code uses a truthy check at line 235 (
{item.value && (...)}), which suppresses valid0values from chart data. This is inconsistent with line 195, which properly checksitem?.value !== undefinedto allow zero values when a formatter is present.In chart tooltips, zero is a legitimate data point and should always be displayed.
Suggested fix
- {item.value && ( + {item.value !== undefined && item.value !== null && ( <span className="text-foreground font-mono font-medium tabular-nums"> {item.value.toLocaleString()} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/chart.tsx` around lines 235 - 238, The tooltip is hiding legitimate zero values because it uses a truthy check ({item.value && ...}); update the condition in the Chart tooltip rendering to explicitly check for presence instead (e.g., item?.value !== undefined && item?.value !== null or the same pattern used at line 195: item?.value !== undefined) so that 0 is rendered; keep the existing span with className "text-foreground font-mono font-medium tabular-nums" and formatting via item.value.toLocaleString().client/components/ui/chart.tsx-82-101 (1)
82-101:⚠️ Potential issue | 🟠 MajorEscape dynamic selector input to prevent CSS injection and selector breakage.
idis interpolated directly into the CSS attribute selector without escaping (line 87:[data-chart=${id}]). Ifidcontains special characters—spaces, quotes, brackets, or other CSS metacharacters—the selector will either break or enable CSS injection.Use
CSS.escape(id)to sanitize the value before injection. This is a standard Web API (CSSOM Level 1) with universal browser support since 2020.Suggested fix
const ChartStyle = ({ id, config }: { id: string; config: ChartConfig }) => { + const escapedId = CSS.escape(id) const colorConfig = Object.entries(config).filter( ([, config]) => config.theme || config.color, ) if (!colorConfig.length) { return null } + const cssText = Object.entries(THEMES) + .map( + ([theme, prefix]) => ` +${prefix} [data-chart="${escapedId}"] { +${colorConfig + .map(([key, itemConfig]) => { + const color = + itemConfig.theme?.[theme as keyof typeof itemConfig.theme] || + itemConfig.color + return color ? ` --color-${key}: ${color};` : null + }) + .join('\n')} +} +`, + ) + .join('\n') + return ( - <style - dangerouslySetInnerHTML={{ - __html: Object.entries(THEMES) - .map( - ([theme, prefix]) => ` -${prefix} [data-chart=${id}] { -${colorConfig - .map(([key, itemConfig]) => { - const color = - itemConfig.theme?.[theme as keyof typeof itemConfig.theme] || - itemConfig.color - return color ? ` --color-${key}: ${color};` : null - }) - .join('\n')} -} -`, - ) - .join('\n'), - }} - /> + <style>{cssText}</style> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/chart.tsx` around lines 82 - 101, The dynamic CSS attribute selector using id inside the style block (in client/components/ui/chart.tsx where the inline style is set via dangerouslySetInnerHTML and the template contains `[data-chart=${id}]`) must escape the id to prevent selector breakage or CSS injection; update the template generation to call CSS.escape(id) (or a safe wrapper) when interpolating the chart id so the selector becomes `[data-chart=${CSS.escape(id)}]`, leaving the rest of the THEMES/colorConfig mapping logic unchanged.client/.gitignore-10-11 (1)
10-11:⚠️ Potential issue | 🟠 MajorExpand env-file ignore coverage to prevent accidental secret commits.
Current pattern only ignores local env files. Sensitive env files like
.envand.env.productioncan still be committed.🔐 Proposed fix
# Environment variables -.env*.local +.env* +!.env.example +!.env*.example🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/.gitignore` around lines 10 - 11, The .gitignore currently only ignores `.env*.local` which still allows committing sensitive files like `.env` and `.env.production`; update the .gitignore to include broader env patterns (e.g., add `.env`, `.env.*`, and optionally `!.env.example`) to prevent accidental secret commits while still allowing example files, replacing or augmenting the existing `.env*.local` entry so all environment files are covered.client/components/ui/progress.tsx-8-26 (1)
8-26:⚠️ Potential issue | 🟠 MajorForward
valuetoProgressPrimitive.Rootand make indicator mathmax-aware.
valueis extracted (Line 10) but never passed toProgressPrimitive.Root(Line 14), so progress semantics/state can drift from the visual indicator. Also, Line 25 assumesmax=100, which breaks when a custommaxis passed.In `@radix-ui/react-progress`, how do `value` and `max` on `Progress.Root` affect ARIA/state, and what is the recommended way to compute indicator fill for non-100 max values?Proposed fix
function Progress({ className, value, + max = 100, ...props }: React.ComponentProps<typeof ProgressPrimitive.Root>) { + const clampedValue = Math.min(max, Math.max(0, value ?? 0)) + const percentage = max > 0 ? (clampedValue / max) * 100 : 0 + return ( <ProgressPrimitive.Root data-slot="progress" + value={clampedValue} + max={max} className={cn( 'bg-primary/20 relative h-2 w-full overflow-hidden rounded-full', className, )} {...props} > <ProgressPrimitive.Indicator data-slot="progress-indicator" className="bg-primary h-full w-full flex-1 transition-all" - style={{ transform: `translateX(-${100 - (value || 0)}%)` }} + style={{ transform: `translateX(-${100 - percentage}%)` }} /> </ProgressPrimitive.Root> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/progress.tsx` around lines 8 - 26, The Progress component extracts value but doesn't forward it to ProgressPrimitive.Root and computes the indicator assuming max=100; update the Progress function to pass the received value (and let other props like max flow through) into ProgressPrimitive.Root and change the Indicator's transform math to compute a percent using max-aware, clamped math (e.g., percent = Math.max(0, Math.min(100, (Number(value) / Number(max || 100)) * 100))) so the ARIA/state (value/max) provided by ProgressPrimitive.Root stays correct and the visual fill matches any custom max; locate these changes in the Progress component and the ProgressPrimitive.Indicator render.client/components/ui/button.tsx-39-56 (1)
39-56:⚠️ Potential issue | 🟠 MajorSet a safe default button type to prevent accidental form submits.
The button element defaults to
type="submit"per HTML spec when the type attribute is not set. This causes unintended form submissions when the Button component is used inside a form without explicitly settingtype. TheInputGroupButtoncomponent in the codebase already works around this by defaulting totype="button", indicating this is a known concern.The fix should extract the
typeprop and provide a safe default:Proposed fix
function Button({ className, variant, size, asChild = false, + type, ...props }: React.ComponentProps<'button'> & VariantProps<typeof buttonVariants> & { asChild?: boolean }) { const Comp = asChild ? Slot : 'button' return ( <Comp data-slot="button" className={cn(buttonVariants({ variant, size, className }))} + type={type ?? 'button'} {...props} /> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/button.tsx` around lines 39 - 56, The Button component currently lets the native button default to type="submit"; modify the Button signature to extract a type prop from the incoming props (e.g., function Button({ type = 'button', ...props }...) or equivalent) and ensure you pass that type down on Comp so the rendered element uses the safe default 'button'; update references in the component that build props spread so that the explicit type value is included while keeping existing behavior of buttonVariants, cn, Slot and other props intact.client/components/ui/navigation-menu.tsx-1-6 (1)
1-6:⚠️ Potential issue | 🟠 MajorMissing
'use client'directive.This component uses Radix UI primitives that require client-side interactivity. Without the
'use client'directive, Next.js App Router will attempt to render this as a Server Component, causing runtime errors.🔧 Proposed fix
+'use client' + import * as React from 'react' import * as NavigationMenuPrimitive from '@radix-ui/react-navigation-menu' import { cva } from 'class-variance-authority' import { ChevronDownIcon } from 'lucide-react'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/navigation-menu.tsx` around lines 1 - 6, Add the missing "use client" directive at the very top of this module (before any imports) so the Radix UI interactive primitives used here (NavigationMenuPrimitive, ChevronDownIcon) and utilities (cva, cn) are executed as a client component; update the file that defines the navigation-menu component to include the directive as the first line to enable client-side interactivity.client/components/ui/empty.tsx-1-5 (1)
1-5:⚠️ Potential issue | 🟠 MajorMissing React import for
React.ComponentProps.The file uses
React.ComponentProps<...>throughout but doesn't import React. This will cause a runtime error.🔧 Proposed fix
import { cva, type VariantProps } from 'class-variance-authority' +import * as React from 'react' import { cn } from '@/lib/utils'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/empty.tsx` around lines 1 - 5, The component uses React.ComponentProps<'div'> in the Empty function signature but doesn't import React, so add an import for React at the top of the file (e.g., import React from 'react') or switch the prop type to a JSX intrinsic type; update the file to import React and keep the existing signature function Empty({ className, ...props }: React.ComponentProps<'div'>) so React.ComponentProps resolves correctly.client/components/ui/pagination.tsx-107-115 (1)
107-115:⚠️ Potential issue | 🟠 Major
aria-hiddencurrently suppresses the screen-reader label.Line 108 hides the entire ellipsis subtree, so Line 114 (
sr-only“More pages”) is not exposed to assistive tech.Suggested fix
<span - aria-hidden data-slot="pagination-ellipsis" className={cn('flex size-9 items-center justify-center', className)} {...props} > - <MoreHorizontalIcon className="size-4" /> + <MoreHorizontalIcon aria-hidden="true" className="size-4" /> <span className="sr-only">More pages</span> </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/pagination.tsx` around lines 107 - 115, The ellipsis wrapper currently has aria-hidden which hides the inner sr-only label; remove aria-hidden from the outer span with data-slot="pagination-ellipsis" so the "More pages" screen-reader text is exposed, and instead mark only the decorative icon as hidden (add aria-hidden={true} or aria-hidden="true" to MoreHorizontalIcon). This preserves the accessible label while keeping the icon decorative; update the span around the icon (MoreHorizontalIcon) and remove aria-hidden from the parent span.client/components/ui/toggle-group.tsx-56-62 (1)
56-62:⚠️ Potential issue | 🟠 MajorPer-item
variant/sizeoverrides are inverted.Lines 56–62 prioritize group context over item props, so item-level overrides don’t take effect. Resolve item props first, then fall back to context.
Suggested fix
data-slot="toggle-group-item" - data-variant={context.variant || variant} - data-size={context.size || size} + data-variant={variant ?? context.variant} + data-size={size ?? context.size} className={cn( toggleVariants({ - variant: context.variant || variant, - size: context.size || size, + variant: variant ?? context.variant, + size: size ?? context.size, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/toggle-group.tsx` around lines 56 - 62, The item component is using context.variant/size before the item props, which prevents per-item overrides; change the resolution order to prefer the item props first (use variant || context.variant and size || context.size or the nullish coalescing equivalent) for the data-variant, data-size attributes and the toggleVariants call so toggleVariants({ variant: variant || context.variant, size: size || context.size }) and className/attributes use the item prop fallback to context.client/components/ui/button-group.tsx-1-2 (1)
1-2:⚠️ Potential issue | 🟠 MajorAdd missing React import for
React.ComponentPropsusage.This file uses
React.ComponentProps<'div'>at lines 28 and 44, andReact.ComponentProps<typeof Separator>at line 64, but does not import React. Withstrict: truein tsconfig, this causes a TypeScript error.Suggested fix
+import * as React from 'react' import { Slot } from '@radix-ui/react-slot' import { cva, type VariantProps } from 'class-variance-authority'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/button-group.tsx` around lines 1 - 2, The file is using React types (e.g., React.ComponentProps<'div'> and React.ComponentProps<typeof Separator>) but never imports React, causing TS errors under strict mode; add an import for React (import React from 'react') at the top of the module so the React namespace is available for those type references (locations: usages of React.ComponentProps in the ButtonGroup component and where Separator prop types are referenced).client/components/ui/carousel.tsx-96-105 (1)
96-105:⚠️ Potential issue | 🟠 MajorUnsubscribe the
reInitlistener in cleanup.Line 99 subscribes
reInit, but cleanup only removesselect(Line 103). This can accumulate handlers across lifecycle churn and duplicate state updates.Suggested fix
React.useEffect(() => { if (!api) return onSelect(api) api.on('reInit', onSelect) api.on('select', onSelect) return () => { - api?.off('select', onSelect) + api.off('reInit', onSelect) + api.off('select', onSelect) } }, [api, onSelect])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/carousel.tsx` around lines 96 - 105, The effect registers two listeners on the carousel API but only removes the 'select' listener on cleanup; update the cleanup in the React.useEffect that uses api and onSelect so it also unsubscribes the 'reInit' listener (i.e., call api.off('reInit', onSelect) alongside api.off('select', onSelect) or remove both via api?.off(...)) to prevent handler accumulation for the api and onSelect handlers.client/next-env.d.ts-3-3 (1)
3-3:⚠️ Potential issue | 🟠 MajorRemove the
.next/devimport fromnext-env.d.ts.The
.next/dev/types/routes.d.tspath is a dev-only generated artifact that is not stable across environments. Sincetsconfig.jsonalready includes.next/types/**/*.tsand referencesnext-env.d.ts, the route types will be available without this direct import. Remove line 3 to eliminate the unstable dependency.Suggested fix
-import "./.next/dev/types/routes.d.ts";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/next-env.d.ts` at line 3, Remove the unstable dev-only import from next-env.d.ts: delete the line importing "./.next/dev/types/routes.d.ts" so the file only relies on the stable types included via tsconfig (e.g., .next/types/**/*.ts) and the next-env.d.ts reference; ensure next-env.d.ts still exists and nothing else in the file is changed.client/components/ui/form.tsx-28-57 (1)
28-57:⚠️ Potential issue | 🟠 MajorMove context validation to the top of
useFormField()before accessing context values.The function reads
fieldContext.nameanditemContext.idbefore validating these contexts exist. Additionally,FormFieldContextandFormItemContextboth declare unsafe empty-object defaults with type assertions, making their validity impossible to check at runtime. Move both validation checks to the beginning of the function, and change both context declarations to use nullable defaults:Suggested fix
-const FormFieldContext = React.createContext<FormFieldContextValue>( - {} as FormFieldContextValue, -) +const FormFieldContext = React.createContext<FormFieldContextValue | null>(null) @@ -const useFormField = () => { +const useFormField = () => { const fieldContext = React.useContext(FormFieldContext) const itemContext = React.useContext(FormItemContext) + if (!fieldContext) { + throw new Error('useFormField should be used within <FormField>') + } + if (!itemContext) { + throw new Error('useFormField should be used within <FormItem>') + } const { getFieldState } = useFormContext() const formState = useFormState({ name: fieldContext.name }) const fieldState = getFieldState(fieldContext.name, formState) - - if (!fieldContext) { - throw new Error('useFormField should be used within <FormField>') - } @@ -const FormItemContext = React.createContext<FormItemContextValue>( - {} as FormItemContextValue, -) +const FormItemContext = React.createContext<FormItemContextValue | null>(null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/form.tsx` around lines 28 - 57, The useFormField function currently reads fieldContext.name and itemContext.id before validating contexts; change FormFieldContext and FormItemContext declarations to use nullable defaults (e.g., createContext<FormFieldContextValue | null>(null)) and update their types accordingly, then move the existence checks for fieldContext and itemContext to the very top of useFormField so you validate both contexts before any property access; update any downstream code in useFormField that assumes non-null values to narrow types after the checks (refer to FormFieldContext, FormItemContext, and useFormField).client/app/globals.css-21-22 (1)
21-22:⚠️ Potential issue | 🟠 MajorDestructive foreground tokens are too close to destructive backgrounds.
In light theme (Line 21–22), foreground equals background, which can make destructive text unreadable. Dark theme values are also very close.
Suggested token correction
- --destructive-foreground: oklch(0.577 0.245 27.325); + --destructive-foreground: oklch(0.985 0 0); ... - --destructive-foreground: oklch(0.637 0.237 25.331); + --destructive-foreground: oklch(0.96 0.01 145);Also applies to: 57-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/globals.css` around lines 21 - 22, The destructive foreground colors currently match (or are too close to) their destructive background tokens (--destructive and --destructive-foreground) for both light and dark themes (also duplicated at the second set of tokens), causing poor contrast; update the --destructive-foreground values so they provide sufficient contrast against --destructive (for light theme use a darker foreground or near-black Oklch value, and for dark theme use a lighter/near-white Oklch value) or replace the foreground with a high-contrast token (e.g., a near-black or near-white Oklch) that meets WCAG contrast ratios, and apply the same change to the corresponding tokens at the second occurrence.client/app/layout.tsx-7-8 (1)
7-8:⚠️ Potential issue | 🟠 MajorWire
next/fontoutputs into the DOM; currently fonts are not applied and Next.js optimization is skipped.The font objects created on Lines 7–8 are instantiated but never used. Without configuring
variableproperties and applying them to the DOM classes, Next.js cannot optimize font loading. The hardcoded font names in globals.css (lines 78–79) will not trigger proper font delivery.Suggested fix
-const _geist = Geist({ subsets: ["latin"] }); -const _geistMono = Geist_Mono({ subsets: ["latin"] }); +const geist = Geist({ subsets: ['latin'], variable: '--font-geist' }) +const geistMono = Geist_Mono({ subsets: ['latin'], variable: '--font-geist-mono' }) ... - <body className="font-sans antialiased"> + <body className={`${geist.variable} ${geistMono.variable} font-sans antialiased`}>Also applies to: 40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/layout.tsx` around lines 7 - 8, The font objects _geist and _geistMono created via Geist(...) and Geist_Mono(...) are never wired into the DOM so Next.js font optimization is skipped; update the Geist and Geist_Mono calls to include a variable option (e.g., variable: '--font-geist' and '--font-geist-mono') and then apply those CSS variables to the root element by adding the generated class/variable to your layout component's html or body className (where layout.tsx renders the root). Also remove or replace the hardcoded font-family names in globals.css (lines using the old font names) so they reference the new CSS variables (e.g., var(--font-geist) / var(--font-geist-mono)) to ensure proper font loading and optimization.client/app/globals.css-4-4 (1)
4-4:⚠️ Potential issue | 🟠 MajorConfigure Stylelint to recognize Tailwind v4 at-rules.
The
stylelint-config-standard-scssconfig enablesscss/at-rule-no-unknown: trueby default, which will flag@custom-variantand other Tailwind v4 directives as unknown at-rules, causing lint failures.Add one of the following to
.stylelintrc.json:Option 1: Use a dedicated Tailwind Stylelint config:
{ "extends": [ "stylelint-config-standard-scss", "@dreamsicle.io/stylelint-config-tailwindcss" ] }Option 2: Manually configure the inherited rule to ignore Tailwind at-rules:
{ "extends": ["stylelint-config-standard-scss"], "rules": { "scss/at-rule-no-unknown": [ true, { "ignoreAtRules": [ "theme", "utility", "source", "custom-variant", "plugin", "apply", "layer", "tailwind", "config" ] } ] } }Also applies to: 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/globals.css` at line 4, The stylelint error is caused by Tailwind v4 at-rules like `@custom-variant` in client/app/globals.css being flagged by scss/at-rule-no-unknown; update .stylelintrc.json to either extend a Tailwind-aware config (e.g., add "@dreamsicle.io/stylelint-config-tailwindcss" to "extends") or keep "stylelint-config-standard-scss" and add a rule override for "scss/at-rule-no-unknown" with an "ignoreAtRules" list that includes "custom-variant" (and other Tailwind directives such as theme, utility, source, plugin, apply, layer, tailwind, config) so `@custom-variant` is no longer reported as unknown.client/next.config.mjs-3-5 (1)
3-5:⚠️ Potential issue | 🟠 MajorRe-enable TypeScript build checks before release.
ignoreBuildErrors: truebypasses TypeScript build errors, conflicting with the"strict": trueconfiguration intsconfig.json. This allows production builds to pass despite unresolved type errors, which can hide runtime defects.Suggested fix
const nextConfig = { typescript: { - ignoreBuildErrors: true, + ignoreBuildErrors: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/next.config.mjs` around lines 3 - 5, The Next.js config currently disables TypeScript build checks via the typescript.ignoreBuildErrors option in next.config.mjs which masks strict:true tsconfig errors; change this to re-enable checks by setting typescript.ignoreBuildErrors to false (or remove the property entirely) so production builds fail on type errors, or conditionally keep it only for local development (e.g., use NODE_ENV check) and ensure the production path uses typescript.ignoreBuildErrors: false to honor strict type checking.firmware/firmware.ino-56-63 (1)
56-63:⚠️ Potential issue | 🟠 MajorUse
digitalWritefor actuator ON/OFF instead of switching pin mode.Line 59–Line 62 and Line 69–Line 73 currently change pin direction to represent state. This can leave outputs floating and produce unpredictable actuator behavior.
⚙️ Suggested change
void setFan(bool on) { fanOn = on; - if (on) { - pinMode(FAN_PIN, OUTPUT);//turn on fan - } else { - pinMode(FAN_PIN, INPUT); //turn off fan - } + digitalWrite(FAN_PIN, on ? HIGH : LOW); logLine(String("FAN -> ") + (on ? "ON" : "OFF")); } void setPump(bool on) { pumpOn = on; - if (on){ - pinMode(PUMP_PIN,OUTPUT); //turn on pump - } - else{ - pinMode(PUMP_PIN,INPUT); //turn off pump - } + digitalWrite(PUMP_PIN, on ? HIGH : LOW); logLine(String("PUMP -> ") + (on ? "ON" : "OFF")); }Also applies to: 66-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/firmware.ino` around lines 56 - 63, The setFan function (and the other similar blocks at lines 66–73) currently toggles the pin direction to represent ON/OFF which can leave the actuator floating; instead, configure FAN_PIN as an OUTPUT once (e.g., in setup/initialization) and in setFan only call digitalWrite(FAN_PIN, HIGH) to turn on and digitalWrite(FAN_PIN, LOW) to turn off, keep updating the fanOn state and log, and remove changing pinMode inside setFan (also ensure initial output state is set after configuring the pin).services/weather-service/handlers.go-115-121 (1)
115-121:⚠️ Potential issue | 🟠 Major
GET /alertsshould not resend the same alert on every poll.This endpoint now emits notifications as a read side effect. Because
generateAlerts()is stable within the hour, a polling dashboard will fan out duplicate notifications for the same alert until the hour changes. Notify only on new/changed alerts, or persist a dedupe key before publishing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/weather-service/handlers.go` around lines 115 - 121, The GET /alerts handler currently publishes every alert returned by generateAlerts() each poll; change it to deduplicate by tracking a persistent or in-memory dedupe key for each alert (e.g., hash of a.Type+a.Severity+a.Message+ExpiresAt) before calling publishNotification so only new or changed alerts are published. Implement a small dedupe store used by the handler (could be an in-memory map with expiration or persistent store) and check against it in the loop that calls publishNotification; insert/update the dedupe key only after successful publish to avoid duplicates on retries. Locate the alert generation/publishing code in the GET /alerts handler and the call to publishNotification to add this dedupe logic.services/iam-service/handlers.go-130-133 (1)
130-133:⚠️ Potential issue | 🟠 MajorEscape user-controlled fields before building HTML notification bodies.
req.FullName/user.FullNameis interpolated straight into HTML here. That allows markup injection into downstream email/UI renderers. Escape dynamic values before formatting the body.Also applies to: 203-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/iam-service/handlers.go` around lines 130 - 133, The HTML notification body is built by interpolating user-controlled values (req.FullName / user.FullName) directly, which allows HTML/JS injection; update the code around h.publishNotification to escape these dynamic fields before formatting the string (e.g., use html.EscapeString or your project’s sanitizer) for req.FullName and req.Role (and similarly in the other occurrence at lines 203-206) so the generated HTML body cannot contain raw markup or scripts. Ensure you escape values prior to concatenation into the subject/body passed to h.publishNotification and any other email/UI renderers.services/analytics-service/handlers.go-403-413 (1)
403-413:⚠️ Potential issue | 🟠 MajorDeduplicate breach notifications before sending them.
Ingestpublishes wheneverlen(decisions) > 0, so a sensor that stays above/below threshold will generate the same alert on every sample. Add state-transition tracking or per-parameter/equipment rate limiting before notifying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/analytics-service/handlers.go` around lines 403 - 413, The handler currently sends notifications for every non-empty decisions slice, causing repeated alerts for the same ongoing breach; add deduplication by tracking last-notified state per equipment/parameter in the handler (e.g., a map like lastAlertState keyed by EquipmentID+Action or parameter) and only include a decision in rows if it represents a state transition or if a configurable cooldown has elapsed; update this map when you send the notification and use this filtered decisions list (the same variable used to build rows and passed to h.publishNotification) so h.publishNotification is only called when there are new/allowed alerts.services/weather-service/handlers.go-43-55 (1)
43-55:⚠️ Potential issue | 🟠 MajorMove JetStream publish off the
GET /alertsrequest path or usePublishAsync().
publishNotificationuses the synchronousPublish()method, which blocks waiting for broker ACK. This makes theGET /alertsendpoint dependent on NATS cluster health and latency. For each request, all alerts are republished synchronously within the loop (lines ~84–91), compounding the delay. Either publish asynchronously usingPublishAsync()and collect futures, or defer notification publishing to a background worker.Additionally,
generateAlerts()uses an hour-based deterministic seed, so the same alerts regenerate every request within that hour—each call republishes identical notifications.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/weather-service/handlers.go` around lines 43 - 55, The publishNotification function currently uses h.js.Publish (synchronous) on the GET /alerts request path which blocks; change it to use h.js.PublishAsync and collect the returned AckFutures (or use an error callback) so the HTTP handler can return immediately, or move publishing out of the request handler entirely by enqueueing messages to a background worker/goroutine that calls Publish/PublishAsync; also update generateAlerts to avoid hour-deterministic reseeding (e.g., use a non-deterministic seed or include per-request unique IDs) or add idempotency checks (track recently sent alert IDs) so identical alerts generated within the hour are not republished each request. Ensure references: update publishNotification, the GET /alerts handler loop that calls publishNotification, and generateAlerts to implement these changes.services/analytics-service/handlers.go-33-46 (1)
33-46:⚠️ Potential issue | 🟠 MajorMove notification publishing to async path.
publishNotificationuses synchronousPublish(line 44), blocking the/ingestendpoint on NATS broker latency and failures. Since alert delivery is optional, notifications should be sent asynchronously—either via a bounded goroutine, worker pool, or separate queue. This is inconsistent with howdispatchHardwareCommandis already handled withgo(async).Additionally, there is no deduplication or rate limiting. If a threshold breach persists, each
/ingestrequest will re-publish the same notification, spamming the broker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/analytics-service/handlers.go` around lines 33 - 46, publishNotification currently blocks on h.js.Publish (in publishNotification) which stalls /ingest; change it to enqueue notifications to an async worker so publish is non-blocking: create a bounded buffered channel (e.g., notificationQueue) owned by Handler and start a fixed worker goroutine pool on Handler initialization that reads from the channel and calls h.js.Publish("notifications.send", msg). Also implement simple dedup/rate-limiting before enqueueing by computing a unique key (e.g., recipient+subject) and checking a short-lived in-memory map with timestamps (protected by a mutex) to skip re-enqueueing identical alerts within a TTL window. Ensure publishNotification returns immediately if h.js is nil or the queue is full (drop with a warning) to avoid blocking request handlers; refer to publishNotification, h.js, and the "notifications.send" subject when locating code to change.services/iam-service/handlers.go-31-44 (1)
31-44:⚠️ Potential issue | 🟠 MajorAvoid synchronous JetStream publishes in auth flows; also fix HTML injection in notification bodies.
nats.JetStreamContext.Publishis synchronous and blocks waiting for broker acknowledgment (lines 130 & 203). This adds NATS round-trip latency to/registerand/loginendpoints even though notification delivery is optional. Move this behind an async publish, background worker, or short timeout to keep auth responsive.Additionally, the notification bodies directly concatenate user-controlled
FullNameandRoleinto HTML without escaping (lines 132 & 205), creating an HTML injection vulnerability. Usehtml.EscapeString()before interpolating user data into the body.Example fix
import "html" h.publishNotification(req.Email, "Welcome to AgriWizard!", "<h2>Welcome, "+html.EscapeString(req.FullName)+"!</h2><p>Your account has been created successfully as <b>"+html.EscapeString(string(req.Role))+"</b>.</p><p>You can now log in and start managing your greenhouse.</p>", )For the latency issue, spawn a goroutine or use a job queue with a short timeout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/iam-service/handlers.go` around lines 31 - 44, publishNotification currently does synchronous h.js.Publish calls (blocking auth flows) and injects raw user input into HTML bodies; modify Handler.publishNotification to perform non-blocking publishes (e.g., spawn a goroutine to call h.js.Publish or enqueue to a background worker with a short timeout) so /register and /login are not blocked, and ensure all user-controlled fields (e.g., FullName, Role) are escaped with html.EscapeString before building the HTML body (use html.EscapeString when constructing subject/body passed into publishNotification); also handle and log publish errors inside the goroutine/worker rather than blocking the request path.services/hardware-service/handlers.go-479-484 (1)
479-484:⚠️ Potential issue | 🟠 MajorDon't hardcode the notification recipient.
Every
LOCKED/DISABLEDalert is routed to the same email address, regardless of which tenant or user owns the equipment. That makes the feature unusable in multi-user deployments and can leak operational data across accounts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/hardware-service/handlers.go` around lines 479 - 484, The notification recipient is hardcoded in the LOCKED/DISABLED branch causing cross-tenant leakage; change the call in the handler where h.publishNotification(...) is invoked to resolve the correct recipient from the equipment context (e.g., use eqStatus.OwnerEmail or lookup via eqStatus.TenantID/EquipmentID with an existing method like h.getTenantContact or a new service call) and fall back to a safe default or skip sending if none found; ensure you pass that resolved email to h.publishNotification and keep the same subject/body generation using eqStatus.EquipmentID and eqStatus.Status.services/hardware-service/main.go-142-143 (1)
142-143:⚠️ Potential issue | 🟠 Major
restoreSubscriptionsdoesn't reattach equipment status listeners.This startup path only restores topics from
hardware.sensors, so previously registered equipment never resubscribe to their.../statustopics after a service restart.handleEquipmentStatuswill stop receiving state changes until those subscriptions are rebuilt as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/hardware-service/main.go` around lines 142 - 143, restoreSubscriptions currently only re-subscribes topics from hardware.sensors and therefore misses reattaching per-equipment status listeners so handleEquipmentStatus won't receive state updates after restart; update restoreSubscriptions (or add a helper it calls) to iterate the stored equipment entries in hardware.sensors (or the same DB result set it uses) and call mqttClient.Subscribe for each equipment status topic (e.g. "<equipmentID>/status" or the actual topic pattern used elsewhere) hooking the existing handleEquipmentStatus callback, ensuring mqttClient and db are passed into restoreSubscriptions if not already so subscriptions are rebuilt on startup.
🧹 Nitpick comments (12)
client/components/ui/use-mobile.tsx (1)
1-19: Consolidate duplicateduseIsMobileimplementation.This hook duplicates
client/hooks/use-mobile.ts. Keeping both copies will drift over time. Prefer one implementation and re-export from the other path if compatibility is needed.♻️ Suggested consolidation (re-export from UI path)
-import * as React from 'react' - -const MOBILE_BREAKPOINT = 768 - -export function useIsMobile() { - const [isMobile, setIsMobile] = React.useState<boolean | undefined>(undefined) - - React.useEffect(() => { - const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`) - const onChange = () => { - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) - } - mql.addEventListener('change', onChange) - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT) - return () => mql.removeEventListener('change', onChange) - }, []) - - return !!isMobile -} +export { useIsMobile } from '@/hooks/use-mobile'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/use-mobile.tsx` around lines 1 - 19, This file duplicates the useIsMobile hook; remove the local implementation and re-export the single source of truth (useIsMobile) from the existing module in client/hooks/use-mobile.ts to avoid drift: replace the function body in this module with a simple re-export like export { useIsMobile } from 'client/hooks/use-mobile' (keeping the same named export), and remove any duplicated constants (MOBILE_BREAKPOINT) from this file so all code uses the hook defined in client/hooks/use-mobile.ts; ensure consumers keep importing the same named symbol useIsMobile so compatibility is preserved.client/package.json (1)
41-41: Removeautoprefixerfrom dependencies — it's unused with Tailwind v4's PostCSS plugin.
autoprefixer(line 41) is not referenced inpostcss.config.mjsand is unnecessary with@tailwindcss/postcss, which handles vendor prefixing internally. It should be removed entirely rather than moved todevDependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/package.json` at line 41, Remove the "autoprefixer" dependency from package.json dependencies (the "autoprefixer" entry currently present) since Tailwind v4's `@tailwindcss/postcss` handles prefixing; delete the entire "autoprefixer" entry rather than moving it to devDependencies and verify postcss.config.mjs continues to reference only `@tailwindcss/postcss` if needed.client/app/page.tsx (1)
1-1: Prefer a Server Component here unless client interactivity is required.This page is static-renderable as written; removing
"use client"will reduce client bundle size.Suggested fix
-"use client"; - import Link from "next/link";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/page.tsx` at line 1, The file currently forces a Client Component via the "use client" directive at the top of client/app/page.tsx; remove that directive to make the page a Server Component (unless you actually need client-only features like useState/useEffect or event handlers). Locate the top of the file where the literal "use client" is declared and delete that line, and if any client-only hooks or browser APIs are used in functions or components in this file, move them into a separate client component (e.g., create a new ClientPage component with "use client") and import it instead.client/app/(auth)/register/page.tsx (1)
187-192: Role value "Agromist" differs from display text "Agronomist".The
SelectItemsends value"Agromist"but displays"Agronomist"to users. This discrepancy could cause confusion for developers. If "Agromist" is the correct backend enum value, consider adding a comment clarifying this, or use a constant/enum to make the mapping explicit.💡 Consider using a constant for role mapping
// In types/api.ts or a constants file export const USER_ROLES = { AGRONOMIST: { value: "Agromist", label: "Agronomist" }, ADMIN: { value: "Admin", label: "Admin" }, } as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/`(auth)/register/page.tsx around lines 187 - 192, The SelectItem for the role uses a mismatched value ("Agromist") vs display text ("Agronomist"); update the SelectItem in page.tsx (the SelectItem element rendering the Agronomist option) so the value and label are consistent—either change the value to "Agronomist" to match the UI or replace the literal with a shared constant/enum (e.g., USER_ROLES.AGRONOMIST) and add a brief comment explaining if the backend expects the spelled variant; ensure any form handling or type references (form field code that reads this SelectItem value) are updated to use the chosen constant/enum.client/components/dashboard/shell.tsx (1)
50-52: Active state won't highlight parent nav items on nested routes.The
NavLinkcomponent uses exact equality (pathname === item.href) which means when navigating to a nested route like/admin/equipment/123, the "Equipment" nav item (/admin/equipment) won't be highlighted as active.Consider using
startsWithfor more robust active state detection:♻️ Proposed fix
function NavLink({ item, onClick }: { item: NavItem; onClick?: () => void }) { const pathname = usePathname(); - const isActive = pathname === item.href; + const isActive = pathname === item.href || + (item.href !== "/" && pathname.startsWith(item.href + "/"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/dashboard/shell.tsx` around lines 50 - 52, NavLink's active detection (in function NavLink using pathname and isActive) uses strict equality which fails for nested routes; update the logic so isActive is true when pathname equals item.href or when pathname is a descendant of item.href (e.g., pathname.startsWith(item.href + '/') or similar) to ensure parent nav items like "/admin/equipment" are highlighted for routes like "/admin/equipment/123"; keep using usePathname and item.href and ensure edge cases (avoid false matches like "/admin" vs "/administer") are handled by adding the trailing slash check.client/components/ui/toast.tsx (1)
73-89: Non-standardtoast-closeattribute.Line 83 uses
toast-close=""which is a non-standard HTML attribute. If this is intentional (e.g., for external tooling or CSS selectors), consider adding a brief comment. Otherwise, it can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/toast.tsx` around lines 73 - 89, The JSX for ToastClose includes a non-standard attribute toast-close="" on the ToastPrimitives.Close element; either remove that attribute or, if it’s intentionally used for styling/tooling, add a brief inline comment next to the ToastClose declaration explaining its purpose; update the ToastClose (forwardRef) usage of ToastPrimitives.Close and its props so no stray non-standard attributes remain unless explicitly documented in a comment for future readers.client/app/(dashboard)/agronomist/page.tsx (1)
27-63: Consider extractingStatsCardto a shared component.This
StatsCardcomponent is duplicated verbatim inclient/app/(dashboard)/admin/page.tsx. Extract it to a shared location like@/components/dashboard/stats-card.tsxto reduce code duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/`(dashboard)/agronomist/page.tsx around lines 27 - 63, Extract the StatsCard component (function StatsCard, including its props typing, variantStyles, and usage of Card, CardHeader, CardTitle, CardContent and Icon) into a shared component file (e.g. create a new component exported as default or named from a new stats-card.tsx under your shared/components or `@/components/dashboard` path), then replace the inline StatsCard definitions in both client/app/(dashboard)/agronomist/page.tsx and client/app/(dashboard)/admin/page.tsx with an import of the shared StatsCard component and update usages to pass the same props (title, value, description, icon, variant) so behavior and styles remain identical.client/app/(dashboard)/admin/page.tsx (2)
18-54: DuplicateStatsCardcomponent.This component is identical to the one in
agronomist/page.tsx. Consider extracting to a shared location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/`(dashboard)/admin/page.tsx around lines 18 - 54, Extract the duplicated StatsCard component into a single shared component and import it where needed: create a new shared component (e.g., StatsCard) that exports the same function signature and props (title, value, description, icon: React.ComponentType<{ className?: string }>, variant) and preserve the variantStyles, JSX structure, and classNames; then replace the local StatsCard definitions in admin/page.tsx and agronomist/page.tsx with an import of the shared StatsCard and remove the duplicated implementations. Ensure the export name matches the imports (named or default) and update both pages to import that symbol so behavior and types remain unchanged.
203-248: DuplicateDashboardSkeletoncomponent.Similar to
StatsCard, this skeleton component is duplicated across dashboard pages. Consider extracting to a shared component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/`(dashboard)/admin/page.tsx around lines 203 - 248, The DashboardSkeleton component is duplicated across dashboard pages (same issue as StatsCard); extract DashboardSkeleton into a single reusable component (e.g., export a DashboardSkeleton React component) and replace the inline/component copies with an import and usage of that shared component; ensure the new component exports the same named function (DashboardSkeleton) or a default export and update all call sites in admin/page.tsx and other dashboard pages to import and reuse it so there is a single source of truth for the skeleton UI.client/components/ui/sidebar.tsx (1)
85-87: Consider adding explicit cookie attributes for clarity.The cookie is set without explicit
SameSiteorSecureattributes. Modern browsers defaultSameSite=Lax, but being explicit improves clarity and ensures consistent behavior.♻️ Proposed cookie improvement
// This sets the cookie to keep the sidebar state. - document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}` + document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}; SameSite=Lax` },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/ui/sidebar.tsx` around lines 85 - 87, The cookie assignment for SIDEBAR_COOKIE_NAME in the function that writes document.cookie should include explicit SameSite and Secure attributes for clarity and consistent behavior: update the string that uses SIDEBAR_COOKIE_NAME, openState, and SIDEBAR_COOKIE_MAX_AGE to append "; SameSite=Lax; Secure" (note HttpOnly cannot be set from client-side JS; ensure Secure is used only when served over HTTPS). Make this change where document.cookie is set so the sidebar state cookie is unambiguous across browsers.client/lib/api/client.ts (1)
77-83: Consider handling JSON parse errors on success responses.If the server returns a 2xx response with invalid JSON,
response.json()will throw an unhandled error. The 204 case is handled, but other success codes with malformed bodies are not.♻️ Proposed defensive handling
// Handle 204 No Content if (response.status === 204) { return {} as T; } - return response.json(); + try { + return await response.json(); + } catch { + throw new ApiError(response.status, "parse_error", "Invalid response from server"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/lib/api/client.ts` around lines 77 - 83, The current response handling returns response.json() directly which will throw on malformed JSON for 2xx responses; update the response parsing (the block using response.status and response.json()) to wrap response.json() in a try/catch, and on failure call response.text() to capture the raw body and rethrow a descriptive error (including response.status and the raw body and the original parse error) so callers get a useful message instead of an unhandled exception.postgres/init/01-create-service-dbs.sql (1)
1-4: Consider making DB creation idempotent for safer re-runs.Current statements fail if databases already exist. A guarded
DOblock is more resilient for repeated local setup.♻️ Suggested change
-CREATE DATABASE iam_db; -CREATE DATABASE hardware_db; -CREATE DATABASE analytics_db; -CREATE DATABASE notification_db; +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_database WHERE datname = 'iam_db') THEN + CREATE DATABASE iam_db; + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_database WHERE datname = 'hardware_db') THEN + CREATE DATABASE hardware_db; + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_database WHERE datname = 'analytics_db') THEN + CREATE DATABASE analytics_db; + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_database WHERE datname = 'notification_db') THEN + CREATE DATABASE notification_db; + END IF; +END $$;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@postgres/init/01-create-service-dbs.sql` around lines 1 - 4, The CREATE DATABASE statements for iam_db, hardware_db, analytics_db, and notification_db are not idempotent and will fail if run again; replace each plain CREATE DATABASE with a guarded check that queries pg_database (e.g., using a DO block in PL/pgSQL that checks EXISTS (SELECT 1 FROM pg_database WHERE datname = 'iam_db') and only issues CREATE DATABASE if missing) and apply the same pattern for hardware_db, analytics_db, and notification_db so re-running the script is safe.
| const char* WIFI_SSID = "Redmi 14C"; | ||
| const char* WIFI_PASSWORD = "Variable557"; | ||
|
|
||
| const char* MQTT_HOST = "0844c36e03374e7682f81036bb673d45.s1.eu.hivemq.cloud"; | ||
| const uint16_t MQTT_PORT = 8883; | ||
| const char* MQTT_USERNAME = "Hasintha"; | ||
| const char* MQTT_PASSWORD = "Agriwizard@999"; |
There was a problem hiding this comment.
Remove committed Wi‑Fi/MQTT credentials from source.
Line 7–Line 13 expose live secrets in plaintext. Rotate these credentials immediately and move them to a non-versioned secrets header (or secure provisioning flow).
🔐 Suggested change
+#include "secrets.h" // keep this file gitignored
-
-const char* WIFI_SSID = "Redmi 14C";
-const char* WIFI_PASSWORD = "Variable557";
+const char* WIFI_SSID = FW_WIFI_SSID;
+const char* WIFI_PASSWORD = FW_WIFI_PASSWORD;
-const char* MQTT_HOST = "0844c36e03374e7682f81036bb673d45.s1.eu.hivemq.cloud";
+const char* MQTT_HOST = FW_MQTT_HOST;
const uint16_t MQTT_PORT = 8883;
-const char* MQTT_USERNAME = "Hasintha";
-const char* MQTT_PASSWORD = "Agriwizard@999";
+const char* MQTT_USERNAME = FW_MQTT_USERNAME;
+const char* MQTT_PASSWORD = FW_MQTT_PASSWORD;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/firmware.ino` around lines 7 - 13, The file firmware.ino currently
contains hardcoded secrets (WIFI_SSID, WIFI_PASSWORD, MQTT_HOST, MQTT_PORT,
MQTT_USERNAME, MQTT_PASSWORD); remove these plaintext values and replace them
with runtime or build-time injection from a non-versioned secrets header (e.g.,
include "secrets.h" added to .gitignore) or from secure provisioning/Flash
storage; also ensure you rotate the exposed Wi‑Fi and MQTT credentials
immediately. Locate the constants WIFI_SSID, WIFI_PASSWORD, MQTT_HOST,
MQTT_PORT, MQTT_USERNAME, and MQTT_PASSWORD and change their assignment to read
from the secrets provider (or define them via compiler defines), update any
initialization code that uses them to handle missing secrets gracefully, and add
secrets.h to .gitignore so credentials are not committed.
| soilSensorTelemetryTopic = "agriwizard/sensor/" + String(SOIL_SENSOR_ID) + "/telemetry"; | ||
| tmpSensorTelemetryTopic = "agriwizard/sensor/" + String(TMP_SENSOR_ID) + "/telemetry"; |
There was a problem hiding this comment.
Fix undeclared topic variables causing compile failure.
Line 253, Line 254, Line 277, and Line 278 reference soilSensorTelemetryTopic and tmpSensorTelemetryTopic, but these identifiers are never declared.
🧩 Suggested change
String commandTopicWildcard = "agriwizard/equipment/+/command";
String sensorTelemetryTopic = "";
+String soilSensorTelemetryTopic = "";
+String tmpSensorTelemetryTopic = "";
String fanCommandTopic = "";
String pumpCommandTopic = "";
String fanStatusTopic = "";Also applies to: 277-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/firmware.ino` around lines 253 - 254, Declare the missing topic
variables so they exist before being assigned and used: add declarations for
soilSensorTelemetryTopic and tmpSensorTelemetryTopic (e.g., "String
soilSensorTelemetryTopic;" and "String tmpSensorTelemetryTopic;") in the
appropriate scope (global or function scope matching where SOIL_SENSOR_ID and
TMP_SENSOR_ID are visible) so the assignments soilSensorTelemetryTopic =
"agriwizard/sensor/" + String(SOIL_SENSOR_ID) + "/telemetry"; and
tmpSensorTelemetryTopic = "agriwizard/sensor/" + String(TMP_SENSOR_ID) +
"/telemetry"; compile, and ensure the same declared variables are used later
where they are referenced (the other occurrences of soilSensorTelemetryTopic and
tmpSensorTelemetryTopic).
| logLine("pump command topic=" + pumpCommandTopic); | ||
| connectWiFi(); | ||
|
|
||
| espClient.setInsecure(); // Skips certificate validation but allows the TLS handshake |
There was a problem hiding this comment.
Do not disable TLS certificate validation.
Line 283 (setInsecure) allows MITM on MQTT TLS. Use a pinned CA/server cert instead.
🛡️ Suggested change
- espClient.setInsecure(); // Skips certificate validation but allows the TLS handshake
+ espClient.setCACert(HIVEMQ_CA_CERT_PEM);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/firmware.ino` at line 283, The code currently calls
espClient.setInsecure(), which disables TLS certificate verification and allows
MITM; remove that call and instead configure certificate pinning by loading the
trusted CA or server certificate into the secure client (use
espClient.setCACert(root_ca_pem) or, if using mutual TLS,
espClient.setCertificate(client_cert_pem) and
espClient.setPrivateKey(client_key_pem)), ensure the CA string variable (e.g.,
root_ca_pem) is defined and contains the PEM for your broker, and keep using the
same espClient instance for MQTT connect so TLS validation is enforced when
mqttClient.connect(...) is called.
| if client := h.mqttClient(); client != nil && client.IsConnected() { | ||
| token := client.Publish(eq.MQTTTopic, 1, false, msgBytes) | ||
| token.Wait() | ||
| if token.Error() != nil { | ||
| log.Printf("[ERROR] DispatchControl: mqtt publish: %v", token.Error()) | ||
| c.JSON(http.StatusInternalServerError, ErrorResponse{Error: "mqtt_error", Message: token.Error().Error()}) | ||
| return | ||
| } | ||
| } else { | ||
| log.Printf("[WARN] DispatchControl: MQTT not connected, command not published via MQTT") | ||
| } |
There was a problem hiding this comment.
Return an error when the command was not published.
If MQTT is down, this branch still falls through to the optimistic DB update and 200 response. That lets the API claim the equipment changed state even though no device ever received the command.
🚨 Suggested fix
if client := h.mqttClient(); client != nil && client.IsConnected() {
token := client.Publish(eq.MQTTTopic, 1, false, msgBytes)
token.Wait()
if token.Error() != nil {
log.Printf("[ERROR] DispatchControl: mqtt publish: %v", token.Error())
c.JSON(http.StatusInternalServerError, ErrorResponse{Error: "mqtt_error", Message: token.Error().Error()})
return
}
} else {
- log.Printf("[WARN] DispatchControl: MQTT not connected, command not published via MQTT")
+ c.JSON(http.StatusServiceUnavailable, ErrorResponse{
+ Error: "mqtt_unavailable",
+ Message: "MQTT is not connected; command was not dispatched",
+ })
+ return
}
// Optimistically update status in DB🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/hardware-service/handlers.go` around lines 223 - 233, The handler's
MQTT branch currently logs a warning when h.mqttClient() is nil or not connected
but continues, letting the DB update and 200 response lie; change the
DispatchControl handler so that if client == nil or !client.IsConnected() you
immediately return an HTTP error (e.g., c.JSON(http.StatusServiceUnavailable,
ErrorResponse{Error:"mqtt_unavailable", Message:"MQTT client not connected"})
and return) instead of falling through, and keep the existing error-return
behavior when token.Error() != nil after Publish(eq.MQTTTopic, 1, false,
msgBytes); ensure both failure paths return before any DB/optimistic state
update.
5 participants
Summary by CodeRabbit
New Features
Documentation