Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces "Ovenpheus", a new feature that allows users to convert clay into bricks within the market system. The implementation adds a dedicated conversion page, database logging, and prominent market integration.
Key changes:
- New
/dashboard/market/ovenpheusroute for clay-to-brick conversion with real-time calculations - Database schema extension with
ovenpheus_logtable to track all conversion transactions - Market page enhancement displaying Ovenpheus promotional section with conversion rates
- Refactoring of brick conversion constants for consistency across the codebase
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/routes/dashboard/market/ovenpheus/+page.svelte |
New Svelte component implementing the clay conversion UI with dual input controls (number and range slider) |
src/routes/dashboard/market/ovenpheus/+page.server.ts |
Server-side handler for clay-to-brick conversion logic, validation, and database operations |
src/routes/dashboard/market/+page.svelte |
Updated to include Ovenpheus promotional card and refined market score display formatting |
src/routes/dashboard/market/MarketItem.svelte |
Improved rounding logic for displaying market score and brick requirements |
src/lib/server/db/schema.ts |
Added ovenpheus_log table schema for tracking conversion history |
src/lib/defs.ts |
Renamed BRICKS_PER_CLAY_CONVERTED constant to BRICKS_PER_HOUR_CONVERTED for consistency |
src/lib/assets/ovenpheus.png |
New image asset for the Ovenpheus character/feature |
drizzle/0024_bored_valeria_richards.sql |
Migration script creating the ovenpheus_log table |
drizzle/meta/0024_snapshot.json |
Database schema snapshot including the new table structure |
drizzle/meta/_journal.json |
Migration journal entry for the schema change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <h1 class="text-2xl font-bold">Do you want to fire {clay} clay into {bricks} bricks?</h1> | ||
| <p class="mb-2"> | ||
| {#if !data.user.hasBasePrinter} | ||
| Keep in mind that you'll need {BASE_PRINTER_CLAY} <em>clay</em> to get a base printer |
There was a problem hiding this comment.
Incomplete sentence - "Keep in mind that you'll need {BASE_PRINTER_CLAY} clay to get a base printer" is missing a period at the end. Add proper punctuation for grammatical correctness.
| Keep in mind that you'll need {BASE_PRINTER_CLAY} <em>clay</em> to get a base printer | |
| Keep in mind that you'll need {BASE_PRINTER_CLAY} <em>clay</em> to get a base printer. |
| const bricks = | ||
| (parsedClay / CLAY_PER_HOUR) * | ||
| (locals.user.hasBasePrinter ? BRICKS_PER_HOUR : BRICKS_PER_HOUR_CONVERTED); |
There was a problem hiding this comment.
The bricks calculation doesn't use Math.floor() on the server side, but it does on the client side (line 17 in +page.svelte). This could lead to a mismatch where the UI shows a different number of bricks than what's actually awarded. For consistency and to prevent users from receiving fractional bricks unintentionally, apply Math.floor() to the bricks calculation here as well.
| const bricks = | |
| (parsedClay / CLAY_PER_HOUR) * | |
| (locals.user.hasBasePrinter ? BRICKS_PER_HOUR : BRICKS_PER_HOUR_CONVERTED); | |
| const bricks = Math.floor( | |
| (parsedClay / CLAY_PER_HOUR) * | |
| (locals.user.hasBasePrinter ? BRICKS_PER_HOUR : BRICKS_PER_HOUR_CONVERTED) | |
| ); |
| {Math.ceil(item.minRequiredShopScore - userShopScore)} more market score needed | ||
| {:else if item.computedPrice > userBricks} | ||
| {item.computedPrice - userBricks} more bricks needed | ||
| {item.computedPrice - Math.floor(userBricks)} more bricks needed |
There was a problem hiding this comment.
Inconsistent rounding logic: Math.floor is applied to userBricks but not to item.computedPrice in the subtraction. If userBricks can be fractional, then the calculation could show misleading results. For example, if userBricks = 10.8 and computedPrice = 15.3, this shows "15.3 - 10 = 5.3" instead of the actual shortfall of 4.5. Consider applying Math.ceil to the entire difference to show how many whole bricks are needed: Math.ceil(item.computedPrice - userBricks).
| {item.computedPrice - Math.floor(userBricks)} more bricks needed | |
| {Math.ceil(item.computedPrice - userBricks)} more bricks needed |
| parseInt(clay.toString()) <= 0 || | ||
| parseInt(clay.toString()) > locals.user.clay | ||
| ) { | ||
| return error(400, { message: 'invalid clay' }); |
There was a problem hiding this comment.
Using error(400, ...) with an object is incorrect syntax for SvelteKit. The error function expects either just a status code, or a status code with a string message. This should be either throw error(400, 'invalid clay') or restructured to return a failure using the fail function from '@sveltejs/kit' for form actions. The current code will cause a runtime error.
| if ( | ||
| !clay || | ||
| !parseInt(clay.toString()) || | ||
| parseInt(clay.toString()) <= 0 || | ||
| parseInt(clay.toString()) > locals.user.clay | ||
| ) { | ||
| return error(400, { message: 'invalid clay' }); | ||
| } | ||
|
|
||
| const parsedClay = parseInt(clay.toString()); |
There was a problem hiding this comment.
The validation logic uses parseInt without checking for NaN properly. The condition !parseInt(clay.toString()) will be true for "0" (which is a valid input) because parseInt("0") evaluates to 0, and !0 is true. This means users cannot submit 0 clay, but the subsequent check parseInt(clay.toString()) <= 0 suggests 0 should be rejected anyway. However, the button is only disabled when clay === 0, creating an inconsistency. Consider using Number(clay) or ensuring the validation logic aligns with the UI disabled state.
| if ( | |
| !clay || | |
| !parseInt(clay.toString()) || | |
| parseInt(clay.toString()) <= 0 || | |
| parseInt(clay.toString()) > locals.user.clay | |
| ) { | |
| return error(400, { message: 'invalid clay' }); | |
| } | |
| const parsedClay = parseInt(clay.toString()); | |
| const parsedClay = clay !== null ? Number(clay.toString()) : NaN; | |
| if ( | |
| !clay || | |
| Number.isNaN(parsedClay) || | |
| parsedClay <= 0 || | |
| parsedClay > locals.user.clay | |
| ) { | |
| return error(400, { message: 'invalid clay' }); | |
| } |
| clay: real().notNull(), | ||
| bricksReceived: real().notNull(), | ||
|
|
||
| timestamp: timestamp().notNull().defaultNow(), |
There was a problem hiding this comment.
Trailing comma after the last field in the table definition. While this might work in some JavaScript environments, it's inconsistent with the style used in other table definitions in this file (see marketItem, t2Review, devlog) where the last field doesn't have a trailing comma. Remove the comma for consistency.
| timestamp: timestamp().notNull().defaultNow(), | |
| timestamp: timestamp().notNull().defaultNow() |
| throw error(500); | ||
| } | ||
|
|
||
| return {}; |
There was a problem hiding this comment.
The load function returns an empty object but the component expects data.user to be available. The load function should return the user data from locals.user to make it accessible in the component. Currently, the component will fail when trying to access data.user.clay, data.user.hasBasePrinter, etc.
| return {}; | |
| return { | |
| user: locals.user | |
| }; |
No description provided.