feat(shimmie-import): username target, retry-failed, run history - #24
Conversation
Carries forward improvements made to the merged importer: - upsertItem takes a discriminated union so a 'complete' row always has an assetId and a 'failed' row never does (the ledger can't record a success with no asset, or a failure pointing at one). - The optimistic run-progress update also guards on status = 'running', so a cancel that lands mid-step can't be resurrected back to running/done by the write; on a lost write it reports the PERSISTED run state, not the dropped local view. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Polish for the importer (on top of the concurrency hardening): - Core: authService.findByUsername(name) -> PublicUser | null (case-insensitive, hash-stripped). The importer resolves the target user by USERNAME now (default: the running admin) and validates it up front, so a typo fails at run creation instead of failing every post with a bad uploaderId FK. Web field is a username. - Retry-failed: a shared ingestPost() (extracted from stepRun, behavior unchanged) + retryFailed() + POST /runs/:id/retry-failed re-attempt a run's own failed posts without re-scanning. The ledger gains a run_id column (migration 0001) so retry is SCOPED to the owning run — it can't re-attribute another run's posts to the wrong target user. Run counters use relative SQL increments (not stale absolute values, which could clobber a concurrent step). Recovered items move from failed -> imported; the client loops until this run's failures reach 0 or a batch makes no progress. Retry is available per-run in the history list (works after a reload), and Cancel shows while retrying. - Run history: the admin console lists recent runs with status/counts/progress. Also lands two design-plan docs (drafts): docs/ingest-hooks.md + docs/ingest-clients.md for the next feature arc. Tests: findByUsername (case-insensitive, no hash, null-on-missing). typecheck + lint:boundaries (184 modules) + full suite (251 pass) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR adds Shimmie import run tracking, failed-post retries, username-based attribution, and recent-run controls. It also adds ChangesShimmie import run management
Ingest client design
Ingest hook design
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant ShimmieImportRoutes
participant retryFailed
participant SourceAdapter
participant ImportLedger
Admin->>ShimmieImportRoutes: POST /runs/:id/retry-failed
ShimmieImportRoutes->>retryFailed: Retry failed items
retryFailed->>ImportLedger: Load failed items for run
retryFailed->>SourceAdapter: Fetch source post
SourceAdapter-->>retryFailed: Post or deleted result
retryFailed->>ImportLedger: Record retry result
retryFailed-->>ShimmieImportRoutes: Return RetryResult
ShimmieImportRoutes-->>Admin: Return retry outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
docs/ingest-clients.md-280-288 (1)
280-288: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce deletion and ban checks at upload commit.
POST /assets/existsis only a preflight. A client can be raced after anabsentresult and before upload, and an untrusted client can skip the endpoint. Check the original hash against tombstones and bans atomically when the upload is committed. Otherwise the next sync can resurrect deleted content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-clients.md` around lines 280 - 288, Update the upload commit flow to atomically check the original hash against server-side deletion tombstones and bans, rejecting the upload when either applies. Do not rely solely on the POST /assets/exists preflight; preserve it as an optimization while enforcing the final decision at commit.docs/ingest-clients.md-236-244 (1)
236-244: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse
originalSha256consistently in gallery sync.Section 2 says clients hash original bytes and
/assets/existsresolvesoriginalSha256. This section switches tosha256andfindBySha256. With transforms or many-to-one variants, that can miss existing assets and re-upload them indefinitely. Make the batchoriginalSha256response authoritative and return canonical asset IDs with each state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-clients.md` around lines 236 - 244, Update the “Gallery sync is mostly already built” section to use originalSha256 consistently instead of sha256 and findBySha256. Treat the batch originalSha256 response from POST /assets/exists as authoritative, and specify that each returned state includes the canonical asset ID to prevent duplicate uploads across transformed or many-to-one variants.docs/ingest-clients.md-175-186 (1)
175-186: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSeparate offset conflicts from absent uploads.
PATCH /uploads/:tokencan return 409 for a staleUpload-Offset, but the docs currently treat every 409 as an abandoned expired/cancelled session and restart at offset zero. Restart at zero only afterHEAD /uploads/:tokenorDELETE /uploads/:tokenexplicitly removes the staged upload, or after an explicit expired/cancelled response. Treat unknown-session 404s separately and preserve explicit finalization idempotency guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-clients.md` around lines 175 - 186, The upload retry guidance must distinguish PATCH offset conflicts from abandoned sessions: do not restart at offset zero for every 409, but reconcile using the server-authoritative offset and retry safely. Restart only when HEAD or DELETE confirms the staged upload is absent, or when the server explicitly reports expiration/cancellation; handle unknown-session 404s separately and retain explicit idempotent finalization behavior.docs/ingest-clients.md-268-269 (1)
268-269: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not rely on
(localId, mtime, size)as the only rehash trigger.A same-size edit with an unchanged or coarse timestamp can reuse a stale hash. Rolling server re-verification cannot detect this when it reuses the cached hash. Add a platform edit/version token where available and periodically rehash cache hits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-clients.md` around lines 268 - 269, Update the rehash-cache guidance around the “avoiding rehashing” rule so `(localId, mtime, size)` is not the sole invalidation trigger: incorporate a platform-provided edit/version token when available, and periodically rehash cache hits even when metadata is unchanged, ensuring server re-verification cannot reuse a stale hash.docs/ingest-clients.md-169-173 (1)
169-173: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist a recoverable byte source, not only upload metadata.
chrome.storagekeeps metadata, but MV3 service-worker restarts may fetch a different source after the original bytes are no longer available or are altered. Resume only from bytes that survive termination: stage them inIndexedDB/OPFS, or re-fetch and validate the full declared size and content hash before appending chunks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-clients.md` around lines 169 - 173, Update the documented resume flow so persisted state includes a recoverable byte source, not just file identity, token, offset, and retry count. Specify staging bytes in IndexedDB or OPFS, or re-fetching and validating the complete declared size and content hash before appending chunks; ensure reconciliation resumes only after that source validation succeeds.docs/ingest-clients.md-206-215 (1)
206-215: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDefine the share-target POST contract before making it queue uploads.
This plan creates a single POST route without defining authentication, CSRF/origin checks, size/content limits, or authorization before media enter the upload queue. Require an authenticated session or one-time upload token, enforce origin/CSRF protection, validate/scope the uploaded file, and make Background Sync refuse expired or unauthenticated requests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-clients.md` around lines 206 - 215, Expand the “PWA share target (Android stopgap)” plan to define the POST route contract before queueing uploads: require an authenticated session or one-time upload token, enforce origin/CSRF protection, validate and scope file size/content, and ensure Background Sync rejects expired or unauthenticated requests.docs/ingest-clients.md-141-153 (1)
141-153: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftGate the complete operation on the user gesture.
The rules only block credentialed requests. A valid sender that fails at
host_permissionscan still use the fallbackfetchwith the extension’s stored API key to upload the URL. Issue a one-shot capability fromchrome.contextMenus.onClickedor the popup, bind it to the tab and origin, and reject the whole operation when it is absent or invalid. Allow an uncredentialed fetch only after the gesture passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-clients.md` around lines 141 - 153, Update the credentialed-fetch flow in the ingest client documentation to require a valid one-shot capability issued by chrome.contextMenus.onClicked or the popup, bound to the originating tab and origin. Reject the entire operation when the capability is missing or invalid; only after validating the gesture may the implementation fall back to an uncredentialed fetch, including when host_permissions validation fails.docs/ingest-clients.md-116-125 (1)
116-125: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the fetched response before buffering it.
The extension example calls
fetch(imgUrl).blob()on a URL that can arrive via extension messaging, then createsFormData. ThePOST /assetssize cap applies only after that response is read and queued; if the response is non-image or huge,blob()can still consume the full body. Checkresponse.ok, allowlisted media types, and declared/actual size before creatingFormData, and route oversized responses directly to413without buffering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-clients.md` around lines 116 - 125, Update the extension fetch flow around the image response before calling blob() or constructing FormData: validate response.ok, allowlisted image media types, and declared/actual size against the upload cap, routing oversized responses directly to 413 without buffering. Preserve the existing POST /assets multipart submission for valid responses.plugins/shimmie-import/src/import.ts-303-305 (1)
303-305: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA canceled run can be retried, because neither the server nor the client checks the run status.
stepRunrefuses to work when the status is notrunning, and this PR adds a status guard to its progress write so a mid-flight cancel survives. The retry path applies no equivalent check at any layer, so cancellation does not stop a run from importing more posts.
plugins/shimmie-import/src/import.ts#L303-L305: after loading the run, reject the retry whenrun.statusiscanceled. This is the authoritative fix.plugins/shimmie-import/src/import.ts#L346-L355: add the same status condition to theimportedandfailedcounter update, so a cancel that lands mid-retry does not move a canceled run's totals.apps/web/src/plugins/shimmie-import/ShimmieImportSection.tsx#L374-L384: render the Retry button only whenrun.statuspermits a retry, so the operator cannot start an action the server will reject.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/shimmie-import/src/import.ts` around lines 303 - 305, Prevent canceled import runs from being retried or updated: in plugins/shimmie-import/src/import.ts lines 303-305, update the retry flow around run loading to reject when run.status is canceled; in plugins/shimmie-import/src/import.ts lines 346-355, add the same status condition to the imported and failed counter update; in apps/web/src/plugins/shimmie-import/ShimmieImportSection.tsx lines 374-384, render the Retry button only for statuses that permit retrying.apps/web/src/plugins/shimmie-import/ShimmieImportSection.tsx-172-174 (1)
172-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRetrying a recent run overwrites the progress panel of a different run.
progressdescribes the run started byonImport.onRetryaccepts anytargetRunId, and the recent-runs list at line 377 calls it for arbitrary runs. This update then adds run B'srecoveredcount to run A's displayedimportedvalue and replaces run A'sfailedvalue with run B'sremainingFailed. The panel shows a mix of two runs.The same call is a no-op when
progressis null, which is the normal state for a recent-run retry, so that path shows no progress feedback at all.Update the panel only when the retried run is the run the panel describes.
🐛 Proposed guard
- setProgress((prev) => - prev ? { ...prev, imported: prev.imported + res.recovered, failed: res.remainingFailed } : prev, - ); + // Only the active run owns the progress panel. A retry launched from the + // recent-runs list must not rewrite another run's displayed totals. + if (targetRunId === runId) { + setProgress((prev) => + prev ? { ...prev, imported: prev.imported + res.recovered, failed: res.remainingFailed } : prev, + ); + }
runIdis read inside an async loop. Read it through a ref, or pass the active run id intoonRetry, so the comparison does not use a stale closure value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/plugins/shimmie-import/ShimmieImportSection.tsx` around lines 172 - 174, Update the retry progress state in onRetry so it only modifies progress when targetRunId matches the run currently described by the panel; otherwise leave progress unchanged. Ensure the comparison uses the current run ID rather than a stale async-loop closure, by reading runId through a ref or passing the active run ID into onRetry, and preserve normal feedback for retries of the active run.plugins/shimmie-import/src/import.ts-330-338 (1)
330-338: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDeleting a stale failed row leaves
importRuns.failedoverstated.When
fetchPostreturns null, this branch removes the ledger row. It does not decrementrun.failed, and the row is counted in neitherrecoverednorstillFailed, so the counter update at line 346 never corrects it.
countFailedthen reportsremainingFailed: 0, but the persistedimportRuns.failedstays at its old value. The recent-runs list inShimmieImportSection.tsxreadsrun.failed, so it keeps showing a non-zero failure count and a Retry button for a run with no failed rows left. Each further retry returns zero work.Track the deleted rows and subtract them from the run counter.
🐛 Proposed counter correction
let recovered = 0; let stillFailed = 0; + let dropped = 0;if (!post) { // Source post is gone now — drop the stale failed row (nothing to recover). await ctx.db .delete(importItems) .where( and(eq(importItems.sourceInstance, adapter.sourceInstance), eq(importItems.sourcePostId, sourcePostId)), ); + dropped += 1; continue; }- if (recovered > 0) { + if (recovered > 0 || dropped > 0) { await ctx.db .update(importRuns) .set({ imported: sql`${importRuns.imported} + ${recovered}`, - failed: sql`greatest(${importRuns.failed} - ${recovered}, 0)`, + failed: sql`greatest(${importRuns.failed} - ${recovered + dropped}, 0)`, updatedAt: new Date(), })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/shimmie-import/src/import.ts` around lines 330 - 338, Update the stale-row removal branch in the retry flow around fetchPost so deleted failed rows are tracked and subtracted from the associated importRuns.failed counter. Ensure the counter update includes these deletions alongside recovered and stillFailed counts, so runs with no remaining failed rows persist a zero failure count.docs/ingest-hooks.md-106-108 (1)
106-108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRe-hash every non-null transform result.
apply()cannot report whether a returnedBlobis byte-identical without reading it. If the pipeline skips hashing a changed blob, the storagesha256and dedupe identity can be wrong.Treat every non-null result as changed, or return an explicit unchanged or hash-bearing result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-hooks.md` around lines 106 - 108, Update the transform pipeline documentation to require re-hashing every non-null result returned by apply(), since it cannot determine byte identity without reading the Blob. Ensure storage sha256 and deduplication identity are recalculated for each non-null result, or specify an explicit unchanged or hash-bearing result contract.docs/ingest-hooks.md-244-260 (1)
244-260: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not return raw guard reasons in HTTP 403 responses.
reasonis plugin-controlled. A banned-hash reason can contain moderator notes or internal policy details.Return a stable public error code or message. Keep the raw reason in server-side logs or authorized admin views.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-hooks.md` around lines 244 - 260, Update the createAssetService ingest denial handling so HTTP 403 responses expose only a stable public error code or message, never the plugin-controlled IngestVerdict.reason; retain the raw reason exclusively in server-side logs or authorized admin views. Keep the IngestGuard contract unchanged.docs/ingest-hooks.md-262-265 (1)
262-265: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProvide a bounded format probe for guards.
The design tells guards to sniff
ctx.blobthemselves. The existingpackages/core/src/services/asset-service.tsingest path applies amaxPixelslimit during image metadata probing.Expose a shared safe metadata helper, or require the same size and pixel limits in guard implementations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-hooks.md` around lines 262 - 265, Update the guard design around the ingest path and asset-service metadata probing to provide a shared bounded format/metadata probe for ctx.blob, reusing the existing size and maxPixels limits. Alternatively, explicitly require every guard that performs its own sniffing to enforce those same bounds; do not leave guards performing unbounded blob inspection.docs/ingest-hooks.md-295-332 (1)
295-332: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine dedupe behavior for deleted assets.
The current
createAssetServiceimplementation checksrepository.findBySha256and returns the existing asset before insert.The design says a deleted hash must upload as
201. A soft-deleted row would still be returned as a200dedupe unless dedupe ignores deleted rows or resurrects them.Specify the tombstone schema, uniqueness rule, and create behavior before implementing the four-state API.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-hooks.md` around lines 295 - 332, Define the deleted-asset tombstone schema and its interaction with the hash uniqueness constraint before implementing the four-state API. Update createAssetService so findBySha256 does not return soft-deleted rows as 200 dedupe: deleted hashes must upload as 201 while preserving the tombstone and enforcing the chosen uniqueness rule, with banned hashes still taking precedence and returning 403.docs/ingest-hooks.md-251-255 (1)
251-255: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine current state for chained transforms.
Multiple transforms need the output of one transform as the input to the next.
originalSha256is explicit, butblob,md5, andsizeBytesdo not identify whether they describe the original or current bytes.Separate original metadata from current transform input, and rebuild the context after each successful transform.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-hooks.md` around lines 251 - 255, Update IngestContext and the IngestTransform chaining flow to distinguish immutable original metadata from the current transform input: retain originalSha256 and original blob metadata separately, and expose current blob, md5, and sizeBytes as the values for the next transform. After each successful transform, rebuild the context from the returned Blob and recompute its current metadata before invoking the next transform.docs/ingest-hooks.md-117-147 (1)
117-147: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftComplete the dual-hash storage contract before implementation.
One
originalSha256column cannot represent multiple originals that transform to one stored asset. Later originals will be lost, so/assets/existswill returnabsentand sync will retry the upload.Use an alias table, or record every original hash on each dedupe hit. Backfill existing rows with
originalSha256 = sha256before querying the new column.The current
packages/core/src/services/asset-service.tsingest implementation also persistsmd5from the pre-transform bytes. Define whethermd5identifies the original bytes or stored bytes. Alignbanned_hashes.sha256with that identity.Also applies to: 282-290
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-hooks.md` around lines 117 - 147, Complete the dual-hash contract before implementing the ingest changes: use an alias/source-hash table, or ensure every original hash is recorded on dedupe hits so `/assets/exists` never loses variants. Backfill existing assets with the original hash equal to `sha256` before querying it. In the asset-service ingest flow, explicitly define whether `md5` represents pre-transform or stored bytes, and align `banned_hashes.sha256` with that same identity.docs/ingest-hooks.md-338-346 (1)
338-346: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winConfirm moderation-state visibility before exposing
/assets/exists.The proposal leaks
bannedto every upload-scoped caller. Decide whether clients need exact moderation state, a generic non-uploadable result, or privileged-only visibility before Phase 4 ships.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-hooks.md` around lines 338 - 346, Before implementing or shipping `/assets/exists`, resolve and document the moderation-state visibility policy for `banned`: choose exact visibility for all callers, a generic non-uploadable response, or privileged-only access. Update the endpoint proposal and related §11 guidance to match the approved policy, rather than exposing banned status by default to every upload-scoped caller.docs/ingest-hooks.md-180-186 (1)
180-186: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not promise hard timeouts for synchronous hooks.
check()andapply()accept synchronous return values:check(ctx: IngestContext): Promise<IngestVerdict> | IngestVerdict; apply(ctx: IngestContext): Promise<Blob | null> | Blob | null;A CPU-bound or infinite synchronous hook blocks the event loop, so timers and
AbortSignalcannot interrupt it.Require asynchronous hooks with cooperative cancellation, or run untrusted/CPU-bound hooks in an isolated worker or process.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-hooks.md` around lines 180 - 186, Update the timeout contract in the documented hook behavior to avoid promising hard timeouts for synchronous check() and apply() implementations. Require hooks to be asynchronous and honor IngestContext’s AbortSignal, or explicitly isolate untrusted/CPU-bound hooks in a worker or process before claiming they can be interrupted.docs/ingest-hooks.md-232-242 (1)
232-242: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd the promised
AbortSignaltoIngestContext.The failure contract requires hooks to receive an
AbortSignal, but the interface does not expose one. Plugins cannot observe timeout or cancellation.Add
readonly signal: AbortSignaland define whether the signal is shared across the pipeline or created per hook.Proposed interface addition
export interface IngestContext { + readonly signal: AbortSignal; readonly originalSha256: string;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-hooks.md` around lines 232 - 242, Update the IngestContext interface to expose a readonly signal: AbortSignal property, and document that the same signal is shared across the entire ingest pipeline so every hook can observe cancellation and timeout consistently.
🟡 Minor comments (6)
packages/core/test/auth-service.test.ts-167-176 (1)
167-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest a mixed-case persisted username.
registerstores"Alice"as"alice"before this lookup runs. The exact-match fake repository can therefore pass the test even when lookup is not case-insensitive. Seed a row withusername: "Alice"without callingregister, then query"ALICE".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/auth-service.test.ts` around lines 167 - 176, Update the createAuthService.findByUsername test to seed the fake repository directly with a user whose persisted username is mixed-case, such as "Alice", instead of creating it through register. Then query "ALICE" and retain the existing assertions for the user identity, returned username, and absence of passwordHash.docs/ingest-clients.md-73-73 (1)
73-73: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a complete access phrase.
Change “An ingest client never needs read, delete, or admin.” to “An ingest client does not need read, delete, or admin access.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ingest-clients.md` at line 73, Update the “Upload-only scope” documentation sentence to use the complete phrase “does not need … access,” while preserving the existing access types and meaning.Source: Linters/SAST tools
plugins/shimmie-import/src/import.ts-276-277 (1)
276-277: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the scope stated in the
remainingFaileddoc comment.The comment says "for the source".
countFailedfilters onrunId, so the value is scoped to one run. The whole retry design is run-scoped. The comment contradicts the exported contract.📝 Proposed comment fix
- /** Failed items remaining for the source after this call. */ + /** Failed items remaining for this run after this call. */ remainingFailed: number;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/shimmie-import/src/import.ts` around lines 276 - 277, Update the doc comment for the exported remainingFailed field to state that it counts failed items remaining for the current run after the call, rather than for the source. Keep the field and its type unchanged.plugins/shimmie-import/src/index.ts-122-123 (1)
122-123: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject stale
targetUserIdrequests for/runs.The
POST /runsbody now acceptstargetUsername, but an unknown field is still ignored. A stale admin UI tab can still sendtargetUserId; the import then runs as the acting admin instead of failing with a clear invalid payload error. Add back compatibility or add runtime validation that rejects unknown body fields for this route.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/shimmie-import/src/index.ts` around lines 122 - 123, Update the POST /runs request validation to reject unknown body fields, specifically stale targetUserId payloads, with a clear invalid-payload error while retaining targetUsername support; ensure the route does not silently fall back to the acting admin when an unsupported field is submitted.apps/web/src/plugins/shimmie-import/ShimmieImportSection.tsx-369-373 (1)
369-373: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGive the
✓and✗counters a text alternative.These glyphs are the only label for the imported and failed counts. Assistive technology announces them inconsistently, and the numbers lose their meaning.
run.statussits next to them, so the row is not unintelligible, but a listener cannot tell which number is which.Label the counts for assistive technology and hide the glyphs from it.
♿ Proposed labels
<span className="flex shrink-0 items-center gap-2 text-muted"> <span> - {run.status} · {run.imported}✓ - {run.failed > 0 ? ` · ${run.failed}✗` : ""} · {run.cursor}/{run.maxId} + {run.status} ·{" "} + <span title={`${run.imported} imported`}> + {run.imported} + <span aria-hidden="true">✓</span> + <span className="sr-only"> imported</span> + </span> + {run.failed > 0 ? ( + <span title={`${run.failed} failed`}> + {" · "} + {run.failed} + <span aria-hidden="true">✗</span> + <span className="sr-only"> failed</span> + </span> + ) : null} + {" · "} + {run.cursor}/{run.maxId} scanned </span>Confirm that an
sr-onlyutility exists in the project's Tailwind setup before you apply this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/plugins/shimmie-import/ShimmieImportSection.tsx` around lines 369 - 373, Update the status markup in ShimmieImportSection around the run counters so the imported and failed numbers have explicit assistive-technology labels, while marking the ✓ and ✗ glyphs as hidden from screen readers. Confirm and use the existing Tailwind sr-only utility for the labels, preserving the current visual layout and conditional rendering of failed counts.plugins/shimmie-import/src/import.ts-94-108 (1)
94-108: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA
setAssetTagsfailure records the post as failed while the asset already exists.If
assets.createsucceeds andsetAssetTagsthrows, the catch block writes a ledger row withstatus: "failed"andassetId: null. The created asset stays in the database with no tags and no ledger link. The retry path recovers it, because core deduplicates on sha256 and re-applies the tags. If the operator never retries, the asset stays untagged and invisible to the ledger.Record the created asset id on the failed row, or log the orphaned asset id so an operator can find it.
🐛 Proposed logging of the orphaned asset
async function ingestPost( ctx: PluginContext, adapter: SourceAdapter, post: SourcePost, runId: number, targetUserId: number, ): Promise<"complete" | "failed"> { + let createdAssetId: number | null = null; try { const bytes = await adapter.fetchBytes(post); const { asset } = await ctx.services.assets.create({ bytes, rating: post.rating, source: post.postUrl, uploaderId: targetUserId, createdAt: post.postedAt, }); + createdAssetId = asset.id; if (post.tags.length > 0) await ctx.services.tags.setAssetTags(asset.id, post.tags); await upsertItem(ctx, adapter.sourceInstance, post.sourcePostId, runId, { assetId: asset.id, status: "complete", error: null, }); return "complete"; } catch (error) { + const message = errorMessage(error); await upsertItem(ctx, adapter.sourceInstance, post.sourcePostId, runId, { assetId: null, status: "failed", - error: errorMessage(error), + error: message, }); - ctx.log.warn("import_post_failed", { sourcePostId: post.sourcePostId, error: errorMessage(error) }); + ctx.log.warn("import_post_failed", { sourcePostId: post.sourcePostId, createdAssetId, error: message }); return "failed"; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/shimmie-import/src/import.ts` around lines 94 - 108, Update the import flow around setAssetTags and the catch block so a tag-assignment failure preserves the asset.id returned by assets.create when recording the failed ledger row. Ensure the failed record links to the created asset, or include that asset ID in the failure log if the existing ledger contract cannot represent it; do not report the failure with assetId: null after creation succeeded.
🧹 Nitpick comments (1)
plugins/shimmie-import/src/schema.ts (1)
46-48: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd an index on
(run_id, status)for the retry queries.
countFailedandretryFailedinplugins/shimmie-import/src/import.tsboth filter onrun_idplusstatus. The retry endpoint runs both on every batch, and the web client loops the endpoint untilremainingFailedreaches 0. The only index on this table covers(source_instance, source_post_id), so each of those queries scans the whole ledger. The ledger grows with every post of every run.A composite index serves both queries.
♻️ Proposed index
- (table) => [unique().on(table.sourceInstance, table.sourcePostId)], + (table) => [ + unique().on(table.sourceInstance, table.sourcePostId), + index("shimmie_import_items_run_id_status_idx").on(table.runId, table.status), + ],Import
indexfromdrizzle-orm/pg-coreand regenerate the migration so the SQL and the snapshot stay in sync.Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/shimmie-import/src/schema.ts` around lines 46 - 48, Update the posts ledger table definition in schema.ts to import and declare a composite index on runId and status, alongside the existing table indexes. Regenerate the migration so both the generated SQL and schema snapshot include this index.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/shimmie-import/drizzle/0001_whole_shotgun.sql`:
- Line 1: Update the migration around shimmie_import_items to add run_id as
nullable, backfill existing rows with the 0 sentinel, then enforce NOT NULL so
it succeeds for non-empty tables while excluding legacy rows from run-scoped
retries. Keep the final column shape consistent with the existing 0001 snapshot,
which should remain valid after the multi-statement SQL change.
---
Major comments:
In `@apps/web/src/plugins/shimmie-import/ShimmieImportSection.tsx`:
- Around line 172-174: Update the retry progress state in onRetry so it only
modifies progress when targetRunId matches the run currently described by the
panel; otherwise leave progress unchanged. Ensure the comparison uses the
current run ID rather than a stale async-loop closure, by reading runId through
a ref or passing the active run ID into onRetry, and preserve normal feedback
for retries of the active run.
In `@docs/ingest-clients.md`:
- Around line 280-288: Update the upload commit flow to atomically check the
original hash against server-side deletion tombstones and bans, rejecting the
upload when either applies. Do not rely solely on the POST /assets/exists
preflight; preserve it as an optimization while enforcing the final decision at
commit.
- Around line 236-244: Update the “Gallery sync is mostly already built” section
to use originalSha256 consistently instead of sha256 and findBySha256. Treat the
batch originalSha256 response from POST /assets/exists as authoritative, and
specify that each returned state includes the canonical asset ID to prevent
duplicate uploads across transformed or many-to-one variants.
- Around line 175-186: The upload retry guidance must distinguish PATCH offset
conflicts from abandoned sessions: do not restart at offset zero for every 409,
but reconcile using the server-authoritative offset and retry safely. Restart
only when HEAD or DELETE confirms the staged upload is absent, or when the
server explicitly reports expiration/cancellation; handle unknown-session 404s
separately and retain explicit idempotent finalization behavior.
- Around line 268-269: Update the rehash-cache guidance around the “avoiding
rehashing” rule so `(localId, mtime, size)` is not the sole invalidation
trigger: incorporate a platform-provided edit/version token when available, and
periodically rehash cache hits even when metadata is unchanged, ensuring server
re-verification cannot reuse a stale hash.
- Around line 169-173: Update the documented resume flow so persisted state
includes a recoverable byte source, not just file identity, token, offset, and
retry count. Specify staging bytes in IndexedDB or OPFS, or re-fetching and
validating the complete declared size and content hash before appending chunks;
ensure reconciliation resumes only after that source validation succeeds.
- Around line 206-215: Expand the “PWA share target (Android stopgap)” plan to
define the POST route contract before queueing uploads: require an authenticated
session or one-time upload token, enforce origin/CSRF protection, validate and
scope file size/content, and ensure Background Sync rejects expired or
unauthenticated requests.
- Around line 141-153: Update the credentialed-fetch flow in the ingest client
documentation to require a valid one-shot capability issued by
chrome.contextMenus.onClicked or the popup, bound to the originating tab and
origin. Reject the entire operation when the capability is missing or invalid;
only after validating the gesture may the implementation fall back to an
uncredentialed fetch, including when host_permissions validation fails.
- Around line 116-125: Update the extension fetch flow around the image response
before calling blob() or constructing FormData: validate response.ok,
allowlisted image media types, and declared/actual size against the upload cap,
routing oversized responses directly to 413 without buffering. Preserve the
existing POST /assets multipart submission for valid responses.
In `@docs/ingest-hooks.md`:
- Around line 106-108: Update the transform pipeline documentation to require
re-hashing every non-null result returned by apply(), since it cannot determine
byte identity without reading the Blob. Ensure storage sha256 and deduplication
identity are recalculated for each non-null result, or specify an explicit
unchanged or hash-bearing result contract.
- Around line 244-260: Update the createAssetService ingest denial handling so
HTTP 403 responses expose only a stable public error code or message, never the
plugin-controlled IngestVerdict.reason; retain the raw reason exclusively in
server-side logs or authorized admin views. Keep the IngestGuard contract
unchanged.
- Around line 262-265: Update the guard design around the ingest path and
asset-service metadata probing to provide a shared bounded format/metadata probe
for ctx.blob, reusing the existing size and maxPixels limits. Alternatively,
explicitly require every guard that performs its own sniffing to enforce those
same bounds; do not leave guards performing unbounded blob inspection.
- Around line 295-332: Define the deleted-asset tombstone schema and its
interaction with the hash uniqueness constraint before implementing the
four-state API. Update createAssetService so findBySha256 does not return
soft-deleted rows as 200 dedupe: deleted hashes must upload as 201 while
preserving the tombstone and enforcing the chosen uniqueness rule, with banned
hashes still taking precedence and returning 403.
- Around line 251-255: Update IngestContext and the IngestTransform chaining
flow to distinguish immutable original metadata from the current transform
input: retain originalSha256 and original blob metadata separately, and expose
current blob, md5, and sizeBytes as the values for the next transform. After
each successful transform, rebuild the context from the returned Blob and
recompute its current metadata before invoking the next transform.
- Around line 117-147: Complete the dual-hash contract before implementing the
ingest changes: use an alias/source-hash table, or ensure every original hash is
recorded on dedupe hits so `/assets/exists` never loses variants. Backfill
existing assets with the original hash equal to `sha256` before querying it. In
the asset-service ingest flow, explicitly define whether `md5` represents
pre-transform or stored bytes, and align `banned_hashes.sha256` with that same
identity.
- Around line 338-346: Before implementing or shipping `/assets/exists`, resolve
and document the moderation-state visibility policy for `banned`: choose exact
visibility for all callers, a generic non-uploadable response, or
privileged-only access. Update the endpoint proposal and related §11 guidance to
match the approved policy, rather than exposing banned status by default to
every upload-scoped caller.
- Around line 180-186: Update the timeout contract in the documented hook
behavior to avoid promising hard timeouts for synchronous check() and apply()
implementations. Require hooks to be asynchronous and honor IngestContext’s
AbortSignal, or explicitly isolate untrusted/CPU-bound hooks in a worker or
process before claiming they can be interrupted.
- Around line 232-242: Update the IngestContext interface to expose a readonly
signal: AbortSignal property, and document that the same signal is shared across
the entire ingest pipeline so every hook can observe cancellation and timeout
consistently.
In `@plugins/shimmie-import/src/import.ts`:
- Around line 303-305: Prevent canceled import runs from being retried or
updated: in plugins/shimmie-import/src/import.ts lines 303-305, update the retry
flow around run loading to reject when run.status is canceled; in
plugins/shimmie-import/src/import.ts lines 346-355, add the same status
condition to the imported and failed counter update; in
apps/web/src/plugins/shimmie-import/ShimmieImportSection.tsx lines 374-384,
render the Retry button only for statuses that permit retrying.
- Around line 330-338: Update the stale-row removal branch in the retry flow
around fetchPost so deleted failed rows are tracked and subtracted from the
associated importRuns.failed counter. Ensure the counter update includes these
deletions alongside recovered and stillFailed counts, so runs with no remaining
failed rows persist a zero failure count.
---
Minor comments:
In `@apps/web/src/plugins/shimmie-import/ShimmieImportSection.tsx`:
- Around line 369-373: Update the status markup in ShimmieImportSection around
the run counters so the imported and failed numbers have explicit
assistive-technology labels, while marking the ✓ and ✗ glyphs as hidden from
screen readers. Confirm and use the existing Tailwind sr-only utility for the
labels, preserving the current visual layout and conditional rendering of failed
counts.
In `@docs/ingest-clients.md`:
- Line 73: Update the “Upload-only scope” documentation sentence to use the
complete phrase “does not need … access,” while preserving the existing access
types and meaning.
In `@packages/core/test/auth-service.test.ts`:
- Around line 167-176: Update the createAuthService.findByUsername test to seed
the fake repository directly with a user whose persisted username is mixed-case,
such as "Alice", instead of creating it through register. Then query "ALICE" and
retain the existing assertions for the user identity, returned username, and
absence of passwordHash.
In `@plugins/shimmie-import/src/import.ts`:
- Around line 276-277: Update the doc comment for the exported remainingFailed
field to state that it counts failed items remaining for the current run after
the call, rather than for the source. Keep the field and its type unchanged.
- Around line 94-108: Update the import flow around setAssetTags and the catch
block so a tag-assignment failure preserves the asset.id returned by
assets.create when recording the failed ledger row. Ensure the failed record
links to the created asset, or include that asset ID in the failure log if the
existing ledger contract cannot represent it; do not report the failure with
assetId: null after creation succeeded.
In `@plugins/shimmie-import/src/index.ts`:
- Around line 122-123: Update the POST /runs request validation to reject
unknown body fields, specifically stale targetUserId payloads, with a clear
invalid-payload error while retaining targetUsername support; ensure the route
does not silently fall back to the acting admin when an unsupported field is
submitted.
---
Nitpick comments:
In `@plugins/shimmie-import/src/schema.ts`:
- Around line 46-48: Update the posts ledger table definition in schema.ts to
import and declare a composite index on runId and status, alongside the existing
table indexes. Regenerate the migration so both the generated SQL and schema
snapshot include this index.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96857106-d966-4ac8-bd03-ccbf2ae179fb
📒 Files selected for processing (12)
apps/api/test/server.test.tsapps/web/src/plugins/shimmie-import/ShimmieImportSection.tsxdocs/ingest-clients.mddocs/ingest-hooks.mdpackages/core/src/services/auth-service.tspackages/core/test/auth-service.test.tsplugins/shimmie-import/drizzle/0001_whole_shotgun.sqlplugins/shimmie-import/drizzle/meta/0001_snapshot.jsonplugins/shimmie-import/drizzle/meta/_journal.jsonplugins/shimmie-import/src/import.tsplugins/shimmie-import/src/index.tsplugins/shimmie-import/src/schema.ts
Address CodeRabbit review on PR #24: Postgres rejects ADD COLUMN ... NOT NULL without a DEFAULT when the table has rows. Add run_id with a transient DEFAULT 0 (backfilling a sentinel that matches no real run), then DROP the default — end state (NOT NULL, no default) matches the Drizzle snapshot, and the plugin always supplies run_id on insert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
- ingestPost: preserve the created asset id in the failure log when tag-assignment fails after create succeeds (the asset isn't lost; a retry re-applies tags). - retryFailed: reject a canceled run (don't resurrect it), and guard the counter update with status != canceled; count deleted stale rows and subtract them (with recovered) from the run's failed counter so a run with no remaining failures persists failed = 0. - /runs body: reject unknown fields (a stale targetUserId now 422s instead of being silently ignored + defaulting the target to the admin). - schema: composite index on (run_id, status) for the retry query (migration 0002). - web: render the per-run Retry button only for non-canceled runs; only update the progress panel when retrying the run it describes (via a ref, not a stale closure); give the run-history ✓/✗ counts sr-only labels + aria-hidden glyphs. - docs: remainingFailed counts THIS run's failures. Skipped (with reason): reseeding the findByUsername test with a mixed-case stored username — the in-memory fake repo does exact-match and doesn't model the real repo's lower() index, so that scenario can't occur through it; the current test validates the service's query-normalization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Migration safety: add run_id with a transient DEFAULT 0 then DROP DEFAULT, so ADD COLUMN NOT NULL doesn't fail on a non-empty table (end state matches the snapshot: NOT NULL, no default). - ingestPost: preserve the created asset id in the failure log when tag-assignment fails after create succeeds. - retryFailed: reject a canceled run + status-guard the counter update; count deleted stale rows and subtract them (with recovered) from the run's failed counter. Scoped to the owning run via a run_id column so it can't re-attribute another run's posts; relative SQL counter increments. - /runs: reject unknown body fields (a stale targetUserId now 422s). - schema: composite index on (run_id, status) for the retry query (migration 0002). - web: per-run Retry only for non-canceled runs; progress panel only updates when retrying the run it describes (ref, not a stale closure); sr-only labels + aria-hidden on the run-history glyphs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b0147e2 to
3762c20
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Why
Polish for the shimmie importer (#23), plus the concurrency hardening carried over from the merged branch. Rounds out day-to-day usability: attribute imports to a real user by name, recover from transient failures, and see past runs.
What
Core (small):
authService.findByUsername(name) → PublicUser | null(case-insensitive, hash-stripped). The importer now resolves the target user by username (default: the running admin) and validates it up front, so a typo fails at run creation rather than failing every post with a baduploaderIdFK. The web field is a username (was a raw numeric id).Retry-failed. A shared
ingestPost()(extracted fromstepRun, behavior unchanged) +retryFailed()+POST /runs/:id/retry-failedre-attempt a run's own failed posts without re-scanning the id space:run_idcolumn (incremental migration0001) so retry is scoped to the owning run — it can't re-attribute another run's posts to the wrong target user.failed → imported; the client loops until this run's failures reach 0 or a batch makes no progress (no infinite loop on permanent failures).Concurrency hardening (carried from the merged branch):
upsertItemdiscriminated union (acompleterow always has an asset, afailednever does); the run-progress update guards onstatus = running+ the read cursor, and reports the persisted state when its optimistic write loses.Run history. The admin console lists recent runs with status/counts/progress.
Also lands two design-plan drafts for the next arc:
docs/ingest-hooks.md(plugin-extensible ingest pipeline) +docs/ingest-clients.md(browser/mobile ingest clients).Verification
bun run typecheck✅ ·bun run lint:boundaries✅ (184 modules) · full suite ✅ (251 pass / 0 fail, incl. newfindByUsernametests).run_idcolumn), relative counter increments, per-run retry that survives reload, Cancel-while-retrying.Notes
run_idis addedNOT NULLviaALTER— safe because the importer is new/unused (empty table).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation