Resolve issues with structutal crashes in Bevel, add colinear metadata to generators, and ensure boolean identity tradeoffs #2068#3943
Conversation
…ents to GitHub environments Restore the CI "View deployment" button in PRs after building Consolidate release.yml functionality into build.yml Move build from ci.yml to a delegated run in build.yml
…tection - Fixes missing colinear metadata in generators by parsing geometry actively. - Adds post-processing for boolean outputs. - Defines current limitation: Identity is not preserved through boolean ops. - Removed index-based iteration in Bevel node to prevent out-of-bounds panics without removing Bevel's segment modifying identities.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the stability and correctness of vector-based operations within the Graphite editor. It resolves crashes, improves the handling of colinear points, and addresses issues with pointer lock functionality in the NumberInput component, ultimately leading to a more robust and predictable user experience. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request includes several changes related to vector graphics and number input components. The NumberInput.svelte component now imports types from @graphite/messages instead of @graphite/../wasm/pkg/graphite_wasm. Additionally, the code now clamps large movementX values during drag start in the NumberInput component to address issues with inaccurate pointer movement data in Chrome and Firefox. The isInteger check was removed from the combineSnapped calculation. A detect_colinear_manipulators function was added to the Vector struct, and it is called in the boolean_operation_on_vector_table function. A set_ellipse_colinear_manipulators function was added, and the colinear_manipulators field is cleared in the spline function. The bevel algorithm was updated to improve segment sorting and handle colinear manipulators.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Confidence score: 3/5
- There is a concrete security-risk regression in
.github/workflows/ci.yml: switching tosecrets: inheritbroadens secret exposure compared with the prior explicit Cloudflare-only injection, which weakens least-privilege controls. .github/workflows/build.ymlmay fail deploys onworkflow_dispatch/workflow_callbecausewrangler pages deployalways consumescf_project, while that output appears to be set only for push runs; confidence is low here, so this is a moderate uncertainty rather than a confirmed blocker.- This looks mergeable with caution, but the combination of a medium-high security concern and CI parity drift in
.github/workflows/ci.yml(merge-queue skip gating no longer equivalent) creates non-trivial regression risk. - Pay close attention to
.github/workflows/ci.yml,.github/workflows/build.yml- secret scope and conditional project-name wiring can change CI/deploy behavior and security posture.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/build.yml">
<violation number="1" location=".github/workflows/build.yml:129">
P2: `cf_project` is only set on push runs, but `wrangler pages deploy` always uses it. On workflow_dispatch/workflow_call runs the output is empty, so deploy will receive an empty `--project-name` and fail. Either set a default for non-push runs or guard the deploy step.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:13">
P2: Reusable `build.yml` call dropped previous merge-queue skip gating for the web build path, so CI behavior is not parity-equivalent after migration.</violation>
<violation number="2" location=".github/workflows/ci.yml:14">
P1: `secrets: inherit` unnecessarily broadens secret scope for this reusable-workflow call, regressing least-privilege versus the prior explicit Cloudflare-only secret injection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| cd frontend | ||
| npm run check | ||
| uses: ./.github/workflows/build.yml | ||
| secrets: inherit |
There was a problem hiding this comment.
P1: secrets: inherit unnecessarily broadens secret scope for this reusable-workflow call, regressing least-privilege versus the prior explicit Cloudflare-only secret injection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 14:
<comment>`secrets: inherit` unnecessarily broadens secret scope for this reusable-workflow call, regressing least-privilege versus the prior explicit Cloudflare-only secret injection.</comment>
<file context>
@@ -8,78 +8,12 @@ env:
- cd frontend
- npm run check
+ uses: ./.github/workflows/build.yml
+ secrets: inherit
+ with:
+ web: true
</file context>
| for ATTEMPT in $(seq 1 $MAX_ATTEMPTS); do | ||
| echo "Attempt $ATTEMPT of $MAX_ATTEMPTS..." | ||
| if OUTPUT=$(npx wrangler@3 pages deploy "frontend/dist" --project-name="graphite-dev" --commit-dirty=true 2>&1); then | ||
| if OUTPUT=$(npx wrangler@3 pages deploy "frontend/dist" --project-name="${{ steps.production-env.outputs.cf_project }}" --commit-dirty=true 2>&1); then |
There was a problem hiding this comment.
P2: cf_project is only set on push runs, but wrangler pages deploy always uses it. On workflow_dispatch/workflow_call runs the output is empty, so deploy will receive an empty --project-name and fail. Either set a default for non-push runs or guard the deploy step.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/build.yml, line 129:
<comment>`cf_project` is only set on push runs, but `wrangler pages deploy` always uses it. On workflow_dispatch/workflow_call runs the output is empty, so deploy will receive an empty `--project-name` and fail. Either set a default for non-push runs or guard the deploy step.</comment>
<file context>
@@ -105,11 +118,15 @@ jobs:
for ATTEMPT in $(seq 1 $MAX_ATTEMPTS); do
echo "Attempt $ATTEMPT of $MAX_ATTEMPTS..."
- if OUTPUT=$(npx wrangler@3 pages deploy "frontend/dist" --project-name="graphite-dev" --commit-dirty=true 2>&1); then
+ if OUTPUT=$(npx wrangler@3 pages deploy "frontend/dist" --project-name="${{ steps.production-env.outputs.cf_project }}" --commit-dirty=true 2>&1); then
URL=$(echo "$OUTPUT" | grep -oP 'https://[^\s]+\.pages\.dev' | head -1)
echo "url=$URL" >> "$GITHUB_OUTPUT"
</file context>
| run: | | ||
| cd frontend | ||
| npm run check | ||
| uses: ./.github/workflows/build.yml |
There was a problem hiding this comment.
P2: Reusable build.yml call dropped previous merge-queue skip gating for the web build path, so CI behavior is not parity-equivalent after migration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 13:
<comment>Reusable `build.yml` call dropped previous merge-queue skip gating for the web build path, so CI behavior is not parity-equivalent after migration.</comment>
<file context>
@@ -8,78 +8,12 @@ env:
- run: |
- cd frontend
- npm run check
+ uses: ./.github/workflows/build.yml
+ secrets: inherit
+ with:
</file context>
2 participants
This fixes several issues with structural instabilities in regards to maintaining vector identities and incorrect assumptions in indices throughout the node stack. The changes resolve fatal crashes in the shape manipulation pipeline and bring our colinear handling into line with disjoint and implicitly merged paths.