fix(slack): preserve thread aliases in runtime outbound sends#62947
fix(slack): preserve thread aliases in runtime outbound sends#62947bek91 merged 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes dropped thread context in the generic runtime outbound sender when Slack callers supply Confidence Score: 5/5Safe to merge — the fix is correct, canonical precedence is preserved, and the new tests cover all three alias-mapping scenarios. Only a P2 style finding (double-evaluation of a null-coalescing expression). No logic errors, no missing edge-case handling, and backward compatibility is intact. No files require special attention.
|
| (opts.replyToMessageId ?? opts.replyToId) == null | ||
| ? undefined | ||
| : normalizeOptionalString(String(opts.replyToMessageId)), | ||
| : normalizeOptionalString(String(opts.replyToMessageId ?? opts.replyToId)), |
There was a problem hiding this comment.
Double evaluation of null-coalescing expression
The opts.replyToMessageId ?? opts.replyToId expression is evaluated twice — once for the null-check and once inside String(...). While safe here (simple property reads), extracting it to a local avoids the repetition and makes the intent clearer.
| (opts.replyToMessageId ?? opts.replyToId) == null | |
| ? undefined | |
| : normalizeOptionalString(String(opts.replyToMessageId)), | |
| : normalizeOptionalString(String(opts.replyToMessageId ?? opts.replyToId)), | |
| const resolvedReplyTo = opts.replyToMessageId ?? opts.replyToId; | |
| replyToId: | |
| resolvedReplyTo == null | |
| ? undefined | |
| : normalizeOptionalString(String(resolvedReplyTo)), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/send-runtime/channel-outbound-send.ts
Line: 40-42
Comment:
**Double evaluation of null-coalescing expression**
The `opts.replyToMessageId ?? opts.replyToId` expression is evaluated twice — once for the null-check and once inside `String(...)`. While safe here (simple property reads), extracting it to a local avoids the repetition and makes the intent clearer.
```suggestion
const resolvedReplyTo = opts.replyToMessageId ?? opts.replyToId;
replyToId:
resolvedReplyTo == null
? undefined
: normalizeOptionalString(String(resolvedReplyTo)),
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
512f332 to
a432bd8
Compare
|
Codex review: this looks good to me. The Only current blocker is CI: the red checks are in BlueBubbles extension lanes, which look unrelated to this PR's touched files. Please rebase/rerun; if it comes back green, I think this is mergeable. |
a432bd8 to
6d561df
Compare
Slack-threaded direct sends that go through the generic runtime wrapper now stay in the intended thread when the caller supplies threadTs.
Summary
threadTsinstead of the genericthreadId/messageThreadIdfields.threadTsas a fallback alias for the existing thread resolver, while preserving canonical precedencemessageThreadId -> threadId -> threadTs.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
threadTsalias when constructing the outbound send context.threadId/replyToIdhelpers, so the rebased fix only needed to extend thread resolution to includethreadTs.Regression Test Plan (if applicable)
src/cli/send-runtime/channel-outbound-send.test.tsthreadTs, the wrapper forwards that value as the outboundthreadId, and canonical fields still win when both forms are present.User-visible / Behavior Changes
Slack-threaded direct sends that go through the generic runtime wrapper now stay in the intended thread when the caller supplies
threadTs.Diagram (if applicable)
Security Impact (required)
NoNoNoNoNoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
threadTsset.Expected
threadTsis forwarded as the effective outbound thread id when no canonical thread field is set.Actual
Evidence
Focused verification run after rebase:
node scripts/run-vitest.mjs run --config test/vitest/vitest.cli.config.ts src/cli/send-runtime/channel-outbound-send.test.tsResult:
1test file passed,6tests passed.Human Verification (required)
What I personally verified (not just CI), and how:
main, resolved conflicts against the newer runtime-send helpers, and ran the focused CLI Vitest file covering the changed behavior.threadTsfallback, and preservation of existing direct-send media/text behavior in the same test file.pnpm checkrun because current-tree unrelated failures remain inextensions/discordandextensions/qa-lab.Review Conversations
Note: the Greptile thread is addressed in code on the rebased branch. I have not resolved the GitHub thread yet.
Compatibility / Migration
YesNoNoRisks and Mitigations
AI-Assisted
codex review --base origin/mainwas run on the rebased diff and did not find a diff-specific regression.