Imports v2.5

[skyfallwastaken]

No description provided.

[greptile-apps]

Greptile Summary

This PR is a substantial v2.5 overhaul of the heartbeat import pipeline, adding a WakaTime manual-download-link flow (for when WakaTime's 7-day export limit is hit), email notifications on completion/failure, streaming gzip decompression, a DB-level unique active-import constraint, and a reduced 8-minute remote cooldown. On the frontend, the Data settings page's state management is simplified to a single activeImport derived value with an optimistic overlay and Inertia's usePoll for live progress.

Key changes:

• New wakatime_download_link source kind and HeartbeatImportRemoteDownloadJob to download presigned S3 URLs directly, skipping the API dump-polling loop
• HeartbeatImportDumpJob now handles ManualDownloadLinkRequiredError by emailing the user with a link to the new WakatimeDownloadLink page
• HeartbeatImportMailer adds completion, failure, and manual-link-required email templates
• Partial unique index on heartbeat_import_runs(user_id) where state is active, preventing race conditions that ensure_no_active_import! alone couldn't stop
• Superadmin cooldown bypass added throughout the import lifecycle
• dump_job_pending_for? uses to_jsonb(?::text) which produces a JSON string, but GoodJob stores integer run IDs as JSON numbers — the duplicate-enqueue guard never matches (see inline comment)
• reset_sailors_log! was silently removed — after a successful remote import, the Sailors Log no longer refreshes; SailorsLogPollForChangesJob only inspects the last 10 minutes of heartbeats and will not pick up historical imported data
• Data.svelte uses a non-standard $derived(() => fn) pattern for isOverlayStale/effectiveImport; $derived.by is the idiomatic Svelte 5 approach
• The spinning SVG is duplicated between the remote and dev-upload status blocks

Confidence Score: 3/5

• Safe to merge with caveats — two logic issues should be confirmed before shipping: the broken duplicate-job guard and the Sailors Log regression.
• The core feature work is solid and well-tested. The JSONB type mismatch in dump_job_pending_for? means the duplicate-enqueue guard is silently no-op (low runtime impact thanks to GoodJob's total_limit: 1, but still broken intent). The removal of reset_sailors_log! is a silent behavioral regression for users with Sailors Log enabled — their project stats won't update after a WakaTime import. Both issues are correctness concerns, not build-blockers, but they affect production user experience.
• Pay close attention to app/services/heartbeat_import_runner.rb — specifically dump_job_pending_for? (line 117) and the missing reset_sailors_log! call in complete_run! (around line 195).

Important Files Changed

Filename	Overview
app/services/heartbeat_import_runner.rb	Major refactor: adds start_wakatime_download_link_import, centralises complete_run!/fail_run!/email helpers, and streaming file decompress. Two issues: dump_job_pending_for? has a JSONB integer-vs-string mismatch so the guard never fires, and reset_sailors_log! was silently removed, breaking Sailors Log refresh after remote imports.
app/jobs/heartbeat_import_dump_job.rb	Refactored into smaller focused helpers; now delegates downloading to HeartbeatImportRemoteDownloadJob, adds poll-timeout logic, and handles ManualDownloadLinkRequiredError by emailing the user. Logic appears correct.
app/jobs/heartbeat_import_remote_download_job.rb	New job: downloads the remote dump file and enqueues HeartbeatImportJob. Has concurrency control, retry-on-transient with fail-run callback, and correct auth-header handling for presigned S3 URLs.
app/models/heartbeat_import_run.rb	Adds wakatime_download_link source kind (enum 3), WAKATIME_SOURCE_KINDS, remote_dump_pollable? predicate, and adjusts remote? / validation accordingly. The distinction between remote? (api-key dump) and wakatime? is intentional.
app/javascript/pages/Users/Settings/Data.svelte	Significant state management overhaul: replaces many individual reactive variables with a single activeImport derived from an overlay+server merge. Uses non-standard $derived(() => fn) pattern (should be $derived.by). SVG spinner is duplicated. Polling and overlay-timeout logic is otherwise correct.
app/javascript/pages/HeartbeatImports/WakatimeDownloadLink.svelte	New standalone page for pasting a WakaTime presigned S3 URL. Uses Inertia <Form>, proper layout = false, URL input with required, and client-side disable while processing. Clean implementation.
app/mailers/heartbeat_import_mailer.rb	New mailer with three actions: import completed, import failed, and WakaTime manual download required. Uses recipient_email_for helper from runner; all templates exist and are tested.
app/services/heartbeat_import_dump_client.rb	Adds ManualDownloadLinkRequiredError/ExistingDumpInProgressError, richer ResponseError with HTTP status, valid_wakatime_download_url? class method, and hackatime-v1 in-progress dump recovery. Auth header logic correctly omits credentials for cross-origin (S3) downloads.
db/migrate/20260313140000_add_unique_active_import_constraint_to_heartbeat_import_runs.rb	Adds a partial unique index on user_id where state IN (0,1,2,3,4) to enforce one active import per user at the DB level, preventing race conditions in ensure_no_active_import!.
app/controllers/my/heartbeat_imports_controller.rb	Adds wakatime_download_link GET action with Inertia render, expands rescue to cover InvalidDownloadUrlError, and permits download_url param. Authentication split: ensure_current_user (JSON 401) for API-like actions, authenticate_user! for the page render.

Sequence Diagram

sequenceDiagram
    actor User
    participant Controller as HeartbeatImportsController
    participant Runner as HeartbeatImportRunner
    participant DumpJob as HeartbeatImportDumpJob
    participant DownloadJob as HeartbeatImportRemoteDownloadJob
    participant ImportJob as HeartbeatImportJob
    participant Mailer as HeartbeatImportMailer
    participant Client as HeartbeatImportDumpClient

    User->>Controller: POST /my/heartbeat_imports (api_key flow)
    Controller->>Runner: start_remote_import(user, provider, api_key)
    Runner->>Runner: ensure_no_active_import! (DB unique index)
    Runner-->>DumpJob: perform_later(run.id)
    DumpJob->>Client: request_dump
    alt WakaTime 7-day limit hit
        Client-->>DumpJob: ManualDownloadLinkRequiredError
        DumpJob->>Mailer: wakatime_manual_download_required
        Mailer-->>User: Email with manual link URL
        User->>Controller: GET /my/heartbeat_imports/wakatime_download_link
        Controller-->>User: WakatimeDownloadLink page
        User->>Controller: POST /my/heartbeat_imports (download_url)
        Controller->>Runner: start_wakatime_download_link_import(user, download_url)
        Runner-->>DownloadJob: perform_later(run.id, download_url)
    else Dump requested successfully
        DumpJob->>DumpJob: schedule_poll (every 3s, max 30min)
        DumpJob->>Client: list_dumps
        Client-->>DumpJob: dump ready
        DumpJob->>Runner: queue_remote_download(run, download_url)
        Runner-->>DownloadJob: perform_later(run.id, download_url)
    end
    DownloadJob->>Client: download_dump(url)
    Client-->>DownloadJob: file_content (gzip or JSON)
    DownloadJob-->>ImportJob: perform_later(run.id, file_path)
    ImportJob->>Runner: run_import (streaming decompression)
    Runner-->>Mailer: import_completed / import_failed
    Mailer-->>User: Email notification

Loading

Comments Outside Diff (4)

1. app/services/heartbeat_import_runner.rb, line 117 (link)

   JSONB type mismatch — guard never fires

   to_jsonb(?::text) casts the run ID to a JSON string (e.g., "42"). GoodJob serialises integer arguments as JSON numbers (e.g., 42), so the comparison 42 = "42" in JSONB is always false. The duplicate-enqueue guard therefore always returns false, meaning stale polling can enqueue duplicates. The total_limit: 1 concurrency control still prevents actual double-execution, but the intent of the check is broken.

   Fix: cast to bigint instead of text:

2. app/services/heartbeat_import_runner.rb, line 195-200 (link)

   reset_sailors_log! silently removed — Sailors Log won't reflect imported heartbeats

   The previous run_import called reset_sailors_log!(user) if run.remote? after a successful import. That reset cleared projects_summary and reinitialised it so the Sailors Log would reflect imported historical heartbeats.

   SailorsLogPollForChangesJob only queries heartbeats from the last 10 minutes, so it will never pick up the historical data that a WakaTime/Hackatime import brings in. Without the explicit reset, users whose Sailors Log was populated before the import will not see any increase in project durations from imported heartbeats.

   Was this removal intentional? If so, it would be good to add a comment explaining why the reset is no longer needed.

3. app/javascript/pages/Users/Settings/Data.svelte, line 274-293 (link)

   Non-standard $derived(() => fn) pattern — prefer $derived.by

   $derived(() => { ... }) derives a function reference as its value, not the function's return value. This is non-standard; calling isOverlayStale() and effectiveImport() works only because Svelte 5 tracks reactive reads through the entire synchronous call stack during activeImport's computation. The idiomatic Svelte 5 way to write multi-statement derivations is $derived.by:

   const isOverlayStale = $derived.by(() => {
     if (!overlayStartTime) return false;
     return Date.now() - overlayStartTime > OVERLAY_TIMEOUT_MS;
   });
   
   const activeImport = $derived.by(() => {
     if (isOverlayStale) return latest_heartbeat_import;
     if (isServerTerminal && importOverlay) return latest_heartbeat_import;
     return importOverlay ?? latest_heartbeat_import;
   });

   This removes the intermediate effectiveImport step and makes the reactivity model explicit.

   _{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

4. app/javascript/pages/Users/Settings/Data.svelte, line 564-584 (link)

   Duplicated SVG spinner — extract to a component

   The spinning SVG block (lines ~564-584 for the remote import section and ~701-722 for the dev-upload section) is copy-pasted verbatim. Per the repo's custom instruction, duplicated UI that makes sense as a component should be extracted. Consider creating app/javascript/components/Spinner.svelte:

   <svg class="h-4 w-4 shrink-0 animate-spin text-primary" viewBox="0 0 24 24" fill="none">
     <circle cx="12" cy="12" r="10" stroke="currentColor" stroke-width="3" class="opacity-25" />
     <path d="M4 12a8 8 0 018-8" stroke="currentColor" stroke-width="3" stroke-linecap="round" />
   </svg>

   Then use <Spinner /> at both call sites.

   _{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Prompt To Fix All With AI

This is a comment left during a code review.
Path: app/services/heartbeat_import_runner.rb
Line: 117

Comment:
**JSONB type mismatch — guard never fires**

`to_jsonb(?::text)` casts the run ID to a JSON *string* (e.g., `"42"`). GoodJob serialises integer arguments as JSON *numbers* (e.g., `42`), so the comparison `42 = "42"` in JSONB is always `false`. The duplicate-enqueue guard therefore always returns `false`, meaning stale polling can enqueue duplicates. The `total_limit: 1` concurrency control still prevents actual double-execution, but the intent of the check is broken.

Fix: cast to `bigint` instead of `text`:

```suggestion
    ).where("serialized_params -> 'arguments' -> 0 = to_jsonb(?::bigint)", run.id).exists?
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: app/services/heartbeat_import_runner.rb
Line: 195-200

Comment:
**`reset_sailors_log!` silently removed — Sailors Log won't reflect imported heartbeats**

The previous `run_import` called `reset_sailors_log!(user) if run.remote?` after a successful import. That reset cleared `projects_summary` and reinitialised it so the Sailors Log would reflect imported historical heartbeats.

`SailorsLogPollForChangesJob` only queries heartbeats from the **last 10 minutes**, so it will never pick up the historical data that a WakaTime/Hackatime import brings in. Without the explicit reset, users whose Sailors Log was populated before the import will not see any increase in project durations from imported heartbeats.

Was this removal intentional? If so, it would be good to add a comment explaining why the reset is no longer needed.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: app/javascript/pages/Users/Settings/Data.svelte
Line: 274-293

Comment:
**Non-standard `$derived(() => fn)` pattern — prefer `$derived.by`**

`$derived(() => { ... })` derives a *function reference* as its value, not the function's return value. This is non-standard; calling `isOverlayStale()` and `effectiveImport()` works only because Svelte 5 tracks reactive reads through the entire synchronous call stack during `activeImport`'s computation. The idiomatic Svelte 5 way to write multi-statement derivations is `$derived.by`:

```ts
const isOverlayStale = $derived.by(() => {
  if (!overlayStartTime) return false;
  return Date.now() - overlayStartTime > OVERLAY_TIMEOUT_MS;
});

const activeImport = $derived.by(() => {
  if (isOverlayStale) return latest_heartbeat_import;
  if (isServerTerminal && importOverlay) return latest_heartbeat_import;
  return importOverlay ?? latest_heartbeat_import;
});
```

This removes the intermediate `effectiveImport` step and makes the reactivity model explicit.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: app/javascript/pages/Users/Settings/Data.svelte
Line: 564-584

Comment:
**Duplicated SVG spinner — extract to a component**

The spinning SVG block (lines ~564-584 for the remote import section and ~701-722 for the dev-upload section) is copy-pasted verbatim. Per the repo's custom instruction, duplicated UI that makes sense as a component should be extracted. Consider creating `app/javascript/components/Spinner.svelte`:

```svelte
<svg class="h-4 w-4 shrink-0 animate-spin text-primary" viewBox="0 0 24 24" fill="none">
  <circle cx="12" cy="12" r="10" stroke="currentColor" stroke-width="3" class="opacity-25" />
  <path d="M4 12a8 8 0 018-8" stroke="currentColor" stroke-width="3" stroke-linecap="round" />
</svg>
```

Then use `<Spinner />` at both call sites.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 272a0f8}

[skyfallwastaken]

@greptileai review? and pay close attention to the overall job flow/architecture.

[skyfallwastaken]

@greptileai review? and pay close attention to the overall job flow/architecture.