fix(condo): reduce Message.processingMeta size#7815
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d84ffb189
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * @returns {Promise<[boolean, {error: string}]|(boolean|{})[]>} | ||
| */ | ||
| async function send ({ notification, message, data, user, remoteClient } = {}, isVoIP = false) { | ||
| async function send ({ notification, omitFromProcessingMeta: { message }, data, user, remoteClient } = {}, isVoIP = false) { |
There was a problem hiding this comment.
When send is called directly with the documented/current context shape, including existing tests that pass { message, data, remoteClient }, this nested destructuring tries to read message from an absent omitFromProcessingMeta object and throws before any push logic runs. This blocks the push transport tests and any direct adapter callers that still use the public shape; keep a fallback to the top-level message or update all callers together.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughA new constant ChangesNon-loggable message context handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DeliverMessageTask
participant PushTransport
participant ProcessingMeta
DeliverMessageTask->>PushTransport: prepareMessageToSend()
PushTransport-->>DeliverMessageTask: { omitFromProcessingMeta: { message } }
DeliverMessageTask->>PushTransport: send({ omitFromProcessingMeta: { message } })
PushTransport->>PushTransport: read message from omitFromProcessingMeta
DeliverMessageTask->>ProcessingMeta: store transportMeta.messageContext (key omitted)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/condo/domains/notification/transports/push.js`:
- Line 323: The send() function in push.js destructures message from
omitFromProcessingMeta without a default, so it throws when that nested object
is missing. Update the send() parameter handling to give omitFromProcessingMeta
its own default object before extracting message, while keeping the existing
notification, data, user, remoteClient, and isVoIP behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9977db3-98e8-4620-9258-4947de5e1346
📒 Files selected for processing (3)
apps/condo/domains/notification/constants/constants.jsapps/condo/domains/notification/tasks/deliverMessage.jsapps/condo/domains/notification/transports/push.js
| * @returns {Promise<[boolean, {error: string}]|(boolean|{})[]>} | ||
| */ | ||
| async function send ({ notification, message, data, user, remoteClient } = {}, isVoIP = false) { | ||
| async function send ({ notification, omitFromProcessingMeta: { message }, data, user, remoteClient } = {}, isVoIP = false) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify send() is always invoked with a fully-populated first argument.
rg -nP -C3 '\bsend\s*\(' apps/condo/domains/notification/transports/push.js apps/condo/domains/notification/transports/push.spec.jsRepository: open-condo-software/condo
Length of output: 8497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- push.js around send/prepareMessageToSend ---'
sed -n '250,420p' apps/condo/domains/notification/transports/push.js
echo
echo '--- all send() call sites in push.js and nearby transport files ---'
rg -n -C 2 '\bsend\s*\(' apps/condo/domains/notification/transports/push.js apps/condo/domains/notification/transports/*.js
echo
echo '--- omitFromProcessingMeta references in push transport ---'
rg -n -C 2 'omitFromProcessingMeta|prepareMessageToSend' apps/condo/domains/notification/transports/push.js apps/condo/domains/notification/transports/*.jsRepository: open-condo-software/condo
Length of output: 20494
Default omitFromProcessingMeta before destructuring message. send() will throw a TypeError whenever that field is missing; the nested object needs its own default.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/condo/domains/notification/transports/push.js` at line 323, The send()
function in push.js destructures message from omitFromProcessingMeta without a
default, so it throws when that nested object is missing. Update the send()
parameter handling to give omitFromProcessingMeta its own default object before
extracting message, while keeping the existing notification, data, user,
remoteClient, and isVoIP behavior unchanged.
Source: Learnings
1 participant
So there is some duplicates in Message.processingMeta json. Here I am removing them:
So this should reduce Message.processingMeta's size for something like 50% based on what I tested -> reduce size of Message / MessageHistoryRecord
Summary by CodeRabbit