add Slack webhook UI and E2E tests

[neilv-g]

Also swapped the ordering of the notification channels on the /settings/notification-channels page.
Before, RSS feeds were listed before webhooks. Now those two are reversed.

[jcscottiii]

Overall, the implementation is solid. I've divided my feedback into Minimal Must-Fixes for this PR and a Long-term Vision for future work.

1. Minimal Must-Fixes (Required for PR 2333)

A. Remove Unsafe as Casting

The primary goal of this PR is type safety. Using as unknown as WebhookConfig undermines that objective. You can solve this minimally by using a Base Class + Type Guard:

Base Class (channel-config-types.ts):

export abstract class ChannelConfigForm extends LitElement {
  @property({type: Object}) abstract channel?: components['schemas']['NotificationChannelResponse'];
  abstract validate(): boolean;
}

Subclass (webhook-config-form.ts):

protected get config(): WebhookConfig | undefined {
  const config = this.channel?.config;
  // Safe narrowing - zero 'as' casting!
  return config?.type === 'webhook' ? config : undefined;
}

B. Replace Native confirm()

The use of confirm() in webstatus-notification-webhook-channels.ts is a UX regression. I recommend replacing it with a simple sl-dialog.

Minimal State Implementation:

@state() private _isDeleteDialogOpen = false;
private _channelToDelete?: Channel;

// In template:
html`<sl-dialog .open=${this._isDeleteDialogOpen} label="Delete Channel">...</sl-dialog>`

---

2. Long-Term Architectural Vision (After this PR)

They are not required for PR 2333, but I've documented them here and in an issue for future refactoring. But just so you are aware.

A. Direct Injection (Registry Inversion)

Instead of forcing every form to narrow its own config, the Registry (which already switches on type) can perform the narrowing once and pass a concrete .config property. This makes the forms truly "Dumb UI" and much more reusable.

B. Unified Modal State (State Machine)

Rather than multiple boolean flags (_isDeleteDialogOpen, _isManageDialogOpen), use a Discriminated Union for the component's state.

type UIState = 
  | { mode: 'none' }
  | { mode: 'delete'; channel: NotificationChannelResponse }
  | { mode: 'edit'; channel: NotificationChannelResponse };

C. Clean Switch-based Rendering

Combining the Unified State with a switch statement in render() eliminates brittle ternary operators and provides perfectly narrowed types in your template.

[jcscottiii]

Mostly nits to playwright tests. And then a note for some unit tests to help prevent regressions when we go for #2342 in the future.

Glad that the tests are working now. What did you change?

[jcscottiii]

Question: Did this make a difference. I wonder if we should add an optional argument to expectDualThemeScreenshot which will move that cursor for us automatically.

webstatus.dev/e2e/tests/utils.ts

Lines 37 to 53 in 7e8601e

	export async function expectDualThemeScreenshot(
	page: Page,
	locator: Locator | Page,
	name: string,
	options?: Parameters<Locator['screenshot']>[0],
	) {
	// 1. Ensure light theme and capture
	await forceTheme(page, 'light');
	await expect(locator).toHaveScreenshot(`${name}.png`, options);
	// 2. Change to dark theme and capture
	await forceTheme(page, 'dark');
	await expect(locator).toHaveScreenshot(`${name}-dark.png`, options);
	// 3. Reset to light for subsequent tests
	await forceTheme(page, 'light');
	}

Don't address that in this PR but something for the future if we see this happening a lot. Just FYI for the future.

[jcscottiii]

Same question and follow up as pointed out in https://github.com/GoogleChrome/webstatus.dev/pull/2333/changes#r2960189732

[jcscottiii]

Is there a reason for this increased timeout? If you're waiting for a response to complete, use page.waitForResponse instead before checking the dialog's visibility. This will help prevent flakiness in resource constrained environments like our CI.

[jcscottiii]

Here are places where we do it already: https://github.com/search?q=repo%3AGoogleChrome%2Fwebstatus.dev%20waitforresponse&type=code

[jcscottiii]

