feat: task comments (backend + UI + tests)#66
Conversation
- Add comments UI in task modal\n- Add comments API endpoints + types\n- Add SQL migration for task_comments + RLS policies\n- Add Playwright E2E + component tests\n- Update DB schema docs
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: fbc6a37959
ℹ️ 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".
| WITH CHECK ( | ||
| EXISTS ( | ||
| SELECT 1 FROM public.tasks | ||
| WHERE public.tasks.id = public.task_comments.task_id | ||
| AND public.tasks.user_id = auth.uid() |
There was a problem hiding this comment.
The INSERT RLS policy only verifies that the task belongs to auth.uid(), but it does not constrain user_id, so an authenticated user can insert comments on their own task while spoofing another user's identity fields via direct Supabase access. In that scenario, comment authorship data is untrusted and the row may become undeletable by the creator because the DELETE policy requires auth.uid() = user_id. Add a user_id = auth.uid() check to the WITH CHECK predicate.
Useful? React with 👍 / 👎.
| const { content } = req.body; | ||
| const user_id = req.user.id; | ||
|
|
||
| if (!content || !content.trim()) { |
There was a problem hiding this comment.
This validation path calls content.trim() without confirming content is a string, so requests like { "content": 123 } throw a TypeError and return 500 from the catch block instead of a 4xx validation response. The bug is triggered by malformed client payloads and makes bad input look like a server failure; guard with typeof content === 'string' before trimming.
Useful? React with 👍 / 👎.
- Enforce auth.uid() = user_id in INSERT policy - Validate comment content type before trimming
|
Gracias por el review. ✅ Ya apliqué ambos cambios en
Volví a correr los tests relevantes y el E2E de comentarios sigue pasando. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff2c7e98d2
ℹ️ 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 (response.data) { | ||
| setComments((prev) => [...prev, response.data!]); |
There was a problem hiding this comment.
This handler only processes the success path and silently ignores response.error, so in supported unauthenticated/offline flows (where the API returns Not authenticated) the form remains usable but comments are never saved and the user gets no feedback. That makes the new comments UI appear to work while dropping user actions, which is a functional regression for local/offline task usage.
Useful? React with 👍 / 👎.
- Surface API errors from addComment via toast - Add unit test for failed comment submission path
|
Update aplicado para el nuevo review ✅ Fix implementado
Cobertura
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7080291e0
ℹ️ 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".
| const response = await taskService.getComments(taskId); | ||
| if (response.data) { | ||
| setComments(response.data); | ||
| } |
There was a problem hiding this comment.
fetchComments only updates state when response.data exists, so on an error path the previous comments array is kept. If a user opens task A, then switches to task B and the B request fails (network/auth/backend error), the UI can keep showing task A comments under task B after loading finishes, which is incorrect task data. Clear comments on task change or explicitly handle response.error to avoid stale cross-task rendering.
Useful? React with 👍 / 👎.
- Reset comments state when loading a different task - Surface getComments API errors with toast - Add unit test for stale comment regression path
|
Nuevo review atendido ✅ (P2 sobre stale comments al cambiar de tarea y fallar fetch) Fix aplicado
Cobertura agregada
Commit
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0751fad0c7
ℹ️ 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 (response.data) { | ||
| setComments(response.data); |
There was a problem hiding this comment.
When taskId changes while a previous getComments call is still in flight, the older response can resolve later and overwrite the current task’s comments through setComments(response.data), so users can see comments from the wrong task under normal network reordering. The same race also affects loading state because each request independently calls setIsLoading(false), so this should be tied to the latest request (for example via request IDs or AbortController) before updating component state.
Useful? React with 👍 / 👎.
- Guard comment state updates with request sequence IDs - Ignore out-of-order responses and stale loading transitions
|
Nuevo review atendido ✅ (race condition por respuestas stale en Fix aplicado
Validación
Commit
Este es un Code Review hecho por Kimi k2.5 @Kimi k2.5 por favor revisa el PR #66 completo y deja findings; si no encuentras issues, responde explícitamente Approved. |
Kimi K2.5 Co-ReviewKimi review: no output (empty response). |
Kimi K2.5 Co-ReviewFindings P1 - Security/Correctness
P2 - UX/Edge Cases
P3 - Performance/Minor
Note on Security (Non-blocking): The implementation correctly uses |
- Add backend comment length guard + lightweight cooldown\n- Add comments pagination and explicit column selection\n- Use maybeSingle for optional profile lookup\n- Add task_comments(task_id) index in migration\n- Add textarea maxLength + live character counter
|
Update aplicado sobre el review largo de Kimi ✅ Cambios hechos en este push (
|
Kimi K2.5 Co-ReviewFindings
Nota menor: El workflow de Kimi Co-Review usa |
Kimi K2.5 Co-ReviewFindings P1 | backend/src/controllers/taskController.jsMemory leak + Rate limiting no distribuido
P2 | backend/src/controllers/taskController.jsEndpoint DELETE documentado pero no implementado
P2 | e2e/task-comments.spec.tsRace condition con cooldown de 1.5s
P3 | src/components/TaskComments.tsxFalta paginación
P3 | src/components/TaskComments.tsxUX: No hay indicación del cooldown
P3 | backend/src/controllers/taskController.jsInconsistencia de tipo en task_id
|
Kimi K2.5 Co-ReviewKimi review failed: request error/timeout. |
Kimi K2.5 Co-ReviewKimi review failed: request error/timeout. |
Kimi K2.5 Co-ReviewCode Review FindingsP2 - Security: Missing task ownership verification on comment mutationsFile: Issue: Both Recommendation: Add a check to verify task ownership before allowing comment modifications: // Verify task belongs to user before allowing comment mutation
const { data: task, error: taskErr } = await db
.from('tasks')
.select('id')
.eq('id', task_id)
.eq('user_id', user_id)
.single();
if (taskErr || !task) {
return res.status(404).json({ error: 'Task not found or access denied' });
}P2 - UX: No rate limiting feedback in frontendFile: Issue: While the backend implements a 1.5s cooldown ( Recommendation: Implement visual cooldown state:
P3 - Security: Missing UPDATE RLS policyFile: Issue: The RLS policies only cover SELECT, INSERT, and DELETE. There is no explicit UPDATE policy, which means updates might fail or fall back to default restrictive behavior depending on Supabase configuration. Recommendation: Add UPDATE policy: CREATE POLICY "Users can update their own comments"
ON public.task_comments
FOR UPDATE
USING (auth.uid() = user_id)
WITH CHECK (auth.uid() = user_id);P3 - Documentation: Incomplete Swagger documentationFile: Issue: Swagger documentation is only provided for GET and POST Recommendation: Add OpenAPI annotations for the remaining endpoints including path parameters ( P3 - UX: Missing optimistic update rollbackFile: Issue: On comment submission success, the component appends the new comment to local state immediately ( Recommendation: Consider invalidating/refetching the comments list after successful submission to ensure consistency, or implement proper optimistic update rollback on error. P3 - Correctness: HTML sanitization edge caseFile: Issue: The regex Recommendation: Consider using a proper sanitization library like Approved
|
Kimi K2.5 Co-ReviewCode Review: PR #66 FindingsP1 - Security/Correctness
P2 - UX Edge Cases / Robustness
P3 - Minor / Best Practices
SummaryNo aprobar hasta resolver P1 (validación de pertenencia task-comment en update/delete). Los demás son mejoras recomendadas. |
Kimi K2.5 Co-ReviewFindingsP2 |
|
Kimi K2.5 Co-ReviewKimi review failed: request error/timeout. |
Kimi K2.5 Co-ReviewFindings
Notas menores
|
Kimi K2.5 Co-ReviewKimi review failed: request error/timeout. |
Kimi K2.5 Co-ReviewCode Review: PR #66 - Task Comments Overall a solid implementation with good security practices (RLS, input sanitization, cooldown) and comprehensive testing. However, there are concurrency and consistency issues that should be addressed before merge. 🔴 P2 - Race Condition in Comment CooldownFile: The DB-backed cooldown check is not atomic. Two concurrent requests can query Recommendation: Use a unique constraint on 🔴 P2 - Inconsistent Error Handling PatternsFile:
Recommendation: Standardize on 🔴 P2 - Missing Transaction BoundariesFile: Task ownership verification and comment insertion are separate queries. If the task is deleted between SELECT and INSERT/UPDATE, it throws a 500 FK error instead of a controlled 404. Recommendation: Use a transaction or at least handle 🟡 P3 - Missing Swagger DocumentationFile: PATCH and DELETE endpoints for comments lack Swagger/OpenAPI documentation (only GET and POST are documented). Recommendation: Add swagger-jsdoc comments for consistency with the rest of the API. 🟡 P3 - Client-Side Clock Drift in Cooldown UXFile: The countdown uses Recommendation: Calculate remaining time based on the server's 🟡 P3 - Missing Composite IndexFile:
Recommendation: Add 🟢 Approved with ReservationsThe implementation is functionally correct and secure for production, but the P2 concurrency issues should be addressed in a follow-up PR to prevent potential abuse or inconsistent state under high load. Security Note: Good job on the backend HTML escaping + React's native escaping (defense in depth). The RLS policies correctly implement user isolation. |
1 participant
Qué incluye
Cómo probar
Notas