Fix/code review 16

[cvega]

@copilot

[Copilot]

Pull request overview

This PR improves the security posture and runtime robustness of the GitHub Migrate SvelteKit app, while also refactoring shared UI utilities and optimizing parts of the backend persistence/SSE pipeline.

Changes:

• Add CSP configuration (moved from manual header) and harden auth rate-limiting IP handling.
• Improve SSE resiliency by adding id: fields and supporting Last-Event-ID / ?after= replay.
• Reduce memory usage and improve DB/query efficiency (stream archive uploads from disk, add indexes, aggregate batch list queries, safer JSON parsing), plus shared UI refactors (typed context keys, shared formatElapsed).

Reviewed changes

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

Show a summary per file

File	Description
svelte.config.js	Adds SvelteKit-managed CSP directives.
src/hooks.server.ts	Hardens auth IP handling, rate-limit map eviction, and moves CSP handling to kit.csp; base64 decode via Buffer.
src/routes/api/migrations/[id]/events/+server.ts	Adds Last-Event-ID support and emits id: lines during replay.
src/routes/api/migrations/+server.ts	Uses narrowBody to avoid double-casts for validated request bodies.
src/routes/api/batches/+server.ts	Same narrowBody refactor for batch POST body.
src/routes/[id]/+page.svelte	Fixes SSE buffer wrap handling by tracking last processed event id; uses shared formatter.
src/routes/batches/[id]/+page.svelte	Uses shared formatter instead of local implementation.
src/routes/+page.svelte	Uses typed Symbol-based Svelte context key for GitHub status.
src/routes/+layout.svelte	Sets typed Symbol-based context for GitHub status.
src/lib/stores/migrations.svelte.ts	Ensures debounce timer is cleared on global SSE destroy.
src/lib/server/validate.ts	Adds narrowBody<T>() helper to centralize the cast after validation.
src/lib/server/upload.ts	Allows Blob/Uint8Array to enable disk streaming and multipart slicing.
src/lib/server/store.ts	Adds safe JSON parsing, batch list aggregation query, and returns inserted event ids for SSE.
src/lib/server/schema.ts	Adds DB indexes for migration state and batch_id.
src/lib/server/monitor.ts	Allows passing pre-fetched source counts to avoid redundant API calls.
src/lib/server/migration.ts	Streams archive upload inputs from disk; improves cancellation semantics; passes sourceCounts to monitor; adjusts resume signature.
src/lib/server/manager.ts	DRYs emit/pipeline handlers; includes id: in SSE broadcast; uses aggregated batch pagination.
src/lib/format.ts	New shared formatElapsed() utility for frontend/server.
src/lib/context-keys.ts	New typed Symbol-based Svelte context keys.
src/lib/components/MigrationCard.svelte	Uses shared formatter with custom fallback.
src/lib/components/FailureDetail.svelte	Uses shared formatter.
README.md	Major documentation refresh and expanded troubleshooting.

Comments suppressed due to low confidence (2)

src/lib/server/migration.ts:566

• As with runMigrationPipeline, skipping failure emission on cancellation can leave SSE consumers without a terminal event and cause unnecessary reconnect loops / stale UI unless something else refreshes state. Consider emitting an explicit cancellation terminal event (or otherwise ensuring SSE clients can stop and refresh when cancelled).

    // Only emit failure events for genuine errors, not user cancellations.
    if (migration.state !== "cancelled") {
      emit({
        migrationId,
        eventType: "failure",

src/lib/server/migration.ts:201

• runMonitor() returns a Phase string (e.g. "UNKNOWN" when exiting due to an abort), so this if (!terminalPhase) guard will never trigger. If the AbortSignal is set during an in-flight poll, runMonitor() can return "UNKNOWN" and the pipeline will continue and mark the migration as succeeded. Handle this explicitly (e.g., treat signal.aborted or terminalPhase === "UNKNOWN" as cancellation/error, or change runMonitor() to return Phase | null/throw on abort).

    // If the signal was aborted during poll() (not during sleep — which throws),
    // the monitor exits cleanly with null. Re-throw so the catch block handles
    // cancellation properly instead of falling through to "succeeded".
    if (!terminalPhase) {
      throw new Error(
        opts.signal?.aborted ? "Migration cancelled" : "Monitor exited without terminal state",
      );
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This catch block skips emitting any terminal event when a migration is cancelled. Because the SSE client stops reconnecting only on complete/failure events, cancellations can leave clients connected/retrying indefinitely and the UI may not refresh from SSE alone. Consider emitting a dedicated terminal event for cancellation (and teaching the client to treat it as terminal), or emitting a final failure/complete-like event that represents cancellation.

This issue also appears on line 562 of the same file.

Suggested change

	// Only emit a failure event for genuine errors, not user cancellations.
	if (migration.state !== "cancelled") {
	emit({
	migrationId,
	// Always emit a terminal event so SSE clients can stop reconnecting.
	if (migration.state === "cancelled") {
	emit({
	migrationId,
	eventType: "cancelled",
	phase: "CANCELLED",
	payload: { reason: "Migration cancelled by user" },
	createdAt: new Date().toISOString(),
	});
	} else {
	emit({
	migrationId,

[Copilot]

Same issue as above: if (!terminalPhase) will never fire because runMonitor() returns a non-empty Phase string. If signal is aborted during a poll, runMonitor() can return "UNKNOWN" and this code will incorrectly proceed to mark the migration as succeeded. Handle abort/"UNKNOWN" explicitly or update runMonitor() to return null/throw when it exits without a terminal phase.

This issue also appears on line 194 of the same file.

[Copilot]

insertEvent() now performs an extra SELECT last_insert_rowid() per event. That doubles DB round-trips on high-frequency event streams. If possible, capture the inserted row id from the insert statement itself (e.g., use the .run() return value if it exposes the last insert rowid, or use an INSERT ... RETURNING id pattern) to avoid the additional query.

See below for a potential fix:

  const result = db
    .prepare(
      `INSERT INTO events (migration_id, event_type, phase, payload, created_at)
			 VALUES (?, ?, ?, ?, ?)`,
    )
    .run(
      event.migrationId,
      event.eventType,
      event.phase,
      JSON.stringify(event.payload),
      event.createdAt,
    );
  // Return the auto-increment row ID so callers can include it in SSE id: fields.
  return result.lastInsertRowid as number;