feat(wren-ui): support cache dashboard item#1594
Conversation
WalkthroughThis change introduces comprehensive support for dashboard cache scheduling and management. It adds database migrations for new scheduling and refresh job tables, extends backend models and repositories to include cache and scheduling fields, and implements a background tracker to handle scheduled cache refreshes. GraphQL schema, resolvers, and services are updated to support querying and mutating dashboard cache schedules, with robust timezone and cron expression handling. On the frontend, new components and UI elements are added for configuring cache schedules, displaying schedule info, and triggering cache refreshes. Utilities for cron validation, timezone formatting, and error messaging are also included, along with extensive tests for scheduling logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (DashboardHeader/Drawer)
participant GraphQL API
participant DashboardResolver
participant DashboardService
participant DB
User->>UI (DashboardHeader/Drawer): Open cache settings / set schedule
UI (DashboardHeader/Drawer)->>GraphQL API: Mutation setDashboardSchedule
GraphQL API->>DashboardResolver: setDashboardSchedule
DashboardResolver->>DashboardService: setDashboardSchedule
DashboardService->>DB: Update dashboard cache/schedule fields
DashboardService-->>DashboardResolver: Updated dashboard
DashboardResolver-->>GraphQL API: Updated dashboard
GraphQL API-->>UI (DashboardHeader/Drawer): Updated dashboard info
Note over DashboardCacheBackgroundTracker: Runs every minute
DashboardCacheBackgroundTracker->>DB: Query dashboards with caching enabled and due
DashboardCacheBackgroundTracker->>DashboardService: Parse cron, get items
loop For each dashboard item
DashboardCacheBackgroundTracker->>DB: Create refresh job (IN_PROGRESS)
DashboardCacheBackgroundTracker->>QueryService: preview (refresh, cacheEnabled)
QueryService->>DB: Run query, update cache
DashboardCacheBackgroundTracker->>DB: Update job (SUCCESS/FAILED)
end
DashboardCacheBackgroundTracker->>DB: Update dashboard nextScheduledAt
Possibly related PRs
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
onlyjackfrost
force-pushed
the
feat/query-cache
branch
from
334fa15 to
a871dc4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
wren-ui/src/apollo/server/services/queryService.ts (1)
214-223: 🛠️ Refactor suggestion
refresh/cacheEnabledshould be optional in the signature
preview()forwardsrefreshandcacheEnableddirectly fromPreviewOptions, where both are declared as optional (?:).
WithstrictNullChecksenabled, passingundefinedto a parameter typed as plainbooleanis a compilation error.- private async ibisQuery( + private async ibisQuery( sql: string, dataSource: DataSourceName, connectionInfo: any, mdl: Manifest, limit: number, - refresh: boolean, - cacheEnabled: boolean, + refresh?: boolean, + cacheEnabled?: boolean, ): Promise<PreviewDataResponse> {
🧹 Nitpick comments (13)
wren-ui/src/apollo/server/utils/timezone.ts (1)
8-8: Remove non-English commentThe comment "轉換為分鐘" (Convert to minutes) should be in English for consistency.
- return (tzDate - utcDate) / 60000; // 轉換為分鐘 + return (tzDate - utcDate) / 60000; // Convert to minuteswren-ui/src/apollo/server/repositories/dashboardRepository.ts (1)
16-25: Consider updating repository methods for new fieldsThe interface has been updated, but no changes were made to the repository class implementation. Consider whether helper methods are needed for schedule-related operations.
export class DashboardRepository extends BaseRepository<Dashboard> implements IDashboardRepository { constructor(knexPg: Knex) { super({ knexPg, tableName: 'dashboard' }); } + // Fetch dashboards that are due for scheduled refresh + async findDashboardsDueForScheduledRun(): Promise<Dashboard[]> { + return this.knexTable + .where('cache_enabled', true) + .whereNotNull('next_scheduled_at') + .where('next_scheduled_at', '<=', new Date()) + .select('*'); + } + + // Update next scheduled run time + async updateNextScheduledRun(id: number, nextScheduledAt: Date | null): Promise<void> { + await this.knexTable + .where({ id }) + .update({ next_scheduled_at: nextScheduledAt }); + } }wren-ui/migrations/20250422000000_alter_dashboard_table.js (1)
7-11: Consider if cache should be enabled by default for all dashboardsThe migration sets
cache_enabledtotrueby default, which means that caching will be automatically enabled for all existing dashboards after this migration runs. Consider if this is the intended behavior, or if it would be better to start with caching disabled until users explicitly enable it.The columns for scheduling functionality are well-structured with appropriate default values and nullability.
If you decide that caching should not be enabled by default, you could modify this line:
- table.boolean('cache_enabled').defaultTo(true); + table.boolean('cache_enabled').defaultTo(false);wren-ui/src/apollo/server/models/dashboard.ts (1)
1-1: Prefer alias import to stay consistent with the rest of the server codeEverywhere else in
@server/*modules the repository layer is imported through the path alias@server/repositories.
Using a relative path here introduces an unnecessary discrepancy and may break if the directory structure is refactored.-import { DashboardItem } from '../repositories'; +import { DashboardItem } from '@server/repositories';wren-ui/src/apollo/server/services/dashboardService.ts (2)
487-489: Remove leftover debugging statement
console.log({ parts });slipped into committed code.
Logging raw cron parts in production clutters logs and may expose internal scheduling details.- console.log({ parts });
107-115: Reset schedule fields when caching is disabledIf
cacheEnabledisfalse, the persisted schedule still keeps the last chosen cron & next run time, which can be misleading when the user later re-enables caching.Consider nulling these columns when caching is turned off.
- await this.dashboardRepository.updateOne(dashboardId, { - cacheEnabled: data.cacheEnabled, - scheduleFrequency: data.schedule.frequency, - scheduleTimezone: data.schedule.timezone, - scheduleCron: cronExpression, - nextScheduledAt, - }); + await this.dashboardRepository.updateOne(dashboardId, { + cacheEnabled: data.cacheEnabled, + scheduleFrequency: data.cacheEnabled ? data.schedule.frequency : null, + scheduleTimezone: data.cacheEnabled ? data.schedule.timezone : null, + scheduleCron: data.cacheEnabled ? cronExpression : null, + nextScheduledAt: data.cacheEnabled ? nextScheduledAt : null, + });wren-ui/src/apollo/server/services/tests/dashboardService.test.ts (3)
72-82: Use enum constant instead of raw string for type-safety
dayis declared as the domain enumCacheScheduleDayEnum, but in this test we pass the raw string"MON".
WithstrictNullChecks/noImplicitAnyon, this will silently widen tostringand lose the protection the enum gives. Using the enum keeps the test aligned with production signatures and prevents typos from compiling.- const schedule = { - frequency: ScheduleFrequencyEnum.Weekly, - day: 'MON', + const schedule = { + frequency: ScheduleFrequencyEnum.Weekly, + day: CacheScheduleDayEnum.MON,
60-71: Prefer exercising the public API over reaching into private helpersThe test invokes private helpers through
(dashboardService as any).
While this works, it tightly couples tests to implementation details and will break on harmless refactors (e.g., renaming a helper). Consider:
- Calling
setDashboardScheduleand asserting the persistedscheduleCroninstead ofgenerateCronExpression.- Parsing/formatting using the exposed
parseCronExpression(if public).This yields more stable, behaviour-focused tests.
151-170: Flaky assertion – relies on wall-clock time
expect(result?.getTime()).toBeGreaterThan(new Date().getTime());can flicker if the test host is under heavy load or the clock moves backwards.
Replace with a deterministic fake time (you already usejest.useFakeTimerselsewhere) or compare deltas within a buffer around an injected base time.Example:
jest.useFakeTimers().setSystemTime(new Date('2024-04-22T10:00:00Z')); ... const now = Date.now(); expect(result!.getTime()).toBeGreaterThan(now);wren-ui/src/apollo/server/resolvers/dashboardResolver.ts (2)
48-55: Duplicate null-check – one is sufficient
dashboardis checked twice in succession. The second branch is unreachable and can be removed for clarity.- if (!dashboard) { - throw new Error('Dashboard not found.'); - } - const schedule = ctx.dashboardService.parseCronExpression(dashboard); - if (!dashboard) { - throw new Error('Dashboard not found.'); - } + if (!dashboard) { + throw new Error('Dashboard not found.'); + } + const schedule = ctx.dashboardService.parseCronExpression(dashboard);
198-215: Mutation always returnstrueeven when repository update fails
setDashboardSchedulereturnstrueafter calling the service, but the service may throw inside yet caller wraps it intry/catchand still returnstruefor unexpected, non-error paths (e.g., repository returns 0 affected rows).Consider returning the boolean result of
dashboardService.setDashboardSchedule(which could be void→undefined) or querying the update count to reflect success/failure.wren-ui/src/apollo/server/schema.ts (2)
889-890:refreshargument should be non-null with a default valueGraphQL optional booleans become
nullrather thanfalse.
If the resolver treatsundefinedandfalsethe same, define a default:input PreviewItemSQLInput { itemId: Int! limit: Int refresh: Boolean = false }
924-929: Enum casing inconsistent with existing styleExisting enums use screaming-snake case (e.g.,
ONBOARDING_FINISHED).
ScheduleFrequencyEnumintroducesDaily,Weekly, … in PascalCase, reducing visual consistency.Consider:
enum ScheduleFrequencyEnum { DAILY WEEKLY CUSTOM NEVER }
🛑 Comments failed to post (9)
wren-ui/src/apollo/server/utils/timezone.ts (3)
1-19: 🛠️ Refactor suggestion
Add input validation to both functions
Both functions lack input validation which might lead to runtime errors with invalid inputs.
export function getUTCOffsetMinutes(timeZone: string) { + if (!timeZone) { + throw new Error('Timezone is required'); + } + + try { const date = new Date(); const utcDate = new Date( date.toLocaleString('en-US', { timeZone: 'UTC' }), ) as any; const tzDate = new Date(date.toLocaleString('en-US', { timeZone })) as any; return (tzDate - utcDate) / 60000; // 轉換為分鐘 + } catch (error) { + throw new Error(`Invalid timezone: ${timeZone}`); + } } export function formatUTCOffset(offsetMinutes: number) { + if (typeof offsetMinutes !== 'number' || isNaN(offsetMinutes)) { + throw new Error('Offset minutes must be a valid number'); + } + const sign = offsetMinutes >= 0 ? '+' : '-'; const absOffset = Math.abs(offsetMinutes); const hours = Math.floor(absOffset / 60) .toString() .padStart(2, '0'); const minutes = (absOffset % 60).toString().padStart(2, '0'); return `UTC${sign}${hours}:${minutes}`; }📝 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 function getUTCOffsetMinutes(timeZone: string) { if (!timeZone) { throw new Error('Timezone is required'); } try { const date = new Date(); const utcDate = new Date( date.toLocaleString('en-US', { timeZone: 'UTC' }), ) as any; const tzDate = new Date( date.toLocaleString('en-US', { timeZone }), ) as any; return (tzDate.getTime() - utcDate.getTime()) / 60000; // 轉換為分鐘 } catch (error) { throw new Error(`Invalid timezone: ${timeZone}`); } } export function formatUTCOffset(offsetMinutes: number) { if (typeof offsetMinutes !== 'number' || isNaN(offsetMinutes)) { throw new Error('Offset minutes must be a valid number'); } const sign = offsetMinutes >= 0 ? '+' : '-'; const absOffset = Math.abs(offsetMinutes); const hours = Math.floor(absOffset / 60) .toString() .padStart(2, '0'); const minutes = (absOffset % 60).toString().padStart(2, '0'); return `UTC${sign}${hours}:${minutes}`; }
1-19: 💡 Verification agent
🧩 Analysis chain
Add unit tests for timezone utility functions
These utility functions are critical for scheduling functionality and should have thorough test coverage.
🏁 Script executed:
#!/bin/bash # Check if there are any tests for the timezone utilities echo "Searching for timezone utility tests..." fd -t f "timezone.*test.*\.(ts|js)" src echo "Checking imports of timezone utilities in test files..." rg -l "getUTCOffsetMinutes|formatUTCOffset" --type ts --type js "**/*.test.ts" "**/*.test.js" "**/__tests__/**"Length of output: 618
#!/bin/bash set -e echo "Searching for timezone utility test files across the repo..." fd -t f "timezone.*test.*\.(ts|js|tsx|jsx)" . echo "Searching for any test importing the timezone utilities..." rg -l "getUTCOffsetMinutes|formatUTCOffset" \ --type ts --type tsx --type js --type jsx \ --glob "*.test.*" --glob "*.spec.*"
#!/bin/bash set -e echo "Searching for timezone utility test files under wren-ui/src..." fd -t f "timezone.*\.(test|spec)\.(ts|js|tsx|jsx)" wren-ui/src echo "Searching for any test importing the timezone utilities..." rg -l "getUTCOffsetMinutes|formatUTCOffset" \ --glob "wren-ui/**/*.test.*.{ts,tsx,js,jsx}" \ --glob "wren-ui/**/*.spec.*.{ts,tsx,js,jsx}"
🏁 Script executed:
#!/bin/bash set -e echo "Searching for timezone utility test files under wren-ui..." fd --regex 'timezone.*\.(test|spec)\.(ts|js|tsx|jsx)$' wren-ui echo "Searching for timezone functions in any test/spec files under wren-ui..." rg -n 'getUTCOffsetMinutes|formatUTCOffset' --glob 'wren-ui/**/*test.*' --glob 'wren-ui/**/*spec.*'Length of output: 457
Add unit tests for timezone utility functions
No existing tests cover
getUTCOffsetMinutesorformatUTCOffset. Please add a new test file, for example:
- File:
wren-ui/src/apollo/server/utils/timezone.test.ts- Tests to include:
• VerifygetUTCOffsetMinutesreturns correct offsets for known zones (e.g., “UTC”, “America/New_York”, “Asia/Tokyo”), including during DST transitions.
• VerifyformatUTCOffsetformats positive, negative, and zero offsets correctly (e.g.,UTC+00:00,UTC-05:30,UTC+09:00).
• Edge cases: non-existent timeZone strings (expecting thrown error or fallback).Sample Jest scaffold:
import { getUTCOffsetMinutes, formatUTCOffset } from "./timezone"; describe("timezone utilities", () => { test("UTC offset for UTC is 0", () => { expect(getUTCOffsetMinutes("UTC")).toBe(0); expect(formatUTCOffset(0)).toBe("UTC+00:00"); }); test("format negative offset", () => { expect(formatUTCOffset(-330)).toBe("UTC-05:30"); }); // add more cases... });
1-9: 🛠️ Refactor suggestion
Improve timezone offset calculation for better reliability
The current implementation of
getUTCOffsetMinutesusesDateobject string conversion and subtraction which can be error-prone. The method relies on converting dates to locale strings and back to Date objects, which might lead to inconsistencies across environments.Consider using the more reliable Intl.DateTimeFormat API instead:
export function getUTCOffsetMinutes(timeZone: string) { - const date = new Date(); - const utcDate = new Date( - date.toLocaleString('en-US', { timeZone: 'UTC' }), - ) as any; - const tzDate = new Date(date.toLocaleString('en-US', { timeZone })) as any; - - return (tzDate - utcDate) / 60000; // 轉換為分鐘 + const date = new Date(); + const formatter = new Intl.DateTimeFormat('en-US', { + timeZone, + year: 'numeric', + month: 'numeric', + day: 'numeric', + hour: 'numeric', + minute: 'numeric', + second: 'numeric', + hour12: false + }); + + const parts = formatter.formatToParts(date); + const formattedDate = new Date( + `${parts.find(p => p.type === 'year')?.value}-${parts.find(p => p.type === 'month')?.value}-${parts.find(p => p.type === 'day')?.value}T${parts.find(p => p.type === 'hour')?.value}:${parts.find(p => p.type === 'minute')?.value}:${parts.find(p => p.type === 'second')?.value}` + ); + + return (formattedDate.getTime() - date.getTime()) / 60000; }Alternatively, consider leveraging the new cron-parser dependency which might have timezone utilities built-in.
Committable suggestion skipped: line range outside the PR's diff.
wren-ui/src/apollo/server/repositories/dashboardRepository.ts (1)
9-13: 💡 Verification agent
🧩 Analysis chain
Verify database schema migration for new fields
The interface has been updated with new fields, but we should verify that a corresponding database migration exists to add these columns to the table.
🏁 Script executed:
#!/bin/bash # Check for migration files that might add cache and schedule columns to dashboard table echo "Looking for relevant migration files..." fd -t f -e js ".*dashboard.*" "migrations" | sort -r | head -5 echo "-----------------" echo "Checking content of recent migrations for dashboard table modifications..." rg -l "cache_enabled|schedule_frequency|schedule_timezone|schedule_cron|next_scheduled_at" --type js "migrations/*.js" | xargs catLength of output: 640
Add missing database migration for new dashboard fields
It appears no migration in the repo updates the
dashboardtable to include the new columns (cache_enabled,schedule_frequency,schedule_timezone,schedule_cron,next_scheduled_at). Please:
- Create a migration that adds these columns (with matching names and types) to the
dashboardtable.- Verify the migration runs cleanly and the new fields are present in your local/staging databases.
- Confirm your ORM models/interfaces stay in sync with the schema changes.
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (1)
547-557:
⚠️ Potential issueFix inconsistency in query parameter names
There's an inconsistency between the parameter names used in the
buildQueryStringmethod and those expected in the tests. This could lead to the backend not recognizing the parameters correctly.private buildQueryString(options: IbisQueryOptions) { if (!options.cacheEnabled) { return ''; } const queryString = []; - queryString.push('cacheEnable=true'); + queryString.push('cacheEnabled=true'); if (options.refresh) { - queryString.push('overrideCache=true'); + queryString.push('cacheOverride=true'); } return `?${queryString.join('&')}`; }The tests at lines 514 and 549 expect
cacheEnabled=trueandcacheOverride=true, but the method usescacheEnable=trueandoverrideCache=true. These should be made consistent.📝 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.private buildQueryString(options: IbisQueryOptions) { if (!options.cacheEnabled) { return ''; } const queryString = []; queryString.push('cacheEnabled=true'); if (options.refresh) { queryString.push('cacheOverride=true'); } return `?${queryString.join('&')}`; }wren-ui/src/apollo/server/models/dashboard.ts (1)
28-35: 🛠️ Refactor suggestion
dayshould be nullable / optional
DashboardSchedule.dayis only meaningful for Weekly schedules.
Current callers (generateCronExpression,validateScheduleInput,parseCronExpression) already treat it as potentiallynull, so the type should reflect that to avoid type-casting and accidental misuse.-export interface DashboardSchedule { +export interface DashboardSchedule { frequency: ScheduleFrequencyEnum; timezone: string; - day: CacheScheduleDayEnum; + day: CacheScheduleDayEnum | null; hour: number; minute: number; cron: string; }📝 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 interface DashboardSchedule { frequency: ScheduleFrequencyEnum; timezone: string; day: CacheScheduleDayEnum | null; hour: number; minute: number; cron: string; }wren-ui/src/apollo/server/resolvers/dashboardResolver.ts (1)
161-170:
⚠️ Potential issuePossible runtime NPE when dashboard is undefined
const { cacheEnabled } = await ctx.dashboardService.getCurrentDashboard();destructures without verifying the dashboard exists. If the call returnsnull, the resolver crashes withCannot read properties of null.Add the same guard you use elsewhere:
- const { cacheEnabled } = await ctx.dashboardService.getCurrentDashboard(); + const dashboard = await ctx.dashboardService.getCurrentDashboard(); + if (!dashboard) { + throw new Error('Dashboard not found.'); + } + const { cacheEnabled } = dashboard;📝 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.try { const item = await ctx.dashboardService.getDashboardItem(itemId); const dashboard = await ctx.dashboardService.getCurrentDashboard(); if (!dashboard) { throw new Error('Dashboard not found.'); } const { cacheEnabled } = dashboard; const project = await ctx.projectService.getCurrentProject(); const deployment = await ctx.deployService.getLastDeployment(project.id); const mdl = deployment.manifest; const data = (await ctx.queryService.preview(item.detail.sql, {wren-ui/src/apollo/server/schema.ts (2)
901-904: 🛠️ Refactor suggestion
scheduleshould be optional when caching is disabled
SetDashboardScheduleInputrequiresscheduleeven ifcacheEnabledisfalse, leading to confusing API usage:input SetDashboardScheduleInput { cacheEnabled: Boolean! schedule: SetDashboardScheduleData }Resolver/service can then ignore
scheduleifcacheEnabledisfalse.
900-922: 🛠️ Refactor suggestion
Schedule input forces hour/minute even when unused
For
CustomandNeverfrequencies onlycron(or nothing) is meaningful, yethour/minuteare declaredInt!. This burdens clients with dummy data and complicates validation logic already handled in the service layer.Recommended shape:
input SetDashboardScheduleData { frequency: ScheduleFrequencyEnum! hour: Int minute: Int day: CacheScheduleDayEnum timezone: String cron: String }The corresponding
DashboardScheduletype should mirror the nullable fields.-type DashboardSchedule { - frequency: ScheduleFrequencyEnum! - hour: Int! - minute: Int! +type DashboardSchedule { + frequency: ScheduleFrequencyEnum! + hour: Int + minute: Int ...
onlyjackfrost
force-pushed
the
feat/query-cache
branch
from
53e03d5 to
ce35f92
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (7)
wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js (1)
9-9: Consider using enum values for status comment.The comment describes possible values as 'success', 'failed', 'in_progress', but the actual values used in the code (DashboardCacheRefreshStatus enum) are slightly different with underscores between words.
- table.string('status').notNullable(); // 'success', 'failed', 'in_progress' + table.string('status').notNullable(); // Values: 'success', 'failed', 'in_progress' - see DashboardCacheRefreshStatus enumwren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts (3)
61-88: Consider adding rate limiting or batching for large dashboard sets.The current implementation processes all cached dashboards sequentially in a single interval run. If many dashboards have similar schedules, this could cause performance issues.
Consider adding:
- A limit on how many dashboards to process per interval
- A batching mechanism to spread the load
- A more sophisticated scheduling approach
private async checkAndRefreshCaches(): Promise<void> { try { // Get all dashboards with cache enabled const dashboards = await this.dashboardRepository.findAllBy({ cacheEnabled: true, }); + // Limit the number of dashboards to process per interval + const MAX_DASHBOARDS_PER_INTERVAL = 5; + let processedCount = 0; + for (const dashboard of dashboards) { if (!dashboard.scheduleCron || !dashboard.nextScheduledAt) { continue; } const now = new Date(); const nextScheduledAt = new Date(dashboard.nextScheduledAt); // Check if it's time to refresh if (now >= nextScheduledAt) { logger.info(`Start Refreshing cache for dashboard ${dashboard.id}`); await this.refreshDashboardCache(dashboard); logger.info( `Finished Refreshing cache for dashboard ${dashboard.id}`, ); + + processedCount++; + if (processedCount >= MAX_DASHBOARDS_PER_INTERVAL) { + logger.info(`Reached maximum dashboard processing limit (${MAX_DASHBOARDS_PER_INTERVAL}). Remaining will be processed in next interval.`); + break; + } } } } catch (error) { logger.error(`Error checking dashboard caches: ${error.message}`); } }
125-154: Add retry mechanism for failed cache refresh operations.Currently, if a cache refresh fails, the error is logged but there's no retry mechanism. Consider implementing a retry strategy for transient failures.
try { await this.queryService.preview(item.detail.sql, { project, manifest: mdl, cacheEnabled: true, refresh: true, }); // Update the record with success await this.dashboardCacheRefreshRepository.updateOne( refreshJob.id, { finishedAt: new Date(), status: DashboardCacheRefreshStatus.SUCCESS, }, ); } catch (error) { + // Check if error is potentially transient and worth retrying + const isTransientError = this.isTransientError(error); + const status = isTransientError + ? DashboardCacheRefreshStatus.FAILED_RETRYABLE + : DashboardCacheRefreshStatus.FAILED; + // Update the record with failure await this.dashboardCacheRefreshRepository.updateOne( refreshJob.id, { finishedAt: new Date(), - status: DashboardCacheRefreshStatus.FAILED, + status, errorMessage: error.message, }, ); logger.debug( `Error refreshing cache for item ${item.id}: ${error.message}`, ); }You would also need to:
- Add a new status to the enum in the repository
- Implement the
isTransientErrormethod to identify errors worth retrying- Add scheduled retry logic for failed-but-retryable jobs
183-193: Consider caching successfully parsed cron expressions.Each call to CronExpressionParser.parse incurs some overhead. Since the same cron expressions are likely used repeatedly, consider caching the parsed results.
+ private cronCache = new Map<string, any>(); private calculateNextRunTime(cronExpression: string): Date | null { try { + // Check if we already parsed this expression + if (this.cronCache.has(cronExpression)) { + const interval = this.cronCache.get(cronExpression); + // Create a fresh iterator starting from now + return interval.clone({ currentDate: new Date() }).next().toDate(); + } + const interval = CronExpressionParser.parse(cronExpression, { currentDate: new Date(), }); + // Cache the parsed expression + this.cronCache.set(cronExpression, interval); return interval.next().toDate(); } catch (error) { logger.error(`Failed to parse cron expression: ${error.message}`); return null; } }wren-ui/src/apollo/server/repositories/dashboardCacheRefreshRepository.ts (3)
48-57: Consider adding limit parameter to findLatestByDashboardId.For dashboards with many refresh operations, retrieving all records and then taking the first could be inefficient. Consider adding a limit parameter with a default of 1 for better performance.
public async findLatestByDashboardId( dashboardId: number, + limit: number = 1 ): Promise<DashboardCacheRefresh | null> { const result = await this.knex .table(this.tableName) .where({ dashboardId }) .orderBy('createdAt', 'desc') + .limit(limit) .first(); return result ? this.transformFromDBData(result) : null; }
70-81: Add pagination support for findInProgressByDashboardId.This method could potentially return a large number of records for dashboards with many in-progress jobs. Adding pagination would improve performance and usability.
public async findInProgressByDashboardId( dashboardId: number, + page: number = 1, + pageSize: number = 20 ): Promise<DashboardCacheRefresh[]> { const results = await this.knex .table(this.tableName) .where({ dashboardId, status: DashboardCacheRefreshStatus.IN_PROGRESS, }) .orderBy('createdAt', 'desc') + .offset((page - 1) * pageSize) + .limit(pageSize); return results.map((result) => this.transformFromDBData(result)); }Apply similar changes to findInProgressByDashboardItemId for consistency.
96-102: Add method to find all records by hash.The current
findByHashmethod only returns a single record, but the hash is used to group related refresh operations. Adding a method to find all records with the same hash would be useful for tracking batch operations.+ public async findAllByHash(hash: string): Promise<DashboardCacheRefresh[]> { + const results = await this.knex + .table(this.tableName) + .where({ hash }) + .orderBy('createdAt', 'desc'); + return results.map((result) => this.transformFromDBData(result)); + } public async findByHash(hash: string): Promise<DashboardCacheRefresh | null> { const result = await this.knex .table(this.tableName) .where({ hash }) .first(); return result ? this.transformFromDBData(result) : null; }
🛑 Comments failed to post (2)
wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts (2)
90-90: 🛠️ Refactor suggestion
Replace 'any' type with explicit dashboard interface.
Using 'any' type reduces type safety. Since this is TypeScript, use a specific dashboard interface type that includes the fields you're using.
- private async refreshDashboardCache(dashboard: any): Promise<void> { + private async refreshDashboardCache(dashboard: { + id: number; + scheduleCron: string | null; + nextScheduledAt: Date | string | null + }): Promise<void> {📝 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.private async refreshDashboardCache(dashboard: { id: number; scheduleCron: string | null; nextScheduledAt: Date | string | null }): Promise<void> {
19-52: 🛠️ Refactor suggestion
Add a method to stop the background tracker for graceful shutdown.
The class starts an interval timer in the constructor but doesn't provide a way to stop it. This could lead to memory leaks if the application needs to shut down gracefully or if you need to replace the tracker instance.
export class DashboardCacheBackgroundTracker { private intervalTime: number; private dashboardRepository: IDashboardRepository; private dashboardItemRepository: IDashboardItemRepository; private dashboardCacheRefreshRepository: IDashboardCacheRefreshRepository; private projectService: IProjectService; private deployService: IDeployService; private queryService: IQueryService; private runningJobs = new Set<number>(); + private intervalId: NodeJS.Timeout | null = null; constructor({ dashboardRepository, dashboardItemRepository, dashboardCacheRefreshRepository, projectService, deployService, queryService, }: { dashboardRepository: IDashboardRepository; dashboardItemRepository: IDashboardItemRepository; dashboardCacheRefreshRepository: IDashboardCacheRefreshRepository; projectService: IProjectService; deployService: IDeployService; queryService: IQueryService; }) { this.dashboardRepository = dashboardRepository; this.dashboardItemRepository = dashboardItemRepository; this.dashboardCacheRefreshRepository = dashboardCacheRefreshRepository; this.projectService = projectService; this.deployService = deployService; this.queryService = queryService; this.intervalTime = 60000; // 1 minute this.start(); } private start(): void { logger.info('Dashboard cache background tracker started'); - setInterval(() => { + this.intervalId = setInterval(() => { this.checkAndRefreshCaches(); }, this.intervalTime); } + public stop(): void { + if (this.intervalId) { + clearInterval(this.intervalId); + this.intervalId = null; + logger.info('Dashboard cache background tracker stopped'); + } + }📝 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 class DashboardCacheBackgroundTracker { private intervalTime: number; private dashboardRepository: IDashboardRepository; private dashboardItemRepository: IDashboardItemRepository; private dashboardCacheRefreshRepository: IDashboardCacheRefreshRepository; private projectService: IProjectService; private deployService: IDeployService; private queryService: IQueryService; private runningJobs = new Set<number>(); private intervalId: NodeJS.Timeout | null = null; constructor({ dashboardRepository, dashboardItemRepository, dashboardCacheRefreshRepository, projectService, deployService, queryService, }: { dashboardRepository: IDashboardRepository; dashboardItemRepository: IDashboardItemRepository; dashboardCacheRefreshRepository: IDashboardCacheRefreshRepository; projectService: IProjectService; deployService: IDeployService; queryService: IQueryService; }) { this.dashboardRepository = dashboardRepository; this.dashboardItemRepository = dashboardItemRepository; this.dashboardCacheRefreshRepository = dashboardCacheRefreshRepository; this.projectService = projectService; this.deployService = deployService; this.queryService = queryService; this.intervalTime = 60000; // 1 minute this.start(); } private start(): void { logger.info('Dashboard cache background tracker started'); this.intervalId = setInterval(() => { this.checkAndRefreshCaches(); }, this.intervalTime); } public stop(): void { if (this.intervalId) { clearInterval(this.intervalId); this.intervalId = null; logger.info('Dashboard cache background tracker stopped'); } } }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wren-ui/src/apollo/server/services/dashboardService.ts (2)
389-406: Cron expression portability & validation
daycomes fromCacheScheduleDayEnum(e.g."MON"). Whilecron-parseraccepts three-letter day names, many cron engines (e.g. Linuxcrond) expect numeric values (0-6). If the backend ever forwards these expressions to external systems, consider normalising to numbers to avoid portability issues.At minimum, document the expectation and validate
dayagainst the allowed set (MON-SUN) before constructing the expression.
257-319: Timezone arithmetic is brittle – lean on battle-tested librariesManual offset math is hard to prove correct for DST transitions and historical timezone changes. A lightweight library such as
luxon(already tree-shake-friendly and no moment-style bloat) can convert between zones safely:import { DateTime } from 'luxon'; private toUTC(schedule: DashboardSchedule): DashboardSchedule { if (!schedule.timezone || schedule.frequency === ScheduleFrequencyEnum.CUSTOM) return schedule; const local = DateTime.fromObject( { hour: schedule.hour, minute: schedule.minute }, { zone: schedule.timezone }, ); const utc = local.toUTC(); return { ...schedule, hour: utc.hour, minute: utc.minute }; }This removes ~130 LOC of intricate offset handling and future-proofs DST edge-cases.
Also applies to: 321-387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
wren-ui/migrations/20250422000000_alter_dashboard_table.js(1 hunks)wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js(1 hunks)wren-ui/package.json(1 hunks)wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(5 hunks)wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(2 hunks)wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts(1 hunks)wren-ui/src/apollo/server/backgrounds/index.ts(1 hunks)wren-ui/src/apollo/server/models/dashboard.ts(1 hunks)wren-ui/src/apollo/server/models/index.ts(1 hunks)wren-ui/src/apollo/server/repositories/dashboardCacheRefreshRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/dashboardRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/index.ts(1 hunks)wren-ui/src/apollo/server/resolvers.ts(2 hunks)wren-ui/src/apollo/server/resolvers/dashboardResolver.ts(4 hunks)wren-ui/src/apollo/server/schema.ts(4 hunks)wren-ui/src/apollo/server/services/dashboardService.ts(5 hunks)wren-ui/src/apollo/server/services/queryService.ts(6 hunks)wren-ui/src/apollo/server/services/tests/dashboardService.test.ts(1 hunks)wren-ui/src/apollo/server/types/context.ts(3 hunks)wren-ui/src/apollo/server/utils/timezone.ts(1 hunks)wren-ui/src/common.ts(6 hunks)wren-ui/src/pages/api/graphql.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- wren-ui/src/apollo/server/models/index.ts
- wren-ui/package.json
🚧 Files skipped from review as they are similar to previous changes (19)
- wren-ui/src/apollo/server/repositories/index.ts
- wren-ui/src/apollo/server/backgrounds/index.ts
- wren-ui/src/apollo/server/types/context.ts
- wren-ui/src/apollo/server/resolvers.ts
- wren-ui/src/apollo/server/utils/timezone.ts
- wren-ui/src/pages/api/graphql.ts
- wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js
- wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts
- wren-ui/src/common.ts
- wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts
- wren-ui/src/apollo/server/repositories/dashboardRepository.ts
- wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts
- wren-ui/src/apollo/server/services/tests/dashboardService.test.ts
- wren-ui/src/apollo/server/repositories/dashboardCacheRefreshRepository.ts
- wren-ui/src/apollo/server/resolvers/dashboardResolver.ts
- wren-ui/src/apollo/server/models/dashboard.ts
- wren-ui/src/apollo/server/services/queryService.ts
- wren-ui/src/apollo/server/schema.ts
- wren-ui/migrations/20250422000000_alter_dashboard_table.js
🔇 Additional comments (2)
wren-ui/src/apollo/server/services/dashboardService.ts (2)
20-20:⚠️ Potential issueIncorrect import style for
cron-parserThe
cron-parserpackage exports a default module whose API exposesparseExpression().
CronExpressionParseris not an exported symbol, thereforeimport { CronExpressionParser }will beundefinedat runtime and every subsequent call toCronExpressionParser.parse()will throw.-import { CronExpressionParser } from 'cron-parser'; +import cronParser from 'cron-parser'; // default exportUpdate the two call-sites below from
CronExpressionParser.parsetocronParser.parseExpression.
Failing to do so will break schedule creation and every dashboard page load that touchesparseCronExpression().⛔ Skipped due to learnings
Learnt from: andreashimin PR: Canner/WrenAI#1587 File: wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx:3-4 Timestamp: 2025-04-23T09:21:38.365Z Learning: The cron-parser package (v5+) provides a named export `CronExpressionParser` with a `parse` method that should be used like: `import { CronExpressionParser } from 'cron-parser'; const interval = CronExpressionParser.parse(cronExpression, options);`
408-418:⚠️ Potential issueUse
parseExpression()instead of the non-existentparse()After fixing the import, adjust the parser invocations:
-const interval = CronExpressionParser.parse(cronExpression, { currentDate: new Date() }); +const interval = cronParser.parseExpression(cronExpression, { currentDate: new Date() });and
-const baseInterval = CronExpressionParser.parse(schedule.cron, { currentDate: new Date() }); +const baseInterval = cronParser.parseExpression(schedule.cron, { currentDate: new Date() }); ... -const nextInterval = CronExpressionParser.parse(schedule.cron, { currentDate: baseDate }); +const nextInterval = cronParser.parseExpression(schedule.cron, { currentDate: baseDate });These changes are mandatory: the current code will throw
TypeError: CronExpressionParser.parse is not a function, preventing any cache schedule from being saved or validated.Also applies to: 449-461
⛔ Skipped due to learnings
Learnt from: andreashimin PR: Canner/WrenAI#1587 File: wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx:3-4 Timestamp: 2025-04-23T09:21:38.365Z Learning: The cron-parser package (v5+) provides a named export `CronExpressionParser` with a `parse` method that should be used like: `import { CronExpressionParser } from 'cron-parser'; const interval = CronExpressionParser.parse(cronExpression, options);`
| return { ...dashboard }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against null dashboards
findOneBy() returns null when no record matches.
Spreading null results in {} which silently hides the error and may cascade into “undefined property” crashes elsewhere.
- const dashboard = await this.dashboardRepository.findOneBy({ projectId: project.id });
- return { ...dashboard };
+ const dashboard = await this.dashboardRepository.findOneBy({ projectId: project.id });
+ if (!dashboard) {
+ throw new Error(`Dashboard for project ${project.id} not found`);
+ }
+ return { ...dashboard };Fail fast here to surface configuration problems early.
📝 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.
| return { ...dashboard }; | |
| } | |
| const dashboard = await this.dashboardRepository.findOneBy({ projectId: project.id }); | |
| if (!dashboard) { | |
| throw new Error(`Dashboard for project ${project.id} not found`); | |
| } | |
| return { ...dashboard }; |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wren-ui/src/apollo/server/services/dashboardService.ts (1)
148-148:⚠️ Potential issueGuard against
nulldashboards.Your implementation of
getCurrentDashboard()spreads the dashboard without checking if it exists first. If no dashboard is found, this will spreadnullresulting in{}which silently hides the error and may cascade into "undefined property" crashes elsewhere.Apply this fix to fail fast and surface configuration problems early:
public async getCurrentDashboard(): Promise<Dashboard> { const project = await this.projectService.getCurrentProject(); const dashboard = await this.dashboardRepository.findOneBy({ projectId: project.id, }); - return { ...dashboard }; + if (!dashboard) { + throw new Error(`Dashboard for project ${project.id} not found`); + } + return { ...dashboard }; }
🧹 Nitpick comments (4)
wren-ui/src/apollo/server/services/dashboardService.ts (3)
429-449: Skip validation in dev mode but enforce in production.Your validation logic for custom cron expressions includes special handling for development mode, but the comment doesn't fully explain the reasoning.
Improve the comment to clearly explain why we're skipping this validation in development:
if (schedule.frequency === ScheduleFrequencyEnum.CUSTOM) { - // can not less than 10 minutes, skip if is local + // Skip minimum interval validation in development for easier testing, + // but enforce 10+ minute intervals in production to prevent excessive load if (process.env.NODE_ENV === 'development') return; const baseInterval = CronExpressionParser.parse(schedule.cron, { currentDate: new Date(), }); // Rest of validation...
447-465: Refactor cron interval validation for better maintainability.The validation logic for checking minimum cron interval is embedded within the main validation method. Extracting this to a separate method would improve readability and maintainability.
Extract the custom cron minimum interval validation to a separate method:
protected validateScheduleInput(data: SetDashboardCacheData): void { const { schedule } = data; // Other validations... if (schedule.frequency === ScheduleFrequencyEnum.CUSTOM) { // can not less than 10 minutes, skip if is local if (process.env.NODE_ENV === 'development') return; - const baseInterval = CronExpressionParser.parse(schedule.cron, { - currentDate: new Date(), - }); - const baseDate = baseInterval.next().toDate(); - - const nextInterval = CronExpressionParser.parse(schedule.cron, { - currentDate: baseDate, - }); - const nextDate = nextInterval.next().toDate(); - const diff = nextDate.getTime() - baseDate.getTime(); - if (diff < 10 * 60 * 1000) { - throw new Error( - 'Custom cron expression must be at least 10 minutes apart', - ); - } + this.validateCronMinimumInterval(schedule.cron); } } + protected validateCronMinimumInterval(cronExpression: string): void { + const baseInterval = CronExpressionParser.parse(cronExpression, { + currentDate: new Date(), + }); + const baseDate = baseInterval.next().toDate(); + + const nextInterval = CronExpressionParser.parse(cronExpression, { + currentDate: baseDate, + }); + const nextDate = nextInterval.next().toDate(); + const diff = nextDate.getTime() - baseDate.getTime(); + if (diff < 10 * 60 * 1000) { + throw new Error( + 'Custom cron expression must be at least 10 minutes apart', + ); + } + }
468-521: Improve type safety in theparseCronExpressionmethod.The implementation uses type assertions (
as DashboardSchedule) multiple times, which can mask potential errors.Refactor to use safer type handling:
public parseCronExpression(dashboard: Dashboard): DashboardSchedule { if (!dashboard.scheduleCron) { - return { + const baseSchedule: DashboardSchedule = { frequency: dashboard.scheduleFrequency, hour: 0, minute: 0, day: null, timezone: dashboard.scheduleTimezone || '', cron: '', - } as DashboardSchedule; + }; + return baseSchedule; } // ... rest of the method case ScheduleFrequencyEnum.NEVER: { - return { + const neverSchedule: DashboardSchedule = { frequency: ScheduleFrequencyEnum.NEVER, hour: null, minute: null, day: null, timezone: dashboard.scheduleTimezone || '', cron: null, - } as DashboardSchedule; + }; + return neverSchedule; } // ... similarly for other cases }wren-ui/src/apollo/server/services/tests/dashboardService.test.ts (1)
29-31: Remove unnecessary return statement from void function.The
testValidateScheduleInputmethod returns a value despite having a void return type, which is causing a static analysis warning.public testValidateScheduleInput(data: SetDashboardCacheData): void { - return this.validateScheduleInput(data); + this.validateScheduleInput(data); }🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/src/apollo/server/services/dashboardService.ts(5 hunks)wren-ui/src/apollo/server/services/tests/dashboardService.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wren-ui/src/apollo/server/services/tests/dashboardService.test.ts (2)
wren-ui/src/apollo/server/services/dashboardService.ts (1)
DashboardService(60-523)wren-ui/src/apollo/server/models/dashboard.ts (2)
DashboardSchedule(28-35)SetDashboardCacheData(37-40)
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/tests/dashboardService.test.ts
[error] 30-30: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
wren-ui/src/apollo/server/services/dashboardService.ts (2)
258-387: Well-implemented timezone conversion with day adjustment.The timezone conversion logic correctly handles complex cases including:
- Minute and hour carryover
- Day adjustments for weekly schedules
- Various timezone offsets around the world
This is a robust implementation that accounts for all edge cases when converting between UTC and local time.
79-128: Well-designed dashboard schedule implementation.The
setDashboardSchedulemethod is well-structured with:
- Proper input validation
- Null checks for the dashboard
- Clear logic for enabled/disabled caching
- Appropriate error handling
wren-ui/src/apollo/server/services/tests/dashboardService.test.ts (6)
88-144: Great test coverage for cron expression generation.The tests comprehensively verify cron expression generation for all supported schedule frequencies (daily, weekly, custom, never). Each test case uses clear inputs and expected outputs.
146-217: Thorough testing of next run time calculation.These tests effectively verify:
- Handling of invalid cron expressions
- Calculation of correct next run times for different schedules
- Time component verification (day of week, hour patterns)
- Timezone considerations
The tests include both basic functionality and edge cases.
219-405: Comprehensive timezone conversion tests.The timezone conversion tests cover an impressive range of scenarios:
- UTC to local time and local time to UTC conversions
- Handling of timezones with different UTC offsets
- Day boundary cases (midnight crossing)
- Day of week adjustments for weekly schedules
- Non-hour offsets (like India's UTC+5:30)
This thorough testing ensures the complex timezone logic works correctly across global use cases.
407-593: Comprehensive timezone-to-local conversion tests.The tests for converting from UTC to local timezone are equally thorough, covering:
- Edge cases like day wrapping (Friday to Saturday)
- Various UTC offsets
- Minute and hour adjustments
- Special cases like custom schedules
This excellent test coverage ensures the timezone conversion logic is robust.
595-775: Thorough input validation testing.The tests for schedule input validation include:
- All error cases (missing day for weekly schedule, missing cron expression, invalid hour/minute/timezone)
- Minimum interval requirements for custom schedules
- Valid cases for each schedule type
This ensures the validation logic correctly enforces all constraints.
777-976: Complete testing of the dashboard schedule update workflow.The tests for
setDashboardScheduleverify the entire workflow including:
- Error handling for missing dashboards
- Correct updating of daily/weekly/custom schedules
- Handling of disabled cache
- Timezone conversions during updates
- Error handling during repository operations
This thorough testing ensures the end-to-end scheduling functionality works as expected.
| @@ -0,0 +1,35 @@ | |||
| exports.up = function (knex) { | |||
| return knex.schema.createTable('dashboard_cache_refresh', (table) => { | |||
There was a problem hiding this comment.
nit: maybe dashboard_item_refresh_job ? to better describe it's the job result of a dashboard item refresh.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
wren-ui/src/apollo/server/schema.ts (2)
892-898: RenamecacheOverrideAttocacheOverrodeAtfor grammatical correctness.The field name should use past tense since it represents a timestamp when an action occurred in the past.
type PreviewItemResponse { data: JSON! cacheHit: Boolean! cacheCreatedAt: String - cacheOverrideAt: String + cacheOverrodeAt: String override: Boolean! }
972-980: Missing default value for cacheEnabled in DetailedDashboard.The
cacheEnabledfield inDetailedDashboardneeds a default value as mentioned in previous review comments.Also, consider how the
schedulefield (which is required with!) behaves whencacheEnabledisfalse. It might be clearer to make this field optional or provide a default "disabled" schedule when caching is turned off.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/migrations/20250422000000_alter_dashboard_table.js(1 hunks)wren-ui/src/apollo/server/schema.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/migrations/20250422000000_alter_dashboard_table.js
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
wren-ui/src/apollo/server/schema.ts (5)
886-890: Good addition of the refresh parameter to control cache behavior.The
refreshflag with a default value offalseis a clean way to allow clients to force a refresh when needed while defaulting to using the cache.
923-938: LGTM! Well-structured enums for scheduling.The enums for schedule frequency and days of the week are well-defined and cover all necessary cases.
961-970: LGTM! Clear and comprehensive Dashboard type definition.The Dashboard type provides all the necessary fields for managing cache settings and scheduling.
1084-1084: LGTM! New dashboard query.This query endpoint will allow clients to fetch the detailed dashboard information including cached items.
1216-1217: LGTM! Well-structured mutations for cache management.The updated return type for
previewItemSQLand the newsetDashboardSchedulemutation are well-designed, providing all necessary information for cache management.
| input SetDashboardScheduleInput { | ||
| cacheEnabled: Boolean! | ||
| schedule: SetDashboardScheduleData! | ||
| } | ||
|
|
||
| type DashboardSchedule { | ||
| frequency: ScheduleFrequencyEnum! | ||
| hour: Int! | ||
| minute: Int! | ||
| day: CacheScheduleDayEnum | ||
| timezone: String | ||
| cron: String | ||
| } | ||
|
|
||
| input SetDashboardScheduleData { | ||
| frequency: ScheduleFrequencyEnum! | ||
| hour: Int | ||
| minute: Int | ||
| day: CacheScheduleDayEnum | ||
| timezone: String | ||
| cron: String | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistency between input and output schedule types.
There's an inconsistency between DashboardSchedule (output) and SetDashboardScheduleData (input):
- In the output type,
hourandminuteare required fields. - In the input type, they are optional.
This could lead to confusion when implementing resolvers that must handle these potentially missing fields.
Consider either:
- Making both consistent (both required or both optional)
- Adding validations in the resolver to ensure
hourandminuteare provided when needed - Renaming for clarity (e.g.,
DashboardScheduleInputandDashboardSchedule)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
wren-ui/src/utils/validator/cronValidator.ts (1)
7-13: Document the unused parameter and its purposeThe
cronValidatorfunction takes a first parameter_that is unused. Consider documenting why this parameter exists but is unused, particularly for developers not familiar with the form validation library's contract.-export const cronValidator = (_, value: string) => { +/** + * Validates a cron expression for form validation + * @param _ - Unused parameter (required by form validator interface) + * @param value - The cron expression string to validate + * @returns A Promise that resolves if valid or rejects with error message if invalid + */ +export const cronValidator = (_, value: string) => { if (!value) return Promise.reject(ERROR_TEXTS.CRON.REQUIRED); if (!isValidCron(value)) { return Promise.reject(ERROR_TEXTS.CRON.INVALID); } return Promise.resolve(); };wren-ui/src/pages/home/dashboard.tsx (1)
22-22: Consider adding null checking for the dashboard grid refWhile the current optional chaining approach works, consider adding more robust error handling for accessing the
onRefreshAllmethod.- const dashboardGridRef = useRef<{ onRefreshAll: () => void }>(null); + const dashboardGridRef = useRef<{ onRefreshAll: () => void }>(null); + + const refreshAllDashboardItems = () => { + if (dashboardGridRef.current) { + dashboardGridRef.current.onRefreshAll(); + } else { + console.warn('Dashboard grid reference not available'); + } + };wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx (3)
12-17: Add JSDoc documentation for better code understandingAdd proper documentation to the props interface to explain each property's purpose and requirements.
+/** + * Props for the DashboardHeader component + */ interface Props { + /** ISO timestamp string for the next scheduled cache refresh */ nextScheduleTime?: string; + /** Schedule configuration object containing frequency and timing details */ schedule?: Schedule; + /** Callback fired when the cache settings action is triggered */ onCacheSettings?: () => void; + /** Callback fired when the refresh all action is triggered */ onRefreshAll?: () => void; }
42-47: Replace empty div with proper CSS for spacingThe empty div at line 44 is being used as a spacer. Consider using CSS for proper layout alignment.
- <StyledHeader> - <div /> - <div> + <StyledHeader> + <div>Update the StyledHeader styled component:
const StyledHeader = styled.div` display: flex; align-items: center; - justify-content: space-between; + justify-content: flex-end; min-height: 49px; padding: 8px 16px; background-color: white; border-bottom: 1px solid var(--gray-4); `;
72-74: Add accessibility attributes to the dropdown buttonThe dropdown button lacks proper accessibility attributes like aria-label to help screen reader users understand its purpose.
- <Button type="text" icon={<MoreIcon />}></Button> + <Button + type="text" + icon={<MoreIcon />} + aria-label="Dashboard cache options" + title="Dashboard cache options" + ></Button>wren-ui/src/components/pages/home/dashboardGrid/index.tsx (2)
286-288: Hard-codedlastRefreshTimeplaceholder will surface in productionA fixed ISO string will mislead users once the feature ships.
Until the backend field arrives, at least guard it behind a feature flag or leave it empty.
292-298: Hook order feels okay but warrants a sanity check
useImperativeHandlecloses overpreviewItemSQL, which is declared after the hook call. Though JavaScript captures by reference and the later assignment is visible, the unusual ordering can be confusing.Consider moving the
usePreviewItemSqlMutationdeclaration aboveuseImperativeHandlefor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
wren-ui/package.json(4 hunks)wren-ui/src/components/diagram/CustomDropdown.tsx(3 hunks)wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx(1 hunks)wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx(1 hunks)wren-ui/src/components/pages/home/dashboardGrid/index.tsx(4 hunks)wren-ui/src/import/antd.ts(1 hunks)wren-ui/src/pages/home/dashboard.tsx(3 hunks)wren-ui/src/styles/antd-variables.less(1 hunks)wren-ui/src/utils/enum/dropdown.ts(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)wren-ui/src/utils/time.ts(1 hunks)wren-ui/src/utils/validator/cronValidator.ts(1 hunks)wren-ui/src/utils/validator/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- wren-ui/src/utils/validator/index.ts
- wren-ui/src/utils/time.ts
- wren-ui/src/utils/enum/dropdown.ts
- wren-ui/src/styles/antd-variables.less
- wren-ui/src/utils/error/dictionary.ts
- wren-ui/src/import/antd.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/package.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
wren-ui/src/utils/validator/cronValidator.ts (1)
wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-153)
wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx (4)
wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx (2)
Schedule(27-34)getScheduleText(98-129)wren-ui/src/utils/time.ts (1)
getCompactTime(21-23)wren-ui/src/components/diagram/CustomDropdown.tsx (1)
DashboardDropdown(137-162)wren-ui/src/utils/icons.ts (1)
MoreIcon(90-90)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
wren-ui/src/components/diagram/CustomDropdown.tsx (3)
11-11: LGTM - Import for new icon added correctlyThe DatabaseOutlined icon import is properly added to support the new cache settings UI element.
137-162: LGTM - New DashboardDropdown follows established patternsThe new DashboardDropdown component correctly follows the existing pattern using the makeDropdown factory function and properly implements the cache management actions.
186-186: LGTM - Label update improves clarityChanging the label from "Refresh" to "Refresh cache" improves clarity about what's being refreshed.
wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx (1)
19-27: LGTM - Clean styled component implementationThe styling follows the project's conventions and provides a clean layout for the header.
wren-ui/src/components/pages/home/dashboardGrid/index.tsx (1)
162-172: Missing dependency onitemRefsbreaksonRefreshAllafter items update
useImperativeHandlecapturesitemRefs.currentonly once.
Ifitemschange (add/remove items) the exposedonRefreshAllkeeps the old snapshot and skips new refs.Add
itemRefs.current(or simplyObject.keys(itemRefs.current).length) to the dependency list.- }, [items]); + }, [items, itemRefs.current]);wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx (3)
116-118: Possible undefined access whendayis absent
capitalize(schedule.day.toLowerCase())assumesschedule.dayis defined.
For safety:- return `Cache refreshes every ${capitalize(schedule.day.toLowerCase())} at ${time}`; + return schedule.day + ? `Cache refreshes every ${capitalize(schedule.day.toLowerCase())} at ${time}` + : '';
165-175: 🛠️ Refactor suggestion
getNextScheduleByCronuses a non-existentparsemethodAfter fixing the import, use
cronParser.parseExpression:- try { - const interval = CronExpressionParser.parse(cron, { tz: 'UTC' }); + try { + const interval = cronParser.parseExpression(cron, { tz: 'UTC' });Also, consider caching the parser instance or surrounding with stricter error handling to avoid unhandled exceptions on invalid cron strings.
⛔ Skipped due to learnings
Learnt from: andreashimin PR: Canner/WrenAI#1587 File: wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx:3-4 Timestamp: 2025-04-23T09:21:38.365Z Learning: The cron-parser package (v5+) provides a named export `CronExpressionParser` with a `parse` method that should be used like: `import { CronExpressionParser } from 'cron-parser'; const interval = CronExpressionParser.parse(cronExpression, options);`
3-4:⚠️ Potential issueIncorrect import –
cron-parserhas no named exportCronExpressionParser
cron-parserexposesparseExpression(and is typically imported as the default or namespace).
This import will beundefinedat runtime and the subsequent.parsecall will throw.-import { CronExpressionParser } from 'cron-parser'; +import * as cronParser from 'cron-parser';⛔ Skipped due to learnings
Learnt from: andreashimin PR: Canner/WrenAI#1587 File: wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx:3-4 Timestamp: 2025-04-23T09:21:38.365Z Learning: The cron-parser package (v5+) provides a named export `CronExpressionParser` with a `parse` method that should be used like: `import { CronExpressionParser } from 'cron-parser'; const interval = CronExpressionParser.parse(cronExpression, options);`
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
wren-ui/src/apollo/server/services/dashboardService.ts (3)
387-404: The cron expression generation logic could be moved to a utility fileThis method is a good candidate for extraction to improve modularity.
Cron expression utilities like this might be needed in other parts of the application as it grows.
466-520: Consider extractingparseCronExpressionto a utility fileThis method contains complex cron parsing logic that could be useful elsewhere.
Extracting cron-related functions to a dedicated utility file would improve code organization and reusability.
146-146:⚠️ Potential issueGuard against null dashboards
Spreading
nullresults in{}which silently hides errors and may cause "undefined property" crashes elsewhere.- return { ...dashboard }; + if (!dashboard) { + throw new Error(`Dashboard for project ${project.id} not found`); + } + return { ...dashboard };wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts (2)
16-17: Consider adjusting logger level for productionSetting the logger level to 'debug' may generate excessive logs in production.
-logger.level = 'debug'; +logger.level = process.env.NODE_ENV === 'production' ? 'info' : 'debug';
111-162: Consider implementing batch processing for dashboard itemsUsing
Promise.allfor all items might cause performance issues with a large number of items.Implement a batching mechanism to process dashboard items in smaller groups, preventing potential resource exhaustion:
// Example batching implementation const batchSize = 5; const itemBatches = []; for (let i = 0; i < items.length; i += batchSize) { itemBatches.push(items.slice(i, i + batchSize)); } for (const batch of itemBatches) { await Promise.all( batch.map(async (item) => { // existing processing logic }) ); }
🧹 Nitpick comments (4)
wren-ui/src/apollo/server/services/dashboardService.ts (3)
256-317: Consider moving timezone conversion methods to a utility fileThe
toUTCmethod contains complex timezone conversion logic that would be better placed in a utility file for better testability and reuse.These timezone conversion methods are complex and could be reused elsewhere in the application. Consider extracting them to a dedicated utility file like
timezone-utils.ts.
319-385: Consider extractingtoTimezoneto a utility fileThis method has similar complexity to
toUTCand both handle timezone conversions that might be needed elsewhere.Both timezone conversion methods contain similar logic with opposite operations. Extracting them to a utility file would improve modularity and testability.
445-463: Enhance validation error message for custom cron expressionsThe error message for minimum interval could be more specific.
- 'Custom cron expression must be at least 10 minutes apart', + `Custom cron expression must have intervals of at least 10 minutes (current: ${Math.floor(diff / 60000)} minutes)`,This would provide the user with more actionable information about how close they are to the minimum requirement.
wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts (1)
152-160: Consider using higher log level for refresh failuresDebug level might be too low for important refresh failures, making it harder to detect issues in production.
- logger.debug( + logger.info( `Error refreshing cache for item ${item.id}: ${error.message}`, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js(1 hunks)wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(5 hunks)wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(2 hunks)wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts(1 hunks)wren-ui/src/apollo/server/models/dashboard.ts(1 hunks)wren-ui/src/apollo/server/repositories/dashboardCacheRefreshRepository.ts(1 hunks)wren-ui/src/apollo/server/resolvers/dashboardResolver.ts(4 hunks)wren-ui/src/apollo/server/schema.ts(4 hunks)wren-ui/src/apollo/server/services/dashboardService.ts(5 hunks)wren-ui/src/apollo/server/services/queryService.ts(6 hunks)wren-ui/src/apollo/server/types/context.ts(3 hunks)wren-ui/src/common.ts(6 hunks)wren-ui/src/pages/api/graphql.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- wren-ui/src/apollo/server/repositories/dashboardCacheRefreshRepository.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- wren-ui/src/pages/api/graphql.ts
- wren-ui/src/apollo/server/types/context.ts
- wren-ui/src/apollo/server/models/dashboard.ts
- wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts
- wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js
- wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts
- wren-ui/src/apollo/server/resolvers/dashboardResolver.ts
- wren-ui/src/common.ts
- wren-ui/src/apollo/server/services/queryService.ts
- wren-ui/src/apollo/server/schema.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
| switch (dashboard.scheduleFrequency) { | ||
| case ScheduleFrequencyEnum.CUSTOM: | ||
| return { | ||
| frequency: ScheduleFrequencyEnum.CUSTOM, | ||
| hour: 0, | ||
| minute: 0, | ||
| day: null, | ||
| timezone: dashboard.scheduleTimezone || '', | ||
| cron: dashboard.scheduleCron, | ||
| }; | ||
| case ScheduleFrequencyEnum.DAILY: | ||
| case ScheduleFrequencyEnum.WEEKLY: { | ||
| const parts = dashboard.scheduleCron.split(' '); | ||
| if (parts.length !== 5) { | ||
| throw new Error('Invalid cron expression format'); | ||
| } | ||
| const [minute, hour, , , dayOfWeek] = parts; | ||
| return this.toTimezone({ | ||
| frequency: dashboard.scheduleFrequency, | ||
| hour: parseInt(hour, 10), | ||
| minute: parseInt(minute, 10), | ||
| day: | ||
| dashboard.scheduleFrequency === ScheduleFrequencyEnum.WEEKLY | ||
| ? (dayOfWeek as CacheScheduleDayEnum) | ||
| : null, | ||
| timezone: dashboard.scheduleTimezone || '', | ||
| cron: null, | ||
| } as DashboardSchedule); | ||
| } | ||
| case ScheduleFrequencyEnum.NEVER: { | ||
| return { | ||
| frequency: ScheduleFrequencyEnum.NEVER, | ||
| hour: null, | ||
| minute: null, | ||
| day: null, | ||
| timezone: dashboard.scheduleTimezone || '', | ||
| cron: null, | ||
| } as DashboardSchedule; | ||
| } | ||
| default: { |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add test cases for edge cases in cron expression parsing
The switch statement handles different schedule frequencies, but edge cases should be thoroughly tested.
Please verify that all edge cases are covered in tests, particularly:
- What happens with malformed cron expressions
- Handling of timezone edge cases when crossing day boundaries
- Conversion between different schedule frequencies
🏁 Script executed:
#!/bin/bash
# Check if there are tests for the parseCronExpression method
rg -A 3 "test.*parseCronExpression" --type tsLength of output: 47
Add comprehensive tests for cron expression parsing in dashboardService
It appears there are no existing tests covering the inline cron‐parsing logic in dashboardService.ts. We should add specs for:
- Malformed cron strings (e.g. wrong field count, non-numeric values)
- Timezone conversions around day boundaries (UTC→local and vice versa)
- Frequency conversions (CUSTOM, DAILY, WEEKLY, NEVER) and their expected outputs
Areas to cover:
- The
dashboard.scheduleCron.split(' ')path in DAILY/WEEKLY - The error path when
parts.length !== 5 toTimezone(...)adjustments for WEEKLY dayOfWeek
Please add these test cases under the existing service‐spec file or create a new dashboardService.spec.ts.
| private calculateNextRunTime(cronExpression: string): Date | null { | ||
| try { | ||
| const interval = CronExpressionParser.parse(cronExpression, { | ||
| currentDate: new Date(), | ||
| }); | ||
| return interval.next().toDate(); | ||
| } catch (error) { | ||
| logger.error(`Failed to parse cron expression: ${error.message}`); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract calculateNextRunTime to a shared utility
This method duplicates logic that also exists in the dashboard service.
This method is identical to the one in dashboardService.ts. Consider extracting it to a shared utility file to avoid duplication and ensure consistent behavior.
| } | ||
| } | ||
|
|
||
| private async refreshDashboardCache(dashboard: any): Promise<void> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with a proper type for dashboard parameter
Using any type reduces type safety and can lead to runtime errors.
- private async refreshDashboardCache(dashboard: any): Promise<void> {
+ private async refreshDashboardCache(dashboard: { id: number; scheduleCron: string }): Promise<void> {Or preferably use the Dashboard type from your repositories:
- private async refreshDashboardCache(dashboard: any): Promise<void> {
+ private async refreshDashboardCache(dashboard: Dashboard): Promise<void> {📝 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.
| private async refreshDashboardCache(dashboard: any): Promise<void> { | |
| private async refreshDashboardCache(dashboard: Dashboard): Promise<void> { |
| private async refreshDashboardCache(dashboard: any): Promise<void> { | |
| private async refreshDashboardCache(dashboard: { id: number; scheduleCron: string }): Promise<void> { |
| constructor({ | ||
| dashboardRepository, | ||
| dashboardItemRepository, | ||
| dashboardItemRefreshJobRepository, | ||
| projectService, | ||
| deployService, | ||
| queryService, | ||
| }: { | ||
| dashboardRepository: IDashboardRepository; | ||
| dashboardItemRepository: IDashboardItemRepository; | ||
| dashboardItemRefreshJobRepository: IDashboardItemRefreshJobRepository; | ||
| projectService: IProjectService; | ||
| deployService: IDeployService; | ||
| queryService: IQueryService; | ||
| }) { | ||
| this.dashboardRepository = dashboardRepository; | ||
| this.dashboardItemRepository = dashboardItemRepository; | ||
| this.dashboardItemRefreshJobRepository = dashboardItemRefreshJobRepository; | ||
| this.projectService = projectService; | ||
| this.deployService = deployService; | ||
| this.queryService = queryService; | ||
| this.intervalTime = 60000; // 1 minute | ||
| this.start(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a method to stop the background tracker
The tracker starts in the constructor but has no way to stop it, which can cause issues during testing or application shutdown.
+ private interval: NodeJS.Timeout;
constructor({
// ...existing code
}) {
// ...existing code
- this.start();
}
+ public start(): void {
+ if (this.interval) {
+ clearInterval(this.interval);
+ }
+ logger.info('Dashboard cache background tracker started');
+ this.interval = setInterval(() => {
+ this.checkAndRefreshCaches();
+ }, this.intervalTime);
+ }
+
+ public stop(): void {
+ if (this.interval) {
+ clearInterval(this.interval);
+ this.interval = null;
+ logger.info('Dashboard cache background tracker stopped');
+ }
+ }
- private start(): void {
- logger.info('Dashboard cache background tracker started');
- setInterval(() => {
- this.checkAndRefreshCaches();
- }, this.intervalTime);
- }📝 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.
| constructor({ | |
| dashboardRepository, | |
| dashboardItemRepository, | |
| dashboardItemRefreshJobRepository, | |
| projectService, | |
| deployService, | |
| queryService, | |
| }: { | |
| dashboardRepository: IDashboardRepository; | |
| dashboardItemRepository: IDashboardItemRepository; | |
| dashboardItemRefreshJobRepository: IDashboardItemRefreshJobRepository; | |
| projectService: IProjectService; | |
| deployService: IDeployService; | |
| queryService: IQueryService; | |
| }) { | |
| this.dashboardRepository = dashboardRepository; | |
| this.dashboardItemRepository = dashboardItemRepository; | |
| this.dashboardItemRefreshJobRepository = dashboardItemRefreshJobRepository; | |
| this.projectService = projectService; | |
| this.deployService = deployService; | |
| this.queryService = queryService; | |
| this.intervalTime = 60000; // 1 minute | |
| this.start(); | |
| } | |
| // add a field to keep track of the interval | |
| private interval: NodeJS.Timeout; | |
| constructor({ | |
| dashboardRepository, | |
| dashboardItemRepository, | |
| dashboardItemRefreshJobRepository, | |
| projectService, | |
| deployService, | |
| queryService, | |
| }: { | |
| dashboardRepository: IDashboardRepository; | |
| dashboardItemRepository: IDashboardItemRepository; | |
| dashboardItemRefreshJobRepository: IDashboardItemRefreshJobRepository; | |
| projectService: IProjectService; | |
| deployService: IDeployService; | |
| queryService: IQueryService; | |
| }) { | |
| this.dashboardRepository = dashboardRepository; | |
| this.dashboardItemRepository = dashboardItemRepository; | |
| this.dashboardItemRefreshJobRepository = dashboardItemRefreshJobRepository; | |
| this.projectService = projectService; | |
| this.deployService = deployService; | |
| this.queryService = queryService; | |
| this.intervalTime = 60000; // 1 minute | |
| // removed automatic start here to defer to the new public start() | |
| } | |
| public start(): void { | |
| if (this.interval) { | |
| clearInterval(this.interval); | |
| } | |
| logger.info('Dashboard cache background tracker started'); | |
| this.interval = setInterval(() => { | |
| this.checkAndRefreshCaches(); | |
| }, this.intervalTime); | |
| } | |
| public stop(): void { | |
| if (this.interval) { | |
| clearInterval(this.interval); | |
| this.interval = null; | |
| logger.info('Dashboard cache background tracker stopped'); | |
| } | |
| } | |
| // removed the old private start(), since it's been promoted to public start() |
onlyjackfrost
force-pushed
the
feat/query-cache
branch
from
8257de6 to
628bbad
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
wren-ui/src/apollo/server/services/tests/dashboardService.test.ts (1)
40-62: Reduce duplication by re-usingcreateScheduleDataMany tests manually craft
DashboardScheduleobjects, duplicating defaults and, as seen, sometimes introducing type errors. UsingcreateScheduleData(or extracting a similar factory for raw schedules) would:
- centralise default handling,
- enforce type-correct values,
- cut ~100 LOC of repetitive setup.
This also future-proofs tests against interface changes.
wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx (2)
153-153: Fix typo in commentThere's a typo in the comment: "postpond" should be "postpone".
- // postpond the time if it's already passed + // postpone the time if it's already passed
207-228: Improve error handling during form submissionCurrently, errors during form submission are only logged to the console, which provides no feedback to users. Consider adding a notification or error message in the UI.
const submit = () => { form .validateFields() .then(async (values) => { const { schedule } = values; - await onSubmit({ - ...values, - schedule: values.enabled - ? { - frequency: schedule?.frequency, - day: schedule?.day, - hour: schedule?.time?.hour(), - minute: schedule?.time?.minute(), - cron: schedule?.cron, - timezone: browserTimeZone, - } - : null, - }); - onClose(); + try { + await onSubmit({ + ...values, + schedule: values.enabled + ? { + frequency: schedule?.frequency, + day: schedule?.day, + hour: schedule?.time?.hour(), + minute: schedule?.time?.minute(), + cron: schedule?.cron, + timezone: browserTimeZone, + } + : null, + }); + onClose(); + } catch (error) { + console.error(error); + // Add UI notification here, e.g.: + // notification.error({ message: 'Failed to save settings', description: error.message }); + } }) .catch(console.error); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (34)
wren-ui/migrations/20250422000000_alter_dashboard_table.js(1 hunks)wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js(1 hunks)wren-ui/package.json(4 hunks)wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(5 hunks)wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(2 hunks)wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts(1 hunks)wren-ui/src/apollo/server/backgrounds/index.ts(1 hunks)wren-ui/src/apollo/server/models/dashboard.ts(1 hunks)wren-ui/src/apollo/server/models/index.ts(1 hunks)wren-ui/src/apollo/server/repositories/dashboardCacheRefreshRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/dashboardRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/index.ts(1 hunks)wren-ui/src/apollo/server/resolvers.ts(2 hunks)wren-ui/src/apollo/server/resolvers/dashboardResolver.ts(4 hunks)wren-ui/src/apollo/server/schema.ts(4 hunks)wren-ui/src/apollo/server/services/dashboardService.ts(5 hunks)wren-ui/src/apollo/server/services/queryService.ts(6 hunks)wren-ui/src/apollo/server/services/tests/dashboardService.test.ts(1 hunks)wren-ui/src/apollo/server/types/context.ts(3 hunks)wren-ui/src/apollo/server/utils/timezone.ts(1 hunks)wren-ui/src/common.ts(6 hunks)wren-ui/src/components/diagram/CustomDropdown.tsx(3 hunks)wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx(1 hunks)wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx(1 hunks)wren-ui/src/components/pages/home/dashboardGrid/index.tsx(4 hunks)wren-ui/src/import/antd.ts(1 hunks)wren-ui/src/pages/api/graphql.ts(3 hunks)wren-ui/src/pages/home/dashboard.tsx(3 hunks)wren-ui/src/styles/antd-variables.less(1 hunks)wren-ui/src/utils/enum/dropdown.ts(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)wren-ui/src/utils/time.ts(1 hunks)wren-ui/src/utils/validator/cronValidator.ts(1 hunks)wren-ui/src/utils/validator/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- wren-ui/src/utils/validator/cronValidator.ts
- wren-ui/src/import/antd.ts
🚧 Files skipped from review as they are similar to previous changes (30)
- wren-ui/src/utils/time.ts
- wren-ui/src/utils/validator/index.ts
- wren-ui/src/apollo/server/models/index.ts
- wren-ui/package.json
- wren-ui/src/utils/enum/dropdown.ts
- wren-ui/src/styles/antd-variables.less
- wren-ui/src/apollo/server/repositories/index.ts
- wren-ui/src/apollo/server/utils/timezone.ts
- wren-ui/src/apollo/server/types/context.ts
- wren-ui/src/pages/api/graphql.ts
- wren-ui/src/apollo/server/repositories/dashboardRepository.ts
- wren-ui/src/components/diagram/CustomDropdown.tsx
- wren-ui/src/pages/home/dashboard.tsx
- wren-ui/migrations/20250422000000_alter_dashboard_table.js
- wren-ui/src/apollo/server/backgrounds/index.ts
- wren-ui/src/common.ts
- wren-ui/src/utils/error/dictionary.ts
- wren-ui/src/apollo/server/repositories/dashboardCacheRefreshRepository.ts
- wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js
- wren-ui/src/apollo/server/models/dashboard.ts
- wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts
- wren-ui/src/apollo/server/services/queryService.ts
- wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts
- wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts
- wren-ui/src/apollo/server/resolvers.ts
- wren-ui/src/apollo/server/resolvers/dashboardResolver.ts
- wren-ui/src/components/pages/home/dashboardGrid/index.tsx
- wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx
- wren-ui/src/apollo/server/schema.ts
- wren-ui/src/apollo/server/services/dashboardService.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
wren-ui/src/apollo/server/services/tests/dashboardService.test.ts (2)
wren-ui/src/apollo/server/services/dashboardService.ts (1)
DashboardService(60-521)wren-ui/src/apollo/server/models/dashboard.ts (2)
DashboardSchedule(28-35)SetDashboardCacheData(37-40)
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/tests/dashboardService.test.ts
[error] 30-30: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wren-ui/src/apollo/server/services/tests/dashboardService.test.ts (1)
152-169: Potential timezone-dependent flakiness in date assertionThe expected timestamp is derived from
new Date()without freezing the clock or forcing a timezone. On CI runners that are not UTC,cron-parserandDatemay interpret12:00differently, leading to sporadic failures.Consider:
-const expectedTime = new Date(); -expectedTime.setHours(12, 0, 0, 0); +// Freeze time to a known instant to remove host-timezone influence +jest.useFakeTimers().setSystemTime(new Date('2024-04-22T00:00:00Z')); +const expectedTime = new Date('2024-04-22T12:00:00Z'); // explicit UTCRemember to call
jest.useRealTimers()inafterEach. This makes the assertion deterministic across environments.wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx (2)
165-175: Good implementation of cron parsing with timezone handlingThe function correctly handles cron expressions, using UTC for parsing (which aligns with server-side scheduling) and converting to local time for display. It also properly validates the cron expression and handles errors gracefully.
314-321: Good implementation of next schedule displayThe UI thoughtfully shows the next scheduled refresh time with the user's timezone, providing clear feedback about when the cache will be refreshed next. This helps users understand the impact of their schedule configuration.
| describe('generateCronExpression', () => { | ||
| it('should generate correct cron expression for daily schedule', () => { | ||
| const schedule = { | ||
| frequency: ScheduleFrequencyEnum.DAILY, | ||
| hour: 14, | ||
| minute: 30, | ||
| timezone: '', | ||
| day: null, | ||
| cron: null, | ||
| }; | ||
|
|
||
| const result = dashboardService.testGenerateCronExpression(schedule); | ||
| expect(result).toBe('30 14 * * *'); | ||
| }); | ||
|
|
||
| it('should generate correct cron expression for weekly schedule', () => { | ||
| const schedule = { | ||
| frequency: ScheduleFrequencyEnum.WEEKLY, | ||
| day: CacheScheduleDayEnum.MON, | ||
| hour: 9, | ||
| minute: 0, | ||
| timezone: '', | ||
| cron: null, | ||
| }; | ||
|
|
||
| const result = dashboardService.testGenerateCronExpression(schedule); | ||
| expect(result).toBe('0 9 * * MON'); | ||
| }); | ||
|
|
||
| it('should return custom cron expression for custom schedule', () => { | ||
| const schedule = { | ||
| frequency: ScheduleFrequencyEnum.CUSTOM, | ||
| cron: '0 0 * * *', | ||
| timezone: '', | ||
| day: null, | ||
| hour: 0, | ||
| minute: 0, | ||
| }; | ||
|
|
||
| const result = dashboardService.testGenerateCronExpression(schedule); | ||
| expect(result).toBe('0 0 * * *'); | ||
| }); | ||
|
|
||
| it('should return null for never schedule', () => { | ||
| const schedule = { | ||
| frequency: ScheduleFrequencyEnum.NEVER, | ||
| timezone: '', | ||
| day: null, | ||
| hour: 0, | ||
| minute: 0, | ||
| cron: null, | ||
| }; | ||
|
|
||
| const result = dashboardService.testGenerateCronExpression(schedule); | ||
| expect(result).toBeNull(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
null values break the DashboardSchedule contract
DashboardSchedule marks day and cron as required, non-nullable strings/enums:
day: CacheScheduleDayEnum;
cron: string;Directly constructing objects with
day: null or cron: null (e.g. lines 90-97, 104-111, 132-139, 196-205, 418-425) causes TypeScript compile errors and defeats strict-null checks.
Prefer one of the following:
- Use the already-defined
createScheduleDatahelper – it always supplies valid defaults. - If you must inline objects, provide dummy but type-correct values:
- day: null,
- cron: null,
+ day: CacheScheduleDayEnum.MON, // ignored for DAILY/CUSTOM
+ cron: '', // ignored when not CUSTOMRefactor all occurrences to keep the test suite compiling under strictNullChecks.
Also applies to: 196-206, 417-427
| public testValidateScheduleInput(data: SetDashboardCacheData): void { | ||
| return this.validateScheduleInput(data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid returning a value from a void-typed helper
validateScheduleInput returns void, yet the wrapper method returns its (non-existent) value. Besides triggering the Biome warning, this can confuse future readers about the intent.
- public testValidateScheduleInput(data: SetDashboardCacheData): void {
- return this.validateScheduleInput(data);
- }
+ public testValidateScheduleInput(data: SetDashboardCacheData): void {
+ // simply delegate – nothing to return
+ this.validateScheduleInput(data);
+ }📝 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.
| public testValidateScheduleInput(data: SetDashboardCacheData): void { | |
| return this.validateScheduleInput(data); | |
| } | |
| public testValidateScheduleInput(data: SetDashboardCacheData): void { | |
| // simply delegate – nothing to return | |
| this.validateScheduleInput(data); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js (1)
9-9: Consider using an enum for the status fieldThe inline comment indicates this field has specific valid values ('success', 'failed', 'in_progress'). Consider adding a CHECK constraint to ensure only these values can be inserted, matching the enum defined in the repository.
- table.string('status').notNullable(); // 'success', 'failed', 'in_progress' + table.string('status').notNullable().checkIn(['success', 'failed', 'in_progress']); // Enforces valid status values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js(1 hunks)wren-ui/src/apollo/server/repositories/dashboardItemRefreshJobRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/src/apollo/server/repositories/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
wren-ui/src/apollo/server/repositories/dashboardItemRefreshJobRepository.ts (1)
wren-ui/src/apollo/server/repositories/baseRepository.ts (2)
IBasicRepository(10-42)BaseRepository(44-185)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
wren-ui/src/apollo/server/repositories/dashboardItemRefreshJobRepository.ts (1)
1-33: LGTM - Well-structured repository implementationThe repository follows the established pattern in the codebase, with clear naming and proper inheritance from BaseRepository. The enum, interface, and class definitions are well-organized and provide a complete representation of dashboard item refresh jobs.
wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js (2)
1-31: Table schema looks solid with proper constraintsThe migration creates a well-structured table with appropriate fields, constraints, and indexes for tracking dashboard item refresh jobs. Good job adding the comment on the hash field to explain its purpose.
33-35: LGTM - Clean down migrationThe down migration properly drops the table to revert the changes.
- Implemented `setDashboardSchedule` resolver to manage dashboard scheduling and caching settings. - Added migration to alter the dashboard table for new scheduling fields. - Updated relevant services and models to support caching and scheduling logic. - Enhanced IbisAdaptor to handle cache-related query parameters.
- Added `schedule_timezone` field to the dashboard table migration. - Updated GraphQL schema to reflect changes in the `previewItemSQL` return type. - Modified `IbisAdaptor` to correctly handle cache-related headers. - Refactored `dashboardRepository` and `dashboardService` to utilize the new `scheduleTimezone` field. - Adjusted `dashboardResolver` to format `nextScheduledAt` correctly.
- Set default value for `refresh` in `PreviewItemSQLInput` to `false`. - Updated `SetDashboardScheduleData` to allow optional `hour` and `minute` fields. - Standardized `ScheduleFrequencyEnum` values to uppercase. - Introduced `Dashboard` type in GraphQL schema for enhanced dashboard management. - Modified `setDashboardSchedule` resolver to return the updated `Dashboard` object. - Adjusted `dashboardService` to handle new scheduling logic and return types. - Updated tests to reflect changes in scheduling frequency constants and input handling.
- Added migration to create `dashboard_cache_refresh` table for tracking cache refresh jobs. - Introduced `DashboardCacheRefreshRepository` for managing cache refresh records. - Implemented `DashboardCacheBackgroundTracker` to periodically refresh dashboard caches based on scheduling. - Updated context and service layers to integrate new cache refresh functionality. - Enhanced GraphQL API to support the new caching features.
…otected - Updated methods in `DashboardService` to be `protected`, allowing access in subclasses. - Introduced a `TestDashboardService` class in tests to facilitate unit testing of these methods. - Adjusted test cases to utilize the new test class for invoking protected methods directly.
- Replaced sequential processing of cache refresh jobs with concurrent execution using Promise.all.
onlyjackfrost
force-pushed
the
feat/query-cache
branch
from
8526943 to
34ba7b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
wren-ui/src/apollo/server/services/dashboardService.ts (2)
418-464: Move validation utilities to a dedicated moduleThe validation logic could be extracted to a utility file for better testability and reuse. This would allow it to be tested independently and potentially reused elsewhere.
As mentioned in previous comments, moving
validateScheduleInput(and possibly other helper methods likeparseCronExpressionandtoTimezone) to a utility module would improve maintainability and testability.
146-146:⚠️ Potential issueGuard against
nulldashboardsThe implementation spreads a potentially null dashboard object without checking, which could lead to runtime errors. This doesn't fully fix the issue noted in the previous review.
- return { ...dashboard }; + if (!dashboard) { + throw new Error(`Dashboard for project ${project.id} not found`); + } + return { ...dashboard };Fail fast to prevent subtle errors when a dashboard doesn't exist.
wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx (2)
1-4: Plan to migrate away from moment.jsMoment.js is in maintenance mode and no longer recommended for new development. Consider migrating to a more modern alternative like date-fns, luxon, or day.js in a future update.
117-117:⚠️ Potential issueFix potential runtime error with undefined day
The code assumes
schedule.dayis always defined in the WEEKLY frequency case, which could cause a runtime error.- return `Cache refreshes every ${capitalize(schedule.day.toLowerCase())} at ${time}`; + return `Cache refreshes every ${capitalize(schedule.day?.toLowerCase() || '')} at ${time}`;wren-ui/src/apollo/server/services/tests/dashboardService.test.ts (2)
29-31:⚠️ Potential issueFix return in void-typed function
The function returns a value despite having a void return type.
public testValidateScheduleInput(data: SetDashboardCacheData): void { - return this.validateScheduleInput(data); + this.validateScheduleInput(data); }🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
88-144:⚠️ Potential issueUpdate test data to match type constraints
The test data uses
nullvalues for properties that are defined as non-nullable in theDashboardScheduletype.Instead of using null for required fields like
dayandcron, provide dummy but type-correct values using thecreateScheduleDatahelper or with appropriate defaults.
🧹 Nitpick comments (1)
wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx (1)
165-175: Improve error handling in getNextScheduleByCronThe error is only logged to console but not communicated to the user. Consider adding more robust error handling.
try { const interval = CronExpressionParser.parse(cron, { tz: 'UTC' }); const targetTime = moment.utc(interval.next().toDate()).local(); return targetTime.isValid() ? targetTime.format('YYYY-MM-DD HH:mm') : null; } catch (error) { console.warn(error); + // Consider returning a specific error value or using a notification system + // to alert the user about cron parsing errors return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (34)
wren-ui/migrations/20250422000000_alter_dashboard_table.js(1 hunks)wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js(1 hunks)wren-ui/package.json(4 hunks)wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(5 hunks)wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(2 hunks)wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts(1 hunks)wren-ui/src/apollo/server/backgrounds/index.ts(1 hunks)wren-ui/src/apollo/server/models/dashboard.ts(1 hunks)wren-ui/src/apollo/server/models/index.ts(1 hunks)wren-ui/src/apollo/server/repositories/dashboardItemRefreshJobRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/dashboardRepository.ts(1 hunks)wren-ui/src/apollo/server/repositories/index.ts(1 hunks)wren-ui/src/apollo/server/resolvers.ts(2 hunks)wren-ui/src/apollo/server/resolvers/dashboardResolver.ts(4 hunks)wren-ui/src/apollo/server/schema.ts(4 hunks)wren-ui/src/apollo/server/services/dashboardService.ts(5 hunks)wren-ui/src/apollo/server/services/queryService.ts(6 hunks)wren-ui/src/apollo/server/services/tests/dashboardService.test.ts(1 hunks)wren-ui/src/apollo/server/types/context.ts(3 hunks)wren-ui/src/apollo/server/utils/timezone.ts(1 hunks)wren-ui/src/common.ts(6 hunks)wren-ui/src/components/diagram/CustomDropdown.tsx(3 hunks)wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx(1 hunks)wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx(1 hunks)wren-ui/src/components/pages/home/dashboardGrid/index.tsx(4 hunks)wren-ui/src/import/antd.ts(1 hunks)wren-ui/src/pages/api/graphql.ts(3 hunks)wren-ui/src/pages/home/dashboard.tsx(3 hunks)wren-ui/src/styles/antd-variables.less(1 hunks)wren-ui/src/utils/enum/dropdown.ts(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)wren-ui/src/utils/time.ts(1 hunks)wren-ui/src/utils/validator/cronValidator.ts(1 hunks)wren-ui/src/utils/validator/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- wren-ui/src/apollo/server/models/index.ts
- wren-ui/src/import/antd.ts
- wren-ui/src/apollo/server/backgrounds/index.ts
🚧 Files skipped from review as they are similar to previous changes (28)
- wren-ui/src/utils/validator/index.ts
- wren-ui/src/utils/time.ts
- wren-ui/src/apollo/server/repositories/index.ts
- wren-ui/src/utils/enum/dropdown.ts
- wren-ui/src/pages/api/graphql.ts
- wren-ui/src/styles/antd-variables.less
- wren-ui/src/utils/validator/cronValidator.ts
- wren-ui/src/apollo/server/resolvers.ts
- wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx
- wren-ui/src/apollo/server/repositories/dashboardRepository.ts
- wren-ui/src/utils/error/dictionary.ts
- wren-ui/package.json
- wren-ui/src/apollo/server/types/context.ts
- wren-ui/migrations/20250422000000_alter_dashboard_table.js
- wren-ui/src/apollo/server/utils/timezone.ts
- wren-ui/src/components/diagram/CustomDropdown.tsx
- wren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.ts
- wren-ui/src/apollo/server/repositories/dashboardItemRefreshJobRepository.ts
- wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts
- wren-ui/migrations/20250423000000_create_dashboard_cache_refresh_table.js
- wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts
- wren-ui/src/apollo/server/resolvers/dashboardResolver.ts
- wren-ui/src/common.ts
- wren-ui/src/pages/home/dashboard.tsx
- wren-ui/src/apollo/server/models/dashboard.ts
- wren-ui/src/components/pages/home/dashboardGrid/index.tsx
- wren-ui/src/apollo/server/services/queryService.ts
- wren-ui/src/apollo/server/schema.ts
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/tests/dashboardService.test.ts
[error] 30-30: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
wren-ui/src/apollo/server/services/dashboardService.ts (5)
79-126: Comprehensive implementation of dashboard schedulingThe implementation is thorough, handling input validation, dashboard verification, and conditional logic for caching state. Error handling is proper with try/catch and logging.
256-317: Well-implemented timezone conversion for UTCThe timezone conversion logic handles all edge cases including minute/hour carry-over and day adjustments for weekly schedules. Particularly impressed with handling edge cases for day boundaries.
319-385: Well-implemented conversion from UTC to timezoneThe implementation mirrors the UTC conversion logic but in reverse direction, maintaining consistency in how timezone offsets are applied. The code properly handles all edge cases.
387-404: Clean implementation of cron expression generationThe method correctly converts schedules to UTC before generating cron expressions and handles all schedule frequency types appropriately.
466-520: Comprehensive cron expression parsing logicThe implementation handles all schedule frequency types and properly parses cron parts for time-based schedules. The timezone conversion is correctly applied.
Please consider adding tests for malformed cron expressions as mentioned in a previous comment, especially for cases like invalid field counts and handling timezone edge cases.
wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx (3)
98-129: Well-structured schedule text generationThe implementation effectively handles all schedule frequency types, generating human-readable descriptions. The time formatting is consistent.
177-269: Clean implementation of the CacheSettingsDrawer componentThe component properly handles form state, validation, and form submission. The form reset on drawer close prevents stale data.
271-324: Well-designed Schedule componentThe Schedule component dynamically renders the appropriate input controls based on the selected frequency and calculates the next schedule preview.
wren-ui/src/apollo/server/services/tests/dashboardService.test.ts (5)
40-62: Well-designed test helper for creating schedule dataThe
createScheduleDatahelper function is well-designed, providing sensible defaults and flexibility for test cases.
146-217: Comprehensive tests for calculateNextRunTimeThe tests thoroughly cover different schedule types, invalid inputs, and timezone handling.
219-405: Excellent timezone conversion testsThe tests are comprehensive, covering various edge cases including day boundary transitions, timezone offsets, and weekly schedule day adjustments.
595-774: Thorough validation testingThe tests verify all validation rules, error messages, and valid input scenarios.
777-975: Complete test coverage for setDashboardScheduleThe tests verify database interactions, error handling, and all schedule types.
| "private": true, | ||
| "scripts": { | ||
| "dev": "next dev", | ||
| "dev": "TZ=UTC next dev", |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
wren-ui/src/components/pages/home/dashboardGrid/index.tsx (2)
147-149:forwardRefparameter type still incorrect
This was flagged earlier: the second parameter offorwardRefis aRef<{ … }>– not aRefObject.
156-162: Recreate item-refs only when needed & prune stale ones
Re-initialising every ref on eachitemschange churns memory and breaks existing imperative handles. The previous review suggested a fix that is still outstanding.- useEffect(() => { - items.forEach((item) => { - itemRefs.current[item.id] = createRef(); - }); - }, [items]); + useEffect(() => { + const next: typeof itemRefs.current = {}; + items.forEach((item) => { + next[item.id] = itemRefs.current[item.id] ?? createRef(); + }); + // remove refs for deleted items + Object.keys(itemRefs.current).forEach((key) => { + if (!next[key]) delete itemRefs.current[key]; + }); + itemRefs.current = next; + }, [items]);
🧹 Nitpick comments (6)
wren-ui/src/apollo/client/graphql/dashboard.ts (2)
70-75: Consider returning only the fields the UI really needs
datamay contain a large result-set for some items. By always requesting the full payload together with the cache-metadata you increase network traffic and GC pressure on the client. If the UI only needs the first N rows, summary counts, etc., expose a dedicated field (or argument) instead of the entire result and request that here.
79-92: Extract aScheduleFieldsfragment to avoid duplicated selections
The groups of fields (scheduleFrequency,scheduleTimezone,scheduleCron,nextScheduledAt, …) are repeated inSET_DASHBOARD_SCHEDULEandDASHBOARD. A small fragment keeps the query documents shorter and guarantees both places stay in sync.+export const SCHEDULE_FIELDS = gql` + fragment ScheduleFields on Dashboard { + cacheEnabled + scheduleFrequency + scheduleTimezone + scheduleCron + nextScheduledAt + } +`; + export const SET_DASHBOARD_SCHEDULE = gql` mutation SetDashboardSchedule($data: SetDashboardScheduleInput!) { setDashboardSchedule(data: $data) { id projectId name - cacheEnabled - scheduleFrequency - scheduleTimezone - scheduleCron - nextScheduledAt + ...ScheduleFields } } ${SCHEDULE_FIELDS} `;wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx (2)
41-47:onMoreClickdoesn’t perform any awaits – drop theasynckeyword
The function body never awaits, therefore the returned Promise is unnecessary and may mislead readers into assuming asynchronous work happens here.- const onMoreClick = async (action: MORE_ACTION) => { + const onMoreClick = (action: MORE_ACTION) => {
55-81: Remove the unnecessary fragment – static-analysis warning
Biome correctly reports that the fragment adds no value because it only wraps a single<Tooltip>/scheduleTimeelement. Removing it saves a few bytes and silences the linter.- <> {nextScheduleTime ? ( <Tooltip @@ scheduleTime - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 56-80: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
wren-ui/src/pages/home/dashboard.tsx (1)
61-67: Surface mutation errors to the user
setDashboardScheduleonly logs errors to the console. A toast similar to the success case (“Failed to update dashboard schedule”) would give immediate feedback in production.- onError: (error) => console.error(error), + onError: () => { + message.error('Failed to update dashboard schedule.'); + },wren-ui/src/apollo/client/graphql/__types__.ts (1)
378-387: Consider making schedule field optional in DetailedDashboard.The
schedulefield in DetailedDashboard is not marked as optional, but there might be cases where a dashboard doesn't have a schedule configured yet (especially ifcacheEnabledis false).export type DetailedDashboard = { __typename?: 'DetailedDashboard'; cacheEnabled: Scalars['Boolean']; description?: Maybe<Scalars['String']>; id: Scalars['Int']; items: Array<DashboardItem>; name: Scalars['String']; nextScheduledAt?: Maybe<Scalars['String']>; - schedule: DashboardSchedule; + schedule?: Maybe<DashboardSchedule>; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
wren-ui/package.json(3 hunks)wren-ui/src/apollo/client/graphql/__types__.ts(12 hunks)wren-ui/src/apollo/client/graphql/dashboard.generated.ts(3 hunks)wren-ui/src/apollo/client/graphql/dashboard.ts(1 hunks)wren-ui/src/components/diagram/CustomDropdown.tsx(3 hunks)wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx(1 hunks)wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx(1 hunks)wren-ui/src/components/pages/home/dashboardGrid/index.tsx(4 hunks)wren-ui/src/pages/home/dashboard.tsx(2 hunks)wren-ui/src/utils/errorHandler.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- wren-ui/src/utils/errorHandler.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- wren-ui/package.json
- wren-ui/src/components/diagram/CustomDropdown.tsx
- wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
wren-ui/src/apollo/client/graphql/dashboard.generated.ts (1)
wren-ui/src/apollo/client/graphql/__types__.ts (2)
Exact(4-4)SetDashboardScheduleInput(1329-1332)
wren-ui/src/apollo/client/graphql/__types__.ts (2)
wren-ui/src/pages/home/dashboard.tsx (1)
Dashboard(33-143)wren-ui/src/apollo/server/repositories/dashboardRepository.ts (1)
Dashboard(5-14)
wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx (4)
wren-ui/src/components/pages/home/dashboardGrid/CacheSettingsDrawer.tsx (2)
Schedule(28-35)getScheduleText(113-144)wren-ui/src/utils/time.ts (1)
getCompactTime(21-23)wren-ui/src/components/diagram/CustomDropdown.tsx (1)
DashboardDropdown(137-162)wren-ui/src/utils/icons.ts (1)
MoreIcon(91-91)
🪛 Biome (1.9.4)
wren-ui/src/components/pages/home/dashboardGrid/DashboardHeader.tsx
[error] 56-80: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
wren-ui/src/apollo/client/graphql/__types__.ts (7)
122-130: Well structured day enumeration for scheduling.This enum provides a clean way to represent days of the week for scheduling. The three-letter abbreviations follow standard convention and make it easy to understand in both code and UI.
241-251: Good extension of Dashboard type with caching and scheduling fields.The Dashboard type has been properly extended with all necessary fields for caching functionality:
cacheEnabledflag controls whether caching is active- Schedule-related fields (
scheduleFrequency,scheduleTimezone,scheduleCron) store the caching schedule configurationnextScheduledAthelps track when the next cache refresh will occurThese fields align well with the server-side Dashboard entity as shown in the relevant code snippets.
293-301: Clean DashboardSchedule type implementation.This type effectively encapsulates all the schedule configuration details with appropriate optionality markers. The
frequencyis correctly required while other fields may be optional depending on the selected frequency type.
1073-1080: Good design for cache metadata in PreviewItemResponse.This response type now includes useful cache metadata fields:
cacheHitindicates whether data came from cachecacheCreatedAtshows when the cache was createdcacheOverrodeAttracks manual cache refresh timestampsoverrideindicates if this was a forced cache overrideThese fields will be valuable for showing cache status to users and making caching behavior transparent.
1085-1085: Effective refresh parameter addition.Adding an optional
refreshparameter toPreviewItemSqlInputis a clean way to allow clients to request a cache refresh when needed. This maintains backward compatibility while adding new functionality.
1299-1304: Comprehensive schedule frequency enumeration.The
ScheduleFrequencyEnumprovides a good set of frequency options:
NEVERfor disabling scheduled refreshDAILYandWEEKLYfor common use casesCUSTOMfor advanced scheduling needsThis covers the common scheduling patterns users would need for dashboard caching.
1320-1333: Well-structured input types for dashboard scheduling.The input types for setting dashboard schedules are well-designed:
SetDashboardScheduleDatacontains all the schedule configuration fieldsSetDashboardScheduleInputwraps the schedule data with thecacheEnabledflagMaking
scheduleoptional inSetDashboardScheduleInputallows turning off caching without requiring schedule data, which is a good design choice.wren-ui/src/apollo/client/graphql/dashboard.generated.ts (4)
47-47: Excellent typed response for PreviewItemSQL mutation.The mutation return type now uses the structured
PreviewItemResponsetype instead of relying on generic JSON responses. This provides better type safety and makes cache-related information available to the frontend.
246-252: Well-defined GraphQL query fields for cache metadata.The GraphQL query now explicitly requests all relevant cache metadata fields, ensuring the client receives complete information about cache status for each dashboard item.
49-54: Complete implementation of SetDashboardSchedule mutation.The mutation is properly defined with appropriate variable types and return fields. The GraphQL document requests all the necessary fields from the server response, and the generated hooks follow Apollo best practices with thorough JSDoc comments.
Also applies to: 281-320
56-59: Comprehensive Dashboard query implementation.The Dashboard query is well implemented to fetch all necessary dashboard data including:
- Basic dashboard information (id, name, description)
- Cache settings (cacheEnabled, nextScheduledAt)
- Complete schedule configuration
- All dashboard items using the existing fragment
The generated hooks are properly typed and documented, making them easy to use in React components.
This aligns well with the usage in the dashboard component shown in the relevant code snippets, where the query results are used to populate UI components for displaying and managing cache settings.
Also applies to: 322-369
| updateQuery: updateDashboardQuery, | ||
| } = useDashboardQuery({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid the deprecated updateQuery, switch to the modern cache.modify API
updateQuery has been soft-deprecated since Apollo Client 3. Using cache.modify gives finer-grained control, avoids re-executing the query in some edge cases and keeps the code aligned with the current docs.
🤖 Prompt for AI Agents (early access)
In wren-ui/src/pages/home/dashboard.tsx around lines 48 to 49, replace the deprecated `updateQuery` usage with Apollo Client's modern `cache.modify` API. Remove the `updateQuery` option from the `useDashboardQuery` hook and implement cache updates using `cache.modify` to directly modify the cached fields. This approach provides finer control over cache updates and aligns with current Apollo Client best practices.
3 participants
support cache dashboard item
Summary by CodeRabbit
New Features
Improvements
Chores
Tests