feat: suggestions import #74
Conversation
WalkthroughAdds retries to Cypress config, replaces iframe helpers with iframeReady/iframeBody and updates tests, introduces Cypress API client helpers and example data, changes dev ports, adds Jest and a new util with tests, generates OpenAPI types, refactors Push flow into a new hook with conflict resolution UI, and adds backend health-check startup logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant PushView as Push.tsx
participant Hook as usePush()
participant API as Backend API
participant Store as Global State
Note over PushView,Hook: Initial load
PushView->>Hook: usePush() init
Hook->>Store: read config (tags/screenshots/namespace)
Note over User,PushView: Submit push
User->>PushView: Click "Push"
PushView->>Hook: push({ override?: false })
alt Screenshots required
Hook->>API: POST /v2/image-upload (per image)
API-->>Hook: image ids
end
Hook->>API: POST update translations (new or old API)
alt New API (resolvable)
API-->>Hook: { unresolvedConflicts? }
Hook-->>PushView: { unresolvedConflicts }
opt Conflicts present
PushView->>User: Show UnresolvedConflicts
User->>PushView: Click "Override all"
PushView->>Hook: push({ override: true })
Hook->>API: POST update translations (override mode)
API-->>Hook: success
end
else Legacy API
API-->>Hook: success
end
opt Tags enabled
Hook->>API: PUT /v2/projects/tag-complex
API-->>Hook: success
end
opt Update screenshots enabled
Hook->>API: POST /v2/projects/big-meta
API-->>Hook: success
end
Hook-->>PushView: success info
PushView->>User: Show success (excluding conflicts)
sequenceDiagram
autonumber
participant Script as runAfterApp.ts
participant Health as http://localhost:22223/actuator/health
participant Compose as docker-compose
participant After as afterProcesses
Script->>Health: GET /actuator/health
alt Healthy
Script->>Script: Skip docker-compose
else Unhealthy
Script->>Compose: docker-compose up
Compose-->>Script: logs/streams
loop Until healthy
Script->>Health: GET /actuator/health
Health-->>Script: status
end
end
Script->>After: start post-app processes
Note over Script: Handle SIGINT / cleanup remaining children
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
fix: add jest tests to pipeline feat: display non-overridable conflicts
stepan662
force-pushed
the
stepangranat/support-new-import
branch
from
0d53fe5 to
68ba732
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress/support/commands.ts (1)
23-40: Remove the orphanediframeDocumentdeclaration.The
iframeDocumentmethod is declared in the interface (line 30) but its implementation has been removed. This creates a type/runtime mismatch that will cause errors if any code attempts to callcy.iframeDocument().Apply this diff:
interface Chainable { - /** - * Custom command to get the document of the plugin iframe. - * @example cy.iframeDocument().should('exist') - */ - iframeDocument(): Chainable<Document>; /** * Custom command to get the body of the plugin iframe. * @example cy.iframeBody().find('button').click() */ iframeBody(): Chainable<JQuery<HTMLBodyElement>>; iframeReady(): void; }
🧹 Nitpick comments (8)
src/types.ts (1)
125-125: LGTM!The optional
overrideAllproperty is a clean addition toGlobalSettings. Consider adding a JSDoc comment to clarify its purpose, similar to the documentation provided forprefillKeyFormatandkeyFormatabove.jest.config.js (1)
1-11: Consider simplifying the transform configuration.The current approach spreads
tsJestTransformCfginto thetransformproperty. SincecreateDefaultPreset().transformalready returns the complete transform configuration object, you can simplify by directly assigning it:-const tsJestTransformCfg = createDefaultPreset().transform; - /** @type {import("jest").Config} **/ module.exports = { testEnvironment: "node", - transform: { - ...tsJestTransformCfg, - }, + transform: createDefaultPreset().transform, };Alternatively, spread the entire preset for cleaner configuration:
-const tsJestTransformCfg = createDefaultPreset().transform; - /** @type {import("jest").Config} **/ module.exports = { + ...createDefaultPreset(), testEnvironment: "node", - transform: { - ...tsJestTransformCfg, - }, };src/ui/views/Push/UnresolvedConflicts.css.d.ts (1)
1-6: Prefer ESM-compatible export for CSS modulesUse default export to match
import styles from './*.css'in TS/ESM.-declare const styles: { +declare const styles: { readonly "conflicts": string; readonly "container": string; readonly "styledHint": string; -}; -export = styles; +}; +export default styles;Please confirm how UnresolvedConflicts.tsx imports the CSS module.
scripts/runAfterApp.ts (1)
45-46: Minor: http.get already ends the requestCalling req.end() after http.get is unnecessary; safe to remove for clarity.
Also applies to: 58-59
src/client/apiSchema.generated.ts (1)
1-17: Remove unused src/client/apiSchema.generated.ts
The file at src/client/apiSchema.generated.ts isn’t referenced (all imports point to src/ui/client/apiSchema.generated.ts); delete it to avoid stale code.src/tools/satisfiesMinimalVersion.test.ts (1)
11-16: Consider clarifying the test description.The test description mentions "higher" but also tests the equality case (line 12). Consider renaming to better reflect all scenarios covered.
Apply this diff:
- it("returns true if version is higher", () => { + it("returns true if version is equal or higher", () => { expect(satisfiesMinimalVersion("v1.2.3", "v1.2.3")).toEqual(true); expect(satisfiesMinimalVersion("v1.2.3", "v1.2.4")).toEqual(true); expect(satisfiesMinimalVersion("v1.2.3", "v1.4.2")).toEqual(true); expect(satisfiesMinimalVersion("v1.2.3", "v2.0.0")).toEqual(true); });src/tools/satisfiesMinimalVersion.ts (1)
1-19: Document version format expectations
Current regex only matchesvX.Y.Z; extend it for prerelease/build metadata (e.g.,v1.2.3-alphaorv1.2.3+build) if your platform emits them, and document the supported format.cypress/e2e/push.cy.ts (1)
25-27: Add null check before cleanup.The
afterEachhook callsdeleteProject(client)without verifying thatclientwas successfully initialized. If thebeforeEachhook fails, this could throw an error.Apply this diff:
afterEach(() => { - deleteProject(client); + if (client) { + deleteProject(client); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.github/workflows/test.yml(1 hunks)cypress.config.mjs(1 hunks)cypress/common/apiClient.ts(1 hunks)cypress/common/exampleProject.ts(1 hunks)cypress/common/languageTestData.ts(1 hunks)cypress/common/tools.ts(1 hunks)cypress/docker-compose.yaml(1 hunks)cypress/e2e/copyView.cy.ts(5 hunks)cypress/e2e/index.cy.ts(4 hunks)cypress/e2e/language.cy.ts(1 hunks)cypress/e2e/pageSetup.cy.ts(1 hunks)cypress/e2e/pull.cy.ts(2 hunks)cypress/e2e/push.cy.ts(2 hunks)cypress/e2e/settings.cy.ts(4 hunks)cypress/e2e/stringDetails.cy.ts(6 hunks)cypress/support/commands.ts(1 hunks)cypress/tsconfig.json(0 hunks)jest.config.js(1 hunks)package.json(2 hunks)scripts/runAfterApp.ts(3 hunks)src/client/apiSchema.generated.ts(1 hunks)src/tools/satisfiesMinimalVersion.test.ts(1 hunks)src/tools/satisfiesMinimalVersion.ts(1 hunks)src/types.ts(1 hunks)src/ui/client/client.ts(2 hunks)src/ui/views/Push/Push.tsx(5 hunks)src/ui/views/Push/UnresolvedConflicts.css(1 hunks)src/ui/views/Push/UnresolvedConflicts.css.d.ts(1 hunks)src/ui/views/Push/UnresolvedConflicts.tsx(1 hunks)src/ui/views/Push/usePush.ts(1 hunks)src/web/ui.tsx(1 hunks)src/web/urlConfig.ts(1 hunks)tsconfig.json(1 hunks)web/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- cypress/tsconfig.json
🧰 Additional context used
🧬 Code graph analysis (10)
cypress/common/tools.ts (1)
src/web/urlConfig.ts (2)
PluginData(5-5)createShortcutUrl(7-9)
cypress/common/exampleProject.ts (1)
src/client/apiSchema.generated.ts (1)
components(11-11)
cypress/common/apiClient.ts (2)
src/client/apiSchema.generated.ts (1)
components(11-11)cypress/common/languageTestData.ts (1)
languagesTestData(3-26)
cypress/e2e/push.cy.ts (4)
cypress/common/apiClient.ts (4)
deleteProject(6-11)Options(13-15)createProjectWithClient(17-62)createPak(76-82)src/web/urlConfig.ts (2)
createTestNode(82-93)SIGNED_IN(39-45)cypress/common/tools.ts (1)
visitWithState(5-8)cypress/common/exampleProject.ts (1)
EXAMPLE_PROJECT(8-19)
src/tools/satisfiesMinimalVersion.test.ts (1)
src/tools/satisfiesMinimalVersion.ts (1)
satisfiesMinimalVersion(3-19)
cypress/e2e/copyView.cy.ts (2)
cypress/common/tools.ts (1)
visitWithState(5-8)src/web/urlConfig.ts (1)
PAGE_COPY_LANGUAGE(62-65)
cypress/e2e/settings.cy.ts (2)
cypress/common/tools.ts (1)
visitWithState(5-8)src/web/urlConfig.ts (1)
DEFAULT_CREDENTIALS(34-37)
src/ui/views/Push/usePush.ts (7)
src/ui/client/apiSchema.generated.ts (1)
components(846-3281)src/ui/client/useQueryApi.ts (1)
useApiMutation(66-113)src/tools/getPushChanges.ts (2)
KeyChanges(17-22)KeyChangeValue(7-15)src/types.ts (1)
FrameScreenshot(68-72)src/tools/compareNs.ts (1)
compareNs(1-3)src/tools/satisfiesMinimalVersion.ts (1)
satisfiesMinimalVersion(3-19)src/ui/client/client.ts (1)
API_VERSION(7-7)
cypress/common/languageTestData.ts (1)
src/client/apiSchema.generated.ts (1)
components(11-11)
src/ui/views/Push/Push.tsx (2)
src/ui/views/Push/usePush.ts (1)
usePush(33-221)src/ui/views/Push/UnresolvedConflicts.tsx (1)
UnresolvedConflicts(22-47)
🔇 Additional comments (26)
src/web/ui.tsx (1)
12-12: Verify no initialization race conditions.Removing the
setTimeoutwrapper makes rendering synchronous instead of deferring it to the next event loop tick. Ensure that no initialization code (e.g., data fetching, event handlers, or Figma plugin API calls) depends on the previous async timing.src/ui/client/client.ts (2)
7-8: LGTM with note on shared mutable state.Exporting a module-level
letvariable for API version tracking is a straightforward pattern. Be aware that if multiple requests are in flight concurrently, the last completed request setsAPI_VERSION, which may not correspond to the most recently initiated request. This is generally acceptable for version headers that should be consistent across responses.
105-106: LGTM!Correctly extracts the version header and handles the special "??" placeholder by treating it as undefined.
cypress/docker-compose.yaml (1)
7-7: LGTM!Port mapping change to 22223 is consistent across the file, and disabling rate limits is appropriate for E2E testing environments to avoid flaky tests due to throttling.
Also applies to: 12-12, 19-21
src/web/urlConfig.ts (1)
35-35: LGTM!The port change to 22223 is consistent with the updated Docker Compose configuration and propagates correctly to all derived configuration objects (SIGNED_IN, PAGE_COPY, etc.).
web/package.json (1)
3-3: LGTM!The explicit port binding to 22224 for the watch script aligns with the broader port reorganization in this PR.
cypress/common/exampleProject.ts (1)
1-19: Ensure @tginternal/client is installed & exports required schema
node_modules/@tginternal/clientnot found—confirm the package is listed in your dependencies and that it exportscomponents["schemas"]["SingleStepImportResolvableRequest"]with the expected structure.cypress.config.mjs (1)
5-7: LGTM: retries configGood defaults for flake reduction in run mode; no issues spotted.
.github/workflows/test.yml (1)
74-76: LGTM: add Jest step before CypressOrder makes sense. Verify Jest is configured (ts-jest) so this step passes reliably.
scripts/runAfterApp.ts (1)
126-134: Global fetch requires Node 18+ (Node 20 in CI is fine). Verify local dev Node versionUsing fetch assumes Node >=18. Ensure local dev uses Node 18/20+, or switch this probe to http.get for broader compatibility.
package.json (1)
9-9: Verify Jest TypeScript configuration Ensure you have a Jest config (jest.config.ts/js or ajestsection in package.json) specifying ts-jest as preset/transform sonpx jestcan run TypeScript tests locally/CI.cypress/common/tools.ts (1)
3-7: LGTM!The port update and migration to
cy.iframeReady()align with the test infrastructure improvements across the codebase.cypress/e2e/copyView.cy.ts (1)
10-127: LGTM!The migration to
iframeBody().within()pattern is consistent throughout the file and aligns with the broader test infrastructure improvements.cypress/e2e/index.cy.ts (1)
13-108: LGTM!The migration to
iframeBody().within()pattern is consistent and aligns with the test infrastructure refactor.cypress/e2e/pull.cy.ts (1)
19-62: LGTM!The migration to
iframeBody().within()pattern is consistent with the test infrastructure improvements.cypress/e2e/language.cy.ts (1)
18-26: LGTM!The migration to
iframeBody().within()is clean and consistent with the broader test suite refactoring. The scoped interactions and use ofcy.gcy()selectors improve test reliability.cypress/e2e/pageSetup.cy.ts (1)
9-23: LGTM!The test correctly adopts the
iframeBody().within()pattern and usescy.gcy()selectors consistently. The flow validates page setup functionality as expected.cypress/e2e/push.cy.ts (1)
29-100: LGTM!The API-backed test setup is a significant improvement over static fixtures. The helper functions are well-structured, and the migration to
iframeBody().within()blocks is consistent across all test cases.Also applies to: 121-159, 165-193
src/ui/views/Push/UnresolvedConflicts.tsx (1)
1-47: LGTM!The component is well-structured and handles conflict display and override workflow cleanly. The use of index as key on Line 30 is acceptable here since the conflicts array is static during rendering and items don't reorder.
src/ui/views/Push/Push.tsx (2)
135-158: LGTM!The refactoring successfully delegates push logic to the
usePushhook while cleanly integrating conflict resolution. The override workflow is intuitive and the success message correctly accounts for unresolved conflicts.
208-219: LGTM!The conditional rendering of
UnresolvedConflictsis correctly placed after the success message, and theonOverridecallback properly triggers a re-submit with override enabled.cypress/e2e/stringDetails.cy.ts (1)
24-32: LGTM!The migration to
iframeBody().within()blocks is thorough and consistent. Removing the arbitrarycy.wait(500)improves test reliability by relying on Cypress's built-in retry logic.Also applies to: 50-56, 73-92, 111-118, 137-144, 162-167
cypress/e2e/settings.cy.ts (1)
10-38: LGTM!The test migration is consistent across all cases. The
iframeBody().within()pattern andcy.gcy()selectors improve test maintainability.Minor note: Lines 22 and 109 still use
cy.wait()with fixed delays. While not critical, consider relying on Cypress's automatic retry logic where possible.Also applies to: 53-77, 91-128, 142-160
cypress/support/commands.ts (1)
42-63: LGTM!The
iframeReadyimplementation properly waits for the iframe to be ready by checking bothreadyState=completeand the presence of#root. The simplifiediframeBodycommand is cleaner and more direct.src/ui/views/Push/usePush.ts (1)
145-145: Clarify undefined API version handling
WhenAPI_VERSIONisundefined,satisfiesMinimalVersionreturnsundefined, soundefined !== falseevaluates totrueand the new API path is used. Confirm this optimistic fallback is intended; otherwise, handleundefinedexplicitly (e.g.,if (satisfiesMinimalVersion("3.127.0", API_VERSION) === true)) to default to the old API on unknown versions.cypress/common/apiClient.ts (1)
6-11: Incorrect concern: projectId is always set before deletion.
createProjectWithClientinvokesclient.setProjectId(...), and every test’sbeforeEachuses it to initializeclientbeforeafterEachcallsdeleteProject, so there’s no risk of passing an undefinedprojectId.Likely an incorrect or invalid review comment.
| const project = await client.POST("/v2/projects", { | ||
| body: { | ||
| name, | ||
| organizationId: organizations.data!._embedded!.organizations![0].id, | ||
| languages: languages ?? languagesTestData, | ||
| icuPlaceholders: editOptions?.icuPlaceholders ?? true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Unsafe chained non-null assertions could cause runtime errors.
Line 32 chains multiple non-null assertions (organizations.data!._embedded!.organizations![0].id). If any of these properties are undefined or the organizations array is empty, this will throw a runtime error.
Apply this diff to add proper error handling:
const organizations = await client.GET("/v2/organizations");
+ if (!organizations.data?._embedded?.organizations?.length) {
+ throw new Error("No organizations found for admin user");
+ }
const { languages, ...editOptions } = options ?? {};
const project = await client.POST("/v2/projects", {
body: {
name,
- organizationId: organizations.data!._embedded!.organizations![0].id,
+ organizationId: organizations.data._embedded.organizations[0].id,
languages: languages ?? languagesTestData,
icuPlaceholders: editOptions?.icuPlaceholders ?? true,
},
});📝 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.
| const project = await client.POST("/v2/projects", { | |
| body: { | |
| name, | |
| organizationId: organizations.data!._embedded!.organizations![0].id, | |
| languages: languages ?? languagesTestData, | |
| icuPlaceholders: editOptions?.icuPlaceholders ?? true, | |
| }, | |
| }); | |
| const organizations = await client.GET("/v2/organizations"); | |
| if (!organizations.data?._embedded?.organizations?.length) { | |
| throw new Error("No organizations found for admin user"); | |
| } | |
| const { languages, ...editOptions } = options ?? {}; | |
| const project = await client.POST("/v2/projects", { | |
| body: { | |
| name, | |
| organizationId: organizations.data._embedded.organizations[0].id, | |
| languages: languages ?? languagesTestData, | |
| icuPlaceholders: editOptions?.icuPlaceholders ?? true, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In cypress/common/apiClient.ts around lines 29 to 36, the code uses unsafe
chained non-null assertions at
organizations.data!._embedded!.organizations![0].id which will throw if any
piece is missing or the array is empty; replace that chain with explicit runtime
checks: verify organizations.data exists, organizations.data._embedded exists,
organizations.data._embedded.organizations is an array with at least one
element, then extract the id; if any check fails, either throw a clear,
descriptive error or handle the case gracefully (return a useful error response
or use a safe fallback) so the POST body never receives an undefined path that
would crash at runtime.
| name, | ||
| organizationId: organizations.data!._embedded!.organizations![0].id, | ||
| languages: languages ?? languagesTestData, | ||
| icuPlaceholders: editOptions?.icuPlaceholders ?? true, |
There was a problem hiding this comment.
Potential configuration override: icuPlaceholders is set twice.
Line 34 defaults icuPlaceholders to true if not provided in editOptions, but line 47 unconditionally sets it to true again in the PUT request, potentially overriding a user-provided false value from editOptions.
Apply this diff to ensure editOptions takes precedence:
body: {
name,
organizationId: organizations.data!._embedded!.organizations![0].id,
languages: languages ?? languagesTestData,
- icuPlaceholders: editOptions?.icuPlaceholders ?? true,
+ icuPlaceholders: true,
},
});
client.setProjectId(project.data!.id);
await client.PUT("/v2/projects/{projectId}", {
params: {
path: {
projectId: client.getProjectId(),
},
},
body: {
- icuPlaceholders: true,
useNamespaces: true,
suggestionsMode: "DISABLED",
translationProtection: "NONE",
...editOptions,
+ icuPlaceholders: editOptions?.icuPlaceholders ?? true,
name,
},
});Also applies to: 47-47
🤖 Prompt for AI Agents
In cypress/common/apiClient.ts around lines 34 and 47, icuPlaceholders is
defaulted from editOptions at line 34 but then overwritten unconditionally to
true in the PUT request at line 47; change the payload at line 47 to use the
editOptions value (e.g., icuPlaceholders: editOptions?.icuPlaceholders ?? true)
instead of hardcoding true so a user-provided false is preserved.
| export async function createPak(client: ApiClient, scopes = DEFAULT_SCOPES) { | ||
| const apiKey = await client.POST("/v2/api-keys", { | ||
| body: { projectId: client.getProjectId(), scopes }, | ||
| }); | ||
|
|
||
| return apiKey.data!.key; | ||
| } |
There was a problem hiding this comment.
Verify API response data exists before non-null assertions.
Lines 81 and 89 use non-null assertions on API response data without verifying the response succeeded. If the API call fails or returns unexpected data, this will cause runtime errors.
Apply this diff to add proper error handling:
export async function createPak(client: ApiClient, scopes = DEFAULT_SCOPES) {
const apiKey = await client.POST("/v2/api-keys", {
body: { projectId: client.getProjectId(), scopes },
});
-
- return apiKey.data!.key;
+ if (!apiKey.data?.key) {
+ throw new Error("Failed to create API key: no key in response");
+ }
+ return apiKey.data.key;
}
export async function createPat(client: ApiClient) {
const apiKey = await client.POST("/v2/pats", {
body: { description: "e2e test pat" },
});
-
- return apiKey.data!.token;
+ if (!apiKey.data?.token) {
+ throw new Error("Failed to create PAT: no token in response");
+ }
+ return apiKey.data.token;
}Also applies to: 84-90
| import { components } from "@tginternal/client"; | ||
|
|
||
| export const languagesTestData: components["schemas"]["LanguageRequest"][] = [ |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the import source and type safety.
The code imports components from @tginternal/client, but the relevant code snippet shows that src/client/apiSchema.generated.ts defines components as Record<string, never>, which suggests the local schema may not have the expected types.
Please confirm:
- Does
@tginternal/clientprovide the correctcomponents["schemas"]["LanguageRequest"]type? - Should this import from a local generated schema instead?
- Are the OpenAPI types properly generated and up-to-date?
Run the following script to check the type definitions:
🏁 Script executed:
#!/bin/bash
# Check if @tginternal/client exports components with LanguageRequest schema
rg -A 10 'export.*components' node_modules/@tginternal/client --type ts 2>/dev/null || echo "@tginternal/client not found or no TypeScript files"
# Check local apiSchema for components definition
echo "--- Local apiSchema.generated.ts ---"
cat src/client/apiSchema.generated.ts | head -20
# Check if there are other generated API schema files
echo "--- Other API schema files ---"
fd 'apiSchema|api.*schema' --type fLength of output: 577
Ensure proper type definitions for LanguageRequest
@tginternal/clienthas no TS exports; switch to importingcomponentsfromsrc/client/apiSchema.generated.ts.- Regenerate your OpenAPI types (e.g. via openapi-typescript) so that
components["schemas"]["LanguageRequest"]is correctly defined.
🤖 Prompt for AI Agents
In cypress/common/languageTestData.ts (lines 1-3) the code imports components
from a package that does not export TypeScript OpenAPI types; replace the import
to pull components from your generated schema file
(src/client/apiSchema.generated.ts) and update the export type to use that
generated components["schemas"]["LanguageRequest"]; then regenerate the OpenAPI
TypeScript types (e.g., run your openapi-typescript generation command and
commit the updated src/client/apiSchema.generated.ts), update any paths/tsconfig
if necessary so the generated file is resolvable, and run the TypeScript build
to confirm LanguageRequest is correctly typed.
| beforeEach(() => { | ||
| return cy.wrap( | ||
| (async () => { | ||
| await createProject({ translationProtection: "PROTECT_REVIEWED" }); | ||
| await markTitleAsReviewed(); | ||
| })() | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Verify async operation completion in beforeEach.
The cy.wrap() pattern with an immediately-invoked async function may not guarantee proper synchronization. Cypress commands and promises don't mix cleanly this way.
Consider this pattern instead:
- beforeEach(() => {
- return cy.wrap(
- (async () => {
- await createProject({ translationProtection: "PROTECT_REVIEWED" });
- await markTitleAsReviewed();
- })()
- );
- });
+ beforeEach(() => {
+ cy.wrap(null).then(async () => {
+ await createProject({ translationProtection: "PROTECT_REVIEWED" });
+ await markTitleAsReviewed();
+ });
+ });Run this script to verify if other tests use similar patterns:
🏁 Script executed:
#!/bin/bash
# Search for cy.wrap with async IIFE pattern
rg -nP 'cy\.wrap\s*\(\s*\(async\s*\(' --type=tsLength of output: 0
Use cy.wrap().then() in beforeEach for async setup
In cypress/e2e/push.cy.ts (lines 16–23), replace:
beforeEach(() => {
return cy.wrap(
(async () => {
await createProject({ translationProtection: "PROTECT_REVIEWED" });
await markTitleAsReviewed();
})()
);
});with:
beforeEach(() => {
cy.wrap(null).then(async () => {
await createProject({ translationProtection: "PROTECT_REVIEWED" });
await markTitleAsReviewed();
});
});🤖 Prompt for AI Agents
In cypress/e2e/push.cy.ts around lines 16–23 the beforeEach currently returns
cy.wrap of an async IIFE which breaks Cypress command chaining; replace that
pattern by removing the return and using cy.wrap(null).then(async () => { await
createProject({ translationProtection: "PROTECT_REVIEWED" }); await
markTitleAsReviewed(); }); so the async setup runs inside a cy.then callback and
participates in the Cypress chain.
| it("returns true if version is lower", () => { | ||
| expect(satisfiesMinimalVersion("v1.2.3", "v1.2.2")).toEqual(false); | ||
| expect(satisfiesMinimalVersion("v1.2.3", "v1.2.1")).toEqual(false); | ||
| expect(satisfiesMinimalVersion("v1.2.3", "v1.1.4")).toEqual(false); | ||
| expect(satisfiesMinimalVersion("v1.2.3", "v0.0.0")).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
Fix misleading test description.
The test description states "returns true if version is lower" but all assertions expect false. This creates confusion about the test's intent.
Apply this diff:
- it("returns true if version is lower", () => {
+ it("returns false if version is lower", () => {
expect(satisfiesMinimalVersion("v1.2.3", "v1.2.2")).toEqual(false);
expect(satisfiesMinimalVersion("v1.2.3", "v1.2.1")).toEqual(false);
expect(satisfiesMinimalVersion("v1.2.3", "v1.1.4")).toEqual(false);
expect(satisfiesMinimalVersion("v1.2.3", "v0.0.0")).toEqual(false);
});📝 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.
| it("returns true if version is lower", () => { | |
| expect(satisfiesMinimalVersion("v1.2.3", "v1.2.2")).toEqual(false); | |
| expect(satisfiesMinimalVersion("v1.2.3", "v1.2.1")).toEqual(false); | |
| expect(satisfiesMinimalVersion("v1.2.3", "v1.1.4")).toEqual(false); | |
| expect(satisfiesMinimalVersion("v1.2.3", "v0.0.0")).toEqual(false); | |
| }); | |
| it("returns false if version is lower", () => { | |
| expect(satisfiesMinimalVersion("v1.2.3", "v1.2.2")).toEqual(false); | |
| expect(satisfiesMinimalVersion("v1.2.3", "v1.2.1")).toEqual(false); | |
| expect(satisfiesMinimalVersion("v1.2.3", "v1.1.4")).toEqual(false); | |
| expect(satisfiesMinimalVersion("v1.2.3", "v0.0.0")).toEqual(false); | |
| }); |
🤖 Prompt for AI Agents
In src/tools/satisfiesMinimalVersion.test.ts around lines 4 to 9, the test title
"returns true if version is lower" is misleading because all assertions expect
false; update the it(...) description to accurately state the expected behavior
(e.g., "returns false if version is lower" or "returns false when provided
version is lower than minimal") so the test name matches the assertions.
| .container { | ||
| display: grid; | ||
| color: var(--figma-color-text-warning); | ||
| margin: 10px 0px |
There was a problem hiding this comment.
Add missing semicolon.
The margin declaration is missing a trailing semicolon, which could cause CSS parsing issues in some browsers or build tools.
Apply this diff:
- margin: 10px 0px
+ margin: 10px 0px;Additionally, consider using 0 instead of 0px for zero values (CSS best practice):
- margin: 10px 0px;
+ margin: 10px 0;📝 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.
| margin: 10px 0px | |
| margin: 10px 0; |
🤖 Prompt for AI Agents
In src/ui/views/Push/UnresolvedConflicts.css around line 4 the CSS rule "margin:
10px 0px" is missing a trailing semicolon; add the semicolon so the line reads
"margin: 10px 0px;" and optionally change "0px" to "0" for best practice (i.e.,
"margin: 10px 0;") to ensure proper parsing and cleaner CSS.
| const imageBlob = new Blob([screenshot.image.buffer as ArrayBuffer], { | ||
| type: "image/png", | ||
| }); | ||
| const location = `figma-${screenshot.info.id}`; | ||
| const infoBlob = new Blob([JSON.stringify({ location })], { | ||
| type: "application/json", | ||
| }); | ||
| const data = await uploadImage.mutateAsync({ | ||
| content: { | ||
| "multipart/form-data": { | ||
| image: imageBlob as any, | ||
| info: infoBlob as any, | ||
| }, | ||
| }, | ||
| }); | ||
| screenshotsMap.set(screenshot, data.id); |
There was a problem hiding this comment.
Verify ArrayBuffer cast and add type safety for multipart form data.
Line 78 casts screenshot.image.buffer to ArrayBuffer without validation, and lines 88-89 use as any type assertions, which bypass TypeScript's type safety.
Apply this diff to add proper type validation:
for (const [i, screenshot] of requiredScreenshots.entries()) {
setLoadingStatus(
`Uploading images (${i + 1}/${requiredScreenshots.length})`
);
+ if (!(screenshot.image.buffer instanceof ArrayBuffer)) {
+ throw new Error(`Screenshot image buffer is not an ArrayBuffer`);
+ }
const imageBlob = new Blob([screenshot.image.buffer as ArrayBuffer], {
type: "image/png",
});
const location = `figma-${screenshot.info.id}`;
const infoBlob = new Blob([JSON.stringify({ location })], {
type: "application/json",
});
const data = await uploadImage.mutateAsync({
content: {
"multipart/form-data": {
- image: imageBlob as any,
- info: infoBlob as any,
+ image: imageBlob as unknown as Blob,
+ info: infoBlob as unknown as Blob,
},
},
});
screenshotsMap.set(screenshot, data.id);
}📝 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.
| const imageBlob = new Blob([screenshot.image.buffer as ArrayBuffer], { | |
| type: "image/png", | |
| }); | |
| const location = `figma-${screenshot.info.id}`; | |
| const infoBlob = new Blob([JSON.stringify({ location })], { | |
| type: "application/json", | |
| }); | |
| const data = await uploadImage.mutateAsync({ | |
| content: { | |
| "multipart/form-data": { | |
| image: imageBlob as any, | |
| info: infoBlob as any, | |
| }, | |
| }, | |
| }); | |
| screenshotsMap.set(screenshot, data.id); | |
| for (const [i, screenshot] of requiredScreenshots.entries()) { | |
| setLoadingStatus( | |
| `Uploading images (${i + 1}/${requiredScreenshots.length})` | |
| ); | |
| if (!(screenshot.image.buffer instanceof ArrayBuffer)) { | |
| throw new Error(`Screenshot image buffer is not an ArrayBuffer`); | |
| } | |
| const imageBlob = new Blob([screenshot.image.buffer as ArrayBuffer], { | |
| type: "image/png", | |
| }); | |
| const location = `figma-${screenshot.info.id}`; | |
| const infoBlob = new Blob([JSON.stringify({ location })], { | |
| type: "application/json", | |
| }); | |
| const data = await uploadImage.mutateAsync({ | |
| content: { | |
| "multipart/form-data": { | |
| image: imageBlob as unknown as Blob, | |
| info: infoBlob as unknown as Blob, | |
| }, | |
| }, | |
| }); | |
| screenshotsMap.set(screenshot, data.id); | |
| } |
| result.push({ | ||
| text: item.newValue, | ||
| uploadedImageId: screenshotsMap.get(screenshot)!, |
There was a problem hiding this comment.
Non-null assertion could fail if screenshot was not uploaded.
Line 106 uses a non-null assertion on screenshotsMap.get(screenshot)!, but if the screenshot wasn't successfully uploaded (e.g., upload failed silently or was skipped), this will return undefined and cause a runtime error.
Apply this diff to add proper error handling:
result.push({
text: item.newValue,
- uploadedImageId: screenshotsMap.get(screenshot)!,
+ uploadedImageId: screenshotsMap.get(screenshot) ?? (() => {
+ throw new Error(`Screenshot not found in upload map for key: ${item.key}`);
+ })(),
positions: relevantNodes.map(({ x, y, width, height }) => {
return {
x,
y,
width,
height,
};
}),
});📝 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.
| result.push({ | |
| text: item.newValue, | |
| uploadedImageId: screenshotsMap.get(screenshot)!, | |
| result.push({ | |
| text: item.newValue, | |
| uploadedImageId: screenshotsMap.get(screenshot) ?? (() => { | |
| throw new Error(`Screenshot not found in upload map for key: ${item.key}`); | |
| })(), | |
| positions: relevantNodes.map(({ x, y, width, height }) => { | |
| return { | |
| x, | |
| y, | |
| width, | |
| height, | |
| }; | |
| }), | |
| }); |
🤖 Prompt for AI Agents
In src/ui/views/Push/usePush.ts around lines 104 to 106, the code uses a
non-null assertion screenshotsMap.get(screenshot)! which can throw if the
screenshot key is missing; replace the assertion with a safe check: retrieve the
value into a variable, if it's undefined handle it (e.g., log an error/warning
with context, skip adding this result or provide a safe fallback id, or surface
the error to the caller) and only push the object when uploadedImageId is
present; ensure control flow avoids dereferencing undefined and consider
returning/reporting a failure for the overall push operation if missing uploads
are critical.
| if (tolgeeConfig?.updateScreenshots ?? true) { | ||
| for (const screenshot of changes.screenshots.values()) { | ||
| const relatedKeys = screenshot.keys | ||
| .map((data) => ({ | ||
| keyName: data.key, | ||
| namespace: data.ns || undefined, | ||
| })) | ||
| .slice(0, 100); | ||
| await bigMeta.mutateAsync({ | ||
| content: { | ||
| "application/json": { | ||
| relatedKeysInOrder: relatedKeys, | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Hard limit of 100 keys per screenshot may silently truncate data.
Line 208 applies .slice(0, 100) to limit the number of related keys per screenshot. If a screenshot has more than 100 keys, the excess keys will be silently dropped without warning or error.
Consider adding a warning when keys are truncated:
if (tolgeeConfig?.updateScreenshots ?? true) {
for (const screenshot of changes.screenshots.values()) {
const relatedKeys = screenshot.keys
.map((data) => ({
keyName: data.key,
namespace: data.ns || undefined,
}))
.slice(0, 100);
+ if (screenshot.keys.length > 100) {
+ console.warn(
+ `Screenshot has ${screenshot.keys.length} keys, but only 100 will be associated due to API limits`
+ );
+ }
await bigMeta.mutateAsync({
content: {
"application/json": {
relatedKeysInOrder: relatedKeys,
},
},
});
}
}📝 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.
| if (tolgeeConfig?.updateScreenshots ?? true) { | |
| for (const screenshot of changes.screenshots.values()) { | |
| const relatedKeys = screenshot.keys | |
| .map((data) => ({ | |
| keyName: data.key, | |
| namespace: data.ns || undefined, | |
| })) | |
| .slice(0, 100); | |
| await bigMeta.mutateAsync({ | |
| content: { | |
| "application/json": { | |
| relatedKeysInOrder: relatedKeys, | |
| }, | |
| }, | |
| }); | |
| } | |
| } | |
| if (tolgeeConfig?.updateScreenshots ?? true) { | |
| for (const screenshot of changes.screenshots.values()) { | |
| const relatedKeys = screenshot.keys | |
| .map((data) => ({ | |
| keyName: data.key, | |
| namespace: data.ns || undefined, | |
| })) | |
| .slice(0, 100); | |
| if (screenshot.keys.length > 100) { | |
| console.warn( | |
| `Screenshot has ${screenshot.keys.length} keys, but only 100 will be associated due to API limits` | |
| ); | |
| } | |
| await bigMeta.mutateAsync({ | |
| content: { | |
| "application/json": { | |
| relatedKeysInOrder: relatedKeys, | |
| }, | |
| }, | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/ui/views/Push/usePush.ts around lines 201 to 217 the code blindly
truncates relatedKeys with .slice(0, 100) which can silently drop data; detect
when screenshot.keys.length > 100 and surface that fact (e.g. add a warning log
via the existing logger or push a warning into bigMeta content) before mutating,
or include an extra boolean/field in the mutation payload like keysTruncated:
true and truncatedCount so the server/client can handle it; ensure the change
preserves current behavior when ≤100 keys but emits the warning/metadata when
truncation occurs.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores