Allow repo-less sessions from web composer#862
Conversation
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
📝 WalkthroughWalkthroughAdds a ChangesNo-repository session support
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR #862, "Allow repo-less sessions from web composer" by @ColeMurray updates the homepage composer to support an explicit No repository target and adds focused tests for repo-less session creation. Reviewed 2 changed files (+232/-38); the implementation is small, consistent with existing repo-less session handling, and I did not find blocking correctness, security, performance, or maintainability issues.
Critical Issues
None found.
Suggestions
None blocking.
Nitpicks
None.
Positive Feedback
- The session creation payload now clearly sends
repoOwner: nullandrepoName: nulland omits branch context for repo-less sessions. - The branch selector is hidden when the selected target is not an actual repository, which keeps the UI state aligned with the request payload.
- The added tests cover both explicit No repository selection and the empty-repository default path.
Questions
None.
Verification
I attempted to run npm test -w @open-inspect/web -- src/app/(app)/page.test.tsx from a temporary PR-head worktree, but dependencies were not installed there, so vitest was unavailable. I did not rerun the full test suite.
Verdict
Approve.
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/app/(app)/page.test.tsx (1)
112-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the standard (non-null) repo + branch session-creation flow.
This is the only test file for the Home page, yet both tests exercise the no-repository path. Neither confirms that selecting an actual repo (the pre-existing/default flow) still sends
repoOwner/repoName/branchcorrectly in the POST body. A regression here (e.g. from thesessionTargetrefactor mentioned in the PR objectives) would go undetected.Consider adding a third test that selects
repo(already fixtured at lines 28-36) and asserts the create-session payload includesrepoOwner: repo.owner,repoName: repo.name, and abranchfield.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/`(app)/page.test.tsx around lines 112 - 151, The Home page tests only cover the no-repository flow, so add coverage for the standard repository-backed session creation path in the Home test suite. In the existing `describe("Home")` block, add a test that selects the pre-fixtured `repo` from the primary selector and submits a session, then assert the POST body created by `sessionCreateBody()` includes `repoOwner`, `repoName`, and `branch` populated from that repo. This should verify the `Home`/`sessionTarget` flow still sends the expected repo fields when a real repository is chosen.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/app/`(app)/page.tsx:
- Line 76: The warmed-session cache and pending-config matching currently ignore
reasoningEffort, so a session can be reused across different effort settings.
Update the warm-session key and the pendingConfigRef shape/match logic in
page.tsx to include reasoningEffort alongside target, model, and branch, and
make sure any session reset/abort path triggered by config changes also treats
reasoningEffort as a distinct configuration value.
---
Nitpick comments:
In `@packages/web/src/app/`(app)/page.test.tsx:
- Around line 112-151: The Home page tests only cover the no-repository flow, so
add coverage for the standard repository-backed session creation path in the
Home test suite. In the existing `describe("Home")` block, add a test that
selects the pre-fixtured `repo` from the primary selector and submits a session,
then assert the POST body created by `sessionCreateBody()` includes `repoOwner`,
`repoName`, and `branch` populated from that repo. This should verify the
`Home`/`sessionTarget` flow still sends the expected repo fields when a real
repository is chosen.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07b768e8-51a4-4c36-b68f-556892bd9946
📒 Files selected for processing (2)
packages/web/src/app/(app)/page.test.tsxpackages/web/src/app/(app)/page.tsx
1 participant
Summary
No repositoryoption to the homepage session target selector.repoOwner: nullandrepoName: null.No repositorywhen the user has no accessible repositories.Testing
npm test -w @open-inspect/web -- src/app/(app)/page.test.tsxnpm run typecheck -w @open-inspect/webnpm run lint -w @open-inspect/webnpm test -w @open-inspect/webSummary by CodeRabbit