Skip to content

feat: [ENG-2167] expose context-tree remote URL via project config#498

Merged
ncnthien merged 2 commits intoproj/local-web-uifrom
feat/ENG-2167
Apr 21, 2026
Merged

feat: [ENG-2167] expose context-tree remote URL via project config#498
ncnthien merged 2 commits intoproj/local-web-uifrom
feat/ENG-2167

Conversation

@ncnthien
Copy link
Copy Markdown
Collaborator

The daemon's state:getProjectConfig handler now also reads the context-tree git remote (defaulting to origin) off disk and returns it alongside the existing project config, running in parallel with the config load so the handler's latency is unchanged. The web UI's project dropdown drops buildRemoteSpaceUrl + the webAppUrl prop threading in favour of reading remoteUrl directly from the config response, so "Open Remote space" now opens whatever the git remote actually points at.

Summary

  • Problem:
  • Why it matters:
  • What changed:
  • What did NOT change (scope boundary):

Type of change

  • Bug fix
  • New feature
  • Refactor (no behavior change)
  • Documentation
  • Test
  • Chore (build, dependencies, CI)

Scope (select all touched areas)

  • TUI / REPL
  • Agent / Tools
  • LLM Providers
  • Server / Daemon
  • Shared (constants, types, transport events)
  • CLI Commands (oclif)
  • Hub / Connectors
  • Cloud Sync
  • CI/CD / Infra

Linked issues

  • Closes #
  • Related #

Root cause (bug fixes only, otherwise write N/A)

  • Root cause:
  • Why this was not caught earlier:

Test plan

  • Coverage added:
    • Unit test
    • Integration test
    • Manual verification only
  • Test file(s):
  • Key scenario(s) covered:

User-visible changes

List user-visible changes (including defaults, config, or CLI output).
If none, write None.

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording

Checklist

  • Tests added or updated and passing (npm test)
  • Lint passes (npm run lint)
  • Type check passes (npm run typecheck)
  • Build succeeds (npm run build)
  • Commits follow Conventional Commits format
  • Documentation updated (if applicable)
  • No breaking changes (or clearly documented above)
  • Branch is up to date with main

Risks and mitigations

List real risks for this PR. If none, write None.

  • Risk:
    • Mitigation:

The daemon's state:getProjectConfig handler now also reads the
context-tree git remote (defaulting to origin) off disk and returns
it alongside the existing project config, running in parallel with
the config load so the handler's latency is unchanged. The web UI's
project dropdown drops buildRemoteSpaceUrl + the webAppUrl prop
threading in favour of reading remoteUrl directly from the config
response, so "Open Remote space" now opens whatever the git remote
actually points at.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 21, 2026

Claude finished @ncnthien's task in 2m 2s —— View job


PR Review

  • Gather context and read changed files
  • Analyze changes for correctness, security, and code quality
  • Post inline comments
  • Post summary with overall score

Summary

Overall score: 3/5

The approach is sound — reading the git remote in parallel with getProjectConfig via Promise.all keeps latency flat, and the graceful undefined fallback on error is correct. The web UI simplification (dropping buildRemoteSpaceUrl + webAppUrl prop threading) is a genuine improvement in cohesion.

However, there are three issues that need addressing before this is ready to merge:

Issues

1. No tests — blocks merge (CLAUDE.md: mandatory TDD)
readContextTreeRemoteUrl has zero test coverage. Per the project's strict TDD policy, tests must be written before or alongside implementation. Required scenarios: happy path, missing context-tree dir (catch branch), and git.getConfig returning undefined when the remote key is absent. See inline comment on brv-server.ts:95.

2. URL scheme not validated before window.open — security
remoteUrl is read verbatim from a git config file and passed to window.open. SSH remotes (git@github.com:..., ssh://...) produce a silent no-op; a crafted .git/config with a javascript: or file: URI could be harmful. Only http:// / https:// URLs should be opened. See inline comment on project-dropdown.tsx:117.

3. Dead code left behind
src/webui/features/project/utils/build-remote-space-url.ts and its test file test/unit/webui/features/project/utils/build-remote-space-url.test.ts are now unreferenced — nothing imports buildRemoteSpaceUrl anymore. Both files should be deleted.

Suggestions

  • UX label: "Open Remote space" implies a ByteRover space URL, but the remote could now be any git host. A neutral label like "Open remote" would be more accurate. See inline comment on project-dropdown.tsx:115.
  • The remote = 'origin' default parameter in readContextTreeRemoteUrl is an unexposed extension point — fine for now, but if it's never intended to be configurable, the parameter can be removed to reduce surface area.

Comment thread src/server/infra/daemon/brv-server.ts Outdated
Comment thread src/webui/features/project/components/project-dropdown.tsx Outdated
Comment thread src/webui/features/project/components/project-dropdown.tsx Outdated
Pull readContextTreeRemoteUrl out of brv-server.ts into its own
module under server/infra/context-tree so the daemon entry file
stays a thin wiring layer and the helper gets real unit coverage
(happy path, missing git repo, no remote configured, non-default
remote name, missing project path). The project dropdown now
refuses to open a remote URL that isn't http(s) — SSH remotes
aren't browser-openable anyway, and guarding the scheme closes
the window where a tampered .git/config could smuggle a
javascript: or file: URI into window.open.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 21, 2026

Claude finished @ncnthien's task in 1m 18s —— View job


PR Review (re-review after sync)

  • Gather context and read changed files
  • Analyze changes against previous review issues
  • Post updated summary

Summary

Overall score: 4/5 (up from 3/5)

Good progress. The two blocking issues from the previous review are resolved:

✅ Issue #1 — Tests added (test/unit/infra/context-tree/read-context-tree-remote.test.ts)
Five solid real-filesystem test cases covering: happy path, missing context-tree dir, git repo with no remote, non-default remote name, and completely absent project path. Matches the project's ES-module / real-fs testing convention perfectly.

✅ Issue #2 — URL scheme validated before window.open
The isSafeHttpUrl guard (new URL() + .protocol check) is more robust than a regex and is applied to both the disabled prop and the onClick handler. The explanatory comment at line 103–104 is a nice touch. No concerns.


Remaining issue

Dead code — buildRemoteSpaceUrl and its test file are still present

Nothing imports buildRemoteSpaceUrl anymore:

  • src/webui/features/project/utils/build-remote-space-url.ts
  • test/unit/webui/features/project/utils/build-remote-space-url.test.ts

Both should be deleted. Leaving unreferenced utility files around inflates the codebase and can confuse future contributors who don't realise the function was superseded.

Fix this →


Minor notes (non-blocking)

  • The remote = 'origin' default parameter in readContextTreeRemoteUrl is fine. If it's never meant to be caller-configurable, it could be removed for reduced surface area — but it's also harmless and the tests already exercise it.
  • UX label "Open Remote space" (noted in prior review, author marked skip) — noted, no further action.

@ncnthien ncnthien merged commit d2c8e28 into proj/local-web-ui Apr 21, 2026
6 checks passed
@github-actions github-actions Bot mentioned this pull request Apr 21, 2026
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants