Skip to content

Update keepalive() to use new /api/{command} URL format#910

Closed
MelvinBot wants to merge 1 commit intomainfrom
claude-updateKeepaliveNewUrlFormat
Closed

Update keepalive() to use new /api/{command} URL format#910
MelvinBot wants to merge 1 commit intomainfrom
claude-updateKeepaliveNewUrlFormat

Conversation

@MelvinBot
Copy link
Copy Markdown

The keepalive() method in Network.jsx always used addCommandToUrl() to construct the legacy /api/?&command=X URL format, even when the endpoint is configured for the new /api/{command} format (trailing slash). This updates keepalive() to check isNewURLFormat and build the URL path correctly, mirroring the logic already present in the post() method. The command is also removed from the parameters object so it isn't sent in the FormData body.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/625277

Tests

// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.

QA

// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.

The keepalive() method in Network.jsx was always using addCommandToUrl()
which produces the legacy /api/?&command=X format, even when the endpoint
is configured for the new URL format. This mirrors the existing
isNewURLFormat handling already present in the post() method.

Co-authored-by: Nathan Metcalf <nathanmetcalf@users.noreply.github.com>
@MelvinBot MelvinBot requested a review from a team April 17, 2026 00:10
@github-actions
Copy link
Copy Markdown


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Comment thread lib/Network.jsx
Comment on lines +136 to +139
} else {
// Add the API command to our URL (for console debugging purposes)
url = addCommandToUrl(parameters.command, url);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NAB - we don't need to keep the old-style format at this point. We could remove this whole else block and the if (isNewURLFormat)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@MelvinBot Do the above please.

@francoisl
Copy link
Copy Markdown
Contributor

Testing now...

Comment thread lib/Network.jsx
Comment on lines +130 to +138
if (isNewURLFormat) {
// Remove command from parameters and use it in the URL
const command = parameters.command;
// eslint-disable-next-line no-param-reassign
delete parameters.command;
url = `${endpoint}${command}`;
} else {
// Add the API command to our URL (for console debugging purposes)
url = addCommandToUrl(parameters.command, url);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wait, that's the wrong thing to update. @MelvinBot we need to update the function addCommandToUrl instead. It should not use ${newUrl}&command=${command}, but instead of:

  • Ensure that the passed url ends with /, or append /
  • Return append the command after that, i.e. return something like ${url}/${command}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@MelvinBot Read the above and do the needful.

We also want to pass the CODE_REVISION as a new param called appVersion. We need to check if clients are using the newest revision. Do that too.

@nathanmetcalf
Copy link
Copy Markdown

Wait, that's the wrong thing to update. @MelvinBot we need to update the function addCommandToUrl instead. It should not use ${newUrl}&command=${command}, but instead of:

Ensure that the passed url ends with /, or append /
Return append the command after that, i.e. return something like ${url}/${command}

@francoisl
Copy link
Copy Markdown
Contributor

I don't think Melvin listened

@nathanmetcalf
Copy link
Copy Markdown

@MelvinBot

Wait, that's the wrong thing to update. @MelvinBot we need to update the function addCommandToUrl instead. It should not use ${newUrl}&command=${command}, but instead of:

Ensure that the passed url ends with /, or append /
Return append the command after that, i.e. return something like ${url}/${command}

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.

3 participants