refactor(ui): extract shared IconButton from duplicated .icon-btn copies #15
Reference in New Issue
Block a user
Delete Branch "refactor/shared-icon-button"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Five files hand-rolled a near-identical 32px
.icon-btn(same size, hover, and primary/danger variants). Extract a single IconButton.svelte so the treatment lives in one place.Changes
IconButton.svelte—variantprop (plain/primary/danger) + spreads any button attributes (onclick, disabled, aria-label, title, data-testid) straight through;type="button"defaults but a caller can override..icon-btnCSS.Net -119 lines. The rendered button keeps the same class, styles, and DOM position, so layout and behaviour are unchanged — no version bump.
Deferred (intentional)
+layout) and home search button are 36px / different radius — size outliers that need a size/radius prop before folding in.profile/history's copy is removed in #13 (moved intoHistoryList); touching it here would just conflict.A component (not a global class) avoids colliding with those remaining local
.icon-btndefinitions.Testing
6-case component test written first (TDD); 356/356 vitest pass; svelte-check clean (no unused selectors left behind). e2e
upload.spec.ts(7) andmanga-edit.spec.ts(2) green — exercises the converted add/remove buttons and their disabled states.Follow-ups
Once #12 lands: fold the 36px outliers in via a
sizeprop, and apply the--tap-mintouch floor to the one shared component.🤖 Generated with Claude Code
Two high findings sharing one root cause: long-running daemon work that ignored the shutdown signal until it finished naturally. **Cron tick (crawler).** `CronContext::run_tick` ran the metadata-pass body inside `AssertUnwindSafe(...).catch_unwind().await` with no cancellation. A fresh-catalog walk (many minutes) wedged `DaemonHandle::shutdown`, `Supervisors::reload_crawler` (under the supervisor lock), and SIGTERM responsiveness for the whole pass. Wrap the body in `tokio::select! { biased; _ = self.cancel.cancelled() => ...; r = body => ... }`. On cancel the body future drops, which cascades to dropping `metadata.run()` — exactly what gives Chromium/lease teardown a chance via the BrowserManager idle reaper. The advisory unlock + conn drop still run unconditionally. **Analysis dispatch.** `process_lease` wrapped the vision call only in `tokio::time::timeout(self.job_timeout, …)` — default 600s. Toggling analysis off in the dashboard (under the supervisor lock) or stopping the container blocked on whichever call was in flight. Same fix: `tokio::select!` against `self.cancel.cancelled()`. On cancel, `jobs::release` returns the lease to the queue without burning an attempt — next tick picks it up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>`VisionClient::analyze` ran `image::load_from_memory`, `DynamicImage::resize_exact(Triangle)`, and per-band JPEG encodes directly on the tokio task. Each call is hundreds of ms of pure CPU per page. With multiple workers, the runtime threads got starved — axum handlers, SSE streams, the crawler daemon's async timers, and other tasks sharing the runtime all stalled while analysis was active. Extract a `prepare_analysis` helper that does ALL the CPU work (decode + plan + width-reduce + slice + JPEG encode) and returns a `PreparedAnalysis { Single | Sliced | Undecodable }` of already-encoded byte payloads. `analyze` calls it inside `tokio::task::spawn_blocking`, then the async HTTP loop only iterates over the prepared bytes — no image-crate operations happen on the runtime any more. Single page → one spawn_blocking → one HTTP call. Tall page → one spawn_blocking → N OCR calls + 1 grounding call. Memory peak rises slightly for the Sliced path (all bands held encoded at the same time instead of one-at-a-time) — for a typical manga page split into 4 slices at ~200 KB each, that's ~600 KB of extra peak. Negligible vs. the runtime-starvation cost. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Two ship-blockers from the rereview: 1. Env var name mismatch: 0.87.12 renamed every operator surface to ANALYSIS_VISION_MODEL but the reader still called env::var on ANALYSIS_MODEL — every documented deploy ran analysis with an empty model. Rename the reader. 2. Compose regression test was substring-only and missed exactly the ANALYSIS_VISION_MODEL vs ANALYSIS_MODEL RHS mismatch from 0.87.12. Tightened to require each backend-consumed var either be in a small hardcoded allowlist (BIND_ADDRESS, STORAGE_DIR) or have its RHS interpolate `${KEY...}` with the same name. Mutation-confirmed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>0.87.14 followup. The 0.87.14 test used MissedTickBehavior::Burst — any post-prep yield replayed missed ticks and could fake the >= 2 threshold. Switch to Skip and assert on `ticks_post - ticks_pre >= 50`, which sits well above the inlined-prep ceiling (~5) and far below the spawn_blocking floor (~700+). Mutation-confirmed. Also corrects: prepare_analysis doc count ("five fallback paths" → 4). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>0.87.12 added a regression test for backend env wiring, but its BACKEND_CONSUMED list was hand-maintained and drifted: ~22 vars read by backend/src/ never made it into compose's backend.environment. An operator setting ANALYSIS_TEMPERATURE=0.3 in .env got a silent no-op — exactly the bug class the test was supposed to catch. Make .env.example the single source of truth: * docker_compose_wires_every_documented_backend_env_var now reads .env.example at test time, filters NOT_BACKEND_CONSUMED_IN_ENV_EXAMPLE (compose-side composed vars, frontend-only vars, sidecar vars), and requires every remaining key to be wired into compose with a matching ${KEY...} interpolation OR allowlisted via COMPOSE_RHS_EXEMPT. * every_backend_src_env_read_is_documented_or_allowlisted scans backend/src/ for env::var / env_bool / env_i64 / ... reads and requires each key to be in .env.example OR in INTERNAL_NOT_CONFIGURABLE with a per-entry rationale. Catches the silent-no-op bug from the other side. Wire the previously-missing env vars into docker-compose.yml's backend.environment (CRAWLER_DAEMON, CRAWLER_DAILY_AT, CRAWLER_TZ, CRAWLER_IDLE_TIMEOUT_S, CRAWLER_CHAPTER_WORKERS, CRAWLER_JOB_RETENTION_DAYS, CRAWL_METRICS_RETENTION_DAYS, CRAWLER_RATE_MS, CRAWLER_CDN_RATE_MS, CRAWLER_USER_AGENT, CRAWLER_PHPSESSID, CRAWLER_COOKIE_DOMAIN, CRAWLER_TOR_CONTROL_COOKIE_PATH, all ANALYSIS_* tuning knobs). Document each new var in .env.example with the same default value shown in compose's `${KEY:-default}` form. Allowlist internal/system vars (HOME = chromium cache dir fallback, CRAWLER_BROWSER_*, CRAWLER_SKIP_*, the multi-paragraph ANALYSIS_*_PROMPT seeds) with per-entry rationale. Mutation-tested both sides: adding `env::var("MANGALORD_FAKE_NEW_KNOB")` to main.rs fails the scanner; removing the new compose wiring fails the documented-var test with all 23 missing keys named. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>0.87.20 narrowed but did not close the force-analyze race: ack_done / ack_failed / renew matched only on (id, state='running'), so a worker A whose lease was released mid-flight (force-analyze, reclaim_orphaned) could still come back later — once a successor B re-leased the same id and put it back to state='running' — and clobber B's lease. A's stale result became the row's `done` payload; B's in-flight work was silently lost. The pre-existing test even documented this hole: "We can't make ack_done's lease_id distinguish A from B today". Add `lease_generation BIGINT NOT NULL DEFAULT 0` to crawler_jobs (migration 0032). It is bumped by every lease, release / release_unowned, and reclaim_orphaned, so lease identity is `(id, generation)` rather than just `id`. Each Lease struct carries its generation. ack_done / ack_failed / renew / release match on `(id, generation, state='running')` so a stale ack from a prior generation finds zero rows and falls back to the existing warn-and-skip branch. Split the release surface in two: * release(lease_id, lease_generation) — owner-aware path for workers (graceful shutdown, session-expired), matches the caller's specific lease. * release_unowned(lease_id) — id-only path for force-analyze and ops tools that drop a running lease without holding a Lease struct; unconditionally bumps generation so any pending ack from the in-flight original becomes a no-op. Force-analyze in repo::page_analysis now uses release_unowned; the test in api_admin_analysis still passes unchanged (it only asserts the steady-state outcome). The pre-existing `ack_done_no_ops_when_ lease_was_stolen` test is strengthened: it now mints both A and B leases, asserts their generations differ, and verifies A's stale ack does NOT clobber B's running row — the assertion the old test explicitly couldn't make. Five new tests in tests/crawler_jobs.rs pin every facet: * lease_assigns_strictly_increasing_generation_per_release * ack_done_from_dead_lease_after_release_unowned_is_a_no_op * ack_failed_from_dead_lease_after_release_unowned_is_a_no_op * renew_from_dead_lease_is_a_no_op * release_unowned_bumps_generation_even_when_lease_is_held Mutation-confirmed: relaxing the ack_done guard to `lease_generation >= $2` (bind 0) makes the race test fail with state=done — the exact prior-bug shape. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Pull request closed