feat: Force-save for chaotic screen changes#36
Conversation
📝 WalkthroughWalkthroughThis change introduces a force-save timeout mechanism for screenshot captures. The system tracks elapsed time since the last save and forces a screenshot if the interval exceeds a configured threshold (default 40 seconds). Implementation spans backend configuration, capture service logic, API schema updates, and frontend settings UI. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend as Frontend Settings
participant API as Backend API
participant Config as Config Service
participant Capture as Capture Service
User->>Frontend: Adjust "Chaotic Save Timeout" slider
Frontend->>API: updateConfig({capture_max_time_without_save: value})
API->>Config: Update CaptureSettings.max_time_without_save
Config->>Config: Persist to file, update change log
API-->>Frontend: Configuration updated
Note over Capture: During normal screenshot capture loop
Capture->>Capture: Check elapsed time since _last_save_time
alt Time exceeds max_time_without_save (and value > 0)
Capture->>Capture: Force-save screenshot (even if unstable)
Capture->>Capture: Reset _last_save_time
else Screen becomes stable
Capture->>Capture: Save screenshot normally
Capture->>Capture: Reset _last_save_time
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/tests/test_api.py (1)
99-133:⚠️ Potential issue | 🟠 MajorAdd missing mock for
max_time_without_saveintest_get_config.The GET
/configendpoint now readsconfig.capture.max_time_without_save(line 103 ofapi/routes/status.py), but the test mock doesn't set this requiredfloatfield. Pydantic validation will fail when it tries to coerce theMagicMockobject to a float, causing the test to break.Fix: add the missing mock attribute
mock_config.capture.quality = 95 + mock_config.capture.max_time_without_save = 40.0 mock_config.compression.enabled = Falsecore/compression.py (1)
133-169:⚠️ Potential issue | 🟠 MajorForce recompress overwrites
original_size_byteswith the already-compressed file size.When
force=Trueand the image is already compressed, Line 152 captures the current (compressed) file size asoriginal_size, and Line 167 writes it to the DB viadb.mark_compressed(screenshot["id"], original_size). This permanently loses the true original size, making storage savings stats inaccurate after force recompression.Consider preserving the existing
original_size_bytesif the screenshot was already compressed:🛡️ Proposed fix
# Get original size - original_size = image_path.stat().st_size + current_size = image_path.stat().st_size + + # Preserve the true original size if already compressed + if force and screenshot.get("is_compressed") and screenshot.get("original_size_bytes"): + original_size = screenshot["original_size_bytes"] + else: + original_size = current_size # Open and re-save at lower quality with Image.open(image_path) as img: @@ .. # Get new size new_size = image_path.stat().st_size # Update database db.mark_compressed(screenshot["id"], original_size) - return original_size - new_size + return current_size - new_size
🤖 Fix all issues with AI agents
In `@web/src/app/settings/page.tsx`:
- Around line 149-160: The handler handleForceRecompressAgeChange does not reset
the user's confirmation when the selected age changes, so call the state setter
that clears the confirmation (setForceRecompressConfirmed(false)) inside
handleForceRecompressAgeChange (e.g., immediately after
setForceRecompressAge(days)) so the confirmation checkbox is unchecked whenever
the dropdown selection changes; keep existing preview/loading logic intact so
previewForceRecompress still runs and updates setForceRecompressPreview.
In `@web/src/components/CompressionSuggestion.tsx`:
- Around line 9-14: The formatBytes function can index past the units array for
values >= 1 TB; update formatBytes so it never reads units out of bounds by
either adding the 'TB' unit to the units array or clamping the computed i to
units.length - 1 (e.g., compute i = Math.min(i, units.length - 1)), and ensure
the toFixed logic still behaves correctly when clamped; adjust references to
units and i in formatBytes accordingly.
🧹 Nitpick comments (3)
core/tests/test_compression.py (1)
286-292: Pagination offset assertions are effectively no-ops.Line 290:
... == 0 or calls[0] == calls[0]— the right side is alwaysTrue, so this assertion always passes regardless of the actual offset value.Line 292: The fallback
or calls[1].kwargs.get("offset", 0) > 0weakens the check. Consider asserting the kwargs directly sincestart_force_recompresscallsget_force_recompressible_screenshots(older_than_days, limit=batch_size, offset=offset):♻️ Suggested fix for meaningful assertions
- # First call: offset=0 - assert calls[0].kwargs.get("offset", calls[0][1][2] if len(calls[0][1]) > 2 else 0) == 0 or calls[0] == calls[0] - # Check that offset increases - assert calls[1][1] == (30,) or calls[1].kwargs.get("offset", 0) > 0 + # First call: offset=0 + assert calls[0].kwargs.get("offset") == 0 + # Second call: offset = batch_size (50) + assert calls[1].kwargs.get("offset") == 50 + # Third call: offset = 2 * batch_size (100) + assert calls[2].kwargs.get("offset") == 100core/config.py (1)
56-56: Default compression quality reduced from 85 to 75.This is a silent behavioral change for new users/installations. Existing users retain their persisted value via
config.load(). Ensure this is intentional and documented (e.g., in release notes), since it affects the visual quality of auto-compressed screenshots.web/src/app/settings/page.tsx (1)
448-525: Modal accessibility: no keyboard dismiss support.The modal overlay doesn't respond to
Escapekey presses. Consider adding anonKeyDownhandler to improve keyboard accessibility.♿ Proposed improvement
Add an effect to listen for
Escapewhen the modal is open:useEffect(() => { if (!showForceRecompress) return; const handleKey = (e: KeyboardEvent) => { if (e.key === 'Escape') { setShowForceRecompress(false); setForceRecompressPreview(null); setForceRecompressConfirmed(false); } }; document.addEventListener('keydown', handleKey); return () => document.removeEventListener('keydown', handleKey); }, [showForceRecompress]);
| const handleForceRecompressAgeChange = async (days: number) => { | ||
| setForceRecompressAge(days); | ||
| setForceRecompressLoading(true); | ||
| try { | ||
| const preview = await previewForceRecompress(days); | ||
| setForceRecompressPreview(preview); | ||
| } catch (err) { | ||
| console.error('Failed to preview:', err); | ||
| } finally { | ||
| setForceRecompressLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Reset the confirmation checkbox when the age selection changes.
When the user changes the age in the dropdown, forceRecompressConfirmed retains its previous value. This means a user who confirmed for "3 months" can unknowingly proceed for "1 year" without re-confirming, since the preview data updates but the checkbox stays checked.
🛡️ Proposed fix
const handleForceRecompressAgeChange = async (days: number) => {
setForceRecompressAge(days);
+ setForceRecompressConfirmed(false);
setForceRecompressLoading(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleForceRecompressAgeChange = async (days: number) => { | |
| setForceRecompressAge(days); | |
| setForceRecompressLoading(true); | |
| try { | |
| const preview = await previewForceRecompress(days); | |
| setForceRecompressPreview(preview); | |
| } catch (err) { | |
| console.error('Failed to preview:', err); | |
| } finally { | |
| setForceRecompressLoading(false); | |
| } | |
| }; | |
| const handleForceRecompressAgeChange = async (days: number) => { | |
| setForceRecompressAge(days); | |
| setForceRecompressConfirmed(false); | |
| setForceRecompressLoading(true); | |
| try { | |
| const preview = await previewForceRecompress(days); | |
| setForceRecompressPreview(preview); | |
| } catch (err) { | |
| console.error('Failed to preview:', err); | |
| } finally { | |
| setForceRecompressLoading(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@web/src/app/settings/page.tsx` around lines 149 - 160, The handler
handleForceRecompressAgeChange does not reset the user's confirmation when the
selected age changes, so call the state setter that clears the confirmation
(setForceRecompressConfirmed(false)) inside handleForceRecompressAgeChange
(e.g., immediately after setForceRecompressAge(days)) so the confirmation
checkbox is unchecked whenever the dropdown selection changes; keep existing
preview/loading logic intact so previewForceRecompress still runs and updates
setForceRecompressPreview.
| function formatBytes(bytes: number): string { | ||
| if (bytes === 0) return '0 B'; | ||
| const units = ['B', 'KB', 'MB', 'GB']; | ||
| const i = Math.floor(Math.log(bytes) / Math.log(1024)); | ||
| return `${(bytes / Math.pow(1024, i)).toFixed(i > 1 ? 1 : 0)} ${units[i]}`; | ||
| } |
There was a problem hiding this comment.
formatBytes can produce undefined unit for values ≥ 1 TB.
If bytes ≥ 1099511627776 (1 TB), i becomes 4, and units[4] is undefined. While unlikely for estimated savings here, this is a reusable utility pattern that should be robust.
🛡️ Proposed fix
function formatBytes(bytes: number): string {
if (bytes === 0) return '0 B';
- const units = ['B', 'KB', 'MB', 'GB'];
- const i = Math.floor(Math.log(bytes) / Math.log(1024));
+ const units = ['B', 'KB', 'MB', 'GB', 'TB'];
+ const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1);
return `${(bytes / Math.pow(1024, i)).toFixed(i > 1 ? 1 : 0)} ${units[i]}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function formatBytes(bytes: number): string { | |
| if (bytes === 0) return '0 B'; | |
| const units = ['B', 'KB', 'MB', 'GB']; | |
| const i = Math.floor(Math.log(bytes) / Math.log(1024)); | |
| return `${(bytes / Math.pow(1024, i)).toFixed(i > 1 ? 1 : 0)} ${units[i]}`; | |
| } | |
| function formatBytes(bytes: number): string { | |
| if (bytes === 0) return '0 B'; | |
| const units = ['B', 'KB', 'MB', 'GB', 'TB']; | |
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1); | |
| return `${(bytes / Math.pow(1024, i)).toFixed(i > 1 ? 1 : 0)} ${units[i]}`; | |
| } |
🤖 Prompt for AI Agents
In `@web/src/components/CompressionSuggestion.tsx` around lines 9 - 14, The
formatBytes function can index past the units array for values >= 1 TB; update
formatBytes so it never reads units out of bounds by either adding the 'TB' unit
to the units array or clamping the computed i to units.length - 1 (e.g., compute
i = Math.min(i, units.length - 1)), and ensure the toFixed logic still behaves
correctly when clamped; adjust references to units and i in formatBytes
accordingly.
When the screen changes too rapidly (e.g. video playback), the stability check repeatedly fails and no screenshots get saved. This adds a configurable time threshold (max_time_without_save, default 40s) that forces a save when stability checks keep failing but enough time has passed since the last save. Static screens are unaffected since the force-save logic only runs when a change is detected.
Add the chaotic save timeout setting to the API schema, route handler, frontend types, and settings page with a slider (0-120s, 0 = off).
a580cec to
3cee584
Compare
1 participant
Summary
max_time_without_save, default 40s) that forces a save when stability checks keep failing but enough time has passed since the last save.Changes
core/config.py— Addedmax_time_without_savetoCaptureSettingswith save/loadcore/capture.py— Force-save logic +_last_save_timetracking in capture loop andcapture_now()api/schemas.py— Added toCaptureConfigandConfigUpdateRequest(validated 0–120)api/routes/status.py— GET/PUT support for the new settingweb/src/types/index.ts,web/src/lib/api.ts,web/src/app/settings/page.tsx— Frontend type, API client, and UI sliderTest plan
/api/v1/configreturnsmax_time_without_saveSummary by CodeRabbit