Repository navigation
[companion] fix(notifications): list system notifications and open their session from the Notifications tab - #92
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to This change adds OS-shown notifications to the Notifications tab and lets users open their session by clicking a row or pressing Enter. No actionable merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 4 | ❌ 1
✨ Finishing Touches 💡 1
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 |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8dbc97aab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void showSystemNotification(rendered).then((shown) => { | ||
| if (!shown) showToast(rendered, event.type); | ||
| // Shown by the OS: log it so the Notifications tab still lists it. | ||
| else toastHistory.record(event.type === "error" ? "error" : "info", rendered.title, rendered.body, { id: rendered.tag, onClick: openFor(rendered) }); |
There was a problem hiding this comment.
For devices with a push subscription, routeNotification() selects kind: "push" instead of the kind: "os" SSE path, and public/sw.js only displays that push without reporting it to the page. Consequently, this new toastHistory.record() branch is never reached for subscribed browsers—the normal configuration after enabling notifications—so their system notifications still do not appear in the Notifications tab. The service worker needs to notify open clients, or the history must be persisted and loaded separately.
AGENTS.md reference: AGENTS.md:L713-L715
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right: subscribed browsers get push, not os, so that branch never ran for them. Fixed in b982685: the push payload now carries type, and after showing a push public/sw.js posts omp-notification-shown to every open window; useNotifications records it in toastHistory with the session onClick. New tests run the real sw.js against a fake worker scope and check the page-side handler (both fail when their part is removed). Remaining limit, now in the CHANGELOG: history is per tab and in memory, so a push that arrives with no omp-web tab open is not listed.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
… panel Closes kahme247#239, kahme247#232, kahme247#237, kahme247#234, kahme247#235, kahme247#236. ## Password hashes, and keeping them out of child processes (kahme247#239) A plaintext web password now stops the server at startup instead of starting it, and omp-web refuses every request while one is configured. Every omp session inherits omp-web's environment, so an agent running `env` could print the password into its session file and send it to the model provider. - `OMP_WEB_PASSWORD_HASH` is the only accepted credential, in a self-describing scrypt format (`scrypt$ln$r$p$salt$digest`, N = 2^15 by default). The cost is floored on read, so a hand-written weak hash cannot be walked back. - `ompweb hash-password` reads the password from stdin (never argv) with the echo off, so it stays out of shell history and out of `ps`. - Sessions are signed with a per-installation random key created on first start at `~/.omp/agent/omp-web/web-auth-secret.json` (mode 0600) mixed with the hash: a copied hash cannot forge a cookie, and changing the password still signs every browser out. - omp-web's own secrets are stripped from every child environment through `createChildProcessEnvironment` (lib/project-command-env.ts): omp sessions and one-shot RPCs, plugin install/upgrade, skill install/update/search, update checks and the self-update worker, agent unpack, the Git commands the file panel and workspace list run, and the spawned PowerShell helpers. Configuration such as `OMP_WEB_OMP_BIN` still reaches the processes that read it. - The service installers hash what they are given: systemd, launchd, the Linux tray's Set Web Password and the Windows service/tray scripts write only the hash, and the tray prompts for a password when exposing past loopback. ## Queue by default, and stop aborting on a running shell command (kahme247#237) Submitting while the agent works now queues the message instead of steering: steering a long tool or shell command could abort the turn mid-task and left the new message undelivered. `getSubmitDuringRunBehavior()` defaults to `queue`; only a stored choice overrides it. Sending during omp-web's own `!!` shell command no longer fails with "Cannot send a prompt while a shell command is running": the wrapper degrades the frame into omp's `follow_up` queue (`queueWhileShellRunning`) and reports `{queued:true, reason:"shellRunning"}`, which the hook surfaces as a notice while withdrawing the phantom optimistic turn. The queue chips still come from omp's own snapshot. ## Expand thinking blocks by default (kahme247#232) An off-by-default switch next to **Keep tool calls collapsed**, the same shape as that preference: `omp-web:expand-thinking` in AppShell, a Settings toggle, and a `MessageView` prop that seeds `ThinkingBlock` as an initial value only, so manual collapse/expand keeps working. It deliberately covers a streaming block too, and a historical block that is `deferred` loads its text through the same loader the click path uses. ## Fit the agent's question panel on a phone (kahme247#234, kahme247#235, kahme247#236) - Pin the `ask` grids to `minmax(0, 1fr)` with `min-width: 0` on the panel, its scroller, the option rows and their text, so a long question, header or preview can no longer widen the panel past the composer column and clip the answer text; previews wrap instead of scrolling sideways (kahme247#234). - Stop the composer-attached focus effect from pulling focus into the answer box on touch input or a phone width, which raised the on-screen keyboard over the unread question. Desktop focus behavior is unchanged (kahme247#235). - Keep the footer reachable with the keyboard up by capping the panel to the visual viewport while it is open, plus an inline send button beside the typed answer that shares the footer's validation; the dialog's buttons follow the **Touch Targets** setting (kahme247#236). BREAKING CHANGE: `OMP_WEB_PASSWORD` and `ompweb --password` no longer work and refuse to start. Generate a hash with `ompweb hash-password` and set `OMP_WEB_PASSWORD_HASH`. Existing sessions are invalidated.
…n from the Notifications tab
b982685 to
f45b7d9
Compare
kahme247#238) * feat(auth): skip the password for requests a trusted proxy vouches for OMP_WEB_TRUSTED_HEADER_SHA256 holds the SHA-256 digest of a secret a reverse proxy sends in X-Omp-Web-Auth for trusted client ranges; a matching request counts as signed in. omp-web never holds the secret, and the digest is stripped from child processes like the other omp-web secrets. Service installers and the Windows launchers pass it on. The Windows tray (native and PowerShell) and background service now read OMP_WEB_PASSWORD_HASH, the legacy OMP_WEB_PASSWORD and the digest fresh from the user, then system, environment on every server start, so rotating or removing them with setx takes effect at the next restart. bin/omp-web-tray.exe is rebuilt from native/tray. README documents the nginx setup (realip for Cloudflare, Cloudflare Tunnel and other proxies, DNS-rebinding guard) and generating the secret without it reaching shell history. * docs(auth): drop reusing a chosen proxy secret; require the generated one The digest is a fast unsalted SHA-256, safe only for a long random secret. * fix(auth): keep stripping the legacy trusted-proxy secret name from child processes
…me247#243) * fix(sessions): find the first user message past injected context omp writes rules, skills and memory-recall entries before the first user message, which pushes it past the 4 KiB list-scan prefix. The sidebar then showed "(no messages)" and auto-name refused with no messages to name until omp set a title. When the prefix has no user message, scan up to 256 KiB. * fix(sessions): scan for the first user message without a fixed window Injected context larger than 256 KiB still hid the first user message. Stream lines until the first textual user entry instead; forEachFileLineSync now stops when onLine returns false. * fix(sessions): skip the first-message scan for files past the load ceiling The streaming scan read oversized files whose prefix has no user message, and a line past the JS string cap threw, so scanSessionInfo dropped the session and its route returned 404 instead of 413.
…h hash-password on Enter (kahme247#244) * fix(hash-password): finish the hidden prompt on Enter A terminal in raw mode sends Enter as a carriage return, so the prompt waited for a newline that never came. End the line at \r or \n. * feat(auth): migrate a plaintext OMP_WEB_PASSWORD to a hash at startup ompweb hashes OMP_WEB_PASSWORD, removes it from the environment before starting the server, and logs a warning with the hash to set as OMP_WEB_PASSWORD_HASH. --password, or both variables set, still stops it. * fix(auth): test the password migration end to end and document its limits Inline the migration in the launcher and replace the source-text security test with one that runs the launcher against a stub server and checks the environment it receives. Refuse both variables with a direct message, and note in the warning and docs that the launcher process still holds the plaintext. * fix(auth): word the OMP_WEB_PASSWORD warning as a deprecation
…story-open # Conflicts: # CHANGELOG.md
Review companion for kahme247#240: same head branch, base pinned to the same upstream/main commit (
cf64eb8), so the diff is identical. Bot reviews only; merge happens upstream.Summary by CodeRabbit