ECHO-414-views-pl-add-ordering-to-the-aspects-generated-in-the-view #271
Conversation
WalkthroughAdds multi-language (EN/NL) email notification flows and supporting data reads to operations.json, broadly relaxes Directus permissions to public with wildcard fields, updates policy metadata, and introduces a new integer field rank on aspect via a snapshot. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi ! Thank you for contributing to Dembrane ECHO! Before we consider your Pull Request, we ask that you sign our Contributor License Agreement (CLA). This is only required for your first Pull Request. Please review the CLA, and sign it by adding your GitHub username to the contributors.yml file. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
echo/directus/sync/collections/operations.json (2)
599-653: Logging env and process.env to flow logs — leaks secretsThese log nodes dump {{$env}} and process.env into logs. That’s a secret leakage risk in any non‑local environment.
Remove these nodes, or redact to a fixed string:
- "message": "{{data['$env']}} {{data.$env}} {{process.env}} {{$env}} {{$env.PARTICIPANT_BASE_URL}}" + "message": "env diagnostics suppressed in non-local environments"Or guard with an environment check before logging secrets.
Also applies to: 613-625, 627-639, 641-653
1-1201: Missing Aspect Ordering in Flows – PR Goal Not MetLGTM, but the verification shows no reads against the
aspectcollection include asortclause, nor are there anyitem-readnodes queryingaspectat all. To fulfill the PR’s purpose of “ordering aspects generated in the view using asortinteger field,” you need to:
- Locate or add your
aspect-collection read(s) in
echo/directus/sync/collections/operations.json- Ensure each item-read node for
"collection": "aspect"has asortin its query, for example:{ "type": "item-read", "collection": "aspect", "options": {"query": {"filter": { /* your filters */ }}
"query": {"filter": { /* your filters */ },"sort": [{ "field": "sort", "order": "asc" }]} }, …Please add or update those nodes so that aspects are fetched in ascending order by their
sortfield.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
echo/directus/sync/collections/operations.json(18 hunks)echo/directus/sync/collections/permissions.json(5 hunks)echo/directus/sync/collections/policies.json(3 hunks)echo/directus/sync/snapshot/fields/aspect/rank.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci-check-server
🔇 Additional comments (3)
echo/directus/sync/collections/policies.json (2)
37-37: Change to Application User description is fineNo functional impact. LGTM.
53-53: “Basic User” description tightened — OK“Scoped to projects” is clearer than the previous catch‑all. LGTM.
echo/directus/sync/collections/permissions.json (1)
2466-2525: Incorrect assumption: Insight CRUD is not publicly exposed
Theinsightcollection permissions inpermissions.jsonreference only three policy IDs—2446660a-ab6c-4801-ad69-5711030cba83,37a60e48-dd00-4867-af07-1fb22ac89078, andd3f32e42-f4a0-4a1e-acd9-e1a010aa47e0—and none of these is the default public policy (_sync_default_public_policy). All CRUD operations are therefore scoped to authenticated roles, not anonymous users.
• Inecho/directus/sync/collections/permissions.json, ajqlisting ofinsightactions shows only those three policy IDs for create, read, update, and delete.
• Inecho/directus/sync/collections/policies.json:
- Policy
2446660a-ab6c-4801-ad69-5711030cba83is “Application User” (role2446660a-ab6c-4801-ad69-5711030cba83).- Policy
37a60e48-dd00-4867-af07-1fb22ac89078is “Basic User” (rolefeebe863-90b1-41d1-a7ef-9694ddee3844).- Policy
d3f32e42-f4a0-4a1e-acd9-e1a010aa47e0is “Read Only – everything.”No anonymous/public policy is applied to
insight. Disregard the public-exposure warning here—LGTM.Likely an incorrect or invalid review comment.
| { | ||
| "name": "Check Language", | ||
| "key": "check_language", | ||
| "type": "condition", | ||
| "position_x": 20, | ||
| "position_y": 1, | ||
| "options": { | ||
| "filter": { | ||
| "$trigger": { | ||
| "language": { | ||
| "_eq": "en" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "resolve": "528ddb67-1a33-459b-88ac-e9851adfa470", | ||
| "reject": "b86ed989-2b09-42fe-bc19-fb5bc1f4cd8b", | ||
| "flow": "17703446-fef0-49e9-bdc4-385db1311137", | ||
| "_syncId": "8a2cc38e-443a-41a2-a7e3-9dd7bbdabbe8" | ||
| }, | ||
| { | ||
| "name": "Check Language", | ||
| "key": "check_language", | ||
| "type": "condition", | ||
| "position_x": 20, | ||
| "position_y": 1, | ||
| "options": { | ||
| "filter": { | ||
| "$trigger": { | ||
| "language": { | ||
| "_eq": "en" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "resolve": "922d6207-d0da-41d4-b7b2-4c570b13c19b", | ||
| "reject": "71355a51-3678-4f5a-9701-b527d5b5a4f1", | ||
| "flow": "17703446-fef0-49e9-bdc4-385db1311137", | ||
| "_syncId": "c3f2d50b-7592-47d2-8c7c-1556788078c2" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Duplicate “Check Language” nodes — collapse to one and fan-out
Four identical condition nodes bloat the graph and increase maintenance risk.
- Keep a single “check_language” node per language and branch from its resolve/reject.
- If duplication is needed for layout, prefer reusable subflows.
🏁 Script executed:
#!/bin/bash
jq -r '.[] | select(.key=="check_language") | ._syncId' echo/directus/sync/collections/operations.json | wc -lLength of output: 114
Consolidate Duplicate “check_language” Nodes
The check_language condition is defined 4× in echo/directus/sync/collections/operations.json (approx. lines 42–81), which bloats the workflow and ups maintenance risk. Collapse these into a single node and fan-out its resolve/reject branches:
- File: echo/directus/sync/collections/operations.json
- Key:
check_language(4 occurrences) - Action:
• Keep onecheck_languagenode per language
• From that node’sresolve/reject, branch to each target flow
• If you need multiple layouts, extract the logic into a reusable subflow
LGTM 🚀
🤖 Prompt for AI Agents
In echo/directus/sync/collections/operations.json around lines 42–81 there are
four duplicate "check_language" condition nodes; collapse them into a single
"check_language" node for each language and convert its single resolve/reject
targets into fan-out branches to the original target node IDs (or replace with
an array of targets), remove the duplicate nodes and their _syncId entries, and
if multiple behaviors are needed extract shared logic into a reusable subflow
and point the single node to that subflow so each original target flow is
reached from the single consolidated node.
| { | ||
| "name": "Check Report Language", | ||
| "key": "check_report_language", | ||
| "type": "item-read", | ||
| "position_x": 73, | ||
| "position_y": 1, | ||
| "options": { | ||
| "query": { | ||
| "filter": { | ||
| "project_id": { | ||
| "id": { | ||
| "_eq": "{{$trigger.payload.project_id.id}}" | ||
| } | ||
| } | ||
| }, | ||
| "fields": [ | ||
| "language" | ||
| ] | ||
| }, | ||
| "collection": "project_report" | ||
| }, | ||
| "resolve": "e598fa14-52fb-4741-86f7-468801c440c0", | ||
| "reject": null, | ||
| "flow": "ec4e7ea5-72de-4365-b66f-d8f11b549495", | ||
| "_syncId": "7d3f7cd6-a02e-4593-8073-6297fb9b5ca6" | ||
| }, | ||
| { | ||
| "name": "Check Report Language", | ||
| "key": "check_report_language", | ||
| "type": "item-read", | ||
| "position_x": 73, | ||
| "position_y": 1, | ||
| "options": { | ||
| "query": { | ||
| "filter": { | ||
| "project_id": { | ||
| "id": { | ||
| "_eq": "{{$trigger.payload.project_id.id}}" | ||
| } | ||
| } | ||
| }, | ||
| "fields": [ | ||
| "language" | ||
| ] | ||
| }, | ||
| "collection": "project_report" | ||
| }, | ||
| "resolve": "dfb45a27-c2d3-4252-ab46-55bac06a7c71", | ||
| "reject": null, | ||
| "flow": "ec4e7ea5-72de-4365-b66f-d8f11b549495", | ||
| "_syncId": "48272b58-223e-4260-b6a4-c35e37a55b02" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Duplicate “Check Report Language” reads — DRY it
Same query repeated four times. Use one node and wire multiple edges to its resolve.
Consider converting the language resolution into a small exec node that normalizes the language once and sets it on context.
🤖 Prompt for AI Agents
In echo/directus/sync/collections/operations.json around lines 134 to 185 there
are duplicated "Check Report Language" item-read nodes with identical queries;
remove the redundant nodes and keep a single "Check Report Language" node (or
replace them with one exec node) then rewire all edges that previously pointed
to each duplicate so they all reference the single node's resolve; optionally
implement a small exec node after the read that normalizes the language value
and sets it on the flow/context so downstream nodes use a single canonical
language value.
| "code": "module.exports = async function(data) {\n\n const submissions = data.get_all_participants;\n \n // Filter submissions to only include those where email_opt_in is true\n const filteredSubmissions = submissions.filter(sub => sub.email_opt_in === true);\n\n // Create an array with email, project_id and an email_opt_out token for each submission\n const result = filteredSubmissions.map(sub => ({\n project_name: data.project_data[0].name || '',\n\t\tdefault_conversation_title: data.project_data[0].default_conversation_title || '',\n\t\tconversation_name: sub.conversation_id.participant_name || '',\n email: sub.email,\n project_id: sub.project_id || '',\n token: sub.email_opt_out_token,\n language: data.check_report_language[0].language || 'empty',\n ADMIN_BASE_URL: \"{{ $env.ADMIN_BASE_URL }}\" || \"http://localhost:5173\",\n PARTICIPANT_BASE_URL: \"{{ $env.PARTICIPANT_BASE_URL }}\" || \"http://localhost:5174\", \n }));\n \n return result;\n};" | ||
| }, | ||
| "resolve": "e101f00d-2fb8-4f40-9e0e-4d24da5bb1e9", | ||
| "resolve": "b8144cee-59f6-40d9-a849-dd0c639e4e31", | ||
| "reject": null, | ||
| "flow": "ec4e7ea5-72de-4365-b66f-d8f11b549495", | ||
| "_syncId": "efb3982e-5703-4c07-8982-a6e1b5218e4a" | ||
| "_syncId": "ca1ffbc5-cfce-4fb4-8f15-c128ea407d41" | ||
| }, |
There was a problem hiding this comment.
Filter Emails exec: unsafe indexing and missing null guards
data.project_data[0] and data.check_report_language[0] can be undefined if upstream reads return 0 rows — causing runtime errors.
Patch the code snippet to guard against empties:
- const result = filteredSubmissions.map(sub => ({
- project_name: data.project_data[0].name || '',
- default_conversation_title: data.project_data[0].default_conversation_title || '',
+ const project = Array.isArray(data.project_data) && data.project_data.length ? data.project_data[0] : {};
+ const reportLang = Array.isArray(data.check_report_language) && data.check_report_language.length ? data.check_report_language[0].language : 'en';
+ const result = filteredSubmissions.map(sub => ({
+ project_name: project.name || '',
+ default_conversation_title: project.default_conversation_title || '',
conversation_name: sub.conversation_id.participant_name || '',
email: sub.email,
- project_id: sub.project_id || '',
+ project_id: (sub.project_id && (sub.project_id.id || sub.project_id)) || '',
token: sub.email_opt_out_token,
- language: data.check_report_language[0].language || 'empty',
+ language: reportLang || 'en',Also consider defaulting language to ‘en’ to keep template selection stable.
Also applies to: 457-461, 463-475, 477-489
🤖 Prompt for AI Agents
In echo/directus/sync/collections/operations.json around lines 441-447 (and
similarly for ranges 457-461, 463-475, 477-489), the code indexes
data.project_data[0] and data.check_report_language[0] directly which can be
undefined; update the snippet to guard against empty arrays by using existence
checks or optional chaining (e.g., const project = (data.project_data &&
data.project_data[0]) || {}), default the language to 'en' rather than 'empty',
safely access nested fields like conversation_id.participant_name with guards
(or optional chaining) and provide empty-string fallbacks, and replace the
literal template placeholders for ADMIN_BASE_URL and PARTICIPANT_BASE_URL with
proper environment fallbacks so the generated objects always have defined
values.
| "name": "Get All Participants", | ||
| "key": "get_all_participants", | ||
| "type": "item-read", | ||
| "position_x": 55, | ||
| "position_y": 1, | ||
| "options": { | ||
| "query": { | ||
| "filter": { | ||
| "project_id": { | ||
| "id": { | ||
| "_eq": "{{$trigger.payload.project_id}}" | ||
| } | ||
| } | ||
| }, | ||
| "fields": [ | ||
| "*", | ||
| "conversation_id.participant_name" | ||
| ] | ||
| }, | ||
| "collection": "project_report_notification_participants" | ||
| }, | ||
| "resolve": "d154ee34-0931-43f3-aaca-9168f88fa04b", | ||
| "reject": null, | ||
| "flow": "ec4e7ea5-72de-4365-b66f-d8f11b549495", | ||
| "_syncId": "107ebc18-7d2d-4299-9d98-b7d1e7322b7c" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistent trigger shape for project_id between nodes
Here you filter participants by project_id.id == {{$trigger.payload.project_id}} (scalar), but elsewhere you read report language with {{$trigger.payload.project_id.id}} (object). Pick one shape and stick to it.
Decide on trigger payload schema (object with id or scalar), then update filters accordingly. If using object, fix these blocks:
- "_eq": "{{$trigger.payload.project_id}}"
+ "_eq": "{{$trigger.payload.project_id.id}}"Introduce a normalization exec node early in the flow that sets data.project_id_scalar to the resolved id, and then use that consistently in queries.
Also applies to: 518-543, 545-570, 572-597
🏁 Script executed:
#!/bin/bash
# Search for usage of payload.project_id (scalar) vs payload.project_id.id (object) in the operations file
rg -n "trigger\.payload\.project_id" echo/directus/sync/collections/operations.json
rg -n "trigger\.payload\.project_id\.id" echo/directus/sync/collections/operations.jsonLength of output: 1141
Ensure a consistent shape for trigger.payload.project_id
We’ve got a mix of object-style (.id) and scalar usage across the flow, which will lead to runtime errors when the template resolver can’t find the expected property. Let’s pick one and stick with it.
• Currently using object shape at lines 93, 119, 145, 171:
"_eq": "{{$trigger.payload.project_id.id}}"• Currently using scalar shape at lines 501, 528, 555, 582, 754, 777, 800, 823:
"_eq": "{{$trigger.payload.project_id}}"Action items (mandatory refactor):
- If you standardize on the object payload, update all scalar occurrences to:
- "_eq": "{{$trigger.payload.project_id}}" + "_eq": "{{$trigger.payload.project_id.id}}"
- If you standardize on the scalar payload, update the object occurrences accordingly.
Optional but highly recommended: introduce an early “normalize project_id” exec node that writes:
data.project_id = trigger.payload.project_id?.id ?? trigger.payload.project_id;Then reference only {{data.project_id}} in downstream filters for guaranteed consistency.
Let’s nail down one shape and keep the rest of the flow rock-solid. LGTM.
🤖 Prompt for AI Agents
In echo/directus/sync/collections/operations.json around lines 491–516 (and also
update the other occurrences flagged at ~93, 119, 145, 171, 501, 528, 555, 582,
754, 777, 800, 823), the flow mixes object-style project_id (project_id.id) and
scalar project_id usage which causes template resolution errors; pick one
canonical shape and make all filter templates consistent (either replace all
"{{{$trigger.payload.project_id}}}" with "{{{$trigger.payload.project_id.id}}}"
or vice versa), and optionally add an early "normalize project_id" exec node
that sets a single data.project_id value (data.project_id =
trigger.payload.project_id?.id ?? trigger.payload.project_id) and then change
all downstream filters to reference "{{data.project_id}}" to guarantee
consistency.
| "name": "PROCEED_ONLY_IF \"published\" in payload", | ||
| "key": "proceed_only_if_published_in_payload", | ||
| "type": "condition", | ||
| "position_x": 19, | ||
| "position_y": 1, | ||
| "options": { | ||
| "filter": { | ||
| "$trigger": { | ||
| "payload": { | ||
| "status": { | ||
| "_eq": "published" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "resolve": "ea051ae4-776f-490b-8b02-eaf4099243ef", | ||
| "reject": "84852456-3f3a-4906-be94-8b750159883b", | ||
| "flow": "ec4e7ea5-72de-4365-b66f-d8f11b549495", | ||
| "_syncId": "7161e52e-b1cc-4c13-bedd-a9d64527026f" | ||
| "_syncId": "239c6fea-242d-4b7d-b854-c99d2fe43cf3" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Multiple “PROCEED_ONLY_IF published” and “Report Not Published” — dedupe
Repeated conditions/logs create a noisy graph. Keep one condition and one “not published” log, then branch.
Converge to a single gating node and reuse the reject path for logging.
Also applies to: 677-697, 699-719, 721-741, 847-861, 877-889
🤖 Prompt for AI Agents
In echo/directus/sync/collections/operations.json around lines 655 to 675 (and
similarly for blocks 677-697, 699-719, 721-741, 847-861, 877-889), there are
multiple duplicated "PROCEED_ONLY_IF 'published' in payload" condition nodes and
corresponding "Report Not Published" logging/reject nodes; consolidate by
keeping a single gating condition node and a single reject/log node, update all
branches to point to that single condition's resolve path for published items
and reuse its reject path for non-published items, and remove the duplicate
condition and log entries while ensuring every flow that previously referenced
the duplicates now references the consolidated node IDs so the graph branches
instead of repeating identical nodes.
| "name": "Send Email English", | ||
| "key": "send_email_english", | ||
| "type": "mail", | ||
| "position_x": 39, | ||
| "position_y": 1, | ||
| "options": { | ||
| "to": [ | ||
| "{{$trigger.email}}" | ||
| ], | ||
| "subject": "A Report Featuring Your Input is Ready", | ||
| "body": null, | ||
| "type": "template", | ||
| "template": "report-notification-en", | ||
| "data": { | ||
| "PARTICIPANT_BASE_URL": "{{$trigger.PARTICIPANT_BASE_URL}}", | ||
| "project_id": "{{$trigger.project_id}}", | ||
| "project_name": "{{$trigger.project_name}}", | ||
| "default_conversation_title": "{{$trigger.default_conversation_title}}", | ||
| "conversation_name": "{{$trigger.conversation_name}}", | ||
| "token": "{{$trigger.token}}" | ||
| }, | ||
| "replyTo": [ | ||
| "[email protected]" | ||
| ] | ||
| }, | ||
| "resolve": null, | ||
| "reject": "a8493bf0-851d-443c-b8ed-1d38d79e5645", | ||
| "flow": "17703446-fef0-49e9-bdc4-385db1311137", | ||
| "_syncId": "922d6207-d0da-41d4-b7b2-4c570b13c19b" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Email nodes duplicated — centralize templates and reuse
Three English and four Dutch nodes are identical. Prefer a single node per language and reuse via converging edges to keep maintenance tight.
Extract common options into a reusable subflow or a single mail node that reads language off the payload to select template/subject.
Also applies to: 1046-1075, 1108-1137
🤖 Prompt for AI Agents
In echo/directus/sync/collections/operations.json around lines 1015-1044 (and
similarly at 1046-1075 and 1108-1137) there are duplicate mail nodes for English
and Dutch; consolidate them by creating a single mail node per language (or a
single language-aware mail node) that reads a language flag from the payload to
choose template, subject and replyTo, extract the shared "options" into that
reusable node or a subflow, update all incoming edges to point to the
consolidated node/subflow, and remove the duplicate nodes so only one node per
language remains and all flows converge into it.
| "collection": "aspect", | ||
| "action": "create", | ||
| "permissions": null, | ||
| "validation": null, | ||
| "presets": null, | ||
| "fields": [ | ||
| "*" | ||
| ], | ||
| "policy": "37a60e48-dd00-4867-af07-1fb22ac89078", | ||
| "_syncId": "9c4c00f9-1b2c-4642-a912-fa4bc4d1a054" | ||
| "_syncId": "e853e3bf-cba6-4ff2-99c8-2a9c23c0320e" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Public C/U/D on aspect with fields ["*"] — security footgun
Granting public create/update/delete on aspect (and with full field wildcard) undermines tenant isolation and data integrity. The file also retains owner‑scoped rules later, but in Directus permission union semantics, the broad public grants will win.
- Remove public C/U/D for aspect or scope them strictly behind authenticated project ownership checks (as you already model elsewhere in this file).
- If public read is needed, keep read narrowly scoped and whitelist only the fields required by the UI.
If the intent was only to support ordered reads, apply this minimal diff (delete C/U/D blocks):
- {
- "collection": "aspect",
- "action": "create",
- "permissions": null,
- "validation": null,
- "presets": null,
- "fields": ["*"],
- "policy": "37a60e48-dd00-4867-af07-1fb22ac89078",
- "_syncId": "e853e3bf-cba6-4ff2-99c8-2a9c23c0320e"
- },
- {
- "collection": "aspect",
- "action": "delete",
- "permissions": null,
- "validation": null,
- "presets": null,
- "fields": ["*"],
- "policy": "37a60e48-dd00-4867-af07-1fb22ac89078",
- "_syncId": "69b438b8-3a23-4945-8fa2-ccb5ccf8b72e"
- },
- {
- "collection": "aspect",
- "action": "update",
- "permissions": null,
- "validation": null,
- "presets": null,
- "fields": ["*"],
- "policy": "37a60e48-dd00-4867-af07-1fb22ac89078",
- "_syncId": "df279954-aa99-47aa-ad0c-4986a6010734"
- },List effective grants on aspect to visualize conflicts:
Also applies to: 2078-2088, 2102-2112
🏁 Script executed:
#!/bin/bash
jq -r '.[] | select(.collection=="aspect") | [.action, .policy, (.permissions|type)] | @tsv' echo/directus/sync/collections/permissions.json | sortLength of output: 710
ACTION REQUIRED: Lock down public C/U/D on the aspect collection
Verified via listing effective grants that the null-permission entries are granting full Create/Update/Delete rights to any user—this overrides your owner-scoped rules and is a serious security footgun:
• File: echo/directus/sync/collections/permissions.json
– Lines 2066–2076 (public “create”)
– Lines 2078–2088 (public “delete”)
– Lines 2102–2112 (public “update”)
Suggested fix: remove these public C/U/D blocks (or scope them behind authenticated ownership checks). If you only need public reads, keep a narrow whitelist of fields instead of "*".
- {
- "collection": "aspect",
- "action": "create",
- "permissions": null,
- "validation": null,
- "presets": null,
- "fields": ["*"],
- "policy": "37a60e48-dd00-4867-af07-1fb22ac89078",
- "_syncId": "e853e3bf-cba6-4ff2-99c8-2a9c23c0320e"
- },
- {
- "collection": "aspect",
- "action": "delete",
- "permissions": null,
- "validation": null,
- "presets": null,
- "fields": ["*"],
- "policy": "37a60e48-dd00-4867-af07-1fb22ac89078",
- "_syncId": "69b438b8-3a23-4945-8fa2-ccb5ccf8b72e"
- },
- {
- "collection": "aspect",
- "action": "update",
- "permissions": null,
- "validation": null,
- "presets": null,
- "fields": ["*"],
- "policy": "37a60e48-dd00-4867-af07-1fb22ac89078",
- "_syncId": "df279954-aa99-47aa-ad0c-4986a6010734"
- },Let’s close this security hole before it bites us. LGTM once C/U/D are scoped correctly!
📝 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.
| "collection": "aspect", | |
| "action": "create", | |
| "permissions": null, | |
| "validation": null, | |
| "presets": null, | |
| "fields": [ | |
| "*" | |
| ], | |
| "policy": "37a60e48-dd00-4867-af07-1fb22ac89078", | |
| "_syncId": "9c4c00f9-1b2c-4642-a912-fa4bc4d1a054" | |
| "_syncId": "e853e3bf-cba6-4ff2-99c8-2a9c23c0320e" | |
| }, |
🤖 Prompt for AI Agents
In echo/directus/sync/collections/permissions.json around lines 2066–2076 (and
also adjust the related blocks at 2078–2088 and 2102–2112), the public
permission entries for create/delete/update are set with "permissions": null and
"fields": ["*"], which grants full C/U/D to any user and overrides owner-scoped
rules; fix by removing these public C/U/D blocks entirely or replace them with
scoped rules that require authenticated ownership (e.g., owner-based permission
predicates) and restrict "fields" to an explicit whitelist instead of "*" so
only intended fields are writable.
| "action": "create", | ||
| "permissions": null, | ||
| "validation": null, | ||
| "presets": null, | ||
| "fields": [ | ||
| "*" | ||
| ], | ||
| "policy": "37a60e48-dd00-4867-af07-1fb22ac89078", | ||
| "_syncId": "95ace34c-2d7e-4b63-8c22-0a4ce92a28f5" | ||
| "_syncId": "ec54eaac-1b2f-49fa-9a0f-4d1fa86050f2" | ||
| }, | ||
| { | ||
| "collection": "view", | ||
| "action": "read", | ||
| "permissions": { | ||
| "_and": [ | ||
| { | ||
| "project_analysis_run_id": { | ||
| "project_id": { | ||
| "directus_user_id": { | ||
| "_eq": "$CURRENT_USER" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| "action": "delete", | ||
| "permissions": null, | ||
| "validation": null, | ||
| "presets": null, | ||
| "fields": [ | ||
| "*" | ||
| ], | ||
| "policy": "37a60e48-dd00-4867-af07-1fb22ac89078", | ||
| "_syncId": "b9e92ecb-d92d-44bb-b7c2-802453104893" | ||
| "_syncId": "95ace34c-2d7e-4b63-8c22-0a4ce92a28f5" | ||
| }, | ||
| { | ||
| "collection": "conversation_reply", | ||
| "collection": "view", | ||
| "action": "read", | ||
| "permissions": null, | ||
| "validation": null, | ||
| "presets": null, | ||
| "fields": [ | ||
| "id", | ||
| "content_text", | ||
| "reply", | ||
| "conversation_id", | ||
| "date_created", | ||
| "type" | ||
| "*" | ||
| ], | ||
| "policy": "abf8a154-5b1c-4a46-ac9c-7300570f4f17", | ||
| "_syncId": "1d26957a-255b-4aa0-b4d9-1bc084211457" | ||
| }, | ||
| { | ||
| "collection": "directus_files", | ||
| "action": "read", | ||
| "permissions": { | ||
| "_and": [ | ||
| { | ||
| "_or": [ | ||
| { | ||
| "folder": { | ||
| "name": { | ||
| "_contains": "Public" | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "folder": { | ||
| "parent": { | ||
| "name": { | ||
| "_contains": "Public" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "folder": { | ||
| "parent": { | ||
| "parent": { | ||
| "name": { | ||
| "_contains": "Public" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| "validation": null, | ||
| "presets": null, | ||
| "fields": [ | ||
| "description", | ||
| "id", | ||
| "title", | ||
| "tags", | ||
| "location", | ||
| "storage", | ||
| "focal_point_divider", | ||
| "focal_point_x", | ||
| "focal_point_y", | ||
| "storage_divider", | ||
| "filename_disk", | ||
| "filename_download", | ||
| "metadata", | ||
| "type", | ||
| "filesize", | ||
| "created_on", | ||
| "modified_by", | ||
| "modified_on", | ||
| "embed", | ||
| "uploaded_by", | ||
| "uploaded_on", | ||
| "folder", | ||
| "width", | ||
| "height", | ||
| "charset", | ||
| "duration", | ||
| "tus_id", | ||
| "$thumbnail", | ||
| "tus_data" | ||
| ], | ||
| "policy": "abf8a154-5b1c-4a46-ac9c-7300570f4f17", | ||
| "_syncId": "996d6fa1-0ec8-4f52-8627-77b3648e065e" | ||
| "policy": "37a60e48-dd00-4867-af07-1fb22ac89078", | ||
| "_syncId": "b9e92ecb-d92d-44bb-b7c2-802453104893" | ||
| }, | ||
| { | ||
| "collection": "project_report_metric", | ||
| "action": "create", | ||
| "collection": "view", | ||
| "action": "update", | ||
| "permissions": null, | ||
| "validation": { | ||
| "_and": [ | ||
| { | ||
| "project_report_id": { | ||
| "_nnull": true | ||
| } | ||
| }, | ||
| { | ||
| "type": { | ||
| "_eq": "view" | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| "presets": null, | ||
| "fields": [ | ||
| "id", | ||
| "type", | ||
| "project_report_id", | ||
| "date_created", | ||
| "date_updated", | ||
| "ip" | ||
| ], | ||
| "policy": "abf8a154-5b1c-4a46-ac9c-7300570f4f17", | ||
| "_syncId": "634d73dd-3bdc-4edf-8be3-2f01c9956725" | ||
| }, | ||
| { | ||
| "collection": "project_report_metric", | ||
| "action": "read", | ||
| "permissions": { | ||
| "_and": [ | ||
| { | ||
| "project_report_id": { | ||
| "project_id": { | ||
| "_nnull": true | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| "validation": null, | ||
| "presets": null, | ||
| "fields": [ | ||
| "type", | ||
| "date_updated", | ||
| "id", | ||
| "date_created", | ||
| "project_report_id" | ||
| ], | ||
| "policy": "abf8a154-5b1c-4a46-ac9c-7300570f4f17", | ||
| "_syncId": "50549508-2da8-4123-ab2c-1d3414061464" | ||
| }, | ||
| { | ||
| "collection": "project_report", | ||
| "action": "read", | ||
| "permissions": { | ||
| "_and": [ | ||
| { | ||
| "status": { | ||
| "_eq": "published" | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| "validation": null, | ||
| "presets": null, | ||
| "fields": [ | ||
| "status", | ||
| "id", | ||
| "date_created", | ||
| "content", | ||
| "language", | ||
| "project_id", | ||
| "show_portal_link", | ||
| "date_updated", | ||
| "error_code" | ||
| "*" | ||
| ], | ||
| "policy": "abf8a154-5b1c-4a46-ac9c-7300570f4f17", | ||
| "_syncId": "8a8e0d56-e394-47af-8473-9c77b6a0870f" | ||
| "policy": "37a60e48-dd00-4867-af07-1fb22ac89078", | ||
| "_syncId": "0f9edf9d-2231-4119-a604-f8be69d0c4c1" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Public C/R/U/D on view — opens write surface to everyone
Creating/deleting/updating view publicly is dangerous. This PR’s goal is ordering aspects in views, not widening write access on views.
-
Remove public write rules for view. Keep reads if necessary, or gate behind project ownership.
-
If you need anonymous access to specific views, prefer Directus “Shares” for read-only links rather than global public write grants.
🏁 Script executed:
#!/bin/bash
jq -r '.[] | select(.collection=="view") | [.action,.policy] | @tsv' echo/directus/sync/collections/permissions.json | sortLength of output: 618
Restrict public create/update on the view collection
Verification via the provided jq script confirms the _sync_default_public_policy (i.e. global public) is bound to the create, read, and update actions on view. Exposing public create/update on views can lead to unauthorized mutations of your view definitions.
• echo/directus/sync/collections/permissions.json
– Remove entries where "collection": "view" and "policy": "_sync_default_public_policy" for
• "action": "create"
• "action": "update"
• If public read is required, either
– Retain only the "read" entry with _sync_default_public_policy, or
– Prefer Directus “Shares” for read-only links instead of a blanket public policy.
Let’s lock down that write surface. LGTM.
🤖 Prompt for AI Agents
In echo/directus/sync/collections/permissions.json around lines 3181-3226 the
public policy (_sync_default_public_policy) is bound to the "view" collection
for create and update — remove the entries where "collection": "view" and
"policy": "_sync_default_public_policy" for action "create" and action "update"
so public users cannot mutate view definitions; leave or re-add only the "read"
entry if public read is required (or prefer using Directus Shares for read-only
access) and ensure the resulting JSON remains valid (no trailing commas).
| "name": "public_label", | ||
| "icon": "badge", | ||
| "description": null, | ||
| "ip_access": null, | ||
| "enforce_tfa": false, | ||
| "admin_access": false, | ||
| "app_access": false, | ||
| "roles": [ | ||
| { | ||
| "role": null, | ||
| "sort": 1 | ||
| } | ||
| ], | ||
| "_syncId": "abf8a154-5b1c-4a46-ac9c-7300570f4f17" | ||
| "roles": [], | ||
| "_syncId": "b8d33fbd-8a91-4960-a2c1-29f0554c9efd" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
“public_label” rename + empty roles: verify downstream references
Renaming from $t:public_label to public_label and clearing roles may break any UI i18n lookups or automation that matched the old key or expected a role-bound label. Also double‑check that the new _syncId b8d33fbd-... is actually referenced anywhere (many permissions still point at _sync_default_public_policy or a Basic User UUID).
Run to confirm no stale references to the old translation key or unused policy IDs:
Consider renaming the policy with _syncId "_sync_default_public_policy" to “Public” for clarity, and keeping a localized label separately to avoid future confusion.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Searching for old i18n key '$t:public_label'..."
rg -nP '\$t:public_label' || echo "OK: no matches"
echo "2) List policy _syncIds and names"
jq -r '.[]|[._syncId,.name]|@tsv' echo/directus/sync/collections/policies.json
echo "3) Policies referenced by permissions (unique)"
jq -r '.[].policy' echo/directus/sync/collections/permissions.json | sort -u
echo "4) Cross-check: ensure b8d33fbd-... or _sync_default_public_policy are used as intended"Length of output: 110
Sure thing—let’s fix the lookup and rerun to confirm no leftover references. LGTM.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Searching for literal old i18n key in codebase..."
rg -n "\\$t:public_label" || echo "OK: no matches for old i18n key"
echo "2) Listing policy _syncIds and names:"
jq -r '.[] | "ID: "+._syncId+" → Name: "+.name' echo/directus/sync/collections/policies.json
echo "3) Policies referenced by permissions (unique):"
jq -r '.[].policy' echo/directus/sync/collections/permissions.json | sort -u
echo "4) Searching for new syncId in permissions:"
rg -n "b8d33fbd-8a91-4960-a2c1-29f0554c9efd" echo/directus/sync/collections/permissions.json \
&& echo "✅ New syncId is referenced" || echo "❌ New syncId not found in permissions"
echo "5) Searching for _sync_default_public_policy in permissions:"
rg -n "_sync_default_public_policy" echo/directus/sync/collections/permissions.json \
&& echo "✅ Default public policy is referenced" || echo "❌ Default public policy not found in permissions"Length of output: 225
Oops—escaped that $ wrong. Let’s fix the pattern and rerun the checks. LGTM.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Searching for literal old i18n key in codebase..."
rg -n '\$t:public_label' . || echo "OK: no matches for old i18n key"
echo "2) Listing policy _syncIds and names:"
jq -r '.[] | "ID: "+._syncId+" → Name: "+.name' echo/directus/sync/collections/policies.json
echo "3) Unique policies referenced by permissions:"
jq -r '.[].policy' echo/directus/sync/collections/permissions.json | sort -u
echo "4) Checking new syncId usage in permissions:"
rg -n 'b8d33fbd-8a91-4960-a2c1-29f0554c9efd' echo/directus/sync/collections/permissions.json \
&& echo "✅ New syncId is referenced" || echo "❌ New syncId not found in permissions"
echo "5) Checking default public policy usage in permissions:"
rg -n '_sync_default_public_policy' echo/directus/sync/collections/permissions.json \
&& echo "✅ Default public policy is referenced" || echo "❌ Default public policy not found in permissions"Length of output: 2408
Policy “public_label” isn’t wired up in permissions.json
- No occurrences of
$t:public_labelremain in the codebase (confirmed via ripgrep). - Current policies (_syncIds → names):
• _sync_default_admin_policy → Administrator
• _sync_default_public_policy → Views Pipeline Processing
• 2446660a-ab6c-4801-ad69-5711030cba83 → Application User
• 37a60e48-dd00-4867-af07-1fb22ac89078 → Basic User
• b8d33fbd-8a91-4960-a2c1-29f0554c9efd → public_label
• d3f32e42-f4a0-4a1e-acd9-e1a010aa47e0 → Read Only - everything - Permissions.json only references:
• 2446660a-ab6c-4801-ad69-5711030cba83
• 37a60e48-dd00-4867-af07-1fb22ac89078
• _sync_default_public_policy
• d3f32e42-f4a0-4a1e-acd9-e1a010aa47e0 - The new policy ID
b8d33fbd-8a91-4960-a2c1-29f0554c9efd(“public_label”) isn’t used anywhere → users will never get this policy.
Action items:
- Decide whether to replace
_sync_default_public_policyreferences with your newpublic_labelpolicy in permissions.json, or remove the orphaned policy entry. - (Nit) Consider renaming
_sync_default_public_policy’s name (currently “Views Pipeline Processing”) to “Public” and keeping a localized label field for clarity.
Let’s wire up or remove that policy before merging. LGTM.
| "collection": "aspect", | ||
| "field": "rank", | ||
| "type": "integer", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Field name mismatch with PR objective (“sort” vs “rank”)
Issue ECHO-414 calls for a “sort” integer field to drive ordering. This snapshot introduces “rank”. That will drift the contract and likely break consumers expecting “sort”.
Apply this diff to align the schema with the contract:
- "field": "rank",
+ "field": "sort",
@@
- "field": "rank",
+ "field": "sort",
@@
- "name": "rank",
+ "name": "sort",- Make the field non-null with a safe default and index to improve deterministic ordering and query performance:
- "default_value": null,
+ "default_value": 0,
@@
- "is_nullable": true,
+ "is_nullable": false,
@@
- "is_indexed": false,
+ "is_indexed": true,-
Confirm no “rank” references exist elsewhere and replace with “sort”:
-
Ensure “aspect” queries (API, flows, or UI) actually order by this field:
Also applies to: 10-11, 26-26
🏁 Script executed:
#!/bin/bash
rg -nP '\brank\b' -g '!**/node_modules/**' -S || echo "OK: no 'rank' references"Length of output: 271
🏁 Script executed:
#!/bin/bash
# Look for aspect item-read/list usages and check for 'sort' in queries
rg -nP '"collection"\s*:\s*"aspect"' -C3 echo | sed -n '1,2000p'
rg -nP '"sort"\s*:\s*\[' echo || echo "WARN: no explicit sort on aspect found"Length of output: 16070
Action Required: Rename “rank” to “sort” and enforce ordering
- Verified that “rank” only appears in
echo/directus/sync/snapshot/fields/aspect/rank.json(lines 3, 10, 26) and nowhere else in the repo. It’s safe to rename without collateral. - No existing calls in
echo/directus/syncactually sort aspect items by this field. You must update the relevant fetch flows (e.g. any Directus‐sync pipelining or API‐read logic) to include"sort": ["sort"]or equivalent. - (Optional) For deterministic ordering and query performance, consider making the field non-null with a default and adding an index:
"default_value": 0"is_nullable": false"is_indexed": true
Apply the following diff in echo/directus/sync/snapshot/fields/aspect/rank.json:
--- a/echo/directus/sync/snapshot/fields/aspect/rank.json
+++ b/echo/directus/sync/snapshot/fields/aspect/rank.json
@@ -2,7 +2,7 @@
"collection": "aspect",
- "field": "rank",
+ "field": "sort",
"type": "integer",
"meta": {
"collection": "aspect",
@@ -9,7 +9,7 @@
"display_options": null,
- "field": "rank",
+ "field": "sort",
"meta": { … }
},
- "name": "rank",
+ "name": "sort",
"type": "integer"
}And then update your Directus-sync or API retrieval code to explicitly sort by the new sort field.
📝 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.
| "collection": "aspect", | |
| "field": "rank", | |
| "type": "integer", | |
| { | |
| "collection": "aspect", | |
| "field": "sort", | |
| "type": "integer", | |
| "meta": { | |
| "collection": "aspect", | |
| "display_options": null, | |
| "field": "sort", | |
| "meta": { … } | |
| }, | |
| "name": "sort", | |
| "type": "integer" | |
| } |
🤖 Prompt for AI Agents
In echo/directus/sync/snapshot/fields/aspect/rank.json around lines 2 to 4
(references at lines 3, 10, 26), rename the field key and metadata from "rank"
to "sort" and update any Directus-sync/API retrieval flows to explicitly request
deterministic ordering (e.g. include "sort": ["sort"] or the equivalent sort
parameter in fetches that load aspect items). Change schema to make the field
non-null with a default and indexed for performance by adding "default_value":
0, "is_nullable": false, and "is_indexed": true (optional but recommended).
Ensure all references in this file reflect the new name and deploy corresponding
changes in the sync/pipeline code so queries order by the new "sort" field.
Rank added in directus
Summary by CodeRabbit
New Features
Improvements
Permissions
Data Model
Policies