[chore: grouped allwedusers, users, responseStyle, tone, response_foramt, reasponseStylePrompt , fallback and guardrails under settings#879
Open
Anushtha-Rathore wants to merge 1 commit intoWalkover-Web-Solution:testingfrom
Conversation
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Other comments (6)
- src/controllers/agentConfig.controller.js (173-176) The PR removes `response_format` and `fall_back` objects but doesn't show how these are now structured within the new `settings` object. Make sure that any code that previously accessed these fields is updated to use the new path (`settings.response_format`, `settings.fall_back`, etc.) to prevent undefined behavior.
- src/controllers/agentConfig.controller.js (206-206) The PR removes the default value for `fall_back` when creating an agent but doesn't provide a default value for the new `settings` field. Consider adding a default structure for `settings` to ensure backward compatibility with code that might expect certain fields to exist.
- src/controllers/template.controller.js (219-219) The `response_format` field is added to FILTER_BRIDGE_EXCLUDE_KEYS and removed from direct initialization in model_data, but I don't see where it's being initialized under the new settings structure. This could lead to undefined behavior if code elsewhere expects this field to be initialized.
- src/controllers/template.controller.js (428-428) The `response_format` initialization is also removed from child agent creation without replacement. For consistency, if this field is now expected to be under settings, there should be a similar initialization pattern for child agents.
- src/controllers/agentConfig.controller.js (321-327) The PR title mentions grouping several fields under settings (allowedusers, users, responseStyle, tone, response_format, responseStylePrompt, fallback, guardrails) but the diff only shows changes for `fall_back` and `guardrails`. Please ensure all mentioned fields are properly migrated to the settings object.
- src/validation/joi_validation/agentConfig.validation.js (87-87) There are typos in the PR title and description: 'allwedusers' should be 'allowedUsers', 'response_foramt' should be 'response_format', and 'reasponseStylePrompt' should be 'responseStylePrompt'. Consider updating these for clarity.
💡 To request another review, post a new comment with "/windsurf-review".
| { | ||
| "page_config.availability": "private", | ||
| "page_config.allowedUsers": userEmail | ||
| "settings.publicUsers": userEmail |
Contributor
There was a problem hiding this comment.
There's an inconsistency between the PR title/description (which mentions 'allwedusers') and the actual code changes (which rename 'allowedUsers' to 'publicUsers'). Please update either the PR title/description or the code to ensure consistency.
Comment on lines
+1160
to
1169
| criteria_check: 1, | ||
| "settings.publicUsers": 1, | ||
| "settings.responseStyle": 1, | ||
| "settings.tone": 1, | ||
| "settings.responseStylePrompt": 1, | ||
| "settings.tonePrompt": 1, | ||
| "settings.response_format": 1, | ||
| "settings.fall_back": 1, | ||
| "settings.guardrails": 1 | ||
| }) |
Contributor
There was a problem hiding this comment.
This PR moves fields from the root level or page_config namespace to the settings namespace. Is there a data migration plan to handle existing documents that use the old field structure? Without migration, this change might break functionality for existing agents.
| service: Joi.string().optional(), | ||
| model: Joi.string().optional() | ||
| settings: Joi.object({ | ||
| publicUsers: Joi.array().items(Joi.string()).optional(), |
Contributor
There was a problem hiding this comment.
The PR title mentions 'allowedUsers', but the code adds 'publicUsers'. Is this intentional or should these be the same field name?
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.
[chore: grouped allwedusers, users, responseStyle, tone, response_foramt, reasponseStylePrompt , fallback and guardrails under settings