chore(stats): delete the unreachable reading-stats feature - #1472
Merged
Merged
Conversation
mauricecarrier7
force-pushed
the
chore/remove-unreachable-stats
branch
from
September 15, 2026 13:32
fe64148 to
8686bf7
Compare
🏗️ CodeAtlas Ledger Analysis✅ All Checks Passed♿ Accessibility (via AccessLint)
✅ No accessibility issues detected 🏛️ Architecture Analysis
🔄 Dependency Cycles (2)2 distinct dependency cycles (largest spans 10 components).
Cycle 1 — 10 components, 18 edges between them
edgesCycle 2 — 3 components, 4 edges between them
edgesDiscounted 📋 Architecture Findings
🔍 Reachability Analysis
ℹ️ No architecture-relevant files in this diff — reachability not evaluated. 📊 0 files analyzed | 📦 Download Full Report Powered by CodeAtlas Ledger |
🧪 Unit Test Results📊 View Full Interactive Report ✅ ALL TESTS PASSED8696 tests | 8680 passed | 0 failed | 14 skipped | ⏱️ 17m 12s | 📊 99.8% | 📈 48.8% coverage All 991 classes (full matrix — click to expand)
📊 Full interactive matrix: report 📊 Testing Coverage BreakdownUnit Test Line Coverage (testable surfaces): 48.8% Total coverage incl. UI/lifecycle: 47.6% (17 files excluded from testable denominator — see
📈 What changed vs. baseTest count: +5 🔗 Interactive HTML Report | CI Run Details Counts above were produced by this CI run's xcresult parse — reproduce via the run link. 📦 Downloadable Artifacts
|
This was referenced Sep 15, 2026
Merged
The Stats feature is a prototype that was never wired in. Verified before
deleting — from outside `Palace/Stats/` there are ZERO references to it:
StatsView( constructed 0
AppTab.stats referenced 0
.stats referenced 0
in any storyboard/xib/plist 0
deep links mentioning stats 0
`case stats` was declared in `AppTabRouter` and never used, and `StatsTab`
gated itself behind a `readingStatsEnabled` remote flag that nothing else
reads. Twenty source files compiled into both targets to render a screen no
patron could open.
Removed: 20 source files, 7 Stats test files, and
`PalaceTests/UIPolish/BadgeUnlockPhaseTests.swift`, which tests
`BadgeUnlockPhase` — a type defined inside `BadgesView.swift` and therefore
deleted with it.
Two dependencies checked and cleared rather than assumed. `ReadingFormat` is
DUPLICATED: a `public` one in the `PalaceReadingPosition` package used
app-wide, and an internal copy in Stats. `ReadingPositionTests` uses the
package one and only named the Stats file in a comment, so it is untouched.
Four other test files matched a grep for "Badge" in the tab-bar, app-icon and
audiobook-format senses; also untouched.
Adds `scripts/pbxproj_remove_swift.rb`, the counterpart to the existing
`pbxproj_add_swift.rb`. A source file lives in six places across two targets
and hand-editing `project.pbxproj` is forbidden; a partial removal leaves a
dangling build file that fails with no useful message. 76 references dropped
across 28 files, groups pruned.
Found while auditing localization coverage: the feature's strings were being
translated into four languages before anyone asked whether the screens were
reachable.
**Scope:** deletion only. No behaviour change to any shipping screen.
**Not done:** no test asserts the feature stays gone — a future commit could
reintroduce an unreachable screen the same way and nothing would notice.
**Deferred:** `%@ day%@` in `StreakView` passed the English plural morpheme
`"s"` as a format argument and could not render correctly in German or Italian
at any count. It dies with this commit; if the feature is ever revived it needs
a `.stringsdict` entry rather than a literal suffix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015xqE9uAiVMUc7kKHsXT17w
mauricecarrier7
force-pushed
the
chore/remove-unreachable-stats
branch
from
September 15, 2026 14:59
8686bf7 to
bc9cbc8
Compare
This was referenced Sep 15, 2026
mauricecarrier7
pushed a commit
that referenced
this pull request
Sep 15, 2026
One flaky test re-ran the entire suite. `-retry-tests-on-failure` is meant to
re-run what FAILED; `-test-repetition-relaunch-enabled YES` silently converts it
into a repetition of the whole plan.
MEASURED, four configurations against one prebuilt bundle, a plan of one
always-failing and three always-passing tests:
flags failing passing
retry + iterations 3 + relaunch YES (was CI) 3x 3x
retry + relaunch YES 3x 3x
retry + iterations 3 3x 1x
retry 3x 1x
The retry CAP is 3 in all four; only the passing-test count moves. `relaunch` is
the multiplier and `-test-iterations` is not — the opposite of what the flag
names suggest, and the opposite of my first reading of them. `-test-iterations 3`
stays: `man xcodebuild` says retry alone "assumes a maximum of 3", so it is
equivalent, it states the cap, and the CI_TEST_ITERATIONS=1 single-pass path
keys off it.
In CI this was 30 minutes against 53. PR #1472 timed out twice against a
60-minute step bound on tests that each PASSED on retry. ~230 wasted billable
minutes per affected run on a 10x-billed macOS runner.
## What this costs, stated plainly
A retry now re-enters the SAME process. A victim wedged by a leaked per-clone
global will fail its retries too, where relaunch would have rescued it. That is
real: of the two failures observed on 2026-09-15, one was a 120.000s wedge (would
have needed relaunch) and one a 0.031s assertion flake (would not).
Taken deliberately. `ISOLATED_SERIAL_TESTS` already quarantines the known
wedge-prone classes out of the oversubscribed parallel leg, and the per-test
fixes (DEFLAKE-PLAN P1/P2) exist to remove the wedge source. Paying in occasional
red on a genuine wedge beats paying 23-30 minutes on EVERY run that stumbles
once.
## Guard
Four assertions over the committed flag list, comments excluded so the
measurement can stay in the file it describes. Proved by reintroducing the flag:
2 of 4 go red, and green again on restore. They also assert the retry safety net
and the explicit cap are still there, so the guard cannot be satisfied by
deleting retry altogether.
**Scope:** one xcodebuild flag and the test that keeps it gone.
**Not done:** the wedge fixes themselves. This makes their absence visible
instead of paying to hide it.
**Deferred:** `-default-test-execution-time-allowance 120` still lets one wedged
test burn two minutes. That is now the dominant per-flake cost rather than a
rounding error next to a full re-run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015xqE9uAiVMUc7kKHsXT17w
mauricecarrier7
added a commit
that referenced
this pull request
Sep 15, 2026
…ound while we watch (#1482) * fix(ci): the test step's 60-min bound could not fit its own retry policy PR #1472 went red twice on `The action 'Run Palace unit tests' has timed out after 60 minutes`. Both times `** TEST SUCCEEDED **`, no timeout or restart lines, and every test that failed an iteration passed on retry. Different tests each time, none related to the branch — which deletes test files and should be the FASTEST branch in flight. The tests were fine; the clock was not. MEASURED across three runs of the same suite on 2026-09-15: depth executions step branch 1x 9,200 30 min fix/reconciliation-article-false-positive 3x 26,118 53 min feat/pp-5094-localization-inventory 2x 17,594 60 min chore/remove-unreachable-stats (timed out) A clean pass is 30 minutes. `-test-iterations 3` repeats the WHOLE plan when any single test fails an iteration, adding 23-30 minutes. So the workflow enabled a retry policy costing up to 3x and budgeted for 1x: 30 fits, 53 barely fits, 60 does not. Every branch is one flake away from this, and the failure reads as a test problem rather than a budget problem, which is how it cost two hours of rerunning before anyone measured the step. Step 60 -> 75; job 90 -> 105 to keep the ~15 minutes of post-processing the job cap was sized for. ## This is a bound, not the fix ~2/3 of the wall-clock in any run that flakes once is re-running tests that already passed. `-retry-tests-on-failure` is supposed to scope a retry to the failing tests; measured, it does not — 8,510 of 9,097 tests ran three times because a handful failed once. Making that scope correctly would put every run near 30 minutes regardless of flakes and put this ceiling out of reach. Tracked separately; the comment in the workflow says explicitly not to raise this number again in its place. **Scope:** two timeout values and the measurements that justify them. **Not done:** the retry-scoping fix. It needs a controlled experiment against `-test-repetition-relaunch-enabled` to establish which flag turns a per-test retry into a whole-plan repetition, and that experiment is not this PR. **Deferred:** `-default-test-execution-time-allowance 120` means one wedged test burns two minutes per iteration. At 3x depth a single wedge costs six. That interacts with the budget but is a different lever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xqE9uAiVMUc7kKHsXT17w * perf(ci): retry the tests that failed, not all 9,100 One flaky test re-ran the entire suite. `-retry-tests-on-failure` is meant to re-run what FAILED; `-test-repetition-relaunch-enabled YES` silently converts it into a repetition of the whole plan. MEASURED, four configurations against one prebuilt bundle, a plan of one always-failing and three always-passing tests: flags failing passing retry + iterations 3 + relaunch YES (was CI) 3x 3x retry + relaunch YES 3x 3x retry + iterations 3 3x 1x retry 3x 1x The retry CAP is 3 in all four; only the passing-test count moves. `relaunch` is the multiplier and `-test-iterations` is not — the opposite of what the flag names suggest, and the opposite of my first reading of them. `-test-iterations 3` stays: `man xcodebuild` says retry alone "assumes a maximum of 3", so it is equivalent, it states the cap, and the CI_TEST_ITERATIONS=1 single-pass path keys off it. In CI this was 30 minutes against 53. PR #1472 timed out twice against a 60-minute step bound on tests that each PASSED on retry. ~230 wasted billable minutes per affected run on a 10x-billed macOS runner. ## What this costs, stated plainly A retry now re-enters the SAME process. A victim wedged by a leaked per-clone global will fail its retries too, where relaunch would have rescued it. That is real: of the two failures observed on 2026-09-15, one was a 120.000s wedge (would have needed relaunch) and one a 0.031s assertion flake (would not). Taken deliberately. `ISOLATED_SERIAL_TESTS` already quarantines the known wedge-prone classes out of the oversubscribed parallel leg, and the per-test fixes (DEFLAKE-PLAN P1/P2) exist to remove the wedge source. Paying in occasional red on a genuine wedge beats paying 23-30 minutes on EVERY run that stumbles once. ## Guard Four assertions over the committed flag list, comments excluded so the measurement can stay in the file it describes. Proved by reintroducing the flag: 2 of 4 go red, and green again on restore. They also assert the retry safety net and the explicit cap are still there, so the guard cannot be satisfied by deleting retry altogether. **Scope:** one xcodebuild flag and the test that keeps it gone. **Not done:** the wedge fixes themselves. This makes their absence visible instead of paying to hide it. **Deferred:** `-default-test-execution-time-allowance 120` still lets one wedged test burn two minutes. That is now the dominant per-flake cost rather than a rounding error next to a full re-run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xqE9uAiVMUc7kKHsXT17w --------- Co-authored-by: t <t@t.io> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Deletes the reading-stats feature — 20 source files, 7 test files, and one more test that depends on a type defined inside a deleted view.
It is a prototype that was never wired in. From outside
Palace/Stats/there are zero references to it:StatsView(constructedAppTab.statsreferenced.statsreferencedcase statswas declared inAppTabRouterand never used, andStatsTabgated itself behind areadingStatsEnabledremote flag nothing else reads. Twenty files compiled into both targets to render a screen no patron could open.Why now
Found while auditing localization coverage for PP-5094: these screens were being translated into four languages before anyone asked whether they were reachable.
What was checked, not assumed
ReadingFormatis duplicated — apublicone in thePalaceReadingPositionpackage used app-wide, and an internal copy in Stats.ReadingPositionTestsuses the package one and only named the Stats file in a comment. Untouched.PalaceTests/UIPolish/BadgeUnlockPhaseTests.swiftis deleted — it testsBadgeUnlockPhase, defined insideBadgesView.swift.Tooling
Adds
scripts/pbxproj_remove_swift.rb, the counterpart to the existingpbxproj_add_swift.rb. A source file lives in six places across two targets and hand-editingproject.pbxprojis forbidden; a partial removal leaves a dangling build file that fails with no useful message. 76 references dropped across 28 files, empty groups pruned.Verification
** TEST BUILD SUCCEEDED **on thePalace(DRM) scheme, 0 compile errors — built on this branch alone, offdevelop, not alongside the localization work.Not done
No test asserts the feature stays gone; a future commit could reintroduce an unreachable screen the same way and nothing would notice.
Deferred
%@ day%@inStreakViewpassed the English plural morpheme"s"as a format argument and could not render correctly in German or Italian at any count. It dies with this commit — if the feature is ever revived it needs a.stringsdictentry rather than a literal suffix.🤖 Generated with Claude Code
https://claude.ai/code/session_015xqE9uAiVMUc7kKHsXT17w