Skip to content

Conversation

Ericode254
Copy link

  • fetch issues from all repos
  • use keymaps from bubbletea

…f a particular repo

- Will enable a user to configure github issues section using a personal
access token from github for authentication
- Users will be able to see issues on a particular repo of their
choosing and be also be able to create notes on them
- You can filter and sort the notes
- Their is an overlay for help
- Remove some comments where the code is self explanatory
- Update the readme with the github setup instructions
- Reverted the colors to how the were
- Removed the convert to note functionality
- Add an overlay for each note for full information
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

viewer.go should not have any changes since it is unrelated to your PR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you paste the diff please bacause i dont remember changing this file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk how I can, you can check the diff in the PR (files changed) section
I cant seem to attach it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diary.go should have no changes, its unrelated to your PR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you look at the file you will see i did not change the file i had accidentaly deleted a line then i pasted it at the top

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the style is changes, form FocusedBorder -> Border

Comment on lines 276 to 280
case enums.GitHubPage:
if m.GitHub != nil {
return m.GitHub.View()
}
return "GitHub model not initialized"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not keep it nil, it leaves room for error
Make sure its always initialized, even though it may hold no values in it, it should be initialized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure its always initialized, add Github model initialization to the
tea.WindowSizeMsg case in Update()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so want it not to be optional anymore but a requirement for users to setup the github issues

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, if it is not setup then no worries, just show "Github Not Setup" or something in the page. Just dont hide it if not setup. People should know it exists

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, if the nil case is handled in root.go properly
Github entry can be in Selection always.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would make it not optional anymore though or is that the point

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i understand now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big file, Ill check it once other stuff is completed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big file Ill check once other stuff is compelted

