Skip to content

Fix link validation clean #6539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: addons
Choose a base branch
from

Conversation

HussainAther
Copy link

What?

This pull request improves link validation and debugging within gltf-model-plus.js. Specifically, it introduces safeguards to handle invalid or malformed URLs more gracefully, preventing potential runtime errors and enhancing overall robustness when loading GLTF models. Additionally, improved debug logging has been added to assist developers in identifying link-related issues more efficiently.

Why?

The current implementation of gltf-model-plus.js lacked sufficient validation for external links, which could lead to failures or unexpected behavior when loading models with incorrect URLs. This PR addresses those limitations by:

Adding proper validation checks before attempting to load resources.
Enhancing debugging outputs to make troubleshooting easier for developers. This helps improve stability, particularly when users or developers input incorrect model URLs.

Examples

Attempting to load a GLTF model with an invalid URL will now trigger a clear warning message in the console rather than causing silent failures or crashes.
Example log output:
"Warning: Invalid GLTF model URL detected - [invalid_url]. Model loading skipped."

How to test

Launch the application and navigate to a scene where gltf-model-plus.js is utilized.
Assign a valid GLTF model URL and verify that it loads correctly.
Assign an invalid or malformed URL (e.g., http://invalid-url/model.gltf).
Check the browser console for appropriate warning/debug messages.
Confirm that no crashes or unexpected behavior occur when invalid URLs are provided.

Documentation of functionality

This pull request improves link validation and debugging within gltf-model-plus.js. Specifically, it introduces safeguards to handle invalid or malformed URLs more gracefully, preventing potential runtime errors and enhancing overall robustness when loading GLTF models. Additionally, improved debug logging has been added to assist developers in identifying link-related issues more efficiently.

Limitations

The current implementation of gltf-model-plus.js lacked sufficient validation for external links, which could lead to failures or unexpected behavior when loading models with incorrect URLs. This PR addresses those limitations by:

Adding proper validation checks before attempting to load resources.
Enhancing debugging outputs to make troubleshooting easier for developers. This helps improve stability, particularly when users or developers input incorrect model URLs.

Alternatives considered

Attempting to load a GLTF model with an invalid URL will now trigger a clear warning message in the console rather than causing silent failures or crashes.
Example log output:
"Warning: Invalid GLTF model URL detected - [invalid_url]. Model loading skipped."

Open questions

Should additional user-facing feedback (beyond console logs) be implemented for invalid model URLs?
Would integrating a centralized logging utility across similar modules be beneficial for consistency?

Additional details or related context

This update aligns with ongoing efforts to improve the reliability of asset loading within the Hubs ecosystem. It complements recent work on add-ons and model handling. Feedback on extending these validation patterns to other modules is welcome.

@DougReeder DougReeder changed the base branch from master to addons April 24, 2025 14:13
@DougReeder DougReeder self-requested a review April 24, 2025 14:54
Copy link
Contributor

@DougReeder DougReeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As updated, this PR looks fine. Unfortunately, I was not able to immediately get the addons branch running, so I'll let someone who has approve this.

Separately, I don't have a link to a bad .glb file; it would be helpful to include a URL.

@DougReeder DougReeder requested a review from Exairnous April 24, 2025 22:28
@DougReeder
Copy link
Contributor

Did you mean to add the new commit to this Pull Request? It doesn't appear to be related.

@HussainAther
Copy link
Author

HussainAther commented May 2, 2025 via email

@DougReeder
Copy link
Contributor

You should probably make a new branch from the head of this branch, then reset to the previous commit.

@HussainAther HussainAther force-pushed the fix-link-validation-clean branch from e46f77b to 5a1ce81 Compare May 2, 2025 22:31
@HussainAther
Copy link
Author

Thanks @DougReeder

I’ve reset the branch to remove the unrelated commit and force-pushed the cleaned-up version. Let me know if anything else needs adjustment.

appreciate the guidance.
— Syed

@DougReeder
Copy link
Contributor

I'm able to run the code, but I have not been able to evoke a situation where Object.prototype.hasOwnProperty.call(ext, "link") at line 597 is truthy. Can you give more specific directions to reproduce the error?

@HussainAther
Copy link
Author

HussainAther commented May 3, 2025 via email

@DougReeder
Copy link
Contributor

I've tried to create a .gltf file that triggers that code branch, by following your directions, but I'm still not seeing that code branch triggered. I think you'll need to give a link to a .gltf file which demonstrated the behavior, before we can actually test this.

@HussainAther
Copy link
Author

Totally fair — this safeguard was added for future-proofing against malformed extensions, but I agree it's hard to trigger right now. Happy to revise or remove if it's blocking this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants