Skip to content

feat(Frontlight): auto brightness and warmth - #590

Merged
OGKevin merged 1 commit into
masterfrom
ogkevin/push/tvmrqnxzvmro
Jun 15, 2026
Merged

feat(Frontlight): auto brightness and warmth#590
OGKevin merged 1 commit into
masterfrom
ogkevin/push/tvmrqnxzvmro

Conversation

@OGKevin

@OGKevin OGKevin commented Jun 13, 2026

Copy link
Copy Markdown
Owner

Change-Id: 64d89c204d8bed70af57523eb93546ad
Change-Id-Short: tvmrqnxzvmro

Closes: #486
Closes: CAD-78


Automatic Frontlight

Adds an automatic frontlight feature that adjusts warmth and brightness based on the sun's position at the device's location.

How it works

A background task runs every 5 minutes and emits an UpdateAutoFrontlight event. The main loop handles this event by computing new frontlight levels via compute_auto_frontlight_levels, which uses the sunrise crate to determine local sunrise and sunset times.

  • Warmth ramps from zero to full over a 1.5-hour window approaching sunset, stays at full through the night, then ramps back to zero over the 1.5-hour window before sunrise.
  • Brightness is left at the user's chosen level during the day and drops to the configured auto-frontlight-night-brightness after sunset.

Making a manual frontlight adjustment while the task is running stops the background task so the chosen level is preserved until the next app start.

Location

Coordinates are resolved from auto-frontlight-manual-coordinates if set, otherwise from auto-frontlight-last-coordinates, which is written automatically during each time sync. The geolocation fetch that was previously embedded in TimeManager has been extracted into a dedicated geolocation module and reused by the time sync task, which now forwards the discovered coordinates to the home view via AutoFrontlightCoordinates.

LightLevel type

intensity and warmth fields on LightLevels are now a newtype LightLevel wrapping f32 and clamped to 0.0–100.0. Conversion helpers (as_fraction, as_10_base, as_10_base_inverted, from_fraction) replace the ad-hoc arithmetic that was previously scattered across the frontlight backends. A turn_off default method on the Frontlight trait replaces repeated set_intensity(0.0); set_warmth(0.0) call sites.

Settings

Three new settings are introduced:

Key Default Description
auto-frontlight false Enable automatic adjustment
auto-frontlight-night-brightness 1.0 Brightness after sunset (0–100)
auto-frontlight-manual-coordinates unset Override coordinates [lat, lon]

auto-frontlight-last-coordinates is written automatically and should not be edited by hand.

All three user-facing settings are exposed in Settings → General and documented in a new auto-frontlight.md page.

@OGKevin OGKevin added this to the 0.11.0 milestone Jun 13, 2026
@OGKevin OGKevin self-assigned this Jun 13, 2026
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 13, 2026 18:16 — with GitHub Actions Inactive
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch from 1505f47 to 3015a4f Compare June 13, 2026 20:11
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 13, 2026 20:14 — with GitHub Actions Inactive
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch 2 times, most recently from 8d8c0e5 to 204d511 Compare June 14, 2026 10:04
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 14, 2026 10:07 — with GitHub Actions Inactive
Comment thread crates/core/src/geolocation.rs Outdated
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch from 204d511 to 2916e0d Compare June 14, 2026 10:26
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 14, 2026 10:30 — with GitHub Actions Inactive
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch from 2916e0d to 62bb996 Compare June 14, 2026 10:57
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch from 62bb996 to 44d7308 Compare June 14, 2026 10:58
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 14, 2026 11:02 — with GitHub Actions Inactive
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch from 44d7308 to 6e575a1 Compare June 14, 2026 13:49
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 14, 2026 13:51 — with GitHub Actions Inactive
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch from 6e575a1 to 2c235ac Compare June 14, 2026 13:56
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 14, 2026 13:59 — with GitHub Actions Inactive
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch from 2c235ac to 3489b8b Compare June 14, 2026 15:22
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 14, 2026 15:25 — with GitHub Actions Inactive
@OGKevin
OGKevin marked this pull request as ready for review June 14, 2026 15:51
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request Subsystem/Settings Editor The system that allows users to change app settings. labels Jun 14, 2026
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 6756549a-642d-48f8-b048-e1a75df27a14

📥 Commits

Reviewing files that changed from the base of the PR and between 3574fa1 and dac8e06.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (30)
  • contrib/Settings-sample.toml
  • crates/cadmus/src/app.rs
  • crates/core/Cargo.toml
  • crates/core/i18n/en-GB/cadmus_core.ftl
  • crates/core/src/context.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/frontlight/mod.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/geolocation.rs
  • crates/core/src/lib.rs
  • crates/core/src/settings/mod.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/task/time_sync.rs
  • crates/core/src/time_manager.rs
  • crates/core/src/view/frontlight.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/view/reader/mod.rs
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/emulator/src/main.rs
  • docs/src/SUMMARY.md
  • docs/src/auto-frontlight.md
  • docs/src/settings/index.md
✅ Files skipped from review due to trivial changes (3)
  • docs/src/SUMMARY.md
  • docs/src/auto-frontlight.md
  • docs/src/settings/index.md
🚧 Files skipped from review as they are similar to previous changes (25)
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/core/src/view/reader/mod.rs
  • crates/core/src/lib.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/i18n/en-GB/cadmus_core.ftl
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/task/time_sync.rs
  • crates/core/src/settings/mod.rs
  • crates/core/Cargo.toml
  • contrib/Settings-sample.toml
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/context.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/emulator/src/main.rs
  • crates/cadmus/src/app.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/geolocation.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/view/frontlight.rs
  • crates/core/src/frontlight/mod.rs
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (4)
crates/core/**/*.rs

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

crates/core/**/*.rs: Rustdoc examples should follow this preference order: fully compilable > no_run > ignore. Use no_run when the example compiles but cannot execute (file I/O, network, database). Use ignore only for private/pub(crate) items unreachable from the test harness with a comment explaining why. Use # to hide boilerplate setup lines. Verify with cargo test --doc.
Use typed SQL macros (sqlx::query!, sqlx::query_as!, sqlx::query_scalar!) in Rust code. Never use untyped sqlx::query() / sqlx::query_as() / sqlx::query_scalar(). Use .flatten() on query_scalar! results for nullable columns (Option<Option<T>>Option<T>).
Untyped SQL queries are allowed only for dynamic SQL (e.g. runtime ORDER BY column) if and only if the function has unit tests covering every dynamic path, with a comment explaining why the typed macro cannot be used.
All user-visible strings must use the fl! macro (use crate::fl;). Never hardcode string literals for labels, buttons, placeholders, or notifications. User-visible strings include: label() implementations on SettingKind traits, input field label strings in Event::OpenNamedInput, menu entry text (EntryKind::Command, EntryKind::RadioButton, etc.), and button labels, notification text.

Files:

  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/time_manager.rs
crates/core/**/*.{sql,rs}

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

All SQL queries must list explicit column names. Do not use SELECT *.

Files:

  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/time_manager.rs
crates/core/**/*.{ftl,rs}

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

Every user-visible string needs a Fluent message ID in crates/core/i18n/en-GB/cadmus_core.ftl. Use fl!("message-id") at the call site. For parameterised strings, use Fluent variables ({ $var }) in the .ftl file and pass values via fl!("id", var = value).

Files:

  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/time_manager.rs
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Prefer ? operator over unwrap() / expect() in library and app code
Use thiserror for custom error types and anyhow for ad-hoc errors in Rust
Use iterators over index-based loops in Rust
Use &str over String in function parameters when ownership is not needed
Prefer borrowing over cloning in Rust
Inline expressions directly into struct fields — avoid intermediate bindings solely to pass them into a struct literal
Avoid unsafe unless required and documented in Rust
Avoid premature collect() — keep iterators lazy in Rust
Ensure Rust code compiles without warnings
Prefer newtype wrappers over raw primitives for domain concepts (e.g., Fingerprint(String) instead of bare String)
Wrap String, i64, and similar primitives when the value represents a specific domain concept (fingerprints, file kinds, identifiers, etc.)
Implement Display, FromStr, and other standard traits on newtype wrappers to keep them ergonomic
Match on enum variants instead of string comparisons — the compiler catches missing arms
Comment why, not what. Avoid inline comments — extract code into well-named, documented functions instead
Avoid dead-code comments (commented-out code) in Rust
Avoid changelog comments (Modified by X on date) in Rust
Avoid divider comments (//=====) in Rust
Annotations (TODO, FIXME, HACK, NOTE) are acceptable with context in Rust
Use structured fields with the tracing crate — never use string formatting for log data
Use only structured fields for logging — data goes in fields, not format strings
Do not use module prefixes in logs — instrumentation scope provides context
Do not mix structured fields with format arguments in logging
Use debug log level for development and troubleshooting detail
Use info log level for important runtime events
Use warn log level for recoverable issues (retries, fallbacks)
Use error log level for failures requiring attention
Return domain types directly from sqlx queries instead of parsin...

Files:

  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/time_manager.rs
🧠 Learnings (1)
📚 Learning: 2026-05-23T14:57:43.820Z
Learnt from: OGKevin
Repo: OGKevin/cadmus PR: 478
File: crates/core/src/view/settings_editor/refresh_rate_by_kind_editor.rs:91-91
Timestamp: 2026-05-23T14:57:43.820Z
Learning: When reviewing Rust code that calls `ToggleableKeyboard::new(parent_rect: Rectangle, number_mode: bool)` (from `crates/core/src/view/toggleable_keyboard.rs`), treat the second argument as `number_mode` (numeric/number-layout mode), not as an initial visibility flag. In the constructor implementation, `visible` is initialized to `false` and the keyboard starts hidden; only the numeric layout changes based on `number_mode` (e.g., for numeric inputs like `refresh rate u8`). Therefore, do not flag `ToggleableKeyboard::new(rect, true)` as “making the keyboard start visible.”

Applied to files:

  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/time_manager.rs
🔇 Additional comments (2)
crates/core/src/time_manager.rs (1)

10-11: LGTM!

Also applies to: 27-33, 82-95

crates/core/src/task/auto_frontlight.rs (1)

1-32: LGTM!


Walkthrough

This PR implements automatic frontlight warmth and brightness adjustment based on the sun's position. It introduces a LightLevel newtype wrapping f32 to represent light intensity/warmth with explicit conversions and comparisons, migrating all frontlight hardware implementations to use it. A new geolocation module fetches coordinates and timezone from ipapi.co, while compute_auto_frontlight_levels calculates intensity (night vs. day) and warmth (timed ramp around sunrise/sunset with midnight wrap-around). A periodic AutoFrontlightTask fires Event::UpdateAutoFrontlight every 5 minutes. The time sync pipeline is updated to fetch geolocation once and share it across timezone detection and frontlight coordinate storage. New settings fields, UI setting kinds, events, and comprehensive documentation are wired throughout, with all frontlight state changes routed through a centralized set_frontlight helper.

Sequence Diagram

sequenceDiagram
  participant TimeSyncTask
  participant geolocation as geolocation module
  participant TimeManager
  participant Home
  participant AutoFrontlightTask
  participant app as app.rs event loop
  participant Frontlight as Frontlight hardware

  rect rgba(100, 149, 237, 0.5)
    Note over TimeSyncTask,Home: Time sync with shared geolocation fetch
    TimeSyncTask->>geolocation: fetch_geolocation(client)
    geolocation-->>TimeSyncTask: GeoLocation { coords, timezone }
    TimeSyncTask->>TimeManager: sync(ntp_host, manual, Some(geo))
    TimeSyncTask->>Home: Event::AutoFrontlightCoordinates(coords)
    Home->>Home: store auto_frontlight_last_coordinates
    Home->>app: Event::AutoFrontlightConfigChanged
  end

  rect rgba(60, 179, 113, 0.5)
    Note over AutoFrontlightTask,Frontlight: Periodic auto-frontlight update (every 5 min)
    AutoFrontlightTask->>app: Event::UpdateAutoFrontlight
    app->>app: resolve_coordinates(settings)
    app->>app: compute_auto_frontlight_levels(now, coords, night_brightness, intensity)
    app->>app: SetFrontlightLevels(levels)
    app->>Frontlight: set_intensity(levels.intensity)
    app->>Frontlight: set_warmth(levels.warmth)
  end
Loading

Possibly Related PRs

  • OGKevin/cadmus#576: Overlaps in crates/core/src/time_manager.rs where both PRs change the time synchronization flow to accept and use geolocation data.

Suggested labels

documentation, dependencies

Poem

☀️ When the sun dips low and stars appear,
The screen grows warm and dims, my dear.
From ipapi.co the coords flow,
A LightLevel wraps the golden glow.
Every five minutes, soft and bright —
Auto frontlight fades into night. 🌙

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title follows Conventional Commits format with 'feat:' prefix and clearly describes the main feature being introduced: automatic brightness adjustment.
Linked Issues check ✅ Passed The PR comprehensively addresses all objectives from issue #486: implements automatic brightness/warmth based on sunrise/sunset times using the sunrise crate, supports coordinate detection via time sync and manual override, and extracts geolocation into a reusable module.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the automatic frontlight feature and supporting infrastructure. No extraneous or unrelated modifications are present.
Docstring Coverage ✅ Passed Docstring coverage is 82.01% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Nitpick comments (3)
crates/core/src/frontlight/standard.rs (1)

25-27: ⚡ Quick win

Document the ioctl safety contract.

This unsafe call may be required, but the block still needs a short SAFETY: note explaining why self.interface.as_raw_fd() and the request code satisfy the kernel contract.

As per coding guidelines: Avoid unsafe unless required and documented in Rust.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/core/src/frontlight/standard.rs` around lines 25 - 27, The unsafe
block containing the write_frontlight_intensity call lacks a SAFETY
documentation comment explaining the safety contract. Add a SAFETY comment
immediately before the unsafe block that documents why
self.interface.as_raw_fd() provides a valid file descriptor and why the request
code passed to write_frontlight_intensity satisfies the kernel ioctl contract
requirements. The comment should be brief but specific about what makes this
unsafe operation sound.

Source: Coding guidelines

crates/core/src/view/settings_editor/kinds/general.rs (1)

953-1029: ⚡ Quick win

Add tests for AutoFrontlightConfigChanged bus dispatch.

These tests cover parsing well, but they don’t assert that handle(...) enqueues Event::AutoFrontlightConfigChanged, which is the key integration signal for task resync.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/core/src/view/settings_editor/kinds/general.rs` around lines 953 -
1029, The tests in the auto_frontlight module verify that apply_text correctly
parses input and updates settings, but they are missing assertions that verify
the critical integration behavior: that handle(...) properly enqueues
Event::AutoFrontlightConfigChanged to the bus when settings are modified.
Enhance each test (brightness_apply_text_parses_and_updates,
brightness_apply_text_ignores_invalid_input,
manual_coordinates_apply_text_parses_and_updates,
manual_coordinates_apply_text_clears_on_empty_input, and
manual_coordinates_apply_text_ignores_invalid_input) by adding assertions that
confirm Event::AutoFrontlightConfigChanged is enqueued on the bus as the key
signal for task resync.
crates/core/src/view/settings_editor/category_editor.rs (1)

800-801: ⚡ Quick win

Add a regression test for the new auto-frontlight input close paths.

This match is a hand-maintained allowlist of closeable overlays. A focused test that opens AutoFrontlightBrightnessInput and AutoFrontlightManualCoordinatesInput, then asserts Event::Close(...) removes the child and clears focus, would make future ViewId churn a lot safer.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/core/src/view/settings_editor/category_editor.rs` around lines 800 -
801, A regression test is needed to ensure the newly added closeable overlay
ViewIds (AutoFrontlightBrightnessInput and AutoFrontlightManualCoordinatesInput)
in the allowlist match statement are properly handled when receiving a Close
event. Create a focused test that opens each of these overlay views, sends an
Event::Close, and then asserts that the child is removed from the view hierarchy
and focus is cleared. This test will prevent future regressions when ViewId
definitions are modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@contrib/Settings-sample.toml`:
- Around line 20-21: The comment for auto-frontlight-night-brightness states the
range is 0.0 to 1.0, but this setting is backed by LightLevel which uses a 0.0
to 100.0 scale, creating misleading documentation that can result in much dimmer
values than intended. Update the comment to correctly state the range as 0.0 to
100.0 instead of 0.0 to 1.0, and adjust the example value from 0.10 to an
appropriate value on the 0-100 scale (such as 10 for 10% brightness).

In `@crates/cadmus/src/app.rs`:
- Around line 1313-1353: The Event::UpdateAutoFrontlight handler sends
SetFrontlightLevels which unconditionally stops the AutoFrontlight task, causing
the periodic auto-update task to stop after its first execution. Additionally,
UpdateAutoFrontlight ignores the context.settings.frontlight on/off flag,
potentially re-enabling the light after the user has disabled it. Fix this by
applying the frontlight levels directly in the UpdateAutoFrontlight handler
instead of routing through SetFrontlightLevels, checking
context.settings.frontlight before applying the levels, and keeping the
task-stop logic only in the SetFrontlightLevels handler for manual adjustments.

In `@crates/core/i18n/en-GB/cadmus_core.ftl`:
- Around line 53-57: The settings-general-auto-frontlight and related keys
(settings-general-auto-frontlight-brightness,
settings-general-auto-frontlight-brightness-input,
settings-general-auto-frontlight-manual-coordinates, and
settings-general-auto-frontlight-manual-coordinates-input) are currently placed
after settings-general-auto-time, breaking alphabetical ordering. Move these
five auto-frontlight keys to their correct alphabetical position before the
auto-time keys, since "auto-frontlight" comes alphabetically before "auto-time"
in the Settings - General section.

In `@crates/core/src/frontlight/auto.rs`:
- Around line 109-125: The make_dt() function treats its input strings as UTC
and converts to Local timezone, while compute_expected_sun_times() returns raw
UTC minutes without timezone conversion, causing a mismatch in test assertions
on non-UTC systems. Fix this by aligning both helpers to work end-to-end in the
same timezone: either modify compute_expected_sun_times() to return local
wall-clock minutes instead of raw UTC minutes by applying the same timezone
conversion logic used in make_dt(), or alternatively modify make_dt() to treat
input strings as local time directly without the UTC intermediate step. Ensure
whichever approach you choose is used consistently throughout the test logic so
that the times passed to make_dt() match what compute_expected_sun_times()
returns.

In `@crates/core/src/frontlight/mod.rs`:
- Around line 100-103: The from_fraction() method in the LightLevel newtype
directly multiplies the input without validation, allowing out-of-range values
when the input is outside [0.0, 1.0] or due to floating-point precision errors.
Fix this by computing the fraction times 100 and then passing the result through
the LightLevel constructor (likely via From<f32> or Self::new) to ensure the
value is properly clamped to the valid 0-100 range, maintaining the newtype
invariant.

In `@crates/core/src/frontlight/premixed.rs`:
- Around line 46-58: Change the Frontlight trait signature to return Result<(),
Error> instead of () for the set_intensity and set_warmth methods (anchor site
in crates/core/src/frontlight/premixed.rs lines 46-58). Replace the unwrap()
call in set_intensity and the expect() calls in set_warmth with the ? operator
to propagate errors. Apply the same Result return type change to the
corresponding methods in the sibling NaturalFrontlight implementation
(crates/core/src/frontlight/natural.rs lines 101-112) and StandardFrontlight.
Update all callers of these methods in reader/mod.rs and context.rs to handle
the returned Result values using ? or explicit error handling. This approach
ensures sysfs write failures are propagated as recoverable errors rather than
panicking the application.

In `@crates/core/src/task/auto_frontlight.rs`:
- Line 21: The code at line 21 uses `.ok()` on the hub.send call, which silently
swallows any SendError if the receiver is no longer available, hiding a broken
event path and making debugging difficult. Replace the `.ok()` call with
explicit error handling for the send operation on Event::UpdateAutoFrontlight;
either log the error when the channel send fails or handle it appropriately so
that broken event paths are visible and not silently ignored.

In `@crates/core/src/task/time_sync.rs`:
- Line 44: Fix the typo in the structured log field name in the tracing::error!
call where the geolocation fetch fails. Change the field key from `erro` to
`error` in the macro call to ensure that log filtering and aggregation systems
can properly match this error field with the standard error field name.
- Around line 31-49: The NTP time synchronization is currently skipped when
geolocation fetch or HTTP client creation fails, which silently disables
auto-time updates on transient failures. Restructure the code so that
TimeManager::sync is always called regardless of geolocation success. Create the
HTTP client first, then attempt to fetch geolocation, but ensure that sync is
invoked with either Some(geo) if geolocation succeeds or None if it fails. Keep
the hub.send(Event::AutoFrontlightCoordinates) call conditional on geolocation
success, but make sure the sync call at the TimeManager::sync invocation always
executes when the client is available.

In `@crates/core/src/view/frontlight.rs`:
- Around line 253-263: The set_frontlight_levels method only updates the slider
UI but slider handlers still merge changes against the stale
context.frontlight.levels(), causing consecutive interactions to compose against
old state. Add a field to FrontlightWindow to cache the current LightLevels as a
local source of truth, update this cache whenever set_frontlight_levels is
called, and modify the slider event handlers (which appear to be in the ranges
around lines 287-296 and 365-380) to merge against this cached value instead of
context.frontlight.levels() until the apply event is processed.

---

Nitpick comments:
In `@crates/core/src/frontlight/standard.rs`:
- Around line 25-27: The unsafe block containing the write_frontlight_intensity
call lacks a SAFETY documentation comment explaining the safety contract. Add a
SAFETY comment immediately before the unsafe block that documents why
self.interface.as_raw_fd() provides a valid file descriptor and why the request
code passed to write_frontlight_intensity satisfies the kernel ioctl contract
requirements. The comment should be brief but specific about what makes this
unsafe operation sound.

In `@crates/core/src/view/settings_editor/category_editor.rs`:
- Around line 800-801: A regression test is needed to ensure the newly added
closeable overlay ViewIds (AutoFrontlightBrightnessInput and
AutoFrontlightManualCoordinatesInput) in the allowlist match statement are
properly handled when receiving a Close event. Create a focused test that opens
each of these overlay views, sends an Event::Close, and then asserts that the
child is removed from the view hierarchy and focus is cleared. This test will
prevent future regressions when ViewId definitions are modified.

In `@crates/core/src/view/settings_editor/kinds/general.rs`:
- Around line 953-1029: The tests in the auto_frontlight module verify that
apply_text correctly parses input and updates settings, but they are missing
assertions that verify the critical integration behavior: that handle(...)
properly enqueues Event::AutoFrontlightConfigChanged to the bus when settings
are modified. Enhance each test (brightness_apply_text_parses_and_updates,
brightness_apply_text_ignores_invalid_input,
manual_coordinates_apply_text_parses_and_updates,
manual_coordinates_apply_text_clears_on_empty_input, and
manual_coordinates_apply_text_ignores_invalid_input) by adding assertions that
confirm Event::AutoFrontlightConfigChanged is enqueued on the bus as the key
signal for task resync.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 1165f537-072d-4136-b4a2-9e1e2f3965d5

📥 Commits

Reviewing files that changed from the base of the PR and between 11a1ee6 and 3489b8b.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (29)
  • contrib/Settings-sample.toml
  • crates/cadmus/src/app.rs
  • crates/core/Cargo.toml
  • crates/core/i18n/en-GB/cadmus_core.ftl
  • crates/core/src/context.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/frontlight/mod.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/geolocation.rs
  • crates/core/src/lib.rs
  • crates/core/src/settings/mod.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/task/time_sync.rs
  • crates/core/src/time_manager.rs
  • crates/core/src/view/frontlight.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/emulator/src/main.rs
  • docs/src/SUMMARY.md
  • docs/src/auto-frontlight.md
  • docs/src/settings/index.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
docs/src/**/*.md

📄 CodeRabbit inference engine (docs/AGENTS.md)

docs/src/**/*.md: Use conversational and friendly tone in user-facing documentation
Use active voice in user-facing documentation (e.g., 'Copy the file' instead of 'The file should be copied')
Avoid jargon in user-facing documentation: replace 'artifact' with 'file' or 'package', 'deploy' with 'install' or 'download', 'configure' with 'set up', 'bundle' with 'package', 'on-device' with 'on your Kobo' or 'wirelessly'
Use standardized admonition syntax for documentation: [!NOTE], [!TIP], [!IMPORTANT], [!WARNING], [!CAUTION]

Files:

  • docs/src/auto-frontlight.md
  • docs/src/SUMMARY.md
  • docs/src/settings/index.md
docs/**/*.md

📄 CodeRabbit inference engine (docs/AGENTS.md)

docs/**/*.md: Format Markdown with Prettier
Ensure Markdown passes markdownlint
Use code blocks with language tags in Markdown

Files:

  • docs/src/auto-frontlight.md
  • docs/src/SUMMARY.md
  • docs/src/settings/index.md
crates/core/**/*.rs

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

crates/core/**/*.rs: Rustdoc examples should follow this preference order: fully compilable > no_run > ignore. Use no_run when the example compiles but cannot execute (file I/O, network, database). Use ignore only for private/pub(crate) items unreachable from the test harness with a comment explaining why. Use # to hide boilerplate setup lines. Verify with cargo test --doc.
Use typed SQL macros (sqlx::query!, sqlx::query_as!, sqlx::query_scalar!) in Rust code. Never use untyped sqlx::query() / sqlx::query_as() / sqlx::query_scalar(). Use .flatten() on query_scalar! results for nullable columns (Option<Option<T>>Option<T>).
Untyped SQL queries are allowed only for dynamic SQL (e.g. runtime ORDER BY column) if and only if the function has unit tests covering every dynamic path, with a comment explaining why the typed macro cannot be used.
All user-visible strings must use the fl! macro (use crate::fl;). Never hardcode string literals for labels, buttons, placeholders, or notifications. User-visible strings include: label() implementations on SettingKind traits, input field label strings in Event::OpenNamedInput, menu entry text (EntryKind::Command, EntryKind::RadioButton, etc.), and button labels, notification text.

Files:

  • crates/core/src/task/time_sync.rs
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/core/src/lib.rs
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/src/time_manager.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/geolocation.rs
  • crates/core/src/context.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/core/src/settings/mod.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/frontlight/mod.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/view/frontlight.rs
crates/core/**/*.{sql,rs}

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

All SQL queries must list explicit column names. Do not use SELECT *.

Files:

  • crates/core/src/task/time_sync.rs
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/core/src/lib.rs
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/src/time_manager.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/geolocation.rs
  • crates/core/src/context.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/core/src/settings/mod.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/frontlight/mod.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/view/frontlight.rs
crates/core/**/*.{ftl,rs}

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

Every user-visible string needs a Fluent message ID in crates/core/i18n/en-GB/cadmus_core.ftl. Use fl!("message-id") at the call site. For parameterised strings, use Fluent variables ({ $var }) in the .ftl file and pass values via fl!("id", var = value).

Files:

  • crates/core/src/task/time_sync.rs
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/core/src/lib.rs
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/i18n/en-GB/cadmus_core.ftl
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/src/time_manager.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/geolocation.rs
  • crates/core/src/context.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/core/src/settings/mod.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/frontlight/mod.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/view/frontlight.rs
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Prefer ? operator over unwrap() / expect() in library and app code
Use thiserror for custom error types and anyhow for ad-hoc errors in Rust
Use iterators over index-based loops in Rust
Use &str over String in function parameters when ownership is not needed
Prefer borrowing over cloning in Rust
Inline expressions directly into struct fields — avoid intermediate bindings solely to pass them into a struct literal
Avoid unsafe unless required and documented in Rust
Avoid premature collect() — keep iterators lazy in Rust
Ensure Rust code compiles without warnings
Prefer newtype wrappers over raw primitives for domain concepts (e.g., Fingerprint(String) instead of bare String)
Wrap String, i64, and similar primitives when the value represents a specific domain concept (fingerprints, file kinds, identifiers, etc.)
Implement Display, FromStr, and other standard traits on newtype wrappers to keep them ergonomic
Match on enum variants instead of string comparisons — the compiler catches missing arms
Comment why, not what. Avoid inline comments — extract code into well-named, documented functions instead
Avoid dead-code comments (commented-out code) in Rust
Avoid changelog comments (Modified by X on date) in Rust
Avoid divider comments (//=====) in Rust
Annotations (TODO, FIXME, HACK, NOTE) are acceptable with context in Rust
Use structured fields with the tracing crate — never use string formatting for log data
Use only structured fields for logging — data goes in fields, not format strings
Do not use module prefixes in logs — instrumentation scope provides context
Do not mix structured fields with format arguments in logging
Use debug log level for development and troubleshooting detail
Use info log level for important runtime events
Use warn log level for recoverable issues (retries, fallbacks)
Use error log level for failures requiring attention
Return domain types directly from sqlx queries instead of parsin...

Files:

  • crates/core/src/task/time_sync.rs
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/core/src/lib.rs
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/emulator/src/main.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/src/time_manager.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/geolocation.rs
  • crates/core/src/context.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/core/src/settings/mod.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/frontlight/mod.rs
  • crates/cadmus/src/app.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/view/frontlight.rs
**/Cargo.toml

📄 CodeRabbit inference engine (AGENTS.md)

**/Cargo.toml: Use caret requirements for most dependencies in Cargo.toml
Alphabetize dependencies within logical groups in Cargo.toml
Use default-features = false when fine-grained control is needed in Cargo.toml
Keep related crate families at compatible versions (e.g. all opentelemetry-* crates) in Cargo.toml

Files:

  • crates/core/Cargo.toml
crates/core/**/*.ftl

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

crates/core/**/*.ftl: Keep Fluent message keys (.ftl file keys) sorted alphabetically within each comment section.
Fluent message key naming convention: use kebab-case, prefixed by feature area. Use settings-<category>-<description> for settings labels, settings-<category>-<description>-input for input fields, and notification-<description> for notifications.

Files:

  • crates/core/i18n/en-GB/cadmus_core.ftl
🧠 Learnings (2)
📚 Learning: 2026-05-31T07:53:12.579Z
Learnt from: OGKevin
Repo: OGKevin/cadmus PR: 553
File: docs/src/installation/migration.md:87-88
Timestamp: 2026-05-31T07:53:12.579Z
Learning: For markdown files under docs/src/ in the cadmus (OGKevin/cadmus) repo, validate mdBook admonition syntax using the allowed types: NOTE, TIP, WARNING, IMPORTANT, and CAUTION. The correct format is a blockquote prefix like `> [!NOTE]`, `> [!TIP]`, `> [!WARNING]`, `> [!IMPORTANT]`, or `> [!CAUTION]`. Do not flag `[!CAUTION]` or `[!IMPORTANT]` as non-standard in this repo.

Applied to files:

  • docs/src/auto-frontlight.md
  • docs/src/SUMMARY.md
  • docs/src/settings/index.md
📚 Learning: 2026-05-23T14:57:43.820Z
Learnt from: OGKevin
Repo: OGKevin/cadmus PR: 478
File: crates/core/src/view/settings_editor/refresh_rate_by_kind_editor.rs:91-91
Timestamp: 2026-05-23T14:57:43.820Z
Learning: When reviewing Rust code that calls `ToggleableKeyboard::new(parent_rect: Rectangle, number_mode: bool)` (from `crates/core/src/view/toggleable_keyboard.rs`), treat the second argument as `number_mode` (numeric/number-layout mode), not as an initial visibility flag. In the constructor implementation, `visible` is initialized to `false` and the keyboard starts hidden; only the numeric layout changes based on `number_mode` (e.g., for numeric inputs like `refresh rate u8`). Therefore, do not flag `ToggleableKeyboard::new(rect, true)` as “making the keyboard start visible.”

Applied to files:

  • crates/core/src/task/time_sync.rs
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/core/src/lib.rs
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/src/time_manager.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/geolocation.rs
  • crates/core/src/context.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/core/src/settings/mod.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/frontlight/mod.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/view/frontlight.rs
🪛 GitHub Actions: PR `#590` / 2_Analyze (rust).txt
crates/cadmus/src/app.rs

[warning] 124-206: CodeQL (Rust autobuild/indexing) reported "macro expansion failed" warnings for macros including 'format_err', 'debug', and 'tracing::error'.

🪛 GitHub Actions: PR `#590` / Analyze (rust)
crates/cadmus/src/app.rs

[warning] 124-206: CodeQL rust autobuild WARN: macro expansion failed for multiple macros (e.g., format_err, debug, tracing::error) during extraction.

🔇 Additional comments (16)
docs/src/SUMMARY.md (1)

20-20: LGTM!

docs/src/auto-frontlight.md (2)

8-10: 💤 Low value

Verify timing of the first automatic adjustment.

Line 9 states "The first adjustment happens as soon as the feature starts, and then repeats on every 5-minute mark." The provided context confirms the task runs every 5 minutes, but the AutoFrontlightTask code itself isn't included in this review. Can you confirm that the initial adjustment truly fires immediately when the feature is activated, rather than waiting for the next 5-minute interval?


1-81: Documentation is well-written, accurate, and properly formatted.

Feature explanation is clear and conversational; all technical claims (5-minute cycle, 1.5-hour ramp windows, brightness/warmth behavior, ipapi.co location source, coordinate priority, default night brightness) align with the implementation details. Cross-references to the settings guide and time-sync guide are in place. Admonition syntax is correct per the repo's standard blockquote format.

Source: Coding guidelines

docs/src/settings/index.md (1)

62-110: Settings documentation is accurate, well-organized, and consistent with implementation.

All four auto-frontlight settings (toggle, night brightness, manual coordinates, last coordinates) are documented with correct indicators, clear descriptions, accurate default behavior, proper ranges, and corresponding TOML examples. The priority rule (manual > auto-detected) and auto-management semantics are clearly explained. Integration with the feature documentation (cross-link at line 31 of auto-frontlight.md) is in place.

Source: Coding guidelines

crates/core/src/settings/mod.rs (1)

7-8: LGTM!

Also applies to: 261-282, 1023-1026, 1055-1063

crates/core/src/time_manager.rs (1)

27-35: LGTM!

Also applies to: 82-94

crates/core/src/frontlight/auto.rs (1)

29-39: No changes needed—sunrise::SolarDay::event_time() returns DateTime<Utc> directly, not Option. The code correctly calls .with_timezone() on the result.

			> Likely an incorrect or invalid review comment.
crates/emulator/src/main.rs (1)

387-387: LGTM!

crates/core/src/view/settings_editor/kinds/mod.rs (1)

40-41: LGTM!

crates/core/src/view/settings_editor/kinds/identity.rs (1)

24-26: LGTM!

crates/core/src/view/settings_editor/kinds/general.rs (1)

422-654: LGTM!

crates/core/src/task/mod.rs (1)

39-40: LGTM!

Also applies to: 103-105, 129-130, 285-286, 351-352, 449-452, 539-559, 673-683

crates/core/src/view/mod.rs (1)

67-67: LGTM!

Also applies to: 491-493, 512-513, 646-647, 833-834

crates/core/src/context.rs (1)

45-45: LGTM!

Also applies to: 213-219, 223-247

crates/cadmus/src/app.rs (1)

201-201: LGTM!

Also applies to: 366-367, 721-721, 1094-1097

crates/core/src/view/home/mod.rs (1)

2366-2376: LGTM!

Comment thread contrib/Settings-sample.toml Outdated
Comment thread crates/cadmus/src/app.rs
Comment thread crates/core/i18n/en-GB/cadmus_core.ftl
Comment thread crates/core/src/frontlight/auto.rs
Comment thread crates/core/src/frontlight/mod.rs
Comment thread crates/core/src/frontlight/premixed.rs Outdated
Comment thread crates/core/src/task/auto_frontlight.rs Outdated
Comment thread crates/core/src/task/time_sync.rs Outdated
Comment thread crates/core/src/task/time_sync.rs Outdated
Comment thread crates/core/src/view/frontlight.rs
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch from 3489b8b to 3574fa1 Compare June 14, 2026 17:26
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 14, 2026 17:28 — with GitHub Actions Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
crates/core/i18n/en-GB/cadmus_core.ftl (1)

52-56: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Alphabetical ordering violation: move auto-frontlight keys before auto-suspend.

The five settings-general-auto-frontlight* keys (lines 52-56) are currently placed after auto-suspend and before auto-time, but alphabetically "frontlight" comes before "suspend". Move all five keys to appear immediately after auto-power-off-input (line 49) and before auto-suspend (line 50).

📝 Suggested reorder
 settings-general-auto-power-off = Auto Power Off (days)
 settings-general-auto-power-off-input = Auto Power Off (days, 0 = never)
+settings-general-auto-frontlight = Automatic Frontlight
+settings-general-auto-frontlight-brightness = Auto Night Brightness
+settings-general-auto-frontlight-brightness-input = Night Brightness (% 0-100)
+settings-general-auto-frontlight-manual-coordinates = Manual Coordinates
+settings-general-auto-frontlight-manual-coordinates-input = lat,lon | coordinates
 settings-general-auto-suspend = Auto Suspend (minutes)
 settings-general-auto-suspend-input = Auto Suspend (minutes, 0 = never)
-settings-general-auto-frontlight = Automatic Frontlight
-settings-general-auto-frontlight-brightness = Auto Night Brightness
-settings-general-auto-frontlight-brightness-input = Night Brightness (% 0-100)
-settings-general-auto-frontlight-manual-coordinates = Manual Coordinates
-settings-general-auto-frontlight-manual-coordinates-input = lat,lon | coordinates
 settings-general-auto-time = Automatic Time Sync
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/core/i18n/en-GB/cadmus_core.ftl` around lines 52 - 56, The five
settings-general-auto-frontlight* keys (settings-general-auto-frontlight,
settings-general-auto-frontlight-brightness,
settings-general-auto-frontlight-brightness-input,
settings-general-auto-frontlight-manual-coordinates, and
settings-general-auto-frontlight-manual-coordinates-input) are currently placed
after auto-suspend, but alphabetically "frontlight" should come before
"suspend". Move all five auto-frontlight keys to appear immediately after the
auto-power-off-input key and before the auto-suspend key to restore proper
alphabetical ordering.

Source: Coding guidelines

🧹 Nitpick comments (1)
crates/core/src/view/reader/mod.rs (1)

4036-4039: ⚡ Quick win

Use the imported error! macro instead of tracing::error!.

Line 4036 and Line 4039 should use error!(...) (already imported) to match the project logging style rule and keep instrumentation context clean.

Suggested patch
-                                    if let Err(error) = context.frontlight.set_intensity(intensity) {
-                                        tracing::error!(error = %error, "failed to set frontlight intensity");
+                                    if let Err(error) = context.frontlight.set_intensity(intensity) {
+                                        error!(error = %error, "failed to set frontlight intensity");
                                     }
                                     if let Err(error) = context.frontlight.set_warmth(warmth) {
-                                        tracing::error!(error = %error, "failed to set frontlight warmth");
+                                        error!(error = %error, "failed to set frontlight warmth");
                                     }

As per coding guidelines, “Do not use module prefixes in logs — instrumentation scope provides context.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/core/src/view/reader/mod.rs` around lines 4036 - 4039, Replace the
module-prefixed `tracing::error!` calls with the imported `error!` macro in the
frontlight intensity and warmth error logging statements. The macro is already
imported in the file, so remove the `tracing::` prefix from both error!
invocations that log "failed to set frontlight intensity" and "failed to set
frontlight warmth" to match the project's logging style guidelines and keep
instrumentation context clean.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/core/src/task/auto_frontlight.rs`:
- Around line 21-23: When hub.send(Event::UpdateAutoFrontlight) fails with an
error, the loop continues indefinitely trying to resend the event even though
the receiver is permanently gone and the channel won't recover. Add a break
statement after logging the error in the error handling block to exit the loop
cleanly when the send fails, allowing the task manager to observe and handle the
failure instead of wasting CPU cycles on futile retries.

---

Duplicate comments:
In `@crates/core/i18n/en-GB/cadmus_core.ftl`:
- Around line 52-56: The five settings-general-auto-frontlight* keys
(settings-general-auto-frontlight, settings-general-auto-frontlight-brightness,
settings-general-auto-frontlight-brightness-input,
settings-general-auto-frontlight-manual-coordinates, and
settings-general-auto-frontlight-manual-coordinates-input) are currently placed
after auto-suspend, but alphabetically "frontlight" should come before
"suspend". Move all five auto-frontlight keys to appear immediately after the
auto-power-off-input key and before the auto-suspend key to restore proper
alphabetical ordering.

---

Nitpick comments:
In `@crates/core/src/view/reader/mod.rs`:
- Around line 4036-4039: Replace the module-prefixed `tracing::error!` calls
with the imported `error!` macro in the frontlight intensity and warmth error
logging statements. The macro is already imported in the file, so remove the
`tracing::` prefix from both error! invocations that log "failed to set
frontlight intensity" and "failed to set frontlight warmth" to match the
project's logging style guidelines and keep instrumentation context clean.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 4e68a0c3-fb88-4a34-9602-43653f0210dc

📥 Commits

Reviewing files that changed from the base of the PR and between 3489b8b and 3574fa1.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (30)
  • contrib/Settings-sample.toml
  • crates/cadmus/src/app.rs
  • crates/core/Cargo.toml
  • crates/core/i18n/en-GB/cadmus_core.ftl
  • crates/core/src/context.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/frontlight/mod.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/geolocation.rs
  • crates/core/src/lib.rs
  • crates/core/src/settings/mod.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/task/time_sync.rs
  • crates/core/src/time_manager.rs
  • crates/core/src/view/frontlight.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/view/reader/mod.rs
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/emulator/src/main.rs
  • docs/src/SUMMARY.md
  • docs/src/auto-frontlight.md
  • docs/src/settings/index.md
✅ Files skipped from review due to trivial changes (3)
  • docs/src/auto-frontlight.md
  • docs/src/settings/index.md
  • docs/src/SUMMARY.md
🚧 Files skipped from review as they are similar to previous changes (19)
  • contrib/Settings-sample.toml
  • crates/core/src/lib.rs
  • crates/emulator/src/main.rs
  • crates/core/src/view/settings_editor/category.rs
  • crates/core/Cargo.toml
  • crates/core/src/view/settings_editor/kinds/mod.rs
  • crates/core/src/view/settings_editor/kinds/identity.rs
  • crates/core/src/view/home/mod.rs
  • crates/core/src/frontlight/premixed.rs
  • crates/core/src/task/mod.rs
  • crates/core/src/context.rs
  • crates/core/src/view/mod.rs
  • crates/core/src/frontlight/auto.rs
  • crates/core/src/time_manager.rs
  • crates/core/src/geolocation.rs
  • crates/cadmus/src/app.rs
  • crates/core/src/view/settings_editor/kinds/general.rs
  • crates/core/src/frontlight/mod.rs
  • crates/core/src/settings/mod.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (40)
  • GitHub Check: test (test + tracing, ubuntu-latest)
  • GitHub Check: test (kobo + profiling, ubuntu-latest)
  • GitHub Check: test (kobo + test, ubuntu-latest)
  • GitHub Check: test (emulator + kobo + profiling, ubuntu-latest)
  • GitHub Check: clippy (test + tracing, ubuntu-latest)
  • GitHub Check: test (default, ubuntu-latest)
  • GitHub Check: test (kobo, ubuntu-latest)
  • GitHub Check: clippy (kobo + test + tracing, ubuntu-latest)
  • GitHub Check: clippy (kobo + profiling + test + tracing, ubuntu-latest)
  • GitHub Check: clippy (emulator + kobo + test + tracing, ubuntu-latest)
  • GitHub Check: clippy (emulator + kobo + profiling + tracing, ubuntu-latest)
  • GitHub Check: clippy (emulator + kobo + profiling + test + tracing, ubuntu-latest)
  • GitHub Check: clippy (emulator + profiling + test + tracing, ubuntu-latest)
  • GitHub Check: clippy (profiling + test + tracing, ubuntu-latest)
  • GitHub Check: clippy (emulator + test + tracing, ubuntu-latest)
  • GitHub Check: clippy (profiling + tracing, ubuntu-latest)
  • GitHub Check: clippy (kobo + profiling + tracing, ubuntu-latest)
  • GitHub Check: clippy (kobo + tracing, ubuntu-latest)
  • GitHub Check: clippy (emulator + profiling + tracing, ubuntu-latest)
  • GitHub Check: clippy (emulator + kobo + tracing, ubuntu-latest)
  • GitHub Check: clippy (kobo + profiling, ubuntu-latest)
  • GitHub Check: clippy (kobo + profiling + test, ubuntu-latest)
  • GitHub Check: clippy (profiling + test, ubuntu-latest)
  • GitHub Check: clippy (emulator + tracing, ubuntu-latest)
  • GitHub Check: clippy (tracing, ubuntu-latest)
  • GitHub Check: clippy (emulator + kobo + profiling, ubuntu-latest)
  • GitHub Check: clippy (emulator + profiling + test, ubuntu-latest)
  • GitHub Check: clippy (emulator + test, ubuntu-latest)
  • GitHub Check: clippy (emulator + kobo + test, ubuntu-latest)
  • GitHub Check: clippy (emulator + profiling, ubuntu-latest)
  • GitHub Check: clippy (profiling, ubuntu-latest)
  • GitHub Check: clippy (kobo + test, ubuntu-latest)
  • GitHub Check: clippy (test, ubuntu-latest)
  • GitHub Check: clippy (emulator + kobo, ubuntu-latest)
  • GitHub Check: clippy (emulator, ubuntu-latest)
  • GitHub Check: clippy (default, ubuntu-latest)
  • GitHub Check: clippy (kobo, ubuntu-latest)
  • GitHub Check: build-kobo
  • GitHub Check: build-kobo-test
  • GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (5)
crates/core/**/*.{ftl,rs}

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

Every user-visible string needs a Fluent message ID in crates/core/i18n/en-GB/cadmus_core.ftl. Use fl!("message-id") at the call site. For parameterised strings, use Fluent variables ({ $var }) in the .ftl file and pass values via fl!("id", var = value).

Files:

  • crates/core/i18n/en-GB/cadmus_core.ftl
  • crates/core/src/view/reader/mod.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/task/time_sync.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/view/frontlight.rs
crates/core/**/*.ftl

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

crates/core/**/*.ftl: Keep Fluent message keys (.ftl file keys) sorted alphabetically within each comment section.
Fluent message key naming convention: use kebab-case, prefixed by feature area. Use settings-<category>-<description> for settings labels, settings-<category>-<description>-input for input fields, and notification-<description> for notifications.

Files:

  • crates/core/i18n/en-GB/cadmus_core.ftl
crates/core/**/*.rs

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

crates/core/**/*.rs: Rustdoc examples should follow this preference order: fully compilable > no_run > ignore. Use no_run when the example compiles but cannot execute (file I/O, network, database). Use ignore only for private/pub(crate) items unreachable from the test harness with a comment explaining why. Use # to hide boilerplate setup lines. Verify with cargo test --doc.
Use typed SQL macros (sqlx::query!, sqlx::query_as!, sqlx::query_scalar!) in Rust code. Never use untyped sqlx::query() / sqlx::query_as() / sqlx::query_scalar(). Use .flatten() on query_scalar! results for nullable columns (Option<Option<T>>Option<T>).
Untyped SQL queries are allowed only for dynamic SQL (e.g. runtime ORDER BY column) if and only if the function has unit tests covering every dynamic path, with a comment explaining why the typed macro cannot be used.
All user-visible strings must use the fl! macro (use crate::fl;). Never hardcode string literals for labels, buttons, placeholders, or notifications. User-visible strings include: label() implementations on SettingKind traits, input field label strings in Event::OpenNamedInput, menu entry text (EntryKind::Command, EntryKind::RadioButton, etc.), and button labels, notification text.

Files:

  • crates/core/src/view/reader/mod.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/task/time_sync.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/view/frontlight.rs
crates/core/**/*.{sql,rs}

📄 CodeRabbit inference engine (crates/core/AGENTS.md)

All SQL queries must list explicit column names. Do not use SELECT *.

Files:

  • crates/core/src/view/reader/mod.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/task/time_sync.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/view/frontlight.rs
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Prefer ? operator over unwrap() / expect() in library and app code
Use thiserror for custom error types and anyhow for ad-hoc errors in Rust
Use iterators over index-based loops in Rust
Use &str over String in function parameters when ownership is not needed
Prefer borrowing over cloning in Rust
Inline expressions directly into struct fields — avoid intermediate bindings solely to pass them into a struct literal
Avoid unsafe unless required and documented in Rust
Avoid premature collect() — keep iterators lazy in Rust
Ensure Rust code compiles without warnings
Prefer newtype wrappers over raw primitives for domain concepts (e.g., Fingerprint(String) instead of bare String)
Wrap String, i64, and similar primitives when the value represents a specific domain concept (fingerprints, file kinds, identifiers, etc.)
Implement Display, FromStr, and other standard traits on newtype wrappers to keep them ergonomic
Match on enum variants instead of string comparisons — the compiler catches missing arms
Comment why, not what. Avoid inline comments — extract code into well-named, documented functions instead
Avoid dead-code comments (commented-out code) in Rust
Avoid changelog comments (Modified by X on date) in Rust
Avoid divider comments (//=====) in Rust
Annotations (TODO, FIXME, HACK, NOTE) are acceptable with context in Rust
Use structured fields with the tracing crate — never use string formatting for log data
Use only structured fields for logging — data goes in fields, not format strings
Do not use module prefixes in logs — instrumentation scope provides context
Do not mix structured fields with format arguments in logging
Use debug log level for development and troubleshooting detail
Use info log level for important runtime events
Use warn log level for recoverable issues (retries, fallbacks)
Use error log level for failures requiring attention
Return domain types directly from sqlx queries instead of parsin...

Files:

  • crates/core/src/view/reader/mod.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/task/time_sync.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/view/frontlight.rs
🧠 Learnings (1)
📚 Learning: 2026-05-23T14:57:43.820Z
Learnt from: OGKevin
Repo: OGKevin/cadmus PR: 478
File: crates/core/src/view/settings_editor/refresh_rate_by_kind_editor.rs:91-91
Timestamp: 2026-05-23T14:57:43.820Z
Learning: When reviewing Rust code that calls `ToggleableKeyboard::new(parent_rect: Rectangle, number_mode: bool)` (from `crates/core/src/view/toggleable_keyboard.rs`), treat the second argument as `number_mode` (numeric/number-layout mode), not as an initial visibility flag. In the constructor implementation, `visible` is initialized to `false` and the keyboard starts hidden; only the numeric layout changes based on `number_mode` (e.g., for numeric inputs like `refresh rate u8`). Therefore, do not flag `ToggleableKeyboard::new(rect, true)` as “making the keyboard start visible.”

Applied to files:

  • crates/core/src/view/reader/mod.rs
  • crates/core/src/view/settings_editor/category_editor.rs
  • crates/core/src/task/auto_frontlight.rs
  • crates/core/src/task/time_sync.rs
  • crates/core/src/frontlight/natural.rs
  • crates/core/src/frontlight/standard.rs
  • crates/core/src/view/frontlight.rs
🔇 Additional comments (10)
crates/core/src/view/frontlight.rs (3)

29-29: LGTM!

Also applies to: 217-222, 259-271, 294-306


144-144: LGTM!

Also applies to: 162-162, 283-284, 397-398


371-390: LGTM!

crates/core/src/view/settings_editor/category_editor.rs (4)

800-802: LGTM!


837-838: LGTM!

Also applies to: 940-941


1016-1021: LGTM!


1566-1615: LGTM!

crates/core/src/task/time_sync.rs (1)

3-5: LGTM!

Also applies to: 31-53

crates/core/src/task/auto_frontlight.rs (2)

1-7: LGTM!


9-12: LGTM!

Comment thread crates/core/src/task/auto_frontlight.rs
@OGKevin
OGKevin force-pushed the ogkevin/push/tvmrqnxzvmro branch from 3574fa1 to c80c05a Compare June 14, 2026 17:54
@OGKevin
OGKevin temporarily deployed to cloudflare-pages-preview June 14, 2026 17:57 — with GitHub Actions Inactive
Change-Id: 64d89c204d8bed70af57523eb93546ad
Change-Id-Short: tvmrqnxzvmro
@OGKevin

OGKevin commented Jun 14, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@OGKevin OGKevin changed the title feat: auto brightness feat(Frontlight): auto brightness and warmth Jun 15, 2026
@OGKevin
OGKevin merged commit 7e1414d into master Jun 15, 2026
82 checks passed
@OGKevin
OGKevin deleted the ogkevin/push/tvmrqnxzvmro branch June 15, 2026 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review enhancement New feature or request Release/0.11.0 size:XXL This PR changes 1000+ lines, ignoring generated files. Subsystem/Settings Editor The system that allows users to change app settings.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auto warmth & brightness

1 participant