feat: Compression improvements with force recompress#34
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a force-recompression feature: preview and start endpoints, request/response schemas, database queries for all older screenshots, background recompression in CompressionService (force flag), frontend modal and API client, a compression suggestion component, and accompanying tests and config tweak. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser/Frontend
participant API as API Server
participant DB as Database
participant Service as CompressionService
participant Worker as Background Worker
User->>Browser: Open Force Recompress modal
Browser->>API: POST /compression/force-recompress/preview {older_than_days}
API->>DB: get_force_recompressible_count(older_than_days)
DB-->>API: {total, already_compressed, not_compressed}
API-->>Browser: preview response (counts, optional warning)
User->>Browser: Confirm recompress
Browser->>API: POST /compression/force-recompress {older_than_days, quality, confirm:true}
API->>Service: start_force_recompress(older_than_days, quality)
Service->>DB: get_force_recompressible_screenshots(older_than_days, limit, offset)
DB-->>Service: batch of screenshots
Service->>Worker: spawn worker thread to process batches
Worker->>Service: call _compress_screenshot(screenshot, quality, force=true)
Service-->>API: {success, affected_count}
API-->>Browser: result response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/compression.py (1)
133-169:⚠️ Potential issue | 🟠 MajorForce recompress overwrites
original_size_byteswith the already-compressed size.When an already-compressed screenshot is force-recompressed, Line 152 captures the current (compressed) file size as
original_size, and Line 167 stores it viamark_compressed. This overwrites the true original size in the database, making historical storage savings metrics inaccurate.Consider preserving the existing
original_size_byteswhen the screenshot is already compressed:🛡️ Proposed fix
def _compress_screenshot(self, screenshot: dict, quality: int, force: bool = False) -> int: ... # 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 + original_size = screenshot.get("original_size_bytes") or current_size # Open and re-save at lower quality with Image.open(image_path) as img: if img.mode in ("RGBA", "P"): img = img.convert("RGB") img.save(str(image_path), "JPEG", quality=quality, optimize=True) new_size = image_path.stat().st_size db.mark_compressed(screenshot["id"], original_size) - return original_size - new_size + return current_size - new_sizeapi/routes/compression.py (1)
44-50:⚠️ Potential issue | 🟡 MinorStale docstring: default values no longer match config.
The docstring references "default: 60" days and "default: 85" quality, but
CompressionSettingsdefaults were updated to 90 days and quality 75 incore/config.py.📝 Proposed fix
""" Start compressing old screenshots. - Compresses screenshots older than the specified days (default: 60) - to a lower quality (default: 85) to save storage space. + Compresses screenshots older than the specified days (default: 90) + to a lower quality (default: 75) to save storage space. This runs in the background - use /compression/status to monitor progress.
🤖 Fix all issues with AI agents
In `@api/routes/analytics.py`:
- Around line 206-232: The current query in the block using cur.execute(...) on
the screenshots table fetches the 100 most recent rows and then sorts those by
filesystem size in largest_files, which misses large files outside the newest
100; update the logic to reliably get the top 10 largest files by either (A)
using the DB column original_size_bytes if available — modify the SQL to SELECT
id, image_path, timestamp, original_size_bytes FROM screenshots ORDER BY
original_size_bytes DESC LIMIT 10 and use original_size_bytes to populate
largest_files — or (B) if original_size_bytes is not present/accurate, scan the
screenshots directory once (using Path on image_path for every file) to compute
sizes for all files, build largest_files from those results, then sort by
size_bytes and take the top 10; adjust references to cur.execute, screenshots,
largest_files, and Path accordingly.
- Around line 91-101: The async endpoints get_overview (and similarly
get_storage_analytics and get_quality_analytics) perform blocking filesystem I/O
by iterating screenshots_dir from get_screenshots_dir and calling stat() for
each file (building total_storage_bytes and file_sizes); move that directory
scan into a synchronous helper function (e.g., _scan_screenshots_dir) and call
it via asyncio.to_thread(...) from inside the async endpoint so the blocking
iterdir/stat work runs off the event loop, or alternatively convert the endpoint
signatures to sync def so FastAPI runs them in a threadpool—ensure references to
get_screenshots_dir, screenshots_dir, total_storage_bytes and file_sizes are
updated to use the helper results.
- Around line 35-45: The parse_timestamp function currently returns
datetime.now() for wrong-length input and will raise uncaught ValueError for
non-numeric 12-char strings; instead, validate the input up-front in
parse_timestamp: require len(ts) == 12 and ts.isdigit(), and if not raise a
clear ValueError (e.g., "Invalid timestamp: {ts}"), then parse using safe
conversion (or datetime.strptime) inside try/except and re-raise as ValueError
with context; this ensures callers receive explicit failures rather than silent
or unpredictable results.
- Around line 566-569: The gap statistics are being computed from the truncated
list `gaps` (so stats change with `limit`); compute `total_gap_time_seconds`,
`longest_gap_seconds`, and `avg_gap_seconds` from the full set of detected gaps
before any slicing/limit is applied. Concretely: capture the full gaps list
(e.g., `all_gaps` or compute stats immediately using `gaps_full`), calculate
`total_gap_time = sum(g["duration_seconds"] for g in <full_gaps>)`, `longest_gap
= max(... )` (or use first if already sorted) and `avg_gap = total_gap_time /
len(<full_gaps>) if <full_gaps> else 0`, then apply the existing
truncation/limit to produce the `gaps` returned to the caller. Update references
to `total_gap_time_seconds`, `longest_gap_seconds`, and `avg_gap_seconds` to use
these pre-truncation values.
- Around line 124-154: The DB cursor is held while calling Path(path).exists()
and path.stat() in the block using with db.cursor(), causing long locks; change
the flow so the SQL query using cur.execute(...) and cur.fetchall() (the rows
variable populated from the SELECT timestamp, image_path FROM screenshots query)
runs inside a short with db.cursor() and immediately returns rows, then perform
the filesystem iteration that builds date_sizes and calls
Path(...).exists()/stat() outside the with db.cursor() block; apply the same
pattern to any subsequent queries in this function so each database access is
limited to a single with db.cursor() that only fetches data (e.g., where
format_timestamp is used) and all expensive per-file operations occur after the
cursor is released.
In `@core/compression.py`:
- Around line 228-234: The loop in _recompress_loop uses
db.get_force_recompressible_screenshots which always returns the same first page
so after filtering by processed_ids the loop exits; modify
get_force_recompressible_screenshots in core/database.py to accept an offset
parameter (default 0) and apply LIMIT/OFFSET in the query, then change the loop
in core/compression.py to track a page or offset (e.g., offset += batch_size
each iteration) and pass that offset into get_force_recompressible_screenshots
instead of relying on processed_ids; keep the existing processed_ids filter to
handle duplicates but rely on offset-based paging to advance through results.
In `@web/src/app/analytics/page.tsx`:
- Around line 467-482: The gaps threshold is inconsistent: the Promise.all call
uses getAnalyticsGaps(5, 50) (minGapMinutes=5) while the UI InfoTooltip text
says "Periods of 30+ minutes without screenshots"; update one to match the
other. Either change the API invocation in the Promise.all to
getAnalyticsGaps(30, 50) so the fetched data reflects 30-minute gaps, or update
the InfoTooltip text near the InfoTooltip component to say "Periods of 5+
minutes without screenshots" (or the desired canonical threshold); adjust
whichever you choose so getAnalyticsGaps and the InfoTooltip wording are
consistent.
In `@web/src/app/page.tsx`:
- Around line 1633-1660: The "Find Similar" button currently uses
selectedImage.timestamp to build a text query via setQuery and does not perform
visual similarity; change its UX to be honest: either remove the button and its
click handler (the onClick that calls setSelectedImage, setActiveView,
setSearchMode and setQuery) or rename/repurpose it (e.g., label "Search by time"
or "Search nearby time") and update any accessible text/tooltip to reflect that
it runs a time-based text search using selectedImage.timestamp; ensure
references to setActiveView('search'), setSearchMode('image') are adjusted or
removed to avoid implying image-similarity.
- Line 793: The getScreenshots call in page.tsx is using a hardcoded page size
100 which is inconsistent with the rest of the gallery; replace the literal 100
with the GALLERY_PAGE_SIZE constant (i.e. call getScreenshots(GALLERY_PAGE_SIZE,
0, timelineStartDate, timelineEndDate, visibilityFilter)), and ensure
GALLERY_PAGE_SIZE is imported/available in this module so the refresh uses the
same page size as other gallery fetches.
In `@web/src/app/settings/page.tsx`:
- Around line 160-168: The handler handleForceRecompress currently closes the
modal and clears preview regardless of API-level failures; change it to capture
the response returned by startForceRecompress (e.g., const res = await
startForceRecompress(forceRecompressAge)), check res.success before calling
setShowForceRecompress(false) and setForceRecompressPreview(null), and when
res.success is false surface the error (set an error state or show the existing
error UI/message and log the details) so the modal stays open and the user sees
the backend error; keep try/catch for network exceptions but treat a falsy
success from the API as a handled failure rather than a success.
🧹 Nitpick comments (4)
web/src/app/analytics/page.tsx (1)
446-454: LocalformatTimestampshadows the module-level function.The inner
formatTimestamp(takesDate) shadows the outer one at Line 94 (takesstring). Consider renaming toformatDateToTimestampor similar to avoid confusion during maintenance.web/src/app/page.tsx (1)
1756-1896: Date Range Picker uses imperative DOM access instead of React state.The modal reads form values via
document.getElementById()and toggles validation error visibility viaclassList. This bypasses React's rendering model and is fragile (e.g., IDs could collide, refs can be stale if the component remounts).Consider converting the date/time inputs to controlled components backed by
useState, and replacing theclassListtoggle on#range-errorwith a boolean state variable.Sketch of controlled approach
+ const [rangeStartDate, setRangeStartDate] = useState(''); + const [rangeStartHour, setRangeStartHour] = useState('00'); + const [rangeStartMin, setRangeStartMin] = useState('00'); + const [rangeEndDate, setRangeEndDate] = useState(''); + const [rangeEndHour, setRangeEndHour] = useState('23'); + const [rangeEndMin, setRangeEndMin] = useState('59'); + const [rangeError, setRangeError] = useState(false);Then bind these to the inputs via
value+onChange, and replace:- const startDate = (document.getElementById('range-start-date') as HTMLInputElement)?.value; + // Use rangeStartDate directly- if (errorEl) errorEl.classList.remove('hidden'); + setRangeError(true);api/routes/analytics.py (2)
53-114: Missingresponse_modelon all endpoints.Per coding guidelines, FastAPI endpoints should define
response_model. This applies to all seven endpoints in this file. Consider defining Pydantic response models (or at minimum usingresponse_model=dict) to get automatic validation and OpenAPI documentation.As per coding guidelines,
api/routes/**/*.py: "define endpoints with response_model and docstrings".
465-486: Loading all screenshot timestamps into memory for gap detection could be expensive.Both queries (
WHERE is_hidden = 0andWHERE is_hidden = 1) fetch every row's timestamp. For a long-running instance with millions of screenshots, this could consume significant memory. Consider adding a date-range bound (e.g., last N days) or paginating the gap detection.
| def parse_timestamp(ts: str) -> datetime: | ||
| """Convert YYMMDDHHMMSS to datetime.""" | ||
| if len(ts) != 12: | ||
| return datetime.now() | ||
| year = 2000 + int(ts[0:2]) | ||
| month = int(ts[2:4]) | ||
| day = int(ts[4:6]) | ||
| hour = int(ts[6:8]) | ||
| minute = int(ts[8:10]) | ||
| second = int(ts[10:12]) | ||
| return datetime(year, month, day, hour, minute, second) |
There was a problem hiding this comment.
parse_timestamp silently returns datetime.now() for invalid-length input and will raise ValueError for non-numeric 12-char strings.
Line 38 returns datetime.now() for timestamps that aren't 12 chars — this silently produces incorrect analytics data. Lines 39–45 will throw an unhandled ValueError if the string contains non-numeric characters (e.g., a corrupted DB entry).
Suggested hardening
def parse_timestamp(ts: str) -> datetime:
"""Convert YYMMDDHHMMSS to datetime."""
- if len(ts) != 12:
+ if len(ts) != 12 or not ts.isdigit():
return datetime.now()
- year = 2000 + int(ts[0:2])
- ...
- return datetime(year, month, day, hour, minute, second)
+ try:
+ year = 2000 + int(ts[0:2])
+ month = int(ts[2:4])
+ day = int(ts[4:6])
+ hour = int(ts[6:8])
+ minute = int(ts[8:10])
+ second = int(ts[10:12])
+ return datetime(year, month, day, hour, minute, second)
+ except (ValueError, OverflowError):
+ return datetime.now()🤖 Prompt for AI Agents
In `@api/routes/analytics.py` around lines 35 - 45, The parse_timestamp function
currently returns datetime.now() for wrong-length input and will raise uncaught
ValueError for non-numeric 12-char strings; instead, validate the input up-front
in parse_timestamp: require len(ts) == 12 and ts.isdigit(), and if not raise a
clear ValueError (e.g., "Invalid timestamp: {ts}"), then parse using safe
conversion (or datetime.strptime) inside try/except and re-raise as ValueError
with context; this ensures callers receive explicit failures rather than silent
or unpredictable results.
| with db.cursor() as cur: | ||
| # Get daily storage data for the last N days | ||
| end_date = datetime.now() | ||
| start_date = end_date - timedelta(days=days) | ||
|
|
||
| daily_data = [] | ||
|
|
||
| # Build a lookup of file sizes by date from the database timestamps | ||
| cur.execute( | ||
| """ | ||
| SELECT timestamp, image_path | ||
| FROM screenshots | ||
| WHERE timestamp >= ? | ||
| ORDER BY timestamp ASC | ||
| """, | ||
| (format_timestamp(start_date),), | ||
| ) | ||
| rows = cur.fetchall() | ||
|
|
||
| # Group by date | ||
| date_sizes: dict[str, list[int]] = {} | ||
| for row in rows: | ||
| ts = row[0] | ||
| path = Path(row[1]) | ||
| if len(ts) >= 6: | ||
| date_key = f"20{ts[0:2]}-{ts[2:4]}-{ts[4:6]}" | ||
| if path.exists(): | ||
| size = path.stat().st_size | ||
| if date_key not in date_sizes: | ||
| date_sizes[date_key] = [] | ||
| date_sizes[date_key].append(size) |
There was a problem hiding this comment.
DB lock held during per-file stat() calls in storage analytics.
The db.cursor() context manager holds a thread lock (see core/database.py:61-74). Inside that lock, lines 148–154 call path.stat() for every screenshot in the date range. This blocks all other database operations for the duration of potentially thousands of filesystem syscalls.
Fetch the rows first, release the cursor, then iterate the filesystem outside the lock.
Proposed restructure
- with db.cursor() as cur:
- ...
- cur.execute(...)
- rows = cur.fetchall()
-
- # Group by date
- date_sizes: dict[str, list[int]] = {}
- for row in rows:
- ts = row[0]
- path = Path(row[1])
- if len(ts) >= 6:
- date_key = f"20{ts[0:2]}-{ts[2:4]}-{ts[4:6]}"
- if path.exists():
- size = path.stat().st_size
+ with db.cursor() as cur:
+ cur.execute(...)
+ rows = cur.fetchall()
+
+ # Filesystem I/O outside the DB lock
+ date_sizes: dict[str, list[int]] = {}
+ for row in rows:
+ ts = row[0]
+ path = Path(row[1])
+ if len(ts) >= 6:
+ date_key = f"20{ts[0:2]}-{ts[2:4]}-{ts[4:6]}"
+ if path.exists():
+ size = path.stat().st_sizeSimilarly split the later queries into separate with db.cursor() blocks.
🤖 Prompt for AI Agents
In `@api/routes/analytics.py` around lines 124 - 154, The DB cursor is held while
calling Path(path).exists() and path.stat() in the block using with db.cursor(),
causing long locks; change the flow so the SQL query using cur.execute(...) and
cur.fetchall() (the rows variable populated from the SELECT timestamp,
image_path FROM screenshots query) runs inside a short with db.cursor() and
immediately returns rows, then perform the filesystem iteration that builds
date_sizes and calls Path(...).exists()/stat() outside the with db.cursor()
block; apply the same pattern to any subsequent queries in this function so each
database access is limited to a single with db.cursor() that only fetches data
(e.g., where format_timestamp is used) and all expensive per-file operations
occur after the cursor is released.
| # Get largest files | ||
| cur.execute( | ||
| """ | ||
| SELECT id, image_path, timestamp | ||
| FROM screenshots | ||
| ORDER BY timestamp DESC | ||
| LIMIT 100 | ||
| """ | ||
| ) | ||
|
|
||
| largest_files = [] | ||
| for row in cur.fetchall(): | ||
| path = Path(row[1]) | ||
| if path.exists(): | ||
| size = path.stat().st_size | ||
| largest_files.append( | ||
| { | ||
| "id": row[0], | ||
| "path": row[1], | ||
| "timestamp": row[2], | ||
| "size_bytes": size, | ||
| } | ||
| ) | ||
|
|
||
| # Sort by size and take top 10 | ||
| largest_files.sort(key=lambda x: x["size_bytes"], reverse=True) | ||
| largest_files = largest_files[:10] |
There was a problem hiding this comment.
"Largest files" query fetches the 100 most recent screenshots, not the largest.
The SQL orders by timestamp DESC LIMIT 100, then Python sorts by file size. This means it only finds the largest among the 100 newest files, missing potentially much larger files earlier in the dataset.
If the goal is to find the top 10 largest files overall, consider either: storing file sizes in the DB (available for compressed files via original_size_bytes), or scanning all files in the directory once and sorting.
🤖 Prompt for AI Agents
In `@api/routes/analytics.py` around lines 206 - 232, The current query in the
block using cur.execute(...) on the screenshots table fetches the 100 most
recent rows and then sorts those by filesystem size in largest_files, which
misses large files outside the newest 100; update the logic to reliably get the
top 10 largest files by either (A) using the DB column original_size_bytes if
available — modify the SQL to SELECT id, image_path, timestamp,
original_size_bytes FROM screenshots ORDER BY original_size_bytes DESC LIMIT 10
and use original_size_bytes to populate largest_files — or (B) if
original_size_bytes is not present/accurate, scan the screenshots directory once
(using Path on image_path for every file) to compute sizes for all files, build
largest_files from those results, then sort by size_bytes and take the top 10;
adjust references to cur.execute, screenshots, largest_files, and Path
accordingly.
| # Calculate statistics | ||
| total_gap_time = sum(g["duration_seconds"] for g in gaps) | ||
| longest_gap = gaps[0]["duration_seconds"] if gaps else 0 | ||
| avg_gap = total_gap_time / len(gaps) if gaps else 0 |
There was a problem hiding this comment.
Gap statistics are computed on the truncated (limit-applied) result set, not all detected gaps.
total_gap_time_seconds, longest_gap_seconds, and avg_gap_seconds are calculated from the post-limit gaps list. This means the reported statistics change depending on the limit parameter, which is misleading — a user expects summary stats to reflect the full dataset. Compute these from the full gaps list before truncation.
Proposed fix: compute stats before truncation
+ # Calculate statistics from ALL detected gaps before limiting
+ all_gap_times = [g["duration_seconds"] for g in gaps]
+ total_gap_time = sum(all_gap_times)
+ longest_gap = max(all_gap_times) if all_gap_times else 0
+ avg_gap = total_gap_time / len(all_gap_times) if all_gap_times else 0
+ full_gap_count = len(gaps)
+
# Separate incognito and regular gaps...
...
gaps = sorted(selected_incognito + selected_regular, ...)
- # Calculate statistics
- total_gap_time = sum(g["duration_seconds"] for g in gaps)
- longest_gap = gaps[0]["duration_seconds"] if gaps else 0
- avg_gap = total_gap_time / len(gaps) if gaps else 0
return {
"gaps": gaps,
"total_gap_time_seconds": total_gap_time,
"longest_gap_seconds": longest_gap,
"avg_gap_seconds": int(avg_gap),
- "gap_count": len(gaps),
+ "gap_count": full_gap_count,
}🤖 Prompt for AI Agents
In `@api/routes/analytics.py` around lines 566 - 569, The gap statistics are being
computed from the truncated list `gaps` (so stats change with `limit`); compute
`total_gap_time_seconds`, `longest_gap_seconds`, and `avg_gap_seconds` from the
full set of detected gaps before any slicing/limit is applied. Concretely:
capture the full gaps list (e.g., `all_gaps` or compute stats immediately using
`gaps_full`), calculate `total_gap_time = sum(g["duration_seconds"] for g in
<full_gaps>)`, `longest_gap = max(... )` (or use first if already sorted) and
`avg_gap = total_gap_time / len(<full_gaps>) if <full_gaps> else 0`, then apply
the existing truncation/limit to produce the `gaps` returned to the caller.
Update references to `total_gap_time_seconds`, `longest_gap_seconds`, and
`avg_gap_seconds` to use these pre-truncation values.
| const handleForceRecompress = async () => { | ||
| try { | ||
| await startForceRecompress(forceRecompressAge); | ||
| setShowForceRecompress(false); | ||
| setForceRecompressPreview(null); | ||
| } catch (err) { | ||
| console.error('Failed to start force recompression:', err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
API-level failures silently close the modal.
If the backend returns {success: false, ...} (e.g., compression started by another client between preview and execute), the modal closes without showing the error. Consider checking the response's success field before closing:
💡 Proposed fix
const handleForceRecompress = async () => {
try {
- await startForceRecompress(forceRecompressAge);
- setShowForceRecompress(false);
- setForceRecompressPreview(null);
+ const result = await startForceRecompress(forceRecompressAge);
+ if (result.success) {
+ setShowForceRecompress(false);
+ setForceRecompressPreview(null);
+ } else {
+ console.error('Force recompress failed:', result.message);
+ // Optionally display result.message to the user
+ }
} catch (err) {
console.error('Failed to start force recompression:', err);
}
};📝 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 handleForceRecompress = async () => { | |
| try { | |
| await startForceRecompress(forceRecompressAge); | |
| setShowForceRecompress(false); | |
| setForceRecompressPreview(null); | |
| } catch (err) { | |
| console.error('Failed to start force recompression:', err); | |
| } | |
| }; | |
| const handleForceRecompress = async () => { | |
| try { | |
| const result = await startForceRecompress(forceRecompressAge); | |
| if (result.success) { | |
| setShowForceRecompress(false); | |
| setForceRecompressPreview(null); | |
| } else { | |
| console.error('Force recompress failed:', result.message); | |
| // Optionally display result.message to the user | |
| } | |
| } catch (err) { | |
| console.error('Failed to start force recompression:', err); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@web/src/app/settings/page.tsx` around lines 160 - 168, The handler
handleForceRecompress currently closes the modal and clears preview regardless
of API-level failures; change it to capture the response returned by
startForceRecompress (e.g., const res = await
startForceRecompress(forceRecompressAge)), check res.success before calling
setShowForceRecompress(false) and setForceRecompressPreview(null), and when
res.success is false surface the error (set an error state or show the existing
error UI/message and log the details) so the modal stays open and the user sees
the backend error; keep try/catch for network exceptions but treat a falsy
success from the API as a handled failure rather than a success.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/src/app/page.tsx`:
- Around line 1860-1889: The click handler currently reads DOM nodes by id
('range-start-date', 'range-start-hour', 'range-start-min', 'range-end-date',
'range-end-hour', 'range-end-min') and toggles validation via classList; convert
these inputs to controlled React state (or at minimum useRef) and replace direct
DOM access with state values, e.g., maintain
startDate/startHour/startMin/endDate/endHour/endMin and a boolean rangeError
state; update the Apply logic to validate using those state values, call
setTimelineStartDate and setTimelineEndDate with the formatted timestamp,
setShowDateRangePicker(false) on success, and render the error UI conditionally
(e.g., {rangeError && <...>}) instead of manipulating classList; consider
extracting this UI and logic into a DateRangePickerModal component to keep the
page component clean.
In `@web/src/components/CompressionSuggestion.tsx`:
- Around line 96-98: The banner in CompressionSuggestion (component
CompressionSuggestion.tsx) hardcodes "older than 2 months" but the app default
and user-configurable value is compression_after_days (now 90); update the copy
to derive the displayed threshold from the actual configured
compression_after_days (or a prop passed into CompressionSuggestion) instead of
hardcoding "2 months" — compute a human-friendly label (e.g., days or months
from compression_after_days) and use it in the paragraph next to
stats.compressible_count so the text always reflects the real configured
threshold.
🧹 Nitpick comments (7)
web/src/app/settings/page.tsx (3)
149-160: Stale preview response can overwrite a newer one.If the user rapidly switches the age selector, two
previewForceRecompresscalls may be in-flight and the earlier request can resolve last, displaying stale data. This is a low-probability issue given discrete select options, but on slow networks it's plausible.A simple guard is to compare the requested
daysvalue against the current state before applying:Sketch using a ref
+ const latestPreviewRef = useRef(0); + const handleForceRecompressAgeChange = async (days: number) => { setForceRecompressAge(days); setForceRecompressLoading(true); + const requestId = ++latestPreviewRef.current; try { const preview = await previewForceRecompress(days); - setForceRecompressPreview(preview); + if (latestPreviewRef.current === requestId) { + setForceRecompressPreview(preview); + } } catch (err) { console.error('Failed to preview:', err); } finally { - setForceRecompressLoading(false); + if (latestPreviewRef.current === requestId) { + setForceRecompressLoading(false); + } } };
135-147: No user-visible feedback when the preview request fails.If
previewForceRecompressthrows, the modal shows with only the age selector and disabled buttons — no indication of what went wrong. Consider setting a small error state so the modal can display a retry prompt or message.
440-516: Modal lacks basic accessibility attributes and keyboard dismissal.The overlay
<div>acts as a dialog but has norole="dialog",aria-modal="true", oraria-labelledby. There's also noEscapekey handler to close it. This can be a barrier for keyboard and screen-reader users.Minimal additions
Add to the outer overlay div:
- <div className="fixed inset-0 z-50 flex items-center justify-center bg-black/70"> + <div + className="fixed inset-0 z-50 flex items-center justify-center bg-black/70" + role="dialog" + aria-modal="true" + aria-labelledby="force-recompress-title" + onKeyDown={(e) => { if (e.key === 'Escape') { setShowForceRecompress(false); setForceRecompressPreview(null); setForceRecompressConfirmed(false); } }} + tabIndex={-1} + >And on the heading:
- <h3 className="text-sm font-medium text-[`#f5f5f5`] mb-4">Force Recompress</h3> + <h3 id="force-recompress-title" className="text-sm font-medium text-[`#f5f5f5`] mb-4">Force Recompress</h3>web/src/components/CompressionSuggestion.tsx (2)
9-14:formatBytescan produceundefinedunit for large or negative inputs.If
bytesexceeds ~1 TB,iwill be ≥ 4 andunits[i]isundefined, rendering something like"1.0 undefined". Negative values causeNaNviaMath.log. Both are unlikely given the usage context, but worth clamping defensively.🛡️ Suggested defensive clamp
function formatBytes(bytes: number): string { if (bytes === 0) return '0 B'; + if (bytes < 0) return '0 B'; const units = ['B', 'KB', 'MB', 'GB']; - const i = Math.floor(Math.log(bytes) / Math.log(1024)); + 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]}`; }
64-65: Magic numbers for savings estimation are reasonable but worth documenting as constants.The
295_000 * 0.55formula is explained by the inline comment, which is fine. Just noting this is a rough estimate — if the default compression quality or capture quality changes, this will silently drift. Optional nit, no action required.web/src/app/page.tsx (2)
1327-1338: Extract the inline IIFE into a helper oruseMemo.The timestamp-parsing + formatting logic inside the JSX render path is hard to read and duplicates the pattern in
formatTimestamp(line 993). A small helper likeformatDateRange(start, end)or auseMemowould improve readability and be reusable.
51-1927: Consider breakingHomeContentinto smaller components.At ~1900 lines with 30+ state variables, this component has grown well past comfortable review/maintenance size. The date range picker modal, lightbox, timeline view, and gallery view are natural extraction candidates. This is non-blocking but will compound with each new feature.
| <button | ||
| onClick={() => { | ||
| const startDate = (document.getElementById('range-start-date') as HTMLInputElement)?.value; | ||
| const startHour = (document.getElementById('range-start-hour') as HTMLSelectElement)?.value || '00'; | ||
| const startMin = (document.getElementById('range-start-min') as HTMLSelectElement)?.value || '00'; | ||
| const endDate = (document.getElementById('range-end-date') as HTMLInputElement)?.value; | ||
| const endHour = (document.getElementById('range-end-hour') as HTMLSelectElement)?.value || '23'; | ||
| const endMin = (document.getElementById('range-end-min') as HTMLSelectElement)?.value || '59'; | ||
| const errorEl = document.getElementById('range-error'); | ||
|
|
||
| if (startDate && endDate) { | ||
| // Validate: start must be before end | ||
| const startDt = new Date(`${startDate}T${startHour}:${startMin}:00`); | ||
| const endDt = new Date(`${endDate}T${endHour}:${endMin}:00`); | ||
|
|
||
| if (startDt >= endDt) { | ||
| if (errorEl) errorEl.classList.remove('hidden'); | ||
| return; | ||
| } | ||
| if (errorEl) errorEl.classList.add('hidden'); | ||
|
|
||
| const formatTs = (date: string, hour: string, min: string) => { | ||
| const [year, month, day] = date.split('-'); | ||
| return `${year.slice(-2)}${month}${day}${hour}${min}00`; | ||
| }; | ||
| setTimelineStartDate(formatTs(startDate, startHour, startMin)); | ||
| setTimelineEndDate(formatTs(endDate, endHour, endMin)); | ||
| setShowDateRangePicker(false); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Date Range Picker reads values via document.getElementById — use React state or refs instead.
The "Apply" handler queries the DOM directly (getElementById + casting) and toggles validation visibility via classList. This bypasses React's rendering model, is fragile (IDs can collide or be absent), and makes the modal untestable in isolation. Use controlled inputs (useState) or at minimum useRef for the form fields, and a boolean state for the validation error.
Sketch of controlled approach
+ // Add state at the top of HomeContent (or extract a DateRangePicker component)
+ const [rangeStartDate, setRangeStartDate] = useState('');
+ const [rangeStartHour, setRangeStartHour] = useState('00');
+ const [rangeStartMin, setRangeStartMin] = useState('00');
+ const [rangeEndDate, setRangeEndDate] = useState('');
+ const [rangeEndHour, setRangeEndHour] = useState('23');
+ const [rangeEndMin, setRangeEndMin] = useState('59');
+ const [rangeError, setRangeError] = useState(false);Then bind with value + onChange on each input/select, and replace the getElementById reads with the state values. Replace the classList toggle with {rangeError && <div className="text-xs text-[#ef4444]">…</div>}.
Given the growing complexity, this modal is also a good candidate for extraction into its own component (e.g., <DateRangePickerModal />).
🤖 Prompt for AI Agents
In `@web/src/app/page.tsx` around lines 1860 - 1889, The click handler currently
reads DOM nodes by id ('range-start-date', 'range-start-hour',
'range-start-min', 'range-end-date', 'range-end-hour', 'range-end-min') and
toggles validation via classList; convert these inputs to controlled React state
(or at minimum useRef) and replace direct DOM access with state values, e.g.,
maintain startDate/startHour/startMin/endDate/endHour/endMin and a boolean
rangeError state; update the Apply logic to validate using those state values,
call setTimelineStartDate and setTimelineEndDate with the formatted timestamp,
setShowDateRangePicker(false) on success, and render the error UI conditionally
(e.g., {rangeError && <...>}) instead of manipulating classList; consider
extracting this UI and logic into a DateRangePickerModal component to keep the
page component clean.
| <p className="text-xs text-[#666] mt-1.5 leading-relaxed"> | ||
| You have <span className="text-[#86efac] font-medium">{stats.compressible_count.toLocaleString()}</span> screenshots older than 2 months that can be compressed. Search and embeddings stay untouched. | ||
| </p> |
There was a problem hiding this comment.
Hardcoded "older than 2 months" likely doesn't match the actual threshold.
The PR changes the default compression_after_days from 60 to 90 (≈3 months), and this value is user-configurable. The banner text says "older than 2 months" which is inaccurate for the new default. Consider either fetching the configured threshold and deriving the text, or at minimum updating it to reflect the 90-day default.
Quick fix for the new default
- You have <span className="text-[`#86efac`] font-medium">{stats.compressible_count.toLocaleString()}</span> screenshots older than 2 months that can be compressed. Search and embeddings stay untouched.
+ You have <span className="text-[`#86efac`] font-medium">{stats.compressible_count.toLocaleString()}</span> screenshots older than 3 months that can be compressed. Search and embeddings stay untouched.📝 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.
| <p className="text-xs text-[#666] mt-1.5 leading-relaxed"> | |
| You have <span className="text-[#86efac] font-medium">{stats.compressible_count.toLocaleString()}</span> screenshots older than 2 months that can be compressed. Search and embeddings stay untouched. | |
| </p> | |
| <p className="text-xs text-[`#666`] mt-1.5 leading-relaxed"> | |
| You have <span className="text-[`#86efac`] font-medium">{stats.compressible_count.toLocaleString()}</span> screenshots older than 3 months that can be compressed. Search and embeddings stay untouched. | |
| </p> |
🤖 Prompt for AI Agents
In `@web/src/components/CompressionSuggestion.tsx` around lines 96 - 98, The
banner in CompressionSuggestion (component CompressionSuggestion.tsx) hardcodes
"older than 2 months" but the app default and user-configurable value is
compression_after_days (now 90); update the copy to derive the displayed
threshold from the actual configured compression_after_days (or a prop passed
into CompressionSuggestion) instead of hardcoding "2 months" — compute a
human-friendly label (e.g., days or months from compression_after_days) and use
it in the paragraph next to stats.compressible_count so the text always reflects
the real configured threshold.
Change default compression quality from 85 to 75 and age threshold from 60 to 90 days. Add force recompression feature that allows re-compressing all screenshots (including already compressed) older than a user-selected age, with preview and warning UI in Settings. - Update CompressionSettings defaults (quality 75, after_days 90) - Add force recompression DB methods with breakdown counts - Add batched force recompress with progress callbacks - Add API preview and execute endpoints with Pydantic schemas - Add force recompress modal in Settings with age selector and warnings
The previous approach fetched the same rows each batch since get_force_recompressible_screenshots doesn't filter on is_compressed. The processed_ids set filtered them all out, breaking the loop after the first batch. Switch to LIMIT/OFFSET pagination instead.
- Add floating banner on timeline when compressible screenshots exist - Banner offers one-click auto-compression enable with dismiss option - Uses localStorage to remember dismissal across sessions - Change default compression threshold from 90 to 60 days (2 months) - Add confirmation checkbox to force recompress modal for safety
- Check start_force_recompress return value to handle TOCTOU race - Save localStorage dismissal immediately, not after animation delay - Reset confirmation checkbox when closing force recompress modal - Handle partial failure: dismiss popup even if startCompression fails after config is successfully enabled
afa2926 to
1894e7e
Compare
Update compression quality default from 85 to 75 in test assertions and add 18 new tests covering force recompress service, DB methods, and API endpoints.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/tests/test_compression.py`:
- Around line 286-292: The test currently inspects
mock_db.get_force_recompressible_screenshots.call_args_list but uses no-op
comparisons; replace those with direct checks of each call's positional/keyword
args: read calls = mock_db.get_force_recompressible_screenshots.call_args_list
and assert calls[0].kwargs.get("offset", calls[0].args[2] if len(calls[0].args)
> 2 else 0) == 0, then assert calls[1].kwargs.get("offset", calls[1].args[2] if
len(calls[1].args) > 2 else None) == batch_size (50) and assert
calls[2].kwargs.get("offset", calls[2].args[2] if len(calls[2].args) > 2 else
None) == batch_size * 2 to validate pagination; also verify limit/batch_size via
calls[*].kwargs.get("limit") if needed.
🧹 Nitpick comments (1)
core/tests/conftest.py (1)
62-66:qualityupdated to match new default; consider aligningafter_daystoo.The
quality=75update correctly mirrors the new default fromcore/config.py. However, the PR also changes the defaultafter_daysfrom 60 → 90 incore/config.py, yet this fixture still usesafter_days=60. If this fixture is meant to track production defaults, it should be updated as well. If60is intentional for test isolation, this is fine as-is.
| # Verify offset-based calls | ||
| calls = mock_db.get_force_recompressible_screenshots.call_args_list | ||
| assert len(calls) == 3 | ||
| # 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 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the actual test file to see lines 286-292
cat -n core/tests/test_compression.py | sed -n '280,300p'Repository: VedankPurohit/LiveRecall
Length of output: 1122
🏁 Script executed:
# Search for calls to get_force_recompressible_screenshots
rg "get_force_recompressible_screenshots" -B 3 -A 3Repository: VedankPurohit/LiveRecall
Length of output: 5408
🏁 Script executed:
# Look at the test method setup and how the mock is configured
cat -n core/tests/test_compression.py | sed -n '252,275p'Repository: VedankPurohit/LiveRecall
Length of output: 1320
🏁 Script executed:
# Let's see the batch_size and the actual implementation of start_force_recompress
rg "batch_size" core/compression.py -B 2 -A 2Repository: VedankPurohit/LiveRecall
Length of output: 1529
🏁 Script executed:
# Let's also check how offset increments in the actual implementation
cat -n core/compression.py | sed -n '190,250p'Repository: VedankPurohit/LiveRecall
Length of output: 2776
🏁 Script executed:
# Let's verify the exact structure of call_args_list to understand calls[1][1]
python3 << 'EOF'
from unittest.mock import Mock, call
# Simulate how mock.call_args_list works
mock_db = Mock()
# Simulate the calls that would happen
mock_db.get_force_recompressible_screenshots(
older_than_days=30, limit=50, offset=0
)
mock_db.get_force_recompressible_screenshots(
older_than_days=30, limit=50, offset=50
)
mock_db.get_force_recompressible_screenshots(
older_than_days=30, limit=50, offset=100
)
calls = mock_db.get_force_recompressible_screenshots.call_args_list
print("Total calls:", len(calls))
for i, c in enumerate(calls):
print(f"\nCall {i}:")
print(f" calls[{i}]: {c}")
print(f" calls[{i}][0] (positional args): {c[0]}")
print(f" calls[{i}][1] (kwargs dict): {c[1]}")
print(f" calls[{i}].args: {c.args}")
print(f" calls[{i}].kwargs: {c.kwargs}")
# Now let's check what the assertions evaluate to
print("\n\nAssertion checks:")
print(f"calls[0] == calls[0]: {calls[0] == calls[0]}") # Always True
print(f"calls[1][1] == (30,): {calls[1][1] == (30,)}") # False (kwargs dict ≠ tuple)
print(f"calls[1].kwargs.get('offset', 0) > 0: {calls[1].kwargs.get('offset', 0) > 0}") # True (offset=50)
print("\n\nWith proposed fix:")
print(f"calls[0].kwargs['offset'] == 0: {calls[0].kwargs['offset'] == 0}")
print(f"calls[1].kwargs['offset'] == 50: {calls[1].kwargs['offset'] == 50}")
print(f"calls[2].kwargs['offset'] == 100: {calls[2].kwargs['offset'] == 100}")
EOFRepository: VedankPurohit/LiveRecall
Length of output: 1154
Line 290: assertion is a no-op — or calls[0] == calls[0] is always True.
The implementation calls get_force_recompressible_screenshots() with keyword arguments: older_than_days, limit=batch_size, offset=offset where batch_size=50. The trailing or calls[0] == calls[0] causes this assertion to pass unconditionally. Line 292 is similarly broken—calls[1][1] == (30,) compares kwargs (a dict) to a tuple, which is always False, leaving the test to rely only on the fallback condition. These assertions don't validate offset pagination at all.
Proposed fix — directly inspect the call arguments
# Verify offset-based calls
calls = mock_db.get_force_recompressible_screenshots.call_args_list
assert len(calls) == 3
- # 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
+ # Verify offsets increase across batches
+ assert calls[0].kwargs["offset"] == 0
+ assert calls[1].kwargs["offset"] == 50
+ assert calls[2].kwargs["offset"] == 100🤖 Prompt for AI Agents
In `@core/tests/test_compression.py` around lines 286 - 292, The test currently
inspects mock_db.get_force_recompressible_screenshots.call_args_list but uses
no-op comparisons; replace those with direct checks of each call's
positional/keyword args: read calls =
mock_db.get_force_recompressible_screenshots.call_args_list and assert
calls[0].kwargs.get("offset", calls[0].args[2] if len(calls[0].args) > 2 else 0)
== 0, then assert calls[1].kwargs.get("offset", calls[1].args[2] if
len(calls[1].args) > 2 else None) == batch_size (50) and assert
calls[2].kwargs.get("offset", calls[2].args[2] if len(calls[2].args) > 2 else
None) == batch_size * 2 to validate pagination; also verify limit/batch_size via
calls[*].kwargs.get("limit") if needed.
1 participant
Summary
is_compressed = 0DB filterChanges
Backend:
core/config.py— UpdatedCompressionSettingsdefaultscore/database.py— Addedget_force_recompressible_screenshots()andget_force_recompressible_count()core/compression.py— Addedforceparam to_compress_screenshot(), addedstart_force_recompress()with batched processing and progress callbacksapi/schemas.py— Added 4 Pydantic schemas for force recompress preview/executeapi/routes/compression.py— AddedPOST /force-recompress/previewandPOST /force-recompressendpointsFrontend:
web/src/types/index.ts— AddedForceRecompressPreviewtypeweb/src/lib/api.ts— AddedpreviewForceRecompress()andstartForceRecompress()API functionsweb/src/app/settings/page.tsx— Added force recompress button and confirmation modal in Storage sectionTest plan
/compression/status🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests