feat(comment-replies): add single threading on comments #108

zzacharo · 2025-10-27T10:04:46Z

closes #107

palkerecsenyi

Thanks Zach, this looks great and I agree with the 1-level limit, this makes it much simpler. The API routes are also nicely laid out, I don't think there's any better way of doing this.

palkerecsenyi · 2025-10-27T10:34:16Z

rfcs/rdm-0108-threaded-comments.md

+
+### **Search index design**
+
+Children are indexed inline with their parent for efficient retrieval and display. The search mappings include:


Apologies if I misunderstand: won't this mean that child comments are retrieved twice, once for the search query and once for the systemfield calculation? Or will we filter for only null parent_ids during the search query and then just fill in the children with the systemfield?

During the search query, the children are already indexed. During the systemfield calculation, they are retrieved and then cached locally for subsequent queries. The children actually are not indexed as independent events in the proposed implementation.

palkerecsenyi · 2025-10-27T10:41:53Z

rfcs/rdm-0108-threaded-comments.md

+When a comment is deleted, the system handles threading carefully:
+
+**Deleting a child comment:**
+- The child is converted to a `LogEvent` with "comment deleted" payload


Would it not be clearer maybe if we kept it as a CommentEvent? And we could remove the content, but add a field to the payload_schema marking it as deleted, so we can render some text in the UI, e.g. "This comment was deleted", with some formatting to clearly differentiate it from regular comment content. And maybe also the time at which it was deleted.

GitHub uses a sort of similar approach: palkerecsenyi/discussiontest#1 (comment). Although this is confusing because it just removes the comment content without showing a message to clarify that it was deleted.

palkerecsenyi · 2025-10-27T10:42:27Z

rfcs/rdm-0108-threaded-comments.md

+
+    # Convert to LogEvent
+    event.type = LogEventType
+    event["payload"] = {"event": "comment_deleted", "content": "deleted a comment"}


Does this get i18n-ised when being rendered or is it always in English?

in the code we translate it, I forgot it in the RFC 🙃

kpsherva · 2025-10-28T08:34:37Z

rfcs/rdm-0108-threaded-comments.md

+- Multiple `children` (replies) - but those children **cannot have children** themselves
+- A computed `children` field that includes full metadata of all child events
+
+**Threading depth is limited to exactly 1 level** to keep discussions simple and readable. Attempting to reply to a reply results in a validation error.


Attempting to reply to a reply results in a validation error.

you mean in the REST API, right? UI shouldn't display this option in a first place, maybe worth mentioning

Yes, exactly. This refers to the REST API

kpsherva · 2025-10-28T08:39:44Z

rfcs/rdm-0108-threaded-comments.md

+Threading is implemented via a **dedicated REST API endpoint** rather than accepting `parent_id` in the request body. This design:
+- Makes the API more explicit and RESTful
+- Prevents users from manipulating `parent_id` in request bodies
+- Clearly separates top-level comments from replies
+- Ensures `parent_id` comes from the trusted URL path, not user input


this part is not super clear to me, especially how URL path is more trusted than having parent_id in the payload? I understand it will be validated either way, not sure we need dedicated endpoint. Unless I am missing some context.

I experimented with having one a new param in the payload for parent_id in the same route as creating a comment. In that solution, you needed to ensure that the parent_id passed down was validated to disallow multiple levels of nested replies e.g

if incoming_event.parent_id: event_parent = _get_event(incoming_event.parent_id) if event_parent.parent_id: raise()

The specific reply endpoint just accepts the message in the payload.

I experimented with having one a new param in the payload for parent_id in the same route as creating a comment. In that solution, you needed to ensure that the parent_id passed down was validated to disallow multiple levels of nested replies e.g

if incoming_event.parent_id: event_parent = _get_event(incoming_event.parent_id) if event_parent.parent_id: raise()

The specific reply endpoint just accepts the message in the payload.

Sorry, the validation of the parent_id is always needed. And indeed, is the validation code that ensures the parent_id not the fact that is coming from the URL. Having a dedicated endpoint seems cleaner in the REST API and simplified a bit how the logic of indexing the parent-child events.

