Skip to content

refactor: update API base URL and enhance request handling #1014

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
+34 −20
Draft

refactor: update API base URL and enhance request handling #1014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/request-card/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ async function createRequestCardComponent({
}

const userImage = requestUser?.picture?.url ?? ICONS.DEFAULT_USER_AVATAR;
let userFirstName = requestUser?.first_name ?? data.assignee;
let userFirstName = requestUser?.first_name ?? data.assignee ?? data.requestedBy;
const userId = requestUser?.id;
const userStatus = userStatusMap?.get(userId);
const committedHours = userStatus?.monthlyHours?.committed;
Expand Down
5 changes: 3 additions & 2 deletions constants.js
View file
Open in desktop
Copy link
Author

Choose a reason for hiding this comment

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

wil change it again once done with the final testing before sending the PRs to review. thanks.

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const API_BASE_URL = window.API_BASE_URL || 'https://api.realdevsquad.com';
const REPO_SYNC_API_URL =
const API_BASE_URL = 'http://localhost:3000';
Copy link

Choose a reason for hiding this comment

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

Production API Endpoint Disabled category Readability

Tell me more
What is the issue?

Hardcoding the API base URL to localhost removes the ability to deploy to production environments and breaks the application's functionality in non-development environments.

Why this matters

The application will fail to make API calls in production as it will attempt to reach localhost instead of the actual production API endpoint.

Suggested change ∙ Feature Preview

Restore the environment-aware API base URL configuration:

const API_BASE_URL = window.API_BASE_URL || 'https://api.realdevsquad.com';
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

// const API_BASE_URL = window.API_BASE_URL || 'https://api.realdevsquad.com';
Comment on lines +1 to +2
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Hardcoded localhost URL will break production deployments.

The API base URL is now hardcoded to localhost, which will cause failures in production environments. The commented dynamic assignment suggests this might be temporary, but this approach creates deployment risks.

Consider using environment-specific configuration instead:

-const API_BASE_URL = 'http://localhost:3000';
-// const API_BASE_URL = window.API_BASE_URL || 'https://api.realdevsquad.com';
+const API_BASE_URL = window.API_BASE_URL || 'https://api.realdevsquad.com';

Alternatively, if localhost is needed for development, use a more robust environment detection mechanism.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const API_BASE_URL = 'http://localhost:3000';
// const API_BASE_URL = window.API_BASE_URL || 'https://api.realdevsquad.com';
const API_BASE_URL = window.API_BASE_URL || 'https://api.realdevsquad.com';
🤖 Prompt for AI Agents 
In constants.js at lines 1 to 2, the API_BASE_URL is hardcoded to
'http://localhost:3000', which will fail in production. Replace this with a
dynamic assignment that uses an environment variable or a global window property
to set the URL based on the environment. Implement a check to differentiate
between development and production environments and assign the API_BASE_URL
accordingly, ensuring it defaults to the correct production URL when not in
development.

const REPO_SYNC_API_URL =``
Copy link

Choose a reason for hiding this comment

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

The backtick string literal for REPO_SYNC_API_URL appears to be malformed. The line contains a template literal that opens with backticks but doesn't properly close before the URL string begins. This syntax error should be fixed by either:

  1. Using standard string quotes: const REPO_SYNC_API_URL = 'https://staging-sync.staging-realdevsquad-com.workers.dev';
  2. Or properly formatting the template literal if needed: const REPO_SYNC_API_URL = `https://staging-sync.staging-realdevsquad-com.workers.dev`;
Suggested change
const REPO_SYNC_API_URL =``
const REPO_SYNC_API_URL = 'https://staging-sync.staging-realdevsquad-com.workers.dev';

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Copy link

Choose a reason for hiding this comment

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

Unnecessary Template Literal category Performance

Tell me more
What is the issue?

Template literal syntax is misused with empty backticks, causing unnecessary string concatenation overhead.

Why this matters

While minor, this creates an unnecessary string concatenation operation that impacts performance and readability.

Suggested change ∙ Feature Preview

Remove the empty template literal and use a regular string:

const REPO_SYNC_API_URL = 'https://staging-sync.staging-realdevsquad-com.workers.dev';
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify impact of emptying REPO_SYNC_API_URL.

