fix(nextly): a redacted view stays redacted inside a group or a repeater row - #1840
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98099a09bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!view) return undefined; | ||
| // A fresh object per call. A shared one would be written into by the | ||
| // caller-side synchronisation a level down. | ||
| return isPlainObject(container) ? container : {}; |
There was a problem hiding this comment.
When a group or repeater is denied by the pre-default access pass but becomes allowed after a sibling default is applied, this fallback replaces the entire missing container view with {}. As a result, nested function defaults cannot read even children that the final access pass allows and stores; for example, a row containing an allowed label and echo: d => String(d.label) persists label but resolves echo from undefined. Preserve the authorized nested children while continuing to redact denied ones rather than treating every child of the temporarily removed container as hidden.
Useful? React with 👍 / 👎.
| // the field misses the `validation: { required: true }` spelling, and a | ||
| // nullish value check calls a whitespace string or an empty array present: | ||
| // either way the group is stored and the next write rejects it. | ||
| if (isRequired(child) && isEmptyRequiredValue(value)) return true; |
There was a problem hiding this comment.
When an invented group contains a repeater with validation: { required: true } and defaultValue: [], this new isRequired call deletes the entire group because isEmptyRequiredValue([]) returns true. The write validator explicitly treats an empty repeater as a provided list and accepts it unless minRows rejects it, so the document would not be refused on its next write as this helper assumes; the first read now silently discards a valid declared default that was retained before this change. Reuse the validator's complete required-value decision, including its list exception, rather than combining only these two predicates.
AGENTS.md reference: AGENTS.md:L299-L302
Useful? React with 👍 / 👎.
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-mcp
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
1 participant
Two post-merge findings on #1821, both measured, both with a control run that fails against
main.A redacted view stays redacted inside a container
The create path judges the field rules on a deep copy of the request and hands that copy to the function defaults, so a default cannot read a value its writer was not allowed to send.
When the rules denied a whole container, they removed it from the copy. The walk into that container then found no counterpart, read that as no copy having been given at all, and fell back to the caller's own row. A nested function default could therefore read a denied sibling and carry it into a child the caller may write, and the pass that decides what is stored removed only the field it came from, so the copy survived.
A container missing from the view is now read as a container the rules emptied, which is the only way one goes missing.
Measured on a repeater whose
createrule is satisfied by a top-level default, holding a deniedsecretand a sibling defaulting from it:A required nested child is judged by the rule the write validator applies
required: trueon the field andvalidation: { required: true }beside its other rules are both supported spellings. A Single's first read tested only the first, so a group invented for one defaulted child was stored with a required sibling empty, and the next write refused the document that read had just created.isRequiredis exported from the validator and shared rather than restated.Not in here
PRRT_kwDOSYwUJs6h0tbZ, "evaluate each function default only once", is left open on #1821 as a design decision rather than a patch. Three requirements from that review cannot all hold in a pass-based design: a later default seeing an earlier one, no default reading a value the caller may not write, and each default running exactly once. Any two are reachable. Filed asdecision:function-defaults-once-versus-reading-a-defaulted-siblingwith options and a recommendation.PRRT_kwDOSYwUJs6h03xa, user callbacks inside the publish transaction, is the accepted cost of the maintainer's decision that the promote gate runs there. Filed astask:a-transaction-bound-query-path-for-user-callbacks, with measuring the hang as its first step.Verification
pnpm --filter nextly check-typesclean.main's source first, and each fails there.