fix: colors of the prompts for the threat level retro match now the prompts' names #9956
Conversation
WalkthroughThe change introduces a new migration script to update the colours in the Changes
Assessment against linked issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
packages/server/postgres/migrations/1720689742934_fixThreatLevelPromptColors.ts
Outdated
Show resolved
Hide resolved
packages/server/postgres/migrations/1720689742934_fixThreatLevelPromptColors.ts
Outdated
Show resolved
Hide resolved
rafaelromcar-parabol
requested review from
Dschoordsch and
mattkrick
and removed request for
tianrunhe and
Dschoordsch
|
|
||
| await r | ||
| .table('ReflectPrompt') | ||
| .filter({id: 'threatLevelPremortemTemplateSeverePrompt'}) |
There was a problem hiding this comment.
-1 use get instead of filter, otherwise it won't use the index
There was a problem hiding this comment.
Done and tested locally again both migrating up and down! Thanks for the correction!
| export async function down() { | ||
| await connectRethinkDB() | ||
|
|
||
| await r | ||
| .table('ReflectPrompt') | ||
| .get('threatLevelPremortemTemplateSeverePrompt') | ||
| .update({groupColor: '#66BC8C'}) | ||
| .run() | ||
| await r | ||
| .table('ReflectPrompt') | ||
| .get('threatLevelPremortemTemplateElevatedPrompt') | ||
| .update({groupColor: '#FD6157'}) | ||
| .run() | ||
| await r | ||
| .table('ReflectPrompt') | ||
| .get('threatLevelPremortemTemplateLowPrompt') | ||
| .update({groupColor: '#FFCC63'}) | ||
| .run() |
There was a problem hiding this comment.
Ensure consistent error handling across migrations.
The same error handling improvement suggested for the up function should be applied here.
} catch (e) {
console.error('Error during migration:', e)
+ throw e
}Committable suggestion was skipped due to low confidence.
Optimize the revert process for efficiency.
Similar to the up function, consider batching these updates into a single transaction to improve performance.
- await r.table('ReflectPrompt').get('threatLevelPremortemTemplateSeverePrompt').update({groupColor: '#66BC8C'}).run()
- await r.table('ReflectPrompt').get('threatLevelPremortemTemplateElevatedPrompt').update({groupColor: '#FD6157'}).run()
- await r.table('ReflectPrompt').get('threatLevelPremortemTemplateLowPrompt').update({groupColor: '#FFCC63'}).run()
+ await r.table('ReflectPrompt').getAll('threatLevelPremortemTemplateSeverePrompt', 'threatLevelPremortemTemplateElevatedPrompt', 'threatLevelPremortemTemplateLowPrompt').update(function(doc) {
+ return r.branch(
+ doc('id').eq('threatLevelPremortemTemplateSeverePrompt'), {groupColor: '#66BC8C'},
+ doc('id').eq('threatLevelPremortemTemplateElevatedPrompt'), {groupColor: '#FD6157'},
+ doc('id').eq('threatLevelPremortemTemplateLowPrompt'), {groupColor: '#FFCC63'}
+ )
+ }).run()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.
| export async function down() { | |
| await connectRethinkDB() | |
| await r | |
| .table('ReflectPrompt') | |
| .get('threatLevelPremortemTemplateSeverePrompt') | |
| .update({groupColor: '#66BC8C'}) | |
| .run() | |
| await r | |
| .table('ReflectPrompt') | |
| .get('threatLevelPremortemTemplateElevatedPrompt') | |
| .update({groupColor: '#FD6157'}) | |
| .run() | |
| await r | |
| .table('ReflectPrompt') | |
| .get('threatLevelPremortemTemplateLowPrompt') | |
| .update({groupColor: '#FFCC63'}) | |
| .run() | |
| await r.table('ReflectPrompt') | |
| .getAll('threatLevelPremortemTemplateSeverePrompt', 'threatLevelPremortemTemplateElevatedPrompt', 'threatLevelPremortemTemplateLowPrompt') | |
| .update(function(doc) { | |
| return r.branch( | |
| doc('id').eq('threatLevelPremortemTemplateSeverePrompt'), {groupColor: '#66BC8C'}, | |
| doc('id').eq('threatLevelPremortemTemplateElevatedPrompt'), {groupColor: '#FD6157'}, | |
| doc('id').eq('threatLevelPremortemTemplateLowPrompt'), {groupColor: '#FFCC63'} | |
| ) | |
| }) | |
| .run() |
| export async function up() { | ||
| await connectRethinkDB() | ||
|
|
||
| await r | ||
| .table('ReflectPrompt') | ||
| .get('threatLevelPremortemTemplateSeverePrompt') | ||
| .update({groupColor: '#FD6157'}) | ||
| .run() | ||
| await r | ||
| .table('ReflectPrompt') | ||
| .get('threatLevelPremortemTemplateElevatedPrompt') | ||
| .update({groupColor: '#FFCC63'}) | ||
| .run() | ||
| await r | ||
| .table('ReflectPrompt') | ||
| .get('threatLevelPremortemTemplateLowPrompt') | ||
| .update({groupColor: '#66BC8C'}) | ||
| .run() |
There was a problem hiding this comment.
Consider batch updating for efficiency.
Updating each prompt separately results in multiple database connections and transactions, which is less efficient. Consider batching these updates into a single transaction to improve performance.
- await r.table('ReflectPrompt').get('threatLevelPremortemTemplateSeverePrompt').update({groupColor: '#FD6157'}).run()
- await r.table('ReflectPrompt').get('threatLevelPremortemTemplateElevatedPrompt').update({groupColor: '#FFCC63'}).run()
- await r.table('ReflectPrompt').get('threatLevelPremortemTemplateLowPrompt').update({groupColor: '#66BC8C'}).run()
+ await r.table('ReflectPrompt').getAll('threatLevelPremortemTemplateSeverePrompt', 'threatLevelPremortemTemplateElevatedPrompt', 'threatLevelPremortemTemplateLowPrompt').update(function(doc) {
+ return r.branch(
+ doc('id').eq('threatLevelPremortemTemplateSeverePrompt'), {groupColor: '#FD6157'},
+ doc('id').eq('threatLevelPremortemTemplateElevatedPrompt'), {groupColor: '#FFCC63'},
+ doc('id').eq('threatLevelPremortemTemplateLowPrompt'), {groupColor: '#66BC8C'}
+ )
+ }).run()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.
| export async function up() { | |
| await connectRethinkDB() | |
| await r | |
| .table('ReflectPrompt') | |
| .get('threatLevelPremortemTemplateSeverePrompt') | |
| .update({groupColor: '#FD6157'}) | |
| .run() | |
| await r | |
| .table('ReflectPrompt') | |
| .get('threatLevelPremortemTemplateElevatedPrompt') | |
| .update({groupColor: '#FFCC63'}) | |
| .run() | |
| await r | |
| .table('ReflectPrompt') | |
| .get('threatLevelPremortemTemplateLowPrompt') | |
| .update({groupColor: '#66BC8C'}) | |
| .run() | |
| export async function up() { | |
| await connectRethinkDB() | |
| await r.table('ReflectPrompt').getAll('threatLevelPremortemTemplateSeverePrompt', 'threatLevelPremortemTemplateElevatedPrompt', 'threatLevelPremortemTemplateLowPrompt').update(function(doc) { | |
| return r.branch( | |
| doc('id').eq('threatLevelPremortemTemplateSeverePrompt'), {groupColor: '#FD6157'}, | |
| doc('id').eq('threatLevelPremortemTemplateElevatedPrompt'), {groupColor: '#FFCC63'}, | |
| doc('id').eq('threatLevelPremortemTemplateLowPrompt'), {groupColor: '#66BC8C'} | |
| ) | |
| }).run() |
Improve error handling by rethrowing the error.
As noted in previous comments, the function logs the error but does not rethrow it, which might make it harder to debug issues in a larger application.
} catch (e) {
console.error('Error during migration:', e)
+ throw e
}Committable suggestion was skipped due to low confidence.
|
Question @mattkrick: what about the migrations and their order? This fix adds a migration, but there have been new migrations added to master since then 🤔 what do you usually do when that happens? rebase, merge master into the branch? recreate the migration after rebasing/merging? just merge the branch into master like it is? |
|
@rafaelromcar-parabol you need to rename the migration so it is newer than anything on master before you merge. I usually just create a new migration and copy and paste, but there might be a more elegant solution. |
|
i usually down-migrate, rename, then up-migrate. no perfect way to do it, distributed state management is hard! |
|
Thanks for the tips! ❤️ and thanks for merging the branch! |
Description
Fixes #9852
https://www.loom.com/share/7f5ddf56c4594e1692e266d556d95298
Summary by CodeRabbit