[Chore]: Added url-checker, updated circular-deps, documented new static analysis .txt pattern #7442

nadr0 · 2025-06-10T20:23:29Z

Update

I moved the 404 links to this PR #7448

Updated the circular deps to use ./scripts/known/circular.txt and ./scripts/known/urls.txt
Updated the bash scripts for the new path
Updated the package.json for the new paths
Added npm run url-checker:diff to the static analysis CI CD job.
Updated url-checker.sh to not use tmp files on disk, everything is in memory so it is less messy
Added a scripts section to the contributing.md file to have others understand the pattern.

Issue

URLs in the application are stale and they 404, we don't have an automatic way to detect this

Implementation

Fixed the 404 urls
Implemented a bash script that has a known-urls.txt file. If we generate a diff, we can test against this and see what the problem is.
I am using two grep commands which can produce some funky results. I think this is reasonable since this did catch all the ones that I had to migrate in this PR, I used that list to fix them.
Anything link that is not 200 goes in that known-urls.txt file

vercel · 2025-06-10T20:23:33Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 1, 2025 5:27pm

jacebrowning · 2025-06-10T20:46:10Z

package.json

@@ -110,6 +110,9 @@
    "remove-importmeta": "sed -i 's/import.meta.url/window.location.origin/g' \"./rust/kcl-wasm-lib/pkg/kcl_wasm_lib.js\"; sed -i '' 's/import.meta.url/window.location.origin/g' \"./rust/kcl-wasm-lib/pkg/kcl_wasm_lib.js\" || echo \"sed for both mac and linux\"",
    "lint-fix": "eslint --fix --ext .ts --ext .tsx src e2e packages/codemirror-lsp-client/src rust/kcl-language-server/client/src",
    "lint": "eslint --max-warnings 0 --ext .ts --ext .tsx src e2e packages/codemirror-lsp-client/src rust/kcl-language-server/client/src",
+    "url-checker":"./scripts/url-checker.sh",
+    "url-checker:overwrite":"npm run url-checker | sed '$d' > known-urls.txt",
+    "url-checker:diff":"./scripts/diff-url-checker.sh",


Kind of a nitpick, so feel free to ignore, but most of these scripts are verbs so maybe check-urls: would be more consistent?

"url-checker":"./scripts/url-checker.sh", "url-checker:overwrite":"npm run url-checker | sed '$d' > known-urls.txt", "url-checker:diff":"./scripts/diff-url-checker.sh", "circular-deps": "dpdm --no-warning --no-tree -T --skip-dynamic-imports=circular src/index.tsx", "circular-deps:overwrite": "npm run circular-deps | sed '$d' | grep -v '^npm run' > known-circular.txt", "circular-deps:diff": "./scripts/diff-circular-deps.sh", "circular-deps:diff:nodejs": "npm run circular-deps:diff || node ./scripts/diff.js",

I followed the same pattern Jess implemented for circular deps. So the verbage is different

jacebrowning · 2025-06-10T20:49:27Z

known-urls.txt

+302  https://stackoverflow.com/a/58436959/22753272
+301  https://text-to-cad.zoo.dev/dashboard
+307  https://zoo.dev/
+308  https://zoo.dev/docs/api/ml/generate-a-cad-model-from-text


I'm not sure we need this as another top-level file. Perhaps something like scripts/output/ could work? There could also be a nested .gitignore in there to hide the temporary files. Open to suggestions.

I can move this, again I followed jess's pattern of known-circular.txt. This is the one file we want to use in the CI CD testing.

I'd wanna move all of these IMO so they follow the same pattern. This is the second script that we run and have a whitelist that needs to be updated otherwise a future test would fail.

I'm in favor of moving both then. I don't thinking adding another text file to the root each time we want to store "last run" state is a pattern we want to replicate.

If you want to ship these new scripts, I'd like to see them hooked up to CI as well. Otherwise, I worry that these will be forgotten about and the code/state will go stale over time. I'd hope we catch broken links in reviews going forward, even without any custom scripts -- not doing so implies a mere "looks good" review that didn't actually try out the code. 😄

It might be worthwhile to ship the 404 fixes in a separate PR so we can get those out independent of any potential tooling discussion.

I'll open a new PR for only the 404 links. I was going to make a CI CD job but I want it in and people to see it before I make it a CI CD enforced job.

I'm in favor of moving both then. I don't thinking adding another text file to the root each time we want to store "last run" state is a pattern we want to replicate.

I will move this into the scripts folder but I do think these types of files fall into a configuration style. It is a project wide state that can be read.

nadr0 · 2025-06-11T14:31:54Z

#7448

PR with 404 fixes only.

graphite-app · 2025-06-11T16:30:22Z

scripts/url-checker.sh

+while read line; do
+    # || true this curl request to bypass any failures and not have the scrip panic.
+    # the set -euo pipefail will cause a panic if a curl fails
+    status=$(curl -o /dev/null -s -w "%{http_code}\n" $line || true)


The $line variable should be quoted to prevent issues with URLs containing special characters or spaces:

status=$(curl -o /dev/null -s -w "%{http_code}\n" "$line" || true)

This ensures the entire URL is passed as a single argument to curl, avoiding potential command injection or unexpected behavior with special characters.

Suggested change

status=$(curl -o /dev/null -s -w "%{http_code}\n" $line || true)

status=$(curl -o /dev/null -s -w "%{http_code}\n" "$line" || true)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

nadr0 · 2025-06-11T17:08:18Z

@jacebrowning -- this ready for a rereview. I've moved everything into ./scripts. I've updated the contributing.md to outline this pattern as well.

I added a step in the CI CD for my known urls.

…dencies? do i need all of these?

CONTRIBUTING.md

jacebrowning

Thanks for consolidating the scripts directory and adding the URL checks to CI!

graphite-app · 2025-07-01T14:46:26Z

scripts/url-checker.sh

+val1=$(grep -Eoh "(https)://[^']+" src/**/*.ts  | remove_after_space | remove_after_backtick | remove_after_end_paren | remove_after_double_quote | remove_after_gt | remove_after_comma)
+
+# Search all src/**/*.tsx files
+val2=$(grep -Eoh "(https)://[^']+" src/**/*.tsx  | remove_after_space | remove_after_backtick | remove_after_end_paren | remove_after_double_quote | remove_after_gt | remove_after_comma)


The grep commands might exit with a non-zero status if no URLs are found in the source files, which would cause the script to fail due to set -euo pipefail. Consider adding || true to handle this case:

val1=$(grep -Eoh "(https)://[^']+" src/**/*.ts | remove_after_space | remove_after_backtick | remove_after_end_paren | remove_after_double_quote | remove_after_gt | remove_after_comma || true) # Search all src/**/*.tsx files val2=$(grep -Eoh "(https)://[^']+" src/**/*.tsx | remove_after_space | remove_after_backtick | remove_after_end_paren | remove_after_double_quote | remove_after_gt | remove_after_comma || true)

This ensures the script continues execution even when no URLs are found in either file type.

Suggested change

val1=$(grep -Eoh "(https)://[^']+" src/**/*.ts | remove_after_space | remove_after_backtick | remove_after_end_paren | remove_after_double_quote | remove_after_gt | remove_after_comma)

# Search all src/**/*.tsx files

val2=$(grep -Eoh "(https)://[^']+" src/**/*.tsx | remove_after_space | remove_after_backtick | remove_after_end_paren | remove_after_double_quote | remove_after_gt | remove_after_comma)

val1=$(grep -Eoh "(https)://[^']+" src/**/*.ts | remove_after_space | remove_after_backtick | remove_after_end_paren | remove_after_double_quote | remove_after_gt | remove_after_comma || true)

# Search all src/**/*.tsx files

val2=$(grep -Eoh "(https)://[^']+" src/**/*.tsx | remove_after_space | remove_after_backtick | remove_after_end_paren | remove_after_double_quote | remove_after_gt | remove_after_comma || true)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-07-01T14:46:27Z

scripts/known/urls.txt

+> [email protected] url-checker
+> ./scripts/url-checker.sh
+


The baseline file in scripts/known/urls.txt contains npm command output headers that won't be present in actual script output during comparison:

> [email protected] url-checker > ./scripts/url-checker.sh

These headers will cause the diff comparison to fail since they're artifacts of how the file was initially generated. Consider either:

Removing these lines from the baseline file, or

Modifying the url-checker:overwrite command to filter out these headers (similar to how circular-deps:overwrite uses sed and grep to clean its output)

This will ensure consistent comparisons between baseline and current output during CI checks.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

…ng-app into nadro/adhoc/url-checking

graphite-app · 2025-07-01T15:38:57Z

scripts/url-checker.sh

+while read line; do
+    # || true this curl request to bypass any failures and not have the scrip panic.
+    # the set -euo pipefail will cause a panic if a curl fails
+    status=$(curl -o /dev/null -s -w "%{http_code}\n" $line || true)


The curl command doesn't properly handle URLs containing template variables like ${BASE_URL} or ${version} which appear in the urls.txt file. These variables won't be interpolated in the shell context, resulting in failed requests or misleading status codes. Consider adding pattern detection to either skip these templated URLs or implement special handling for strings containing ${} patterns to prevent false positives in the URL checking process.

Suggested change

status=$(curl -o /dev/null -s -w "%{http_code}\n" $line || true)

if [[ "$line" =~ \$\{.*\} ]]; then

echo "Skipping templated URL: $line"

continue

fi

status=$(curl -o /dev/null -s -w "%{http_code}\n" $line || true)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-07-01T15:38:58Z

