Skip to content

Conversation

scott-ray-wilson
Copy link
Contributor

Description 📣

This PR contains various secret update requests fixes:

  • support granular field updates without overwriting values (differentiate between value/comment clearing vs non-update)
  • UI fixes/handling to properly display state
  • migration to remove default skipMultilineEncoding to approval requests (always setting false if not updated when previously true)
  • removes passing secret value in overview mutation to update secret name

Type ✨

  • Bug fix
  • New feature
  • Improvement
  • Breaking change
  • Documentation

Tests 🛠️

# Here's some code block to paste some code snippets

…e changes, update change request UI to properly display state, and remove secret value setting on overview rename mutation
@maidul98
Copy link
Collaborator

maidul98 commented Sep 22, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR implements comprehensive fixes for secret change requests, specifically addressing how UPDATE operations handle granular field updates. The changes enable the system to differentiate between three distinct states for secret fields: (1) undefined/null means "don't update this field", (2) empty string means "clear this field", and (3) actual value means "update to this value".

The core architectural change centers around modifying the setKnexStringValue utility function to accept an allowEmptyString parameter, which prevents automatic conversion of empty strings to null when explicitly preserving them is required. This enables proper semantic distinction at the database level between field clearing and non-updates.

In the backend, the secret approval request service now implements conditional decryption logic with explicit null/undefined checks, ensuring that only fields intended for update are processed. The router schemas have been updated to support optional secretComment and skipMultilineEncoding fields by explicitly omitting them from inherited schemas and re-adding them with proper nullish handling.

A database migration removes the default value from the skipMultilineEncoding column in the approval requests table, allowing the application to distinguish between explicit false values and unset states. The data access layer has been updated to properly fetch and map the skipMultilineEncoding field in approval request queries.

On the frontend, the secret rename operation has been optimized to only pass the fields being updated (removing unnecessary secretValue transmission), and the approval request UI now includes proper fallback logic using nullish coalescing operators to gracefully handle partial updates and display appropriate values when some fields aren't being modified.

Confidence score: 4/5

  • This PR addresses legitimate issues in secret update handling but involves complex state management changes that could have subtle edge cases
  • Score reflects well-structured changes across multiple layers but complexity in differentiating between null/undefined/empty string states increases risk
  • Pay close attention to the database migration and the setKnexStringValue function modifications as they affect core data persistence logic

7 files reviewed, 2 comments

Edit Code Review Bot Settings | Greptile

@scott-ray-wilson scott-ray-wilson merged commit 362abc4 into main Sep 23, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants