feat(rewind): add deviceName to screenshots for multi-device disambiguation#6321
feat(rewind): add deviceName to screenshots for multi-device disambiguation#6321eulicesl wants to merge 2 commits intoBasedHardware:mainfrom
Conversation
…uation Users running Omi on multiple Macs have no way to distinguish which machine captured a given screenshot. This stamps each screenshot with the local machine hostname (Host.current().localizedName) at capture time. Changes: - Add deviceName field to Screenshot model (nullable for backward compat) - Add GRDB migration to add deviceName column to screenshots table - Wire Host.current().localizedName into all 4 Screenshot creation sites - Include deviceName in ScreenActivitySyncService backend payload - Add deviceName to ChatPrompts column annotations for AI queries
eulicesl
force-pushed
the
feat/add-device-name-to-screenshots
branch
from
8baebcc to
ac9a668
Compare
Greptile SummaryThis PR adds a Confidence Score: 5/5Safe to merge — all findings are P2 suggestions with no blocking correctness issues. The migration is correctly ordered, nullable, and additive. Model and sync changes are consistent. Both findings are P2: one is a performance/style suggestion (cache desktop/Desktop/Sources/Rewind/Services/RewindIndexer.swift — minor concerns around Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Screen Frame Captured] --> B{Creation Path}
B --> C[processFrame variant 1]
B --> D[processFrame variant 2]
B --> E[processFrame variant 3]
B --> F[rebuildFromVideoFiles]
C --> G["deviceName: Host.current().localizedName"]
D --> G
E --> G
F --> G
G --> H[Screenshot struct\ndeviceName: String?]
H --> I[RewindDatabase\ninsertScreenshot]
I --> J[(screenshots table\ndeviceName TEXT nullable)]
J --> K{ScreenActivitySyncService\nsyncTick}
K -->|deviceName != nil| L[Sync payload\nincludes deviceName]
K -->|deviceName == nil\nlegacy rows| M[Sync payload\nomits deviceName]
L --> N[Backend API]
M --> N
Reviews (1): Last reviewed commit: "feat(rewind): add deviceName to screensh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ocrDataJson: nil, | ||
| isIndexed: false // Will need re-OCR | ||
| isIndexed: false, // Will need re-OCR | ||
| deviceName: Host.current().localizedName |
There was a problem hiding this comment.
Host.current() called on every screenshot instead of cached
Host.current().localizedName is called at all 4 Screenshot creation sites, performing a system-level lookup each time. Since the hostname never changes mid-run, caching this once at the actor level avoids the repeated overhead and makes the intent clearer.
// At the top of the actor body
private static let deviceName: String? = Host.current().localizedNameThen each call site becomes deviceName: RewindIndexer.deviceName. The same pattern applies to the other three call sites in this file.
There was a problem hiding this comment.
Latest commit 737ff7a updates this branch to cache the computer name once at actor scope instead of calling Host.current().localizedName at each screenshot construction site. The visible check on the current PR head is still green (Lint & Format Check).
| ocrDataJson: nil, | ||
| isIndexed: false // Will need re-OCR | ||
| isIndexed: false, // Will need re-OCR | ||
| deviceName: Host.current().localizedName |
There was a problem hiding this comment.
rebuildFromVideoFiles stamps historical frames with current machine's name
rebuildFromVideoFiles reconstructs database rows from video files that already exist on disk — potentially files that were recorded before the migration or even copied from another machine. Tagging them with the current machine's localizedName silently asserts provenance that isn't actually known. A user who migrates their Rewind database from Machine A to Machine B and then triggers a rebuild will see all historical frames attributed to Machine B.
A safer approach is to pass nil here (leave provenance unknown for historical frames) or document in a code comment that the assumption is "rebuild only runs on the originating machine".
There was a problem hiding this comment.
Latest commit 737ff7a changes rebuildFromVideoFiles to leave historical screenshot provenance unset (deviceName: nil) rather than stamping rebuilt rows with the current machine name.
There was a problem hiding this comment.
Pull request overview
Adds per-screenshot device attribution to support multi-Mac usage by persisting a device label in the local DB and including it in the backend sync payload.
Changes:
- Add nullable
deviceNameto theScreenshotmodel andscreenshotstable via GRDB migration. - Stamp
deviceNameat allScreenshot(...)creation sites inRewindIndexer. - Include
deviceNameinScreenActivitySyncServiceSQL selection + outbound sync payload, and annotate the column for chat/schema prompts.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| desktop/Desktop/Sources/ScreenActivitySyncService.swift | Selects deviceName from screenshots and conditionally includes it in the sync payload. |
| desktop/Desktop/Sources/Rewind/Services/RewindIndexer.swift | Stamps a device identifier into new screenshot rows at all creation points. |
| desktop/Desktop/Sources/Rewind/Core/RewindModels.swift | Adds deviceName: String? to the Screenshot record model and initializer. |
| desktop/Desktop/Sources/Rewind/Core/RewindDatabase.swift | Adds GRDB migration to add the deviceName column to screenshots. |
| desktop/Desktop/Sources/Chat/ChatPrompts.swift | Annotates deviceName in schema hints so chat tooling can query/filter by device. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ocrText: ocrText, | ||
| ocrDataJson: ocrDataJson, | ||
| isIndexed: isIndexed, | ||
| skippedForBattery: skippedForBattery | ||
| skippedForBattery: skippedForBattery, | ||
| deviceName: Host.current().localizedName |
There was a problem hiding this comment.
Host.current().localizedName is the user-facing “Computer Name” and is not necessarily the machine hostname. Either switch to a true hostname source (e.g., ProcessInfo.processInfo.hostName / Host.current().name) if the backend expects hostnames, or rename the field/docs/PR description to reflect that this is a localized device/computer name.
There was a problem hiding this comment.
Latest commit 737ff7a keeps Host.current().localizedName and renames the surrounding docs/schema text from hostname to computer/device name so the stored value matches the implementation.
| /// Whether OCR was skipped because the Mac was on battery (needs backfill when AC reconnects) | ||
| var skippedForBattery: Bool | ||
|
|
||
| /// Machine hostname that captured this screenshot (for multi-device disambiguation) |
There was a problem hiding this comment.
The docstring says this stores a “Machine hostname”, but the implementation is currently using Host.current().localizedName (computer name). Please update the comment/field description to match what’s actually stored, or change the stored value to a true hostname.
| /// Machine hostname that captured this screenshot (for multi-device disambiguation) | |
| /// Computer/device name that captured this screenshot (for multi-device disambiguation) |
There was a problem hiding this comment.
Latest commit 737ff7a updates the screenshot model comment to describe this field as a computer/device name rather than a hostname.
| "ocrText": "Full OCR-extracted text from the screen", | ||
| "focusStatus": "Whether user was focused or distracted (focused/distracted)", | ||
| "skippedForBattery": "OCR was skipped on battery; text may be missing", | ||
| "deviceName": "Machine hostname that captured this screenshot (for multi-device queries)", |
There was a problem hiding this comment.
Column annotation describes deviceName as a “Machine hostname”, but the code path is using Host.current().localizedName (computer name). Update this annotation (and any related docs) to avoid misleading prompt/schema guidance, or change the stored value to an actual hostname.
| "deviceName": "Machine hostname that captured this screenshot (for multi-device queries)", | |
| "deviceName": "Computer/device name that captured this screenshot (from the system display name, for multi-device queries)", |
There was a problem hiding this comment.
Latest commit 737ff7a updates the prompt/schema annotation to describe deviceName as the computer/device name captured from the system display name, not a hostname.
Summary
deviceNamefield to desktop screenshot records, stamped new screenshots withHost.current().localizedName, and includeddeviceNamein sync payloadsLinked Issue / Bounty
Root Cause (bug fixes only)
How It Works
RewindModels.swiftaddsdeviceName: String?toScreenshotfor backward-compatible persistence.RewindDatabase.swiftregisters an additive migration that adds the nullabledeviceNamecolumn.RewindIndexer.swiftpassesHost.current().localizedNameat each screenshot creation site, including rebuild paths.ScreenActivitySyncService.swiftincludesdeviceNamein the sync query and payload when present.ChatPrompts.swiftannotates the new column so chat-side querying can reason about device provenance.Change Type
Scope
Security Impact
Yes— risk and mitigation: N/ATesting
Screenshot(...)creation sites inRewindIndexer.swiftcreateLocalKnowledgeGraphHost.current().localizedNameis used at capture timeScreenActivitySyncServiceincludesdeviceNameonly when non-nilHuman Verification
deviceName = nil; sync payload includesdeviceNamewhen presentNULL; rebuild path also stamps the current machine name; additive migration remains nullableEvidence
Notes:
Docs
Performance, Privacy, and Reliability
NULLand current queries tolerate absent values.Migration / Backward Compatibility
deviceNametoscreenshots.NULL.deviceNameautomatically.Risks and Mitigations
NULL, and current-machine stamping is limited to rebuild-time reconstruction.Host.current()lookups are minor overhead in hot paths.AI Disclosure