Conversation
- Add Google Fonts (Outfit, Open Sans, JetBrains Mono) to index.html - Replace generic blue palette with maritime CSS custom properties (navy, teal, rust) - Add dark mode color variants - Define btn-primary (rust) and btn-secondary (teal) component classes - Extend Tailwind config with semantic color tokens and font families Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e with bio - Replace cyan/blue/purple SaaS palette with maritime navy/teal/rust - Add Outfit, Open Sans, JetBrains Mono fonts via Google Fonts - Remove all gradients, glassmorphism, and backdrop-blur effects - Rename app from "Local Catch" to "Fish Cost Calculator" throughout - Make Calculator the homepage (/ route), remove landing page - Merge Home.jsx content (how it works, features, community) into About.jsx - Add Ryan Horwath bio section with NAMA photo to About page - Update all components: navbar, footer, login, roadmap, data pages - CSS bundle reduced 42% (52KB to 30KB) from removing gradient utilities Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ontext - Calculator now loads fish data instantly from static import (no API fetch) - Saved calculations persist in IndexedDB via DataContext - Custom yields read/write through DataContext (IndexedDB-backed) - Guest mode enabled: no account needed for full calculator functionality - DataManagement works for all users, sync banner shown for guests - CSV export now generated client-side from local data Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wire syncEngine into DataContext with debounced sync after local writes, sync on mount/online events, and add a colored status dot to the NavBar. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Show manual "Add to Home Screen" instructions on iOS Safari since beforeinstallprompt is not supported. Detects iOS Safari specifically (excludes Chrome/Firefox/Opera on iOS) and checks navigator.standalone. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move Ryan's bio section after Contact (project-first focus) - Add links: pacificcloudseafoods.com, ryan-h.org - Update contact email to ryan@ryan-h.org Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add worktrees/ to .gitignore - Update CLAUDE.md with project documentation - Split AuthContext into separate modules (authContext.js, useAuth.js) - Fix duplicate line in token expiry scheduler Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds backend security hardening (JWT fail-fast, origin allowlist, stricter upload parsing, CSV sanitization), a client offline-first sync layer (IndexedDB localStore, DataProvider, syncEngine), large frontend UI/theme/PWA additions, multiple new components, environment/docs updates, and template env/gitignore changes. Changes
Sequence DiagramsequenceDiagram
actor User
participant App as React App
participant DataProv as DataProvider
participant LocalDB as LocalStore (IndexedDB)
participant Sync as SyncEngine
participant API as Server/API
User->>App: open / interact
App->>DataProv: mount -> load
DataProv->>LocalDB: get saved calcs/yields/species
LocalDB-->>DataProv: return cached data
DataProv->>DataProv: set dataLoaded, isOnline
alt online & token present
DataProv->>Sync: triggerSync(token)
Sync->>LocalDB: getAllPendingSync()
LocalDB-->>Sync: pending items
loop push pending items
Sync->>API: POST /api/save-calc or /api/user-data
API-->>Sync: 200 { id } / error
Sync->>LocalDB: mark*Synced(localId, serverId) or record error
end
loop pull server state
Sync->>API: GET /api/saved-calcs, /api/user-data
API-->>Sync: [server items]
Sync->>LocalDB: mergeSynced*(serverItems)
end
Sync-->>DataProv: sync results
DataProv->>App: update syncStatus/UI
end
User->>App: create calc
App->>DataProv: saveCalc(payload)
DataProv->>LocalDB: add(calc, syncStatus: local)
LocalDB-->>DataProv: return local calc
DataProv->>DataProv: schedule debounced triggerSync()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| body: JSON.stringify({ | ||
| species: yld.species, | ||
| product: yld.product, | ||
| yield_percentage: yld.yield, |
There was a problem hiding this comment.
Bug: The sync engine sends custom yields with the key yield_percentage, but the API expects yield, causing the request to fail validation and preventing sync.
Severity: HIGH
Suggested Fix
In app/src/lib/syncEngine.js at line 88, change the key in the payload from yield_percentage to yield to match the API's expectation. The line should be changed to send yield: yld.yield.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/src/lib/syncEngine.js#L88
Potential issue: When syncing custom yield data, `syncEngine.js` at line 88 sends a
payload with the key `yield_percentage`. However, the receiving API endpoint at
`/api/user-data` expects this data under the key `yield`. This mismatch causes the API's
validation to fail, returning an HTTP 400 error. As a result, no custom yield data can
be successfully uploaded and saved to the server, breaking the sync functionality for
this data type.
Did we get this right? 👍 / 👎 to inform future reviews.
| local.push({ | ||
| species: sy.species, | ||
| product: sy.product, | ||
| yield: sy.yield_percentage, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| mode: calc.mode, | ||
| cost: calc.cost, | ||
| target_weight: calc.target_weight, | ||
| yield_value: calc.yield, |
There was a problem hiding this comment.
Bug: The sync engine sends calculation data using the key yield_value, but the API expects yield, causing the yield value to be silently lost upon saving.
Severity: CRITICAL
Suggested Fix
In syncEngine.js at line 40, change the payload key from yield_value to yield to match the API's expectation. The line should be updated to send yield: calc.yield.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/src/lib/syncEngine.js#L40
Potential issue: When the sync engine pushes a saved calculation to the `/api/save-calc`
endpoint, it sends the payload with the key `yield_value`. However, the API endpoint
destructures this value expecting the key `yield`. This mismatch results in the `yield`
parameter being `undefined` on the server. The calculation is then inserted into the
database with a `NULL` value for the yield field, and the sync is marked as successful.
This causes silent data loss for all synced calculations.
Did we get this right? 👍 / 👎 to inform future reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/public-calcs.js (1)
17-22:⚠️ Potential issue | 🟠 MajorMissing
namecolumn breaks frontend display of custom calculation names.The
namecolumn has been removed from the SELECT query, but the frontend inCalculator.jsx(line 993) renders{calc.name || calc.species}. Withoutnamein the response, this will always fall back tospecies, silently hiding user-defined calculation names in the public history.The
save-calc.jsendpoint still insertsnameinto the database, creating an inconsistency where data is stored but never returned by this endpoint.Proposed fix: restore the `name` column
const result = await query( - `SELECT id, species, product, cost, yield, result, date + `SELECT id, name, species, product, cost, yield, result, date FROM calculations ORDER BY date DESC LIMIT 100` );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/public-calcs.js` around lines 17 - 22, The public calculations SELECT removed the name column so frontend (Calculator.jsx using calc.name) can't show custom names; update the SQL in api/public-calcs.js where the query is built (the const result = await query(...) call) to include the name column in the selected fields (e.g., SELECT id, name, species, product, cost, yield, result, date) so stored names from save-calc.js are returned to the client.server/server.js (1)
122-135:⚠️ Potential issue | 🟡 MinorJWT error handling inconsistency between environments.
The
authenticatemiddleware inserver/server.jsreturns401forTokenExpiredErrorand403for other JWT errors. However,verifyTokeninapi/_lib/auth.js(lines 23-27) catches all JWT errors uniformly and returnsnull, whichrequireAuththen treats as401.This means non-expiration JWT errors (e.g., invalid signature, malformed token) return
403in local dev but401in production. For consistency, either differentiate error types in production to match dev's granularity, or simplify dev to always return401.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/server.js` around lines 122 - 135, The authenticate middleware currently maps TokenExpiredError -> 401 and other JWT errors -> 403, causing inconsistency with verifyToken/requireAuth which treat any JWT failure as 401; change authenticate (function authenticate) to always return 401 for any jwt.verify error (remove the err.name conditional) so its behavior matches verifyToken/requireAuth, or alternatively update verifyToken to rethrow JWT errors with their names and adjust requireAuth to map names to statuses—pick one approach and apply it consistently across authenticate, verifyToken, and requireAuth.
🧹 Nitpick comments (8)
app/src/components/Footer.jsx (1)
111-113: Use a dynamic year to avoid annual churn.The hard-coded year will go stale. Prefer deriving it at render time.
♻️ Proposed fix
- <p className="text-white/40 text-sm text-center md:text-left"> - © 2026 Fish Cost Calculator. Free & open source. - </p> + <p className="text-white/40 text-sm text-center md:text-left"> + © {new Date().getFullYear()} Fish Cost Calculator. Free & open source. + </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Footer.jsx` around lines 111 - 113, The footer currently hard-codes "© 2026" which will become stale; update the Footer component (Footer.jsx) to compute the current year at render time (e.g., const currentYear = new Date().getFullYear()) and use that variable in the JSX instead of the literal "2026" so the displayed year stays correct automatically.app/src/context/useAuth.js (1)
1-4: Consider adding a guard for usage outside AuthProvider.If
useAuth()is called outsideAuthProvider, it will silently returnundefined, leading to confusing runtime errors when destructuring properties.♻️ Optional defensive pattern
import { useContext } from 'react'; import { AuthContext } from './authContext'; -export const useAuth = () => useContext(AuthContext); +export const useAuth = () => { + const context = useContext(AuthContext); + if (context === undefined) { + throw new Error('useAuth must be used within an AuthProvider'); + } + return context; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/context/useAuth.js` around lines 1 - 4, The hook useAuth currently returns useContext(AuthContext) which can be undefined if called outside an AuthProvider; update useAuth to retrieve the context value, check if it's falsy/undefined and throw a clear error (e.g., "useAuth must be used within an AuthProvider") so callers get an immediate, descriptive failure; locate the useAuth function in useAuth.js and add the guard around the value obtained from AuthContext before returning it.app/src/lib/syncEngine.js (1)
127-152:stats.pulledcounts total server items, not newly merged items.Lines 134 and 147 increment
pulledby the total length of server arrays, not by the count of items actually merged. SincemergeSyncedCalcsandmergeSyncedYieldsdeduplicate, many items may already exist locally. This could mislead callers expecting accurate sync metrics.📊 Option: Return merge counts from localStore functions
If accurate metrics are needed, modify
mergeSyncedCalcs/mergeSyncedYieldsto return the count of newly added items:// In localStore.js export async function mergeSyncedCalcs(serverCalcs) { // ... existing logic ... let added = 0; for (const sc of serverCalcs) { if (!localIds.has(String(sc.id))) { local.push({ /* ... */ }); added++; } } await set(SAVED_CALCS_KEY, local); return added; // Return count } // In syncEngine.js const added = await mergeSyncedCalcs(serverCalcs); stats.pulled += added;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/syncEngine.js` around lines 127 - 152, The pull metrics are inflated because stats.pulled is incremented by server array lengths rather than newly added items; update mergeSyncedCalcs and mergeSyncedYields to return the count of items they actually add (e.g., number of new IDs merged) and then use those return values in syncEngine.js to increment stats.pulled (replace stats.pulled += serverCalcs.length / serverYields.length with stats.pulled += await merge... return value). Ensure function signatures (mergeSyncedCalcs, mergeSyncedYields) and their callers are updated to handle the returned integer.app/src/components/About.jsx (2)
394-399: Consider adding a fallback for the external profile image.The profile image is loaded from an external domain (
namanet.org). If this URL becomes unavailable, the image will fail silently. Consider adding anonErrorhandler or using a local fallback.🖼️ Add image error handling
<img src="https://www.namanet.org/wp-content/uploads/inline-images/ryan_jig-Fish-750x750.jpg" alt="Ryan Horwath" className="rounded-xl border border-border w-full md:w-[200px] h-auto object-cover" + onError={(e) => { e.target.style.display = 'none'; }} />Or consider hosting the image locally for reliability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/About.jsx` around lines 394 - 399, In the About.jsx component, add an image error handler to the <img> that swaps the external src to a local fallback when the external URL fails (e.g., create a handleImageError function or inline onError that sets src to a bundled/local asset like '/images/ryan-fallback.jpg'); ensure you guard against an infinite loop by only swapping once (use a boolean flag or component state such as imageErrored) and reference the existing img element's src and alt attributes when implementing the change.
1-18: Remove unusedLinkimport.
Linkis imported fromreact-router-dom(line 2) but never used in the component. The AI summary also flagged this.🧹 Remove unused import
import React from "react"; -import { Link } from "react-router-dom"; import {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/About.jsx` around lines 1 - 18, Remove the unused Link import from the top-level import list so the React import stays but "Link" is no longer imported from "react-router-dom"; edit the import statement that currently includes Link (the one alongside other imports like React) to drop the "Link" symbol to eliminate the unused-import warning.app/src/context/DataContext.jsx (1)
56-74: Token retrieval inconsistency with auth state.
triggerSyncreads the token directly fromlocalStorage(line 57) whiledebouncedSyncchecksuserfromuseAuth()(line 80). This could cause sync attempts when:
- Token exists in localStorage but
userstate is null (during auth loading)- User logged out but localStorage wasn't cleared yet
Consider using the
tokenfromuseAuth()consistently, or accept the token as a parameter.♻️ Consistent token access
+import { useAuth } from './useAuth'; + export function DataProvider({ children }) { - const { user } = useAuth(); + const { user, token } = useAuth(); // ... const triggerSync = useCallback(async () => { - const token = localStorage.getItem('token'); if (!token || !navigator.onLine) return; // ... - }, []); + }, [token]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/context/DataContext.jsx` around lines 56 - 74, triggerSync reads the token directly from localStorage which can be inconsistent with the auth state used by debouncedSync; change triggerSync to use the token (or user) from useAuth() or accept a token parameter and use that consistently (align with debouncedSync) before calling syncAll, and update any call sites accordingly (functions/methods to update: triggerSync, debouncedSync, and any component using useAuth to invoke them); ensure the same token source is used for the early-return check and passed into syncAll so syncStatus updates (setSyncStatus) remain correct.app/src/context/AuthContext.jsx (1)
63-131: Consider consolidating duplicate JWT validation logic.The JWT validation and token clearing logic is duplicated between the
checkSessioneffect (lines 35-55) and this new auto-expiry scheduling effect (lines 76-123). Both effects run whentokenchanges and perform the same structural validation (tokenParts.length !== 3) and decoding.The scheduling logic itself is solid:
- Correct
MAX_TIMEOUTvalue (2147483647ms) to handlesetTimeout's 32-bit signed int limit- Proper cleanup with
cancelledflag andclearTimeoutqueueMicrotaskavoids state updates during render♻️ Optional: Extract shared validation
// Helper to decode and validate JWT payload function decodeJwtPayload(token) { const parts = token.split('.'); if (parts.length !== 3) return null; try { return JSON.parse(atob(parts[1])); } catch { return null; } }Then reuse in both effects to reduce duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/context/AuthContext.jsx` around lines 63 - 131, The JWT parsing and validation is duplicated between the existing checkSession effect and the new auto-expiry scheduling effect; extract a shared helper (e.g., decodeJwtPayload(token)) that returns the parsed payload or null and use it in both places (replace the token.split('.') and try/catch JSON.parse(atob(...)) blocks), then update the scheduling effect to call decodeJwtPayload(token) and proceed only if it returns a payload with exp; keep the existing scheduleExpiry, clearToken, MAX_TIMEOUT, cancelled flag and cleanup logic unchanged.app/src/components/Calculator.jsx (1)
401-412: Clarify time cost calculation model in UX.The time tracking adds labor cost as a flat per-pound amount (line 411:
baseRes += totalTimeCost). However, the label says "Time (minutes/lb)" (line 744), implying the cost scales with weight. This seems correct mathematically (time per pound × labor rate = cost per pound), but the UI could be clearer that this is the total labor cost per pound of output.Consider adding a helper text like "Total labor cost will be added per pound of processed product."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Calculator.jsx` around lines 401 - 412, The UX needs explicit clarification that the computed time-based cost (calculated when showTimeTracking is true from processingSteps and added into baseRes as totalTimeCost) is applied per pound; update the Calculator component to add a short helper text next to the existing "Time (minutes/lb)" label (or its rendering location) stating something like "Total labor cost will be added per pound of processed product" so users understand totalTimeCost represents labor cost per lb and not a flat order-level fee.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/_lib/auth.js`:
- Around line 5-9: Remove the top-level throw that reads JWT_SECRET at import
time and instead read/check the secret lazily inside the authentication flow
(e.g., inside requireAuth and/or verifyUser); if JWT_SECRET is missing, do not
throw—fall back to the Neon Auth path (the existing verifyUser fallback) or
return a clear auth error at request-time so the module imports succeed; update
the code that currently references the module-level JWT_SECRET constant to fetch
process.env.JWT_SECRET within requireAuth/verifyUser and handle the
missing-secret case there.
In `@api/_lib/cors.js`:
- Around line 32-46: Update the CORS allowlist in setCorsHeaders to include the
frontend's custom header by adding "x-stack-access-token" to the
Access-Control-Allow-Headers value; locate the setCorsHeaders function and
append "x-stack-access-token" (case-insensitive header name) to the existing
'Content-Type, Authorization' list so preflight responses expose that header to
the browser.
In `@app/src/App.jsx`:
- Around line 56-72: Replace the decorative status dot (the span that uses
isOnline and syncStatus to set className and title) with an accessible status
element: keep the colored dot for visual users but add an adjacent readable text
node derived from isOnline/syncStatus (e.g., "Offline", "Syncing...", "Synced",
"Changes pending", "Ready") and mark the container with role="status" and
aria-live="polite" so screen readers announce changes; ensure the visual dot
remains non-essential (use aria-hidden="true") and the textual status is
focusable/readable by keyboard/screen-readers if needed. Locate the current span
using isOnline and syncStatus to implement these changes and update its title
usage to the semantic text/ARIA attributes.
In `@app/src/components/DataManagement.jsx`:
- Around line 150-194: The four label/input pairs ("Species Name",
"Product/Conversion", "Yield (%)", "Source/Notes") are not associated; add
unique id attributes to each corresponding input (e.g., id="species",
id="product", id="yield", id="source") and set each label's htmlFor to the
matching id so clicking the label focuses the input and screen readers announce
them (follow the same htmlFor/id pattern used in ContributorProfile); update the
input elements that reference formData.species, formData.product,
formData.yield, and formData.source accordingly.
- Around line 94-108: The CSV export writer in the onClick handler currently
uses raw customYields values (in the rows mapping) which can start with =, +, -,
or @ and trigger spreadsheet formulas; sanitize each cell before quoting by
converting to string, escaping internal quotes, and if the value begins with one
of those danger characters prefix it (for example) with a single-quote character
to neutralize formulas; update the rows/map logic that builds CSV cells (the
mapping that currently does [item.species, item.product, item.yield, item.source
|| ''] and the inner .map((v) => ...)) to perform this sanitization before
joining and creating the Blob/URL.
In `@app/src/components/FeaturesRoadmap.jsx`:
- Around line 119-129: The persisted roadmap initializer in FeaturesRoadmap.jsx
trusts raw localStorage JSON and loses Lucide icon functions (causing
FeatureCard to render undefined); wrap JSON.parse in try/catch and validate the
parsed value is an array of objects, then when merging INITIAL_FEATURES with
parsed entries ensure each feature has required fields (id, title, votes) and
restore a valid icon function by mapping a saved icon identifier to the actual
Lucide icon or falling back to a default icon; apply the same hardening to the
other initializers referenced (lines around where INITIAL_FEATURES is used and
the later persisted merges) so any malformed storage value is ignored and a
safe, validated features array is returned.
- Around line 253-262: Add an explicit accessible label for the request input in
FeaturesRoadmap.jsx: give the input an id (e.g., "feature-request") and add a
corresponding <label htmlFor="feature-request"> either visible or with a
"sr-only" class, ensuring the label text describes the field (e.g., "Feature
request"). Update the JSX around the input that uses newRequest and
setNewRequest (inside the form handled by handleSubmitRequest) to reference that
id so assistive tech receives a proper accessible name.
In `@app/src/components/InstallPrompt.jsx`:
- Around line 4-7: The isIosSafari function fails to detect iPadOS Safari;
update its detection to also treat devices reporting MacIntel as iPadOS when
they have touch support. Specifically, in isIosSafari() include an additional
condition like (navigator.platform === 'MacIntel' && navigator.maxTouchPoints >
1) or check for 'iPad' in navigator.userAgent, combined with the existing WebKit
and non-CriOS/FxiOS/OPiOS checks so iPadOS Safari is treated the same as
iPhone/iPod/iPad Safari for showing the Add-to-Home-Screen instructions.
In `@app/src/components/Login.jsx`:
- Around line 133-139: The error text in Login.jsx uses a low-contrast utility
class ("text-red-400") making the small error message hard to read in light
mode; update the error <p> rendering (the block that checks the "error" variable
and renders the paragraph) to use a higher-contrast color utility such as
"text-red-600" or "text-red-700" (or a token like "text-danger-700" if your
design system provides one), keeping the existing accessibility attributes
(role="alert" aria-live="assertive") and other classes intact so the behavior
and layout don't change.
In `@app/src/components/SubmitRequest.jsx`:
- Around line 91-96: The UI currently sets setSubmitStatus("success") and clears
the form immediately after setting window.location.href to a mailto link (in the
SubmitRequest component), which can silently drop requests; change the flow so
that opening the mail client does not auto-mark success or clear form: instead
set a neutral status like "draft-opened" or "pending", do not call
setFormData({}) or setSubmitStatus("success") until you have explicit
confirmation of delivery (e.g., user clicks a "Confirm sent" button) or until a
reliable fallback succeeds (POST to a server endpoint). Update the submit
handler that builds mailtoSubject/mailtoBody and the code paths that currently
call setSubmitStatus and setFormData so they use the new
"draft-opened"/"pending" status and only clear the form on confirmed success or
successful fallback submission.
- Around line 184-195: The request-type buttons are only visually indicated as
selected; update the button element in SubmitRequest.jsx (the button rendered
for each type using type.id and onClick={() => handleInputChange("type",
type.id)}) to expose selection to assistive tech by adding aria-pressed tied to
the selection state (aria-pressed={isSelected}); ensure the attribute uses the
existing isSelected boolean so screen readers can announce the active toggle.
In `@app/src/components/UploadData.jsx`:
- Around line 106-110: The status banner in UploadData.jsx uses hardcoded
dark-theme color classes for both themes, causing poor contrast on light
backgrounds; update the div that checks status.type (the conditional className
and role/aria-live block) to apply theme-specific Tailwind classes (use dark:...
and base/light variants) so success and error each get a readable bg/text/border
for light and dark modes (e.g., a light-mode background/text/border set and a
dark: prefixed set), keeping the same conditional branching on status.type and
preserving role/aria-live logic.
In `@app/src/context/authContextStore.js`:
- Around line 1-3: This file incorrectly creates a second React context
(AuthContext via createContext) which will disconnect consumers from the real
provider; replace the local creation with a re-export of the single shared
context used by the provider/hook by importing the existing AuthContext from
'./authContext' and exporting it (i.e., remove createContext usage and export
the imported AuthContext) so all modules share the same context instance.
In `@app/src/context/DataContext.jsx`:
- Around line 84-96: The two useEffect blocks can both call triggerSync on mount
causing duplicate syncs; remove the first effect that checks navigator.onLine
and keep a single effect that watches isOnline, user, dataLoaded, and
triggerSync (or merge them into one effect) so only the isOnline-backed effect
triggers sync; update references to use the isOnline state (which is initialized
from navigator.onLine) and ensure only one effect containing triggerSync remains
in DataContext.jsx.
- Around line 76-82: The debounced sync timeout set in debouncedSync can fire
after DataProvider unmounts causing state updates on an unmounted component; add
a cleanup effect in the DataProvider component that clears
syncTimeoutRef.current on unmount (and optionally when dependencies change) to
prevent the delayed callback from running after unmount. Locate debouncedSync
and syncTimeoutRef in DataContext.jsx and add a useEffect that returns a cleanup
function which checks syncTimeoutRef.current and calls
clearTimeout(syncTimeoutRef.current), and then nulls the ref to avoid later
reuse.
In `@app/src/lib/localStore.js`:
- Around line 148-159: The loop that maps serverCalcs into local entries uses
sc.created_at for createdAt/updatedAt but the server returns date; update the
mapping in the block that pushes into local (inside the for (const sc of
serverCalcs) { ... } loop) to use sc.date as the source timestamp (falling back
to new Date().toISOString() if sc.date is missing), i.e. set createdAt and
updatedAt from sc.date (or its ISO string) while still setting serverId=sc.id
and id=crypto.randomUUID() and syncStatus='synced'.
- Around line 167-179: In mergeSyncedYields (the loop over serverYields where
each sy is processed), the code reads sy.yield_percentage which doesn't exist on
the server object; update the field access to use the server's yield property
(or use a safe fallback like sy.yield || sy.yield_percentage) so the merged
object's yield value is populated correctly (refer to the variables
serverYields, sy, and the created local item with key yield).
In `@docs/ENVIRONMENT_VARIABLES.md`:
- Around line 37-38: The docs state that VITE_GEMINI_API_KEY is required and
exposed to the browser while elsewhere saying Gemini keys must be server-only;
reconcile this by updating the ENVIRONMENT_VARIABLES wording to clearly mark
VITE_GEMINI_API_KEY as "server-only (DO NOT expose to client)" and provide the
correct alternative for client usage (e.g., server-proxied requests or a
separate client token), and similarly adjust any repeated references (lines
mentioning VITE_GEMINI_API_KEY and related guidance around lines 52-60) so they
consistently instruct using server-side storage or proxying instead of bundling
the secret into client code; also verify VITE_OCR_ENDPOINT remains clearly
optional and unaffected.
In `@docs/superpowers/specs/2026-03-27-branding-redesign-design.md`:
- Around line 88-90: The documented Primary CTA color (rust `#D4622B`) with white
text fails WCAG AA for normal/small text (~3.8:1); update the spec for the
Primary button (the "Primary" entry and any repeated mentions) to either use a
darker rust token (choose a hex that yields ≥4.5:1 against white) or specify a
darker foreground (e.g., near-black) for small/normal CTA text; mention the
chosen remedy and include the classes referenced (rounded-lg,
active:scale-[0.98]) so implementers know to apply the corrected colors to the
Primary button style.
- Around line 58-60: Replace runtime Google Fonts fetches for Outfit, Open Sans,
and JetBrains Mono by bundling the font files with the app: add the font files
to your static assets, create `@font-face` rules in the global stylesheet (e.g.,
styles.css / global.css used by the app shell) to declare Outfit (600,700), Open
Sans (400,500,600) and JetBrains Mono variants, remove the Google Fonts <link>
tags from the HTML (index.html) and update any CSS font-family references to use
the local families, and ensure the service worker precache list (or asset
manifest) includes the new font files so they are available offline at app shell
load.
---
Outside diff comments:
In `@api/public-calcs.js`:
- Around line 17-22: The public calculations SELECT removed the name column so
frontend (Calculator.jsx using calc.name) can't show custom names; update the
SQL in api/public-calcs.js where the query is built (the const result = await
query(...) call) to include the name column in the selected fields (e.g., SELECT
id, name, species, product, cost, yield, result, date) so stored names from
save-calc.js are returned to the client.
In `@server/server.js`:
- Around line 122-135: The authenticate middleware currently maps
TokenExpiredError -> 401 and other JWT errors -> 403, causing inconsistency with
verifyToken/requireAuth which treat any JWT failure as 401; change authenticate
(function authenticate) to always return 401 for any jwt.verify error (remove
the err.name conditional) so its behavior matches verifyToken/requireAuth, or
alternatively update verifyToken to rethrow JWT errors with their names and
adjust requireAuth to map names to statuses—pick one approach and apply it
consistently across authenticate, verifyToken, and requireAuth.
---
Nitpick comments:
In `@app/src/components/About.jsx`:
- Around line 394-399: In the About.jsx component, add an image error handler to
the <img> that swaps the external src to a local fallback when the external URL
fails (e.g., create a handleImageError function or inline onError that sets src
to a bundled/local asset like '/images/ryan-fallback.jpg'); ensure you guard
against an infinite loop by only swapping once (use a boolean flag or component
state such as imageErrored) and reference the existing img element's src and alt
attributes when implementing the change.
- Around line 1-18: Remove the unused Link import from the top-level import list
so the React import stays but "Link" is no longer imported from
"react-router-dom"; edit the import statement that currently includes Link (the
one alongside other imports like React) to drop the "Link" symbol to eliminate
the unused-import warning.
In `@app/src/components/Calculator.jsx`:
- Around line 401-412: The UX needs explicit clarification that the computed
time-based cost (calculated when showTimeTracking is true from processingSteps
and added into baseRes as totalTimeCost) is applied per pound; update the
Calculator component to add a short helper text next to the existing "Time
(minutes/lb)" label (or its rendering location) stating something like "Total
labor cost will be added per pound of processed product" so users understand
totalTimeCost represents labor cost per lb and not a flat order-level fee.
In `@app/src/components/Footer.jsx`:
- Around line 111-113: The footer currently hard-codes "© 2026" which will
become stale; update the Footer component (Footer.jsx) to compute the current
year at render time (e.g., const currentYear = new Date().getFullYear()) and use
that variable in the JSX instead of the literal "2026" so the displayed year
stays correct automatically.
In `@app/src/context/AuthContext.jsx`:
- Around line 63-131: The JWT parsing and validation is duplicated between the
existing checkSession effect and the new auto-expiry scheduling effect; extract
a shared helper (e.g., decodeJwtPayload(token)) that returns the parsed payload
or null and use it in both places (replace the token.split('.') and try/catch
JSON.parse(atob(...)) blocks), then update the scheduling effect to call
decodeJwtPayload(token) and proceed only if it returns a payload with exp; keep
the existing scheduleExpiry, clearToken, MAX_TIMEOUT, cancelled flag and cleanup
logic unchanged.
In `@app/src/context/DataContext.jsx`:
- Around line 56-74: triggerSync reads the token directly from localStorage
which can be inconsistent with the auth state used by debouncedSync; change
triggerSync to use the token (or user) from useAuth() or accept a token
parameter and use that consistently (align with debouncedSync) before calling
syncAll, and update any call sites accordingly (functions/methods to update:
triggerSync, debouncedSync, and any component using useAuth to invoke them);
ensure the same token source is used for the early-return check and passed into
syncAll so syncStatus updates (setSyncStatus) remain correct.
In `@app/src/context/useAuth.js`:
- Around line 1-4: The hook useAuth currently returns useContext(AuthContext)
which can be undefined if called outside an AuthProvider; update useAuth to
retrieve the context value, check if it's falsy/undefined and throw a clear
error (e.g., "useAuth must be used within an AuthProvider") so callers get an
immediate, descriptive failure; locate the useAuth function in useAuth.js and
add the guard around the value obtained from AuthContext before returning it.
In `@app/src/lib/syncEngine.js`:
- Around line 127-152: The pull metrics are inflated because stats.pulled is
incremented by server array lengths rather than newly added items; update
mergeSyncedCalcs and mergeSyncedYields to return the count of items they
actually add (e.g., number of new IDs merged) and then use those return values
in syncEngine.js to increment stats.pulled (replace stats.pulled +=
serverCalcs.length / serverYields.length with stats.pulled += await merge...
return value). Ensure function signatures (mergeSyncedCalcs, mergeSyncedYields)
and their callers are updated to handle the returned integer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0f33ea8-64a3-4e81-9374-d36f3879224c
⛔ Files ignored due to path filters (5)
app/package-lock.jsonis excluded by!**/package-lock.jsonapp/public/icons/apple-touch-icon.svgis excluded by!**/*.svgapp/public/icons/icon-192.svgis excluded by!**/*.svgapp/public/icons/icon-512-maskable.svgis excluded by!**/*.svgapp/public/icons/icon-512.svgis excluded by!**/*.svg
📒 Files selected for processing (40)
.gitignoreCLAUDE.mdDEPLOYMENT.mdapi/_lib/auth.jsapi/_lib/cors.jsapi/export.jsapi/login.jsapi/public-calcs.jsapp/.env.developmentapp/.env.exampleapp/.env.productionapp/index.htmlapp/package.jsonapp/src/App.jsxapp/src/components/About.jsxapp/src/components/Calculator.jsxapp/src/components/ContributorProfile.jsxapp/src/components/DataManagement.jsxapp/src/components/DataTransparency.jsxapp/src/components/FeaturesRoadmap.jsxapp/src/components/Footer.jsxapp/src/components/InstallPrompt.jsxapp/src/components/Login.jsxapp/src/components/SubmitRequest.jsxapp/src/components/UploadData.jsxapp/src/context/AuthContext.jsxapp/src/context/DataContext.jsxapp/src/context/authContext.jsapp/src/context/authContextStore.jsapp/src/context/useAuth.jsapp/src/index.cssapp/src/lib/localStore.jsapp/src/lib/syncEngine.jsapp/tailwind.config.jsapp/vite.config.jsdocs/API.mddocs/ENVIRONMENT_VARIABLES.mddocs/superpowers/specs/2026-03-27-branding-redesign-design.mdserver/.env.exampleserver/server.js
Comment on lines
+5
to
+9
| const JWT_SECRET = process.env.JWT_SECRET; | ||
|
|
||
| if (!JWT_SECRET) { | ||
| throw new Error('JWT_SECRET is required for JWT authentication'); | ||
| } |
There was a problem hiding this comment.
Don't crash auth-protected routes at import time.
This throw happens before the handler or CORS wrapper for every module that imports requireAuth (for example api/save-calc.js Lines 1-4), so a missing JWT_SECRET becomes a platform-level 500 and verifyUser() never reaches the Neon Auth fallback.
💡 Suggested fix
-const JWT_SECRET = process.env.JWT_SECRET;
-
-if (!JWT_SECRET) {
- throw new Error('JWT_SECRET is required for JWT authentication');
-}
-
export function verifyToken(req) {
+ const jwtSecret = process.env.JWT_SECRET;
const authHeader = req.headers.authorization;
if (!authHeader || !authHeader.startsWith('Bearer ')) {
return null;
}
+
+ if (!jwtSecret) {
+ return null;
+ }
const token = authHeader.split(' ')[1];
try {
- return jwt.verify(token, JWT_SECRET);
+ return jwt.verify(token, jwtSecret);
} catch (err) {
return null;
}
}Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/_lib/auth.js` around lines 5 - 9, Remove the top-level throw that reads
JWT_SECRET at import time and instead read/check the secret lazily inside the
authentication flow (e.g., inside requireAuth and/or verifyUser); if JWT_SECRET
is missing, do not throw—fall back to the Neon Auth path (the existing
verifyUser fallback) or return a clear auth error at request-time so the module
imports succeed; update the code that currently references the module-level
JWT_SECRET constant to fetch process.env.JWT_SECRET within
requireAuth/verifyUser and handle the missing-secret case there.
Comment on lines
+32
to
+46
| export function setCorsHeaders(req, res) { | ||
| const allowedOrigin = resolveOrigin(req); | ||
|
|
||
| if (allowedOrigin) { | ||
| res.setHeader('Access-Control-Allow-Origin', allowedOrigin); | ||
| if (allowCredentials) { | ||
| res.setHeader('Access-Control-Allow-Credentials', 'true'); | ||
| } | ||
| } | ||
|
|
||
| res.setHeader('Vary', 'Origin'); | ||
| res.setHeader('Access-Control-Allow-Methods', 'GET,POST,PUT,DELETE,OPTIONS'); | ||
| res.setHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization'); | ||
|
|
||
| return allowedOrigin; |
There was a problem hiding this comment.
Allow the Stack access-token header in preflight responses.
The frontend now sends x-stack-access-token (see app/src/components/ContributorProfile.jsx Line 33), but this allowlist still only exposes Content-Type and Authorization. Cross-origin OAuth requests will be blocked by the browser before they reach the handler.
💡 Suggested fix
- res.setHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization');
+ res.setHeader(
+ 'Access-Control-Allow-Headers',
+ 'Content-Type, Authorization, x-stack-access-token'
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/_lib/cors.js` around lines 32 - 46, Update the CORS allowlist in
setCorsHeaders to include the frontend's custom header by adding
"x-stack-access-token" to the Access-Control-Allow-Headers value; locate the
setCorsHeaders function and append "x-stack-access-token" (case-insensitive
header name) to the existing 'Content-Type, Authorization' list so preflight
responses expose that header to the browser.
Comment on lines
+56
to
+72
| <div className="flex items-center gap-2"> | ||
| <span | ||
| className={`w-2 h-2 rounded-full ${ | ||
| !isOnline ? 'bg-gray-400' : | ||
| syncStatus === 'syncing' ? 'bg-amber-400 animate-pulse' : | ||
| syncStatus === 'synced' ? 'bg-emerald-400' : | ||
| syncStatus === 'pending' ? 'bg-orange-400' : | ||
| 'bg-gray-400' | ||
| }`} | ||
| title={ | ||
| !isOnline ? 'Offline' : | ||
| syncStatus === 'syncing' ? 'Syncing...' : | ||
| syncStatus === 'synced' ? 'Synced' : | ||
| syncStatus === 'pending' ? 'Changes pending' : | ||
| 'Ready' | ||
| } | ||
| /> |
There was a problem hiding this comment.
Expose sync status as text, not just a colored dot.
Right now the state change is only color plus a title on a non-focusable span. Keyboard, screen-reader, and color-blind users do not get equivalent feedback.
Proposed fix
<span
+ aria-hidden="true"
className={`w-2 h-2 rounded-full ${
!isOnline ? 'bg-gray-400' :
syncStatus === 'syncing' ? 'bg-amber-400 animate-pulse' :
syncStatus === 'synced' ? 'bg-emerald-400' :
syncStatus === 'pending' ? 'bg-orange-400' :
'bg-gray-400'
}`}
title={
!isOnline ? 'Offline' :
syncStatus === 'syncing' ? 'Syncing...' :
syncStatus === 'synced' ? 'Synced' :
syncStatus === 'pending' ? 'Changes pending' :
'Ready'
}
/>
+ <span className="text-xs text-white/70" aria-live="polite">
+ {!isOnline ? 'Offline' :
+ syncStatus === 'syncing' ? 'Syncing…' :
+ syncStatus === 'synced' ? 'Synced' :
+ syncStatus === 'pending' ? 'Pending' :
+ 'Ready'}
+ </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/App.jsx` around lines 56 - 72, Replace the decorative status dot (the
span that uses isOnline and syncStatus to set className and title) with an
accessible status element: keep the colored dot for visual users but add an
adjacent readable text node derived from isOnline/syncStatus (e.g., "Offline",
"Syncing...", "Synced", "Changes pending", "Ready") and mark the container with
role="status" and aria-live="polite" so screen readers announce changes; ensure
the visual dot remains non-essential (use aria-hidden="true") and the textual
status is focusable/readable by keyboard/screen-readers if needed. Locate the
current span using isOnline and syncStatus to implement these changes and update
its title usage to the semantic text/ARIA attributes.
Comment on lines
+94
to
108
| onClick={() => { | ||
| const header = 'species,product,yield,source\n'; | ||
| const rows = customYields.map((item) => | ||
| [item.species, item.product, item.yield, item.source || ''].map((v) => `"${String(v).replace(/"/g, '""')}"`).join(',') | ||
| ).join('\n'); | ||
| const blob = new Blob([header + rows], { type: 'text/csv' }); | ||
| const url = window.URL.createObjectURL(blob); | ||
| const a = document.createElement('a'); | ||
| a.href = url; | ||
| a.download = 'custom-yield-data.csv'; | ||
| document.body.appendChild(a); | ||
| a.click(); | ||
| window.URL.revokeObjectURL(url); | ||
| document.body.removeChild(a); | ||
| }} |
There was a problem hiding this comment.
Sanitize the client-side CSV export too.
The server export now neutralizes spreadsheet formulas, but this path still writes raw species, product, and source values. A custom entry beginning with =, +, -, or @ will still execute when the downloaded CSV is opened in Excel/Sheets.
💡 Suggested fix
+const CSV_FORMULA_PREFIX = /^[=+\-@]/;
+const sanitizeCsvValue = (value) => {
+ const stringValue = value == null ? '' : String(value);
+ const escaped = stringValue.replace(/"/g, '""');
+ return CSV_FORMULA_PREFIX.test(escaped.trimStart()) ? `'${escaped}` : escaped;
+};
+
...
- const rows = customYields.map((item) =>
- [item.species, item.product, item.yield, item.source || ''].map((v) => `"${String(v).replace(/"/g, '""')}"`).join(',')
- ).join('\n');
+ const rows = customYields.map((item) =>
+ [item.species, item.product, item.yield, item.source ?? '']
+ .map((v) => `"${sanitizeCsvValue(v)}"`)
+ .join(',')
+ ).join('\n');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/DataManagement.jsx` around lines 94 - 108, The CSV export
writer in the onClick handler currently uses raw customYields values (in the
rows mapping) which can start with =, +, -, or @ and trigger spreadsheet
formulas; sanitize each cell before quoting by converting to string, escaping
internal quotes, and if the value begins with one of those danger characters
prefix it (for example) with a single-quote character to neutralize formulas;
update the rows/map logic that builds CSV cells (the mapping that currently does
[item.species, item.product, item.yield, item.source || ''] and the inner
.map((v) => ...)) to perform this sanitization before joining and creating the
Blob/URL.
Comment on lines
+150
to
+194
| <label className="block text-sm font-medium text-text-secondary mb-2">Species Name</label> | ||
| <input | ||
| type="text" | ||
| value={formData.species} | ||
| onChange={(e) => setFormData({...formData, species: e.target.value})} | ||
| className="w-full bg-slate-100 dark:bg-slate-700 border border-slate-300 dark:border-slate-600 rounded-lg p-3 text-slate-800 dark:text-white focus:ring-2 focus:ring-cyan-500 outline-none" | ||
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | ||
| placeholder="e.g. Atlantic Salmon" | ||
| required | ||
| /> | ||
| </div> | ||
|
|
||
| <div> | ||
| <label className="block text-sm font-medium text-slate-600 dark:text-gray-300 mb-2">Product/Conversion</label> | ||
| <label className="block text-sm font-medium text-text-secondary mb-2">Product/Conversion</label> | ||
| <input | ||
| type="text" | ||
| value={formData.product} | ||
| onChange={(e) => setFormData({...formData, product: e.target.value})} | ||
| className="w-full bg-slate-100 dark:bg-slate-700 border border-slate-300 dark:border-slate-600 rounded-lg p-3 text-slate-800 dark:text-white focus:ring-2 focus:ring-cyan-500 outline-none" | ||
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | ||
| placeholder="e.g. Skinless Fillet" | ||
| required | ||
| /> | ||
| </div> | ||
|
|
||
| <div> | ||
| <label className="block text-sm font-medium text-slate-600 dark:text-gray-300 mb-2">Yield (%)</label> | ||
| <label className="block text-sm font-medium text-text-secondary mb-2">Yield (%)</label> | ||
| <input | ||
| type="number" | ||
| step="0.1" | ||
| min="0" | ||
| max="100" | ||
| value={formData.yield} | ||
| onChange={(e) => setFormData({...formData, yield: e.target.value})} | ||
| className="w-full bg-slate-100 dark:bg-slate-700 border border-slate-300 dark:border-slate-600 rounded-lg p-3 text-slate-800 dark:text-white focus:ring-2 focus:ring-cyan-500 outline-none" | ||
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | ||
| placeholder="e.g. 45" | ||
| required | ||
| /> | ||
| </div> | ||
|
|
||
| <div> | ||
| <label className="block text-sm font-medium text-slate-600 dark:text-gray-300 mb-2">Source/Notes</label> | ||
| <label className="block text-sm font-medium text-text-secondary mb-2">Source/Notes</label> | ||
| <input | ||
| type="text" | ||
| value={formData.source} | ||
| onChange={(e) => setFormData({...formData, source: e.target.value})} | ||
| className="w-full bg-slate-100 dark:bg-slate-700 border border-slate-300 dark:border-slate-600 rounded-lg p-3 text-slate-800 dark:text-white focus:ring-2 focus:ring-cyan-500 outline-none" | ||
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" |
There was a problem hiding this comment.
Bind these labels to their inputs.
These four labels still aren't associated with their controls, so clicking the label won't focus the field and screen readers won't announce the field name reliably. ContributorProfile already uses the right htmlFor/id pattern in this PR.
💡 Suggested fix
- <label className="block text-sm font-medium text-text-secondary mb-2">Species Name</label>
+ <label htmlFor="species" className="block text-sm font-medium text-text-secondary mb-2">Species Name</label>
<input
+ id="species"
type="text"
value={formData.species}
onChange={(e) => setFormData({...formData, species: e.target.value})}
className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none"
placeholder="e.g. Atlantic Salmon"
required
/>
- <label className="block text-sm font-medium text-text-secondary mb-2">Product/Conversion</label>
+ <label htmlFor="product" className="block text-sm font-medium text-text-secondary mb-2">Product/Conversion</label>
<input
+ id="product"
type="text"
value={formData.product}
onChange={(e) => setFormData({...formData, product: e.target.value})}
className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none"
placeholder="e.g. Skinless Fillet"
required
/>
- <label className="block text-sm font-medium text-text-secondary mb-2">Yield (%)</label>
+ <label htmlFor="yield" className="block text-sm font-medium text-text-secondary mb-2">Yield (%)</label>
<input
+ id="yield"
type="number"
step="0.1"
min="0"
max="100"
value={formData.yield}
onChange={(e) => setFormData({...formData, yield: e.target.value})}
className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none"
placeholder="e.g. 45"
required
/>
- <label className="block text-sm font-medium text-text-secondary mb-2">Source/Notes</label>
+ <label htmlFor="source" className="block text-sm font-medium text-text-secondary mb-2">Source/Notes</label>
<input
+ id="source"
type="text"
value={formData.source}
onChange={(e) => setFormData({...formData, source: e.target.value})}
className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none"
placeholder="e.g. Personal experience"
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <label className="block text-sm font-medium text-text-secondary mb-2">Species Name</label> | |
| <input | |
| type="text" | |
| value={formData.species} | |
| onChange={(e) => setFormData({...formData, species: e.target.value})} | |
| className="w-full bg-slate-100 dark:bg-slate-700 border border-slate-300 dark:border-slate-600 rounded-lg p-3 text-slate-800 dark:text-white focus:ring-2 focus:ring-cyan-500 outline-none" | |
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | |
| placeholder="e.g. Atlantic Salmon" | |
| required | |
| /> | |
| </div> | |
| <div> | |
| <label className="block text-sm font-medium text-slate-600 dark:text-gray-300 mb-2">Product/Conversion</label> | |
| <label className="block text-sm font-medium text-text-secondary mb-2">Product/Conversion</label> | |
| <input | |
| type="text" | |
| value={formData.product} | |
| onChange={(e) => setFormData({...formData, product: e.target.value})} | |
| className="w-full bg-slate-100 dark:bg-slate-700 border border-slate-300 dark:border-slate-600 rounded-lg p-3 text-slate-800 dark:text-white focus:ring-2 focus:ring-cyan-500 outline-none" | |
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | |
| placeholder="e.g. Skinless Fillet" | |
| required | |
| /> | |
| </div> | |
| <div> | |
| <label className="block text-sm font-medium text-slate-600 dark:text-gray-300 mb-2">Yield (%)</label> | |
| <label className="block text-sm font-medium text-text-secondary mb-2">Yield (%)</label> | |
| <input | |
| type="number" | |
| step="0.1" | |
| min="0" | |
| max="100" | |
| value={formData.yield} | |
| onChange={(e) => setFormData({...formData, yield: e.target.value})} | |
| className="w-full bg-slate-100 dark:bg-slate-700 border border-slate-300 dark:border-slate-600 rounded-lg p-3 text-slate-800 dark:text-white focus:ring-2 focus:ring-cyan-500 outline-none" | |
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | |
| placeholder="e.g. 45" | |
| required | |
| /> | |
| </div> | |
| <div> | |
| <label className="block text-sm font-medium text-slate-600 dark:text-gray-300 mb-2">Source/Notes</label> | |
| <label className="block text-sm font-medium text-text-secondary mb-2">Source/Notes</label> | |
| <input | |
| type="text" | |
| value={formData.source} | |
| onChange={(e) => setFormData({...formData, source: e.target.value})} | |
| className="w-full bg-slate-100 dark:bg-slate-700 border border-slate-300 dark:border-slate-600 rounded-lg p-3 text-slate-800 dark:text-white focus:ring-2 focus:ring-cyan-500 outline-none" | |
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | |
| <label htmlFor="species" className="block text-sm font-medium text-text-secondary mb-2">Species Name</label> | |
| <input | |
| id="species" | |
| type="text" | |
| value={formData.species} | |
| onChange={(e) => setFormData({...formData, species: e.target.value})} | |
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | |
| placeholder="e.g. Atlantic Salmon" | |
| required | |
| /> | |
| </div> | |
| <div> | |
| <label htmlFor="product" className="block text-sm font-medium text-text-secondary mb-2">Product/Conversion</label> | |
| <input | |
| id="product" | |
| type="text" | |
| value={formData.product} | |
| onChange={(e) => setFormData({...formData, product: e.target.value})} | |
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | |
| placeholder="e.g. Skinless Fillet" | |
| required | |
| /> | |
| </div> | |
| <div> | |
| <label htmlFor="yield" className="block text-sm font-medium text-text-secondary mb-2">Yield (%)</label> | |
| <input | |
| id="yield" | |
| type="number" | |
| step="0.1" | |
| min="0" | |
| max="100" | |
| value={formData.yield} | |
| onChange={(e) => setFormData({...formData, yield: e.target.value})} | |
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | |
| placeholder="e.g. 45" | |
| required | |
| /> | |
| </div> | |
| <div> | |
| <label htmlFor="source" className="block text-sm font-medium text-text-secondary mb-2">Source/Notes</label> | |
| <input | |
| id="source" | |
| type="text" | |
| value={formData.source} | |
| onChange={(e) => setFormData({...formData, source: e.target.value})} | |
| className="w-full bg-surface border border-border rounded-lg p-3 text-navy dark:text-text-primary focus:ring-2 focus:ring-teal outline-none" | |
| placeholder="e.g. Personal experience" | |
| /> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/DataManagement.jsx` around lines 150 - 194, The four
label/input pairs ("Species Name", "Product/Conversion", "Yield (%)",
"Source/Notes") are not associated; add unique id attributes to each
corresponding input (e.g., id="species", id="product", id="yield", id="source")
and set each label's htmlFor to the matching id so clicking the label focuses
the input and screen readers announce them (follow the same htmlFor/id pattern
used in ContributorProfile); update the input elements that reference
formData.species, formData.product, formData.yield, and formData.source
accordingly.
Comment on lines
+148
to
+159
| for (const sc of serverCalcs) { | ||
| if (!localIds.has(String(sc.id))) { | ||
| local.push({ | ||
| ...sc, | ||
| serverId: sc.id, | ||
| id: crypto.randomUUID(), | ||
| syncStatus: 'synced', | ||
| updatedAt: sc.created_at || new Date().toISOString(), | ||
| createdAt: sc.created_at || new Date().toISOString(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Field name mismatch: created_at vs date.
The server returns date (from the calculations table), not created_at. While the fallback to new Date().toISOString() prevents errors, it loses the original timestamp from the server.
🔧 Proposed fix
if (!localIds.has(String(sc.id))) {
local.push({
...sc,
serverId: sc.id,
id: crypto.randomUUID(),
syncStatus: 'synced',
- updatedAt: sc.created_at || new Date().toISOString(),
- createdAt: sc.created_at || new Date().toISOString(),
+ updatedAt: sc.date || new Date().toISOString(),
+ createdAt: sc.date || new Date().toISOString(),
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const sc of serverCalcs) { | |
| if (!localIds.has(String(sc.id))) { | |
| local.push({ | |
| ...sc, | |
| serverId: sc.id, | |
| id: crypto.randomUUID(), | |
| syncStatus: 'synced', | |
| updatedAt: sc.created_at || new Date().toISOString(), | |
| createdAt: sc.created_at || new Date().toISOString(), | |
| }); | |
| } | |
| } | |
| for (const sc of serverCalcs) { | |
| if (!localIds.has(String(sc.id))) { | |
| local.push({ | |
| ...sc, | |
| serverId: sc.id, | |
| id: crypto.randomUUID(), | |
| syncStatus: 'synced', | |
| updatedAt: sc.date || new Date().toISOString(), | |
| createdAt: sc.date || new Date().toISOString(), | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/localStore.js` around lines 148 - 159, The loop that maps
serverCalcs into local entries uses sc.created_at for createdAt/updatedAt but
the server returns date; update the mapping in the block that pushes into local
(inside the for (const sc of serverCalcs) { ... } loop) to use sc.date as the
source timestamp (falling back to new Date().toISOString() if sc.date is
missing), i.e. set createdAt and updatedAt from sc.date (or its ISO string)
while still setting serverId=sc.id and id=crypto.randomUUID() and
syncStatus='synced'.
Comment on lines
+167
to
+179
| for (const sy of serverYields) { | ||
| if (!localServerIds.has(String(sy.id))) { | ||
| local.push({ | ||
| species: sy.species, | ||
| product: sy.product, | ||
| yield: sy.yield_percentage, | ||
| source: sy.source || 'User Input', | ||
| serverId: sy.id, | ||
| id: crypto.randomUUID(), | ||
| syncStatus: 'synced', | ||
| updatedAt: new Date().toISOString(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Critical: Field name mismatch causes undefined yield values.
mergeSyncedYields reads sy.yield_percentage (line 172), but the server endpoint /api/user-data returns yield (see api/user-data/index.js and scripts/neon-schema.sql). All merged yields will have undefined for the yield value, breaking calculation functionality.
🐛 Proposed fix
local.push({
species: sy.species,
product: sy.product,
- yield: sy.yield_percentage,
+ yield: sy.yield,
source: sy.source || 'User Input',
serverId: sy.id,
id: crypto.randomUUID(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const sy of serverYields) { | |
| if (!localServerIds.has(String(sy.id))) { | |
| local.push({ | |
| species: sy.species, | |
| product: sy.product, | |
| yield: sy.yield_percentage, | |
| source: sy.source || 'User Input', | |
| serverId: sy.id, | |
| id: crypto.randomUUID(), | |
| syncStatus: 'synced', | |
| updatedAt: new Date().toISOString(), | |
| }); | |
| } | |
| for (const sy of serverYields) { | |
| if (!localServerIds.has(String(sy.id))) { | |
| local.push({ | |
| species: sy.species, | |
| product: sy.product, | |
| yield: sy.yield, | |
| source: sy.source || 'User Input', | |
| serverId: sy.id, | |
| id: crypto.randomUUID(), | |
| syncStatus: 'synced', | |
| updatedAt: new Date().toISOString(), | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/localStore.js` around lines 167 - 179, In mergeSyncedYields (the
loop over serverYields where each sy is processed), the code reads
sy.yield_percentage which doesn't exist on the server object; update the field
access to use the server's yield property (or use a safe fallback like sy.yield
|| sy.yield_percentage) so the merged object's yield value is populated
correctly (refer to the variables serverYields, sy, and the created local item
with key yield).
Comment on lines
+37
to
+38
| - `VITE_GEMINI_API_KEY` - **Required**; Gemini API key used for Gemini integration and **bundled into client code (exposed to the browser)** (e.g. `AIza...`) | ||
| - `VITE_OCR_ENDPOINT` - **Optional**; custom OCR backend endpoint URL (default: use the app's built-in OCR flow/config if unset; e.g. `http://localhost:3000/api/ocr`) |
There was a problem hiding this comment.
Gemini env guidance is internally conflicting.
The doc currently says browser-exposed VITE_GEMINI_API_KEY is required, while also saying Gemini keys must be server-only and browser access should never happen. Teams can’t safely implement both as written.
📝 Suggested wording alignment
-- `VITE_GEMINI_API_KEY` - **Required**; Gemini API key used for Gemini integration and **bundled into client code (exposed to the browser)** (e.g. `AIza...`)
+- `VITE_GEMINI_API_KEY` - **Optional (legacy browser-only flows)**; bundled into client code and publicly exposed. Prefer not setting this when server proxying is available.
...
-- `GEMINI_API_KEY` - **MUST be server-side only** (no `VITE_` prefix); Gemini API key for AI features (e.g., spreadsheet parsing). **NEVER expose to client code**. All Gemini API calls must be routed through server endpoints (e.g., `/api/parse-spreadsheet`) - the browser should never have direct access to this key.
+- `GEMINI_API_KEY` - **Required for server-proxied Gemini features** (no `VITE_` prefix). Keep server-side only. Route Gemini calls through server endpoints (e.g., `/api/parse-spreadsheet`).Also applies to: 52-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ENVIRONMENT_VARIABLES.md` around lines 37 - 38, The docs state that
VITE_GEMINI_API_KEY is required and exposed to the browser while elsewhere
saying Gemini keys must be server-only; reconcile this by updating the
ENVIRONMENT_VARIABLES wording to clearly mark VITE_GEMINI_API_KEY as
"server-only (DO NOT expose to client)" and provide the correct alternative for
client usage (e.g., server-proxied requests or a separate client token), and
similarly adjust any repeated references (lines mentioning VITE_GEMINI_API_KEY
and related guidance around lines 52-60) so they consistently instruct using
server-side storage or proxying instead of bundling the secret into client code;
also verify VITE_OCR_ENDPOINT remains clearly optional and unaffected.
Comment on lines
+58
to
+60
| - **Headings:** Outfit (600, 700) via Google Fonts | ||
| - **Body:** Open Sans (400, 500, 600) via Google Fonts | ||
| - **Monospace:** JetBrains Mono for calculator numbers, costs, yields |
There was a problem hiding this comment.
Bundle these fonts instead of requiring Google Fonts at runtime.
This spec currently pushes the implementation toward external font fetches, which cuts against the offline-first PWA goal. If the app shell is meant to work cleanly offline, the brand fonts should be self-hosted or otherwise shipped with the app.
Also applies to: 131-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-03-27-branding-redesign-design.md` around lines
58 - 60, Replace runtime Google Fonts fetches for Outfit, Open Sans, and
JetBrains Mono by bundling the font files with the app: add the font files to
your static assets, create `@font-face` rules in the global stylesheet (e.g.,
styles.css / global.css used by the app shell) to declare Outfit (600,700), Open
Sans (400,500,600) and JetBrains Mono variants, remove the Google Fonts <link>
tags from the HTML (index.html) and update any CSS font-family references to use
the local families, and ensure the service worker precache list (or asset
manifest) includes the new font files so they are available offline at app shell
load.
Comment on lines
+88
to
+90
| ### Buttons | ||
| - **Primary:** Rust fill, white text, `rounded-lg`, `active:scale-[0.98]` | ||
| - **Secondary:** Transparent with teal border, teal text, `rounded-lg` |
There was a problem hiding this comment.
The AA contrast claim is too strong for the documented CTA colors.
White text on #D4622B is only about 3.8:1, so normal-size primary button text misses the 4.5:1 AA threshold. Either darken the rust token or use a darker foreground for smaller CTA text.
Also applies to: 122-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-03-27-branding-redesign-design.md` around lines
88 - 90, The documented Primary CTA color (rust `#D4622B`) with white text fails
WCAG AA for normal/small text (~3.8:1); update the spec for the Primary button
(the "Primary" entry and any repeated mentions) to either use a darker rust
token (choose a hex that yields ≥4.5:1 against white) or specify a darker
foreground (e.g., near-black) for small/normal CTA text; mention the chosen
remedy and include the classes referenced (rounded-lg, active:scale-[0.98]) so
implementers know to apply the corrected colors to the Primary button style.
- AGENTS.md: Repository guidelines for AI-assisted development - PRD.md: Product requirements document v2.0 - SECURITY_NOTICE.md: Environment variable credential cleanup record - issue list.md: Security and accessibility issue tracker (P0-P2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PRD.md`:
- Line 776: The "Next Review: February 2026" entry is stale; update the "Next
Review" line so it points to a future date (e.g., set a monthly or quarterly
cadence like "May 2026" or "Q2 2026") to keep ownership signals actionable —
locate the exact "Next Review" line (the string "Next Review: February 2026") in
PRD.md and replace the date with the chosen future date or a cadence format, and
ensure the format matches surrounding date conventions.
- Line 3: The document uses the old product name "Value Add Cost Calculator" in
multiple places; update every occurrence of the exact string "Value Add Cost
Calculator" to the new product name "Fish Cost Calculator" (e.g., replace
instances in the title and body including the occurrence referenced as line
769). Search the file for the literal phrase "Value Add Cost Calculator" and
replace each match, and then run a quick pass to ensure other variants
(capitalization/punctuation) are also standardized to "Fish Cost Calculator".
- Around line 286-290: The PWA section headed "### 4.6 Mobile App (PWA)"
incorrectly shows "Status: Not Started" despite the PR shipping installable
PWA/offline-first behavior; update that section by changing "Status: Not
Started" to a current state (e.g., "Status: Implemented / Partial") and add a
short summary of the implemented features (installable PWA, offline-first
caching) under the heading, then move any remaining work items into an
"Enhancements / Future work" bullet list below while keeping "Priority: Medium"
as-is so the doc reflects what is shipped and what remains.
- Around line 318-319: Update the Storage section in PRD.md by replacing the
current single-line artifact "**Storage:** LocalStorage (templates, votes), API
calls for user data" with a local-first architecture description: specify
IndexedDB as the primary client-side store (templates, votes, cached user data)
with offline-first reads/writes and a background sync-to-server when
online/authenticated, falling back to API calls for real-time data or initial
bootstrap; also mention conflict resolution strategy and eviction/sync triggers.
Target the exact phrase "**Storage:** LocalStorage (templates, votes), API calls
for user data" when applying the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d466f47-cc74-4dfe-83c4-04b64ff61070
📒 Files selected for processing (4)
AGENTS.mdPRD.mdSECURITY_NOTICE.mdissue list.md
✅ Files skipped from review due to trivial changes (3)
- AGENTS.md
- SECURITY_NOTICE.md
- issue list.md
| @@ -0,0 +1,776 @@ | |||
| # Product Requirements Document | |||
|
|
|||
| ## Value Add Cost Calculator | |||
There was a problem hiding this comment.
Align product name with the shipped rebrand.
Line 3 and Line 769 use “Value Add Cost Calculator”, but the PR objective says the app was renamed to “Fish Cost Calculator.” Please standardize the product name to avoid roadmap/docs drift.
Proposed doc fix
-## Value Add Cost Calculator
+## Fish Cost Calculator
...
-- **v2.0 (Jan 2026):** Rebrand to "Value Add Cost Calculator", add Time Tracking, Economy of Scale, Features Roadmap
+- **v2.0 (Jan 2026):** Rebrand to "Fish Cost Calculator", add Time Tracking, Economy of Scale, Features RoadmapAlso applies to: 769-769
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Use a hyphen to join words.
Context: ... Product Requirements Document ## Value Add Cost Calculator Version: 2.0 ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PRD.md` at line 3, The document uses the old product name "Value Add Cost
Calculator" in multiple places; update every occurrence of the exact string
"Value Add Cost Calculator" to the new product name "Fish Cost Calculator"
(e.g., replace instances in the title and body including the occurrence
referenced as line 769). Search the file for the literal phrase "Value Add Cost
Calculator" and replace each match, and then run a quick pass to ensure other
variants (capitalization/punctuation) are also standardized to "Fish Cost
Calculator".
Comment on lines
+286
to
+290
| ### 4.6 Mobile App (PWA) | ||
|
|
||
| **Priority:** Medium | ||
| **Status:** Not Started | ||
|
|
There was a problem hiding this comment.
PWA status is outdated and contradicts implemented scope.
Line 289 says “Not Started”, but the PR already ships installable PWA/offline-first behavior. Update this section to reflect current implementation and move only remaining items into enhancements.
Proposed doc fix
-**Status:** Not Started
+**Status:** ✅ Implemented (core), enhancements planned🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PRD.md` around lines 286 - 290, The PWA section headed "### 4.6 Mobile App
(PWA)" incorrectly shows "Status: Not Started" despite the PR shipping
installable PWA/offline-first behavior; update that section by changing "Status:
Not Started" to a current state (e.g., "Status: Implemented / Partial") and add
a short summary of the implemented features (installable PWA, offline-first
caching) under the heading, then move any remaining work items into an
"Enhancements / Future work" bullet list below while keeping "Priority: Medium"
as-is so the doc reflects what is shipped and what remains.
Comment on lines
+318
to
+319
| - **Storage:** LocalStorage (templates, votes), API calls for user data | ||
|
|
There was a problem hiding this comment.
Storage architecture section is missing the local-first implementation.
Line 318 currently frames storage as LocalStorage + API, but v2.0 behavior includes IndexedDB (local-first persistence) and sync-to-server when online/authenticated. This should be reflected in the architecture section.
Proposed doc fix
-- **Storage:** LocalStorage (templates, votes), API calls for user data
+- **Storage:** IndexedDB (local-first calculations/custom yields via idb-keyval), LocalStorage (lightweight prefs/votes), background sync to API when online/authenticated📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Storage:** LocalStorage (templates, votes), API calls for user data | |
| - **Storage:** IndexedDB (local-first calculations/custom yields via idb-keyval), LocalStorage (lightweight prefs/votes), background sync to API when online/authenticated |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PRD.md` around lines 318 - 319, Update the Storage section in PRD.md by
replacing the current single-line artifact "**Storage:** LocalStorage
(templates, votes), API calls for user data" with a local-first architecture
description: specify IndexedDB as the primary client-side store (templates,
votes, cached user data) with offline-first reads/writes and a background
sync-to-server when online/authenticated, falling back to API calls for
real-time data or initial bootstrap; also mention conflict resolution strategy
and eviction/sync triggers. Target the exact phrase "**Storage:** LocalStorage
(templates, votes), API calls for user data" when applying the change.
| --- | ||
|
|
||
| **Document Owner:** Ryan Horwath (ryan@pacificcloudseafoods.com) | ||
| **Next Review:** February 2026 |
There was a problem hiding this comment.
Next Review date is already stale.
Line 776 sets February 2026, which is already past. Please move this to a future date (e.g., monthly/quarterly cadence) so ownership signals remain actionable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PRD.md` at line 776, The "Next Review: February 2026" entry is stale; update
the "Next Review" line so it points to a future date (e.g., set a monthly or
quarterly cadence like "May 2026" or "Q2 2026") to keep ownership signals
actionable — locate the exact "Next Review" line (the string "Next Review:
February 2026") in PRD.md and replace the date with the chosen future date or a
cadence format, and ensure the format matches surrounding date conventions.
- Auto-select first From State and To Product when species is chosen - Show modified yield indicator with original value and reset option - Allow saving custom yield defaults per conversion - Make economy of scale tiers inline-editable with add/delete/persist - Add share button with copy link, copy summary, and native share - Parse URL params to pre-fill calculator from shared links - Consolidate auth context from 4 files to 1 (delete dead code) - DRY local store with generic createSyncedStore factory - Surface sync errors with red status dot and error details - Fix DataContext memory leak (clear timeout on unmount) - Fix PWA icon manifest (.png → .svg to match actual files) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
paccloud
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/context/AuthContext.jsx (1)
17-27:⚠️ Potential issue | 🟠 MajorDon't leave OAuth sessions with an unresolved API token.
This branch sets
userand returns without reconcilingtoken. Downstream code gates features onuserbut still sends the JWT from context/localStorage, so OAuth users either hit authenticated actions that always fail or reuse a stale bearer token from an earlier password session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/context/AuthContext.jsx` around lines 17 - 27, The OAuth branch that handles stackClientApp.getUser sets the user and returns without clearing the JWT/token, causing downstream calls to use a stale bearer token; update the branch in the AuthContext (the code handling stackUser, setUser and setLoading) to also clear the stored auth token by calling the context setter (e.g., setToken(null) or setAuthToken('')) and remove any persisted token (e.g., localStorage.removeItem for the token key) so OAuth sessions never leave an unresolved JWT; ensure you update the same branch where stackUser is handled so token and any storage are reconciled before returning.app/src/components/Login.jsx (1)
50-59:⚠️ Potential issue | 🟠 MajorHandle the boolean OAuth failure path here.
signInWithOAuth()already catches and returnsfalseinAuthContext, so thiscatchnever runs. A failed provider login leavesoauthLoadingset and both OAuth buttons disabled until reload.💡 Suggested fix
try { - await signInWithOAuth(provider); + const success = await signInWithOAuth(provider); + if (!success) { + setError(`Failed to sign in with ${provider}`); + setOauthLoading(null); + } // OAuth will redirect, so we don't need to navigate here } catch (e) { setError(`Failed to sign in with ${provider}`); setOauthLoading(null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Login.jsx` around lines 50 - 59, handleOAuthSignIn currently relies on the catch block but signInWithOAuth returns false on failure, so failed OAuth never clears state; update handleOAuthSignIn to capture the return value from signInWithOAuth(provider), and if it is strictly === false setError(`Failed to sign in with ${provider}`) and setOauthLoading(null); keep the existing catch for unexpected exceptions but ensure both the false-return path and exceptions clear oauthLoading and set an appropriate error message so buttons are re-enabled.
♻️ Duplicate comments (4)
app/src/lib/localStore.js (1)
139-145:⚠️ Potential issue | 🔴 CriticalMap pulled yields from
sy.yield, notsy.yield_percentage.The merged local object uses the
yieldfield everywhere else. Readingsy.yield_percentagehere leaves pulled entries with an undefined yield, so synced custom conversions stop working once they come back down from the API.💡 Suggested fix
- yield: sy.yield_percentage, + yield: sy.yield ?? sy.yield_percentage,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/localStore.js` around lines 139 - 145, The merged object for pulled server yields mistakenly reads sy.yield_percentage which leaves the resulting object's yield undefined; update the mapping inside the loop that builds the local array (the block iterating serverYields and pushing into local) to use sy.yield for the yield property so the created object uses the same yield field as elsewhere (refer to variables serverYields, localServerIds, local and the loop variable sy).app/src/components/Login.jsx (1)
133-136:⚠️ Potential issue | 🟠 MajorGive the inline error higher contrast in light mode.
text-red-400is still too faint on the light card for small alert copy.💡 Suggested fix
- {error && ( - <p className="text-red-400 text-sm text-center" role="alert" aria-live="assertive"> - {error} - </p> - )} + {error && ( + <p + className="rounded-lg border border-red-200 bg-red-50 px-3 py-2 text-center text-sm text-red-800 dark:border-red-500/30 dark:bg-red-900/40 dark:text-red-300" + role="alert" + aria-live="assertive" + > + {error} + </p> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Login.jsx` around lines 133 - 136, The inline error text in the Login component is using the low-contrast class "text-red-400"; update the error rendering (the conditional block that renders {error} in Login.jsx) to use a higher-contrast utility such as "text-red-600" or "text-red-700" (and optionally increase emphasis with "font-medium" or "font-semibold") so the small alert copy is readable on light cards while keeping the existing role="alert" and aria-live attributes intact.app/src/components/DataManagement.jsx (2)
94-108:⚠️ Potential issue | 🟠 MajorSanitize CSV cells before quoting them.
These rows still serialize raw
species,product, andsourcevalues. If any of them begin with=,+,-, or@, opening the export in Excel/Sheets will execute them as formulas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/DataManagement.jsx` around lines 94 - 108, The CSV export currently serializes raw values from customYields (in the DataManagement.jsx onClick handler constructing header/rows) which allows values starting with =, +, -, or @ to be interpreted as formulas by spreadsheet apps; add a sanitize step (e.g., a sanitizeCell(value) helper used when building rows) that converts value to string, escapes internal double-quotes as "" as you already do, and if the (trimmed) string begins with one of the characters =, +, -, or @, prefix it with a single quote (') before wrapping it in quotes; update the rows construction to call this sanitizer for item.species, item.product, item.yield, and item.source to prevent CSV injection while preserving existing quoting logic.
150-195:⚠️ Potential issue | 🟡 MinorBind the four labels to their inputs.
These controls still lack matching
id/htmlFor, so label clicks won't focus the field and screen readers lose the field name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/DataManagement.jsx` around lines 150 - 195, The four labels in DataManagement.jsx are not associated with their inputs; add unique id attributes to each input (e.g. "species-input", "product-input", "yield-input", "source-input") and set the corresponding label htmlFor to the same id so clicks and screen readers will focus/announce the correct field; update the input elements that read/write formData (species, product, yield, source) to include these ids and confirm handlers (setFormData) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/context/AuthContext.jsx`:
- Around line 35-54: The JWT payload segment is being decoded with atob()
directly (tokenParts[1]) which fails for base64url-encoded JWTs; add a small
normalize-and-decode step (or reuse a base64url helper) that replaces '-' with
'+', '_' with '/', pads the string with '=' to a length multiple of 4, then call
atob() and JSON.parse on the result; update the logic around tokenParts/token
payload parsing in the checkSession flow (and the expiry scheduling code that
also decodes tokenParts[1]) to use this normalized decode so valid base64url
JWTs are handled correctly and tokens are not incorrectly cleared.
In `@app/src/lib/localStore.js`:
- Around line 118-122: The pull sync currently only appends server records when
local records lack a serverId (uses localIds Set and local.push inside the
serverCalcs loop), so existing local records with matching server IDs are never
updated; modify the merge logic that iterates serverCalcs (and the analogous
yields loop) to locate the corresponding local record by matching String(sc.id)
to either local.serverId or local.id, then apply last-write-wins updates
(compare updatedAt timestamps and copy/merge server fields into the local
object) instead of skipping it; if no local match is found, keep the existing
local.push behavior.
In `@app/src/lib/syncEngine.js`:
- Around line 88-103: The loop is POSTing yields that already have a serverId,
creating duplicate server rows; update the selection logic so only new local
yields are sent to the create endpoint by filtering pending.yields for
syncStatus === 'local' AND no serverId (e.g., change the filter to (y) =>
y.syncStatus === 'local' && !y.serverId) or add an early check inside the loop
to skip items with yld.serverId; keep using markYieldSynced(yld.id, data.id) for
newly created rows and ensure items with an existing serverId are not POSTed (or
routed to the update path instead).
---
Outside diff comments:
In `@app/src/components/Login.jsx`:
- Around line 50-59: handleOAuthSignIn currently relies on the catch block but
signInWithOAuth returns false on failure, so failed OAuth never clears state;
update handleOAuthSignIn to capture the return value from
signInWithOAuth(provider), and if it is strictly === false setError(`Failed to
sign in with ${provider}`) and setOauthLoading(null); keep the existing catch
for unexpected exceptions but ensure both the false-return path and exceptions
clear oauthLoading and set an appropriate error message so buttons are
re-enabled.
In `@app/src/context/AuthContext.jsx`:
- Around line 17-27: The OAuth branch that handles stackClientApp.getUser sets
the user and returns without clearing the JWT/token, causing downstream calls to
use a stale bearer token; update the branch in the AuthContext (the code
handling stackUser, setUser and setLoading) to also clear the stored auth token
by calling the context setter (e.g., setToken(null) or setAuthToken('')) and
remove any persisted token (e.g., localStorage.removeItem for the token key) so
OAuth sessions never leave an unresolved JWT; ensure you update the same branch
where stackUser is handled so token and any storage are reconciled before
returning.
---
Duplicate comments:
In `@app/src/components/DataManagement.jsx`:
- Around line 94-108: The CSV export currently serializes raw values from
customYields (in the DataManagement.jsx onClick handler constructing
header/rows) which allows values starting with =, +, -, or @ to be interpreted
as formulas by spreadsheet apps; add a sanitize step (e.g., a
sanitizeCell(value) helper used when building rows) that converts value to
string, escapes internal double-quotes as "" as you already do, and if the
(trimmed) string begins with one of the characters =, +, -, or @, prefix it with
a single quote (') before wrapping it in quotes; update the rows construction to
call this sanitizer for item.species, item.product, item.yield, and item.source
to prevent CSV injection while preserving existing quoting logic.
- Around line 150-195: The four labels in DataManagement.jsx are not associated
with their inputs; add unique id attributes to each input (e.g. "species-input",
"product-input", "yield-input", "source-input") and set the corresponding label
htmlFor to the same id so clicks and screen readers will focus/announce the
correct field; update the input elements that read/write formData (species,
product, yield, source) to include these ids and confirm handlers (setFormData)
remain unchanged.
In `@app/src/components/Login.jsx`:
- Around line 133-136: The inline error text in the Login component is using the
low-contrast class "text-red-400"; update the error rendering (the conditional
block that renders {error} in Login.jsx) to use a higher-contrast utility such
as "text-red-600" or "text-red-700" (and optionally increase emphasis with
"font-medium" or "font-semibold") so the small alert copy is readable on light
cards while keeping the existing role="alert" and aria-live attributes intact.
In `@app/src/lib/localStore.js`:
- Around line 139-145: The merged object for pulled server yields mistakenly
reads sy.yield_percentage which leaves the resulting object's yield undefined;
update the mapping inside the loop that builds the local array (the block
iterating serverYields and pushing into local) to use sy.yield for the yield
property so the created object uses the same yield field as elsewhere (refer to
variables serverYields, localServerIds, local and the loop variable sy).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 353248f9-adf1-49fb-90b3-2a6364dc1bbe
📒 Files selected for processing (11)
app/src/App.jsxapp/src/components/Calculator.jsxapp/src/components/ContributorProfile.jsxapp/src/components/DataManagement.jsxapp/src/components/Login.jsxapp/src/components/UploadData.jsxapp/src/context/AuthContext.jsxapp/src/context/DataContext.jsxapp/src/lib/localStore.jsapp/src/lib/syncEngine.jsapp/vite.config.js
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/components/ContributorProfile.jsx
- app/vite.config.js
- app/src/context/DataContext.jsx
- app/src/components/UploadData.jsx
- app/src/App.jsx
Comment on lines
+245
to
+257
| // Custom yields from DataContext (replaces API user-data fetch) | ||
| const customData = useMemo(() => { | ||
| const mapped = {}; | ||
| customYields.forEach(item => { | ||
| if (!mapped[item.species]) mapped[item.species] = { conversions: {} }; | ||
| mapped[item.species].conversions[`Custom: ${item.product}`] = { | ||
| yield: parseFloat(item.yield), | ||
| from: 'Custom', | ||
| to: item.product | ||
| }; | ||
| }); | ||
| return mapped; | ||
| }, [customYields]); |
There was a problem hiding this comment.
“Save as my default” never overrides the selected conversion.
The prompt stores product as ${fromState} → ${toState}, but customData later forces every saved yield to { from: 'Custom', to: item.product }. That turns a saved Round → Fillet override into a brand-new Custom → "Round → Fillet" conversion, so the research/default conversion keeps winning.
Also applies to: 790-796
Comment on lines
+35
to
+54
| const tokenParts = token.split('.'); | ||
| if (tokenParts.length !== 3) { | ||
| localStorage.removeItem('token'); | ||
| setToken(null); | ||
| setUser(null); | ||
| } else { | ||
| try { | ||
| const payload = JSON.parse(atob(tokenParts[1])); | ||
| if (payload.exp && payload.exp * 1000 < Date.now()) { | ||
| localStorage.removeItem('token'); | ||
| setToken(null); | ||
| setUser(null); | ||
| } else { | ||
| setUser({ username: payload.username, authProvider: 'password' }); | ||
| } | ||
| } catch { | ||
| localStorage.removeItem('token'); | ||
| setToken(null); | ||
| setUser(null); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Are JWT header and payload segments base64url-encoded, and in browser JavaScript do they need '-'/'_' normalization plus '=' padding before calling atob()?
💡 Result:
Yes, JWT header and payload segments are base64url-encoded per RFC 7519 and RFC 7515. In browser JavaScript, to decode them using atob (which expects standard base64), you must normalize by replacing '-' with '+' and '_' with '/', and add padding '=' to make the length a multiple of 4 before calling atob. Direct atob calls fail due to these differences.
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc7519
- 2: https://datatracker.ietf.org/doc/html/rfc7519
- 3: https://jwt.io/introduction
- 4: https://www.rfc-editor.org/rfc/rfc7515
- 5: https://allenhwkim.medium.com/decode-jwt-on-a-browser-7e6aa6deb937
- 6: https://stackoverflow.com/questions/72866826/decoding-a-jwt-token-on-frontend-with-atob-gives-me-failed-to-execute-atob-on
- 7: https://stackoverflow.com/questions/38552003/how-to-decode-jwt-token-without-using-a-library
- 8: https://devkit.escalixstudio.com/blog/decode-jwt-token-javascript
- 9: https://blog.jesselangford.dev/post/decrypting-jwts-without-a-library
- 10: https://dev.to/tommy_worklab/i-built-a-jwt-decoder-and-lost-half-a-day-to-atob-2cok
🏁 Script executed:
# First, locate and examine AuthContext.jsx
find . -name "AuthContext.jsx" -type fRepository: paccloud/Fish_Cost_Calculator
Length of output: 106
🏁 Script executed:
# Check the specific lines in the file to confirm the issue
if [ -f "app/src/context/AuthContext.jsx" ]; then
echo "=== Lines 35-54 ==="
sed -n '35,54p' "app/src/context/AuthContext.jsx" | cat -n
echo ""
echo "=== Lines 76-123 ==="
sed -n '76,123p' "app/src/context/AuthContext.jsx" | cat -n
fiRepository: paccloud/Fish_Cost_Calculator
Length of output: 2516
🏁 Script executed:
# Search for any base64url or JWT decode helpers in the codebase
rg -i "base64url|atob" --type js --type jsx -B 2 -A 2Repository: paccloud/Fish_Cost_Calculator
Length of output: 100
🏁 Script executed:
# Check if there's a shared utility function for JWT decoding
rg -i "decode.*jwt|jwt.*decode" --type js --type jsx -B 2 -A 2Repository: paccloud/Fish_Cost_Calculator
Length of output: 100
🏁 Script executed:
# Search for any existing JWT or base64 decode functions in the codebase
rg "function|const.*=" app/src --type-list | head -20
rg "atob\|btoa\|decode" app/src -lRepository: paccloud/Fish_Cost_Calculator
Length of output: 462
🏁 Script executed:
# Let's check if there are any utility files
find app/src -name "*util*" -o -name "*helper*" -o -name "*decode*" | head -20Repository: paccloud/Fish_Cost_Calculator
Length of output: 55
🏁 Script executed:
# Get the full AuthContext.jsx to understand the context better
wc -l "app/src/context/AuthContext.jsx"Repository: paccloud/Fish_Cost_Calculator
Length of output: 108
🏁 Script executed:
# Read the full file to see if there's any normalization logic elsewhere
head -250 "app/src/context/AuthContext.jsx"Repository: paccloud/Fish_Cost_Calculator
Length of output: 5698
Normalize JWT payload before decoding with atob().
Both locations use atob(tokenParts[1]) directly on JWT payload segments. Per RFC 7519 and RFC 7515, JWT segments are base64url-encoded, not standard base64. Valid tokens containing -, _, or with missing padding will fail to decode and be silently cleared. Normalize the segment first by replacing - with +, _ with /, and adding = padding before calling atob(), or use a shared base64url decode helper.
Affects: lines 43 (checkSession) and 87 (expiry scheduling)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/context/AuthContext.jsx` around lines 35 - 54, The JWT payload
segment is being decoded with atob() directly (tokenParts[1]) which fails for
base64url-encoded JWTs; add a small normalize-and-decode step (or reuse a
base64url helper) that replaces '-' with '+', '_' with '/', pads the string with
'=' to a length multiple of 4, then call atob() and JSON.parse on the result;
update the logic around tokenParts/token payload parsing in the checkSession
flow (and the expiry scheduling code that also decodes tokenParts[1]) to use
this normalized decode so valid base64url JWTs are handled correctly and tokens
are not incorrectly cleared.
Comment on lines
+118
to
+122
| const localIds = new Set(local.map((c) => c.serverId || c.id)); | ||
|
|
||
| for (const sc of serverCalcs) { | ||
| if (!localIds.has(String(sc.id))) { | ||
| local.push({ |
There was a problem hiding this comment.
Pull sync never refreshes an existing server record.
Both merge helpers only append rows whose serverId is missing locally. If the same calc or yield changes on another device, the matching local record is skipped forever, so pull sync can't implement the advertised last-write-wins behavior.
Also applies to: 137-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/localStore.js` around lines 118 - 122, The pull sync currently
only appends server records when local records lack a serverId (uses localIds
Set and local.push inside the serverCalcs loop), so existing local records with
matching server IDs are never updated; modify the merge logic that iterates
serverCalcs (and the analogous yields loop) to locate the corresponding local
record by matching String(sc.id) to either local.serverId or local.id, then
apply last-write-wins updates (compare updatedAt timestamps and copy/merge
server fields into the local object) instead of skipping it; if no local match
is found, keep the existing local.push behavior.
Comment on lines
+88
to
+103
| for (const yld of pending.yields.filter((y) => y.syncStatus === 'local')) { | ||
| try { | ||
| const res = await fetch(apiUrl('/api/user-data'), { | ||
| method: 'POST', | ||
| headers, | ||
| body: JSON.stringify({ | ||
| species: yld.species, | ||
| product: yld.product, | ||
| yield_percentage: yld.yield, | ||
| source: yld.source || 'User Input', | ||
| }), | ||
| }); | ||
| if (res.ok) { | ||
| const data = await res.json(); | ||
| await markYieldSynced(yld.id, data.id); | ||
| stats.pushed++; |
There was a problem hiding this comment.
Don't POST edits for yields that already have serverId.
Locally edited yields keep their existing serverId, but this loop still sends them to the create endpoint. Each sync creates a second server row, then markYieldSynced() points the local item at the new id and leaves the previous row orphaned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/syncEngine.js` around lines 88 - 103, The loop is POSTing yields
that already have a serverId, creating duplicate server rows; update the
selection logic so only new local yields are sent to the create endpoint by
filtering pending.yields for syncStatus === 'local' AND no serverId (e.g.,
change the filter to (y) => y.syncStatus === 'local' && !y.serverId) or add an
early check inside the loop to skip items with yld.serverId; keep using
markYieldSynced(yld.id, data.id) for newly created rows and ensure items with an
existing serverId are not POSTed (or routed to the update path instead).
White silhouette fish with font-rendered $ glyph as tail, bold FC text in teal negative space. Solid fills for readability at all icon sizes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment on lines
+101
to
+111
| if (dataLoaded && user && navigator.onLine) { | ||
| triggerSync(); | ||
| } | ||
| }, [dataLoaded, user, triggerSync]); | ||
|
|
||
| // Sync when coming back online | ||
| useEffect(() => { | ||
| if (isOnline && user && dataLoaded) { | ||
| triggerSync(); | ||
| } | ||
| }, [isOnline, user, dataLoaded, triggerSync]); |
There was a problem hiding this comment.
Bug: The client sends yield_value when syncing calculations, but the server API expects yield. This results in the yield value being saved as null.
Severity: CRITICAL
Suggested Fix
In app/src/lib/syncEngine.js, change the key in the JSON body from yield_value to yield to match the API endpoint's expectation. The line yield_value: calc.yield should be changed to yield: calc.yield.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/src/context/DataContext.jsx#L100-L111
Potential issue: When syncing a calculation to the server via `syncAll`, the client
sends a JSON body with the key `yield_value`. However, the API endpoint
`/api/save-calc.js` expects the key to be `yield`. This mismatch causes the `yield`
value to be saved as null or undefined in the database for any calculation synced from
the web client. This will lead to data corruption and break yield reporting for affected
calculations.
| res.json({ message: `Imported ${count} records` }); | ||
| } catch (e) { | ||
| res.status(400).json({ error: 'Failed to process file. Ensure it is a valid spreadsheet.' }); | ||
| } finally { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Keep ALLOWED_ORIGINS/CORS docs from pr-11, merge expanded .env.example from main with Gemini API comments from pr-11. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/.env.example (1)
1-31:⚠️ Potential issue | 🔴 CriticalRemove duplicate and incorrect environment variable definitions in lines 8-31.
The
.env.examplefile contains overlapping configuration blocks. Verification confirms the server code expects:
JWT_SECRET(line 1) ✓ALLOWED_ORIGINS(line 2) ✓JWT_EXPIRES_IN_SECONDS(line 6) ✓However, lines 8-31 redefine conflicting or incorrect variables:
JWT_SECRET(line 15): Duplicate definition with different valueCORS_ORIGINS(line 26): Server code usesALLOWED_ORIGINSinsteadJWT_EXPIRES_IN(line 18): Server code usesJWT_EXPIRES_IN_SECONDSinsteadIn
.envfiles, later definitions override earlier ones, causing the wrong values to be loaded. Remove the entire old configuration block (lines 8-31) and retain only the new configuration (lines 1-6). IfPORTandNODE_ENVare still required, move them to the new configuration section at the top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/.env.example` around lines 1 - 31, The .env.example has duplicate/conflicting vars; remove the second block that redefines JWT_SECRET, JWT_EXPIRES_IN and CORS_ORIGINS (the old config in lines 8-31) and keep the canonical top entries (JWT_SECRET, ALLOWED_ORIGINS, JWT_EXPIRES_IN_SECONDS); if PORT and NODE_ENV are required by the server, add them into the kept top section so only one definition exists for each environment variable (avoid CORS_ORIGINS and JWT_EXPIRES_IN since the code uses ALLOWED_ORIGINS and JWT_EXPIRES_IN_SECONDS).
🧹 Nitpick comments (1)
server/.env.example (1)
2-6: Optional: Address dotenv-linter key ordering warnings.The static analysis tool flags that
ALLOWED_ORIGINS,CORS_ALLOW_CREDENTIALS, andJWT_EXPIRES_IN_SECONDSkeys should be ordered beforeJWT_SECRETalphabetically. While this doesn't affect functionality, alphabetical ordering can improve consistency in.envfiles.📝 Optional: Alphabetize environment variables
-JWT_SECRET=change-me-in-development ALLOWED_ORIGINS=http://localhost:5173,http://localhost:3000 -# Optional: set to true only if you use cookies across origins CORS_ALLOW_CREDENTIALS=false -# Optional: JWT lifetime in seconds (defaults to 86400 = 24h) JWT_EXPIRES_IN_SECONDS=86400 +JWT_SECRET=change-me-in-development +# Optional: set to true only if you use cookies across origins +# Optional: JWT lifetime in seconds (defaults to 86400 = 24h)Note: You may prefer logical grouping (CORS together, JWT together) over strict alphabetization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/.env.example` around lines 2 - 6, The dotenv-linter warning is about key ordering in the .env example: move ALLOWED_ORIGINS, CORS_ALLOW_CREDENTIALS, and JWT_EXPIRES_IN_SECONDS so they appear alphabetically before JWT_SECRET; update the .env.example ordering (reorder the lines containing ALLOWED_ORIGINS, CORS_ALLOW_CREDENTIALS, JWT_EXPIRES_IN_SECONDS and JWT_SECRET) to satisfy the linter while keeping logical groupings if you prefer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/.env.example`:
- Around line 1-31: The .env.example has duplicate/conflicting vars; remove the
second block that redefines JWT_SECRET, JWT_EXPIRES_IN and CORS_ORIGINS (the old
config in lines 8-31) and keep the canonical top entries (JWT_SECRET,
ALLOWED_ORIGINS, JWT_EXPIRES_IN_SECONDS); if PORT and NODE_ENV are required by
the server, add them into the kept top section so only one definition exists for
each environment variable (avoid CORS_ORIGINS and JWT_EXPIRES_IN since the code
uses ALLOWED_ORIGINS and JWT_EXPIRES_IN_SECONDS).
---
Nitpick comments:
In `@server/.env.example`:
- Around line 2-6: The dotenv-linter warning is about key ordering in the .env
example: move ALLOWED_ORIGINS, CORS_ALLOW_CREDENTIALS, and
JWT_EXPIRES_IN_SECONDS so they appear alphabetically before JWT_SECRET; update
the .env.example ordering (reorder the lines containing ALLOWED_ORIGINS,
CORS_ALLOW_CREDENTIALS, JWT_EXPIRES_IN_SECONDS and JWT_SECRET) to satisfy the
linter while keeping logical groupings if you prefer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f5f6151-62da-4adc-9203-83a0052dfb30
⛔ Files ignored due to path filters (4)
app/public/icons/apple-touch-icon.svgis excluded by!**/*.svgapp/public/icons/icon-192.svgis excluded by!**/*.svgapp/public/icons/icon-512-maskable.svgis excluded by!**/*.svgapp/public/icons/icon-512.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
server/.env.example
Covers current architecture audit, 5-phase migration plan, cost comparison, risk register, and timeline. See #15 for tracking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/AUTH_MIGRATION_ROADMAP.md (1)
290-296: Recheck secret list for post-migration auth model consistency.Line 290 includes
JWT_SECRET, but the target design replaces custom JWT verification with Better Auth sessions. Keeping this in the final Cloudflare setup can confuse operators and prolong legacy config drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/AUTH_MIGRATION_ROADMAP.md` around lines 290 - 296, The secret list still includes JWT_SECRET alongside the new Better Auth flow; update the commands so operators no longer create JWT_SECRET: remove or comment out the `npx wrangler secret put JWT_SECRET` entry in the secret setup section and ensure only the new session secret `BETTER_AUTH_SECRET` (and any OAuth client secrets like GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, GITHUB_CLIENT_ID, GITHUB_CLIENT_SECRET) are listed via `npx wrangler secret put`; add a short inline note next to `BETTER_AUTH_SECRET` clarifying it replaces the legacy JWT_SECRET to avoid future config drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/AUTH_MIGRATION_ROADMAP.md`:
- Line 14: Several fenced code blocks in AUTH_MIGRATION_ROADMAP.md are unlabeled
(e.g., the blocks around the sections that include "Before: JWT verify → Stack
Auth API call fallback" and other unlabeled triple-backtick sections); update
each unlabeled ``` block (including those at the instances indicated by the
reviewer) to use a language label such as ```text so markdownlint MD040 warnings
stop and readability improves. Search for the unlabeled triple-backtick fences
in the document and replace each opening ``` with ```text, keeping the content
unchanged.
- Around line 272-274: Replace the inline credentialed connection string passed
to the command shown (the npx wrangler hyperdrive create fish-cost-db
--connection-string argument) with a placeholder or environment-variable
indirection; update the example to reference a tokenized placeholder such as a
clearly marked CONNECTION_STRING placeholder or an environment variable like
$CONNECTION_STRING and add a short note instructing users to set that env var
before running the command so no raw credentials appear inline.
---
Nitpick comments:
In `@docs/AUTH_MIGRATION_ROADMAP.md`:
- Around line 290-296: The secret list still includes JWT_SECRET alongside the
new Better Auth flow; update the commands so operators no longer create
JWT_SECRET: remove or comment out the `npx wrangler secret put JWT_SECRET` entry
in the secret setup section and ensure only the new session secret
`BETTER_AUTH_SECRET` (and any OAuth client secrets like GOOGLE_CLIENT_ID,
GOOGLE_CLIENT_SECRET, GITHUB_CLIENT_ID, GITHUB_CLIENT_SECRET) are listed via
`npx wrangler secret put`; add a short inline note next to `BETTER_AUTH_SECRET`
clarifying it replaces the legacy JWT_SECRET to avoid future config drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5675f051-c965-4ab1-897f-da5369a5a70b
📒 Files selected for processing (1)
docs/AUTH_MIGRATION_ROADMAP.md
|
|
||
| ## Current Architecture | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add languages to fenced code blocks to satisfy lint and improve readability.
Line 14, Line 53, and Line 158 use unlabeled fenced blocks; markdownlint MD040 will keep warning on these.
Suggested diff
-```
+```text
...
-```
+```text
-```
+```text
...
-```
+```text
-```
+```text
Before: JWT verify → Stack Auth API call fallback
After: Better Auth session verify (local, no external calls)
-```
+```textAlso applies to: 53-53, 158-158
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/AUTH_MIGRATION_ROADMAP.md` at line 14, Several fenced code blocks in
AUTH_MIGRATION_ROADMAP.md are unlabeled (e.g., the blocks around the sections
that include "Before: JWT verify → Stack Auth API call fallback" and other
unlabeled triple-backtick sections); update each unlabeled ``` block (including
those at the instances indicated by the reviewer) to use a language label such
as ```text so markdownlint MD040 warnings stop and readability improves. Search
for the unlabeled triple-backtick fences in the document and replace each
opening ``` with ```text, keeping the content unchanged.
Comment on lines
+272
to
+274
| npx wrangler hyperdrive create fish-cost-db \ | ||
| --connection-string="postgresql://agent:...@ep-xxx.us-east-2.aws.neon.tech/neondb" | ||
| ``` |
There was a problem hiding this comment.
Avoid inline DB credentials in command examples.
Line 273 embeds a credential-shaped connection string in a shell command, which is easy to leak via shell history or screenshots. Prefer env var indirection or a placeholder tokenized example.
Suggested diff
-npx wrangler hyperdrive create fish-cost-db \
- --connection-string="postgresql://agent:...@ep-xxx.us-east-2.aws.neon.tech/neondb"
+export NEON_DATABASE_URL="postgresql://<user>:<password>@<host>/<db>"
+npx wrangler hyperdrive create fish-cost-db \
+ --connection-string="$NEON_DATABASE_URL"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| npx wrangler hyperdrive create fish-cost-db \ | |
| --connection-string="postgresql://agent:...@ep-xxx.us-east-2.aws.neon.tech/neondb" | |
| ``` | |
| export NEON_DATABASE_URL="postgresql://<user>:<password>@<host>/<db>" | |
| npx wrangler hyperdrive create fish-cost-db \ | |
| --connection-string="$NEON_DATABASE_URL" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/AUTH_MIGRATION_ROADMAP.md` around lines 272 - 274, Replace the inline
credentialed connection string passed to the command shown (the npx wrangler
hyperdrive create fish-cost-db --connection-string argument) with a placeholder
or environment-variable indirection; update the example to reference a tokenized
placeholder such as a clearly marked CONNECTION_STRING placeholder or an
environment variable like $CONNECTION_STRING and add a short note instructing
users to set that env var before running the command so no raw credentials
appear inline.
Comment on lines
+101
to
+111
| if (dataLoaded && user && navigator.onLine) { | ||
| triggerSync(); | ||
| } | ||
| }, [dataLoaded, user, triggerSync]); | ||
|
|
||
| // Sync when coming back online | ||
| useEffect(() => { | ||
| if (isOnline && user && dataLoaded) { | ||
| triggerSync(); | ||
| } | ||
| }, [isOnline, user, dataLoaded, triggerSync]); |
There was a problem hiding this comment.
Bug: Two useEffect hooks can call triggerSync() concurrently on component mount, leading to a race condition where two sync operations run simultaneously, risking data loss.
Severity: MEDIUM
Suggested Fix
Introduce a locking mechanism to prevent concurrent executions of triggerSync(). A simple approach is to use a state variable or a ref (e.g., isSyncing) to track if a sync is already in progress. Check this lock before calling triggerSync() and set it to true at the beginning of the function and false at the end, within a finally block to ensure it's always reset.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/src/context/DataContext.jsx#L99-L111
Potential issue: A race condition exists within the `DataContext` component. On initial
load, state updates to `dataLoaded` and `user` can cause two separate `useEffect` hooks
to trigger `triggerSync()` concurrently. This results in two simultaneous sync
operations being dispatched. The consequences include redundant network requests,
incorrect UI state updates (e.g., `stats.pulled` being counted twice), and a risk of
data loss. The concurrent calls to `mergeSyncedCalcs()` and `mergeSyncedYields()` can
cause interleaved writes to IndexedDB, where changes from one sync operation are
overwritten by the other before being fully processed.
Required by repo ruleset to unblock PR merges. Runs on pushes to main and all PRs targeting main. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const uploadSingle = upload.single('file'); | ||
| const cleanupUpload = (filePath) => { | ||
| if (filePath) { | ||
| fs.unlink(filePath, (err) => { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
General fix: Before passing a path derived from user input (here, req.file.path) to fs.unlink, normalize and validate it to ensure it lies within a designated safe directory (e.g., the uploads folder). Use path.resolve to normalize the path, compare it to the canonical upload directory, and only perform deletion if the path is within that directory.
Concrete approach for this file:
- Define a constant
UPLOAD_DIRrepresenting the absolute path of the upload directory, derived frompath.resolve('uploads'). - In
cleanupUpload, instead of callingfs.unlink(filePath, ...)directly, compute a normalized absolute path:const resolvedPath = path.resolve(filePath);- Check that
resolvedPathstarts withUPLOAD_DIR + path.sepor equalsUPLOAD_DIR. This prevents directory traversal or pointing outside the upload directory.
- Only call
fs.unlinkif the check passes; otherwise, log a warning and skip deletion. - Keep existing behavior (asynchronous deletion, best-effort cleanup) and signatures unchanged, so other code calling
cleanupUploadstill works.
Changes needed in server/server.js:
- Add
const UPLOAD_DIR = path.resolve('uploads');near the multer configuration (afterdest: 'uploads/'or close to the top wherepathis imported) so it’s clear what directory is allowed. - Update the body of
cleanupUploadto:- Guard on
filePath. - Resolve
filePathto an absolute path. - Check it lies under
UPLOAD_DIR. - Only then call
fs.unlink.
- Guard on
No new methods or external libraries are required; existing fs and path imports are sufficient.
Suggested changeset
1
server/server.js
| @@ -201,6 +201,8 @@ | ||
| 'application/csv' | ||
| ]; | ||
|
|
||
| const UPLOAD_DIR = path.resolve('uploads'); | ||
|
|
||
| const upload = multer({ | ||
| dest: 'uploads/', | ||
| limits: { fileSize: MAX_UPLOAD_SIZE_BYTES }, | ||
| @@ -218,11 +220,17 @@ | ||
| const uploadSingle = upload.single('file'); | ||
| const cleanupUpload = (filePath) => { | ||
| if (filePath) { | ||
| fs.unlink(filePath, (err) => { | ||
| if (err) { | ||
| console.error('Failed to cleanup upload', err); | ||
| } | ||
| }); | ||
| const resolvedPath = path.resolve(filePath); | ||
| // Ensure we only delete files within the uploads directory | ||
| if (resolvedPath === UPLOAD_DIR || resolvedPath.startsWith(UPLOAD_DIR + path.sep)) { | ||
| fs.unlink(resolvedPath, (err) => { | ||
| if (err) { | ||
| console.error('Failed to cleanup upload', err); | ||
| } | ||
| }); | ||
| } else { | ||
| console.warn('Skipping cleanup for file outside upload directory:', resolvedPath); | ||
| } | ||
| } | ||
| }; | ||
|
|
Copilot is powered by AI and may make mistakes. Always verify output.
| } | ||
| }; | ||
|
|
||
| app.post('/api/upload-data', authenticate, (req, res) => { |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, the fix is to introduce a rate-limiting middleware (e.g., using express-rate-limit) and apply it to routes that perform expensive operations after authorization, such as authenticated file uploads and database-heavy endpoints. This middleware should limit the number of requests per client (by IP, or another key) over a defined time window, returning 429 responses when the limit is exceeded.
For this specific code, the minimal, non-breaking fix is:
- Import
express-rate-limitnear the top ofserver/server.jswithout modifying existing imports. - Define a limiter instance with reasonable defaults, for example, a stricter limiter for the upload route (since uploads are relatively expensive).
- Apply this limiter to the
/api/upload-dataroute by adding it betweenauthenticateand the actual handler (uploadSingleinvocation). This keeps existing authentication and functionality intact while enforcing rate limits only on this route. - Optionally, if desired (but not required by the alert), the same limiter (or a different one) could be reused for other authenticated, heavy routes; to keep changes minimal, we only touch the route highlighted by CodeQL.
Concretely:
- Add
const rateLimit = require('express-rate-limit');after the existingrequiredeclarations. - Add a
const uploadLimiter = rateLimit({ ... })configuration after app/DB setup but before route definitions. - Change the route declaration from
app.post('/api/upload-data', authenticate, (req, res) => { ... })toapp.post('/api/upload-data', authenticate, uploadLimiter, (req, res) => { ... }).
Suggested changeset
2
server/server.js
| @@ -9,6 +9,7 @@ | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const crypto = require('crypto'); | ||
| const rateLimit = require('express-rate-limit'); | ||
|
|
||
| const app = express(); | ||
| const PORT = process.env.PORT || 3000; | ||
| @@ -118,6 +119,14 @@ | ||
| )`); | ||
| }); | ||
|
|
||
| // Rate limiting for expensive authenticated routes | ||
| const uploadLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 50, // limit each IP to 50 upload requests per windowMs | ||
| standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers | ||
| legacyHeaders: false, // Disable the `X-RateLimit-*` headers | ||
| }); | ||
|
|
||
| // Auth Middleware | ||
| const authenticate = (req, res, next) => { | ||
| const authHeader = req.headers['authorization']; | ||
| @@ -226,7 +235,7 @@ | ||
| } | ||
| }; | ||
|
|
||
| app.post('/api/upload-data', authenticate, (req, res) => { | ||
| app.post('/api/upload-data', authenticate, uploadLimiter, (req, res) => { | ||
| uploadSingle(req, res, async (err) => { | ||
| if (err) { | ||
| let message = 'Failed to upload file.'; |
server/package.json
Outside changed files
| @@ -17,6 +17,7 @@ | ||
| "jsonwebtoken": "^9.0.3", | ||
| "multer": "^2.0.2", | ||
| "sqlite3": "^5.1.7", | ||
| "xlsx": "^0.18.5" | ||
| "xlsx": "^0.18.5", | ||
| "express-rate-limit": "^8.3.1" | ||
| } | ||
| } |
This fix introduces these dependencies
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.3.1 | None |
Copilot is powered by AI and may make mistakes. Always verify output.
Comment on lines
+93
to
+98
| body: JSON.stringify({ | ||
| species: yld.species, | ||
| product: yld.product, | ||
| yield_percentage: yld.yield, | ||
| source: yld.source || 'User Input', | ||
| }), |
There was a problem hiding this comment.
Bug: The client sends yield_percentage when syncing custom yields, but the server API expects the key yield, causing the request to fail validation and preventing data from being saved.
Severity: HIGH
Suggested Fix
In app/src/lib/syncEngine.js, modify the body of the POST request sent to /api/user-data. Change the key yield_percentage to yield to match the server's expectation. The line yield_percentage: yld.yield should be updated to yield: yld.yield.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/src/lib/syncEngine.js#L93-L98
Potential issue: When syncing a user-defined custom yield, the client-side code in
`syncEngine.js` sends a POST request to `/api/user-data` with a JSON payload containing
the key `yield_percentage`. However, the server-side API handler expects this value
under the key `yield`. This mismatch causes the server's validation to fail, as
`yieldVal` becomes `undefined`, leading to a 400 Bad Request response. As a result,
custom yields are never persisted to the server, breaking the sync functionality for
this feature.
9 tasks
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
2 participants
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.
Summary
Test plan
npm run buildpasses,npm testpasses (1788 tests)🤖 Generated with Claude Code