Conversation
feat: integrate branding
chore: regenerate supabase types chore(db): align rbac with initial schema
feat: setup homepage redirect
feat: resident onboarding
feat: admin panel
fix: sync supabase types
…eporting feat: implement form-based incident reporting
feat: status page, health checkpoint, and terms and privacy pages
![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: e60cdf30b1
ℹ️ 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".
| begin | ||
| if target_user_id is null then | ||
| raise exception 'target_user_id is required'; | ||
| end if; | ||
|
|
There was a problem hiding this comment.
set_staff_role is created as SECURITY DEFINER but only validates input values, not the caller’s privileges. Because this function performs direct DELETE/INSERT on public.role_assignments, any authenticated user who can call the RPC can promote themselves (or others) to admin/responder, bypassing the table’s RLS policies. Add an explicit role check inside the function (or revoke execute from non-privileged roles and expose a guarded wrapper).
Useful? React with 👍 / 👎.
| const { data, error } = await adminClient.auth.admin.listUsers({ | ||
| page: 1, | ||
| perPage: 1000, | ||
| }); |
There was a problem hiding this comment.
emailBelongsToExistingUser only inspects the first listUsers page (page: 1, perPage: 1000). Once the project has more than 1000 users, existing accounts on later pages will be missed, so invite creation/reissue can incorrectly proceed for already-registered emails and fail later during acceptance. Use pagination (or a direct lookup by email) so this check remains correct at larger user counts.
Useful? React with 👍 / 👎.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions.
This PR introduces a dynamic webhook routing system and enhances authentication with gated registration, while improving CI reliability with mock fallbacks. No critical runtime issues were found, but several architectural and UX changes require attention.
📄 Documentation Diagram
This diagram illustrates the new dynamic webhook routing system introduced in this PR.
sequenceDiagram
participant External as External Service
participant API as API Endpoint
participant Bot as Bot Handler
note over API: PR #35;1 changed to dynamic routing
External->>API: POST /api/webhooks/[platform]
API->>API: Extract platform from route
alt Platform exists in bot.webhooks
API->>Bot: Call handler(platform)
Bot-->>API: Response
else Platform not found
API-->>External: 404 Not Found
end
API-->>External: Return response
🌟 Strengths
- Improved CI robustness for forked PRs with safe fallback values, preventing workflow failures.
- Enhanced user experience with success notices and a guided, invite-based authentication flow.
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P2 | .github/workflows/ci.yml | Maintainability | Prevents CI failures but hardcoded fallbacks may cause runtime issues. | path:.env.example |
| P2 | app/api/webhooks/[platform]/route.ts | Architecture | Breaks existing webhook endpoints; requires external config updates. | |
| P2 | app/auth/invite/page.tsx | Performance | Database connection calls could increase latency on auth pages. | path:app/auth/login/page.tsx |
| P2 | app/auth/sign-up/page.tsx | Business Logic | Disables public sign-ups after initial admin, enforcing invite-only access. | |
| P2 | app/auth/login/page.tsx | User Experience | Adds conditional success notices to improve post-action user feedback. | path:app/auth/sign-up-success/page.tsx |
🔍 Notable Themes
- Architectural Refactoring: The webhook system has been generalized to support multiple platforms, requiring updates to integration points and potentially breaking existing setups.
- Authentication Flow Enhancement: New pages and logic for invite-based registration and success feedback create a more controlled and user-friendly onboarding process.
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| # Forked PRs do not receive repository secrets, so fall back to safe | ||
| # placeholders instead of leaving the env vars empty. | ||
| TELEGRAM_BOT_TOKEN: ${{ secrets.TELEGRAM_BOT_TOKEN || 'mock-telegram-bot-token' }} | ||
| TELEGRAM_WEBHOOK_SECRET_TOKEN: ${{ secrets.TELEGRAM_WEBHOOK_SECRET_TOKEN || 'mock-webhook-secret-token' }} | ||
| TELEGRAM_BOT_USERNAME: ${{ secrets.TELEGRAM_BOT_USERNAME || 'project_hermes_bot' }} |
There was a problem hiding this comment.
P2 | Confidence: High
The GitHub Actions workflow now provides fallback values for Telegram environment variables when secrets are unavailable (e.g., in forked PRs). This prevents CI failures due to missing secrets. However, the fallback values are hardcoded mock strings ('mock-telegram-bot-token', 'mock-webhook-secret-token', 'project_hermes_bot'). While this ensures the workflow runs, any code path that depends on these tokens being valid (e.g., actual Telegram API calls) will fail or behave unexpectedly at runtime. The change in .env.example adds these same variables, confirming they are required runtime configuration.
|
|
||
| type Platform = keyof typeof bot.webhooks; | ||
|
|
||
| export async function POST( |
There was a problem hiding this comment.
P2 | Confidence: High
(Auto-downgraded from P0/P1: missing non-empty code snippet evidence for a non-speculative finding) This introduces a breaking change to the webhook routing architecture. The previous dedicated endpoint (/api/webhooks/telegram) has been removed and replaced with a dynamic route (/api/webhooks/[platform]). This changes the public API for webhook integrations. Any external service (like Telegram's webhook configuration) or internal routing that directly called /api/webhooks/telegram will now receive a 404 unless updated to use the new dynamic path pattern. The bot.webhooks object must now be a map keyed by platform strings (e.g., 'telegram'). This is a significant architectural shift from a single, known endpoint to a pluggable multi-platform system.
| ); | ||
| } | ||
|
|
||
| export default async function Page({ |
There was a problem hiding this comment.
P2 | Confidence: High
The pattern await connection() is used in multiple new auth-related pages (/invite, /login, /sign-up). This appears to be a database connection initialization call. Running this on every page request, especially for public-facing auth pages, could introduce latency and unnecessary database load if not managed efficiently (e.g., connection pooling). The related context shows this pattern is now widespread. While necessary for server-side data fetching, its performance impact should be monitored, especially under load.
| function getNoticeMessage(notice: string | undefined) { | ||
| switch (notice) { | ||
| case 'bootstrap-admin-created': | ||
| return 'Initial admin account created. Sign in to continue.'; | ||
| case 'invite-accepted': | ||
| return 'Account setup complete. Sign in to continue.'; | ||
| default: | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: High
The login page now conditionally displays success notices via URL parameters (?notice=bootstrap-admin-created). This improves user feedback after key actions (initial setup, invite acceptance). The related change to sign-up-success/page.tsx adjusts its messaging to direct users to login, creating a cohesive flow: Sign-up → Success Page → Login with notice. This is a positive UX improvement but relies on correct parameter passing from the redirecting actions.
| } from '@/components/ui/card'; | ||
| import { connection } from 'next/server'; | ||
|
|
||
| export default async function Page() { |
There was a problem hiding this comment.
P2 | Confidence: Medium
This change implements a gated registration system. After the initial 'bootstrap' admin is created, public sign-ups are disabled, and new users must be invited. This is a core business logic change for access control. The isBootstrapRegistrationOpen() function's implementation is not visible, but its result dictates the entire page's behavior. This could confuse users if the state changes unexpectedly or if the function has edge cases (e.g., race conditions during the first admin creation).
3 participants
No description provided.