fix wwebjs chat list for lid message keys#748
Conversation
rmyndharis
left a comment
rmyndharis
left a comment
There was a problem hiding this comment.
Thanks for the PR @SkywardLab — patching at the build layer is a reasonable idea for this class of upstream break. I've been over the diff, though, and I can't merge this as-is. A few things:
Merge-blocker — unrelated, regressive Dockerfile change
This is bundled in but has nothing to do with the lid-key fix:
- chown -R openwa:openwa /app
+ chown -R openwa:openwa ./dataThis narrows the openwa user's ownership from the entire /app tree to only ./data. The existing chown -R openwa:openwa /app was deliberate — the runtime reads its own app files, and the surrounding Chromium/XDG setup (Dockerfile:119-128, with HOME=/app/data, XDG_CONFIG_HOME/XDG_CACHE_HOME) was built around the current ownership model. Anything that writes outside ./data at runtime risks breaking. If narrowing ownership is intentional, it belongs in its own PR with its own justification — please drop this hunk.
Scope — this fixes chat-list, not media download (#747)
To set expectations on what this actually fixes: the exact-string block it patches (chat.lastReceivedKey._serialized in whatsapp-web.js/src/util/Injected/Utils.js ~L984 — the chat last-message resolver used by getChats) is one of several call sites broken by the WA Web _serialized rename. It does not touch Message.downloadMedia() (whatsapp-web.js/src/structures/Message.js ~L513), which is the path reported in #747 (inbound photo/file download). So this is a sibling symptom of the same rename, not a fix for #747. Could you note in the description which behavior/issue this is meant to fix? Right now the body is the empty template (Closes # is blank, no description).
The .toString() fallback isn't verified
chat.lastReceivedKey?._serialized || chat.lastReceivedKey?.toString?.() — if _serialized is genuinely gone (renamed by the minifier), toString() on a WA MsgKey isn't guaranteed to return a usable serialized id, so Msg.get(...) may still resolve nothing. That risks silently degrading to "no last message" rather than actually fixing the lookup. Have you confirmed a chat list comes back with real last messages on a broken build, or only that it no longer throws? The added test only asserts the patch text gets substituted; a test asserting the resolved id round-trips through Msg.get would carry real weight.
Fragility of the build-time patch
The OLD_BLOCK exact-string match means any whatsapp-web.js bump that touches that block turns the Docker build into throw new Error('Unsupported whatsapp-web.js Utils.js shape') — a hard image-build failure the next time the dep moves. The already-patched idempotency guard is good, but the throw-on-mismatch is sharp: a routine dependency bump would take down image builds with no prior warning. Prefer warn-and-continue (or version-gate the patch) over hard-fail.
PR hygiene
Empty description, no Closes #, no CI run, all checklist boxes unchecked. For a change that patches a vendored dependency at build time, the description should call out exactly what symptom it fixes and how it was verified on a live broken session.
Suggestion
The cleaner long-term fix for the _serialized rename is upstream — wwebjs/whatsapp-web.js#201832 normalizes the id at the root (Base._normalizeId), which covers downloadMedia, getChat, getQuotedMessage, and the lastReceivedKey path in one place, rather than patching each call site. I'd prefer to track that and bump the dep once it merges. If you'd like to pursue the build-time patch as a stopgap for the chat-list symptom specifically, I'm happy to re-review once the Dockerfile change is dropped and the fallback is verified against a live broken session — please rebase and push.
…#779) The id-rename sweep taught the send (#762), ack and status (#773) paths to read `$1` when `_serialized` is absent. It missed the busiest path in the product. buildIncomingMessageBase runs on every arriving message (adapter:517 onMessage) and every message sent from a linked phone (:593 onMessage_create), and read msg.id._serialized unguarded. Worse, RawMessageFields declared the id as `{ _serialized: string }`, so on a renamed build `$1` was not merely unread but unrepresentable — the fallback could not be written without a cast, and the `id: string` the mapper produced was undefined at runtime for every inbound message. No dedup key, no reply target, nothing for an ack to match, on the highest-volume path with no defence in depth. The build-time backport normally makes it moot, which is why it survived the sweep: the one path where a patcher failure hurts most is the one that assumed it never would. The sentinel needed care. `?? ''` alone would have been a NEW bug: unlike saveOutgoingMessage, which normalizes an empty id to NULL at its chokepoint and documents why (the (sessionId, waMessageId) unique index is not partial, so NULL is exempt while a second `''` collides and the row is lost silently), the inbound persist passed incoming.id straight through. The same normalization is now applied where inbound messages are stored, and only then is `''` safe to mean "received, id unreadable". Both tests fail against the previous mapper. Also credits @SkywardLab in the chat-list changelog entry: #748 reported that site here first, independently, and the credit outlasts the PR thread. Refs #747, #762, #773, #748.
|
Thanks @SkywardLab — closing this, and I owe you a clear account of where it landed and where I got it wrong. Your diagnosis was correct. What superseded this, and where
- const lastMessage = chat.lastReceivedKey
+ const _lastReceivedKeyId = chat.lastReceivedKey
+ ? chat.lastReceivedKey._serialized || chat.lastReceivedKey.$1
+ : null;
+ const lastMessage = _lastReceivedKeyId
? window
.require('WAWebCollections')
- .Msg.get(chat.lastReceivedKey._serialized) ||
+ .Msg.get(_lastReceivedKeyId) ||It runs at On the fallback itself: you used Where I have to correct myselfOn the hard-fail. I told you the throw-on-mismatch was too sharp and to prefer warn-and-continue or a version gate. The patcher I shipped hard-fails the image build on drift as well — On the process. I closed my review with "if you'd like to pursue the build-time patch as a stopgap, I'm happy to re-review... please rebase and push." About 25 hours later I opened #758/#759 doing that stopgap myself and merged it, without coming back here to tell you. That's on me. I left you holding an open invitation to do work I then did myself, and you should have heard it in this thread rather than from the changelog. The plan itself hasn't changed — upstream #201832 is still open and the backport still stands down when it lands — only who did the interim work, and I should have said so here. On priority, since I'd rather state it than have it inferred: upstream #201832 was opened at The Why closing rather than asking for a rebaseThere's nothing left to rebase onto. The block is already rewritten on I've credited #748 in the changelog entry for the chat-list fix ( |
3 participants
Description
Brief description of changes
Type of Change
Checklist
Screenshots (if applicable)
Related Issues
Closes #