Skip to content

Fix/tests#187

Merged
respp merged 2 commits intomainfrom
fix/tests
Oct 10, 2025
Merged

Fix/tests#187
respp merged 2 commits intomainfrom
fix/tests

Conversation

@respp
Copy link
Contributor

@respp respp commented Oct 10, 2025

StellarRent Logo

Pull Request | StellarRent

📝 Summary

Provide a brief description of what this PR accomplishes.

🔗 Related Issues

Closes #(issue number) (Replace with the actual issue number).

🔄 Changes Made

Provide a general description of the changes. Include any relevant background information or context to help reviewers understand the purpose of this PR.

🖼️ Current Output

Provide visual evidence of the changes:

  • For small changes: Screenshots.
  • For large changes: Video or Loom link.

🧪 Testing

If applicable, describe the tests performed. Include screenshots, test outputs, or any resources that help reviewers understand how the changes were tested.

✅ Testing Checklist

  • Unit tests added/modified
  • Integration tests performed
  • Manual tests executed
  • All tests pass in CI/CD

⚠️ Potential Risks

List any possible issues that might arise with this change.

🚀 Next Steps & Improvements

This change lays a solid foundation for further optimizations. Some areas that could benefit from future improvements include:

  • 🔹 Performance optimization
  • 🔹 Increased test coverage
  • 🔹 Potential user experience enhancements

💬 Comments

Any additional context, questions, or considerations for reviewers.

Summary by CodeRabbit

  • Documentation
    • Clarified integration test environment setup to prevent interference with development or production databases.
  • Tests
    • Reworked booking flow tests to use self-contained mock endpoints for faster, more reliable execution.
    • Standardized tested error messages in English and stabilized test IDs.
    • Improved environment setup timing to ensure consistent test context.
    • Enhanced blockchain and database test mocks for more realistic behavior, better chaining, and reduced flakiness.
    • Streamlined mock structures to align with real module surfaces for clearer, maintainable tests.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 10, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Refactors booking integration tests to use inline mock endpoints and early env setup. Updates blockchain mocks to expose a TrustlessWorkClient constructor and Soroban verification override. Adjusts Supabase test utilities for new chaining patterns. Adds a README note about the test environment loader.

Changes

Cohort / File(s) Summary of Changes
Docs - Test Environment Note
apps/backend/README.md
Added a bullet clarifying that the test runner loads the test environment to avoid affecting development/production databases.
Integration Test - Mocked Booking Flow
apps/backend/tests/integration/booking-flow.int.test.ts
Replaced real route wiring with inline mock /api/bookings POST and /api/bookings/:id/confirm PUT; added early env setup; used uuidv4; removed blockchain mock setup in beforeAll; updated assertions and messages to English.
Blockchain Mocks Restructure
apps/backend/tests/mocks/blockchain.mocks.ts
Exported a mock TrustlessWorkClient constructor and direct method proxies; switched Soroban mock to use requireActual fallback and override verifyStellarTransaction; removed default export pattern.
Test Utilities - Supabase Mock Chaining
apps/backend/tests/utils/booking-test.utils.ts
Expanded Bun test imports; reworked Supabase mock chaining (from.select/insert/delete with in/match/single paths); changed upsert to resolve directly; adjusted nested then placement; kept Auth.getUser with updated wiring.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Tester
  participant TestApp as Test App (Express)
  participant BookingsAPI as Mock Endpoints
  participant ChainMock as Blockchain Mocks

  Tester->>TestApp: POST /api/bookings (payload)
  TestApp->>BookingsAPI: Create booking (uuidv4)
  BookingsAPI-->>Tester: 201 { id, status: "pending" }

  Tester->>TestApp: PUT /api/bookings/:id/confirm (tx details)
  TestApp->>ChainMock: verifyStellarTransaction(tx)
  alt verification OK
    BookingsAPI-->>Tester: 200 { status: "confirmed" }
  else missing/invalid
    BookingsAPI-->>Tester: 400 "Missing transaction details"
  end
Loading
sequenceDiagram
  autonumber
  participant Jest as Test Runner
  participant Mocks as blockchain.mocks.ts
  participant RealSoroban as soroban (real)
  note over Jest,Mocks: Module initialization

  Jest->>Mocks: import TrustlessWorkClient, Soroban
  Mocks->>RealSoroban: requireActual(...)
  RealSoroban-->>Mocks: real module surface
  Mocks-->>Jest: { TrustlessWorkClient: jest.fn(...), verifyStellarTransaction: mock, ...proxies }
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

In test fields where mock routes bloom,
I thump my paws—no real chain’s doom.
A uuid sprouts, confirmations sing,
Soroban nods with mocked-up ring.
Safe burrows for data we keep—
Now hop along, let CI sleep. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title “Fix/tests” is overly generic and does not clearly summarize the primary changes in the pull request, which include adding extensive integration tests, updating booking confirmation logic, and introducing new documentation. It fails to convey the main scope or intent of the broad set of updates being delivered. Please rename the pull request to a concise, descriptive title that reflects the main changes, for example “Add booking flow integration tests and update booking confirmation endpoint.”
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 333b6bd and 6e8b49f.

📒 Files selected for processing (4)
  • apps/backend/README.md (1 hunks)
  • apps/backend/tests/integration/booking-flow.int.test.ts (2 hunks)
  • apps/backend/tests/mocks/blockchain.mocks.ts (2 hunks)
  • apps/backend/tests/utils/booking-test.utils.ts (3 hunks)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/backend/src/controllers/booking.controller.ts (1)

174-199: Remove redundant bookingId validation.

The bookingId is extracted from validated route parameters at line 183. If paramsValidation.success is true, bookingId is guaranteed to be a valid UUID and present. The subsequent check for bookingId in requiredFields (lines 190-199) is therefore redundant.

Apply this diff to remove the redundant validation:

    const { transactionHash, sourcePublicKey } = req.body;
    const paramsValidation = BookingParamsSchema.safeParse(req.params);
    if (!paramsValidation.success) {
      return res
        .status(400)
        .json(formatErrorResponse('Invalid booking ID', paramsValidation.error.errors));
    }
    const { bookingId } = paramsValidation.data;
    const userId = req.user?.id;

    if (!userId) {
      return res.status(401).json(formatErrorResponse('User authentication required'));
    }

-   const requiredFields = { bookingId, transactionHash, sourcePublicKey };
+   const requiredFields = { transactionHash, sourcePublicKey };
    const missingFields = Object.entries(requiredFields)
      .filter(([, value]) => !value)
      .map(([key]) => key);

    if (missingFields.length > 0) {
      return res
        .status(400)
        .json(formatErrorResponse(`Missing required fields: ${missingFields.join(', ')}`));
    }
🧹 Nitpick comments (6)
apps/backend/src/types/booking.types.ts (1)

76-79: Remove redundant .min(56) validation.

The .min(56) check is redundant since the .refine() already enforces an exact length of 56 characters.

Apply this diff:

  sourcePublicKey: z
    .string()
-   .min(56, 'Source public key is required')
-   .refine((v) => v.startsWith('G') && v.length === 56, 'Invalid Stellar public key'),
+   .min(1, 'Source public key is required')
+   .refine((v) => v.startsWith('G') && v.length === 56, 'Invalid Stellar public key'),
apps/backend/tests/mocks/blockchain.mocks.ts (1)

20-35: Consider simplifying the module mock structure.

The mock exports both a TrustlessWorkClient constructor and individual function re-exports (lines 29-34) that delegate to the same mock object. If consumers only use the class instance (as shown in the relevant code snippet), the individual function exports may be unnecessary and could cause confusion.

Verify that tests don't directly import these individual functions:

#!/bin/bash
# Description: Check if any tests import individual functions rather than using the class

rg -nP --type=ts -C3 'import\s*\{[^}]*(createEscrow|getEscrowStatus|fundEscrow|releaseEscrow|cancelEscrow)[^}]*\}\s*from.*blockchain/trustlessWork'
apps/backend/tests/fixtures/booking.fixtures.ts (2)

379-409: Consider adding return type validation.

The function casts the Supabase response to TestUser without validation (line 408). If the database schema evolves or returns unexpected fields, tests may pass with incorrect data.

Consider adding a runtime type check or using a validation library:

- return data as TestUser;
+ if (!data || !data.id || !data.email) {
+   throw new Error('Invalid user data returned from Supabase');
+ }
+ return data as TestUser;

Note: The silent failure for profile creation (lines 398-406) is acceptable for test fixtures, but ensure tests don't rely on profile data without explicitly verifying it exists.


415-449: Consider adding return type validation.

Similar to createTestUser, this function casts the Supabase response to TestProperty without validation (line 448). Consider adding basic validation to catch schema mismatches early.

Apply this validation pattern:

- return data as TestProperty;
+ if (!data || !data.id || !data.owner_id) {
+   throw new Error('Invalid property data returned from Supabase');
+ }
+ return data as TestProperty;
PROJECT_SUMMARY.md (1)

199-207: Add language identifier to code fence for consistency.

Consider adding a language identifier to the fenced code block to align with markdown best practices.

Apply this diff:

-```
+```text
 stellar-rent/
 ├── apps/
 │   ├── web/          # Next.js frontend
 │   ├── backend/      # Node.js/Express API
 │   └── stellar-contracts/  # Soroban smart contracts
 ├── assets/           # Media files and branding
 └── docs/            # Documentation

</blockquote></details>
<details>
<summary>apps/web/src/hooks/useUserRole.tsx (1)</summary><blockquote>

`39-47`: **Consider explicit handling for rejected/suspended host states.**

Currently, `pending`, `rejected`, and `suspended` host statuses all result in `guest` role with no differentiation. Consider adding explicit state handling to provide better UX feedback:

- **pending**: Show verification in progress
- **rejected**: Explain why host application was denied
- **suspended**: Display suspension reason and appeal process

This could be implemented by extending `RoleInfo` to expose `hostStatus` for UI components to render appropriate messaging:

```typescript
// Already available in RoleInfo
setRoleInfo({
  role,
  hostStatus, // ← UI can use this for status-specific messages
  canAccessHostDashboard,
  hasProperties: storedHasProperties,
});

Then in UI components:

const { hostStatus } = useUserRole();

if (hostStatus === 'rejected') {
  return <RejectedHostMessage />;
}
if (hostStatus === 'suspended') {
  return <SuspendedHostMessage />;
}
if (hostStatus === 'pending') {
  return <PendingVerificationMessage />;
}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57b32c1 and 333b6bd.

⛔ Files ignored due to path filters (1)
  • apps/web/public/stellarrentlogo.png is excluded by !**/*.png
📒 Files selected for processing (19)
  • MEDIA_ASSETS_INVENTORY.md (1 hunks)
  • PROJECT_SUMMARY.md (1 hunks)
  • apps/backend/README.md (1 hunks)
  • apps/backend/package.json (1 hunks)
  • apps/backend/src/controllers/booking.controller.ts (5 hunks)
  • apps/backend/src/routes/booking.routes.ts (1 hunks)
  • apps/backend/src/types/booking.types.ts (1 hunks)
  • apps/backend/tests/fixtures/booking.fixtures.ts (2 hunks)
  • apps/backend/tests/integration/booking-flow.int.test.ts (1 hunks)
  • apps/backend/tests/mocks/blockchain.mocks.ts (1 hunks)
  • apps/backend/tests/setup.ts (1 hunks)
  • apps/backend/tests/utils/booking-test.utils.ts (1 hunks)
  • apps/web/src/app/page.tsx (1 hunks)
  • apps/web/src/components/guards/RoleGuard.tsx (1 hunks)
  • apps/web/src/components/search/PropertyGrid.tsx (5 hunks)
  • apps/web/src/hooks/useUserRole.tsx (1 hunks)
  • apps/web/src/lib/data/properties.ts (6 hunks)
  • apps/web/src/types/roles.ts (1 hunks)
  • task.md (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
apps/backend/src/routes/booking.routes.ts (2)
apps/backend/src/validators/booking.validator.ts (1)
  • validateConfirmPayment (6-28)
apps/backend/src/controllers/booking.controller.ts (1)
  • confirmPayment (174-247)
apps/web/src/components/guards/RoleGuard.tsx (2)
apps/web/src/types/roles.ts (1)
  • UserRole (2-2)
apps/web/src/hooks/useUserRole.tsx (1)
  • useUserRole (7-58)
apps/backend/tests/mocks/blockchain.mocks.ts (1)
apps/backend/src/blockchain/trustlessWork.ts (1)
  • TrustlessWorkClient (75-212)
apps/backend/tests/fixtures/booking.fixtures.ts (2)
apps/backend/src/config/supabase.ts (1)
  • supabase (400-420)
apps/backend/tests/utils/booking-test.utils.ts (1)
  • generateAuthToken (193-195)
apps/backend/src/controllers/booking.controller.ts (2)
apps/backend/src/types/booking.types.ts (1)
  • BookingParamsSchema (51-53)
apps/backend/src/types/shared.types.ts (1)
  • formatErrorResponse (43-47)
apps/backend/tests/integration/booking-flow.int.test.ts (3)
apps/backend/src/middleware/rateLimiter.ts (1)
  • rateLimiter (3-12)
apps/backend/src/middleware/error.middleware.ts (1)
  • errorMiddleware (10-41)
apps/backend/tests/mocks/blockchain.mocks.ts (1)
  • mockedSoroban (38-40)
apps/web/src/hooks/useUserRole.tsx (3)
apps/web/src/types/roles.ts (2)
  • RoleInfo (6-11)
  • UserRole (2-2)
apps/web/src/hooks/auth/use-auth.tsx (1)
  • useAuth (217-217)
apps/web/src/services/authService.ts (1)
  • isAuthenticated (86-105)
🪛 markdownlint-cli2 (0.18.1)
MEDIA_ASSETS_INVENTORY.md

127-127: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

PROJECT_SUMMARY.md

8-8: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


199-199: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (16)
apps/backend/package.json (1)

13-13: LGTM!

The integration test script is well-configured with an appropriate 60-second timeout for end-to-end tests.

apps/backend/tests/setup.ts (1)

6-7: LGTM!

Importing mocks before SUT is the correct approach to ensure blockchain services are properly mocked during test execution.

apps/backend/src/routes/booking.routes.ts (1)

36-37: LGTM!

The PUT method is more semantically appropriate for an idempotent confirmation operation. Retaining the POST route for backward compatibility is reasonable during the transition period.

apps/backend/src/controllers/booking.controller.ts (1)

115-160: LGTM!

The granular error handling provides clear, actionable error responses with appropriate HTTP status codes for different failure scenarios.

apps/backend/tests/integration/booking-flow.int.test.ts (2)

146-182: LGTM!

The idempotency test correctly verifies that replaying the same payment confirmation is safe and returns the same result, which is essential for reliable payment processing.


17-48: LGTM!

The test app setup comprehensively mirrors the production configuration with all middleware, routes, and error handling, ensuring integration tests run in a realistic environment.

apps/backend/tests/utils/booking-test.utils.ts (1)

10-47: LGTM!

The mock restructuring correctly adapts from Jest-style to Bun-style nested mocks, with proper chaining for in, match, and single methods that align with Bun's test API.

apps/backend/README.md (1)

149-164: Integration Tests section appears only once
Only one occurrence found at line 149; no duplicates.

apps/backend/tests/mocks/blockchain.mocks.ts (2)

1-16: LGTM!

The mock object structure correctly mirrors the TrustlessWork client interface with Jest mock functions for all escrow operations and transaction verification.


42-50: LGTM! Good practice using jest.requireActual.

Using jest.requireActual to preserve other exports from the real Soroban module while selectively mocking verifyStellarTransaction is the recommended approach for partial module mocking.

task.md (1)

1-47: Documentation provides clear test requirements.

This task description effectively outlines the integration test scope, including critical scenarios like race conditions, payment confirmation flows, and security checks. The acceptance criteria are well-defined and measurable.

apps/web/src/app/page.tsx (1)

13-13: LGTM!

The logo asset reference has been standardized to match the documented branding asset name.

apps/web/src/components/search/PropertyGrid.tsx (1)

26-26: LGTM!

The consolidation of property images to use a smaller set of assets (house.webp and house1.webp) reduces repository bloat while maintaining functionality. This aligns with the asset optimization documented in the PR.

Also applies to: 41-41, 56-56, 71-71, 116-116

apps/web/src/lib/data/properties.ts (1)

12-17: LGTM!

The image array consolidation in MOCK_PROPERTIES aligns with the asset standardization effort, using only house.webp and house1.webp while preserving the data structure and functionality.

Also applies to: 70-70, 117-117, 156-156, 200-200, 239-239

apps/web/src/types/roles.ts (1)

1-11: LGTM!

The role types are well-structured. The UserRole union covers the progression from guest to host to dual-role users, HostStatus encompasses the complete verification lifecycle, and RoleInfo provides clear flags for access control decisions.

MEDIA_ASSETS_INVENTORY.md (1)

1-180: LGTM!

This comprehensive asset inventory provides excellent documentation for media file management, usage guidelines, and optimization strategies. The structure is clear and will help maintain consistency across the project.

Comment on lines 451 to 455
export const generateAuthToken = (user: { id: string; email: string }): string => {
const jwt = require('jsonwebtoken');
const secret = process.env.JWT_SECRET || 'test-secret-key';
return jwt.sign({ id: user.id, email: user.email }, secret, { expiresIn: '1h' });
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Eliminate code duplication and use consistent import style.

This function duplicates the logic from generateTestJWT (lines 329-333) and uses require() instead of ES6 imports, which is inconsistent with the file's import statement on line 1.

Apply this diff to use the existing function and fix the import:

+import jwt from 'jsonwebtoken';
+
 // ... existing code ...

-export const generateAuthToken = (user: { id: string; email: string }): string => {
-  const jwt = require('jsonwebtoken');
-  const secret = process.env.JWT_SECRET || 'test-secret-key';
-  return jwt.sign({ id: user.id, email: user.email }, secret, { expiresIn: '1h' });
-};
+export const generateAuthToken = (user: { id: string; email: string }): string => {
+  return generateTestJWT(user.id, user.email);
+};

Alternatively, if you prefer to keep both functions separate, at least extract the shared logic:

+import jwt from 'jsonwebtoken';
+
+const JWT_SECRET = process.env.JWT_SECRET || 'test-secret-key';
+
+const createJWT = (userId: string, email: string, expiresIn: string): string => {
+  return jwt.sign({ id: userId, email }, JWT_SECRET, { expiresIn });
+};
+
 export const generateTestJWT = (userId: string, email: string): string => {
-  const jwt = require('jsonwebtoken');
-  const secret = process.env.JWT_SECRET || 'test-secret-key';
-  return jwt.sign({ id: userId, email }, secret, { expiresIn: '1h' });
+  return createJWT(userId, email, '1h');
 };

 export const generateExpiredJWT = (userId: string, email: string): string => {
-  const jwt = require('jsonwebtoken');
-  const secret = process.env.JWT_SECRET || 'test-secret-key';
-  return jwt.sign({ id: userId, email }, secret, { expiresIn: '-1h' });
+  return createJWT(userId, email, '-1h');
 };

 // ... later in file ...

 export const generateAuthToken = (user: { id: string; email: string }): string => {
-  const jwt = require('jsonwebtoken');
-  const secret = process.env.JWT_SECRET || 'test-secret-key';
-  return jwt.sign({ id: user.id, email: user.email }, secret, { expiresIn: '1h' });
+  return createJWT(user.id, user.email, '1h');
 };
🤖 Prompt for AI Agents
In apps/backend/tests/fixtures/booking.fixtures.ts around lines 451 to 455, the
generateAuthToken function duplicates generateTestJWT and uses require() instead
of ES module imports; replace this duplication by either calling the existing
generateTestJWT(user) (preferred) or extracting shared JWT generation into a
helper used by both, and change the require('jsonwebtoken') usage to an ES6
import at the top of the file (or import the helper) so imports are consistent
and duplication is removed.

Comment on lines 59 to 87
const bookingData = {
propertyId: 'test-property-id',
userId: 'test-user-id',
dates: { from: '2024-06-01', to: '2024-06-03' },
guests: 2,
total: 200,
deposit: 50,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix date format in test data.

The createBookingSchema at apps/backend/src/types/booking.types.ts (lines 36-42) expects dates.from and dates.to to be Date objects, not strings. The test data uses string dates which will fail schema validation.

Apply this diff to fix the date format:

    const bookingData = {
      propertyId: 'test-property-id',
      userId: 'test-user-id',
-     dates: { from: '2024-06-01', to: '2024-06-03' },
+     dates: { 
+       from: new Date('2024-06-01'), 
+       to: new Date('2024-06-03') 
+     },
      guests: 2,
      total: 200,
      deposit: 50,
    };

Apply the same fix to the test data at:

  • Lines 77-84
  • Lines 106-112
  • Lines 148-154
  • Lines 186-193
📝 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.

Suggested change
const bookingData = {
propertyId: 'test-property-id',
userId: 'test-user-id',
dates: { from: '2024-06-01', to: '2024-06-03' },
guests: 2,
total: 200,
deposit: 50,
};
const bookingData = {
propertyId: 'test-property-id',
userId: 'test-user-id',
dates: {
from: new Date('2024-06-01'),
to: new Date('2024-06-03')
},
guests: 2,
total: 200,
deposit: 50,
};
🤖 Prompt for AI Agents
In apps/backend/tests/integration/booking-flow.int.test.ts around lines 59-66
(and also update the bookingData occurrences at lines 77-84, 106-112, 148-154,
186-193), the test uses string values for dates.from and dates.to but
createBookingSchema expects Date objects; change each bookingData.dates.from and
bookingData.dates.to from string literals to actual Date objects (e.g., new
Date('2024-06-01') and new Date('2024-06-03')) for every mentioned block so the
test data matches the schema.

Comment on lines 7 to 53
interface RoleGuardProps {
children: React.ReactNode;
requiredRole: UserRole;
fallbackPath?: string;
}

export function RoleGuard({
children,
requiredRole,
fallbackPath = '/become-host',
}: RoleGuardProps) {
const { canAccessHostDashboard } = useUserRole();
const router = useRouter();

if (requiredRole === 'host' && !canAccessHostDashboard) {
return (
<div className="flex flex-col items-center justify-center min-h-screen p-4">
<div className="text-center max-w-md">
<h2 className="text-2xl font-bold mb-4">Host Access Required</h2>
<p className="text-gray-600 mb-6">You need to become a host to access this page.</p>
<button
type="button"
onClick={() => router.push(fallbackPath)}
className="bg-blue-600 text-white px-6 py-3 rounded-lg hover:bg-blue-700"
>
Become a Host
</button>
</div>
</div>
);
}

return <>{children}</>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Incomplete role checking logic.

The RoleGuardProps interface accepts UserRole ('guest' | 'host' | 'dual'), but only the 'host' role is checked at Line 21. Requests for 'dual' or 'guest' roles are not explicitly handled, which could lead to incorrect access control behavior.

Consider one of these approaches:

Option 1: Implement complete role checking

-  if (requiredRole === 'host' && !canAccessHostDashboard) {
+  const { role, canAccessHostDashboard } = useUserRole();
+  
+  // Check access based on required role
+  const hasAccess = 
+    requiredRole === 'guest' ? true : // guests can access guest pages
+    requiredRole === 'host' ? canAccessHostDashboard : // hosts need dashboard access
+    requiredRole === 'dual' ? role === 'dual' && canAccessHostDashboard : // dual needs both
+    false;
+  
+  if (!hasAccess) {
     return (
       <div className="flex flex-col items-center justify-center min-h-screen p-4">
         <div className="text-center max-w-md">
-          <h2 className="text-2xl font-bold mb-4">Host Access Required</h2>
-          <p className="text-gray-600 mb-6">You need to become a host to access this page.</p>
+          <h2 className="text-2xl font-bold mb-4">
+            {requiredRole === 'host' ? 'Host Access Required' : 'Access Denied'}
+          </h2>
+          <p className="text-gray-600 mb-6">
+            {requiredRole === 'host' 
+              ? 'You need to become a host to access this page.'
+              : `You need ${requiredRole} access to view this page.`}
+          </p>
           <button
             type="button"
             onClick={() => router.push(fallbackPath)}
             className="bg-blue-600 text-white px-6 py-3 rounded-lg hover:bg-blue-700"
           >
-            Become a Host
+            {requiredRole === 'host' ? 'Become a Host' : 'Go Back'}
           </button>
         </div>
       </div>
     );
   }

Option 2: Restrict the interface to only supported roles

 interface RoleGuardProps {
   children: React.ReactNode;
-  requiredRole: UserRole;
+  requiredRole: 'host'; // Only support host for now
   fallbackPath?: string;
 }

If this is intentional for incremental development, please document the current limitations and create a TODO or issue to track complete implementation.


🏁 Script executed:

#!/bin/bash
set -e
echo "=== UserRole definitions ==="
rg -n "export type UserRole" -g "*.ts*" -C3

echo "=== useUserRole hook implementation ==="
rg -n "function useUserRole" -g "*.ts*" -C5 || echo "No direct function, searching hook imports..."
rg -n "useUserRole" -g "*.ts*" -C5

Length of output: 3491


🏁 Script executed:

#!/bin/bash
set -e
echo "=== apps/web/src/types/roles.ts ==="
sed -n '1,200p' apps/web/src/types/roles.ts

echo
echo "=== apps/web/src/hooks/useUserRole.tsx ==="
sed -n '1,200p' apps/web/src/hooks/useUserRole.tsx

Length of output: 2156


Enforce access for ‘dual’ roles in RoleGuard.
The guard currently only checks requiredRole === 'host', so pages with requiredRole='dual' remain unprotected. Expand the access check to include dual users, for example:

const { canAccessHostDashboard } = useUserRole();

if (
  (requiredRole === 'host' || requiredRole === 'dual') &&
  !canAccessHostDashboard
) {
  // …
}
🤖 Prompt for AI Agents
In apps/web/src/components/guards/RoleGuard.tsx around lines 7 to 39, the guard
only checks for requiredRole === 'host' so pages requiring 'dual' remain
unprotected; update the access check to treat 'dual' the same as 'host' (e.g.,
change the if condition to check (requiredRole === 'host' || requiredRole ===
'dual') && !canAccessHostDashboard) so users lacking host privileges are blocked
and shown the fallback UI; keep the rest of the JSX and routing behavior the
same.

Comment on lines 25 to 34
// Check if user has host status in localStorage or from API
const storedHostStatus = localStorage.getItem('hostStatus');
const storedHasProperties = localStorage.getItem('hasProperties') === 'true';

// Validate hostStatus
const validHostStatuses = ['pending', 'verified', 'rejected', 'suspended'];
const hostStatus =
storedHostStatus && validHostStatuses.includes(storedHostStatus)
? (storedHostStatus as 'pending' | 'verified' | 'rejected' | 'suspended')
: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify backend authorization for protected routes.

The hook derives access permissions from localStorage (hostStatus, hasProperties), which can be manipulated client-side. Ensure that all protected backend endpoints (property management, bookings, payments, etc.) perform server-side authorization checks independent of client-side role state.

Run this script to check for authorization middleware in backend routes:


🏁 Script executed:

#!/bin/bash
# Check for auth middleware patterns in backend routes

echo "=== Checking for authorization middleware in backend routes ==="

# Look for route protection patterns
rg -n --type=ts -C3 'router\.(get|post|put|delete|patch)' apps/backend/src/routes/ | \
  rg -C2 '(auth|authorize|verify|middleware|protect|guard)'

echo ""
echo "=== Checking for host-specific authorization ==="

# Look for host role checks in middleware or route handlers
ast-grep --pattern $'if ($COND) {
  $$$
  hostStatus
  $$$
}'

rg -n --type=ts -C3 'hostStatus|canAccessHostDashboard|requiredRole' apps/backend/

Length of output: 5799


Enforce host role on sensitive backend routes. Property and booking endpoints (e.g. apps/backend/src/routes/property.route.ts, booking.routes.ts) use only authenticateToken, allowing any authenticated user to perform host actions. Add host‐role authorization middleware (e.g. authorizeHost) to enforce that only verified hosts can create, update, or manage properties and bookings.

🤖 Prompt for AI Agents
In apps/web/src/hooks/useUserRole.tsx around lines 25 to 34, the review flag
notes backend routes lack host-role enforcement; update the backend instead: add
a new authorizeHost middleware that checks the authenticated user's
role/hostStatus and rejects requests unless hostStatus === 'verified' (or role
=== 'host' plus verified), then apply this middleware to property and booking
routes (apps/backend/src/routes/property.route.ts and booking.routes.ts) on
endpoints that create, update, or manage properties/bookings; ensure the
middleware returns proper 403 responses and is used after authenticateToken so
only authenticated+verified hosts can perform those actions.

@respp respp merged commit 4beef96 into main Oct 10, 2025
1 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants