Repository navigation
Redesign hosted WebUI around a server workspace - #5
Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f1639d09a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| configurationContent.value = ''; | ||
| inputGeneration++; | ||
| logout.hidden = false; | ||
| authCard.hidden = true; |
There was a problem hiding this comment.
After a successful login, this hides the card containing #message, but loadNodes() continues to send loading summaries and request errors there, and the failed-logout path uses it as its only feedback. Consequently, authenticated users cannot see pagination/truncation notices or the instruction to retry when sign-out cannot be confirmed; move the shared status output outside the authentication card or use a visible in-app status element.
Useful? React with 👍 / 👎.
| const registered = nodeIndex.get(backend.backendId); | ||
| details.append(text(document.createElement('span'), registered | ||
| ? `Enrolled in Control · ${registered.online ? 'Control connected' : 'Control disconnected'}` | ||
| : 'Not enrolled in Control')); |
There was a problem hiding this comment.
When an enrolled backend has not registered yet, or a registered backend's credential has subsequently been revoked, nodeIndex does not reflect its actual enrollment state: it contains only registered nodes from the current /api/v1/nodes page. The UI therefore reports “Not enrolled” for valid enrolled nodes and “Enrolled” for revoked ones; use the IDs returned by /api/v1/enrollments for this label and treat registry presence separately.
Useful? React with 👍 / 👎.
| visibleNodeItems = body.items; | ||
| nodeIndex = new Map(body.items.map(node => [node.nodeId, node])); |
There was a problem hiding this comment.
When more than 100 nodes are registered, body.items is only the requested page because the nodes API caps limit at 100, yet this page-local slice becomes the entire picker/index and feeds the Overview metrics and topology. The dashboard consequently presents partial counts as network totals, fails to correlate nodes on other pages, and silently replaces the selected server and configuration target during pagination; obtain an aggregate/full index separately or clearly scope these views without resetting the workspace selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a72641bb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function selectPrimaryServer(nodeId) { | ||
| if (nodeId && !nodeIndex.has(nodeId)) return; | ||
| selectedServerId = nodeId; | ||
| serverPicker.value = nodeId; | ||
| selectedNodes.clear(); | ||
| if (nodeId) selectedNodes.add(nodeId); | ||
| clearApprovals(); |
There was a problem hiding this comment.
When a user reads a configuration from server A and then selects capable server B, this changes the apply target but leaves configurationContent and the routing form populated with A's values. Because clearApprovals() re-enables preview whenever the retained content is nonempty, the user can preview and apply A's entire configuration to B while the page identifies B as the current context; clear target-specific form state or require a fresh read when the primary server changes.
Useful? React with 👍 / 👎.
| const page = await authorized(`/api/v1/nodes?offset=${offset}&limit=${PAGE_SIZE}`); | ||
| items.push(...page.items); | ||
| if (page.items.length < PAGE_SIZE) return items; |
There was a problem hiding this comment.
The new loadAllNodes() is fresh evidence that the earlier page-local aggregation was addressed, but every /api/v1/nodes response still caps and truncates its embedded backend summaries, and this loop discards backendItemsTruncated. With a proxy reporting over 256 backends, or a 100-node page exceeding the 4096-summary budget, allNodeItems therefore remains incomplete and the Overview backend count, issue count, topology, and proxy relationships silently omit entries; propagate truncation into those views or obtain an untruncated aggregate.
Useful? React with 👍 / 👎.
| const preservesComments = selected.acceptedCapabilities.includes('config.file-comments.v1'); | ||
| text(commentPreservationState, preservesComments ? 'Comments preserved' : 'Comments not guaranteed'); | ||
| commentPreservationState.className = `pill ${preservesComments ? 'online' : 'warning'}`; |
There was a problem hiding this comment.
When the picker server advertises config.file-comments.v1 but another checkbox-selected file target does not, this badge says “Comments preserved” even though file previews and applies are sent to every selected node with config.files.v1. The unsupported target can normalize away comments while the editor presents the batch as comment-preserving; derive this state from all active file targets or prevent mixed-capability batches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a7a12a7e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const fileTargets = targets('config.files.v1'); | ||
| const preservesComments = fileTargets.length > 0 && fileTargets.every(nodeId => | ||
| nodeCapabilities.get(nodeId)?.includes('config.file-comments.v1')); | ||
| text(commentPreservationState, preservesComments ? 'Comments preserved for every target' : 'Comments not guaranteed for every target'); | ||
| commentPreservationState.className = `pill ${preservesComments ? 'online' : 'warning'}`; |
There was a problem hiding this comment.
When a user adds or removes a config.files.v1 target using a Servers checkbox, this new every(...) calculation is not run again: the checkbox handler updates only suggestions and buttons, and opening the Configurations tab does not rerender the summary. The badge can therefore remain green after adding a target without comment support, misleading the user before a multi-node preview or apply; recalculate it whenever selectedNodes changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26373bbfd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| option.value = node.nodeId; | ||
| return option; | ||
| })); | ||
| if (!nodeIndex.has(previousValue)) selectedServerId = chooseDefaultServer(ordered)?.nodeId || ''; |
There was a problem hiding this comment.
When the selected server disappears from the registry during a refresh, this silently chooses a different default without going through selectPrimaryServer(), so configuration fields loaded from the old server remain populated while the new server becomes the apply target. After the approval is cleared, the retained YAML can still be previewed and applied to the replacement server. Fresh evidence in this revision is that manual picker changes now clear the forms, but this automatic fallback bypasses that cleanup.
Useful? React with 👍 / 👎.
| const visibleIds = new Set(registry.items.filter(node => node.online && node.acceptedCapabilities.some(value => value.startsWith('config.'))) | ||
| .map(node => node.nodeId)); | ||
| const filteredSelection = new Set([...selectedNodes].filter(node => visibleIds.has(node))); |
There was a problem hiding this comment.
When more than 100 capable nodes are registered, this registry-wide filtering preserves selections while the user pages through the Servers view, allowing 101 or more same-capability targets to accumulate. Every read or preview then sends the full set, but ConfigurationOperations.validateTargets() rejects lists larger than 100, leaving the workflow unusable until the user navigates back and deselects nodes; enforce the API limit while selecting or split the operation deliberately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97873af410
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return allNodeItems.filter(isProxy).filter(proxy => | ||
| (Array.isArray(proxy.backends) ? proxy.backends : []).some(backend => backend.backendId === backendId)); |
There was a problem hiding this comment.
When a proxy is offline but its retained topology still contains this backend ID, proxyReportsFor() includes it because the predicate never checks proxy.online. The backend card and selected-server summary then suppress the “No connected proxy reports this backend ID” state and present the stale proxy as the backend's current relationship; filter for connected proxies here or label offline reports explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e240bec3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| text(operationStatus, `At most ${MAX_CONFIGURATION_TARGETS} servers can be configured at once.`); | ||
| return; | ||
| } | ||
| if (checkbox.checked) selectedNodes.add(node.nodeId); else selectedNodes.delete(node.nodeId); |
There was a problem hiding this comment.
When the user unchecks the server currently shown in the global picker and checks another server, this updates only selectedNodes; selectedServerId and the Configurations header continue to identify the unchecked server. A subsequent file read targets the checked server and loads its YAML into an editor still labeled with the original server, so the user can edit and apply configuration under the wrong server context. Either prevent removal of the primary server, update the primary selection, or display the actual read/apply targets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ec18a1b31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const fileTargets = targets('config.files.v1'); | ||
| const preservesComments = fileTargets.length > 0 && fileTargets.every(nodeId => | ||
| nodeCapabilities.get(nodeId)?.includes('config.file-comments.v1')); |
There was a problem hiding this comment.
When a selected file target restarts or downgrades after previewing and loses config.file-comments.v1 while retaining config.files.v1, this refreshed badge becomes a warning but approvedFilePreview remains valid and the Apply button stays enabled. The resulting apply can use different comment-preservation behavior from the exact preview the user approved; treat this per-target capability as preview-relevant and require another preview whenever it changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ed82c31c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| selectedServerId = nodeId; | ||
| serverPicker.value = nodeId; | ||
| selectedNodes.clear(); | ||
| if (nodeId) selectedNodes.add(nodeId); |
There was a problem hiding this comment.
Fresh evidence after preventing the primary checkbox from being cleared: selecting an offline node or one without config.files.v1 still adds it to selectedNodes, while targets() filters it out by capability. If another checked node does qualify, a file read or preview proceeds only against that secondary node even though the Configurations header continues to identify the primary, so server B's YAML can be edited under server A's context. Disable that workflow unless the primary qualifies, or display the actual operation targets.
Useful? React with 👍 / 👎.
| text(metricNodes, allNodeItems.length); | ||
| text(metricOnline, allNodeItems.filter(node => node.online).length); | ||
| text(metricBackends, backendTopologyTruncated ? `${backendIds.size}+` : backendIds.size); | ||
| const issueCount = allNodeItems.filter(node => !node.online).length + unenrolledBackends; |
There was a problem hiding this comment.
When an enrolled backend is Control-connected but its proxy reports presenceKnown: true and available: false, this calculation contributes neither the node nor the backend to “Needs attention,” so the overview can report zero issues while the topology simultaneously says “Minecraft unavailable.” Include known-unavailable backend IDs in the issue count, ideally deduplicating them from other issue categories.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d711c38f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| text(configurationContext, | ||
| `${selected.displayName} (${selected.nodeId}) · ${roleLabel(selected)} · ${selected.online ? 'Control connected' : 'Control disconnected'}`); |
There was a problem hiding this comment.
When the primary server A and another capable server B are selected, both read handlers choose the first successful value from operation.results, whose ordering does not identify A; if B completes first or A's read fails, B's routing or YAML is loaded while this context still labels the editor as A, and that content can then be previewed against every target. Fresh evidence after keeping the primary in the target set is that readConfiguration() and readFileConfiguration() still use Object.values(...).find(...); select operation.results[selectedServerId] instead and leave the form unchanged when the primary read fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e93462df1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| selectedNodes = filteredSelection; | ||
| renderServerPicker(); | ||
| if (needsDefaultTargets && selectedServerId && | ||
| nodeCapabilities.get(selectedServerId)?.some(value => value.startsWith('config.'))) { | ||
| selectedNodes.add(selectedServerId); |
There was a problem hiding this comment.
When the selected primary server is offline during a refresh, this filtering removes it from selectedNodes; after it reconnects, selectedServerId still exists, so needsDefaultTargets is false and the server is never re-added. Because the primary server's checkbox is disabled, all configuration actions remain unavailable due to an empty target set until the user switches the picker away and back; preserve the primary selection or re-add it when it becomes eligible again.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e93462df1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| configurationContent.setSelectionRange(Math.max(lineStart, start - removable), Math.max(lineStart, end - removable)); | ||
| } | ||
| } else { | ||
| configurationContent.setRangeText(' ', start, end, 'end'); |
There was a problem hiding this comment.
When the editor contains a nonempty selection, pressing Tab passes the whole selected range to setRangeText, replacing all selected YAML with two spaces. This can silently delete multiple configuration lines when a user selects them intending to indent the block; insert indentation at each selected line while preserving the selection's contents instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 477f54f078
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const filteredSelection = new Set([...selectedNodes].filter(node => visibleIds.has(node))); | ||
| if (filteredSelection.size !== selectedNodes.size) { | ||
| renderServerPicker(); | ||
| if (selectedServerId && visibleIds.has(selectedServerId)) filteredSelection.add(selectedServerId); |
There was a problem hiding this comment.
When an offline primary has been removed from selectedNodes, the user can select 100 other capable servers; if the primary then reconnects, this unconditional add creates 101 targets. A subsequent operation sends all 101 and is rejected by ConfigurationOperations.validateTargets() because it permits at most 100 entries. Fresh evidence beyond the earlier checkbox-cap issue is this refresh path, which bypasses the checkbox's limit guard; evict another target or otherwise enforce the cap before restoring the primary.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function renderTopology() { | ||
| const proxies = allNodeItems.filter(isProxy); |
There was a problem hiding this comment.
When a proxy times out while retaining its last backend snapshot, this includes it in the topology and topologyLink() still labels those backends as “Minecraft reachable” or “Minecraft unavailable” from stale data. Fresh evidence beyond the earlier backend-relationship issue is that the Overview topology independently renders every proxy rather than using proxyReportsFor()'s online filter; preserve the configured relationship if useful, but mark its presence as unavailable/stale while the reporting proxy is disconnected.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73bdcbb441
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (backends.length === 0) { | ||
| list.append(text(document.createElement('li'), 'No configured backends reported by this proxy.')); | ||
| } else { | ||
| backends.forEach(backend => list.append(backendCard(backend))); |
There was a problem hiding this comment.
When the Servers view is refreshed after a proxy times out, its retained backend snapshot is still passed to backendCard() without the proxy's connectivity state, so those rows continue to report “Minecraft reachable” or “Minecraft unavailable” as current. The topology path now passes proxy.online and marks the same data stale, but this separate rendering path remains misleading; pass the reporter state here and suppress or label stale presence.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5b3a63c98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (enrollmentsLoaded) { | ||
| backendIds.forEach(backendId => { if (!enrollmentIds.has(backendId)) issueIds.add(backendId); }); | ||
| } |
There was a problem hiding this comment.
When an online proxy reports an available backend whose credential is enrolled but whose Control node has never registered or has been reaped, this loop adds no issue: the backend is present in enrollmentIds, absent from the initial registry-derived issueIds, and not Minecraft-unavailable. The topology simultaneously labels it “Control not registered,” so the Overview can report zero nodes needing attention despite a reported backend having no Control connection; add proxy-reported backend IDs that are absent from nodeIndex.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd6afd4520
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (let offset = 0; ; offset += PAGE_SIZE) { | ||
| const page = await authorized(`/api/v1/nodes?offset=${offset}&limit=${PAGE_SIZE}`); | ||
| items.push(...page.items); |
There was a problem hiding this comment.
When a node registers or is reaped while this multi-request offset scan is running, the stable ID ordering can shift between pages: an insertion before the current offset duplicates a boundary node and omits the new node, while a removal can skip an existing node. The resulting allNodeItems can therefore contain duplicates or omissions indefinitely until another refresh, producing incorrect metrics, duplicate picker options, and missing configuration targets; use a server-side snapshot/cursor or detect and retry an inconsistent scan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33882b8a90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| const registry = await loadAllNodes(); | ||
| visibleNodeItems = registry.items.slice(pageOffset, pageOffset + PAGE_SIZE); | ||
| allNodeItems = registry.items; |
There was a problem hiding this comment.
When a registry refresh fails after an earlier successful load, the catch path clears only the Servers list; this cached allNodeItems state, nodeIndex, capability maps, picker, metrics, and topology remain rendered from the previous refresh. The Overview can therefore continue presenting removed or changed nodes as current while the Servers tab reports that network data is unavailable; clear and rerender the workspace state on failure, or explicitly mark all retained views as stale.
Useful? React with 👍 / 👎.
|
Follow-up validation:
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Final follow-up validation:
AI-assisted implementation; changes and test coverage were reviewed before publication. @codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
config.file-comments.v1and show whether the selected backend guarantees comment preservationSafety / scope
Validation
node --check src/main/resources/web/app.jsgit diff --check