Repository navigation
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: Adds a Convert dropdown to make schema dialect upgrades discoverable in the UI.
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| document.addEventListener("keydown", (event) => { | ||
| if (event.key === "Escape") { | ||
| close(); |
There was a problem hiding this comment.
Could you check the open → Tab into a conversion link → Escape sequence at src/web/scripts/dropdown.js:27: close() hides the focused link without returning focus to the toggle. Focus falls back to the document, so keyboard users lose their position and cannot immediately reopen Convert with Enter/Space; the current Escape test keeps focus on the toggle and misses this case.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/e2e/html/hurl/html.all.hurl">
<violation number="1" location="test/e2e/html/hurl/html.all.hurl:624">
P2: This test's two `count == 0` assertions are trivially true in the community build (the `conversions` metadata is enterprise-gated in explorer.h, and schema.cc only emits the dropdown when `conversions` is non-empty), so the schema choice does not actually exercise the "nothing can be converted into" behavior the comment claims. If `html.all.hurl` also runs against the enterprise build, these assertions depend on the enterprise build treating a draft-07 metaschema with only `$schema` and `$id` as unconvertible, which nothing in this PR verifies. Consider asserting on a schema whose enterprise metadata reliably has empty `conversions`, or moving the no-conversion case to the enterprise suite alongside schemas-as.all.hurl.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| [Asserts] | ||
| header "Vary" == "Accept, Accept-Encoding" | ||
| header "Date" matches /^(Mon|Tue|Wed|Thu|Fri|Sat|Sun), (0[1-9]|[12][0-9]|3[01]) (Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec) [0-9]{4} ([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9] GMT$/ | ||
| xpath "count(//button[@data-sourcemeta-ui-dropdown-toggle])" == 0 |
There was a problem hiding this comment.
P2: This test's two count == 0 assertions are trivially true in the community build (the conversions metadata is enterprise-gated in explorer.h, and schema.cc only emits the dropdown when conversions is non-empty), so the schema choice does not actually exercise the "nothing can be converted into" behavior the comment claims. If html.all.hurl also runs against the enterprise build, these assertions depend on the enterprise build treating a draft-07 metaschema with only $schema and $id as unconvertible, which nothing in this PR verifies. Consider asserting on a schema whose enterprise metadata reliably has empty conversions, or moving the no-conversion case to the enterprise suite alongside schemas-as.all.hurl.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test/e2e/html/hurl/html.all.hurl, line 624:
<comment>This test's two `count == 0` assertions are trivially true in the community build (the `conversions` metadata is enterprise-gated in explorer.h, and schema.cc only emits the dropdown when `conversions` is non-empty), so the schema choice does not actually exercise the "nothing can be converted into" behavior the comment claims. If `html.all.hurl` also runs against the enterprise build, these assertions depend on the enterprise build treating a draft-07 metaschema with only `$schema` and `$id` as unconvertible, which nothing in this PR verifies. Consider asserting on a schema whose enterprise metadata reliably has empty `conversions`, or moving the no-conversion case to the enterprise suite alongside schemas-as.all.hurl.</comment>
<file context>
@@ -607,3 +607,20 @@ Cache-Control: no-store
+[Asserts]
+header "Vary" == "Accept, Accept-Encoding"
+header "Date" matches /^(Mon|Tue|Wed|Thu|Fri|Sat|Sun), (0[1-9]|[12][0-9]|3[01]) (Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec) [0-9]{4} ([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9] GMT$/
+xpath "count(//button[@data-sourcemeta-ui-dropdown-toggle])" == 0
+xpath "count(//ul[@data-sourcemeta-ui-dropdown-menu])" == 0
+xpath "boolean(//a[@href='/test/schemas/custom-meta-draft7.json?bundle=1'])" == true
</file context>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Benchmark (community)
Details
| Benchmark suite | Current: b6e5464 | Previous: 2af7c6b | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
147 ms |
195 ms |
0.75 |
Add one schema (100 existing) |
29 ms |
35 ms |
0.83 |
Add one schema (1000 existing) |
85 ms |
153 ms |
0.56 |
Add one schema (10000 existing) |
759 ms |
834 ms |
0.91 |
Update one schema (1 existing) |
25 ms |
26 ms |
0.96 |
Update one schema (101 existing) |
28 ms |
35 ms |
0.80 |
Update one schema (1001 existing) |
78 ms |
96 ms |
0.81 |
Update one schema (10001 existing) |
693 ms |
806 ms |
0.86 |
Cached rebuild (1 existing) |
9 ms |
10 ms |
0.90 |
Cached rebuild (101 existing) |
10 ms |
11 ms |
0.91 |
Cached rebuild (1001 existing) |
32 ms |
35 ms |
0.91 |
Cached rebuild (10001 existing) |
267 ms |
273 ms |
0.98 |
Index 100 schemas |
307 ms |
352 ms |
0.87 |
Index 1000 schemas |
851 ms |
1138 ms |
0.75 |
Index 10000 schemas |
9501 ms |
12308 ms |
0.77 |
Index 10000 schemas (custom meta-schema) |
10789 ms |
14127 ms |
0.76 |
Index 10000 schemas ($ref fan-out) |
10790 ms |
13382 ms |
0.81 |
test/e2e/html: Schema Fetch (p50) |
233 us |
302 us |
0.77 |
test/e2e/html: Schema Fetch (p99) |
412 us |
352 us |
1.17 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Benchmark (enterprise)
Details
| Benchmark suite | Current: b6e5464 | Previous: 2af7c6b | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
340 ms |
346 ms |
0.98 |
Add one schema (100 existing) |
117 ms |
119 ms |
0.98 |
Add one schema (1000 existing) |
182 ms |
176 ms |
1.03 |
Add one schema (10000 existing) |
806 ms |
937 ms |
0.86 |
Update one schema (1 existing) |
107 ms |
118 ms |
0.91 |
Update one schema (101 existing) |
126 ms |
131 ms |
0.96 |
Update one schema (1001 existing) |
178 ms |
177 ms |
1.01 |
Update one schema (10001 existing) |
1074 ms |
841 ms |
1.28 |
Cached rebuild (1 existing) |
14 ms |
15 ms |
0.93 |
Cached rebuild (101 existing) |
18 ms |
15 ms |
1.20 |
Cached rebuild (1001 existing) |
49 ms |
42 ms |
1.17 |
Cached rebuild (10001 existing) |
317 ms |
323 ms |
0.98 |
Index 100 schemas |
464 ms |
612 ms |
0.76 |
Index 1000 schemas |
1432 ms |
1742 ms |
0.82 |
Index 10000 schemas |
14342 ms |
14163 ms |
1.01 |
Index 10000 schemas (custom meta-schema) |
15782 ms |
15758 ms |
1.00 |
Index 10000 schemas ($ref fan-out) |
17703 ms |
18102 ms |
0.98 |
enterprise/e2e/auth: Schema Anonymous (p50) |
384 us |
402 us |
0.96 |
enterprise/e2e/auth: Schema Anonymous (p99) |
509 us |
524 us |
0.97 |
enterprise/e2e/auth: Schema API Key Identity (p50) |
390 us |
408 us |
0.96 |
enterprise/e2e/auth: Schema API Key Identity (p99) |
493 us |
529 us |
0.93 |
enterprise/e2e/auth: Schema API Key SHA256 (p50) |
397 us |
414 us |
0.96 |
enterprise/e2e/auth: Schema API Key SHA256 (p99) |
511 us |
548 us |
0.93 |
enterprise/e2e/auth: Schema JWT (p50) |
523 us |
541 us |
0.97 |
enterprise/e2e/auth: Schema JWT (p99) |
676 us |
713 us |
0.95 |
test/e2e/html: Schema Fetch (p50) |
394 us |
408 us |
0.97 |
test/e2e/html: Schema Fetch (p99) |
499 us |
495 us |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="enterprise/e2e/html/playwright/convert.spec.js">
<violation number="1" location="enterprise/e2e/html/playwright/convert.spec.js:109">
P2: This test never clicks anywhere: it calls `bundle.focus()`, a programmatic focus that bypasses the user interaction the title describes. The menu actually closes only as a side effect of the dropdown.js `focusout` handler, and the test never asserts the menu closed or `aria-expanded="false"`, so an outside-click closing regression would go unnoticed. Rename the test to what it does (focus moved outside is not yanked back) and assert the menu closed, or actually click outside and assert focus stays away from the toggle.</violation>
</file>
<file name="src/web/scripts/dropdown.js">
<violation number="1" location="src/web/scripts/dropdown.js:31">
P3: This focusout handler closes the dropdown when a click inside the menu lands on non-focusable space (menu padding, the gap between toggle and menu), because the browser blurs to document.body and focusout reports a non-inside relatedTarget; it also closes whenever the window loses focus (e.g. Alt-Tab). Before this change those inside clicks kept the menu open. Track in-dropdown clicks with a mousedown guard, or only close when relatedTarget is a real element that is genuinely outside, so inside clicks don't dismiss the menu.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
Comment on lines
+109
to
+120
| test('closing by clicking elsewhere leaves focus where it was put', | ||
| async ({ page }) => { | ||
| await page.goto('/test/draft3/string'); | ||
|
|
||
| const toggle = page.locator('[data-sourcemeta-ui-dropdown-toggle]'); | ||
| const bundle = page.locator('a[href="/test/draft3/string.json?bundle=1"]'); | ||
| await toggle.click(); | ||
|
|
||
| await bundle.focus(); | ||
|
|
||
| await expect(toggle).not.toBeFocused(); | ||
| await expect(bundle).toBeFocused(); |
There was a problem hiding this comment.
P2: This test never clicks anywhere: it calls bundle.focus(), a programmatic focus that bypasses the user interaction the title describes. The menu actually closes only as a side effect of the dropdown.js focusout handler, and the test never asserts the menu closed or aria-expanded="false", so an outside-click closing regression would go unnoticed. Rename the test to what it does (focus moved outside is not yanked back) and assert the menu closed, or actually click outside and assert focus stays away from the toggle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At enterprise/e2e/html/playwright/convert.spec.js, line 109:
<comment>This test never clicks anywhere: it calls `bundle.focus()`, a programmatic focus that bypasses the user interaction the title describes. The menu actually closes only as a side effect of the dropdown.js `focusout` handler, and the test never asserts the menu closed or `aria-expanded="false"`, so an outside-click closing regression would go unnoticed. Rename the test to what it does (focus moved outside is not yanked back) and assert the menu closed, or actually click outside and assert focus stays away from the toggle.</comment>
<file context>
@@ -74,6 +74,63 @@ test.describe('Schema Convert Dropdown', () => {
+ await expect(menu).not.toBeVisible();
+ });
+
+ test('closing by clicking elsewhere leaves focus where it was put',
+ async ({ page }) => {
+ await page.goto('/test/draft3/string');
</file context>
Suggested change
| test('closing by clicking elsewhere leaves focus where it was put', | |
| async ({ page }) => { | |
| await page.goto('/test/draft3/string'); | |
| const toggle = page.locator('[data-sourcemeta-ui-dropdown-toggle]'); | |
| const bundle = page.locator('a[href="/test/draft3/string.json?bundle=1"]'); | |
| await toggle.click(); | |
| await bundle.focus(); | |
| await expect(toggle).not.toBeFocused(); | |
| await expect(bundle).toBeFocused(); | |
| test('clicking elsewhere closes it without moving focus back', | |
| async ({ page }) => { | |
| await page.goto('/test/draft3/string'); | |
| const toggle = page.locator('[data-sourcemeta-ui-dropdown-toggle]'); | |
| const menu = page.locator('[data-sourcemeta-ui-dropdown-menu]'); | |
| await toggle.click(); | |
| await page.locator('footer').click(); | |
| await expect(menu).not.toBeVisible(); | |
| await expect(toggle).not.toBeFocused(); | |
| }); |
| } | ||
| }); | ||
|
|
||
| dropdown.addEventListener("focusout", (event) => { |
There was a problem hiding this comment.
P3: This focusout handler closes the dropdown when a click inside the menu lands on non-focusable space (menu padding, the gap between toggle and menu), because the browser blurs to document.body and focusout reports a non-inside relatedTarget; it also closes whenever the window loses focus (e.g. Alt-Tab). Before this change those inside clicks kept the menu open. Track in-dropdown clicks with a mousedown guard, or only close when relatedTarget is a real element that is genuinely outside, so inside clicks don't dismiss the menu.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/web/scripts/dropdown.js, line 31:
<comment>This focusout handler closes the dropdown when a click inside the menu lands on non-focusable space (menu padding, the gap between toggle and menu), because the browser blurs to document.body and focusout reports a non-inside relatedTarget; it also closes whenever the window loses focus (e.g. Alt-Tab). Before this change those inside clicks kept the menu open. Track in-dropdown clicks with a mousedown guard, or only close when relatedTarget is a real element that is genuinely outside, so inside clicks don't dismiss the menu.</comment>
<file context>
@@ -22,6 +28,12 @@ document.querySelectorAll("[data-sourcemeta-ui-dropdown]").forEach((dropdown) =>
}
});
+ dropdown.addEventListener("focusout", (event) => {
+ if (!dropdown.contains(event.relatedTarget)) {
+ close();
</file context>
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/web/helpers.h">
<violation number="1" location="src/web/helpers.h:108">
P3: This new comment ends mid-sentence at “by,” leaving the helper’s purpose unclear; replace it with a complete description.</violation>
<violation number="2" location="src/web/helpers.h:111">
P2: `dialect_short_name` duplicates the dialect→short-name mapping already defined by `conversion_name` in `enterprise/conversion/include/sourcemeta/one/enterprise_conversion.h` (identical values: "2020-12", "2019-09", "draft7", "draft6", "draft4", "draft3"). `schema.cc` now depends on these two sets staying byte-identical: the visibility filter hides a conversion when `conversion.first == declared`, and `declared` comes from `dialect_short_name` while `conversion.first` came from `conversion_name`. A rename on either side breaks the UI silently with no compile error. Share a single mapping (move it to a header both builds include) or add a test/static assertion pinning the two lists to the same canonical source.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| // The short name a dialect goes by, which is also the name a conversion into | ||
| // it is asked for by | ||
| inline auto | ||
| dialect_short_name(const sourcemeta::core::JSON::String &base_dialect_uri) |
There was a problem hiding this comment.
P2: dialect_short_name duplicates the dialect→short-name mapping already defined by conversion_name in enterprise/conversion/include/sourcemeta/one/enterprise_conversion.h (identical values: "2020-12", "2019-09", "draft7", "draft6", "draft4", "draft3"). schema.cc now depends on these two sets staying byte-identical: the visibility filter hides a conversion when conversion.first == declared, and declared comes from dialect_short_name while conversion.first came from conversion_name. A rename on either side breaks the UI silently with no compile error. Share a single mapping (move it to a header both builds include) or add a test/static assertion pinning the two lists to the same canonical source.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/web/helpers.h, line 111:
<comment>`dialect_short_name` duplicates the dialect→short-name mapping already defined by `conversion_name` in `enterprise/conversion/include/sourcemeta/one/enterprise_conversion.h` (identical values: "2020-12", "2019-09", "draft7", "draft6", "draft4", "draft3"). `schema.cc` now depends on these two sets staying byte-identical: the visibility filter hides a conversion when `conversion.first == declared`, and `declared` comes from `dialect_short_name` while `conversion.first` came from `conversion_name`. A rename on either side breaks the UI silently with no compile error. Share a single mapping (move it to a header both builds include) or add a test/static assertion pinning the two lists to the same canonical source.</comment>
<file context>
@@ -105,49 +105,56 @@ inline auto dialect_display_name(const std::string_view name) -> std::string {
- "https://json-schema.org/draft/2020-12/hyper-schema") {
- return {"2020-12", true};
- }
+dialect_short_name(const sourcemeta::core::JSON::String &base_dialect_uri)
+ -> std::string_view {
+ if (base_dialect_uri == "https://json-schema.org/draft/2020-12/schema" ||
</file context>
Comment on lines
+108
to
+109
| // The short name a dialect goes by, which is also the name a conversion into | ||
| // it is asked for by |
There was a problem hiding this comment.
P3: This new comment ends mid-sentence at “by,” leaving the helper’s purpose unclear; replace it with a complete description.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/web/helpers.h, line 108:
<comment>This new comment ends mid-sentence at “by,” leaving the helper’s purpose unclear; replace it with a complete description.</comment>
<file context>
@@ -105,49 +105,56 @@ inline auto dialect_display_name(const std::string_view name) -> std::string {
return result;
}
+// The short name a dialect goes by, which is also the name a conversion into
+// it is asked for by
inline auto
</file context>
Suggested change
| // The short name a dialect goes by, which is also the name a conversion into | |
| // it is asked for by | |
| // The short name for a dialect, also used as the target name for conversions. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Juan Cruz Viotti jv@jviotti.com