Add server browser popup with auto-refresh from ninbot.com API by nykwil · Pull Request #1 · nykwil/ninjam-next-plugin

nykwil · 2026-03-31T07:30:24Z

ServerBrowserComponent: TableListBox showing servers fetched from
http://ninbot.com/app/servers.php, sorted by active user count
FetchThread: background juce::Thread with WeakReference safety,
posts result back to message thread via callAsync
Auto-refreshes every 30 seconds; manual Refresh button also available
Double-click or Use Server fills the host field and closes dialog
Browse... button added to connection row in main editor

https://claude.ai/code/session_01F92j9HpCksU8TBEkeBhWaT

- ServerBrowserComponent: TableListBox showing servers fetched from http://ninbot.com/app/servers.php, sorted by active user count - FetchThread: background juce::Thread with WeakReference safety, posts result back to message thread via callAsync - Auto-refreshes every 30 seconds; manual Refresh button also available - Double-click or Use Server fills the host field and closes dialog - Browse... button added to connection row in main editor https://claude.ai/code/session_01F92j9HpCksU8TBEkeBhWaT

gemini-code-assist

Code Review

This pull request introduces a server browser component that allows users to fetch and select public Ninjam servers. The implementation includes an asynchronous fetch thread, a table display for server details, and an auto-refresh mechanism. The review feedback highlights several critical improvements: avoiding the anti-pattern of starting threads within constructors to prevent race conditions, replacing a static timer variable with an instance member to avoid state sharing between multiple browser windows, and enhancing JSON parsing logic to ensure the UI correctly reports errors instead of showing a false success status.

gemini-code-assist · 2026-03-31T07:32:22Z

+ServerBrowserComponent::FetchThread::FetchThread(juce::WeakReference<ServerBrowserComponent> ws)
+  : juce::Thread("NinjamServerFetch"), weakSelf(ws)
+{
+  startThread();
+}


Starting a thread in its constructor is generally considered an anti-pattern in C++. If the thread's run() method begins executing before the constructor has finished, it could access partially initialized members. It is safer to call startThread() from the code that creates the thread object.

ServerBrowserComponent::FetchThread::FetchThread(juce::WeakReference<ServerBrowserComponent> ws) : juce::Thread("NinjamServerFetch"), weakSelf(ws) { }

gemini-code-assist · 2026-03-31T07:32:22Z

+  static int ticksUntilRefresh = 30;
+  if (--ticksUntilRefresh <= 0)
+  {
+    ticksUntilRefresh = 30;
+    fetchServers();
+  }


Using a static variable for the refresh timer means the state is shared across all instances of ServerBrowserComponent. If multiple browser windows are opened, they will interfere with each other's countdown, causing refreshes to happen much more frequently than every 30 seconds. This should be a member variable instead.

Suggested change

static int ticksUntilRefresh = 30;

if (--ticksUntilRefresh <= 0)

{

ticksUntilRefresh = 30;

fetchServers();

}

if (--ticksUntilRefresh <= 0)

fetchServers();

gemini-code-assist · 2026-03-31T07:32:22Z

+  fetching = true;
+  statusLabel.setText("Fetching...", juce::dontSendNotification);
+  fetchThread = std::make_unique<FetchThread>(juce::WeakReference<ServerBrowserComponent>(this));


The refresh timer should be reset when a fetch is initiated (either automatically or manually). Also, since startThread() was removed from the FetchThread constructor for safety, it should be called here after the thread object is created.

fetching = true; ticksUntilRefresh = 30; statusLabel.setText("Fetching...", juce::dontSendNotification); fetchThread = std::make_unique<FetchThread>(juce::WeakReference<ServerBrowserComponent>(this)); fetchThread->startThread();

gemini-code-assist · 2026-03-31T07:32:22Z

+  juce::Array<ServerEntry> newServers;
+  auto parsed = juce::JSON::parse(json);
+  if (auto* obj = parsed.getDynamicObject())
+  {
+    if (auto* arr = obj->getProperty("servers").getArray())
+    {
+      for (const auto& item : *arr)
+      {
+        if (auto* s = item.getDynamicObject())
+        {
+          ServerEntry entry;
+          entry.name      = s->getProperty("name").toString();
+          entry.host      = s->getProperty("host").toString();
+          entry.port      = s->getProperty("port").toString();
+          entry.bpm       = s->getProperty("bpm").toString().getIntValue();
+          entry.bpi       = s->getProperty("bpi").toString().getIntValue();
+          entry.userCount = s->getProperty("user_count").toString().getIntValue();
+          entry.userLimit = s->getProperty("user_limit").toString().getIntValue();
+          if (entry.host.isNotEmpty() && entry.port.isNotEmpty())
+            newServers.add(entry);
+        }
+      }
+    }
+  }
+
+  std::sort(newServers.begin(), newServers.end(), [](const ServerEntry& a, const ServerEntry& b) {
+    return a.userCount > b.userCount;
+  });
+
+  servers = std::move(newServers);
+  table.updateContent();
+
+  if (json.isEmpty())
+    statusLabel.setText("Error: could not reach server list", juce::dontSendNotification);
+  else
+    statusLabel.setText("Updated " + juce::Time::getCurrentTime().formatted("%H:%M:%S")
+                        + "  (double-click or Use Server to connect)",
+                        juce::dontSendNotification);
+}


