Blake1/bac 45 comment thread reply notification service support

[cowlicks]

No description provided.

[linear]

BAC-45 Comment Thread Reply Notification Service Support

[whutchinson98]

The backend side looks good to me.

@cowlicks can you get someone with more solid experience to review the js portion please

[github-actions]

Preview: https://blake1-bac-45-comment-thread-r-jcmwxk.preview.macro.com/app (9e17bce)

[cowlicks]

I think the failures are due to both FE & BE types changes. They should be sync'd though.

[claude]

Code review

Found one issue that needs attention:

Missing middleware for edit_comment_handler

Location: rust/cloud-storage/document_storage_service/src/api/annotations/mod.rs lines 53-56

Issue: The edit_comment_handler function requires a document_context: Extension<DocumentBasic> parameter (see edit_comment.rs:53), but the route in mod.rs does not have the ensure_document_exists middleware applied. This will cause a runtime error when the endpoint is called because the extension will not be present.

Comparison: The create_comment_handler route at mod.rs:38-45 correctly applies the middleware:

.route(
    "/comments/document/:document_id",
    post(create_comment::create_comment_handler).layer(ServiceBuilder::new().layer(
        axum::middleware::from_fn_with_state(
            state.clone(),
            macro_middleware::cloud_storage::document::ensure_document_exists::handler,
        ),
    )),
)

Fix needed: The edit_comment route needs either:

1. The same middleware applied to the route, OR
2. A different approach to obtain the DocumentBasic context (e.g., fetching it from the comment's associated document within the handler)

Note: The current route path /comments/comment/:comment_id only has comment_id, while the middleware expects a document_id path parameter. This will need to be addressed in the fix.

[whutchinson98]

@cowlicks Claude caught a good bug actually. This can't get attached since there is no document_id in the request. Is this even needed?

[cowlicks]

Hmmm yeah I'm not sure what to do about that. Ideas:

• add a new route /document/:document_id/comment/:comment_id. breaking the current API.
• adding document_id etc to body. we would still have to hit the db to verify the comment is actually on the document. And if we are doing that we might as well drop the body data and do....
• Fetch the document_id etc from the db and use it to build and send the notification. This seems like the best option. Leaving the API as is. We could later use this logic if we move to using pg_notify for events.

[cowlicks]

It looks like we're already getting the document_id & owner within the edit_document_comment so I'll the needed document data there.

[whutchinson98]

awesome

stale