Add production FlatPolygon browser smoke coverage - #92
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughFlatPolygon model creation now preserves the layer shader assembler and avoids unnecessary initial recreation. A production consumer smoke harness renders canonical polygon fixtures, validates runtime behavior with Playwright, and runs in a dedicated CI job. ChangesFlatPolygon validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant Playwright
participant Vite
participant Consumer
participant Fixtures
participant Deck
CI->>Vite: Build and preview consumer
Playwright->>Vite: Open flat-polygon app
Consumer->>Fixtures: Fetch shapes metadata
Consumer->>Deck: Create and render flat-polygons layer
Deck-->>Consumer: Report frames or errors
Consumer-->>Playwright: Expose readiness and runtime state
Playwright-->>CI: Complete smoke-test assertions
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/production/flat-polygon/vite.config.ts (2)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
node_modulesnesting path is fragile to pnpm hoisting changes.
packages/layers/node_modules/@deck.gl/core/dist/index.jsassumes a specific pnpm dependency-resolution layout. If hoisting behavior or the pinned@deck.gl/coreversion changes, this path silently breaks the build with an opaque resolution error.♻️ Suggested resolution via require.resolve
-const deckCoreRoot = path.join( - workspaceRoot, - 'packages/layers/node_modules/@deck.gl/core/dist/index.js' -); +const deckCoreRoot = require.resolve('`@deck.gl/core`', { paths: [path.join(workspaceRoot, 'packages/layers')] });🤖 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 `@tests/production/flat-polygon/vite.config.ts` around lines 12 - 15, Replace the hardcoded `@deck.gl/core` node_modules path used by deckCoreRoot with module-resolution logic based on require.resolve, so the configuration locates the installed package entry point regardless of pnpm hoisting or dependency layout.
51-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlias replacements point at single files, not package roots — subpath imports would resolve incorrectly.
Vite/rollup-plugin-alias matches ids that start with
find + '/'too, but these replacements are exactdist/index.jsfile paths. A future@spatialdata/core/<subpath>import would resolve to<...>/index.js/<subpath>, which doesn't exist. Not currently triggered by any import in this diff, but worth a defensive note for future maintainers of this consumer harness.🤖 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 `@tests/production/flat-polygon/vite.config.ts` around lines 51 - 75, Update the alias replacements for the package-root aliases in the Vite configuration so subpath imports resolve beneath each package’s dist root rather than beneath a file path. Preserve the dedicated worker alias and existing external dependency aliases, and ensure the affected `@spatialdata` and zarrextra package aliases still resolve their main entry points.tests/production/flat-polygon/smoke.spec.ts (1)
20-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOrdering-dependent 404/console-error correlation risks flaky failures.
Gating
allowedMetadataConsoleErrorsagainstexpectedMetadata404Countat event-arrival time assumes theresponseevent for a given 404 always arrives before its corresponding console error. That ordering isn't guaranteed across CDP event delivery, so a console error legitimately caused by an expected 404 could still fail the test if it's processed first.♻️ Suggested fix: compare aggregate counts after collection instead of gating live
page.on('console', (message) => { if (message.type() !== 'error') return; - if ( - expectedMetadataConsoleError.test(message.text()) && - allowedMetadataConsoleErrors < expectedMetadata404Count - ) { - allowedMetadataConsoleErrors += 1; - return; - } - consoleErrors.push(message.text()); + if (expectedMetadataConsoleError.test(message.text())) { + allowedMetadataConsoleErrors += 1; + return; + } + consoleErrors.push(message.text()); });And after the page interactions complete:
+ expect(allowedMetadataConsoleErrors).toBeLessThanOrEqual(expectedMetadata404Count);🤖 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 `@tests/production/flat-polygon/smoke.spec.ts` around lines 20 - 38, Update the response and console event handlers in the smoke test to collect expected 404 and matching console-error counts independently, without gating allowedMetadataConsoleErrors against expectedMetadata404Count during event arrival. After page interactions complete, compare aggregate counts and exclude only the number of expected metadata console errors that can be matched to expected 404 responses, while preserving unexpected 404 URL and remaining console-error reporting.test-results/.last-run.json (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove committed Playwright artifact; deferred to consolidated comment.
See the consolidated comment covering this file andtest-fixtures.🤖 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-results/.last-run.json` around lines 1 - 4, Remove the committed Playwright artifact represented by .last-run.json from version control, ensuring generated test-result files are not included in the change.packages/layers/src/FlatPolygonLayer.ts (1)
107-114: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd coverage for the model recreation branch or track the initial creation more robustly.
This code prevents destroying the model on the first
extensionsChangedupdate from Deck, butpackages/layers/tests/flatPolygonLayer.spec.tsonly tests_getModel()passes Deck’sshaderAssembler; it has no cases for the first post-initializeStateupdate or a later realextensionschange.🤖 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 `@packages/layers/src/FlatPolygonLayer.ts` around lines 107 - 114, Add tests in flatPolygonLayer.spec.ts covering updateState after initializeState with extensionsChanged and no oldProps.extensions, asserting the existing model is retained, plus a later genuine extensions change with oldProps.extensions defined, asserting the model is destroyed and recreated. Reuse the existing _getModel/shaderAssembler test setup and target the updateState model-recreation branch.
🤖 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 @.github/workflows/test.yml:
- Line 166: Update the actions/checkout step in this job to set
persist-credentials to false, matching the existing react-lint and biome-lint
checkout steps while preserving the current checkout behavior.
- Around line 185-194: Update the “Cache generated test fixtures” step in the
fixture-generation job to use a distinct cache key and path from the full
fixture cache used by the test job. Ensure the corresponding “Generate test
fixtures (Python spatialdata 0.7.2)” step uses that isolated partial-fixture
location and cache-hit output, preventing the 0.7.2-only cache from being
restored as the complete fixture set.
In `@test-fixtures`:
- Line 1: Remove the tracked test-fixtures symlink containing the local absolute
path and remove the generated Playwright state file at
test-results/.last-run.json. Update .gitignore to ignore both test-fixtures/ and
test-results/, ensuring generated fixture and test-result directories can be
recreated by tests and CI without being tracked.
---
Nitpick comments:
In `@packages/layers/src/FlatPolygonLayer.ts`:
- Around line 107-114: Add tests in flatPolygonLayer.spec.ts covering
updateState after initializeState with extensionsChanged and no
oldProps.extensions, asserting the existing model is retained, plus a later
genuine extensions change with oldProps.extensions defined, asserting the model
is destroyed and recreated. Reuse the existing _getModel/shaderAssembler test
setup and target the updateState model-recreation branch.
In `@test-results/.last-run.json`:
- Around line 1-4: Remove the committed Playwright artifact represented by
.last-run.json from version control, ensuring generated test-result files are
not included in the change.
In `@tests/production/flat-polygon/smoke.spec.ts`:
- Around line 20-38: Update the response and console event handlers in the smoke
test to collect expected 404 and matching console-error counts independently,
without gating allowedMetadataConsoleErrors against expectedMetadata404Count
during event arrival. After page interactions complete, compare aggregate counts
and exclude only the number of expected metadata console errors that can be
matched to expected 404 responses, while preserving unexpected 404 URL and
remaining console-error reporting.
In `@tests/production/flat-polygon/vite.config.ts`:
- Around line 12-15: Replace the hardcoded `@deck.gl/core` node_modules path used
by deckCoreRoot with module-resolution logic based on require.resolve, so the
configuration locates the installed package entry point regardless of pnpm
hoisting or dependency layout.
- Around line 51-75: Update the alias replacements for the package-root aliases
in the Vite configuration so subpath imports resolve beneath each package’s dist
root rather than beneath a file path. Preserve the dedicated worker alias and
existing external dependency aliases, and ensure the affected `@spatialdata` and
zarrextra package aliases still resolve their main entry points.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e92f197-e3e2-416b-a533-d1d34eac7620
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltest-results/smoke-the-built-consumer-r-e303a-shapes-without-WebGL-errors/flat-polygon.pngis excluded by!**/*.png
📒 Files selected for processing (11)
.github/workflows/test.ymlpackage.jsonpackages/layers/src/FlatPolygonLayer.tspackages/layers/tests/flatPolygonLayer.spec.tstest-fixturestest-results/.last-run.jsontests/production/flat-polygon/index.htmltests/production/flat-polygon/playwright.config.tstests/production/flat-polygon/smoke.spec.tstests/production/flat-polygon/src.tsxtests/production/flat-polygon/vite.config.ts
2d677c9 to
d2f9e0b
Compare
`pnpm test:fixtures:generate:0.7.2 -- --output-dir X` forwards the `--` separator through to the Python script verbatim, and argparse treats every argument after a bare `--` as positional. The parser has no positionals, so it exits 2: generate_fixtures.py: error: unrecognized arguments: -- --output-dir X This was latent in #92 rather than new. The job's first CI run passed only because it shared a fixture cache key with the `test` job and got a cache hit, so the generation step was skipped entirely. Giving it a distinct key (the correct fix for that collision) forced a cache miss and ran the step for the first time, exposing the bad invocation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…orkflow lint gate (#93) * Fix invalid test.yml and add an independent workflow lint gate #92 left a duplicate `with:` key on the `react-lint` checkout step, which makes test.yml unparseable. GitHub then cannot evaluate the file's `on:` triggers, so no job runs and the failure surfaces only as a 0-second run with no jobs and no logs. Three changes: - Remove the duplicate key, restoring test.yml to a parseable state. - Add .github/workflows/workflow-lint.yml, running actionlint over .github/workflows. It is a separate file on purpose: a workflow cannot validate itself, so the check that would have caught this is precisely the one that could not run. An independent file still parses and reports a real file:line annotation. Verified against the broken commit: `test.yml:27:9: key "with" is duplicated in element of "steps" section`. - Give every step in test.yml a distinct `name:`. The root cause was four byte-identical `- uses: actions/checkout@v5` anchors: a correct, correctly anchored review suggestion for the new production-browser job was also applied to react-lint, which already had the setting. Unique names remove the ambiguity rather than just detecting its consequences. Also switches `for i in {1..30}` to `for _` to clear the one pre-existing shellcheck finding, so the new gate starts green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fix fixture generation in the production-browser job `pnpm test:fixtures:generate:0.7.2 -- --output-dir X` forwards the `--` separator through to the Python script verbatim, and argparse treats every argument after a bare `--` as positional. The parser has no positionals, so it exits 2: generate_fixtures.py: error: unrecognized arguments: -- --output-dir X This was latent in #92 rather than new. The job's first CI run passed only because it shared a fixture cache key with the `test` job and got a cache hit, so the generation step was skipped entirely. Giving it a distinct key (the correct fix for that collision) forced a cache miss and ran the step for the first time, exposing the bad invocation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
FlatPolygonLayermodel initialization and preserve Deck’s shader assembler hooks.Testing
Summary by CodeRabbit
Bug Fixes
Tests