Feat/3 diagnoses table r2 storage#1
Open
flvvius wants to merge 3 commits into
Open
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client as WhatsApp Client
participant Server as Webhook Handler
participant R2 as R2 Storage
participant DB as D1 Database
participant Meta as Meta API
participant Gemini as Gemini API
participant Weather as Weather Service
Client->>Server: POST image + farmer ID
Server->>Meta: Fetch media metadata
Meta-->>Server: MIME type
Server->>Meta: Download media bytes
Meta-->>Server: Media bytes
Server->>Gemini: Send image for diagnosis
Gemini-->>Server: Diagnosis text + confidence
Server->>R2: Save image file
R2-->>Server: Image stored
Server->>DB: INSERT diagnosis record
DB-->>Server: Record saved
Server->>Weather: Get treatment window (optional)
Weather-->>Server: Treatment recommendations
Server->>Client: Send WhatsApp reply with diagnosis + treatment window
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (1)
src/index.ts (1)
205-221: Avoid maintaining a third copy of thediagnosesDDL.The same table definition now exists in
drizzle/0003_diagnoses.sql,src/db/migrate.ts, and here. The next schema change has three places to keep in sync, which is easy to miss. Consider reusing a shared SQL constant here or relying on migrations plus a lighter readiness check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 205 - 221, The CREATE TABLE DDL inside ensureDiagnosesTable is a third copy of the diagnoses schema; instead remove the inline DDL in ensureDiagnosesTable and either import and reuse a single shared SQL constant (from the existing migration module that exposes the diagnoses DDL or a new exported constant) or replace this heavy migration-style SQL with a lightweight readiness check that queries env.DB for the diagnoses table existence; update the function ensureDiagnosesTable to use the shared constant or the lighter existence check so schema changes live in one place (e.g., the migration file or exported constant) rather than duplicated here.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 11: The db:rows npm script (the "db:rows" entry invoking wrangler d1
execute with a UNION ALL of three SELECT COUNT(*) queries) fails if the
diagnoses table is missing; change the script so the diagnoses branch is guarded
by an existence check against sqlite_master (only run the COUNT(*) FROM
diagnoses if the table exists) or split the single SQL into separate wrangler d1
execute calls and safely ignore missing-table errors for the diagnoses query;
update the SQL/commands referenced in the package.json "db:rows" script
accordingly so remote environments without that migration do not error.
In `@src/index.ts`:
- Around line 393-423: After diagnoseFromWhatsAppMedia returns, ensure
delivering result.diagnosisText to the user does not depend on downstream work:
move or add the user-reply/send step so it runs immediately after
diagnoseFromWhatsAppMedia succeeds, then perform getOrCreateFarmer, BUCKET.put,
getWeatherTreatmentWindow and saveDiagnosis as best-effort operations each
wrapped in their own try/catch that logs failures (don’t rethrow). Specifically,
keep diagnoseFromWhatsAppMedia(result), send the reply with result.diagnosisText
(and optionally append weatherWindow only if enrichment succeeds), and wrap
calls to getOrCreateFarmer, c.env.BUCKET.put(imageKey,...),
getWeatherTreatmentWindow, and saveDiagnosis in isolated try/catch blocks so
storage/enrichment errors won’t trigger the outer catch or prevent the user from
receiving the diagnosis.
In `@src/services/weather.ts`:
- Around line 176-179: The recommendations array currently pushes "favorabil
stropire" based only on rain and separately pushes "vant puternic" based on
wind, allowing contradictory entries for the same day; change the logic around
the recommendations array so that "favorabil stropire" is only added when both
rain <= 1 and wind <= 25, and keep the "vant puternic" branch for wind > 25 (or
check wind first and avoid adding the favorable message if wind > 25),
referencing the existing variables recommendations, rain, and wind.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 205-221: The CREATE TABLE DDL inside ensureDiagnosesTable is a
third copy of the diagnoses schema; instead remove the inline DDL in
ensureDiagnosesTable and either import and reuse a single shared SQL constant
(from the existing migration module that exposes the diagnoses DDL or a new
exported constant) or replace this heavy migration-style SQL with a lightweight
readiness check that queries env.DB for the diagnoses table existence; update
the function ensureDiagnosesTable to use the shared constant or the lighter
existence check so schema changes live in one place (e.g., the migration file or
exported constant) rather than duplicated here.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c5355da-5391-4eb4-85f0-0dcd212553e7
📒 Files selected for processing (8)
drizzle/0003_diagnoses.sqlpackage.jsonsrc/db/migrate.tssrc/db/schema.tssrc/index.tssrc/services/diagnosis.tssrc/services/weather.tswrangler.toml
| "db:farmers": "wrangler d1 execute agrobot-db --remote --command \"SELECT id, phone, name, location, crops, onboarding_step, onboarding_completed, created_at, last_active FROM farmers ORDER BY last_active DESC LIMIT 50;\"", | ||
| "db:farmers:count": "wrangler d1 execute agrobot-db --remote --command \"SELECT COUNT(*) AS farmers_count FROM farmers;\"", | ||
| "db:rows": "wrangler d1 execute agrobot-db --remote --command \"SELECT 'farmers' AS table_name, COUNT(*) AS rows_count FROM farmers UNION ALL SELECT 'price_cache' AS table_name, COUNT(*) AS rows_count FROM price_cache;\"" | ||
| "db:rows": "wrangler d1 execute agrobot-db --remote --command \"SELECT 'farmers' AS table_name, COUNT(*) AS rows_count FROM farmers UNION ALL SELECT 'price_cache' AS table_name, COUNT(*) AS rows_count FROM price_cache UNION ALL SELECT 'diagnoses' AS table_name, COUNT(*) AS rows_count FROM diagnoses;\"" |
There was a problem hiding this comment.
Make db:rows tolerant of unmigrated databases.
SELECT COUNT(*) FROM diagnoses will fail on a remote environment that has not applied this migration yet, so the whole inspection script breaks during rollout. Guard that branch with a sqlite_master existence check or split it into separate commands.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 11, The db:rows npm script (the "db:rows" entry
invoking wrangler d1 execute with a UNION ALL of three SELECT COUNT(*) queries)
fails if the diagnoses table is missing; change the script so the diagnoses
branch is guarded by an existence check against sqlite_master (only run the
COUNT(*) FROM diagnoses if the table exists) or split the single SQL into
separate wrangler d1 execute calls and safely ignore missing-table errors for
the diagnoses query; update the SQL/commands referenced in the package.json
"db:rows" script accordingly so remote environments without that migration do
not error.
Comment on lines
+393
to
+423
| const result = await diagnoseFromWhatsAppMedia(imageMediaId, c.env); | ||
| const { farmer } = await getOrCreateFarmer(c.env, from); | ||
|
|
||
| let imageKey = ""; | ||
| if (c.env.BUCKET) { | ||
| const extension = getExtensionFromMimeType(result.mimeType); | ||
| imageKey = `incoming/${from}/${Date.now()}-${imageMediaId}.${extension}`; | ||
| await c.env.BUCKET.put(imageKey, result.mediaBytes, { | ||
| httpMetadata: { contentType: result.mimeType }, | ||
| }); | ||
| } | ||
|
|
||
| const weatherWindow = farmer.location | ||
| ? await getWeatherTreatmentWindow(farmer.location) | ||
| : null; | ||
|
|
||
| const finalReply = weatherWindow | ||
| ? `${result.diagnosisText}\n\n${weatherWindow}` | ||
| : result.diagnosisText; | ||
|
|
||
| await saveDiagnosis(c.env, { | ||
| id: crypto.randomUUID(), | ||
| farmer_id: from, | ||
| image_key: imageKey, | ||
| diagnosis: result.diagnosisSummary, | ||
| confidence: result.confidence, | ||
| gemini_response: result.rawModelText, | ||
| feedback_correct: null, | ||
| created_at: Math.floor(Date.now() / 1000), | ||
| }); | ||
|
|
There was a problem hiding this comment.
Keep diagnosis delivery independent from storage/enrichment failures.
After diagnoseFromWhatsAppMedia() succeeds, any failure in farmer lookup, R2 upload, weather lookup, or saveDiagnosis() drops into the outer catch and sends the user a false "Nu pot identifica clar..." failure. It also risks orphaning an already-uploaded image. These steps should be best-effort; the user should still receive result.diagnosisText.
Suggested structure
- const { farmer } = await getOrCreateFarmer(c.env, from);
+ let farmerLocation: string | null = null;
+ if (c.env.DB) {
+ try {
+ const { farmer } = await getOrCreateFarmer(c.env, from);
+ farmerLocation = farmer.location;
+ } catch (error) {
+ console.error("[webhook] farmer lookup failed", {
+ from,
+ error: error instanceof Error ? error.message : String(error),
+ });
+ }
+ }
let imageKey = "";
if (c.env.BUCKET) {
- const extension = getExtensionFromMimeType(result.mimeType);
- imageKey = `incoming/${from}/${Date.now()}-${imageMediaId}.${extension}`;
- await c.env.BUCKET.put(imageKey, result.mediaBytes, {
- httpMetadata: { contentType: result.mimeType },
- });
+ try {
+ const extension = getExtensionFromMimeType(result.mimeType);
+ imageKey = `incoming/${from}/${Date.now()}-${imageMediaId}.${extension}`;
+ await c.env.BUCKET.put(imageKey, result.mediaBytes, {
+ httpMetadata: { contentType: result.mimeType },
+ });
+ } catch (error) {
+ console.error("[webhook] R2 upload failed", {
+ from,
+ error: error instanceof Error ? error.message : String(error),
+ });
+ imageKey = "";
+ }
}
- const weatherWindow = farmer.location
- ? await getWeatherTreatmentWindow(farmer.location)
- : null;
+ let weatherWindow: string | null = null;
+ if (farmerLocation) {
+ try {
+ weatherWindow = await getWeatherTreatmentWindow(farmerLocation);
+ } catch (error) {
+ console.error("[webhook] weather lookup failed", {
+ from,
+ error: error instanceof Error ? error.message : String(error),
+ });
+ }
+ }
- await saveDiagnosis(c.env, {
- id: crypto.randomUUID(),
- farmer_id: from,
- image_key: imageKey,
- diagnosis: result.diagnosisSummary,
- confidence: result.confidence,
- gemini_response: result.rawModelText,
- feedback_correct: null,
- created_at: Math.floor(Date.now() / 1000),
- });
+ try {
+ await saveDiagnosis(c.env, {
+ id: crypto.randomUUID(),
+ farmer_id: from,
+ image_key: imageKey,
+ diagnosis: result.diagnosisSummary,
+ confidence: result.confidence,
+ gemini_response: result.rawModelText,
+ feedback_correct: null,
+ created_at: Math.floor(Date.now() / 1000),
+ });
+ } catch (error) {
+ console.error("[webhook] diagnosis persistence failed", {
+ from,
+ error: error instanceof Error ? error.message : String(error),
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const result = await diagnoseFromWhatsAppMedia(imageMediaId, c.env); | |
| const { farmer } = await getOrCreateFarmer(c.env, from); | |
| let imageKey = ""; | |
| if (c.env.BUCKET) { | |
| const extension = getExtensionFromMimeType(result.mimeType); | |
| imageKey = `incoming/${from}/${Date.now()}-${imageMediaId}.${extension}`; | |
| await c.env.BUCKET.put(imageKey, result.mediaBytes, { | |
| httpMetadata: { contentType: result.mimeType }, | |
| }); | |
| } | |
| const weatherWindow = farmer.location | |
| ? await getWeatherTreatmentWindow(farmer.location) | |
| : null; | |
| const finalReply = weatherWindow | |
| ? `${result.diagnosisText}\n\n${weatherWindow}` | |
| : result.diagnosisText; | |
| await saveDiagnosis(c.env, { | |
| id: crypto.randomUUID(), | |
| farmer_id: from, | |
| image_key: imageKey, | |
| diagnosis: result.diagnosisSummary, | |
| confidence: result.confidence, | |
| gemini_response: result.rawModelText, | |
| feedback_correct: null, | |
| created_at: Math.floor(Date.now() / 1000), | |
| }); | |
| const result = await diagnoseFromWhatsAppMedia(imageMediaId, c.env); | |
| let farmerLocation: string | null = null; | |
| if (c.env.DB) { | |
| try { | |
| const { farmer } = await getOrCreateFarmer(c.env, from); | |
| farmerLocation = farmer.location; | |
| } catch (error) { | |
| console.error("[webhook] farmer lookup failed", { | |
| from, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } | |
| } | |
| let imageKey = ""; | |
| if (c.env.BUCKET) { | |
| try { | |
| const extension = getExtensionFromMimeType(result.mimeType); | |
| imageKey = `incoming/${from}/${Date.now()}-${imageMediaId}.${extension}`; | |
| await c.env.BUCKET.put(imageKey, result.mediaBytes, { | |
| httpMetadata: { contentType: result.mimeType }, | |
| }); | |
| } catch (error) { | |
| console.error("[webhook] R2 upload failed", { | |
| from, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| imageKey = ""; | |
| } | |
| } | |
| let weatherWindow: string | null = null; | |
| if (farmerLocation) { | |
| try { | |
| weatherWindow = await getWeatherTreatmentWindow(farmerLocation); | |
| } catch (error) { | |
| console.error("[webhook] weather lookup failed", { | |
| from, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } | |
| } | |
| const finalReply = weatherWindow | |
| ? `${result.diagnosisText}\n\n${weatherWindow}` | |
| : result.diagnosisText; | |
| try { | |
| await saveDiagnosis(c.env, { | |
| id: crypto.randomUUID(), | |
| farmer_id: from, | |
| image_key: imageKey, | |
| diagnosis: result.diagnosisSummary, | |
| confidence: result.confidence, | |
| gemini_response: result.rawModelText, | |
| feedback_correct: null, | |
| created_at: Math.floor(Date.now() / 1000), | |
| }); | |
| } catch (error) { | |
| console.error("[webhook] diagnosis persistence failed", { | |
| from, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 393 - 423, After diagnoseFromWhatsAppMedia
returns, ensure delivering result.diagnosisText to the user does not depend on
downstream work: move or add the user-reply/send step so it runs immediately
after diagnoseFromWhatsAppMedia succeeds, then perform getOrCreateFarmer,
BUCKET.put, getWeatherTreatmentWindow and saveDiagnosis as best-effort
operations each wrapped in their own try/catch that logs failures (don’t
rethrow). Specifically, keep diagnoseFromWhatsAppMedia(result), send the reply
with result.diagnosisText (and optionally append weatherWindow only if
enrichment succeeds), and wrap calls to getOrCreateFarmer,
c.env.BUCKET.put(imageKey,...), getWeatherTreatmentWindow, and saveDiagnosis in
isolated try/catch blocks so storage/enrichment errors won’t trigger the outer
catch or prevent the user from receiving the diagnosis.
Comment on lines
+176
to
+179
| const recommendations: string[] = []; | ||
| if (rain <= 1) recommendations.push("favorabil stropire"); | ||
| if (rain > 1) recommendations.push("evita stropire inainte de ploaie"); | ||
| if (wind > 25) recommendations.push("vant puternic"); |
There was a problem hiding this comment.
Don't mark high-wind days as favorable for spraying.
Line 177 only checks rain, so a 0 mm / 40 km/h day becomes both favorabil stropire and vant puternic. That is contradictory guidance for treatment timing.
Suggested fix
- if (rain <= 1) recommendations.push("favorabil stropire");
- if (rain > 1) recommendations.push("evita stropire inainte de ploaie");
- if (wind > 25) recommendations.push("vant puternic");
+ if (rain > 1) {
+ recommendations.push("evita stropire inainte de ploaie");
+ } else if (wind > 25) {
+ recommendations.push("evita stropire (vant puternic)");
+ } else {
+ recommendations.push("favorabil stropire");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const recommendations: string[] = []; | |
| if (rain <= 1) recommendations.push("favorabil stropire"); | |
| if (rain > 1) recommendations.push("evita stropire inainte de ploaie"); | |
| if (wind > 25) recommendations.push("vant puternic"); | |
| const recommendations: string[] = []; | |
| if (rain > 1) { | |
| recommendations.push("evita stropire inainte de ploaie"); | |
| } else if (wind > 25) { | |
| recommendations.push("evita stropire (vant puternic)"); | |
| } else { | |
| recommendations.push("favorabil stropire"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/weather.ts` around lines 176 - 179, The recommendations array
currently pushes "favorabil stropire" based only on rain and separately pushes
"vant puternic" based on wind, allowing contradictory entries for the same day;
change the logic around the recommendations array so that "favorabil stropire"
is only added when both rain <= 1 and wind <= 25, and keep the "vant puternic"
branch for wind > 25 (or check wind first and avoid adding the favorable message
if wind > 25), referencing the existing variables recommendations, rain, and
wind.
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.
No description provided.