Same question and follow up as pointed out in https://github.com/GoogleChrome/webstatus.dev/pull/2333/changes#r2960189732

[jcscottiii]

Nit: try something like this when able.

Suggested change

	const deleteButton = updatedItem.locator('sl-button[aria-label="Delete"]');
	const deleteButton = updatedItem.getByRole('button', { name: 'Delete' });

[jcscottiii]

This should work instead using the preferred locator.

Suggested change

	const deleteDialog = webhookPanel.locator(
	'sl-dialog[label="Delete Webhook Channel"]',
	);
	const deleteDialog = webhookPanel.getByRole('dialog', { name: 'Delete Webhook Channel' });

[jcscottiii]

Suggested change

	const deleteDialog = webhookPanel.locator(
	'sl-dialog[label="Delete Webhook Channel"]',
	);
	const deleteDialog = webhookPanel.getByRole('dialog', { name: 'Delete Webhook Channel' });

[jcscottiii]

Suggested change

	// Exposed for testing/querying the internal form.
	get configForm(): ChannelConfigComponent {
	const form = this.renderRoot.querySelector('.config-form');
	return (form as ChannelConfigComponent) || this._configForm;
	}

I don't think we need this anymore since we use this._configForm (which uses @query). Right?

[jcscottiii]

Could you add some unit tests for this component?

Gemini suggested these.

Click to expand)

import {expect, fixture, html} from '@open-wc/testing';
import sinon from 'sinon';
import '../webstatus-manage-notification-channel-dialog.js';
import {ManageNotificationChannelDialog} from '../webstatus-manage-notification-channel-dialog.js';
import type {components} from 'webstatus.dev-backend';
import {SlButton, SlDialog} from '@shoelace-style/shoelace';

describe('webstatus-manage-notification-channel-dialog', () => {
  let element: ManageNotificationChannelDialog;

  const mockChannel: components['schemas']['NotificationChannelResponse'] = {
    id: 'test-channel-id',
    type: 'webhook',
    name: 'My Webhook',
    config: {type: 'webhook', url: 'https://hooks.slack.com/services/test'},
    status: 'enabled',
    created_at: new Date().toISOString(),
    updated_at: new Date().toISOString(),
  };

  beforeEach(async () => {
    element = await fixture<ManageNotificationChannelDialog>(html`
      <webstatus-manage-notification-channel-dialog></webstatus-manage-notification-channel-dialog>
    `);
  });

  it('renders correctly initially when closed', async () => {
    expect(element).to.be.instanceOf(ManageNotificationChannelDialog);
    expect(element.open).to.be.false;
    const dialog = element.shadowRoot?.querySelector('sl-dialog');
    expect(dialog?.hasAttribute('open')).to.be.false;
  });

  it('sets title and button text correctly for create mode', async () => {
    element.open = true;
    element.mode = 'create';
    element.type = 'webhook';
    await element.updateComplete;
    
    const dialog = element.shadowRoot?.querySelector('sl-dialog');
    expect(dialog?.label).to.equal('Create Webhook Channel');
    
    const saveBtn = element.shadowRoot?.querySelector('sl-button[variant="primary"]');
    expect(saveBtn?.textContent?.trim()).to.equal('Create');
  });

  it('sets title and button text correctly for edit mode', async () => {
    element.open = true;
    element.mode = 'edit';
    element.channel = mockChannel;
    await element.updateComplete;
    
    const dialog = element.shadowRoot?.querySelector('sl-dialog');
    expect(dialog?.label).to.equal('Edit Webhook Channel');
    
    const saveBtn = element.shadowRoot?.querySelector('sl-button[variant="primary"]');
    expect(saveBtn?.textContent?.trim()).to.equal('Save');
  });

  it('disables save button in edit mode if there are no pending updates', async () => {
    element.open = true;
    element.mode = 'edit';
    element.channel = mockChannel;
    await element.updateComplete;

    const saveBtn = element.shadowRoot?.querySelector<SlButton>('sl-button[variant="primary"]');
    expect(saveBtn?.disabled).to.be.true;

    // Simulate an update bubbling up from the internal config form registry
    element['_pendingUpdate'] = { updates: { name: 'New Name' }, mask: ['name'] } as any;
    await element.updateComplete;

    expect(saveBtn?.disabled).to.be.false;
  });

  it('never proactively disables create button based on pending updates', async () => {
    element.open = true;
    element.mode = 'create';
    await element.updateComplete;

    const saveBtn = element.shadowRoot?.querySelector<SlButton>('sl-button[variant="primary"]');
    expect(saveBtn?.disabled).to.be.false;
  });

  it('reflects loading state onto the primary action button', async () => {
    element.open = true;
    element.loading = true;
    await element.updateComplete;

    const saveBtn = element.shadowRoot?.querySelector<SlButton>('sl-button[variant="primary"]');
    expect(saveBtn?.loading).to.be.true;
  });

  it('emits sl-hide when the cancel button is clicked', async () => {
    const hideSpy = sinon.spy();
    element.addEventListener('sl-hide', hideSpy);

    const cancelBtn = element.shadowRoot?.querySelector<SlButton>('sl-button:not([variant="primary"])');
    cancelBtn?.click();
    
    expect(hideSpy).to.have.been.calledOnce;
  });

  it('clears pending updates internally when the dialog is closed', async () => {
    element.open = true;
    await element.updateComplete;

    element['_pendingUpdate'] = { updates: { name: 'Foo' }, mask: ['name'] } as any;
    
    element.open = false; // Trigger Lit Element lifecycle update
    await element.updateComplete;

    expect(element['_pendingUpdate']).to.be.undefined;
  });

  it('does not emit save event if the nested config form fails validation', async () => {
    element.open = true;
    await element.updateComplete;

    const saveSpy = sinon.spy();
    element.addEventListener('save', saveSpy);

    sinon.stub(element.configForm, 'validate').returns(false);

    const saveBtn = element.shadowRoot?.querySelector<SlButton>('sl-button[variant="primary"]');
    saveBtn?.click();

    expect(saveSpy).to.not.have.been.called;
  });

  it('emits a save event packed with state details when the form is valid', async () => {
    element.open = true;
    element.mode = 'edit';
    element.channel = mockChannel;
    await element.updateComplete;

    const mockUpdate = { updates: { name: 'Updated name' }, mask: ['name'] };
    element['_pendingUpdate'] = mockUpdate as any;

    const saveSpy = sinon.spy();
    element.addEventListener('save', saveSpy);
    
    sinon.stub(element.configForm, 'validate').returns(true);

    const saveBtn = element.shadowRoot?.querySelector<SlButton>('sl-button[variant="primary"]');
    saveBtn?.click();

    expect(saveSpy).to.have.been.calledOnce;
    expect(saveSpy.args[0][0].detail).to.deep.equal({
      mode: 'edit',
      channelId: 'test-channel-id',
      ...mockUpdate,
    });
  });
});

[jcscottiii]

Could you add a new unit test file for this too?

Gemini generated these. your mileage may vary if they work

(Click to expand)

import {expect, fixture, html} from '@open-wc/testing';
import sinon from 'sinon';
import '../webhook-config-form.js';
import {WebhookConfigForm} from '../webhook-config-form.js';
import type {components} from 'webstatus.dev-backend';
import {SlInput} from '@shoelace-style/shoelace';

describe('webhook-config-form', () => {
  let element: WebhookConfigForm;

  const mockChannel: components['schemas']['NotificationChannelResponse'] = {
    id: 'test-channel-id',
    type: 'webhook',
    name: 'Original Webhook Name',
    config: {type: 'webhook', url: 'https://hooks.slack.com/services/original'},
    status: 'enabled',
    created_at: new Date().toISOString(),
    updated_at: new Date().toISOString(),
  };

  describe('Create Mode (No Initial Channel)', () => {
    beforeEach(async () => {
      element = await fixture<WebhookConfigForm>(html`
        <webhook-config-form></webhook-config-form>
      `);
    });

    it('renders empty inputs initially', async () => {
      expect(element).to.be.instanceOf(WebhookConfigForm);
      const nameInput = element.shadowRoot?.querySelector<SlInput>('#webhook-name');
      const urlInput = element.shadowRoot?.querySelector<SlInput>('#webhook-url');
      expect(nameInput?.value).to.equal('');
      expect(urlInput?.value).to.equal('');
    });

    it('is initially not dirty', async () => {
      expect(element.isDirty()).to.be.false;
    });

    it('becomes dirty when inputs are typed into', async () => {
      const nameInput = element.shadowRoot?.querySelector<SlInput>('#webhook-name');
      nameInput!.value = 'New Name';
      nameInput?.dispatchEvent(new CustomEvent('sl-input'));
      await element.updateComplete;

      expect(element.isDirty()).to.be.true;
    });

    it('emits a change event with full payload when input occurs', async () => {
      const changeSpy = sinon.spy();
      element.addEventListener('change', changeSpy);

      const nameInput = element.shadowRoot?.querySelector<SlInput>('#webhook-name');
      nameInput!.value = 'New Name';
      nameInput?.dispatchEvent(new CustomEvent('sl-input'));
      await element.updateComplete;

      expect(changeSpy).to.have.been.calledOnce;
      const detail = changeSpy.args[0][0].detail;
      // In create mode, both name and config masks are always forced because !this.channel
      expect(detail.mask).to.deep.equal(['name', 'config']);
      expect(detail.updates).to.deep.equal({
        name: 'New Name',
        config: { type: 'webhook', url: '' } // URL hasn't been typed yet
      });
    });

    it('validates correctly by delegating to input elements', async () => {
      const nameInput = element.shadowRoot?.querySelector<SlInput>('#webhook-name');
      const urlInput = element.shadowRoot?.querySelector<SlInput>('#webhook-url');
      
      const nameStub = sinon.stub(nameInput!, 'reportValidity').returns(true);
      const urlStub = sinon.stub(urlInput!, 'reportValidity').returns(false);

      expect(element.validate()).to.be.false;
      expect(nameStub).to.have.been.calledOnce;
      expect(urlStub).to.have.been.calledOnce;
    });
  });

  describe('Edit Mode (With Initial Channel)', () => {
    beforeEach(async () => {
      element = await fixture<WebhookConfigForm>(html`
        <webhook-config-form .channel=${mockChannel}></webhook-config-form>
      `);
    });

    it('renders pre-filled inputs', async () => {
      const nameInput = element.shadowRoot?.querySelector<SlInput>('#webhook-name');
      const urlInput = element.shadowRoot?.querySelector<SlInput>('#webhook-url');
      expect(nameInput?.value).to.equal('Original Webhook Name');
      expect(urlInput?.value).to.equal('https://hooks.slack.com/services/original');
    });

    it('is initially not dirty', async () => {
      expect(element.isDirty()).to.be.false;
    });

    it('does not become dirty if typing the exact same value', async () => {
      const nameInput = element.shadowRoot?.querySelector<SlInput>('#webhook-name');
      nameInput!.value = 'Original Webhook Name';
      nameInput?.dispatchEvent(new CustomEvent('sl-input'));
      await element.updateComplete;

      expect(element.isDirty()).to.be.false;
    });

    it('only includes modified fields in the update payload mask', async () => {
      const changeSpy = sinon.spy();
      element.addEventListener('change', changeSpy);

      // We ONLY update the URL for an existing channel
      const urlInput = element.shadowRoot?.querySelector<SlInput>('#webhook-url');
      urlInput!.value = 'https://example.com/new-hook';
      urlInput?.dispatchEvent(new CustomEvent('sl-input'));
      await element.updateComplete;

      expect(changeSpy).to.have.been.calledOnce;
      const detail = changeSpy.args[0][0].detail;
      
      // Because we only typed in URL, name should NOT be in the mask
      expect(detail.mask).to.deep.equal(['config']);
      expect(detail.updates).to.deep.equal({
        config: { type: 'webhook', url: 'https://example.com/new-hook' }
      });
      expect(detail.updates.name).to.be.undefined;
    });
  });
});