Skip to content

Latest commit

 

History

History
48 lines (38 loc) · 4.19 KB

File metadata and controls

48 lines (38 loc) · 4.19 KB

Tenki Code Review (2025-12-21)

High priority findings

  • Uses http for the WeatherAPI call, leaking keys and allowing tampering. Switch to https in the request URL (main.ts).
  • Refresh interval is stored in seconds but presented/parsed as minutes, so the default 30 becomes 0.5 minutes in the UI and refreshes every 30 seconds instead of the documented 30 minutes (main.ts, main.ts, README.md).
  • Multiple timers can run concurrently: initView, onOpen, and refreshWeather each schedule setInterval without canceling a prior one, and intervals are not registered with the plugin for automatic cleanup (main.ts, main.ts).
  • clearUpdateInterval relies on a truthy check and initializes updateInterval to -1; clearInterval(-1) is a no-op and leaves state ambiguous (main.ts, main.ts).
  • Commands call this.view directly without ensuring the view exists; invoking Refresh before the view is created will throw (main.ts).

Medium priority findings

  • Location debounce is only 750 ms; users can overrun the rate limit while typing. The debounce also runs even when the value is empty (main.ts).
  • Network errors are only logged to the console; no user feedback or exponential backoff is provided (main.ts).
  • Air-quality rendering assumes us-epa-index is truthy; index 0 or missing values skip the label silently (main.ts).
  • The view instantiation logic in initView manually constructs WeatherView even though setViewState already uses the registered constructor; this risks double state and duplicated timers (main.ts).

Low priority/UI

  • Forecast tooltip uses absolute positioning without offset; on narrow panes it can overflow. Consider tippy.js (already a dependency) for better placement.
  • Emoji icons mix Unicode planes; stick to ASCII or an icon set to avoid font fallbacks.
  • Styles use display: column; (invalid) on .weather-container (styles.css).

Suggested improvements

  1. Interval & lifecycle
  • Store updateInterval as number | null, use this.registerInterval to let Obsidian clean up, and centralize scheduling in one place (e.g., after setViewState and when the refresh interval changes). Guard view existence before calling displayTemperature().
  1. Debounce & settings
  • Raise location debounce to 1500–2000 ms and ignore empty strings before fetching. Expose a setting if users want to tune it.
  • Align the refresh interval units: store minutes (or seconds) consistently, update defaults/docs, and validate a reasonable minimum (WeatherAPI caps free tier to ~1k calls/day).
  1. Networking robustness
  • Switch to https, add a short timeout and better error handling (Notice on failure, retry with backoff, and surface the last successful update time). Abort inflight requests when scheduling a new refresh to avoid race conditions.
  1. View management
  • Let setViewState create the view and remove the manual new WeatherView branch. Use getRightLeaf(true) (or getLeaf('split', 'tab') on newer Obsidian) to auto-create the pane when absent.
  • Use registerEvent for onLayoutReady and any future workspace listeners to ensure disposal.
  1. UI/UX polish
  • Add a loading/empty state when API key/location is missing. Disable the Refresh command/button until required settings exist.
  • Label units explicitly (“Feels like: 23 °C”) and add tooltips for AQ values. Consider Intl.DateTimeFormat for locale-aware forecast labels.
  1. Testing/typing
  • Type the requestUrl response and handle non-200 statuses; WeatherAPI returns 4xx JSON with error fields. Add unit tests for the forecast extraction and AQ mapping logic.

Quick wins to ship first

  • Change the endpoint to https.
  • Unify refresh units (minutes) and raise the minimum to 30 minutes by default.
  • Centralize interval management with registerInterval and clear-before-set semantics.
  • Increase location debounce to ~1500 ms and skip fetches on empty input.
  • Show a Notice when fetch fails or when key/location is missing.