feat: Add global decrypt/encrypt all buttons for messages#13
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a per-thread message decryption toggle to XorCrypt. A new ChangesMessage Decryption Toggle
Sequence DiagramsequenceDiagram
participant User
participant ThreadHeader
participant State
participant Renderer
User->>ThreadHeader: click decrypt/encrypt button
ThreadHeader->>State: set messagesDecrypted (true/false)
ThreadHeader->>Renderer: call renderDmMessages() or renderGroupMessages()
Renderer->>State: read messagesDecrypted flag
Renderer->>Renderer: re-render message bubbles (conditional plaintext display)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
- Replace per-message decrypt buttons with global 'decrypt all' and 'encrypt all' buttons in message thread headers - Messages now appear encrypted by default in DMs and group chats - Global buttons toggle visibility of all messages in the current thread at once - State resets when switching between conversations
3dfcd2e to
9d3dac4
Compare
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 `@static/js/app.js`:
- Around line 1152-1153: Do not unconditionally set state.messagesDecrypted =
false on every fetch; instead detect a thread change and only reset when the
active thread id changes: compare the previous saved thread id (e.g.,
state.currentThreadId) to the incoming/selected thread id (e.g., newThreadId or
selectedThreadId derived where the fetch/reload runs) and only set
state.messagesDecrypted = false and update state.currentThreadId = newThreadId
when they differ; apply this conditional logic in both locations where
state.messagesDecrypted is currently reset (the two occurrences of
state.messagesDecrypted = false).
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 67f63105-c2c9-4562-80a9-67d311e060da
📒 Files selected for processing (1)
static/js/app.js
| // Reset decryption state for new thread | ||
| state.messagesDecrypted = false; |
There was a problem hiding this comment.
Reset decryption state only when thread changes, not on every reload.
Line [1152] and Line [1171] execute on every fetch path (polling, manual refresh, and post-send reload), so decrypt all gets undone even when the user stays in the same conversation. This breaks the intended behavior and causes UX flicker.
Proposed fix
async function openDmThread(peerId) {
+ const switchedThread =
+ state.activeThread?.kind !== "dm" || state.activeThread?.peerId !== peerId;
const t = state.threads.find(x => x.peerId === peerId);
const peerEmail = t?.peerEmail || state.activeThread?.peerEmail || "";
state.activeThread = { kind: "dm", peerId, peerEmail };
+ if (switchedThread) state.messagesDecrypted = false;
msgEls.header.innerHTML = `
...
`; async function openGroupThread(groupId) {
+ const switchedThread =
+ state.activeThread?.kind !== "group" || state.activeThread?.groupId !== groupId;
const g = state.groups.find(x => x.id === groupId);
if (!g) return;
if (!g.key) { toast("Could not unwrap this group's key — re-join with the code.", "error"); return; }
state.activeThread = { kind: "group", groupId, name: g.name, key: g.key };
+ if (switchedThread) state.messagesDecrypted = false;
msgEls.header.innerHTML = `
...
`; async function loadDmMessages(peerId) {
try {
state.messages = await api.get(`/api/messages?peer=${peerId}&limit=200`);
- // Reset decryption state for new thread
- state.messagesDecrypted = false;
await renderDmMessages(); async function loadGroupMessages(groupId) {
try {
state.messages = await api.get(`/api/groups/${groupId}/messages?limit=200`);
- // Reset decryption state for new thread
- state.messagesDecrypted = false;
await renderGroupMessages();Also applies to: 1171-1172
🤖 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 `@static/js/app.js` around lines 1152 - 1153, Do not unconditionally set
state.messagesDecrypted = false on every fetch; instead detect a thread change
and only reset when the active thread id changes: compare the previous saved
thread id (e.g., state.currentThreadId) to the incoming/selected thread id
(e.g., newThreadId or selectedThreadId derived where the fetch/reload runs) and
only set state.messagesDecrypted = false and update state.currentThreadId =
newThreadId when they differ; apply this conditional logic in both locations
where state.messagesDecrypted is currently reset (the two occurrences of
state.messagesDecrypted = false).
There was a problem hiding this comment.
Pull Request Overview
While the PR successfully implements the global toggle functionality, it introduces a performance bottleneck and a regression in error reporting. Specifically, the application now performs sequential decryption for every message in a thread upon loading, regardless of whether the user has clicked 'decrypt all', which will cause significant UI lag. Additionally, the UI now obscures genuine decryption failures (like missing keys) by using the same 'encrypted' label for both hidden and failed states. These issues, along with missing automated tests for the state transitions, should be addressed before merging.
About this PR
- There is significant logic duplication between the 'openDmThread' and 'openGroupThread' functions regarding the new global toggle buttons and event listeners. This pattern is prone to bugs and increases maintenance overhead.
- The PR description mentions replacing per-message decrypt buttons, but the implementation does not appear to remove them from the message bubble actions. Please verify if the intention was to maintain both or if the removal was missed.
Test suggestions
- Verify messages are rendered as 'encrypted' with a lock icon when a thread is first loaded.
- Verify 'decrypt all' button updates the global state and triggers a re-render of messages.
- Verify 'encrypt all' button hides decrypted content and restores the 'encrypted' meta text.
- Verify decryption state resets to false when calling loadDmMessages for a different peerId.
- Verify decryption state resets to false when calling loadGroupMessages for a different groupId.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify messages are rendered as 'encrypted' with a lock icon when a thread is first loaded.
2. Verify 'decrypt all' button updates the global state and triggers a re-render of messages.
3. Verify 'encrypt all' button hides decrypted content and restores the 'encrypted' meta text.
4. Verify decryption state resets to false when calling loadDmMessages for a different peerId.
5. Verify decryption state resets to false when calling loadGroupMessages for a different groupId.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| const meta = isDecrypted && decrypted.ok | ||
| ? `<span>${escapeHtml(opts.label || decrypted.keyLabel || "key")}</span><span>·</span><span>${fmtTime(m.createdAt)}</span>` | ||
| : `<span>can't decrypt</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; | ||
| : `<span>encrypted</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
This logic obscures decryption failures. When 'messagesDecrypted' is true but a message cannot be decrypted, the UI should still show 'can't decrypt' to inform the user that a key is missing, rather than the generic 'encrypted' state used for the global toggle.
| const meta = isDecrypted && decrypted.ok | |
| ? `<span>${escapeHtml(opts.label || decrypted.keyLabel || "key")}</span><span>·</span><span>${fmtTime(m.createdAt)}</span>` | |
| : `<span>can't decrypt</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; | |
| : `<span>encrypted</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; | |
| const meta = isDecrypted | |
| ? (decrypted.ok ? `<span>${escapeHtml(opts.label || decrypted.keyLabel || "key")}</span><span>·</span><span>${fmtTime(m.createdAt)}</span>` : `<span>can't decrypt</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`) | |
| : `<span>encrypted</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; |
|
|
||
| function renderBubble(m, decrypted, side, opts = {}) { | ||
| const meta = decrypted.ok | ||
| const isDecrypted = state.messagesDecrypted; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Expensive decryption is performed for all messages sequentially even when 'state.messagesDecrypted' is false. This causes significant UI lag when opening threads or refreshing, as the rendering loop awaits decryption for every message before displaying the list.
| const isDecrypted = state.messagesDecrypted; | |
| const r = state.messagesDecrypted ? await tryDecryptDm(m.ciphertext, state.activeThread.peerId) : { ok: false }; |
| <button class="icon-btn" id="msg-decrypt-all" type="button">decrypt all</button> | ||
| <button class="icon-btn" id="msg-encrypt-all" type="button">encrypt all</button> | ||
| <button class="icon-btn" id="msg-refresh" type="button">refresh</button> | ||
| <button class="icon-btn" id="msg-block" type="button">block</button> | ||
| </div> | ||
| `; | ||
| $("msg-decrypt-all").addEventListener("click", () => { | ||
| state.messagesDecrypted = true; | ||
| renderDmMessages(); | ||
| }); | ||
| $("msg-encrypt-all").addEventListener("click", () => { | ||
| state.messagesDecrypted = false; | ||
| renderDmMessages(); | ||
| }); |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Duplicate header button logic found in both openDmThread and openGroupThread. Consider extracting this to a shared function to improve maintainability.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
There was a problem hiding this comment.
Pull Request Overview
The PR successfully implements the global decryption toggle system but introduces a major performance bottleneck and a UI regression.
Specifically, the implementation performs sequential cryptographic decryption for every message in a thread even when the global state is set to 'encrypted', which will lead to significant UI lag and unnecessary battery drain. Additionally, the new logic conflates 'privacy mode' with 'decryption failure' in the UI, making it impossible for users to diagnose missing keys.
There is also a discrepancy between the PR description and the implementation: the description mentions replacing per-message buttons, but those buttons have not been removed from the code. These issues should be addressed before merging.
About this PR
- PR Description Discrepancy: The description states that per-message decrypt buttons are being replaced/removed, but the
b-actionscontainer inrenderBubbleremains unchanged. Please update the code to remove the legacy buttons or update the description. - Systemic Redundancy: The logic for creating buttons and attaching event listeners is duplicated almost exactly between
openDmThreadandopenGroupThread. Consider refactoring this into a shared utility function to improve maintainability.
1 comment outside of the diff
static/js/app.js
line 1293🔴 HIGH RISK
Suggestion: Performance bottleneck: Every message in the thread is decrypted sequentially usingtryDecryptDmbefore rendering, even ifstate.messagesDecryptedis false. This causes significant UI blocking and battery drain.Optimization Suggestion:
- Skip the
tryDecryptDmcall ifstate.messagesDecryptedis false.- Parallelize the remaining decryption calls using
Promise.allto avoid the sequential await bottleneck.Example fix:
const r = state.messagesDecrypted ? await tryDecryptDm(m.ciphertext, state.activeThread.peerId) : { ok: false };
Test suggestions
- Verify messages are rendered as encrypted upon initial load of a DM thread.
- Verify 'decrypt all' button in DM header triggers re-render of messages in decrypted state.
- Verify 'encrypt all' button in Group header triggers re-render of messages in encrypted state.
- Verify switching from DM thread A to DM thread B resets state.messagesDecrypted to false.
- Verify performance of global toggle with a large number of messages (checking for UI lag due to decryption loop).
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify messages are rendered as encrypted upon initial load of a DM thread.
2. Verify 'decrypt all' button in DM header triggers re-render of messages in decrypted state.
3. Verify 'encrypt all' button in Group header triggers re-render of messages in encrypted state.
4. Verify switching from DM thread A to DM thread B resets state.messagesDecrypted to false.
5. Verify performance of global toggle with a large number of messages (checking for UI lag due to decryption loop).
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| `; | ||
| $("msg-decrypt-all").addEventListener("click", () => { | ||
| state.messagesDecrypted = true; | ||
| renderDmMessages(); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Race Condition Risk: The global toggle buttons trigger asynchronous render functions without execution guards. Rapidly clicking these buttons can cause multiple decryption loops to run concurrently, leading to flickering or inconsistent message displays. These handlers should be guarded to prevent concurrent execution.
| const meta = isDecrypted && decrypted.ok | ||
| ? `<span>${escapeHtml(opts.label || decrypted.keyLabel || "key")}</span><span>·</span><span>${fmtTime(m.createdAt)}</span>` | ||
| : `<span>can't decrypt</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; | ||
| : `<span>encrypted</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
UI Regression: Distinguish between the globally encrypted state and an actual decryption failure. The current implementation displays 'encrypted' even when a decryption attempt fails, which prevents users from identifying key issues.
| const meta = isDecrypted && decrypted.ok | |
| ? `<span>${escapeHtml(opts.label || decrypted.keyLabel || "key")}</span><span>·</span><span>${fmtTime(m.createdAt)}</span>` | |
| : `<span>can't decrypt</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; | |
| : `<span>encrypted</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; | |
| const meta = isDecrypted ? (decrypted.ok ? `<span>${escapeHtml(opts.label || decrypted.keyLabel || "key")}</span><span>·</span><span>${fmtTime(m.createdAt)}</span>` : `<span>can't decrypt</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`) : `<span>encrypted</span><span>·</span><span>${fmtTime(m.createdAt)}</span>`; |
Replace per-message decrypt buttons with global 'decrypt all' and 'encrypt all' buttons in message thread headers.
Changes:
Testing:
Summary by CodeRabbit
New Features
Style