-
Notifications
You must be signed in to change notification settings - Fork 7
Add Shop Page and Orders #14
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
base: main
Are you sure you want to change the base?
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 adds a shop feature where users can purchase items with their snowflake currency. The changes introduce an order processing system with shipping information collection, an admin interface for order management, and modify the user authorization model from a string-based role field to a boolean admin field.
Key changes:
- Order placement flow with form validation and shipping address collection
- Admin order management with reject/refund and shipping tracking capabilities
- User schema migration from
rolestring toadminboolean flag
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/shop/+page.svelte | Adds snowflake balance display and buy buttons with affordability checks |
| src/routes/shop/+page.server.ts | Adds snowflake count fetching to shop page load function |
| src/routes/shop/order/[itemId]/+page.svelte | New order form page for collecting shipping information |
| src/routes/shop/order/[itemId]/+page.server.ts | Handles order submission, snowflake deduction, and rollback logic |
| src/routes/admin/+page.svelte | New admin interface for viewing and managing shop orders |
| src/routes/admin/+page.server.ts | Admin order management actions (reject with/without refund, mark shipped) |
| src/routes/profile/+page.svelte | Updates snowflake display to use new data source |
| src/routes/profile/+page.server.ts | Adds snowflake count fetching alongside project data |
| src/routes/auth/callback/+server.ts | Changes user creation to use admin boolean instead of role string |
| src/lib/server/db.ts | Updates User interface to replace role with admin boolean |
| src/lib/server/airtable.ts | Adds utility functions for safe Airtable query construction |
| migrations/004_add_admin.js | Adds admin boolean column to users table |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| snowflakeRecordId | ||
| }; |
Copilot
AI
Jan 4, 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 snowflakeRecordId is loaded and returned but never used. While this might be for future use, currently it's unnecessary data exposure. If it's not needed by the frontend, consider removing it from the return value to minimize data exposure.
|
|
||
| <label> | ||
| <span>Phone Number *</span> | ||
| <input type="tel" name="phoneNumber" required /> |
Copilot
AI
Jan 4, 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 phone number input uses type="tel" but lacks validation pattern or format requirements. Phone numbers can contain arbitrary text, which could lead to data quality issues and make it difficult to contact customers. Consider adding a validation pattern or using a library to validate and format phone numbers according to international standards (E.164 format).
| <input type="tel" name="phoneNumber" required /> | |
| <input | |
| type="tel" | |
| name="phoneNumber" | |
| required | |
| pattern="^\+?[0-9\s\-().]{7,20}$" | |
| title="Enter a valid phone number, e.g. +1234567890" | |
| /> |
| const name = formData.get('name')?.toString().trim(); | ||
| const addressLine1 = formData.get('addressLine1')?.toString().trim(); | ||
| const addressLine2 = formData.get('addressLine2')?.toString().trim() || ''; | ||
| const city = formData.get('city')?.toString().trim(); | ||
| const state = formData.get('state')?.toString().trim(); | ||
| const country = formData.get('country')?.toString().trim(); | ||
| const zipCode = formData.get('zipCode')?.toString().trim(); | ||
| const phoneNumber = formData.get('phoneNumber')?.toString().trim(); | ||
| const notes = formData.get('notes')?.toString().trim() || ''; |
Copilot
AI
Jan 4, 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.
User-provided address and name fields are only trimmed but not sanitized or validated. Special characters, potentially malicious scripts, or extremely long strings could be stored in Airtable. Consider adding: 1) maximum length limits on inputs, 2) sanitization to remove or escape potentially problematic characters, 3) validation to ensure data contains expected character sets (e.g., alphanumerics, spaces, basic punctuation for addresses).
| if (snowflakeResponse.ok) { | ||
| const snowflakeData = await snowflakeResponse.json(); | ||
| const userRecord = snowflakeData.records?.find( | ||
| (r: { fields?: { Email?: string } }) => r.fields?.Email?.trim() === locals.user.email | ||
| ); | ||
| snowflakeCount = userRecord?.fields?.['Snowflake Count'] ?? 0; | ||
| } else { | ||
| console.error('Airtable snowflake count fetch failed:', snowflakeResponse.statusText); | ||
| } |
Copilot
AI
Jan 4, 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 snowflake count is fetched by retrieving ALL records from the Snowflake Count table and then filtering in JavaScript. This is inefficient and will not scale well as the number of users grows. Consider using Airtable's filterByFormula parameter to filter on the server side, similar to how it's done in the profile page (profile/+page.server.ts line 44 uses buildFilterFormula).
| const currentBalance = userRecord.fields?.['Snowflake Count'] ?? 0; | ||
|
|
||
| if (currentBalance < cost) { | ||
| return fail(400, { error: 'Not enough snowflakes' }); | ||
| } | ||
|
|
||
| const newBalance = currentBalance - cost; | ||
| const updateResponse = await fetch( | ||
| `https://api.airtable.com/v0/${AIRTABLE_BASE_ID}/Snowflake%20Count/${userRecord.id}`, | ||
| { | ||
| method: 'PATCH', | ||
| headers: { | ||
| Authorization: `Bearer ${AIRTABLE_API_KEY}`, | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify({ | ||
| fields: { | ||
| 'Snowflake Count': newBalance | ||
| } | ||
| }) | ||
| } | ||
| ); |
Copilot
AI
Jan 4, 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.
Race condition vulnerability in order processing. The balance check on line 158 and the balance update on lines 163-177 are not atomic. If a user submits multiple orders simultaneously, both requests could pass the balance check before either deducts the snowflakes, allowing the user to overspend their balance. Consider implementing optimistic locking or using a database transaction with row-level locks to ensure atomic read-modify-write operations.
| return knex.schema.alterTable('users', (table) => { | ||
| table.boolean('admin').defaultTo(false).notNullable(); | ||
| }); |
Copilot
AI
Jan 4, 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 migration adds the 'admin' column but doesn't remove the 'role' column that was defined in the initial migration (001_create_users.js line 17). This leaves both columns in the database, which could lead to confusion and data inconsistencies. Consider either: 1) dropping the 'role' column in this migration, or 2) migrating the data from 'role' to 'admin' (e.g., setting admin=true where role='admin') before dropping the old column.
| return knex.schema.alterTable('users', (table) => { | |
| table.boolean('admin').defaultTo(false).notNullable(); | |
| }); | |
| return knex.schema | |
| .alterTable('users', (table) => { | |
| table.boolean('admin').defaultTo(false).notNullable(); | |
| }) | |
| .then(() => | |
| knex('users') | |
| .where('role', 'admin') | |
| .update({ admin: true }) | |
| ) | |
| .then(() => | |
| knex.schema.alterTable('users', (table) => { | |
| table.dropColumn('role'); | |
| }) | |
| ); |
| <div class="modal-overlay" on:click={closeTrackingModal} on:keydown={(e) => e.key === 'Escape' && closeTrackingModal()} role="button" tabindex="0"> | ||
| <div class="modal" on:click|stopPropagation on:keydown|stopPropagation role="dialog" aria-modal="true" tabindex="0"> |
Copilot
AI
Jan 4, 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 modal overlay uses keyboard events without proper handler implementation. The keydown handler on line 102 uses a shorthand expression that doesn't prevent default behavior or stop propagation. For proper Escape key handling, this should be a full function that checks the key and calls the close function. Additionally, clicking on interactive elements inside the modal would trigger both the stopPropagation and the click handler, which could be confusing.
| <div class="modal-overlay" on:click={closeTrackingModal} on:keydown={(e) => e.key === 'Escape' && closeTrackingModal()} role="button" tabindex="0"> | |
| <div class="modal" on:click|stopPropagation on:keydown|stopPropagation role="dialog" aria-modal="true" tabindex="0"> | |
| <div | |
| class="modal-overlay" | |
| on:click={closeTrackingModal} | |
| on:keydown={(event) => { | |
| if (event.key === 'Escape' || event.key === 'Esc') { | |
| event.preventDefault(); | |
| event.stopPropagation(); | |
| closeTrackingModal(); | |
| } | |
| }} | |
| role="button" | |
| tabindex="0" | |
| > | |
| <div | |
| class="modal" | |
| on:click|stopPropagation | |
| on:keydown={(event) => { | |
| if (event.key === 'Escape' || event.key === 'Esc') { | |
| event.stopPropagation(); | |
| } | |
| }} | |
| role="dialog" | |
| aria-modal="true" | |
| tabindex="0" | |
| > |
| fetch(`https://api.airtable.com/v0/${AIRTABLE_BASE_ID}/Snowflake%20Count`, { | ||
| headers: { | ||
| Authorization: `Bearer ${AIRTABLE_API_KEY}`, | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| }) | ||
| ]); |
Copilot
AI
Jan 4, 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 snowflake count is fetched by retrieving ALL records from the Snowflake Count table and then filtering in JavaScript. This is inefficient and will not scale well as the number of users grows. Consider using Airtable's filterByFormula parameter to filter on the server side, similar to how it's done in the profile page (profile/+page.server.ts line 44 uses buildFilterFormula).
| const snowflakeResponse = await fetch( | ||
| `https://api.airtable.com/v0/${AIRTABLE_BASE_ID}/Snowflake%20Count`, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${AIRTABLE_API_KEY}`, | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| if (snowflakeResponse.ok) { | ||
| const snowflakeData = await snowflakeResponse.json(); | ||
| const userRecord = snowflakeData.records?.find( | ||
| (r: SnowflakeRecord) => r.fields?.Email?.trim() === userEmail | ||
| ); |
Copilot
AI
Jan 4, 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 refund operation fetches ALL records from the Snowflake Count table and then filters in JavaScript. This is inefficient and will not scale well as the number of users grows. Consider using Airtable's filterByFormula parameter to filter on the server side, similar to how it's done in the profile page (profile/+page.server.ts line 44 uses buildFilterFormula).
| await fetch( | ||
| `https://api.airtable.com/v0/${AIRTABLE_BASE_ID}/Snowflake%20Count/${userRecord.id}`, | ||
| { | ||
| method: 'PATCH', | ||
| headers: { | ||
| Authorization: `Bearer ${AIRTABLE_API_KEY}`, | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify({ | ||
| fields: { | ||
| 'Snowflake Count': currentBalance + refundAmount | ||
| } | ||
| }) | ||
| } | ||
| ); | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 4, 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 refund operation does not check if the snowflake update was successful before marking the order as rejected with refund. If the refund fails (e.g., due to network error or API failure), the order status will still be changed to 'Rejected With Refund', but the user won't actually receive their snowflakes back. This creates a financial discrepancy. Consider checking the response status and only updating the order status if the refund succeeds, or implement compensating transactions.
@jeninh