Gotta grow fast#1371
Draft
Phrosfire wants to merge 7 commits into
Draft
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughAdds a complete farming occupation to the game: a new ChangesFarming Occupation System
Sequence Diagram(s)sequenceDiagram
actor Player
participant FarmPage as farm/page.tsx
participant OccFarming as OccupationFarming
participant API as api.farming (tRPC)
participant FarmCanvas as FarmCanvas (Three.js)
participant PlotPanel as PlotActionPanel
participant ShopPanel as FarmShopPanel
Player->>FarmPage: navigate to /farm
FarmPage->>OccFarming: render (village access gated)
OccFarming->>API: getFarmState (poll every 30s)
API-->>OccFarming: FarmStateResponse
OccFarming->>FarmCanvas: updateState(visualState) via ref
Player->>FarmCanvas: click plot
FarmCanvas-->>OccFarming: onPlotClick(plotState)
alt plot isReady
OccFarming->>API: harvestPlot(plotId)
API-->>OccFarming: updated FarmStateResponse
else plot selected
OccFarming->>PlotPanel: activePlot = plotState
Player->>PlotPanel: plant seed / water / fertilize
PlotPanel->>API: plantSeed / waterPlot / applyFertilizer
API-->>PlotPanel: success + invalidate farm state
end
Player->>ShopPanel: buy plot / extractor / seed
ShopPanel->>API: buyShopItem(type, itemId)
API-->>ShopPanel: success + updated FarmStateResponse
ShopPanel->>FarmCanvas: expandGrid(newTotalPlots, slotIndex)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
the
size:XXL
Phrosfire
force-pushed
the
new-occupations
branch
from
c2aaf36 to
1aecf0b
Compare
Contributor
Confidence Score: 2/5The farming router has a race condition in harvestPlot that allows a user to receive duplicate crop items by sending two concurrent requests. In harvestPlot, awardItemToUser runs in the same Promise.all as the CAS guard on the plot update. Two rapid requests can both read a ready plot, both pass the isPlotReady pre-check, and both execute awardItemToUser — even though only one CAS update will succeed. The net result is a player receives two crops from one harvest. app/src/server/api/routers/farming.ts — specifically harvestPlot (Promise.all race), the three XP fetch helpers (plantSeed, harvestPlot, applyFertilizer), the SEED/FERTILIZER double-item-fetch in buyShopItem, and the sequential queries in fetchFarmPlotWithItems. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant farmingRouter
participant DB
Client->>farmingRouter: plantSeed(plotId, seedItemId)
farmingRouter->>DB: Promise.all[fetchUser, fetchUserItems, farmPlot, item]
farmingRouter->>DB: claimUserSnapshot (CAS on updatedAt)
farmingRouter->>DB: updateUserItemQuantityAtomically (CAS on quantity)
farmingRouter->>DB: update farmPlot (set seedItemId, finishAt)
farmingRouter->>DB: fetchSeedPlantExperience ⚠️ redundant
farmingRouter->>DB: awardFarmingExperience
farmingRouter-->>Client: success
Client->>farmingRouter: harvestPlot(plotId)
farmingRouter->>DB: Promise.all[fetchUser, fetchUserItems, fetchFarmPlotWithItems]
Note over farmingRouter,DB: fetchFarmPlotWithItems = 3 sequential queries
farmingRouter->>DB: Promise.all[CAS update farmPlot, awardItemToUser] ⚠️ race
farmingRouter->>DB: fetchCropHarvestExperience ⚠️ redundant
farmingRouter->>DB: awardFarmingExperience
farmingRouter-->>Client: success
Client->>farmingRouter: buyShopItem(SEED, itemId)
farmingRouter->>DB: fetchUser
farmingRouter->>DB: resolveShopEntry → item (query 1)
farmingRouter->>DB: item findFirst (query 2) ⚠️ redundant
farmingRouter->>DB: awardItemToUser
farmingRouter-->>Client: success
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant farmingRouter
participant DB
Client->>farmingRouter: plantSeed(plotId, seedItemId)
farmingRouter->>DB: Promise.all[fetchUser, fetchUserItems, farmPlot, item]
farmingRouter->>DB: claimUserSnapshot (CAS on updatedAt)
farmingRouter->>DB: updateUserItemQuantityAtomically (CAS on quantity)
farmingRouter->>DB: update farmPlot (set seedItemId, finishAt)
farmingRouter->>DB: fetchSeedPlantExperience ⚠️ redundant
farmingRouter->>DB: awardFarmingExperience
farmingRouter-->>Client: success
Client->>farmingRouter: harvestPlot(plotId)
farmingRouter->>DB: Promise.all[fetchUser, fetchUserItems, fetchFarmPlotWithItems]
Note over farmingRouter,DB: fetchFarmPlotWithItems = 3 sequential queries
farmingRouter->>DB: Promise.all[CAS update farmPlot, awardItemToUser] ⚠️ race
farmingRouter->>DB: fetchCropHarvestExperience ⚠️ redundant
farmingRouter->>DB: awardFarmingExperience
farmingRouter-->>Client: success
Client->>farmingRouter: buyShopItem(SEED, itemId)
farmingRouter->>DB: fetchUser
farmingRouter->>DB: resolveShopEntry → item (query 1)
farmingRouter->>DB: item findFirst (query 2) ⚠️ redundant
farmingRouter->>DB: awardItemToUser
farmingRouter-->>Client: success
Prompt To Fix All With AIFix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
app/src/server/api/routers/farming.ts:291-310
**Harvest item awarded unconditionally in parallel with CAS guard**
`awardItemToUser` is launched in the same `Promise.all` as the CAS update, so it runs concurrently — not after — the guard. If two harvest requests arrive before either commits (e.g. a double-click or network retry), both will pass the `isPlotReady` pre-check on stale plot state, and both will call `awardItemToUser` even though only one CAS update will succeed, yielding a duplicate crop.
The fix is to await the CAS update first, verify `rowsAffected === 1`, then call `awardItemToUser`.
### Issue 2 of 5
app/src/server/api/routers/farming.ts:111
**Redundant DB round-trips for XP values already loaded earlier**
`plantSeed` calls `fetchSeedPlantExperience` (an extra `SELECT`) on line 111, but `seedItem` was already fetched in the opening `Promise.all` and carries `farmPlantExperience` / `farmHarvestExperience` — calling `getFarmPlantExperience(seedItem)` directly would eliminate this round-trip.
The same pattern recurs in `harvestPlot` (`fetchCropHarvestExperience` on line 288 when `plot.yieldItem.farmHarvestExperience` is already available from `fetchFarmPlotWithItems`) and in `applyFertilizer` (`fetchFertilizerExperience` on line 235 when `fertilizerStack.item.farmFertilizerExperience` is already in the loaded `userItems` relation).
### Issue 3 of 5
app/src/server/api/routers/farming.ts:468-488
**Redundant item fetch in the SEED/FERTILIZER purchase path**
`resolveShopEntry` already queries the item (line 593 inside the function) and returns its `itemId` via `shopEntry.itemId`. Lines 469-472 then fetch the same item again purely to extract its `id`. Since `awardItemToUser` only needs the `itemId`, this second fetch can be replaced with `input.itemId` (already validated non-null by the `!input.itemId` guard on line 468).
### Issue 4 of 5
app/src/server/api/routers/farming.ts:562-583
**Three sequential DB queries where the existing Drizzle relation is unused**
`fetchFarmPlotWithItems` makes three back-to-back round-trips: one for the plot, one for the seed item, one for the yield item. The `farmPlotRelations` in `schema.ts` already defines a `seedItem` relation on `farmPlot`, so using `with: { seedItem: true }` in the initial `findFirst` call would collapse the first two round-trips into one JOIN query, leaving only the yield-item lookup as an unavoidable second step.
### Issue 5 of 5
app/src/libs/dayNight.ts:26-33
**`phaseDuration` is hard-coded to day duration for both phases**
`getWorldCyclePosition` always returns `phaseDuration: dayMs` regardless of whether the current phase is day or night. Today this is harmless because `WORLD_DAY_SECONDS === WORLD_NIGHT_SECONDS`, but `getWorldCycleBrightness` uses `phaseDuration` to compute transition windows for both day and night. If `WORLD_NIGHT_SECONDS` is ever made different from `WORLD_DAY_SECONDS`, the night brightness transitions will be computed against the wrong duration.
Reviews (1): Last reviewed commit: "Fix level again" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+291
to
+310
| await Promise.all([ | ||
| ctx.drizzle | ||
| .update(farmPlot) | ||
| .set({ | ||
| seedItemId: null, | ||
| plantedAt: null, | ||
| finishAt: null, | ||
| lastWateredAt: null, | ||
| fertilizerApplied: false, | ||
| updatedAt: now, | ||
| }) | ||
| .where( | ||
| and( | ||
| eq(farmPlot.id, plot.id), | ||
| eq(farmPlot.userId, ctx.userId), | ||
| sql`${farmPlot.finishAt} <= ${now}`, | ||
| ), | ||
| ), | ||
| awardItemToUser(ctx.drizzle, ctx.userId, plot.yieldItem.id, 1), | ||
| ]); |
Contributor
There was a problem hiding this comment.
awardItemToUser is launched in the same Promise.all as the CAS update, so it runs concurrently — not after — the guard. If two harvest requests arrive before either commits (e.g. a double-click or network retry), both will pass the isPlotReady pre-check on stale plot state, and both will call awardItemToUser even though only one CAS update will succeed, yielding a duplicate crop.
The fix is to await the CAS update first, verify rowsAffected === 1, then call awardItemToUser.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/farming.ts
Line: 291-310
Comment:
**Harvest item awarded unconditionally in parallel with CAS guard**
`awardItemToUser` is launched in the same `Promise.all` as the CAS update, so it runs concurrently — not after — the guard. If two harvest requests arrive before either commits (e.g. a double-click or network retry), both will pass the `isPlotReady` pre-check on stale plot state, and both will call `awardItemToUser` even though only one CAS update will succeed, yielding a duplicate crop.
The fix is to await the CAS update first, verify `rowsAffected === 1`, then call `awardItemToUser`.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const now = new Date(); | ||
| const finishAt = new Date(now.getTime() + seedItem.farmGrowTimeSeconds * 1000); | ||
| const xpGain = await fetchSeedPlantExperience(ctx.drizzle, input.seedItemId); |
Contributor
There was a problem hiding this comment.
plantSeed calls fetchSeedPlantExperience (an extra SELECT) on line 111, but seedItem was already fetched in the opening Promise.all and carries farmPlantExperience / farmHarvestExperience — calling getFarmPlantExperience(seedItem) directly would eliminate this round-trip.
The same pattern recurs in harvestPlot (fetchCropHarvestExperience on line 288 when plot.yieldItem.farmHarvestExperience is already available from fetchFarmPlotWithItems) and in applyFertilizer (fetchFertilizerExperience on line 235 when fertilizerStack.item.farmFertilizerExperience is already in the loaded userItems relation).
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/farming.ts
Line: 111
Comment:
**Redundant DB round-trips for XP values already loaded earlier**
`plantSeed` calls `fetchSeedPlantExperience` (an extra `SELECT`) on line 111, but `seedItem` was already fetched in the opening `Promise.all` and carries `farmPlantExperience` / `farmHarvestExperience` — calling `getFarmPlantExperience(seedItem)` directly would eliminate this round-trip.
The same pattern recurs in `harvestPlot` (`fetchCropHarvestExperience` on line 288 when `plot.yieldItem.farmHarvestExperience` is already available from `fetchFarmPlotWithItems`) and in `applyFertilizer` (`fetchFertilizerExperience` on line 235 when `fertilizerStack.item.farmFertilizerExperience` is already in the loaded `userItems` relation).
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+468
to
+488
| if (!input.itemId) return errorResponse("Item required"); | ||
| const shopItem = await ctx.drizzle.query.item.findFirst({ | ||
| where: eq(item.id, input.itemId), | ||
| }); | ||
| if (!shopItem) return errorResponse("Item not found"); | ||
|
|
||
| const currencyResult = await ctx.drizzle | ||
| .update(userData) | ||
| .set({ farmCurrency: sql`${userData.farmCurrency} - ${shopEntry.cost}` }) | ||
| .where( | ||
| and( | ||
| eq(userData.userId, ctx.userId), | ||
| gte(userData.farmCurrency, shopEntry.cost), | ||
| ), | ||
| ); | ||
| if (currencyResult.rowsAffected !== 1) { | ||
| return errorResponse("Not enough farm coins"); | ||
| } | ||
|
|
||
| await awardItemToUser(ctx.drizzle, ctx.userId, shopItem.id, 1); | ||
| return { success: true, message: `Purchased ${shopItem.name}` }; |
Contributor
There was a problem hiding this comment.
resolveShopEntry already queries the item (line 593 inside the function) and returns its itemId via shopEntry.itemId. Lines 469-472 then fetch the same item again purely to extract its id. Since awardItemToUser only needs the itemId, this second fetch can be replaced with input.itemId (already validated non-null by the !input.itemId guard on line 468).
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/farming.ts
Line: 468-488
Comment:
**Redundant item fetch in the SEED/FERTILIZER purchase path**
`resolveShopEntry` already queries the item (line 593 inside the function) and returns its `itemId` via `shopEntry.itemId`. Lines 469-472 then fetch the same item again purely to extract its `id`. Since `awardItemToUser` only needs the `itemId`, this second fetch can be replaced with `input.itemId` (already validated non-null by the `!input.itemId` guard on line 468).
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+562
to
+583
| const fetchFarmPlotWithItems = async ( | ||
| client: DrizzleClient, | ||
| userId: string, | ||
| plotId: string, | ||
| ) => { | ||
| const plot = await client.query.farmPlot.findFirst({ | ||
| where: and(eq(farmPlot.id, plotId), eq(farmPlot.userId, userId)), | ||
| }); | ||
| if (!plot?.seedItemId) | ||
| return plot ? { ...plot, seedItem: null, yieldItem: null } : null; | ||
|
|
||
| const seedItem = await client.query.item.findFirst({ | ||
| where: eq(item.id, plot.seedItemId), | ||
| }); | ||
| const yieldItem = seedItem?.farmYieldItemId | ||
| ? await client.query.item.findFirst({ | ||
| where: eq(item.id, seedItem.farmYieldItemId), | ||
| }) | ||
| : null; | ||
|
|
||
| return { ...plot, seedItem: seedItem ?? null, yieldItem: yieldItem ?? null }; | ||
| }; |
Contributor
There was a problem hiding this comment.
fetchFarmPlotWithItems makes three back-to-back round-trips: one for the plot, one for the seed item, one for the yield item. The farmPlotRelations in schema.ts already defines a seedItem relation on farmPlot, so using with: { seedItem: true } in the initial findFirst call would collapse the first two round-trips into one JOIN query, leaving only the yield-item lookup as an unavoidable second step.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/farming.ts
Line: 562-583
Comment:
**Three sequential DB queries where the existing Drizzle relation is unused**
`fetchFarmPlotWithItems` makes three back-to-back round-trips: one for the plot, one for the seed item, one for the yield item. The `farmPlotRelations` in `schema.ts` already defines a `seedItem` relation on `farmPlot`, so using `with: { seedItem: true }` in the initial `findFirst` call would collapse the first two round-trips into one JOIN query, leaving only the yield-item lookup as an unavoidable second step.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+26
to
+33
| return { | ||
| cycleStart, | ||
| cycleMs, | ||
| dayMs, | ||
| phase, | ||
| phaseOffset, | ||
| phaseDuration: dayMs, | ||
| }; |
Contributor
There was a problem hiding this comment.
phaseDuration is hard-coded to day duration for both phases
getWorldCyclePosition always returns phaseDuration: dayMs regardless of whether the current phase is day or night. Today this is harmless because WORLD_DAY_SECONDS === WORLD_NIGHT_SECONDS, but getWorldCycleBrightness uses phaseDuration to compute transition windows for both day and night. If WORLD_NIGHT_SECONDS is ever made different from WORLD_DAY_SECONDS, the night brightness transitions will be computed against the wrong duration.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/libs/dayNight.ts
Line: 26-33
Comment:
**`phaseDuration` is hard-coded to day duration for both phases**
`getWorldCyclePosition` always returns `phaseDuration: dayMs` regardless of whether the current phase is day or night. Today this is harmless because `WORLD_DAY_SECONDS === WORLD_NIGHT_SECONDS`, but `getWorldCycleBrightness` uses `phaseDuration` to compute transition windows for both day and night. If `WORLD_NIGHT_SECONDS` is ever made different from `WORLD_DAY_SECONDS`, the night brightness transitions will be computed against the wrong duration.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
app/src/layout/Sector.tsx (1)
1028-1028: ⚡ Quick winThrottle canvas brightness style updates in the animation loop.
Brightness changes over minutes, so updating
element.style.filterevery frame does unnecessary work on a hot path.♻️ Proposed refactor
let lastTime = Date.now(); let animationId = 0; let userAngle = 0; + let lastBrightnessUpdateAt = 0; @@ - applyCanvasDayNightBrightness(renderer?.domElement); + const nowMs = Date.now(); + if (nowMs - lastBrightnessUpdateAt >= 1000) { + applyCanvasDayNightBrightness(renderer?.domElement); + lastBrightnessUpdateAt = nowMs; + } if (!contextHandlers.isContextLost() && renderer) { renderer.render(scene, camera); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/layout/Sector.tsx` at line 1028, The applyCanvasDayNightBrightness function is being called every animation frame, unnecessarily updating the canvas element's style.filter property despite brightness changes only occurring over minutes. Implement throttling for this function call by tracking the last time it was executed and only calling applyCanvasDayNightBrightness when a sufficient time interval (such as several seconds or minutes) has elapsed since the last call, rather than on every frame iteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/layout/FarmCanvas.tsx`:
- Around line 231-233: The propsAreEqual comparator function always returns
true, which prevents the FarmCanvas memoized component from ever re-rendering
based on prop changes, causing stale callbacks and behavior. Remove the custom
propsAreEqual function entirely and call memo on FarmCanvasInner with only the
component argument to use React's default shallow prop comparison, or if custom
comparison is necessary, implement logic that returns true only when props are
actually equal and false when they differ.
- Around line 122-125: When the renderer creation fails in the setupScene
function, the window resize listener that was registered earlier is not cleaned
up before the early return. Add code to remove the resize event listener (likely
using window.removeEventListener with the 'resize' event) in the error branch
where renderer is falsy, before the return statement, to prevent orphaned
listeners from accumulating.
- Around line 89-103: In the expandGrid function, after updating
farmStateRef.current with the new totalPlots value, reposition all existing
plots since getPlotGridPosition depends on totalPlots to calculate coordinates.
After the farmStateRef.current update, iterate through all
plotEntriesRef.current entries and recalculate their grid positions using
getPlotGridPosition with newTotalPlots, then update each plot's soil mesh
position to reflect the new coordinates. This ensures that when the grid expands
(especially crossing row boundaries), all existing plots maintain correct visual
alignment instead of keeping stale coordinates from the previous totalPlots
value.
- Around line 196-200: Replace the modulo-based timing logic with elapsed-time
accumulation to ensure consistent periodic refresh behavior. Instead of using
`now % 2000 < 16`, maintain a reference variable to track the last refresh
timestamp (initialize it when the component mounts or when you first enter the
refresh logic). On each frame, calculate the elapsed time by subtracting the
last refresh timestamp from the current Date.now() value. When the elapsed time
exceeds your desired interval (2000ms), call refreshPlotVisuals with
selectedSlotRef.current and then update the last refresh timestamp to the
current time. This ensures the refresh happens at consistent intervals
regardless of frame rate or when the component lifecycle begins.
In `@app/src/layout/ItemWithEffects.tsx`:
- Around line 428-432: In the conditional logic that displays farming experience
in the item details, the second branch checks for item.farmHarvestExperience but
incorrectly labels it as "plant" XP instead of "harvest" XP. Change the label
text in the second ternary branch from "plant" to "harvest" so that when
farmHarvestExperience is being displayed, it correctly shows "harvest" instead
of "plant".
In `@app/src/layout/OccupationFarming.tsx`:
- Around line 69-74: In the OccupationFarming.tsx file, the condition that
handles when prevTotal is greater than zero and totalPlots increases only syncs
the last new plot by passing totalPlots - 1 to syncCanvas. If totalPlots jumps
by more than one slot in a single refresh, intermediate slots between the
previous and new total are never created. Modify the logic in this branch to
iterate through all new plots that need to be created (from prevTotal to the new
isFarmState.totalPlots) and ensure syncCanvas is called for each intermediate
slot, not just the final one.
In `@app/src/libs/dayNight.ts`:
- Around line 24-33: The phaseDuration field is hardcoded to always be dayMs,
but it should reflect the actual duration of the current phase. When the phase
is "night" (determined by the phase constant), phaseDuration should be the night
duration (cycleMs minus dayMs), not dayMs. Fix this by making phaseDuration
conditional: set it to dayMs when phase is "day", and set it to cycleMs - dayMs
when phase is "night". Apply this fix to both occurrences of this return
statement in the file (around lines 24-33 and lines 61-67).
In `@app/src/server/api/routers/farming.ts`:
- Around line 346-351: The selling operation in the farmSellValue check block
only validates that the item exists and has sufficient quantity, but does not
verify that the item stack is actively available (not in auction, home storage,
or crafting). Add an availability guard check after confirming the cropStack
exists to ensure the item can only be sold when actively available. Apply the
same availability validation to the extractSeeds function (around lines 517-526)
to prevent operating on unavailable inventory stacks in both locations.
- Around line 418-438: The shop purchase logic reads a stale value of
farmPlotsPurchased to calculate newTotal, then updates the counter without
verifying it hasn't changed by concurrent requests, creating a race condition
that can bypass purchase limits or cause mismatched slot insertion. Add a
compare-and-swap guard to the WHERE clause by checking
eq(userData.farmPlotsPurchased, user.farmPlotsPurchased) alongside the existing
conditions in the update statement, and treat rowsAffected !== 1 as a retryable
state-changed error. Apply the same compare-and-swap pattern to the
farmExtractorsOwned update in the extractors purchase code (around lines
449-460).
- Around line 164-176: The farmPlot update operation and subsequent XP award
lack concurrency protection, allowing parallel requests to grant duplicate XP
for a single watering cycle. Modify the update call on farmPlot to capture the
rowsAffected result and add a compare-and-swap predicate in the WHERE clause
that validates the plot state hasn't changed (such as checking the current value
of finishAt or lastWateredAt to ensure no concurrent modification), then only
proceed to call awardFarmingExperience if rowsAffected is greater than zero,
ensuring XP is only awarded when the update actually succeeded.
- Around line 291-310: The Promise.all block in the farming.ts file runs the
farmPlot update and awardItemToUser call in parallel, creating a race condition
where both operations can award items even if the update affects zero rows.
Replace the parallel Promise.all execution with sequential operations: first
execute the farmPlot update to reset the plot state, capture the result and
check its rowsAffected count, then only call awardItemToUser if rowsAffected is
at least one. This ensures rewards are only granted when the plot-state update
succeeds, preventing duplicate yields from concurrent harvest requests.
- Around line 885-910: The code in the awardItemToUser function does not
properly respect stack size constraints when inserting items. Both the overflow
insertion (when overflow > 0) and the new item insertion (when no existing item
is found) can create oversized stacks that exceed itemInfo.stackSize. Fix this
by splitting both the overflow quantity and the initial quantity parameter into
multiple rows, each respecting the itemInfo.stackSize limit. Instead of
inserting overflow as a single row with the calculated overflow quantity, create
a loop that chunks the overflow into stack-sized pieces and inserts each chunk
as a separate row. Similarly, for the case where no existing item is found,
chunk the quantity parameter into stack-sized pieces and insert each chunk as a
separate row with quantity not exceeding itemInfo.stackSize.
In `@app/tests/libs/dayNight.test.ts`:
- Around line 12-13: The test file dayNight.test.ts uses hardcoded minute
offsets (8 and 23) to create the midDay and midNight timestamps, which makes the
test brittle if the day/night cycle constants are adjusted in production code.
Replace the hardcoded offsets in the midDay and midNight variable assignments
with values derived from the same constants that the production logic uses for
the day/night cycle timing, ensuring the test stays synchronized with any future
changes to those constants.
---
Nitpick comments:
In `@app/src/layout/Sector.tsx`:
- Line 1028: The applyCanvasDayNightBrightness function is being called every
animation frame, unnecessarily updating the canvas element's style.filter
property despite brightness changes only occurring over minutes. Implement
throttling for this function call by tracking the last time it was executed and
only calling applyCanvasDayNightBrightness when a sufficient time interval (such
as several seconds or minutes) has elapsed since the last call, rather than on
every frame iteration.
🪄 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: e150dc25-3b0a-488b-ac78-77ae494af5c0
📒 Files selected for processing (27)
app/drizzle/constants.tsapp/drizzle/migrations/0017_nice_thunderbolt.sqlapp/drizzle/schema.tsapp/src/app/farm/page.tsxapp/src/app/home/page.tsxapp/src/hooks/day-night-cycle.tsapp/src/hooks/farm-cycle.tsapp/src/hooks/item.tsapp/src/layout/DayNightIndicator.tsxapp/src/layout/FarmCanvas.tsxapp/src/layout/FarmHUD.tsxapp/src/layout/FarmShopPanel.tsxapp/src/layout/ItemWithEffects.tsxapp/src/layout/OccupationFarming.tsxapp/src/layout/PlotActionPanel.tsxapp/src/layout/Sector.tsxapp/src/libs/dayNight.tsapp/src/libs/farming.tsapp/src/libs/threejs/dayNight.tsapp/src/libs/threejs/farming.tsapp/src/server/api/root.tsapp/src/server/api/routers/farming.tsapp/src/server/api/routers/item.tsapp/src/validators/combat.tsapp/src/validators/farming.tsapp/tests/libs/dayNight.test.tsapp/tests/libs/farming.test.ts
Comment on lines
+89
to
+103
| expandGrid: (newTotalPlots: number, newSlotIndex: number) => { | ||
| const groups = groupsRef.current; | ||
| if (!groups) return; | ||
| if (plotEntriesRef.current.some((e) => e.slotIndex === newSlotIndex)) return; | ||
|
|
||
| const entry = createPlotMesh(newSlotIndex, newTotalPlots); | ||
| startPlotExpandAnimation(entry); | ||
| groups.plots.add(entry.soil); | ||
| plotEntriesRef.current.push(entry); | ||
| farmStateRef.current = { | ||
| ...farmStateRef.current, | ||
| totalPlots: newTotalPlots, | ||
| }; | ||
| refreshPlotVisuals(selectedSlotRef.current); | ||
| }, |
Contributor
There was a problem hiding this comment.
Reposition existing plot meshes when expanding the grid.
getPlotGridPosition depends on totalPlots, but expandGrid only positions the new slot. When total plots cross a row boundary (e.g., 5 → 6), existing plots keep stale coordinates and the grid becomes visually misaligned.
Suggested fix
import {
animatePlotEntries,
createFarmGroups,
+ getPlotGridPosition,
createPlotMesh,
drawFarmBackground,
type PlotMeshEntry,
@@
expandGrid: (newTotalPlots: number, newSlotIndex: number) => {
const groups = groupsRef.current;
if (!groups) return;
if (plotEntriesRef.current.some((e) => e.slotIndex === newSlotIndex)) return;
+
+ for (const existingEntry of plotEntriesRef.current) {
+ const nextPosition = getPlotGridPosition(existingEntry.slotIndex, newTotalPlots);
+ existingEntry.soil.position.set(nextPosition.x, 0.01, nextPosition.z);
+ }
const entry = createPlotMesh(newSlotIndex, newTotalPlots);📝 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
| expandGrid: (newTotalPlots: number, newSlotIndex: number) => { | |
| const groups = groupsRef.current; | |
| if (!groups) return; | |
| if (plotEntriesRef.current.some((e) => e.slotIndex === newSlotIndex)) return; | |
| const entry = createPlotMesh(newSlotIndex, newTotalPlots); | |
| startPlotExpandAnimation(entry); | |
| groups.plots.add(entry.soil); | |
| plotEntriesRef.current.push(entry); | |
| farmStateRef.current = { | |
| ...farmStateRef.current, | |
| totalPlots: newTotalPlots, | |
| }; | |
| refreshPlotVisuals(selectedSlotRef.current); | |
| }, | |
| import { | |
| animatePlotEntries, | |
| createFarmGroups, | |
| getPlotGridPosition, | |
| createPlotMesh, | |
| drawFarmBackground, | |
| type PlotMeshEntry, | |
| // ... other imports | |
| } | |
| // ... in the code where expandGrid is defined: | |
| expandGrid: (newTotalPlots: number, newSlotIndex: number) => { | |
| const groups = groupsRef.current; | |
| if (!groups) return; | |
| if (plotEntriesRef.current.some((e) => e.slotIndex === newSlotIndex)) return; | |
| for (const existingEntry of plotEntriesRef.current) { | |
| const nextPosition = getPlotGridPosition(existingEntry.slotIndex, newTotalPlots); | |
| existingEntry.soil.position.set(nextPosition.x, 0.01, nextPosition.z); | |
| } | |
| const entry = createPlotMesh(newSlotIndex, newTotalPlots); | |
| startPlotExpandAnimation(entry); | |
| groups.plots.add(entry.soil); | |
| plotEntriesRef.current.push(entry); | |
| farmStateRef.current = { | |
| ...farmStateRef.current, | |
| totalPlots: newTotalPlots, | |
| }; | |
| refreshPlotVisuals(selectedSlotRef.current); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/layout/FarmCanvas.tsx` around lines 89 - 103, In the expandGrid
function, after updating farmStateRef.current with the new totalPlots value,
reposition all existing plots since getPlotGridPosition depends on totalPlots to
calculate coordinates. After the farmStateRef.current update, iterate through
all plotEntriesRef.current entries and recalculate their grid positions using
getPlotGridPosition with newTotalPlots, then update each plot's soil mesh
position to reflect the new coordinates. This ensures that when the grid expands
(especially crossing row boundaries), all existing plots maintain correct visual
alignment instead of keeping stale coordinates from the previous totalPlots
value.
Comment on lines
+122
to
+125
| if (!renderer) { | ||
| setWebglError(true); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Clean up the resize listener on WebGL init failure.
setupScene registers window.resize before returning. If renderer creation fails, this branch exits without removing the listener.
Suggested fix
if (!renderer) {
+ window.removeEventListener("resize", handleResize);
setWebglError(true);
return;
}📝 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
| if (!renderer) { | |
| setWebglError(true); | |
| return; | |
| } | |
| if (!renderer) { | |
| window.removeEventListener("resize", handleResize); | |
| setWebglError(true); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/layout/FarmCanvas.tsx` around lines 122 - 125, When the renderer
creation fails in the setupScene function, the window resize listener that was
registered earlier is not cleaned up before the early return. Add code to remove
the resize event listener (likely using window.removeEventListener with the
'resize' event) in the error branch where renderer is falsy, before the return
statement, to prevent orphaned listeners from accumulating.
Comment on lines
+196
to
+200
| // Refresh growth stages periodically without full React re-render | ||
| const now = Date.now(); | ||
| if (now % 2000 < 16) { | ||
| refreshPlotVisuals(selectedSlotRef.current); | ||
| } |
Contributor
There was a problem hiding this comment.
Use elapsed-time accumulation instead of % Date.now() for periodic refresh.
now % 2000 < 16 is frame-phase dependent and can miss refresh windows on lower FPS, causing inconsistent visual updates.
Suggested fix
const clock = new Clock();
let animationId: number;
+ let refreshAccumulator = 0;
@@
const delta = clock.getDelta();
animatePlotEntries(plotEntriesRef.current, delta);
+ refreshAccumulator += delta;
@@
- const now = Date.now();
- if (now % 2000 < 16) {
+ if (refreshAccumulator >= 2) {
refreshPlotVisuals(selectedSlotRef.current);
+ refreshAccumulator = 0;
}📝 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
| // Refresh growth stages periodically without full React re-render | |
| const now = Date.now(); | |
| if (now % 2000 < 16) { | |
| refreshPlotVisuals(selectedSlotRef.current); | |
| } | |
| const clock = new Clock(); | |
| let animationId: number; | |
| let refreshAccumulator = 0; | |
| // Animation loop | |
| const animate = () => { | |
| animationId = requestAnimationFrame(animate); | |
| const delta = clock.getDelta(); | |
| animatePlotEntries(plotEntriesRef.current, delta); | |
| refreshAccumulator += delta; | |
| // Refresh growth stages periodically without full React re-render | |
| if (refreshAccumulator >= 2) { | |
| refreshPlotVisuals(selectedSlotRef.current); | |
| refreshAccumulator = 0; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/layout/FarmCanvas.tsx` around lines 196 - 200, Replace the
modulo-based timing logic with elapsed-time accumulation to ensure consistent
periodic refresh behavior. Instead of using `now % 2000 < 16`, maintain a
reference variable to track the last refresh timestamp (initialize it when the
component mounts or when you first enter the refresh logic). On each frame,
calculate the elapsed time by subtracting the last refresh timestamp from the
current Date.now() value. When the elapsed time exceeds your desired interval
(2000ms), call refreshPlotVisuals with selectedSlotRef.current and then update
the last refresh timestamp to the current time. This ensures the refresh happens
at consistent intervals regardless of frame rate or when the component lifecycle
begins.
Comment on lines
+231
to
+233
| const propsAreEqual = () => true; | ||
|
|
||
| export const FarmCanvas = memo(FarmCanvasInner, propsAreEqual); |
Contributor
There was a problem hiding this comment.
Avoid an always-true memo comparator.
Returning true unconditionally blocks prop-driven updates forever (including callback replacement), which can leave stale behavior wired in.
Suggested fix
-const propsAreEqual = () => true;
+const propsAreEqual = (prev: FarmCanvasProps, next: FarmCanvasProps) =>
+ prev.onPlotClick === next.onPlotClick;
export const FarmCanvas = memo(FarmCanvasInner, propsAreEqual);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/layout/FarmCanvas.tsx` around lines 231 - 233, The propsAreEqual
comparator function always returns true, which prevents the FarmCanvas memoized
component from ever re-rendering based on prop changes, causing stale callbacks
and behavior. Remove the custom propsAreEqual function entirely and call memo on
FarmCanvasInner with only the component argument to use React's default shallow
prop comparison, or if custom comparison is necessary, implement logic that
returns true only when props are actually equal and false when they differ.
Comment on lines
+428
to
+432
| {item.farmPlantExperience > 0 | ||
| ? ` · plant ${item.farmPlantExperience} XP` | ||
| : item.farmHarvestExperience > 0 | ||
| ? ` · plant ${item.farmHarvestExperience} XP` | ||
| : ""} |
Contributor
There was a problem hiding this comment.
Fix XP label mismatch in seed details
Line 431 shows farmHarvestExperience but labels it as plant XP. That displays incorrect semantics in the item details UI.
Suggested fix
- ? ` · plant ${item.farmHarvestExperience} XP`
+ ? ` · harvest ${item.farmHarvestExperience} XP`📝 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
| {item.farmPlantExperience > 0 | |
| ? ` · plant ${item.farmPlantExperience} XP` | |
| : item.farmHarvestExperience > 0 | |
| ? ` · plant ${item.farmHarvestExperience} XP` | |
| : ""} | |
| {item.farmPlantExperience > 0 | |
| ? ` · plant ${item.farmPlantExperience} XP` | |
| : item.farmHarvestExperience > 0 | |
| ? ` · harvest ${item.farmHarvestExperience} XP` | |
| : ""} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/layout/ItemWithEffects.tsx` around lines 428 - 432, In the
conditional logic that displays farming experience in the item details, the
second branch checks for item.farmHarvestExperience but incorrectly labels it as
"plant" XP instead of "harvest" XP. Change the label text in the second ternary
branch from "plant" to "harvest" so that when farmHarvestExperience is being
displayed, it correctly shows "harvest" instead of "plant".
Comment on lines
+291
to
+310
| await Promise.all([ | ||
| ctx.drizzle | ||
| .update(farmPlot) | ||
| .set({ | ||
| seedItemId: null, | ||
| plantedAt: null, | ||
| finishAt: null, | ||
| lastWateredAt: null, | ||
| fertilizerApplied: false, | ||
| updatedAt: now, | ||
| }) | ||
| .where( | ||
| and( | ||
| eq(farmPlot.id, plot.id), | ||
| eq(farmPlot.userId, ctx.userId), | ||
| sql`${farmPlot.finishAt} <= ${now}`, | ||
| ), | ||
| ), | ||
| awardItemToUser(ctx.drizzle, ctx.userId, plot.yieldItem.id, 1), | ||
| ]); |
Contributor
There was a problem hiding this comment.
Gate harvest rewards on a successful plot-state CAS update
Line 291 grants the crop in parallel with the plot reset. If two harvest requests race, one update can affect 0 rows while still awarding items, duplicating yield.
Suggested fix
- await Promise.all([
- ctx.drizzle
- .update(farmPlot)
- .set({
- seedItemId: null,
- plantedAt: null,
- finishAt: null,
- lastWateredAt: null,
- fertilizerApplied: false,
- updatedAt: now,
- })
- .where(
- and(
- eq(farmPlot.id, plot.id),
- eq(farmPlot.userId, ctx.userId),
- sql`${farmPlot.finishAt} <= ${now}`,
- ),
- ),
- awardItemToUser(ctx.drizzle, ctx.userId, plot.yieldItem.id, 1),
- ]);
+ const resetResult = await ctx.drizzle
+ .update(farmPlot)
+ .set({
+ seedItemId: null,
+ plantedAt: null,
+ finishAt: null,
+ lastWateredAt: null,
+ fertilizerApplied: false,
+ updatedAt: now,
+ })
+ .where(
+ and(
+ eq(farmPlot.id, plot.id),
+ eq(farmPlot.userId, ctx.userId),
+ sql`${farmPlot.finishAt} <= ${now}`,
+ ),
+ );
+ if (Number(resetResult.rowsAffected ?? 0) !== 1) {
+ return errorResponse("Crop was already harvested");
+ }
+
+ await awardItemToUser(ctx.drizzle, ctx.userId, plot.yieldItem.id, 1);As per coding guidelines, "Use compare-and-swap predicates and verify rowsAffected before granting irreversible rewards in tRPC endpoints".
📝 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
| await Promise.all([ | |
| ctx.drizzle | |
| .update(farmPlot) | |
| .set({ | |
| seedItemId: null, | |
| plantedAt: null, | |
| finishAt: null, | |
| lastWateredAt: null, | |
| fertilizerApplied: false, | |
| updatedAt: now, | |
| }) | |
| .where( | |
| and( | |
| eq(farmPlot.id, plot.id), | |
| eq(farmPlot.userId, ctx.userId), | |
| sql`${farmPlot.finishAt} <= ${now}`, | |
| ), | |
| ), | |
| awardItemToUser(ctx.drizzle, ctx.userId, plot.yieldItem.id, 1), | |
| ]); | |
| const resetResult = await ctx.drizzle | |
| .update(farmPlot) | |
| .set({ | |
| seedItemId: null, | |
| plantedAt: null, | |
| finishAt: null, | |
| lastWateredAt: null, | |
| fertilizerApplied: false, | |
| updatedAt: now, | |
| }) | |
| .where( | |
| and( | |
| eq(farmPlot.id, plot.id), | |
| eq(farmPlot.userId, ctx.userId), | |
| sql`${farmPlot.finishAt} <= ${now}`, | |
| ), | |
| ); | |
| if (Number(resetResult.rowsAffected ?? 0) !== 1) { | |
| return errorResponse("Crop was already harvested"); | |
| } | |
| await awardItemToUser(ctx.drizzle, ctx.userId, plot.yieldItem.id, 1); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/server/api/routers/farming.ts` around lines 291 - 310, The
Promise.all block in the farming.ts file runs the farmPlot update and
awardItemToUser call in parallel, creating a race condition where both
operations can award items even if the update affects zero rows. Replace the
parallel Promise.all execution with sequential operations: first execute the
farmPlot update to reset the plot state, capture the result and check its
rowsAffected count, then only call awardItemToUser if rowsAffected is at least
one. This ensures rewards are only granted when the plot-state update succeeds,
preventing duplicate yields from concurrent harvest requests.
Source: Coding guidelines
Comment on lines
+346
to
+351
| const cropStack = userItems.find((ui) => ui.id === input.userItemId); | ||
| if (!cropStack) return errorResponse("Item not found"); | ||
| const sellValue = cropStack.item.farmSellValue; | ||
| if (sellValue <= 0) return errorResponse("This crop cannot be sold here"); | ||
| if (cropStack.quantity < input.quantity) return errorResponse("Not enough crops"); | ||
|
|
Contributor
There was a problem hiding this comment.
Block selling/extracting inventory that is not actively available
These checks only match by userItemId and quantity. They currently allow operating on stacks that are in auction, stored at home, or still crafting.
Suggested fix
const cropStack = userItems.find((ui) => ui.id === input.userItemId);
if (!cropStack) return errorResponse("Item not found");
+ if (
+ cropStack.storedAtHome ||
+ cropStack.isInAuction ||
+ (cropStack.craftingFinishedAt && cropStack.craftingFinishedAt > new Date())
+ ) {
+ return errorResponse("Item is not available in inventory");
+ }Apply the same availability guard in extractSeeds.
Also applies to: 517-526
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/server/api/routers/farming.ts` around lines 346 - 351, The selling
operation in the farmSellValue check block only validates that the item exists
and has sufficient quantity, but does not verify that the item stack is actively
available (not in auction, home storage, or crafting). Add an availability guard
check after confirming the cropStack exists to ensure the item can only be sold
when actively available. Apply the same availability validation to the
extractSeeds function (around lines 517-526) to prevent operating on unavailable
inventory stacks in both locations.
Comment on lines
+418
to
+438
| const currencyResult = await ctx.drizzle | ||
| .update(userData) | ||
| .set({ | ||
| farmCurrency: sql`${userData.farmCurrency} - ${shopEntry.cost}`, | ||
| farmPlotsPurchased: sql`${userData.farmPlotsPurchased} + 1`, | ||
| }) | ||
| .where( | ||
| and( | ||
| eq(userData.userId, ctx.userId), | ||
| gte(userData.farmCurrency, shopEntry.cost), | ||
| ), | ||
| ); | ||
| if (currencyResult.rowsAffected !== 1) { | ||
| return errorResponse("Not enough farm coins"); | ||
| } | ||
|
|
||
| await ctx.drizzle.insert(farmPlot).values({ | ||
| id: nanoid(), | ||
| userId: ctx.userId, | ||
| slotIndex: newTotal - 1, | ||
| }); |
Contributor
There was a problem hiding this comment.
Use compare-and-swap guards for capped counters in shop purchases
Both branches mutate farmPlotsPurchased / farmExtractorsOwned using stale pre-read values. Concurrent buys can bypass limits or charge currency with conflicting slot insertion (slotIndex derived from stale state).
Suggested fix direction
- const currencyResult = await ctx.drizzle
+ const currencyResult = await ctx.drizzle
.update(userData)
.set({
farmCurrency: sql`${userData.farmCurrency} - ${shopEntry.cost}`,
farmPlotsPurchased: sql`${userData.farmPlotsPurchased} + 1`,
})
.where(
and(
eq(userData.userId, ctx.userId),
gte(userData.farmCurrency, shopEntry.cost),
+ eq(userData.farmPlotsPurchased, user.farmPlotsPurchased),
),
);Apply the same CAS pattern for extractors (eq(userData.farmExtractorsOwned, user.farmExtractorsOwned)), and treat CAS failure as a retryable "state changed" error.
As per coding guidelines, "Use compare-and-swap predicates and verify rowsAffected before granting irreversible rewards in tRPC endpoints".
Also applies to: 449-460
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/server/api/routers/farming.ts` around lines 418 - 438, The shop
purchase logic reads a stale value of farmPlotsPurchased to calculate newTotal,
then updates the counter without verifying it hasn't changed by concurrent
requests, creating a race condition that can bypass purchase limits or cause
mismatched slot insertion. Add a compare-and-swap guard to the WHERE clause by
checking eq(userData.farmPlotsPurchased, user.farmPlotsPurchased) alongside the
existing conditions in the update statement, and treat rowsAffected !== 1 as a
retryable state-changed error. Apply the same compare-and-swap pattern to the
farmExtractorsOwned update in the extractors purchase code (around lines
449-460).
Source: Coding guidelines
Comment on lines
+885
to
+910
| if (existing && itemInfo.canStack) { | ||
| const newQty = Math.min(existing.quantity + quantity, itemInfo.stackSize); | ||
| await client | ||
| .update(userItem) | ||
| .set({ quantity: newQty, updatedAt: new Date() }) | ||
| .where(eq(userItem.id, existing.id)); | ||
| const overflow = existing.quantity + quantity - newQty; | ||
| if (overflow > 0) { | ||
| await client.insert(userItem).values({ | ||
| id: nanoid(), | ||
| userId, | ||
| itemId, | ||
| quantity: overflow, | ||
| equipped: "NONE", | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| await client.insert(userItem).values({ | ||
| id: nanoid(), | ||
| userId, | ||
| itemId, | ||
| quantity, | ||
| equipped: "NONE", | ||
| }); |
Contributor
There was a problem hiding this comment.
Split awarded quantities into stack-sized chunks
awardItemToUser inserts overflow as one row and can exceed itemInfo.stackSize (and also creates oversized single rows when quantity is large and no stack exists). That breaks stack invariants.
Suggested fix direction
- if (overflow > 0) {
- await client.insert(userItem).values({
- id: nanoid(),
- userId,
- itemId,
- quantity: overflow,
- equipped: "NONE",
- });
- }
+ let remaining = overflow;
+ while (remaining > 0) {
+ const chunk = Math.min(remaining, itemInfo.stackSize);
+ await client.insert(userItem).values({
+ id: nanoid(),
+ userId,
+ itemId,
+ quantity: chunk,
+ equipped: "NONE",
+ });
+ remaining -= chunk;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/server/api/routers/farming.ts` around lines 885 - 910, The code in
the awardItemToUser function does not properly respect stack size constraints
when inserting items. Both the overflow insertion (when overflow > 0) and the
new item insertion (when no existing item is found) can create oversized stacks
that exceed itemInfo.stackSize. Fix this by splitting both the overflow quantity
and the initial quantity parameter into multiple rows, each respecting the
itemInfo.stackSize limit. Instead of inserting overflow as a single row with the
calculated overflow quantity, create a loop that chunks the overflow into
stack-sized pieces and inserts each chunk as a separate row. Similarly, for the
case where no existing item is found, chunk the quantity parameter into
stack-sized pieces and insert each chunk as a separate row with quantity not
exceeding itemInfo.stackSize.
Comment on lines
+12
to
+13
| const midDay = new Date(dayStart.getTime() + 8 * 60 * 1000); | ||
| const midNight = new Date(dayStart.getTime() + 23 * 60 * 1000); |
Contributor
There was a problem hiding this comment.
Avoid hardcoded minute offsets in the cycle test.
Using fixed 8/23 minute offsets makes this test fragile if day/night constants are tuned. Derive both timestamps from the same constants used by production logic.
🧪 Proposed fix
import {
+ WORLD_DAY_SECONDS,
WORLD_CYCLE_SECONDS,
} from "`@/drizzle/constants`";
@@
const cycleMs = WORLD_CYCLE_SECONDS * 1000;
const dayStart = new Date(cycleMs * 5);
- const midDay = new Date(dayStart.getTime() + 8 * 60 * 1000);
- const midNight = new Date(dayStart.getTime() + 23 * 60 * 1000);
+ const dayMs = WORLD_DAY_SECONDS * 1000;
+ const nightMs = cycleMs - dayMs;
+ const midDay = new Date(dayStart.getTime() + dayMs / 2);
+ const midNight = new Date(dayStart.getTime() + dayMs + nightMs / 2);📝 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
| const midDay = new Date(dayStart.getTime() + 8 * 60 * 1000); | |
| const midNight = new Date(dayStart.getTime() + 23 * 60 * 1000); | |
| import { | |
| WORLD_DAY_SECONDS, | |
| WORLD_CYCLE_SECONDS, | |
| } from "`@/drizzle/constants`"; | |
| const cycleMs = WORLD_CYCLE_SECONDS * 1000; | |
| const dayStart = new Date(cycleMs * 5); | |
| const dayMs = WORLD_DAY_SECONDS * 1000; | |
| const nightMs = cycleMs - dayMs; | |
| const midDay = new Date(dayStart.getTime() + dayMs / 2); | |
| const midNight = new Date(dayStart.getTime() + dayMs + nightMs / 2); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/tests/libs/dayNight.test.ts` around lines 12 - 13, The test file
dayNight.test.ts uses hardcoded minute offsets (8 and 23) to create the midDay
and midNight timestamps, which makes the test brittle if the day/night cycle
constants are adjusted in production code. Replace the hardcoded offsets in the
midDay and midNight variable assignments with values derived from the same
constants that the production logic uses for the day/night cycle timing,
ensuring the test stays synchronized with any future changes to those constants.
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.
Pull Request
Please fill: what was implemented, why, possibly include screenshots, are any of the changes breaking, etc.
License
By making this pull request, I confirm that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the Studie-Tech ApS organization has the copyright to use and modify my contribution for perpetuity.
Summary by CodeRabbit