kpsherva · 2025-10-28T08:47:26Z

rfcs/rdm-0108-threaded-comments.md

+- The parent is converted to a `LogEvent`
+- All children are recursively converted to `LogEvents`


wouldn't it be less complex if we keep the same CommentEvent but marked as deleted?

This was the case in the past. We will evaluate if it makes sense to change this to actually a CommentEvent or display them differently in the UI

kpsherva · 2025-10-28T08:50:39Z

rfcs/rdm-0108-threaded-comments.md

+**Key insights**:
+
+1. **Separate methods**: `create()` for top-level, `reply()` for threaded - clear separation of concerns
+2. **URL-based parent_id**: Parent comes from URL path, not request body (prevents manipulation)


I am not convinced about "prevents manipulation" part... You can do POST to /comment/234/reply and /comment/254, nothing prevents you to do that, unless I am missing some other detail or misunderstanding the thought behind this.
How are you planning to prevent replies to random comments (especially in request which user shouldn't have access to)?

The permission is being checked on the request level i.e full URL is /requests/<request_id>/comments/<comment_id>/reply. There is no POST action on /requests/<request_id>/comments/<comment_id>, what is your concern?

kpsherva · 2025-10-28T08:57:45Z

rfcs/rdm-0108-threaded-comments.md

+
+## Unresolved questions
+
+1. **Notification strategy**: Should replies trigger different notifications than top-level comments? Should users be notified when someone replies to their comment specifically?


could it be opt in/opt out, like "follow the thread"?

kpsherva · 2025-10-28T08:59:15Z

rfcs/rdm-0108-threaded-comments.md

+
+1. **Notification strategy**: Should replies trigger different notifications than top-level comments? Should users be notified when someone replies to their comment specifically?
+
+2. **Search and filtering**: How should search work across threaded comments? Should searching return top-level comments with matching children highlighted, or individual matching children?


I saw in some solutions both the whole thread and the child comment were highlighted on search, with an effect of narrowing focus from "big picture" to single comment

zzacharo force-pushed the comments-threading-rfc branch 3 times, most recently from 14ed33c to d7b6bd0 Compare October 27, 2025 10:07

zzacharo added this to Sprint Q4/2025 Oct 27, 2025

zzacharo moved this to In review 🔍 in Sprint Q4/2025 Oct 27, 2025

zzacharo moved this from In review 🔍 to In progress in Sprint Q4/2025 Oct 27, 2025

zzacharo removed this from Sprint Q4/2025 Oct 27, 2025

feat(comment-replies): add single threading on comments

fbe0ed4

* closes inveniosoftware#107

zzacharo force-pushed the comments-threading-rfc branch from d7b6bd0 to fbe0ed4 Compare October 27, 2025 10:27

palkerecsenyi reviewed Oct 27, 2025

View reviewed changes

kpsherva reviewed Oct 28, 2025

View reviewed changes

zzacharo force-pushed the comments-threading-rfc branch 2 times, most recently from 340c6ee to 609db6d Compare October 29, 2025 13:57

feat(comment-replies) v2: add single threading on comments

5b7188c

zzacharo force-pushed the comments-threading-rfc branch from 609db6d to 5b7188c Compare October 30, 2025 15:08


		### Search index design

		Children are indexed inline with their parent for efficient retrieval and display. The search mappings include:

		- The parent is converted to a `LogEvent`
		- All children are recursively converted to `LogEvents`


		## Unresolved questions

		1. Notification strategy: Should replies trigger different notifications than top-level comments? Should users be notified when someone replies to their comment specifically?


		1. Notification strategy: Should replies trigger different notifications than top-level comments? Should users be notified when someone replies to their comment specifically?

		2. Search and filtering: How should search work across threaded comments? Should searching return top-level comments with matching children highlighted, or individual matching children?

feat(comment-replies): add single threading on comments #108

Are you sure you want to change the base?

feat(comment-replies): add single threading on comments #108

Uh oh!

Conversation

zzacharo commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

palkerecsenyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpsherva Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zzacharo commented Oct 27, 2025 •

edited

Loading

kpsherva Oct 28, 2025 •

edited

Loading