The REPO_SYNC_API_URL has been set to an empty template literal, effectively removing its value. This could break any functionality that depends on this URL.

🏁 Script executed:

#!/bin/bash
# Search for usage of REPO_SYNC_API_URL to assess impact
rg -A 3 "REPO_SYNC_API_URL" --type js

Length of output: 502

Restore a valid REPO_SYNC_API_URL or handle empty values

The constant REPO_SYNC_API_URL is currently set to an empty string, but it’s used in a fetch() call in script.js. This means requests will be made to the current origin (empty URL) and break any sync functionality.

• constants.js:3 – REPO_SYNC_API_URL is assigned `` (empty template literal)
• script.js: the fetch(REPO_SYNC_API_URL) call will now resolve to an invalid URL

Suggested fix (either restore the hard-coded URL or switch to an environment variable with a fallback):

- const REPO_SYNC_API_URL = ``
+ const REPO_SYNC_API_URL =
+   process.env.REPO_SYNC_API_URL ||
+   'https://staging-sync.staging-realdevsquad-com.workers.dev';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const REPO_SYNC_API_URL =``
const REPO_SYNC_API_URL =
process.env.REPO_SYNC_API_URL ||
'https://staging-sync.staging-realdevsquad-com.workers.dev';
🤖 Prompt for AI Agents 
In constants.js at line 3, the REPO_SYNC_API_URL is currently an empty string,
causing fetch calls in script.js to use an invalid URL. Fix this by assigning
REPO_SYNC_API_URL a valid URL string, either by restoring the original
hard-coded URL or by reading from an environment variable with a fallback
default URL to ensure fetch requests target the correct endpoint.

'https://staging-sync.staging-realdevsquad-com.workers.dev';
Comment on lines +3 to 4
Copy link

Choose a reason for hiding this comment

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

Unnecessary String Syntax category Readability

Tell me more
What is the issue?

