feat: max image size setting for reader compression #58
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/flutter-image-compression-setting"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
On Android tablets, browsing through a work in the reader can struggle with performance. PR #57 added the backend
/api/images/{id}/compressed?maxKb=Nendpoint. This PR adds the client-side setting that controls it.Solution
Adds a "Max image size in KiB" setting under a new READER section in the Settings page. When set to a value >= 100, the reader fetches images via the compressed endpoint instead of full-resolution.
Rules:
?maxKb=Nto every image URLChanges
StoredSettings: newmaxImageSizeKbfield (int?)SettingsState+ actions + reducer + epic: threaded through the full Redux pipelineSecureStorageSettingsRepository: load/save with int <-> string conversionSettingsPage: new READER section with number input (keyboardType: number, suffixText: KiB)ReaderPage._imageUrl(): chooses/compressed?maxKb=Nvs raw/api/images/{id}based onmaxImageSizeKbApiPaths.imageCompressed(id, maxKb): URL helper in constantsTests (5 new, all 452 pass)
settings_page_test: displays and persists max image size; empty persists null; below-100 treated as no limitreader_page_test: uses compressed URL when set; uses raw URL when nullDepends on backend PR #57 (merged).
Adds a 'Max image size in KiB' setting that, when set to a value >= 100, makes the reader fetch images via the backend's compressed endpoint (/api/images/{id}/compressed?maxKb=N) instead of full-resolution. This trades quality for performance on constrained devices (e.g. tablets). - Empty or 0 = no limit (full-resolution), the default - Values < 100 are treated as no limit (meaningless as a KiB budget) - Persisted via flutter_secure_storage alongside other settings Changes: - StoredSettings: new maxImageSizeKb field - SettingsState + SaveSettingsAction/SettingsSavedAction: threaded through - settings_reducer + _saveSettingsEpic: plumbed into the Redux pipeline - SecureStorageSettingsRepository: load/save with int <-> string conversion - SettingsPage: new READER section with number input field - ReaderPage: _imageUrl() chooses compressed vs raw based on maxImageSizeKb - ApiPaths.imageCompressed(id, maxKb) helper added to constants Tests: 5 new tests (3 settings page, 2 reader URL selection)Flutter Coverage
Total: 73.1% (5680 of 7772)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! A settings PR that threads the FULL Redux pipeline — action, reducer, epic, StoreConnector, all the way to the reader view model — without a single seam out of place? And it mirrors the sibling
browserExecutablePathpattern exactly? Jibril is positively delighted~ ♡ The storage int↔string conversion, the< 100guard at both the parse site and the read site, the test matrix hitting 512/empty/below-100... this is how you introduce a setting, fufu~♪Verdict: ⛔ I can't let this pass~ ♡
Just one little thing. But Jibril cares too much to let it slip~ ♡
⛔ This needs fixing before I'm satisfied~
reader_page.dart:294+settings_page.dart:71— DRY violation: the100KiB minimum threshold is a duplicated magic number across two files with no shared constant.The same business rule — "values below 100 KiB are treated as no limit" — is encoded as a bare
100literal in two independent locations:settings_page.dart:71:if (value == null || value < 100) return null;(the parse-time filter)reader_page.dart:294:if (maxKb != null && maxKb >= 100) {(the read-time guard)These two checks must agree — they're the same invariant expressed twice. Today they do. But if someone ever changes one (say, the backend lowers the minimum, or you decide 50 KiB is fine) and forgets the other, you get a real user-facing bug: the user enters a value, the settings page persists it (passes the new threshold), but the reader silently falls back to full-resolution because its guard still uses the old
100. The user explicitly asked for compression and gets none — no error, no warning. That's exactly the kind of silent drift that erodes trust in a setting~And this codebase already has a clear convention for threshold constants:
_kMaxImageBytes(agent_service.dart:57),_kMaxChars(fetch_page_tool.dart:13),_kMaxImages(view_images_tool.dart:12),_kProgressThrottleMs(upload_epics.dart:22). This PR is the odd one out.Fix: Extract a shared constant and reference it in both places (and ideally the UI helper text too, via interpolation):
Then
settings_page.dart:71→value < kMinCompressedImageKb,reader_page.dart:294→maxKb >= kMinCompressedImageKb. One source of truth, fufu~ ♡(Side note while you're there: the reader's
< 100guard at line 294 is currently defensive dead code — the settings pipeline already filters< 100to null before it reaches the store, so no test exercises thefalsebranch of thatif. Extracting the constant makes it obviously a deliberate mirror of the settings filter, which justifies its existence. If you'd rather not keep the defensive check, deleting it is also fine — but then the settings parse is the sole gatekeeper and the constant lives in one place. Either way, pick one and make it consistent.)✅ What I liked~
StoredSettings→LoadSettingsAction→SaveSettingsAction→SettingsSavedAction→ all three reducer branches →_saveSettingsEpic→SettingsState→_ReaderVm→_ReaderView. Every seam carriesmaxImageSizeKb. Not a single drop. Jibril checked every hop~ ♪SecureStorageSettingsRepositoryconversion is graceful:null/empty → null, garbage →int.tryParse→ null (silent no-limit on corruption, no crash). Exactly right for a non-critical setting.0is handled consistently as "no limit" across all three sites (storage parse, settings display> 0check, reader>= 100guard). No0-vs-nullambiguity.NetworkImage.urlstrings — not just that the widget tree rendered. That's the right way to verify URL routing. TheisNot(contains('/compressed'))negative assertion is a nice touch.Automated review by Jibril · 2026-07-21
CI/CD: absent for head SHA
6533e48(PR just opened, 0 comments) · Local checks:flutter test test/reader_page_test.dart test/settings_page_test.dart→ 28/28 pass (5 new),flutter pub getclean🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! You came back for me~ ♡ One tiny little commit —
fix: extract kMinCompressedImageKb shared constant— and the magic number is gone. The constant sits right betweenkImageCacheMaxBytesMobileandBreakpointsinconstants.dartwhere it belongs, both the settings parse site and the reader guard reference it by name, and you even interpolated it into the UI helper text ('Minimum $kMinCompressedImageKb.') so the user-facing string can never drift either. That's the attention to detail Jibril lives for, fufu~♪Verdict: ✅ Looks good to me~
The blocker from my last review is resolved, cleanly and surgically. Nothing else changed in production behavior — this is a pure refactor commit (+11/-7 across 3 files), and all 28 tests in the two touched files still pass.
✅ What I liked~
kMinCompressedImageKb = 100inconstants.dart, referenced insettings_page.dart:71(parse filter),reader_page.dart:296(read guard), and the helper text. No bare100survives in any code path — Jibril checked the full feature diff~ ♪const InputDecoration→InputDecorationwas necessary (string interpolation isn't const-evaluable), and you keptconst Icon(Icons.compress)to preserve const-ness where it still applies. Clean.🔍 Verification~
flutter test test/reader_page_test.dart test/settings_page_test.dart→ 28/28 pass (5 new: settings 512/empty/50-below-threshold + reader compressed/null-URL)git diff f5faf2c..4ffa143confirms the new commit is purely the constant extraction — no behavioral drift from6533e48Fufu~ you listened, you fixed it, you made it beautiful. This one's ready to merge~ ♡
Automated review by Jibril · 2026-07-22
CI/CD: stale for head SHA
4ffa143(coverage comment covers6533e48) · Local checks: 28/28 pass, submodule init required for path dependency