Add Platform.System.os_description/0#74
Conversation
Replace direct :wx_misc.getOsDescription/0 usage so apps can read a device/OS description on Wx, Json/mobile bridge, and Browser backends without depending on the Hex Bridge GenServer. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Code Review
This pull request introduces Desktop.Platform.System.os_description/0 as a backend-safe replacement for :wx_misc.getOsDescription/0 across different backends (Wx, Json bridge, and Browser). It includes corresponding backend implementations, tests, documentation updates, and a guard rule to prevent direct calls to :wx_misc.getOsDescription/0. Feedback suggests refactoring os_description/0 in lib/desktop/platform/system.ex to avoid piping a block directly into a case statement, which is an Elixir anti-pattern, and instead bind the block's result to a local variable first.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def os_description do | ||
| Helpers.with_wx_env(fn -> | ||
| case impl().os_description() do | ||
| nil -> nil | ||
| desc when is_list(desc) -> List.to_string(desc) | ||
| desc when is_binary(desc) -> desc | ||
| _ -> nil | ||
| end | ||
| end) | ||
| |> case do | ||
| nil -> | ||
| nil | ||
|
|
||
| str -> | ||
| case String.trim(str) do | ||
| "" -> nil | ||
| other -> other | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Piping the result of a block directly into a case statement is generally considered an anti-pattern in Elixir (as highlighted by tools like Credo). It can make the code harder to read and debug.
Instead, bind the result of Helpers.with_wx_env/1 to a local variable first, and then perform the case match on that variable.
def os_description do
raw_desc =
Helpers.with_wx_env(fn ->
case impl().os_description() do
desc when is_binary(desc) -> desc
desc when is_list(desc) -> List.to_string(desc)
_ -> nil
end
end)
case raw_desc do
nil ->
nil
str ->
case String.trim(str) do
"" -> nil
trimmed -> trimmed
end
end
end
References
- Avoid piping into control flow structures like
case,if, orcondto maintain readability and idiomatic Elixir style. (link)
|
@gemini review this PR |
Bind Helpers.with_wx_env/1 before matching so we avoid piping into case, and add Platform.System tests for charlist/binary and blank → nil. Co-authored-by: Dominic Letz <dominicletz@users.noreply.github.com>
wx_use_env/0 calls Platform.System.set_env/1 when Desktop.Env is running, so the os_description stub must no-op set_env/1 for host wx tests. Co-authored-by: Dominic Letz <dominicletz@users.noreply.github.com>
Summary
Desktop.Platform.System.os_description/0as the backend-safe replacement for:wx_misc.getOsDescription/0:wx_misc), Json (Desktop.Bridge.Protocol), and Browser (nil)~c"Mock OS"; facade normalizes charlist/binary and trims empty →nil:wx_misc.getOsDescription/0directlyNeeded by diode-drive: after the mobile bridge embed (#73),
OS.model/0still called:wx_misc.getOsDescription/0, which targeted the removed HexBridgeGenServer and crashed Android device linking.Test plan
NO_WX=1 mix test test/desktop/backend/json_test.exs test/desktop/backend/browser_test.exs test/desktop/regression/beam_wx_calls_test.exs --exclude wxmix test.guardxvfb-run mix test.wx(or local wx) forT-WX: os_description via Platform.SystemMade with Cursor