fix(cli): Fix --no-tunnel option being ignored in fedify inbox command
#284
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @notJoon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a critical bug in the fedify inbox command where the --no-tunnel option was not functioning as intended, leading to the command always creating a public tunnel. The changes involve refining the command-line option parsing logic to correctly interpret the --no-tunnel flag, introducing dedicated types for better clarity, and adding new unit tests to validate the corrected behavior.
Highlights
- Fix for
--no-tunneloption: The core issue where the--no-tunneloption in thefedify inboxcommand was being ignored has been resolved. The logic for determining whether to disable tunneling now correctly respects the flag, preventing unintended public tunnels. - Improved Option Handling and Type Safety: An
InboxOptionsinterface has been introduced to explicitly define the command's options, enhancing type safety. Additionally, aTunnelConfigutility object with ashouldDisableTunnelhelper function centralizes and clarifies the tunneling decision logic. - New Test Coverage: A new test file,
inbox.test.ts, has been added. It includes comprehensive unit tests for theshouldDisableTunnellogic, covering various scenarios such as the--no-tunnelflag, its shorthand (-T), default behavior, and other option combinations, ensuring the fix is robust.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
github-actions
bot
added
component/cli
There was a problem hiding this comment.
Code Review
This pull request successfully fixes a bug where the --no-tunnel flag was being ignored. The approach of introducing a dedicated InboxOptions interface and a TunnelConfig object to handle the logic is sound, and the addition of tests is a great improvement. My review focuses on enhancing the new tests for better robustness and clarity. The most critical point is to ensure the tests validate the actual production code instead of a mock implementation. I've also provided a few suggestions to improve code structure and comments for better long-term maintainability.
cli/inbox.test.ts
Outdated
| function shouldDisableTunnel(options: InboxOptions): boolean { | ||
| return options.tunnel === false || options.noTunnel === true; | ||
| } |
There was a problem hiding this comment.
This test file re-implements the logic from inbox.tsx instead of testing the actual implementation. This is a critical issue because it can lead to the tests passing even if the production code is broken. Please replace this mocked function with the actual implementation from inbox.tsx. You'll need to add import { TunnelConfig } from './inbox.tsx'; at the top of the file.
| function shouldDisableTunnel(options: InboxOptions): boolean { | |
| return options.tunnel === false || options.noTunnel === true; | |
| } | |
| import { TunnelConfig } from './inbox.tsx'; | |
| const shouldDisableTunnel = TunnelConfig.shouldDisableTunnel; |
| const optionsWithT: InboxOptions = { | ||
| tunnel: true, | ||
| noTunnel: true, |
There was a problem hiding this comment.
The name of this test case and the mock data are a bit confusing. The flag definition '-T, --no-tunnel' suggests that -T is an alias for --no-tunnel, and both should result in options.tunnel being false. However, the test data here is { tunnel: true, noTunnel: true }, which is different from the test data for --no-tunnel ({ tunnel: false }). If cliffy indeed behaves this way, it would be great to add a comment explaining this behavior. Otherwise, if -T and --no-tunnel are supposed to be identical, the test data and name should be adjusted for clarity.
Deno.test("handles -T alias for --no-tunnel correctly", () => {
cli/inbox.test.ts
Outdated
| const result = shouldDisableTunnel(options); | ||
| assertEquals( | ||
| typeof result, | ||
| "boolean", | ||
| "shouldDisableTunnel must return boolean", | ||
| ); |
There was a problem hiding this comment.
This test only verifies that shouldDisableTunnel returns a boolean value, but it doesn't check if the returned value is correct for each combination of options. This test could be made much stronger by converting it into a data-driven test that asserts the expected outcome for each input case. This would make the test suite more robust.
Deno.test("shouldDisableTunnel handles various option combinations", () => {
const testCases: { name: string; options: InboxOptions; expected: boolean }[] = [
{ name: "should enable tunnel by default", options: { tunnel: true }, expected: false },
{ name: "should disable tunnel with --no-tunnel", options: { tunnel: false }, expected: true },
{ name: "should disable tunnel with -T", options: { tunnel: true, noTunnel: true }, expected: true },
{ name: "should disable tunnel if both flags are present", options: { tunnel: false, noTunnel: true }, expected: true },
];
for (const { name, options, expected } of testCases) {
assertEquals(shouldDisableTunnel(options), expected, name);
}
});| follow?: string[]; | ||
| acceptFollow?: string[]; | ||
| tunnel: boolean; | ||
| noTunnel?: boolean; // for -T shorthand support |
There was a problem hiding this comment.
This comment is helpful, but it hints at a potentially confusing behavior from the cliffy library where an alias (-T) for a negated flag (--no-tunnel) behaves differently. For future maintainability, it would be very beneficial to add a more detailed explanation here or in a comment above the shouldDisableTunnel function, clarifying why two separate properties (tunnel and noTunnel) are needed.
cli/inbox.tsx
Outdated
| shouldDisableTunnel: (opts: InboxOptions): boolean => { | ||
| return opts.tunnel === false || opts.noTunnel === true; | ||
| } |
There was a problem hiding this comment.
Using a TunnelConfig object to namespace a single function might be a slight over-complication. You could simplify this by exporting shouldDisableTunnel as a standalone function. This would also require updating the call site on line 86. If you plan to add more tunnel-related logic to TunnelConfig in the future, the current approach is fine.
/**
* Determines whether to disable the tunnel based on command-line options.
*/
export function shouldDisableTunnel(opts: InboxOptions): boolean {
return opts.tunnel === false || opts.noTunnel === true;
}
|
The docs for this pull request have been published: |
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
Looks good to me! Just left a review comment about the changelog.
|
The latest push to this pull request has been published to JSR and npm as a pre-release:
|
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
Oh, I missed that it's a bug fix! Could you change the target branch of this pull request to 1.7-maintenance instead of main?
2 participants
Description
fixes #243
This PR fixes the
--no-tunneloption in thefedify inboxcommand was being ignored, causing the server to always create a public tunnel regardless of the flag.Changes
!options.tunneltooptions.tunnel === false || options.noTunnel === trueInboxOptionsinterfaceinbox.test.tsfileProblem
The
fedify inboxcommand spin up an ephemeral ActivityPub server that can either be exposed to the public internet via a tunnel (default behavior) or run locally without tunneling when the--no-tunnel(or-T) flag is provided. However, as described in original issue, even when using the--no-tunnelflag, the server would still create a public HTTPS tunnel and be accessible from the outside.The problem was the option handling logic of
inbox.tsx. This was checking!options.tunnelto determine whether to disable tunneling.fedify/cli/inbox.tsx
Lines 69 to 71 in 62db879
However, due to how the
Cliffyframework handles negated boolean options, this logic was incorrect. When no flag is provided,options.tunnelisundefined, and!undefinedevaluates totrue, which incorrectly disabled tunneling by default. This meant the server would never create a tunnel unless the logic was inverted.After fix, We can see the
fedify inboxcreates local server as expected:With
--no-tunneloptionWithout
--no-tunneloption