Skip to content
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

Links in Expanded "Learn More" Section Not Functional #13

Open
ninjaeon opened this issue Nov 17, 2024 · 7 comments
Open

Links in Expanded "Learn More" Section Not Functional #13

ninjaeon opened this issue Nov 17, 2024 · 7 comments

Comments

@ninjaeon
Copy link

Description:

Clicking on links within the expanded "Learn More" section of a response does not open the linked content.

Steps to Reproduce:

  1. Open the Gemini Desktop app.
  2. Ask a question that generates a response with a "Learn More" section.
  3. Click the caret (^) to expand the "Learn More" section.
  4. Click on any link within the expanded content.

Expected Result:

The link should open in the user's default web browser.

Actual Result:

Nothing happens when the link is clicked.

@rajmondx
Copy link

I never noticed that. Thank you.

Current workaround could be to copy the message and extract the URL from the clipboard.

image

@ninjaeon
Copy link
Author

ninjaeon commented Dec 4, 2024

I used Windsurf + GPT 4o to fix this issue in a fork. I'm not a programmer so I can't review the code if it's good, but it fixed all the links to open in my system's default browser when clicked. Here's the commit that did it: ninjaeon@da2576a

@ninjaeon
Copy link
Author

ninjaeon commented Dec 6, 2024

I've created a fork of the project that implements two potentially useful features:

  • Open links in external browser: All links clicked within the application will now open in the user's default web browser.
  • Enable context menus: Right-click context menus are now enabled within the application.

Release:

You can find pre-built binaries for Windows, Linux, and macOS on the release page of my fork: https://github.com/ninjaeon/gemini-desktop-fork

Important Notes:

  • Untested builds: The Linux and macOS builds are untested. Feedback on their functionality is greatly appreciated.
  • Experimental code: I am not a programmer and developed this using Windsurf, GPT-4o, and Claude Sonnet 3.5 through trial and error. The code has not been formally reviewed and may contain bugs or inefficiencies.
  • Limited support: While I'll do my best to address any issues, my ability to troubleshoot may be limited.

Contribution:

If these features are deemed valuable to the main project, I would be happy to submit a pull request for consideration. Any feedback or suggestions for improvement are welcome.

@rajmondx
Copy link

rajmondx commented Dec 7, 2024

The code doesn’t work as intended, but thanks for trying! Seriously, it's an impressive effort for an AI. However, after reviewing the differences main...ninjaeon:gemini-desktop-fork:main , it’s clear that the implementation doesn’t perform a real update—it simply redirects the user to an incorrect GitHub page.

Let me provide an example, focusing specifically on the auto-update feature. I’ll regard everything else as "out of scope". When I looked into the changes, I noticed several questionable decisions, like the following:


Example 1

// Intercept window.open calls
window.open = (url) => {
    if (url && url.startsWith('http')) {
        shell.openExternal(url).catch(console.error);
    }
    return null;
};

// Handle new window events
window.addEventListener('new-window', (e) => {
    e.preventDefault();
    if (e.url && e.url.startsWith('http')) {
        shell.openExternal(e.url).catch(console.error);
    }
});
  1. Why are event listeners being added for click and new-window? To allow clicking on the urls so you get redirected to the update page? You are overriding the internal routing with this because not every href which starts with http is meant to go outside.

  2. Overriding global functions like window.open is bad practice. These functions aren’t yours to replace.

  3. The event listeners are contradicting each other, e.g. the event listener for 'click' would handle all archor elements with href starting with 'http' but the event listener 'new-window' is meant to do that? Same with window.open?

  4. Why does window.open return null? Afaik only IE10 does respectively did (12y ago) that.


Another issue is the extensive logging introduced in the code. The original implementation barely included logs, likely because there’s no logger, and regular users wouldn’t have access to them. These additional logs are mostly useless in the current state.

Moreover, I couldn’t find evidence of a proper auto-update implementation. Based on my review, it seems to rely on events like update-available and update-downloaded, but these events are never triggered. Instead, the current implementation redirects users to a GitHub page, which is not a real update process.

Even if auto-update were functional, the error-handling logic for the update-not-available event would spam error messages unnecessarily. This is poor design.

Finally, the most frustrating aspect is the apparent attempt to claim/override authorship of the project. This is completely unacceptable and undermines your credibility.

@ninjaeon
Copy link
Author

ninjaeon commented Dec 9, 2024

The code doesn’t work as intended, but thanks for trying! Seriously, it's an impressive effort for an AI. However, after reviewing the differences main...ninjaeon:gemini-desktop-fork:main , it’s clear that the implementation doesn’t perform a real update—it simply redirects the user to an incorrect GitHub page.

Let me provide an example, focusing specifically on the auto-update feature. I’ll regard everything else as "out of scope". When I looked into the changes, I noticed several questionable decisions, like the following:

Example 1

// Intercept window.open calls
window.open = (url) => {
    if (url && url.startsWith('http')) {
        shell.openExternal(url).catch(console.error);
    }
    return null;
};

// Handle new window events
window.addEventListener('new-window', (e) => {
    e.preventDefault();
    if (e.url && e.url.startsWith('http')) {
        shell.openExternal(e.url).catch(console.error);
    }
});
  1. Why are event listeners being added for click and new-window? To allow clicking on the urls so you get redirected to the update page? You are overriding the internal routing with this because not every href which starts with http is meant to go outside.
  2. Overriding global functions like window.open is bad practice. These functions aren’t yours to replace.
  3. The event listeners are contradicting each other, e.g. the event listener for 'click' would handle all archor elements with href starting with 'http' but the event listener 'new-window' is meant to do that? Same with window.open?
  4. Why does window.open return null? Afaik only IE10 does respectively did (12y ago) that.

Another issue is the extensive logging introduced in the code. The original implementation barely included logs, likely because there’s no logger, and regular users wouldn’t have access to them. These additional logs are mostly useless in the current state.

Moreover, I couldn’t find evidence of a proper auto-update implementation. Based on my review, it seems to rely on events like update-available and update-downloaded, but these events are never triggered. Instead, the current implementation redirects users to a GitHub page, which is not a real update process.

Even if auto-update were functional, the error-handling logic for the update-not-available event would spam error messages unnecessarily. This is poor design.

Finally, the most frustrating aspect is the apparent attempt to claim/override authorship of the project. This is completely unacceptable and undermines your credibility.

Thank you for the feedback! I'm not a programmer and learning as I go, so this helps immensely!

The auto-update is setup to work with the fork repo, otherwise I would had no way of testing that it works (that I'm currently aware of yet). So far I've confirmed that it works on Windows since I've pushed version fork-3. I'm assuming that if there's a pull request (I've never done a pull request in my life, hence the "assumption"), the maintainer would edit the repo address from to fork's address back to the original repo's address.

The logging comes from Windsurf (via Sonnet 3.5 or GPT-4o) adding it while debugging. Prompting the AI to add a feature seldom works the first time around, so when I tell it that something is broken, it adds logging so I can copy and paste error messages from the builds. The error messages are shown in command prompt when you start the app from there. Then it debugs based on the error messages. Repeat this until the new feature is fixed and added successfully. The logging is sometimes automatically removed by the AI, and in other times its not unless explicitly prompted to. I'm going prompt to try to remove all logging in the next version.

To make clear to anyone that thinks I'm trying to claim "authorship" of this, I'm removing my username anywhere in the code (with exception of references to the fork repo URL so the updater continues to work with the fork until if and when the additional features get merged with the parent repo). The AI chats say I'm supposed to put my name in this as a contributor for my modifications (anytime you see my username in the code, it was suggested by AI, especially when I was setting up Github Actions workflows for Linux builds), but it doesn't matter to me. I want to learn how to do this right, and I will defer to human criticism. The original project lists ISC license in the package.json so I'm trying to adhere to that (while trying to learn how to do that properly).

I personally use this app daily, so that was my primary motivation for the fork. Putting the code up is my attempt to learn how to Github and hopefully provide some value to others in the process. I appreciate the efforts of real programmers like nekupaw and yourself, as well as the rest of the open source community!

And for your specific criticisms of the code, I will be copying and pasting what you wrote as a prompt and see if Windsurf can act on it (I think it should).

THANK YOU!

ninjaeon added a commit to ninjaeon/gemini-desktop-fork that referenced this issue Dec 9, 2024
To avoid the perception that I was trying to claim/override authorship of the project nekupaw#13 (comment)
ninjaeon referenced this issue in ninjaeon/gemini-desktop-fork Dec 9, 2024
To make clear to anyone that thinks I'm trying to claim "ownership" of this, I've removed my username.  The AI chats say I'm supposed to put my name in this as a contributor for my modifications (anytime you see my username in the code, it was suggested by AI), but it doesn't matter to me.  I just want to learn how to do this right, and I will defer to human criticism.  The original project lists ISC license in the package.json so I'm trying to adhere to that.
@ninjaeon
Copy link
Author

ninjaeon commented Dec 9, 2024

Moreover, I couldn’t find evidence of a proper auto-update implementation. Based on my review, it seems to rely on events like update-available and update-downloaded, but these events are never triggered. Instead, the current implementation redirects users to a GitHub page, which is not a real update process.

Since the electron-updater implementation worked for me, I didn't question the code. But on further review (based on my limited comprehension), it seems to be adhering to https://www.electron.build/auto-update

ninjaeon added a commit to ninjaeon/gemini-desktop-fork that referenced this issue Dec 9, 2024
To avoid the perception of "apparent attempt to claim/override authorship of the project" nekupaw#13 (comment)
@ninjaeon
Copy link
Author

ninjaeon commented Dec 9, 2024

// Intercept window.open calls
window.open = (url) => {
    if (url && url.startsWith('http')) {
        shell.openExternal(url).catch(console.error);
    }
    return null;
};

// Handle new window events
window.addEventListener('new-window', (e) => {
    e.preventDefault();
    if (e.url && e.url.startsWith('http')) {
        shell.openExternal(e.url).catch(console.error);
    }
});

Made changes to this when I removed console error logging: ninjaeon@18d4a00#diff-032e180739c4103bc3c3e71de4e372ef41482cfedaf727481a9f0724a5763f5bR20

That said, when I put your full comment on this code in the prompt, the AI agrees with you, then changes the code into something that completely breaks links from opening at all. I went around and around with it for a while and had to give up - it's beyond my (prompting) abilities for now. I'll revisit this when Windsurf upgrades their models (hopefully Gemini 2.0 Flash soon?). I had similar problems when I first tried getting the external link to open originally...it took a lot of prompting and failed builds (read: hours of trial and error) to get something that worked.

I pushed a new release that removed the console error logging. Initially it broke the updater since it was relying on the console logging, but now it's updated and uses dialog boxes for errors. Since the updater code has changed, I am back to not being 100% sure the updater is working until the next release (it was before).

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

No branches or pull requests

2 participants