fix: improve desktop menu updates and feedback - #98
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a renderer→preload→main DesktopContext bridge with per-window storage and menu-locale persistence; implements a discriminated updater controller and finalizer/merge for updater metadata; adds feedback/problem-report tooling, localized menu templates, CI workflow changes, and many unit/contract tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Renderer
participant Preload as Preload (ipcRenderer)
participant IPC as Main IPC
participant Store as DesktopContextStore
participant Menu as Menu System
Renderer->>Preload: setDesktopContext(context)
Preload->>IPC: invoke "set-desktop-context"
IPC->>Store: set(windowId, context)
Store->>Menu: notify locale/context change -> rebuild menu
Menu->>Menu: build localized template using stored menu-locale
sequenceDiagram
actor User
participant Menu
participant Updater as Updater Controller
participant Remote as Remote (check/download)
participant Dialog
User->>Menu: Click "Check for Updates"
Menu->>Updater: check()
alt inflight
Updater->>Dialog: return status "busy"
else
Updater->>Remote: deps.checkForUpdates()
alt no update
Updater->>Dialog: return status "none"
else metadata present
Updater->>Remote: deps.downloadUpdate()
alt download success
Updater->>Dialog: return status "ready" with version
else download fails
Updater->>Dialog: return status "failed" with reason/message
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust diagnostic feedback system and an enhanced application updater for the desktop client. Key changes include the implementation of a desktop context store to track active sessions and routes across windows, a new problem reporting utility that generates markdown-based diagnostic reports, and a refactored updater controller with improved state management and localized feedback. Additionally, the release metadata scripts were updated to support merging architecture-specific manifests, and the application menu was refactored to support multi-language localization. I have no feedback to provide as no review comments were included.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20505eee4c
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 321-335: The GH release download commands currently end with "||
true" which hides failures; update the step that sets existing_dir and tag so
that after each gh release download (the lines invoking `gh release download
"$tag" --pattern latest-mac.yml` and `gh release download "$tag" --pattern
latest.yml"`) you detect failure and echo a brief warning indicating which
pattern wasn't found and that this may be the first release (e.g., check the
command exit status and use echo to log the missing metadata), while preserving
the existing behavior for first releases; reference the existing variables
existing_dir and tag to build the message.
In `@packages/app/src/app.tsx`:
- Around line 145-153: The call to window.api.setDesktopContext inside
createEffect can reject and should mirror the project's pattern for swallowing
promise rejections; update the invocation of
window.api.setDesktopContext(buildDesktopContext(...)) in the createEffect
callback so the returned promise is handled with a .catch(() => undefined)
(instead of just using void), keeping the same arguments (route:
`${location.pathname}${location.search}${location.hash}`, locale:
language.locale()) to avoid unhandled rejections.
In `@packages/app/src/context/platform.tsx`:
- Around line 10-16: The UpdateInfo type is too loose and allows invalid
combinations; replace the single interface with a discriminated union for
UpdateInfo keyed by updateAvailable and status so each state enforces required
fields (e.g., a "ready" variant requires version, a "failed" variant requires
reason and message, "disabled"/"none"/"busy" variants omit version/message).
Update all usages to expect the new union shape and narrow on the discriminant
(updateAvailable/status) before accessing version/message; reference the
UpdateInfo type in packages/app/src/context/platform.tsx and adjust any
functions reading UpdateInfo accordingly.
In `@packages/desktop-electron/scripts/finalize-latest-yml.ts`:
- Around line 103-117: The catch block in downloadExisting currently swallows
all gh release download errors; change it so only the "asset not found" case
returns undefined while all other errors are propagated. In the catch for the
shell invocation of `gh release download ${tag} --pattern ${filename} ...`,
inspect the thrown error (e.g., error.message, error.stderr or exit code) and if
it clearly indicates a missing asset/404/Not Found return undefined, otherwise
rethrow the error so the job fails; keep the EXISTING_LATEST_YML_DIR
early-return logic and the final readFile(path.join(existingDir, filename))
unchanged.
In `@packages/desktop-electron/src/main/constants.ts`:
- Line 6: FEEDBACK_FORM_URL currently exports the raw env value which can be a
whitespace-only string and thus truthy; change the export to trim the env
variable before falling back so whitespace-only values become an empty string
(i.e. use (import.meta.env.PAWWORK_FEEDBACK_FORM_URL ?? "").trim() when
assigning FEEDBACK_FORM_URL) so downstream checks on FEEDBACK_FORM_URL correctly
treat blank values as disabled.
In `@packages/desktop-electron/src/main/desktop-context-store.ts`:
- Around line 8-15: When updating an existing window context in the store,
ensure the Map insertion order is updated so `latest` always reflects the most
recently updated context: in the `set(windowID: number, context:
DesktopContext)` function, if `contexts.has(windowID)` delete that key first
before calling `contexts.set(windowID, context)` so the entry is moved to the
end; then assign `latest = context`. Keep the `delete(windowID: number)` logic
as-is (it correctly falls back to `latest = [...contexts.values()].at(-1) ??
initial`) so removals still pick the newest remaining entry.
In `@packages/desktop-electron/src/main/feedback.ts`:
- Around line 15-17: The helper function errorMessage is duplicated in
feedback.ts and updater.ts; extract it into a single exported utility (e.g.,
utils.ts) as export function errorMessage(error: unknown): string { return error
instanceof Error ? error.message : String(error) } and then replace the local
definitions in both feedback.ts and updater.ts with an import of that shared
function and use it where previously called (ensure correct named import and
update any references to errorMessage).
In `@packages/desktop-electron/src/main/logging.ts`:
- Around line 24-26: The cleanup() function still constructs/reads the log file
path inline instead of using the new filePath() helper; update cleanup() to call
filePath() wherever it currently computes or accesses the log transport path so
the helper becomes the single source of truth (ensure references to
log.transports.file.getFile().path are removed and replaced with filePath()
inside cleanup()).
In `@packages/desktop-electron/src/main/menu-i18n.ts`:
- Around line 4-8: In readStoredMenuLocale, the conditional `if (stored !== "en"
|| raw)` is subtle; add a brief inline comment above that line explaining the
fallback logic: that parseMenuLocale should be returned when a user has
explicitly chosen any locale (including "en"), and only when nothing was stored
(raw falsy) should the code fall back to detectSystemMenuLocale(systemLocale);
reference the surrounding symbols parseMenuLocale,
getStore("opencode.global.dat").get("language"), and detectSystemMenuLocale to
help locate the spot.
In `@packages/desktop-electron/src/main/menu-labels.ts`:
- Around line 125-129: detectSystemMenuLocale currently maps "zh-TW" and "zh-HK"
to "zht" but misses "zh-Hant" / "zh-Hant-*" variants; update
detectSystemMenuLocale to treat any locale that startsWith("zh-Hant") (and keep
the existing "zh-TW"/"zh-HK" checks) as "zht" before the generic "zh" branch so
locales like "zh-Hant" or "zh-Hant-TW" return "zht". Ensure you reference the
detectSystemMenuLocale function and preserve the fallback to "en".
In `@packages/desktop-electron/src/main/menu.ts`:
- Around line 10-23: createMenu currently defaults locale to
readStoredMenuLocale(app.getLocale()) but callers that rebuild the menu (e.g.,
the place that updates menuLocale) still call createMenu(deps) so updates are
ignored; change the caller to pass the tracked menuLocale through to
createMenu(menuDeps, menuLocale) and ensure buildMenuTemplate receives that
locale (createMenu -> buildMenuTemplate locale parameter) so the rebuilt menu
uses the updated locale instead of the default readStoredMenuLocale value.
In `@packages/desktop-electron/src/main/problem-report.ts`:
- Around line 110-114: The regex in parseProblemReportPayload uses a greedy
([\s\S]*) which can span multiple fenced blocks; change it to a non-greedy match
(use ([\s\S]*?) ) when matching the ```json ... ``` block so the function only
captures the first JSON code fence and then JSON.parse(match[1]) as before.
In `@packages/desktop-electron/src/main/updater.ts`:
- Around line 50-54: The Deps type for checkForUpdates is too permissive: update
the signature of Deps["checkForUpdates"] (the checkForUpdates function
referenced in updater.ts) to match electron-updater v6 by returning Promise<{
isUpdateAvailable: boolean; updateInfo?: UpdateInfo } | null> instead of
allowing isUpdateAvailable?: boolean or undefined; ensure any call sites that
check for null use a null check (not undefined) and remove unnecessary defensive
handling for undefined isUpdateAvailable.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 51b1d75f-5ac1-49b9-adb0-6500c8e74792
📒 Files selected for processing (40)
.github/workflows/build.ymlpackages/app/src/app.tsxpackages/app/src/components/settings-general.tsxpackages/app/src/context/platform.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/i18n/zht.tspackages/app/src/pages/error-update.test.tspackages/app/src/pages/error-update.tspackages/app/src/pages/error.tsxpackages/app/src/pages/session.tsxpackages/app/src/utils/desktop-context.test.tspackages/app/src/utils/desktop-context.tspackages/desktop-electron/electron.vite.config.tspackages/desktop-electron/scripts/finalize-latest-yml.tspackages/desktop-electron/scripts/release-metadata-contract.test.tspackages/desktop-electron/src/main/constants.tspackages/desktop-electron/src/main/desktop-context-store.test.tspackages/desktop-electron/src/main/desktop-context-store.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/logging.tspackages/desktop-electron/src/main/menu-i18n.tspackages/desktop-electron/src/main/menu-labels.test.tspackages/desktop-electron/src/main/menu-labels.tspackages/desktop-electron/src/main/menu-template.tspackages/desktop-electron/src/main/menu.test.tspackages/desktop-electron/src/main/menu.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/updater-dialog-labels.test.tspackages/desktop-electron/src/main/updater-dialog-labels.tspackages/desktop-electron/src/main/updater.test.tspackages/desktop-electron/src/main/updater.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.tspackages/desktop-electron/src/renderer/index.tsxpackages/opencode/test/github/build-workflow.test.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
61c424e to
a574360
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 322-336: The workflow step named "Download existing updater
metadata" downloads both latest-mac.yml and latest.yml every run; change it to
conditionally download only the platform-appropriate file by splitting or
guarding the gh release download lines with runner OS conditions: download
latest-mac.yml only when runner.os == 'macOS' (matching finalize-latest-yml.ts
usage for mac) and download latest.yml only when runner.os == 'Windows'
(matching finalize-latest-yml.ts usage for windows), keeping the same
error-handling echo and env variables so the rest of the job and
finalize-latest-yml.ts continue to work.
In `@packages/app/src/app.tsx`:
- Around line 82-87: The window.api declaration currently inlines the payload
shape for setDesktopContext; instead import and reuse the shared DesktopContext
type (from the existing desktop-context definition) and replace the inline
object signature with setDesktopContext?: (context: DesktopContext) =>
Promise<void> in the window.api declaration so the renderer contract references
the central DesktopContext type; update any related imports to bring
DesktopContext into scope and remove the duplicated inline type.
In `@packages/app/src/pages/error-update.ts`:
- Around line 6-12: The helper currently treats readiness via the condition
`result.updateAvailable && result.version`, which can misclassify a
`result.status === "ready"` without a version; add an explicit branch that
checks `result.status === "ready"` (before the "up to date" fallthrough) and
return the expected shape (e.g., { version: result.version ?? undefined,
actionError: undefined, actionMessage: undefined }) so the `ready` state is
handled as a distinct case alongside the existing `result.updateAvailable`
logic.
In `@packages/app/src/pages/session.tsx`:
- Around line 340-350: The createEffect currently fire-and-forgets the IPC
invoke window.api.setDesktopContext and can produce unhandled promise
rejections; wrap the call so rejections are handled (e.g. await it inside an
async IIFE with try/catch or append .catch()) and log or silently ignore
expected teardown/navigation errors. Update the block that calls
window.api.setDesktopContext (which uses buildDesktopContext, sdk.directory,
params.id, and language.locale()) to ensure any thrown error is caught and
handled to prevent unhandled promise rejections.
In `@packages/desktop-electron/scripts/release-metadata-contract.test.ts`:
- Around line 27-36: The test assumes Unix paths and env separators; update the
three Bun.spawn() invocations (the ones that call
"./scripts/finalize-latest-yml.ts" and the other two similar calls) to build cwd
and script paths using platform-aware utilities (e.g., path.join/path.sep or
derive from import.meta.url) instead of string replace with "/scripts", and
construct the PATH env value using path.delimiter instead of ":"; also modify
the writeFakeGh() helper so the fake gh executable filename is platform-aware
(append ".cmd" or ".bat" on Windows and ensure it's created as an executable
script for POSIX vs a .cmd wrapper on Windows) and update any test references to
that filename accordingly so the spawn calls find the stub on all platforms.
In `@packages/desktop-electron/src/main/feedback.test.ts`:
- Around line 44-48: Add a complementary test that verifies the English/fallback
labels returned by feedbackDialogLabels by calling feedbackDialogLabels("en")
(or feedbackDialogLabels(undefined) if code uses fallback) and asserting title
and confirm match the expected English strings; update the describe block in
feedback.test.ts alongside the existing Simplified Chinese test so both locales
(feedbackDialogLabels("zh") and feedbackDialogLabels("en")) are covered.
In `@packages/desktop-electron/src/main/index.ts`:
- Around line 455-461: checkUpdate currently doesn't handle result.status ===
"disabled" (and checkForUpdates short-circuits before that state can surface),
causing silent no-feedback when the updater is gated off; update checkUpdate to
explicitly handle the "disabled" state by returning an object like {
updateAvailable: false, status: "disabled", reason?: result.reason, message?:
result.message } based on the value from updater.check(), and adjust
checkForUpdates to not early-return before calling checkUpdate so the "disabled"
status can be propagated to callers/UI; reference checkUpdate, checkForUpdates,
and updater.check() when making the changes.
In `@packages/desktop-electron/src/main/updater.ts`:
- Around line 31-33: The ready flag updateReady is being cleared too early
causing install() to return false even when a downloaded update exists; modify
the logic in run() and the code paths referenced around install() so updateReady
remains true (latched) until the actual installation process begins — only set
updateReady = false at the moment install() starts (or immediately before
invoking the installer) and avoid resetting it on subsequent status checks or
when returning intermediate results; update references to run(), install(), and
the updateReady variable so the ready state reflects a pending-on-disk update
until install initiation.
In `@packages/desktop-electron/src/renderer/index.tsx`:
- Around line 35-43: The startup code uses void initI18n().then(...) and calls
window.api.setDesktopContext(...) without handling promise rejections; update
the bootstrap to explicitly handle failures by adding a .catch (or using
try/catch with await) on initI18n() and on the setDesktopContext call so any
errors are logged/handled instead of causing unhandled rejections; specifically
wrap the initI18n() promise chain that sets currentLocale and calls
window.api.setDesktopContext (and the setDesktopContext promise itself) with
explicit rejection handlers that log the error and perform any necessary
fallback behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9b4b733-3b96-4ca8-a8df-011d9b11b423
📒 Files selected for processing (39)
.github/workflows/build.ymlpackages/app/src/app.tsxpackages/app/src/components/settings-general.tsxpackages/app/src/context/platform.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/error-update.test.tspackages/app/src/pages/error-update.tspackages/app/src/pages/error.tsxpackages/app/src/pages/session.tsxpackages/app/src/utils/desktop-context.test.tspackages/app/src/utils/desktop-context.tspackages/desktop-electron/electron.vite.config.tspackages/desktop-electron/scripts/finalize-latest-yml.tspackages/desktop-electron/scripts/release-metadata-contract.test.tspackages/desktop-electron/src/main/constants.tspackages/desktop-electron/src/main/desktop-context-store.test.tspackages/desktop-electron/src/main/desktop-context-store.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/logging.tspackages/desktop-electron/src/main/menu-i18n.tspackages/desktop-electron/src/main/menu-labels.test.tspackages/desktop-electron/src/main/menu-labels.tspackages/desktop-electron/src/main/menu-template.tspackages/desktop-electron/src/main/menu.test.tspackages/desktop-electron/src/main/menu.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/updater-dialog-labels.test.tspackages/desktop-electron/src/main/updater-dialog-labels.tspackages/desktop-electron/src/main/updater.test.tspackages/desktop-electron/src/main/updater.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.tspackages/desktop-electron/src/renderer/index.tsxpackages/opencode/test/github/build-workflow.test.ts
a574360 to
0a96c00
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/src/app.tsx`:
- Around line 140-149: The root createEffect currently calls
window.api.setDesktopContext with buildDesktopContext which defaults omitted
session fields to null, overwriting richer session state; update the effect to
either (A) skip calling setDesktopContext for session routes (detect via
location.pathname or an isSessionRoute helper used in pages/session.tsx) or (B)
fetch/merge the existing desktop context before calling setDesktopContext so you
only update route/locale and preserve directory/sessionID; reference the
createEffect, window.api.setDesktopContext, and buildDesktopContext symbols when
making the change.
In `@packages/desktop-electron/src/main/index.ts`:
- Around line 360-375: The setDesktopContext handler currently trusts
renderer-provided DesktopContext and directly uses context.locale to set global
menuLocale and call wireMenu; instead validate and normalize the incoming
context before storing: in setDesktopContext (and before writing desktopContexts
and updating menuLocale) check that context and context.locale exist and that
context.locale matches one of the supported locales (or a Locale enum/array used
elsewhere), coerce it to a safe canonical form or fallback to a defaultLocale,
then store the sanitized object in desktopContexts and only update
menuLocale/wireMenu with the validated locale; also ensure you handle missing or
malformed DesktopContext fields and keep contextWindowCleanup logic unchanged.
In `@packages/desktop-electron/src/main/problem-report.ts`:
- Around line 93-107: The current truncation only reduces messages and logTail
but can still leave markdown(makePayload()) > maxBytes if sessionExport.info or
sessionExport.diagnostics contain large data; update the trimming logic to also
shrink those fields until bytes(output) <= maxBytes. After the existing
message/logTail loops, add one or more loops that (1) for
sessionExport.diagnostics, remove/halve entries (use Math.max(1,
Math.ceil(length/2))) and accumulate omitted bytes, (2) for sessionExport.info,
truncate or remove the largest string fields (or progressively shorten long
string values) similarly, each time recomputing output = markdown(makePayload())
and repeating until bytes(output) <= maxBytes; reference makePayload(),
sessionExport, sessionExport.info, sessionExport.diagnostics, messages, logTail,
bytes(), and markdown() when making the changes.
In `@packages/desktop-electron/src/main/updater.test.ts`:
- Around line 57-73: The test currently expects updater.check() to return "none"
after a ready status, but the ready state must persist until install() is
invoked; update the assertions so the second call to setup.updater.check() still
resolves to { status: "ready", version: "0.2.5" } (and only after calling
setup.updater.install() should state change and setup.calls.install be
incremented), keeping the existing install() assertions to verify install()
flips the state.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 690f52d3-5c48-493e-a255-77bff292f695
📒 Files selected for processing (36)
.github/workflows/build.ymlpackages/app/src/app.tsxpackages/app/src/components/settings-general.tsxpackages/app/src/context/platform.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/error-update.test.tspackages/app/src/pages/error-update.tspackages/app/src/pages/error.tsxpackages/app/src/pages/session.tsxpackages/app/src/utils/desktop-context.test.tspackages/app/src/utils/desktop-context.tspackages/desktop-electron/electron.vite.config.tspackages/desktop-electron/scripts/finalize-latest-yml.tspackages/desktop-electron/scripts/release-metadata-contract.test.tspackages/desktop-electron/src/main/constants.tspackages/desktop-electron/src/main/desktop-context-store.test.tspackages/desktop-electron/src/main/desktop-context-store.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/logging.tspackages/desktop-electron/src/main/menu-i18n.tspackages/desktop-electron/src/main/menu-labels.test.tspackages/desktop-electron/src/main/menu-labels.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/updater-dialog-labels.test.tspackages/desktop-electron/src/main/updater-dialog-labels.tspackages/desktop-electron/src/main/updater.test.tspackages/desktop-electron/src/main/updater.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.tspackages/desktop-electron/src/renderer/index.tsxpackages/opencode/test/github/build-workflow.test.ts
2895858 to
351e1ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 332-336: The current `gh release download ... || echo "No existing
..."` silences all errors (auth, network, API) and lets `finalize-latest-yml.ts`
run without required metadata; change the logic so the script runs `gh release
download` and, on failure, inspects the error message/exit code: if the failure
explicitly indicates "no such asset"/404 (e.g., contains "could not find" or
"Resource not found") then continue (log the expected first-release case),
otherwise abort the workflow with a non-zero exit so auth/network/API errors are
not swallowed; apply this change to both branches that reference RUNNER_OS, `gh
release download` (using the `tag`, `--pattern` and `--dir "$existing_dir"
--repo "$GITHUB_REPOSITORY"` arguments) and ensure `finalize-latest-yml.ts` only
runs when a legitimate "asset not found" is detected.
In `@packages/desktop-electron/src/main/index.ts`:
- Around line 383-385: When setDesktopContext updates the locale (the branch
where next.locale !== menuLocale), persist that change to the same store read by
readStoredMenuLocale: update getStore("opencode.global.dat").set("language",
next.locale) after assigning menuLocale and before/after calling wireMenu so the
stored language matches the in-memory menuLocale and survives restarts; modify
the handler in setDesktopContext to perform this write whenever menuLocale is
changed.
In `@packages/desktop-electron/src/main/updater-dialog-labels.test.ts`:
- Around line 16-21: The test currently calls updaterDialogLabels("en") but is
named to imply fallback behavior; rename the test to reflect actual behavior
(e.g., change the test description to "returns English labels for 'en' locale"
or similar) so it matches the code path exercised, or alternatively update the
call to exercise a real fallback (e.g., call
updaterDialogLabels("unknown-locale") and assert it returns English labels).
Ensure the change references the updaterDialogLabels function and updates the
test description string only (or updates the input locale to an unsupported
value if you choose to test fallback).
In `@packages/desktop-electron/src/main/updater.test.ts`:
- Around line 34-109: Add a regression test that asserts the updater reports
status "disabled" when the controller is created with enabled: false: call
controller({ enabled: false }) (use the same controller helper and
setup.updater.check() symbol), await
expect(setup.updater.check()).resolves.toEqual({ status: "disabled" }) and
ensure no download/install calls are made (check setup.calls.download/install
remain 0) to lock in the disabled behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72bae934-d6c1-45bd-8439-ca69f13088dc
📒 Files selected for processing (36)
.github/workflows/build.ymlpackages/app/src/app.tsxpackages/app/src/components/settings-general.tsxpackages/app/src/context/platform.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/error-update.test.tspackages/app/src/pages/error-update.tspackages/app/src/pages/error.tsxpackages/app/src/pages/session.tsxpackages/app/src/utils/desktop-context.test.tspackages/app/src/utils/desktop-context.tspackages/desktop-electron/electron.vite.config.tspackages/desktop-electron/scripts/finalize-latest-yml.tspackages/desktop-electron/scripts/release-metadata-contract.test.tspackages/desktop-electron/src/main/constants.tspackages/desktop-electron/src/main/desktop-context-store.test.tspackages/desktop-electron/src/main/desktop-context-store.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/logging.tspackages/desktop-electron/src/main/menu-i18n.tspackages/desktop-electron/src/main/menu-labels.test.tspackages/desktop-electron/src/main/menu-labels.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/updater-dialog-labels.test.tspackages/desktop-electron/src/main/updater-dialog-labels.tspackages/desktop-electron/src/main/updater.test.tspackages/desktop-electron/src/main/updater.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.tspackages/desktop-electron/src/renderer/index.tsxpackages/opencode/test/github/build-workflow.test.ts
351e1ba to
359f10d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/desktop-electron/src/main/index.ts`:
- Around line 68-76: The fallback desktop context currently uses a frozen
defaultDesktopContext created from the startup menuLocale, causing stale locale
values when menuLocale changes; update the logic so the store fallback is
derived from the up-to-date menuLocale (or explicitly refresh the store fallback
whenever setDesktopContext updates menuLocale). Locate the defaultDesktopContext
and createDesktopContextStore usage and change the fallback resolution used by
desktopContexts.current(windowID) to read menuLocale dynamically (or call a
method on the desktopContexts store to update its fallback) so functions like
checkForUpdates() and diagnostics() always see the current locale after
setDesktopContext runs.
In `@packages/desktop-electron/src/main/ipc.ts`:
- Around line 37-47: The inline discriminated union return type for the
checkUpdate IPC handler duplicates UpdateInfo from the app package; replace the
inline type with a shared type reference by importing UpdateInfo (or an
equivalent exported type) from packages/app/src/context/platform.tsx into
packages/desktop-electron/src/main/ipc.ts and use that type for the Promise
return (e.g., Promise<UpdateInfo>), or if importing causes circular deps, create
a small shared types module (e.g., a new exported UpdateInfo in a shared package
or packages/app/src/types) and reference that instead; update the checkUpdate
signature to use the shared UpdateInfo type and ensure all call sites still
satisfy the discriminated union shape.
In `@packages/desktop-electron/src/main/menu.test.ts`:
- Around line 40-44: The test relies on menu ordering via template[0] and
template.at(-1) which is fragile; update the assertions to locate menus by label
instead (e.g., replace uses of template[0] with template.find(m => m.label ===
"App" || m.label === "File")?.submenu and replace template.at(-1) with
template.find(m => m.label === "Help")?.submenu) and add a null-safe guard
(fallback to [] if submenu is undefined) so the test fails clearly if the
expected menu label is missing; alternatively, if you prefer to keep positional
access, add a brief comment above the test documenting the assumed menu order
(references: template, appMenu, template.at(-1)).
In `@packages/desktop-electron/src/main/problem-report.ts`:
- Around line 167-169: The regex in parseProblemReportPayload only matches LF
line endings so Windows CRLF reports fail; update the pattern used where
input.match(...) is called to accept optional CR before newlines (use \r?\n) for
both the opening and closing fenced code markers so the JSON block is found on
CRLF and LF inputs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9a773053-7442-4ed0-b26f-836a9c2d7d3d
📒 Files selected for processing (39)
.github/workflows/build.ymlpackages/app/src/app.tsxpackages/app/src/components/settings-general.tsxpackages/app/src/context/platform.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/error-update.test.tspackages/app/src/pages/error-update.tspackages/app/src/pages/error.tsxpackages/app/src/pages/session.tsxpackages/app/src/utils/desktop-context.test.tspackages/app/src/utils/desktop-context.tspackages/desktop-electron/electron.vite.config.tspackages/desktop-electron/scripts/finalize-latest-yml.tspackages/desktop-electron/scripts/release-metadata-contract.test.tspackages/desktop-electron/src/main/constants.tspackages/desktop-electron/src/main/desktop-context-store.test.tspackages/desktop-electron/src/main/desktop-context-store.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/logging.tspackages/desktop-electron/src/main/menu-i18n.tspackages/desktop-electron/src/main/menu-labels.test.tspackages/desktop-electron/src/main/menu-labels.tspackages/desktop-electron/src/main/menu-template.tspackages/desktop-electron/src/main/menu.test.tspackages/desktop-electron/src/main/menu.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/updater-dialog-labels.test.tspackages/desktop-electron/src/main/updater-dialog-labels.tspackages/desktop-electron/src/main/updater.test.tspackages/desktop-electron/src/main/updater.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.tspackages/desktop-electron/src/renderer/index.tsxpackages/opencode/test/github/build-workflow.test.ts
359f10d to
076d0ad
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Astro-Han
left a comment
There was a problem hiding this comment.
Overall quality is high and most Codex/CodeRabbit feedback has already been absorbed. No P0 findings. Posting 7 inline comments: 1 P1 to confirm (concurrency trade-off), plus P2/P3 readability and dead-code items.
Praise worth calling out:
problem-report.tshas clean staged truncation (messages → logTail → sessionInfo → diagnostics) against a byte budget, with unit tests covering each truncation branch.release-metadata-contract.test.tsuses a fake-gh shim to black-box verify the finalize script across platforms — good template.- Already-absorbed review items:
constants.tstrim,desktop-context-store.tsMap re-insertion order,parseProblemReportPayloadnon-greedy + CRLF,finalize-latest-yml.tsmissing-asset vs real-error split,updater.tsDeps.checkForUpdates allowing null.
Only P1 needs confirmation before merge.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
076d0ad to
fa829dc
Compare
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
68dbe0c to
79f2ecf
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Astro-Han
left a comment
There was a problem hiding this comment.
Review
A few nits and questions:
1. updater.ts — install() returns boolean but callers ignore it
install() returns true/false to indicate whether it actually triggered the install, but index.ts:installUpdate() just calls updater.install() and discards the result. Consider either using the return value or changing the signature to void.
2. menu-template.ts — checkForUpdates menu item lost enabled: UPDATER_ENABLED
The old menu had enabled: UPDATER_ENABLED on the "Check for Updates..." item. The new template always enables it. If the updater is disabled, clicking it will silently do nothing (the handler in index.ts still guards with UPDATER_ENABLED, but the menu item is no longer greyed out). Was this intentional?
3. problem-report.ts — truncateString is a no-op for the first 1024 chars
truncateString(value, 1024) returns value unchanged when value.length <= 1024. The first iteration of the diagnostics-truncation loop therefore does nothing except increment omittedDiagnosticsBytes to 0. You could start diagnosticStringLimit at 512 to skip the wasted iteration.
4. index.ts — sessionExport mixes fetch timeout with AbortController
The 10-second timeout uses AbortController, but fetch in Electron’s main process may not always respect AbortSignal depending on the Node version. Consider adding a Promise.race with a manual timeout rejection for safety.
5. menu-i18n.ts — writeStoredMenuLocale stores a JSON string, but readStoredMenuLocale parses it back
writeStoredMenuLocale does JSON.stringify({ locale }), then readStoredMenuLocale calls parseMenuLocale which does JSON.parse. This round-trip is harmless but slightly odd — the store already serializes values. Storing the raw string \"zh\" or \"en\" would be simpler and avoid the extra parse.
6. build.yml — downloadExistingMetadataStep condition is complex and duplicated
The if condition appears verbatim in three places (download step, collect step, finalize step). A workflow-level env or a reusable expression would reduce duplication and the risk of the conditions drifting out of sync.
7. feedback.ts — errorMessage is duplicated from updater.ts
Both files define an identical errorMessage helper. Consider extracting it to a shared utility to keep error formatting consistent.
8. index.ts — syncMenuLocaleForWindow calls wireMenu() on every focus change
If a user rapidly switches between two windows with different locales, wireMenu() rebuilds the entire application menu each time. In practice this is probably fine, but it’s worth noting that macOS menu rebuilding is not free and can cause subtle flicker or accessibility tree churn.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.github/workflows/build.yml (1)
334-345:⚠️ Potential issue | 🟠 MajorRestrict
download_or_warn()to asset-specific stderr.The current grep still matches generic
"not found"failures, so a bad tag, missing release, or repo/token problem can be logged as an expected first-release case instead of failing here. Keep this allowlist aligned withpackages/desktop-electron/scripts/finalize-latest-yml.tsand only continue on messages that unambiguously mean “no matching asset.”Suggested fix
- if grep -qiE 'not found|no assets match|could not find|resource not found' "$err"; then + if grep -qiE 'no assets to download|no matches found|could not find any assets|no assets match' "$err"; then echo "No existing $pattern found; expected for first release." return 0 fiFor GitHub CLI `gh release download`, what stderr strings specifically indicate (1) no asset matched `--pattern`, versus (2) release/tag/repository not found or insufficient access?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 334 - 345, The download_or_warn function currently treats any stderr containing "not found" as an expected-first-release; tighten the grep to only allow asset-specific messages by matching explicit asset-mismatch phrases (e.g. case-insensitive patterns like "no assets match", "no matching assets", "could not find any assets", or "no assets were found") so that release/tag/repo/auth errors (e.g. "release not found", "resource not found", "could not find release") still cause a non-zero exit; update the grep in download_or_warn (using the pattern variable and $err) to only accept those asset-specific strings (to mirror packages/desktop-electron/scripts/finalize-latest-yml.ts) and leave all other stderr contents to be printed and return 1.packages/desktop-electron/src/main/index.ts (1)
552-554:⚠️ Potential issue | 🟠 MajorDon’t discard the updater controller’s
falseinstall result.
createUpdaterController.install()usesfalseto mean “no downloaded update is ready”, but this wrapper resolvesvoideither way. That leaves renderer/menu callers with a dead “Restart to Update” action whenever another path has already calleddismissReady()orinstallUpdate()is triggered twice.🛠️ Suggested fix
async function installUpdate() { - updater.install() + if (!updater.install()) { + throw new Error("No downloaded update is ready to install") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-electron/src/main/index.ts` around lines 552 - 554, The wrapper installUpdate currently calls updater.install() and discards its boolean result; change installUpdate to return the value from createUpdaterController.install() (i.e., return await updater.install()) so callers receive false when no downloaded update is ready (which prevents dead “Restart to Update” actions after dismissReady()/double installs). Update the function signature/return type accordingly and propagate the boolean to any caller code that expects the install result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/src/pages/session.tsx`:
- Around line 350-368: The completion handlers for syncDesktopContext are
touching desktopContextRetryTimer even when the completion is for a stale
payload; update both the .then and .catch branches to first check that
pendingDesktopContext === serialized (and return if not) before clearing or
setting desktopContextRetryTimer, so only the current pending payload can cancel
or schedule retries; use the existing symbols pendingDesktopContext,
lastDesktopContext, desktopContextRetryTimer, disposed, serialized, and
syncDesktopContext to locate the logic to guard.
In `@packages/desktop-electron/src/main/index.ts`:
- Line 183: The onError handler currently only calls logger.error and leaves
users unaware when the feedback flow fails; update the onError passed to
createFeedbackHandler (the onError lambda in index.ts) to also surface a
user-visible message (e.g., via Electron dialog.showErrorBox or
dialog.showMessageBox / a toast) that explains the failure (e.g., "Could not
copy report to clipboard" or "Could not open link") and include the error
message/details; keep the existing logger.error call but add the
dialog/notification so clipboard or shell.openExternal failures are visible to
the user.
In `@packages/desktop-electron/src/main/menu-labels.ts`:
- Around line 1-2: The MenuLocale type is overly restrictive and
parseMenuLocale() and detectSystemMenuLocale() collapse non-zh locales to "en";
update MenuLocale to include all supported locales (e.g., ja, de, fr, ko, etc.)
or stop using MenuLocale for translations and instead pull desktop menu/updater
strings from the shared app i18n tables. Concretely, extend the exported
MenuLocale union to list all locales the app supports and adjust
parseMenuLocale() and detectSystemMenuLocale() to map/validate against that
expanded set, or refactor menu-label resolution to call the app i18n lookup
functions (reuse the existing i18n keys) rather than returning a two-value
MenuLocale.
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 334-345: The download_or_warn function currently treats any stderr
containing "not found" as an expected-first-release; tighten the grep to only
allow asset-specific messages by matching explicit asset-mismatch phrases (e.g.
case-insensitive patterns like "no assets match", "no matching assets", "could
not find any assets", or "no assets were found") so that release/tag/repo/auth
errors (e.g. "release not found", "resource not found", "could not find
release") still cause a non-zero exit; update the grep in download_or_warn
(using the pattern variable and $err) to only accept those asset-specific
strings (to mirror packages/desktop-electron/scripts/finalize-latest-yml.ts) and
leave all other stderr contents to be printed and return 1.
In `@packages/desktop-electron/src/main/index.ts`:
- Around line 552-554: The wrapper installUpdate currently calls
updater.install() and discards its boolean result; change installUpdate to
return the value from createUpdaterController.install() (i.e., return await
updater.install()) so callers receive false when no downloaded update is ready
(which prevents dead “Restart to Update” actions after dismissReady()/double
installs). Update the function signature/return type accordingly and propagate
the boolean to any caller code that expects the install result.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1eeb94fc-40ef-4e7e-ac8c-a7aca08efb6e
📒 Files selected for processing (39)
.github/workflows/build.ymlpackages/app/src/app.tsxpackages/app/src/components/settings-general.tsxpackages/app/src/context/platform.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/app/src/pages/error-update.test.tspackages/app/src/pages/error-update.tspackages/app/src/pages/error.tsxpackages/app/src/pages/session.tsxpackages/app/src/utils/desktop-context.test.tspackages/app/src/utils/desktop-context.tspackages/desktop-electron/electron.vite.config.tspackages/desktop-electron/scripts/finalize-latest-yml.tspackages/desktop-electron/scripts/release-metadata-contract.test.tspackages/desktop-electron/src/main/constants.tspackages/desktop-electron/src/main/desktop-context-store.test.tspackages/desktop-electron/src/main/desktop-context-store.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/logging.tspackages/desktop-electron/src/main/menu-i18n.tspackages/desktop-electron/src/main/menu-labels.test.tspackages/desktop-electron/src/main/menu-labels.tspackages/desktop-electron/src/main/menu-template.tspackages/desktop-electron/src/main/menu.test.tspackages/desktop-electron/src/main/menu.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/updater-dialog-labels.test.tspackages/desktop-electron/src/main/updater-dialog-labels.tspackages/desktop-electron/src/main/updater.test.tspackages/desktop-electron/src/main/updater.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.tspackages/desktop-electron/src/renderer/index.tsxpackages/opencode/test/github/build-workflow.test.ts
79f2ecf to
480db64
Compare
|
Closing in favor of a fresh replacement PR from the same branch after resolving the accumulated review threads. |
Summary
PAWWORK_FEEDBACK_FORM_URL.Why
Fixes desktop menu cleanup and update feedback gaps from #84. Also gives users without GitHub accounts a lower-friction Feishu form based bug report path while preserving the GitHub issue entry for developer workflows.
This also addresses the real v0.2.5 release failure mode: the release published successfully, but
latest-mac.ymlended up containing onlypawwork-mac-x64.zip/.dmgand dropped the arm64 entries. The old workflow packaged each macOS architecture independently and did not run a final metadata merge/upload step. This PR adds that workflow step and hardensfinalize-latest-yml.tsso existing, live, and current-run updater metadata are merged beforelatest*.ymlis re-uploaded.Related Issue
Fixes #84
How To Verify
Manual/config checks:
latest-mac.ymlonly includes x64 metadata, validating the release metadata clobbering path this PR fixes.PawWork Problem Reportin theCENO&纯刻用户服务project knowledge space.shared=true,shared_limit=anyone_editable.PAWWORK_FEEDBACK_FORM_URL.Screenshots or Recordings
Not included. Changes are native menu/dialog flows plus release metadata behavior. The external Feishu form was created and configured separately, and its URL is supplied by repository variable at build time.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit