feat(ui): Add option to disable auto mark as read#13168
Conversation
|
Thank you for the contribution! |
|
@nimishavijay small decision needed. Our friends have a setting for this as well but with a few options 
(excuse the German UI, you get the idea 🙊) Should we keep it binary (on vs off) or also offer a few options like off, instant, 3 seconds and 30 seconds? |
093040c to
97bb494
Compare
97bb494 to
1e84dfc
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
Initially I thought adding a timer would be overengineering but I checked outlook and they also seem to have a timer setting (support link), so I'm guessing it would be a gap if we don't have it. Your timer options also sound great @ChristophWurst so we could go with that :) |
1e84dfc to
61e41e6
Compare
|
Thanks @ChristophWurst and @nimishavijay! That makes total sense. I have updated the PR to replace the binary toggle with the proposed radio group options (Immediately, After 3 seconds, After 30 seconds, and Manually). Backward compatibility for existing preference values has also been included. It looks like this now: 
|
61e41e6 to
1606737
Compare
1606737 to
dbe9df4
Compare
|
Conflicts |
dbe9df4 to
86c544d
Compare
|
@ChristophWurst, Rebased onto current main and resolved the conflicts. |
86c544d to
dd07474
Compare
| if (value === 'true') { | ||
| return '3000' | ||
| } | ||
| if (value === 'false') { | ||
| return '-1' | ||
| } | ||
| return value |
There was a problem hiding this comment.
true/false are unexpected except for your dev env. this doesn't need handling.
dd07474 to
1b70b51
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds a configurable auto-mark-as-read preference with immediate, delayed, and manual modes. Server initial state and client store hydration carry the preference, settings persist it, and ChangesAuto-mark-as-read preference
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AppSettingsMenu
participant mainStore
participant PageController
participant ThreadEnvelope
AppSettingsMenu->>mainStore: Save auto-mark-as-read preference
PageController->>ThreadEnvelope: Provide initial preference state
ThreadEnvelope->>ThreadEnvelope: Parse configured delay
ThreadEnvelope->>ThreadEnvelope: Mark immediately or schedule toggleEnvelopeSeen
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/Controller/PageControllerTest.php (1)
191-208: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the exact initial-state expectation.
testIndex()now mocksauto-mark-as-read, but its expectedpreferencesarray still omits'auto-mark-as-read' => '3000'. BecauseprovideInitialState()is asserted with the complete array, this test will fail.Proposed fix
'compact-mode' => 'false' + 'auto-mark-as-read' => '3000'
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 93a5b030-fd7d-481f-9e9e-20c8565d7cc2
📒 Files selected for processing (5)
lib/Controller/PageController.phpsrc/components/AppSettingsMenu.vuesrc/components/ThreadEnvelope.vuesrc/init.jstests/Unit/Controller/PageControllerTest.php
| const autoMarkReadSetting = this.mainStore.getPreference('auto-mark-as-read', '3000') | ||
| const delay = parseInt(autoMarkReadSetting, 10) | ||
|
|
||
| if (!this.envelope.flags.seen && this.hasSeenAcl && delay >= 0) { | ||
| if (delay === 0) { | ||
| logger.info('Marking message as seen/read immediately') | ||
| this.mainStore.toggleEnvelopeSeen({ envelope: this.envelope }) | ||
| this.seenTimer = undefined | ||
| }, 2000) | ||
| } else { | ||
| logger.info(`Starting timer (${delay}ms) to mark message as seen/read`) | ||
| this.seenTimer = setTimeout(() => { | ||
| this.mainStore.toggleEnvelopeSeen({ envelope: this.envelope }) | ||
| this.seenTimer = undefined | ||
| }, delay) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Guard delayed timers before changing read state.
The timeout callback unconditionally toggles the envelope. A manual read action before the timeout can therefore toggle the message back to unread, and switching to “Manually” while the timer is pending still marks it automatically. Re-check the current seen state and preference before toggling, and clear pending timers when the envelope is collapsed.
There was a problem hiding this comment.
Checking if the setting changes is overkill.
There was a problem hiding this comment.
@ChristophWurst, agreed—the pending timer does not need to react to preference changes. I’ll narrow this to only avoiding a stale timer toggling a message back to unread after it was manually marked as read.
✏️ Learnings added
Learnt from: ChristophWurst
Repo: nextcloud/mail PR: 13168
File: src/components/ThreadEnvelope.vue:856-869
Timestamp: 2026-07-24T11:53:01.429Z
Learning: For the configurable auto-mark-as-read timer in `src/components/ThreadEnvelope.vue`, do not require pending timers to react to preference changes; that behavior is considered overkill. Review stale-timer behavior independently from settings changes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
But the manual intervention is an interesting case. If that happened we should not toggle. Example:
- User set 30s timeout
- User opens email, reads it quickly and 10s decides to mark it read
- Use navigates to next email
Email will be marked unread 20s later
There was a problem hiding this comment.
@ChristophWurst, agreed. The callback must guard against the message already being read before calling the toggle:
this.seenTimer = setTimeout(() => {
if (!this.envelope.flags.seen) {
this.mainStore.toggleEnvelopeSeen({ envelope: this.envelope })
}
this.seenTimer = undefined
}, delay)That prevents the pending 30-second timer from turning a manually read message back into unread, without coupling existing timers to later preference changes.
✏️ Learnings added
Learnt from: ChristophWurst
Repo: nextcloud/mail PR: 13168
File: src/components/ThreadEnvelope.vue:856-869
Timestamp: 2026-07-24T11:54:18.413Z
Learning: In `src/components/ThreadEnvelope.vue`, the configurable auto-mark-as-read timeout must not call `mainStore.toggleEnvelopeSeen` when the envelope has already been manually marked as read before the timer expires. Because `toggleEnvelopeSeen` toggles rather than unconditionally marks as read, its timeout callback must guard with `!this.envelope.flags.seen`. Pending timers do not need to react to preference changes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@ChristophWurst, Good Point. Added a check if (!this.envelope.flags.seen) inside the timer callback to prevent toggling the message back to unread if it was already manually marked as read before the timer expires.
df63599 to
e860e20
Compare
| const delay = parseInt(autoMarkReadSetting, 10) | ||
|
|
||
| if (!this.envelope.flags.seen && this.hasSeenAcl && delay >= 0) { | ||
| if (delay === 0) { |
There was a problem hiding this comment.
Could we unify the delay branching here and always go through the same setTimeout path? Using a delay of 0 should preserve the behavior while reducing the number of code paths we need to maintain.
That would make the read-timing logic a bit easier to follow and lowers the chance that one branch drifts from the other over time.
There was a problem hiding this comment.
Thanks @kesselb! Unified the branching to always use the setTimeout path for delay >= 0.
e860e20 to
40b0b0d
Compare
Signed-off-by: SoleroTG <github-29h@solero.quietmail.eu>
40b0b0d to
e261143
Compare
| const delay = parseInt(autoMarkReadSetting, 10) | ||
|
|
||
| if (!this.envelope.flags.seen && this.hasSeenAcl && delay >= 0) { | ||
| logger.info(`Starting timer (${delay}ms) to mark message as seen/read`) |
There was a problem hiding this comment.
| logger.info(`Starting timer (${delay}ms) to mark message as seen/read`) | |
| logger.debug(`Starting timer (${delay}ms) to mark message as seen/read`) |
4 participants
Fixes #13061
Summary
This Pull Request introduces a user preference option to fully disable the "auto mark as read" feature when opening a message in Nextcloud Mail (#13061). When disabled, viewing/skimming messages does not automatically change their status to read, making "mark as read" an explicitly manual action.
Key Changes
"Automatically mark messages as read when opened"in the App Settings Menu under the "Messages" section.auto-mark-as-readuser preference key, integrated it into the backend PHP initial state payload (PageController), and synced it with the frontend Vue Pinia store state on application bootstrap (init.js).ThreadEnvelope.vue(fetchMessage()) to retrieve and check the value ofauto-mark-as-read(defaulting totrue) before starting the 2000ms delay timer that toggles the envelope toseen.How to Test
npm run devornpm run watch).seenflag is sent to the server).Checklist
npm run lintandcomposer run cs:check)npm run test:unit)🤖 AI (if applicable)
CC: @GretaD (as you previously expressed interest in this feature/discussion)
Summary by CodeRabbit
New Features
Bug Fixes