Reader: FreshRSS-style keyboard shortcuts and mark-groups-read-on-view setting #28

Manually merged
marvin8 merged 6 commits from feat/issue-5-keyboard-shortcuts into main 2026-09-12 06:53:01 +00:00
Collaborator

Implements #5 plus the companion reading setting requested in tandem.

Keyboard shortcuts (#5)

Progressive enhancement only — a single static shortcuts.js; every action drives links/forms/routes that already work without JavaScript (the sanctioned minimal exception to the no-JS rule, recorded in the issue findings).

  • j/k (and n/p) — move between items; visible focus indicator, scrolls into view
  • h — jump to next unread article
  • r — toggle read state of the current article (fetch POST to the existing read/unread routes, badge and state update in place)
  • space — open the current article on its original website (new tab)
  • o/enter — open the current item
  • c — collapse/expand the combined summary on aggregate pages
  • 1/2/3 — go to Reader / Feeds / Settings
  • f1 or ? — help overlay; escape closes it
  • Skipped as having no yunjin equivalent: q (no refresh route — fetching stays CLI-side), m (no pagination), f/l/s/a/u (no favourites/labels/sharing/search)
  • Keys are ignored while typing in inputs/textareas/selects and with modifier keys held

Server-rendered HTML contract (data attributes for items, article URLs, read state) covered by route tests; JS itself has no test infrastructure — manual checklist in the comments below.

Mark-groups-read-on-view setting (companion, Marvin8 request)

  • New setting "Mark groups as read when viewed" (default off) under Settings → Reading
  • When on, viewing a group marks all its articles read — rendered with the pre-marking read states so the page does not change under you — which makes h, hide-read, and the reader index a true unread-workflow

226 tests, all 9 nox sessions green.

Closes #5

Implements #5 plus the companion reading setting requested in tandem. ## Keyboard shortcuts (#5) Progressive enhancement only — a single static `shortcuts.js`; every action drives links/forms/routes that already work without JavaScript (the sanctioned minimal exception to the no-JS rule, recorded in the issue findings). - `j`/`k` (and `n`/`p`) — move between items; visible focus indicator, scrolls into view - `h` — jump to next unread article - `r` — toggle read state of the current article (fetch POST to the existing read/unread routes, badge and state update in place) - `space` — open the current article on its original website (new tab) - `o`/`enter` — open the current item - `c` — collapse/expand the combined summary on aggregate pages - `1`/`2`/`3` — go to Reader / Feeds / Settings - `f1` or `?` — help overlay; `escape` closes it - Skipped as having no yunjin equivalent: `q` (no refresh route — fetching stays CLI-side), `m` (no pagination), `f`/`l`/`s`/`a`/`u` (no favourites/labels/sharing/search) - Keys are ignored while typing in inputs/textareas/selects and with modifier keys held Server-rendered HTML contract (data attributes for items, article URLs, read state) covered by route tests; JS itself has no test infrastructure — manual checklist in the comments below. ## Mark-groups-read-on-view setting (companion, Marvin8 request) - New setting **"Mark groups as read when viewed"** (default off) under Settings → Reading - When on, viewing a group marks all its articles read — rendered with the pre-marking read states so the page does not change under you — which makes `h`, hide-read, and the reader index a true unread-workflow 226 tests, all 9 nox sessions green. Closes #5
Add setting to mark groups read when viewed
All checks were successful
/ gitleaks (pull_request) Successful in 14s
/ checks (pull_request) Successful in 1m21s
/ pr-review (pull_request) Successful in 4m34s
64000265a8
Author
Collaborator

Manual verification checklist (no JS test infrastructure in this project)

Server-side behaviour is covered by 12 new tests (HTML contract: data attributes, script inclusion, static asset, read-on-view semantics). The JavaScript itself needs eyeballs:

  1. Reader index: j/k move the blue outline between group cards, scrolling follows; o/enter opens the focused group; 1/2/3 jump between pages.
  2. Aggregate page: j/k walk the article list; space opens the focused article's original site in a new tab; o/enter opens the article detail; r toggles Read/Unread badge in place (persists after reload); c collapses/expands the Combined Summary card.
  3. With Settings → Reading → Mark groups as read when viewed enabled: opening a group returns to a reader index where that group is read (disappears with Hide read on); h skips to the next group with unread articles.
  4. f1 or ? opens the help overlay; escape or clicking the backdrop closes it.
  5. Typing in the settings form fields (e.g. the custom CSS box) does not trigger shortcuts.
  6. With JavaScript disabled, everything still works via the existing links and forms — no layout shift from the script tag.
## Manual verification checklist (no JS test infrastructure in this project) Server-side behaviour is covered by 12 new tests (HTML contract: data attributes, script inclusion, static asset, read-on-view semantics). The JavaScript itself needs eyeballs: 1. Reader index: `j`/`k` move the blue outline between group cards, scrolling follows; `o`/`enter` opens the focused group; `1`/`2`/`3` jump between pages. 2. Aggregate page: `j`/`k` walk the article list; `space` opens the focused article's original site in a new tab; `o`/`enter` opens the article detail; `r` toggles Read/Unread badge in place (persists after reload); `c` collapses/expands the Combined Summary card. 3. With Settings → Reading → *Mark groups as read when viewed* enabled: opening a group returns to a reader index where that group is read (disappears with Hide read on); `h` skips to the next group with unread articles. 4. `f1` or `?` opens the help overlay; `escape` or clicking the backdrop closes it. 5. Typing in the settings form fields (e.g. the custom CSS box) does not trigger shortcuts. 6. With JavaScript disabled, everything still works via the existing links and forms — no layout shift from the script tag.
forgejo-actions left a comment

WuMing

Found 6 issue(s). See inline comments below.

## WuMing Found **6** issue(s). See inline comments below.
@ -160,0 +187,4 @@
if settings_db.get_setting(db, user_id=user_id, key="mark_read_on_view") == "true":
for article_id in article_ids:
if not users.is_article_read(db, user_id, article_id):
users.mark_article_read(db, user_id, article_id)

security [LOW]

This causes a database state change during a GET request when mark_read_on_view is enabled. A cross-site image or fetch can trigger /aggregate/ and mark groups read for a logged-in user. State changes should be made via POST with CSRF protection, not as a side effect of GET navigation.

**security** [LOW] This causes a database state change during a GET request when mark_read_on_view is enabled. A cross-site image or fetch can trigger /aggregate/<id> and mark groups read for a logged-in user. State changes should be made via POST with CSRF protection, not as a side effect of GET navigation. <!-- wuming:sha256:cabd34d0cb846de76cbc819338a1b111f21d08dcd6f84ab0a285e7d617a8dc11 -->
@ -106,6 +108,29 @@ def save_display_settings() -> ResponseReturnValue:
return redirect(url_for("settings.manage_tags"))
@bp.route("/reading", methods=["POST"])

security [MEDIUM]

The new state-changing POST route does not validate a CSRF token. An attacker-controlled page can auto-submit a form to /settings/reading and change the victim's mark_read_on_view setting. Add CSRF protection to all state-changing routes.

**security** [MEDIUM] The new state-changing POST route does not validate a CSRF token. An attacker-controlled page can auto-submit a form to /settings/reading and change the victim's mark_read_on_view setting. Add CSRF protection to all state-changing routes. <!-- wuming:sha256:3257566f280d2fffde7befdab59f9983dfea16f5ff310f6caca9e4ed1dba812e -->
@ -109,0 +117,4 @@
"""
db = get_db()
user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)

security [LOW]

Settings are read and written using a hard-coded user_id=1 rather than the authenticated session's user identity. This is an insecure direct object reference/missing authorization once multiple users exist, or if any user context is supplied. Use the current authenticated user ID.

**security** [LOW] Settings are read and written using a hard-coded user_id=1 rather than the authenticated session's user identity. This is an insecure direct object reference/missing authorization once multiple users exist, or if any user context is supplied. Use the current authenticated user ID. <!-- wuming:sha256:6a2e5a095bdd1565b2c21767efd8379da9282bfdebca4d90a27d80b69e177073 -->
@ -0,0 +1,243 @@
/**

tests [LOW]

New non-trivial source file (key/state handling, fetch-based read toggle, overlay construction, index/data-* bookkeeping) is added with no automated tests. The route tests added in tests/test_web.py only assert the server-rendered HTML contract (data-shortcut-item, data-article-url, data-read), not any of the JS behaviour (key dispatch, text-entry/modifier guards, move/nextUnread boundaries, toggleRead state flips). If a browser/JS test harness is unavailable, at minimum extract the pure helpers (move/bounds, isTextEntryTarget, help row rendering) so they can be unit-tested in isolation, and record the manual checklist as a tracked artifact.

**tests** [LOW] New non-trivial source file (key/state handling, fetch-based read toggle, overlay construction, index/`data-*` bookkeeping) is added with no automated tests. The route tests added in tests/test_web.py only assert the server-rendered HTML contract (`data-shortcut-item`, `data-article-url`, `data-read`), not any of the JS behaviour (key dispatch, text-entry/modifier guards, move/nextUnread boundaries, toggleRead state flips). If a browser/JS test harness is unavailable, at minimum extract the pure helpers (move/bounds, isTextEntryTarget, help row rendering) so they can be unit-tested in isolation, and record the manual checklist as a tracked artifact. <!-- wuming:sha256:e3a08856d183ad0e6890fe887f8d502527b55d3ad5075d5b004d0a7c6a4e1b6b -->
@ -0,0 +125,4 @@
if (!element || !element.dataset.articleUrl) {
return;
}
window.open(element.dataset.articleUrl, "_blank", "noopener");

security [MEDIUM]

element.dataset.articleUrl is derived from feed/article data and is opened without validating the URL scheme. A malicious feed item could use a javascript: or data: URL, enabling script execution or phishing via the space shortcut. Restrict to http/https schemes.

**security** [MEDIUM] element.dataset.articleUrl is derived from feed/article data and is opened without validating the URL scheme. A malicious feed item could use a javascript: or data: URL, enabling script execution or phishing via the space shortcut. Restrict to http/https schemes. <!-- wuming:sha256:7b869d94d3d05eb391d56501c7f25056afed1e82d2682d27f875cca206ad421e -->
@ -0,0 +138,4 @@
element.getAttribute("href") ||
element.dataset.articleUrl;
if (url) {
window.location.href = url;

security [HIGH]

Unsanitized dataset value is assigned to window.location.href. If a feed item can supply a javascript: or other dangerous URL, pressing o/Enter executes script in the application origin (DOM XSS). Validate that the target URL uses http/https before navigation.

**security** [HIGH] Unsanitized dataset value is assigned to window.location.href. If a feed item can supply a javascript: or other dangerous URL, pressing o/Enter executes script in the application origin (DOM XSS). Validate that the target URL uses http/https before navigation. <!-- wuming:sha256:647654bea7b0dd45a31b8f8cadba194e048eb9ac06f17c827e327d0dbc23bd9a -->
🔒 Restrict shortcut navigation to http/https URLs
All checks were successful
/ gitleaks (pull_request) Successful in 13s
/ checks (pull_request) Successful in 1m18s
/ pr-review (pull_request) Successful in 4m24s
0d12a82842
Author
Collaborator

src/yunjin/web/static/shortcuts.js

Unsanitized dataset value is assigned to window.location.href…

Fixed — isSafeUrl() parses the target with the URL constructor and only navigates for http:/https: protocols; javascript:/data: and unparseable values are inert. Applies to both o/enter and the original-site open. Related note: the server-rendered <a href> links carry the same feed-supplied URLs (Jinja-escaped but not scheme-checked) — scheme validation at article ingest is a sensible follow-up beyond this PR's scope.

[`src/yunjin/web/static/shortcuts.js`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2782) > Unsanitized dataset value is assigned to window.location.href… ✅ Fixed — `isSafeUrl()` parses the target with the URL constructor and only navigates for `http:`/`https:` protocols; `javascript:`/`data:` and unparseable values are inert. Applies to both `o`/`enter` and the original-site open. Related note: the server-rendered `<a href>` links carry the same feed-supplied URLs (Jinja-escaped but not scheme-checked) — scheme validation at article ingest is a sensible follow-up beyond this PR's scope.
Author
Collaborator

src/yunjin/web/static/shortcuts.js

element.dataset.articleUrl is derived from feed/article data and is opened without validating the URL scheme…

Fixed — same isSafeUrl() guard applied to the window.open path (space shortcut); duplicate of the navigation finding above.

[`src/yunjin/web/static/shortcuts.js`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2783) > element.dataset.articleUrl is derived from feed/article data and is opened without validating the URL scheme… ✅ Fixed — same `isSafeUrl()` guard applied to the `window.open` path (`space` shortcut); duplicate of the navigation finding above.
Author
Collaborator

src/yunjin/web/routes/reader.py

This causes a database state change during a GET request when mark_read_on_view is enabled…

🔴 Not actioned — the state change on view is the feature's defining semantic ("viewed = read"), standard in feed readers; requiring a POST to view a page would break it. Harm is low and reversible (unread toggle), and with no sessions today a cross-site image is indistinguishable from a direct request — once #25 adds authenticated sessions with SameSite cookies, the cross-site-image vector closes automatically. Noted there.

[`src/yunjin/web/routes/reader.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2786) > This causes a database state change during a GET request when mark_read_on_view is enabled… 🔴 Not actioned — the state change on view is the feature's defining semantic ("viewed = read"), standard in feed readers; requiring a POST to view a page would break it. Harm is low and reversible (unread toggle), and with no sessions today a cross-site image is indistinguishable from a direct request — once #25 adds authenticated sessions with SameSite cookies, the cross-site-image vector closes automatically. Noted there.
Author
Collaborator

src/yunjin/web/routes/settings.py

The new state-changing POST route does not validate a CSRF token…

🔴 Not actioned in this PR — CSRF protection spans every state-changing route and is tracked in #25 (it becomes meaningful together with sessions), consistent with the standing decisions on PRs #24/#27.

[`src/yunjin/web/routes/settings.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2784) > The new state-changing POST route does not validate a CSRF token… 🔴 Not actioned in this PR — CSRF protection spans every state-changing route and is tracked in #25 (it becomes meaningful together with sessions), consistent with the standing decisions on PRs #24/#27.
Author
Collaborator

src/yunjin/web/routes/settings.py

Settings are read and written using a hard-coded user_id=1…

🔴 Not actioned in this PR — the user_id = 1 Phase 6 pattern is project-wide across every route (session-derived identity arrives with #25); this route deliberately matches its siblings.

[`src/yunjin/web/routes/settings.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2785) > Settings are read and written using a hard-coded user_id=1… 🔴 Not actioned in this PR — the `user_id = 1` Phase 6 pattern is project-wide across every route (session-derived identity arrives with #25); this route deliberately matches its siblings.
Author
Collaborator

src/yunjin/web/static/shortcuts.js

New non-trivial source file… is added with no automated tests…

🔴 Acknowledged limitation — the project has no JavaScript test infrastructure, so this PR ships a manual verification checklist (posted above) covering every shortcut. Introducing a JS runner (node/vitest) is a project-level tooling decision to be made separately; the server-rendered HTML contract the JS depends on is under automated test.

[`src/yunjin/web/static/shortcuts.js`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2787) > New non-trivial source file… is added with no automated tests… 🔴 Acknowledged limitation — the project has no JavaScript test infrastructure, so this PR ships a manual verification checklist (posted above) covering every shortcut. Introducing a JS runner (node/vitest) is a project-level tooling decision to be made separately; the server-rendered HTML contract the JS depends on *is* under automated test.
forgejo-actions left a comment

WuMing

Found 2 issue(s). See inline comments below.

## WuMing Found **2** issue(s). See inline comments below.
@ -47,11 +47,13 @@ def manage_tags() -> str:
db = get_db()
all_tags = tags_db.get_all_tags(db)
ignored_tags = tags_db.get_ignored_tags(db)
mark_read_on_view = settings_db.get_setting(db, user_id=1, key="mark_read_on_view") == "true"

security [MEDIUM]

A01 Broken Access Control: the settings page reads mark_read_on_view for hard-coded user_id=1 instead of the current authenticated user. Any user will see/use user 1's settings once multi-user is added. Use the session user ID.

**security** [MEDIUM] A01 Broken Access Control: the settings page reads mark_read_on_view for hard-coded user_id=1 instead of the current authenticated user. Any user will see/use user 1's settings once multi-user is added. Use the session user ID. <!-- wuming:sha256:e5ac0dbec6c8ec943bdb4130b99182f862bd1d79f1324ec62892ca24531877a7 -->
@ -0,0 +1,257 @@
/**

tests [LOW]

New ~257-line source file with non-trivial logic (key dispatch, read-state toggling via /article//read and /unread, URL allow-listing, DOM focus management) has no automated tests. The added TestKeyboardShortcutContracts tests only assert the server-rendered HTML data attributes; nothing verifies that the routes the script fetches actually exist/behave as the script expects, nor the guard behaviour (text-entry/modifier keys, unsafe-URL rejection) on the JS side. Consider adding at least route-level tests for the endpoints the shortcuts depend on, or make the manual-only coverage explicit as a documented, tracked gap.

**tests** [LOW] New ~257-line source file with non-trivial logic (key dispatch, read-state toggling via /article/<id>/read and /unread, URL allow-listing, DOM focus management) has no automated tests. The added TestKeyboardShortcutContracts tests only assert the server-rendered HTML data attributes; nothing verifies that the routes the script fetches actually exist/behave as the script expects, nor the guard behaviour (text-entry/modifier keys, unsafe-URL rejection) on the JS side. Consider adding at least route-level tests for the endpoints the shortcuts depend on, or make the manual-only coverage explicit as a documented, tracked gap. <!-- wuming:sha256:6546be568fd8de55a1ced210a46213cafc8e8971236b911e53fc562beb9e2061 -->
Author
Collaborator

⏸️ Standing positions

user_id=1 · no JS test infrastructure

Both re-raise points already answered above: session-derived identity lands with #25 (Phase 6) and applies project-wide; the JavaScript testing gap is a documented project-level tooling decision with a manual verification checklist covering every shortcut. No further action in this PR.

### ⏸️ Standing positions [`user_id=1`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2799) · [`no JS test infrastructure`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2800) Both re-raise points already answered above: session-derived identity lands with #25 (Phase 6) and applies project-wide; the JavaScript testing gap is a documented project-level tooling decision with a manual verification checklist covering every shortcut. No further action in this PR.
Extend reader shortcuts to groups with unread toggle
All checks were successful
/ gitleaks (pull_request) Successful in 16s
/ checks (pull_request) Successful in 1m19s
/ pr-review (pull_request) Successful in 3m38s
deff23c804
Author
Collaborator

Group-level shortcuts + reading checkbox layout (review feedback)

Shortcuts now work on groups (the reader index is the primary surface):

  • Aggregate cards carry data-aggregate-id + group read state (unread_count == 0)
  • r on a focused group toggles it: anything unread → all articles marked read; all read → all marked unread (new POST /aggregate/<id>/unread route); the page reloads so counts, badges, and hide-read filtering re-render server-side
  • h jumps to the next unread group on the index (and still works per-article inside a group)
  • j/k/o/enter already worked on groups; aggregate pages keep the article-level j/k/r/space
  • Help overlay text updated to the group-first phrasing

Reading settings layout: the checkbox now sits to the left of the title + explanation text (flex row), no longer stacked on top.

Tests: mark-aggregate-unread route (+404), index cards expose group state; all 229 tests and 9 nox sessions green.

## Group-level shortcuts + reading checkbox layout (review feedback) **Shortcuts now work on groups** (the reader index is the primary surface): - Aggregate cards carry `data-aggregate-id` + group read state (`unread_count == 0`) - `r` on a focused group toggles it: anything unread → all articles marked read; all read → all marked unread (new `POST /aggregate/<id>/unread` route); the page reloads so counts, badges, and hide-read filtering re-render server-side - `h` jumps to the next unread **group** on the index (and still works per-article inside a group) - `j`/`k`/`o`/`enter` already worked on groups; aggregate pages keep the article-level `j`/`k`/`r`/`space` - Help overlay text updated to the group-first phrasing **Reading settings layout**: the checkbox now sits to the left of the title + explanation text (flex row), no longer stacked on top. Tests: mark-aggregate-unread route (+404), index cards expose group state; all 229 tests and 9 nox sessions green.
forgejo-actions left a comment

WuMing

Found 5 issue(s). See inline comments below.

## WuMing Found **5** issue(s). See inline comments below.
@ -160,0 +187,4 @@
if settings_db.get_setting(db, user_id=user_id, key="mark_read_on_view") == "true":
for article_id in article_ids:
if not users.is_article_read(db, user_id, article_id):
users.mark_article_read(db, user_id, article_id)

security [MEDIUM]

A01: When mark_read_on_view is enabled, a plain GET to /aggregate/ modifies read state. This state-changing GET can be triggered cross-site via image/link/prefetch and lacks CSRF protection. Use POST or require an explicit user action with CSRF protection.

**security** [MEDIUM] A01: When mark_read_on_view is enabled, a plain GET to /aggregate/<id> modifies read state. This state-changing GET can be triggered cross-site via image/link/prefetch and lacks CSRF protection. Use POST or require an explicit user action with CSRF protection. <!-- wuming:sha256:9a21e96d10bf2199c00f2c34db541b9402a3964c6a701a67ea814f9d9eceac6b -->
@ -241,6 +272,33 @@ def mark_aggregate_read(aggregate_id: int) -> ResponseReturnValue:
return redirect(url_for("reader.view_aggregate", aggregate_id=aggregate_id))

code [HIGH]

When mark_read_on_view is enabled, this redirect causes the unread action to be immediately undone: view_aggregate sees the setting and marks every article in the aggregate read again. The r group shortcut and this route therefore become no-ops for unread whenever that setting is on. Redirect to a target that suppresses mark-on-view after unmarking (e.g. a query flag), or otherwise ensure view_aggregate does not re-mark the group just after unreading it, and add a regression test.

**code** [HIGH] When `mark_read_on_view` is enabled, this redirect causes the unread action to be immediately undone: `view_aggregate` sees the setting and marks every article in the aggregate read again. The `r` group shortcut and this route therefore become no-ops for unread whenever that setting is on. Redirect to a target that suppresses mark-on-view after unmarking (e.g. a query flag), or otherwise ensure `view_aggregate` does not re-mark the group just after unreading it, and add a regression test. <!-- wuming:sha256:d75426ed47c5b7ab60a7415d30af6351e5de2556fd59a1030f4ebae23934697e -->
@ -244,0 +287,4 @@
"""
db = get_db()
user_id = 1

security [HIGH]

A01: The new unauthenticated route hardcodes user_id=1 and only checks that the aggregate exists, without ownership or session authorization. Any caller can mark all articles in any aggregate unread for the default user. Derive the user from the session and verify aggregate ownership.

**security** [HIGH] A01: The new unauthenticated route hardcodes user_id=1 and only checks that the aggregate exists, without ownership or session authorization. Any caller can mark all articles in any aggregate unread for the default user. Derive the user from the session and verify aggregate ownership. <!-- wuming:sha256:2af90be480eace876bb433d53b488d284bb544badaa64d96d9a27f56b4033829 -->
@ -0,0 +1,283 @@
/**

tests [LOW]

New non-trivial client-side logic (~280 lines of keyboard handling, read-state toggling, URL-safety checks, and help overlay) ships with no automated tests. The added tests only assert the server-rendered HTML contract and that the asset is served; the actual key handling, isSafeUrl guard, and toggle/fetch branches are never exercised. The PR notes there is no JS test infrastructure, but at minimum the pure helpers (isSafeUrl, the move/nextUnread index logic) could be extracted and unit-tested (e.g. with a lightweight runner) rather than relying solely on a manual checklist.

**tests** [LOW] New non-trivial client-side logic (~280 lines of keyboard handling, read-state toggling, URL-safety checks, and help overlay) ships with no automated tests. The added tests only assert the server-rendered HTML contract and that the asset is served; the actual key handling, `isSafeUrl` guard, and toggle/fetch branches are never exercised. The PR notes there is no JS test infrastructure, but at minimum the pure helpers (`isSafeUrl`, the move/nextUnread index logic) could be extracted and unit-tested (e.g. with a lightweight runner) rather than relying solely on a manual checklist. <!-- wuming:sha256:343303fe3811f352ff4b085bcc6aa1895da7c901009da9b567ee4e72d8b48e8d -->
@ -0,0 +222,4 @@
if (event.defaultPrevented || event.ctrlKey || event.metaKey || event.altKey) {
return;
}
if (isTextEntryTarget(event.target)) {

code [MEDIUM]

The keydown handler only ignores inputs, textareas, and selects. If a link or button has focus, pressing Enter or Space is intercepted by the shortcuts (openCurrent/openOriginal), preventDefault() runs, and the focused control cannot be activated via keyboard. Add a guard for interactive elements such as a, button, summary, and [role="button"] (e.g. target.closest("a, button, summary, [role='button']")).

**code** [MEDIUM] The keydown handler only ignores inputs, textareas, and selects. If a link or button has focus, pressing Enter or Space is intercepted by the shortcuts (openCurrent/openOriginal), `preventDefault()` runs, and the focused control cannot be activated via keyboard. Add a guard for interactive elements such as `a`, `button`, `summary`, and `[role="button"]` (e.g. `target.closest("a, button, summary, [role='button']")`). <!-- wuming:sha256:8466eca6573e7180436f524611c3d83c38374c99aaccf2591a88aa8da83a1f8c -->
🐛 Unread route redirects to index; Enter/Space activate focused controls
All checks were successful
/ gitleaks (pull_request) Successful in 22s
/ checks (pull_request) Successful in 1m21s
/ pr-review (pull_request) Successful in 4m23s
038cd18d2b
Author
Collaborator

src/yunjin/web/routes/reader.py

When mark_read_on_view is enabled, this redirect causes the unread action to be immediately undone…

Fixed — exactly right, and a sharp catch. mark_aggregate_unread now redirects to the reader index (preserving the hide_read filter) instead of the aggregate view, so the freshly-unread group is not immediately re-marked read; under hide-read it reappears, which is the desired workflow. Regression test posts the unread action with mark-read-on-view enabled, follows the redirect, and asserts the articles remain unread.

[`src/yunjin/web/routes/reader.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2806) > When mark_read_on_view is enabled, this redirect causes the unread action to be immediately undone… ✅ Fixed — exactly right, and a sharp catch. `mark_aggregate_unread` now redirects to the **reader index** (preserving the `hide_read` filter) instead of the aggregate view, so the freshly-unread group is not immediately re-marked read; under hide-read it reappears, which is the desired workflow. Regression test posts the unread action with mark-read-on-view enabled, follows the redirect, and asserts the articles remain unread.
Author
Collaborator

src/yunjin/web/static/shortcuts.js

If a link or button has focus, pressing Enter or Space is intercepted by the shortcuts…

Fixed — space and o/enter are no longer intercepted while a link/button (or [role=button]) has focus; the browser's native activation runs instead. Movement and read-toggle keys still work.

[`src/yunjin/web/static/shortcuts.js`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2807) > If a link or button has focus, pressing Enter or Space is intercepted by the shortcuts… ✅ Fixed — `space` and `o`/`enter` are no longer intercepted while a link/button (or `[role=button]`) has focus; the browser's native activation runs instead. Movement and read-toggle keys still work.
Author
Collaborator

src/yunjin/web/routes/reader.py

A01: When mark_read_on_view is enabled, a plain GET to /aggregate/ modifies read state…

⏸️ Standing — as answered for the earlier GET-side-effect comment: the state change on view is the feature's defining semantic ("viewed = read", as in FreshRSS); a POST-to-view model would break it. Low harm, reversible, and the cross-site trigger vector closes with #25 sessions + SameSite cookies.

[`src/yunjin/web/routes/reader.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2808) > A01: When mark_read_on_view is enabled, a plain GET to /aggregate/<id> modifies read state… ⏸️ Standing — as answered for the earlier GET-side-effect comment: the state change on view is the feature's defining semantic ("viewed = read", as in FreshRSS); a POST-to-view model would break it. Low harm, reversible, and the cross-site trigger vector closes with #25 sessions + SameSite cookies.
Author
Collaborator

src/yunjin/web/routes/reader.py

A01: The new unauthenticated route hardcodes user_id=1…

⏸️ Standing — the user_id = 1 Phase 6 pattern is project-wide across every route (tracked in #25); this route deliberately matches its siblings.

[`src/yunjin/web/routes/reader.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2809) > A01: The new unauthenticated route hardcodes user_id=1… ⏸️ Standing — the `user_id = 1` Phase 6 pattern is project-wide across every route (tracked in #25); this route deliberately matches its siblings.
Author
Collaborator

src/yunjin/web/static/shortcuts.js

New non-trivial client-side logic… ships with no automated tests…

⏸️ Standing — acknowledged limitation; manual verification checklist covers every shortcut and the server-rendered HTML contract is under automated test. A JS runner remains a separate tooling decision.

[`src/yunjin/web/static/shortcuts.js`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2810) > New non-trivial client-side logic… ships with no automated tests… ⏸️ Standing — acknowledged limitation; manual verification checklist covers every shortcut and the server-rendered HTML contract is under automated test. A JS runner remains a separate tooling decision.
forgejo-actions left a comment

WuMing

Found 2 issue(s). See inline comments below.

## WuMing Found **2** issue(s). See inline comments below.
@ -244,0 +287,4 @@
"""
db = get_db()
user_id = 1

security [HIGH]

A01: The new route hard-codes the user identity to 1 and performs no authentication or authorization check. Any caller can mark all articles in any aggregate unread for that user. Use the authenticated session's user ID and authorize that the aggregate belongs to that user.

**security** [HIGH] A01: The new route hard-codes the user identity to 1 and performs no authentication or authorization check. Any caller can mark all articles in any aggregate unread for that user. Use the authenticated session's user ID and authorize that the aggregate belongs to that user. <!-- wuming:sha256:e98743b66f6285397e51e792c9d42bb4996602d805d8f6437f46fb6586566d59 -->
@ -0,0 +1,291 @@
/**

tests [LOW]

This is a substantial new source file (~291 lines) with non-trivial logic (focus management, read-state toggling via fetch, group read/unread toggling, safe-URL checks, help overlay, key routing). The tests added in this diff only assert the server-rendered data-attribute contract (data-shortcut-item, data-article-id, data-read, etc.) and that the static asset is served; none exercise the JS behavior itself. The PR notes there is no JS test infrastructure and relies on a manual checklist, but a unit test harness (e.g., jsdom + a small test for move/nextUnread/toggleRead/isSafeUrl handlers) would lock in the key handling and the http(s)-only URL guard. Consider adding minimal automated coverage or tracking the manual checklist as a follow-up.

**tests** [LOW] This is a substantial new source file (~291 lines) with non-trivial logic (focus management, read-state toggling via fetch, group read/unread toggling, safe-URL checks, help overlay, key routing). The tests added in this diff only assert the server-rendered data-attribute contract (data-shortcut-item, data-article-id, data-read, etc.) and that the static asset is served; none exercise the JS behavior itself. The PR notes there is no JS test infrastructure and relies on a manual checklist, but a unit test harness (e.g., jsdom + a small test for move/nextUnread/toggleRead/isSafeUrl handlers) would lock in the key handling and the http(s)-only URL guard. Consider adding minimal automated coverage or tracking the manual checklist as a follow-up. <!-- wuming:sha256:692054f80290ff0172ef04f88aab42e3c46632e8d9aca1bf64c16742f9401ba0 -->
Author
Collaborator

⏸️ Standing positions (both re-raises, no new substance)

user_id=1 / no auth — tracked in #25 (Phase 6), project-wide pattern.
no automated JS tests — acknowledged limitation; manual checklist covers every shortcut; runner is a separate tooling decision.

Not re-actioning in this PR.

### ⏸️ Standing positions (both re-raises, no new substance) [`user_id=1 / no auth`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2820) — tracked in #25 (Phase 6), project-wide pattern. [`no automated JS tests`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2821) — acknowledged limitation; manual checklist covers every shortcut; runner is a separate tooling decision. Not re-actioning in this PR.
marvin8 approved these changes 2026-09-12 05:43:03 +00:00
Dismissed
Add g shortcut to open next unread group
All checks were successful
/ gitleaks (pull_request) Successful in 18s
/ checks (pull_request) Successful in 1m28s
/ pr-review (pull_request) Successful in 4m30s
3050d5391f
agent-pi dismissed marvin8's review 2026-09-12 06:05:52 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

Author
Collaborator

g shortcut: open next unread group (review discussion)

Per Marvin8's request, g (single key, "next Group") opens the next unread group directly — no detour through the reader index, and no capital-letter two-key chord.

  • Server-computed: the aggregate view resolves the next aggregate with unread articles (reader-index ordering: most unread first, then newest), excluding the current group, and exposes it as data-next-unread-url plus a visible "Next unread group →" button next to the other controls — works without JavaScript, clickable without keyboard
  • g navigates there; when no unread groups remain, a small toast says "No unread groups" instead of a silent no-op
  • The current group is always skipped, even if it still has unread articles; hide-read state is preserved through the jump

Final key map: h = next unread article (within the open group) · g = open next unread group · r = toggle read (article, or whole group on the index) · j/k/n/p = move · space = original site · o/enter = open item · c = summary · 1/2/3 = pages · f1/? = help · escape = close

New db query (get_next_aggregate_with_unread) with ordering/coverage tests; route contract tests for the link and its absence when everything is read.

## `g` shortcut: open next unread group (review discussion) Per Marvin8's request, `g` (single key, "next **G**roup") opens the next unread group directly — no detour through the reader index, and no capital-letter two-key chord. - Server-computed: the aggregate view resolves the next aggregate with unread articles (reader-index ordering: most unread first, then newest), **excluding the current group**, and exposes it as `data-next-unread-url` plus a visible **"Next unread group →"** button next to the other controls — works without JavaScript, clickable without keyboard - `g` navigates there; when no unread groups remain, a small toast says "No unread groups" instead of a silent no-op - The current group is always skipped, even if it still has unread articles; hide-read state is preserved through the jump Final key map: `h` = next unread article (within the open group) · `g` = open next unread group · `r` = toggle read (article, or whole group on the index) · `j`/`k`/`n`/`p` = move · `space` = original site · `o`/`enter` = open item · `c` = summary · `1`/`2`/`3` = pages · `f1`/`?` = help · `escape` = close New db query (`get_next_aggregate_with_unread`) with ordering/coverage tests; route contract tests for the link and its absence when everything is read.
forgejo-actions left a comment

WuMing

Found 4 issue(s). See inline comments below.

## WuMing Found **4** issue(s). See inline comments below.
@ -160,0 +184,4 @@
# renders with the read states captured above, so what you are reading
# does not change under you; the group simply counts as read afterwards
# (reader index, hide-read filter, next-unread navigation).
if settings_db.get_setting(db, user_id=user_id, key="mark_read_on_view") == "true":

security [MEDIUM]

A01/A04: Enabling mark_read_on_view makes a GET request mutate read state for all articles. GET state changes can be triggered cross-site via links, prefetch, or crawlers. State changes should be POST-only with CSRF protection or an explicit user action.

**security** [MEDIUM] A01/A04: Enabling mark_read_on_view makes a GET request mutate read state for all articles. GET state changes can be triggered cross-site via links, prefetch, or crawlers. State changes should be POST-only with CSRF protection or an explicit user action. <!-- wuming:sha256:bbeef3e00b559e3abbdef76d932a63df7bd383e54cbc884c067277ec0d8d7b54 -->
@ -241,6 +282,40 @@ def mark_aggregate_read(aggregate_id: int) -> ResponseReturnValue:
return redirect(url_for("reader.view_aggregate", aggregate_id=aggregate_id))
@bp.route("/aggregate/<int:aggregate_id>/unread", methods=["POST"])

security [MEDIUM]

A01: New state-changing POST route lacks visible CSRF protection. An attacker can forge a request to mark any aggregate unread if the user is authenticated. Add CSRF token validation.

**security** [MEDIUM] A01: New state-changing POST route lacks visible CSRF protection. An attacker can forge a request to mark any aggregate unread if the user is authenticated. Add CSRF token validation. <!-- wuming:sha256:3fe25dace1e3ab6417f7f8eca869cd5e6b964a1d40f22ab59881fe3169449821 -->
@ -244,0 +297,4 @@
"""
db = get_db()
user_id = 1

security [HIGH]

A01: Hard-coded user_id=1 with no authenticated session and no ownership check on aggregate_id allows any caller to modify any aggregate's read state (IDOR). Use the current authenticated user and scope aggregate lookup to that user.

**security** [HIGH] A01: Hard-coded user_id=1 with no authenticated session and no ownership check on aggregate_id allows any caller to modify any aggregate's read state (IDOR). Use the current authenticated user and scope aggregate lookup to that user. <!-- wuming:sha256:dd5768878592fbcccb6f78131008470b79c786b93dd508ee51a1bb105a97b698 -->
@ -0,0 +1,312 @@
/**

tests [LOW]

This ~313-line keyboard-shortcut script contains the bulk of the new non-trivial logic (key dispatch, modifier/typing guards, read/unread fetch toggle, in-place badge updates, next-unread navigation, help overlay) but has no automated coverage. The only tests touching it verify that the asset is served and that shortcuts.js is referenced by templates, which would still pass if every handler were broken. The PR acknowledges this (no JS test infra, manual checklist only) and the server-rendered data attributes are contract-tested, which is good, but consider adding at least a minimal DOM-level test (e.g. jsdom/node or Playwright) for the key-to-action mapping and the input/modifier guard, since these are the most regression-prone parts.

**tests** [LOW] This ~313-line keyboard-shortcut script contains the bulk of the new non-trivial logic (key dispatch, modifier/typing guards, read/unread fetch toggle, in-place badge updates, next-unread navigation, help overlay) but has no automated coverage. The only tests touching it verify that the asset is served and that `shortcuts.js` is referenced by templates, which would still pass if every handler were broken. The PR acknowledges this (no JS test infra, manual checklist only) and the server-rendered data attributes are contract-tested, which is good, but consider adding at least a minimal DOM-level test (e.g. jsdom/node or Playwright) for the key-to-action mapping and the input/modifier guard, since these are the most regression-prone parts. <!-- wuming:sha256:dde3874e48504891c53e585253978fc47bb10179cdacc5360ebdd8d2324217c4 -->
Author
Collaborator

⏸️ Standing decisions (all four re-raises, no new substance)

user_id=1 / IDOR · GET state change · CSRF · no JS tests

Each was answered in detail earlier in this PR: auth/session identity and CSRF protection are tracked in #25 (Phase 6, where they become meaningful together with sessions); the GET-side-effect is the defining semantic of mark-read-on-view (FreshRSS-standard, low harm, reversible) and its cross-site vector closes with #25 SameSite cookies; the JS test gap is a documented tooling decision with a manual checklist covering every shortcut. Not re-actioning further in this PR — the maintainer has already approved.

### ⏸️ Standing decisions (all four re-raises, no new substance) [`user_id=1 / IDOR`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2832) · [`GET state change`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2830) · [`CSRF`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2831) · [`no JS tests`](https://forge.marvin8.zone/marvin8/yunjin/pulls/28#issuecomment-2833) Each was answered in detail earlier in this PR: auth/session identity and CSRF protection are tracked in #25 (Phase 6, where they become meaningful together with sessions); the GET-side-effect is the defining semantic of mark-read-on-view (FreshRSS-standard, low harm, reversible) and its cross-site vector closes with #25 SameSite cookies; the JS test gap is a documented tooling decision with a manual checklist covering every shortcut. Not re-actioning further in this PR — the maintainer has already approved.
marvin8 approved these changes 2026-09-12 06:51:21 +00:00
marvin8 manually merged commit 857002cdef into main 2026-09-12 06:53:01 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
marvin8/yunjin!28
No description provided.