Comment on lines 66 to 71
func (api *GitHubAPI) FetchIssues() ([]GitHubIssue, error) {
if api.token == "" {
return nil, fmt.Errorf("GitHub token is missing")
}
return api.FetchAllIssuesForUser()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant function

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry i thought it would be easier but was in a harry didnt see the redundancy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big file, Ill check it once other stuff is completed

cmd/github.go Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big file, Ill check it once other stuff is completed

@NucleoFusion
Copy link
Member

Remember to keep resolving each review comment, so you and I both can keep a better track at whats done or is left

@NucleoFusion
Copy link
Member

Okay, so I was thinking, and rather than github issues being a seperate thing, I want it to be kind of like a tab inside the Daily Tasks section. So maybe you get the logic sorted whilst I also work at revamping the Daily Tasks section so it can fit the use case

@NucleoFusion
Copy link
Member

Are you still gonna work on it?
I am making some changes to the tasks section, so You will have to accomadate them

@Ericode254
Copy link
Author

yeah am gonna work on it, just been a little busy

@NucleoFusion
Copy link
Member

I added changes to the Daily Tasks Section. Now we have tabs inside the Daily Tasks Page, so instead of a seperate Github Page, add it there.
In All -> Show all Tasks (incl Github Ones, custom style)
In Github -> Show Github Tasks (Filter in the daily.go section, In Refresh())

@Ericode254
Copy link
Author

okay

All TaskTabs = "All"
Tasks TaskTabs = "Tasks"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont change what is not needed
This is a good change, but for this it should be a separate PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry about that will change it back

Comment on lines +51 to +55
key.WithHelp("right", "tasks/github tab"),
),
TabLeft: key.NewBinding(
key.WithKeys("left"),
key.WithHelp("left", "left tab"),
key.WithHelp("left", "tasks/github tab"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep left/right tab
This is a circularly linked, so that doesnt make sense

Comment on lines +22 to +26
type GitHubSyncMsg struct {
Issues []github.GitHubIssue
Error error
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

messages in messages package

Comment on lines -51 to +63
Tabs: []enums.TaskTabs{enums.All, enums.Unique, enums.Recurring, enums.Github},
Tabs: []enums.TaskTabs{enums.Tasks, enums.Github},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? Why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry forgot to remove them

Comment on lines +128 to +132
// Check if selected item is a GitHub task
item := m.List.SelectedItem()
if _, isGithubTask := item.(GithubTask); isGithubTask {
return m, nil // Don't allow deletion of GitHub tasks
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Allow for all, If user deletes, its up to him

Comment on lines 315 to 340
func convertGitHubIssuestoGithubTasks(issues []github.GitHubIssue) []GithubTask {
tasks := make([]GithubTask, len(issues))
for i, issue := range issues {
// Extract owner and repo from the repo field (format: "owner/repo")
repoParts := strings.Split(issue.Repo, "/")
owner := ""
repo := ""
if len(repoParts) == 2 {
owner = repoParts[0]
repo = repoParts[1]
}

tasks[i] = GithubTask{
TaskTitle: issue.IssueTitle,
TaskDesc: issue.Body,
Status: enums.Pending, // GitHub issues are typically "pending" in task context
Ref: fmt.Sprintf("#%d", issue.Number),
Repo: repo,
Owner: owner,
Link: issue.HTMLURL,
Labels: convertLabelsToStrings(issue.Labels),
Assignee: issue.Assignees,
}
}
return tasks
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Shouldnt be here, attach it to the GithubTask type in the items.go file

Comment on lines 52 to 54
lst1 := TaskToItems(m.Recurring)
lst2 := TaskToItems(m.Unique)
lst1 := RecurringTaskToItems(m.Recurring)
lst2 := UniqueTaskToItems(m.Unique)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Different functions?
Excess code not needed. Both are of same type, one function can be used

Comment on lines 77 to 85
func UniqueTaskToItems(tasks []Task) []list.Item {
list := make([]list.Item, 0)
for i, v := range tasks {
v.Index = i
v.TaskType = enums.UniqueTask
list = append(list, v)
}
return list
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary

}
} else if gt, ok := item.(GithubTask); ok {
// Handle GitHub Task with special styling
githubIcon := "🐙" // GitHub icon
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats not a github icon

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will just add text

Comment on lines +57 to 89
repoInfo := fmt.Sprintf("[%s/%s%s]", gt.Owner, gt.Repo, gt.Ref)

switch gt.Status {
case enums.Complete:
text += fmt.Sprintf("%s\n%s",
styles.CompletedStyle().Title.Render(fmt.Sprintf("%s | %s %s", cfg.CompletedIcon, Shorten(gt.Title(), 25), repoInfo)),
styles.CompletedStyle().Desc.Render(fmt.Sprintf("%s %s", githubIcon, Shorten(gt.Description(), 50))),
)
case enums.Pending:
text += fmt.Sprintf("%s\n%s",
styles.PendingStyle().Title.Render(fmt.Sprintf("%s | %s %s", cfg.PendingIcon, Shorten(gt.Title(), 25), repoInfo)),
styles.PendingStyle().Desc.Render(fmt.Sprintf("%s %s", githubIcon, Shorten(gt.Description(), 50))),
)
case enums.Started:
text += fmt.Sprintf("%s\n%s",
styles.StartedStyle().Title.Render(fmt.Sprintf("%s | %s %s", cfg.StartedIcon, Shorten(gt.Title(), 25), repoInfo)),
styles.StartedStyle().Desc.Render(fmt.Sprintf("%s %s", githubIcon, Shorten(gt.Description(), 50))),
)
case enums.Abandoned:
text += fmt.Sprintf("%s\n%s",
styles.AbandonedStyle().Title.Render(fmt.Sprintf("%s | %s %s", cfg.AbandonedIcon, Shorten(gt.Title(), 25), repoInfo)),
styles.AbandonedStyle().Desc.Render(fmt.Sprintf("%s %s", githubIcon, Shorten(gt.Description(), 50))),
)
default:
// Default to pending style for GitHub issues (since they're typically open)
text += fmt.Sprintf("%s\n%s",
styles.PendingStyle().Title.Render(fmt.Sprintf("%s | %s %s", cfg.PendingIcon, Shorten(gt.Title(), 25), repoInfo)),
styles.PendingStyle().Desc.Render(fmt.Sprintf("%s %s", githubIcon, Shorten(gt.Description(), 50))),
)
}
} else {
return
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets Discuss How we want it to look, the Github Task formats

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you suggest

@NucleoFusion
Copy link
Member

Okay so after you do changes, please run it and see if it works before putting a PR.
Also, again I am saying, do not do changes unrelated to your PR.
The Fetch Issues thing still remains, Assigned & Open Issues, not closed ones.

This is a big PR, this also paves the way for future Github Integration for everything to be done right, If you want to move to a simpler PR first, go for it, I will co-author the rest of the PR (your name will still be attached to the PR). If you do want to continue with this, then just comment here.

But please take my advice of checking your work before PR-ing, the Home is also still broken in this for me.

@Ericode254
Copy link
Author

can you explain how the home is broken for you because i dont remember touching it

@Ericode254
Copy link
Author

dont worry i have seen how the home page was broken, i think

- Fixed the broken viewport of the home page
- removed unnecessary code
Copy link
Member

@NucleoFusion NucleoFusion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bro you have changed everything, some previous changes are still not resolved and you have merged unique and recurring for some reason everywhere.
This is not how it works

At this point i cant even consider merging this branch, since it has too many changes and too many commits.
If you do want to continue with this, I would say delete this branch and start fresh with the PR.
If you want to look at how I have implemented TODO task integration, similar to the Github one, Ill put up a PR for it, its probably 200-300 lines of code, compared to your 1.6k lines. It will tell you how it should be done nicely.

Try to vibe less, the more AI you use the more it will mess up the repo. This is an OSS project, not a sideproject. It needs a certain level of quality for its code.

Comment on lines +48 to +56
Create: "c",
Edit: "e",
StatusChange: "s",
Delete: "d",
ExitPopup: "esc",
Enter: "enter",
FormUp: "ctrl+up",
FormDown: "ctrl+down",
BackToMenu: "esc",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed createRecurring?

Comment on lines +34 to +42
Create string `mapstructure:"create"`
Delete string `mapstructure:"delete"`
StatusChange string `mapstructure:"status_change"`
Edit string `mapstructure:"edit"`
BackToMenu string `mapstructure:"return_to_menu"`
FormUp string `mapstructure:"form_up"`
FormDown string `mapstructure:"form_down"`
ExitPopup string `mapstructure:"exit_popup"`
Enter string `mapstructure:"enter"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here
why are you changing the keybinds? that are irrelevant to your PR

Comment on lines 15 to 17
Tasks TaskTabs = "Tasks"
Unique TaskTabs = "Unique"
Recurring TaskTabs = "Recurring"
Github TaskTabs = "Github"
Tasks TaskTabs = "Tasks"
Github TaskTabs = "Github"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here
we want Unique and Recurring to be seperate
thats the whole point of the feature

Comment on lines 5 to +6
const (
CreateUnique = iota
CreateRecurring
CreateTask = iota
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, unique and recurring are combined

Comment on lines -4 to -8

const (
RecurringTask = iota
UniqueTask
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need i say something?

Comment on lines +11 to 19
CreateTask key.Binding
EditTask key.Binding
ChangeStatus key.Binding
DeleteTask key.Binding
BackToMenu key.Binding
TabRight key.Binding
TabLeft key.Binding
RefreshGithub key.Binding
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@@ -77,16 +76,14 @@ func (m *Daily) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
m.Tasks.Github = []GithubTask{}
} else {
m.GithubError = ""
m.Tasks.Github = convertGitHubIssuestoGithubTasks(msg.Issues)
m.Tasks.Github = ConvertGitHubIssuestoGithubTasks(msg.Issues)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i remember saying to attach this function to the Github Tasks type right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, the issues type

case enums.CreateRecurring:
fallthrough
case enums.CreateUnique:
m.CreateTask(msg, msg.Type == enums.CreateUnique)
case enums.CreateTask:
m.CreateTask(msg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change

Comment on lines -110 to +108
case key.Matches(msg, m.Keymap.CreateUnique):
m.Popup = taskpopup.NewPopup(m.Width, m.Height, enums.CreateUnique)
m.ShowPopup = true
return m, nil
case key.Matches(msg, m.Keymap.CreateRecurring):
m.Popup = taskpopup.NewPopup(m.Width, m.Height, enums.CreateRecurring)
case key.Matches(msg, m.Keymap.CreateTask):
m.Popup = taskpopup.NewPopup(m.Width, m.Height, enums.CreateTask)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change

Comment on lines -10 to 18
func (m Daily) CreateTask(msg messages.TaskPopupMessage, isUnique bool) {
func (m Daily) CreateTask(msg messages.TaskPopupMessage) {
task := Task{
TaskTitle: msg.Title,
TaskDesc: msg.Desc,
Status: msg.Status,
}

if isUnique {
m.Tasks.Unique = append(m.Tasks.Unique, task) // Seperate Task Input for recurring / unique tasks
} else {
m.Tasks.Recurring = append(m.Tasks.Recurring, task) // Seperate Task Input for recurring / unique tasks
}
m.Tasks.All = append(m.Tasks.All, task)
WriteItems(m.Tasks)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants