search

saumya-banthia / tailscale-control

A Decky plugin to control Tailscale, while staying in Gaming mode.
BSD 3-Clause "New" or "Revised" License
53 stars 8 forks source link

Custom flags for tailscale up & Misc fix #12

Closed SaigyoujiYuyuko233 closed 7 months ago

SaigyoujiYuyuko233 commented 8 months ago

Relate to issue: #11

This PR will:

Here is how it looks like: image image

saumya-banthia commented 8 months ago

@SaigyoujiYuyuko233 thanks for the feature addon, I'm currently away from home, will review code as soon as I'm back, revert if something requires change and then merge.

Just an initial observation based query: Any reason to create a modal popup for the settings you've added (instead of how other settings currently are)?

SaigyoujiYuyuko233 commented 7 months ago

@SaigyoujiYuyuko233 thanks for the feature addon, I'm currently away from home, will review code as soon as I'm back, revert if something requires change and then merge.

Just an initial observation based query: Any reason to create a modal popup for the settings you've added (instead of how other settings currently are)?

For some unknow reason, in my deck (running nobara instead of steamos), the virtual input method does not popup when I add TextInput in quick access. The ip input in Decky Developer -> Remote React Developtools also has the same issue.

And I think popup is just wider and bigger, so it is easier for user to type and view after they finish editing.

SaigyoujiYuyuko233 commented 7 months ago

I kept the login server input because I think this entry is longer than other and it is easier to edit with a dedicate input box. update: image

saumya-banthia commented 7 months ago

Will take a look at this starting the next week, I do want to ensure a few things:

None of these are specific to any code you have written, as I haven't found time to review code so far. I just want to make sure it goes by the same general rules of the repo.

SaigyoujiYuyuko233 commented 7 months ago

Will take a look at this starting the next week, I do want to ensure a few things:

  • Adding new functionality, should not break previously working stuff (meaning people should be able to opt-in not opt-out of additional config setup) - this also prevents default config errors.
  • Another consideration is to not build a monolith, as I think a 3rd party plugin can/should go only so far, given how just one change can render previously usable things unusable.
  • Easily maintainable (more so, for this being a hobby project).

None of these are specific to any code you have written, as I haven't found time to review code so far. I just want to make sure it goes by the same general rules of the repo.

For the first rule, I can't really test that because v0.1.0-1 does not work for me since I'm using headscale & non-deck operator. But I think people can upgrade it without extra steps because I config default value for them. For the second & third, sure, keep it simple and stupid

saumya-banthia commented 7 months ago

For the first rule, I can't really test that because v0.1.0-1 does not work for me since I'm using headscale & non-deck operator.

I tested the code with a non-headscale setup. Needed a few changes, will be pushing it to a temp PR branch, test it with your setup and report back. I'll update this comment with the PR branch link, once it is pushed.

saumya-banthia commented 7 months ago

Pushed https://github.com/saumya-banthia/tailscale-control/tree/pr12

SaigyoujiYuyuko233 commented 7 months ago

Pushed https://github.com/saumya-banthia/tailscale-control/tree/pr12

Should I mark this pr as resolved for you and you can create a new one?

saumya-banthia commented 7 months ago

Pushed https://github.com/saumya-banthia/tailscale-control/tree/pr12

Should I mark this pr as resolved for you and you can create a new one?

Does the pr12 branch work for you (I can only confirm the non-headscale version to be working)?

SaigyoujiYuyuko233 commented 7 months ago

Pushed https://github.com/saumya-banthia/tailscale-control/tree/pr12

Should I mark this pr as resolved for you and you can create a new one?

Does the pr12 branch work for you (I can only confirm the non-headscale version to be working)?

Not sure if this is a special case, I can't type into the input box.

This work around works for me:

  1. Delete https://github.com/saumya-banthia/tailscale-control/blob/25710dbef7af54a3b5567da07cdf4fd0b1d92f6a/src/index.tsx#L228
  2. Put the following lines into Popup function (does not match the picture below, but you get it) 
    const [login_server, set_login_server] = useState<string>(tailscaleLoginServer);
const [custom_flags, set_custom_flags] = useState<string>(tailscaleUpCustomFlags);
  3. Replace onChange field from TextFields
  4. Replace value field from TextFields

BTW, moving L228(same as above) into Popup function and Replace value field from TextFields will not work.

It will look like this: image

Edit: I guess all the input option inside a modal popup should have its local variable inside the function, because setting exit node ip is not working either.
Another hypothesis is that the issue is cause by tailscale restart operation. image After tailscale goes down, it will not turn back on because the device list get wiped which cause by the error above. Just an inital observation, might not be the cause

saumya-banthia commented 7 months ago

I'll look into this, seems a recent DFL or Decky update might have caused it. On a side note: I faced a personal loss a few days back, so might take a bit longer to review things, expect delay in response.

saumya-banthia commented 7 months ago

@SaigyoujiYuyuko233: Pushed few more changes to tree#pr12, check to see if this works with your setup.

@ticky: pushed a fix to https://github.com/saumya-banthia/tailscale-control/issues/14

SaigyoujiYuyuko233 commented 7 months ago

@SaigyoujiYuyuko233: Pushed few more changes to tree#pr12, check to see if this works with your setup.

@ticky: pushed a fix to #14

Works beautifully on my deck! Thanks for the nice work!