Conversation
in case there's an effects change which affects how spell stuff is calculated
cropping is just for avatar display
prevents incorrect level display on characters page
makes sure our ulids are as-often as-possible monotonically increasing
they're sortable, like dates, always indexed, and we at least try to make sure they're monotonically increasing (when generated together on the same worker, which is when we are likeliest to get collisions)
vs a boring table.
button was not working
mostly affects notes fields
There was a problem hiding this comment.
Pull request overview
This PR addresses bug fixes and refactoring across the codebase with a focus on improving consistency and fixing ULID-based sorting issues. The main changes include:
Purpose: Fix multiple bugs related to sorting, form IDs, item state updates, and avatar cropping. Introduces item effects tools for the AI assistant (Reed) and improves the character list UI.
Key Changes:
- Centralized ULID generation to
@src/lib/idsfor consistent monotonic ID generation - Fixed avatar cropper to properly handle image padding in both dimensions
- Updated database queries to sort by
id(ULID) instead ofcreated_atfor reliable chronological ordering - Added AI tools for creating and deleting item effects
- Refactored character list from table to card-based grid with avatar display
- Standardized form patterns using new
ModalFormcomponent
Reviewed changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
static/avatar-cropper.js |
Fixed vertical padding calculations for proper crop coordinate conversion |
src/lib/ids.ts |
Centralized monotonic ULID factory |
src/db/*.ts |
Updated imports to use centralized ULID, replaced created_at sorting with id sorting |
src/services/listCharacters.ts |
Added avatar data to character list with proper ULID-based sorting |
src/services/*ItemEffect.ts |
Added AI tool wrappers for item effect management |
src/tools.ts |
Registered new item effect tools |
src/routes/character.tsx |
Added missing SpellsPanel OOB swaps for item state changes |
src/components/ModalForm.tsx |
New reusable component for consistent form patterns |
src/components/*Form.tsx |
Refactored to use ModalForm with consistent ID naming |
src/components/Characters.tsx |
Converted table layout to card grid with avatars |
src/test/factories/item.ts |
New factory for test item creation |
src/routes/character.test.ts |
Updated test assertion for title attribute |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const cropX = cropForm.querySelector('input[name="crop_x_percent"]'); | ||
| const cropY = cropForm.querySelector('input[name="crop_y_percent"]'); | ||
| const cropW = cropForm.querySelector('input[name="crop_width_percent"]'); | ||
| const cropH = cropForm.querySelector('input[name="crop_height_percent"]'); |
There was a problem hiding this comment.
The form field IDs have been updated to use hyphens (e.g., crop-x-percent), but the JavaScript code still references the old underscore names (e.g., crop_x_percent). The querySelector calls will fail to find these elements.
Update the JavaScript to use the new hyphenated IDs:
const cropX = cropForm.querySelector('input[id="crop-x-percent"]');
const cropY = cropForm.querySelector('input[id="crop-y-percent"]');
const cropW = cropForm.querySelector('input[id="crop-width-percent"]');
const cropH = cropForm.querySelector('input[id="crop-height-percent"]');we prefer `-` to `_`. we fix some mismatches between labels and elements
if it exceeds them, we snap back to the bounds, not just prevent the event default.
d0ec337 to
3acc4f3
Compare
allows us to manage the common htmx attributes in one spot
we try to make submit take precedence over validation, though we don't always succeed
3acc4f3 to
2e63121
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 61 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/components/CharacterImport.tsx:157
- Missing
idattribute on the input element. The label referencescharimport-ability-${ability}but the input doesn't have anyidattribute set, breaking the label-input association.
Add id={charimport-ability-${ability}} to the input element.
<div class="input-group">
<input
type="number"
class={clsx("form-control", { "is-invalid": hasError })}
id={`charimport-ability-${ability}`}
name={`ability_${ability}`}
value={score}
min="3"
max="20"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <label class="form-check-label small" for={`edititem-damage-versatile-${i}`}> | ||
| Versatile |
There was a problem hiding this comment.
The label's for attribute references damage-versatile-${i} but the input's id is edititem-damage-versatile-${i}. This mismatch will break the label-input association.
Update the label's for attribute to match: for={edititem-damage-versatile-${i}}
| data-existingx={existingCrop.x ?? null} | ||
| data-existingy={existingCrop.y ?? null} | ||
| data-existingw={existingCrop.width ?? null} | ||
| data-existingh={existingCrop.height ?? null} |
There was a problem hiding this comment.
The data-existing* attributes are set to null when values are not present, but JavaScript's dataset API converts null to the string "null". This means the condition existingx != null on line 136 of avatar-cropper.js will always be true (even when attributes are "null"), breaking the logic.
Change ?? null to ?? undefined for all four data attributes, as undefined won't render the attribute at all, which is the correct behavior.
| data-existingx={existingCrop.x ?? null} | |
| data-existingy={existingCrop.y ?? null} | |
| data-existingw={existingCrop.width ?? null} | |
| data-existingh={existingCrop.height ?? null} | |
| data-existingx={existingCrop.x ?? undefined} | |
| data-existingy={existingCrop.y ?? undefined} | |
| data-existingw={existingCrop.width ?? undefined} | |
| data-existingh={existingCrop.height ?? undefined} |
mostly bug fixes:
we do have two new features: