fix: blockchain sync service #205
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughA trailing newline is added to the RoleGuard component file. No functional modifications to the component logic, rendering, access control, or UI behavior are introduced. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
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/tests/sync.service.test.ts (1)
450-470: This error-path test is now stale (SyncService no longer usesfetch).
getCurrentBlockHeightuses CLI/SDK, sofetchwon’t be called and the test won’t exercise the error path. Mock CLI/SDK failures instead (e.g., mocknode:child_processto throw andserver.getLatestLedgerto reject) so the test tracks the current behavior.
🤖 Fix all issues with AI agents
In `@apps/backend/src/blockchain/eventListener.ts`:
- Around line 1-2: Reorder the imports so third-party package imports come
before local imports and sort the named exports alphabetically: replace "import
{ Contract, Networks, xdr, rpc } from '@stellar/stellar-sdk'; import { supabase
} from '../config/supabase';" with the third‑party import first and the local
import after a blank line, and alphabetize the named imports for the stellar SDK
(Contract, Networks, rpc, xdr) so the linter passes.
In `@apps/backend/src/services/sync.service.ts`:
- Around line 21-22: The import statement from '@stellar/stellar-sdk' has its
named imports out of alphabetical order which fails the linter; reorder the
named imports to be alphabetically sorted (Contract, Networks, nativeToScVal,
rpc, scValToNative) and keep the third-party import group before the local
import (supabase) with a separating blank line to satisfy the CI import-order
rules.
- Around line 418-500: The error check in getLatestLedgerFromCLI compares the
spawned process exit code using err.code === '1' which fails because err.code is
a number; update the check to compare numerically (e.g., err.code === 1) or
normalize it (Number(err.code) === 1 or String(err.code) === '1') and adjust the
conditional that checks for network-related failures (the block referencing
err.code and err.message) so it correctly detects non-zero exit codes from the
Stellar CLI and logs/throws the expected network error.
🧹 Nitpick comments (1)
apps/backend/src/blockchain/eventListener.ts (1)
306-326: Reuse the existing logId on errors to avoid orphaned logs.
logBlockchainOperationalready runs in the try path, but the catch path creates a second log entry and leaves the original without success/error. Reuse the original logId (and guard logging failures) so each operation is finalized once.♻️ Suggested refactor
- try { - const logId = await loggingService.logBlockchainOperation('processEvent', { event }); + let logId: string | null = null; + try { + logId = await loggingService.logBlockchainOperation('processEvent', { event }); // Store event in database await this.storeEvent(event); // Call registered callbacks const callback = this.eventCallbacks.get(event.type); if (callback) { await callback(event); } // Log success await loggingService.logBlockchainSuccess(logId, { eventId: event.id }); } catch (error) { console.error(`Error processing event ${event.id}:`, error); await this.markEventFailed(event.id, error as Error); - const log = await loggingService.logBlockchainOperation('processEvent', { eventId: event.id }); - await loggingService.logBlockchainError(log, error); + try { + if (logId) { + await loggingService.logBlockchainError(logId, error as Error); + } else { + const log = await loggingService.logBlockchainOperation('processEvent', { eventId: event.id }); + await loggingService.logBlockchainError(log, error as Error); + } + } catch (logError) { + console.error('Failed to record blockchain error:', logError); + } }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/backend/src/tests/sync.service.test.ts (3)
150-155: Mixing Jest APIs with Bun test framework will cause failures.The test imports
mockfrombun:testbut usesjest.fn()for creating mock functions. In Bun's test environment,jestis not defined, which will cause runtime errors.Replace all
jest.fn()usages with Bun'smock():🐛 Proposed fix
- mockSupabase.from.mockReturnValue({ - select: jest.fn(() => ({ - single: jest.fn(() => Promise.resolve({ data: null, error: null })), - })), - } as unknown as ReturnType<typeof supabase.from>); + mockSupabase.from.mockReturnValue({ + select: mock(() => ({ + single: mock(() => Promise.resolve({ data: null, error: null })), + })), + } as unknown as ReturnType<typeof supabase.from>);This pattern needs to be applied throughout the file wherever
jest.fn()appears (lines 152-154, 166-168, 180-183, 200-212, 234-239, 243-244, 265-278, 314-318, 383-388, 470-475, 504-512, 546-550, 563-566).
479-481:jest.spyOnis not available in Bun test environment.Bun's test runner doesn't provide
jest.spyOn. Use Bun'smock()orspyOnfrombun:testinstead:🐛 Proposed fix
+import { spyOn } from 'bun:test'; + // In the test: - const mockFetch = jest - .spyOn(global, 'fetch') - .mockRejectedValueOnce(new Error('Blockchain connection failed')); + const mockFetch = spyOn(global, 'fetch').mockImplementation(() => + Promise.reject(new Error('Blockchain connection failed')) + );
541-543: Incorrect mock assignment forglobal.fetch.Using
jest.fn()here won't work in Bun. Additionally, directly assigning toglobal.fetchwithout proper cleanup can leak between tests.🐛 Proposed fix
- global.fetch = jest.fn(() => - Promise.reject(new Error('Network error')) - ) as unknown as typeof fetch; + const originalFetch = global.fetch; + global.fetch = mock(() => Promise.reject(new Error('Network error'))) as typeof fetch; + // Remember to restore in afterEach or at test end: + // global.fetch = originalFetch;apps/web/src/components/guards/RoleGuard.tsx (2)
4-8: Resolve leftover merge-conflict markers before merging.The
<<<<<<<,=======,>>>>>>>blocks will break compilation and leave two divergent implementations in the same file. Please resolve to a single version and remove the conflict markers.Also applies to: 23-71, 77-84
64-67: UsefallbackPathin the CTA for consistency.The redirect uses
fallbackPath, but the button hardcodes/become-host, which breaks custom fallbacks.🛠️ Suggested fix
- onClick={() => router.push('/become-host')} + onClick={() => router.push(fallbackPath)}
♻️ Duplicate comments (1)
apps/backend/src/services/sync.service.ts (1)
524-530: Exit code comparison uses string instead of number.When a child process exits with a non-zero status,
execpasses an error where the exit code is numeric, not a string. The comparisonerr.code === '1'will never match becauseerr.codefor syscall errors likeENOENTis a string, but for process exit codes the pattern differs—the exit code is typically onerror.codeas a number or accessed differently.This condition appears unreachable. If the intent is to detect exit code 1 with a network-related message, verify the actual error structure from
promisify(exec)when the CLI fails.♻️ Suggested fix
- if (err.code === '1' && err.message?.includes('network')) { + if ((err.code === 1 || err.code === '1') && err.message?.includes('network')) {Or remove this branch if it's not reachable in practice.
,
🧹 Nitpick comments (3)
apps/backend/src/blockchain/eventListener.ts (1)
321-328: Redundant log entry creation in the error handler.A
logIdis already created at line 308 at the start ofprocessEvent. The error handler creates a second log entry (line 324) instead of using the existinglogIdto record the error. This results in two separate log entries for the same operation—one incomplete (success path not reached) and one with just the error.Consider using the existing
logIdin the catch block:♻️ Suggested fix
} catch (error) { console.error(`Error processing event ${event.id}:`, error); await this.markEventFailed(event.id, error as Error); - const log = await loggingService.logBlockchainOperation('processEvent', { - eventId: event.id, - }); - await loggingService.logBlockchainError(log, error); + await loggingService.logBlockchainError(logId, error); }apps/backend/src/services/sync.service.ts (1)
443-539: Consider extracting the CLI command building for testability.The
getLatestLedgerFromCLImethod is well-structured with comprehensive error handling. However, the inline dynamic imports ofnode:child_processandnode:utilmake it harder to mock in tests. The tests work around this by mocking the entire module, but moving the exec call or the promisified function to a mockable property could simplify testing.apps/backend/src/tests/sync.service.test.ts (1)
576-624: Duplicate test cases.The tests "should use custom polling interval from environment variable" (lines 576-599) and "should fallback to default polling interval for invalid environment value" (lines 601-624) are duplicates of tests already defined in the "Initialization" describe block (lines 96-144).
Consider removing the duplicates to reduce test maintenance overhead.
There was a problem hiding this comment.
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 (4)
apps/web/src/components/dashboard/NotificationSystem.tsx (1)
299-311: MissingtabIndexandroleattributes for keyboard accessibility.The
onKeyDownhandler won't work for keyboard users because the<div>is not focusable withouttabIndex={0}. Additionally,role="button"should be added for screen reader users to understand this is an interactive element.Proposed fix
<div key={notification.id} + role="button" + tabIndex={0} className={`p-4 hover:bg-gray-50 dark:hover:bg-gray-800 transition-colors cursor-pointer border-l-4 ${getNotificationColor(notification.type)} ${ !notification.read ? 'bg-blue-50 dark:bg-blue-900/10' : '' }`} onClick={() => handleNotificationClick(notification)} onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); handleNotificationClick(notification); } }} >apps/web/src/components/blockchain/BlockchainStatusBadge.tsx (1)
28-53: Critical: Rules of Hooks violation — hooks called after conditional return.The early return on lines 28-31 executes before
useCallback(line 33) anduseEffect(line 49). WhenisBlockchainEnabled()returns false, these hooks are skipped, violating React's requirement that hooks be called in the same order on every render. This can cause runtime crashes or unpredictable behavior.Move all hooks before the conditional return:
🔧 Proposed fix
export function BlockchainStatusBadge({ propertyId, className = '', showIcon = true, size = 'sm', }: BlockchainStatusBadgeProps) { const [status, setStatus] = useState<PropertyBlockchainStatus | null>(null); const [isLoading, setIsLoading] = useState(false); - // Don't render if blockchain features are disabled - if (!isBlockchainEnabled()) { - return null; - } - const loadStatus = useCallback(async () => { setIsLoading(true); try { const blockchainStatus = await getPropertyBlockchainStatus(propertyId); setStatus(blockchainStatus); } catch (error) { console.error('Failed to load blockchain status:', error); setStatus({ isVerified: false, error: 'Failed to load status', }); } finally { setIsLoading(false); } }, [propertyId]); useEffect(() => { - if (propertyId) { + if (propertyId && isBlockchainEnabled()) { loadStatus(); } }, [propertyId, loadStatus]); + + // Don't render if blockchain features are disabled + if (!isBlockchainEnabled()) { + return null; + }apps/web/src/components/dashboard/PropertyCalendar.tsx (1)
312-332: Fix pipeline failure: addrole="button"for accessibility compliance.The linter correctly flags that a
<div>withtabIndex={0}is non-interactive. Adding keyboard support is excellent, but the element also needs a semantic role to be accessible.Add
role="button"to make the element semantically interactive, and include an accessible label:Suggested fix
<div key={date.toISOString()} + role="button" + aria-label={`${date.toLocaleDateString()}, ${dayBookings.length} bookings`} + aria-pressed={isSelected} onClick={() => handleDateClick(date)} onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); handleDateClick(date); } }} tabIndex={0} className={`p-2 min-h-[100px] border ...`} >This pattern (
role="button"+tabIndex={0}+onKeyDown+onClick) is the standard approach for custom button-like elements. Thearia-labelprovides context for screen reader users, andaria-pressedconveys selection state.apps/backend/src/tests/sync.service.test.ts (1)
479-481:jest.spyOnunavailable in Bun test context.Use Bun's mocking approach instead:
- const mockFetch = jest - .spyOn(global, 'fetch') - .mockRejectedValueOnce(new Error('Blockchain connection failed')); + const originalFetch = global.fetch; + global.fetch = mock(() => Promise.reject(new Error('Blockchain connection failed'))); + // ... test code ... + // Restore in afterEach or finally block + global.fetch = originalFetch;
🤖 Fix all issues with AI agents
In `@apps/backend/src/tests/sync.service.test.ts`:
- Around line 33-37: Tests in sync.service.test.ts import from 'bun:test' but
call jest.fn() and jest.spyOn(), which will fail at runtime; replace all uses of
jest.fn() and jest.spyOn() with Bun's test mocking APIs (you already import mock
from 'bun:test') or switch the whole test file to Jest. Concretely, in
sync.service.test.ts find every occurrence of jest.fn() and jest.spyOn() and:
either convert them to use the imported mock utilities (e.g., use mock to create
mocks/spies per Bun's API) or change the test runner import to Jest and remove
the bun:test import so the global jest is available; ensure functions like the
SyncService tests that currently rely on jest mocks are updated to use mock from
'bun:test' (or adjust to Jest) consistently.
- Around line 2-22: The current mock.module call creates plain functions so test
code cannot call .mockReturnValue on mockSupabase.from; change the mock to use
real mock functions (e.g., mock.fn()/jest.fn() or Bun's mock.fn) for
supabase.from and all nested method stubs (select, single, eq, order, limit,
insert, update, upsert, delete) so they expose
.mockReturnValue/.mockImplementation; alternatively, keep the module-level mock
minimal (exporting mock.fn() for from) and re-mock per-test in beforeEach by
calling mock.module('../config/supabase', ...) or replacing
mockSupabase.from.mockReturnValue(...) in tests to provide different behaviors
for lines that call mockSupabase.from.mockReturnValue.
- Around line 321-345: The current test mocks node:util.promisify as identity
which prevents exec being converted to a promise and forces the SDK fallback;
fix by changing the mock for promisify so it returns an async wrapper that calls
the original exec callback and resolves/rejects a Promise (thereby letting
execAsync(command, opts) behave like a promise), or alternatively mock the
promise-returning execAsync directly; update the mock.module('node:util') entry
so promisify((fn) => ...) returns a function that calls fn(command, options,
callback) and resolves with { stdout, stderr } or rejects with error, ensuring
getCurrentBlockHeight() exercises the CLI path instead of the SDK fallback.
In `@apps/web/src/components/dashboard/PropertyCalendar.tsx`:
- Around line 302-305: In PropertyCalendar's calendarDays.map callback, avoid
using the array index as the React key for empty cells (currently
`empty-${index}`); instead compute a stable key derived from the grid position
(e.g., use the day offset from the month start or a tuple like
`empty-${weekIndex}-${dayIndex}` you can derive from calendarDays) and replace
the `key` with that value, add aria-hidden="true" to these placeholder divs, and
if the linter still complains add an inline
eslint-disable-next-line/react/no-array-index-key comment scoped to that line.
♻️ Duplicate comments (1)
apps/backend/src/services/sync.service.ts (1)
524-530: Exit code comparison uses string instead of number.The comparison
err.code === '1'will never match because child process exit codes are numeric. When the CLI exits with code 1 for a network error,err.codewill be1(number), not'1'(string).- if (err.code === '1' && err.message?.includes('network')) { + if (err.code === 1 && err.message?.includes('network')) {Alternatively, use loose equality or convert types:
String(err.code) === '1'orerr.code == 1.
🧹 Nitpick comments (4)
apps/web/src/components/dashboard/NotificationSystem.tsx (1)
237-252: Consider usingas constto avoid the explicit type cast.Defining the tabs array with
as constwould let TypeScript infer literal types forkey, eliminating the need for the cast.Suggested improvement
- {[ + {([ { key: 'all', label: 'All' }, { key: 'unread', label: 'Unread' }, { key: 'booking', label: 'Booking' }, { key: 'payment', label: 'Payment' }, { key: 'review', label: 'Review' }, { key: 'system', label: 'System' }, - ].map((tab) => ( + ] as const).map((tab) => ( <button key={tab.key} type="button" - onClick={() => - setFilter( - tab.key as 'all' | 'unread' | 'booking' | 'payment' | 'review' | 'system' - ) - } + onClick={() => setFilter(tab.key)}apps/web/src/components/blockchain/BlockchainStatusBadge.tsx (1)
131-148: Same Rules of Hooks violation exists inBlockchainStatusIcon.This component has the identical pattern: an early return at lines 131-133 before
useEffectat line 135. Although not modified in this PR, consider fixing it alongside theBlockchainStatusBadgechanges for consistency.♻️ Proposed fix
export function BlockchainStatusIcon({ propertyId, className = '', size = 16, }: { propertyId: string; className?: string; size?: number; }) { const [status, setStatus] = useState<PropertyBlockchainStatus | null>(null); - if (!isBlockchainEnabled()) { - return null; - } - useEffect(() => { const loadStatus = async () => { try { const blockchainStatus = await getPropertyBlockchainStatus(propertyId); setStatus(blockchainStatus); } catch (error) { console.error('Failed to load blockchain status:', error); } }; - if (propertyId) { + if (propertyId && isBlockchainEnabled()) { loadStatus(); } }, [propertyId]); + + if (!isBlockchainEnabled()) { + return null; + }apps/backend/src/tests/sync.service.test.ts (2)
359-360: Unused variable declaration.
_initialLastProcessedBlockis declared but never used. Remove it to reduce noise.const initialStatus = syncService.getStatus(); - const _initialLastProcessedBlock = initialStatus.lastProcessedBlock;
576-624: Duplicate test cases.These two tests at the file's root level are duplicates of tests already defined within the
Initializationdescribe block (lines 96-119 and 121-144). Remove the duplicates to avoid redundant test execution.- it('should use custom polling interval from environment variable', () => { - // ... identical to lines 96-119 - }); - - it('should fallback to default polling interval for invalid environment value', () => { - // ... identical to lines 121-144 - }); });
| mock.module('../config/supabase', () => ({ | ||
| supabase: mockSupabase, | ||
| supabase: { | ||
| from: () => ({ | ||
| select: () => ({ | ||
| single: () => Promise.resolve({ data: null, error: null }), | ||
| eq: () => ({ | ||
| single: () => Promise.resolve({ data: null, error: null }), | ||
| }), | ||
| order: () => ({ | ||
| limit: () => Promise.resolve({ data: [], error: null }), | ||
| }), | ||
| }), | ||
| insert: () => Promise.resolve({ data: null, error: null }), | ||
| update: () => ({ | ||
| eq: () => Promise.resolve({ data: null, error: null }), | ||
| }), | ||
| upsert: () => Promise.resolve({ data: null, error: null }), | ||
| delete: () => Promise.resolve({ data: null, error: null }), | ||
| }), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
Mock structure incompatible with test usage.
The supabase mock is defined as a plain object with regular functions, but tests attempt to call .mockReturnValue() on mockSupabase.from (lines 151, 166, 179, etc.). Plain functions don't have Jest's mock methods.
Either convert to proper Bun mocks with tracking:
mock.module('../config/supabase', () => ({
- supabase: {
- from: () => ({
- select: () => ({
+ supabase: {
+ from: mock(() => ({
+ select: mock(() => ({Or restructure tests to re-mock the module before each test case that needs different behavior.
🤖 Prompt for AI Agents
In `@apps/backend/src/tests/sync.service.test.ts` around lines 2 - 22, The current
mock.module call creates plain functions so test code cannot call
.mockReturnValue on mockSupabase.from; change the mock to use real mock
functions (e.g., mock.fn()/jest.fn() or Bun's mock.fn) for supabase.from and all
nested method stubs (select, single, eq, order, limit, insert, update, upsert,
delete) so they expose .mockReturnValue/.mockImplementation; alternatively, keep
the module-level mock minimal (exporting mock.fn() for from) and re-mock
per-test in beforeEach by calling mock.module('../config/supabase', ...) or
replacing mockSupabase.from.mockReturnValue(...) in tests to provide different
behaviors for lines that call mockSupabase.from.mockReturnValue.
| import { afterEach, beforeEach, describe, expect, it, mock } from 'bun:test'; | ||
| import type { ExecOptions } from 'node:child_process'; | ||
| import type { rpc } from '@stellar/stellar-sdk'; | ||
| import { supabase } from '../config/supabase'; | ||
| import { SyncService, syncService } from '../services/sync.service'; |
There was a problem hiding this comment.
Critical: Mixing bun:test with Jest APIs will cause runtime failures.
The file imports from bun:test but uses jest.fn() (lines 152, 167, etc.) and jest.spyOn() (line 479) throughout the tests. Bun's test runner does not provide a global jest object, so these calls will throw ReferenceError: jest is not defined at runtime.
Use Bun's mock function consistently:
- mockSupabase.from.mockReturnValue({
- select: jest.fn(() => ({
- single: jest.fn(() => Promise.resolve({ data: null, error: null })),
+ mock.module('../config/supabase', () => ({
+ supabase: {
+ from: mock(() => ({
+ select: mock(() => ({
+ single: mock(() => Promise.resolve({ data: null, error: null })),
+ })),
+ })),
+ },
+ }));Alternatively, switch entirely to Jest if that's the intended test runner.
🤖 Prompt for AI Agents
In `@apps/backend/src/tests/sync.service.test.ts` around lines 33 - 37, Tests in
sync.service.test.ts import from 'bun:test' but call jest.fn() and jest.spyOn(),
which will fail at runtime; replace all uses of jest.fn() and jest.spyOn() with
Bun's test mocking APIs (you already import mock from 'bun:test') or switch the
whole test file to Jest. Concretely, in sync.service.test.ts find every
occurrence of jest.fn() and jest.spyOn() and: either convert them to use the
imported mock utilities (e.g., use mock to create mocks/spies per Bun's API) or
change the test runner import to Jest and remove the bun:test import so the
global jest is available; ensure functions like the SyncService tests that
currently rely on jest mocks are updated to use mock from 'bun:test' (or adjust
to Jest) consistently.
aguilar1x
force-pushed
the
fix/blockchain-sync-service
branch
2 times, most recently
from
a2a344c to
60fbb2e
Compare
aguilar1x
force-pushed
the
fix/blockchain-sync-service
branch
from
60fbb2e to
7c83ae9
Compare
- Organize imports in backend files (eventListener.ts, sync.service.ts) - Add missing 'rpc' import from @stellar/stellar-sdk - Resolve git merge conflict in roles.ts type definition - All backend files now pass Biome formatting and import organization rules
…t#196) - Add Stellar CLI command 'stellar ledger latest' as fallback method - Implement robust error handling for malformed JSON responses - Add comprehensive fallback mechanism when RPC endpoint is unavailable - SDK method (getLatestLedger) as primary, CLI method as secondary - Enhanced error handling with specific error types and timeouts - Verified CLI availability and functionality on testnet Fixes Stellar-Rent#196: Blockchain Sync Service - Use Correct Stellar RPC Endpoint
- Organize imports in sync.service.ts (node modules first, then stellar SDK) - Fix optional chaining usage in CLI stderr handling - Apply automatic code formatting to fix long lines - All backend files now pass Biome linting with 0 errors, 0 warnings Resolves linting issues identified in sync.service.ts
|
Hey! I need help with CI/CD, as it's throwing lint errors in the web directories, which isn't related to what I did in the issue. |
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 (3)
apps/web/src/components/blockchain/BlockchainStatusBadge.tsx (2)
29-53: Rules of Hooks violation: conditional return before hooks.The early return at lines 29-31 executes before
useCallback(line 33) anduseEffect(line 49) are called. React requires hooks to be called unconditionally and in the same order on every render. This will cause runtime errors or unpredictable behavior whenisBlockchainEnabled()returns false.Move the conditional return after all hooks, or guard the effect's body instead.
🐛 Proposed fix
export function BlockchainStatusBadge({ propertyId, className = '', showIcon = true, size = 'sm', }: BlockchainStatusBadgeProps) { const [status, setStatus] = useState<PropertyBlockchainStatus | null>(null); const [isLoading, setIsLoading] = useState(false); + const blockchainEnabled = isBlockchainEnabled(); - // Don't render if blockchain features are disabled - if (!isBlockchainEnabled()) { - return null; - } - const loadStatus = useCallback(async () => { setIsLoading(true); try { const blockchainStatus = await getPropertyBlockchainStatus(propertyId); setStatus(blockchainStatus); } catch (error) { console.error('Failed to load blockchain status:', error); setStatus({ isVerified: false, error: 'Failed to load status', }); } finally { setIsLoading(false); } }, [propertyId]); useEffect(() => { - if (propertyId) { + if (blockchainEnabled && propertyId) { loadStatus(); } - }, [propertyId, loadStatus]); + }, [blockchainEnabled, propertyId, loadStatus]); + + // Don't render if blockchain features are disabled + if (!blockchainEnabled) { + return null; + } if (isLoading) {
131-148: Same Rules of Hooks violation inBlockchainStatusIcon.The conditional return at lines 131-133 precedes the
useEffectat line 135. This component has the same issue asBlockchainStatusBadgeand needs the same fix: move the early return after the hook.🐛 Proposed fix
export function BlockchainStatusIcon({ propertyId, className = '', size = 16, }: { propertyId: string; className?: string; size?: number; }) { const [status, setStatus] = useState<PropertyBlockchainStatus | null>(null); + const blockchainEnabled = isBlockchainEnabled(); - if (!isBlockchainEnabled()) { - return null; - } - useEffect(() => { const loadStatus = async () => { try { const blockchainStatus = await getPropertyBlockchainStatus(propertyId); setStatus(blockchainStatus); } catch (error) { console.error('Failed to load blockchain status:', error); } }; - if (propertyId) { + if (blockchainEnabled && propertyId) { loadStatus(); } - }, [propertyId]); + }, [blockchainEnabled, propertyId]); + + if (!blockchainEnabled) { + return null; + } if (!status) {apps/web/src/components/guards/RoleGuard.tsx (1)
55-58: UsefallbackPathfor the unauthorized CTA.The redirect uses
fallbackPath, but the button always pushes/become-host, which breaks custom fallback targets.🐛 Proposed fix
- onClick={() => router.push('/become-host')} + onClick={() => router.push(fallbackPath)}
🤖 Fix all issues with AI agents
In `@biome.json`:
- Around line 37-41: Re-enable the suspicious rules noExplicitAny and
noArrayIndexKey in the biome.json rules block (remove or change "off"), then fix
code that violates them: replace inappropriate uses of any by adding proper
types or generics (search for occurrences of explicit "any" and update function
signatures/types) and replace array-index keys in React components with stable
unique keys (search for map callbacks using index as key and use id or generate
stable keys). If you cannot fix all violations immediately, restore the rules
but add a TODO comment referencing an issue/id in the codebase near each
temporary suppression and create the tracking issue so these rules can be
re-enabled later.
- Around line 42-44: The biome.json currently disables the entire accessibility
category via the "a11y" -> "all": false setting which removes all accessibility
linting; restore accessibility checks by removing or changing that "a11y" all
flag (e.g., set it to true or remove the key) and instead disable only
individual rules causing problems by listing them under "a11y": { "rules": {
"<rule-name>": "off", ... } } or apply inline suppression comments in the
specific legacy files; reference the "a11y" key in biome.json and the specific
rule names you intend to exempt when making the change.
🧹 Nitpick comments (6)
apps/web/src/components/booking/BookingForm.tsx (1)
31-31: Consider removing unused destructuring or implementing wallet validation.The underscore-prefixed variables
_isConnectedand_publicKeyare destructured but never used. If suppressing linter warnings is the only goal, consider removing the destructuring entirely:- const { isConnected: _isConnected, publicKey: _publicKey } = useWallet(); + const { } = useWallet();Or simply don't call the hook if it's unused:
- const { isConnected: _isConnected, publicKey: _publicKey } = useWallet();Alternatively, if wallet connection should be validated before booking submission, consider using these values:
const handleSubmit = async () => { + if (!_isConnected) { + toast.error('Please connect your wallet first'); + return; + } if (!selectedDates?.from || !selectedDates?.to) {apps/web/src/components/search/SearchBar.tsx (1)
11-22: Consider removing commented-out dead code.With the
zodimport now removed, this commented-out schema validation code is truly dead and cannot be restored without re-adding the dependency. If there's no plan to reintroduce validation using this schema, consider removing it to reduce noise.biome.json (3)
9-19: Glob patterns and file ignores look appropriate, but clarify the specific file exclusion.The recursive glob patterns (
**/) are more comprehensive and idiomatic. However, the explicit ignore ofapps/backend/src/config/supabase.tsshould have a comment explaining why this file needs to be excluded from linting—otherwise future maintainers won't understand the reasoning.
45-47: DisablinguseImportTypeis acceptable but consider keeping it enabled.The
useImportTyperule encourages usingimport typefor type-only imports, which can improve tree-shaking and clarify intent. Disabling it is a style choice, but it's generally considered a best practice in TypeScript projects.
1-58: Overall: Config changes are overly broad for the PR's scope.Based on the PR objectives, this PR is meant to fix the blockchain sync service endpoint issues. However, these Biome configuration changes affect the entire monorepo and appear designed to suppress lint errors rather than fix the underlying code issues.
Consider:
- Keeping the schema upgrade and ignore pattern improvements.
- Re-enabling
a11yrules (or disable only specific problematic rules).- Re-enabling
noExplicitAnyandnoArrayIndexKey, and fixing the actual code violations.- If time constraints require temporarily disabling rules, document this with TODO comments and create follow-up issues to address the technical debt.
apps/web/src/components/guards/RoleGuard.tsx (1)
22-38: SimplifyisCheckingto avoid an extra render/spinner.
isCheckingjust mirrorsisLoading, and the state update causes a needless extra render (spinner flash) when data is already loaded. Consider deriving it directly.♻️ Proposed simplification
- const [isChecking, setIsChecking] = useState(true); + const isChecking = isLoading; useEffect(() => { // Wait for role data to load if (!isLoading) { - setIsChecking(false); - // Redirect if user doesn't have required access if (requiredRole === 'host' && !canAccessHostDashboard) { router.push(fallbackPath); } } }, [requiredRole, canAccessHostDashboard, isLoading, router, fallbackPath]); // Show loading state while checking authentication - if (isLoading || isChecking) { + if (isChecking) { return ( ... ); }
| "suspicious": { | ||
| "noDebugger": "warn", | ||
| "noExplicitAny": "off", | ||
| "noArrayIndexKey": "off", | ||
| "noConsoleLog": "off" | ||
| }, |
There was a problem hiding this comment.
Disabling noExplicitAny and noArrayIndexKey weakens type safety and React correctness.
noExplicitAny: "off"allows untypedanythroughout the codebase, reducing TypeScript's effectiveness at catching bugs.noArrayIndexKey: "off"hides warnings about using array indices as React keys, which can cause subtle rendering bugs when lists are reordered.
These appear to be workarounds to pass CI rather than addressing the underlying issues. Consider:
- Fixing the actual type issues where
anyis used inappropriately. - Using proper stable keys for React lists.
- If these must remain off temporarily, add a TODO comment with an issue reference to track re-enabling them.
🤖 Prompt for AI Agents
In `@biome.json` around lines 37 - 41, Re-enable the suspicious rules
noExplicitAny and noArrayIndexKey in the biome.json rules block (remove or
change "off"), then fix code that violates them: replace inappropriate uses of
any by adding proper types or generics (search for occurrences of explicit "any"
and update function signatures/types) and replace array-index keys in React
components with stable unique keys (search for map callbacks using index as key
and use id or generate stable keys). If you cannot fix all violations
immediately, restore the rules but add a TODO comment referencing an issue/id in
the codebase near each temporary suppression and create the tracking issue so
these rules can be re-enabled later.
| "a11y": { | ||
| "all": false | ||
| }, |
There was a problem hiding this comment.
Disabling all accessibility rules is a significant compliance concern.
Setting "a11y": { "all": false } completely disables accessibility linting across the entire codebase. This removes guardrails for WCAG compliance and can lead to accessibility barriers for users with disabilities.
If specific a11y rules are causing issues, consider disabling only those rules individually rather than blanket-disabling the entire category. If the issues are in legacy code, consider using inline suppression comments on specific files instead.
🤖 Prompt for AI Agents
In `@biome.json` around lines 42 - 44, The biome.json currently disables the
entire accessibility category via the "a11y" -> "all": false setting which
removes all accessibility linting; restore accessibility checks by removing or
changing that "a11y" all flag (e.g., set it to true or remove the key) and
instead disable only individual rules causing problems by listing them under
"a11y": { "rules": { "<rule-name>": "off", ... } } or apply inline suppression
comments in the specific legacy files; reference the "a11y" key in biome.json
and the specific rule names you intend to exempt when making the change.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.