[DX-931] Fix missing message fields for channel subscribe/history#151
[DX-931] Fix missing message fields for channel subscribe/history#151
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a shared message formatting layer (human and JSON) and refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
sacOO7
force-pushed
the
fix/pubsub-subscribe-history-formatting
branch
from
541b2d9 to
3f54bc1
Compare
sacOO7
force-pushed
the
fix/pubsub-subscribe-history-formatting
branch
from
fd28898 to
c034670
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR standardizes the output format for the channels subscribe and channels history CLI commands so they display the same set of fields (timestamp, channel, event, id, clientId, serial, data) in a consistent layout. Previously, the two commands used different field subsets and different formatting — subscribe showed only channel/event/data on a compact line, while history used an index/timestamp header with a different structure.
Changes:
- Introduced shared
formatMessagesOutput()andtoMessageJson()helpers (plusMessageDisplayFieldsinterface) insrc/utils/output.tsto ensure both commands use identical formatting logic for human-readable and JSON output. - Updated
channels subscribeandchannels historycommands to use the shared helpers, addingserialfield support, removingconnectionId/encodingfrom subscribe's JSON output, and changing history's JSON output from{ messages: [...] }to a plain array. - Updated unit tests for both commands to match the new output format; also widened the
formatJsonOutputsignature inbase-command.tsto accept arrays.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/output.ts |
Added MessageDisplayFields interface, formatMessagesOutput(), and toMessageJson() shared helpers |
src/commands/channels/subscribe.ts |
Replaced inline formatting with shared helpers; added serial field support |
src/commands/channels/history.ts |
Replaced inline formatting with shared helpers; changed JSON output to plain array |
src/base-command.ts |
Widened formatJsonOutput parameter type to accept Record<string, unknown>[] |
test/unit/commands/channels/subscribe.test.ts |
Updated assertions to match new output format fields |
test/unit/commands/channels/history.test.ts |
Updated assertions for new format and JSON structure (plain array) |
SUBSCRIBE_HISTORY_FORMATTING_PLAN.md |
New implementation plan document (should not be committed) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/output.ts`:
- Around line 80-81: The JSON branch in the output rendering uses
isJsonData(msg.data) but passes the original string to formatJson, so JSON
strings (e.g. '{"a":1}') remain quoted; modify the branch that checks
isJsonData(msg.data) to detect when msg.data is a string, parse it (e.g. const
parsed = JSON.parse(msg.data)) and pass parsed to formatJson instead of msg.data
(keep passing msg.data when it's already an object/array); reference the
isJsonData check and the formatJson call to locate the change and wrap
JSON.parse in a try/catch if you want to be defensive.
In `@SUBSCRIBE_HISTORY_FORMATTING_PLAN.md`:
- Line 6: Annotate every triple-backtick fence in
SUBSCRIBE_HISTORY_FORMATTING_PLAN.md with the appropriate language identifier
(e.g., ```text, ```json, ```typescript, ```markdown) so markdownlint MD040 stops
flagging them; update each example fence (the ones noted in the review) to use
the correct language label based on the block content (use `json` for JSON,
`typescript` for TS snippets, `markdown` for rendered markdown examples, `text`
for plain text) and ensure there are no bare ``` fences left.
In `@test/unit/commands/channels/history.test.ts`:
- Around line 74-79: Update the mocked history items to include a serial field
and replace raw millisecond timestamps with values that your code will convert
to ISO 8601, then extend the existing stdout assertions that currently check for
"Timestamp:", "Channel:", "test-channel", "[1]", and "[2]" to also assert that
"Serial:" appears and that the printed JSON payload contains the ISO 8601
timestamp string (e.g., "2021-01-01T00:00:00.000Z") instead of a millisecond
number; apply the same change pattern to the other history assertion blocks
referenced in the test so all mocks include serial and the expectations verify
ISO-formatted timestamps.
In `@test/unit/commands/channels/subscribe.test.ts`:
- Around line 128-138: Update the subscribe tests to assert the new JSON/serial
contract: when you publish the test message (the same fixture used by the
"subscribe" tests), include a "serial" field in the message payload and run both
the plain text and --json subscribe flows; add assertions that the text-mode
output contains "Serial:" and the serial value and that the JSON-mode output
parses to an object containing serial with the correct value and that it does
NOT contain legacy keys "connectionId" or "encoding"; modify the existing expect
blocks around the "Timestamp"/"Channel"/"Event"/"ID"/"Client ID"/"Data" checks
(and the similar block at the later range) to include these new positive and
negative assertions so the tests fail if serial is omitted or deprecated fields
reappear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e473cdf9-f5b2-454e-80c6-fad7faf5c18c
📒 Files selected for processing (7)
SUBSCRIBE_HISTORY_FORMATTING_PLAN.mdsrc/base-command.tssrc/commands/channels/history.tssrc/commands/channels/subscribe.tssrc/utils/output.tstest/unit/commands/channels/history.test.tstest/unit/commands/channels/subscribe.test.ts
# Conflicts: # src/commands/channels/history.ts # src/commands/channels/subscribe.ts # src/utils/output.ts
sacOO7
force-pushed
the
fix/pubsub-subscribe-history-formatting
branch
from
f27bde1 to
3d99c37
Compare
sacOO7
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
sacOO7
force-pushed
the
fix/pubsub-subscribe-history-formatting
branch
from
3d99c37 to
a08e9f2
Compare
sacOO7
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/channels/channels-e2e.test.ts (1)
294-296: Assert the new metadata fields in the E2E contract.These checks only verify
Data:/ array membership, so they would still pass ifTimestamp,Channel,Event,ID,Client ID, orSerialvanished again. Since those fields are the purpose of this PR, please add explicit assertions for them in both the human and JSON paths.Also applies to: 349-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/channels/channels-e2e.test.ts` around lines 294 - 296, Add explicit assertions for the metadata fields in both the human and JSON paths: for the human output (historyResult.stdout) add expects for the labeled fields (e.g., expect(historyResult.stdout).toContain("Timestamp:"); expect(historyResult.stdout).toContain("Channel:"); expect(historyResult.stdout).toContain("Event:"); expect(historyResult.stdout).toContain("ID:"); expect(historyResult.stdout).toContain("Client ID:"); expect(historyResult.stdout).toContain("Serial:"); and keep the existing Data/array and message checks; for the JSON path (e.g., historyJson.stdout or the variable used for JSON output) parse the JSON and assert each returned record contains keys "timestamp" (or "Timestamp" depending on JSON naming), "channel", "event", "id", "clientId"/"client_id", and "serial" and that one record's data matches "E2E History Test" to ensure both formats explicitly include the metadata fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/channels/history.ts`:
- Line 95: The history prefix currently builds indexPrefix without dimming the
index; update the construction of indexPrefix to wrap the `[${index + 1}]`
portion with chalk.dim so it matches the repo style. Locate where indexPrefix is
assigned (uses index, formatTimestamp, formatMessageTimestamp, and
message.timestamp) and change it to use chalk.dim for the bracketed index only,
keeping the existing spacing and calling
formatTimestamp(formatMessageTimestamp(message.timestamp)) after the dimmed
index.
In `@src/utils/output.ts`:
- Line 81: The shared message shape currently stores timestamp as raw epoch
milliseconds (the timestamp property) and emits it unchanged; update the code
that constructs/emits the shared message (where timestamp is set and in the
shared subscribe/history formatter functions—e.g., the functions that build/emit
shared messages and history entries) to normalize the value to a canonical
string (use new Date(timestamp).toISOString() or the existing formatter helper
if one exists) so subscribers and history emit the same formatted timestamp the
CLI expects instead of a Unix number. Ensure every place that sets or returns
the timestamp property (the shared message constructor/serializer and the
history/subscribe emit paths) performs this conversion.
- Line 152: The output builder currently assigns "data: msg.data" which allows
JSON.stringify to drop the data key when msg.data is undefined; change the
assignment in the output construction (where "data: msg.data" appears) to
normalize missing payloads to null (e.g., use msg.data ?? null) so the
serialized JSON always contains a data key for name-only messages and preserves
a stable shared JSON shape.
---
Nitpick comments:
In `@test/e2e/channels/channels-e2e.test.ts`:
- Around line 294-296: Add explicit assertions for the metadata fields in both
the human and JSON paths: for the human output (historyResult.stdout) add
expects for the labeled fields (e.g.,
expect(historyResult.stdout).toContain("Timestamp:");
expect(historyResult.stdout).toContain("Channel:");
expect(historyResult.stdout).toContain("Event:");
expect(historyResult.stdout).toContain("ID:");
expect(historyResult.stdout).toContain("Client ID:");
expect(historyResult.stdout).toContain("Serial:"); and keep the existing
Data/array and message checks; for the JSON path (e.g., historyJson.stdout or
the variable used for JSON output) parse the JSON and assert each returned
record contains keys "timestamp" (or "Timestamp" depending on JSON naming),
"channel", "event", "id", "clientId"/"client_id", and "serial" and that one
record's data matches "E2E History Test" to ensure both formats explicitly
include the metadata fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5bd63bec-1840-43a0-9c16-4288fbe2da40
📒 Files selected for processing (5)
src/base-command.tssrc/commands/channels/history.tssrc/commands/channels/subscribe.tssrc/utils/output.tstest/e2e/channels/channels-e2e.test.ts
| data: message.data, | ||
| event: message.name || "(none)", | ||
| id: message.id, | ||
| indexPrefix: `[${index + 1}] ${formatTimestamp(formatMessageTimestamp(message.timestamp))}`, |
There was a problem hiding this comment.
Dim the history index marker to match the standard format.
Line 95 leaves [${index + 1}] in normal intensity, so the prefix no longer matches the repo’s required [index] timestamp styling. Use chalk.dim() on the index segment before formatTimestamp(...).
As per coding guidelines, "Use [index] timestamp ordering format: ${chalk.dim([${index + 1}])} ${formatTimestamp(timestamp)} for history output".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/channels/history.ts` at line 95, The history prefix currently
builds indexPrefix without dimming the index; update the construction of
indexPrefix to wrap the `[${index + 1}]` portion with chalk.dim so it matches
the repo style. Locate where indexPrefix is assigned (uses index,
formatTimestamp, formatMessageTimestamp, and message.timestamp) and change it to
use chalk.dim for the bracketed index only, keeping the existing spacing and
calling formatTimestamp(formatMessageTimestamp(message.timestamp)) after the
dimmed index.
| indexPrefix?: string; | ||
| sequencePrefix?: string; | ||
| serial?: string; | ||
| timestamp: number; |
There was a problem hiding this comment.
Normalize timestamps in the shared message shape.
Line 81 stores timestamp as raw epoch milliseconds, and Lines 101-105 / 146 emit that value unchanged. That makes the new shared subscribe/history formatter surface Unix numbers instead of the formatted message timestamps the CLI uses elsewhere, so both the human and JSON outputs drift from the intended contract.
Also applies to: 101-105, 146-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/output.ts` at line 81, The shared message shape currently stores
timestamp as raw epoch milliseconds (the timestamp property) and emits it
unchanged; update the code that constructs/emits the shared message (where
timestamp is set and in the shared subscribe/history formatter functions—e.g.,
the functions that build/emit shared messages and history entries) to normalize
the value to a canonical string (use new Date(timestamp).toISOString() or the
existing formatter helper if one exists) so subscribers and history emit the
same formatted timestamp the CLI expects instead of a Unix number. Ensure every
place that sets or returns the timestamp property (the shared message
constructor/serializer and the history/subscribe emit paths) performs this
conversion.
| ...(msg.id ? { id: msg.id } : {}), | ||
| ...(msg.clientId ? { clientId: msg.clientId } : {}), | ||
| ...(msg.serial ? { serial: msg.serial } : {}), | ||
| data: msg.data, |
There was a problem hiding this comment.
Keep data present for name-only messages.
If msg.data is undefined, JSON.stringify() will drop the data key entirely, so the new shared JSON shape becomes inconsistent for messages that only carry name. Normalizing missing data to null here keeps the output stable.
💡 Proposed fix
- data: msg.data,
+ data: msg.data ?? null,📝 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.
| data: msg.data, | |
| data: msg.data ?? null, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/output.ts` at line 152, The output builder currently assigns "data:
msg.data" which allows JSON.stringify to drop the data key when msg.data is
undefined; change the assignment in the output construction (where "data:
msg.data" appears) to normalize missing payloads to null (e.g., use msg.data ??
null) so the serialized JSON always contains a data key for name-only messages
and preserves a stable shared JSON shape.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Verbose logging removed for received messages in subscribe
- Reintroduced the per-message
logCliEventcall inchannels subscribeusing the new message display fields payload.
- Reintroduced the per-message
- ✅ Fixed: History shows redundant timestamp in two different formats
- Updated history index prefixes to show only the entry number so timestamps are rendered once in the shared
Timestamp:field.
- Updated history index prefixes to show only the entry number so timestamps are rendered once in the shared
Or push these changes by commenting:
@cursor push 5226309f87
Preview (5226309f87)
diff --git a/src/commands/channels/history.ts b/src/commands/channels/history.ts
--- a/src/commands/channels/history.ts
+++ b/src/commands/channels/history.ts
@@ -7,8 +7,6 @@
import { errorMessage } from "../../utils/errors.js";
import {
formatMessagesOutput,
- formatTimestamp,
- formatMessageTimestamp,
limitWarning,
toMessageJson,
} from "../../utils/output.js";
@@ -92,7 +90,7 @@
data: message.data,
event: message.name || "(none)",
id: message.id,
- indexPrefix: `[${index + 1}] ${formatTimestamp(formatMessageTimestamp(message.timestamp))}`,
+ indexPrefix: `[${index + 1}]`,
serial: message.serial,
timestamp: message.timestamp ?? Date.now(),
}),
diff --git a/src/commands/channels/subscribe.ts b/src/commands/channels/subscribe.ts
--- a/src/commands/channels/subscribe.ts
+++ b/src/commands/channels/subscribe.ts
@@ -209,6 +209,14 @@
: {}),
};
+ this.logCliEvent(
+ flags,
+ "subscribe",
+ "messageReceived",
+ `Message received on channel ${channel.name}`,
+ toMessageJson(msgFields),
+ );
+
if (this.shouldOutputJson(flags)) {
const jsonMsg = toMessageJson(msgFields);
if (flags["sequence-numbers"]) {|
|
||
| if (this.shouldOutputJson(flags)) { | ||
| this.log(this.formatJsonOutput(messageEvent, flags)); | ||
| } else { |
There was a problem hiding this comment.
Verbose logging removed for received messages in subscribe
Low Severity
The logCliEvent call for the messageReceived event was removed during the refactoring. Previously, each received message triggered a logCliEvent(flags, "subscribe", "messageReceived", ...) call, which provided diagnostic output in --verbose mode. This was dropped when the old messageEvent variable was replaced with msgFields, causing a silent regression for users relying on verbose logging to see per-message CLI events.
| id: message.id, | ||
| indexPrefix: `[${index + 1}] ${formatTimestamp(formatMessageTimestamp(message.timestamp))}`, | ||
| serial: message.serial, | ||
| timestamp: message.timestamp ?? Date.now(), |
There was a problem hiding this comment.
History shows redundant timestamp in two different formats
Low Severity
History messages display the timestamp twice in different formats. The indexPrefix includes a formatted ISO timestamp via formatTimestamp(formatMessageTimestamp(message.timestamp)), and formatMessagesOutput also renders a Timestamp: line with raw milliseconds from msg.timestamp. Each history entry shows something like [1] [2023-11-14T22:13:20.000Z] followed by Timestamp: 1700000000000, which is redundant and inconsistent with the subscribe output that only shows the raw millisecond timestamp.
Additional Locations (1)
sacOO7
force-pushed
the
fix/pubsub-subscribe-history-formatting
branch
from
b972e93 to
e64aec5
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
| } | ||
|
|
||
| if (msg.annotations && Object.keys(msg.annotations.summary).length > 0) { |
There was a problem hiding this comment.
Accessing annotations.summary without null check crashes at runtime
High Severity
Object.keys(msg.annotations.summary) will throw a TypeError if msg.annotations exists but msg.annotations.summary is undefined or null. The guard only checks for msg.annotations being truthy, not for the presence of .summary. This can crash the CLI when a message has an annotations object without a populated summary field.
sacOO7
force-pushed
the
fix/pubsub-subscribe-history-formatting
branch
from
e64aec5 to
094cee2
Compare
Old =>
New =>
Summary by CodeRabbit
Improvements
Documentation
Tests
Note
Medium Risk
Medium risk because it changes the CLI output contract (notably
channels:history --jsonshape) and could break scripts or downstream parsing, though changes are localized to formatting/output.Overview
Standardizes
channels:subscribeandchannels:historymessage rendering by introducing shared formatting helpers (formatMessagesOutput,toMessageJson) and a commonMessageDisplayFieldsshape, so human output consistently prints labeled fields (timestamp, channel, event, id/clientId, serial, version, annotations, data).Updates JSON output to match the same field naming and content, including changing
channels:history --jsonfrom{ messages: [...] }to a plain array of message objects;formatJsonOutputnow accepts arrays. Unit/E2E tests are updated to assert the new output and added coverage forversionandannotations.Written by Cursor Bugbot for commit 094cee2. This will update automatically on new commits. Configure here.