ENG-430 Add access-token table #196
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughA new database table named "access-token" has been introduced, along with its schema, migration script, and TypeScript type definitions. Configuration files were updated to include the new schema. The table features several columns, indexes, a check constraint, and grants specific privileges to certain roles. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
Client->>Database: Insert new access-token (with code/state)
Database-->>Client: Insert success/failure (check constraint enforced)
Client->>Database: Query access-token by code/state/access-token
Database-->>Client: Return matching access-token rows
Client->>Database: Update access-token fields
Database-->>Client: Update success/failure
Client->>Database: Delete access-token
Database-->>Client: Delete success/failure
Poem
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
![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
🧹 Nitpick comments (2)
packages/database/types.gen.ts (1)
12-35: Consider reviewing naming conventions for database identifiers.The TypeScript types are correctly structured and consistent with the database schema. However, consider these naming convention improvements:
- Table name
"access-token"uses hyphens, which is less common than underscores in database naming- Column name
"access-token"matches the table name, which could cause confusionConsider renaming to improve clarity:
- "access-token": { + "access_token": { Row: { - "access-token": string + token: stringpackages/database/supabase/schemas/access_token.sql (1)
12-15: Optimize indexing strategy for nullable columns.Creating indexes on nullable columns (
code,state) may have limited effectiveness depending on data distribution. Consider:
- Partial indexes for better performance:
-create index access_token_code_idx on "access-token" (code); -create index access_token_state_idx on "access-token" (state); +create index access_token_code_idx on "access-token" (code) where code is not null; +create index access_token_state_idx on "access-token" (state) where state is not null;
- Consider composite indexes if you frequently query by combinations of these fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/database/supabase/config.toml(1 hunks)packages/database/supabase/migrations/20250609051037_create_access_table.sql(1 hunks)packages/database/supabase/schemas/access_token.sql(1 hunks)packages/database/types.gen.ts(1 hunks)
🔇 Additional comments (2)
packages/database/supabase/config.toml (1)
62-62: LGTM! Schema path correctly added.The new access_token.sql schema file is properly integrated into the migration configuration following the existing pattern.
packages/database/supabase/migrations/20250609051037_create_access_table.sql (1)
22-24: Good practice: Proper constraint validation pattern.The migration correctly uses the two-step constraint creation pattern (add as
not valid, thenvalidate), which is a PostgreSQL best practice for large tables as it avoids blocking writes during validation.
packages/database/supabase/migrations/20250609051037_create_access_table.sql
Outdated
Show resolved
Hide resolved
packages/database/supabase/migrations/20250609051037_create_access_table.sql
Outdated
Show resolved
Hide resolved
|
Ok, a few questions: Are access tokens unique for spaces? In which case should the Id not be the foreign key to the state instead of an independent identity? |
|
Another issue, on me: I'm maintaining the linkml/svg with tooling that I did not include in the build, because I did not want to impose it to everyone to install. As long as I was mostly manipulating the tables, not an issue. Do you have an opinion on whether I should include it or keep maintaining it in the background? |
For this use case it is authorizing a GitHub App to act on users behalf to sync a Discourse Node to a GitHub Issue.
Not necessarily, they could possibly have a PlatformAccount in the future. Other uses for this table might be tied to spaces. In either case, we'll address that when if/when it shows up.
Both will be used to query the table and return
I don't see the need currently.
I don't think so.
Maybe. We'll cross that bridge when we come to it. |
Any feature in the repo should not be dependent on a single dev. If we keep it, instructions need to be included. Ideally this would be automated by an |
This comment was marked as spam.
This comment was marked as spam.
mdroidian
force-pushed
the
eng-430-add-access-token-table
branch
from
fbd47d8 to
f5e2dd3
Compare
…r consistency. Update permissions and indexes in SQL migration files.
|
@maparent added |
|
Still trying to understand some of the details of part of the flow:
Anyhow the reason I'm asking for details, besides clarity, is I'm trying to see if there's a primary key in there besides the numeric identity. I suspect |
That's why I have not done it yet, LinkML is a python package, no NPM version. I could probably make it run using minimal tooling installs, but I want an ok for installing non-npm tools. Same with plantuml, there's a npm version but it's quite outdated. Npm is a bottleneck. |
|
Other than the primary key issue, the rest looks good. |
…, remove id, and update constraints.
|
Reviewed live by @maparent |
To be used for GitHub Apps initially.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores