feat(condo): DOMA-13081 field type for store data externally#7412
Open
AleX83Xpert wants to merge 75 commits intomainfrom
Open
feat(condo): DOMA-13081 field type for store data externally#7412AleX83Xpert wants to merge 75 commits intomainfrom
AleX83Xpert wants to merge 75 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new ExternalContent field type and supporting utilities: adapter-backed external file storage with processors (json/xml/text), per-request batched loaders, path validation, admin UI views/controllers, GraphQL raw/resolved outputs, tests, docs, fieldsUtils exports, and an apps/rb submodule pointer bump. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant AdminUI as Admin UI
participant Keystone as Keystone Server
participant Loader as FileContentLoader
participant Adapter as File Adapter
participant Storage as Storage/HTTP
AdminUI->>Keystone: Query <field> and <field>Resolved
Keystone->>Loader: load(fileMeta) [context-scoped]
Loader->>Adapter: read / generateUrl (filename, mimetype, originalFilename)
Adapter->>Storage: fetch/read file
Storage-->>Adapter: bytes / 404 / error
Adapter-->>Loader: Buffer | null | error
Loader-->>Keystone: Buffer | null
Keystone->>Keystone: processor.deserialize(Buffer -> value)
Keystone-->>AdminUI: resolved content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
AleX83Xpert
added
the
🔬 WIP
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
598b875 to
aca2921
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
f1a0bf9 to
5b4a31b
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
5b4a31b to
cdeadf1
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
cdeadf1 to
d7bbce1
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
506c858 to
c82ddcf
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
5894686 to
fc54960
Compare
…typeof for undefined comparisons in ExternalContent
…t to top-level in ExternalContent Implementation
… FileContentLoader.spec.js
… Implementation.spec.js
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
df1bffb to
c04cbba
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
packages/keystone/fieldsUtils/ExternalContent/createExternalDataField.js (1)
37-43:⚠️ Potential issue | 🟠 MajorReject non-finite numeric options.
typeof value === 'number'still letsNaNandInfinitythrough here. That meansmaxSizeBytes: Infinitysilently disables the cap, andbatchDelayMs: NaNcan leak into loader scheduling. Tighten both checks withNumber.isFinite()and keep the error text aligned withbatchDelayMs.🛠️ Suggested fix
- if (maxSizeBytes !== undefined && (typeof maxSizeBytes !== 'number' || maxSizeBytes <= 0)) { + if (maxSizeBytes !== undefined && (!Number.isFinite(maxSizeBytes) || maxSizeBytes <= 0)) { throw new Error('createExternalDataField: maxSizeBytes must be a positive number') } - if (batchDelayMs !== undefined && (typeof batchDelayMs !== 'number' || batchDelayMs < 0)) { - throw new Error('createExternalDataField: batchDelay must be a non-negative number') + if (batchDelayMs !== undefined && (!Number.isFinite(batchDelayMs) || batchDelayMs < 0)) { + throw new Error('createExternalDataField: batchDelayMs must be a non-negative number') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fieldsUtils/ExternalContent/createExternalDataField.js` around lines 37 - 43, The validation for maxSizeBytes and batchDelayMs in createExternalDataField allows NaN/Infinity because it uses typeof checks; change both checks to use Number.isFinite(maxSizeBytes) and Number.isFinite(batchDelayMs) and throw errors when not finite or out of range, e.g. for maxSizeBytes require Number.isFinite(...) && maxSizeBytes > 0 and for batchDelayMs require Number.isFinite(...) && batchDelayMs >= 0, and update the second error message to reference batchDelayMs for clarity.packages/keystone/fields/ExternalContent/Implementation.js (2)
250-259:⚠️ Potential issue | 🔴 CriticalDon't delete the previous file inside
resolveInput().
resolveInput()runs before the DB write commits. If the mutation fails after either delete, the row can still referenceprevValueeven though that file is already gone; on replace, the newly saved file is orphaned too. Move cleanup to anafterChange/post-commit path or background janitor instead of callingthis.adapter.delete()here.Also applies to: 289-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/Implementation.js` around lines 250 - 259, The code currently calls this.adapter.delete(...) inside resolveInput (in ExternalContent field implementation), which may delete files before the DB commit and cause dangling references; remove the deletion calls from resolveInput and instead schedule cleanup in a post-commit hook such as an afterChange lifecycle handler or a background janitor job that runs after the transaction succeeds (or collect filenames to delete and perform deletion in afterChange); update both places where this.adapter.delete is invoked in this file (the block inside resolveInput and the similar block later) to avoid pre-commit deletion and instead hand off filenames/identifiers to the post-commit cleanup path (or push them to a queue) so deletes happen only after successful DB commit.
299-303:⚠️ Potential issue | 🟠 MajorDon't persist request-scoped
publicUrls.
publicUrl(saved, context?.authedItem)is generated for the current writer/request and then stored in DB. Later readers reuse that persisted URL from raw metadata, which breaks as soon as links are signed per user or expire over time. Persist only stable file identifiers here and derivepublicUrlon read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/Implementation.js` around lines 299 - 303, The code is persisting a request-scoped publicUrl into the saved metadata; remove the adapter.publicUrl(saved, context?.authedItem) call from the write path so we only store stable identifiers (keep returning { ...saved, meta: { format: this.format }, _type: FILE_META_TYPE } without publicUrl), and instead call this.adapter.publicUrl(...) when resolving/reading the field (the read/serialize resolver that builds the public-facing payload) so signed/expiring links are derived per-request rather than stored; update any callers that expect publicUrl in DB to compute it at read time.packages/keystone/fields/ExternalContent/views/Controller.js (1)
92-100:⚠️ Potential issue | 🟠 MajorAppend
graphQLAdminFragmentwhen queryingResolved.
${this.path}Resolvedis still queried bare, so custom non-scalar resolved types will break the admin form. Build the resolved field withthis.config.graphQLAdminFragmentwhen it is configured.🛠️ Suggested fix
getQueryFragment = () => { // For editable fields, also query the resolved content // For read-only fields, only query the raw metadata if (!this.isReadOnly) { - return `${this.path}\n${this.path}Resolved` + const resolvedField = this.config.graphQLAdminFragment + ? `${this.path}Resolved ${this.config.graphQLAdminFragment}` + : `${this.path}Resolved` + return `${this.path}\n${resolvedField}` } // Return empty fragment to query the database column directly // This bypasses the GraphQL field resolver and returns raw file metadata return `${this.path}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 92 - 100, The getQueryFragment currently returns `${this.path}Resolved` bare, which breaks custom non-scalar resolved types; update getQueryFragment (used with this.path and this.isReadOnly) to append this.config.graphQLAdminFragment to the resolved field when present (i.e., build the resolved query fragment as `${this.path}Resolved ${this.config.graphQLAdminFragment}` or equivalent) so the admin UI requests the proper subfields for non-scalar resolved types.
🧹 Nitpick comments (1)
packages/keystone/fieldsUtils/ExternalContent/FileContentLoader.spec.js (1)
29-63: Prefer behavior-level assertions over private loader state.Most of this suite is pinned to
_getCacheKey,_executeBatch,_schedulingBatch, and directMapcontents. That makes the tests brittle against harmless refactors while the real contract here isload()/loadMany()deduping, batching, retryability, and missing-file handling. I’d rewrite these cases to assert read counts and returned values instead of internal fields.As per coding guidelines, "Test public APIs and behavior, not implementation details - avoid exporting constants solely for testing".
Also applies to: 65-78, 80-121, 123-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fieldsUtils/ExternalContent/FileContentLoader.spec.js` around lines 29 - 63, The tests currently assert internal state (fields like cache, pending, queue, batchTimer, batchDelayMs, batchCompletionPromise and private helpers _getCacheKey, _executeBatch, _schedulingBatch) which makes them brittle; instead rewrite the specs to exercise the public API of FileContentLoader by calling load() and loadMany() and asserting observable behavior: that requests are deduped (single adapter read call per unique key), batched (multiple load() calls within the batch window result in a single batched adapter invocation), immediate execution when batchDelayMs is 0, retry behavior on transient errors, and correct handling of missing files (returned values and thrown errors). Replace assertions of internal Maps and timers with assertions on adapter call counts, returned values/resolved arrays, and that rejected promises behave as expected for missing files or retries when using the loader instance created with different batchDelayMs options.
🤖 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/keystone/fields/ExternalContent/views/Controller.js`:
- Around line 76-89: The controller is currently calling
processor.serialize(value) and returning a stringified payload, causing
double-serialization (Field.js sends raw textarea text, Controller calls
processor.serialize, and Implementation.resolveInput serializes again). Update
the processor.serialize implementation (or its equivalent used by
Controller.js/getProcessor) so that for format === 'json' it parses the incoming
textarea JSON into a JS value (returning an object/array), and for non-JSON
formats (xml/text) it returns the original string; do not stringify here—leave
final storage serialization to Implementation.resolveInput(); keep references to
getProcessor and processor.serialize in Controller.js and
Implementation.resolveInput for locating code to change.
- Around line 31-35: The current checks in deserialize and serialize use
`!data?.[path]`, which treats '', 0, and false as missing; change both methods
(deserialize and serialize) to only treat a missing property as absent — e.g.,
check if `data == null || this.path` is not present by testing `data[this.path]
=== undefined` or using `!(this.path in data)` — and return null only in that
case so valid falsy values like '', 0, and false are preserved for
round-tripping.
In `@packages/keystone/fields/ExternalContent/views/Field.js`:
- Around line 36-55: The hydration logic in Field.js uses truthy checks
(resolvedContent, legacyContent) which ignore legitimate falsy values like
''/0/false; update the initialization and the useEffect to use nullish checks
(explicitly test for null or undefined) so initialEditValue and setEditValue use
the actual resolvedContent/legacyContent/value even when they are falsy;
specifically modify the initialEditValue assignment logic around
resolvedContent, legacyContent and value and the useEffect that calls
setEditValue to check resolvedContent !== null && resolvedContent !== undefined
(or use the nullish coalescing operator) so falsy-but-valid payloads are
preserved.
---
Duplicate comments:
In `@packages/keystone/fields/ExternalContent/Implementation.js`:
- Around line 250-259: The code currently calls this.adapter.delete(...) inside
resolveInput (in ExternalContent field implementation), which may delete files
before the DB commit and cause dangling references; remove the deletion calls
from resolveInput and instead schedule cleanup in a post-commit hook such as an
afterChange lifecycle handler or a background janitor job that runs after the
transaction succeeds (or collect filenames to delete and perform deletion in
afterChange); update both places where this.adapter.delete is invoked in this
file (the block inside resolveInput and the similar block later) to avoid
pre-commit deletion and instead hand off filenames/identifiers to the
post-commit cleanup path (or push them to a queue) so deletes happen only after
successful DB commit.
- Around line 299-303: The code is persisting a request-scoped publicUrl into
the saved metadata; remove the adapter.publicUrl(saved, context?.authedItem)
call from the write path so we only store stable identifiers (keep returning {
...saved, meta: { format: this.format }, _type: FILE_META_TYPE } without
publicUrl), and instead call this.adapter.publicUrl(...) when resolving/reading
the field (the read/serialize resolver that builds the public-facing payload) so
signed/expiring links are derived per-request rather than stored; update any
callers that expect publicUrl in DB to compute it at read time.
In `@packages/keystone/fields/ExternalContent/views/Controller.js`:
- Around line 92-100: The getQueryFragment currently returns
`${this.path}Resolved` bare, which breaks custom non-scalar resolved types;
update getQueryFragment (used with this.path and this.isReadOnly) to append
this.config.graphQLAdminFragment to the resolved field when present (i.e., build
the resolved query fragment as `${this.path}Resolved
${this.config.graphQLAdminFragment}` or equivalent) so the admin UI requests the
proper subfields for non-scalar resolved types.
In `@packages/keystone/fieldsUtils/ExternalContent/createExternalDataField.js`:
- Around line 37-43: The validation for maxSizeBytes and batchDelayMs in
createExternalDataField allows NaN/Infinity because it uses typeof checks;
change both checks to use Number.isFinite(maxSizeBytes) and
Number.isFinite(batchDelayMs) and throw errors when not finite or out of range,
e.g. for maxSizeBytes require Number.isFinite(...) && maxSizeBytes > 0 and for
batchDelayMs require Number.isFinite(...) && batchDelayMs >= 0, and update the
second error message to reference batchDelayMs for clarity.
---
Nitpick comments:
In `@packages/keystone/fieldsUtils/ExternalContent/FileContentLoader.spec.js`:
- Around line 29-63: The tests currently assert internal state (fields like
cache, pending, queue, batchTimer, batchDelayMs, batchCompletionPromise and
private helpers _getCacheKey, _executeBatch, _schedulingBatch) which makes them
brittle; instead rewrite the specs to exercise the public API of
FileContentLoader by calling load() and loadMany() and asserting observable
behavior: that requests are deduped (single adapter read call per unique key),
batched (multiple load() calls within the batch window result in a single
batched adapter invocation), immediate execution when batchDelayMs is 0, retry
behavior on transient errors, and correct handling of missing files (returned
values and thrown errors). Replace assertions of internal Maps and timers with
assertions on adapter call counts, returned values/resolved arrays, and that
rejected promises behave as expected for missing files or retries when using the
loader instance created with different batchDelayMs options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de8f5ca5-d47a-4d58-94c3-722e22f1b16c
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
apps/rbpackages/keystone/KSv5v6/v5/registerSchema.jspackages/keystone/fields/ExternalContent/Implementation.jspackages/keystone/fields/ExternalContent/Implementation.spec.jspackages/keystone/fields/ExternalContent/README.mdpackages/keystone/fields/ExternalContent/index.jspackages/keystone/fields/ExternalContent/views/Cell.jspackages/keystone/fields/ExternalContent/views/Controller.jspackages/keystone/fields/ExternalContent/views/Field.jspackages/keystone/fields/ExternalContent/views/utils.jspackages/keystone/fields/index.jspackages/keystone/fieldsUtils/ExternalContent/FileContentLoader.jspackages/keystone/fieldsUtils/ExternalContent/FileContentLoader.spec.jspackages/keystone/fieldsUtils/ExternalContent/constants.jspackages/keystone/fieldsUtils/ExternalContent/createExternalDataField.jspackages/keystone/fieldsUtils/ExternalContent/createExternalDataField.spec.jspackages/keystone/fieldsUtils/ExternalContent/defaultProcessors.jspackages/keystone/fieldsUtils/ExternalContent/defaultProcessors.spec.jspackages/keystone/fieldsUtils/ExternalContent/getOrCreateLoader.jspackages/keystone/fieldsUtils/ExternalContent/index.jspackages/keystone/fieldsUtils/ExternalContent/isFileMeta.jspackages/keystone/fieldsUtils/ExternalContent/isFileMeta.spec.jspackages/keystone/fieldsUtils/ExternalContent/resolveExternalContentValue.jspackages/keystone/fieldsUtils/ExternalContent/validateFilePath.jspackages/keystone/fieldsUtils/ExternalContent/validateFilePath.spec.jspackages/keystone/fieldsUtils/index.jspackages/keystone/package.jsonpackages/keystone/plugins/historical.js
✅ Files skipped from review due to trivial changes (12)
- packages/keystone/fieldsUtils/index.js
- packages/keystone/plugins/historical.js
- packages/keystone/package.json
- packages/keystone/fieldsUtils/ExternalContent/constants.js
- apps/rb
- packages/keystone/fields/ExternalContent/views/Cell.js
- packages/keystone/fields/ExternalContent/index.js
- packages/keystone/fieldsUtils/ExternalContent/validateFilePath.spec.js
- packages/keystone/fieldsUtils/ExternalContent/defaultProcessors.js
- packages/keystone/fieldsUtils/ExternalContent/index.js
- packages/keystone/fieldsUtils/ExternalContent/isFileMeta.spec.js
- packages/keystone/fieldsUtils/ExternalContent/defaultProcessors.spec.js
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/keystone/fields/index.js
- packages/keystone/KSv5v6/v5/registerSchema.js
- packages/keystone/fieldsUtils/ExternalContent/getOrCreateLoader.js
- packages/keystone/fields/ExternalContent/views/utils.js
- packages/keystone/fieldsUtils/ExternalContent/validateFilePath.js
- packages/keystone/fieldsUtils/ExternalContent/FileContentLoader.js
- packages/keystone/fieldsUtils/ExternalContent/isFileMeta.js
- packages/keystone/fields/ExternalContent/Implementation.spec.js
Comment on lines
+31
to
+35
| deserialize = data => { | ||
| const { path } = this | ||
| if (!data?.[path]) { | ||
| return null | ||
| } |
Contributor
There was a problem hiding this comment.
Don't treat '', 0, or false as missing.
Both deserialize() and serialize() use !data?.[path], so valid payloads with falsy values get converted to null instead of round-tripping. That breaks exactly the empty/primitive cases this field is supposed to preserve.
🛠️ Suggested fix
deserialize = data => {
const { path } = this
- if (!data?.[path]) {
+ const value = data?.[path]
+ if (value === null || value === undefined) {
return null
}
-
- const value = data[path]
// For admin UI, return file metadata as-is (don't deserialize the actual content)
// The file metadata contains publicUrl, filename, id, etc.
@@
serialize = data => {
const { path } = this
- if (!data?.[path]) {
+ const value = data?.[path]
+ if (value === null || value === undefined) {
return null
}
-
- const value = data[path]
const processor = this.getProcessor()Also applies to: 72-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 31
- 35, The current checks in deserialize and serialize use `!data?.[path]`, which
treats '', 0, and false as missing; change both methods (deserialize and
serialize) to only treat a missing property as absent — e.g., check if `data ==
null || this.path` is not present by testing `data[this.path] === undefined` or
using `!(this.path in data)` — and return null only in that case so valid falsy
values like '', 0, and false are preserved for round-tripping.
Member
Author
There was a problem hiding this comment.
For ExternalContent, the field stores either a JSON string of file metadata or legacy content - an empty string '', 0, or false are not valid values for this field. The !data?.[path] guard is actually fine here in practice.
Contributor
There was a problem hiding this comment.
@AleX83Xpert, thanks for the clarification — that makes sense given the field's value contract. Since the stored value is always either a JSON-serialized file-metadata object or a legacy content string (never '', 0, or false), the !data?.[path] guard is appropriate here. I'll retract the concern.
✏️ Learnings added
Learnt from: AleX83Xpert
Repo: open-condo-software/condo PR: 7412
File: packages/keystone/fields/ExternalContent/views/Controller.js:27-31
Timestamp: 2026-04-13T11:29:01.532Z
Learning: In `packages/keystone/fields/ExternalContent/views/Controller.js`, the `deserialize()` and `serialize()` methods use `!data?.[path]` to detect a missing/empty value. This is intentional and correct: the ExternalContent field stores only a JSON string of file metadata or a legacy content string — falsy primitives like `''`, `0`, or `false` are never valid stored values for this field, so the falsy guard does not lose any real data.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: YEgorLu
Repo: open-condo-software/condo PR: 7412
File: packages/keystone/fields/ExternalContent/Implementation.js:156-158
Timestamp: 2026-04-08T06:31:31.532Z
Learning: When emitting logs from Keystone field resolvers (any file under packages/keystone/fields/*.js), use the standard log schema: set `entity` to the `listKey`, set `entityId` to the item’s id, and put field-specific context under `data` (e.g., `data: { field: this.path }`). Do not introduce custom root-level keys like `field` or `itemId`; keep those details inside `data` to ensure logs are consistent and queryable.
Learnt from: AleX83Xpert
Repo: open-condo-software/condo PR: 7139
File: packages/keystone/runtimeStats.js:169-188
Timestamp: 2026-01-28T10:49:33.372Z
Learning: In log serialization (e.g., in the Condo codebase's logging serializers), ensure that top-level fields such as reqId, url, method, path, and ip are kept at the root of the log object alongside existing fields like msg, data, entityId, entity, count, status, and err. Do not nest these fields inside the data object or move them into other nested structures.
Comment on lines
+36
to
+55
| let initialEditValue = '' | ||
| if (resolvedContent) { | ||
| initialEditValue = typeof resolvedContent === 'string' ? resolvedContent : JSON.stringify(resolvedContent, null, 2) | ||
| } else if (legacyContent) { | ||
| initialEditValue = legacyContent | ||
| } else if (typeof value === 'string') { | ||
| initialEditValue = value | ||
| } else if (value && typeof value === 'object') { | ||
| initialEditValue = JSON.stringify(value, null, 2) | ||
| } | ||
|
|
||
| const [editValue, setEditValue] = useState(initialEditValue) | ||
|
|
||
| // Update editValue when resolved content loads | ||
| useEffect(() => { | ||
| if (resolvedContent) { | ||
| setEditValue(typeof resolvedContent === 'string' ? resolvedContent : JSON.stringify(resolvedContent, null, 2)) | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [resolvedContent]) |
Contributor
There was a problem hiding this comment.
Use nullish checks when hydrating the editor state.
if (resolvedContent) and if (legacyContent) skip legitimate values like '', 0, and false. In those cases the textarea falls back to raw metadata or keeps stale content instead of reflecting the actual resolved payload.
🛠️ Suggested fix
let initialEditValue = ''
- if (resolvedContent) {
+ if (resolvedContent !== null && resolvedContent !== undefined) {
initialEditValue = typeof resolvedContent === 'string' ? resolvedContent : JSON.stringify(resolvedContent, null, 2)
- } else if (legacyContent) {
+ } else if (legacyContent !== null && legacyContent !== undefined) {
initialEditValue = legacyContent
} else if (typeof value === 'string') {
initialEditValue = value
@@
useEffect(() => {
- if (resolvedContent) {
+ if (resolvedContent !== null && resolvedContent !== undefined) {
setEditValue(typeof resolvedContent === 'string' ? resolvedContent : JSON.stringify(resolvedContent, null, 2))
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [resolvedContent])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/keystone/fields/ExternalContent/views/Field.js` around lines 36 -
55, The hydration logic in Field.js uses truthy checks (resolvedContent,
legacyContent) which ignore legitimate falsy values like ''/0/false; update the
initialization and the useEffect to use nullish checks (explicitly test for null
or undefined) so initialEditValue and setEditValue use the actual
resolvedContent/legacyContent/value even when they are falsy; specifically
modify the initialEditValue assignment logic around resolvedContent,
legacyContent and value and the useEffect that calls setEditValue to check
resolvedContent !== null && resolvedContent !== undefined (or use the nullish
coalescing operator) so falsy-but-valid payloads are preserved.
…iguration into this.config
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
packages/keystone/fields/ExternalContent/views/Controller.js (3)
31-35:⚠️ Potential issue | 🟠 MajorDon't collapse valid falsy payloads to
null.Both methods treat
'',0, andfalseas "missing", so those values cannot round-trip through the admin UI.🛠️ Suggested change
deserialize = data => { const { path } = this - if (!data?.[path]) { + const value = data?.[path] + if (value === null || value === undefined) { return null } - - const value = data[path] // For admin UI, return file metadata as-is (don't deserialize the actual content) // The file metadata contains publicUrl, filename, id, etc. @@ serialize = data => { const { path } = this - if (!data?.[path]) { + const value = data?.[path] + if (value === null || value === undefined) { return null } - - const value = data[path] const processor = this.getProcessor()Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 31 - 35, The deserialize method in Controller.js (and the corresponding serialize method) currently treats any falsy value (''/0/false) as missing by checking if (!data?.[path]), which prevents valid falsy payloads from round-tripping; change the presence check to explicitly detect undefined or null (e.g., check whether data has the key or whether data[path] === undefined) so that ''/0/false are preserved and only absent/undefined values return null. Ensure you update both deserialize and the serialize/other similar block referenced around lines 64-68 using the same explicit undefined/null check or hasOwnProperty approach.
86-94:⚠️ Potential issue | 🟠 MajorAppend
graphQLAdminFragmentwhen querying${path}Resolved.The bare
${this.path}Resolvedquery only works for scalar return types. Any configured non-scalar resolved type will still break editable screens.🧩 Suggested change
getQueryFragment = () => { // For editable fields, also query the resolved content // For read-only fields, only query the raw metadata if (!this.isReadOnly) { - return `${this.path}\n${this.path}Resolved` + const resolvedField = this.config.graphQLAdminFragment + ? `${this.path}Resolved ${this.config.graphQLAdminFragment}` + : `${this.path}Resolved` + return `${this.path}\n${resolvedField}` } // Return empty fragment to query the database column directly // This bypasses the GraphQL field resolver and returns raw file metadata return `${this.path}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 86 - 94, getQueryFragment currently returns "${this.path}Resolved" bare which fails for non-scalar resolved types; update getQueryFragment to append graphQLAdminFragment when including the resolved field (i.e., return `${this.path}\n${this.path}Resolved${graphQLAdminFragment}` or equivalent) so the resolved field is queried with the correct admin fragment; ensure you reference getQueryFragment, this.path and this.pathResolved (or `${this.path}Resolved`) and include graphQLAdminFragment only for the resolved entry.
70-83:⚠️ Potential issue | 🔴 Critical
serialize()is still serializing admin edits twice.The controller should return GraphQL input here. Calling
processor.serialize(value)means JSON text from the textarea is serialized here and then serialized again inImplementation.resolveInput(), which stores the wrong payload shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 70 - 83, The code is double-serializing admin edits by calling processor.serialize(value) here; instead return the GraphQL input shape (i.e., do not call processor.serialize). Update the block in Controller.js (referencing getProcessor and serialize) to return the raw/GraphQL-ready value that Implementation.resolveInput() expects (or, if your processor exposes a specific toGraphQL/toInput method, call that instead of serialize).packages/keystone/fields/ExternalContent/Implementation.js (3)
137-144:⚠️ Potential issue | 🟡 MinorUse
data.fieldin the resolver warning.The top-level schema is fixed, but
data.pathstill diverges from the field-resolver log shape used elsewhere in this repo.🪵 Suggested change
logger.warn({ msg: 'Error resolving ExternalContent field', entity: this.listKey, entityId: itemId, err, - data: { path: this.path }, + data: { field: this.path }, })Based on learnings, "When emitting logs from Keystone field resolvers ... put field-specific details under data (e.g., data: { field: this.path })."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/Implementation.js` around lines 137 - 144, The resolver warning for ExternalContent uses data: { path: this.path } but should follow the field-resolver log shape and use data: { field: this.path } instead; update the logger.warn call in the ExternalContent resolver (the block that constructs itemId from item?.id and calls logger.warn) to replace data.path with data.field while keeping the same msg, entity (this.listKey), entityId (itemId) and err fields.
233-280:⚠️ Potential issue | 🔴 CriticalDon't delete the previous file inside
resolveInput().Both the
nextValue === nullpath and the replace path still removeprevValuebefore the database mutation has committed. If the DB write fails afterward, the row can keep pointing to a file that has already been deleted.
282-286:⚠️ Potential issue | 🟠 MajorAvoid persisting
publicUrlin stored file metadata.This URL is generated for the current write request using
context?.authedItem, so signed or user-scoped URLs can become stale or wrong for later readers. Persist stable file identifiers only, and compute the current URL on read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/Implementation.js` around lines 282 - 286, The current write path adds a request-scoped publicUrl into the persisted file record by returning { ...saved, publicUrl, meta: { format: this.config.format }, _type: FILE_META_TYPE }; remove publicUrl from the object returned here so only stable identifiers and meta are persisted (e.g., return { ...saved, meta: { format: this.config.format }, _type: FILE_META_TYPE }). Keep the call to this.adapter.publicUrl(saved, context?.authedItem) for generating URLs, but move its usage to the read/resolve path (where files are read or serialized for clients) so the adapter's publicUrl is computed per-request rather than stored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/keystone/fields/ExternalContent/Implementation.js`:
- Around line 137-144: The resolver warning for ExternalContent uses data: {
path: this.path } but should follow the field-resolver log shape and use data: {
field: this.path } instead; update the logger.warn call in the ExternalContent
resolver (the block that constructs itemId from item?.id and calls logger.warn)
to replace data.path with data.field while keeping the same msg, entity
(this.listKey), entityId (itemId) and err fields.
- Around line 282-286: The current write path adds a request-scoped publicUrl
into the persisted file record by returning { ...saved, publicUrl, meta: {
format: this.config.format }, _type: FILE_META_TYPE }; remove publicUrl from the
object returned here so only stable identifiers and meta are persisted (e.g.,
return { ...saved, meta: { format: this.config.format }, _type: FILE_META_TYPE
}). Keep the call to this.adapter.publicUrl(saved, context?.authedItem) for
generating URLs, but move its usage to the read/resolve path (where files are
read or serialized for clients) so the adapter's publicUrl is computed
per-request rather than stored.
In `@packages/keystone/fields/ExternalContent/views/Controller.js`:
- Around line 31-35: The deserialize method in Controller.js (and the
corresponding serialize method) currently treats any falsy value (''/0/false) as
missing by checking if (!data?.[path]), which prevents valid falsy payloads from
round-tripping; change the presence check to explicitly detect undefined or null
(e.g., check whether data has the key or whether data[path] === undefined) so
that ''/0/false are preserved and only absent/undefined values return null.
Ensure you update both deserialize and the serialize/other similar block
referenced around lines 64-68 using the same explicit undefined/null check or
hasOwnProperty approach.
- Around line 86-94: getQueryFragment currently returns "${this.path}Resolved"
bare which fails for non-scalar resolved types; update getQueryFragment to
append graphQLAdminFragment when including the resolved field (i.e., return
`${this.path}\n${this.path}Resolved${graphQLAdminFragment}` or equivalent) so
the resolved field is queried with the correct admin fragment; ensure you
reference getQueryFragment, this.path and this.pathResolved (or
`${this.path}Resolved`) and include graphQLAdminFragment only for the resolved
entry.
- Around line 70-83: The code is double-serializing admin edits by calling
processor.serialize(value) here; instead return the GraphQL input shape (i.e.,
do not call processor.serialize). Update the block in Controller.js (referencing
getProcessor and serialize) to return the raw/GraphQL-ready value that
Implementation.resolveInput() expects (or, if your processor exposes a specific
toGraphQL/toInput method, call that instead of serialize).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92688bb8-4c3c-4ddd-9c42-0b31ed058eb6
📒 Files selected for processing (2)
packages/keystone/fields/ExternalContent/Implementation.jspackages/keystone/fields/ExternalContent/views/Controller.js
…k in ExternalContent field
…nConfig in ExternalContentController getQueryFragment
update XmlLog.log field config
…utomatic file cleanup
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/keystone/fields/ExternalContent/views/Controller.js (3)
27-33:⚠️ Potential issue | 🟠 MajorDon't collapse falsy payloads to
null.Both
deserialize()andserialize()still use!data?.[path], so'',0, andfalseare treated as missing instead of round-tripping through the admin UI.Suggested fix
deserialize = data => { const { path } = this - if (!data?.[path]) { + const value = data?.[path] + if (value === null || value === undefined) { return null } - - const value = data[path] @@ serialize = data => { const { path } = this - if (!data?.[path]) { + const value = data?.[path] + if (value === null || value === undefined) { return null } - - const value = data[path]Also applies to: 60-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 27 - 33, In deserialize and serialize, avoid treating falsy values ('', 0, false) as missing by replacing the current existence check (!data?.[path]) with a check that only treats undefined or missing keys as absent—e.g. use Object.prototype.hasOwnProperty.call(data, path) or data[path] === undefined (and also guard if data is null/undefined) so that legitimate falsy payloads round-trip; update both deserialize (function deserialize) and serialize (function serialize) accordingly (also apply same fix to the other occurrence around the 60-64 region).
66-79:⚠️ Potential issue | 🔴 CriticalStop serializing admin input in the controller.
Field.jsalready sends textarea text, andImplementation.resolveInput()serializes for storage again. Callingprocessor.serialize(value)here double-encodes JSON edits, so{ "a": 1 }comes back from${this.path}Resolvedas a string instead of an object.Suggested fix
- // Use processor's serialize function - try { - return processor.serialize(value) - } catch (err) { - return value - } + if (this.format !== 'json' || typeof value !== 'string') { + return value + } + + try { + return value.trim() === '' ? null : JSON.parse(value) + } catch (err) { + return value + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 66 - 79, The controller is double-serializing admin textarea input by calling processor.serialize(value) and returning a JSON string; stop doing that — in the method that reads data[path] (the code using this.getProcessor() and processor.serialize(value)), return the raw value (data[path]) directly so that Implementation.resolveInput() can perform serialization for storage; remove the try/catch around processor.serialize and any call to processor.serialize(value), but keep the existing fallback when !this.getProcessor() and ensure ${this.path}Resolved receives the original object/textarea value rather than a serialized string.
82-87:⚠️ Potential issue | 🟠 MajorAppend
graphQLAdminFragmentwhen queryingResolved.Bare
${this.path}Resolvedonly works for scalar return types. If a processor exposes an object type, editable admin screens will issue an invalid query unless the configured fragment is applied here.Suggested fix
getQueryFragment = () => { // For editable fields, also query the resolved content // For read-only fields, only query the raw metadata if (this.config?.adminConfig?.isReadOnly === false) { - return `${this.path}\n${this.path}Resolved` + const resolvedField = this.config?.graphQLAdminFragment + ? `${this.path}Resolved ${this.config.graphQLAdminFragment}` + : `${this.path}Resolved` + return `${this.path}\n${resolvedField}` } // Return empty fragment to query the database column directly // This bypasses the GraphQL field resolver and returns raw file metadata return `${this.path}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 82 - 87, getQueryFragment currently appends only `${this.path}Resolved` which fails for non-scalar resolved types; update getQueryFragment to append the configured fragment (graphQLAdminFragment) when querying the resolved field so editable admin screens produce valid GraphQL (i.e., return `${this.path}Resolved ${this.config?.adminConfig?.graphQLAdminFragment}` or equivalent), referencing the getQueryFragment method, this.path and the config.adminConfig.graphQLAdminFragment to locate and apply the change.packages/keystone/fields/ExternalContent/views/Field.js (1)
35-54:⚠️ Potential issue | 🟠 MajorUse nullish checks when hydrating
editValue.
resolvedContentandlegacyContentare still gated by truthiness, so'',0, andfalsefall through to the raw fallback instead of rendering the actual stored value.Suggested fix
- if (resolvedContent) { + if (resolvedContent !== null && resolvedContent !== undefined) { initialEditValue = typeof resolvedContent === 'string' ? resolvedContent : JSON.stringify(resolvedContent, null, 2) - } else if (legacyContent) { + } else if (legacyContent !== null && legacyContent !== undefined) { initialEditValue = legacyContent } else if (typeof value === 'string') { initialEditValue = value @@ - if (resolvedContent) { + if (resolvedContent !== null && resolvedContent !== undefined) { setEditValue(typeof resolvedContent === 'string' ? resolvedContent : JSON.stringify(resolvedContent, null, 2)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Field.js` around lines 35 - 54, The hydration logic for initialEditValue and the useEffect uses truthy checks so stored falsy values like '' , 0, or false get ignored; change the conditions to nullish checks (e.g. resolvedContent !== null && resolvedContent !== undefined or resolvedContent ??) for resolvedContent and legacyContent (and similar for value when distinguishing string vs object) so that falsy-but-valid values are preserved when computing initialEditValue and when calling setEditValue in the useEffect; update the checks around initialEditValue, the useState initializer (initialEditValue), and the effect body that calls setEditValue to use nullish checks rather than truthiness.packages/keystone/fields/ExternalContent/Implementation.js (1)
244-246:⚠️ Potential issue | 🟠 MajorDon't persist
publicUrlin the stored file metadata.
adapter.publicUrl(saved, context?.authedItem)can be user-scoped or expiring. Saving it here means later readers inpackages/keystone/fields/ExternalContent/views/Field.jsreuse the writer's URL instead of resolving one for the current request. Store only stable file identifiers in DB and derive the current URL on read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/Implementation.js` around lines 244 - 246, The returned saved file object in ExternalContent/Implementation.js must not persist a transient or user-scoped publicUrl; remove adapter.publicUrl(saved, context?.authedItem) from the stored payload and return only stable identifiers (e.g., the saved object, meta: { format: this.config.format }, _type: FILE_META_TYPE). Update the consumer (ExternalContent/views/Field.js) to call this.adapter.publicUrl(fileRecord, context?.authedItem) at read/render time so each request resolves a fresh, user-scoped or expiring URL instead of reusing the writer's URL.
🧹 Nitpick comments (1)
packages/keystone/fields/ExternalContent/Implementation.spec.js (1)
459-460: Assert loader behavior, not_externalContentLoadersinternals.These checks pin the spec to the current cache shape instead of the public behavior. The resolved values plus
mockReadFilecall counts already prove reuse and separation without reaching intocontext._externalContentLoaders.As per coding guidelines, "Test public APIs and behavior, not implementation details - avoid exporting constants solely for testing".
Also applies to: 519-520, 553-553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/Implementation.spec.js` around lines 459 - 460, Remove assertions that inspect the internal cache shape (context._externalContentLoaders and its size) and instead assert public behavior: verify the returned/resolved values from the loader calls and the mockReadFile call counts to prove reuse/separation. Update the three occurrences (around lines with expect(context._externalContentLoaders)... and the other two mentions) to drop the internal inspections and rely on existing assertions that check resolved values and mockReadFile.mock.calls (or mockReadFile.mock.calls.length) to demonstrate caching and isolation.
🤖 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/keystone/fields/ExternalContent/views/Controller.js`:
- Around line 1-3: The file imports FieldController from the forbidden package
'@open-keystone/fields/Controller' and requires DEFAULT_PROCESSORS from
'@open-condo/keystone/fieldsUtils/ExternalContent/defaultProcessors'; replace
both with the project-approved internal exports instead: import the
FieldController symbol from the repository's internal fields controller export
and import DEFAULT_PROCESSORS from the internal fields utils external-content
default processors export (keep the same symbol names FieldController and
DEFAULT_PROCESSORS so usages in Controller.js remain unchanged), then run the
build/tests to verify no other forbidden imports remain.
---
Duplicate comments:
In `@packages/keystone/fields/ExternalContent/Implementation.js`:
- Around line 244-246: The returned saved file object in
ExternalContent/Implementation.js must not persist a transient or user-scoped
publicUrl; remove adapter.publicUrl(saved, context?.authedItem) from the stored
payload and return only stable identifiers (e.g., the saved object, meta: {
format: this.config.format }, _type: FILE_META_TYPE). Update the consumer
(ExternalContent/views/Field.js) to call this.adapter.publicUrl(fileRecord,
context?.authedItem) at read/render time so each request resolves a fresh,
user-scoped or expiring URL instead of reusing the writer's URL.
In `@packages/keystone/fields/ExternalContent/views/Controller.js`:
- Around line 27-33: In deserialize and serialize, avoid treating falsy values
('', 0, false) as missing by replacing the current existence check
(!data?.[path]) with a check that only treats undefined or missing keys as
absent—e.g. use Object.prototype.hasOwnProperty.call(data, path) or data[path]
=== undefined (and also guard if data is null/undefined) so that legitimate
falsy payloads round-trip; update both deserialize (function deserialize) and
serialize (function serialize) accordingly (also apply same fix to the other
occurrence around the 60-64 region).
- Around line 66-79: The controller is double-serializing admin textarea input
by calling processor.serialize(value) and returning a JSON string; stop doing
that — in the method that reads data[path] (the code using this.getProcessor()
and processor.serialize(value)), return the raw value (data[path]) directly so
that Implementation.resolveInput() can perform serialization for storage; remove
the try/catch around processor.serialize and any call to
processor.serialize(value), but keep the existing fallback when
!this.getProcessor() and ensure ${this.path}Resolved receives the original
object/textarea value rather than a serialized string.
- Around line 82-87: getQueryFragment currently appends only
`${this.path}Resolved` which fails for non-scalar resolved types; update
getQueryFragment to append the configured fragment (graphQLAdminFragment) when
querying the resolved field so editable admin screens produce valid GraphQL
(i.e., return `${this.path}Resolved
${this.config?.adminConfig?.graphQLAdminFragment}` or equivalent), referencing
the getQueryFragment method, this.path and the
config.adminConfig.graphQLAdminFragment to locate and apply the change.
In `@packages/keystone/fields/ExternalContent/views/Field.js`:
- Around line 35-54: The hydration logic for initialEditValue and the useEffect
uses truthy checks so stored falsy values like '' , 0, or false get ignored;
change the conditions to nullish checks (e.g. resolvedContent !== null &&
resolvedContent !== undefined or resolvedContent ??) for resolvedContent and
legacyContent (and similar for value when distinguishing string vs object) so
that falsy-but-valid values are preserved when computing initialEditValue and
when calling setEditValue in the useEffect; update the checks around
initialEditValue, the useState initializer (initialEditValue), and the effect
body that calls setEditValue to use nullish checks rather than truthiness.
---
Nitpick comments:
In `@packages/keystone/fields/ExternalContent/Implementation.spec.js`:
- Around line 459-460: Remove assertions that inspect the internal cache shape
(context._externalContentLoaders and its size) and instead assert public
behavior: verify the returned/resolved values from the loader calls and the
mockReadFile call counts to prove reuse/separation. Update the three occurrences
(around lines with expect(context._externalContentLoaders)... and the other two
mentions) to drop the internal inspections and rely on existing assertions that
check resolved values and mockReadFile.mock.calls (or
mockReadFile.mock.calls.length) to demonstrate caching and isolation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f888f6c9-13bb-437e-8c3f-e2491943d9f9
📒 Files selected for processing (6)
apps/rbpackages/keystone/fields/ExternalContent/Implementation.jspackages/keystone/fields/ExternalContent/Implementation.spec.jspackages/keystone/fields/ExternalContent/views/Controller.jspackages/keystone/fields/ExternalContent/views/Field.jspackages/keystone/fieldsUtils/ExternalContent/createExternalDataField.js
✅ Files skipped from review due to trivial changes (1)
- apps/rb
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/keystone/fieldsUtils/ExternalContent/createExternalDataField.js
Comment on lines
+1
to
+3
| import FieldController from '@open-keystone/fields/Controller' | ||
|
|
||
| const { DEFAULT_PROCESSORS } = require('@open-condo/keystone/fieldsUtils/ExternalContent/defaultProcessors') |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid @open-keystone/fields* imports in new field views.
This controller is reintroducing an import family the repo explicitly forbids. Please switch this to the approved internal export before more code depends on it.
As per coding guidelines, "Don't import from @open-keystone/fields* or @open-condo/keystone/fields".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 1
- 3, The file imports FieldController from the forbidden package
'@open-keystone/fields/Controller' and requires DEFAULT_PROCESSORS from
'@open-condo/keystone/fieldsUtils/ExternalContent/defaultProcessors'; replace
both with the project-approved internal exports instead: import the
FieldController symbol from the repository's internal fields controller export
and import DEFAULT_PROCESSORS from the internal fields utils external-content
default processors export (keep the same symbol names FieldController and
DEFAULT_PROCESSORS so usages in Controller.js remain unchanged), then run the
build/tests to verify no other forbidden imports remain.
Member
Author
There was a problem hiding this comment.
@open-keystone/fields/Controller is used in all the other field controllers in the same package (Json, Options, ...) - it's the standard pattern here, not a forbidden import. And @open-condo/keystone/fieldsUtils/ExternalContent/defaultProcessors is already the internal path.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other field controllers import FieldController
rg "from '@open-keystone/fields/Controller'" packages/keystone/fields --include="*.js" -n
rg "require('@open-keystone/fields/Controller')" packages/keystone/fields --include="*.js" -n
Length of output: 883
🏁 Script executed:
#!/bin/bash
# Check how other field controllers import FieldController
rg "from '@open-keystone/fields/Controller'" packages/keystone/fields -n --type js
Length of output: 358
@AleX83Xpert, you're right on both counts. I've confirmed that packages/keystone/fields/Json/views/Controller.js and packages/keystone/fields/Options/views/Controller.js both use the identical import FieldController from '@open-keystone/fields/Controller' pattern — this is the established standard for extending field controllers within packages/keystone/fields, and the guideline against @open-keystone/fields* imports is intended for app-level and consumer code, not for field implementations themselves. Similarly, @open-condo/keystone/fieldsUtils/ExternalContent/defaultProcessors is already an internal path. My original comment was incorrect. Apologies for the noise!
✏️ Learnings added
Learnt from: AleX83Xpert
Repo: open-condo-software/condo PR: 7412
File: packages/keystone/fields/ExternalContent/views/Controller.js:1-3
Timestamp: 2026-04-13T11:30:59.702Z
Learning: In `packages/keystone/fields`, field controller files (e.g. `views/Controller.js`) must import `FieldController` from `'open-keystone/fields/Controller'` — this is the established pattern used by all field controllers (Json, Options, ExternalContent, etc.) and is explicitly allowed within `packages/keystone/fields`. The guideline "Don't import from `open-keystone/fields`*" applies only to app-level and consumer code, not to field implementations inside `packages/keystone/fields` itself.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: AleX83Xpert
Repo: open-condo-software/condo PR: 7412
File: packages/keystone/fields/ExternalContent/views/Controller.js:27-31
Timestamp: 2026-04-13T11:29:01.532Z
Learning: In `packages/keystone/fields/ExternalContent/views/Controller.js`, the `deserialize()` and `serialize()` methods use `!data?.[path]` to detect a missing/empty value. This is intentional and correct: the ExternalContent field stores only a JSON string of file metadata or a legacy content string — falsy primitives like `''`, `0`, or `false` are never valid stored values for this field, so the falsy guard does not lose any real data.
Learnt from: CR
Repo: open-condo-software/condo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T14:03:17.418Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Don't import from open-keystone/fields* or open-condo/keystone/fields
Learnt from: CR
Repo: open-condo-software/condo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T14:03:17.418Z
Learning: Applies to apps/*/!(node_modules)/**/*.{js,ts,jsx,tsx} : Prefer importing from open-condo packages in /packages/* over internal app code for reusable functionality
Learnt from: YEgorLu
Repo: open-condo-software/condo PR: 7412
File: packages/keystone/fields/ExternalContent/Implementation.js:156-158
Timestamp: 2026-04-08T06:31:31.532Z
Learning: When emitting logs from Keystone field resolvers (any file under packages/keystone/fields/*.js), use the standard log schema: set `entity` to the `listKey`, set `entityId` to the item’s id, and put field-specific context under `data` (e.g., `data: { field: this.path }`). Do not introduce custom root-level keys like `field` or `itemId`; keep those details inside `data` to ensure logs are consistent and queryable.
Learnt from: AleX83Xpert
Repo: open-condo-software/condo PR: 7139
File: packages/keystone/runtimeStats.js:169-188
Timestamp: 2026-01-28T10:49:33.372Z
Learning: In log serialization (e.g., in the Condo codebase's logging serializers), ensure that top-level fields such as reqId, url, method, path, and ip are kept at the root of the log object alongside existing fields like msg, data, entityId, entity, count, status, and err. Do not nest these fields inside the data object or move them into other nested structures.
… before processing ExternalContent values
…dation error message
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
packages/keystone/fields/ExternalContent/views/Controller.js (4)
27-33:⚠️ Potential issue | 🟠 MajorPreserve falsy payloads in admin round-trips.
!data?.[path]still treats'',0, andfalseas missing. On editable fields those values deserialize/serialize asnull, so valid content gets cleared just by opening and saving the item.Also applies to: 60-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 27 - 33, The deserialize function's existence check (deserialize in Controller.js using !data?.[path]) treats falsy values like '', 0, and false as missing and wipes valid content; change the guard to explicitly check for missing data or missing key (e.g., data == null || !(this.path in data)) so falsy values are preserved; apply the same fix to the corresponding serialize check around the serialize function (the block referenced at lines ~60-66) to ensure round-trips keep falsy payloads.
1-1: 🛠️ Refactor suggestion | 🟠 MajorStop importing
FieldControllerfrom@open-keystone/fields/*.This new view reintroduces a forbidden dependency family. Please switch it to the repo-approved internal controller export before more field views depend on the upstream path directly.
#!/bin/bash # Compare FieldController imports across sibling field views to find the repo-approved export path. rg -n --iglob '*/views/Controller.js' "FieldController|@open-keystone/fields/Controller" packages/keystone/fieldsAs per coding guidelines, "Don't import from
@open-keystone/fields* or@open-condo/keystone/fields".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` at line 1, The file Controller.js currently imports FieldController from the forbidden upstream path "@open-keystone/fields/Controller"; replace that import with the repo-approved internal export used by our other field views (i.e., import FieldController from the internal controller export used across sibling field view files), ensuring the imported symbol remains FieldController and updating the import path to the internal package/alias the repo uses for field controllers; verify by checking other views' Controller.js files for the canonical import and match that path.
82-87:⚠️ Potential issue | 🟠 MajorCarry
graphQLAdminFragmentonto${path}Resolved.Editable admin queries still request
${this.path}Resolvedas a bare field. That works for scalars, but any custom processor returning a non-scalar type will break item screens because the resolved field is missing its fragment selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 82 - 87, The getQueryFragment method currently returns `${this.path}Resolved` as a bare field which breaks non-scalar resolved types; update getQueryFragment (in Controller.js) to append the existing graphQLAdminFragment selection to the resolved field — e.g., return `${this.path}\n${this.path}Resolved ${this.graphQLAdminFragment}` (or interpolate graphQLAdminFragment appropriately) so `${path}Resolved` carries the fragment selection for custom processors.
66-79:⚠️ Potential issue | 🔴 CriticalDon't serialize JSON twice before
resolveInput().
Field.jssends textarea text, this controller runsprocessor.serialize(value), andImplementation.resolveInput()serializes again on the server. Forformat: 'json', editing{ "a": 1 }ends up saving a JSON string payload instead of an object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Controller.js` around lines 66 - 79, The controller is double-serializing JSON: it calls processor.serialize(value) in getProcessor()/serialize path but the server side Implementation.resolveInput() also serializes for format: 'json', causing stored payloads to be JSON strings. Update the Controller.js logic to skip client-side serialization for JSON-formatted processors — e.g. check processor.format === 'json' (or detect JSON type) and simply return the raw value; otherwise continue to call processor.serialize(value). Keep references to getProcessor(), processor.serialize, and resolveInput() so reviewers can locate the change.packages/keystone/fields/ExternalContent/Implementation.js (1)
244-246:⚠️ Potential issue | 🟠 MajorDon't persist request-scoped
publicUrls in file meta.This writes the URL minted for the updater into the stored payload. The AWS ACL helper uses short-lived signed URLs by default, so
views/Field.jslater renders links that are already stale or scoped to the wrong viewer. Persist only stable file identifiers here and mint the current URL on read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/Implementation.js` around lines 244 - 246, The code is persisting a request-scoped signed URL by assigning publicUrl = this.adapter.publicUrl(saved, context?.authedItem) into the saved payload; remove that so only stable identifiers are stored (e.g., keep saved as-is with meta: { format: this.config.format } and FILE_META_TYPE) and do NOT write publicUrl into the database. Instead mint the signed/reader-scoped URL at read/render time (e.g., in the resolve/serialization path used by views/Field.js or the field resolver) by calling this.adapter.publicUrl(saved, viewer) so links are fresh and scoped per-request.packages/keystone/fields/ExternalContent/views/Field.js (1)
35-44:⚠️ Potential issue | 🟠 MajorHydrate the editor with nullish checks, not truthy checks.
resolvedContentandlegacyContentcan legitimately be'',0, orfalse. The current checks skip those values, so the textarea falls back to stale/raw metadata instead of the actual payload.Also applies to: 49-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/keystone/fields/ExternalContent/views/Field.js` around lines 35 - 44, The logic that hydrates initialEditValue skips valid falsy payloads because it uses truthy checks; change the checks on resolvedContent and legacyContent (and the similar block around lines 49-52) to nullish checks (e.g., resolvedContent !== null && resolvedContent !== undefined or the nullish coalescing operator) so that values like '', 0, and false are accepted; preserve the existing string vs object handling (JSON.stringify) for value when it is an object, and keep the same fallback order using these nullish checks on resolvedContent, legacyContent, and value.
🤖 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/keystone/fields/ExternalContent/Implementation.js`:
- Around line 106-113: extendAdminMeta currently returns adminConfig from
this.adminConfig which is never set; change it to read adminConfig from
this.config (e.g., use this.config.adminConfig || {}) so the admin meta includes
the configured admin options. Update the extendAdminMeta method (in
Implementation.js) to return adminConfig: this.config.adminConfig || {} so the
UI components (views/Controller.js and views/Field.js) receive the intended
isReadOnly and other admin settings.
In `@packages/keystone/fields/ExternalContent/README.md`:
- Around line 103-118: The README shows inconsistent usage of the
ExternalContent field (sometimes as type: 'ExternalContent' and sometimes as
type: ExternalContent) and the access-control example never imports
ExternalContent, which will break copy-paste; pick one style and make all
examples consistent—either use the identifier form and add the import/require
for ExternalContent and FileAdapter (e.g.,
require('@open-condo/keystone/fields').ExternalContent and
require('@open-condo/keystone/fileAdapter/fileAdapter') for MyFieldFileAdapter)
or use the string form ('ExternalContent') everywhere; update all blocks
including the schema example (MySchema with fields.myData and adapter:
MyFieldFileAdapter, format, maxSizeBytes) and the access-control example to
match that chosen form so examples are copy-paste safe.
In `@packages/keystone/fields/ExternalContent/views/Field.js`:
- Around line 77-85: The jsx in ExternalContent Field uses Emotion's css prop on
the textarea and anchor elements which triggers react/no-unknown-property;
either remove the css={{...}} props in the Field component and apply equivalent
styling via the standard style prop or a className/CSS module (replace css on
the textarea and the anchor) or alternatively install and configure
eslint-plugin-emotion (and ensure the Emotion pragma is recognized) so the css
prop is allowed; locate the occurrences by searching for the textarea and the
anchor elements in Field.js and update them accordingly.
---
Duplicate comments:
In `@packages/keystone/fields/ExternalContent/Implementation.js`:
- Around line 244-246: The code is persisting a request-scoped signed URL by
assigning publicUrl = this.adapter.publicUrl(saved, context?.authedItem) into
the saved payload; remove that so only stable identifiers are stored (e.g., keep
saved as-is with meta: { format: this.config.format } and FILE_META_TYPE) and do
NOT write publicUrl into the database. Instead mint the signed/reader-scoped URL
at read/render time (e.g., in the resolve/serialization path used by
views/Field.js or the field resolver) by calling this.adapter.publicUrl(saved,
viewer) so links are fresh and scoped per-request.
In `@packages/keystone/fields/ExternalContent/views/Controller.js`:
- Around line 27-33: The deserialize function's existence check (deserialize in
Controller.js using !data?.[path]) treats falsy values like '', 0, and false as
missing and wipes valid content; change the guard to explicitly check for
missing data or missing key (e.g., data == null || !(this.path in data)) so
falsy values are preserved; apply the same fix to the corresponding serialize
check around the serialize function (the block referenced at lines ~60-66) to
ensure round-trips keep falsy payloads.
- Line 1: The file Controller.js currently imports FieldController from the
forbidden upstream path "@open-keystone/fields/Controller"; replace that import
with the repo-approved internal export used by our other field views (i.e.,
import FieldController from the internal controller export used across sibling
field view files), ensuring the imported symbol remains FieldController and
updating the import path to the internal package/alias the repo uses for field
controllers; verify by checking other views' Controller.js files for the
canonical import and match that path.
- Around line 82-87: The getQueryFragment method currently returns
`${this.path}Resolved` as a bare field which breaks non-scalar resolved types;
update getQueryFragment (in Controller.js) to append the existing
graphQLAdminFragment selection to the resolved field — e.g., return
`${this.path}\n${this.path}Resolved ${this.graphQLAdminFragment}` (or
interpolate graphQLAdminFragment appropriately) so `${path}Resolved` carries the
fragment selection for custom processors.
- Around line 66-79: The controller is double-serializing JSON: it calls
processor.serialize(value) in getProcessor()/serialize path but the server side
Implementation.resolveInput() also serializes for format: 'json', causing stored
payloads to be JSON strings. Update the Controller.js logic to skip client-side
serialization for JSON-formatted processors — e.g. check processor.format ===
'json' (or detect JSON type) and simply return the raw value; otherwise continue
to call processor.serialize(value). Keep references to getProcessor(),
processor.serialize, and resolveInput() so reviewers can locate the change.
In `@packages/keystone/fields/ExternalContent/views/Field.js`:
- Around line 35-44: The logic that hydrates initialEditValue skips valid falsy
payloads because it uses truthy checks; change the checks on resolvedContent and
legacyContent (and the similar block around lines 49-52) to nullish checks
(e.g., resolvedContent !== null && resolvedContent !== undefined or the nullish
coalescing operator) so that values like '', 0, and false are accepted; preserve
the existing string vs object handling (JSON.stringify) for value when it is an
object, and keep the same fallback order using these nullish checks on
resolvedContent, legacyContent, and value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3121357-e561-4220-863e-2f2d11358b65
📒 Files selected for processing (7)
apps/rbpackages/keystone/fields/ExternalContent/Implementation.jspackages/keystone/fields/ExternalContent/Implementation.spec.jspackages/keystone/fields/ExternalContent/README.mdpackages/keystone/fields/ExternalContent/views/Controller.jspackages/keystone/fields/ExternalContent/views/Field.jspackages/keystone/fieldsUtils/ExternalContent/createExternalDataField.js
✅ Files skipped from review due to trivial changes (1)
- apps/rb
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/keystone/fieldsUtils/ExternalContent/createExternalDataField.js
Comment on lines
+106
to
+113
| extendAdminMeta (meta) { | ||
| return { | ||
| ...meta, | ||
| graphQLAdminFragment: this.config.graphQLAdminFragment || '', | ||
| format: this.config.format, | ||
| processors: this.formatProcessors, | ||
| adminConfig: this.adminConfig || {}, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare admin-meta builders and how they propagate adminConfig.
rg -n -C2 "extendAdminMeta|this\.config\.adminConfig|this\.adminConfig|adminConfig:" packages/keystone/fieldsRepository: open-condo-software/condo
Length of output: 3285
🏁 Script executed:
cat -n packages/keystone/fields/ExternalContent/Implementation.js | head -150Repository: open-condo-software/condo
Length of output: 6852
🏁 Script executed:
rg -n "constructor|this\.adminConfig\s*=" packages/keystone/fields/ExternalContent/Implementation.jsRepository: open-condo-software/condo
Length of output: 237
🏁 Script executed:
rg -n "adminConfig:" packages/keystone/fields -A1 -B1 | head -60Repository: open-condo-software/condo
Length of output: 972
🏁 Script executed:
rg -n "adminConfig" packages/keystone/fields/ExternalContent/views -A2 -B2Repository: open-condo-software/condo
Length of output: 1183
🏁 Script executed:
rg -n "field\.config\.adminConfig|adminConfig\.isReadOnly" packages/keystone/fields -lRepository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
rg -n "this\.adminConfig\s*=" packages/keystone/fields/ExternalContentRepository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
rg -n "Object\.defineProperty.*adminConfig" packages/keystone/fields/ExternalContentRepository: open-condo-software/condo
Length of output: 51
Pass adminConfig from this.config into admin meta.
extendAdminMeta() references this.adminConfig, which is never set. The constructor passes options to the parent Implementation class, which stores them in this.config. Both views/Controller.js and views/Field.js expect adminConfig to be read from field.config?.adminConfig, but extendAdminMeta() returns an undefined value, causing the admin UI to default to read-only mode instead of respecting the configured isReadOnly setting.
Suggested change
return {
...meta,
graphQLAdminFragment: this.config.graphQLAdminFragment || '',
format: this.config.format,
processors: this.formatProcessors,
- adminConfig: this.adminConfig || {},
+ adminConfig: this.config.adminConfig || {},
}📝 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.
Suggested change
| extendAdminMeta (meta) { | |
| return { | |
| ...meta, | |
| graphQLAdminFragment: this.config.graphQLAdminFragment || '', | |
| format: this.config.format, | |
| processors: this.formatProcessors, | |
| adminConfig: this.adminConfig || {}, | |
| } | |
| extendAdminMeta (meta) { | |
| return { | |
| ...meta, | |
| graphQLAdminFragment: this.config.graphQLAdminFragment || '', | |
| format: this.config.format, | |
| processors: this.formatProcessors, | |
| adminConfig: this.config.adminConfig || {}, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/keystone/fields/ExternalContent/Implementation.js` around lines 106
- 113, extendAdminMeta currently returns adminConfig from this.adminConfig which
is never set; change it to read adminConfig from this.config (e.g., use
this.config.adminConfig || {}) so the admin meta includes the configured admin
options. Update the extendAdminMeta method (in Implementation.js) to return
adminConfig: this.config.adminConfig || {} so the UI components
(views/Controller.js and views/Field.js) receive the intended isReadOnly and
other admin settings.
Comment on lines
+103
to
+118
| ```javascript | ||
| const { ExternalContent } = require('@open-condo/keystone/fields') | ||
| const FileAdapter = require('@open-condo/keystone/fileAdapter/fileAdapter') | ||
|
|
||
| const MyFieldFileAdapter = new FileAdapter('MyFieldFolder') // The folder where files will be stored | ||
|
|
||
| const MySchema = { | ||
| fields: { | ||
| myData: { | ||
| type: 'ExternalContent', | ||
| adapter: MyFieldFileAdapter, | ||
| format: 'json', // or 'xml', 'text' | ||
| maxSizeBytes: 10 * 1024 * 1024, // 10MB (optional, default: 10MB) | ||
| }, | ||
| }, | ||
| } |
Contributor
There was a problem hiding this comment.
Make the direct-configuration examples internally consistent.
The README alternates between type: 'ExternalContent' and type: ExternalContent, but the access-control example never imports ExternalContent. Copy-pasting that block will fail or at least send readers down the wrong path. Pick one form and reuse it consistently.
Also applies to: 346-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/keystone/fields/ExternalContent/README.md` around lines 103 - 118,
The README shows inconsistent usage of the ExternalContent field (sometimes as
type: 'ExternalContent' and sometimes as type: ExternalContent) and the
access-control example never imports ExternalContent, which will break
copy-paste; pick one style and make all examples consistent—either use the
identifier form and add the import/require for ExternalContent and FileAdapter
(e.g., require('@open-condo/keystone/fields').ExternalContent and
require('@open-condo/keystone/fileAdapter/fileAdapter') for MyFieldFileAdapter)
or use the string form ('ExternalContent') everywhere; update all blocks
including the schema example (MySchema with fields.myData and adapter:
MyFieldFileAdapter, format, maxSizeBytes) and the access-control example to
match that chosen form so examples are copy-paste safe.
Comment on lines
+77
to
+85
| css={{ | ||
| width: '100%', | ||
| minHeight: '200px', | ||
| fontFamily: 'monospace', | ||
| fontSize: '12px', | ||
| padding: '8px', | ||
| border: '1px solid #ccc', | ||
| borderRadius: '4px', | ||
| }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the offending props and inspect repo config for Emotion/React property support.
sed -n '70,110p' packages/keystone/fields/ExternalContent/views/Field.js
rg -n "@emotion|react/no-unknown-property|jsxImportSource" --iglob '.eslintrc*' --iglob 'package.json' --iglob 'babel.config.*' --iglob 'tsconfig*.json' .Repository: open-condo-software/condo
Length of output: 2210
🏁 Script executed:
#!/bin/bash
# Check if packages/keystone has tsconfig with jsxImportSource
ls -la packages/keystone/ | grep -E 'tsconfig|babel'
cat packages/keystone/tsconfig.json 2>/dev/null || echo "tsconfig.json not found in packages/keystone"Repository: open-condo-software/condo
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Check the actual file imports and setup
cat -n packages/keystone/fields/ExternalContent/views/Field.js | head -30Repository: open-condo-software/condo
Length of output: 1441
🏁 Script executed:
#!/bin/bash
# Search for ESLint config and react/no-unknown-property rule
find . -maxdepth 3 -name '.eslintrc*' -o -name '.eslintrc.json' | head -5
rg -A 5 "react/no-unknown-property" .eslintrc* 2>/dev/null || rg -A 5 "react/no-unknown-property" --type json --type jsRepository: open-condo-software/condo
Length of output: 249
🏁 Script executed:
#!/bin/bash
# Check ESLint configurations
echo "=== ROOT .eslintrc.js ==="
cat ./.eslintrc.js | head -60
echo ""
echo "=== packages/.eslintrc.json ==="
cat ./packages/.eslintrc.jsonRepository: open-condo-software/condo
Length of output: 2070
🏁 Script executed:
#!/bin/bash
# Verify if packages/keystone overrides ESLint config
ls -la packages/keystone/ | grep eslint
cat packages/keystone/.eslintrc.json 2>/dev/null || echo "No ESLint config in packages/keystone"Repository: open-condo-software/condo
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Check the full .eslintrc.js for react/no-unknown-property rule and any emotion plugin config
cat ./.eslintrc.js | grep -A 20 "react/no-unknown-property"
# Also search for emotion plugin references
rg "emotion" ./.eslintrc.jsRepository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check if eslint-plugin-emotion is installed or configured
rg "eslint-plugin-emotion|emotion.*eslint" package.json packages/keystone/package.json apps/*/package.jsonRepository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check the complete rules section of .eslintrc.js
tail -100 ./.eslintrc.js | head -80Repository: open-condo-software/condo
Length of output: 3150
Remove the css props or configure ESLint for Emotion support.
The css={{ ... }} props on textarea (lines 77-85) and a (lines 95-99) are flagged by ESLint's react/no-unknown-property rule. While the file has the correct Emotion pragma (/** @jsx jsx */), the current ESLint configuration does not recognize it. Either remove these props and use alternative styling approaches, or install and configure eslint-plugin-emotion to handle Emotion's JSX pragma.
🧰 Tools
🪛 ESLint
[error] 77-85: Unknown property 'css' found
(react/no-unknown-property)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/keystone/fields/ExternalContent/views/Field.js` around lines 77 -
85, The jsx in ExternalContent Field uses Emotion's css prop on the textarea and
anchor elements which triggers react/no-unknown-property; either remove the
css={{...}} props in the Field component and apply equivalent styling via the
standard style prop or a className/CSS module (replace css on the textarea and
the anchor) or alternatively install and configure eslint-plugin-emotion (and
ensure the Emotion pragma is recognized) so the css prop is allowed; locate the
occurrences by searching for the textarea and the anchor elements in Field.js
and update them accordingly.
…e adminConfig instead of isAdminUIReadOnly
|
sitozzz
reviewed
Apr 14, 2026
|
|
||
| const { FILE_META_TYPE, DEFAULT_PROCESSORS, isFileMeta, resolveExternalContentValue } = ExternalContent | ||
|
|
||
| const DEFAULT_MAX_SIZE_BYTES = 10 * 1024 * 1024 // Default max size: 10MB |
Member
There was a problem hiding this comment.
Why not 100mb as "max size" that could be uploaded to platform?
sitozzz
reviewed
Apr 14, 2026
Comment on lines
+218
to
+222
| if (nextValue === undefined) return undefined | ||
|
|
||
| if (nextValue === null) { | ||
| return null | ||
| } |
Member
There was a problem hiding this comment.
Is that original code of resolveInput hook? Looks a little strange)
Suggested change
| if (nextValue === undefined) return undefined | |
| if (nextValue === null) { | |
| return null | |
| } | |
| if (nextValue === undefined || nextValue === null) return nextValue |
sitozzz
reviewed
Apr 14, 2026
| try { | ||
| await this.adapter.delete(prevValue) | ||
| } catch (err) { | ||
| logger.debug({ msg: 'Failed to delete old file after change', err, data: { filename: prevFilename } }) |
Member
There was a problem hiding this comment.
I think this should be not a debug log. At least info or warning
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
3 participants
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.
Code Review Guide - ExternalContent Field
🎯 Start Here
This PR adds a new
ExternalContentfield type that stores large data in external files instead of the database. It also includes a generic backfill script to migrate any existing inline JSON field to file storage.📍 Review Priority Order
1. Security - Path Traversal Protection⚠️ CRITICAL
File:
packages/keystone/fieldsUtils/ExternalContent/validateFilePath.jsWhat to check:
validateFilePath()(for bothbasePathandfilename)path.isAbsolute(filename)before any joinpath.join()+path.normalize()on both the full path and the base pathnormalized === normalizedBase(resolving to base dir itself)normalized.startsWith(normalizedBase + path.sep)- returnsnormalized, notfullPathWhy it matters: Prevents attackers from reading arbitrary files from the server.
2. Race Condition Fix - DataLoader⚠️ CRITICAL
File:
packages/keystone/fieldsUtils/ExternalContent/FileContentLoader.jsWhat to check:
cache(Map),pending(Map), andqueue(Array)load()checkscachefirst, thenpendingmap before creating a new promisependingimmediately after creation, keyed bycacheKeypromise.finally()moves result frompendingtocacheand cleans uppending_schedulingBatchflag to prevent a second timer from being set between the!this.batchTimercheck and the timer assignmentbatchDelayMs: 0usessetImmediateinstead ofsetTimeoutfor immediate executionWhy it matters: Without
pendingmap, concurrent GraphQL resolver calls for the same file create duplicate queue entries and duplicate reads.3. Backfill Script - Migration Safety⚠️ CRITICAL
File:
apps/rb/bin/backfillExternalContentField.jsWhat to check:
isFileMeta(fieldValue)check skips records that are already migrated (idempotent)null/undefinedraw values are skipped (falsy check is intentional -false/0/''are valid JSON payloads)serverUtils[model].update()which triggersresolveInput— the field's own save logic handles file creation and DB update atomically within Keystone's mutation pipeline--dry-runflag logs what would happen without writing files or updating DB--continue-on-errorflag allows partial runs; failed record IDs are collected and printed at the endskip) — safe because processed records change their shape (become file-meta), so they won't appear in a re-runWhy it matters: Prevents re-migrating already-migrated records and provides a safe recovery path on partial failures.
4. Dual GraphQL Fields -
fieldvsfieldResolvedFile:
packages/keystone/fields/ExternalContent/Implementation.jsWhat to check:
gqlOutputFields()returns two fields:${path}: String(raw file-meta JSON for Admin UI) and${path}Resolved: <format type>(deserialized content for API clients)gqlOutputFieldResolvers()only registers a resolver for${path}Resolved— the raw field is served directly from the DB columnresolveInput()receivescontextand passescontext?.authedItemasusertoadapter.publicUrl()— needed for adapters that sign URLs per-user_type: FILE_META_TYPEandmeta: { format }soresolveExternalContentValuecan deserialize without guessing the format5. Admin UI - Field Component
File:
packages/keystone/fields/ExternalContent/views/Field.jsWhat to check:
isReadOnly = field.isReadOnly !== false— reads directly from the controller instance, not fromfield.controllerinitialEditValuestringify logic anduseEffectboth usetypeof resolvedContent === 'string' ? resolvedContent : JSON.stringify(resolvedContent, null, 2)— consistent handling prevents[object Object]in textareauseEffectdependency is[resolvedContent]only — triggers when async resolved content arrives after initial render6. Input Validation
File:
packages/keystone/fieldsUtils/ExternalContent/createExternalDataField.jsWhat to check:
adapteris required — throws immediately with clear message if missingmaxSizeBytesvalidated as a positive finite numberbatchDelayMsvalidated as a non-negative finite numberisAdminUIReadOnlymapped toadminConfig.isReadOnlyand not leaked into the returned field objecttype: 'ExternalContent'is placed after...otherPropsto prevent the caller from overriding itWhy it matters: Fails fast with clear errors if misconfigured.
Summary by CodeRabbit