feat:change model #2329
feat:change model #2329
Conversation
Summary of ChangesHello @thewindwillstop, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature that enables users to change the style of a character image while ensuring the character's identity is preserved. It involves adding a new API endpoint, implementing the core logic in the backend controller, integrating with the Recraft AI service for image processing, and providing thorough validation and testing for the new functionality. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to change a character's style while preserving its identity, primarily leveraging the Recraft API. The changes include a new API endpoint, controller logic, service implementation, and extensive tests. The implementation is solid, but I've identified a few areas for improvement regarding error handling, a logic bug in parameter validation, and code maintainability. Specifically, there's a bug that prevents users from setting preserve_identity to false, and another issue where PNG images are incorrectly saved with an .svg extension. Addressing these points will enhance the robustness and correctness of the new feature.
| if strength, err := strconv.ParseFloat(strengthStr, 64); err == nil { | ||
| params.Strength = strength | ||
| } |
There was a problem hiding this comment.
Parsing for the strength parameter silently ignores errors. If a user provides a non-numeric value (e.g., strength=abc), the error from strconv.ParseFloat is ignored, and the default value is used without notifying the user. This can be confusing. It's better to return an error for invalid input.
if strength, err := strconv.ParseFloat(strengthStr, 64); err == nil {
params.Strength = strength
} else {
replyWithCodeMsg(ctx, errorInvalidArgs, "Invalid value for strength parameter")
return
}
| } | ||
|
|
||
| // ChangeCharacterStyle changes character styling while preserving character identity and returns the styled image metadata. | ||
| func (ctrl *Controller) ChangeCharacterStyle(ctx context.Context, params *ChangeCharacterStyleParams, imageData []byte) (*ChangeCharacterStyleResponse, error) { |
There was a problem hiding this comment.
The public method ChangeCharacterStyle should validate its input parameters. Currently, it relies on the caller to perform validation. It's a good practice for public methods to validate their arguments to ensure robustness and prevent misuse. Please add a call to params.Validate() at the beginning of this function.
For example:
if ok, msg := params.Validate(); !ok {
return nil, errors.New(msg)
}| var errResp map[string]interface{} | ||
| _ = json.NewDecoder(resp.Body).Decode(&errResp) | ||
| logger.Printf("[RECRAFT] Error response body: %+v", errResp) |
There was a problem hiding this comment.
When handling a non-2xx response from the Recraft API, the error from decoding the JSON error response is ignored. If the error response is not valid JSON, the log message for errResp will be empty or misleading. It would be more robust to read the raw response body into a string and log that, ensuring you always capture the error details from the upstream service. You will need to import the io package.
| var errResp map[string]interface{} | |
| _ = json.NewDecoder(resp.Body).Decode(&errResp) | |
| logger.Printf("[RECRAFT] Error response body: %+v", errResp) | |
| bodyBytes, _ := io.ReadAll(resp.Body) | |
| logger.Printf("[RECRAFT] Error response body: %s", string(bodyBytes)) |
| // buildCharacterPreservationPrompt builds a prompt that emphasizes character preservation during style changes. | ||
| func (s *RecraftService) buildCharacterPreservationPrompt(stylePrompt string, preserveIdentity bool) string { | ||
| var promptBuilder strings.Builder | ||
|
|
||
| if preserveIdentity { | ||
| // Add character preservation instructions | ||
| promptBuilder.WriteString("保持角色的面部特征、体型和基本外观不变，") | ||
| promptBuilder.WriteString("只改变") | ||
| } | ||
|
|
||
| promptBuilder.WriteString(stylePrompt) | ||
|
|
||
| if preserveIdentity { | ||
| promptBuilder.WriteString("，确保角色身份完全保持不变，面部特征和体型必须保持一致") | ||
| } | ||
|
|
||
| return promptBuilder.String() | ||
| } | ||
|
|
||
| // buildCharacterPreservationNegativePrompt builds negative prompts to avoid unwanted character changes. | ||
| func (s *RecraftService) buildCharacterPreservationNegativePrompt(negativePrompt string) string { | ||
| var negativeBuilder strings.Builder | ||
|
|
||
| if negativePrompt != "" { | ||
| negativeBuilder.WriteString(negativePrompt) | ||
| negativeBuilder.WriteString(", ") | ||
| } | ||
|
|
||
| // Add character preservation negative prompts | ||
| negativeBuilder.WriteString("改变面部特征, 改变角色身份, 不同的人, 面部变形, 体型改变, 性别改变, 年龄变化") | ||
|
|
||
| return negativeBuilder.String() | ||
| } |
There was a problem hiding this comment.
The Chinese strings used to build the character preservation prompt and negative prompt are hardcoded inline within the buildCharacterPreservationPrompt and buildCharacterPreservationNegativePrompt functions. It's better to define these as constants at the top of the file. This improves readability and makes them easier to manage or change in the future.
For example:
const (
characterPreservationInstruction = "保持角色的面部特征、体型和基本外观不变，"
characterStyleChangeInstruction = "只改变"
// ... and so on for other strings
)
![niupilot[bot]](https://avatars.githubusercontent.com/in/1894735?s=60&v=4){kind=small}
| // Default to preserve identity if not specified | ||
| if !p.PreserveIdentity { | ||
| p.PreserveIdentity = true // Default to preserve identity | ||
| } |
There was a problem hiding this comment.
🐛 Critical Bug - Broken Default Logic
This condition is always true when PreserveIdentity is false (zero value for bool), making it impossible to explicitly set it to false.
| } | |
| // PreserveIdentity defaults to true if not set | |
| // Note: To support explicit false, consider using *bool instead |
Alternatively, remove this block and document that default is false, or use a pointer type *bool to distinguish between unset and explicitly false.
| ) | ||
|
|
||
| ctx := &Context | ||
| if _, ok := ensureAuthenticatedUser(ctx); !ok { |
There was a problem hiding this comment.
🔒 Security - Missing Quota/Rate Limiting
Unlike copilot endpoints, this doesn't enforce any quota limits. This could allow abuse of expensive Recraft API calls.
Consider adding quota enforcement similar to post_copilot_message.yap:
if err := authz.ConsumeQuota(ctx.Context(), authz.ResourceAIImageGen, 1); err != nil {
replyWithCodeMsg(ctx, errorTooManyRequests, "Image generation quota exceeded")
return
}
🔍 Code Review SummaryReviewed by specialized agents for code quality, performance, documentation, and security. Overall solid implementation with good test coverage, but critical issues require attention before merge. 🚨 Must Fix
|
|
This PR has been deployed to the preview environment. You can explore it using the preview URL. Warning Please note that deployments in the preview environment are temporary and will be automatically cleaned up after a certain period. Make sure to explore it before it is removed. For any questions, contact the Go+ Builder team. |
2 participants
petezhuang#154