fix: resolve collection of CLI and UI bugs (#3168, #3105, #3139)#3195
fix: resolve collection of CLI and UI bugs (#3168, #3105, #3139)#3195deepshekhardas wants to merge 5 commits intotriggerdotdev:mainfrom
Conversation
…ient to prevent startup crash
…nd github install url
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
WalkthroughThis pull request encompasses multiple, heterogeneous changes across the application. Key modifications include: updating RuntimeIcon to display "Unknown" instead of defaulting to Node.js when runtime cannot be parsed; normalizing ClickHouse URLs by removing the "secure" query parameter across multiple service files; updating a GitHub App installation link; adding filesystem validation for Bun executable paths; making schema fields for runtime and runtimeVersion nullable; enhancing the flattenAttributes utility with key escaping logic to handle dots and backslashes; improving Playwright browser parsing with awk-based extraction; adding conditional Docker registry credential handling for ECR; modifying API client payload handling with JSON serialization fallback; updating stream data parsing with conditional JSON parsing for string payloads; and adding an EmailLinkStrategy configuration option. Additionally, two new test files are introduced to validate the flatten/unflatten functionality. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 Map keys with dots are not escaped (pre-existing)
The new escapeKey function is applied to regular object keys at line 233, but the pre-existing Map handling code at lines 145-153 still constructs prefixes with raw (unescaped) Map keys: ${prefix || "map"}.${keyStr}. If a Map has a key containing a dot (e.g., "a.b"), the flattened key will contain an unescaped dot, causing unflattenAttributes to split it incorrectly. This is a pre-existing issue not introduced by this PR, but worth noting since the PR adds escaping for regular objects but not Maps.
(Refers to lines 145-153)
Was this helpful? React with 👍 or 👎 to provide feedback.
| import { flattenAttributes, unflattenAttributes } from "./packages/core/src/v3/utils/flattenAttributes"; | ||
|
|
||
| const cases = [ | ||
| { | ||
| name: "Key with period", | ||
| obj: { "Key 0.002mm": 31.4 }, | ||
| }, | ||
| { | ||
| name: "Nested key with period", | ||
| obj: { parent: { "child.key": "value" } }, | ||
| }, | ||
| { | ||
| name: "Regular nested key", | ||
| obj: { parent: { child: "value" } }, | ||
| }, | ||
| { | ||
| name: "Array with period in key", | ||
| obj: { "list.0": ["item1"] }, | ||
| }, | ||
| { | ||
| name: "Complex mixed", | ||
| obj: { | ||
| "a.b": { | ||
| "c.d": "value", | ||
| e: [1, 2] | ||
| } | ||
| } | ||
| } | ||
| ]; | ||
|
|
||
| let allPassed = true; | ||
|
|
||
| for (const { name, obj } of cases) { | ||
| const flattened = flattenAttributes(obj); | ||
| const unflattened = unflattenAttributes(flattened); | ||
| const success = JSON.stringify(unflattened) === JSON.stringify(obj); | ||
|
|
||
| console.log(`Case: ${name}`); | ||
| console.log(" Flattened:", JSON.stringify(flattened)); | ||
| console.log(" Unflattened:", JSON.stringify(unflattened)); | ||
| console.log(" Result:", success ? "SUCCESS" : "FAILURE"); | ||
|
|
||
| if (!success) allPassed = false; | ||
| } | ||
|
|
||
| if (allPassed) { | ||
| console.log("\nALL TESTS PASSED!"); | ||
| } else { | ||
| console.log("\nSOME TESTS FAILED!"); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🟡 Debug/reproduction scripts committed to repository root
Two debug/reproduction scripts (repro_1510.ts and test-flatten.ts) are committed to the repository root. These files import from relative paths, use console.log for assertions, and use process.exit(1) instead of proper test infrastructure. This violates AGENTS.md ("Follow the existing project conventions. Test files live beside the files under test and use descriptive describe and it blocks") and CLAUDE.md ("We use vitest exclusively"). These should either be converted to proper vitest tests placed next to packages/core/src/v3/utils/flattenAttributes.ts, or removed entirely.
Prompt for agents
Delete the files repro_1510.ts and test-flatten.ts from the repository root. If the test coverage is needed, add proper vitest tests in packages/core/test/flattenAttributes.test.ts (which already exists) using describe/it blocks to cover the round-trip escaping behavior for keys with periods and backslashes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| import { flattenAttributes, unflattenAttributes } from "./packages/core/src/v3/utils/flattenAttributes"; | ||
|
|
||
| const obj1 = { | ||
| "my.key.with.periods": "value1", | ||
| nested: { | ||
| "another.key": "value2" | ||
| } | ||
| }; | ||
|
|
||
| const flat = flattenAttributes(obj1); | ||
| console.log("Flattened:", flat); | ||
|
|
||
| const unflat = unflattenAttributes(flat); | ||
| console.log("Unflattened:", unflat); |
There was a problem hiding this comment.
🟡 Debug/reproduction script committed to repository root
Same issue as repro_1510.ts — test-flatten.ts is a debug script at the repository root that uses console.log instead of vitest, violating the project's testing conventions specified in AGENTS.md and CLAUDE.md.
Prompt for agents
Delete test-flatten.ts from the repository root. If coverage is needed, add the test cases to the existing vitest file at packages/core/test/flattenAttributes.test.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let credentials; | ||
| if (cloudRegistryHost.endsWith("amazonaws.com")) { | ||
| const [credentialsError, result] = await tryCatch( | ||
| getDockerUsernameAndPassword(apiClient, deploymentId) | ||
| ); | ||
|
|
||
| if (credentialsError) { | ||
| return { | ||
| ok: false as const, | ||
| error: `Failed to get docker credentials: ${credentialsError.message}`, | ||
| logs: "", | ||
| }; | ||
| if (credentialsError) { | ||
| return { | ||
| ok: false as const, | ||
| error: `Failed to get docker credentials: ${credentialsError.message}`, | ||
| logs: "", | ||
| }; | ||
| } | ||
| credentials = result; | ||
| } | ||
|
|
||
| logger.debug(`Logging in to docker registry: ${cloudRegistryHost}`); | ||
| if (credentials) { | ||
| logger.debug(`Logging in to docker registry: ${cloudRegistryHost}`); | ||
|
|
||
| const loginProcess = x( | ||
| "docker", | ||
| ["login", "--username", credentials.username, "--password-stdin", cloudRegistryHost], | ||
| { | ||
| nodeOptions: { | ||
| cwd: options.cwd, | ||
| }, | ||
| } | ||
| ); | ||
| const loginProcess = x( | ||
| "docker", | ||
| ["login", "--username", credentials.username, "--password-stdin", cloudRegistryHost], | ||
| { | ||
| nodeOptions: { | ||
| cwd: options.cwd, | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| loginProcess.process?.stdin?.write(credentials.password); | ||
| loginProcess.process?.stdin?.end(); | ||
| loginProcess.process?.stdin?.write(credentials.password); | ||
| loginProcess.process?.stdin?.end(); | ||
|
|
||
| for await (const line of loginProcess) { | ||
| errors.push(line); | ||
| logger.debug(line); | ||
| } | ||
| for await (const line of loginProcess) { | ||
| errors.push(line); | ||
| logger.debug(line); | ||
| } | ||
|
|
||
| if (loginProcess.exitCode !== 0) { | ||
| return { | ||
| ok: false as const, | ||
| error: `Failed to login to registry: ${cloudRegistryHost}`, | ||
| logs: extractLogs(errors), | ||
| }; | ||
| } | ||
| if (loginProcess.exitCode !== 0) { | ||
| return { | ||
| ok: false as const, | ||
| error: `Failed to login to registry: ${cloudRegistryHost}`, | ||
| logs: extractLogs(errors), | ||
| }; | ||
| } | ||
|
|
||
| options.onLog?.(`Successfully logged in to the remote registry`); | ||
| options.onLog?.(`Successfully logged in to the remote registry`); | ||
| } else { | ||
| logger.debug( | ||
| `Skipping automatic registry login for ${cloudRegistryHost}. Please ensure you are logged in locally.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🚩 Non-AWS registries now skip server-side authentication entirely
The buildImage.ts change at lines 489-539 now only fetches credentials from the Trigger.dev API and performs docker login for registries ending in amazonaws.com. For all other registries (GCR, GHCR, Azure CR, custom registries), the code logs a debug message and relies on the user being logged in locally. This is a behavioral change from the previous code which attempted API-based authentication for ALL registries when push && options.authenticateToRegistry. If getDockerUsernameAndPassword previously returned valid credentials for non-AWS registries (e.g., via a generic credential proxy), those deployments would now fail with authentication errors during push. The change appears intentional for self-hosted non-AWS scenarios, but the impact on existing users of non-AWS cloud registries should be verified.
Was this helpful? React with 👍 or 👎 to provide feedback.
| secret, | ||
| callbackURL: "/magic", | ||
| sessionMagicLinkKey: "triggerdotdev:magiclink", | ||
| validateSession: false, |
There was a problem hiding this comment.
🚩 validateSession: false reduces magic link security
Adding validateSession: false at apps/webapp/app/services/emailAuth.server.tsx:20 disables the check that the browser session requesting the magic link is the same one verifying it. This means magic links can be used from any browser/device, which is convenient but also means intercepted links (e.g., by email proxies, link preview bots, or attackers) can be used to authenticate. This is a common trade-off in magic link implementations and is likely intentional to fix cross-device/browser issues, but it does reduce the security posture of the auth flow.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This PR resolves the following issues: