feat(stack): usability improvements#301
Conversation
🦋 Changeset detectedLatest commit: c447e26 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes null plaintext support across the stack package: types, public API signatures, operation implementations, helpers, docs/examples, and tests. Batch/bulk paths now process all items uniformly (no null-short-circuits) and encryption/decryption types and results are non-nullable. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stack/src/encryption/operations/bulk-encrypt.ts`:
- Around line 94-97: Fix the positional mapping in BulkEncryptOperation.execute
and BulkEncryptOperationWithLockContext.execute: before calling encryptBulk,
create a filtered array of non-null plaintexts (preserving order), call
encryptBulk on that, then rebuild the returned results by iterating over the
original this.plaintexts and using a counter into the encryptedData array to
assign data for non-null entries and { id, data: null } for null plaintexts so
ids remain correctly paired with their encrypted payloads.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.changeset/fresh-phones-smile.mdpackages/stack/__tests__/bulk-protect.test.tspackages/stack/__tests__/encrypt-query-searchable-json.test.tspackages/stack/__tests__/encrypt-query-stevec.test.tspackages/stack/__tests__/encrypt-query.test.tspackages/stack/__tests__/encryption-helpers.test.tspackages/stack/__tests__/json-protect.test.tspackages/stack/__tests__/number-protect.test.tspackages/stack/__tests__/protect-ops.test.tspackages/stack/__tests__/searchable-json-pg.test.tspackages/stack/src/encryption/helpers/index.tspackages/stack/src/encryption/index.tspackages/stack/src/encryption/operations/batch-encrypt-query.tspackages/stack/src/encryption/operations/bulk-decrypt.tspackages/stack/src/encryption/operations/bulk-encrypt.tspackages/stack/src/encryption/operations/decrypt.tspackages/stack/src/encryption/operations/encrypt-query.tspackages/stack/src/encryption/operations/encrypt.tspackages/stack/src/types-public.tspackages/stack/src/types.ts
💤 Files with no reviewable changes (9)
- packages/stack/tests/searchable-json-pg.test.ts
- packages/stack/src/types-public.ts
- packages/stack/tests/protect-ops.test.ts
- packages/stack/tests/encrypt-query-stevec.test.ts
- packages/stack/tests/encrypt-query.test.ts
- packages/stack/src/encryption/helpers/index.ts
- packages/stack/tests/number-protect.test.ts
- packages/stack/tests/encryption-helpers.test.ts
- packages/stack/tests/json-protect.test.ts
| return encryptedData.map((data, i) => ({ | ||
| id: this.plaintexts[i].id, | ||
| data, | ||
| })) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.ts" -path "*/protect-ffi/*" | head -20Repository: cipherstash/protectjs
Length of output: 47
🏁 Script executed:
git ls-files | grep -E "(protect-ffi|bulk-encrypt)" | head -20Repository: cipherstash/protectjs
Length of output: 422
🏁 Script executed:
cat -n packages/stack/src/encryption/operations/bulk-encrypt.ts | head -100Repository: cipherstash/protectjs
Length of output: 3581
🏁 Script executed:
rg -n "encryptBulk" --type=ts -A 5 -B 2 | head -100Repository: cipherstash/protectjs
Length of output: 8131
🏁 Script executed:
cat -n packages/protect/src/ffi/operations/bulk-encrypt.ts | head -150Repository: cipherstash/protectjs
Length of output: 5184
🏁 Script executed:
rg -n "originalIndex" --type=tsRepository: cipherstash/protectjs
Length of output: 2014
🏁 Script executed:
fd -t f -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "encryptBulk" | head -10Repository: cipherstash/protectjs
Length of output: 295
🏁 Script executed:
rg -n "encryptBulk" packages/protect/src/ffi/operations/bulk-encrypt.ts -A 10 -B 2 | head -150Repository: cipherstash/protectjs
Length of output: 1514
🏁 Script executed:
cat packages/stack/src/encryption/operations/bulk-encrypt.ts | sed -n '23,97p'Repository: cipherstash/protectjs
Length of output: 2068
🏁 Script executed:
rg -n "plaintext.*null" packages/protect/src/ffi/operations/ --type=ts -B 2 -A 2 | head -80Repository: cipherstash/protectjs
Length of output: 5099
🏁 Script executed:
cat packages/protect/src/ffi/operations/bulk-encrypt.ts | sed -n '49,69p'Repository: cipherstash/protectjs
Length of output: 595
🏁 Script executed:
cat packages/stack/src/encryption/operations/bulk-encrypt.ts | sed -n '160,185p'Repository: cipherstash/protectjs
Length of output: 746
🏁 Script executed:
rg -n "class BulkEncryptOperationWithLockContext" packages/stack/src/encryption/operations/bulk-encrypt.ts -A 40 | head -50Repository: cipherstash/protectjs
Length of output: 1466
Apply null-safe result mapping to both BulkEncryptOperation.execute() and BulkEncryptOperationWithLockContext.execute().
The current positional mapping assumes encryptBulk returns exactly one result per input in the same order:
encryptedData.map((data, i) => ({
id: this.plaintexts[i].id,
data,
}))However, the FFI layer filters null plaintexts before encryption (see packages/protect/src/ffi/operations/bulk-encrypt.ts lines 30–40 and 49–69), so the encrypted results array will have fewer items than the input array if any plaintexts are null. This causes a silent mismatch: this.plaintexts[i].id will resolve to the wrong plaintext when i exceeds the count of non-null items.
Implement the same pattern used in the FFI layer: filter nulls before encryption, then reconstruct the full result array using a counter to track the position in the filtered encrypted data. This ensures null plaintexts map to { id, data: null } without corrupting the id-to-data mapping.
Affects both variants: lines 94–97 and 182–185.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stack/src/encryption/operations/bulk-encrypt.ts` around lines 94 -
97, Fix the positional mapping in BulkEncryptOperation.execute and
BulkEncryptOperationWithLockContext.execute: before calling encryptBulk, create
a filtered array of non-null plaintexts (preserving order), call encryptBulk on
that, then rebuild the returned results by iterating over the original
this.plaintexts and using a counter into the encryptedData array to assign data
for non-null entries and { id, data: null } for null plaintexts so ids remain
correctly paired with their encrypted payloads.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/stack/src/schema/index.ts (1)
148-157:⚠️ Potential issue | 🟡 MinorJSDoc for
EncryptedField.dataType()still documents the default as'string'; effective default is now'text'.Lines 153–156 say "By default all values are treated as
'string'". The same staleness exists inEncryptedColumn.dataType()at lines 200–204.📝 Proposed doc fix
- * By default all values are treated as `'string'`. Use this method to specify - * a different type so the encryption layer knows how to encode the plaintext - * before encrypting. + * By default all values are treated as `'text'`. Use this method to specify + * a different type so the encryption layer knows how to encode the plaintext + * before encrypting.Apply the same fix to the identical paragraph in
EncryptedColumn.dataType()(around line 200–204).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/src/schema/index.ts` around lines 148 - 157, Update the JSDoc for EncryptedField.dataType() and EncryptedColumn.dataType(): change the sentence that currently states "By default all values are treated as 'string'." to reflect the effective default "text" (e.g., "By default all values are treated as 'text'.") so the docs match the implementation and keep the rest of the description intact.packages/stack/README.md (1)
627-631:⚠️ Potential issue | 🟡 MinorUpdate stale
csValuereference toencryptedFieldin README.Lines 627–631 incorrectly reference
csValue(valueName)(returningProtectValue), which is not exported from the current API. The correct export isencryptedField(valueName)returningEncryptedField. The same stale name appears in the Subpath Exports table at line 639.Proposed fixes
Lines 627–631:
-csValue(valueName) // returns ProtectValue (for nested values) +encryptedField(valueName) // returns EncryptedField (for nested encrypted fields)Subpath Exports table:
-| `@cipherstash/stack/schema` | `encryptedTable`, `encryptedColumn`, `csValue`, schema types | +| `@cipherstash/stack/schema` | `encryptedTable`, `encryptedColumn`, `encryptedField`, schema types |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/README.md` around lines 627 - 631, The README contains stale API names: replace any occurrences of csValue(valueName) and its return type ProtectValue with the current API encryptedField(valueName) returning EncryptedField (e.g., in the example block alongside encryptedTable and encryptedColumn and in the Subpath Exports table); update the example line to use encryptedField(valueName) and adjust the return type text to EncryptedField so the docs match the exported symbols.
🧹 Nitpick comments (1)
packages/stack/src/schema/index.ts (1)
192-195:EncryptedColumnconstructor initializescastAsValueto'string'whileEncryptedFieldwas updated to'text'.The inconsistency is masked by the normalization in
build(), but leaves internal state out of sync between the two classes. Worth aligning.♻️ Proposed fix
constructor(columnName: string) { this.columnName = columnName - this.castAsValue = 'string' + this.castAsValue = 'text' }With this change the guard in
build()at line 341 (=== 'string' ? 'text' : ...) becomes a no-op safety net, which is fine to leave in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/src/schema/index.ts` around lines 192 - 195, The EncryptedColumn constructor sets this.castAsValue = 'string' which drifts from EncryptedField's updated default of 'text'; update the EncryptedColumn constructor (the constructor in class EncryptedColumn) to initialize castAsValue to 'text' so internal state matches EncryptedField, leaving the existing guard in build() (the === 'string' ? 'text' : ...) as a redundant safety net.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stack/__tests__/schema-builders.test.ts`:
- Around line 23-31: Update the test descriptions to match the actual asserted
value: rename the test titles that currently say "string" to say "text" so they
reflect the expectation of built.cast_as === 'text'; specifically modify the
tests around encryptedColumn('name').build()/dataType('string') (using
encryptedColumn, .dataType, and .build which assert cast_as) and the other
occurrence at the later test (around line ~317) to change "defaults castAs to
string" and ".dataType(\"string\") sets castAs to string" to use "text" instead
of "string".
---
Outside diff comments:
In `@packages/stack/README.md`:
- Around line 627-631: The README contains stale API names: replace any
occurrences of csValue(valueName) and its return type ProtectValue with the
current API encryptedField(valueName) returning EncryptedField (e.g., in the
example block alongside encryptedTable and encryptedColumn and in the Subpath
Exports table); update the example line to use encryptedField(valueName) and
adjust the return type text to EncryptedField so the docs match the exported
symbols.
In `@packages/stack/src/schema/index.ts`:
- Around line 148-157: Update the JSDoc for EncryptedField.dataType() and
EncryptedColumn.dataType(): change the sentence that currently states "By
default all values are treated as 'string'." to reflect the effective default
"text" (e.g., "By default all values are treated as 'text'.") so the docs match
the implementation and keep the rest of the description intact.
---
Nitpick comments:
In `@packages/stack/src/schema/index.ts`:
- Around line 192-195: The EncryptedColumn constructor sets this.castAsValue =
'string' which drifts from EncryptedField's updated default of 'text'; update
the EncryptedColumn constructor (the constructor in class EncryptedColumn) to
initialize castAsValue to 'text' so internal state matches EncryptedField,
leaving the existing guard in build() (the === 'string' ? 'text' : ...) as a
redundant safety net.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
docs/reference/model-operations.mddocs/reference/searchable-encryption-postgres.mdpackages/stack/README.mdpackages/stack/__tests__/schema-builders.test.tspackages/stack/package.jsonpackages/stack/src/encryption/index.tspackages/stack/src/schema/index.tsskills/stash-encryption/SKILL.md
💤 Files with no reviewable changes (1)
- docs/reference/model-operations.md
✅ Files skipped from review due to trivial changes (1)
- packages/stack/package.json
calvinbrewer
changed the title
2 participants
Summary by CodeRabbit
Type Safety
New Features
Documentation
Tests