Add specs for modal, image, and data-table sort components

[fpigeonjr]

Description

Raises unit-test coverage for three src/ui-kit/components/ files called out in the parent coverage epic (#576) as the largest individual gaps: modal.component.ts, image.component.ts, and sort.directive.ts (data-table).

• src/ui-kit/components/modal/modal.spec.ts — un-skips the pre-existing describe.skip wrapper (dating to a 2018 Karma-flake workaround, Skip Broken Tests #164); those 3 tests already pass unmodified under the current Vitest harness. Adds 10 new specs covering previously-uncovered public behaviour: the openModal re-open guard, closeModal(false) (no emit), the closeEscape Escape-key branches (closeOnEscape true/false, non-Escape keys), setModalElementIds/modalElIds construction, the selectedType alert-class branch, and the set5082 Tab-focus-trap keydown listeners (forward/backward wrap and the single-focusable-element case), which were entirely uncovered before this PR.
• src/ui-kit/components/image/image.spec.ts — adds 5 new specs driving ngOnInit's wired-up RxJS streams through simulated DOM events: file-picker change → save-button click → fileChange emit; cancel-button click clearing tmp state; drag-and-drop (onDragEnter/onDragOver/onDropEvent) setting the tmp file only while in edit mode.
• src/ui-kit/components/data-table/sort.directive.spec.ts (new file) — SamSortDirective had no dedicated spec at all (only exercised indirectly via a fully commented-out data-table.spec.ts). Adds 12 specs against its public API: register/deregister, duplicate-id and missing-id errors, sort() activation and direction cycling (asc→desc→clear and desc→asc→clear), disableClear overrides at the directive vs. sortable level, and the getNextSortDirection no-sortable guard.

All new specs test through each component's/directive's public API (inputs, outputs, DOM events) — no private-method testing.

Coverage (line %, before → after, measured via test-app/coverage/coverage-summary.json):

File	Before	After
modal.component.ts	37.14%	99.04%
image.component.ts	45.61%	92.98%
sort.directive.ts	28.12%	100%

Global coverage-floor gate metrics all moved up (statements 53.56%→55.10%, branches 39.77%→41.39%, functions 50.31%→51.53%, lines 52.84%→54.43%). Per repo convention, coverage-floor.json is not touched in this PR — raising the ratchet floor is a separate, deliberate npm run coverage:bump commit.

Motivation and Context

Closes #632

Type of Change (Select One and Apply Label)

• Bug fix (non-breaking change which fixes an issue) → Apply bugfix label
• New feature (non-breaking change which adds functionality) → Apply enhancement label
• Breaking change (fix or feature that would cause existing functionality to change) → Apply breaking label
• Documentation / configuration update → Apply maintenance label

How to Test

1. npm ci && npm ci --prefix test-app
2. npm --prefix test-app test — runs the full spec suite via Vitest with coverage.
3. npm run coverage:check — verifies the coverage-floor gate still passes.
4. npm run lint && npm run format:check — no new lint/format issues introduced.

Expected result: Test Files 107 passed (107) / Tests 697 passed (697) (up from 106/667 on master), coverage-floor gate passes at statements 55.10% / branches 41.39% / functions 51.53% / lines 54.43% (all above the recorded floors), and modal.component.ts / image.component.ts / sort.directive.ts line coverage measured at 99.04% / 92.98% / 100% respectively in test-app/coverage/coverage-summary.json.

Screenshots (if appropriate)

N/A — test-only changes, no UI/behavioral changes.

Checklist

• Branch name follows convention (e.g. gh-<number>-<slug>)
• PR title starts with a verb in the imperative mood
• I have self-reviewed my own code
• format:check passes (npm run format:check)
• lint passes (npm run lint)
• build passes (cd test-app && npm run build)
• Tests pass and coverage is reported (cd test-app && npm test)
• If this change requires a documentation update, I have updated it accordingly
• If there are dependent changes, they have been merged and published in downstream modules

[Copilot]

Pull request overview

This PR increases unit-test coverage in the src/ui-kit/components/ area by adding/expanding Vitest specs for the modal and image components and adding a new spec suite for the data-table sort directive, aligning with the coverage epic goals.

Changes:

• Unskips and significantly expands SamModalComponent specs (open/close behavior, escape handling, id/type branches, focus trapping).
• Adds DOM-event-driven specs for SamImageComponent (file input change/save/cancel, drag-and-drop behaviors).
• Introduces a new spec file for SamSortDirective covering registration, sorting cycles, and disableClear behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/ui-kit/components/modal/modal.spec.ts	Unskips and expands modal specs; adds behavioral coverage for open/close, escape handling, and focus-trap logic.
src/ui-kit/components/image/image.spec.ts	Adds event-driven specs covering file selection, save/cancel flows, and drag/drop handling.
src/ui-kit/components/data-table/sort.directive.spec.ts	Adds a new unit test suite validating sort directive registration and direction-cycling behavior.

Suppressed comments (6)

src/ui-kit/components/modal/modal.spec.ts:180

• This KeyboardEvent initialization uses keyCode in the init dict, which is not reliably honored; the listener checks ev.keyCode === 9. Define keyCode/shiftKey explicitly so the focus-wrap assertion is meaningful.

      firstFocusEl.dispatchEvent(
        new KeyboardEvent("keydown", { keyCode: 9, shiftKey: true })
      );

src/ui-kit/components/modal/modal.spec.ts:186

• Same issue here: keyCode passed to the KeyboardEvent constructor is not reliably reflected on the event object, so the ev.keyCode === 9 branch may never run. Define keyCode/shiftKey explicitly.

      lastFocusEl.dispatchEvent(
        new KeyboardEvent("keydown", { keyCode: 9, shiftKey: false })
      );

src/ui-kit/components/modal/modal.spec.ts:226

• The "single focusable element" assertion depends on ev.keyCode === 9, but new KeyboardEvent(..., { keyCode: 9 }) often yields keyCode === 0. Use an Event with a defined keyCode so preventDefault() is actually exercised.

      const event = new KeyboardEvent("keydown", {
        keyCode: 9,
        shiftKey: false,
      });
      const preventDefaultSpy = vi.spyOn(event, "preventDefault");
      modalContentEl.dispatchEvent(event);

src/ui-kit/components/image/image.spec.ts:110

• This spec relies on setTimeout(10) to wait for FileReader/RxJS processing, which is nondeterministic. Mock readAsDataURL to fire onload immediately so the drop test doesn’t depend on timing.

    containerEl.dispatchEvent(dropEvent);

    await new Promise((resolve) => setTimeout(resolve, 10));

src/ui-kit/components/modal/modal.spec.ts:207

• KeyboardEvent init dict keyCode isn’t reliably supported; this can make the Shift+Tab branch in set5082() not run under jsdom/browsers. Define keyCode/shiftKey explicitly on a dispatched event.

      modalContentEl.dispatchEvent(
        new KeyboardEvent("keydown", { keyCode: 9, shiftKey: true })
      );

src/ui-kit/components/image/image.spec.ts:84

• This spec uses setTimeout(10) to wait for FileReader async completion, which is timing-dependent. Mock readAsDataURL to trigger onload synchronously and remove the sleep.

    fileInputEl.dispatchEvent(new Event("change"));

    await new Promise((resolve) => setTimeout(resolve, 10));
    expect(component.getFileName()).toBe("washington.png");

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.