The current logic shows an 'Updated' status even if JSON parsing fails or the expected 'servers' array is missing. This can be misleading to the user, who might see an empty list and a success message. It's better to verify the parsing success before updating the UI state.

juce::Array<ServerEntry> newServers; auto parsed = juce::JSON::parse(json); bool parseSuccess = false; if (auto* obj = parsed.getDynamicObject()) { if (auto* arr = obj->getProperty("servers").getArray()) { parseSuccess = true; for (const auto& item : *arr) { if (auto* s = item.getDynamicObject()) { ServerEntry entry; entry.name = s->getProperty("name").toString(); entry.host = s->getProperty("host").toString(); entry.port = s->getProperty("port").toString(); entry.bpm = s->getProperty("bpm").toString().getIntValue(); entry.bpi = s->getProperty("bpi").toString().getIntValue(); entry.userCount = s->getProperty("user_count").toString().getIntValue(); entry.userLimit = s->getProperty("user_limit").toString().getIntValue(); if (entry.host.isNotEmpty() && entry.port.isNotEmpty()) newServers.add(entry); } } } } if (parseSuccess) { std::sort(newServers.begin(), newServers.end(), [](const ServerEntry& a, const ServerEntry& b) { return a.userCount > b.userCount; }); servers = std::move(newServers); table.updateContent(); statusLabel.setText("Updated " + juce::Time::getCurrentTime().formatted("%H:%M:%S") + " (double-click or Use Server to connect)", juce::dontSendNotification); } else { statusLabel.setText(json.isEmpty() ? "Error: could not reach server list" : "Error: invalid server list data", juce::dontSendNotification); }

gemini-code-assist · 2026-03-31T07:32:22Z

+
+  juce::Array<ServerEntry> servers;
+  std::unique_ptr<FetchThread> fetchThread;
+  bool fetching = false;


Add a member variable to track the refresh countdown, replacing the static variable in the implementation file.

Suggested change

bool fetching = false;

bool fetching = false;

int ticksUntilRefresh = 30;

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a04652f400

ℹ️ 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".

chatgpt-codex-connector · 2026-03-31T07:33:54Z

+
+void ServerBrowserComponent::FetchThread::run()
+{
+  juce::URL url("http://ninbot.com/app/servers.php");


Fetch server list over HTTPS

The server browser pulls data from http://ninbot.com/... without TLS, so anyone on-path (public Wi‑Fi, proxy, corporate gateway) can tamper with the JSON and inject attacker-controlled hosts into the selectable list. Because selecting an entry auto-fills the connection target, this can redirect users to malicious servers and expose credentials. Use an HTTPS endpoint (and fail/flag when TLS is unavailable) to protect integrity of the server list.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-03-31T07:33:54Z

+  if (json.isEmpty())
+    statusLabel.setText("Error: could not reach server list", juce::dontSendNotification);
+  else
+    statusLabel.setText("Updated " + juce::Time::getCurrentTime().formatted("%H:%M:%S")


Treat non-JSON fetch responses as errors

The status message only checks whether the raw response is empty, so any non-empty invalid payload (for example an HTML error page, rate-limit text, or proxy banner) is shown as a successful "Updated" refresh while the parsed server list is actually empty. That makes failures silent and misleading for users. The success path should depend on parse/shape validity (e.g., valid servers array) rather than json.isEmpty().

Useful? React with 👍 / 👎.

nykwil · 2026-03-31T17:12:21Z

Merged into dev manually

Merge PR #1 (server browser popup with ninbot.com API). Fix timerCallback using static local instead of member variable for the auto-refresh countdown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gemini-code-assist Bot reviewed Mar 31, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Mar 31, 2026

View reviewed changes

nykwil closed this Mar 31, 2026

nykwil deleted the claude/research-server-browser-dpdb8 branch March 31, 2026 17:12

Navigation Menu

Add server browser popup with auto-refresh from ninbot.com API - #1

Add server browser popup with auto-refresh from ninbot.com API#1
nykwil wants to merge 1 commit into
mainfrom
claude/research-server-browser-dpdb8

nykwil commented Mar 31, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Mar 31, 2026

Uh oh!

chatgpt-codex-connector Bot Mar 31, 2026

Uh oh!

nykwil commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	bool fetching = false;
	bool fetching = false;
	int ticksUntilRefresh = 30;

Conversation

nykwil commented Mar 31, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

nykwil commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants