Skip to content

[DevTools] PR2: Backend Services, Console Viewer, and Resources #2879

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

Open
wants to merge 75 commits into
base: feature/dev-tools
Choose a base branch
from

Conversation

megha-narayanan
Copy link

@megha-narayanan megha-narayanan commented Jul 2, 2025

Changes

Implements a functional DevTools UI for the Amplify Sandbox, building upon the foundation established in PR1. Key features include:

  • Web-based interface for managing sandbox environments
  • Real-time monitoring of sandbox status and deployment progress
  • Console log viewer with formatted logs
  • Resource explorer to view and manage deployed AWS resources
  • WebSocket-based communication between UI and backend
  • Enhanced sandbox lifecycle management (start, stop, delete)
  • Improved error handling and status reporting

The implementation uses Express with Socket.IO for the backend server and React with Cloudscape Design components for the frontend UI.

Validation

Unit tests have been included, though some files will only be further tested in later PRs

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

changeset-bot bot commented Jul 2, 2025

🦋 Changeset detected

Latest commit: 4861a0d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@aws-amplify/deployed-backend-client Patch
@aws-amplify/platform-core Patch
@aws-amplify/cli-core Patch
@aws-amplify/sandbox Minor
@aws-amplify/backend-cli Minor
@aws-amplify/backend-deployer Patch
@aws-amplify/ai-constructs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@megha-narayanan megha-narayanan marked this pull request as ready for review July 3, 2025 00:13
@megha-narayanan megha-narayanan requested review from a team as code owners July 3, 2025 00:13
Copy link
Contributor

@Amplifiyer Amplifiyer left a comment

Choose a reason for hiding this comment

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

Since it's quite a big PR, I've reviewed the backend pieces at a high level and left feedback. I'll take a pass at frontend components later.

package.json Outdated
Comment on lines 62 to 66
"@aws-sdk/client-cloudformation": "^3.828.0",
"@aws-sdk/client-cloudwatch-logs": "^3.835.0",
"@aws-sdk/client-cognito-identity-provider": "^3.750.0",
"@aws-sdk/client-dynamodb": "^3.750.0",
"@aws-sdk/client-iam": "^3.750.0",
"@aws-sdk/client-iam": "^3.835.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need aws-sdk upgrade as part of this? If not, we should decouple.

Copy link
Author

Choose a reason for hiding this comment

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

reverting.

