Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 3, 2025

The required attribute on InputSelect components was non-functional, allowing form submission without selection.

Root Cause

The component declared element: HTMLSelectElement but never bound it, causing validation logic to fail silently. Additionally, pink-svelte's Input.Select uses type="hidden" inputs which don't participate in HTML5 constraint validation per spec.

Changes

  • Added validation layer: Introduced visually-hidden native <select> element that participates in HTML5 validation
  • Bound element reference: Used bind:this={element} on the hidden select so setCustomValidity() calls execute
  • Maintained accessibility: Hidden select has aria-hidden="true" and tabindex="-1" to prevent interaction
  • Value synchronization: Both selects share bind:value to keep validation state aligned with UI
<!-- New: Validation layer -->
<select
    bind:this={element}
    {required}
    bind:value
    on:invalid={handleInvalid}
    tabindex="-1"
    aria-hidden="true"
    style="position: absolute; opacity: 0; pointer-events: none; width: 1px; height: 1px;">
    <option value="" disabled selected={!isNotEmpty(value)}></option>
    {#each options as option}
        <option value={option.value}>{option.label}</option>
    {/each}
</select>

<!-- Existing: UI layer -->
<Input.Select bind:value {options} ... />

Impact

Fixes validation for all InputSelect instances with required:

  • Organization plan changes (reported issue at line 454)
  • Domain record creation
  • Member role selection
  • Storage/messaging/auth settings
  • Database index creation

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • o1.ingest.sentry.io
    • Triggering command: node (vitest) (dns block)
    • Triggering command: node build.js (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

This

is supposed to be required but it's not


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@appwrite
Copy link

appwrite bot commented Nov 3, 2025

Console

Project ID: 688b7bf400350cbd60e9

Sites (1)
Site Status Logs Preview QR
 console-stage
688b7cf6003b1842c9dc
Ready Ready View Logs Preview URL QR Code

Tip

You can use Avatars API to generate QR code for any text or URLs.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 3, 2025

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI changed the title [WIP] Fix required field issue in organization change plan Fix InputSelect required attribute validation Nov 3, 2025
Copilot AI requested a review from stnguyen90 November 3, 2025 23:08
Copilot finished work on behalf of stnguyen90 November 3, 2025 23:08
</script>

<!-- Hidden select for HTML5 validation -->
<select
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot this doesnt seem right. The pink select input does have an input already so we shouldn't need another one.

See https://github.com/appwrite/pink/blob/next/v2/pink-sb/src/lib/input/Select.svelte

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that pink Select has an input element. However, it uses type="hidden" which doesn't participate in HTML5 constraint validation per spec - the browser skips validation for hidden inputs even with required attribute.

The original code tried to work around this by calling setCustomValidity() on an element variable, but that variable was never bound to anything.

Would you prefer I:

  1. Keep the dual-select approach (current fix)
  2. Submit a PR to pink-svelte to change the hidden input to a visually-hidden text input
  3. Remove HTML5 validation and use custom validation logic instead

What's your preference?

Copilot AI requested a review from stnguyen90 November 4, 2025 06:26
Copilot finished work on behalf of stnguyen90 November 4, 2025 06:26
@stnguyen90 stnguyen90 closed this Nov 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants