hubs icon indicating copy to clipboard operation
hubs copied to clipboard

Fix link validation clean

Open HussainAther opened this issue 11 months ago • 8 comments

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.

HussainAther avatar Apr 23 '25 19:04 HussainAther

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

DougReeder avatar May 02 '25 19:05 DougReeder

Yes, thanks.do you want to correct it?

Syed Hussain Ather (he/him) https://www.gofundme.com/manage/join-us-in-advancing-breast-cancer-imaging-technology https://github.com/hussainather https://scholar.google.com/citations?user=jPL2-dEAAAAJ&hl=en https://www.linkedin.com/in/syed-hussain-ather-049919137/ https://twitter.com/shussainather http://hussainather.com

On Fri, May 2, 2025 at 3:57 PM Doug Reeder @.***> wrote:

DougReeder left a comment (Hubs-Foundation/hubs#6539) https://github.com/Hubs-Foundation/hubs/pull/6539#issuecomment-2848001796

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

— Reply to this email directly, view it on GitHub https://github.com/Hubs-Foundation/hubs/pull/6539#issuecomment-2848001796, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB44CMPCMXHUN5F4B7IXBYD24PEZ3AVCNFSM6AAAAAB3XFKAESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNBYGAYDCNZZGY . You are receiving this because you authored the thread.Message ID: @.***>

HussainAther avatar May 02 '25 20:05 HussainAther

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

DougReeder avatar May 02 '25 20:05 DougReeder

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

HussainAther avatar May 02 '25 22:05 HussainAther

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?

DougReeder avatar May 03 '25 16:05 DougReeder

Ty.

yes, the condition at line 597 is a safeguard for edge cases where the ext object may contain a malformed or misused link property during GLTF model loading. It's relevant when:

  • A custom GLTF extension includes a "link" field for remote assets
  • A user (or external tool) injects non-standard metadata into the model's extensions block

To reproduce:

  1. Load a GLTF file with a manually added "extensions" block like this:
"extensions": {
"CUSTOM_extension": {
"link": "http://invalid-url.com/model.glb"
}
}
  1. That will hit the condition if your loader surfaces the ext object through parser.json.extensions[...].

I agree it's not easy to trigger under normal usage—but it adds resilience in edge cases where malformed GLTF metadata could otherwise crash or fail silently.

Let me know if you’d prefer I refactor or relocate that check!

— SHA

Syed Hussain Ather (he/him) https://www.gofundme.com/manage/join-us-in-advancing-breast-cancer-imaging-technology https://github.com/hussainather https://scholar.google.com/citations?user=jPL2-dEAAAAJ&hl=en https://www.linkedin.com/in/syed-hussain-ather-049919137/ https://twitter.com/shussainather http://hussainather.com

On Sat, May 3, 2025 at 12:20 PM Doug Reeder @.***> wrote:

DougReeder left a comment (Hubs-Foundation/hubs#6539) https://github.com/Hubs-Foundation/hubs/pull/6539#issuecomment-2848694746

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?

— Reply to this email directly, view it on GitHub https://github.com/Hubs-Foundation/hubs/pull/6539#issuecomment-2848694746, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB44CMNBXCI7NMGK5UYBM3D24TUDBAVCNFSM6AAAAAB3XFKAESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNBYGY4TINZUGY . You are receiving this because you authored the thread.Message ID: @.***>

HussainAther avatar May 03 '25 16:05 HussainAther

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.

DougReeder avatar May 03 '25 16:05 DougReeder

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.

HussainAther avatar May 04 '25 00:05 HussainAther