Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }, | ||
| "packages/shared": { | ||
| "entry": [ | ||
| "src/{model,git,logging,shell,Net,DrainableWorker,KeyedCoalescingWorker,schemaJson,Struct,String}.ts!", |
There was a problem hiding this comment.
Missing serverSettings from shared package entry list
Low Severity
The packages/shared entry pattern explicitly enumerates every production export from package.json (model, git, logging, shell, Net, DrainableWorker, KeyedCoalescingWorker, schemaJson, Struct, String) but omits serverSettings, which is both exported in packages/shared/package.json and actively imported by apps/desktop and apps/server. While Knip auto-discovers entries from package.json exports, the explicit brace-expansion list creates a false impression of completeness that could mislead future maintainers.
- narrow module surface areas by making internal types and constants private - drop stale exports, helpers, and UI code no longer used - reduce unused dependencies and simplify server/web modules
- Move shared workspace deps into runtime packages - Hide internal schema helpers and rename turn diff contract - Remove obsolete web history bootstrap module
- Extract shared bootstrap, open, keybinding, and receipt logic - Move integration-only projection checkpoint repository support - Update tests and web helpers to use the new shared modules
- Split common server, git, persistence, and UI helpers into shared/testing modules - Reduce duplication across tests and runtime code - Keep integration and provider flows wired to the new shared layers
- Re-export SQLite and keybinding helpers from shared modules - Adjust git branch listing and localStorage fallback behavior - Stabilize MessagesTimeline tests with a mocked markdown component
- Export Claude auth/status helpers from `ClaudeProvider` - Remove the separate testing-only Claude provider module - Update provider registry tests to import the shared implementation
- move CLI config resolution into `cli.logic.ts` - tighten git upstream refresh handling and remote parsing - update tests and web/server plumbing for the refactor
- Move shared Codex provider helpers into `CodexProvider.shared` - Add tests for custom model_provider parsing and auth status JSON handling
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
- remove obsolete trace sink and tracer layers - add observability config plumbing and branch pagination updates - align tests with the new server and Git status shape
github-actions
bot
added
size:XXL
macroscopeapp
bot
dismissed
their stale review
Dismissing prior approval to re-evaluate eaf00af
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| const resolveBooleanFlag = (flag: Option.Option<boolean>, envValue: boolean) => | ||
| Option.getOrElse(flag, () => envValue); |
There was a problem hiding this comment.
Inconsistent resolveBooleanFlag semantics across codebase
Medium Severity
The resolveBooleanFlag in cli.logic.ts dropped the Option.filter(flag, Boolean) call that existed in the original cli.ts, changing its semantics: Some(false) now returns false instead of falling through to envValue. While the new behavior is arguably more correct (and a new test validates it), the identical function in scripts/build-desktop-artifact.ts still uses the old Option.filter(flag, Boolean) pattern. These two copies now silently disagree on how explicit false flags are handled, which is likely to cause confusion or incorrect behavior when one is updated but not the other.
| export function parseKeybindingShortcut(value: string): KeybindingShortcut | null { | ||
| const rawTokens = value | ||
| .toLowerCase() | ||
| .split("+") | ||
| .map((token) => token.trim()); | ||
| const tokens = [...rawTokens]; | ||
| let trailingEmptyCount = 0; | ||
| while (tokens[tokens.length - 1] === "") { | ||
| trailingEmptyCount += 1; | ||
| tokens.pop(); | ||
| } | ||
| if (trailingEmptyCount > 0) { | ||
| tokens.push("+"); | ||
| } | ||
| if (tokens.some((token) => token.length === 0)) { | ||
| return null; | ||
| } | ||
| if (tokens.length === 0) return null; |
There was a problem hiding this comment.
🟢 Low src/keybindings.logic.ts:46
Empty string or whitespace-only input to parseKeybindingShortcut returns a shortcut for the + key instead of null. For input "": split("+") yields [""], the while loop pops it and pushes "+", so the function returns { key: "+", ... } rather than rejecting the invalid input.
export function parseKeybindingShortcut(value: string): KeybindingShortcut | null {
+ const trimmed = value.trim();
+ if (trimmed.length === 0) {
+ return null;
+ }
const rawTokens = value
.toLowerCase()
.split("+")
.map((token) => token.trim());🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/keybindings.logic.ts around lines 46-63:
Empty string or whitespace-only input to `parseKeybindingShortcut` returns a shortcut for the `+` key instead of `null`. For input `""`: `split("+")` yields `[""]`, the while loop pops it and pushes `"+"`, so the function returns `{ key: "+", ... }` rather than rejecting the invalid input.
Evidence trail:
apps/server/src/keybindings.logic.ts lines 46-99 at REVIEWED_COMMIT. The function `parseKeybindingShortcut` at line 46 processes empty string by: split("+") yields [""], while loop pops it (line 53-56), then pushes "+" (line 58), validation checks pass (lines 59-61), and the "+" token becomes the key in the default case (line 94-96), returning { key: "+", ... } instead of null.
| auth: extractAuthBoolean(JSON.parse(trimmed)), | ||
| }; | ||
| } catch { | ||
| return { attemptedJsonParse: false as const, auth: undefined as boolean | undefined }; |
There was a problem hiding this comment.
🟢 Low Layers/ClaudeProvider.logic.ts:140
When JSON.parse throws on malformed JSON, attemptedJsonParse is incorrectly set to false in the catch block, causing the function to fall through to the result.code === 0 check and return status: "ready" / auth: { status: "authenticated" } even though the auth status could not be verified. The catch block should set attemptedJsonParse: true since parsing was attempted but failed.
- return { attemptedJsonParse: false as const, auth: undefined as boolean | undefined };🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/Layers/ClaudeProvider.logic.ts around line 140:
When `JSON.parse` throws on malformed JSON, `attemptedJsonParse` is incorrectly set to `false` in the catch block, causing the function to fall through to the `result.code === 0` check and return `status: "ready"` / `auth: { status: "authenticated" }` even though the auth status could not be verified. The catch block should set `attemptedJsonParse: true` since parsing was attempted but failed.
Evidence trail:
apps/server/src/provider/Layers/ClaudeProvider.logic.ts lines 130-167 at REVIEWED_COMMIT. The catch block at line 140 sets `attemptedJsonParse: false`. Following the control flow: if `auth` is undefined and `attemptedJsonParse` is false, lines 144, 147, and 153 all skip, and line 160's `if (result.code === 0)` returns `status: 'ready', auth: { status: 'authenticated' }` even though JSON parsing failed and auth status is unknown.
1 participant
What Changed
This PR adds Knip to the repo in a minimal setup-only pass.
It introduces:
knipas a root dev dependencybun run knipscriptknip.jsoncovering the apps, packages, and scripts workspacesThis PR does not include any dead-code cleanup. It just wires Knip in so the current baseline is visible from CI/local runs.
Why
The earlier branch mixed Knip setup with a much larger cleanup/rebase diff. This resets the work to a small, reviewable PR that only adds the tool and its config.
Knip currently reports existing unused files, dependencies, exports, and types in the repo. That cleanup can happen in follow-up PRs once the config is accepted.
Changes
What This PR Actually Changes
knip,knip --production, andknip --strictare meaningful.*.shared.ts,*.logic.ts, and*.testing.tsmodules instead of exporting internals from production files.src/and into integration support.mainbehavior.Notable dead code removed
apps/web/src/components/ui/card.tsxapps/web/src/components/ui/field.tsxapps/web/src/components/ui/fieldset.tsxapps/web/src/components/ui/form.tsxapps/web/src/components/ui/input-group.tsxapps/web/src/components/ui/kbd.tsxapps/web/src/components/ui/radio-group.tsxapps/web/src/historyBootstrap.tsapps/web/src/rpc/client.tsapps/server/src/orchestration/Errors.tsapps/server/src/orchestration/Schemas.tsapps/server/src/persistence/Errors.tsapps/server/src/observability/Server cleanup
keybindings.ts->keybindings.logic.tsopen.ts->open.logic.ts/open.shared.tscli.ts->cli.logic.tsbootstrap.ts->bootstrap.shared.tsserverRuntimeStartup.ts->serverRuntimeStartup.logic.tscodexAppServerManager.ts->codexAppServerManager.shared.tsgit/Layers/GitCore.ts->GitCore.shared.tsterminal/Layers/Manager.ts->Manager.shared.tsClaudeProvider.logic.ts/CodexProvider.shared.tsapps/server/integration/support/ProjectionCheckpoints.tsWeb cleanup
store.ts->store.logic.tsuiStateStore.ts->uiStateStore.logic.tsrpc/atomRegistry.testing.tsnativeApi.testing.tsBehavior
Validation
bun fmtbun lintbun typecheckbun run testbun run knipbun run knip -- --productionbun run knip -- --strictAll pass.
Checklist
Note
Medium Risk
Moderate risk due to refactoring core server plumbing (CLI config resolution and
GitCoreimplementation) and changing boolean flag precedence, which could affect runtime defaults and git operations if behavior diverges from the previous implementation.Overview
Refactors server code to reduce runtime module surface by extracting reusable/test-only logic into new
*.shared.ts,*.logic.ts, and*.testing.tsmodules (e.g.,bootstrap.shared.ts,cli.logic.ts,codexAppServerManager.shared.ts, andgit/Layers/GitCore.shared.ts).Updates CLI config resolution to centralize precedence handling and ensure explicit
falseflags override env/bootstrap values, with new test coverage. Integration tests are adjusted to useSqlite.testing.ts, move projection checkpoint repository support intointegration/support, and shorten default wait timeouts.Minor cleanup includes reclassifying workspace deps as runtime dependencies in app
package.jsonfiles and narrowing a couple of exported TS interfaces/constants to internal-only types.Written by Cursor Bugbot for commit eaf00af. This will update automatically on new commits. Configure here.
Note
Add Knip to remove unused exports and dead code across the monorepo
apps/web,apps/server, andpackages/contracts, including types, interfaces, constants, and helper functions..shared.tsmodules (e.g.terminalContext.shared.ts,Sqlite.shared.ts,cli.logic.ts,keybindings.logic.ts) so that previously-exported internals can remain accessible to tests without polluting public APIs..testing.tsmodules for test-only helpers that were previously exported from production modules.components/uilibrary surface significantly — many sub-components (e.g.AlertDialogTrigger,AutocompleteInput,DropdownMenu*aliases,SheetPanel) are removed from their respective modules.@t3tools/contractsand@t3tools/sharedfromdevDependenciestodependenciesinapps/desktopandapps/server.📊 Macroscope summarized eaf00af. 83 files reviewed, 7 issues evaluated, 3 issues filtered, 2 comments posted
🗂️ Filtered Issues
apps/server/src/git/Layers/GitCore.shared.ts — 0 comments posted, 2 evaluated, 1 filtered
2(rename/copy) entries in porcelain v2 output, the code extracts the original path (after the tab) instead of the new path (before the tab). Git porcelain v2 format is2 ... <path>\t<origPath>where<path>is the new destination. The code doesline.slice(tabIndex + 1)which gets<origPath>. This causes renamed files to be recorded with their old path, creating inconsistency withparseNumstatEntrieswhich correctly extracts the new path, potentially causing duplicate entries or mismatched file tracking. [ Out of scope (triage) ]apps/server/src/provider/Layers/ClaudeProvider.logic.ts — 1 comment posted, 2 evaluated, 1 filtered
findSubscriptionType, nested arrays under container keys are silently skipped. At line 224-227,asRecord(record[key])returnsNonewhenrecord[key]is an array (due to the!globalThis.Array.isArray(v)check inasRecord), so the array is never searched. This contradicts the function's stated intent to "Walk an unknown parsed JSON value" since arrays are only handled at the top level (line 212-214), not when nested under keys like"account"or"subscription". If the CLI outputs JSON like{"account": [{"subscriptionType": "max"}]}, detection will fail. The fix would be to callfindSubscriptionType(record[key])directly instead of wrapping withasRecord().pipe(Option.flatMap(findSubscriptionType)). [ Out of scope (triage) ]apps/web/src/store.logic.ts — 0 comments posted, 1 evaluated, 1 filtered
mapSession, line 101 assignssession.updatedAtto thecreatedAtfield. This causescreatedAtandupdatedAtto always have the same value, losing the actual creation timestamp. IfOrchestrationSessionhas acreatedAtproperty, it should be used here instead. [ Failed validation ]