-
Notifications
You must be signed in to change notification settings - Fork 13k
feat: Move Timestamp parser out of Feature Preview #36976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 3c8cfca The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughRemoves the “enable-timestamp-message-parser” feature flag and related preview/i18n entries, making timestamp parsing always active. Updates Gazzodown to render Timestamp unconditionally, adjusts context shape, and cleans up client usage. Adds a changeset declaring major bumps for multiple packages and notes promotion of Timestamp Parser to stable. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client as Client UI
participant GZD as Gazzodown
participant TW as TimestampWrapper
participant EB as ErrorBoundary
participant TS as Timestamp
User->>Client: View message
Client->>GZD: Render markup
GZD->>TW: Render timestamp node
TW->>EB: Wrap render
EB->>TS: Render timestamp component
TS-->>EB: Formatted time
EB-->>GZD: Result (or UTC fallback on error)
GZD-->>Client: Rendered content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #36976 +/- ##
=================================================
- Coverage 66.43% 66.38% -0.06%
=================================================
Files 3276 3312 +36
Lines 109596 111663 +2067
Branches 20860 20941 +81
=================================================
+ Hits 72814 74122 +1308
- Misses 34116 34863 +747
- Partials 2666 2678 +12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (8)
.changeset/empty-buses-walk.md (1)
1-8: Add explicit BREAKING notes and migration guidance to the changeset.Major bumps are appropriate. Please expand the body with concrete breaking changes and migration steps (context/type removals, flag removal, i18n key deletions) to help integrators.
Apply this diff to append actionable notes:
Promotes Timestamp Parser from preview state to stable + +BREAKING: +- @rocket.chat/gazzodown: MarkupInteractionContextValue no longer includes `enableTimestamp`; Timestamp always renders. +- @rocket.chat/ui-client: Feature flag `enable-timestamp-message-parser` removed from `FeaturesAvailable` and from defaults. +- @rocket.chat/i18n: Removed keys `Enable_timestamp` and `Enable_timestamp_description`. + +MIGRATION: +- Remove any usage of `enableTimestamp` from Markup providers/consumers. +- Drop persisted `enable-timestamp-message-parser` entries from user/admin settings payloads if you store custom lists. +- Replace any i18n references to the removed keys.packages/gazzodown/src/Markup.stories.tsx (1)
81-107: Add a locale showcase to validate RelativeTime i18n.Consider an extra story passing
languagein the context (e.g., 'pt-BR', 'sv') to visually verify formatting.packages/gazzodown/src/elements/Timestamp/index.tsx (6)
38-42: Always-on rendering: good; ensure radix for parseInt.Guard against accidental octal/hex by specifying base 10.
- <ErrorBoundary fallback={<>{new Date(parseInt(children.value.timestamp) * 1000).toUTCString()}</>}> - <Timestamp format={children.value.format} value={new Date(parseInt(children.value.timestamp) * 1000)} /> + <ErrorBoundary fallback={<>{new Date(parseInt(children.value.timestamp, 10) * 1000).toUTCString()}</>}> + <Timestamp format={children.value.format} value={new Date(parseInt(children.value.timestamp, 10) * 1000)} />
79-91: Language is ignored after the first tick; fix effect to use context language.The interval callback hardcodes 'en' and
languageisn’t in deps, so changing language won’t update the text.- const { language } = useContext(MarkupInteractionContext); + const { language } = useContext(MarkupInteractionContext); const [text, setTime] = useState(() => intlFormatDistance(time, Date.now(), { locale: language ?? 'en' })); const [timeToRefresh, setTimeToRefresh] = useState(() => getTimeToRefresh(time)); useEffect(() => { const interval = setInterval(() => { - setTime(intlFormatDistance(value.getTime(), Date.now(), { locale: 'en' })); + setTime(intlFormatDistance(value.getTime(), Date.now(), { locale: language ?? 'en' })); setTimeToRefresh(getTimeToRefresh(time)); - }, timeToRefresh); + }, timeToRefresh); return () => clearInterval(interval); - }, [time, timeToRefresh, value]); + }, [time, timeToRefresh, value, language]);
95-115: Refresh cadence uses signed delta; use absolute difference to avoid 1s refresh forever.Past timestamps yield negative deltas, hitting the “< 1 minute” branch indefinitely.
-const getTimeToRefresh = (time: number): number => { - const timeToRefresh = time - Date.now(); +const getTimeToRefresh = (time: number): number => { + const diff = Math.abs(Date.now() - time); // less than 1 minute - if (timeToRefresh < 60000) { + if (diff < 60_000) { return 1000; } // if the difference is in the minutes range, we should refresh the time in 1 minute / 2 - if (timeToRefresh < 3600000) { - return 60000 / 2; + if (diff < 3_600_000) { + return 60_000 / 2; } // if the difference is in the hours range, we should refresh the time in 5 minutes - if (timeToRefresh < 86400000) { - return 60000 * 5; + if (diff < 86_400_000) { + return 60_000 * 5; } // refresh the time in 1 hour - return 3600000; + return 3_600_000;
63-73: Optional: Title localization.
titleuses browser default. ConsidertoLocaleString(language)for consistency with inline text.
9-11: Nit: Rename prop type.
BoldSpanPropsis misleading here; considerTimestampSpanProps.
32-34: Nit: Remove JSON.stringify from default branch.Rendering a raw number JSON is odd; prefer ISO string.
- return <time dateTime={value.toISOString()}> {JSON.stringify(value.getTime())}</time>; + return <time dateTime={value.toISOString()}>{value.toISOString()}</time>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/public/images/featurePreview/timestamp.pngis excluded by!**/*.png
📒 Files selected for processing (11)
.changeset/empty-buses-walk.md(1 hunks)apps/meteor/client/components/GazzodownText.tsx(0 hunks)packages/gazzodown/src/Markup.stories.tsx(1 hunks)packages/gazzodown/src/MarkupInteractionContext.ts(0 hunks)packages/gazzodown/src/elements/Timestamp/index.tsx(1 hunks)packages/i18n/src/locales/en.i18n.json(0 hunks)packages/i18n/src/locales/nb.i18n.json(0 hunks)packages/i18n/src/locales/nn.i18n.json(0 hunks)packages/i18n/src/locales/pt-BR.i18n.json(0 hunks)packages/i18n/src/locales/sv.i18n.json(0 hunks)packages/ui-client/src/hooks/useFeaturePreviewList.ts(1 hunks)
💤 Files with no reviewable changes (7)
- packages/gazzodown/src/MarkupInteractionContext.ts
- packages/i18n/src/locales/pt-BR.i18n.json
- packages/i18n/src/locales/sv.i18n.json
- apps/meteor/client/components/GazzodownText.tsx
- packages/i18n/src/locales/nb.i18n.json
- packages/i18n/src/locales/en.i18n.json
- packages/i18n/src/locales/nn.i18n.json
🧰 Additional context used
🧬 Code graph analysis (1)
packages/gazzodown/src/Markup.stories.tsx (1)
packages/gazzodown/src/MarkupInteractionContext.ts (1)
MarkupInteractionContext(28-28)
🔇 Additional comments (3)
packages/gazzodown/src/Markup.stories.tsx (1)
18-18: LGTM: provider updated to the new context shape.No gating needed anymore.
packages/ui-client/src/hooks/useFeaturePreviewList.ts (2)
3-3: LGTM: removed timestamp feature flag from the public union.Type-level breakage is expected and correct for a removed feature.
20-62: Confirm no stale persisted entries or i18n keys remain downstream.
Repo search forenable-timestamp-message-parser/enableTimestampandEnable_timestamp(_description)?returned no matches.
Manually verify and remove any stale translations (translation repo/service) and persisted user/DB feature settings.
Proposed changes (including videos or screenshots)
Turn the timestamp parser into a full fledged feature
Issue(s)
CORE-1349
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Chores