Skip to content

Feature/hub sync2 - #1106

Open
jefeish wants to merge 10 commits into
github-community-projects:yadhav/fix-recent-issuesfrom
jefeish:feature/hub-sync2
Open

jefeish wants to merge 10 commits into
github-community-projects:yadhav/fix-recent-issuesfrom
jefeish:feature/hub-sync2

Conversation

@jefeish

@jefeish jefeish commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

No description provided.

decyjphr and others added 3 commits October 2, 2026 12:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved critical API access-control, startup, and validation defects block approval.

23 open findings

And 3 more that still need to be addressed.

What changed in this PR

Integrates hub synchronization with Safe Settings webhooks and adds a dashboard for configuration browsing, organization status, logs, and imports.

Changes:

  • Adds hub-aware validation, synchronization integration, installation caching, and backend APIs.
  • Introduces dashboard pages, theme controls, and configurable URL-prefix routing.
  • Adds regression tests, configuration examples, and deployment documentation.
File Description
ui/​src/​app/​utils/​basePath.js Constructs prefixed links and API URLs.
ui/​src/​app/​not-found.jsx Adds dashboard recovery navigation.
ui/​src/​app/​layout.jsx Adds shared layout, metadata, and theme provider.
ui/​src/​app/​hooks/​useHydrated.js Tracks client hydration.
ui/​src/​app/​hooks/​useClientSafe.js Adds client-safe rendering and formatting helpers.
ui/​src/​app/​globals.css Defines dashboard theme styling.
ui/​src/​app/​dashboard/​safe-settings-hub/​page.jsx Hosts hub configuration browsing.
ui/​src/​app/​dashboard/​page.jsx Adds the dashboard landing page.
ui/​src/​app/​dashboard/​organizations/​page.jsx Hosts organization reporting.
ui/​src/​app/​dashboard/​logs/​page.jsx Displays and filters logs.
ui/​src/​app/​dashboard/​help/​page.jsx Provides dashboard usage guidance.
ui/​src/​app/​dashboard/​env/​page.jsx Hosts environment configuration reporting.
ui/​src/​app/​components/​TitleBar.jsx Adds dashboard navigation and theme controls.
ui/​src/​app/​components/​TitleBar.css Styles dashboard navigation.
ui/​src/​app/​components/​ThemeToggle.jsx Adds theme switching.
ui/​src/​app/​components/​ThemeContext.jsx Persists and applies theme preferences.
ui/​src/​app/​components/​Safe-settings-hubContent.jsx Browses hub directories and files.
ui/​src/​app/​components/​OrganizationsTable.jsx Displays organization status and import actions.
ui/​src/​app/​components/​HubOrgGraph.jsx Visualizes installed organizations.
ui/​src/​app/​components/​EnvVariables.jsx Displays searchable environment settings.
ui/​src/​app/​api/​logs/​route.js Adds a static-export log endpoint notice.
ui/​src/​app/​[slug]/​route.js Adds example static JSON routes.
ui/​shield.svg Adds shield artwork.
ui/​README.md Documents UI setup and development.
ui/​public/​favicon.svg Provides the public SVG favicon.
ui/​public/​favicon.ico Provides the ICO favicon.
ui/​package.json Defines UI dependencies and scripts.
ui/​next.config.js Configures static export and base path.
ui/​favicon.svg Adds favicon artwork.
ui/​.gitignore Excludes UI build artifacts.
ui/​.eslintrc.json Configures Next.js linting.
test/​unit/​lib/​hubSyncHandler-import.test.js Tests REST-based imports and manifest loading.
test/​unit/​lib/​env.test.js Tests URL-prefix normalization.
lib/​routes.js Serves dashboard assets and reporting/import APIs.
lib/​installationCache.js Caches app installation metadata.
lib/​env.js Adds hub configuration defaults.
index.js Integrates routes, caching, and hub webhook handling.
docs/​hubSyncHandler/​usecase-merge-behavior.md Adds a use-case documentation stub.
docs/​hubSyncHandler/​README.md Documents hub setup, configuration, and APIs.
docs/​hubSyncHandler/​IMPLEMENTATION-SUMMARY.md Summarizes manifest filtering behavior.
docs/​hubSyncHandler/​BASE_PATH.md Documents URL-prefix deployments.
docs/​hubSyncHandler/​architecture-sequence.md Describes synchronization and import flows.
docs/​hubSyncHandler/​ADR-reimport_control.md Describes re-import controls.
docs/​hubSyncHandler/​ADR-org-file-selective-sync.md Documents organization/file filtering policies.
docs/​hubSyncHandler/​ADR-manifest-control.md Records manifest-control design.
.gitignore Excludes runtime logs.
.env.example Adds hub configuration examples.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread index.js Resolved
Comment thread index.js Resolved
Comment thread lib/routes.js
Comment on lines +59 to +60
router.use(express.json({ limit: '1mb' }))
router.use(express.urlencoded({ extended: true }))
Comment thread lib/routes.js Resolved
Comment thread lib/routes.js Resolved
Comment thread ui/src/app/components/EnvVariables.jsx Resolved
Comment thread ui/src/app/globals.css Resolved
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 2, 2026 20:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new API routes lack authentication and authorization, allowing anyone who can reach the server to read private hub configuration and create branches and PRs using the app’s installation credentials.

9 open issues
2 previously missed issues

In code that hasn't changed since last review

Medium severity Import UI treats unsuccessful HTTP 200 responses as success

ui/​src/​app/​components/​OrganizationsTable.jsx:260

The import endpoint deliberately returns HTTP 200 with { ok: false, error, results: [] } for failures such as a missing hub installation. This code treats that response as success and sets retrieveResults to an empty array, which also suppresses the message/error branch in the render. Check json.ok before processing results and throw the returned error so the user sees why the import failed.

Medium severity Folder controls are inaccessible to keyboard users

ui/​src/​app/​components/​Safe-settings-hubContent.jsx:147

Unlike the file entries, folder entries are mouse-only divs: they cannot receive keyboard focus or be activated with Enter/Space. After navigating into a folder or opening a file, keyboard users cannot use the tree to return to a parent or expand sibling folders. Render this control as a native button and expose its expanded state.

16 resolved since last review

Review effort: Balanced

Comment thread index.js Resolved
Comment thread ui/package.json Resolved
Update pull request handling and modify package scripts.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 3, 2026 17:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unauthenticated API routes allow callers to read private hub configuration and create branches and PRs using the app’s installation credentials.

7 open issues
5 previously missed issues

In code that hasn't changed since last review

High severity Reject content paths that escape CONFIG_PATH

lib/​routes.js:357

The wildcard is normalized without checking that it stays inside CONFIG_PATH: /content/..%2F.env resolves outside .github to the repository-root .env file. This lets callers read files outside the endpoint's documented configuration scope using the hub installation's credentials. Reject paths that escape CONFIG_PATH before making GitHub content requests.

Medium severity Skip syntax reads for deleted hub YAML files

index.js:917

Deleting a hub YAML file makes validation fail because validateAndReportHubSync fetches every changed filename at the PR head and treats the deleted file's 404 as a syntax error. Preserve file statuses when passing changes to the validator, and skip syntax reads for removed paths while handling deletion semantics separately.

Medium severity Pass the parent environment's prefix into the UI build

ui/​next.config.js:8

Following the documented workflow of setting the prefix in the repository's .env and then running cd ui && npm run build does not supply that value here: Next.js loads environment files from the UI project directory, not the parent directory. The UI is then exported for /safe-settings while the backend reads the root .env and mounts a different prefix. Load the parent environment explicitly or pass the prefix into the UI build environment so both layers use the same setting.

Medium severity Stringify boolean values before rendering settings

ui/​src/​app/​components/​EnvVariables.jsx:132

The environment API returns CREATE_DEFAULT_BRANCH and FULL_SYNC_NOP as booleans. maskedValue() returns those values unchanged, but React renders both true and false as empty children, so their settings appear blank in this reporting page. Please stringify non-null values before rendering them.

Medium severity Include all emitted log levels in the filter

ui/​src/​app/​dashboard/​logs/​page.jsx:11

The file logger also emits FATAL, TRACE and NOTICE, and the backend uses UNKNOWN for unparsed lines such as stack-trace continuations. Because the level filter always runs and this list is also the only set of selectable levels, those entries can never be displayed—even with every checkbox selected. Please include the supported levels so fatal errors and diagnostic continuations remain accessible.

2 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 4, 2026 20:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread index.js
// the runtime provides one.
if (typeof addHandler === 'function') {
try {
setupRoutes(robot, addHandler)
Comment thread ui/.eslintrc.json
@@ -0,0 +1,3 @@
{
"extends": ["next", "next/core-web-vitals"]
Comment thread index.js
Comment on lines +917 to +920
if (hubMasterRepo && hubSyncFilesChanged.length > 0) {
robot.log.info(`Hub-sync master files detected: ${hubSyncFilesChanged.join(', ')}`)
const { validateAndReportHubSync } = require('./lib/hubSyncHandler')
return validateAndReportHubSync(robot, context, payload, pull_request, hubSyncFilesChanged, baseRef)
Comment thread lib/installationCache.js
async function ensureFresh (robot) {
const ttl = getTtlMs()
if (!lastFetchedAt || (Date.now() - lastFetchedAt.getTime()) > ttl) {
try { await refresh(robot) } catch (_) { /* stale ok */ }
Comment thread ui/src/app/components/EnvVariables.jsx Resolved
Comment thread ui/src/app/dashboard/logs/page.jsx Resolved
Copilot AI balanced review requested due to automatic review settings November 4, 2026 20:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread index.js
robot.on('pull_request.closed', async context => {
if (!context.payload.pull_request?.merged || context.payload.pull_request.base.ref !== context.payload.repository.default_branch) return null
try {
await hubSyncHandler(robot, context)
Comment thread docs/hubSyncHandler/README.md Resolved
const fetchData = () => {
if (!hydrated) return;
setLoading(true); setError(null);
fetch(withBasePath('/api/safe-settings/hub/content?fetchContent=true'))
Comment thread docs/hubSyncHandler/BASE_PATH.md Resolved
Updated deployment instructions and UI rebuild notes. Changed header to button for better accessibility.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 4, 2026 20:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot AI balanced review requested due to automatic review settings November 4, 2026 22:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot AI balanced review requested due to automatic review settings November 4, 2026 23:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch has not been deployed

No deployments
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.

3 participants