fix(message-parser): parse timestamps inside formatted text#39455
Open
Harshith1702 wants to merge 4 commits intoRocketChat:developfrom
Open
fix(message-parser): parse timestamps inside formatted text#39455Harshith1702 wants to merge 4 commits intoRocketChat:developfrom
Harshith1702 wants to merge 4 commits intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
🦋 Changeset detectedLatest commit: 9bf92ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughGrammar updates allow Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 2 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/message-parser/src/grammar.pegjs (1)
531-570:⚠️ Potential issue | 🟠 MajorUpdate bold/italic renderers before emitting
TIMESTAMPnodes here.These rules now produce
TIMESTAMPchildren insideBOLDandITALIC, butpackages/gazzodown/src/elements/BoldSpan.tsx:25-75andpackages/gazzodown/src/elements/ItalicSpan.tsx:25-75still fall through todefaultand returnnullfor that block type. As written,**<t:...>**and_ <t:...> _will parse, but the timestamp will disappear at render time until those spans acceptMessageParser.TimestamplikeStrikeSpanalready does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/src/grammar.pegjs` around lines 531 - 570, Update the BoldSpan and ItalicSpan renderers to handle MessageParser.Timestamp children the same way StrikeSpan does: in packages/gazzodown/src/elements/BoldSpan.tsx and ItalicSpan.tsx detect nodes of type MessageParser.Timestamp (or TIMESTAMP) in the children iteration/switch and return the appropriate timestamp element instead of falling through to default/null; mirror the handling logic used in StrikeSpan for rendering/propagating timestamp nodes and ensure any props/formatting applied to other inline children are correctly preserved for Timestamp nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/message-parser/tests/timestamp.test.ts`:
- Around line 38-47: Add a test case in
packages/message-parser/tests/timestamp.test.ts that mirrors the existing bold
assertions but uses italic markup (_..._ and/or __...__) to cover the changed
italic parsing in grammar.pegjs; specifically add an input like '_hello
<t:1708551317> world_' (and/or '__hello <t:1708551317> world__') and assert the
parsed output uses paragraph/italic/plain/timestampNode('1708551317') structure
similar to the bold test, ensuring timestampNode is nested inside the italic
node path.
- Line 1: Remove the invalid import "import { timeStamp } from 'node:console';"
from the test file (it's not a named export and is unused), and add a new unit
test in timestamp.test.ts that wraps an existing timestamp fixture with the
italic helper (e.g., use italic(`<t:${timestamp}>`) from the helpers module) to
assert the parser produces the same timestamp tokenization/AST as the non-italic
form; reference the existing timestamp test harness and the italic helper to
mirror the other timestamp assertions so italic-formatted timestamps are
covered.
---
Outside diff comments:
In `@packages/message-parser/src/grammar.pegjs`:
- Around line 531-570: Update the BoldSpan and ItalicSpan renderers to handle
MessageParser.Timestamp children the same way StrikeSpan does: in
packages/gazzodown/src/elements/BoldSpan.tsx and ItalicSpan.tsx detect nodes of
type MessageParser.Timestamp (or TIMESTAMP) in the children iteration/switch and
return the appropriate timestamp element instead of falling through to
default/null; mirror the handling logic used in StrikeSpan for
rendering/propagating timestamp nodes and ensure any props/formatting applied to
other inline children are correctly preserved for Timestamp nodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14f8a175-d471-4065-8eaa-c24608e1f1a4
📒 Files selected for processing (2)
packages/message-parser/src/grammar.pegjspackages/message-parser/tests/timestamp.test.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/message-parser/tests/timestamp.test.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
📚 Learning: 2026-03-06T18:09:14.766Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:14.766Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2026-03-06T18:02:11.732Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/RelativeTime.spec.tsx:63-70
Timestamp: 2026-03-06T18:02:11.732Z
Learning: In RocketChat/Rocket.Chat, tests in the `packages/gazzodown` package (and likely the broader test suite) are always expected to run in the UTC timezone. This makes `toLocaleString()` output deterministic, so snapshot tests that include locale/timezone-sensitive content (such as `title` attributes from `toLocaleString()`) are stable and do not need to be replaced with targeted assertions.
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/message-parser/tests/timestamp.test.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/message-parser/tests/timestamp.test.ts
Author
|
Made the changes suggested by the review:
All tests pass locally. |
Author
|
Hi maintainers, This PR fixes timestamp parsing inside formatted text (bold/italic) and includes tests. All tests pass locally. Could someone please review it and approve the workflows so CI can complete? Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes an issue where
<t:...>timestamps were not parsed when usedinside formatted text such as bold or strike.
This updates the parser grammar so
TimestampRulesare included inthe bold and italic preferential parsing patterns.
Additionally, a test case was added to verify timestamps inside
formatted text.
Summary by CodeRabbit
Bug Fixes
Tests
Chores