package.json Outdated
Comment on lines 124 to 133
"dependencies": {
"@aws-sdk/client-lambda": "^3.835.0",
"cookie-signature": "1.0.6",
"debounce-promise": "^3.1.2",
"esbuild": "0.25.5",
"express": "^5.1.0",
"express-rate-limit": "^7.5.0",
"jszip": "^3.10.1",
"ws": "^8.18.2"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need all these dependencies, and in the root package?

Copy link
Author

Choose a reason for hiding this comment

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

Nope. Removing

Comment on lines 15 to 21
* Attempts to start the server on the specified port
* @param server The HTTP server
* @param port The port to use
* @returns A promise that resolves with the port when the server starts
* @throws Error if the port is already in use
*/
async findAvailablePort(
Copy link
Contributor

Choose a reason for hiding this comment

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

The method name is not matching the documentation and what it is doing. And why are we starting a server in PortChecker class?

Copy link
Author

Choose a reason for hiding this comment

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

moving the server starting to devtools command -- this is a relic from when we tried multiple ports.

Comment on lines 28 to 31
const cleanAnsiCodes = (text: string): string => {
// This regex handles various ANSI escape sequences including colors, bold, dim, etc.
return text.replace(/\u001b\[\d+(;\d+)*m|\[2m|\[22m|\[1m|\[36m|\[39m/g, '');
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Look into stripANSI which is already used in this repo.

Copy link
Member

Choose a reason for hiding this comment

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

Have we already discussed keeping the styles offline with something like ansi-to-html?

Copy link
Author

Choose a reason for hiding this comment

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

switched to stripANSI for now-- is there a benefit to taking it offline if stripAnsi already exists in the repo?

* @param constructPath The CDK construct path
* @returns A normalized construct path
*/
const normalizeCDKConstructPath = (constructPath: string): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems duplicated from here

private normalizeCDKConstructPath = (constructPath: string): string => {

We should see if we can refactor cfn_deployment_progress_logger.ts such that it can be used for both CLI and DevConsole instead of duplicating the logic

Copy link
Author

Choose a reason for hiding this comment

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

pulled that function out and exported it to get rid of duplication.

if (sandboxState === 'unknown') {
try {
// Check AWS stack state
const cfnClient = new CloudFormationClient();
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these clients should be injected at runtime. Will make the unit testing much easier and simpler.

Copy link
Author

Choose a reason for hiding this comment

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

done. this does make unit testing a lot easier! I'll go ahead and make unit tests more robust in a separate commit

Comment on lines 154 to 156
await cfnClient.send(
new DescribeStacksCommand({ StackName: stackName }),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

You are not using the response here?

Copy link
Author

Choose a reason for hiding this comment

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

we're just checking if the stack exists. but maybe using exceptions for control flow is not appropriate. Would it be better if I used ListStacksCommand or validateTemplate. Or waitUntilStackExists with a short timeout?

Comment on lines 257 to 272
// Override printer.log to capture the log level
printer.log = function (message: string, level?: LogLevel) {
currentLogLevel = level ? level : LogLevel.DEBUG;
const result = originalLog.call(this, message, currentLogLevel);
currentLogLevel = null;
return result;
};

// Override printer.print (the lower-level method)
printer.print = function (message: string) {
// Call the original print method
originalPrint.call(this, message);
// Avoid double processing if this is called from printer.log
if (processingMessage) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid doing this. This creates sideeffects in other parts of code that would be very hard to debug/troubleshoot

Copy link
Author

Choose a reason for hiding this comment

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

refactored to pass in my own printer. required taking sandboxfactory out of the injected dependencies -- let me know what you think about the approach I just pushed.

Comment on lines 195 to 218
// Get region information
const cfnClient = new CloudFormationClient();
const regionValue = cfnClient.config.region;
let region = null;

try {
if (typeof regionValue === 'function') {
if (regionValue.constructor.name === 'AsyncFunction') {
region = await regionValue();
} else {
region = regionValue();
}
} else if (regionValue) {
region = String(regionValue);
}

// Final check to ensure region is a string
if (region && typeof region !== 'string') {
region = String(region);
}
} catch (error) {
printer.log('Error processing region: ' + error, LogLevel.ERROR);
region = null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

See if we can reuse this

through a DI, so we can reuse the cached results instead of finding the region every time.

Copy link
Author

Choose a reason for hiding this comment

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

yep, this also makes testing easy. Adding it.

/**
* Service for handling socket events
*/
export class SocketHandlerService {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class also needs required unit test coverage

Copy link
Author

Choose a reason for hiding this comment

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

yep, I hadn't put it in this PR. updated and added.

Copy link
Member

@svidgen svidgen left a comment

Choose a reason for hiding this comment

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

Can't say I did a full review here — there's a lot here. 🎉 But, I left a handful of comments for you for things that jumped out at me.

My other big concern across the board is testing. Given that this is destined for a feature branch, and given that it's already huge, I could be convinced to see a separate PR focused on e2e testing. Some lower level unit/interaction testing would be good too. But, long term, before I'd put this in front of customers, I'd want to know I "can't" break it. And, I think e2e's are how I gain that confidence.

Comment on lines 28 to 31
const cleanAnsiCodes = (text: string): string => {
// This regex handles various ANSI escape sequences including colors, bold, dim, etc.
return text.replace(/\u001b\[\d+(;\d+)*m|\[2m|\[22m|\[1m|\[36m|\[39m/g, '');
};
Copy link
Member

Choose a reason for hiding this comment

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

Have we already discussed keeping the styles offline with something like ansi-to-html?

Comment on lines 74 to 96
/**
* Clean ANSI escape codes from text
* @param text The text to clean
* @returns The cleaned text
*/
export const cleanAnsiCodes = (text: string): string => {
// Split the regex into parts to avoid control characters
const ansiEscapeCodesPattern = new RegExp(
[
// ESC [ n ; n ; ... m
String.fromCharCode(27) + '\\[\\d+(?:;\\d+)*m',
// Other common ANSI sequences
'\\[2m',
'\\[22m',
'\\[1m',
'\\[36m',
'\\[39m',
].join('|'),
'g',
);

return text.replace(ansiEscapeCodesPattern, '');
};
Copy link
Member

Choose a reason for hiding this comment

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

What's functionally different about this cleaner and the one below?

Copy link
Author

Choose a reason for hiding this comment

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

already resolved in commit 87fa132 (duplicate from trimming down PR)

Comment on lines 55 to 72
/**
* Extracts a friendly name from a nested stack logical ID
* @param stackName The stack name to process
* @returns A friendly name or undefined if no match
*/
const getFriendlyNameFromNestedStackName = (
stackName: string,
): string | undefined => {
const parts = stackName.split('-');

if (parts && parts.length === 7 && parts[3] === 'sandbox') {
return parts[5].slice(0, -10) + ' stack';
} else if (parts && parts.length === 5 && parts[3] === 'sandbox') {
return 'root stack';
}

return undefined;
};
Copy link
Member

Choose a reason for hiding this comment

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

A good habit when baking arcane splits and magic numbers into a function like this is to show an example or two of the before/after in the docstring. If there is a good name you can give to the magic constants here, give them names. Else, it's helpful to indicate where they come from in the docstring examples.

In this case, the naming patterns might be completely obvious to others in the codebase. But, if someone new (it's always Day 1, right?) jumps in at this point, examples will help.

Leaving this comment on this particularly arcane looking set of manipulations. But, please apply anywhere you feel arcane incantations are being performed.

Copy link
Author

Choose a reason for hiding this comment

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

got it. actually figured out how to actually get non-null metadata which eliminates the need for some of these functions. but examples have been added in the remaining functions where I felt it was needed.

* @param metadata.constructPath Optional construct path from CDK metadata
* @returns A user-friendly name for the resource
*/
export const createFriendlyName = (
Copy link
Member

Choose a reason for hiding this comment

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

Borderline arcane incantations herein. Partially cp-ing a comment from below. Examples in docstrings will help future maintainers.

Copy link
Member

Choose a reason for hiding this comment

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

Thank-you! Probably goes without saying, but explicit test coverage would also be ideal. Unless I'm overlooking it, just a small set of examples like you have here in a test loop would be good.

Comment on lines 106 to 112
cleanedMessage.includes('_IN_PROGRESS') ||
cleanedMessage.includes('CREATE_') ||
cleanedMessage.includes('DELETE_') ||
cleanedMessage.includes('UPDATE_') ||
cleanedMessage.includes('Deployment in progress') ||
cleanedMessage.includes('COMPLETE') ||
cleanedMessage.includes('FAILED') ||
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't mind seeing something like this if it speaks to you:

[
  '_IN_PROGRESS',
  'CREATE_',
  'DELETE_',
  'UPDATE_',
  'Deployment in progress',
  'COMPLETE',
  'FAILED'
].some(pattern => cleanedMessage.includes(pattern))

But, with room for opinion here, so adopt only if it really speaks to you.

Copy link
Author

Choose a reason for hiding this comment

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

will do.


// Exit the process if requested
if (exitProcess) {
// Short delay to allow messages to be sent
Copy link
Member

Choose a reason for hiding this comment

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

What messages? Why is 500 ms the right amount of time to wait for them to send?

Copy link
Author

@megha-narayanan megha-narayanan Jul 9, 2025

Choose a reason for hiding this comment

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

I was sending a shutdown log but I guess that isn't super helpful because the process is, well, shutting down. If we want to log, we can always log on socket disconnect. Getting rid of the log and delay. This delay was also for the server close, which is async but doesn't return a promise. resolving that properly with a promise wrapper to make sure server closing is finished before we end the process.

private sandbox: Sandbox,
private getSandboxState: () => Promise<string>,
private backendId: { name: string },
private shutdownService: import('./shutdown_service.js').ShutdownService,
Copy link
Member

Choose a reason for hiding this comment

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

Why import() ShutdownService as a function here instead of letting it hang out with its other import-ant friends?

Copy link
Author

Choose a reason for hiding this comment

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

Moving it to the top with the other imports!

Comment on lines 124 to 129
socket.on('savedResources', handleSavedResources);
socket.on('deployedBackendResources', handleDeployedBackendResources);
socket.on('customFriendlyNames', handleCustomFriendlyNames);
socket.on('customFriendlyNameUpdated', handleCustomFriendlyNameUpdated);
socket.on('customFriendlyNameRemoved', handleCustomFriendlyNameRemoved);
socket.on('error', handleError);
Copy link
Member

Choose a reason for hiding this comment

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

Just picking a random spot to leave this comment: Would be really good to have constants imported from a central spot (at least per group) to match on instead of duplicated strings all over. In theory, a typo in one of those strings could be caught by a good suite of tests as well, but caught much sooner if caught statically.

(Bonus points if you include docstrings to indicate what the constants mean.)

Most IDEs can also help track down usage of a variable/constant more definitively than searching for a string literal.

Copy link
Author

Choose a reason for hiding this comment

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

done. put constants in a shared location that frontend and backend can both access.

Comment on lines 78 to 82
if (regionValue.constructor.name === 'AsyncFunction') {
region = await regionValue();
} else {
region = regionValue();
}
Copy link
Member

Choose a reason for hiding this comment

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

Q for the team: Do we have lint rules in place that would prevent this from just being await regionValue() either way? If not, there's usually not much reason to check whether a thing is async. If it can be async, just await it. (Right?)

Copy link
Author

Choose a reason for hiding this comment

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

resolved by eliminating this code by using RegionFetcher

Comment on lines 124 to 129
// case 'AWS::DynamoDB::Table': // COMMENT OUT (UNTESTED)
// // Check if physicalId is an ARN and extract table name if needed
// const dynamoTableName = physicalId.includes(':table/')
// ? physicalId.split(':table/')[1]
// : physicalId;
// return `${baseUrl}/dynamodb/home?region=${region}#/tables/view/${encodeURIComponent(dynamoTableName)}`;
Copy link
Member

Choose a reason for hiding this comment

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

You mean not even manually tested? The others aren't covered by unit or integ tests yet, are they?

Copy link
Author

Choose a reason for hiding this comment

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

yep, I did manual testing now though so I've added these back in.

Megha Narayanan added 7 commits July 8, 2025 15:40
…eploymentProgressLogger and createFriendlyName. maintains backward compatibility
- Introduced SocketClientService to handle socket connections and events.
- Created SandboxClientService and ResourceClientService for sandbox and resource-specific socket communication.
- Updated App component to utilize context for socket services.
- Refactored useResourceManager hook to use ResourceClientService.
- Updated ResourceConsole and Header components to use new service structure.
Copy link
Contributor

@rtpascual rtpascual left a comment

Choose a reason for hiding this comment

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

Looking good and this is a lot to go through, for starters I left a few comments focusing on the frontend parts of the PR

Comment on lines 74 to 76
window.confirm(
'Are you sure you want to delete the sandbox? This will remove all resources and cannot be undone.',
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why window instead of Modal here? Same in handleStopDevTools.

Copy link
Author

Choose a reason for hiding this comment

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

Good question! I've made them a custom Modal now.

Comment on lines 95 to 96
? `Sandbox ${sandboxStatus === 'nonexistent' ? '' : `(${sandboxIdentifier}) `}`
: 'Sandbox ';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think it would be easier to read by moving the space after the status text to the StatusIndicator text. This avoids accidentally duplicating the space between words:

<StatusIndicator>{statusText} Deploying</StatusIndicator>

Resulting in <statusText>(two spaces)Deploying

Comment on lines 303 to 338
useEffect(() => {
if (!connected) return;

// If status is unknown, request it periodically
if (sandboxStatus === 'unknown') {
const statusCheckInterval = setInterval(() => {
console.log('Requesting sandbox status due to unknown state');
sandboxClientService.getSandboxStatus();

// Add a log entry to show we're still trying
setLogs((prev) => [
...prev,
{
id: Date.now().toString(),
timestamp: new Date().toISOString(),
level: 'INFO',
message: 'Requesting sandbox status...',
},
]);
}, 5000); // Check every 5 seconds

return () => clearInterval(statusCheckInterval);
}
}, [sandboxStatus, connected, sandboxClientService]);

// Force a status check if we've been in unknown state for too long
useEffect(() => {
if (sandboxStatus === 'unknown' && connected) {
const forceStatusCheck = setTimeout(() => {
console.log('Forcing sandbox status check after timeout');
sandboxClientService.getSandboxStatus();
}, 2000); // Force a check after 2 seconds

return () => clearTimeout(forceStatusCheck);
}
}, [sandboxStatus, connected, sandboxClientService]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these useEffects be combined into one since the effect dependencies are identical?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, done

Copy link
Contributor

@Amplifiyer Amplifiyer left a comment

Choose a reason for hiding this comment

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

Looking really good. Still haven't gone through the entire PR, but wanted to leave this feedback early.

package.json Outdated
Comment on lines 62 to 65
"@aws-sdk/client-cloudformation": "^3.828.0",
"@aws-sdk/client-cloudwatch-logs": "^3.835.0",
"@aws-sdk/client-cognito-identity-provider": "^3.750.0",
"@aws-sdk/client-dynamodb": "^3.750.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I still see these upgrades for some aws-sdk clients. We should avoid this

Copy link
Author

Choose a reason for hiding this comment

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

oops. missed that. fixed now!

Comment on lines 63 to 68
/**
* Extract nested stack names
* @deprecated Use the shared normalizeCDKConstructPath function from formatters/cdk_path_formatter.js instead
*/
private normalizeCDKConstructPath = normalizeCDKConstructPath;

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

I just wanted to be super careful about backward compatibility. If you're ok with it, I'll go ahead and remove.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, as long as these are local variables and not exporting something out of package, it's fine to get rid of them

* @param data The event data
*/
protected emit<T>(event: string, data?: T): void {
if (!this.socket) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a throw instead? Or at least an error?

Copy link
Contributor

Choose a reason for hiding this comment

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

The socket is only set at the initialization, right? In which scenario would this variable be null?

Copy link
Author

Choose a reason for hiding this comment

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

I thought throwing an error would be too disruptive since the socket.io client has reconnection logic, and so the socket might temporarily be null during reconnection attempts. Plus then we would need try/catch blocks everywhere. But I agree silently failing isn't ideal. I've added a console.error instead. Does this seem like a good solution?

if (!this.socket) {
  console.error(`Cannot emit ${event}: Socket is not initialized`);
  return;
}

Copy link
Author

Choose a reason for hiding this comment

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

ah, I misunderstood the socket logic. yep, you're right. making it a throw.

protected on<T>(event: string, handler: (data: T) => void): () => void {
if (!this.socket) return () => {};
this.socket.on(event, handler);
return () => this.socket?.off(event, handler);
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be a named method, called unsubscribe or something?

Copy link
Author

@megha-narayanan megha-narayanan Jul 16, 2025

Choose a reason for hiding this comment

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

It definitely can be, but I don't see a clear benefit. Is it for visibility in stack traces? This is called by this class and its subclasses, and whenever called, the return value is made a named function, so that seems to mitigate the stack trace issue. I can absolutely change it just for style, just want to make sure I'm understanding this comment properly before acting on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the DX issue. If I'm understanding it correctly, currently the DX is

const subscription = clientService.on();
// To unsubscribe
subscription();

What I'm proposing is

const subscription = clientService.on();
// To unsubscribe
subscription.unsubscribe();

Copy link
Author

@megha-narayanan megha-narayanan Jul 16, 2025

Choose a reason for hiding this comment

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

got it. I had them like this: const unsubscribeError = resourceClientService.onError(handleError); and then just call unsubscribeError() which matches all the other handlers. I'll go ahead and change them all, but then you have a bunch of calls like this unsubscribeLog.unsubscribe();

Comment on lines 18 to 23
/**
* Interface for sandbox options
*/
export interface SandboxOptions {
identifier?: string;
once?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should ideally come from sandbox package to avoid duplication and sandbox being the authority of what options it accepts.

Copy link
Author

Choose a reason for hiding this comment

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

fixed with the temp fix we discussed and added a note about the issue in the comment

Comment on lines 398 to 418
const backendClient = new DeployedBackendClientFactory().getInstance(
this.awsClientProvider,
);

// Create a custom logger that forwards logs to Socket.IO clients
const devToolsLogger = new DevToolsLogger(
this.printer,
io,
minimumLogLevel,
);
this.printer = devToolsLogger; // Use the custom logger for all CLI output from now on

// Create a new SandboxSingletonFactory with our custom logger
// Our server, and thus logger, are only created at runtime, so we cannot use the
// SandboxSingletonFactory as an injected dependency.
const localSandboxFactory = new SandboxSingletonFactory(
this.sandboxBackendIdResolver.resolve,
new SDKProfileResolverProvider().resolve,
devToolsLogger,
this.format,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we inject these through factory as well?

Copy link
Author

Choose a reason for hiding this comment

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

See the comment. I couldn't figure out a neat way to do it. Any input?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can create a DevToolsLoggerFactory, inject it through devtools command factory, and use it to instantiate an instance at runtime.

Similarly, sandboxFactory.getInstance() can take an optional instance of logger which is available at runtime

Copy link
Author

Choose a reason for hiding this comment

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

got it.

Co-authored-by: Roshane Pascual <rtpascual@users.noreply.github.com>
Co-authored-by: Amplifiyer <51211245+Amplifiyer@users.noreply.github.com>
Comment on lines 28 to 30
resourceConfigChanged: [
async () => {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find where this event is being emitted by the sandbox

Copy link
Author

Choose a reason for hiding this comment

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

Yep, its extraneous now that it been replaced with successfulDeployment/failedDeployment for better error handling. removed in PR3 but forgot to propogate back to PR2. Doing that now.

Comment on lines +37 to +41
/**
* Gets the current state of the sandbox
* @returns The current state: 'running', 'stopped', 'deploying', 'deleting', 'nonexistent', or 'unknown'
*/
getState: () => SandboxStatus;
Copy link
Contributor

Choose a reason for hiding this comment

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

This state management needs more unit test coverage.

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely. In writing my tests I also discovered a tiny edge case error in my state management -- fixed that and added tests.

Comment on lines 112 to 113
metadata:
templateMetadata[stackResourceSummary.LogicalResourceId || ''],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be

Suggested change
metadata:
templateMetadata[stackResourceSummary.LogicalResourceId || ''],
metadata:
templateMetadata[stackResourceSummary.LogicalResourceId] || '',

Copy link
Author

@megha-narayanan megha-narayanan Jul 16, 2025

Choose a reason for hiding this comment

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

EDIT: okay, turns out I actually misunderstood this. The reason I had my original code was because in case LogicalResourceId is undefined (which it can be), then we just look for the the metadata of ''. Though I imagine this is not the best way to handle that error. The metadata property should be of type{ constructPath?: string } | undefined, not string. I have now re-updated this to the following line, which is more intentional about handling the undefined case - instead of falling back to an empty string key (which likely doesn't exist in the metadata, but maybe in an error case, COULD), it explicitly returns undefined. It avoids a potential issue where templateMetadata[''] might actually contain something unexpected. stackResourceSummary.LogicalResourceId ? templateMetadata[stackResourceSummary.LogicalResourceId] : undefined,

Comment on lines 104 to 106
const statusText = sandboxIdentifier
? `Sandbox ${sandboxStatus === 'nonexistent' ? '' : `(${sandboxIdentifier}) `}`
: 'Sandbox ';
? `Sandbox${sandboxStatus === 'nonexistent' ? '' : ` (${sandboxIdentifier})`}`
: 'Sandbox';
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking another look at this, we can make this easier to read if we break this out of nested ternaries

const statusText = sandboxIdentifier && sandboxStatus !== 'nonexistent' ?
  `Sandbox (${sandboxIdentifier})` : 'Sandbox';

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.

4 participants