Repository navigation
refactor(server): move the plan route and types into their own slice - #1694
Merged
Merged
Conversation
A unit-lane test through a real createServer, with sessionSnapshot stubbed, so it runs without herdr: the route wants GET and a pane_id, tells a pane herdr does not have, or one that keeps no plan, that it has none on this PC's clock, and does the same when herdr does not answer within its second.
The next feature slice out of server/index.ts and shared/protocol.ts (plan: .omo/plans/feature-slices-plan.md). GET /api/pane/plan moves, unchanged, into handlePlanRequest in server/plan-routes.ts, called at the same place in the dispatch order and given the core's own OmO, Claude subagent and plan trackers (it makes none of its own). PlanStep and PanePlan move verbatim into shared/plans.ts (types only), and protocol.ts re-exports them. PlanSummary stays in protocol.ts: the session snapshot and the pane-status message carry it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 5 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: moving the server plan route and related types into dedicated modules. Description check The description includes the required Change and Validation sections. It explains the refactor, preserved behavior, test coverage, validation results, risks, and changelog decision. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
devswha
added a commit
that referenced
this pull request
Oct 23, 2026
…#1696) The last inline feature slice out of `server/index.ts` and `shared/protocol.ts`, after integrations (#1686), plugins (#1687), background tasks (#1688), the agent catalog (#1690) and plans (#1694): the file routes and their types move into their own files. Behaviour, paths, status codes, headers and messages stay the same. The plan is `.omo/plans/feature-slices-plan.md` in the maintainer's workspace. This PR moves responsibility, so it is **not move-only**. ## Two commits: the check first, then the move 1. **`test(server): pin the file routes before they move`** adds `server/file-routes.test.ts`. - **Why it is needed:** the existing route coverage is mostly live: `page-media.contract.test.ts`, `route-envelope.contract.test.ts` and `device-access.contract.test.ts`. The helpers have unit tests (`file-view.test.ts`, `project-favicon.test.ts`, `files.test.ts`). In the unit lane, only `open-access.test.ts` reaches a file route, and only for the watching-device refusal. - **How it runs without herdr, in the unit lane:** a real `createServer({ port: 0, hostname: "127.0.0.1", stateDir: mkdtempSync(…), … })` reading a temp project folder, with `paneGet` stubbed through `spyOn(herdr, "paneGet")` where a pane is involved. - **What it pins:** - **Methods and other sites:** - a POST answers `400 method_not_allowed`, and HEAD is allowed; - another site's `no-cors` read answers `403 cross_site`, while a top-level navigation is served. - **A file by its absolute path:** - `/api/fs/stat` answers its name, kind and size; - `/api/fs/file` answers its bytes, `inline` by default and `attachment` with `download=1`; - a missing file answers `404 not_found`. - **A relative path in a pane's folder:** with `pane_id`, a relative path is found in the folder herdr reports for that pane. When herdr cannot say that folder, the relative path is `404 not_found`, but an absolute path is still served. - **The project icon:** `/api/fs/favicon` without `root` answers `400 missing_root`. A folder with `public/favicon.ico` answers that icon, and a folder without one answers `{ icon: null }`. - **Page media:** an address never given out answers `404 not_found`, and a POST answers `400 method_not_allowed`. On unchanged code it gives **5 pass, 0 fail**. 2. **`refactor(server): move the file routes and types into their own slice`** - **Moved:** both branches, unchanged, into `handleFileRequest(request, url, pathname, requester): Promise<Response | null>` in the new `server/file-routes.ts`: - `/api/fs/stat`, `/api/fs/file` and `/api/fs/favicon`; - `/api/fs/page-media/:media_id`. - **Moved with them:** `claudeTempFolder`, which only the file read used. - **Who asks:** the access gate in `index.ts` already worked out who asks: `bridgeAuthorized`, `requestHost`, `forwarded`, `client` and `access`. The route gets those values as they are (`FileRequester`), because a page's media plays only for the client and device that rendered it. Nothing is computed again. - **Shared state stays where it is:** the page-media registry stays in `file-view.ts`. Sign-out and device revocation still call `forgetPageMedia` from `index.ts`. - **Moved:** `FileKind`, `FileInfo` and `ProjectFavicon`, verbatim with their doc comments, into the new `shared/files.ts` (types only). - **Stays in `index.ts`:** one call at the same place in the dispatch order, after the agent catalog and before the workspace routes. Every gate before it is untouched, including the watching-device refusal of file reads. The imports only these routes used leave `index.ts`: `paneContext`, `resolveAgent`, `claudePaneSession`, `claudeSessionTempDir`, `findProjectFavicon`, `paneGet`, `isSecureRequest`, the file-view helpers except `forgetPageMedia`, and `ProjectFavicon`. - **Stays in `protocol.ts`:** a type re-export of the three names, so every importer compiles unchanged. ## Checks - **After the move:** `HERDR_TEST_MODE=unit bun test server/file-routes.test.ts server/file-view.test.ts server/project-favicon.test.ts server/files.test.ts server/open-access.test.ts server/startup-order.test.ts` gives ****71 pass, 0 fail****. The pinning test is unchanged between the two commits. - **`--color-moved` on the move commit:** `git diff --color-moved=plain --color-moved-ws=allow-indentation-change HEAD~1 HEAD -- server shared` shows 168 lines as moved: both branches (one indent level shallower in their new file), `claudeTempFolder` and the three types. The 26 other lines are: - in `index.ts`: - the shortened protocol, `auth.ts` and `file-view.ts` imports (each before and after); - the new `file-routes.ts` import; - the dropped `paneGet` import line; - the 2-line call. - in the new route file: - its 4-line header; - the 6 imports that did not move with the code; - the `FileRequester` type with its comment; - the function's doc comment and signature; - `return null`. - in `protocol.ts`: the re-export line. The imports only the moved code used (`paneContext`/`resolveAgent`, `claudePaneSession`, `claudeSessionTempDir`, `findProjectFavicon`) show as moved from `index.ts` into the route file. - **Client imports:** `bun run typecheck` passes on both tsconfigs. No file in `src/` or `site/` imports `shared/files.ts`. `shared/files.ts` has no import and no value export. - **Sizes:** `server/index.ts` goes from 704 to 657 lines and `shared/protocol.ts` from 1,381 to 1,353. The new modules are 69 and 29 lines. - **Risks checked:** - Import cycles: none new. The route imports `access.ts` (type only), `auth.ts`, `claude-store.ts`, `conversation.ts`, `file-view.ts`, `herdr/client.ts`, `http.ts`, `pane-context.ts` and `project-favicon.ts`. These were all already imported by `index.ts`, and none of them imports `index.ts`. - Initialization order: unchanged. No module state moves. - `bun run check fast`: passes (unit 6161 pass, 0 fail, 12 skip in 440 files, 8 at a time, 57.4s). - CI's integration lane runs `page-media.contract.test.ts`, `route-envelope.contract.test.ts` and `device-access.contract.test.ts` against an isolated live herdr. ## Overlapping open PRs Open PRs that touch these files: #1652, #1598, #1442 and #1427. I checked each diff for the moved lines, the page-media names and the three file types: - #1652, #1442 and #1427 change none of them. - #1598 changes the `bridgePath` list in `index.ts`, which names `/api/fs/`. That list stays in `index.ts`, and this PR does not touch it. No changelog fragment: users see no change.
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
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.
The next feature slice out of
server/index.tsandshared/protocol.ts, after integrations (#1686), plugins (#1687), background tasks (#1688) and the agent catalog (#1690): the plan route and its types move into their own files. Behaviour, path, status codes and messages stay the same. The plan is.omo/plans/feature-slices-plan.mdin the maintainer's workspace.This PR moves responsibility, so it is not move-only.
Two commits: the check first, then the move
test(server): pin GET /api/pane/plan before it movesaddsserver/plan-routes.test.ts.api.contract.test.ts(missing pane, wrong method) andclaude-session.contract.test.ts(a Claude Code pane's plan). The plan readers have unit tests (session-plan.test.ts,session-plan-readers.test.ts).createServer({ port: 0, hostname: "127.0.0.1", stateDir: mkdtempSync(…), … }), withsessionSnapshotstubbed throughspyOn(herdr, "sessionSnapshot").400 method_not_allowed, and a request withoutpane_idanswers400 missing_pane_id;{ plan: null }with a validserver_time;{ plan: null }once its 1-second bound (within(1000, …)) runs out. That makes this test take about a second.On unchanged code it gives 3 pass, 0 fail.
refactor(server): move the plan route and types into their own slicehandlePlanRequest(request, url, pathname, { omo, claudeAgents, plans }): Promise<Response | null>in the newserver/plan-routes.ts. This is the nullable route pattern of the earlier slices.index.tspasses the core's ownomo(OmoStatus),claudeAgents(ClaudeSubagentStatus) andplans(SessionPlans). The route's type allows only the methods it calls:apply;ensure; andknown,ensure,planOf. It constructs nothing, so there is one tracker each as before and startup order does not change.PlanStepandPanePlan, verbatim with their doc comments, into the newshared/plans.ts(types only).protocol.ts:PlanSummary, because the session snapshot's pane enrichment and thepane-statusWebSocket message both carry it; a re-export would add nothing there. Also a type re-export ofPlanStepandPanePlan, soserver/session-plan.ts,server/session-plan-reader.ts,server/claude-session.contract.test.ts,src/lib/api.ts,src/lib/plan.tsand its test,BackgroundTasks.tsxandsite/demo/transport.tscompile unchanged.index.ts: one call at the same place in the dispatch order, after the background tasks route and before session history. The now-unusedwithin,sessionSnapshot,HerdrPaneandPanePlanimports leave.Checks
HERDR_TEST_MODE=unit bun test server/plan-routes.test.ts server/tasks-routes.test.ts server/session-plan.test.ts server/session-plan-readers.test.ts src/lib/plan.test.ts server/startup-order.test.tsgives 41 pass, 0 fail. The pinning test is unchanged between the two commits.--color-movedon the move commit:git diff --color-moved=plain --color-moved-ws=allow-indentation-change HEAD~1 HEAD -- server sharedshows 102 lines as moved: the branch (one indent level shallower in its new file) and both types. The 23 other lines are:index.ts:withinandsessionSnapshotimports;plan-routes.tsimport;PlanTrackerstype with its comment;return null.protocol.ts: the re-export line.bun run typecheckpasses on both tsconfigs. No file insrc/orsite/importsshared/plans.ts.shared/plans.tshas no import and no value export.server/index.tsgoes from 724 to 704 lines andshared/protocol.tsfrom 1,404 to 1,381. The new modules are 41 and 24 lines.claude-subagents.ts,herdr/client.tsandhttp.ts, plus types only fromomo-status.tsandsession-plan.ts. None of them reachesindex.ts, and nothing comes fromcore/.bun run check fast: passes (unit 6156 pass, 0 fail, 12 skip in 439 files, 8 at a time, 57.9s).api.contract.test.tsandclaude-session.contract.test.tsplan cases against an isolated live herdr.Overlapping open PRs
Open PRs that touch these files: #1652, #1598, #1442 and #1427. I checked each diff for the moved lines and for
PlanStepandPanePlan:PanePlanandPlanStep. It imports them fromprotocol.ts, so the line still resolves through the re-export.No changelog fragment: users see no change.