Empty template literal (```) followed by a string literal creates confusing and unnecessary syntax

Why this matters

This syntax could be mistaken for a template literal implementation and makes the code harder to read without providing any benefit.

Suggested change ∙ Feature Preview

Use a simple string literal:

const REPO_SYNC_API_URL = 'https://staging-sync.staging-realdevsquad-com.workers.dev';
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

const USER_MANAGEMENT_LINK = 'user-management-link';
const EXTENSION_REQUESTS_LINK = 'extension-requests-link';
Expand Down
32 changes: 26 additions & 6 deletions requests/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const API_BASE_URL = window.API_BASE_URL;
const API_BASE_URL = 'http://localhost:3000';
Copy link

Choose a reason for hiding this comment

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

Hardcoded Local API URL category Functionality

Tell me more
What is the issue?

Hardcoding the API base URL to localhost removes the ability to deploy the application to different environments.

Why this matters

The application will only work in local development and will fail in production or other environments where the API is hosted at a different URL.

Suggested change ∙ Feature Preview

Restore the dynamic API URL configuration and use environment variables:

const API_BASE_URL = window.API_BASE_URL || 'http://localhost:3000';
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

// const API_BASE_URL = window.API_BASE_URL;
Copy link
Author

Choose a reason for hiding this comment

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

wil change it again once done with the final testing before sending the PRs to review. thanks.

Comment on lines +1 to +2
Copy link

Choose a reason for hiding this comment

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

Missing API URL switch context category Documentation

Tell me more
What is the issue?

The commented code lacks explanation of when to use window.API_BASE_URL vs hardcoded localhost URL.

Why this matters

Without understanding the context, other developers may accidentally commit the hardcoded localhost URL to production.

Suggested change ∙ Feature Preview

// Local development uses localhost, production uses window.API_BASE_URL
const API_BASE_URL = 'http://localhost:3000';
// const API_BASE_URL = window.API_BASE_URL;

Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +1 to +2
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Hardcoded localhost URL creates deployment risk.

Same issue as in constants.js - hardcoding the API base URL to localhost will break production deployments. This creates inconsistency since some files still use the dynamic assignment pattern.

Consider using the global constant from constants.js instead:

-const API_BASE_URL = 'http://localhost:3000';
-// const API_BASE_URL = window.API_BASE_URL;
+// Use the global constant from constants.js instead of redefining

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In requests/script.js at lines 1 to 2, the API base URL is hardcoded to
'http://localhost:3000', which risks breaking production deployments. Replace
this hardcoded string with an import or reference to the global API base URL
constant defined in constants.js to ensure consistent and
environment-appropriate configuration across the project.

const requestContainer = document.getElementById(REQUEST_CONTAINER_ID);
const lastElementContainer = document.querySelector(LAST_ELEMENT_CONTAINER);
const params = new URLSearchParams(window.location.search);
Expand Down Expand Up @@ -142,17 +143,31 @@ onboardingExtensionTabLink.addEventListener('click', async function (event) {
function updateUrlWithQuery(type) {
const url = new URL(window.location);
url.searchParams.set('type', type.toLowerCase());
if (isDev) {
url.searchParams.set('dev', 'true');
}
Comment on lines 143 to +148
Copy link

Choose a reason for hiding this comment

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

Inconsistent Dev Mode Persistence category Functionality

Tell me more
What is the issue?

The dev parameter is being added only when switching tabs, but not preserved when applying filters or performing other navigation actions.

Why this matters

Users will lose their development mode state when interacting with filters or other navigation features, leading to inconsistent behavior.

Suggested change ∙ Feature Preview

Move the dev parameter logic to a common function that's used across all URL updates:

function updateUrlParams(params) {
  const url = new URL(window.location);
  Object.entries(params).forEach(([key, value]) => {
    url.searchParams.set(key, value);
  });
  if (isDev) {
    url.searchParams.set('dev', 'true');
  }
  return url;
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.


window.history.pushState({ path: url.toString() }, '', url.toString());
}

async function getRequests(requestType, query = {}) {
let finalUrl =
API_BASE_URL +
(nextLink || `/requests${getQueryParamsString(requestType, query)}`);
(nextLink ||
`/requests${getQueryParamsString(
requestType,
query,
requestType === 'OOO' ? isDev : true,
)}`);
Comment on lines +156 to +161
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Proper conditional dev parameter usage.

The conditional logic for passing the dev parameter to getQueryParamsString is correctly implemented, enabling dev mode only for OOO requests when isDev is true, otherwise defaulting to true.

However, consider using template literals as suggested by static analysis:

-  let finalUrl =
-    API_BASE_URL +
-    (nextLink ||
-      `/requests${getQueryParamsString(
-        requestType,
-        query,
-        requestType === 'OOO' ? isDev : true,
-      )}`);
+  let finalUrl = `${API_BASE_URL}${nextLink || `/requests${getQueryParamsString(
+    requestType,
+    query,
+    requestType === 'OOO' ? isDev : true,
+  )}`}`;

Also applies to: 165-170

🤖 Prompt for AI Agents 
In requests/script.js around lines 156 to 161 and 165 to 170, the current code
uses string concatenation with the getQueryParamsString function call inside
parentheses. Refactor these lines to use template literals for constructing the
URL string, embedding the function call directly within backticks and ${} syntax
to improve readability and consistency with modern JavaScript practices.


if (query?.state?.[0] || query?.requestedBy) {
finalUrl =
API_BASE_URL + `/requests${getQueryParamsString(requestType, query)}`;
API_BASE_URL +
`/requests${getQueryParamsString(
requestType,
query,
requestType === 'OOO' ? isDev : true,
)}`;
Comment on lines 153 to +170
Copy link

Choose a reason for hiding this comment

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

Complex URL Construction Logic category Readability

Tell me more
What is the issue?

URL construction logic is complex with multiple conditions and repeated patterns.

Why this matters

The URL construction is difficult to follow with nested ternaries and repeated concatenation patterns, making it hard to understand the final URL structure.

Suggested change ∙ Feature Preview
const buildRequestUrl = (requestType, query) => {
  const useDevParam = requestType === 'OOO' ? isDev : true;
  const queryString = getQueryParamsString(requestType, query, useDevParam);
  return `${API_BASE_URL}/requests${queryString}`;
};

async function getRequests(requestType, query = {}) {
  const finalUrl = nextLink || buildRequestUrl(requestType, query);
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

}
const notFoundErrorMessage =
requestType === ONBOARDING_EXTENSION_REQUEST_TYPE
Expand Down Expand Up @@ -617,13 +632,18 @@ async function performAcceptRejectAction(isAccepted, e) {
let remark = document.getElementById(`remark-text-${requestId}`).value;
let body = JSON.stringify({
type: currentReqType,
message: remark,
state: isAccepted ? 'APPROVED' : 'REJECTED',
[currentReqType === 'OOO' ? 'comment' : 'message']: remark,
[currentReqType === 'OOO' ? 'status' : 'state']: isAccepted
? 'APPROVED'
: 'REJECTED',
});
if (remark === '' || remark === undefined || remark === null) {
body = JSON.stringify({
type: currentReqType,
state: isAccepted ? 'APPROVED' : 'REJECTED',
[currentReqType === 'OOO' ? 'comment' : 'message']: remark,
[currentReqType === 'OOO' ? 'status' : 'state']: isAccepted
? 'APPROVED'
: 'REJECTED',
});
}

Expand Down
15 changes: 4 additions & 11 deletions requests/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ function createElementFromMap(domObjectMap) {
return el;
}

function getQueryParamsString(requestType, query) {
function getQueryParamsString(requestType, query, dev = true) {
const params = new URLSearchParams({
dev: 'true',
dev,
type: requestType,
size: '12',
});
Expand Down Expand Up @@ -71,18 +71,11 @@ function convertDateToReadableStringDate(date, format) {
}

function getFullNameOfUser(user) {
console.log('user', user);
Copy link

Choose a reason for hiding this comment

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

Debug Log in Production Code category Logging

Tell me more
What is the issue?

Debug console.log statement left in production code

Why this matters

Unintended logging in production can expose sensitive user data and impact application performance

Suggested change ∙ Feature Preview

Remove the console.log statement:

function getFullNameOfUser(user) {
  if (!user || typeof user !== 'object') {
    return 'N/A';
  }
  return user.username;
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Remove debug logging statement.

The console.log statement appears to be debug code that should not be committed to the codebase.

-  console.log('user', user);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
console.log('user', user);
🤖 Prompt for AI Agents 
In requests/util.js at line 74, remove the console.log('user', user) statement
as it is a debug log that should not be present in the committed code.

if (!user || typeof user !== 'object') {
return 'N/A';
}
const firstName = user.first_name || 'N/A';
const lastName = user.last_name || '';
return (
firstName.charAt(0).toUpperCase() +
firstName.slice(1).toLowerCase() +
' ' +
lastName.charAt(0).toUpperCase() +
lastName.slice(1).toLowerCase()
);
return user.username;
Comment on lines 73 to +78
Copy link

Choose a reason for hiding this comment

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

Misleading function name vs behavior category Documentation

Tell me more
What is the issue?

Function name is misleading as it now returns username instead of full name, and contains a debug console.log statement.

Why this matters

Outdated function documentation leads to confusion about the actual behavior of the code.

Suggested change ∙ Feature Preview

// Rename function to getUserDisplayName or update implementation to match function name

Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

}
Comment on lines 73 to 79
Copy link

Choose a reason for hiding this comment

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

Function Returns Username Instead of Full Name category Functionality

Tell me more
What is the issue?

The function has been changed to return username instead of the full name, which contradicts its name and expected functionality.

Why this matters

This change creates misleading behavior where callers expect a full name but receive a username, potentially breaking UI displays and data consistency across the application.

Suggested change ∙ Feature Preview

Either rename the function to getUsernameOfUser to match its new behavior, or restore the original full name functionality:

function getFullNameOfUser(user) {
  if (!user || typeof user !== 'object') {
    return 'N/A';
  }
  const firstName = user.first_name || 'N/A';
  const lastName = user.last_name || '';
  return (
    firstName.charAt(0).toUpperCase() +
    firstName.slice(1).toLowerCase() +
    ' ' +
    lastName.charAt(0).toUpperCase() +
    lastName.slice(1).toLowerCase()
  );
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.


function extractQueryParameters(url) {
Expand Down
Loading