feat(config): show subscription status on the configuration screen - #7
Conversation
load() and all() each had their own copy of read -> json_decode -> normalise, and the copies had already diverged: load() logged unusable JSON while all() skipped it in silence, and only all() rejected an empty url. The silence was the wrong way round. all() exists to feed a status screen -- the one view whose purpose is showing what is broken -- so a corrupt state file vanished from it entirely, indistinguishable from a subscription that never existed, while the hot path that does not need the warning emitted one. all() cannot simply call load(): load() derives its directory from a resource URL via sha1(), which does not invert, so all() has to recover the URL from the file. That justifies two methods, not two parsers. Both now share readState(). Also constrains `kind` to the two known values in normalise(), so a caller can key off it without validating, and documents that all() yields in hash order and is single-pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Until now the only way to tell whether a feed used rssCloud, and whether its handshake succeeded, was to read data/users/_/log_rsscloud.txt or cat the JSON under data/rssCloud/resources/ inside the container. Adds a read-only table to the extension's configuration screen: one row per discovered resource, with its type, state, cloud server, last renewal attempt, last notification received, and the users interested in it. The state column distinguishes four cases that the raw file does not make obvious. lease_start is stamped *before* the request goes out, deliberately, so that a crashed call cannot spin -- which means a non-zero lease_start does not imply the handshake worked. Only `error` says that, and it defaults to true when absent. So: pending (discovered, never attempted), failed (with the message), active (registered and inside the renewal window), stale (registered but the lease is older than the window and may have lapsed). The model layer for this already existed and was simply never called. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
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 |
The repo had no test harness, so the refactor that unified load() and all() was verified out-of-tree and thrown away. This lands it. No second set of dev dependencies: the tests borrow core's PHPUnit and core's autoloader, exactly as phpstan and phpcs already do, so there is no composer.json here to drift out of step. The bootstrap requires core's constants.php and lib_rss.php directly rather than core's own tests/bootstrap.php -- the two lines are identical, and constants.php is far less likely to move. Using the real Minz_Log rather than a stub means the tests assert on the log file itself, which is what the refactor was about: unusable state must be reported, not silently skipped. Removing that one Minz_Log::warning call fails three of these tests, so they are load-bearing rather than decorative. Runs on both matrix legs, and verified against 1.29.0 as well as edge before landing. Also corrects the README, which still claimed CI ran against edge only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Why
There was no way to tell whether a feed used rssCloud, or whether its handshake worked, without
going into the container and reading
data/users/_/log_rsscloud.txtorcat-ing the JSON underdata/rssCloud/resources/.The model layer for this already existed —
RssCloud_Registry::all()yielded exactly the rightshape and had zero callers. This is a missing view over a finished model.
The state column
The raw file doesn't make the important distinction obvious.
lease_startis stamped before therequest goes out (
Subscriber.php:79-81), deliberately, so a crashed call can't spin — so anon-zero
lease_startdoes not mean the handshake succeeded. Onlyerrorsays that, and itdefaults to
truewhen absent. The table separates four cases:pleaseNotifyrejected or unreachable, with the messageColumns: resource, type (feed / dynamic OPML), state, cloud server, last renewal attempt, last
notification received, and the users interested in it.
Registry refactor (first commit)
load()andall()each had their own copy of read → decode → normalise, and they had alreadydiverged:
load()logged unusable JSON,all()skipped it silently, and onlyall()rejected anempty
url.That was backwards.
all()feeds the status screen — the one view whose job is showing what'sbroken — so a corrupt state file vanished from it entirely, indistinguishable from a subscription
that never existed, while the hot path that doesn't need the warning was the one emitting it.
all()can't just callload():load()derives its directory from a resource URL viasha1(),which doesn't invert, so
all()must recover the URL from the file contents. Two methods justified;two parsers not. Both now share
readState().Also constrains
kindto the two known values innormalise()so callers can key off it withoutvalidating, and documents that
all()yields in hash order and is single-pass.Verification
php -lover php+phtml clean · PHPStan level 10[OK] No errors· phpcs 10/10 clean · every_t()key cross-checked against
i18n/en/ext.phpin both directions (none missing, none orphaned).The repo has no test harness, so I exercised the refactored
Registryagainst fixture directoriesout-of-tree — 14 assertions covering valid load, malformed JSON, empty URL, absent file, orphan
directory, unknown
kind, andall()filtering — all passing, and confirmingall()now logs thetwo unusable entries it previously swallowed. That harness is not in this PR; there's nowhere to
put it yet. Happy to add PHPUnit and land it as a real test if wanted.
The configuration screen itself is not rendered by any of this — no bootstrap to render it against.
Not included: the feed list
I checked
Minz_HookTypeat both 1.29.0 and edge. There is no hook on the subscription or feedlist page — the only addition since 1.29.0 is
EntriesRead, andMinz_Extensionoffers noview-override API. The only route would be injecting global JS that scrapes core's markup, backed by
a new authenticated status endpoint. That's fragile across core releases and adds real surface to an
extension that already exposes an unauthenticated callback, so I haven't done it silently. Say the
word if you want it anyway.
🤖 Generated with Claude Code