-
Notifications
You must be signed in to change notification settings - Fork 4
feat: deprecate integration_id in favor of deployment_id #26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Update codebase and documentation to use deployment_id as the standard parameter name. integration_id is deprecated but remains supported for backwards compatibility.
📝 WalkthroughWalkthroughThis pull request migrates the codebase from using Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
|
🔒 Entelligence AI Vulnerability Scanner ✅ No security vulnerabilities found! Your code passed our comprehensive security analysis. 📊 Files Analyzed: 6 files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
30-30: Consider minor grammar improvements.The deprecation note is clear and helpful. However, there are two minor grammar issues:
- "Deploy tab" → Consider "Deployments tab" or "deployment settings"
- Missing comma: "backwards compatibility but is deprecated" → "backwards compatibility, but is deprecated"
Apply this diff:
-2. Replace `"Kbo"` with your Deployment ID, as it appears on the Copilot's Deploy tab. (Note: `integration_id` is still supported for backwards compatibility but is deprecated.) +2. Replace `"Kbo"` with your Deployment ID, as it appears on the Copilot's Deployments tab. (Note: `integration_id` is still supported for backwards compatibility, but is deprecated.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(5 hunks)src/api/streaming.ts(1 hunks)src/contexts/MessagesContext.tsx(4 hunks)src/contexts/types.ts(1 hunks)src/lib.tsx(1 hunks)src/main.js(1 hunks)src/widgets/index.tsx(1 hunks)test.html(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~30-~30: The word ‘Deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...ment ID, as it appears on the Copilot's Deploy tab. (Note: integration_id is still s...
(PREPOSITION_VERB)
[uncategorized] ~30-~30: Possible missing comma found.
Context: ...ll supported for backwards compatibility but is deprecated.) ## Configuration Optio...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~149-~149: Loose punctuation mark.
Context: ...party services. - GOOGLE_MAPS_API_KEY: Optional Google Maps API key that enabl...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
src/api/streaming.ts (1)
24-24: LGTM! Endpoint path updated correctly.The migration from
/v3/integrations/stream/to/v3/deployments/stream/aligns with the broader API migration to deployment-based terminology.src/main.js (1)
25-25: LGTM! Default configuration updated correctly.The configuration key has been properly renamed from
integration_idtodeployment_id, aligning with the new standard.src/contexts/MessagesContext.tsx (4)
182-185: LGTM! Conversation initialization updated correctly.The migration to
deployment_idis clean. The backward compatibility is handled upstream insrc/widgets/index.tsx(lines 47-49), which normalizesconfig.deployment_idto fall back toconfig.integration_idbefore this context is initialized.
327-328: LGTM! Bot ID assignment updated correctly.The
bot_idnow correctly referencesconfig?.deployment_id, consistent with the broader migration.
366-366: LGTM! Dependency array updated correctly.The callback dependency correctly references
config?.deployment_idto ensure proper memoization.
381-381: LGTM! Payload construction updated correctly.The payload now correctly includes
deployment_idfor API calls, aligning with the updated endpoint path insrc/api/streaming.ts.src/widgets/index.tsx (1)
47-49: Excellent backward compatibility implementation!The normalization logic elegantly handles the migration by preferring
deployment_idwhile falling back tointegration_idfor backward compatibility. This ensures all downstream code can consistently usedeployment_idwithout additional checks.test.html (1)
13-14: LGTM! Test usage updated correctly.The test file now demonstrates the preferred usage pattern with
deployment_idinstead of the deprecatedintegration_id.src/contexts/types.ts (1)
3-5: LGTM! Type definitions updated correctly.The type definition properly reflects the migration:
deployment_idis now the required fieldintegration_idis optional with a clear deprecation noticeThis aligns with the normalization logic in
src/widgets/index.tsxand provides helpful guidance to developers through the JSDoc comment.README.md (3)
19-19: LGTM! Example code updated correctly.The example now demonstrates the preferred
deployment_idparameter.
83-87: LGTM! Parameter documentation updated correctly.The documentation clearly identifies
deployment_idas required, explains the snake_case convention, and provides a clear deprecation notice forintegration_id.
145-150: LGTM! Secrets section documented clearly.The new
secretsconfiguration section is well-documented with clear guidance on the optional Google Maps API key and its fallback behavior.
| secrets: { | ||
| GOOGLE_MAPS_API_KEY: "your-google-maps-api-key-here", | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove duplicate secrets configuration.
The secrets configuration block appears twice in the example (lines 45-47 and again at lines 65-67). This duplication could confuse users.
Apply this diff to remove the duplicate:
branding: {
name: "Farmer.CHAT",
byLine: "By Digital Green",
description:
"An AI Assistant designed to help farmer extension agents in India.",
conversationStarters: [
"When should I plant chili?",
"How can I get rid of black ants on my coffee?",
"What is the best time to grow pepper in India?",
],
fabLabel: "Help",
photoUrl:
"https://digitalgreen.org/wp-content/themes/digital-green/images/favicons/apple-touch-icon.png",
websiteUrl: "https://digitalgreen.org/",
showPoweredByGooey: true,
color: { primary: "purple", "secondary": "blue" },
},
- secrets: {
- GOOGLE_MAPS_API_KEY: "your-google-maps-api-key-here",
- },
payload: {
user_id: "123",
variables: {
product_id: "sample_id",
product_name: "sample_name",
},
},Also applies to: 65-67
🤖 Prompt for AI Agents
In README.md around lines 45-47 and again at lines 65-67, the example contains a
duplicated secrets configuration block (GOOGLE_MAPS_API_KEY). Remove the second
duplicate block (lines 65-67) so the secrets section appears only once, and
update any surrounding commas or formatting to keep the YAML/JSON example valid
after removal.
| if (!config.integration_id && !config.deployment_id) { | ||
| throw new Error( | ||
| `Integration ID is required. Please provide an "integration_id" in the config object.`, | ||
| `Deployment ID is required. Please provide an "deployment_id" in the config object.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix article in error message.
The validation logic correctly maintains backward compatibility by accepting either integration_id or deployment_id. However, there's a grammatical error in the message.
Apply this diff:
- `Deployment ID is required. Please provide an "deployment_id" in the config object.`,
+ `Deployment ID is required. Please provide a "deployment_id" in the config object.`,📝 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.
| if (!config.integration_id && !config.deployment_id) { | |
| throw new Error( | |
| `Integration ID is required. Please provide an "integration_id" in the config object.`, | |
| `Deployment ID is required. Please provide an "deployment_id" in the config object.`, | |
| ); | |
| } | |
| if (!config.integration_id && !config.deployment_id) { | |
| throw new Error( | |
| `Deployment ID is required. Please provide a "deployment_id" in the config object.`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/lib.tsx around lines 21 to 25, the error message uses incorrect article
wording; update the thrown Error text to use proper grammar such as "A
deployment ID is required. Please provide a \"deployment_id\" in the config
object." so the message reads clearly and consistently.
Review Summary🏷️ Draft Comments (4)
🔍 Comments beyond diff scope (2)
|
Update codebase and documentation to use deployment_id as the standard parameter name. integration_id is deprecated but remains supported for backwards compatibility.
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.