scripts/url-checker.sh

+        problematic+="$status\t$line\n"
+    fi
+done < <(echo "$uniqued")
+echo -e $problematic | column -t


The echo command should include double quotes around the $problematic variable to properly handle special characters and whitespace in the output:

echo -e "$problematic" | column -t

Without quotes, shell word splitting could break the formatting or cause unexpected behavior when the variable contains special characters.

Suggested change

echo -e $problematic | column -t

echo -e "$problematic" | column -t

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Co-authored-by: Jace Browning <[email protected]>

nadr0 added 3 commits June 10, 2025 14:29

fix: ignoring url checker files

dc37f44

fix: url checker

75b2d1b

fix: auto fmt and cleanup

f8d6799

nadr0 requested a review from a team June 10, 2025 20:28

nadr0 changed the title ~~Fixing 404 URLs, implemented known-url.txt for url checking.~~ Fixing 404 URLs, implemented known-urls.txt for url checking. Jun 10, 2025

vercel bot deployed to Preview June 10, 2025 20:45 View deployment

jacebrowning reviewed Jun 10, 2025

View reviewed changes

nadr0 mentioned this pull request Jun 11, 2025

[Fix]: All 404 links in the repo #7448

Merged

fix: moving the bash scripts and known files into the scripts repo

c53732f

graphite-app bot reviewed Jun 11, 2025

View reviewed changes

nadr0 added 6 commits June 11, 2025 11:37

fix: removed all url_results and made it be all in memory

0f2e7d1

fix: fixed the newline issue

4bfca11

fix: url checking as a step to the static analysis

6b4e304

fix: removed old code

37da00f

chore: writing documentation on our static checker pattern

d45e643

fix: updating the docs more to be clearer

2a8d954

fix: copy and paste without understanding requirements of ci cd depen…

b34cc89

…dencies? do i need all of these?

vercel bot deployed to Preview June 11, 2025 17:33 View deployment

nadr0 changed the title ~~Fixing 404 URLs, implemented known-urls.txt for url checking.~~ [Chore]: Added url-checker, updated circular-deps, documented new static analysis .txt pattern Jun 11, 2025

nadr0 requested a review from a team June 12, 2025 14:19

jacebrowning reviewed Jun 17, 2025

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

jacebrowning approved these changes Jun 17, 2025

View reviewed changes

Merge branch 'main' into nadro/adhoc/url-checking

291a181

graphite-app bot reviewed Jul 1, 2025

View reviewed changes

vercel bot deployed to Preview July 1, 2025 14:57 View deployment

fix: updating

ba18e27

Merge branch 'nadro/adhoc/url-checking' of github.com:KittyCAD/modeli…

fcc60b3

…ng-app into nadro/adhoc/url-checking

graphite-app bot reviewed Jul 1, 2025

View reviewed changes

vercel bot deployed to Preview July 1, 2025 15:51 View deployment

fix: I thought this got in?

6ee969e

jacebrowning assigned nadr0 Jul 1, 2025

vercel bot deployed to Preview July 1, 2025 17:02 View deployment

Update CONTRIBUTING.md

2ec593b

Co-authored-by: Jace Browning <[email protected]>

vercel bot deployed to Preview July 1, 2025 17:27 View deployment

nadr0 merged commit 7f9851a into main Jul 1, 2025
57 checks passed

nadr0 deleted the nadro/adhoc/url-checking branch July 1, 2025 18:01

	status=$(curl -o /dev/null -s -w "%{http_code}\n" $line \|\| true)
	status=$(curl -o /dev/null -s -w "%{http_code}\n" "$line" \|\| true)

-    status=$(curl -o /dev/null -s -w "%{http_code}\n" $line || true)
+    if [[ "$line" =~ \$\{.*\} ]]; then
+        echo "Skipping templated URL: $line"
+        continue
+    fi
+    status=$(curl -o /dev/null -s -w "%{http_code}\n" $line || true)

	echo -e $problematic \| column -t
	echo -e "$problematic" \| column -t

[Chore]: Added url-checker, updated circular-deps, documented new static analysis .txt pattern #7442

[Chore]: Added url-checker, updated circular-deps, documented new static analysis .txt pattern #7442

Uh oh!

Conversation

nadr0 commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update

Issue

Implementation

Uh oh!

vercel bot commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacebrowning Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

nadr0 Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

jacebrowning Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nadr0 Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

jacebrowning Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

nadr0 Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

nadr0 Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

nadr0 commented Jun 11, 2025

Uh oh!

graphite-app bot Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

nadr0 commented Jun 11, 2025

Uh oh!

Uh oh!

jacebrowning left a comment

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nadr0 commented Jun 10, 2025 •

edited

Loading

vercel bot commented Jun 10, 2025 •

edited

Loading

jacebrowning Jun 10, 2025 •

edited

Loading