fix(cockpit-examples): use CSS sidebar widths#779
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @blove's task in 2m 59s —— View job PR Review
SummaryThe core fix is correct and well-executed. All 14 Two issues worth addressing before merge: Medium — Hardcoded spec indices in streaming harness
Low —
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const stateFixture = TestBed.createComponent(StateManagementComponent); | ||
| const stateComponent = stateFixture.componentInstance as unknown as RenderExampleHarness; | ||
| stateComponent.simulator.setSource(STATE_MANAGEMENT_SPECS[1].json); | ||
| stateComponent.simulator.seek(STATE_MANAGEMENT_SPECS[1].json.length); | ||
| stateFixture.detectChanges(); | ||
|
|
||
| const computedFixture = TestBed.createComponent(ComputedFunctionsComponent); | ||
| const computedComponent = computedFixture.componentInstance as unknown as RenderExampleHarness; | ||
| computedComponent.simulator.setSource(COMPUTED_FUNCTIONS_SPECS[1].json); | ||
| computedComponent.simulator.seek(COMPUTED_FUNCTIONS_SPECS[1].json.length); | ||
| computedFixture.detectChanges(); | ||
|
|
||
| expect((stateFixture.nativeElement as HTMLElement).textContent).toContain('30'); | ||
| expect((computedFixture.nativeElement as HTMLElement).textContent).toContain('42'); |
There was a problem hiding this comment.
Magic indices make this test silently validate the wrong fixture if STATE_MANAGEMENT_SPECS or COMPUTED_FUNCTIONS_SPECS are ever reordered or a spec is inserted before index 1.
| const stateFixture = TestBed.createComponent(StateManagementComponent); | |
| const stateComponent = stateFixture.componentInstance as unknown as RenderExampleHarness; | |
| stateComponent.simulator.setSource(STATE_MANAGEMENT_SPECS[1].json); | |
| stateComponent.simulator.seek(STATE_MANAGEMENT_SPECS[1].json.length); | |
| stateFixture.detectChanges(); | |
| const computedFixture = TestBed.createComponent(ComputedFunctionsComponent); | |
| const computedComponent = computedFixture.componentInstance as unknown as RenderExampleHarness; | |
| computedComponent.simulator.setSource(COMPUTED_FUNCTIONS_SPECS[1].json); | |
| computedComponent.simulator.seek(COMPUTED_FUNCTIONS_SPECS[1].json.length); | |
| computedFixture.detectChanges(); | |
| expect((stateFixture.nativeElement as HTMLElement).textContent).toContain('30'); | |
| expect((computedFixture.nativeElement as HTMLElement).textContent).toContain('42'); | |
| const statePrimSpec = STATE_MANAGEMENT_SPECS.find((s) => s.label === 'Nested Paths')!; | |
| stateComponent.simulator.setSource(statePrimSpec.json); | |
| stateComponent.simulator.seek(statePrimSpec.json.length); | |
| stateFixture.detectChanges(); | |
| const computedFixture = TestBed.createComponent(ComputedFunctionsComponent); | |
| const computedComponent = computedFixture.componentInstance as unknown as RenderExampleHarness; | |
| const computedPrimSpec = COMPUTED_FUNCTIONS_SPECS.find((s) => s.label === 'Data Display')!; | |
| computedComponent.simulator.setSource(computedPrimSpec.json); | |
| computedComponent.simulator.seek(computedPrimSpec.json.length); | |
| computedFixture.detectChanges(); |
Also note this test doesn't call await fixture.whenStable() after detectChanges(), unlike the loop-based tests above (line 62). Works today because signal propagation is synchronous, but it's an asymmetry worth closing to avoid latent flakiness if an async effect is added.
| function parseJsonRenderSpecs(source: string): unknown[] { | ||
| const specs: unknown[] = []; | ||
| const pattern = /json:\s*JSON\.stringify\(([\s\S]*?),\s*null,\s*2\)/g; | ||
| for (const match of source.matchAll(pattern)) { | ||
| specs.push(Function(`"use strict"; return (${match[1]});`)()); | ||
| } | ||
| return specs; |
There was a problem hiding this comment.
Function() is effectively eval() over spec source text. For version-controlled fixtures the practical risk is low, but any spec file that happens to contain a JavaScript expression with side-effects (e.g. a computed initializer calling process.exit) will execute at test time. Consider statically importing the spec arrays instead of parsing their source:
// In each specs.ts, export a named constant the test can import directly.
// Then collectDirectiveNames() can run on the imported value without eval.
import { STATE_MANAGEMENT_SPECS } from '../state-management/angular/src/app/specs';
// …and so on for the other render examples.If the regex-parse approach must stay, at minimum validate that the captured expression only looks like an object/array literal before passing it to Function.
blove
force-pushed
the
blove/examples-latent-bug-sweep
branch
from
466a271 to
92f3bd6
Compare
|
Claude finished @blove's task in 2m 34s —— View job PR Review & Fixes
Both issues from the prior review have been implemented locally and committed. The commit is staged but requires push approval to land on the branch. Changes made
Replaced const statePrimSpec = STATE_MANAGEMENT_SPECS.find((s) => s.label === 'Nested Paths')!;
stateComponent.simulator.setSource(statePrimSpec.json);
stateComponent.simulator.seek(statePrimSpec.json.length);
stateFixture.detectChanges();
await stateFixture.whenStable();
const computedPrimSpec = COMPUTED_FUNCTIONS_SPECS.find((s) => s.label === 'Data Display')!;
computedComponent.simulator.setSource(computedPrimSpec.json);
computedComponent.simulator.seek(computedPrimSpec.json.length);
computedFixture.detectChanges();
await computedFixture.whenStable();
Removed import { REGISTRY_SPECS } from '../registry/angular/src/app/specs';
import { SPEC_RENDERING_SPECS } from '../spec-rendering/angular/src/app/specs';
// … four more …
const ALL_RENDER_SPECS = [
...REGISTRY_SPECS, ...SPEC_RENDERING_SPECS, ...ELEMENT_RENDERING_SPECS,
...STATE_MANAGEMENT_SPECS, ...REPEAT_LOOPS_SPECS, ...COMPUTED_FUNCTIONS_SPECS,
];
// in the test:
for (const spec of ALL_RENDER_SPECS) {
for (const directive of collectDirectiveNames(JSON.parse(spec.json))) {
if (!SUPPORTED_RENDER_DIRECTIVES.has(directive)) {
unsupported.push(`${spec.label}: ${directive}`);
}
}
}The
|
|
Claude finished @blove's task in 3m 41s —— View job PR Review
SummaryCore fix is correct. All 14 Two issues worth fixing before merge: Medium — Magic indices + missing
|
| stateComponent.simulator.setSource(STATE_MANAGEMENT_SPECS[1].json); | ||
| stateComponent.simulator.seek(STATE_MANAGEMENT_SPECS[1].json.length); | ||
| stateFixture.detectChanges(); | ||
|
|
||
| const computedFixture = TestBed.createComponent(ComputedFunctionsComponent); | ||
| const computedComponent = computedFixture.componentInstance as unknown as RenderExampleHarness; | ||
| computedComponent.simulator.setSource(COMPUTED_FUNCTIONS_SPECS[1].json); | ||
| computedComponent.simulator.seek(COMPUTED_FUNCTIONS_SPECS[1].json.length); | ||
| computedFixture.detectChanges(); |
There was a problem hiding this comment.
Two issues here, both confirmed after reading the actual spec arrays:
Magic indices — STATE_MANAGEMENT_SPECS[1] is "Nested Paths" and COMPUTED_FUNCTIONS_SPECS[1] is "Data Display" right now, so the '30' / '42' assertions are correct. But if either array is reordered, this test silently validates the wrong fixture. The previous review's inline suggestion is the right fix.
Missing await fixture.whenStable() — the loop-based tests above (lines 62–63) call detectChanges() then await fixture.whenStable() then detectChanges() again. This standalone test calls detectChanges() only once per fixture and skips whenStable(). Works today because signal propagation is synchronous, but it's an asymmetry that will bite if an async effect is added to either component.
| stateComponent.simulator.setSource(STATE_MANAGEMENT_SPECS[1].json); | |
| stateComponent.simulator.seek(STATE_MANAGEMENT_SPECS[1].json.length); | |
| stateFixture.detectChanges(); | |
| const computedFixture = TestBed.createComponent(ComputedFunctionsComponent); | |
| const computedComponent = computedFixture.componentInstance as unknown as RenderExampleHarness; | |
| computedComponent.simulator.setSource(COMPUTED_FUNCTIONS_SPECS[1].json); | |
| computedComponent.simulator.seek(COMPUTED_FUNCTIONS_SPECS[1].json.length); | |
| computedFixture.detectChanges(); | |
| const statePrimSpec = STATE_MANAGEMENT_SPECS.find((s) => s.label === 'Nested Paths')!; | |
| stateComponent.simulator.setSource(statePrimSpec.json); | |
| stateComponent.simulator.seek(statePrimSpec.json.length); | |
| stateFixture.detectChanges(); | |
| await stateFixture.whenStable(); | |
| const computedFixture = TestBed.createComponent(ComputedFunctionsComponent); | |
| const computedComponent = computedFixture.componentInstance as unknown as RenderExampleHarness; | |
| const computedPrimSpec = COMPUTED_FUNCTIONS_SPECS.find((s) => s.label === 'Data Display')!; | |
| computedComponent.simulator.setSource(computedPrimSpec.json); | |
| computedComponent.simulator.seek(computedPrimSpec.json.length); | |
| computedFixture.detectChanges(); | |
| await computedFixture.whenStable(); |
| const specs: unknown[] = []; | ||
| const pattern = /json:\s*JSON\.stringify\(([\s\S]*?),\s*null,\s*2\)/g; | ||
| for (const match of source.matchAll(pattern)) { | ||
| specs.push(Function(`"use strict"; return (${match[1]});`)()); |
There was a problem hiding this comment.
Function() is effectively eval(). The practical risk for version-controlled fixtures is low, but there's a simpler fix available: each spec file already exports a named *_SPECS array where spec.json is the result of JSON.stringify(obj, null, 2). You can import those arrays statically and round-trip with JSON.parse instead of evaluating source text:
// Replace parseJsonRenderSpecs + this loop with static imports:
import { STATE_MANAGEMENT_SPECS } from '../state-management/angular/src/app/specs';
import { ELEMENT_RENDERING_SPECS } from '../element-rendering/angular/src/app/specs';
// ... other spec arrays
const ALL_SPECS = [
...STATE_MANAGEMENT_SPECS,
...ELEMENT_RENDERING_SPECS,
// ...
];
for (const spec of ALL_SPECS) {
const obj = JSON.parse(spec.json);
for (const directive of collectDirectiveNames(obj)) { ... }
}Tradeoff: the static list needs to be updated when new render examples are added, unlike the current dynamic scan. If you want to keep dynamic discovery, dynamic import() over renderSpecFiles() paths avoids Function() while preserving the automation.
blove
force-pushed
the
blove/examples-latent-bug-sweep
branch
from
92f3bd6 to
a2633a3
Compare
|
Claude finished @blove's task in 2m 50s —— View job PR Review & Fixes
SummaryThe core fix (14 Medium — Magic indices in streaming harness
Low —
|
| computedFixture.detectChanges(); | ||
|
|
||
| expect((stateFixture.nativeElement as HTMLElement).textContent).toContain('30'); | ||
| expect((computedFixture.nativeElement as HTMLElement).textContent).toContain('42'); |
There was a problem hiding this comment.
Medium — Magic indices + missing whenStable
STATE_MANAGEMENT_SPECS[1] is "Nested Paths" and COMPUTED_FUNCTIONS_SPECS[1] is "Data Display" today — the '30' / '42' assertions are factually correct. But index coupling means a spec reorder silently validates the wrong fixture, and both detectChanges() calls skip the await whenStable() that the loop tests above use (lines 61–63).
| expect((computedFixture.nativeElement as HTMLElement).textContent).toContain('42'); | |
| const stateFixture = TestBed.createComponent(StateManagementComponent); | |
| const stateComponent = stateFixture.componentInstance as unknown as RenderExampleHarness; | |
| const statePrimSpec = STATE_MANAGEMENT_SPECS.find((s) => s.label === 'Nested Paths')!; | |
| stateComponent.simulator.setSource(statePrimSpec.json); | |
| stateComponent.simulator.seek(statePrimSpec.json.length); | |
| stateFixture.detectChanges(); | |
| await stateFixture.whenStable(); | |
| const computedFixture = TestBed.createComponent(ComputedFunctionsComponent); | |
| const computedComponent = computedFixture.componentInstance as unknown as RenderExampleHarness; | |
| const computedPrimSpec = COMPUTED_FUNCTIONS_SPECS.find((s) => s.label === 'Data Display')!; | |
| computedComponent.simulator.setSource(computedPrimSpec.json); | |
| computedComponent.simulator.seek(computedPrimSpec.json.length); | |
| computedFixture.detectChanges(); | |
| await computedFixture.whenStable(); |
| const specs: unknown[] = []; | ||
| const pattern = /json:\s*JSON\.stringify\(([\s\S]*?),\s*null,\s*2\)/g; | ||
| for (const match of source.matchAll(pattern)) { | ||
| specs.push(Function(`"use strict"; return (${match[1]});`)()); |
There was a problem hiding this comment.
Low — Function() is effectively eval()
The streaming harness file already statically imports all six *_SPECS arrays. Use the same pattern here — import them, concatenate into ALL_RENDER_SPECS, and round-trip with JSON.parse(spec.json). This removes the eval surface entirely and the error message on failure becomes ${spec.label}: ${directive} instead of a file path, which is more readable.
| specs.push(Function(`"use strict"; return (${match[1]});`)()); | |
| function parseJsonRenderSpecs(source: string): unknown[] { |
Replace renderSpecFiles(), parseJsonRenderSpecs(), and the eval-based loop with:
import { COMPUTED_FUNCTIONS_SPECS } from '../computed-functions/angular/src/app/specs';
import { ELEMENT_RENDERING_SPECS } from '../element-rendering/angular/src/app/specs';
import { REGISTRY_SPECS } from '../registry/angular/src/app/specs';
import { REPEAT_LOOPS_SPECS } from '../repeat-loops/angular/src/app/specs';
import { SPEC_RENDERING_SPECS } from '../spec-rendering/angular/src/app/specs';
import { STATE_MANAGEMENT_SPECS } from '../state-management/angular/src/app/specs';
const ALL_RENDER_SPECS = [
...REGISTRY_SPECS, ...SPEC_RENDERING_SPECS, ...ELEMENT_RENDERING_SPECS,
...STATE_MANAGEMENT_SPECS, ...REPEAT_LOOPS_SPECS, ...COMPUTED_FUNCTIONS_SPECS,
];
// in the test:
for (const spec of ALL_RENDER_SPECS) {
for (const directive of collectDirectiveNames(JSON.parse(spec.json))) {
if (!SUPPORTED_RENDER_DIRECTIVES.has(directive)) {
unsupported.push(`${spec.label}: ${directive}`);
}
}
}1 participant
Summary
example-chat-layoutsidebarWidthwith CSS length values across chat and deep-agents cockpit examples.sidebarWidthdead-prop regressions.[object Object]/blank render regressions plus numeric display text.Findings
cockpit/chat/threads/angular/src/app/threads.component.tspassedsidebarWidth="w-64"to an input consumed as a CSS custom property, so the width was a silent no-op. Fixed to16rem.w-72orw-80into the same CSS-length input. Fixed to18remand20remrespectively.@json-render/coreexpression set.Verification
npx vitest run cockpit/render/shared/render-examples-latent-bugs.spec.ts cockpit/render/shared/render-examples-display-coercion.spec.ts cockpit/render/shared/to-display-text.spec.tsnpx vitest run src/lib/render-examples-streaming-harness.spec.ts --config vite.config.mtsfromlibs/rendernpx nx test render --skip-nx-cachenpx nx affected -t build --configuration=production --skip-nx-cacheThe affected build completed for 46 projects plus dependency tasks. It emitted existing budget/CommonJS/unused-import warnings, but exited successfully.