feat: implement runtime and build time env variables for remote access#1316
feat: implement runtime and build time env variables for remote access#1316Crunchyman-ralph merged 7 commits intonextfrom
Conversation
- add search for brief selection
|
WalkthroughPrefer runtime TM_* environment variables over build-time TM_PUBLIC_* fallbacks; add interactive CLI post-login context setup with search-driven brief selection; enrich Briefs with document metadata; add storage getType() methods; suppress repeated credential-expiration warnings; defer dev CLI import until dotenv loads. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI_Auth as apps/cli auth.command
participant CLI_Context as apps/cli context.command
participant OrgSvc as tm-core organization.service
User->>CLI_Auth: start interactive login
CLI_Auth->>CLI_Auth: performInteractiveAuth()
CLI_Auth->>User: prompt & authenticate
alt auth success
CLI_Auth->>CLI_Context: setupContextInteractive()
CLI_Context->>User: search/select organization
User-->>CLI_Context: choose org
CLI_Context->>OrgSvc: getBriefs(orgId)
OrgSvc-->>CLI_Context: briefs (may include document metadata)
CLI_Context->>User: search/select brief (or skip)
User-->>CLI_Context: brief chosen / skip
CLI_Context->>CLI_Auth: return setup result
CLI_Auth->>User: login + context setup complete
else auth failure
CLI_Auth->>User: login failed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)apps/cli/src/commands/auth.command.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
apps/cli/package.json(1 hunks)apps/cli/src/commands/auth.command.ts(2 hunks)apps/cli/src/commands/context.command.ts(7 hunks)packages/tm-core/src/auth/config.ts(1 hunks)packages/tm-core/src/auth/credential-store.ts(3 hunks)packages/tm-core/src/clients/supabase-client.ts(1 hunks)packages/tm-core/src/services/export.service.ts(1 hunks)packages/tm-core/src/services/organization.service.ts(5 hunks)packages/tm-core/src/storage/api-storage.ts(1 hunks)packages/tm-core/src/storage/file-storage/file-storage.ts(1 hunks)packages/tm-core/src/storage/storage-factory.ts(2 hunks)scripts/dev.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
📄 CodeRabbit inference engine (.cursor/rules/tests.mdc)
**/*.js: Declare and initialize global variables at the top of modules to avoid hoisting issues.
Use proper function declarations to avoid hoisting issues and initialize variables before they are referenced.
Do not reference variables before their declaration in module scope.
Use dynamic imports (import()) to avoid initialization order issues in modules.
Files:
scripts/dev.js
🧠 Learnings (2)
📚 Learning: 2025-10-15T14:43:40.383Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.383Z
Learning: In the claude-task-master codebase, `getCredentials()` in `credential-store.ts` defaults to `allowExpired: true` to enable Supabase refresh flows. The Supabase client automatically handles token refresh when credentials are expired but have a valid refresh token. The `SupabaseSessionStorage` updates the credentials after Supabase performs the refresh. This is an intentional design pattern where the credential store is passive storage and Supabase manages the active token lifecycle.
Applied to files:
packages/tm-core/src/auth/credential-store.ts
📚 Learning: 2025-09-02T21:51:41.383Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1178
File: packages/tm-core/src/auth/config.ts:0-0
Timestamp: 2025-09-02T21:51:41.383Z
Learning: In packages/tm-core/src/auth/config.ts, the BASE_DOMAIN configuration intentionally does not include runtime environment variable fallbacks like TM_BASE_DOMAIN or HAMSTER_BASE_URL. The maintainers prefer to keep these capabilities "hush hush" and undocumented, using only the build-time TM_PUBLIC_BASE_DOMAIN and the default value.
Applied to files:
packages/tm-core/src/auth/config.tspackages/tm-core/src/storage/storage-factory.ts
🧬 Code graph analysis (3)
apps/cli/src/commands/auth.command.ts (1)
apps/cli/src/commands/context.command.ts (1)
ContextCommand(32-785)
packages/tm-core/src/auth/config.ts (1)
packages/tm-core/src/auth/types.ts (1)
AuthConfig(39-43)
packages/tm-core/src/clients/supabase-client.ts (1)
packages/tm-core/src/auth/types.ts (1)
AuthenticationError(88-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (19)
packages/tm-core/src/storage/api-storage.ts (1)
115-120: LGTM!The
getType()method provides a clean way to identify storage type at runtime, complementingFileStorage.getType()for consistent type identification across storage implementations.packages/tm-core/src/storage/file-storage/file-storage.ts (1)
47-52: LGTM!The
getType()method provides consistent storage type identification, mirroring the implementation inApiStorage.packages/tm-core/src/storage/storage-factory.ts (2)
85-86: Runtime variable precedence established.The change prioritizes
TM_BASE_DOMAIN(runtime) overTM_PUBLIC_BASE_DOMAIN(build-time), establishing a clear precedence hierarchy that aligns with the PR's objective to support runtime environment variables for remote access.Note: This represents a shift from the previous approach documented in learnings, where runtime variables like
TM_BASE_DOMAINwere intentionally kept undocumented. This PR appears to formalize runtime variable support across the codebase.Based on learnings
115-117: Complete fallback chain maintains backward compatibility.The three-tier fallback (
TM_BASE_DOMAIN→TM_PUBLIC_BASE_DOMAIN→ hard-coded default) ensures backward compatibility while enabling runtime configuration.packages/tm-core/src/clients/supabase-client.ts (1)
32-42: Consistent runtime-first precedence with clear error messaging.The Supabase client configuration now follows the same runtime-first pattern as other configuration modules, with
TM_SUPABASE_URLandTM_SUPABASE_ANON_KEYtaking precedence over their build-time counterparts. The error message helpfully references both sets of environment variables.scripts/dev.js (1)
12-22: Proper initialization order with dynamic import.The reordering ensures environment variables load before any module-level code executes by:
- Moving
dotenv.config()before other imports (line 14)- Using dynamic
import()with await (line 22) to defer module loadingThis follows the coding guidelines for JavaScript files to avoid initialization order issues and ensures environment variables are available when modules initialize.
As per coding guidelines
packages/tm-core/src/auth/credential-store.ts (1)
27-28: Effective warning suppression prevents log spam.The instance-level flag
hasWarnedAboutMissingExpirationsuppresses repeated warnings within a single run while still allowing the warning to reappear after new credentials are saved. This is particularly useful during auth flows wheregetCredentials()may be called multiple times withallowExpired: trueto enable Supabase refresh flows.Based on learnings
Also applies to: 89-93, 184-185
packages/tm-core/src/services/organization.service.ts (3)
30-35: Brief interface enriched with document details.The optional
documentfield extends the Brief interface with related document information, maintaining backward compatibility while enabling richer brief data.
180-185: Database join enriches brief list with document data.The
getBriefsquery now joins document fields and materializesid,document_name, andtitle. The mapping correctly handles cases where no related document exists.Also applies to: 210-217
245-251: Single brief retrieval includes document description.The
getBriefquery extends the document join to includedescription, providing more comprehensive document data for individual brief lookups compared to the list view.Also applies to: 280-288
packages/tm-core/src/services/export.service.ts (1)
361-366: Export API endpoint resolution aligns with broader configuration changes.The API endpoint resolution now prioritizes
TM_BASE_DOMAINoverTM_PUBLIC_BASE_DOMAIN, consistent with the runtime-first precedence established across other configuration modules in this PR. The conditional logic and URL construction cleanly utilize the resolved endpoint.packages/tm-core/src/auth/config.ts (2)
10-16: Verify alignment with project policy on runtime environment variables.Based on learnings, the maintainers previously preferred to keep runtime environment variable capabilities (TM_BASE_DOMAIN, HAMSTER_BASE_URL) "hush hush" and undocumented, relying only on build-time TM_PUBLIC_BASE_DOMAIN. This PR explicitly documents and prioritizes runtime variables, which may conflict with that policy.
Based on learnings.
24-24: Non-null assertion risks runtime error when both environment variables are undefined.The non-null assertion
BASE_DOMAIN!will cause TypeScript to assumeBASE_DOMAINis never undefined, but if bothTM_BASE_DOMAINandTM_PUBLIC_BASE_DOMAINare undefined at runtime, this will result inbaseUrl: undefined, which violates theAuthConfiginterface requirement thatbaseUrl: string.Apply this diff to provide a safe fallback:
- baseUrl: BASE_DOMAIN!, + baseUrl: BASE_DOMAIN || 'https://tryhamster.com',Or update the type to allow undefined and handle it at call sites:
export interface AuthConfig { - baseUrl: string; + baseUrl: string | undefined; configDir: string; configFile: string; }⛔ Skipped due to learnings
Learnt from: Crunchyman-ralph PR: eyaltoledano/claude-task-master#1178 File: packages/tm-core/src/auth/config.ts:0-0 Timestamp: 2025-09-02T21:51:41.383Z Learning: In packages/tm-core/src/auth/config.ts, the BASE_DOMAIN configuration intentionally does not include runtime environment variable fallbacks like TM_BASE_DOMAIN or HAMSTER_BASE_URL. The maintainers prefer to keep these capabilities "hush hush" and undocumented, using only the build-time TM_PUBLIC_BASE_DOMAIN and the default value.apps/cli/package.json (1)
25-25: LGTM!The addition of
@inquirer/searchaligns with the search-based brief selection feature introduced inapps/cli/src/commands/context.command.ts. The version constraint^3.2.0follows standard semver practices.apps/cli/src/commands/context.command.ts (5)
160-169: LGTM!The display logic correctly handles all combinations of briefName and briefId, showing a user-friendly format with the short ID in parentheses when both are available.
333-372: LGTM! Search implementation is well-structured.The search-based brief selection handles filtering by title, full ID, and short ID. The optional chaining
brief.document?.titlesafely handles missing document data, falling back to a formatted ID.Minor observation: The fallback
Brief ${brief.id.slice(0, 8)}is consistently applied throughout the file (lines 362, 378, 527), ensuring uniform display of briefs without document metadata.
376-390: LGTM!The consistent derivation of
briefNamefrombrief.document?.titlewith a fallback to formatted ID ensures proper display across all flows. The success message on line 390 correctly reflects the selected brief's title.
526-527: Consistent brief name derivation.The derivation pattern
brief.document?.title || 'Brief ${brief.id.slice(0, 8)}'is consistently applied across all context-setting flows, ensuring uniform behavior.
723-768: LGTM! Well-structured post-auth context setup flow.The
setupContextInteractivemethod provides a clear, optional workflow for context setup after authentication. Key strengths:
- User can opt out via confirmation prompt
- Returns detailed status flags for caller
- Graceful error handling with user-friendly fallback message
- Appropriate integration with existing
selectOrganization()andselectBrief()methodsThe error handling on line 760-767 ensures that context setup failures don't break the authentication flow, which is the correct behavior since this is a post-auth enhancement.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/cli/src/commands/auth.command.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cli/src/commands/auth.command.ts (1)
apps/cli/src/commands/context.command.ts (1)
ContextCommand(32-785)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (1)
apps/cli/src/commands/auth.command.ts (1)
17-17: LGTM!The import statement is correctly formatted and consistent with the existing ES module patterns.
Crunchyman-ralph
force-pushed
the
ralph/feat/add.prod.hamster
branch
from
2ef678f to
2100fe4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/cli/src/commands/auth.command.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cli/src/commands/auth.command.ts (1)
apps/cli/src/commands/context.command.ts (1)
ContextCommand(32-785)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (1)
apps/cli/src/commands/auth.command.ts (1)
17-17: LGTM!The import is correctly placed and necessary for the post-auth context setup workflow.
Crunchyman-ralph
force-pushed
the
ralph/feat/add.prod.hamster
branch
from
2100fe4 to
6596fed
Compare
Crunchyman-ralph
force-pushed
the
ralph/feat/add.prod.hamster
branch
from
6596fed to
22a6c79
Compare
1 participant
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ralph Khreish 35776126+Crunchyman-ralph@users.noreply.github.com# What type of PR is this?
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chore