Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughNew components Changes
Sequence DiagramsequenceDiagram
actor User
participant Modal as CharacterCreateModal
participant TagInput as TagInput Component
participant API as PocketBase API
participant Parent as Parent Page Component
User->>Modal: Opens modal (open = true)
User->>Modal: Fills name, description, origin
User->>TagInput: Enters and adds tags
User->>Modal: Selects image files
User->>Modal: Clicks Create button
activate Modal
Modal->>Modal: Validate required fields
alt Validation fails
Modal->>User: Display error message
else Validation passes
Modal->>Modal: Build FormData (name, description, origin, tags, images, owner)
Modal->>API: POST /characters (FormData)
activate API
API-->>Modal: Success response
deactivate API
Modal->>Modal: Reset form state & close dialog
Modal->>Parent: Trigger afterCreate callback
activate Parent
Parent->>Parent: Reload character list
deactivate Parent
end
deactivate Modal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the character and CP creation and editing workflows by introducing a reusable TagInput component, a CharacterCreateModal, and a toFormData utility for PocketBase integration. The reviewer identified a bug in the CP edit page where the wrong field name was used for images and noted a validation inconsistency for the character origin field. Recommendations were also made to improve error handling by safely accessing API error data and to enhance the user experience by using SvelteKit's goto function for client-side navigation instead of full page reloads.
| name, | ||
| description, | ||
| owner: record.owner, | ||
| pictures, |
There was a problem hiding this comment.
The field name in the PocketBase collection is likely images, not pictures. Using the wrong key will prevent the images from being updated. This is inconsistent with the creation logic and other edit pages.
images: pictures,
| const createCharacter = async () => { | ||
| errorText = ''; | ||
|
|
||
| if (!name.trim() || !description.trim() || !origin.trim()) { |
There was a problem hiding this comment.
The origin field is checked for presence in this validation, but it is not marked as required in the HTML input (line 100) and its label doesn't indicate it's mandatory. If origin is intended to be optional, it should be removed from this validation check to avoid blocking character creation when the field is empty.
if (!name.trim() || !description.trim()) {
| errorText = err.data.data?.message ?? 'Create failed. Please try again.'; | ||
|
|
||
| const firstKey = Object.keys(err.data.data)[0]; | ||
| if (firstKey) { | ||
| const friendlyMessage = `${firstKey}: ${err.data.data[firstKey].message}`; | ||
| console.error(friendlyMessage); | ||
| errorText = friendlyMessage; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Accessing err.data.data directly can cause a runtime error if err.data or err.data.data is undefined (e.g., in case of network errors or non-validation API errors). Additionally, PocketBase's top-level error message is typically in err.data.message. It's safer to use optional chaining and verify the existence of the validation data before calling Object.keys.
errorText = err.data?.message ?? 'Create failed. Please try again.';
const firstKey = err.data?.data ? Object.keys(err.data.data)[0] : null;
if (firstKey) {
const friendlyMessage = `${firstKey}: ${err.data.data[firstKey].message}`;
console.error(friendlyMessage);
errorText = friendlyMessage;
return;
}
| @@ -1,54 +1,54 @@ | |||
| <script lang="ts"> | |||
| import pb from '$lib/pocketbase'; | |||
| import { onMount } from 'svelte'; | |||
There was a problem hiding this comment.
Import goto from $app/navigation to enable client-side navigation, which is more efficient and provides a better user experience than full page reloads.
import { onMount } from 'svelte';
import { goto } from '$app/navigation';
| characters: [character1, character2] | ||
| }) | ||
| ); | ||
| window.location.pathname = '/explore'; |
There was a problem hiding this comment.
Using window.location.pathname causes a full page reload. Use SvelteKit's goto function for a faster, client-side transition.
goto('/explore');
| errorText = err.data.data?.message ?? 'Update failed. Please try again.'; | ||
|
|
||
| const firstKey = Object.keys(err.data.data)[0]; | ||
| if (firstKey) { | ||
| const friendlyMessage = `${firstKey}: ${err.data.data[firstKey].message}`; | ||
| console.error(friendlyMessage); | ||
| charErrorText = friendlyMessage; | ||
| errorText = friendlyMessage; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Ensure safe access to err.data and err.data.data to prevent crashes when the error is not a validation error. Also, use err.data.message for the general error message fallback.
errorText = err.data?.message ?? 'Update failed. Please try again.';
const firstKey = err.data?.data ? Object.keys(err.data.data)[0] : null;
if (firstKey) {
const friendlyMessage = `${firstKey}: ${err.data.data[firstKey].message}`;
console.error(friendlyMessage);
errorText = friendlyMessage;
return;
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/src/routes/edit/cps/[id]/+page.svelte (1)
35-46:⚠️ Potential issue | 🟡 MinorGuard against missing
err.data.
err.data.data?.messagedereferenceserr.datawithout a guard; if the thrown error is not a PocketBaseClientResponseError(e.g. a network error),err.datawill beundefinedand this line throws, masking the original error. Same pattern repeats inwebsite/src/routes/edit/characters/[id]/+page.svelte,website/src/routes/create/+page.svelte, andwebsite/src/lib/components/CharacterCreateModal.svelte.- } catch (err: any) { - errorText = err.data.data?.message ?? 'Update failed. Please try again.'; - console.error(err.data); - - const firstKey = Object.keys(err.data.data)[0]; + } catch (err: any) { + errorText = err?.data?.data?.message ?? err?.message ?? 'Update failed. Please try again.'; + console.error(err); + + const firstKey = err?.data?.data ? Object.keys(err.data.data)[0] : undefined; if (firstKey) { const friendlyMessage = `${firstKey}: ${err.data.data[firstKey].message}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/src/routes/edit/cps/`[id]/+page.svelte around lines 35 - 46, The catch block currently assumes err.data exists which can throw for non-ClientResponseError errors; update the handler used in the catch blocks (where variables errorText, firstKey, friendlyMessage are set) to guard all accesses with optional chaining and fallbacks (e.g. read err?.data?.data?.message first, then err?.message or String(err)) and only call Object.keys when err?.data?.data is an object; apply the same defensive pattern to the other occurrences in the codebase (the similar catch blocks in the other +page.svelte and CharacterCreateModal.svelte) so you never dereference undefined and you preserve a useful fallback errorText.
🧹 Nitpick comments (1)
website/src/lib/utils/api.ts (1)
28-31: Consider handlingDateand object values explicitly.The
elsebranch usesvalue.toString(), which produces"[object Object]"for plain objects and a locale-formatted string forDate— usually not what a PocketBase field expects. If callers are always expected to pre-serialize such values this is fine, but handling (or at least rejecting) these explicitly would be safer as the helper sees more call sites.♻️ Optional refinement
- } else { - // 其他基本类型 (string, number) - formData.append(key, value.toString()); - } + } else if (value instanceof Date) { + formData.append(key, value.toISOString()); + } else if (typeof value === 'object') { + formData.append(key, JSON.stringify(value)); + } else { + // 其他基本类型 (string, number) + formData.append(key, value.toString()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/src/lib/utils/api.ts` around lines 28 - 31, The else branch currently calls value.toString() (seen where formData.append(key, value.toString()) is used) which turns plain objects into "[object Object]" and Dates into locale strings; update the FormData-building function in this module to handle Date and object values explicitly: if value instanceof Date call value.toISOString() before append, if typeof value === "object" use JSON.stringify(value) (or throw a clear error if you prefer callers to serialize), and keep primitives (string/number/boolean) as-is; ensure these checks replace the current fallback that calls value.toString().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/src/lib/components/CharacterCreateModal.svelte`:
- Around line 28-31: The create modal currently enforces origin as required
(checks !origin.trim() and sets errorText) but the edit page (+page.svelte for
edit/characters/[id]) does not — make behavior consistent: either remove origin
from the required-check in CharacterCreateModal.svelte (omit !origin.trim() from
the validation and update its label to show "(Optional)" if needed) or add the
same required validation and label change in the edit page (ensure the edit form
checks !origin.trim(), surfaces the same errorText, and updates the "Origin"
label to match the required state); reference the origin variable, errorText,
CharacterCreateModal.svelte submit/validation block and +page.svelte edit form
to implement the change.
- Around line 25-58: The modal's form state (name, description, origin,
pictures, tags, errorText) is only cleared on successful create inside
createCharacter, so closing the modal via Esc/Close leaves stale values; add a
reset-on-close guard: introduce a boolean like initialized, set initialized =
true when open becomes true, then watch open and when initialized && open
becomes false call a reset routine that clears name, description, origin,
pictures, tags, errorText (same reset steps used in the success branch) to avoid
wiping on initial mount; reference the open reactive prop and the
createCharacter reset logic to reuse the same clearing behavior.
In `@website/src/lib/components/ui/TagInput.svelte`:
- Around line 8-21: In addTag, make the max error message dynamic and clearable:
replace the hardcoded "ten" with the configured max (use max in the message),
and ensure errorText is cleared on any successful add (after pushing to tags) so
it doesn't linger; implement case-insensitive duplicate detection by normalizing
both currentInput and existing tags (e.g., compare lowercase versions) and when
a duplicate is detected set a user-visible errorText like "Tag already exists"
instead of silently dropping it; also clear errorText in your remove handler
(the tag removal function referenced in the file) when tags.length falls below
max or after a removal so the max error disappears.
In `@website/src/routes/create/`+page.svelte:
- Around line 66-73: The handler handleChangeSelect currently only mutates the
DOM value (target.value = '') when the user picks 'new', but the Svelte bound
state (character1/character2) remains 'new'; update the corresponding bound
variable instead of only the DOM: detect which bind was changed (use target.name
or a data attribute on the <select>) and set that variable (character1 or
character2) to '' before opening the dialog and returning, then set
isNewCharDialogOpen = true; this ensures the reactive state matches the cleared
select and prevents 'new' from being submitted.
In `@website/src/routes/edit/cps/`[id]/+page.svelte:
- Around line 20-23: The current validation only checks character1 ===
character2 which allows empty/undefined selections to slip through or falsely
trigger the "cannot be the same" message; update the submit/validation logic
(the block that sets errorText using character1 and character2) to first require
both character1 and character2 to be non-empty/defined (e.g. check for falsy or
explicit undefined/empty-string) and set an appropriate errorText if either is
missing, and only then check for equality and set "Character `#1` and Character `#2`
cannot be the same"; mirror the same validation flow used in
website/src/routes/create/+page.svelte so both non-empty and uniqueness are
enforced before allowing submission.
---
Outside diff comments:
In `@website/src/routes/edit/cps/`[id]/+page.svelte:
- Around line 35-46: The catch block currently assumes err.data exists which can
throw for non-ClientResponseError errors; update the handler used in the catch
blocks (where variables errorText, firstKey, friendlyMessage are set) to guard
all accesses with optional chaining and fallbacks (e.g. read
err?.data?.data?.message first, then err?.message or String(err)) and only call
Object.keys when err?.data?.data is an object; apply the same defensive pattern
to the other occurrences in the codebase (the similar catch blocks in the other
+page.svelte and CharacterCreateModal.svelte) so you never dereference undefined
and you preserve a useful fallback errorText.
---
Nitpick comments:
In `@website/src/lib/utils/api.ts`:
- Around line 28-31: The else branch currently calls value.toString() (seen
where formData.append(key, value.toString()) is used) which turns plain objects
into "[object Object]" and Dates into locale strings; update the
FormData-building function in this module to handle Date and object values
explicitly: if value instanceof Date call value.toISOString() before append, if
typeof value === "object" use JSON.stringify(value) (or throw a clear error if
you prefer callers to serialize), and keep primitives (string/number/boolean)
as-is; ensure these checks replace the current fallback that calls
value.toString().
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef3515ba-a446-4cfb-a43a-6660c30dac3d
📒 Files selected for processing (7)
website/src/lib/components/CharacterCreateModal.sveltewebsite/src/lib/components/ui/TagInput.sveltewebsite/src/lib/utils/api.tswebsite/src/routes/+layout.sveltewebsite/src/routes/create/+page.sveltewebsite/src/routes/edit/characters/[id]/+page.sveltewebsite/src/routes/edit/cps/[id]/+page.svelte
💤 Files with no reviewable changes (1)
- website/src/routes/+layout.svelte
| const createCharacter = async () => { | ||
| errorText = ''; | ||
|
|
||
| if (!name.trim() || !description.trim() || !origin.trim()) { | ||
| errorText = 'All fields must be filled out'; | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await pb | ||
| .collection('characters') | ||
| .create(toFormData({ name, description, origin, tag_names: tags, images: pictures, owner: pb.authStore.record!.id })); | ||
| name = ''; | ||
| description = ''; | ||
| origin = ''; | ||
| pictures = undefined; | ||
| tags = []; | ||
| errorText = ''; | ||
| open = false; | ||
| if (afterCreate) { | ||
| afterCreate(); | ||
| } | ||
| } catch (err: any) { | ||
| errorText = err.data.data?.message ?? 'Create failed. Please try again.'; | ||
|
|
||
| const firstKey = Object.keys(err.data.data)[0]; | ||
| if (firstKey) { | ||
| const friendlyMessage = `${firstKey}: ${err.data.data[firstKey].message}`; | ||
| console.error(friendlyMessage); | ||
| errorText = friendlyMessage; | ||
| return; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Form state isn't reset when the modal is dismissed without submitting.
Fields (name, description, origin, pictures, tags, errorText) are only cleared in the success branch. If the user opens the modal, types values, and then closes it via Esc or the Close button (line 128-132), the state persists into the next open. Consider resetting when open transitions to false:
$effect(() => {
if (open) {
dialogRef?.showModal();
} else {
dialogRef?.close();
+ name = '';
+ description = '';
+ origin = '';
+ pictures = undefined;
+ tags = [];
+ errorText = '';
}
});(Take care to avoid an initial-mount wipe if that matters — a let initialized = $state(false) guard works.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/src/lib/components/CharacterCreateModal.svelte` around lines 25 - 58,
The modal's form state (name, description, origin, pictures, tags, errorText) is
only cleared on successful create inside createCharacter, so closing the modal
via Esc/Close leaves stale values; add a reset-on-close guard: introduce a
boolean like initialized, set initialized = true when open becomes true, then
watch open and when initialized && open becomes false call a reset routine that
clears name, description, origin, pictures, tags, errorText (same reset steps
used in the success branch) to avoid wiping on initial mount; reference the open
reactive prop and the createCharacter reset logic to reuse the same clearing
behavior.
| function addTag(e: KeyboardEvent) { | ||
| if (e.key === 'Enter' && currentInput.trim()) { | ||
| e.preventDefault(); | ||
| if (tags.length >= max) { | ||
| errorText = 'Maximum of ten tags'; | ||
| return; | ||
| } | ||
| // 如果标签不存在则添加 | ||
| if (!tags.includes(currentInput.trim())) { | ||
| tags.push(currentInput.trim()); | ||
| } | ||
| currentInput = ''; // 清空输入 | ||
| } | ||
| } |
There was a problem hiding this comment.
A few small UX issues in addTag.
- The error message hardcodes "ten" even though
maxis configurable (e.g.,max=5would still say "Maximum of ten tags"). errorTextis set when the max is hit but never cleared — it will remain visible even after the user removes a tag (when the count is back belowmax), or after a subsequent successful add.- Duplicate detection is case-sensitive (
"Tag"and"tag"are both accepted) and silently drops duplicates without any feedback. Consider normalizing (e.g.,toLowerCase) and/or surfacing a "tag already exists" message.
🛠 Suggested fix
function addTag(e: KeyboardEvent) {
if (e.key === 'Enter' && currentInput.trim()) {
e.preventDefault();
if (tags.length >= max) {
- errorText = 'Maximum of ten tags';
+ errorText = `Maximum of ${max} tags`;
return;
}
- // 如果标签不存在则添加
- if (!tags.includes(currentInput.trim())) {
- tags.push(currentInput.trim());
- }
- currentInput = ''; // 清空输入
+ const next = currentInput.trim();
+ if (!tags.includes(next)) {
+ tags.push(next);
+ errorText = '';
+ }
+ currentInput = '';
}
}You may also want to clear errorText inside the remove handler on line 39.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/src/lib/components/ui/TagInput.svelte` around lines 8 - 21, In
addTag, make the max error message dynamic and clearable: replace the hardcoded
"ten" with the configured max (use max in the message), and ensure errorText is
cleared on any successful add (after pushing to tags) so it doesn't linger;
implement case-insensitive duplicate detection by normalizing both currentInput
and existing tags (e.g., compare lowercase versions) and when a duplicate is
detected set a user-visible errorText like "Tag already exists" instead of
silently dropping it; also clear errorText in your remove handler (the tag
removal function referenced in the file) when tags.length falls below max or
after a removal so the max error disappears.
| const handleChangeSelect = (e: Event) => { | ||
| const target = e.target as HTMLSelectElement; | ||
| if (target.value === 'new') { | ||
| target.value = ''; | ||
| newCharDialog.showModal(); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| // New Char Dialog | ||
| let newCharTags = $state<string[]>([]); | ||
| let newCharTagsCurrentInput = $state(''); | ||
| let newCharErrorText = $state(''); | ||
|
|
||
| function newCharAddTag(e: KeyboardEvent) { | ||
| if (e.key === 'Enter' && newCharTagsCurrentInput.trim()) { | ||
| e.preventDefault(); | ||
| if (newCharTags.length >= 10) { | ||
| newCharErrorText = 'Maximum of ten tags'; | ||
| return; | ||
| } | ||
| // 如果标签不存在则添加 | ||
| if (!newCharTags.includes(newCharTagsCurrentInput.trim())) { | ||
| newCharTags.push(newCharTagsCurrentInput.trim()); | ||
| } | ||
| newCharTagsCurrentInput = ''; // 清空输入 | ||
| } | ||
| } | ||
|
|
||
| let newCharName = $state(''); | ||
| let newCharDescription = $state(''); | ||
| let newCharOrigin = $state(''); | ||
| let newCharPictures: FileList | undefined = $state(); | ||
|
|
||
| const createCharacter = async () => { | ||
| const formData = new FormData(); | ||
| newCharErrorText = ''; | ||
|
|
||
| if (!newCharName.trim() || !newCharDescription.trim() || !newCharOrigin.trim()) { | ||
| newCharErrorText = 'All fields must be filled out'; | ||
| isNewCharDialogOpen = true; | ||
| return; | ||
| } | ||
|
|
||
| if (newCharPictures && newCharPictures.length !== 0) { | ||
| for (let file of newCharPictures) { | ||
| formData.append('images', file); | ||
| } | ||
| } | ||
|
|
||
| formData.append('name', newCharName); | ||
| formData.append('description', newCharDescription); | ||
| formData.append('origin', newCharOrigin); | ||
| newCharTags.forEach((tag) => { | ||
| formData.append('tag_names', tag); | ||
| }); | ||
|
|
||
| formData.append('owner', pb.authStore.record!.id); | ||
|
|
||
| try { | ||
| await pb.collection('characters').create(formData); | ||
| newCharDialog.close(); | ||
| newCharName = ''; | ||
| newCharDescription = ''; | ||
| newCharOrigin = ''; | ||
| newCharPictures = undefined; | ||
| newCharTags = []; | ||
| newCharTagsCurrentInput = ''; | ||
| newCharErrorText = ''; | ||
| reloadCharacters(); | ||
| } catch (err: any) { | ||
| newCharErrorText = err.data.data?.message ?? 'Create failed. Please try again.'; | ||
|
|
||
| const firstKey = Object.keys(err.data.data)[0]; | ||
| if (firstKey) { | ||
| const friendlyMessage = `${firstKey}: ${err.data.data[firstKey].message}`; | ||
| console.error(friendlyMessage); | ||
| newCharErrorText = friendlyMessage; | ||
| return; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Svelte 5 bind:value select programmatic value assignment change event
💡 Result:
In Svelte 5, using bind:value on a element creates a two-way binding between the select's value property and a reactive $state variable. Programmatically assigning a new value to the bound variable (e.g., selected = newValue) updates the select's displayed/selected option due to Svelte's reactivity system. However, this programmatic update does not trigger the DOM change event (or on:change handler). Svelte updates the element's .value property directly, which browsers do not dispatch events for—consistent with native DOM behavior. Key points: - User selection → updates bound variable + fires on:change - Programmatic variable change → updates select UI but no on:change - To simulate a change event after programmatic update, use selectEl.dispatchEvent(new Event('change')) where selectEl is obtained via bind:this. Example: <script> let selected = $state('option1'); let selectEl; function updateProgrammatically { selected = 'option2'; // Updates UI, no change event selectEl?.dispatchEvent(new Event('change')); // Manually trigger } </script> <select bind:this={selectEl} bind:value={selected} on:change={ => console.log('changed', selected)}> Option 1 Option 2 Update This behavior is unchanged from prior Svelte versions and documented implicitly via binding mechanics (Svelte sets .value directly).
Citations:
- 1: https://learn.svelte.dev/docs/svelte/bind
- 2: http://svelte.dev/docs/svelte/bind
- 3: https://svelte.dev/docs/svelte/bind
- 4: Setting the value of an input doesn't update value bindings sveltejs/svelte#14865
- 5: DOM input event NOT fired in Svelte's two-way binding (
bind:value) sveltejs/svelte#5783 - 6: Best Practice re: dispatchEvent input & change when bind:value variable updates sveltejs/svelte#5379
🏁 Script executed:
cat -n website/src/routes/create/+page.svelte | head -100Repository: pdnode-team/cp
Length of output: 3359
🏁 Script executed:
cat -n website/src/routes/create/+page.svelte | sed -n '100,150p'Repository: pdnode-team/cp
Length of output: 1844
🏁 Script executed:
cat -n website/src/routes/create/+page.svelte | sed -n '150,200p'Repository: pdnode-team/cp
Length of output: 1717
Selecting "Create a new character" may leave 'new' in the bound state.
When the user selects the "Create a new character" option, bind:value synchronously updates character1 or character2 to 'new'. Setting target.value = '' afterwards mutates only the DOM but does not trigger the change event, leaving the reactive state holding 'new'. If the user opens the modal, dismisses it, and submits without reselecting, characters: [character1, character2] will contain 'new' and fail the form validation at line 28–29.
Proposed fix
const handleChangeSelect = (e: Event) => {
const target = e.target as HTMLSelectElement;
if (target.value === 'new') {
+ if (target.name === 'cpCharacter1') character1 = '';
+ else if (target.name === 'cpCharacter2') character2 = '';
target.value = '';
isNewCharDialogOpen = 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.
| const handleChangeSelect = (e: Event) => { | |
| const target = e.target as HTMLSelectElement; | |
| if (target.value === 'new') { | |
| target.value = ''; | |
| newCharDialog.showModal(); | |
| return; | |
| } | |
| }; | |
| // New Char Dialog | |
| let newCharTags = $state<string[]>([]); | |
| let newCharTagsCurrentInput = $state(''); | |
| let newCharErrorText = $state(''); | |
| function newCharAddTag(e: KeyboardEvent) { | |
| if (e.key === 'Enter' && newCharTagsCurrentInput.trim()) { | |
| e.preventDefault(); | |
| if (newCharTags.length >= 10) { | |
| newCharErrorText = 'Maximum of ten tags'; | |
| return; | |
| } | |
| // 如果标签不存在则添加 | |
| if (!newCharTags.includes(newCharTagsCurrentInput.trim())) { | |
| newCharTags.push(newCharTagsCurrentInput.trim()); | |
| } | |
| newCharTagsCurrentInput = ''; // 清空输入 | |
| } | |
| } | |
| let newCharName = $state(''); | |
| let newCharDescription = $state(''); | |
| let newCharOrigin = $state(''); | |
| let newCharPictures: FileList | undefined = $state(); | |
| const createCharacter = async () => { | |
| const formData = new FormData(); | |
| newCharErrorText = ''; | |
| if (!newCharName.trim() || !newCharDescription.trim() || !newCharOrigin.trim()) { | |
| newCharErrorText = 'All fields must be filled out'; | |
| isNewCharDialogOpen = true; | |
| return; | |
| } | |
| if (newCharPictures && newCharPictures.length !== 0) { | |
| for (let file of newCharPictures) { | |
| formData.append('images', file); | |
| } | |
| } | |
| formData.append('name', newCharName); | |
| formData.append('description', newCharDescription); | |
| formData.append('origin', newCharOrigin); | |
| newCharTags.forEach((tag) => { | |
| formData.append('tag_names', tag); | |
| }); | |
| formData.append('owner', pb.authStore.record!.id); | |
| try { | |
| await pb.collection('characters').create(formData); | |
| newCharDialog.close(); | |
| newCharName = ''; | |
| newCharDescription = ''; | |
| newCharOrigin = ''; | |
| newCharPictures = undefined; | |
| newCharTags = []; | |
| newCharTagsCurrentInput = ''; | |
| newCharErrorText = ''; | |
| reloadCharacters(); | |
| } catch (err: any) { | |
| newCharErrorText = err.data.data?.message ?? 'Create failed. Please try again.'; | |
| const firstKey = Object.keys(err.data.data)[0]; | |
| if (firstKey) { | |
| const friendlyMessage = `${firstKey}: ${err.data.data[firstKey].message}`; | |
| console.error(friendlyMessage); | |
| newCharErrorText = friendlyMessage; | |
| return; | |
| } | |
| } | |
| }; | |
| const handleChangeSelect = (e: Event) => { | |
| const target = e.target as HTMLSelectElement; | |
| if (target.value === 'new') { | |
| if (target.name === 'cpCharacter1') character1 = ''; | |
| else if (target.name === 'cpCharacter2') character2 = ''; | |
| target.value = ''; | |
| isNewCharDialogOpen = true; | |
| return; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/src/routes/create/`+page.svelte around lines 66 - 73, The handler
handleChangeSelect currently only mutates the DOM value (target.value = '') when
the user picks 'new', but the Svelte bound state (character1/character2) remains
'new'; update the corresponding bound variable instead of only the DOM: detect
which bind was changed (use target.name or a data attribute on the <select>) and
set that variable (character1 or character2) to '' before opening the dialog and
returning, then set isNewCharDialogOpen = true; this ensures the reactive state
matches the cleared select and prevents 'new' from being submitted.
| if (character1 === character2) { | ||
| errorText = 'Character #1 and Character #2 cannot be the same'; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Validation permits empty character selections.
If the record on load has fewer than two characters (e.g. record.characters[1] is undefined), both character1 and character2 can end up as ''/undefined and the equality check on line 20 will not trigger ('' !== undefined, or '' === '' triggers the wrong branch and displays "cannot be the same" when both are simply unset). Consider also requiring both to be non-empty before submitting, matching the validation in website/src/routes/create/+page.svelte:
- if (character1 === character2) {
+ if (!character1 || !character2) {
+ errorText = 'Both characters must be selected';
+ return;
+ }
+ if (character1 === character2) {
errorText = 'Character `#1` and Character `#2` cannot be the same';
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/src/routes/edit/cps/`[id]/+page.svelte around lines 20 - 23, The
current validation only checks character1 === character2 which allows
empty/undefined selections to slip through or falsely trigger the "cannot be the
same" message; update the submit/validation logic (the block that sets errorText
using character1 and character2) to first require both character1 and character2
to be non-empty/defined (e.g. check for falsy or explicit
undefined/empty-string) and set an appropriate errorText if either is missing,
and only then check for equality and set "Character `#1` and Character `#2` cannot
be the same"; mirror the same validation flow used in
website/src/routes/create/+page.svelte so both non-empty and uniqueness are
enforced before allowing submission.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements