fix: improve feedback problem report upload flow#179
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughFeedback now copies a short summary to the clipboard and writes a full problem-report markdown to disk (deterministic filename), attempts to reveal/open the saved file, cleans old reports, uses an AbortSignal-aware session export with timeout, and surfaces secondary failures via an optional handled-error hook while preventing concurrent runs. Changes
Sequence DiagramsequenceDiagram
actor User
participant Dialog as Feedback Dialog
participant Handler as Feedback Handler
participant ReportGen as Problem Report Builder
participant Session as Session Export
participant FileOps as File Persistence
participant Logger as onHandledError/onError
User->>Dialog: confirm
Dialog->>Handler: onConfirm()
Handler->>Handler: acquire inFlight guard
Handler->>ReportGen: buildProblemReportSummary()
ReportGen-->>Handler: short summary
Handler->>Session: sessionExport(context, signal)
note right of Session: timeout wrapper with AbortController
Session-->>Handler: session data or failure
Handler->>ReportGen: buildProblemReport(reportId, generatedAt)
ReportGen-->>Handler: {markdown, reportId, generatedAt}
Handler->>FileOps: saveReport(markdown) -> writeProblemReportFile
FileOps-->>Handler: savedPath, fileName
Handler->>FileOps: showItemInFolder(savedPath)
alt reveal succeeds
FileOps-->>Handler: revealed
else reveal fails
FileOps-->>Handler: error
Handler->>FileOps: openPath(dirname) %% fallback
FileOps-->>Handler: opened or error
Handler->>Logger: onHandledError(error)
end
Handler->>Dialog: copy summary to clipboard
Dialog->>User: open external form
Handler->>FileOps: cleanupReports(root, keep, currentPath)
FileOps-->>Handler: cleanup result
Handler->>Handler: release inFlight guard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/feedback.ts`:
- Around line 73-125: reportId() duplicates defaultReportId()—remove the local
reportId function and instead import and reuse defaultReportId from
problem-report.ts; export defaultReportId (or rename/export an equivalent) from
the ProblemReport module and replace any local calls to reportId() in
feedback.ts with the imported defaultReportId to ensure a single source of truth
for report ID generation.
🪄 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: 4f751938-0ebe-4400-b839-148a09f5c2fa
📒 Files selected for processing (7)
packages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: smoke-macos-arm64
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-opencode
- GitHub Check: unit-desktop
- GitHub Check: unit-app
- GitHub Check: typecheck
- GitHub Check: analyze-js-ts
- GitHub Check: e2e-artifacts
🧰 Additional context used
📓 Path-based instructions (1)
packages/desktop-electron/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/desktop-electron/AGENTS.md)
Renderer process should only call
window.apifromsrc/preload
Files:
packages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/feedback.test.ts
🧠 Learnings (15)
📚 Learning: 2026-04-20T14:36:08.745Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/desktop-electron/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:08.745Z
Learning: Applies to packages/desktop-electron/src/main/ipc.ts : Main process should register IPC handlers in `src/main/ipc.ts`
Applied to files:
packages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-22T05:24:42.404Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 98
File: packages/desktop-electron/src/main/menu-labels.ts:1-2
Timestamp: 2026-04-22T05:24:42.404Z
Learning: In Astro-Han/pawwork, the app i18n layer (`packages/app/src/i18n/`) only contains `en.ts` and `zh.ts`, and `normalizeLocale` (in `packages/app/src/context/language.tsx`) only returns `"en"` or `"zh"`. The desktop `MenuLocale = "en" | "zh"` union in `packages/desktop-electron/src/main/menu-labels.ts` is intentionally limited to these two locales and is not a broader restriction — do not flag it as overly restrictive or suggest adding other locales.
Applied to files:
packages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `tmpdir` function from `fixture/fixture.ts` to create temporary directories for tests with automatic cleanup. Use `await using` syntax to ensure automatic cleanup when the variable goes out of scope.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-22T08:49:44.563Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:44.563Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Prefer Effect-aware helpers from `fixture/fixture.ts` over building manual runtimes in tests: use `tmpdirScoped()` for scoped temp directories, `provideInstance(dir)(effect)` for low-level binding without directory creation, `provideTmpdirInstance(...)` for single temp instance binding, or `provideTmpdirServer(...)` for tests that also need the test LLM server.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : When a test needs instance-local state, prefer `provideTmpdirInstance(...)` or `provideInstance(...)` over manual `Instance.provide(...)` inside Promise-style tests.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use `it.effect(...)` when the test should run with `TestClock` and `TestConsole`. Use `it.live(...)` when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test files should be named with the pattern `feature-name.spec.ts`
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `init` option in `tmpdir` to define custom setup functions that can return extra data accessible via `tmp.extra`, and use the `dispose` option for custom cleanup logic.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `config` option in `tmpdir` to write an `opencode.json` config file during test setup by passing a partial Config.Info object.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Import test utilities from `../fixtures` instead of `playwright/test`
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : When using the `tmpdir` function with git repository support, pass the `git: true` option to initialize a git repo with a root commit.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `expect.poll(...)` for probing mock or backend state rather than transient DOM visibility
Applied to files:
packages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/desktop-electron/src/main/feedback.test.ts
🔇 Additional comments (27)
packages/desktop-electron/src/main/index.ts (2)
35-35: LGTM!The timeout constant, imports, and report root computation are correctly wired. The 3-second timeout is reasonable for the session export operation, and computing
problemReportRootonce at initialization is appropriate sinceuserDatapath is stable.Also applies to: 61-61, 105-105
186-224: LGTM!The feedback handler wiring correctly passes all new dependencies:
- File operations (
saveReport,cleanupReports) are properly bound with the report root- Reveal/open fallbacks use
shell.showItemInFolderandshell.openPath- The 10-report retention limit is reasonable
onHandledErrorappropriately routes secondary failures to the loggerpackages/desktop-electron/src/main/problem-report-files.test.ts (4)
13-24: LGTM!The temporary directory management pattern is well-implemented with proper cleanup in
afterEach. Usingrmwithrecursive: true, force: trueensures reliable cleanup even if tests fail.
27-40: LGTM!The test correctly validates that filenames are generated using local time components and include the reportId for collision resistance.
42-60: LGTM!The test correctly validates the non-destructive write semantics: duplicate writes are rejected and original content is preserved.
62-101: LGTM!Excellent test coverage for location hints and cleanup behavior. The cleanup test comprehensively validates that:
- Current report is preserved
- Non-matching files are skipped
- Non-regular entries (directories, symlinks) are skipped
- Only matching regular files beyond the retention limit are deleted
packages/desktop-electron/src/main/problem-report.test.ts (5)
34-45: LGTM!The test correctly validates that caller-provided
reportIdandgeneratedAtvalues are propagated both in the function return and in the parsed payload.
47-80: LGTM!The tests correctly validate:
- Updated markdown instruction text for the file upload workflow
- Required
reportIdin payloads- Summary content inclusion/exclusion rules (no sensitive paths, logs, or session data)
- Compact summary size constraint (≤28 lines)
82-132: LGTM!Comprehensive coverage for failure modes and error handling:
- Failed report status messaging with fallback instructions
- Error line limit (10) and single-line extraction
- Truncation of oversized error lines to maintain compact summary (<1000 chars)
134-226: LGTM!Thorough sanitization test coverage:
- Route query parameters and hash fragments are stripped (prevents prompt leakage)
- Session IDs are truncated and path-cleaned
- Platform-specific filesystem paths are redacted (Windows, Linux, macOS, network paths)
- All tests verify the summary remains compact
336-430: LGTM!JSON fixtures correctly updated to include the now-required
reportIdfield, and the parser validation test correctly verifies that incomplete payloads (missinggeneratedAt) are still rejected.packages/desktop-electron/src/main/feedback.test.ts (4)
20-71: LGTM!The test setup correctly mocks all new dependencies:
- File operations (
saveReport,showItemInFolder,openPath,cleanupReports)- Call trackers for verification (
shown,openedPath,savedMarkdown,handledErrors)- Fast timeout (10ms) for test efficiency
- Separate tracking for handled vs fatal errors
73-94: LGTM!Dialog label tests correctly validate the updated user-facing copy that explains the new two-artifact workflow (clipboard summary + full report file for upload).
96-136: LGTM!The tests correctly validate:
- Separation of clipboard summary vs saved full report
- File reveal behavior
- Concurrency guard preventing duplicate operations
- Guard release on cancel
164-263: LGTM!Excellent failure scenario coverage:
- Session export failures/timeouts still produce report artifacts
- File write failures provide graceful degradation (summary-only)
- Diagnostics failures fall back to minimum summary
- Non-critical failures (reveal, cleanup) use
onHandledErrorand don't block the form- Critical failures (form open, copy) properly report errors
packages/desktop-electron/src/main/problem-report.ts (5)
1-63: LGTM!Well-defined constants for summary field limits and properly extended types. The
Optionstype correctly makesreportIdandgeneratedAtoptional (with defaults generated internally), while thePayloadtype requires them for validation.
69-88: LGTM!The
defaultReportId()generates collision-resistant IDs with a compact format (base36 timestamp + random suffix), and the markdown instruction text correctly reflects the file upload workflow.
160-242: LGTM!
buildProblemReportcorrectly:
- Uses default values for
reportIdandgeneratedAtwhen not provided- Includes them in the payload
- Returns all three values for downstream consumers (feedback handler needs them for file naming and summary)
255-323: LGTM!The summary sanitization functions provide comprehensive protection:
redactLocalPathFragments()handles Windows (drive letters, UNC), macOS (/Users), Linux (/home,/tmp), and common temp directoriessafeSummaryRoute()strips query parameters and hash fragments (prevents prompt leakage)- Error lines are single-lined, truncated, and capped at 10
- The summary format is well-structured and consistently safe
375-388: LGTM!Payload validation correctly requires
reportIdto be a non-empty string, ensuring parsed payloads conform to the updated schema.packages/desktop-electron/src/main/feedback.ts (3)
1-41: LGTM!The imports and type definitions correctly support the new workflow:
dirnamefor parent folder fallback on reveal failureSavedReportandSaveReportInputtypes match the file utilities contractFeedbackDepsextended with all required dependencies for the two-artifact flow
43-71: LGTM!The dialog labels clearly explain the new workflow to users:
- Summary copied to clipboard
- Full report saved locally for upload
- Post-submission cleanup instructions
127-219: LGTM!The feedback handler implementation is robust:
- Concurrency guard via
inFlightprevents duplicate operations- Context is captured before confirmation to handle focus changes
- Comprehensive error handling with graceful degradation at each step
- Secondary failures (reveal, cleanup) use
onHandledErrorwithout blocking the form- Users always receive at least a summary even when the full report fails
packages/desktop-electron/src/main/problem-report-files.ts (4)
1-21: LGTM!The filename generation correctly:
- Validates
generatedAtas a valid ISO timestamp- Uses local time components for human-readable sorting
- Produces a deterministic, collision-resistant filename with the reportId
23-30: LGTM!The utility functions correctly:
problemReportsRoot()computes the reports directory pathreportLocationHint()provides a privacy-respecting hint with platform-appropriate prefix
32-60: LGTM!The atomic write implementation is robust:
- Exclusive create (
wx) for temp file prevents overwrites- Hardlink provides atomic final file creation
EEXISTon link correctly indicates collision (not retry-safe)- Temp file cleanup in both success and error paths
- Restrictive permissions (
0o600) for user-only access
62-90: LGTM!The cleanup logic correctly:
- Excludes the current report from consideration
- Only processes matching regular files (skips directories, symlinks)
- Sorts by mtime (newest first) to preserve recent reports
- Keeps
keep - 1files from the list (plus the excluded current =keeptotal)- Gracefully handles missing directories and stat/delete errors
There was a problem hiding this comment.
Code Review
This pull request refactors the problem reporting system to save full reports as local files while copying a redacted summary to the clipboard. It introduces a more robust error handling mechanism, localized labels, and automated cleanup of old reports. Feedback focuses on improving the reliability of asynchronous operations and filesystem interactions. Specifically, it is noted that shell.openPath returns an error string rather than throwing, requiring a manual check to trigger fallback logic. Additionally, using fs.promises.rename instead of link is recommended for atomic file operations across different partitions, and passing abort signals to timed-out session exports would prevent background resource usage.
dd9553f to
dc9bdc0
Compare
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/feedback.ts`:
- Around line 158-162: The catch block for sessionExport loses the original
error by replacing it with safeFailureReason(error); instead, preserve the real
error in sessionExport so the saved report includes the actual fetch/status
error: when sessionExportWithTimeout throws, set sessionExport = { status:
"failed", error: error } (letting the downstream problem-report truncation
handle oversized values) rather than calling safeFailureReason; update the catch
around sessionExportWithTimeout to assign the original thrown error to the
sessionExport.error field.
In `@packages/desktop-electron/src/main/problem-report.ts`:
- Around line 160-165: In buildProblemReport, validate any caller-provided
overrides before using them: ensure options.reportId (used with
defaultReportId()) is a non-empty string (trim and reject empty) and ensure
options.generatedAt is a valid ISO timestamp (parseable to a Date and finite)
before assigning generatedAt; if either is invalid, either fall back to
defaultReportId()/new Date().toISOString() or throw a clear error so downstream
functions like parseProblemReportPayload() and problemReportFileName() never
receive invalid values. Locate and update buildProblemReport to perform these
checks on options.reportId and options.generatedAt immediately before they are
assigned to reportId and generatedAt.
🪄 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: 95d36af9-752b-497c-a24b-119be6b66e73
📒 Files selected for processing (7)
packages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
packages/desktop-electron/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/desktop-electron/AGENTS.md)
Renderer process should only call
window.apifromsrc/preload
Files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/feedback.test.ts
🧠 Learnings (21)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 98
File: packages/desktop-electron/src/main/menu-labels.ts:1-2
Timestamp: 2026-04-22T05:24:42.404Z
Learning: In Astro-Han/pawwork, the app i18n layer (`packages/app/src/i18n/`) only contains `en.ts` and `zh.ts`, and `normalizeLocale` (in `packages/app/src/context/language.tsx`) only returns `"en"` or `"zh"`. The desktop `MenuLocale = "en" | "zh"` union in `packages/desktop-electron/src/main/menu-labels.ts` is intentionally limited to these two locales and is not a broader restriction — do not flag it as overly restrictive or suggest adding other locales.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/ui/src/theme/context.tsx:11-16
Timestamp: 2026-04-22T09:32:52.535Z
Learning: In Astro-Han/pawwork (`packages/ui/src/theme/context.tsx` and related files), the renaming of localStorage theme keys from `opencode-*` to `pawwork-*` (THEME_ID, COLOR_SCHEME, THEME_CSS_LIGHT, THEME_CSS_DARK) is intentional and should NOT include a migration path from the old keys. Migrating would re-couple PawWork and OpenCode browser storage namespaces, which the PR is explicitly designed to avoid. A reset to the PawWork default theme on upgrade is acceptable by design.
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `tmpdir` function from `fixture/fixture.ts` to create temporary directories for tests with automatic cleanup. Use `await using` syntax to ensure automatic cleanup when the variable goes out of scope.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-22T08:49:44.563Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:44.563Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : When a test needs instance-local state, prefer `provideTmpdirInstance(...)` or `provideInstance(...)` over manual `Instance.provide(...)` inside Promise-style tests.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use `it.effect(...)` when the test should run with `TestClock` and `TestConsole`. Use `it.live(...)` when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Prefer Effect-aware helpers from `fixture/fixture.ts` over building manual runtimes in tests: use `tmpdirScoped()` for scoped temp directories, `provideInstance(dir)(effect)` for low-level binding without directory creation, `provideTmpdirInstance(...)` for single temp instance binding, or `provideTmpdirServer(...)` for tests that also need the test LLM server.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `init` option in `tmpdir` to define custom setup functions that can return extra data accessible via `tmp.extra`, and use the `dispose` option for custom cleanup logic.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `config` option in `tmpdir` to write an `opencode.json` config file during test setup by passing a partial Config.Info object.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Call `project.trackSession(sessionID, directory?)` and `project.trackDirectory(directory)` for any resources created outside the fixture so teardown can clean them up
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test files should be named with the pattern `feature-name.spec.ts`
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : When using the `tmpdir` function with git repository support, pass the `git: true` option to initialize a git repo with a root commit.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:08.745Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/desktop-electron/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:08.745Z
Learning: Applies to packages/desktop-electron/src/main/ipc.ts : Main process should register IPC handlers in `src/main/ipc.ts`
Applied to files:
packages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-20T14:36:21.274Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.274Z
Learning: Applies to packages/opencode/**/*.ts : Prefer `FileSystem.FileSystem` instead of raw `fs/promises` for effectful file I/O in Effect services
Applied to files:
packages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : In terminal tests, type through the browser using `runTerminal()` and `waitTerminalReady()` instead of writing to the PTY through the SDK
Applied to files:
packages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Import test utilities from `../fixtures` instead of `playwright/test`
Applied to files:
packages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report.test.ts
📚 Learning: 2026-04-22T05:24:42.404Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 98
File: packages/desktop-electron/src/main/menu-labels.ts:1-2
Timestamp: 2026-04-22T05:24:42.404Z
Learning: In Astro-Han/pawwork, the app i18n layer (`packages/app/src/i18n/`) only contains `en.ts` and `zh.ts`, and `normalizeLocale` (in `packages/app/src/context/language.tsx`) only returns `"en"` or `"zh"`. The desktop `MenuLocale = "en" | "zh"` union in `packages/desktop-electron/src/main/menu-labels.ts` is intentionally limited to these two locales and is not a broader restriction — do not flag it as overly restrictive or suggest adding other locales.
Applied to files:
packages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Never use wall-clock waits like `page.waitForTimeout(...)` to make a test pass
Applied to files:
packages/desktop-electron/src/main/feedback.ts
📚 Learning: 2026-04-20T14:21:51.047Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 71
File: packages/app/src/components/session/session-status-connections.tsx:146-147
Timestamp: 2026-04-20T14:21:51.047Z
Learning: In the Astro-Han/pawwork repository (SolidJS app), `sync.data.config` is always initialized to `{}` at `packages/app/src/context/global-sync.tsx` line 71 and is never `undefined` at runtime. Non-optional property access like `sync.data.config.plugin` is intentional and consistent with the pattern used in `packages/app/src/components/status-popover-body.tsx` line 243. Do not flag `sync.data.config.plugin` as needing optional chaining.
Applied to files:
packages/desktop-electron/src/main/feedback.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `expect.poll(...)` for probing mock or backend state rather than transient DOM visibility
Applied to files:
packages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/desktop-electron/src/main/feedback.test.ts
dc9bdc0 to
48c3b12
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/feedback.ts`:
- Line 208: The call to deps.openExternal(deps.feedbackUrl) can throw and
currently lets the flow hit the generic onError path; wrap this call in a
try/catch inside the feedback flow (where deps.openExternal is invoked) so
failures are handled locally: on error, log the exception, do NOT rethrow, and
surface a manual fallback that presents deps.feedbackUrl to the user (e.g., show
a dialog or notification with the URL and an action to copy it) so the prepared
summary/markdown is not treated as a full failure; reference deps.openExternal
and deps.feedbackUrl when locating the change and ensure no existing onError
path is triggered by this catch.
In `@packages/desktop-electron/src/main/problem-report-files.ts`:
- Around line 6-20: The filename builder problemReportFileName currently
interpolates reportId directly which can allow path separators or unsafe chars;
update problemReportFileName to validate reportId against a whitelist (e.g.
/^[A-Za-z0-9_]+$/) before using it, and throw a clear error (or return a
failure) if the value does not match; reference the function
problemReportFileName and the existing cleanup code/REPORT_FILE_PATTERN in the
repo so you ensure the same allowed character class is used and invalid reportId
values are rejected rather than written into the filename.
In `@packages/desktop-electron/src/main/problem-report.ts`:
- Around line 163-166: The generatedAt check currently uses Date.parse which
accepts date-only strings; tighten validation by requiring a canonical ISO 8601
timestamp (full datetime + timezone) — e.g., validate that
Date.parse(generatedAt) is valid and that new Date(generatedAt).toISOString()
=== generatedAt (or otherwise match a strict ISO-datetime regex) before
accepting it; update the validation in the block that sets reportId/generatedAt
(referencing generatedAt and reportId) so problemReportFileName receives a
canonical timestamp.
🪄 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: d6a69724-541c-402f-90d9-4cb9a5659535
📒 Files selected for processing (7)
packages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: smoke-macos-arm64
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-opencode
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-desktop
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-app
- GitHub Check: typecheck
- GitHub Check: e2e-artifacts
- GitHub Check: analyze-js-ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/desktop-electron/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/desktop-electron/AGENTS.md)
Renderer process should only call
window.apifromsrc/preload
Files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/problem-report-files.ts
🧠 Learnings (20)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 98
File: packages/desktop-electron/src/main/menu-labels.ts:1-2
Timestamp: 2026-04-22T05:24:42.404Z
Learning: In Astro-Han/pawwork, the app i18n layer (`packages/app/src/i18n/`) only contains `en.ts` and `zh.ts`, and `normalizeLocale` (in `packages/app/src/context/language.tsx`) only returns `"en"` or `"zh"`. The desktop `MenuLocale = "en" | "zh"` union in `packages/desktop-electron/src/main/menu-labels.ts` is intentionally limited to these two locales and is not a broader restriction — do not flag it as overly restrictive or suggest adding other locales.
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `tmpdir` function from `fixture/fixture.ts` to create temporary directories for tests with automatic cleanup. Use `await using` syntax to ensure automatic cleanup when the variable goes out of scope.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Prefer Effect-aware helpers from `fixture/fixture.ts` over building manual runtimes in tests: use `tmpdirScoped()` for scoped temp directories, `provideInstance(dir)(effect)` for low-level binding without directory creation, `provideTmpdirInstance(...)` for single temp instance binding, or `provideTmpdirServer(...)` for tests that also need the test LLM server.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : When a test needs instance-local state, prefer `provideTmpdirInstance(...)` or `provideInstance(...)` over manual `Instance.provide(...)` inside Promise-style tests.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-22T08:49:44.563Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:44.563Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `init` option in `tmpdir` to define custom setup functions that can return extra data accessible via `tmp.extra`, and use the `dispose` option for custom cleanup logic.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `config` option in `tmpdir` to write an `opencode.json` config file during test setup by passing a partial Config.Info object.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test files should be named with the pattern `feature-name.spec.ts`
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : When using the `tmpdir` function with git repository support, pass the `git: true` option to initialize a git repo with a root commit.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use `it.effect(...)` when the test should run with `TestClock` and `TestConsole`. Use `it.live(...)` when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:21.274Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.274Z
Learning: Applies to packages/opencode/**/*.ts : Prefer `FileSystem.FileSystem` instead of raw `fs/promises` for effectful file I/O in Effect services
Applied to files:
packages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : In terminal tests, type through the browser using `runTerminal()` and `waitTerminalReady()` instead of writing to the PTY through the SDK
Applied to files:
packages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Import test utilities from `../fixtures` instead of `playwright/test`
Applied to files:
packages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-20T14:36:08.745Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/desktop-electron/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:08.745Z
Learning: Applies to packages/desktop-electron/src/main/ipc.ts : Main process should register IPC handlers in `src/main/ipc.ts`
Applied to files:
packages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-22T05:24:42.404Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 98
File: packages/desktop-electron/src/main/menu-labels.ts:1-2
Timestamp: 2026-04-22T05:24:42.404Z
Learning: In Astro-Han/pawwork, the app i18n layer (`packages/app/src/i18n/`) only contains `en.ts` and `zh.ts`, and `normalizeLocale` (in `packages/app/src/context/language.tsx`) only returns `"en"` or `"zh"`. The desktop `MenuLocale = "en" | "zh"` union in `packages/desktop-electron/src/main/menu-labels.ts` is intentionally limited to these two locales and is not a broader restriction — do not flag it as overly restrictive or suggest adding other locales.
Applied to files:
packages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Never use wall-clock waits like `page.waitForTimeout(...)` to make a test pass
Applied to files:
packages/desktop-electron/src/main/feedback.ts
📚 Learning: 2026-04-20T14:21:51.047Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 71
File: packages/app/src/components/session/session-status-connections.tsx:146-147
Timestamp: 2026-04-20T14:21:51.047Z
Learning: In the Astro-Han/pawwork repository (SolidJS app), `sync.data.config` is always initialized to `{}` at `packages/app/src/context/global-sync.tsx` line 71 and is never `undefined` at runtime. Non-optional property access like `sync.data.config.plugin` is intentional and consistent with the pattern used in `packages/app/src/components/status-popover-body.tsx` line 243. Do not flag `sync.data.config.plugin` as needing optional chaining.
Applied to files:
packages/desktop-electron/src/main/feedback.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `expect.poll(...)` for probing mock or backend state rather than transient DOM visibility
Applied to files:
packages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/desktop-electron/src/main/feedback.test.ts
🔇 Additional comments (1)
packages/desktop-electron/src/main/index.ts (1)
137-163: Abort propagation is wired cleanly.Bridging the optional external
AbortSignalinto a local controller and unregistering the listener infinallyavoids the timeout leak/race that usually shows up in this pattern.
6a8732f to
6f09aff
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/problem-report-files.ts`:
- Around line 1-10: The isCanonicalIsoTimestamp implementation in
problem-report-files.ts is duplicated from problem-report.ts; extract that logic
into a shared utility function (e.g., export function
isCanonicalIsoTimestamp(value: string)) in a common module and update both
problem-report-files.ts and problem-report.ts to import and use that exported
function instead of keeping local copies; remove the local
isCanonicalIsoTimestamp definition from problem-report-files.ts, update imports
to reference the new shared symbol, and run types/checks to ensure no
unused/duplicate definitions remain.
- Line 92: The retention slice is off-by-one: in the loop that starts with "for
(const entry of entries.slice(Math.max(0, input.keep - 1)))" change the start
index to Math.max(0, input.keep) so that when input.keep is 10 you keep 10
entries rather than 9; update the slice call referencing entries and input.keep
(the same statement that begins the for loop) to use input.keep instead of
input.keep - 1.
🪄 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: b3d9fc72-a90c-425e-acc8-a3ca6c86c320
📒 Files selected for processing (7)
packages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-session
- GitHub Check: typecheck
- GitHub Check: unit-desktop
- GitHub Check: smoke-macos-arm64
- GitHub Check: e2e-artifacts
- GitHub Check: analyze-js-ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/desktop-electron/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/desktop-electron/AGENTS.md)
Renderer process should only call
window.apifromsrc/preload
Files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/feedback.test.ts
🧠 Learnings (25)
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `tmpdir` function from `fixture/fixture.ts` to create temporary directories for tests with automatic cleanup. Use `await using` syntax to ensure automatic cleanup when the variable goes out of scope.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-22T08:49:44.563Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:44.563Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test files should be named with the pattern `feature-name.spec.ts`
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Prefer Effect-aware helpers from `fixture/fixture.ts` over building manual runtimes in tests: use `tmpdirScoped()` for scoped temp directories, `provideInstance(dir)(effect)` for low-level binding without directory creation, `provideTmpdirInstance(...)` for single temp instance binding, or `provideTmpdirServer(...)` for tests that also need the test LLM server.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `init` option in `tmpdir` to define custom setup functions that can return extra data accessible via `tmp.extra`, and use the `dispose` option for custom cleanup logic.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : When a test needs instance-local state, prefer `provideTmpdirInstance(...)` or `provideInstance(...)` over manual `Instance.provide(...)` inside Promise-style tests.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.tspackages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : When using the `tmpdir` function with git repository support, pass the `git: true` option to initialize a git repo with a root commit.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `config` option in `tmpdir` to write an `opencode.json` config file during test setup by passing a partial Config.Info object.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.test.ts
📚 Learning: 2026-04-20T14:36:21.274Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.274Z
Learning: Applies to packages/opencode/**/*.ts : Prefer `FileSystem.FileSystem` instead of raw `fs/promises` for effectful file I/O in Effect services
Applied to files:
packages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-22T09:32:52.535Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/ui/src/theme/context.tsx:11-16
Timestamp: 2026-04-22T09:32:52.535Z
Learning: In Astro-Han/pawwork (`packages/ui/src/theme/context.tsx` and related files), the renaming of localStorage theme keys from `opencode-*` to `pawwork-*` (THEME_ID, COLOR_SCHEME, THEME_CSS_LIGHT, THEME_CSS_DARK) is intentional and should NOT include a migration path from the old keys. Migrating would re-couple PawWork and OpenCode browser storage namespaces, which the PR is explicitly designed to avoid. A reset to the PawWork default theme on upgrade is acceptable by design.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/feedback.ts
📚 Learning: 2026-04-22T05:24:42.404Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 98
File: packages/desktop-electron/src/main/menu-labels.ts:1-2
Timestamp: 2026-04-22T05:24:42.404Z
Learning: In Astro-Han/pawwork, the app i18n layer (`packages/app/src/i18n/`) only contains `en.ts` and `zh.ts`, and `normalizeLocale` (in `packages/app/src/context/language.tsx`) only returns `"en"` or `"zh"`. The desktop `MenuLocale = "en" | "zh"` union in `packages/desktop-electron/src/main/menu-labels.ts` is intentionally limited to these two locales and is not a broader restriction — do not flag it as overly restrictive or suggest adding other locales.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-21T13:45:41.773Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 99
File: packages/desktop-electron/src/renderer/i18n/index.ts:30-35
Timestamp: 2026-04-21T13:45:41.773Z
Learning: In Astro-Han/pawwork, the locale normalization helpers in `packages/app/src/context/language.tsx` (`normalizeLocale`) and `packages/desktop-electron/src/renderer/i18n/index.ts` (`parseLocale`) are intentionally kept separate and have different fallback shapes: `normalizeLocale` always returns a concrete `Locale` (falling back to `"en"`), while `parseLocale` returns `Locale | null` so the desktop shell can decide whether to fall back to browser detection. Do not suggest extracting a shared normalization helper across these two runtimes.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/feedback.ts
📚 Learning: 2026-04-20T14:21:51.047Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 71
File: packages/app/src/components/session/session-status-connections.tsx:146-147
Timestamp: 2026-04-20T14:21:51.047Z
Learning: In the Astro-Han/pawwork repository (SolidJS app), `sync.data.config` is always initialized to `{}` at `packages/app/src/context/global-sync.tsx` line 71 and is never `undefined` at runtime. Non-optional property access like `sync.data.config.plugin` is intentional and consistent with the pattern used in `packages/app/src/components/status-popover-body.tsx` line 243. Do not flag `sync.data.config.plugin` as needing optional chaining.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/feedback.ts
📚 Learning: 2026-04-20T17:03:37.710Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 73
File: packages/opencode/src/cli/cmd/tui/context/sync.tsx:486-489
Timestamp: 2026-04-20T17:03:37.710Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/cli/cmd/tui/context/sync.tsx`), `sync.ready` returning `true` when `process.env.OPENCODE_FAST_BOOT` is set is intentional. The plugin-facing data properties `state.config` (initialized to `{}`) and `state.provider` (initialized to `[]`) expose safe-empty defaults, so they are safe to access before bootstrap completes. Do not flag these as needing null-guards or conditional patterns to match `vcs` — the difference is intentional because `vcs` starts as `undefined` while the others have initialized defaults. Changing this would alter the plugin API contract without a concrete failing case.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.ts
📚 Learning: 2026-04-21T16:00:43.416Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 102
File: packages/opencode/src/config/model-id.ts:9-11
Timestamp: 2026-04-21T16:00:43.416Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/config/model-id.ts`), `ConfigModelID` uses a regex that requires a non-empty provider segment and a non-empty model segment separated by `/`, but intentionally allows additional slashes within the model segment. This is because `Provider.parseModel` supports model IDs like `synthetic/hf:zai-org/GLM-4.7`. Do not flag the permissive slash handling as a bug.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.ts
📚 Learning: 2026-04-21T16:57:22.813Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 102
File: packages/opencode/src/config/agent.ts:108-119
Timestamp: 2026-04-21T16:57:22.813Z
Learning: In `packages/opencode/src/config/agent.ts` (Astro-Han/pawwork), `ConfigPermission.Info` only accepts permission objects or the three action strings `"ask"`, `"allow"`, `"deny"`, and transforms those action strings into `{ "*": action }` before `normalize()` runs. By the time `normalize()` is reached, `configuredPermission` is always either `undefined` or a `Record<string, Rule>` — never a raw arbitrary string. The `Object.assign(permission, configuredPermission)` pattern is therefore safe. Do not flag it as corrupting string permission references.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/feedback.ts
📚 Learning: 2026-04-21T16:57:20.257Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 102
File: packages/opencode/src/config/command.ts:23-29
Timestamp: 2026-04-21T16:57:20.257Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/config/`), `Schema.Struct` and `Schema.Class` are both valid and intentionally used patterns across config modules (e.g. `formatter.ts`, `provider.ts`, `permission.ts`, `lsp.ts`, `config.ts`, `keybinds.ts`, `command.ts`). Do not flag `Schema.Struct` usage in config schemas as needing conversion to `Schema.Class` — the choice is driven by upstream alignment and local context, not inconsistency.
Applied to files:
packages/desktop-electron/src/main/problem-report-files.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : In terminal tests, type through the browser using `runTerminal()` and `waitTerminalReady()` instead of writing to the PTY through the SDK
Applied to files:
packages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Import test utilities from `../fixtures` instead of `playwright/test`
Applied to files:
packages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-20T14:36:08.745Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/desktop-electron/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:08.745Z
Learning: Applies to packages/desktop-electron/src/main/ipc.ts : Main process should register IPC handlers in `src/main/ipc.ts`
Applied to files:
packages/desktop-electron/src/main/index.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Never use wall-clock waits like `page.waitForTimeout(...)` to make a test pass
Applied to files:
packages/desktop-electron/src/main/feedback.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Applied to files:
packages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `expect.poll(...)` for probing mock or backend state rather than transient DOM visibility
Applied to files:
packages/desktop-electron/src/main/feedback.test.ts
📚 Learning: 2026-04-20T14:36:31.017Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.017Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/desktop-electron/src/main/feedback.test.ts
🔇 Additional comments (28)
packages/desktop-electron/src/main/index.ts (3)
35-35: LGTM!The 3-second timeout constant for session export is reasonable and appropriately named with the
_MSsuffix for clarity.
137-163: Signal handling and cleanup are correct.The
sessionExportfunction properly:
- Checks for immediate abort before adding the listener
- Wires the external signal to the internal controller
- Cleans up the listener in
finallyto prevent memory leaks- Clears the timeout in all cases
190-237: LGTM!The
createFeedbackHandlerintegration is well-structured:
- All
FeedbackDepsproperties are correctly wiredshowFeedbackUrlFallbackprovides graceful degradation when the form can't opensaveReportandcleanupReportsproperly delegate to the new problem-report-files utilitiesonHandledErrorroutes secondary failures to the logger without triggering the generic error dialogpackages/desktop-electron/src/main/problem-report.test.ts (4)
1-57: LGTM!The tests properly verify:
- Caller-provided
reportIdandgeneratedAtare honored and embedded in the payload- The updated markdown instructions reference file upload
- The parsed payload correctly includes the
reportId
59-132: Comprehensive summary builder coverage.The tests thoroughly verify:
- Summary includes report metadata and excludes sensitive paths/logs
- Line count bounds (≤28 lines)
- Failed/unavailable report messaging with fallback instructions
- Recent errors limited to 10 single-line entries
- Oversized error truncation with
"..."and character limit enforcement
134-226: Good sanitization coverage.Tests verify path and sensitive data redaction across:
- Query parameters and hash fragments in routes
- Long session IDs with embedded paths
- Windows paths (including non-ASCII usernames and spaces)
- Linux home/temp paths and network shares
336-348: LGTM!The validation tests correctly verify rejection of:
- Empty and whitespace-only
reportId- Non-canonical timestamps (date-only, missing milliseconds)
packages/desktop-electron/src/main/feedback.test.ts (4)
77-98: LGTM!Localization tests correctly verify the updated confirmation dialog labels for both English and Chinese, including the new "copy summary and open form" messaging and full report file instructions.
107-140: Good coverage of the core flow and concurrency guard.The tests verify:
- Summary is copied (not the full JSON report)
- Full markdown is saved separately
- File is revealed in folder
- Form opens after artifacts are ready
- Busy guard prevents concurrent confirmations and releases properly on cancel
181-200: LGTM!The timeout test properly verifies:
- AbortSignal is fired on timeout
- Summary and saved report are still produced despite timeout
- The
"session export timed out"error is captured in the saved markdown
202-295: Comprehensive failure recovery coverage.Tests verify graceful degradation when:
- File write fails (summary-only, no attachment instructions, no reveal)
- Report construction fails (minimum summary still copied)
- Form open fails (fallback URL shown, logged via
onHandledError)- File reveal fails (directory open fallback attempted)
- Cleanup fails (form still opens, logged via
onHandledError)packages/desktop-electron/src/main/problem-report-files.test.ts (4)
1-24: LGTM!The test setup properly manages temporary directories with cleanup in
afterEachto prevent test pollution.
27-62: Good validation coverage.Tests verify:
- Deterministic filename generation with local time formatting
- Rejection of unsafe
reportIdvalues (path separators, dots, spaces, hyphens)- Rejection of non-canonical timestamps
64-100: LGTM!The write tests correctly verify:
- First write succeeds
- Second write to same target throws
"Problem report already exists"- Original file content is preserved on collision
- Temp cleanup failure after linking doesn't corrupt the saved report
117-141: No action needed. Symlink support on Windows is handled correctly.Modern Node.js (including Bun 1.3.11) supports unprivileged symlinks on Windows 10+ without requiring elevated privileges, and the test runs successfully on the Windows CI matrix. The
cleanupProblemReportsfunction correctly filters out non-regular files (including symlinks) usingstat.isFile(), so symlinks are safely skipped during cleanup rather than causing issues.> Likely an incorrect or invalid review comment.packages/desktop-electron/src/main/problem-report.ts (5)
1-7: LGTM!The constants define reasonable limits for summary fields to prevent overly long content from breaking the Feishu form.
69-76: LGTM!The
isCanonicalIsoTimestampround-trip check is stricter than regex-based validation and correctly rejects date-only strings and non-UTC offsets. ThedefaultReportIdgenerates collision-resistant IDs using timestamp and random components.
165-171: LGTM!Validation correctly:
- Rejects empty/whitespace-only
reportId- Requires canonical ISO timestamp format for
generatedAt
262-298: LGTM!The sanitization helpers properly:
- Extract only the first line and normalize whitespace
- Redact Windows paths, UNC paths, and Unix home/tmp paths
- Truncate with
"..."suffix when exceeding limits- Strip query parameters and hash fragments from routes
300-330: LGTM!The summary builder correctly assembles a compact report with:
- Conditional messaging based on full report status
- Redacted diagnostics (route, session ID)
- Bounded recent errors list
packages/desktop-electron/src/main/feedback.ts (5)
45-87: LGTM!The dialog labels are properly localized for both English and Chinese, with clear messaging about the new copy-summary + save-full-report flow. The
formOpenFailedTitleandformOpenFailedMessageprovide helpful fallback instructions.
89-123: LGTM!The helper functions are well-designed:
safeFailureReasonmaps errors to safe, non-sensitive classificationsfallbackDiagnosticsprovides minimal viable info when diagnostics collection failsrecentKeyErrorsextracts relevant log lines with proper filtering and bounds
125-141: LGTM!The timeout wrapper correctly:
- Creates an AbortController to propagate cancellation
- Races the export against the timeout
- Cleans up the timeout in
finallyto prevent leaks- Aborts the controller when timing out
143-201: LGTM!The report preparation flow correctly:
- Snapshots context before confirmation (preventing focus-change issues)
- Handles diagnostics/logTail failures gracefully with fallbacks
- Preserves the original error message in failed session exports
- Builds summary with appropriate status based on save success/failure
202-245: LGTM!The post-preparation flow correctly:
- Copies summary to clipboard
- Attempts to reveal the saved file with directory-open fallback
- Handles Electron's
openPatherror strings (non-empty string = error)- Routes secondary failures through
onHandledErrorinstead ofonError- Cleanup failures don't block form opening
- Form-open failures show a fallback dialog with the URL
- Concurrency guard prevents multiple confirmations and releases on completion
packages/desktop-electron/src/main/problem-report-files.ts (3)
12-28: LGTM!The filename builder correctly:
- Validates
reportIdagainst the safe character pattern to prevent path traversal- Requires canonical ISO timestamps
- Uses local time for human-readable filenames
- Produces collision-resistant names with millisecond precision
39-69: LGTM!The atomic write implementation is robust:
- Creates the directory if needed
- Uses exclusive creation (
wx) for the temp file to prevent races- Hard links to the final path for atomic visibility
- Converts
EEXISTto a clear error message- Best-effort temp cleanup doesn't fail the operation after successful link
- Outer catch cleans up temp on any failure before the link
71-95: LGTM!The cleanup logic correctly:
- Filters to only matching report filenames (prevents deleting unrelated files)
- Preserves the current report via
currentPathcheck- Uses
lstatto skip symlinks and directories- Sorts by mtime descending to keep newest
- Silently ignores read/stat/delete failures for resilience
6f09aff to
0044776
Compare
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
11 tasks
11 tasks
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
userData/problem-reportsfor manual upload in the feedback form.Why
The Feishu feedback form rejects or becomes impractical with the full problem report pasted into a text field. This follow-up keeps the form payload short and moves detailed diagnostics into an uploadable markdown file.
Related Issue
Refs #84. This only covers the feedback report upload flow and does not close all remaining #84 scope.
How To Verify
Screenshots or Recordings
N/A. Native confirmation dialog copy changed; localized copy and flow behavior are covered by unit tests.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests