OpenConceptLab/ocl_online#81 | Using AI Assistant to generate change comment in concept#25
OpenConceptLab/ocl_online#81 | Using AI Assistant to generate change comment in concept#25snyaggarwal wants to merge 4 commits into
Conversation
paynejd
left a comment
paynejd
left a comment
There was a problem hiding this comment.
Review changes as discussed — most are inline suggestions you can commit directly. Two items can't be expressed as suggestions and are noted below.
1. New i18n keys (can't suggest — translations.json isn't in this PR's diff)
Add to src/i18n/locales/en/translations.json (and es/zh, or they'll fall back to en):
// "common": { ...
"generate_with_ai": "Generate with AI",
// "concept": { ...
"ai_assistant_not_configured": "AI assistant is not configured for this environment.",
"make_change_before_generating": "Make a change to the concept before generating a comment.",
"generate_comment_aria": "Generate comment with AI",
"try_again_in_a_moment": "Try again in a moment."2. Backend auth (context, not a code change here)
Server-side, POST /prompts/{key}/$invoke/ calls check_auth_group(MAPPER_AI_ASSISTANT_GROUP) (mapper_ai_assistant) — it is not superuser-gated. So the front-end visibility gate (superuser / core_user) and the back-end permission (mapper_ai_assistant) are different groups. That's the intended "safe even if it doesn't yet work for everyone" state — worth a follow-up ticket to align the groups before opening this to all users.
paynejd
left a comment
paynejd
left a comment
There was a problem hiding this comment.
Reviewed against ocl_online#81. Overall a clean, faithful implementation — env-var plumbing mirrors the existing ANALYTICS_API pattern, token forwarding works via currentUserToken(), metadata-exclusion lists match the spec, and the loading/disabled/tooltip states cover the acceptance criteria. A few inline notes below, plus one cross-repo dependency on backend permissions.
| if(edit) | ||
| payload.update_comment = fields.comment.value |
There was a problem hiding this comment.
getValues() already puts comment in the payload, so the PUT now carries both comment and update_comment. Worth dropping the stray key. (This line is also effectively the fix that makes update comments persist at all — previously the value was only sent as comment, which the update endpoint ignores.)
| if(edit) | |
| payload.update_comment = fields.comment.value | |
| if(edit) { | |
| payload.update_comment = fields.comment.value | |
| delete payload.comment | |
| } |
| const { conceptClasses, datatypes, locales, nameTypes, descriptionTypes, fields } = this.state | ||
| const { conceptClasses, datatypes, locales, nameTypes, descriptionTypes, fields, generatingChangeComment } = this.state | ||
| const aiAssistantConfigured = Boolean(this.getAIAssistantURL()) | ||
| const canSeeGenerateComment = edit && (isSuperuser() || hasAuthGroup(getCurrentUser(), 'core_user')) |
There was a problem hiding this comment.
This gates the button to core_users in addition to superusers, but per the ticket the AI Assistant $invoke endpoint still requires superuser — it notes this "must be relaxed to allow staff/authenticated users before this feature ships (separate ticket needed)." @snyaggarwal could you take the backend permissions work so core_users can actually invoke this? Otherwise non-superuser core_users will see the button and hit a 403.
| /*eslint no-undef: 0*/ | ||
| getAIAssistantURL = () => window.AI_ASSISTANT_API_URL || process.env.AI_ASSISTANT_API_URL |
There was a problem hiding this comment.
Minor: the bare /*eslint no-undef: 0*/ disables the rule for the rest of the file. Since process is already declared via /*global process*/ at the top, scoping it to this one line is tighter:
| /*eslint no-undef: 0*/ | |
| getAIAssistantURL = () => window.AI_ASSISTANT_API_URL || process.env.AI_ASSISTANT_API_URL | |
| // eslint-disable-next-line no-undef | |
| getAIAssistantURL = () => window.AI_ASSISTANT_API_URL || process.env.AI_ASSISTANT_API_URL |
| "custom": "Personalizado", | ||
| "load_more": "Cargar más" | ||
| "load_more": "Cargar más", | ||
| "generate_with_ai": "Generar con IA" |
There was a problem hiding this comment.
common.generic_error is the catch-all fallback in generateChangeComment but only exists in en. Easy to round out while you're here:
| "generate_with_ai": "Generar con IA" | |
| "generate_with_ai": "Generar con IA", | |
| "generic_error": "Algo salió mal." |
| "custom": "自定义", | ||
| "none": "无", | ||
| "load_more": "加载更多", | ||
| "generate_with_ai": "使用 AI 生成", |
There was a problem hiding this comment.
Same here — add the generic_error fallback used by generateChangeComment:
| "generate_with_ai": "使用 AI 生成", | |
| "generate_with_ai": "使用 AI 生成", | |
| "generic_error": "出了些问题。", |
2 participants
Linked Issue
Refs: OpenConceptLab/ocl_online#81