Skip to content

Centralize routed item builders#229

Open
mariusvniekerk wants to merge 2 commits intomainfrom
architecture-route-items
Open

Centralize routed item builders#229
mariusvniekerk wants to merge 2 commits intomainfrom
architecture-route-items

Conversation

@mariusvniekerk
Copy link
Copy Markdown
Collaborator

Summary

  • add shared route builders for pull requests, issues, focus lists, and routed item refs
  • replace scattered string-built item links across frontend and packages/ui callers
  • document the shared route helper convention in the UI context notes

How this makes the code better

  • gives navigation callers one vocabulary for item route construction instead of repeating URL string templates
  • preserves platform_host query handling through typed helpers, reducing host-qualified issue link drift
  • makes route changes easier because list, detail, sidebar, and activity selection callers depend on the same route module

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 1, 2026

roborev: Combined Review (9d460de)

Medium-confidence concern: route-builder coverage may be insufficient for the routed maintainer workflows; no security issues were reported.

Medium

  • packages/ui/src/routes.test.ts:1 - The route-builder refactor changes PR, issue, and focus navigation paths across maintainer workflows, but current coverage appears limited to isolated unit tests. Add e2e coverage for selecting PRs, issues with platform_host, and focus routes through the real UI/API-backed routing flow.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 1, 2026

roborev: Combined Review (5f79435)

No Medium, High, or Critical issues found.

All reviewers reported the code is clean.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@mariusvniekerk mariusvniekerk marked this pull request as draft May 1, 2026 23:51
mariusvniekerk and others added 2 commits May 2, 2026 16:06
PR and issue routes were split across the browser router, sidebar lists, focus views, and repository cards, which made each caller repeat the same owner/name/number/platformHost shape and URL encoding rules. Centralizing those refs and builders in the UI package gives routed item identity one home while keeping the frontend router focused on adapting browser location state.

This deepens the route item module so new PR, issue, files, and focus callers can import the same named contracts instead of recreating URL strings locally. That improves locality and keeps issue platform host query handling uniform across list, focus, drawer, and repo-summary flows.

Validation: bun run --cwd frontend typecheck; bun run --cwd packages/ui typecheck; bun run --cwd frontend test -- routes.test.ts router.test.ts router.initialization.test.ts activitySelection.test.ts RepoSummaryPage.test.ts; bun run --cwd frontend lint; bun run --cwd packages/ui lint; git diff --check.

Co-authored-by: OpenAI Codex <noreply@openai.com>
Add full-stack e2e coverage for PR, issue, and focus routes so the shared route builders keep platform_host and API detail requests aligned.
@mariusvniekerk mariusvniekerk force-pushed the architecture-route-items branch from 5f79435 to e1396e8 Compare May 2, 2026 20:07
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 2, 2026

roborev: Combined Review (e1396e8)

No Medium, High, or Critical issues found.

All reviewers agree the changes are clean.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@mariusvniekerk mariusvniekerk marked this pull request as ready for review May 3, 2026 04:19
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.

1 participant