Conversation
This reverts commit ef85bdb.
|
@duck-master is attempting to deploy a commit to the polylove Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a CI workflow scoped to the Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub
participant CI_Workflow
participant Runner
participant Repo
GitHub->>CI_Workflow: Trigger on PR/push (files: common/** excluding .md/.txt)
CI_Workflow->>Runner: Start selected jobs (test, eslint, prettier, typecheck)
Runner->>Repo: checkout
Runner->>Runner: setup Node.js, install Yarn
Runner->>Repo: install dependencies (yarn)
alt test
Runner->>Repo: run `yarn test`
else eslint
Runner->>Repo: run `yarn eslint .`
else prettier
Runner->>Repo: run `yarn prettier . --check --ignore-unknown`
else typecheck
Runner->>Repo: run `yarn tsc . --noEmit --strict`
end
Runner-->>CI_Workflow: report job results
CI_Workflow-->>GitHub: update status / PR checks
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
common/src/util/parse.ts (1)
55-56: Ternary precedence bug produces wrong output
'[embed]' + node.attrs.src ? ...is parsed as('[embed]' + node.attrs.src) ? ..., so the condition is always truthy (even whensrcis undefined) and the[embed]prefix is lost. Wrap the ternary operand in parentheses:- '[embed]' + node.attrs.src ? `(${node.attrs.src})` : '', + '[embed]' + (node.attrs.src ? `(${node.attrs.src})` : ''),common/src/love/lover.ts (1)
7-10: Handle potential Supabase query errors
run()may return{ error, data }. The current implementation ignoreserrorand blindly accessesdata[0], which will throw ifdataisnull. Propagate or handle the error to avoid runtime failures.- const res = await run(db.from('lovers').select('*').eq('user_id', userId)) - return res.data[0] + const { data, error } = await run( + db.from('lovers').select('*').eq('user_id', userId) + ) + if (error) throw error + return data?.[0] ?? null
🧹 Nitpick comments (5)
common/src/util/parse.ts (1)
5-5: Remove commented-out import instead of leaving dead codeThe
Nodeimport was unused and correctly removed, but leaving it commented clutters the file. Delete the line entirely to keep the import section clean.- //Node,common/src/love/lover.ts (1)
3-3: Delete commented-outDatabaseimportIf the type is no longer required, remove the comment entirely; if it is required elsewhere, restore the proper import. Commented imports add noise and risk drifting out of date.
-//import { Database } from 'common/supabase/schema'common/src/util/json.ts (1)
3-5: Parsing''always throws – consider'null'insteadUsing
''as the default causesJSON.parseto throw every time the input is falsy, relying on the catch block for normal control flow. Parsing'null'avoids the exception and is slightly cheaper.- return JSON.parse(json ?? '') + return JSON.parse(json ?? 'null')common/src/api/schema.ts (1)
39-39: Follow TypeScript naming conventions for type aliases.Type aliases should use PascalCase according to TypeScript conventions.
-type apiTypeCheck = { [x: string]: APIGenericSchema } +type APITypeCheck = { [x: string]: APIGenericSchema }And update the usage:
-export const API: apiTypeCheck = { +export const API: APITypeCheck = {.github/workflows/ci-common.yml (1)
15-27: Consider optimizing the duplicated setup steps.All four jobs repeat identical setup steps (checkout, node setup, yarn install, dependencies). This creates maintenance overhead and longer CI times.
Consider using a composite action or job matrix to reduce duplication:
+ setup: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: '18' + cache: 'yarn' + - run: npm install -g yarn + - run: yarn install --frozen-lockfile + test: runs-on: ubuntu-latest + needs: setup steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 - with: - cache: 'yarn' - - name: Install yarn - run: npm install -g yarn - - name: Install dependencies - run: yarn - name: Run tests run: yarn testAlso consider:
- Pinning Node.js version (e.g.,
node-version: '18')- Using
--frozen-lockfilefor reproducible buildsAlso applies to: 28-40, 41-53, 54-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/ci-common.yml(1 hunks)common/eslint.config.mjs(1 hunks)common/jest.config.js(1 hunks)common/package.json(2 hunks)common/src/api/schema.ts(2 hunks)common/src/api/websocket-client.ts(1 hunks)common/src/api/zod-types.ts(0 hunks)common/src/envs/is-prod.ts(1 hunks)common/src/love/lover.ts(1 hunks)common/src/secrets.ts(1 hunks)common/src/socials.test.ts(4 hunks)common/src/util/algos.ts(1 hunks)common/src/util/api.ts(1 hunks)common/src/util/json.ts(1 hunks)common/src/util/parse.ts(1 hunks)common/src/util/random.ts(1 hunks)common/src/util/string.ts(1 hunks)common/tsconfig.js(1 hunks)common/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- common/src/api/zod-types.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
common/src/socials.test.ts (1)
common/src/socials.ts (2)
strip(22-23)getSocialUrl(72-73)
common/src/util/api.ts (1)
common/src/api/utils.ts (1)
getApiUrl(34-39)
🔇 Additional comments (16)
common/src/util/random.ts (1)
54-55: LGTM – newline onlyOnly a trailing newline was added; no functional impact.
common/src/api/websocket-client.ts (1)
89-102: Good cleanup of unused parameterRemoving the unused
_evparameter silences ESLint without altering behaviour.common/src/api/schema.ts (1)
41-479: LGTM! Excellent refactoring to address ESLint warnings.The conversion from an unused variable to a proper type alias effectively resolves the ESLint warning while maintaining type safety. The
as constassertion at the end properly preserves literal types for better type inference.common/tsconfig.js (1)
1-22: tsconfig.js and tsconfig.json are in sync ✅The key compiler options (
target,module,strict) in common/tsconfig.js match those in common/tsconfig.json. This workaround is solid for Jest/Prettier compatibility and aligns with your PR objectives.• No mismatches found in key settings.
• Composite project configuration and path mappings look correct.Suggestion: Add a brief comment at the top of common/tsconfig.js (or a CI validation step) pointing to common/tsconfig.json to remind future maintainers to keep them aligned.
common/src/util/string.ts (1)
7-8: LGTM! Good practice to ensure consistent file endings.Adding the newline at the end of the file follows standard formatting conventions and helps maintain consistency across the codebase.
common/src/secrets.ts (1)
94-94: LGTM! Clean code improvement by removing unused error parameter.Since the caught error was not being used (the catch block throws a new error with a custom message), removing the unused parameter follows best practices and likely addresses ESLint warnings.
common/tsconfig.json (1)
16-18: LGTM! Beneficial TypeScript configuration improvements.The addition of
resolveJsonModuleandesModuleInteropcompiler options enhances module compatibility and aligns with the ES module migration described in the PR objectives.common/src/util/api.ts (1)
18-18: Approve type safety change but verify numeric path formattingThe
String(path)cast correctly ensuresgetApiUrlalways receives a string, covering bothstringandnumberinputs without runtime errors.•
APIPathis defined askeyof typeof APIin common/src/api/schema.ts:481
• No instances of numeric literals passed totypedAPICallwere found—ensure any dynamic numeric segments (e.g., IDs) don’t require custom formatting (zero-padding, prefixes, etc.) beyond the defaultString()conversion.common/src/envs/is-prod.ts (1)
1-2: LGTM! Proper ES module migration.Converting from dynamic
requireto staticimportaligns with the ES module migration and improves static analysis capabilities.common/src/socials.test.ts (1)
26-28: LGTM! Improved test readability.The multi-line formatting for long expected values enhances readability without changing test logic.
Also applies to: 37-39, 48-53, 59-61, 63-65, 74-76, 77-79, 80-82, 83-85, 86-88, 89-91, 92-94, 99-101, 105-107, 108-110
common/package.json (1)
3-3: LGTM! Enables ES module support.Setting
"type": "module"properly configures the package for ES module usage, aligning with the broader migration.common/jest.config.js (2)
5-12: Export syntax and configuration look correct.The conversion from
module.exportstoexport defaultis properly implemented, and the usage oftsconfig.compilerOptions.pathsaligns with the imported module structure.
2-3: tsconfig.js presence and structure verifiedThe
common/tsconfig.jsfile exists and correctly exports a default object withcompilerOptions.paths. The originalcommon/tsconfig.jsonremains intact for reference. No further changes needed—LGTM.common/eslint.config.mjs (2)
1-10: Modern ESLint flat config setup looks good.The imports, file patterns, and use of recommended configurations are appropriate. The combination of browser and node globals makes sense for a common library that might run in different environments.
11-17: Investigate the no-unused-expressions rule error rather than disabling itThe comment in common/eslint.config.mjs (around lines 13–17) indicates a “Cannot read properties of undefined” failure coming from the @typescript-eslint/no-unused-expressions rule—this points to a configuration or version mismatch, not a style choice. Please reproduce the error in your local environment, identify whether it’s caused by parserOptions, plugin loading, or a plugin bug, and restore the rule to “error” once the root cause is addressed.
• File: common/eslint.config.mjs
Lines: ~13–17 – don’t leave@typescript-eslint/no-unused-expressionsturned off; reproduce the error locally and update your ESLint config or plugin version to fix it
• Steps you might try:
– Ensureparser,parserOptions, andpluginsare correctly set for TypeScript files
– Confirm you’re on a compatible @typescript-eslint version or upgrade/downgrade as needed
– Run ESLint directly in a local shell (not via Yarn’s sandbox) to see the full stack trace and pinpoint the failing code path.github/workflows/ci-common.yml (1)
3-9: Path-based triggering is well configured.The workflow correctly triggers only for changes in the
commondirectory while excluding documentation files. This ensures efficient CI runs.
| "zod": "3.21.4" | ||
| }, | ||
| "devDependencies": { | ||
| "@eslint/js": "9.32.0", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify ESLint dependency versions for compatibility and security.
The ESLint-related dependencies look reasonable, but ensure these versions are compatible with each other and free from known vulnerabilities.
Also applies to: 33-34, 37-38
🏁 Script executed:
#!/bin/bash
# Description: Check for compatibility issues and security advisories for ESLint dependencies
# Check for npm audit issues with the new dependencies
echo "Checking for security vulnerabilities..."
npm audit --package-lock-only --audit-level=moderate
# Verify ESLint and typescript-eslint compatibility
echo "Checking ESLint version compatibility..."
curl -s https://registry.npmjs.org/typescript-eslint/8.39.0 | jq '.peerDependencies.eslint'Length of output: 697
🏁 Script executed:
#!/bin/bash
# List all ESLint-related devDependencies and their versions
echo "ESLint-related devDependencies in common/package.json:"
jq -r '.devDependencies
| to_entries[]
| select(.key | test("eslint"))
| "\(.key)@\(.value)"'
echo
# For each, fetch its peerDependencies.eslint requirement
echo "peerDependencies.eslint for each:"
for dep in $(jq -r '.devDependencies | keys[] | select(test("eslint"))' common/package.json); do
version=$(jq -r ".devDependencies[\"$dep\"]" common/package.json)
requirement=$(curl -s "https://registry.npmjs.org/$dep/$version" \
| jq -r '.peerDependencies.eslint // "none"')
echo "$dep@$version → $requirement"
doneLength of output: 1505
ESLint dependencies compatibility confirmed; add a lockfile for security auditing
- common/package.json devDependencies:
- eslint@9.32.0
- @eslint/js@9.32.0
- typescript-eslint@8.39.0 → requires
eslint^8.57.0 || ^9.0.0(9.32.0 satisfies this)
- No other ESLint-related peer dependencies detected.
- A lockfile is missing, so
npm auditcannot run. Please generate a package-lock.json (e.g.npm i --package-lock-only) and rerunnpm audit --audit-level=moderateto surface any vulnerabilities.
Also applies to: lines 33–34, 37–38
🤖 Prompt for AI Agents
In common/package.json around lines 29, 33-34, and 37-38, ESLint-related
dependencies versions are compatible, but there is no lockfile present. Generate
a package-lock.json by running `npm i --package-lock-only` in the common
directory, then rerun `npm audit --audit-level=moderate` to identify any
vulnerabilities and ensure security auditing is enabled.
1 participant
This is the second in my PR series (previously: this one). This was a lot more annoying (and also more sprawling!) than the first for several reasons:
jest.config.jsto useimportstyle imports.with { type: "json" }statement when importingtsconfig.jsondirectly, Jest would fail, but Prettier would succeed. However, if I did add this statement, Jest would succeed, but Prettier would fail. So neither worked. Instead, as a hacky workaround, I created a file calledtsconfig.jswhich exports the exact same thing astsconfig.json, and imported that instead. If there's a less hacky solution for this, I'd love to know._apiTypeCheckvariable as unused. I got around this by making it atypedeclaration.typedAPICallincommon/src/util/api.ts. Namely, one input,path, could conceivably be either a string or a number, butgetApiUrlonly expects a string input. I fixed this by stringifying it using theString()function. That said, this could conceivably lead to bugs ifpathdoes in fact happen to be a number and the app is expecting us to stringify it a different way.common/src/util/algos.ts, there is a while true loop, which I fixed by limitingito 1000002. However, you could write a cleaner version that explicitly frames the process as a for loop rather than a while loop. I considered this but decided against it since that would be a bigger change that would require me to write a new set of automated tests. (By the way, huge kudos for even setting up jest to begin with!)Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Documentation