-
Notifications
You must be signed in to change notification settings - Fork 6
Add ovenpheus #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ovenpheus #88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' }); |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()); |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(), |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {}; |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.