feat: replace refresh DB flag polling with durably job trigger#208
Merged
feat: replace refresh DB flag polling with durably job trigger#208
Conversation
Instead of storing refreshRequestedAt in the DB and waiting for the hourly scheduler to pick it up, the refresh button now triggers a durably crawl job immediately with real-time progress tracking in the UI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract shared RunStatusAlerts component to deduplicate progress/status UI between RefreshSection and RecalculateSection - Remove empty loader (no data needed, avoids unnecessary revalidation) - Remove unreachable error alert from RefreshSection - Tighter button disabling: disable when runId exists to prevent gap between submit completion and useRun activation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDBフラグ 変更内容
Sequence Diagram(s)sequenceDiagram
participant User as User (ブラウザ)
participant UI as DataManagementPage UI
participant Server as Remix Route Action
participant Durably as Durably Service
participant DB as Tenant DB
User->>UI: 「Refresh」クリック
UI->>Server: POST action (trigger crawl)
Server->>Durably: enqueue crawl job
Durably-->>Server: returns runId
Server-->>UI: 200 { runId }
UI->>Durably: useRun(runId) / subscribe
Durably-->>UI: run status updates (running -> completed/failed)
Note over DB,Server: `refresh_requested_at` は DB に保存されない（列削除）
推定コード審査時間🎯 4 (Complex) | ⏱️ ~45 分 Possibly related PRs
詩
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
runId persists in fetcher.data after completion, so isBusy was always true. Revert to using isRunning for the disabled state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/routes/`$orgSlug/settings/data-management/index.tsx:
- Around line 35-43: The call to serverDurably.jobs.crawl.trigger in the refresh
handler is uncaught so failures bubble up as a 500 and the UI has no way to show
the error; wrap the trigger call in a try/catch, catch errors from
serverDurably.jobs.crawl.trigger and return a structured response (e.g. return
data({ intent: 'refresh', ok: false, error: err.message || String(err), runId:
null })), replacing the existing return data({ intent: 'refresh' as const, ok:
true, runId: run.id }) on success; also update the client-side fetcher handling
for the same flow (the other occurrence around serverDurably.jobs.crawl.trigger
at the 194-202 region) to detect ok:false and render a destructive alert with
the returned error message.
- Around line 157-189: The busy check wrongly includes runId != null which stays
set after a completed/failed run and keeps the Refresh button disabled; update
the isBusy calculation in the component to only consider submission or an active
non-terminal run (use isSubmitting || isRunning) and remove the runId != null
clause so that completed/failed runs no longer block re-submission; adjust any
related logic that relied on runId (e.g., where you pass runId into
durably.crawl.useRun) if you need to show previous run details, but do not use
runId alone to gate button enabled state.
In `@db/migrations/tenant/20260317120000_drop_refresh_requested_at.sql`:
- Around line 1-3: The migration drops the column
organization_settings.refresh_requested_at but db/schema.sql still contains that
column (see organization_settings.refresh_requested_at in db/schema.sql), so
update db/schema.sql to remove the refresh_requested_at column definition to
keep schema and migrations in sync; after editing db/schema.sql, apply the
migration and refresh the canonical schema by running the repository DB tasks
(e.g., generate/apply per project workflow: run pnpm db:migrate if you need to
generate migrations or pnpm db:apply then pnpm db:setup to ensure db/schema.sql
and the live DB are consistent).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4293d26-b68d-4fe5-8e9e-f2dad4ae6bb2
⛔ Files ignored due to path filters (1)
db/migrations/tenant/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
app/routes/$orgSlug/settings/data-management/index.tsxapp/services/jobs/crawl.server.tsapp/services/tenant-type.tsbatch/db/queries.tsbatch/job-scheduler.tsdb/migrations/tenant/20260317120000_drop_refresh_requested_at.sqldb/tenant.sql
💤 Files with no reviewable changes (4)
- app/services/tenant-type.ts
- db/tenant.sql
- app/services/jobs/crawl.server.ts
- batch/db/queries.ts
Comment on lines
+1
to
+3
| -- Drop column "refresh_requested_at" from table: "organization_settings" | ||
| -- Refresh scheduling is now handled directly via durably jobs instead of DB flag polling | ||
| ALTER TABLE `organization_settings` DROP COLUMN `refresh_requested_at`; |
There was a problem hiding this comment.
この migration だけだと db/schema.sql と実DBがずれます。
関連する db/schema.sql:103-118 にはまだ organization_settings.refresh_requested_at が残っています。このままだと pnpm db:setup や次回の migration 生成が古い定義を基準にしてしまうので、対応する schema 定義も同じ PR で更新してください。
As per coding guidelines, "Store database schema changes in db/schema.sql and generate migrations via pnpm db:migrate, then apply with pnpm db:apply".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrations/tenant/20260317120000_drop_refresh_requested_at.sql` around
lines 1 - 3, The migration drops the column
organization_settings.refresh_requested_at but db/schema.sql still contains that
column (see organization_settings.refresh_requested_at in db/schema.sql), so
update db/schema.sql to remove the refresh_requested_at column definition to
keep schema and migrations in sync; after editing db/schema.sql, apply the
migration and refresh the canonical schema by running the repository DB tasks
(e.g., generate/apply per project workflow: run pnpm db:migrate if you need to
generate migrations or pnpm db:apply then pnpm db:setup to ensure db/schema.sql
and the live DB are consistent).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/$orgSlug/settings/data-management/index.tsx (1)
63-75:⚠️ Potential issue | 🟠 MajorRecalculate トリガーにも同様のエラーハンドリングが必要です。
バリデーションエラー（lines 53-61）は適切に処理されていますが、
serverDurably.jobs.recalculate.trigger()の呼び出し自体が失敗した場合のハンドリングがありません。Refresh と同様に try/catch で囲む必要があります。🛡️ エラーハンドリング追加の提案
.with('recalculate', async () => { const selectedSteps = formData.getAll('steps').map(String) const steps = { upsert: selectedSteps.includes('upsert'), classify: selectedSteps.includes('classify'), export: selectedSteps.includes('export'), } satisfies JobSteps if (!steps.upsert && !steps.classify && !steps.export) { return data( { intent: 'recalculate' as const, error: 'At least one step must be selected', }, { status: 400 }, ) } - const run = await serverDurably.jobs.recalculate.trigger( - { organizationId: org.id, steps }, - { - concurrencyKey: `recalculate:${org.id}`, - labels: { organizationId: org.id }, - }, - ) - - return data({ - intent: 'recalculate' as const, - ok: true, - runId: run.id, - }) + try { + const run = await serverDurably.jobs.recalculate.trigger( + { organizationId: org.id, steps }, + { + concurrencyKey: `recalculate:${org.id}`, + labels: { organizationId: org.id }, + }, + ) + + return data({ + intent: 'recalculate' as const, + ok: true, + runId: run.id, + }) + } catch { + return data( + { + intent: 'recalculate' as const, + ok: false, + error: 'Recalculate の開始に失敗しました。時間をおいて再試行してください。', + }, + { status: 502 }, + ) + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/data-management/index.tsx around lines 63 - 75, The recalculate trigger call using serverDurably.jobs.recalculate.trigger(…) lacks error handling; wrap the trigger invocation (where run is assigned) in a try/catch mirroring the Refresh path, catching thrown errors and returning a data response with intent: 'recalculate', ok: false and an error message (or include error details) instead of assuming success, and ensure the catch also logs or handles the error consistent with the existing refresh error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/routes/`$orgSlug/settings/data-management/index.tsx:
- Around line 63-75: The recalculate trigger call using
serverDurably.jobs.recalculate.trigger(…) lacks error handling; wrap the trigger
invocation (where run is assigned) in a try/catch mirroring the Refresh path,
catching thrown errors and returning a data response with intent: 'recalculate',
ok: false and an error message (or include error details) instead of assuming
success, and ensure the catch also logs or handles the error consistent with the
existing refresh error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f5cba8c-3730-4504-b22b-19c11ba1cace
📒 Files selected for processing (1)
app/routes/$orgSlug/settings/data-management/index.tsx
Wrap durably.jobs.crawl.trigger in try/catch so failures return a structured error instead of a 500. Add error alert in RefreshSection UI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
schema.sql was a pre-multi-tenant remnant not referenced by atlas.hcl. Atlas uses shared.sql and tenant.sql as schema sources. Update CLAUDE.md project structure and comments to reflect the actual dual-schema setup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/$orgSlug/settings/data-management/index.tsx (1)
69-81:⚠️ Potential issue | 🟠 Major
recalculateトリガーにも try/catch が必要です。
refreshハンドラは try/catch で保護されていますが、recalculateは保護されていません。trigger()が失敗すると 500 エラーとなり、ユーザーには失敗理由が表示されません。🔧 修正案
- const run = await serverDurably.jobs.recalculate.trigger( - { organizationId: org.id, steps }, - { - concurrencyKey: `recalculate:${org.id}`, - labels: { organizationId: org.id }, - }, - ) - - return data({ - intent: 'recalculate' as const, - ok: true, - runId: run.id, - }) + try { + const run = await serverDurably.jobs.recalculate.trigger( + { organizationId: org.id, steps }, + { + concurrencyKey: `recalculate:${org.id}`, + labels: { organizationId: org.id }, + }, + ) + return data({ + intent: 'recalculate' as const, + ok: true, + runId: run.id, + }) + } catch { + return data( + { intent: 'recalculate' as const, error: 'Failed to start recalculation' }, + { status: 500 }, + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/data-management/index.tsx around lines 69 - 81, Wrap the call to serverDurably.jobs.recalculate.trigger in a try/catch: call serverDurably.jobs.recalculate.trigger(...) inside a try block, assign the result to run as before, and on success return the existing data payload with intent/recalculate and runId; in the catch block log or capture the error (including the thrown error details) and return a data response indicating ok: false and an error message (or include the error in the response) so failures from serverDurably.jobs.recalculate.trigger are handled gracefully instead of causing a 500.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/routes/`$orgSlug/settings/data-management/index.tsx:
- Around line 69-81: Wrap the call to serverDurably.jobs.recalculate.trigger in
a try/catch: call serverDurably.jobs.recalculate.trigger(...) inside a try
block, assign the result to run as before, and on success return the existing
data payload with intent/recalculate and runId; in the catch block log or
capture the error (including the thrown error details) and return a data
response indicating ok: false and an error message (or include the error in the
response) so failures from serverDurably.jobs.recalculate.trigger are handled
gracefully instead of causing a 500.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a841833e-434e-4d77-a14d-debf87364b4d
📒 Files selected for processing (1)
app/routes/$orgSlug/settings/data-management/index.tsx
Wrap durably.jobs.recalculate.trigger in try/catch to match the refresh handler, so failures return a structured error instead of a 500. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add triggerError prop to RunStatusAlerts to eliminate duplicated fetcher error alert blocks in RefreshSection and RecalculateSection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/routes/$orgSlug/settings/data-management/index.tsx (1)
28-33:intent/stepsの入力はparseWithZod+zxに寄せるのを推奨します。現在の
formData.get*ベースだと型安全性が弱く、許可外値の混入を見落としやすいです。ルート方針に合わせて Zod スキーマで受ける形に揃えると保守しやすくなります。As per coding guidelines
app/routes/**/*.ts{,x}: Use Conform with Zod for type-safe form validation: parseWithZod from@conform-to/zod/v4and zx from@coji/zodix/v4``.Also applies to: 51-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/data-management/index.tsx around lines 28 - 33, Replace the manual formData.get usage in the exported action with a Zod-backed parse flow: define a Zod schema (e.g., IntentSchema) describing allowed intent/steps values, then call parseWithZod(request, IntentSchema) and use zx (from `@coji/zodix/v4`) to extract typed values; update the code paths that currently read the raw intent variable (the intent constant in action) to use the validated, typed result instead so only allowed values proceed. Ensure you reference parseWithZod and zx imports, validate both intent and steps per the schema, and return/handle errors from parseWithZod consistently inside action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/routes/`$orgSlug/settings/data-management/index.tsx:
- Around line 28-33: Replace the manual formData.get usage in the exported
action with a Zod-backed parse flow: define a Zod schema (e.g., IntentSchema)
describing allowed intent/steps values, then call parseWithZod(request,
IntentSchema) and use zx (from `@coji/zodix/v4`) to extract typed values; update
the code paths that currently read the raw intent variable (the intent constant
in action) to use the validated, typed result instead so only allowed values
proceed. Ensure you reference parseWithZod and zx imports, validate both intent
and steps per the schema, and return/handle errors from parseWithZod
consistently inside action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98ea7152-0357-4a00-9b02-a140b5ea1718
📒 Files selected for processing (3)
CLAUDE.mdapp/routes/$orgSlug/settings/data-management/index.tsxdb/schema.sql
💤 Files with no reviewable changes (1)
- db/schema.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
refreshRequestedAtDB flag + hourly polling flow with a direct durably crawl job trigger from the UIRunStatusAlertscomponent to deduplicate progress/status UI between Refresh and Recalculate sectionsrefresh_requested_atcolumn fromorganization_settings(tenant migration)Closes #206 (refresh scheduling candidate)
Test plan
refresh: false)pnpm db:setupto verify migration applies cleanlypnpm validatepasses🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
改善
Chores