feat: flow items on web atrium#211
Conversation
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
apps/atrium-telegram/server/api/flow/index.post.ts (1)
26-39: Clarify the status of commented notification code.This block contains bot notification logic that appears to be incomplete or temporarily disabled. Consider either:
- Removing it if the feature is no longer planned
- Converting it to a tracked TODO/issue if implementation is deferred
- Uncommenting and completing it if it's ready
apps/web-app/app/components/NotificationsDrawer.vue (2)
28-38: Consider renaming component to match new purpose.The component has been repurposed from rendering notifications to rendering flow items, but retains the name "NotificationsDrawer". For clarity and maintainability, consider renaming it to
FlowDraweror similar to reflect its actual functionality.
32-38: Consider adding an empty state for the flow list.When
flowStore.itemsis empty, the component renders a blank container. Consider adding an empty state message or placeholder to improve user experience.Example:
<div class="flex flex-col gap-3.5"> + <UCard v-if="flowStore.items.length === 0"> + <p class="text-muted">No flow items yet.</p> + </UCard> <FlowItemCard v-for="item in flowStore.items" :key="item.id" :item="item" /> </div>apps/web-app/server/api/flow/list.get.ts (1)
4-4: Consider adding pagination to prevent large responses.The endpoint returns all flow items without pagination, which could lead to performance issues as the dataset grows. Consider adding pagination parameters (e.g., limit, offset or cursor-based pagination).
Example:
export default defineEventHandler(async (event) => { try { const query = getQuery(event) const limit = Number(query.limit) || 20 const offset = Number(query.offset) || 0 return db.flow.listItems({ limit, offset }) } catch (error) { throw errorResolver(error) } })apps/web-app/app/stores/flow.ts (2)
22-29: Improve error handling pattern.Checking
error.message.includes('401')anderror.message.includes('404')is fragile because:
- Error message format can vary across environments
- The message might contain these strings in different contexts
- HTTP status codes should be checked via a dedicated property
Consider checking the error's statusCode property instead:
} catch (error) { - if (error instanceof Error) { - if (error.message.includes('401')) { - // No - } - if (error.message.includes('404')) { - // Not found - } + const statusCode = error?.statusCode || error?.response?.status + if (statusCode === 401) { + // Unauthorized + } else if (statusCode === 404) { + // Not found + } else if (error) { + console.error('Error in flow store:', error) } }Note: Apply this pattern to both
update()andaddView()error handlers.Also applies to: 41-48
33-50: Optimize addView to avoid refetching entire list.After successfully creating a view,
addViewcallsupdate()which refetches the entire flow items list from the server. This is inefficient, especially if the list is large.Consider updating the local state directly instead:
async function addView(itemId: string) { try { await $fetch(`/api/flow/id/${itemId}/view`, { method: 'POST', }) - await update() + // Update local state instead of refetching + const item = items.value.find(i => i.id === itemId) + if (item) { + item.views.push({ + itemId, + userId: useUserStore().id, + createdAt: new Date().toISOString(), + }) + } } catch (error) {This avoids an unnecessary network round-trip and improves perceived performance.
apps/web-app/app/components/FlowItemCard.vue (1)
80-92: Consider handling unknown flow item types.The
getIconNamefunction handles specific types but falls back to a generic clipboard icon for unknown types. Consider logging unknown types to help identify missing mappings during development.function getIconName(type: FlowItemWithData['type']): string { switch (type) { case 'user_post': return 'i-lucide-square-user-round' case 'partner_maintenance': return 'i-lucide-user' case 'daily_task_report': case 'weekly_task_report': return 'i-lucide-clipboard-check' default: + if (import.meta.dev) { + console.warn(`Unknown flow item type: ${type}`) + } return 'i-lucide-clipboard' } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/atrium-telegram/app/components/form/CreateFlowItem.vue(1 hunks)apps/atrium-telegram/app/components/form/CreateFlowItemComment.vue(1 hunks)apps/atrium-telegram/server/api/flow/id/[itemId]/comment.post.ts(1 hunks)apps/atrium-telegram/server/api/flow/index.post.ts(1 hunks)apps/web-app/app/app.vue(3 hunks)apps/web-app/app/components/FlowItemCard.vue(1 hunks)apps/web-app/app/components/Header.vue(3 hunks)apps/web-app/app/components/Navigation.vue(0 hunks)apps/web-app/app/components/NotificationsDrawer.vue(2 hunks)apps/web-app/app/stores/flow.ts(1 hunks)apps/web-app/server/api/flow/id/[itemId]/comment.post.ts(1 hunks)apps/web-app/server/api/flow/id/[itemId]/view.post.ts(1 hunks)apps/web-app/server/api/flow/list.get.ts(1 hunks)packages/essence/package.json(1 hunks)packages/essence/src/schema/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web-app/app/components/Navigation.vue
🧰 Additional context used
🧬 Code graph analysis (4)
apps/web-app/app/stores/flow.ts (1)
packages/database/src/types/tables.ts (3)
FlowItem(166-166)FlowItemComment(169-169)FlowItemView(172-172)
apps/web-app/server/api/flow/id/[itemId]/comment.post.ts (2)
packages/essence/src/schema/services/flow.ts (1)
createFlowItemCommentSchema(12-14)apps/web-app/server/services/db.ts (1)
db(164-164)
apps/web-app/server/api/flow/list.get.ts (1)
apps/web-app/server/services/db.ts (1)
db(164-164)
apps/web-app/server/api/flow/id/[itemId]/view.post.ts (1)
apps/web-app/server/services/db.ts (1)
db(164-164)
⏰ 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). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build
🔇 Additional comments (14)
apps/atrium-telegram/app/components/form/CreateFlowItemComment.vue (1)
34-35: LGTM: Import migration to centralized schema package.The import path change aligns with the PR's objective to consolidate flow schemas into the essence package. The component logic remains unchanged.
apps/atrium-telegram/server/api/flow/index.post.ts (1)
2-2: LGTM: Import migration to centralized schema package.The import path change is consistent with the schema consolidation effort across the codebase.
apps/atrium-telegram/app/components/form/CreateFlowItem.vue (1)
44-45: LGTM: Import migration to centralized schema package.The import path changes align with the PR's schema consolidation objective. Component logic remains intact.
apps/atrium-telegram/server/api/flow/id/[itemId]/comment.post.ts (1)
2-2: LGTM: Import migration to centralized schema package.The import path change maintains consistency with the schema consolidation effort across the codebase.
packages/essence/package.json (1)
7-8: LGTM: Schema export entry enables external access.The new export path properly exposes the schema module, allowing consumers to import from
@roll-stack/essence/schemaas demonstrated throughout the PR.apps/web-app/app/components/NotificationsDrawer.vue (1)
50-50: LGTM: Store migration to flow-based data.The switch from notification store to flow store aligns with the PR's objective to introduce flow-based UI.
packages/essence/src/schema/index.ts (1)
1-1: All required schemas are exported fromservices/flow. The re-export inschema/index.tscorrectly exposescreateFlowItemSchema,CreateFlowItem,createFlowItemCommentSchema, andCreateFlowItemComment.apps/web-app/app/components/Header.vue (1)
14-33: LGTM!The transition from notifications to flow stream is implemented correctly. The icon change from bell to waves and the badge now tracking unviewed flow items align well with the new Flow feature.
Also applies to: 51-51
apps/web-app/server/api/flow/id/[itemId]/comment.post.ts (2)
23-35: LGTM!The comment creation logic correctly validates the item's existence before creating the comment, and properly associates it with the current user.
3-3: arktype import is required for error handling.
createFlowItemCommentSchemais defined withtype()from arktype, so checkingdata instanceof type.errorsis correct; no change needed.Likely an incorrect or invalid review comment.
apps/web-app/app/app.vue (1)
54-54: LGTM!The store replacement from Epic to Flow is correctly implemented across initialization and periodic updates.
Also applies to: 80-80, 90-90
apps/web-app/app/stores/flow.ts (1)
11-11: Verify userStore initialization order.Line 11 calls
useUserStore().iddirectly within the computed property. If the user store isn't fully initialized when this computed runs, it could cause issues.Consider storing the user ID in a local variable or ensuring the user store is initialized before the flow store is accessed. Can you verify the initialization order in app.vue to ensure userStore is loaded before flowStore is used?
apps/web-app/app/components/FlowItemCard.vue (2)
77-78: Verify store initialization and add null safety.Lines 77-78 access
useUserStore()directly within computed properties, which could cause issues if stores aren't properly initialized. Additionally, there's no null safety foritem.viewsoruserStore.users.Consider defensive checks:
-const isViewed = computed(() => item?.views.some((view) => view.userId === useUserStore().id)) -const userAvatarUrl = computed(() => userStore.users.find((user) => user.id === item.userId)?.avatarUrl ?? undefined) +const isViewed = computed(() => item?.views?.some((view) => view.userId === userStore.id) ?? false) +const userAvatarUrl = computed(() => { + if (!item.userId || !userStore.users) return undefined + return userStore.users.find((user) => user.id === item.userId)?.avatarUrl ?? undefined +})
1-65: LGTM with minor suggestions!The component template is well-structured with clear conditional rendering, proper use of date formatting, and good visual feedback for unviewed items.
| if (item.views.some((view) => view.userId === event.context.user.id)) { | ||
| throw createError({ | ||
| statusCode: 400, | ||
| message: 'Already viewed', | ||
| }) | ||
| } | ||
|
|
||
| await db.flow.createItemView({ | ||
| itemId, | ||
| userId: event.context.user.id, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
Potential race condition in duplicate view check.
Between checking if the user has already viewed the item (lines 23-28) and creating the view record (lines 30-33), another concurrent request could create a duplicate view entry. This is a classic TOCTOU (time-of-check to time-of-use) issue.
Consider one of these solutions:
- Add a unique constraint on (itemId, userId) in the database schema and handle the constraint violation error
- Use database-level upsert or "INSERT ... ON CONFLICT DO NOTHING" if supported
- Implement optimistic locking or transaction-level isolation
Can you verify whether db.flow.createItemView or the database schema has safeguards against duplicate view entries?
🏁 Script executed:
#!/bin/bash
# Description: Check database schema for unique constraints on flow item views
# Search for flowItemViews table definition and unique constraints
rg -n -A 10 -B 2 'flowItemViews' --type=ts | rg -E '(unique|primaryKey|table|schema)'Length of output: 396
🏁 Script executed:
#!/bin/bash
# Find Prisma schema files
fd schema.prisma
echo "-----"
# Inspect schema for ItemView model and constraints
for f in $(fd schema.prisma); do
echo "In $f:"
rg -n -C3 "model ItemView" $f || true
rg -n "@@unique" $f || true
rg -n "@unique" $f || true
done
echo "-----"
# Locate createItemView usage or definition
rg -n -C3 "createItemView" --type=ts || trueLength of output: 1581
🏁 Script executed:
#!/usr/bin/env bash
# Show import of flowItemViews in repository code
sed -n '1,20p' packages/database/src/repository/flow.ts
# Locate the flowItemViews table definition
rg -n -C5 "flowItemViews" packages/database/srcLength of output: 5463
🏁 Script executed:
#!/usr/bin/env bash
# Extract the full flow_item_views table definition to inspect columns and constraints
rg -n -C10 "export const flowItemViews = pgTable" -n packages/database/src/tables.ts
rg -n -A10 -nP "export const flowItemViews = pgTable\\('flow_item_views', \\{[^}]*\\}\\)" packages/database/src/tables.tsLength of output: 1336
Enforce uniqueness on view records at the database level
There’s no unique constraint on flow_item_views(userId, itemId), so concurrent requests can still insert duplicates.
• Add a unique index/constraint on (user_id, item_id) in the schema
• Use INSERT … ON CONFLICT DO NOTHING or an upsert via Drizzle ORM
• Catch and surface unique‐violation errors in createItemView to return 400
| export default defineEventHandler(async () => { | ||
| return db.flow.listItems() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
Add authentication to protect the flow list endpoint.
The endpoint has no authentication check, making flow items publicly accessible. Verify if this is intentional, or add authentication similar to other flow endpoints.
Compare with the authenticated comment endpoint at apps/atrium-telegram/server/api/flow/id/[itemId]/comment.post.ts which uses event.context.user.id. If flow items should be protected, apply authentication middleware or add an auth check:
-export default defineEventHandler(async () => {
+export default defineEventHandler(async (event) => {
+ // Add authentication check if required
+ if (!event.context.user) {
+ throw createError({
+ statusCode: 401,
+ message: 'Unauthorized',
+ })
+ }
+
try {
return db.flow.listItems()
} catch (error) {
throw errorResolver(error)
}
})Verify the intended access control for this endpoint:
🏁 Script executed:
#!/bin/bash
# Description: Check authentication patterns in other API endpoints
# Search for authentication patterns in web-app server routes
rg -A 5 "event\.context\.user|Authorization" apps/web-app/server/api/
# Check if there's middleware handling authentication
fd -t f "middleware" apps/web-app/server/Length of output: 23119
Require authentication on flow list endpoint
apps/web-app/server/api/flow/list.get.ts currently returns flow items without checking event.context.user. Add an event parameter and guard:
export default defineEventHandler(async (event) => {
if (!event.context.user) {
throw createError({ statusCode: 401, message: 'Unauthorized' })
}
return db.flow.listItems()
})Mirror auth logic from other flow routes.
🤖 Prompt for AI Agents
In apps/web-app/server/api/flow/list.get.ts around lines 3 to 5, the handler
returns flow items without authenticating; update the default export to accept
an event parameter, check event.context.user and if missing throw createError({
statusCode: 401, message: 'Unauthorized' }), otherwise proceed to call and
return db.flow.listItems(); mirror the same auth guard pattern used in the other
flow routes so unauthorized requests are rejected.
Add error handling to the endpoint.
The handler lacks try-catch error handling. If db.flow.listItems() throws an exception, it will result in an unhandled error or generic 500 response without proper error messaging.
Apply this diff to add error handling:
export default defineEventHandler(async () => {
- return db.flow.listItems()
+ try {
+ return db.flow.listItems()
+ } catch (error) {
+ throw errorResolver(error)
+ }
})📝 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.
| export default defineEventHandler(async () => { | |
| return db.flow.listItems() | |
| }) | |
| export default defineEventHandler(async () => { | |
| try { | |
| return db.flow.listItems() | |
| } catch (error) { | |
| throw errorResolver(error) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In apps/web-app/server/api/flow/list.get.ts around lines 3 to 5, the handler
directly returns db.flow.listItems() without try/catch; wrap the async handler
body in a try/catch, call db.flow.listItems() inside try, and in catch log the
error and turn it into a proper HTTP error response (use sendError(createError({
statusCode: 500, statusMessage: 'Failed to list flows', data: error })) or
otherwise set a 500 status and return a clear error message), ensuring the
original error details are included for debugging.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/atrium-telegram/server/api/flow/id/[itemId]/comment.post.ts (1)
34-34: Validate user context existence before access.
event.context.user.idis accessed without verifying thatevent.context.userexists. If authentication middleware doesn't populate this, it will cause a runtime error.Add a guard to check user authentication:
+ if (!event.context.user) { + throw createError({ + statusCode: 401, + message: 'Authentication required', + }) + } + await db.flow.createItemComment({ text: data.text, itemId, userId: event.context.user.id, })
🧹 Nitpick comments (2)
apps/web-app/server/api/flow/id/[itemId]/comment.post.ts (2)
1-41: Consider extracting shared logic to reduce duplication.This endpoint is nearly identical to
apps/atrium-telegram/server/api/flow/id/[itemId]/comment.post.ts. The only difference appears to be the hosting application. Consider extracting the shared validation and comment creation logic into a reusable utility function in@roll-stack/schemaor a shared server utilities package.Example refactoring approach:
In
packages/schema/src/services/flow.tsor a shared utilities package:export async function handleCreateFlowItemComment( itemId: string, body: unknown, userId: string ) { const data = createFlowItemCommentSchema(body) if (data instanceof type.errors) { throw data } const item = await db.flow.findItem(itemId) if (!item) { throw createError({ statusCode: 404, message: 'Item not found', }) } await db.flow.createItemComment({ text: data.text, itemId, userId, }) return { ok: true } }Then both endpoints can call this shared function with minimal boilerplate.
16-19: Consider improving error response format.arktype validation errors are thrown directly as
type.errors, which may not provide user-friendly error messages in API responses. Consider transforming these into a more structured error format.Example transformation:
const data = createFlowItemCommentSchema(body) if (data instanceof type.errors) { - throw data + throw createError({ + statusCode: 400, + message: 'Validation failed', + data: data.summary, + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/atrium-telegram/app/components/form/CreateFlowItem.vue(1 hunks)apps/atrium-telegram/app/components/form/CreateFlowItemComment.vue(1 hunks)apps/atrium-telegram/package.json(1 hunks)apps/atrium-telegram/server/api/flow/id/[itemId]/comment.post.ts(1 hunks)apps/atrium-telegram/server/api/flow/index.post.ts(1 hunks)apps/web-app/package.json(1 hunks)apps/web-app/server/api/flow/id/[itemId]/comment.post.ts(1 hunks)packages/schema/package.json(1 hunks)packages/schema/src/index.ts(1 hunks)packages/schema/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/schema/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/atrium-telegram/server/api/flow/index.post.ts
- apps/atrium-telegram/app/components/form/CreateFlowItem.vue
- apps/atrium-telegram/app/components/form/CreateFlowItemComment.vue
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web-app/server/api/flow/id/[itemId]/comment.post.ts (2)
packages/schema/src/services/flow.ts (1)
createFlowItemCommentSchema(12-14)apps/web-app/server/services/db.ts (1)
db(164-164)
⏰ 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: build
🔇 Additional comments (4)
packages/schema/tsconfig.json (1)
1-32: LGTM!The TypeScript configuration is well-structured with appropriate strict settings for a schema package. Key highlights:
- Strict mode with additional safety checks (
noUncheckedIndexedAccess,noImplicitThis)- Modern module resolution (
preserve+Bundler)- Proper isolation settings for library code
apps/atrium-telegram/server/api/flow/id/[itemId]/comment.post.ts (1)
2-2: LGTM! Import refactored to use centralized schema package.The import change successfully migrates from internal path to the new
@roll-stack/schemaworkspace package, aligning with the broader refactoring effort.packages/schema/src/index.ts (1)
1-1: Approve re-export of services/flow
services/flow.tsexportscreateFlowItemSchema,createFlowItemCommentSchema(and their types), so the barrel export correctly exposes the intended API.apps/atrium-telegram/package.json (1)
21-21: Schema package exports verified.@roll-stack/schemacorrectly re-exportscreateFlowItemSchemaandcreateFlowItemCommentSchemaviasrc/index.ts.
| "@pinia/nuxt": "catalog:", | ||
| "@roll-stack/database": "workspace:*", | ||
| "@roll-stack/essence": "workspace:*", | ||
| "@roll-stack/schema": "workspace:*", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify arktype dependency status.
The dependency addition is correct. However, the AI summary mentions "removed the arktype catalog dependency," but arktype is still imported and used in the server API files (e.g., import { type } from 'arktype' in comment.post.ts). Ensure arktype remains as a dependency if it's still being used.
🏁 Script executed:
#!/bin/bash
# Description: Check if arktype is still used in the codebase
echo "=== Searching for arktype imports ==="
rg -nP --type=ts "import.*from ['\"]arktype['\"]" apps/web-app/
echo ""
echo "=== Checking current dependencies ==="
cat apps/web-app/package.json | jq '.dependencies, .devDependencies'Length of output: 5237
Declare arktype in package.json
Arktype is imported across multiple server and shared files but missing from apps/web-app/package.json dependencies—add "arktype": "<appropriate version>".
🤖 Prompt for AI Agents
In apps/web-app/package.json around line 21, the project imports arktype but it
is not declared as a dependency; add "arktype": "<appropriate version>" to the
dependencies object (use the repo workspace spec if this is a monorepo, e.g.
"workspace:*", or a specific semver like "^x.y.z"), save the file and run your
package manager (npm/yarn/pnpm) to update node_modules and the lockfile so
builds and imports resolve correctly.
| await db.flow.createItemComment({ | ||
| text: data.text, | ||
| itemId, | ||
| userId: event.context.user.id, |
There was a problem hiding this comment.
Validate user context existence before access.
event.context.user.id is accessed without verifying that event.context.user exists. If authentication middleware doesn't populate this, it will cause a runtime error.
Add authentication guard:
+ if (!event.context.user) {
+ throw createError({
+ statusCode: 401,
+ message: 'Authentication required',
+ })
+ }
+
await db.flow.createItemComment({
text: data.text,
itemId,
userId: event.context.user.id,
})🤖 Prompt for AI Agents
In apps/web-app/server/api/flow/id/[itemId]/comment.post.ts around line 34, the
code accesses event.context.user.id without ensuring event.context.user exists;
add a guard that checks event.context and event.context.user before reading .id,
and return an appropriate authentication error (e.g., 401) or throw a clear
error when the user context is missing; update the handler flow so that after
the guard you can safely use userId and, if applicable, narrow the type (or
assert) to avoid TypeScript errors.
1 participant
Summary by CodeRabbit