Skip to content

feat: use deprecated tag for measurements#847

Open
alexey-yarmosh wants to merge 1 commit into
masterfrom
username-change
Open

feat: use deprecated tag for measurements#847
alexey-yarmosh wants to merge 1 commit into
masterfrom
username-change

Conversation

@alexey-yarmosh
Copy link
Copy Markdown
Member

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

Walkthrough

This PR extends the adopted-probes system to support a deprecatedPrefix field alongside the existing defaultPrefix. A new deprecated_prefix column is added to the directus_users table schema, the DProbe type is extended to include the field, and the fetchDProbes() query now loads this value from the database. The getUpdatedTags method applies the deprecated prefix to system tags using the same logic pattern as the default prefix. Type safety is maintained by updating the formatProbeAsDProbe return type omission list. Unit tests are added to verify the tag computation behavior.

Possibly related PRs

  • jsdelivr/globalping#791: Both PRs update src/lib/override/adopted-probes.ts—the main PR adds deprecatedPrefix handling including formatProbeAsDProbe, while the retrieved PR adds localAdoptionServer persistence and also extends formatProbeAsDProbe—so they overlap at the same probe-formatting function despite different fields.

Suggested reviewers

  • MartinKolarik
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description references the associated GitHub issue but provides minimal detail about the actual changes being made in this pull request. Consider expanding the description to briefly explain what changes were made and how the deprecated tag for measurements is being implemented.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: use deprecated tag for measurements' accurately describes the main purpose of the changes, which add deprecated prefix tracking and tagging functionality.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch username-change

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

src/lib/override/adopted-probes.ts

Oops! Something went wrong! :(

ESLint: 10.4.1

SyntaxError: Unexpected token ':'
at compileSourceTextModule (node:internal/modules/esm/utils:318:16)
at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:90:18)
at #translate (node:internal/modules/esm/loader:451:20)
at afterLoad (node:internal/modules/esm/loader:507:29)
at ModuleLoader.loadAndTranslate (node:internal/modules/esm/loader:512:12)
at #getOrCreateModuleJobAfterResolve (node:internal/modules/esm/loader:555:36)
at afterResolve (node:internal/modules/esm/loader:603:52)
at ModuleLoader.getOrCreateModuleJob (node:internal/modules/esm/loader:609:12)
at node:internal/modules/esm/loader:628:32
at TracingChannel.tracePromise (node:diagnostics_channel:362:14)

test/tests/unit/override/adopted-probes.test.ts

Oops! Something went wrong! :(

ESLint: 10.4.1

SyntaxError: Unexpected token ':'
at compileSourceTextModule (node:internal/modules/esm/utils:318:16)
at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:90:18)
at #translate (node:internal/modules/esm/loader:451:20)
at afterLoad (node:internal/modules/esm/loader:507:29)
at ModuleLoader.loadAndTranslate (node:internal/modules/esm/loader:512:12)
at #getOrCreateModuleJobAfterResolve (node:internal/modules/esm/loader:555:36)
at afterResolve (node:internal/modules/esm/loader:603:52)
at ModuleLoader.getOrCreateModuleJob (node:internal/modules/esm/loader:609:12)
at node:internal/modules/esm/loader:628:32
at TracingChannel.tracePromise (node:diagnostics_channel:362:14)


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/tests/unit/override/adopted-probes.test.ts (1)

1391-1405: ⚡ Quick win

Add coverage for equal-prefix deduping.

Please add a test where defaultPrefix and deprecatedPrefix are identical and assert only one u-* system tag is returned.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/tests/unit/override/adopted-probes.test.ts` around lines 1391 - 1405,
Add a unit test to cover equal-prefix deduping: create an AdoptedProbes instance
(AdoptedProbes) and stub sql.select to return an adoption record where
deprecatedPrefix equals the defaultPrefix (set deprecatedPrefix to the same
value as defaultAdoption.defaultPrefix or defaultPrefix value used in other
tests), call syncDashboardData(), then call
getUpdatedTags(defaultConnectedProbe) and assert that only a single "u-<prefix>"
system tag is present (i.e., no duplicate u-* tag); reference getUpdatedTags and
the deprecatedPrefix/defaultPrefix fields to locate where to change the test
input and the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/override/adopted-probes.ts`:
- Around line 295-298: The spread that conditionally adds a system tag uses
adoption.deprecatedPrefix without checking whether it matches the default
prefix, causing duplicate u-* tags when deprecatedPrefix === defaultPrefix;
update the conditional around the spread (the expression building the array with
AdoptedProbes.getGlobalUserTag(adoption.deprecatedPrefix)) to require
adoption.publicProbes && adoption.deprecatedPrefix && adoption.deprecatedPrefix
!== defaultPrefix so the tag is only appended when the deprecated prefix is
present and different from defaultPrefix.

---

Nitpick comments:
In `@test/tests/unit/override/adopted-probes.test.ts`:
- Around line 1391-1405: Add a unit test to cover equal-prefix deduping: create
an AdoptedProbes instance (AdoptedProbes) and stub sql.select to return an
adoption record where deprecatedPrefix equals the defaultPrefix (set
deprecatedPrefix to the same value as defaultAdoption.defaultPrefix or
defaultPrefix value used in other tests), call syncDashboardData(), then call
getUpdatedTags(defaultConnectedProbe) and assert that only a single "u-<prefix>"
system tag is present (i.e., no duplicate u-* tag); reference getUpdatedTags and
the deprecatedPrefix/defaultPrefix fields to locate where to change the test
input and the assertion.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8e6987d7-0988-4fb4-aa8b-f8bb538faed0

📥 Commits

Reviewing files that changed from the base of the PR and between f2534b4 and 557027e.

📒 Files selected for processing (3)
  • migrations/dashboard/create-tables.js.sql
  • src/lib/override/adopted-probes.ts
  • test/tests/unit/override/adopted-probes.test.ts

Comment thread src/lib/override/adopted-probes.ts
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