feat: increase support for multiple time zones #6839
Conversation
ming4762
requested review from
a team,
anncwb,
jinmao88,
mynetfan and
vince292007
as code owners
|
|
Warning Rate limit exceeded@ming4762 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds end-to-end timezone support: mock backend endpoints and data, timezone utilities, client APIs and Pinia stores, a TimezoneButton UI widget with header slot integration, date/time utilities and typings, preferences toggle, and i18n strings across apps and packages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as TimezoneButton
participant Store as useUserProfileStore
participant API as requestClient
participant Backend as backend-mock
User->>UI: open modal & select timezone
UI->>UI: confirm -> call okHandler
UI->>Store: setTimezone(timezone)
Store->>API: setUserTimezoneApi(timezone) POST /user/setTimezone
API->>Backend: POST /user/setTimezone
Backend->>Backend: verifyAccessToken, setTimezone(mock)
Backend-->>API: 200 success
API-->>Store: resolved
Store->>Store: update timezone state, setDefaultTimezone
Store-->>UI: success
UI->>User: show success, close modal
Note over Store,Backend: On initialization
Store->>API: getUserTimezoneApi() GET /user/timezone
API->>Backend: GET /user/timezone
Backend-->>API: current timezone (or null)
API-->>Store: timezone
Store->>Store: apply timezone via setDefaultTimezone
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (1)
apps/web-antd/src/api/core/user-profile.ts (1)
1-3: Same as web‑naive API: confirm TimezoneOption uses the same key ('timezone').See the verification script added in apps/web-naive/src/api/core/user-profile.ts.
Also applies to: 8-16, 21-23
🧹 Nitpick comments (14)
packages/@core/base/shared/src/utils/date.ts (1)
37-39: Consider simplifying the timezone setter.The ternary operator works correctly but could be more explicit for better readability.
export const setDefaultTimezone = (timezone?: string) => { - timezone ? dayjs.tz.setDefault(timezone) : dayjs.tz.setDefault(); + if (timezone) { + dayjs.tz.setDefault(timezone); + } else { + dayjs.tz.setDefault(); + } };apps/backend-mock/api/user/setTimezone.ts (1)
11-12: Consider adding input validation for the timezone parameter.The timezone value from the request body is used directly without validation. While this is a mock endpoint, adding basic validation (e.g., checking if timezone exists and is a non-empty string) would improve robustness and catch potential issues during development.
Example validation:
const { timezone } = await readBody(event); + if (!timezone || typeof timezone !== 'string') { + setResponseStatus(event, 400); + return useResponseError('Invalid timezone', 'Timezone is required and must be a string'); + } setTimezone(timezone);apps/web-ele/src/api/core/user-profile.ts (1)
14-16: Consider simplifying the return type.The return type
Promise<null | string | undefined>suggests three distinct states (null, undefined, string), which may be unnecessarily complex. Consider whetherPromise<string | null>orPromise<string | undefined>would suffice.apps/web-antd/src/store/user-profile.ts (1)
41-41: Consider deferring auto-initialization.Calling
initTimezone()during store creation is a side effect that executes immediately when the module loads. This could cause issues if:
- The API is not yet ready
- Authentication is not yet established
- Multiple components trigger store creation simultaneously
Consider one of these approaches:
- Let consuming components call
initTimezone()explicitly when appropriate- Use a lazy initialization pattern that triggers on first access
- Add a flag to track initialization state and prevent duplicate calls
apps/web-ele/src/store/user-profile.ts (2)
41-41: Consider deferring auto-initialization.Calling
initTimezone()during store creation creates a side effect that executes immediately when the module loads. This could cause issues if the API or authentication is not yet ready.Consider:
- Explicit initialization by consuming components
- Lazy initialization on first access
- Initialization state tracking to prevent duplicate calls
1-56: Consider extracting shared store logic.This store is nearly identical to
apps/web-antd/src/store/user-profile.ts. Consider extracting the common timezone management logic into a shared composable or utility to maintain consistency and reduce duplication across projects.apps/web-ele/src/layouts/basic.vue (3)
77-89: Add error feedback on save failure.Currently only finally resets loading. Surface failures to users.
const handleSetTimezone = async ( timezone: string, modalApi: ExtendedModalApi, ) => { try { modalApi.setState({ confirmLoading: true }); await userProfileStore.setTimezone(timezone); ElMessage.success($t('ui.widgets.timezone.setSuccess')); modalApi.close(); + } catch (err) { + ElMessage.error($t?.('common.error') || 'Failed to update timezone'); } finally { modalApi.setState({ confirmLoading: false }); } };
178-185: Minor: remove unused name attr; rely on unwrapped ref.name="out" is unused and leaks to DOM. Also, after fixing computedTimezone, this remains valid.
<TimezoneButton :ok-handler="handleSetTimezone" :timezone="computedTimezone" :timezone-options="timezoneOptions" - name="out" />
72-76: Defensive mapping for backend key drift (timezone vs timeZone).Until backend/type is unified, tolerate both keys to avoid undefined values.
- timezoneOptions.value = ((await getTimezoneOptionsApi()) || []).map( - (item) => item.timezone, - ); + const list = (await getTimezoneOptionsApi()) || []; + timezoneOptions.value = list.map((item: any) => item.timezone ?? item.timeZone).filter(Boolean);packages/effects/layouts/src/widgets/timezone/timezone-button.vue (1)
70-78: Minor: fix label a11y and style class.Bind label to control and align class name with CSS.
- <div + <div class="flex cursor-pointer items-center gap-2" v-for="item in props.timezoneOptions" :key="`container${item}`" @click="handleClickItem(item)" > - <RadioGroupItem :id="item" :value="item" /> - <label class="cursor-pointer">{{ item }}</label> + <RadioGroupItem :id="item" :value="item" /> + <label class="cursor-pointer" :for="item">{{ item }}</label> </div> @@ -<style scoped> -.container { - padding-left: 20px; -} -</style> +<style scoped> +.timezone-container { + padding-left: 20px; +} +</style>Also applies to: 85-89
apps/web-naive/src/layouts/basic.vue (2)
78-90: Add error feedback on failure.try { modalApi.setState({ confirmLoading: true }); await userProfileStore.setTimezone(timezone); message.success($t('ui.widgets.timezone.setSuccess')); modalApi.close(); + } catch (err) { + message.error($t?.('common.error') || 'Failed to update timezone'); } finally { modalApi.setState({ confirmLoading: false }); }
181-187: Minor: remove unused name attr.<TimezoneButton :ok-handler="handleSetTimezone" :timezone="computedTimezone" :timezone-options="timezoneOptions" - name="out" />apps/web-antd/src/layouts/basic.vue (2)
77-89: Add error feedback on failure.try { modalApi.setState({ confirmLoading: true }); await userProfileStore.setTimezone(timezone); message.success($t('ui.widgets.timezone.setSuccess')); modalApi.close(); + } catch (err) { + message.error($t?.('common.error') || 'Failed to update timezone'); } finally { modalApi.setState({ confirmLoading: false }); }
179-185: Minor: remove unused name attr.<TimezoneButton :ok-handler="handleSetTimezone" :timezone="computedTimezone" :timezone-options="timezoneOptions" - name="out" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
apps/backend-mock/api/profile/timezone.ts(1 hunks)apps/backend-mock/api/user/setTimezone.ts(1 hunks)apps/backend-mock/api/user/timezone.ts(1 hunks)apps/backend-mock/utils/mock-data.ts(2 hunks)apps/backend-mock/utils/timezone-utils.ts(1 hunks)apps/web-antd/src/api/core/index.ts(1 hunks)apps/web-antd/src/api/core/user-profile.ts(1 hunks)apps/web-antd/src/layouts/basic.vue(4 hunks)apps/web-antd/src/store/index.ts(1 hunks)apps/web-antd/src/store/user-profile.ts(1 hunks)apps/web-ele/src/api/core/index.ts(1 hunks)apps/web-ele/src/api/core/user-profile.ts(1 hunks)apps/web-ele/src/layouts/basic.vue(4 hunks)apps/web-ele/src/store/index.ts(1 hunks)apps/web-ele/src/store/user-profile.ts(1 hunks)apps/web-naive/src/api/core/index.ts(1 hunks)apps/web-naive/src/api/core/user-profile.ts(1 hunks)apps/web-naive/src/layouts/basic.vue(4 hunks)apps/web-naive/src/store/index.ts(1 hunks)apps/web-naive/src/store/user-profile.ts(1 hunks)packages/@core/base/shared/src/utils/date.ts(2 hunks)packages/@core/base/typings/src/index.ts(1 hunks)packages/@core/base/typings/src/user-profile.d.ts(1 hunks)packages/@core/preferences/src/config.ts(1 hunks)packages/@core/preferences/src/types.ts(1 hunks)packages/effects/layouts/src/basic/header/header.vue(1 hunks)packages/effects/layouts/src/basic/layout.vue(1 hunks)packages/effects/layouts/src/widgets/index.ts(1 hunks)packages/effects/layouts/src/widgets/timezone/index.ts(1 hunks)packages/effects/layouts/src/widgets/timezone/timezone-button.vue(1 hunks)packages/locales/src/langs/en-US/ui.json(1 hunks)packages/locales/src/langs/zh-CN/ui.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
apps/web-ele/src/api/core/user-profile.ts (3)
apps/web-antd/src/api/core/user-profile.ts (3)
getTimezoneOptionsApi(8-10)getUserTimezoneApi(14-16)setUserTimezoneApi(21-23)apps/web-naive/src/api/core/user-profile.ts (3)
getTimezoneOptionsApi(8-10)getUserTimezoneApi(14-16)setUserTimezoneApi(21-23)packages/@core/base/typings/src/user-profile.d.ts (1)
TimezoneOption(9-9)
apps/backend-mock/api/profile/timezone.ts (2)
apps/backend-mock/utils/response.ts (1)
useResponseSuccess(5-12)apps/backend-mock/utils/mock-data.ts (1)
TIME_ZONE_OPTIONS(400-421)
apps/backend-mock/api/user/setTimezone.ts (3)
apps/backend-mock/utils/jwt-utils.ts (1)
verifyAccessToken(29-58)apps/backend-mock/utils/response.ts (2)
unAuthorizedResponse(52-55)useResponseSuccess(5-12)apps/backend-mock/utils/timezone-utils.ts (1)
setTimezone(3-5)
apps/backend-mock/utils/timezone-utils.ts (1)
packages/@core/base/shared/src/utils/date.ts (1)
getTimezone(45-47)
apps/web-antd/src/store/user-profile.ts (3)
apps/backend-mock/utils/timezone-utils.ts (2)
getTimezone(7-10)setTimezone(3-5)packages/@core/base/shared/src/utils/date.ts (2)
getTimezone(45-47)setDefaultTimezone(37-39)apps/web-antd/src/api/core/user-profile.ts (2)
setUserTimezoneApi(21-23)getUserTimezoneApi(14-16)
apps/web-naive/src/api/core/user-profile.ts (3)
apps/web-antd/src/api/core/user-profile.ts (3)
getTimezoneOptionsApi(8-10)getUserTimezoneApi(14-16)setUserTimezoneApi(21-23)apps/web-ele/src/api/core/user-profile.ts (3)
getTimezoneOptionsApi(8-10)getUserTimezoneApi(14-16)setUserTimezoneApi(21-23)packages/@core/base/typings/src/user-profile.d.ts (1)
TimezoneOption(9-9)
apps/web-ele/src/store/user-profile.ts (2)
packages/@core/base/shared/src/utils/date.ts (2)
getTimezone(45-47)setDefaultTimezone(37-39)apps/web-ele/src/api/core/user-profile.ts (2)
setUserTimezoneApi(21-23)getUserTimezoneApi(14-16)
packages/@core/base/shared/src/utils/date.ts (1)
apps/backend-mock/utils/timezone-utils.ts (1)
getTimezone(7-10)
apps/backend-mock/api/user/timezone.ts (3)
apps/backend-mock/utils/jwt-utils.ts (1)
verifyAccessToken(29-58)apps/backend-mock/utils/response.ts (2)
unAuthorizedResponse(52-55)useResponseSuccess(5-12)apps/backend-mock/utils/timezone-utils.ts (1)
getTimezone(7-10)
apps/web-antd/src/api/core/user-profile.ts (3)
apps/web-ele/src/api/core/user-profile.ts (3)
getTimezoneOptionsApi(8-10)getUserTimezoneApi(14-16)setUserTimezoneApi(21-23)apps/web-naive/src/api/core/user-profile.ts (3)
getTimezoneOptionsApi(8-10)getUserTimezoneApi(14-16)setUserTimezoneApi(21-23)packages/@core/base/typings/src/user-profile.d.ts (1)
TimezoneOption(9-9)
apps/web-naive/src/store/user-profile.ts (3)
apps/backend-mock/utils/timezone-utils.ts (2)
getTimezone(7-10)setTimezone(3-5)packages/@core/base/shared/src/utils/date.ts (2)
getTimezone(45-47)setDefaultTimezone(37-39)apps/web-naive/src/api/core/user-profile.ts (2)
setUserTimezoneApi(21-23)getUserTimezoneApi(14-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (21)
packages/locales/src/langs/zh-CN/ui.json (1)
106-108: LGTM!The timezone localization strings are properly structured and translations appear appropriate for the Chinese locale.
packages/locales/src/langs/en-US/ui.json (1)
106-108: LGTM!The timezone localization strings are properly structured and consistent with the Chinese locale file.
packages/@core/preferences/src/types.ts (1)
278-279: LGTM!The timezone widget preference field is properly typed and follows the existing pattern for widget configuration flags.
packages/@core/base/shared/src/utils/date.ts (3)
2-6: LGTM!The dayjs timezone and UTC plugins are properly imported and extended in the correct order.
45-46: LGTM!The
getTimezonefunction correctly uses dayjs's built-in timezone guessing functionality.
14-14: Verify the semantic change in timezone handling is intentional.The
date.tz()method without arguments will use the default timezone if one is set, otherwise it falls back to the local timezone and does not throw an error. This means the code is safe and won't fail unexpectedly.However, changing from
date.format(format)todate.tz().format(format)shifts the function's behavior from timezone-unaware to timezone-aware (defaulting to local timezone). Confirm this semantic change aligns with the intended behavior offormatDate.apps/backend-mock/utils/timezone-utils.ts (1)
3-5: LGTM!The mock
setTimezonefunction is appropriately simple for backend testing purposes.apps/web-ele/src/api/core/index.ts (1)
4-4: LGTM!The user-profile module re-export follows the existing barrel export pattern.
apps/web-naive/src/store/index.ts (1)
2-2: LGTM!The user-profile store module re-export follows the existing barrel export pattern.
apps/web-naive/src/api/core/index.ts (1)
4-4: LGTM!The user-profile module re-export follows the existing barrel export pattern.
packages/effects/layouts/src/basic/layout.vue (1)
305-307: LGTM! Slot implementation follows existing patterns.The timezone slot is correctly implemented and follows the same pattern as the notification slot above it. This provides a clean extension point for timezone UI components.
packages/@core/preferences/src/config.ts (1)
137-137: LGTM! Widget configuration follows established pattern.The timezone widget flag is correctly added with a default value of
true, consistent with other widget preferences in the configuration object.apps/backend-mock/api/profile/timezone.ts (1)
5-6: LGTM! Appropriate implementation for a public options endpoint.This endpoint correctly returns the available timezone options without requiring authentication, which is appropriate for a resource that provides configuration options to all users.
packages/effects/layouts/src/widgets/timezone/index.ts (1)
1-1: LGTM!Standard barrel export pattern for the TimezoneButton component.
packages/@core/base/typings/src/user-profile.d.ts (1)
1-9: LGTM!Type definition is clear and appropriately structured for timezone options.
packages/effects/layouts/src/widgets/index.ts (1)
11-11: LGTM!Consistent barrel export pattern extending the public API to include timezone widgets.
packages/effects/layouts/src/basic/header/header.vue (1)
75-86: LGTM!Slot index adjustments are correct. Fullscreen moved to +50 and notification moved to +60 to accommodate the new timezone slot at +40.
apps/web-ele/src/api/core/user-profile.ts (2)
8-10: LGTM!API wrapper is correctly implemented with proper JSDoc documentation.
Ensure that the
requestClienthas appropriate error handling and retry logic for these timezone endpoints, especially forgetTimezoneOptionsApiwhich is critical for the timezone selection UI.
21-23: LGTM!Timezone persistence API is correctly implemented.
apps/web-antd/src/store/user-profile.ts (1)
10-12: LGTM!Fallback logic appropriately handles cases where
getTimezone()returns a falsy value by using the browser's default timezone.apps/backend-mock/api/user/timezone.ts (1)
6-12: LGTM!Mock endpoint correctly implements authentication check and returns timezone data. Implementation is clean and follows established patterns.
| export interface TimeZoneOption { | ||
| offset: number; | ||
| timeZone: string; | ||
| } |
There was a problem hiding this comment.
Unify timezone key and type name; current code won’t type‑check.
Interface uses timeZone while data uses timezone. Also type is TimeZoneOption vs shared TimezoneOption elsewhere.
-export interface TimeZoneOption {
- offset: number;
- timeZone: string;
-}
+export interface TimezoneOption {
+ offset: number;
+ timezone: string;
+}
@@
-export const TIME_ZONE_OPTIONS: TimeZoneOption[] = [
+export const TIME_ZONE_OPTIONS: TimezoneOption[] = [Also applies to: 400-421
🤖 Prompt for AI Agents
In apps/backend-mock/utils/mock-data.ts around lines 10 to 13 (and similarly
lines ~400-421), the interface and the data are mismatched: the interface
declares timeZone: string and is named TimeZoneOption while the actual mock
objects use timezone and the shared type elsewhere is named TimezoneOption;
update the file so the property name and type name match the rest of the
codebase — either rename the interface to export interface TimezoneOption {
offset: number; timezone: string; } or change the mock data to use timeZone and
export the interface as TimeZoneOption consistently, and update any
imports/usages to reference the unified type name.
| @@ -1,7 +1,8 @@ | |||
| <script lang="ts" setup> | |||
| import type { ExtendedModalApi } from '@vben/common-ui'; | |||
There was a problem hiding this comment.
Unify ExtendedModalApi source with widget (popup-ui).
-import type { ExtendedModalApi } from '@vben/common-ui';
+import type { ExtendedModalApi } from '@vben-core/popup-ui';📝 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.
| import type { ExtendedModalApi } from '@vben/common-ui'; | |
| import type { ExtendedModalApi } from '@vben-core/popup-ui'; |
🤖 Prompt for AI Agents
In apps/web-antd/src/layouts/basic.vue around line 2, the ExtendedModalApi type
is imported from '@vben/common-ui' but should be unified with the widget's
popup-ui; change the import source to the widget module (e.g.
'@vben/widget/popup-ui'), update the import statement accordingly, and ensure
TypeScript path/module resolution is correct so the new module compiles.
| async function setTimezone(timezone: string) { | ||
| timezoneRef.value = timezone; | ||
| // 设置dayjs默认时区 | ||
| setDefaultTimezone(timezone); | ||
| // 保存用户的时区设置 | ||
| await setUserTimezoneApi(timezone); | ||
| } |
There was a problem hiding this comment.
Add error handling for timezone persistence.
The setTimezone function updates local state before confirming the server update succeeds. If setUserTimezoneApi fails, the local timezone will be out of sync with the server.
Consider this approach:
async function setTimezone(timezone: string) {
- timezoneRef.value = timezone;
- // 设置dayjs默认时区
- setDefaultTimezone(timezone);
- // 保存用户的时区设置
- await setUserTimezoneApi(timezone);
+ try {
+ // 保存用户的时区设置
+ await setUserTimezoneApi(timezone);
+ // 只有在服务器更新成功后才更新本地状态
+ timezoneRef.value = timezone;
+ // 设置dayjs默认时区
+ setDefaultTimezone(timezone);
+ } catch (error) {
+ console.error('Failed to set timezone:', error);
+ throw error; // Re-throw to allow caller to handle
+ }
}📝 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.
| async function setTimezone(timezone: string) { | |
| timezoneRef.value = timezone; | |
| // 设置dayjs默认时区 | |
| setDefaultTimezone(timezone); | |
| // 保存用户的时区设置 | |
| await setUserTimezoneApi(timezone); | |
| } | |
| async function setTimezone(timezone: string) { | |
| try { | |
| // 保存用户的时区设置 | |
| await setUserTimezoneApi(timezone); | |
| // 只有在服务器更新成功后才更新本地状态 | |
| timezoneRef.value = timezone; | |
| // 设置dayjs默认时区 | |
| setDefaultTimezone(timezone); | |
| } catch (error) { | |
| console.error('Failed to set timezone:', error); | |
| throw error; // Re-throw to allow caller to handle | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/web-antd/src/store/user-profile.ts around lines 19–25, setTimezone
currently updates timezoneRef and calls setDefaultTimezone before awaiting
setUserTimezoneApi, which can leave local state inconsistent if the API fails;
change the flow to await the API call inside a try/catch and only update
timezoneRef and call setDefaultTimezone after a successful response, and in the
catch log the error (or show user feedback) and do not change local state (or if
you prefer optimistic update, perform the update first but revert it in the
catch and notify the user); ensure the function rethrows or returns a failure
indicator so callers can react.
| const timezoneRef = ref( | ||
| getTimezone() || new Intl.DateTimeFormat().resolvedOptions().timeZone, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Prevent init/save races and set default TZ immediately.
- initTimezone may overwrite a user’s selection if it resolves later.
- Default timezone isn’t set from the initial value until init or set.
- No rollback on failed save.
const timezoneRef = ref(
getTimezone() || new Intl.DateTimeFormat().resolvedOptions().timeZone,
);
+ // Ensure dayjs default timezone is set from initial value
+ setDefaultTimezone(timezoneRef.value);
@@
async function setTimezone(timezone: string) {
- timezoneRef.value = timezone;
- // 设置dayjs默认时区
- setDefaultTimezone(timezone);
- // 保存用户的时区设置
- await setUserTimezoneApi(timezone);
+ const prev = timezoneRef.value;
+ timezoneRef.value = timezone;
+ setDefaultTimezone(timezone);
+ try {
+ await setUserTimezoneApi(timezone);
+ } catch (err) {
+ // rollback on failure
+ timezoneRef.value = prev;
+ setDefaultTimezone(prev);
+ throw err;
+ }
}
@@
async function initTimezone() {
- // 从服务器获取用户时区
- const timezone = await getUserTimezoneApi();
- if (timezone) {
- timezoneRef.value = timezone;
- // 设置dayjs默认时区
- setDefaultTimezone(timezone);
- }
+ const snapshot = timezoneRef.value;
+ try {
+ const timezone = await getUserTimezoneApi();
+ if (timezone && timezoneRef.value === snapshot) {
+ timezoneRef.value = timezone;
+ setDefaultTimezone(timezone);
+ }
+ } catch {
+ // ignore init errors; keep local guess
+ }
}
@@
- initTimezone();
+ initTimezone();Also applies to: 21-25, 31-39, 41-41
🤖 Prompt for AI Agents
In apps/web-naive/src/store/user-profile.ts around lines 10-13 (and also
affecting 21-25, 31-39, 41), the timezone handling can race with async
init/save, doesn't apply the default TZ immediately, and lacks rollback on
failed saves; change the logic to: set the timezone ref synchronously to the
computed default immediately on creation, use a local incrementing
request/version token or abortable promise so any async initTimezone result is
ignored if a newer user selection happened, perform optimistic updates when
setTimezone is called (update ref/store and persist), and if the save/persist
call fails revert the ref/store to the prior value and surface the error; also
ensure initTimezone writes only when no user-selection/version supersedes it and
that all persistence calls return and handle errors for rollback.
| if (preferences.widget.timezone) { | ||
| list.push({ | ||
| index: REFERENCE_VALUE + 40, | ||
| name: 'timezone', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Critical: Timezone slot is registered but never rendered in the template.
The timezone slot is correctly added to the rightSlots computed property, but there's no corresponding template rendering block in the <template> section (lines 150-176). This means the timezone widget will never be displayed.
Add the timezone template block after the language-toggle section:
<template v-else-if="slot.name === 'language-toggle'">
<LanguageToggle class="mr-1" />
</template>
+ <template v-else-if="slot.name === 'timezone'">
+ <slot name="timezone"></slot>
+ </template>
<template v-else-if="slot.name === 'fullscreen'">
<VbenFullScreen class="mr-1" />
</template>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/effects/layouts/src/basic/header/header.vue around lines 69 and
150-176, the computed rightSlots registers a timezone entry but the template
never renders it; add a template block immediately after the language-toggle
block that conditionally renders the timezone slot when
preferences.widget.timezone is truthy, using the same wrapper structure/classes
as the other widgets and the named slot "timezone" so the registered slot
content is actually displayed.
| @@ -0,0 +1,89 @@ | |||
| <script setup lang="ts"> | |||
| import type { ExtendedModalApi } from '@vben-core/popup-ui'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
Type alignment: ExtendedModalApi must match consumers.
Parents import ExtendedModalApi from '@vben/common-ui' while this component uses '@vben-core/popup-ui'. Unify on one (recommend popup-ui everywhere, since useVbenModal comes from it), or re-export the type from a shared package.
🏁 Script executed:
#!/bin/bash
# Where ExtendedModalApi is imported
rg -nP "ExtendedModalApi" -C1 --type=ts --type=vueLength of output: 27
🏁 Script executed:
#!/bin/bash
# Search for ExtendedModalApi imports without type restrictions
rg -nP "ExtendedModalApi" -C2 -g "*.{ts,tsx,js,jsx,vue}"Length of output: 6115
🏁 Script executed:
#!/bin/bash
# Find where @vben/common-ui is located and check exports
fd -t f "package.json" -x grep -l "common-ui" {} \;Length of output: 174
🏁 Script executed:
#!/bin/bash
# Search for ExtendedModalApi exports (not just imports)
rg -nP "export.*ExtendedModalApi|export.*from.*ExtendedModalApi" -g "*.{ts,tsx,js,jsx}"Length of output: 97
🏁 Script executed:
#!/bin/bash
# Find main/index exports from @vben/common-ui
fd -t f "index.ts\|index.js" -path "*packages/effects/common-ui*"Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check common-ui exports for ExtendedModalApi
rg -nP "ExtendedModalApi" -g "*.{ts,tsx,js,jsx}" "packages/effects/common-ui/"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check common-ui package.json for dependencies
cat packages/effects/common-ui/package.jsonLength of output: 1513
🏁 Script executed:
#!/bin/bash
# Check main index file from @vben/common-ui
cat packages/effects/common-ui/src/index.tsLength of output: 51
🏁 Script executed:
#!/bin/bash
# List structure of common-ui/src
find packages/effects/common-ui/src -name "*.ts" -type f | head -20Length of output: 1228
🏁 Script executed:
#!/bin/bash
# Check ui directory exports from common-ui
cat packages/effects/common-ui/src/ui/index.tsLength of output: 115
🏁 Script executed:
#!/bin/bash
# Double-check: does common-ui actually export ExtendedModalApi anywhere?
rg -nP "ExtendedModalApi" packages/effects/common-ui/Length of output: 0
Fix parent layout imports: ExtendedModalApi unavailable from @vben/common-ui.
Parents (apps/web-{ele,antd,naive}/src/layouts/basic.vue) import ExtendedModalApi from '@vben/common-ui', but this type is not exported by that package—it is defined only in @vben-core/popup-ui. The timezone-button.vue component correctly imports from @vben-core/popup-ui.
Either:
- Update parents to import from
@vben-core/popup-ui, or - Re-export
ExtendedModalApifrom@vben/common-ui's index
🤖 Prompt for AI Agents
In packages/effects/layouts/src/widgets/timezone/timezone-button.vue around line
2, parents currently import ExtendedModalApi from @vben/common-ui but that type
is only defined in @vben-core/popup-ui; fix by updating parent layout imports
(apps/web-{ele,antd,naive}/src/layouts/basic.vue) to import ExtendedModalApi
directly from '@vben-core/popup-ui' (or alternatively add a re-export of
ExtendedModalApi from the @vben/common-ui index) so the type resolves correctly.
| const timezoneValue = ref(props.timezone); | ||
| watch( | ||
| () => props.timezone, | ||
| (newTimezone) => { | ||
| timezoneValue.value = newTimezone; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ensure timezoneValue is always a string (unwrap incoming refs).
Avoid ref-of-ref when parent passes a Ref. Also keep in sync on prop change.
-const timezoneValue = ref(props.timezone);
+const timezoneValue = ref<string | undefined>(unref(props.timezone));
watch(
() => props.timezone,
(newTimezone) => {
- timezoneValue.value = newTimezone;
+ timezoneValue.value = unref(newTimezone);
},
);📝 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.
| const timezoneValue = ref(props.timezone); | |
| watch( | |
| () => props.timezone, | |
| (newTimezone) => { | |
| timezoneValue.value = newTimezone; | |
| }, | |
| ); | |
| const timezoneValue = ref<string | undefined>(unref(props.timezone)); | |
| watch( | |
| () => props.timezone, | |
| (newTimezone) => { | |
| timezoneValue.value = unref(newTimezone); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In packages/effects/layouts/src/widgets/timezone/timezone-button.vue around
lines 45 to 51, props.timezone may be a plain string or a Ref, causing a
ref-of-ref; ensure timezoneValue is always a simple string by unwrapping
incoming refs when initializing and updating it. Initialize timezoneValue with
ref(String(unref(props.timezone) ?? '')), and in the watcher set
timezoneValue.value = String(unref(newTimezone) ?? ''); this prevents nested
refs and keeps the value synchronized with prop changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/web-ele/src/store/user-profile.ts (2)
31-39: Add error handling for timezone initialization.Missing error handling could lead to unhandled promise rejections if the API call fails.
async function initTimezone() { - // 从服务器获取用户时区 - const timezone = await getUserTimezoneApi(); - if (timezone) { - timezoneRef.value = timezone; - // 设置dayjs默认时区 - setDefaultTimezone(timezone); - } + try { + // 从服务器获取用户时区 + const timezone = await getUserTimezoneApi(); + if (timezone) { + timezoneRef.value = timezone; + // 设置dayjs默认时区 + setDefaultTimezone(timezone); + } + } catch (error) { + console.error('Failed to initialize timezone:', error); + // Fallback to browser timezone already set in timezoneRef initialization + } }
19-25: Add error handling for timezone persistence.The function now correctly calls the API before updating local state, which is an improvement. However, error handling is still missing. If
setUserTimezoneApifails, the error will propagate silently without logging.Apply this diff to add error handling:
async function setTimezone(timezone: string) { - // 保存用户的时区设置 - await setUserTimezoneApi(timezone); - timezoneRef.value = timezone; - // 设置dayjs默认时区 - setDefaultTimezone(timezone); + try { + // 保存用户的时区设置 + await setUserTimezoneApi(timezone); + timezoneRef.value = timezone; + // 设置dayjs默认时区 + setDefaultTimezone(timezone); + } catch (error) { + console.error('Failed to set timezone:', error); + throw error; + } }packages/effects/layouts/src/widgets/timezone/timezone-button.vue (1)
45-51: Normalize timezoneValue to a plain string; avoid ref-of-ref and undefined.Initialize and sync as a string to prevent v-model edge cases and nested refs.
-const timezoneValue = ref<string | undefined>(unref(props.timezone)); +const timezoneValue = ref<string>(String(unref(props.timezone) ?? '')); watch( () => props.timezone, (newTimezone) => { - timezoneValue.value = unref(newTimezone); + timezoneValue.value = String(unref(newTimezone) ?? ''); }, );apps/web-antd/src/layouts/basic.vue (1)
2-2: Wrong source for ExtendedModalApi — switch to popup-ui.
@vben/common-uidoes not export ExtendedModalApi; use@vben-core/popup-uito match the widget and avoid TS errors.-import type { ExtendedModalApi } from '@vben/common-ui'; +import type { ExtendedModalApi } from '@vben-core/popup-ui';#!/bin/bash # Verify there are no remaining imports from @vben/common-ui for ExtendedModalApi rg -nP "import\s+type\s+\{\s*ExtendedModalApi\s*\}\s+from\s+'@vben/common-ui'" -g "apps/**/layouts/**.vue" -S
🧹 Nitpick comments (6)
packages/effects/layouts/src/widgets/timezone/timezone-button.vue (3)
69-79: Improve a11y: sanitize IDs and link label via :for.Timezone strings contain slashes; prefer safe IDs and associate label.
+const toId = (tz: string) => `tz-${tz.replace(/[^A-Za-z0-9_-]/g, '-')}`;(Place in <script setup>)
- <RadioGroupItem :id="item" :value="item" /> - <label class="cursor-pointer">{{ item }}</label> + <RadioGroupItem :id="toId(item)" :value="item" /> + <label class="cursor-pointer" :for="toId(item)">{{ item }}</label>
85-89: Fix style class mismatch.Template uses
.timezone-containerbut CSS targets.container. Align them.-<style scoped> -.container { - padding-left: 20px; -} -</style> +<style scoped> +.timezone-container { + padding-left: 20px; +} +</style>
52-55: Emit 'change' only if consumed (or document it).Parent in apps/web-antd/basic.vue doesn't listen to
change; either remove emit to avoid confusion or ensure parent registers@change.Do you intend consumers to react instantly on selection (before OK)? If yes, add
@changein parents; otherwise, drop the emit here.apps/web-antd/src/layouts/basic.vue (3)
71-76: Harden options mapping: filter invalids and dedupe.APIs may return null/duplicate entries; keep
string[]invariant.-onMounted(async () => { - timezoneOptions.value = ((await getTimezoneOptionsApi()) || []).map( - (item) => item.timezone, - ); -}); +onMounted(async () => { + const list = (await getTimezoneOptionsApi()) ?? []; + timezoneOptions.value = Array.from( + new Set( + list + .map((item) => item?.timezone) + .filter((tz): tz is string => typeof tz === 'string' && tz.length > 0), + ), + ); +});
77-92: Surface errors to users when setTimezone fails.Add an error toast in the catch to improve UX.
try { modalApi.setState({ confirmLoading: true }); await userProfileStore.setTimezone(timezone); message.success($t('ui.widgets.timezone.setSuccess')); modalApi.close(); +} catch (err) { + message.error(($t('ui.error.operationFailed') as string) || 'Failed to set timezone'); } finally { modalApi.setState({ confirmLoading: false }); }
181-188: Remove stray attribute on TimezoneButton.
name="out"isn’t a declared prop; it bubbles to DOM. Drop it unless required.- <TimezoneButton + <TimezoneButton :ok-handler="handleSetTimezone" :timezone="computedTimezone" :timezone-options="timezoneOptions" - name="out" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/@core/preferences/__tests__/__snapshots__/config.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
apps/backend-mock/utils/timezone-utils.ts(1 hunks)apps/web-antd/src/layouts/basic.vue(4 hunks)apps/web-antd/src/store/user-profile.ts(1 hunks)apps/web-ele/src/layouts/basic.vue(4 hunks)apps/web-ele/src/store/user-profile.ts(1 hunks)apps/web-naive/src/layouts/basic.vue(4 hunks)apps/web-naive/src/store/user-profile.ts(1 hunks)packages/effects/layouts/src/widgets/timezone/timezone-button.vue(1 hunks)packages/effects/plugins/src/echarts/use-echarts.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web-naive/src/store/user-profile.ts
- apps/web-ele/src/layouts/basic.vue
- apps/web-antd/src/store/user-profile.ts
- apps/web-naive/src/layouts/basic.vue
🧰 Additional context used
🧬 Code graph analysis (2)
apps/backend-mock/utils/timezone-utils.ts (1)
packages/@core/base/shared/src/utils/date.ts (1)
getTimezone(45-47)
apps/web-ele/src/store/user-profile.ts (2)
packages/@core/base/shared/src/utils/date.ts (2)
getTimezone(45-47)setDefaultTimezone(37-39)apps/web-ele/src/api/core/user-profile.ts (2)
setUserTimezoneApi(21-23)getUserTimezoneApi(14-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (5)
packages/effects/plugins/src/echarts/use-echarts.ts (1)
70-73: Trailing comma added—unrelated to timezone feature.The trailing comma is syntactically valid and follows modern formatting conventions. However, this change appears unrelated to the PR's stated objective of adding timezone support.
apps/backend-mock/utils/timezone-utils.ts (1)
1-9: LGTM! Clean mock implementation.The previous debug logging issue has been resolved. The implementation is clean and appropriate for a mock backend utility.
apps/web-ele/src/store/user-profile.ts (2)
1-12: LGTM: Solid initialization with fallback.The import structure and timezone initialization logic are well-designed. The fallback chain ensures a timezone is always available, even if
getTimezone()returns undefined.
43-56: LGTM: Proper store setup and HMR support.The store return statement correctly exposes the timezone state and actions. HMR support is properly configured for development experience.
apps/web-antd/src/layouts/basic.vue (1)
68-70: Nice fix: avoid nested Ref for timezone.Using
unref(userProfileStore.timezone)ensures a plain string for the child prop.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/effects/layouts/src/widgets/timezone/timezone-button.vue (1)
69-74: Potential redundant state update from v-model and click handler.The
RadioGroupat line 69 is bound withv-model="timezoneValue", which automatically updatestimezoneValuewhen aRadioGroupItemis selected. The@click="handleClickItem(item)"handler at line 74 also manually updatestimezoneValue.value = timezone. This causes the value to be set twice for each selection.If you want the
changeevent to emit immediately on selection, keep the click handler but remove the manual assignment insidehandleClickItemsince v-model handles it. If you want changes to apply only on modal confirmation, consider removing the immediate click handler and emit logic.Apply this diff to remove the redundant manual assignment:
const handleClickItem = (timezone: string) => { - timezoneValue.value = timezone; emit('change', timezone); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/layouts/src/widgets/timezone/timezone-button.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Test (windows-latest)
packages/effects/layouts/src/widgets/timezone/timezone-button.vue
Outdated
Show resolved
Hide resolved
1 participant
feat: increase support for multiple time zones
Summary by CodeRabbit
New Features
Localization